diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 989aa4aef..9fc3bfc95 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -124,6 +124,7 @@ jobs: src/utils/spawn.launch-regression.test.ts src/utils/spawn.percent-escape.test.ts src/utils/spawn.test.ts + src/utils/process-tree.test.ts src/utils/run-git-command.test.ts src/utils/checkout-git-rev-parse.test.ts src/terminal/worker-terminal-manager.test.ts diff --git a/packages/server/src/server/agent/providers/codex-app-server-agent.ts b/packages/server/src/server/agent/providers/codex-app-server-agent.ts index d309ae244..162a4b6c0 100644 --- a/packages/server/src/server/agent/providers/codex-app-server-agent.ts +++ b/packages/server/src/server/agent/providers/codex-app-server-agent.ts @@ -50,6 +50,7 @@ import { type ProviderRuntimeSettings, } from "../provider-launch-config.js"; import { findExecutable, isCommandAvailable } from "../../../utils/executable.js"; +import { terminateProcessTree } from "../../../utils/process-tree.js"; import { spawnProcess } from "../../../utils/spawn.js"; import { extractCodexTerminalSessionId, nonEmptyString } from "./tool-call-mapper-utils.js"; import { buildCodexFeatures, codexModelSupportsFastMode } from "./codex-feature-definitions.js"; @@ -602,17 +603,12 @@ class CodexAppServerClient { private nextId = 1; private disposed = false; private stderrBuffer = ""; - private readonly exitPromise: Promise; - private resolveExitPromise: (() => void) | null = null; constructor( private readonly child: ChildProcessWithoutNullStreams, private readonly logger: Logger, ) { this.rl = readline.createInterface({ input: child.stdout }); - this.exitPromise = new Promise((resolve) => { - this.resolveExitPromise = resolve; - }); this.rl.on("line", (line) => this.handleLine(line)); child.stderr.on("data", (chunk) => { @@ -630,8 +626,6 @@ class CodexAppServerClient { } this.pending.clear(); this.disposed = true; - this.resolveExitPromise?.(); - this.resolveExitPromise = null; }); child.on("exit", (code, signal) => { @@ -646,8 +640,6 @@ class CodexAppServerClient { } this.pending.clear(); this.disposed = true; - this.resolveExitPromise?.(); - this.resolveExitPromise = null; }); } @@ -704,39 +696,21 @@ class CodexAppServerClient { } catch { // ignore } - signalChildProcessTree(this.child, "SIGTERM"); - if (await this.waitForExit(APP_SERVER_GRACEFUL_SHUTDOWN_TIMEOUT_MS)) { - return; - } - - this.logger.warn( - { timeoutMs: APP_SERVER_GRACEFUL_SHUTDOWN_TIMEOUT_MS }, - "Codex app-server did not exit after SIGTERM; sending SIGKILL", - ); - signalChildProcessTree(this.child, "SIGKILL"); - if (await this.waitForExit(APP_SERVER_FORCE_SHUTDOWN_TIMEOUT_MS)) { - return; - } - - this.logger.warn( - { timeoutMs: APP_SERVER_FORCE_SHUTDOWN_TIMEOUT_MS }, - "Codex app-server did not report exit after SIGKILL", - ); - } - - private async waitForExit(timeoutMs: number): Promise { - let timer: NodeJS.Timeout | null = null; - try { - return await Promise.race([ - this.exitPromise.then(() => true), - new Promise((resolve) => { - timer = setTimeout(() => resolve(false), timeoutMs); - }), - ]); - } finally { - if (timer) { - clearTimeout(timer); - } + const result = await terminateProcessTree(this.child, { + gracefulTimeoutMs: APP_SERVER_GRACEFUL_SHUTDOWN_TIMEOUT_MS, + forceTimeoutMs: APP_SERVER_FORCE_SHUTDOWN_TIMEOUT_MS, + onForceSignal: () => { + this.logger.warn( + { timeoutMs: APP_SERVER_GRACEFUL_SHUTDOWN_TIMEOUT_MS }, + "Codex app-server did not exit after SIGTERM; sending SIGKILL", + ); + }, + }); + if (result === "kill-timeout") { + this.logger.warn( + { timeoutMs: APP_SERVER_FORCE_SHUTDOWN_TIMEOUT_MS }, + "Codex app-server did not report exit after SIGKILL", + ); } } @@ -789,30 +763,6 @@ class CodexAppServerClient { } } -function signalChildProcessTree( - child: ChildProcessWithoutNullStreams, - signal: NodeJS.Signals, -): void { - if (child.exitCode !== null || child.signalCode !== null) { - return; - } - - if (process.platform !== "win32" && typeof child.pid === "number" && child.pid > 0) { - try { - process.kill(-child.pid, signal); - return; - } catch { - // Fall back to the direct child when no separate process group exists. - } - } - - try { - child.kill(signal); - } catch { - // ignore - } -} - function toAgentUsage(tokenUsage: unknown): AgentUsage | undefined { if (!tokenUsage || typeof tokenUsage !== "object") return undefined; const usage = tokenUsage as { diff --git a/packages/server/src/server/agent/providers/opencode-agent.ts b/packages/server/src/server/agent/providers/opencode-agent.ts index d45e36194..ea6209aae 100644 --- a/packages/server/src/server/agent/providers/opencode-agent.ts +++ b/packages/server/src/server/agent/providers/opencode-agent.ts @@ -46,6 +46,7 @@ import { type ProviderRuntimeSettings, } from "../provider-launch-config.js"; import { findExecutable, isCommandAvailable } from "../../../utils/executable.js"; +import { terminateProcessTree } from "../../../utils/process-tree.js"; import { withTimeout } from "../../../utils/promise-timeout.js"; import { spawnProcess } from "../../../utils/spawn.js"; import { buildToolCallDisplayModel } from "../../../shared/tool-call-display.js"; @@ -109,6 +110,8 @@ type OpenCodeMcpConfig = const MCP_ALREADY_PRESENT_ERROR_TOKENS = ["already", "exists", "connected"] as const; const OPENCODE_PROVIDER_LIST_TIMEOUT_MS = 30_000; +const OPENCODE_SERVER_GRACEFUL_SHUTDOWN_TIMEOUT_MS = 5_000; +const OPENCODE_SERVER_FORCE_SHUTDOWN_TIMEOUT_MS = 1_000; const OPENCODE_HANDLED_BUILTIN_SLASH_COMMANDS: AgentSlashCommand[] = [ { name: "compact", description: "Compact the current session", argumentHint: "" }, { name: "summarize", description: "Compact the current session", argumentHint: "" }, @@ -888,6 +891,7 @@ export class OpenCodeServerManager { launchPrefix.command, [...launchPrefix.args, "serve", "--port", String(port)], { + detached: process.platform !== "win32", stdio: ["ignore", "pipe", "pipe"], ...createProviderEnvSpec({ runtimeSettings: this.runtimeSettings }), }, @@ -990,24 +994,22 @@ export class OpenCodeServerManager { if (server.process.killed) { return; } - await new Promise((resolve) => { - let pendingResolve: (() => void) | null = resolve; - const settle = () => { - if (!pendingResolve) return; - const fn = pendingResolve; - pendingResolve = null; - fn(); - }; - const timeout = setTimeout(() => { - server.process.kill("SIGKILL"); - settle(); - }, 5000); - server.process.on("exit", () => { - clearTimeout(timeout); - settle(); - }); - server.process.kill("SIGTERM"); + const result = await terminateProcessTree(server.process, { + gracefulTimeoutMs: OPENCODE_SERVER_GRACEFUL_SHUTDOWN_TIMEOUT_MS, + forceTimeoutMs: OPENCODE_SERVER_FORCE_SHUTDOWN_TIMEOUT_MS, + onForceSignal: () => { + this.logger.warn( + { timeoutMs: OPENCODE_SERVER_GRACEFUL_SHUTDOWN_TIMEOUT_MS }, + "OpenCode server did not exit after SIGTERM; sending SIGKILL", + ); + }, }); + if (result === "kill-timeout") { + this.logger.warn( + { timeoutMs: OPENCODE_SERVER_FORCE_SHUTDOWN_TIMEOUT_MS }, + "OpenCode server did not report exit after SIGKILL", + ); + } } } diff --git a/packages/server/src/utils/process-tree.test.ts b/packages/server/src/utils/process-tree.test.ts new file mode 100644 index 000000000..e56e71f01 --- /dev/null +++ b/packages/server/src/utils/process-tree.test.ts @@ -0,0 +1,259 @@ +import { execFile, spawn } from "node:child_process"; +import { EventEmitter } from "node:events"; +import { promisify } from "node:util"; +import { describe, expect, test, vi } from "vitest"; + +import { signalProcessTree, terminateProcessTree, type ProcessTreeTarget } from "./process-tree.js"; + +const execFileAsync = promisify(execFile); + +describe("signalProcessTree", () => { + test.each(["darwin", "linux"] as const)("signals the %s process group", (platform) => { + const child = createChild({ pid: 1234 }); + const kill = vi.fn(() => true); + + signalProcessTree(child, "SIGTERM", { platform, kill }); + + expect(kill).toHaveBeenCalledWith(-1234, "SIGTERM"); + expect(child.kill).not.toHaveBeenCalled(); + }); + + test("falls back to direct child signaling when POSIX process group signaling fails", () => { + const child = createChild({ pid: 1234 }); + + signalProcessTree(child, "SIGTERM", { + platform: "darwin", + kill: vi.fn(() => { + throw new Error("no process group"); + }), + }); + + expect(child.kill).toHaveBeenCalledWith("SIGTERM"); + }); + + test("uses taskkill for Windows process trees", () => { + const child = createChild({ pid: 1234 }); + const execFileMock = vi.fn((_file, _args, callback) => { + callback(null); + }); + + signalProcessTree(child, "SIGTERM", { platform: "win32", execFile: execFileMock }); + + expect(execFileMock).toHaveBeenCalledWith( + "taskkill", + ["/pid", "1234", "/T"], + expect.any(Function), + ); + expect(child.kill).not.toHaveBeenCalled(); + }); + + test("forces Windows process tree cleanup for SIGKILL", () => { + const child = createChild({ pid: 1234 }); + const execFileMock = vi.fn((_file, _args, callback) => { + callback(null); + }); + + signalProcessTree(child, "SIGKILL", { platform: "win32", execFile: execFileMock }); + + expect(execFileMock).toHaveBeenCalledWith( + "taskkill", + ["/pid", "1234", "/T", "/F"], + expect.any(Function), + ); + }); + + test("falls back to direct child signaling when taskkill cannot run", () => { + const child = createChild({ pid: 1234 }); + const execFileMock = vi.fn((_file, _args, callback) => { + callback(Object.assign(new Error("taskkill failed"), { code: "ENOENT" })); + }); + + signalProcessTree(child, "SIGTERM", { platform: "win32", execFile: execFileMock }); + + expect(child.kill).toHaveBeenCalledWith("SIGTERM"); + }); + + test("does not signal an exited child", () => { + const child = createChild({ pid: 1234, exitCode: 0 }); + const kill = vi.fn(() => true); + + signalProcessTree(child, "SIGTERM", { platform: "darwin", kill }); + + expect(kill).not.toHaveBeenCalled(); + expect(child.kill).not.toHaveBeenCalled(); + }); + + test("terminateProcessTree returns after graceful exit", async () => { + const child = createEventedChild({ pid: 1234 }); + const kill = vi.fn(() => true); + + const resultPromise = terminateProcessTree(child, { + platform: "darwin", + gracefulTimeoutMs: 100, + forceTimeoutMs: 100, + kill, + }); + child.emit("exit"); + + await expect(resultPromise).resolves.toBe("terminated"); + expect(kill).toHaveBeenCalledWith(-1234, "SIGTERM"); + expect(kill).not.toHaveBeenCalledWith(-1234, "SIGKILL"); + }); + + test("terminateProcessTree sends SIGKILL after graceful timeout", async () => { + vi.useFakeTimers(); + try { + const child = createEventedChild({ pid: 1234 }); + const kill = vi.fn(() => true); + const onForceSignal = vi.fn(); + + const resultPromise = terminateProcessTree(child, { + platform: "darwin", + gracefulTimeoutMs: 100, + forceTimeoutMs: 100, + kill, + onForceSignal, + }); + + await vi.advanceTimersByTimeAsync(100); + expect(onForceSignal).toHaveBeenCalledTimes(1); + expect(kill).toHaveBeenNthCalledWith(1, -1234, "SIGTERM"); + expect(kill).toHaveBeenNthCalledWith(2, -1234, "SIGKILL"); + + child.emit("exit"); + await expect(resultPromise).resolves.toBe("killed"); + } finally { + vi.useRealTimers(); + } + }); + + test("terminateProcessTree reports when forced cleanup does not exit", async () => { + vi.useFakeTimers(); + try { + const child = createEventedChild({ pid: 1234 }); + const kill = vi.fn(() => true); + + const resultPromise = terminateProcessTree(child, { + platform: "darwin", + gracefulTimeoutMs: 100, + forceTimeoutMs: 100, + kill, + }); + + await vi.advanceTimersByTimeAsync(200); + + await expect(resultPromise).resolves.toBe("kill-timeout"); + expect(kill).toHaveBeenNthCalledWith(1, -1234, "SIGTERM"); + expect(kill).toHaveBeenNthCalledWith(2, -1234, "SIGKILL"); + } finally { + vi.useRealTimers(); + } + }); + + test.skipIf(process.platform === "win32")("kills a detached POSIX process group", async () => { + const child = spawn("/bin/sh", ["-c", "sleep 60 & wait"], { + detached: true, + stdio: "ignore", + }); + const pid = child.pid; + if (typeof pid !== "number") { + throw new Error("spawned shell did not expose a pid"); + } + + let childPids: number[] = []; + try { + childPids = await waitForChildren(pid); + await expect( + terminateProcessTree(child, { + gracefulTimeoutMs: 1000, + forceTimeoutMs: 1000, + }), + ).resolves.toMatch(/^(terminated|killed)$/); + await waitForNoLivePids(childPids); + } finally { + cleanupPidGroup(pid); + for (const childPid of childPids) { + cleanupPid(childPid); + } + } + }); +}); + +function createChild(options: { pid?: number; exitCode?: number | null } = {}): ProcessTreeTarget { + return { + pid: options.pid, + exitCode: options.exitCode ?? null, + signalCode: null, + kill: vi.fn(() => true), + }; +} + +function createEventedChild(options: { pid?: number }): ProcessTreeTarget & EventEmitter { + const child = new EventEmitter() as ProcessTreeTarget & EventEmitter; + child.pid = options.pid; + child.exitCode = null; + child.signalCode = null; + child.kill = vi.fn(() => true); + return child; +} + +async function waitForChildren(parentPid: number): Promise { + return waitFor(async () => { + const { stdout } = await execFileAsync("pgrep", ["-P", String(parentPid)]).catch(() => ({ + stdout: "", + })); + const pids = stdout + .trim() + .split(/\s+/) + .filter(Boolean) + .map((value) => Number.parseInt(value, 10)) + .filter((value) => Number.isInteger(value) && value > 0); + return pids.length > 0 ? pids : null; + }); +} + +async function waitForNoLivePids(pids: number[]): Promise { + await waitFor(() => (pids.every((pid) => !isPidAlive(pid)) ? true : null)); +} + +async function waitFor(probe: () => T | null | Promise): Promise { + const deadline = Date.now() + 3000; + let lastError: unknown; + while (Date.now() < deadline) { + try { + const value = await probe(); + if (value !== null) { + return value; + } + } catch (error) { + lastError = error; + } + await new Promise((resolve) => setTimeout(resolve, 50)); + } + throw lastError instanceof Error ? lastError : new Error("timed out waiting for condition"); +} + +function isPidAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +function cleanupPidGroup(pid: number): void { + try { + process.kill(-pid, "SIGKILL"); + } catch { + // ignore cleanup races + } +} + +function cleanupPid(pid: number): void { + try { + process.kill(pid, "SIGKILL"); + } catch { + // ignore cleanup races + } +} diff --git a/packages/server/src/utils/process-tree.ts b/packages/server/src/utils/process-tree.ts new file mode 100644 index 000000000..5963d8ffc --- /dev/null +++ b/packages/server/src/utils/process-tree.ts @@ -0,0 +1,155 @@ +import { execFile } from "node:child_process"; + +export interface ProcessTreeTarget { + pid?: number; + killed?: boolean; + exitCode?: number | null; + signalCode?: NodeJS.Signals | null; + kill(signal?: NodeJS.Signals | number): boolean; + once?(event: "exit", listener: () => void): unknown; +} + +interface SignalProcessTreeOptions { + platform?: NodeJS.Platform; + kill?: (pid: number, signal?: NodeJS.Signals | number) => boolean; + execFile?: ( + file: string, + args: string[], + callback: (error: NodeJS.ErrnoException | null) => void, + ) => unknown; +} + +interface TerminateProcessTreeOptions extends SignalProcessTreeOptions { + gracefulSignal?: NodeJS.Signals; + forceSignal?: NodeJS.Signals; + gracefulTimeoutMs: number; + forceTimeoutMs?: number; + onForceSignal?: () => void; +} + +export type TerminateProcessTreeResult = + | "already-exited" + | "terminated" + | "killed" + | "kill-timeout"; + +export async function terminateProcessTree( + child: ProcessTreeTarget, + options: TerminateProcessTreeOptions, +): Promise { + if (isProcessExited(child)) { + return "already-exited"; + } + + const exitPromise = waitForProcessExit(child); + signalProcessTree(child, options.gracefulSignal ?? "SIGTERM", options); + if (await waitForExitOrTimeout(exitPromise, options.gracefulTimeoutMs)) { + return "terminated"; + } + + options.onForceSignal?.(); + signalProcessTree(child, options.forceSignal ?? "SIGKILL", options); + if (options.forceTimeoutMs === undefined) { + return "killed"; + } + return (await waitForExitOrTimeout(exitPromise, options.forceTimeoutMs)) + ? "killed" + : "kill-timeout"; +} + +export function signalProcessTree( + child: ProcessTreeTarget, + signal: NodeJS.Signals, + options: SignalProcessTreeOptions = {}, +): void { + if (isProcessExited(child)) { + return; + } + + const pid = child.pid; + if (typeof pid === "number" && pid > 0) { + const platform = options.platform ?? process.platform; + if (platform === "win32") { + signalWindowsProcessTree(child, pid, signal, options); + return; + } + + try { + (options.kill ?? process.kill)(-pid, signal); + return; + } catch { + // Fall back to the direct child when no separate process group exists. + } + } + + signalDirectChild(child, signal); +} + +function signalWindowsProcessTree( + child: ProcessTreeTarget, + pid: number, + signal: NodeJS.Signals, + options: SignalProcessTreeOptions, +): void { + const args = ["/pid", String(pid), "/T"]; + if (signal === "SIGKILL") { + args.push("/F"); + } + + try { + (options.execFile ?? execFile)("taskkill", args, (error) => { + if (error) { + signalDirectChild(child, signal); + } + }); + } catch { + signalDirectChild(child, signal); + } +} + +function signalDirectChild(child: ProcessTreeTarget, signal: NodeJS.Signals): void { + try { + child.kill(signal); + } catch { + // Ignore cleanup races. + } +} + +function isProcessExited(child: ProcessTreeTarget): boolean { + return ( + (child.exitCode !== null && child.exitCode !== undefined) || + (child.signalCode !== null && child.signalCode !== undefined) + ); +} + +function waitForProcessExit(child: ProcessTreeTarget): Promise { + if (isProcessExited(child)) { + return Promise.resolve(); + } + if (!child.once) { + return new Promise(() => undefined); + } + + return new Promise((resolve) => { + child.once?.("exit", resolve); + }); +} + +async function waitForExitOrTimeout( + exitPromise: Promise, + timeoutMs: number, +): Promise { + let timer: NodeJS.Timeout | null = null; + try { + return await Promise.race([ + exitPromise.then(() => true), + new Promise((resolve) => { + timer = setTimeout(() => resolve(false), timeoutMs); + }), + ]); + } finally { + if (timer) { + clearTimeout(timer); + } + } +}