Bun.file().slice(): enforce the slice window on unknown-size sources (chardev over-read + stream hang) - #31680
Bun.file().slice(): enforce the slice window on unknown-size sources (chardev over-read + stream hang)#31680robobun wants to merge 3 commits into
Conversation
WalkthroughThis PR fixes sliced file stream completion on large files by stopping streaming reads after callback-driven completion and by treating exhausted slice windows as finished. It also adds a regression test for large sliced streams and reformats two documentation pages. ChangesSliced stream completion fix
Documentation formatting
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
|
Updated 11:05 PM PT - Jul 25th, 2026
❌ @robobun, your commit 6a5e98a has 2 failures in
🧪 To try this PR locally: bunx bun-pr 31680That installs a local version of the PR into your bun-31680 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Re #25796: same underlying bug (#18192), but that PR patches |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find correctness issues, but this touches the core posix drain loops in PipeReader.rs (shared by all pipe/file/socket reads, including the HUP-drain path) and the Windows reader-buffer handling needed a follow-up commit — worth a human pass, and please confirm the Windows CI failure on streams.test.js is cleared by 26ac746.
Extended reasoning...
Overview
The PR fixes a hang in Bun.file(path).slice(start, end).stream() on files larger than the per-loop read buffer by treating max_size exhaustion as the stream's EOF. It changes three files: src/runtime/webcore/FileReader.rs (the on_read_chunk max_size window, was_done propagation, and the final ret computation), src/io/PipeReader.rs (adds parent.is_done() checks after on_read_chunk callbacks in three drain-loop sites so a callback that closes the reader stops further reads on the now-stale fd), and a regression test in streams.test.js.
Security risks
None apparent. No auth, crypto, or untrusted-input parsing is involved; the change adjusts internal stream-termination control flow. The buffer().clear() before close() on the already-exhausted path prevents over-delivery of bytes past the slice window on Windows, which is a correctness (not security) concern.
Level of scrutiny
Medium-high. PipeReader.rs::read_with_fn and read_blocking_pipe are the shared posix read loops used by every BufferedReader parent (subprocess stdout/stderr, shell pipes, FIFOs, sockets, file streams), and the new parent.is_done() early-returns sit in the HUP-drain path where the existing logic is already subtle (the !keep_reading && !received_hup asymmetry is documented as load-bearing). The FileReader.rs change also threads close through was_done in four result-construction branches and into the trailing ret, which interacts with the pending-read state machine and the Windows path (where chunks land in the reader's own buffer rather than the stack buffer — the second commit 26ac746 was specifically to handle that). These are not mechanical edits.
Other factors
- CI: robobun's last status update reports
streams.test.jsfailing on all three Windows targets for commit 8a31275; commit 26ac746 (the reader-buffer clear) was pushed afterward to address it but I don't see an updated CI result for it yet. - The PR description and verification section are thorough and the added test covers the small-window, multi-chunk-spanning, and empty-window cases.
- No human reviewer has looked at this yet; given the blast radius of the
PipeReader.rsdrain-loop changes and the platform-specific follow-up, a human sign-off is appropriate.
|
Confirmed: the |
There was a problem hiding this comment.
I didn't find any bugs, but this touches the shared POSIX drain loops in PipeReader.rs (used by subprocess/shell/file readers) and the FileReader close/done lifecycle across both platforms, so it's worth a human look at the fd-staleness and is_done() ordering changes.
Extended reasoning...
Overview
This PR fixes a hang where Bun.file(path).slice(start, end).stream() never signals completion on files larger than ~640 KiB. It modifies three files: src/runtime/webcore/FileReader.rs (treat reaching max_size as the stream's EOF — set close/was_done, clear the reader buffer, and call reader().close() on the already-exhausted early return), src/io/PipeReader.rs (in three POSIX drain-loop sites, check parent.is_done() after the on_read_chunk callback so a callback that closes the reader mid-drain stops the loop instead of reading a stale fd), and a new regression test in streams.test.js.
Security risks
None apparent. This is internal stream-completion / fd-lifecycle logic with no auth, crypto, parsing of untrusted input, or permission surface. The reader().buffer().clear() before close() actually tightens behavior (prevents past-window bytes from being delivered).
Level of scrutiny
High. PosixBufferedReader::read_with_fn and read_blocking_pipe are the shared drain loops for all POSIX buffered readers — subprocess stdout/stderr, shell pipes, FIFOs, sockets, and file streams — not just sliced Bun.file. The new parent.is_done() checks change loop-exit conditions on the HUP-drain path for every consumer of BufferedReaderVTable. The FileReader changes also alter when results are tagged *AndDone vs not, and re-enter reader().close() from inside on_read_chunk (which is itself invoked from inside a &mut BufferedReader borrow per the file's own aliasing notes). The PR author has clearly thought about this (extensive comments, cross-platform Windows buffer-clear handling, broad test-suite verification), but the interaction surface is subtle enough that a maintainer familiar with the reader lifecycle should confirm the ordering is sound.
Other factors
- No prior
claude[bot]reviews on this PR. - Bug-hunting system found nothing.
- CI reported green on the latest commit per the author's follow-up (70/70 checks on build #59737, including the Windows lanes that initially failed before 26ac746).
- Good regression test coverage for the specific bug (small window, multi-chunk window, empty window).
- The diff is well-commented and the description is thorough, which lowers risk — but does not make the change mechanical.
|
CI state for maintainers: every lane that actually ran is green across two consecutive builds (#59737, #59803) — Linux (glibc/musl, x64/aarch64, baseline), all Windows lanes, and the darwin build steps. The only red is |
|
CI state for the current head (#81677, sha 6a5e98a — comment-only trims + merge of main), now settled:
The diff is 4 files ( |
There was a problem hiding this comment.
I didn't find any bugs and CI is green, but this changes shared drain-loop control flow in PosixBufferedReader (used by subprocess/shell/socket/FIFO readers, not just sliced files) and adds a close-from-inside-callback re-entrancy path — worth a human look at the is_done() early-returns and the Windows buffer().clear() before close().
Extended reasoning...
Overview
The PR fixes a hang in Bun.file(path).slice(start, end).stream() where the stream delivers the slice's bytes but never signals done on files larger than the per-loop read buffer. It touches three files: src/runtime/webcore/FileReader.rs (treat reaching max_size as EOF in on_read_chunk, propagate close into was_done for all pending-result variants, and close the reader on the already-exhausted early return), src/io/PipeReader.rs (three new parent.is_done() checks after on_read_chunk in the streaming drain loops so a callback that closes the reader stops the loop instead of reading a stale fd), and a new regression test in streams.test.js.
Security risks
None identified. This is internal stream-termination/control-flow logic; no parsing, auth, crypto, or user-controlled-path handling is introduced. The change strictly narrows behavior (stops reading earlier when the window is satisfied) rather than exposing new surface.
Level of scrutiny
Moderate-to-high. The FileReader.rs half is scoped to the max_size (sliced-file) path and is straightforward to reason about. The PipeReader.rs half, however, modifies the streaming drain loops in read_with_fn and read_blocking_pipe — code shared by every BufferedReader consumer (subprocess stdout/stderr, shell pipes, sockets, FIFOs), with extensive existing commentary about noalias laundering and re-entrancy hazards. The new pattern — on_read_chunk calling self.reader().close() from inside the reader's own drain loop, then the loop detecting that via is_done() and bailing — is a re-entrancy interaction that someone familiar with this layer's invariants should confirm, particularly the HUP-drain paths where the previous logic deliberately kept reading even when the callback returned false.
Other factors
The PR description is thorough, the new test exercises three window shapes (in-first-chunk, spanning-chunks, empty), and CI is green across Linux/Windows/macOS (the only red lanes are unrelated macOS agent-provisioning failures). The author also ran the subprocess/shell/spawn streaming suites. The Windows-specific reader().buffer().clear() before close() in the early-return path has a plausible rationale comment but is the kind of platform-asymmetric detail that benefits from a maintainer's eye. No CODEOWNERS apply to these paths.
|
Rebased onto current await Bun.write(p, new Uint8Array(1024 * 1024));
for await (const c of Bun.file(p).slice(100, 164).stream()) console.log(c.length); // 64, then never exitsRe-ran the proof against the merged head:
On the previous build (#59919): every red lane was macOS agent or container infra (squid did not start, This is ready for a maintainer. The fresh CI run from this push should confirm. |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Re the out-of-scope warning and the Mintlify preview: the two docs files in the diff are not hand edits from this change. autofix.ci pushed b3e52f1 after the branch was updated with main, reformatting |
b3e52f1 to
54e09a6
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
robobun
changed the title
When the underlying file has no stat size (a character device like /dev/zero or /dev/urandom, or a regular file larger than the streaming read buffer) the same sliced Blob disagreed between consumers: - .arrayBuffer()/.bytes()/.text() returned more than the slice, rounded up to the internal read-chunk quantum: slice(0, 1_000_000) returned 1_048_576 bytes. The POSIX do_read_loop never advanced read_off, so remaining_buffer capped each read at max_length instead of max_length - bytes_read_so_far; the loop broke at >= max_length without truncating. Track progress via buffer.len() and truncate at the break. - .stream() delivered exactly end - start bytes and then never closed: on_read_chunk truncated the chunk to the window but left close / has_more unset, so the final chunk was delivered as non-final and the already-at-max early return stopped the drain loop without closing the reader or resolving the pending read. Nothing re-arms a read on a regular file and /dev/zero never drains, so the consumer pends forever. Treat reaching max_size as the stream's EOF: deliver the chunk as done, close the reader, and stop the drain loop when the callback finishes the reader. Fixes #31675 Fixes #18192
54e09a6 to
c0ddfaa
Compare
There was a problem hiding this comment.
I didn't find bugs, but this touches the shared PosixBufferedReader drain loops (used by shell, subprocess stdio, and file streams) plus re-entrant reader().close() inside on_read_chunk, so it's worth a maintainer look.
What was reviewed:
read_file.rs:read_off = buffer.len()+ truncate — checked thatread_offhas no other POSIX-side reader andmax_length as usizematches the existing>=guard.FileReader.rs: tracedclose_if_needed!()→reader().close()→on_reader_done()refcount flow; the JS-finalizer ref keeps theSourcebox live so the caller's post-callbackparent.is_done()inPipeReader.rsis not a UAF.PipeReader.rs: checked the un-touchedon_read_chunksites (the>128_000flush and theread_blocking_pipecapacity>0 branch) — both already return on!keep_going, and the newret = !close && ...makes that fire when the window is exhausted.
Extended reasoning...
Overview
Fixes two independent bugs in Bun.file(path).slice(start, end) on sources whose stat size is unknown: (1) the buffered POSIX do_read_loop never advanced read_off, so remaining_buffer() capped each read at max_length instead of max_length - bytes_read and over-read to the next chunk quantum; (2) FileReader::on_read_chunk truncated the window-satisfying chunk but never marked it final, so the stream delivered the right bytes and then hung. Touches src/runtime/webcore/blob/read_file.rs (2 lines), src/runtime/webcore/FileReader.rs (~10 lines across the max_size guard, was_done computation, and the fallthrough ret), and src/io/PipeReader.rs (3 is_done() short-circuits after on_read_chunk callbacks in the POSIX drain loops). New tests cover /dev/zero, /dev/urandom, and a 1 MiB regular file across .arrayBuffer()/.bytes()/.text()/.stream()/getReader().
Security risks
None identified. The change tightens an over-read (delivers fewer bytes, exactly what the slice requested) and closes a hang. No new user-controlled input reaches size arithmetic; max_size/max_length were already the bounds — this just enforces them.
Level of scrutiny
High. PosixBufferedReader::read_with_fn / read_blocking_pipe are the shared drain loops for shell pipes, subprocess stdio, and file streams; a mis-step here hangs or double-closes across all of them. FileReader::on_read_chunk now calls self.reader().close() from inside a vtable callback while the caller holds &mut BufferedReader — the exact re-entrancy shape the R-2 laundering comments in PipeReader.rs warn about. I traced the refcount flow (waiting_for_on_reader_done + the #32921 pin around p.run()/close_if_needed!() + the JS-finalizer ref) and believe the Source box stays live through the post-callback parent.is_done() read, but this is subtle enough that a maintainer who owns the BufferedReaderParent aliasing contract should confirm.
Other factors
- The PR author already ran the shared-consumer suites (
shell-blocking-pipe,spawn-streaming-stdout/stdin,body-stream) against the debug build and reports green; CI across Linux/Windows/macOS is green on multiple builds. - The comment-cop bot flagged verbose comments, which were trimmed in 6a5e98a (all threads resolved).
- The
read_blocking_pipecapacity>0 streaming branch and the>128_000flush inread_with_fnwere not given explicitis_done()guards, but both already return on!keep_going, andon_read_chunknow returnsfalse(ret = !close && ...) when it closes — so those sites stop correctly without the extra check. - I did not find a path where the early-return
total_readed >= max_sizeblock reaches!is_done()on POSIX (the prior chunk'sclose_if_needed!()should have closed it), but it's reachable on Windows where the read continuation is callback-driven, and the guard is harmless either way.
2 participants
What
Bun.file(path).slice(start, end)does not enforce the slice window when the underlying source's stat size is unknown, and the same slice's consumers disagree with each other:Observed sizes from the buffered consumers:
slice(0, 500_000)returns 524_288,slice(0, 999_999)returns 1_048_576,slice(0, 3_000_000)returns 4_194_304. So.size(and by extension Content-Length framing) understates the delivered bytes, and "give me N random bytes from/dev/urandom" hands back N rounded up.Fixes #31675
Fixes #18192
Why
Two independent paths:
Buffered read over-read (
src/runtime/webcore/blob/read_file.rs): the POSIXdo_read_looppassesself.read_offtoremaining_buffer()as "bytes read so far", but never increments it (WindowsReadFileUVdoes). So each read is capped atmax_lengthrather thanmax_length - bytes_read_so_far; theVeccapacity doubles, the 64 KB stack read fills the spare, and the loop breaks atbuffer.len() >= max_lengthwithout truncating. For regular files this does not bite because the initial allocation isstat.st_size + 16and the first read is exact; forcould_blocksources the initial allocation is 4 KB and the loop keeps reading.Stream hang (
src/runtime/webcore/FileReader.rs):on_read_chunkcaps the stream atmax_size, but the chunk that satisfies the window was truncated and delivered as a non-final chunk (has_more/closestay unset because theif buf.is_empty()branch is dead code after thetotal_readed >= max_sizeguard). The next chunk then hittotal_readed >= max_sizeand returnedfalse, which stops theread_with_fndrain loop without callingdone()or closing the reader. Regular files are not pollable so nothing re-arms a read, and/dev/zerojust keeps producing on the next pull; the pending read promise leaks and the consumer hangs.How
read_file.rs: keepread_offin sync withbuffer.len()soremaining_buffer()computes the actual remaining window, and truncate tomax_lengthat the>=break.FileReader.rs: treat reachingmax_sizeas this stream's EOF. Setclose = true/has_more = falsewhen the window is satisfied, foldcloseintowas_doneso the pending path yields*AndDone, returnfalsefrom the fallthroughret, and close the reader on the already-exhausted early return.PipeReader.rs: stop the posix drain loops whenon_read_chunkfinished the reader; the capturedfdis stale after the callback closes it.Verification
New tests in
test/js/bun/util/bun-file.test.tscover the/dev/zeroand/dev/urandomslice windows across.arrayBuffer()/.bytes()/.text()/.stream()/getReader().read(), plus a 1 MiB regular file (window inside the first chunk, spanning multiple chunks, and empty). Onmainthe buffered-read cases fail with the rounded-up byte counts and every.stream()case times out; with the fix they pass in about 6 seconds.Supersedes #27213 (same logic applied to the pre-port
FileReader.zig).[review] gate passed · iteration 13 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 13
evidence per changed file