From b9f10c0adbdfee5639f9c7d0a87e7041bfd77e72 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Wed, 2 Apr 2025 13:36:28 -0700 Subject: [PATCH] Fix redundant FS file watches due to LSP path watching (#27957) Release Notes: - Fixed a bug where Zed sometimes added multiple redundant FS watchers when language servers requested to watch paths. This could cause saves and git operations to fail if Zed exceeded the file descriptor limit. --------- Co-authored-by: Piotr --- crates/fs/src/fs.rs | 83 ++++++--- crates/project/src/lsp_store.rs | 255 +++++++++++++--------------- crates/project/src/project_tests.rs | 111 +++++++++++- 3 files changed, 279 insertions(+), 170 deletions(-) diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs index 5c75c3cacb..272a05e9b8 100644 --- a/crates/fs/src/fs.rs +++ b/crates/fs/src/fs.rs @@ -861,7 +861,7 @@ struct FakeFsState { next_inode: u64, next_mtime: SystemTime, git_event_tx: smol::channel::Sender, - event_txs: Vec>>, + event_txs: Vec<(PathBuf, smol::channel::Sender>)>, events_paused: bool, buffered_events: Vec, metadata_call_count: usize, @@ -1013,7 +1013,7 @@ impl FakeFsState { fn flush_events(&mut self, mut count: usize) { count = count.min(self.buffered_events.len()); let events = self.buffered_events.drain(0..count).collect::>(); - self.event_txs.retain(|tx| { + self.event_txs.retain(|(_, tx)| { let _ = tx.try_send(events.clone()); !tx.is_closed() }); @@ -1112,7 +1112,7 @@ impl FakeFs { } pub async fn insert_file(&self, path: impl AsRef, content: Vec) { - self.write_file_internal(path, content).unwrap() + self.write_file_internal(path, content, true).unwrap() } pub async fn insert_symlink(&self, path: impl AsRef, target: PathBuf) { @@ -1134,30 +1134,50 @@ impl FakeFs { state.emit_event([(path, None)]); } - fn write_file_internal(&self, path: impl AsRef, content: Vec) -> Result<()> { + fn write_file_internal( + &self, + path: impl AsRef, + new_content: Vec, + recreate_inode: bool, + ) -> Result<()> { let mut state = self.state.lock(); - let file = Arc::new(Mutex::new(FakeFsEntry::File { - inode: state.get_and_increment_inode(), - mtime: state.get_and_increment_mtime(), - len: content.len() as u64, - content, - })); + let new_inode = state.get_and_increment_inode(); + let new_mtime = state.get_and_increment_mtime(); + let new_len = new_content.len() as u64; let mut kind = None; - state.write_path(path.as_ref(), { - let kind = &mut kind; - move |entry| { - match entry { - btree_map::Entry::Vacant(e) => { - *kind = Some(PathEventKind::Created); - e.insert(file); - } - btree_map::Entry::Occupied(mut e) => { - *kind = Some(PathEventKind::Changed); - *e.get_mut() = file; + state.write_path(path.as_ref(), |entry| { + match entry { + btree_map::Entry::Vacant(e) => { + kind = Some(PathEventKind::Created); + e.insert(Arc::new(Mutex::new(FakeFsEntry::File { + inode: new_inode, + mtime: new_mtime, + len: new_len, + content: new_content, + }))); + } + btree_map::Entry::Occupied(mut e) => { + kind = Some(PathEventKind::Changed); + if let FakeFsEntry::File { + inode, + mtime, + len, + content, + .. + } = &mut *e.get_mut().lock() + { + *mtime = new_mtime; + *content = new_content; + *len = new_len; + if recreate_inode { + *inode = new_inode; + } + } else { + anyhow::bail!("not a file") } } - Ok(()) } + Ok(()) })?; state.emit_event([(path.as_ref(), kind)]); Ok(()) @@ -1589,6 +1609,15 @@ impl FakeFs { self.state.lock().read_dir_call_count } + pub fn watched_paths(&self) -> Vec { + let state = self.state.lock(); + state + .event_txs + .iter() + .filter_map(|(path, tx)| Some(path.clone()).filter(|_| !tx.is_closed())) + .collect() + } + /// How many `metadata` calls have been issued. pub fn metadata_call_count(&self) -> usize { self.state.lock().metadata_call_count @@ -1765,7 +1794,7 @@ impl Fs for FakeFs { ) -> Result<()> { let mut bytes = Vec::new(); content.read_to_end(&mut bytes).await?; - self.write_file_internal(path, bytes)?; + self.write_file_internal(path, bytes, true)?; Ok(()) } @@ -1782,7 +1811,7 @@ impl Fs for FakeFs { let mut bytes = Vec::new(); entry.read_to_end(&mut bytes).await?; self.create_dir(path.parent().unwrap()).await?; - self.write_file_internal(&path, bytes)?; + self.write_file_internal(&path, bytes, true)?; } } Ok(()) @@ -1976,7 +2005,7 @@ impl Fs for FakeFs { async fn atomic_write(&self, path: PathBuf, data: String) -> Result<()> { self.simulate_random_delay().await; let path = normalize_path(path.as_path()); - self.write_file_internal(path, data.into_bytes())?; + self.write_file_internal(path, data.into_bytes(), true)?; Ok(()) } @@ -1987,7 +2016,7 @@ impl Fs for FakeFs { if let Some(path) = path.parent() { self.create_dir(path).await?; } - self.write_file_internal(path, content.into_bytes())?; + self.write_file_internal(path, content.into_bytes(), false)?; Ok(()) } @@ -2107,8 +2136,8 @@ impl Fs for FakeFs { ) { self.simulate_random_delay().await; let (tx, rx) = smol::channel::unbounded(); - self.state.lock().event_txs.push(tx); let path = path.to_path_buf(); + self.state.lock().event_txs.push((path.clone(), tx)); let executor = self.executor.clone(); ( Box::pin(futures::StreamExt::filter(rx, move |events| { diff --git a/crates/project/src/lsp_store.rs b/crates/project/src/lsp_store.rs index 24e0c80a12..3b27ce7190 100644 --- a/crates/project/src/lsp_store.rs +++ b/crates/project/src/lsp_store.rs @@ -3026,149 +3026,89 @@ impl LocalLspStore { language_server_id ); - enum PathToWatch { - Worktree { - literal_prefix: Arc, - pattern: String, - }, - Absolute { - path: Arc, - pattern: String, - }, - } for watcher in watchers { - let mut found_host = false; - for worktree in &worktrees { - let glob_is_inside_worktree = worktree.update(cx, |tree, _| { - let worktree_root_path = tree.abs_path(); - let path_to_watch = match &watcher.glob_pattern { - lsp::GlobPattern::String(s) => { - let watcher_path = SanitizedPath::from(s); - match watcher_path.as_path().strip_prefix(&worktree_root_path) { - Ok(relative) => { - let pattern = relative.to_string_lossy().to_string(); - let literal_prefix = glob_literal_prefix(relative).into(); - - PathToWatch::Worktree { - literal_prefix, - pattern, - } - } - Err(_) => { - let path = glob_literal_prefix(watcher_path.as_path()); - let pattern = watcher_path - .as_path() - .strip_prefix(&path) - .map(|p| p.to_string_lossy().to_string()) - .unwrap_or_else(|e| { - debug_panic!( - "Failed to strip prefix for string pattern: {}, with prefix: {}, with error: {}", - s, - path.display(), - e - ); - watcher_path.as_path().to_string_lossy().to_string() - }); - let path = if path.components().next().is_none() { - worktree_root_path.clone() - } else { - path.into() - }; - - PathToWatch::Absolute { path, pattern } - } - } - } - lsp::GlobPattern::Relative(rp) => { - let Ok(mut base_uri) = match &rp.base_uri { - lsp::OneOf::Left(workspace_folder) => &workspace_folder.uri, - lsp::OneOf::Right(base_uri) => base_uri, - } - .to_file_path() else { - return false; - }; - - match base_uri.strip_prefix(&worktree_root_path) { - Ok(relative) => { - let mut literal_prefix = relative.to_owned(); - literal_prefix - .push(glob_literal_prefix(Path::new(&rp.pattern))); - - PathToWatch::Worktree { - literal_prefix: literal_prefix.into(), - pattern: rp.pattern.clone(), - } - } - Err(_) => { - let path = glob_literal_prefix(Path::new(&rp.pattern)); - let pattern = Path::new(&rp.pattern) - .strip_prefix(&path) - .map(|p| p.to_string_lossy().to_string()) - .unwrap_or_else(|e| { - debug_panic!( - "Failed to strip prefix for relative pattern: {}, with prefix: {}, with error: {}", - rp.pattern, - path.display(), - e - ); - rp.pattern.clone() - }); - base_uri.push(path); - - let path = if base_uri.components().next().is_none() { - debug_panic!("base_uri is empty, {}", base_uri.display()); - worktree_root_path.clone() - } else { - base_uri.into() - }; - PathToWatch::Absolute { path, pattern } - } - } - } - }; - match path_to_watch { - PathToWatch::Worktree { - literal_prefix, - pattern, - } => { - if let Some((tree, glob)) = - tree.as_local_mut().zip(Glob::new(&pattern).log_err()) - { - tree.add_path_prefix_to_scan(literal_prefix); - worktree_globs - .entry(tree.id()) - .or_insert_with(GlobSetBuilder::new) - .add(glob); - } else { - return false; - } - } - PathToWatch::Absolute { path, pattern } => { - if let Some(glob) = Glob::new(&pattern).log_err() { - abs_globs - .entry(path) - .or_insert_with(GlobSetBuilder::new) - .add(glob); - } - } + if let Some((worktree, literal_prefix, pattern)) = + self.worktree_and_path_for_file_watcher(&worktrees, &watcher, cx) + { + worktree.update(cx, |worktree, _| { + if let Some((tree, glob)) = + worktree.as_local_mut().zip(Glob::new(&pattern).log_err()) + { + tree.add_path_prefix_to_scan(literal_prefix.into()); + worktree_globs + .entry(tree.id()) + .or_insert_with(GlobSetBuilder::new) + .add(glob); } - true }); - if glob_is_inside_worktree { - log::trace!( - "Watcher pattern `{}` has been attached to the worktree at `{}`", - serde_json::to_string(&watcher.glob_pattern).unwrap(), - worktree.read(cx).abs_path().display() - ); - found_host = true; + } else { + let (path, pattern) = match &watcher.glob_pattern { + lsp::GlobPattern::String(s) => { + let watcher_path = SanitizedPath::from(s); + let path = glob_literal_prefix(watcher_path.as_path()); + let pattern = watcher_path + .as_path() + .strip_prefix(&path) + .map(|p| p.to_string_lossy().to_string()) + .unwrap_or_else(|e| { + debug_panic!( + "Failed to strip prefix for string pattern: {}, with prefix: {}, with error: {}", + s, + path.display(), + e + ); + watcher_path.as_path().to_string_lossy().to_string() + }); + (path, pattern) + } + lsp::GlobPattern::Relative(rp) => { + let Ok(mut base_uri) = match &rp.base_uri { + lsp::OneOf::Left(workspace_folder) => &workspace_folder.uri, + lsp::OneOf::Right(base_uri) => base_uri, + } + .to_file_path() else { + continue; + }; + + let path = glob_literal_prefix(Path::new(&rp.pattern)); + let pattern = Path::new(&rp.pattern) + .strip_prefix(&path) + .map(|p| p.to_string_lossy().to_string()) + .unwrap_or_else(|e| { + debug_panic!( + "Failed to strip prefix for relative pattern: {}, with prefix: {}, with error: {}", + rp.pattern, + path.display(), + e + ); + rp.pattern.clone() + }); + base_uri.push(path); + (base_uri, pattern) + } + }; + + if let Some(glob) = Glob::new(&pattern).log_err() { + if !path + .components() + .any(|c| matches!(c, path::Component::Normal(_))) + { + // For an unrooted glob like `**/Cargo.toml`, watch it within each worktree, + // rather than adding a new watcher for `/`. + for worktree in &worktrees { + worktree_globs + .entry(worktree.read(cx).id()) + .or_insert_with(GlobSetBuilder::new) + .add(glob.clone()); + } + } else { + abs_globs + .entry(path.into()) + .or_insert_with(GlobSetBuilder::new) + .add(glob); + } } } - if !found_host { - log::error!( - "Watcher pattern `{}` has not been attached to any worktree or absolute path", - serde_json::to_string(&watcher.glob_pattern).unwrap() - ) - } } let mut watch_builder = LanguageServerWatchedPathsBuilder::default(); @@ -3185,6 +3125,45 @@ impl LocalLspStore { watch_builder } + fn worktree_and_path_for_file_watcher( + &self, + worktrees: &[Entity], + watcher: &FileSystemWatcher, + cx: &App, + ) -> Option<(Entity, PathBuf, String)> { + worktrees.iter().find_map(|worktree| { + let tree = worktree.read(cx); + let worktree_root_path = tree.abs_path(); + match &watcher.glob_pattern { + lsp::GlobPattern::String(s) => { + let watcher_path = SanitizedPath::from(s); + let relative = watcher_path + .as_path() + .strip_prefix(&worktree_root_path) + .ok()?; + let literal_prefix = glob_literal_prefix(relative); + Some(( + worktree.clone(), + literal_prefix, + relative.to_string_lossy().to_string(), + )) + } + lsp::GlobPattern::Relative(rp) => { + let base_uri = match &rp.base_uri { + lsp::OneOf::Left(workspace_folder) => &workspace_folder.uri, + lsp::OneOf::Right(base_uri) => base_uri, + } + .to_file_path() + .ok()?; + let relative = base_uri.strip_prefix(&worktree_root_path).ok()?; + let mut literal_prefix = relative.to_owned(); + literal_prefix.push(glob_literal_prefix(Path::new(&rp.pattern))); + Some((worktree.clone(), literal_prefix, rp.pattern.clone())) + } + } + }) + } + fn rebuild_watched_paths( &mut self, language_server_id: LanguageServerId, diff --git a/crates/project/src/project_tests.rs b/crates/project/src/project_tests.rs index d9ffd8590f..8c00b98423 100644 --- a/crates/project/src/project_tests.rs +++ b/crates/project/src/project_tests.rs @@ -27,7 +27,7 @@ use lsp::{ WillRenameFiles, notification::DidRenameFiles, }; use parking_lot::Mutex; -use paths::tasks_file; +use paths::{config_dir, tasks_file}; use postage::stream::Stream as _; use pretty_assertions::{assert_eq, assert_matches}; use serde_json::json; @@ -919,6 +919,7 @@ async fn test_reporting_fs_changes_to_language_servers(cx: &mut gpui::TestAppCon path!("/the-root"), json!({ ".gitignore": "target\n", + "Cargo.lock": "", "src": { "a.rs": "", "b.rs": "", @@ -943,9 +944,37 @@ async fn test_reporting_fs_changes_to_language_servers(cx: &mut gpui::TestAppCon }), ) .await; + fs.insert_tree( + path!("/the-registry"), + json!({ + "dep1": { + "src": { + "dep1.rs": "", + } + }, + "dep2": { + "src": { + "dep2.rs": "", + } + }, + }), + ) + .await; + fs.insert_tree( + path!("/the/stdlib"), + json!({ + "LICENSE": "", + "src": { + "string.rs": "", + } + }), + ) + .await; let project = Project::test(fs.clone(), [path!("/the-root").as_ref()], cx).await; - let language_registry = project.read_with(cx, |project, _| project.languages().clone()); + let (language_registry, lsp_store) = project.read_with(cx, |project, _| { + (project.languages().clone(), project.lsp_store()) + }); language_registry.add(rust_lang()); let mut fake_servers = language_registry.register_fake_lsp( "Rust", @@ -978,6 +1007,7 @@ async fn test_reporting_fs_changes_to_language_servers(cx: &mut gpui::TestAppCon &[ (Path::new(""), false), (Path::new(".gitignore"), false), + (Path::new("Cargo.lock"), false), (Path::new("src"), false), (Path::new("src/a.rs"), false), (Path::new("src/b.rs"), false), @@ -988,8 +1018,26 @@ async fn test_reporting_fs_changes_to_language_servers(cx: &mut gpui::TestAppCon let prev_read_dir_count = fs.read_dir_call_count(); - // Keep track of the FS events reported to the language server. let fake_server = fake_servers.next().await.unwrap(); + let (server_id, server_name) = lsp_store.read_with(cx, |lsp_store, _| { + let (id, status) = lsp_store.language_server_statuses().next().unwrap(); + (id, LanguageServerName::from(status.name.as_str())) + }); + + // Simulate jumping to a definition in a dependency outside of the worktree. + let _out_of_worktree_buffer = project + .update(cx, |project, cx| { + project.open_local_buffer_via_lsp( + lsp::Url::from_file_path(path!("/the-registry/dep1/src/dep1.rs")).unwrap(), + server_id, + server_name.clone(), + cx, + ) + }) + .await + .unwrap(); + + // Keep track of the FS events reported to the language server. let file_changes = Arc::new(Mutex::new(Vec::new())); fake_server .request::(lsp::RegistrationParams { @@ -1017,6 +1065,18 @@ async fn test_reporting_fs_changes_to_language_servers(cx: &mut gpui::TestAppCon ), kind: None, }, + lsp::FileSystemWatcher { + glob_pattern: lsp::GlobPattern::String( + path!("/the/stdlib/src/**/*.rs").to_string(), + ), + kind: None, + }, + lsp::FileSystemWatcher { + glob_pattern: lsp::GlobPattern::String( + path!("**/Cargo.lock").to_string(), + ), + kind: None, + }, ], }, ) @@ -1036,12 +1096,23 @@ async fn test_reporting_fs_changes_to_language_servers(cx: &mut gpui::TestAppCon cx.executor().run_until_parked(); assert_eq!(mem::take(&mut *file_changes.lock()), &[]); - assert_eq!(fs.read_dir_call_count() - prev_read_dir_count, 4); + assert_eq!(fs.read_dir_call_count() - prev_read_dir_count, 5); + + let mut new_watched_paths = fs.watched_paths(); + new_watched_paths.retain(|path| !path.starts_with(config_dir())); + assert_eq!( + &new_watched_paths, + &[ + Path::new(path!("/the-root")), + Path::new(path!("/the-registry/dep1/src/dep1.rs")), + Path::new(path!("/the/stdlib/src")) + ] + ); // Now the language server has asked us to watch an ignored directory path, // so we recursively load it. project.update(cx, |project, cx| { - let worktree = project.worktrees(cx).next().unwrap(); + let worktree = project.visible_worktrees(cx).next().unwrap(); assert_eq!( worktree .read(cx) @@ -1052,6 +1123,7 @@ async fn test_reporting_fs_changes_to_language_servers(cx: &mut gpui::TestAppCon &[ (Path::new(""), false), (Path::new(".gitignore"), false), + (Path::new("Cargo.lock"), false), (Path::new("src"), false), (Path::new("src/a.rs"), false), (Path::new("src/b.rs"), false), @@ -1088,12 +1160,37 @@ async fn test_reporting_fs_changes_to_language_servers(cx: &mut gpui::TestAppCon ) .await .unwrap(); + fs.save( + path!("/the-root/Cargo.lock").as_ref(), + &"".into(), + Default::default(), + ) + .await + .unwrap(); + fs.save( + path!("/the-stdlib/LICENSE").as_ref(), + &"".into(), + Default::default(), + ) + .await + .unwrap(); + fs.save( + path!("/the/stdlib/src/string.rs").as_ref(), + &"".into(), + Default::default(), + ) + .await + .unwrap(); // The language server receives events for the FS mutations that match its watch patterns. cx.executor().run_until_parked(); assert_eq!( &*file_changes.lock(), &[ + lsp::FileEvent { + uri: lsp::Url::from_file_path(path!("/the-root/Cargo.lock")).unwrap(), + typ: lsp::FileChangeType::CHANGED, + }, lsp::FileEvent { uri: lsp::Url::from_file_path(path!("/the-root/src/b.rs")).unwrap(), typ: lsp::FileChangeType::DELETED, @@ -1106,6 +1203,10 @@ async fn test_reporting_fs_changes_to_language_servers(cx: &mut gpui::TestAppCon uri: lsp::Url::from_file_path(path!("/the-root/target/y/out/y2.rs")).unwrap(), typ: lsp::FileChangeType::CREATED, }, + lsp::FileEvent { + uri: lsp::Url::from_file_path(path!("/the/stdlib/src/string.rs")).unwrap(), + typ: lsp::FileChangeType::CHANGED, + }, ] ); }