FileReader: apply the slice window on the read_into pull path and end the stream when it is used up

[robobun]

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 Streams: one PipeReader loop with owned chunks, hold-not-adopt buffer pins, right-sized native pulls #38886.
• Cause: the slice window (FileReader.max_size / total_readed) is only applied in FileReader::on_read_chunk (src/runtime/webcore/FileReader.rs:637). Streams: one PipeReader loop with owned chunks, hold-not-adopt buffer pins, right-sized native pulls #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 Streams: one PipeReader loop with owned chunks, hold-not-adopt buffer pins, right-sized native pulls #38886 on every path (stream on sliced Bunfile doesn't work #18192, file.slice(a, b).stream() buffered consumption never resolves for ~1MB+ files #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 stream on sliced Bunfile doesn't work #18192 and file.slice(a, b).stream() buffered consumption never resolves for ~1MB+ files #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 Streams: one PipeReader loop with owned chunks, hold-not-adopt buffer pins, right-sized native pulls #38886). cargo check -p bun_runtime for the Windows and macOS targets.
• Not a replacement for Bun.file().slice(): enforce the slice window on unknown-size sources (chardev over-read + stream hang) #31680 or Bun.file().slice(): resolve the file size before applying the W3C relative clamp #33601: both predate Streams: one PipeReader loop with owned chunks, hold-not-adopt buffer pins, right-sized native pulls #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 (Bun.file().slice(): enforce the slice window on unknown-size sources (chardev over-read + stream hang) #31680: buffered reads of character devices in read_file.rs; Bun.file().slice(): resolve the file size before applying the W3C relative clamp #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.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 37 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.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 64eeb632-4bcc-4728-bc47-48f58b30bc32

📥 Commits

Reviewing files that changed from the base of the PR and between 2f941ed and 86c6488.

📒 Files selected for processing (3)

• src/runtime/webcore/FileReader.rs
• test/js/bun/util/bun-stdin-slice.test.ts
• test/js/web/fetch/blob.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status: approved, CI green apart from a pre-existing main failure; ready to merge.

Reproduced on a debug build of main (88a6398): new Response(Bun.file(p).slice(0, 5)).body streamed all 100 bytes of a 100-byte file, slice(5, 5).stream() streamed 95, and Bun.file(p).slice(0, 5).stream() on a 1 MiB file streamed the whole file. Every pull went through the read_into path added in #38886, which skips the window clamp in on_read_chunk. With this branch all of those deliver exactly the slice, and the slice-of-a-larger-file cases that used to never finish through the on_read_chunk path (HTMLRewriter over a sliced file, #18192, #31675) now complete.

Fail-before / pass-after against a build with main's FileReader.rs: 20 of the 29 blob.test.ts window cases fail and both new bun-stdin-slice.test.ts cases time out; all 108 tests across the two files pass with the fix.

CI (build 98774 at a2c2a37): the only red test is test/bake/deinitialization.test.ts on Windows 2019 x64, which also fails on main and is unrelated to this change; the rest of the listed failures passed on retry.

[github-actions]

This PR may be a duplicate of:

1. Bun.file().slice(): enforce the slice window on unknown-size sources (chardev over-read + stream hang) #31680 - Changes the same FileReader::on_read_chunk slice-window block to treat reaching max_size as EOF and close the reader, and claims the same Fixes #18192 / Fixes #31675.
2. Bun.file().slice(): resolve the file size before applying the W3C relative clamp #33601 - Its src/runtime/webcore/FileReader.rs hunk edits the same on_read_chunk max_size block for the same purpose: closing the reader once the window is used up instead of leaving the stream open.

🤖 Generated with Claude Code

[robobun]

Neither is the same fix.

• Bun.file().slice(): enforce the slice window on unknown-size sources (chardev over-read + stream hang) #31680 and Bun.file().slice(): resolve the file size before applying the W3C relative clamp #33601 both predate Streams: one PipeReader loop with owned chunks, hold-not-adopt buffer pins, right-sized native pulls #38886. Their FileReader.rs hunks patch the old on_read_chunk window block so that reaching max_size closes the reader (the stream on sliced Bunfile doesn't work #18192 hang); the code they patch was rewritten by Streams: one PipeReader loop with owned chunks, hold-not-adopt buffer pins, right-sized native pulls #38886, and this PR covers that case through the shared window helpers, so those hunks become unnecessary once this lands. Their main changes are elsewhere and still stand on their own: Bun.file().slice(): enforce the slice window on unknown-size sources (chardev over-read + stream hang) #31680 fixes the buffered read of character devices in read_file.rs, Bun.file().slice(): resolve the file size before applying the W3C relative clamp #33601 resolves the file size in Blob::get_slice so negative indices work.
• The bug this PR is for is the read_into pull path added in Streams: one PipeReader loop with owned chunks, hold-not-adopt buffer pins, right-sized native pulls #38886 (on_pull reading straight into the pull buffer), which skips on_read_chunk and therefore the window entirely, so a slice streams the whole file from its offset. That path did not exist when either of those PRs was written, and neither touches it.

[claude]

I reviewed this PR and didn't find any bugs. Because it changes control flow in FileReader::on_read_chunk/on_pull — re-entrancy-sensitive code on the critical file-streaming path, with different execution paths on POSIX vs Windows — a human look would still be worthwhile.

What was reviewed:

• Window accounting: on_read_chunk truncates to remaining before consume_window(chunk.len()), and on_pull caps len at remaining with amount_read <= len, so the debug_assert!(total_readed <= max_size) holds on both paths.
• Empty-destination read_into on POSIX returns (0, Progress) early (PipeReader.rs:864), so a zero-length slice closes on the first pull without issuing a read; on Windows read_into returns (0, Progress) and all bytes arrive via on_read_chunk, so consume_window(0) in on_pull is a no-op there unless max_size == 0.
• end_at_window is called after the chunk is delivered (sink write / pending resolve / IntoArrayAndDone return), matching the EOF ordering; it's guarded by is_done() so a window that ends exactly at real EOF doesn't double-close.
• read_into never dispatches on_read_chunk (only done()/error on EOF), so the window can't be double-charged between the on_pull cap and a re-entrant chunk delivery.

Extended reasoning...

Overview

The PR fixes a regression from #38886: FileReader::on_pull now reads via IOReader::read_into directly into the pull buffer, bypassing on_read_chunk, which was the only place the blob slice window (max_size/total_readed) was enforced. As a result, Bun.file(path).slice(a, b).stream() streamed the whole file from offset a. The fix extracts the window bookkeeping into three helpers (window_remaining, consume_window, end_at_window) and applies them on both delivery paths. It also fixes a pre-existing hang (#18192, #31675): when the window is exhausted, the reader is now closed so the stream ends instead of staying open forever. Tests add 21 cases across four consumers (.stream(), .stream().bytes(), Response(...).body, HTMLRewriter.transform) × five window shapes.

Security risks

None identified. The change tightens bounds enforcement (delivering fewer bytes than before, per the slice contract) rather than loosening it. No new user-controlled input reaches size arithmetic; max_size and start_offset are set at construction from the blob's own slice bounds.

Level of scrutiny

High. FileReader backs every file-based ReadableStream (Bun.file().stream(), new Response(file).body, HTMLRewriter file bodies, fetch file uploads). The changed methods are re-entrancy-sensitive (on_read_chunk runs inside the BufferedReader read loop; read_into may dispatch on_reader_done which runs user JS) and behave differently per platform (POSIX regular files hit read_into; Windows and pollable fds go through on_read_chunk). The old on_read_chunk early-returned when total_readed >= max_size; the new code instead truncates to zero and flows through the sink/pending delivery before closing — a control-flow change whose interaction with sink backpressure and parked reads deserves a maintainer's eye.

Other factors

The fix is well-argued and thoroughly tested (author verified 12/16 new cases fail on unfixed main, and ran the wider stream/stdin/spawn/HTMLRewriter suites). I traced read_into in src/io/PipeReader.rs to confirm the empty-buffer and re-entrancy claims hold. The drain() paths in on_pull do not charge the window, but that is pre-existing and those bytes come from buffered (already charged by on_read_chunk) or the reader's internal buffer on paths that don't use read_into. Nothing here looks wrong to me, but the combination of critical path + re-entrancy + cross-platform divergence + control-flow reshaping in on_read_chunk puts this outside what I'd approve without human review.

[robobun]

For whoever reviews this, the two interactions worth checking are the ones where the window-ending chunk goes through on_read_chunk. In both, the window end takes exactly the steps a real EOF already takes on that path; on POSIX the read loop closes the fd before delivering the final chunk and calls done() after it, here end_at_window plays the part of that done().

