fix: kill entire CLI process tree on stop/forceStop (Windows) by rinceyuan · Pull Request #2073 · github/copilot-sdk

rinceyuan · 2026-07-24T03:33:54Z

Summary

Fix process tree leak on Windows when stop()/forceStop() terminates the CLI.

On Windows, ChildProcess.kill() (Node.js) and Popen.terminate()/kill() (Python) only terminate the immediate process via TerminateProcess(), leaving grandchildren orphaned. Each create_session()/stop() cycle leaks a full copilot process tree (~3-5 processes) that survives until idle timeout or OOM.

Changes

SDK	Fix
Node.js (`client.ts`)	New `killProcessTree()` helper: uses `taskkill /T /F /PID` on Windows, falls back to standard `.kill(signal)` on Unix
Python (`client.py`)	New `_kill_process_tree()` helper: uses `taskkill /T` on Windows, `os.killpg()` on Unix, with fallback to `proc.kill()`

Validation

TypeScript compiles clean (tsc --noEmit)
Python syntax validated (ast.parse)
Manual Windows test: spawned a 2-level node process tree, confirmed taskkill /T kills both parent and grandchild (see test output below)

parent=47632 child=27936 pre:parent alive pre:child alive taskkill OK post:parent DEAD(OK) post:child DEAD(OK)

Notes

Go, .NET, Rust, and Java SDKs may have similar issues but use different process management; those are left for follow-up.
The Unix path uses os.killpg() in Python (requires the child to be a process group leader); falls back gracefully if not applicable.

Closes #1804

rinceyuan · 2026-07-24T03:34:07Z

@microsoft-github-policy-service agree company=Microsoft

rinceyuan · 2026-07-24T03:34:09Z

@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.

SteveSandersonMS · 2026-07-28T10:26:27Z

The way this is implemented in the PR currently won’t work because:

Python calls killpg() without starting the CLI in a separate process group, which can kill the host and sibling processes.
Node’s process-group behavior is opt-in and defaults to false, so normal POSIX clients retain the leak.
Python does not check whether taskkill succeeded.
The change covers only Node and Python, despite the same lifecycle requirement applying across SDKs.
There are no real process-tree tests, and the existing Node lifecycle test fails.

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:

Windows: taskkill /T /F /PID <root>
POSIX:   signal the runtime’s private process group

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:

SDK	Spawn-time isolation	Teardown
Node	`detached: true`	`process.kill(-pid, signal)`
Python	`start_new_session=True`	`os.killpg(pid, signal)`
Go	`SysProcAttr.Setpgid = true`	`syscall.Kill(-pid, signal)`
Rust	`process_group(0)`	signal negative PID
Java	Use `ProcessHandle.descendants()`	snapshot descendants, kill them, then root
.NET	None needed	existing `Kill(entireProcessTree: true)`

Each SDK should call that helper from the process-termination section used by stop() and forceStop(). External-server and in-proc paths must not call it.

This is about the smallest useful implementation across all languages and OSes:

one private helper per SDK;
one POSIX spawn flag per applicable SDK;
no public options;
no Job Objects;
no crash-cleanup guarantee;
no redesign of graceful shutdown.

The tests can also be narrow: start a helper process that starts one long-lived child, then verify both disappear after stop() and forceStop() on Windows and POSIX. Also verify external and in-proc modes do not enter tree termination.

SteveSandersonMS · 2026-07-31T13:11:38Z

I'll move this back to draft, but please mark as ready to review if it later becomes ready.

rinceyuan · 2026-08-03T02:08:27Z

@SteveSandersonMS Reworked per your feedback. Single commit, all 6 SDKs:

Spawn-time isolation:

Node: \detached: true\ (always, not opt-in)
Python: \start_new_session=True\
Go: \SysProcAttr.Setpgid = true\
Rust: \process_group(0)\
Java/.NET: no spawn change needed

Teardown (private helpers, no public API):

Windows: \ askkill /T /F\
POSIX: \kill(-pid, SIGKILL)\ (process group signal)
Java: \ProcessHandle.descendants()\ snapshot + destroyForcibly
.NET: already uses \Kill(entireProcessTree: true)\ — unchanged

Removed the public \processGroup\ option. External-server and in-process (FFI) paths are not affected.

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

rinceyuan · 2026-08-03T02:22:56Z

@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

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.

SteveSandersonMS · 2026-08-03T13:00:11Z

+        if let Some(mut child) = self.inner.child.lock().take() {
+            force_kill_process_tree(&mut child);
+        }
        }


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.

SteveSandersonMS · 2026-08-03T13:00:11Z

+                ["taskkill", "/T", "/F", "/PID", str(pid)],
+                capture_output=True,
+                timeout=5,
+            )


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.

SteveSandersonMS · 2026-08-03T13:00:11Z

+    }
+    // POSIX: signal the process group (negative PID).
+    try {
+        process.kill(-pid, signal);


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).

SteveSandersonMS · 2026-08-03T13:00:11Z

 	// 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 {


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.

+        except Exception:
+            try:
+                proc.kill()
+            except Exception:


+        except (ProcessLookupError, PermissionError, OSError):
+            try:
+                proc.kill()
+            except Exception:


rinceyuan requested a review from a team as a code owner July 24, 2026 03:33

SteveSandersonMS added the waiting on response label Jul 28, 2026

SteveSandersonMS marked this pull request as draft July 31, 2026 13:11

SteveSandersonMS mentioned this pull request Jul 31, 2026

fix(python): terminate owned CLI process trees on stop #1829

Closed

rinceyuan force-pushed the fix/windows-process-tree-kill branch from 6fc9770 to 4364b34 Compare August 3, 2026 02:08

rinceyuan marked this pull request as ready for review August 3, 2026 02:08

rinceyuan force-pushed the fix/windows-process-tree-kill branch from 4364b34 to 6ffcb43 Compare August 3, 2026 02:10

SteveSandersonMS requested changes Aug 3, 2026

View reviewed changes

SteveSandersonMS marked this pull request as draft August 3, 2026 13:00

github-advanced-security AI found potential problems Aug 3, 2026

View reviewed changes

Comment thread python/copilot/client.py

except Exception:

try:

proc.kill()

except Exception:

Comment thread python/copilot/client.py

except (ProcessLookupError, PermissionError, OSError):

try:

proc.kill()

except Exception:

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

rinceyuan commented Jul 24, 2026

Uh oh!

rinceyuan commented Jul 24, 2026

Uh oh!

rinceyuan commented Jul 24, 2026

Uh oh!

SteveSandersonMS commented Jul 28, 2026

Uh oh!

SteveSandersonMS commented Jul 31, 2026

Uh oh!

rinceyuan commented Aug 3, 2026

Uh oh!

rinceyuan commented Aug 3, 2026

Uh oh!

SteveSandersonMS left a comment

Uh oh!

SteveSandersonMS Aug 3, 2026

Uh oh!

SteveSandersonMS Aug 3, 2026

Uh oh!

SteveSandersonMS Aug 3, 2026

Uh oh!

SteveSandersonMS Aug 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

rinceyuan commented Jul 24, 2026

Summary

Changes

Validation

Notes

Uh oh!

rinceyuan commented Jul 24, 2026

Uh oh!

rinceyuan commented Jul 24, 2026

Uh oh!

SteveSandersonMS commented Jul 28, 2026

Uh oh!

SteveSandersonMS commented Jul 31, 2026

Uh oh!

rinceyuan commented Aug 3, 2026

Uh oh!

rinceyuan commented Aug 3, 2026

Uh oh!

SteveSandersonMS left a comment

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants