From 57a73b261192e9728b89a980a3f238a237f9997d Mon Sep 17 00:00:00 2001 From: SmokeDev Date: Tue, 4 Aug 2026 18:40:45 -0700 Subject: [PATCH] fix(acp): kill the whole process tree on Windows, not just the wrapper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `shutdown()` kills the process group on Unix and falls back to `start_kill()` everywhere else, because `kill_process_group` was `#[cfg(not(unix))] -> false`. On Windows that fallback is not equivalent. Rust runs a `.cmd`/`.bat` shim through an intermediate `cmd.exe` (the post-CVE-2024-24576 behaviour), and npm installs the ACP agents as exactly that kind of shim — `claude-code-acp`, `hermes-acp`. So `start_kill()` reaps `cmd.exe` and leaves the real agent, plus any MCP servers it started, running. Every timeout-then-respawn cycle strands another live agent for the rest of the session; on a repeating failure (see #4098) they accumulate. This adds the Windows arm using `taskkill /T /F`, which covers the tree the way `killpg` does on Unix, so teardown is symmetric across platforms. It mirrors `taskkill_tree` in the desktop crate, which solved the same problem for managed-agent teardown, including its `CREATE_NO_WINDOW` flag so cleanup never flashes a console at a GUI user — matching the existing `configure_no_window` in this file. No unsafe, so the crate's `#![deny(unsafe_code)]` policy is preserved. The non-Unix fallback stays for platforms that are neither, and the Unix path is untouched — the new code is `#[cfg(windows)]`. Verified on Windows: `cargo check`, `cargo clippy` and `cargo fmt --check` are clean for the crate. I did not use the crate's test suite as evidence either way: on this Windows box it is non-deterministic (a run on untouched main fails a different set each time, consistent with #2492), so any before/after comparison would be noise rather than a receipt. Signed-off-by: SmokeDev --- crates/buzz-acp/src/acp.rs | 39 +++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/crates/buzz-acp/src/acp.rs b/crates/buzz-acp/src/acp.rs index 700d5e8dcf..9941748f91 100644 --- a/crates/buzz-acp/src/acp.rs +++ b/crates/buzz-acp/src/acp.rs @@ -422,8 +422,9 @@ impl AcpClient { // ensures subprocesses (MCP servers, tool processes) are cleaned up // rather than orphaned to init. // - // Falls back to start_kill() (direct child only) on non-Unix or if - // the child has been polled to completion (id() returns None). + // Falls back to start_kill() (direct child only) when the platform + // has no tree-kill or the child has been polled to completion + // (id() returns None). match self.child.id() { Some(pid) if kill_process_group(pid) => {} _ => { @@ -2232,9 +2233,37 @@ fn kill_process_group(pid: u32) -> bool { killpg(Pid::from_raw(pid as i32), Signal::SIGKILL).is_ok() } -/// Fallback for non-Unix: process-group kill not available. -/// Returns `false` so the caller falls back to `child.start_kill()`. -#[cfg(not(unix))] +/// Kill an entire process tree on Windows via `taskkill /T /F`. +/// +/// `start_kill()` alone is not equivalent here. Rust runs a `.cmd`/`.bat` +/// shim (how npm installs `claude-code-acp`, `hermes-acp` and friends) +/// through an intermediate `cmd.exe`, so killing the direct child reaps the +/// wrapper and leaves the real agent — and its MCP servers — running. On a +/// timeout/respawn loop each cycle then strands another live agent for the +/// life of the session. +/// +/// `/T` covers the tree the way `killpg` does on Unix, keeping teardown +/// symmetric across platforms. `CREATE_NO_WINDOW` matches +/// [`configure_no_window`] so cleanup never flashes a console at a GUI user. +/// Mirrors `taskkill_tree` in the desktop crate, which solved the same +/// problem for managed-agent teardown. +#[cfg(windows)] +fn kill_process_group(pid: u32) -> bool { + use std::os::windows::process::CommandExt; + + const CREATE_NO_WINDOW: u32 = 0x0800_0000; + std::process::Command::new("taskkill") + .args(["/T", "/F", "/PID", &pid.to_string()]) + .creation_flags(CREATE_NO_WINDOW) + .status() + .map(|status| status.success()) + .unwrap_or(false) +} + +/// Fallback for platforms that are neither Unix nor Windows: process-group +/// kill not available. Returns `false` so the caller falls back to +/// `child.start_kill()`. +#[cfg(not(any(unix, windows)))] fn kill_process_group(_pid: u32) -> bool { false }