From ad089de69b94bf0b87c31c8ff03f25bfd2179cd3 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 27 Aug 2026 01:42:58 +0000 Subject: [PATCH 1/6] Bun.serve: never terminate a failed body stream as a complete message When a Response body stream errored before any body byte reached uWS, the server ended the response with the terminating chunk. The client received the Response's status and headers with an empty chunked body and a clean `0\r\n\r\n`: a complete, successful-looking message for a body that failed. Only an error after the first body byte closed the connection without the terminator. The three sites that 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 such as a proxied fetch body) now share `close_incomplete_stream`: while the response is still pending the connection is force-closed, whether or not body bytes went out first. `end_chunk` also reports the producer's error the way the JS stream path does, instead of dropping it. The existing tests that pinned the old contract (status and headers with an empty terminated body, error() not called) now expect the connection to close without a complete response. error() is still not called once the status is committed. --- src/runtime/server/RequestContext.rs | 58 +++--- .../js/bun/http/async-iterator-stream.test.ts | 14 +- .../http/readable-stream-throws.fixture.js | 1 + .../http/serve-direct-readable-stream.test.ts | 13 +- .../http/serve-stream-body-error-fixture.ts | 20 +- .../bun/http/serve-stream-body-error.test.ts | 178 ++++++++++++++++-- test/js/bun/http/serve.test.ts | 33 +++- .../web/encoding/text-encoder-stream.test.ts | 5 +- 8 files changed, 247 insertions(+), 75 deletions(-) diff --git a/src/runtime/server/RequestContext.rs b/src/runtime/server/RequestContext.rs index cd7327a0ec0f..ab9635989bec 100644 --- a/src/runtime/server/RequestContext.rs +++ b/src/runtime/server/RequestContext.rs @@ -1003,21 +1003,16 @@ where // The status line is already committed (a direct ReadableStream's // pull() threw synchronously after do_render_stream wrote headers). // error() cannot replace a response whose status is on the wire; - // report the failure and terminate the body so the client observes an - // incomplete message instead of a second header block spliced into - // the chunked body. + // report the failure and close without the terminating chunk so the + // client observes an incomplete message instead of a second header + // block spliced into the chunked body. if !has_responded && self.flags.has_written_status() { if !value.is_empty_or_undefined_or_null() && let Some(server) = self.server.get() { server.vm().as_mut().run_error_handler(value, None); } - let state = resp.state(); - if state.is_http_write_called() && state.is_response_pending() { - self.force_close(); - } else { - self.end_stream(self.should_close_connection()); - } + self.close_incomplete_stream(); return; } @@ -1290,6 +1285,24 @@ where } } + /// The body source failed after the status line was committed. The + /// terminating chunk would turn the truncated body into a complete, + /// successful-looking message (RFC 9112 section 7), so while the response + /// is still pending close the connection instead and let the client see + /// an incomplete message. Whether any body bytes went out first does not + /// matter: an empty chunked body with a terminator is just as complete. + /// Once the sink already ended the response there is nothing left to + /// signal and `end_stream()` only releases the context. + pub(crate) fn close_incomplete_stream(&self) { + if let Some(resp) = self.resp.get() { + if resp.state().is_response_pending() { + self.force_close(); + return; + } + } + self.end_stream(self.should_close_connection()); + } + fn on_writable_complete_response_buffer( this: *mut Self, write_offset: u64, @@ -3054,17 +3067,7 @@ where self.end_already_responded_stream(); return; } - // Body bytes were already written: close without the terminating chunk - // (RFC 9112 section 7) so the client sees an incomplete message, not a - // truncated body that looks like a complete, successful response. - if let Some(resp) = self.resp.get() { - let state = resp.state(); - if state.is_http_write_called() && state.is_response_pending() { - self.force_close(); - return; - } - } - self.end_stream(self.should_close_connection()); + self.close_incomplete_stream(); } pub(crate) fn do_render_with_body( @@ -3353,13 +3356,16 @@ where this.run_error_handler(js_err); return; } - if let Some(resp) = this.resp.get() { - let state = resp.state(); - if state.is_http_write_called() && state.is_response_pending() { - this.force_close(); - return; - } + // Same as handle_reject_stream: error() cannot replace a + // committed status, so report the failure and close without the + // terminating chunk. + let global_this = this.server().global_this(); + let js_err = err.to_js(global_this); + if !js_err.is_empty_or_undefined_or_null() { + this.server().vm().as_mut().run_error_handler(js_err, None); } + this.close_incomplete_stream(); + return; } else if !this.flags.has_written_status() { // Upstream ended cleanly before any chunk: flush the deferred // status/headers so the client sees them before the terminator. diff --git a/test/js/bun/http/async-iterator-stream.test.ts b/test/js/bun/http/async-iterator-stream.test.ts index e6412d841691..8ee36eef08ff 100644 --- a/test/js/bun/http/async-iterator-stream.test.ts +++ b/test/js/bun/http/async-iterator-stream.test.ts @@ -114,10 +114,17 @@ describe.concurrent("Streaming body via", () => { expect(exitCode).toBe(0); }); - test("async generator function throws an error but continues to send the headers", async () => { + // The generator fails before producing any chunk. The status and headers + // were already committed to uWS, so the server closes the connection without + // a complete response instead of sending them with an empty body and a clean + // chunked terminator. + test("async generator function throws an error before its first chunk closes the connection", async () => { + let outcome: string | undefined; const onMessage = mock(async url => { - const response = await fetch(url); - expect(response.headers.get("X-Hey")).toBe("123"); + outcome = await fetch(url).then( + response => `resolved ${response.status}`, + () => "rejected", + ); subprocess?.kill(); }); @@ -132,6 +139,7 @@ describe.concurrent("Streaming body via", () => { let [exitCode, stderr] = await Promise.all([subprocess.exited, subprocess.stderr.text()]); expect(exitCode).toBeInteger(); + expect(outcome).toBe("rejected"); expect(stderr).toContain("error: Oops"); expect(onMessage).toHaveBeenCalledTimes(1); }); diff --git a/test/js/bun/http/readable-stream-throws.fixture.js b/test/js/bun/http/readable-stream-throws.fixture.js index ff32f9450787..7fc86669fad7 100644 --- a/test/js/bun/http/readable-stream-throws.fixture.js +++ b/test/js/bun/http/readable-stream-throws.fixture.js @@ -2,6 +2,7 @@ const server = Bun.serve({ port: 0, idleTimeout: 0, error(err) { + console.error("error handler called"); return new Response("Failed", { status: 555 }); }, diff --git a/test/js/bun/http/serve-direct-readable-stream.test.ts b/test/js/bun/http/serve-direct-readable-stream.test.ts index 1e36b5aeb619..9179b054dca9 100644 --- a/test/js/bun/http/serve-direct-readable-stream.test.ts +++ b/test/js/bun/http/serve-direct-readable-stream.test.ts @@ -793,7 +793,7 @@ describe("sync pull() throw after status is written does not re-render error()", expect(exitCode).toBe(0); }); - test("no body bytes flushed: stream is ended without splicing error() headers", async () => { + test("no body bytes flushed: connection is force-closed without splicing error() headers", async () => { await using proc = Bun.spawn({ cmd: [bunExe(), "-e", fixture(`throw new Error("boom");`)], env: bunEnv, @@ -803,11 +803,12 @@ describe("sync pull() throw after status is written does not re-render error()", const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); expect(stderr).toContain("error: boom"); const { wire, errorHandlerCalls } = JSON.parse(stdout); - // Status 200 was already written; the stream is ended empty. The error() - // response's status (500), headers, and body must not appear on the wire. - expect(wire).not.toContain("x-err"); - expect(wire).not.toContain("FROM-ERROR-HANDLER"); - expect(wire.startsWith("HTTP/1.1 200 OK\r\n")).toBe(true); + // Status 200 was already written to the corked response, so error() cannot + // replace it. Ending the stream here would send that 200 with an empty + // chunked body and a clean terminator: a complete-looking response for a + // body that failed. The connection is closed instead, and since the status + // never left the cork buffer the client sees an empty reply. + expect(wire).toBe(""); expect(errorHandlerCalls).toBe(0); expect(exitCode).toBe(0); }); diff --git a/test/js/bun/http/serve-stream-body-error-fixture.ts b/test/js/bun/http/serve-stream-body-error-fixture.ts index 41aec508a0e3..f72074f97107 100644 --- a/test/js/bun/http/serve-stream-body-error-fixture.ts +++ b/test/js/bun/http/serve-stream-body-error-fixture.ts @@ -135,7 +135,10 @@ await using server = Bun.serve({ errorCb++; return new Response("err-body", { status: 500 }); }, - fetch() { + fetch(req) { + if (new URL(req.url).pathname === "/ok") { + return new Response("ok"); + } return new Response(source()); }, }); @@ -143,12 +146,12 @@ await using server = Bun.serve({ // Sends one request over a raw socket and returns everything received before // the server closed the connection, so the test can assert on the HTTP // framing. A forced close (ECONNRESET) is an expected, asserted-on outcome -// for the mid-stream variant, so socket errors are not fatal. -function rawRequest(abort: boolean): Promise { +// for the erroring variants, so socket errors are not fatal. +function rawRequest(path: string, abort: boolean): Promise { const chunks: Buffer[] = []; return new Promise(resolve => { const sock = net.connect(server.port, "127.0.0.1", () => { - sock.write("GET / HTTP/1.1\r\nHost: 127.0.0.1\r\nConnection: close\r\n\r\n"); + sock.write(`GET ${path} HTTP/1.1\r\nHost: 127.0.0.1\r\nConnection: close\r\n\r\n`); }); sock.on("data", d => { chunks.push(d); @@ -165,12 +168,11 @@ function rawRequest(abort: boolean): Promise { }); } -const wire = await rawRequest(clientAborts); +const wire = await rawRequest("/", clientAborts); if (clientAborts) await cancelRan.promise; -// A second request proves the server is still accepting and answering. For the -// cancel variants the body stream never self-terminates, so abort that one too; -// only the status line is asserted. -const secondWire = await rawRequest(clientAborts); +// A second request to a healthy route proves the server is still accepting +// and answering; only its status line is asserted. +const secondWire = await rawRequest("/ok", false); // Cycle the event loop so any stray rejected promise reaches the // unhandledRejection reporter before we declare success. diff --git a/test/js/bun/http/serve-stream-body-error.test.ts b/test/js/bun/http/serve-stream-body-error.test.ts index c0e5635ea7bd..5a6960d9fb62 100644 --- a/test/js/bun/http/serve-stream-body-error.test.ts +++ b/test/js/bun/http/serve-stream-body-error.test.ts @@ -1,5 +1,6 @@ // A `Response` body backed by a `ReadableStream` whose source errors must not -// take the whole server down. Before the fix: +// take the whole server down, and must never reach the client as a complete +// message. Before the fixes: // 1. The rejection from the stream pump was never given a handler, so it // reached the global unhandledRejection reporter, whose default policy // exits the process: one bad request was a whole-process outage. @@ -8,6 +9,10 @@ // 3. A body that errored after chunks were already sent was still terminated // with a clean `0\r\n\r\n`, so the client could not tell the truncated // body apart from a complete one. +// 4. A body that errored before any chunk was sent went out as the +// Response's status and headers plus an empty chunked body with a clean +// terminator: a complete, successful-looking response for a body that +// failed. import { expect, test } from "bun:test"; import { bunEnv, bunExe, isASAN } from "harness"; import { join } from "node:path"; @@ -25,25 +30,25 @@ async function runFixture(variant: string, ...extra: string[]) { return { stdout, stderr, exitCode }; } -// The wire-level contract for a stream source that errors before producing any -// body bytes is unchanged: the Response's own status and headers go out with -// an empty chunked body and the server `error()` callback is NOT invoked (see -// "throw on pull renders headers, does not call error handler" in -// serve.test.ts). What changes is that the rejection no longer reaches the -// unhandledRejection reporter; it is reported directly instead. +// A stream source that errors before producing any body bytes. The status and +// headers were already handed to uWS (corked) when the error arrives, so the +// server `error()` callback cannot supply a replacement and is NOT invoked. +// The connection is closed before anything reaches the wire: the client sees +// an empty reply, never a complete 200 with an empty body. The rejection does +// not reach the unhandledRejection reporter; it is reported directly instead. test.concurrent.each([ "pull-throw", "pull-async-reject", "controller-error", "start-async-reject", "deferred-pull-throw", -])("%s: the rejection is handled and the error is still reported", async variant => { +])("%s: the connection is closed without a complete response and the error is reported", async variant => { const { stdout, stderr, exitCode } = await runFixture(variant); expect({ result: JSON.parse(stdout), exitCode }).toEqual({ result: { - statusLine: "HTTP/1.1 200 OK", - cleanChunkedTerminator: true, - body: "0\r\n\r\n", + statusLine: "", + cleanChunkedTerminator: false, + body: "", errorCb: 0, unhandled: 0, secondStatusLine: "HTTP/1.1 200 OK", @@ -56,13 +61,13 @@ test.concurrent.each([ }); // Same under `development: true` (the DEBUG RequestContext monomorphization). -test.concurrent("pull-throw in development mode: the rejection is handled", async () => { +test.concurrent("pull-throw in development mode: the connection is closed without a complete response", async () => { const { stdout, stderr, exitCode } = await runFixture("pull-throw", "development"); expect({ result: JSON.parse(stdout), exitCode }).toEqual({ result: { - statusLine: "HTTP/1.1 200 OK", - cleanChunkedTerminator: true, - body: "0\r\n\r\n", + statusLine: "", + cleanChunkedTerminator: false, + body: "", errorCb: 0, unhandled: 0, secondStatusLine: "HTTP/1.1 200 OK", @@ -72,6 +77,134 @@ test.concurrent("pull-throw in development mode: the rejection is handled", asyn expect(stderr).toContain("boom"); }); +// The headers are already on the wire (the first pull() was still pending +// when the server flushed them) and the source then errors without ever +// producing a chunk. The 200 is irrevocable, so the connection is closed +// without the terminating chunk: headers, then an incomplete body. +test.concurrent( + "async error before the first chunk: headers go out, the body is not terminated as complete", + async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + `import net from "node:net"; + const server = Bun.serve({ + port: 0, + development: false, + error() { return new Response("err-body", { status: 500 }); }, + fetch() { + return new Response(new ReadableStream({ + async pull(c) { await Bun.sleep(20); c.error(new Error("boom")); }, + }), { headers: { "x-marker": "1" } }); + }, + }); + const wire = await new Promise(resolve => { + let buf = ""; + const sock = net.connect(server.port, "127.0.0.1", () => { + sock.write("GET / HTTP/1.1\\r\\nHost: x\\r\\nConnection: close\\r\\n\\r\\n"); + }); + sock.on("data", d => (buf += d.toString("latin1"))); + sock.on("error", () => {}); + sock.on("close", () => resolve(buf)); + }); + const [head, ...body] = wire.split("\\r\\n\\r\\n"); + console.log(JSON.stringify({ + statusLine: head.split("\\r\\n")[0], + marker: /x-marker: 1/i.test(head), + body: body.join("\\r\\n\\r\\n"), + cleanChunkedTerminator: wire.endsWith("0\\r\\n\\r\\n"), + })); + server.stop(true);`, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, _stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // No stderr assertion: see the mid-stream test below for why this path is + // quiet with `development: false`. + expect({ result: JSON.parse(stdout), exitCode }).toEqual({ + result: { + statusLine: "HTTP/1.1 200 OK", + marker: true, + body: "", + cleanChunkedTerminator: false, + }, + exitCode: 0, + }); + }, +); + +// A native byte stream body (here: a proxied upstream fetch body) whose +// producer fails before the first chunk. The downstream client used to get a +// clean, empty 200 and nothing was reported. Now the upstream failure is +// reported and the downstream connection is closed without the terminator. +test.concurrent("proxied upstream body that fails before its first chunk is not forwarded as complete", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + `import net from "node:net"; + // The upstream answers with headers only; the proxy drops the connection + // once fetch() has parsed them, so the body fails before its first chunk. + let dropUpstream; + const upstream = net.createServer(sock => { + sock.once("data", () => { + sock.write("HTTP/1.1 200 OK\\r\\nContent-Type: text/plain\\r\\nTransfer-Encoding: chunked\\r\\n\\r\\n"); + dropUpstream = () => sock.destroy(); + }); + }); + await new Promise(resolve => upstream.listen(0, "127.0.0.1", resolve)); + const server = Bun.serve({ + port: 0, + development: false, + error() { return new Response("err-body", { status: 500 }); }, + async fetch() { + const up = await fetch("http://127.0.0.1:" + upstream.address().port + "/"); + dropUpstream(); + return new Response(up.body, { status: up.status, headers: { "x-proxied": "1" } }); + }, + }); + const wire = await new Promise(resolve => { + let buf = ""; + const sock = net.connect(server.port, "127.0.0.1", () => { + sock.write("GET / HTTP/1.1\\r\\nHost: x\\r\\nConnection: close\\r\\n\\r\\n"); + }); + sock.on("data", d => (buf += d.toString("latin1"))); + sock.on("error", () => {}); + sock.on("close", () => resolve(buf)); + }); + const [head, ...body] = wire.split("\\r\\n\\r\\n"); + console.log(JSON.stringify({ + statusLine: head.split("\\r\\n")[0], + proxied: /x-proxied: 1/i.test(head), + body: body.join("\\r\\n\\r\\n"), + cleanChunkedTerminator: wire.endsWith("0\\r\\n\\r\\n"), + })); + server.stop(true); + upstream.close();`, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // The proxy's status and headers are flushed as soon as the upstream headers + // arrive, so they are on the wire; the body must then end incomplete. + expect({ result: JSON.parse(stdout), exitCode }).toEqual({ + result: { + statusLine: "HTTP/1.1 200 OK", + proxied: true, + body: "", + cleanChunkedTerminator: false, + }, + exitCode: 0, + }); + // The upstream failure is reported instead of being swallowed. + expect(stderr).toContain("ECONNRESET"); +}); + // The body errors after a chunk has already been flushed to the client. The // 200 is irrevocable at that point, but the connection must be closed without // the terminating `0\r\n\r\n` chunk (RFC 9112 section 7) so the client can @@ -218,16 +351,21 @@ test.concurrent("a stream body error does not kill the server process", async () `const server = Bun.serve({ port: 0, development: false, - fetch() { + fetch(req) { + if (new URL(req.url).pathname === "/ok") return new Response("ok"); return new Response(new ReadableStream({ pull() { throw new Error("boom"); } })); }, }); - const res = await fetch(server.url); - await res.arrayBuffer(); + // The failed body closes the connection without a complete response. + const first = await fetch(server.url).then( + res => res.arrayBuffer().then(() => "complete"), + () => "rejected", + ); // Tick the event loop past the unhandledRejection checkpoint that used // to exit the process before this line was reached. for (let i = 0; i < 10; i++) await Bun.sleep(0); - console.log("alive", res.status); + const res = await fetch(new URL("/ok", server.url)); + console.log("alive", first, res.status); server.stop(true);`, ], env: bunEnv, @@ -235,7 +373,7 @@ test.concurrent("a stream body error does not kill the server process", async () stderr: "pipe", }); const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - expect({ stdout, exitCode }).toEqual({ stdout: "alive 200\n", exitCode: 0 }); + expect({ stdout, exitCode }).toEqual({ stdout: "alive rejected 200\n", exitCode: 0 }); // The error must still be surfaced to the operator. expect(stderr).toContain("boom"); }); diff --git a/test/js/bun/http/serve.test.ts b/test/js/bun/http/serve.test.ts index 83ed8ab21198..dfbd4116e0f3 100644 --- a/test/js/bun/http/serve.test.ts +++ b/test/js/bun/http/serve.test.ts @@ -622,12 +622,17 @@ it.each([ describe("streaming", () => { describe("error handler", () => { - it("throw on pull renders headers, does not call error handler", async () => { + // The body source fails before any byte is written. The Response's status + // and headers are already committed to uWS, so error() cannot replace them + // and is not called; the connection is closed without a complete response + // so the client cannot mistake the failed body for an empty one. + it("throw on pull closes the connection, does not call error handler", async () => { + let outcome: string | undefined; const onMessage = mock(async url => { - const response = await fetch(url); - expect(response.status).toBe(402); - expect(response.headers.get("X-Hey")).toBe("123"); - expect(response.text()).resolves.toBe(""); + outcome = await fetch(url).then( + response => `resolved ${response.status}`, + () => "rejected", + ); subprocess.kill(); }); @@ -642,17 +647,23 @@ describe("streaming", () => { let [exitCode, stderr] = await Promise.all([subprocess.exited, subprocess.stderr.text()]); expect(exitCode).toBeInteger(); + expect(outcome).toBe("rejected"); expect(stderr).toContain("error: Oops"); + expect(stderr).not.toContain("error handler called"); expect(onMessage).toHaveBeenCalled(); }); - it("throw on pull after writing should not call the error handler", async () => { + // pull() queues two chunks, requests close, then throws: per the streams + // spec the error wins and the queued chunks are discarded, so this is the + // same "failed before any byte" case as above. + it("throw on pull after writing closes the connection, does not call the error handler", async () => { + let outcome: string | undefined; const onMessage = mock(async href => { const url = new URL("write", href); - const response = await fetch(url); - expect(response.status).toBe(402); - expect(response.headers.get("X-Hey")).toBe("123"); - expect(response.text()).resolves.toBe(""); + outcome = await fetch(url).then( + response => `resolved ${response.status}`, + () => "rejected", + ); subprocess.kill(); }); @@ -667,7 +678,9 @@ describe("streaming", () => { let [exitCode, stderr] = await Promise.all([subprocess.exited, subprocess.stderr.text()]); expect(exitCode).toBeInteger(); + expect(outcome).toBe("rejected"); expect(stderr).toContain("error: Oops"); + expect(stderr).not.toContain("error handler called"); expect(onMessage).toHaveBeenCalled(); }); diff --git a/test/js/web/encoding/text-encoder-stream.test.ts b/test/js/web/encoding/text-encoder-stream.test.ts index d37ff1663d23..d35e1b465c58 100644 --- a/test/js/web/encoding/text-encoder-stream.test.ts +++ b/test/js/web/encoding/text-encoder-stream.test.ts @@ -408,7 +408,10 @@ describe.skipIf(!isASAN)("a failed output buffer allocation errors the stream in }); const results = {}; for (const name of Object.keys(inputs)) { - await fetch(new URL(name, server.url)).then(response => response.arrayBuffer(), () => {}); + // The failed body closes the connection without a complete response: + // fetch() itself or the body read rejects, depending on whether the + // headers were already flushed. + await fetch(new URL(name, server.url)).then(response => response.arrayBuffer()).catch(() => {}); results[name] = await cancelReasons[name]; } results.afterwards = Array.from(await (await fetch(new URL("small", server.url))).bytes()); From 8321a8f173c814b739aff03717a980e81b6b5574 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 27 Aug 2026 02:15:02 +0000 Subject: [PATCH 2/6] test: make the headers-then-error cases deterministic and assert the fetch error code The forced close sends a RST, and a RST discards data the peer has not read yet (always on Windows). The two tests that expect the status line on the wire before the failure now trigger the failure only once the client has read it: the pending-error-after-headers fixture variant and the proxied upstream test drop the source from the client's data handler. uWS terminates the header block only with the first body byte, so the trigger waits for the status line (or the proxied header), not for a blank line. The rewritten "throw on pull" tests assert the fetch rejection code (ECONNRESET) instead of only that the fetch rejected. --- .../js/bun/http/async-iterator-stream.test.ts | 4 +- .../http/serve-stream-body-error-fixture.ts | 26 +++++- .../bun/http/serve-stream-body-error.test.ts | 92 +++++++------------ test/js/bun/http/serve.test.ts | 8 +- 4 files changed, 63 insertions(+), 67 deletions(-) diff --git a/test/js/bun/http/async-iterator-stream.test.ts b/test/js/bun/http/async-iterator-stream.test.ts index 8ee36eef08ff..537f30cc3000 100644 --- a/test/js/bun/http/async-iterator-stream.test.ts +++ b/test/js/bun/http/async-iterator-stream.test.ts @@ -123,7 +123,7 @@ describe.concurrent("Streaming body via", () => { const onMessage = mock(async url => { outcome = await fetch(url).then( response => `resolved ${response.status}`, - () => "rejected", + (err: any) => `rejected ${err.code}`, ); subprocess?.kill(); }); @@ -139,7 +139,7 @@ describe.concurrent("Streaming body via", () => { let [exitCode, stderr] = await Promise.all([subprocess.exited, subprocess.stderr.text()]); expect(exitCode).toBeInteger(); - expect(outcome).toBe("rejected"); + expect(outcome).toBe("rejected ECONNRESET"); expect(stderr).toContain("error: Oops"); expect(onMessage).toHaveBeenCalledTimes(1); }); diff --git a/test/js/bun/http/serve-stream-body-error-fixture.ts b/test/js/bun/http/serve-stream-body-error-fixture.ts index f72074f97107..c6ad6ac0364e 100644 --- a/test/js/bun/http/serve-stream-body-error-fixture.ts +++ b/test/js/bun/http/serve-stream-body-error-fixture.ts @@ -17,6 +17,14 @@ const development = process.argv[3] === "development"; // wire. Called from the raw request's data handler below. let midStreamResolve: (() => void) | undefined; +// Set by the pending-error-after-headers variant so it only errors once the +// status line has provably reached the client socket. (uWS terminates the +// header block only with the first body byte, so a blank line never arrives +// for this variant.) The forced close that follows sends a RST, and a RST can +// discard data the peer has not read yet (Windows always does), so the error +// must not race the client's read. +let statusLineReceivedResolve: (() => void) | undefined; + // Set by the cancel variants: resolved once the source's cancel() has run, so // the test observes any resulting rejection before declaring success. const cancelRan = Promise.withResolvers(); @@ -62,6 +70,17 @@ const sources: Record ReadableStream> = { }, { highWaterMark: 0 }, ), + // The first pull() is pending when the server flushes the headers; the + // source then errors without ever producing a chunk. + "pending-error-after-headers": () => + new ReadableStream({ + async pull(c) { + const { promise, resolve } = Promise.withResolvers(); + statusLineReceivedResolve = resolve; + await promise; + c.error(new Error("boom")); + }, + }), // Errors only after a chunk has already been flushed to the client. "mid-stream-reject": () => new ReadableStream({ @@ -155,7 +174,12 @@ function rawRequest(path: string, abort: boolean): Promise { }); sock.on("data", d => { chunks.push(d); - if (Buffer.concat(chunks).includes("chunk-a")) { + const received = Buffer.concat(chunks); + if (received.includes("\r\n")) { + statusLineReceivedResolve?.(); + statusLineReceivedResolve = undefined; + } + if (received.includes("chunk-a")) { midStreamResolve?.(); // The cancel variants tear down the socket once a body chunk has // provably reached the client, so the server's onAborted path fires diff --git a/test/js/bun/http/serve-stream-body-error.test.ts b/test/js/bun/http/serve-stream-body-error.test.ts index 5a6960d9fb62..60ec17401ac0 100644 --- a/test/js/bun/http/serve-stream-body-error.test.ts +++ b/test/js/bun/http/serve-stream-body-error.test.ts @@ -81,60 +81,23 @@ test.concurrent("pull-throw in development mode: the connection is closed withou // when the server flushed them) and the source then errors without ever // producing a chunk. The 200 is irrevocable, so the connection is closed // without the terminating chunk: headers, then an incomplete body. -test.concurrent( - "async error before the first chunk: headers go out, the body is not terminated as complete", - async () => { - await using proc = Bun.spawn({ - cmd: [ - bunExe(), - "-e", - `import net from "node:net"; - const server = Bun.serve({ - port: 0, - development: false, - error() { return new Response("err-body", { status: 500 }); }, - fetch() { - return new Response(new ReadableStream({ - async pull(c) { await Bun.sleep(20); c.error(new Error("boom")); }, - }), { headers: { "x-marker": "1" } }); - }, - }); - const wire = await new Promise(resolve => { - let buf = ""; - const sock = net.connect(server.port, "127.0.0.1", () => { - sock.write("GET / HTTP/1.1\\r\\nHost: x\\r\\nConnection: close\\r\\n\\r\\n"); - }); - sock.on("data", d => (buf += d.toString("latin1"))); - sock.on("error", () => {}); - sock.on("close", () => resolve(buf)); - }); - const [head, ...body] = wire.split("\\r\\n\\r\\n"); - console.log(JSON.stringify({ - statusLine: head.split("\\r\\n")[0], - marker: /x-marker: 1/i.test(head), - body: body.join("\\r\\n\\r\\n"), - cleanChunkedTerminator: wire.endsWith("0\\r\\n\\r\\n"), - })); - server.stop(true);`, - ], - env: bunEnv, - stdout: "pipe", - stderr: "pipe", - }); - const [stdout, _stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - // No stderr assertion: see the mid-stream test below for why this path is - // quiet with `development: false`. - expect({ result: JSON.parse(stdout), exitCode }).toEqual({ - result: { - statusLine: "HTTP/1.1 200 OK", - marker: true, - body: "", - cleanChunkedTerminator: false, - }, - exitCode: 0, - }); - }, -); +// +// No stderr assertion: see the mid-stream test below for why this path is +// quiet with `development: false`. +test.concurrent("pending-error-after-headers: headers go out, the body is not terminated as complete", async () => { + const { stdout, exitCode } = await runFixture("pending-error-after-headers"); + expect({ result: JSON.parse(stdout), exitCode }).toEqual({ + result: { + statusLine: "HTTP/1.1 200 OK", + cleanChunkedTerminator: false, + body: "", + errorCb: 0, + unhandled: 0, + secondStatusLine: "HTTP/1.1 200 OK", + }, + exitCode: 0, + }); +}); // A native byte stream body (here: a proxied upstream fetch body) whose // producer fails before the first chunk. The downstream client used to get a @@ -146,8 +109,12 @@ test.concurrent("proxied upstream body that fails before its first chunk is not bunExe(), "-e", `import net from "node:net"; - // The upstream answers with headers only; the proxy drops the connection - // once fetch() has parsed them, so the body fails before its first chunk. + // The upstream answers with headers only. Its connection is dropped once + // the proxied headers have provably reached the downstream client, so + // the body fails before its first chunk and the forced close (a RST, + // which can discard unread data on the peer) cannot race the client's + // read. uWS terminates the header block only with the first body byte, + // so no blank line ever arrives here. let dropUpstream; const upstream = net.createServer(sock => { sock.once("data", () => { @@ -162,7 +129,6 @@ test.concurrent("proxied upstream body that fails before its first chunk is not error() { return new Response("err-body", { status: 500 }); }, async fetch() { const up = await fetch("http://127.0.0.1:" + upstream.address().port + "/"); - dropUpstream(); return new Response(up.body, { status: up.status, headers: { "x-proxied": "1" } }); }, }); @@ -171,7 +137,13 @@ test.concurrent("proxied upstream body that fails before its first chunk is not const sock = net.connect(server.port, "127.0.0.1", () => { sock.write("GET / HTTP/1.1\\r\\nHost: x\\r\\nConnection: close\\r\\n\\r\\n"); }); - sock.on("data", d => (buf += d.toString("latin1"))); + sock.on("data", d => { + buf += d.toString("latin1"); + if (/x-proxied: 1\\r\\n/i.test(buf)) { + dropUpstream?.(); + dropUpstream = undefined; + } + }); sock.on("error", () => {}); sock.on("close", () => resolve(buf)); }); @@ -359,7 +331,7 @@ test.concurrent("a stream body error does not kill the server process", async () // The failed body closes the connection without a complete response. const first = await fetch(server.url).then( res => res.arrayBuffer().then(() => "complete"), - () => "rejected", + err => "rejected " + err.code, ); // Tick the event loop past the unhandledRejection checkpoint that used // to exit the process before this line was reached. @@ -373,7 +345,7 @@ test.concurrent("a stream body error does not kill the server process", async () stderr: "pipe", }); const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - expect({ stdout, exitCode }).toEqual({ stdout: "alive rejected 200\n", exitCode: 0 }); + expect({ stdout, exitCode }).toEqual({ stdout: "alive rejected ECONNRESET 200\n", exitCode: 0 }); // The error must still be surfaced to the operator. expect(stderr).toContain("boom"); }); diff --git a/test/js/bun/http/serve.test.ts b/test/js/bun/http/serve.test.ts index dfbd4116e0f3..1cce88860437 100644 --- a/test/js/bun/http/serve.test.ts +++ b/test/js/bun/http/serve.test.ts @@ -631,7 +631,7 @@ describe("streaming", () => { const onMessage = mock(async url => { outcome = await fetch(url).then( response => `resolved ${response.status}`, - () => "rejected", + (err: any) => `rejected ${err.code}`, ); subprocess.kill(); }); @@ -647,7 +647,7 @@ describe("streaming", () => { let [exitCode, stderr] = await Promise.all([subprocess.exited, subprocess.stderr.text()]); expect(exitCode).toBeInteger(); - expect(outcome).toBe("rejected"); + expect(outcome).toBe("rejected ECONNRESET"); expect(stderr).toContain("error: Oops"); expect(stderr).not.toContain("error handler called"); expect(onMessage).toHaveBeenCalled(); @@ -662,7 +662,7 @@ describe("streaming", () => { const url = new URL("write", href); outcome = await fetch(url).then( response => `resolved ${response.status}`, - () => "rejected", + (err: any) => `rejected ${err.code}`, ); subprocess.kill(); }); @@ -678,7 +678,7 @@ describe("streaming", () => { let [exitCode, stderr] = await Promise.all([subprocess.exited, subprocess.stderr.text()]); expect(exitCode).toBeInteger(); - expect(outcome).toBe("rejected"); + expect(outcome).toBe("rejected ECONNRESET"); expect(stderr).toContain("error: Oops"); expect(stderr).not.toContain("error handler called"); expect(onMessage).toHaveBeenCalled(); From 126cf90a3616f0dc5d12e17c96bc6123e04c0511 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 27 Aug 2026 02:18:02 +0000 Subject: [PATCH 3/6] Bun.serve: trim the body failure comments --- src/runtime/server/RequestContext.rs | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/src/runtime/server/RequestContext.rs b/src/runtime/server/RequestContext.rs index ab9635989bec..8591e6f31853 100644 --- a/src/runtime/server/RequestContext.rs +++ b/src/runtime/server/RequestContext.rs @@ -1001,11 +1001,8 @@ where let has_responded = resp.has_responded(); // The status line is already committed (a direct ReadableStream's - // pull() threw synchronously after do_render_stream wrote headers). - // error() cannot replace a response whose status is on the wire; - // report the failure and close without the terminating chunk so the - // client observes an incomplete message instead of a second header - // block spliced into the chunked body. + // pull() threw synchronously after do_render_stream wrote headers), so + // error() cannot replace it: report the failure and close instead. if !has_responded && self.flags.has_written_status() { if !value.is_empty_or_undefined_or_null() && let Some(server) = self.server.get() @@ -1285,14 +1282,10 @@ where } } - /// The body source failed after the status line was committed. The - /// terminating chunk would turn the truncated body into a complete, - /// successful-looking message (RFC 9112 section 7), so while the response - /// is still pending close the connection instead and let the client see - /// an incomplete message. Whether any body bytes went out first does not - /// matter: an empty chunked body with a terminator is just as complete. - /// Once the sink already ended the response there is nothing left to - /// signal and `end_stream()` only releases the context. + /// The body failed after the status line was committed. A terminating + /// chunk would make the truncated body look complete (RFC 9112 section 7), + /// so close the connection while the response is still pending. Once the + /// sink already ended it, `end_stream()` only releases the context. pub(crate) fn close_incomplete_stream(&self) { if let Some(resp) = self.resp.get() { if resp.state().is_response_pending() { @@ -3356,9 +3349,7 @@ where this.run_error_handler(js_err); return; } - // Same as handle_reject_stream: error() cannot replace a - // committed status, so report the failure and close without the - // terminating chunk. + // Committed status: report and close, like handle_reject_stream. let global_this = this.server().global_this(); let js_err = err.to_js(global_this); if !js_err.is_empty_or_undefined_or_null() { From c413210d1812c32203b432e66dd7e04e4560b72c Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 27 Aug 2026 02:22:20 +0000 Subject: [PATCH 4/6] Bun.serve: shorten the body failure comments, assert stdout before the exit code --- src/runtime/server/RequestContext.rs | 11 ++- .../bun/http/serve-stream-body-error.test.ts | 71 +++++++++---------- 2 files changed, 36 insertions(+), 46 deletions(-) diff --git a/src/runtime/server/RequestContext.rs b/src/runtime/server/RequestContext.rs index 8591e6f31853..1ab9af674b0e 100644 --- a/src/runtime/server/RequestContext.rs +++ b/src/runtime/server/RequestContext.rs @@ -1000,9 +1000,8 @@ where // SAFETY: FFI handle, just checked Some let has_responded = resp.has_responded(); - // The status line is already committed (a direct ReadableStream's - // pull() threw synchronously after do_render_stream wrote headers), so - // error() cannot replace it: report the failure and close instead. + // The status line is already committed (a direct stream's pull() threw + // synchronously after the headers were written): report and close. if !has_responded && self.flags.has_written_status() { if !value.is_empty_or_undefined_or_null() && let Some(server) = self.server.get() @@ -1282,10 +1281,8 @@ where } } - /// The body failed after the status line was committed. A terminating - /// chunk would make the truncated body look complete (RFC 9112 section 7), - /// so close the connection while the response is still pending. Once the - /// sink already ended it, `end_stream()` only releases the context. + /// Closes a response whose body failed after the status line was committed. + /// Never the terminating chunk: it would make a truncated body look complete. pub(crate) fn close_incomplete_stream(&self) { if let Some(resp) = self.resp.get() { if resp.state().is_response_pending() { diff --git a/test/js/bun/http/serve-stream-body-error.test.ts b/test/js/bun/http/serve-stream-body-error.test.ts index 60ec17401ac0..b4a13d959440 100644 --- a/test/js/bun/http/serve-stream-body-error.test.ts +++ b/test/js/bun/http/serve-stream-body-error.test.ts @@ -44,37 +44,33 @@ test.concurrent.each([ "deferred-pull-throw", ])("%s: the connection is closed without a complete response and the error is reported", async variant => { const { stdout, stderr, exitCode } = await runFixture(variant); - expect({ result: JSON.parse(stdout), exitCode }).toEqual({ - result: { - statusLine: "", - cleanChunkedTerminator: false, - body: "", - errorCb: 0, - unhandled: 0, - secondStatusLine: "HTTP/1.1 200 OK", - }, - exitCode: 0, + expect(JSON.parse(stdout)).toEqual({ + statusLine: "", + cleanChunkedTerminator: false, + body: "", + errorCb: 0, + unhandled: 0, + secondStatusLine: "HTTP/1.1 200 OK", }); // With `development: false` the unhandledRejection report used to be the // only place the error surfaced; it must still reach stderr without it. expect(stderr).toContain("boom"); + expect(exitCode).toBe(0); }); // Same under `development: true` (the DEBUG RequestContext monomorphization). test.concurrent("pull-throw in development mode: the connection is closed without a complete response", async () => { const { stdout, stderr, exitCode } = await runFixture("pull-throw", "development"); - expect({ result: JSON.parse(stdout), exitCode }).toEqual({ - result: { - statusLine: "", - cleanChunkedTerminator: false, - body: "", - errorCb: 0, - unhandled: 0, - secondStatusLine: "HTTP/1.1 200 OK", - }, - exitCode: 0, + expect(JSON.parse(stdout)).toEqual({ + statusLine: "", + cleanChunkedTerminator: false, + body: "", + errorCb: 0, + unhandled: 0, + secondStatusLine: "HTTP/1.1 200 OK", }); expect(stderr).toContain("boom"); + expect(exitCode).toBe(0); }); // The headers are already on the wire (the first pull() was still pending @@ -86,17 +82,15 @@ test.concurrent("pull-throw in development mode: the connection is closed withou // quiet with `development: false`. test.concurrent("pending-error-after-headers: headers go out, the body is not terminated as complete", async () => { const { stdout, exitCode } = await runFixture("pending-error-after-headers"); - expect({ result: JSON.parse(stdout), exitCode }).toEqual({ - result: { - statusLine: "HTTP/1.1 200 OK", - cleanChunkedTerminator: false, - body: "", - errorCb: 0, - unhandled: 0, - secondStatusLine: "HTTP/1.1 200 OK", - }, - exitCode: 0, + expect(JSON.parse(stdout)).toEqual({ + statusLine: "HTTP/1.1 200 OK", + cleanChunkedTerminator: false, + body: "", + errorCb: 0, + unhandled: 0, + secondStatusLine: "HTTP/1.1 200 OK", }); + expect(exitCode).toBe(0); }); // A native byte stream body (here: a proxied upstream fetch body) whose @@ -164,17 +158,15 @@ test.concurrent("proxied upstream body that fails before its first chunk is not const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); // The proxy's status and headers are flushed as soon as the upstream headers // arrive, so they are on the wire; the body must then end incomplete. - expect({ result: JSON.parse(stdout), exitCode }).toEqual({ - result: { - statusLine: "HTTP/1.1 200 OK", - proxied: true, - body: "", - cleanChunkedTerminator: false, - }, - exitCode: 0, + expect(JSON.parse(stdout)).toEqual({ + statusLine: "HTTP/1.1 200 OK", + proxied: true, + body: "", + cleanChunkedTerminator: false, }); // The upstream failure is reported instead of being swallowed. expect(stderr).toContain("ECONNRESET"); + expect(exitCode).toBe(0); }); // The body errors after a chunk has already been flushed to the client. The @@ -345,9 +337,10 @@ test.concurrent("a stream body error does not kill the server process", async () stderr: "pipe", }); const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - expect({ stdout, exitCode }).toEqual({ stdout: "alive rejected ECONNRESET 200\n", exitCode: 0 }); + expect(stdout).toBe("alive rejected ECONNRESET 200\n"); // The error must still be surfaced to the operator. expect(stderr).toContain("boom"); + expect(exitCode).toBe(0); }); // An already-rejected async fetch handler reaches handle_reject synchronously From 137dbe5fdef2484ffea589ce6f0c1b34b5fc571b Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 27 Aug 2026 06:59:58 +0000 Subject: [PATCH 5/6] Bun.serve: share the committed body error report between JS and native bodies handle_reject_stream and end_chunk each had their own copy of the report for a body that failed after the status line was committed. The JS copy appended the bake dev server error page; the native copy did not. Both now call report_committed_body_error, so an HTMLRewriter or proxied fetch body that fails under a dev server gets the same error page as a JS stream. 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. Tests: the fixture gains the HTMLRewriter and proxied fetch variants, run in both modes, plus two rows under a bake dev server. The inline proxied upstream test is now the proxied-fetch-after-status row. --- src/runtime/server/RequestContext.rs | 94 ++++++------ .../http/serve-stream-body-error-fixture.html | 3 + .../http/serve-stream-body-error-fixture.ts | 137 +++++++++++++++--- .../bun/http/serve-stream-body-error.test.ts | 135 ++++++++--------- 4 files changed, 232 insertions(+), 137 deletions(-) create mode 100644 test/js/bun/http/serve-stream-body-error-fixture.html diff --git a/src/runtime/server/RequestContext.rs b/src/runtime/server/RequestContext.rs index 1ab9af674b0e..c1d6a13ebdee 100644 --- a/src/runtime/server/RequestContext.rs +++ b/src/runtime/server/RequestContext.rs @@ -3004,50 +3004,9 @@ where self.render_metadata(); } - if DEBUG_MODE { - if let Some(server) = self.server.get() { - if !err.is_empty_or_undefined_or_null() { - let server = &*server; - let mut exception_list: jsc::ExceptionList = Vec::new(); - server - .vm() - .as_mut() - .run_error_handler(err, Some(&mut exception_list)); - - // The fallback page below writes into `resp`, which must - // not be dereferenced once the sink has already ended the - // response (see `end_already_responded_stream`). - if !ended_response && server.dev_server().is_some() { - // Render the error fallback HTML page like renderDefaultError does - if !self.flags.has_written_status() { - self.flags.set_has_written_status(true); - if let Some(resp) = self.resp.get() { - resp.write_status(b"500 Internal Server Error"); - resp.write_header( - b"content-type", - &bun_http_types::MimeType::HTML.value, - ); - } - } - - let bb = DevErrorPage { - message: b"Stream error during server-side rendering", - cwd: bun_resolver::fs::FileSystem::get().top_level_dir, - exceptions: &exception_list, - log: None, - } - .render(); - - if let Some(resp) = self.resp.get() { - // SAFETY: FFI handle - resp.write(&bb); - } - - self.end_stream(self.should_close_connection()); - return; - } - } - } + // Production mode keeps this asynchronous JS path quiet. + if DEBUG_MODE && self.report_committed_body_error(err, !ended_response) { + return; } // HTTP/1 only: the sink already fully ended the response, so `resp` // can no longer be dereferenced (see `end_already_responded_stream`). @@ -3060,6 +3019,45 @@ where self.close_incomplete_stream(); } + /// Reports a body failure that arrived after the status line was + /// committed, so `error()` can no longer answer. Under a bake dev server + /// the error page is appended to the body and the response ends normally: + /// returns `true`, and the caller must not touch `resp` again. + /// `resp_writable` is false once the sink has already ended the response + /// (see `end_already_responded_stream`). + fn report_committed_body_error(&self, err: JSValue, resp_writable: bool) -> bool { + if err.is_empty_or_undefined_or_null() { + return false; + } + let server = self.server(); + if !DEBUG_MODE || !resp_writable || server.dev_server().is_none() { + server.vm().as_mut().run_error_handler(err, None); + return false; + } + + let mut exception_list: jsc::ExceptionList = Vec::new(); + server + .vm() + .as_mut() + .run_error_handler(err, Some(&mut exception_list)); + + let bb = DevErrorPage { + message: b"Stream error during server-side rendering", + cwd: bun_resolver::fs::FileSystem::get().top_level_dir, + exceptions: &exception_list, + log: None, + } + .render(); + + if let Some(resp) = self.resp.get() { + // SAFETY: FFI handle + resp.write(&bb); + } + + self.end_stream(self.should_close_connection()); + true + } + pub(crate) fn do_render_with_body( &self, value: *mut Body::Value, @@ -3346,11 +3344,11 @@ where this.run_error_handler(js_err); return; } - // Committed status: report and close, like handle_reject_stream. + // Committed status: report in both modes, then close like + // handle_reject_stream. let global_this = this.server().global_this(); - let js_err = err.to_js(global_this); - if !js_err.is_empty_or_undefined_or_null() { - this.server().vm().as_mut().run_error_handler(js_err, None); + if this.report_committed_body_error(err.to_js(global_this), true) { + return; } this.close_incomplete_stream(); return; diff --git a/test/js/bun/http/serve-stream-body-error-fixture.html b/test/js/bun/http/serve-stream-body-error-fixture.html new file mode 100644 index 000000000000..f75efe4b1148 --- /dev/null +++ b/test/js/bun/http/serve-stream-body-error-fixture.html @@ -0,0 +1,3 @@ + + +dev server page diff --git a/test/js/bun/http/serve-stream-body-error-fixture.ts b/test/js/bun/http/serve-stream-body-error-fixture.ts index c6ad6ac0364e..25e9ded60a14 100644 --- a/test/js/bun/http/serve-stream-body-error-fixture.ts +++ b/test/js/bun/http/serve-stream-body-error-fixture.ts @@ -1,28 +1,45 @@ // An error thrown by a ReadableStream used as a Response body must never // escape as a global unhandledRejection. Bun's default unhandledRejection // policy exits the process, so before the fix a single bad request took the -// entire server down. +// entire server down. The `nativeBodies` variants pin the same wire and +// reporting contract for bodies Bun.serve streams without a JS ReadableStream. // -// argv[2]: variant name (see `sources` below) -// argv[3]: "development" to run the server with development: true +// argv[2]: variant name (see `sources` and `nativeBodies` below) +// argv[3..]: "development" to run the server with development: true; +// "dev-server" to also give it an HTML route, which makes the +// development server run a bake dev server (the requests below +// still go to fetch()) // // Prints a single JSON object on stdout and exits 0. import net from "node:net"; +import devServerPage from "./serve-stream-body-error-fixture.html"; const variant = process.argv[2]; -const development = process.argv[3] === "development"; +const flags = process.argv.slice(3); +const development = flags.includes("development"); +const devServer = flags.includes("dev-server"); -// Set by the mid-stream variant so it only errors once its chunk has provably -// reached the client socket, i.e. after the 200 response is committed on the -// wire. Called from the raw request's data handler below. +// Set by the mid-stream variants so they only error once their chunk has +// provably reached the client socket, i.e. after the 200 response is committed +// on the wire. Called from the raw request's data handler below. let midStreamResolve: (() => void) | undefined; -// Set by the pending-error-after-headers variant so it only errors once the -// status line has provably reached the client socket. (uWS terminates the -// header block only with the first body byte, so a blank line never arrives -// for this variant.) The forced close that follows sends a RST, and a RST can -// discard data the peer has not read yet (Windows always does), so the error -// must not race the client's read. +// Arms `midStreamResolve` for one request. Every mid-stream variant calls this +// before it emits "chunk-a", so the resolver is in place by the time the +// client can see the chunk. +function chunkReachedClient(): Promise { + const { promise, resolve } = Promise.withResolvers(); + midStreamResolve = resolve; + return promise; +} + +// Set by the variants that fail after the status line but before any body byte +// (pending-error-after-headers, proxied-fetch-after-status) so they only error +// once the status line has provably reached the client socket. (uWS terminates +// the header block only with the first body byte, so a blank line never +// arrives for these variants.) The forced close that follows sends a RST, and +// a RST can discard data the peer has not read yet (Windows always does), so +// the error must not race the client's read. let statusLineReceivedResolve: (() => void) | undefined; // Set by the cancel variants: resolved once the source's cancel() has run, so @@ -85,10 +102,9 @@ const sources: Record ReadableStream> = { "mid-stream-reject": () => new ReadableStream({ async pull(c) { - const { promise, resolve } = Promise.withResolvers(); - midStreamResolve = resolve; + const reached = chunkReachedClient(); c.enqueue("chunk-a"); - await promise; + await reached; throw new Error("boom"); }, }), @@ -131,8 +147,92 @@ const sources: Record ReadableStream> = { }), }; +// HTMLRewriter input that lets the rewritten "chunk-a" reach the client +// before delivering the element whose handler fails. +function rewriterInput(): ReadableStream { + const reached = chunkReachedClient(); + let pulls = 0; + return new ReadableStream({ + async pull(c) { + if (pulls++ === 0) { + c.enqueue("chunk-a"); + return; + } + await reached; + c.enqueue("

