Repository navigation
Conversation
A server can refuse a request over HTTP/2 with HTTP_1_1_REQUIRED (RFC 9113 section 7). The fetch client rejected such a request. The h2 session now marks a stream that gets RST_STREAM with the code, or that is above the last stream id of a GOAWAY with the code. When the stream has no response yet, the request is pinned to HTTP/1.1 and starts again. A request that names its protocol, or whose body is a stream, behaves as before.
|
Status: the change is up. It waits for CI and review. How it was reproduced: a raw TLS server offers PR: #46259 |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 11 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Updated 3:58 AM PT - Oct 21st, 2026
❌ @robobun, your commit 392f82e has 5 failures in
🧪 To try this PR locally: bunx bun-pr 46259That installs a local version of the PR into your bun-46259 --bun |
Under ASAN the file lets 4 subprocesses run at a time, and a test waits for its slot while its timeout runs. On a 12-core machine a test waits up to 2.6 s, and 2 of 4 runs of the file failed, each time with another test at the 5 s default.
![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.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/http/h2_client/dispatch.rs— pre-existing: a request still unanswered when the server's GOAWAY(HTTP_1_1_REQUIRED) names its stream keeps rejecting with HTTP2GoAway, so the fallback never fires for a Node server'ssession.goaway(NGHTTP2_HTTP_1_1_REQUIRED), which names the stream it received. dispatch.rs:549-554 sets onlyfatal_errorfor streams at or below last_stream_id, neverhttp1_required, so ClientSession.rs:1218 does not retry. Fix: mark those streams too whenstatus_code == 0, limited to idempotent methods (GET, HEAD, OPTIONS, PUT, DELETE) if the replay concern in the PR notes stands; Chromium retries this frame shape for every method.Why this was flagged
Trigger: the server sends GOAWAY with error code 0xd and last_stream_id equal to or above the open stream, before any response HEADERS;
session.goaway(http2.constants.NGHTTP2_HTTP_1_1_REQUIRED)in node:http2 (the only API a Node server has for this code) sends exactly that, and the PR's own test 'GOAWAY that names the stream: the request rejects' codifies it. In dispatch.rs:539-555 thes.id > last_idarm setss.http1_required; theelse if !graceful && !s.remote_closed()arm at 549-554 setss.fatal_error = Some(crate::Error::HTTP2GoAway)only. Indeliver_streamthe new check at ClientSession.rs:1218 requiresstream.http1_required, so the request goes toh2_fail(err)andfetch()rejects withHTTP2GoAway, exactly as on the base branch, although the server asked for HTTP/1.1 andcan_retry_over_http1()is true. The PR notes name this as left open pending a decision on replaying non-idempotent requests; restricting the retry to idempotent methods or to streams whose request body was never sent removes that concern for the common GET case.Verification: Pre-existing; acknowledged in diff. The trigger is a server GOAWAY(0xd) with last_stream_id >= the open stream before any response HEADERS. In src/http/h2_client/dispatch.rs:549-554 the else arm sets only
s.fatal_error = Some(crate::Error::HTTP2GoAway)and never toucheshttp1_required, so ClientSession.rs:1218 does not retry and fetch rejects with HTTP2GoAway; the base takes the same path.
| debug_assert!(self.h2.is_none()); | ||
| debug_assert!(self.can_retry_over_http1()); | ||
| self.pin_to_http1(); | ||
| self.restart_from_top(); |
There was a problem hiding this comment.
🟡 (optional) Users running with the h2 flag can lose HTTP/2 for an origin after a single HTTP_1_1_REQUIRED fallback, for as long as traffic keeps the fallback socket alive. The resent request parks its HTTP/1.1 socket in the keep-alive pool on completion. find_in (HTTPContext.rs:869-875) then hands that socket to any later request that offers H1OrH2, and each use re-parks it. Fix: keep a socket opened by retry_over_http1 out of the pool, or make the pool prefer an idle h2 session over an h1 socket for H1OrH2 requests, so only requests pinned to h1 take it. The PR asks whether this is acceptable; with the 5-minute idle default (HTTPContext.rs:781) and steady traffic the socket never expires.
Why this was flagged
A server answers one request with RST_STREAM(HTTP_1_1_REQUIRED) while the h2 flag is on. ClientSession.rs:1218-1219 calls retry_over_http1, lib.rs:2199-2204, which pins the request and restarts it over a new HTTP/1.1 TLS connection; on completion that socket is released to the keep-alive pool like any h1 socket. A later request without protocol calls alpn_offer() -> H1OrH2; connect (HTTPContext.rs:1124) asks existing_socket, and find_in returns the first slot that passes HTTPContext.rs:869-875, where an h1 socket is accepted for H1OrH2. Each such use re-parks the socket and re-arms the 5-minute timer (HTTPContext.rs:779-783), so under continuous traffic the origin stays on HTTP/1.1 for one request at a time, with no multiplexing, until the socket is idle for 5 minutes. On the base branch only an explicit protocol: "http1.1" could put an h1 socket in the pool while the flag is on, so this did not occur for ordinary requests. The PR description flags this and asks if it is acceptable; no code bounds it.
Verification: The PR description's "Downsides" names it and the new test "an HTTP/1.1 connection that became idle before the h2 connection carries the next request" (test/js/web/fetch/fetch-http2-client.test.ts:2980-3009) pins it: /next is answered "200 over h1" although an idle h2 session is in the pool. find_in (HTTPContext.rs:867-875) accepts the h1 socket for any H1OrH2 caller.
There was a problem hiding this comment.
I left this as it is. The question is in the Downsides of the description, for a maintainer to decide.
- A request takes the idle HTTP/1.1 connection only when no h2 connection is in use.
connectinHTTPContext.rschecksactive_h2_sessionsbefore the pool, so concurrent requests still join an active h2 connection. - The two fixes have opposite costs. If the pool prefers h2, an origin that refuses every request with
HTTP_1_1_REQUIREDpays one refused h2 attempt for each request. If the connection of the resend stays out of the pool, each refused request pays a new TLS handshake. - Chromium goes the other way. After one
HTTP_1_1_REQUIREDit uses HTTP/1.1 for the origin (SetHTTP11Required). That needs a table per origin, which is a separate change.
If a maintainer wants the pool to keep a connection that never offered h2 away from a request that offers it, that is one flag on the pooled socket and one check in find_in. I can add it here.
…ream A GOAWAY(HTTP_1_1_REQUIRED) whose last stream id is at or above the stream says that the peer may have processed the request. An idempotent request is now sent again over HTTP/1.1. Another request rejects as before. Tests: a test takes its subprocess slot in a beforeEach hook. The wait no longer counts against the timeout of the test, so the new block can be concurrent and the file needs no longer default timeout under ASAN. Each new comment in src/ is one line.
|
About the |
There was a problem hiding this comment.
I reviewed the latest push and didn't find any bugs; the two test nits from my last pass (the ASAN-wide setDefaultTimeout and the serial describe) are addressed by moving the slot wait into a beforeEach hook. Because this commit also widens the replay to requests the peer may already have processed, and the .d.ts change is in a CODEOWNER-owned path, a human look is still worthwhile.
What was reviewed:
- The new GOAWAY arm in dispatch.rs that resends a request whose stream is at or below
last_stream_id: it is gated onMethod::is_idempotent(GET/HEAD/PUT/DELETE/OPTIONS/TRACE only), and the POST variant of that test still asserts theHTTP2GoAwayrejection. - The
status_code == 0gate indeliver_streamagainst a100 Continuebefore the reset: 1xx HEADERS leavestatus_codeat 0, so the resend path is reached as the test expects. - The
restart_from_topfactoring:h2_retries += 1stays only inretry_from_h2, andpin_to_http1is a no-op onceforced_protocolis set, soprotocol: "http2"is not overridden for coalesced waiters.
Extended reasoning...
The diff adds HTTP/1.1 fallback to the experimental HTTP/2 fetch client: a new ErrorCode constant, per-stream and per-session http1_required flags set from RST_STREAM and GOAWAY in dispatch.rs, a retry branch in deliver_stream, and can_retry_over_http1/retry_over_http1/pin_to_http1/restart_from_top in lib.rs, plus a JSDoc note and about 350 lines of tests. It touches no auth, crypto or injection surface; the sensitive part is request replay over a different protocol, which the latest commit extends to idempotent requests whose stream the GOAWAY names. The source change is small and I verified the helpers it relies on, but packages/bun-types/globals.d.ts is covered by .github/CODEOWNERS, and the author's open question about the HTTP/1.1 socket left in the keep-alive pool is a design call, so defer rather than approve.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
Problem
--experimental-http2-fetch,fetch()rejects withHTTP2StreamResetorHTTP2GoAwaywhen the server refuses a request withHTTP_1_1_REQUIRED(RFC 9113 section 7). Without the flag it returns 200. IIS and Apache httpd send this code.dispatch_frame(src/http/h2_client/dispatch.rs:507) has no arm for the code. No user reported this: tests found it.Fix
RST_STREAM(HTTP_1_1_REQUIRED)or such aGOAWAY. AGOAWAYthat names the stream marks only an idempotent request. Without a response,HTTPClient::retry_over_http1pins the request to HTTP/1.1 and restarts it.protocol: "http2"or a stream body behaves as on main.can_offer_h2refuses a pinned request. curl and Chromium also resend afterRST_STREAM. Node 26.3 rejects.test/js/web/fetch/fetch-http2-client.test.ts, 14 fail on main. Self-reviewed: 9 concerns raised, 7 addressed (Notes).Background
RST_STREAMends one stream (one request).GOAWAYends the connection and names the last stream that the server may have processed.flags.forced_protocol, the field behindprotocol.resolve_pending_h2writes it when ALPN selects HTTP/1.1.Downsides
Notes
What the change covers
RST_STREAM(HTTP_1_1_REQUIRED)before a responseHTTP2StreamReset100 Continueand the whole request bodyHTTP2StreamResetGOAWAY(HTTP_1_1_REQUIRED)with a last stream id below the streamHTTP2GoAwayGOAWAY(HTTP_1_1_REQUIRED)that names the stream of a GETHTTP2GoAwayRST_STREAM(HTTP_1_1_REQUIRED)and a gracefulGOAWAYin one readHTTP2RefusedStreamGOAWAY(HTTP_1_1_REQUIRED), last stream id 0 or 1, while 3 more requests wait forSETTINGSHTTP2GoAwayA sibling stream on the same connection is not touched. It completes over h2.
What still rejects
protocol: "http2". It has no HTTP/1.1 to go to. WithRST_STREAMplus a gracefulGOAWAYit goes to the next h2 connection, as on main.ReadableStream. The body can be consumed already.GOAWAY(HTTP_1_1_REQUIRED)names its stream:HTTP2GoAway, as on main. RFC 9113 section 6.8 says that the server may have processed a stream at or below the last stream id. An idempotent request (Method::is_idempotent) goes again, the rule thaton_closeinsrc/http/lib.rsuses for a connection that closes.session.goaway(NGHTTP2_HTTP_1_1_REQUIRED)ofnode:http2sends this frame: Node 26.3.0 names the stream that the server got. Chromium sends such a request again for every method.H3_VERSION_FALLBACK(RFC 9114 section 8.1) still rejects withHTTP3StreamReset, andon_conn_closedoes not read the code of aCONNECTION_CLOSE. The branchrobobun/e999e598/h3-version-fallback-wiphas an implementation with 9 tests. It conflicts with http(h3): use TCP when the QUIC connection to an Alt-Svc alternative cannot be established #45539 and http(h3): do not replay a non-idempotent request that was already sent #42579 and belongs on top of them. It also needs a fix inus_quic_on_resetfirst: that function answers aRESET_STREAMfrom the peer withlsquic_stream_close, which ends an upload in progress with a FIN.What other clients do
fetchRST_STREAM(HTTP_1_1_REQUIRED)ERR_HTTP2_STREAM_ERROR)GOAWAY(HTTP_1_1_REQUIRED), stream above the last idERR_HTTP2_SESSION_ERROR)GOAWAY(HTTP_1_1_REQUIRED)that names the streamERR_HTTP2_SESSION_ERROR)lib/multi.c:1858-1877setsdata->state.http_neg, andCurl_pretransferis the only place that resets it.SpdySession::OnRstStreamandSpdySession::OnGoAwayinnet/spdy/spdy_session.ccdrain the whole session withERR_HTTP_1_1_REQUIRED, andDoDrainSessioncallsSetHTTP11Requiredfor the origin.HttpNetworkTransaction::HandleHttp11Requiredsends the request again.H2_ERR_HTTP_1_1_REQUIREDwhen a request needs a TLS renegotiation (modules/http2/h2_headers.c).Costs, measured
Release builds of 9b4284f with and without this change:
dispatch::parse_frames+159,ClientSession::on_data+52,ClientSession::drain_pending+22,HTTPClient::resolve_pending_h2+16,HTTPClient::first_call::<true>+2,ClientSession::maybe_release+1. No other symbol changes size. The.textsection grows by 256 bytes (81,904,485 to 81,904,741), and the stripped binary by one 4 KiB page (107,493,088 to 107,497,184).Stream184 bytes, h2ClientSession464,HTTPClient1680,Flags20.ClientSession::attach, before and after (gdbstepi).RST_STREAMarm, theGOAWAYarm, the arm ofdeliver_streamfor a failed stream, the arm ofdrain_pendingfor a request that finds no room, and the HTTP/1.1 arm ofresolve_pending_h2.restart_from_top, the body of the existingREFUSED_STREAMretry.The connection pool after a resend
The tests "after RST_STREAM, the next request tries h2 first", "after GOAWAY, the next request takes the idle HTTP/1.1 connection" and "an HTTP/1.1 connection that became idle before the h2 connection carries the next request" hold the current behaviour:
RST_STREAM, the h2 connection goes to the pool first. The next request takes it and tries h2 again.GOAWAY, the h2 connection is closed. The next request takes the idle HTTP/1.1 connection.The pool gives a request the first idle connection that can carry it (
find_ininsrc/http/HTTPContext.rs). Before this PR onlyprotocol: "http1.1"in user code could put such a connection in the pool while the flag is on.Overlap with open PRs
src/http/lib.rs. Both PRs addrestart_from_topwith the same body. The resolution keepsh3_upgrade_failedof that PR next to it.Self-review
1xxbefore the reset, and for aGOAWAYthat names the stream. The test for waiting requests now waits for its precondition. The comparison with curl, Chromium and Node is per frame.REFUSED_STREAMarm and changes no behaviour.Upgradeheader over h2. It is a different mechanism and not part of this change.Tests
test/js/web/fetch/fetch-http2-client.test.ts: 107 pass on the debug build, 7 runs of 7. Of the 19 new tests, 14 fail and 5 pass on a release build of main. The 5 hold behaviour that does not change.fetch-http2-adversarial.test.ts(30),fetch-session.test.ts(34),fetch-http3-client.test.ts(67),test/js/bun/http/serve-http2.test.ts(93).spawnCappedintobeforeEachandafterEach. Under ASAN the file lets 4 run at a time, and a test waited for its slot inside its own timeout: up to 2.6 s of the 5 s on a 12-core machine, where 2 of 4 runs of the file from main failed, each time in another test. The wait is now in the hook, so the new block can bedescribe.concurrenttoo.[human-review] gate passed · iteration 1 · 7 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