Repository navigation
Conversation
…() encodes no fields
sendTrailers() with a block whose values are all empty arrays (for example
{ "x-none": [] }) encoded to zero fields and still went out as a HEADERS
frame with a zero-length block, which the peer reports as a 'trailers'
event with no headers. Node's Http2Stream::SubmitTrailers sends an empty
END_STREAM DATA frame when the encoded block has no fields, the same frame
sendTrailers({}) produces, so the peer sees no trailers at all.
The native sendTrailers() now takes the noTrailers() path when nothing was
encoded; both share one helper. sentTrailers still records the block that
was passed, as in node.
|
Updated 1:26 AM PT - Aug 13th, 2026
❌ @robobun, your commit 0fa14c2 has some failures in 🧪 To try this PR locally: bunx bun-pr 37951That installs a local version of the PR into your bun-37951 --bun |
|
Status: reproduced on the 1.4.0 release and on main with CI on the final revision (build 93984): every test lane that ran is green (177 jobs; the only annotations are retried-then-passed tests unrelated to http2). The build shows as failing only because the two darwin 26 aarch64 test jobs were canceled without ever being assigned an agent, which also happened on the previous build (93682, where the darwin 14 jobs did run and pass). Ready for a maintainer. |
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — narrow Node-compat fix with comprehensive wire-level coverage.
Checked that send_no_trailers() is a byte-identical extraction of the old no_trailers() body, that &mut stream at the new call site coerces via GuardedStream: DerefMut<Target = Stream>, and that the empty-block early return skips the same state-transition/onStreamEnd tail that send_data(.., close=true, ..) already performs on the existing noTrailers() path. Tests cover server (three empty variants + one non-empty control) and client, plus an end-to-end bun↔bun case asserting no 'trailers' event and preserved sentTrailers; all wire error handlers to reject, use port: 0, and clean up in finally.
Extended reasoning...
Overview
The PR fixes a Node.js-compat divergence in node:http2: when stream.sendTrailers() is passed a block whose values all encode to nothing (e.g. { "x-none": [] }), Bun was writing a zero-length HEADERS(END_STREAM|END_HEADERS) frame, causing the peer to emit a spurious 'trailers' event with an empty header list. Node instead sends an empty DATA(END_STREAM) frame and no 'trailers' event fires. The fix is 4 new lines in H2FrameParser::send_trailers() checking encoded_headers.is_empty() after the walk, plus a 2-line helper send_no_trailers() extracted verbatim from the existing no_trailers() host fn so both paths share one implementation. src/js/node/http2.ts only has a comment rewrite (no logic change). Two test files gain 6 tests total.
Security risks
None. This is protocol framing on the outbound path — it decides between two already-legal ways to end a stream we control. No untrusted input parsing, no size arithmetic, no new allocation, no auth/crypto surface. The empty-block case previously wrote a 0-length HEADERS frame (accepted by peers); now it writes a 0-length DATA frame instead.
Level of scrutiny
Medium — native HTTP/2 code, but the change is mechanically small and reuses an existing shipped code path. I verified: (1) the helper is a pure extraction — no_trailers() previously did stream.wait_for_trailers = false; let _ = this.send_data(stream, b"", true, JSValue::UNDEFINED, false, false), which is exactly what send_no_trailers() now does; (2) the new call site passes &mut stream where stream: GuardedStream<'_>, which auto-derefs to &mut Stream via the existing DerefMut impl; (3) the early return skips the HEADERS-frame tail (state transition + onStreamEnd dispatch), but send_data(.., true, ..) performs the equivalent END_STREAM handling — this is the same tail behavior no_trailers() has always relied on. The dispatch guard held by GuardedStream remains armed through the helper call, and send_data with JSValue::UNDEFINED callback runs no user JS.
Other factors
Test coverage is thorough for a change this size: wire-level assertions on frame type/flags/length for three empty-encoding variants plus {}, a non-empty control ({ "x-none": [], "x-sent": "1" }) proving the fix does not swallow real trailers, both server→client and client→server directions, and an end-to-end bun↔bun test asserting rstCode: 0, no 'trailers' event, and sentTrailers still set. Tests wire 'error' to reject, use port: 0, await observable conditions (frame arrival) rather than sleeps, and clean up in finally. The PR description documents that three of the five wire tests fail on the release binary and cites the Node source (Http2Stream::SubmitTrailers). No CODEOWNERS entries match these files. No prior reviewer comments to address.
|
@coderabbitai review |
|
… the block concurrently
![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 routing an empty-encoded trailer block through the existing noTrailers path.
What was reviewed:
send_no_trailershelper is a straight extraction of the two linesno_trailersalready ran; both call sites (no_trailershost fn and the newencoded_headers.is_empty()branch) now share it.- The empty check sits after the encode loop and before frame construction, so single-value bookkeeping and validation errors still fire first;
&mut stream(aGuardedStream) deref-coerces to&mut Streamfor the helper. - Test matrix covers server × {empty-array, single-value empty-array, empty-name,
{}control, mixed real+empty control} and client, asserting the exact terminating frame at wire level plussentTrailersretention; the end-to-end test wires every failure path to reject.
Extended reasoning...
Overview
Four files: a 4-line early-return in H2FrameParser::send_trailers that diverts to a new 3-line send_no_trailers helper when the HPACK-encoded trailer block is empty; the helper is the extracted body of the existing no_trailers host function (clear wait_for_trailers, send_data(b"", close=true)). A stale 6-line JS comment is deleted. Two test files gain a wire-level describe.concurrent block (raw H2 peer records frames, asserts DATA(END_STREAM, len 0) and exactly one HEADERS frame) and one bun-server↔bun-client end-to-end test (no 'trailers' event, rstCode 0, sentTrailers preserved).
Security risks
None. No user-controlled input reaches new parsing logic; the change only picks a different terminal frame type for an already-validated outbound block. No allocation, no bounds arithmetic, no new JS-visible surface.
Level of scrutiny
Moderate — HTTP/2 wire code, but the new branch is the exact path sendTrailers({}) and the no-'wantTrailers'-listener case have always taken, so its state transition, onStreamEnd dispatch, and backpressure queueing are already exercised in production. The check is on the encoder's own output buffer, so it cannot misfire on a block that produced any bytes.
Other factors
The PR description cites Node's Http2Stream::SubmitTrailers as the reference, and every asserted outcome was cross-run under Node v26.3.0. Tests follow the file's harness conventions (raw-peer helpers, Promise.withResolvers with error handlers wired to reject, test.each for the matrix, a positive control that must still send trailers). The comment-cop threads were addressed in the follow-up commits and are resolved. The noted overlap with #37953 is a few-line same-spot conflict, not a semantic disagreement.
This branch has not been deployed
1 participant
Problem
stream.sendTrailers({ "x-none": [] })(any block whose fields all encode to nothing: empty-array values, or an empty name) writes a trailerHEADERSframe withEND_STREAM | END_HEADERSand a 0-byte header block. The peer decodes an empty block and emits'trailers'with no headers (rawHeadersis[]). Same from a client stream'ssendTrailers().END_STREAMDATAframe instead, so the peer gets no'trailers'event at all. A bun or node client talking to a bun server sees one'trailers'event; talking to a node server it sees none.H2FrameParser::send_trailers()insrc/runtime/api/bun/h2_frame_parser.rswrites whatever it encoded as aHEADERSframe, even when that is nothing. The JS side (Http2Stream#sendTrailersinsrc/js/node/http2.ts) only routes tonoTrailers()when the object has no keys, so a block with keys that encode to nothing reaches the native encoder. The comment there claimed strict peers reject a zero-length block; they do not (node's client accepts it and emits'trailers'), the difference is only the spurious event.Fix
send_trailers()checks the encoded block after the walk: if it is empty, it ends the stream the wayno_trailers()does (clearwait_for_trailers, send an emptyEND_STREAMDATAframe) instead of writing aHEADERSframe. The two sites share a newsend_no_trailers()helper so they cannot drift. The stale JS comment is removed; the JS{}shortcut itself is unchanged.Http2Stream::SubmitTrailers(src/node_http2.cc) submitsDATAwithEND_STREAMwhen the header count is 0 andnghttp2_submit_trailerotherwise; the count comes frombuildNgHeaderString, which emits no field for an empty array or an empty name. The check is on the encoded output, so a block with one real field next to an empty array still goes out as trailers, and the JS{}shortcut and the native path now produce the same frame.noTrailers()path, so its stream-state transition,onStreamEnddispatch and stream release are the onessendTrailers({})has always used (the removedHEADERStail did the same things); under backpressure the frame is queued like any otherEND_STREAMframe.sentTrailersis unchanged: it is set to the block before the native call and stays set, as node keepskSentTrailerseither way. Client and server streams share the native function, so one change covers both.undefined-valued properties), opened shortly after this PR, carries an inline copy of the sameencoded_headers.is_empty()rule insend_trailers(), because skippingundefinedadds another way to reach an empty block. The rules are the same; whichever lands second drops its copy (a few-line conflict at the same spot). This PR is the standalone fix with the wire-level tests.test/js/node/http2/h2-conformance.test.ts, describesendTrailers() with a block that encodes to no fields. A raw peer records the frames. Server with{ "x-none": [] },{ "x-none": [], "content-type": [] }(a single-value field, whose bookkeeping must not count the empty array as a field),{ "": "x" }and{}, and a client with{ "x-none": [] }, must end the stream withDATA(END_STREAM, length 0)and send exactly oneHEADERSframe (the response/request block);{ "x-none": [], "x-sent": "1" }must still end with a non-empty trailerHEADERSframe. Four of the six fail on the release binary (they getHEADERS(0x5, length 0)and a secondHEADERSframe;{}already took the JS shortcut and the control already passed), all pass with the fix. Every row was also run under node v26.3.0, which produces the asserted outcome.test/js/node/http2/node-http2.test.js,sendTrailers() whose values are all empty arrays ends the stream without a 'trailers' event: bun server to bun client; the client must reach'close'withrstCode0 and no'trailers'event, and the server stream'ssentTrailersmust still be the block. Fails on the release binary (one'trailers'event with[]), passes with the fix.h2-conformance.test.ts(67 pass) andnode-http2.test.js(356 pass; the two "DATA payload survives its ArrayBuffer being detached" cases only hit the 5 s default timeout when the whole file runs on this debug build, pass on their own, and do not involve trailers),node-http2-continuation.test.ts, and the 14 upstreamtest-http2-*trailers*,*multiheaders*,*single-headers-validation*,sent-headers,misused-pseudoheaders,exceeds-server-trailer-sizeandno-wanttrailers-listenerfiles, all passing. The probes below (including the queued-frame path) were run against node v26.3.0 and the fixed build with the same output.Background
waitForTrailers,end()does not finish the stream. The stream emits'wantTrailers'and the app callssendTrailers(block), which normally goes out as a finalHEADERSframe carryingEND_STREAM. A stream can equally be finished by a zero-lengthDATAframe carryingEND_STREAM; that is whatsendTrailers({})(and a stream with no'wantTrailers'listener) sends in both node and bun.sentTrailers: the stream property holding the block passed tosendTrailers(); it also backs theERR_HTTP2_TRAILERS_ALREADY_SENTguard, which is why it stays set even when the block sends no fields.Http2Stream#sendTrailersinhttp2.tsvalidates the argument and calls eithernoTrailers(id)orsendTrailers(id, block, sensitive)on the nativeH2FrameParser; the nativesend_trailers()walks the object, HPACK-encodes each field into a buffer and writes the frame. The fix is in that native function because it is the only place that knows how many fields the walk produced.send_data()queues the frame per stream and the drain path writes it later, completing the stream's close at that point. ThenoTrailers()path has always gone through this; the trailerHEADERSframe was written directly.Probes: server sends each block, client reports what it observed (node v26.3.0 vs bun)
Server:
respond({":status": 200}, {waitForTrailers: true}),sendTrailers(block)from'wantTrailers',end("body"). Client: logs the'trailers'event (if any) andrstCodeat'close'.{ "x-none": [] }'trailers'with{}/[], rstCode 0'trailers'event, rstCode 0{ "x-none": [], "x-other": [] }'trailers'with{}/[], rstCode 0'trailers'event, rstCode 0{ "": "x" }'trailers'with{}/[], rstCode 0'trailers'event, rstCode 0{}'trailers'event'trailers'event{ "x-real": "1" }'trailers'with{ "x-real": "1" }{ "x-none": [], "x-real": "1" }'trailers'with{ "x-real": "1" }stream.sentTrailerson the server is the passed block in every row on all three. The client direction (req.sendTrailers({ "x-none": [] })against a same-runtime server) behaves the same way: the unfixed bun server logs a'trailers'event with[], node and the fixed build log none.Queued path: stream A's response exhausts the send windows while the client is not reading it, then stream B sends
{ "x-none": [] }as trailers, so B's terminating frame goes through the outbound queue. Node and the fixed build: B closes on both ends with rstCode 0 and no'trailers'event, and A still delivers all 196605 bytes once the client reads it; bun 1.4.0: the same, except one'trailers'event on B.Review notes
A self-review of the diff found no problem introduced by it. Two pre-existing things it turned up, neither changed here: streams whose final
END_STREAMframe goes out through the outbound queue are never released until the session ends (this predates the PR and applies tosendTrailers({})today; reported separately), and a clientrequest()withendStreampluswaitForTrailersmarks the stream half-closed up front (#37718 is changing that path). Review nits on test coverage and comments were applied in the follow-up commits.