http: gate fetch TCP keepalive on the existing keepalive option#30640
Conversation
The 60s keepalive added in b9c757b mirrors undici's default. Point the comment at the exact undici call site (lib/core/connect.js, nodejs/undici#1904) so the provenance of the value is traceable.
Mirror undici's `options.keepAlive` guard in `buildConnector`: when
`fetch(url, { keepalive: false })` is used (which already disables HTTP
connection reuse), also skip the SO_KEEPALIVE setsockopt instead of
applying it unconditionally.
`node:http`/`node:https` already forward `agent.keepAlive` to fetch's
`keepalive` option in `_http_client.ts`, and both `http.globalAgent` and
`https.globalAgent` are constructed with `{ keepAlive: true }` (matching
Node 19+ defaults), so requests through the global agents get TCP
keepalive while `agent: false` or a fresh `new Agent()` (whose
`keepAlive` defaults to `false`) skip it — same as Node.
|
Updated 4:53 PM PT - May 13th, 2026
❌ @cirospaciari, your commit b1cc618 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 30640That installs a local version of the PR into your bun-30640 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
@robobun adopt |
|
✅ Adopted & verified — all 4 cases in CI #54099: the diff's own test file passes on debian x64-asan. The red lanes are pre-existing flakes that also hit other unrelated PRs this week:
All three pass locally with this change applied; none go through the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward gate on an existing flag, matching undici's options.keepAlive guard, with thorough Linux-only test coverage.
Extended reasoning...
Overview
This PR wraps the recently-added socket.setKeepAlive(true, 60) call in onOpen (src/http/http.zig) with a guard on the pre-existing client.flags.disable_keepalive flag. The net functional change is a single if condition. The rest is a refactor of fetch-tcp-keepalive.test.ts to extract a probeClientSocket helper and add three new cases covering fetch({ keepalive: false }), node:http with agent: false, and node:http with globalAgent.
Security risks
None. The change only skips a setsockopt(SO_KEEPALIVE) call when the caller has explicitly opted out of keepalive. Default behavior (the path used by bun install, plain fetch(), and http.globalAgent) is unchanged. No new inputs are parsed and no new syscalls are introduced.
Level of scrutiny
Low. disable_keepalive is an established field on Flags (http.zig:644) already used to gate HTTP connection reuse and the Connection header throughout this file, and I confirmed _http_client.ts:298-302 forwards agent.keepAlive → fetch keepalive exactly as the comment and PR description claim. The semantics — conflating HTTP-level keepalive with TCP SO_KEEPALIVE — are an intentional mirror of undici's buildConnector (linked in the comment), not an accidental conflation.
Other factors
Tests are well-constructed: each case spins up a fresh server on port 0 so the connection pool can't reuse a socket from a prior test, and assertions check both the positive (02) and negative (00) timer states. No outstanding reviewer comments and no bugs flagged by the bug-hunting system.
### What does this PR do? Ports the TCP keepalive call from `src/http/http.zig` `onOpen` (added in #30627, gated in #30640) into the Rust `on_open` in `src/http/lib.rs`. The Rust HTTP client landed in #30412 from a branch cut before #30627 merged, so the `set_keepalive` call was never carried over. This is the cause of `test/js/web/fetch/fetch-tcp-keepalive.test.ts` failing on every Linux test job since #30412 merged. The test reads `/proc/self/net/tcp` and expects `timer_active=02` (keepalive armed) but gets `00` because the Rust client never calls `set_keepalive`. Build #54331 (and every Linux job since): 2 pass / 2 fail; build #54099 (#30640's PR, Zig build): 4 pass / 0 fail; build #54202 (#30412's PR): test file not in tree, so it never ran. The change is the same shape as the Zig reference: after `self.set_timeout(socket)`, guarded by `!self.flags.disable_keepalive` so `fetch(url, { keepalive: false })` and `node:http` non-keepalive agents skip it (matching undici's `buildConnector`). `set_keep_alive` already exists on `NewSocketHandler` (`src/uws_sys/socket.rs:466`) and the `disable_keepalive` flag is already wired through. ### How did you verify your code works? I have not built or run this — opening it directly per request so CI validates. The covering test is `test/js/web/fetch/fetch-tcp-keepalive.test.ts` (Linux-only), which is the test currently failing on every PR.
3 participants
Includes #30637 (undici source link for the 60s default).
Mirror undici's
options.keepAliveguard inbuildConnector: whenfetch(url, { keepalive: false })is used (which already disables HTTP connection reuse), also skip theSO_KEEPALIVEsetsockoptinstead of applying it unconditionally.node:http/node:httpsdefaults (no changes needed)ClientRequestalready forwardsagent.keepAliveto fetch'skeepalivein_http_client.ts, and both global agents are constructed with{ keepAlive: true, scheduling: "lifo", timeout: 5000 }— matching Node 19+:agent.keepAliveSO_KEEPALIVEhttp.globalAgent/https.globalAgenttruenew Agent()/agent: falsefalseHow did you verify your code works?
test/js/web/fetch/fetch-tcp-keepalive.test.ts(Linux-only, reads/proc/self/net/tcp) extended with cases forkeepalive: false,node:httpagent: false, andnode:httpglobalAgent.