Repository navigation
Conversation
…_PROMISE left A pushStream() whose header block fails native validation releases the reserved stream id through setStreamContext(id, undefined). That only dropped the sctx root. The entry in H2FrameParser.streams stayed open with its own js_context still set to the destroyed JS stream, so a later client frame on that id (RST_STREAM, WINDOW_UPDATE with a zero increment) reached the streamError handler through the entry's own context and lowered the session's open-stream count a second time. With one request stream open the count hit zero, and session.close() destroyed the session at once. The release now also marks the entry CLOSED and runs free_resources, which detaches the context and queues the eviction of the entry. A later frame on the id finds no JS stream to report to.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for one more day.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for one more day. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 8 minutes for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 2:50 PM PT - Sep 19th, 2026
❌ @robobun, your commit 7a7da10 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 43553That installs a local version of the PR into your bun-43553 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked whether the new free_resources call in set_stream_context can re-enter JS while the raw &mut Stream is live — it only queues the id, drops the sctx/context roots, cleans the outbound queue and drops the signal, so no user code runs under the borrow. Also looked at the new test.each block for timing sensitivity: it awaits PING/ACK and the stream/session close events rather than sleeping, so the ordering it asserts is event-driven.
Extended reasoning...
Findings were reported inline, so this body only records what else was examined. The free_resources::<false> path (h2_frame_parser.rs:1932) pushes to pending_engine_stream_closes, removes the sctx entry, calls detach_context, clean_queue, and drops the abort signal; none of those enter JS synchronously, so the diagnostics-channel re-entrancy concern raised during the hunt does not apply. The conformance test synchronizes on PING/ACK and close events with no fixed sleeps. A human should still weigh the two inline findings (HPACK encoder divergence after a mid-encode validation failure, and the client-side request() sibling that still leaks its native entry).
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/api/bun/h2_frame_parser.rs— pre-existing: a real HTTP/2 client gets corrupted or undecodable headers on later responses after a pushStream() whose header block fails native validation. push_promise validates and encodes header by header, so ":path" is already added to the HPACK encoder's dynamic table at h2_frame_parser.rs:6130 when "bad header" throws at h2_frame_parser.rs:6075; the block never reaches the wire, so the peer's decoder table lacks that entry. Fix: validate every name and value before the first encode_header_into_list call, or discard the partial encode on throw, which covers the same interleaving in request()/respond() at h2_frame_parser.rs:6616. This PR handles the stream left behind by that throw but leaves the encoder desynced.Extended reasoning...
The new release path in set_stream_context (h2_frame_parser.rs:6310) is only reached after push_promise threw mid-encode, so the cleanup this PR adds runs on a session whose HPACK encoder state already diverges from the peer.
The JS pushStream at src/js/node/http2.ts:3228 spreads the user headers first, then appends :method/:scheme/:authority, so property order is ":path", "bad header", ... .
push_promise pass 0 iterates every key: ":path" passes to_valid_header_name, is a pseudo-header, and encode_header_into_list runs at h2_frame_parser.rs:6130. lshpack_wrapper_encode (src/jsc/bindings/c-bindings.cpp:476) leaves indexed_type 0, which is lshpack's "add" mode, so ":path: /p" is pushed into the encoder dynamic table.
The next key "bad header" fails to_valid_header_name and throws INVALID_HTTP_TOKEN at h2_frame_parser.rs:6075. encoded_headers is a local Vec and is dropped; nothing is written to the socket. The encoder's dynamic table keeps the entry.
The peer's decoder never saw that entry. Every later header block the server emits that references a dynamic-table…
Verification: pre-existing. Trigger: a
pushStream()on a node:http2 server whose header block fails native validation after at least one pseudo-header with a non-static-table value was already HPACK-encoded, followed by any later header block on the same session that repeats that (name, value) pair (e.g. a second push of the same:path, or — for an invalid-value failure, which happens in pass 1 after…
|
CI: 180 of 181 jobs pass. The one red job (debian 13 x64-asan) fails in test/js/bun/spawn/spawn.test.ts, which also fails on main and does not touch this diff. The http2 suites pass on every lane. |
Fixes #43509
Problem
node:http2server, apushStream()whose header block fails native validation (for example the name"bad header",ERR_INVALID_HTTP_TOKEN) reserves a stream id that never reaches the wire. A later client RST_STREAM or WINDOW_UPDATE with a zero increment on that id makessession.close()destroy the session at once, while a request stream is still open. The client gets an emptyDATA(END_STREAM)and the socket closes.set_stream_context(id, undefined)(src/runtime/api/bun/h2_frame_parser.rs:6304) removed only thesctxroot. The entry inH2FrameParser.streamsstayed open with its ownjs_contextset to the destroyed JS stream.on_stream_resetfell back to that context, and thestreamErrorhandler insrc/js/node/http2.tsran#connections--a second time. The count reached 0 with stream 1 open.Fix
CLOSEDand runsfree_resourceson it, which detaches the JS context, drops thesctxroot, and queues the eviction of the entry.undefined, which they ignore.end_stream,abort_stream,on_stream_reset).test/js/node/http2/h2-conformance.test.ts, describepush reserved but never announced(three triggers, two fail on main). Also the fulltest/js/node/http2/directory and node's sixtest-http2-*push*tests.Background
H2FrameParserkeeps one nativeStreamentry per stream id instreams, and a separatesctxmap that roots the JS stream object for the engine's dispatches.rewrite_stream_ctxreadssctxfirst and falls back to the entry's ownjs_context.free_resourcesis the native close routine: it detaches both roots and pushes the id topending_engine_stream_closes. The next inbound read batch at dispatch depth 0 evicts the engine entry and frees the native slot.ServerHttp2Sessioncounts open streams in#connections.close()sends GOAWAY and destroys the session when that count is 0.Notes
ERR_INVALID_HTTP_TOKENsynchronously frompushStream()and reserves no id, so it never has this state. Bun reports the error through the callback, which is an existing, separate difference.on_stream_resetpath. The test keeps that case covered.close()takes the settings-ACK grace period and the early destroy is hidden.none,rstandwu0all printmain.destroyed=false session.destroyed=falseafterclose(), thenmain close | session close.Follow-ups found in review, pre-existing and out of scope here:
request()catch (src/js/node/http2.ts:6254) runs#connections--but never releases the native entry thatgetNextStream()created. One nativeStreamper failed request stays instreamsfor the life of the session.push_promise(andrequest()/respond()) encode header by header. A name that fails validation after an earlier pair was already added to the HPACK encoder's dynamic table leaves the encoder ahead of the peer's decoder. A later block that references that entry by index is undecodable on the client.