fix: drain tunnel gateway routes and sessions on shutdown - #5935
Conversation
|
|
Running ultrareview automatically — This drain-on-shutdown PR adds substantial concurrency (publisher waits, bounded cleanup, registry drain state) across gateway and registry; a subtle race could stall shutdown or leak stale routes.. I'll post findings when complete. |
There was a problem hiding this comment.
Ultrareview completed in 28m 28s
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
bflad
left a comment
There was a problem hiding this comment.
Approving, the drain sequencing and the test coverage here are solid and this should eliminate the deploy 502 bursts in the common case. I left inline comments on findings worth discussing: three edge cases that can undermine the drain (a late publish landing after the sweep, one 25s budget shared by all shutdown phases, and /healthz staying green while draining) and three smaller ones (retained ownership never retried, cleanupSessionState budget growth plus drain redundancy, and the MarkConnected drain re-check being best-effort). None of these block merging from my side and they could all be followups.
4af0381 to
a7a106a
Compare
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Fix all with cubic | Re-trigger cubic
a7a106a to
2f176a4
Compare
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
2f176a4 to
c943cd7
Compare
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
c943cd7 to
c67226b
Compare
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Summary
Introduces a single-writer
routeReconciler: one per-pod goroutine owns every mutatingroute-store call (publish, connection snapshots, unpublish, global revoke). Handlers mark a
tunnel dirty and nudge; the reconciler reads live registry state at write time, batch-refreshes
all live tunnels on one ticker (replacing the per-session refresher goroutines), gates
publishes on
IsActive, and prunes tracking for cleanly-removed tunnels.Because all writes are serialized through one goroutine, graceful shutdown becomes the
reconciler's final act: on SIGTERM the registry stops admitting agent connects (checked again
under the lock at registration), the reconciler performs one bounded batched cleanup of every
route this pod may own, the forward server drains in-flight requests, agent sessions close
concurrently, and
mainblocks until the sequence completes — all within the existing 25sbudget. Publish-after-cleanup races are impossible by construction rather than coordinated
away.
Tests are black-box at the gateway boundary (real WebSocket agent + yamux sessions against an
in-memory store): post-drain store emptiness with no writes after drain returns, 503 on
connect during drain, in-flight forwards completing through shutdown, TTL refresh cadence,
revocation across multiple owners, and a concurrent-churn convergence property under the race
detector.
Motivation
The gateway exited without unpublishing its routes, so every rollout left pod-IP routes in
Redis for up to the 30s TTL while the pod was already gone. gram-server dialed the dead IP and
each affected tunneled request hung ~30s before surfacing a 502, producing an error burst on
every deploy. Draining removes the stale-route window; agents re-home to surviving pods in
~0.5–2s using their existing reconnect backoff. Complements #5888 (fast dial timeout) and
#5930 (dead-route eviction on dial failure), which cover unplanned pod death.
closes AIM-166