node:http2: record empty trailers as sent when the stream ends without a 'wantTrailers' listener - #37722
node:http2: record empty trailers as sent when the stream ends without a 'wantTrailers' listener#37722robobun wants to merge 7 commits into
Conversation
…er ends the stream
…body instead of a flow-control stall
|
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 (2)
WalkthroughThe HTTP/2 implementation centralizes trailer handling for client and server streams. The conformance suite verifies trailer submission errors, automatic empty completion, stream closure, event ordering, and emitted frames. ChangesHTTP/2 trailer lifecycle
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: reproduced and fixed; the diff is green, and this is ready for a maintainer. Reproduced with the raw-peer script in the description against bun 1.4.0 and current main: after the no-listener The first three of the five tests added to CI: the code has been unchanged since 73280c1. Build 93354 for that commit passed 191 jobs (darwin-14-aarch64 included, which runs these tests); its only red job was Reviews: claude[bot] and CodeRabbit both reviewed the final code with no findings; all review threads are resolved. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused Node compat fix that consolidates the three wantTrailers dispatch sites into one helper and records sentTrailers = {} before the native noTrailers() call.
What was reviewed:
- Verified the new
[kWantTrailers]helper is inside theHttp2Streamclass body so#sentTrailers/#idprivate-field access is valid, and that all three former call sites (client session handler, server session handler,_final) now route through it. - Checked the
if (native)guard vs. the old?.noTrailers()— the emit-with-null-native case is unreachable (the callback is invoked from native) and a listener'ssendTrailers()would no-op there anyway. - Confirmed
sendTrailers()checksdestroyed || closedbefore#sentTrailers, so the closed-stream path still reportsERR_HTTP2_INVALID_STREAMas node does. - Tests await wire frames / PING-ACK barriers (no sleeps), assert exact frame sequences, and cover all three paths.
Extended reasoning...
Overview
The PR fixes a Node.js compat gap in src/js/node/http2.ts: when a waitForTrailers stream ends without a 'wantTrailers' listener, Bun's session-level handlers called native noTrailers() without recording #sentTrailers, so a later sendTrailers() would write a trailer HEADERS frame on an already half-closed stream (RFC 9113 §5.1/§8.1 violation). The _final path did record it, but after the native call, leaving a 'prefinish' window. The fix extracts one [kWantTrailers](native) helper on Http2Stream that (a) sets #sentTrailers = {} before noTrailers(), (b) applies the StreamState.Closed guard uniformly (previously only _final had it), and (c) is called from all three sites. Net production diff is ~16 lines added and ~24 removed — mostly deduplication.
Security risks
None. This is JS-side bookkeeping in the node:http2 compat layer that tightens behavior toward the spec (rejecting a late sendTrailers() instead of writing an extra frame). No new user input is parsed; no auth/crypto/permission surface.
Level of scrutiny
Medium. node:http2 is production-facing, but the change is a narrow refactor-plus-ordering-fix at the JS bookkeeping layer with no native changes. I verified: private-field access from the symbol-keyed method works because it's declared in the class body; the sendTrailers() guard order (closed → #sentTrailers → WantTrailer) means the closed-stream branch still throws ERR_HTTP2_INVALID_STREAM; and the one behavioral difference in the null-native edge (previously would still emit, now skips) is unreachable since these callbacks are dispatched from native, and even if reached the listener's sendTrailers() would no-op on session[bunHTTP2Native]?..
Other factors
Three new wire-level tests in h2-conformance.test.ts cover each path (client via native dispatch, server via _final/'prefinish', closed stream not emitting), all awaiting real frames with a PING-ACK barrier before asserting the exact stream-1 frame sequence — no sleeps, try/finally cleanup, matches the file's existing harness conventions. The PR description confirms they fail on main and pass with the fix, and that the existing test-http2-trailers* / test-http2-no-wanttrailers-listener upstream tests still pass. No prior reviewer comments; bug-hunting system found nothing.
|
Follow-up on verification: the full debug build has since completed here, and |
…inal chunk on a connected stream
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused Node-compat fix that consolidates three copies of the wantTrailers no-listener path into one helper and records sentTrailers = {} before the native noTrailers() call.
What was reviewed:
- Verified the helper's ordering change (set
#sentTrailersbeforenoTrailers()) is what makes the'prefinish'case throwERR_HTTP2_TRAILERS_ALREADY_SENT, and thatsendTrailers()checksclosedfirst so the closed-stream branch setting#sentTrailersis not observably wrong. - Checked the
if (native)guard at both session call sites: previously the listener branch emitted regardless ofnative, but a listener'ssendTrailers()no-ops on a null native anyway, so no behavior lost. - Confirmed the auto-close hook at line ~3364 (
WantTrailer && sentTrailers === undefined) is unaffected — the session handlers already setWantTrailerbefore calling the helper, and the helper now setssentTrailers, which is the state that lets auto-close proceed. - Tests cover all three call sites (client native dispatch, server
_final, closed-stream) with wire-level frame assertions and a PING barrier.
Extended reasoning...
Overview
This PR fixes a Node.js compatibility gap in node:http2: when a waitForTrailers stream ends without a 'wantTrailers' listener, Bun's session-level handlers called native noTrailers() without recording #sentTrailers, so a later sendTrailers() passed the guard and wrote a trailer HEADERS frame on an already half-closed stream (RFC 9113 §8.1 violation). Node throws ERR_HTTP2_TRAILERS_ALREADY_SENT in this case.
The fix extracts one Http2Stream#[kWantTrailers] helper backing all three sites (ClientHttp2Session, ServerHttp2Session, and _final). The helper sets #sentTrailers = {} before calling native.noTrailers() (fixing the _final path's ordering bug where 'prefinish' fires inside that call), and applies _final's existing closed-stream rule to the session handlers (matching Node's onStreamTrailers early return). Net: ~15 lines removed from src, one 8-line helper added, three wire-level tests added.
Security risks
None. This is JS-side bookkeeping in the http2 stream lifecycle — no parsing of untrusted input, no auth/crypto, no new external surface. The change makes behavior stricter (rejects a late sendTrailers() that previously succeeded and wrote a spec-violating frame).
Level of scrutiny
Medium. HTTP/2 stream lifecycle is subtle, but this change is a consolidation + ordering fix, not new logic. Each of the three call sites already had a variant of this code; the helper is the union of their correct behaviors. I traced:
sendTrailers()checksdestroyed || closedbefore#sentTrailers, so setting#sentTrailers = {}on the closed-stream branch doesn't change which error a caller sees there.- The
if (native)guard replaces?.— previously the listener branch would emit even with a null native, but the listener'ssendTrailers()is itself a no-op on null native, so nothing observable is lost. - Private-field access (
this.#sentTrailers,this.#id) from a computed-name method on the same class is valid. - The auto-close hook that gates on
sentTrailers === undefinednow correctly proceeds on the client no-listener path (previously it would have held indefinitely there, though the closed-stream teardown masks that in practice).
Other factors
- Three new tests in
h2-conformance.test.tscover each call site with raw-peer wire assertions (exact frame sequence + PING barrier to prove nothing else was sent). The PR description confirms all three fail on main. - The full h2-conformance suite and the upstream Node trailer tests were run against the debug build.
- No CODEOWNERS entry for this path.
- The comment-cop bot flagged earlier revisions of the helper's comment; the author iteratively shortened it to two lines, which is reasonable documentation of non-obvious Node-matching behavior (not a workaround justification). No human reviewer comments are outstanding.
|
Pushed since the last update:
The PR description's verification section is updated to match. With the current build: h2-conformance.test.ts 64/64 and node-http2.test.js 356 pass, 0 fail. |
…ke node close() already arranges for the RST_STREAM, and the writable finishes through _final / the EndStreamSent short-circuit, so ending the stream with an empty END_STREAM frame first only told the peer the message was complete right before resetting it. Covers the listener-calls-close() contract on both sides.
|
Pushed 73280c1 after another self-review pass over the closed-stream branch of the helper. The previous revision still ended a closed stream with an empty END_STREAM DATA frame before close()'s RST_STREAM went out (carried over from the old _final code, and newly applied to the session handlers). That frame turned out to be unnecessary: the writable finishes through _final's own callback (server path) or the EndStreamSent short-circuit (client dispatch path) either way, and close() has already scheduled the RST. Node's onStreamTrailers simply returns for a closed stream, so the body frames are followed by RST_STREAM(NO_ERROR) alone and sentTrailers stays undefined. The helper now does the same on both paths; the no-listener fix itself is unchanged. The third test's expectations follow node's wire, and two tests were added for a listener that calls close() instead of sendTrailers() (client and server), which had no coverage. Verified with the debug build: h2-conformance.test.ts (66 tests), node-http2.test.js, and the 256 upstream test-http2-*.js files. The description is updated accordingly. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes HTTP/2 stream-state handling (the closed-stream branch in _final no longer calls noTrailers() and now relies on close()'s RST_STREAM alone), a human look at that transition would still be worthwhile.
What was reviewed:
- Traced all three call sites into
[kWantTrailers]:#sentTrailers = {}now precedesnoTrailers(), closing the'prefinish'window; theWantTrailerflag still guards double-dispatch. - Verified the closed-stream early return still lets
_final's callback run so'finish'fires and the deferredrstNextTicksends the RST_STREAM;sendTrailers()on that stream hitsERR_HTTP2_INVALID_STREAMvia thethis.closedguard, so leaving#sentTrailersunset there is fine. - Checked the
if (native)guard in the session handlers: previously a null native still emitted'wantTrailers'; now it does not, but the handler is only reached from a native dispatch sonativeis present. - Confirmed the five wire-level tests await real frame arrivals (PING barrier / RST) and assert exact frame sequences with no sleeps.
Extended reasoning...
Overview
The PR consolidates three near-identical wantTrailers handling sites in src/js/node/http2.ts — Http2Stream#_final, and the ClientHttp2Session / ServerHttp2Session native-dispatch handlers — into a single Http2Stream#[kWantTrailers] helper. The functional changes are: (1) #sentTrailers = {} is recorded before the native noTrailers() call rather than after (or not at all in the session handlers), so a sendTrailers() fired from inside that call (e.g. from 'prefinish') hits ERR_HTTP2_TRAILERS_ALREADY_SENT; and (2) a stream already marked Closed returns early without calling noTrailers() or emitting the event, matching node's onStreamTrailers. Five new wire-level tests in h2-conformance.test.ts assert exact frame sequences against a raw peer.
Security risks
None identified. This is bookkeeping in the JS layer of node:http2; no untrusted-input parsing, no auth/crypto, no resource-limit changes. The fix removes a way to write a trailer HEADERS frame onto an already half-closed stream, which is a protocol-conformance improvement.
Level of scrutiny
Moderate-to-high. node:http2 is a production Node-compat module and HTTP/2 stream state transitions are subtle. The change is small (~20 net runtime lines) and is a deduplicating refactor plus an ordering fix, but the closed-stream branch changes observable wire behavior in _final: previously a closed waitForTrailers stream reaching _final sent an empty DATA(END_STREAM) and recorded sentTrailers = {}; now it sends nothing and leaves sentTrailers undefined, relying on close()'s RST_STREAM to terminate. This matches node and is covered by the third and fourth new tests, but it is a real behavior change that a maintainer familiar with the http2 stream lifecycle should confirm.
Other factors
- The PR description's "such a stream is still ended with the empty END_STREAM DATA frame" line is stale relative to the tip commit 73280c1, which changed the closed-stream branch to skip that frame entirely; the tests and the code comment reflect the current behavior.
- The unresolved
comment-copbot comment on line 2637 flags the 3-line helper comment; the author already justified keeping a shortened version in earlier (resolved) threads, and the current comment is a one-sentence node source reference, not a workaround justification. - CI on 9e1bb6b passed on all lanes that ran;
h2-conformance.test.ts,node-http2.test.js, and the upstreamtest-http2-trailers*/test-http2-no-wanttrailers-listenersuites were reported passing with the debug build. - No prior review from me on this PR.
|
The description was refreshed for 73280c1 a few minutes before that review landed, so the sentence it quotes about still sending the END_STREAM frame is gone; the Fix section now describes the early return and why the frame was not needed. Build 93354 for 73280c1 has 190 jobs green so far with the remaining 4 waiting for agents; no review threads are open. |
|
Build 93539 (the re-run for the empty commit e8c7c68) failed before compiling anything: every build agent got "Failed to download after 5 attempts" fetching c-ares, mimalloc and WebKit from github.com, the same connectivity problem that broke the protoc download in build 93354. The code is unchanged since 73280c1, which built and passed on all lanes that ran in build 93354. The status comment at the top has the full picture; nothing further to push from my side. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The [kWantTrailers] consolidation and the #sentTrailers = {}-before-noTrailers() ordering fix look correct and match node's onStreamTrailers. Because the closed-stream early-return in _final changes wire output (drops the noTrailers() call that previously wrote the END_STREAM DATA frame), a maintainer look at the HTTP/2 stream-state interaction would still be worthwhile.
What was reviewed:
- The three call sites (both session
wantTrailershandlers and_final) now route through one helper; verified_final's callback still fires via line 2940 when the helper early-returns on a closed stream. - The
if (native)guard in the session handlers replacing?.— native dispatches these, soself[bunHTTP2Native]being null there is not reachable in practice. - The file-response auto-close guard at
sentTrailers === undefined(line ~3366) — the new ordering means it now passes through on the no-listener client path where it previously held; this looks like an incidental fix rather than a regression.
Extended reasoning...
Overview
The PR consolidates three copies of the wantTrailers dispatch (in Http2Stream#_final, ServerHttp2Session's handler, and ClientHttp2Session's handler) into one Http2Stream#[kWantTrailers] helper that mirrors node's onStreamTrailers. Two behavior changes: (1) #sentTrailers = {} is now recorded before the native noTrailers() call rather than after (closing the 'prefinish' race), and applied on the session-handler path where it was previously missing entirely; (2) a stream already marked StreamState.Closed is left alone — no noTrailers() call, no event, no sentTrailers — matching node's early return. Five wire-level tests are added covering client/server × no-listener/listener-close.
Security risks
None. This is JS-side bookkeeping for the trailer-sent flag and event emission; no parsing of untrusted input, no auth/crypto.
Level of scrutiny
Medium-high. The runtime diff is small (~19 net lines) and directly ports node's shape with a source citation, but it sits inside _final's synchronous-reentry maze (the surrounding code has extensive comments about markWritableDone racing the callback stash) and changes what goes on the wire for a close()d stream. The PR itself went through a self-review iteration (73280c1) that revised exactly this closed-stream branch, which suggests the interaction is subtle enough to merit a second reader.
Other factors
The verification is unusually thorough: fails-on-main/passes-on-PR evidence for both ASAN and release builds, wire-level frame-sequence assertions with PING barriers, and a full run of the 256 upstream test-http2-*.js files plus node-http2.test.js. All comment-cop threads are resolved. CI is green on 190+ jobs. I traced the _final closed-stream path and confirmed the writable's callback still fires at line 2940 after the helper's early return, and checked that the sentTrailers === undefined guard in maybeScheduleFileResponseClose now passes through (rather than holding) on the client no-listener path — that reads as a fix, not a break. No concrete issue found; deferring only because HTTP/2 stream-state changes benefit from a maintainer familiar with the native noTrailers/writeStream contract confirming the dropped call is safe on every _final entry shape.
|
On the one open question in that review (is skipping
The native side is also not put into a new state by this: a stream left open with On the auto-close guard ( |
This branch has not been deployed
1 participant
Problem
waitForTrailersstream that ends its body with no'wantTrailers'listener still accepts a latersendTrailers(): a trailerHEADERSframe goes out on a stream bun already half-closed with anEND_STREAMDATAframe (RFC 9113 5.1 / 8.1). Node throwsERR_HTTP2_TRAILERS_ALREADY_SENTand leavessentTrailersas{}.sendTrailers(), and the native send has no stream-state check.sendTrailers()issued from'prefinish'(which fires inside that call) still reached the wire.'wantTrailers'on a stream that had beenclose()d before its last chunk went out. Node does not ask a closed stream for trailers.Fix
onStreamTrailers: a closed stream is left alone; with no listener, recordsentTrailers = {}and then end the stream; otherwise emit'wantTrailers'.sendTrailers()after that point, including one from inside the ending call, hits the existing already-sent guard and writes nothing. No native change.END_STREAMframe; the body is followed byclose()'sRST_STREAM(NO_ERROR)alone andsentTrailersstaysundefined, as in node. The server path used to send anEND_STREAMframe right before the reset.close()contract the helper now routes. The upstreamtest-http2-*suite was also run on a debug build.Background
waitForTrailers(an option torequest()/respond()) stops the stream from ending after the last body chunk. Instead it emits'wantTrailers'and the app callssendTrailers(headers), a finalHEADERSframe carryingEND_STREAM, orclose(). With no listener, the stream is expected to end itself with empty trailers.END_STREAM, the only thing it may still send on that stream isRST_STREAM. A trailerHEADERSframe after anEND_STREAMDATAframe is a protocol violation.sentTrailersis the stream property recording the trailers that went out. It doubles as the guard behindERR_HTTP2_TRAILERS_ALREADY_SENT;undefinedmeans the stream was never completed with trailers.close()on anHttp2StreamsendsRST_STREAM(NO_ERROR)once the writable finishes, so a closed stream needs noEND_STREAMframe to be torn down.[review] gate passed · iteration 4 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 4
evidence per changed file
Original description
Repro
A
waitForTrailersstream whose body finishes with no'wantTrailers'listener, followed by a latesendTrailers()(the peer here is a raw server that only completes the SETTINGS handshake, so the stream stays half-closed):sendTrailers()sentTrailersHEADERS,DATA(5),DATA(0, END_STREAM)ERR_HTTP2_TRAILERS_ALREADY_SENT{}HEADERS(END_STREAM){ "x-late": "1" }That extra trailer
HEADERSframe is written on a stream we already half-closed with theEND_STREAMDATAframe (RFC 9113 5.1 / 8.1).Cause
Node's
onStreamTrailershandles the no-listener case by callingstream.sendTrailers({})itself, which both ends the stream (emptyEND_STREAMDATAframe) and records the trailers as sent. Bun's session-levelwantTrailershandlers (ClientHttp2SessionandServerHttp2Sessioninsrc/js/node/http2.ts) called nativenoTrailers()directly without recording anything, so a latersendTrailers()passed both thesentTrailersandWantTrailerguards and nativesend_trailers()(which has no stream-state check) wrote the frame.Http2Stream#_finalhad its own copy of this logic that did record the trailers, but only after the native call returned, so asendTrailers()issued from inside that call (the writable's'prefinish'fires there) still went through. The client path is the one hit in practice: client streams always get theirwantTrailersthrough the native dispatch, server streams through_final.Fix
One
Http2Stream#[kWantTrailers]helper now backs all three sites (both session handlers and_final) and has the shape of node'sonStreamTrailers: a closed stream is left alone; otherwise, with nothing listening for'wantTrailers', it recordssentTrailers = {}and then callsnoTrailers(); otherwise it emits the event.The closed-stream rule is a small behavior change in its own right. The session handlers previously had no such rule, so a stream
close()d before its last chunk went out still got'wantTrailers'emitted, and a listener'ssendTrailers()could only throwERR_HTTP2_INVALID_STREAMfrom inside the native dispatch (the compat layer'sonStreamTrailersReadyis such a listener, attached to everyHttp2ServerResponse)._finaldid have the rule, but ended the stream with an emptyEND_STREAMDATAframe instead of asking; that frame was not needed for anything (the writable finishes through_final's own callback, or through theEndStreamSentshort-circuit on the dispatch path, andclose()has already arranged for theRST_STREAM), and it told the peer the message was complete right before resetting it. Node sends nothing there: the body frames are followed byRST_STREAM(NO_ERROR)alone andsentTrailersstaysundefined, which is what both paths do now. The same holds when a listenerclose()s the stream instead of sending trailers (the other half of the documented'wantTrailers'contract), which is unchanged here but was untested.No native change: the JS bookkeeping is the layer node enforces this at, and it is what makes the error code (
ERR_HTTP2_TRAILERS_ALREADY_SENTrather than a native throw) match.Related but separate: #37718 (endStream + waitForTrailers on request()) and #37712 (empty DATA frames / frameError stream id) change what happens before the trailer block is requested; this PR is about what happens after the no-listener path has ended the stream, and the handler both of them route through is the one fixed here.
Verification
Five wire-level tests in
test/js/node/http2/h2-conformance.test.ts(raw peer on the other side of anode:http2client or server). The first three fail on main and pass with the fix; the last two pass on both and pin the listener-calls-close()contract the helper now routes:sentTrailersis{}, the latesendTrailers()throwsERR_HTTP2_TRAILERS_ALREADY_SENT, and a PING round trip confirms nothing else reached the wire after theEND_STREAMDATAframe. On main:sentTrailersisundefined._finalpath):sendTrailers()from'prefinish'and from'finish'both throwERR_HTTP2_TRAILERS_ALREADY_SENTand the wire ends with theEND_STREAMDATAframe. On main the'prefinish'call succeeds and a trailerHEADERSframe follows theEND_STREAMframe.close()d while the chunk passed toend()was still buffered behind an in-flight write:'wantTrailers'is not emitted,sentTrailersstaysundefined, andDATA,DATA,RST_STREAM(NO_ERROR)go out with noEND_STREAMframe, as in node. On main the event fires on the closed stream. (An earlier version of this test issued the request before the session had connected; that variant never saw theRST_STREAMon the darwin-14-aarch64 lane, deterministically, while the linux lanes passed. The RST for a queued stream is sent by the pre-existingsendRstOnReadypath, which this PR does not touch; the current test uses a connected stream and does not depend on it.)close(): events are'wantTrailers','finish','close',sentTrailersisundefined, and the bodyDATAis followed byRST_STREAM(NO_ERROR)alone.Also run with the debug build: the rest of
h2-conformance.test.ts,node-http2.test.js, and all 256 upstreamtest-http2-*.jsfiles intest/js/node/test/parallel(one of them,test-http2-forget-closed-streams.js, a 10,000-request loop, timed out once on a heavily loaded box and passed on rerun; it does not involve trailers).