Fix deadlock in Manager.Stop/StopReaders - #288
Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 1 commit intoSep 4, 2026
Merged
Conversation
brycekahle
approved these changes
Sep 3, 2026
brycekahle
left a comment
Member
There was a problem hiding this comment.
It looks like this will fix StopReaders for now, but it appears any code which calls perf/ring Stop while holding the manager stateLock could continue to deadlock. I can explore how to fix that in a more permanent and less brittle way.
Contributor
Author
|
exactly @brycekahle , that's why I put a comment on top of the function. |
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
theop-dd/fix-race-stop-readers-waiting-reader
branch
September 4, 2026 06:54
This was referenced Sep 4, 2026
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.
What does this PR do?
Split
stopReaders()into 3 phases:manager.stateLock) — signals goroutines to stopmanager.stateLock— lets handler callbacks complete, goroutines exitmanager.stateLock— clean up underlying mapsstop()now setsm.state = resetbefore callingstopReaders()to prevent concurrentNewPerfRing/NewRingBufferfrom adding readers during the unlock window.Known limitations
PerfMap.Stop()/RingBuffer.Stop()still hold their ownstateLockduringwgReader.Wait(). This is fine for current callers (only used for cleanup whenStart()fails, so no goroutine is running). A warning comment documents the risk.StopReaders()does not set manager state during the unlock window, so a concurrentNewPerfRingcall could theoretically slip through. This is documented as a usage error.Motivation
Manager.Stop()andManager.StopReaders()can deadlock when a handler callback (DataHandler, RecordHandler, etc.) calls Manager methods likeGetMap()orGetProbe().The deadlock occurs because:
Stopholdsmanager.stateLock(write lock) and waits for the reader goroutine to exit (wgReader.Wait())manager.stateLock.RLock()to call Manager methodsNeither can make progress → deadlock.
This is a race condition: it only triggers if
Stoptakes the lock while a handler is mid-execution. In production it's masked by SIGKILL at shutdown, but it surfaces in tests that stop and restart the manager in the same process.This deadlock occurs when we call
Stopon the manager. We never saw it in prod when the agent runs because if it happens, the process is eventually killed. This was however causing incidents on CI, where the hang caused every subsequent test to time out.