Repository navigation
node:http2: put END_STREAM on the HEADERS frame of an endStream request even with waitForTrailers - #37718
node:http2: put END_STREAM on the HEADERS frame of an endStream request even with waitForTrailers#37718robobun wants to merge 1 commit into
Conversation
…st even with waitForTrailers When a client request was submitted with endStream (explicitly, or by default for GET/HEAD/DELETE...) together with waitForTrailers, native request() left END_STREAM off the HEADERS frame, marked the stream HALF_CLOSED_LOCAL anyway and dispatched onWantTrailers, so the stream was terminated by a trailer HEADERS frame (or, without a 'wantTrailers' listener, an empty END_STREAM DATA frame) and 'wantTrailers' fired for a request that has no body. Node puts END_STREAM on the request HEADERS and never asks for trailers: an endStream stream is submitted without the nghttp2 data provider that trailers are requested through. request() now sets END_STREAM whenever endStream is set and only records waitForTrailers on the stream when the HEADERS frame did not already end it, for client and server alike; the server-side respond() no longer needs to strip waitForTrailers itself.
|
Updated 9:31 PM PT - Aug 11th, 2026
❌ @robobun, your commit 768d0f3 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 37718That installs a local version of the PR into your bun-37718 --bun |
|
Status: fix pushed; builds and Linux test lanes (incl. ASAN/musl) green, Windows and darwin test lanes still running. Reproduced with a raw |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughChanges
HTTP/2 stream termination
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
One correction to the walkthrough above, for anyone skimming: |
There was a problem hiding this comment.
LGTM — focused node-compat fix that moves the endStream/waitForTrailers rule into native request() where it belongs.
What was reviewed:
- Traced the new
stream.wait_for_trailers = wait_for_trailers && !end_streamassignment against every consumer (send_data,flush_queue,no_trailers) — the field is now set once, at the same or later point than the old code, so early-error returns (memory limit, header block length) are unaffected. - Confirmed removing the server-side JS
waitForTrailers: falsestrip is safe: native now clears it, and the JSoptions?.waitForTrailers && !endStreamguard at http2.ts:3841 still keepsbunHTTP2WaitForTrailersoff. - Checked the removed
onWantTrailersearly-return: the endStream path now falls through to the existing HALF_CLOSED_LOCAL/CLOSED transition +onStreamEnddispatch, which is what makes'finish'fire (asserted by the new tests).
Extended reasoning...
Overview
This PR fixes a node:http2 client-side wire-format divergence: when a request is submitted with both endStream (explicit or defaulted for GET/HEAD/etc.) and waitForTrailers, Bun was leaving END_STREAM off the initial HEADERS frame and then emitting 'wantTrailers', resulting in a trailer HEADERS block or empty END_STREAM DATA frame following the request headers. Node puts END_STREAM directly on the HEADERS frame and never fires 'wantTrailers' for such a stream (nghttp2 has no data provider to request trailers through).
The Rust change in H2FrameParser::request() (h2_frame_parser.rs:9251-9620) is ~15 net lines: (1) always set the END_STREAM flag when endStream is true regardless of waitForTrailers, (2) record stream.wait_for_trailers only when the HEADERS frame did not end the stream, and (3) remove the special-case branch that dispatched onWantTrailers and returned early. The endStream path now falls through to the pre-existing state transition and onStreamEnd dispatch — the same code the server side already used via its is_server exception, which is now deleted.
The JS change in ServerHttp2Stream.respond() (http2.ts:3812-3818) removes the waitForTrailers: false strip that was working around the old native behavior; the native layer now enforces the rule for both client and server, and the existing options?.waitForTrailers && !endStream guard further down keeps the JS-side trailers flag off. Server wire output is unchanged (covered by the new server test).
Security risks
None. This is HTTP/2 frame-emission ordering on the outbound side; no untrusted-input parsing changed. The change makes Bun emit fewer frames (one HEADERS instead of HEADERS + trailer/DATA), reducing surface if anything.
Level of scrutiny
Medium. node:http2 is production networking code and protocol-level mistakes surface as interop failures with real servers (grpc, envoy, etc.). That said: the change is small and localized to one function, the invariant is moved to the layer that owns it (native request()) rather than patched at each caller, the old server-side is_server special case and the JS workaround both collapse into one rule, and the PR description documents empirical verification against node v26.3.0 plus passing runs of the full test/js/node/http2/ suite, all 261 upstream test-http2-* tests, and the grpc-js suites.
Other factors
The six new wire-level tests in h2-conformance.test.ts are strong: they use the file's existing raw net server/client harness to assert exact frame sequences on stream 1, use a PING/ACK barrier to prove the frame list is complete before asserting, cover the variant matrix (implicit vs explicit endStream, with/without listener, before/after connect, client and server), pin the still-working body-with-trailers path, and check the specific error codes (ERR_HTTP2_TRAILERS_NOT_READY / ERR_HTTP2_INVALID_STREAM / ERR_HTTP2_TRAILERS_ALREADY_SENT) that distinguish the fixed behavior from the old. The PR notes these fail on the unfixed binary. The related-PR cross-references (#37712, #36389) were checked for interaction and the description explains why both stay green.
|
Thanks for the review. Nothing to change from it. CI so far: every build lane and all Linux test lanes (including the ASAN and musl ones) are green; the Windows and darwin test lanes are still running. |
|
Closing this PR because it has been inactive for more than 90 days. |
Problem
node:http2client request withendStream(explicit, or implied for GET/HEAD/DELETE/OPTIONS) pluswaitForTrailersgoes out asHEADERS(flags=0x4)and then a second terminating frame: a trailerHEADERS(flags=0x5), or an empty END_STREAMDATAframe with no'wantTrailers'listener. Node sends oneHEADERS(flags=0x5).'wantTrailers'fires on such a stream (node: never). A latersendTrailers()throwsERR_HTTP2_TRAILERS_ALREADY_SENTwhere node throwsERR_HTTP2_TRAILERS_NOT_READY; with no listener it does not throw and writes a trailer block after the stream was already ended.endStreamwithwaitForTrailersmeant "end the stream after the trailers". The HEADERS frame went out without END_STREAM, the stream was marked half-closed although nothing on the wire had closed it, and JS was asked for trailers. The server side already had an exception.Fix
endStreamnow always puts END_STREAM on the HEADERS frame, client and server alike, and a stream only rememberswaitForTrailerswhen that frame did not end it. The deferred want-trailers branch is removed.sendTrailers()reportsERR_HTTP2_TRAILERS_NOT_READY. Node behaves the same, and its docs place trailers after the final DATA frame, which such a stream does not have.respond()no longer stripswaitForTrailersitself; the native rule covers it and its wire output is unchanged.respond({ endStream: true, waitForTrailers: true })pin the paths that must not change. Outcomes were checked against node v26.3.0; the existing bun, upstream node and grpc-js http2 suites still pass.Background
waitForTrailersstops the stream from ending by itself after the body:'wantTrailers'is emitted and the app callssendTrailers()to send the closing block.endStreamdeclares there is no body.src/js/node/http2.ts(the node-facing API) and a native frame parser that builds frames and tracks per-stream state. One native entry point serves both the client'srequest()and the server'srespond(), so a rule there covers both sides.Original description
What
A
node:http2client request submitted withendStream(explicitly, or by default for GET/HEAD/DELETE/OPTIONS...) together withwaitForTrailersnow goes out the way node sends it: END_STREAM on the request HEADERS frame, no trailers requested, no'wantTrailers'event.Repro
Raw
netserver that sends SETTINGS after the preface, ACKs the client's SETTINGS and logs every frame it receives on stream 1:Same with
{ ":method": "POST" }and{ endStream: true, waitForTrailers: true }.HEADERS(flags=0x5)HEADERS(flags=0x4), trailerHEADERS(flags=0x5)'wantTrailers'listenerHEADERS(flags=0x5)HEADERS(flags=0x4),DATA(len 0, END_STREAM)'wantTrailers'emittedsendTrailers()ERR_HTTP2_TRAILERS_NOT_READYERR_HTTP2_TRAILERS_ALREADY_SENT(with a listener); with no listener it does not throw and writes a trailer HEADERS frame after the END_STREAM DATA frameCause
H2FrameParser::request()(src/runtime/api/bun/h2_frame_parser.rs) handledendStreamas "end the stream after the trailers" on the client: it left END_STREAM off the HEADERS frame whenwaitForTrailerswas set (the server already had anis_serverexception), and after writing the frame it set the stream to HALF_CLOSED_LOCAL, although nothing had ended it on the wire yet, and dispatchedonWantTrailers. The JSwantTrailershandler then terminated the stream with a trailer block or an empty END_STREAM DATA frame.Fix
request()puts END_STREAM on the HEADERS frame wheneverendStreamis set, and only recordswaitForTrailerson the stream when that frame did not end it (stream.wait_for_trailers = wait_for_trailers && !end_stream). The deferredonWantTrailersbranch is gone, so anendStreamstream takes the same path as one withoutwaitForTrailers: state transition plusonStreamEnd, the writable side finishes, andsendTrailers()reportsERR_HTTP2_TRAILERS_NOT_READYbecause trailers were never requested.The rule now lives in one place and applies to both sides, so the server-side
respond()insrc/js/node/http2.tsno longer has to stripwaitForTrailersitself before calling into native (its existingoptions?.waitForTrailers && !endStreamguard still keeps the JS-side trailers flag off). The wire output ofrespond({ endStream: true, waitForTrailers: true })is unchanged.Why this is the right behavior
This is what node does for both
request()andrespond(): withendStream,Http2Session::SubmitRequest/Http2Stream::SubmitResponsepassSTREAM_OPTION_EMPTY_PAYLOAD, which makes thenghttp2_data_providernull, so nghttp2 sets END_STREAM on the HEADERS frame and the trailers callback (which hangs off the data provider) can never run.waitForTrailerstherefore has no effect on such a stream, which also matches its documentation: trailers are sent "after the final DATA frame", and anendStreamstream has none. Requests that do carry a body keep requesting trailers exactly as before (stream.wait_for_trailersis still set for them; the upstream trailer tests cover that path).Verification
New
endStream with waitForTrailersblock intest/js/node/http2/h2-conformance.test.ts, wire-level against the file's raw server/client. After the client's HEADERS frame arrives the raw server sends a PING; anything the client wrote while submitting the request is ahead of the PING ACK on the wire, so once the ACK arrives the stream-1 frame list is complete.waitForTrailers(impliedendStream), POST withendStream: true, waitForTrailers: true, the same without a'wantTrailers'listener, and a request submitted after'connect'(direct submission rather than the queued-until-connect path): the stream-1 frames are exactlyHEADERS(0x5),'wantTrailers'never fires,'finish'still fires,sendTrailers()throwsERR_HTTP2_TRAILERS_NOT_READY,sentTrailersis undefined. These four fail on the unfixed binary with the frame lists from the table above.'wantTrailers'and sends its trailers (HEADERS,DATA, trailerHEADERS(0x5)), pinning the path that is still supposed to request trailers.respond({ endStream: true, waitForTrailers: true })still produces exactlyHEADERS(0x5)with no'wantTrailers', covering the removed JS strip.Outcomes of every case were checked against node v26.3.0. Also passing with the fix:
test/js/node/http2/(all files), all 261 upstreamtest-http2-*tests intest/js/node/test/{parallel,sequential}, and the grpc-jstest-server,test-end-to-endandtest-metadatasuites.Related: #37712 fixes the empty DATA frame emitted on the body-carrying trailers path and lists this flow as out of its scope; #36389's "GET with waitForTrailers still half-closes its side" test accepts END_STREAM on either HEADERS or DATA, so it stays green with this change.