fix(realtime): repair hub shutdown deadlock and WaitGroup misuse - #221
Merged
Conversation
internal/realtime has timed out at 3m under -race on seven CI runs since 2026-08-21 (#212), on PRs that touch no Go code. Three defects, all in the Hub start/stop lifecycle: 1. The writer goroutine's cleanup sent on h.unregister unconditionally after a racy h.stopped pre-check. Stop() can flip that flag and the run loop can return between the load and the send, leaving the writer blocked forever on a channel nobody drains — so writerWg.Wait() never returns and shutdown wedges. This is the 3m hang. Select on stopCh and close the client directly instead; the existing CAS guard keeps that safe against the run loop's own stop-path close. HandleWebSocket's h.register send had the same shape and gets the same treatment. 2. Run() called h.wg.Add(1) from inside the goroutine while Stop() called h.wg.Wait() — an Add from zero concurrent with Wait. Stop() could return before the loop had started, leaving it running afterwards. The count is now taken in NewHub and consumed by whichever of Run or Stop claims runOwner first, so Add always happens-before Wait and a hub that is never Run still stops cleanly. 3. HandleWebSocket called h.writerWg.Add(1) with no ordering against Stop()'s writerWg.Wait(), same misuse. Writer slots are now reserved through admitWriter() under lifecycleMu, which refuses once Stop has begun, so every Add is ordered before the Wait. The dead h.stopped flag is removed with its last reader. TestStopDoesNotDeadlockAgainstChurn drives connect/disconnect churn against a concurrent Stop() and fails on the unpatched hub (verified: DATA RACE) while passing on the fix. Also run: 20x internal/realtime under -race with GOMAXPROCS=2 (340 tests, green) and the full suite (1527 tests, 30 packages, 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
1 participant
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.
Closes #212.
internal/realtimehas timed out at 3m under-raceon seven CI runs since 2026-08-21 — including three simultaneous hits in today's dependabot batch (#132, #141, #145), all on PRs that touch no Go code. It is currently blocking dependency merges.This is not a test defect. Three real bugs in the Hub start/stop lifecycle:
1. Shutdown deadlock (the 3m hang). The writer goroutine's cleanup sent on
h.unregisterunconditionally, guarded only by a racyh.stoppedpre-check.Stop()can flip that flag and the run loop can return between the load and the send — nothing drainsh.unregisterafter that, so the writer blocks forever andwriterWg.Wait()never returns. In production this hangs graceful shutdown at step 2 of the documented shutdown order until the container is killed. Fixed by selecting onstopChand closing the client directly; the existing CAS guard keeps that safe against the run loop's own stop-path close.HandleWebSocket'sh.registersend had the identical shape and gets the same treatment.2.
wgAdd-from-zero concurrent with Wait.Run()calledh.wg.Add(1)from inside the goroutine whileStop()calledh.wg.Wait().Stop()could return before the loop had even started, leaving it running afterwards. The count is now taken inNewHuband consumed by whichever ofRun/StopclaimsrunOwnerfirst — Add always happens-before Wait, and a hub that is neverRun(several tests do this) still stops cleanly.3.
writerWgsame misuse.HandleWebSocketcalledh.writerWg.Add(1)with no ordering againstStop()'swriterWg.Wait(). Writer slots are now reserved viaadmitWriter()underlifecycleMu, which refuses onceStophas begun.The dead
h.stoppedflag is removed along with its last reader.Verification
TestStopDoesNotDeadlockAgainstChurndrives connect/disconnect churn against a concurrentStop(). Fails on the unpatched hub (verified by checking out main'shub.gounder the new test:WARNING: DATA RACE), passes on the fix.GOMAXPROCS=2 go test -race -count=20 ./internal/realtime/— 340 tests green, amplifying the starved-runner condition CI hits.go test ./...— 1527 tests, 30 packages, green.Honest scope note: the new test reproduces the WaitGroup race deterministically; the 3-minute CI hang is the rarer deadlock manifestation of defect 1, which is fixed by construction rather than by a test that reliably reproduces a timing window.