node:http2.createServer — fix h2c compatibility with strict peers - #29075
Merged
Merged
Conversation
ServerHttp2Session's initial SETTINGS frame advertised ENABLE_PUSH=1,
and Http2ServerResponse.end(body) emitted an extra empty DATA frame
followed by a zero-length trailer HEADERS frame. Strict peers (curl's
nghttp2, Node's http2 client) rejected the connection with
NGHTTP2_ERR_CALLBACK_FAILURE — hence 'Empty reply from server' from
curl --http2-prior-knowledge.
RFC 9113 §7.2.2: a server MUST NOT send SETTINGS_ENABLE_PUSH with a
value other than 0 — clients treat nonzero as PROTOCOL_ERROR. Default
the server's initial settings to enablePush: false (user settings
still override).
The compat Http2ServerResponse always passes waitForTrailers: true to
stream.respond() and then unconditionally calls sendTrailers({}) from
onStreamTrailersReady. Track waitForTrailers in a stream-local symbol
set from respond(); in _final, drive the wantTrailers path directly
instead of writing a bare empty DATA frame first. Also short-circuit
Http2Stream.sendTrailers to the native noTrailers path when the
header object is empty so that the stream terminates with a single
empty DATA + END_STREAM, matching Node's wire output.
Fixes #29073
Collaborator
Author
|
Updated 4:05 PM PT - Apr 17th, 2026
❌ @cirospaciari, your commit 263870e has 1 failures in 🧪 To try this PR locally: bunx bun-pr 29075That installs a local version of the PR into your bun-29075 --bun |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdd per-stream trailer coordination for HTTP/2, treat empty trailer objects as "no trailers", adjust stream finalization to wait for trailers when requested, set StreamResponded before emitting stream events, force-disable server push in parser/settings, and add five h2c regression tests validating on-the-wire framing and 204 behavior. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- respond(): only set bunHTTP2WaitForTrailers when the HEADERS frame
did not already close the stream. Status codes 204/205/304 and HEAD
requests force endStream=true earlier and the native request() writes
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. Guard with '!endStream'.
- ServerHttp2Session: move 'enablePush: false' to the end of the
settings spread so user-supplied settings cannot re-enable push.
RFC 9113 §7.2.2 is unconditional — a server MUST NOT advertise
SETTINGS_ENABLE_PUSH != 0.
- 29073 test: parse frames incrementally and resolve on the stream's
terminating DATA/HEADERS frame with END_STREAM instead of waiting
3s for a socket close that never comes (HTTP/2 keeps the TCP
connection open after a stream ends). First test drops from ~4.1s
to ~1.1s.
- Add a 204 No Content regression test exercising the respond()
guard above.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29073.test.ts`:
- Around line 107-155: Add a second regression test that constructs the server
with a caller-supplied settings object to ensure the server still advertises
SETTINGS_ENABLE_PUSH=0 even when the user passes enablePush: true; specifically,
duplicate the existing test "http2.createServer serves h2c response with
well-formed frames (`#29073`)" but call http2.createServer({ settings: {
enablePush: true } }, ...) and reuse rawH2cRequest to inspect frames, then
assert (using the same logic around serverSettings and checking setting id 0x02)
that the server's SETTINGS frame reports ENABLE_PUSH value 0; place this new
test alongside the existing one so it validates the ServerHttp2Session path that
previously had the spread-order bug.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 17462fef-cb7e-4ca8-b6ab-54c7adc57171
📒 Files selected for processing (2)
src/js/node/http2.tstest/regression/issue/29073.test.ts
Contributor
There was a problem hiding this comment.
LGTM — all previously raised issues are fixed; the remaining nit about #sentTrailers assignment in the direct noTrailers path of _final() is minor and limited to low-level ServerHttp2Stream consumers.
Extended reasoning...
Overview
The PR fixes two root-cause bugs in src/js/node/http2.ts that caused strict HTTP/2 peers (curl/nghttp2, Node's own http2.connect) to reject connections from Bun's http2.createServer:
ServerHttp2SessionadvertisedSETTINGS_ENABLE_PUSH=1— an RFC 9113 §7.2.2 PROTOCOL_ERROR.- The compat
Http2ServerResponsepath emitted an extra empty DATA frame plus an empty trailer HEADERS frame instead of a single empty DATA+END_STREAM.
A new regression test file (test/regression/issue/29073.test.ts) exercises both the raw TCP/frame-level path and the http2.connect client path, including a 204 response test added in the follow-up commit.
Security risks
No security-sensitive code (auth, crypto, permissions) is touched. The changes are protocol-compliance fixes in the HTTP/2 transport layer with no credential or access-control implications.
Level of scrutiny
Moderate — HTTP/2 protocol implementation is subtle, but the three changes are narrow and well-motivated:
enablePush: falselast in the spread (RFC-unconditional override) ✓if (options.waitForTrailers && \!endStream)guard prevents callingnoTrailerson already-closed streams for 204/205/304/HEAD ✓sendTrailers({})short-circuits tonoTrailersto avoid zero-length trailer HEADERS ✓
The test is event-driven (END_STREAM detection, no setTimeout) and covers the fixed paths directly.
Other factors
All three bugs I flagged in prior review rounds were addressed. The one remaining nit (inline comment on this run) is that this.#sentTrailers is not assigned when _final() takes the no-listener noTrailers() shortcut, leaving stream.sentTrailers returning undefined instead of {} and theoretically allowing a redundant noTrailers() call if sendTrailers() is invoked after end(). This only affects direct consumers of the low-level ServerHttp2Stream API (the compat layer is unaffected) and requires an unusual post-end() sendTrailers() call to trigger the double-call path — acceptable as a follow-up.
- _final()'s direct native.noTrailers() path (no wantTrailers listener)
now assigns this.#sentTrailers = {} so a later stream.sendTrailers()
call trips the ERR_HTTP2_TRAILERS_ALREADY_SENT guard instead of
calling native.noTrailers() a second time on an already-half-closed
stream. The emit("wantTrailers") path already ends up in
sendTrailers({}) which assigns #sentTrailers itself, so this just
makes the two branches consistent. Only affects direct consumers of
the low-level ServerHttp2Stream API; the compat Http2ServerResponse
always registers a wantTrailers listener.
- Add a regression test exercising createServer({ settings: {
enablePush: true } }, ...) to lock in the spread-order fix: the
server must still advertise SETTINGS_ENABLE_PUSH=0 regardless of
caller input (RFC 9113 §7.2.2 is unconditional).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/http2.ts`:
- Around line 3121-3127: The Server may still send ENABLE_PUSH=1 when a caller
later invokes ServerHttp2Session.settings(...); update the
ServerHttp2Session.settings method to coerce/override any user-supplied
enablePush value to false before emitting the SETTINGS frame or merging into the
session's settings (i.e., ensure enablePush: false is applied last when building
the outgoing settings object, similar to the initial settings merge shown in the
settings: { ...options, ...options?.settings, enablePush: false } line); locate
the ServerHttp2Session.settings function and make sure any path that forwards
settings to the wire or internal store forcibly sets enablePush to false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: efab157e-8fc7-4bbc-a58c-e3d868d873e7
📒 Files selected for processing (2)
src/js/node/http2.tstest/regression/issue/29073.test.ts
The initial SETTINGS frame is fixed at construction, but a later
session.settings({ enablePush: true }) on a ServerHttp2Session would
still forward ENABLE_PUSH=1 to the wire mid-connection. RFC 9113
§7.2.2 applies to every SETTINGS frame a server sends, not just the
initial one. Force-override enablePush=false in the server's
settings() method before forwarding to the parser (ClientHttp2Session's
settings() is untouched — clients are allowed to advertise enablePush).
Add a regression test that installs a 'session' handler which calls
session.settings({ enablePush: true }) as soon as a session comes up,
then asserts every non-ACK SETTINGS frame on the wire reports
ENABLE_PUSH=0.
The server/client StreamResponded bit was set via stream[bunHTTP2StreamStatus] = status | StreamState.StreamResponded where `status` was captured BEFORE synchronously dispatching the 'stream' / 'response' event to user code. If the user handler sets any other bits on the same stream (via stream.respond/stream.end, both call paths set WantTrailer/FinalCalled/EndedCalled/WritableClosed), the later write back of the stale `status` clobbers them. In the async-wantTrailers path this manifests as ERR_HTTP2_TRAILERS_NOT_READY when the user listener finally calls sendTrailers after awaiting I/O. Switch both call sites to `stream[bunHTTP2StreamStatus] |= StreamState.StreamResponded` (atomic read-modify-write) and set the bit BEFORE dispatching the event so the stored value reflects any bits the user handler adds.
Contributor
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js/node/http2.ts:3118-3130— The PR correctly forcesenablePush: falsein the wire SETTINGS frame (constructor spread at line 3127), butServerHttp2Session.#localSettingsis still initialized withenablePush: true(the shared default), sosession.localSettings.enablePushreturnstruein asessionevent handler even though the wire already advertisedENABLE_PUSH=0. Fix: change the#localSettingsfield initializer inServerHttp2SessionfromenablePush: truetoenablePush: falseto match the actual wire behavior from the start.Extended reasoning...
The bug and how it manifests
ServerHttp2Sessionshares the same#localSettingsfield initializer asClientHttp2Session, which defaults toenablePush: true. Before this PR, both the wire SETTINGS frame and#localSettingsagreed onenablePush: true— wrong per RFC 9113 §7.2.2 but internally consistent. This PR correctly fixes the wire by placingenablePush: falselast in the H2FrameParser settings spread (line 3127), so the initial SETTINGS frame now advertisesENABLE_PUSH=0. However,#localSettingsis never given a corresponding server-side override, so it still starts asenablePush: true. The PR therefore introduces a new inconsistency: the wire is correct, but the JS API disagrees until the client's SETTINGS ACK arrives and thelocalSettingshandler at line 2951 () overwrites the stale value.The specific code path that triggers it
sessionevent handlers run viaprocess.nextTick(line 3137:process.nextTick(emitConnectNT, this, socket)), which fires before any network I/O. At that point#localSettingsholds its initial value. Any code that readssession.localSettings.enablePushinside asessionhandler will observetruewhile the wire has already sentENABLE_PUSH=0:server.on("session", session => { console.log(session.localSettings.enablePush); // true — wrong, wire sent 0 });
Why existing code does not prevent it
The
localSettingscallback that corrects#localSettingsonly fires once the SETTINGS ACK round-trip completes. There is no mechanism to pre-seedServerHttp2Session.#localSettingswith server-specific defaults separate from the shared class initializer. Node.js initializes its server-sessionlocalSettingswithenablePush: falsefrom the very start.What the impact would be
The window from session creation until the first
localSettingsevent is narrow but real. It encompasses thesessionevent handler, which is the canonical place to inspect or reconfigure a newly established session. Code comparingsession.localSettings.enablePushagainstfalse(e.g., to guard push-related logic) would get the wrong answer during this window, diverging from Node.js behavior.How to fix it
Change the
#localSettingsfield initializer inServerHttp2SessionfromenablePush: truetoenablePush: false. This mirrors what Node.js does and ensures the JS API is consistent with the wire from the very first tick. ThelocalSettingshandler will continue to update#localSettingson SETTINGS ACK as before.Step-by-step proof
- Client connects; Bun creates a
ServerHttp2Sessionwhose#localSettings.enablePushistrue(the shared default). - The H2FrameParser constructor sends the initial SETTINGS frame with
ENABLE_PUSH=0(from the fixed spread{ ...options, ...options?.settings, enablePush: false }). process.nextTick(emitConnectNT, this, socket)queues thesessionevent before any I/O.- On the next tick the
sessionevent fires.session.localSettings.enablePushreads#localSettings→true. - Much later, the client sends a SETTINGS ACK. The
localSettingshandler fires and sets#localSettings = { ..., enablePush: false }. - After step 5,
session.localSettings.enablePushcorrectly returnsfalse— but the staletruewas observable in step 4.
- Client connects; Bun creates a
|
Tested this also fixed #29365 |
Collaborator
|
@robobun resume thsi PR and address the feedback from claude |
… override
- ServerHttp2Stream.respond(): use options?.waitForTrailers so respond(headers, null)
doesn't throw TypeError (typeof null === 'object' enters the else branch).
- ServerHttp2Session.settings(): run validateSettings() on the caller-supplied
argument BEFORE spreading { ...settings, enablePush: false }, so null/array/
primitive inputs still throw ERR_INVALID_ARG_TYPE instead of being silently
coerced to a bare { enablePush: false } SETTINGS frame.
- ServerHttp2Session #localSettings initializer: default enablePush to false so
session.localSettings.enablePush agrees with the wire (which already clamps
ENABLE_PUSH=0 in the constructor) before the peer's SETTINGS ACK arrives.
For 204/205/304/HEAD responses (or any respond() call where endStream ends
up true), the HEADERS frame carries END_STREAM and the native layer marks
the stream HALF_CLOSED_LOCAL. If waitForTrailers is still present in the
options forwarded to native request(), the Zig parser dispatches
onWantTrailers immediately after writing HEADERS+END_STREAM. The compat
layer's wantTrailers listener then calls sendTrailers({}) → noTrailers →
sendData('', true), emitting a spurious empty DATA frame on the
already-half-closed stream — an RFC 9113 §5.1 violation that strict peers
reject as STREAM_CLOSED.
The JS-side guard (options?.waitForTrailers && !endStream) only covers the
_final() path and runs AFTER the native call, so it can't stop the
synchronous native dispatch. Fix at the source: spread waitForTrailers:false
alongside endStream:true so native never fires onWantTrailers for these
responses.
Add a wire-level regression test that inspects the raw frames for a 204
response and asserts no DATA frame follows HEADERS+END_STREAM.
cirospaciari
previously approved these changes
Apr 17, 2026
…r-impl constraint
cirospaciari
approved these changes
Apr 17, 2026
structwafel
pushed a commit
to structwafel/bun
that referenced
this pull request
Apr 25, 2026
…en-sh#29075) Fixes oven-sh#29073. ## Repro ```js import http2 from "node:http2"; http2.createServer((req, res) => { res.writeHead(200, { "Content-Type": "text/plain; charset=utf-8" }); res.end("ok"); }).listen(8000); ``` `curl --http2-prior-knowledge http://localhost:8000` against Bun: ``` * Request completely sent off * nghttp2 recv error -902: The user callback function failed curl: (52) Empty reply from server ``` ## Cause Two distinct bugs on Bun's server side, either of which is enough for strict h2 peers (curl's nghttp2, Node's `http2.connect`) to reject the connection. They were only visible against strict peers — Bun's own `http2.connect` client tolerated both, which is why Bun-to-Bun tests and `Bun.serve({ http2: true })` worked. **1. Server SETTINGS advertised `ENABLE_PUSH=1`.** `FullSettingsPayload` defaults `enablePush = 1` for both client and server, and `ServerHttp2Session` never overrode it. RFC 9113 §7.2.2: > For a server, this initial value has no effect, and is equivalent to the value 0. Any value other than 0 for a server MUST be treated by a client as a connection error (Section 5.4.1) of type PROTOCOL_ERROR. nghttp2 reports this as `NGHTTP2_ERR_CALLBACK_FAILURE` on the first frame after the preface — exactly what curl surfaces. **2. `res.end(body)` emitted an extra empty DATA frame + a zero-length trailer HEADERS frame.** The compat layer always sets `waitForTrailers: true` on `stream.respond()` and then `onStreamTrailersReady` unconditionally calls `sendTrailers(this[kResponse][kTrailers])` with the response's (empty) trailers object. Bun's wire output was: ``` DATA "ok" flags=0 DATA len=0 flags=0 ← extra empty DATA from _final() HEADERS len=0 flags=END_STREAM ← zero-length trailer HPACK block ``` Node's wire output is: ``` DATA "ok" flags=0 DATA len=0 flags=END_STREAM ``` An empty trailer HEADERS frame has no valid HTTP fields, which nghttp2 rejects; even when that's fixed, emitting an extra empty DATA frame without END_STREAM *followed by* another empty DATA with END_STREAM trips nghttp2's strict state machine. ## Fix All changes are in `src/js/node/http2.ts`: - **`ServerHttp2Session`** constructor now defaults its initial parser settings to `enablePush: false`. User-provided settings still override (`{ enablePush: false, ...options, ...options?.settings }`). - **`ServerHttp2Stream.respond()`** tags the stream with a private `bunHTTP2WaitForTrailers` symbol when called with `waitForTrailers: true`. - **`Http2Stream._final()`** checks that flag. When set, it skips `native.writeStream(id, "", "ascii", true, cb)` (which would emit the extra empty DATA) and instead drives the wantTrailers path directly — either emitting `wantTrailers` (so user handlers can call `sendTrailers(realTrailers)`) or calling native `noTrailers` if no listener. The compat layer's `onStreamTrailersReady` still calls `sendTrailers({})` as before; the short-circuit below routes that to the single-empty-DATA-END_STREAM path. - **`Http2Stream.sendTrailers()`** short-circuits to native `noTrailers` when the processed header object has zero keys, matching Node's wire output (single empty DATA + END_STREAM, no trailer HEADERS frame). Non-empty trailers still go through the normal `sendTrailers` native binding. Client streams are unaffected — the `bunHTTP2WaitForTrailers` flag is only set from `respond()`, which clients never call. ## Verification After the fix, Bun's wire against the same curl command: ``` SETTINGS (enablePush=0) SETTINGS ACK HEADERS (flags=END_HEADERS) DATA "ok" (flags=0) DATA len=0 (flags=END_STREAM) ``` `curl --http2-prior-knowledge` returns `ok`. ### Regression test — `test/regression/issue/29073.test.ts` Two tests: 1. A raw-TCP prior-knowledge client (mirrors what curl does) sends the h2 preface + SETTINGS + HEADERS, then parses the server's frames and asserts: - no `ENABLE_PUSH != 0` entry in the server's initial SETTINGS, - exactly one HEADERS frame on stream 1 (the response — no trailer HEADERS), - the body bytes spell `ok`, - the last DATA frame on stream 1 carries END_STREAM. 2. Bun's own `http2.connect` client against the same server reads the response body end-to-end. ```sh bun bd test test/regression/issue/29073.test.ts # pass USE_SYSTEM_BUN=1 bun test test/regression/issue/29073.test.ts # fail: Expected: 0 Received: 1 at the ENABLE_PUSH assertion ``` Existing `test/js/node/test/parallel/test-http2-trailers*` tests (non-empty trailers) and `test-http2-compat-serverresponse-{end,close,finished}.js` all continue to pass under the debug build. --- ## Likely related issues - oven-sh#21759 — `@grpc/grpc-js` server on Bun emits repeated empty HTTP/2 DATA frames; Envoy aborts with `PROTOCOL_ERROR` / "Too many consecutive frames with an empty payload". The extra-empty-DATA bug (`_final` always writing an empty close frame even with `waitForTrailers: true`) matches the symptom precisely — Envoy's empty-frame guardrail is exactly what this PR removes. - oven-sh#19208 — `ERR_HTTP2_PROTOCOL_ERROR 200` for browser-served assets in a docker container. The `ENABLE_PUSH=1` fix is a plausible match if HTTP/2 is in play (reporter's stack is express serving a vite bundle, so http/2 isn't guaranteed), but without a repro I'm not auto-closing it. Not using `Fixes #` on either — leaving it to the reporters / verifiers to confirm on canary once this merges. --------- Co-authored-by: robobun <robobun@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com> Co-authored-by: Ciro Spaciari MacBook <ciro@anthropic.com> Co-authored-by: Ciro Spaciari <ciro.spaciari@gmail.com>
xhjkl
pushed a commit
to xhjkl/bun
that referenced
this pull request
May 14, 2026
…en-sh#29075) Fixes oven-sh#29073. ## Repro ```js import http2 from "node:http2"; http2.createServer((req, res) => { res.writeHead(200, { "Content-Type": "text/plain; charset=utf-8" }); res.end("ok"); }).listen(8000); ``` `curl --http2-prior-knowledge http://localhost:8000` against Bun: ``` * Request completely sent off * nghttp2 recv error -902: The user callback function failed curl: (52) Empty reply from server ``` ## Cause Two distinct bugs on Bun's server side, either of which is enough for strict h2 peers (curl's nghttp2, Node's `http2.connect`) to reject the connection. They were only visible against strict peers — Bun's own `http2.connect` client tolerated both, which is why Bun-to-Bun tests and `Bun.serve({ http2: true })` worked. **1. Server SETTINGS advertised `ENABLE_PUSH=1`.** `FullSettingsPayload` defaults `enablePush = 1` for both client and server, and `ServerHttp2Session` never overrode it. RFC 9113 §7.2.2: > For a server, this initial value has no effect, and is equivalent to the value 0. Any value other than 0 for a server MUST be treated by a client as a connection error (Section 5.4.1) of type PROTOCOL_ERROR. nghttp2 reports this as `NGHTTP2_ERR_CALLBACK_FAILURE` on the first frame after the preface — exactly what curl surfaces. **2. `res.end(body)` emitted an extra empty DATA frame + a zero-length trailer HEADERS frame.** The compat layer always sets `waitForTrailers: true` on `stream.respond()` and then `onStreamTrailersReady` unconditionally calls `sendTrailers(this[kResponse][kTrailers])` with the response's (empty) trailers object. Bun's wire output was: ``` DATA "ok" flags=0 DATA len=0 flags=0 ← extra empty DATA from _final() HEADERS len=0 flags=END_STREAM ← zero-length trailer HPACK block ``` Node's wire output is: ``` DATA "ok" flags=0 DATA len=0 flags=END_STREAM ``` An empty trailer HEADERS frame has no valid HTTP fields, which nghttp2 rejects; even when that's fixed, emitting an extra empty DATA frame without END_STREAM *followed by* another empty DATA with END_STREAM trips nghttp2's strict state machine. ## Fix All changes are in `src/js/node/http2.ts`: - **`ServerHttp2Session`** constructor now defaults its initial parser settings to `enablePush: false`. User-provided settings still override (`{ enablePush: false, ...options, ...options?.settings }`). - **`ServerHttp2Stream.respond()`** tags the stream with a private `bunHTTP2WaitForTrailers` symbol when called with `waitForTrailers: true`. - **`Http2Stream._final()`** checks that flag. When set, it skips `native.writeStream(id, "", "ascii", true, cb)` (which would emit the extra empty DATA) and instead drives the wantTrailers path directly — either emitting `wantTrailers` (so user handlers can call `sendTrailers(realTrailers)`) or calling native `noTrailers` if no listener. The compat layer's `onStreamTrailersReady` still calls `sendTrailers({})` as before; the short-circuit below routes that to the single-empty-DATA-END_STREAM path. - **`Http2Stream.sendTrailers()`** short-circuits to native `noTrailers` when the processed header object has zero keys, matching Node's wire output (single empty DATA + END_STREAM, no trailer HEADERS frame). Non-empty trailers still go through the normal `sendTrailers` native binding. Client streams are unaffected — the `bunHTTP2WaitForTrailers` flag is only set from `respond()`, which clients never call. ## Verification After the fix, Bun's wire against the same curl command: ``` SETTINGS (enablePush=0) SETTINGS ACK HEADERS (flags=END_HEADERS) DATA "ok" (flags=0) DATA len=0 (flags=END_STREAM) ``` `curl --http2-prior-knowledge` returns `ok`. ### Regression test — `test/regression/issue/29073.test.ts` Two tests: 1. A raw-TCP prior-knowledge client (mirrors what curl does) sends the h2 preface + SETTINGS + HEADERS, then parses the server's frames and asserts: - no `ENABLE_PUSH != 0` entry in the server's initial SETTINGS, - exactly one HEADERS frame on stream 1 (the response — no trailer HEADERS), - the body bytes spell `ok`, - the last DATA frame on stream 1 carries END_STREAM. 2. Bun's own `http2.connect` client against the same server reads the response body end-to-end. ```sh bun bd test test/regression/issue/29073.test.ts # pass USE_SYSTEM_BUN=1 bun test test/regression/issue/29073.test.ts # fail: Expected: 0 Received: 1 at the ENABLE_PUSH assertion ``` Existing `test/js/node/test/parallel/test-http2-trailers*` tests (non-empty trailers) and `test-http2-compat-serverresponse-{end,close,finished}.js` all continue to pass under the debug build. --- ## Likely related issues - oven-sh#21759 — `@grpc/grpc-js` server on Bun emits repeated empty HTTP/2 DATA frames; Envoy aborts with `PROTOCOL_ERROR` / "Too many consecutive frames with an empty payload". The extra-empty-DATA bug (`_final` always writing an empty close frame even with `waitForTrailers: true`) matches the symptom precisely — Envoy's empty-frame guardrail is exactly what this PR removes. - oven-sh#19208 — `ERR_HTTP2_PROTOCOL_ERROR 200` for browser-served assets in a docker container. The `ENABLE_PUSH=1` fix is a plausible match if HTTP/2 is in play (reporter's stack is express serving a vite bundle, so http/2 isn't guaranteed), but without a repro I'm not auto-closing it. Not using `Fixes #` on either — leaving it to the reporters / verifiers to confirm on canary once this merges. --------- Co-authored-by: robobun <robobun@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com> Co-authored-by: Ciro Spaciari MacBook <ciro@anthropic.com> Co-authored-by: Ciro Spaciari <ciro.spaciari@gmail.com>
This was referenced Jul 5, 2026
Closed
robobun
added a commit
that referenced
this pull request
Sep 19, 2026
Each comment keeps the fact that the code cannot show: the options copy is deliberate, paddingStrategy is a bun extension, the native writer is shared with request(), and statCheck can write to the options it gets. The paragraph from #29075 keeps its text. Only the sentence that named a guard this branch removed is new.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #29073.
Repro
curl --http2-prior-knowledge http://localhost:8000against Bun:Cause
Two distinct bugs on Bun's server side, either of which is enough for strict h2 peers (curl's nghttp2, Node's
http2.connect) to reject the connection. They were only visible against strict peers — Bun's ownhttp2.connectclient tolerated both, which is why Bun-to-Bun tests andBun.serve({ http2: true })worked.1. Server SETTINGS advertised
ENABLE_PUSH=1.FullSettingsPayloaddefaultsenablePush = 1for both client and server, andServerHttp2Sessionnever overrode it. RFC 9113 §7.2.2:nghttp2 reports this as
NGHTTP2_ERR_CALLBACK_FAILUREon the first frame after the preface — exactly what curl surfaces.2.
res.end(body)emitted an extra empty DATA frame + a zero-length trailer HEADERS frame. The compat layer always setswaitForTrailers: trueonstream.respond()and thenonStreamTrailersReadyunconditionally callssendTrailers(this[kResponse][kTrailers])with the response's (empty) trailers object. Bun's wire output was:Node's wire output is:
An empty trailer HEADERS frame has no valid HTTP fields, which nghttp2 rejects; even when that's fixed, emitting an extra empty DATA frame without END_STREAM followed by another empty DATA with END_STREAM trips nghttp2's strict state machine.
Fix
All changes are in
src/js/node/http2.ts:ServerHttp2Sessionconstructor now defaults its initial parser settings toenablePush: false. User-provided settings still override ({ enablePush: false, ...options, ...options?.settings }).ServerHttp2Stream.respond()tags the stream with a privatebunHTTP2WaitForTrailerssymbol when called withwaitForTrailers: true.Http2Stream._final()checks that flag. When set, it skipsnative.writeStream(id, "", "ascii", true, cb)(which would emit the extra empty DATA) and instead drives the wantTrailers path directly — either emittingwantTrailers(so user handlers can callsendTrailers(realTrailers)) or calling nativenoTrailersif no listener. The compat layer'sonStreamTrailersReadystill callssendTrailers({})as before; the short-circuit below routes that to the single-empty-DATA-END_STREAM path.Http2Stream.sendTrailers()short-circuits to nativenoTrailerswhen the processed header object has zero keys, matching Node's wire output (single empty DATA + END_STREAM, no trailer HEADERS frame). Non-empty trailers still go through the normalsendTrailersnative binding.Client streams are unaffected — the
bunHTTP2WaitForTrailersflag is only set fromrespond(), which clients never call.Verification
After the fix, Bun's wire against the same curl command:
curl --http2-prior-knowledgereturnsok.Regression test —
test/regression/issue/29073.test.tsTwo tests:
ENABLE_PUSH != 0entry in the server's initial SETTINGS,ok,http2.connectclient against the same server reads the response body end-to-end.Existing
test/js/node/test/parallel/test-http2-trailers*tests (non-empty trailers) andtest-http2-compat-serverresponse-{end,close,finished}.jsall continue to pass under the debug build.Likely related issues
@grpc/grpc-jsserver on Bun emits repeated empty HTTP/2 DATA frames; Envoy aborts withPROTOCOL_ERROR/ "Too many consecutive frames with an empty payload". The extra-empty-DATA bug (_finalalways writing an empty close frame even withwaitForTrailers: true) matches the symptom precisely — Envoy's empty-frame guardrail is exactly what this PR removes.ERR_HTTP2_PROTOCOL_ERROR 200for browser-served assets in a docker container. TheENABLE_PUSH=1fix is a plausible match if HTTP/2 is in play (reporter's stack is express serving a vite bundle, so http/2 isn't guaranteed), but without a repro I'm not auto-closing it.Not using
Fixes #on either — leaving it to the reporters / verifiers to confirm on canary once this merges.