fix(messagequeue): bound a subscription to the caller's context - #628
Open
behinddwalls wants to merge 2 commits into
Open
fix(messagequeue): bound a subscription to the caller's context#628behinddwalls wants to merge 2 commits into
behinddwalls wants to merge 2 commits into
Conversation
## Summary ### Why? The `Subscriber` interface documents that the delivery channel closes when the subscriber is closed *or the context is cancelled*. The MySQL subscriber — the only implementation — honoured just the first half: `Subscribe`'s `ctx` appeared solely in the signature and was never read, and the subscription instead ran on an independent `context.Background()` that only `Close` could cancel. That inert parameter is what let the package's own test leak. `TestSubscriber_Subscribe` called `Subscribe` with no paired `Close`, leaving a `managePartitions` supervisor per topic running on a one-second discovery ticker. Under the race detector the suite runs slowly enough for a tick to land after the subtest has returned, calling `GetLeasedPartitions` against a finished gomock controller while `zaptest` logs to a completed `*testing.T`. Without `-race` the suite finishes inside a second and the tick never fires, so the failure surfaces only under `--@rules_go//go/config:race` — which CI does not run. ### What? `Subscribe` derives the subscription context from the caller's `ctx`. `Close` still cancels that context directly, so a caller whose `ctx` never completes is unaffected. Honouring `ctx` opens a hazard that could not previously exist: cancellation ends the supervisor and closes `deliveryCh`, but leaves the entry in the subscriptions map, so the next `Subscribe` on that key would hand a new caller an already-closed channel. Each subscription now carries a `done` channel, closed by the supervisor on its way out; `Subscribe` treats an entry whose `done` is closed as absent and replaces it. That signal has to be a channel rather than a lock-guarded flag, because `Close` holds `subMu` across `cancelFunc` and `wg.Wait` for every subscription — acquiring the same mutex on the shutdown path would deadlock against the very `Close` that triggered it. The consumer now hands `Subscribe` a detached context. Every service starts its consumers with a SIGTERM-cancelled context, and feeding that straight through would close the delivery channel the moment shutdown began, cutting the consume loop's drain short. The consume loop already ran detached for exactly that reason, so the two now share one context. Production shutdown behaviour is unchanged. `TestSubscriber_Subscribe` now closes its subscriber, and two new tests cover the semantics: cancelling the context closes the delivery channel, and a stale entry is replaced rather than reused. `make test-race` runs the unit suite under the detector. It needs `--build_tests_only` — the `//...` pattern otherwise pulls in the cross-compiled `*_linux` binaries, which are built without cgo, and race instrumentation requires cgo. ## Test Plan Reproduced first: three of five runs of the filtered test failed with `WARNING: DATA RACE`, the reader being the `managePartitions` goroutine reaching `GetLeasedPartitions` while the subtest goroutine had already finished. ✅ 10/10 runs clean under `--@rules_go//go/config:race` for both `//platform/extension/messagequeue/mysql` and `//platform/consumer`; confirmed with `-test.v` that the new tests actually ran rather than silently matching nothing. ✅ `make test-race` — 109/109 targets pass repo-wide, so no other package carried a latent leak. ✅ `make test` (109/109), gazelle produced no BUILD changes, license headers clean, mocks unchanged. ✅ Integration, uncached, covering the real subscribe/close and shutdown-ordering paths: `//test/integration/submitqueue/core/consumer` and `//test/integration/extension/messagequeue/mysql`. Checked that the new guard is meaningful: with the stale-entry eviction removed, `TestSubscriber_SubscribeReplacesStaleSubscription` fails on both assertions and passes again once restored.
behinddwalls
marked this pull request as ready for review
August 21, 2026 17:51
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.
Summary
Why?
The
Subscriberinterface documents that the delivery channel closes when the subscriber is closed or the context is cancelled. The MySQL subscriber — the only implementation — honoured just the first half:Subscribe'sctxappeared solely in the signature and was never read, and the subscription instead ran on an independentcontext.Background()that onlyClosecould cancel.That inert parameter is what let the package's own test leak.
TestSubscriber_SubscribecalledSubscribewith no pairedClose, leaving amanagePartitionssupervisor per topic running on a one-second discovery ticker. Under the race detector the suite runs slowly enough for a tick to land after the subtest has returned, callingGetLeasedPartitionsagainst a finished gomock controller whilezaptestlogs to a completed*testing.T. Without-racethe suite finishes inside a second and the tick never fires, so the failure surfaces only under--@rules_go//go/config:race— which CI does not run.What?
Subscribederives the subscription context from the caller'sctx.Closestill cancels that context directly, so a caller whosectxnever completes is unaffected.Honouring
ctxopens a hazard that could not previously exist: cancellation ends the supervisor and closesdeliveryCh, but leaves the entry in the subscriptions map, so the nextSubscribeon that key would hand a new caller an already-closed channel. Each subscription now carries adonechannel, closed by the supervisor on its way out;Subscribetreats an entry whosedoneis closed as absent and replaces it. That signal has to be a channel rather than a lock-guarded flag, becauseCloseholdssubMuacrosscancelFuncandwg.Waitfor every subscription — acquiring the same mutex on the shutdown path would deadlock against the veryClosethat triggered it.The consumer now hands
Subscribea detached context. Every service starts its consumers with a SIGTERM-cancelled context, and feeding that straight through would close the delivery channel the moment shutdown began, cutting the consume loop's drain short. The consume loop already ran detached for exactly that reason, so the two now share one context. Production shutdown behaviour is unchanged.TestSubscriber_Subscribenow closes its subscriber, and two new tests cover the semantics: cancelling the context closes the delivery channel, and a stale entry is replaced rather than reused.make test-raceruns the unit suite under the detector. It needs--build_tests_only— the//...pattern otherwise pulls in the cross-compiled*_linuxbinaries, which are built without cgo, and race instrumentation requires cgo.Test Plan
Reproduced first: three of five runs of the filtered test failed with
WARNING: DATA RACE, the reader being themanagePartitionsgoroutine reachingGetLeasedPartitionswhile the subtest goroutine had already finished.✅ 10/10 runs clean under
--@rules_go//go/config:racefor both//platform/extension/messagequeue/mysqland//platform/consumer; confirmed with-test.vthat the new tests actually ran rather than silently matching nothing.✅
make test-race— 109/109 targets pass repo-wide, so no other package carried a latent leak.✅
make test(109/109), gazelle produced no BUILD changes, license headers clean, mocks unchanged.✅ Integration, uncached, covering the real subscribe/close and shutdown-ordering paths:
//test/integration/submitqueue/core/consumerand//test/integration/extension/messagequeue/mysql.Checked that the new guard is meaningful: with the stale-entry eviction removed,
TestSubscriber_SubscribeReplacesStaleSubscriptionfails on both assertions and passes again once restored.