diff --git a/server/index.ts b/server/index.ts index f3731f4b..0ab640e6 100644 --- a/server/index.ts +++ b/server/index.ts @@ -14,6 +14,9 @@ import { agentsRouter } from "./routes/agents.js"; import { skillsRouter } from "./routes/skills.js"; import { getAvailableAgentProviders } from "./lib/agents.js"; import { listSessionRuntimes } from "./lib/session-runtime.js"; +import { + installSessionRuntimeShutdownHandlers, +} from "./lib/process-shutdown.js"; import { listPersistedAttentionSessionIds } from "./lib/session-attention.js"; import { getProject } from "./lib/projects.js"; import { resolveWorktree } from "./lib/worktrees.js"; @@ -63,6 +66,8 @@ function parsePort(value: string | undefined, fallback: number): number { const DEV_API_BASE_PORT = 3102; const app = express(); + +installSessionRuntimeShutdownHandlers(); app.use(cors()); app.use(express.json({ limit: "50mb" })); diff --git a/server/lib/__tests__/anita-spawn.test.ts b/server/lib/__tests__/anita-spawn.test.ts index 7e988f78..d663daa3 100644 --- a/server/lib/__tests__/anita-spawn.test.ts +++ b/server/lib/__tests__/anita-spawn.test.ts @@ -1,10 +1,10 @@ import test from "node:test"; import assert from "node:assert/strict"; import { mkdtempSync, writeFileSync, chmodSync, rmSync } from "node:fs"; -import { spawn as childSpawn } from "node:child_process"; +import { execFileSync, spawn as childSpawn } from "node:child_process"; import os from "node:os"; import path from "node:path"; -import { getAgentProvider } from "../agents.js"; +import { getAgentProvider, signalAgentProcess } from "../agents.js"; const anita = getAgentProvider("anita"); assert.ok(anita, "anita provider must be registered"); @@ -192,6 +192,66 @@ test("child_process.spawn resolves an installed binary (sanity)", () => { }); }); +test( + "agent spawn isolates the CLI from Controller's POSIX process group", + { skip: process.platform === "win32" }, + async () => { + const dir = mkdtempSync(path.join(os.tmpdir(), "anita-process-group-")); + const shim = path.join(dir, "anita"); + writeFileSync(shim, "#!/bin/sh\nsleep 30\n"); + chmodSync(shim, 0o755); + + const child = anita!.spawn({ + message: "Hello", + cwd: dir, + env: {}, + command: shim, + model: "test/model", + }); + + try { + assert.ok(child.pid, "spawned agent must have a pid"); + const agentPgid = Number( + execFileSync("ps", ["-o", "pgid=", "-p", String(child.pid)], { + encoding: "utf8", + }).trim() + ); + const controllerPgid = Number( + execFileSync("ps", ["-o", "pgid=", "-p", String(process.pid)], { + encoding: "utf8", + }).trim() + ); + + assert.equal( + agentPgid, + child.pid, + "the agent must lead its own process group" + ); + assert.notEqual( + agentPgid, + controllerPgid, + "the agent process group must not include Controller" + ); + + const closed = new Promise((resolve, reject) => { + child.once("error", reject); + child.once("close", (_code, signal) => resolve(signal)); + }); + assert.equal(signalAgentProcess(child, "SIGTERM"), true); + assert.equal(await closed, "SIGTERM"); + } finally { + if (child.exitCode === null && child.signalCode === null) { + try { + signalAgentProcess(child, "SIGKILL"); + } catch { + // Best-effort cleanup for a failed assertion. + } + } + rmSync(dir, { recursive: true, force: true }); + } + } +); + const codex = getAgentProvider("codex"); assert.ok(codex, "codex provider must be registered"); diff --git a/server/lib/__tests__/process-shutdown.test.ts b/server/lib/__tests__/process-shutdown.test.ts new file mode 100644 index 00000000..9b4e9842 --- /dev/null +++ b/server/lib/__tests__/process-shutdown.test.ts @@ -0,0 +1,44 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { EventEmitter } from "node:events"; +import { installSessionRuntimeShutdownHandlers } from "../process-shutdown.js"; + +function fakeProcess() { + const emitter = new EventEmitter(); + const signals: Array<[number, NodeJS.Signals]> = []; + const target = Object.assign(emitter, { + pid: 1234, + kill(pid: number, signal: NodeJS.Signals) { + signals.push([pid, signal]); + return true; + }, + }) as unknown as Pick< + NodeJS.Process, + "pid" | "once" | "removeAllListeners" | "kill" + >; + return { emitter, signals, target }; +} + +test("normal process exit stops active session runtimes", () => { + const { emitter, target } = fakeProcess(); + let stops = 0; + installSessionRuntimeShutdownHandlers(target, () => ++stops); + + emitter.emit("exit"); + + assert.equal(stops, 1); +}); + +for (const signal of ["SIGINT", "SIGTERM"] as const) { + test(`${signal} stops runtimes and then restores default signal behavior`, () => { + const { emitter, signals, target } = fakeProcess(); + let stops = 0; + installSessionRuntimeShutdownHandlers(target, () => ++stops); + + emitter.emit(signal); + + assert.equal(stops, 1); + assert.equal(emitter.listenerCount(signal), 0); + assert.deepEqual(signals, [[1234, signal]]); + }); +} diff --git a/server/lib/__tests__/session-runtime.test.ts b/server/lib/__tests__/session-runtime.test.ts index 718a8410..12d8eae7 100644 --- a/server/lib/__tests__/session-runtime.test.ts +++ b/server/lib/__tests__/session-runtime.test.ts @@ -7,7 +7,10 @@ import { markSessionInactive, recordSessionAttentionEvent, setSessionAwaitingUserInput, + stopAllSessionRuntimes, } from "../session-runtime.js"; +import { EventEmitter } from "node:events"; +import type { ChildProcess } from "node:child_process"; function runtimeSummary(sessionId: string) { return listSessionRuntimes().find((entry) => entry.sessionId === sessionId); @@ -46,3 +49,22 @@ test("approval attention clears when the response consumes the request", () => { assert.ok(consumePendingApproval(sessionId, "approval-1")); assert.equal(runtimeSummary(sessionId)?.awaitingInput, undefined); }); + +test("shutdown signals every active agent runtime and marks it inactive", () => { + const signals: string[] = []; + const child = Object.assign(new EventEmitter(), { + exitCode: null, + killed: false, + kill(signal: NodeJS.Signals) { + signals.push(signal); + this.killed = true; + return true; + }, + }) as unknown as ChildProcess; + + markSessionActive("runtime-shutdown", { provider: "anita", child }); + + assert.equal(stopAllSessionRuntimes(), 1); + assert.deepEqual(signals, ["SIGTERM"]); + assert.equal(runtimeSummary("runtime-shutdown")?.active, false); +}); diff --git a/server/lib/agents.ts b/server/lib/agents.ts index 7fef4ba1..ca728773 100644 --- a/server/lib/agents.ts +++ b/server/lib/agents.ts @@ -1,4 +1,8 @@ -import { spawn, type ChildProcess } from "node:child_process"; +import { + spawn, + type ChildProcess, + type SpawnOptions as NodeSpawnOptions, +} from "node:child_process"; import { randomUUID } from "node:crypto"; import { resolveCommand, @@ -192,6 +196,62 @@ export interface AgentProvider { export type AgentStreamParseResult = AgentStreamEvent | AgentStreamEvent[] | null; +/* + * Agent CLIs can run arbitrary descendant process trees. Keep those trees in + * a process group that is separate from Controller's Electron/server process: + * a tool runner or test framework may legitimately signal its whole group, + * and without this boundary that signal also terminates Controller. + * + * The WeakSet lets signalAgentProcess distinguish provider children from + * arbitrary ChildProcess instances supplied by tests or other subsystems. + */ +const isolatedAgentProcesses = new WeakSet(); + +function spawnAgentProcess( + command: string, + args: readonly string[], + options: NodeSpawnOptions +): ChildProcess { + const isolateProcessGroup = process.platform !== "win32"; + const child = spawn(command, args, { + ...options, + // POSIX: make the agent the leader of a new session/process group. A + // descendant's kill(0, signal) is then contained to the agent tree. + // Windows has different detached-process semantics, so retain the + // existing direct-child lifecycle there. + detached: isolateProcessGroup, + }); + if (isolateProcessGroup) { + isolatedAgentProcesses.add(child); + } + return child; +} + +/** Signal an agent and all descendants in its isolated POSIX process group. */ +export function signalAgentProcess( + child: ChildProcess, + signal: NodeJS.Signals +): boolean { + if ( + process.platform !== "win32" && + child.pid !== undefined && + isolatedAgentProcesses.has(child) + ) { + try { + process.kill(-child.pid, signal); + return true; + } catch (error) { + // The group may have disappeared between the exitCode check and kill. + // Fall back to ChildProcess.kill so callers retain the old behavior and + // its boolean result for already-exited children. + if ((error as NodeJS.ErrnoException).code !== "ESRCH") { + throw error; + } + } + } + return child.kill(signal); +} + function normalizeToolResultContent(value: unknown): string { if (typeof value === "string") return value; if (value == null) return ""; @@ -600,7 +660,7 @@ const anitaProvider: AgentProvider = { const fullCmd = `anita ${[...cmdArgs, ...args].join(" ")}`; console.log(`[anita] ${fullCmd.slice(0, 100)}...`); - return spawn(command ?? "anita", [...cmdArgs, ...args], { + return spawnAgentProcess(command ?? "anita", [...cmdArgs, ...args], { cwd, env: childProcessEnv(env), stdio: ["pipe", "pipe", "pipe"], @@ -669,7 +729,7 @@ const codexProvider: AgentProvider = { const fullCmd = `codex ${args.join(" ")}`; console.log(`[codex] ${fullCmd.slice(0, 100)}...`); - return spawn(command ?? "codex", args, { + return spawnAgentProcess(command ?? "codex", args, { cwd, env: childProcessEnv(env), stdio: ["pipe", "pipe", "pipe"], @@ -757,7 +817,7 @@ const claudeProvider: AgentProvider = { const fullCmd = `claude ${args.join(" ")}`; console.log(`[claude] ${fullCmd.slice(0, 100)}...`); - const child = spawn(command ?? "claude", args, { + const child = spawnAgentProcess(command ?? "claude", args, { cwd, env: childProcessEnv(env), // Control-channel turns keep stdin open for the live approval channel; diff --git a/server/lib/process-shutdown.ts b/server/lib/process-shutdown.ts new file mode 100644 index 00000000..d14c748f --- /dev/null +++ b/server/lib/process-shutdown.ts @@ -0,0 +1,30 @@ +import { stopAllSessionRuntimes } from "./session-runtime.js"; + +type ShutdownProcess = Pick< + NodeJS.Process, + "pid" | "once" | "removeAllListeners" | "kill" +>; + +const TERMINATION_SIGNALS = ["SIGINT", "SIGTERM"] as const; + +/** + * Reap isolated agent process groups on both orderly exits and termination + * signals. Installing a signal listener replaces Node's default termination + * behavior, so remove it and re-send the same signal after cleanup. + */ +export function installSessionRuntimeShutdownHandlers( + target: ShutdownProcess = process, + stopRuntimes: () => number = stopAllSessionRuntimes +): void { + target.once("exit", () => { + stopRuntimes(); + }); + + for (const signal of TERMINATION_SIGNALS) { + target.once(signal, () => { + stopRuntimes(); + target.removeAllListeners(signal); + target.kill(target.pid, signal); + }); + } +} diff --git a/server/lib/session-runtime.ts b/server/lib/session-runtime.ts index d3c62ad2..0d763a1c 100644 --- a/server/lib/session-runtime.ts +++ b/server/lib/session-runtime.ts @@ -1,5 +1,9 @@ import type { ChildProcess } from "node:child_process"; -import type { AgentStreamEvent, ClaudeApprovalRequest } from "./agents.js"; +import { + signalAgentProcess, + type AgentStreamEvent, + type ClaudeApprovalRequest, +} from "./agents.js"; export interface SessionRuntimeMetadata { projectId: string; @@ -170,6 +174,27 @@ export function listSessionRuntimes( return summaries; } +/** + * Best-effort synchronous shutdown hook for the Electron/server host. Agent + * processes live in isolated groups, so they no longer receive Controller's + * own termination signal implicitly and must be reaped explicitly. + */ +export function stopAllSessionRuntimes(): number { + let stopped = 0; + for (const [sessionId, runtime] of runtimes) { + const child = runtime.child; + if (!runtime.active || !child || child.exitCode !== null) continue; + try { + if (signalAgentProcess(child, "SIGTERM")) stopped += 1; + } catch { + // Process exit cleanup cannot recover or await; continue reaping the + // remaining agent groups. + } + markSessionInactive(sessionId); + } + return stopped; +} + export async function stopSessionRuntime(sessionId: string): Promise { const runtime = runtimes.get(sessionId); if (!runtime?.active) { @@ -210,14 +235,14 @@ export async function stopSessionRuntime(sessionId: string): Promise { const forceKillTimer = setTimeout(() => { if (child.exitCode === null) { - child.kill("SIGKILL"); + signalAgentProcess(child, "SIGKILL"); } }, 2000); child.once("exit", onExit); child.once("error", onError); - const signalled = child.kill("SIGINT"); + const signalled = signalAgentProcess(child, "SIGINT"); if (!signalled) { finish(); } diff --git a/server/routes/sessions.ts b/server/routes/sessions.ts index 8934cc17..502eaf76 100644 --- a/server/routes/sessions.ts +++ b/server/routes/sessions.ts @@ -43,6 +43,7 @@ import { resolveAgentCommand, sendAnitaApprovalDecision, sendClaudeApprovalDecision, + signalAgentProcess, type AgentStreamEvent, type ClaudeApprovalDecision, type ClaudeApprovalRequest, @@ -1350,9 +1351,9 @@ export async function handleSessionStream( // `run.cancelled` we already streamed (issue #94). if (runCancelled) { if (child.exitCode === null && !child.killed) { - child.kill("SIGTERM"); + signalAgentProcess(child, "SIGTERM"); setTimeout(() => { - if (child.exitCode === null) child.kill("SIGKILL"); + if (child.exitCode === null) signalAgentProcess(child, "SIGKILL"); }, 2000); } return; @@ -1372,9 +1373,9 @@ export async function handleSessionStream( .then(() => persistAgentEvent(failureEvent)) .catch(() => {}); if (child.exitCode === null && !child.killed) { - child.kill("SIGTERM"); + signalAgentProcess(child, "SIGTERM"); setTimeout(() => { - if (child.exitCode === null) child.kill("SIGKILL"); + if (child.exitCode === null) signalAgentProcess(child, "SIGKILL"); }, 2000); } } @@ -1463,7 +1464,7 @@ export async function handleSessionStream( .catch(() => {}); if (providerId === "claude" && event.type === "user.input_requested") { pausedForClaudeUserInput = true; - child.kill("SIGTERM"); + signalAgentProcess(child, "SIGTERM"); } // Stand the watchdog down while an approval is pending, and re-arm // it the moment Claude resumes with any other event. Done