Repository navigation
fix(config): reload a global config written before the watch started (flaky watcher test) - #662
Merged
Merged
Conversation
Collaborator
Author
|
Verification:
|
…(PLAN-544) The daemon loads config.toml, then starts the watcher. A write in between raised its events before any watch existed, and the stat tick compares against the inode it attached to, so nothing reloaded until the next edit. The same window is why TestGlobalConfigWatcher_DeleteKeepsLoadedConfig (and siblings that Save right after `go Run`) timed out at 10 s on loaded CI runners: the Save beat the first attach. Run now arms one debounced reload when its first attach finds the file, as onError already does after a blind recreate. The new test writes the file before Run, which makes the window certain; it fails on the old code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Plumb-Session: scarlet-badger
…ed time #651's comments named PLAN-544, a number since given to an unrelated card. The cost test now logs the census time beside its 1.5 s bound, so a CI slowdown reads as a shrinking margin before it reddens (review NIT on #651). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Plumb-Session: scarlet-badger
…ubscribes Review findings on the start reload: - The watcher started before store.Subscribe(topoPool.Reconcile); a start reload landing before that subscription updated the store but never the pools built from the first load. It now starts after the subscriber. - Arm the debounce timer unconditionally: with no file the settle's attach fails and nothing reloads, so the armed flag was unobservable. - The new test fails with Run's own error if Run ends early, and waits 10 s rather than 2.5 s so a loaded runner cannot fail a correct watcher. - The census Logf comment and the CHANGELOG wording now claim only what holds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Plumb-Session: scarlet-badger
…44 entry Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Plumb-Session: scarlet-badger
golimpio
force-pushed
the
atlas/fix-544-config-watcher-startup
branch
from
October 14, 2026 06:37
319337a to
5a84dd2
Compare
golimpio
approved these changes
Oct 14, 2026
golimpio
left a comment
golimpio
left a comment
Contributor
There was a problem hiding this comment.
Approved under the delegated merge bar, on head 5a84dd2:
- an independent agent review returned PASS twice: first on the fix, then on the commit that folded in its findings;
- CI is 9/9 green on this head;
make verify-full-containerexited 0 on this head;plumb mutation_test: the start-reload mutant is killed by the new test.
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
TestGlobalConfigWatcher_DeleteKeepsLoadedConfigfailed onmain(verify, macOS) and on unrelated PRs, after 10 s with "the watcher never reloaded strict=true". It isn't test slack: the test lands in a real window in the global config watcher.The window. The daemon loads
config.toml, then startsglobalConfigWatcher.Run. A write in between raises its events before any watch exists. The one-second stat tick then compares against the inode the first attach just recorded, which is the same one, so it sees no change. The daemon keeps the older config until the next edit.The test. It saves right after
go gw.Run(ctx). On a loaded runner the save beats the first attach, so nothing ever reloads. Sibling tests of the same shape share the race.The fix. When
Run's first attach finds the file, it arms one debounced reload. This is the same "reload, we may have missed something" steponErroralready takes after recreating a lost watcher. There is no retry and no wider wait.Also in this PR
A second commit carries two follow-ups from the review of #651:
Tests
TestGlobalConfigWatcher_ReloadsAFileWrittenBeforeTheWatch. It writes the file beforeRun, which makes the window certain. It is red on the old code (never reloaded within two ticks) and green with the fix.-tags integration.plumb mutation_test):armed" is killed;Verification
make verify-full-container: result in a comment below.🤖 Generated with Claude Code