http.request: emit 'error' with AbortError when options.signal aborts

[robobun]

Fixes #31167

Reproduction

import http from "node:http";

const signal = AbortSignal.timeout(200);
const req = http.request("http://127.0.0.1:<unreachable-port>", { signal });
req.on("error", err => console.log("error:", err.code));
req.end();
// Node: prints 'error: ABORT_ERR' after 200ms.
// Bun before this PR: hangs until the socket times out (~10s+).

Cause

In src/js/node/_http_client.ts, the options.signal listener only forwarded the abort to the internal AbortController, and the internal onAbort only emitted 'abort' + 'close'. The fetch-promise .catch path explicitly swallowed AbortError with the comment "The 'abort' listener on the abort controller should have called this" — but nothing ever emitted an 'error' event. A second gap: if signal.aborted was already true when http.request() was called, addEventListener("abort", …) was never fired and no abort ever happened.

Fix

• When options.signal fires, emit 'error' with \$makeAbortError(undefined, { cause: signal.reason }) (Node-compatible: name: 'AbortError', code: 'ABORT_ERR', cause = the signal's reason), then run socketCloseListener and abort, all on one nextTick. This gives the Node-matching event order error → close → abort.
• Skip the signal branch entirely once res.complete is true — a late-firing AbortSignal.timeout(N) used as a request deadline must not fire 'error' on a request that already succeeded. The old iteration double-emitted here because abortHandler calls both kAbortController.abort() and onAbort() directly, and the controller fires its listener once.
• A signalAbortHandled latch + eager abortedSymbol set prevents the redundant direct onAbort() call from re-entering socketCloseListener.
• If the signal is already aborted at construction, schedule the abort handler on process.nextTick so the same error is delivered.
• req.abort() / req.destroy() paths are unaffected — they set abortedSymbol before calling the internal AbortController, so the signal-specific branch never runs for them.

Verification

Four tests in a new test/js/node/http/client-signal-abort.test.ts (under describe("http.request options.signal")):

1. Signal fires mid-request → 'error' is emitted with AbortError / ABORT_ERR / cause.name === 'TimeoutError'.
2. Signal is aborted before the request runs → 'error' still emitted with the supplied reason as cause.
3. Signal fires after the response completes → no error emitted (regression for the double-emit / late-fire case flagged in review).
4. Event order is error before close when the signal fires.

(Kept in its own file to isolate from pre-existing neighbor failures in node-http.test.ts; the existing "should attempt to make a standard GET request and abort" test in that file gains a no-op 'error' listener to swallow the now-emitted AbortError.)

Error shape cross-checked against Node 24:

name: AbortError
code: ABORT_ERR
cause.name: TimeoutError

[robobun]

^{Updated 11:35 AM PT - May 21st, 2026}

❌ @robobun, your commit f0a3ea3 has 1 failures in Build #56675 (All Failures):

• test/js/node/http/node-http-backpressure.test.ts - SIGKILL on 🐧 25.04 x64-baseline
• test/js/node/http/node-http-backpressure.test.ts - SIGKILL on 🐧 13 x64-baseline
• test/js/node/http/node-http-backpressure.test.ts - SIGKILL on 🐧 25.04 x64
• test/js/node/http/node-http-backpressure.test.ts - SIGKILL on 🐧 13 x64

---

🧪   To try this PR locally:

bunx bun-pr 31170

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

bun-31170 --bun

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Centralizes http.request options.signal wiring, emits an AbortError on next tick using signal.reason as cause before socket cleanup and legacy "abort", installs a noop error listener to avoid uncaught exceptions, and adds tests for in-flight, pre-aborted, post-response, and event-ordering cases.

Changes

AbortSignal handling in HTTP client

Layer / File(s)	Summary
ClientRequest abort wiring and next-tick error emission src/js/node/_http_client.ts	Add module noop error listener; refactor onAbort() to add signalAbortHandled guard and emitSignalAbortNT path that emits an AbortError (using signal.reason as cause), runs socket close/cleanup, and emits legacy "abort"; replace direct signal.addEventListener("abort", ...) with an abortHandler that aborts internal kAbortController and invokes onAbort(), scheduling next-tick when the signal is already aborted.
Tests: signal-abort regression suite test/js/node/http/client-signal-abort.test.ts	Add http.request options.signal tests: in-flight abort via AbortSignal.timeout() (assert error name/code and cause type), pre-aborted signal preserves provided reason as cause, post-response abort produces no request "error", and event-ordering test asserts "error" is emitted before terminal "close"; tests use a local node:http server and close it in finally.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title accurately describes the main change: implementing 'error' event emission with AbortError when options.signal aborts http.request.
Description check	✅ Passed	The PR description provides comprehensive details on what was fixed, reproduction steps, root causes, and verification through test cases; it exceeds the basic template requirements.
Linked Issues check	✅ Passed	The PR fully addresses the coding objectives from issue #31167: emit 'error' with AbortError (code 'ABORT_ERR', cause = signal.reason), handle pre-aborted signals, skip late-firing, and prevent double-emissions.
Out of Scope Changes check	✅ Passed	All code changes in src/js/node/_http_client.ts and the test file are directly scoped to fixing AbortSignal handling in http.request; no unrelated modifications detected.

_{✏️ 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.}

[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/js/node/_http_client.ts`:
- Around line 268-276: abortHandler currently calls
this[kAbortController]?.abort?.() and then directly calls onAbort(), causing
duplicate abort logic and potential duplicate "error" events or late AbortError
emissions after request completion; change abortHandler to only call
this[kAbortController].abort() (or, if there is no controller, call onAbort()),
and guard onAbort from emitting after the request is finished by checking
this[abortedSymbol] or request completion state before calling
process.nextTick(emitAbortErrorNextTick, ...). Update references in abortHandler
and any similar handlers (see other occurrence around lines 739-742) so abort is
triggered once via the controller signal and onAbort only runs when the request
is still active, using the existing symbols: this[kAbortController], onAbort,
this[kSignal], abortedSymbol, emitAbortErrorNextTick, and $makeAbortError.

🪄 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: 87c7070d-7185-4ff8-bc8d-fa7f2ad10b75

📥 Commits

Reviewing files that changed from the base of the PR and between 8bd80ef and 71ea063.

📒 Files selected for processing (3)

• src/js/node/_http_client.ts
• test/js/node/http/client-signal-abort.test.ts
• test/js/node/http/node-http.test.ts

[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 `@test/js/node/http/client-signal-abort.test.ts`:
- Around line 101-102: Replace the arbitrary delay using setTimeout in the test
with Bun.sleep(0) so the macrotask barrier drains the process.nextTick/microtask
queue; locate the await new Promise(r => setTimeout(r, 50)) line in the test (in
the client-signal-abort.test) and change it to await Bun.sleep(0) to follow the
guideline of avoiding setTimeout in tests.

🪄 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: 86fb050a-f5c0-4f8c-a6a6-e5358fba67bf

📥 Commits

Reviewing files that changed from the base of the PR and between 71ea063 and 687016a.

📒 Files selected for processing (2)

• src/js/node/_http_client.ts
• test/js/node/http/client-signal-abort.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 `@src/js/node/_http_client.ts`:
- Around line 290-307: The onAbort handler currently still runs teardown logic
for late aborts and the options.signal listener isn't removed on request settle;
update onAbort (and the settle/destroy paths that register the listener) so that
if this[kSignal]?.aborted fires after the response is complete it becomes a true
no-op (return immediately without calling socketCloseListener or toggling
this[abortedSymbol]), and ensure the options.signal listener is explicitly
removed when the request settles or is destroyed (use
signal.removeEventListener/removeListener where the listener was originally
registered). Tie the removal to the same settle/cleanup code paths that call
this[kClearTimeout] and socketCloseListener, and keep the existing
emitSignalAbortNT + $makeAbortError behavior only for aborts that occur before
res.complete; reference onAbort, options.signal, this[kSignal],
this[abortedSymbol], socketCloseListener, emitSignalAbortNT and $makeAbortError
when making the changes.

In `@test/js/node/http/client-signal-abort.test.ts`:
- Around line 134-135: The test currently only checks ordering with
expect(events.indexOf("error")).toBeLessThan(events.indexOf("close")) which can
pass if "error" is missing; update the assertion to first assert both events
exist (e.g., expect(events).toContain("error") and
expect(events).toContain("close")) and then assert ordering using the same
indexOf checks; locate the failing line in the test that calls
expect(events.indexOf("error")).toBeLessThan(events.indexOf("close")) and add
the two presence assertions immediately before it.

🪄 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: 3f0b7d72-13de-4a55-8cb9-db0705d62c7a

📥 Commits

Reviewing files that changed from the base of the PR and between 687016a and 4329461.

📒 Files selected for processing (2)

• src/js/node/_http_client.ts
• test/js/node/http/client-signal-abort.test.ts

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🔴 src/js/node/_http_client.ts:739-742 — Related to the earlier comment on line 274, but a distinct trigger that the suggested && !this?.res?.complete guard does not cover: const req = http.request(url, {signal}); req.destroy() (never calling end()). Here kAbortController is still null so the pre-PR listener was a complete no-op, but the new abortHandler now calls onAbort() directly; destroy() sets neither abortedSymbol nor res, so when the signal later fires onAbort schedules an unguarded emit('error', AbortError) on a request the user already tore down — an uncaught exception if no listener is attached. The guard at line 274 also needs && !this.destroyed (or remove the signal listener when the request settles, as Node does via eos()).

  Extended reasoning...

  What the bug is

  The new abortHandler (src/js/node/_http_client.ts:739-742) calls onAbort() directly after this[kAbortController]?.abort?.(). Before this PR the listener was just () => this[kAbortController]?.abort(), which was a complete no-op whenever kAbortController was null — i.e. whenever startFetch() had not yet run. The direct onAbort() call removes that implicit short-circuit, so a user-signal abort now drives onAbort even on a request that was destroyed before any I/O started.

  This is closely related to the earlier comment on line 274 (spurious error after a successful response), but it is a distinct trigger that the fix suggested there does not cover. That comment proposed adding && !this?.res?.complete to the guard; in this scenario res is undefined, so !this?.res?.complete evaluates to !undefined → true and the error still fires.

  Code path

  const req = http.request(url, { signal: AbortSignal.timeout(1000) });
  req.destroy();           // user changes mind, never calls end()
  // ...1s later the timeout fires

  Step-by-step proof

  1. http.request(url, {signal}): signal is not yet aborted, so signal.addEventListener('abort', abortHandler, {once:true}) is registered (line 746). this[kAbortController] = null (line 723). startFetch never runs because end()/write()/flushHeaders() are never called.
  2. req.destroy() (lines 218-237): sets destroyed = true, finished = true. this[kAbortController]?.abort?.() is a no-op on null. this.socket.destroy() runs on the lazy FakeSocket. Crucially: abortedSymbol stays unset (only req.abort() or the block at line 278-281 set it), _closed stays false, and the signal listener is never removed.
  3. t=1000ms — signal fires → abortHandler runs → this[kAbortController]?.abort?.() is a no-op (null) → onAbort() called directly.
  4. In onAbort (line 274): signal.aborted === true and !this[abortedSymbol] === true → schedules process.nextTick(emitAbortErrorNextTick, this, abortError). Then socketCloseListener() emits 'close' (since _closed was false). Then line 278: !this[abortedSymbol] && !this?.res?.complete → true && !undefined → true → schedules 'abort'.
  5. nextTick: emitAbortErrorNextTick (line 1094) unconditionally calls self.emit('error', err) — it deliberately bypasses the destroyed check. With no 'error' listener (typical when the user themselves called destroy() and considers the request done), this throws as an uncaught exception from inside process.nextTick.

  Why none of the existing/proposed guards help

  Guard	Value here	Blocks?
  !this[abortedSymbol]	true (never set)	❌
  !this?.res?.complete (comment on line 274)	!undefined = true	❌
  !this.finished (CodeRabbit)	false — would block, but also blocks the primary fix since req.end() sets finished=true before an in-flight abort	❌ wrong guard

  Node.js avoids this entirely by removing the signal listener via eos() once the request settles (including on destroy()), so the late signal is a no-op there.

  Impact

  This is a regression introduced by this PR: pre-PR Bun did nothing in this scenario (the old listener no-opped on a null controller). Post-PR, a user who creates a request with a deadline signal and then explicitly destroys it (e.g. external cancellation, retry logic that gives up) gets an uncaught AbortError when the deadline later expires — potentially crashing the process. Less common than the res.complete case from the other comment, but still a real-world pattern.

  How to fix

  Either extend the guard at line 274 to also check !this.destroyed:

  if (signal?.aborted && !this[abortedSymbol] && !this.destroyed && !this?.res?.complete) {
    process.nextTick(emitAbortErrorNextTick, this, $makeAbortError(undefined, { cause: signal.reason }));
  }

  …or, more robustly, remove abortHandler from options.signal once the request settles (success, error, or destroy), matching Node's eos()-based cleanup. The latter also fixes the res.complete case from the other comment and avoids leaking the listener on long-lived/shared signals.

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🔴 src/js/node/_http_client.ts:691-694 — maybeEmitClose() runs on the nextTick immediately after emit('response', res) (line 496) — i.e. when only headers have arrived and the body is still streaming — so calling removeSignalListener() here detaches options.signal before the response completes. A signal that fires during body streaming (e.g. AbortSignal.timeout(N) on a slow/hung response) then becomes a complete no-op: abortHandler is gone, kAbortController is never aborted, and the response can no longer be cancelled — a regression vs both pre-PR Bun and Node (which keeps the listener until responseOnEnd). The !this?.res?.complete guard in onAbort already handles the post-completion late-fire case this call was added for, so this line can simply be dropped (or gated on this.res?.complete).

  Extended reasoning...

  What the bug is

  maybeEmitClose() (src/js/node/_http_client.ts:689) is invoked from the nextTick scheduled by handleResponse() — specifically from the finally { if (self.finished) maybeEmitClose(); } block at line 494-496, which runs immediately after self.emit('response', res). At that moment only the response headers have arrived; the body is still being streamed asynchronously through the IncomingMessage (driven by the fetch body ReadableStream), and res.complete is still false.

  Commit 9e90768 added removeSignalListener() at line 694 inside maybeEmitClose(), unconditionally and before the !this._closed check. This detaches abortHandler from the user's options.signal the instant headers arrive. From that point until the body finishes, firing the signal is a complete no-op: abortHandler never runs, this[kAbortController] is never aborted, the underlying fetch keeps streaming, and onAbort() is never reached.

  Why this is a regression introduced by this PR

  Before this PR the user-signal listener was never removed (the diff shows it was a plain { once: true } addEventListener with no removeEventListener anywhere). A signal firing while the response body was streaming reached this[kAbortController]?.abort() → fetch body aborted + the controller's onAbort listener → socketCloseListener() → res.destroy(new ConnResetException('aborted')). The response stream was torn down and the user saw an error/abort.

  After this PR the listener is gone before the body starts arriving, so the same signal does nothing — a slow, large, or hung response body can no longer be cancelled via options.signal once headers have been received. AbortSignal.timeout(N) as a request deadline silently stops working past the headers point.

  Node.js wires the signal up via addAbortSignal and removes it with this.once('close', disposable[SymbolDispose]), where ClientRequest 'close' fires only after responseOnEnd() (i.e. after the body completes). So in Node the signal can abort throughout body streaming. Bun already emitted req 'close' early (a pre-existing divergence), but coupling removeSignalListener() to that early point turns the divergence into a functional regression.

  Why existing guards don't help

  • The !this?.res?.complete guard in onAbort (line 307) is irrelevant — onAbort is never reached because abortHandler was removed from the signal.
  • signalAbortHandled is irrelevant for the same reason.
  • The other two removeSignalListener() call sites (socketCloseListener line 256, this.destroy() line 228) are fine — they run on actual teardown. Only the maybeEmitClose() site fires prematurely.
  • None of the new tests exercise the during-body window: test #3 ("does not emit error when the signal fires after the response completes") aborts only after res.on('end') fires, so it never covers the headers-arrived-body-streaming window.

  Step-by-step proof

  const server = http.createServer((_, res) => {
    res.write('chunk');           // headers + first chunk; never end()
  });
  const req = http.get(url, { signal: AbortSignal.timeout(200) }, res => res.resume());
  req.on('error', e => console.log(e.code));

  1. t≈1ms — nodeHttpClient(...).then(response => ...) resolves with headers. handleResponse() schedules a nextTick.
  2. nextTick — self.emit('response', res) fires; finally block runs; self.finished is true (set synchronously by send() when req.end()/http.get ran) → maybeEmitClose() → removeSignalListener() detaches abortHandler → schedules emitCloseNTAndComplete. res.complete is still false; body is still streaming.
  3. t=200ms — timeout signal fires → no listener attached → nothing happens. kAbortController is never aborted; the fetch body stream keeps reading; onAbort never runs. Request hangs indefinitely.
  4. Pre-PR / Node: at t=200ms, abortHandler runs → kAbortController.abort() → fetch body aborts + onAbort → socketCloseListener() → res.destroy(ConnResetException('aborted')) → user sees the error.

  Impact

  Any code that passes { signal: AbortSignal.timeout(N) } (or an AbortController signal) to bound total request time — a very common pattern — loses the ability to cancel once headers arrive. A server that sends headers and then stalls (or trickles a large body) will hang the client past the deadline. This is strictly worse than both pre-PR Bun and Node.

  How to fix

  The removeSignalListener() call at line 694 is redundant for its stated purpose — the !this?.res?.complete guard in onAbort (line 307) already makes a post-completion late signal a no-op (it skips the emitSignalAbortNT branch when res.complete is true), and socketCloseListener() already calls removeSignalListener() on actual teardown. Simply dropping line 691-694 restores the during-body cancellation behaviour while keeping the late-fire protection intact.

  If listener cleanup on the success path is desired for memory/retention reasons, it should be deferred until res.complete becomes true (e.g. hooked from emitEOFIncomingMessage or res.on('end')), not run at headers time.

[robobun]

Addressed claude[bot]'s latest finding in a384341 — dropped removeSignalListener() from maybeEmitClose(). That call ran on the nextTick after response headers arrived (before body streaming), so it made options.signal a no-op for the body-streaming window. The !res.complete guard in onAbort already prevents the spurious-late-signal case the call was added for. New regression test covers the headers-arrived / body-streaming window by awaiting data then aborting manually.

[robobun]

CI #56650, #56658, and #56665 all red on the same pre-existing unrelated lane:

• test/js/node/test/parallel/test-listen-fd-detached.js → Invalid stdio option[3] "[object Listener]" — reproduces on main; fd-passing via IPC is not yet implemented (tracked in fix(ipc): implement sendHandle for passing net.Socket via IPC (#6743) #27532). My diff doesn't touch child_process or net.

Already spent my one ci: retrigger earlier in this PR. All 9 new tests in test/js/node/http/client-signal-abort.test.ts pass locally under both bd and release; test/js/node/http/node-http.test.ts has the same 1 pre-existing proxy failure before and after this PR. Diff is green — needs a maintainer to merge past the flaky lane.

[robobun]

Heads up: #31587 landed on main and rewrote the HTTP client on net/tls + llhttp, replacing the fetch-based implementation this PR patched. The new client wires options.signal through Node's real addAbortSignal(signal, this), which is exactly the mechanism #31167 was missing.

Checking whether the rewrite already fixes #31167 by running this PR's regression suite against current main. If it passes clean, this PR is obsolete and I'll close it; if gaps remain, I'll port the fix onto the new client.

[robobun]

Closing this as obsolete.

#31587 rewrote the node:http client on net/tls + llhttp and replaced the fetch-based implementation this PR patched. I verified against current main (454e3b2) that the rewrite already fixes #31167: the exact issue scenario now emits the AbortError as expected.

// http.request({ signal }), signal aborts mid-request
node:       {"name":"AbortError","code":"ABORT_ERR","hasCause":true}
main (bun): {"name":"AbortError","code":"ABORT_ERR","hasCause":true}

The rewrite wires options.signal through Node's real addAbortSignal(signal, this), and ships its own regression test (the ClientRequest.signal describe block in node-http.test.ts, asserting err.name === 'AbortError' on AbortSignal.timeout).

I ran this PR's 13-test suite against main: the 6 core-behavior tests pass, and the 4 that fail were all asserting the old fetch-based client's behavior, which diverged from Node. I confirmed each against Node on main:

• destroy()-then-signal: both emit ECONNRESET (my test expected none)
• sync-throwing options.lookup: both throw synchronously out of http.request() (my test expected an 'error' event)
• destroy + late async options.lookup error: both emit socket hang up (my test expected none)

So the new client is strictly more Node-compatible here, and there's nothing left to port. The code this PR changes no longer exists.