Conversation
The two abort-while-connecting tests dialed 192.0.2.1 and relied on the network between the CI host and the internet to drop the packets. A host whose network answers with ICMP unreachable, or that has no route at all, fails the fetch before the signal fires. Dial a local listener instead. It is a raw libc socket with the smallest accept queue, and a few connections of our own fill the queue, so the kernel drops the SYN of every later connect and the fetch stays in SYN_SENT until the signal aborts it. Windows answers a full accept queue with RST, so it keeps dialing TEST-NET-1.
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 30 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 42 seconds), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
|
Status: ready for a maintainer. Current revision: 68bb9b1. How it was reproduced:
CI for 68bb9b1 (build 102654): the three files pass on darwin aarch64, debian x64, debian x64 asan, alpine x64 and windows 2019 x64. The valkey block runs on the docker lanes (58 pass) and is skipped on darwin and windows as before. The only failure that did not pass on retry is |
There was a problem hiding this comment.
LGTM — well-researched test de-flake that removes the network-topology dependence.
What was reviewed:
- The full-accept-queue technique matches the existing pattern in
dns-interleave.test.ts;libcPathForDlopen()covers linux/darwin and Windows keeps the original path. - Checked
sockaddr_inlayout for both the Linux and BSD branches, and the big-endian port read aftergetsockname— both correct. - Verified
beforeAllsetsurlbefore thetest.concurrentbodies run, and that LIFO cleanup destroys fillers before closing the listen fd. - Confirmed the filler-ordering claim (all 8
connect(2)are issued in one tick before any'connect'event can fire), so the queue is full before any test's fetch sends a SYN.
Extended reasoning...
Overview
This PR modifies a single test file, test/js/bun/io/fetch/fetch-abort-slow-connect.test.ts, to remove its dependence on how the CI host's network routes packets to TEST-NET-1 (192.0.2.1). The old test assumed a silent SYN drop; a new darwin CI host (bingus) instead returns ICMP unreachable in ~20 ms, which beats the 50 ms AbortSignal.timeout and flips the rejection to a connection error rather than a TimeoutError. The new setup creates a raw libc listener on 127.0.0.1:0 with the smallest possible backlog, fills its accept queue with 8 net.connect fillers, and points the three tests at that URL. On Linux/macOS this reliably leaves later connects stuck in SYN_SENT; Windows (where a full queue produces RST) keeps the original 192.0.2.1 path since Windows CI has always passed this file.
Security risks
None. Test-only change, no runtime code touched. The raw libc socket is bound to loopback with an ephemeral port and closed in afterAll.
Level of scrutiny
Low-to-medium. It's a targeted CI de-flake with no production surface, following an existing in-tree pattern (dns-interleave.test.ts uses the same full-accept-queue trick on Linux, and the PR description references valkey-gc.test.ts for the raw-libc-listener approach). The PR description documents empirical verification on the failing host (5/5 pass), a darwin x64 host, and multiple debug runs, plus ss/netstat state captures showing the intended SYN_SENT state.
Other factors
- The
sockaddr_inconstruction correctly branches on Linux vs BSD layout (16-bitsin_familyvssin_len+sin_family), and port is read big-endian from offsets 2–3 aftergetsockname— correct for both. AF_INET=2/SOCK_STREAM=1are correct on both Linux and macOS;libcPathForDlopen()in harness handles both platforms.- Cleanup is LIFO via
cleanup.pop(), so fillers are destroyed before the listen fd is closed; filler sockets have an'error'handler so late SYN-retry timeouts don't surface as unhandled. - The three tests remain
test.concurrent;beforeAllfully awaits the first filler's'connect'(guaranteeing all 8 SYNs are already sent) before any test starts, so there is no race onurlor on queue fullness. - The PR notes that
USE_SYSTEM_BUN=1also passes — expected here since this is test-infrastructure hardening, not new coverage for a Bun-side fix. The test's original assertion contract (TimeoutErrorwithin 1 s) is unchanged. - No prior human review comments to address; only a CodeRabbit rate-limit notice on the timeline.
|
The review above asks for no changes. One update to the body: |
…or the other TEST-NET-1 tests The listener is the same code that fetch-abort-slow-connect.test.ts set up inline. fetch-abort-ssl-context-eviction.test.ts and the valkey connection timeout test dialed 192.0.2.1 for the same reason and now dial the listener too. On a host that rejects TEST-NET-1, the eviction test ran with no connecting sockets and the valkey test got a connect error instead of the timeout.
|
Updated 6:36 AM PT - Aug 21st, 2026
❌ @robobun, your commit 68bb9b1 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 39903That installs a local version of the PR into your bun-39903 --bun |
|
Pushed 68bb9b1 after a self-review of the first revision. It raised two points:
The copies inside the child scripts of |
There was a problem hiding this comment.
LGTM — test-only de-flake that replaces network-dependent TEST-NET-1 dials with a local full-accept-queue listener.
What was reviewed:
test/blackhole.tsmatches the existing raw-libc listener pattern invalkey-gc.test.tsanddns-interleave.test.ts; the sockaddr_in Linux/BSD branching and backlog values (0 vs 1) line up with those precedents.- Checked that the ssl-context-eviction fixture still works when the listener lives in the parent and children dial it — the queue is kernel state so cross-process connects also see dropped SYNs; Windows keeps the old TEST-NET-1 path unchanged.
- The valkey
connection timeouttest also fixes an unawaited asynctoThrow(previously vacuous) into a properawait expect(...).rejectswith try/finally cleanup. - Confirmed
libcPathForDlopencovers linux/darwin and Windows returns early before reaching it; filler sockets and the fd are released viaSymbol.disposeon both success and error paths.
Extended reasoning...
Overview
This PR removes a CI dependency on how the local network routes packets to TEST-NET-1 (192.0.2.1). Three test files that previously assumed a connect to that address would hang forever now dial a local listener whose accept queue is deliberately filled, so the kernel drops further SYNs and the connect stays in SYN_SENT. The pattern is extracted into a new shared helper test/blackhole.ts (with a tsconfig.json path mapping) rather than duplicated. No production code is touched.
Security risks
None. This is test infrastructure only. The FFI use is limited to socket/bind/listen/getsockname/close on 127.0.0.1, following the same pattern already present in test/js/valkey/valkey-gc.test.ts and test/js/bun/dns/dns-interleave.test.ts.
Level of scrutiny
Low-to-medium. It's a test-only change fixing environment-dependent flakiness, but it introduces a shared helper with raw libc FFI and platform-specific sockaddr layout, so I checked it against the two existing in-tree copies of the same technique — the struct layout, AF_INET/SOCK_STREAM constants, and per-kernel backlog choice all match. The PR body documents verification on the failing host (bingus, 5/5 pass), a darwin-x64 host, Linux debug, and cites ss/netstat output confirming the fetch socket sits in SYN_SENT.
Other factors
- The valkey
connection-failureschange is a strict improvement independent of the flake fix: the oldexpect(async () => ...).toThrowErrorMatchingInlineSnapshot(...)was never awaited, so it could not fail. The new form awaits.rejectsand closes the client infinally. - Resource cleanup in the helper is sound: the fd and filler sockets are released via
[Symbol.dispose], and the helper's own error path callsdispose()before rethrowing. Callers useusingorafterAll. - Windows behavior is unchanged everywhere (the helper returns the same
192.0.2.1:80there), and the PR notes Windows CI has always passed this file. - CI on the PR passed the rewritten file across darwin-aarch64, debian x64 (+asan), alpine x64, and windows; the one unrelated failure also fails on main.
|
One correction to the review summary above: the old valkey assertion was not vacuous. In bun:test an unawaited |
Problem
fetch-abort-slow-connect.test.tsfails on darwin whenever shard 0 lands on the mac hostdarwin-arm64-bingus(builds 102348, 102370):expect(e.name).toBe("TimeoutError")at line 19 gets"TypeError"after 3.5 ms.192.0.2.1) to get a connect that never completes. That host, first seen in CI today, gets an ICMP unreachable back in 20 ms instead. The connect error beatsAbortSignal.timeout(50). On such a hostfetch-abort-ssl-context-eviction.test.tsruns with no connecting sockets, and the valkey timeout test gets a connect error.Fix
test/blackhole.tsexportsblackholeListener(): a raw libc listener on127.0.0.1that nobody accepts from, with eight connections of our own in its accept queue. The kernel drops the SYN of every later connect, so a dial sits inSYN_SENTuntil it is closed. The three tests dial it.register_abort_trackerafterconnectinsrc/http/lib.rs, eviction with active sockets, the valkey timeout).bun bd testhere, where the old slow-connect file fails. Onbingusthe old file fails 3 of 3 runs, the listener version passes 5 of 5. With the listener, the eviction fixture's 65 TLS fetches sit inSYN-SENT(ss).Background
bun:ffi, on thetest/mkfifo.tspattern. Linux admitsbacklog + 1connections. macOS treats 0 as the default, so the helper passes 1 there.Notes
darwin-aarch64-26.6.2-1, hostdarwin-arm64-bingus(172.92.21.195, LAN gateway 192.168.50.1, connected at 06:13 UTC). Build 102348 shard 0 ran on the same host and failed the same way. Shard 1 jobs on that host pass. Shard 0 jobs onhardtack,croutonandbreadstickpass. The agent namedarwin-aarch64-26.6.2-1is shared bybingusandhardtack.bingusserved none of the last 60 main builds (since Aug 19) while every other persistent mac did, so today's PR jobs were its first.192.0.2.1:80fails in 22 ms withNo route to host(EHOSTUNREACH).203.0.113.1getsNetwork is unreachable.198.51.100.1,240.0.0.1and10.255.255.1hang. Fromhardtack,192.0.2.1hangs for the full 5 s probe window.192.0.2.1at all. The old slow-connect file fails here too:connectfails synchronously withENETUNREACH(UnreachableErrorwith the release build), and under the debug build both connect tests fail. The eviction test passed here before this change with every connect already failed.[ DARWIN ] ... [ FLAKY ]entry totest/expectations.txtbecause the darwin agents of that time had no route to192.0.2.1, and asked for a local hanging connect as the follow-up. test: prune stale entries from expectations.txt #36780 removed the entry after a probe build passed. This PR is that follow-up.dns-interleave.test.tsandvalkey-gc.test.tsalready carry inline copies of the same listener and that the eviction test had the same dependence, so the listener moved totest/blackhole.tsand the two other tests were converted. The inline copies indns-interleave.test.ts(binds specific addresses inside a child script) andvalkey-gc.test.ts(inside a child script) andconnect-autoselectfamily-destroy-fixture.js(needs two addresses, skips itself today) are left for a follow-up. fetch: add an RFC 8305 per-attempt connect timer so a blackholed address does not stall for ~130 s #32949 adds another inline copy and could use the helper once this lands.binguswith bun 1.3.13: 3 of 3 runs fail the first test (Received: "Error", 3.3 to 4.0 ms). The listener version, byte for byte the code now intest/blackhole.ts, passes 5 of 5 runs there and 3 of 3 on a darwin x64 host (macOS 14).ss -tanandnetstat -an. Linux:LISTENwith recv-q 1, one fillerESTAB, seven fillers plus the fetch inSYN-SENT. macOS 26 onbingus: one fillerESTABLISHED, seven fillers plus the fetch inSYN_SENT. With 65 TLS fetches (distinctserverNames, as in the eviction fixture) against the listener,ssshows 72SYN-SENTsockets after 300 ms and all 65 reject withAbortErroron abort."Connection timeout reached after 2ms").expect(asyncFn).toThrow...in bun:test returns synchronously, so the old form could not be followed byclient.close(). The.rejectsform is awaited, verified to resolve only after the rejection, and asserts the same message.AbortSignal.timeout(1)) had the same dependence. It passed onbingusonly because the ICMP reply took longer than 1 ms.test/expected-durations.jsonlists it at 114 ms there), so TEST-NET-1 stays as the Windows branch inside the helper. A full accept queue on Windows producesECONNREFUSED(see the note intest/js/bun/http/request-smuggling.test.ts), not a pending connect. The eviction test now dials port 80 instead of 443 there, which makes no difference for a dropped SYN.src/http/lib.rs:2884refers to the slow-connect file by path. The path is unchanged, as are the entries intest/parallel-denylist.txtandtest/expected-durations.json.connection-failures.test.tsunderbun bd test(46 pass, 13 skipped for docker), the slow-connect file withUSE_SYSTEM_BUN=1, and six debug-build runs of it in a row.no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.