Compare commits
2 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| 17b610e1a1 | |||
| ca913c76ea |
@@ -407,6 +407,12 @@ async function runClaude(params: RunParams): Promise<ClaudeRunResult> {
|
|||||||
activityTimeout: 300_000,
|
activityTimeout: 300_000,
|
||||||
onActivityTimeout: params.onActivityTimeout,
|
onActivityTimeout: params.onActivityTimeout,
|
||||||
stdio: ["ignore", "pipe", "pipe"],
|
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) => {
|
onStdout: async (chunk) => {
|
||||||
const text = chunk.toString();
|
const text = chunk.toString();
|
||||||
output += text;
|
output += text;
|
||||||
|
|||||||
@@ -368,6 +368,16 @@ async function runOpenCode(params: RunParams): Promise<AgentResult> {
|
|||||||
// mismatched callID" signal).
|
// mismatched callID" signal).
|
||||||
const knownNonTaskCallIDs = new Set<string>();
|
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(
|
function emitSubagentFinished(
|
||||||
dispatch: TaskDispatch,
|
dispatch: TaskDispatch,
|
||||||
status: string,
|
status: string,
|
||||||
@@ -709,6 +719,20 @@ async function runOpenCode(params: RunParams): Promise<AgentResult> {
|
|||||||
activityTimeout: 300_000,
|
activityTimeout: 300_000,
|
||||||
onActivityTimeout: params.onActivityTimeout,
|
onActivityTimeout: params.onActivityTimeout,
|
||||||
stdio: ["ignore", "pipe", "pipe"],
|
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) => {
|
onStdout: async (chunk) => {
|
||||||
const text = chunk.toString();
|
const text = chunk.toString();
|
||||||
output += text;
|
output += text;
|
||||||
|
|||||||
+1
-1
@@ -1,6 +1,6 @@
|
|||||||
{
|
{
|
||||||
"name": "pullfrog",
|
"name": "pullfrog",
|
||||||
"version": "0.1.0",
|
"version": "0.1.1",
|
||||||
"type": "module",
|
"type": "module",
|
||||||
"bin": {
|
"bin": {
|
||||||
"pullfrog": "dist/cli.mjs",
|
"pullfrog": "dist/cli.mjs",
|
||||||
|
|||||||
@@ -1,3 +1,4 @@
|
|||||||
|
import { performance } from "node:perf_hooks";
|
||||||
import { describe, expect, it } from "vitest";
|
import { describe, expect, it } from "vitest";
|
||||||
import { spawn } from "./subprocess.ts";
|
import { spawn } from "./subprocess.ts";
|
||||||
|
|
||||||
@@ -48,6 +49,79 @@ describe("spawn error path", () => {
|
|||||||
expect(afterHandles).toBeLessThanOrEqual(beforeHandles);
|
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 () => {
|
it("reports signal-killed subprocesses as failures, not success", async () => {
|
||||||
// regression: before the fix, `child.on("close", (exitCode) => ...)`
|
// regression: before the fix, `child.on("close", (exitCode) => ...)`
|
||||||
// discarded the signal parameter and `exitCode || 0` coerced the
|
// discarded the signal parameter and `exitCode || 0` coerced the
|
||||||
|
|||||||
+54
-5
@@ -106,6 +106,25 @@ export interface SpawnOptions {
|
|||||||
stdio?: ("pipe" | "ignore" | "inherit")[];
|
stdio?: ("pipe" | "ignore" | "inherit")[];
|
||||||
onStdout?: (chunk: string) => void;
|
onStdout?: (chunk: string) => void;
|
||||||
onStderr?: (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 {
|
export interface SpawnResult {
|
||||||
@@ -127,6 +146,8 @@ export async function spawn(options: SpawnOptions): Promise<SpawnResult> {
|
|||||||
let stdoutBuffer = "";
|
let stdoutBuffer = "";
|
||||||
let stderrBuffer = "";
|
let stderrBuffer = "";
|
||||||
|
|
||||||
|
const killGroup = options.killGroup ?? false;
|
||||||
|
|
||||||
return new Promise((resolve, reject) => {
|
return new Promise((resolve, reject) => {
|
||||||
// security: caller must provide complete env object, not merged with process.env
|
// security: caller must provide complete env object, not merged with process.env
|
||||||
const child = nodeSpawn(options.cmd, options.args, {
|
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"],
|
stdio: options.stdio || ["pipe", "pipe", "pipe"],
|
||||||
cwd: options.cwd || process.cwd(),
|
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
|
// track child for cleanup on Ctrl+C
|
||||||
trackChild({ child });
|
trackChild({ child, killGroup });
|
||||||
|
|
||||||
let timeoutId: NodeJS.Timeout | undefined;
|
let timeoutId: NodeJS.Timeout | undefined;
|
||||||
let sigkillEscalatorId: NodeJS.Timeout | undefined;
|
let sigkillEscalatorId: NodeJS.Timeout | undefined;
|
||||||
@@ -157,7 +196,7 @@ export async function spawn(options: SpawnOptions): Promise<SpawnResult> {
|
|||||||
if (options.timeout) {
|
if (options.timeout) {
|
||||||
timeoutId = setTimeout(() => {
|
timeoutId = setTimeout(() => {
|
||||||
isTimedOut = true;
|
isTimedOut = true;
|
||||||
child.kill("SIGTERM");
|
killSelf("SIGTERM");
|
||||||
|
|
||||||
// track the escalator so a graceful SIGTERM response (close fires
|
// track the escalator so a graceful SIGTERM response (close fires
|
||||||
// before the 5s elapses) can clear it. without capture, this timer
|
// 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.
|
// past a timed-out subprocess's clean exit.
|
||||||
sigkillEscalatorId = setTimeout(() => {
|
sigkillEscalatorId = setTimeout(() => {
|
||||||
if (!child.killed) {
|
if (!child.killed) {
|
||||||
child.kill("SIGKILL");
|
killSelf("SIGKILL");
|
||||||
}
|
}
|
||||||
}, 5000);
|
}, 5000);
|
||||||
}, options.timeout);
|
}, 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`
|
`spawn activity timer: pid=${child.pid} cmd=${options.cmd} timeout=${activityTimeoutMs}ms`
|
||||||
);
|
);
|
||||||
activityCheckIntervalId = setInterval(() => {
|
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;
|
const idleMs = performance.now() - lastActivityTime;
|
||||||
log.debug(
|
log.debug(
|
||||||
`spawn activity check: pid=${child.pid} idle=${Math.round(idleMs)}ms / ${activityTimeoutMs}ms`
|
`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;
|
killedAtIdleMs = idleMs;
|
||||||
const idleSec = Math.round(idleMs / 1000);
|
const idleSec = Math.round(idleMs / 1000);
|
||||||
log.info(
|
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);
|
clearInterval(activityCheckIntervalId);
|
||||||
try {
|
try {
|
||||||
options.onActivityTimeout?.();
|
options.onActivityTimeout?.();
|
||||||
|
|||||||
Reference in New Issue
Block a user