Skip to content

Commit 4364b34

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 4364b34

7 files changed

Lines changed: 217 additions & 29 deletions

File tree

go/client.go

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -634,7 +634,11 @@ 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+
if c.process != nil {
638+
killProcessTree(c.process)
639+
} else {
640+
p.Kill()
641+
}
638642
}
639643

640644
// Clear sessions immediately without trying to destroy them
@@ -2188,10 +2192,9 @@ func (c *Client) killProcess() error {
21882192
c.ffiHost.Dispose()
21892193
c.ffiHost = nil
21902194
}
2191-
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-
}
2195+
if c.process != nil {
2196+
killProcessTree(c.process)
2197+
c.osProcess.Store(nil)
21952198
}
21962199
c.process = nil
21972200
return nil

go/process_other.go

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,25 @@
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+
// killProcessTree 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+
// killProcessTree signals the runtime's process group (negative PID).
17+
// Falls back to killing the immediate process if the group signal fails.
18+
func killProcessTree(cmd *exec.Cmd) {
19+
if cmd.Process == nil {
20+
return
21+
}
22+
// Signal the entire process group.
23+
if err := syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL); err != nil {
24+
_ = cmd.Process.Kill()
25+
}
1126
}

go/process_windows.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
package copilot
44

55
import (
6+
"fmt"
67
"os/exec"
78
"syscall"
89
)
@@ -14,3 +15,15 @@ func configureProcAttr(cmd *exec.Cmd) {
1415
HideWindow: true,
1516
}
1617
}
18+
19+
// killProcessTree terminates the runtime's entire process tree using
20+
// taskkill /T /F. Falls back to killing the immediate process.
21+
func killProcessTree(cmd *exec.Cmd) {
22+
if cmd.Process == nil {
23+
return
24+
}
25+
kill := exec.Command("taskkill", "/T", "/F", "/PID", fmt.Sprintf("%d", cmd.Process.Pid))
26+
if err := kill.Run(); err != nil {
27+
_ = cmd.Process.Kill()
28+
}
29+
}

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: 48 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";
@@ -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,7 +1116,7 @@ export class CopilotClient {
10821116
this.cliProcess = null;
10831117
try {
10841118
if (child.exitCode == null && child.signalCode == null) {
1085-
child.kill();
1119+
killProcessTree(child);
10861120
if (!(await waitForChildExit(child, RUNTIME_SHUTDOWN_TIMEOUT_MS))) {
10871121
errors.push(
10881122
new Error(
@@ -1209,7 +1243,7 @@ export class CopilotClient {
12091243
// Force kill CLI process (only if we spawned it)
12101244
if (this.cliProcess && !this.isExternalServer) {
12111245
try {
1212-
this.cliProcess.kill("SIGKILL");
1246+
killProcessTree(this.cliProcess, "SIGKILL");
12131247
} catch {
12141248
// Ignore errors
12151249
}
@@ -2468,22 +2502,33 @@ export class CopilotClient {
24682502
: ["ignore", "pipe", "pipe"];
24692503

24702504
// For .js files, spawn node explicitly; for executables, spawn directly
2505+
// Place the runtime in its own process group so killProcessTree()
2506+
// can signal all descendants atomically. On Windows detached has
2507+
// no effect — taskkill /T handles tree termination instead.
2508+
const detached = process.platform !== "win32";
24712509
const isJsFile = this.resolvedCliPath.endsWith(".js");
24722510
if (isJsFile) {
24732511
this.cliProcess = spawn(getNodeExecPath(), [this.resolvedCliPath, ...args], {
24742512
stdio: stdioConfig,
24752513
cwd: this.options.workingDirectory,
24762514
env: envWithoutNodeDebug,
24772515
windowsHide: true,
2516+
detached,
24782517
});
24792518
} else {
24802519
this.cliProcess = spawn(this.resolvedCliPath, args, {
24812520
stdio: stdioConfig,
24822521
cwd: this.options.workingDirectory,
24832522
env: envWithoutNodeDebug,
24842523
windowsHide: true,
2524+
detached,
24852525
});
24862526
}
2527+
// Prevent the detached child from keeping the parent's event loop
2528+
// alive when the embedder exits without calling stop().
2529+
if (detached) {
2530+
this.cliProcess.unref();
2531+
}
24872532

24882533
let stdout = "";
24892534
let resolved = false;

python/copilot/client.py

Lines changed: 43 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1200,6 +1200,42 @@ 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+
subprocess.run(
1220+
["taskkill", "/T", "/F", "/PID", str(pid)],
1221+
capture_output=True,
1222+
timeout=5,
1223+
)
1224+
except Exception:
1225+
try:
1226+
proc.kill()
1227+
except Exception:
1228+
pass
1229+
else:
1230+
try:
1231+
os.killpg(pid, 9) # SIGKILL to the runtime's process group
1232+
except (ProcessLookupError, PermissionError, OSError):
1233+
try:
1234+
proc.kill()
1235+
except Exception:
1236+
pass
1237+
1238+
12031239
def _get_or_download_cli(*, include_runtime_lib: bool = False) -> str | None:
12041240
"""Get the cached CLI binary, downloading if necessary.
12051241
@@ -1910,14 +1946,14 @@ async def stop(self) -> None:
19101946
poll = getattr(self._cli_process, "poll", None)
19111947
is_running = poll is None or poll() is None
19121948
if is_running:
1913-
self._cli_process.terminate()
1949+
_kill_process_tree(self._cli_process)
19141950
try:
19151951
await asyncio.to_thread(
19161952
self._cli_process.wait,
19171953
timeout=_CLI_PROCESS_EXIT_TIMEOUT_SECONDS,
19181954
)
19191955
except subprocess.TimeoutExpired:
1920-
self._cli_process.kill()
1956+
_kill_process_tree(self._cli_process)
19211957
try:
19221958
await asyncio.to_thread(
19231959
self._cli_process.wait,
@@ -1976,7 +2012,7 @@ async def force_stop(self) -> None:
19762012
if self._process is not None and self._process is not self._cli_process:
19772013
self._process.terminate()
19782014
if self._cli_process is not None:
1979-
self._cli_process.kill()
2015+
_kill_process_tree(self._cli_process)
19802016
self._process = None
19812017
self._cli_process = None
19822018
except Exception:
@@ -4027,6 +4063,9 @@ async def _start_cli_server(self) -> None:
40274063
cwd=cwd,
40284064
env=env,
40294065
creationflags=creationflags,
4066+
# Place the runtime in its own process group so
4067+
# _kill_process_tree() can signal all descendants.
4068+
start_new_session=(sys.platform != "win32"),
40304069
)
40314070
self._cli_process = self._process
40324071
else:
@@ -4040,6 +4079,7 @@ async def _start_cli_server(self) -> None:
40404079
cwd=cwd,
40414080
env=env,
40424081
creationflags=creationflags,
4082+
start_new_session=(sys.platform != "win32"),
40434083
)
40444084
self._cli_process = self._process
40454085
log_timing(

0 commit comments

Comments
 (0)