Skip to content

Keep gravity reconnects on discovered endpoints#238

Merged
jhaynie merged 1 commit intomainfrom
fix/gravity-reconnect-discovered-endpoints-clean
Apr 28, 2026
Merged

Keep gravity reconnects on discovered endpoints#238
jhaynie merged 1 commit intomainfrom
fix/gravity-reconnect-discovered-endpoints-clean

Conversation

@jhaynie
Copy link
Copy Markdown
Member

@jhaynie jhaynie commented Apr 28, 2026

Summary

  • keep multi-endpoint Gravity reconnects on the discovered direct endpoint set
  • avoid collapsing direct per-Ion endpoints back to the shared bootstrap/NLB address during reconnect
  • add a regression test covering the direct-endpoint-to-bootstrap collapse case

Validation

  • go test ./gravity -run 'TestReResolveEndpointURL_PrefersDiscoveredDirectEndpointsOverBootstrapDNS|TestReResolveEndpointURL_'
  • go test ./gravity

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced endpoint resolution logic to prioritize direct discovered endpoints during failover in multi-endpoint configurations, improving system reliability and reducing unnecessary DNS re-resolution attempts.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 28, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f1af9150-b5d2-4e21-941a-041a7ce59c54

📥 Commits

Reviewing files that changed from the base of the PR and between 6638d0e and 9cff982.

📒 Files selected for processing (2)
  • gravity/grpc_client.go
  • gravity/reresolve_test.go
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build
  • GitHub Check: Analyze (go)
🔇 Additional comments (3)
gravity/reresolve_test.go (1)

199-232: LGTM! Well-designed regression test.

The test correctly validates the new behavior where reResolveEndpointURL avoids collapsing direct per-Ion endpoints back to the shared bootstrap/NLB address. The assertions comprehensively verify:

  1. No URL rewrite occurs (newURL == "").
  2. The endpoint URL remains unchanged on the direct Ion address.
  3. DNS re-resolution is skipped (mock.callCount() == 0).

The mock setup accurately represents the real-world scenario of multiple direct endpoints sharing a bootstrap TLSServerName.

gravity/grpc_client.go (2)

4000-4009: LGTM! Correct integration of discovered endpoint preference.

The new block properly gates the discovered-set lookup behind both multiEndpointMode and discoveryResolveFunc != nil checks. The handled return value correctly suppresses the DNS fallback path when the current endpoint is already part of the discovered direct set.


4095-4149: LGTM! Clean implementation of discovered-set preference logic.

The helper method correctly:

  1. Prioritizes switching to an alternative discovered endpoint not already in use by sibling endpoints.
  2. Suppresses DNS fallback when the current endpoint is already in the discovered direct set (preventing collapse to shared bootstrap/NLB).
  3. Falls through to DNS resolution only when the current endpoint is not part of the discovered set.

The lock handling around the endpoint URL update (lines 4133-4138) is correct, and the inUse host comparison is consistent with the caller's construction of the map.


📝 Walkthrough

Walkthrough

Updates multi-endpoint resolution flow in the gRPC client to prioritize switching to alternative endpoints from DNS discovery before re-resolving TLS server names. A new private helper method reResolveFromDiscoveredSet is added alongside corresponding unit tests validating the behavior.

Changes

Cohort / File(s) Summary
Endpoint re-resolution logic
gravity/grpc_client.go, gravity/reresolve_test.go
Introduces new reResolveFromDiscoveredSet helper to attempt switching to alternative discovered endpoints before triggering DNS re-resolution. Helper fetches discovered URLs and replaces endpoint if possible with a unique alternative. Test validates control flow behavior when discovered endpoints cover the endpoint pool and ensures DNS resolution is not triggered unnecessarily.
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@jhaynie jhaynie merged commit 2eaf46a into main Apr 28, 2026
5 checks passed
@jhaynie jhaynie deleted the fix/gravity-reconnect-discovered-endpoints-clean branch April 28, 2026 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant