From a359c96845315b187ebb3094416c6be440eb3c1a Mon Sep 17 00:00:00 2001 From: Jordan Coin Jackson Date: Sun, 2 Aug 2026 20:20:58 -0400 Subject: [PATCH] fix(watch): Coalesce control events into one dependency rebuild Addresses both Copilot findings on #101. Control events bypassed the debouncer: the event loop handled them and continued before takeDueBeforeEvent ran. That was cheap when the handler only re-walked the tree, but #101 made it rebuild the dependency graph, which runs ast-grep over the whole repo. fsnotify emits several events per save, so one config.json write triggered three full rebuilds back-to-back, serialized in the event loop. A test over a five-write burst reproduced exactly that count. Control events now get their own trailing-edge debounce, separate from the file-event debouncer so neither perturbs the other. The resetIgnoreCache flag is OR-ed across the burst, so a coalesced refresh still resets the ignore cache when any event in it was a .gitignore. The two filter-change tests waited on in-memory graph fields. The refresh nils the graph before rebuilding, so those conditions could be satisfied mid-refresh, before writeState ran, leaving the ReadState assertions racing the daemon. They now wait for the published state to advance past a captured baseline, which is the observable end of a refresh cycle. Verified with -count=8 and -race. Co-Authored-By: Claude Opus 5 (1M context) --- watch/control_events_test.go | 95 ++++++++++++++++++++++++++++++++++++ watch/daemon.go | 4 ++ watch/events.go | 42 ++++++++++++++-- watch/more_test.go | 34 ++++++++++++- 4 files changed, 171 insertions(+), 4 deletions(-) create mode 100644 watch/control_events_test.go diff --git a/watch/control_events_test.go b/watch/control_events_test.go new file mode 100644 index 0000000..1f1620e --- /dev/null +++ b/watch/control_events_test.go @@ -0,0 +1,95 @@ +package watch + +import ( + "os" + "path/filepath" + "sync/atomic" + "testing" + "time" +) + +// startControlEventDaemon boots a daemon over a minimal Go project and returns +// it with the path to its config file. +func startControlEventDaemon(t *testing.T) (*Daemon, string) { + t.Helper() + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, ".codemap"), 0o755); err != nil { + t.Fatal(err) + } + configPath := filepath.Join(root, ".codemap", "config.json") + if err := os.WriteFile(configPath, []byte(`{"only":["go"]}`), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, "main.go"), []byte("package main\n\nfunc main() {}\n"), 0o644); err != nil { + t.Fatal(err) + } + d, err := NewDaemon(root, false) + if err != nil { + t.Fatal(err) + } + if err := d.Start(); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { d.Stop() }) + return d, configPath +} + +// TestControlEventBurstTriggersOneRefresh pins that a burst of control events +// coalesces into a single refresh. Each refresh rebuilds the dependency graph, +// which runs ast-grep over the whole repo, and fsnotify routinely emits several +// events per save — so refreshing per event stalls the event loop and repeats +// the most expensive work the daemon does. +func TestControlEventBurstTriggersOneRefresh(t *testing.T) { + var refreshes atomic.Int32 + original := daemonRefreshConfiguredFiles + t.Cleanup(func() { daemonRefreshConfiguredFiles = original }) + daemonRefreshConfiguredFiles = func(d *Daemon, resetIgnoreCache bool) error { + refreshes.Add(1) + return original(d, resetIgnoreCache) + } + + _, configPath := startControlEventDaemon(t) + + // A tight burst, well inside the coalescing window. + for i := 0; i < 5; i++ { + if err := os.WriteFile(configPath, []byte(`{"only":["go","md"]}`), 0o644); err != nil { + t.Fatal(err) + } + } + + waitForWatchCondition(t, 5*time.Second, func() bool { return refreshes.Load() >= 1 }) + // Let any further coalesced refreshes land before counting. + time.Sleep(500 * time.Millisecond) + + if got := refreshes.Load(); got != 1 { + t.Fatalf("control event burst caused %d refreshes, want 1", got) + } +} + +// TestControlEventBurstPreservesIgnoreCacheReset guards the coalescing: when a +// burst mixes a .gitignore change with a config change, the single refresh must +// still reset the ignore cache, or gitignore edits are silently dropped. +func TestControlEventBurstPreservesIgnoreCacheReset(t *testing.T) { + var sawReset atomic.Bool + var refreshes atomic.Int32 + original := daemonRefreshConfiguredFiles + t.Cleanup(func() { daemonRefreshConfiguredFiles = original }) + daemonRefreshConfiguredFiles = func(d *Daemon, resetIgnoreCache bool) error { + refreshes.Add(1) + if resetIgnoreCache { + sawReset.Store(true) + } + return original(d, resetIgnoreCache) + } + + d, configPath := startControlEventDaemon(t) + + if err := os.WriteFile(configPath, []byte(`{"only":["go","md"]}`), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(d.root, ".gitignore"), []byte("build/\n"), 0o644); err != nil { + t.Fatal(err) + } + + waitForWatchCondition(t, 5*time.Second, func() bool { return sawReset.Load() }) +} diff --git a/watch/daemon.go b/watch/daemon.go index bdd8198..f6ab360 100644 --- a/watch/daemon.go +++ b/watch/daemon.go @@ -212,6 +212,10 @@ func (d *Daemon) isConfiguredFile(path string) bool { return scanner.MatchesFilters(path, filepath.Ext(path), cfg.Only, cfg.Exclude) } +// daemonRefreshConfiguredFiles is the seam the event loop calls, so tests can +// observe how often control events trigger a refresh. +var daemonRefreshConfiguredFiles = (*Daemon).refreshConfiguredFiles + func (d *Daemon) refreshConfiguredFiles(resetIgnoreCache bool) error { gitCache := d.gitCache if resetIgnoreCache { diff --git a/watch/events.go b/watch/events.go index ed548d3..9690b5f 100644 --- a/watch/events.go +++ b/watch/events.go @@ -41,6 +41,10 @@ const ( debounceDefer ) +// controlRefreshWindow is how long the event loop waits for a control-event +// burst to settle before refreshing configured files once. +const controlRefreshWindow = 150 * time.Millisecond + func newEventDebouncer(window time.Duration) *eventDebouncer { pruneAfter := 10 * window if pruneAfter < time.Second { @@ -171,6 +175,34 @@ func (d *Daemon) eventLoop() { d.handleEvent(event) } } + + // Control events (config.json, any .gitignore) are coalesced separately + // from file events. Each refresh rebuilds the dependency graph, which runs + // ast-grep over the whole repo, and fsnotify routinely emits several events + // per save — so a trailing-edge debounce keeps one save to one rebuild. + controlTimer := time.NewTimer(time.Hour) + controlTimer.Stop() + defer controlTimer.Stop() + var controlTimerC <-chan time.Time + controlResetIgnoreCache := false + armControlTimer := func() { + if !controlTimer.Stop() { + select { + case <-controlTimer.C: + default: + } + } + controlTimer.Reset(controlRefreshWindow) + controlTimerC = controlTimer.C + } + refreshConfigured := func() { + controlTimerC = nil + resetIgnoreCache := controlResetIgnoreCache + controlResetIgnoreCache = false + if err := daemonRefreshConfiguredFiles(d, resetIgnoreCache); err == nil { + d.writeState() + } + } defer func() { for _, event := range debouncer.takeAll() { d.handleEvent(event) @@ -187,15 +219,19 @@ func (d *Daemon) eventLoop() { flushDue(now) armTimer(time.Now()) + case <-controlTimerC: + refreshConfigured() + case event, ok := <-d.watcher.Events: if !ok { return } now := time.Now() if resetIgnoreCache, control := d.filterControlEvent(event.Name); control { - if err := d.refreshConfiguredFiles(resetIgnoreCache); err == nil { - d.writeState() - } + // OR the flag across the burst: a coalesced refresh must still + // reset the ignore cache if any event in it was a .gitignore. + controlResetIgnoreCache = controlResetIgnoreCache || resetIgnoreCache + armControlTimer() continue } for _, pending := range debouncer.takeDueBeforeEvent(event, now) { diff --git a/watch/more_test.go b/watch/more_test.go index fbf9a2e..ff2ada8 100644 --- a/watch/more_test.go +++ b/watch/more_test.go @@ -299,6 +299,7 @@ func TestConfiguredFilterChangeInvalidatesDependencyState(t *testing.T) { d.graph.DepCtx = map[string]*DepContext{"old.go": {Importers: []string{"a.go"}}} d.graph.HasDeps = true d.graph.mu.Unlock() + baseline := publishedStateTime(t, root) if err := os.WriteFile(configPath, []byte(`{"only":["sql"]}`), 0o644); err != nil { t.Fatal(err) } @@ -306,14 +307,22 @@ func TestConfiguredFilterChangeInvalidatesDependencyState(t *testing.T) { // not that the graph is left destroyed: refreshConfiguredFiles rebuilds it // (see TestConfiguredFilterChangeRebuildsDependencyGraph), so assert the // stale entries are gone rather than that the graph is nil. + // + // Wait on the published state advancing rather than on in-memory fields. + // The refresh nils the graph before rebuilding it, so an in-memory + // condition can be satisfied mid-refresh, before writeState has run, and + // the ReadState assertion below would then race the daemon. waitForWatchCondition(t, 5*time.Second, func() bool { + if !publishedStateAdvanced(root, baseline) { + return false + } d.graph.mu.RLock() defer d.graph.mu.RUnlock() if _, stale := d.graph.DepCtx["old.go"]; stale { return false } if d.graph.FileGraph == nil { - return true + return false } _, stale := d.graph.FileGraph.Importers["old.go"] return !stale @@ -491,6 +500,7 @@ func TestConfiguredFilterChangeRebuildsDependencyGraph(t *testing.T) { d.graph.DepCtx = map[string]*DepContext{"stale.go": {Importers: []string{"a.go"}}} d.graph.HasDeps = true d.graph.mu.Unlock() + baseline := publishedStateTime(t, root) // Widen the filters; Go files stay configured, so dependency intelligence // must come back rather than stay dropped. @@ -498,6 +508,9 @@ func TestConfiguredFilterChangeRebuildsDependencyGraph(t *testing.T) { t.Fatal(err) } waitForWatchCondition(t, 5*time.Second, func() bool { + if !publishedStateAdvanced(root, baseline) { + return false + } d.graph.mu.RLock() defer d.graph.mu.RUnlock() if !d.graph.HasDeps || d.graph.FileGraph == nil { @@ -507,3 +520,22 @@ func TestConfiguredFilterChangeRebuildsDependencyGraph(t *testing.T) { return !stale }) } + +// publishedStateTime returns the current state file timestamp, used as a +// baseline so tests can wait for the daemon to publish a *newer* state rather +// than sampling in-memory fields mid-refresh. +func publishedStateTime(t *testing.T, root string) time.Time { + t.Helper() + state := ReadState(root) + if state == nil { + t.Fatal("daemon has not published initial state") + } + return state.UpdatedAt +} + +// publishedStateAdvanced reports whether the daemon has written state newer +// than baseline, which is the observable completion of a refresh cycle. +func publishedStateAdvanced(root string, baseline time.Time) bool { + state := ReadState(root) + return state != nil && state.UpdatedAt.After(baseline) +}