From ed6a9c596825b726daf38b66e6627ec701a3d93b Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 5 Jul 2026 13:11:17 +0000 Subject: [PATCH 1/4] node:http2: don't emit 'aborted' when respond() ends the stream A respond() whose HEADERS frame carries END_STREAM (204, 205, 304, a HEAD request, or endStream: true) ended the writable side only after the native request() call. When the peer had already half-closed, that call drives the stream straight to closed and dispatches onStreamEnd synchronously, so _destroy ran with the writable side still open and treated the teardown as a client abort: a spurious 'aborted' event and stream.aborted === true. End the writable side before submitting the frame, as node does. _final parks its callback instead of writing an empty DATA frame, and the onStreamEnd dispatch settles it. --- src/js/node/http2.ts | 26 +++++++- test/js/node/http2/node-http2.test.js | 92 +++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 3 deletions(-) diff --git a/src/js/node/http2.ts b/src/js/node/http2.ts index 14302e78b572..9c753ad27bca 100644 --- a/src/js/node/http2.ts +++ b/src/js/node/http2.ts @@ -2050,6 +2050,9 @@ enum StreamState { // The native side fully closed and freed the stream (state 7 delivered): there is // nothing left to send on the wire for it. NativeClosed = 1 << 6, // 1000000 = 64 + // respond() is ending the writable side ahead of a HEADERS frame that carries + // END_STREAM, so `_final` must not write an empty DATA frame of its own. + EndStreamOnHeaders = 1 << 7, // 10000000 = 128 } function markWritableDone(stream: Http2Stream) { const _final = stream[bunHTTP2StreamFinal]; @@ -2421,6 +2424,13 @@ class Http2Stream extends Duplex { const native = session[bunHTTP2Native]; if (native) { this[bunHTTP2StreamStatus] |= StreamState.FinalCalled; + // respond() is about to submit a HEADERS frame carrying END_STREAM, so there is no + // DATA frame left to write. Park the callback: the onStreamEnd dispatch that the + // native request() makes runs markWritableDone, which invokes it. + if ((status & StreamState.EndStreamOnHeaders) !== 0) { + this[bunHTTP2StreamFinal] = callback; + return; + } // When waitForTrailers is active, writing an empty DATA frame with // close=true emits a bare empty DATA frame (flags=0) to the wire // before the trailer/noTrailers path runs, which then emits ANOTHER @@ -3116,6 +3126,13 @@ class ServerHttp2Stream extends Http2Stream { } const wireHeaders = rawHeadersList !== null ? rawHeadersList : headers; + if (endStream) { + // node ends the writable side before submitting the HEADERS frame. request() below can + // close an already half-closed stream and destroy it synchronously, and _destroy reads a + // still-open writable side as a client abort - ending first keeps 'aborted' from firing. + this[bunHTTP2StreamStatus] |= StreamState.EndStreamOnHeaders; + this.end(); + } if (typeof options === "undefined") { session[bunHTTP2Native]?.request(this.id, undefined, wireHeaders, sensitiveNames); } else { @@ -3132,14 +3149,17 @@ class ServerHttp2Stream extends Http2Stream { this[bunHTTP2WaitForTrailers] = true; } } + if (endStream) { + // The onStreamEnd dispatch request() makes normally settles the `_final` callback parked + // above. Idempotent here, and it keeps a submit the native layer rejected (no dispatch) + // from leaving the writable side hanging. + markWritableDone(this); + } this.headersSent = true; if (onServerStreamFinishChannel.hasSubscribers) { onServerStreamFinishChannel.publish({ stream: this, headers, flags: 0 }); } this[bunHTTP2Headers] = headers; - if (endStream) { - this.end(); - } return; } diff --git a/test/js/node/http2/node-http2.test.js b/test/js/node/http2/node-http2.test.js index 8f658be0d0f6..5a1b32c4a5b3 100644 --- a/test/js/node/http2/node-http2.test.js +++ b/test/js/node/http2/node-http2.test.js @@ -3186,3 +3186,95 @@ it("http2 allowHTTP1 fallback omits the Connection header on a close-delimited r server.close(); } }); + +// Collects the server stream's lifecycle events for one request, plus `stream.aborted` as read +// from the 'close' handler. 'end' is filtered out: bun and node disagree on whether it lands +// before or after 'finish', which is unrelated to what these tests assert. +async function serverStreamLifecycle(onStream, requestHeaders = { ":path": "/" }) { + const events = []; + let aborted = null; + const { promise: streamClosed, resolve: streamDidClose } = Promise.withResolvers(); + + const server = http2.createServer(); + server.on("stream", stream => { + stream.on("aborted", () => events.push("aborted")); + stream.on("finish", () => events.push("finish")); + stream.on("close", () => { + aborted = stream.aborted; + events.push("close"); + streamDidClose(); + }); + onStream(stream); + }); + + await new Promise(resolve => server.listen(0, resolve)); + const client = http2.connect(`http://localhost:${server.address().port}`); + client.on("error", () => {}); + try { + const req = client.request(requestHeaders); + const responseHeaders = await new Promise((resolve, reject) => { + req.on("error", reject); + req.on("response", resolve); + req.end(); + }); + const body = []; + req.on("data", chunk => body.push(chunk)); + await new Promise((resolve, reject) => { + req.on("error", reject); + req.on("end", resolve); + }); + await streamClosed; + return { events, aborted, status: responseHeaders[":status"], body: Buffer.concat(body).toString() }; + } finally { + client.close(); + server.close(); + } +} + +it("http2 respondWithFile statCheck returning false does not emit a spurious 'aborted'", async () => { + // The documented statCheck veto: reply 304 from a cache validator and cancel the file send. + // Node treats that as a completed response, so no 'aborted' fires and stream.aborted stays false. + const result = await serverStreamLifecycle(stream => { + stream.respondWithFile( + import.meta.path, + { "content-type": "text/plain" }, + { + statCheck() { + stream.respond({ ":status": 304 }); + stream.end(); + return false; + }, + }, + ); + }); + + expect(result).toEqual({ events: ["finish", "close"], aborted: false, status: 304, body: "" }); +}); + +it("http2 server respond() with END_STREAM after the request body does not emit a spurious 'aborted'", async () => { + // Any respond() whose HEADERS frame carries END_STREAM (204/205/304, HEAD, endStream: true) + // fully closes a stream whose peer already half-closed. That is a completed response, so the + // writable side must be ended before the frame is submitted or the teardown looks like an abort. + const cases = [ + { name: "304", status: 304, respond: stream => stream.respond({ ":status": 304 }) }, + { name: "204", status: 204, respond: stream => stream.respond({ ":status": 204 }) }, + { name: "205", status: 205, respond: stream => stream.respond({ ":status": 205 }) }, + { name: "endStream", status: 200, respond: stream => stream.respond({ ":status": 200 }, { endStream: true }) }, + { name: "head", status: 200, respond: stream => stream.respond({ ":status": 200 }), method: "HEAD" }, + ]; + + const results = []; + for (const { name, respond, method } of cases) { + // Respond once the request body is fully received, which is when the stream is already + // half-closed by the peer and submitting END_STREAM closes it outright. + const { events, aborted, status, body } = await serverStreamLifecycle( + stream => stream.on("end", () => respond(stream)), + method ? { ":path": "/", ":method": method } : { ":path": "/" }, + ); + results.push({ name, events, aborted, status, body }); + } + + expect(results).toEqual( + cases.map(({ name, status }) => ({ name, events: ["finish", "close"], aborted: false, status, body: "" })), + ); +}); From 4f7749c8ff3e27581abfb8bc6481a4d28e1cf0f2 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 5 Jul 2026 14:06:02 +0000 Subject: [PATCH 2/4] node:http2: settle _final inline for an END_STREAM respond() markWritableDone only needs to run from the native onStreamEnd dispatch, so _final can call its callback straight back instead of parking it: 'finish' is queued on the next tick either way, after request() has put the HEADERS frame on the wire. This is what node's _final does here (handle.shutdown() reports a synchronous finish and the callback runs inline), and it means no path can leave the callback unsettled if request() throws. --- src/js/node/http2.ts | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/js/node/http2.ts b/src/js/node/http2.ts index 9c753ad27bca..48ebec46cae3 100644 --- a/src/js/node/http2.ts +++ b/src/js/node/http2.ts @@ -2424,11 +2424,11 @@ class Http2Stream extends Duplex { const native = session[bunHTTP2Native]; if (native) { this[bunHTTP2StreamStatus] |= StreamState.FinalCalled; - // respond() is about to submit a HEADERS frame carrying END_STREAM, so there is no - // DATA frame left to write. Park the callback: the onStreamEnd dispatch that the - // native request() makes runs markWritableDone, which invokes it. + // respond() is about to submit a HEADERS frame carrying END_STREAM, so there is no DATA + // frame left to write. Settle now, like node's _final does for this case; 'finish' is + // queued on the next tick, after request() has put the frame on the wire. if ((status & StreamState.EndStreamOnHeaders) !== 0) { - this[bunHTTP2StreamFinal] = callback; + callback(); return; } // When waitForTrailers is active, writing an empty DATA frame with @@ -3149,12 +3149,6 @@ class ServerHttp2Stream extends Http2Stream { this[bunHTTP2WaitForTrailers] = true; } } - if (endStream) { - // The onStreamEnd dispatch request() makes normally settles the `_final` callback parked - // above. Idempotent here, and it keeps a submit the native layer rejected (no dispatch) - // from leaving the writable side hanging. - markWritableDone(this); - } this.headersSent = true; if (onServerStreamFinishChannel.hasSubscribers) { onServerStreamFinishChannel.publish({ stream: this, headers, flags: 0 }); From 6f98db01328b3f767e5ea4cfb8f4f31482b76a12 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 5 Jul 2026 15:23:42 +0000 Subject: [PATCH 3/4] node:http2: flag the in-flight END_STREAM instead of reordering respond() Ending the writable side before the native request() call, the way node does, does not hold in bun: node validates headers in JS before ending, while bun validates them inside request(). A respond() that threw on a bad header left the writable side already ended, so the documented catch-and-retry recovery (respond again with a body-carrying status, then end) silently dropped its END_STREAM and hung the client. Keep this.end() where it was and record the in-flight END_STREAM on the stream status instead, cleared once the frame is out. _destroy reads it to tell a completed response apart from a client abort. A request() that threw submitted nothing, so the writable side stays genuinely open and the retry works. EndStreamOnHeaders has to be its own bit: NativeClosed and WritableClosed are also set when the peer cancels with RST_STREAM(NO_ERROR), which dispatches onStreamEnd(CLOSED) too, and suppressing 'aborted' there would swallow a real client cancel. Also shrink the maxSessionMemory stress test to 1k requests under debug+ASAN (88s -> 13s); at 10k it sat close enough to its own timeout to fail on a loaded machine. --- src/js/node/http2.ts | 65 ++++++++++++++------------- test/js/node/http2/node-http2.test.js | 53 ++++++++++++++++++++-- 2 files changed, 82 insertions(+), 36 deletions(-) diff --git a/src/js/node/http2.ts b/src/js/node/http2.ts index 48ebec46cae3..4a36eea30322 100644 --- a/src/js/node/http2.ts +++ b/src/js/node/http2.ts @@ -2050,8 +2050,9 @@ enum StreamState { // The native side fully closed and freed the stream (state 7 delivered): there is // nothing left to send on the wire for it. NativeClosed = 1 << 6, // 1000000 = 64 - // respond() is ending the writable side ahead of a HEADERS frame that carries - // END_STREAM, so `_final` must not write an empty DATA frame of its own. + // Set only while respond() has a HEADERS frame carrying END_STREAM in flight: the writable + // side is finished even though this.end() has not run yet. Read by _destroy, which the native + // request() reaches synchronously when that frame closes an already half-closed stream. EndStreamOnHeaders = 1 << 7, // 10000000 = 128 } function markWritableDone(stream: Http2Stream) { @@ -2341,8 +2342,10 @@ class Http2Stream extends Duplex { // closed by definition — closing it is not an abort and nothing must be sent on the wire. if (!ending && !this[kPush]) { // If the writable side of the Http2Stream is still open, emit the - // 'aborted' event and set the aborted flag. - if (!this.aborted) { + // 'aborted' event and set the aborted flag. EndStreamOnHeaders means respond() is + // submitting END_STREAM on the HEADERS frame right now and will call end() once the + // native call returns, so the response completed - nothing was cut short. + if (!this.aborted && (this[bunHTTP2StreamStatus] & StreamState.EndStreamOnHeaders) === 0) { this[kAborted] = true; this.emit("aborted"); } @@ -2424,13 +2427,6 @@ class Http2Stream extends Duplex { const native = session[bunHTTP2Native]; if (native) { this[bunHTTP2StreamStatus] |= StreamState.FinalCalled; - // respond() is about to submit a HEADERS frame carrying END_STREAM, so there is no DATA - // frame left to write. Settle now, like node's _final does for this case; 'finish' is - // queued on the next tick, after request() has put the frame on the wire. - if ((status & StreamState.EndStreamOnHeaders) !== 0) { - callback(); - return; - } // When waitForTrailers is active, writing an empty DATA frame with // close=true emits a bare empty DATA frame (flags=0) to the wire // before the trailer/noTrailers path runs, which then emits ANOTHER @@ -3126,34 +3122,39 @@ class ServerHttp2Stream extends Http2Stream { } const wireHeaders = rawHeadersList !== null ? rawHeadersList : headers; - if (endStream) { - // node ends the writable side before submitting the HEADERS frame. request() below can - // close an already half-closed stream and destroy it synchronously, and _destroy reads a - // still-open writable side as a client abort - ending first keeps 'aborted' from firing. - this[bunHTTP2StreamStatus] |= StreamState.EndStreamOnHeaders; - this.end(); - } - if (typeof options === "undefined") { - session[bunHTTP2Native]?.request(this.id, undefined, wireHeaders, sensitiveNames); - } else { - session[bunHTTP2Native]?.request(this.id, undefined, wireHeaders, sensitiveNames, options); - // Only track waitForTrailers when the HEADERS frame above did NOT end - // the stream. Status codes 204/205/304 and HEAD requests force - // endStream=true earlier in this method, which means the native - // request() already wrote END_STREAM on the HEADERS frame — driving - // the wantTrailers path from `_final` on such a stream would call - // `noTrailers`/`emit("wantTrailers")` on an already-half-closed - // stream and corrupt state. Use optional chaining: `options` may be - // `null` here (typeof null === "object" enters this else branch). - if (options?.waitForTrailers && !endStream) { - this[bunHTTP2WaitForTrailers] = true; + // An END_STREAM HEADERS frame can close an already half-closed stream outright, and the + // native request() then destroys it synchronously - before the this.end() below runs. Tell + // _destroy the writable side is finished, and clear it again once the frame is out: a + // request() that threw submitted nothing, so the writable side is still genuinely open. + if (endStream) this[bunHTTP2StreamStatus] |= StreamState.EndStreamOnHeaders; + try { + if (typeof options === "undefined") { + session[bunHTTP2Native]?.request(this.id, undefined, wireHeaders, sensitiveNames); + } else { + session[bunHTTP2Native]?.request(this.id, undefined, wireHeaders, sensitiveNames, options); + // Only track waitForTrailers when the HEADERS frame above did NOT end + // the stream. Status codes 204/205/304 and HEAD requests force + // endStream=true earlier in this method, which means the native + // request() already wrote END_STREAM on the HEADERS frame — driving + // the wantTrailers path from `_final` on such a stream would call + // `noTrailers`/`emit("wantTrailers")` on an already-half-closed + // stream and corrupt state. Use optional chaining: `options` may be + // `null` here (typeof null === "object" enters this else branch). + if (options?.waitForTrailers && !endStream) { + this[bunHTTP2WaitForTrailers] = true; + } } + } finally { + this[bunHTTP2StreamStatus] &= ~StreamState.EndStreamOnHeaders; } this.headersSent = true; if (onServerStreamFinishChannel.hasSubscribers) { onServerStreamFinishChannel.publish({ stream: this, headers, flags: 0 }); } this[bunHTTP2Headers] = headers; + if (endStream) { + this.end(); + } return; } diff --git a/test/js/node/http2/node-http2.test.js b/test/js/node/http2/node-http2.test.js index 5a1b32c4a5b3..86e80116f614 100644 --- a/test/js/node/http2/node-http2.test.js +++ b/test/js/node/http2/node-http2.test.js @@ -11,10 +11,12 @@ import { Duplex } from "stream"; import http2utils from "./helpers"; import { nodeEchoServer, TLS_CERT, TLS_OPTIONS } from "./http2-helpers"; const { describe, expect, it, beforeAll, afterAll, createCallCheckCtx } = createTest(import.meta.path); -// bun-debug ships with ASAN but isn't named bun-asan, so isASAN is false -// there; the 10k-request maxSessionMemory stress test takes ~90s under -// debug+ASAN vs ~2s release, so scale for either. +// bun-debug ships with ASAN but isn't named bun-asan, so isASAN is false there. const ASAN_MULTIPLIER = isDebug ? 10 : isASAN ? 3 : 1; +// The maxSessionMemory stress test costs ~2s for 10k requests in release but ~90s under +// debug+ASAN, close enough to its timeout that a loaded machine trips it. Shrink the workload +// rather than the deadline; 1k sequential requests still exercises the memory accounting. +const MAX_SESSION_MEMORY_REQUESTS = isDebug || isASAN ? 1_000 : 10_000; function invalidArgTypeHelper(input) { if (input === null) return " Received null"; @@ -1874,7 +1876,7 @@ it( const client = http2.connect(`http://localhost:${port}`); function next(i) { - if (i === 10000) { + if (i === MAX_SESSION_MEMORY_REQUESTS) { client.close(); server.close(); resolve(); @@ -3278,3 +3280,46 @@ it("http2 server respond() with END_STREAM after the request body does not emit cases.map(({ name, status }) => ({ name, events: ["finish", "close"], aborted: false, status, body: "" })), ); }); + +it("http2 a respond() that throws on invalid headers leaves the writable side open", async () => { + // 304 forces END_STREAM, and the invalid response pseudo-header makes respond() throw. Nothing + // was submitted, so the handler can still recover with a fresh respond() + end(). + const server = http2.createServer(); + let thrownCode = null; + server.on("stream", stream => { + try { + stream.respond({ ":status": 304, ":path": "/" }); + } catch (err) { + thrownCode = err.code; + stream.respond({ ":status": 500 }); + stream.end("err"); + } + }); + + await new Promise(resolve => server.listen(0, resolve)); + const client = http2.connect(`http://localhost:${server.address().port}`); + client.on("error", () => {}); + try { + const req = client.request({ ":path": "/" }); + const headers = await new Promise((resolve, reject) => { + req.on("error", reject); + req.on("response", resolve); + req.end(); + }); + const body = []; + req.on("data", chunk => body.push(chunk)); + await new Promise((resolve, reject) => { + req.on("error", reject); + req.on("end", resolve); + }); + + expect({ + thrownCode, + status: headers[":status"], + body: Buffer.concat(body).toString(), + }).toEqual({ thrownCode: "ERR_HTTP2_INVALID_PSEUDOHEADER", status: 500, body: "err" }); + } finally { + client.close(); + server.close(); + } +}); From 69433a5af677fe10fdbefefcbd1457b4ea75fde2 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 5 Jul 2026 17:11:19 +0000 Subject: [PATCH 4/4] ci: retrigger Build #68587 was green on 280 jobs; the three reds never touched this diff: darwin aarch64 (both shards) failed with 'buildkite-agent artifact download timed out after 120s' before running any test, ubuntu x64 OOM-killed v8-heap-snapshot.test.ts, and the windows 2019 agent failed to provision.