Skip to content

Commit 0b74510

Browse files
author
j-zhangyiyuan
committed
fix: terminate owned runtime process tree on stop/forceStop
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 #1804
1 parent 72de60f commit 0b74510

7 files changed

Lines changed: 215 additions & 35 deletions

File tree

go/client.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -634,7 +634,7 @@ func (c *Client) ForceStop() {
634634
// Kill the process without waiting for startStopMux, which Start may hold.
635635
// This unblocks any I/O Start is doing (connect, version check).
636636
if p := c.osProcess.Swap(nil); p != nil {
637-
p.Kill()
637+
killProcessTreeByPid(p.Pid)
638638
}
639639

640640
// Clear sessions immediately without trying to destroy them
@@ -2189,9 +2189,7 @@ func (c *Client) killProcess() error {
21892189
c.ffiHost = nil
21902190
}
21912191
if p := c.osProcess.Swap(nil); p != nil {
2192-
if err := p.Kill(); err != nil {
2193-
return fmt.Errorf("failed to kill CLI process: %w", err)
2194-
}
2192+
killProcessTreeByPid(p.Pid)
21952193
}
21962194
c.process = nil
21972195
return nil

go/process_other.go

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,18 @@
22

33
package copilot
44

5-
import "os/exec"
5+
import (
6+
"os/exec"
7+
"syscall"
8+
)
69

7-
// configureProcAttr configures platform-specific process attributes.
8-
// On non-Windows platforms, this is a no-op.
10+
// configureProcAttr places the runtime in its own process group so
11+
// killProcessTreeByPid can signal all descendants atomically.
912
func configureProcAttr(cmd *exec.Cmd) {
10-
// No special configuration needed on non-Windows platforms
13+
cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
14+
}
15+
16+
// killProcessTreeByPid signals the process group (negative PID) with SIGKILL.
17+
func killProcessTreeByPid(pid int) {
18+
_ = syscall.Kill(-pid, syscall.SIGKILL)
1119
}

go/process_windows.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,19 @@
33
package copilot
44

55
import (
6+
"fmt"
67
"os/exec"
78
"syscall"
89
)
910

10-
// configureProcAttr configures platform-specific process attributes.
11-
// On Windows, this hides the console window to avoid distracting users in GUI apps.
11+
// configureProcAttr hides the console window on Windows.
1212
func configureProcAttr(cmd *exec.Cmd) {
1313
cmd.SysProcAttr = &syscall.SysProcAttr{
1414
HideWindow: true,
1515
}
1616
}
17+
18+
// killProcessTreeByPid terminates the entire process tree via taskkill /T /F.
19+
func killProcessTreeByPid(pid int) {
20+
_ = exec.Command("taskkill", "/T", "/F", "/PID", fmt.Sprintf("%d", pid)).Run()
21+
}

java/src/main/java/com/github/copilot/CopilotClient.java

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -480,19 +480,19 @@ private static void cleanupCliProcess(Process process, boolean forceImmediately)
480480
// will never come just wastes time, so terminate the child
481481
// immediately and only wait to reap it.
482482
if (forceImmediately) {
483-
process.destroyForcibly();
483+
killProcessTree(process);
484484
if (!process.waitFor(FORCE_KILL_TIMEOUT_SECONDS, TimeUnit.SECONDS)) {
485485
LOG.fine("Process did not terminate within force kill timeout");
486486
}
487487
return;
488488
}
489489

490-
process.destroy();
490+
killProcessTree(process);
491491
if (process.waitFor(FORCE_KILL_TIMEOUT_SECONDS, TimeUnit.SECONDS)) {
492492
return;
493493
}
494494

495-
process.destroyForcibly();
495+
killProcessTree(process);
496496
if (!process.waitFor(FORCE_KILL_TIMEOUT_SECONDS, TimeUnit.SECONDS)) {
497497
LOG.fine("Process did not terminate within force kill timeout");
498498
}
@@ -505,6 +505,22 @@ private static void cleanupCliProcess(Process process, boolean forceImmediately)
505505
}
506506
}
507507

