Skip to content

h2 server: decode and discard HEADERS on a closed client stream instead of resetting or re-opening it - #37985

Open
alii wants to merge 1 commit into
mainfrom
ali/h2-headers-on-closed-stream
Open

alii wants to merge 1 commit into
mainfrom
ali/h2-headers-on-closed-stream

Conversation

@alii

@alii alii commented Aug 13, 2026

Copy link
Copy Markdown
Member

Problem

node:http2 server, Rust h2 engine (src/runtime/api/bun/h2/connection.rs). When a client sends HEADERS on a stream id that is already closed, bun did one of two wrong things depending on timing:

  • If the closing RST_STREAM and the late HEADERS were parsed in the same read, bun decoded the block and answered RST_STREAM(STREAM_CLOSED). RFC 9113 §5.1 says an endpoint "MUST NOT send frames other than PRIORITY on a closed stream"; the RST reply was RFC 7540's rule, dropped in 9113.
  • If the HEADERS arrived in a later read (the normal case), the closed entry had already been evicted from streams, so handle_headers saw !streams.contains_key(id) and treated it as a brand-new request: fired on_stream_open, delivered it to JS as a 'stream' event, and answered 200. Same for a never-used lower id (HEADERS(3) after stream 5 was opened), which §5.1.1 says is implicitly closed. That is stream-id reuse being accepted.

Measured against node v22.20.0 and v26.4.0 (nghttp2 1.69.0) with a raw-socket probe: in every one of these cases node decodes the block (HPACK stays in sync), sends nothing, surfaces nothing to JS, and keeps the connection up — nghttp2's NGHTTP2_ERR_IGN_HEADER_BLOCK path in session_on_request_headers_received.

scenario (server side) node 22 / 26 bun before bun after
RST(1) then HEADERS(1)+CONTINUATION, same read discard, nothing sent RST_STREAM(1, STREAM_CLOSED) discard, nothing sent
RST(1) … later read … HEADERS(1) discard new 'stream' id 1 to JS, 200 sent discard
stream 1 fully answered … later HEADERS(1) discard new 'stream' id 1 to JS, 200 sent discard
stream 5 opened … later HEADERS(3) discard new 'stream' id 3 to JS, 200 sent discard
HPACK dynamic table after the discarded block in sync in sync in sync

