node:http: apply the server's idleTimeout at accept so headersTimeout > ~13s is honored

[robobun]

What

node:http Server: any headersTimeout above ~13s was silently capped. A connection that sent a partial request head (or nothing at all) was closed with a bare FIN at ~12s regardless of headersTimeout; values below the cap worked, and keep-alive idle between requests was not affected.

Repro

import http from "node:http";
import net from "node:net";
const server = http.createServer((req, res) => res.end("ok"));
server.headersTimeout = 40000;
server.listen(0, "127.0.0.1", () => {
  const s = net.connect(server.address().port, "127.0.0.1", () => s.write("GET /o"));
  const t0 = Date.now();
  s.on("close", () => console.log("closed after", Date.now() - t0, "ms"));
});

Before: closed after 12008 ms (bare FIN). Node v26.3.0 holds the connection to ~40s then answers 408.

Cause

HttpResponseData::idleTimeout is compiled in as 10 and onOpen calls resetTimeout() with that value before any request bytes arrive. The server's configured idleTimeout (0 for node:http) is only pushed per request, once a head is parsed (on_node_http_request / prepare_js_request_context). So the uWS 10s default governs the pre-request window (usockets' 4s sweep granularity puts the reap at ~12s wall clock), preempting the checkConnections sweep that enforces headersTimeout.

Fix

Store the configured idleTimeout on HttpContextData and seed each accepted socket from it in onOpen, so the same value governs the pre-request window that governs an in-flight request. node:http's idleTimeout: 0 disables the uWS window and the checkConnections sweep owns the deadline (408 + 'clientError' with ERR_HTTP_REQUEST_TIMEOUT). A Bun.serve idleTimeout now also applies from accept; the default (10) is unchanged, so Bun.serve without the option still reaps a stalled pre-request connection at ~10s.

Verification

New case in test/js/node/http/node-http-server-timeouts.test.ts: partial-head connection with headersTimeout = 15000 survives past 13s and is then reaped by the sweep with ERR_HTTP_REQUEST_TIMEOUT. Fails on the previous build at ~12s with a bare FIN.

Note: #33061 is a larger reimplementation of headers/request timeout enforcement inside uWS itself; this change is the minimal fix on top of the existing checkConnections sweep on main.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 16 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 30d418b0-642b-46a7-ac0b-7d5e19d84991

📥 Commits

Reviewing files that changed from the base of the PR and between df6c7ee and b4fe629.

📒 Files selected for processing (7)

• packages/bun-uws/src/App.h
• packages/bun-uws/src/HttpContext.h
• packages/bun-uws/src/HttpContextData.h
• src/runtime/server/mod.rs
• src/uws_sys/App.rs
• src/uws_sys/libuwsockets.cpp
• test/js/node/http/node-http-server-timeouts.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

This PR may be a duplicate of:

1. node:http: enforce server headersTimeout and requestTimeout #33061 - Both fix headersTimeout not being enforced because uWS hardcodes idleTimeout to 10s in onOpen, preempting the server's configured timeout; node:http: enforce server headersTimeout and requestTimeout #33061 is a broader reimplementation also covering requestTimeout while this PR is a minimal targeted fix

🤖 Generated with Claude Code

[claude]

Beyond the inline findings, I also checked: plain node:http (non-TLS) is still covered — onOpen fires the filter for !SSL, so the socket enters kTrackedConnections at accept and checkConnections reaps it; and Bun.serve without an idleTimeout option is unchanged (HttpContextData::idleTimeout defaults to 10, same as the old compiled-in per-socket default). The regression is scoped to the node:https pre-handshake window.

Extended reasoning...

The HTTPS pre-handshake finding is security-relevant (slowloris regression on the default node:https configuration), so this needs a human look. I confirmed the mechanism: onOpen<true> skips the filter for SSL (HttpContext.h if(!SSL)), the filter runs only in onHandshake, and checkConnections (_http_server.ts:437-447) iterates only kTrackedConnections, which is populated from the filter path. Plain HTTP and default Bun.serve were checked and are not affected.

[claude]

🔴 For node:https, seeding idleTimeout=0 at accept removes the only timer that covered the TLS pre-handshake window. The filter that enrolls a socket in kTrackedConnections fires in onHandshake for SSL (not onOpen), so a client that TCP-connects and stalls the TLS handshake is never swept by checkConnections, has no uWS idle timer, and stays open forever — a slowloris regression vs. the previous ~12s reap. Consider firing the filter (or at least stamping lastMessageStartMs and tracking the socket) at TCP accept for SSL, or arming a native handshake timeout when the context's idleTimeout is 0.

