Bun.file().slice(): resolve the file size before applying the W3C relative clamp - #33601
Bun.file().slice(): resolve the file size before applying the W3C relative clamp#33601robobun wants to merge 8 commits into
Conversation
…e-end clamp Negative start/end (and positive overshoot) on a lazy Bun.file() blob were clamped against the MAX_SIZE sentinel instead of the actual file size, so slice(1, -1) on a 10-byte file reported .size == 2^52-ish and read through to EOF. Resolve the size first, same as the .size getter does.
|
Warning Review limit reached
Next review available in: 4 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 (4)
WalkthroughModifies ChangesBlob slice fix
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 6:15 AM PT - Jul 7th, 2026
❌ @robobun, your commit a1ce5dc has 4 failures in
🧪 To try this PR locally: bunx bun-pr 33601That installs a local version of the PR into your bun-33601 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #32794 or #33360. Those fix Bun.file(p).slice(1, -1).size // 4503599627370493, should be 8Neither of those PRs touches |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
It discarded the File store (so a nonexistent-file slice resolved to empty instead of ENOENT) and the contentType argument. The clamp already yields (0, 0) for an empty source and get_slice_from preserves both.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/web/fetch/blob.test.ts`:
- Around line 111-153: The file-writing blob slice tests are running
sequentially even though each case uses its own tempDir and isolated file I/O.
Update the parameterized slice coverage in blob.test.ts to use
test.concurrent.each for the cases table, and make the standalone ENOENT test
concurrent as well so these filesystem-heavy tests can run in parallel without
shared state. Keep the existing assertions and the tempDir/Bun.file/p-based
setup unchanged, just switch the test definitions to the concurrent variants.
🪄 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: 1a91ecb5-6388-41a9-9227-5ac41f42c7ea
📒 Files selected for processing (2)
src/runtime/webcore/Blob.rstest/js/web/fetch/blob.test.ts
on_read_chunk returned false without closing the reader or clearing its buffer when total_readed >= max_size. On Windows the async uv read loop kept going and the populated _buffer was handed out at EOF via the empty-buf / on_reader_done path, so a size-0 file slice streamed the whole file from offset to EOF. Mark done, clear the buffer, and close instead; also hoist the cap check outside the non-empty-buf guard so the Windows EOF-with-empty-buf call is covered.
…d one The test asserted this range yields an empty body, which only held while Bun.file().slice() clamped negative start against the MAX_SIZE sentinel. With the clamp fixed it correctly addresses the last half of the file, matching ArrayBuffer.prototype.slice. Move the case out of badRanges and assert the bytes match full.buffer.slice(start, Infinity).
On macOS fstat on a pipe reports the currently-buffered byte count in st_size, so resolve_size() picked that up as the blob's size and the slice clamp then capped against a transient value. A pipe has no meaningful size for the W3C clamp; restore the sentinel after resolving so Bun.stdin.slice(n) keeps its prior behaviour.
|
The diff is green: The remaining red is one |
… the stream when it is used up (#39201) ### Problem - `new Response(Bun.file(path).slice(0, 5)).body` and `Bun.file(path).slice(0, 5).stream()` deliver the whole file from the slice offset: 100 bytes for a 100-byte file, 1 MiB for a 1 MiB file. A zero-length slice streams to EOF. `test/js/web/fetch/blob.test.ts` "streams only the slice" fails on main (`Expected: 5, Received: 100`). Regressed in #38886. - Cause: the slice window (`FileReader.max_size` / `total_readed`) is only applied in `FileReader::on_read_chunk` (`src/runtime/webcore/FileReader.rs:637`). #38886 made `FileReader::on_pull` read straight into the pull buffer with `IOReader::read_into` (`FileReader.rs:832`), which does not go through `on_read_chunk`, and on POSIX that is the path every pull of a regular file takes. - Pre-existing, same mechanism: when the window was used up, `on_read_chunk` returned `false` without closing the reader, so a slice of a file that continues past it never finished on the paths that still go through `on_read_chunk` (native sinks such as HTMLRewriter, pollable fds, Windows), and before #38886 on every path (#18192, #31675). ### Fix - Both delivery paths share one window (`window_remaining` / `consume_window`): `on_pull` cuts the `read_into` destination to what is left of the window and charges what was read; `on_read_chunk` truncates the chunk to it, as before. - Whichever path uses the window up closes the reader (`end_at_window`), after the bytes have been handed over. This is the same sequence as a real EOF on that path (the final chunk, then `on_reader_done`), so sinks, parked reads and the JS adapter end the stream the way they already do at EOF. A zero-length window closes on the first pull without a read (`read_into` reads nothing into an empty destination). - Correct because the window is the blob's contract: `ReadableStream::from_blob_copy_ref` sets `start_offset`/`max_size` from the slice's offset and size, and `.text()` / `.arrayBuffer()` on the same slice already return exactly that window. The reader's offset was still honored (`pread` from `start_offset`); only the end of the window was lost. - Fixes #18192 and #31675 as a consequence: the window end now ends the stream instead of leaving the reader open. - Verified: `bun bd test test/js/web/fetch/blob.test.ts test/js/bun/util/bun-stdin-slice.test.ts` (108 pass). The new `blob.test.ts` cases under "a slice of a file that continues past it" cover `.stream()` + `for await`, `.stream().bytes()`, `Response(...).body` (all `read_into` pulls on POSIX, `on_read_chunk` on Windows) and `HTMLRewriter.transform(new Response(slice))` (native sink, `on_read_chunk`), each with a window inside the first read, of exactly the first pull buffer, spanning several pulls, ending at EOF, running past EOF, and empty, plus an unsliced file whose resolved size gives it a window ending at EOF. The new `bun-stdin-slice.test.ts` cases stream `Bun.stdin.slice(0, N)` over a pipe that is never closed, in one write and in two, which is the parked-read branch of `on_read_chunk`. Against a build with main's `FileReader.rs`, 20 of the 29 `blob.test.ts` cases fail (wrong byte counts, or a timeout where the stream never ends; the nine that pass are the EOF-bounded windows and the resolved-size guard) and both stdin cases time out. - Also run with the fix: `test/js/web/streams/streams.test.js`, `test/js/bun/util/bun-file*.test.ts`, `bun-stdin-slice.test.ts`, `test/js/workerd/html-rewriter.test.js`, the `spawn` stdio stream tests, `child_process.test.ts`, `process-stdin.test.ts`, `fetch-file-upload.test.ts`, `bun-serve-file.test.ts`; manual checks of `Bun.stdin.stream()` / `process.stdin` over a pipe and a file redirect, a FIFO slice whose writer stays open, and `/dev/zero` / `/dev/urandom` slices (now deliver exactly the slice; previously unbounded on main, hung before #38886). `cargo check -p bun_runtime` for the Windows and macOS targets. - Not a replacement for #31680 or #33601: both predate #38886 and carry a patch of the old `on_read_chunk` block for the window-end hang, which this PR makes unnecessary, but the `read_into` path this PR is about did not exist when they were written. Their main changes (#31680: buffered reads of character devices in `read_file.rs`; #33601: negative `slice()` indices in `Blob::get_slice`) are independent of this. ### Background - `FileReader` is the native source behind a file-backed `ReadableStream` (`Bun.file().stream()`, `new Response(file).body`, and the stream HTMLRewriter or fetch wire up for a file body). A file Blob carries an `offset` and a `size`; for a slice these describe the window, and `from_blob_copy_ref` copies them into the reader as `start_offset` and `max_size`. - It gets bytes two ways. A JS pull (`on_pull`) may read synchronously straight into the pull buffer via `BufferedReader::read_into`. Everything else comes from the `BufferedReader` read loop, which delivers through `on_read_chunk`: native sinks (`pull_into_sink`), pollable fds whose poll fired, and all reads on Windows, where reads complete through libuv. - The stream only ends when the reader reports done: `reader().close()` runs `on_reader_done`, which ends an attached sink or settles a parked read and tells the JS adapter to close; `on_pull` returns `Done` once `reader().is_done()`. A reader that is merely no longer being read from leaves the stream open forever, which is what the old window-exhausted `return false` did.
|
Heads-up for rebasing: #39201 (merged as eec9c8b) rewrote the FileReader.rs window block this PR also patches, and closing the reader at the end of the window is handled there now, so the FileReader.rs hunk here can be dropped. The Blob::get_slice change (resolving the file size before the relative clamp) is unaffected and still needed. |
2 participants
Repro
slice(start, -n)is the idiom for "everything but the trailing N bytes". On a file-backed blob Bun reads through to EOF, returning exactly the bytes the caller excluded, and.sizereports theMAX_SIZEsentinel..text(),.bytes(),.arrayBuffer()and.stream()all over-read. Negativestartis wrong in the other direction (slice(-3)reads from byte 0), andslice(0, 100)on a 10-byte file reports.size == 100.Cause
Blob::get_sliceapplies the W3C relative-start/end clamp againstself.size. For a lazyBun.file()the size is still theMAX_SIZEsentinel (the file has not been statted), soend < 0 ? max(size + end, 0)becomes a 2^52-scale number instead offileSize + end. The in-memoryBlobpath is correct because its size is always known.Fix
Resolve the file size at the top of
get_slice(same call.sizealready makes) when the store is file-backed and the size is still the sentinel, so the clamp and the resulting.sizematch the in-memoryBlob.slicepath. S3-backed blobs are left unchanged.Verification
The existing
Bun.file().slicetest had anif (!is_file)guard with a comment noting "file will lazy read until EOF if the size is wrong"; that guard is removed.Related: #32794 fixes adjacent
resolve_sizeoverwrites on an already-sliced file blob; this PR fixes slice creation on a not-yet-statted one.