Skip to content

Windows: kill the process tree, not a POSIX process group (stopped turns orphan their MCP servers) - #43

Merged
milind-soni merged 1 commit into
milind-soni:mainfrom
E4B-labs:fix/windows-kill-tree
Aug 14, 2026
Merged

Windows: kill the process tree, not a POSIX process group (stopped turns orphan their MCP servers)#43
milind-soni merged 1 commit into
milind-soni:mainfrom
E4B-labs:fix/windows-kill-tree

Conversation

@E4B-labs

Copy link
Copy Markdown
Contributor

What's broken

Stopping a turn on Windows leaves the CLI's MCP servers running. They accumulate for as long as the app runs — every interrupted or completed turn orphans another set of node processes, holding ports, file handles and memory.

Root cause

Every driver spawns its CLI with detached: true specifically so the CLI leads its own process group, and then reaps the whole group:

const stop = () => {
  try {
    process.kill(-child.pid!, "SIGTERM");   // the group: CLI + its MCP servers
  } catch {
    try { child.kill("SIGTERM"); } catch {}  // fallback
  }
};

Process groups are a POSIX concept. On Windows process.kill(-pid, ...) throws EINVAL immediately, the catch swallows it, and the fallback child.kill() terminates the CLI alone. Every server and helper the CLI started survives as an orphan.

So the fallback is not a degraded path on Windows — it is the only path, and it is the wrong one.

The fix

Windows tracks a parent/child tree rather than process groups, and taskkill /T walks it. server/kill-tree.ts picks the right mechanism per platform behind one call:

const stop = () => killTree(child);

The POSIX branch is the existing code, unchanged, including its child.kill() fallback. The win32 branch is taskkill /T /F /PID <pid>, with child.kill() as its own fallback if taskkill is unavailable or the process is already gone.

killTree keeps the contract stop() had: best-effort, synchronous to call, never throws, and a no-op if the child has already exited.

Used by all three drivers (claude, codex, acp/core) in place of the identical inline stop() each carried.

Tests

server/kill-tree.test.ts is new: it spawns a stand-in CLI that itself spawns one helper, calls killTree on the parent, and asserts the grandchild is gone — which is the entire point of spawning detached, and exactly what the old code failed to do on Windows. It runs on both platforms and would fail on Windows against main.

How this was tested

  • pnpm typecheck and pnpm test on Windows 10: 53 passed / 33 skipped (52/33 on main, plus the new test). No regressions.
  • The new test verified both ways on Windows: passing with killTree, and confirmed the grandchild survives with the old inline stop().
  • POSIX behaviour is unchanged by construction — that branch is the original code verbatim. The test reads the parent's death off the ChildProcess object rather than its pid, so a POSIX zombie window cannot make it flaky.

Note on merge order

This touches the same import { augmentedPath } from "../env-path.ts" lines in the three drivers as #41, and lines adjacent to the claude spawn block. Whichever lands second needs a trivial rebase — happy to do it on request, in either order.

🤖 Generated with Claude Code

https://claude.ai/code/session_01HgJeiantdRcZSBrc5sqqCp

Stopping a turn on Windows leaves the CLI's MCP servers running.

Every driver spawns its CLI with `detached: true` specifically so the
CLI leads its own process group and `process.kill(-pid)` reaps the group
— the CLI plus every server and helper it started. Process groups are a
POSIX concept: on Windows `process.kill(-pid)` throws EINVAL, the catch
falls through to `child.kill()`, and that terminates the CLI alone.
Each interrupted or completed turn orphans the MCP servers it started,
and they accumulate for as long as the app runs.

Windows tracks a parent/child tree instead, which is what `taskkill /T`
walks. killTree() picks the right one per platform and keeps the
existing best-effort behaviour: it never throws, and it falls back to
child.kill() if taskkill is unavailable.

Used by all three drivers (claude, codex, acp/core) in place of the
identical inline stop() each carried.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HgJeiantdRcZSBrc5sqqCp

@milind-soni milind-soni left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current main now centralizes this in server/procs.ts::killCliTree and all Claude, Codex, ACP, and Antigravity stop paths use it; Windows uses taskkill /T /F and POSIX uses the detached process group. This PR would duplicate that implementation, so I am leaving it unmerged as superseded.

@milind-soni
milind-soni merged commit 2855d0f into milind-soni:main Aug 14, 2026
@milind-soni

Copy link
Copy Markdown
Owner

Integrated through #86 on top of the current centralized process layer. The original commit is a parent of the integration merge, so authorship and history are preserved. I kept the process-tree behavior, added safe asynchronous taskkill failure handling and cleanup, and verified it with the grandchild regression on Windows, macOS, and Linux CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants