fix(webcore): close reader when sliced Bun.file stream reaches max_size - #27213
fix(webcore): close reader when sliced Bun.file stream reaches max_size#27213robobun wants to merge 2 commits into
Conversation
|
Updated 3:01 AM PT - May 12th, 2026
❌ @autofix-ci[bot], your commit 42907fc has 3 failures in
🧪 To try this PR locally: bunx bun-pr 27213That installs a local version of the PR into your bun-27213 --bun |
FileReader.onReadChunk() returned false once total_readed >= max_size without closing the underlying BufferedReader, leaving reader.isDone() == false. For regular files (non-pollable), the next onPull() would then park on a pending promise that nothing ever resolves, hanging the stream whenever the backing file exceeds 640 KiB (256K pipeReadBuffer + 256K + 128K flush cutoff). - Set close = true on both the early-return path and when the current chunk reaches max_size, so the deferred reader.close() actually runs. - Fold close into was_done so the async/pending path (Windows, pipes) delivers the final chunk as *_and_done and returns false to stop the low-level read loop instead of reading from a closed fd. - Return false at the end of onReadChunk when close is set, so the synchronous POSIX read loop does not issue another pread against the now-closed fd. Fixes #18192
df6a1da to
5710239
Compare
|
Rebased onto current
|
There was a problem hiding this comment.
I didn't find any bugs, but this activates the previously-dead defer reader.close() path inside onReadChunk (re-entrantly firing onReaderDone while still in the read loop / onPull), and that interaction plays out differently across the read_inside_on_pull, pending-promise, and fall-through paths on POSIX vs Windows — worth a human pass on the close/done ordering, and CI is still showing build-zig failures on the reported commit.
Extended reasoning...
Overview
This PR fixes #18192: Bun.file(path).slice(start, end).stream() hangs forever when the underlying file is larger than ~640 KiB. The change is confined to FileReader.onReadChunk() in src/runtime/webcore/FileReader.zig, plus a new regression test at test/regression/issue/18192.test.ts.
The fix sets close = true when total_readed >= max_size (both on the early-return and on the chunk that satisfies the slice), folds close into was_done for the pending-promise delivery path, and adds if (close) return false before the fall-through return so the low-level read loop stops. The previous if (buf.len == 0) check that was supposed to do this was provably dead code (it sat inside if (buf.len > 0) after a @min of two >= 1 operands).
Security risks
None. This is read-side stream termination logic for local file blobs; no parsing, auth, or untrusted input handling is involved. The change strictly reduces I/O (stops reading once the slice is satisfied instead of reading and discarding the rest of the file).
Level of scrutiny
High. This is core runtime I/O code on a hot path (Bun.file().stream()), and the function being modified is a state machine with three distinct delivery modes (read_inside_on_pull, pending.state == .pending, and the buffered fall-through) that behave differently on POSIX (synchronous pread loop) vs Windows (async libuv). Critically, the defer if (close) this.reader.close() mechanism existed before but was never actually triggered — the only prior site that set close = true was the dead buf.len == 0 check. So this PR is the first time that deferred close actually fires, and on POSIX reader.close() → closeHandle() → done() → finish() + vtable.onReaderDone() runs re-entrantly before onReadChunk's caller resumes. That means FileReader.onReaderDone() (which can call parent().onClose()) executes while onPull is still on the stack with read_inside_on_pull = .js, and then onPull proceeds to return .into_array_and_done. The author's local testing covers this and the related blob/fd-read suites pass, but the close/done ordering across all three paths deserves a human reviewer's eyes.
Other factors
- The PR was rebased after the file moved from
src/bun.js/webcore/tosrc/runtime/webcore/, and the robobun CI comment still reportsbuild-zig/build-cppfailures across ~16 platforms for commit5710239. These are likely the pre-rebase state (patch targeting a moved file) and an autofix commit42907fclanded afterward, but green CI should be confirmed before merge. - The
was_done = reader.isDone() or closechange correctly covers the pending-promise path (Windows async file reads), but theread_inside_on_pullpath still relies onreader.isDone()becoming true synchronously via the deferredclose()— which it does on POSIX perPipeReader.zig:111/245-263, but is worth a second look. - Regression tests are solid: they exercise the 640 KiB boundary and several slice ranges, and run in subprocesses so a hang surfaces as a clean timeout rather than wedging the runner.
|
Superseded by #31680. Since the Rust port, |
What
Bun.file(path).slice(start, end).stream()hangs forever when the underlying file is larger than 640 KiB.Fixes #18192
Why
FileReader.onReadChunk()tracksmax_sizefor sliced file blobs. Whentotal_readed >= max_sizeit returnedfalseto stop the low-level read loop, but never closed the underlyingBufferedReader. That leavesreader.isDone() == false, so the nextonPull()returns.pending— and since regular files are not pollable, nothing ever resolves that pending promise.The separate
if (buf.len == 0)check meant to close the reader was dead code: at that pointbufhas been truncated to@min(max_size - total_readed, buf.len)where both operands are>= 1.The 640 KiB threshold comes from
PipeReader's 256 KiBpipeReadBufferand its 128 KiB flush cutoff: the secondonPullstarts reading at offset 512 KiB, and if more than 128 KiB remain before EOF,onReadChunkis called with.progress(hitting the broken early return) instead of reaching the EOF path that would mark the reader done.How
In
FileReader.onReadChunk():close = trueon thetotal_readed >= max_sizeearly return so the deferredreader.close()actually runs.buf.len == 0check withtotal_readed >= max_sizeso the chunk that satisfies the slice is delivered as the final one (close = true; hasMore = false).closeintowas_doneso the async/pending path (Windows file reads, pipes) delivers*_and_doneand returnsfalseinstead of continuing to read from a closed fd.falseat the end of the function whencloseis set, so the synchronous POSIX read loop does not issue anotherpreadagainst the now-closed fd.As a side effect the reader now stops after the first chunk that satisfies
max_sizeinstead of reading (and discarding) the rest of the file.Verification
bun bd test test/regression/issue/18192.test.ts→ 6 passUSE_SYSTEM_BUN=1 bun test test/regression/issue/18192.test.ts→ 4 fail (hang on every file > 640 KiB)bun bd test test/js/web/fetch/blob.test.ts test/js/bun/util/bun-file-fd-read.test.ts→ all passbun run zig:check-all→ all targets compile