From d4541ec58607c5e55688802e71e79384a33b41e6 Mon Sep 17 00:00:00 2001 From: "zed-zippy[bot]" <234243425+zed-zippy[bot]@users.noreply.github.com> Date: Fri, 26 Dec 2025 18:59:08 +0000 Subject: [PATCH] Fix Zed OOM-ing when macOS file descriptors become invalid (#45669) (cherry-pick to preview) (#45700) Cherry-pick of #45669 to preview ---- 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 Co-authored-by: Kirill Bulatov --- 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 e6f69a14593a0246ae8ccb4aa4673f4e1f5a1e8e..d973a60cfa2f4b82004781028fc223afb52bfef8 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 ea667ecbb479ca347914ee11ec789a14f29cf474..c83b993875e2c0f8092545608b431dfa1f90bb4c 100644 --- a/crates/project_panel/src/project_panel.rs +++ b/crates/project_panel/src/project_panel.rs @@ -3485,6 +3485,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 e1ce31c038de9136109c3c8566e5e497dfa4f239..f986cb9f98973f3a27eeb8858b38b70bb0e39c1b 100644 --- a/crates/worktree/src/worktree.rs +++ b/crates/worktree/src/worktree.rs @@ -3887,21 +3887,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; } @@ -4322,7 +4328,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; } };