fix(test): make TestContextInCallbacks deterministic - #118
Open
tigerquoll wants to merge 1 commit into
Open
Conversation
TestContextInCallbacks raced a free-running cancel() goroutine against Event(). When cancel fired before the transition's ctx.Err() check, the enter_end callback was skipped, the goroutine that closes enterEndAsyncWorkDone was never spawned, and the test deadlocked until the 10m test-binary timeout. Trigger the cancellation from inside enter_end instead, so it always happens after the transition has begun.
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.
Problem
TestContextInCallbacksdeadlocks intermittently and takes the whole package down with it: when it loses an internal race, the test blocks forever on a channel receive and the test binary dies on the 10-minute timeout, so every other test's result is discarded too. This is what failed CI on #116 (a PR that doesn't touchfsm_test.go), and it reproduces onmainin seconds with:Root cause
The test races a free-running goroutine against
Event:Event's transition function checksctx.Err()before running enter-state callbacks. Ifcancel()fires before that check, the transition aborts early:Eventstill returnscontext.Canceled— so the test's error assertion passes — butenter_endnever runs, the goroutine that closesenterEndAsyncWorkDoneis never spawned, and<-enterEndAsyncWorkDoneblocks forever. The goroutine dump in the CI failure confirms it: the test goroutine is parked on that receive and no goroutine is waiting inside the callback.The intended path only happens when
cancel()loses the race, which it usually does on a fast machine and frequently doesn't on a loaded CI runner.Fix
Test-only. Trigger the cancellation from inside
enter_end, after the watcher goroutine is spawned, so it always happens after the transition has begun and can no longer race the context check. The assertions are unchanged.Verification
mainwithin ~100 iterations and hits the timeout.-race; full suite passes under-race;go vetclean.Note: #117 fixes a different (production-facing) consequence of the same early-abort path — the two are independent and don't conflict.