terminal: Fix procfs file descriptor leak in PTY process tracking (#58683)
Closes #58651 ## Problem On Linux, long-lived Zed sessions accumulate thousands of open procfs handles (`/proc/<pid>/stat`, `/proc/<pid>/task/<tid>/stat`). A degraded 6-hour session was measured holding **9,675 FDs, 97% of them never-closed procfs stat handles**, alongside system-wide clipboard breakage (Wayland clipboard `send()` stretching from 13 ms to ~17 s, which makes every other client abort the paste). The leak comes from `PtyProcessInfo` in `crates/terminal/src/pty_info.rs`, through three interacting sysinfo 0.37 behaviors: 1. **Hidden full-system snapshot per terminal.** `System::new_with_specifics(RefreshKind::nothing().with_processes(…))` immediately refreshes **all** processes. Each refreshed process gets a `Process` entry that caches an **open `/proc/<pid>/stat` file** for the lifetime of the entry (a documented sysinfo optimization). 2. **Task enumeration is opt-out.** `ProcessRefreshKind::nothing()` keeps `tasks: true`, so that constructor snapshot also creates one entry — and one open `/proc/<pid>/task/<tid>/stat` — **per thread on the machine**, per terminal. 3. **No eviction outside the refreshed set.** The recurring refresh uses `ProcessesToUpdate::Some(&[foreground_pid])`; sysinfo's dead-process sweep only visits pids in that list. Every former foreground process (one per command the user runs) therefore stays in the map forever, pinning its FD. sysinfo silently raises `RLIMIT_NOFILE` and budgets half of it (~262k) for this cache, so the process never hits `EMFILE` — it just degrades. Reproduction (standalone, sysinfo 0.37.2, Fedora 44): constructing one `System` the way `PtyProcessInfo::new` does instantly retains **3,566 FDs**; each simulated shell command leaks one more, unbounded. ## Fix All changes confined to `pty_info.rs`: - `.without_tasks()` on the `ProcessRefreshKind` — eliminates the per-thread handle class entirely. - Replace the constructor's implicit all-process scan with `System::new()` plus a refresh of just the spawned child, so `kill_child_process()` still works before the first foreground refresh. - Rebuild the `System` whenever the foreground pid changes, and refresh `[foreground, child]` so the dead-process sweep can actually evict stale entries. The map is now bounded at 2 entries per terminal. With the fix, the same repro stays flat at ~6 FDs across arbitrary command churn, and the foreground process info (name/cwd/argv for tab titles) keeps working. The regression test fails on `main` (3,747 retained entries on a desktop system) and passes with the fix (≤ 2). Note: the ~17 s clipboard `send()` on the degraded session is a coupled symptom; the FD/process-map bloat is the verified defect fixed here. The Wayland data-source lifecycle is tracked separately in #58446. Release Notes: - Fixed a file descriptor leak in terminal process tracking that degraded long-running sessions on Linux, including system-wide clipboard breakage ([#58651](https://github.com/zed-industries/zed/issues/58651)). Signed-off-by: Florentin Dubois <florentin.dubois@clever.cloud>
This commit is contained in:
1 parent
aa0cfcd190
commit
d412fb10d7
1 file changed
+85
-13
@@ -5,7 +5,7 @@ use std::{path::PathBuf, sync::Arc};
|
||||
#[cfg(target_os = "windows")]
|
||||
use windows::Win32::{Foundation::HANDLE, System::Threading::GetProcessId};
|
||||
|
||||
use sysinfo::{Pid, Process, ProcessRefreshKind, RefreshKind, System, UpdateKind};
|
||||
use sysinfo::{Pid, Process, ProcessRefreshKind, ProcessesToUpdate, System, UpdateKind};
|
||||
|
||||
use crate::{Event, Terminal};
|
||||
|
||||
@@ -77,23 +77,38 @@ pub(crate) struct PtyProcessInfo {
|
||||
system: RwLock<System>,
|
||||
refresh_kind: ProcessRefreshKind,
|
||||
pid_getter: ProcessIdGetter,
|
||||
last_foreground_pid: Mutex<Option<Pid>>,
|
||||
pub(crate) current: RwLock<Option<ProcessInfo>>,
|
||||
task: Mutex<Option<Task<()>>>,
|
||||
}
|
||||
|
||||
impl PtyProcessInfo {
|
||||
pub(crate) fn new(pid_getter: ProcessIdGetter) -> PtyProcessInfo {
|
||||
// Task enumeration is on by default and would retain a `Process` entry
|
||||
// per thread, each pinning an open `/proc/<pid>/task/<tid>/stat` handle
|
||||
// on Linux (#58651).
|
||||
let process_refresh_kind = ProcessRefreshKind::nothing()
|
||||
.with_cmd(UpdateKind::Always)
|
||||
.with_cwd(UpdateKind::Always)
|
||||
.with_exe(UpdateKind::Always);
|
||||
let refresh_kind = RefreshKind::nothing().with_processes(process_refresh_kind);
|
||||
let system = System::new_with_specifics(refresh_kind);
|
||||
.with_exe(UpdateKind::Always)
|
||||
.without_tasks();
|
||||
// `System::new_with_specifics` with a process refresh kind would
|
||||
// snapshot every process on the machine into this terminal's `System`,
|
||||
// retaining one open procfs handle per process for the lifetime of the
|
||||
// terminal (#58651). Refresh only the spawned child so that
|
||||
// `kill_child_process` works before the first foreground refresh.
|
||||
let mut system = System::new();
|
||||
system.refresh_processes_specifics(
|
||||
ProcessesToUpdate::Some(&[pid_getter.fallback_pid()]),
|
||||
true,
|
||||
process_refresh_kind,
|
||||
);
|
||||
|
||||
PtyProcessInfo {
|
||||
system: RwLock::new(system),
|
||||
refresh_kind: process_refresh_kind,
|
||||
pid_getter,
|
||||
last_foreground_pid: Mutex::new(None),
|
||||
current: RwLock::new(None),
|
||||
task: Mutex::new(None),
|
||||
}
|
||||
@@ -105,16 +120,25 @@ impl PtyProcessInfo {
|
||||
|
||||
fn refresh(&self) -> Option<MappedRwLockReadGuard<'_, Process>> {
|
||||
let pid = self.pid_getter.pid()?;
|
||||
if self.system.write().refresh_processes_specifics(
|
||||
sysinfo::ProcessesToUpdate::Some(&[pid]),
|
||||
true,
|
||||
self.refresh_kind,
|
||||
) == 1
|
||||
{
|
||||
RwLockReadGuard::try_map(self.system.read(), |system| system.process(pid)).ok()
|
||||
} else {
|
||||
None
|
||||
let fallback_pid = self.pid_getter.fallback_pid();
|
||||
let mut system = self.system.write();
|
||||
// sysinfo never evicts processes that are absent from the refreshed pid
|
||||
// set, so entries for former foreground processes (each pinning an open
|
||||
// `/proc/<pid>/stat` handle on Linux) would otherwise accumulate for as
|
||||
// long as this terminal lives (#58651). Rebuild the `System` whenever
|
||||
// the foreground process changes to keep the map bounded.
|
||||
if self.last_foreground_pid.lock().replace(pid) != Some(pid) {
|
||||
*system = System::new();
|
||||
}
|
||||
let pids = [pid, fallback_pid];
|
||||
let pids = if pid == fallback_pid {
|
||||
&pids[..1]
|
||||
} else {
|
||||
&pids[..]
|
||||
};
|
||||
system.refresh_processes_specifics(ProcessesToUpdate::Some(pids), true, self.refresh_kind);
|
||||
drop(system);
|
||||
RwLockReadGuard::try_map(self.system.read(), |system| system.process(pid)).ok()
|
||||
}
|
||||
|
||||
fn get_child(&self) -> Option<MappedRwLockReadGuard<'_, Process>> {
|
||||
@@ -206,3 +230,51 @@ impl PtyProcessInfo {
|
||||
self.pid_getter.pid()
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(all(test, unix))]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
/// Regression test for <https://github.com/zed-industries/zed/issues/58651>:
|
||||
/// on Linux, sysinfo keeps an open `/proc/<pid>/stat` handle for every
|
||||
/// `Process` entry retained in a `System`, and never evicts entries that are
|
||||
/// absent from the refreshed pid set. The per-terminal `System` must
|
||||
/// therefore not snapshot every process on the machine, nor accumulate an
|
||||
/// entry per foreground process that has ever run in this terminal.
|
||||
#[test]
|
||||
#[allow(
|
||||
clippy::disallowed_methods,
|
||||
reason = "the test needs real short-lived child processes and may block"
|
||||
)]
|
||||
fn process_map_stays_bounded() {
|
||||
let mut info = PtyProcessInfo::new(ProcessIdGetter::new(-1, std::process::id()));
|
||||
assert!(
|
||||
info.get_child().is_some(),
|
||||
"the spawned child must be inspectable for kill_child_process \
|
||||
before the first foreground refresh"
|
||||
);
|
||||
assert!(info.load_for_test().is_some());
|
||||
let initial_len = info.system.read().processes().len();
|
||||
assert!(
|
||||
initial_len <= 2,
|
||||
"creating a terminal retained {initial_len} process entries"
|
||||
);
|
||||
|
||||
for _ in 0..3 {
|
||||
let mut child = std::process::Command::new("sleep")
|
||||
.arg("30")
|
||||
.spawn()
|
||||
.expect("failed to spawn child process");
|
||||
info.pid_getter = ProcessIdGetter::new(-1, child.id());
|
||||
assert!(info.load_for_test().is_some());
|
||||
child.kill().expect("failed to kill child process");
|
||||
child.wait().expect("failed to wait for child process");
|
||||
}
|
||||
|
||||
let churned_len = info.system.read().processes().len();
|
||||
assert!(
|
||||
churned_len <= 2,
|
||||
"foreground process churn retained {churned_len} process entries"
|
||||
);
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user