Skip to content

Commit a50c5b2

Browse files
Fix Zed OOM-ing when macOS file descriptors become invalid (zed-industries#45669)
Closes zed-industries#42845 Repro steps: zed-industries#42845 (comment) Initial investigation and Zed memory trace: zed-industries#42845 (comment) The PR consists of 2 commits: * [first](zed-industries@732d308) 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](zed-industries@7ebfe5d) 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](zed-industries@1a7560c) 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
1 parent f1b7239 commit a50c5b2

File tree

5 files changed

+37
-21
lines changed

5 files changed

+37
-21
lines changed

crates/fs/src/fs.rs

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -335,12 +335,11 @@ impl FileHandle for std::fs::File {
335335
let mut path_buf = MaybeUninit::<[u8; libc::PATH_MAX as usize]>::uninit();
336336

337337
let result = unsafe { libc::fcntl(fd.as_raw_fd(), libc::F_GETPATH, path_buf.as_mut_ptr()) };
338-
if result == -1 {
339-
anyhow::bail!("fcntl returned -1".to_string());
340-
}
338+
anyhow::ensure!(result != -1, "fcntl returned -1");
341339

342340
// SAFETY: `fcntl` will initialize the path buffer.
343341
let c_str = unsafe { CStr::from_ptr(path_buf.as_ptr().cast()) };
342+
anyhow::ensure!(!c_str.is_empty(), "Could find a path for the file handle");
344343
let path = PathBuf::from(OsStr::from_bytes(c_str.to_bytes()));
345344
Ok(path)
346345
}
@@ -372,12 +371,11 @@ impl FileHandle for std::fs::File {
372371
kif.kf_structsize = libc::KINFO_FILE_SIZE;
373372

374373
let result = unsafe { libc::fcntl(fd.as_raw_fd(), libc::F_KINFO, kif.as_mut_ptr()) };
375-
if result == -1 {
376-
anyhow::bail!("fcntl returned -1".to_string());
377-
}
374+
anyhow::ensure!(result != -1, "fcntl returned -1");
378375

379376
// SAFETY: `fcntl` will initialize the kif.
380377
let c_str = unsafe { CStr::from_ptr(kif.assume_init().kf_path.as_ptr()) };
378+
anyhow::ensure!(!c_str.is_empty(), "Could find a path for the file handle");
381379
let path = PathBuf::from(OsStr::from_bytes(c_str.to_bytes()));
382380
Ok(path)
383381
}
@@ -398,18 +396,21 @@ impl FileHandle for std::fs::File {
398396
// Query required buffer size (in wide chars)
399397
let required_len =
400398
unsafe { GetFinalPathNameByHandleW(handle, &mut [], FILE_NAME_NORMALIZED) };
401-
if required_len == 0 {
402-
anyhow::bail!("GetFinalPathNameByHandleW returned 0 length");
403-
}
399+
anyhow::ensure!(
400+
required_len != 0,
401+
"GetFinalPathNameByHandleW returned 0 length"
402+
);
404403

405404
// Allocate buffer and retrieve the path
406405
let mut buf: Vec<u16> = vec![0u16; required_len as usize + 1];
407406
let written = unsafe { GetFinalPathNameByHandleW(handle, &mut buf, FILE_NAME_NORMALIZED) };
408-
if written == 0 {
409-
anyhow::bail!("GetFinalPathNameByHandleW failed to write path");
410-
}
407+
anyhow::ensure!(
408+
written != 0,
409+
"GetFinalPathNameByHandleW failed to write path"
410+
);
411411

412412
let os_str: OsString = OsString::from_wide(&buf[..written as usize]);
413+
anyhow::ensure!(!os_str.is_empty(), "Could find a path for the file handle");
413414
Ok(PathBuf::from(os_str))
414415
}
415416
}

crates/fsevent/src/fsevent.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ impl EventStream {
7676
cf::CFRelease(cf_path);
7777
cf::CFRelease(cf_url);
7878
} else {
79-
log::error!("Failed to create CFURL for path: {}", path.display());
79+
log::error!("Failed to create CFURL for path: {path:?}");
8080
}
8181
}
8282

crates/project/src/environment.rs

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -216,15 +216,23 @@ impl ProjectEnvironment {
216216
let shell = shell.clone();
217217
let tx = self.environment_error_messages_tx.clone();
218218
cx.spawn(async move |cx| {
219-
let mut shell_env = cx
219+
let mut shell_env = match cx
220220
.background_spawn(load_directory_shell_environment(
221221
shell,
222222
abs_path.clone(),
223223
load_direnv,
224224
tx,
225225
))
226226
.await
227-
.log_err();
227+
{
228+
Ok(shell_env) => Some(shell_env),
229+
Err(e) => {
230+
log::error!(
231+
"Failed to load shell environment for directory {abs_path:?}: {e:#}"
232+
);
233+
None
234+
}
235+
};
228236

229237
if let Some(shell_env) = shell_env.as_mut() {
230238
let path = shell_env

crates/project_panel/src/project_panel.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3599,6 +3599,7 @@ impl ProjectPanel {
35993599
== worktree_snapshot.root_entry()
36003600
{
36013601
let Some(path_name) = worktree_abs_path.file_name() else {
3602+
entry_iter.advance();
36023603
continue;
36033604
};
36043605
let depth = 0;

crates/worktree/src/worktree.rs

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3995,21 +3995,27 @@ impl BackgroundScanner {
39953995
.snapshot
39963996
.root_file_handle
39973997
.clone()
3998-
.and_then(|handle| handle.current_path(&self.fs).log_err())
3998+
.and_then(|handle| match handle.current_path(&self.fs) {
3999+
Ok(new_path) => Some(new_path),
4000+
Err(e) => {
4001+
log::error!("Failed to refresh worktree root path: {e:#}");
4002+
None
4003+
}
4004+
})
39994005
.map(|path| SanitizedPath::new_arc(&path))
40004006
.filter(|new_path| *new_path != root_path);
40014007

40024008
if let Some(new_path) = new_path {
40034009
log::info!(
4004-
"root renamed from {} to {}",
4005-
root_path.as_path().display(),
4006-
new_path.as_path().display()
4010+
"root renamed from {:?} to {:?}",
4011+
root_path.as_path(),
4012+
new_path.as_path(),
40074013
);
40084014
self.status_updates_tx
40094015
.unbounded_send(ScanState::RootUpdated { new_path })
40104016
.ok();
40114017
} else {
4012-
log::warn!("root path could not be canonicalized: {:#}", err);
4018+
log::error!("root path could not be canonicalized: {err:#}");
40134019
}
40144020
return;
40154021
}
@@ -4457,7 +4463,7 @@ impl BackgroundScanner {
44574463
Ok(Some(metadata)) => metadata,
44584464
Ok(None) => continue,
44594465
Err(err) => {
4460-
log::error!("error processing {child_abs_path:?}: {err:?}");
4466+
log::error!("error processing {child_abs_path:?}: {err:#}");
44614467
continue;
44624468
}
44634469
};

0 commit comments

Comments
 (0)