Repository navigation
Conversation
When the backing file of a Bun.file response shrinks while the body is being sent (log rotation, truncate+rewrite deploy), sendfile(2) returns 0 with bytes still owed. Previously this was folded into the normal completion arm, leaving the keep-alive socket open with the response marked done. The client then waits forever for bytes that will never arrive (with idleTimeout: 0), and the next request on that socket is served inside the first response's declared Content-Length window: an HTTP/1.1 framing desync (RFC 9112 6.2). Force-close the connection on early EOF so the client observes the short body immediately, matching Node.js. Applied to both the sendfile backend (Linux) and the buffered-reader backend (SSL / macOS / Windows / small files), which share the same shape.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 34 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 (2)
Comment |
|
Updated 7:11 PM PT - Jul 14th, 2026
❌ @autofix-ci[bot], your commit 0a89fd9 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34185That installs a local version of the PR into your bun-34185 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline coverage nit, I also checked whether the reader-path force_close() dropping the final chunk (which the old resp.end(chunk, …) would have written) is a problem — it isn't: the body is already short of Content-Length, so the client must treat it as truncated regardless of whether that last partial chunk lands before the close.
Extended reasoning...
The finder agents flagged that on the reader-backend short-EOF path, self.resp.force_close() replaces self.resp.end(chunk, …) and therefore never writes the final chunk bytes. Verifiers confirmed this is intentional and harmless: the connection is being torn down precisely because the advertised Content-Length can no longer be honoured, so whether the client receives N or N+chunk.len() bytes before the close, it observes the same truncated-body condition. The fail_with() path already uses the identical detach_resp(); force_close(); (on_…)(ctx, resp) sequence, so lifecycle ordering is established. Not approving because this is core Bun.serve connection-lifecycle code and CI hasn't reported yet.
The buffered-reader path (SSL / macOS / Windows) reaches early EOF via on_reader_done -> finish(), not on_read_chunk(_, Eof), when the resumed read() returns 0 with an empty stack buffer. Move the short-body check into finish() so that path force-closes too, and route on_read_chunk's short-EOF case through the same path. Add SSL test variants (static route + fetch handler) that exercise the reader backend on all platforms; the plain-TCP variants remain Linux-gated to exercise sendfile.
There was a problem hiding this comment.
Thanks — 37cfce2 fully addresses the reader-backend coverage gap (and turned up the on_reader_done → finish() path I'd missed). No further issues found, but this touches Bun.serve connection lifecycle so worth a human look before merge.
Checked: end_sendfile(short) / finish() follow the existing fail_with() pattern (detach_resp() before force_close(), on_complete still fires); max_size.is_some_and(|m| m > 0) in finish() is only reachable via on_reader_done/on_writable with !RESPONSE_DONE, so it can't force-close a fully-delivered body (max_size is decremented to 0 on the happy path) or a pipe/FIFO (length: None). The 4-variant test now covers sendfile × reader backend × FileRoute × RequestContext.
Extended reasoning...
Overview
The PR changes early-EOF handling in FileResponseStream (the shared file-body streamer for Bun.serve static routes and fetch-handler Bun.file responses). Three sites now detect "file shrank below advertised Content-Length" and force_close() the socket instead of ending the response cleanly on a keep-alive connection: on_sendfile (Linux sendfile backend, sent == 0 && remain > 0), on_read_chunk (defensive early-return on Eof with max_size > 0), and finish() (the actual reader-backend termination point). A new 4-variant test in bun-serve-file.test.ts truncates a 32 MiB file mid-stream and asserts the socket closes short rather than desyncing a follow-up pipelined request.
Changes since my last review
My previous comment flagged that the reader backend had no test coverage. The author added SSL variants (which force the reader backend on all platforms via can_sendfile() returning false for non-TCP) and, in doing so, discovered the original on_read_chunk fix was dead code — early EOF on a resumed zero-byte read reaches finish() via on_reader_done, not on_read_chunk(_, Eof). The fix was moved to finish() and the on_read_chunk guard kept as a defensive early-return. All four test variants are reported to fail on stock bun and pass with the fix.
Security risks
None introduced. The change tightens behavior (closing a connection that would otherwise desync) and does not add any new input parsing or trust boundary.
Level of scrutiny
Medium-high. This is production Bun.serve connection-lifecycle code interacting with uWS callback userdata and intrusive refcounts. The changes themselves are small and follow the existing fail_with() pattern exactly (detach_resp() → force_close() → callback), and I traced that finish()'s new max_size > 0 check cannot fire on the happy path (decremented to Some(0)) or for unbounded streams (None). But subtle uWS lifetime interactions in this file have caused UAFs before (see the neighboring FIFO-backpressure and double-close regression tests), so a maintainer familiar with AnyResponse::force_close semantics should confirm.
Other factors
CI build #73073 was still running at review time and the PR description notes the tests were not run locally (platform-gated). The plain-TCP variants remain skipIf(!isLinux), which is reasonable given sendfile is Linux-only, and the SSL variants run everywhere.
|
CI build #73073: 283/286 passed. The three red lanes failed on tests that also fail on main and are unrelated to this diff:
Ready for review. |
…after it (#39296) Follow-up to #39201, where moving the slice window into the reader was agreed as a separate PR. ### Problem - Streaming a slice of a pipe, socket or stdin consumes bytes past the slice. With `0123456789` written to a child's stdin, `for await (const c of Bun.stdin.slice(0, 3).stream())` delivers `012` and whoever reads stdin next gets nothing: the other 7 bytes were read and dropped. Same through a native sink (`HTMLRewriter.transform(new Response(Bun.stdin.slice(0, 3)))`). With 60000 bytes written, the stream leaves 0 of them in the pipe. Same on Windows. - `Bun.stdin.slice(0, 0).stream()` (any empty slice of a pollable source) never ends while the pipe stays open and silent; it ends only when a byte arrives, which it drops, or at EOF. Same through a native sink. - `Bun.serve` responding with `Bun.file(p).slice(0, 2048)` of a 4 KiB file issues two `read()`s per response (a 256 KiB request that returns the whole file, then an EOF probe) and ends the response from a queued task. Harmless for regular files, but it is the same after-the-fact window as above, implemented a second time. - Cause: the window is applied by the reader's parents after the bytes have been read. `FileReader::on_read_chunk` cuts the delivered chunk down (`src/runtime/webcore/FileReader.rs:636`); the only place that cut the read itself was `on_pull`'s `read_into` branch (`FileReader.rs:849`), which is skipped whenever a poll is registered, i.e. for every pollable fd, and `read_into` is a stub on Windows. The read loop in `src/io/PipeReader.rs` reads up to its 256 KiB scratch buffer (`fill_scratch`, line 653) and the Windows reader hands libuv its whole spare capacity (`get_read_buffer_with_stable_memory_address`, line 1166); neither knows about the window. The empty-window hang is the same gap: the check lived on the `read_into` branch, and a pollable source parks on its poll before reaching it. `FileResponseStream` keeps a third copy of the window (`src/runtime/server/FileResponseStream.rs:277`) and needs the `FileResponseStreamEof` task to end itself. ### Fix - `PosixBufferedReader` and `WindowsBufferedReader` get a `ReadLimit` next to the pread offset, set with `set_limit` before starting. `read_once` (every POSIX read: loop, buffer and `read_into`) and the libuv buffer on Windows cut each read to what is left of it, and charging it down to zero is reported the way EOF already is (`Stop::Eof` / `ReadState::Eof`, then `close` and `on_reader_done`). A limit that starts at zero makes `start` register no poll and issue no read; the parent's first read request (`read`, `read_into`, Windows `unpause`) reports EOF without touching the fd. - `FileReader` passes the blob's size to the reader at start and `FileResponseStream` passes the body length; both now treat the end of the window exactly like EOF, because that is what the reader reports. Deleted: `FileReader`'s `total_readed` / `window_remaining` / `consume_window` / `end_at_window`, `FileResponseStream`'s `max_size` and the `FileResponseStreamEof` task (tag, dispatch arm, `Taskable` impl), and `Chunk::truncate`, which only existed for this. - `FileResponseStream` implements the reader's `ref_` / `deref` (as `FileReader` does). The chunk that fills the window is now followed by `on_reader_done`, which releases the stream's owning ref, while the reader's read loop (POSIX) or libuv completion (Windows, which re-checks the reader's flags after delivering a data read) is still on the stack with the reader embedded in the stream; the pin keeps the stream alive until those frames return. This is the job the queued task used to do. - Why this is the right place: the window is a bound on how many bytes may be taken out of the source, and on anything that is not seekable the bytes a read returns are gone, so the bound has to be applied to the read, and only the reader knows whether a read is needed at all. Reporting it as EOF is correct because for this reader the source does end there, and every consumer already handles EOF (real EOF and `maxBuffer` exhaustion end streams through the same path), which is what lets the two parent implementations (three code paths, two of which #39201 had to bring back into agreement) go away. For regular files the delivered bytes are unchanged; only the reads get shorter. - Verified with `bun bd test test/js/bun/util/bun-stdin-slice.test.ts test/js/bun/http/serve-file-slice-read-error.test.ts`. The five new `bun-stdin-slice` cases run the slice in a grandchild sharing the child's stdin and then read what is left: a write larger than the window and an empty window with nothing written, each via `stream()` and via HTMLRewriter, plus a window filled by a write larger than its remainder. With `src/` stashed, the three over-read cases get `""` instead of `3456789` / `456` back and the two empty-window cases time out; with the fix all pass on Linux and on a Windows x64 debug build (the block is not Windows-skipped; the same protocol reproduces the over-read with the current Windows canary). `serve-file-slice-read-error.test.ts` kept its ptrace read counter: 3 responses now take 3 `read()`s and the EIO injected into the second one fails that response alone (`DONE 2`); without the fix it is 6 reads and `DONE 3`. - Also run with the fix: `blob.test.ts` (the #39201 window cases: `read_into`, sink, Windows `on_read_chunk`, empty window on a regular file), `bun-serve-file`, `bun-serve-static`, `serve-directory-routes` (empty files give `FileResponseStream` a zero length), `bun-file*`, `spawn.test.ts`, `spawn-maxbuf` (the budget shares `read_once` / `on_read`), `spawn-streaming-stdout`, `spawn-stream-serve`, `spawn-stdout-filereader-gc-uaf`, `streams.test.js`, `html-rewriter*`, `body*.test.ts`, `process-stdin`, `bun-write`; on Windows additionally `blob`, `bun-serve-file`, `bun-serve-static`, `spawn-maxbuf`, `bun-file*`. `cargo fmt` clean. ### Background - `BufferedReader` (`src/io/PipeReader.rs`, one POSIX and one libuv implementation behind a cfg alias) reads an fd on behalf of a parent embedded around it and reports through a vtable: `on_read_chunk` per read, then `on_reader_done` at EOF or `on_reader_error`. `FileReader` is the parent behind a file-backed `ReadableStream` (`Bun.file().stream()`, `new Response(file).body`, the stream a native sink such as HTMLRewriter or a fetch body pulls from); `FileResponseStream` is the parent that writes a `Bun.serve` file body to the socket. - A file blob carries an offset and a size; for a slice these describe the window. `ReadableStream::from_blob_copy_ref` copies them into `FileReader` as `start_offset` / `max_size`; `Bun.serve` computes the body's offset and length from the blob (or a Range header) and passes them to `FileResponseStream`. The offset was already the reader's job (`start_file_offset`); this PR gives it the length as well. - On POSIX a pollable fd (pipe, socket, tty) is read when its poll fires, from the reader's own loop; a regular file is read synchronously when the parent asks (`read`, or `read_into` straight into the JS pull buffer). `FileReader::on_pull` only uses `read_into` when no poll is registered, which is why a window applied there never affected pollable sources. On Windows every read completes through libuv into a buffer the reader allocates up front, so that allocation is where a read's size is decided. - `ref_` / `deref` on the parent vtable are taken by the reader around a read dispatch (`ParentKeepAlive`) because the reader touches its own fields after calling into the parent; a parent whose `on_reader_done` can free it needs them so that free happens after the dispatch returns. `maxBuffer` (`MaxBuf`) is the reader's other byte bound; the limit is charged and reported alongside it. <details> <summary>Related open PRs</summary> - #39257 (alii) fixes the empty-window hang inside `FileReader` with the helpers this PR deletes; if it lands first this PR will be rebased to drop those two guards (the reader covers them) and keep its tests, which pass here as well. The over-read is not covered there. - #31680 and #33510 are about the buffered (`.text()` / `.arrayBuffer()`) path in `blob/read_file.rs`, which does not use `BufferedReader`; #31680's `FileReader.rs` hunk was made unnecessary by #39201. - #34185 wanted `FileResponseStream`'s remaining count to tell a short file from a complete one; with this PR that is the bytes it has written versus the length it was given. </details>
|
Closing: superseded by #41622. That change covers the same bug with one shared byte balance across the sendfile and reader backends, adds the finish() EOF site that this branch misses, and tests both plain and TLS. This branch is in conflict with main on both files. |
|
Correction to my closing comment above. This branch does handle the The reasons to continue in #41622 are these:
|
Problem
Bun.serveserving aBun.filebody: when the backing file is truncated while the response is being sent (log rotation, truncate+rewrite deploy, non-atomic asset rewrite), the server treats early EOF as normal completion. The body ends millions of bytes short of its declaredContent-Lengthwith no close, and on a keep-alive connection the next request's entire response (status line + headers + body) is delivered inside the first response's declared body region: an HTTP/1.1 framing desync (RFC 9112 6.2). WithidleTimeout: 0the client stalls forever.Node closes the connection after hitting EOF so the client detects the short body immediately.
Cause
FileResponseStream::on_sendfiletreatssent == 0the same asremain == 0:sendfile(2)returning 0 with bytes still owed means the input file's EOF is now below our offset. The advertisedContent-Lengthcan no longer be honoured, so the only framing-safe recovery is to close the connection. The buffered-reader backend (SSL / macOS / Windows / sub-1MiB files) has the same shape: earlyReadState::Eofwithmax_size > 0was routed toresp.end().Fix
On early EOF in either backend,
force_close()the socket instead of marking the response done, so the client observes the short body and cannot desync.on_completestill fires for caller cleanup.Test
test/js/bun/http/bun-serve-file.test.tsgains a Linux-gated pair covering both the static-route path (FileRoute) and thefetchhandler path (RequestContext): a 32 MiB file is truncated to 1 KiB mid-stream, the test waits forserver.pendingRequests == 0, then asserts the socket closed with a short body and that a follow-up request was not served inside the first body window.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/http/bun-serve-file.test.ts