fix(watch): Coalesce control events into one dependency rebuild - #102
Merged
Conversation
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) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes watch daemon refresh behavior by coalescing “control events” (config changes and .gitignore edits) into a single refresh/rebuild, and hardens tests against races by waiting on observable published state updates rather than in-memory intermediate fields.
Changes:
- Add a dedicated trailing-edge debounce window for control events so a single save burst triggers one configured-files refresh and dependency-graph rebuild.
- Introduce a test seam (
daemonRefreshConfiguredFiles) so tests can count refresh invocations without intrusive instrumentation. - Update filter-change tests to wait for the daemon’s published state (
state.jsonUpdatedAt) to advance, avoiding mid-refresh race conditions; add new control-event burst tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| watch/events.go | Adds a separate control-event debounce timer and coalesces .codemap/config.json / .gitignore bursts into one refresh + writeState(). |
| watch/daemon.go | Introduces a package-level seam for refresh calls so tests can observe refresh frequency. |
| watch/more_test.go | Makes filter-change tests wait on published state advancement to avoid racy in-memory conditions; adds small helper functions. |
| watch/control_events_test.go | Adds new tests asserting control-event bursts coalesce into one refresh and preserve ignore-cache reset semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+60
to
+64
| 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 { |
8 tasks
JordanCoin
added a commit
that referenced
this pull request
Aug 3, 2026
Two real findings plus one readability change. checkScopedFile's doc comment still described the earlier behavior, where the first scope's failure was reported. The implementation was changed to prefer a scope that exists but is misconfigured over one that is merely absent, so the comment contradicted the code directly above it. TestControlEventBurstTriggersOneRefresh slept a hard-coded 500ms while waiting for further coalesced refreshes, which is disconnected from controlRefreshWindow and would silently stop testing anything if that window grew. Derived from the window instead. Also switched *failures++ to (*failures)++ to match the surrounding style. Copilot reported the original as incrementing the pointer and failing to compile; neither is true, since Go has no pointer arithmetic and ++ applies to the *failures expression, which is why CI passed on three platforms and three Go versions. Verified separately. The change is for clarity only. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Addresses both Copilot findings on #101. Thanks — the second one is a real regression that PR introduced.
1. Control events bypassed the debouncer
The event loop handled control events (
config.json, any.gitignore) andcontinued beforetakeDueBeforeEventran, so they were never debounced. That was harmless 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 oneconfig.jsonwrite triggered multiple full rebuilds back-to-back, serialized in the event loop.Reproduced before fixing — a five-write burst produced 3 rebuilds:
Control events now get their own trailing-edge debounce (150ms), kept separate from the file-event debouncer so neither perturbs the other's semantics.
resetIgnoreCacheis OR-ed across the burst, so a coalesced refresh still resets the ignore cache when any event in it was a.gitignore— covered by a second test, since that's the thing a naive coalescing would silently drop.2. The wait conditions were racy
Both filter-change tests waited on in-memory graph fields. Since the refresh nils the graph before rebuilding it, those conditions could be satisfied mid-refresh — before
writeStateran — leaving theReadStateassertions racing the daemon. Exactly as described.They now wait for the published state to advance past a captured baseline (
state.UpdatedAt), which is the observable end of a refresh cycle rather than a sample of intermediate state.FileGraph == nilno longer counts as completion.Type of change
Checklist
go build && ./codemap .CONTRIBUTING.md; this does not add a new language.Verification
go test ./...,go vet,staticcheck,gofmt, plus-count=8and-raceon the affected tests to confirm the flakiness is actually gone rather than just re-timed.go vetclean cross-compiled for windows/amd64 and linux/amd64.The refresh is now called through a
daemonRefreshConfiguredFilespackage var so tests can count invocations — same seam pattern already used fordoctorLookPath/doctorVersionProbe.