Skip to content

Bun.serve: report a native body error that arrives after the status was sent - #39442

Closed
robobun wants to merge 5 commits into
mainfrom
farm/c35cefe5/serve-report-native-body-error
Closed

robobun wants to merge 5 commits into
mainfrom
farm/c35cefe5/serve-report-native-body-error

Conversation

@robobun

@robobun robobun commented Aug 17, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • A Bun.serve handler returns rewriter.transform(response) and a content handler throws (or its returned promise rejects) after the rewriter has already emitted output. The client gets HTTP/1.1 200 OK with a truncated body, error() is not called, and nothing is printed, in either development mode. The exception is gone.
  • The same handler failing before the first output byte is routed to error() and answered with a 500 (since HTMLRewriter: stream through SinkHandle/SourceHandle; suspend async handlers instead of wait_for_promise #36733), so only the after-first-byte case is silent. A plain new Response(readableStream) whose stream errors mid-body also commits the 200, but prints the error in development mode.
  • Cause: src/runtime/server/RequestContext.rs, end_chunk. This is where a natively streamed body (Source::Bytes: an HTMLRewriter output, a fetch() Response returned as-is) delivers its terminal error. With no status written it converts the error and runs error(); with the status already written it only called force_close() and never looked at err. The JS stream twin, handle_reject_stream, reports the error through vm.run_error_handler in development mode before force-closing.
  • Reproduces on the 1.4 canary in this container (8326d1b); the code on main is the same. 1.3 buffered the whole rewrite before handing it to the server, so the error surfaced there, which is why this is new in 1.4.

Fix

  • RequestContext had three copies of "the body failed after the status went out": handle_reject (a direct stream's pull() throwing synchronously during render, Bun.serve: do not re-render error() after the status line is committed #33810: reports in both modes), handle_reject_stream (the pump's promise rejecting later: reports in development mode, plus the bake dev server error page), and end_chunk (native producers: reported nothing). This PR extracts the development-mode report into report_committed_body_error and the force-close-or-end decision into close_failed_body; handle_reject_stream and end_chunk use both, handle_reject uses close_failed_body and keeps its unconditional report. The is_http_write_called() && decision now exists once instead of three times.
  • Why end_chunk gets the handle_reject_stream policy: the errors it receives arrive from the body after the handler's Response was rendered, which is the situation handle_reject_stream handles for JS bodies; the two sync sites are a different moment (the handler is still being rendered). Whether production mode should also report these late failures applies to the JS and native paths alike and is not decided here; with the shared helper it becomes a one-place change (Bun.serve: route Response body stream errors to error() until the first byte is written #35229 proposes it inside a larger JS stream rework and has been conflicting since July; it would rebase onto these helpers). The new production-mode tests therefore assert only the wire, like the existing mid-stream-reject test. The default development: true reports.
  • Behavior of the pre-first-byte branch of end_chunk, of the JS stream path, and of handle_reject is unchanged. Two things dropped while moving the block were dead: the if let Some(server) (is_aborted_or_ended() just returned false, and it checks the same field) and the 500 fallback inside the dev server branch (render_metadata() runs unconditionally right before it whenever resp is still writable, so the status is always written there). In production mode end_chunk does not build the JS error at all.
  • Same branch, other producer: a proxied fetch() body whose upstream dies after the status is committed now prints the fetch ECONNRESET TypeError in development mode, whether or not a body byte went out. The wire is unchanged in both cases (force-close after bytes, 0\r\n\r\n with none; the latter is also what ByteStream: report a stored producer error and keep the taken prefix with the buffer #38003's after-attach test pins).
  • Out of scope, same family: new Response(Bun.file(...)) bodies report a read/sendfile failure through on_file_stream_error (likewise FileRoute and DirectoryRoute), which discards a bun_sys::Error after the stream has already closed the socket itself. Reporting there needs an errno split first (a peer reset during sendfile is not worth printing), so it is a separate change.
  • Tests: test/js/bun/http/serve-stream-body-error.test.ts gains four fixture variants (rewriter handler throwing synchronously, rewriter async handler rejecting, proxied upstream dying after chunk-a reached the client, proxied upstream dying after only the status reached the client), each run with development: true (wire plus the stderr report) and development: false (wire), plus two rows under a bake dev server (the fixture gets an HTML route, which is what makes Bun.serve run one): the native body now gets the dev error page appended and a normal end, with the JS stream as the control that always behaved that way. The proxied variants use a per-request raw upstream that announces more body than it sends, the shape ByteStream: report a stored producer error and keep the taken prefix with the buffer #38003 adds to the same fixture, so the two sets of variants fold into one table whichever lands second.
    • Unfixed bun (main's RequestContext.rs built into this tree): the four development-mode tests fail with Received: "" for stderr while the wire assertions already hold; the native dev server row fails on all of terminator, page and stderr (the body is force-closed after chunk-a, which also confirms dev server requests reach end_chunk); the production-mode tests and the JS dev server row pass.
    • Fixed debug build: 25/25 in that file; serve-direct-readable-stream.test.ts (its two "sync pull() throw after status is written" tests cover the handle_reject change), serve-stream-reject-flush-leak.test.ts, async-iterator-stream.test.ts, serve-error-handler-stream.test.ts and test/js/workerd/html-rewriter.test.js (169) pass; serve.test.ts and bun-server.test.ts show the same IPv6/root-only failures as the unmodified binary in this container.
    • The script from the report prints error: boom for both after-first-byte cases under the fix and stays quiet with development: false, matching its JS stream control case both ways.

Background

  • Bun.serve streams a Response body in one of two ways. A JS ReadableStream is pumped by JS into a ResponseStream sink; the pump's promise settles into handle_resolve_stream / handle_reject_stream, except that a source throwing synchronously inside the first render lands in handle_reject. A body that is already native (a ByteStream: what fetch() produces, and what an HTMLRewriter transform writes its output into) is attached directly with SinkHandle::ServerResponse; the ByteStream then calls write_chunk per chunk and end_chunk(err) once, with err set when the producer failed.
  • has_written_status records that the status line and headers were handed to uWS. For rewriter output this is deferred until the first output byte, precisely so that an earlier failure can still reach error(); for other native bodies it is set when the body is attached.
  • RequestContext is monomorphized on DEBUG_MODE, which is development: true (the default unless NODE_ENV=production or development: false). vm.run_error_handler is the printer used for uncaught errors; it formats the value to stderr and nothing else. The dev server error page branch only applies when the server runs a bake dev server.
  • Force-closing a chunked response without its 0\r\n\r\n terminator is how the client is told the body is incomplete (RFC 9112 section 7); that part predates this change.
Output of the report's script on this branch
# development (default)
leading=0&async=0 -> "HTTP/1.1 500 Internal Server Error" body: "from error(): boom" error(): called
leading=0&async=1 -> "HTTP/1.1 500 Internal Server Error" body: "from error(): boom" error(): called
error: boom
      at <anonymous> (rewriter-serve.ts:50:25)
leading=1&async=0 -> "HTTP/1.1 200 OK" body: "" error(): NOT called
error: boom
      at <anonymous> (rewriter-serve.ts:47:25)
leading=1&async=1 -> "HTTP/1.1 200 OK" body: "c\r\n<html><body>\r\n" error(): NOT called
error: js-boom            # control: plain ReadableStream body, unchanged
js=1 -> "HTTP/1.1 200 OK" body: "c\r\n<html><body>\r\n" error(): NOT called

# development: false
(same four wire results, nothing printed for either the rewriter or the control case)

Before this change the two leading=1 lines printed nothing in either mode.

First revision of this PR

The first push used the two helpers from handle_reject_stream and end_chunk only, described development-mode-only reporting as a contract settled by #32842 (that PR preserved the production silence of the async path rather than deciding it; #33810 later chose to report in both modes on the sync path), and pinned stderr empty in the production-mode tests. The current revision routes handle_reject through close_failed_body as well, states the actual split above, asserts only the wire in production mode, and adds the zero-body-bytes variant; a later push added the dev server rows.


[review] gate passed · iteration 1 · 4 files touched

fails on main (without fix)
ASAN without fix: 5 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/http/serve-stream-body-error.test.ts
bun test v1.4.0 (8326d1bd3)

test/js/bun/http/serve-stream-body-error.test.ts:
(pass) controller-error: the rejection is handled and the error is still reported [1342.12ms]
(pass) pull-throw: the rejection is handled and the error is still reported [1403.80ms]
(pass) pull-async-reject: the rejection is handled and the error is still reported [1374.95ms]
(pass) start-async-reject: the rejection is handled and the error is still reported [1374.27ms]
(pass) deferred-pull-throw: the rejection is handled and the error is still reported [1381.81ms]
(pass) pull-throw in development mode: the rejection is handled [1322.65ms]
(pass) mid-stream error: the chunked body is not terminated as complete [1310.78ms]
(pass) mid-stream error in development mode: reported and not terminated as complete [1314.23ms]
151 |         unhandled: 0,
152 |         secondStatusLine: "HTTP/1.1 200 OK",
153 |       },
154 |       exitCode: 0,
155 |     });
156 |     expect(stderr).toContain(reported);
                 
