From 3d15fcb40952138e9ba49b783481e1c5ed67c493 Mon Sep 17 00:00:00 2001 From: damocles Date: Fri, 19 Jun 2026 08:20:22 +0200 Subject: [PATCH] hive-bash-mcp: kill is fire-and-forget; retry with force to escalate; must_use (review) --- hive-bash-mcp/src/bin/mcp.rs | 20 ++++++----- hive-bash-mcp/src/runner.rs | 64 ++++++++++++++---------------------- 2 files changed, 36 insertions(+), 48 deletions(-) diff --git a/hive-bash-mcp/src/bin/mcp.rs b/hive-bash-mcp/src/bin/mcp.rs index e3bbc427..10ba08b8 100644 --- a/hive-bash-mcp/src/bin/mcp.rs +++ b/hive-bash-mcp/src/bin/mcp.rs @@ -221,10 +221,11 @@ struct BashKillArgs { /// Task ID (from `run`) to kill. id: String, /// `false` (default): SIGINT to the task's process group — graceful, the - /// process can clean up — escalating to SIGKILL after a grace period if it - /// doesn't exit. `true`: SIGKILL immediately. Either way the whole process + /// process can clean up. `true`: SIGKILL. Either way the whole process /// group is signalled, so children of the shell (e.g. a `cargo`/`nix` - /// invocation) are stopped too, not just the shell itself. + /// invocation) are stopped too, not just the shell. Fire-and-forget: the + /// call sends the signal and returns without waiting. If a SIGINT'd task + /// doesn't exit, call `kill` again with `force: true` to send SIGKILL. #[serde(default)] force: bool, } @@ -290,12 +291,13 @@ impl BashMcp { #[tool( description = "Kill a background bash task you started (by its ID from `run`). \ - `force: false` (default) sends SIGINT — graceful, lets the process clean up — \ - then escalates to SIGKILL after a grace period if it doesn't exit; `force: true` \ - sends SIGKILL immediately. Either way the task's whole process group is signalled, \ - so a runaway child (cargo/nix/etc.) is stopped too, not just the shell. A \ - still-pending task is cancelled before it starts. The task ends as `killed` and \ - fires the usual completion wake." + `force: false` (default) sends SIGINT — graceful, lets the process clean up; \ + `force: true` sends SIGKILL. Either way the task's whole process group is \ + signalled, so a runaway child (cargo/nix/etc.) is stopped too, not just the shell. \ + Fire-and-forget: sends the signal and returns without waiting. If a SIGINT'd task \ + doesn't exit, call kill again with `force: true` to SIGKILL. A still-pending task \ + is cancelled before it starts. The task ends as `killed` and fires the usual \ + completion wake." )] async fn kill(&self, Parameters(args): Parameters) -> String { let req = DaemonRequest::BashKill { diff --git a/hive-bash-mcp/src/runner.rs b/hive-bash-mcp/src/runner.rs index c18494dc..058b9932 100644 --- a/hive-bash-mcp/src/runner.rs +++ b/hive-bash-mcp/src/runner.rs @@ -40,10 +40,6 @@ pub const MAX_WAIT_SECS: u64 = 30; /// Poll interval used by the inline-wait loops. const POLL_MS: u64 = 100; -/// Grace window after a graceful (`SIGINT`) kill before escalating to -/// `SIGKILL` — bounds how long a SIGINT-ignoring process can linger. -const KILL_GRACE: Duration = Duration::from_secs(10); - static TASK_SEQ: AtomicU64 = AtomicU64::new(0); // --------------------------------------------------------------------------- @@ -300,12 +296,14 @@ pub async fn wait_for_task(id: &str, wait_secs: u64) -> Option { /// Kill a running or still-pending task. /// +/// Fire-and-forget: this **sends a signal and returns** — it does not wait +/// for the process to actually exit. +/// /// - **Running** (in the registry): signals its process group via the -/// `exec_cmd` cancel path. `force` → `SIGKILL`. Otherwise `SIGINT`, then -/// `SIGKILL` if the process hasn't exited within [`KILL_GRACE`] — so a -/// SIGINT-ignoring process is still stopped rather than lingering. The task -/// transitions to [`TaskStatus::Killed`] once the process exits and a -/// completion wake fires as usual. +/// `exec_cmd` cancel path — `SIGKILL` if `force`, else `SIGINT`. If the +/// process ignores `SIGINT`, the caller re-invokes with `force` to send +/// `SIGKILL` (no daemon-side auto-escalation). The task transitions to +/// [`TaskStatus::Killed`] once the process exits and a completion wake fires. /// - **Pending** (queued, not yet started): marked `Killed` directly so the /// runner loop never starts it. No process exists yet, so `force` is moot. /// - **Terminal or unknown id**: no-op. @@ -318,6 +316,7 @@ pub async fn wait_for_task(id: &str, wait_secs: u64) -> Option { /// Panics if the running-task registry mutex is poisoned (a prior holder /// panicked) — unrecoverable, consistent with the rest of the daemon's /// `Mutex` usage. +#[must_use] pub fn kill_task(id: &str, force: bool) -> (bool, bool) { // Running task: signal via the registry. { @@ -594,37 +593,24 @@ async fn exec_cmd( let wait = child.wait(); tokio::pin!(wait); tokio::pin!(timeout_fut); - tokio::select! { - res = &mut wait => res, - () = &mut timeout_fut => { - signal_group(pgid, libc::SIGKILL); - timed_out = true; - (&mut wait).await - } - () = cancel.notified() => { - if force.load(Ordering::SeqCst) { - // Forced: SIGKILL can't be caught/ignored. + loop { + tokio::select! { + res = &mut wait => break res, + () = &mut timeout_fut => { + // Per-task timeout: hard-kill the whole group and reap. signal_group(pgid, libc::SIGKILL); - killed = Some(true); - (&mut wait).await - } else { - // Graceful: SIGINT first, then escalate to SIGKILL if the - // process hasn't exited within the grace window — so a - // SIGINT-ignoring process is still stopped rather than - // leaving the task hung "running" (silent no-op). The - // reported signal reflects whichever actually ended it. - signal_group(pgid, libc::SIGINT); - match tokio::time::timeout(KILL_GRACE, &mut wait).await { - Ok(res) => { - killed = Some(false); - res - } - Err(_elapsed) => { - signal_group(pgid, libc::SIGKILL); - killed = Some(true); - (&mut wait).await - } - } + timed_out = true; + break (&mut wait).await; + } + () = cancel.notified() => { + // A kill was requested: send the signal and keep waiting. + // `kill` is fire-and-forget — we do NOT block-for-exit or + // auto-escalate. If the process ignores SIGINT the caller + // re-invokes kill with `force`, which re-fires this arm + // with SIGKILL. `killed` tracks the last signal sent. + let forced = force.load(Ordering::SeqCst); + signal_group(pgid, if forced { libc::SIGKILL } else { libc::SIGINT }); + killed = Some(forced); } } }