From 031dedebe3eb29808447637a5bdbead1139fcb1f Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Fri, 10 Jul 2026 11:05:54 -0400 Subject: [PATCH 1/3] fix(desktop): walk ancestor chain in orphan sweep exemption The one-level PPID/PGID exemption from #1359 assumed all harness descendants share the harness PGID, but buzz-acp spawns each child with process_group(0), so live grandchildren (node shim -> codex-acp) were flagged as orphans every periodic tick. The kill was then silently no-oped by the PID-recycling guard, leaving misleading 'cleaning up' log spam and a detection layer one structural change away from killing healthy agent sessions. Exempt any process whose parent chain reaches a tracked harness PID (bounded walk, 32 hops), and log when the recycling guard drops every candidate group so a suppressed kill is never silent again. The #1359 regression test modeled an intermediate that inherits the harness group; add the own-group intermediate variant that mirrors the real spawn path. --- .../src-tauri/src/managed_agents/runtime.rs | 91 +++++++------ .../src/managed_agents/runtime/sweep.rs | 124 ++++++++++++++++++ .../src/managed_agents/runtime/tests.rs | 92 +++++++++++++ 3 files changed, 261 insertions(+), 46 deletions(-) diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index 5a62a53e527..3714c95a79c 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -356,6 +356,12 @@ fn resolve_pgids_and_kill(candidate_pids: &[i32]) { let alive = unsafe { libc::kill(pgid, 0) } == 0; !alive }); + if pgids.is_empty() && !candidate_pids.is_empty() { + eprintln!( + "buzz-desktop: orphan sweep: all {} candidate group(s) skipped by PID-recycling guard (live foreign group leader); nothing signalled", + candidate_pids.len() + ); + } let unique: Vec = pgids.into_iter().collect(); sigterm_then_sigkill(&unique); } @@ -384,6 +390,12 @@ fn resolve_pgids_and_kill(candidate_pids: &[i32]) { let alive = unsafe { libc::kill(pgid, 0) } == 0; !alive }); + if pgids.is_empty() && !candidate_pids.is_empty() { + eprintln!( + "buzz-desktop: orphan sweep: all {} candidate group(s) skipped by PID-recycling guard (live foreign group leader); nothing signalled", + candidate_pids.len() + ); + } let unique: Vec = pgids.into_iter().collect(); sigterm_then_sigkill(&unique); } @@ -515,13 +527,14 @@ pub(crate) fn sweep_system_agent_processes(instance_id: &str, skip_pids: &[u32]) if info.pbi_uid != my_uid { continue; } - // Live child of a tracked harness — not an orphan. - if skip_pids.contains(&info.pbi_ppid) { + // Walk the full ancestor chain: each harness child may start its own + // process group, so a one-level PPID check misses grandchildren like + // codex-acp. A bounded walk (32 hops) catches all live descendants. + if sweep::walk_has_tracked_ancestor(upid, skip_pids, sweep::ppid_of_macos) { continue; } - // Grandchild check: the harness is spawned with process_group(0), so - // all descendants share its PGID. If this process's PGID matches a - // tracked harness PID, it's a live descendant — not an orphan. + // PGID fast-path: if this process's PGID is itself a tracked harness + // PID, the whole group is a live descendant — not an orphan. let pgid = unsafe { libc::getpgid(pid) }; if pgid > 0 && skip_pids.contains(&(pgid as u32)) { continue; @@ -541,20 +554,8 @@ pub(crate) fn sweep_system_agent_processes(instance_id: &str, skip_pids: &[u32]) } } -/// Read the parent PID of a process from /proc//stat. -/// The comm field (field 2) may contain spaces and parens, so we find the last -/// ')' and parse fields after it. Field 1 after ')' is state, field 2 is PPID. -#[cfg(all(unix, not(target_os = "macos")))] -fn read_ppid_linux(pid: u32) -> Option { - let stat = std::fs::read_to_string(format!("/proc/{pid}/stat")).ok()?; - let after_comm = stat.rsplit_once(')')?.1; - // Fields after ')': " S ppid pgid ..." - let ppid_str = after_comm.split_whitespace().nth(1)?; - ppid_str.parse::().ok() -} - /// Read the process group ID from /proc//stat. Same parsing strategy as -/// `read_ppid_linux` — field 3 after the closing ')' is the PGID. +/// `sweep::ppid_of_linux` — field 3 after the closing ')' is the PGID. #[cfg(all(unix, not(target_os = "macos")))] fn read_pgid_linux(pid: u32) -> Option { let stat = std::fs::read_to_string(format!("/proc/{pid}/stat")).ok()?; @@ -599,19 +600,17 @@ pub(crate) fn sweep_system_agent_processes(instance_id: &str, skip_pids: &[u32]) if !process_belongs_to_us(upid) || !process_has_buzz_marker(upid, instance_id) { continue; } - // Live child of a tracked harness — not an orphan. If /proc//stat - // is unreadable (process exiting, transient I/O error), we treat the - // process as orphaned — safe because an exiting process will disappear - // shortly, and the two-tick grace in the periodic path prevents acting - // on transient failures. - if let Some(ppid) = read_ppid_linux(upid) { - if skip_pids.contains(&ppid) { - continue; - } + // Walk the full ancestor chain: each harness child may start its own + // process group, so a one-level PPID check misses grandchildren like + // codex-acp. A bounded walk (32 hops) catches all live descendants. + // If /proc//stat is unreadable the walk returns false (not a + // known descendant) — an exiting process will disappear shortly, and + // the two-tick grace prevents acting on transient failures. + if sweep::walk_has_tracked_ancestor(upid, skip_pids, sweep::ppid_of_linux) { + continue; } - // Grandchild check: the harness is spawned with process_group(0), so - // all descendants share its PGID. If this process's PGID matches a - // tracked harness PID, it's a live descendant — not an orphan. + // PGID fast-path: if this process's PGID is itself a tracked harness + // PID, the whole group is a live descendant — not an orphan. if let Some(pgid) = read_pgid_linux(upid) { if skip_pids.contains(&pgid) { continue; @@ -714,13 +713,14 @@ pub(crate) fn collect_same_instance_orphans( if info.pbi_uid != my_uid { continue; } - // Live child of a tracked harness — not an orphan. - if skip_pids.contains(&info.pbi_ppid) { + // Walk the full ancestor chain: each harness child may start its own + // process group, so a one-level PPID check misses grandchildren like + // codex-acp. A bounded walk (32 hops) catches all live descendants. + if sweep::walk_has_tracked_ancestor(upid, skip_pids, sweep::ppid_of_macos) { continue; } - // Grandchild check: the harness is spawned with process_group(0), so - // all descendants share its PGID. If this process's PGID matches a - // tracked harness PID, it's a live descendant — not an orphan. + // PGID fast-path: if this process's PGID is itself a tracked harness + // PID, the whole group is a live descendant — not an orphan. let pgid = unsafe { libc::getpgid(pid) }; if pgid > 0 && skip_pids.contains(&(pgid as u32)) { continue; @@ -769,18 +769,17 @@ pub(crate) fn collect_same_instance_orphans( if !process_belongs_to_us(upid) || !process_has_buzz_marker(upid, instance_id) { continue; } - // Live child of a tracked harness — not an orphan. If /proc//stat - // is unreadable (process exiting, transient I/O error), we treat the - // process as orphaned — safe because an exiting process will disappear - // shortly, and the two-tick grace prevents acting on transient failures. - if let Some(ppid) = read_ppid_linux(upid) { - if skip_pids.contains(&ppid) { - continue; - } + // Walk the full ancestor chain: each harness child may start its own + // process group, so a one-level PPID check misses grandchildren like + // codex-acp. A bounded walk (32 hops) catches all live descendants. + // If /proc//stat is unreadable the walk returns false (not a + // known descendant) — an exiting process will disappear shortly, and + // the two-tick grace prevents acting on transient failures. + if sweep::walk_has_tracked_ancestor(upid, skip_pids, sweep::ppid_of_linux) { + continue; } - // Grandchild check: the harness is spawned with process_group(0), so - // all descendants share its PGID. If this process's PGID matches a - // tracked harness PID, it's a live descendant — not an orphan. + // PGID fast-path: if this process's PGID is itself a tracked harness + // PID, the whole group is a live descendant — not an orphan. if let Some(pgid) = read_pgid_linux(upid) { if skip_pids.contains(&pgid) { continue; diff --git a/desktop/src-tauri/src/managed_agents/runtime/sweep.rs b/desktop/src-tauri/src/managed_agents/runtime/sweep.rs index 2e026ec5fc0..e53a8b3f4e9 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/sweep.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/sweep.rs @@ -94,6 +94,70 @@ pub(super) fn procargs2_buffer(pid: u32) -> Option> { Some(buf) } +// ── Ancestor walk ──────────────────────────────────────────────────────── + +/// True if walking `start`'s parent chain reaches any PID in `skip_pids`. +/// Bounded to 32 hops to guard against PPID cycles from PID reuse; a lookup +/// failure or reaching PID ≤ 1 ends the walk (process is not a descendant of +/// any tracked harness). +/// +/// The candidate itself being in `skip_pids` is handled at the call site — +/// this function checks strict ancestors only. +#[cfg(unix)] +pub(super) fn walk_has_tracked_ancestor( + start: u32, + skip_pids: &[u32], + parent_of: impl Fn(u32) -> Option, +) -> bool { + const MAX_DEPTH: usize = 32; + let mut cur = start; + for _ in 0..MAX_DEPTH { + let Some(parent) = parent_of(cur) else { + return false; + }; + if parent <= 1 || parent == cur { + return false; + } + if skip_pids.contains(&parent) { + return true; + } + cur = parent; + } + false +} + +/// Return the parent PID of a process on macOS via `proc_pidinfo`. +/// Returns `None` if the syscall fails (process may have exited). +#[cfg(target_os = "macos")] +pub(super) fn ppid_of_macos(pid: u32) -> Option { + let mut info = std::mem::MaybeUninit::::zeroed(); + let ret = unsafe { + super::proc_pidinfo( + pid as libc::c_int, + super::PROC_PIDTBSDINFO, + 0, + info.as_mut_ptr() as *mut libc::c_void, + std::mem::size_of::() as libc::c_int, + ) + }; + if ret <= 0 { + return None; + } + Some(unsafe { info.assume_init() }.pbi_ppid) +} + +/// Return the parent PID of a process from `/proc//stat`. +/// Same parsing strategy as `read_pgid_linux` — field 1 after the last `)` +/// is state, field 2 (index 1) is PPID. +#[cfg(all(unix, not(target_os = "macos")))] +pub(super) fn ppid_of_linux(pid: u32) -> Option { + let stat = std::fs::read_to_string(format!("/proc/{pid}/stat")).ok()?; + let after_comm = stat.rsplit_once(')')?.1; + // Fields after ')': " S ppid pgid ..." + let ppid_str = after_comm.split_whitespace().nth(1)?; + ppid_str.parse::().ok() +} + // ── ProcessSnapshot and pure decision function ──────────────────────────── /// A snapshot of one process for the pure kill-decision function. Holds only @@ -497,4 +561,64 @@ mod tests { let result = select_untracked_bundle_harnesses(&snaps, &PathBuf::from(BUNDLE_HARNESS), &[]); assert_eq!(result, vec![3001]); } + + // ── walk_has_tracked_ancestor ──────────────────────────────────────── + + #[cfg(unix)] + fn map_parent(tree: &std::collections::HashMap, pid: u32) -> Option { + tree.get(&pid).copied() + } + + #[cfg(unix)] + #[test] + fn walk_direct_child_of_tracked_harness_is_exempted() { + // PID 101's parent is 100 (tracked) → live descendant, not an orphan. + let tree: std::collections::HashMap = [(101, 100)].into_iter().collect(); + assert!(walk_has_tracked_ancestor(101, &[100], |p| map_parent( + &tree, p + ))); + } + + #[cfg(unix)] + #[test] + fn walk_grandchild_via_own_group_wrapper_is_exempted() { + // Production tree: harness(100) → node-wrapper(101, own group) → codex-acp(102). + // One-level PPID check misses 102; the walk catches it. + let tree: std::collections::HashMap = + [(101, 100), (102, 101)].into_iter().collect(); + assert!(walk_has_tracked_ancestor(102, &[100], |p| map_parent( + &tree, p + ))); + } + + #[cfg(unix)] + #[test] + fn walk_real_orphan_ending_at_pid1_returns_false() { + // Genuine orphan: chain ends at init (PID 1), no tracked ancestor. + let tree: std::collections::HashMap = [(201, 1)].into_iter().collect(); + assert!(!walk_has_tracked_ancestor(201, &[100], |p| map_parent( + &tree, p + ))); + } + + #[cfg(unix)] + #[test] + fn walk_ppid_cycle_terminates_and_returns_false() { + // PPID cycle (a → b → a) from PID reuse must terminate, not loop. + let tree: std::collections::HashMap = + [(300, 301), (301, 300)].into_iter().collect(); + assert!(!walk_has_tracked_ancestor(300, &[999], |p| map_parent( + &tree, p + ))); + } + + #[cfg(unix)] + #[test] + fn walk_missing_parent_entry_returns_false() { + // proc_pidinfo / /proc stat failure (process exited) → not a descendant. + let tree: std::collections::HashMap = [].into_iter().collect(); + assert!(!walk_has_tracked_ancestor(400, &[100], |p| map_parent( + &tree, p + ))); + } } diff --git a/desktop/src-tauri/src/managed_agents/runtime/tests.rs b/desktop/src-tauri/src/managed_agents/runtime/tests.rs index bfb7e1cbd29..cceda1d8903 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/tests.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/tests.rs @@ -670,3 +670,95 @@ fn grandchild_inherits_pgid_of_process_group_leader() { unsafe { libc::kill(-harness_pid, libc::SIGTERM) }; let _ = harness.wait(); } + +/// Validates that `walk_has_tracked_ancestor` catches the production case the +/// old PGID check missed: the intermediate process is in its OWN process group +/// (mirroring the node npm-shim wrapper that starts `codex-acp`). The +/// grandchild's PGID matches the intermediate's PID, not the harness's — so +/// `skip_pids.contains(&grandchild_pgid)` returns false. The ancestor walk +/// must still find the harness as an ancestor and return true. +#[cfg(unix)] +#[test] +fn own_group_grandchild_detected_by_ancestor_walk() { + use std::os::unix::process::CommandExt; + use std::process::Command; + + // The test process is the "harness". Spawn an intermediate with its own + // process group (mirrors the node shim). It backgrounds a grandchild + // (sleep 30) and prints the grandchild PID so we can inspect it. + let mut intermediate = { + let mut cmd = Command::new("sh"); + cmd.args(["-c", "sleep 30 & echo $!; wait"]) + .stdout(std::process::Stdio::piped()) + .process_group(0); + cmd.spawn().expect("spawn intermediate") + }; + + use std::io::BufRead; + let stdout = intermediate.stdout.take().unwrap(); + let reader = std::io::BufReader::new(stdout); + let grandchild_pid: u32 = reader + .lines() + .next() + .expect("should get a line") + .expect("should read line") + .trim() + .parse() + .expect("should parse grandchild PID"); + + let intermediate_pid = intermediate.id(); + let harness_pid = std::process::id(); + + // The intermediate is its own process group leader. + let intermediate_pgid = unsafe { libc::getpgid(intermediate_pid as i32) }; + assert_eq!( + intermediate_pgid, intermediate_pid as i32, + "intermediate should be its own process group leader" + ); + + // The grandchild inherits the intermediate's group — NOT the harness's. + let grandchild_pgid = unsafe { libc::getpgid(grandchild_pid as i32) }; + assert_eq!( + grandchild_pgid, intermediate_pid as i32, + "grandchild PGID should be the intermediate, not the harness" + ); + assert_ne!( + grandchild_pgid, harness_pid as i32, + "grandchild PGID must not equal harness PID — this is the false-positive shape" + ); + + // The ancestor walk finds the harness even though PGID doesn't match it. + let skip_pids = vec![harness_pid]; + #[cfg(target_os = "macos")] + let found = super::sweep::walk_has_tracked_ancestor( + grandchild_pid, + &skip_pids, + super::sweep::ppid_of_macos, + ); + #[cfg(all(unix, not(target_os = "macos")))] + let found = super::sweep::walk_has_tracked_ancestor( + grandchild_pid, + &skip_pids, + super::sweep::ppid_of_linux, + ); + assert!( + found, + "walk must detect grandchild as a live descendant of the tracked harness" + ); + + // Contrast: empty skip_pids → not a descendant of any tracked harness. + #[cfg(target_os = "macos")] + let not_found = + super::sweep::walk_has_tracked_ancestor(grandchild_pid, &[], super::sweep::ppid_of_macos); + #[cfg(all(unix, not(target_os = "macos")))] + let not_found = + super::sweep::walk_has_tracked_ancestor(grandchild_pid, &[], super::sweep::ppid_of_linux); + assert!( + !not_found, + "walk with empty skip_pids must return false for a real orphan" + ); + + // Cleanup: SIGKILL the intermediate's process group (takes sleep 30 with it). + unsafe { libc::kill(-(intermediate_pid as i32), libc::SIGKILL) }; + let _ = intermediate.wait(); +} From db7f2c7bc52695316508d17d3a9718b8216da7e5 Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Fri, 10 Jul 2026 11:06:03 -0400 Subject: [PATCH 2/3] chore(desktop): clear clippy backlog blocking desktop-tauri-clippy Six pre-existing errors (unnecessary_map_or x3, unused import, slice-clone, doc list indentation) fail 'just desktop-tauri-clippy' and the pre-push hook on main. CI's desktop-ci recipe does not run the clippy gate, which is how they slipped through. --- desktop/src-tauri/src/commands/agent_settings.rs | 2 +- desktop/src-tauri/src/commands/channels_tests.rs | 2 +- desktop/src-tauri/src/managed_agents/storage.rs | 2 +- desktop/src-tauri/src/migration_databricks_tests.rs | 6 +++--- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/desktop/src-tauri/src/commands/agent_settings.rs b/desktop/src-tauri/src/commands/agent_settings.rs index e4c52066a26..54b52bd1978 100644 --- a/desktop/src-tauri/src/commands/agent_settings.rs +++ b/desktop/src-tauri/src/commands/agent_settings.rs @@ -1,4 +1,4 @@ -use tauri::{AppHandle, Manager, State}; +use tauri::{AppHandle, Manager}; use crate::{ app_state::AppState, diff --git a/desktop/src-tauri/src/commands/channels_tests.rs b/desktop/src-tauri/src/commands/channels_tests.rs index b0143daef70..8eac36fca84 100644 --- a/desktop/src-tauri/src/commands/channels_tests.rs +++ b/desktop/src-tauri/src/commands/channels_tests.rs @@ -33,7 +33,7 @@ fn directory_cursor_keeps_same_second_tiebreaker() { let event = ev_at(39000, "{}", vec![], timestamp); let mut filter = serde_json::json!({"kinds": [39000], "limit": DIRECTORY_PAGE_SIZE}); - advance_directory_cursor(&mut filter, &[event.clone()]); + advance_directory_cursor(&mut filter, std::slice::from_ref(&event)); assert_eq!(filter["until"], serde_json::json!(timestamp.as_secs())); assert_eq!(filter["before_id"], serde_json::json!(event.id.to_hex())); diff --git a/desktop/src-tauri/src/managed_agents/storage.rs b/desktop/src-tauri/src/managed_agents/storage.rs index ef8fc665fee..abd5c9596b3 100644 --- a/desktop/src-tauri/src/managed_agents/storage.rs +++ b/desktop/src-tauri/src/managed_agents/storage.rs @@ -436,7 +436,7 @@ const DEV_MIGRATION_MARKER: &str = "_dev_migration_v1"; /// /// On subsequent boots (marker already present): /// 1. One `dst.load_all_readonly()` — dev blob read (1 keychain prompt) -/// Returns immediately — prod keyring is NEVER accessed. +/// Returns immediately — prod keyring is NEVER accessed. /// /// Idempotency: keys already present in `dst` are not overwritten (the agent /// may have rotated their key in the dev service after initial migration). diff --git a/desktop/src-tauri/src/migration_databricks_tests.rs b/desktop/src-tauri/src/migration_databricks_tests.rs index 2e50fae17de..842507ec831 100644 --- a/desktop/src-tauri/src/migration_databricks_tests.rs +++ b/desktop/src-tauri/src/migration_databricks_tests.rs @@ -32,7 +32,7 @@ fn reconcile_databricks_v1_to_v2_rewrites_v1_provider_on_block_build() { // Stale V1 model must be cleared so the baked DATABRICKS_MODEL is not // shadowed by BUZZ_AGENT_MODEL at spawn time (last-write-wins in Command::env). assert!( - records[0].get("model").map_or(true, |v| v.is_null()), + records[0].get("model").is_none_or(|v| v.is_null()), "stale V1 model field must be cleared when provider is rewritten to V2" ); } @@ -98,12 +98,12 @@ fn reconcile_databricks_v1_to_v2_clears_model_on_provider_rewrite() { // V1 records: provider migrated, model cleared. assert_eq!(records[0]["provider"], "databricks_v2"); assert!( - records[0].get("model").map_or(true, |v| v.is_null()), + records[0].get("model").is_none_or(|v| v.is_null()), "model must be cleared for V1→V2 migrated record A" ); assert_eq!(records[1]["provider"], "databricks_v2"); assert!( - records[1].get("model").map_or(true, |v| v.is_null()), + records[1].get("model").is_none_or(|v| v.is_null()), "model must be cleared for V1→V2 migrated record B" ); // V2 record: model untouched. From f587bc55de9fae675be98c5101f53501f2a32f4e Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Fri, 10 Jul 2026 11:30:09 -0400 Subject: [PATCH 3/3] refactor(desktop): consolidate sweep exemption into per-OS helpers Code review follow-up: the walk + PGID exemption block was duplicated verbatim at all four detection sites, and macOS checked the buzz marker after the walk while Linux checked it before. Extract per-OS is_live_descendant helpers with one canonical ordering (marker first), a single /proc//stat parse shared by PPID and PGID lookups, and a reparenting-guard comment explaining why the PGID check is not a redundant fast-path. Sharpen the guard-skip log to count deduped groups and name both skip causes. Add depth-cap boundary tests and a PID-reuse guard before the real-process test's group kill. --- .../src-tauri/src/managed_agents/runtime.rs | 84 ++++--------- .../src/managed_agents/runtime/sweep.rs | 113 ++++++++++++++++-- .../src/managed_agents/runtime/tests.rs | 30 ++--- 3 files changed, 139 insertions(+), 88 deletions(-) diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index 3714c95a79c..cb07d001cef 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -349,6 +349,7 @@ fn resolve_pgids_and_kill(candidate_pids: &[i32]) { // PID-recycling guard: if a resolved PGID is alive but isn't one of our // orphan candidates, the old harness PID was recycled by a new process // that called setsid() — skip it to avoid killing an unrelated group. + let candidate_groups = pgids.len(); pgids.retain(|&pgid| { if candidate_set.contains(&pgid) { return true; @@ -356,10 +357,9 @@ fn resolve_pgids_and_kill(candidate_pids: &[i32]) { let alive = unsafe { libc::kill(pgid, 0) } == 0; !alive }); - if pgids.is_empty() && !candidate_pids.is_empty() { + if pgids.is_empty() && candidate_groups > 0 { eprintln!( - "buzz-desktop: orphan sweep: all {} candidate group(s) skipped by PID-recycling guard (live foreign group leader); nothing signalled", - candidate_pids.len() + "buzz-desktop: orphan sweep: skipped all {candidate_groups} candidate group(s) (live foreign group leader or candidate already exited); nothing signalled" ); } let unique: Vec = pgids.into_iter().collect(); @@ -383,6 +383,7 @@ fn resolve_pgids_and_kill(candidate_pids: &[i32]) { // PID-recycling guard: if a resolved PGID is alive but isn't one of our // orphan candidates, the old harness PID was recycled by a new process // that called setsid() — skip it to avoid killing an unrelated group. + let candidate_groups = pgids.len(); pgids.retain(|&pgid| { if candidate_set.contains(&pgid) { return true; @@ -390,10 +391,9 @@ fn resolve_pgids_and_kill(candidate_pids: &[i32]) { let alive = unsafe { libc::kill(pgid, 0) } == 0; !alive }); - if pgids.is_empty() && !candidate_pids.is_empty() { + if pgids.is_empty() && candidate_groups > 0 { eprintln!( - "buzz-desktop: orphan sweep: all {} candidate group(s) skipped by PID-recycling guard (live foreign group leader); nothing signalled", - candidate_pids.len() + "buzz-desktop: orphan sweep: skipped all {candidate_groups} candidate group(s) (live foreign group leader or candidate already exited); nothing signalled" ); } let unique: Vec = pgids.into_iter().collect(); @@ -527,19 +527,11 @@ pub(crate) fn sweep_system_agent_processes(instance_id: &str, skip_pids: &[u32]) if info.pbi_uid != my_uid { continue; } - // Walk the full ancestor chain: each harness child may start its own - // process group, so a one-level PPID check misses grandchildren like - // codex-acp. A bounded walk (32 hops) catches all live descendants. - if sweep::walk_has_tracked_ancestor(upid, skip_pids, sweep::ppid_of_macos) { - continue; - } - // PGID fast-path: if this process's PGID is itself a tracked harness - // PID, the whole group is a live descendant — not an orphan. - let pgid = unsafe { libc::getpgid(pid) }; - if pgid > 0 && skip_pids.contains(&(pgid as u32)) { + if !process_has_buzz_marker(upid, instance_id) { continue; } - if !process_has_buzz_marker(upid, instance_id) { + // Live descendants of a tracked harness are exempt — see sweep::is_live_descendant_*. + if sweep::is_live_descendant_macos(upid, info.pbi_ppid, skip_pids) { continue; } orphans.push(pid); @@ -554,15 +546,12 @@ pub(crate) fn sweep_system_agent_processes(instance_id: &str, skip_pids: &[u32]) } } -/// Read the process group ID from /proc//stat. Same parsing strategy as -/// `sweep::ppid_of_linux` — field 3 after the closing ')' is the PGID. +/// Read the process group ID from /proc//stat by delegating to the shared +/// stat parser in `sweep`. Keeps a single parse site for the `/proc//stat` +/// field layout. #[cfg(all(unix, not(target_os = "macos")))] fn read_pgid_linux(pid: u32) -> Option { - let stat = std::fs::read_to_string(format!("/proc/{pid}/stat")).ok()?; - let after_comm = stat.rsplit_once(')')?.1; - // Fields after ')': " S ppid pgid ..." - let pgid_str = after_comm.split_whitespace().nth(2)?; - pgid_str.parse::().ok() + sweep::proc_stat_ppid_pgid_linux(pid).map(|(_, pgid)| pgid) } #[cfg(all(unix, not(target_os = "macos")))] @@ -600,22 +589,10 @@ pub(crate) fn sweep_system_agent_processes(instance_id: &str, skip_pids: &[u32]) if !process_belongs_to_us(upid) || !process_has_buzz_marker(upid, instance_id) { continue; } - // Walk the full ancestor chain: each harness child may start its own - // process group, so a one-level PPID check misses grandchildren like - // codex-acp. A bounded walk (32 hops) catches all live descendants. - // If /proc//stat is unreadable the walk returns false (not a - // known descendant) — an exiting process will disappear shortly, and - // the two-tick grace prevents acting on transient failures. - if sweep::walk_has_tracked_ancestor(upid, skip_pids, sweep::ppid_of_linux) { + // Live descendants of a tracked harness are exempt — see sweep::is_live_descendant_*. + if sweep::is_live_descendant_linux(upid, skip_pids) { continue; } - // PGID fast-path: if this process's PGID is itself a tracked harness - // PID, the whole group is a live descendant — not an orphan. - if let Some(pgid) = read_pgid_linux(upid) { - if skip_pids.contains(&pgid) { - continue; - } - } orphans.push(pid); } @@ -713,21 +690,14 @@ pub(crate) fn collect_same_instance_orphans( if info.pbi_uid != my_uid { continue; } - // Walk the full ancestor chain: each harness child may start its own - // process group, so a one-level PPID check misses grandchildren like - // codex-acp. A bounded walk (32 hops) catches all live descendants. - if sweep::walk_has_tracked_ancestor(upid, skip_pids, sweep::ppid_of_macos) { + if !process_has_buzz_marker(upid, instance_id) { continue; } - // PGID fast-path: if this process's PGID is itself a tracked harness - // PID, the whole group is a live descendant — not an orphan. - let pgid = unsafe { libc::getpgid(pid) }; - if pgid > 0 && skip_pids.contains(&(pgid as u32)) { + // Live descendants of a tracked harness are exempt — see sweep::is_live_descendant_*. + if sweep::is_live_descendant_macos(upid, info.pbi_ppid, skip_pids) { continue; } - if process_has_buzz_marker(upid, instance_id) { - orphans.insert(upid); - } + orphans.insert(upid); } orphans } @@ -769,22 +739,10 @@ pub(crate) fn collect_same_instance_orphans( if !process_belongs_to_us(upid) || !process_has_buzz_marker(upid, instance_id) { continue; } - // Walk the full ancestor chain: each harness child may start its own - // process group, so a one-level PPID check misses grandchildren like - // codex-acp. A bounded walk (32 hops) catches all live descendants. - // If /proc//stat is unreadable the walk returns false (not a - // known descendant) — an exiting process will disappear shortly, and - // the two-tick grace prevents acting on transient failures. - if sweep::walk_has_tracked_ancestor(upid, skip_pids, sweep::ppid_of_linux) { + // Live descendants of a tracked harness are exempt — see sweep::is_live_descendant_*. + if sweep::is_live_descendant_linux(upid, skip_pids) { continue; } - // PGID fast-path: if this process's PGID is itself a tracked harness - // PID, the whole group is a live descendant — not an orphan. - if let Some(pgid) = read_pgid_linux(upid) { - if skip_pids.contains(&pgid) { - continue; - } - } orphans.insert(upid); } orphans diff --git a/desktop/src-tauri/src/managed_agents/runtime/sweep.rs b/desktop/src-tauri/src/managed_agents/runtime/sweep.rs index e53a8b3f4e9..3060ff6593a 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/sweep.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/sweep.rs @@ -1,9 +1,12 @@ -//! Boot-time sweep for untracked same-bundle harness processes. +//! Boot-time sweep for untracked same-bundle harness processes, plus low-level +//! process-tree helpers shared with the periodic orphan sweeps in `runtime.rs`. //! //! The env-var and PID-file sweeps cannot see a harness whose receipt is gone //! or that predates `BUZZ_MANAGED_AGENT` injection. This sweep derives the //! expected `buzz-acp` path from the running executable and kills any process -//! whose exe matches exactly, minus the tracked set. +//! whose exe matches exactly, minus the tracked set. The PID enumeration, +//! procargs, parent/PGID lookups, and live-descendant classification helpers +//! collected here are also called directly by the periodic orphan sweeps. use std::path::{Path, PathBuf}; @@ -126,6 +129,22 @@ pub(super) fn walk_has_tracked_ancestor( false } +/// OS-resolved parent-PID lookup for `walk_has_tracked_ancestor`. +/// Test-only: lets tests call a single platform-agnostic name without +/// cfg gates; production code calls `ppid_of_macos`/`ppid_of_linux` directly. +#[cfg(all(test, target_os = "macos"))] +pub(super) fn ppid_of(pid: u32) -> Option { + ppid_of_macos(pid) +} + +/// OS-resolved parent-PID lookup for `walk_has_tracked_ancestor`. +/// Test-only: lets tests call a single platform-agnostic name without +/// cfg gates; production code calls `ppid_of_macos`/`ppid_of_linux` directly. +#[cfg(all(test, unix, not(target_os = "macos")))] +pub(super) fn ppid_of(pid: u32) -> Option { + ppid_of_linux(pid) +} + /// Return the parent PID of a process on macOS via `proc_pidinfo`. /// Returns `None` if the syscall fails (process may have exited). #[cfg(target_os = "macos")] @@ -146,16 +165,66 @@ pub(super) fn ppid_of_macos(pid: u32) -> Option { Some(unsafe { info.assume_init() }.pbi_ppid) } -/// Return the parent PID of a process from `/proc//stat`. -/// Same parsing strategy as `read_pgid_linux` — field 1 after the last `)` -/// is state, field 2 (index 1) is PPID. +/// Parse the PPID and PGID fields from `/proc//stat` in one read. +/// Fields after the last `)` (comm may contain spaces/parens): index 1 is +/// PPID, index 2 is PGID. #[cfg(all(unix, not(target_os = "macos")))] -pub(super) fn ppid_of_linux(pid: u32) -> Option { +pub(super) fn proc_stat_ppid_pgid_linux(pid: u32) -> Option<(u32, u32)> { let stat = std::fs::read_to_string(format!("/proc/{pid}/stat")).ok()?; let after_comm = stat.rsplit_once(')')?.1; // Fields after ')': " S ppid pgid ..." - let ppid_str = after_comm.split_whitespace().nth(1)?; - ppid_str.parse::().ok() + let mut fields = after_comm.split_whitespace(); + let _state = fields.next()?; // index 0: state + let ppid = fields.next()?.parse::().ok()?; // index 1: PPID + let pgid = fields.next()?.parse::().ok()?; // index 2: PGID + Some((ppid, pgid)) +} + +/// Return the parent PID of a process from `/proc//stat`. +#[cfg(all(unix, not(target_os = "macos")))] +pub(super) fn ppid_of_linux(pid: u32) -> Option { + proc_stat_ppid_pgid_linux(pid).map(|(ppid, _)| ppid) +} + +/// True if `pid` is a live descendant of any tracked harness in `skip_pids`. +/// +/// Three complementary checks: +/// 1. Direct parent — `ppid` was already fetched by the caller's BSDInfo +/// UID gate, so this hop is free. +/// 2. Reparenting guard — if an intermediate in the ancestor chain died, +/// the process reparents to init (PPID 1) and the ancestor walk can no +/// longer reach the harness; a process that started inside the +/// harness's process group still has PGID == harness PID, so the PGID +/// check spares it. This is NOT a redundant fast-path — it covers a +/// case the walk cannot. +/// 3. Bounded ancestor walk from `ppid` — covers deeper live chains where +/// intermediates run in their own process groups (e.g. buzz-acp -> +/// node shim -> codex-acp). +#[cfg(target_os = "macos")] +pub(super) fn is_live_descendant_macos(pid: u32, ppid: u32, skip_pids: &[u32]) -> bool { + if skip_pids.contains(&ppid) { + return true; + } + let pgid = unsafe { libc::getpgid(pid as i32) }; + if pgid > 0 && skip_pids.contains(&(pgid as u32)) { + return true; + } + walk_has_tracked_ancestor(ppid, skip_pids, ppid_of_macos) +} + +/// Linux variant: reads PPID and PGID from `/proc//stat` in a single +/// read, then applies the same three checks as the macOS variant. An +/// unreadable stat file (process exiting) yields `false` — the two-tick +/// grace in the periodic sweep absorbs transient failures. +#[cfg(all(unix, not(target_os = "macos")))] +pub(super) fn is_live_descendant_linux(pid: u32, skip_pids: &[u32]) -> bool { + let Some((ppid, pgid)) = proc_stat_ppid_pgid_linux(pid) else { + return false; + }; + if skip_pids.contains(&ppid) || skip_pids.contains(&pgid) { + return true; + } + walk_has_tracked_ancestor(ppid, skip_pids, ppid_of_linux) } // ── ProcessSnapshot and pure decision function ──────────────────────────── @@ -621,4 +690,32 @@ mod tests { &tree, p ))); } + + #[cfg(unix)] + #[test] + fn walk_finds_ancestor_at_exact_depth_cap() { + // Chain with exactly 32 edges: 1000 → 1001 → … → 1032. + // The ancestor at hop 32 is within MAX_DEPTH and must be found. + let mut tree = std::collections::HashMap::new(); + for i in 0..32u32 { + tree.insert(1000 + i, 1000 + i + 1); + } + assert!(walk_has_tracked_ancestor(1000, &[1032], |p| map_parent( + &tree, p + ))); + } + + #[cfg(unix)] + #[test] + fn walk_misses_ancestor_beyond_depth_cap() { + // Chain with 33 edges: 1000 → 1001 → … → 1033. + // Hop 33 exceeds MAX_DEPTH (32) — the ancestor must not be found. + let mut tree = std::collections::HashMap::new(); + for i in 0..33u32 { + tree.insert(1000 + i, 1000 + i + 1); + } + assert!(!walk_has_tracked_ancestor(1000, &[1033], |p| map_parent( + &tree, p + ))); + } } diff --git a/desktop/src-tauri/src/managed_agents/runtime/tests.rs b/desktop/src-tauri/src/managed_agents/runtime/tests.rs index cceda1d8903..ea054cbc103 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/tests.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/tests.rs @@ -729,35 +729,31 @@ fn own_group_grandchild_detected_by_ancestor_walk() { // The ancestor walk finds the harness even though PGID doesn't match it. let skip_pids = vec![harness_pid]; - #[cfg(target_os = "macos")] - let found = super::sweep::walk_has_tracked_ancestor( - grandchild_pid, - &skip_pids, - super::sweep::ppid_of_macos, - ); - #[cfg(all(unix, not(target_os = "macos")))] - let found = super::sweep::walk_has_tracked_ancestor( - grandchild_pid, - &skip_pids, - super::sweep::ppid_of_linux, - ); + let found = + super::sweep::walk_has_tracked_ancestor(grandchild_pid, &skip_pids, super::sweep::ppid_of); assert!( found, "walk must detect grandchild as a live descendant of the tracked harness" ); // Contrast: empty skip_pids → not a descendant of any tracked harness. - #[cfg(target_os = "macos")] let not_found = - super::sweep::walk_has_tracked_ancestor(grandchild_pid, &[], super::sweep::ppid_of_macos); - #[cfg(all(unix, not(target_os = "macos")))] - let not_found = - super::sweep::walk_has_tracked_ancestor(grandchild_pid, &[], super::sweep::ppid_of_linux); + super::sweep::walk_has_tracked_ancestor(grandchild_pid, &[], super::sweep::ppid_of); assert!( !not_found, "walk with empty skip_pids must return false for a real orphan" ); + // Guard against PID reuse: verify the intermediate is still alive before + // cleanup so a recycled PID can't corrupt the kill target. + assert!( + intermediate + .try_wait() + .expect("try_wait on intermediate") + .is_none(), + "intermediate exited before cleanup — its PID may have been recycled" + ); + // Cleanup: SIGKILL the intermediate's process group (takes sleep 30 with it). unsafe { libc::kill(-(intermediate_pid as i32), libc::SIGKILL) }; let _ = intermediate.wait();