diff --git a/crates/pi-shell/src/process.rs b/crates/pi-shell/src/process.rs index ede0fd3f8..67a78f187 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, @@ -1613,12 +1638,43 @@ 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 { + /// 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. + /// + /// 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. #[must_use] pub fn new() -> Self { @@ -1626,24 +1682,64 @@ 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. + /// + /// 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 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; + } } /// 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. + /// + /// 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 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 { - 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) @@ -1655,6 +1751,40 @@ impl SpawnRegistry { } } +/// 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| { + 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)) + }); +} + /// 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 @@ -1752,4 +1882,206 @@ 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}; + + // 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 long_pid = i32::try_from(long.id()).expect("child pid fits in i32"); + + let registry = SpawnRegistry::new(); + let pinned = Process::from_pid(long_pid).expect("pin child at record time"); + registry.record(None, Some(pinned)); + + 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)" + ); + + 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 registry.build_targets().is_empty() { + empty_after_exit = true; + break; + } + thread::sleep(Duration::from_millis(25)); + } + assert!( + 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" + ); + } + + /// `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"); + } + + /// 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.state.lock().spawned.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" + ); + } + + /// 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" + ); + } } 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 c8c4ffef6..cec936333 100644 --- a/packages/natives/CHANGELOG.md +++ b/packages/natives/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixed - Fixed the native build script failing to locate the `@napi-rs/cli` `napi` binary on Windows because the `PATH` lookup joined entries with a Unix `:` separator instead of the platform delimiter (`path.delimiter`). +- 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