Keep Bun.file().slice() bounds when an unread file stream is turned back into a Blob - #39241
Keep Bun.file().slice() bounds when an unread file stream is turned back into a Blob#39241robobun wants to merge 1 commit into
Conversation
…ned back into a Blob to_any_blob() rebuilt the Blob from the FileReader's store alone, so a consumer taking that shortcut (Bun.serve bodies, routes, Response and Request readers other than text()) got the whole file for Bun.file(path).slice(a, b).stream(). Copy the reader's start_offset and max_size, which from_blob_copy_ref took from the source Blob, back onto the rebuilt Blob.
|
Warning Review limit reached
Next review available in: 30 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the fix is the exact inverse of from_blob_copy_ref and mirrors what the Source::Blob arm already does.
What was reviewed:
- Traced
from_blob_copy_ref→FileReader.start_offset/max_size→to_any_bloband confirmed the round-trip is symmetric;init_with_storedefaults (offset 0, size = store.size()) are preserved when the fields areNone. - Checked the
usize as SizeTypecast matches the existing pattern at Blob.rs:2408/4184 and is lossless (values originated asSizeType as usize). - Tests cover Request and Response, bounded/open-ended/past-EOF/empty/nested slices, the body-getter path, and both static routes and handlers with GET/HEAD.
Extended reasoning...
Overview
The PR fixes a bug where Bun.file(path).slice(a, b).stream(), when converted straight back into a Blob by ReadableStream::to_any_blob (the fast path used by Bun.serve response rendering, static routes, and the Body mixin readers other than text()), lost its slice window and delivered the whole file. The 9-line change in src/runtime/webcore/ReadableStream.rs copies the reader's start_offset / max_size back onto the rebuilt Blob's offset / size — the exact inverse of what from_blob_copy_ref did when the stream was created, and the same thing the sibling Source::Blob arm does via ByteBlobLoader::to_any_blob. Two test files gain coverage.
Security risks
None. This narrows what a consumer sees from a sliced file stream to the window the caller asked for; it never widens access. No auth, path handling, or untrusted-input parsing is touched.
Level of scrutiny
Low-to-medium. The source change is 9 lines that set two Cell fields on a freshly created Blob, using values that originated from the same Blob type on the other side of a round-trip. I verified: (a) FileReader.start_offset / max_size are Option<usize> set only in from_blob_copy_ref from blob.offset / blob.size; (b) init_with_store sets offset = 0 and size = store.size() (= MAX_SIZE for a file store), so leaving them alone when the option is None reproduces the pre-PR behavior for unsliced files; (c) the usize as SizeType cast is the same pattern used throughout Blob.rs and is lossless because the value was cast the other way first. No refcount, lifetime, or allocation changes.
Other factors
The tests are thorough for a fix of this size: both Request and Response, five slice shapes plus a whole-file control for bytes(), plus arrayBuffer() / blob() / json() and the body-getter path in body.test.ts; and static-route + handler + GET/HEAD across four slice shapes in bun-serve-file.test.ts. The PR description documents that all new tests fail on the unpatched build and on 1.4.0. The known unfixed sibling (Bun.spawn({ stdin }), #31931) is a pre-existing limitation in a different layer and is correctly called out as out of scope.
|
Status
|
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
On the #31674 overlap: that PR does contain this same nine-line arm change, but as one piece of a larger rework (making the conversion reachable from more Response paths, detaching the stream after conversion, HEAD parity, |
|
Updated 4:45 PM PT - Aug 15th, 2026
❌ @robobun, your commit c6dbc05 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 39241That installs a local version of the PR into your bun-39241 --bun |
1 participant
Problem
Bun.file(path).slice(3, 8).stream()delivers the whole file wherever the stream is converted straight back into a Blob instead of being read: aBun.servehandler returningnew Response(sliceStream)(body andContent-Lengthare the whole file, HEAD too),routes: { "/x": new Response(sliceStream) }, andarrayBuffer()/bytes()/blob()/json()/formData()on aResponseorRequestbuilt from the stream (blob()resolves to a Blob whosesizeis the file's). Readingnew Response(Bun.file(p).slice(3, 8)).bodyand thenbytes()hits it as well, because the body getter turns the slice into one of these streams. Reproduces on the released 1.4.0.text(),for await,Bun.readableStreamTo*()pump the stream and do not take this path. (The pump has its own end-of-window bug from Streams: one PipeReader loop with owned chunks, hold-not-adopt buffer pins, right-sized native pulls #38886, fixed separately in FileReader: apply the slice window on the read_into pull path and end the stream when it is used up #39201; the two do not overlap.)Source::Filearm ofReadableStream::to_any_blob(src/runtime/webcore/ReadableStream.rs:227) builds the result withBlob::init_with_store(store), which is offset 0 and the whole store. The window is not on the store:from_blob_copy_ref(ReadableStream.rs:529) moved it from the source Blob onto theFileReaderasstart_offset/max_size, and the arm never reads those. TheSource::Blobarm copies its window back (ByteBlobLoader::to_any_blob, ByteBlobLoader.rs:157), which is whynew Blob([..]).slice().stream()is unaffected.Fix
Filearm setsoffset/sizeon the rebuilt Blob from the reader'sstart_offset/max_size, the inverse offrom_blob_copy_refand the same thing theBlobarm does.[start_offset, start_offset + max_size), so the Blob handed to the consumer has to describe that range. The rebuilt Blob now has the sameoffset/sizeas the Blob.stream()was called on, so every consumer behaves exactly as it does when given that Blob directly (new Response(Bun.file(p).slice(3, 8))): sendfile offset and length,Content-Length,Blob::do_read_file. An unslicedBun.file(p).stream()hasstart_offset == Some(0)andmax_size == Noneand comes out as before.Bun.spawn({ stdin: sliceStream })is not fixed by this: it now receives the windowed Blob, but spawn'sextract_blob(src/runtime/api/bun/spawn/stdio.rs:597) redirects any file-backed Blob by path or fd and ignores the window, for a directly passedBun.file().slice()too. That is spawn: respect Blob offset/size when a sliced Bun.file is used as stdin #31931. Take the native blob path for Response-wrapped Bun.file() streams #31674 carries this same arm change inside a larger rework of the Response/file-stream paths.test/js/web/fetch/body.test.ts,blob-backed stream bodies > made from a sliced Bun.file()(run for bothRequestandResponse):bytes()over a bounded, open-ended, past-EOF, empty and nested slice plus a whole-file control,arrayBuffer()andblob()(size and bytes),json(), and the body-getter case.test/js/bun/http/bun-serve-file.test.ts,File slicing: a route built from the stream, and a handler returning it, checked with GET and HEAD for a bounded, open-ended and empty slice plus a whole-file control. All 10 fail on a debug build without thesrc/change (whole file, orFailed to parse JSON) and on 1.4.0, and pass with it.Background
Blobis a view (offset,size) onto a refcountedBlob.Store;slice()shares the store and only changes the view. A file-backed store holds the path and a stat cache, and reports its size asMAX_SIZE("unknown"), so a Blob built from the store alone means "the whole file".blob.stream()on a file-backed Blob (from_blob_copy_ref) creates aFileReadersource that holds the store plus the view asstart_offset/max_size; nothing is opened until the stream is first read.to_any_blobis a shortcut used by consumers that can work from a Blob (Bun.serve response rendering and routes, the Body readers other thantext(),WebAssembly.compileStreaming): if a native stream has not been read yet, they take the Blob it was made from instead of pumping it.Probe (1.4.0 vs this build)
26-byte file
abcdefghijklmnopqrstuvwxyz, every line usesBun.file(p).slice(3, 8).stream():WebAssembly.compileStreamingon a Response built from such a stream re-streams the converted Blob: on 1.4.0 it fails at byte 0 (the file's leading bytes), with this change it starts at the slice; the end of the window on that re-streamed read is #39201's.