From ce76309065d511f4ec083cf52d6b2589e69c4c39 Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 5 Jul 2026 10:06:00 +0000 Subject: [PATCH 1/4] fix(pi-shell): pinned spawn observer identity against Windows pid reuse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SpawnRegistry` previously stored only the raw pid reported by brush's `SpawnObserver` hook and deferred `Process::from_pid` to `build_targets` at cancellation time. Between the moment a bash-spawned child exited and the moment cancellation fired, Windows could recycle that freed pid onto an unrelated process — typically another `pwsh.exe` or `powershell.exe` in a different Cursor terminal tab, since PowerShell is the parent shell. `Process::from_pid` at kill time then opened the impostor, and `signal_tree` walked the current Toolhelp snapshot for `ppid == root` matches and `TerminateProcess`'d whatever subtree happened to live under that recycled pid. That is the reporter's Variant A symptom: OMP crashing kills unrelated PowerShell sessions. The `SpawnObserver` impl now pins a stable `Process` handle *at spawn time*, before any pid recycling window can open: - Windows: an open process handle keeps the pid slot reserved for the handle's lifetime (Raymond Chen's documented invariant), so the pid cannot be reassigned while the registry holds a reference. - Linux: the pidfd carries identity independent of the numeric pid. - macOS: the recorded `(pid, start_tvsec, start_tvusec)` triple detects impersonation on every subsequent access. `TerminationTargets::add_process` accepts a pre-pinned handle and skips the `Process::from_pid` re-open entirely, and `build_targets` no longer consults the raw pid at all — an entry the observer failed to pin (the child exited before we could open a handle) becomes a no-op instead of racing pid reuse. Fixes #4605 --- crates/pi-shell/src/process.rs | 137 ++++++++++++++++++++++++++++++--- crates/pi-shell/src/shell.rs | 9 ++- packages/natives/CHANGELOG.md | 4 + 3 files changed, 139 insertions(+), 11 deletions(-) diff --git a/crates/pi-shell/src/process.rs b/crates/pi-shell/src/process.rs index ede0fd3f8..4eccb80ad 100644 --- a/crates/pi-shell/src/process.rs +++ b/crates/pi-shell/src/process.rs @@ -1571,6 +1571,11 @@ impl TerminationTargets { /// Record a pid. Duplicates are ignored. If the pid is alive, opens /// a stable [`Process`] reference so the descendant tree can be /// killed even if the original pid is reused later. + /// + /// Prefer [`add_process`](Self::add_process) when the caller already holds a + /// [`Process`] captured at spawn time: opening by pid here loses the + /// original identity if the pid was recycled between the child exiting + /// and this call. pub fn add_pid(&mut self, pid: i32) { if self.seen_pids.insert(pid) && let Some(process) = Process::from_pid(pid) @@ -1579,6 +1584,17 @@ impl TerminationTargets { } } + /// Record a pre-pinned [`Process`] handle. Duplicates (by pid) are ignored. + /// + /// This is the correct entry point when the caller captured the handle at + /// spawn time — the handle already pins OS-level identity, so no `from_pid` + /// re-open (and its PID-reuse race) is needed at cancellation time. + pub fn add_process(&mut self, process: Process) { + if self.seen_pids.insert(process.pid()) { + self.processes.push(process); + } + } + /// True when no targets have been recorded. #[must_use] pub const fn is_empty(&self) -> bool { @@ -1599,10 +1615,19 @@ impl TerminationTargets { } /// A single external child reported by the shell's spawn-observer hook. -#[derive(Debug, Clone, Copy)] +/// +/// `process` is captured *at spawn time* so its OS-level identity is pinned +/// before the pid can be recycled. On Windows an open process handle keeps +/// the pid reserved for the lifetime of the reference; on Linux the pidfd +/// pins identity; on macOS the recorded `(pid, start_time)` triple detects +/// impersonation. Storing only the raw pid and re-opening at cancellation +/// time — as previous versions did — leaked kills onto unrelated processes +/// that happened to acquire the recycled pid between the child exiting and +/// the run being cancelled (issue #4605). +#[derive(Clone)] struct SpawnedProcess { - pid: i32, - pgid: Option, + process: Option, + pgid: Option, } /// Per-run record of the OS processes a single shell command launched, @@ -1626,24 +1651,37 @@ impl SpawnRegistry { } /// Record a freshly spawned child. Called from the spawn-observer hook. - pub fn record(&self, pid: i32, pgid: Option) { - self.spawned.lock().push(SpawnedProcess { pid, pgid }); + /// + /// The `Process` handle MUST be opened by the caller *immediately* after + /// the child's pid becomes visible, so identity is pinned before any race + /// with pid recycling can start. When the pin fails (child already exited + /// before we could `Process::from_pid`) the entry becomes a no-op at + /// termination time — there is nothing left to signal. + pub fn record(&self, pgid: Option, process: Option) { + self.spawned.lock().push(SpawnedProcess { process, pgid }); } /// Build the kill set from the processes recorded so far. Re-read on every /// signal wave so a child spawned during a grace window — between the /// cancel firing and the next wave — is still reaped. /// - /// A recorded pid contributes only while alive (`add_pid` opens a stable - /// handle, skipping the dead); a recorded pgid contributes only while the - /// group still has members, so once the run's whole tree exits the targets - /// are empty and the wave loop can stop early. + /// A recorded process contributes only while alive; a recorded pgid + /// contributes only while the group still has members, so once the run's + /// whole tree exits the targets are empty and the wave loop can stop early. #[must_use] pub fn build_targets(&self) -> TerminationTargets { let mut targets = TerminationTargets::new(); let spawned = self.spawned.lock().clone(); for entry in spawned { - targets.add_pid(entry.pid); + if let Some(process) = entry.process { + targets.add_process(process); + } + // If the observer failed to pin a handle at spawn time (the child + // exited before `Process::from_pid` could open it), the child is + // already gone — signalling anything for that pid would either + // no-op or, worse, race a recycled pid onto an unrelated process. + // Drop the entry entirely rather than reintroduce the pid-reuse + // window this whole change exists to close (#4605). if let Some(pgid) = entry.pgid && pgid > 0 && process_group_alive(pgid) @@ -1752,4 +1790,83 @@ mod tests { broken `proc_listchildpids`", ); } + + /// Regression test for issue #4605: `SpawnRegistry` MUST pin a stable + /// [`Process`] reference at spawn time rather than defer re-opening the + /// pid until termination. + /// + /// Before the fix, `SpawnRegistry` stored only the raw pid; `build_targets` + /// called `Process::from_pid` at cancellation time. On Windows pids recycle + /// aggressively, so a bash-spawned `pwsh.exe` that had already exited could + /// see its pid reassigned to an unrelated PowerShell session (e.g. the + /// user's other Cursor terminal). `Process::from_pid` at cancel time would + /// happily open that unrelated process, and `signal_tree` would then + /// enumerate — and `TerminateProcess` — the entire foreign subtree. + /// + /// This test cannot literally trigger Windows pid recycling from a + /// cross-platform Rust test, but it can prove the observable defense: a + /// recorded process reference survives the original pid's death (so no + /// "look it up again" step exists to be raced), and the registry never + /// consults `Process::from_pid` when a handle was pinned at record time. + #[cfg(unix)] + #[test] + fn spawn_registry_pins_identity_at_record_time() { + use std::{process::Command, thread, time::Duration}; + + let mut child = Command::new("sleep") + .arg("30") + .spawn() + .expect("spawn sleep"); + let child_pid = i32::try_from(child.id()).expect("child pid fits in i32"); + + let registry = SpawnRegistry::new(); + // Simulate brush's `on_spawn` hook: pin the handle *now*, while the + // child is definitely alive, before any race with pid recycling could + // start. + let pinned = Process::from_pid(child_pid).expect("pin child at record time"); + registry.record(None, Some(pinned)); + + // Kill the child so the pid becomes eligible for reuse. + let _ = child.kill(); + let _ = child.wait(); + + // Sanity-check that a fresh `Process::from_pid` against the dead pid + // no longer resolves — mirroring the moment on Windows where the pid + // could recycle to an unrelated process before cancellation fires. + for _ in 0..40 { + if Process::from_pid(child_pid).is_none() { + break; + } + thread::sleep(Duration::from_millis(25)); + } + + let targets = registry.build_targets(); + assert!( + !targets.is_empty(), + "SpawnRegistry must retain the pinned handle even after the pid dies; otherwise the \ + kill set is either silently empty (misses legitimate targets) or would need to \ + re-open by pid (racing with pid reuse — issue #4605)" + ); + } + + /// `TerminationTargets::add_process` must accept a pre-pinned handle + /// without going through `Process::from_pid`. This is the API contract + /// `SpawnRegistry` relies on to avoid the PID-reuse race. + #[cfg(unix)] + #[test] + fn add_process_bypasses_from_pid_lookup() { + let self_pid = i32::try_from(std::process::id()).expect("self pid fits in i32"); + let pinned = Process::from_pid(self_pid).expect("pin self"); + + let mut targets = TerminationTargets::new(); + targets.add_process(pinned.clone()); + assert!(!targets.is_empty(), "add_process must record the pinned handle"); + + // Adding the same pid again through either entry point must dedupe: + // otherwise every wave in `terminate_run` would re-signal the same + // tree N times. + targets.add_process(pinned); + targets.add_pid(self_pid); + assert_eq!(targets.processes.len(), 1, "duplicate pids must be deduped"); + } } diff --git a/crates/pi-shell/src/shell.rs b/crates/pi-shell/src/shell.rs index f00982a92..80634bf91 100644 --- a/crates/pi-shell/src/shell.rs +++ b/crates/pi-shell/src/shell.rs @@ -1372,7 +1372,14 @@ async fn read_output_bytes( impl SpawnObserver for process::SpawnRegistry { fn on_spawn(&self, pid: i32, pgid: Option) { - self.record(pid, pgid); + // Pin a stable process reference *now*, before the pid can be recycled. + // On Windows an open handle keeps the pid slot reserved for the lifetime + // of the handle; on Linux the pidfd carries identity; on macOS the + // recorded start-time triple detects impersonation. Deferring the open + // to `build_targets` (as the old code did) let a recycled pid resolve + // to an unrelated process — issue #4605. + let process = process::Process::from_pid(pid); + self.record(pgid, process); } } diff --git a/packages/natives/CHANGELOG.md b/packages/natives/CHANGELOG.md index d12104af1..7f029b3a6 100644 --- a/packages/natives/CHANGELOG.md +++ b/packages/natives/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed a Windows regression where an abnormal `omp` exit or bash cancellation could `TerminateProcess` unrelated `pwsh.exe` / `powershell.exe` sessions (including other Cursor terminal tabs). `SpawnRegistry` stored only the raw pid of each brush-spawned child and re-opened it via `Process::from_pid` at cancellation time; between those two moments Windows could recycle a freed pid onto an unrelated PowerShell, and `signal_tree` then walked the wrong subtree via Toolhelp. The observer now pins a stable `Process` handle at spawn time — on Windows the open handle keeps the pid slot reserved, on Linux the pidfd carries identity, on macOS the `(pid, start_time)` triple detects impersonation — so cancellation can only reach children this run actually launched. ([#4605](https://github.com/can1357/oh-my-pi/issues/4605)) + ## [16.3.6] - 2026-07-04 ### Changed From 79e083124c78c1879d3800ab274527f2d42a93d3 Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 5 Jul 2026 10:14:07 +0000 Subject: [PATCH 2/4] fix(pi-shell): pruned exited entries from SpawnRegistry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the review on #4606: pinning a stable `Process` per spawn is correct against pid reuse, but a long-running shell command that spawns many short-lived external processes (a bash loop invoking a binary per iteration) would otherwise retain one owned OS handle per historical spawn — a pidfd on Linux, a process `HANDLE` on Windows — until the run ends. Under enough iterations that hits the per-process FD/handle limit and starts breaking `Process::from_pid` (or any other file operation) for the rest of the run. `SpawnRegistry` now sweeps entries whose pinned process and process group are both gone. The sweep runs opportunistically inside `record` once the recorded vec crosses a small threshold (`PRUNE_THRESHOLD = 64`) and unconditionally at the top of `build_targets`, so the retained handle count is bounded by the current live tree rather than the historical spawn count. Amortized cost per spawn stays O(1); a sweep is O(N) probes of `Process::status` (non-blocking pidfd `poll` on Linux, `WaitForSingle Object(_, 0)` on Windows), running at most once per `PRUNE_THRESHOLD` records. The previous identity-pinning regression test was rewritten to defend the actual invariant — the pinned handle carries into `build_targets` while the child is alive, and the entry is dropped (never re-opened by pid) once the child exits. A new regression asserts pruning keeps retained entries under `PRUNE_THRESHOLD` after 2×threshold spawns of short-lived children. Fixes #4605 --- crates/pi-shell/src/process.rs | 160 ++++++++++++++++++++++++++++----- packages/natives/CHANGELOG.md | 2 +- 2 files changed, 140 insertions(+), 22 deletions(-) diff --git a/crates/pi-shell/src/process.rs b/crates/pi-shell/src/process.rs index 4eccb80ad..d9aea6b07 100644 --- a/crates/pi-shell/src/process.rs +++ b/crates/pi-shell/src/process.rs @@ -1644,6 +1644,19 @@ pub struct SpawnRegistry { } impl SpawnRegistry { + /// Amortized-cost threshold for opportunistic pruning of exited entries. + /// + /// A shell run that spawns many short-lived external commands (e.g. a bash + /// loop invoking a binary per iteration) would otherwise retain one owned + /// process handle per spawn — a pidfd on Linux, a `HANDLE` on Windows — for + /// the lifetime of the run, exhausting per-process FD/handle limits. When + /// the recorded vec grows past this many entries, `record` sweeps out + /// entries whose pinned process AND process group are both gone. Cost per + /// sweep is `O(N)` (one non-blocking status probe per entry) and a sweep + /// runs at most once per this many `record` calls, so the amortized cost + /// per spawn is `O(1)` — cheap next to the spawn itself. + const PRUNE_THRESHOLD: usize = 64; + /// Create an empty registry. #[must_use] pub fn new() -> Self { @@ -1657,8 +1670,17 @@ impl SpawnRegistry { /// with pid recycling can start. When the pin fails (child already exited /// before we could `Process::from_pid`) the entry becomes a no-op at /// termination time — there is nothing left to signal. + /// + /// Once the recorded vec reaches [`Self::PRUNE_THRESHOLD`], exited entries + /// are swept so long-running loops of short external commands cannot + /// exhaust the process' FD/handle limit by retaining one owned handle per + /// historical spawn. pub fn record(&self, pgid: Option, process: Option) { - self.spawned.lock().push(SpawnedProcess { process, pgid }); + let mut spawned = self.spawned.lock(); + spawned.push(SpawnedProcess { process, pgid }); + if spawned.len() >= Self::PRUNE_THRESHOLD { + prune_exited(&mut spawned); + } } /// Build the kill set from the processes recorded so far. Re-read on every @@ -1668,10 +1690,17 @@ impl SpawnRegistry { /// A recorded process contributes only while alive; a recorded pgid /// contributes only while the group still has members, so once the run's /// whole tree exits the targets are empty and the wave loop can stop early. + /// + /// Pruning also runs here so a cancellation cycle sees a compact target + /// set even when the record-time threshold hasn't fired yet. #[must_use] pub fn build_targets(&self) -> TerminationTargets { let mut targets = TerminationTargets::new(); - let spawned = self.spawned.lock().clone(); + let spawned = { + let mut guard = self.spawned.lock(); + prune_exited(&mut guard); + guard.clone() + }; for entry in spawned { if let Some(process) = entry.process { targets.add_process(process); @@ -1693,6 +1722,24 @@ impl SpawnRegistry { } } +/// Drop registry entries whose pinned process *and* process group are both +/// gone: with neither still-live, they contribute nothing to the next +/// termination wave and only pin an owned OS handle for no reason. A pgid-only +/// entry (observer failed to pin the process but the group is still alive) is +/// retained so `build_targets` can still signal the group. +fn prune_exited(spawned: &mut Vec) { + spawned.retain(|entry| { + let process_live = entry + .process + .as_ref() + .is_some_and(|process| process.status() == ProcessStatus::Running); + let group_live = entry + .pgid + .is_some_and(|pgid| pgid > 0 && process_group_alive(pgid)); + process_live || group_live + }); +} + /// True when process group `pgid` still has at least one member. `kill(2)` /// with signal 0 performs permission/existence checks without delivering a /// signal; `EPERM` means the group exists but is not ours to signal, which @@ -1813,39 +1860,52 @@ mod tests { fn spawn_registry_pins_identity_at_record_time() { use std::{process::Command, thread, time::Duration}; - let mut child = Command::new("sleep") + // Phase 1: while the child is alive, the pinned handle carries identity + // forward into `build_targets` without any `Process::from_pid` re-open + // step existing to be raced against pid reuse. + let mut long = Command::new("sleep") .arg("30") .spawn() .expect("spawn sleep"); - let child_pid = i32::try_from(child.id()).expect("child pid fits in i32"); + let long_pid = i32::try_from(long.id()).expect("child pid fits in i32"); let registry = SpawnRegistry::new(); - // Simulate brush's `on_spawn` hook: pin the handle *now*, while the - // child is definitely alive, before any race with pid recycling could - // start. - let pinned = Process::from_pid(child_pid).expect("pin child at record time"); + let pinned = Process::from_pid(long_pid).expect("pin child at record time"); registry.record(None, Some(pinned)); - // Kill the child so the pid becomes eligible for reuse. - let _ = child.kill(); - let _ = child.wait(); + let live_targets = registry.build_targets(); + assert!( + !live_targets.is_empty(), + "a still-live pinned child must appear in the target set — otherwise the \ + cancellation cleanup would silently miss it" + ); + let live_pids: Vec = live_targets.processes.iter().map(Process::pid).collect(); + assert_eq!( + live_pids, + vec![long_pid], + "target set must come from the pinned handle recorded at spawn time, not a \ + re-lookup by pid (which would race pid reuse — issue #4605)" + ); - // Sanity-check that a fresh `Process::from_pid` against the dead pid - // no longer resolves — mirroring the moment on Windows where the pid - // could recycle to an unrelated process before cancellation fires. + let _ = long.kill(); + let _ = long.wait(); + + // Phase 2: once the child exits, the registry MUST drop the entry + // rather than reintroduce a `Process::from_pid` re-open at kill time. + // Poll until pruning sees the pidfd as Exited (kernel-visible within + // milliseconds in practice). + let mut empty_after_exit = false; for _ in 0..40 { - if Process::from_pid(child_pid).is_none() { + if registry.build_targets().is_empty() { + empty_after_exit = true; break; } thread::sleep(Duration::from_millis(25)); } - - let targets = registry.build_targets(); assert!( - !targets.is_empty(), - "SpawnRegistry must retain the pinned handle even after the pid dies; otherwise the \ - kill set is either silently empty (misses legitimate targets) or would need to \ - re-open by pid (racing with pid reuse — issue #4605)" + empty_after_exit, + "once the pinned child exits the registry must drop it — re-opening by pid at \ + termination time is exactly the pid-reuse race #4605 closes" ); } @@ -1869,4 +1929,62 @@ mod tests { targets.add_pid(self_pid); assert_eq!(targets.processes.len(), 1, "duplicate pids must be deduped"); } + + /// Regression test for the review on PR #4606: a long-running shell + /// command that spawns many short-lived external processes must not + /// retain one owned handle per historical spawn — that would exhaust + /// per-process FD/handle limits (pidfd on Linux, `HANDLE` on Windows). + /// The registry MUST prune dead entries once the recorded vec crosses + /// the sweep threshold. + #[cfg(unix)] + #[test] + fn spawn_registry_prunes_exited_entries() { + use std::{thread, time::Duration}; + + let registry = SpawnRegistry::new(); + + // Fabricate many recorded-then-exited children by pinning ourselves, + // pushing the entry, then immediately treating it as "dead" from the + // registry's perspective. To simulate the exit without actually + // killing the harness, use `Process::from_pid(1)` for a pid that + // (on Linux) is init and never exits — but wrap the recording in a + // pattern that guarantees `status()` returns Exited for the pruner: + // spawn a tiny child, pin it, wait for exit, then record. + for _ in 0..(SpawnRegistry::PRUNE_THRESHOLD * 2) { + let mut child = std::process::Command::new("true") + .spawn() + .expect("spawn true"); + let pid = i32::try_from(child.id()).expect("child pid fits in i32"); + let pinned = Process::from_pid(pid); + let _ = child.wait(); + // Give the kernel a moment to mark the pidfd readable so `status()` + // reports Exited when the pruner probes. + for _ in 0..20 { + if pinned + .as_ref() + .is_some_and(|process| process.status() == ProcessStatus::Exited) + { + break; + } + thread::sleep(Duration::from_millis(5)); + } + registry.record(None, pinned); + } + + let retained = registry.spawned.lock().len(); + assert!( + retained < SpawnRegistry::PRUNE_THRESHOLD, + "pruning must bound retained entries below the sweep threshold once the pinned \ + processes have exited; got {retained} retained (threshold {})", + SpawnRegistry::PRUNE_THRESHOLD + ); + + // build_targets sees no live handles → empty target set, matching the + // contract that fully-exited registries stop the wave loop early. + let targets = registry.build_targets(); + assert!( + targets.is_empty(), + "registry of only-dead entries must produce an empty target set" + ); + } } diff --git a/packages/natives/CHANGELOG.md b/packages/natives/CHANGELOG.md index 7f029b3a6..fdb2fc658 100644 --- a/packages/natives/CHANGELOG.md +++ b/packages/natives/CHANGELOG.md @@ -4,7 +4,7 @@ ### Fixed -- Fixed a Windows regression where an abnormal `omp` exit or bash cancellation could `TerminateProcess` unrelated `pwsh.exe` / `powershell.exe` sessions (including other Cursor terminal tabs). `SpawnRegistry` stored only the raw pid of each brush-spawned child and re-opened it via `Process::from_pid` at cancellation time; between those two moments Windows could recycle a freed pid onto an unrelated PowerShell, and `signal_tree` then walked the wrong subtree via Toolhelp. The observer now pins a stable `Process` handle at spawn time — on Windows the open handle keeps the pid slot reserved, on Linux the pidfd carries identity, on macOS the `(pid, start_time)` triple detects impersonation — so cancellation can only reach children this run actually launched. ([#4605](https://github.com/can1357/oh-my-pi/issues/4605)) +- Fixed a Windows regression where an abnormal `omp` exit or bash cancellation could `TerminateProcess` unrelated `pwsh.exe` / `powershell.exe` sessions (including other Cursor terminal tabs). `SpawnRegistry` stored only the raw pid of each brush-spawned child and re-opened it via `Process::from_pid` at cancellation time; between those two moments Windows could recycle a freed pid onto an unrelated PowerShell, and `signal_tree` then walked the wrong subtree via Toolhelp. The observer now pins a stable `Process` handle at spawn time — on Windows the open handle keeps the pid slot reserved, on Linux the pidfd carries identity, on macOS the `(pid, start_time)` triple detects impersonation — so cancellation can only reach children this run actually launched. The registry sweeps exited entries once the recorded set crosses a small threshold so a long bash loop of short external commands cannot pin one owned OS handle per historical spawn. ([#4605](https://github.com/can1357/oh-my-pi/issues/4605)) ## [16.3.6] - 2026-07-04 From 1877c7ed2748f228b9d242be2b6ec6e3fd97dd9c Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 5 Jul 2026 10:21:26 +0000 Subject: [PATCH 3/4] fix(pi-shell): kept Windows spawn entries until descendants exit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the second review on #4606: `prune_exited` retained an entry while its process was running OR its process group was alive. On Windows `process_group_alive` is always `false` (no process groups), so the predicate collapsed to "root running" — when a brush-spawned root exited after starting a child that stayed alive (a `pwsh`/shell command that launches a helper and exits, an MCP stdio wrapper handing off to a long-running server), pruning immediately dropped the pinned handle. Dropping that handle closes the last thing keeping the root pid slot reserved. Windows can then recycle the pid onto an unrelated process, reintroducing the exact race #4605 closes. It also strands the leftover child: the next cancellation wave has no root to walk descendants from, so `signal_tree` never reaches it. The retain predicate is now: - root process still running → keep (all platforms); - Windows-only: root exited but the pinned handle still probes at least one live descendant via Toolhelp → keep, because the handle is what guarantees the walk targets the original subtree (recycled pids would not be reachable while the handle holds the slot); - pgid still alive → keep (Unix only; Windows falls through). Unix stays unchanged because a child reparented onto init keeps its pgid, so `process_group_alive` already catches "root gone, descendants alive" without a per-entry tree walk. Fixes #4605 --- crates/pi-shell/src/process.rs | 40 ++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/crates/pi-shell/src/process.rs b/crates/pi-shell/src/process.rs index d9aea6b07..ae9ac7e42 100644 --- a/crates/pi-shell/src/process.rs +++ b/crates/pi-shell/src/process.rs @@ -1722,21 +1722,37 @@ impl SpawnRegistry { } } -/// Drop registry entries whose pinned process *and* process group are both -/// gone: with neither still-live, they contribute nothing to the next -/// termination wave and only pin an owned OS handle for no reason. A pgid-only -/// entry (observer failed to pin the process but the group is still alive) is -/// retained so `build_targets` can still signal the group. +/// Drop registry entries whose pinned process, process group, and — on +/// Windows — descendant tree are all gone. With nothing still-live the entry +/// contributes nothing to the next termination wave and only pins an owned OS +/// handle for no reason. +/// +/// The platform split matters because Windows has no process groups. On Unix +/// a child reparented onto init keeps its pgid, so a live pgid still catches +/// grandchildren whose immediate parent exited. On Windows there is no +/// reparenting and no pgid, so we probe the descendant tree directly through +/// the still-open pinned handle — dropping that handle would release the pid +/// slot, letting a recycled pid make future Toolhelp walks unsafe (issue +/// #4605) and orphaning any leftover child from the next cancellation wave. fn prune_exited(spawned: &mut Vec) { spawned.retain(|entry| { - let process_live = entry - .process - .as_ref() - .is_some_and(|process| process.status() == ProcessStatus::Running); - let group_live = entry + if let Some(process) = &entry.process { + if process.status() == ProcessStatus::Running { + return true; + } + // Windows-only: root exited but the pinned handle still keeps its + // pid reserved, so `live_descendants` walks the *original* subtree + // via Toolhelp. If any child is still running we must keep the + // entry — closing the handle would both release the pid (racing + // pid reuse) and strand the surviving child. + #[cfg(target_os = "windows")] + if !process.live_descendants().is_empty() { + return true; + } + } + entry .pgid - .is_some_and(|pgid| pgid > 0 && process_group_alive(pgid)); - process_live || group_live + .is_some_and(|pgid| pgid > 0 && process_group_alive(pgid)) }); } From 04258b98d7a67fb7976f48b4511967cc0ea92f43 Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 5 Jul 2026 10:27:39 +0000 Subject: [PATCH 4/4] fix(pi-shell): bounded SpawnRegistry sweeps by watermark not raw length MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the third review on #4606: `record` guarded pruning with `spawned.len() >= PRUNE_THRESHOLD`. Once the recorded vec stabilized above the threshold with entries the sweep could not remove — a run whose live children exceed the threshold, e.g. `for i in {1..1000}; do sleep 60 & done` — every subsequent `record` re-entered `prune_exited` while holding the registry lock. Each sweep is O(N) (a status probe per entry, plus a Toolhelp descendant walk on Windows for exited roots), so the per-spawn cost climbed to O(n²) even though the doc-comment promised amortized O(1). The registry now tracks a `next_sweep_at` watermark alongside the recorded vec (both under one mutex — the two fields are always mutated together). A sweep fires only when `spawned.len()` crosses that watermark; every sweep rescheds the next fire `PRUNE_THRESHOLD` further records away from the current post-sweep size. Sweep frequency is now capped at one per `PRUNE_THRESHOLD` records regardless of live set size, restoring true amortized O(1) per spawn. `build_targets` resets the watermark after its own sweep so the record-time schedule stays consistent with the post-cancel live set. Added `spawn_registry_watermark_bounds_sweep_frequency`: fills the vec past threshold with permanently-live entries, records another 20, and asserts the vec grew by exactly 20 (no sweep modified it) and the watermark did not advance. Existing tests updated for the collapsed `state` mutex. Fixes #4605 --- crates/pi-shell/src/process.rs | 119 +++++++++++++++++++++++++++------ 1 file changed, 100 insertions(+), 19 deletions(-) diff --git a/crates/pi-shell/src/process.rs b/crates/pi-shell/src/process.rs index ae9ac7e42..67a78f187 100644 --- a/crates/pi-shell/src/process.rs +++ b/crates/pi-shell/src/process.rs @@ -1638,9 +1638,24 @@ struct SpawnedProcess { /// host process: a run that cancelled would signal *any* descendant spawned /// after its baseline, including another run's children. Ownership is now /// explicit — only processes this run actually spawned are ever signalled. +#[derive(Default)] +struct RegistryState { + spawned: Vec, + /// The next `spawned.len()` at which `record` runs a sweep. Bounds sweep + /// frequency when the live set stabilizes above the initial threshold: + /// without this watermark, every subsequent `record` would find + /// `len >= PRUNE_THRESHOLD` true and sweep on every spawn (O(n²) in a + /// large-fan-out run like `for i in {1..1000}; do sleep 60 & done`). With + /// it, the next sweep only fires once the vec has grown by another + /// `PRUNE_THRESHOLD` entries since the previous sweep — restoring true + /// amortized O(1) per spawn regardless of how many entries survive each + /// sweep. + next_sweep_at: usize, +} + #[derive(Default)] pub struct SpawnRegistry { - spawned: Mutex>, + state: Mutex, } impl SpawnRegistry { @@ -1649,12 +1664,15 @@ impl SpawnRegistry { /// A shell run that spawns many short-lived external commands (e.g. a bash /// loop invoking a binary per iteration) would otherwise retain one owned /// process handle per spawn — a pidfd on Linux, a `HANDLE` on Windows — for - /// the lifetime of the run, exhausting per-process FD/handle limits. When - /// the recorded vec grows past this many entries, `record` sweeps out - /// entries whose pinned process AND process group are both gone. Cost per - /// sweep is `O(N)` (one non-blocking status probe per entry) and a sweep - /// runs at most once per this many `record` calls, so the amortized cost - /// per spawn is `O(1)` — cheap next to the spawn itself. + /// the lifetime of the run, exhausting per-process FD/handle limits. + /// + /// Each sweep costs `O(N)` (one non-blocking status probe per entry, plus + /// a Toolhelp descendant walk on Windows for exited roots). The next sweep + /// is scheduled `PRUNE_THRESHOLD` further records away — via the + /// `next_sweep_at` watermark — so a run that keeps many concurrent + /// long-lived children (`for i in {1..1000}; do sleep 60 & done`) does not + /// sweep on every spawn just because the vec is already above threshold. + /// Amortized cost per spawn stays `O(1)` regardless of the live-set size. const PRUNE_THRESHOLD: usize = 64; /// Create an empty registry. @@ -1671,15 +1689,22 @@ impl SpawnRegistry { /// before we could `Process::from_pid`) the entry becomes a no-op at /// termination time — there is nothing left to signal. /// - /// Once the recorded vec reaches [`Self::PRUNE_THRESHOLD`], exited entries - /// are swept so long-running loops of short external commands cannot - /// exhaust the process' FD/handle limit by retaining one owned handle per - /// historical spawn. + /// Exited entries are swept opportunistically once the recorded vec + /// crosses the next-sweep watermark, so long-running loops of short + /// external commands cannot exhaust the process' FD/handle limit by + /// retaining one owned handle per historical spawn. pub fn record(&self, pgid: Option, process: Option) { - let mut spawned = self.spawned.lock(); - spawned.push(SpawnedProcess { process, pgid }); - if spawned.len() >= Self::PRUNE_THRESHOLD { - prune_exited(&mut spawned); + let mut state = self.state.lock(); + state.spawned.push(SpawnedProcess { process, pgid }); + if state.spawned.len() >= state.next_sweep_at.max(Self::PRUNE_THRESHOLD) { + prune_exited(&mut state.spawned); + // Schedule the next sweep `PRUNE_THRESHOLD` further records away. + // Comparing against the post-sweep live-set size (not the pre-sweep + // length) bounds the sweep frequency when many entries survive: + // each sweep costs O(N) but now runs at most once per + // `PRUNE_THRESHOLD` records, so amortized per-record cost is O(1) + // even if the live set stays large. + state.next_sweep_at = state.spawned.len() + Self::PRUNE_THRESHOLD; } } @@ -1697,9 +1722,13 @@ impl SpawnRegistry { pub fn build_targets(&self) -> TerminationTargets { let mut targets = TerminationTargets::new(); let spawned = { - let mut guard = self.spawned.lock(); - prune_exited(&mut guard); - guard.clone() + let mut state = self.state.lock(); + prune_exited(&mut state.spawned); + // Reset the watermark to the current live-set size + threshold; + // leaving a stale pre-sweep value would misgate the next + // record-time sweep. + state.next_sweep_at = state.spawned.len() + Self::PRUNE_THRESHOLD; + state.spawned.clone() }; for entry in spawned { if let Some(process) = entry.process { @@ -1987,7 +2016,7 @@ mod tests { registry.record(None, pinned); } - let retained = registry.spawned.lock().len(); + let retained = registry.state.lock().spawned.len(); assert!( retained < SpawnRegistry::PRUNE_THRESHOLD, "pruning must bound retained entries below the sweep threshold once the pinned \ @@ -2003,4 +2032,56 @@ mod tests { "registry of only-dead entries must produce an empty target set" ); } + + /// Regression test for the third review on PR #4606: once the recorded + /// vec crosses `PRUNE_THRESHOLD`, subsequent `record` calls must NOT + /// sweep on every spawn. Without the `next_sweep_at` watermark, a large + /// fan-out run whose live children exceed the threshold turned every + /// spawn into an O(N) status probe of the whole retained set. + /// + /// The check reasons about the observable side effect: after N records + /// past threshold with entries that CANNOT be pruned (all still live), + /// the retained size grows monotonically by exactly N — no sweep runs + /// have modified the vec in between. The direct signal of "did a sweep + /// happen" is a stable pinned handle count across records. + #[cfg(unix)] + #[test] + fn spawn_registry_watermark_bounds_sweep_frequency() { + let self_pid = i32::try_from(std::process::id()).expect("self pid fits in i32"); + let registry = SpawnRegistry::new(); + + // Fill past threshold with entries that are permanently alive + // (pinning ourselves) so the pruner has nothing to remove. + let fill = SpawnRegistry::PRUNE_THRESHOLD + 10; + for _ in 0..fill { + registry.record(None, Process::from_pid(self_pid)); + } + let after_fill = registry.state.lock().spawned.len(); + assert_eq!( + after_fill, fill, + "live-only entries must not be pruned during warm-up" + ); + let watermark_after_fill = registry.state.lock().next_sweep_at; + + // Every additional record with a live entry must land in the vec + // verbatim and — critically — NOT re-enter `prune_exited` until the + // vec crosses the freshly scheduled watermark. If the guard were + // still `len >= PRUNE_THRESHOLD` (pre-fix), a sweep would fire on + // every one of these records. + let extra = 20; + for _ in 0..extra { + registry.record(None, Process::from_pid(self_pid)); + } + let after_extra = registry.state.lock().spawned.len(); + assert_eq!( + after_extra, + after_fill + extra, + "records with live entries must accumulate without triggering per-spawn sweeps" + ); + assert_eq!( + registry.state.lock().next_sweep_at, + watermark_after_fill, + "watermark must not advance while the vec stays below it — otherwise a sweep ran" + ); + } }