fix(acp): kill the agent's whole process tree on Windows - #5944
fix(acp): kill the agent's whole process tree on Windows#5944Chessing234 wants to merge 3 commits into
Conversation
`shutdown` kills the child's process group on Unix, which reaps the harness's own workers (MCP servers, tool subprocesses) with it. Windows has no process groups, so `kill_process_group` returned false there and the caller fell back to `start_kill()` — the direct child only. Every cancel-drain timeout then left one tree behind: block#5849 reports 20 live `hermes-acp.exe` and 42 python children after a burst of failed turns, all under a single `buzz-acp`, none of them exiting. Terminate the tree with `taskkill /T /F`. Windows' job-object API would need `unsafe`, which this crate denies; taskkill ships with every supported Windows and needs no privilege beyond killing the child itself. The flags are the whole point of the change, so they are asserted from a pure command-builder on every platform. Signed-off-by: Taksh <takshkothari09@gmail.com>
themiguelamador
left a comment
There was a problem hiding this comment.
Requesting changes for two Windows process-lifecycle issues:
- High — executable search-path hijack.
Command::new("taskkill")supplies a bare executable name. WindowsCreateProcesssearch order checks the application directory and the parent process's current directory before the system directory. Because the ACP harness commonly runs with an agent-controlled repository as its current directory, a repository containingtaskkill.execould execute arbitrary code as the Buzz user whenever an agent shuts down or is replaced. Microsoft recommends a fully qualified path forCreateProcess-style launches. See CreateProcess executable search order and Microsoft's search-path security guidance. - Medium — synchronous process execution inside async shutdown.
.status()blocks untiltaskkillexits, butAcpClient::shutdowncalls it directly on the Tokio worker before entering the existing five-second child-wait timeout. A stalled system command can therefore pin an executor thread indefinitely and evade the shutdown bound entirely.
I fixed both in 37a1c7679: taskkill.exe is resolved beneath an absolute SystemRoot\\System32 (otherwise the existing direct-child fallback is used), and async shutdown moves the synchronous command to the blocking pool with its own five-second bound. The /T and /F behavior remains consistent with Microsoft's taskkill contract.
Verification:
cargo fmt --all -- --checkcargo test -p buzz-acp --lib— 779 passedcargo test -p buzz-acp --lib process_tree_kill_testscargo clippy -p buzz-acp --all-targets -- -D warningsgit diff --check
I also attempted the installed x86_64-pc-windows-msvc target check; as noted in the PR, this macOS host cannot complete the crate because native crypto dependencies require unavailable Windows SDK headers. The platform-neutral command construction is covered by the regression test, but behavior still needs a real Windows validation run.
Review, high severity: `Command::new("taskkill")` passes a bare executable
name. Windows `CreateProcess` resolves that against the application directory
and the parent process's *current* directory before it reaches the system
directory, and the ACP harness commonly runs with an agent-controlled
repository as its current directory. A `taskkill.exe` committed to such a
repository would have run as the Buzz user every time an agent was shut down
or replaced — which is every drain timeout this PR exists to handle.
The program is now resolved beneath an absolute `%SystemRoot%\System32`, per
Microsoft's guidance to fully qualify the path for `CreateProcess`-style
launches. A `SystemRoot` that is missing or not absolute yields `None` and the
existing direct-child fallback runs instead: leaking the tree is bad, running
an attacker-placed binary is worse.
`taskkill_program_in` checks "absolute" by Windows' own rules — a drive path
or a UNC path — rather than `Path::is_absolute`, so the check is exercised by
the test suite on every platform rather than only on a Windows runner.
- `cargo test -p buzz-acp --lib` — 781 passed
- `cargo clippy -p buzz-acp --all-targets -- -D warnings`
- `cargo fmt --all -- --check`
Signed-off-by: Taksh <takshkothari09@gmail.com>
Review: `.status()` blocks until `taskkill` exits, and `AcpClient::shutdown` called it directly on the Tokio worker — before entering the existing five-second child-wait timeout. A stalled system command could therefore pin an executor thread indefinitely and evade the shutdown bound entirely, which is the opposite of what a bounded shutdown is for. `kill_process_group_async` moves the Windows path to the blocking pool under its own five-second bound; a timeout there falls back to `start_kill()` the same way an unsuccessful kill does. Unix keeps calling `killpg` inline — it is a syscall that returns immediately and has nothing to offload. `Drop` still uses the synchronous form, since it cannot await. The match guard could not host an `.await`, so the call site is now a plain `let` binding and an `if`. - `cargo test -p buzz-acp --lib` — 781 passed - `cargo clippy -p buzz-acp --all-targets -- -D warnings` - `cargo fmt --all -- --check` Signed-off-by: Taksh <takshkothari09@gmail.com>
|
Both fixed, one commit each. 1. Search-path hijack. Agreed, and thanks — the current directory being an agent-controlled repository is exactly what makes this reachable. One deliberate difference from a plain 2. Blocking call in async shutdown. Right — the On Windows validation — same limit you hit: Verification:
|
Fixes #5849.
AcpClient::shutdownkills the child's process group — the child is spawned withprocess_group(0), so its PID is its PGID andkillpgtakes the harness's own workers (MCP servers, tool subprocesses) down with it. That path is#[cfg(unix)]. On Windowskill_process_groupreturnedfalseunconditionally, so the caller fell back tochild.start_kill(), which ends the direct child and nothing below it.Every cancel-drain timeout therefore leaked a tree. The report has 20
hermes-acp.exeand 42 python children under onebuzz-acp, spawned in a five-second burst, none of them exiting — and the respawn path is working exactly as designed on top of that, which is why the count grows with each failed turn rather than stabilising.This adds the Windows branch:
taskkill /PID <pid> /T /F./Tis the tree,/Fis unconditional — a drain timeout means the agent already ignored a polite stop. The job-object API would be the more elegant answer, but it needsunsafe, and this crate is#![deny(unsafe_code)];taskkillships with every supported Windows and needs no privilege beyond the one already required to kill the child. The non-Unix non-Windows fallback keeps its old behaviour.Verified:
cargo clippy --workspace --all-targets -- -D warningsclean,cargo fmt --all --checkclean,cargo test -p buzz-acp --lib779 passed. Thecfg(windows)code is type-checked forx86_64-pc-windows-msvc(I installed the target and compiled the branch in isolation — the crate itself cannot cross-compile here because a C dependency needs the MSVC toolchain), and the flags are asserted from a pure command-builder that runs on every platform. Not run: an actual Windows machine. I have no Windows host, so the behavioural claim rests ontaskkill's documented semantics, not on my having watched the tree die.