From 7a48a48a192b6fb1ecdaa62a0df3253ab8db8910 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sat, 10 Oct 2026 01:08:17 -0700 Subject: [PATCH 1/8] Read an oversized body to its end before refusing it (#157) A POST over its body limit got its 413 while the client was still sending. The half-read connection was held until Node's keep-alive timeout (about 6 s), so a shutdown started then waited that long, and a client still sending could be reset before it read the reply. Past the limit the server now discards the rest of the body without keeping it and replies 413 once it ends. Shutdown still destroys a request that is discarding its body. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/runner-lifecycle.md | 3 +- docs/plan-format.md | 2 +- test/runner-shutdown.test.ts | 51 ++++++++++++++++++++++++- web/server.ts | 7 +++- 4 files changed, 59 insertions(+), 4 deletions(-) diff --git a/docs/implementation/runner-lifecycle.md b/docs/implementation/runner-lifecycle.md index dcb68072..1bfdd3da 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 a body over its limit is read to its end and discarded before the 413 goes out (#157): a reply sent while the body is unread leaves the connection busy until that timeout, and 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, and shutdown then settles without waiting for the keep-alive timeout. Still sending at the drain limit → destroyed, no reply | | 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 3fcac869..7e28026b 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 without being kept, 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..de01b24f 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'; @@ -198,6 +198,55 @@ describe('server shutdown', { timeout: 15_000 }, () => { }); }); +describe('an oversized request body (#157)', () => { + /** Starts a POST with `first` bytes of body; `send` writes more, `end` ends it. Records when the reply arrived. */ + function oversized(app: App, first: number) { + const url = new URL(origin(app)); + const state: { status?: number; body?: string; error?: Error; endedAt?: number; repliedAt?: number } = {}; + const req = httpRequest({ host: url.hostname, port: url.port, path: '/api/action', method: 'POST', + headers: { 'x-codeboost-token': app.token, 'content-type': 'application/json' } }, res => { + state.repliedAt = performance.now(); state.status = res.statusCode; + 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(); } }; + } + + it('replies 413 only once the whole body has arrived', async () => { + const { app } = await serve(); + const sent = oversized(app, MAX_REQUEST_BYTES * 2); + await new Promise(resolve => setTimeout(resolve, 300)); + expect(sent.state.status).toBeUndefined(); + sent.send(4 * 1024 * 1024); sent.end(); + await sent.settled; + expect(sent.state).toMatchObject({ status: 413, body: JSON.stringify({ error: 'Request too large.' }) }); + expect(sent.state.error).toBeUndefined(); + expect(sent.state.repliedAt).toBeGreaterThanOrEqual(sent.state.endedAt!); + }); + it('shuts down promptly after refusing a body far over the limit', async () => { + const { app, close } = await serve(); + const response = await fetch(`${origin(app)}/api/action`, { method: 'POST', body: 'x'.repeat(4 * 1024 * 1024), + headers: { 'x-codeboost-token': app.token, 'content-type': 'application/json' } }); + expect(response.status).toBe(413); + await response.text(); + // A half-read connection was held until Node's keep-alive timeout (5 s, plus 1 s of buffer) before close settled. + const started = performance.now(); + await close(); + expect(performance.now() - started).toBeLessThan(3_000); + }); + it('destroys an oversized request still being sent after the drain limit, and close settles', async () => { + const { app, close } = await serve(); + const sent = oversized(app, MAX_REQUEST_BYTES * 2); + await new Promise(resolve => setTimeout(resolve, 100)); + await close(); + await sent.settled; + expect(sent.state.status).toBeUndefined(); + expect(sent.state.error).toBeDefined(); + }); +}); + describe('/api/runner', () => { function fakeRunner() { const settles: ((over?: Partial) => void)[] = []; diff --git a/web/server.ts b/web/server.ts index cba4b5a8..4a91c0cb 100644 --- a/web/server.ts +++ b/web/server.ts @@ -602,8 +602,13 @@ 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). A reply + // while the client is still sending leaves the connection half read: it is held until the keep-alive timeout, + // delaying shutdown, and the client can be reset 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); } } 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. From 86cab51504e3f1e7dc89ab28aad30ee0ea0cc06f Mon Sep 17 00:00:00 2001 From: mchwang Date: Sat, 10 Oct 2026 01:21:09 -0700 Subject: [PATCH 2/8] Close the connection on a refusal sent before the body is read (#157 review) A 403 or 405 sent before a large body was read held the connection until the keep-alive timeout too: Node discards only an unread body that fits its socket buffers. Every reply sent while the request body is unread now carries Connection: close. Tests wait for the handler before shutdown, cover an oversized body that completes during the drain, and pin the keep-alive of requests whose body was read. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/runner-lifecycle.md | 4 +- docs/plan-format.md | 2 +- test/runner-shutdown.test.ts | 69 +++++++++++++++++++------ web/server.ts | 23 ++++++--- 4 files changed, 72 insertions(+), 26 deletions(-) diff --git a/docs/implementation/runner-lifecycle.md b/docs/implementation/runner-lifecycle.md index 1bfdd3da..300c755d 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. For the same reason a body over its limit is read to its end and discarded before the 413 goes out (#157): a reply sent while the body is unread leaves the connection busy until that timeout, and a request still sending at the drain limit is destroyed like any other. | 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 sent while the request body is still unread, such as a 403 or 405 before the body is read, carries `Connection: close`: Node discards only an unread body that fits its socket buffers, and a larger one held the connection until that timeout. A body over its limit is instead read to its end and discarded before the 413 goes out, so 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,7 +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, and shutdown then settles without waiting for the keep-alive timeout. Still sending at the drain limit → destroyed, no reply | +| 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 7e28026b..3693ff07 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 read to its end without being kept, 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). +`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 de01b24f..a18fe4b8 100644 --- a/test/runner-shutdown.test.ts +++ b/test/runner-shutdown.test.ts @@ -199,13 +199,13 @@ describe('server shutdown', { timeout: 15_000 }, () => { }); describe('an oversized request body (#157)', () => { - /** Starts a POST with `first` bytes of body; `send` writes more, `end` ends it. Records when the reply arrived. */ - function oversized(app: App, first: number) { + /** 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 } = {}) { const url = new URL(origin(app)); - const state: { status?: number; body?: string; error?: Error; endedAt?: number; repliedAt?: number } = {}; - const req = httpRequest({ host: url.hostname, port: url.port, path: '/api/action', method: 'POST', - headers: { 'x-codeboost-token': app.token, 'content-type': 'application/json' } }, res => { - state.repliedAt = performance.now(); state.status = res.statusCode; + 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', + headers: { 'x-codeboost-token': options.token ?? app.token, 'content-type': options.contentType ?? 'application/json' } }, 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); }); @@ -213,38 +213,77 @@ describe('an oversized request body (#157)', () => { 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's handler has run up to its first await for the next request (the body read, for a POST). */ + const handled = (app: App) => new Promise(resolve => { app.server.once('request', () => setImmediate(resolve)); }); + const timed = async (close: () => Promise) => { const started = performance.now(); await close(); return performance.now() - started; }; + const big = 4 * 1024 * 1024; it('replies 413 only once the whole body has arrived', async () => { const { app } = await serve(); - const sent = oversized(app, MAX_REQUEST_BYTES * 2); + const reading = handled(app), sent = oversized(app, MAX_REQUEST_BYTES * 2); + await reading; await new Promise(resolve => setTimeout(resolve, 300)); expect(sent.state.status).toBeUndefined(); - sent.send(4 * 1024 * 1024); sent.end(); + sent.send(big); sent.end(); await sent.settled; expect(sent.state).toMatchObject({ status: 413, body: JSON.stringify({ error: 'Request too large.' }) }); expect(sent.state.error).toBeUndefined(); - expect(sent.state.repliedAt).toBeGreaterThanOrEqual(sent.state.endedAt!); }); it('shuts down promptly after refusing a body far over the limit', async () => { const { app, close } = await serve(); - const response = await fetch(`${origin(app)}/api/action`, { method: 'POST', body: 'x'.repeat(4 * 1024 * 1024), + 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(response.status).toBe(413); await response.text(); // A half-read connection was held until Node's keep-alive timeout (5 s, plus 1 s of buffer) before close settled. - const started = performance.now(); - await close(); - expect(performance.now() - started).toBeLessThan(3_000); + expect(await timed(close)).toBeLessThan(3_000); + }); + it('still replies 413 to an oversized body that completes during the shutdown drain', async () => { + const { app, close } = await serve(); + const reading = handled(app), sent = oversized(app, MAX_REQUEST_BYTES * 2); + await reading; + const closing = close(); + await tick(); + sent.send(big); sent.end(); + await sent.settled; + expect(sent.state).toMatchObject({ status: 413, body: JSON.stringify({ error: 'Request too large.' }) }); + await closing; }); it('destroys an oversized request still being sent after the drain limit, and close settles', async () => { const { app, close } = await serve(); - const sent = oversized(app, MAX_REQUEST_BYTES * 2); - await new Promise(resolve => setTimeout(resolve, 100)); + const reading = handled(app), sent = oversized(app, MAX_REQUEST_BYTES * 2); + await reading; 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], + ] as const)('closes the connection when it refuses %s before reading a large body, so shutdown is prompt', async (_label, options, status) => { + const { app, close } = await serve(); + 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(3_000); + }); + it.each([['GET', '/api/review', undefined], ['POST', '/api/settings', JSON.stringify({ questionProvider: 'claude' })]] as const)( + 'keeps the connection open after a %s whose body was read', async (method, path, body) => { + 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: 200, connection: 'keep-alive' }); + }); }); describe('/api/runner', () => { diff --git a/web/server.ts b/web/server.ts index 4a91c0cb..0a4d03e1 100644 --- a/web/server.ts +++ b/web/server.ts @@ -575,7 +575,14 @@ 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 while the request body is still unread (a refusal before the body is read) closes the connection + // (#157). Node discards only an unread body that fits its socket buffers; a larger one held the connection until + // the keep-alive timeout, which delayed shutdown by about 6 s. + const head = (status: number, contentType: string) => { + if (!req.complete) 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,11 +609,11 @@ 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; - // Past the limit, the rest of the body is read and discarded, and the 413 goes out once it ends (#157). A reply - // while the client is still sending leaves the connection half read: it is held until the keep-alive timeout, - // delaying shutdown, and the client can be reset 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); } } + // Past the limit, the rest of the body is read and discarded, and the 413 goes out once it ends (#157). Closing + // the connection instead (as `head` does for a refusal before the body is read) can 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)); @@ -700,11 +707,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) { From 5d4aac5e39952ebeac81e93a921960d4354f268a Mon Sep 17 00:00:00 2001 From: mchwang Date: Sat, 10 Oct 2026 01:30:17 -0700 Subject: [PATCH 3/8] Close only on a declared, unread body; make the #157 tests discriminate req.complete is still false while the handler starts, even for a request with no body, so most GET replies lost keep-alive. A reply now closes the connection only when the request declares a body that has not been read to its end. The timing tests use a drain limit above the keep-alive timeout, wait for the server to read past the limit, and keep-alive is pinned for static files, synchronous and asynchronous reads, 404s and a POST whose body was read. Co-Authored-By: Claude Opus 5.5 --- docs/implementation/runner-lifecycle.md | 2 +- test/runner-shutdown.test.ts | 83 ++++++++++++++----------- web/server.ts | 6 +- 3 files changed, 53 insertions(+), 38 deletions(-) diff --git a/docs/implementation/runner-lifecycle.md b/docs/implementation/runner-lifecycle.md index 300c755d..3c2524c1 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. For the same reason (#157), a reply sent while the request body is still unread, such as a 403 or 405 before the body is read, carries `Connection: close`: Node discards only an unread body that fits its socket buffers, and a larger one held the connection until that timeout. A body over its limit is instead read to its end and discarded before the 413 goes out, so 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. | +| 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 only an unread body that fits its socket buffers, and a larger one held the connection until that timeout. A body over its limit is instead read to its end and discarded before the 413 goes out, so 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) | diff --git a/test/runner-shutdown.test.ts b/test/runner-shutdown.test.ts index a18fe4b8..f6ffd518 100644 --- a/test/runner-shutdown.test.ts +++ b/test/runner-shutdown.test.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); @@ -199,6 +199,10 @@ describe('server shutdown', { timeout: 15_000 }, () => { }); describe('an oversized request body (#157)', () => { + // The drain limit is above Node's keep-alive timeout (5 s, plus 1 s of buffer), so a connection held until that + // timeout 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 } = {}) { const url = new URL(origin(app)); @@ -213,46 +217,50 @@ describe('an oversized request body (#157)', () => { 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's handler has run up to its first await for the next request (the body read, for a POST). */ - const handled = (app: App) => new Promise(resolve => { app.server.once('request', () => setImmediate(resolve)); }); + /** 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 big = 4 * 1024 * 1024; + const refused = JSON.stringify({ error: 'Request too large.' }); it('replies 413 only once the whole body has arrived', async () => { - const { app } = await serve(); - const reading = handled(app), sent = oversized(app, MAX_REQUEST_BYTES * 2); - await reading; + 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: JSON.stringify({ error: 'Request too large.' }) }); + 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 serve(); + const { app, close } = await serveLong(); + // fetch keeps the connection for reuse, as the browser does; Node's http client would drop it after the reply. 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(response.status).toBe(413); - await response.text(); - // A half-read connection was held until Node's keep-alive timeout (5 s, plus 1 s of buffer) before close settled. - expect(await timed(close)).toBeLessThan(3_000); + expect({ status: response.status, body: await response.text() }).toEqual({ status: 413, body: refused }); + expect(await timed(close)).toBeLessThan(prompt); }); - it('still replies 413 to an oversized body that completes during the shutdown drain', async () => { - const { app, close } = await serve(); - const reading = handled(app), sent = oversized(app, MAX_REQUEST_BYTES * 2); - await reading; + 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 closing = close(); await tick(); - sent.send(big); sent.end(); + sent.send(MAX_REQUEST_BYTES * 4); sent.end(); await sent.settled; - expect(sent.state).toMatchObject({ status: 413, body: JSON.stringify({ error: 'Request too large.' }) }); + expect(sent.state).toMatchObject({ status: 413, body: refused }); + expect(sent.state.repliedAt).toBeGreaterThanOrEqual(sent.state.endedAt!); await closing; }); it('destroys an oversized request still being sent after the drain limit, and close settles', async () => { const { app, close } = await serve(); - const reading = handled(app), sent = oversized(app, MAX_REQUEST_BYTES * 2); - await reading; + 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(); @@ -263,27 +271,32 @@ describe('an oversized request body (#157)', () => { ['an unsupported content type', { contentType: 'text/plain' }, 405], ['a path that takes no POST', { path: '/' }, 405], ] as const)('closes the connection when it refuses %s before reading a large body, so shutdown is prompt', async (_label, options, status) => { - const { app, close } = await serve(); + 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(3_000); + expect(await timed(close)).toBeLessThan(prompt); }); - it.each([['GET', '/api/review', undefined], ['POST', '/api/settings', JSON.stringify({ questionProvider: 'claude' })]] as const)( - 'keeps the connection open after a %s whose body was read', async (method, path, body) => { - 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: 200, connection: 'keep-alive' }); + 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', () => { diff --git a/web/server.ts b/web/server.ts index 0a4d03e1..53f8066b 100644 --- a/web/server.ts +++ b/web/server.ts @@ -577,9 +577,11 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge 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'"); // A reply sent while the request body is still unread (a refusal before the body is read) closes the connection // (#157). Node discards only an unread body that fits its socket buffers; a larger one held the connection until - // the keep-alive timeout, which delayed shutdown by about 6 s. + // the keep-alive timeout, which delayed shutdown by about 6 s. `req.complete` cannot tell: it is still false while + // the handler starts, even for a request with no body. + const declaresBody = req.headers['transfer-encoding'] !== undefined || Number(req.headers['content-length'] ?? 0) > 0; const head = (status: number, contentType: string) => { - if (!req.complete) res.setHeader('Connection', 'close'); + 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)); }; From 694c36d595728daa3ca6828a34e203153dc8169b Mon Sep 17 00:00:00 2001 From: mchwang Date: Sat, 10 Oct 2026 01:30:35 -0700 Subject: [PATCH 4/8] Split the #157 shutdown note into sentences Co-Authored-By: Claude Opus 5.5 --- docs/implementation/runner-lifecycle.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/implementation/runner-lifecycle.md b/docs/implementation/runner-lifecycle.md index 9aa9c352..007ccf9b 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. 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 only an unread body that fits its socket buffers, and a larger one held the connection until that timeout. A body over its limit is instead read to its end and discarded before the 413 goes out, so 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. | +| 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 only an unread body that fits its socket buffers, and a larger one held the connection until that timeout. A body over its limit is instead read to its end and discarded before the 413 goes out, so 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) | From 0dc4f9a58e881c718b075b72f732a9d0b9877c06 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sat, 10 Oct 2026 01:39:27 -0700 Subject: [PATCH 5/8] Time the #157 drain case and give its tests room under load Co-Authored-By: Claude Opus 5.5 --- test/runner-shutdown.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/runner-shutdown.test.ts b/test/runner-shutdown.test.ts index f6ffd518..10c44635 100644 --- a/test/runner-shutdown.test.ts +++ b/test/runner-shutdown.test.ts @@ -198,7 +198,7 @@ describe('server shutdown', { timeout: 15_000 }, () => { }); }); -describe('an oversized request body (#157)', () => { +describe('an oversized request body (#157)', { timeout: 30_000 }, () => { // The drain limit is above Node's keep-alive timeout (5 s, plus 1 s of buffer), so a connection held until that // timeout shows as a slow close instead of being cut at the drain limit. const drainMs = 10_000, prompt = 3_000, big = 4 * 1024 * 1024; @@ -249,13 +249,14 @@ describe('an oversized request body (#157)', () => { const { app, close } = await serveLong(); const read = serverRead(app, MAX_REQUEST_BYTES * 2), sent = oversized(app, MAX_REQUEST_BYTES * 2); await read; - const closing = close(); + 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(); From 5de9bea8f535a1eaba7e744b4ce7967ab70b3752 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sat, 10 Oct 2026 01:50:50 -0700 Subject: [PATCH 6/8] Correct how Node holds a half-read or unread connection (#157 review) Co-Authored-By: Claude Opus 5.5 --- docs/implementation/runner-lifecycle.md | 2 +- test/runner-shutdown.test.ts | 6 +++--- web/server.ts | 15 ++++++++------- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/docs/implementation/runner-lifecycle.md b/docs/implementation/runner-lifecycle.md index 007ccf9b..7d7a15db 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. 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 only an unread body that fits its socket buffers, and a larger one held the connection until that timeout. A body over its limit is instead read to its end and discarded before the 413 goes out, so 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. | +| 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. 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) | diff --git a/test/runner-shutdown.test.ts b/test/runner-shutdown.test.ts index 10c44635..70350e33 100644 --- a/test/runner-shutdown.test.ts +++ b/test/runner-shutdown.test.ts @@ -199,8 +199,8 @@ describe('server shutdown', { timeout: 15_000 }, () => { }); describe('an oversized request body (#157)', { timeout: 30_000 }, () => { - // The drain limit is above Node's keep-alive timeout (5 s, plus 1 s of buffer), so a connection held until that - // timeout shows as a slow close instead of being cut at the drain limit. + // 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. */ @@ -239,7 +239,7 @@ describe('an oversized request body (#157)', { timeout: 30_000 }, () => { }); it('shuts down promptly after refusing a body far over the limit', async () => { const { app, close } = await serveLong(); - // fetch keeps the connection for reuse, as the browser does; Node's http client would drop it after the reply. + // 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 }); diff --git a/web/server.ts b/web/server.ts index 53f8066b..891c428d 100644 --- a/web/server.ts +++ b/web/server.ts @@ -575,10 +575,10 @@ 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'"); - // A reply sent while the request body is still unread (a refusal before the body is read) closes the connection - // (#157). Node discards only an unread body that fits its socket buffers; a larger one held the connection until - // the keep-alive timeout, which delayed shutdown by about 6 s. `req.complete` cannot tell: it is still false while - // the handler starts, even for a request with no body. + // 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. + // `req.complete` cannot tell whether a body is unread: it is still false while the handler starts, even with none. 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'); @@ -611,9 +611,10 @@ 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; - // Past the limit, the rest of the body is read and discarded, and the 413 goes out once it ends (#157). Closing - // the connection instead (as `head` does for a refusal before the body is read) can reset a client still sending - // before it reads the reply. Shutdown still destroys a request that is discarding its body (readingBody stays + // 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 left the connection half + // read until the keep-alive timeout (about 6 s). Closing the connection instead (as `head` does for a refusal + // before the body is read) can 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; } From 2c91b6f41b0f05c5f500cfcb491438d74e52eca8 Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 11 Oct 2026 18:59:55 -0700 Subject: [PATCH 7/8] State the reset trade-off of closing on a refusal (#157 review) Co-Authored-By: Claude Opus 5.5 --- docs/implementation/runner-lifecycle.md | 2 +- web/server.ts | 14 ++++++++------ 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/docs/implementation/runner-lifecycle.md b/docs/implementation/runner-lifecycle.md index 7d7a15db..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. 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. 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. | +| 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) | diff --git a/web/server.ts b/web/server.ts index 891c428d..42d705bc 100644 --- a/web/server.ts +++ b/web/server.ts @@ -577,8 +577,9 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge 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'"); // 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. - // `req.complete` cannot tell whether a body is unread: it is still false while the handler starts, even with none. + // 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'); @@ -612,10 +613,11 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge const limit = path === '/api/plan/import' ? MAX_IMPORT_REQUEST_BYTES : MAX_REQUEST_BYTES; activeRequest.readingBody=true; // 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 left the connection half - // read until the keep-alive timeout (about 6 s). Closing the connection instead (as `head` does for a refusal - // before the body is read) can 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. + // 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; } From 1ebf2cbfdc94eef672bfaad2eab67ce3f07f412a Mon Sep 17 00:00:00 2001 From: mchwang Date: Sun, 11 Oct 2026 19:09:50 -0700 Subject: [PATCH 8/8] Cover Content-Length refusals and the closed Store in the #157 tests Co-Authored-By: Claude Opus 5.5 --- test/runner-shutdown.test.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/test/runner-shutdown.test.ts b/test/runner-shutdown.test.ts index 70350e33..15cb4124 100644 --- a/test/runner-shutdown.test.ts +++ b/test/runner-shutdown.test.ts @@ -204,11 +204,13 @@ describe('an oversized request body (#157)', { timeout: 30_000 }, () => { 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 } = {}) { + 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', - headers: { 'x-codeboost-token': options.token ?? app.token, 'content-type': options.contentType ?? 'application/json' } }, res => { + // 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; }); }); @@ -225,6 +227,8 @@ describe('an oversized request body (#157)', { timeout: 30_000 }, () => { }); 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(); @@ -244,6 +248,7 @@ describe('an oversized request body (#157)', { timeout: 30_000 }, () => { 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(); @@ -271,7 +276,8 @@ describe('an oversized request body (#157)', { timeout: 30_000 }, () => { ['a wrong token', { token: 'f'.repeat(64) }, 403], ['an unsupported content type', { contentType: 'text/plain' }, 405], ['a path that takes no POST', { path: '/' }, 405], - ] as const)('closes the connection when it refuses %s before reading a large body, so shutdown is prompt', async (_label, options, status) => { + ].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(); @@ -280,6 +286,7 @@ describe('an oversized request body (#157)', { timeout: 30_000 }, () => { 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],