Add tunnel-ready gravity endpoint callback#241
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 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). (3)
🔇 Additional comments (7)
📝 WalkthroughWalkthroughAdds GravityConfig.OnEndpointTunnelReady and emits it asynchronously when endpoints become tunnel-ready (eligible for routing) during startup and reconnection. Implements tunnel-ready selection logic in the client and updates tests to validate tunnel-ready index selection and related health handling. Changes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Tools execution failed with the following error: Failed to run tools: 14 UNAVAILABLE: read ECONNRESET Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gravity/hardening_test.go (1)
1563-1565: Strengthen the new test by setting explicit pre-refresh health state.Set both endpoints to healthy before
refreshEndpointHealth()so the test verifies an actual transition to unhealthy for endpoint 0, not just default zero-value behavior.Suggested tweak
ep0 := &GravityEndpoint{URL: "grpc://10.0.0.1:443"} + ep0.healthy.Store(true) ep1 := &GravityEndpoint{URL: "grpc://10.0.0.2:443"} + ep1.healthy.Store(true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gravity/hardening_test.go` around lines 1563 - 1565, Test currently relies on zero-value health, so set explicit healthy state on both endpoints before calling refreshEndpointHealth(): locate the test's ep0 and ep1 GravityEndpoint instances and set their health field/property (e.g., Healthy = true or the appropriate health flag on GravityEndpoint) so refreshEndpointHealth() can actually flip ep0 to unhealthy and the assertion verifies a real transition; ensure you set the same initial healthy state on both ep0 and ep1 prior to invoking refreshEndpointHealth().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@gravity/hardening_test.go`:
- Around line 1563-1565: Test currently relies on zero-value health, so set
explicit healthy state on both endpoints before calling refreshEndpointHealth():
locate the test's ep0 and ep1 GravityEndpoint instances and set their health
field/property (e.g., Healthy = true or the appropriate health flag on
GravityEndpoint) so refreshEndpointHealth() can actually flip ep0 to unhealthy
and the assertion verifies a real transition; ensure you set the same initial
healthy state on both ep0 and ep1 prior to invoking refreshEndpointHealth().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a66992b5-4158-4dda-ac24-2adee959edf4
📒 Files selected for processing (4)
gravity/control_stream_resilience_test.gogravity/endpoint_independence_test.gogravity/grpc_client.gogravity/hardening_test.go
📜 Review details
🔇 Additional comments (4)
gravity/grpc_client.go (1)
1823-1846: LGTM — Correctly gates endpoint health on tunnel stream availability.The implementation properly derives endpoint health from two conditions:
- The gRPC connection is healthy (
connectionHealth[connIndex])- At least one healthy tunnel stream exists for that connection (
healthyTunnelByConn[connIndex])The snapshot-under-lock approach minimizes lock contention while maintaining consistency. The bounds check on line 1846 is sufficient since
healthyTunnelByConnis created withlen(connectionHealth), guaranteeing equal lengths.gravity/endpoint_independence_test.go (1)
2268-2271: Comment update is accurate and helpful.The revised note correctly reflects the new endpoint-health derivation contract and should reduce future test confusion.
gravity/control_stream_resilience_test.go (1)
7-7: Timing fix is reasonable for the handshake sequencing race.This change aligns the test with the drain/hello ordering and makes the tunnel-establishment assertions meaningful.
Also applies to: 239-239
gravity/hardening_test.go (1)
1480-1482: Great coverage update for the new health derivation rules.These tests now validate the connection-health + healthy-tunnel conjunction and close a key behavior gap.
Also applies to: 1501-1506, 1519-1520, 1543-1549, 1560-1594
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gravity/grpc_client.go`:
- Around line 2846-2860: The tunnelReadyEndpointIndices function only inspects
g.endpoints, so in single-URL mode where g.endpoints is empty it never reports
readiness; update tunnelReadyEndpointIndices to also check the single-endpoint
field g.endpoint (in addition to g.endpoints) and, when that endpoint is non-nil
and its healthy.Load() returns true, include it in the returned ready indices so
the startup/reconnect logic (and OnEndpointTunnelReady callbacks) see a ready
endpoint; make this change inside tunnelReadyEndpointIndices while preserving
the existing endpointsMu lock and the ready slice semantics so callers of
tunnelReadyEndpointIndices (and code that triggers OnEndpointTunnelReady)
receive a readiness entry for the single-endpoint client.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62a258d4-0e26-4f9e-860c-208003859c32
📒 Files selected for processing (3)
gravity/grpc_client.gogravity/hardening_test.gogravity/types.go
🚧 Files skipped from review as they are similar to previous changes (1)
- gravity/hardening_test.go
📜 Review details
🔇 Additional comments (2)
gravity/types.go (1)
89-101: Good callback-stage separation in config API.The split between
OnEndpointReady(control-plane readiness) andOnEndpointTunnelReady(routing readiness) is clear and improves integration safety for consumers.gravity/grpc_client.go (1)
2828-2844: Callback invocation wrapper looks robust.Async dispatch plus panic recovery around
OnEndpointTunnelReadyis a solid safety pattern for user-provided callbacks.
| func (g *GravityClient) tunnelReadyEndpointIndices() []int { | ||
| g.endpointsMu.RLock() | ||
| defer g.endpointsMu.RUnlock() | ||
|
|
||
| ready := make([]int, 0, len(g.endpoints)) | ||
| for i, ep := range g.endpoints { | ||
| if ep == nil { | ||
| continue | ||
| } | ||
| if ep.healthy.Load() { | ||
| ready = append(ready, i) | ||
| } | ||
| } | ||
| return ready | ||
| } |
There was a problem hiding this comment.
Single-endpoint mode currently suppresses OnEndpointTunnelReady.
Line 2847-Line 2859 only inspects g.endpoints. In the single-URL path, g.endpoints is empty, so startup callbacks at Line 757 and reconnection callback checks never see a ready endpoint. Result: OnEndpointTunnelReady does not fire for single-endpoint clients.
💡 Proposed fix
func (g *GravityClient) tunnelReadyEndpointIndices() []int {
g.endpointsMu.RLock()
+ if len(g.endpoints) == 0 {
+ g.endpointsMu.RUnlock()
+ // Single-endpoint mode fallback: endpoint index 0 is tunnel-ready
+ // when the client has a healthy control stream and at least one
+ // healthy tunnel stream.
+ if g.hasHealthyEndpoint() {
+ return []int{0}
+ }
+ return nil
+ }
defer g.endpointsMu.RUnlock()
ready := make([]int, 0, len(g.endpoints))
for i, ep := range g.endpoints {
if ep == nil {
continue
}
if ep.healthy.Load() {
ready = append(ready, i)
}
}
return ready
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gravity/grpc_client.go` around lines 2846 - 2860, The
tunnelReadyEndpointIndices function only inspects g.endpoints, so in single-URL
mode where g.endpoints is empty it never reports readiness; update
tunnelReadyEndpointIndices to also check the single-endpoint field g.endpoint
(in addition to g.endpoints) and, when that endpoint is non-nil and its
healthy.Load() returns true, include it in the returned ready indices so the
startup/reconnect logic (and OnEndpointTunnelReady callbacks) see a ready
endpoint; make this change inside tunnelReadyEndpointIndices while preserving
the existing endpointsMu lock and the ready slice semantics so callers of
tunnelReadyEndpointIndices (and code that triggers OnEndpointTunnelReady)
receive a readiness entry for the single-endpoint client.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gravity/grpc_client.go`:
- Around line 2867-2870: The code reads len(g.streamManager.connectionHealth)
before acquiring g.streamManager.healthMu, which can race with concurrent
appends; fix by acquiring the read lock (g.streamManager.healthMu.RLock())
before computing the length and allocating connectionHealth and keep the lock
held while calling copy(connectionHealth, g.streamManager.connectionHealth),
then RUnlock(); i.e., move the RLock() to precede the make(...) and perform
make/len/copy inside the locked region for g.streamManager.connectionHealth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b74facc7-fc2d-493e-ba01-9fce385dbb86
📒 Files selected for processing (2)
gravity/grpc_client.gogravity/hardening_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- gravity/hardening_test.go
📜 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). (1)
- GitHub Check: build
🔇 Additional comments (2)
gravity/grpc_client.go (2)
2828-2844: Panic-safe async callback wrapper looks good.
fireOnEndpointTunnelReadymirrors the existing callback safety pattern and prevents callback panics from destabilizing the client.
755-759: Good callback emission points for startup and endpoint reconnection.These hooks correctly gate
OnEndpointTunnelReadybehind derived readiness and cover both initial connect and reconnect paths.Also applies to: 1057-1059, 4563-4568
1 participant
Summary
Validation
Summary by CodeRabbit
New Features
Documentation
Tests