From e5f954b0fe3e838549d04b8313b243991e4cd6ff Mon Sep 17 00:00:00 2001 From: roboomp Date: Mon, 3 Aug 2026 03:42:50 +0000 Subject: [PATCH] fix(pi-shell): prune protected subtrees from cancellation sweeps The flattened descendant list can contain a protected node (the harness, on a Windows PID-reuse false-descendant) together with that node's real children, collected by recursing through it. Skipping only the exact protected pid kept omp alive but still TerminateProcess'd its unrelated worker/tool subprocesses. signal_tree/terminate_tree now drop every node whose recorded parent chain within the enumerated set passes through a protected pid, so a false descendant of the harness can no longer drag the harness's real children into the kill set. Fixes #7452 --- crates/pi-shell/src/process.rs | 95 ++++++++++++++++++++++++++---- packages/coding-agent/CHANGELOG.md | 2 +- 2 files changed, 83 insertions(+), 14 deletions(-) diff --git a/crates/pi-shell/src/process.rs b/crates/pi-shell/src/process.rs index 0b178fdb2..0e1d091aa 100644 --- a/crates/pi-shell/src/process.rs +++ b/crates/pi-shell/src/process.rs @@ -1,6 +1,9 @@ //! Cross-platform process tree management. -use std::{collections::HashSet, time::Duration}; +use std::{ + collections::{HashMap, HashSet}, + time::Duration, +}; use anyhow::Result; use parking_lot::Mutex; @@ -1405,7 +1408,7 @@ impl Process { /// as a false descendant; `TerminateProcess`-ing it drops the whole session /// with no cleanup and no `session_exit` record (#7452, related #4605). fn signal_tree_excluding(&self, signal: i32, protected: &HashSet) -> u32 { - let descendants = self.live_descendants(); + let descendants = self.signalable_descendants(protected); let mut signaled = 0u32; // If self leads its own process group, also signal the group — this catches // grandchildren reparented to init when their immediate parent died inside @@ -1416,9 +1419,6 @@ impl Process { let _ = kill_process_group(pgid, signal); } for child in &descendants { - if protected.contains(&child.pid()) { - continue; - } if child.inner.kill(signal) { signaled += 1; } @@ -1429,6 +1429,27 @@ impl Process { signaled } + /// Live descendants with every protected subtree pruned, not just the exact + /// protected pids. + /// + /// The flattened descendant list can contain a protected node (the harness, + /// on a Windows PID-reuse false-descendant) *together with* that node's real + /// children, which were collected by recursing through it. Skipping only the + /// exact protected pid would still terminate those unrelated worker/tool + /// subprocesses, so drop every node whose recorded parent chain — within the + /// enumerated set — passes through a protected pid (#7452 review). + fn signalable_descendants(&self, protected: &HashSet) -> Vec { + let descendants = self.live_descendants(); + let parents: HashMap = descendants + .iter() + .filter_map(|descendant| descendant.ppid().map(|parent| (descendant.pid(), parent))) + .collect(); + descendants + .into_iter() + .filter(|descendant| !pid_in_protected_subtree(descendant.pid(), protected, &parents)) + .collect() + } + async fn terminate_tree_impl( &self, group: bool, @@ -1447,11 +1468,8 @@ impl Process { if let Some(pgid) = process_group { let _ = kill_process_group(pgid, TERM_SIGNAL); } - let mut descendants = self.live_descendants(); + let mut descendants = self.signalable_descendants(&protected); for child in &descendants { - if protected.contains(&child.pid()) { - continue; - } let _ = child.inner.kill(TERM_SIGNAL); } if !protected.contains(&self.pid()) { @@ -1478,11 +1496,8 @@ impl Process { if let Some(pgid) = process_group { let _ = kill_process_group(pgid, KILL_SIGNAL); } - descendants = self.live_descendants(); + descendants = self.signalable_descendants(&protected); for child in &descendants { - if protected.contains(&child.pid()) { - continue; - } let _ = child.inner.kill(KILL_SIGNAL); } if !protected.contains(&self.pid()) { @@ -1529,6 +1544,30 @@ fn host_protected_pids() -> HashSet { protected } +/// True when `pid` is itself protected or descends — within the enumerated +/// `parents` map (pid -> recorded parent pid) — from a protected pid. Used to +/// prune a whole protected subtree from a cancellation sweep so a false +/// descendant of the harness cannot drag the harness's real children into the +/// kill set (#7452). +fn pid_in_protected_subtree( + pid: i32, + protected: &HashSet, + parents: &HashMap, +) -> bool { + let mut current = pid; + // Bound the walk against a corrupted or cyclic parent chain. + for _ in 0..256 { + if protected.contains(¤t) { + return true; + } + match parents.get(¤t) { + Some(&parent) if parent != current => current = parent, + _ => return false, + } + } + false +} + async fn wait_for_exit( root: &Process, descendants: &[Process], @@ -1931,6 +1970,36 @@ mod tests { let _ = child.wait(); } + /// Regression test for the #7453 review: pruning a protected node must drop + /// its whole subtree, not just the exact protected pid. A Windows PID-reuse + /// false-descendant collects the harness together with the harness's real + /// children (LSP servers, worker/tool subprocesses); skipping only the host + /// pid would still terminate those. `pid_in_protected_subtree` walks the + /// enumerated parent map so any node under a protected pid is excluded. + #[test] + fn protected_subtree_is_pruned_not_just_the_pid() { + // root(1) -> host(2, protected) -> worker(3); root(1) -> real_child(4). + let parents: HashMap = [(2, 1), (3, 2), (4, 1)].into_iter().collect(); + let protected: HashSet = [2].into_iter().collect(); + + assert!( + pid_in_protected_subtree(2, &protected, &parents), + "the protected node itself must be excluded", + ); + assert!( + pid_in_protected_subtree(3, &protected, &parents), + "a child collected through the protected node must be excluded too", + ); + assert!( + !pid_in_protected_subtree(4, &protected, &parents), + "a real child of the sweep root must still be signalled", + ); + assert!( + !pid_in_protected_subtree(1, &protected, &parents), + "the sweep root must not be pruned", + ); + } + /// `kill_process_group` is the last line of defense: even if a future /// caller manages to feed the harness's own pgid into the signal path, /// this wrapper must refuse to deliver the signal. diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index e0ac147ef..32f486f43 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -25,7 +25,7 @@ ### Fixed -- Fixed the Windows bash tool silently taking down the whole omp process when a command blocked until its timeout: cancelling a timed-out run walked the spawned child's descendant tree from raw `th32ParentProcessID` links, and a recycled pid matching the harness's stale recorded parent pid could enumerate omp (or an ancestor) as a false descendant and `TerminateProcess` it, killing the session with no `session_exit` record. Run-cancellation sweeps now refuse to signal the harness pid or its ancestor chain ([#7452](https://github.com/can1357/oh-my-pi/issues/7452)). +- Fixed the Windows bash tool silently taking down the whole omp process when a command blocked until its timeout: cancelling a timed-out run walked the spawned child's descendant tree from raw `th32ParentProcessID` links, and a recycled pid matching the harness's stale recorded parent pid could enumerate omp (or an ancestor) as a false descendant and `TerminateProcess` it, killing the session with no `session_exit` record. Run-cancellation sweeps now refuse to signal the harness, its ancestor chain, or any process collected beneath them ([#7452](https://github.com/can1357/oh-my-pi/issues/7452)). - Fixed session transcript entries being lost on process crash: file-backed JSONL writers now write each completed entry to the OS page cache on append instead of microtask-batching; concurrent appends during an in-place atomic rewrite supersede the paused publish with a synchronous full-body rewrite so fenced events are durable before return; concurrent appends during `/move` write a full body to the live relocation path (source pre-rename, destination post-rename) so a crash mid-move no longer drops completed events; and the first write failure latches `#diskFailure` synchronously via `appendSync` so a later flush/close reports it instead of silently succeeding after a discarded rejected Promise ([#7444](https://github.com/can1357/oh-my-pi/pull/7444) by [@olegpulatov](https://github.com/olegpulatov)). - Fixed `/mcp reauth` sending literal `${VAR}` placeholders instead of env-expanded OAuth client credentials during the token exchange, and `MCPOAuthFlow.exchangeToken()` accepting an HTTP-200 token response with no `access_token` (e.g. a Slack `{ ok: false, error }` body), which stored an empty access token and only surfaced `invalid_token` on a later MCP request ([#7440](https://github.com/can1357/oh-my-pi/issues/7440)). - Fixed fuzzy replace-all edits re-matching replacement text indefinitely, which could freeze the TUI and exhaust memory ([#7432](https://github.com/can1357/oh-my-pi/issues/7432)).