From b4b658eea3e1c98a39bac1d94fdb23714e860107 Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Wed, 2 Sep 2026 11:33:55 +0000 Subject: [PATCH 1/3] starlark: fix the miri regression suite under the `pagable` feature `Module::freeze` is compiled out when `pagable` is on, so the suite added in 76aab728d6 only builds standalone. In the cargo workspace, where buck2 turns the feature on for every crate, `cargo clippy --tests` cannot compile it and takes the whole lint run down with it. Freeze with a name instead, which is the API that exists in both configurations. The name is incidental to what the test covers: interning the single-character member name. --- starlark-rust/starlark/tests/miri.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/starlark-rust/starlark/tests/miri.rs b/starlark-rust/starlark/tests/miri.rs index 27611148b359a..b87a401267c93 100644 --- a/starlark-rust/starlark/tests/miri.rs +++ b/starlark-rust/starlark/tests/miri.rs @@ -28,6 +28,7 @@ use starlark::environment::Module; use starlark::eval::Evaluator; use starlark::syntax::AstModule; use starlark::syntax::Dialect; +use starlark::values::FrozenHeapName; use starlark::values::Heap; use starlark::values::Value; use starlark::values::list::AllocList; @@ -91,6 +92,6 @@ fn single_character_module_name() { let value = module.heap().alloc("value"); module.set("x", value); assert_eq!(module.get("x").unwrap().unpack_str(), Some("value")); - module.freeze().unwrap(); + module.freeze_named(FrozenHeapName::user("miri")).unwrap(); }); } From 38770c03a322eccecee567bc02b4d9284819a2a3 Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Sat, 1 Aug 2026 06:02:57 +0000 Subject: [PATCH 2/3] debug: add `buck2 debug watches` 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//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 --- app/buck2_cmd_debug_client/src/lib.rs | 9 ++ app/buck2_cmd_debug_client/src/watches.rs | 136 ++++++++++++++++++++++ 2 files changed, 145 insertions(+) create mode 100644 app/buck2_cmd_debug_client/src/watches.rs diff --git a/app/buck2_cmd_debug_client/src/lib.rs b/app/buck2_cmd_debug_client/src/lib.rs index d1007c5c1ce60..5cb3b68b46fae 100644 --- a/app/buck2_cmd_debug_client/src/lib.rs +++ b/app/buck2_cmd_debug_client/src/lib.rs @@ -37,6 +37,8 @@ use crate::set_log_filter::SetLogFilterCommand; use crate::thread_dump::ThreadDumpCommand; use crate::trace_io::TraceIoCommand; use crate::upload_re_logs::UploadReLogsCommand; +#[cfg(target_os = "linux")] +use crate::watches::WatchesCommand; mod allocative; mod allocator_stats; @@ -59,6 +61,8 @@ mod set_log_filter; mod thread_dump; mod trace_io; mod upload_re_logs; +#[cfg(target_os = "linux")] +mod watches; #[derive(Debug, clap::Parser)] #[clap(about = "Hidden debug commands useful for testing buck2")] @@ -100,6 +104,9 @@ pub enum DebugCommand { Paranoid(ParanoidCommand), Eval(EvalCommand), ThreadDump(ThreadDumpCommand), + /// Lists project directories the daemon holds no inotify watch for. + #[cfg(target_os = "linux")] + Watches(WatchesCommand), /// Control DICE node value page-out / page-in. #[clap(subcommand)] Hydration(HydrationCommand), @@ -135,6 +142,8 @@ impl DebugCommand { DebugCommand::Paranoid(cmd) => cmd.exec(matches, ctx), DebugCommand::Eval(cmd) => ctx.exec(cmd, matches, events_ctx), DebugCommand::ThreadDump(cmd) => cmd.exec(matches, ctx), + #[cfg(target_os = "linux")] + DebugCommand::Watches(cmd) => cmd.exec(matches, ctx), DebugCommand::Hydration(cmd) => ctx.exec(cmd, matches, events_ctx), } } diff --git a/app/buck2_cmd_debug_client/src/watches.rs b/app/buck2_cmd_debug_client/src/watches.rs new file mode 100644 index 0000000000000..a5d8625eeae75 --- /dev/null +++ b/app/buck2_cmd_debug_client/src/watches.rs @@ -0,0 +1,136 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is dual-licensed under either the MIT license found in the + * LICENSE-MIT file in the root directory of this source tree or the Apache + * License, Version 2.0 found in the LICENSE-APACHE file in the root directory + * of this source tree. You may select, at your option, one of the + * above-listed licenses. + */ + +//! Report project directories the daemon holds no inotify watch for. +//! +//! A directory that never got a watch makes every later change under it invisible for the life of +//! the daemon: no `File changed:` line, no invalidation, and a build that quietly uses stale +//! contents. Nothing inside buck2 can see that state today, so this reads the daemon's watch +//! descriptors out of `/proc` and compares them against the tree. +//! +//! Debugging aid, Linux only. Directories excluded by `[project] ignore` are reported too, since +//! resolving cell ignores needs the daemon; only the top of each unwatched subtree is listed, so +//! they show up as one line each rather than as a flood. + +use std::collections::HashSet; +use std::fs; +use std::os::unix::fs::MetadataExt; +use std::path::Path; +use std::path::PathBuf; + +use buck2_client_ctx::client_ctx::ClientCommandContext; +use buck2_client_ctx::common::BuckArgMatches; +use buck2_client_ctx::exit_result::ExitResult; +use buck2_error::BuckErrorContext; + +/// List project directories the running daemon has no inotify watch for. +#[derive(Debug, clap::Parser)] +pub struct WatchesCommand { + /// List every unwatched directory, not just the top of each unwatched subtree. + #[clap(long)] + all: bool, +} + +/// The inodes the daemon holds inotify watches on, from every inotify fd it has open. +fn watched_inodes(pid: u64) -> buck2_error::Result> { + let mut inodes = HashSet::new(); + let fdinfo = PathBuf::from(format!("/proc/{pid}/fdinfo")); + for entry in fs::read_dir(&fdinfo) + .with_buck_error_context(|| format!("Reading `{}`", fdinfo.display()))? + { + let Ok(contents) = fs::read_to_string(entry?.path()) else { + continue; // fds come and go while we read them + }; + for line in contents.lines() { + if let Some(rest) = line.strip_prefix("inotify wd:") { + for field in rest.split_whitespace() { + if let Some(hex) = field.strip_prefix("ino:") + && let Ok(ino) = u64::from_str_radix(hex, 16) + { + inodes.insert(ino); + } + } + } + } + } + Ok(inodes) +} + +/// Walk the project, collecting directories with no watch. Descends into an unwatched directory +/// only when asked, so an ignored subtree costs one line. +fn unwatched(root: &Path, watched: &HashSet, all: bool) -> Vec { + let mut holes = Vec::new(); + let mut pending = vec![root.to_path_buf()]; + while let Some(directory) = pending.pop() { + let Ok(entries) = fs::read_dir(&directory) else { + continue; + }; + for entry in entries.flatten() { + let path = entry.path(); + if !entry.file_type().is_ok_and(|kind| kind.is_dir()) { + continue; + } + if path + .file_name() + .is_some_and(|name| name == "buck-out" || name == ".git") + { + continue; + } + let Ok(metadata) = fs::symlink_metadata(&path) else { + continue; + }; + if metadata.is_symlink() { + continue; + } + if watched.contains(&metadata.ino()) { + pending.push(path); + } else { + holes.push(path.clone()); + if all { + pending.push(path); + } + } + } + } + holes.sort(); + holes +} + +impl WatchesCommand { + pub fn exec(self, _matches: BuckArgMatches<'_>, ctx: ClientCommandContext<'_>) -> ExitResult { + let paths = ctx.paths()?; + let info = paths.daemon_dir()?.buckd_info(); + let daemon: serde_json::Value = serde_json::from_str( + &fs::read_to_string(&info) + .with_buck_error_context(|| format!("Reading `{}`", info.display()))?, + )?; + let Some(pid) = daemon["pid"].as_u64() else { + return ExitResult::bail("No pid in buckd.info; is a daemon running?"); + }; + + let root = paths.project_root().root(); + let watched = watched_inodes(pid)?; + let holes = unwatched(root.as_path(), &watched, self.all); + + buck2_client_ctx::println!("daemon {} holds {} watches", pid, watched.len())?; + for hole in &holes { + buck2_client_ctx::println!( + "unwatched: {}", + hole.strip_prefix(root.as_path()).unwrap_or(hole).display() + )?; + } + buck2_client_ctx::println!( + "{} unwatched {} (ignored directories are expected here)", + holes.len(), + if self.all { "directories" } else { "subtrees" }, + )?; + ExitResult::success() + } +} From 44c5561eea4091061b77bbfa2233928acdb72895 Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Sat, 1 Aug 2026 06:02:46 +0000 Subject: [PATCH 3/3] file_watcher: rebuild the watch tree when its coverage may be incomplete 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 https://github.com/notify-rs/notify/pull/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 --- app/buck2_file_watcher/src/notify.rs | 210 ++++++++++++++++++++++++--- 1 file changed, 188 insertions(+), 22 deletions(-) diff --git a/app/buck2_file_watcher/src/notify.rs b/app/buck2_file_watcher/src/notify.rs index 586a06a71a24f..e1cb71c97f1ad 100644 --- a/app/buck2_file_watcher/src/notify.rs +++ b/app/buck2_file_watcher/src/notify.rs @@ -8,7 +8,9 @@ * above-listed licenses. */ +use std::collections::HashSet; use std::mem; +use std::path::PathBuf; use std::sync::Arc; use std::sync::Mutex; @@ -81,8 +83,19 @@ impl NotifyFileData { cells: &CellResolver, ignore_specs: &StdBuckHashMap, ) -> buck2_error::Result<()> { - let event = - event.map_err(|e| from_any_with_tag(e, buck2_error::ErrorTag::NotifyWatcher))?; + let event = match event { + Ok(event) => event, + // The watcher failed at something, typically installing a watch for a directory that + // just appeared. It is not fatal, but from here on the watch tree covers less than the + // whole project, and a directory nobody watches produces no events at all: the daemon + // would keep serving whatever it read last. Treat it exactly like dropped events, which + // clears DICE and registers the tree again. + Err(e) => { + self.missed_events = true; + info!("FileWatcher: watcher error, coverage may be incomplete: {e:?}"); + return Ok(()); + } + }; // Checked before the path loop: the kernel-overflow rescan signal can arrive with no // paths attached (platform dependent), and a missed rescan means the daemon keeps @@ -130,9 +143,9 @@ impl NotifyFileData { fn sync(self) -> (buck2_data::FileWatcherStats, Option) { // The changes that go into the DICE transaction let mut changed = FileChangeTracker::new(); - // If we missed events, sync2() will drop the entire DICE graph. Surface that to - // telemetry/UI by reusing the fresh-instance fields the watchman path uses for - // the equivalent wipe. + // If we missed events, sync2() will drop the entire DICE graph and register the watch + // tree again. Surface that to telemetry/UI by reusing the fresh-instance fields the + // watchman path uses when it clears DICE for the same reason. let base = if self.missed_events { buck2_data::FileWatcherStats { fresh_instance: true, @@ -142,7 +155,7 @@ impl NotifyFileData { cleared_dep_files: false, }), incomplete_events_reason: Some( - "notify dropped events (kernel queue overflow)".to_owned(), + "notify dropped events or failed to watch a directory".to_owned(), ), ..Default::default() } @@ -268,12 +281,30 @@ impl NotifyFileData { } } +/// What it takes to register the watch tree, kept so that it can be built again. +struct Registration { + root: ProjectRoot, + cells: CellResolver, + ignore_specs: StdBuckHashMap, +} + +/// Paths whose watch could not be installed. +/// +/// A path that failed once fails again on every registration, a directory the user has no +/// permission for being the obvious case. Remembering them keeps such a directory from making every +/// single command drop DICE and walk the tree again; they cost that once. +type FailedWatches = Arc>>; + #[derive(Allocative)] pub struct NotifyFileWatcher { + /// Never used directly, but must be kept alive: dropping the watcher removes all its watches. + /// Replaced wholesale when the tree has to be registered again. #[allocative(skip)] - #[expect(unused)] - // FIXME(JakobDegen): Clarify if this just needs to be kept alive or can be removed? - watcher: RecommendedWatcher, + watcher: Mutex, + #[allocative(skip)] + registration: Registration, + #[allocative(skip)] + failed: FailedWatches, data: Arc>>, } @@ -284,21 +315,69 @@ impl NotifyFileWatcher { ignore_specs: StdBuckHashMap, ) -> buck2_error::Result { let data = Arc::new(Mutex::new(Ok(NotifyFileData::new()))); - let data2 = data.dupe(); - let root2 = root.dupe(); - let mut watcher = notify::recommended_watcher(move |event| { - let mut guard = data2.lock().unwrap(); - if let Ok(state) = &mut *guard { - if let Err(e) = state.process(event, &root2, &cells, &ignore_specs) { - *guard = Err(e); - } - } + let registration = Registration { + root: root.dupe(), + cells, + ignore_specs, + }; + let failed: FailedWatches = Default::default(); + let watcher = Self::register(®istration, data.dupe(), failed.dupe())?; + Ok(Self { + watcher: Mutex::new(watcher), + registration, + failed, + data, }) - .map_err(|e| from_any_with_tag(e, buck2_error::ErrorTag::NotifyWatcher))?; + } + + /// Watch the whole project. + fn register( + registration: &Registration, + data: Arc>>, + failed: FailedWatches, + ) -> buck2_error::Result { + let root = registration.root.dupe(); + let cells = registration.cells.dupe(); + let ignore_specs = registration.ignore_specs.clone(); + let mut watcher = + notify::recommended_watcher(move |event: notify::Result| { + // A path we already know we cannot watch is not news, and reacting to it again would + // make every command drop DICE for as long as it exists. + if let Err(e) = &event { + let mut failed = failed.lock().unwrap(); + let novel: Vec<_> = e.paths.iter().map(|p| failed.insert(p.clone())).collect(); + if !e.paths.is_empty() && !novel.contains(&true) { + debug!("FileWatcher: {:?} failed to watch again", e.paths); + return; + } + } + let mut guard = data.lock().unwrap(); + if let Ok(state) = &mut *guard { + if let Err(e) = state.process(event, &root, &cells, &ignore_specs) { + *guard = Err(e); + } + } + }) + .map_err(|e| from_any_with_tag(e, buck2_error::ErrorTag::NotifyWatcher))?; watcher - .watch(root.root().as_path(), notify::RecursiveMode::Recursive) + .watch( + registration.root.root().as_path(), + notify::RecursiveMode::Recursive, + ) .map_err(|e| from_any_with_tag(e, buck2_error::ErrorTag::NotifyWatcher))?; - Ok(Self { watcher, data }) + Ok(watcher) + } + + /// Register the tree again, after events were dropped or a watch failed to install. + /// + /// Dropping DICE recovers from the changes we did not see, but not from a directory that has no + /// watch at all: nothing would ever report a change there again. The new registration walks the + /// tree and covers whatever the old one is missing. It is built before the old one is dropped, + /// so no window opens where the project is unwatched; the overlap only duplicates events. + fn reregister(&self) -> buck2_error::Result<()> { + let watcher = Self::register(&self.registration, self.data.dupe(), self.failed.dupe())?; + *self.watcher.lock().unwrap() = watcher; + Ok(()) } fn sync2( @@ -313,8 +392,10 @@ impl NotifyFileWatcher { if let Some(changes) = changes { changes.write_to_dice(&mut dice)?; } else { - // We missed some file system notifications, so we drop everything + // We missed some file system notifications, so we drop everything and make sure the + // watch tree covers the project again before we read it back. dice = dice.unstable_take(); + self.reregister()?; } Ok((stats, dice)) } @@ -428,4 +509,89 @@ mod tests { assert!(!stats.fresh_instance); assert!(stats.incomplete_events_reason.is_none()); } + + /// Coverage recovery, which needs a directory that cannot be watched: unix permissions + /// are the only portable way to make one. + #[cfg(unix)] + mod coverage { + use std::fs; + use std::os::unix::fs::PermissionsExt; + use std::thread::sleep; + use std::time::Duration; + use std::time::Instant; + + use super::*; + + /// Wait for the watcher thread to catch up with what the test did. + fn wait_for(watcher: &NotifyFileWatcher, done: impl Fn(&NotifyFileData) -> bool) -> bool { + let deadline = Instant::now() + Duration::from_secs(5); + while Instant::now() < deadline { + if let Ok(data) = &*watcher.data.lock().unwrap() { + if done(data) { + return true; + } + } + sleep(Duration::from_millis(50)); + } + false + } + + /// A watch that could not be installed has to leave the daemon knowing that its coverage is + /// incomplete, so that the next sync clears DICE and registers the tree again. + /// + /// Ignored because notify reports the failure only with + /// ; run with `--ignored` against a notify that + /// carries it. + #[test] + #[ignore = "needs notify-rs/notify#970 for the failure to be reported at all"] + fn a_failed_watch_counts_as_missed_events() { + let tempdir = tempfile::tempdir().unwrap(); + let project = tempdir.path().join("project"); + let staging = tempdir.path().join("staging"); + fs::create_dir(&project).unwrap(); + fs::create_dir_all(staging.join("readable")).unwrap(); + let unwatchable = staging.join("unwatchable"); + fs::create_dir(&unwatchable).unwrap(); + fs::set_permissions(&unwatchable, fs::Permissions::from_mode(0o000)).unwrap(); + if fs::read_dir(&unwatchable).is_ok() { + return; // running as root, which can watch a directory it cannot read + } + + let root = ProjectRoot::new( + fs_util::canonicalize(AbsNormPathBuf::new(project.clone()).unwrap()).unwrap(), + ) + .unwrap(); + let cells = CellResolver::testing_with_name_and_path( + CellName::testing_new("root"), + CellRootPathBuf::testing_new(""), + ); + let watcher = NotifyFileWatcher::new(&root, cells, StdBuckHashMap::default()).unwrap(); + + // Moved in whole, so the walk it triggers is certain to meet the unwatchable directory. + let appearing = project.join("appearing"); + fs::rename(&staging, &appearing).unwrap(); + let missed = wait_for(&watcher, |data| data.missed_events); + // Before the asserts: a directory the test cannot read is one tempfile cannot remove. + fs::set_permissions( + appearing.join("unwatchable"), + fs::Permissions::from_mode(0o755), + ) + .unwrap(); + assert!( + missed, + "expected the failed watch to count as missed events" + ); + + // Registering again is what buys back the coverage the failure cost us. + watcher.reregister().unwrap(); + fs::write(appearing.join("readable").join("file"), "x").unwrap(); + assert!( + wait_for(&watcher, |data| data + .events + .iter() + .any(|(path, _)| path.to_string().ends_with("file"))), + "expected a change under the sibling of the unwatchable directory to be seen" + ); + } + } }