Repository navigation
Conversation
render_metadata()/do_write_headers() stripped Content-Length on every response, then only re-emitted it for the sendfile path. A Response with a ReadableStream body and an explicit Content-Length therefore went out as Transfer-Encoding: chunked, so proxies and download endpoints that already knew the size could never tell the client (no progress bar, no resumable downloads, 411 from S3 presigned PUT targets). Capture the handler's Content-Length in render_metadata() before it is stripped, only when the body is a ReadableStream (sink/byte_stream set) and the handler did not also set Transfer-Encoding. Emit it and mark the uWS response so write()/end() use content-length framing instead of chunked. The sink's single-write try_end() fast path now routes to end() when the header is already marked, avoiding a duplicate Content-Length. HEAD over a stream body frames the same way. In-memory and file bodies are unchanged: their size is known and Bun keeps framing from it. The fetch-gzip stream test set Content-Length: 1 on a larger gzip stream to rely on the header being discarded; drop that header so the test keeps its actual subject (gzip decoding over a streamed body). Fixes #10507
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 22 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)
WalkthroughChangesUpdated HTTP streaming to preserve valid handler-provided Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 11:27 AM PT - Jul 25th, 2026
@autofix-ci[bot], your commit eeba92a is building: |
|
Reproduced on main: |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I checked whether the HEAD Locked arm could double-emit Content-Length via render_metadata()'s new capture — it can't, because sink/byte_stream are not set on the HEAD path, so the capture guard is false. Also confirmed the new res.end() branch in send_without_auto_flusher only fires under requested_end && !is_http_write_called(), so end_from_js's post-return ended_response = true still applies. This changes HTTP/1.1 wire framing in Bun.serve's hot path, so a maintainer familiar with the uWS write()/end() state machine should confirm the mark_wrote_content_length_header interaction with multi-chunk streams and keep-alive.
Extended reasoning...
The PR captures a handler-set Content-Length for ReadableStream response bodies in render_metadata() (gated on sink || byte_stream, no needs_content_length, and no user Transfer-Encoding), emits it and marks HTTP_WROTE_CONTENT_LENGTH_HEADER on the uWS response, and adjusts HTTPServerWritable::send* to use end() instead of try_end() when that flag is set so uWS doesn't write a second CL. The HEAD Locked arm mirrors the capture. I verified the HEAD path doesn't double-emit (sink/byte_stream are only assigned in the GET stream-setup path at RequestContext.rs:2040/3240, after the HEAD branch), and that the ended_response bookkeeping still holds for the new end() branch. The fetch-gzip.test.ts edit removes a bogus Content-Length: 1 that only passed because the header was previously discarded — reasonable. Deferring because this is a wire-protocol behavior change in production-critical framing code with subtle backpressure/keep-alive interactions; the test coverage is good but a maintainer sign-off on the uWS state-flag semantics is warranted.
Addresses review: render_metadata() now commits the handler's Content-Length before the stream pump starts, so a stream that errors before writing any body bytes would leave the client waiting on the promised bytes over keep-alive. handle_reject, handle_reject_stream and render_missing now force-close in that state (same treatment as the existing mid-body error case). Also adds a single-write sub-highWaterMark test that exercises the end() branch in HTTPServerWritable, and makes the proxy test stream its upstream body so the byte_stream path is taken deterministically.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
The ByteStream error path (a proxied fetch().body whose upstream dies) routed through on_pipe, which ended the response cleanly even when a committed Content-Length could no longer be satisfied, leaving keep-alive clients waiting on the promised bytes. Apply the same close-instead-of-end rule handle_reject uses. Also await the socket close event directly in the error-path tests: once(socket, "close") rejects on the ECONNRESET a forced close surfaces on Windows. Trim review-flagged comments.
![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/RequestContext.rs`:
- Around line 2608-2620: Extract the repeated Transfer-Encoding check and
Content-Length parsing into a shared helper near the relevant RequestContext
framing logic, then reuse it in both the HEAD locked-body path and
render_metadata() before headers are stripped. Keep the non-body branch on
HTTP::parse_content_length, while making the shared parser choice explicit and
preserving the existing Option behavior.
In `@test/js/bun/http/bun-serve-headers.test.ts`:
- Around line 300-304: Strengthen the assertion in the socket truncation test by
first requiring the response headers to contain the committed Content-Length
value of 100, then explicitly assert that the received body is shorter than 100
bytes. Keep the existing Buffer collection and header/body parsing flow, and
ensure the test fails when the connection closes without sending the status and
Content-Length headers.
🪄 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: 979d617b-cd6b-4308-b652-310deac11a04
📒 Files selected for processing (4)
src/runtime/server/RequestContext.rssrc/runtime/webcore/streams.rstest/js/bun/http/bun-serve-headers.test.tstest/js/web/fetch/fetch-gzip.test.ts
💤 Files with no reviewable changes (1)
- test/js/web/fetch/fetch-gzip.test.ts
Extract handler_content_length (Transfer-Encoding skip + Content-Length parse) shared by the HEAD locked-body path and render_metadata, and force_close_if_framing_committed shared by handle_reject, handle_reject_stream and the on_pipe error arm. Check the committed head strictly in the stream-error test when it was received; a reset can beat the header flush, so the close event itself stays the unconditional assertion.
| /// Framing (a chunked write or a Content-Length) is already on the wire but | ||
| /// the body cannot complete; close so the client sees a truncated message | ||
| /// instead of waiting on the promised bytes over keep-alive. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Handler-set Content-Length for framing a stream body (#10507); None when the | ||
| /// handler also set Transfer-Encoding or the value does not parse. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
No further issues from me — the prior three rounds of feedback (socket 'error' handlers in the raw-socket tests, the missing on_pipe byte_stream force-close, and the dedup of force_close_if_framing_committed / handler_content_length) are all addressed in 60772af and c90b0f3. Deferring to a human because this changes HTTP/1.1 response framing on the Bun.serve hot path: honouring a handler-set Content-Length for stream bodies is a user-visible behaviour change (a wrong CL now mis-frames instead of being discarded), and the four new force-close sites on error paths interact with keep-alive reuse.
What was reviewed:
render_metadata/do_render_head_responsecapture the handler CL only forsink/byte_streambodies, skip whenTransfer-Encodingis set, and markHTTP_WROTE_CONTENT_LENGTH_HEADERso uWS writes raw — no path double-emits the header.HTTPServerWritable::send*route tores.end()instead oftry_end()when the CL header is already marked;ended_responseis still set by the callers on the same paths as before.- All four stream-error exits (
handle_reject,handle_reject_stream,on_pipeErr,render_missing_corked) now force-close when framing is committed; theon_pipenon-CL fallthrough still terminates chunked correctly. - The
fetch-gzip.test.tsedit drops aContent-Length: 1that only worked because it was previously ignored — the test's subject (gzip decode) is unchanged.
Extended reasoning...
Overview
The PR makes Bun.serve honour a handler-set Content-Length header when the response body is a ReadableStream, instead of unconditionally stripping it and falling back to Transfer-Encoding: chunked. Touches src/runtime/server/RequestContext.rs (~70 lines: render_metadata, do_render_head_response, handle_reject, handle_reject_stream, on_pipe, render_missing_corked, end_stream, plus two extracted helpers), src/runtime/webcore/streams.rs (~13 lines in HTTPServerWritable::send*), ~380 lines of new tests in bun-serve-headers.test.ts, and a one-line test fixture correction in fetch-gzip.test.ts.
Security risks
HTTP/1.1 framing changes carry request-smuggling / response-desync risk. The change only affects the server's outgoing framing and only when the handler explicitly sets Content-Length on a stream body; the handler already controls both the header and the byte count, so this doesn't introduce a new attacker-controlled desync vector. The keep-alive test verifies the next request on the same socket parses correctly after a CL-framed stream body. The force-close-on-error additions are the correct RFC 9112 behaviour (truncate rather than under-deliver over keep-alive). The Transfer-Encoding guard prevents emitting both TE and CL. I did not find a smuggling exposure, but framing changes on the core server warrant a maintainer's eyes.
Level of scrutiny
High. RequestContext.rs is the per-request state machine for Bun.serve — the hottest and most correctness-sensitive path in the runtime. This is a deliberate behaviour change (previously a wrong handler CL was silently discarded; now it's honoured verbatim, matching node:http), and it threads through four error-handling exits plus the uWS try_end/end/markDone state machine. The test matrix is thorough (async pull, single-write fast path, Readable.toWeb, proxied fetch().body, HEAD, keep-alive framing, both sink- and byte_stream-path error truncation, and the negative/no-CL invariants), and all prior review feedback has been incorporated. But the interaction with uWS response state (HTTP_WROTE_CONTENT_LENGTH_HEADER, markDone, sendTerminatingChunk) is subtle enough that a maintainer familiar with that layer should sign off.
Other factors
Two comment-cop bot flags remain unresolved on the doc comments for force_close_if_framing_committed (line 1232) and handler_content_length (line 4597); both are 2–3 line function doc comments, not workaround justifications, so they read as false positives. This PR supersedes two prior attempts (#27262, #28673) against the pre-Rust tree.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-25, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. The linked issue (#10507) stays open. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
Fixes #10507.
Problem
Bun.servestrips a handler-setContent-Lengthheader when theResponsebody is aReadableStream, and frames the body asTransfer-Encoding: chunkedinstead. A handler that proxies an upstream body, streams a file whose size it already knows, or pipes to an S3 presigned URL cannot tell the client how long the body is (no download progress, no resumable downloads, 411 from S3).render_metadata()always callsheaders.fast_remove(ContentLength)before writing headers to uWS, and only thesendfilepath re-emits it from the stat size. Stream bodies have no known size there, so nothing puts the header back.Fix
In
render_metadata(), when the body is a ReadableStream (sinkorbyte_streamis set) and the handler did not also setTransfer-Encoding, read the handler'sContent-Lengthbefore it is stripped and emit it after the status/headers, markingHTTP_WROTE_CONTENT_LENGTH_HEADERon the uWS response. uWS then writes the body raw instead of chunk-framing it.Follow-on adjustments:
HTTPServerWritable::send*routed a single-write stream body throughtry_end(), which writes its ownContent-Length. When the header is already marked, useend()instead so it is not duplicated.do_render_head_response'sLockedarm wroteTransfer-Encoding: chunkedunconditionally for stream bodies; it now emits the handler'sContent-Lengthwhen present so HEAD matches GET.handle_reject,handle_reject_stream,render_missingand theByteStreampipe path (on_pipe, a proxiedfetch().bodywhose upstream errors) force-close the socket when theContent-Lengthis already on the wire but the stream errors before delivering the promised bytes, so the client sees a truncated message rather than waiting forever over keep-alive.In-memory and
Bun.filebodies are unchanged: Bun keeps framing those from the actual byte length. A handler that sets aContent-Lengththat does not match the bytes the stream then produces gets exactly what it asked for, same asnode:http; no byte-count enforcement is added here.Why this is correct
Node's
httpmodule honours a user-setContent-Lengthverbatim. The server does not know a stream's length, so the handler is the only source of that information, and RFC 9112 permits either framing for a known-length body. Honouring the header for stream bodies is strictly more capable than forcing chunked; handlers that do not set it keep chunked.Testing
New tests in
test/js/bun/http/bun-serve-headers.test.tscover an asyncpull()stream, a sub-highWaterMark single-write stream,Readable.toWeb, proxiedfetch().body, HEAD, keep-alive framing, a stream that errors after the header is committed, the no-CL chunked fallback, and the in-memory-body invariant. The behaviour-change tests fail before the fix (stripped CL / chunked) and pass after; the invariant tests pass on both.test/js/web/fetch/fetch-gzip.test.tssetContent-Length: 1on a larger gzip stream and relied on the header being discarded; that header is dropped so the test keeps its actual subject (gzip decoding over a streamed body).Supersedes #27262 and #28673, which targeted the pre-rewrite Zig tree.