Bun.serve: do not re-render error() after the status line is committed - #33810
Conversation
A direct ReadableStream whose pull() throws synchronously reaches handle_reject after do_render_stream has already written the 200 status and headers. handle_reject gated the error() re-render only on has_responded() (response ended), never on has_written_status(), so the server's error() handler was invoked and its Response rendered into the in-flight exchange. Debug builds hit the !has_written_status assert in do_write_status and aborted the process; release builds spliced the error() header block into the chunked body, corrupting HTTP framing. When the status is already committed handle_reject now reports the failure via the VM error reporter and terminates the body (force_close if body bytes were written, end_stream otherwise), matching what handle_reject_stream already does for the async-reject path.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughModifies the ChangesReject handling after status written
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 12:52 AM PT - Jul 9th, 2026
❌ @robobun, your commit 224d78c has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33810That installs a local version of the PR into your bun-33810 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this touches RequestContext connection-lifecycle/refcount paths (force_close/end_stream both deref) and makes a small behavior choice (reporting the error via the VM handler unconditionally, whereas handle_reject_stream only does so under DEBUG_MODE) — worth a human look.
Extended reasoning...
Overview
This PR fixes a debug-build assertion crash / release-build HTTP framing corruption in Bun.serve when a direct ReadableStream's pull() throws synchronously after the status line has already been written. The fix adds an early-return branch in handle_reject (src/runtime/server/RequestContext.rs) that checks has_written_status(), reports the error via the VM's error reporter, and terminates the body via force_close() or end_stream() instead of re-rendering the user's error() Response. Two subprocess tests are added covering the with-flush and without-flush shapes.
Security risks
None identified. This is error-path handling for server-side response streaming; no auth, crypto, or untrusted-input parsing is touched. If anything, the fix removes a wire-corruption case where a second header block was spliced into a chunked body.
Level of scrutiny
High. RequestContext is the core per-request state machine for Bun.serve, and both force_close() and end_stream() call self.deref() — the ref balance on this new early-return path needs to match what the caller (do_render_stream at the handle_reject call sites around lines 2119/2240) expects. The new branch also introduces an unsafe const→mut cast on VirtualMachine (following the existing drain_microtasks() pattern) and runs user-observable JS (the VM error reporter) before terminating the connection.
Other factors
The termination logic (is_http_write_called() && is_response_pending() → force_close, else end_stream) exactly mirrors the tail of the async sibling handle_reject_stream (lines ~3088-3095), which is reassuring. However, the sibling only calls vm.run_error_handler under DEBUG_MODE, whereas this new branch reports unconditionally — the tests assert on stderr containing the error in development: false mode, so this is intentional, but it's a small behavior-policy choice a maintainer should confirm. The tests are well-constructed (raw TCP wire inspection, both variants covered, subprocess isolation).
|
On the two points raised: Refcount balance: traced all four Error reporting unconditionally vs DEBUG_MODE-only: |
There was a problem hiding this comment.
I didn't find any issues, but this touches the RequestContext refcount/lifecycle path in Bun.serve with an unsafe VM backref and a new terminal branch — worth a human confirming the ref-balance reasoning.
Extended reasoning...
Overview
This PR adds a ~26-line guard in handle_reject (src/runtime/server/RequestContext.rs) so that when a direct ReadableStream's pull() throws synchronously after do_render_stream has already written the status line, the server reports the error via the VM reporter and terminates the body (force_close() or end_stream()) instead of invoking the user's error() handler and splicing a second header block into the in-flight chunked body. Two subprocess tests in serve-direct-readable-stream.test.ts cover the with-flush and without-flush shapes with wire-level assertions.
Security risks
None identified. The change narrows an existing error path to avoid protocol corruption; no new input parsing, auth, or trust boundaries are introduced. The unsafe const→mut VM cast follows the exact pattern already used in handle_reject_stream and elsewhere in this file.
Level of scrutiny
High. RequestContext is the core per-request lifecycle object for Bun.serve, and the new branch is a terminal path that must consume exactly one base ref (via force_close()/end_stream() → deref()) to match what the old run_error_handler path consumed. The author's follow-up comment traces all four handle_reject callers and the sibling handle_reject_stream tail to argue balance, which looks correct to me — the new tail mirrors lines ~3088-3095 of handle_reject_stream at the same ref state. But refcount reasoning in this file is subtle enough (see the surrounding defer_deinit_until_callback_completes machinery it now bypasses) that a maintainer familiar with the RequestContext lifecycle should confirm.
Other factors
The fix is small, mirrors an established sibling pattern, and comes with strong verification evidence (fails on main in both debug-ASAN and release, passes with fix). The tests are well-constructed subprocess tests with raw-socket wire inspection. No prior human review comments to address. My hesitation is purely about the criticality of the code path and the unsafe/refcount surface, not about any specific concern with the change itself.
|
CI status: the diff itself is green. The new tests in The single failing lane across both #70840 and #70852 is Ready for review. |
2 participants
Reproduction
One request to this server on a debug build aborts the process:
On a release build the
error()Response's header block is written where a chunk-size line belongs, destroying the chunked framing:Without the preceding
write()/flush(), the 500 status is silently dropped and the client receives a clean200 OKcarryingx-err: 1and theerror()body.Cause
do_render_streamwrites the status+headers (render_metadata) before invokingassign_to_stream, which runs the direct stream'spull(). A synchronous throw frompull()is routed tohandle_reject, which gated theerror()re-render only onresp.has_responded()(response ended), never onhas_written_status(). The server'serror()handler was therefore asked to produce a second Response for an exchange whose status line was already on the wire, andrender_metadatawrote a second status/header block into the in-flight body.The async sibling (
handle_reject_stream) already handles this state correctly.Fix
When
has_written_status()is set,handle_rejectnow reports the failure via the VM error reporter and terminates the body (force_closeif body bytes were written,end_streamotherwise) instead of re-rendering, matchinghandle_reject_stream.Verification
Added two subprocess tests in
test/js/bun/http/serve-direct-readable-stream.test.tscovering the with-flush and without-flush shapes. Both fail on the unfixed build (debug:!has_written_statusassert; release:error()headers spliced into the wire) and pass with the fix.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file