node:http2: name only peer streams in GOAWAY and never raise the id (RFC 9113 6.8) by robobun · Pull Request #37588 · oven-sh/bun

robobun · 2026-08-11T20:02:02Z

Problem

A GOAWAY from bun can name a stream that the sender opened. A client sends last_stream_id=1 after one request (node: 0). nghttp2 answers GOAWAY: invalid last_stream_id, a connection error.
The id can rise: after session.goaway(0, 1), destroy() sends 3 (node: 1).
The cause: local_connection_error (src/runtime/api/bun/h2/connection.rs:448) and four senders in h2_frame_parser.rs write last_stream_id, which counts both sides.

Fix

Connection owns last_proc_stream_id: requests on a server, promised streams on a client. Every automatic GOAWAY carries it.
next_goaway_last_stream_id lowers each id to the last GOAWAY's id. goaway(code, id) sends nothing when only the sender can open id.
Verified: test/js/node/http2/h2-goaway-last-stream-id.test.ts, 20 tests. All pass with node --test on v26.3.0, 18 fail on main. Also 261 node test-http2-* files and grpc-js suites.
Self-reviewed: 15 concerns raised, 10 addressed. 4 are separate work, 1 is rejected (Notes).

Background

Last-Stream-ID (RFC 9113 6.8) is the highest peer stream that the sender processed. A later GOAWAY must not carry a higher id.
Connection parses inbound frames. H2FrameParser writes the GOAWAY frames that JS asks for, so it keeps a copy.
Considered a fix of the budget GOAWAY alone: four other senders keep the wrong id.

Downsides

