diff --git a/docs/implementation/runner-lifecycle.md b/docs/implementation/runner-lifecycle.md index d8211b04..42a58bc6 100644 --- a/docs/implementation/runner-lifecycle.md +++ b/docs/implementation/runner-lifecycle.md @@ -254,7 +254,7 @@ codeboost never writes the user's repository. The commits it makes live in a **r | 1 | Set `stopping` on the server and make the runner coordinator reject admission, in the same synchronous turn, before any active-work list is copied. New API requests get HTTP 503, and new runner work throws `ShuttingDownError`. **Admitted requests are not rejected:** AGENTS.md says to drain already-admitted HTTP requests, and to recheck the flag only at irreversible boundaries. So after reading a body, the server rechecks `stopping` only for `merge`. The question coordinator also stops admission in this turn (`questions.stopAdmission()`): a question admitted before shutdown still saves its note, but starts no agent and gets a retryable "Server stopped" answer (`markStopped`). **Changed while implementing F1c:** an earlier revision of this row rejected every admitted write and closed the Store write gate here. That broke the drain rule and the existing browser regression "drains an in-flight question request before closing its agent manager", so the gate now closes in step 3. | server flag, merge recheck and question admission stop: yes. Runner barrier: new. | | 2 | Stop accepting connections, and wait for admitted requests up to the drain limit (at most 14.5 s, below the 15 s request timeout). | yes | | 2a | Abort every publish and PR close in progress (`TaskPublishing.close()`, #103, #111) in the same turn as step 1, and await it here, at the end of the drain, before the write gate closes: the publisher already refuses every later push and opening through the coordinator's `closing` flag, and a publish's last records (an abandoned opening, the outcome) must still land. A publish stopped this way is owed, so the next start publishes it again. | new (#103) | -| 3 | After the drain limit, abort the signals of the remaining requests, destroy requests that are still reading a body, then **close the `Store` write gate**, then await request-owned work (the merge coordinator). From here on, every `Store` write throws `ShuttingDownError` unless the caller passes the shutdown capability. The server hands that capability at construction only to coordinators' settlement and close code (the runner, questions' `finishAnswer`, merge settlement after an irreversible command, and E3's `settleSuggestion` when it is wired), and to startup recovery; HTTP handlers and `ReviewService` never receive it. So a request-path write still pending after the abort (an aborted `/api/review` whose `load()` reaches `recordHistory`, or merge reconciliation and queue polling after a GitHub await) fails. The server maps `ShuttingDownError` to HTTP 503 "The review server is shutting down.", never to the 409 used for review errors, and there is no partial view. **Today the merge coordinator catches every error in `displayStatus()` and `#pollQueue()`**, which would swallow the gate error and answer 200; F1 makes each of those catch blocks rethrow `ShuttingDownError` first. The next startup checks GitHub again, so a skipped merge observation is recovered. The server also closes idle keep-alive connections once shutdown starts and after each request finishes, so a finished request's socket cannot hold `server.close()` open until its keep-alive timeout. | abort: yes. Gate, capability, rethrow and idle-connection close: new. | +| 3 | After the drain limit, abort the signals of the remaining requests, destroy requests that are still reading a body, then **close the `Store` write gate**, then await request-owned work (the merge coordinator). From here on, every `Store` write throws `ShuttingDownError` unless the caller passes the shutdown capability. The server hands that capability at construction only to coordinators' settlement and close code (the runner, questions' `finishAnswer`, merge settlement after an irreversible command, and E3's `settleSuggestion` when it is wired), and to startup recovery; HTTP handlers and `ReviewService` never receive it. So a request-path write still pending after the abort (an aborted `/api/review` whose `load()` reaches `recordHistory`, or merge reconciliation and queue polling after a GitHub await) fails. The server maps `ShuttingDownError` to HTTP 503 "The review server is shutting down.", never to the 409 used for review errors, and there is no partial view. **Today the merge coordinator catches every error in `displayStatus()` and `#pollQueue()`**, which would swallow the gate error and answer 200; F1 makes each of those catch blocks rethrow `ShuttingDownError` first. The next startup checks GitHub again, so a skipped merge observation is recovered. The server also closes idle keep-alive connections once shutdown starts and after each request finishes, so a finished request's socket cannot hold `server.close()` open until its keep-alive timeout. For the same reason (#157), a reply to a request that declares a body (`Content-Length` above 0, or `Transfer-Encoding`) sent before that body has been read to its end, such as a 403 or 405, carries `Connection: close`. Every other reply keeps the connection. Node discards a body the handler never read, but the connection is busy while it does, so shutdown's idle-connection close skips it and nothing closes it once it goes idle. Node does not discard the rest of a body the handler has started reading, so a reply sent mid-body held the connection until the keep-alive timeout, or reset the client. A refusal that closes the connection can still reset a client sending a large body before it reads the reply; that is accepted, and the UI keeps an import's action ID for a resend. A body over its limit is therefore read to its end and discarded before the 413 goes out, which also means a client still sending is not reset before it reads the reply; a request still sending at the drain limit is destroyed like any other. | abort: yes. Gate, capability, rethrow and idle-connection close: new. | | 4 | For every `pending` and `running` attempt that has an in-memory job, record the first reason `shutdown` **only if no first reason is set yet**. A job already marked `cancelled`, `stale` or `time-limit` keeps its reason. For `running`, call `cancel('shutdown')` and await `settled`. For `pending`, await its preparation promise (the D start call itself returns at once, so it is never in progress), remove only its host-side preparation files (task storage waits for the terminal write, as in step 5), and cancel any handle that start returned, then await its `settled` (the "Launch" table). F does not abandon a job after a timer (decision 4). **This ends:** D's supervisor escalates to a forced kill, and retries unfinished container, network or setup cleanup every second for at most 60 seconds after the first failure (`CLEANUP_RETRY_WINDOW_MS`, `agents/adapters/supervisor.ts`). Retries after the first get only what is left of the window, and every cleanup subprocess is killed at its deadline, so settlement ends about 90 seconds after cleanup starts at worst (up to 30 seconds for the first, failed attempt, then the 60-second window). Then `settled` resolves with a `stopReason` and an `unreleased` list, even if Docker is unreachable or hangs. F records `unreleased` durably before it frees the slot. See the prerequisite below. | new | | 5 | For each attempt from step 4, in the order set by the task-storage holder row: the partial-output export (writable attempts), then the terminal state from its first reason (a `shutdown` reason gives `cancelled` "Stopped by shutdown") with its `diagnostic_ref`, then `removeTaskFilesystems`, and only then free its slot. A failed write leaves the row non-terminal, and a failed removal keeps the slot held. Startup recovery handles both. | new | | 5a | Await every plan run in progress (`ItemExecutor.close()`, #91). A run resumes after its attempt settles and may then pause for amendment or move the task to needs human: settlement writes, made with the capability, that must land before step 7 closes the `Store`. Admission is closed, so no run starts another item. | new (#91) | @@ -495,6 +495,7 @@ Each case needs a test that fails before the fix and passes after it. Each test | Reason write fails (review round 9) | The first-reason write throws → settlement → the terminal write stores `cancelled` with the reason from memory | | Time limit, then crash (review round 9) | `time-limit` saved → process killed → restart → row `cancelled`, task `needs human`, retry refused | | Request still reading after the drain limit (review round 9, revised) | Body half-sent → shutdown → the drain limit passes → the request is destroyed → no `Store` change. A request arriving after shutdown began gets 503 | +| Oversized body (#157) | Body over the limit → the 413 goes out only after the body ends, also during the drain, and shutdown then settles without waiting for the keep-alive timeout. Still sending at the drain limit → destroyed, no reply. A large body refused before it is read (403, 405) → the connection closes and shutdown is prompt | | Stale, then shutdown (review round 9) | Attempt marked `stale` → shutdown → row `stale`, not requeued | | Copied database (review round 9) | Copy the database file → start the copy → new token; recovery for the copy leaves the original's resources alone | | Review edit during the final merge check (review round 10) | Merge passes its GitHub checks → approval changed before the final re-read → merge refused | diff --git a/docs/plan-format.md b/docs/plan-format.md index 24bf73d3..80001313 100644 --- a/docs/plan-format.md +++ b/docs/plan-format.md @@ -95,7 +95,7 @@ questions: [] Import accepts UTF-8 JSON or one YAML 1.2 document representing JSON-compatible data. Reject invalid UTF-8, input above 1 MiB, and nesting deeper than 50 containers before producing a plan. Parse into an intermediate syntax tree with duplicate-key detection; do not convert an unchecked YAML graph into application objects. -`POST /api/plan/import` sends the source as one JSON string in a request body of at most 6 MiB + 16 KiB (6,307,840 bytes, `MAX_IMPORT_REQUEST_BYTES` in `web/server.ts`). Any JSON encoding writes each UTF-8 byte of the source as at most 6 bytes: YAML accepts a raw control character in a quoted scalar, and JSON writes it as `\u0001`. So every source within the 1 MiB parser limit fits, with 16 KiB left for the rest of the request. A larger body is refused with HTTP 413 before it is parsed. A body within the limit whose source is above 1 MiB gets the parser's refusal. Every other `POST` keeps its 16 KiB limit (#146). +`POST /api/plan/import` sends the source as one JSON string in a request body of at most 6 MiB + 16 KiB (6,307,840 bytes, `MAX_IMPORT_REQUEST_BYTES` in `web/server.ts`). Any JSON encoding writes each UTF-8 byte of the source as at most 6 bytes: YAML accepts a raw control character in a quoted scalar, and JSON writes it as `\u0001`. So every source within the 1 MiB parser limit fits, with 16 KiB left for the rest of the request. A larger body is read to its end, then refused with HTTP 413 before it is parsed (#157). A body within the limit whose source is above 1 MiB gets the parser's refusal. Every other `POST` keeps its 16 KiB limit (#146). For both formats, object keys must be strings and unique after decoding at every level; reject duplicates rather than choosing the first or last value. JSON follows RFC 8259 syntax. YAML allows mappings, sequences, strings (including quoted and block strings), the exact plain literals `true`, `false`, and `null`, and numbers spelled using the JSON number grammar. Reject empty implicit values, non-finite values (`.nan`/`.inf`), non-JSON number spellings such as hex/octal or numeric separators, complex/non-string keys, every anchor and alias, merge keys (`<<`), explicit tags (including custom tags), extra documents, and parser warnings/errors before schema validation. Ordinary YAML 1.2 plain strings remain strings; do not infer dates, functions, or application-specific types. Integers used for issue, revision, or indexes must also be exactly representable safe integers in the implementation. diff --git a/test/runner-shutdown.test.ts b/test/runner-shutdown.test.ts index af3c6a2c..15cb4124 100644 --- a/test/runner-shutdown.test.ts +++ b/test/runner-shutdown.test.ts @@ -5,7 +5,7 @@ import { request as httpRequest } from 'node:http'; import { randomUUID } from 'node:crypto'; import { afterEach, describe, expect, it, vi } from 'vitest'; import { createDemo } from '../scripts/demo.ts'; -import { RUNNER_NOT_IN_DEMO, startServer } from '../web/server.ts'; +import { MAX_REQUEST_BYTES, RUNNER_NOT_IN_DEMO, startServer } from '../web/server.ts'; import { Store } from '../runner/store.ts'; import { ReviewService } from '../runner/review.ts'; import { MergeCoordinator } from '../runner/merge.ts'; @@ -28,9 +28,9 @@ afterEach(async () => { const oid = (n: number) => n.toString(16).padStart(40, '0'); function demo() { const root = mkdtempSync(join(tmpdir(), 'codeboost-shutdown-')); roots.push(root); return createDemo(join(root, 'demo')); } type App = Awaited>; -async function serve(deps?: RunnerDeps, agent: QuestionAgent = async () => 'answer') { +async function serve(deps?: RunnerDeps, agent: QuestionAgent = async () => 'answer', drainMs = 2_000) { const config = demo(); - const app = await startServer(config, 0, agent, undefined, 2_000, undefined, deps); + const app = await startServer(config, 0, agent, undefined, drainMs, undefined, deps); let closed = false; const close = async () => { if (!closed) { closed = true; await app.close(); } }; cleanups.push(close); @@ -198,6 +198,115 @@ describe('server shutdown', { timeout: 15_000 }, () => { }); }); +describe('an oversized request body (#157)', { timeout: 30_000 }, () => { + // The drain limit is long, so a connection held open after the reply (until the client or Node's keep-alive timeout + // drops it, about 4–6 s) shows as a slow close instead of being cut at the drain limit. + const drainMs = 10_000, prompt = 3_000, big = 4 * 1024 * 1024; + const serveLong = () => serve(undefined, undefined, drainMs); + /** Starts a POST with `first` bytes of body; `send` writes more, `end` ends it. Records the reply and when it came. */ + function oversized(app: App, first: number, options: { path?: string; token?: string; contentType?: string; length?: number } = {}) { + const url = new URL(origin(app)); + const state: { status?: number; body?: string; connection?: string; error?: Error; endedAt?: number; repliedAt?: number } = {}; + const req = httpRequest({ host: url.hostname, port: url.port, path: options.path ?? '/api/action', method: 'POST', + // Without a length the body is sent chunked; the browser sends a Content-Length. + headers: { 'x-codeboost-token': options.token ?? app.token, 'content-type': options.contentType ?? 'application/json', + ...(options.length === undefined ? {} : { 'content-length': options.length }) } }, res => { + state.repliedAt = performance.now(); state.status = res.statusCode; state.connection = res.headers.connection; + let data = ''; res.on('data', chunk => { data += chunk; }); res.on('end', () => { state.body = data; }); + }); + const settled = new Promise(resolve => { req.on('close', resolve); }); + req.on('error', error => { state.error = error; }); + req.write('x'.repeat(first)); + return { state, settled, send: (bytes: number) => req.write('x'.repeat(bytes)), end: () => { state.endedAt = performance.now(); req.end(); } }; + } + /** Resolves once the server has taken `bytes` off the next connection, so the handler has the body past that point. */ + const serverRead = (app: App, bytes: number) => new Promise(resolve => { + app.server.once('connection', socket => { + const check = setInterval(() => { if (socket.bytesRead >= bytes || socket.destroyed) { clearInterval(check); setImmediate(resolve); } }, 10); + }); + }); + const timed = async (close: () => Promise) => { const started = performance.now(); await close(); return performance.now() - started; }; + const refused = JSON.stringify({ error: 'Request too large.' }); + /** close() ends by closing the Store, so a read through it then fails. */ + const storeClosed = (app: App) => expect(() => app.service.store.questionProvider()).toThrow(); + + it('replies 413 only once the whole body has arrived', async () => { + const { app } = await serveLong(); + const read = serverRead(app, MAX_REQUEST_BYTES * 2), sent = oversized(app, MAX_REQUEST_BYTES * 2); + await read; + await new Promise(resolve => setTimeout(resolve, 300)); + expect(sent.state.status).toBeUndefined(); + sent.send(big); sent.end(); + await sent.settled; + expect(sent.state).toMatchObject({ status: 413, body: refused }); + expect(sent.state.error).toBeUndefined(); + }); + it('shuts down promptly after refusing a body far over the limit', async () => { + const { app, close } = await serveLong(); + // fetch keeps the connection for reuse after the reply, as the browser does. + const response = await fetch(`${origin(app)}/api/action`, { method: 'POST', body: 'x'.repeat(big), + headers: { 'x-codeboost-token': app.token, 'content-type': 'application/json' } }); + expect({ status: response.status, body: await response.text() }).toEqual({ status: 413, body: refused }); + expect(await timed(close)).toBeLessThan(prompt); + storeClosed(app); + }); + it('still replies 413, after the body ends, to an oversized body that completes during the shutdown drain', async () => { + const { app, close } = await serveLong(); + const read = serverRead(app, MAX_REQUEST_BYTES * 2), sent = oversized(app, MAX_REQUEST_BYTES * 2); + await read; + const started = performance.now(), closing = close(); + await tick(); + sent.send(MAX_REQUEST_BYTES * 4); sent.end(); + await sent.settled; + expect(sent.state).toMatchObject({ status: 413, body: refused }); + expect(sent.state.repliedAt).toBeGreaterThanOrEqual(sent.state.endedAt!); + await closing; + expect(performance.now() - started).toBeLessThan(prompt); + }); + it('destroys an oversized request still being sent after the drain limit, and close settles', async () => { + const { app, close } = await serve(); + const read = serverRead(app, MAX_REQUEST_BYTES * 2), sent = oversized(app, MAX_REQUEST_BYTES * 2); + await read; + await close(); + await sent.settled; + expect(sent.state.status).toBeUndefined(); + expect(sent.state.error).toBeDefined(); + }); + it.each([ + ['a wrong token', { token: 'f'.repeat(64) }, 403], + ['an unsupported content type', { contentType: 'text/plain' }, 405], + ['a path that takes no POST', { path: '/' }, 405], + ].flatMap(([label, options, status]) => [[label, 'chunked', options, status], [label, 'Content-Length', { ...options as object, length: big }, status]]) as + [string, string, Parameters[2], number][])('closes the connection when it refuses %s before reading a large %s body, so shutdown is prompt', async (_label, _kind, options, status) => { + const { app, close } = await serveLong(); + const sent = oversized(app, big, options); + sent.end(); + await sent.settled; + // The client may be reset while still sending; when the reply arrives first, it closes the connection. + if (sent.state.status !== undefined) expect(sent.state).toMatchObject({ status, connection: 'close' }); + else expect(sent.state.error).toBeDefined(); + expect(await timed(close)).toBeLessThan(prompt); + storeClosed(app); + }); + it.each([ + ['a static file', 'GET', '/', undefined, 200], + ['an API read answered without waiting', 'GET', '/api/settings', undefined, 200], + ['an API read answered after waiting', 'GET', '/api/review', undefined, 200], + ['an unknown path', 'GET', '/missing', undefined, 404], + ['a POST whose body was read', 'POST', '/api/settings', JSON.stringify({ questionProvider: 'claude' }), 200], + ] as const)('keeps the connection open after %s', async (_label, method, path, body, status) => { + const { app } = await serve(); + const url = new URL(origin(app)); + const reply = await new Promise<{ status: number; connection?: string }>((resolve, reject) => { + const req = httpRequest({ host: url.hostname, port: url.port, path, method, + headers: { 'x-codeboost-token': app.token, ...(body ? { 'content-type': 'application/json' } : {}) } }, + res => { res.resume(); resolve({ status: res.statusCode!, connection: res.headers.connection }); }); + req.on('error', reject); req.end(body); + }); + expect(reply).toEqual({ status, connection: 'keep-alive' }); + }); +}); + describe('/api/runner', () => { function fakeRunner() { const settles: ((over?: Partial) => void)[] = []; diff --git a/web/server.ts b/web/server.ts index cba4b5a8..42d705bc 100644 --- a/web/server.ts +++ b/web/server.ts @@ -575,7 +575,17 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge const origin = `http://127.0.0.1:${actualPort}`; res.setHeader('Cache-Control', 'no-store'); res.setHeader('X-Content-Type-Options', 'nosniff'); res.setHeader('Content-Security-Policy', "default-src 'self'; script-src 'self'; style-src 'self'; font-src 'self'; connect-src 'self'; img-src 'self' data:; frame-ancestors 'none'; base-uri 'none'; form-action 'self'"); - const json = (status: number, value: unknown) => { res.writeHead(status, { 'Content-Type': 'application/json' }); res.end(JSON.stringify(value)); }; + // A reply sent before the request's body is read (a refusal) closes the connection (#157). Node then discards the + // body, and the connection stays busy while it does: shutdown's idle-connection close skips it, and nothing closes + // it once it goes idle, so a large body held it until the client or the keep-alive timeout dropped it. Accepted + // cost: a client still sending can be reset before it reads the refusal. The UI keeps an import's action ID for a + // resend. `req.complete` cannot tell whether a body is unread: it is still false while the handler starts. + const declaresBody = req.headers['transfer-encoding'] !== undefined || Number(req.headers['content-length'] ?? 0) > 0; + const head = (status: number, contentType: string) => { + if (declaresBody && !req.readableEnded) res.setHeader('Connection', 'close'); + res.writeHead(status, { 'Content-Type': contentType }); + }; + const json = (status: number, value: unknown) => { head(status, 'application/json'); res.end(JSON.stringify(value)); }; try { if (req.headers.host !== `127.0.0.1:${actualPort}` || (req.headers.origin && req.headers.origin !== origin)) { json(403, { error: 'Local origin required.' }); return; } const path = new URL(req.url ?? '/', origin).pathname; @@ -602,8 +612,15 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge const chunks: Buffer[] = []; let size = 0; const limit = path === '/api/plan/import' ? MAX_IMPORT_REQUEST_BYTES : MAX_REQUEST_BYTES; activeRequest.readingBody=true; - try { for await (const chunk of req) { size += chunk.length; if (size > limit) { json(413, { error: 'Request too large.' }); return; } chunks.push(chunk); } } + // Past the limit, the rest of the body is read and discarded, and the 413 goes out once it ends (#157). Node does + // not discard the rest of a body the handler has started reading, so a reply mid-body either held the connection + // half read until the keep-alive timeout (about 6 s) or reset the client. Closing the connection instead (as + // `head` does for a refusal before the body is read) can also reset a client still sending before it reads the + // reply. Shutdown still destroys a request that is discarding its body (readingBody stays set), as it does any + // request still reading one. + try { for await (const chunk of req) { size += chunk.length; if (size <= limit) chunks.push(chunk); else chunks.length = 0; } } finally { activeRequest.readingBody=false; } + if (size > limit) { json(413, { error: 'Request too large.' }); return; } const body = new TextDecoder('utf-8', { fatal: true }).decode(Buffer.concat(chunks)); const input=JSON.parse(body); // Admitted requests drain normally (AGENTS.md); only the irreversible merge boundary rechecks the flag. @@ -695,11 +712,11 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge if (req.method !== 'GET') { json(405, { error: 'Method not allowed.' }); return; } const files: Record = { '/': ['index.html', 'text/html'], '/app.js': ['app.js', 'text/javascript'], '/style.css': ['style.css', 'text/css'] }; const file = files[path]; - if (file) { res.writeHead(200, { 'Content-Type': `${file[1]}; charset=utf-8` }); res.end(readFileSync(new URL(file[0], publicRoot))); return; } + if (file) { head(200, `${file[1]}; charset=utf-8`); res.end(readFileSync(new URL(file[0], publicRoot))); return; } const font = /^\/fonts\/(ibm-plex-(?:sans|mono)-latin-(?:400|500|600)-normal\.woff2)$/.exec(path); if (font) { const family = font[1]!.startsWith('ibm-plex-sans') ? 'ibm-plex-sans' : 'ibm-plex-mono'; - res.writeHead(200, { 'Content-Type': 'font/woff2' }); res.end(readFileSync(fileURLToPath(new URL(`../node_modules/@fontsource/${family}/files/${font[1]}`, import.meta.url)))); return; + head(200, 'font/woff2'); res.end(readFileSync(fileURLToPath(new URL(`../node_modules/@fontsource/${family}/files/${font[1]}`, import.meta.url)))); return; } json(404, { error: 'Not found.' }); } catch (error) {