Repository navigation
Conversation
…ders() runs Node's Http2Stream#sentInfoHeaders getter returns the slot as is. Only additionalHeaders() creates the array. Bun's getter fell back to an empty array, so a stream that sent no 1xx block reported [] where node reports undefined.
additionalHeaders() recorded the block before the native call that validates and sends it. A block with an invalid header name threw, sent nothing, and still showed in sentInfoHeaders. Node validates before it records the block. The test now runs one scenario in this process and under Node.js, and compares both against the same expected document.
|
Updated 10:31 PM PT - Sep 18th, 2026
❌ @robobun, your commit f853af6 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 43395That installs a local version of the PR into your bun-43395 --bun |
|
Status Reproduced on bun 1.4.3 against node v26.3.0:
The repro scripts and both outputs are in the Notes block of the PR description. Fail-before: CI: no failure comes from this diff. Two unrelated jobs are red in build 118162.
The PR is ready for a maintainer to review and merge. Found on the way, outside this PR. Separate work tracks each one:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughChangesThe HTTP/2 implementation leaves HTTP/2 informational header tracking
Suggested reviewers: Priority: ⬇️ Low 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the maxSendHeaderBlockLength path in the reordered additionalHeaders(): the native request() returns normally there (it dispatches onFrameError/onStreamError instead of throwing), so the block is still recorded in sentInfoHeaders — which matches Node, whose additionalHeaders() also records before the info() send that can fail on size.
Extended reasoning...
The diff is small: the getter drops its || [] fallback and the native validate-and-send call moves ahead of the kInfoHeaders push in ServerHttp2Stream.additionalHeaders(). I traced the one path where the native call fails without throwing (h2_frame_parser.rs request() when encoded_size > max_send_header_block_length): it closes the stream, emits frame/stream errors, and returns a stream id, so the JS side still records the block. Node's additionalHeaders() records the block before calling this[kHandle].info(headers), so the observable sentInfoHeaders is the same there; that candidate is not a divergence introduced by this PR. The remaining inline finding (a block without :status being treated as informational) is pre-existing and acknowledged as out of scope in the description.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/js/node/http2.ts— Servers that calladditionalHeaders({})or a block without:statussend a final 200 HEADERS frame instead of an informational one, then respond() sends a second final HEADERS, and the block is recorded in sentInfoHeaders as if it were a 1xx. src/js/node/http2.ts:3470 injects:status: 200before validation, and the 1xx range check at 3476 is skipped becausehasStatusis false. Fix: do not synthesize a status in additionalHeaders(); a block without:statusmust either be sent as-is like Node or be rejected, and it must never be recorded as an informational block.Extended reasoning...
Pre-existing at the base commit, and the PR names it as out of scope, but this PR is the one that defines what sentInfoHeaders means and adds the test at node-http2.test.js:1840 asserting it 'lists only the 1xx blocks that were sent'. A block with no :status is the missing row in that test. Trace: additionalHeaders() copies the object at http2.ts:3448, checks
headers[HTTP2_HEADER_STATUS] === undefinedat 3469, sets it to 200 at 3470 andhasStatus = false, so thestatusCode < 100 || statusCode >= 200check at 3476 never runs. request() at 3495 encodes and writes a HEADERS frame with:status: 200, which HTTP/2 clients treat as the final response headers. The block is then pushed into kInfoHeaders at 3496-3500. A later respond() writes another HEADERS with :status 200; a strict client (nghttp2-based, browsers) treats a second non-1xx HEADERS as a protocol error and resets the stream. Population: any server that passes hint headers to additionalHeaders() without an explicit :status, which Node accepts. Rate: every such request. Node does not add a status here (PR description confirms).…Verification: pre-existing — the injection and the wrong frame both exist byte-for-byte at the base commit and the PR description lists "Bun adds
:status: 200to a block that has no:status. Node does not." under "Not changed here"; the diff touches the same function (moves the native call above thekInfoHeadersrecord) but does not alter this path. Trigger: a server calls… | pre-existing…
|
The finding about a block with no
The request fails in both runtimes, so this PR leaves that path as it is. Separate work now tracks the status default. That work can add a row for a block with no The |
There was a problem hiding this comment.
LGTM — the follow-up commit trimmed the additionalHeaders() comment, and the two-line runtime change plus the Node-vs-Bun fixture read correctly.
What was reviewed:
sentInfoHeadersgetter returning the raw slot:kInfoHeadershas no other readers insrc/, and the only other test assertion on the old[](line 892 ofnode-http2.test.js) was updated; the vendoredtest-http2-sent-headers.jsindexes[0]only after a successfuladditionalHeaders(), so it is unaffected.- The
request()-before-record reorder: nothing between the native call and the push observeskInfoHeaders, the sameheadersobject is stored either way, and the pre-existing?.onbunHTTP2Nativeis unchanged behavior. - Fixture hermeticity: one session per request,
error/closewired to reject/resolve,client.close()andserver.close()infinally, listens on port 0 at 127.0.0.1;onServerStreamCloseis reassigned before each sequential run so it cannot cross-talk.
Extended reasoning...
Overview
The PR touches src/js/node/http2.ts (two edits: the sentInfoHeaders getter now returns this[kInfoHeaders] without the || [] fallback, and ServerHttp2Stream.additionalHeaders() now calls the native request() before pushing the block onto kInfoHeaders), adds test/js/node/http2/http2-sent-info-headers.fixture.js (a CJS module that drives /plain, /invalid, and /info-then-invalid and snapshots sentInfoHeaders at each lifecycle point on both sides), and extends test/js/node/http2/node-http2.test.js with a test that runs the fixture in-process and, when nodeExe() is available, under real Node, comparing both against one expected document. One existing assertion (req.sentInfoHeaders.length is 0) was updated to toBeUndefined().
Since the prior review on this PR, only two commits landed: an empty CI retrigger and a commit that shortens the comment above the native call at the exact line where an inline comment had been left. No runtime logic changed between the previously reviewed version and HEAD.
Security risks
None identified. The change does not alter header validation, wire encoding, or the HPACK path; it only changes when a JS-side bookkeeping array is populated and what the getter returns when nothing has been recorded. The fixture and test contact only a local loopback server on port 0.
Level of scrutiny
Low-to-moderate. The runtime diff is two lines in a builtin JS module, both matching Node's lib/internal/http2/core.js semantics (validate, record, send — Bun's native request() validates and throws before sending, so calling it first yields the same observable state as Node). I grepped src/ for kInfoHeaders/sentInfoHeaders: the getter and additionalHeaders() are the only readers/writers, so returning undefined cannot break another internal caller. I also grepped test/ for the old [] behavior: the one in-repo assertion was updated in this PR, and the vendored Node test test-http2-sent-headers.js only indexes sentInfoHeaders[0] after a successful additionalHeaders() call, which still populates the array.
Other factors
The test is capable of failing on the pre-fix build for both changes: /plain would yield [] instead of "undefined" (getter), and /invalid would yield the rejected block instead of "undefined" (reorder). The fixture is hermetic: one session per request, error and close wired to reject/resolve, client.close() and server.close() in finally, sequential runs so the shared onServerStreamClose cannot cross-talk, and the Node subprocess drains stdout/stderr/exited concurrently with await using. The .fixture.js suffix is not picked up as a test file. No CODEOWNERS entry covers the changed paths, no human reviews are outstanding, and the bug hunt ran to a dry streak with no findings.
Problem
Http2Stream#sentInfoHeadersreturns[]on a stream that sent no informational (1xx) headers. Node v26.3.0 returnsundefined. This holds for client requests and server streams.src/js/node/http2.ts:2355:return this[kInfoHeaders] || [];. The[]is left from the client-only stub in feat(http2) Client Support #6761. feat(node:http2) Implement HTTP2 server support #14286 added the real slot and kept the fallback.additionalHeaders()records the block athttp2.ts:3492, before the native call that validates and sends it. A block with an invalid header name throwsERR_INVALID_HTTP_TOKEN, sends nothing, and still shows insentInfoHeaders.Fix
lib/internal/http2/core.js:return this[kInfoHeaders];.additionalHeaders()records the block after the native call returns.respond()uses this order forsentHeaders. Node validates the header list before it records the block.test/js/node/http2/node-http2.test.js. The new test runs one fixture in the test process and under Node.js, and compares both with one expected document. Stock bun fails it and the six "settings and properties" tests.Background
:status, for example 102 or 103. A server sends it withstream.additionalHeaders()before the final response headers.sentInfoHeaderslists the 1xx blocks that the stream sent.kInfoHeadersis the symbol-keyed slot that stores the list.request()call validates each header name and value, encodes the block, and writes the frame. It throws on the first invalid header.Notes
Node source, v26.3.0:
additionalHeaders()validates the header list (L3208-L3212), then records the block (L3213-L3216), then sends it (L3218): https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js#L3208-L3218Repro (
bun file.mjsandnode file.mjs):Node v26.3.0 prints
undefinedfour times. Bun 1.4.3 prints[]four times.The block that throws, on a server stream:
Node v26.3.0 prints
ERR_INVALID_HTTP_TOKEN undefined. Bun 1.4.3 printsERR_INVALID_HTTP_TOKEN [{":status":103,"inv alid":"x"}]. With this change Bun prints the same line as node.Test:
test/js/node/http2/http2-sent-info-headers.fixture.jscollectssentInfoHeaderson both sides of three requests: no 1xx block, a block that throws, and a 102 block followed by a block that throws. It writesundefinedas the string"undefined", because JSON dropsundefined.req.sentInfoHeaders.lengthis 0) throws under node. It now expectsundefined.@types/nodedeclares the property asOutgoingHttpHeaders[] | undefined.Also ran with the debug build, all pass: the vendored node tests
test-http2-sent-headers.js,test-http2-info-headers.js,test-http2-compat-write-early-hints*.js,test-http2-compat-expect-continue*.js,test-http2-status-code*.js. The whole ofnode-http2.test.jspassed (388 pass) with the getter change. With both changes the new test and the changed assertion pass. On a heavily loaded machine, some tests in that file that spawn a debug-build subprocess hit the 5 s timeout, a different set on each run. None of them readssentInfoHeadersor callsadditionalHeaders().Self-review concerns, all addressed:
sentInfoHeaders. The second commit moves the record after the native call.undefinedwas not explicit in the compared data, and the throw cases had no rows. The fixture now has both.Not changed here:
:status: 200to a block that has no:status. Node does not.additionalHeaders()on a HEAD request throwsERR_HTTP2_PAYLOAD_FORBIDDENin Bun and sends the block in node. A separate fix is in progress.additionalHeaders()accepts connection-specific headers in Bun. node:http2: normalize outbound header objects like node #41614 covers that.sentHeaders), node:http2: make the stream lifecycle getters total, like node #41945 (endAfterHeaders,rstCode,headersSent).[human-review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file