fetch: honour Connection: close on non-2xx responses and HTTP/1.0 defaults in the keep-alive pool - #35545
fetch: honour Connection: close on non-2xx responses and HTTP/1.0 defaults in the keep-alive pool#35545robobun wants to merge 5 commits into
Conversation
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
|
Looked at the four suggested issues; none are a clear enough match to auto-close:
Leaving them unlinked. |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #31464. That PR parses |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline finding, I verified the PR's h2/h3 claim: apply_multiplexed_headers at src/http/lib.rs:386 does overwrite allow_keepalive = true after handle_response_metadata, so multiplexed pooling is unaffected by the new minor_version == 0 default.
Extended reasoning...
Confirmed the h2/h3 path builds a synthetic response with minor_version: 0 (line 373) but unconditionally sets allow_keepalive = true at line 386 after the header loop, so the new HTTP/1.0 default does not regress h2/h3 connection reuse. The CONNECT-tunnel regression in the inline comment is the one path where the flag sticks.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…aults The HTTP client only applied the response's Connection header when the status code was 2xx, so a 3xx/4xx/5xx response carrying Connection: close was returned to the keep-alive pool and the next request to that origin was written onto a socket the server declared closed. Origins commonly send close on error responses and then actually close, so the reused connection races the FIN. Separately, HTTP/1.0 responses were treated as persistent by default. RFC 9112 9.3: an HTTP/1.0 response is non-persistent unless it carries an explicit Connection: keep-alive. Drop the status-code guard on the Connection header, and set allow_keepalive = false for HTTP/1.0 responses before the header loop so an explicit keep-alive can turn it back on.
handle_response_metadata runs once for the proxy's CONNECT reply and again for the origin response inside the tunnel, with no state reset in between. An HTTP/1.0 200 Connection Established from the proxy (Squid, tinyproxy, Apache mod_proxy_connect) would latch allow_keepalive=false and the origin's implicit HTTP/1.1 keep-alive could not clear it, so the tunnel was closed instead of pooled after every request. Gate the default on !(proxy_tunneling && proxy_tunnel.is_none()), mirroring the CONNECT exemption on the Content-Length and Transfer-Encoding arms. Add a regression test that counts CONNECTs through a raw HTTP/1.0 proxy.
9a743bd to
ff67ffe
Compare
There was a problem hiding this comment.
Beyond the inline nit: re-checked the CONNECT gate now in place (matches the Content-Length/Transfer-Encoding exemption predicate) and the new proxy regression test covers it. Also verified h2/h3 pooling is unaffected — apply_multiplexed_headers synthesizes minor_version: 0 but overwrites allow_keepalive = true immediately after handle_response_metadata returns (lib.rs:386). The now-unguarded Connection header arm doesn't regress CONNECT-200 behavior since 200 was already inside the old 2xx gate.
Extended reasoning...
Re-review after the CONNECT-proxy fix landed. The src/ change is small and spec-correct; the remaining inline finding is a cosmetic comment-ordering nit in the test file. Deferring rather than approving because this is core fetch() connection-pool logic with broad blast radius, and CI (#80237) is still building.
Keeps the 'Negative contract for the gate above' comment adjacent to the test it refers to.
There was a problem hiding this comment.
LGTM — the CONNECT gate, comment cleanup, and test ordering from earlier rounds are all addressed.
What was reviewed:
- Traced the new
minor_version == 0default through the CONNECT-reply path (now correctly exempted via the same predicate as the Content-Length/Transfer-Encoding arms) and the h2/h3 path (apply_multiplexed_headersoverwritesallow_keepalive = trueat lib.rs:386, so the syntheticminor_version: 0there is harmless). - Checked that dropping the 2xx guard on the Connection header doesn't regress the CONNECT flow: 200 CONNECT was already inside the old range, and non-200 CONNECT sets
disable_keepalive = trueunconditionally. - New tests cover the matrix (close on 200/404/503/302, HTTP/1.0 with and without explicit keep-alive, HTTP/1.1 implicit keep-alive as negative control) plus the HTTP/1.0-CONNECT-proxy regression guard; both follow the file's subprocess pattern and assert exact per-connection ids.
Extended reasoning...
Overview
Two-file change: src/http/lib.rs (~17 net lines in handle_response_metadata) and two new tests in test/js/web/fetch/fetch-keepalive.test.ts (~150 lines). The production change (1) removes the status_code >= 200 && <= 299 guard around the Connection header arm so close/keep-alive apply on any status, and (2) adds an HTTP/1.0 non-persistent default (allow_keepalive = false when minor_version == 0), gated to skip the proxy CONNECT reply.
Security risks
None introduced. The change makes connection reuse more conservative, closing sockets that were previously (incorrectly) pooled. There is no new parsing, no new user-controlled input path, and the HTTP/1.0 default fails closed. The one potential regression surface — a CONNECT reply's HTTP version poisoning tunnel pooling — was caught in the first review round and is now both gated and covered by a dedicated regression test.
Level of scrutiny
Medium. This is hot-path HTTP client code, but the change is small, mechanical, and directly implements RFC 9112 §9.3/§9.6. I verified the three paths that could interact with the new logic:
- h2/h3 (
apply_multiplexed_headers, lib.rs:367-386): builds a synthetic response withminor_version: 0, but unconditionally setsallow_keepalive = trueimmediately afterhandle_response_metadatareturns, so the new default is a no-op there. - CONNECT reply: the
!(proxy_tunneling && proxy_tunnel.is_none())gate mirrors the existing Content-Length / Transfer-Encoding exemptions at lib.rs:4785-4790 and 4848-4853. For the un-gated Connection-header arm, a 200 CONNECT withConnection: closebehaved identically before (200 was inside the old guard), and a non-200 CONNECT already forcesdisable_keepalive = trueat lib.rs:4979. - Redirects:
state.reset()on the redirect path restoresallow_keepalive's default oftruefor the next hop, so aConnection: closeon a 302 correctly closes only that hop's socket.
Other factors
All three prior review threads (CONNECT gate, two comment-cop blocks, test-pair ordering) are resolved and reflected in the current diff. The evidence block shows the new test failing on the unpatched debug build and passing with the fix; the release-without-fix pass is expected (release canary predates the test). Test quality is good: subprocess isolation for pool state, exact per-request connection-id assertions, negative-control rows (1.0 200 keep-alive → 1 conn, 1.1 404 implicit → 1 conn) that would catch over-eager closing, and the CONNECT-proxy test locks in the exemption. This is complementary to (not a duplicate of) #31464, which the author already noted.
|
Diff is ready. CI build #81220 finished with a single unrelated failure: The earlier Locally the new test fails on main's |
A completed chunked ReadableStream body sets request_stage=Done, so the do_redirect pool/close decision applies. With Connection: close ignored on 3xx (the bug this PR fixes) the closing socket was pooled and immediately reused for the follow-up GET, which either hangs (origin lingers per RFC 9112 9.6) or gets delivered twice (origin closes, GET hits FIN, idempotent retry fires).
|
This also fixes a downstream symptom: a Two failure modes depending on what the origin does after
node/undici open a fresh connection in both cases. Added a test in 000f112 that reproduces the linger variant: without this PR's src change |
There was a problem hiding this comment.
LGTM — the CONNECT-proxy regression I flagged earlier is fixed and now guarded by a test, and the test-ordering nit is resolved.
What was reviewed:
- Verified the
minor_version == 0default is overwritten byapply_multiplexed_headers(line 386) so h2/h3 pooling is unaffected, and that the CONNECT exemption predicate matches the existing Content-Length/Transfer-Encoding gates. - Checked that dropping the 2xx guard on the Connection header doesn't change CONNECT-reply behaviour (200 was already inside the old guard) and that a non-200 CONNECT hits
is_proxy_connect_failureregardless. - The 8-row matrix test covers both directions (rows that must open 3 connections and rows that must reuse 1), plus the CONNECT-proxy and 303-redirect-after-streaming-POST regression tests.
Extended reasoning...
Overview
The PR changes handle_response_metadata in src/http/lib.rs (net +4 lines): it removes the status_code >= 200 && status_code <= 299 guard around the Connection header handling so close/keep-alive apply on any status, and adds an HTTP/1.0 non-persistent default (allow_keepalive = false when minor_version == 0) gated on !(proxy_tunneling && proxy_tunnel.is_none()) to skip the CONNECT reply. Three new subprocess tests in fetch-keepalive.test.ts cover an 8-row status×version matrix, the CONNECT-proxy exemption, and a 303-with-close redirect after a streaming POST.
Security risks
None introduced. The change is conservative in direction: it causes more connections to be closed rather than pooled, so the failure mode is extra TCP/TLS handshakes rather than reuse of a desynchronised or half-closed socket. The 303 test in fact demonstrates a request-smuggling-adjacent symptom (follow-up GET pipelined into a closing connection) that this fix eliminates.
Level of scrutiny
Moderate. src/http/lib.rs is production-critical, but the change is a small, spec-driven correction (RFC 9112 §9.3/§9.6) to a single header-processing branch. I traced the three interaction points that could regress: h2/h3 (apply_multiplexed_headers unconditionally rewrites allow_keepalive = true after calling this function, so the synthetic minor_version: 0 is harmless), CONNECT tunneling (the added exemption uses the exact predicate already used by the Content-Length arm and the start_proxy_handshake dispatch), and the Connection header on CONNECT replies (200 was already inside the old 2xx guard, so no behaviour change there; non-200 CONNECT sets is_proxy_connect_failure and never pools anyway).
Other factors
All three of my earlier review comments were addressed with follow-up commits and regression tests. The new tests follow repo conventions (subprocess isolation with a stated reason, port: 0, concurrent stdout/stderr/exited drain, combined-object assertions, bunEnv spread). The matrix test includes negative-contract rows (1.0 200 keep-alive → 1 connection, 1.1 404 implicit keep-alive → 1 connection) so over-eager closing would also fail. The CI binary-size failure is baseline drift unrelated to this +4-line src change. The evidence block shows the test fails on main and passes with the fix.
|
Closing this since #36370 (http: honor Connection: close on non-2xx responses) merged and covers the same ground. Thank you @robobun for the PR — if there's a piece of this that #36370 didn't pick up, please say so and we'll take another look. (This comment was written by Claude, on behalf of the Bun team.) |
…p-alive (#37530) `fetch()` returns the connection that carried an HTTP/1.0 response to the keep-alive pool as long as the response had a Content-Length, whether or not the server said `Connection: keep-alive`. ### Repro Raw server that answers every request with `HTTP/1.0 200 OK\r\nContent-Length: 2\r\n\r\nok` and leaves the socket open, counting accepted connections: ```js import net from "node:net"; let connections = 0; const server = net.createServer(sock => { connections++; sock.on("data", () => sock.write("HTTP/1.0 200 OK\r\nContent-Length: 2\r\n\r\nok")); }).listen(0, "127.0.0.1", async () => { const url = `http://127.0.0.1:${server.address().port}/`; for (let i = 0; i < 4; i++) await (await fetch(url)).text(); console.log(connections); // bun 1.4.0 and main: 1, node (http.Agent keepAlive) and undici: 4 process.exit(); }); ``` Adding `Connection: keep-alive` to the response is what should make this print 1 (and does, in bun and node alike). Real HTTP/1.0 servers (`python -m http.server`, and most other respond-and-close servers) close the socket right after the response, so in practice the pooled socket races the server's FIN: the next fetch to that origin is written onto it and only succeeds because `on_close` retries idempotent requests on reused connections. A POST loses that race with ECONNRESET. Since #37451 the hop of a followed redirect takes the 3xx's connection back out of the pool synchronously, which makes the race a certainty for an HTTP/1.0 3xx (#37522 separately gates that release on the hop being idempotent; the HTTP/1.0 default underneath is still wrong and this fix is independent of it). ### Cause `handle_response_metadata` never looks at `response.minor_version`. `allow_keepalive` starts out `true` and is only cleared by `Connection: close`, missing framing, or an upgrade, so an HTTP/1.0 response with a Content-Length falls through to HTTP/1.1's persistent default. ### Fix The `Connection` header arm now also records whether any field line carried a `keep-alive` token (`close` still clears `allow_keepalive` on the spot, so it stays sticky across lines and wins over `keep-alive`). After the header loop, an HTTP/1.0 response without such a token clears `allow_keepalive`. Why this is the right rule: RFC 9112 section 9.3 makes an HTTP/1.0 response non-persistent unless it carries `Connection: keep-alive` (that header is how HTTP/1.0 keep-alive was negotiated in the first place, and bun already sends `Connection: keep-alive` on its requests, so servers that support it will answer with it). Node's HTTP parser (`shouldKeepAlive`), undici and curl all implement the same rule; the numbers in the repro are node's. Only `Connection` is consulted: a `Keep-Alive:` parameters header on its own does not count, same as in node and curl. The check is placed after the early return for a proxy's 2xx reply to CONNECT on purpose. tinyproxy, Apache mod_proxy_connect and older Squid answer CONNECT with `HTTP/1.0 200 Connection established`; that status line describes the hop to the proxy, and since `state` is not reset between the CONNECT reply and the tunneled response, clearing `allow_keepalive` there would stop every tunnel through such a proxy from being pooled. The origin's response inside the tunnel goes through the same function later and is judged on its own version, exactly like a direct connection. The h2/h3 sessions feed a synthetic `minor_version: 0` through this function too; they already overwrite `allow_keepalive` right afterwards because HTTP/1.x persistence rules do not apply to them, and the comment there now says so. ### Tests `test/js/web/fetch/fetch-keepalive.test.ts`: - The `Connection: close` table is generalised to status line + header lines + expected connection count and gains HTTP/1.0 rows: no Connection header (200 and 404), `Keep-Alive:` header alone, `Connection: keep-alive` (both spellings, still pooled), and `keep-alive` combined with `close` on one line or across two lines in either order (still not pooled). HTTP/1.1 without a Connection header is pinned as still pooled. - A CONNECT-proxy table: `HTTP/1.0 200` CONNECT reply with an HTTP/1.1 origin still pools the tunnel, so does an HTTP/1.0 origin that says keep-alive, and an HTTP/1.0 origin without it makes each fetch open a new tunnel. - The redirect table gains an HTTP/1.0 302 (hop and later fetches dial again, like the `Connection: close` row) and an HTTP/1.0 302 with `Connection: keep-alive` (one connection throughout). Without the `src/http/lib.rs` change, main fails exactly the five rows that describe the bug (three direct, the HTTP/1.0-origin tunnel row and the HTTP/1.0 302 row, each with one connection instead of 4 / 3 / 5); with it all 36 tests in the file pass. `fetch-redirect`, `fetch-connection-header`, `fetch-url-after-redirect`, `fetch-proxy-connect-tunnel-split-envelope`, `fetch-http2-client`, `fetch-http3-client`, `client-fetch`, `proxy.test.ts`, `proxy-stress-protocol` (which has an HTTP/1.0-origin-through-tunnel group), `proxy-stress-matrix` and `proxy-stress-concurrent` pass on the debug build as well. Note: #37522 rewrites the redirect section of the same test file; whichever of the two lands second needs its two HTTP/1.0 redirect rows moved over. The `lib.rs` changes do not overlap. This is the half of #35545 that #36370 did not pick up.
2 participants
What
fetch()was returning a connection to the keep-alive pool after a response that saidConnection: close, as long as the status code was not 2xx. It was also pooling HTTP/1.0 responses that carried noConnection: keep-alive.RFC 9112 §9.6: once a
closeconnection option is received the client MUST NOT send further requests on that connection. RFC 9112 §9.3: an HTTP/1.0 response is non-persistent by default. Node/undici open a fresh connection in every one of these cases.Repro
Before (bun 1.4.0-canary):
200 closeopens 3 connections, the other three rows all printconnections: 1(the closed/non-persistent socket was reused). Node prints 3 for every row. Origins that actually close after an error response (nginx 4xx/5xx, LB drains, S3) turn the reuse into sporadic ECONNRESET on the follow-up request.Cause
handle_response_metadataguarded theConnectionheader withresponse.status_code >= 200 && response.status_code <= 299, socloseon a 3xx/4xx/5xx was ignored andallow_keepalivestayed at its default oftrue. There was nominor_versioncheck at all, so HTTP/1.0 responses also fell through to the HTTP/1.1 persistent default.Fix
Connectionheader branch;close/keep-alivenow apply regardless of status code.allow_keepalive = falsewhenresponse.minor_version == 0; an explicitConnection: keep-alivein the loop can turn it back on. h2/h3 pass through this function with a syntheticminor_version: 0but already overwriteallow_keepalive = trueafterwards, so multiplexed pooling is unaffected.Verification
bun bd test test/js/web/fetch/fetch-keepalive.test.tspasses (6/6). The new test fails on an unpatched build withconnections: 1for the 404/503/302+close and HTTP/1.0 rows and passes with the fix.fetch-redirect.test.ts,fetch-connection-header.test.tsandfetch-http2-client.test.tsare unchanged.[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