Extended reasoning...

What the bug is

This PR seeds each accepted socket's HttpResponseData::idleTimeout from HttpContextData::idleTimeout in onOpen, and set_routes() populates the context value from config.idle_timeout. node:http/node:https (src/js/node/_http_server.ts:675) pass idleTimeout: 0 to Bun.serve, so after this PR the context value is 0 and onOpen calls resetTimeout() → us_socket_timeout(s, 0) — the uWS idle timer is disabled from TCP accept.

The PR's stated invariant is that node:http's own checkConnections sweep (headersTimeout/requestTimeout) covers the pre-request window instead. That holds for plain HTTP, but not for HTTPS: for SSL sockets the filter that enrolls the connection in the sweep fires only after the TLS handshake completes, so a stalled-handshake client is never swept and never times out.

Code path

1. https.createServer() → http.Server with a tls config → Bun.serve({ idleTimeout: 0, tls, ... }) (_http_server.ts:675).
2. set_routes() (mod.rs:2101-2108) calls app.set_idle_timeout(0) → HttpContextData::idleTimeout = 0.
3. On TCP accept, us_internal_ssl_on_open dispatches HttpContext::onOpen<true> before the TLS handshake starts. onOpen now sets httpResponseData->idleTimeout = 0 and calls resetTimeout() → us_socket_timeout(s, 0), which sets s->timeout = 255 (disabled).
4. For SSL, onOpen skips the filter (if constexpr (!SSL) at HttpContext.h:224); the filter runs only in onHandshake (HttpContext.h:177-180), i.e. after the handshake completes.
5. The filter is what invokes on_connection_callback (server_body.rs:3768-3770 explicitly notes "for TLS, when its handshake completes"), which is what constructs NodeHTTPServerSocket and adds it to kTrackedConnections (_http_server.ts:1580).
6. checkConnections() (_http_server.ts:437-454) iterates only this[kTrackedConnections]; a pre-handshake socket is never in it.
7. There is no separate handshake timeout on this path: _http_server.ts uses Bun.serve directly and does not wire tls.Server's _handshakeTimeout (that lives in net.ts's initAcceptedTLSSocket, a different code path).

Why existing code doesn't prevent it

Before this PR, onOpen armed the compiled-in HttpResponseData::idleTimeout = 10, so a stalled TLS handshake was reaped by uWS at ~12s wall-clock regardless of what the JS layer did. This PR replaces that 10 with the context's 0 for node:https, and the replacement mechanism (checkConnections) cannot see the socket until the handshake completes. TCP_DEFER_ACCEPT doesn't help: sending a single byte (a partial ClientHello) defeats it on Linux, and it doesn't exist on macOS/Windows. onOpen does stamp lastMessageStartMs on the native ext block, but nothing reads it — the sweep runs in JS over kTrackedConnections, which the socket never enters.

Step-by-step proof

1. Start https.createServer((req, res) => res.end()).listen(0) with default timeouts.
2. net.connect(port) (raw TCP, no TLS) and write one byte, e.g. "\x16", then go idle.
3. onOpen<true> fires: idleTimeout ← 0, us_socket_timeout(s, 0) disables the uWS timer; if(!SSL) skips the filter.
4. usockets feeds the byte to OpenSSL; SSL_do_handshake returns WANT_READ; onHandshake never fires.
5. checkConnections runs every 30s, iterates kTrackedConnections — this socket isn't in it.
6. No timer, no sweep, no handshake timeout → the fd stays open indefinitely. Repeat with N sockets for a slowloris fd exhaustion.

Before this PR, step 3 armed a 10s timer and the socket closed at ~12s.

Impact

Security-relevant DoS regression on the default node:https server configuration introduced by this PR. Users have no knob to opt back in — idleTimeout: 0 is hardcoded internally for node:http compat, and server.headersTimeout/requestTimeout cannot reach pre-handshake sockets. Per REVIEW.md's security guidance ("security checks fail closed and cover every path to the protected effect"), removing the reap without a replacement covering the SSL pre-handshake path should block.

Fix

Either (a) fire the filter (+1) for SSL in onOpen too so NodeHTTPServerSocket is created and tracked from TCP accept (Node itself emits 'connection' and starts the parser timing at accept, before 'secureConnection'), or (b) when httpContextData->idleTimeout == 0 and SSL, arm a nonzero native timeout in onOpen that onHandshake clears — a native equivalent of Node's tls.Server handshakeTimeout (default 120s). Option (a) also fixes checkConnections semantics for HTTPS more generally.

