From 75c71a38a508d4956904a146ae846e12505e3ef5 Mon Sep 17 00:00:00 2001 From: matthiasdebernardini Date: Tue, 21 Jul 2026 21:00:29 -0500 Subject: [PATCH] fix(desktop): detach managed agents from the controlling terminal (setsid) When the desktop app is launched from a terminal (`just dev`), the managed agent harness subtree (buzz-acp -> claude-agent-acp -> engine + MCP servers) inherited the terminal's controlling TTY. Running as background process groups of that session, macOS job control intermittently STOPPED the actively-working agent's process group (SIGTTIN/SIGTTOU-class stops, observed as `T` state in ps), freezing agent turns indefinitely. Packaged .app launches have no controlling TTY and were unaffected. Spawn the harness via `pre_exec(setsid())` on unix so the whole agent tree runs in its own session with NO controlling terminal, making the freeze impossible. This replaces the previous `process_group(0)`: setsid makes the child both session AND process-group leader (pid == pgid), so the existing `-pid` group-kill paths still reach the entire tree. We drop `process_group(0)` because std may apply setpgid before pre_exec runs, which would make setsid fail EPERM. If setsid does fail, we fall back to a best-effort setpgid(0, 0) and never fail the spawn. Adds a unix test asserting the spawned child is a session leader in a session distinct from the test process. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PCWMx2BkoFDZtFRiDny16W --- .../src/managed_agents/process_lifecycle.rs | 5 +- .../src-tauri/src/managed_agents/runtime.rs | 35 +++++++++- .../src/managed_agents/runtime/tests.rs | 65 +++++++++++++++++++ .../src-tauri/src/managed_agents/storage.rs | 5 +- 4 files changed, 103 insertions(+), 7 deletions(-) diff --git a/desktop/src-tauri/src/managed_agents/process_lifecycle.rs b/desktop/src-tauri/src/managed_agents/process_lifecycle.rs index 1ef3eae010..dcc34e7642 100644 --- a/desktop/src-tauri/src/managed_agents/process_lifecycle.rs +++ b/desktop/src-tauri/src/managed_agents/process_lifecycle.rs @@ -1,6 +1,7 @@ //! Windows process-tree lifecycle primitives for managed agents. //! -//! The Unix teardown uses `process_group(0)` + group signals (in `runtime.rs`). +//! The Unix teardown uses `setsid()` (own session + process group) + group +//! signals (in `runtime.rs`). //! Windows has no process groups, so the harness's 24 agent workers + MCP //! servers are reaped two ways here: //! - [`JobHandle`] / [`create_job_for_child`] — the in-process stop path. A @@ -17,7 +18,7 @@ use windows_sys::Win32::Foundation::HANDLE; /// Win32 Job Object that owns the harness process and (via Windows' default /// child-inheritance) every process it spawns. Dropping the handle with /// `JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE` set kills the whole tree — the Windows -/// mirror of the Unix `process_group(0)` + group-signal teardown. This is what +/// mirror of the Unix `setsid()` + group-signal teardown. This is what /// guarantees the 24 agent workers + MCP servers die when we stop or when the /// app exits, instead of being orphaned by a bare `Child::kill()`. pub struct JobHandle(HANDLE); diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index 77b3a241e0..555bd4005f 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -1881,12 +1881,41 @@ pub fn spawn_agent_child( // env_clear(). command.env("BUZZ_MANAGED_AGENT", current_instance_id(app)); - // Spawn the harness in its own process group so we can kill the entire - // tree (harness + MCP servers + agent subprocesses) on shutdown. + // Spawn the harness in its OWN SESSION (setsid) so the entire agent tree + // (harness + adapter + engine + MCP servers) has no controlling terminal. + // + // WHY setsid and not process_group(0): when the desktop app is launched + // from a terminal (`just dev`), the harness subtree inherits that + // terminal's controlling TTY. As background process groups, macOS job + // control intermittently STOPS the actively-working agent's group + // (SIGTTIN/SIGTTOU-class stops, seen as `T` state in ps), freezing agent + // turns indefinitely. setsid() makes the child a new session leader with + // NO controlling terminal, so /dev/tty access can never stop the tree. + // Packaged .app launches have no controlling TTY and were unaffected. + // + // setsid() also makes the child both session AND process-group leader + // (pid == pgid), so the group-kill paths in `sigterm_then_sigkill` + // (which signal `-pid`) still reach the whole tree — the same invariant + // process_group(0) provided. Note we do NOT set process_group(0) here: + // std may apply setpgid() before pre_exec runs, which would make the child + // a group leader and cause setsid() to fail with EPERM. #[cfg(unix)] { use std::os::unix::process::CommandExt; - command.process_group(0); + // SAFETY: setsid/setpgid are async-signal-safe and touch only the + // child's own session/group. This file already relies on raw libc + // process control (see `sigterm_then_sigkill`'s `libc::kill`). + unsafe { + command.pre_exec(|| { + if libc::setsid() == -1 { + // EPERM: already a group leader. Fall back to a best-effort + // new process group so group-kill still works; do not fail + // the spawn over it. + let _ = libc::setpgid(0, 0); + } + Ok(()) + }); + } } // Windows: suppress the harness console window. Without this a bare // terminal pops for buzz-acp.exe and lingers (the app itself sets diff --git a/desktop/src-tauri/src/managed_agents/runtime/tests.rs b/desktop/src-tauri/src/managed_agents/runtime/tests.rs index 6a75867506..f1b334a86d 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/tests.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/tests.rs @@ -799,3 +799,68 @@ fn own_group_grandchild_detected_by_ancestor_walk() { unsafe { libc::kill(-(intermediate_pid as i32), libc::SIGKILL) }; let _ = intermediate.wait(); } + +/// Validates the TTY-detach fix: a child spawned with the same `pre_exec` +/// `setsid()` the harness uses becomes its own SESSION leader (sid == pid) in +/// a session distinct from the test process. This is what frees the agent tree +/// from the launching terminal's controlling TTY so background-group job +/// control (SIGTTIN/SIGTTOU stops) can no longer freeze agent turns. +/// +/// Note: this exercises the setsid pattern on a stand-in child, not +/// `spawn_agent_child` itself (which needs an `AppHandle`). It will NOT catch +/// a regression that re-adds `process_group(0)` to the harness `Command` — +/// that would make `setsid()` fail EPERM at spawn time and silently reattach +/// the tree to the launching terminal's session. Keep the spawn free of +/// `process_group` calls. +#[cfg(unix)] +#[test] +fn spawned_harness_is_session_leader_in_new_session() { + use std::os::unix::process::CommandExt; + use std::process::Command; + + let parent_sid = unsafe { libc::getsid(0) }; + + let mut child = { + let mut cmd = Command::new("sleep"); + cmd.arg("30").stdin(std::process::Stdio::null()); + // Mirror the production spawn: detach into a new session with no + // controlling terminal. + unsafe { + cmd.pre_exec(|| { + if libc::setsid() == -1 { + let _ = libc::setpgid(0, 0); + } + Ok(()) + }); + } + cmd.spawn().expect("spawn detached sleep") + }; + + let child_pid = child.id() as i32; + + // The child is a session leader: its session id equals its own pid. + let child_sid = unsafe { libc::getsid(child_pid) }; + assert_eq!( + child_sid, child_pid, + "child should be its own session leader (sid == pid) after setsid()" + ); + + // And it is a DIFFERENT session than the test process — no shared + // controlling terminal. + assert_ne!( + child_sid, parent_sid, + "child session must differ from the test process session" + ); + + // setsid also makes the child a process-group leader (pid == pgid), so the + // production group-kill path (`kill(-pid, ...)`) still reaches the tree. + let child_pgid = unsafe { libc::getpgid(child_pid) }; + assert_eq!( + child_pgid, child_pid, + "child should be its own process-group leader (pgid == pid) after setsid()" + ); + + // Cleanup: kill the child's process group. + unsafe { libc::kill(-child_pid, libc::SIGKILL) }; + let _ = child.wait(); +} diff --git a/desktop/src-tauri/src/managed_agents/storage.rs b/desktop/src-tauri/src/managed_agents/storage.rs index 7eb07b9741..95d76d5a73 100644 --- a/desktop/src-tauri/src/managed_agents/storage.rs +++ b/desktop/src-tauri/src/managed_agents/storage.rs @@ -598,8 +598,9 @@ fn agent_pids_dir(app: &AppHandle) -> Result { Ok(dir) } -/// Write a PID file for a spawned agent. The PID equals the PGID since we -/// spawn with `process_group(0)`. +/// Write a PID file for a spawned agent. The PID equals the PGID since the +/// spawn's `pre_exec` calls `setsid()` (making the harness a session and +/// process-group leader). pub fn write_agent_pid_file(app: &AppHandle, pubkey: &str, pid: u32) -> Result<(), String> { let path = agent_pids_dir(app)?.join(format!("{pubkey}.pid")); fs::write(&path, pid.to_string())