As in node, a client's state.lastProcStreamID is 0 until a push arrives (main: the last request id), and goaway(code, id) with the sender's own id sends nothing.
A stream that JS refuses after the engine opened it is still named: a request over maxConcurrentStreams (fix in node:http2: refuse a stream over the advertised limit before it is opened, on maxSessionInvalidFrames #44337), a push over maxReservedRemoteStreams.
A session grows 16 bytes. Release .text grows 256 bytes.

Notes

Which stream node names. A server has an open request on stream 1. A second request arrives on stream 3. Then the session closes. The table shows the Last-Stream-ID of the GOAWAY.

request on stream 3	node v26.3.0	main	this PR
malformed (no `:path`)	3	3	3
HPACK decode error	3	3	3
refused for `maxSessionMemory`	3	1	3
refused over `maxConcurrentStreams`	1	3	3
over both limits	1	1	3
refused by the `'stream'` handler	3	3	3

nghttp2 sets last_proc_stream_id after its stream-limit checks and before the on_begin_headers callback. Node refuses for maxSessionMemory in that callback. So node counts a stream that it refused for memory, and does not count a stream over the limit.

The report about the rejected-stream limit. A server has maxConcurrentStreams: 1 and maxSessionRejectedStreams: 3. Stream 1 is open. HEADERS arrive on 3, 5 and 7. Main and this PR send GOAWAY(ENHANCE_YOUR_CALM) with Last-Stream-ID 7. Node names 1: it sends GOAWAY(PROTOCOL_ERROR) with id 1 when the client ACKed the limit. When the client did not, node sends three RST_STREAM(REFUSED_STREAM) and no GOAWAY. The report is correct. The cause is that the engine opens each of those streams before JS refuses it. #44337 refuses the stream before it opens, and its test a refused stream is not the last stream that the session processed covers the id. This PR does not do that work a second time. The match on BlockDisposition in handle_headers has no wildcard arm. A new refusal variant does not compile until it says whether the stream counts.

The client has the same gap for a push. With maxReservedRemoteStreams: 1 and promises of 2 and 4, JS cancels stream 4 after the engine reserved it. Node names 2, this PR names 4, bun 1.4.2 names 0. That refusal must move into the engine too. It is separate work.

The id does not rise. Three rules keep the id of a later GOAWAY at or below the id of an earlier one.

After a GOAWAY of ours, a request that the session refuses and a promised stream do not raise the mark. A client calls goaway(), then the server sends PUSH_PROMISE(2) and a PING with a bad length. Ids of the client's GOAWAY frames: node 0, 0. Main 0, 2, 0. This PR 0, 0, 0.
H2FrameParser::next_goaway_last_stream_id lowers each id to the id of the last GOAWAY (nghttp2's local_last_stream_id). The engine reaches it through Sink::clamp_goaway_last_stream_id. A server with requests on 1 and 3 calls session.goaway(0, 1). Ids after close(), destroy(), a second goaway() or a connection error: node 1, 1. Bun 1.4.2: 1, 3. This PR 1, 1. A client with pushed streams 2 and 4 calls goaway(0, 2): node 2, 2. Bun 1.4.2: 2, 0 (2, 4 after a connection error). This PR 2, 2.
goaway(code, id) with an id that only the sender can open sends nothing. nghttp2 returns NGHTTP2_ERR_INVALID_ARGUMENT and node ignores it. Bun 1.4.2 sends that GOAWAY. With rule 2 alone, every later GOAWAY would then carry the sender's own id. The id is read with ToInt32, as node reads it: 2 ** 31 and Infinity mean the last processed stream, and 2 ** 32 + 2 is the id 2. For 11 ids outside the int32 range, on a client and on a server, the frames are the same as node's.

Node applies rule 2 when it writes the frame. So goaway(0, 1) and close() in one tick give 1, 3 in node. Bun applies the rule at the call and gives 1, 1.

Bun still serves a request that opens after the server's GOAWAY (#43475 covers it). Such a request raises state.lastProcStreamID, but the next GOAWAY keeps the earlier id. A server sends GOAWAY with id 1, then a request arrives on 3, then close(): node does not serve it and sends 1, 1. Bun 1.4.2 serves it and sends 1, 3. This PR serves it and sends 1, 1. #43612 discards the late PUSH_PROMISE itself. Node also closes the open streams above the id of a GOAWAY that it sent. Bun leaves them open. That is separate work.

The 20 tests. All are in test/js/node/http2/h2-goaway-last-stream-id.test.ts and use a raw socket as the peer. On a debug build of main (71d0d43) 18 fail and tests 8 and 9 pass. The value in parentheses is what bun 1.4.2 gives.

Server close() after a request refused for maxSessionMemory: names 3 (1).
Client connection error after one request: names 0, state.lastProcStreamID is 0 (1).
Client connection error after requests 1 and 3 and a push of 2: names 2 (3).
Client close() after a push: names 2 (0).
Client with maxSessionRejectedStreams, after a refused response: each GOAWAY names 0 (1).
Client GOAWAY after a trailer block that HPACK cannot encode: names 0 (1).
Client with maxOutstandingSettings: 0, second settings() call: each GOAWAY names 0 (1).
Server connection error after requests 1 and 3: names 3. Passes without the fix.
Server, a new request on 1 after a request on 3: state.lastProcStreamID stays 3 and close() names 3. Passes without the fix.
Server that pushed 2 and 4, then a malformed request on 3, then close(): each GOAWAY names 3 (4).
Server with maxConcurrentStreams: 1 that pushed 2 and 4, then a request on 3: no GOAWAY names an even id (4).
Server, request refused for maxSessionMemory after session.goaway(), then a connection error: names 1 (3).
Client, PUSH_PROMISE after client.goaway(): state.lastProcStreamID stays 0 and the next GOAWAY names 0 (1, then 2).
Server goaway(0, 1), goaway(0, 3), destroy(): three GOAWAY frames, each names 1 (1, 3, 3).
Server goaway(0, 1), then a connection error: names 1 (3).
Client goaway(0, 2) after pushes 2 and 4, close(), destroy(err): three GOAWAY frames, each names 2 (lastProcStreamID is 1).
Server goaway(0, 2): no GOAWAY. destroy() then names 3 (2, 3).
Client goaway(0, 1): no GOAWAY. close() then names 4 (1, 0).
Server goaway(0, 2 ** 32 + 2): no GOAWAY. goaway(0, 2 ** 31) names 3 (2147483647).
Client goaway(0, 2 ** 31) after pushes 2 and 4: names 4 (2147483647).

Tests 5, 7, 10 and 11 assert the id of every GOAWAY and not the number of frames. Node and bun write a different set of GOAWAY frames when a session budget is used up. Test 10 uses a malformed request, because #44248 stops the count of a stream that the handler refuses. Tests 7 and 11 cover the GOAWAY of the pending SETTINGS limit and of the rstStream refusal budget. Tests 14 and 16 write three frames, so a helper that keeps the last requested id and not the lowest sent id fails. A wait for a stream or session event rejects when that stream or session closes first.

The file uses node:test and node:assert, so node --test test/js/node/http2/h2-goaway-last-stream-id.test.ts runs it as it is. The cases are not in h2-conformance.test.ts, because that file holds cases that fail now and then on a debug build: on main, a stream-reset flood case timed out in 1 of 2 runs.

Engine unit tests. connection.rs has 7 new #[test]s against a bare Connection. The workspace cannot run the unit tests of bun_runtime. A standalone crate that compiles the h2/ files with lshpack runs them: 31 tests pass. Four mutations fail the matching tests: count a stream refused after our GOAWAY (1 fails), count a stream promised after our GOAWAY (1 fails), write last_stream_id into the GOAWAY (5 fail), skip the clamp hook (1 fails).

Cost.

Connection is 448 bytes on main and 456 bytes here. H2FrameParser is 1504 bytes on main and 1512 bytes here (Cell<Option<u32>> for the id of the last GOAWAY). Both from the debug binary with gdb.
Release build, main and this PR: .text 58,161,589 and 58,161,845 bytes, .rodata 19,804,908 bytes in both, stripped binary 80,844,360 bytes in both.
Per request on a server: the parity test and the compare in handle_received_stream_id are gone. handle_headers does one compare, and one Cell store when the mark rises. Per GOAWAY: one min and one Cell store. No allocation and no JS call is new.

Other suites. On a debug build of this branch, after the merge of main: the 10 other test files of test/js/node/http2/, 261 node test-http2-* files and the grpc-js server, server-errors, retry, idle-timer, server-deadlines and channelz suites pass. h2-conformance.test.ts passed in 1 of 2 runs with --timeout 30000: a stream-reset flood case timed out, as it does on main.

Self-review. Two reviews ran: one on the mark change, one on the pushed diff with probes on node v26.3.0 and on a build of this PR. They raised 15 concerns.

A later GOAWAY could carry a higher id than goaway(code, id) sent. Addressed: rule 2.
An id that only the sender can open then stayed in every later GOAWAY. Addressed: rule 3.
The GOAWAY of the rstStream budget and of the pending SETTINGS limit had no test. Addressed: tests 7 and 11.
The mark had the name of a different value in node:http2: reject a PUSH_PROMISE whose promised id does not exceed the previous one #37563 and h2 server: decode and discard HEADERS on a closed client stream instead of resetting or re-opening it #37985. Addressed: it is last_proc_stream_id now.
Comments of more than one line. Addressed.
The id was read with a saturating conversion, so a client sent no GOAWAY for goaway(code, 2 ** 31). Addressed: ToInt32, tests 19 and 20.
The clamp was tested over two GOAWAY frames only. Addressed: tests 14 and 16.
No test gave the mark a lower id after a higher one. Addressed: test 9.
Test 12 passed without the fix. Addressed: it ends with a connection error now.
A wait with once() did not end when the stream closed first. Addressed.
Node closes the streams above the id of a GOAWAY that it sent. Separate work.
A push over maxReservedRemoteStreams still counts. Separate work.
goaway() with more than 16376 octets of opaque data sends a frame that node does not send. Not from this diff: node:http2: send nothing from goaway() when opaqueData does not fit a GOAWAY frame #42461.
Over a Duplex transport the GOAWAY of a connection error does not reach the wire. Not from this diff: node:http2: deliver the destroy() GOAWAY on sessions whose socket has no native handle #38158.
CI compiles the #[test]s of connection.rs and does not run them. Rejected: no crate test of bun_runtime runs in CI, and the ten older tests of this module are in the same state. The JS tests cover each rule.

The second review also ran 700 random sequences of requests, pushes, resets, goaway(code, id) calls and shutdowns on node and on the previous head of this PR. The GOAWAY ids and state.lastProcStreamID were the same in all 700.

Open PRs that touch the same lines.

node:http2: count a refused stream where the session refuses it, not in rstStream #44248 and node:http2: refuse a stream over the advertised limit before it is opened, on maxSessionInvalidFrames #44337 change on_stream_rejected and the budget block in rst_stream. Their GOAWAY call passes self.last_stream_id.get(). After this PR it passes None.
node:http2: refuse a stream over the advertised limit before it is opened, on maxSessionInvalidFrames #44337, node:http2: ignore a request that opens after the server's GOAWAY #43475, node:http2: discard a PUSH_PROMISE that arrives after the client sent GOAWAY #43612 and node:http2: refuse a PUSH_PROMISE on a request stream that user code already closed #43465 add BlockDisposition variants for a stream that never opens. Each needs an arm in the new match that does not count the stream. A push that node:http2: refuse a PUSH_PROMISE on a request stream that user code already closed #43465 cancels must not reach note_processed_stream.
node:http2: ignore a request that opens after the server's GOAWAY #43475 adds the same field, sent_goaway_last_stream_id: Cell<Option<u32>>, and lowers the id in local_connection_error only. Keep one field and the helper of this PR.
node:http2: reject a PUSH_PROMISE whose promised id does not exceed the previous one #37563 and h2 server: decode and discard HEADERS on a closed client stream instead of resetting or re-opening it #37985 add Connection::last_peer_stream_id, the highest id that the peer used (nghttp2's last_recv_stream_id). That is a different value from last_proc_stream_id. Keep both.
node:http2: send nothing from goaway() when opaqueData does not fit a GOAWAY frame #42461 and node:http2: read the session.goaway() error code with ToUint32, like node #37558 edit the goaway host function next to the new check of the id.

[human-review] gate passed · iteration 2 · 3 files touched

fails on main (without fix)

ASAN without fix: 18 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" "test/js/node/http2/h2-goaway-last-stream-id.test.ts"
bun test v1.4.4 (367d939d9)

test/js/node/http2/h2-goaway-last-stream-id.test.ts:
357 |       await c.waitFor(f => f.type === FrameType.DATA && f.streamId === 1);
358 |       c.sendFrame(FrameType.HEADERS, 0x5, 3, requestHeaderBlock("GET"));
359 |       const rst = await c.waitFor(f => f.type === FrameType.RST_STREAM && f.streamId === 3);
360 |       assert.strictEqual(rst.payload.readUInt32BE(0), ErrorCode.ENHANCE_YOUR_CALM);
361 |       assert.deepStrictEqual(seen, [1]);
362 |       assert.strictEqual(session.state.lastProcStreamID, 3);
                   ^
AssertionError: Expected values to be strictly equal:

1 !== 3

 generatedMessage: true,
     actual: 1,
   expected: 3,
   operator: "strictEqual",
       diff: "simple",
       code: "ERR_ASSERTION"

      at /workspace/bun/test/js/node/http2/h2-goaway-last-stream-id.test.ts:362:14
      at node:test:1781:26
      at executeTestNode (node:test:1785:63)
      at processTicksAndRejections (native:34:24)
(fail) GOAWAY last-stream
... (truncated)

release without fix: 18 FAILED
bun test v1.4.3-canary.1 (367d939d9)

test/js/node/http2/h2-goaway-last-stream-id.test.ts:
357 |       await c.waitFor(f => f.type === FrameType.DATA && f.streamId === 1);
358 |       c.sendFrame(FrameType.HEADERS, 0x5, 3, requestHeaderBlock("GET"));
359 |       const rst = await c.waitFor(f => f.type === FrameType.RST_STREAM && f.streamId === 3);
360 |       assert.strictEqual(rst.payload.readUInt32BE(0), ErrorCode.ENHANCE_YOUR_CALM);
361 |       assert.deepStrictEqual(seen, [1]);
362 |       assert.strictEqual(session.state.lastProcStreamID, 3);
                   ^
AssertionError: Expected values to be strictly equal:

1 !== 3

 generatedMessage: true,
     actual: 1,
   expected: 3,
   operator: "strictEqual",
       diff: "simple",
       code: "ERR_ASSERTION"

      at /workspace/bun/test/js/node/http2/h2-goaway-last-stream-id.test.ts:362:14
      at node:test:1445:26
      at executeTestNode (node:test:1448:63)
      at processTicksAndRejections (native:7:39)
(fail) GOAWAY last-stream-id (RFC 9113 §6.8) > a graceful GOAWAY names a stream that was refused for maxSessionMemory [78.51ms]
375 |       const { client, requests } = await connectClient(raw, 1);
376 
... (truncated)

passes on PR (with fix)

ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" "test/js/node/http2/h2-goaway-last-stream-id.test.ts"
bun test v1.4.4 (367d939d9)

test/js/node/http2/h2-goaway-last-stream-id.test.ts:
(pass) GOAWAY last-stream-id (RFC 9113 §6.8) > a graceful GOAWAY names a stream that was refused for maxSessionMemory [1483.45ms]
(pass) GOAWAY last-stream-id (RFC 9113 §6.8) > a client's connection-error GOAWAY does not name its own request stream [709.35ms]
(pass) GOAWAY last-stream-id (RFC 9113 §6.8) > a client's connection-error GOAWAY names the last pushed stream, not its own higher request [262.76ms]
(pass) GOAWAY last-stream-id (RFC 9113 §6.8) > a client's close() after a push names the pushed stream [196.54ms]
(pass) GOAWAY last-stream-id (RFC 9113 §6.8) > no GOAWAY a client sends after a refused response names its own request [184.56ms]
(pass) GOAWAY last-stream-id (RFC 9113 §6.8) > a client's GOAWAY after an unencodable trailer block does not name its own request [365.75ms]
(pass) GOAWAY last-stream-id (RFC 9113 §6.8) > no GOAWAY a client sends after too many pending SETTINGS names its 
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped)
  target       linux-x64-gnu
  build type   Release
  build dir    ./build/release
  revision     2109f5f5ad
  features     lto, baseline

23 deps, 136 codegen, 1176 objects in 2066ms

ninja: Entering directory `/workspace/bun/build/release'
[1/4] fetch lolhtml
[lolhtml] up to date
[2/4] fetch rust-argon2
[rust-argon2] up to date
[2/4] cargo plan → /workspace/bun/build/release/rust-target/plan.json
247 units: 175 lib, 16 proc-macro (host), 19 custom-build (host), 15 run custom-build, 17 lib (host), 4 run custom-build (host), 1 rlib
[3/4] reconfigure
[1/1502] mkdir stamps
[2/1502] mkdir codegen
[3/1502] gen bake.{client,server,error}.js
-> bake.client.js, bake.server.js, bake.error.js
[4/1502] gen string-map defines_table
[5/1502] gen ErrorCode+*.h
[6/1502] install /workspace/bun
bun install v1.4.3-canary.1 (367d939d9)

Checked 26 installs across 65 packages (no changes) [1340.00ms]
[7/1502] install /workspace/bun/packages/bun-error
bun install v1.4.3-canary.1 (367d939d9)

Checked 1 install across 2 packages (no changes) [57.00ms]
[8/1502] rustc unicode_ident 
[9/1502] rustc unicod
... (truncated)

diff hotspot

src/runtime/api/bun/h2/connection.rs               | 285 ++++++-
 src/runtime/api/bun/h2_frame_parser.rs             |  80 +-
 .../js/node/http2/h2-goaway-last-stream-id.test.ts | 929 +++++++++++++++++++++
 3 files changed, 1258 insertions(+), 36 deletions(-)

gate history · 2 passed · 1 rejected · iteration 2

evidence per changed file

file                                                 reads  edits  tests
src/runtime/api/bun/h2/connection.rs                    10      9     21
src/runtime/api/bun/h2_frame_parser.rs                   5      2     30
test/js/node/http2/h2-goaway-last-stream-id.test.ts      0      0      7

The inbound engine wrote Connection::last_stream_id into the GOAWAY it sends on a locally detected connection error. That mark also counts locally initiated streams (a client's own requests, once their response arrives), so a client's GOAWAY could name one of its own odd ids, which RFC 9113 6.8 forbids and nghttp2 rejects as a connection PROTOCOL_ERROR. Connection now tracks last_peer_stream_id (request ids on a server, refused ones included; promised ids on a client), uses it for its own GOAWAYs, and pushes it to the embedder through a new Sink callback. The embedder's copy backs session.goaway()/close()/destroy(), the rejection budget GOAWAYs and state.lastProcStreamID, so all of them now report the same value node does (previously the copy never saw client pushes and the budget GOAWAYs used the mixed mark).

coderabbitai · 2026-08-11T20:02:12Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Essentials
Run ID: c02061e8-853f-4d97-b57e-9e6af6f20753

📥 Commits

Reviewing files that changed from the base of the PR and between 5f3547f and 2109f5f.

📒 Files selected for processing (2)

src/runtime/api/bun/h2_frame_parser.rs
test/js/node/http2/h2-goaway-last-stream-id.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.

Walkthrough

The HTTP/2 connection engine now tracks processed peer streams separately from all streams. The frame parser uses that state to select GOAWAY last-stream IDs, enforce non-increasing IDs, and report the selected ID in callbacks and current state. New tests cover these behaviors.

Changes

HTTP/2 GOAWAY stream tracking

Layer / File(s)	Summary
Track processed peer streams `src/runtime/api/bun/h2/connection.rs`	The connection tracks the highest processed peer stream and reports advances through the sink. Local connection errors use a clamped processed ID in GOAWAY frames and error callbacks.
Apply processed IDs to GOAWAY `src/runtime/api/bun/h2_frame_parser.rs`	The parser uses the engine’s processed-stream ID as the default GOAWAY ID and prevents later GOAWAY IDs from increasing. It also uses the selected ID in callbacks and current-state reporting.
Validate GOAWAY last-stream IDs `test/js/node/http2/h2-goaway-last-stream-id.test.ts`	Raw-frame tests cover processed and refused streams, PUSH_PROMISE, later GOAWAY frames, `session.goaway()` inputs, and int32 conversion.

Possibly related PRs

oven-sh/bun#43475: Tracks GOAWAY limits and peer streams, extending the same stream-processing and GOAWAY state.

Suggested reviewers: dylan-conway

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 2109f

No actionable GOAWAY issue remains identified; the change is mergeable after normal checks.

Pre-merge checks | 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Title check		The title clearly identifies the main HTTP/2 GOAWAY change: naming only peer streams and preventing the Last-Stream-ID from increasing.
Description check		The description provides detailed problem, fix, verification results, test coverage, known limitations, and related work. It does not use the template headings exactly, but it contains the required in…

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

robobun · 2026-08-11T20:02:57Z

Status: reproduced on bun 1.4.0 with a raw net server. After a request on stream 1, the client's GOAWAY carries last_stream_id=1 (node v26.3.0 sends 0); with a push of stream 2 plus requests on 1 and 3, bun sends 3 and node sends 2. Fix and tests are in this PR; the seven failing-before cases are listed in the description. Review threads are addressed; waiting on CI for the current head.

claude

I reviewed this PR and didn't find any bugs. Because it changes wire-level HTTP/2 GOAWAY semantics in the native engine and overlaps with two other in-flight PRs (#37563, #36343) that add the same last_peer_stream_id field, a human look would still be worthwhile to sequence the merges.

What was reviewed:

note_peer_stream advances both marks and pushes to the Sink before the stream is surfaced, so callback-triggered GOAWAYs see the updated value; ordering looks correct.
The remaining self.last_stream_id.get() calls in h2_frame_parser.rs are either in the dead legacy inbound parser (per the file header) or in paths where the value is informational (on_too_many_invalid_frames, unsolicited-PING-ACK) and the actual GOAWAY comes from destroy()→goaway() which already reads last_peer_stream_id.
The seven wire-level tests await concrete frame conditions (no sleeps), use port: 0, wire error handlers, and were verified against node v26.3.0 per the description.

Extended reasoning...

Overview

The PR adds Connection::last_peer_stream_id to the HTTP/2 engine (src/runtime/api/bun/h2/connection.rs), tracking the highest peer-initiated stream id (request ids on a server, promised ids on a client) separately from the existing last_stream_id which also counts locally-initiated streams. local_connection_error now writes the peer mark into every engine-generated GOAWAY, and a new Sink::on_last_peer_stream_id callback keeps the embedder's mirror (H2FrameParser.last_peer_stream_id) in sync so its own GOAWAYs (session.goaway()/close()/destroy(), budget-triggered ones, state.lastProcStreamID) carry the same value. Eight call sites in h2_frame_parser.rs are switched from last_stream_id to last_peer_stream_id; the parity-based advance in handle_received_stream_id is removed in favor of the engine callback. Four Rust #[test]s and seven wire-level TS tests are added.

Security risks

None identified. The change only affects which stream id is written into an outbound GOAWAY frame's last-stream-id field — a 4-byte informational value the peer uses to decide which requests to retry. There is no new parsing of untrusted input, no allocation sizing driven by peer data, and no auth/crypto surface. The refused-stream path already advanced last_stream_id for §5.1 checks; adding the peer-mark advance there is monotonic and bounded by the u32 stream id space.

Level of scrutiny

High. This is protocol-critical native code on the HTTP/2 wire path used by both client and server sessions. RFC 9113 §6.8 conformance affects interop with nghttp2-based peers (which reject a GOAWAY naming the sender's own stream with a connection PROTOCOL_ERROR). While the change itself is narrow and mechanically clear (one new field, one new callback, a handful of last_stream_id → last_peer_stream_id swaps), a wrong value here silently breaks graceful shutdown against real servers. The PR description is unusually thorough — every expected value was reproduced against node v26.3.0, and both the client-with-push and server-refused-stream edge cases are covered — but that thoroughness itself argues for a maintainer confirming the semantics against nghttp2's last_proc_stream_id.

Other factors

Overlapping PRs: the description explicitly flags #37563 and #36343 as adding an engine-side last_peer_stream_id with the same definition for §5.1 checks. A human needs to decide merge order and reconcile.
Remaining last_stream_id uses: I checked the ~60 remaining self.last_stream_id.get() sites in h2_frame_parser.rs. Nearly all are in the legacy inbound parser marked dead at the file header. The two live-looking ones in the Sink impl (on_too_many_invalid_frames at line ~5938, unsolicited PING ACK at ~6043) pass the value to JS onError but the resulting destroy() writes its GOAWAY via goaway() which reads last_peer_stream_id — so the wire is correct even if those args are stale. highest_started_stream_id() and get_next_stream_id() correctly keep using the mixed mark.
Test quality: the new tests follow harness conventions (raw sockets, waitFor on concrete frame predicates, error handlers wired, port: 0, cleanup in finally). Six of seven are stated to fail on USE_SYSTEM_BUN=1, and node-http2.test.js was rerun.

…e overall high-water mark The push scenarios now deliver the peer-initiated id after a locally initiated stream with a higher id, which is the case the mixed mark gets wrong; the describe comment no longer presents bun's rejection-budget GOAWAYs as node behaviour.

robobun · 2026-08-12T01:25:13Z

^{Updated 7:17 AM PT - Oct 10th, 2026}

✅ @robobun, your commit 2109f5f5ad297542bcea23f398483f5c1097135a passed in Build #124306! 🎉

🧪 To try this PR locally:

bunx bun-pr 37588

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

bun-37588 --bun

…ter, NodeHTTPResponse, and three JS binding objects (oven-sh#42682) ### Problem - Some native members exist only for built-in JS to call, and no built-in JS calls them. The largest is `H2FrameParser.setStreamPriority` (`src/runtime/api/bun/h2_frame_parser.rs`). `Http2Stream#priority()` is a no-op since RFC 9113. - Three binding objects (`node:vm`, `node:crypto`, `bun:sql`) carry properties that their only JS consumer never reads. - `packages/bun-usockets` keeps a commented-out `bsd_udp_packet_buffer_ecn` from 2024-04. Nothing reads `completions/spec.yaml`, a 2021 appspec file. ### Fix - Delete each item: 15 files, 535 lines removed. The Notes list every symbol. - Correct because every removed name has zero references in `src/`, `packages/`, `scripts/`, `test/` and `build/debug/codegen/` outside its own definition. No removed member is documented or typed API. User code reaches the h2 parser handle only through the undocumented `Symbol.for("::bunhttp2native::")` key. - The `-D dead-code` lint then reported three more items in `h2_frame_parser.rs`, and two `Stream` fields became write-only. They are removed too. - Verified: `bun bd` and `bun run rust:check-all` (12 targets) pass. The http2, shell, vm, crypto, `node:http`, sql, udp and streams test files pass (list in the Notes). ### Background - A `*.classes.ts` file declares the methods of a native class. The generator emits the C++ wrapper and the Rust glue from it. A `proto` entry that no JS reads is dead, and so is its Rust function. - A binding object is a plain object that native code fills and one built-in module destructures once. A property that the module does not destructure is never read. - 28 other dead-code pull requests are open. This one deletes nothing that they delete. oven-sh#40240 and oven-sh#37588 edit the body of `set_stream_priority`. Each conflict resolves by taking the deletion. <details><summary>Notes</summary> Removed, one line each: - `H2FrameParser.setStreamPriority` / `set_stream_priority` (109 lines). No `.setStreamPriority(` in `src/js` or `test/`. - `H2FrameParser.isStreamAborted` / `is_stream_aborted`. Same check. - `H2FrameParser.hasNativeRead` / `has_native_read`. Same check. - `FrameType::HTTP_FRAME_PRIORITY`, `ErrorCode::PROTOCOL_ERROR`, `SignalRef::is_aborted`. Reported by the dead-code lint once the three functions above were gone. Both enums already list only the wire values that the file uses. - `Stream::stream_dependency`, `Stream::exclusive`. Their only reads were in `set_stream_priority`. The declarations, the initializers and the two stores in `request()` go. The locals that `request()` writes to the wire stay, and so does `Stream::weight` (read by `getStreamState`). - `ShellInterpreter.isRunning` / `is_running` and `.started` / `get_started`. `src/js/builtins/shell.ts` only calls `interp.run()`. - `Interpreter::started`. The field was only read by `get_started`. The two stores and the `AtomicBool` import go with it. - `NodeHTTPResponse.dumpRequestBody` / `dump_request_body`. Added in oven-sh#17093, never called from JS at any commit. - `createNodeVMBinding`: `kUnlinked`, `kLinking`, `kEvaluating`, `kSourceText`, `kSynthetic`. `src/js/node/vm.ts` destructures `kLinked`, `kEvaluated`, `kErrored` and never touches the binding object again. - `createNodeCryptoBinding`: `SecretKeyObject`, `PublicKeyObject`, `PrivateKeyObject`. Not destructured in `src/js/node/crypto.ts`. The classes stay, only the unread properties go. - `bun_sql_jsc::mysql::create_binding`: `MySQLConnection`. `bun_sql_jsc::postgres::create_binding`: `PostgresSQLConnection`. `src/js/internal/sql/{mysql,postgres}.ts` destructure `createConnection`, `createQuery`, `init` only. - `packages/bun-usockets`: the commented-out `bsd_udp_packet_buffer_ecn` (`bsd.c`), its commented-out wrapper `us_udp_packet_buffer_ecn` (`udp.c`), and the two commented-out declarations (`libusockets.h`, `internal/networking/bsd.h`). `git blame`: 589f941, 2024-04-26. The `libusockets.h` lines are context lines of a hunk in oven-sh#40294. - `completions/spec.yaml`. No hit for `spec.yaml` or `appspec` in the repo. It still lists the removed `bun dev` subcommand. The shell completions are hand-maintained and embedded with `include_bytes!`. Tests run with the debug build: `test/js/node/http2/` (7 files, `node-http2.test.js` 387 pass, `h2-conformance.test.ts` 70 pass), the four `test-http2-*priority*` Node tests, `test/js/bun/shell/` (4 files), `test/js/node/vm/vm.test.ts`, `crypto.key-objects.test.ts`, five `test/js/node/http/` files, five `test/js/sql/` files, `udp_socket.test.ts`, `streams.test.js`. How the candidates were found: - A repo-wide identifier index (definitions with zero other mentions, with and without comments). - A relink of the debug binary with `--gc-sections --print-gc-sections`. Almost every real hit from that pass is already deleted by one of the open pull requests. The rest were inlined functions, `const fn`s used only at compile time, and Windows or macOS paths. - A per-class comparison of every `*.classes.ts` member against `src/js`, `packages/bun-types` and `test/`. The generated thunk for a `proto` entry is `#[no_mangle]` and `#[allow(dead_code)]`, so neither rustc nor the linker can flag these members. - Checks for `.rs` files outside every `mod` tree, headers that nothing includes, C/C++ files outside the build, Cargo features that nothing enables, and long commented-out blocks. All clean apart from the usockets block. Found, not deleted here: - `TCPSocket`/`TLSSocket` `endBuffered` (`$end`): no JS caller, but its removal leaves `write_or_end_buffered::<IS_END>` with one instantiation. That is a refactor, not a deletion. - `*InternalReadableStreamSource.isClosed` getter: no reader, but its removal leaves `NewSource::is_closed` write-only, and the stores are in files that oven-sh#41088 touches. - `NodeHTTPResponse.onwritable`: no reader in `src/js` today, but oven-sh#41822 starts to use it. - The HEADERS+PRIORITY emitter and the native `SignalRef` abort path in `H2FrameParser::request()`. `http2.ts` warns with DEP0194 for the priority options and handles `options.signal` itself. Whether these native paths still run needs a separate look. - The `IPV6_RECVTCLASS` / `IP_RECVTOS` `setsockopt` calls in `packages/bun-usockets/src/bsd.c` ("used for getting the ECN"). Nothing reads the ECN now, but their removal changes socket options, so it is not a pure deletion. - `macro(mockedFunction)` and `macro(writer)` in `src/js/builtins/BunBuiltinNames.h` (the builtin-name entries, not the live `mockedFunction` string in `BunCommonStrings.h`): no `mockedFunctionPrivateName` or `writerPrivateName` user, but two open pull requests edit adjacent lines. - `scripts/packer/build-image.pkr.hcl`, `scripts/lldb-inline.sh`, `scripts/lldb-inline-tool.cpp`, `scripts/github-metrics.ts`, `scripts/gamble.ts`: nothing references them, but they are standalone tools that a person can run by hand. Self-reviewed: 12 concerns raised, 10 addressed (the two `Stream` fields, the header comment lines, and the body corrections above). Rejected: a split into three pull requests, because every deletion is verified the same way and a split triples the CI and review rounds. Deferred: a caller lint for `*.classes.ts` `proto` entries, as a follow-up that does not gate this change. </details>

After this side sent a GOAWAY, a stream that it does not open no longer moves the mark: a request that the server refuses, and a stream that a PUSH_PROMISE promises to a client. The id in a later GOAWAY then cannot exceed the id in an earlier one (RFC 9113 6.8). send_go_away takes Option<u32> and reads the mark itself. Only session.goaway(code, lastStreamID) passes an id. The test for the server's budget GOAWAY now uses a malformed request after two pushes, so that the server's own id 4 is above the request on 3.

…the GOAWAY mark A match with no wildcard arm: a new kind of refusal has to say whether the stream counts as processed.

RFC 9113 6.8: an endpoint must not increase the last stream id that it sends. Both GOAWAY writers now take the id from H2FrameParser::next_goaway_last_stream_id. It lowers the id to the id of the last GOAWAY that the session wrote, as nghttp2 does with local_last_stream_id.

…n open nghttp2_submit_goaway refuses such an id and node ignores the error, so session.goaway(code, id) sends nothing. Without this rule, the id clamp puts the sender's own stream id in every later GOAWAY. Add a test for the GOAWAY of the rstStream refusal budget and a test for the GOAWAY of the pending SETTINGS limit.

The mark is not the highest stream id that the peer used: a stream that is refused or promised after a GOAWAY of ours does not raise it. The new name keeps it apart from a highest-received id (nghttp2's last_recv_stream_id). The engine field is private, so only note_processed_stream can raise it.

claude

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

…t GOAWAY id in an Option node reads lastStreamID with ToInt32, so 2**31 and Infinity mean the last processed stream and 2**32+2 is the id 2. The saturating conversion made every such value 2147483647, which a client took for an id of its own. The tests now write three GOAWAYs for the clamp, end the refused-after- GOAWAY case with a connection error, cover a request with a lower id, and reject a wait when the stream or session closes first.

claude

I reviewed the latest push (ToInt32 coercion and the own-parity no-op in goaway(), plus the Option<u32> rework of send_go_away) and found no bugs; since this changes what node:http2 puts on the wire in GOAWAY and makes session.goaway(code, id) a silent no-op for an id of the sender's own parity, a maintainer look at that behavioral choice is still worthwhile.

What was reviewed:

goaway() in h2_frame_parser.rs: is_number() is checked before coerce_to_i32, so no user JS runs between coercion and send_go_away; the parity gate (is_multiple_of(2) == is_server) matches "peer-initiated only" for both roles.
Monotonic clamp: both the engine path (Sink::clamp_goaway_last_stream_id) and every embedder sender go through next_goaway_last_stream_id, so no GOAWAY writer bypasses it.
note_processed_stream runs before on_stream_open/header delivery in handle_headers and handle_push_promise, so a synchronous goaway()/destroy() from JS already sees the advanced mark.
Tests use port: 0, bounded waitFor/waitClosed deadlines rather than sleeps, and assert exact last-stream-id values per GOAWAY.

Extended reasoning...

The diff touches the H2 engine (src/runtime/api/bun/h2/connection.rs), its embedder (src/runtime/api/bun/h2_frame_parser.rs), and adds 17 raw-socket tests plus 7 engine unit tests for GOAWAY Last-Stream-ID semantics. It touches no auth, crypto, or injection surface, but it changes wire-visible protocol behavior for every node:http2 session and makes session.goaway(code, id) silently send nothing for a same-parity id. The bug hunt ran dry and my own read of the coercion ordering, the clamp coverage, and the mark-before-surface ordering found nothing, but the size (~950 lines) and the user-facing behavioral choice mean a human maintainer should weigh it rather than this review approving alone.

…ay-last-peer-stream-id # Conflicts: # test/js/node/http2/h2-conformance.test.ts

…GOAWAY tests

h2-conformance.test.ts holds cases that fail now and then on a debug build (the stream-reset floods, the cases that count live stream objects). The new file has only the 20 GOAWAY cases. It uses node:test and node:assert, so `node --test` runs it as it is.

claude

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

robobun · 2026-10-21T17:38:48Z

A note for this branch. CI now refuses a branch that does not contain main commit b7efd52 (#46227). The pipeline step prints "This branch is too far behind main to build. Merge main into it." and stops. This branch is behind that commit, so its next build will stop there too.

#43475 is stacked on this PR and hit that check. Its base is now robobun/d5f1ee93/h2-goaway-base, which is this branch merged with main at 9634520. The merge has no conflicts. When this branch has main merged, #43475 can use it as its base again.

One more point from the work on #43475. It changes the Refused arm of the processed match to true without a condition. With the gate of #43475, a request that the server refuses for maxSessionMemory after a GOAWAY of its own can only arrive in the read that sends the GOAWAY. Measured on node v26.3.0 over a duplexPair: a 'ping' listener calls goaway(0, 3), HEADERS(3) follows in the same write, and the session is over its memory limit. node sends RST_STREAM(3, ENHANCE_YOUR_CALM), state.lastProcStreamID is 3, and the next GOAWAY names 3.

robobun assigned cirospaciari Aug 11, 2026

github-actions Bot added the claude label Aug 11, 2026

claude Bot reviewed Aug 11, 2026

View reviewed changes

robobun mentioned this pull request Aug 11, 2026

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

Open