test: remove the timer race from the fetch redirect + Connection: close test

[robobun]

Problem

• test/js/web/fetch/fetch.test.ts, "should allow to follow redirect if connection is closed, abort should work even if the socket was closed before the redirect", fails on every run of a debug build:

  UnreachableError: reached unreachable code
      at <anonymous> (test/js/web/fetch/fetch.test.ts:2878:16)
  

• Its success case passes signal: AbortSignal.timeout(150) to a fetch that has to complete a 308 hop plus a second connection against a node:net server. On a debug build that takes 250 to 650 ms (first use of node:net alone is a few hundred ms), so the timeout wins, the fetch rejects, and the catch branch hits expect.unreachable(). The same budget is a flake source on loaded release lanes; --timeout scaling does not help because the 150 ms is wall clock inside the test.
• Its abort case raced two timers as well: a 200 ms server-side setTimeout against the 150 ms client timeout.
• The try/catch shape also turned any failed assertion inside the try into UnreachableError, hiding the real failure.
• The server listened on the wildcard address and the test fetched http://[::]:port, unlike the neighbouring raw-server tests which bind 127.0.0.1. In an environment with HTTP_PROXY set, [::] is also not covered by the usual NO_PROXY entries, so the request leaves through the proxy and the test fails for an unrelated reason.

Fix

• Test-only change; no runtime code is touched.
• Replaces the loop with two tests under describe("redirect whose response closes the connection") that share a listen() helper; each test starts its own raw net server and passes in its own handler for the redirected request. The server shape that matters is unchanged: the 308 carries Connection: close and the socket is ended with it, so the client has to open a new connection for the hop (the scenario from fix(fetch) fix redirect + Connection: close #15623).
• "follows the redirect on a new connection": awaits the fetch (with a never-aborted signal attached) and checks status, redirected, and body. No deadline.
• "can still be aborted once the redirected request is in flight": the server never answers the redirected request; the test waits until that request has arrived, aborts, and checks the AbortError rejection plus the close of the connection the redirect opened. No timers at all, and the abort is guaranteed to happen after the hop, which the old 150 ms timer only made likely.
• Why the abort test still covers the original bug: the fetch promise is only rejected after the HTTP thread closes the socket it finds in the abort tracker (src/http/HTTPThread.rs drain_queued_shutdowns, src/runtime/webcore/fetch/FetchTasklet.rs abort_task). do_redirect and on_close unregister the tracker entry (src/http/lib.rs), and on_open on the new connection registers it again. If that re-registration broke, the abort would find no socket, the promise would never settle, and the test would fail.
• Binds the server to 127.0.0.1 and fetches that address, matching the tests around it.
• Verified:
  • bun bd test test/js/web/fetch/fetch.test.ts -t "redirect whose response closes the connection": 2 pass (about 440 ms + 90 ms on the debug build).
  • Same filter with the release binary (USE_SYSTEM_BUN=1), 5 runs: 2 pass each time, about 10 ms + 3 ms.
  • Old test on the same debug build, 3 runs: fails every time (547 ms, 643 ms, 616 ms against the 150 ms budget); it passes on the release binary.
  • Full fetch.test.ts on the debug build: both new tests pass in the middle of the file as well.

Background

• AbortSignal.timeout(ms) rejects the fetch with a TimeoutError once ms of wall clock pass; a test that expects the fetch to succeed while such a signal is attached is asserting a speed, not a behaviour.
• The abort tracker is a map on the HTTP thread from request id to the socket currently carrying that request. Aborting from JS queues a shutdown for the id; the HTTP thread closes whatever socket the map holds and only then reports the failure back, which is what rejects the promise. A redirect replaces the socket, so the entry has to be dropped and re-added for the new connection.
• events.once(socket, "close") rejects if the emitter emits "error" first. The client resets the connection on abort, so the server side sees ECONNRESET before "close"; the test listens for "close" directly for that reason.

[robobun]

^{Updated 3:06 PM PT - Aug 12th, 2026}

❌ @robobun, your commit eec7c31 has 3 failures in Build #93486 (All Failures):

• test/js/third_party/stripe/stripe.test.ts - code 1 on 🐧 25.04 x64
• test/js/third_party/stripe/stripe.test.ts - code 1 on 🐧 13 x64-asan
• test/cli/install/migration/complex-workspace.test.ts - code 1: error: Failed to install on 🐧 25.04 aarch64
• scripts/build/ci.ts - build failed on 🐧 x64-android - build-bun
• scripts/build/ci.ts - build failed on 🐧 x64-musl - build-bun

---

🧪   To try this PR locally:

bunx bun-pr 37913

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

bun-37913 --bun

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 44 seconds

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: a92da4d1-bf8c-4ccf-8678-53f377177896

📥 Commits

Reviewing files that changed from the base of the PR and between 9a543cc and eec7c31.

📒 Files selected for processing (1)

• test/js/web/fetch/fetch.test.ts

---

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

[robobun]

Status: reproduced and fixed, test-only change.

• Reproduced on a debug build with bun bd test test/js/web/fetch/fetch.test.ts -t "abort should work even if the socket was closed before the redirect": fails 3 of 3 runs (547 ms, 643 ms, 616 ms against the test's 150 ms AbortSignal.timeout), passes on the release binary.
• With this PR, bun bd test test/js/web/fetch/fetch.test.ts -t "redirect whose response closes the connection" passes on the debug build, and 5 of 5 runs pass on the release binary.

The earlier bot comment above is only a review rate-limit notice; nothing to address from it.

[claude]

LGTM — test-only de-flake that swaps a 150 ms wall-clock race for event-driven synchronization.

What was reviewed:

• Confirmed the rewrite preserves the #15623 scenario (308 + Connection: close forces a new connection; abort must track the new socket) and strengthens it — abort now provably fires after the hop, and the test additionally asserts the redirected connection is torn down.
• Checked for lingering timers/sleeps, hardcoded ports, and resource leaks — none; await using on the server and await closed on the second socket match neighboring raw-server tests in this file.
• Verified the socket.once("close", ...) choice over events.once is correct given the ECONNRESET the client abort produces, and that redirected.resolve passed bare works with Promise.withResolvers.

Extended reasoning...

Overview

This PR touches only test/js/web/fetch/fetch.test.ts, replacing one flaky test (should allow to follow redirect if connection is closed...) with a two-test describe block. The old test used AbortSignal.timeout(150) on both its success and failure paths and a server-side setTimeout(200), which raced wall-clock time — deterministic-fail on debug builds (250–650 ms per hop) and a flake source on loaded release lanes. The new tests await observable conditions instead: the success case awaits the fetch directly with a never-fired signal attached; the abort case waits for the redirected request to actually arrive at the server (Promise.withResolvers resolved from the connection handler), then aborts and awaits both the rejection and the server-side socket close.

Security risks

None. Test-only change to a local node:net server bound to 127.0.0.1:0. No runtime, auth, crypto, or network-facing production code is touched.

Level of scrutiny

Low-to-moderate. This is a de-flake of an existing test with no production code changes. Per REVIEW.md the key checks for de-flaking are (1) the property the original assertion protected is still asserted, and (2) no new timing dependencies are introduced. Both hold: the 308-with-Connection: close server shape is unchanged, the success path still asserts status/body (plus redirected: true, which is stricter), and the abort path is now stronger than before — it guarantees the abort happens after the redirect hop rather than merely making it likely via a 150 ms/200 ms race, and it additionally verifies the new connection is torn down. The PR description's explanation of why the abort test would fail if the tracker re-registration broke (the fetch promise would never settle) is sound.

Other factors

• The throw inside the socket error handler (line 2829) predates this PR — it's carried over unchanged from the original test.
• The switch from wildcard/[::] to 127.0.0.1 matches every other raw-net.createServer test in this file and avoids the HTTP_PROXY/NO_PROXY interaction the description calls out.
• await using server on a net.Server is the established pattern in this file (four other tests do the same), so async-dispose support is already relied on.
• The author verified debug-build pass, release-build pass across 5 runs, and full-file pass — consistent with the repo's test-verification requirements.
• No prior human or bot reviews to reconcile with.