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..046fd5e4e --- /dev/null +++ b/changelog.d/fix-code-execution-abort-while-starting.fixed.md @@ -0,0 +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. 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 817e38343..3b37f553a 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,12 @@ 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 (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; @@ -302,16 +318,38 @@ export class PyRunner { console.error(`[pytc:${this.label}] ${chunk.trimEnd()}`); }); + // 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) => { // Spawn failure lands here (e.g. python3 missing); readiness rejects. console.error(`[pytc:${this.label}] python process error: ${err.message}`); }); 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'})`, + 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, }); @@ -344,10 +382,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 +395,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 }; @@ -432,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); @@ -492,6 +536,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,13 +556,19 @@ export class PyRunner { this.child = null; this.childReady = null; this.readyResolver = null; + this.readyFailer = null; child.removeAllListeners('exit'); try { child.stdin.end(); } 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 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/src/framework.ts b/src/framework.ts index 5b4f3785f..d5879ace8 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 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 5deec59aa..0b0093d75 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 { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { ChildProcess, execFileSync } from 'node:child_process'; +import { chmodSync, existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import type { @@ -32,6 +32,34 @@ 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)); + } +}; + +/** 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']); @@ -229,20 +257,37 @@ 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('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(); } @@ -264,8 +309,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,17 +346,233 @@ 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; 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, 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[] = []; + const runner = new PyRunner({ + onToolCall: async (name) => { + calls.push(name); + return 'echo'; + }, + }); + 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'); + 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/); + // Waiting on the killed interpreter's readiness would take its 15 s start timeout. + const took = Date.now() - started; + 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'); + assert.deepStrictEqual(calls, [], 'the aborted script never ran'); + } finally { + runner.dispose(); + } + }); + + 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")', []); + 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('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)', []); + 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/); + // 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'); + } 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 < 10_000, `settled after ${took} ms`); + }); + } + + 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 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 within(runner.exec('print(1)', []), 'the exec to settle'); + assert.strictEqual(result.aborted, true); + assert.match(result.stderr, /^python runtime stopped reading its input, so it was killed/); + assert.strictEqual(runner.busy, false); + 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 within(pending, `the exec stopped by ${stop}`); + 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 { + 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)', () => { - 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: { @@ -1369,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(); + } + }); });