Bun.serve: terminate FIFO/pipe file responses at EOF - #37082
Conversation
When a pipe's writer closes after its last write was already consumed, EOF reaches FileResponseStream through on_reader_done with no pending bytes, and finish() ended the response with end_without_body(), which writes a bare CRLF and marks the response done without completing the body framing. A chunk-framed body never got its terminating 0-chunk, so the wire ended "5\r\nhello\r\n\r\n" and clients fail parsing (curl: "chunk hex-length char not a hex digit: 0xd", node: HPE_INVALID_CHUNK_SIZE), and a pipe that closed without writing produced a head with neither Content-Length nor Transfer-Encoding that never completes. End the response with end() instead: it writes the terminating chunk when in chunked mode and Content-Length: 0 when nothing was written.
Walkthrough
ChangesFIFO response completion
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/server/FileResponseStream.rs`:
- Around line 544-552: Shorten the comment above the EOF handling to state only
the durable invariant: call end with empty data so the response correctly
completes body framing and emits the appropriate zero-length response headers.
Remove the historical explanation of end_without_body and its failure symptoms.
In `@test/js/bun/http/bun-serve-file.test.ts`:
- Around line 1225-1227: Remove the fixed 3000 ms setTimeout deadline around
resolveWire in the wireDone response flow, and await the observable completion
event instead so the test runner supplies the hang timeout. Apply the same
change to the related timer at the additionally referenced lines, preserving the
existing wire collection and assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4eb00569-54ec-4f6a-9e66-fac8cc47c02a
📒 Files selected for processing (2)
src/runtime/server/FileResponseStream.rstest/js/bun/http/bun-serve-file.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Updated 8:13 PM PT - Aug 6th, 2026
❌ @robobun, your commit d789bfc has 1 failures in
🧪 To try this PR locally: bunx bun-pr 37082That installs a local version of the PR into your bun-37082 --bun |
The tests relied on the write-end open blocking until the server opened the FIFO's read end. That rendezvous raced: the writer dance could run before the request was served (EPIPE on write, or a server that waits forever for a writer that already left), and on macOS a FIFO kevent registered while no writer exists never fires, so the darwin lanes never streamed at all. Hold the FIFO open read+write before the request, hand over to the real writer only after the response head reaches the client (proving the read end is open and polled), and resolve the ordering promises on close/error so a broken build fails with the captured wire instead of hanging to the runner timeout. idleTimeout bounds that capture.
There was a problem hiding this comment.
LGTM — the one-line end_without_body → end(b"", …) swap is the correct completion for the reader-EOF path, and the earlier test race/cleanup concerns from this review were addressed in d789bfc.
What was reviewed:
- Traced
finish()'s!RESPONSE_DONEbranch to its only live entry (on_reader_donewith no trailing bytes); the abort/error/sendfile/deferred-EOF paths all setRESPONSE_DONEfirst, so they are unaffected. - Confirmed against uWS
internalEnd(HttpResponse.h) thatend("")emits the 0-chunk whenHTTP_WRITE_CALLEDandContent-Length: 0otherwise, whereasendWithoutBody(nullopt)writes only the bare CRLF — matches the PR's cause analysis. - Re-checked the d789bfc test rewrite: keeper
r+fd is held until the real writer is established,headSeen/payloadSeen/wireDoneall resolve onclose/error, writer and keeper fds are closed infinally, andidleTimeout: 5bounds a broken build instead of a fixed JS deadline.
Extended reasoning...
Overview
The runtime change is a single line in FileResponseStream::finish(): when the buffered reader reports EOF with no trailing data (the pipe/FIFO case where data and EOF arrive as separate poll events), the response now completes via resp.end(b"", …) instead of resp.end_without_body(…). The test change adds a two-variant describe block to bun-serve-file.test.ts that drives a FIFO body over a raw socket and asserts the wire forms a complete HTTP/1.1 message.
Correctness of the runtime change
I traced every caller of finish(). on_aborted, fail_with, end_sendfile, and the deferred FileResponseStreamEof task (via on_read_chunk's inline EOF branch) all set RESPONSE_DONE before reaching finish(), so the changed branch is unreachable from them. It fires only from on_reader_done() (and the theoretically-reachable on_writable → reader.is_done() path) when the reader closed with no bytes left. I then read packages/bun-uws/src/HttpResponse.h::internalEnd: with HTTP_WRITE_CALLED set it emits the terminating 0-chunk; without it and with allowContentLength=true it emits Content-Length: 0. endWithoutBody(nullopt) passes allowContentLength=false and falls through to a bare \r\n at line 224 — exactly the malformed wire the PR reproduces. Regular-file responses reach EOF via on_read_chunk's state == Eof arm (which already calls resp.end(chunk, …)), so they never enter this branch. The swap is strictly more correct with no behavior change on other paths.
Security risks
None. No user input parsing, no allocation, no lifetime changes. The refcount / ScopedRef structure around finish() is untouched. end() and end_without_body() have identical ownership semantics on the uWS side (both mark HTTP_END_CALLED and hand the socket back).
Level of scrutiny
Moderate — this is a hot response-completion path in Bun.serve, but the diff is a one-token semantic swap between two adjacent uWS calls whose difference I verified in the C++ source. The test file additions are larger (~140 lines) but purely additive and confined to a Windows-skipped describe block.
Other factors
This PR has already been through three feedback rounds. CodeRabbit's comment-length and fixed-deadline nits were applied; comment-cop's paragraph-comment flag was resolved down to two lines; and my own two prior inline findings (the keeper-fd race that hung one Ubuntu lane and never streamed on Darwin, and the unwired payloadSeen/writer-fd cleanup) were both fixed in d789bfc — the author confirmed the earlier shape actually failed in CI, which validates the concern was real. The current test uses the same keeper-fd pattern as the neighboring FIFO test in this file, wires every failure event to resolve all three promises, releases both fds in finally, and relies on idleTimeout + runner timeout instead of an in-test deadline. The full bun-serve-file.test.ts suite (107 tests) passes per the PR body. Nothing outstanding.
2 participants
Repro
With the pipe's data and EOF arriving as separate poll events (which the write+close pattern produces), the response head says
Transfer-Encoding: chunkedbut the body is never terminated. The wire ends:a bare CRLF where the
0\r\n\r\nlast-chunk belongs, and the keep-alive connection stays open. curl fails with(56) chunk hex-length char not a hex digit: 0xd, node http withHPE_INVALID_CHUNK_SIZE, andfetchhangs or errors. A pipe whose writer closes without writing anything is worse: the head carries neitherContent-LengthnorTransfer-Encoding(norDate) and the response never completes, so clients hang.Cause
Since the pipe bodies stopped being mis-framed as
Content-Length: 0(#36243), a FIFO body streams throughFileResponseStreamwith no Content-Length, entering uWS chunked mode on the first body write. When the pipe's EOF arrives as its own poll event with no pending bytes, the reader reports it throughon_reader_donerather than an EOF-flagged data chunk, andFileResponseStream::finish()ended the response withend_without_body().uws_res_end_without_bodyis a C shim that writes a bare\r\n(the header-terminating blank line) and marks the response done. It has no awareness of chunked mode: in a chunk-framed body that stray CRLF is the garbage curl chokes on and the terminating 0-chunk is never written, and for a body with no writes at all it terminates the headers with no framing declared, leaving the client waiting forever on a connection the server keeps open.Fix
finish()now ends the response withend()and empty data, which completes whichever framing the response is in: the terminating0\r\n\r\nchunk when body writes entered chunked mode, andContent-Length: 0when nothing was written. The data+EOF-coalesced path (on_read_chunkwith EOF state) already did this with the final chunk and is unchanged, as are the abort/error paths.Verification
New tests in
test/js/bun/http/bun-serve-file.test.tsdrive both shapes over a raw socket, closing the FIFO's write end only after the payload bytes came out the other side so EOF deterministically arrives as its own event, and assert the wire forms a complete HTTP/1.1 message (chunked with terminator, or satisfied Content-Length).The full
bun-serve-file.test.tssuite passes (107 tests), curl now exits 0 against a FIFO response where it previously failed with error 56, and an empty pipe yieldsHTTP/1.1 200 OKwithContent-Length: 0. Regular-file responses,type: "direct"streams, and HEAD requests are unaffected (they never reach this path).no test proof · iteration 1 · 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