From d412fb10d7de44ffde39a8422c3ed404648fbbdf Mon Sep 17 00:00:00 2001 From: Florentin DUBOIS Date: Fri, 26 Jun 2026 18:38:42 +0200 Subject: [PATCH] terminal: Fix procfs file descriptor leak in PTY process tracking (#58683) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #58651 ## Problem On Linux, long-lived Zed sessions accumulate thousands of open procfs handles (`/proc//stat`, `/proc//task//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//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//task//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 --- crates/terminal/src/pty_info.rs | 98 ++++++++++++++++++++++++++++----- 1 file changed, 85 insertions(+), 13 deletions(-) diff --git a/crates/terminal/src/pty_info.rs b/crates/terminal/src/pty_info.rs index 576a833f01..8e98143248 100644 --- a/crates/terminal/src/pty_info.rs +++ b/crates/terminal/src/pty_info.rs @@ -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, refresh_kind: ProcessRefreshKind, pid_getter: ProcessIdGetter, + last_foreground_pid: Mutex>, pub(crate) current: RwLock>, task: Mutex>>, } 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//task//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> { 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//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> { @@ -206,3 +230,51 @@ impl PtyProcessInfo { self.pid_getter.pid() } } + +#[cfg(all(test, unix))] +mod tests { + use super::*; + + /// Regression test for : + /// on Linux, sysinfo keeps an open `/proc//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" + ); + } +}