Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions go/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
16 changes: 12 additions & 4 deletions go/process_other.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
9 changes: 7 additions & 2 deletions go/process_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
36 changes: 33 additions & 3 deletions java/sdk/src/main/java/com/github/copilot/CopilotClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand All @@ -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.
* <p>
Expand Down
60 changes: 56 additions & 4 deletions nodejs/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default stop() path sends SIGTERM and waits only for the root. I manually tested a runtime descendant that ignores SIGTERM: the root exited, stop() completed, and the descendant remained alive. Since runtime.shutdown has already completed, final owned-tree teardown should be definitive (or follow SIGTERM with an unconditional group SIGKILL check).

return true;
} catch {
return child.kill(signal);
}
}

/**
* Convert tool parameters to JSON schema format for sending to CLI
*/
Expand Down Expand Up @@ -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`
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -2510,22 +2551,33 @@ 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], {
stdio: stdioConfig,
cwd: this.options.workingDirectory,
env: envWithoutNodeDebug,
windowsHide: true,
detached,
});
} else {
this.cliProcess = spawn(this.resolvedCliPath, args, {
stdio: stdioConfig,
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;
Expand Down
Loading