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

Merged
RO
3mo ago
+169-20

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.