From e4eaffdcec8b6867c46ce0d5615d535f63f227b7 Mon Sep 17 00:00:00 2001 From: Jordan Coin Jackson Date: Sun, 11 Oct 2026 22:47:54 +0200 Subject: [PATCH] fix(watch): order the lazy dependency-worker start against Stop 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 Claude-Session: https://claude.ai/code/session_01NRaBgKRAZMX9hjbkkefA7T --- watch/daemon.go | 68 +++++++++++++++++++++++++---------------- watch/stop_race_test.go | 38 +++++++++++++++++++++++ 2 files changed, 80 insertions(+), 26 deletions(-) create mode 100644 watch/stop_race_test.go diff --git a/watch/daemon.go b/watch/daemon.go index 38b4d6e..fbeb71b 100644 --- a/watch/daemon.go +++ b/watch/daemon.go @@ -52,7 +52,8 @@ type Daemon struct { dependencyRequests chan dependencyGraphSnapshot dependencyResults chan dependencyGraphResult - dependencyOnce sync.Once + dependencyMu sync.Mutex // guards dependencyStarted, dependencyCancel and WG.Add against Stop + dependencyStarted bool dependencyCancel context.CancelFunc dependencyWorkerWG sync.WaitGroup // These flags are owned by eventLoop; the worker only exchanges snapshots @@ -270,9 +271,14 @@ func (d *Daemon) computeTopology() { // Stop gracefully shuts down the daemon func (d *Daemon) Stop() { close(d.done) + // Under the same lock as the lazy start: a start that wins the lock + // first sets dependencyCancel before we read it; one that loses sees + // done closed and never starts, so no worker outlives Stop (#217). + d.dependencyMu.Lock() if d.dependencyCancel != nil { d.dependencyCancel() } + d.dependencyMu.Unlock() d.eventLoopWG.Wait() d.dependencyWorkerWG.Wait() _ = d.closeWatcher() @@ -456,36 +462,46 @@ func buildDependencyGraph(ctx context.Context, root string, filters scanner.Filt } func (d *Daemon) startDependencyWorker() { - d.dependencyOnce.Do(func() { - if d.dependencyRequests == nil { - d.dependencyRequests = make(chan dependencyGraphSnapshot, 1) - } - if d.dependencyResults == nil { - d.dependencyResults = make(chan dependencyGraphResult, 1) - } - ctx, cancel := context.WithCancel(context.Background()) - d.dependencyCancel = cancel - d.dependencyWorkerWG.Add(1) - go func() { - defer d.dependencyWorkerWG.Done() - for { + d.dependencyMu.Lock() + defer d.dependencyMu.Unlock() + if d.dependencyStarted { + return + } + select { + case <-d.done: + // Stop already ran; starting now would leak a worker it cannot cancel. + return + default: + } + d.dependencyStarted = true + if d.dependencyRequests == nil { + d.dependencyRequests = make(chan dependencyGraphSnapshot, 1) + } + if d.dependencyResults == nil { + d.dependencyResults = make(chan dependencyGraphResult, 1) + } + ctx, cancel := context.WithCancel(context.Background()) + d.dependencyCancel = cancel + d.dependencyWorkerWG.Add(1) + go func() { + defer d.dependencyWorkerWG.Done() + for { + select { + case <-ctx.Done(): + return + case snapshot := <-d.dependencyRequests: + started := time.Now() + filters := scanner.Filters{Only: snapshot.config.Only, Exclude: snapshot.config.Exclude} + graph, err := buildDependencyGraph(ctx, d.root, filters, buildFileGraph) + result := dependencyGraphResult{snapshot: snapshot, graph: graph, err: err, started: started} select { + case d.dependencyResults <- result: case <-ctx.Done(): return - case snapshot := <-d.dependencyRequests: - started := time.Now() - filters := scanner.Filters{Only: snapshot.config.Only, Exclude: snapshot.config.Exclude} - graph, err := buildDependencyGraph(ctx, d.root, filters, buildFileGraph) - result := dependencyGraphResult{snapshot: snapshot, graph: graph, err: err, started: started} - select { - case d.dependencyResults <- result: - case <-ctx.Done(): - return - } } } - }() - }) + } + }() } // handleDependencyGraphResult is called by eventLoop and owns the worker state flags. diff --git a/watch/stop_race_test.go b/watch/stop_race_test.go new file mode 100644 index 0000000..2d07be5 --- /dev/null +++ b/watch/stop_race_test.go @@ -0,0 +1,38 @@ +package watch + +import ( + "os" + "path/filepath" + "testing" + "time" + + "github.com/fsnotify/fsnotify" +) + +// Stop read dependencyCancel and waited on dependencyWorkerWG while the +// event loop could still be inside the lazy worker start, so a worker could +// be started after Stop returned and outlive the daemon (#217). +func TestStopRacesLazyWorkerStart(t *testing.T) { + root := t.TempDir() + if err := os.WriteFile(filepath.Join(root, "a.go"), []byte("package a\n"), 0o644); err != nil { + t.Fatal(err) + } + d, err := NewDaemon(root, false) + if err != nil { + t.Fatal(err) + } + d.watcher.Close() + d.watcher.Events = make(chan fsnotify.Event, 8) + d.watcher.Errors = make(chan error) + if err := os.MkdirAll(d.runtimeDir, 0o755); err != nil { + t.Fatal(err) + } + d.eventLoopWG.Add(1) + go func() { + defer d.eventLoopWG.Done() + d.eventLoop() + }() + d.watcher.Events <- fsnotify.Event{Name: filepath.Join(root, "a.go"), Op: fsnotify.Create} + time.Sleep(time.Millisecond) + d.Stop() +}