Conversation
…as sent A natively streamed body (an HTMLRewriter output, a fetch() body returned as-is) delivers its terminal error to RequestContext::end_chunk. With the status already written it only force-closed or ended the response and never looked at the error, so a rewriter handler that threw after the first emitted byte left no trace in either mode. handle_reject, handle_reject_stream and end_chunk each had a copy of the "body failed after the status went out" close. They now share close_failed_body. handle_reject_stream and end_chunk share report_committed_body_error: the development-mode report, plus the bake dev server error page when one is attached, so a native body gets the same page a JS stream already got. The dev page branch no longer writes a 500 status: render_metadata always runs first when resp is still writable, so the status is already written there. The wire is unchanged: force-close without the chunked terminator after body bytes went out, a normal end otherwise. Tests: the fixture gains the HTMLRewriter and proxied fetch variants in both modes, plus two rows under a bake dev server.
…irst chunk is written A Response body stream that failed before any body byte was written went out as a complete message: the status, the headers, an empty chunked body and a clean terminator. curl exited 0 and a cache could store it. Only a failure after the first body byte closed the connection without the terminator. close_failed_body now force-closes an HTTP/1.1 response while it is pending, with or without body bytes out. A chunked message is complete only with its terminator (RFC 9112 section 7), so the close is the only way to signal the failure. When the status is still in the cork buffer the close discards it and the client sees an empty reply. HTTP/3 keeps the normal end when no body byte went out: its force-close is a FIN, and a FIN before the HEADERS frame makes the client re-send the request. end_chunk reports the producer's error in both modes, like handle_reject. The asynchronous JS path stays quiet in production mode. This reverses the contract pinned by "throw on pull renders headers, does not call error handler" (#4251) and "async generator function throws an error but continues to send the headers" (#8941). Both tests now expect the connection to close. error() is still not called once the status is committed.
|
Updated 6:07 AM PT - Aug 27th, 2026
❌ @robobun, your commit 53ef213 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 40648That installs a local version of the PR into your bun-40648 --bun |
StatusReady for review at 53ef213. Stacked on #40596; the diff here is only the framing change. Reproduced with This reverses the contract pinned in #4251 and #8941 (the status and headers were sent, the body was empty, 13 of 26 tests in |
|
|
||
| /// A `T` built by its `from_js_async` parser or owned constructor (see | ||
| /// [`ThreadIsolatedArg`]); nothing else constructs one. | ||
| pub struct ThreadIsolated<T>(T); |
There was a problem hiding this comment.
🟡 nit: new ThreadIsolated<T> dropped #[repr(transparent)]; the Windows args_as! SAFETY comment in node_fs.rs still claims it is repr(transparent)
Extended reasoning...
The old ThreadIsolated<T> in src/jsc/node_path.rs was #[repr(transparent)] explicitly "so identity-casts in the const-generic dispatch macros (see node_fs.rs's args_as!) remain bit-exact". The replacement at src/runtime/node/types.rs:245 is a plain pub struct ThreadIsolated<T>(T); without the attribute. The Windows-only args_as! macro at src/runtime/node/node_fs.rs:707 still says in its SAFETY comment "ThreadIsolated<A> is repr(transparent); deref through it for the inner A." — the code is actually correct (it goes through Deref::deref → &self.0, not a transmute), but per REVIEW.md SAFETY comments must be accurate. Either restore #[repr(transparent)] on the new type or update the stale comment; relative to base this is a documentation-accuracy regression only, no runtime behavior change.
Verification: nit — The claim is factually accurate and the diff created the inconsistency, but nothing breaks. Old definition (removed by this PR, src/jsc/node_path.rs base): /// `repr(transparent)` so identity-casts in the const-generic dispatch macros /// (see `node_fs.rs`'s `args_as!`) remain bit-exact. #[repr(transparent)] pub struct ThreadIsolated<T: Unprotect>(T); New definition (added by this…
| call). Values parsed from JS for an async call, stored, or sent to another thread (the | ||
| `from_js_async` parsers, which return `ThreadIsolated<T>`; | ||
| `PathLike::thread_isolated_copy` for a `Blob` store) is `'static`. |
There was a problem hiding this comment.
🟡 nit: grammar regression in CLAUDE.md — "Values parsed from JS ... is 'static" (subject/verb disagree)
Extended reasoning...
The rewritten sentence now reads "Values parsed from JS for an async call, stored, or sent to another thread (...) is 'static." The base said "Anything parsed from JS ... is 'static", which agreed. The new plural subject needs "are". Purely a doc nit; no runtime impact, but CLAUDE.md is developer-facing guidance and REVIEW.md treats docs as reviewed word-for-word.
Verification: nit — The diff at src/CLAUDE.md (lines ~125-130 post-change) replaces "Anything parsed from JS, stored, or sent to another thread ... is 'static." with "Values parsed from JS for an async call, stored, or sent to another thread (the from_js_async parsers, which return ThreadIsolated<T>; PathLike::thread_isolated_copy for a Blob store) is 'static." The subject changed from…
|
@robobun rebase |
|
Closing: #40596 (ec70edb) merged with this behavior. On main, Six of this PR's eight test files are byte-identical on main at e3b6d63: The one residue is the HTTP/3 arm ( |
Stacked on #40596. This changes a wire contract and needs a maintainer decision (second Fix bullet).
Problem
Bun.servebody stream that fails before its first byte reaches the client as a complete message: status, headers, empty chunked body, clean0\r\n\r\n.curlexits 0 and a cache can store it.close_failed_bodyinsrc/runtime/server/RequestContext.rsforce-closes only afteris_http_write_called(). Otherwiseend_stream()writes the terminator.Fix
close_failed_bodyforce-closes while the response is pending, with or without body bytes out. Correct because a chunked message is complete only with its terminator (RFC 9112 section 7). Closing without it is the only HTTP/1.1 signal for an incomplete message.ResponseandRequestfor bodies #8941 (an async generator that throws "continues to send the headers"). Both tests now expect the connection to close.error()is still not called once the status is committed. The alternative is Bun.serve: route Response body stream errors to error() until the first byte is written #35229: defer the status line soerror()can answer with a 500.end_chunk(native bodies) now reports in both modes, likehandle_reject. The asynchronous JS path stays quiet in production mode, as before.test/js/bun/http/serve-stream-body-error.test.ts(26 tests, 13 fail on Bun.serve: never terminate a failed body stream as a complete message #40596's build). Other suites in Notes.Background
do_render_streamcommits status and headers before it attaches the body, so a body error arrives withhas_written_status()true. Unchanged.force_close()drops that buffer: a synchronous failure leaves nothing on the wire, an asynchronous one leaves headers, then a reset.start(c) { c.enqueue(x) }with apull()that errors still goes out as a completeContent-Lengthresponse. The sink ends the buffered chunk before the error reachesRequestContext.Notes
Also run:
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.Wire before, for
new Response(new ReadableStream({ start(c) { c.enqueue(chunk); c.error(new Error("boom")); } })):After: the connection closes with 0 bytes sent (the status was still corked),
curlexits 52. For a stream whose firstpull()is asynchronous, the headers were already flushed, so the client getsHTTP/1.1 200 OKplus headers and then a connection reset with no terminating chunk,curlexits 56. Both are incomplete messages.Outcome matrix on this branch (HTTP/1.1): synchronous failure before any byte: empty reply. Asynchronous failure before any byte: headers, then reset. Failure after a chunk: headers, chunk, then reset (unchanged). The async generator cases from #35229 (throw before the first yield, synchronous yields then throw, awaited yields then throw) all end incomplete.
Probed sources, all closed incomplete:
controller.errorinstart, synchronous and asynchronouspull()throw, async generator throw,Readable.toWebof an erroring node stream, a throwingTransformStream,DecompressionStreamon bad bytes, atype: "direct"stream that throws, anHTMLRewriterhandler that throws after output, a proxied upstream reset before and after its first chunk.Bun.file()on a missing path reacheserror()and answers 500 (unchanged).Tests re-pinned:
serve.test.ts("throw on pull renders headers" and "after writing"),async-iterator-stream.test.ts,serve-direct-readable-stream.test.ts,text-encoder-stream.test.ts, the pre-first-byte variants inserve-stream-body-error.test.ts. The fixture's second request now goes to a healthy/okroute, since a second failing request no longer answers with a status line.Review history: this framing change was first proposed as the whole of #40596. Its review found the contract reversal, the HTTP/3 re-send and the sink gap, so #40596 was reduced to the wire-neutral reporting fix and this part moved here. #35229 (the
error()alternative, conflicts with #33660) and #39442 (folded into #40596) are closed. #38003 pins the old framing for the after-attach case on the same fixture; whichever lands second adjusts that row.