From e130c736e4b2b9605d5f4227440560694aea7db3 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 7 Aug 2026 21:59:11 -0400 Subject: [PATCH 1/2] fix: report session-workspace failures faithfully and prime inline files atomically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three fixes to how session mode reports and protects workspace state, all found reviewing the subtree import of this code into a downstream monorepo. Inline priming destroyed before it wrote. writeFile unlinked the destination and then wrote the replacement, so a write that failed partway (ENOSPC/EIO) left the previous turn's bytes gone and the workspace dirty — forcing a recycle/restore that loses warm state when no checkpoint exists yet. By-reference priming already avoids exactly this: it keeps a regular file in place and lets a rename replace it atomically. Inline priming now does the same, and still clears a squatting symlink or directory so a prior turn cannot redirect the write. Checkpoint/restore collapsed two different bind failures into one generic 409. A missing header (a caller error) and a REJECTED bind (this runner is pinned to a different session, so it must be recycled) both answered "Missing runtime session header" with no error code, leaving the control plane unable to see the conflict. The conflict case now returns the same session_workspace_dirty signal /execute already uses for this condition, so an older service fronting a newer runner still recycles the VM. A malformed or duplicated header now returns 400 instead of propagating SessionWorkspaceBindingError out of the route. A validation failure after priming reported the workspace as dirty. Any error thrown once priming completed took the dirty branch, so a ValidationError — a deterministic rejection of the request, after which nothing ran — answered session_workspace_dirty instead of 400. That cost a needless restore and hid the reason the caller needed to fix its request. Each fix is covered by a test that fails without it. --- api/src/api/v2-checkpoint-binding.test.ts | 123 ++++++++++++++++++++++ api/src/api/v2-session-binding.test.ts | 32 ++++++ api/src/api/v2.ts | 61 ++++++++--- api/src/inline-prime-atomicity.test.ts | 119 +++++++++++++++++++++ api/src/job.ts | 30 ++++-- 5 files changed, 346 insertions(+), 19 deletions(-) create mode 100644 api/src/api/v2-checkpoint-binding.test.ts create mode 100644 api/src/inline-prime-atomicity.test.ts diff --git a/api/src/api/v2-checkpoint-binding.test.ts b/api/src/api/v2-checkpoint-binding.test.ts new file mode 100644 index 0000000..894a08b --- /dev/null +++ b/api/src/api/v2-checkpoint-binding.test.ts @@ -0,0 +1,123 @@ +import { afterAll, afterEach, beforeAll, describe, expect, test } from 'bun:test'; +import express from 'express'; +import * as fsp from 'fs/promises'; +import type { Server } from 'http'; +import * as os from 'os'; +import * as path from 'path'; +import { config } from '../config'; +import { bindSessionWorkspace, resetSessionWorkspaceStateForTests } from '../session-workspace'; +import v2Router from './v2'; + +/** + * The checkpoint/restore routes bind the session straight off the header, and + * the control plane acts on WHICH way that bind failed: a missing header is a + * caller error, while a rejected bind means this runner is pinned to a + * different session and must be recycled. Both used to answer with the same + * generic 409, so a real conflict was invisible and the VM stayed in service. + */ + +let server: Server; +let baseUrl: string; +let packageDir: string; +const savedSessionWorkspaceEnabled = config.session_workspace_enabled; + +beforeAll(async () => { + packageDir = await fsp.mkdtemp(path.join(os.tmpdir(), 'checkpoint-binding-')); + const app = express(); + app.use('/api/v2', v2Router); + await new Promise((resolve) => { + server = app.listen(0, '127.0.0.1', () => resolve()); + }); + const address = server.address(); + baseUrl = `http://127.0.0.1:${typeof address === 'object' && address ? address.port : 0}`; +}); + +afterAll(async () => { + await new Promise((resolve) => server.close(() => resolve())); + await fsp.rm(packageDir, { recursive: true, force: true }); +}); + +afterEach(() => { + config.session_workspace_enabled = savedSessionWorkspaceEnabled; + resetSessionWorkspaceStateForTests(); +}); + +const checkpoint = (headers: Record = {}) => + fetch(`${baseUrl}/api/v2/session/checkpoint`, { headers }); + +const restore = (headers: Record = {}) => + fetch(`${baseUrl}/api/v2/session/restore`, { + method: 'POST', + headers: { 'Content-Type': 'application/x-gtar', ...headers }, + body: 'not-a-real-archive', + }); + +describe('checkpoint/restore session binding', () => { + test('a missing header stays a plain 409 without the recycle signal', async () => { + config.session_workspace_enabled = true; + + for (const response of [await checkpoint(), await restore()]) { + expect(response.status).toBe(409); + const body = (await response.json()) as { error?: string; message?: string }; + expect(body.message).toBe('Missing runtime session header'); + /* A caller that simply forgot the header must NOT trigger a VM recycle. */ + expect(body.error).toBeUndefined(); + } + }); + + test('a bind rejected by a different bound session reports the recycle signal', async () => { + config.session_workspace_enabled = true; + expect(bindSessionWorkspace({ runtimeSessionId: 'rt_already_bound' })).toBeDefined(); + + for (const response of [ + await checkpoint({ 'X-Runtime-Session-Id': 'rt_other' }), + await restore({ 'X-Runtime-Session-Id': 'rt_other' }), + ]) { + expect(response.status).toBe(409); + const body = (await response.json()) as { error?: string; message?: string }; + /* Same code /execute returns for this condition, so an older service + * fronting a newer runner still recycles the VM. */ + expect(body.error).toBe('session_workspace_dirty'); + expect(body.message).toBe('Runner is bound to a different runtime session'); + } + }); + + test('a malformed header is a 400, not a session conflict', async () => { + config.session_workspace_enabled = true; + + const response = await checkpoint({ 'X-Runtime-Session-Id': 'not a valid id!' }); + expect(response.status).toBe(400); + const body = (await response.json()) as { error?: string; message?: string }; + expect(body.error).toBeUndefined(); + expect(body.message).toContain('malformed'); + }); + + test('a duplicated header is a 400 rather than an unhandled throw', async () => { + config.session_workspace_enabled = true; + + /* Node joins repeated occurrences of this header into one comma-separated + * string rather than surfacing an array, so the value fails the id pattern + * instead of the appears-once check. Either way it must be a caller error, + * never an unhandled throw or a session-conflict signal. */ + const response = await fetch(`${baseUrl}/api/v2/session/checkpoint`, { + headers: [ + ['X-Runtime-Session-Id', 'rt_one'], + ['X-Runtime-Session-Id', 'rt_two'], + ] as unknown as HeadersInit, + }); + expect(response.status).toBe(400); + const body = (await response.json()) as { error?: string; message?: string }; + expect(body.error).toBeUndefined(); + expect(body.message).toMatch(/malformed|exactly once/); + }); + + test('a matching header binds and proceeds past the gate', async () => { + config.session_workspace_enabled = true; + expect(bindSessionWorkspace({ runtimeSessionId: 'rt_same' })).toBeDefined(); + + const response = await checkpoint({ 'X-Runtime-Session-Id': 'rt_same' }); + /* Whatever the handler then does, it must not be rejected by the gate. */ + expect(response.status).not.toBe(409); + expect(response.status).not.toBe(400); + }); +}); diff --git a/api/src/api/v2-session-binding.test.ts b/api/src/api/v2-session-binding.test.ts index 4e183a6..a3c8098 100644 --- a/api/src/api/v2-session-binding.test.ts +++ b/api/src/api/v2-session-binding.test.ts @@ -7,6 +7,7 @@ import * as path from 'path'; import { config } from '../config'; import { Job } from '../job'; import { loadPackage } from '../runtime'; +import { ValidationError } from '../validation'; import { getBoundSessionWorkspace, resetSessionWorkspaceStateForTests, @@ -130,4 +131,35 @@ describe('per-request session binding', () => { Job.prototype.cleanup = originalCleanup; } }); + + test('a validation failure after priming answers 400 instead of the recycle signal', async () => { + config.session_workspace_enabled = true; + config.require_execution_manifest = false; + + const originalPrime = Job.prototype.prime; + const originalExecute = Job.prototype.execute; + const originalCleanup = Job.prototype.cleanup; + + /* Priming succeeds, then execution rejects the REQUEST. Nothing ran, so the + * workspace holds exactly what priming wrote — reporting + * `session_workspace_dirty` here would cost a needless restore and hide the + * 400 the caller needs to fix its request. */ + Job.prototype.prime = async function primeWithoutFilesystem(): Promise {}; + Job.prototype.execute = async function executeRejectingRequest() { + throw new ValidationError('files must include at least one utf8 encoded file'); + }; + Job.prototype.cleanup = async function cleanupWithoutFilesystem(): Promise {}; + + try { + const response = await execute('rt_validation_after_prime'); + expect(response.status).toBe(400); + const body = (await response.json()) as { error?: string; message?: string }; + expect(body.error).toBeUndefined(); + expect(body.message).toBe('files must include at least one utf8 encoded file'); + } finally { + Job.prototype.prime = originalPrime; + Job.prototype.execute = originalExecute; + Job.prototype.cleanup = originalCleanup; + } + }); }); diff --git a/api/src/api/v2.ts b/api/src/api/v2.ts index 895fec6..94b1b8c 100644 --- a/api/src/api/v2.ts +++ b/api/src/api/v2.ts @@ -543,6 +543,16 @@ router.post('/execute', express.json({ limit: config.execute_body_limit }), asyn metricsOutcome = 'success'; return res.status(200).json(result); } catch (error) { + /* A ValidationError is a deterministic rejection of the REQUEST, not + * evidence that the workspace is in an unknown state: nothing ran, so the + * workspace holds exactly the inputs priming wrote. Treating it as dirty + * (below) answered a caller error with `session_workspace_dirty`, which + * the control plane reads as a recycle signal — costing a restore and + * hiding the 400 the caller needed to fix its request. */ + if (error instanceof ValidationError) { + metricsOutcome = 'validation_error'; + return res.status(400).json({ message: error.message }); + } if (primeCompleted && job?.markSessionDirty('execution failed after input priming')) { metricsOutcome = 'execution_error'; logger.error({ job: job.uuid, err: error }, 'Session execution left workspace state unknown'); @@ -624,28 +634,55 @@ router.get('/runtimes', (_req: Request, res: Response) => { * silently continues with an empty workspace (checkpoint state lost on expiry). * Returns false (→ fail closed) when this request carries no valid header, so a * headerless/malformed request never operates on a stale prior session. */ -function bindSessionFromHeader(req: Request): boolean { - const binding = parseSessionBindingFromHeader(req.headers[RUNTIME_SESSION_ID_HEADER]); - if (!binding) return false; - /* A REJECTED bind (this runner is already pinned to a different session) - * must fail the request too — proceeding would run checkpoint/restore/file - * delivery for the requested session against the previously bound session's - * workspace. */ - return bindSessionWorkspace(binding) != null; +type SessionBindFailure = { status: number; body: Record }; + +/* Distinguishes the two ways a bind can fail, because the control plane acts on + * them differently: a missing/malformed header is a caller error, while a + * REJECTED bind means this runner is pinned to a DIFFERENT session and must be + * recycled. Collapsing both into one generic 409 (as this did) hid the conflict + * and left the VM in service. `session_workspace_dirty` is the established + * recycle signal — the same code /execute returns for this condition. */ +function bindSessionFromHeader(req: Request): SessionBindFailure | null { + let binding; + try { + binding = parseSessionBindingFromHeader(req.headers[RUNTIME_SESSION_ID_HEADER]); + } catch (error) { + /* A duplicated or malformed header throws SessionWorkspaceBindingError; it + * is a bad request, not a session conflict. */ + return { + status: 400, + body: { message: error instanceof Error ? error.message : 'Invalid runtime session header' }, + }; + } + if (!binding) { + return { status: 409, body: { message: 'Missing runtime session header' } }; + } + if (bindSessionWorkspace(binding) == null) { + return { + status: 409, + body: { + error: 'session_workspace_dirty', + message: 'Runner is bound to a different runtime session', + }, + }; + } + return null; } /* Express 4 (pinned) does NOT auto-forward rejected route-handler promises, so * `.catch(next)` is required or a rejection (e.g. session.ownership()) hangs the * request and surfaces as an unhandled rejection instead of a 5xx. */ router.get('/session/checkpoint', (req: Request, res: Response, next: NextFunction) => { - if (!bindSessionFromHeader(req)) { - return res.status(409).json({ message: 'Missing runtime session header' }); + const failure = bindSessionFromHeader(req); + if (failure) { + return res.status(failure.status).json(failure.body); } return streamSessionCheckpoint(res).catch(next); }); router.post('/session/restore', (req: Request, res: Response, next: NextFunction) => { - if (!bindSessionFromHeader(req)) { - return res.status(409).json({ message: 'Missing runtime session header' }); + const failure = bindSessionFromHeader(req); + if (failure) { + return res.status(failure.status).json(failure.body); } return restoreSessionCheckpoint(req, res).catch(next); }); diff --git a/api/src/inline-prime-atomicity.test.ts b/api/src/inline-prime-atomicity.test.ts new file mode 100644 index 0000000..ce85ebf --- /dev/null +++ b/api/src/inline-prime-atomicity.test.ts @@ -0,0 +1,119 @@ +import { afterEach, describe, expect, spyOn, test } from 'bun:test'; +import * as fsp from 'fs/promises'; +import * as os from 'os'; +import * as path from 'path'; +import { Job, type TFile } from './job'; +import { SessionWorkspace } from './session-workspace'; + +/** + * Inline (`content`) priming must protect prior session bytes the same way + * by-reference priming does. The by-ref path leaves an existing regular file in + * place and lets a rename replace it, so a failed write cannot erase the + * previous turn's state; inline priming used to unlink first and write second, + * which could destroy a file and then fail — forcing a recycle/restore, and + * losing warm workspace state when no checkpoint exists yet. + */ + +let tmpDir: string; + +afterEach(async () => { + if (tmpDir) await fsp.rm(tmpDir, { recursive: true, force: true }).catch(() => {}); +}); + +function makeJob(files: TFile[], session?: object): Job { + return new Job({ + session_id: 'inline-prime-test', + runtime: { language: 'bash', version: '5.0.0', aliases: [], runtime: 'bash' } as never, + args: [], + stdin: '', + files, + timeouts: { run: 5000, compile: 5000 }, + cpu_times: { run: 5000, compile: 5000 }, + memory_limits: { run: 128 * 1024 * 1024, compile: 128 * 1024 * 1024 }, + session, + } as never); +} + +const writeInline = (job: Job, file: TFile): Promise => + (job as unknown as { writeFile(f: TFile): Promise }).writeFile(file); + +describe('inline priming in a session workspace', () => { + test('replaces an existing file without unlinking it first', async () => { + tmpDir = await fsp.mkdtemp(path.join(os.tmpdir(), 'inline-prime-')); + const session = new SessionWorkspace({ runtimeSessionId: 'rt_inline_1' }); + const job = makeJob([], session); + (job as unknown as { submissionDir: string }).submissionDir = tmpDir; + + await fsp.writeFile(path.join(tmpDir, 'main.py'), 'print("old")\n'); + await writeInline(job, { name: 'main.py', content: 'print("new")\n' }); + + expect(await fsp.readFile(path.join(tmpDir, 'main.py'), 'utf8')).toBe('print("new")\n'); + /* The temp file used for the atomic swap must not linger in the workspace, + * or the output scan would surface it as a generated file. */ + const leftovers = (await fsp.readdir(tmpDir)).filter((n) => n.startsWith('.tmp-')); + expect(leftovers).toEqual([]); + }); + + test('a failed write leaves the previous turn bytes intact', async () => { + tmpDir = await fsp.mkdtemp(path.join(os.tmpdir(), 'inline-prime-fail-')); + const session = new SessionWorkspace({ runtimeSessionId: 'rt_inline_2' }); + const job = makeJob([], session); + (job as unknown as { submissionDir: string }).submissionDir = tmpDir; + + const target = path.join(tmpDir, 'keep.txt'); + await fsp.writeFile(target, 'previous turn\n'); + + /* Inject the failure at the write itself — the ENOSPC/EIO shape this + * guards against. Note a read-only workspace dir does NOT reproduce it: + * that blocks the unlink too, so the old code never got far enough to + * destroy anything. The write has to fail while the destination is already + * removable for the regression to show. */ + const spy = spyOn(fsp, 'writeFile').mockImplementation(async () => { + throw Object.assign(new Error('no space left on device'), { code: 'ENOSPC' }); + }); + try { + await expect(writeInline(job, { name: 'keep.txt', content: 'replacement\n' })).rejects.toThrow(); + } finally { + spy.mockRestore(); + } + + /* The old rm-then-write order deleted this before failing, forcing a + * recycle/restore that loses warm state when no checkpoint exists. */ + expect(await fsp.readFile(target, 'utf8')).toBe('previous turn\n'); + }); + + test('still clears a squatting symlink instead of following it', async () => { + tmpDir = await fsp.mkdtemp(path.join(os.tmpdir(), 'inline-prime-link-')); + const session = new SessionWorkspace({ runtimeSessionId: 'rt_inline_3' }); + const job = makeJob([], session); + (job as unknown as { submissionDir: string }).submissionDir = tmpDir; + + const outside = path.join(tmpDir, 'outside-target.txt'); + await fsp.writeFile(outside, 'must not be clobbered\n'); + await fsp.symlink(outside, path.join(tmpDir, 'link.txt')); + + await writeInline(job, { name: 'link.txt', content: 'fresh regular file\n' }); + + const stat = await fsp.lstat(path.join(tmpDir, 'link.txt')); + expect(stat.isSymbolicLink()).toBe(false); + expect(await fsp.readFile(path.join(tmpDir, 'link.txt'), 'utf8')).toBe('fresh regular file\n'); + /* The symlink target is untouched — writeFile never followed the link. */ + expect(await fsp.readFile(outside, 'utf8')).toBe('must not be clobbered\n'); + }); + + test('a directory squatting the destination is still removed', async () => { + tmpDir = await fsp.mkdtemp(path.join(os.tmpdir(), 'inline-prime-dir-')); + const session = new SessionWorkspace({ runtimeSessionId: 'rt_inline_4' }); + const job = makeJob([], session); + (job as unknown as { submissionDir: string }).submissionDir = tmpDir; + + await fsp.mkdir(path.join(tmpDir, 'squat.txt')); + await fsp.writeFile(path.join(tmpDir, 'squat.txt', 'inner.txt'), 'inner\n'); + + await writeInline(job, { name: 'squat.txt', content: 'now a file\n' }); + + const stat = await fsp.lstat(path.join(tmpDir, 'squat.txt')); + expect(stat.isFile()).toBe(true); + expect(await fsp.readFile(path.join(tmpDir, 'squat.txt'), 'utf8')).toBe('now a file\n'); + }); +}); diff --git a/api/src/job.ts b/api/src/job.ts index 1193469..04fd5c2 100644 --- a/api/src/job.ts +++ b/api/src/job.ts @@ -1447,13 +1447,29 @@ export class Job { else await fsp.mkdir(parentDir, { recursive: true }); await this.secureAncestors(parentDir, operation.submissionDir, operation.identity); throwIfAborted(operation.signal); - /* In a persistent session workspace a prior turn could have left a symlink - * (or a directory) squatting this path; the default writeFile would follow - * the symlink and clobber its target as root. Remove whatever is there - * first (unlink never follows a link) so we always write a fresh regular - * file. A fresh per-job workspace has nothing here. */ - if (this.session) await fsp.rm(filePath, { force: true, recursive: true }); - await fsp.writeFile(filePath, content); + if (this.session) { + /* Mirrors the by-reference prime above. A prior turn could have left a + * symlink (or directory) squatting this path, and writeFile would follow + * the link and clobber its target as root — so clear anything that is + * NOT a regular file (unlink never follows a link). A regular file is + * LEFT in place and replaced by the rename below, so a write that fails + * partway cannot erase the previous turn's bytes and force a + * recycle/restore. A fresh per-job workspace has nothing here. */ + const existing = await fsp.lstat(filePath).catch(() => null); + if (existing && !existing.isFile()) { + await fsp.rm(filePath, { force: true, recursive: true }); + } + const tempPath = path.join(operation.submissionDir, `.tmp-${nanoid()}`); + try { + await fsp.writeFile(tempPath, content, { mode: SANDBOX_FILE_MODE }); + await fsp.rename(tempPath, filePath); + } catch (error) { + try { await fsp.unlink(tempPath); } catch { /* may not exist */ } + throw error; + } + } else { + await fsp.writeFile(filePath, content); + } await this.applySandboxFilePermissions(filePath, false, operation.identity); const hash = crypto.createHash('sha256').update(content).digest('hex'); From e77db43998b5fb6b4f86c4ee059f870a224b002f Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 8 Aug 2026 07:24:59 -0400 Subject: [PATCH 2/2] fix: reject unrunnable requests before priming instead of after MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review feedback on the previous commit. Rejecting a request with no runnable source AFTER priming is too late, and answering it with a clean 400 was worse than the behavior it replaced. `getJob`'s gate accepted any utf8 file, including the `.dirkeep` sentinel, while `Job.execute` required a utf8 file that is NOT `.dirkeep`. A request carrying only `.dirkeep` plus binary inputs therefore passed the gate, reached `prime()` — replacing files in the session workspace and recording priming metadata — and only then failed. Session cleanup deliberately preserves the workspace, so the rejected request's writes stayed visible to the next execution. The previous commit made that case return 400 by moving the ValidationError branch above the dirty branch, which traded a false-dirty for a false-clean: the workspace really had been written to. Both checks now call a single `hasRunnableSource` predicate, and the request gate runs it before any Job is built, so such a request is rejected without touching the workspace. The catch block is restored to its original order: once priming has completed, any later failure — validation included — reports the workspace as dirty, which is the honest answer. Tests: a request with nothing runnable is rejected with prime() never called, and a genuine post-prime failure still reports dirty. Both fail without this change. --- api/src/api/v2-session-binding.test.ts | 72 ++++++++++++++++++++++---- api/src/api/v2.ts | 30 +++++------ api/src/job.ts | 6 ++- api/src/validation.ts | 21 ++++++++ 4 files changed, 103 insertions(+), 26 deletions(-) diff --git a/api/src/api/v2-session-binding.test.ts b/api/src/api/v2-session-binding.test.ts index a3c8098..1e85100 100644 --- a/api/src/api/v2-session-binding.test.ts +++ b/api/src/api/v2-session-binding.test.ts @@ -132,7 +132,7 @@ describe('per-request session binding', () => { } }); - test('a validation failure after priming answers 400 instead of the recycle signal', async () => { + test('a request with nothing runnable is rejected BEFORE anything is primed', async () => { config.session_workspace_enabled = true; config.require_execution_manifest = false; @@ -140,22 +140,74 @@ describe('per-request session binding', () => { const originalExecute = Job.prototype.execute; const originalCleanup = Job.prototype.cleanup; - /* Priming succeeds, then execution rejects the REQUEST. Nothing ran, so the - * workspace holds exactly what priming wrote — reporting - * `session_workspace_dirty` here would cost a needless restore and hide the - * 400 the caller needs to fix its request. */ - Job.prototype.prime = async function primeWithoutFilesystem(): Promise {}; - Job.prototype.execute = async function executeRejectingRequest() { - throw new ValidationError('files must include at least one utf8 encoded file'); + let primed = false; + Job.prototype.prime = async function trackPrime(): Promise { + primed = true; + }; + Job.prototype.execute = async function executeWithoutSandbox() { + return {} as Awaited>; }; Job.prototype.cleanup = async function cleanupWithoutFilesystem(): Promise {}; try { - const response = await execute('rt_validation_after_prime'); + /* A lone `.dirkeep` plus a binary input satisfied the old gate (it only + * asked for "some utf8 file"), so the request primed its writes into the + * session workspace and only then failed the stricter check inside + * execute. Session cleanup deliberately preserves the workspace, so those + * writes stayed visible to the next execution. */ + const response = await fetch(`${baseUrl}/api/v2/execute`, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'X-Runtime-Session-Id': 'rt_nothing_runnable', + }, + body: JSON.stringify({ + language: testLanguage, + version: testVersion, + files: [ + { name: '.dirkeep', content: '' }, + { name: 'blob.bin', content: 'AA==', encoding: 'base64' }, + ], + }), + }); + expect(response.status).toBe(400); const body = (await response.json()) as { error?: string; message?: string }; expect(body.error).toBeUndefined(); - expect(body.message).toBe('files must include at least one utf8 encoded file'); + expect(body.message).toContain('runnable source file'); + /* The point of the fix: the workspace was never touched. */ + expect(primed).toBe(false); + } finally { + Job.prototype.prime = originalPrime; + Job.prototype.execute = originalExecute; + Job.prototype.cleanup = originalCleanup; + } + }); + + test('a post-prime failure still reports the workspace as dirty', async () => { + config.session_workspace_enabled = true; + config.require_execution_manifest = false; + + const originalPrime = Job.prototype.prime; + const originalExecute = Job.prototype.execute; + const originalCleanup = Job.prototype.cleanup; + + Job.prototype.prime = async function primeWithoutFilesystem(): Promise {}; + Job.prototype.execute = async function executeFailingAfterPrime() { + throw new ValidationError('files must include at least one runnable source file'); + }; + Job.prototype.cleanup = async function cleanupWithoutFilesystem(): Promise {}; + + try { + /* Once priming has written to the workspace, ANY later failure leaves + * state the next execute must not inherit silently — so the dirty signal + * outranks the 400 here, even for a validation error. */ + const response = await execute('rt_dirty_after_prime'); + expect(response.status).toBe(409); + expect(await response.json()).toEqual({ + error: 'session_workspace_dirty', + message: 'Session workspace must be restored before another execute', + }); } finally { Job.prototype.prime = originalPrime; Job.prototype.execute = originalExecute; diff --git a/api/src/api/v2.ts b/api/src/api/v2.ts index 94b1b8c..b489573 100644 --- a/api/src/api/v2.ts +++ b/api/src/api/v2.ts @@ -8,6 +8,7 @@ import { Job, SessionWorkspaceDirtyError, ValidationError, + hasRunnableSource, validateFilePath, } from '../job'; import { EXECUTION_MANIFEST_HEADER, ExecutionManifestError, type ExecutionManifestClaims } from '../execution-manifest'; @@ -267,11 +268,14 @@ function getJob( throw { message: `${language}-${version} runtime is unknown` }; } - if ( - rt.language !== 'file' && - !files.some(file => !file.encoding || file.encoding === 'utf8') - ) { - throw { message: 'files must include at least one utf8 encoded file' }; + /* Reject a request with nothing runnable BEFORE anything is primed. This + * gate used to count a lone `.dirkeep` as a utf8 source, so such a request + * reached prime(), had its files written into the session workspace and its + * priming metadata recorded, and only then failed the stricter check in + * Job.execute — leaving the rejected request's writes visible to the next + * execution. Both sides now ask `hasRunnableSource`. */ + if (!hasRunnableSource(files, rt.language)) { + throw { message: 'files must include at least one runnable source file' }; } validateConstraints(body, rt); @@ -543,16 +547,12 @@ router.post('/execute', express.json({ limit: config.execute_body_limit }), asyn metricsOutcome = 'success'; return res.status(200).json(result); } catch (error) { - /* A ValidationError is a deterministic rejection of the REQUEST, not - * evidence that the workspace is in an unknown state: nothing ran, so the - * workspace holds exactly the inputs priming wrote. Treating it as dirty - * (below) answered a caller error with `session_workspace_dirty`, which - * the control plane reads as a recycle signal — costing a restore and - * hiding the 400 the caller needed to fix its request. */ - if (error instanceof ValidationError) { - metricsOutcome = 'validation_error'; - return res.status(400).json({ message: error.message }); - } + /* Deliberately BEFORE the ValidationError branch below: once priming has + * completed, the workspace has been written to, so any later failure — + * including a validation one — leaves state the next execute must not + * inherit silently. Requests with nothing runnable are rejected up front + * (see `hasRunnableSource`), so reaching here with a ValidationError + * means files really were primed and dirty is the honest answer. */ if (primeCompleted && job?.markSessionDirty('execution failed after input priming')) { metricsOutcome = 'execution_error'; logger.error({ job: job.uuid, err: error }, 'Session execution left workspace state unknown'); diff --git a/api/src/job.ts b/api/src/job.ts index 04fd5c2..eceffdf 100644 --- a/api/src/job.ts +++ b/api/src/job.ts @@ -35,6 +35,7 @@ import { SANDBOX_DIR_MODE, SANDBOX_FILE_MODE, ValidationError, + hasRunnableSource, isDirkeep, isValidPathShape, validateFilePath, @@ -47,6 +48,7 @@ export { SANDBOX_DIR_MODE, SANDBOX_FILE_MODE, ValidationError, + hasRunnableSource, isDirkeep, checkPathShape, isValidPathShape, @@ -1533,7 +1535,9 @@ export class Job { const codeFiles = this.files.filter( f => !isDirkeep(f.name) && (!f.encoding || f.encoding === 'utf8'), ); - if (this.runtime.language !== 'file' && codeFiles.length === 0) { + /* The request gate rejects this before priming (see hasRunnableSource); + * this stays as the invariant for callers that build a Job directly. */ + if (!hasRunnableSource(this.files, this.runtime.language)) { throw new ValidationError('files must include at least one runnable source file'); } this.entryPointName = codeFiles[0]?.name; diff --git a/api/src/validation.ts b/api/src/validation.ts index 2e937ba..5b94457 100644 --- a/api/src/validation.ts +++ b/api/src/validation.ts @@ -20,6 +20,27 @@ export function isDirkeep(name: string): boolean { return path.basename(name) === DIRKEEP; } +/** + * Whether a request carries something the runtime can actually run: a utf8 + * source file that is not the .dirkeep sentinel. `file` runtimes are exempt — + * they take arbitrary inputs. + * + * Single source of truth on purpose. The request gate and `Job.execute` used to + * ask this question differently (the gate accepted a lone `.dirkeep` as a utf8 + * file), so a request with only `.dirkeep` plus binary inputs passed the gate, + * primed its files into the session workspace, and only then failed in + * execute — leaving the rejected request's writes behind. + */ +export function hasRunnableSource( + files: Array<{ name: string; encoding?: string }>, + language: string, +): boolean { + if (language === 'file') return true; + return files.some( + (file) => !isDirkeep(file.name) && (!file.encoding || file.encoding === 'utf8'), + ); +} + export class ValidationError extends Error { constructor(message: string) { super(message);