Skip to content

Commit da078a6

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 1935fd3 commit da078a6

9 files changed

Lines changed: 487 additions & 30 deletions

File tree

‎go/client.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -656,7 +656,9 @@ func (c *Client) ForceStop() {
656656
// Kill the process without waiting for startStopMux, which Start may hold.
657657
// This unblocks any I/O Start is doing (connect, version check).
658658
if p := c.osProcess.Swap(nil); p != nil {
659-
p.Kill()
659+
if err := killProcessTreeByPid(p.Pid); err != nil {
660+
p.Kill()
661+
}
660662
}
661663

662664
// Clear sessions immediately without trying to destroy them
@@ -2231,8 +2233,10 @@ func (c *Client) killProcess() error {
22312233
c.ffiHost = nil
22322234
}
22332235
if p := c.osProcess.Swap(nil); p != nil {
2234-
if err := p.Kill(); err != nil {
2235-
return fmt.Errorf("failed to kill CLI process: %w", err)
2236+
if err := killProcessTreeByPid(p.Pid); err != nil {
2237+
if killErr := p.Kill(); killErr != nil {
2238+
return fmt.Errorf("failed to kill CLI process: %w", killErr)
2239+
}
22362240
}
22372241
}
22382242
c.process = 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) error {
18+
return 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) error {
20+
return exec.Command("taskkill", "/T", "/F", "/PID", fmt.Sprintf("%d", pid)).Run()
21+
}

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

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -812,19 +812,19 @@ private static void cleanupCliProcess(Process process, boolean forceImmediately)
812812
// will never come just wastes time, so terminate the child
813813
// immediately and only wait to reap it.
814814
if (forceImmediately) {
815-
process.destroyForcibly();
815+
killProcessTree(process, true);
816816
if (!process.waitFor(FORCE_KILL_TIMEOUT_SECONDS, TimeUnit.SECONDS)) {
817817
LOG.fine("Process did not terminate within force kill timeout");
818818
}
819819
return;
820820
}
821821

822-
process.destroy();
822+
killProcessTree(process, false);
823823
if (process.waitFor(FORCE_KILL_TIMEOUT_SECONDS, TimeUnit.SECONDS)) {
824824
return;
825825
}
826826

827-
process.destroyForcibly();
827+
killProcessTree(process, true);
828828
if (!process.waitFor(FORCE_KILL_TIMEOUT_SECONDS, TimeUnit.SECONDS)) {
829829
LOG.fine("Process did not terminate within force kill timeout");
830830
}
@@ -837,6 +837,36 @@ private static void cleanupCliProcess(Process process, boolean forceImmediately)
837837
}
838838
}
839839

