fix(gravity): grow tunnel slots for discovered endpoints#247
Conversation
📝 WalkthroughWalkthroughIntroduces per-endpoint tunnel stream slot preallocation via a new ChangesTunnel Stream Slot Preallocation
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gravity/endpoint_independence_test.go (1)
827-828: 🏗️ Heavy liftExercise the reconnect path, not only the helper.
Line 828 proves
ensureTunnelStreamSlotsitself resizes correctly, but the production regression was the missing call from the reconnect/stream-establish path. If that call is removed or skipped later, this test still passes. Please add one assertion throughestablishTunnelStreams/the reconnect flow so the regression test protects the call site too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gravity/endpoint_independence_test.go` around lines 827 - 828, The test currently only verifies ensureTunnelStreamSlots directly; change it to exercise the reconnect/stream-establish path by invoking the actual reconnect/establishment flow (call establishTunnelStreams or the function that triggers reconnect behavior) and assert the pool's stream slot count after that call; specifically, ensure the test calls establishTunnelStreams (or the reconnect entrypoint used in production) after setting streamsPerGravity and still observes the resized slots from ensureTunnelStreamSlots, so the regression where the helper is skipped during reconnect is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@gravity/endpoint_independence_test.go`:
- Around line 827-828: The test currently only verifies ensureTunnelStreamSlots
directly; change it to exercise the reconnect/stream-establish path by invoking
the actual reconnect/establishment flow (call establishTunnelStreams or the
function that triggers reconnect behavior) and assert the pool's stream slot
count after that call; specifically, ensure the test calls
establishTunnelStreams (or the reconnect entrypoint used in production) after
setting streamsPerGravity and still observes the resized slots from
ensureTunnelStreamSlots, so the regression where the helper is skipped during
reconnect is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d98a7aaf-266a-4bab-9176-514514531913
📒 Files selected for processing (2)
gravity/endpoint_independence_test.gogravity/grpc_client.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). (2)
- GitHub Check: build
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
gravity/grpc_client.go (2)
2030-2043: Tunnel slot growth logic is correct and state-preserving.This helper safely grows
tunnelStreamsunder lock and preserves existing entries while appending missing slots, which matches the reconnect safety requirement for higher endpoint indexes.
4788-4789: Preallocation is correctly wired before tunnel offset indexing.Calling
ensureTunnelStreamSlotsbefore computing/usingtunnelOffsetcloses the out-of-bounds path during endpoint reconnect for newly discovered indexes.
Summary
Validation
Production evidence
During us-west Ion rollout, Hadrons cycled control to the new Ion and fired OnEndpointReady, but Ion saw 0 StreamSessionPackets and timed out forwarders with no tunnel streams. This fixes the missing tunnel slot path that prevented reconnectSingleEndpoint from opening streams for endpoint indexes beyond the initial tunnelStreams length.
Summary by CodeRabbit
Bug Fixes
Tests