fix(orchestrator): bound Consul KV slot operations with a context and timeout - #3509
fix(orchestrator): bound Consul KV slot operations with a context and timeout#3509tomassrnka wants to merge 2 commits into
Conversation
PR SummaryHigh Risk Overview Reviewed by Cursor Bugbot for commit b0a2345. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
1849c40 to
a76c268
Compare
… timeout StorageKV.Release took no context and StorageKV.Acquire ignored the one it was given, so every Consul KV round trip was unbounded. The Consul API client builds its http.Client with neither a Timeout nor a ResponseHeaderTimeout, so an agent that accepts the connection but never answers blocks the caller forever. That stalls orchestrator shutdown: Pool.Close waits for in-flight returns and then drains every pooled slot through Storage.Release. Because the shutdown context is cancelled immediately when FORCE_STOP is set, even a forced stop could not cut these calls short. - Storage.Release now takes a context; StorageKV honors it on Get/DeleteCAS, and Acquire honors it on CAS/Keys, via Consul's Query/WriteOptions. - Every round trip is additionally bounded by a per-request timeout (5s), so a caller context without a deadline still cannot hang. The Consul client now also carries an explicit http.Client.Timeout as a backstop for any call site added later without a per-operation context. - Acquire checks for cancellation between retries instead of spending the remaining attempts on doomed round trips. - Pool.Close detaches its drain from the caller's context and bounds it with a 15s deadline instead of passing a possibly-cancelled context through. The template-manager runs the orchestrator binary with FORCE_STOP=true, which cancels the shutdown context up front, so passing it through would skip the release of every pooled slot. The keys are node-scoped and nothing reclaims them, so that would leak up to 131 slot indices per deploy. - The two pool paths that clean up with an already-dead context likewise detach via context.WithoutCancel, bounded by the per-request timeout.
a76c268 to
0fdba68
Compare
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
70949dd to
b0a2345
Compare
1 participant
Linear: https://linear.app/e2b/issue/EN-981/networkstoragekvrelease-makes-unbounded-consul-calls-without-context
Broken: a wedged Consul agent (accepts TCP, never answers) stalls orchestrator shutdown indefinitely —
Pool.Closedrains every pooled slot throughStorage.Release, and sinceFORCE_STOPcancels the shutdown context up-front, even a forced stop couldn't cut the calls short.Root cause:
Storage.Releasetook no context,StorageKV.Acquirenamed its context_, all four Consul KV calls passed nil options, andconsul/apibuilds itshttp.Clientwith noTimeoutorResponseHeaderTimeout— nothing bounded the round trip.Repro: against a black-hole HTTP server on unmodified main:
Releasestill blocked after 5 s,Releasewith an already-cancelled context still blocked,Acquire(cancelledCtx)still blocked.Fix:
Releasetakes a context, both methods plumb it through ConsulQuery/WriteOptions, every round trip gets a 5 s per-request timeout, the client gets anhttp.Client.Timeoutbackstop for future call sites, andAcquirechecks cancellation between retries.Pool.Closedetaches its drain from the caller's context and caps it at 15 s instead of passing a possibly-cancelled context through — the template-manager is the orchestrator binary withFORCE_STOP=trueand a Consul-backed pool, so passing the cancelled context would skip releasing up to 131 node-scoped keys per deploy that nothing reclaims (ReclaimLeakedSlotsonly handles netns entries). 15 s sits inside the shared 1 mkill_timeout(~26 closers, leaves 45 s); a healthy drain finishes well under a second, so the cap only bites when Consul is unresponsive.Alternative considered: #3515 fixes the same ticket without changing the
Storageinterface (context.Background()+ 10 s insideRelease, plus the same client timeout) — avoids the key leak but leaves the drain stall:Releaseis uncancellable and the drain is serial, so a wedged agent costs up to 131 x 10 s (~22 min), far past the 1 mkill_timeout— the exact symptom EN-981 reports. This PR threads the context and bounds the whole drain once, getting both properties.Verification: new
storage_kv_test.gocovers cancelled-context, per-request timeout, client backstop, and happy paths against a fake Consul (it doesn't compile against main — the missing context parameter is the bug).TestClose_ReleasesPooledSlotsWithCanceledContextassertsStorage.Releasegets a live context during forced shutdown and fails against the interface-only version.go build,go vet,go test -race, golangci-lint v2.11.4 all clean.