Compare commits
2 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| 17b610e1a1 | |||
| ca913c76ea |
@@ -407,6 +407,12 @@ async function runClaude(params: RunParams): Promise<ClaudeRunResult> {
|
||||
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;
|
||||
|
||||
@@ -368,6 +368,16 @@ async function runOpenCode(params: RunParams): Promise<AgentResult> {
|
||||
// mismatched callID" signal).
|
||||
const knownNonTaskCallIDs = new Set<string>();
|
||||
|
||||
// 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<AgentResult> {
|
||||
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-<plat>-<arch> 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;
|
||||
|
||||
+1
-1
@@ -1,6 +1,6 @@
|
||||
{
|
||||
"name": "pullfrog",
|
||||
"version": "0.1.0",
|
||||
"version": "0.1.1",
|
||||
"type": "module",
|
||||
"bin": {
|
||||
"pullfrog": "dist/cli.mjs",
|
||||
|
||||
@@ -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
|
||||
|
||||
+54
-5
@@ -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<SpawnResult> {
|
||||
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<SpawnResult> {
|
||||
},
|
||||
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<SpawnResult> {
|
||||
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<SpawnResult> {
|
||||
// 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<SpawnResult> {
|
||||
`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<SpawnResult> {
|
||||
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?.();
|
||||
|
||||
Reference in New Issue
Block a user