Skip to content

node:http2: do not refuse respond() when the session is over maxSessionMemory - #43419

Open
robobun wants to merge 1 commit into
mainfrom
robobun/79f0da79/http2-respond-session-memory
Open

robobun wants to merge 1 commit into
mainfrom
robobun/79f0da79/http2-respond-session-memory

Conversation

@robobun

@robobun robobun commented Sep 19, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • A node:http2 client hangs when it makes 56 parallel requests with 200 KB responses. Requests past the 53rd get no 'response', 'error' or 'close'. Node v26.3.0 completes every request.
  • The cause is the session memory check in the native request() (src/runtime/api/bun/h2_frame_parser.rs:7048). A server's respond() runs it too. Over maxSessionMemory it closes the stream locally (Stream closed with error code NGHTTP2_ENHANCE_YOUR_CALM). No HEADERS and no RST_STREAM reach the wire.

Fix

  • The check now applies to a client only. A server reaches request() only for an open stream.
  • Correct because node applies maxSessionMemory to new streams only. Http2Stream::SubmitResponse has no memory check. The refusal of a new inbound stream (can_open_stream) is unchanged.
  • Verified: test/js/node/http2/h2-conformance.test.ts (one new test, fails on bun 1.4.3 canary). Also all of test/js/node/http2/ and node's 261 test-http2-*.js files.
  • Self-reviewed: 4 concerns raised, 2 addressed. The other 2 exist on main (see Notes).

Background

  • maxSessionMemory is a session option in MB (default 10). Queued outbound data counts toward it. A response larger than the peer's flow-control window stays queued until WINDOW_UPDATE frames arrive.
  • Considered: keep the refusal and send RST_STREAM. It still fails requests that node completes.

Downsides

Notes

Repro (real TCP sockets, bun par.mjs 60):