Fix

  • Add last_peer_stream_id: the highest client-initiated stream id an inbound HEADERS has opened (refused streams included; on a client, the highest promised id) — nghttp2's last_recv_stream_id. In handle_headers, a server-side id with no streams entry at or below that mark is a closed stream, not a new one: no entry is created, nothing is opened/refused/counted, and the block gets BlockDisposition::StreamClosed.
  • BlockDisposition::StreamClosed (also reached when an existing entry is State::Closed) now means §5.1 minimal processing: finish_header_block still HPACK-decodes the whole block, then returns without sending a frame or calling the sink. HEADERS on a half-closed (remote) stream still escalates to a connection error STREAM_CLOSED, as nghttp2 does.
  • The existing mixed last_stream_id (also raised by the engine's own send_header_block / send_push_promise) is left as-is for GOAWAY and the RST_STREAM idle check; a separate peer-only mark is what keeps a server that has promised even ids from misreading a lower odd id as closed. Under today's embedder, which does not send through those engine APIs, the two marks hold the same value on a server.
  • Client-side HEADERS handling, PUSH_PROMISE, refused streams (maxSessionMemory), trailers, DATA/RST_STREAM/WINDOW_UPDATE on closed streams: unchanged.
  • Three comments that described the old behavior (RST reply; "a late HEADERS for an evicted id re-opens a fresh entry") are corrected.

Tests

test/js/node/http2/h2-conformance.test.ts, describe("inbound stream lifecycle"), raw-socket client against a bun server. Each closed-stream test sends the late block as HEADERS + CONTINUATION whose CONTINUATION inserts x-bun-sync: 1 into the HPACK dynamic table, then opens a fresh stream referencing index 62 and uses a PING ack as the barrier. Asserts: nothing sent on the closed id, no GOAWAY, the probe stream is answered (so the discarded block was decoded), and only the probe stream reached JS.

  • discards HEADERS following the peer's RST_STREAM in the same write
  • discards HEADERS arriving in a later read for a stream the peer reset
  • discards HEADERS reusing the id of a fully answered stream
  • discards HEADERS on a lower stream id than one already opened
  • a client stream id below the server's promised ids is still new (request 1 pushes 2/4/6, then HEADERS(3) must be served)

The first four fail on main (bun-debug built from 8a1cd8d) and pass with this change; the fifth passes on both today and guards the peer-only mark.

Verification

Debug build on macOS arm64:

  • bun bd test test/js/node/http2/h2-conformance.test.ts: 66 pass.
  • bun bd test test/js/node/http2/: 468 pass, 6 skip, 1 fail — the failure is node-http2-upgrade.test.mts "tests should run on node.js" (spawns the local node --test on an .mts file) and fails identically on main.
  • All 256 test/js/node/test/parallel/test-http2-* and 5 test/sequential/test-http2-* exit 0.
  • cargo clippy -p bun_runtime -- -D warnings clean.

Not in this PR (found while probing)

When RST_STREAM(1) arrives in the same read as the HEADERS that opened stream 1, bun still writes the response HEADERS/DATA for stream 1 that the JS 'stream' handler produced; node writes nothing. The handler runs synchronously from on_headers_complete and respond() serializes straight into the corked write buffer (h2_frame_parser.rs request / write_stream) before the RST later in the batch is parsed, and on_stream_reset cannot retract already-serialized bytes; request also lacks the closed-stream guard write_stream has. Separate change.

@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

HTTP/2 closed stream handling

Layer / File(s) Summary
Peer stream high-water tracking
src/runtime/api/bun/h2/connection.rs
The connection tracks peer-initiated stream IDs separately and uses this value to classify late inbound HEADERS.
Closed HEADERS disposition
src/runtime/api/bun/h2/connection.rs
Closed-stream HEADERS are decoded for HPACK synchronization and discarded without resets or callbacks. Client promised streams can still be reopened.
Conformance coverage
test/js/node/http2/h2-conformance.test.ts
Tests cover HEADERS after reset, completed streams, implicitly closed streams, and client streams below promised server push IDs.

Possibly related PRs

Suggested reviewers: robobun, cirospaciari, jarred-sumner

Mergeability Score: 🔵 Low · up to 18c2e

The PR changes closed-stream HTTP/2 HEADERS handling to discard late blocks while preserving connection state; the remaining merge-readiness issue is confined to the test helper, where a connection failure can leave the server listening and cause a timeout instead of reporting the original error. This is mergeable with explicit follow-up to close the server on that failure path.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: discarding HEADERS on closed client streams without resetting or reopening them.
Description check ✅ Passed The description explains the problem, fix, tests, verification results, and scope exclusions in sufficient detail.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/node/http2/h2-conformance.test.ts`:
- Around line 1303-1320: Update recordingSession to close the server when
RawH2.connect rejects, while preserving the successful return path. Wrap the
connection attempt in failure handling that awaits server.close() before
rethrowing the original error, ensuring the listening handle is released.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 04efe4f8-6c52-45b4-8fc2-e84586321919

📥 Commits

Reviewing files that changed from the base of the PR and between a28fcc4 and 18c2e93.

📒 Files selected for processing (2)
  • src/runtime/api/bun/h2/connection.rs
  • test/js/node/http2/h2-conformance.test.ts

Comment thread test/js/node/http2/h2-conformance.test.ts
@robobun

robobun commented Aug 13, 2026 •

Copy link
Copy Markdown
Collaborator
Updated 10:56 AM PT - Aug 13th, 2026

✅ @alii, your commit 18c2e930f75e385e809a0aca973a273cdd3234c9 passed in Build #93827! 🎉


🧪   To try this PR locally:

bunx bun-pr 37985

That installs a local version of the PR into your bun-37985 executable, so you can run:

bun-37985 --bun

@alii
alii requested a review from cirospaciari August 13, 2026 21:30
robobun added a commit that referenced this pull request Aug 13, 2026
…he previous one

RFC 9113 5.1.1 numbers every new promised stream above all earlier ones.
The client only caught a reused or lower id while the earlier promised
stream still had an engine entry; once that stream closed and was evicted
the promise was accepted and delivered again. Track the highest promised
id (the peer high-water mark, declared as in #37985 so the two merge
cleanly in either order) and fail the session like nghttp2 does.
@robobun

robobun commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator

@alii #44337 (the stream limit, enforced in Connection::handle_headers) needs the rule that this PR adds: a client stream id at or below last_peer_stream_id with no entry is closed, and its block is decoded and dropped.

#44337 refuses a stream over the limit before it makes a stream entry. So a later HEADERS or DATA frame on that id has nothing to find. To give such a frame no second answer, #44337 keeps a sorted list of the refused ids (refused_ids, about 35 lines). With the mark of this PR the list is not needed: the mark goes up for a refused stream too, and the test is_new && is_server && id <= last_peer_stream_id sits in front of the limit test, as in nghttp2_session_on_request_headers_received.

This branch conflicts with main now. Which do you prefer?

  1. You rebase this PR. node:http2: refuse a stream over the advertised limit before it is opened, on maxSessionInvalidFrames #44337 then rebases on it and drops the list.
  2. I rebase this branch for you.
  3. I carry this commit under node:http2: refuse a stream over the advertised limit before it is opened, on maxSessionInvalidFrames #44337, with you as the author.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants