Conversation
…ODY_ALREADY_USED The used-body guard added in #33118 covers Body.Value.Used but a Response whose ReadableStream body the handler already consumed stays Body.Value.Locked with a disturbed stream in the JS-side cache. The server only checked is_locked() on that stream, so a disturbed-but-unlocked stream (await resp.text(), or getReader()+read()+releaseLock()) fell through to do_render_stream and produced a silent empty 200, never invoking error(). Check is_disturbed() on the stream and route it to the same ERR_BODY_ALREADY_USED error path as Body.Value.Used.
|
Status: diff is green; CI failures are unrelated to this change. Reproduced with the repro from the report:
CI build #83412: the only non-flaky failure is |
WalkthroughChangesResponse body reuse handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — closes the ReadableStream gap in the existing ERR_BODY_ALREADY_USED handling from #33118.
What was reviewed:
is_disturbed()is a plain C++ field read (stream->m_disturbed, WebStreamsExports.cpp:115) — no user-JS re-entry or exception concerns.- Cleanup via
response_body_readable_stream_ref.deinit()matches the siblingSource::Invalidearly-return;is_aborted_or_ended()was already checked at the top of theLockedarm. - Error construction and
run_error_handlercall are byte-identical to the existingBody::Value::Usedarm. - Tests extend the existing file with an
it.eachmatrix (start/pull sources, partial-read+releaseLock) and assert exact error code/name/message.
Extended reasoning...
Overview
The PR adds a single is_disturbed() guard in do_render_with_body's Body::Value::Locked arm (RequestContext.rs:3167-3179), routing consumed-ReadableStream response bodies to the same ERR_BODY_ALREADY_USED error handler that already fires for Body::Value::Used (string/Blob/Uint8Array bodies, added in #33118). The test file gains an it.each matrix over five body kinds plus a partial-read case.
Security risks
None. This tightens behavior — a case that previously silently served an empty/truncated 200 now surfaces as a 500 via the user's error handler. No new input parsing, no auth/crypto, no untrusted-data handling.
Level of scrutiny
Low-to-medium. The change is 14 lines of Rust that mirror two adjacent sibling patterns exactly: the error object and run_error_handler call are copied from the Body::Value::Used arm at line 3122, and the .deinit() cleanup matches the Source::Invalid branch at line 3202. ReadableStream__isDisturbed (WebStreamsExports.cpp:115) is a dynamicDowncast + bool field read — it cannot throw or run user JS, so no exception-scope or re-entrancy concerns. The is_aborted_or_ended() guard at the top of the Locked arm (line 3145) already covers this path, matching the existing is_locked() branch which also doesn't re-check.
Other factors
- The new check is placed before the existing
is_locked()check, which is correct ordering: a fully-consumed stream is disturbed but not locked, andBODY_ALREADY_USEDis the more specific/helpful error. - Tests are in the right file, use
port: 0/await using, assert exact{code, name, message}, and the PR description confirms they fail on system bun and pass on the debug build. - The partial-read test explicitly guards against the old "serve the unread remainder" behavior with a comment and exact-body assertion.
- No prior reviewer comments to address; only a robobun status note in the timeline.
…42609) ### Problem - `Bun.serve()` sends a second `Response` around an already sent `ReadableStream` as a `200` with an empty body. The `error` handler does not run. - A native sink consumer (`Bun.serve()`, a `fetch()` upload, `Bun.write()`, `Bun.spawn()` stdin, S3, `HTMLRewriter`) pumps a JS-backed stream in `BunStreamSource.cpp`. At the end `rsisFinally` (`:853`) releases the reader and `directStreamOnClose` (`:662`) drops the lock: `locked === false`. A `type: "direct"` stream, so every async iterable body, is not disturbed either. `RequestContext.rs:3152` checks the lock alone. - Found by a comparison with node v26.3.0, not by a user report. ### Fix - `rsisBegin` and `readDirectStream` call the new `JSReadableStream::markConsumedAsBody()` once the pump holds the stream. The stream stays locked after the pump lets go. - Behaviour change: after such a consumer took a stream, `getReader()`, `tee()`, `pipeTo()` and `cancel()` fail with a `TypeError`. Body mixin methods (#42116) and natively wired sinks (#42477) already do this. Neither is released. - `pipeTo()`, async iteration and `Bun.readableStreamTo*()` do not reach these pumps and still unlock. - Verified: `web-stream-state.test.ts` (30 new tests fail on main), `body.test.ts` (12), `serve-reused-response.test.ts` (2). Self-reviewed: 3 concerns, 2 addressed, 1 out of scope (Notes). ### Background - A JSSink is a native sink (`HTTPResponseSink`, `FileSink`). `JSSink::assign_to_stream` pumps a stream into it. - `readStreamIntoSink` reads through a default reader. `readDirectStream` gives the sink to the `pull()` of a direct stream. - `m_consumedAsBody` (#42116) is a bit that `isReadableStreamLocked()` includes and nothing clears. The fetch spec never releases the reader of a body. <details><summary>Notes</summary> **The rule.** A consumer that takes a stream as a body keeps it locked: the body mixin methods (#42116), `to_any_blob` lifts (#42516), natively wired `ByteStream`/`FileReader` sinks (#42477), and now the two pumps. A reader at the stream level releases its lock as the Streams spec says. Checked on this branch: `pipeTo()`, `pipeThrough()`, `for await`, `getReader()` + `releaseLock()`, `Bun.readableStreamToText()`, `Bun.readableStreamToArrayBuffer()`, `stream.text()`, `stream.bytes()`, `stream.json()`, `stream.blob()` all end with `locked === false`. **Scope of the pumps.** `readStreamIntoSink` and `readDirectStream` have one caller, `assignToStream`, which only `JSSinkController__assignToStream` calls (Rust `JSSink::assign_to_stream`: `HTTPResponseSink` and its TLS and HTTP/3 siblings, `FetchRequestBodySink`, `NetworkSink`, `FileSink`, `RewriterPipe`). **State on main.** The lock state after the hand-off depends on how the pump ended. A clean end and a sink that closes early (client gone, aborted upload, child exited) go through `rsisFinish` and release the reader one microtask after the stream closes. A producer error goes through `rsisAbrupt`, which orphans the reader, so that stream stays locked. A direct stream is never disturbed. **Repro (the user-visible part).** ```js const stream = new ReadableStream({ start(c) { c.enqueue(new TextEncoder().encode("payload")); c.close(); } }); const responses = [new Response(stream), new Response(stream)]; await using server = Bun.serve({ port: 0, fetch: () => responses.shift(), error: e => new Response(e.code, { status: 500 }) }); for (let i = 0; i < 2; i++) { const r = await fetch(server.url); console.log(r.status, JSON.stringify(await r.text())); } // main: 200 "payload", 200 "" // branch: 200 "payload", 500 "ERR_STREAM_CANNOT_PIPE" (Stream already used, please create a new one) ``` For a direct stream `new Response(stream)` per request shows the same on main: `200 "hello"`, `200 ""`, `500`. On this branch the second `new Response(stream)` throws `Body object should not be disturbed or locked`. **Other observable changes.** `request.bodyUsed` after `fetch(request)` with an async iterable body is now `true` (was `false`, the stream was not disturbed). `Bun.readableStreamToText(stream)` on a stream a sink holds rejects with "ReadableStream has already been used" in place of "ReadableStream is locked" (same `ERR_INVALID_STATE`). **Bare streams.** `fetch(url, { body: stream })`, `Bun.write(path, stream)` and `Bun.spawn({ stdin: stream })` keep a bare stream locked too. Natively wired sources already do this for the same calls (`ReadableStream__lockNative` never unlocks, see "errors a Bun.file() stream whose file does not open" from #42477). **node v26.3.0.** `fetch(url, { method: "POST", body: jsStream, duplex: "half" })`: afterwards `locked === true` and `getReader()` throws. The same after an upload that an `AbortSignal` stopped. The other five consumers are Bun APIs with no Node counterpart. **The mark comes after the reader acquisition.** A stream that another reader already holds is not marked. `Bun.serve()`, `fetch()`, `Bun.write()` and `HTMLRewriter` reject such a stream before the pump. `Bun.spawn()` stdin checks only `is_disturbed` (`stdio.rs:388`, `:536`), so a locked stream reaches the pump there. **Out of scope (the concern not addressed).** `rsisFinally` calls `clearStreamControllerSlots` also when the pump never got the lock. `Bun.spawn({ stdin: stream })` with a stream the caller holds through `getReader()` reaches that: the caller's `reader.read()` then never settles. This is the same on main and on this branch. #41532 rejects a locked stdin stream before the pump. **Not covered here.** - A handler that drains or partly reads a stream with its own reader, releases it, and then returns it in a `Response` still gets a `200` with the empty or remaining body. #36110 covers that gate. - Handing the same Node `Readable` or generator object to a second consumer makes a new stream each time. **Changed tests.** Three tests in `bun-write.test.js` (from #42114) read the terminal state through `stream.getReader().closed`. One test in `streams.test.js` (from #33781) polled `ts.readable.locked` to wait for the sink teardown. Both observed the unlock as a means, not as the subject. They use `finished()` from `node:stream/promises` now. The `streams.test.js` test still proves the teardown ran: `desiredSize` reads `null` only after the controller slot is cleared, and reads `0` for a stream that is only closed. **Fail before.** With `src/` from main and `bun bd`: `web-stream-state.test.ts` 30 of 46 fail, `body.test.ts` 12 of 778 fail, `serve-reused-response.test.ts` 2 of 9 fail, and the three `bun-write.test.js` tests fail on the new `locked` assertion. With this branch all pass. On main only the `Bun.serve()` row of "the stdout of a running child" fails: the other four consumers wire a pipe-backed `FileReader` natively. **Built-in JS.** I searched `src/js` for code that touches a stream after a native sink took it. The only `stream.cancel()` on a web stream is `ReadableFromWeb._destroy`, which #42573 handles for the body mixin case. **Earlier reports of the same class.** #7001 (fixed in #7861) and #6860. **Suites run on the debug build.** `test/js/web/streams/{streams,streams-leak,readable-stream-blob-consumed,readable-stream-terminal-barrier-release,transform-stream-leak}`, `test/js/third_party/wpt-streams`, `test/js/web/fetch/{body,body-stream,body-stream-excess,body-clone,body-async-iterator,body-mixin-errors,blob-write,fetch,fetch.stream,fetch-backpressure,fetch-abort-stream-body,fetch-stream-cancel-leak,fetch-redirect,response}`, `test/js/web/request/request`, `test/js/bun/http/{serve,bun-server,serve-reused-response,serve-direct-readable-stream,serve-body-leak,serve-pending-promise-abort-leak,async-iterator-stream,serve-async-stream-client-abort,serve-error-handler-stream,serve-response-stream-sink-leak,serve-stream-body-error,serve-stream-reject-flush-leak}`, `test/js/bun/spawn/{spawn,spawn-stdin-readable-stream}`, `test/js/bun/io/bun-write`, `test/js/workerd/{html-rewriter,html-rewriter-leak}`, `test/js/bun/s3/{s3-stream-cancel-leak,s3-stream-error-gc,s3-upload-stream-gc,s3-write-to-file-sync-close,s3-connection-close}`, `test/js/node/stream/{node-stream,web-stream-state}`, `test/js/node/async_hooks/AsyncLocalStorage`, `test/regression/issue/07001`. The failures that remain also fail with `src/` from main in this container: IPv6, tests that need a non-root user, external hosts, and 5 s timeouts of the debug build. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/streams/streams.test.js, test/js/bun/io/bun-write.test.js <!-- robobun:evidence:end -->
…ven-sh#42609) ### Problem - `Bun.serve()` sends a second `Response` around an already sent `ReadableStream` as a `200` with an empty body. The `error` handler does not run. - A native sink consumer (`Bun.serve()`, a `fetch()` upload, `Bun.write()`, `Bun.spawn()` stdin, S3, `HTMLRewriter`) pumps a JS-backed stream in `BunStreamSource.cpp`. At the end `rsisFinally` (`:853`) releases the reader and `directStreamOnClose` (`:662`) drops the lock: `locked === false`. A `type: "direct"` stream, so every async iterable body, is not disturbed either. `RequestContext.rs:3152` checks the lock alone. - Found by a comparison with node v26.3.0, not by a user report. ### Fix - `rsisBegin` and `readDirectStream` call the new `JSReadableStream::markConsumedAsBody()` once the pump holds the stream. The stream stays locked after the pump lets go. - Behaviour change: after such a consumer took a stream, `getReader()`, `tee()`, `pipeTo()` and `cancel()` fail with a `TypeError`. Body mixin methods (oven-sh#42116) and natively wired sinks (oven-sh#42477) already do this. Neither is released. - `pipeTo()`, async iteration and `Bun.readableStreamTo*()` do not reach these pumps and still unlock. - Verified: `web-stream-state.test.ts` (30 new tests fail on main), `body.test.ts` (12), `serve-reused-response.test.ts` (2). Self-reviewed: 3 concerns, 2 addressed, 1 out of scope (Notes). ### Background - A JSSink is a native sink (`HTTPResponseSink`, `FileSink`). `JSSink::assign_to_stream` pumps a stream into it. - `readStreamIntoSink` reads through a default reader. `readDirectStream` gives the sink to the `pull()` of a direct stream. - `m_consumedAsBody` (oven-sh#42116) is a bit that `isReadableStreamLocked()` includes and nothing clears. The fetch spec never releases the reader of a body. <details><summary>Notes</summary> **The rule.** A consumer that takes a stream as a body keeps it locked: the body mixin methods (oven-sh#42116), `to_any_blob` lifts (oven-sh#42516), natively wired `ByteStream`/`FileReader` sinks (oven-sh#42477), and now the two pumps. A reader at the stream level releases its lock as the Streams spec says. Checked on this branch: `pipeTo()`, `pipeThrough()`, `for await`, `getReader()` + `releaseLock()`, `Bun.readableStreamToText()`, `Bun.readableStreamToArrayBuffer()`, `stream.text()`, `stream.bytes()`, `stream.json()`, `stream.blob()` all end with `locked === false`. **Scope of the pumps.** `readStreamIntoSink` and `readDirectStream` have one caller, `assignToStream`, which only `JSSinkController__assignToStream` calls (Rust `JSSink::assign_to_stream`: `HTTPResponseSink` and its TLS and HTTP/3 siblings, `FetchRequestBodySink`, `NetworkSink`, `FileSink`, `RewriterPipe`). **State on main.** The lock state after the hand-off depends on how the pump ended. A clean end and a sink that closes early (client gone, aborted upload, child exited) go through `rsisFinish` and release the reader one microtask after the stream closes. A producer error goes through `rsisAbrupt`, which orphans the reader, so that stream stays locked. A direct stream is never disturbed. **Repro (the user-visible part).** ```js const stream = new ReadableStream({ start(c) { c.enqueue(new TextEncoder().encode("payload")); c.close(); } }); const responses = [new Response(stream), new Response(stream)]; await using server = Bun.serve({ port: 0, fetch: () => responses.shift(), error: e => new Response(e.code, { status: 500 }) }); for (let i = 0; i < 2; i++) { const r = await fetch(server.url); console.log(r.status, JSON.stringify(await r.text())); } // main: 200 "payload", 200 "" // branch: 200 "payload", 500 "ERR_STREAM_CANNOT_PIPE" (Stream already used, please create a new one) ``` For a direct stream `new Response(stream)` per request shows the same on main: `200 "hello"`, `200 ""`, `500`. On this branch the second `new Response(stream)` throws `Body object should not be disturbed or locked`. **Other observable changes.** `request.bodyUsed` after `fetch(request)` with an async iterable body is now `true` (was `false`, the stream was not disturbed). `Bun.readableStreamToText(stream)` on a stream a sink holds rejects with "ReadableStream has already been used" in place of "ReadableStream is locked" (same `ERR_INVALID_STATE`). **Bare streams.** `fetch(url, { body: stream })`, `Bun.write(path, stream)` and `Bun.spawn({ stdin: stream })` keep a bare stream locked too. Natively wired sources already do this for the same calls (`ReadableStream__lockNative` never unlocks, see "errors a Bun.file() stream whose file does not open" from oven-sh#42477). **node v26.3.0.** `fetch(url, { method: "POST", body: jsStream, duplex: "half" })`: afterwards `locked === true` and `getReader()` throws. The same after an upload that an `AbortSignal` stopped. The other five consumers are Bun APIs with no Node counterpart. **The mark comes after the reader acquisition.** A stream that another reader already holds is not marked. `Bun.serve()`, `fetch()`, `Bun.write()` and `HTMLRewriter` reject such a stream before the pump. `Bun.spawn()` stdin checks only `is_disturbed` (`stdio.rs:388`, `:536`), so a locked stream reaches the pump there. **Out of scope (the concern not addressed).** `rsisFinally` calls `clearStreamControllerSlots` also when the pump never got the lock. `Bun.spawn({ stdin: stream })` with a stream the caller holds through `getReader()` reaches that: the caller's `reader.read()` then never settles. This is the same on main and on this branch. oven-sh#41532 rejects a locked stdin stream before the pump. **Not covered here.** - A handler that drains or partly reads a stream with its own reader, releases it, and then returns it in a `Response` still gets a `200` with the empty or remaining body. oven-sh#36110 covers that gate. - Handing the same Node `Readable` or generator object to a second consumer makes a new stream each time. **Changed tests.** Three tests in `bun-write.test.js` (from oven-sh#42114) read the terminal state through `stream.getReader().closed`. One test in `streams.test.js` (from oven-sh#33781) polled `ts.readable.locked` to wait for the sink teardown. Both observed the unlock as a means, not as the subject. They use `finished()` from `node:stream/promises` now. The `streams.test.js` test still proves the teardown ran: `desiredSize` reads `null` only after the controller slot is cleared, and reads `0` for a stream that is only closed. **Fail before.** With `src/` from main and `bun bd`: `web-stream-state.test.ts` 30 of 46 fail, `body.test.ts` 12 of 778 fail, `serve-reused-response.test.ts` 2 of 9 fail, and the three `bun-write.test.js` tests fail on the new `locked` assertion. With this branch all pass. On main only the `Bun.serve()` row of "the stdout of a running child" fails: the other four consumers wire a pipe-backed `FileReader` natively. **Built-in JS.** I searched `src/js` for code that touches a stream after a native sink took it. The only `stream.cancel()` on a web stream is `ReadableFromWeb._destroy`, which oven-sh#42573 handles for the body mixin case. **Earlier reports of the same class.** oven-sh#7001 (fixed in oven-sh#7861) and oven-sh#6860. **Suites run on the debug build.** `test/js/web/streams/{streams,streams-leak,readable-stream-blob-consumed,readable-stream-terminal-barrier-release,transform-stream-leak}`, `test/js/third_party/wpt-streams`, `test/js/web/fetch/{body,body-stream,body-stream-excess,body-clone,body-async-iterator,body-mixin-errors,blob-write,fetch,fetch.stream,fetch-backpressure,fetch-abort-stream-body,fetch-stream-cancel-leak,fetch-redirect,response}`, `test/js/web/request/request`, `test/js/bun/http/{serve,bun-server,serve-reused-response,serve-direct-readable-stream,serve-body-leak,serve-pending-promise-abort-leak,async-iterator-stream,serve-async-stream-client-abort,serve-error-handler-stream,serve-response-stream-sink-leak,serve-stream-body-error,serve-stream-reject-flush-leak}`, `test/js/bun/spawn/{spawn,spawn-stdin-readable-stream}`, `test/js/bun/io/bun-write`, `test/js/workerd/{html-rewriter,html-rewriter-leak}`, `test/js/bun/s3/{s3-stream-cancel-leak,s3-stream-error-gc,s3-upload-stream-gc,s3-write-to-file-sync-close,s3-connection-close}`, `test/js/node/stream/{node-stream,web-stream-state}`, `test/js/node/async_hooks/AsyncLocalStorage`, `test/regression/issue/07001`. The failures that remain also fail with `src/` from main in this container: IPv6, tests that need a non-root user, external hosts, and 5 s timeouts of the debug build. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/streams/streams.test.js, test/js/bun/io/bun-write.test.js <!-- robobun:evidence:end -->
Repro
String/Blob/Uint8Array/
Response.jsonbodies already 500 withERR_BODY_ALREADY_USEDhere (since #33118). A Response whose ReadableStream body the handler consumed was served as a silent empty200withContent-Length: 0, and a partially-read stream (getReader()+read()+releaseLock()) silently served only the unread remainder.Cause
After
await resp.text()on a stream body, the body staysBody::Value::Lockedwithaction = GetTextand the disturbed stream still in the Response's JS-sidestreamcache slot.do_render_with_bodyre-reads that stream viaget_body_readable_stream, but the Locked arm only checkedstream.is_locked(), notstream.is_disturbed(). A consumed stream is disturbed but not locked, so it fell through todo_render_streamand rendered as empty.Fix
Check
is_disturbed()on the response's ReadableStream indo_render_with_bodyand route to the sameERR_BODY_ALREADY_USEDerror-handler path as the existingBody::Value::Usedarm.Verification
Extended
test/js/bun/http/serve-reused-response.test.tsto cover the stream variants (start-source, pull-source, partial-read-then-releaseLock) alongside the existing string/blob cases.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file