x

"); + c.close(); + }, + }); +} + +function rewriterResponse(element: () => void | Promise): Response { + return new HTMLRewriter() + .on("p", { element }) + .transform(new Response(rewriterInput(), { headers: { "content-type": "text/html" } })); +} + +// Upstream for the proxied variants: a scratch server for one request that +// answers with `firstWrite` (announcing more body than it will ever send), so +// the fetch() body fails deterministically when `fail()` closes the connection. +async function fetchFromFailingUpstream(firstWrite: string) { + const sockets: net.Socket[] = []; + const upstream = net.createServer(socket => { + sockets.push(socket); + socket.resume(); + socket.on("error", () => {}); + socket.write(firstWrite); + }); + await new Promise(resolve => upstream.listen(0, "127.0.0.1", resolve)); + // Must never be what keeps the process alive if fail() is not reached. + upstream.unref(); + const { port } = upstream.address() as net.AddressInfo; + const res = await fetch(`http://127.0.0.1:${port}/`); + return { + res, + fail() { + for (const socket of sockets) socket.end(); + upstream.close(); + }, + }; +} + +// Bodies Bun.serve streams natively (no JS ReadableStream pump) whose producer +// fails after the status line is committed. error() can no longer answer, so +// the failure is reported and the connection is closed without the terminating +// chunk, whether or not a body byte went out first. +const nativeBodies: Record Response | Promise> = { + "rewriter-mid-stream-throw": () => + rewriterResponse(() => { + throw new Error("boom"); + }), + "rewriter-mid-stream-reject": () => + rewriterResponse(async () => { + await Bun.sleep(0); + throw new Error("boom"); + }), + // `return fetch(...)` whose upstream dies once its first chunk has made it + // through to our client. + "proxied-fetch-mid-stream": async () => { + const reached = chunkReachedClient(); + const { res, fail } = await fetchFromFailingUpstream("HTTP/1.1 200 OK\r\nContent-Length: 100\r\n\r\nchunk-a"); + reached.then(fail); + return res; + }, + // Same, but the upstream dies before producing a single body byte. Bun.serve + // commits the status as soon as it attaches to a fetch() body, so the client + // has the status line and then the connection closes with no body at all. + "proxied-fetch-after-status": async () => { + const { promise, resolve } = Promise.withResolvers(); + statusLineReceivedResolve = resolve; + const { res, fail } = await fetchFromFailingUpstream("HTTP/1.1 200 OK\r\nContent-Length: 100\r\n\r\n"); + promise.then(fail); + return res; + }, +}; + const source = sources[variant]; -if (!source) { +const nativeBody = nativeBodies[variant]; +if (!source && !nativeBody) { console.error(`unknown variant: ${variant}`); process.exit(3); } @@ -150,6 +250,7 @@ let errorCb = 0; await using server = Bun.serve({ port: 0, development, + ...(devServer ? { routes: { "/dev-server-page": devServerPage } } : {}), error() { errorCb++; return new Response("err-body", { status: 500 }); @@ -158,7 +259,7 @@ await using server = Bun.serve({ if (new URL(req.url).pathname === "/ok") { return new Response("ok"); } - return new Response(source()); + return nativeBody ? nativeBody() : new Response(source()); }, }); diff --git a/test/js/bun/http/serve-stream-body-error.test.ts b/test/js/bun/http/serve-stream-body-error.test.ts index b4a13d959440..80177caa5f09 100644 --- a/test/js/bun/http/serve-stream-body-error.test.ts +++ b/test/js/bun/http/serve-stream-body-error.test.ts @@ -13,6 +13,9 @@ // Response's status and headers plus an empty chunked body with a clean // terminator: a complete, successful-looking response for a body that // failed. +// The natively streamed bodies further down (HTMLRewriter, proxied fetch) pin +// the same contract for the non-ReadableStream path, which also dropped the +// producer's error without reporting it. import { expect, test } from "bun:test"; import { bunEnv, bunExe, isASAN } from "harness"; import { join } from "node:path"; @@ -93,80 +96,70 @@ test.concurrent("pending-error-after-headers: headers go out, the body is not te expect(exitCode).toBe(0); }); -// A native byte stream body (here: a proxied upstream fetch body) whose -// producer fails before the first chunk. The downstream client used to get a -// clean, empty 200 and nothing was reported. Now the upstream failure is -// reported and the downstream connection is closed without the terminator. -test.concurrent("proxied upstream body that fails before its first chunk is not forwarded as complete", async () => { - await using proc = Bun.spawn({ - cmd: [ - bunExe(), - "-e", - `import net from "node:net"; - // The upstream answers with headers only. Its connection is dropped once - // the proxied headers have provably reached the downstream client, so - // the body fails before its first chunk and the forced close (a RST, - // which can discard unread data on the peer) cannot race the client's - // read. uWS terminates the header block only with the first body byte, - // so no blank line ever arrives here. - let dropUpstream; - const upstream = net.createServer(sock => { - sock.once("data", () => { - sock.write("HTTP/1.1 200 OK\\r\\nContent-Type: text/plain\\r\\nTransfer-Encoding: chunked\\r\\n\\r\\n"); - dropUpstream = () => sock.destroy(); - }); - }); - await new Promise(resolve => upstream.listen(0, "127.0.0.1", resolve)); - const server = Bun.serve({ - port: 0, - development: false, - error() { return new Response("err-body", { status: 500 }); }, - async fetch() { - const up = await fetch("http://127.0.0.1:" + upstream.address().port + "/"); - return new Response(up.body, { status: up.status, headers: { "x-proxied": "1" } }); +// Bodies that Bun.serve streams natively (an HTMLRewriter transform, a proxied +// fetch() Response) reach the server through RequestContext::end_chunk rather +// than handle_reject_stream. Their status is committed before the body starts, +// so error() cannot answer. Before the fix end_chunk dropped the error (a +// rewriter handler that threw after the first byte left no trace anywhere) and +// a producer that failed before its first chunk went out as a clean, empty 200. +// Now the failure is reported in both modes and the connection is closed +// without the terminating chunk, with whatever body bytes were already out. +// +// [variant, body bytes on the wire, what the report must name] +const nativeBodies = [ + ["rewriter-mid-stream-throw", "e\r\nchunk-a\r\n", "boom"], + ["rewriter-mid-stream-reject", "e\r\nchunk-a\r\n", "boom"], + ["proxied-fetch-mid-stream", "7\r\nchunk-a\r\n", "ECONNRESET"], + ["proxied-fetch-after-status", "", "ECONNRESET"], +] as const; + +for (const flags of [[], ["development"]]) { + const mode = flags.length ? "development" : "production"; + test.concurrent.each(nativeBodies)( + `%s in ${mode} mode: reported, and the body is not terminated as complete`, + async (variant, body, reported) => { + const { stdout, stderr, exitCode } = await runFixture(variant, ...flags); + expect({ result: JSON.parse(stdout), exitCode }).toEqual({ + result: { + statusLine: "HTTP/1.1 200 OK", + cleanChunkedTerminator: false, + body, + errorCb: 0, + unhandled: 0, + secondStatusLine: "HTTP/1.1 200 OK", }, + exitCode: 0, }); - const wire = await new Promise(resolve => { - let buf = ""; - const sock = net.connect(server.port, "127.0.0.1", () => { - sock.write("GET / HTTP/1.1\\r\\nHost: x\\r\\nConnection: close\\r\\n\\r\\n"); - }); - sock.on("data", d => { - buf += d.toString("latin1"); - if (/x-proxied: 1\\r\\n/i.test(buf)) { - dropUpstream?.(); - dropUpstream = undefined; - } - }); - sock.on("error", () => {}); - sock.on("close", () => resolve(buf)); - }); - const [head, ...body] = wire.split("\\r\\n\\r\\n"); - console.log(JSON.stringify({ - statusLine: head.split("\\r\\n")[0], - proxied: /x-proxied: 1/i.test(head), - body: body.join("\\r\\n\\r\\n"), - cleanChunkedTerminator: wire.endsWith("0\\r\\n\\r\\n"), - })); - server.stop(true); - upstream.close();`, - ], - env: bunEnv, - stdout: "pipe", - stderr: "pipe", - }); - const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - // The proxy's status and headers are flushed as soon as the upstream headers - // arrive, so they are on the wire; the body must then end incomplete. - expect(JSON.parse(stdout)).toEqual({ - statusLine: "HTTP/1.1 200 OK", - proxied: true, - body: "", - cleanChunkedTerminator: false, + expect(stderr).toContain(reported); + }, + ); +} + +// When the development server also runs a bake dev server (it has an HTML +// route), the report additionally renders the dev error page as the rest of +// the body and ends the response normally, so the browser shows the error +// after whatever was already streamed. The JS stream has always taken this +// branch; the native body now goes through the same code. +test.concurrent.each([ + ["rewriter-mid-stream-throw", "e\r\nchunk-a\r\n"], + ["mid-stream-reject", "7\r\nchunk-a\r\n"], +])("%s under a dev server: the error page is appended to the body", async (variant, firstChunk) => { + const { stdout, stderr, exitCode } = await runFixture(variant, "development", "dev-server"); + const { body, ...result } = JSON.parse(stdout); + expect({ result, exitCode }).toEqual({ + result: { + statusLine: "HTTP/1.1 200 OK", + cleanChunkedTerminator: true, + errorCb: 0, + unhandled: 0, + secondStatusLine: "HTTP/1.1 200 OK", + }, + exitCode: 0, }); - // The upstream failure is reported instead of being swallowed. - expect(stderr).toContain("ECONNRESET"); - expect(exitCode).toBe(0); + expect(body).toStartWith(firstChunk); + expect(body).toContain("Stream error during server-side rendering"); + expect(body).toContain("boom"); + expect(stderr).toContain("boom"); }); // The body errors after a chunk has already been flushed to the client. The From 4041ad8f3eff2f274fd9e275b7082e7d8a2de987 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 27 Aug 2026 07:08:00 +0000 Subject: [PATCH 6/6] Bun.serve: shorten the committed body error comments --- src/runtime/server/RequestContext.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/runtime/server/RequestContext.rs b/src/runtime/server/RequestContext.rs index c1d6a13ebdee..850ee0bf43b3 100644 --- a/src/runtime/server/RequestContext.rs +++ b/src/runtime/server/RequestContext.rs @@ -3020,11 +3020,8 @@ where } /// Reports a body failure that arrived after the status line was - /// committed, so `error()` can no longer answer. Under a bake dev server - /// the error page is appended to the body and the response ends normally: - /// returns `true`, and the caller must not touch `resp` again. - /// `resp_writable` is false once the sink has already ended the response - /// (see `end_already_responded_stream`). + /// committed. Under a bake dev server the error page ends the response: + /// returns `true`, and `resp` must not be touched again. fn report_committed_body_error(&self, err: JSValue, resp_writable: bool) -> bool { if err.is_empty_or_undefined_or_null() { return false; @@ -3344,8 +3341,7 @@ where this.run_error_handler(js_err); return; } - // Committed status: report in both modes, then close like - // handle_reject_stream. + // Committed status: report in both modes, then close. let global_this = this.server().global_this(); if this.report_committed_body_error(err.to_js(global_this), true) { return;