... (truncated)

release without fix: 1 skipped
bun test v1.4.0-canary.1 (674e3e803)

test/js/bun/http/serve-stream-body-error.test.ts:
(pass) start-async-reject: the rejection is handled and the error is still reported [36.76ms]
(pass) mid-stream error: the chunked body is not terminated as complete [35.02ms]
(pass) pull-throw: the rejection is handled and the error is still reported [39.05ms]
(pass) deferred-pull-throw: the rejection is handled and the error is still reported [37.68ms]
(pass) pull-throw in development mode: the rejection is handled [41.56ms]
(pass) pull-async-reject: the rejection is handled and the error is still reported [43.76ms]
(pass) controller-error: the rejection is handled and the error is still reported [43.67ms]
(pass) rewriter-mid-stream-throw in development mode: reported, and the body is terminated [40.48ms]
(pass) rewriter-mid-stream-reject: the body is terminated [44.41ms]
(pass) mid-stream error in development mode: reported and not terminated as complete [64.09ms]
(pass) a stream body error does not kill the server process [29.52ms]
(pass) mid-stream-reject under a dev server: the error page is appended to the body [38.09ms]
(pass) rewriter-mid-stream-reject in development mod
... (truncated)
passes on PR (with fix)
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/http/serve-stream-body-error.test.ts
bun test v1.4.0 (8326d1bd3)

test/js/bun/http/serve-stream-body-error.test.ts:
(pass) start-async-reject: the rejection is handled and the error is still reported [1338.64ms]
(pass) deferred-pull-throw: the rejection is handled and the error is still reported [1351.54ms]
(pass) pull-throw: the rejection is handled and the error is still reported [1413.35ms]
(pass) controller-error: the rejection is handled and the error is still reported [1378.21ms]
(pass) pull-async-reject: the rejection is handled and the error is still reported [1395.03ms]
(pass) mid-stream error in development mode: reported and not terminated as complete [1300.92ms]
(pass) mid-stream error: the chunked body is not terminated as complete [1316.07ms]
(pass) pull-throw in development mode: the rejection is handled [1340.84ms]
(pass) rewriter-mid-stream-throw in development mode: reported, and the body is terminated [1318.24ms]
(pass) rewriter-mid-stream-reject in development mode: reported, and the body is terminated [
... (truncated)

release with fix: 1 skipped
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped)
  target       linux-x64-gnu
  build type   Release
  build dir    ./build/release
  revision     a706dc726a
  features     baseline

22 deps, 120 codegen, 1174 objects in 663ms

ninja: Entering directory `/workspace/bun/build/release'
[1/8] cxx obj/unified/UnifiedSource-src_jsc_bindings-5.cpp.o
[2/8] gen generated_host_exports.rs
generated_host_exports.rs: 92 exports (host=3, lazy=10, generic=79, rust=0); 241 extern-C blocks audited
[3/8] gen cpp.rs (cppbind)
[3/8] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)

  nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19)

�[1m�[92m   Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core)
�[1m�[92m   Compiling�[0m bun_alloc v0.0.0 (/workspace/bun/src/bun_alloc)
�[1m�[92m   Compiling�[0m bun_libdeflate_sys v0.0.0 (/workspace/bun/src/libdeflate_sys)
�[1m�[92m   Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno)
�[1m�[92m   Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr)
�[1m�[92m   Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys)

... (truncated)
diff hotspot
src/runtime/server/RequestContext.rs               | 114 +++++++++---------
 .../bun/http/serve-stream-body-error-fixture.html  |   3 +
 .../js/bun/http/serve-stream-body-error-fixture.ts | 132 +++++++++++++++++++--
 test/js/bun/http/serve-stream-body-error.test.ts   |  85 +++++++++++++
 4 files changed, 260 insertions(+), 74 deletions(-)

gate history · 2 passed · 1 rejected · iteration 1

evidence per changed file
file                                                   reads  edits  tests
src/runtime/server/RequestContext.rs                      19     13      0
test/js/bun/http/serve-stream-body-error-fixture.html      0      1      0
test/js/bun/http/serve-stream-body-error-fixture.ts        4     12      0
test/js/bun/http/serve-stream-body-error.test.ts           3      5      0

@coderabbitai

coderabbitai Bot commented Aug 17, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 23592bd9-8265-4d83-bc0b-d674f129d288

📥 Commits

Reviewing files that changed from the base of the PR and between ff5fc1f and dc47777.

📒 Files selected for processing (3)
  • src/runtime/server/RequestContext.rs
  • test/js/bun/http/serve-stream-body-error-fixture.ts
  • test/js/bun/http/serve-stream-body-error.test.ts

Included review availability: Your plan includes up to 5 reviews per rolling hour; 0 remain after this review.


Walkthrough

Changes

Stream body error handling

Layer / File(s) Summary
Shared server error paths
src/runtime/server/RequestContext.rs
Pre-status producer failures use run_error_handler. Committed failures use shared reporting and failed-body closure logic.
Native body streaming fixtures
test/js/bun/http/serve-stream-body-error-fixture.ts
The fixture adds client timing coordination and HTMLRewriter and proxied-fetch failure variants for native streamed bodies.
Native body error coverage
test/js/bun/http/serve-stream-body-error.test.ts
Development and production tests verify error reporting, response termination, status preservation, unhandled rejection behavior, and server availability.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely identifies the main change: reporting native body errors after the response status was sent.
Description check ✅ Passed The description explains the problem, fix, scope, behavior, and verification results, although it does not use the template headings exactly.

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 17, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: fix and tests ready for review at head a706dc7 (rebased onto main; the last push only adds the dev server test rows). Waiting on CI.

Reproduced with the script from the report on the 1.4 canary in this container (8326d1b): an HTMLRewriter handler failing after the first output byte returned a truncated 200 with nothing on stderr, while the same script's plain ReadableStream control case printed its error. The four development-mode variants added to test/js/bun/http/serve-stream-body-error.test.ts fail the same way on that binary (stderr empty, wire assertions already holding), the native dev server row fails on it too (force-closed, no error page) and pass on this branch, together with the rest of that file, serve-direct-readable-stream.test.ts (covers the handle_reject change), serve-stream-reject-flush-leak.test.ts, async-iterator-stream.test.ts, serve-error-handler-stream.test.ts and test/js/workerd/html-rewriter.test.js.

CI so far: build 100020 failed only in test/cli/install git-dependency tests (GitHub 429s); build 100279 failed only in test/bake/deinitialization.test.ts (Windows 2019 teardown segfault, seen identically on main build 99388). Both reported to triage; neither area is touched here, and the new tests passed on every lane in both runs.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. Because it refactors error-path control flow in RequestContext::handle_reject_stream and end_chunk — a hot, lifetime-sensitive part of Bun.serve — a human look would still be worthwhile.

What was reviewed:

  • Traced the handle_reject_stream refactor: the extracted report_committed_body_error + close_failed_body preserve the original control flow; the dropped if let Some(server) is dead (checked by is_aborted_or_ended at line 2871/3226) and the dropped 500-status write is dead (render_metadata at 2880 always runs first when resp_writable).
  • Verified self.server() unwrap is safe at both call sites since is_aborted_or_ended() returned false just above.
  • Confirmed the DEBUG_MODE && guard in end_chunk avoids building err.to_js() in production, matching the PR description.
  • Checked the test fixture's midStreamResolve sequencing across the two sequential rawRequest calls and the lazy upstream server — no races found.
Extended reasoning...

Overview

This PR makes Bun.serve report native-body errors (HTMLRewriter transform output, proxied fetch() bodies) that arrive after the HTTP status has already been committed. Previously end_chunk only force_close()d and dropped the error; now it routes through the same development-mode reporting that handle_reject_stream uses for JS ReadableStream bodies. The shared logic is extracted into two helpers, report_committed_body_error and close_failed_body, and both call sites now use them. Six new test cases (three native-body variants × dev/prod) are added to the existing serve-stream-body-error.test.ts fixture harness.

Security risks

None identified. The change is confined to error reporting and connection teardown after a response body fails; no new input parsing, no auth/permission logic, no allocation on untrusted sizes. The dev-server error page write already existed on the JS-stream path.

Level of scrutiny

High. RequestContext.rs is the per-request state machine for Bun.serve, where the resp uWS handle has strict liveness rules (see the ended_response / end_already_responded_stream comments) and getting a close/write ordering wrong is a UAF. The refactor changes control flow on error paths in both handle_reject_stream and end_chunk, and makes the bake dev-server error-page write newly reachable from end_chunk (appending HTML after a partially-written rewriter/fetch body). I traced the refactor as behavior-preserving for the JS-stream path and verified the two dropped guards are dead, but a maintainer familiar with the resp lifetime invariants and the bake dev-server path should confirm.

Other factors

  • Tests are well-constructed: they await the observable condition (chunk-a reaching the client socket) before triggering the error, cover sync throw / async reject / upstream-drop, assert the exact wire bytes and stderr in both development modes, and reuse the existing subprocess fixture pattern with test.concurrent.each.
  • The PR description documents that the three development-mode tests fail on unfixed bun and all 21 pass on the fixed build, plus adjacent suites (serve-direct-readable-stream, html-rewriter) pass.
  • No prior human reviews or outstanding comments on the timeline.

@robobun

robobun commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator Author

On the one point flagged for a human look, the dev server error page becoming reachable from end_chunk: that is intended. It is the same branch a JS ReadableStream body has taken in handle_reject_stream since the beginning (appending the page to whatever part of the body already went out, then ending the response), and it only applies when the server runs a bake dev server, which a plain Bun.serve never does. The new tests exercise the plain server in both development modes; the bake branch has no coverage before or after this change, and the diff moves it without altering it. The resp liveness rules are unchanged as well: end_chunk runs after is_aborted_or_ended() and nothing on the native path ends the uWS response before it, so it passes resp_writable = true; handle_reject_stream keeps passing !ended_response for the sink-already-ended case.

Comment thread src/runtime/server/RequestContext.rs Resolved
Comment thread src/runtime/server/RequestContext.rs Resolved
Comment thread src/runtime/server/RequestContext.rs Resolved
Comment thread src/runtime/server/RequestContext.rs Resolved
Comment thread src/runtime/server/RequestContext.rs Resolved
Comment thread src/runtime/server/RequestContext.rs Resolved
Comment thread src/runtime/server/RequestContext.rs Resolved
Comment thread src/runtime/server/RequestContext.rs Resolved
@robobun

robobun commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed a revision (c28520c, plus two comment-only commits) after a self-review of the first push; the PR description now describes this state.

  • handle_reject had a third inline copy of the force-close-or-end decision; it now calls close_failed_body too. Its report stays unconditional (Bun.serve: do not re-render error() after the status line is committed #33810 pins that with development: false), so the description no longer presents development-mode-only reporting as a settled contract: it is what the two late-failure sites (handle_reject_stream, now also end_chunk) do, and whether production should report there as well is left as it was, now changeable in one place. The new production-mode tests assert only the wire for the same reason, like the existing mid-stream-reject test.
  • One more variant: the proxied upstream dying after only the status reached the client (status committed, zero body bytes), which takes the end_stream side of close_failed_body and is reported in development mode as well. The proxied variants now use a per-request raw upstream of the same shape ByteStream: report a stored producer error and keep the taken prefix with the buffer #38003 adds to this fixture.
  • on_file_stream_error (file bodies) drops its error in the same way but needs an errno split first; listed in the description as a separate follow-up.

The automated review limit notices above and the comment-length bot threads (addressed by trimming, two kept as the new functions' doc comments) need nothing further.

@robobun

robobun commented Aug 18, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 8:39 PM PT - Aug 17th, 2026

✅ @robobun, your commit a706dc726a9e2befec131837f99fd871c6ef6f9b passed in Build #100322! 🎉


🧪   To try this PR locally:

bunx bun-pr 39442

That installs a local version of the PR into your bun-39442 executable, so you can run:

bun-39442 --bun

Comment thread src/jsc/bindings/DOMURLBaseCache.h Resolved
@robobun
robobun force-pushed the farm/c35cefe5/serve-report-native-body-error branch from 276e8ad to dc47777 Compare August 18, 2026 02:15
@robobun

robobun commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

Correction: the ci: retrigger push at 276e8ad was meant to be empty but picked up four files that were sitting staged in the working copy (ASCIIHostPunycodeCheck.h, DOMURLBaseCache.h, NodeVMRunTermination.{cpp,h}, all identical to what is already on main from #39468 and the worker teardown work). They have nothing to do with this change. The branch was rewritten to dc47777 (an actually empty commit on top of fa778db), so the PR is back to the same three files and the same diff as fa778db; the bot threads on those files are resolved.

The retrigger itself is for build 100279, whose only non-flaky failure was test/bake/deinitialization.test.ts segfaulting on Windows 2019 x64 at dev server teardown, which main build 99388 hit identically two days ago (reported to triage); this PR does not touch bake, and the new tests passed on every lane in both runs so far.

…as sent

A ByteStream-backed Response body (an HTMLRewriter transform, a proxied
fetch() Response) that fails after its status and headers are on the wire
reaches RequestContext::end_chunk, which force-closed the connection and
dropped the error: a rewriter handler throwing after the first output byte
left nothing on stderr and never reached error().

Move the development-mode report out of handle_reject_stream into a helper
shared with end_chunk, so a native body failing mid-stream is reported
(and, under a bake dev server, rendered) the same way a JS ReadableStream
body is. Production mode stays quiet on both paths, as before.
handle_reject's committed-status branch carried its own copy of the
force-close-or-end decision; route it through close_failed_body (its
unconditional report is unchanged) and document which committed-status
failures are reported in which mode.

The fixture's proxied variants now use a per-request upstream that announces
more body than it sends, which also gives the variant where the upstream dies
after the status is committed but before any body byte. The production-mode
rows assert the wire only, like the existing mid-stream test.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this and didn't find any bugs. Because it refactors three error-completion paths in RequestContext (the Bun.serve response lifecycle) and drops code as dead along the way, a maintainer look at the shape of the shared helpers and the bake dev-server branch now reachable from end_chunk would still be worthwhile.

What was reviewed:

  • Verified the removed 500 fallback in the dev-server branch is unreachable: render_metadata() runs first whenever !ended_response, and it always calls do_write_status when resp is Some (guaranteed after is_aborted_or_ended()).
  • Verified self.server() is safe at both new call sites: both run after is_aborted_or_ended() returned false, which checks server.get().is_none().
  • Verified close_failed_body() preserves the old force-close/end-stream decision at all three call sites, and that the DEBUG_MODE && guard in end_chunk avoids building the JS error in production.
  • Tests: subprocess pipes drained concurrently, port: 0, local upstreams only, mid-stream variants await the chunk reaching the client rather than sleeping.
Extended reasoning...

Overview

Three-file change: src/runtime/server/RequestContext.rs extracts two helpers (report_committed_body_error, close_failed_body) from the inline block in handle_reject_stream and routes handle_reject and end_chunk through them; end_chunk gains a development-mode run_error_handler call it previously lacked. The test file and fixture add four native-body variants (HTMLRewriter throw/reject, proxied fetch mid-stream/after-status) exercised in both development modes.

Security risks

None identified. This is error-reporting plumbing on an already-failing response; no new input parsing, no auth/crypto, no changes to what reaches the client on the wire (the force-close/end-stream decision is byte-identical to before).

Level of scrutiny

High. RequestContext is the per-request state machine for Bun.serve, and the three touched sites (handle_reject, handle_reject_stream, end_chunk) are exactly where response lifetime, uWS resp liveness, and force-close semantics are decided. The refactor is small but consolidates control flow that previously diverged, drops two pieces of code as dead (the 500-status fallback and the if let Some(server) guard), and makes the bake dev-server error page reachable from end_chunk for the first time. Each of those is well-argued in the PR description and I verified them against the source, but this is the kind of change where a maintainer familiar with the resp liveness rules and the bake dev-server contract should confirm the shape.

Other factors

  • The PR author self-flagged the dev-server-page-from-end_chunk reachability as the one point for a human look, then explained why it's intended (matches the JS-stream twin, only fires under a bake dev server which plain Bun.serve never has, no coverage before or after).
  • Test quality is high: fixture-driven subprocess tests with test.concurrent, wire-level assertions on chunked framing, both development monomorphizations, a zero-body-bytes variant covering the end_stream side of close_failed_body. Production-mode tests intentionally leave stderr unasserted (matching the existing mid-stream-reject test) rather than pinning a policy the PR does not decide.
  • CI history: two prior builds green on the touched tests across all lanes; unrelated flakes (GitHub 429s in install tests, a Windows bake teardown segfault also on main) reported to triage. Build 100286 pending at time of review.
  • All comment-cop threads on the touched file are resolved (comments trimmed in c40afe7/fa778dbb); the remaining doc comments on the two new functions are appropriate. The stray unrelated files from the accidental 276e8ad commit were removed by the branch rewrite.

@robobun
robobun force-pushed the farm/c35cefe5/serve-report-native-body-error branch from dc47777 to 674e3e8 Compare August 18, 2026 02:35
@robobun

robobun commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main (head is now 674e3e8; the empty retrigger commit is gone). The diff against main is byte-for-byte the one that was at fa778db; the rebase was needed because main gained files under src/jsc/bindings that this branch's old base lacked. Same suites re-run on the rebased debug build: serve-stream-body-error (23), serve-direct-readable-stream, serve-stream-reject-flush-leak, async-iterator-stream, serve-error-handler-stream, html-rewriter all pass. Nothing to act on in the two most recent bot reviews.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. Because it refactors error-path logic across three RequestContext sites and makes the bake dev server error page newly reachable from end_chunk (a path with no coverage per the author's own note), a human look would still be worthwhile.

What was reviewed:

  • self.server() replacing if let Some(server) — safe: both callers pass through is_aborted_or_ended() first, which checks server.get().is_none().
  • Dropped 500-status fallback in the dev-server branch — dead: render_metadata() runs first whenever resp_writable, and end_chunk only enters with has_written_status() already true.
  • close_failed_body() vs the three inlined originals — same force-close/end-stream decision on every path, including the end_chunk fall-through when no body bytes went out.
  • The DEBUG_MODE && guard in end_chunk avoids building err.to_js() in production even though the helper re-checks the flag.
Extended reasoning...

Overview

This PR fixes silent error swallowing in Bun.serve when a natively-streamed body (HTMLRewriter output, proxied fetch() response) fails after the HTTP status has already been committed. Previously RequestContext::end_chunk force-closed the connection but never looked at the error; now it reports through vm.run_error_handler in development mode, matching what handle_reject_stream already does for JS ReadableStream bodies. The fix extracts two shared helpers (report_committed_body_error, close_failed_body) from three copy-pasted sites in handle_reject, handle_reject_stream, and end_chunk. Eight new test variants (four native-body scenarios × two development modes) are added to the existing serve-stream-body-error test file and fixture.

Security risks

None identified. The change surfaces errors that were previously dropped; it does not introduce new user-controlled input paths, does not touch auth/TLS/crypto, and does not change what goes on the wire (the wire assertions in the new production-mode tests already held on the unfixed binary per the PR description).

Level of scrutiny

High. RequestContext.rs is the core per-request state machine for Bun.serve, and this change touches error-termination logic on three separate paths. The refactor removes two pieces of code claimed dead (the if let Some(server) guard and the 500-status fallback in the dev-server branch); I verified both claims against is_aborted_or_ended() and the render_metadata() call ordering, and they hold. However, the bake dev server error page is now reachable from end_chunk for the first time — the author acknowledges this branch "has no coverage before or after this change" and only applies when a bake dev server is running, which the new tests do not exercise.

Other factors

The PR description is unusually thorough, the tests follow the harness conventions (subprocess isolation, test.concurrent, deterministic wire-level sync via chunkReachedClient() rather than sleeps), and CI failures on prior builds were unrelated flakes per the timeline. The comment-cop bot threads have been resolved by trimming. The remaining reason to defer is scope: a three-site error-path refactor in the HTTP server with a newly-reachable branch is the kind of change a maintainer familiar with the resp liveness rules and the bake dev server integration should sign off on, even though I found nothing wrong with it.

…tatus

With an HTML route the development server runs a bake dev server, and a body
failing after the status is committed gets the dev error page appended and
the response ended normally. The JS stream always did; the native body now
takes the same branch, so pin both.
@robobun

robobun commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

The dev server branch that the reviews kept pointing at is now covered (a706dc7). The fixture takes a dev-server flag that gives the server an HTML route (serve-stream-body-error-fixture.html), which is what makes a development Bun.serve run a bake dev server; the requests still go to fetch(). Two rows: the rewriter body failing after chunk-a (the branch this PR makes reachable for native bodies) and the JS mid-stream-reject stream as the control. Both now produce chunk-a, then the dev error page naming boom and "Stream error during server-side rendering", then a normal 0\r\n\r\n, with the error on stderr and error() not invoked.

Checked against a build of this tree with main's RequestContext.rs: the native row fails there (the body is force-closed right after chunk-a, no page, nothing on stderr), which also confirms that requests under a dev server do reach end_chunk rather than some other path; the JS row passes on both, as a control should. The file is 25/25 on the fixed debug build. The description's test bullets are updated; nothing to act on in the rate limit notice above.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. Because it refactors the committed-body error paths in RequestContext.rs (three call sites now sharing two new helpers, with dead-code removal and a new dev-server-error-page path reachable from end_chunk), a human look would still be worthwhile.

What was reviewed:

  • Verified self.server() is safe at both new call sites (guarded by is_aborted_or_ended() at RequestContext.rs:2865 and :3211).
  • Verified the removed 500 fallback in the dev-server branch was dead: render_metadata() at :2873 always writes the status before report_committed_body_error runs with resp_writable = true.
  • Verified close_failed_body() preserves the exact force-close/end-stream decision at all three former inline sites, and that the else if → early-return + if restructure in end_chunk is equivalent.
  • Checked the DEBUG_MODE && guard in end_chunk avoids building the JS error in production; the helper's own !DEBUG_MODE check makes it redundant but not wrong.
Extended reasoning...

Overview

The PR fixes a silent-error case in Bun.serve: when a natively streamed body (an HTMLRewriter transform or a proxied fetch() Response) fails after the status line has been committed, end_chunk previously force-closed the connection without ever looking at the error. The fix extracts two helpers from the three existing "body failed after status went out" sites — report_committed_body_error (development-mode stderr report + optional bake dev-server error page) and close_failed_body (the force-close-vs-end-stream decision) — and routes handle_reject, handle_reject_stream, and end_chunk through them. Net effect: end_chunk now reports the error in development mode and can append the bake dev-server error page, matching what handle_reject_stream already did for JS-pumped bodies. Test coverage is thorough: 10 new fixture-driven cases (four native-body variants × two development modes, plus two dev-server rows) added to the existing serve-stream-body-error suite.

Security risks

None identified. The change is confined to how an already-failed response body is reported and terminated; no new input parsing, no auth/crypto, no new user-controlled data reaching native code. The dev-server error page renders an exception list into the response body, but that path already existed for JS streams and only fires under an explicit bake dev server.

Level of scrutiny

High. RequestContext.rs is the core per-request state machine for Bun.serve, and the refactor touches three error-path exits with subtle liveness invariants (resp writability after a sink may have ended it, self.server presence after is_aborted_or_ended()). The PR removes two pieces of code as dead (the 500-status fallback inside the dev-server branch and the if let Some(server) guard) and asserts resp_writable = true unconditionally for end_chunk. I traced each of these and they check out — is_aborted_or_ended() at both entry points guarantees server is Some, and render_metadata() at line 2873 always writes the status before the dev-server branch could fire — but this is exactly the kind of invariant reasoning that benefits from a maintainer who knows the H1/H3 sink lifecycle confirming the resp_writable = true claim for the native ByteStream path.

Other factors

The tests are well-constructed (raw-socket wire assertions, event-driven synchronization via chunkReachedClient() rather than sleeps, per-request scratch upstreams for the proxied variants), and the PR description documents that the four development-mode tests fail on an unfixed build. CI has been green on the touched suites across two builds, with the only failures being unrelated flakes reported to triage. The comment-cop bot threads were addressed (helper doc comments trimmed to a few lines each). The one thing I'd want a human to confirm is the newly reachable dev-server-error-page path from end_chunk — the head commit added coverage for it, but it's a fresh behavior for native bodies that a maintainer should sanity-check.

@robobun

robobun commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator Author

Closing in favor of #40596, which covers the same three sites in RequestContext.rs and now carries the parts of this PR that were not already there:

  • report_committed_body_error, shared by handle_reject_stream and end_chunk, including the bake dev server error page for native bodies (with the dead 500 status branch removed, as here).
  • The rewriter-mid-stream-throw, rewriter-mid-stream-reject, proxied-fetch-mid-stream and proxied-fetch-after-status fixture variants, the dev server rows, and the HTML fixture.

Two things differ in #40596. end_chunk reports in both modes, not only in development mode, and a native body that fails before its first byte is force-closed instead of ended with 0\r\n\r\n. The test expectations were adjusted for that.

@robobun

robobun commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator Author

Update: a review of #40596 found that the framing change it carried needs a maintainer decision on its own, so #40596 was reduced to the shape of this PR (the shared report helper, the dev error page for native bodies, and the fixture variants from here, rebased onto current main). The framing change and the production-mode report for native bodies are now #40648, stacked on #40596. This PR stays closed; its content lives in #40596.

Jarred-Sumner pushed a commit that referenced this pull request Aug 27, 2026
…#40596)

### Problem
- A `Bun.serve` Response whose body stream fails before any body byte is
written is delivered to the client as a complete message: the Response's
status and headers, `Transfer-Encoding: chunked`, an empty body and a
clean `0\r\n\r\n`. `curl` exits 0. A proxied upstream body that dies
before its first chunk is forwarded the same way, and nothing is
reported. Only a failure after the first body byte closed the connection
without the terminator (#32842).
- Cause: three sites in `src/runtime/server/RequestContext.rs` handle a
body failure after the status line is committed (`handle_reject` for a
direct stream's synchronous `pull()` throw, `handle_reject_stream` for a
JS stream, `end_chunk` for a native byte stream). Each force-closed only
when `is_http_write_called()` and otherwise called `end_stream()`, which
writes the terminating chunk.

### Fix
- The three sites share a new `close_incomplete_stream`: while the
response is still pending, `force_close()` the connection, whether or
not body bytes went out first. Once the sink has already ended the
response, `end_stream()` only releases the context, as before.
- Correct because a chunked message is complete only with its terminator
(RFC 9112 section 7). An empty chunked body with a terminator is as
complete as a truncated one. Closing without it is the only way HTTP/1.1
can signal an incomplete message. When the status is still in the cork
buffer, the close discards it and the client sees an empty reply, the
same as Node's `res.destroy()` before the headers flush.
- `end_chunk` (HTMLRewriter output, a proxied `fetch()` body) no longer
drops the producer's error: it reports it in both modes through the
shared `report_committed_body_error`, which also gives native bodies the
bake dev server error page that JS streams already had. This carries the
remaining parts of #39442, closed in favor of this PR.
- Verified: `test/js/bun/http/serve-stream-body-error.test.ts` (JS
stream variants, pending error after the headers, HTMLRewriter and
proxied fetch bodies in both modes, two dev server rows; 17 of 26 fail
on stock bun). Also `serve.test.ts`,
`serve-direct-readable-stream.test.ts`, `async-iterator-stream.test.ts`,
`serve-http3.test.ts`, `serve-error-handler-stream.test.ts`,
`serve-stream-reject-flush-leak.test.ts`, `text-encoder-stream.test.ts`,
`html-rewriter.test.js`.

### Background
- `RequestContext` is the per-request state of `Bun.serve`.
`do_render_stream` writes the Response's status and headers into the
corked uWS response before it attaches the body stream, so by the time a
body error arrives `has_written_status()` is already true and `error()`
cannot supply a replacement. That contract is unchanged here: `error()`
is still not called once the status is committed.
- uWS corks a socket while a request handler runs: writes go to a
per-socket buffer that is flushed when the handler returns.
`force_close()` closes the socket directly and drops that buffer, so a
synchronous failure leaves nothing on the wire. An asynchronous failure
arrives after the flush, so the client gets the headers and then a
reset.
- Behavior change: the tests that pinned the old contract ("throw on
pull renders headers", "async generator ... continues to send the
headers", the pre-first-byte variants in
`serve-stream-body-error.test.ts`) now expect the connection to close
without a complete response.

<details><summary>Notes</summary>

Wire before the fix, for `new Response(new ReadableStream({ start(c) {
c.enqueue(chunk); c.error(new Error("boom")); } }))`:

```
HTTP/1.1 200 OK
Content-Type: text/plain;charset=utf-8
Date: ...
Transfer-Encoding: chunked

