[robobun]

What does this PR do?

res.setTimeout(msecs) on a client-side IncomingMessage was creating a ref'd setTimeout which kept the event loop alive for the full timeout duration even after the response completed. In Node.js, IncomingMessage.prototype.setTimeout delegates to socket.setTimeout(), which uses an unref'd timer.

Repro

const http = require("http");
const agent = new http.Agent({ keepAlive: true });
http.get({ host, port, agent }, res => {
  res.setTimeout(90000);
  res.resume();
  res.on("end", () => { /* nothing left to do */ });
});

Node exits immediately after end. Bun (before) waits the full 90s.

Cause

_http_client.ts overrides res.setTimeout on the IncomingMessage with an inline implementation that called setTimeout(fn, msecs) without .unref(), without returning this, and without treating msecs === 0 as "clear the timeout".

Fix

• .unref() the timer so it doesn't keep the event loop alive
• return res (Node returns this)
• msecs <= 0 clears the existing timer instead of scheduling setTimeout(fn, 0)
• attach the callback via res.on("timeout", cb) so repeated setTimeout calls stack listeners the same way Node does

Relation to "idle keep-alive sockets block exit"

Investigated whether Bun's HTTP keep-alive pool itself refs the event loop — it doesn't. http.ClientRequest goes through native fetch; pooled sockets live on the HTTP thread's own loop and the only JS-loop ref is FetchTasklet.poll_ref, which is released when the response body finishes. Plain http.get / https.get / axios / agentkeepalive with keepAlive: true all exit immediately. The one path that produces the "process waits N seconds after the last request" symptom is this ref'd res.setTimeout timer.

How did you verify your code works?

New test in test/js/node/http/node-http.test.ts spawns a child that makes a keep-alive http.get, calls res.setTimeout(60000), consumes the body, and asserts the child exits without hitting a 2s sentinel.

• USE_SYSTEM_BUN=1 bun test → fails (BAD_RETURN + STILL_ALIVE)
• bun bd test → passes
• git stash push -- src/ && bun bd test → fails; git stash pop && bun bd test → passes

Also verified:

• timeout still fires when the server is slow to respond
• listener semantics (on, not removed by setTimeout(0)) match Node
• existing test/js/node/http/client-timeout-error.test.ts and node-http.test.ts -t setTimeout still pass

[robobun]

^{Updated 12:01 AM PT - Apr 25th, 2026}

❌ @robobun, your commit 925ef69 has 2 failures in Build #47895 (All Failures):

• test/js/node/v8/v8-date-parser.test.js - code 1 on 🍎 26 aarch64
• test/js/bun/webview/webview-chrome.test.ts - code 1 on 🍎 14 aarch64

---

🧪   To try this PR locally:

bunx bun-pr 29701

That installs a local version of the PR into your bun-29701 executable, so you can run:

bun-29701 --bun

[robobun]

Status: addressed review feedback — hoisted res.setTimeout to module scope (925ef69) so it doesn't close over ClientRequest locals. CI complete.

Repro:

http.get({ host, port, agent: new http.Agent({ keepAlive: true }) }, res => {
  res.setTimeout(60000);
  res.resume();
});

Node exits immediately after end; Bun (before) waits the full 60s because the res.setTimeout override created a ref'd timer.

CI — node-http-res-settimeout-unref.test.ts passed on every shard in every build. All failures are pre-existing cross-branch flakes unrelated to node:http:

Build 47895 (271 pass / 2 fail):

test	also failing on unrelated branches
test/js/bun/webview/webview-chrome.test.ts	#47798 #47789 #47784 #47777 #47774 #47767 #47766 #47757 #47750
test/js/node/v8/v8-date-parser.test.js (darwin 26 aarch64)	#47857 #47853 #47851 #47845 #47840

Earlier builds 47855 / 47811: bake/dev/esm, dlopen-*, prefetch, v8-heap-snapshot, next-pages/dev-server — all cross-referenced as pre-existing above.

Linked-issue check: neither #23648 (missing http.Server._handle → supertest never closes its implicit server) nor #23776 (FakeSocket.unref() no-op) is addressed by this change — both have different root causes, noted above.

[github-actions]

Found 2 issues this PR may fix:

1. IncomingMessage socket unref doesn't work #23776 - Directly addresses IncomingMessage socket unref not working, where the process hangs because ref'd resources prevent the event loop from exiting
2. Bun process hangs when using Supertestwith Express — does not exit after test completion #23648 - Process hangs when using Supertest with Express after test completion, consistent with a ref'd timer on client-side IncomingMessage keeping the event loop alive

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #23776
Fixes #23648

