http: honor Connection: close on non-2xx responses by robobun · Pull Request #36370 · oven-sh/bun

robobun · 2026-07-29T12:43:27Z

Fixes the flake in test/js/bun/http/proxy-stress-protocol.test.ts early reply during upload, seen on debian 13 x64-asan:

error: The socket connection was closed unexpectedly. For more information, pass `verbose: true` in the second argument to fetch()
  path: "http://localhost:45859/",
 errno: 0,
  code: "ECONNRESET"
✗ early reply during upload > https-proxy → http-origin, 500 after 1KB of a 1MB upload

Cause

The response Connection header was only applied when the status code was 2xx:

h if h == hash_header_const(b"Connection") => {
    if response.status_code >= 200 && response.status_code <= 299 {
        if ...eql...(header.value(), b"close") {
            self.state.flags.allow_keepalive = false;
        }
        ...

That guard dates from 2b45c8d when keep-alive defaulted off and this block only handled Connection: keep-alive (opt in on 2xx success). Once the default flipped to keep-alive on, the same guard started meaning "ignore Connection: close on 4xx/5xx", so a 413/500 carrying Connection: close left allow_keepalive true and the socket was released into the pool.

In the flaky test: origin replies 500 / Connection: close after 1KB of a 1MB upload and ends; the proxy relays the reply and the FIN. bun parses the reply, pools the socket (because 500 skipped the close check, and the 1MB body already fit in the loopback send buffer so request_side_drained was true), and a concurrent early-reply test sharing the same sharedHttpsProxy picks the about-to-die socket from the pool. Absolute-form proxy connections are keyed on the proxy URL alone, so the three https-proxy → http-origin variants all draw from the same pool entry.

With BUN_DEBUG_fetch=1 under concurrency:

[fetch] progressUpdate true
[fetch] release socket        <- pooled despite Connection: close on a 500
[fetch] done
...
[fetch] Closed  http://localhost:34835/   <- next fetch on that pooled socket

Fix

Honor Connection: close for every status code (RFC 9112 section 9.6: "A client that receives a 'close' connection option MUST cease sending requests on that connection"). The 2xx guard is kept only for the explicit Connection: keep-alive case.

The earlier test-side workaround (keeping the origin socket open) is reverted; the original sock.end() is realistic server behavior and is now handled correctly.

Verification

Standalone stress repro (https-proxy to http-origin early-reply, keepalive: true, 24 concurrent x 300 iterations, debug+ASAN):

	before	after
origin `sock.end()` (as in the test)	91/300 ECONNRESET	0/300
`keepalive: false`	0/300	0/300

bun bd test test/js/web/fetch/fetch-keepalive.test.ts: new a {200,400,413,500} response with Connection: close is not pooled cases fail before (connections: 1 for 4xx/5xx) and pass after (connections: 4)
bun bd test test/js/bun/http/proxy-stress-protocol.test.ts: 102 pass, 20/20 iterations under ASAN
All other proxy-stress-*.test.ts files pass

The origin wrote its early reply and then immediately sock.end()'d. The proxy relays that FIN and end()s the client leg while bun is still writing ~1MB of request body; if bun's on_writable/on_close fires before the reply bytes are processed it observes a shut-down socket and fails with ConnectionClosed (surfaced as ECONNRESET, errno 0). Keep the origin socket open after writing the reply; the client closes after reading Connection: close, which tears down the proxy->origin leg. Disposal now destroys any socket still open.

coderabbitai · 2026-07-29T12:43:35Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 96f21093-6913-4f3c-a955-dcbbe0314acd

📥 Commits

Reviewing files that changed from the base of the PR and between b28d9a9 and a47c6c9.

📒 Files selected for processing (2)

src/http/lib.rs
test/js/web/fetch/fetch-keepalive.test.ts

Walkthrough

Changes

HTTP keep-alive handling

Layer / File(s)	Summary
Connection closure handling and validation `src/http/lib.rs`, `test/js/web/fetch/fetch-keepalive.test.ts`	`Connection: close` disables keep-alive regardless of response status, while `Connection: keep-alive` remains limited to 2xx responses. Parameterized fetch tests verify separate connections for statuses 200, 400, 413, and 500.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: honoring Connection: close on non-2xx responses.
Description check	✅ Passed	The description covers the change and verification, though it uses custom headings instead of the template's exact section names.
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.}

The response Connection header was only applied when the status code was 2xx. That guard dates from when keep-alive defaulted off and the block only handled "Connection: keep-alive"; once the default flipped to keep-alive on, the guard started meaning "ignore Connection: close on 4xx/5xx", so a 413/500 with Connection: close was pooled. When the server (or a relaying proxy) then closed the socket, a concurrent fetch that picked it from the pool failed with ECONNRESET. Honor "Connection: close" for every status code (RFC 9112 section 9.6); keep the 2xx guard only for the explicit "keep-alive" case. This reverts the previous test-side workaround in proxy-stress-protocol.test.ts; the original sock.end() behavior is realistic and is now handled correctly.

coderabbitai

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 `@src/http/lib.rs`:
- Around line 4907-4918: Update the Connection-header handling around the
existing allow_keepalive assignments to parse every comma-separated token across
repeated Connection fields, rather than comparing each full field value. Trim
and case-insensitively match tokens, make any “close” token terminal so later
“keep-alive” values cannot re-enable pooling, and add coverage for
comma-separated values plus both repeated-field orderings.

🪄 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: 76f974e6-9897-42cc-8da5-b4d7621b4bb8

📥 Commits

Reviewing files that changed from the base of the PR and between e1b8e61 and 8554a06.

📒 Files selected for processing (2)

src/http/lib.rs
test/js/web/fetch/fetch-keepalive.test.ts

coderabbitai

Actionable comments posted: 2

🤖 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/web/fetch/fetch-keepalive.test.ts`:
- Line 83: Update the parameterized test declaration for the independent status
cases to use test.concurrent.each instead of test.each, preserving the existing
statuses, test name, and callback behavior.
- Around line 121-126: Update the assertion around the child process result in
the fetch keepalive test so stderr is always validated, including when stdout
contains JSON. Preserve the existing JSON result and exitCode expectations, and
assert that the child process emits no unexpected stderr, consistent with
subprocesses launched through bunEnv.

🪄 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: 1d13aa23-6929-4fcb-882e-28cc58607e82

📥 Commits

Reviewing files that changed from the base of the PR and between 8554a06 and b28d9a9.

📒 Files selected for processing (1)

test/js/web/fetch/fetch-keepalive.test.ts

claude

LGTM — both earlier concerns are addressed: the test-side workaround in proxy-stress-protocol.test.ts was reverted in favor of the real runtime fix (net diff to that file is now zero, so the reply-then-close scenario stays covered), and the source comment is trimmed to the durable RFC 9112 §9.6 invariant.

What was reviewed:

Connection header handling in handle_response_metadata: close now unconditionally clears allow_keepalive; the 2xx gate remains only on the explicit keep-alive opt-in. Strictly narrows pooling — fail-safe direction.
New test.concurrent.each([200, 400, 413, 500]) case follows the existing subprocess pattern in the same file; verified it asserts distinct connections (4) rather than reuse (1).
Confirmed proxy-stress-protocol.test.ts is no longer in the diff, so the original sock.end() early-close coverage is intact.

Extended reasoning...

Overview

Two files: src/http/lib.rs moves the Connection: close check out from under the 200..=299 status guard so it applies on any status (RFC 9112 §9.6), while keeping the explicit Connection: keep-alive opt-in gated on 2xx. test/js/web/fetch/fetch-keepalive.test.ts adds a parameterized subprocess test proving a Connection: close response is not pooled for 200/400/413/500. The earlier commit that removed sock.end() from the proxy-stress test fixture has been reverted — that file has zero net diff.

Security risks

None introduced. The change strictly reduces the set of sockets returned to the keep-alive pool (never pools a socket the server said it is closing on a non-2xx). Pooling fewer connections is the safe direction; there is no path here that pools a connection that would previously have been closed.

Level of scrutiny

HTTP client connection pooling is production-critical, but the change is ~14 lines, mechanically restructures an existing conditional, and is behavior-preserving on every path except the one being fixed (non-2xx + Connection: close, which now correctly disables reuse). I traced each of the four (2xx/non-2xx × close/keep-alive) combinations against old vs new code and only the intended one changes. The PR description includes git-blame archaeology explaining why the guard existed (2b45c8d, when keep-alive defaulted off) and stress-test data (91/300 → 0/300 ECONNRESET).

Other factors

Both of my prior inline comments are resolved: (1) my concern about routing around a runtime bug by editing the test fixture was addressed by fixing the runtime and reverting the fixture change; (2) the source comment was trimmed to two lines carrying only the spec citation and asymmetry rationale. CodeRabbit's token-list-parsing note was correctly scoped out as pre-existing behavior. The comment-cop and test.concurrent.each feedback were applied in a47c6c9. No findings from the bug hunter this run.

Jarred-Sumner · 2026-07-29T14:00:02Z

@robobun in another PR, figure out why this test is slow and ifx it:


test/js/node/http/node-http-pinned-write.test.ts - 5 failing in the parallel batch (450.13s)
--
  | ✗ Content-Length (non-chunked) path delivers the exact bytes
  | test timed out
  | ✗ a second write before drain is ordered after the pending tail
  | test timed out
  | ✗ large string writes (latin1 ascii, utf8 encoding) are held by reference
  | test timed out
  | ✗ large string writes with latin1 encoding deliver the exact bytes
  | test timed out
  | ✗ a second write before drain releases the pin on the first buffer
  | test timed out
  |  

<br class="Apple-interchange-newline">

Jarred-Sumner · 2026-07-29T14:01:19Z

@robobun most likely that test should be refactored to explicitly set send/recv buffer sizes so it can avoid the 64 MB network activity saturating network in --parallel

robobun · 2026-07-29T14:23:09Z

Opened #36376 for the node-http-pinned-write.test.ts slowdown.

robobun · 2026-07-29T15:06:15Z

CI on build #85284: all 13 failing files are tagged flaky by the scraper (parallel-batch timeouts, timing assertions under ASAN, Windows install). None touch fetch-keepalive.test.ts, proxy-stress-protocol.test.ts, or the Connection-header path. node-http-pinned-write.test.ts is on the list and is addressed separately in #36376.

Diff is green for what it changes.

robobun · 2026-07-29T17:05:50Z

^{Updated 10:05 AM PT - Jul 29th, 2026}

@robobun, your commit a47c6c9 is building: #85284

…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.

robobun assigned Jarred-Sumner Jul 29, 2026

github-actions Bot added the claude label Jul 29, 2026

claude Bot reviewed Jul 29, 2026

View reviewed changes

Comment thread test/js/bun/http/proxy-stress-protocol.test.ts Outdated

github-actions Bot reviewed Jul 29, 2026

View reviewed changes

Comment thread src/http/lib.rs Outdated

robobun changed the title ~~test(proxy-stress-protocol): deflake early-reply-during-upload~~ http: honor Connection: close on non-2xx responses Jul 29, 2026

[autofix.ci] apply automated fixes

b28d9a9

coderabbitai Bot reviewed Jul 29, 2026

View reviewed changes

Comment thread src/http/lib.rs

coderabbitai Bot reviewed Jul 29, 2026

View reviewed changes

Comment thread test/js/web/fetch/fetch-keepalive.test.ts Outdated

Comment thread test/js/web/fetch/fetch-keepalive.test.ts

address review: trim comment, test.concurrent.each

a47c6c9

github-actions Bot reviewed Jul 29, 2026

View reviewed changes

Comment thread src/http/lib.rs

claude Bot reviewed Jul 29, 2026

View reviewed changes

Comment thread src/http/lib.rs Outdated

Jarred-Sumner approved these changes Jul 29, 2026

View reviewed changes

Jarred-Sumner enabled auto-merge (squash) July 29, 2026 13:51

claude Bot reviewed Jul 29, 2026

View reviewed changes

Jarred-Sumner disabled auto-merge July 29, 2026 16:07

Jarred-Sumner merged commit 6d3f97f into main Jul 29, 2026
50 of 53 checks passed

Jarred-Sumner deleted the farm/da52c690/deflake-proxy-stress-early-reply branch July 29, 2026 16:07

Jarred-Sumner mentioned this pull request Aug 1, 2026

fetch: honour Connection: close on non-2xx responses and HTTP/1.0 defaults in the keep-alive pool #35545

Closed

This was referenced Aug 2, 2026

http: parse Connection/Transfer-Encoding/Content-Encoding/Upgrade as token lists #36777

Merged

fix(anthropic): surface upstream error bodies through sidecar bridges lidge-jun/opencodex#1020

Merged

coderabbitai Bot mentioned this pull request Aug 11, 2026

http: don't pool a connection when bytes trail a bodyless response #37438

Merged

robobun mentioned this pull request Aug 11, 2026

fetch: do not pool HTTP/1.0 responses unless they say Connection: keep-alive #37530

Merged

Navigation Menu

http: honor Connection: close on non-2xx responses - #36370

http: honor Connection: close on non-2xx responses#36370
Jarred-Sumner merged 4 commits into
mainfrom
farm/da52c690/deflake-proxy-stress-early-reply

robobun commented Jul 29, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 29, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot left a comment

Uh oh!

Jarred-Sumner commented Jul 29, 2026

Uh oh!

Jarred-Sumner commented Jul 29, 2026

Uh oh!

robobun commented Jul 29, 2026

Uh oh!

robobun commented Jul 29, 2026

Uh oh!

Uh oh!

robobun commented Jul 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

robobun commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Cause

Fix

Verification

Uh oh!

coderabbitai Bot commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

Jarred-Sumner commented Jul 29, 2026

Uh oh!

Jarred-Sumner commented Jul 29, 2026

Uh oh!

robobun commented Jul 29, 2026

Uh oh!

robobun commented Jul 29, 2026

Uh oh!

Uh oh!

robobun commented Jul 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robobun commented Jul 29, 2026 •

edited

Loading

coderabbitai Bot commented Jul 29, 2026 •

edited

Loading