diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index e48c252272..f54e43c76b 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/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 diff --git a/desktop/src-tauri/src/commands/agents.rs b/desktop/src-tauri/src/commands/agents.rs index 4890063e58..1e651c939c 100644 --- a/desktop/src-tauri/src/commands/agents.rs +++ b/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); diff --git a/desktop/src-tauri/src/lib.rs b/desktop/src-tauri/src/lib.rs index 0f4621aa07..bf6cda9c01 100644 --- a/desktop/src-tauri/src/lib.rs +++ b/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 = 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, + } + + let mut to_stop: Vec = 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}"); + } } } + _ => {} }); } diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index be721ce92f..25ce6e0051 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/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//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 = 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, +) -> 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, @@ -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, ) -> 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!( diff --git a/desktop/src-tauri/src/managed_agents/storage.rs b/desktop/src-tauri/src/managed_agents/storage.rs index 4eed75f0e8..89c175ce70 100644 --- a/desktop/src-tauri/src/managed_agents/storage.rs +++ b/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 { + 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 { if !path.exists() { return Ok(String::new());