Conversation
WalkthroughThe request-body resolution path now passes fetch headers into ChangesRequest body Blob type preservation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:45 PM PT - Jul 8th, 2026
❌ @robobun, your commit 9301f5f has 1 failures in 🧪 To try this PR locally: bunx bun-pr 32806That installs a local version of the PR into your bun-32806 --bun |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/server/RequestContext.rs`:
- Around line 3853-3859: The zero-body resolve path in on_start_buffering()
still drops request headers, so forward the same fetch headers there that are
already passed in the last-chunk branch. Update the Body::Value::resolve call in
RequestContext’s on_start_buffering logic to use headers from
request_weakref/get_fetch_headers instead of None, matching the existing resolve
flow used for the non-empty body path. Keep the change scoped to the request
blob resolution path so Content-Type survives Content-Length: 0 and header-only
requests.
🪄 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: b79e66ff-26b1-4023-bdd7-11d56a788cdd
📒 Files selected for processing (2)
src/runtime/server/RequestContext.rstest/js/bun/http/serve.test.ts
|
This change is green on every lane it affects, including the CI has been red across several runs, but on a different unrelated test each time, and never on this change's code or test:
Each run's failures clear on the next run and are replaced by a different set, which is the signature of cross-platform CI flakiness (timeouts, OOM, GC timing), not a defect in this diff. This PR only threads the request I've used my one CI re-roll. The diff is ready for review; the remaining red is pre-existing/flaky infrastructure on lanes unrelated to this change. (For context, #33128 is a broader, spec-aligned fix for the same issue; this PR is the minimal targeted alternative. Merge order is a maintainer call.) |
There was a problem hiding this comment.
LGTM — minimal, well-tested fix that threads request headers into the locked-body resolve path.
Extended reasoning...
Overview
This PR fixes #32801: request.blob() in a Bun.serve handler was dropping the request's Content-Type header and falling back to text/plain;charset=utf-8. The fix is a 5-line change in src/runtime/server/RequestContext.rs (on_buffered_body_chunk) that passes the request's fetch headers (via request_weakref.get().and_then(get_fetch_headers)) into Body::Value::resolve instead of None. A regression test in test/js/bun/http/serve.test.ts POSTs four File bodies with distinct MIME types and asserts each round-trips through (await request.blob()).type.
Security risks
None. The change only plumbs an already-available request header (Content-Type) through to the resolved Blob's type field. No new parsing, no auth/permissions, no allocation/lifetime changes — request_weakref.get() already returns Option, so a dead weakref degrades to the previous None behavior. The headers are passed as Option<NonNull<FetchHeaders>>, matching the existing resolve signature exactly.
Level of scrutiny
Low. This is a surgical bug fix that mirrors two existing patterns in the codebase: the fetch client path already passes response headers into resolve, and the synchronous get_blob_with_this_value path in Body.rs (line ~2136) already reads Content-Type from BodyMixin::get_fetch_headers for the non-locked case. The change is gated inside if matches!(old, Body::Value::Locked(_)), and resolve's Action::GetBlob arm is the only consumer of the headers argument — it just sets blob.content_type from the header value.
Other factors
The one CodeRabbit suggestion (forward headers in on_start_buffering too) was correctly rebutted (that branch's guard is the exact negation of the condition that produces a Locked body, so headers there would be dead code) and CodeRabbit withdrew it. The PR description explicitly documents this invariant. No CODEOWNERS cover the modified files. The bug-hunting system found no issues. The robobun CI comment notes failures in the build, but the change itself is sound and the test is straightforward; any CI failure is likely unrelated flakiness given the scope of this diff.
|
#33128 also fixes #32801, as a side effect of deriving blob()'s type from the header list per the fetch spec: the normalized type is computed at .blob() time, where the Request and its headers are available, and carried through |
When a Bun.serve handler reads the request body with request.blob(), the body is resolved asynchronously after the bytes arrive. That resolve path passed no headers, so the resulting Blob always fell back to text/plain;charset=utf-8 instead of the request's Content-Type. Pass the request's fetch headers into Body::Value::resolve so the Blob keeps the Content-Type, matching the fetch client path.
1d4da0c to
df0723c
Compare
There was a problem hiding this comment.
LGTM — minimal fix mirroring the fetch-client resolve path, and the nit from my earlier pass was addressed.
Extended reasoning...
Overview
This PR changes one call site in src/runtime/server/RequestContext.rs (on_buffered_body_chunk): instead of passing None for the headers argument to Body::Value::resolve, it now passes the request's fetch headers obtained via request_weakref.get().and_then(BodyMixin::get_fetch_headers). A regression test in test/js/bun/http/serve.test.ts posts File bodies with four distinct MIME types and asserts that (await request.blob()).type round-trips each one.
Security risks
None. The change threads an already-available Option<NonNull<FetchHeaders>> through an existing parameter that resolve already knows how to consume. No new parsing of untrusted input, no allocation, no auth/crypto surface. request_weakref.get() returns None if the request was collected, degrading to the prior behavior.
Level of scrutiny
Low-to-moderate. It touches the HTTP server request-body pipeline, but the change is a 6-line pattern application that exactly mirrors the established fetch-client path in FetchTasklet.rs:793 (response.get_fetch_headers() → resolve(..., headers)). BodyMixin::get_fetch_headers is a simple accessor with no side effects or exception paths. The sibling resolve call in on_start_buffering was deliberately left as None with a well-reasoned justification (that branch cannot run with a Locked body, so headers there would be dead code) — CodeRabbit raised and then withdrew a concern about it after the author explained the invariant.
Other factors
The bug-hunting system found no issues. My earlier nit (bug-history comment in the test) was addressed in 8a33b42. CI on the touched paths is green per the author's build summary. The author flagged that PR #33128 also fixes #32801 via a broader spec-aligned refactor; that's a process/merge-order question for maintainers rather than a defect in this change — this fix is correct on its own and would simply be subsumed if #33128 lands.
|
Closing in favor of #33128, which now carries this fix: it derives If the larger change turns out not to be wanted, this one can be reopened as the minimal fix for #32801. |
Fixes #32801
Repro
Every content type collapsed to the
text/plain;charset=utf-8fallback, regardless of what the client sent.Cause
When a
Bun.servehandler reads the request body withrequest.blob(), the body is in theLockedstate and is resolved asynchronously once the bytes arrive, inon_buffered_body_chunk(src/runtime/server/RequestContext.rs). That call passedNonefor the headers argument ofBody::Value::resolve, so theContent-Typeheader was never consulted and the Blob fell back totext/plain;charset=utf-8.The fetch client path already does this correctly, passing the response's fetch headers into
resolve.Fix
Pass the request object's fetch headers into
resolve(reached viarequest_weakref), mirroring the fetch client.resolvereads theContent-Typefrom them and sets it on the resolved Blob.Note: the sibling
resolvecall inon_start_bufferingkeepsNone. That branch is only reached when the body is notLocked(its guard is the exact negation of the condition that makes the bodyLocked), so it never produces a Blob and passing headers there would be dead code.Verification
test/js/bun/http/serve.test.tsPOSTs aFilewith four distinct types and asserts(await request.blob()).typeround-trips each one.text/plain;charset=utf-8.