test(events): hold the SUBSCRIBE ack at the wire until the timer arm is taken (BUG-2786) - #1214
Merged
Merged
Conversation
…is taken (BUG-2786)
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.
Fixes BUG-2786. Test-only change in
internal/events; no production behaviour changes (one doc comment on thebeforeUnconfirmedMarkseam corrected).The defect was in the instrument, not the timing
TestAConfirmedSubscriptionIsNeverLeftMarkedUnconfirmedsetsconfirmTimeout = 1nsand hooksbeforeUnconfirmedMark, which only runs on the timer arm ofestablishSubscription's confirmation select. A 1ns bound guarantees the timer arm is ready — not that the acknowledgement is not. Under CI load the receive goroutine consumed the SUBSCRIBE reply before the establisher reached its select, both arms were ready, Go chose at random, and on the confirmed arm the hook never ran: the guard then reported "the acknowledgement never landed before the mark" when it had landed too early for the interleave to exist.Measured before the fix with a throwaway
hookRanflag: unloaded-race -count=300→ 300/300; 16 busy loops +GOMAXPROCS=2 -race -count=1000→ 10 failures, allhookRan=false, zero stalled-ack. The paired 200ms/50ms WARNs in the CI log come from sibling tests that expire their bound by design — they were never load evidence.The fix
A signal-released variant of the file's existing SUBSCRIBE delay proxy (
newSubscribeGateProxy): the reply is parked at the wire until the test releases it, so only the timer arm can be ready at the select and the interleave is a construction rather than a coin toss. The three other tests that relied on a fixed delay outrunning their bound (margins 350ms / 1.95s / 2.8s) are converted too, so the class is closed structurally, not by margin; each releases only after the mark is observed (Subscribe returned + observer count, or observer report), so the acknowledgement is late by construction. Premise guards run in an order where each guard's cause is the only one remaining (waitParked→forcedOpen→hookRan→raced).Evidence (all on a185790 unless stated)
go build ./...,go vet ./...,make lint(0 issues),go test -race ./internal/events/ok.GOMAXPROCS=2 -race -count=1000→ 1000/1000.-count=250→ ok, 0 failures. The pre-conversion tree (31d7f58c, gate released from the pre-lock hook) fails that same leg 13 times — that is codex round 3's P1, reproduced by measurement before it was fixed.confirmClosedre-check inmarkUnconfirmedAdmission): detected 10/10 unloaded and 10/10 loaded, via the real assertion, not a premise guard.release()double-close; gate released before the mark; one-shotheldreads not being barriers; gate published after the accept goroutine; guard order). One R5 finding is pre-existing and recorded rather than widened: the proxy scans for the RESP marker within a singleRead, so a split marker is not parked — same code as the delay proxy on main, and a miss is now named bywaitParkedrather than silent.Sweep boundary:
internal/eventsandinternal/watcheventssearched for other timer-arm-dependent tests; the four here are the population.