Repository navigation
Conversation
The client streamStart handler ran for every stream id the native parser registered. That includes a stream the peer opens with HEADERS that this side neither opened nor reserved with PUSH_PROMISE. For an even id the handler built a ClientHttp2Stream that user code is never handed, so session teardown raised its 'error' as an uncaughtException. For every id it counted the stream in #connections, so close() waited forever on a stream nothing can close. Remove the handler. request() counts a stream where it takes the stream id, and streamPush already creates and counts a real pushed stream.
|
Updated 1:51 AM PT - Sep 12th, 2026
✅ @robobun, your commit 608bf8cbb4c60840b4ee1aec2968d259d1f8cee1 passed in 🧪 To try this PR locally: bunx bun-pr 42369That installs a local version of the PR into your bun-42369 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. WalkthroughThe client HTTP/2 session now counts only allocated, user-visible streams. Conformance tests cover unsolicited streams, malformed headers, GOAWAY handling, and graceful session closure. ChangesHTTP/2 client stream accounting
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The HTTP/2 stream-accounting change has no unresolved merge-blocking risk. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status at 608bf8c (fa0d813 plus a merge of main and one more test) Reproduced on 1.4.3-canary.1+4ff919377 and on main 6b394bf (Linux x64) with the script in the PR description. Bun prints New tests:
CI at fa0d813: the diff is green. Build 114476 passes 180 of 181 jobs. The one red test is PR: #42369 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The #connections bookkeeping move looks correct end-to-end, but since it touches the client session's graceful-close gate across several lifecycle paths, a human look from someone familiar with the h2 parser dispatch would still be worthwhile.
What was reviewed
- Native side tolerates a missing
streamStarthandler:handler_pair!skips storing it andhandle_received_stream_idearly-returns at theonStreamStart.get()guard, sogetNextStream()on the client no longer touches JS. - Counter balance: increments now at
request()post-getNextStream(),#flushPendingRequests(), andstreamPush; every decrement site (aborted,streamError,streamEndstate 7, both catch blocks) either already guards ontypeof stream !== "object"or is paired with a JS-side increment. TheconnectionsCounted = truemove to after the id check matches the new increment point. - Tests: the subprocess fixtures assert an exact sorted event list (positive check — an
uncaughtExceptionwould appear in stdout and fail the match); the30_000timeout matches five other tests in the same file.
Extended reasoning...
Overview
The PR removes ClientHttp2Session's streamStart handler and relocates its #connections++ into the two JS call sites that allocate a client stream id (request() and #flushPendingRequests()). Previously the native parser invoked streamStart for any newly-registered stream id — including one opened by hostile inbound HEADERS on a stream the client never opened or reserved — which (a) inflated the graceful-close counter with nothing to decrement it, hanging session.close(), and (b) constructed an orphaned ClientHttp2Stream that later received the session teardown error with no user listener, raising uncaughtException. Four new tests in test/js/node/http2/h2-conformance.test.ts cover both failure modes against Node v26.3.0's observed behavior.
Security risks
None identified. The change narrows what the client tracks in response to peer-initiated frames (the peer could previously inflate #connections and hang close(); now it cannot). No new parsing, no auth/crypto/permission surface, no user-controlled input reaching a new sink.
Level of scrutiny
Medium-high. The source change is small and mechanical, but #connections gates graceful close in roughly fifteen places across the client session (goaway handling, settings-ack grace, ping drain, close(), streamEnd, aborted, streamError). I traced each: every decrement site is either behind a typeof stream !== "object" guard (so a stream with no JS context — the hostile case — never decrements) or is paired with a JS-side increment in the same function. The native handler_pair! macro (h2_frame_parser.rs:757) uses get_truthy and simply skips absent handlers; handle_received_stream_id early-returns when onStreamStart.get() is None, so removing the JS handler is safe. The connectionsCounted flag move in request() correctly follows the increment to its new location.
Other factors
The tests follow file conventions (subprocess -e fixtures with Promise.all drain and a combined-object assertion, RawH2Server/once() for the in-process cases, describe.concurrent, port: 0 via the existing helper). The 30_000 per-test timeout matches five existing tests in the same file, so it's the local convention rather than a new deviation from test/CLAUDE.md. The subprocess tests assert an exact sorted event list on stdout — a positive check where an uncaughtException would appear and fail the match — so they don't rely solely on the weak stderr: not.stringContaining assertion. No CODEOWNERS entry covers these paths. Deferring rather than approving because this is bot-authored connection-lifecycle bookkeeping in a protocol-heavy Node compat module, and the PR notes its own self-review workflow had not completed; a maintainer who knows the h2 dispatch paths should confirm no other client-side path relied on streamStart running.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…elled the request
|
#43534 changes the engine side of this. On a client, I ran the five tests that this PR adds on the #43534 branch, without the
|
This branch has not been deployed
2 participants
Problem
uncaughtExceptionon thehttp2.connect()client:error: Protocol error,code: "ERR_HTTP2_ERROR"(orERR_HTTP2_GOAWAY_SESSION). Every stream that user code holds has an'error'listener. Bun exits 1, node v26.3.0 exits 0.session.close()wait forever. The second case needs no hostile server: a response that was in flight when the client cancelled the request is enough.streamStarthandler (src/js/node/http2.ts:4972). The native parser calls it for every stream id it registers, also one that the peer opens with HEADERS. The handler counted that stream in#connections, the count of open streams thatclose()waits on. For an even id it built aClientHttp2Streamthat user code never receives. Teardown destroys it with the session error.Fix
request()counts a stream where it takes the id.streamPushalready creates and counts a real push.streamPush. Every client handler already returns early for a stream with no object.test/js/node/http2/h2-conformance.test.ts(5 new tests, all fail on 1.4.3). Alsotest/js/node/http2/and the 261test-http2-*node tests.Background
streamStartis the native parser's callback for a new stream id (handle_received_stream_id,h2_frame_parser.rs). On a client that is arequest(), or inbound HEADERS on a stream that the parser does not know.Notes
Repro (from the report,
bun repro.js):session error ERR_HTTP2_ERROR | session close | UNCAUGHT ERR_HTTP2_ERROR | request error ERR_HTTP2_ERROR | request closerequest error ERR_HTTP2_ERROR | request close | session error ERR_HTTP2_ERROR | session closesession error ERR_HTTP2_ERROR | session close | request error ERR_HTTP2_ERROR | request closeThe two paths to the uncaught error. A connection error or
destroy(err)runsemitErrorToAllStreams. ThestreamErrorhandler then reachesemitStreamErrorNTandHttp2Stream._destroy, which takes the error fromsession[kSessionDestroyError]. A GOAWAY with an error code runsforEachStream(rejectStreamAboveGoawayLastId), which callsstream.destroy(err)withERR_HTTP2_GOAWAY_SESSION. Both reached the object thatstreamStartbuilt. Neither reaches a stream that has no object:for_each_streamskips it, and every client handler returns early when its stream argument is not an object.Why a real push never reached the removed branch. The parser's inbound engine (
src/runtime/api/bun/h2/connection.rs) reserves the promised id inhandle_push_promiseand does not callon_stream_open. When the block completes,on_headers_completedispatchesstreamPush, which builds theClientHttp2Stream, counts it and stores it withsetStreamContext. The later response HEADERS find the reserved stream, sois_newis false andon_stream_opendoes not run.handle_received_stream_id(the only caller ofstreamStart) runs on a client fromgetNextStream()(odd ids) and fromon_stream_open, whichhandle_headerscalls only for a stream the engine does not know: never promised, or closed and evicted.close()hang. Before this change (1.4.3): after HEADERS on stream 2 below a promised stream 4, or HEADERS on a finished stream 1 in a later read,client.close()never emits'close'. Node finishes both. nghttp2 treats both streams as closed and ignores the frame (session_on_request_headers_receivedreturnsNGHTTP2_ERR_IGN_HEADER_BLOCK).Tests. Two child-process tests cover the
uncaughtException. Three in-process tests coverclose(): an even stream below a promised one, HEADERS on a request stream that finished, and response HEADERS that arrive afterreq.close(NGHTTP2_CANCEL). The two child-process tests print the events without error codes. Node raises its connection error at the HEADERS frame itself, so the codes of the GOAWAY case differ from bun until the engine rejects the frame (#36343). The event set is the same in node and bun. The twoclose()tests use streams that are not idle, so a later engine-level idle check does not change them.Other suites on the debug build.
test/js/third_party/grpc-js/test-server,test-end-to-end,test-idle-timer,test-retry,test-deadline,test-server-errorspass.test-client.test.tshas 3 failures that are identical with main'shttp2.ts(a 100 ms connect deadline under debug+ASAN).Source. Found by frame-mutation fuzzing of a raw h2 server against
http2.connect(). There is no user report.Not in this PR (same result on 1.4.3 and on this branch, node differs):
handle_headersinsrc/runtime/api/bun/h2/connection.rs). Each frame leaves one native entry until the session ends. The frame also raises the id that the nextrequest()takes, and one frame with a very high id makes later requests fail withERR_HTTP2_OUT_OF_STREAMS. Node answers an idle id with a connection error. node:http2: reject the RFC 9113/7541 client-side protocol violations node does #36343 and node:http2: treat frames on idle streams as connection errors (RFC 9113 5.1) #32987 (idle ids), h2 server: decode and discard HEADERS on a closed client stream instead of resetting or re-opening it #37985 (closed ids on a server) and node:http2: keep the next local stream id independent of the peer's streams #37676 (next request id) work on that layer. All are open.ERR_HTTP2_ERROR"Stream was already closed or invalid". Node ignores that block. The new test uses the later-read timing only.pushed.close(NGHTTP2_CANCEL)keeps its count, so a latersession.close()does not finish. node:http2: fix rejecting a server push with close(NGHTTP2_REFUSED_STREAM) #33000 covers that. node:http2: reject a PUSH_PROMISE whose promised id does not exceed the previous one #37563 covers a PUSH_PROMISE whose id does not exceed the previous one.'error'or'close'(node destroys it). That is a separate bug with a separate fix.[human-review] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 2 rejected · iteration 2
evidence per changed file