840+
/**
841+
* Terminates the runtime's process tree, ending the descendants before the root
842+
* so none of them are reparented and left behind.
843+
*
844+
* @param process
845+
* the runtime process
846+
* @param force
847+
* {@code true} to terminate forcibly, {@code false} to request a
848+
* graceful exit first
849+
*/
850+
private static void killProcessTree(Process process, boolean force) {
851+
try {
852+
// descendants() is empty once the root is gone, so collect first.
853+
process.toHandle().descendants().toList().forEach(ph -> {
854+
if (force) {
855+
ph.destroyForcibly();
856+
} else {
857+
ph.destroy();
858+
}
859+
});
860+
} catch (Exception e) {
861+
LOG.log(Level.FINE, "Error terminating process descendants", e);
862+
}
863+
if (force) {
864+
process.destroyForcibly();
865+
} else {
866+
process.destroy();
867+
}
868+
}
869+
840870
/**
841871
* Creates a new Copilot session with the specified configuration.
842872
* <p>

‎nodejs/src/client.ts‎

Lines changed: 56 additions & 4 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,42 @@ 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`. The signal
160+
* is ignored because Windows has no graceful equivalent and `/T` can only
161+
* enumerate the tree while the root is still alive.
162+
* - POSIX: the runtime is spawned in its own process group (`detached: true`),
163+
* so `kill(-pid, signal)` signals every process in that group.
164+
*
165+
* Falls back to `child.kill(signal)` whenever the tree-wide path is unavailable
166+
* or fails, so behaviour degrades to the single-process termination it replaced.
167+
*
168+
* @see https://github.com/github/copilot-sdk/issues/1804
169+
*/
170+
function killProcessTree(child: ChildProcess, signal: NodeJS.Signals = "SIGTERM"): boolean {
171+
const pid = child.pid;
172+
if (pid == null) {
173+
return child.kill(signal);
174+
}
175+
if (process.platform === "win32") {
176+
try {
177+
execSync(`taskkill /T /F /PID ${pid}`, { stdio: "ignore", timeout: 5000 });
178+
return true;
179+
} catch {
180+
return child.kill(signal);
181+
}
182+
}
183+
// POSIX: signal the process group (negative PID).
184+
try {
185+
process.kill(-pid, signal);
186+
return true;
187+
} catch {
188+
return child.kill(signal);
189+
}
190+
}
191+
156192
/**
157193
* Convert tool parameters to JSON schema format for sending to CLI
158194
*/
@@ -1104,8 +1140,13 @@ export class CopilotClient {
11041140
this.cliProcess = null;
11051141
try {
11061142
if (child.exitCode == null && child.signalCode == null) {
1107-
child.kill();
1108-
if (!(await waitForChildExit(child, RUNTIME_SHUTDOWN_TIMEOUT_MS))) {
1143+
killProcessTree(child, "SIGTERM");
1144+
const rootExited = await waitForChildExit(child, RUNTIME_SHUTDOWN_TIMEOUT_MS);
1145+
// The root exiting says nothing about descendants that ignored
1146+
// SIGTERM, and they are the orphans this is meant to prevent, so
1147+
// sweep the group either way.
1148+
killProcessTree(child, "SIGKILL");
1149+
if (!rootExited && !(await waitForChildExit(child, RUNTIME_SHUTDOWN_TIMEOUT_MS))) {
11091150
errors.push(
11101151
new Error(
11111152
`Timed out waiting for CLI process to exit after kill: ${RUNTIME_SHUTDOWN_TIMEOUT_MS}ms`
@@ -1231,7 +1272,7 @@ export class CopilotClient {
12311272
// Force kill CLI process (only if we spawned it)
12321273
if (this.cliProcess && !this.isExternalServer) {
12331274
try {
1234-
this.cliProcess.kill("SIGKILL");
1275+
killProcessTree(this.cliProcess, "SIGKILL");
12351276
} catch {
12361277
// Ignore errors
12371278
}
@@ -2510,22 +2551,33 @@ export class CopilotClient {
25102551
: ["ignore", "pipe", "pipe"];
25112552

25122553
// For .js files, spawn node explicitly; for executables, spawn directly
2554+
// Place the runtime in its own process group so killProcessTree()
2555+
// can signal all descendants atomically. On Windows detached has
2556+
// no effect — taskkill /T handles tree termination instead.
2557+
const detached = process.platform !== "win32";
25132558
const isJsFile = this.resolvedCliPath.endsWith(".js");
25142559
if (isJsFile) {
25152560
this.cliProcess = spawn(getNodeExecPath(), [this.resolvedCliPath, ...args], {
25162561
stdio: stdioConfig,
25172562
cwd: this.options.workingDirectory,
25182563
env: envWithoutNodeDebug,
25192564
windowsHide: true,
2565+
detached,
25202566
});
25212567
} else {
25222568
this.cliProcess = spawn(this.resolvedCliPath, args, {
25232569
stdio: stdioConfig,
25242570
cwd: this.options.workingDirectory,
25252571
env: envWithoutNodeDebug,
25262572
windowsHide: true,
2573+
detached,
25272574
});
25282575
}
2576+
// Prevent the detached child from keeping the parent's event loop
2577+
// alive when the embedder exits without calling stop().
2578+
if (detached) {
2579+
this.cliProcess.unref();
2580+
}
25292581

25302582
let stdout = "";
25312583
let resolved = false;

0 commit comments

Comments
 (0)