import http2 from "node:http2";
import { once } from "node:events";
const N = Number(process.argv[2] ?? 60);
const server = http2.createServer();
server.on("stream", stream => {
  stream.on("data", () => {});
  stream.on("end", () => { stream.respond({ ":status": 200 }); stream.end(Buffer.alloc(200_000, "r")); });
  stream.on("error", () => {});
});
server.listen(0, "127.0.0.1");
await once(server, "listening");
const client = http2.connect(`http://127.0.0.1:${server.address().port}`);
const results = { N, responses: 0, closes: 0, bytes: 0, errors: [] };
process.on("exit", code => console.log(JSON.stringify({ ...results, exit: code })));
await Promise.all(Array.from({ length: N }, (_, i) => new Promise(resolve => {
  const req = client.request({ ":method": "POST", ":path": "/" + i });
  req.on("response", () => results.responses++);
  req.on("data", c => (results.bytes += c.length));
  req.on("error", e => results.errors.push(String(e.code ?? e.message)));
  req.on("close", () => { results.closes++; resolve(); });
  req.end("hi");
})));
client.close(); server.close();
server client N before after
bun bun 60 hang, 53 responses 60 responses
bun node v26.3.0 60 hang, 53 responses 60 responses
node v26.3.0 bun 60 60 responses 60 responses
bun bun 200 GOAWAY code 11, 53 responses 200 responses
bun bun, over duplexPair() 60 hang 60 responses
  • 53 x 200 KB is 10.6 MB, the first count over the 10 MiB default. Every stream was already open at that point (the server log shows 60 'stream' events), so the refusal came from respond(), not from the inbound HEADERS check.
  • The refused respond() also counted toward maxSessionRejectedStreams. After 100 refusals the server sent GOAWAY with ENHANCE_YOUR_CALM. That is the N=200 row.
  • Node source: Http2Session::OnBeginHeadersCallback calls CanAddStream() for a new inbound stream only. Http2Stream::AddHeader checks the budget for each inbound header field. Http2Stream::SubmitResponse and Http2Session::SubmitRequest have no check (src/node_http2.cc, v26.3.0).
  • The new test passes on node v26.3.0 when run as a standalone script. It uses a raw TCP client, so the 4 MiB body moves in one flow-control round trip and the test stays fast on debug builds.
  • The client side of this check is unchanged. A client request() made while the session is over budget still fails with ENHANCE_YOUR_CALM. Node fails the same request with the same code, later, when the response headers do not fit the budget.
  • Not in this PR: the check runs after the header block was HPACK-encoded. A refused client request() leaves entries in the encoder table that the peer never sees, and later requests on the session fail. node:http2: encode each header block in one call, all fields or none #41520 moves the check before the encode.
  • Not in this PR: respond() with a header block over maxSendHeaderBlockLength has the same shape (the stream closes locally, no frame is sent). Node sends RST_STREAM with FRAME_SIZE_ERROR and closes the session. That needs new wire behavior, so it is tracked in node:http2: respond() with a header block over maxSendHeaderBlockLength sends no frame, the client request hangs #43423.
  • Self-review detail: the two concerns that exist on main are the client request() path and the maxSendHeaderBlockLength path in the two bullets above. The two addressed concerns are the wording of the code comment and a slow end-to-end test, which one raw-client test replaced.
  • Not in this PR: the other uses of the rejected-stream count. on_stream_rejected (src/runtime/api/bun/h2_frame_parser.rs:4167 on main) also counts a malformed header block and an oversized header list (src/runtime/api/bun/h2/connection.rs:1419 and :1430). It counts them for trailers and for a client session too, and the count never goes back to 0. Node counts only a new stream that it cannot create, and sets the count to 0 when it creates a stream (https://github.com/nodejs/node/blob/v26.3.0/src/node_http2.cc#L1035-L1050).
  • Measured for that count with maxSessionRejectedStreams: 3, bun 1.4.3 canary against node v26.3.0: 8 requests with an oversized header list end the Bun session at the 3rd (GOAWAY code 11). Node answers 8 RST_STREAM frames and keeps the session. The result is the same for 8 malformed blocks, for trailers on open streams, and for a client session. No open PR covers it. node:http2: count a refused stream where the session refuses it, not in rstStream #44248 removes the count from rstStream only.

…onMemory

The native request() function sends the header block for a client
request() and for a server respond() or additionalHeaders(). It refused
the block when the session was over maxSessionMemory. On a server this
closed the stream locally and wrote no frame, so the peer's request never
got 'response', 'error' or 'close'.

node applies the budget to new streams only (Http2Session::CanAddStream)
and Http2Stream::SubmitResponse has no memory check. The check now
applies to a client only.
@robobun

robobun commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator Author

Status for #43419.

How I reproduced it:

  • Ran the script in the PR notes (60 parallel POST requests, each answered with a 200 KB body) with bun 1.4.3-canary.1+367d939d9 on linux x64. The client stops at 53 responses and the other 7 requests get no event. Node v26.3.0 completes all 60.
  • The same client against a Bun server hangs with a node v26.3.0 client too. A Bun client against a node server completes. So the fault is on the server side.
  • The server log shows Stream closed with error code NGHTTP2_ENHANCE_YOUR_CALM on each lost stream, raised from respond().
  • Reduced it to a deterministic case: a maxSessionMemory: 1 server, two open streams, 4 MiB queued on the first, then respond() on the second. Stock bun sends nothing for the second stream. This is the new test in test/js/node/http2/h2-conformance.test.ts.

With the fix, N=60 and N=200 complete over TCP and over duplexPair(), and a node client against the fixed server completes.

@coderabbitai

coderabbitai Bot commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: oven-sh/bun/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: fcb82fd9-8487-4e8b-8e8f-e821659cb2e5

📥 Commits

Reviewing files that changed from the base of the PR and between 26e7a4b and fcadb16.

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

Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.


Walkthrough

The parser now applies session-memory rejection only to client-initiated streams. A conformance test verifies that already-open streams still receive 4 MiB and 2-byte responses after flow-control credit.

Changes

HTTP/2 session memory handling

Layer / File(s) Summary
Response memory behavior and conformance coverage
src/runtime/api/bun/h2_frame_parser.rs, test/js/node/http2/h2-conformance.test.ts
H2FrameParser::request no longer rejects server-side responses because of session memory. New inbound streams remain controlled by can_open_stream. The test verifies that two streams opened before exhaustion receive headers and complete their expected response bodies.

Suggested reviewers: jarred-sumner, cirospaciari

Priority: ➖ Normal

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly identifies the main change: preventing node:http2 respond() from being refused when the session exceeds maxSessionMemory.
Description check ✅ Passed The description explains the problem, implementation, verification steps, behavior changes, and known limitations. It provides the information requested by the template, although it uses Problem and F…

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

@claude claude 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.

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 the server's other outbound header path for the same gate: push_promise() in h2_frame_parser.rs never applied the maxSessionMemory check, and additionalHeaders() reaches the same request() line with is_server set, so after this change every server-side submission is consistently unchecked (matching node, where only OnBeginHeadersCallback refuses). The new test's helpers (drainFirstStream, RawH2, requestHeaderBlock) are defined in the same file/describe scope, and the pre-fix behavior fails the test via waitFor timeout rather than passing vacuously.

Extended reasoning...

The two inline findings cover the remaining branches of this bug class (the maxSendHeaderBlockLength refusal a few lines below, and the client-side ordering of the memory check relative to HPACK encoding). Separately, I traced the server-side entry points that could still hit the narrowed check: push_promise() (line ~6017) has no memory-limit call at all, and additionalHeaders()/respond() both go through request() with is_server true, so the server half is now internally consistent with can_open_stream() as the only inbound gate. The test uses the existing drainFirstStream helper and RawH2.waitFor (2s default timeout rejecting on no frame), so on the unfixed code it fails by timeout/RST rather than passing.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🟣 src/runtime/api/bun/h2_frame_parser.rs — Servers that set maxSendHeaderBlockLength still leave the peer hanging when respond() sends an oversized header block, the same hang shape this PR fixes one branch above. At h2_frame_parser.rs:7087-7105 the block sets stream.state CLOSED and dispatches onFrameError/onStreamError, but writes no HEADERS and no RST_STREAM. The peer waits forever on that stream, exactly like the memory refusal at 7053 did before this PR. Node sends RST_STREAM FRAME_SIZE_ERROR and tears the session down. Fix: every local refusal in request() that closes a stream must put a RST_STREAM on the wire, or the refusal must be moved before encoding so the peer stays in sync.

    Extended reasoning...

    A server is created with maxSendHeaderBlockLength set (default is 0 at 7468, so the population is servers that opted in). A handler calls respond() with a header block whose encoded size exceeds the limit, for example a large set-cookie or a long custom header. request() encodes the block (6500-6873), then 7087 compares encoded_size and takes the refusal branch. stream.state becomes CLOSED and rst_code REFUSED_STREAM, JS gets onFrameError then onStreamError, and the function returns at 7105. Nothing was written to this.to_writer(), so the client sees neither HEADERS nor RST_STREAM and its request never settles. Also, the encoder table was mutated by the failed encode, so subsequent responses on the session decode wrong at the peer. The PR comment at 7048-7051 says a refusal here puts no frame on the wire so the peer would wait forever; that reasoning applies word for word to 7087. The dismissing finder read the base and found it identical; that is true, but the PR touched this function to fix exactly this shape and left the sibling. Remedy: emit RST_STREAM(FRAME_SIZE_ERROR) and follow…

    Verification: pre-existing; acknowledged in diff: the PR description says "Not in this PR: respond() with a header block over maxSendHeaderBlockLength has the same shape (the stream closes locally, no frame is sent) ... tracked as separate work" — that note is accurate about the code, but it records the hazard rather than resolving it. Triggering condition: a server created with maxSendHeaderBlockLength…

Comment thread src/runtime/api/bun/h2_frame_parser.rs
@robobun

robobun commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator Author

On the maxSendHeaderBlockLength finding: confirmed, and tracked in #43423 with a repro and node's output.

The branch at h2_frame_parser.rs:7087 closes the stream locally and writes no frame, so the client request hangs. This PR does not change it. The remedy is different from the maxSessionMemory case. For session memory node never refuses respond(), so the fix is to stop the refusal. For an oversized header block node does refuse: it emits 'frameError', then resets the stream with FRAME_SIZE_ERROR and closes the session (onFrameError in lib/internal/http2/core.js). Bun needs new wire behavior and a different error code for that, so it is separate work. #41520 also edits that block (it moves the check before the HPACK encode).

The inline finding about the client path has a reply in its thread.

@robobun

robobun commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 1:59 AM PT - Sep 19th, 2026

✅ @robobun, your commit fcadb16a91c852036773ea456ac522f8289721d3 passed in Build #118223! 🎉


🧪   To try this PR locally:

bunx bun-pr 43419

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

bun-43419 --bun

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant