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() + } +} 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" + ); + } + } } 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(); }); }