Conversation
WalkthroughAdds a ChangesContent-Type preservation and 206 slice status fixes
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 1:44 AM PT - Aug 13th, 2026
❌ @robobun, your commit cf1d285 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 32913That installs a local version of the PR into your bun-32913 --bun |
There was a problem hiding this comment.
I didn't find any issues — the fix looks correct and well-tested — but since this changes the eager allocation behavior in the core Request/Response constructors (hot path, borrowck-sensitive Rust), it's worth a human look.
Extended reasoning...
Overview
This PR fixes an order-dependence bug: the body-derived Content-Type (from FormData, URLSearchParams, or a typed Blob) was only copied into headers lazily on first .headers access. If the body was consumed first (replacing the internal Blob with Value::Used), the lazy path found no blob and the header — including the multipart boundary — was lost. The fix materializes the headers eagerly at construction time in Request::construct_into and Response::constructor when the extracted body has a non-empty content type, while preserving the fast_has guard so an explicit Content-Type from the init still wins. ~20 lines of logic change across two Rust files plus ~60 lines of new tests.
Security risks
None identified. This is header-population timing; no new untrusted input parsing, no auth/crypto/permissions surface.
Level of scrutiny
Moderate-to-high. The diff is small and the logic is straightforward, but it sits in the Request/Response constructors — a hot, critical path that runs for every typed-body construction in the runtime. The Request-side change reshuffles a borrowck workaround (ct_ptr: *const [u8]) and adds an eager HeadersRef::create_empty() allocation where there previously was none when no headers init was supplied. I traced the error paths: in Request, the bail! → cleanup → finalize_without_deinit path drops the newly-created HeadersRef; in Response, init's field drop glue releases it on ?. Both look correct.
Other factors
- Test coverage is thorough: Request × Response × {FormData, URLSearchParams, typed Blob} × {arrayBuffer, bytes, text, blob}, plus order-independence and explicit-override assertions, with the FormData case verifying the boundary in the header matches the body bytes.
- No CODEOWNERS for these paths.
- The behavioral change (eager
FetchHeadersallocation for every typed-body Request/Response, even if.headersis never read) is a deliberate trade-off for correctness; a human familiar with the allocation/perf characteristics here should confirm that's acceptable. - The bug-hunting system found nothing.
Given this touches core constructor logic with unsafe pointer reshaping rather than a mechanical/config change, deferring to a human reviewer.
7039f32 to
510113b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/webcore/Response.rs`:
- Around line 1385-1399: The `headers_from_init` flag in `Response` construction
is being derived from `result.headers.is_some()`, which loses the distinction
between an explicit empty `headers` init and no init at all. Update the
`Response` init handling paths (including the direct `Response` clone path and
the general init path) to capture whether the caller supplied `headers` before
normalization/cloning, and assign that presence boolean to `headers_from_init`
instead of recomputing it from `result.headers`. This should preserve explicit
empty headers for downstream logic in `RequestContext` and avoid incorrect slice
auto-206 promotion.
In `@test/js/bun/http/serve.test.ts`:
- Around line 1350-1363: The slice-response tests in the fetch handlers should
also assert the `Content-Range` behavior, not just the status and body. Update
the relevant `runTest` cases around the `Response(Bun.file(fixture).slice(...))`
paths to verify that `Content-Range` is present when the response is promoted to
`206`, and absent when it should not be emitted. Use the existing `response`
assertions in these slice cases and add checks that strongly validate the
`206`/`Content-Range` split in `serve.test.ts`.
In `@test/js/web/fetch/body.test.ts`:
- Around line 746-768: The current fetch(Request) regression test in Request
content-type survives fetch(request) and matches the wire only covers the
FormData path, so broaden it to cover the other body variants mentioned in this
PR. Extend the same test or add adjacent cases for URLSearchParams and typed
Blob bodies, verifying that fetch(req) preserves the request content-type and
that the server sees the same header/body pairing. Use the existing Request,
fetch, and Bun.serve setup as the reference point so the regression is checked
across the full body matrix.
- Around line 704-711: The shared loop in the body header test is swallowing all
`formData()` errors, which hides regressions in the success path for multipart
and urlencoded bodies. Split the `json()` and `formData()` cases in the test
around the `obj[consume]()` call so `json()` can still tolerate rejection, but
`formData()` must be awaited without a catch and assert the strongest invariant
using the existing `makeBody`, `obj`, and `consume` test setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 34ef8634-3884-4176-b597-91a1e2acc32a
📒 Files selected for processing (6)
src/runtime/server/RequestContext.rssrc/runtime/webcore/Body.rssrc/runtime/webcore/Request.rssrc/runtime/webcore/Response.rstest/js/bun/http/serve.test.tstest/js/web/fetch/body.test.ts
|
Current state of this PR (head Reproduction. On an unmodified build, With the fix. CI on build #94033. 175 jobs passed; the two darwin-14 jobs are still queued. The only hard failures are two tests on the
Everything else in the build's failure list is tagged flaky by the CI tooling (passed on retry or when run alone) and is unrelated to this change. A retrigger was already used on this PR earlier, so I am not pushing another; the diff itself is ready for review. |
7ef4ec0 to
4490d18
Compare
…headers
A Request or Response built from a FormData, URLSearchParams, or typed
Blob body carries its Content-Type (for FormData, the multipart boundary)
only on the body Blob. The headers object was created lazily, and the
Blob was gone by then if the body had been consumed first, so
headers.get("content-type") returned null and the boundary was
unrecoverable. Node/undici fix the header list at construction, as the
Fetch spec requires.
Request: materialize the headers at construction, copying the body's
Content-Type in, so it survives whatever consumes the Blob later.
Response: materializing at construction would make Bun.serve treat every
typed-Blob response as if the caller had supplied headers, which it uses
to decide whether a sliced Bun.file() auto-promotes to 206. Instead,
every body-consuming BodyMixin method (text/json/arrayBuffer/bytes/blob/
formData, .body, .textStream) calls preserve_body_content_type first,
which copies the Content-Type over while the Blob is still there.
Bun.serve: the auto-206 decision now keys on a new Init.headers_from_init
flag rather than on init.headers being present, so headers materialized
by the above (or by the lazy .headers getter) no longer flip a sliced
response from 206 to 200.
4490d18 to
cf1d285
Compare
|
Superseded by #42015. It fixes the same access-order bug by capturing the body's Closing this one so there is a single PR to review. Reopen if the consumption-time shape is preferred. |
…nstead of teeing it (#42053) ### Problem - `new Response(Bun.file("a.html").stream())` answers `blob().type === "text/html;charset=utf-8"` and Bun.serve sends that Content-Type. After `.clone()` both bodies answer `""` and Bun.serve sends none. A `Bun.file()` or typed `Blob` body loses it too once `.body` was observed first. - Cause: `Value::clone_with_readable_stream` (`src/runtime/webcore/Body.rs`) tees every stream-backed body through JS, so the store behind an unread native stream (MIME type, sendfile path) is gone on both sides. The Blob arm has the mirror bug: `new Response(Bun.stdin).clone()` dupes a Blob over fd 0 and the clone reads `""`. ### Fix - `clone()` decides per store. A store that reads the same twice (memory, S3, a regular file by path) is duped. An unread native stream first moves back into its Blob through `ReadableStream::to_any_blob`, as the readers and Bun.serve already do. A store that yields its bytes once (any fd, a FIFO by path) is read as one stream and teed. JS streams and partly read bodies tee as before. - The pre-clone stream is detached, so a held reference reads as locked, as after a tee. The cached `.body` is cleared and rebuilt from the Blob (the #33779 guarantees hold). - Correct because both bodies now resolve to what the un-cloned body resolves to, so every consumer answers the clone as it answers the original. - Verified: `test/js/web/fetch/body-clone.test.ts` (13 new tests, 6 fail on stock bun) and the neighbouring body, response, request, FormData, blob, serve and fetch suites. Self-reviewed: 2 concerns raised, both addressed. ### Background - A body `Value` is a `Blob` (memory, `Bun.file()` store, S3), a string or byte buffer, or `Locked` (a `ReadableStream`). `new Response(stream)` is `Locked`, as is any body once `.body` ran. - A native stream over a Blob or an unopened file holds a ref to its `Store`. `to_any_blob` turns it back into a Blob without reading it. - The JS wrapper caches `.body` and its stream. A clone that changes what the body holds must resync both (#33779). <details><summary>Notes</summary> - Ledger context: content-type propagation matrix, member "`new Response(Bun.file(p).stream())`: type recovered for the direct return, dropped by `.clone()`; Bun.serve `/direct text/html`, `/clone null`". Expected: clone == original. - `store_reads_repeatably()` (`Blob.rs`, next to `resolve_file_stat`): Bytes and S3 are repeatable; a `File` over an fd never is (reads share the fd offset, and a pipe's bytes are gone once read: `new Response(Bun.file(fd)).clone()` read `""` on the second body even for a regular file); a `File` over a path is stat'd once if `seekable` is unknown and is repeatable unless stat says it is not a regular file. A missing path stays repeatable (both bodies fail the same way). - Probes: with an fd-only guard, `new Response(Bun.file(fifoPath).stream()).clone()` hung on the second open while stock's tee delivered the bytes to both; `new Response(Bun.stdin).clone()` gives `["hello world", ""]` on stock. Both are tests now (posix for the FIFO). - Not changed here, left for a maintainer call: `new Response(new Blob([..], { type }).stream())` puts the type into the header list at construction (undici: `null`) because `Value::from_js` moves a memory-blob stream into its Blob eagerly, while a file stream stays `Locked` until first use, so its headers stay `null`. Making file streams eager too would route `fetch(url, { body: Bun.file(p).stream() })` through the file-blob upload path (a synchronous whole-file read over https, `fetch.rs` "TODO: make this async + lazy") instead of a streamed upload, so it is not done here. - Side effect worth knowing: `Response` derives a missing `Content-Type` header lazily from a Blob body on first `.headers` access. For `new Response(file.stream())`, calling `.clone()` before the first `.headers` access therefore makes `content-type: text/html` appear on both. Header derivation is already order-dependent today (`.body` or `.text()` before `.headers` drops it, see #32913); this adds no new mechanism. - Suites run locally on the debug+ASAN build: body-clone (76/76), body (476), response, request, body-stream (9086), FormData (149), blob (110), bun-serve-static (46), regression 18547/02368/2993/25648, serve.test.ts -t "clone|type|Content|file" (84), fetch.test.ts -t clone. `request-clone-leak.test.ts` and `request-method-getter.test.ts` time out at 5 s per test on this build for the constructor-only cases too; unrelated. </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/fetch/body-clone.test.ts <!-- robobun:evidence:end -->
Problem
The Content-Type header that Bun derives from a
FormData,URLSearchParams, or typedBlobbody is only copied into the Request/Response headers lazily, on the first access of.headers. Consuming the body first replaces the internal Blob withValue::Used, after which the lazy path finds no Blob and synthesizes empty headers.For a multipart body the boundary lives only in that header, so it became unrecoverable:
Reading
.headersbefore the body returned the correct value, so the result depended on access order. This breaks the standard proxy/signing/middleware shape (consume or hash the body, then forward the headers plus bytes): the forwarded multipart body has no boundary the upstream can parse. Per Fetch, the header list is fixed at construction, so it must not depend on which of the two is read first.URLSearchParamsandBlobbodies with a type lost their Content-Type the same way,Responsehad the identical bug, andResponse.bodylost it too.fetch(request)was also affected: it extracts the body Blob directly, so afterwards the user-held Request'sheaders.get("content-type")wasnulleven though the correct boundary went out on the wire.Cause
Request::construct_intoonly copied the Blob's content type into the headers when aheadersinit was supplied; otherwise it deferred toensure_fetch_headers, which reads the content type offBodyValue::Blob(blob). Once the body is consumed that match arm no longer hits and nothing is written.Response::constructor/get_or_create_headershad the same guard.Fix
The two classes need different treatment.
Request: write the Content-Type into the headers at construction (
construct_into). When the extracted body is a Blob with a non-empty content type, the headers object is created (if it does not already exist) and the Content-Type written, unless the headers already have one. Nothing depends on a user-constructed Request's headers being unmaterialized, and fixing it at construction closes every path that takes the body Blob afterwards at once: theBodyMixinconsumers,fetch(request)(fetch.rs:1249),Bun.write, the shell,new Blob([req]), and anything added later.fetch()already skips the body's content type when the headers contain one (from_fetch_headers), so nothing is sent twice.Response: preserve the Content-Type at consumption time. Construction-time materialization regressed 25 Content-Range tests in
serve.test.tsplus 1 inbun-serve-file.test.tsin an earlier revision of this PR, becauseBun.servereadResponse.init.headersbeing unset as "the user passed no headers init" when deciding whether a slicedBun.file()response gets an automatic 206 (RequestContext::render_metadata). The last commit replaces that overloaded signal (see the second fix below), but preservation at consumption time is still the right shape for Response: it is the only approach that also covers the Responsesfetch()builds internally fordata:,file:, andblob:URLs (which never pass through the JS constructor), and it keepsnew Response(body)on theBun.servepath from allocating aFetchHeadersnothing reads. SoBodyMixingainspreserve_body_content_type, a gated default that materializes the lazy headers only when they do not exist yet and the body is aValue::Blobwith a non-empty content type; every body-consuming method (get_text,get_json,get_array_buffer,get_bytes,get_form_data,get_blob_with_this_value, andget_text_stream, which main added while this PR was open) and the.bodygetter call it right before the Blob is taken.RequestandResponseeach supply a one-linematerialize_headersthat routes to their existing lazy materializer. For a Request this hook is now a no-op (the headers are already materialized), which its doc comment states.Response external consumers that bypass
BodyMixin(Bun.write(dest, response),HTMLRewriter.transform, the shell,WebAssembly.compileStreaming,new Blob([response]), and theBun.servesend path) are intentionally not hooked: they consume the Response as an input rather than as the headers-then-body proxy shape this fixes. The internaljsFunctionGetCompleteRequestOrResponseBodyValueAsArrayBufferis also not hooked; it has no JS call site.Verification
test/js/web/fetch/body.test.ts, under"content-type survives reading the body before the headers", covers Request and Response x {FormData, URLSearchParams, typed Blob} x {arrayBuffer, bytes, text, blob, json, formData, draining.body, draining.textStream()}, asserts the header matches the headers-first order, asserts the multipart boundary in the header is the one actually used in the body bytes, and asserts an explicitcontent-typefrom the init is not overridden. A separate test,"Request content-type survives fetch(request) and matches the wire", fetches the Request to a localBun.serveand asserts the user-held Request's boundary is non-null and identical to the Content-Type the server received and to the boundary in the body bytes.A third block,
"fetch() non-remote response content-type survives body consumption", covers the threefetch()URL schemes that build a Response whose Content-Type lives only on the body Blob and so had the identical bug:data:,file:, andblob:URLs each hadheaders.get("content-type")returnnullif the body was read first. TheBodyMixinhook fixes them; these tests pin that down.Against unmodified
mainplus only the tests:bun bd test test/js/web/fetch/body.test.ts -t "content-type survives": 52 fail, 8 pass (the 8 are the explicit-header cases and Request's.bodypath, which already worked)bun bd test test/js/web/fetch/body.test.ts -t "via draining .textStream": 6 fail, 0 passbun bd test test/js/web/fetch/body.test.ts -t "survives fetch": 1 failWith the fix (numbers as of the rebase onto current main):
bun bd test test/js/web/fetch/body.test.ts: 512 pass, 0 failbun bd test test/js/bun/http/serve.test.ts -t "Content-Range": 42 pass, 0 failbun bd test test/js/bun/http/bun-serve-file.test.ts: 105 pass, 0 failbun bd test test/js/bun/http/fetch-file-upload.test.ts: 11 pass, 0 failRebase notes
Rebased onto current main as a single commit. Three source conflicts, all resolved in favor of keeping both sides:
RequestContext.rs: main movedsendfileinto aCell<SendfileContext>read once into a local, so the 206 condition now uses that local instead ofself.sendfile.total.Body.rs: main added stream-ownership bookkeeping (check_body_stream_ref) to the.bodygetter; thepreserve_body_content_typecall sits before it. Main also added a new consumer,get_text_stream(.textStream()), which takes the Blob the same way, so it gets the same call and a row in the test matrix (fails 6/6 without the fix).Response.rs: main narrowed theInitfields topub(crate); the newheaders_from_initfield follows suit.Also verified that
fetch()with a FormData body (both the options-object and thefetch(new Request(...))shapes) still sends a singlemultipart/form-data; boundary=...header whose boundary matches the body bytes, thatrequest.clone()and the original keep the same boundary, and that typeless bodies (plain string, Blob without a type) still correctly get no Content-Type.Second fix:
Bun.serveauto-206 for a sliced blob keyed on the wrong signalFound while landing the above.
Bun.servedecides whether a.slice()-drivenBun.file()response should auto-promote to 206 with aContent-Rangeby checking whetherResponse.init.headersexists. That signal is overloaded:init.headersis also created by the lazyheadersgetter, and (with this PR) by the Content-Type preservation above, neither of which means the caller supplied aheadersinit.The lazy-getter case is a pre-existing bug, reproducible on unmodified bun:
Response::Initgains aheaders_from_initflag, set insideInit::init(the one place a userResponseInitis parsed, sonew Response(body, init),Response.json(data, init), andResponse.redirect(url, init)are all covered, not just the constructor).Bun.servekeys the auto-206 decision on it instead of oninit.headersbeing present.Init::inithas three paths (a Request as the init, a Response as the init, and a plain object). Each computesheaders_from_initasresult.headers.is_some()for itself. The Response-as-init path clones the donor'sInitfor the other fields but must not inherit the donor'sheaders_from_init: the donor may have materialized its own headers lazily, butnew Response(slice, donor)is still a headers-carrying init from this caller's point of view. An earlier revision did inherit it viaInit::clone, which would have regressed that shape from 200 to 206;Init::cloneis otherwise unchanged since its other caller isresponse.clone(), where the verbatim copy is correct.This is also what makes it safe for anything other than a user init to create
init.headersat all, and it removes the one consumer that depended on a Response's headers staying unmaterialized.New tests in
test/js/bun/http/serve.test.tsunder"should support Content-Range with Bun.file()":"206 for a slice is not defeated by reading response.headers": fails on unmodified bun (200), passes with the fix."slice with a headers init and no Content-Range stays 200": guards the preserved contract, that a caller who supplies aheadersinit without aContent-Rangeis managing the response themselves. Passes before and after."slice with a Response as the headers-carrying init stays 200": the Response-as-init shape above. Passes on unmodified bun and with the fix, and would have failed on the revision that inheritedheaders_from_initfrom the donor.With the fix,
bun bd test test/js/bun/http/serve.test.ts -t "Content-Range": 42 pass, 0 fail.no test proof · iteration 10 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/http/serve.test.ts test/js/web/fetch/body.test.ts