🤖 Generated with Claude Code

[coderabbitai]

Walkthrough

Replaced inline res.setTimeout logic with a module-scoped clientResponseSetTimeout that stores timeout handles on responses, clears previous timers, schedules timers only for msecs > 0, calls setTimeout(...).unref(), emits "timeout" on firing, and attaches provided callbacks as "timeout" listeners. Added a test ensuring the timeout does not keep the event loop alive.

Changes

Cohort / File(s)	Summary
Client response timeout src/js/node/_http_client.ts	Extracted and centralized res.setTimeout into clientResponseSetTimeout. Stores timer handle on the res using a symbol, clears existing timers when rescheduling, schedules only for msecs > 0, uses .unref(), emits "timeout" instead of invoking callbacks directly, and wires res.setTimeout to the shared function.
Event-loop liveness test test/js/node/http/node-http-res-settimeout-unref.test.ts	New test that spawns a Node script which calls res.setTimeout(60000, ...) and uses an unref'd 2s sentinel to assert the process exits after response completion, verifying the timeout timer does not keep the event loop alive.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and specifically describes the main change: unreferencing the client IncomingMessage.setTimeout timer to fix the event loop being held alive.
Description check	✅ Passed	The PR description comprehensively covers both required sections with detailed explanations of the problem, solution, reproduction case, and verification approach.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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

[claude]

LGTM — small, well-scoped Node compat fix with a clear regression test; the stderr assertion nit below is non-blocking.

Extended reasoning...

Overview

This PR touches two files: a ~20-line change to the inline res.setTimeout override in src/js/node/_http_client.ts, and a ~55-line regression test in test/js/node/http/node-http.test.ts. The fix unrefs the timer, returns res, treats msecs <= 0 as clear, and attaches the callback via res.on('timeout', cb) — all of which align with Node's IncomingMessage.prototype.setTimeout semantics.

Security risks

None. This is purely timer ref/unref and event-listener wiring on the client-side response object. No auth, crypto, parsing, or untrusted-input handling is touched.

Level of scrutiny

Low-to-moderate. The change is mechanical and mirrors the existing pattern already used a few hundred lines down in ClientRequestPrototype.setTimeout (which also .unref()s its timer). The behavior change — callback now fires via the 'timeout' event rather than being invoked directly — is strictly more Node-compatible and avoids the previous double-invocation when a user both passed a callback and listened to 'timeout'.

Other factors

The new test spawns a child process and asserts exact stdout + exit code, which would have failed before the fix (BAD_RETURN + STILL_ALIVE). The PR description documents that existing client-timeout-error.test.ts and node-http.test.ts -t setTimeout still pass. No CODEOWNERS cover these paths. The one inline nit (weak stderr assertion) is a test-convention issue only and doesn't affect the soundness of the test, since the exact-stdout and exitCode checks are already strong.

[robobun]

Re: the linked issues — tested both against this branch:

• Bun process hangs when using Supertestwith Express — does not exit after test completion #23648 (supertest + express hang): not fixed by this PR. Root cause is that Bun's http.Server never sets _handle, so supertest's if (server && server._handle) server.close(...) check is always false and the implicitly-spawned server is never closed. Separate Node-compat gap in _http_server.ts.
• IncomingMessage socket unref doesn't work #23776 (logStream.socket.unref()): not fixed by this PR. That needs FakeSocket.unref() on a client IncomingMessage to release FetchTasklet.poll_ref while the body is still streaming — native change, separate work.

This PR only covers the res.setTimeout() ref'd-timer case.

[claude]

LGTM — the stderr nit was addressed in a8fcf82, and the fix mirrors the existing .unref() pattern already used by ClientRequestPrototype.setTimeout in the same file.

Extended reasoning...

Overview

This PR touches two files: src/js/node/_http_client.ts (~15 net lines changed in the inline res.setTimeout override) and test/js/node/http/node-http.test.ts (one new regression test). The src change makes the client-side IncomingMessage.setTimeout override (1) .unref() its timer, (2) return res, (3) treat msecs <= 0 as clear-only, and (4) register the callback via res.on('timeout', cb) rather than capturing it in the closure. All four are convergence toward Node's documented socket.setTimeout semantics.

Security risks

None. This is event-loop-ref bookkeeping in the Node http client compat layer — no auth, crypto, parsing, or untrusted input handling is touched.

Level of scrutiny

