Skip to content

fix: kill entire CLI process tree on stop/forceStop (Windows) - #2073

Open
rinceyuan wants to merge 1 commit into
github:mainfrom
rinceyuan:fix/windows-process-tree-kill
Open

fix: kill entire CLI process tree on stop/forceStop (Windows)#2073
rinceyuan wants to merge 1 commit into
github:mainfrom
rinceyuan:fix/windows-process-tree-kill

Conversation

@rinceyuan

@rinceyuan rinceyuan commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Rust is not included - #2292 landed that fix, and this branch takes it as-is.

Fix the process-tree leak when stop() / forceStop() terminates the CLI. On Windows ChildProcess.kill() / Popen.terminate() only ends the root, leaving grandchildren orphaned. On POSIX a SIGTERM-resistant descendant survives after the root exits.

Closes #1804.

Spawn-time isolation

Put the CLI in its own process group so the whole tree can be signalled:

SDK Mechanism
Node.js detached: true
Python start_new_session=True
Go SysProcAttr{Setpgid: true}
Java / .NET not needed (tree-kill APIs handle it)

Teardown

Private helpers, no public API change, called from stop() and forceStop():

Platform Mechanism
Windows taskkill /T /F /PID
POSIX kill(-pid, signal) on the process group
Java ProcessHandle.descendants() collected before the root is signalled
.NET already uses Kill(entireProcessTree: true) - unchanged

stop() is graceful first and escalates: POSIX sends SIGTERM to the group, waits, then SIGKILLs the group. The escalation is unconditional because the root exiting says nothing about a descendant that ignored SIGTERM.

Windows always uses taskkill /T /F. It has no graceful signal (kill() is TerminateProcess regardless), and /T can only enumerate the tree while the root is alive, so a graceful root close would strand the descendants.

Failure handling

Every helper falls back to the single-process termination it replaced when the tree-wide path is unavailable or fails - missing pid, non-zero taskkill, ESRCH from killpg. Go propagates the error after the fallback also fails; Rust checks the exit status and reaps the root.

Go's killProcessTreeByPid uses the PID from the atomically swapped osProcess, not the mutex-guarded c.process.

Tests

nodejs/test/process_tree_kill.test.ts drives CopilotClient.stop() / forceStop() over a real spawned tree, so removing the tree termination fails them:

  1. stop() terminates descendants of the owned runtime
  2. forceStop() terminates descendants of the owned runtime
  3. stop() reaps a descendant that ignores SIGTERM (POSIX only)
  4. External-server connections are left alone

python/test_client.py gains TestKillProcessTree covering the group signal, the taskkill invocation and both fallbacks.

Validation

python -m pytest test_client.py -q                 122 passed   (ruff check/format clean)
npx vitest run test/process_tree_kill.test.ts      3 passed, 1 skipped (Windows)
  same file under WSL Ubuntu                       4 passed
npx tsc --noEmit / eslint                          clean
go vet ./... ; go test .                           clean / ok
  GOOS=linux and GOOS=darwin go build ./...        clean
mvnw test -pl sdk -Dtest=CopilotClientTest         41 passed   (spotless + checkstyle clean)
cargo +nightly fmt --check ; clippy -D warnings    clean
cargo test --all-features --lib                    226 passed

Not run locally: the internal/e2e Go package and the Rust e2e target, which need the replay harness and fail identically on a clean checkout.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CopilotClient.stop() leaks the CLI server's child process tree on Windows (orphaned node/copilot.exe per session)

5 participants