Read an oversized body before refusing it, and close on early refusals (#157)

[mchwang]

Closes #157.

What actually happened

#157 says shutdown never settles after a 413 on a large body. On the real server it does settle, but about 6 s late (Node's 5 s keepAliveTimeout plus its 1 s buffer). The lock is released. The original repro used a 3 s cutoff. Two mechanisms caused the delay:

1. Body over the limit. The handler replied 413 partway through reading the body. Node does not discard the rest of a body the handler has started reading, so the connection stayed half-read until the keep-alive timeout. A client still sending could instead be reset before it read the 413.
2. Refusal before the body is read (403 bad token or origin, 405, 503 while stopping). Node discards a never-read body, but the connection is 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 about 4–6 s.

What changed

• web/server.ts:
  • Over the limit: the rest of the body is read and discarded, and the 413 is sent once the body ends. Shutdown still destroys a request that is discarding its body, because readingBody stays set.
  • Refusals before the body is read: every reply now goes through a head() helper. When a request declares a body (Content-Length > 0 or Transfer-Encoding) that has not been read to its end, head() sends Connection: close. Every other reply keeps the connection open. req.complete could not serve as the test: it is false while the handler starts, even for a GET with no body.
  • Accepted cost: a client still sending a large body can be reset before it reads a 403, 405 or 503. The UI treats a reply with no status as ambiguous and keeps the import's action ID for a resend (web/public/app.js, planImportRetries).
• Docs: shutdown step 3 and a new test-table row in docs/implementation/runner-lifecycle.md. The 413 sentence in docs/plan-format.md.

Tests

The an oversized request body (#157) block in test/runner-shutdown.test.ts has 15 tests. Its timing tests use a 10 s drain limit, because the suite's default of 2 s masked the 6 s hold. The tests cover:

• The 413 arrives only after the body ends, after the server has read past the limit.
• Shutdown is prompt after a 413 sent with fetch, which keeps the connection the way a browser does. The Store is closed afterwards.
• An oversized body that completes during the shutdown drain still gets a 413 after its end, and shutdown is prompt.
• An oversized request still being sent at the drain limit is destroyed with no reply.
• A 403, the 405 for a wrong content type and the 405 for a POST to /, each with both a chunked and a Content-Length 4 MiB body: the connection closes and shutdown is prompt.
• Keep-alive is kept after a static file, a synchronous API read, an asynchronous API read, a 404, and a POST whose body was read.

Mutation results (reverting one part each):

Mutation	Fails
main's web/server.ts	10 (all but the 5 keep-alive guards)
413 sent mid-body	the 4 oversized-body tests
no Connection: close	the 6 refusal cases
declaresBody = Transfer-Encoding only	the 3 Content-Length refusal cases
declaresBody = Content-Length only	the 3 chunked refusal cases
!req.complete instead	static file, synchronous read, 404 keep-alive
always Connection: close	all 5 keep-alive cases

Validation

Gate	Result
Type check (tsc --noEmit)	clean on validated head 1ebf2cb
Affected suites (runner-shutdown, runner-planning-feedback)	pass at 1ebf2cb
Full suite, parallel project	74 files, 2005/2005 pass on validated head 1ebf2cb (load average about 278)
CI	pending
Real Docker	not triggered (agent-isolation.yml paths not touched)

Review record

Every independent round used a fresh agent on the full diff against the base shown.

Round	Type	Base / head	Findings	What changed
1	Self-review	a764e19 / 7a48a48	Comment claimed requestTimeout bounds the discard. A probe showed no 408 within 60 s.	Claim removed
2	Independent	a764e19 / 7a48a48	403/405/503 early refusals hold a large body the same way. Test 3 did not wait for the handler. "Not kept" claim inaccurate. No drain-completion test.	head() with Connection: close; chunks cleared past the limit; tests reworked (86cab51)
3	Independent	a764e19 / 86cab51	High: req.complete closed most GET replies. Timing tests could not fail under the 2 s drain. Drain test ordering unchecked. Load fragility.	declaresBody && !req.readableEnded; 10 s drain for timing tests; serverRead precondition; more keep-alive cases (5d4aac5). Merged origin/main 04a5714
4	Independent	04a5714 / 694c36d	Default 5 s test timeout too tight. Drain case untimed.	{ timeout: 30_000 }; drain case timed (0dc4f9a)
5	Independent	04a5714 / 0dc4f9a	Comments described the wrong Node mechanism for never-read bodies. Test comments wrong about Node's client.	Comments and docs corrected (5de9bea)
6	Independent	04a5714 / 5de9bea	Reset trade-off not stated. Mid-body comment named only one outcome. Long comment line.	Trade-off documented; comments fixed (2c91b6f)
7	Independent	04a5714 / 2c91b6f	Medium: no refusal test sent Content-Length, the browser's shape. Dropping that half of the check passed every test.	Chunked and Content-Length variants; Store-closed assertions (1ebf2cb)
8	Independent	04a5714 / 1ebf2cb	Clean. Optional: no test sends a body to the 503 shutting-down refusal.	Declined: it goes through the tested head() and needs a narrow race (a body request on a live keep-alive connection after shutdown starts)
9	Copilot (requested)	04a5714 / 1ebf2cb	0 findings, "Approval recommended"	No change. Head unchanged, so round 8's clean independent pass still applies and no re-request is needed

Known and accepted:

• Reading an oversized body to its end costs CPU and bandwidth until Node's request timeout (about 60–90 s in practice) or shutdown ends it. Memory stays bounded, and only a token holder can trigger it.
• An empty chunked body gets an unneeded Connection: close.
• Commit 86cab51's message states an earlier, wrong mechanism ("discards only an unread body that fits its socket buffers"). This PR body is the corrected account.

Review-lesson audit

• Round 1 requestTimeout claim; round 2 "not kept"; rounds 5–6 comment accuracy: covered by CLAUDE.md "Review readiness", "Treat every behavioural claim … as something to verify".
• Round 3 timing tests masked by the 2 s drain; round 7 chunked-only refusals: covered by "A test's setup must leave the state the production path would". Production uses a 14.5 s drain and Content-Length bodies.
• Round 2 test 3 not waiting for the handler: covered by "A validation fixture … must assert the disputed intermediate representation or state before using a downstream outcome as evidence".
• Round 3 req.complete misuse: one-off Node API semantics. It is recorded in the code comment.
• Round 2 other early refusals: one-off scope finding, fixed here.
• No AGENTS.md/CLAUDE.md update needed.

Out of scope, noticed

• Existing comments call 15 s "the request timeout", but Node enforces it only every 30 s (observed 60–90 s).
• Static and font replies write headers before readFileSync. A read failure would make the catch path set headers again and throw out of the async handler. Pre-existing on main.

🤖 Generated with Claude Code

[Copilot]

🟢 Approval recommended

The implementation addresses the reported lifecycle issue with comprehensive regression coverage and no unresolved correctness concerns.

0 open findings

What changed in this PR

Ensures HTTP shutdown completes promptly after large-body refusals while preserving bounded memory and keep-alive behavior.

Changes:

• Drains oversized bodies before returning 413.
• Closes connections for early refusals with unread bodies.
• Adds shutdown regressions and lifecycle documentation.

File	Description
web/​server.ts	Implements body draining and conditional connection closure.
test/​runner-shutdown.test.ts	Covers oversized bodies, refusals, shutdown, and keep-alive.
docs/​plan-format.md	Documents oversized import handling.
docs/​implementation/​runner-lifecycle.md	Updates shutdown lifecycle and regression scenarios.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.