From a50c5b2c10fef326e691567400845d3e1d7fccfe Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Fri, 26 Dec 2025 20:47:53 +0200 Subject: [PATCH] Fix Zed OOM-ing when macOS file descriptors become invalid (#45669) Closes https://github.com/zed-industries/zed/issues/42845 Repro steps: https://github.com/zed-industries/zed/issues/42845#issuecomment-3687413958 Initial investigation and Zed memory trace: https://github.com/zed-industries/zed/issues/42845#issuecomment-3687877977 The PR consists of 2 commits: * [first](https://github.com/zed-industries/zed/pull/45669/changes/732d308c8d7e9af3649ac71ea65a9c029af820fc) adds cosmetic fixes to remove backtraces from logs yet again and print paths in quotes, as file descriptors may return empty paths. It also stubs the cause if OOM in project panel: that one traversed all worktrees in `for worktree_snapshot in visible_worktrees` and "accepted" the one with empty paths + never called `entry_iter.advance();` in "no file name found for the worktree" case, thus looping endlessly and bloating the memory quite fast. * [second](https://github.com/zed-industries/zed/pull/45669/changes/7ebfe5da2fc6d32f3fa2d71c761f8b2ec26d945b) adds something that resembles a fix: `fn current_path` on macOS used the file handler to re-fetch the worktree root file path on worktree root canonicalization failure. What's odd, is that `libc::fcntl` returns `0` in the case when external volume is not mounted, thus resulting in the `""` path string that is propagated all the way up. * [third](https://github.com/zed-industries/zed/pull/45669/changes/1a7560cef3e9fac604124c19f46b1f9c7b91815f) moves the fix down to the platform-related FS implementations The "fix" now checks the only usage of this method inside `async fn process_events` for an empty path and bails if that is the case. I am not sure what is a better fix, but this stops any memory leaks and given how bad the situation now, seems ok to merge for now with the `TODO` comment for more clever people to fix properly later. ---------------- Now, when I disconnect the SMB share and reconnect it again, Zed stops displaying any files in the project tree but the ones opened as editors. As before, at first, when the share is unmounted, Zed fails to save any changes because of the timeouts. Later, when the share is re-connected, macOS Finder hangs still but Zed starts to react on saves yet still only shows the files that are open as editors. The files can be edited and saved from now on. Later, when Finder finally stops hanging and indicates that the share is mounted fully, the rest of the file structure reappear in the project panel, and all file saves are propagated, hence can be observed in the share in Finder. It feels that one good improvement to add on top is some "disconnected" indicator that clearly shows that the file is not properly handles in the OS. This requires much more changes and thinking as nothing like that exists in Zed yet, hence not done. Release Notes: - Fixed Zed OOM-ing when macOS file descriptors become invalid --- crates/fs/src/fs.rs | 25 ++++++++++++----------- crates/fsevent/src/fsevent.rs | 2 +- crates/project/src/environment.rs | 12 +++++++++-- crates/project_panel/src/project_panel.rs | 1 + crates/worktree/src/worktree.rs | 18 ++++++++++------ 5 files changed, 37 insertions(+), 21 deletions(-) diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs index 3b2f318119fc3e1ff75bf19e30798aaf6630dfa7..61dd521cf79f1153dc561d3e092f048cf5ecb1fb 100644 --- a/crates/fs/src/fs.rs +++ b/crates/fs/src/fs.rs @@ -335,12 +335,11 @@ impl FileHandle for std::fs::File { let mut path_buf = MaybeUninit::<[u8; libc::PATH_MAX as usize]>::uninit(); let result = unsafe { libc::fcntl(fd.as_raw_fd(), libc::F_GETPATH, path_buf.as_mut_ptr()) }; - if result == -1 { - anyhow::bail!("fcntl returned -1".to_string()); - } + anyhow::ensure!(result != -1, "fcntl returned -1"); // SAFETY: `fcntl` will initialize the path buffer. let c_str = unsafe { CStr::from_ptr(path_buf.as_ptr().cast()) }; + anyhow::ensure!(!c_str.is_empty(), "Could find a path for the file handle"); let path = PathBuf::from(OsStr::from_bytes(c_str.to_bytes())); Ok(path) } @@ -372,12 +371,11 @@ impl FileHandle for std::fs::File { kif.kf_structsize = libc::KINFO_FILE_SIZE; let result = unsafe { libc::fcntl(fd.as_raw_fd(), libc::F_KINFO, kif.as_mut_ptr()) }; - if result == -1 { - anyhow::bail!("fcntl returned -1".to_string()); - } + anyhow::ensure!(result != -1, "fcntl returned -1"); // SAFETY: `fcntl` will initialize the kif. let c_str = unsafe { CStr::from_ptr(kif.assume_init().kf_path.as_ptr()) }; + anyhow::ensure!(!c_str.is_empty(), "Could find a path for the file handle"); let path = PathBuf::from(OsStr::from_bytes(c_str.to_bytes())); Ok(path) } @@ -398,18 +396,21 @@ impl FileHandle for std::fs::File { // Query required buffer size (in wide chars) let required_len = unsafe { GetFinalPathNameByHandleW(handle, &mut [], FILE_NAME_NORMALIZED) }; - if required_len == 0 { - anyhow::bail!("GetFinalPathNameByHandleW returned 0 length"); - } + anyhow::ensure!( + required_len != 0, + "GetFinalPathNameByHandleW returned 0 length" + ); // Allocate buffer and retrieve the path let mut buf: Vec = vec![0u16; required_len as usize + 1]; let written = unsafe { GetFinalPathNameByHandleW(handle, &mut buf, FILE_NAME_NORMALIZED) }; - if written == 0 { - anyhow::bail!("GetFinalPathNameByHandleW failed to write path"); - } + anyhow::ensure!( + written != 0, + "GetFinalPathNameByHandleW failed to write path" + ); let os_str: OsString = OsString::from_wide(&buf[..written as usize]); + anyhow::ensure!(!os_str.is_empty(), "Could find a path for the file handle"); Ok(PathBuf::from(os_str)) } } diff --git a/crates/fsevent/src/fsevent.rs b/crates/fsevent/src/fsevent.rs index 8af57c19ee242d62e3fe10fa4d4f3ea5cc945ebd..0fe3592010fa0e7e12b68c8dfc919807e91c8e64 100644 --- a/crates/fsevent/src/fsevent.rs +++ b/crates/fsevent/src/fsevent.rs @@ -76,7 +76,7 @@ impl EventStream { cf::CFRelease(cf_path); cf::CFRelease(cf_url); } else { - log::error!("Failed to create CFURL for path: {}", path.display()); + log::error!("Failed to create CFURL for path: {path:?}"); } } diff --git a/crates/project/src/environment.rs b/crates/project/src/environment.rs index c4e807621ee27374b970b956551575ca4ae94f8b..0d590d6adcb4ff5321de7c44bd419ebceaa80ee6 100644 --- a/crates/project/src/environment.rs +++ b/crates/project/src/environment.rs @@ -216,7 +216,7 @@ impl ProjectEnvironment { let shell = shell.clone(); let tx = self.environment_error_messages_tx.clone(); cx.spawn(async move |cx| { - let mut shell_env = cx + let mut shell_env = match cx .background_spawn(load_directory_shell_environment( shell, abs_path.clone(), @@ -224,7 +224,15 @@ impl ProjectEnvironment { tx, )) .await - .log_err(); + { + Ok(shell_env) => Some(shell_env), + Err(e) => { + log::error!( + "Failed to load shell environment for directory {abs_path:?}: {e:#}" + ); + None + } + }; if let Some(shell_env) = shell_env.as_mut() { let path = shell_env diff --git a/crates/project_panel/src/project_panel.rs b/crates/project_panel/src/project_panel.rs index 43f63d90789a65bce54814f3adbc6f1d53235568..3f650d98c13fb1baefba6c42ebf4333ad7f6e645 100644 --- a/crates/project_panel/src/project_panel.rs +++ b/crates/project_panel/src/project_panel.rs @@ -3599,6 +3599,7 @@ impl ProjectPanel { == worktree_snapshot.root_entry() { let Some(path_name) = worktree_abs_path.file_name() else { + entry_iter.advance(); continue; }; let depth = 0; diff --git a/crates/worktree/src/worktree.rs b/crates/worktree/src/worktree.rs index ca5500249e95f0b9d3fb25f75393ab5c9ca5be9b..7d682c0321569fa0255b740fb537bf8ac2076b87 100644 --- a/crates/worktree/src/worktree.rs +++ b/crates/worktree/src/worktree.rs @@ -3995,21 +3995,27 @@ impl BackgroundScanner { .snapshot .root_file_handle .clone() - .and_then(|handle| handle.current_path(&self.fs).log_err()) + .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().display(), - new_path.as_path().display() + "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::warn!("root path could not be canonicalized: {:#}", err); + log::error!("root path could not be canonicalized: {err:#}"); } return; } @@ -4457,7 +4463,7 @@ impl BackgroundScanner { Ok(Some(metadata)) => metadata, Ok(None) => continue, Err(err) => { - log::error!("error processing {child_abs_path:?}: {err:?}"); + log::error!("error processing {child_abs_path:?}: {err:#}"); continue; } };