0
```

After: the connection closes with 0 bytes sent (the status was still
corked). For a stream whose first `pull()` is asynchronous, the headers
were already flushed, so the client gets `HTTP/1.1 200 OK` plus headers
and then a connection reset with no terminating chunk. Both are
incomplete messages. The error reaches stderr through the existing
reporters; the production-mode asynchronous JS path stays quiet, as
today.

The forced close sends a RST, and a RST discards data the peer has not
read yet (always on Windows). The tests that expect the status line on
the wire before the failure therefore trigger the failure from the
client's data handler, once the status line has arrived. uWS terminates
the header block only with the first body byte, so those tests wait for
the status line, not for a blank line.

Dev server exception: under a bake dev server (`development: true` plus
an HTML route), a body that fails after the status is committed gets the
dev error page appended and the response ends normally, so the browser
shows the error after what was already streamed. JS streams always did
this; native bodies now take the same branch.

Probed sources, all now closed incomplete: `controller.error` in
`start`, synchronous and asynchronous `pull()` throw, async generator
throw, `Readable.toWeb` of an erroring node stream, a throwing
`TransformStream`, `DecompressionStream` on bad bytes, a `type:
"direct"` stream that throws, an HTMLRewriter handler that throws or
rejects after the first byte, a proxied upstream fetch body reset before
and after its first chunk. `Bun.file()` on a missing path already
reaches `error()` and answers 500 (unchanged). A user `Content-Length`
header on a stream Response is ignored in favor of chunked framing, so
it does not change the outcome.

Related PRs in this area: #35229 deferred the status line until the
first body byte so that `error()` can answer a pre-first-byte failure
with a 500 (a larger change to `do_render_stream` and the sink), closed
in favor of this PR; that design remains a possible follow-up. #38003
touches the same sites for the stored-ByteStream-error case. This PR
only enforces the framing invariant; it does not decide whether
`error()` should be reachable for these failures.

HTTP/3 is not fixed by this change: `uws_h3_res_force_close` closes the
QUIC stream with a FIN, which is a complete message in HTTP/3. That is
tracked separately in #40598.

</details>

<!-- robobun:evidence:begin -->

---

**no test proof** · iteration 5 · platform-specific test(s) that do not
run on this machine, deferring to CI, which covers all platforms:
test/js/bun/http/serve.test.ts

<!-- robobun:evidence:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant