Repository navigation
test(http): lock AbortSignal behavior for http.request options.signal - #34567
armancharan wants to merge 2 commits into
Conversation
Add regression coverage for oven-sh#31167: AbortSignal.timeout must emit AbortError on http.request, including during connect to an unreachable host. Behavior is already fixed on main via oven-sh#31587; this prevents regressing it (still broken on 1.3.14).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
WalkthroughChangesThe PR adds Node HTTP client regression tests covering AbortSignal behavior before, during, and after requests, including error metadata, event ordering, streaming responses, connection failures, and timeout settlement. HTTP client AbortSignal behavior
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Drop PR-history preamble and clarify the blackhole timeout note.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
test/js/node/http/client-signal-abort.test.ts (5)
28-31: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the timeout cause by identity.
Checking only
cause.nameallows a newly createdTimeoutErrorto pass. Assert thaterr.causeis exactlysignal.reason.As per coding guidelines, assertions should prove the strongest invariant. The supplied AbortSignal contract requires cause-object identity.
Proposed fix
- expect((err as any).cause).toBeDefined(); - expect((err as any).cause.name).toBe("TimeoutError"); + expect((err as any).cause).toBe(signal.reason);🤖 Prompt for 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. In `@test/js/node/http/client-signal-abort.test.ts` around lines 28 - 31, Update the timeout-cause assertions in the abort test to verify that err.cause is the exact same object as signal.reason, rather than checking only that it exists and has a "TimeoutError" name. Preserve the existing timeout scenario and use the signal instance already exercised by the test.Source: Coding guidelines
32-34: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAwait server shutdown in every
finallyblock.
server.close()is asynchronous; invoking it without awaiting completion can leave sockets or close events pending across tests.As per coding guidelines, abort and timeout paths must complete resource release before the operation finishes.
Also applies to: 61-63, 93-95, 128-130, 190-192
🤖 Prompt for 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. In `@test/js/node/http/client-signal-abort.test.ts` around lines 32 - 34, Update every finally block in the test cases to await the asynchronous server.close() call, including the blocks around the referenced abort and timeout paths. Ensure each test completes server shutdown and associated resource cleanup before its operation finishes.Source: Coding guidelines
138-142: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse a dynamically closed local port instead of port 1.
Port 1 is not guaranteed to be unused. Allocate an ephemeral local port, close the listener, then connect to that captured port so the test deterministically exercises the connection-failure path.
As per coding guidelines, tests must be hermetic and avoid environment-dependent fixtures.
🤖 Prompt for 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. In `@test/js/node/http/client-signal-abort.test.ts` around lines 138 - 142, Update the request setup around req and controller to first allocate an ephemeral local port with a temporary listener, capture its assigned port, close the listener, and use that port instead of hardcoded port 1. Preserve the test’s connection-failure and abort behavior while removing dependence on environment-specific port availability.Source: Coding guidelines
195-213: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRemove the environment-dependent blackhole address.
10.255.255.1may fail immediately withENETUNREACHorECONNREFUSEDdepending on CI routing, so this test can fail before the 150 ms abort or validate the wrong path. Use a deterministic connect-stall fixture or a repository-supported network fixture.As per coding guidelines, tests must be hermetic and avoid external hosts.
🤖 Prompt for 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. In `@test/js/node/http/client-signal-abort.test.ts` around lines 195 - 213, Replace the hard-coded blackhole URL in the AbortSignal.timeout test with a deterministic repository-supported connect-stall fixture that keeps the connection pending until the signal aborts. Preserve the existing request, AbortError assertions, and prompt-settlement check while ensuring the test does not depend on external network routing.Source: Coding guidelines
174-187: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the streaming completion assertion observable.
reject(new Error(...))is ineffective once the request-error promise has resolved, so a response that emitsendafter abort will not fail this test. Trackendseparately, await request closure, and assert that the response never completed.As per coding guidelines, every assertion must be able to fail and tests must await observable conditions.
Proposed fix
const controller = new AbortController(); + let responseEnded = false; const { promise, resolve, reject } = Promise.withResolvers<Error>(); const req = request(`http://127.0.0.1:${port}`, { signal: controller.signal }, res => { res.on("data", () => { controller.abort(); }); res.on("end", () => { - reject(new Error("response should not complete after abort")); + responseEnded = true; }); }); req.on("error", resolve); req.end(); const err = await promise; + await once(req, "close"); expect(err.name).toBe("AbortError"); expect((err as any).code).toBe("ABORT_ERR"); + expect(responseEnded).toBe(false);🤖 Prompt for 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. In `@test/js/node/http/client-signal-abort.test.ts` around lines 174 - 187, Update the request test around the Promise.withResolvers flow so response end is tracked independently from the request error promise. Await request closure after abort, then assert that the response did not emit end; do not use reject from the error promise as the completion assertion.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/js/node/http/client-signal-abort.test.ts`:
- Around line 28-31: Update the timeout-cause assertions in the abort test to
verify that err.cause is the exact same object as signal.reason, rather than
checking only that it exists and has a "TimeoutError" name. Preserve the
existing timeout scenario and use the signal instance already exercised by the
test.
- Around line 32-34: Update every finally block in the test cases to await the
asynchronous server.close() call, including the blocks around the referenced
abort and timeout paths. Ensure each test completes server shutdown and
associated resource cleanup before its operation finishes.
- Around line 138-142: Update the request setup around req and controller to
first allocate an ephemeral local port with a temporary listener, capture its
assigned port, close the listener, and use that port instead of hardcoded port
1. Preserve the test’s connection-failure and abort behavior while removing
dependence on environment-specific port availability.
- Around line 195-213: Replace the hard-coded blackhole URL in the
AbortSignal.timeout test with a deterministic repository-supported connect-stall
fixture that keeps the connection pending until the signal aborts. Preserve the
existing request, AbortError assertions, and prompt-settlement check while
ensuring the test does not depend on external network routing.
- Around line 174-187: Update the request test around the Promise.withResolvers
flow so response end is tracked independently from the request error promise.
Await request closure after abort, then assert that the response did not emit
end; do not use reject from the error promise as the completion assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4770c724-9b22-40e2-9337-6cf53235f729
📒 Files selected for processing (1)
test/js/node/http/client-signal-abort.test.ts
|
Thanks for putting this together. #31167 was fixed on main by the http client rewrite in #31587, and that PR also brought in coverage for these cases:
The one case here that main does not have, the request to Since the reported behavior is already covered on main, closing this one. If you would like to add hermetic versions of the streaming-body or abort-after-completion cases, a smaller follow-up would be welcome. |
2 participants
Summary
mainvia the net/tls + llhttp client rewrite (#31587); #31170 correctly closed as obsolete.options.signal/AbortSignal.timeoutcannot silently regress.http://10.255.255.1/hang): on Bun 1.3.14 the test times out; on currentmainit settles withAbortError(ABORT_ERR) in ~150ms.Why tests-only
Re-implementing the old fetch-client patch would be incorrect. Depth here is verifying the rewrite, documenting the gap, and locking Node-parity abort lifecycle with tests maintainers can keep.
Test plan
bun bd test test/js/node/http/client-signal-abort.test.ts— 7 passUSE_SYSTEM_BUN=1 bun test … -t "unreachable host"— times out on 1.3.14 (bug still present in release)Made with Cursor