Fix shutdown: kill all agent processes reliably (#227)

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

desktop/scripts/check-file-sizes.mjs

@@ -43,6 +43,7 @@ const overrides = new Map([
   ["src/shared/api/relayClientSession.ts", 790], // durable websocket session manager with reconnect/replay/recovery state + sendTypingIndicator + fetchChannelHistoryBefore
   ["src/shared/api/tauri.ts", 1100], // remote agent provider API bindings + canvas API functions
   ["src-tauri/src/commands/agents.rs", 849], // remote agent lifecycle routing (local + provider branches) + scope enforcement; rustfmt adds line breaks around long tuple/closure blocks
+  ["src-tauri/src/managed_agents/runtime.rs", 650], // KNOWN_AGENT_BINARIES const + process_belongs_to_us FFI (macOS proc_name + Linux /proc/comm) + terminate_process + start/stop/sync lifecycle
   ["src-tauri/src/managed_agents/backend.rs", 530], // provider IPC, validation, discovery, binary resolution + tests
   ["src/features/agents/ui/AgentsView.tsx", 790], // remote agent stop/delete + channel UUID resolution + presence-aware delete guard + persona/team import + provider/model fields
   ["src/features/agents/ui/CreateAgentDialog.tsx", 685], // provider selector + config form + schema-typed config coercion + required field validation + locked scopes

desktop/src-tauri/src/commands/agents.rs

@@ -611,7 +611,7 @@ pub fn stop_managed_agent(
                 "remote agents are stopped via !shutdown message, not this command".to_string(),
             );
         }
-        stop_managed_agent_process(record, &mut runtimes)?;
+        stop_managed_agent_process(&app, record, &mut runtimes)?;
     }
     save_managed_agents(&app, &records)?;
     let record = records
@@ -662,7 +662,7 @@ pub fn delete_managed_agent(
     if let Some(record) = records.iter_mut().find(|record| record.pubkey == pubkey) {
         // For local agents: kills the process. For remote agents: no-op (the frontend
         // sends !shutdown via WebSocket before calling delete). Either way, safe.
-        stop_managed_agent_process(record, &mut runtimes)?;
+        stop_managed_agent_process(&app, record, &mut runtimes)?;
     }
     let initial_len = records.len();
     records.retain(|record| record.pubkey != pubkey);

desktop/src-tauri/src/lib.rs

@@ -10,8 +10,8 @@ mod util;
 use app_state::{build_app_state, resolve_persisted_identity, AppState};
 use commands::*;
 use managed_agents::{
-    find_managed_agent_mut, load_managed_agents, save_managed_agents, start_managed_agent_process,
-    stop_managed_agent_process, sync_managed_agent_processes, BackendKind,
+    find_managed_agent_mut, kill_stale_tracked_processes, load_managed_agents, save_managed_agents,
+    start_managed_agent_process, sync_managed_agent_processes, BackendKind, ManagedAgentProcess,
 };
 use tauri::{http, Manager, RunEvent};
 use tauri_plugin_window_state::StateFlags;
@@ -28,6 +28,17 @@ fn restore_managed_agents_on_launch(app: &tauri::AppHandle) -> Result<(), String
         .lock()
         .map_err(|error| error.to_string())?;
     let mut changed = sync_managed_agent_processes(&mut records, &mut runtimes);
+    changed |= kill_stale_tracked_processes(&mut records, &runtimes);
+
+    // PID-file sweep: kill any orphaned agent processes we have receipts for
+    // that weren’t tracked in records (e.g. escaped process groups, double-forked).
+    let tracked_pids: Vec<u32> = records
+        .iter()
+        .filter_map(|r| r.runtime_pid)
+        .chain(runtimes.values().map(|rt| rt.child.id()))
+        .collect();
+    managed_agents::sweep_orphaned_agent_processes(app, &tracked_pids);
+
     let pubkeys_to_restore = records
         .iter()
         .filter(|record| record.start_on_app_launch && record.backend == BackendKind::Local)
@@ -67,20 +78,106 @@ fn shutdown_managed_agents(app: &tauri::AppHandle) -> Result<(), String> {
         .lock()
         .map_err(|error| error.to_string())?;
     let mut changed = sync_managed_agent_processes(&mut records, &mut runtimes);
+    changed |= kill_stale_tracked_processes(&mut records, &runtimes);
 
-    for record in records.iter_mut() {
-        // Only stop Local agents — Provider agents are managed externally.
+    // Stop all tracked agents. Send SIGTERM to all process
+    // groups first, then wait for exits in parallel to avoid serial 1s waits.
+    struct AgentToStop {
+        idx: usize,
+        pid: u32,
+        runtime: Option<ManagedAgentProcess>,
+    }
+
+    let mut to_stop: Vec<AgentToStop> = Vec::new();
+    for (idx, record) in records.iter_mut().enumerate() {
         if record.backend != BackendKind::Local {
             continue;
         }
         if record.runtime_pid.is_none() && !runtimes.contains_key(&record.pubkey) {
             continue;
         }
-
-        stop_managed_agent_process(record, &mut runtimes)?;
-        changed = true;
+        let runtime = runtimes.remove(&record.pubkey);
+        let Some(pid) = runtime
+            .as_ref()
+            .map(|rt| rt.child.id())
+            .or(record.runtime_pid)
+        else {
+            continue;
+        };
+        to_stop.push(AgentToStop { idx, pid, runtime });
     }
 
+    if !to_stop.is_empty() {
+        changed = true;
+
+        // Fan-out: send SIGTERM to all process groups at once.
+        #[cfg(unix)]
+        for agent in &to_stop {
+            let pgid = -(agent.pid as i32);
+            unsafe {
+                libc::kill(pgid, libc::SIGTERM);
+            }
+        }
+
+        // Wait up to 2s for all to exit, checking in a polling loop.
+        let deadline = std::time::Instant::now() + std::time::Duration::from_secs(2);
+        loop {
+            if to_stop
+                .iter()
+                .all(|a| !managed_agents::process_is_running(a.pid))
+            {
+                break;
+            }
+            if std::time::Instant::now() >= deadline {
+                break;
+            }
+            std::thread::sleep(std::time::Duration::from_millis(50));
+        }
+
+        // Fan-out: SIGKILL any survivors.
+        #[cfg(unix)]
+        for agent in &to_stop {
+            if managed_agents::process_is_running(agent.pid) {
+                let pgid = -(agent.pid as i32);
+                unsafe {
+                    libc::kill(pgid, libc::SIGKILL);
+                }
+            }
+        }
+
+        // Reap children and update records.
+        for mut agent in to_stop {
+            if let Some(ref mut rt) = agent.runtime {
+                // Best-effort reap — don’t block shutdown if the child is stuck
+                // in uninterruptible sleep. The zombie will be cleaned up when
+                // our process exits and launchd reaps it.
+                let _ = rt.child.try_wait();
+                // Write log marker (best-effort).
+                let record = &records[agent.idx];
+                let _ = managed_agents::append_log_marker(
+                    &rt.log_path,
+                    &format!(
+                        "=== stopped {} ({}) at {} ===",
+                        record.name,
+                        record.pubkey,
+                        util::now_iso()
+                    ),
+                );
+            }
+            let record = &mut records[agent.idx];
+            record.runtime_pid = None;
+            record.last_stopped_at = Some(util::now_iso());
+            record.updated_at = util::now_iso();
+            record.last_exit_code = None;
+            record.last_error = None;
+        }
+    }
+
+    // Final sweep: kill any orphaned agent processes we have PID file receipts
+    // for that escaped process-group kills or weren't tracked in records.
+    // All tracked PIDs have already been killed above, so pass an empty skip list.
+    managed_agents::sweep_orphaned_agent_processes(app, &[]);
+
     if changed {
         save_managed_agents(app, &records)?;
     }
@@ -297,12 +394,16 @@ pub fn run() {
         .build(tauri::generate_context!())
         .expect("error while building tauri application");
 
-    app.run(|app_handle, event| {
-        if matches!(event, RunEvent::ExitRequested { .. }) {
-            if let Err(error) = shutdown_managed_agents(app_handle) {
-                eprintln!("sprout-desktop: failed to stop managed agents: {error}");
+    let shutdown_done = std::sync::atomic::AtomicBool::new(false);
+    app.run(move |app_handle, event| match event {
+        RunEvent::ExitRequested { .. } | RunEvent::Exit => {
+            if !shutdown_done.swap(true, std::sync::atomic::Ordering::SeqCst) {
+                if let Err(error) = shutdown_managed_agents(app_handle) {
+                    eprintln!("sprout-desktop: failed to stop managed agents: {error}");
+                }
             }
         }
+        _ => {}
     });
 }
 

desktop/src-tauri/src/managed_agents/runtime.rs

@@ -1,4 +1,4 @@
-use std::{collections::HashMap, process::Command};
+use std::collections::HashMap;
 
 use tauri::AppHandle;
 
@@ -11,60 +11,228 @@ use crate::{
     util::now_iso,
 };
 
+/// Binary name fragments for all known agent/harness processes that Sprout
+/// may spawn. Used by `process_belongs_to_us()` and the orphan sweep to
+/// identify processes we should clean up. Both hyphenated and underscored
+/// variants are listed because macOS `proc_name()` and Linux `/proc/comm`
+/// may report either form depending on how the binary was built.
+pub(crate) const KNOWN_AGENT_BINARIES: &[&str] = &[
+    "sprout-acp",
+    "sprout_acp",
+    "claude-agent-acp",
+    "claude_agent_acp",
+    "claude-code-acp",
+    "claude_code_acp",
+    "codex-acp",
+    "codex_acp",
+    "goose",
+    "sprout-mcp",
+    "sprout_mcp",
+];
+
+/// Check if a process name matches any of our known agent binaries.
+/// Uses exact match or prefix-with-separator to avoid false positives
+/// (e.g. `"goose"` must not match `"mongoose"`).
+fn name_matches_known_binary(name: &str) -> bool {
+    KNOWN_AGENT_BINARIES.iter().any(|&binary| {
+        name == binary || {
+            name.starts_with(binary) && {
+                let rest = &name[binary.len()..];
+                rest.starts_with('-') || rest.starts_with('_') || rest.starts_with('.')
+            }
+        }
+    })
+}
+
 #[cfg(unix)]
-fn process_is_running(pid: u32) -> bool {
-    Command::new("kill")
-        .arg("-0")
-        .arg(pid.to_string())
-        .status()
-        .map(|status| status.success())
+pub(crate) fn process_is_running(pid: u32) -> bool {
+    // Use libc::kill with signal 0 instead of forking a subprocess.
+    // Returns true only if the process exists AND we can signal it.
+    // Returns false for non-existent PIDs (ESRCH) and PIDs owned by
+    // other users (EPERM) — callers should not interact with those.
+    unsafe { libc::kill(pid as i32, 0) == 0 }
+}
+
+#[cfg(not(unix))]
+pub(crate) fn process_is_running(_pid: u32) -> bool {
+    false
+}
+
+/// Check if a PID belongs to a known agent process we spawned.
+/// Returns false for recycled PIDs that now belong to other processes.
+#[cfg(target_os = "macos")]
+pub(crate) fn process_belongs_to_us(pid: u32) -> bool {
+    // Use proc_name() from libproc to get the process name without spawning
+    // a subprocess.
+    extern "C" {
+        fn proc_name(pid: libc::c_int, buffer: *mut libc::c_void, buffersize: u32) -> libc::c_int;
+    }
+    let mut buf = [0u8; 1024];
+    let len = unsafe {
+        proc_name(
+            pid as i32,
+            buf.as_mut_ptr() as *mut libc::c_void,
+            buf.len() as u32,
+        )
+    };
+    if len <= 0 {
+        return false;
+    }
+    let name = String::from_utf8_lossy(&buf[..len as usize]);
+    name_matches_known_binary(&name)
+}
+
+#[cfg(all(unix, not(target_os = "macos")))]
+pub(crate) fn process_belongs_to_us(pid: u32) -> bool {
+    // On Linux, read /proc/<pid>/comm
+    std::fs::read_to_string(format!("/proc/{pid}/comm"))
+        .map(|name| name_matches_known_binary(name.trim()))
         .unwrap_or(false)
 }
 
 #[cfg(not(unix))]
-fn process_is_running(_pid: u32) -> bool {
+pub(crate) fn process_belongs_to_us(_pid: u32) -> bool {
     false
 }
 
 #[cfg(unix)]
-fn terminate_process(pid: u32) -> Result<(), String> {
-    let pid_arg = pid.to_string();
-    let status = Command::new("kill")
-        .arg("-TERM")
-        .arg(&pid_arg)
-        .status()
-        .map_err(|error| format!("failed to terminate process {pid}: {error}"))?;
-    if !status.success() && process_is_running(pid) {
-        return Err(format!(
-            "failed to terminate process {pid}: signal was rejected"
-        ));
+pub(crate) fn terminate_process(pid: u32) -> Result<(), String> {
+    // The child was spawned with process_group(0), so pid == pgid.
+    // Kill the entire process group to avoid orphaning MCP servers
+    // and agent subprocesses.
+    let pgid = -(pid as i32);
+
+    // Try graceful shutdown first (SIGTERM to the group).
+    if unsafe { libc::kill(pgid, libc::SIGTERM) } != 0 {
+        // ESRCH means the process is already gone — that's fine.
+        let err = std::io::Error::last_os_error();
+        if err.raw_os_error() != Some(libc::ESRCH) && process_is_running(pid) {
+            return Err(format!("failed to terminate process group {pid}: {err}"));
+        }
+        return Ok(());
     }
 
+    // Wait up to 1s for graceful exit.
     for _ in 0..10 {
         if !process_is_running(pid) {
             return Ok(());
         }
-
         std::thread::sleep(std::time::Duration::from_millis(100));
     }
 
-    let kill_status = Command::new("kill")
-        .arg("-KILL")
-        .arg(&pid_arg)
-        .status()
-        .map_err(|error| format!("failed to kill process {pid}: {error}"))?;
-    if !kill_status.success() && process_is_running(pid) {
-        return Err(format!("failed to kill process {pid}: signal was rejected"));
+    // Escalate to SIGKILL on the entire group.
+    if unsafe { libc::kill(pgid, libc::SIGKILL) } != 0 {
+        let err = std::io::Error::last_os_error();
+        if err.raw_os_error() != Some(libc::ESRCH) && process_is_running(pid) {
+            return Err(format!("failed to kill process group {pid}: {err}"));
+        }
     }
 
     Ok(())
 }
 
 #[cfg(not(unix))]
-fn terminate_process(_pid: u32) -> Result<(), String> {
+pub(crate) fn terminate_process(_pid: u32) -> Result<(), String> {
     Err("managed agent shutdown after app restart is only supported on Unix".to_string())
 }
 
+/// Send SIGTERM to all given PIDs (as process groups), wait, then SIGKILL
+/// any survivors. Uses `-pid` to kill the entire process group — if an
+/// orphaned agent called `setsid()`, it IS the group leader, so this
+/// reaches its children too.
+#[cfg(unix)]
+fn sigterm_then_sigkill(pids: &[i32]) {
+    // Send SIGTERM to each process group. Track whether any signal was
+    // actually delivered so we can skip the sleep when everything is
+    // already gone.
+    let mut any_signalled = false;
+    for &pid in pids {
+        if unsafe { libc::kill(-pid, libc::SIGTERM) } == 0 {
+            any_signalled = true;
+        }
+    }
+
+    if !any_signalled {
+        return;
+    }
+
+    std::thread::sleep(std::time::Duration::from_millis(200));
+
+    for &pid in pids {
+        if process_is_running(pid as u32) {
+            unsafe {
+                libc::kill(-pid, libc::SIGKILL);
+            }
+        }
+    }
+}
+
+/// Kill orphaned agent processes using PID file receipts. Reads all files from
+/// `agent-pids/`, verifies each PID still belongs to a known agent binary,
+/// then kills the process group. Deletes the PID file after killing.
+///
+/// `skip_pids` are PIDs already handled by the tracked-agent path.
+#[cfg(unix)]
+pub(crate) fn sweep_orphaned_agent_processes(app: &AppHandle, skip_pids: &[u32]) {
+    let entries = super::read_all_agent_pid_files(app);
+    let orphans: Vec<i32> = entries
+        .iter()
+        .filter(|(_, pid)| {
+            !skip_pids.contains(pid) && process_is_running(*pid) && process_belongs_to_us(*pid)
+        })
+        .map(|(_, pid)| *pid as i32)
+        .collect();
+
+    if !orphans.is_empty() {
+        sigterm_then_sigkill(&orphans);
+    }
+
+    // Clean up PID files for processes we just killed or that are already gone.
+    for (pubkey, pid) in &entries {
+        if skip_pids.contains(pid) {
+            continue;
+        }
+        if !process_is_running(*pid) || !process_belongs_to_us(*pid) {
+            super::remove_agent_pid_file(app, pubkey);
+        }
+    }
+}
+
+#[cfg(not(unix))]
+pub(crate) fn sweep_orphaned_agent_processes(app: &AppHandle, _skip_pids: &[u32]) {
+    let _ = app;
+}
+
+/// Kill stale agent processes from a previous session whose PID is still alive
+/// but not tracked in the current `runtimes` map. Updates the record fields and
+/// returns `true` if any records were modified.
+pub fn kill_stale_tracked_processes(
+    records: &mut [ManagedAgentRecord],
+    runtimes: &HashMap<String, ManagedAgentProcess>,
+) -> bool {
+    use crate::managed_agents::BackendKind;
+
+    let mut changed = false;
+    for record in records.iter_mut() {
+        if record.backend != BackendKind::Local {
+            continue;
+        }
+        let Some(pid) = record.runtime_pid else {
+            continue;
+        };
+        if !runtimes.contains_key(&record.pubkey) {
+            if process_belongs_to_us(pid) {
+                let _ = terminate_process(pid);
+            }
+            record.runtime_pid = None;
+            record.last_stopped_at = Some(crate::util::now_iso());
+            record.updated_at = crate::util::now_iso();
+            changed = true;
+        }
+    }
+    changed
+}
+
 pub fn sync_managed_agent_processes(
     records: &mut [ManagedAgentRecord],
     runtimes: &mut HashMap<String, ManagedAgentProcess>,
@@ -333,6 +501,14 @@ pub fn start_managed_agent_process(
         command.env_remove("SPROUT_API_TOKEN");
     }
 
+    // Spawn the harness in its own process group so we can kill the entire
+    // tree (harness + MCP servers + agent subprocesses) on shutdown.
+    #[cfg(unix)]
+    {
+        use std::os::unix::process::CommandExt;
+        command.process_group(0);
+    }
+
     let child = command.spawn().map_err(|error| {
         format!(
             "failed to spawn `{}` for agent {}: {error}",
@@ -341,6 +517,10 @@ pub fn start_managed_agent_process(
         )
     })?;
 
+    // Write a PID file so the orphan sweep can find this process even if the
+    // record is stale or the app crashes before updating records.
+    let _ = super::write_agent_pid_file(app, &record.pubkey, child.id());
+
     let now = now_iso();
     record.updated_at = now.clone();
     record.runtime_pid = Some(child.id());
@@ -357,6 +537,7 @@ pub fn start_managed_agent_process(
 }
 
 pub fn stop_managed_agent_process(
+    app: &AppHandle,
     record: &mut ManagedAgentRecord,
     runtimes: &mut HashMap<String, ManagedAgentProcess>,
 ) -> Result<(), String> {
@@ -373,10 +554,11 @@ pub fn stop_managed_agent_process(
             record.last_exit_code = None;
             record.last_error = None;
         }
+        super::remove_agent_pid_file(app, &record.pubkey);
         return Ok(());
     };
 
-    let _ = runtime.child.kill();
+    terminate_process(runtime.child.id())?;
     let status = runtime
         .child
         .wait()
@@ -388,6 +570,8 @@ pub fn stop_managed_agent_process(
     record.last_exit_code = status.code();
     record.last_error = None;
 
+    super::remove_agent_pid_file(app, &record.pubkey);
+
     append_log_marker(
         &runtime.log_path,
         &format!(

desktop/src-tauri/src/managed_agents/storage.rs

@@ -71,6 +71,48 @@ pub(crate) fn append_log_marker(path: &Path, message: &str) -> Result<(), String
     writeln!(file, "{message}").map_err(|error| format!("failed to write log marker: {error}"))
 }
 
+fn agent_pids_dir(app: &AppHandle) -> Result<PathBuf, String> {
+    let dir = managed_agents_base_dir(app)?.join("agent-pids");
+    fs::create_dir_all(&dir)
+        .map_err(|error| format!("failed to create agent-pids dir: {error}"))?;
+    Ok(dir)
+}
+
+/// Write a PID file for a spawned agent. The PID equals the PGID since we
+/// spawn with `process_group(0)`.
+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())
+        .map_err(|error| format!("failed to write PID file {}: {error}", path.display()))
+}
+
+/// Remove the PID file for an agent (e.g. on normal stop).
+pub fn remove_agent_pid_file(app: &AppHandle, pubkey: &str) {
+    if let Ok(dir) = agent_pids_dir(app) {
+        let _ = fs::remove_file(dir.join(format!("{pubkey}.pid")));
+    }
+}
+
+/// Read all PID files from `agent-pids/`, returning `(pubkey, pid)` pairs.
+pub fn read_all_agent_pid_files(app: &AppHandle) -> Vec<(String, u32)> {
+    let Ok(dir) = agent_pids_dir(app) else {
+        return Vec::new();
+    };
+    let Ok(entries) = fs::read_dir(&dir) else {
+        return Vec::new();
+    };
+    entries
+        .flatten()
+        .filter_map(|entry| {
+            let name = entry.file_name();
+            let name = name.to_str()?;
+            let pubkey = name.strip_suffix(".pid")?;
+            let pid: u32 = fs::read_to_string(entry.path()).ok()?.trim().parse().ok()?;
+            Some((pubkey.to_string(), pid))
+        })
+        .collect()
+}
+
 pub fn read_log_tail(path: &Path, max_lines: usize) -> Result<String, String> {
     if !path.exists() {
         return Ok(String::new());