Bun.serve: chunk-frame FIFO/pipe file bodies instead of sending Content-Length: 0 - #36243
Conversation
WalkthroughChangesFIFO response framing
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Overlaps with #34257 on the
#34257 currently has merge conflicts; this one is the minimal fix + desync test and is independently landable. Whichever lands first, the other rebases cleanly over it. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
My earlier writerFd cleanup nit is addressed and I didn't find further issues. The one-line runtime change brings do_sendfile in line with FileRoute (which already gates Content-Length on file_type == File), and regular-file responses are unaffected. Deferring to a maintainer given this is Bun.serve response-framing code and there's #34257 to sequence.
What was reviewed:
- Traced
needs_content_length→render_metadata→mark_wrote_content_length_header; with the flag off,FileResponseStream'sresp.write()chunk-frames as intended, and theis_regularsendfile fast path is untouched. - Confirmed
FileRoute.rs:576already does the equivalent gate, so this is convergence not divergence. - Test:
try/finallynow coverswriterFd, the body-on-wire poll exits onsocketClosed, hermetic (port: 0,tempDir,skipIf(isWindows)).
Extended reasoning...
Overview
One-line runtime change in src/runtime/server/RequestContext.rs: self.flags.set_needs_content_length(true) → set_needs_content_length(is_regular) inside do_sendfile. Plus a ~90-line wire-level regression test in test/js/bun/http/bun-serve-file.test.ts that opens a FIFO, serves it via a fetch handler over a raw keep-alive TCP socket, and asserts the first response is chunk-framed (no Content-Length, not glued to the next status line).
Security risks
The bug being fixed is itself a mild security concern — an HTTP/1.1 keep-alive framing desync (body bytes emitted past a declared Content-Length: 0) is the shape of a response-smuggling primitive. The fix removes it. The change cannot introduce a new desync: it only stops writing a header that was wrong; uWS then falls back to standard chunked transfer-encoding. No user-controlled input is newly interpreted.
Level of scrutiny
Higher than the diff size suggests, because it's Bun.serve response-framing on the fetch-handler path. I traced the flag's single consumer (render_metadata, line ~3768) and the downstream FileResponseStream::start call: for !is_regular, length: None is passed and the stream uses resp.write(), which chunk-frames when mark_wrote_content_length_header() was never called. Regular files still hit set_needs_content_length(true) and the sendfile fast path unchanged. The sibling FileRoute path already gates the same header on file_type == FileType::File (FileRoute.rs:576), so this is bringing the two entry points into agreement rather than introducing new behavior.
Other factors
- My previous review flagged the
writerFdleak-on-failure; 4cd369c wrapped it intry/finallywith awriterFd = undefinedsentinel and added asocketClosedescape on the poll loop — verified in the current diff. - The comment-cop bot's complaints were addressed (inline comment dropped in a297e93); all threads resolved.
- PR body shows the test fails under
USE_SYSTEM_BUN=1and passes with the fix, and the full 104-test file still passes. - Overlaps intentionally with open PR #34257 on the same line (different failure mode: char-device epoll EPERM vs. FIFO framing desync). A maintainer should decide which lands first / whether to fold them; either way they compose.
I'm not approving outright because HTTP response framing in Bun.serve is a core-enough path that a human should sign off, and the #34257 overlap is a coordination decision I shouldn't make.
|
Updated 11:05 AM PT - Jul 29th, 2026
❌ @Jarred-Sumner, your commit 175aae3 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 36243That installs a local version of the PR into your bun-36243 --bun |
There was a problem hiding this comment.
LGTM — the one-line runtime change brings do_sendfile in line with FileRoute, which already gates Content-Length on file_type == File.
What was reviewed:
- Traced
needs_content_lengththroughrender_metadata→mark_wrote_content_length_header()→ uWSwrite()chunk-framing; regular-file and HEAD/empty paths (is_regular && remain == 0) are unchanged. - Confirmed
FileResponseStream::startalready receiveslength: Nonefor non-regular files, so the priorContent-Length: 0header was the only thing suppressing chunked framing. - Test:
writerFdnow released viatry/finally(prior feedback addressed), the wire-capture promise resolves onclose/errorso a regression fails the assertion instead of timing out, andawait using serverdisposes before thefinallyruns.
Extended reasoning...
Overview
One-line change in src/runtime/server/RequestContext.rs: do_sendfile now sets needs_content_length to is_regular instead of unconditionally true. For FIFOs/char-devices/sockets (S_ISFIFO/S_ISCHR/S_ISSOCK), stat.st_size is 0, so the old code wrote Content-Length: 0 and then streamed the fd to EOF via resp.write() — bytes past the declared length, which desyncs the next response on a keep-alive connection. With the flag off, render_metadata skips mark_wrote_content_length_header() and uWS chunk-frames the body writes. The test in test/js/bun/http/bun-serve-file.test.ts opens a raw TCP client, captures the wire bytes, and asserts the head has Transfer-Encoding: chunked, no Content-Length, and the body payload arrives.
Security risks
The bug being fixed is itself a mild HTTP desync (response-framing) hazard on keep-alive connections. The fix removes it. No new attack surface: the change only stops emitting a header that was wrong; it does not touch input parsing, auth, or TLS.
Level of scrutiny
Low-to-medium. The runtime change is a single boolean and mirrors the existing FileRoute behavior at FileRoute.rs:576 (if file_type == FileType::File && !resp.state().has_written_content_length_header()), so this is bringing the fetch-handler path in line with its sibling rather than introducing new logic. The regular-file sendfile fast path, the empty-regular-file early-end (is_regular && remain == 0), and Range handling are all gated on is_regular and unaffected.
Other factors
My prior review comment (fd leak on the failure path) was addressed in 4cd369c and refined in 022d2d8: writerFd is now wrapped in try/finally, and the wire-capture uses Promise.withResolvers resolved from data/close/error so a regression that never delivers the payload fails the assertion rather than spinning to timeout. The comment-cop feedback on the inline Rust comment was also addressed (comment removed; rationale lives in the commit message). The PR body notes the intentional overlap with #34257 and the two compose. The full bun-serve-file.test.ts suite (including the existing FIFO backpressure test that exercises the same FileResponseStream path) is reported passing.
|
Build #85261 (latest, on the merge-from-main): Open question from above: happy to extract a shared |
…nt-Length: 0 do_sendfile wrote Content-Length from the stat-derived blob size for every file body and then streamed non-regular fds to EOF with no length bound. A FIFO stats as 0 bytes, so the head said Content-Length: 0 while the pipe bytes still went out after it. On a keep-alive connection those bytes land where the client parses the next response's status line (RFC 9112 6.3). Only set needs_content_length for regular files. With no Content-Length marked, uWS's write()/end() path chunk-frames the body (or writes a correct Content-Length when the whole body arrives in the single end() call), and the regular-file sendfile fast path is unchanged.
Also exit the body-on-wire poll if the socket closes first, so a regression that never delivers the body fails the assertion instead of spinning to timeout.
…in the commit message
The second-request assertion depended on the server cleanly terminating the chunked body when the FIFO writer closes, which turns out to be platform-dependent (Linux serves the pipelined request without a 0\r\n\r\n terminator; macOS force-closes). That is a separate pre-existing behaviour and not what this test covers, so drop the keep-alive leg and assert on what this change actually fixes: no Content-Length in the head, body framed as chunked, and no body bytes emitted past a Content-Length: 0.
022d2d8 to
6eba603
Compare
There was a problem hiding this comment.
LGTM — one-line fix bringing the fetch-handler file path in line with what FileRoute already does for non-regular files.
Reviewed: do_sendfile — is_regular is already the gate for the neighboring size-clamp and content-range blocks, and render_metadata only writes/marks Content-Length when this flag is set, so non-regular fds now fall through to uWS chunked mode instead of CL: 0 + raw body bytes. Regular-file sendfile path is unchanged. Test asserts wire-level framing (no CL, transfer-encoding: chunked, body delivered) over a raw socket; the earlier fd-leak nit is addressed with try/finally and the poll now resolves on socket close/error. CI reports bun-serve-file.test.ts green on all lanes.
Extended reasoning...
Overview
One production line in src/runtime/server/RequestContext.rs: self.flags.set_needs_content_length(true) → set_needs_content_length(is_regular) inside do_sendfile. This stops the fetch-handler file-response path from writing Content-Length: 0 for FIFOs/character devices/sockets (whose st_size is 0) and then streaming the fd's bytes past that declared length — an HTTP/1.1 framing desync on keep-alive. With the flag off, render_metadata skips the CL header and mark_wrote_content_length_header(), so uWS's write() enters chunked mode. The static FileRoute path already gates CL on file_type == File; this brings the dynamic path to parity. A new wire-level test in bun-serve-file.test.ts opens a FIFO, serves it via a fetch handler, and asserts the raw response head over a Bun.connect socket.
Security risks
The bug being fixed is itself a response-framing / keep-alive desync (body bytes past a Content-Length: 0 head land where the client parses the next status line). The fix moves in the strictly safer direction: unknown-length bodies become chunked instead of mis-declared. Regular files keep their exact CL. No new attack surface introduced.
Level of scrutiny
Bun.serve HTTP framing is a critical path, but the change is a single boolean substitution using a local that already gates every neighboring block (b.size.set(...), needs_content_range, the sendfile.remain clamp). I traced the consumer at RequestContext.rs:3768-3771 — when the flag is false the CL header and the wrote-CL mark are simply skipped, which is exactly what lets uWS chunk-frame subsequent write() calls. The change cannot affect the regular-file fast path (is_regular == true there). Given the mechanical nature and the existing sibling pattern in FileRoute, this doesn't need a human to re-derive the reasoning.
Other factors
- My earlier inline nit (writerFd only closed on the happy path) was addressed: the test body is now wrapped in
try { ... } finally { closeSync(writerFd) }, and the wire-capture wait resolves ondatacontaining the marker or on socketclose/error, so a regressed build fails the assertion instead of spinning. USE_SYSTEM_BUN=1fail /bun bdpass verification is documented in the PR body; robobun reportsbun-serve-file.test.tsgreen on all CI lanes including darwin.- Overlap with #34257 on this exact line is acknowledged in the PR body and thread; whichever lands first, the other rebases trivially. That coordination doesn't block this change.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Can we reuse more code from FileRoute?
|
The piece that's actually duplicated between
So a bigger shared helper would need ~5 callbacks and wouldn't shrink either site. The small win is a Happy to add that helper here, or keep this PR to the one-line framing fix and do the extraction as a follow-up. Your call. |
There was a problem hiding this comment.
LGTM — the fd-cleanup nit from the earlier pass is addressed, and the one-line is_regular gate matches what FileRoute already does.
What was reviewed:
render_metadataat RequestContext.rs:3768 — with the flag false,mark_wrote_content_length_header()is skipped so uWS chunk-frameswrite()calls.- Regular-file path unchanged: the fetch-handler empty-file test (
returns 200 for empty files served from the fetch handler) still assertscontent-length: 0, andis_regularis true there. - Test now resolves on
close/erroras well as body arrival, so a regressed build fails thebodyDeliveredassertion instead of timing out;writerFdis released infinally.
Extended reasoning...
Overview
One-line change in RequestContext::do_sendfile (set_needs_content_length(true) → set_needs_content_length(is_regular)) plus a wire-level framing test in bun-serve-file.test.ts. The bug: for FIFO/char-device/socket bodies, the fetch-handler path wrote content-length: 0 (from the 0-byte stat) and then streamed the fd to EOF via resp.write(), putting body bytes past the declared length and desyncing the next keep-alive response. The fix suppresses the Content-Length header for non-regular files so uWS chunk-frames the body — the same gate FileRoute already applies via file_type == File.
Security risks
The pre-fix behavior was itself the security-relevant issue (HTTP response desync on a keep-alive connection). The fix narrows behavior to match the sibling static-route path; no new attack surface.
Level of scrutiny
HTTP framing in Bun.serve is production-critical, but the change is a single boolean whose only effect is on the !is_regular branch. I traced needs_content_length to its consumer at render_metadata (RequestContext.rs:3768–3771): when false, neither write_header_int("content-length", ...) nor mark_wrote_content_length_header() runs, which is exactly what lets uWS enter chunked mode on the first body write. The is_regular = true path (including zero-byte regular files, covered by the existing "returns 200 for empty files served from the fetch handler" test asserting content-length: 0) is byte-identical to before.
Other factors
- I reviewed an earlier revision and raised one nit (fd leak on failure paths); it was fixed by wrapping in
try/finallyand replacing the unbounded poll with aPromise.withResolversthat also resolves on socketclose/error. The comment-cop feedback on the source comment was also addressed (comment removed). - The test asserts the strongest observable invariant (raw wire bytes: no CL header,
transfer-encoding: chunkedpresent, body delivered, no body-past-CL:0) rather than going throughfetch(), which would mask the framing. - CI:
bun-serve-file.test.tspasses on every lane that ran; the redbuild-bunsteps are the unrelated "sibling build-cpp errored" infra failure. - Known overlap with #34257 on this exact line is acknowledged in the PR body; whichever lands first, the other rebases trivially.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/js/bun/http/bun-serve-file.test.ts`:
- Around line 1122-1147: Extend the keep-alive test around the existing captured
response and client connection to send a second GET request after the FIFO
response settles, then verify the subsequent response is parsed cleanly and
separately. Preserve the current assertions for chunked transfer, body delivery,
and absence of Content-Length, and ensure the client remains open until the
second response is received.
🪄 Autofix (Beta)
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: 3102c4c5-2713-4b0e-939b-a268965d24ec
📒 Files selected for processing (2)
src/runtime/server/RequestContext.rstest/js/bun/http/bun-serve-file.test.ts
|
@robobun fix conflcits or close |
|
Conflicts resolved in 0c84a70 (main refactored |
There was a problem hiding this comment.
LGTM — one-token fix bringing do_sendfile into parity with FileRoute's existing file_type == File gate on Content-Length.
What was reviewed:
- Confirmed
FileRoute::onalready gatescontent-lengthonfile_type == FileType::File(FileRoute.rs:493), so this change makes the fetch-handler path match the static-route path. - Verified
render_metadataonly writes/marks Content-Length when the flag is set (RequestContext.rs:3947), so with the flag off uWS falls through to chunked framing as intended. - Test:
writerFdcleanup now intry/finally(my earlier concern addressed);wireDoneresolves on close/error so regressions fail the assertion rather than spinning; on the pre-fix build the payload still reaches the wire raw sohasContentLength: falseis the assertion that fails.
Extended reasoning...
Overview
One-line source change in src/runtime/server/RequestContext.rs: self.flags.set_needs_content_length(true) → set_needs_content_length(is_regular). is_regular is computed a few lines above via S::ISREG(mode). For FIFOs/char-devices/sockets, st_size is 0 but the body streams to EOF; emitting Content-Length: 0 and then raw-writing the body is an HTTP/1.1 framing desync. The sibling static-file-route path (FileRoute::on, line 493) already gates Content-Length on file_type == FileType::File, so this brings the two paths into agreement. The rest of do_sendfile already branches on is_regular for size clamping and content-range, so this is the last unconditional piece.
The test addition is a raw-TCP wire-level check: opens a FIFO r+, serves Bun.file(fifo) from a fetch handler, writes a payload into the FIFO, and asserts the response head has transfer-encoding: chunked and no content-length. The test is skipped on Windows (no mkfifo) and lives next to the existing FIFO backpressure test.
Security risks
None introduced. This removes a response-smuggling / framing-desync vector on keep-alive connections when a handler returns a FIFO/pipe body. No new input parsing.
Level of scrutiny
Low-to-medium. The source change is a single boolean substitution whose value is computed immediately above and already used to gate three neighboring decisions in the same function. The parallel code path (FileRoute) already does exactly this. CI on the target test file passed on every lane; the two build failures were unrelated main-break flakes.
Other factors
- My prior inline comment (fd leak on the failure path) was addressed:
writerFdis now closed infinally, and the wire poll resolves on socket close/error instead of spinning. - CodeRabbit's suggestion to add a second keep-alive request was declined with a concrete reason (macOS FIFO-EOF path force-closes the connection, a separate pre-existing issue) and CodeRabbit withdrew it.
- The comment-cop nags were addressed (inline comment removed; rationale lives in the PR body).
- Jarred asked for conflicts to be fixed, which was done via merge commit 0c84a70; the diff is unchanged in substance.
- The open "extract
FileType::from_stat_modehelper" question was explicitly offered as a follow-up; Jarred's response was to land as-is. - Overlap with #34257 is noted and intentional; whichever lands first, the other rebases cleanly.
…nt-Length: 0 (oven-sh#36243) ### Repro ```js // mkfifo /tmp/p; (sleep 0.3; printf 'PIPEBYTES!' > /tmp/p) & const srv = Bun.serve({ port: 0, fetch: r => new URL(r.url).pathname === '/fifo' ? new Response(Bun.file('/tmp/p')) : new Response('SECOND-RESPONSE') }); // raw keep-alive client: GET /fifo, then GET /plain ``` Wire capture on one keep-alive connection, before: ``` HTTP/1.1 200 OK content-type: application/octet-stream content-disposition: filename="p" content-length: 0 Date: ... PIPEBYTES!HTTP/1.1 200 OK <- pipe body glued onto response 2's status line content-length: 15 ... SECOND-RESPONSE ``` The first response declares `Content-Length: 0` and then still streams the FIFO's bytes after the head. A client that honours the declared length parses `PIPEBYTES!...` as the next response's status line (RFC 9112 6.3), i.e. a framing desync on the keep-alive connection. ### Cause `RequestContext::do_sendfile` classifies `S_ISFIFO`/`S_ISCHR`/`S_ISSOCK` as non-regular (`is_regular = false`), sets the blob size to `min(original_size, st_size) = 0` (a FIFO stats as 0 bytes), then unconditionally sets `needs_content_length = true`, so `render_metadata` writes `content-length: 0` and marks the header as written. `FileResponseStream` is then started with `length: None` and streams the fd to EOF via `resp.write()`; with the Content-Length mark set, uWS emits those bytes raw instead of chunk-framing them. Two framing decisions that disagree. `FileRoute` (static file routes) already gates the Content-Length header on `file_type == File`, so only the fetch-handler path is affected. ### Fix Set `needs_content_length` only for regular files. With no Content-Length marked, uWS's `write()` enters chunked mode on the first body chunk (and `end()` writes a correct `Content-Length: N` when the whole body arrives in the single terminating read). The regular-file sendfile fast path is unchanged. ### Verification ``` $ USE_SYSTEM_BUN=1 bun test test/js/bun/http/bun-serve-file.test.ts -t 'frames the body as chunked' (fail) Response(Bun.file(FIFO)) frames the body as chunked, not Content-Length: 0 { firstHasContentLength: true, firstIsChunked: false, gluedToNextStatusLine: true } $ bun bd test test/js/bun/http/bun-serve-file.test.ts -t 'frames the body as chunked' (pass) Response(Bun.file(FIFO)) frames the body as chunked, not Content-Length: 0 ``` Full `bun-serve-file.test.ts` (104 tests, including the existing FIFO backpressure test) still passes. Related: oven-sh#34257 changes the same line for a different symptom (character-device epoll `EPERM`); the two compose. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 8 · 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 <!-- robobun:evidence:end --> --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
2 participants
Repro
Wire capture on one keep-alive connection, before:
The first response declares
Content-Length: 0and then still streams the FIFO's bytes after the head. A client that honours the declared length parsesPIPEBYTES!...as the next response's status line (RFC 9112 6.3), i.e. a framing desync on the keep-alive connection.Cause
RequestContext::do_sendfileclassifiesS_ISFIFO/S_ISCHR/S_ISSOCKas non-regular (is_regular = false), sets the blob size tomin(original_size, st_size) = 0(a FIFO stats as 0 bytes), then unconditionally setsneeds_content_length = true, sorender_metadatawritescontent-length: 0and marks the header as written.FileResponseStreamis then started withlength: Noneand streams the fd to EOF viaresp.write(); with the Content-Length mark set, uWS emits those bytes raw instead of chunk-framing them. Two framing decisions that disagree.FileRoute(static file routes) already gates the Content-Length header onfile_type == File, so only the fetch-handler path is affected.Fix
Set
needs_content_lengthonly for regular files. With no Content-Length marked, uWS'swrite()enters chunked mode on the first body chunk (andend()writes a correctContent-Length: Nwhen the whole body arrives in the single terminating read). The regular-file sendfile fast path is unchanged.Verification
Full
bun-serve-file.test.ts(104 tests, including the existing FIFO backpressure test) still passes.Related: #34257 changes the same line for a different symptom (character-device epoll
EPERM); the two compose.no test proof · iteration 8 · 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