fix: cancellation/teardown hardening sweep#131
Merged
Conversation
Four related hardening items from an adversarial re-review of the shutdown/teardown path. Conservative throughout — a teardown that hangs is worse than the issues fixed, so every new wait is bounded. S4 — wait for background goroutines on shutdown: Add App.bgWG. The projector, consumer, changes/broadcast tailers and the three TTL sweepers each Add(1) at spawn + defer Done. Shutdown waits on bgWG AFTER backplaneCancel + backplane.Close + closing stopSweep (so the goroutines can observe the stop and exit first), and the wait is bounded by the shutdown ctx: a goroutine wedged on a backend that ignores cancellation cannot hang the drain past its deadline. S3 — prompt Subscribe teardown regardless of backend: The projector/consumer/changes/broadcast loops now select on the record channel AND backplaneDone, so teardown is prompt even against a backend that does not close the channel on ctx cancel. Reconnect-on-transient- close semantics are unchanged (a real channel close still re-subscribes). S10 — ctx-aware CAS backoff: casSleep now takes a context and selects on ctx.Done, so a contended StateApp/StateSess.Update mid-retry aborts the backoff the instant the backplane context is cancelled instead of adding up to the cap to the drain. Threaded app.backplaneCtx from both Update call sites. Theme-1 — surface swallowed backplane errors: WARN-log (greppable "via: backplane …" prefix) the previously dropped errors: backplane.Close in Shutdown; Subscribe/Head failure before a tailer goroutine returns; the broadcast Append error; the reconcile/ applyChange LoadSnapshot errors. Control flow is unchanged — the ok=false non-error case still does not log. Operators now see the cause of state divergence instead of only the symptom. Tested: casSleep prompt-return on cancelled ctx (internal test); Shutdown does not hang on a non-closing Subscribe channel and returns before its deadline; a failing-Subscribe stub surfaces the theme-1 WARN. Existing backplane-context cancellation tests and the full -race suite stay green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four related hardening items from an adversarial re-review of the shutdown/teardown path. Conservative throughout — a teardown that hangs is worse than the issues fixed, so every new wait is bounded and the cancel/close happens before the wait (no new deadlock).
Items
S4 — wait for background goroutines on shutdown. New
App.bgWG. The projector, consumer, changes/broadcast tailers and the three TTL sweepers eachbgWG.Add(1)at spawn +defer bgWG.Done().Shutdownwaits onbgWGafterbackplaneCancel()+backplane.Close()+ closingstopSweep(so the goroutines observe the stop and exit), and the wait is bounded by the shutdown ctx via adone-chan +select { case <-done: case <-ctx.Done(): }. A goroutine wedged on a backend that ignores cancellation cannot hang the drain past its deadline — a leaked goroutine is strictly better than a hung shutdown.S3 — prompt Subscribe teardown regardless of backend. The projector/consumer/changes/broadcast loops now
selecton the record channel andbackplaneDone, so teardown is prompt even against a backend that doesn't close the channel on ctx cancel. Reconnect-on-transient-close semantics are unchanged (a real channel close still re-subscribes from the cursor).S10 — ctx-aware CAS backoff.
casSleepnow takes acontext.Contextandselects onctx.Done(), so a contendedStateApp/StateSess.Updatemid-retry aborts the backoff the instant the backplane ctx is cancelled instead of adding up to ~10ms to the drain. Threadedapp.backplaneCtxfrom both Update call sites.Theme-1 — surface swallowed backplane errors. WARN-log (greppable
via: backplane …prefix) the previously dropped errors:backplane.Closein Shutdown; Subscribe/Head failure before a tailer goroutine returns; the broadcastAppenderror; the reconcile/applyChangeLoadSnapshoterrors in appval.go. Control flow unchanged; theok=falsenon-error case still does not log.Tested vs. couldn't
Covered by tests:
casSleepreturns promptly when the ctx is already cancelled (internal test); existing within-ceiling bound test updated to the new signature.Shutdowndoes not hang on a backend whose Subscribe channel never closes, and returns before its own deadline (proves the bounded wait + the S3 ctx.Done wake).TestShutdownCancelsInflight…,…WritePathContext) stay green.Implemented but not directly unit-tested (asserted via the full -race suite + existing conformance/backplanetest, not a bespoke test): the per-loop
backplaneDonewake on the consumer/changes/broadcast tailers in isolation, and each individual LoadSnapshot/Append/Close WARN site beyond the projector-subscribe one. These are log-only / control-flow-preserving changes verified by the existing suite staying green.No deadlock + bounded shutdown
backplaneCancel()+Close()+close(stopSweep)all run beforebgWG.Wait(), so every tracked goroutine can observe its stop signal first; the wait itself is wrapped in a ctx-boundedselect. VerifiedShutdown(context.Background())(no deadline) still completes because all goroutines exit onbackplaneDone/stopSweep.CI
./ci-check.shgreen: gofmt, vet, golangci-lint, govulncheck, API-compat baselines,-raceacross all packages, and the allocation gates (CounterRender 183/185, Action 137/149, ActionWithLogger 137/149 — unchanged).