Repository navigation
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughThe HTTP/2 implementation now handles oversized headers, closed streams, missing-stream resets, and deferred frame-error session closure. Conformance tests cover server and client reset behavior, event ordering, graceful shutdown, and existing-stream continuity. ChangesHTTP/2 frame-error handling
Suggested reviewers: Priority: ⬇️ Low 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:49 PM PT - Sep 21st, 2026
❌ @robobun, your commit 0bbd87f has 1 failures in
🧪 To try this PR locally: bunx bun-pr 43523That installs a local version of the PR into your bun-43523 --bun |
|
Status: ready for review. The diff is green. How I reproduced it:
Fix: this PR (#43523). The 8 new tests in CI, build 119462 on 0bbd87f: 180 of 181 jobs passed, and no |
![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.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/api/bun/h2_frame_parser.rs— pre-existing: a server that sets maxSendHeaderBlockLength still puts trailer and PUSH_PROMISE header blocks over that limit on the wire, so the cap this PR enforces for respond() is bypassed there. Only request() at h2_frame_parser.rs:7103 compares encoded_size with max_send_header_block_length. send_trailers (h2_frame_parser.rs:5781) and push_promise write the block unchecked, while nghttp2 applies the limit in session_prep_frame to every HEADERS and PUSH_PROMISE. Fix: run the same check plus onFrameError and end_stream(FRAME_SIZE_ERROR) for every outbound header block (request, trailers, push promise), which covers the 3 sites.Extended reasoning...
The PR is titled as resetting the stream when a response header block is over maxSendHeaderBlockLength, and its new tests only cover respond() and additionalHeaders(). The option is read once at h2_frame_parser.rs:7637-7643 into max_send_header_block_length. A repo grep shows that field is consumed at exactly one place, h2_frame_parser.rs:7103-7104 inside request(). send_trailers() computes encoded_size at h2_frame_parser.rs:5781 and goes straight to frame writing at 5794-5842; its only frameError site (5671) fires on an HPACK encode failure, not on the configured limit. push_promise() (6025 onward) writes its block at 6225-6265 with no comparison either. In nghttp2, session_prep_frame returns NGHTTP2_ERR_FRAME_SIZE_ERROR when the estimated block exceeds max_send_header_block_length for both NGHTTP2_HEADERS (including trailers) and NGHTTP2_PUSH_PROMISE, and node then emits 'frameError', resets the stream and closes the session. Trigger: http2.createServer({ maxSendHeaderBlockLength: 200 }), respond({':status':200},{waitForTrailers:true}), then sendTrailers({'x-big': 'b'.repeat(1000)})…
Verification: pre-existing. Triggering condition: a server (or client, for trailers) created with
maxSendHeaderBlockLengthcallsstream.sendTrailers()orstream.pushStream()with a header block whose HPACK-encoded size exceeds the limit. Mechanism verified.max_send_header_block_lengthis consumed at exactly one site in /home/claude/bun/src/runtime/api/bun/h2_frame_parser.rs: lines 7103-7104 inside… -
🟣
src/runtime/api/bun/h2_frame_parser.rs— Clients of a Bun server still hang with no 'response', 'error' or 'close' whenever the server closes a stream locally for any reason other than the header-size cap. The maxSessionMemory exit at h2_frame_parser.rs:7069 and the options/parent/weight exits at 6914, 6991 and 7033 still set CLOSED and write no RST_STREAM. Fix: every locally-closing exit of request() on a server must go through end_stream so a RST_STREAM reaches the peer, not only the 7103 branch.Extended reasoning...
The PR's fix is one of five sibling exits in request() that share the same shape: set stream.state = CLOSED, set rst_code, dispatch onStreamError, return. At 7069 is_over_session_memory_limit() takes this path. Its default limit is 10 MB of session memory, so it fires for any busy connection with many buffered responses, a far larger population than the opt-in maxSendHeaderBlockLength cap this PR fixes. The peer's stream stays open forever on the client side, exactly the hang the PR title promises to remove. On the base every one of these exits hangs the client. After the PR the header-size exit resets the stream and the others still do not. REVIEW.md treats same-class sites as one concern; the PR names #43478 and #43419 as owners, but those are not in this checkout and an intentional exclusion leaves a production hang that the same three-line change (end_stream(FRAME_SIZE_ERROR or ENHANCE_YOUR_CALM)) would close. Remedy: route each server-side local close in request() through end_stream with the matching code, or extract a helper used by all five sites.
Verification: pre-existing, acknowledged in diff: the PR description names the sibling exits as "owned by other PRs: #43478 (request options passed to respond()) and #43419 (maxSessionMemory)" — the bound is accurate: none of these exits is touched by this diff and the base commit fails by the same route. Trigger: a server stream's respond()/additionalHeaders() reaches request() while the session is over…
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Reply to the two findings outside the diff:
The two inline findings that this diff caused are fixed in 6e41cb1 (the second RST_STREAM, and the client stream entry that stayed in the map). |
There was a problem hiding this comment.
I re-reviewed the follow-up commits and found no bugs; because this changes native stream lifecycle (free_resources/end_stream from inside request()) and the on-wire behavior of node:http2 on both server and client, a human look would still be worthwhile.
What was reviewed:
- The new server
end_streamand clientfree_resourcescalls inrequest(): they run under theGuardedStreamdispatch guard withensure_still_alive()on the identifier, matching the existingrst_stream/send_trailersshapes, so the deferred map eviction cannot run mid-call. - The second-RST_STREAM concern raised last round:
rst_stream()now only hand-writes for client pushed ids, andend_streamearly-returns on CLOSED entries, so the deferred_destroyreset is a no-op on both branches. - The
setImmediate(session.close)added to bothframeErrorhandlers is idempotent against the trailer path, which already sends GOAWAY and destroys the session synchronously viaonEnd. - The HPACK encoder-state gap after a refused block remains as documented in the description (deferred to #41520); the new tests send no later header block, so they do not depend on it.
Extended reasoning...
Overview
The PR touches src/runtime/api/bun/h2_frame_parser.rs (the maxSendHeaderBlockLength branch in request(), new CLOSED-state early returns in request()/send_trailers()/no_trailers(), and a narrowing of the unknown-id branch in rst_stream()), src/js/node/http2.ts (both frameError handlers schedule a graceful session.close() one setImmediate later), and test/js/node/http2/h2-conformance.test.ts (seven new raw-frame tests). The change converts a "stream silently closed locally, peer hangs" outcome into node's observable RST_STREAM(FRAME_SIZE_ERROR) + GOAWAY(NO_ERROR) sequence on the server, and a local REFUSED_STREAM failure plus graceful close on the client.
Security risks
No new parsing of peer-controlled bytes is introduced; the changed paths are driven by locally-produced header blocks. The memory-safety-relevant part is the new free_resources::<false> and end_stream calls while holding a &mut Stream derived from this.streams: both happen inside enter_stream_dispatch, which arms the dispatch depth so the only eviction path (rewrite_read's pending-close drain) is deferred, and the identifier JSValue is pinned with ensure_still_alive() before the Strong context is detached. This is the same shape the existing rst_stream (h2_frame_parser.rs:6442-6448) and trailer-size path (5670-5679) already use. The rst_stream() unknown-id branch now writes fewer frames, not more, which removes a way to emit RST_STREAM for an id the peer never saw.
Level of scrutiny
Medium-high. It is a small diff, but it changes stream lifecycle in unsafe Rust and alters wire behavior for both server and client sessions in a Node-compat module. I did not find defects, and the earlier concerns (the second RST_STREAM after eviction, the client stream entry retained until session death with a justifying comment) are addressed in the code now: is_pushed_to_client gates the hand-written RST, end_stream early-returns on CLOSED, and the client branch frees resources immediately. The remaining known divergences from node (reset timing within the same tick, RST_STREAM(NO_ERROR) for endStream: true, HPACK table drift deferred to #41520) are stated in the description rather than hidden. No debug build was available in this checkout, so I did not execute the new tests myself; the test structure (framing-aware waitFor, PING barriers to order frames, every failure event wired to the awaited promise, bounded keepSending loops) follows the file's existing conventions.
Other factors
No CODEOWNERS entry covers the changed files. The timeline shows prior inline threads resolved by the author with follow-up commits that do correspond to the code changes I verified, so I treated them as addressed by the code rather than by the resolution metadata. The setImmediate(closeSessionAfterFrameError) addition applies to every onFrameError dispatch, including the existing trailer-size path; I checked that close() returns early when the session is already destroyed, so that path does not emit a second GOAWAY.
|
Cross-link: #43440 fixes the same hang and was opened a few hours before this PR. Merge only one of the two. They conflict in The comparison of the two PRs and the two possible landing orders are in #43440 (comment). |
|
@robobun On Node v26.3.0 the PING ACK is written inside the read callback (src/node_http2.cc:981) while |
…SendHeaderBlockLength A server stream whose respond() or additionalHeaders() block is over maxSendHeaderBlockLength was closed locally with no frame on the wire, so the peer waited forever. Write RST_STREAM(FRAME_SIZE_ERROR) for it and close the session gracefully, as node's onFrameError does. A client request never reached the wire, so it still fails locally with REFUSED_STREAM. JS learns of a native close one tick later. request() on a server, send_trailers() and no_trailers() no longer write frames on a stream that the native side has already closed.
…ide released rst_stream() writes the frame itself for an id that is not in the stream map. That exists for a client's pushed streams, which never get an entry. Every other id had one, so a missing entry means the stream is already closed. The stream's _destroy still asks for a reset one setImmediate later, and once a read had released the entry that request became a second RST_STREAM on a server, or a RST_STREAM for an idle id on a client. With that, a client request refused for maxSendHeaderBlockLength releases its entry at once.
…nder node's timing node writes a PING ACK inside the read callback and resets the stream one setImmediate after the frame error, so a PING ACK does not mark the point where the reset has arrived. The server cases now wait for the RST_STREAM frame. The additionalHeaders() case no longer calls respond() in the same tick: node still sends that respond(), and Bun resets the stream at once. A new case covers that sequence with a real client and asserts what both agree on: the connection stays usable and the open request completes.
59f2a21 to
0bbd87f
Compare
|
@cirospaciari Confirmed on Node v26.3.0, and fixed in 0bbd87f. I ran the new Changes:
Result: all 8 cases pass on Node v26.3.0 and on the debug build, and all 8 fail with main's |
There was a problem hiding this comment.
I reviewed the five follow-up commits and found no bugs; the two concerns from my earlier pass (a second RST_STREAM after the entry was evicted, and the refused client request pinning its Stream/Strong context until session death) are both addressed in the code now. Because this changes stream lifecycle in the native h2 parser and relies on node/nghttp2 wire semantics I could not run node against here, a human look from someone who owns node:http2 would still be worthwhile.
What was reviewed:
rst_stream()unknown-id gating:on_stream_openalways registers server streams and clients onlyrequest()with a fresh id, so the only entry-less ids left are client-side pushed streams — theis_pushed_to_clientnarrowing is sound.- New
frameError->setImmediate(session.close)alongside the existing trailer-size path (h2_frame_parser.rs ~5683) that already sends a native GOAWAY:send_go_away(emit_error=true)dispatchesonEnd, which destroys the session, soclose()is a no-op there — no duplicate GOAWAY. - Client branch ordering: identifier captured and
ensure_still_alivebeforefree_resourcesdrops the Strong,dispatch_deptharmed viaGuardedStreamacross the JS dispatch; mirrors the existingend_streamshape. - Server-only CLOSED guard in
request()runs before HPACK so a dropped block no longer desyncs the encoder table; the client is exempt since it only ever passes a new id.
Extended reasoning...
The change touches src/runtime/api/bun/h2_frame_parser.rs (over-limit header block path now resets on the wire for servers, frees the refused client stream, gates the raw RST_STREAM fallback to pushed streams, and adds CLOSED early-returns in request/send_trailers/no_trailers), src/js/node/http2.ts (frameError now schedules session.close()), and adds seven conformance tests. No auth, crypto, or injection surface; it is protocol-lifecycle code with raw *mut Stream borrows and JS re-entry. Deferring rather than approving because the correctness claims rest on measured node v26.3.0 wire behavior I could not reproduce in this sandbox, the native stream-lifecycle changes carry UAF risk if a caller shape was missed, and a maintainer (cirospaciari) commented before the latest commits with content I cannot see.
This branch has not been deployed
2 participants
Problem
node:http2server setsmaxSendHeaderBlockLength, thenrespond()oradditionalHeaders()sends a larger header block. Bun closes the stream locally and writes no frame, so the client request hangs. Node v26.3.0 sendsRST_STREAM(FRAME_SIZE_ERROR), thenGOAWAY(NO_ERROR).maxSendHeaderBlockLengthcheck inrequest()(src/runtime/api/bun/h2_frame_parser.rs:7082). It marks the streamCLOSEDwithREFUSED_STREAMand returns.Fix
end_stream(FRAME_SIZE_ERROR), which writes the RST_STREAM. TheframeErrorhandlers callsession.close()onesetImmediatelater, like node'sonFrameError. A client request never reached the wire, so it fails locally withREFUSED_STREAM, as in nghttp2.request()on a server,send_trailers()andno_trailers()wrote frames on the closed stream. They now return early.rst_stream()writes a frame for an id with no map entry only for a client's pushed stream. Before,_destroy's later reset request became a second RST_STREAM once a read released the entry.test/js/node/http2/h2-conformance.test.ts(8 new tests, all fail on main). Alsotest/js/node/http2/, node'stest-http2-*.jsand grpc-js.Background
maxSendHeaderBlockLengthcaps a header block that this side sends. nghttp2 fails a larger HEADERS frame, and node'sonFrameErrorcallsstream.close(code)andsession.close().RST_STREAMends one stream.GOAWAY(NO_ERROR)stops new streams, and open streams can finish.Notes
Repro. On main the client prints nothing and hangs. Node and this PR print
error: Stream closed with error code NGHTTP2_FRAME_SIZE_ERROR.Wire output. A raw h2 client opens stream 1. The server sends its response HEADERS and holds the body back. The client then opens stream 3, which the server answers over the limit.
respond(),end()RST_STREAM(3, 6),GOAWAY(0)RST_STREAM(3, 6),GOAWAY(0)res.setHeader(),res.end()DATA(3, END_STREAM)with no HEADERSrespond({ waitForTrailers }),end(),sendTrailers()HEADERS(3)with no response HEADERSadditionalHeaders(),respond(),end()HEADERS(3), then nothingHEADERS(3)+DATA(3),GOAWAY(0)respond()RST_STREAM(push, 6),GOAWAY(0)With this PR and with node, stream 1 still delivers its body after the GOAWAY, and the connection then ends. I also ran the patched server against a node client and a Bun client. Both report
NGHTTP2_FRAME_SIZE_ERRORon the failed request and complete the open one.The second reset.
_destroyasks the native side to reset the stream onesetImmediateafter the stream closes. While the stream's map entry exists, that call does nothing.rst_stream()also writes the frame itself for an id that has no entry. That path exists for a client's pushed streams, which never get an entry. Supposerespond()ran from asetImmediatecallback and the peer sent any frame in between. The read released the entry, and a secondRST_STREAM(6)went out. With the first commit of this PR alone, the new "one RST_STREAM" test fails with two resets. For a client the same call would name an id that the peer has never seen, which is a connection error. The refused client request kept its entry for that reason. It now releases the entry at once, and the new raw-server test covers that case.Differences from node that remain.
setImmediateafter the frame error. In node, arespond()in the same tick as a failedadditionalHeaders()still goes out. A laterrespond()finds the stream reset. Here the stream is reset at once in both cases, and the same-tickrespond()writes nothing. On main thatrespond()would send a block with HPACK indexes the peer does not have (see "HPACK"), and the peer then ends the connection withCOMPRESSION_ERROR. A new test covers this sequence with a real client. It asserts what both runtimes agree on: no session error, the open request completes,GOAWAY(NO_ERROR).respond(headers, { endStream: true })node sendsRST_STREAM(NO_ERROR), because its writable side had already closed the stream. Here the code isFRAME_SIZE_ERROR.'error'listenersession.closedis stillfalsehere. The session closes onesetImmediatelater.HPACK. On main the check runs after the block went through the HPACK encoder. After a refused block the encoder table is ahead of the peer. A later header block on the same session can reference an entry the peer does not have. #41520 fixes that, and this PR does not change it. Before this PR the session stayed open, so every later stream was exposed. Now only the streams that are already open are. The new tests send no header block after the refused one, so they pass with or without #41520.
Client side. The outcome for the request is unchanged:
'frameError', thenERR_HTTP2_STREAM_ERRORwithNGHTTP2_REFUSED_STREAM, which node'stest-http2-options-max-headers-block-length.jsexpects. The new part is the gracefulsession.close(). Before, the session stayed open. The next request on it failed withSession closed with error code 9because of the HPACK state above.Sibling exits of
request(). The other exits that close a server stream locally are owned by other PRs: #43478 (request options passed torespond()) and #43419 (maxSessionMemory). Their diffs apply on top of this branch with no conflict. Therequest()guard is limited to servers because a client only callsrequest()with the id of a new stream.Not covered here.
pushStream()call in the same tick as the failedrespond()is not guarded.maxSendHeaderBlockLengthis still not applied to trailers or topushStream()headers. Node applies it to every HEADERS and PUSH_PROMISE frame.Node cross-check. The eight new tests also pass on Node v26.3.0. I ran the file under
node --testwith a small shim that mapsbun:testtonode:test. The server cases wait for the RST_STREAM frame itself. Node acknowledges a PING inside the read callback and resets the stream onesetImmediatelater, so a PING ACK is not a marker for the reset.Review changes.
request()guard is limited to servers.rst_stream()no longer writes the second reset.Suites run with the debug build:
test/js/node/http2/(587 pass, 6 skip)test/js/node/test/parallel/test-http2-*.js(256) andtest/js/node/test/sequential/test-http2-*.js(5)test/js/third_party/grpc-js/node:http2(309 pass)One test in
node-http2-streams-rehash.test.tstakes 4.7 to 5.0 s on a debug build. It can pass the 5 s limit in a full-directory run, and it passes alone. In the grpc-js suite the DNS and tonic tests need the network. All of them fail the same way without this change.[human-review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 1
evidence per changed file