diff --git a/src/flows/executors/shell.ts b/src/flows/executors/shell.ts index f4d322f7..0f3ef035 100644 --- a/src/flows/executors/shell.ts +++ b/src/flows/executors/shell.ts @@ -1,7 +1,16 @@ -import { spawn } from "node:child_process"; -import { TimeoutError } from "../../async-control.js"; +import { spawn, type ChildProcess, type ChildProcessWithoutNullStreams } from "node:child_process"; +import { InterruptedError, TimeoutError } from "../../async-control.js"; import type { ShellActionExecution, ShellActionResult } from "../runtime.js"; +const SHELL_CHILD_KILL_GRACE_MS = 1_000; + +function terminateShellChild(child: ChildProcess): void { + child.kill("SIGTERM"); + setTimeout(() => { + child.kill("SIGKILL"); + }, SHELL_CHILD_KILL_GRACE_MS).unref(); +} + export function formatShellActionSummary(spec: ShellActionExecution): string { return `shell: ${renderShellCommand(spec.command, spec.args ?? [])}`; } @@ -40,11 +49,35 @@ function rejectIfShellFailed( return undefined; } -export async function runShellAction(spec: ShellActionExecution): Promise { - const cwd = spec.cwd ?? process.cwd(); - const args = spec.args ?? []; - const startMs = Date.now(); - const child = spawn(spec.command, args, { +function settleShellResult( + spec: ShellActionExecution, + args: string[], + result: ShellActionResult, + timedOut: boolean, + aborted: boolean, +): Error | undefined { + if (aborted && !timedOut) { + return new InterruptedError(); + } + return rejectIfShellFailed(spec, args, result, timedOut); +} + +function scheduleShellTimeout( + timeoutMs: number | undefined, + onTimeout: () => void, +): NodeJS.Timeout | undefined { + if (timeoutMs == null || timeoutMs <= 0) { + return undefined; + } + return setTimeout(onTimeout, timeoutMs); +} + +function spawnShellChild( + spec: ShellActionExecution, + cwd: string, + args: string[], +): ChildProcessWithoutNullStreams { + return spawn(spec.command, args, { cwd, env: { ...process.env, @@ -54,13 +87,56 @@ export async function runShellAction(spec: ShellActionExecution): Promise boolean; dispose: () => void } { + let aborted = false; + const onAbort = () => { + if (aborted) { + return; + } + aborted = true; + terminateShellChild(child); + }; + if (!signal) { + return { + isAborted: () => aborted, + dispose: () => undefined, + }; + } + signal.addEventListener("abort", onAbort, { once: true }); + if (signal.aborted) { + onAbort(); + } + return { + isAborted: () => aborted, + dispose: () => { + signal.removeEventListener("abort", onAbort); + }, + }; +} +function waitForShellExit( + child: ChildProcessWithoutNullStreams, + spec: ShellActionExecution, + args: string[], + cwd: string, + startMs: number, + getFlags: () => { timedOut: boolean; aborted: boolean }, +): Promise { let stdout = ""; let stderr = ""; - let timedOut = false; - let timeout: NodeJS.Timeout | undefined; - - const finish = new Promise((resolve, reject) => { + return new Promise((resolve, reject) => { child.stdout.setEncoding("utf8"); child.stderr.setEncoding("utf8"); child.stdout.on("data", (chunk: string) => { @@ -69,9 +145,9 @@ export async function runShellAction(spec: ShellActionExecution): Promise { stderr += chunk; }); - child.once("error", reject); - child.once("exit", (exitCode, signal) => { + child.once("exit", (exitCode, exitSignal) => { + const flags = getFlags(); const result: ShellActionResult = { command: spec.command, args, @@ -80,34 +156,38 @@ export async function runShellAction(spec: ShellActionExecution): Promise 0) { - timeout = setTimeout(() => { - timedOut = true; - child.kill("SIGTERM"); - setTimeout(() => { - child.kill("SIGKILL"); - }, 1_000).unref(); - }, spec.timeoutMs); - } +async function runSpawnedShellAction( + spec: ShellActionExecution, + signal: AbortSignal | undefined, +): Promise { + const cwd = spec.cwd ?? process.cwd(); + const args = spec.args ?? []; + const startMs = Date.now(); + const child = spawnShellChild(spec, cwd, args); + let timedOut = false; + const abort = bindShellAbort(signal, child); + const finish = waitForShellExit(child, spec, args, cwd, startMs, () => ({ + timedOut, + aborted: abort.isAborted(), + })); + writeShellStdin(child, spec.stdin); + const timeout = scheduleShellTimeout(spec.timeoutMs, () => { + timedOut = true; + terminateShellChild(child); + }); try { return await finish; @@ -115,5 +195,16 @@ export async function runShellAction(spec: ShellActionExecution): Promise { + if (options?.signal?.aborted) { + throw new InterruptedError(); } + return await runSpawnedShellAction(spec, options?.signal); } diff --git a/src/flows/runtime-support.ts b/src/flows/runtime-support.ts index 36a766f3..5b3498ec 100644 --- a/src/flows/runtime-support.ts +++ b/src/flows/runtime-support.ts @@ -28,16 +28,19 @@ export function isoNow(): string { return new Date().toISOString(); } +export function isRunFailureFinalized(state: FlowRunState): boolean { + return ( + state.finishedAt !== undefined && (state.status === "failed" || state.status === "timed_out") + ); +} + export function persistRunFailure( store: FlowRunStore, runDir: string, state: FlowRunState, error: unknown, ): Promise { - if ( - state.finishedAt !== undefined && - (state.status === "failed" || state.status === "timed_out") - ) { + if (isRunFailureFinalized(state)) { return Promise.resolve(); } diff --git a/src/flows/runtime.ts b/src/flows/runtime.ts index 4d08366a..af3bb664 100644 --- a/src/flows/runtime.ts +++ b/src/flows/runtime.ts @@ -36,6 +36,7 @@ import { finalizeStepTrace, findConversationDeltaStart, isoNow, + isRunFailureFinalized, makeFlowNodeContext, markNodeStarted, nextAttemptId, @@ -67,6 +68,7 @@ import type { FlowNodeResult, ResolvedFlowAgent, ShellActionExecution, + ShellActionNodeDefinition, } from "./types.js"; export { acp, action, checkpoint, compute, defineFlow, shell }; @@ -160,6 +162,7 @@ export class FlowRunner { private readonly services; private readonly store; private readonly pendingPersistentSessionClients = new Map(); + private activeRunAbort: AbortController | null = null; constructor(options: FlowRunnerOptions) { this.resolveAgent = options.resolveAgent; @@ -216,14 +219,23 @@ export class FlowRunner { inputArtifact, }); + const runAbort = new AbortController(); + this.activeRunAbort = runAbort; try { return await withInterrupt( async () => await this.executeFlowRun(flow, input, runDir, state), async () => { - await persistRunFailure(this.store, runDir, state, new InterruptedError()); + try { + await persistRunFailure(this.store, runDir, state, new InterruptedError()); + } finally { + runAbort.abort(); + } }, ); } finally { + if (this.activeRunAbort === runAbort) { + this.activeRunAbort = null; + } await this.closePendingPersistentSessionClients(); } } @@ -323,8 +335,14 @@ export class FlowRunner { params.node, context, ); + if (isRunFailureFinalized(params.state)) { + throw new InterruptedError(); + } return await this.createSuccessfulFlowStep(params, executed); } catch (error) { + if (isRunFailureFinalized(params.state)) { + throw error; + } return await this.createFailedFlowStep(params, error); } } @@ -478,6 +496,9 @@ export class FlowRunner { statusDetail?: string; } = {}, ): Promise { + if (isRunFailureFinalized(state)) { + return; + } state.updatedAt = isoNow(); clearActiveNode(state, overrides.statusDetail); state.steps.push({ @@ -583,111 +604,162 @@ export class FlowRunner { }; } - const { output, rawText, trace } = await this.runWithHeartbeat( - runDir, - state, - state.currentNode ?? "", - node, - nodeTimeoutMs, - async () => { - const execution = await Promise.resolve(node.exec(context)); - const effectiveExecution: ShellActionExecution = { - ...execution, - cwd: resolveShellActionCwd(this.defaultCwd, execution.cwd), - timeoutMs: execution.timeoutMs ?? nodeTimeoutMs, - }; - updateStatusDetail(state, formatShellActionSummary(effectiveExecution)); - await this.store.writeLive(runDir, state, { - scope: "node", - type: "node_heartbeat", - nodeId: state.currentNode, - attemptId: state.currentAttemptId, - payload: { - statusDetail: state.statusDetail, - }, - }); - await this.store.appendTrace(runDir, state, { - scope: "action", - type: "action_prepared", - nodeId: state.currentNode, - attemptId: state.currentAttemptId, - payload: { - action: { - actionType: "shell", - command: effectiveExecution.command, - args: effectiveExecution.args ?? [], - cwd: effectiveExecution.cwd, - }, - }, - }); - const result = await runShellAction(effectiveExecution); - const stdoutArtifact = await this.store.writeArtifact(runDir, state, result.stdout, { - mediaType: "text/plain", - extension: "txt", - nodeId: state.currentNode, - attemptId: state.currentAttemptId, - }); - const stderrArtifact = await this.store.writeArtifact(runDir, state, result.stderr, { - mediaType: "text/plain", - extension: "txt", - nodeId: state.currentNode, - attemptId: state.currentAttemptId, - }); - await this.store.appendTrace(runDir, state, { - scope: "action", - type: "action_completed", - nodeId: state.currentNode, - attemptId: state.currentAttemptId, - payload: { - action: { - actionType: "shell", - command: result.command, - args: result.args, - cwd: result.cwd, - exitCode: result.exitCode, - signal: result.signal, - durationMs: result.durationMs, - }, - stdoutArtifact, - stderrArtifact, - }, - }); - const trace: FlowStepTrace = { - action: { - actionType: "shell", - command: result.command, - args: result.args, - cwd: result.cwd, - exitCode: result.exitCode, - signal: result.signal, - durationMs: result.durationMs, - }, - stdoutArtifact, - stderrArtifact, - }; - let parsedOutput: unknown; - try { - parsedOutput = node.parse ? await node.parse(result, context) : result; - } catch (error) { - throw attachStepTrace(error, trace); - } - return { - output: parsedOutput, - rawText: result.combinedOutput, - trace, - }; + const shellAbort = this.createShellActionAbort(); + try { + const { output, rawText, trace } = await this.runWithHeartbeat( + runDir, + state, + state.currentNode ?? "", + node, + nodeTimeoutMs, + async () => + await this.executeShellAction( + runDir, + state, + node, + context, + nodeTimeoutMs, + shellAbort.signal, + ), + async () => { + shellAbort.abort(); + }, + ); + return { + output, + promptText: null, + rawText, + sessionInfo: null, + agentInfo: null, + trace, + }; + } finally { + shellAbort.dispose(); + } + } + + private async executeShellAction( + runDir: string, + state: FlowRunState, + node: ShellActionNodeDefinition, + context: FlowNodeContext, + nodeTimeoutMs: number | undefined, + signal: AbortSignal, + ): Promise<{ output: unknown; rawText: string; trace: FlowStepTrace }> { + const execution = await Promise.resolve(node.exec(context)); + const effectiveExecution: ShellActionExecution = { + ...execution, + cwd: resolveShellActionCwd(this.defaultCwd, execution.cwd), + timeoutMs: execution.timeoutMs ?? nodeTimeoutMs, + }; + updateStatusDetail(state, formatShellActionSummary(effectiveExecution)); + await this.store.writeLive(runDir, state, { + scope: "node", + type: "node_heartbeat", + nodeId: state.currentNode, + attemptId: state.currentAttemptId, + payload: { + statusDetail: state.statusDetail, }, - ); + }); + await this.store.appendTrace(runDir, state, { + scope: "action", + type: "action_prepared", + nodeId: state.currentNode, + attemptId: state.currentAttemptId, + payload: { + action: { + actionType: "shell", + command: effectiveExecution.command, + args: effectiveExecution.args ?? [], + cwd: effectiveExecution.cwd, + }, + }, + }); + const result = await runShellAction(effectiveExecution, { signal }); + const stdoutArtifact = await this.store.writeArtifact(runDir, state, result.stdout, { + mediaType: "text/plain", + extension: "txt", + nodeId: state.currentNode, + attemptId: state.currentAttemptId, + }); + const stderrArtifact = await this.store.writeArtifact(runDir, state, result.stderr, { + mediaType: "text/plain", + extension: "txt", + nodeId: state.currentNode, + attemptId: state.currentAttemptId, + }); + await this.store.appendTrace(runDir, state, { + scope: "action", + type: "action_completed", + nodeId: state.currentNode, + attemptId: state.currentAttemptId, + payload: { + action: { + actionType: "shell", + command: result.command, + args: result.args, + cwd: result.cwd, + exitCode: result.exitCode, + signal: result.signal, + durationMs: result.durationMs, + }, + stdoutArtifact, + stderrArtifact, + }, + }); + const trace: FlowStepTrace = { + action: { + actionType: "shell", + command: result.command, + args: result.args, + cwd: result.cwd, + exitCode: result.exitCode, + signal: result.signal, + durationMs: result.durationMs, + }, + stdoutArtifact, + stderrArtifact, + }; + let parsedOutput: unknown; + try { + parsedOutput = node.parse ? await node.parse(result, context) : result; + } catch (error) { + throw attachStepTrace(error, trace); + } return { - output, - promptText: null, - rawText, - sessionInfo: null, - agentInfo: null, + output: parsedOutput, + rawText: result.combinedOutput, trace, }; } + private createShellActionAbort(): { + signal: AbortSignal; + abort: () => void; + dispose: () => void; + } { + const controller = new AbortController(); + const parent = this.activeRunAbort?.signal; + const onParentAbort = () => { + controller.abort(); + }; + if (parent?.aborted) { + controller.abort(); + } else { + parent?.addEventListener("abort", onParentAbort, { once: true }); + } + return { + signal: controller.signal, + abort: () => { + controller.abort(); + }, + dispose: () => { + parent?.removeEventListener("abort", onParentAbort); + }, + }; + } + private async executeCheckpointNode( runDir: string, state: FlowRunState, @@ -1024,9 +1096,13 @@ export class FlowRunner { }, heartbeatMs); } + const runPromise = run(); try { - return await withTimeout(run(), timeoutMs); + return await withTimeout(runPromise, timeoutMs); } catch (error) { + void runPromise.catch(() => { + // ignore + }); if (error instanceof TimeoutError && onTimeout) { await onTimeout().catch(() => { // best effort cancellation only diff --git a/test/flows-shell.test.ts b/test/flows-shell.test.ts index 485eef48..b61e2b4e 100644 --- a/test/flows-shell.test.ts +++ b/test/flows-shell.test.ts @@ -1,11 +1,15 @@ import assert from "node:assert/strict"; +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; import test from "node:test"; -import { TimeoutError } from "../src/async-control.js"; +import { InterruptedError, TimeoutError } from "../src/async-control.js"; import { formatShellActionSummary, renderShellCommand, runShellAction, } from "../src/flows/executors/shell.js"; +import { isProcessAlive } from "../src/process-liveness.js"; test("renderShellCommand quotes arguments consistently", () => { assert.equal(renderShellCommand("echo", ["hello", "two words"]), 'echo "hello" "two words"'); @@ -77,3 +81,88 @@ test("runShellAction rejects commands terminated by signal", async () => { /signal SIGTERM/, ); }); + +test("runShellAction already-aborted signal does not spawn a child", async () => { + const controller = new AbortController(); + controller.abort(); + await assert.rejects( + async () => + await runShellAction( + { + command: process.execPath, + args: ["-e", "process.exit(0)"], + }, + { signal: controller.signal }, + ), + InterruptedError, + ); +}); + +test("runShellAction abort kills the child process", async () => { + const pidDir = await fs.mkdtemp(path.join(os.tmpdir(), "acpx-shell-abort-")); + const pidPath = path.join(pidDir, "pid"); + const controller = new AbortController(); + let pid: number | undefined; + const run = runShellAction( + { + command: process.execPath, + args: ["-e", writePidAndKeepAliveScript(pidPath)], + }, + { signal: controller.signal }, + ); + + try { + pid = await waitForPidFile(pidPath, 2_000); + assert.equal(isProcessAlive(pid), true); + const rejected = assert.rejects(async () => await run, InterruptedError); + controller.abort(); + assert.equal(await waitUntilDead(pid, 1_500), true); + await rejected; + } finally { + forceKill(pid); + await run.catch(() => undefined); + await fs.rm(pidDir, { recursive: true, force: true }); + } +}); + +function writePidAndKeepAliveScript(pidPath: string): string { + return `require("node:fs").writeFileSync(${JSON.stringify(pidPath)}, String(process.pid)); setInterval(() => {}, 60_000);`; +} + +async function waitForPidFile(pidPath: string, timeoutMs: number): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + try { + const value = Number(await fs.readFile(pidPath, "utf8")); + if (Number.isInteger(value) && value > 0) { + return value; + } + } catch { + // not written yet + } + await new Promise((resolve) => setTimeout(resolve, 20)); + } + throw new Error(`Timed out waiting for pid file ${pidPath}`); +} + +async function waitUntilDead(pid: number, timeoutMs: number): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (!isProcessAlive(pid)) { + return true; + } + await new Promise((resolve) => setTimeout(resolve, 20)); + } + return !isProcessAlive(pid); +} + +function forceKill(pid: number | undefined): void { + if (!pid || !isProcessAlive(pid)) { + return; + } + try { + process.kill(pid, "SIGKILL"); + } catch { + // already gone + } +} diff --git a/test/flows.test.ts b/test/flows.test.ts index 8928455b..6bc43a1e 100644 --- a/test/flows.test.ts +++ b/test/flows.test.ts @@ -25,6 +25,7 @@ import type { ShellActionNodeDefinition, } from "../src/flows/runtime.js"; import { flowRunsBaseDir } from "../src/flows/store.js"; +import { isProcessAlive } from "../src/process-liveness.js"; import type { PromptInput } from "../src/types.js"; const MOCK_AGENT_PATH = fileURLToPath(new URL("./mock-agent.js", import.meta.url)); @@ -1411,6 +1412,72 @@ test("FlowRunner keeps same session handles isolated by working directory", asyn }); }); +test("FlowRunner kills shell child when outer node timeout fires", async () => { + await withTempHome(async () => { + const outputRoot = await fs.mkdtemp(path.join(os.tmpdir(), "acpx-flow-store-")); + const pidDir = await fs.mkdtemp(path.join(os.tmpdir(), "acpx-flow-shell-pid-")); + const pidPath = path.join(pidDir, "pid"); + let pid: number | undefined; + const runner = new FlowRunner({ + resolveAgent: () => ({ + agentName: "unused", + agentCommand: "unused", + cwd: process.cwd(), + }), + permissionMode: "approve-all", + outputRoot, + }); + + const flow = defineFlow({ + name: "outer-timeout-kills-shell", + startAt: "slow", + nodes: { + slow: shell({ + timeoutMs: 200, + exec: () => ({ + command: process.execPath, + args: [ + "-e", + `require("node:fs").writeFileSync(${JSON.stringify(pidPath)}, String(process.pid)); setInterval(() => {}, 60_000);`, + ], + timeoutMs: 0, + }), + }), + }, + edges: [], + }); + + const runPromise = runner.run(flow, {}); + try { + pid = await waitFor(async () => { + try { + const value = Number(await fs.readFile(pidPath, "utf8")); + return Number.isInteger(value) && value > 0 ? value : null; + } catch { + return null; + } + }, 2_000); + assert.equal(isProcessAlive(pid), true); + await assert.rejects(async () => await runPromise, TimeoutError); + const deadline = Date.now() + 1_500; + while (isProcessAlive(pid) && Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 20)); + } + assert.equal(isProcessAlive(pid), false); + } finally { + if (pid && isProcessAlive(pid)) { + try { + process.kill(pid, "SIGKILL"); + } catch { + // already gone + } + } + await runPromise.catch(() => undefined); + await fs.rm(pidDir, { recursive: true, force: true }); + } + }); +}); + test("FlowRunner marks timed out shell steps explicitly", async () => { await withTempHome(async () => { const outputRoot = await fs.mkdtemp(path.join(os.tmpdir(), "acpx-flow-store-"));