Low-to-medium. The change is small, self-contained, and follows the exact pattern already used ~370 lines lower in the same file (ClientRequestPrototype.setTimeout already does setTimeout(...).unref() and return this). The behavioral deltas are all in the direction of Node compatibility, and the only way this could regress someone is if they were relying on the bug (a ref'd response timeout holding the process open), which is the symptom being fixed. No CODEOWNERS cover these paths.

Other factors

• My previous review flagged a weak expect(stderr).not.toContain('error') assertion; the author removed it in a8fcf82 and left a comment explaining why stdout + exitCode suffice (debug/ASAN startup banner on stderr). The remaining assertions (stdout === 'end:hello\\n', exitCode === 0) are strong and the test's 2s unref'd sentinel guarantees a fast, deterministic failure on regression rather than a 60s hang.
• The PR description documents a stash/unstash gate and confirms the existing client-timeout-error.test.ts and -t setTimeout tests still pass.
• Bug-hunting system found nothing; CodeRabbit found nothing.

[claude]

LGTM — nit addressed, small Node-compat fix with a focused regression test.

Extended reasoning...

Overview

This PR touches one source file (src/js/node/_http_client.ts, ~15 lines net) and adds one new test file. The source change refactors the inline res.setTimeout override on client IncomingMessage to: (1) .unref() the timer so it doesn't hold the event loop open, (2) return res for chaining, (3) treat msecs <= 0 as clear-only, and (4) register the callback via res.on('timeout', cb) so it fires through the normal emit path. All four are direct Node-compat alignments, and the .unref() pattern already exists a few hundred lines down in ClientRequestPrototype.setTimeout in the same file.

Security risks

None. No auth, crypto, permissions, or untrusted-input handling is touched. The change only affects whether a JS timer is ref'd and how a callback is dispatched on an object the caller already controls.

Level of scrutiny

Low-to-moderate. This is a localized behavioral fix in the Node http client shim with a clear repro and a regression test that fails on main and passes with the fix (verified by the author via stash/unstash). The blast radius is limited to callers of res.setTimeout on client responses — the worst plausible regression (timer never fires because it's now unref'd and nothing else keeps the loop alive) is exactly the Node behavior being matched, and the existing client-timeout-error.test.ts still passes per the PR description.

Other factors

I previously left a nit about a weak not.toContain('error') stderr assertion; the author dropped the stderr check entirely in a8fcf82 with a reasonable rationale (debug/ASAN startup banners), and the remaining exact-stdout + exit-code assertions are sufficient to catch the regression. The test was subsequently moved to its own file in f98aad5, which is cleaner than appending to the already-large node-http.test.ts. No CODEOWNERS cover these paths. No outstanding human review comments.

[claude]

LGTM — focused Node-compat fix, prior nit addressed.

Extended reasoning...

Overview

This PR touches one runtime file (src/js/node/_http_client.ts, ~15 lines net) and adds one new test file. It rewrites the inline res.setTimeout override on client IncomingMessage to: register the callback as a "timeout" listener, clear any prior timer, only schedule when msecs > 0, .unref() the timer, and return res. This mirrors the pattern already used a few lines down in ClientRequestPrototype.setTimeout (which already .unref()s its timer), so it's consistent with established code in the same file.

Security risks

None. No user-controlled input parsing, no auth/crypto/permissions surface — this only changes whether a JS timer is ref'd and how the optional callback is attached.

Level of scrutiny

Low-to-medium. It's a Node.js-compat behavioral fix in production code, but the change is mechanical: the previous implementation was clearly wrong (ref'd timer, no return value, no msecs === 0 handling), and the new behavior matches Node's documented socket.setTimeout semantics. The only edge to consider — "does .unref() ever cause a timeout to be missed?" — is the intended behavior: if nothing else is keeping the loop alive, the process should exit rather than wait, exactly as Node does.

Other factors

• My prior nit (weak stderr substring assertion) was addressed in a8fcf82 — the assertion was removed with a reasonable justification (debug/ASAN banners), and the remaining stdout.toBe("end:hello\n") + exitCode.toBe(0) assertions are strong.
• The new test passed on all CI shards; the 4 CI failures are documented cross-branch flakes unrelated to node:http.
• The author verified the timeout still fires when the server is slow, listener semantics match Node, and existing client-timeout-error.test.ts / node-http.test.ts -t setTimeout still pass.
• No CODEOWNERS entry covers these paths.

[claude]

LGTM — small, well-tested Node-compat fix; my earlier nit was addressed.

Extended reasoning...

Overview

This PR touches one production file (src/js/node/_http_client.ts, ~15 lines changed in the inline res.setTimeout override) and adds one new isolated test file. The fix makes the client-side IncomingMessage.setTimeout timer unref'd so it no longer holds the event loop open, adds return res for chainability, treats msecs <= 0 as a clear, and attaches the callback via res.on('timeout', cb) — all of which match Node's documented IncomingMessage.prototype.setTimeout semantics.

Security risks

None. This is timer ref/unref bookkeeping in the Node http client compat layer; no auth, crypto, parsing, or untrusted-input handling is touched.

Level of scrutiny

Low-to-moderate. The change is small, mechanical, and directly mirrors the existing ClientRequestPrototype.setTimeout implementation ~400 lines below in the same file, which already calls .unref() on its timer. The .on('timeout', cb) choice matches Node's actual IncomingMessage.prototype.setTimeout source (which uses on, not once). No CODEOWNERS apply to this path.

Other factors

• The new test is in its own file, spawns a child with a 60s res.setTimeout + keep-alive agent, and asserts exact stdout + exit code 0; a 2s unref'd sentinel prevents hangs on regression. It passed on all CI shards.
• My earlier nit (weak stderr substring assertion) was addressed by dropping the check entirely, with a reasonable justification (debug/ASAN startup banners) and a precedent cited in-repo. The inline comment is resolved.
• The 4 CI failures are documented pre-existing flakes seen across multiple unrelated branches; none touch node:http.
• The PR author explicitly verified against Node, against the unpatched build (fails), and against existing setTimeout tests.

[Jarred-Sumner]

Can you hoist the function out so it doesn't acess varaibles from the parent scope and avoids keeping objects alive longer than necessary?

[robobun]

@Jarred-Sumner done in 925ef69 — hoisted to module scope:

• clientResponseSetTimeout(msecs, callback) uses this (the IncomingMessage), stores the timer on this[kResTimeoutTimer] instead of a closure variable
• onClientResponseTimeout(res) is the timer callback, passed res as the extra setTimeout arg

No closures over ClientRequest locals; the only thing the unref'd timer holds is the IncomingMessage itself (needed to emit 'timeout'). Tests still pass.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/js/node/_http_client.ts`:
- Around line 1053-1056: The clientResponseSetTimeout function currently uses a
truthy check for callback which allows non-function falsy values; change the
logic to explicitly detect when a callback was provided (callback !== undefined)
and call validateFunction(callback, "callback") from internal/validators before
registering it (this will throw the appropriate $ERR_* for invalid types), then
only call this.on("timeout", callback) after validation; keep the existing
behavior of not validating when callback is omitted.

🪄 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: 98587ebd-5c75-41b4-a8a7-8ea15dea9369

📥 Commits

Reviewing files that changed from the base of the PR and between 78452b1 and 925ef69.

📒 Files selected for processing (1)

• src/js/node/_http_client.ts

[claude]

LGTM — straightforward Node-compat fix to unref the res.setTimeout timer, with a targeted regression test; my earlier nit was addressed.

Extended reasoning...

Overview

This PR rewrites the inline res.setTimeout override on client-side IncomingMessage (in src/js/node/_http_client.ts) as a module-scope function that (1) .unref()s the timer so it doesn't keep the event loop alive, (2) returns this, (3) treats msecs <= 0 as clear-only, and (4) attaches the callback via on('timeout', cb) — all of which match Node's IncomingMessage.prototype.setTimeout → socket.setTimeout semantics. A new test (node-http-res-settimeout-unref.test.ts) spawns a child that calls res.setTimeout(60000) on a keep-alive response and asserts the process exits immediately with exact stdout and exit code.

Security risks

None. This only changes timer ref/unref and listener-attachment semantics on an internal HTTP client helper; no auth, crypto, input parsing, or trust-boundary code is touched.

Level of scrutiny

Low-to-moderate. The source change is ~30 net lines replacing a 13-line closure with an equivalent module-scope function plus .unref(). The behavior delta is strictly closer to Node (the previous implementation already emitted 'timeout' and skipped when res.complete; this preserves both). The hoist to module scope is a mechanical refactor that avoids closing over ClientRequest locals.

Other factors

• My earlier inline nit about expect(stderr).not.toContain('error') was addressed by dropping the stderr assertion entirely (a8fcf82), with a reasonable rationale (debug/ASAN banner). The remaining stdout.toBe('end:hello\\n') + exitCode.toBe(0) assertions are exact and would fail loudly on regression (BAD_RETURN/STILL_ALIVE in stdout, exit 1).
• CI: the new test passed on every shard; remaining failures are documented cross-branch flakes unrelated to node:http.
• No CODEOWNERS coverage for src/js/node/ or test/js/node/http/.
• The bug-hunting system found no issues.