Repository navigation
Conversation
An interim (1xx) header block is not the response, so a stream cannot end on it. The client engine now treats a 1xx HEADERS frame that carries END_STREAM as a malformed block: it resets the stream with PROTOCOL_ERROR and does not deliver the block. nghttp2 applies the same rule (nghttp2_http_on_remote_end_stream with EXPECT_FINAL_RESPONSE set).
|
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 19 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 4:53 PM PT - Sep 19th, 2026
✅ @robobun, your commit d3bdb07decd86a8b1171fbe71cc3f3f928c0d062 passed in 🧪 To try this PR locally: bunx bun-pr 43575That installs a local version of the PR into your bun-43575 --bun |
|
Status: reproduced and fixed in this PR (#43575). How I reproduced it: a raw TCP HTTP/2 server answers the request on stream 1 with one HEADERS frame,
The new tests in DATA that arrives before the final HEADERS is the other half of the same report. #43558 covers it. CI (build #118594): 179 of 181 jobs passed, 2 are still queued, and no job failed. Four tests failed once and passed on a retry, all outside this diff: |
![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 two interactions of the new informational && end_stream check in finish_header_block (src/runtime/api/bun/h2/connection.rs:1279): on the server side an inbound :status already sets malformed via wrong_direction, so request handling is unchanged; and informational is only computed for Deliver blocks, so the Refused/StreamClosed early returns and the rejected ENHANCE_YOUR_CALM path keep their existing precedence over the new PROTOCOL_ERROR reset.
Extended reasoning...
The diff is a four-line control-flow addition plus a conformance test block. I traced the informational flag from its assignment at connection.rs:1180 (inside the disposition == Deliver branch of the decode loop) to the new check at :1279 and to the existing malformed && !rejected / rejected branches. Server-side inbound :status is already marked malformed by wrong_direction at :1157, so the change is effectively client-only. Header-list-size rejection (rejected) still wins at :1303 because the malformed branch is gated on !rejected. The Bun-to-Bun respond({':status': 100}, {endStream: true}) case was investigated in a prior pass and is consistent with node's behaviour. The three confirmed inline findings (DATA-after-1xx gap, invalid-frame budget parity, double RST_STREAM on pushed streams) mean a human should still weigh in on scope before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/runtime/api/bun/h2/connection.rs— A long-lived client session to a server that repeatedly answers with a 1xx block plus END_STREAM is torn down with GOAWAY ENHANCE_YOUR_CALM after 100 such responses, failing every other in-flight stream on that session; node keeps the session alive until 1000 invalid frames. The new malformed route at connection.rs:1279 reaches sink.on_stream_rejected at connection.rs:1300, and h2_frame_parser.rs:4153-4165 counts it against maxSessionRejectedStreams (default 100) and sends GOAWAY at the limit. Fix: on the client, a malformed response block should count only against maxSessionInvalidFrames (connection.rs:1286) as node does, not against the rejected-streams budget, or the PR must document the session-level consequence.Extended reasoning...
A client opens one session and issues many requests. The server answers each with HEADERS :status 100 carrying END_STREAM. Each block sets malformed at connection.rs:1279. connection.rs:1286-1288 increments invalid_frame_count (node parity). connection.rs:1295-1300 then sends RST_STREAM(PROTOCOL_ERROR) and calls sink.on_stream_rejected. h2_frame_parser.rs:4156 increments rejected_streams and h2_frame_parser.rs:4157 compares against max_rejected_streams; at 100 it calls send_go_away with ENHANCE_YOUR_CALM, destroying the whole session. In node, Http2Session::OnInvalidFrame only bumps the invalid-frames counter (limit 1000); maxSessionRejectedStreams counts only memory-refused streams from OnBeginHeaders. The same accounting exists on the base for other malformed blocks, but this PR adds a response shape that a misconfigured proxy can emit for every request, so a busy session reaches the cap at request rate. Remedy: skip on_stream_rejected for client-side malformed response blocks, or raise the check to match node's counters.
Verification: nit (the budget divergence itself lives in untouched code; this PR adds one more input class to it) — acknowledged in diff: the PR description's "Invalid-frame budget" note says the block is counted against maxSessionInvalidFrames and maxSessionRejectedStreams "like every other malformed block" and that "node counts it as an invalid frame too"; the first half is accurate for Bun, but the…
-
🟣
src/runtime/api/bun/h2/connection.rs— Every pushed response that ends on a 1xx block now makes the Bun client write two RST_STREAM frames for the promised stream where node writes one. The engine sends RST_STREAM at connection.rs:1295 and then dispatches on_stream_reset at connection.rs:1299, and the JS pushed-stream teardown submits its own RST_STREAM again. Fix: the engine-initiated reset must mark the stream so the JS close path does not resubmit RST_STREAM, for every malformed pushed block, not only this input.Extended reasoning...
A server pushes stream 2 via PUSH_PROMISE and then answers it with HEADERS :status 100 carrying END_STREAM. finish_header_block runs with push_parent None for the HEADERS on stream 2, sets malformed at connection.rs:1279, sends RST_STREAM(PROTOCOL_ERROR) at connection.rs:1295, marks the stream Closed at 1297, and calls sink.on_stream_reset at 1299. h2_frame_parser.rs:4168 marks the legacy stream CLOSED with rst_code and dispatches onStreamError; the JS ClientHttp2Session streamError handler (http2.ts:5033) defers emitStreamErrorNT, which destroys the pushed stream, and the pushed-stream destroy path submits a second RST_STREAM for stream 2. The wire then carries two RST_STREAM frames for one stream; node sends exactly one. The PR text acknowledges this. A strict peer that treats RST_STREAM after its own END_STREAM as a stream error, or a conformance harness counting frames, observes the extra frame per malformed push. Rate: once per malformed pushed response. Remedy: have the JS reset path skip submitting RST_STREAM when the native stream already carries an engine-set rst_code.
Verification: pre-existing (nit). acknowledged in diff: PR description "Seen during the work, not changed here: The client sends RST_STREAM twice when the engine resets a pushed stream. Any malformed pushed response does this on main. node sends one." — that bound is accurate; this PR routes a new input class (1xx + END_STREAM on a promised stream, which base delivered with 0 RSTs) into the same untouched…
|
Replies to the two findings outside the diff. Rejected-streams budget. Correct. The malformed path calls Two RST_STREAM frames on a pushed stream. Correct. main does this for every engine reset of a pushed stream. #43539 fixes it in The inline finding (DATA after only a 1xx block) has its reply in the thread. #43558 owns that half, with the server fix it needs. |
This branch has not been deployed
1 participant
Problem
node:http2client accepts a response that ends on a 1xx header block. HEADERS:status: 100with END_STREAM givesheaders 100, end, rstCode=0: a clean end with no'response'event. node v26.3.0 sends RST_STREAM(PROTOCOL_ERROR), and the stream fails withERR_HTTP2_STREAM_ERROR.finish_header_blockinsrc/runtime/api/bun/h2/connection.rsuses itsinformationalflag only to keep the next block from being read as trailers. Nothing rejects END_STREAM on the 1xx block.Fix
finish_header_blockmarks a 1xx block that carries END_STREAM as malformed. The existing malformed path resets the stream with PROTOCOL_ERROR and does not deliver the block. HEADERS on a promised stream use the same code, so a pushed response follows the rule too.nghttp2_http_on_remote_end_streamwithNGHTTP2_HTTP_FLAG_EXPECT_FINAL_RESPONSE), and so does Bun's fetch HTTP/2 client (src/http/h2_client/dispatch.rs).test/js/node/http2/h2-conformance.test.ts(4 new tests, 2 fail on 1.4.3). Alsotest/js/node/http2/and the 261test-http2-*node tests.Background
Connectionis the inbound HTTP/2 engine. It parses frames and calls JS through aSink.Notes
Repro. A raw TCP HTTP/2 server answers stream 1, because
respond()cannot produce this shape. Client events, then the frames the client sent:error ERR_HTTP2_STREAM_ERROR, rstCode=1, RST_STREAM(1)headers 100, end, rstCode=0headers 103, end, rstCode=0headers 100, end, rstCode=0The other half of the report. The same report covered DATA that arrives before the final HEADERS (none yet, or only 1xx). #43558 owns that. It needs a server fix, because Bun's server writes an empty END_STREAM DATA frame when a stream closes before
respond(). An earlier version of this branch had a DATA check too. A Bun client on a Bun server then gotERR_HTTP2_STREAM_ERRORfromfor await (const chunk of req)where 1.4.3 and node-to-node give a clean end. This PR does not touch the DATA path. The two PRs change different functions inconnection.rsand add their test blocks at different places.Can Bun's own server send this shape? Not through valid API use.
additionalHeaders(),writeContinue(),writeEarlyHints()and the automatic100 Continuesend the 1xx block with no options, so it never carries END_STREAM.Bun.serveHTTP/2 sends its100 Continuethe same way. Two misuses can:respond()orwriteHead()with a 1xx status (node throwsERR_HTTP2_STATUS_INVALID, #43453 adds that check), and aBun.serveHTTP/2Responsewith status 101. A node client rejects both. Bun-to-Bun flows that send a 1xx block and then respond, end, close or destroy give the same client events as 1.4.3 (checked with the core and the compat API, with and without'error'listeners).Invalid-frame budget. The malformed path counts the block against
maxSessionInvalidFrames(default 1000) andmaxSessionRejectedStreams(default 100), like every other malformed block on main. So the 100th such response on one session makes the client send GOAWAY(ENHANCE_YOUR_CALM), and the session ends. node counts only an invalid frame for it (OnInvalidFramewithNGHTTP2_ERR_HTTP_MESSAGING), so its limit is 1000. main has this difference for every malformed response block, and this PR does not change it.Self-review. Addressed: the Bun-to-Bun regression and the overlap with #43558 (the DATA check was removed), three misleading comments, a test that hung instead of failing without the fix, an imprecise test comment, and a test without a session
'error'listener. Not addressed:respond()with a 1xx status on a Bun server (above). node:http2: validate the server stream :status like node #43453 owns it.connection.rs. The conformance tests cover both paths and fail by assertion without the fix.Seen during the work, not changed here.
content-length: a short or long body ends with a clean'end', and node resets the stream. node:http2: enforce RFC 9113 message framing on received responses #33004 fixed that and was closed as stale.:statusafter a 1xx block counts as the final response. node:http/http2 hardening: enforce h2 request pseudo-headers, never report a failed handler as success, and bound/validate the parser paths (+1 upstream test) #33191 rejects it.Overlap. #33191 and a pending change to the
:statusvalue check editfinish_header_blocknear theinformationallines. This PR adds four lines after the content-length block and does not change howinformationalis computed.Tests.
describe("END_STREAM on a 1xx HEADERS block (RFC 9113 §8.1)")drives the client withRawH2Server, on a request and on a pushed stream. Each case ends with a PING round trip, so the RST_STREAM is on the wire before the assertion and a dead session fails the test. The two control cases (1xx, then a final HEADERS with END_STREAM) pass with and without the fix.Suites run with the debug build.
h2-conformance.test.ts: 74 pass.test/js/node/http2/: 580 pass, 6 skip. The 261 vendoredtest-http2-*node tests pass.[human-review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file