Skip to content

Body lifecycle: empty chunks, multipart boundary quoting, logging tap after close (#77) - #91

Merged
Wahbeh-Mohammad merged 5 commits into
audit/remediation-67from
audit/67/77-body-lifecycle
Sep 5, 2026
Merged

Body lifecycle: empty chunks, multipart boundary quoting, logging tap after close (#77)#91
Wahbeh-Mohammad merged 5 commits into
audit/remediation-67from
audit/67/77-body-lifecycle

Conversation

@Wahbeh-Mohammad

Copy link
Copy Markdown
Contributor

Closes #77. Part of the audit remediation umbrella #67, milestone 4, wave 4. Decisions: D15 (mine), D0 and D1 (constraining).

What changed

1. streamBody refuses a zero-length delivery during an exact-length copy (HIGH). StreamBody.#writeExactly read, checked the over-run, then wrote, with no guard on value.length === 0. Two consequences, neither diagnosable anywhere else: a source that only ever yields empty chunks never signals done, so the delivered-of-declared check at the end of the loop is never reached (the auditor's probe forwarded 200,000 empty chunks first); and every empty chunk reached the transport sink, where to an HTTP/1.1 chunked-encoding transport a zero-length chunk is the terminating chunk — io/buffered-sink.ts:66-71 says so and refuses to emit one. assertNonEmptyChunk now raises SourceContractViolationError, with io/retention-window.ts:177-183's wording and error type, which is also what body/response-body-logging.ts:82-88 already does for BODY-25 on the response side. A declared length of 0 stays a legitimate empty write: that is a source signalling {done: true} immediately, which never reaches the check. The pipeTo path (unknown declared length) is unchanged — the runtime owns that pump and HTTP-39/BODY-10 scopes its rule to the exact-length copy.

2. The multipart boundary parameter is quoted when it is not a bare token. MultipartBody built multipart/form-data; boundary=${boundary} by interpolation while BOUNDARY_PATTERN (multipart-body.ts:29-31) admits the full RFC 2046 bchars set — and bchars and RFC 9110 tchar are different: ' ', ,, :, =, ?, /, ( and ) are legal in a boundary and illegal bare in a header parameter value. The header is now rendered through MediaType.of(...).render(), the module that already owns HTTP-25's token-or-quoted-string decision and guarantees parse(render(x)) === x. A boundary that is a bare token still renders bare, so the generated default and every prior expectation are byte-identical. Narrowing validateBoundary to tchar was rejected per D15: HTTP-51 asks that a boundary violating RFC 2046 be refused, not that a conforming one be — the defect was in the rendering.

3. The response logging tap is inert after close(), not poisoned by it. closeDelegate releases the reader (response-body-logging.ts:62) but snapshot() called startDrain unconditionally, so a post-close snapshot read from a detached reader, drainOnce's catch cached the raw TypeError: Invalid state: The reader is not attached to a stream as the wrapper's failure, and error() reported that fabricated upstream failure forever over a capture that never failed. read() after close() rejected with the same raw TypeError. startDrain now returns immediately when the wrapper is closed and no drain was ever started; a drain already in flight is left alone, because on the fits-cap path the drain closes the delegate itself and its promise is what read() awaits. read() gains ClosedResourceError('LoggedResponseBody') after the fits-cap and tail-consumed checks, in that order — fits-cap still serves a fresh non-consuming view after close (BODY-23 and BODY-28 both require it, so "closed" must not mean "unreadable"), a consumed tail still reports ConsumedBodyError because that says more than "closed" (BODY-24), and everything else with the delegate gone is IO-42's state error rather than a silently truncated response.

4. The five Web Streams bridges get Node-runtime coverage, which they had none of. toReadableStream, toWritableStream, TeeSink's bridge, withRequestLogging and withResponseLogging are each a hand-written underlying source or sink object, so what they exercise is the runtime's own pull scheduling, cancel dispatch and reader-lock bookkeeping — CLAUDE.md's runtime-divergent surface. Cases went into the two existing topic files, per D15, not one file per bridge.

Test rows added

Where Rows What
packages/core/src/body/stream-body.test.ts 3 empty-only source raises on the first chunk rather than spinning (pull count asserted, not the 1000 an unguarded run makes); an empty chunk between real chunks raises and never reaches the sink; declared length 0 over an immediately-closing source is still a clean empty write
packages/core/src/body/multipart-body.test.ts 8 boundary quoted when not a bare token; left bare when it is (including the generated default); six Response.formData() round-trips over a,b, bound ary, a:b, a=b, a?b, (a)/b
packages/core/src/body/response-body-logging.test.ts 6 close-then-snapshot; close-before-any-read; close-then-read → ClosedResourceError; close-then-read in the exceeds-cap regime; close-then-error reports only the genuine failure; fits-cap stays repeatably readable after its own close
tests/node-conformance/io-byte-stream.test.mjs 9 one-chunk-at-a-time pull; natural EOF closing the bridge but not the source (IO-19); cancel closing the source and releasing the caller stream's lock; a mid-stream read failure closing the source itself, because the spec does not dispatch cancel on an errored stream; pipeTo chaining close to the destination; abort carrying its reason instead of collapsing into a close; a zero-length chunk dropped rather than forwarded; the tee bridge routing through the tee so the tap still sees the bytes; abort reaching the primary while the tap survives
tests/node-conformance/body-lifecycle.test.mjs 13 six multipart round-trips + the bare-token case; the zero-length refusal and the declared-length-0 allowance; the request tap's mirror, clear-between-writes and abort of a sink the delegate never touched; the response tap's prefix-then-tail, close-once cancel, inertness after close(), and repeatable fits-cap reads

tests/node-conformance/README.md's membership rule now names the five bridges and the two files they landed in.

The multipart rows are the reason the Node tree is not optional here. Bun's Response.formData() accepts boundary=a,b; Node's (undici's) rejects the whole body with TypeError: Failed to parse body as FormData. The Bun suite was green over a Content-Type no Node peer could parse. The Bun rows are the regression guard; only the Node case reproduces the defect. Both test files say so where the cases are.

Deviation rows added

None, and one judgement call to flag. docs/deviations.md is outside this task's file partition (it belongs to #73 this wave), so no row was written; this is the recommendation for the supervisor to accept or reject on the umbrella.

HTTP-39/BODY-10 scopes its rule to "a zero-length read for a positive request". The guard here fires on any zero-length delivery inside the exact-length copy, including when the declared length is 0 — a widening. It is the only reading that also satisfies the same sentence's "never as an infinite spin": with the guard scoped to declared > 0, an unbounded source of empty chunks against a declared length of 0 still spins forever. Two sibling modules already resolved the identical "for a positive requested count" qualifier the same way and recorded it in a source comment rather than a deviation row — io/retention-window.ts:177-183 (IO-17) and body/response-body-logging.ts:70-88 (BODY-25) — so this change follows that precedent and cites both. If the supervisor prefers a row, the evidence is packages/core/src/body/stream-body.ts:11-39.

No other reading here departs from the spec text: quoting the boundary is what makes HTTP-51 and HTTP-25 hold rather than a departure from either, and ClosedResourceError on a closed wrapper is IO-42's own rule.

Deferred — release machinery

Suspended under D1; nothing below was done.

  • Patch changeset for @dexpace/core, covering three observable behaviour changes: streamBody(...).writeTo on a declared-length body now rejects with SourceContractViolationError where it previously forwarded the empty chunk and reported EndOfStreamError (or never returned); MultipartBody.mediaType now quotes a non-token boundary parameter, so a consumer string-matching on the old unquoted form sees a different value; withResponseLogging's read() after close() now rejects with ClosedResourceError instead of a raw TypeError, and error() no longer reports a close-induced failure. The issue text asks for exactly this changeset; D1 overrides it.
  • Shipped .d.ts prose changed and is therefore also patch-note material: streamBody's and StreamBody.writeTo's @throws gained a SourceContractViolationError clause, and MultipartBody.mediaType's summary now states the quoting rule. packages/core/etc/core.api.md is unchanged — the report records signatures, not doc comments, confirmed by running bun run api:local after each of the three fixes.
  • No docs/first-release.md edit, though the MultipartBody.mediaType change is squarely in its "free only before the first version bump" class.

Gates

node .claude/skills/ci-preflight/run-ci.mjs --clean, run once on the final tree, without --node-floor:

  PASS  install                    1s
  PASS  verify:knowledge-structure   0s
  PASS  typecheck                 23s
  PASS  lint                      11s
  PASS  build                      9s
  PASS  test                       6s
  PASS  test:scripts               0s
  PASS  api                       15s
  PASS  lint:publish               7s
  PASS  verify:dual-consumption    0s
  PASS  verify:consumer-types      1s
  PASS  verify:seam-1              0s
  PASS  verify:sse-37              0s
  PASS  verify:runtime-floor       0s
  PASS  verify:test-partition      0s
  PASS  test:examples              0s
  PASS  verify:import-cycles       0s
  PASS  verify:reproducible-build  36s
  PASS  audit                      0s
  PASS  test:node                  0s

CI preflight: all 20 steps passed.

test:node ran on Node v26.2.0; the engines.node floor leg (20.3.0) was not exercised locally, per the run's instruction to skip --node-floor while other agents run alongside. node .claude/skills/housekeeping/probe.mjs: no drift found.

`StreamBody.#writeExactly` read, checked the over-run, then wrote — with no
guard on `value.length === 0`. Two consequences, neither diagnosable anywhere
else:

- A source that only ever yields empty chunks never signals `done`, so the
  delivered-of-declared check at the end of the loop is never reached. The
  audit's probe forwarded 200,000 empty chunks before the source closed.
- Every empty chunk reached the transport sink. To an HTTP/1.1
  chunked-encoding transport a zero-length chunk is the TERMINATING chunk —
  `io/buffered-sink.ts:66-71` says so and refuses to emit one — so tolerating
  it ends the request body early while the copy still believes it is mid-body.

`assertNonEmptyChunk` now raises `SourceContractViolationError` with
`io/retention-window.ts:177-183`'s wording and error type, which is also what
`body/response-body-logging.ts:82-88` does for BODY-25 on the response side. A
request body reaches both this copy and `BufferedSource`; a divergence would
make the same upstream fail or succeed depending only on which wrapper it
passed through.

A declared length of 0 stays a legitimate empty write (BODY-10): that is a
source signalling `{done: true}` immediately, which never reaches the check.

The `pipeTo` path (unknown declared length) is unchanged — the runtime owns
that pump, and HTTP-39/BODY-10's rule is scoped to the exact-length copy.

`bun run api:local` on core: no change, the report records signatures only.

Refs #77, #67. HTTP-39, BODY-10.
`MultipartBody` built its own `Content-Type` by interpolation —
`multipart/form-data; boundary=${boundary}` — while `BOUNDARY_PATTERN`
(`multipart-body.ts:29-31`) admits the full RFC 2046 `bchars` set. `bchars`
and RFC 9110 `tchar` are different: ' ', ',', ':', '=', '?', '/', '(' and ')'
are legal in a boundary and illegal bare in a header parameter value. So
`multipartBody(parts, 'a,b')` emitted `boundary=a,b`, which an RFC 9110
parameter parser reads as `boundary=a` plus a junk parameter and then never
finds a delimiter for. Node's own FormData parser rejects the whole body with
`TypeError: Failed to parse body as FormData`.

The header is now rendered through `MediaType.of(...).render()`, the module
that already owns HTTP-25's token-or-quoted-string decision and guarantees
`parse(render(x)) === x`. A boundary that IS a bare token still renders bare,
so the generated default and every existing expectation are byte-identical.

Narrowing `validateBoundary` to `tchar` was rejected (D15): HTTP-51 asks that
a boundary violating the RFC 2046 grammar be refused, not that a conforming
one be. The defect was in the rendering.

The round-trip case is a parser disagreement, not a one-runtime bug: Bun's
`Response.formData()` tolerates the unquoted form, Node's does not. The Bun
rows are the regression guard; the reproducer is the matching case in
`tests/node-conformance/body-lifecycle.test.mjs` (next commit).

Refs #77, #67. HTTP-51.
…close()

`closeDelegate` releases the reader (`response-body-logging.ts:62`), but
`snapshot()` called `startDrain` unconditionally. So a `snapshot()` after
`close()` read from a detached reader, `drainOnce`'s catch cached the raw
`TypeError: Invalid state: The reader is not attached to a stream` as this
wrapper's `failure`, and `error()` then reported a fabricated upstream failure
forever — over a capture that never failed. `read()` after `close()` rejected
with the same raw TypeError instead of an error from the SDK's own tree.

`startDrain` now returns immediately when the wrapper is closed and no drain
was ever started; a drain already in flight is left alone, because on the
fits-cap path the drain closes the delegate itself and its promise is what
`read()` awaits. `read()` gains a `ClosedResourceError('LoggedResponseBody')`
after the fits-cap and tail-consumed checks, in that order:

- fits-cap still serves a fresh non-consuming view after close — BODY-23 and
  BODY-28 both require it, so "closed" must not mean "unreadable";
- a consumed tail still reports `ConsumedBodyError`, which says more than
  "closed" (BODY-24);
- everything else with the delegate gone is IO-42's state error, because the
  captured prefix is not the whole body and serving it would hand the consumer
  a silently truncated response.

`error()` is unchanged and now cannot be poisoned. `snapshot()` is the
post-mortem accessor BODY-28 asks for.

`bun run api:local` on core: no change — `LoggedResponseBody` is `@internal`.

Refs #77, #67. BODY-26, BODY-27, BODY-28, IO-42.
…he three #77 fixes on Node

`toReadableStream`, `toWritableStream`, `TeeSink`'s bridge, `withRequestLogging`
and `withResponseLogging` had zero cases under `tests/node-conformance/`. Every
one is a hand-written underlying source or sink object, so what they exercise is
the runtime's own pull scheduling, cancel dispatch and reader-lock bookkeeping —
CLAUDE.md's runtime-divergent surface, and the tree's membership rule.

`io-byte-stream.test.mjs` (9 cases): one-chunk-at-a-time pull rather than an
eager drain; natural EOF closing the bridge but not the owning source, so an
outstanding peek is still readable (IO-19); cancel closing the source AND
releasing the caller stream's lock; a mid-stream read failure closing the source
itself, because the spec does not dispatch `cancel` on an errored stream;
`pipeTo` chaining close through the bridge to the destination; abort carrying its
reason instead of collapsing into a graceful close; a zero-length chunk dropped
rather than forwarded as a chunked-encoding terminator; the tee bridge routing
through the tee so the tap still sees the bytes; abort reaching the primary while
the tap survives.

`body-lifecycle.test.mjs` (13 cases): the multipart boundary round-trip; the
zero-length-chunk refusal and the declared-length-0 allowance; the request tap's
mirror, its clear-between-writes and its abort of a sink the delegate never
touched; the response tap's prefix-then-tail, its close-once cancel, its
inertness after `close()` and its repeatable fits-cap reads.

The multipart rows are the reason this is not optional. Bun's
`Response.formData()` accepts `boundary=a,b`; Node's rejects the whole body with
`TypeError: Failed to parse body as FormData`. The Bun suite was green over a
Content-Type no Node peer could parse, and only a case here reproduces it.

Both taps are `@internal`, so they come in by direct `dist/` path like `io/`.

Refs #77, #67. IO-16, IO-19, IO-26, HTTP-39, BODY-10, HTTP-51, BODY-17..28.
…ayBuffer

`BodyInit` excludes a view over a `SharedArrayBuffer`, so `Promise<Uint8Array>`
— whose default parameter is `ArrayBufferLike` — is not assignable to the
platform `Response` constructor the boundary round-trip feeds. Bun's runner
does not typecheck, so this only surfaced under `bun run typecheck`.

Refs #77, #67.
@Wahbeh-Mohammad
Wahbeh-Mohammad merged commit 4212de3 into audit/remediation-67 Sep 5, 2026
3 checks passed
@Wahbeh-Mohammad
Wahbeh-Mohammad deleted the audit/67/77-body-lifecycle branch September 7, 2026 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant