Repository navigation
node:http2: close a pushed stream whose END_STREAM HEADERS and CONTINUATION arrive in separate reads - #43586
node:http2: close a pushed stream whose END_STREAM HEADERS and CONTINUATION arrive in separate reads#43586robobun wants to merge 5 commits into
Conversation
A pushed stream's response HEADERS with END_STREAM moved the entry to Closed when the frame arrived, before its CONTINUATION. The end-of-read eviction then removed the Closed entry, so finish_header_block could not find it and never reported on_stream_end. The pushed stream never emitted 'close' and the session never finished closing. handle_headers now applies only RecvHeaders. finish_header_block applies RecvEndStream next to on_stream_end, the same way finish_streamed_data does for a streamed DATA frame. The end states are unchanged: RecvHeaders followed by RecvEndStream reaches the state that RecvHeadersEndStream reached, and both classify errors the same way.
|
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 (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughThe HTTP/2 connection now defers ChangesHTTP/2 header completion
Suggested reviewers: Priority: ➖ Normal Severity of issue fixed: Medium 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 6:39 PM PT - Sep 19th, 2026
✅ @robobun, your commit d9e093fd8be25392a14982413f3828c2c79034b3 passed in 🧪 To try this PR locally: bunx bun-pr 43586That installs a local version of the PR into your bun-43586 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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 finding, I also checked that RecvHeaders followed by RecvEndStream in stream.rs lands on the same state as the removed RecvHeadersEndStream from every start state (Idle, ReservedRemote, Open, HalfClosedLocal) and raises the same error class from HalfClosedRemote/Closed, so the state on_headers_complete/on_stream_end observe is unchanged. The ignored Err in the new finish_header_block block is only reachable if the stream was locally reset between the HEADERS and CONTINUATION reads, in which case the state stays Closed and eviction still runs at the end of the read.
Extended reasoning...
Findings were reported inline, so this is the brief ruled-out note only. The Rust diff is small: handle_headers now always applies RecvHeaders, and finish_header_block applies RecvEndStream after the block completes, mirroring the existing DATA-path sites at connection.rs:1441 and :1572. I traced the state machine in src/runtime/api/bun/h2/stream.rs for every start state and confirmed the composed transition is equivalent to the removed single-step event, including error classification, so no other caller of the sink sees a different state. The only new behavior is that a stream sits in Open/HalfClosedLocal (instead of HalfClosedRemote/Closed) between reads while a CONTINUATION is pending; since RFC 9113 §4.3 forbids any other peer frame in that window, the only way the deferred transition can fail is a local reset, which leaves the entry Closed and evicted as before. The PR is not approved because verified findings remain unposted and a variant-matrix gap was flagged inline.
There was a problem hiding this comment.
LGTM — the END_STREAM transition now lands with the completed header block, and the latest commit covers the split-trailers variant raised last round.
What was reviewed:
- Walked every start state in stream.rs: RecvHeaders followed by RecvEndStream reaches the same state and classifies errors identically to the removed RecvHeadersEndStream, so the state passed to on_headers_complete/on_stream_end is unchanged.
- Checked the early-return paths in finish_header_block (Refused, StreamClosed, malformed, rejected): each sets Closed explicitly, so dropping the frame-time END_STREAM does not leave a stream stuck open there.
- Checked that reading end_state before on_headers_complete is safe: the engine RefCell is mutably borrowed across receive(), so JS dispatched from the sink cannot alter engine stream state mid-block; only CONTINUATION frames are dispatched while a block is parked, and replenish_windows branches solely on Closed.
Extended reasoning...
Overview
The production change is small: handle_headers in /home/claude/bun/src/runtime/api/bun/h2/connection.rs now applies only RecvHeaders when a HEADERS frame arrives, and finish_header_block applies RecvEndStream once the block is complete, immediately before on_headers_complete/on_stream_end. This mirrors the shape finish_streamed_data already uses. A Rust unit test and two integration tests in /home/claude/bun/test/js/node/http2/h2-conformance.test.ts (initial response block and trailer block on a pushed stream, each split across reads at the SETTINGS ACK boundary) exercise the fix; the trailer variant was added in the most recent commit in response to the previous inline nit.
Security risks
This is peer-facing frame handling, so the main question is whether the deferral loosens any check. It does not: RecvHeaders errors on HalfClosedRemote and Closed exactly as RecvHeadersEndStream did, so the connection-level STREAM_CLOSED escalation and the stream-level refusal path are unchanged. While a block is parked, dispatch rejects any frame other than a CONTINUATION on the same stream with GOAWAY, so a peer cannot interleave DATA or RST to exploit the intermediate Open/HalfClosedLocal state. Bounds and HPACK handling are untouched.
Level of scrutiny
Moderate. The diff is ~15 lines of engine code, but it is a state-machine relocation in a protocol implementation, so I verified equivalence from stream.rs for every start state rather than trusting the PR description, and confirmed the early-return paths in finish_header_block (Refused, StreamClosed disposition, malformed, rejected) all set Closed explicitly so no stream is left open by the removed frame-time transition. I also checked that reading end_state before the on_headers_complete dispatch is equivalent to the previous read-after ordering: rewrite_read in h2_frame_parser.rs holds the engine RefCell mutably across receive(), so JS callbacks cannot mutate engine stream state mid-block, and close_stream from the pending-close queue runs only between reads (leaving map to return None, the same outcome as before). The silently ignored Err from the deferred RecvEndStream is unreachable from the two states a successful RecvHeaders can leave.
Other factors
No CODEOWNERS entry covers the changed files. The bug hunt exited on a dry streak with no findings. The new JS tests follow the existing conventions of the file (no-op error handlers, RawH2Server.waitFor with its bounded timeout) and assert exact event sequences. The Rust unit test mirrors the neighbouring push_promise_roundtrip_server_to_client test; I could not compile it locally (no build artifacts, and the author notes the lib test target does not link), so its correctness rests on reading it against the CaptureSink helpers, which match. The open related PR (#43575) touching the same tail of finish_header_block is a rebase concern, not a correctness one.
This branch has not been deployed
1 participant
Problem
node:http2client, a pushed stream never emits'close'when its response HEADERS carries END_STREAM but not END_HEADERS, and the CONTINUATION arrives in a later read.session.close()never completes. node v26.3.0 closes both. Fixes node:http2 client: a pushed stream never closes when its END_STREAM HEADERS and the CONTINUATION arrive in different reads #43493.handle_headers(src/runtime/api/bun/h2/connection.rs:945) appliedRecvHeadersEndStreamwhen the HEADERS frame arrived. A pushed stream went from reserved (remote) toClosedat once.receive()ends every read withreplenish_windows, which evicts everyClosedentry. When the CONTINUATION completed the block,finish_header_blockfound no entry and never calledon_stream_end.Fix
handle_headersapplies onlyRecvHeaders.finish_header_blockappliesRecvEndStreamwhen the block is complete, next toon_stream_end. This is the shapefinish_streamed_dataalready uses for a streamed DATA frame.stream.rs,RecvHeadersfollowed byRecvEndStreamreaches the same state asRecvHeadersEndStreamfrom every start state, and both classify errors the same way. The state thaton_headers_completeandon_stream_endsee is the same as before.test/js/node/http2/h2-conformance.test.ts(one new test, times out on 1.4.3) and a Rust unit test next to the existing push test. Also the rest oftest/js/node/http2/.Background
header_block_in_flightuntil END_HEADERS. No other frame may arrive in between.replenish_windowsruns at the end of eachreceive(). It sends WINDOW_UPDATE frames and drops every stream entry inClosedso the map stays bounded.on_stream_endis the sink callback that tells the JS layer a stream's receive side ended. The JS layer closes the stream and decrements the session's open-stream count from it.Notes
Closedthrough END_STREAM on HEADERS. A request or response stream reaches half-closed (remote), which the sweep does not evict. Split trailers on a pushed stream hit the same path.bun_runtimelib test target does not link locally (undefined C++ symbols), and CI does not run it. Each assertion in it was checked against the code by hand.node-http2.test.jshas a few tests that time out at 5 s when the whole file runs on the debug ASAN build (the DATA payload detach cases, the goaway cases with a bun server). Each of them passes when run alone, at 3 to 4 s. They do not touch the inbound header path.finish_header_block, so one of the two needs a small rebase.["push 200","end"]and node prints["push 200","end","close","session close"]. With this change bun prints the node output.[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
root cause · written by the author bot
The client applied the END_STREAM state transition in handle_headers as soon as the HEADERS frame arrived, so a pushed stream moved to Closed before its header block was complete, and the read-ending replenish_windows pass evicted the Closed entry while the block was still parked awaiting CONTINUATION. When the CONTINUATION later completed the block, finish_header_block could not find the stream entry, so on_stream_end never fired, JS never received 'close', and the session's open-stream count never dropped. The fix keeps the stream in RecvHeaders when the frame arrives and applies RecvEndS…