merge PR #4606: fix(pi-shell): pin spawn observer identity against Windows pid reuse
This commit is contained in:
can1357
+344
-12
@@ -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<i32>,
|
||||
process: Option<Process>,
|
||||
pgid: Option<i32>,
|
||||
}
|
||||
|
||||
/// 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<SpawnedProcess>,
|
||||
/// 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<Vec<SpawnedProcess>>,
|
||||
state: Mutex<RegistryState>,
|
||||
}
|
||||
|
||||
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<i32>) {
|
||||
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<i32>, process: Option<Process>) {
|
||||
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<SpawnedProcess>) {
|
||||
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<i32> = 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"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1372,7 +1372,14 @@ async fn read_output_bytes(
|
||||
|
||||
impl SpawnObserver for process::SpawnRegistry {
|
||||
fn on_spawn(&self, pid: i32, pgid: Option<i32>) {
|
||||
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);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user