Repository navigation
fix(watch): order the lazy dependency-worker start against Stop - #220
Merged
Merged
Conversation
Stop read dependencyCancel and waited on dependencyWorkerWG while the event loop could still be inside startDependencyWorker, so a worker could start after Stop had read a nil cancel and outlive the daemon. The race detector reports it on the issue's repro and the run then hangs in dependencyWorkerWG.Wait, which is the outlive case made visible. The eager-start option was tried first and rejected: Start already calls startDependencyWorker before the event loop exists, so in production the lazy path runs on one goroutine ahead of any reader; the race only reaches Stop when eventLoop runs without Start. Making NewDaemon start the worker broke every graph_state test that builds a Daemon literal and relies on the lazy start. The mutex keeps laziness: start and the cancel read share one lock, and a start that sees done closed refuses, so nothing can be added to the WaitGroup after Stop has decided what to wait for. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NRaBgKRAZMX9hjbkkefA7T
# Conflicts: # watch/daemon.go
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.
The race
StopreaddependencyCanceland then waited ondependencyWorkerWGwith nothing ordering it against the lazystartDependencyWorkeron the event-loop goroutine. The issue's repro (eventLoopdriven withoutStart, one Create event,Stop) reports the race on main, and the run then hangs independencyWorkerWG.Waitfor the remaining-countiterations: a worker started afterStopread a nil cancel is never cancelled. That hang is the "worker outlives Stop" case made visible.Fix: mutex, not eager start
startDependencyWorkerand the cancel section ofStopsharedependencyMu. A start that takes the lock first writesdependencyCancelbeforeStopreads it; a start that takes it afterStopseesdoneclosed and refuses, so nothing is added to the WaitGroup afterStophas decided what to wait for.dependencyOnceis replaced by adependencyStartedflag under the same lock.refreshDependenciesalready selects ondone, so a refused start returns through that arm.Eager start (the issue's first proposal) was tried and rejected:
Startalready callsstartDependencyWorkerbefore the event loop exists, so in production the lazy path runs on one goroutine ahead of any reader and the race only reachesStopwheneventLoopruns withoutStart; moving the start intoNewDaemonbroke everygraph_state_testthat builds aDaemonliteral and relies on the lazy start (two of them hung the full suite at the 10-minute test timeout).Proof
TestStopRacesLazyWorkerStart(new,watch/stop_race_test.go): fails with the race report on main; passesgo test -race -run TestStopRacesLazyWorkerStart -count=50 ./watch/after (ok codemap/watch 49.8s).go build ./... && go test ./... && gofmt -l .clean.Not changed: daemon ownership and reaping (#155), the burst handling on #215 (expect a trivial merge; this diff is inside
daemon.goplus one new test file).Fixes #217
🤖 Generated with Claude Code
https://claude.ai/code/session_01NRaBgKRAZMX9hjbkkefA7T