Repository navigation
test: take the refused port of tls-shutdown-before-handshake-fixture.mjs from bind() - #45869
dylan-conway wants to merge 1 commit into
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 4 minutes. View limit details
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 |
|
Updated 9:38 PM PT - Oct 17th, 2026
@dylan-conway, your commit 96b560f is building: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked that Bun's own net.connect honors localAddress the way the comment assumes: src/js/node/net.ts internalConnect leaves the JS-side _handle.bind as a TODO, but kConnectTcp forwards localAddress to the native connect in src/runtime/socket/Listener.rs (around line 1192), which binds to that address with port 0 before connecting, so the holder's port really does come from bind() under both runtimes.
Extended reasoning...
Three-line change to a spawned test fixture only; no runtime or security-sensitive surface. The fix is consistent with the Linux connect()-autobind semantics it describes and leaves the socket under test and expected reports untouched; the remaining question for a human is whether the same-class sibling site flagged inline should land in this PR.
| // localAddress has bind() choose it: a port that connect() chose, Linux may also choose for | ||
| // `raw`, which then connects to itself. | ||
| const holder = net.connect({ port: peer.port, host: "127.0.0.1", localAddress: "127.0.0.1" }); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: the same Linux self-connect flake this PR fixes here stays in the sibling helper node-tls-wrapped-socket-close.test.ts:311-324, so that file keeps failing intermittently in CI. Its refusedPort() still takes holder.address().port from a connect()-chosen port at line 313, which a later connect() to that port can be handed as its own source port. Fix: hold the "refused" port with a bind()-chosen local port at every site that uses a live connection's local port as a port nothing listens on, i.e. pass localAddress: "127.0.0.1" to the holder in refusedPort() exactly as line 184 does here. Tests at node-tls-wrapped-socket-close.test.ts:424, :433 and :452 then expect raw error ECONNREFUSED from a socket that has in fact connected to itself.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
On Linux a connect() with no explicit bind may be given, as its source port, a port that another connect()-autobound socket already uses, as long as the four-tuples differ. node-tls-wrapped-socket-close.test.ts:313 creates holder with net.connect(await listen(sink), "127.0.0.1"), a connect()-chosen port, and line 316 hands holder.address().port out as a port nothing listens on. node-tls-wrapped-socket-close.test.ts:424, :433 and :452 then do net.connect(refused.port, "127.0.0.1") and await closed expecting raw error ECONNREFUSED (line 426, 435) or a close after the failure (line 455). When the kernel assigns that same port as the new socket's source port, the socket connects to itself, so the assertion fails or the test times out. The diff fixes only the copy of this pattern in tls-shutdown-before-handshake-fixture.mjs:184 by adding localAddress: "127.0.0.1" so bind() chooses the port; the base and this branch both leave the sibling helper on the connect()-chosen port, so that file flakes at the same per-attempt rate.
Verification: On Linux, an unbound connect() to 127.0.0.1:P is handed P itself as source port, so the "refused" connect self-connects. node-tls-wrapped-socket-close.test.ts is not in the diff: line 313 (no localAddress), line 316 handed out as the "refused" port. Lines 426 and 435 assert "raw error ECONNREFUSED", which a self-connected socket never emits. Already present on the base branch.
|
Folded into #45873. |
### What does this PR do? Fixes 20 causes of flaky tests, found in the annotations of the last 400 CI builds. Only files under `test/` change. No limit or deadline is raised, and no assertion is dropped except where the table says on which platform. Each row was first its own PR. That PR has the CI sightings, the cause and the numbers in full. | test file | what went wrong | what changes | write-up | |---|---|---|---| | `cli/install/bun-install-streaming-extract.test.ts` | the line taken for the error could be one of the verbose extract log | those lines are removed before the `error:` line is looked for | #45842 | | `cli/install/bun-install-streaming-extract.test.ts` | a body that is all there at the client's first read is not streamed, so `Streamed ` is not printed | the fixture is larger than one read of the client holds | #45844 | | `js/bun/shell/yield.test.ts` | Windows keeps the working directory of a killed process busy for a while (`EBUSY` on `rm`); the pid file exists before the pid is in it | the pid file is named by its full path and nothing has the directory as its cwd; the waiter waits for the file's content | #45846 | | `internal/harness-max-rss.test.ts` | a busy macOS machine no longer keeps the runner's 256 MiB resident when they are read | the two assertions on the runner's own RSS are for Linux, the one platform where a child inherits it | #45847 | | `js/web/fetch/body-clone.test.ts` | the FIFO case also wrote to a stdin that its child never reads (`EPIPE`) | that case gets no stdin | #45848 | | `js/first_party/ws/ws-server-compat-close.test.ts` | the count of sends that were written assumed the loopback takes bytes at once | it counts up to the last send that left nothing buffered | #45849 | | `js/bun/spawn/spawn-stdin-destroy.test.ts` | `sleep(80)` stood for "the child has exited" | `await child.exited` | #45850 | | `regression/issue/29181.test.ts` | file descriptors are closed on the work pool, so some are still open right after the last response | the count is polled until it is under the same limit, with a deadline | #45852 | | `js/web/fetch/fetch-keepalive.test.ts` | Windows Server 2019 delivers loopback bytes late while other loopback connections are busy, as macOS does | the five TCP cases are left out on `10.0.17763` as they are on macOS | #45853 | | `js/bun/jsc/bun-jsc.test.ts` | one run of 10 ms gets no sample when the sampler thread gets no CPU | `profile()` is repeated until it has a sample, with a deadline | #45854 | | `js/third_party/jsonwebtoken/async_sign.test.js` | two tokens signed on either side of a second differ in `iat` | the payload has a fixed `iat` | #45855 | | `js/bun/test/parallel/test-http-should-emit-events-in-the-right-order.ts` | the fixture asked a host on the internet | it asks a local server | #45856 | | `cli/run/transpiler-cache.test.ts` | a killed helper still had the temp directory as its cwd when the next test removed it (`EBUSY`, Windows) | the helper is held with `await using` | #45857 | | `js/web/fetch/fetch-abort-stream-body.test.ts` | `expect(p).rejects` in a socket callback runs a nested tick of the event loop, after which a concurrent test's `proc.stdout.text()` never settles | the promise is awaited | #45862 | | `js/bun/gc/gc-controller-cadence.test.ts` | the wait ended at `<= limit`, the assertion wants `< limit` | the wait goes on at `>= limit`, and the reading that ends it is the one asserted | #45863 | | `js/web/websocket/websocket-client.test.ts` | a limit of 1 s per test from 2023, now with a real closing handshake with a `node` process inside it | the limit is removed, the runner's applies | #45864 | | `js/node/tls/node-tls-connect.test.ts` | in TLS 1.3 the server's handshake ends after the client's, and the test waited for the client's | it waits for both | #45866 | | `js/node/tls/tls-shutdown-before-handshake-fixture.mjs` | the "refused" port was one `connect()` chose, which Linux can hand to the socket under test as its own: it connects to itself | `bind()` chooses the port | #45869 | | `js/bun/module-graph/module-graph-io.test.ts` | on macOS `fs.watch()` returns before anything is watched, and the case wrote once | it writes until the listener is called | #45870 | | `js/bun/module-graph/module-graph-io.test.ts` | the closed UDP port can be given to the socket under test, which then hears itself and is never refused | the port stays bound until that socket exists | #45871 | ### How did you verify your code works? Each change on its own, with release and ASAN binaries that CI built from main's code: the failure reproduced with main's file (under load, or with the losing order forced by a delayed loopback, a blocked thread, a stopped process or a narrow port range), the same conditions with the fix, and a negative test that shows the test still fails on what it guards. Linux x64 locally, Windows 11 x64 for the Windows rows. Some of the numbers: | | main | fix | |---|---|---| | `yield.test.ts`, Windows, 96 at a time | 2878 of 4000 fail | 0 of 4000 | | `transpiler-cache.test.ts`, Windows | 99 of 120 | 0 of 120 | | `bun-install-streaming-extract.test.ts` (body size), Windows | 326 of 6000 | 0 of 6000 | | `29181.test.ts` | 143 of 280 | 0 of 280 | | `bun-jsc.test.ts`, contended CPU | 111 of 200 | 0 of 200 | | `node-tls-connect.test.ts`, loopback delayed by 5 ms | 3 of 3 in each round | 0 | | `tls-shutdown-before-handshake-fixture.mjs`, 32 ports, node and Bun | 12 of 100 each | 0 of 100 each | All of it together, on Linux x64: every changed file passes, but for two tests that fail with main's files on my machine too ("a fifo in place of an entry is removed instead of opened" in `transpiler-cache.test.ts`, and "under node > a session that is set before the handshake starts is resumed" in `node-tls-connect.test.ts`, which runs under my `node` v26.7.0). Not run on the platform they are for: the macOS rows (`harness-max-rss`, `ws-server-compat-close`, the `fs.watch` case), which rest on the CI failures, the source and an emulation on Linux. The Server 2019 numbers of `fetch-keepalive` are from a CI run.
What does this PR do?
Fixes a flake of
test/js/node/tls/node-tls-connect.test.ts. "end() and destroySoon() before the handshake completes (node) > a tls.connect({ socket }) transport that closes before it connects closes the socket" failed and passed on a retry in build 127629 (Debian 13 x64, ASAN lane). The fixture ran undernodethere, and its report for "end refused" waswhere
["close"]andfalseare expected: the connection that was to be refused had connected.For a port that nothing listens on,
tls-shutdown-before-handshake-fixture.mjstakes the local port of a live outgoing connection (holder.localPort). That port was chosen byconnect(), and Linux may choose such a port again for anotherconnect()as long as the four addresses differ. When it givesraw, which connects to127.0.0.1:P, that samePas its source port,rawconnects to itself.holdernow passeslocalAddress, which hasbind()choose its port, andconnect()does not pick a port thatbind()holds.raw, the socket under test, and the expected reports stay as they are.How did you verify your code works?
Linux x64,
nodev26.7.0 and a release binary of Bun that CI built from main's code (d0831bc). Both behave the same.To make it frequent, the runs are in a network namespace with few ephemeral ports (
ip_local_port_range).The fixture itself (
closed-transport), 32 ports. Runs whose report has afinish:The mechanism alone, 256 ports: a script that opens such a
holder, connects to its local port, and notes whether it was refused orlocalPort === remotePort, 300 times:connect()chose the holder's portbind()chose itThe tests that use the fixture pass (43 pass). With Bun 1.3.14 the fixture's shapes report
readyState: "opening", destroyed: false, so the test still fails there.