Repository navigation
Conversation
|
Status Reproduced on main (1.4.3-canary.1+367d939d9) and on the #43440 head with a raw frame client against a With this PR the wire matches node: Eleven of the twelve tests fail on main. All twelve pass with this change, and all twelve pass on node v26.3.0 (run through a small stand-in for The branch is three commits (plus one empty CI commit) on the #43440 head (4566950). It is not rebased any more, because #43619 and #43632 are stacked on it. #43440 conflicts with main since #43649 (one hunk in This PR supersedes #36389 for this bug. That PR was closed in the stale PR sweep on 2026-09-13 and conflicts with main. CI on e7d15ae (build 119478). 179 of 181 jobs passed. Every http2 test passed on every lane. The two red jobs do not come from this diff, and the rerun (build 119697, an empty commit) shows the same two:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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 findings, I also checked the swap from the raw &mut *stream deref to enter_stream_dispatch at the three request() failure sites: reject_oversized_header_block runs onFrameError, end_stream and send_go_away synchronously with the &mut Stream live, but the GuardedStream arms the dispatch depth for the whole scope, so the pending-close drain that frees the map entry is deferred until the guard drops — no use-after-free through that path.
Extended reasoning...
Findings were reported inline (helper duplication, missing boundary tests, pushStream and client sibling sites, the name + value vs. block-size predicate), so this note only records what else was examined. The one memory-safety question a human would raise on this diff is whether holding a &mut Stream across three synchronous JS dispatches that tear the session down can free the stream; I read enter_stream_dispatch (h2_frame_parser.rs:2401) and reject_oversized_header_block (h2_frame_parser.rs:2192) and confirmed the DispatchGuard inside GuardedStream keeps the dispatch depth armed for the borrow's lifetime, which is the documented condition for deferring the only free path. The remaining findings still warrant a human look, so this is not an approval.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/runtime/api/bun/h2_frame_parser.rs— Servers calling pushStream() with a header over 64 KiB lose the entire connection: every in-flight stream dies with a session error, whereas Node only fails the pushed stream and the parent completes. The encode failure at src/runtime/api/bun/h2_frame_parser.rs:6162 still calls schedule_header_compression_session_error, the sibling server site the PR's new helper does not cover. The comment at 6159 ("Same as the request/respond encode failures") is now stale: respond no longer fails the session. Fix: route the pushStream failure through a per-stream rejection (frameError type PUSH_PROMISE on the parent, RST_STREAM INTERNAL_ERROR on the pushed stream) and update the comment.Extended reasoning...
The base commit behaves the same, so the finder filed it as pre-existing. But this PR introduces the is_server distinction and a server-only helper, and leaves the only other server-side encode-failure site with a comment that now describes behavior the PR removed. Trigger: server handler on stream 1 calls stream.pushStream({':path':'/big', 'x-big': <70000 bytes>}, cb) while streams 3 and 5 are also being served on the same connection. push_stream at 6152 fails encode_header_into_list for x-big. 6162 schedules the session error. Next tick: session error COMPRESSION_ERROR, GOAWAY 9, streams 1, 3, 5 all destroyed with ERR_HTTP2_SESSION_ERROR. Node: frameError(5, ...) on stream 1, pushed stream closed INTERNAL_ERROR, streams 1, 3, 5 complete normally. Population: any server using push with a large header, per push. Remedy: reject the pushed stream only, following the shape the PR already added for respond, and remove the stale comment at 6159.
Verification: pre-existing (acknowledged in diff: the PR description's "Not in this PR" list names pushStream() with an oversized field and correctly states the Node shape — frameError type 5 on the parent, pushed stream INTERNAL_ERROR, parent completes). Trigger: a server calls stream.pushStream() with a single header field whose name+value exceeds the default 65536 limit while other streams are live on…
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 2:56 AM PT - Sep 22nd, 2026
❌ @robobun, your commit 71d879a has 2 failures in
Add 🧪 To try this PR locally: bunx bun-pr 43474That installs a local version of the PR into your bun-43474 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
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 findings, I also checked the array-value site's return value changing from undefined to the stream id through the new helper — no caller in src/js/node/http2.ts reads request()'s return, so it is inert — and the helper's enter_stream_dispatch + reject_oversized_header_block shape, which matches the existing user-set-limit site at h2_frame_parser.rs:7108.
Extended reasoning...
This run's confirmed findings are posted inline, so approval is off the table. Since the last review the author added the 65536/65537 boundary cases and collapsed the three duplicated failure blocks into fail_unencodable_header_field, addressing two earlier threads; the client-side path and the name + value vs deflate_bound predicate remain as previously noted. Two additional candidates were investigated and ruled out: the changed return value at the array-value site (JSValue::UNDEFINED → stream id) has no consumer in http2.ts (request() results are discarded at lines 3498, 3631, 3633, 6246, 6248, 6313, 6315), and the helper's guarded-stream pattern mirrors the existing reject_oversized_header_block call site, so the switch from the raw &mut *stream deref is not a new lifetime concern.
a45fea2 to
46a31fd
Compare
|
The base of this PR (#43440) changed shape at 1d6085f, so this branch needs a rebase.
The case here (one field over 64 KiB with the option unset) can use the same check: the field sizes are known before the encode, so the block can be refused before the encoder sees any of it. Then no session error and no early teardown are needed for that case either. |
46a31fd to
a05f627
Compare
The HPACK encoder cannot emit a field whose name and value exceed 65536 bytes. With maxSendHeaderBlockLength unset, request() reported a session error and left the stream open, so the response body went out as DATA with no HEADERS frame. request() now stages the fields of every block, also when the option is unset. With the option unset it refuses a block that holds such a field through the same path as a block over a user-set limit: frameError, then RST_STREAM FRAME_SIZE_ERROR on a server or a local REFUSED_STREAM on a client. nghttp2's default limit of 65536 refuses the same blocks. The refused block never reaches the encoder, so the other streams of the session keep a valid HPACK state.
a05f627 to
8268eab
Compare
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 findings, I also checked the always-staged refactor for lost behavior: the stream lookup and set_context at h2_frame_parser.rs:6897-6903 still run before both the new over_send_limit check and the encode loop, so the remaining schedule_header_compression_session_error fallback keeps the context the deleted inline blocks used to set. The assertion change in node-http2.test.js:584-587 (session error to NGHTTP2_REFUSED_STREAM) matches the client branch of the shared refusal path, which now covers a >64 KiB field with the option unset.
Extended reasoning...
Findings were reported inline, so this body only records what else was examined on the new push. The refactor removes three inline encoder-failure blocks and encode_or_stage_header, staging every field in a HeaderList unconditionally; I confirmed the stream context is set once at 6897-6903 before the send-limit check and the encode loop, so the retained encoder-failure fallback at 7141 is not missing the set_context the deleted blocks performed. The max_send_header_block_length == 0 arm uses has_field_over(65536) while the set arm keeps the deflate_bound() + priority > limit check, and both feed the pre-existing server/client refusal branch, which is consistent with the updated expectation in node-http2.test.js. The staged copy adds one extra copy of every header on the default path, a perf cost but not a correctness issue.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/api/bun/h2_frame_parser.rs— pre-existing: a server that calls respond() with a 1xx :status and a header field over 64 KiB still writes DATA on a stream that never got HEADERS, the wire violation this PR sets out to fix. h2_frame_parser.rs:7118 skips end_stream for every 1xx block, but respond() (http2.ts:3646) then sets headersSent and the user's end("body") passes can_send_data. Node resets the stream with FRAME_SIZE_ERROR for every refused HEADERS block, 1xx included. Fix: reset the stream on every refused block, or key the exemption on the additionalHeaders() entry point instead of the :status value, so no refused block can be followed by DATA.Extended reasoning...
respond() in http2.ts:3500-3656 accepts any status 100-599 except 101 (:3594-3600), so stream.respond({":status": 103, "x-big": big}) reaches native request() with a 1xx block. Only the compat layer blocks 1xx (http2.ts:863); the core API does not.
In request(), has_field_over(NGHTTP2_MAX_HEADERSLEN) is true at :7102, the server branch dispatches onFrameError at :7111, then :7118 sees is_informational() (HeaderList::is_informational at :2015 matches any :status whose value starts with '1') and skips end_stream. The stream stays OPEN and no HEADERS was encoded or written.
Back in JS, respond() sets this.headersSent = true at :3646 and returns. The handler's stream.end("body") goes through _write (:2876) to native write_stream (:5932); can_send_data at :1861 returns true for OPEN, so send_data writes a DATA frame with END_STREAM on a stream the peer has never seen HEADERS for. nghttp2 clients treat DATA on a stream in opening state as a connection PROTOCOL_ERROR.
On the base the same input produced a session error plus the same DATA-without-HEADERS frame, so this is the one path…Verification: pre-existing — the base already writes DATA with no HEADERS for this input by the encoder-failure route, and this PR fixes that wire violation for final responses but does not reach the 1xx-via-respond() case. Triggering condition: a server calls
stream.respond({":status": 1xx, "x-big": <field whose name+value > 65536>})withmaxSendHeaderBlockLengthunset, then writes/ends the stream.…
…EADERS A refused 1xx block keeps the stream open for the final response. Bun's respond() accepted a 1xx status, so respond() with status 103 and a field over 64 KiB still left DATA on the wire with no HEADERS. Node's respond() throws ERR_HTTP2_STATUS_INVALID for a status below 200. Bun now does the same, so only additionalHeaders() can send a 1xx block. additionalHeaders() without ":status" sends a block that is not 1xx. A refusal resets that stream, and a respond() in the same tick wrote HEADERS after the RST_STREAM. The native request() now returns for a closed stream, like no_trailers() and send_trailers(). The comment on NGHTTP2_MAX_HEADERSLEN now cites nghttp2 and names the encoder constant it equals.
|
The Node never sends a 1xx block from The same commit adds a closed-stream check to the native Two new wire tests cover both cases. All eleven tests in the block pass. Ten fail on main, and nine fail with a debug build of the #43440 head. |
There was a problem hiding this comment.
I reviewed the two newest commits and didn't find any bugs; the earlier nits (boundary tests, asserted stream errors, the three duplicated failure blocks, the NGHTTP2_MAX_HEADERSLEN citation) are all addressed in the code now. Because this changes two user-visible contracts (a client request with an oversized field now fails with a stream error instead of a session error, and respond() now throws for 1xx), a human look would still be worthwhile.
What was reviewed:
- Staging every header in HeaderList: push() copies name and value bytes, so slices of the reused stack name_buffer and per-iteration JS string slices are not held past the loop.
- Unset-limit predicate: has_field_over(65536) uses the same
>as the encoder's LSHPACK_MAX_HEADER_SIZE check in src/jsc/bindings/c-bindings.cpp:471, so with the option unset the staged encode loop cannot fail and the remaining session-error path is only reachable with a user-set limit above 65536. - The new StreamState::CLOSED early return in request(): base would have encoded and written HEADERS on a reset stream; the new return touches neither the encoder nor the wire.
- respond() rejecting 1xx: compat writeContinue/writeEarlyHints already go through additionalHeaders(), and no existing test under test/js/node/http2 calls respond() with a 1xx status.
Extended reasoning...
Overview
The diff (4 files) reworks how the native request() in src/runtime/api/bun/h2_frame_parser.rs handles an outgoing header block with a field the HPACK encoder cannot emit. All three header-input shapes (single value, array value, raw list) now stage into a HeaderList unconditionally, the encode_or_stage_header helper and its three copied failure blocks are deleted, and a single over_send_limit match decides refusal: has_field_over(NGHTTP2_MAX_HEADERSLEN) when maxSendHeaderBlockLength is unset, the existing deflate_bound() + priority > limit when set. A new early return after enter_stream_dispatch makes request() a no-op on a stream already in StreamState::CLOSED. src/js/node/http2.ts tightens respond() to reject status codes below 200. Tests add a describe block in h2-conformance.test.ts with a raw-frame client and change one expectation in node-http2.test.js (which runs against both node and bun).
Security risks
No auth, crypto, or path handling is involved. The change reduces attack surface slightly: an oversized field no longer poisons the shared HPACK encoder state, and it no longer converts one bad response into a session-wide COMPRESSION_ERROR that kills every other stream on the connection. The staging copy is bounded by the same header-size limits that already applied. Allocation failures in HeaderList::push are routed to a thrown JS error rather than a panic.
Level of scrutiny
Moderate. The native code is a protocol-state change in a hot path, but the diff is mostly a simplification (net -111 lines in the Rust file) and the refusal path it routes to already existed from the stacked PR. I checked the points a refactor like this usually breaks: HeaderList::push copies bytes so the reused name_buffer slice is safe to stage; the unset-limit predicate uses the same strict > as the encoder limit in c-bindings.cpp:471, so the staged encode loop can only fail when a user-set limit exceeds 65536 (the PR explicitly keeps the session error there); the CLOSED early return skips set_context only on a stream that is already reset, where base would have written a HEADERS frame on a closed stream. The two user-visible contract changes (client oversized request now ERR_HTTP2_STREAM_ERROR/NGHTTP2_REFUSED_STREAM; respond() throws ERR_HTTP2_STATUS_INVALID for 1xx) are both aligned with node's observable behavior and the node-run test file pins the first, but they are deliberate behavior changes a maintainer should consciously accept, which is why I defer rather than approve.
Other factors
The bug hunt exited on a dry streak with no findings, and I did not run the debug build here (no existing build artifact; the PR states the http2 suite and node's test-http2-* files pass). All prior review threads were resolved by the author, but the current code shows each was actually addressed: boundary tests at 65536/65537 exist and the 65536 case names the divergence from node in its title, streamErrors and sessionErrors are asserted in the shared refused expectation, the three duplicated failure blocks are gone, and the new constant cites nghttp2_frame.h and names LSHPACK_MAX_HEADER_SIZE. One pre-existing detail outside this diff: deflate_bound() uses 12 bytes per field where nghttp2 uses 6; this only makes the user-set-limit check slightly more conservative and is unchanged by this PR.
|
Thanks. For the maintainer who looks at the two contract changes, the evidence for each is in the Notes of the PR description:
One correction to the note on |
|
@robobun Two new tests pin behaviour Node v26.3.0 does not have. A response field of exactly 65536 bytes is refused by nghttp2's deflate bound (deps/nghttp2/lib/nghttp2_session.c:2095) where Bun sends it, and after a refused |
…ter a refused additionalHeaders() block With maxSendHeaderBlockLength unset, request() refused only a block that held a field over 65536 bytes. nghttp2 uses 65536 as the limit in that case and compares it with its bound for the whole block. over_send_limit() now does the same. A block that node refuses is refused, and a block that node sends is sent: the flip point for respond() is a 65435-byte value on both. A refused additionalHeaders() block is not the response. Node drops it and still sends respond() and end() from the same tick. The native request() took that from the :status value, and bun adds :status 200 to a block that has none, so that block reset the stream. additionalHeaders() now tells request() that its block is informational. The closed-stream check in request() is removed. No path reaches it now. The tests in the describe block hold node v26.3.0's expectations only.
|
@cirospaciari Both now match Node in e7d15ae. Neither was for safety: the first was me keeping header blocks over 64 KiB that bun sends today, and the second came from a Default limit. With
Your repro ( Refused Tests. The describe block holds Node's expectations only. The 65536 test is now a pair at nghttp2's bound (exactly 65536 is sent, 65537 is refused), there is a new test for many small fields, and the Also run on the debug build: |
robobun
changed the title
|
@cirospaciari CI on e7d15ae finished: 179 of 181 jobs passed, and the http2 tests passed on every lane. The two red jobs are not from this diff (details in the status comment above):
I do not rebase in either case, because #43619 and #43632 are stacked on this branch. |
There was a problem hiding this comment.
I reviewed the latest push (e7d15ae, the switch to nghttp2's default send limit) and found no bugs; since it turns a single-field guard into a default behavior change for every node:http2 response and client request over 64 KiB, a human look is still worthwhile.
What was reviewed:
- The removed inline encoder-failure blocks in the three header loops:
set_contextnow runs unconditionally at h2_frame_parser.rs:6900 before the deferred encode loop, and that loop still callsschedule_header_compression_session_error, so the old path's side effects survive for a user-set limit above 65536. over_send_limit()against nghttp2:12 + 12*n + name/value bytesmatchesnghttp2_hd_deflate_bound, and the 5 priority bytes are always added as insession_prep_frame; the 65536/65537 boundary test's56overhead is right for:status/200+x-bigwithsendDate: false, whichrespond()honors.- The
informationalflag: onlyadditionalHeaders()passes it,respond()now rejects 1xx like node'svalidatePreparedResponseHeaders, and the compatwriteHeadalready throwsERR_HTTP2_INFO_STATUS_NOT_ALLOWEDfor 1xx. pushStream()/sendTrailers()still encode inline and can hit the session-level COMPRESSION_ERROR for a >64 KiB field; the PR names this as excluded (overlap with #41520).
Extended reasoning...
The change touches src/runtime/api/bun/h2_frame_parser.rs (request() now stages every header field, checks the block against nghttp2's 65536 default via over_send_limit(), and drops the HeaderList::is_informational() heuristic and encode_or_stage_header() for a sixth boolean argument), src/js/node/http2.ts (additionalHeaders() passes that flag; respond() rejects status < 200), and two test files adding about 230 lines of raw-frame conformance tests plus one updated client-side expectation. It touches no auth, crypto, or injection surface; the sensitive part is HPACK encoder state on a shared connection, which the refusal-before-encode ordering protects. Deferring rather than approving because the latest commit changes default user-visible behavior (any response or request block over 64 KiB is now refused with RST_STREAM/REFUSED_STREAM instead of sent or session-failed), a maintainer (cirospaciari) commented on 2026-09-21 and the follow-up commit's text is not visible to me, and I could not run the debug build in this session to execute the new tests.
This branch has not been deployed
3 participants
Problem
node:http2server responds with one header field over 64 KiB. The wire showsDATAwithEND_STREAMand noHEADERS, thenGOAWAYcode 9. Node sendsRST_STREAMFRAME_SIZE_ERRORandGOAWAYNO_ERROR.maxSendHeaderBlockLengthunset, the nativerequest()(src/runtime/api/bun/h2_frame_parser.rs) callsschedule_header_compression_session_error(). The stream stays open, sostream.end()writes DATA.maxSendHeaderBlockLength(default 65536) before it encodes it. Bun has no default.Fix
request()now collects every block inHeaderList(from node:http2: reset the stream when respond() exceeds maxSendHeaderBlockLength #43440) before it encodes it. An unset option means 65536. A block over the limit takes node:http2: reset the stream when respond() exceeds maxSendHeaderBlockLength #43440's refusal path:frameError, thenRST_STREAMFRAME_SIZE_ERRORon a server or a localREFUSED_STREAMon a client.maxSendHeaderBlockLengthraises the limit.respond()now throws for a 1xx status, and a refusedadditionalHeaders()block leaves the stream open forrespond().test/js/node/http2/h2-conformance.test.ts(twelve new tests: eleven fail on main, all twelve pass on node v26.3.0),test/js/node/http2/, node'stest-http2-*.js. Self-reviewed: 3 concerns raised, 2 addressed.Background
frameError.request()servesrespond(),additionalHeaders()and the clientrequest().Notes
Measured with a raw frame client, node v26.3.0 as the reference. The big field is 70000 or 200000 bytes, the option is unset.
respond()+end("body")with the fieldRST_STREAM 6,GOAWAY 0,frameError 1,6, stream errorNGHTTP2_FRAME_SIZE_ERRORDATA "body" END_STREAMwith no HEADERS,GOAWAY 9,ERR_HTTP2_SESSION_ERRORres.setHeader()+res.end()additionalHeaders()with the field (103, or no:status), thenrespond(200)+end("body")frameError, then HEADERS 200 andDATA,GOAWAY 0,rstCode 0DATA,GOAWAY 9GOAWAY 0GOAWAY 0request()with the fieldframeError 1,6,ERR_HTTP2_STREAM_ERRORNGHTTP2_REFUSED_STREAM, then node closes the sessionERR_HTTP2_SESSION_ERRORcode 9. The peer also sees an emptyDATAframe on a stream with no HEADERSThe default limit. The first two versions of this PR refused only a block that holds a single field over 65536 bytes, to keep every block that bun sends today. A review asked for node's behavior.
over_send_limit()now uses nghttp2's default (nghttp2_frame.h#L58) with the bound from #43440:nghttp2_hd_deflate_bound()(12, plus 12 per field, plus the name and value bytes) plus the 5 priority bytes of HEADERS. Flip points, found by bisection, are the same on node v26.3.0 and on this branch:respond({ ":status": 200, "x-big": v })(with the defaultdatefield)client.request({ ":path": "/", "x-big": v })(5-digit port in:authority)respond()withmaxSendHeaderBlockLength: 400With the option unset the encoder can no longer fail on size, because a block under the limit holds no field over 65536 bytes.
The tests hold node's expectations only. I ran the describe block on node v26.3.0: the test file transpiled with
bun build --no-bundle, andbun:test/harnessreplaced by a 50-line stand-in (describe,test,test.each,expect().toEqual()onassert.deepStrictEqual). 12 pass. The version before this change failed two tests there: the 65536-byte field (bun sent it, node refuses it) and the refusedadditionalHeaders()block without:status(bun reset the stream, node sends the response).respond()with a 1xx status, andadditionalHeaders(). A refusedadditionalHeaders()block is not the response, so the stream stays open andrespond()+end()from the same tick still go out. If nothing responds in that tick, theframeErrorhandler from #43440 resets the stream onsetImmediate, as node'sonFrameErrordoes. #43440 told a 1xx block by its:statusvalue. That was wrong in two cases. Bun'srespond()accepted 100 to 199, sorespond({ ":status": 103, <the field> })kept the stream open andend("body")wrote DATA with no HEADERS. Node'svalidatePreparedResponseHeaders()throwsERR_HTTP2_STATUS_INVALIDfor a status below 200 (core.js#L2670-L2678), andrespond()now does the same. And bun'sadditionalHeaders()adds:status: 200to a block without one, so that block reset the stream.additionalHeaders()now passes a flag to the nativerequest(), andHeaderList::is_informational()is gone. An earlier commit of this PR added a closed-stream check torequest()for the second case. It is removed, because no path reaches it now.HPACK state after a refusal. The test
a stream in flight completes and its headers still decodesends the same small field in the refused block and in the response of the second stream. A real client decodes the second response. If the refused block had reached the encoder, the second block would carry an index that the client never received. I also checked the client direction against a node server (strict decoder): the request after a refused request decodes there.A side effect of the staging. The encoder now runs after the walk, after the options are parsed and after the session memory check. A call that throws during the walk (an invalid header value after a valid field), or returns early after it (an invalid
weight, an abortedsignal,ENHANCE_YOUR_CALM), no longer leaves a block in the encoder that the peer never receives. Measured for the throw, with a node v26.3.0 server as the peer:client.request({ ":path": "/one", "x-a": "v", "x-bad": "line\nbreak" })throwsERR_HTTP2_INVALID_HEADER_VALUEon both builds. On main the next request that carriesx-amakes the node peer reportProtocol error, and the session ends with code 9. With this PR the next two requests get 200 (3 of 3 runs).Overlap with #41520. That PR fixes the throw case for
request(),pushStream()andsendTrailers()with its ownHeaderList, and it also adds the pre-compression check for a user-set limit. With the option unset, an encoder failure there still reports the session error and leaves the stream open (read from its diff at 90f334a, not run). #41520 and the #43440 stack both rewrite the walk in the nativerequest(), so they conflict in source. The one that lands second needs a rebase.Where the session error stays. Server or client with
maxSendHeaderBlockLength: 100000and a 90000-byte field. Node sends that block (HEADERS plus four CONTINUATION frames). Bun cannot, because the encoder limit is per field. The block is under the user's limit, so the staged encode loop fails and the session error from #34432 stays.fails the whole session when an outbound header block cannot be encoded,delivers a session error from the event loop, not inside the call that detected itanddelivers the reserved push stream and fails the session when its headers cannot be encodedstill pass without a change.One existing test changes.
headers cannot be bigger than 65536 bytesinnode-http2.test.jspinnedERR_HTTP2_SESSION_ERRORcode 9 for a client with the default limit. Node v26.3.0 givesERR_HTTP2_STREAM_ERRORNGHTTP2_REFUSED_STREAMfor that request (measured with and without TLS). The test now expects that.Self-review. Three concerns. Two are addressed:
respond()with a 1xx status, and theadditionalHeaders()block without:status. The third is the raised limit case above. It is not addressed: three tests pin that session error, and the connection ends in both outcomes.Not in this PR
pushStream()andsendTrailers()with a block over the limit. node:http2: fail only the push when a pushStream() header block is over the send limit #43619 and node:http2: refuse a trailer block over the send limit #43632 are stacked on this PR for them.maxSendHeaderBlockLength: 0. Node passes 0 to nghttp2, which then refuses every block (measured). Bun reads 0 as unset.frameErrorargument (the stream id) that node passes.History. The first version called
reject_oversized_header_block()from an older #43440 head. #43440 then moved toHeaderListstaging and a graceful close, and removed that helper, so the second version staged every block and refused a single field over 65536 bytes. The third version (e7d15ae) uses nghttp2's default for the whole block, after review. The branch is not rebased any more, because #43619 and #43632 sit on it.Suites run with the debug (ASan) build at e7d15ae:
test/js/node/http2/(600 pass, 6 skip, 0 fail), 261test/js/node/test/{parallel,sequential}/test-http2-*.jsfiles (all exit 0),test/js/bun/http/serve-http2.test.ts(93 pass),test/js/web/fetch/fetch-http2-client.test.ts(75 pass),test/regression/issue/{25589,24924,26915,29073}.test.ts(27 pass),test/js/third_party/grpc-js/(318 pass, 10 fail: five DNS tests and the tonic test fail with the release build in this container too, and the fourtest-outlier-detectioncases are 5 s timeouts under load that also occur with a debug build of the #43440 head).[human-review] gate passed · iteration 6 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 6
evidence per changed file