fix(http2): server refuses streams beyond maxConcurrentStreams with REFUSED_STREAM#29054
fix(http2): server refuses streams beyond maxConcurrentStreams with REFUSED_STREAM#29054cirospaciari wants to merge 1 commit intomainfrom
Conversation
…EFUSED_STREAM RFC 9113 Section 5.1.2: an endpoint that receives a HEADERS frame causing its advertised concurrent-stream limit to be exceeded MUST treat this as a stream error (PROTOCOL_ERROR or REFUSED_STREAM). Bun's server accepted unlimited concurrent streams regardless of localSettings.maxConcurrentStreams. Now handleReceivedStreamID counts open streams and writes RST_STREAM(REFUSED_STREAM) before allocating a Stream when the limit is reached, so the JS 'stream' event never fires for the refused request. Ports Node test test/parallel/test-http2-max-concurrent-streams.js.
|
Updated 7:09 PM PT - Apr 8th, 2026
❌ @cirospaciari, your commit a045dba has 3 failures in
🧪 To try this PR locally: bunx bun-pr 29054That installs a local version of the PR into your bun-29054 --bun |
WalkthroughAdded HTTP/2 RFC 9113 Section 5.1.2 enforcement in the H2FrameParser to reject streams exceeding the server's maxConcurrentStreams limit by sending RST_STREAM frames with REFUSED_STREAM error codes. Includes a corresponding test validating behavior with concurrent stream attempts. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/api/bun/h2_frame_parser.zig`:
- Around line 2531-2555: The current pre-dispatch concurrent-stream check in
readBytes()/h2_frame_parser.zig uses null to indicate a REFUSED_STREAM, which
collides with the existing null sentinel handled by handleHeadersFrame()
(causing GOAWAY/PROTOCOL_ERROR and skipping payload consumption); move the
maxConcurrentStreams enforcement out of the pre-dispatch path into the specific
HEADERS handling branch and/or return a distinct sentinel instead of null (e.g.,
a tagged enum or a specific error variant like RefusedStream) so callers like
readBytes() and handleHeadersFrame() can differentiate "refused stream" from the
existing null/connection-error path; update uses of lastStreamID, the code that
writes the RST_STREAM(REFUSED_STREAM) (the FrameHeader creation and write
sequence), and any code that relies on currentFrame/remainingLength to ensure
payload is consumed or skipped correctly when a refusal is issued.
In `@test/js/node/test/parallel/test-http2-max-concurrent-streams.js`:
- Around line 14-17: The current test ends the first HTTP/2 stream immediately
(inside server.on('stream') where stream.respond() and stream.end('hello world')
are called), making the max-concurrent-streams behavior timing-dependent; change
the server stream handler to keep the first stream open (do not call stream.end)
and only close it after the second client request has been observed and
explicitly rejected (e.g., detect REFUSED_STREAM or error on the second request
produced by clientSession.request()); after asserting the second request is
refused, send a follow-up successful request on the same session and assert it
succeeds, then close the first stream and perform the existing session
cleanup—use the existing identifiers server, stream, clientSession/request
handlers to locate the code to modify.
🪄 Autofix (Beta)
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: b503d5c7-ba66-40f6-94fb-bf978a597ca7
📒 Files selected for processing (2)
src/bun.js/api/bun/h2_frame_parser.zigtest/js/node/test/parallel/test-http2-max-concurrent-streams.js
| // RFC 9113 Section 5.1.2: an endpoint that receives a HEADERS frame that | ||
| // causes its advertised concurrent stream limit to be exceeded MUST treat | ||
| // this as a stream error of type PROTOCOL_ERROR or REFUSED_STREAM. We use | ||
| // REFUSED_STREAM so the client may retry safely. | ||
| if (this.isServer and this.localSettings.maxConcurrentStreams != std.math.maxInt(u32)) { | ||
| var open: u32 = 0; | ||
| var it = this.streams.valueIterator(); | ||
| while (it.next()) |s| { | ||
| if (s.state != .CLOSED) open += 1; | ||
| } | ||
| if (open >= this.localSettings.maxConcurrentStreams) { | ||
| if (streamIdentifier > this.lastStreamID) this.lastStreamID = streamIdentifier; | ||
| var buffer: [FrameHeader.byteSize + 4]u8 = undefined; | ||
| @memset(&buffer, 0); | ||
| var writerStream = std.io.fixedBufferStream(&buffer); | ||
| const writer = writerStream.writer(); | ||
| var frame: FrameHeader = .{ .type = @intFromEnum(FrameType.HTTP_FRAME_RST_STREAM), .flags = 0, .streamIdentifier = streamIdentifier, .length = 4 }; | ||
| _ = frame.write(@TypeOf(writer), writer); | ||
| var value: u32 = @intFromEnum(ErrorCode.REFUSED_STREAM); | ||
| value = @byteSwap(value); | ||
| _ = writer.write(std.mem.asBytes(&value)) catch 0; | ||
| _ = this.write(&buffer); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't use null here to mean "refused stream".
Line 2553 collides with the existing null sentinel used by the frame handlers: readBytes() passes this result straight into handleHeadersFrame(), whose null branch is the connection-stream error path, so an over-limit request will send RST_STREAM(REFUSED_STREAM) and then still emit GOAWAY(PROTOCOL_ERROR). That fast-fail also skips payload consumption, leaving currentFrame/remainingLength stale. Because this helper runs before frame-type dispatch, the new branch can also refuse fresh non-HEADERS frames. Please move the limit enforcement into the HEADERS path and represent refusal with a distinct state/sentinel instead of null.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/api/bun/h2_frame_parser.zig` around lines 2531 - 2555, The current
pre-dispatch concurrent-stream check in readBytes()/h2_frame_parser.zig uses
null to indicate a REFUSED_STREAM, which collides with the existing null
sentinel handled by handleHeadersFrame() (causing GOAWAY/PROTOCOL_ERROR and
skipping payload consumption); move the maxConcurrentStreams enforcement out of
the pre-dispatch path into the specific HEADERS handling branch and/or return a
distinct sentinel instead of null (e.g., a tagged enum or a specific error
variant like RefusedStream) so callers like readBytes() and handleHeadersFrame()
can differentiate "refused stream" from the existing null/connection-error path;
update uses of lastStreamID, the code that writes the RST_STREAM(REFUSED_STREAM)
(the FrameHeader creation and write sequence), and any code that relies on
currentFrame/remainingLength to ensure payload is consumed or skipped correctly
when a refusal is issued.
| server.on('stream', common.mustCall((stream) => { | ||
| stream.respond(); | ||
| stream.end('hello world'); | ||
| })); |
There was a problem hiding this comment.
Make this regression deterministic and prove the session stays alive.
The first stream is ended immediately, so the second request only exercises the limit if timing happens to keep the first one open long enough. Then Lines 22-25 shut both sides down as soon as the two streams close, so this would still pass if the implementation sent REFUSED_STREAM and then GOAWAYed the session. Please hold the first stream open until the second request is refused, then verify a follow-up request still succeeds before cleanup.
Also applies to: 22-25, 31-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/node/test/parallel/test-http2-max-concurrent-streams.js` around lines
14 - 17, The current test ends the first HTTP/2 stream immediately (inside
server.on('stream') where stream.respond() and stream.end('hello world') are
called), making the max-concurrent-streams behavior timing-dependent; change the
server stream handler to keep the first stream open (do not call stream.end) and
only close it after the second client request has been observed and explicitly
rejected (e.g., detect REFUSED_STREAM or error on the second request produced by
clientSession.request()); after asserting the second request is refused, send a
follow-up successful request on the same session and assert it succeeds, then
close the first stream and perform the existing session cleanup—use the existing
identifiers server, stream, clientSession/request handlers to locate the code to
modify.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The concurrent-stream limit check returns null from handleReceivedStreamID, but every frame handler treats null as a connection-level PROTOCOL_ERROR, so a refused stream triggers both RST_STREAM(REFUSED_STREAM) and GOAWAY(PROTOCOL_ERROR) — killing the entire connection rather than allowing safe client retry. The check also fires for non-HEADERS frames (DATA, PRIORITY, etc.) which do not create streams per RFC 9113 §5.1.2.
Extended reasoning...
Overview
The PR adds a concurrent-stream limit check inside handleReceivedStreamID in h2_frame_parser.zig. When the server is at capacity, it writes RST_STREAM(REFUSED_STREAM) and returns null. A test ported from Node is also added.
Security Risks
No direct security implications. The change affects HTTP/2 stream lifecycle management.
Level of Scrutiny
This is production-critical HTTP/2 networking code. The bug reports identify a real design flaw: the pre-existing invariant is that handleReceivedStreamID returns null only for stream-0 (connection control stream), always a PROTOCOL_ERROR. The PR introduces a second null-return path without updating any callers. Every frame handler — handleHeadersFrame (line 2350), handleContinuationFrame (line 2309), handleDataFrame (line 1994), handlePriorityFrame (line 2271) — has an orelse sendGoAway(PROTOCOL_ERROR) branch. So a refused stream sends both RST_STREAM(REFUSED_STREAM) and GOAWAY(PROTOCOL_ERROR), tearing down the entire connection.
Additionally, the check fires for all frame types at the dispatch site (lines 2607, 2655, 2689), not just HEADERS. RFC 9113 §5.1.2 restricts the concurrent-stream limit to HEADERS frames. Sending RST_STREAM(REFUSED_STREAM) for a PRIORITY frame on an idle stream is semantically wrong and also triggers GOAWAY.
Other Factors
The bundled test may pass due to nghttp2 client-side timing (RST_STREAM processed before GOAWAY), masking the GOAWAY-induced connection teardown. The correct fix is to move the check inside handleHeadersFrame only, send RST_STREAM(REFUSED_STREAM), and return normally rather than null — preserving the existing invariant.
| // RFC 9113 Section 5.1.2: an endpoint that receives a HEADERS frame that | ||
| // causes its advertised concurrent stream limit to be exceeded MUST treat | ||
| // this as a stream error of type PROTOCOL_ERROR or REFUSED_STREAM. We use | ||
| // REFUSED_STREAM so the client may retry safely. | ||
| if (this.isServer and this.localSettings.maxConcurrentStreams != std.math.maxInt(u32)) { | ||
| var open: u32 = 0; | ||
| var it = this.streams.valueIterator(); | ||
| while (it.next()) |s| { | ||
| if (s.state != .CLOSED) open += 1; | ||
| } | ||
| if (open >= this.localSettings.maxConcurrentStreams) { | ||
| if (streamIdentifier > this.lastStreamID) this.lastStreamID = streamIdentifier; | ||
| var buffer: [FrameHeader.byteSize + 4]u8 = undefined; | ||
| @memset(&buffer, 0); | ||
| var writerStream = std.io.fixedBufferStream(&buffer); | ||
| const writer = writerStream.writer(); | ||
| var frame: FrameHeader = .{ .type = @intFromEnum(FrameType.HTTP_FRAME_RST_STREAM), .flags = 0, .streamIdentifier = streamIdentifier, .length = 4 }; | ||
| _ = frame.write(@TypeOf(writer), writer); | ||
| var value: u32 = @intFromEnum(ErrorCode.REFUSED_STREAM); | ||
| value = @byteSwap(value); | ||
| _ = writer.write(std.mem.asBytes(&value)) catch 0; | ||
| _ = this.write(&buffer); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🔴 The new null return path in handleReceivedStreamID is treated as a connection-level error by every caller: handleHeadersFrame, handleContinuationFrame, handleDataFrame, and handlePriorityFrame all have an orelse sendGoAway(PROTOCOL_ERROR) branch that fires on null, so a refused stream causes RST_STREAM(REFUSED_STREAM) followed immediately by GOAWAY(PROTOCOL_ERROR), which kills the entire connection and makes safe client retry impossible—directly contradicting the stated RFC 9113 intent. Additionally, the concurrent-stream limit check fires for all incoming frame types (DATA, PRIORITY, etc.), not just HEADERS; only HEADERS frames open new streams, so sending REFUSED_STREAM for a PRIORITY frame on an idle stream (valid per RFC 9113 §5.3.1) is semantically wrong and also triggers the GOAWAY path.
Extended reasoning...
Root cause — broken null invariant
The pre-existing invariant in this file is: handleReceivedStreamID returns null only when the frame targets stream 0 (the connection control stream), which is always a connection-level protocol error. Every frame handler relies on this: handleHeadersFrame (line 2350), handleContinuationFrame (line 2309), handleDataFrame (line 1994), and handlePriorityFrame (line 2271) all contain an orelse { this.sendGoAway(frame.streamIdentifier, ErrorCode.PROTOCOL_ERROR, ..., true); return data.len; } branch. The PR introduces a second null-return path—refused streams—without updating any of these callers.
Concrete HEADERS path (bug_001)
- Client sends HEADERS for stream N when
open >= maxConcurrentStreams. readBytes→handleReceivedStreamID(N): check fires, writesRST_STREAM(REFUSED_STREAM), returnsnull.readBytes→handleHeadersFrame(header, bytes, null).handleHeadersFrameline 2350:var stream = stream_ orelse { this.sendGoAway(frame.streamIdentifier, ErrorCode.PROTOCOL_ERROR, "Headers frame on connection stream", this.lastStreamID, true); return data.len; };sendGoAwaywithemitError=truedispatches.onErrorand.onEnd, terminating the entire HTTP/2 connection.
Net result: the server sends both RST_STREAM(REFUSED_STREAM) and GOAWAY(PROTOCOL_ERROR). The GOAWAY kills the connection for all in-flight streams, defeating the stated goal of allowing the client to retry safely. The same path fires through handleContinuationFrame for multi-frame HEADERS sequences.
Non-HEADERS frame types (bug_002)
handleReceivedStreamID is invoked at lines 2607, 2655, and 2689 for all frame types before the per-type dispatch. The new check fires whenever a frame arrives with an unknown stream ID while the server is at capacity, regardless of frame type. RFC 9113 §5.1.2 restricts the concurrent-stream limit to HEADERS frames (stream creation). Specifically:
- A PRIORITY frame on an idle stream is explicitly valid per RFC 9113 §5.3.1. The server incorrectly sends
RST_STREAM(REFUSED_STREAM), thenhandlePriorityFrame(line 2271) receivesnulland firessendGoAway(PROTOCOL_ERROR), terminating the connection for RFC-valid client behavior. - DATA on a non-existent stream is a protocol violation, but the correct response is
STREAM_CLOSED, notREFUSED_STREAM(which semantically means "retry is safe"). - WINDOW_UPDATE on a non-existent stream avoids GOAWAY because its handler uses
if (stream) |s|, but still incorrectly emitsRST_STREAM(REFUSED_STREAM).
Why the bundled test may pass despite the bug
The nghttp2 client library processes RST_STREAM before GOAWAY if both arrive in the same TCP segment. The second request's aborted event fires from the RST_STREAM, and the countdown.dec() in the close handler may decrement to zero before the GOAWAY-induced connection teardown prevents the first request from completing. This makes the test timing-sensitive rather than a definitive correctness proof.
Fix
Move the concurrent-stream limit check inside handleHeadersFrame (or pass the frame type into handleReceivedStreamID and gate the check on FrameType.HTTP_FRAME_HEADERS). When the limit is exceeded, send RST_STREAM(REFUSED_STREAM) and return normally (do not return null), so no GOAWAY is triggered. This preserves the existing null-means-connection-error invariant and restricts the refused-stream logic to HEADERS frames only, as RFC 9113 §5.1.2 requires.
|
Converting to draft per review: returning |
2 participants
RFC 9113 §5.1.2: an endpoint that receives a HEADERS frame causing its advertised concurrent-stream limit to be exceeded MUST treat this as a stream error of type PROTOCOL_ERROR or REFUSED_STREAM. Bun's server accepted unlimited concurrent streams regardless of
localSettings.maxConcurrentStreams.Now
handleReceivedStreamIDcounts open streams and writesRST_STREAM(REFUSED_STREAM)before allocating a Stream when the limit is reached, so the JS'stream'event never fires for the refused request and the client seesNGHTTP2_REFUSED_STREAM.Ports Node's
test/parallel/test-http2-max-concurrent-streams.js.