fix(acp): kill the whole process tree on Windows, not just the wrapper - #4732
Open
TheSmokeDev wants to merge 1 commit into
Open
fix(acp): kill the whole process tree on Windows, not just the wrapper#4732TheSmokeDev wants to merge 1 commit into
TheSmokeDev wants to merge 1 commit into
Conversation
`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 block#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 block#2492), so any before/after comparison would be noise rather than a receipt. Signed-off-by: SmokeDev <test@test.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
AcpClient::shutdown()kills the process group on Unix and falls back tostart_kill()everywhere else, becausekill_process_groupis#[cfg(not(unix))] -> false(crates/buzz-acp/src/acp.rs). On Windows that fallback is not equivalent.Rust runs a
.cmd/.batshim through an intermediatecmd.exe(post-CVE-2024-24576 behaviour), and npm installs the ACP agents as exactly that kind of shim —claude-code-acp,hermes-acp. Sostart_kill()reaps thecmd.exewrapper and leaves the real agent — plus any MCP servers it started — alive.Every timeout-then-respawn cycle therefore strands another live agent for the rest of the session. On a repeating failure it accumulates: I found this while tracing #4098, where the 60s timeout respawns the child in a loop.
Change
Adds the Windows arm using
taskkill /T /F, which covers the tree the waykillpgdoes on Unix, so teardown is symmetric across platforms.It deliberately mirrors
taskkill_treeindesktop/src-tauri/src/managed_agents/process_lifecycle.rs— the same problem, already solved once for managed-agent teardown — including itsCREATE_NO_WINDOWflag so cleanup never flashes a console at a GUI user. That also matches the existingconfigure_no_windowhelper in this file.unsafe, so the crate's#![deny(unsafe_code)]policy is preserved.#[cfg(windows)].#[cfg(not(any(unix, windows)))]fallback stays for other platforms.Verification
On Windows:
cargo check -p buzz-acp,cargo clippy -p buzz-acp --all-targets(0 warnings) andcargo fmt -p buzz-acp -- --check(no diffs) are all clean.I am not offering the crate's test suite as evidence either way. On this Windows box it is non-deterministic — a run against untouched
mainfails a different set each time, which is consistent with #2492 — so a before/after comparison would be noise, not a receipt. Happy to run anything specific you'd trust more.Related: #4098 (the respawn loop that surfaced this), #2492 (
just cion Windows).Duplicates: searched open PRs for
taskkill,kill_process_group, andprocess tree— none found.