Repository navigation
Conversation
|
Status Reproduced on bun 1.4.3-canary.1+367d939d9 against node v26.3.0 with a raw TCP HTTP/2 server.
Bun wrote DATA on a reserved pushed stream (review request): the same server sends PUSH_PROMISE(1 -> 2) and then DATA on stream 2 before its HEADERS. Node answers Fail-before: CI: build 119472 passed 180 of 181 jobs, and every |
|
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 (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. WalkthroughHTTP/2 reserved-stream DATA now causes a connection-level ChangesHTTP/2 protocol and reset handling
Suggested reviewers: Priority: ➖ Normal 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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 found no bugs, but since it changes which RST_STREAM frames reach the wire based on cross-layer (native/JS) timing, a human look at the flag-ordering choices would still be worthwhile.
What was reviewed:
- Traced every native
onStreamError/onAborteddispatch site insrc/runtime/api/bun/h2_frame_parser.rs(end_stream,abort_stream,on_stream_reset,emit_error_to_all_streams,emit_abort_to_all_streams, therequest()validation exits): each setsCLOSEDfirst and writes the RST_STREAM (when one is due) after the dispatch, soNativeClosedis set only when nothing is left to send. - Checked the
_destroyguard relaxation (rstCode !== 0 ||dropped) againstdestroyStreamForSessionDestroyandemitStreamErrorNT: asession.destroy()with no error in the same turn as a peer reset now defers that stream's destroy to'end', butemitStreamErrorNTstill destroys it on the next tick. - Confirmed a synchronous
destroy(err)/close(code)from an'aborted'listener runs_destroybefore the flag is set, and that the newrstNextTickearly-return catches that case atsetImmediatetime; the RST written by the native side precedes it. - Test file:
memoryPairdestroy recursion terminates (seconddestroy()is a no-op on an already-destroyed Duplex), each new test asserts an exact frame list ([code]vs[code, code]/[]), and resources are released infinally.
Extended reasoning...
Overview
The production change is ~30 lines in src/js/node/http2.ts: rstNextTick is rebound to the stream (session passed explicitly) and returns early when StreamState.NativeClosed is set; the four native aborted/streamError handlers (server and client) now set NativeClosed; and _destroy's deferred-reset guard drops the rstCode !== 0 || clause so any natively-closed stream skips the host call. All five rstNextTick.bind call sites are updated consistently. The test file adds an in-memory Duplex pair, generalizes RawH2/RawH2Server to accept any Duplex, and adds a describe block with nine tests that count RST_STREAM frames after a stream's 'close', one setImmediate, and a PING round trip.
Security risks
None specific to this change. The code path only decides whether to emit an additional RST_STREAM frame for a stream that is already closed at the native layer; it does not parse untrusted input or change resource limits. One positive side effect: a peer's RST_STREAM(REFUSED_STREAM) no longer feeds the server's maxSessionRejectedStreams budget via the JS-side echo, which removes a way for a client to trip GOAWAY(ENHANCE_YOUR_CALM) on itself.
Level of scrutiny
Medium. The diff is small but the correctness argument depends on ordering between native dispatches, setImmediate callbacks, process.nextTick(emitStreamErrorNT), and user code run from 'aborted'. I verified the native dispatch sites and the JS consumers of NativeClosed (rstNextTick, _destroy, destroyStreamForSessionDestroy) and found the reasoning holds, but the choice to set the flag after emit("aborted") and the interaction with two open PRs touching the same lines (rstNextTick first-wins guard, pushed-stream teardown) are judgment calls a maintainer familiar with the http2 layer should confirm. No debug build was available in this environment, so I did not execute the new tests.
Other factors
The PR description claims the ten new tests fail on the unfixed build and pass with the fix, and that the full test/js/node/http2/ directory plus 279 vendored node http2 tests pass; the test assertions are exact frame lists, so the before/after distinction is real rather than vacuous. No CODEOWNERS entry covers the changed files. The bug hunt exited on a dry streak with no findings, and the five candidate issues it examined were either pre-existing behavior (client GOAWAY sweep, close() from an 'aborted' listener on an open writable) or tick-level timing differences that resolve on the next tick.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 6:02 PM PT - Sep 21st, 2026
❌ @robobun, your commit 1123ac7 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 43539That installs a local version of the PR into your bun-43539 --bun |
|
Two changes came out of the review.
The three code comments that were longer than one line are one line each now. |
|
A second run on #43510 reached the same fix: the streamError and aborted handlers mark the stream native-closed and _destroy skips the deferred rstStream for it. That branch reuses NativeClosed and adds one conformance test: https://github.com/oven-sh/bun/tree/robobun/063e1f88/http2-single-rst-after-native-reset. This PR covers it, so no second PR. |
|
@robobun On DATA for a pushed stream that is still reserved, Node v26.3.0 treats it as a connection error (deps/nghttp2/lib/nghttp2_session.c:5262): it sends GOAWAY(PROTOCOL_ERROR) and the pushed stream reports rstCode 2. The test "a client resets a pushed stream once for DATA ahead of the response HEADERS" expects one RST_STREAM(STREAM_CLOSED) and a connection that survives, so it fails on Node. Node is the stricter side here, so please match Node so the test passes on Node and Bun. |
After the native layer closes a stream (its own reset, the peer's RST_STREAM, or both END_STREAM flags), Http2Stream#_destroy and close() still submitted a deferred rstStream. Native drops that call only while the stream's table entry exists. A client's pushed stream has no entry and any other stream loses its entry on the next read, so the call wrote a second RST_STREAM, or answered the peer's RST_STREAM with one. On a server a peer's RST_STREAM(REFUSED_STREAM) also counted against maxSessionRejectedStreams. The streamError and aborted handlers now mark the stream NativeClosed, _destroy skips the deferred reset for such a stream whatever its rstCode, and rstNextTick checks the mark again when it runs.
NativeClosed is also read by destroyStreamForSessionDestroy, which lets a cleanly closed stream with buffered data finish before it is destroyed. A request() that fails native validation keeps its table entry, so a session.destroy() in the same turn took that branch for it once the streamError handler set NativeClosed. The streamError and aborted handlers now set NativeReset, and set it before 'aborted' is emitted. _destroy and rstNextTick skip the reset for either bit. NativeClosed keeps its meaning.
…error
RFC 9113 5.1 makes any frame other than HEADERS, RST_STREAM or PRIORITY
on a reserved (remote) stream a connection error of type PROTOCOL_ERROR,
and nghttp2 ends the session for it ("DATA: stream in reserved"). The
engine answered with RST_STREAM(STREAM_CLOSED) and kept the connection.
It now sends GOAWAY(PROTOCOL_ERROR), for a whole DATA frame and for one
that arrives incomplete.
6bdbe60 to
71c7679
Compare
|
@cirospaciari Done in 71c7679. I also rebased the branch onto main.
Two results in that scenario still differ from node. Both exist on main for every engine connection error, so the tests do not assert them.
The tests use TCP for this case. Over a JS Duplex transport the engine's GOAWAY does not reach the peer at all, also on main. |
There was a problem hiding this comment.
Beyond the inline findings, two more things were checked and ruled out: the server-side twin of the new GOAWAY arm (a client sending DATA on a stream the server reserved with PUSH_PROMISE, State::ReservedLocal) still answers with RST_STREAM(STREAM_CLOSED) via the can_receive_data branch at src/runtime/api/bun/h2/connection.rs:1525 — that is pre-existing behavior this PR does not change, so it is not a regression here. And the _destroy guard change (dropping the rstCode !== 0 exception for a NativeClosed stream) matches node's if (!this.closed) rule: a stream that reached state 7 never gets a RST_STREAM in node either, while half-closed streams still do here since NativeClosed is not set for them.
Extended reasoning...
The push since the last review adds a Rust change in src/runtime/api/bun/h2/connection.rs (DATA on a ReservedRemote stream becomes GOAWAY(PROTOCOL_ERROR) on both the streamed and whole-frame paths) and a NativeReset bit in src/js/node/http2.ts that suppresses the deferred RST_STREAM from close()/destroy() once native has already closed or reset the stream. No security-sensitive surface beyond protocol-error handling of peer frames. The inline findings (a pushed stream left without 'close'/'error' after the new GOAWAY, and WINDOW_UPDATE on a reserved stream not being rejected) already signal that a human look is needed; the note records the server-side ReservedLocal sibling and the _destroy guard semantics as examined and not regressed.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/api/bun/h2/connection.rs— pre-existing, nit: a client still accepts WINDOW_UPDATE on a promised stream that is reserved (remote), where node/nghttp2 end the connection with GOAWAY(PROTOCOL_ERROR). This PR applies the RFC 9113 §5.1 reserved-remote rule only to DATA (connection.rs:1383, connection.rs:1524); handle_window_update at connection.rs:869 has no State::ReservedRemote check and just grows the window. Fix: apply the same reserved-remote connection error to every frame type §5.1 lists (WINDOW_UPDATE, both the zero-increment arm at connection.rs:852 and the normal arm at connection.rs:869), ideally through one shared predicate the DATA paths also use. nghttp2's reason text is "WINDOW_UPDATE to reserved stream".Why this was flagged
A server sends PUSH_PROMISE reserving stream 2, so the client engine inserts an entry with State::ReservedRemote at src/runtime/api/bun/h2/connection.rs:1738. The server then sends WINDOW_UPDATE on stream 2 before the pushed HEADERS. handle_window_update at src/runtime/api/bun/h2/connection.rs:869 finds the entry, increases send_window, and calls sink.on_window_update; the session continues. RFC 9113 §5.1 says any frame other than HEADERS, RST_STREAM or PRIORITY on a reserved (remote) stream is a connection error of type PROTOCOL_ERROR, and nghttp2's session_on_stream_window_update_received returns a connection error "WINDOW_UPDATE to reserved stream", which node surfaces as a GOAWAY and session error. The base branch behaves the same as this PR here, so this is pre-existing; it is the sibling arm of the rule this PR adds for DATA with DATA_ON_RESERVED_STREAM at connection.rs:1383 and connection.rs:1524. No safeguard covers it: the only state check in handle_window_update is the send_window overflow at connection.rs:871.
Verification: pre-existing (nit). Triggering condition: a server sends WINDOW_UPDATE on a promised stream after PUSH_PROMISE and before the pushed HEADERS. Mechanism verified:
handle_push_promiseinserts the promised stream withentry.state = State::ReservedRemote(src/runtime/api/bun/h2/connection.rs:1738);handle_window_updateat connection.rs:869-878 does `else if let Some(s) =… | pre-existing (nit).…
A client session's teardown does not reach a pushed stream, so after the GOAWAY for DATA on a reserved (remote) stream the pushed stream never closed. The engine now reports that stream to the embedder before the connection error. It closes with rstCode 2, as in node.
|
On the two review findings for 71c7679:
|
There was a problem hiding this comment.
I re-reviewed after the latest push and found no new bugs; the newest commit addresses the pushed-stream teardown gap I flagged earlier, and because this changes HTTP/2 connection-error semantics across the native engine and the JS stream lifecycle, a maintainer look is still worthwhile.
What was reviewed:
data_on_reserved_streamin connection.rs: it now reports the promised stream viasink.on_stream_resetbefore GOAWAY; traced through the embedder, that dispatchesonStreamErrorto JS without writing an RST_STREAM frame, so the test's "no RST_STREAM" assertion is consistent with the code.- The
NativeResetbit and therstNextTickrebinding: all four.bind(session, ...)call sites were updated to.bind(this, session, ...), and both client and serveraborted/streamErrorhandlers set the bit before_destroycan read it. - The
Some(st)borrow inbegin_streamed_dataends at theReservedRemotecomparison, mirroring the existingcan_receive_dataarm, so the&mut selfcall after it is fine. - I could not run the debug build here, so the test assertions in
h2-conformance.test.tswere checked by reading, not by execution.
Extended reasoning...
The PR touches src/js/node/http2.ts (a new StreamState.NativeReset bit, rstNextTick rebound to the stream with an early return when native already closed or reset it) and src/runtime/api/bun/h2/connection.rs (DATA on a ReservedRemote stream becomes a connection PROTOCOL_ERROR via a new data_on_reserved_stream helper wired into both the whole-frame and streamed-data paths), plus conformance tests with an in-memory Duplex socket pair. It touches no auth, crypto, or injection surface; it changes wire-level HTTP/2 error handling on peer-controlled input. The latest commit resolves the inline finding from my previous run, and no CODEOWNER covers these files. I chose defer over approve because the change alters protocol-level teardown behavior across two layers, a human maintainer commented mid-review with content I cannot see, and I was unable to execute the debug build to run the tests in this environment.
This branch has not been deployed
2 participants
Problem
node:http2client writesRST_STREAMtwice when its inbound engine resets a server-pushed stream (node v26.3.0: once). It also answers the peer'sRST_STREAM. Other streams do both when a read precedes the deferred reset.Http2Stream#_destroy(src/js/node/http2.ts:2685) defersrstStream(id, rstCode)even after the native layer closed the stream. The nativerst_stream(src/runtime/api/bun/h2_frame_parser.rs:5056) drops the call only while the stream has a stream table entry.RST_STREAM(STREAM_CLOSED). RFC 9113 §5.1 makes it a connection error. Node sendsGOAWAY(PROTOCOL_ERROR).Fix
streamErrorandabortedhandlers set a newStreamState.NativeResetbit. Native dispatches them only after it closed the stream._destroyand the callback that submits the reset skip a stream with that bit orNativeClosed, as node does (if (!this.closed)).src/runtime/api/bun/h2/connection.rs) answers DATA on a reserved (remote) stream withGOAWAY(PROTOCOL_ERROR).test/js/node/http2/h2-conformance.test.tsandh2-push-refusal-staged.test.ts(12 tests fail without the fix). Alsotest/js/node/http2/and 279 vendored node tests.rst_stream(Notes).Background
H2FrameParserhas two layers. The inbound engine (src/runtime/api/bun/h2/) parses frames and raises stream and connection errors. The older stream table serves outbound calls such asrstStream.rstStreamcannot tell it from an unknown stream.Notes
Wire output, bun 1.4.3-canary.1+367d939d9 against node v26.3.0. Raw TCP server unless marked. "mem" is an in-memory Duplex transport where two frames are two reads in the same turn.
RST(2)=1RST(2)=1twiceRST(2)=1GOAWAY(PROTOCOL_ERROR)RST(2)=5twiceGOAWAY(PROTOCOL_ERROR)RST(2)=2on a pushed streamRST(2)=2RST(2)=8on a pushed streamRST(2)=8RST(1)=1RST(1)=1twiceRST(1)=1req.close(8)on an open POST, one read after the firstRST_STREAM(mem)RST(1)=8RST(1)=8twiceRST(1)=8stream.close(2)on an open POST while the peer sends PING frames (TCP)RST(1)=2RST(1)=2twiceRST(1)=2'aborted'listener callsclose(8), one more read (mem)RST(1)=8twicedestroy(err)RST(1)=2RST(1)=8maxSessionRejectedStreams: 1, peer sendsRST(1)=7, then PINGGOAWAY(ENHANCE_YOUR_CALM)DATA on a reserved (remote) stream. RFC 9113 §5.1: "Receiving any type of frame other than HEADERS, RST_STREAM, or PRIORITY on a stream in this state MUST be treated as a connection error of type PROTOCOL_ERROR." nghttp2 ends the session with the reason "DATA: stream in reserved" (
session_on_data_received_fail_fast), and node reportsERR_HTTP2_ERRORwithrstCode2 on every stream. The engine now does the same inhandle_dataand inbegin_streamed_data, the path for a DATA frame whose payload is not complete yet. Two tests pinned the old stream error: the push-state test inh2-conformance.test.ts, which now has a row for a whole frame and a row for the head of a frame, and its staged twin inh2-push-refusal-staged.test.ts. They assert what node does and bun now does: the first GOAWAY carries PROTOCOL_ERROR, no RST_STREAM goes out, the session error isERR_HTTP2_ERROR, the request and the pushed stream close withrstCode2, and the payload never reaches the pushed stream. Only the reserved (remote) state changes. #43494 covers DATA from a client on a server-pushed stream.A client session's teardown does not reach a pushed stream (#42410 adds that). So before the GOAWAY the engine reports the promised stream to the embedder with
on_stream_reset(id, INTERNAL_ERROR), and the pushed stream closes withrstCode2. With #42410 merged those two engine lines can go.Three differences from node remain in that scenario, and the tests do not assert them. The pushed stream is destroyed before the session error exists, so its error is
ERR_HTTP2_STREAM_ERRORwhere node reportsERR_HTTP2_ERROR. After an engine connection error bun writes a secondGOAWAY(INTERNAL_ERROR)fromsession.destroy(err). Over a JS Duplex transport the engine's GOAWAY does not reach the peer at all.Why the native side cannot decide this.
rst_streamfinds a closed stream through its table entry (end_streamreturns early onCLOSED). Without an entry it writes the frame for any nonzero code. That branch exists so that a client can refuse or cancel a pushed stream, which is never in the table. An evicted entry and a pushed stream look the same there. Every native site that dispatchesonStreamErrororonAbortedsets the stream toCLOSEDfirst and has already written theRST_STREAMwhen one was due (end_stream, the abort signal path,on_stream_reset,emit_error_to_all_streams,emit_abort_to_all_streams, and therequest()validation exits, which never reach the wire). So the dispatch is the point where JS learns that nothing is left to send.Why the
setImmediatecallback (rstNextTick) checks again.close()queues its reset before the native side has closed the stream. Two cases reach the callback with the bit set: aclose()from an'aborted'listener, and the reset that_destroyqueues afterclose(code). In the second case the first reset goes through nativeend_stream, which writes the frame and dispatchesstreamError, so the handler sets the bit before the second callback runs.Why a new bit and not
NativeClosed.NativeClosedmeans that state 7 (both END_STREAM flags) arrived.destroyStreamForSessionDestroyreads it to let a cleanly closed stream with buffered data finish before the destroy. Arequest()that fails native validation dispatchesstreamErrorand keeps its table entry, so asession.destroy()in the same turn would take that branch for it. WithNativeResetno reader ofNativeClosedchanges. The handlers set the bit before'aborted'is emitted, so adestroy()from that listener already sees it.Tests. The nine tests of the new block fail on the unfixed build, release and debug, with the frame counts from the table. They wait for the stream's
'close', then for onesetImmediate(the deferred reset was queued before'close'), then for a PING round trip, and only then count frames.RawH2andRawH2Servercan now run overmemoryPair(), two linked Duplex streams, to make "one more read before the deferred reset" deterministic.Suites run on the debug (ASAN) build.
h2-conformance.test.ts80 pass, and the new block in 25 more runs.test/js/node/http2/589 pass, 0 fail. All 279test/js/node/test/{parallel,sequential}/test-http2-*.jsandtest-diagnostics-channel-http2-*.js. grpc-js:test-deadline,test-server-deadlines,test-server-errors,test-call-propagation,test-retry,test-idle-timer,test-server,test-server-interceptors.test-clientfails on the debug build at its 100 mswaitForReadydeadline (a connect does not fit in 100 ms there) and passes on a release build.Self-review. Five concerns, four addressed.
_destroyqueues afterclose(code)had no test. The tenth test pins it.destroyStreamForSessionDestroyalso readsNativeClosed, and a review comment showed one path where a shared flag changes it. The reset now has its own bit,NativeReset.setImmediateafter'close'first.rstNextTick. This change gives it the same signature, so the two guards merge as one line each.rst_streamin place of the JS flag. Native has no record that tells an evicted entry from a client's pushed stream, and the branch must keep writing for the second.Seen and not changed here. WINDOW_UPDATE on a reserved (remote) stream is accepted, where node v26.3.0 writes
RST_STREAM(1)=2andGOAWAYcode 2.pushed.close(code)on a client fails withInvalid stream idand never emits'close'(the bug #33000 addressed before it went stale). A peer cancel of a pushed stream still emits'aborted'.destroy(err)on a cleanly closed stream reportsrstCode2 where node keeps 0 (#43433 changes that line).Nearby open PRs. #33380 adds a first-wins guard to
rstNextTickfor a reset thatclose()and_destroyboth submit. #42410 tears a client's pushed streams down with the session. Both touch the same lines as this change and neither covers a stream that the native layer closed.[human-review] gate passed · iteration 1 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 1
evidence per changed file