508+
/**
509+
* Terminate the runtime's process tree: snapshot all descendants, destroy
510+
* them, then destroy the root. Uses {@link ProcessHandle#descendants()}
511+
* which works cross-platform (Windows, Linux, macOS).
512+
*/
513+
private static void killProcessTree(Process process) {
514+
try {
515+
process.toHandle().descendants().forEach(ph -> {
516+
try { ph.destroyForcibly(); } catch (Exception ignored) {}
517+
});
518+
} catch (Exception e) {
519+
LOG.log(Level.FINE, "Error killing process descendants", e);
520+
}
521+
process.destroyForcibly();
522+
}
523+
508524
/**
509525
* Creates a new Copilot session with the specified configuration.
510526
* <p>

nodejs/src/client.ts

Lines changed: 57 additions & 8 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";
@@ -153,6 +153,40 @@ async function waitForChildExit(child: ChildProcess, timeoutMs: number): Promise
153153
});
154154
}
155155

156+
/**
157+
* Terminate the runtime's process tree.
158+
*
159+
* - Windows: `taskkill /T /F` kills the entire tree rooted at `pid`.
160+
* - POSIX: the runtime is spawned in its own process group (`detached: true`),
161+
* so `kill(-pid)` signals every process in that group.
162+
*
163+
* Falls back to `child.kill(signal)` if the tree-wide signal fails (e.g. the
164+
* process already exited).
165+
*
166+
* @see https://github.com/github/copilot-sdk/issues/1804
167+
*/
168+
function killProcessTree(child: ChildProcess, signal: NodeJS.Signals = "SIGTERM"): boolean {
169+
const pid = child.pid;
170+
if (pid == null) {
171+
return false;
172+
}
173+
if (process.platform === "win32") {
174+
try {
175+
execSync(`taskkill /T /F /PID ${pid}`, { stdio: "ignore", timeout: 5000 });
176+
return true;
177+
} catch {
178+
return child.kill(signal);
179+
}
180+
}
181+
// POSIX: signal the process group (negative PID).
182+
try {
183+
process.kill(-pid, signal);
184+
return true;
185+
} catch {
186+
return child.kill(signal);
187+
}
188+
}
189+
156190
/**
157191
* Convert tool parameters to JSON schema format for sending to CLI
158192
*/
@@ -1082,13 +1116,17 @@ export class CopilotClient {
10821116
this.cliProcess = null;
10831117
try {
10841118
if (child.exitCode == null && child.signalCode == null) {
1085-
child.kill();
1119+
killProcessTree(child, "SIGTERM");
10861120
if (!(await waitForChildExit(child, RUNTIME_SHUTDOWN_TIMEOUT_MS))) {
1087-
errors.push(
1088-
new Error(
1089-
`Timed out waiting for CLI process to exit after kill: ${RUNTIME_SHUTDOWN_TIMEOUT_MS}ms`
1090-
)
1091-
);
1121+
// SIGTERM-resistant descendants may survive; escalate to SIGKILL.
1122+
killProcessTree(child, "SIGKILL");
1123+
if (!(await waitForChildExit(child, RUNTIME_SHUTDOWN_TIMEOUT_MS))) {
1124+
errors.push(
1125+
new Error(
1126+
`Timed out waiting for CLI process to exit after kill: ${RUNTIME_SHUTDOWN_TIMEOUT_MS}ms`
1127+
)
1128+
);
1129+
}
10921130
}
10931131
}
10941132
} catch (error) {
@@ -1209,7 +1247,7 @@ export class CopilotClient {
12091247
// Force kill CLI process (only if we spawned it)
12101248
if (this.cliProcess && !this.isExternalServer) {
12111249
try {
1212-
this.cliProcess.kill("SIGKILL");
1250+
killProcessTree(this.cliProcess, "SIGKILL");
12131251
} catch {
12141252
// Ignore errors
12151253
}
@@ -2468,22 +2506,33 @@ export class CopilotClient {
24682506
: ["ignore", "pipe", "pipe"];
24692507

24702508
// For .js files, spawn node explicitly; for executables, spawn directly
2509+
// Place the runtime in its own process group so killProcessTree()
2510+
// can signal all descendants atomically. On Windows detached has
2511+
// no effect — taskkill /T handles tree termination instead.
2512+
const detached = process.platform !== "win32";
24712513
const isJsFile = this.resolvedCliPath.endsWith(".js");
24722514
if (isJsFile) {
24732515
this.cliProcess = spawn(getNodeExecPath(), [this.resolvedCliPath, ...args], {
24742516
stdio: stdioConfig,
24752517
cwd: this.options.workingDirectory,
24762518
env: envWithoutNodeDebug,
24772519
windowsHide: true,
2520+
detached,
24782521
});
24792522
} else {
24802523
this.cliProcess = spawn(this.resolvedCliPath, args, {
24812524
stdio: stdioConfig,
24822525
cwd: this.options.workingDirectory,
24832526
env: envWithoutNodeDebug,
24842527
windowsHide: true,
2528+
detached,
24852529
});
24862530
}
2531+
// Prevent the detached child from keeping the parent's event loop
2532+
// alive when the embedder exits without calling stop().
2533+
if (detached) {
2534+
this.cliProcess.unref();
2535+
}
24872536

24882537
let stdout = "";
24892538
let resolved = false;

python/copilot/client.py

Lines changed: 45 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1200,6 +1200,44 @@ def _session_lifecycle_event_from_dict(data: dict) -> SessionLifecycleEvent:
12001200
_CLI_PROCESS_EXIT_TIMEOUT_SECONDS = 5
12011201

12021202

1203+
def _kill_process_tree(proc: subprocess.Popen[Any]) -> None:
1204+
"""Terminate the runtime's process tree.
1205+
1206+
Windows: ``taskkill /T /F`` kills the entire tree rooted at *pid*.
1207+
POSIX: the runtime is spawned with ``start_new_session=True``, so
1208+
``os.killpg(pid, signal)`` signals every process in that group.
1209+
1210+
Falls back to ``proc.kill()`` if the tree-wide signal fails.
1211+
1212+
See: https://github.com/github/copilot-sdk/issues/1804
1213+
"""
1214+
pid = proc.pid
1215+
if pid is None:
1216+
return
1217+
if sys.platform == "win32":
1218+
try:
1219+
result = subprocess.run(
1220+
["taskkill", "/T", "/F", "/PID", str(pid)],
1221+
capture_output=True,
1222+
timeout=5,
1223+
)
1224+
if result.returncode != 0:
1225+
proc.kill()
1226+
except Exception:
1227+
try:
1228+
proc.kill()
1229+
except Exception:
1230+
pass
1231+
else:
1232+
try:
1233+
os.killpg(pid, 9) # SIGKILL to the runtime's process group
1234+
except (ProcessLookupError, PermissionError, OSError):
1235+
try:
1236+
proc.kill()
1237+
except Exception:
1238+
pass
1239+
1240+
12031241
def _get_or_download_cli(*, include_runtime_lib: bool = False) -> str | None:
12041242
"""Get the cached CLI binary, downloading if necessary.
12051243
@@ -1910,14 +1948,14 @@ async def stop(self) -> None:
19101948
poll = getattr(self._cli_process, "poll", None)
19111949
is_running = poll is None or poll() is None
19121950
if is_running:
1913-
self._cli_process.terminate()
1951+
_kill_process_tree(self._cli_process)
19141952
try:
19151953
await asyncio.to_thread(
19161954
self._cli_process.wait,
19171955
timeout=_CLI_PROCESS_EXIT_TIMEOUT_SECONDS,
19181956
)
19191957
except subprocess.TimeoutExpired:
1920-
self._cli_process.kill()
1958+
_kill_process_tree(self._cli_process)
19211959
try:
19221960
await asyncio.to_thread(
19231961
self._cli_process.wait,
@@ -1976,7 +2014,7 @@ async def force_stop(self) -> None:
19762014
if self._process is not None and self._process is not self._cli_process:
19772015
self._process.terminate()
19782016
if self._cli_process is not None:
1979-
self._cli_process.kill()
2017+
_kill_process_tree(self._cli_process)
19802018
self._process = None
19812019
self._cli_process = None
19822020
except Exception:
@@ -4027,6 +4065,9 @@ async def _start_cli_server(self) -> None:
40274065
cwd=cwd,
40284066
env=env,
40294067
creationflags=creationflags,
4068+
# Place the runtime in its own process group so
4069+
# _kill_process_tree() can signal all descendants.
4070+
start_new_session=(sys.platform != "win32"),
40304071
)
40314072
self._cli_process = self._process
40324073
else:
@@ -4040,6 +4081,7 @@ async def _start_cli_server(self) -> None:
40404081
cwd=cwd,
40414082
env=env,
40424083
creationflags=creationflags,
4084+
start_new_session=(sys.platform != "win32"),
40434085
)
40444086
self._cli_process = self._process
40454087
log_timing(

0 commit comments

Comments
 (0)