Skip to content

Commit 0396fc4

Browse files
author
j-zhangyiyuan
committed
fix: kill entire CLI process tree on stop/forceStop (Windows)
On Windows, ChildProcess.kill() / Popen.terminate() only terminates the immediate process via TerminateProcess(), leaving grandchildren (copilot workers, MCP servers, etc.) orphaned. Each session cycle leaks a full process tree that accumulates until OOM. Fix: use \ askkill /T /F /PID\ on Windows to terminate the entire tree. On Unix, use os.killpg() in Python for process group cleanup. Node.js (client.ts): - Add killProcessTree() helper wrapping taskkill /T on Windows - Replace child.kill() in stop() and forceStop() with killProcessTree() Python (client.py): - Add _kill_process_tree() helper with taskkill /T (Windows) and os.killpg() (Unix) strategies - Replace self._cli_process.terminate()/kill() in stop() and force_stop() with _kill_process_tree() Tested: spawned a 2-level process tree on Windows, confirmed both parent and child are terminated after taskkill /T. Closes #1804
1 parent 949de90 commit 0396fc4

2 files changed

Lines changed: 77 additions & 6 deletions

File tree

nodejs/src/client.ts

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
* @module client
1212
*/
1313

14-
import { spawn, type ChildProcess } from "node:child_process";
14+
import { spawn, execSync, type ChildProcess } from "node:child_process";
1515
import { randomUUID } from "node:crypto";
1616
import { existsSync } from "node:fs";
1717
import { createRequire } from "node:module";
@@ -152,6 +152,36 @@ async function waitForChildExit(child: ChildProcess, timeoutMs: number): Promise
152152
});
153153
}
154154

155+
/**
156+
* Kills a child process and its entire process tree.
157+
*
158+
* On Windows, `ChildProcess.kill()` only terminates the immediate process via
159+
* `TerminateProcess()`, leaving grandchildren (e.g. copilot workers, MCP servers)
160+
* orphaned. This helper uses `taskkill /T` to terminate the full tree.
161+
*
162+
* On Unix, the standard `.kill(signal)` is sufficient because the CLI is spawned
163+
* directly (not via a shell wrapper) and handles signals properly.
164+
*
165+
* @see https://github.com/github/copilot-sdk/issues/1804
166+
*/
167+
function killProcessTree(child: ChildProcess, signal: NodeJS.Signals = "SIGTERM"): boolean {
168+
const pid = child.pid;
169+
if (pid == null) {
170+
return false;
171+
}
172+
if (process.platform === "win32") {
173+
try {
174+
// /T = tree kill (all child processes), /F = force
175+
execSync(`taskkill /T /F /PID ${pid}`, { stdio: "ignore", timeout: 5000 });
176+
return true;
177+
} catch {
178+
// taskkill may fail if process already exited; fall back to standard kill
179+
return child.kill(signal);
180+
}
181+
}
182+
return child.kill(signal);
183+
}
184+
155185
/**
156186
* Convert tool parameters to JSON schema format for sending to CLI
157187
*/
@@ -1086,7 +1116,7 @@ export class CopilotClient {
10861116
this.cliProcess = null;
10871117
try {
10881118
if (child.exitCode == null && child.signalCode == null) {
1089-
child.kill();
1119+
killProcessTree(child);
10901120
if (!(await waitForChildExit(child, RUNTIME_SHUTDOWN_TIMEOUT_MS))) {
10911121
errors.push(
10921122
new Error(
@@ -1213,7 +1243,7 @@ export class CopilotClient {
12131243
// Force kill CLI process (only if we spawned it)
12141244
if (this.cliProcess && !this.isExternalServer) {
12151245
try {
1216-
this.cliProcess.kill("SIGKILL");
1246+
killProcessTree(this.cliProcess, "SIGKILL");
12171247
} catch {
12181248
// Ignore errors
12191249
}

python/copilot/client.py

Lines changed: 44 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1182,6 +1182,47 @@ def _session_lifecycle_event_from_dict(data: dict) -> SessionLifecycleEvent:
11821182
_CLI_PROCESS_EXIT_TIMEOUT_SECONDS = 5
11831183

11841184

1185+
def _kill_process_tree(proc: subprocess.Popen[Any]) -> None:
1186+
"""Kill a process and its entire process tree.
1187+
1188+
On Windows, ``Popen.terminate()``/``kill()`` only terminates the immediate
1189+
process via ``TerminateProcess()``, leaving grandchildren (copilot workers,
1190+
MCP servers) orphaned. This helper uses ``taskkill /T`` to terminate the
1191+
full tree.
1192+
1193+
On Unix, ``os.killpg()`` sends the signal to the entire process group when
1194+
the child was started with a new session (``start_new_session=True``).
1195+
Falls back to ``proc.kill()`` if the process group approach is unavailable.
1196+
1197+
See: https://github.com/github/copilot-sdk/issues/1804
1198+
"""
1199+
pid = proc.pid
1200+
if pid is None:
1201+
return
1202+
if sys.platform == "win32":
1203+
try:
1204+
subprocess.run(
1205+
["taskkill", "/T", "/F", "/PID", str(pid)],
1206+
capture_output=True,
1207+
timeout=5,
1208+
)
1209+
except Exception:
1210+
# taskkill may fail if process already exited; fall back
1211+
try:
1212+
proc.kill()
1213+
except Exception:
1214+
pass
1215+
else:
1216+
try:
1217+
os.killpg(os.getpgid(pid), 9) # SIGKILL to process group
1218+
except (ProcessLookupError, PermissionError, OSError):
1219+
# Process group kill failed (not a group leader, already dead, etc.)
1220+
try:
1221+
proc.kill()
1222+
except Exception:
1223+
pass
1224+
1225+
11851226
def _get_or_download_cli(*, include_runtime_lib: bool = False) -> str | None:
11861227
"""Get the cached CLI binary, downloading if necessary.
11871228
@@ -1892,14 +1933,14 @@ async def stop(self) -> None:
18921933
poll = getattr(self._cli_process, "poll", None)
18931934
is_running = poll is None or poll() is None
18941935
if is_running:
1895-
self._cli_process.terminate()
1936+
_kill_process_tree(self._cli_process)
18961937
try:
18971938
await asyncio.to_thread(
18981939
self._cli_process.wait,
18991940
timeout=_CLI_PROCESS_EXIT_TIMEOUT_SECONDS,
19001941
)
19011942
except subprocess.TimeoutExpired:
1902-
self._cli_process.kill()
1943+
_kill_process_tree(self._cli_process)
19031944
try:
19041945
await asyncio.to_thread(
19051946
self._cli_process.wait,
@@ -1958,7 +1999,7 @@ async def force_stop(self) -> None:
19581999
if self._process is not None and self._process is not self._cli_process:
19592000
self._process.terminate()
19602001
if self._cli_process is not None:
1961-
self._cli_process.kill()
2002+
_kill_process_tree(self._cli_process)
19622003
self._process = None
19632004
self._cli_process = None
19642005
except Exception:

0 commit comments

Comments
 (0)