• Sink with backpressure on the final chunk: write_chunk_to_sink writes it as TemporaryAndDone, the sink answers Backpressure, so sink_paused is set and the reader paused. end_at_window then runs on_reader_done, which sees sink_paused and leaves the sink alone. When the sink drains, pull_into_sink finds reader().is_done(), nothing buffered, and calls sink.end(None). That is the existing EOF-under-backpressure sequence (on_read_chunk(.., Eof) followed by the loop's done()). Without backpressure the chunk is written, the sink ended, and on_reader_done finds no sink. Covered by the HTMLRewriter cases.
• Parked JS read: resolve_pending_read settles it with the truncated chunk first; end_at_window then runs on_reader_done, which has no pending read left and queues the adapter's close, so the chunk is enqueued before the close. Checked by hand with a FIFO whose writer sends 20 bytes after the read has parked, window of 10: one chunk of 10 bytes, then the stream ends while the writer is still open. If the settle makes JS pull again before end_at_window runs (this happens), that pull finds a 0-byte window, closes the reader itself and returns Done, and end_at_window is a no-op.
• Chunk arriving when the window is already empty (zero-length slice, or a read the close could not stop): truncated to nothing, delivered as the end on whichever path is active (a parked read settles with Done, a sink is ended without a write), then closed. on_reader_done cannot hand out the over-read bytes because since Streams: one PipeReader loop with owned chunks, hold-not-adopt buffer pins, right-sized native pulls #38886 the reader's buffer is taken out before on_read_chunk is dispatched, so consume_reader_buffer finds it empty.

[robobun]

Data point from main-break triage: this test is now failing on unrelated PRs' CI too (build 98665, debian 13 x64-asan: blob.test.ts "streams only the slice", Expected: 5, Received: 100). It is deterministic, not intermittent: a debug build of main at 8f8695f fails it every run and eabb96d (before #38886) passes, so builds that still pass it are just based on a main from before #38886. No second PR is being opened for it.

[robobun]

^{Updated 4:06 PM PT - Aug 15th, 2026}

🔄 @robobun, the build for your commit a2c2a378 (Build #98774) was cancelled — waiting for the next build...

[alii]

Fix looks right and this unbreaks main, so let's land it quickly. Two things on the tests below, both small. The bigger question of whether the window should live in the buffered reader itself (next to _offset, so FileReader and FileResponseStream stop hand-rolling it) is a follow-up, not for this PR.

[robobun]

Pushed the test additions (b10085b): the two extra windows in blob.test.ts and a pair of Bun.stdin.slice(0, N).stream() cases over a pipe that stays open in bun-stdin-slice.test.ts, both of which take the parked-read branch of on_read_chunk and time out without the fix. a2c2a37 only shortens three doc comments. Replies with the details are on the threads.

Agreed on the follow-up: with _offset already in the reader, a length next to it would let read_into and the read loop stop on their own and retire both this window and the one in FileResponseStream. Left out of this PR as discussed.

[alii]

Both asks addressed; the pipe tests hit the parked-read branch as intended.

[robobun]

Thanks. Threads resolved; nothing further from my side. a2c2a37 only touches doc comments, so its CI run should match the earlier one at c8bd2ad (green apart from retried flakes on unrelated lanes).

[claude]

I reviewed this PR and the automated bug-hunting pass found no issues. Since it changes re-entrancy-sensitive native streaming code (reader().close() firing on_reader_done mid-pull / mid-chunk-delivery) and a human reviewer is already engaged, a human sign-off is still worthwhile.

What was reviewed:

• window_remaining/consume_window invariant (total_readed <= max_size) holds on both paths — on_pull caps the read destination before charging, on_read_chunk truncates before charging.
• end_at_window ordering: called after the chunk is delivered on both paths, and the is_done() guard makes it a no-op when EOF or a re-entrant pull already closed the reader.
• Refcount balance across end_at_window → on_reader_done on the on_pull path (regular file, waiting_for_on_reader_done unset) and the on_read_chunk path (read-loop pin held) — no net decrement that could free self before the caller returns.
• The old on_read_chunk window block's close = true arm was dead (len was always > 0 there), confirming the pre-existing hang the new code fixes.

Extended reasoning...

Overview

This PR fixes a regression from #38886 where Bun.file(path).slice(start, end).stream() delivered the whole file from start instead of stopping at end. The fix adds three small helpers (window_remaining, consume_window, end_at_window) to FileReader and applies them on both delivery paths: the read_into pull path (new in #38886, previously unclamped) and the on_read_chunk path (previously clamped but never closed the reader when the window ran out — #18192/#31675). Tests cover four consumers (.stream(), .stream().bytes(), Response(...).body, HTMLRewriter.transform) × seven window shapes, plus two stdin-over-pipe cases for the parked-read branch.

Security risks

None. The change narrows what a file-backed stream delivers to match the slice contract; it does not touch input parsing, auth, or crypto. max_size and total_readed are usize and total_readed is only advanced by amounts already bounded to max_size - total_readed, so window_remaining's subtraction cannot underflow.

Level of scrutiny

High. FileReader.rs is core native streaming code with GC-rooting, refcount pins, and re-entrancy across reader().close() → on_reader_done() → user JS. The PR body and follow-up comment walk through the sink-backpressure and parked-read orderings in detail, and the tests exercise them, but this is exactly the class of change where a subtle sequencing mistake becomes a UAF or a hang on one platform. A human reviewer (alii) already left two rounds of feedback (both addressed and resolved) but has not yet approved.

Other factors

• alii's requested test additions (parked-read stdin cases, window-past-EOF, window-exactly-one-pull-buffer) are present in the diff and the threads are marked resolved.
• The comment-cop bot's three long-comment flags were addressed in a2c2a37 (doc comments shortened to one line each).
• CI on the head commit is still in progress; the one failure so far (test/bake/deinitialization.test.ts segfault on Windows 2019 x64) is in an unrelated subsystem.
• I traced the old on_read_chunk window block: the if len == 0 { close = true } arm was unreachable (both max_size - total_readed > 0 and chunk.len() > 0 were guaranteed at that point), so the old code never closed on window exhaustion — matching the PR's description of the pre-existing hang.

[robobun]

CI for a2c2a37 (build 98774): 177 of 179 jobs passed with the last two finishing up. The one test still red is test/bake/deinitialization.test.ts, a segfault at teardown of the dev server fixture on Windows 2019 x64 that is also failing on main and does not involve this code; everything else listed on the build passed on retry. Should be good to merge.