fix(gravity): bound tunnel sends and isolate stalled endpoints - #319
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesThe client now sends tunnel packets and keepalives through per-stream bounded queues, quarantines endpoints whose sends time out, cancels contexts before shutdown waits, and adds tests for blocked sends, endpoint isolation, reconnection, and handler cancellation. Tunnel send reliability
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
gravity/grpc_client.go (1)
5885-5891: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftPer-packet copy adds an allocation to the outbound hot path.
The copy is only needed because the send may outlive the caller.
WritePacket/sendTunnelPacketare per-TUN-packet, so this is an extra alloc + memcpy for every packet in the steady state, on top of the channel handoff. Consider sourcing the copy fromg.bufferPool, or passing ownership explicitly so callers that already own a private buffer skip the copy.🤖 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/grpc_client.go` around lines 5885 - 5891, Update the per-packet ownership handling in WritePacket/sendTunnelPacket to avoid allocating and copying a new Data slice on every outbound packet. Reuse g.bufferPool for the required copy, or propagate explicit buffer ownership so privately owned caller buffers bypass it, while preserving immutable ownership for sends that can outlive the caller.
🤖 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.
Inline comments:
In `@gravity/grpc_client.go`:
- Around line 1453-1454: Update the comment near the send path to describe
serialization through the per-stream sendQueue worker instead of the removed
sendMu mutex, while retaining the note that timed-out sends quarantine and
reconnect only the affected endpoint.
- Around line 5892-5916: Update sendTunnelStreamBounded to detect a cancelled or
exited stream before attempting the unbuffered stream.sendQueue handoff,
returning ErrConnectionClosed immediately instead of waiting for the timeout.
Ensure the check covers the stream context and worker lifecycle so sendOnce
cannot leave later sends blocked, while preserving the existing timeout behavior
for an active worker.
---
Nitpick comments:
In `@gravity/grpc_client.go`:
- Around line 5885-5891: Update the per-packet ownership handling in
WritePacket/sendTunnelPacket to avoid allocating and copying a new Data slice on
every outbound packet. Reuse g.bufferPool for the required copy, or propagate
explicit buffer ownership so privately owned caller buffers bypass it, while
preserving immutable ownership for sends that can outlive the caller.
🪄 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: 18bb649b-1a9d-4c00-a257-b255cb08f80f
📒 Files selected for processing (2)
gravity/grpc_client.gogravity/tunnel_dataplane_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: build
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
gravity/grpc_client.go (7)
66-68: LGTM!Also applies to: 302-302, 436-448, 582-582
3914-3929: LGTM!
4129-4133: LGTM!
5919-5931: LGTM!
5933-5948: LGTM!
6011-6013: LGTM!
6279-6306: 🎯 Functional CorrectnessTunnel health is tracked separately; this isn’t a circuit-breaker bug. Tunnel sends update
StreamInfoand endpoint health on failure, so theg.circuitBreakersbounds check is just defensive and doesn’t leave breaker state stale.> Likely an incorrect or invalid review comment.gravity/tunnel_dataplane_test.go (1)
62-63: LGTM!Also applies to: 79-99, 238-288, 290-323, 325-343
1 participant
Summary
Sendcannot indefinitely freeze Hadron TUN packet processing or create per-packet goroutinesRCA
US Central repeatedly entered a state where Hadron control sessions, resource sync, readiness, and PSC health remained green while deployment VIP probes timed out and Ions suppressed routes. The failing VIPs moved among Hadrons and Ions, and restarting a Hadron process temporarily restored traffic.
Hadron's outbound dataplane synchronously called the bidirectional gRPC stream's
Send. That API has no per-call context. If HTTP/2 flow control or a half-open stream wedged the call, the shared outbound TUN processing path could stop making progress indefinitely while control-plane health continued to look healthy. Keepalive sends detected a block but previously only logged it; they did not quarantine or reconnect the endpoint.This change gives packet and keepalive sends one bounded path. When its deadline expires, it marks the stream and endpoint unhealthy, cancels that endpoint's streams through the existing endpoint disconnect path, and reconnects only that endpoint. Healthy sibling endpoints continue serving.
Tests
go test -race ./gravity -count=1go test ./...git diff --checkFocused regression coverage verifies:
Rollout / canary plan
go-commonversion separately.hadron-pmxhdgcl.edge.agentcompany.com.Operational note
This PR does not tag or release
go-common, update Hadron dependencies, or deploy any production component.Summary by CodeRabbit