Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 51 additions & 3 deletions crates/fs/src/fs_watcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<u64>,
}

impl FsWatcher {
Expand All @@ -52,9 +61,30 @@ impl FsWatcher {
fn add_existing_path(&self, path: Arc<Path>) -> 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(),
Expand Down Expand Up @@ -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<u64> {
#[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<Path>,
case_insensitive: bool,
tx: async_channel::Sender<()>,
pending_path_events: Arc<Mutex<Vec<PathEvent>>>,
) -> anyhow::Result<Option<FsWatcherRegistration>> {
let inode = path_inode(path.as_ref());
let mode = if requires_poll_watcher(path.as_ref()) {
log::info!(
"Using poll watcher ({}ms interval) for {}",
Expand Down Expand Up @@ -251,6 +298,7 @@ fn register_existing_path(
Ok(Some(FsWatcherRegistration {
id: registration_id,
mode,
inode,
}))
}

Expand Down
59 changes: 32 additions & 27 deletions crates/worktree/src/worktree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5207,6 +5207,31 @@ impl BackgroundScanner {
let mut root_canonical_path = None;
let mut new_entries: Vec<Entry> = Vec::new();
let mut new_jobs: Vec<Option<ScanJob>> = 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<Arc<Path>> = if job.is_external {
self.fs
.canonicalize(job.abs_path.as_ref())
.await
.ok()
.map(|canonical| {
let canonical: Arc<Path> = 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)
Expand Down Expand Up @@ -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<Arc<Path>> = if job.is_external {
self.fs
.canonicalize(job.abs_path.as_ref())
.await
.ok()
.map(|canonical| {
let canonical: Arc<Path> = 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
Expand Down
104 changes: 104 additions & 0 deletions crates/worktree/tests/integration/worktree_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
Loading