smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(pi-shell): kept recycled targets killable

Browse Source 
Protect only the harness pid during cancellation sweeps. The host recorded parent pid can be stale on Windows and recycled onto the hung command; adding that raw pid to the protected set spared the cancellation target and pruned its whole subtree from cleanup.

Keep protected-subtree pruning rooted at the harness itself, which still spares its real workers while allowing the timed-out target to be reaped.

Fixes #7452
This commit is contained in:
roboomp
2026-08-03 03:47:32 +00:00
parent e5f954b0fe
commit dc22ea5dad
2 changed files with 24 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/pi-shell/src/process.rs
+23 -37
View File
@@ -1400,8 +1400,8 @@ impl Process {
/// Signal this process and its live descendants (children first), skipping
/// any pid in `protected`.
///
/// `protected` shields the harness and its ancestor chain: a
/// run-cancellation sweep must never hard-kill the host. On Windows the
/// `protected` shields the harness itself: a run-cancellation sweep must
/// never hard-kill the host. On Windows the
/// descendant tree is derived from raw `th32ParentProcessID` values that
/// outlive their recorded parent, so a freshly spawned child whose recycled
/// pid matches the harness's stale parent pid makes the harness enumerate
@@ -1509,39 +1509,24 @@ impl Process {
}
}
/// The harness pid plus its resolvable ancestor chain — the set of processes a
/// run-cancellation sweep must never signal.
/// The harness pid — the one process a run-cancellation sweep must never
/// signal.
///
/// On Unix the descendant walk is identity-pinned (pidfd / start-time), so the
/// host can never appear as a false descendant and this set is a harmless
/// no-op safety net. On Windows the descendant tree is derived from raw
/// `th32ParentProcessID` values that survive their recorded parent's death: a
/// freshly spawned child whose recycled pid matches the harness's stale parent
/// pid makes the harness (and any of its ancestors) enumerate as a false
/// descendant, so cancelling a timed-out bash run would `TerminateProcess` the
/// host with no cleanup and no `session_exit` record (#7452, related #4605).
/// pid makes the harness enumerate as a false descendant, so cancelling a
/// timed-out bash run would `TerminateProcess` the host with no cleanup and no
/// `session_exit` record (#7452, related #4605).
///
/// The host pid is inserted first, before any `Process::from_pid`, so the
/// harness stays protected even when an ancestor handle cannot be opened.
/// Do not walk the host's numeric parent chain here. On Windows the host's
/// recorded parent pid can itself have been recycled onto the cancellation
/// target; treating that raw pid as protected would spare the hung command and
/// prune all of its descendants from cleanup.
fn host_protected_pids() -> HashSet<i32> {
let mut protected = HashSet::new();
let Ok(mut pid) = i32::try_from(std::process::id()) else {
return protected;
};
// Bound the walk: a corrupted or cyclic parent chain must not loop forever.
for _ in 0..64 {
if !protected.insert(pid) {
break;
}
let Some(parent) = Process::from_pid(pid).and_then(|process| process.ppid()) else {
break;
};
if parent <= 0 || parent == pid {
break;
}
pid = parent;
}
protected
i32::try_from(std::process::id()).into_iter().collect()
}
/// True when `pid` is itself protected or descends — within the enumerated
@@ -1917,22 +1902,23 @@ const fn platform_process_group_alive(_pgid: i32) -> bool {
mod tests {
use super::*;
/// The harness pid must always be in the protected set, even before any
/// ancestor handle can be resolved: it is inserted before the first
/// `Process::from_pid`. Without this, a run-cancellation sweep that
/// enumerated the host as a false descendant would hard-kill the session.
/// The harness pid must be the only protected pid. Including its recorded
/// parent would be unsafe on Windows: that stale numeric pid can have been
/// recycled onto the timed-out command, causing cancellation to spare the
/// hung target and its whole subtree.
#[test]
fn host_protected_pids_includes_self() {
let self_pid = i32::try_from(std::process::id()).expect("self pid fits in i32");
assert!(
host_protected_pids().contains(&self_pid),
"the harness pid must always be protected from cancellation sweeps",
assert_eq!(
host_protected_pids(),
HashSet::from([self_pid]),
"only the harness pid may be protected from cancellation sweeps",
);
}
/// Regression test for #7452: a cancellation sweep must never signal a pid
/// in the protected (host + ancestors) set, even when it is enumerated as
/// the sweep root. On Windows a recycled pid can make the harness surface as
/// Regression test for #7452: a cancellation sweep must never signal the
/// protected host pid, even when it is enumerated as the sweep root. On
/// Windows a recycled pid can make the harness surface as
/// a false descendant of a just-spawned child; `TerminateProcess`-ing it
/// killed the whole session with no `session_exit` record. The observable
/// defense — provable cross-platform — is that `signal_tree_excluding`
packages/coding-agent/CHANGELOG.md
+1 -1
View File
@@ -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, its ancestor chain, or any process collected beneath them ([#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 as a false descendant and `TerminateProcess` it, killing the session with no `session_exit` record. Run-cancellation sweeps now refuse to signal the harness or any process collected beneath it, while still reaping the timed-out target when it owns a recycled ancestor pid ([#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)).