From ed98e7ca80758a0423f867da26a35f2e808ed657 Mon Sep 17 00:00:00 2001 From: Cole Miller Date: Mon, 7 Sep 2026 15:03:38 +0000 Subject: [PATCH] Use fewer file descriptors on macOS to detect worktree root path changes (#63863) Previously, on macOS, we set the [WatchRoot flag](https://developer.apple.com/documentation/coreservices/kfseventstreamcreateflagwatchroot) to `FsEventStreamCreate` (via our notify fork). This causes the resulting stream to produce an event when any segment of the watched path changes due to a rename (e.g. `/Users/cole/zed/crates/gpui` -> `/Users/cole/zed2/crates/gpui`). The cost of this is that for each path we watch on macOS, we hold open file descriptors for that path and all its parents (and these are not deduplicated across watched paths). This especially blows up our file descriptor usage with non-visible worktrees from GTD jumps: ``` zed 25415 cole 721r REG 1,14 99826 168007500 /Users/cole/.rustup/toolchains/1.97.1-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/option.rs zed 25415 cole 722r DIR 1,14 2432 168008864 /Users/cole/.rustup/toolchains/1.97.1-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src zed 25415 cole 723r DIR 1,14 128 168008863 /Users/cole/.rustup/toolchains/1.97.1-aarch64-apple-darwin/lib/rustlib/src/rust/library/core zed 25415 cole 724r DIR 1,14 928 168008723 /Users/cole/.rustup/toolchains/1.97.1-aarch64-apple-darwin/lib/rustlib/src/rust/library zed 25415 cole 725r DIR 1,14 128 168008722 /Users/cole/.rustup/toolchains/1.97.1-aarch64-apple-darwin/lib/rustlib/src/rust zed 25415 cole 726r DIR 1,14 96 168008721 /Users/cole/.rustup/toolchains/1.97.1-aarch64-apple-darwin/lib/rustlib/src zed 25415 cole 727r DIR 1,14 704 168004176 /Users/cole/.rustup/toolchains/1.97.1-aarch64-apple-darwin/lib/rustlib zed 25415 cole 728r DIR 1,14 256 168004175 /Users/cole/.rustup/toolchains/1.97.1-aarch64-apple-darwin/lib zed 25415 cole 729r DIR 1,14 224 168004164 /Users/cole/.rustup/toolchains/1.97.1-aarch64-apple-darwin zed 25415 cole 730r DIR 1,14 576 292591 /Users/cole/.rustup/toolchains zed 25415 cole 731r DIR 1,14 224 292480 /Users/cole/.rustup zed 25415 cole 732r DIR 1,14 8576 43880 /Users/cole zed 25415 cole 733r DIR 1,14 160 18221 /Users ``` This PR bumps our notify dependency so that we no longer set that flag, and instead just wake the background scanner loop every 5s to check whether the worktree root path has changed. Note that both before and after this change, the process_events arm of that loop checks the worktree root path in the same way before doing anything else, so there is no risk of descending further into the background scanner code with an incorrect root path. Release Notes: - Made Zed use fewer file descriptors on macOS. --- Cargo.lock | 4 +- Cargo.toml | 4 +- crates/fs/src/fs.rs | 34 ++--- crates/fs/tests/integration/fs_tests.rs | 10 +- crates/worktree/src/worktree.rs | 125 +++++++++++------- .../tests/integration/worktree_tests.rs | 64 +++++++++ 6 files changed, 170 insertions(+), 71 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2f63722843..4029b426c5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11410,7 +11410,7 @@ dependencies = [ [[package]] name = "notify" version = "9.0.0-rc.4" -source = "git+https://github.com/zed-industries/notify?rev=0890bbb8ca40a4b5d1f67031698dd7918b37d991#0890bbb8ca40a4b5d1f67031698dd7918b37d991" +source = "git+https://github.com/zed-industries/notify?rev=d842f16b2716bd60f09caf3ae3a894237ab38f54#d842f16b2716bd60f09caf3ae3a894237ab38f54" dependencies = [ "bitflags 2.13.1", "inotify 0.11.0", @@ -11454,7 +11454,7 @@ dependencies = [ [[package]] name = "notify-types" version = "2.1.0" -source = "git+https://github.com/zed-industries/notify?rev=0890bbb8ca40a4b5d1f67031698dd7918b37d991#0890bbb8ca40a4b5d1f67031698dd7918b37d991" +source = "git+https://github.com/zed-industries/notify?rev=d842f16b2716bd60f09caf3ae3a894237ab38f54#d842f16b2716bd60f09caf3ae3a894237ab38f54" dependencies = [ "bitflags 2.13.1", ] diff --git a/Cargo.toml b/Cargo.toml index 8c4c71f044..f3d5d49741 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -982,8 +982,8 @@ windows-capture = { git = "https://github.com/zed-industries/windows-capture.git calloop = { git = "https://github.com/zed-industries/calloop" } livekit = { git = "https://github.com/zed-industries/livekit-rust-sdks", rev = "0a1c519cfce9b365229026b55de9b9dbdb6fed3c" } libwebrtc = { git = "https://github.com/zed-industries/livekit-rust-sdks", rev = "0a1c519cfce9b365229026b55de9b9dbdb6fed3c" } -notify = { git = "https://github.com/zed-industries/notify", rev = "0890bbb8ca40a4b5d1f67031698dd7918b37d991" } -notify-types = { git = "https://github.com/zed-industries/notify", rev = "0890bbb8ca40a4b5d1f67031698dd7918b37d991" } +notify = { git = "https://github.com/zed-industries/notify", rev = "d842f16b2716bd60f09caf3ae3a894237ab38f54" } +notify-types = { git = "https://github.com/zed-industries/notify", rev = "d842f16b2716bd60f09caf3ae3a894237ab38f54" } webrtc-sys = { git = "https://github.com/zed-industries/livekit-rust-sdks", rev = "0a1c519cfce9b365229026b55de9b9dbdb6fed3c" } # corgi runs build.rs/proc-macros in a sandbox with no write access outside the diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs index e592934a4a..f128b7e579 100644 --- a/crates/fs/src/fs.rs +++ b/crates/fs/src/fs.rs @@ -1438,7 +1438,6 @@ struct FakeFsState { metadata_call_count: usize, read_dir_call_count: usize, path_write_counts: std::collections::HashMap, - moves: std::collections::HashMap, job_event_subscribers: Arc>>, trash: Mutex>, file_to_create_before_watch_add: Option<(PathBuf, PathBuf)>, @@ -1772,7 +1771,6 @@ impl FakeFs { read_dir_call_count: 0, metadata_call_count: 0, path_write_counts: Default::default(), - moves: Default::default(), job_event_subscribers: Arc::new(Mutex::new(Vec::new())), trash: Mutex::new(SlotMap::with_key()), file_to_create_before_watch_add: None, @@ -2869,13 +2867,23 @@ struct FakeHandle { impl FileHandle for FakeHandle { fn current_path(&self, fs: &Arc) -> Result { let fs = fs.as_fake(); - let mut state = fs.state.lock(); - let Some(target) = state.moves.get(&self.inode).cloned() else { - anyhow::bail!("fake fd not moved") - }; - - if state.try_entry(&target, false).is_some() { - return Ok(target); + let state = fs.state.lock(); + let mut queue = collections::VecDeque::new(); + queue.push_back((PathBuf::from(util::path!("/")), &state.root)); + while let Some((path, entry)) = queue.pop_front() { + match entry { + FakeFsEntry::File { inode, .. } | FakeFsEntry::Dir { inode, .. } + if *inode == self.inode => + { + return Ok(path); + } + FakeFsEntry::Dir { entries, .. } => { + for (name, entry) in entries { + queue.push_back((path.join(name), entry)); + } + } + _ => {} + } } anyhow::bail!("fake fd target not found") } @@ -3030,12 +3038,6 @@ impl Fs for FakeFs { return Ok(()); } - let inode = match moved_entry { - FakeFsEntry::File { inode, .. } => inode, - FakeFsEntry::Dir { inode, .. } => inode, - _ => 0, - }; - let mut moved = true; state.write_path(&new_path, |e| { match e { @@ -3062,8 +3064,6 @@ impl Fs for FakeFs { return Ok(()); } - state.moves.insert(inode, new_path.clone()); - state .write_path(&old_path, |e| { if let btree_map::Entry::Occupied(e) = e { diff --git a/crates/fs/tests/integration/fs_tests.rs b/crates/fs/tests/integration/fs_tests.rs index a18aef6edd..9b7fc979cd 100644 --- a/crates/fs/tests/integration/fs_tests.rs +++ b/crates/fs/tests/integration/fs_tests.rs @@ -631,11 +631,11 @@ async fn test_fake_fs_rename_ignore_if_exists_leaves_source_and_target_unchanged "from target" ); - // An ignored rename must not be recorded as a move either, or a handle held - // across it reports a path its file never went to. - assert!( - handle.current_path(&(fs.clone() as Arc)).is_err(), - "an ignored rename should not record a move" + // A handle held across an ignored rename must keep reporting the path the + // file is actually at, not the one it never went to. + assert_eq!( + handle.current_path(&(fs.clone() as Arc)).unwrap(), + PathBuf::from(path!("/root/source.txt")) ); } diff --git a/crates/worktree/src/worktree.rs b/crates/worktree/src/worktree.rs index bffbe72449..9d218c7741 100644 --- a/crates/worktree/src/worktree.rs +++ b/crates/worktree/src/worktree.rs @@ -82,6 +82,12 @@ pub use worktree_settings::WorktreeSettings; pub const FS_WATCH_LATENCY: Duration = Duration::from_millis(100); +/// How often the background scanner verifies that the worktree root still +/// exists at its recorded path. Native watchers report the root itself being +/// renamed or deleted, but not a rename of one of its ancestors, which leaves +/// the watcher silently attached to a path that no longer exists. +pub const ROOT_PATH_CHECK_INTERVAL: Duration = Duration::from_secs(5); + /// A set of local or remote files that are being opened as part of a project. /// Responsible for tracking related FS (for local)/collab (for remote) events and corresponding updates. /// Stores git repositories data and the diagnostics for the file(s). @@ -4558,6 +4564,10 @@ impl BackgroundScanner { // Continue processing events until the worktree is dropped. self.phase = BackgroundScannerPhase::Events; + let root_path_check_timer = self.executor.timer(ROOT_PATH_CHECK_INTERVAL).fuse(); + futures::pin_mut!(root_path_check_timer); + let mut root_path_missing = false; + loop { select_biased! { // Process any path refresh requests from the worktree. Prioritize @@ -4612,6 +4622,20 @@ impl BackgroundScanner { self.update_global_gitignore(&path).await; } } + + _ = root_path_check_timer => { + let root_path = self.state.lock().await.snapshot.abs_path.clone(); + match self.fs.canonicalize(root_path.as_path()).await { + Ok(_) => root_path_missing = false, + Err(error) => { + if !root_path_missing { + root_path_missing = true; + self.report_root_moved_or_deleted(&root_path, &error).await; + } + } + } + root_path_check_timer.set(self.executor.timer(ROOT_PATH_CHECK_INTERVAL).fuse()); + } } } } @@ -4746,56 +4770,67 @@ impl BackgroundScanner { events } + /// Called when the worktree root can no longer be canonicalized. Uses the + /// open handle on the root to find where it moved to, and reports the new + /// location (or, for single-file worktrees, the deletion) to the worktree. + async fn report_root_moved_or_deleted( + &self, + root_path: &Arc, + canonicalize_error: &anyhow::Error, + ) { + let new_path = self + .state + .lock() + .await + .snapshot + .root_file_handle + .clone() + .and_then(|handle| match handle.current_path(&self.fs) { + Ok(new_path) => Some(new_path), + Err(e) => { + log::error!("Failed to refresh worktree root path: {e:#}"); + None + } + }) + .map(|path| SanitizedPath::new_arc(&path)) + .filter(|new_path| new_path != root_path); + + if let Some(new_path) = new_path { + log::info!( + "root renamed from {:?} to {:?}", + root_path.as_path(), + new_path.as_path(), + ); + self.status_updates_tx + .unbounded_send(ScanState::RootUpdated { new_path }) + .ok(); + } else { + log::error!("root path could not be canonicalized: {canonicalize_error:#}"); + + // For single-file worktrees, if we can't canonicalize and the file handle + // fallback also failed, the file is gone - close the worktree + if self.is_single_file { + log::info!( + "single-file worktree root {:?} no longer exists, marking as deleted", + root_path.as_path() + ); + self.status_updates_tx + .unbounded_send(ScanState::RootDeleted) + .ok(); + } + } + } + async fn process_events(&self, mut events: Vec) { let root_path = self.state.lock().await.snapshot.abs_path.clone(); - let root_canonical_path = self.fs.canonicalize(root_path.as_path()).await; - let root_canonical_path = match &root_canonical_path { - Ok(path) => SanitizedPath::new(path), - Err(err) => { - let new_path = self - .state - .lock() - .await - .snapshot - .root_file_handle - .clone() - .and_then(|handle| match handle.current_path(&self.fs) { - Ok(new_path) => Some(new_path), - Err(e) => { - log::error!("Failed to refresh worktree root path: {e:#}"); - None - } - }) - .map(|path| SanitizedPath::new_arc(&path)) - .filter(|new_path| *new_path != root_path); - - if let Some(new_path) = new_path { - log::info!( - "root renamed from {:?} to {:?}", - root_path.as_path(), - new_path.as_path(), - ); - self.status_updates_tx - .unbounded_send(ScanState::RootUpdated { new_path }) - .ok(); - } else { - log::error!("root path could not be canonicalized: {err:#}"); - - // For single-file worktrees, if we can't canonicalize and the file handle - // fallback also failed, the file is gone - close the worktree - if self.is_single_file { - log::info!( - "single-file worktree root {:?} no longer exists, marking as deleted", - root_path.as_path() - ); - self.status_updates_tx - .unbounded_send(ScanState::RootDeleted) - .ok(); - } - } + let root_canonical_path = match self.fs.canonicalize(root_path.as_path()).await { + Ok(path) => path, + Err(error) => { + self.report_root_moved_or_deleted(&root_path, &error).await; return; } }; + let root_canonical_path = SanitizedPath::new(&root_canonical_path); { let state = self.state.lock().await; diff --git a/crates/worktree/tests/integration/worktree_tests.rs b/crates/worktree/tests/integration/worktree_tests.rs index be84ed1436..8f6476bbde 100644 --- a/crates/worktree/tests/integration/worktree_tests.rs +++ b/crates/worktree/tests/integration/worktree_tests.rs @@ -5949,6 +5949,70 @@ async fn test_single_file_worktree_deleted(cx: &mut TestAppContext) { ); } +#[gpui::test] +async fn test_root_ancestor_rename_is_detected_without_fs_events(cx: &mut TestAppContext) { + init_test(cx); + let fs = FakeFs::new(cx.background_executor.clone()); + fs.insert_tree( + path!("/code"), + json!({ + "project": { + "src": { + "main.rs": "fn main() {}", + }, + }, + }), + ) + .await; + + let tree = Worktree::local( + Path::new(path!("/code/project")), + 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; + + // Renaming an ancestor of the root produces no event under the watched + // path, so only the periodic root check can notice it. + fs.rename( + Path::new(path!("/code")), + Path::new(path!("/src")), + Default::default(), + ) + .await + .unwrap(); + cx.background_executor.run_until_parked(); + tree.read_with(cx, |tree, _| { + assert_eq!(tree.abs_path().as_ref(), Path::new(path!("/code/project"))); + }); + + cx.background_executor + .advance_clock(worktree::ROOT_PATH_CHECK_INTERVAL); + cx.background_executor.run_until_parked(); + + tree.read_with(cx, |tree, _| { + assert_eq!(tree.abs_path().as_ref(), Path::new(path!("/src/project"))); + assert_eq!(tree.root_name(), "project"); + assert_eq!( + tree.entries(false, 0) + .map(|entry| entry.path.clone()) + .collect::>(), + vec![ + rel_path("").into(), + rel_path("src").into(), + rel_path("src/main.rs").into(), + ] + ); + }); +} + #[gpui::test] async fn test_remote_worktree_without_git_emits_root_repo_event_after_first_update( cx: &mut TestAppContext,