node-fetch, undici: end the body stream when a body method took the response - #42573
Merged
Jarred-Sumner merged 3 commits intoSep 14, 2026
Merged
Conversation
…esponse A body method (text(), json(), ...) keeps the web stream of the body locked since #42116. The node stream that the node-fetch and undici shims hand out as the body wraps that web stream and opens it on first use, so resume(), destroy() and iteration after a body method emitted 'error' (ReadableStream is locked). ReadableFromWeb takes a responseBody option. With it, a lock on the web stream that the wrapper has not opened yet means that the Response took the body: the wrapper ends and does not call getReader() or cancel(). Readable.fromWeb() does not set the option. node-fetch: the body methods no longer read the body getter first. That was needed when a wrapper made after the consume crashed. clone() drops the cached node stream, because the body moves to a new web stream.
Collaborator
Author
|
Updated 1:12 AM PT - Sep 13th, 2026
✅ @robobun, your commit d9e58e7b354ac0e13b82270a1f05601612e92299 passed in 🧪 To try this PR locally: bunx bun-pr 42573That installs a local version of the PR into your bun-42573 --bun |
Collaborator
Author
|
Status: fix pushed, waiting for CI. Reproduction (main at f04caca, and canary import nodeFetch from "node-fetch";
await using server = Bun.serve({ port: 0, fetch: () => new Response("hello") });
const res = await nodeFetch(server.url);
const body = res.body;
await res.arrayBuffer();
body.on("error", e => console.log("error:", e.message)).on("end", () => console.log("end"));
body.resume();
With the |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughThe change updates web-stream adapters and fetch integrations to handle response bodies that are locked by another consumer. It also adds Undici and node-fetch tests for stream lifecycle, concurrent reads, body consumption, streaming, and cloning. ChangesResponse body streams
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The response-body handling and clone behavior preserve independent streams without emitting errors for bodies already consumed by response methods. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The adapter change is well-gated behind the new responseBody flag and the test matrix is thorough, but since it adjusts lock/cancel semantics in the shared ReadableFromWeb adapter, a human look from someone familiar with #42116 would still be worthwhile.
What was reviewed:
_destroystill reachescallback(error)on the new taken-by-response branch (falls through to line 149);_readchecksstream.lockedbeforegetReader()so the adapter's own reader never trips#takenByResponse.Readable.fromWeb()(newStreamReadableFromReadableStream) does not passresponseBodyand still throws up front on a locked stream, so the public API is unchanged.- Removed body-method overrides in
node-fetch.tsare covered by the new "body read for the first time after the method" cases;clone()cache-drop matches node-fetch'sPassThroughreplacement. - Tests:
port: 0,await usingservers, event-driveneventsUntilClose(no sleeps), errors captured into the asserted array,secondChunkresolved on every path.
Extended reasoning...
Overview
This PR fixes an unreleased regression (from #42116) where the node-fetch and undici thirdparty shims' res.body node stream would emit 'error' (locked ReadableStream) instead of 'end'/'close' after a body method (text(), json(), etc.) had already consumed the underlying web stream. The fix adds an opt-in responseBody flag to ReadableFromWeb in src/js/internal/webstreams_adapters.ts: when set and the wrapped web stream is already locked before the adapter opened it, _read() ends the node stream cleanly with push(null) and _destroy() skips stream.cancel(). Both shims pass the flag; Readable.fromWeb() does not. The node-fetch Response also drops five body-method overrides (now redundant) and clears its cached node-stream wrapper on clone(). 27 new tests across both shim suites cover every body method × resume/destroy/for-await, mid-read, aborted text(), rejected formData(), the streaming happy path, and clone().
Security risks
None. This is JS-only stream-adapter plumbing in thirdparty compatibility shims. No auth, crypto, path handling, or untrusted-input parsing is touched. The responseBody option is internal-only (not exposed through Readable.fromWeb()'s validated options), and stream.locked is read the same way the file already reads it at line 578.
Level of scrutiny
Medium. The src change is small (~50 lines) and gated behind a flag that defaults to false, so the shared Readable.fromWeb() path is provably unchanged. I verified the two correctness edges called out in the repo's review conventions: _destroy still invokes callback on the new branch (it falls through past the if (stream) block to the existing callback(error) at line 149), and the #takenByResponse check in _read runs while this.#reader is still undefined and this.#stream is still set — once the adapter calls getReader() it clears #stream, so its own lock never re-enters the check. The #takenByResponse helper takes stream as an argument, so _destroy setting this.#stream = undefined before the check is fine. Tamper-resistance matches the file's existing style (direct .locked/.getReader()/.cancel() calls throughout).
Other factors
Test quality is high and follows REVIEW.md: added to existing suite files, port: 0, await using/using for servers, no setTimeout/sleep (event-driven eventsUntilClose with Promise.withResolvers gating), error events captured into the asserted array so a regression surfaces as a .toEqual mismatch rather than an unhandled rejection, combined-object .toEqual assertions, and a guard test that the normal streaming path still delivers every chunk. The removed node-fetch overrides (originally added for #6200) are covered by the "body read for the first time after the method" case in each .each block. The PR description includes a full before/after behavior table against 1.4.2. I'm deferring rather than approving only because the change adjusts cancel/lock semantics in a shared internal adapter and interacts with #42116's changes — a maintainer familiar with that PR is best placed to confirm the approach.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Jarred-Sumner
pushed a commit
that referenced
this pull request
Sep 14, 2026
…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 -->
Jarred-Sumner
deleted the
robobun/3931eb73/shim-body-stream-after-consume
branch
September 14, 2026 06:31
usrbinkat
pushed a commit
to usrbinkat/bun
that referenced
this pull request
Sep 15, 2026
…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 -->
usrbinkat
pushed a commit
to usrbinkat/bun
that referenced
this pull request
Sep 15, 2026
…esponse (oven-sh#42573) ### Problem - With the built-in `node-fetch` and `undici` shims, `res.body` emits `'error'` after a body method: `Invalid state: ReadableStream is locked` from `resume()`, `pipe()`, `for await`, and `Cannot cancel a locked ReadableStream` from `destroy()` (for example `finally { body.destroy() }`). Bun 1.4.2 and Node emit `'end'`, `'close'`. Regression from oven-sh#42116 (not released). No user reported it. - Cause: `ReadableFromWeb` (`src/js/internal/webstreams_adapters.ts`) calls `stream.getReader()` in the first `_read()` and `stream.cancel()` in `_destroy()`. Since oven-sh#42116 a body method keeps the web stream locked, so both throw. ### Fix - `ReadableFromWeb` takes a `responseBody` option. Both shims set it, `Readable.fromWeb()` does not. If the wrapper has not opened the web stream and the stream is locked, the Response took the body: `_read()` pushes `null`, `_destroy()` skips `cancel()`. - The check reads `stream.locked`, not `bodyUsed`: after an aborted `text()`, `bodyUsed` is `false` and the stream is locked. - `node-fetch`: the body methods no longer read `this.body` first, so the five overrides are deleted. `clone()` drops the cached node stream, because the body moves to a new web stream. - Verified: `test/js/node/http/node-fetch.test.js`, `test/js/first_party/undici/undici.test.ts` (27 new tests, 25 fail with main's `src/`). Self-reviewed: 3 concerns raised, 3 addressed. ### Background - Both shims hand out the body as a node `Readable` (`ReadableFromWeb`) around the web `ReadableStream` of a Bun `Response`. `text()`, `json()`, ... are the native methods and read the web stream directly. - `ReadableFromWeb` opens the web stream lazily, so a body method can take it while the wrapper has no reader. - Real `node-fetch` and `undici` read that node stream in `text()`, so it is ended afterwards. <details><summary>Notes</summary> **Origin.** Found during the review of oven-sh#42516. That PR lists this symptom as tracked separately. **Reproduction.** ```js import nodeFetch from "node-fetch"; await using server = Bun.serve({ port: 0, fetch: () => new Response("hello") }); const res = await nodeFetch(server.url); const body = res.body; await res.arrayBuffer(); body.on("error", e => console.log("error:", e.message)).on("end", () => console.log("end")); body.resume(); // 1.4.2: end // main: error: Invalid state: ReadableStream is locked ``` **Behaviour per case**, both shims, checked against a 1.4.2 binary. "clean" means `end`, `close` for `resume()`, `close` for `destroy()`, and no chunks for `for await`. | case | 1.4.2 | main | this PR | | --- | --- | --- | --- | | `body` taken, then `text()` / `json()` / `arrayBuffer()` / `blob()` / `formData()` / `buffer()` / `bytes()`, then `resume()`, `destroy()` or `for await` | clean | `error` | clean | | body method first, then `res.body` for the first time | clean (`bytes()` throws) | `error` | clean | | `text()` aborted by the signal (or `AbortSignal.timeout()`), then `resume()` / `destroy()` | clean | `error` | clean | | `formData()` rejects the content type, then `resume()` / `destroy()` | clean | `error` | clean | | `json()` rejects with a `SyntaxError`, then `resume()` / `destroy()` | clean | `error` | clean | | `text()` still pending, then `resume()` / `destroy()` | `error` (locked) | `error` | clean, and `text()` resolves with the full body | | `body` taken, then `res.clone()`, then read `res.body` | throws (locked by `tee()`) | throws | `res.body` is a new node stream and delivers the body. The old node stream ends. | | `res.body` streamed with `for await`, `on("data")`, `pipe()` | works | works | works | | `destroy()` on a body that nothing read | cancels the fetch | same | same | **Why `stream.locked`.** The first version of this change asked the Response for `bodyUsed`. That misses every body method that rejects after it took the stream: on main `bodyUsed` stays `false` there while the stream stays locked (node reports `true` and locked). The lock of the stream itself is what makes `getReader()` and `cancel()` throw, so the wrapper checks that. **Why the overrides are deleted.** oven-sh#6983 (for oven-sh#6200) made `text()`, `json()`, `arrayBuffer()`, `blob()`, `formData()` read `this.body` first, because a wrapper made after the consume crashed in the stream code of that time. Such a wrapper now ends without an error, and the test for each body method reads `res.body` for the first time after the method. Without the preload a plain `await res.json()` does not create a web stream and a node stream that nothing reads. **`clone()`.** `super.clone()` moves the body of the original to a new web stream (a `tee()` branch, or a fresh stream over the same bytes). The cached wrapper held the old stream, so `res.body` taken before `clone()` threw `ReadableStream is locked` on read after it, also on 1.4.2. With the lock check it would have ended without data instead. `clone()` now clears the cache. Real `node-fetch` also replaces `body` with a new `PassThrough` in `clone()`. **A body method that fails** (abort, network error, bad JSON) leaves the node stream ended without an error. The rejected promise of the body method carries the error. **Not in this PR.** On main `bodyUsed` is `false` after a body method that rejects when `.body` was read before it. That is native (`src/runtime/webcore/Body.rs`) and oven-sh#35847 covers the `formData()` part. **Suites run** on the debug build: `node-fetch.test.js`, `undici.test.ts`, `node-fetch-cjs.test.js`, `node-fetch-primordials.test.ts`, `undici-primordials.test.ts`, `node-stream.test.js`, `node-http-agent-free-socket.test.ts`, regression `26225`, `014865`, `04947`, and the Node `test-stream-readable-from-web-termination.js`, `test-readable-from-web-enqueue-then-close.js`, `test-whatwg-webstreams-adapters-to-*.js` files. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 2 · 5 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 25 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/first_party/undici/undici.test.ts test/js/node/http/node-fetch.test.js bun test v1.4.3 (6a92015) test/js/first_party/undici/undici.test.ts: (pass) undici > request > should make a GET request when passed a URL string [68.88ms] (pass) undici > request > should error when body has already been consumed [13.68ms] 97 | const url = new URL(method, server.url).href; 98 | 99 | for (const [action, expected] of bodyActions) { 100 | const { body } = await request(url); 101 | await body[method](); 102 | expect(await eventsUntilClose(body, () => body[action]())).toEqual([...expected]); ^ error: expect(received).toEqual(expected) [ - "end", + [TypeError: Invalid state: ReadableStream is locked], "close", ] - Expected - 1 + Received + 1 at <anonymous> (/workspace/bun/test/js/first_party/undici/undici.test.ts:102:72) (fail) undici > request > body stream when a body method took the r ... (truncated) release without fix: all passed bun test v1.4.3-canary.1 (7b5b9c1) test/js/first_party/undici/undici.test.ts: (pass) undici > request > should make a GET request when passed a URL string [2.61ms] (pass) undici > request > should error when body has already been consumed [0.37ms] (pass) undici > request > body stream when a body method took the response > ends without an error after arrayBuffer() consumed it [5.40ms] (pass) undici > request > body stream when a body method took the response > ends without an error after blob() consumed it [1.87ms] (pass) undici > request > body stream when a body method took the response > ends without an error after formData() consumed it [2.11ms] (pass) undici > request > body stream when a body method took the response > ends without an error after json() consumed it [2.05ms] (pass) undici > request > body stream when a body method took the response > ends without an error after text() consumed it [1.75ms] (pass) undici > request > body stream when a body method took the response > resume() leaves a body method that is still reading alone [2.04ms] (pass) undici > request > body stream when a body method took the response > destroy() leaves a body method that ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/first_party/undici/undici.test.ts test/js/node/http/node-fetch.test.js bun test v1.4.3 (6a92015) test/js/first_party/undici/undici.test.ts: (pass) undici > request > should make a GET request when passed a URL string [78.45ms] (pass) undici > request > should error when body has already been consumed [15.95ms] (pass) undici > request > body stream when a body method took the response > ends without an error after arrayBuffer() consumed it [258.02ms] (pass) undici > request > body stream when a body method took the response > ends without an error after blob() consumed it [57.58ms] (pass) undici > request > body stream when a body method took the response > ends without an error after formData() consumed it [50.54ms] (pass) undici > request > body stream when a body method took the response > ends without an error after json() consumed it [48.81ms] (pass) undici > request > body stream when a body method took the response > ends without an error after text() consumed it [48.04ms] (pass) undici > request > body stream when a bo ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 639ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/21] gen JS modules (bundle-modules) Preprocess modules (7038ms) Bundle modules (48ms) Postprocesss modules (24ms) Bundle Functions (489ms) Generate Code (42ms) [7.65s] Bundled "src/js" for production 2600 kb 197 internal modules 13 native modules 50 internal functions across 16 files [1/6] cargo bun_runtime → libbun_runtime.a �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[92m Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys) �[1m�[92m Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety) �[1m�[92m Compiling�[0m bun_base64 v0.0.0 (/workspace/bun/src/base64) �[1m�[92m Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys) �[1m�[92m Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys) �[1m�[92m Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd) �[1m�[92m Compili ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/js/internal/webstreams_adapters.ts | 30 ++++-- src/js/thirdparty/node-fetch.ts | 36 +------ src/js/thirdparty/undici.js | 2 +- test/js/first_party/undici/undici.test.ts | 127 +++++++++++++++++++++++++ test/js/node/http/node-fetch.test.js | 150 ++++++++++++++++++++++++++++++ 5 files changed, 304 insertions(+), 41 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 2 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/js/internal/webstreams_adapters.ts 5 11 17 src/js/thirdparty/node-fetch.ts 3 3 17 src/js/thirdparty/undici.js 2 3 17 test/js/first_party/undici/undici.test.ts 1 1 12 test/js/node/http/node-fetch.test.js 2 3 17 ``` </details> <!-- robobun:evidence:end -->
This was referenced Sep 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
node-fetchandundicishims,res.bodyemits'error'after a body method:Invalid state: ReadableStream is lockedfromresume(),pipe(),for await, andCannot cancel a locked ReadableStreamfromdestroy()(for examplefinally { body.destroy() }). Bun 1.4.2 and Node emit'end','close'. Regression from Keep body stream bookkeeping independent of the body's source #42116 (not released). No user reported it.ReadableFromWeb(src/js/internal/webstreams_adapters.ts) callsstream.getReader()in the first_read()andstream.cancel()in_destroy(). Since Keep body stream bookkeeping independent of the body's source #42116 a body method keeps the web stream locked, so both throw.Fix
ReadableFromWebtakes aresponseBodyoption. Both shims set it,Readable.fromWeb()does not. If the wrapper has not opened the web stream and the stream is locked, the Response took the body:_read()pushesnull,_destroy()skipscancel().stream.locked, notbodyUsed: after an abortedtext(),bodyUsedisfalseand the stream is locked.node-fetch: the body methods no longer readthis.bodyfirst, so the five overrides are deleted.clone()drops the cached node stream, because the body moves to a new web stream.test/js/node/http/node-fetch.test.js,test/js/first_party/undici/undici.test.ts(27 new tests, 25 fail with main'ssrc/). Self-reviewed: 3 concerns raised, 3 addressed.Background
Readable(ReadableFromWeb) around the webReadableStreamof a BunResponse.text(),json(), ... are the native methods and read the web stream directly.ReadableFromWebopens the web stream lazily, so a body method can take it while the wrapper has no reader.node-fetchandundiciread that node stream intext(), so it is ended afterwards.Notes
Origin. Found during the review of #42516. That PR lists this symptom as tracked separately.
Reproduction.
Behaviour per case, both shims, checked against a 1.4.2 binary. "clean" means
end,closeforresume(),closefordestroy(), and no chunks forfor await.bodytaken, thentext()/json()/arrayBuffer()/blob()/formData()/buffer()/bytes(), thenresume(),destroy()orfor awaiterrorres.bodyfor the first timebytes()throws)errortext()aborted by the signal (orAbortSignal.timeout()), thenresume()/destroy()errorformData()rejects the content type, thenresume()/destroy()errorjson()rejects with aSyntaxError, thenresume()/destroy()errortext()still pending, thenresume()/destroy()error(locked)errortext()resolves with the full bodybodytaken, thenres.clone(), then readres.bodytee())res.bodyis a new node stream and delivers the body. The old node stream ends.res.bodystreamed withfor await,on("data"),pipe()destroy()on a body that nothing readWhy
stream.locked. The first version of this change asked the Response forbodyUsed. That misses every body method that rejects after it took the stream: on mainbodyUsedstaysfalsethere while the stream stays locked (node reportstrueand locked). The lock of the stream itself is what makesgetReader()andcancel()throw, so the wrapper checks that.Why the overrides are deleted. #6983 (for #6200) made
text(),json(),arrayBuffer(),blob(),formData()readthis.bodyfirst, because a wrapper made after the consume crashed in the stream code of that time. Such a wrapper now ends without an error, and the test for each body method readsres.bodyfor the first time after the method. Without the preload a plainawait res.json()does not create a web stream and a node stream that nothing reads.clone().super.clone()moves the body of the original to a new web stream (atee()branch, or a fresh stream over the same bytes). The cached wrapper held the old stream, sores.bodytaken beforeclone()threwReadableStream is lockedon read after it, also on 1.4.2. With the lock check it would have ended without data instead.clone()now clears the cache. Realnode-fetchalso replacesbodywith a newPassThroughinclone().A body method that fails (abort, network error, bad JSON) leaves the node stream ended without an error. The rejected promise of the body method carries the error.
Not in this PR. On main
bodyUsedisfalseafter a body method that rejects when.bodywas read before it. That is native (src/runtime/webcore/Body.rs) and #35847 covers theformData()part.Suites run on the debug build:
node-fetch.test.js,undici.test.ts,node-fetch-cjs.test.js,node-fetch-primordials.test.ts,undici-primordials.test.ts,node-stream.test.js,node-http-agent-free-socket.test.ts, regression26225,014865,04947, and the Nodetest-stream-readable-from-web-termination.js,test-readable-from-web-enqueue-then-close.js,test-whatwg-webstreams-adapters-to-*.jsfiles.[human-review] gate passed · iteration 2 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 2
evidence per changed file