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..1e85100 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,87 @@ describe('per-request session binding', () => { Job.prototype.cleanup = originalCleanup; } }); + + test('a request with nothing runnable is rejected BEFORE anything is primed', 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; + + 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 { + /* 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).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; + Job.prototype.cleanup = originalCleanup; + } + }); }); diff --git a/api/src/api/v2.ts b/api/src/api/v2.ts index 895fec6..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,6 +547,12 @@ router.post('/execute', express.json({ limit: config.execute_body_limit }), asyn metricsOutcome = 'success'; return res.status(200).json(result); } catch (error) { + /* 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'); @@ -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..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, @@ -1447,13 +1449,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'); @@ -1517,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);