fix: never abandon a recursive watch, report what failed instead - #970
fix: never abandon a recursive watch, report what failed instead#970martinpitt wants to merge 3 commits into
Conversation
|
FTR, I ran into this with the buck2 build system. This will need a corresponding half to fix this for good (rebuilding the watch trees that failed, see facebook/buck2#1434). This was several hours of debugging.. there was an unreadable directory appearing during some split second, which cut off an entire branch of the watched tree, and from then on the daemon kept sending old data until I killed it. That effect was very subtle, I just kept wondering why my changes to the tree didn't work and I got outdated file contents. |
A directory on which the daemon holds no inotify watch is invisible: nothing under it produces an event, so buck keeps serving whatever it read last. This had two causes: - Dropped events cleared DICE, but the directories created while they were being dropped were never watched. - A watcher error discarded the events buffered with it and failed the next command, after which the daemon carried on as if nothing had been missed. To fix this, treat both as the same thing: clear DICE, as before, and then register the tree again so that it covers the project. The new registration is built before the old one is dropped, so nothing goes unwatched in between, and the overlap only duplicates events. Paths that cannot be watched at all (like an inaccessible directory) are remembered, so that they trigger that recovery once rather than on every command. This depends on the `notify` crate actually reporting these failures to us, which needs notify-rs/notify#970. Until that lands, a failed watch install is not among the errors that reach us. The new unit test passes with that notify fix; it stays `#[ignore]`d here until a notify release carrying it is picked up. Signed-off-by: Martin Pitt <martin@amutable.com>
Switch the notify watcher to a notify fork (patched in via the daandemeyer/notify GitHub fork) that adds a watch_filtered() API taking a filter which decides what gets watched: recursive scans do not descend into rejected directories, directories created later are checked against the filter before being auto-watched, and events beneath rejected directories are suppressed. The notify watcher passes a filter rejecting buck-out, ignored directories, and paths buck cannot represent. On Linux (inotify) these never get watch descriptors, so they generate no events at all: this eliminates the dominant source of useless wakeups during builds (buck-out writes), stops large buck-out trees from exhausting fs.inotify.max_user_watches, and speeds up daemon startup. On macOS (FSEvents) and Windows, which cannot watch directories selectively, the events are suppressed inside the notify backend before reaching our callback. The filter prunes any directory whose own path matches an ignore pattern, so a file-shaped glob (e.g. *.tmp) matching a directory name prunes that whole subtree. The fork also carries notify-rs/notify#970, which reports a failed watch install instead of dropping it, so the coverage-recovery test added earlier in this series no longer has to be #[ignore]d. Signed-off-by: Daan De Meyer <daan@amutable.com>
|
Pushed a clean rebase against current main. |
A directory on which the daemon holds no inotify watch is invisible: nothing under it produces an event, so buck keeps serving whatever it read last. This had two causes: - Dropped events cleared DICE, but the directories created while they were being dropped were never watched. - A watcher error discarded the events buffered with it and failed the next command, after which the daemon carried on as if nothing had been missed. To fix this, treat both as the same thing: clear DICE, as before, and then register the tree again so that it covers the project. The new registration is built before the old one is dropped, so nothing goes unwatched in between, and the overlap only duplicates events. Paths that cannot be watched at all (like an inaccessible directory) are remembered, so that they trigger that recovery once rather than on every command. This depends on the `notify` crate actually reporting these failures to us, which needs notify-rs/notify#970. Until that lands, a failed watch install is not among the errors that reach us. The new unit test passes with that notify fix; it stays `#[ignore]`d here until a notify release carrying it is picked up. Signed-off-by: Martin Pitt <martin@amutable.com>
Switch the notify watcher to a notify fork (patched in via the daandemeyer/notify GitHub fork) that adds a watch_filtered() API taking a filter which decides what gets watched: recursive scans do not descend into rejected directories, directories created later are checked against the filter before being auto-watched, and events beneath rejected directories are suppressed. The notify watcher passes a filter rejecting buck-out, ignored directories, and paths buck cannot represent. On Linux (inotify) these never get watch descriptors, so they generate no events at all: this eliminates the dominant source of useless wakeups during builds (buck-out writes), stops large buck-out trees from exhausting fs.inotify.max_user_watches, and speeds up daemon startup. On macOS (FSEvents) and Windows, which cannot watch directories selectively, the events are suppressed inside the notify backend before reaching our callback. The filter prunes any directory whose own path matches an ignore pattern, so a file-shaped glob (e.g. *.tmp) matching a directory name prunes that whole subtree. The fork also carries notify-rs/notify#970, which reports a failed watch install instead of dropping it, so the coverage-recovery test added earlier in this series no longer has to be #[ignore]d. Signed-off-by: Daan De Meyer <daan@amutable.com>
| - FEATURE: [windows] report created file/folder kinds when they can be determined [#935] | ||
| - CHANGE: [macOS] improve FSEvents callback performance by avoiding unnecessary allocations and repeated handler locking | ||
| - PERF: [kqueue] avoid filesystem walks for recursive kqueue unwatch | ||
| - FIX: never abandon a recursive watch, report what failed instead |
There was a problem hiding this comment.
| - FIX: never abandon a recursive watch, report what failed instead | |
| - FIX: [inotify] never abandon a recursive watch, report what failed instead |
There was a problem hiding this comment.
Done, and also added the PR ref. I also added a second entry for the walk errors you suggested. LMK if I should rather merge them.
Unrelated: [#964] a few lines up has no link definition.
| Err(err) if matches!(err.kind, ErrorKind::PathNotFound) => {} | ||
| // Neither should anything else: returning would leave every directory the walk has | ||
| // not reached yet unwatched, with nothing to notice. Report it and carry on. | ||
| Err(err) => self.event_handler.handle_event(Err(err)), |
There was a problem hiding this comment.
For recursive additions triggered by add_watch(..., watch_self = false) reaches this arm. The error is sent to the handler and add_watches_for_paths then returns Ok(()), so the existing reached_limit check in handle_inotify never sees it and cannot stop processing the remaining paths, it's a waste of processing time.
Could we special-case MaxFilesWatch before this arm and return Err(err)? That would preserve the existing stop-on-limit behavior while still continuing for recoverable, path-specific failures.
There was a problem hiding this comment.
Ugh yes, thanks for spotting! This was even worse than wasting time: once the limit is hit, every remaining directory would have handed the handler its own copy of the same error. 馃檲
To make this more robust, I split out that policy decision into a separate function and WalkFailure type, to avoid this oversight. It's a little more intrusive that way, but I think/hope worth it. Let me know if you don't like it.
| if let ErrorKind::MaxFilesWatch = add_watch_error.kind { | ||
| self.event_handler.handle_event(Err(add_watch_error)); | ||
| let reached_limit = matches!(add_watch_error.kind, ErrorKind::MaxFilesWatch); | ||
| self.event_handler.handle_event(Err(add_watch_error)); |
There was a problem hiding this comment.
add_watches contains directories discovered through CREATE and MOVED_TO events. By the time this code processes such an event, the directory may already have been deleted or renamed; the inotify documentation explicitly calls out this race (see the Limitations and caveats section on https://man7.org/linux/man-pages/man7/inotify.7.html).
We should continue skipping PathNotFound here.
There was a problem hiding this comment.
Agreed, fixed. The event loop consults the same policy as the recursive walk now, and PathNotFound maps to skipping it silently.
Note, since it is not obvious from the diff: add_watch stats the path before anything else, and Error::io_watch already maps ENOENT to ErrorKind::PathNotFound. So the created-then-deleted directory really does arrive as that kind and really is filtered.
There was a problem hiding this comment.
Note that there is another source of partial recursive watches that your PR doesn't currently cover: errors produced by WalkDir. So some fail paths will be ignored continuously. You might also want to touch that part if your problem would be affected.
There was a problem hiding this comment.
You are right, and it is the same bug one layer up: filter_dir threw away every walkdir::Error, so a directory that could not be listed silently pruned that whole subtree. Fixed in a separate commit.
This requires a little dance: An unreadable directory fails both inotify_add_watch and read_dir, so for tasteful/useful error reporting these duplicates need to be filtered out. That commit does it and unit-tests it.
Adding watches down a tree already treats one failure differently from the rest: a directory that vanishes between being listed and being watched is skipped; anything else ends the walk. The distinction lived in a match guard inside the inotify watch-adding code, where it was hard to reach for unit tests, at least in restricted build environments. Move the policy into a plain function over the error kind. This by itself does not change behaviour. Signed-off-by: Martin Pitt <martin@amutable.com>
A subdirectory whose watch could not be installed previously ended the whole recursive add: every directory the walk had not reached yet was left eternally unwatched. Nothing retried it and nothing reported it, and the only symptom was a program that stopped seeing changes in part of its tree. To fix this, carry on with the rest of the walk and hand each failure to the event handler, so that callers can tell their coverage is incomplete and act on it. The event loop consults the same policy now, rather than dropping every failure that is not the watch limit. Two failures keep the treatment they had: a directory that vanished between being listed and being watched stays silent, and the watch limit still ends the walk, as every directory after it would only run into the same limit. Watching a tree that holds one unreadable directory succeeds now, covering everything else, where it used to fail as a whole. That is the treatment a directory appearing later already got, except that its failure was dropped rather than reported. Cover both cases with tests. Signed-off-by: Martin Pitt <martin@amutable.com>
The walk dropped every error `walkdir()` handed it. A directory it could not list caused the whole subtree below it to get silently ignored. That is the same problem the previous fixed for from failed watches, one layer up. It is reachable by default because symlinks are followed (unless configured otherwise), so a link into its own ancestor loses everything the walk had not seen yet. Hand walk errors to the same policy that gets applied to failed watches. A vanished directory stays silent, the rest reach the event handler, and the caller knows which subtree is missing. Report the error pair that a single unreadable directory produces only once: it can neither be watched nor be listed, and walkdir reports the second right after the first. But for the caller these look identical and should not cause duplicated treatment. Signed-off-by: Martin Pitt <martin@amutable.com>
2 participants
A subdirectory whose watch could not be installed previously ended the whole recursive add: every directory the walk had not reached yet was left eternally unwatched. Nothing retried it and nothing reported it, and the only symptom was a program that stopped seeing changes in part of its tree.
To fix this, carry on with the rest of the walk and hand each failure to the event handler, so that callers can tell their coverage is incomplete and act on it. The event loop reports failures for the same reason, rather than dropping everything that is not the watch limit.
Watching a tree that holds one unreadable directory succeeds now, covering everything else, where it used to fail as a whole. That is the treatment a directory appearing later already got, except that its failure was dropped rather than reported.
Cover both cases with tests.
Contribution Agreement
I agree.
Changelog
Done.
Testing
I'll watch out for them. I covered both fixes with tests, and they fail without the fix.
Code Quality
Done, they pass.