Repository navigation
test: wait for the server's handshake in the EISCONN tests of node-tls-connect.test.ts - #45866
dylan-conway wants to merge 1 commit into
Conversation
…s-connect.test.ts
|
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 5 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 1:05 AM PT - Oct 18th, 2026
@dylan-conway, your commit ad5340c 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.
LGTM — a one-line de-flake that awaits the server's secureConnection alongside the client's handshake event, so accepted is guaranteed to be 1 before client.connect() tears the socket down. The inline note about the server-side failure path not rejecting the awaited promise is a diagnosability nit, not a correctness problem with the fix.
Extended reasoning...
The change touches only test/js/node/tls/node-tls-connect.test.ts, replacing await once(client, established) with a Promise.all that also waits for the server's secureConnection in the three parameterized EISCONN tests; no runtime code or security surface is affected. The fix is sound because tls.createServer's connection listener (which increments accepted) is attached before the test's once listener on the same event, so accepted === 1 holds whenever the awaited promise resolves, and the assertions that follow are unchanged. The one inline finding concerns a hang-instead-of-fail on a server-side handshake failure that cannot occur with the self-signed cert and rejectUnauthorized: false used here, which is why it does not delay approval. The hunt ran dry rather than hitting a budget bound, and there are no third-party objections on the timeline.
| try { | ||
| await once(client, established); | ||
| // The server's handshake ends after the client's: it has yet to read the client's Finished. | ||
| await Promise.all([once(client, established), once(server, "secureConnection")]); |
There was a problem hiding this comment.
🟡 (optional) CI runners get a silent timeout hang instead of a diagnosable failure when the server side of the handshake fails after the client's side completed. At test/js/node/tls/node-tls-connect.test.ts:5631 the Promise.all awaits once(server, "secureConnection"), but a server-side handshake failure is reported through "tlsClientError" (src/js/node/net.ts:1361, 1495), never as a server "error", so events.once never rejects. The base branch awaited only the client and then failed fast at expect(accepted).toBe(1). Fix: wire every failure signal to reject the awaited promise: a server "tlsClientError" listener and a client "close"/"error" listener racing the Promise.all, as REVIEW.md requires for awaited conditions.
Why this was flagged
Trigger: the client finishes its TLS 1.3 handshake (once(client, established) resolves at test/js/node/tls/node-tls-connect.test.ts:5631) and the server then fails or never completes its side. Server failures are emitted as "tlsClientError" (net.ts:1361, net.ts:1495), not "error", so events.once(server, "secureConnection") never rejects. Once the client promise has settled, the error listener that events.once installed on the client is removed, so a later client "error" or "close" cannot reject the Promise.all either. The test then hangs until the per-test timeout with no message. On the base branch the same condition proceeded past the await, called client.connect, and failed quickly at expect(accepted).toBe(1) with "Expected: 1 Received: 0". REVIEW.md 'Tests reviewers reject' requires wiring every failure event to reject the awaited promise. Remedy: race the Promise.all against a rejecting listener on server "tlsClientError" and client "close".
Verification: test/js/node/tls/node-tls-connect.test.ts:5631 once(server, "secureConnection") has no failure wiring: in src/js/node/net.ts the handshake-failure path does server?.emit("tlsClientError", ...) (line 1495), never error on server, so it never settles and the test runs into the per-test timeout. On the base branch the same situation failed fast at expect(accepted).toBe(1) (line 5657).
|
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 the flaky "connect(port) on a TLSSocket that runs over a transport the caller handed in reports EISCONN" tests of
test/js/node/tls/node-tls-connect.test.ts. "tls.connect({ socket: net.Socket })" failed in the parallel batch and passed alone in build 127723 (macOS aarch64):acceptedcounts the server'ssecureConnectionevents, to show thatclient.connect(port)opened no second connection. The test waits for the client's handshake only. In TLS 1.3 the client is done when it has sent its Finished, and the server is done when it has read that. Everything the test does from there on is local to the client:connect()fails withEISCONN, the client closes, its transport closes. So the test can get toexpect(accepted)before the server has read the Finished.The test now waits for the server's
secureConnectionalong with the client's event.How did you verify your code works?
Linux x64, with a release binary CI built from main's code (d0831bc).
With a delay on the loopback interface (
tc qdisc add dev lo root netem delay ...in a network namespace), the three tests, main's file and this PR's in alternating rounds, 3 rounds each:Every failure is
Expected: 1, Received: 0.The tests still catch what they are for: with Bun 1.3.14 all three fail.
The whole file gives 352 pass, 1 skip, 1 todo and 1 fail. The one is "under node > a session that is set before the handshake starts is resumed", which runs under the
nodeof my machine (v26.7.0) and fails with main's file too.