node:http2: make setNextStreamID ignore the ids nghttp2 rejects - #37553
node:http2: make setNextStreamID ignore the ids nghttp2 rejects#37553robobun wants to merge 4 commits into
Conversation
WalkthroughThe HTTP/2 frame parser now derives stream IDs from endpoint parity and validates IDs before updating session state. New client and server tests cover boundary, parity, ordering, range, request, and push-stream behavior. ChangesHTTP/2 stream ID handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 2:05 AM PT - Aug 12th, 2026
❌ @robobun, your commit 5032536 has some failures in 🧪 To try this PR locally: bunx bun-pr 37553That installs a local version of the PR into your bun-37553 --bun |
|
Status Reproduced on bun 1.4.0 (release) and on a debug build of main: Rebased onto main after #37542 landed (head CI on the rebased head (build 92804): all 192 jobs that ran passed, including every http2 lane. The build is marked failed only because the two |
![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 tightens setNextStreamID to match nghttp2's rejection rules.
What was reviewed:
- Traced the parity/range/monotonicity guard against nghttp2's
nghttp2_session_set_next_stream_idsemantics and RFC 9113 §5.1.1; theid - 2store round-trips correctly throughget_next_stream_idfor both client (odd) and server (even) parity, and cannot underflow since the minimum accepted id is ≥ 3. - Verified the
last_stream_id <= MAX_STREAM_IDinvariant the reverted-to-plain+ 2step relies on: wire ids are masked viaUInt31WithReserved::uint31()at both frame-header decode (h2_frame_parser.rs:5511) and PUSH_PROMISE payload read (:3962), and the JS-facing setters (get_next_stream,request,push_promise) all reject> MAX_STREAM_IDbefore storing. - Checked the removed
stream_id == 0client branch was dead — 0 is even, already handled by theis_multiple_of(2)arm. - Tests cover the full matrix (wrong parity, below/equal/above next, 31-bit boundary on both sides) with real requests and a
pushStream, error events wired to reject, cleanup infinally, and expected values taken from node v26.3.0.
Extended reasoning...
Overview
The PR changes H2FrameParser::set_next_stream_id in src/runtime/api/bun/h2_frame_parser.rs to silently ignore ids that Node's nghttp2 backend would reject (wrong parity for the session's role, > 2³¹−1, or not strictly above the current next id) instead of rounding them or moving the counter backwards. get_next_stream_id is simplified back to plain arithmetic (dropping #37542's saturating step and a dead == 0 branch) with a doc comment stating the invariant that keeps it from overflowing. Two tests are added to test/js/node/http2/node-http2.test.js: the stacked #37542 edge-case test (with one expectation updated to Node's value) and a comprehensive new test driving both a client and a server session through the full input matrix with real traffic in between.
Security risks
None. This strictly narrows the set of ids the native setter accepts; the JS wrapper's existing range/type validation is unchanged. No new untrusted-input parsing, no allocation, no unsafe blocks. The 31-bit invariant it relies on for the non-saturating + 2 was verified against every writer of last_stream_id: wire frame headers and PUSH_PROMISE promised ids are masked through UInt31WithReserved::uint31() (& 0x7fff_ffff), and the local-initiation paths (get_next_stream, push_promise, request) all check > MAX_STREAM_ID before handle_received_stream_id stores the id — so last_stream_id + 2 is at most 2³¹+1 and cannot overflow u32.
Level of scrutiny
Medium. Native HTTP/2 code, but the change is ~15 lines of straightforward guard logic with no memory management, no JS re-entrancy, and no threading. The correctness argument is arithmetic that can be checked by hand: for a client, the smallest accepted id is 3 (odd, > initial next id 1), storing 1, and get_next_stream_id() from odd 1 steps to 3; for a server, smallest accepted is 4, storing 2, stepping to 4. The <= on the monotonicity check makes the equal-to-current case a no-op, matching nghttp2 accepting it with no state change.
Other factors
The test coverage is thorough and follows harness conventions: subprocess test drains stdout/stderr/exited concurrently and asserts stderr before exit code; the in-process test uses port: 0, wires error events to reject the awaited promises, cleans up in finally, and asserts one combined object so a failure shows every diverging value. The server-side reach via Symbol.for("::bunhttp2native::") matches the symbol defined in src/js/node/http2.ts. Expected values were derived from running the same sequence on Node v26.3.0, and the PR description lists the ported Node tests that continue to pass (including test-http2-no-more-streams.js, which exercises the 2³¹−1 client boundary this change must not break). No prior reviewer comments to address; the bug-hunting pass found nothing.
|
Heads-up: #37547 adds the public |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/http2/node-http2.test.js`:
- Around line 2636-2666: Update the HTTP/2 test around the push-stream setup to
use a dedicated promise resolver for push failures, ensuring the push callback’s
serverSide.reject path remains live after serverSide.resolve(seen). Keep a
client session-error promise attached for the entire test, and race it against
the combined serverSide.promise and pushed assertions so late session errors
reject the test instead of being absorbed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0037ab5d-a1f2-4725-81b6-0867544d5eef
📒 Files selected for processing (2)
src/runtime/api/bun/h2_frame_parser.rstest/js/node/http2/node-http2.test.js
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/runtime/api/bun/h2_frame_parser.rs:8310-8318— Nit / FYI: the<= this.get_next_stream_id()floor is stricter than nghttp2's when peer-initiated streams have advancedlast_stream_id. nghttp2'snext_stream_idtracks only locally-initiated streams, whereasget_next_stream_id()derives fromlast_stream_id, which is also bumped by peer ids (client PUSH_PROMISE at ~4004; peer HEADERS viahandle_received_stream_idat ~5313) — so e.g. a client that sent stream 1 and received pushes 2,4,6,8 hassetNextStreamID(5)accepted by Node (nghttp2 next=3) but ignored here (next=9). This fails safe and is self-consistent with Bun's ownstate.nextStreamID(which already reports 9 here, before and after this PR), and the root cause — the pre-existing conflation of local/peer ids inlast_stream_id— is out of scope; just noting the gap since the new test never sets an id between the last local and last peer stream.Extended reasoning...
What the gap is
The new guard at
h2_frame_parser.rs:8312rejects any id<= this.get_next_stream_id(), mirroring nghttp2'ssession->next_stream_id > (uint32_t)next_stream_idcheck innghttp2_session_set_next_stream_id. But the two counters track different things: nghttp2'ssession->next_stream_idis advanced only when the local endpoint initiates a stream, whereas Bun'sget_next_stream_id()derives fromlast_stream_id, which is also bumped by peer-initiated ids — a client receiving PUSH_PROMISE setslast_stream_id = promisedat line ~4004-4005, and a server receiving client HEADERS sets it viahandle_received_stream_idat line ~5312-5313. So whenever peer streams have raisedlast_stream_idabove the local high-water-mark, the guard rejects ids that nghttp2 would accept.Step-by-step example
Client side, with server push enabled:
- Client sends its first request → stream 1 opens. nghttp2
next_stream_id = 3; Bunlast_stream_id = 1,get_next_stream_id() = 3. - Server sends PUSH_PROMISE for streams 2, 4, 6, 8. Bun's PUSH_PROMISE handler at ~4004 does
last_stream_id.set(8)→get_next_stream_id() = 9. nghttp2'snext_stream_idstays 3 (pushes are peer-initiated). - User calls
client.setNextStreamID(5).- Node: 5 ≥ 3 → nghttp2 accepts; next
client.request()uses stream 5. - Bun with this PR: 5 ≤ 9 → guard hits, silent no-op; next request uses stream 9.
- Node: 5 ≥ 3 → nghttp2 accepts; next
- Stream 5 is RFC-valid here — §5.1.1 requires only that a new client-initiated id exceed every id this endpoint previously opened (5 > 1); the server's even-numbered reservations don't constrain the client's odd sequence.
The server side is analogous once #37547 exposes
setNextStreamIDthere: a server that has received client streams 1..999 before pushing anything has nghttp2 next=2 vs Bun next=1000, sosetNextStreamID(4)is accepted by Node and ignored by Bun.Why the test doesn't catch it
The new test's server side runs while handling stream 1 (
last_stream_id = 1, so local-next and peer-influenced-next agree at 2), and the client side never receives a push before callingsetNextStreamID. Neither exercises the "target id between last-local and last-peer" window.Addressing the counter-argument
One reasonable objection is that this isn't really a regression: pre-PR, Bun accepted
setNextStreamID(5)here only because it blindly movedlast_stream_idbackwards for any id — the exact bug this PR fixes — and that same mechanism also accepted the RFC-violatingsetNextStreamID(5)after the client itself had sent stream 11. Without separate local-vs-peer tracking the setter can't distinguish the two, and rejecting both is the safe, self-consistent choice: Bun's ownstate.nextStreamIDalready reports 9 (not Node's 3) in this scenario before and after this PR, so the setter now agrees with the getter where before they contradicted each other.That objection is well-taken, and it's why this is a nit rather than a blocker. But it's still worth surfacing because (a) the PR's stated goal is matching what nghttp2 accepts/rejects, and this is a concrete case where nghttp2 accepts and the new guard rejects; (b) Bun already tracks
last_peer_stream_idseparately (line ~5316-5319 and ~4006-4008), so a follow-up that maintains a distinct local-id high-water-mark for both the getter and this floor has an obvious foothold; and (c) the new test's coverage doesn't include this window, so the gap would otherwise be invisible.Impact and fix
Fails safe: the higher id Bun uses (9 in the example) is still RFC-valid on the wire, so nothing hangs or errors — the user just doesn't get the id they asked for. Triggers only in a narrow scenario: calling
setNextStreamIDwith an id above the last locally-initiated stream but at or below the highest peer-initiated one, which needs server push (client) or prior client traffic before the first push (server).setNextStreamIDis a niche API to begin with.The proper fix is architectural and out of this PR's scope: track a separate
last_local_stream_id(or derive the floor fromlast_stream_idminus the peer-parity contribution) and use that for bothstate.nextStreamIDand this guard's floor. Within this PR, no change is needed; a comment on the guard noting the known divergence, or a follow-up issue, would be enough. - Client sends its first request → stream 1 opens. nghttp2
|
On the merged-counter note from the review: the server half is right and is the deliberate difference described in the PR body (received client streams advance the server's only counter, so a server id at or below them is ignored where nghttp2 would take it). The client half does not happen on the current code: a received PUSH_PROMISE goes through the engine ( |
|
@robobun rebase |
H2FrameParser::set_next_stream_id rounded an id of the peer's parity up to this side's parity and accepted ids below the current next id, moving the stream counter back over ids that were already used on the connection. It also accepted ids above 2^31 - 1, which no stream can use. nghttp2_session_set_next_stream_id, which node's setNextStreamID calls, ignores all three, so do the same: an id only takes effect when it has this side's parity, fits in 31 bits and is above the current next id. With that, last_stream_id stays within MAX_STREAM_ID on every path, so the stream id step no longer needs to saturate.
…plains Wrong-parity ids two steps past the next id (the adjacent ones read back the same whether or not parity is checked), the lowest accepted id on each side with a request landing on it, and lastProcStreamID on the equal-to-next rows, which is what distinguishes an ignored call from one that re-stores the current position.
…ough one live rejection The pushStream callback runs after the server-side results promise was already resolved, so rejecting it there was a no-op and a push failure would only show up as a timeout.
253b650 to
5032536
Compare
|
Rebased onto main now that #37542 has landed ( |
There was a problem hiding this comment.
LGTM — the two nits from the earlier pass are addressed (push/session errors now feed one live failure rejection raced against the assertions; the get_next_stream_id invariant holds on the live inbound path since read_bytes has no callers and the engine masks ids at h2/wire.rs:168).
What was reviewed:
set_next_stream_id: parity/range/monotonicity guards matchnghttp2_session_set_next_stream_id;next_stream_id - 2cannot underflow given> get_next_stream_id()and correct parity.- Reverting to plain
+inget_next_stream_id: traced every livelast_stream_id.setcaller (engineon_stream_open,get_next_stream, request paths) — all bounded byMAX_STREAM_ID, so the step stays within u32. - Updated
4294967295 → 2expectation in the overflow test matches Node (odd id ignored on a server).
Extended reasoning...
Overview
Follow-up to #37542 that rewrites H2FrameParser::set_next_stream_id in src/runtime/api/bun/h2_frame_parser.rs to mirror Node's nghttp2_session_set_next_stream_id: ids of the peer's parity, ids not strictly above the current next id, and ids beyond the 31-bit range are silently ignored instead of being rounded or moving the counter backwards. get_next_stream_id reverts from saturating_add to plain arithmetic now that every live writer of last_stream_id is bounded. One existing test expectation is updated to the Node-correct value, and a new ~140-line test drives client and server sessions through parity, monotonicity, boundary, and 31-bit-range cases with real requests and a server push, checking each result against values captured from Node v26.3.0.
Security risks
None identified. The change tightens validation (rejects more inputs than before) rather than loosening it, and prevents the stream-id counter from moving backwards below already-used ids — which was itself an RFC 9113 §5.1.1 violation on the wire. Inputs reach this setter only through the JS wrapper, which already range-checks and coerces.
Level of scrutiny
Medium. The Rust change is ~20 net lines of straightforward guard logic with a well-defined reference (nghttp2). It touches HTTP/2 stream-id bookkeeping, so I traced every last_stream_id.set call site to confirm the plain-+ invariant holds on live paths: read_bytes/lookup_inbound_stream (the one unmasked path I flagged earlier) has no callers — both read() and on_native_read route through rewrite_read() and the engine, which masks stream ids with & 0x7fff_ffff at h2/wire.rs:168. The remaining callers (on_stream_open, get_next_stream, the request/push paths at 8336/8947/9114/9184/9202) all bound the id by MAX_STREAM_ID before storing. The next_stream_id - 2 subtraction cannot underflow: with correct parity and > get_next_stream_id() (≥1 client / ≥2 server), the smallest accepted id is 3 (client) or 4 (server).
Other factors
Both 🟡 findings from my previous pass are resolved. The test now threads a single failure withResolvers through sessionError, client error, and the pushStream callback error arm, and races it against run(), so a push failure or late session error surfaces as a rejection instead of a timeout. The read_bytes dead-code explanation checks out against the source. The comment-cop bot flags were addressed (comments cut to one line each). CI passed on the rebased head per the status comment, and the ported Node tests (test-http2-client-setNextStreamID-errors, test-http2-no-more-streams) still pass. The noted overlap with #37547 is a future rebase concern, not a correctness one.
2 participants
Follow-up to #37542 (now on main), rebased on top of it.
Repro
Node ignores the even id on a client and the id below the current next one; bun rounds the first up and moves the counter back for the second, and the next request goes out on stream 5 after stream 11 was already used on the connection. A server session does the same through the native setter (
101reads back as102,50is accepted after stream 100 was pushed), and both sides accept ids above2 ** 31 - 1, after whichstate.nextStreamIDreads back2147483649or4294967295and the nextrequest()fails withERR_HTTP2_OUT_OF_STREAMS; node ignores those too.The backwards case matters on the wire: a new stream id has to be higher than every id the endpoint used before (RFC 9113 section 5.1.1). Against a node server the second request above just hangs, because nghttp2 drops the header block for a non-increasing id without answering on it, while the same call on node is a no-op. (bun's own server answers it; that leniency is a separate server-side issue.)
Cause
H2FrameParser::set_next_stream_id(src/runtime/api/bun/h2_frame_parser.rs) storedid - 1orid - 2depending on the id's parity, so an id of the peer's parity came back out ofget_next_stream_id()rounded up, any id was accepted no matter where the counter already was, and nothing bounded it by the 31-bit stream id space. Node'ssetNextStreamIDhands the id tonghttp2_session_set_next_stream_id, which rejects (and node then silently ignores) an id of the wrong parity, an id below the session's current next id and, because node converts the argument withInt32Value, anything above2 ** 31 - 1.Fix
The setter is a no-op unless the id has this side's parity, is at most
MAX_STREAM_IDand is above the currentget_next_stream_id(); it then storesid - 2, which both sides step from to exactlyid. An id equal to the current next id is a no-op as well, which is what nghttp2 accepting it amounts to; treating it as a no-op (rather than re-storingid - 2) is what keepslast_stream_id, and with itstate.lastProcStreamID, from moving. The JS wrapper already mirrors node's own checks (non-numbers,<= 0and> 2 ** 32 - 1throw), so what throws and what is ignored now matches node for every input.One deliberate difference: bun's parser keeps a single
last_stream_idhigh-water mark for both directions (received client streams bump it on a server), so "current next id" on a server session is that merged value. A server id below a client stream that was already received is therefore ignored here, where nghttp2, with its own counter per direction, would accept it. The alternative, letting the setter move the shared mark back below streams the peer already opened, would also corrupt whatlastProcStreamID, the legacy GOAWAY paths and the engine's late-RST_STREAM tolerance read from it. The only thing that differs in that case is which even ids later pushes get; splitting the counter is a larger change than this fix and nothing in the ported suites depends on it.With the setter bounded, every write to
last_stream_idstays withinMAX_STREAM_ID(wire ids are masked to 31 bits,getNextStream/requestreject above it), so the saturating step #37542 added toget_next_stream_idcan no longer trigger; it goes back to plain arithmetic with the invariant noted on the function. #37542's test still passes with one expectation changed:2 ** 32 - 1on a server now leaves the next id at2, which is node's value, instead of reading back4294967295.Verification
New test
http2 setNextStreamID ignores the ids nghttp2 rejects instead of rounding them or moving backwardsintest/js/node/http2/node-http2.test.jsdrives a client and a server session through wrong-parity ids (both the adjacent one and one two steps out, since the adjacent one reads back the same whether or not parity is checked), ids below and equal to the current next id (the equal rows also readlastProcStreamID, which is what distinguishes an ignored call from one that re-stores the position), the lowest accepted id on each side, and both ends of the 31-bit range, with real traffic in between: the ids the requests actually go out on, and apushStream()landing on stream 100. Every expected value was taken from running the same sequence on node v26.3.0 (session.setNextStreamIDon the server there). On the released bun the test fails with 14 differing values; with this branch it passes, as do the rest ofnode-http2.test.js,h2-conformance.test.ts,node-http2-streams-rehash.test.ts,h2-push-refusal-staged.test.ts, and the portedtest-http2-client-setNextStreamID-errors.js,test-http2-no-more-streams.js(sets2 ** 31 - 1on a client and must still get that stream),test-http2-client-destroy.js,test-http2-session-stream-state.jsand the server push tests.Node v26.3.0 has no further upstream test for this (
test-http2-client-setNextStreamID-errors.jsonly covers the throwing paths), hence the test here.[review] gate passed · iteration 3 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 3
evidence per changed file