fix: kill entire CLI process tree on stop/forceStop (Windows) - #2073
fix: kill entire CLI process tree on stop/forceStop (Windows)#2073rinceyuan wants to merge 1 commit into
Conversation
|
@microsoft-github-policy-service agree company=Microsoft |
|
@stephentoub This fixes the Windows process tree leak reported in #1804. Affects both Node.js and Python SDKs — each stop()/forceStop() cycle was orphaning the CLI's child processes. The fix uses \ askkill /T\ on Windows. Manually verified on Windows 11. Happy to add the Go/.NET fixes in a follow-up if desired. |
|
The way this is implemented in the PR currently won’t work because:
There is a small, coherent cross-language change we could accept: add one private “terminate owned runtime tree” operation per SDK, called from the existing owned-process termination point. Its behavior should be: POSIX also needs one small spawn-time change to place the runtime in its own process group/session. Otherwise group termination could kill the host. No public API is needed. Per language, this is approximately:
Each SDK should call that helper from the process-termination section used by This is about the smallest useful implementation across all languages and OSes:
The tests can also be narrow: start a helper process that starts one long-lived child, then verify both disappear after |
|
I'll move this back to draft, but please mark as ready to review if it later becomes ready. |
6fc9770 to
4364b34
Compare
|
@SteveSandersonMS Reworked per your feedback. Single commit, all 6 SDKs: Spawn-time isolation:
Teardown (private helpers, no public API):
Removed the public \processGroup\ option. External-server and in-process (FFI) paths are not affected. |
4364b34 to
6ffcb43
Compare
|
@SteveSandersonMS Ready for re-review. All 5 points from your feedback are addressed — private helpers in all 6 SDKs, POSIX spawn isolation, no public API, guarded by isExternalServer. Also fixed a Rust compile issue (replaced libc::kill with kill command to avoid adding a new dependency). |
SteveSandersonMS
left a comment
SteveSandersonMS
left a comment
There was a problem hiding this comment.
The cross-language direction is right, but this revision is not ready to merge. Rust does not compile, the existing Node lifecycle test fails, tree-kill failures can still be reported as success, and there is no process-tree coverage. I also manually exercised the public Node API: stop()/forceStop() remove a normal descendant, but stop() leaves a descendant that ignores SIGTERM. Please keep the private cross-language design, make final teardown definitive and error-aware, preserve Go's concurrency-safe process ownership, add stop()/forceStop() process-tree tests on Windows and POSIX (plus external/in-process negative coverage), and update the stale PR description.
| if let Some(mut child) = self.inner.child.lock().take() { | ||
| force_kill_process_tree(&mut child); | ||
| } | ||
| } |
There was a problem hiding this comment.
This extra closing brace makes the Rust SDK fail to compile (unexpected closing delimiter). Please fix this and run the Rust build before marking ready again.
| ["taskkill", "/T", "/F", "/PID", str(pid)], | ||
| capture_output=True, | ||
| timeout=5, | ||
| ) |
There was a problem hiding this comment.
subprocess.run() does not raise when taskkill exits nonzero, so this path can silently leave the whole tree alive and skip the fallback. Check the return status (for example with check=True) and surface or explicitly handle failure rather than returning success-shaped behavior.
| } | ||
| // POSIX: signal the process group (negative PID). | ||
| try { | ||
| process.kill(-pid, signal); |
There was a problem hiding this comment.
The default stop() path sends SIGTERM and waits only for the root. I manually tested a runtime descendant that ignores SIGTERM: the root exited, stop() completed, and the descendant remained alive. Since runtime.shutdown has already completed, final owned-tree teardown should be definitive (or follow SIGTERM with an unconditional group SIGKILL check).
| // This unblocks any I/O Start is doing (connect, version check). | ||
| if p := c.osProcess.Swap(nil); p != nil { | ||
| p.Kill() | ||
| if c.process != nil { |
There was a problem hiding this comment.
This newly reads c.process outside startStopMux, while ForceStop deliberately uses the atomically swapped osProcess to interrupt a concurrent Start. That introduces a race and may target a different process than p. Make the tree-kill helper operate from the atomically owned *os.Process/PID instead of consulting c.process here.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| except Exception: | ||
| try: | ||
| proc.kill() | ||
| except Exception: |
| except (ProcessLookupError, PermissionError, OSError): | ||
| try: | ||
| proc.kill() | ||
| except Exception: |
6ffcb43 to
0b74510
Compare
|
Pushed fixes for all 4 inline comments:
Still TODO: process-tree tests. Working on those next. |
0b74510 to
747a755
Compare
|
@SteveSandersonMS All feedback addressed + process-tree tests added:
Ready for re-review. |
|
@SteveSandersonMS Gentle re-review ping: this is now out of draft, all requested fixes and process-tree coverage are in the single commit, and I rechecked that it still merges cleanly with current main. No further changes since the last summary. |
747a755 to
2865649
Compare
|
Rebased onto current One real fix while re-validating: Verified on the rebased branch:
|
There was a problem hiding this comment.
Pull request overview
This PR updates SDK process teardown to terminate the full Copilot CLI process tree, addressing orphaned descendants during stop() and forceStop().
Changes:
- Isolates spawned CLI runtimes into POSIX process groups.
- Adds platform-specific tree termination across Node.js, Python, Go, Rust, and Java.
- Adds Node.js process-tree termination tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
nodejs/src/client.ts |
Adds process-group spawning and tree termination with signal escalation. |
nodejs/test/process_tree_kill.test.ts |
Adds POSIX, Windows, and external-server process termination tests. |
python/copilot/client.py |
Adds session isolation and platform-specific process-tree cleanup. |
go/client.go |
Routes normal and forced shutdown through process-tree termination. |
go/process_other.go |
Adds POSIX process groups and group signaling. |
go/process_windows.go |
Adds Windows taskkill /T /F termination. |
rust/src/lib.rs |
Adds process groups and tree-kill helpers for shutdown and drop. |
java/sdk/src/main/java/com/github/copilot/CopilotClient.java |
Adds descendant enumeration and forcible tree termination. |
Suppressed comments (2)
rust/src/lib.rs:2666
- Both platform branches in this synchronous helper discard the tree-kill command result and return immediately. If the utility fails,
force_stop()andDropdo nothing and never reach the existingstart_kill()fallback. Only return after a successful status; otherwise invoke the child-handle fallback and log the original failure.
fn force_kill_process_tree(child: &mut Child) {
rust/src/lib.rs:2659
- The
taskkillresult is discarded and this branch returnsOk(())even when the command cannot start or exits nonzero, so the documentedchild.kill()fallback is never used andstop()reports success while the CLI may remain alive. Validatestatus.success(), fall back on failure, and await the root child so it is reaped.
.args(["/T", "/F", "/PID", &pid.to_string()])
.stdout(std::process::Stdio::null())
.stderr(std::process::Stdio::null())
.status();
return Ok(());
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const pid = child.pid; | ||
| if (pid == null) { | ||
| return false; | ||
| } |
| if (!(await waitForChildExit(child, RUNTIME_SHUTDOWN_TIMEOUT_MS))) { | ||
| errors.push( | ||
| new Error( | ||
| `Timed out waiting for CLI process to exit after kill: ${RUNTIME_SHUTDOWN_TIMEOUT_MS}ms` | ||
| ) | ||
| ); | ||
| // SIGTERM-resistant descendants may survive; escalate to SIGKILL. | ||
| killProcessTree(child, "SIGKILL"); |
| // Kill process group (same as SDK does) | ||
| try { | ||
| process.kill(-parentPid, "SIGKILL"); | ||
| } catch { | ||
| parent.kill("SIGKILL"); | ||
| } |
| describe("CopilotClient external/in-process modes", () => { | ||
| it("should not attempt tree termination for external-server connections", async () => { |
| } | ||
|
|
||
| process.destroy(); | ||
| killProcessTree(process); |
| if p := c.osProcess.Swap(nil); p != nil { | ||
| if err := p.Kill(); err != nil { | ||
| return fmt.Errorf("failed to kill CLI process: %w", err) | ||
| } | ||
| killProcessTreeByPid(p.Pid) |
| .args(["-9", &format!("-{}", pid)]) | ||
| .stdout(std::process::Stdio::null()) | ||
| .stderr(std::process::Stdio::null()) | ||
| .status(); | ||
| return Ok(()); |
| is_running = poll is None or poll() is None | ||
| if is_running: | ||
| self._cli_process.terminate() | ||
| _kill_process_tree(self._cli_process) |
2865649 to
d7e353f
Compare
|
Follow-up: I got the sandbox unblocked and can now run the Node and Python suites locally, which surfaced a real defect in the Python change.
Verified on the rebased branch: |
Add a private kill-process-tree helper to each SDK, called from the existing owned-process termination points in stop() and forceStop(). Spawn-time isolation (POSIX): - Node.js: detached: true - Python: start_new_session=True - Go: SysProcAttr.Setpgid = true - Rust: process_group(0) Teardown: - Windows (all): taskkill /T /F /PID - Node.js/Python/Go (POSIX): kill(-pid, SIGKILL) — process group signal - Rust (POSIX): libc::kill(-pid, SIGKILL) - Java: ProcessHandle.descendants() snapshot + destroyForcibly each - .NET: already uses Kill(entireProcessTree: true) — no change needed No public API changes. External-server and in-process (FFI) paths are not affected. Closes github#1804
d7e353f to
da078a6
Compare
|
Thanks - all eight points were real. Fixed, and I now have Linux (WSL) and the Node/Python/Go toolchains available locally, so these are verified rather than reasoned about. Node - Windows semantics - I had briefly made the graceful pass use Node tests - rewritten. They now drive
The in-process claim was wrong, so the header and the PR body now describe the external-server case only. Java - Go - Rust - split out Python - the graceful/force split you asked about, plus Still not run here: the |
Summary
Fix the process-tree leak when
stop()/forceStop()terminates the CLI. On WindowsChildProcess.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:
detached: truestart_new_session=TrueSysProcAttr{Setpgid: true}process_group(0)Teardown
Private helpers, no public API change, called from
stop()andforceStop():taskkill /T /F /PIDkill(-pid, signal)on the process groupProcessHandle.descendants()collected before the root is signalledKill(entireProcessTree: true)- unchangedstop()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()isTerminateProcessregardless), and/Tcan 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,ESRCHfromkillpg. Go propagates the error after the fallback also fails; Rust checks the exit status and reaps the root.Go's
killProcessTreeByPiduses the PID from the atomically swappedosProcess, not the mutex-guardedc.process.Tests
nodejs/test/process_tree_kill.test.tsdrivesCopilotClient.stop()/forceStop()over a real spawned tree, so removing the tree termination fails them:stop()terminates descendants of the owned runtimeforceStop()terminates descendants of the owned runtimestop()reaps a descendant that ignores SIGTERM (POSIX only)python/test_client.pygainsTestKillProcessTreecovering the group signal, thetaskkillinvocation and both fallbacks.Validation
Not run locally: the
internal/e2eGo package and the Ruste2etarget, which need the replay harness and fail identically on a clean checkout.