fix(desktop): walk ancestor chain in orphan sweep exemption (#1711)

This commit is contained in:
Will Pfleger
2026-07-10 12:11:29 -04:00
committed by GitHub
parent 868c9ad055
commit 7fb215c4fc
7 changed files with 347 additions and 81 deletions
@@ -1,4 +1,4 @@
use tauri::{AppHandle, Manager, State};
use tauri::{AppHandle, Manager};
use crate::{
app_state::AppState,
@@ -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()));
+30 -73
View File
@@ -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,6 +357,11 @@ fn resolve_pgids_and_kill(candidate_pids: &[i32]) {
let alive = unsafe { libc::kill(pgid, 0) } == 0;
!alive
});
if pgids.is_empty() && candidate_groups > 0 {
eprintln!(
"buzz-desktop: orphan sweep: skipped all {candidate_groups} candidate group(s) (live foreign group leader or candidate already exited); nothing signalled"
);
}
let unique: Vec<i32> = pgids.into_iter().collect();
sigterm_then_sigkill(&unique);
}
@@ -377,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;
@@ -384,6 +391,11 @@ fn resolve_pgids_and_kill(candidate_pids: &[i32]) {
let alive = unsafe { libc::kill(pgid, 0) } == 0;
!alive
});
if pgids.is_empty() && candidate_groups > 0 {
eprintln!(
"buzz-desktop: orphan sweep: skipped all {candidate_groups} candidate group(s) (live foreign group leader or candidate already exited); nothing signalled"
);
}
let unique: Vec<i32> = pgids.into_iter().collect();
sigterm_then_sigkill(&unique);
}
@@ -515,20 +527,13 @@ 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) {
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.
let pgid = unsafe { libc::getpgid(pid) };
if pgid > 0 && skip_pids.contains(&(pgid as u32)) {
continue;
}
if !process_has_buzz_marker(upid, instance_id) {
continue;
}
// 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);
}
@@ -541,27 +546,12 @@ pub(crate) fn sweep_system_agent_processes(instance_id: &str, skip_pids: &[u32])
}
}
/// Read the parent PID of a process from /proc/<pid>/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<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::<u32>().ok()
}
/// Read the process group ID from /proc/<pid>/stat. Same parsing strategy as
/// `read_ppid_linux` — field 3 after the closing ')' is the PGID.
/// Read the process group ID from /proc/<pid>/stat by delegating to the shared
/// stat parser in `sweep`. Keeps a single parse site for the `/proc/<pid>/stat`
/// field layout.
#[cfg(all(unix, not(target_os = "macos")))]
fn read_pgid_linux(pid: u32) -> Option<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 pgid_str = after_comm.split_whitespace().nth(2)?;
pgid_str.parse::<u32>().ok()
sweep::proc_stat_ppid_pgid_linux(pid).map(|(_, pgid)| pgid)
}
#[cfg(all(unix, not(target_os = "macos")))]
@@ -599,23 +589,9 @@ 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/<pid>/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;
}
}
// 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.
if let Some(pgid) = read_pgid_linux(upid) {
if skip_pids.contains(&pgid) {
continue;
}
// Live descendants of a tracked harness are exempt — see sweep::is_live_descendant_*.
if sweep::is_live_descendant_linux(upid, skip_pids) {
continue;
}
orphans.push(pid);
}
@@ -714,20 +690,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) {
if !process_has_buzz_marker(upid, instance_id) {
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.
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,9 @@ 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/<pid>/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;
}
}
// 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.
if let Some(pgid) = read_pgid_linux(upid) {
if skip_pids.contains(&pgid) {
continue;
}
// Live descendants of a tracked harness are exempt — see sweep::is_live_descendant_*.
if sweep::is_live_descendant_linux(upid, skip_pids) {
continue;
}
orphans.insert(upid);
}
@@ -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};
@@ -94,6 +97,136 @@ pub(super) fn procargs2_buffer(pid: u32) -> Option<Vec<u8>> {
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<u32>,
) -> 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
}
/// 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<u32> {
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<u32> {
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")]
pub(super) fn ppid_of_macos(pid: u32) -> Option<u32> {
let mut info = std::mem::MaybeUninit::<super::BSDInfo>::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::<super::BSDInfo>() as libc::c_int,
)
};
if ret <= 0 {
return None;
}
Some(unsafe { info.assume_init() }.pbi_ppid)
}
/// Parse the PPID and PGID fields from `/proc/<pid>/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 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 mut fields = after_comm.split_whitespace();
let _state = fields.next()?; // index 0: state
let ppid = fields.next()?.parse::<u32>().ok()?; // index 1: PPID
let pgid = fields.next()?.parse::<u32>().ok()?; // index 2: PGID
Some((ppid, pgid))
}
/// Return the parent PID of a process from `/proc/<pid>/stat`.
#[cfg(all(unix, not(target_os = "macos")))]
pub(super) fn ppid_of_linux(pid: u32) -> Option<u32> {
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/<pid>/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 ────────────────────────────
/// A snapshot of one process for the pure kill-decision function. Holds only
@@ -497,4 +630,92 @@ 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<u32, u32>, pid: u32) -> Option<u32> {
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<u32, u32> = [(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<u32, u32> =
[(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<u32, u32> = [(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<u32, u32> =
[(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<u32, u32> = [].into_iter().collect();
assert!(!walk_has_tracked_ancestor(400, &[100], |p| map_parent(
&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
)));
}
}
@@ -670,3 +670,91 @@ 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];
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.
let not_found =
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();
}
@@ -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).
@@ -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.