node:http2: keep the next local stream id independent of the peer's streams

[robobun]

Problem

• A node:http2 server's state.nextStreamID, and the ids pushStream() hands out, differ from node for the same program: after four client requests node pushes on stream 2, bun 1.4.0 and main push on stream 8.
• The ids bun sends are still legal (RFC 9113 only requires each side's own ids to increase), so this is a node compat divergence, not a protocol bug.
• Cause: the parser had one counter that every stream, peer or local, advanced, and derived the next local id from it. Node keeps the next local id as a separate counter that only local streams and setNextStreamID() move.
• Because the counter was shared, setNextStreamID() on a server also moved state.lastProcStreamID and the high-water mark used to tolerate a late RST_STREAM on an evicted stream. Node's setter moves neither.

Fix

• Adds a dedicated next-local-id counter: 2 on a server, 1 on a client, advanced only when a stream of this side's parity is registered. Because it advances at registration, any local id that enters the stream map pushes the counter past itself, so an id in use is never handed out again.
• setNextStreamID() now writes to this counter. The old shared counter is still raised by every stream, so lastProcStreamID, the late-RST mark and the GOAWAY paths that read it behave as before.
• One input changes: a setter argument that truncates to 0 (a fractional id below 1) is now ignored, as nghttp2 does, instead of rewinding a used session onto ids it already handed out.
• Verification: two new tests whose expected values come from running the same sequences on node v26.3.0. Both fail on main (7 differing values in the first, stream 1 reused in the second) and pass with this change; the existing http2 suites still pass on a debug build.

Background

• HTTP/2 stream ids: a client opens odd-numbered streams, a server even-numbered ones. Each side's ids must increase, but nothing ties one side's sequence to the other's.
• Server push: pushStream() makes the server open a stream itself, announced with a PUSH_PROMISE frame, so a server consumes ids from its own even sequence.
• H2FrameParser (h2_frame_parser.rs) is the native engine behind node:http2. session.state and setNextStreamID() read and write its counters directly.
• nghttp2 is the C library node's http2 is built on. Node's state.nextStreamID reads nghttp2's next_stream_id field, so matching that field's contract is what node compat means here.
• lastProcStreamID is node's name for the last peer-initiated stream the session processed. The parser also keeps a highest-id-ever mark, which is the cell the setter used to write into.

---

[review] gate passed · iteration 1 · 2 files touched

fails on main (without fix)

ASAN without fix: 2 failed, 6 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/node/http2/node-http2.test.js"
bun test v1.4.0 (c2e587460)

test/js/node/http2/node-http2.test.js:
(pass) node none > Client Basics > should be able to send a GET request [1068.49ms]
(pass) node none > Client Basics > should be able to send a POST request [787.51ms]
(pass) node none > Client Basics > constants [25.19ms]
(pass) node none > Client Basics > getDefaultSettings [9.09ms]
(pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [26.06ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [6.23ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [3.36ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [5.21ms]
(pass) node none > Client Basics > should be able to send data using end [837.18ms]
(pass) node none > Client Basics > should be able to mutiplex GET requests [828.11ms]
(pass) node none > Client Basics > http2 should receive remoteSettings when receiving
... (truncated)

release without fix: 12 failed, 6 skipped
bun test v1.4.0-canary.1 (da3851e57)

test/js/node/http2/node-http2.test.js:
(pass) node none > Client Basics > constants [0.82ms]
(pass) node none > Client Basics > getDefaultSettings [0.17ms]
(pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [0.37ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [0.10ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [0.04ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [0.06ms]
(pass) node none > Client Basics > is possible to abort request [1.69ms]
(pass) node none > Client Basics > aborted event should work with abortController [0.74ms]
(pass) node none > Client Basics > aborted event should work with aborted signal [0.65ms]
(pass) node none > Client Basics > signal validation matches node: non-signal objects throw, duck-typed { aborted } is accepted [1.11ms]
(pass) node none > Client Basics > should fail to connect over HTTP/1.1 [32.69ms]
(skip) node none > Client Basics > should not leak memory
(pass) node none > Client Basics > headers cannot be bigge
... (truncated)

passes on PR (with fix)

ASAN with fix: 6 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/node/http2/node-http2.test.js"
bun test v1.4.0 (c2e587460)

test/js/node/http2/node-http2.test.js:
(pass) node none > Client Basics > should be able to send a GET request [816.38ms]
(pass) node none > Client Basics > should be able to send a POST request [539.51ms]
(pass) node none > Client Basics > constants [17.80ms]
(pass) node none > Client Basics > getDefaultSettings [6.68ms]
(pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [22.02ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [5.01ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [3.12ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [5.15ms]
(pass) node none > Client Basics > should be able to send data using end [568.21ms]
(pass) node none > Client Basics > should be able to mutiplex GET requests [557.95ms]
(pass) node none > Client Basics > http2 should receive remoteSettings when receiving 
... (truncated)

release with fix: 6 skipped
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped)
  target       linux-x64-gnu
  build type   Release
  build dir    ./build/release
  revision     c2e5874605
  features     baseline

22 deps, 107 codegen, 1176 objects in 1090ms

ninja: Entering directory `/workspace/bun/build/release'
[1/1238] install /workspace/bun
bun install v1.4.0-canary.1 (da3851e57)

Checked 107 installs across 153 packages (no changes) [12.00ms]
[2/1238] install /workspace/bun/packages/bun-error
bun install v1.4.0-canary.1 (da3851e57)

Checked 1 install across 2 packages (no changes) [1.00ms]
[3/1238] install /workspace/bun/src/node-fallbacks
bun install v1.4.0-canary.1 (da3851e57)

Checked 129 installs across 147 packages (no changes) [6.00ms]
[4/1238] gen ErrorCode+*.h
[5/1238] gen bindgenv2
[6/1238] gen JSEvent.lut.h
Generating /workspace/bun/build/release/codegen/JSEvent.lut.h from /workspace/bun/src/jsc/bindings/webcore/JSEvent.cpp
[7/1238] fetch libjpeg-turbo
[libjpeg-turbo] up to date
[8/1238] gen ProcessBindingHTTPParser.lut.h
Generating /workspace/bun/build/release/codegen/ProcessBindingHTTPParser.lut.h from /workspace/bun/src/jsc/bindings/ProcessB
... (truncated)

diff hotspot

src/runtime/api/bun/h2_frame_parser.rs |  66 +++++++++--------
 test/js/node/http2/node-http2.test.js  | 132 +++++++++++++++++++++++++++++++++
 2 files changed, 166 insertions(+), 32 deletions(-)

gate history · 2 passed · 0 rejected · iteration 1

evidence per changed file

file                                    reads  edits  tests
src/runtime/api/bun/h2_frame_parser.rs     20     16      0
test/js/node/http2/node-http2.test.js       9      3      0

Original description

Repro

const http2 = require("node:http2");
const server = http2.createServer();
server.on("stream", (stream, headers) => {
  console.log(`client stream ${stream.id} arrived; server nextStreamID = ${stream.session.state.nextStreamID}`);
  if (headers[":path"] !== "/push") return stream.respond(), stream.end();
  stream.pushStream({ ":path": "/pushed" }, (err, pushed) => {
    console.log("pushed on stream", pushed.id);
    pushed.respond(); pushed.end(); stream.respond(); stream.end();
  });
});
server.listen(0, "127.0.0.1", () => {
  const client = http2.connect(`http://127.0.0.1:${server.address().port}`);
  client.on("stream", pushed => pushed.resume());
  const paths = ["/a", "/b", "/c", "/push"];
  (function next() {
    const path = paths.shift();
    if (!path) return client.close(), server.close();
    client.request({ ":path": path }).on("close", next).resume();
  })();
});

node v26.3.0                                   bun 1.4.0 (and main)
client stream 1 arrived; nextStreamID = 2       client stream 1 arrived; nextStreamID = 2
client stream 3 arrived; nextStreamID = 2       client stream 3 arrived; nextStreamID = 4
client stream 5 arrived; nextStreamID = 2       client stream 5 arrived; nextStreamID = 6
client stream 7 arrived; nextStreamID = 2       client stream 7 arrived; nextStreamID = 8
pushed on stream 2                              pushed on stream 8

The promised ids bun puts on the wire are still legal (RFC 9113 section 5.1.1 only requires each side's ids to increase), so this is a node compat divergence: state.nextStreamID and the ids pushStream() hands out differ from node for the same program, and setNextStreamID() on a server session (the native setter today, the public method once #37547 lands) had two side effects node's does not have: state.lastProcStreamID changed with it (node: the last peer stream processed, unaffected by the setter), and so did the stream id high-water mark the engine consults to tolerate a late RST_STREAM on an evicted stream.

Cause

H2FrameParser had one counter, last_stream_id, advanced by handle_received_stream_id for every stream registered on the session, peer-initiated or local, and get_next_stream_id() derived the next local id from it (set_next_stream_id wrote into it too). That counter is legitimately shared for its other job (the highest id that has ever existed on the session, highest_started_stream_id), but the next local id is a different quantity. nghttp2 keeps next_stream_id as an independent counter that only local submissions and nghttp2_session_set_next_stream_id move, and node's state.nextStreamID reads it directly, which is why a node server keeps pushing on 2, 4, ... regardless of how many client streams have arrived.

Fix

src/runtime/api/bun/h2_frame_parser.rs gets a dedicated next_stream_id, modeled on nghttp2's field:

• initialized to 2 on a server and 1 on a client (first_local_stream_id(), set where the constructor settles is_server);
• handle_received_stream_id advances it to id + 2 when a stream of this side's parity is registered, next to the existing last_stream_id / last_peer_stream_id bookkeeping. Peer streams leave it alone. Keeping the advance at the registration point (rather than in getNextStream()) means any local-parity id that enters the stream map, through getNextStream() or request(), pushes the counter past itself, so an id in use can never be handed out again;
• getCurrentState(), getNextStream() and the no-id path of request() read the field; get_next_stream_id() is gone;
• setNextStreamID() stores into it. Its observable behavior is unchanged for every id it handled before (an id of the peer's parity still rounds up to this side's next one; moving backwards is still allowed, that part is node:http2: make setNextStreamID ignore the ids nghttp2 rejects #37553's subject), and it no longer touches last_stream_id, so lastProcStreamID and the high-water mark stay put. The one input whose outcome changes is 0 (what a fractional id below 1 truncates to after passing the JS range check): node:http2: fix stream id overflow in setNextStreamID and nextStreamID #37542, now on main, saturates it back to the initial state, which on a session that has already used ids rewinds the counter onto them; this branch ignores it, which is what nghttp2 does, so node and this branch both leave the counter where it was.

last_stream_id is otherwise untouched: it is still raised by every registration, so highest_started_stream_id() and the GOAWAY paths that read it behave as before (h2-late-rst-staged.test.ts and the late-RST test in node-http2.test.js cover that mark and still pass). That the setter no longer lowers the mark is pinned through the lastProcStreamID assertion in the first test, which reads the same cell today; I did not add a setter variant of the late-RST wire test, since it would only stay distinguishable until #37553 turns backwards ids into no-ops, and that scenario is the one with a known platform flake.

Why this is the right shape rather than, say, subtracting peer ids back out: the two quantities have different definitions in both the RFC and nghttp2, and only a separate counter gives node's values in every case, including the edges. After a client uses 2 ** 31 - 1, both node and this branch report nextStreamID 2147483649 and fail the next request with ERR_HTTP2_OUT_OF_STREAMS (test-http2-no-more-streams.js). Storing the next id directly also supersedes the saturating predecessor arithmetic #37542 added (merged while this was open; this branch is rebased over it and replaces both of the functions it touched), since there is no predecessor arithmetic left. #37542's test stays and passes unchanged: on a fresh session a server setter argument of 0 or 2 ** 32 - 1 still reads back as 2 and 4294967295. The difference is only visible on a used session, where 0 is ignored instead of rewinding the counter, which is what the second test below pins.

Relationship to the other open setNextStreamID work: #37553 rewrites the setter body this PR replaces; on top of this branch it reduces to its nghttp2 validity checks followed by next_stream_id.set(id) (its "not above the current next id" check also covers the requested == 0 arm here, which it can then drop). #37588 (GOAWAY / lastProcStreamID from the peer mark) touches adjacent lines in handle_received_stream_id but a different field; the lastProcStreamID value asserted below is the same under both definitions. The client side was compared against node as well: received PUSH_PROMISEs do not register in the embedder's map, so a client's next request id already matched node (3 after pushes 2, 4, 6, 8 arrive); the refactor keeps it that way and the new test pins the client's nextStreamID too.

Verification

New test http2 server push stream ids are independent of the client's stream ids in test/js/node/http2/node-http2.test.js: three client requests, then a request whose handler pushes, then one whose handler sets the next id to 6 and pushes. It asserts the server's nextStreamID as each client stream arrives, the id before/after each push, nextStreamID/lastProcStreamID after the setter, the promised ids the client actually receives on the wire, and the client's own request ids and nextStreamID. Every expected value was produced by running the same sequence on node v26.3.0 (session.setNextStreamID there; here the native setter, since ServerHttp2Session does not expose the method yet). On USE_SYSTEM_BUN=1 it fails with 7 differing values (nextStreamID 4/6 on arrival, push on 8 instead of 2, lastProcStreamID 4 instead of 9 after the setter); with this change it passes.

Second test, http2 setNextStreamID ignores a fractional id below 1 instead of reusing stream ids: a client uses stream 1, calls setNextStreamID(0.5) and requests again. Node keeps nextStreamID at 3 and uses 3; main (with #37542) and the released bun rewind to 1 and hand out stream 1 again, so the test fails there on after and secondId, and passes with this change. It runs in-process: #37542's own test already covers the input that used to abort, and nothing aborts on it any more.

Also passing on the debug build, rebased onto current main: all of node-http2.test.js (359 tests, #37542's included), h2-conformance, h2-late-rst-staged, h2-push-refusal-staged, node-http2-streams-rehash, node-http2-continuation, node-http2-invalid-padding, and the ported node tests that exercise push or stream ids (test-http2-no-more-streams, test-http2-client-setNextStreamID-errors, test-http2-session-stream-state, test-http2-client-destroy, test-http2-server-push-stream*, test-http2-respond-file-push, test-http2-compat-serverresponse-createpushresponse, test-http2-misbehaving-multiplex, test-http2-invalid-last-stream-id, the goaway and rst tests; 23 files in all). cargo fmt --check and cargo clippy -p bun_runtime are clean.

[coderabbitai]

Walkthrough

The HTTP/2 parser now tracks locally initiated stream IDs separately from processed stream IDs, preserves endpoint-specific parity, and handles setNextStreamID() values without resetting allocation. Regression tests cover server push allocation and fractional values.

Changes

HTTP/2 stream ID management

Layer / File(s)	Summary
Local stream ID state and allocation src/runtime/api/bun/h2_frame_parser.rs	The parser stores a dedicated local stream counter, initializes it from the endpoint role, advances it for matching-parity streams, reports it in state, and uses it for stream allocation. set_next_stream_id preserves parity and ignores zero.
Stream ID regression validation test/js/node/http2/node-http2.test.js	Tests validate independent server push IDs, setNextStreamID(6), state reporting, and fractional setNextStreamID(0.5) handling.

Possibly related PRs

• oven-sh/bun#37553: Both changes address HTTP/2 stream ID parity, allocation, and setNextStreamID behavior.

Suggested reviewers: jarred-sumner, cirospaciari, dylan-conway

🚥 Pre-merge checks | ✅ 2 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	#39 concerns Node.js-compatible build output, but this PR changes HTTP/2 stream ID handling and addresses none of that issue's coding requirements.	Link the PR to the relevant HTTP/2 issue, or add changes that address #39 requirements such as CommonJS output, external node:* modules, require handling, or build parallelization.
Out of Scope Changes check	⚠️ Warning	The HTTP/2 parser and regression tests are unrelated to #39's Node.js build-output objectives, so the changes are out of scope for the linked issue.	Relink this PR to the applicable HTTP/2 issue or remove these changes from the #39 implementation.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: separating locally initiated HTTP/2 stream IDs from peer stream IDs.
Description check	✅ Passed	The description explains the problem, fix, verification steps, test results, and relevant background in sufficient detail.

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 12:59 PM PT - Aug 12th, 2026}

❌ @robobun, your commit c2e5874 has 4 failures in Build #93537 (All Failures):

• step failed outside runner - exit 22 on 🐧 aarch64-musl - verify-baseline
• test/cli/install/migration/complex-workspace.test.ts - code 1: error: Failed to install on 🐧 13 x64-asan
• test/cli/install/migration/complex-workspace.test.ts - code 1: error: Failed to install on 🐧 13 x64
• /opt/winsysroot/Windows Kits/10/Include/10.0.26100/um/winver.h - clang warning on 🪟 aarch64 - build-bun
• /opt/winsysroot/Windows Kits/10/Include/10.0.26100/um/winver.h - clang warning on 🪟 x64 - build-bun
• scripts/build/ci.ts - build failed on 🐧 x64-android - build-bun
• scripts/build/ci.ts - build failed on 🍎 aarch64 - build-bun
• scripts/build/ci.ts - build failed on 🐧 aarch64-android - build-bun
• scripts/build/ci.ts - build failed on :freebsd: x64 - build-bun
• scripts/build/ci.ts - build failed on :freebsd: aarch64 - build-bun

---

🧪   To try this PR locally:

bunx bun-pr 37676

That installs a local version of the PR into your bun-37676 executable, so you can run:

bun-37676 --bun

[robobun]

Status

Reproduced on bun 1.4.0 and main with the script in the description: after client streams 1, 3, 5, 7 the server reports state.nextStreamID 2, 4, 6, 8 and pushes on stream 8; node v26.3.0 reports 2 throughout and pushes on 2. Same sequence as test/js/node/http2/node-http2.test.js "http2 server push stream ids are independent of the client's stream ids", which fails on the released binary and passes with this branch.

Fix is in this PR (#37676): a dedicated next_stream_id counter in h2_frame_parser.rs, moved only by local allocations and setNextStreamID(). Rebased onto main after #37542 merged into the same two functions (its test is kept and passes here); all review threads are addressed.

CI: the diff itself is green; what is red is infrastructure. Build 93241 (5ddea63, the current source) built and passed the tests on every lane, with the only non-passing jobs being two darwin 26 aarch64 test jobs that expired waiting for an agent. The re-run 93537 (same tree plus an empty commit) lost its darwin, android, freebsd and windows build lanes to "Failed to download after 5 attempts" for the c-ares / mimalloc / libuv / lol-html tarballs and the WebKit prebuilt from github.com, plus an aarch64-musl verify step that failed outside the runner; the Linux lanes that did build passed the http2 suites, and the one hard test failure there (test/cli/install/migration/complex-workspace.test.ts, "install failed") is unrelated to this change and has been reported separately. A Buildkite retry of the failed jobs is all this needs; I am not pushing further retrigger commits.

[claude]

I reviewed this PR and didn't find any bugs. Because it reworks HTTP/2 stream-id bookkeeping in the frame parser and interacts with several open PRs (#37542, #37553, #37588) touching the same setter/getter, a human look would still be worthwhile.

What was reviewed:

• next_stream_id advance in handle_received_stream_id: id + 2 cannot overflow (ids ≤ MAX_STREAM_ID = i32::MAX, well within u32).
• setNextStreamID edge inputs (0, peer parity, u32::MAX) — saturating_add(1) and .max(first_local_stream_id()) handle them; last_stream_id is no longer mutated so highest_started_stream_id()/GOAWAY are unaffected.
• All former get_next_stream_id() call sites updated; the client-side PUSH_PROMISE receipt path (which bumps last_stream_id/last_peer_stream_id directly) correctly leaves next_stream_id alone.
• Test: failure events wired to a shared reject, sequential requests avoid ordering flake, cleanup in finally.

Extended reasoning...

Overview

Adds a dedicated next_stream_id: Cell<u32> to H2FrameParser so the next locally-initiated stream id (client requests / server pushes) is tracked independently of last_stream_id (the all-direction high-water mark). get_next_stream_id() is removed; getCurrentState, getNextStream, and the no-id path of request read the new field directly. setNextStreamID now writes only next_stream_id (previously it back-computed a predecessor into last_stream_id). handle_received_stream_id advances next_stream_id by 2 whenever a stream of local parity is registered. A comprehensive new test in node-http2.test.js pins node v26.3.0's exact values for server nextStreamID across arriving client streams, push ids, the setter's effect on nextStreamID/lastProcStreamID, and the client's own ids.

Security risks

None identified. This is stream-id bookkeeping; no untrusted input parsing changed. The only arithmetic added is stream_identifier + 2 on a value already bounded to 31 bits by MAX_STREAM_ID checks at every entry point, and requested.saturating_add(1) in the setter — neither can overflow. The change actually removes the two subtraction underflows the old setNextStreamID had (server arg 0 → 0 - 2; u32::MAX cases).

Level of scrutiny

High. This is wire-protocol state in the HTTP/2 engine: the ids it hands out go into PUSH_PROMISE frames and interact with GOAWAY / late-RST tolerance via last_stream_id. The change is deliberately narrow and last_stream_id semantics are preserved (still raised by every registration; highest_started_stream_id() unchanged), but the setter's side-effect removal and the coordination with three other open PRs on the same code deserve a maintainer's eye.

Other factors

• Verified no remaining callers of the removed get_next_stream_id().
• has_local_parity is correct for both roles (server → even, client → odd); the existing peer_parity open-code just above it could be folded in later but that's cosmetic.
• The client-side PUSH_PROMISE handler (around line 4009) updates last_stream_id/last_peer_stream_id directly and does not go through handle_received_stream_id, so a client's odd next_stream_id is correctly untouched by incoming even push ids — matching the PR's stated node comparison.
• Test quality is strong: local server, sequential requests to avoid arrival-order races, all error/sessionError events reject a shared failure promise raced against each request, single toEqual on a composite object, cleanup in finally. It reaches into Symbol.for("::bunhttp2native::") because ServerHttp2Session.setNextStreamID isn't public yet — reasonable given #37547 is still open.
• Deferring rather than approving because this is a semantic change to protocol state that overlaps with #37542/#37553/#37588; a human should confirm the merge/supersession plan the description lays out.

[robobun]

One clarification on the review above: the PUSH_PROMISE handler near line 4009 that bumps last_stream_id directly is in the legacy inbound parser, which nothing calls any more (read_bytes has no callers; inbound traffic goes through rewrite_read and the engine in src/runtime/api/bun/h2/). On the live path a received PUSH_PROMISE only sets rewrite_pending_push, so it touches neither counter; the client's nextStreamID matched node before this change and still does, which is what the clientNextStreamID assertion in the new test pins.

On the overlap with #37542, #37553 and #37588: this PR does not change what setNextStreamID() accepts (still #37553's subject) and does not change where lastProcStreamID comes from (#37588's subject); it only changes where the next local id is stored. Whichever order these land in, the other two rebase onto a next_stream_id.set(...) in the setter, and #37542's saturating arithmetic has nothing left to apply to.

[claude]

I reviewed this PR and didn't find any bugs. Because it reworks HTTP/2 stream-id bookkeeping and overlaps with three other open PRs (#37542, #37553, #37588), and there are two unresolved comment-cop flags (one on the doc comment that was expanded per maintainer request, one on the setter comment at ~L8312), a human sign-off would still be worthwhile.

What was reviewed:

• next_stream_id init (default 1, reset to 1/2 after is_server is settled) and advance-by-2 in handle_received_stream_id only for local-parity ids — peer streams leave it alone.
• All former get_next_stream_id() call sites (getCurrentState, getNextStream, request) now read the field; no stragglers.
• Overflow at the edges: MAX_STREAM_ID + 2 fits in u32 and is gated by the > MAX_STREAM_ID check before registration; setNextStreamID(0) and setNextStreamID(u32::MAX) land on the values described via saturating_add + .max(first_local_stream_id()).
• last_stream_id / last_peer_stream_id writes are unchanged, so highest_started_stream_id() and GOAWAY paths are unaffected.

Extended reasoning...

Overview

The PR splits H2FrameParser's single last_stream_id counter into two: the existing high-water mark (unchanged) and a new next_stream_id that tracks only locally-initiated stream ids, matching nghttp2's model and node's observable state.nextStreamID. Changes are confined to src/runtime/api/bun/h2_frame_parser.rs (field addition, init, advance in handle_received_stream_id, and rewrites of set_next_stream_id / getNextStream / getCurrentState / request to read/write the new field) plus a comprehensive new test in test/js/node/http2/node-http2.test.js that pins server and client nextStreamID, push ids on the wire, and lastProcStreamID after a setter call — all against values produced by node v26.3.0.

Security risks

None identified. The change is internal bookkeeping on a per-session u32 counter; no new untrusted-input parsing, no allocation sizing, no cross-thread state. The overflow surface actually shrinks (predecessor arithmetic in the old setter is gone; the one remaining add uses saturating_add, and stream_identifier + 2 is only reached for ids ≤ MAX_STREAM_ID = 2^31-1, well within u32).

Level of scrutiny

Medium-high. This is protocol state in the HTTP/2 engine — a wrong id allocation would produce on-the-wire protocol errors or id collisions. The change is small and mechanically traceable, but it also intentionally alters observable behavior (push ids and state.nextStreamID) and interacts with three other open PRs touching the same getter/setter and adjacent lastProcStreamID logic. That coordination is a human call.

Other factors

• cirospaciari is already engaged and their one request (nghttp2 source references in the doc comment) was addressed in 3909747 and marked resolved.
• Two comment-cop bot flags remain unresolved. The one on the field doc comment (L1344) is in direct tension with the maintainer's request for more nghttp2 references — a human should decide whether to trim or dismiss. The one on the setter (L8312) is a two-line inline comment that could plausibly be shortened.
• CI (#92629) was still building at review time.
• The test is thorough: wires every error event to reject, awaits observable conditions, asserts exact values against node, and covers both server and client sides plus the setter's side-effect isolation from lastProcStreamID.

[coderabbitai]

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 3962-3969: Update the set-then-push test case around the native
setNextStreamID call to pass the odd requested ID 5 instead of 6, while
retaining the expected normalized next stream ID of 6 and the existing
assertions.

🪄 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: e4fe106b-3d05-4598-a398-835648063140

📥 Commits

Reviewing files that changed from the base of the PR and between 56f7772 and c5f622e.

📒 Files selected for processing (2)

• src/runtime/api/bun/h2_frame_parser.rs
• test/js/node/http2/node-http2.test.js

[claude]

I reviewed this PR and didn't find any bugs. Since it reworks HTTP/2 stream-id bookkeeping in h2_frame_parser.rs, changes wire-visible push ids, and overlaps with several open PRs (#37542/#37547/#37553/#37588), a human look is still worthwhile.

What was reviewed:

• next_stream_id init, advance-on-local-parity in handle_received_stream_id, and reads in getCurrentState/getNextStream/request — consistent, no remaining callers of the removed get_next_stream_id.
• Overflow on stream_identifier + 2 — callers cap at MAX_STREAM_ID (2³¹−1), so it stays in u32; saturating_add(1) covers the setter's off-parity edge.
• setNextStreamID(0) no-op path and that last_stream_id/last_peer_stream_id are no longer touched by the setter.
• New tests: failure paths reject the awaited promise, subprocess pipes drained concurrently, sequential requests avoid ordering races.

Extended reasoning...

Overview

This PR splits the HTTP/2 "next local stream id" out of the shared last_stream_id counter in H2FrameParser, adding a dedicated next_stream_id: Cell<u32> that only local-parity stream registrations and setNextStreamID() move — matching nghttp2's next_stream_id and node's state.nextStreamID. It removes get_next_stream_id(), adds first_local_stream_id()/has_local_parity() helpers, rewrites the setter to store directly (ignoring 0), and wires the constructor to initialize the field per is_server. Two new tests in node-http2.test.js pin the server-push id sequence, nextStreamID/lastProcStreamID after setNextStreamID, client-side ids, and the setNextStreamID(0.5) no-op.

Security risks

None identified. This is internal id-allocation bookkeeping; no untrusted input parsing changes, no auth/crypto surface. The only arithmetic added (+ 2, saturating_add(1)) operates on values already bounded by MAX_STREAM_ID checks at every call site that reaches handle_received_stream_id.

Level of scrutiny

High. This is production HTTP/2 protocol state in native code, changes on-wire push stream ids, and sits in the middle of a cluster of related open PRs (#37542, #37547, #37553, #37588) whose rebase story the description spells out but a maintainer should confirm. cirospaciari is already engaged on the PR (requested and resolved the nghttp2 source-pointer comment), so human review is already in flight.

Other factors

The comment-cop bot is still flagging the next_stream_id doc comment as too long, but that content was explicitly requested by the maintainer and the thread is resolved — leaving that call to them. The tests look solid (error events wired to reject, sequential requests to avoid arrival-order flake, spawned fixture for the debug-abort case with concurrent pipe drain), and the description reports the full h2 suite plus 23 ported node tests passing. I found no correctness issues, but the scope and cross-PR coordination put this outside auto-approval.

[claude]

I reviewed this PR and didn't find any bugs. Since it changes HTTP/2 stream ID allocation (protocol-critical) and coordinates with three other open PRs (#37542, #37553, #37588), a final look from cirospaciari would be worthwhile — their earlier feedback on the doc comment appears addressed as of 86c690a.

What was reviewed:

• next_stream_id advance in handle_received_stream_id — only local-parity ids move it; + 2 cannot overflow because callers gate on MAX_STREAM_ID first.
• set_next_stream_id no longer touching last_stream_id — confirmed highest_started_stream_id() and the GOAWAY/late-RST paths still read last_stream_id, which is still raised for every registration.
• All former get_next_stream_id() call sites (getCurrentState, getNextStream, request) updated; no stragglers.
• Constructor ordering — next_stream_id is reset from first_local_stream_id() right after is_server is settled, before any stream can register.

Extended reasoning...

Overview

The PR adds a dedicated next_stream_id: Cell<u32> to H2FrameParser so the next locally-initiated stream id is tracked independently of last_stream_id (the both-directions high-water mark). It removes get_next_stream_id() (which derived the next id from last_stream_id), adds first_local_stream_id() / has_local_parity() helpers, rewrites set_next_stream_id() to store into the new field (ignoring 0, parity-normalizing with saturating_add), and advances the counter inside handle_received_stream_id when a local-parity stream is registered. Two new tests in node-http2.test.js pin the exact values node v26.3.0 produces for a push sequence and for setNextStreamID(0.5).

Security risks

None identified. This is stream-id bookkeeping; no untrusted input reaches new arithmetic without an existing > MAX_STREAM_ID gate. Checked that stream_identifier + 2 in handle_received_stream_id cannot wrap: every caller (getNextStream, request) rejects ids above MAX_STREAM_ID = 2^31-1 before registration, and wire ids are 31-bit. set_next_stream_id uses saturating_add(1) for the parity bump.

Level of scrutiny

High. HTTP/2 stream id allocation is protocol-critical — a wrong id can collide with an in-use stream or violate RFC 9113 §5.1.1 monotonicity. The change also decouples setNextStreamID from last_stream_id, which feeds highest_started_stream_id() (late-RST tolerance) and GOAWAY. The PR description argues this decoupling is the fix (node/nghttp2 don't couple them) and cites the passing h2-late-rst-staged and GOAWAY tests, but that's exactly the kind of invariant a maintainer who owns this subsystem should sign off on.

Other factors

• cirospaciari has been actively reviewing (requested nghttp2 source pointers, comment shortening, line-anchored link) and all threads are resolved, but they have not approved.
• The PR explicitly overlaps with three other open PRs (#37542, #37553, #37588) touching the same setter/getter; landing order matters and the description spells out how each rebases, which is a coordination call best made by a human.
• Tests are thorough (exact node-derived values, error events wired to reject, subprocess pipes drained concurrently, spawned fixture for the debug-abort case), and the description lists 23 passing http2 test files plus the USE_SYSTEM_BUN=1 failure.
• No CODEOWNERS entry for this path.

Given the protocol sensitivity and the multi-PR coordination, deferring rather than approving.

[claude]

I reviewed this PR and didn't find any bugs. Since it changes HTTP/2 stream-id bookkeeping and interacts with several other open PRs (#37542, #37553, #37588, #37547), a final look from cirospaciari would be worthwhile before merge.

What was reviewed:

• next_stream_id init/advance: seeded from first_local_stream_id() after is_server is settled; advanced only on local-parity registrations in handle_received_stream_id; peer streams and the legacy PUSH_PROMISE path (dead read_bytes) leave it alone.
• Overflow: stream_identifier + 2 is bounded by MAX_STREAM_ID + 2 (all callers gate on > MAX_STREAM_ID first); the setter's saturating_add(1) handles u32::MAX.
• last_stream_id unchanged for GOAWAY / highest_started_stream_id; setNextStreamID no longer touches it, so lastProcStreamID is unaffected.
• Tests: both new tests assert node v26.3.0 values, wire error events to reject, and the fractional-id test doesn't await the second request so it fails on the assertion rather than hanging on the released binary.

Extended reasoning...

Overview

The PR adds a dedicated next_stream_id: Cell<u32> to H2FrameParser, decoupling the next locally-initiated stream id from last_stream_id (the highest id seen in either direction). Readers (getCurrentState, getNextStream, request's no-id path) and the setter (setNextStreamID) move to the new field; get_next_stream_id() and its predecessor arithmetic are deleted. Two new tests in node-http2.test.js pin node v26.3.0's observable values for server push ids, state.nextStreamID, lastProcStreamID after the setter, and the setNextStreamID(0.5) edge.

Security risks

None identified. This is bookkeeping for locally-initiated stream ids; the RFC 9113 §5.1.1 monotonicity constraint on the wire was already satisfied and remains so. The setter's new early-return on 0 removes a debug-build underflow panic, and the + 2 in handle_received_stream_id cannot overflow because every caller gates the id at MAX_STREAM_ID (2^31-1) before registration.

Level of scrutiny

Medium-high. The change is small and mechanically clean, but it sits in protocol state management, is modeled on an external reference implementation (nghttp2), and the PR description explicitly enumerates four other open PRs whose diffs overlap or become obsolete on top of this one. A maintainer (cirospaciari) is already actively reviewing — their feedback so far (nghttp2 source pointer, comment length, line-anchored link) has all been addressed and the threads are resolved — but they have not yet signed off on the substance.

Other factors

The tests are well-constructed: error events wired to reject, exact-value assertions on a combined object, sequential requests to avoid ordering races, and the description records the USE_SYSTEM_BUN=1 failure mode. The fractional-id test was moved in-process in the latest commit (5ddea63), which is safe now that the setter no longer panics on 0; on the released binary it fails on the {after: 3, secondId: 3} assertion rather than hanging because the second request is not awaited. The CodeRabbit suggestion to test setNextStreamID(5) was correctly declined (it would pin a pre-existing divergence that #37553 removes). Given the cross-PR coordination and the maintainer already in the loop, deferring for their final approval rather than auto-approving.

[claude]

I reviewed this PR and didn't find any bugs. The change is well-scoped and thoroughly tested against node v26.3.0, and all review threads are resolved — but since it reworks stream-id state in the HTTP/2 frame parser, a final look from cirospaciari (who's been actively engaged) would be worthwhile before merge.

What was reviewed:

• next_stream_id advancement in handle_received_stream_id — verified stream_identifier + 2 cannot overflow (all callers guard > MAX_STREAM_ID and wire ids are 31-bit).
• set_next_stream_id rewrite — parity rounding preserved, saturating_add(1) handles u32::MAX inputs, 0 ignored per nghttp2; last_stream_id/lastProcStreamID no longer touched.
• Constructor ordering — next_stream_id initialized to 1 then reset to first_local_stream_id() after is_server is settled.
• get_next_stream_id() fully removed with no remaining callers; getNextStream/request/getCurrentState all read the new field.

Extended reasoning...

Overview

This PR splits HTTP/2 local stream-id allocation out of last_stream_id into a dedicated next_stream_id counter on H2FrameParser, matching nghttp2's next_stream_id semantics so that state.nextStreamID, pushStream() ids, and setNextStreamID() behavior align with Node.js. The native change is ~30 lines in src/runtime/api/bun/h2_frame_parser.rs (one new field, two small parity helpers replacing get_next_stream_id(), a rewritten set_next_stream_id, one advancement site in handle_received_stream_id, constructor init). Two new tests (~130 lines) in node-http2.test.js pin the exact values node v26.3.0 produces for a push sequence and for setNextStreamID(0.5) on a used session.

Security risks

None identified. The change is bookkeeping over a u32 counter. Checked the one non-saturating arithmetic site (stream_identifier + 2): all paths into handle_received_stream_id are bounded by MAX_STREAM_ID (2³¹-1) or 31-bit wire ids, so the sum stays in u32. set_next_stream_id uses saturating_add for the parity bump. No new untrusted-input parsing.

Level of scrutiny

Moderate-to-high. This is protocol-level state in the HTTP/2 frame parser, and while the change is small and the reasoning in the PR description is meticulous (traced against nghttp2 source, cross-checked against node, interactions with #37542/#37553/#37588 spelled out), stream-id bookkeeping errors can cause protocol violations or id reuse. The maintainer for this area (cirospaciari) has been actively reviewing and has only requested comment-length/link-format tweaks so far, all resolved — which suggests the substance is acceptable, but they haven't formally approved.

Other factors

• All inline threads resolved (cirospaciari's comment requests, comment-cop, CodeRabbit's withdrawn parity suggestion).
• The PR description documents fails-without-fix / passes-with-fix on both ASAN debug and release, plus 23 related test files passing.
• last_stream_id and its consumers (highest_started_stream_id, GOAWAY paths, late-RST tolerance) are deliberately untouched; the new test's lastProcStreamID: 9 assertion pins that the setter no longer perturbs them.
• Tests follow harness conventions (port 0, error events wired to reject, Promise.race against a failure promise, cleanup in finally).

Given the maintainer's active engagement and that this is a non-mechanical change to protocol state, deferring for their final sign-off rather than auto-approving.