Fix Zed OOM-ing when macOS file descriptors become invalid (#45669)

Kirill Bulatov created 2 days ago

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

Change summary

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(-)

Detailed changes

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<u16> = 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))
     }
 }

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:?}");
                 }
             }
 

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

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;

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;
                 }
             };