file_watcher: rebuild the watch tree when its coverage may be incomplete - #1434
file_watcher: rebuild the watch tree when its coverage may be incomplete#1434martinpitt wants to merge 2 commits into
Conversation
Nothing in buck2 can tell whether its watch tree actually covers the project, which is what made a missing inotify watch take hours to find: the only way to see it was to read /proc/<pid>/fdinfo by hand and match the inodes against the tree. Do that from the client instead. It lists the top of each unwatched subtree, so the directories a project ignores cost one line each rather than flooding the output; `--all` lists every one. Linux only, since it reads the watch descriptors out of /proc. Signed-off-by: Martin Pitt <martin@amutable.com>
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>
|
Hi @martinpitt! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
meta-cla
Bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
@8Keep has imported this pull request. If you are a Meta employee, you can view this in D115657706. (Because this pull request was imported automatically, there will not be any future comments.) |
1 participant
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
notifycrate actually reporting these failures tous, 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]dhere until a notify release carrying it is picked up.
Signed-off-by: Martin Pitt martin@amutable.com
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 (and hence dangerous!), I just kept wondering why my changes to the tree didn't have any effect and I got outdated file contents.
Warning: This was done in large parts by Claude Opus 5. I fully understand the bug root cause and the notify fix and the test case, but I don't know about the innards of buck2, so the actual file watcher code changes need an architecture critique, and I'm happy to rework it.
The second commit adds a
buck2 debug watchescommand, to make this easier to investigate. It's of course not critical, might just save the next pour soul some hours. Let me know if you want this or not.