diff --git a/go/client.go b/go/client.go index fb02897f9..7823237a8 100644 --- a/go/client.go +++ b/go/client.go @@ -656,7 +656,9 @@ func (c *Client) ForceStop() { // Kill the process without waiting for startStopMux, which Start may hold. // This unblocks any I/O Start is doing (connect, version check). if p := c.osProcess.Swap(nil); p != nil { - p.Kill() + if err := killProcessTreeByPid(p.Pid); err != nil { + p.Kill() + } } // Clear sessions immediately without trying to destroy them @@ -2231,8 +2233,10 @@ func (c *Client) killProcess() error { c.ffiHost = nil } if p := c.osProcess.Swap(nil); p != nil { - if err := p.Kill(); err != nil { - return fmt.Errorf("failed to kill CLI process: %w", err) + if err := killProcessTreeByPid(p.Pid); err != nil { + if killErr := p.Kill(); killErr != nil { + return fmt.Errorf("failed to kill CLI process: %w", killErr) + } } } c.process = nil diff --git a/go/process_other.go b/go/process_other.go index 5b3ba6353..1e64ddc25 100644 --- a/go/process_other.go +++ b/go/process_other.go @@ -2,10 +2,18 @@ package copilot -import "os/exec" +import ( + "os/exec" + "syscall" +) -// configureProcAttr configures platform-specific process attributes. -// On non-Windows platforms, this is a no-op. +// configureProcAttr places the runtime in its own process group so +// killProcessTreeByPid can signal all descendants atomically. func configureProcAttr(cmd *exec.Cmd) { - // No special configuration needed on non-Windows platforms + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} +} + +// killProcessTreeByPid signals the process group (negative PID) with SIGKILL. +func killProcessTreeByPid(pid int) error { + return syscall.Kill(-pid, syscall.SIGKILL) } diff --git a/go/process_windows.go b/go/process_windows.go index 37f954fca..cab164e89 100644 --- a/go/process_windows.go +++ b/go/process_windows.go @@ -3,14 +3,19 @@ package copilot import ( + "fmt" "os/exec" "syscall" ) -// configureProcAttr configures platform-specific process attributes. -// On Windows, this hides the console window to avoid distracting users in GUI apps. +// configureProcAttr hides the console window on Windows. func configureProcAttr(cmd *exec.Cmd) { cmd.SysProcAttr = &syscall.SysProcAttr{ HideWindow: true, } } + +// killProcessTreeByPid terminates the entire process tree via taskkill /T /F. +func killProcessTreeByPid(pid int) error { + return exec.Command("taskkill", "/T", "/F", "/PID", fmt.Sprintf("%d", pid)).Run() +} diff --git a/java/sdk/src/main/java/com/github/copilot/CopilotClient.java b/java/sdk/src/main/java/com/github/copilot/CopilotClient.java index cdd1b9ff3..154e5889f 100644 --- a/java/sdk/src/main/java/com/github/copilot/CopilotClient.java +++ b/java/sdk/src/main/java/com/github/copilot/CopilotClient.java @@ -812,19 +812,19 @@ private static void cleanupCliProcess(Process process, boolean forceImmediately) // will never come just wastes time, so terminate the child // immediately and only wait to reap it. if (forceImmediately) { - process.destroyForcibly(); + killProcessTree(process, true); if (!process.waitFor(FORCE_KILL_TIMEOUT_SECONDS, TimeUnit.SECONDS)) { LOG.fine("Process did not terminate within force kill timeout"); } return; } - process.destroy(); + killProcessTree(process, false); if (process.waitFor(FORCE_KILL_TIMEOUT_SECONDS, TimeUnit.SECONDS)) { return; } - process.destroyForcibly(); + killProcessTree(process, true); if (!process.waitFor(FORCE_KILL_TIMEOUT_SECONDS, TimeUnit.SECONDS)) { LOG.fine("Process did not terminate within force kill timeout"); } @@ -837,6 +837,36 @@ private static void cleanupCliProcess(Process process, boolean forceImmediately) } } + /** + * Terminates the runtime's process tree, ending the descendants before the root + * so none of them are reparented and left behind. + * + * @param process + * the runtime process + * @param force + * {@code true} to terminate forcibly, {@code false} to request a + * graceful exit first + */ + private static void killProcessTree(Process process, boolean force) { + try { + // descendants() is empty once the root is gone, so collect first. + process.toHandle().descendants().toList().forEach(ph -> { + if (force) { + ph.destroyForcibly(); + } else { + ph.destroy(); + } + }); + } catch (Exception e) { + LOG.log(Level.FINE, "Error terminating process descendants", e); + } + if (force) { + process.destroyForcibly(); + } else { + process.destroy(); + } + } + /** * Creates a new Copilot session with the specified configuration. *

diff --git a/nodejs/src/client.ts b/nodejs/src/client.ts index 30095186e..013826a08 100644 --- a/nodejs/src/client.ts +++ b/nodejs/src/client.ts @@ -11,7 +11,7 @@ * @module client */ -import { spawn, type ChildProcess } from "node:child_process"; +import { spawn, execSync, type ChildProcess } from "node:child_process"; import { randomUUID } from "node:crypto"; import { existsSync } from "node:fs"; import { createRequire } from "node:module"; @@ -153,6 +153,42 @@ async function waitForChildExit(child: ChildProcess, timeoutMs: number): Promise }); } +/** + * Terminate the runtime's process tree. + * + * - Windows: `taskkill /T /F` kills the entire tree rooted at `pid`. The signal + * is ignored because Windows has no graceful equivalent and `/T` can only + * enumerate the tree while the root is still alive. + * - POSIX: the runtime is spawned in its own process group (`detached: true`), + * so `kill(-pid, signal)` signals every process in that group. + * + * Falls back to `child.kill(signal)` whenever the tree-wide path is unavailable + * or fails, so behaviour degrades to the single-process termination it replaced. + * + * @see https://github.com/github/copilot-sdk/issues/1804 + */ +function killProcessTree(child: ChildProcess, signal: NodeJS.Signals = "SIGTERM"): boolean { + const pid = child.pid; + if (pid == null) { + return child.kill(signal); + } + if (process.platform === "win32") { + try { + execSync(`taskkill /T /F /PID ${pid}`, { stdio: "ignore", timeout: 5000 }); + return true; + } catch { + return child.kill(signal); + } + } + // POSIX: signal the process group (negative PID). + try { + process.kill(-pid, signal); + return true; + } catch { + return child.kill(signal); + } +} + /** * Convert tool parameters to JSON schema format for sending to CLI */ @@ -1104,8 +1140,13 @@ export class CopilotClient { this.cliProcess = null; try { if (child.exitCode == null && child.signalCode == null) { - child.kill(); - if (!(await waitForChildExit(child, RUNTIME_SHUTDOWN_TIMEOUT_MS))) { + killProcessTree(child, "SIGTERM"); + const rootExited = await waitForChildExit(child, RUNTIME_SHUTDOWN_TIMEOUT_MS); + // The root exiting says nothing about descendants that ignored + // SIGTERM, and they are the orphans this is meant to prevent, so + // sweep the group either way. + killProcessTree(child, "SIGKILL"); + if (!rootExited && !(await waitForChildExit(child, RUNTIME_SHUTDOWN_TIMEOUT_MS))) { errors.push( new Error( `Timed out waiting for CLI process to exit after kill: ${RUNTIME_SHUTDOWN_TIMEOUT_MS}ms` @@ -1231,7 +1272,7 @@ export class CopilotClient { // Force kill CLI process (only if we spawned it) if (this.cliProcess && !this.isExternalServer) { try { - this.cliProcess.kill("SIGKILL"); + killProcessTree(this.cliProcess, "SIGKILL"); } catch { // Ignore errors } @@ -2510,6 +2551,10 @@ export class CopilotClient { : ["ignore", "pipe", "pipe"]; // For .js files, spawn node explicitly; for executables, spawn directly + // Place the runtime in its own process group so killProcessTree() + // can signal all descendants atomically. On Windows detached has + // no effect — taskkill /T handles tree termination instead. + const detached = process.platform !== "win32"; const isJsFile = this.resolvedCliPath.endsWith(".js"); if (isJsFile) { this.cliProcess = spawn(getNodeExecPath(), [this.resolvedCliPath, ...args], { @@ -2517,6 +2562,7 @@ export class CopilotClient { cwd: this.options.workingDirectory, env: envWithoutNodeDebug, windowsHide: true, + detached, }); } else { this.cliProcess = spawn(this.resolvedCliPath, args, { @@ -2524,8 +2570,14 @@ export class CopilotClient { cwd: this.options.workingDirectory, env: envWithoutNodeDebug, windowsHide: true, + detached, }); } + // Prevent the detached child from keeping the parent's event loop + // alive when the embedder exits without calling stop(). + if (detached) { + this.cliProcess.unref(); + } let stdout = ""; let resolved = false; diff --git a/nodejs/test/process_tree_kill.test.ts b/nodejs/test/process_tree_kill.test.ts new file mode 100644 index 000000000..0ae791bf9 --- /dev/null +++ b/nodejs/test/process_tree_kill.test.ts @@ -0,0 +1,168 @@ +/** + * Tests for process-tree termination on stop()/forceStop(). + * + * Each case spawns a real process tree (a runtime stand-in that forks a + * long-lived grandchild), hands it to a CopilotClient as its owned runtime, + * and drives the public teardown methods. The assertions are on the SDK's + * behaviour, so removing the tree termination fails these tests. + * + * @see https://github.com/github/copilot-sdk/issues/1804 + */ +import { describe, expect, it } from "vitest"; +import { spawn, type ChildProcess } from "node:child_process"; +import { platform } from "node:os"; +import { CopilotClient, RuntimeConnection } from "../src/index.js"; + +const isWindows = platform() === "win32"; + +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +function sleep(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +/** Poll until the pid is gone, so the assertion does not race the OS. */ +async function waitForExit(pid: number, timeoutMs = 10000): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (!isProcessAlive(pid)) { + return true; + } + await sleep(50); + } + return !isProcessAlive(pid); +} + +/** + * Spawn a runtime stand-in that forks a long-lived grandchild, matching the + * SDK's own spawn flags so the group/tree shape is the real one. + * + * The grandchild is detached on Windows and left in the group on POSIX. Either + * way it outlives a kill aimed only at the root, which is what makes these + * tests fail if tree termination regresses to terminating the root. + * + * @param ignoreSigterm - make the grandchild survive SIGTERM, reproducing the + * descendant that outlived teardown in the linked issue. + */ +function spawnRuntimeStandIn(ignoreSigterm = false): { + parent: ChildProcess; + grandchildPid: Promise; +} { + const grandchildBody = ignoreSigterm + ? `process.on("SIGTERM", () => {}); process.stdout.write("ready"); setTimeout(() => {}, 120000);` + : `process.stdout.write("ready"); setTimeout(() => {}, 120000);`; + // The grandchild only reports its pid once it is running, otherwise a signal + // can land before its SIGTERM handler is installed and it dies by default. + const helperScript = ` + const { spawn } = require("child_process"); + const child = spawn(process.execPath, ["-e", ${JSON.stringify(grandchildBody)}], { + stdio: ["ignore", "pipe", "ignore"], + detached: ${String(isWindows)}, + }); + child.stdout.once("data", () => process.stdout.write(String(child.pid))); + child.unref(); + setTimeout(() => {}, 120000); + `; + const parent = spawn(process.execPath, ["-e", helperScript], { + stdio: ["ignore", "pipe", "ignore"], + detached: !isWindows, + }); + + const grandchildPid = new Promise((resolve, reject) => { + let data = ""; + parent.stdout!.on("data", (chunk) => { + data += chunk.toString(); + const pid = parseInt(data.trim(), 10); + if (!isNaN(pid) && pid > 0) resolve(pid); + }); + parent.once("error", reject); + setTimeout(() => reject(new Error("Timeout waiting for grandchild PID")), 5000); + }); + + return { parent, grandchildPid }; +} + +/** A client that owns `child` as its runtime, without starting a real CLI. */ +function clientOwning(child: ChildProcess): CopilotClient { + const client = new CopilotClient({ + connection: RuntimeConnection.forStdio({ path: "copilot" }), + }); + (client as unknown as { cliProcess: ChildProcess }).cliProcess = child; + (client as unknown as { isExternalServer: boolean }).isExternalServer = false; + return client; +} + +describe("process tree termination", () => { + it("stop() terminates descendants of the owned runtime", async () => { + const { parent, grandchildPid } = spawnRuntimeStandIn(); + const grandchild = await grandchildPid; + const parentPid = parent.pid!; + expect(isProcessAlive(parentPid)).toBe(true); + expect(isProcessAlive(grandchild)).toBe(true); + + const errors = await clientOwning(parent).stop(); + + expect(errors).toHaveLength(0); + expect(await waitForExit(parentPid)).toBe(true); + expect(await waitForExit(grandchild)).toBe(true); + }); + + it("forceStop() terminates descendants of the owned runtime", async () => { + const { parent, grandchildPid } = spawnRuntimeStandIn(); + const grandchild = await grandchildPid; + const parentPid = parent.pid!; + + await clientOwning(parent).forceStop(); + + expect(await waitForExit(parentPid)).toBe(true); + expect(await waitForExit(grandchild)).toBe(true); + }); + + // Windows has no SIGTERM, so the escalation this covers is POSIX-only. + it.skipIf(isWindows)( + "stop() reaps a descendant that ignores SIGTERM", + async () => { + const { parent, grandchildPid } = spawnRuntimeStandIn(true); + const grandchild = await grandchildPid; + const parentPid = parent.pid!; + + await clientOwning(parent).stop(); + + expect(await waitForExit(parentPid)).toBe(true); + expect(await waitForExit(grandchild)).toBe(true); + }, + 30000 + ); + + it("leaves processes alone for external-server connections", async () => { + const { parent, grandchildPid } = spawnRuntimeStandIn(); + const grandchild = await grandchildPid; + const parentPid = parent.pid!; + + const client = new CopilotClient({ + connection: RuntimeConnection.forUri("http://localhost:19999"), + }); + (client as unknown as { cliProcess: ChildProcess }).cliProcess = parent; + expect((client as unknown as { isExternalServer: boolean }).isExternalServer).toBe(true); + + const errors = await client.stop(); + + expect(errors).toHaveLength(0); + expect(isProcessAlive(parentPid)).toBe(true); + expect(isProcessAlive(grandchild)).toBe(true); + + parent.kill("SIGKILL"); + try { + process.kill(grandchild, "SIGKILL"); + } catch { + // Already gone. + } + }); +}); diff --git a/python/copilot/client.py b/python/copilot/client.py index 6cdd765c3..a75ad081b 100644 --- a/python/copilot/client.py +++ b/python/copilot/client.py @@ -20,6 +20,7 @@ import os import re import shutil +import signal import subprocess import sys import threading @@ -1259,6 +1260,40 @@ def _session_lifecycle_event_from_dict(data: dict) -> SessionLifecycleEvent: _CLI_PROCESS_EXIT_TIMEOUT_SECONDS = 5 +def _kill_process_tree(proc: subprocess.Popen[Any], *, force: bool = False) -> None: + """Signal the runtime's process tree. + + Windows: ``taskkill /T /F`` walks the tree rooted at *pid*. ``force`` is + ignored there because Windows has no graceful equivalent and ``/T`` can only + enumerate the tree while the root is still alive. POSIX: the runtime is + spawned with ``start_new_session=True``, so ``os.killpg`` reaches every + descendant that stayed in the group. + + Falls back to the ``Popen`` methods whenever a real pid is unavailable or + the tree-wide signal fails, so behaviour degrades to the single-process + termination this replaced. + + See: https://github.com/github/copilot-sdk/issues/1804 + """ + fallback = proc.kill if force else proc.terminate + pid = getattr(proc, "pid", None) + if not isinstance(pid, int): + fallback() + return + if sys.platform == "win32": + try: + args = ["taskkill", "/T", "/F", "/PID", str(pid)] + if subprocess.run(args, capture_output=True, timeout=5).returncode != 0: + fallback() + except Exception: + fallback() + return + try: + os.killpg(pid, signal.SIGKILL if force else signal.SIGTERM) + except (ProcessLookupError, PermissionError, OSError): + fallback() + + def _get_or_download_cli(*, include_runtime_lib: bool = False) -> str | None: """Get the cached CLI binary, downloading if necessary. @@ -1990,14 +2025,14 @@ async def stop(self) -> None: poll = getattr(self._cli_process, "poll", None) is_running = poll is None or poll() is None if is_running: - self._cli_process.terminate() + _kill_process_tree(self._cli_process) try: await asyncio.to_thread( self._cli_process.wait, timeout=_CLI_PROCESS_EXIT_TIMEOUT_SECONDS, ) except subprocess.TimeoutExpired: - self._cli_process.kill() + _kill_process_tree(self._cli_process, force=True) try: await asyncio.to_thread( self._cli_process.wait, @@ -2056,7 +2091,7 @@ async def force_stop(self) -> None: if self._process is not None and self._process is not self._cli_process: self._process.terminate() if self._cli_process is not None: - self._cli_process.kill() + _kill_process_tree(self._cli_process, force=True) self._process = None self._cli_process = None except Exception: @@ -4182,6 +4217,9 @@ async def _start_cli_server(self) -> None: cwd=cwd, env=env, creationflags=creationflags, + # Place the runtime in its own process group so + # _kill_process_tree() can signal all descendants. + start_new_session=(sys.platform != "win32"), ) self._cli_process = self._process else: @@ -4195,6 +4233,7 @@ async def _start_cli_server(self) -> None: cwd=cwd, env=env, creationflags=creationflags, + start_new_session=(sys.platform != "win32"), ) self._cli_process = self._process log_timing( diff --git a/python/test_client.py b/python/test_client.py index cf4bdf192..dd9b87991 100644 --- a/python/test_client.py +++ b/python/test_client.py @@ -7,12 +7,14 @@ import asyncio import inspect import os +import signal from datetime import UTC, datetime from tempfile import TemporaryDirectory from unittest.mock import AsyncMock, Mock, patch import pytest +import copilot.client from copilot import ( CanvasProviderIdentity, CapiSessionOptions, @@ -174,6 +176,89 @@ async def test_force_stop_external_server_clears_process_references(self): assert client._cli_process is None +class TestKillProcessTree: + """Coverage for the platform paths of the process-tree termination helper.""" + + @staticmethod + def _proc(pid=4321): + proc = Mock() + proc.pid = pid + return proc + + @staticmethod + def _as_posix(monkeypatch): + """Pretend to be POSIX, supplying the symbols Windows hosts lack.""" + monkeypatch.setattr(copilot.client.sys, "platform", "linux") + monkeypatch.setattr(copilot.client.signal, "SIGKILL", 9, raising=False) + + def test_posix_signals_the_process_group(self, monkeypatch): + proc = self._proc() + signalled: list[tuple[int, int]] = [] + self._as_posix(monkeypatch) + monkeypatch.setattr( + copilot.client.os, + "killpg", + lambda pid, sig: signalled.append((pid, sig)), + raising=False, + ) + + copilot.client._kill_process_tree(proc) + copilot.client._kill_process_tree(proc, force=True) + + assert signalled == [(4321, signal.SIGTERM), (4321, copilot.client.signal.SIGKILL)] + proc.terminate.assert_not_called() + proc.kill.assert_not_called() + + def test_posix_falls_back_when_the_group_is_gone(self, monkeypatch): + proc = self._proc() + self._as_posix(monkeypatch) + + def boom(pid, sig): + raise ProcessLookupError + + monkeypatch.setattr(copilot.client.os, "killpg", boom, raising=False) + + copilot.client._kill_process_tree(proc) + + proc.terminate.assert_called_once() + + def test_windows_uses_taskkill_for_the_whole_tree(self, monkeypatch): + proc = self._proc() + commands: list[list[str]] = [] + monkeypatch.setattr(copilot.client.sys, "platform", "win32") + monkeypatch.setattr( + copilot.client.subprocess, + "run", + lambda args, **kwargs: commands.append(args) or Mock(returncode=0), + ) + + copilot.client._kill_process_tree(proc) + + assert commands == [["taskkill", "/T", "/F", "/PID", "4321"]] + proc.terminate.assert_not_called() + + def test_windows_falls_back_when_taskkill_fails(self, monkeypatch): + proc = self._proc() + monkeypatch.setattr(copilot.client.sys, "platform", "win32") + monkeypatch.setattr( + copilot.client.subprocess, "run", lambda args, **kwargs: Mock(returncode=128) + ) + + copilot.client._kill_process_tree(proc, force=True) + + proc.kill.assert_called_once() + + def test_missing_pid_falls_back_to_the_process_handle(self, monkeypatch): + proc = Mock() # Mock.pid is not an int + monkeypatch.setattr( + copilot.client.os, "killpg", Mock(side_effect=AssertionError), raising=False + ) + + copilot.client._kill_process_tree(proc) + + proc.terminate.assert_called_once() + + class TestPermissionHandlerOptional: @pytest.mark.asyncio async def test_create_session_allows_missing_permission_handler(self):