diff --git a/agents/claude.ts b/agents/claude.ts index 225bca5..45d3a0e 100644 --- a/agents/claude.ts +++ b/agents/claude.ts @@ -407,6 +407,12 @@ async function runClaude(params: RunParams): Promise { activityTimeout: 300_000, onActivityTimeout: params.onActivityTimeout, stdio: ["ignore", "pipe", "pipe"], + // run claude in its own process group so SIGKILL on activity timeout / + // outer cancellation reaches any subprocesses it spawns (rg, file + // watchers, mcp transports, etc). claude itself is a node bundle so + // there's no shim-orphan issue like opencode-ai/bin/opencode, but + // detached + killGroup is the right default for any agent runtime. + killGroup: true, onStdout: async (chunk) => { const text = chunk.toString(); output += text; diff --git a/agents/opencode.ts b/agents/opencode.ts index a59aa70..9779414 100644 --- a/agents/opencode.ts +++ b/agents/opencode.ts @@ -368,6 +368,16 @@ async function runOpenCode(params: RunParams): Promise { // mismatched callID" signal). const knownNonTaskCallIDs = new Set(); + // a subagent is "in flight" while at least one `task` tool_use is awaiting + // its tool_result. used to suspend spawn's activity timer (which only sees + // opencode's stdout) so a long-running subagent — whose internal events + // don't surface on the parent NDJSON stream — can't trip the 5min timeout. + // boolean state instead of a heartbeat so there's no race window between a + // periodic tick and a subagent that finishes between ticks. + function isSubagentInFlight(): boolean { + return taskDispatchByCallID.size > 0 || pendingTaskDispatches.length > 0; + } + function emitSubagentFinished( dispatch: TaskDispatch, status: string, @@ -709,6 +719,20 @@ async function runOpenCode(params: RunParams): Promise { activityTimeout: 300_000, onActivityTimeout: params.onActivityTimeout, stdio: ["ignore", "pipe", "pipe"], + // node_modules/opencode-ai/bin/opencode is a Node shim that spawnSyncs + // the native opencode-- binary with stdio:"inherit". without + // a process-group kill, SIGKILL hits only the shim, the native binary + // is reparented to PID 1, holds our stdout pipe open, and `child.close` + // never fires — producing zombie runs. detached + killGroup nukes the + // whole tree. + killGroup: true, + // suspend the inner activity timer while a `task` subagent is in flight. + // opencode's task tool encapsulates subagent execution in-process — the + // subagent's internal events don't surface on the parent NDJSON stream, + // so without this the 5min timeout would falsely fire mid-subagent. + // suspend/resume is preferable to a heartbeat because there's no race + // between a periodic tick and a subagent finishing between ticks. + isPausedExternally: isSubagentInFlight, onStdout: async (chunk) => { const text = chunk.toString(); output += text; diff --git a/utils/subprocess.test.ts b/utils/subprocess.test.ts index 9eb4686..a26c8b8 100644 --- a/utils/subprocess.test.ts +++ b/utils/subprocess.test.ts @@ -1,3 +1,4 @@ +import { performance } from "node:perf_hooks"; import { describe, expect, it } from "vitest"; import { spawn } from "./subprocess.ts"; @@ -48,6 +49,79 @@ describe("spawn error path", () => { expect(afterHandles).toBeLessThanOrEqual(beforeHandles); }); + it("killGroup: true propagates SIGKILL to grandchildren so close fires promptly", async () => { + // regression: node_modules/opencode-ai/bin/opencode is a Node shim that + // spawnSyncs the native binary with stdio:"inherit". without killGroup, + // child.kill("SIGKILL") hit only the shim — the native binary was + // reparented to PID 1, kept holding our stdout pipe via the inherited + // fds, and `child.on("close")` never fired (because pipes stayed open). + // a 5-min outer safety-net timer eventually rejected the agent promise, + // but the grandchild kept running until the GitHub Actions job-level + // timeout. this test replicates the shape with bash + a backgrounded + // sleep grandchild: with killGroup, close fires promptly after SIGKILL; + // without it, the parent would wait for sleep to exit (30s). + // + // the activity-check interval is fixed at 5s so the earliest the kill + // can fire is ~5s after start. budget 15s end-to-end. + const before = performance.now(); + const result = await spawn({ + cmd: "bash", + args: ["-c", "sleep 30 & wait"], + env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "" }, + activityTimeout: 1000, + killGroup: true, + }).catch((err) => err); + const elapsed = performance.now() - before; + + expect(result).toBeInstanceOf(Error); + // 10s ceiling: 5s activity-check tick + signal delivery. a regression + // here (no killGroup) would hang for the full 30s sleep. + expect(elapsed).toBeLessThan(10_000); + }, 20_000); + + it("isPausedExternally suspends the activity timer while it returns true", async () => { + // opencode's `task` tool encapsulates subagent execution in-process — + // subagent-internal events don't reach our parent NDJSON stream. this + // means the child's stdout falls silent for the full subagent duration + // even when real work is happening. opencode passes a predicate keyed + // off its in-flight task dispatch tracker; while it returns true, the + // activity timer must skip the kill decision and reset its baseline. + const result = await spawn({ + cmd: "bash", + args: ["-c", "sleep 8; echo done"], + env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "" }, + activityTimeout: 1000, + isPausedExternally: () => true, + }); + + expect(result.exitCode).toBe(0); + expect(result.stdout).toContain("done"); + }, 20_000); + + it("isPausedExternally fires normally once it returns false again", async () => { + // verifies suspend/resume rather than unconditional bypass: the predicate + // flips to false after a few hundred ms, so the timer should resume from + // a fresh baseline (advanced during the paused window) and fire after + // activityTimeout idle time. critical that lastActivityTime is reset + // while paused — otherwise the resume tick would see a stale baseline + // and kill instantly. + let paused = true; + setTimeout(() => { + paused = false; + }, 500); + + const result = await spawn({ + cmd: "sleep", + args: ["30"], + env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "" }, + activityTimeout: 1000, + isPausedExternally: () => paused, + }).catch((err) => err); + + expect(result).toBeInstanceOf(Error); + expect(String(result)).toMatch(/activity timeout/i); + }, 20_000); + it("reports signal-killed subprocesses as failures, not success", async () => { // regression: before the fix, `child.on("close", (exitCode) => ...)` // discarded the signal parameter and `exitCode || 0` coerced the diff --git a/utils/subprocess.ts b/utils/subprocess.ts index 868eb0a..8654617 100644 --- a/utils/subprocess.ts +++ b/utils/subprocess.ts @@ -106,6 +106,25 @@ export interface SpawnOptions { stdio?: ("pipe" | "ignore" | "inherit")[]; onStdout?: (chunk: string) => void; onStderr?: (chunk: string) => void; + // when true, spawn the child detached (its own process group) and route all + // kill paths (timeout, activity timeout, ctrl-c) through `process.kill(-pid, ...)` + // so signals reach grandchildren too. critical for binaries that fork through + // a shim (e.g. node_modules/opencode-ai/bin/opencode is a Node shim that + // spawnSync's the native binary; without killGroup, SIGKILL only hits the + // shim and the native binary is reparented to PID 1, holds our stdout pipe + // open, keeps emitting NDJSON, and `child.on("close")` never fires — + // producing zombie runs that hang until the GitHub Actions job timeout). + killGroup?: boolean; + // optional pause predicate consulted on every activity check. when it + // returns true the activity timer skips the kill decision *and* resets + // its idle baseline so a clean unpause can't immediately fire on a stale + // lastActivityTime. opencode uses this because its `task` tool encapsulates + // subagent execution in-process — subagent-internal events don't surface + // on the parent's NDJSON stream, so the local stdout-only signal would + // falsely fire mid-subagent. preferred over fake-activity heartbeats + // because there's no race window between a heartbeat tick and a subagent + // that finishes between ticks. + isPausedExternally?: () => boolean; } export interface SpawnResult { @@ -127,6 +146,8 @@ export async function spawn(options: SpawnOptions): Promise { let stdoutBuffer = ""; let stderrBuffer = ""; + const killGroup = options.killGroup ?? false; + return new Promise((resolve, reject) => { // security: caller must provide complete env object, not merged with process.env const child = nodeSpawn(options.cmd, options.args, { @@ -136,10 +157,28 @@ export async function spawn(options: SpawnOptions): Promise { }, stdio: options.stdio || ["pipe", "pipe", "pipe"], cwd: options.cwd || process.cwd(), + detached: killGroup, }); + // sends `signal` to the entire process group when killGroup is set, so + // grandchildren (e.g. the native opencode binary spawned by the + // opencode-ai Node shim) die with the parent. falls back to a direct + // child kill if the process-group send fails (common when the child + // already exited or was never made a process group leader). + const killSelf = (signal: NodeJS.Signals): void => { + if (killGroup && child.pid) { + try { + process.kill(-child.pid, signal); + return; + } catch { + // fall through to direct kill + } + } + child.kill(signal); + }; + // track child for cleanup on Ctrl+C - trackChild({ child }); + trackChild({ child, killGroup }); let timeoutId: NodeJS.Timeout | undefined; let sigkillEscalatorId: NodeJS.Timeout | undefined; @@ -157,7 +196,7 @@ export async function spawn(options: SpawnOptions): Promise { if (options.timeout) { timeoutId = setTimeout(() => { isTimedOut = true; - child.kill("SIGTERM"); + killSelf("SIGTERM"); // track the escalator so a graceful SIGTERM response (close fires // before the 5s elapses) can clear it. without capture, this timer @@ -165,7 +204,7 @@ export async function spawn(options: SpawnOptions): Promise { // past a timed-out subprocess's clean exit. sigkillEscalatorId = setTimeout(() => { if (!child.killed) { - child.kill("SIGKILL"); + killSelf("SIGKILL"); } }, 5000); }, options.timeout); @@ -177,6 +216,16 @@ export async function spawn(options: SpawnOptions): Promise { `spawn activity timer: pid=${child.pid} cmd=${options.cmd} timeout=${activityTimeoutMs}ms` ); activityCheckIntervalId = setInterval(() => { + // when an external pause predicate says we're suspended (e.g. + // opencode in a long-running task subagent whose internal events + // don't surface on stdout), advance lastActivityTime to "now" so a + // clean unpause doesn't immediately fire on a stale baseline, and + // skip the kill decision for this tick. + if (options.isPausedExternally?.()) { + lastActivityTime = performance.now(); + log.debug(`spawn activity check: pid=${child.pid} paused externally`); + return; + } const idleMs = performance.now() - lastActivityTime; log.debug( `spawn activity check: pid=${child.pid} idle=${Math.round(idleMs)}ms / ${activityTimeoutMs}ms` @@ -186,9 +235,9 @@ export async function spawn(options: SpawnOptions): Promise { killedAtIdleMs = idleMs; const idleSec = Math.round(idleMs / 1000); log.info( - `no output for ${idleSec}s from pid=${child.pid} (${options.cmd}), killing process` + `no output for ${idleSec}s from pid=${child.pid} (${options.cmd}), killing process${killGroup ? " group" : ""}` ); - child.kill("SIGKILL"); + killSelf("SIGKILL"); clearInterval(activityCheckIntervalId); try { options.onActivityTimeout?.();