diff --git a/crates/fs/src/fs_watcher.rs b/crates/fs/src/fs_watcher.rs index 99664ae74802e2..e01b3a316985ba 100644 --- a/crates/fs/src/fs_watcher.rs +++ b/crates/fs/src/fs_watcher.rs @@ -32,6 +32,15 @@ pub struct FsWatcher { struct FsWatcherRegistration { id: WatcherRegistrationId, mode: WatcherMode, + /// Inode of the path at the time it was registered. Used to detect when a + /// watched path has been replaced by a different file/directory (e.g. one + /// that was deleted and quickly recreated). On Linux, inotify silently + /// invalidates the kernel watch on the old inode when the directory is + /// removed, but this registration lingers; without re-registering, a later + /// `add` for the recreated path would be treated as already-watched and the + /// new inode would never be watched. `None` when the inode is unavailable + /// (non-Unix, or the stat failed), in which case the check is skipped. + inode: Option, } impl FsWatcher { @@ -52,9 +61,30 @@ impl FsWatcher { fn add_existing_path(&self, path: Arc) -> anyhow::Result<()> { let case_insensitive = case_insensitive_path(&path); let key = WatchKey::for_registration(SanitizedPath::new(&path), case_insensitive); - if self.registrations.lock().contains_key(&key) { - log::trace!("path to watch is already watched: {path:?}"); - return Ok(()); + // Bind the lookup to a local so the registrations lock is released before + // the block below re-acquires it; `parking_lot::Mutex` is not reentrant. + let existing = self.registrations.lock().get(&key).copied(); + if let Some(existing) = existing { + let current_inode = path_inode(&path); + // Treat the path as already watched unless the inode changed, which + // means the file/directory here was replaced (e.g. deleted and + // recreated). In that case inotify's watch on the old inode is dead, + // so drop the stale registration and fall through to register a + // fresh watch on the new inode. When either inode is unknown we can't + // tell, so conservatively keep the existing registration. + if current_inode.is_none() + || existing.inode.is_none() + || current_inode == existing.inode + { + log::trace!("path to watch is already watched: {path:?}"); + return Ok(()); + } + log::trace!( + "path {path:?} was recreated (inode {:?} -> {current_inode:?}); re-registering watch", + existing.inode, + ); + self.registrations.lock().remove(&key); + global_watcher().remove(existing.id); } match register_existing_path( path.clone(), @@ -210,12 +240,29 @@ pub fn requires_poll_watcher(path: &Path) -> bool { } } +/// The inode of `path` itself (a final symlink is not followed), or `None` when +/// the inode is unavailable (non-Unix platforms, or the stat failed). Used to +/// detect a path replaced by a newly-created inode. See [`FsWatcherRegistration::inode`]. +fn path_inode(path: &Path) -> Option { + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + std::fs::symlink_metadata(path).ok().map(|meta| meta.ino()) + } + #[cfg(not(unix))] + { + let _ = path; + None + } +} + fn register_existing_path( path: Arc, case_insensitive: bool, tx: async_channel::Sender<()>, pending_path_events: Arc>>, ) -> anyhow::Result> { + let inode = path_inode(path.as_ref()); let mode = if requires_poll_watcher(path.as_ref()) { log::info!( "Using poll watcher ({}ms interval) for {}", @@ -251,6 +298,7 @@ fn register_existing_path( Ok(Some(FsWatcherRegistration { id: registration_id, mode, + inode, })) } diff --git a/crates/worktree/src/worktree.rs b/crates/worktree/src/worktree.rs index 9ca1fa52abc780..bc6e20d5921ed4 100644 --- a/crates/worktree/src/worktree.rs +++ b/crates/worktree/src/worktree.rs @@ -5207,6 +5207,31 @@ impl BackgroundScanner { let mut root_canonical_path = None; let mut new_entries: Vec = Vec::new(); let mut new_jobs: Vec> = Vec::new(); + + // Establish the watch on this directory *before* enumerating its + // contents, so that any child created after the enumeration but before + // the watch would otherwise be active still produces an FS event and is + // not silently lost. Without this ordering, a directory that is deleted + // and quickly recreated while it is being repopulated (e.g. a build or + // test step that clears then rewrites an output directory) only reflects + // the entries that happened to exist at the instant of enumeration. For + // external entries we watch the canonical (resolved) path, matching the + // bookkeeping recorded after `populate_dir` below. See zed#53901. + let watched_abs_path: Option> = if job.is_external { + self.fs + .canonicalize(job.abs_path.as_ref()) + .await + .ok() + .map(|canonical| { + let canonical: Arc = canonical.into(); + self.watcher.add(&canonical).log_err(); + canonical + }) + } else { + self.watcher.add(job.abs_path.as_ref()).log_err(); + Some(job.abs_path.clone()) + }; + let mut child_paths = self .fs .read_dir(&job.abs_path) @@ -5418,33 +5443,13 @@ impl BackgroundScanner { } state.populate_dir(job.path.clone(), new_entries, new_ignore); - // For external entries, watch the canonical (resolved) path so OS-level - // FS events on the real filesystem location are observed. The same - // canonical path is stored in both `external_canonical_to_relative` - // (for translating canonical-path FS events back to worktree-relative - // paths) and `watched_dir_abs_paths_by_entry_id` (used by `remove_path` - // to know which abs path to unwatch), so both cleanup paths agree on - // the path the watcher was actually registered on. - // - // `canonicalize` is an async filesystem operation that may suspend, so - // the lock must not be held across the await point below. - drop(state); - let watched_abs_path: Option> = if job.is_external { - self.fs - .canonicalize(job.abs_path.as_ref()) - .await - .ok() - .map(|canonical| { - let canonical: Arc = canonical.into(); - self.watcher.add(&canonical).log_err(); - canonical - }) - } else { - self.watcher.add(job.abs_path.as_ref()).log_err(); - Some(job.abs_path.clone()) - }; - - let mut state = self.state.lock().await; + // The watch on this directory was already established before its + // contents were enumerated (see above). Record the bookkeeping that + // maps the watched abs path to this entry: `watched_dir_abs_paths_by_entry_id` + // is used by `remove_path` to know which abs path to unwatch, and (for + // external entries) `external_canonical_to_relative` translates + // canonical-path FS events back to worktree-relative paths. Both cleanup + // paths therefore agree on the path the watcher was registered on. if let Some(watched_abs_path) = &watched_abs_path { if job.is_external { state diff --git a/crates/worktree/tests/integration/worktree_tests.rs b/crates/worktree/tests/integration/worktree_tests.rs index a9a673d10e7a55..8bf6442353ee56 100644 --- a/crates/worktree/tests/integration/worktree_tests.rs +++ b/crates/worktree/tests/integration/worktree_tests.rs @@ -4712,6 +4712,110 @@ fn drain_git_repo_updates(events: &mut futures::channel::mpsc::UnboundedReceiver found } +// Regression test for https://github.com/zed-industries/zed/issues/53901 (the +// delete-then-recreate cause, distinct from the macOS fd-saturation cause): +// a directory that is removed and immediately recreated while its children are +// still being written (e.g. a build/test step that clears then repopulates an +// output directory) must end up with ALL of its children reflected in the +// worktree - not just the handful that happened to exist at the instant the +// recreated directory was rescanned. +#[gpui::test] +async fn test_rapid_delete_recreate_dir_shows_all_children(cx: &mut TestAppContext) { + cx.executor().allow_parking(); + init_test(cx); + + const COUNT: usize = 20; + + let fs = Arc::new(RealFs::new(None, cx.executor())); + let temp_root = TempTree::new(json!({ + "test_results": {}, + })); + let results_dir = temp_root.path().join("test_results"); + + // Simulate the first run: populate `test_results` with COUNT files + COUNT dirs. + for i in 0..COUNT { + fs.create_file( + &results_dir.join(format!("file{i}.txt")), + Default::default(), + ) + .await + .unwrap(); + fs.create_dir(&results_dir.join(format!("dir{i}"))) + .await + .unwrap(); + } + + let tree = Worktree::local( + temp_root.path(), + true, + fs.clone(), + Default::default(), + true, + WorktreeId::from_proto(0), + &mut cx.to_async(), + ) + .await + .unwrap(); + + cx.read(|cx| tree.read(cx).as_local().unwrap().scan_complete()) + .await; + + // Second run: remove the directory and *immediately* recreate + repopulate it, + // with no delay between the removal and recreation (the trigger), and each child + // created ~10ms apart so the burst spans more than FS_WATCH_LATENCY (100ms). + fs.remove_dir( + &results_dir, + RemoveOptions { + recursive: true, + ignore_if_not_exists: true, + }, + ) + .await + .unwrap(); + fs.create_dir(&results_dir).await.unwrap(); + for i in 0..COUNT { + cx.background_executor + .timer(std::time::Duration::from_millis(10)) + .await; + fs.create_file( + &results_dir.join(format!("file{i}.txt")), + Default::default(), + ) + .await + .unwrap(); + fs.create_dir(&results_dir.join(format!("dir{i}"))) + .await + .unwrap(); + } + + tree.flush_fs_events(cx).await; + + let missing = tree.read_with(cx, |tree, _| { + let mut missing = Vec::new(); + for i in 0..COUNT { + for name in [ + format!("test_results/file{i}.txt"), + format!("test_results/dir{i}"), + ] { + if tree + .entry_for_path(RelPath::from_unix_str(&name).unwrap()) + .is_none() + { + missing.push(name); + } + } + } + missing + }); + + assert!( + missing.is_empty(), + "{} of {} entries missing from the worktree after a rapid delete/recreate: {missing:?}", + missing.len(), + COUNT * 2, + ); +} + fn init_test(cx: &mut gpui::TestAppContext) { zlog::init_test();