Fix indexwatch send on closed channel panic on watch-stream reconnect#887
Conversation
The Watcher's updates channel had two unsynchronized senders: the run goroutine that delivers snapshots, and the RPC dispatch goroutine that invokes our WatchIndex callback for live events. The run goroutine also owned the close (a deferred close(w.updates) on teardown), but the RPC callback's lifetime isn't bounded by the WatchIndex call returning, so on Stop or reconnect a callback could still be mid-send when run closed the channel underneath it, panicking the whole process. The existing select-with-ctx.Done guard didn't help: once the channel is closed, the send case is permanently ready (it panics rather than blocks), and select picks randomly among ready cases, so it hit the closed channel roughly half the time even with the context already cancelled. Serialize send and close with a mutex and a closed flag so a late callback can never touch a closed channel. We keep closing the channel on Stop rather than dropping the close, since consumers and a test rely on that contract.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesThe Watcher type in pkg/entity/indexwatch/watcher.go adds a mutex and a closed flag to coordinate closure of its updates channel with in-flight sends. The send method now locks the mutex and checks the closed flag before sending, holding the lock across the send/context-cancellation select. A new closeUpdates method locks the mutex, sets closed to true, and closes the channel; the watch goroutine's deferred cleanup now calls closeUpdates instead of closing the channel directly. A new test file adds two tests verifying safe behavior when sending after close and under concurrent send/close operations. Sequence Diagram(s)Included in the hidden review stack artifact above. Estimated code review effort: 3/5 (concurrency-sensitive locking logic requiring careful review of race conditions) Related issues: None specified. Related PRs: None specified. Suggested labels: bug, concurrency, indexwatch Suggested reviewers: None specified. 🐰 A mutex guards the gate so tight, Comment |
phinze
deleted the
phinze/mir-1307-index-watch-send-on-closed-channel-panic-on-watch-stream
branch
2 participants
Garden hard-crashed with
panic: send on closed channelcoming out of the indexwatch abstraction during a watch-stream reconnect. With garden restarting repeatedly as PRs deployed, the reconnect/teardown path got hammered and this race surfaced as a whole-process fatal.The root cause is that the Watcher's
updateschannel has two senders that don't know about each other. Therungoroutine delivers snapshots on it directly, and the RPC dispatch goroutine delivers live watch events on it by invoking theWatchIndexcallback we registered. Butrunalso owns the close: it does adefer close(w.updates)when it tears down. The catch is that the RPC callback's lifetime isn't bounded by theWatchIndexcall returning (the RPC layer doesn't join in-flight handler goroutines when the call unwinds), so onStopor a reconnect a callback can still be sitting insendat the exact momentruncloses the channel underneath it.The
sendalready had aselectonctx.Done(), which looks like it should cover this, but it doesn't. Once a channel is closed, its send case becomes permanently ready (a send on a closed channel panics rather than blocks), and Go'sselectpicks randomly among ready cases. So even with the context already cancelled,selectwould still choose the closed-channel send about half the time and panic.The fix serializes
sendand the close behind a mutex plus aclosedflag, so a check-and-send is atomic with respect to the close and a late callback can never touch a closed channel. We deliberately kept closing the channel onStoprather than taking the "don't close from the producer" route, because both real consumers (dns, ipalloc) andTestWatcher_StopClosesUpdatesrely on that contract.There's a new white-box regression test that reproduces the panic (it fails without the guard) plus a
-racestress test over concurrent send/close.Closes MIR-1307