Take the native blob path for Response-wrapped Bun.file() streams - #31674
Take the native blob path for Response-wrapped Bun.file() streams#31674alii wants to merge 22 commits into
Conversation
new Response(Bun.file(p).stream()) was always rendered through the per-chunk JS streaming loop, never the blob/sendfile path that new Response(Bun.file(p)) takes. Worse, wrapping a sliced file's stream broke outright: serving it stalled until idleTimeout and reset the connection, and .bytes()/.text() on the Response never resolved. Root cause: Body::extract stores file-source streams as Value::Locked, and check_body_stream_ref then migrates the stream from Locked.readable into the JS-side cached slot to break a GC cycle. Every later to_blob_if_possible call reads only Locked.readable, finds it empty, and silently fails - so the conversion that would route file streams onto the blob path was dead code. Fixes, all on that one root cause: - ReadableStream::to_any_blob's File arm now restores the FileReader's slice window (start_offset/max_size) onto the rebuilt Blob; it previously spanned the whole store, which would serve the entire file for new Response(file.slice(a, b).stream()). - New BodyMixin::try_blob_from_resolved_stream: retries the blob conversion with a stream resolved from either slot. Used by all five body consumers (text/json/arrayBuffer/bytes/blob). Bails out when the body already has a promise/action/on_receive_value or when the stream is locked or disturbed, so user-observable streaming semantics are unchanged. - RequestContext::do_render_with_body's Source::Blob|File render arm tries the same conversion before falling back to the JS streaming loop, so file streams reach the existing sendfile machinery (Content-Length instead of chunked encoding). Serving a 1MB Response(file.stream()) now uses the sized-body path, and Response-wrapped sliced file streams serve/resolve correctly. .bytes() on these responses returns Uint8Array as specified (the JS streaming fallback resolved it as ArrayBuffer). The remaining direct-consumption hang (file.slice(a, b).stream() .bytes() with no Response wrapper, >512KB files) is a separate FileReader bug and is not addressed here.
|
Updated 12:17 AM PT - Jun 10th, 2026
❌ @robobun, your commit 26fdee3 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31674That installs a local version of the PR into your bun-31674 --bun |
|
@robobun adopt |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Can you add a test that checks stream.cancel wroks? And that aborting the request works still?
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughConvert unlocked resolved file/blob ReadableStreams into AnyBlob (preserving slice offsets/sizes); body consumer accessors and RequestContext use blob-backed paths when possible and fall back to streaming. Update Blob EOF-clamping and add tests for full/sliced file responses, locking, cancelation, and HTMLRewriter on streamed files. ChangesFile-backed stream blob conversion
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
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/Body.rs`:
- Around line 1792-1823: The function try_blob_from_resolved_stream should not
convert/detach the original ReadableStream once the JS-visible body cache has
been populated; add a guard at the start of try_blob_from_resolved_stream to
check the JS body cache (the same condition used by body_get_cached(js_ref) /
body_get_cached) and return false if that cache exists so we keep the streaming
path intact. Specifically, in try_blob_from_resolved_stream (and before calling
stream.to_any_blob or detach_readable_stream) check the body cache via
get_body_value()/body_get_cached-equivalent and bail out when a cached JS body
is present, preserving the existing checks for
locked/promise/on_receive_value/action and stream.is_locked.
In `@test/js/bun/http/serve.test.ts`:
- Around line 2405-2412: The helper makeStreamFile uses tmpdirSync(); replace
that with the test harness tempDir helper: import tempDir from 'harness' (or add
it to the existing harness import) and call tempDir() to create the temporary
directory inside makeStreamFile, then use join(tempDir(),
"serve-file-stream.bin") as the path; update any references to tmpdirSync()
within makeStreamFile to use tempDir and keep the rest of the function (writing
bytes, returning { path, bytes }) unchanged.
In `@test/js/web/fetch/body.test.ts`:
- Around line 745-751: The helper function makeFile uses require("fs") inline;
replace this dynamic require with a module-scope import and call to that
imported binding: add a top-level import (e.g., import * as fs from "fs" or
const fs = require("fs") at module scope) and then update makeFile to call
fs.writeFileSync(path, bytes) instead of require("fs").writeFileSync(...); keep
the same function name makeFile, SIZE usage, and filename
"body-file-stream.bin".
- Around line 753-762: Replace inline dynamic requires with a module-scoped
import: add an import for tempDirWithFiles from "harness" at the top of the test
module, then remove the per-test uses of const { tempDirWithFiles } =
require("harness"); inside each test (these occur in the tests that call
tempDirWithFiles in the body test suite). Update all five occurrences so tests
instead call tempDirWithFiles directly; no other logic should change (tests like
the one using makeFile, file(path).stream(), and Response(...).bytes() remain
the same).
🪄 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: 48e123fa-2526-4e73-82fd-ca8bee4f1654
📒 Files selected for processing (5)
src/runtime/server/RequestContext.rssrc/runtime/webcore/Body.rssrc/runtime/webcore/ReadableStream.rstest/js/bun/http/serve.test.tstest/js/web/fetch/body.test.ts
|
✅ Ready for review / merge. Head 26fdee3 has all requested work in:
CI (build 61608, final): 284 jobs passed, including every lane that exercises this diff (debian x64-asan, windows, darwin x64, darwin 26 aarch64, linux aarch64). Two red jobs, both unrelated to the diff:
Both need a Buildkite retry from someone with write access. All review threads resolved. |
…e harness helpers - pin that response.body.cancel() still works and marks the body used - pin that a previously exposed .body ends up released and drained after .bytes(), matching the existing Response(Bun.file(path)) behavior - race request aborts against the native file-stream response and verify the server keeps serving complete bodies afterwards - verify a client cancel mid-transfer doesn't break the server - replace tmpdirSync/inline require() with module-scope harness imports
…d streams - ReadableStream::to_any_blob (File arm): force_detach the JS stream after converting. The FileReader keeps its lazy store (the converted blob only clones it), so a captured stream reference could otherwise be wrapped into a new Response and re-read the file from disk. Detaching leaves the stream in the exact state the JS streaming path leaves consumed file streams in: disturbed and locked, with re-wrapping throwing "ReadableStream has already been used". - ValueBufferer::buffer_locked_body_value: blob/file-source streams that check_body_stream_ref migrated into the JS-side cache reached the "toBlobIfPossible should've caught this" unreachable and panicked, e.g. new HTMLRewriter().transform(new Response(Bun.file(p).stream())). Convert them and re-dispatch through the Blob arm instead. - tests: pin the re-wrap rejection and the exposed-.body end state, and cover HTMLRewriter transforming a file-stream Response
… behavior get_form_data must not take try_blob_from_resolved_stream: its parser reads blob.slice() synchronously and a converted file-backed blob has no in-memory bytes yet, so converting would break slices the streaming path serves fine today. Pin that formData() on a file stream keeps parsing correctly.
The four consumers that kept it can't be affected by it: with the stream migrated to the JS-side cache the call is a documented no-op, and with Locked.readable still populated try_blob_from_resolved_stream performs the same conversion on the stream resolved from either slot. Matches get_text.
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 `@test/js/workerd/html-rewriter.test.js`:
- Around line 168-179: Replace the manual tmpdirSync() usage with the harness
tempDir automatic-cleanup pattern: import tempDir from 'harness' and create the
temp directory via const dir = using(tempDir()), then build the file path using
join(dir.path, "html-rewriter-stream.txt.js") and write the file there; keep the
rest of the test (HTMLRewriter, response stream, and assertions) unchanged so
cleanup happens automatically via Symbol.dispose on dir.
🪄 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: 54f70555-89cf-4bac-b943-819db5ff774a
📒 Files selected for processing (5)
src/runtime/webcore/Body.rssrc/runtime/webcore/ReadableStream.rstest/js/bun/http/serve.test.tstest/js/web/fetch/body.test.tstest/js/workerd/html-rewriter.test.js
do_render_head_response had the same dead to_blob_if_possible shape the GET render path had, so after the GET fix a HEAD request to the same handler still replied Transfer-Encoding: chunked. Resolve and convert blob/file-backed streams there too so HEAD reports the same Content-Length as GET. Also switch the html-rewriter test to tempDir.
resolve_size()'s File arm unconditionally replaced the blob's size with store_size - offset, so a slice with a concrete size was widened to the end of the file. GET was unaffected (do_sendfile saves the original size before the stat), but the HEAD render path reports blob.size after resolve_size, so HEAD on new Response(file.slice(a,b)) — and, since the HEAD-parity change, file.slice(a,b).stream() — emitted the store-wide length instead of the slice's. Apply the same only-resolve-unknown/clamp guard #31210 added to the Bytes arm, in both resolve_size and resolved_size, and extend the HEAD parity test with sliced stream and sliced blob cases.
The native ReadableStream::isLocked tests $reader.isTrue(), which never matches the reader objects the stream builtins actually store, so the blob/file fast paths could convert a stream out from under a reader created with new ReadableStreamDefaultReader(body) — which, unlike getReader(), doesn't run the deferred $start thunk that marks lazy native streams disturbed. Add ReadableStream__hasReader (the $isReadableStreamLocked truthiness test) and gate the conversions on it, so consumption and serving fall back to the streaming paths' locked rejection.
to_any_blob's File arm detached the stream after conversion, but the Blob and Bytes arms only called done(), whose detach_if_possible tail is intentionally empty. A captured reference to a converted blob or fetch body stream could therefore be wrapped into a new Response and consumed again, silently yielding an empty body where the JS streaming path - and released versions - throw "ReadableStream has already been used". Hoist the post-success bookkeeping out of the arms: any conversion that produces a blob now leaves the stream done and force-detached. The two regression tests throw on released versions and on this fix; the pre-fix branch accepted the re-wrap and returned "".
ee0309c leaves every natively converted stream force-detached, so calling .blob() on a captured stream after the body was consumed now rejects with ERR_INVALID_STATE (ReadableStream is locked) instead of ERR_BODY_ALREADY_USED. The property the test pins is unchanged: the call returns a rejected promise rather than crashing or throwing synchronously.
|
@robobun fix review comments |
The two remaining consumers of the dead to_blob_if_possible path: - get_form_data now converts migrated blob/file-backed streams and, for file-backed (and S3) blobs, stamps the body's content type onto the blob and takes Blob's async read-then-parse path instead of synchronously parsing the empty in-memory view. This also fixes the pre-existing silent-empty FormData from Response(Bun.file(p)) bodies (the parse previously saw zero bytes and resolved with no entries; multipart errored with 'missing final boundary', missing files resolved empty instead of rejecting). - spawn stdio's Request/Response arms convert before extract_body_value so file-backed body streams take the blob path; previously stdin: new Response(Bun.file(p).stream()) hung forever because the streaming stdin consumer never received the lazy file stream's data.
|
Done in 8da2c60. Every review thread was already addressed in code except the two findings I had deferred with in-thread verification; both are now fixed:
7 new tests (5 formData in body.test.ts, 2 stdin in spawn.test.ts); the fix-provers hang or resolve-empty on the unfixed build. Replies with details are on the two original threads. |
…eam-sendfile # Conflicts: # src/runtime/webcore/Body.rs
Main's #31884 fixed ReadableStream::isLocked to implement the full isReadableStreamLocked semantics, so the pre-existing is_locked guards ahead of the ValueBufferer and do_render_with_body conversion arms now reject reader-held streams on their own, and the reader-only ReadableStream__hasReader duplicate is no longer needed. try_blob_from_resolved_stream uses is_locked directly. Serving a reader-held stream now fails before headers with ERR_STREAM_CANNOT_PIPE instead of erroring mid-stream after a 200; the two subprocess tests pin the new shape.
The read path hands every *_with_bytes consumer a leaked Box<[u8]> with Lifetime::Temporary that the consumer must reclaim, but to_form_data_with_bytes ignored its lifetime parameter (as the Zig original did), leaking the file contents on every file-backed Blob::to_form_data. LeakSanitizer flagged it once get_form_data started routing Response(file).formData() through this path. Free it with the TemporaryBytes guard like the sibling arms, covering the invalid-encoding early return too, and switch to_form_data's synchronous call site to Lifetime::Share since it passes a store-backed view it does not own.
What does this PR do?
new Response(Bun.file(p).stream())always rendered through the per-chunk JS streaming loop, never the blob/sendfile path thatnew Response(Bun.file(p))takes. Worse, wrapping a sliced file's stream broke outright on released Bun:Root cause:
Body::extractstores file-source streams asValue::Locked, andcheck_body_stream_refthen migrates the stream fromLocked.readableinto the JS-side cached slot to break a GC cycle. Every laterto_blob_if_possiblecall reads onlyLocked.readable, finds it empty, and silently fails — the conversion that would route file streams onto the blob path was dead code.Changes (all on that one root cause)
ReadableStream::to_any_blob(File arm): restore the FileReader's slice window (start_offset/max_size) onto the rebuilt Blob. It previously spanned the whole store, which would have served the entire file fornew Response(file.slice(a,b).stream())the moment the conversion became reachable.Body.rs: newBodyMixin::try_blob_from_resolved_stream— retries the blob conversion with a stream resolved from either slot. Wired into all five body consumers (text/json/arrayBuffer/bytes/blob). Bails out when the body already has a promise/action/on_receive_value, or when the stream is locked or disturbed, so user-observable streaming semantics are unchanged.RequestContext::do_render_with_body: theSource::Blob | Source::Filerender arm tries the same conversion before falling back to the JS streaming loop, so file streams reach the existing sendfile machinery. The pre-existing locked-stream check still runs first.ReadableStream::to_any_blob(File arm):force_detachthe JS stream after a successful conversion. The FileReader keeps its lazy store (the converted blob only clones it), so a captured stream reference could otherwise be wrapped into a new Response and re-read the file from disk. Detaching leaves the stream in the exact state the JS streaming path leaves consumed file streams in: disturbed and locked,new Response(stream)throws "ReadableStream has already been used". (Blob/Bytes sources keep their existing post-conversion semantics —readable-stream-blob-consumed.test.tspins those.)ValueBufferer::buffer_locked_body_value: blob/file-source streams thatcheck_body_stream_refmigrated into the JS-side cache reached the// toBlobIfPossible should've caught this→unreachable!()and panicked the process —new HTMLRewriter().on(...).transform(new Response(Bun.file(p).stream()))crashes released Bun. Convert them there and re-dispatch through the existing Blob arm (buffered bytes / async file read).RequestContext::do_render_head_response: the HEAD render path had the same deadto_blob_if_possibleshape, which would have made HEAD replyTransfer-Encoding: chunkedwhile GET repliesContent-Lengthfor the same handler. Run the same conversion there so HEAD and GET report identical headers.Blob::resolve_size/resolved_size(File arm): a concrete slice size was clobbered withstore_size - offset, widening the view to the end of the file — HEAD onnew Response(Bun.file(p).slice(a,b))(pre-existing) and.slice(a,b).stream()reported the store-wide length while GET served the slice. Apply the same only-resolve-unknown/clamp guard blob: stop resolve_size() widening a sliced Blob to the end of its store #31210 added to the Bytes arm.new ReadableStreamDefaultReader(body)(which, unlikegetReader(), doesn't run the deferred$startthunk that marks lazy native streams disturbed) could have the stream converted and consumed/served out from under it, because the nativeReadableStream::isLockednever matched real readers. This PR originally shipped a scopedReadableStream__hasReaderfor its new conversion sites; after webcore: fix ReadableStream::isLocked never matching any stream #31884 fixedisLockedglobally on main, that duplicate was dropped andtry_blob_from_resolved_streamusesis_lockeddirectly, with the pre-existingis_lockedchecks ahead of theValueBuffereranddo_render_with_bodyarms now live and rejecting reader-held streams before the arms run.get_form_data+ spawn stdio (the last two consumers of the dead path):get_form_datanow runs the conversion and, for file-backed (and S3) blobs, stamps the body's content type onto the blob and takesBlob::to_form_data's async read-then-parse path instead of synchronously parsing the empty in-memory view — also fixing pre-existingResponse(Bun.file(p)).formData()(urlencoded silently resolved empty, multipart rejected "missing final boundary", missing files resolved empty instead of rejecting). Spawn stdio'sRequest/Responsearms convert beforeextract_body_value, sostdin: new Response(Bun.file(p).stream())(previously hung forever) delivers the file. Routing formData through the read path surfaced a pre-existing leak under LeakSanitizer:to_form_data_with_bytesignored its lifetime parameter (as the Zig original did) and never reclaimed theTemporaryread buffer, unlike every sibling*_with_bytesarm; it now frees it (and the synchronous call site passesSharefor its store-backed view, which it never owned).Observable effects
Bun.serve+Response(file.stream())Transfer-Encoding: chunked, per-chunk JS loopContent-Length, native blob/sendfile pathBun.serve+Response(file.slice(a,b).stream())Response(file.stream()).bytes()ArrayBufferUint8Array(as specified)Response(file.slice(a,b).stream()).bytes()/.text()HTMLRewriter().transform(Response(file.stream()))unreachable!())Response(file.stream())handlerTransfer-Encoding: chunkedContent-Lengthas GETBun.file()/ sliced stream handlerContent-Length(blob) / chunked (stream)Content-Length, same as GETnew ReadableStreamDefaultReader(body)attachedERR_STREAM_CANNOT_PIPE(500), per the now-working locked check from #31884Response(Bun.file(p)).formData()spawn({ stdin: new Response(file.stream()) })Related issues, verified against this build:
streamon slicedBunfiledoesn't work #18192 (directBun.readableStreamToText(file.slice(a,b).stream())hang, no Response involved) is a separate FileReader bug and is not fixed here — the repro still hangs on this branch.Response.clone().bytes()returns incorrectArrayBufferinstead ofUint8Array#30797 (Response.clone().bytes()returningArrayBuffer) is the clone/tee variant of the type bug. Cloned bodies go through the JSreadableStreamToBytesmulti-chunk path, whoseconcatArrayBuffersarg-order bug is fixed separately in webstreams: return ArrayBuffer from single-string-chunk arrayBuffer() consumer #30810 — the clone repro still returnsArrayBufferon this branch, so neither issue should be auto-closed by this PR.Tests
test/js/web/fetch/body.test.ts: 15 new tests — Uint8Array type + full contents, sliced.bytes()exact-window content (position-dependent file data), sliced.text()/.arrayBuffer(), disturbed-stream construction throw pin, locked-reader rejection pin,response.body.cancel()pin (bodyUsedflips, consumption rejects with "Body already used", same as released Bun), an exposed-.bodypin (after.bytes()the previously returned stream is consumed and unusable —locked === true, reads throw "ReadableStream is locked", re-wrap throws — byte-for-byte the streaming path's end state), a re-wrap pin (a consumed file stream can't be wrapped into a new Response and re-read), raw-constructor-reader rejection pins for file- and blob-backed bodies (locked, not disturbed — the stream must not be stolen), and 5.formData()tests (file-stream parse, direct file-backed body parse, multipart direct+stream, missing-file rejection, and a no-steal pin for a failed encoding check).test/js/bun/http/serve.test.ts: 7 new tests — Content-Length + body equality for streamed file, sliced stream serves exactly the slice, HEAD/GET Content-Length parity, cannot-pipe error-semantics pins for reader-held streams (subprocess;getReader()and raw-constructor variants; rejected before headers with a 500 since webcore: fix ReadableStream::isLocked never matching any stream #31884), 100 aborted-mid-transfer requests (mirroring the existing sendfile abort test) followed by a full-body fetch, and a clientreader.cancel()mid-transfer followed by a full-body fetch.test/js/workerd/html-rewriter.test.js:(from file stream) supports element handlers— crashes released Bun with theunreachable!()panic, passes with the fix — and a reader-held variant that errorsStream already usedinstead of panicking/stealing.test/js/bun/spawn/spawn.test.ts: 2 new tests —Response/Request-wrapped file streams as stdin (both hang by timeout on the unfixed build).USE_SYSTEM_BUN=1: the fix-proving tests fail on released Bun (wrong type ×2, chunked encoding, timeout hangs for sliced consumption and wrapped-stream stdin, the HTMLRewriter panic, and the silent-empty/multipart-error formData cases); the behavior pins (cancel, abort, re-wrap rejection, locked/disturbed semantics, failed-encoding no-steal) pass on both.serve.test.ts,serve-static.test.ts,body*.test.ts(all five),response.test.ts,streams.test.js— 9,745 pass / 0 fail.