Repository navigation
Conversation
Body.formData() rejected early on an unrecognized Content-Type without touching the body, leaving bodyUsed false and allowing a subsequent text()/json() to read the full payload. The spec's "consume body" algorithm fully reads the body before packageData runs (and may throw), so a rejected MIME check must still consume it. Node/undici agree: bodyUsed is true after the rejection and further reads throw TypeError. For synchronous bodies the fix is to call use_as_any_blob() before the encoding check. For Locked bodies the encoder Option is passed through Action::GetFormData: a None encoder still drains the ReadableStream (via a contentType sentinel the fulfillment handler rejects on) or still buffers the pending HTTP body before resolve() rejects. Null bodies stay unconsumed, matching Node.
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 11:16 PM PT - Jul 25th, 2026
❌ @robobun, your commit 02ea67d has 2 failures in
🧪 To try this PR locally: bunx bun-pr 35847That installs a local version of the PR into your bun-35847 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this changes user-visible Body.formData() semantics (previously-working try { formData() } catch { json() } now throws) and threads a false sentinel through the promise-reaction context as a workaround for null/undefined being dropped — worth a maintainer's eyes on the approach.
Checked: the contentType.isFalse() branch in the shared onReadableStreamToFormDataFulfilled handler is also reachable from public Bun.readableStreamToFormData(stream, false) — that call now rejects with ERR_FORMDATA_PARSE_ERROR instead of falling through to FormData.from(blob, false), which was already invalid usage. The removed ZigString/ZigStringJsc imports have no remaining uses in Body.rs. Null-body path verified: use_as_any_blob() leaves Value::Null as Null, so bodyUsed stays false per the test.
Extended reasoning...
Overview
The PR aligns Body.formData() with the fetch spec's consume body algorithm: the body must be fully read before packageData runs the MIME check, so a MIME-type rejection must still mark the body as used. Three code paths in src/runtime/webcore/Body.rs are updated (synchronous body via use_as_any_blob() before the encoder check; Value::Locked with a readable stream via Action::GetFormData(None) → JSValue::FALSE sentinel → drain then reject; Value::Locked pending resolve via the form_data_slot.take() None arm), plus a 5-line addition to jsWebStreamsHandler_onReadableStreamToFormDataFulfilled in BunStreamConsumers.cpp to convert the sentinel into ERR_FORMDATA_PARSE_ERROR. A shared form_data_mime_error helper deduplicates the error construction. Tests in test/js/web/fetch/body.test.ts cover string/Uint8Array/Blob/ReadableStream/JSON bodies × Request/Response, the null-body carve-out, and an incoming Bun.serve request.
Security risks
None identified. This tightens (not loosens) body-reuse semantics; no new parsing of untrusted input, no auth/crypto/permission surface.
Level of scrutiny
Moderate-to-high. This is a deliberate, user-visible behavioral change to a core Web API path (Request/Response body mixin) — code that previously relied on Bun's non-spec fallback (try { formData() } catch { json() }) will now throw. The change is spec-correct and matches Node/undici, but it warrants a maintainer confirming the compat trade-off is acceptable. Separately, the JSValue::FALSE sentinel plumbed through performPromiseThenWithContext is an implementation choice the author explicitly justified (null/undefined are dropped by the reaction-context layer) after comment-cop flagged it — a maintainer should confirm this is the preferred shape vs. e.g. a distinct action variant or a dedicated drain-then-reject path.
Other factors
- The previously-unreachable
form_data_slot.take() == Nonearm (which used to reject with "Internal error: task for FormData must not be null") is now a legitimate path; the message change from internal-error toERR_FORMDATA_PARSE_ERRORis correct. .err(...).reject()andJSPromise::rejected_promise(global, form_data_mime_error(global)).to_js()are equivalent (ErrorCode::rejectcallsrejected_promiseinternally).- All comment-cop threads on the PR are resolved (comments trimmed in a1efe80 / 1f5cfdc).
- Test coverage is good and follows harness conventions (
port: 0,await using server,test.each, awaited.rejects).
Deferring for a human to sign off on the behavioral change and the sentinel design.
|
On the sentinel design: the alternative I considered was a distinct The |
There was a problem hiding this comment.
I didn't find any bugs, but this changes user-visible formData() behavior (spec-correct, but code doing try { formData() } catch { text() } will now break) and the author left the false-sentinel-vs-dedicated-drain-path design open for maintainer preference — worth a human look.
What was reviewed:
- Traced the three consume paths (sync
use_as_any_blob, Locked→ReadableStream drain, Locked→on_start_buffering→resolve()) — all mark the body used before rejecting;blob.detach()is called on every reject arm. Value::NullstaysNullthroughuse_as_any_blob(), so the null-body test'sbodyUsed === falseholds.- Checked the
contentType.isFalse()branch injsWebStreamsHandler_onReadableStreamToFormDataFulfilled: only reachable viaBun.readableStreamToFormData(stream, false), which already rejected (author confirmed; only the error code changes).
Extended reasoning...
Overview
Aligns Body.formData() with the fetch spec's consume body algorithm: when the Content-Type is not multipart/urlencoded, the body is now fully read and marked used before the ERR_FORMDATA_PARSE_ERROR rejection. Touches src/runtime/webcore/Body.rs (the BodyMixin::get_form_data sync path, the PendingValue::set_promise ReadableStream path, and the Value::resolve buffered-HTTP path), plus a 5-line sentinel check in BunStreamConsumers.cpp. New tests in body.test.ts cover Request/Response × {string, Uint8Array, Blob, ReadableStream, JSON content-type}, null body, and a Bun.serve incoming request.
Security risks
None identified. No new parsing of untrusted input; the change only reorders when consumption happens relative to an existing MIME-type rejection. The false sentinel is compared via JSValue::isFalse() (exact identity), not truthiness.
Level of scrutiny
Medium-high. Body.rs is on the hot path for every incoming HTTP request body and every Response consumer. The change is small (~35 lines of logic) and well-localized, but it is a deliberate observable behavior change to a Web API — code that relied on Bun's previous permissive fallback (try { formData() } catch { json() }) will now throw. That is spec-correct and matches Node/undici, but it is the kind of compat trade-off a maintainer should sign off on rather than a bot.
Other factors
- The
JSValue::FALSEsentinel threaded throughperformPromiseThenWithContextis a workaround for the reaction-context layer droppingnull/undefined. The author explicitly offered to replace it with a dedicated drain-then-reject path if preferred — that's an open design question. - The public
Bun.readableStreamToFormData(stream, false)API now rejects withERR_FORMDATA_PARSE_ERRORinstead of the previous "boundary must be a string or ArrayBufferView" TypeError. Minor, but a user-visible message change on a Bun-namespaced API. - The
Action::GetFormData(None)arm inValue::resolvepreviously represented an internal-error state ("task for FormData must not be null") and now represents the intended MIME-failure path. I verified there are no other producers ofAction::GetFormData(None)besides this PR'sset_promisecall site, so the repurposing is safe, but a maintainer may want to confirm no other in-flight work relies on that being an internal error. - Test coverage is thorough and the PR includes evidence that the tests fail on main and pass with the fix under both ASAN debug and release.
|
CI status: the diff is green. Gate (fail-before/fail-after on ASAN and release) passed. The red on builds #81875 and #81977 is the |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-26, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
…esponse (#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 #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 #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 #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.** #6983 (for #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 #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 -->
…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 -->
|
An update for anyone who lands here. On 1.4.3 a failed #44999 fixes that with no behaviour change: the type is checked before the payload is lifted out of the stream, so a failed #44969 rebuilds the change this PR proposed on current main: the body is read before the MIME check, so a failed #44930 fixes the same locked stream for a Response that is used as a |
What does this PR do?
Body.formData()rejected early on an unrecognized Content-Type without touching the body. After the TypeError,bodyUsedstayedfalseand a subsequenttext()/json()/blob()re-read the full payload. Code liketry { await req.formData() } catch { await req.json() }silently worked in Bun and threw everywhere else.The spec's consume body fully reads the body before
packageDataruns (which performs the MIME check and may throw), so a failed MIME check must still consume the body. Node/undici match the spec:bodyUsedistrueafter the rejection and further reads throw TypeError.How did you verify your code works?
New tests in
test/js/web/fetch/body.test.tscover string / Uint8Array / Blob / ReadableStream / JSON bodies on both Request and Response, a null body (stays unconsumed, matching Node), and an incomingBun.serverequest body. All fail on the previous build and pass with the fix.Fix
use_as_any_blob()before the encoding check so the body is markedUsedfirst.Option<encoder>throughAction::GetFormData. WithNone, the ReadableStream is still drained (via afalsecontentType sentinel that the fulfillment handler turns into theERR_FORMDATA_PARSE_ERRORrejection), and a pending HTTP body is still buffered beforeresolve()rejects.use_as_any_blob()leavesNullasNull), matching Node.[review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file