[claude]

🟡 This test necessarily runs ≥15s serial (it must survive past the old ~12s reap and then hit the 15s headersTimeout), which pushes the file well over the ~10s default-lane budget REVIEW.md scrutinizes, and it passes a per-test 30000 timeout that test/CLAUDE.md:120 says not to set. The workload genuinely can't be shrunk here (that's the whole point of the fix), so consider marking it test.concurrent — it's fully isolated on its own port: 0 server and client socket — and dropping the third test(..., 30000) argument in favor of the harness default.

Extended reasoning...

What this is

The new test "headersTimeout above the old ~12s cap is honored" is a legitimate outlier: to prove the fix works it must keep a partial-head connection alive past the old ~12s uWS reap window (10s compiled-in idleTimeout + usockets' 4s sweep granularity) and then wait for the configured headersTimeout = 15000 sweep to fire. That's ≥15.2s of wall-clock per run, and the test also passes 30000 as a per-test timeout.

Why it gets flagged

Two repo rules apply directly:

• test/CLAUDE.md:120 — "CRITICAL: Do not set a timeout on tests. Bun already has timeouts." None of the other tests in this file set a per-test timeout; this one does at line 199 (}, 30000);).
• REVIEW.md (Tests reviewers reject → harness conventions) — "Keep tests fast (~1s per test; debug+ASAN runs 10-100x slower); a new file over ~10s on the default lane gets scrutinized for test.concurrent and staying serial needs a stated reason. A correct but slow test still gets changes-requested." and "Don't raise per-test timeouts to make a slow test pass; shrink the workload."

The other five tests in this file each finish in well under a second; this one addition dominates the file's serial runtime by more than an order of magnitude.

Step-by-step

1. server.headersTimeout = 15000, connectionsCheckingInterval: 200 → the sweep reaps at ~15.0–15.2s.
2. setTimeout(() => markStillOpen(true), 13000) is the fail-before witness — the test cannot resolve stillOpenAt13s before 13s of wall clock have elapsed.
3. expect(await stillOpenAt13s).toBe(true) blocks until (2) fires.
4. await closed then blocks until the sweep at ≥15s.
5. Total serial wall-clock: ≥15.2s. With the test(...) form (not test.concurrent), the entire file waits on it.

Why the workload can't just be shrunk

REVIEW.md's usual answer is "shrink the workload," but here the bug being fixed is specifically that the compiled-in 10s default (reaped at ~12s wall clock by usockets' 4s-granularity sweep) preempted any headersTimeout above ~13s. A test with headersTimeout < 12s would pass on the unfixed build. So this is a genuine outlier, not a lazily-slow test.

Suggested change

• Switch to test.concurrent("headersTimeout above the old ~12s cap is honored", async () => { ... }). The test is fully isolated: own http.createServer on port: 0, own net.connect client, no shared globals, no ordering dependency on the other cases in the describe. Running it concurrently overlaps its 15s wait with the rest of the file (and with other concurrent tests in the lane) instead of adding it serially.
• Drop the , 30000 third argument. The harness default timeout already covers a ~15s wall-clock wait (this is timer-driven idle time, not CPU work, so the debug/ASAN 10–100× multiplier does not apply). If it turns out the default is too tight on a specific CI lane, that's the point to add a per-test override with a comment — root CLAUDE.md permits "a per-test timeout only for the rare outlier," but test/CLAUDE.md's stricter rule is what reviewers will cite first.

Minor: the setTimeout(..., 13000) marker is a >50ms literal sleep, which REVIEW.md says needs a comment naming why no observable signal exists. The block comment above the test already explains it as the fail-before witness, so this is likely fine as-is; if you want to be explicit, a one-line // no observable signal for 'still open at 13s' — the old bug closed with a bare FIN next to the setTimeout would satisfy the letter of the rule.

[robobun]

^{Updated 4:53 AM PT - Jul 25th, 2026}

⏳ @robobun, your commit b4fe629 is still building in Build #80618, but has 3 failures so far (All Failures):

• test/js/node/test/parallel/test-http-server-close-idle.js - timeout on 🐧 13 x64
• test/js/node/test/parallel/test-https-server-close-idle.js - timeout on 🐧 13 x64
• test/js/node/test/parallel/test-http-socket-error-listeners.js - timeout on 🐧 13 x64
• test/js/node/test/parallel/test-http-socket-error-listeners.js - timeout on 🐧 25.04 x64