From 46a458cbac4cae1a045080a2d5fa13ecc5d88de6 Mon Sep 17 00:00:00 2001 From: Ansel-2428 Date: Sat, 17 Oct 2026 12:28:37 -0400 Subject: [PATCH 1/4] fix(code-execution): an abort or dispose while the interpreter starts stops the script PyRunner.exec claimed the runner with a `starting` flag while it awaited the interpreter, and recorded its pending exec only once the interpreter was ready. abort() returns when there's no pending exec, and dispose() settles only a pending one, so in that window both did nothing to the exec: an abort was lost, and the script then ran to its end or its time limit after its turn had died. Meanwhile `busy` was true, so each later code_execution of the agent was refused as another script already running. dispose() also tore the starting interpreter down, and teardownChild removes the child's exit listeners before the kill, the readiness promise's among them, so exec waited out the 15 s start timeout and then reported a start failure that hadn't happened. This is xpkg's flaky "abort() settles a running script and reclaims the interpreter": at load 83 the interpreter took longer than the test's 500 ms wait to start, and the abort landed in that window. - exec creates its pending record before it awaits the interpreter, in place of `starting`, and `busy` is `pending !== null`. abort() and dispose() settle it in either phase and reclaim the interpreter, starting or not. The record's `sent` marks the script as sent; exec checks the record once the interpreter is ready and returns the settled result without sending if it has already settled, as for an abort on the tick exec is called on a warm interpreter. - reclaim() fails the readiness of an interpreter that is still starting, and so clears its 15 s timer, so an exec waiting on it returns at once. It's in reclaim() rather than teardownChild(), which an interpreter's own exit also runs, so that exit still reads "exited before becoming ready". - The child's exit handler settles only a sent exec, so an interpreter that exits before it's ready still reads "Failed to start python runtime". exec's catch settles and reclaims only while its own record is unsettled, so an exec aborted while starting can't fail the exec that claimed the runner after it, or kill its interpreter. The abort test now waits for its script's first tool call rather than 500 ms, so it tests a running script, as its name says. New cases, in "PyRunner: an abort or dispose while the interpreter starts": an abort on the tick exec is called; an abort 300 ms into a start that takes 1 s (a shell stand-in for python3), the old test's steps; an exec issued on the tick an abort settles the first; an abort on a warm interpreter on the exec's tick, with a 300 ms deadline that must not reach the next exec; a dispose, and an abort then a dispose, each settling well before the 15 s start timeout; and a stand-in that exits before it's ready, which still reads as a failed start. --- ...de-execution-abort-while-starting.fixed.md | 1 + src/code-execution/py-runner.ts | 164 ++++++++++-------- src/framework.ts | 2 +- test/code-execution.test.ts | 158 ++++++++++++++++- 4 files changed, 249 insertions(+), 76 deletions(-) create mode 100644 changelog.d/fix-code-execution-abort-while-starting.fixed.md diff --git a/changelog.d/fix-code-execution-abort-while-starting.fixed.md b/changelog.d/fix-code-execution-abort-while-starting.fixed.md new file mode 100644 index 000000000..4d2b9cedb --- /dev/null +++ b/changelog.d/fix-code-execution-abort-while-starting.fixed.md @@ -0,0 +1 @@ +- A `code_execution` script whose interpreter is still starting is now stopped by `PyRunner.abort()` and `dispose()`, as a running one is, and so is one aborted on the tick `exec` is called. The exec settles at once, and the script never runs. Before, an abort in that window did nothing, though `busy` was already true: when a turn died then, its script ran to its end or its time limit, calling tools as it went, and each `code_execution` call of the agent's later turns was refused as another script already running until then. A dispose there, as a background script's cancel does, waited out the interpreter's 15 s start timeout and then reported that the runtime had failed to start. The interpreter starts at an agent's first script, after it has been idle, and after a script is aborted, killed at its time limit or crashes. diff --git a/src/code-execution/py-runner.ts b/src/code-execution/py-runner.ts index 817e38343..c6c081173 100644 --- a/src/code-execution/py-runner.ts +++ b/src/code-execution/py-runner.ts @@ -101,6 +101,10 @@ interface PendingExec { settled: boolean; /** Set once the deadline fired: the result then says the script ran out of time. */ deadlineMs: number | null; + /** Set once the script went to the interpreter. Before that, the exec is + * waiting for the interpreter to start, and an interpreter that exits then + * failed to start (see exec). */ + sent: boolean; } export class PyRunner { @@ -121,8 +125,6 @@ export class PyRunner { private idleTimer: ReturnType | null = null; private execCounter = 0; private disposed = false; - /** An exec has passed the busy check but is still awaiting the interpreter. */ - private starting = false; constructor(options: PyRunnerOptions) { this.pythonPath = options.pythonPath ?? 'python3'; @@ -135,7 +137,7 @@ export class PyRunner { } get busy(): boolean { - return this.pending !== null || this.starting; + return this.pending !== null; } /** @@ -170,76 +172,87 @@ export class PyRunner { } this.clearIdleTimer(); - this.starting = true; // claimed before the await: a same-tick second exec must see it + // The exec claims the runner before the await: a same-tick second exec + // sees it busy, and abort() or dispose() while the interpreter starts + // settles this exec, whose script then never runs. + let resolveResult!: (result: ExecResult) => void; + const settled = new Promise((resolve) => { + resolveResult = resolve; + }); + const pending: PendingExec = { + id: `e${++this.execCounter}`, + resolve: resolveResult, + deadlineTimer: null, + killTimer: null, + settled: false, + deadlineMs: null, + sent: false, + }; + this.pending = pending; try { await this.ensureChild(); } catch (err) { - const message = err instanceof Error ? err.message : String(err); - this.reclaim('spawn-failed'); - return { - stdout: '', - stderr: `Failed to start python runtime (${this.pythonPath}): ${message}`, - returnCode: 1, - aborted: true, - }; - } finally { - this.starting = false; // `pending` is set below, with no await in between + // Settled already when an abort or dispose reclaimed the interpreter + // as it started; another exec may have claimed the runner since. + if (!pending.settled) { + const message = err instanceof Error ? err.message : String(err); + this.settlePending({ + stdout: '', + stderr: `Failed to start python runtime (${this.pythonPath}): ${message}`, + returnCode: 1, + aborted: true, + }); + this.reclaim('spawn-failed'); + } + return settled; } + if (pending.settled) return settled; - const execId = `e${++this.execCounter}`; + const execId = pending.id; // Clamped: a deadline past Node's timer range would fire at once instead of never. const deadlineMs = Math.min(MAX_TIMER_MS, background?.lifetimeMs ?? opts?.deadlineMs ?? this.scriptTimeoutMs); this.onWake = background?.onWake ?? null; - const result = await new Promise((resolve) => { - const pending: PendingExec = { - id: execId, - resolve, - deadlineTimer: null, - killTimer: null, - settled: false, - deadlineMs: null, - }; - this.pending = pending; - - pending.deadlineTimer = setTimeout(() => { - pending.deadlineMs = deadlineMs; - // Deadline: ask politely first (script sees CancelledError and its - // exec_result still flows back), then kill on unresponsiveness. - this.send({ op: 'cancel', id: execId, reason: 'deadline' }); - // The cancel lands only when the script awaits. Blocking code (time.sleep, a - // busy loop, a blocking read) never does, so interrupt it as well: the runtime - // raises KeyboardInterrupt in the script's own code, or schedules the - // cancellation of a script that is waiting. Repeated, because a waiting script - // can resume into blocking code before that cancellation runs; the runtime - // ignores the repeats once the script was interrupted or cancelled. - this.interruptChild(); - pending.interruptTimer = setInterval(() => this.interruptChild(), INTERRUPT_REPEAT_MS); - pending.killTimer = setTimeout(() => { - pending.deadlineMs = null; // this message already says why - this.settlePending({ - stdout: '', - stderr: - `script stopped: it reached its ${formatLimit(deadlineMs)} time limit and did not respond, ` + - 'so it was killed and the interpreter restarted (variables from earlier scripts are gone)', - returnCode: 1, - aborted: true, - }); - this.reclaim('deadline-kill'); - }, this.cancelGraceMs); - }, deadlineMs); - // A day-scale background deadline must not hold the process open. - if (background) pending.deadlineTimer.unref?.(); - - this.send({ - op: 'init', - tools: tools.map((t) => ({ py_name: t.pyName, tool_name: t.toolName })), - call_timeout_s: Math.round(this.toolCallTimeoutMs / 1000), - ...(background - ? { background: true, log_path: background.logPath ?? null } - : {}), - }); - this.send({ op: 'exec', id: execId, code }); + pending.deadlineTimer = setTimeout(() => { + pending.deadlineMs = deadlineMs; + // Deadline: ask politely first (script sees CancelledError and its + // exec_result still flows back), then kill on unresponsiveness. + this.send({ op: 'cancel', id: execId, reason: 'deadline' }); + // The cancel lands only when the script awaits. Blocking code (time.sleep, a + // busy loop, a blocking read) never does, so interrupt it as well: the runtime + // raises KeyboardInterrupt in the script's own code, or schedules the + // cancellation of a script that is waiting. Repeated, because a waiting script + // can resume into blocking code before that cancellation runs; the runtime + // ignores the repeats once the script was interrupted or cancelled. + this.interruptChild(); + pending.interruptTimer = setInterval(() => this.interruptChild(), INTERRUPT_REPEAT_MS); + pending.killTimer = setTimeout(() => { + pending.deadlineMs = null; // this message already says why + this.settlePending({ + stdout: '', + stderr: + `script stopped: it reached its ${formatLimit(deadlineMs)} time limit and did not respond, ` + + 'so it was killed and the interpreter restarted (variables from earlier scripts are gone)', + returnCode: 1, + aborted: true, + }); + this.reclaim('deadline-kill'); + }, this.cancelGraceMs); + }, deadlineMs); + // A day-scale background deadline must not hold the process open. + if (background) pending.deadlineTimer.unref?.(); + + pending.sent = true; + this.send({ + op: 'init', + tools: tools.map((t) => ({ py_name: t.pyName, tool_name: t.toolName })), + call_timeout_s: Math.round(this.toolCallTimeoutMs / 1000), + ...(background + ? { background: true, log_path: background.logPath ?? null } + : {}), }); + this.send({ op: 'exec', id: execId, code }); + + const result = await settled; this.onWake = null; if (!background) this.armIdleTimer(); @@ -247,9 +260,11 @@ export class PyRunner { } /** - * Abort a running script (turn cancelled, agent reset/stopped). The - * interpreter is reclaimed: after an abort its state is suspect (a script - * died midway), and the next exec gets a fresh one. + * Abort the runner's script (turn cancelled, agent reset/stopped), running + * or still waiting for the interpreter to start, when it never runs. The + * exec settles at once, and the interpreter is reclaimed, a starting one + * too: after an abort its state is suspect (a script died midway), and the + * next exec gets a fresh one. */ abort(reason: string): void { if (!this.pending) return; @@ -308,7 +323,9 @@ export class PyRunner { }); child.on('exit', (exitCode, signal) => { - if (this.pending) { + // An exec still waiting for the interpreter is settled by exec, as a + // runtime that failed to start. + if (this.pending?.sent) { this.settlePending({ stdout: '', stderr: `python runtime exited unexpectedly (code=${exitCode}, signal=${signal ?? 'none'})`, @@ -344,10 +361,12 @@ export class PyRunner { const cleanup = () => { clearTimeout(timeout); this.readyResolver = null; + this.readyFailer = null; child.off('exit', onExit); child.off('error', onError); }; this.readyResolver = onReady; + this.readyFailer = onError; child.on('exit', onExit); child.on('error', onError); }); @@ -355,6 +374,8 @@ export class PyRunner { } private readyResolver: (() => void) | null = null; + /** Fails the readiness of an interpreter that is still starting (see reclaim). */ + private readyFailer: ((err: Error) => void) | null = null; private handleLine(line: string, child: ChildProcessWithoutNullStreams): void { let msg: { op?: string; id?: string; name?: string; args?: unknown; stdout?: string; stderr?: string; return_code?: number }; @@ -492,6 +513,12 @@ export class PyRunner { if (this.child) { console.error(`[pytc:${this.label}] reclaiming python interpreter (${reason})`); } + // An interpreter killed as it starts never says it's ready: fail its + // readiness now, so an exec waiting on it returns at once rather than at + // the 15 s timeout. Here rather than in teardownChild, which an + // interpreter's own exit also runs, so that one still reads "exited + // before becoming ready". + this.readyFailer?.(new Error(`python runtime reclaimed (${reason}) before it was ready`)); this.teardownChild(); this.clearIdleTimer(); } @@ -506,6 +533,7 @@ export class PyRunner { this.child = null; this.childReady = null; this.readyResolver = null; + this.readyFailer = null; child.removeAllListeners('exit'); try { child.stdin.end(); diff --git a/src/framework.ts b/src/framework.ts index 5b4f3785f..d215339e5 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -14898,7 +14898,7 @@ export class AgentFramework { }); } - /** Abort a running script when the agent's turn dies underneath it. */ + /** Abort the agent's script, running or still starting, when its turn dies underneath it. */ private abortAgentScript(agentName: string, reason: string): void { const runner = this.codeExecutionRunners.get(agentName); if (runner?.busy) { diff --git a/test/code-execution.test.ts b/test/code-execution.test.ts index 5deec59aa..dc6f881a1 100644 --- a/test/code-execution.test.ts +++ b/test/code-execution.test.ts @@ -11,7 +11,7 @@ import { describe, it } from 'node:test'; import assert from 'node:assert'; import { execFileSync } from 'node:child_process'; -import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { chmodSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import type { @@ -229,11 +229,20 @@ describe('PyRunner (real python3)', () => { }); it('abort() settles a running script and reclaims the interpreter', async () => { - const runner = new PyRunner({ onToolCall: async () => '' }); + let called!: () => void; + const running = new Promise((resolve) => { + called = resolve; + }); + const runner = new PyRunner({ + onToolCall: async () => { + called(); + return 'echo'; + }, + }); try { - const pending = runner.exec('import asyncio\nawait asyncio.sleep(60)', []); - // Let the exec actually start before aborting. - await new Promise((r) => setTimeout(r, 500)); + const pending = runner.exec('await test__echo({})\nimport asyncio\nawait asyncio.sleep(60)', ECHO_TOOLS); + // The script's tool call says it's running. + await running; assert.strictEqual(runner.busy, true); runner.abort('test abort'); const result = await pending; @@ -264,8 +273,8 @@ describe('PyRunner (real python3)', () => { }); it('rejects a second exec issued before the first has started (same tick)', async () => { - // exec() awaits the interpreter before it records the running script, so - // a check made only after that await let two execs in at once. + // exec() awaits the interpreter, so a claim made only after that await + // would let two execs in at once. const runner = new PyRunner({ onToolCall: async () => '' }); try { for (const warm of [false, true]) { @@ -301,6 +310,141 @@ describe('PyRunner (real python3)', () => { }); }); +// While the interpreter starts, the exec has claimed the runner but its +// script hasn't been sent. An abort or a dispose then settles the exec, and +// the script never runs. +describe('PyRunner: an abort or dispose while the interpreter starts (real python3)', () => { + /** An executable standing in for python3: a shell script with the given body. */ + const standIn = (body: string): { path: string; done: () => void } => { + const dir = mkdtempSync(join(tmpdir(), 'af-pytc-standin-')); + const path = join(dir, 'python3'); + writeFileSync(path, `#!/bin/sh\n${body}\n`); + chmodSync(path, 0o755); + return { path, done: () => rmSync(dir, { recursive: true, force: true }) }; + }; + + it('an abort settles the exec at once, and its script never runs', async () => { + const calls: string[] = []; + const runner = new PyRunner({ + onToolCall: async (name) => { + calls.push(name); + return 'echo'; + }, + }); + try { + const pending = runner.exec('await test__echo({})\nimport asyncio\nawait asyncio.sleep(2)', ECHO_TOOLS); + assert.strictEqual(runner.busy, true); + runner.abort('turn abandoned'); + assert.strictEqual(runner.busy, false, 'the next turn can run a script'); + const result = await pending; + assert.strictEqual(result.aborted, true); + assert.match(result.stderr, /aborted by host: turn abandoned/); + const after = await runner.exec('print("alive")', []); + assert.strictEqual(after.returnCode, 0, after.stderr); + assert.strictEqual(after.stdout, 'alive\n'); + assert.deepStrictEqual(calls, [], 'the aborted script never ran'); + } finally { + runner.dispose(); + } + }); + + it('an abort 300 ms into a start that takes 1 s settles the exec', async () => { + // The steps the abort test took with a 500 ms wait, which a loaded + // machine's start outlasted. + const slow = standIn('sleep 1\nexec python3 "$@"'); + const runner = new PyRunner({ pythonPath: slow.path, onToolCall: async () => '' }); + try { + const pending = runner.exec('import asyncio\nawait asyncio.sleep(2)\nprint("ran")', []); + await new Promise((r) => setTimeout(r, 300)); + assert.strictEqual(runner.busy, true); + runner.abort('test abort'); + const result = await pending; + assert.strictEqual(result.aborted, true); + assert.match(result.stderr, /aborted by host: test abort/); + assert.strictEqual(result.stdout, ''); + } finally { + runner.dispose(); + slow.done(); + } + }); + + it('an exec issued on the tick an abort settles the first runs whole in a fresh interpreter', async () => { + const runner = new PyRunner({ onToolCall: async () => '' }); + try { + const first = runner.exec('import asyncio\nawait asyncio.sleep(2)', []); + runner.abort('turn abandoned'); + const second = runner.exec('print("second")', []); + const [a, b] = await Promise.all([first, second]); + assert.strictEqual(a.aborted, true); + assert.match(a.stderr, /aborted by host: turn abandoned/); + assert.strictEqual(b.returnCode, 0, b.stderr); + assert.strictEqual(b.stdout, 'second\n'); + } finally { + runner.dispose(); + } + }); + + it('an abort on a warm interpreter, on the tick exec is called, settles it before its script is sent', async () => { + const runner = new PyRunner({ onToolCall: async () => '' }); + try { + await runner.exec('print("warm")', []); + const first = runner.exec('import asyncio\nawait asyncio.sleep(2)', [], undefined, { deadlineMs: 300 }); + runner.abort('turn abandoned'); + const stopped = await first; + assert.strictEqual(stopped.aborted, true); + assert.match(stopped.stderr, /aborted by host: turn abandoned/); + // An exec that went on to arm its deadline would interrupt this one at 300 ms. + const next = await runner.exec('import asyncio\nawait asyncio.sleep(1)\nprint("whole")', []); + assert.strictEqual(next.returnCode, 0, next.stderr); + assert.strictEqual(next.stdout, 'whole\n'); + } finally { + runner.dispose(); + } + }); + + const stops: [string, (runner: PyRunner) => void, RegExp][] = [ + ['a dispose', (runner) => runner.dispose(), /^code_execution runner disposed$/], + [ + 'an abort then a dispose (a background cancel)', + (runner) => { + runner.abort('cancelled by agent'); + runner.dispose(); + }, + /^script aborted by host: cancelled by agent$/, + ], + ]; + for (const [how, stop, says] of stops) { + it(`${how} settles the exec at once`, async () => { + const runner = new PyRunner({ onToolCall: async () => '' }); + const started = Date.now(); + const pending = runner.exec('import asyncio\nawait asyncio.sleep(2)', []); + stop(runner); + const result = await pending; + assert.strictEqual(result.aborted, true); + assert.match(result.stderr, says); + // Waiting on the killed interpreter's readiness would take its 15 s start timeout. + const took = Date.now() - started; + assert.ok(took < 5_000, `settled after ${took} ms`); + }); + } + + it('an interpreter that exits before it is ready still reads as a failed start', async () => { + const exits = standIn('exit 3'); + const runner = new PyRunner({ pythonPath: exits.path, onToolCall: async () => '' }); + try { + const result = await runner.exec('print(1)', []); + assert.strictEqual(result.aborted, true); + assert.strictEqual( + result.stderr, + `Failed to start python runtime (${exits.path}): python runtime exited before becoming ready`, + ); + } finally { + runner.dispose(); + exits.done(); + } + }); +}); + // Inner-call ids restart in every interpreter: a result that outlives its // interpreter must not resolve a call of the one that replaced it. describe('code_execution inner-call results after the interpreter is replaced (real python3)', () => { From aae1f007b3a30ec62bdafb5085c6d2bb216fd385 Mon Sep 17 00:00:00 2001 From: Ansel-2428 Date: Sat, 17 Oct 2026 13:22:25 -0400 Subject: [PATCH 2/4] fix(code-execution): answer the start-window review: the input stream's error listener, a kill only for a child with a pid, and tests that can't hang or pass by timing The review of 46a458c (af-f71-abort-while-starting, wf-2317) and Gordon-2414's read of its unread spots found: - A miss, from Gordon's runs (J16402, J16438): a spawn that fails, such as for a missing interpreter, leaves a child with no pid and a null exitCode until Node's 'error' for it a tick later, and teardownChild killed any child with a null exitCode. Node then signals whatever pid the child's handle held, since libuv sets it only for a spawn that succeeds (Gordon's reading of libuv 1.46.0 and Node 20.20.2): in a fresh Node 20 process that was 0, so SIGKILL went to the host's own process group (his strace), and on Node 24 a different large pid each time. dispose() on exec's tick reached it on main already; this change's claim before the await lets abort() reach it too. teardownChild now kills only a child with a pid. - I7: send() catches only what write() throws, and the interpreter's stdin had no 'error' listener, so a write to an interpreter that no longer reads its input fails afterwards on the stream and is thrown in the host as an uncaught `write EPIPE`. A probe showed it at both heads: on main for exec's own first write to an interpreter that closed its input after saying it was ready, and at 46a458c also for the cancel that abort() now writes to a starting interpreter. ensureChild gives the stream a listener that logs; the interpreter's exit settles its exec, as before. - I8: the stream-error retry and the provider's cooldown and wait reset a turn to run again rather than end it (Gordon read the six sites). abortAgentScript's comment and the fragment say a script is aborted when its turn ends or is reset to run again. - I1: the abort test's wait for its script's first tool call had no bound, and npm test sets no per-test timeout. It now races that call against the exec. - I3: the 300 ms test stood in a fixed wait for "still starting". Its stand-in now never says it's ready (`exec sleep 30`), so the abort lands while it starts however late the wait ends. - I4: no abort-only case was timed. The tick abort now settles within 5 s, against the 15 s start timeout. - I9, I10, I11: the stand-in cases are skipped on Windows; abort()'s docstring and the warm test's comment say what they mean. New cases: an abort while a stand-in that has closed its input starts, and an exec to one that closes it once it's ready, each throw nothing; an abort and a dispose on exec's tick with a missing interpreter each settle with their own text, and no kill reaches a child with no pid. A stub of ChildProcess.prototype.kill records such a kill and passes on only a child's with a pid, so with the check taken out the case fails on every Node and nothing is signalled (Gordon's suggestion: a real kill's red depended on the heap). The `until` helper moves to the file's top. The fragment adds the logged write and the kill. --- ...de-execution-abort-while-starting.fixed.md | 4 +- src/code-execution/py-runner.ts | 25 +++- src/framework.ts | 2 +- test/code-execution.test.ts | 135 ++++++++++++++---- 4 files changed, 134 insertions(+), 32 deletions(-) diff --git a/changelog.d/fix-code-execution-abort-while-starting.fixed.md b/changelog.d/fix-code-execution-abort-while-starting.fixed.md index 4d2b9cedb..8f4af0af5 100644 --- a/changelog.d/fix-code-execution-abort-while-starting.fixed.md +++ b/changelog.d/fix-code-execution-abort-while-starting.fixed.md @@ -1 +1,3 @@ -- A `code_execution` script whose interpreter is still starting is now stopped by `PyRunner.abort()` and `dispose()`, as a running one is, and so is one aborted on the tick `exec` is called. The exec settles at once, and the script never runs. Before, an abort in that window did nothing, though `busy` was already true: when a turn died then, its script ran to its end or its time limit, calling tools as it went, and each `code_execution` call of the agent's later turns was refused as another script already running until then. A dispose there, as a background script's cancel does, waited out the interpreter's 15 s start timeout and then reported that the runtime had failed to start. The interpreter starts at an agent's first script, after it has been idle, and after a script is aborted, killed at its time limit or crashes. +- A `code_execution` script whose interpreter is still starting is now stopped by `PyRunner.abort()` and `dispose()`, as a running one is, and so is one aborted on the tick `exec` is called. The exec settles at once, and the script never runs. Before, an abort in that window did nothing, though `busy` was already true: when a script's turn ended or was reset to run again in that window, the script ran to its end or its time limit, calling tools as it went, and each `code_execution` call of the agent's later turns was refused as another script already running until then. A dispose there, as a background script's cancel does, waited out the interpreter's 15 s start timeout and then reported that the runtime had failed to start. The interpreter starts at an agent's first script, after it has been idle, and after a script is aborted, killed at its time limit or crashes. +- A write to a `code_execution` interpreter that no longer reads its input, such as one that died before its exit was handled, is now logged. It was thrown in the host as an uncaught exception (`write EPIPE`). The interpreter's exit settles its script as before. +- An abort or a dispose on the tick `exec` is called, when the interpreter couldn't be spawned, no longer signals another process. The runner killed the failed spawn's child, which has no pid, in the tick before Node's `error` for it, and Node then signals whatever pid the child's handle held: in a fresh Node 20 process that was 0, so `SIGKILL` went to the host's own process group. diff --git a/src/code-execution/py-runner.ts b/src/code-execution/py-runner.ts index c6c081173..ab08d1854 100644 --- a/src/code-execution/py-runner.ts +++ b/src/code-execution/py-runner.ts @@ -260,11 +260,11 @@ export class PyRunner { } /** - * Abort the runner's script (turn cancelled, agent reset/stopped), running - * or still waiting for the interpreter to start, when it never runs. The - * exec settles at once, and the interpreter is reclaimed, a starting one - * too: after an abort its state is suspect (a script died midway), and the - * next exec gets a fresh one. + * Abort the runner's script (turn cancelled, agent reset/stopped), whether + * it's running or still waiting for its interpreter to start; a waiting + * script then never runs. The exec settles at once, and the interpreter is + * reclaimed, a starting one too: after an abort its state is suspect (a + * script died midway), and the next exec gets a fresh one. */ abort(reason: string): void { if (!this.pending) return; @@ -317,6 +317,14 @@ export class PyRunner { console.error(`[pytc:${this.label}] ${chunk.trimEnd()}`); }); + // A write to an interpreter that no longer reads its input, such as one + // that died before its exit was handled, fails on the stream after + // write() returns. With no listener, that error is thrown as an uncaught + // exception. The interpreter's exit settles its exec. + child.stdin.on('error', (err) => { + console.error(`[pytc:${this.label}] write to the python runtime failed: ${err.message}`); + }); + child.on('error', (err) => { // Spawn failure lands here (e.g. python3 missing); readiness rejects. console.error(`[pytc:${this.label}] python process error: ${err.message}`); @@ -540,7 +548,12 @@ export class PyRunner { } catch { // stream may already be destroyed } - if (child.exitCode === null && !child.killed) { + // A child whose spawn failed has no pid, and its exitCode stays null + // until Node's 'error' for it, a tick later. A kill() in that tick, as + // from an abort or dispose on the tick exec was called, signals no + // child of ours: on Node 20 it sent SIGKILL to the host's own process + // group. + if (child.exitCode === null && !child.killed && child.pid !== undefined) { child.kill('SIGKILL'); } } diff --git a/src/framework.ts b/src/framework.ts index d215339e5..d5879ace8 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -14898,7 +14898,7 @@ export class AgentFramework { }); } - /** Abort the agent's script, running or still starting, when its turn dies underneath it. */ + /** Abort the agent's script, running or still starting, when the turn it belongs to ends or is reset to run again. */ private abortAgentScript(agentName: string, reason: string): void { const runner = this.codeExecutionRunners.get(agentName); if (runner?.busy) { diff --git a/test/code-execution.test.ts b/test/code-execution.test.ts index dc6f881a1..3a510d1c4 100644 --- a/test/code-execution.test.ts +++ b/test/code-execution.test.ts @@ -10,8 +10,8 @@ import { describe, it } from 'node:test'; import assert from 'node:assert'; -import { execFileSync } from 'node:child_process'; -import { chmodSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { ChildProcess, execFileSync } from 'node:child_process'; +import { chmodSync, existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import type { @@ -32,6 +32,15 @@ const ECHO_TOOLS: { pyName: string; toolName: string }[] = [ { pyName: 'test__echo', toolName: 'test--echo' }, ]; +/** Wait for a condition, failing after 10 s rather than hanging the run. */ +const until = async (cond: () => boolean, what: string) => { + const deadline = Date.now() + 10_000; + while (!cond()) { + if (Date.now() > deadline) throw new Error(`timed out waiting for ${what}`); + await new Promise((r) => setTimeout(r, 20)); + } +}; + describe('buildInjectedTools', () => { it('sanitizes -- to __ and keeps valid identifiers', () => { const injected = buildInjectedTools(['mcpl--discord--send_message', 'think']); @@ -241,8 +250,12 @@ describe('PyRunner (real python3)', () => { }); try { const pending = runner.exec('await test__echo({})\nimport asyncio\nawait asyncio.sleep(60)', ECHO_TOOLS); - // The script's tool call says it's running. - await running; + // The script's tool call says it's running. An exec that settles first + // fails the test rather than leaving it waiting. + await Promise.race([ + running, + pending.then((r) => assert.fail(`the exec settled before its script called its tool: ${r.stderr}`)), + ]); assert.strictEqual(runner.busy, true); runner.abort('test abort'); const result = await pending; @@ -315,13 +328,17 @@ describe('PyRunner (real python3)', () => { // the script never runs. describe('PyRunner: an abort or dispose while the interpreter starts (real python3)', () => { /** An executable standing in for python3: a shell script with the given body. */ - const standIn = (body: string): { path: string; done: () => void } => { + const standIn = (body: string): { path: string; dir: string; done: () => void } => { const dir = mkdtempSync(join(tmpdir(), 'af-pytc-standin-')); const path = join(dir, 'python3'); writeFileSync(path, `#!/bin/sh\n${body}\n`); chmodSync(path, 0o755); - return { path, done: () => rmSync(dir, { recursive: true, force: true }) }; + return { path, dir, done: () => rmSync(dir, { recursive: true, force: true }) }; }; + // The stand-ins are shell scripts. + const posix = { skip: process.platform === 'win32' ? 'the stand-ins need /bin/sh' : false }; + // A write's failure comes back through the event loop: two turns of it. + const turns = () => new Promise((r) => setImmediate(() => setImmediate(r))); it('an abort settles the exec at once, and its script never runs', async () => { const calls: string[] = []; @@ -332,6 +349,7 @@ describe('PyRunner: an abort or dispose while the interpreter starts (real pytho }, }); try { + const started = Date.now(); const pending = runner.exec('await test__echo({})\nimport asyncio\nawait asyncio.sleep(2)', ECHO_TOOLS); assert.strictEqual(runner.busy, true); runner.abort('turn abandoned'); @@ -339,6 +357,9 @@ describe('PyRunner: an abort or dispose while the interpreter starts (real pytho const result = await pending; assert.strictEqual(result.aborted, true); assert.match(result.stderr, /aborted by host: turn abandoned/); + // Waiting on the killed interpreter's readiness would take its 15 s start timeout. + const took = Date.now() - started; + assert.ok(took < 5_000, `settled after ${took} ms`); const after = await runner.exec('print("alive")', []); assert.strictEqual(after.returnCode, 0, after.stderr); assert.strictEqual(after.stdout, 'alive\n'); @@ -348,23 +369,24 @@ describe('PyRunner: an abort or dispose while the interpreter starts (real pytho } }); - it('an abort 300 ms into a start that takes 1 s settles the exec', async () => { - // The steps the abort test took with a 500 ms wait, which a loaded - // machine's start outlasted. - const slow = standIn('sleep 1\nexec python3 "$@"'); - const runner = new PyRunner({ pythonPath: slow.path, onToolCall: async () => '' }); + it('an abort 300 ms into a start that never finishes settles the exec', posix, async () => { + // The abort test's old steps, a wait and then an abort, which a loaded + // machine's start outlasted. The stand-in never says it's ready, so the + // abort lands while it starts, however late the wait ends. + const never = standIn('exec sleep 30'); + const runner = new PyRunner({ pythonPath: never.path, onToolCall: async () => '' }); try { - const pending = runner.exec('import asyncio\nawait asyncio.sleep(2)\nprint("ran")', []); + const pending = runner.exec('print("ran")', []); await new Promise((r) => setTimeout(r, 300)); assert.strictEqual(runner.busy, true); runner.abort('test abort'); const result = await pending; assert.strictEqual(result.aborted, true); - assert.match(result.stderr, /aborted by host: test abort/); + assert.strictEqual(result.stderr, 'script aborted by host: test abort'); assert.strictEqual(result.stdout, ''); } finally { runner.dispose(); - slow.done(); + never.done(); } }); @@ -393,7 +415,8 @@ describe('PyRunner: an abort or dispose while the interpreter starts (real pytho const stopped = await first; assert.strictEqual(stopped.aborted, true); assert.match(stopped.stderr, /aborted by host: turn abandoned/); - // An exec that went on to arm its deadline would interrupt this one at 300 ms. + // Had the aborted exec gone on to arm its deadline, that would interrupt + // this next script at 300 ms. const next = await runner.exec('import asyncio\nawait asyncio.sleep(1)\nprint("whole")', []); assert.strictEqual(next.returnCode, 0, next.stderr); assert.strictEqual(next.stdout, 'whole\n'); @@ -428,7 +451,79 @@ describe('PyRunner: an abort or dispose while the interpreter starts (real pytho }); } - it('an interpreter that exits before it is ready still reads as a failed start', async () => { + it('an abort while an interpreter that has stopped reading its input starts throws nothing', posix, async () => { + // The stand-in closes its input, says so, and never gets ready, so the + // abort's write meets a pipe nobody reads. + const closed = standIn('exec 0<&-\ntouch "$(dirname "$0")/closed"\nexec sleep 30'); + const runner = new PyRunner({ pythonPath: closed.path, onToolCall: async () => '' }); + try { + const pending = runner.exec('print(1)', []); + await until(() => existsSync(join(closed.dir, 'closed')), 'the stand-in to close its input'); + runner.abort('turn abandoned'); + const result = await pending; + assert.strictEqual(result.stderr, 'script aborted by host: turn abandoned'); + // An unheard write error would fail this test as an uncaught exception. + await turns(); + } finally { + runner.dispose(); + closed.done(); + } + }); + + it("an exec whose interpreter stops reading its input once it's ready throws nothing, and settles at its exit", posix, async () => { + // The script's own writes, the init and the exec, meet a pipe nobody reads. + const closes = standIn(`exec 0<&-\necho '{"op":"ready"}'\nexec sleep 1`); + const runner = new PyRunner({ pythonPath: closes.path, onToolCall: async () => '' }); + try { + const result = await runner.exec('print(1)', []); + assert.strictEqual(result.aborted, true); + assert.strictEqual(result.stderr, 'python runtime exited unexpectedly (code=0, signal=none)'); + await turns(); + } finally { + runner.dispose(); + closes.done(); + } + }); + + it("an abort or a dispose on the tick exec is called, with an interpreter that can't be spawned, kills no child without a pid", async () => { + // A failed spawn's child has no pid, and its exitCode stays null until + // Node's 'error' for it, a tick later. A kill() in that tick signals + // whatever pid Node's handle held: in a fresh Node 20 process that was 0, + // the caller's own process group. So the stub records such a kill + // instead, and passes on only a child's with a pid. + const kill = ChildProcess.prototype.kill; + const pidless: (NodeJS.Signals | number | undefined)[] = []; + ChildProcess.prototype.kill = function (this: ChildProcess, signal?: NodeJS.Signals | number) { + if (this.pid === undefined) { + pidless.push(signal); + return false; + } + return kill.call(this, signal); + }; + try { + for (const stop of ['abort', 'dispose'] as const) { + const runner = new PyRunner({ pythonPath: '/definitely/not/a/python', onToolCall: async () => '' }); + try { + const pending = runner.exec('print(1)', []); + if (stop === 'abort') runner.abort('stopped on the spawn tick'); + else runner.dispose(); + const result = await pending; + assert.strictEqual(result.aborted, true, stop); + assert.strictEqual( + result.stderr, + stop === 'abort' ? 'script aborted by host: stopped on the spawn tick' : 'code_execution runner disposed', + ); + assert.deepStrictEqual(pidless, [], `${stop}: a kill reached a child with no pid`); + } finally { + runner.dispose(); + } + } + } finally { + ChildProcess.prototype.kill = kill; + } + }); + + it('an interpreter that exits before it is ready still reads as a failed start', posix, async () => { const exits = standIn('exit 3'); const runner = new PyRunner({ pythonPath: exits.path, onToolCall: async () => '' }); try { @@ -448,14 +543,6 @@ describe('PyRunner: an abort or dispose while the interpreter starts (real pytho // Inner-call ids restart in every interpreter: a result that outlives its // interpreter must not resolve a call of the one that replaced it. describe('code_execution inner-call results after the interpreter is replaced (real python3)', () => { - const until = async (cond: () => boolean, what: string) => { - const deadline = Date.now() + 10_000; - while (!cond()) { - if (Date.now() > deadline) throw new Error(`timed out waiting for ${what}`); - await new Promise((r) => setTimeout(r, 20)); - } - }; - const stops: Record void }> = { aborted: { code: 'print(await test__echo({}))', stop: (runner) => runner.abort('turn abandoned') }, crashed: { From 11f0f6473c73c18584962381f1c49ca6d7054051 Mon Sep 17 00:00:00 2001 From: Ansel-2428 Date: Sat, 17 Oct 2026 15:12:58 -0400 Subject: [PATCH 3/4] fix(code-execution): answer the round's review: end an interpreter that stops reading its input, and pin the reclaim's kill The review of aae1f00 (af-f71-r2, wf-2375: 14 issues) found: - I1: the stdin 'error' listener only logged. An interpreter can stop reading its input and run on: after a script's os.close(0), the runtime's reader thread dies with EBADF. Every later message to it is lost, so the next exec waited out its time limit plus the 10 s grace and was told it had reached its time limit (a probe with the real runtime: 13 s with a 3 s limit). On main 3adbdb86 the same write was an uncaught `write EPIPE` that ended the host's process, on Node 24.22.0 and 20.20.2. The listener now kills an interpreter that is still running, and its exit settles the exec with "python runtime stopped reading its input, so it was killed and the interpreter restarted ... A script that closes its standard input (file descriptor 0) does this." The message is used only when the exit is the kill's SIGKILL, so an interpreter that had died already settles with its own code. - I2: no test pinned the reclaim's kill. The start-window stand-in now writes its pid, and the test checks the process is gone after the abort (the reclaim of a starting interpreter, and teardownChild's kill of a child with a pid). The running-script abort test now sets a variable before its tool call and checks the next exec doesn't see it. - I3: a within() helper bounds at 10 s the abort test's wait for its script's tool call and the kill-stub test's await of each exec. - I4: the closed-input test's stand-in no longer exits on its own (exec sleep 30), so the exec settles only at the failed write. - I6: the two settle bounds go from 5 s to 10 s, still below the 15 s readiness timeout they guard against. - I8, I9: the start-window test waits for the stand-in's pid file rather than 300 ms, and two test names say what they check. - I12, I13: teardownChild's comment names both outcomes of a kill without a pid (the host's own process group on Node 20.20.2, another process's pid on 24.19), and abort()'s docstring lists a turn reset to run again. Not changed: I5 (a log line on a failed spawn's tick), I7 (no realistic read error on stdout or stderr), I14 (a probe under EMFILE on Node 24.22.0 and 20.20.2 found the child's stdio streams present and the 'error' heard; PyRunner settled "Failed to start python runtime ... EMFILE" with nothing uncaught). The fragment says what callers now get. --- ...de-execution-abort-while-starting.fixed.md | 2 +- src/code-execution/py-runner.ts | 37 ++++++--- test/code-execution.test.ts | 82 +++++++++++++------ 3 files changed, 82 insertions(+), 39 deletions(-) diff --git a/changelog.d/fix-code-execution-abort-while-starting.fixed.md b/changelog.d/fix-code-execution-abort-while-starting.fixed.md index 8f4af0af5..ac1ebcec8 100644 --- a/changelog.d/fix-code-execution-abort-while-starting.fixed.md +++ b/changelog.d/fix-code-execution-abort-while-starting.fixed.md @@ -1,3 +1,3 @@ - A `code_execution` script whose interpreter is still starting is now stopped by `PyRunner.abort()` and `dispose()`, as a running one is, and so is one aborted on the tick `exec` is called. The exec settles at once, and the script never runs. Before, an abort in that window did nothing, though `busy` was already true: when a script's turn ended or was reset to run again in that window, the script ran to its end or its time limit, calling tools as it went, and each `code_execution` call of the agent's later turns was refused as another script already running until then. A dispose there, as a background script's cancel does, waited out the interpreter's 15 s start timeout and then reported that the runtime had failed to start. The interpreter starts at an agent's first script, after it has been idle, and after a script is aborted, killed at its time limit or crashes. -- A write to a `code_execution` interpreter that no longer reads its input, such as one that died before its exit was handled, is now logged. It was thrown in the host as an uncaught exception (`write EPIPE`). The interpreter's exit settles its script as before. +- A `code_execution` interpreter that stops reading its input, as one does after a script closes its standard input (`os.close(0)`), is now ended at the first message that can't reach it, and the script waiting on it settles at once, saying the runtime stopped reading its input. That message's failed write was thrown in the host as an uncaught exception (`write EPIPE`), which ended the host's process. An interpreter that died before its exit was handled still settles with its own exit code. - An abort or a dispose on the tick `exec` is called, when the interpreter couldn't be spawned, no longer signals another process. The runner killed the failed spawn's child, which has no pid, in the tick before Node's `error` for it, and Node then signals whatever pid the child's handle held: in a fresh Node 20 process that was 0, so `SIGKILL` went to the host's own process group. diff --git a/src/code-execution/py-runner.ts b/src/code-execution/py-runner.ts index ab08d1854..8528bbad4 100644 --- a/src/code-execution/py-runner.ts +++ b/src/code-execution/py-runner.ts @@ -260,11 +260,12 @@ export class PyRunner { } /** - * Abort the runner's script (turn cancelled, agent reset/stopped), whether - * it's running or still waiting for its interpreter to start; a waiting - * script then never runs. The exec settles at once, and the interpreter is - * reclaimed, a starting one too: after an abort its state is suspect (a - * script died midway), and the next exec gets a fresh one. + * Abort the runner's script (its turn ended or is reset to run again, or the + * agent was reset or stopped), whether it's running or still waiting for its + * interpreter to start; a waiting script then never runs. The exec settles + * at once, and the interpreter is reclaimed, a starting one too: after an + * abort its state is suspect (a script died midway), and the next exec gets + * a fresh one. */ abort(reason: string): void { if (!this.pending) return; @@ -317,12 +318,19 @@ export class PyRunner { console.error(`[pytc:${this.label}] ${chunk.trimEnd()}`); }); - // A write to an interpreter that no longer reads its input, such as one - // that died before its exit was handled, fails on the stream after - // write() returns. With no listener, that error is thrown as an uncaught - // exception. The interpreter's exit settles its exec. + // A write to an interpreter that no longer reads its input fails on the + // stream after write() returns; with no listener, that error is thrown as + // an uncaught exception. Such an interpreter can go on running, as after a + // script closes file descriptor 0, and every later message to it is lost: + // its exec would wait out its time limit. So end it, and its exit settles + // the exec now. One already dead or killed is left to its own exit. + let inputFailed = false; child.stdin.on('error', (err) => { console.error(`[pytc:${this.label}] write to the python runtime failed: ${err.message}`); + if (child.exitCode === null && !child.killed && child.pid !== undefined) { + inputFailed = true; + child.kill('SIGKILL'); + } }); child.on('error', (err) => { @@ -336,7 +344,12 @@ export class PyRunner { if (this.pending?.sent) { this.settlePending({ stdout: '', - stderr: `python runtime exited unexpectedly (code=${exitCode}, signal=${signal ?? 'none'})`, + stderr: + inputFailed && signal === 'SIGKILL' + ? 'python runtime stopped reading its input, so it was killed and the interpreter restarted ' + + '(variables from earlier scripts are gone). A script that closes its standard input ' + + '(file descriptor 0) does this.' + : `python runtime exited unexpectedly (code=${exitCode}, signal=${signal ?? 'none'})`, returnCode: 1, aborted: true, }); @@ -551,8 +564,8 @@ export class PyRunner { // A child whose spawn failed has no pid, and its exitCode stays null // until Node's 'error' for it, a tick later. A kill() in that tick, as // from an abort or dispose on the tick exec was called, signals no - // child of ours: on Node 20 it sent SIGKILL to the host's own process - // group. + // child of ours but whatever pid Node's handle held: the host's own + // process group on Node 20.20.2, another process's pid on 24.19. if (child.exitCode === null && !child.killed && child.pid !== undefined) { child.kill('SIGKILL'); } diff --git a/test/code-execution.test.ts b/test/code-execution.test.ts index 3a510d1c4..fd57fcb70 100644 --- a/test/code-execution.test.ts +++ b/test/code-execution.test.ts @@ -11,7 +11,7 @@ import { describe, it } from 'node:test'; import assert from 'node:assert'; import { ChildProcess, execFileSync } from 'node:child_process'; -import { chmodSync, existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { chmodSync, existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import type { @@ -41,6 +41,25 @@ const until = async (cond: () => boolean, what: string) => { } }; +/** Wait for a promise, failing after 10 s rather than hanging the run. */ +const within = (promise: Promise, what: string): Promise => { + let timer: ReturnType | undefined; + const late = new Promise((_, reject) => { + timer = setTimeout(() => reject(new Error(`timed out waiting for ${what}`)), 10_000); + }); + return Promise.race([promise, late]).finally(() => clearTimeout(timer)); +}; + +/** Whether a process is still there to signal. */ +const alive = (pid: number): boolean => { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +}; + describe('buildInjectedTools', () => { it('sanitizes -- to __ and keeps valid identifiers', () => { const injected = buildInjectedTools(['mcpl--discord--send_message', 'think']); @@ -249,22 +268,26 @@ describe('PyRunner (real python3)', () => { }, }); try { - const pending = runner.exec('await test__echo({})\nimport asyncio\nawait asyncio.sleep(60)', ECHO_TOOLS); - // The script's tool call says it's running. An exec that settles first - // fails the test rather than leaving it waiting. - await Promise.race([ - running, - pending.then((r) => assert.fail(`the exec settled before its script called its tool: ${r.stderr}`)), - ]); + const pending = runner.exec('marker = 1\nawait test__echo({})\nimport asyncio\nawait asyncio.sleep(60)', ECHO_TOOLS); + // The script's tool call says it's running. An exec that settles first, + // or a call that never comes, fails the test rather than leaving it waiting. + await within( + Promise.race([ + running, + pending.then((r) => assert.fail(`the exec settled before its script called its tool: ${r.stderr}`)), + ]), + 'the script to call its tool', + ); assert.strictEqual(runner.busy, true); runner.abort('test abort'); const result = await pending; assert.strictEqual(result.aborted, true); assert.match(result.stderr, /aborted by host: test abort/); - // Runner remains usable after reclaim (fresh interpreter, state gone). - const after = await runner.exec('print("alive")', []); + // Runner remains usable after reclaim, in a fresh interpreter: the + // aborted script's state is gone. + const after = await runner.exec('try:\n marker\n print("kept")\nexcept NameError:\n print("fresh")', []); assert.strictEqual(after.returnCode, 0, after.stderr); - assert.match(after.stdout, /alive/); + assert.strictEqual(after.stdout, 'fresh\n'); } finally { runner.dispose(); } @@ -359,7 +382,7 @@ describe('PyRunner: an abort or dispose while the interpreter starts (real pytho assert.match(result.stderr, /aborted by host: turn abandoned/); // Waiting on the killed interpreter's readiness would take its 15 s start timeout. const took = Date.now() - started; - assert.ok(took < 5_000, `settled after ${took} ms`); + assert.ok(took < 10_000, `settled after ${took} ms`); const after = await runner.exec('print("alive")', []); assert.strictEqual(after.returnCode, 0, after.stderr); assert.strictEqual(after.stdout, 'alive\n'); @@ -369,28 +392,32 @@ describe('PyRunner: an abort or dispose while the interpreter starts (real pytho } }); - it('an abort 300 ms into a start that never finishes settles the exec', posix, async () => { - // The abort test's old steps, a wait and then an abort, which a loaded - // machine's start outlasted. The stand-in never says it's ready, so the - // abort lands while it starts, however late the wait ends. - const never = standIn('exec sleep 30'); + it('an abort while the interpreter is still starting settles the exec and kills the interpreter', posix, async () => { + // The flaky abort test's case, an abort that lands while the interpreter + // starts. The stand-in writes its pid and never says it's ready, so the + // abort lands while it starts, however late it comes. + const never = standIn('echo $$ > "$(dirname "$0")/pid"\nexec sleep 30'); const runner = new PyRunner({ pythonPath: never.path, onToolCall: async () => '' }); try { const pending = runner.exec('print("ran")', []); - await new Promise((r) => setTimeout(r, 300)); + const pidFile = join(never.dir, 'pid'); + await until(() => existsSync(pidFile) && readFileSync(pidFile, 'utf8').endsWith('\n'), 'the interpreter to start'); + const pid = Number(readFileSync(pidFile, 'utf8')); assert.strictEqual(runner.busy, true); runner.abort('test abort'); const result = await pending; assert.strictEqual(result.aborted, true); assert.strictEqual(result.stderr, 'script aborted by host: test abort'); assert.strictEqual(result.stdout, ''); + // Left running, the stand-in would sleep on for 30 s. + await until(() => !alive(pid), 'the starting interpreter to be killed'); } finally { runner.dispose(); never.done(); } }); - it('an exec issued on the tick an abort settles the first runs whole in a fresh interpreter', async () => { + it('a second exec, issued on the tick an abort settles the first, runs whole in a fresh interpreter', async () => { const runner = new PyRunner({ onToolCall: async () => '' }); try { const first = runner.exec('import asyncio\nawait asyncio.sleep(2)', []); @@ -447,7 +474,7 @@ describe('PyRunner: an abort or dispose while the interpreter starts (real pytho assert.match(result.stderr, says); // Waiting on the killed interpreter's readiness would take its 15 s start timeout. const took = Date.now() - started; - assert.ok(took < 5_000, `settled after ${took} ms`); + assert.ok(took < 10_000, `settled after ${took} ms`); }); } @@ -470,14 +497,17 @@ describe('PyRunner: an abort or dispose while the interpreter starts (real pytho } }); - it("an exec whose interpreter stops reading its input once it's ready throws nothing, and settles at its exit", posix, async () => { - // The script's own writes, the init and the exec, meet a pipe nobody reads. - const closes = standIn(`exec 0<&-\necho '{"op":"ready"}'\nexec sleep 1`); + it("an exec whose interpreter has stopped reading its input but runs on is settled at the failed write", posix, async () => { + // As after a script closes file descriptor 0: the interpreter is alive, + // and the exec's own writes, the init and the exec, meet a pipe nobody + // reads. Left running, the exec would wait out its 10-minute time limit. + const closes = standIn(`exec 0<&-\necho '{"op":"ready"}'\nexec sleep 30`); const runner = new PyRunner({ pythonPath: closes.path, onToolCall: async () => '' }); try { - const result = await runner.exec('print(1)', []); + const result = await within(runner.exec('print(1)', []), 'the exec to settle'); assert.strictEqual(result.aborted, true); - assert.strictEqual(result.stderr, 'python runtime exited unexpectedly (code=0, signal=none)'); + assert.match(result.stderr, /^python runtime stopped reading its input, so it was killed/); + assert.strictEqual(runner.busy, false); await turns(); } finally { runner.dispose(); @@ -507,7 +537,7 @@ describe('PyRunner: an abort or dispose while the interpreter starts (real pytho const pending = runner.exec('print(1)', []); if (stop === 'abort') runner.abort('stopped on the spawn tick'); else runner.dispose(); - const result = await pending; + const result = await within(pending, `the exec stopped by ${stop}`); assert.strictEqual(result.aborted, true, stop); assert.strictEqual( result.stderr, From 90409f34913324b87082a88d466c82e529025e78 Mon Sep 17 00:00:00 2001 From: Ansel-2428 Date: Sat, 17 Oct 2026 16:20:40 -0400 Subject: [PATCH 4/4] fix(code-execution): put the time-limit note on a line of its own, and name the closed-input race in the changelog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit settlePending appends "script stopped: it reached its … time limit\n" to the stderr (and a background script's tail) of an exec that its deadline stopped. It joined the note to the text as it was, so after the runner's own texts, which end without a newline, the two ran together on one line: "python runtime exited unexpectedly (code=3, signal=none)script stopped: …" on main, and this branch's "… (file descriptor 0) does this.script stopped: …" in the closed-input race below. The note now starts on a line of its own after a text that doesn't end with a newline; an empty text gets the note alone, as before. The race (found while saving the PR's receipts, Ada-1017's call room-203 #133952): after a script's os.close(0), 13 runs at 11f0f64 on the VPS (Node 24.22.0 and 20.20.2; af-f71-checks/probes-race-11f0f647.txt and probes-ebadf-11f0f647.txt): - 7: the next exec ran, and the one after settled within 5 ms with the stopped-reading text; - 4: the next exec settled within 3 ms with that text, without running; - 2: the next exec's message was written without an error and never ran, so it waited out its 3 s limit, then settled with that text and the time-limit note (3004 and 3008 ms). Why the message in the last case is lost is not shown. Its cause, the runtime's protocol sharing fds 0 and 1 with every script, is main's, and this change leaves it as it is (af-f71-checks/probes-protocol-fds.txt). The changelog fragment's closed-input bullet now says that such a script sometimes settles only at its time limit. Test: "the time-limit note starts on a line of its own after an interpreter exit, whose text ends without one" (the real runtime; a script whose SIGINT handler exits the interpreter with code 3, stopped at a 1 s limit). With settlePending's join as at 11f0f64, it fails with the run-together text. code-execution.test.js, 61 tests, passes on Node 24.22.0 and 20.20.2 at this tree. --- ...-code-execution-abort-while-starting.fixed.md | 3 ++- src/code-execution/py-runner.ts | 4 +++- test/code-execution.test.ts | 16 ++++++++++++++++ 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/changelog.d/fix-code-execution-abort-while-starting.fixed.md b/changelog.d/fix-code-execution-abort-while-starting.fixed.md index ac1ebcec8..046fd5e4e 100644 --- a/changelog.d/fix-code-execution-abort-while-starting.fixed.md +++ b/changelog.d/fix-code-execution-abort-while-starting.fixed.md @@ -1,3 +1,4 @@ - A `code_execution` script whose interpreter is still starting is now stopped by `PyRunner.abort()` and `dispose()`, as a running one is, and so is one aborted on the tick `exec` is called. The exec settles at once, and the script never runs. Before, an abort in that window did nothing, though `busy` was already true: when a script's turn ended or was reset to run again in that window, the script ran to its end or its time limit, calling tools as it went, and each `code_execution` call of the agent's later turns was refused as another script already running until then. A dispose there, as a background script's cancel does, waited out the interpreter's 15 s start timeout and then reported that the runtime had failed to start. The interpreter starts at an agent's first script, after it has been idle, and after a script is aborted, killed at its time limit or crashes. -- A `code_execution` interpreter that stops reading its input, as one does after a script closes its standard input (`os.close(0)`), is now ended at the first message that can't reach it, and the script waiting on it settles at once, saying the runtime stopped reading its input. That message's failed write was thrown in the host as an uncaught exception (`write EPIPE`), which ended the host's process. An interpreter that died before its exit was handled still settles with its own exit code. +- A `code_execution` interpreter that stops reading its input, as one does after a script closes its standard input (`os.close(0)`), is now ended at the first message that can't reach it, and the script waiting on it settles at once, saying the runtime stopped reading its input. That message's failed write was thrown in the host as an uncaught exception (`write EPIPE`), which ended the host's process. After a script's `os.close(0)`, the next script's message is sometimes written without an error and never runs, and that script settles this way only at its time limit. An interpreter that died before its exit was handled still settles with its own exit code. +- A `code_execution` script stopped at its time limit now reads the note "script stopped: it reached its … time limit" on a line of its own when the text before it doesn't end with a newline, as the runner's own texts don't, such as "python runtime exited unexpectedly (…)". The note used to run on at the end of that text's line. - An abort or a dispose on the tick `exec` is called, when the interpreter couldn't be spawned, no longer signals another process. The runner killed the failed spawn's child, which has no pid, in the tick before Node's `error` for it, and Node then signals whatever pid the child's handle held: in a fresh Node 20 process that was 0, so `SIGKILL` went to the host's own process group. diff --git a/src/code-execution/py-runner.ts b/src/code-execution/py-runner.ts index 8528bbad4..3b37f553a 100644 --- a/src/code-execution/py-runner.ts +++ b/src/code-execution/py-runner.ts @@ -474,8 +474,10 @@ export class PyRunner { // and finished its work returns 0, and must not read as stopped. if (pending.deadlineMs !== null && result.returnCode !== 0) { const note = `script stopped: it reached its ${formatLimit(pending.deadlineMs)} time limit\n`; + // On a line of its own: the runner's own texts, such as an interpreter's exit, end without a newline. + const withNote = (text: string) => (text === '' || text.endsWith('\n') ? text : `${text}\n`) + note; // A background script reports its output tail, so the note goes there too. - result = { ...result, stderr: result.stderr + note, ...(result.tail !== undefined ? { tail: result.tail + note } : {}) }; + result = { ...result, stderr: withNote(result.stderr), ...(result.tail !== undefined ? { tail: withNote(result.tail) } : {}) }; } if (pending.deadlineTimer) clearTimeout(pending.deadlineTimer); if (pending.killTimer) clearTimeout(pending.killTimer); diff --git a/test/code-execution.test.ts b/test/code-execution.test.ts index fd57fcb70..0b0093d75 100644 --- a/test/code-execution.test.ts +++ b/test/code-execution.test.ts @@ -1630,4 +1630,20 @@ describe('code_execution time limit vs blocking code (real python3)', { skip: pr runner.dispose(); } }); + + it('the time-limit note starts on a line of its own after an interpreter exit, whose text ends without one', async () => { + const runner = new PyRunner({ onToolCall: async () => '' }); + try { + // The interrupt at the limit ends the interpreter, so the exit handler settles the exec. + const code = 'import os, signal, time\nsignal.signal(signal.SIGINT, lambda *_: os._exit(3))\ntime.sleep(30)'; + const result = await runner.exec(code, [], undefined, { deadlineMs: 1000 }); + assert.strictEqual(result.returnCode, 1); + assert.strictEqual( + result.stderr, + 'python runtime exited unexpectedly (code=3, signal=none)\nscript stopped: it reached its 1s time limit\n', + ); + } finally { + runner.dispose(); + } + }); });