test: cover terminate() on a TLS socket sending a bare RST - #40040
test: cover terminate() on a TLS socket sending a bare RST#40040robobun wants to merge 1 commit into
Conversation
Run the in-process terminate() case of "close() error after the peer resets the connection" for tls as well as tcp. A TLS terminate() sends no close_notify since #39632, so the reading peer gets read ECONNRESET and not a clean close. Before that change the tls case fails: the peer consumes the close_notify queued ahead of the RST as a clean end.
|
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 9 minutes), 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 (1)
Comment |
|
Adopted from #40006 at the request of @Jarred-Sumner. Test-only PR: the fix it proves is already on main (#39632). Reproduced: with CI (build 103178): |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only change that extends the existing in-process terminate() reset test to run under TLS as well as TCP.
What was reviewed:
- The test now sits inside the
describe.each(["tcp", "tls"])block and mirrors the sibling child-process test's tls setup (open vs handshake gating,{ ca: tls.cert }on connect) — patterns match line for line. - Failure paths are wired to reject:
connectError, the newerrorhandler, and handshake failure all reject the awaited promises, so a regression fails rather than hangs. - No sleeps,
port: 0,using listener, and the assertion (closeErrorShape→readReset) is unchanged from the tcp-only version.
Extended reasoning...
Overview
This PR moves the "a connected socket reports the reset the same way" test from outside the describe.each(["tcp", "tls"]) block into it, so the in-process terminate() case now runs for TLS in addition to plain TCP. The only additions beyond the move are the tls-conditional tls: options on both listen and connect, a handshake handler that mirrors the sibling test's, and an error handler on the client that rejects the greeted resolver. The stale comment claiming an in-process TLS terminate() was unusable is replaced. There are no src/ or packages/ changes — the fix (#39632) is already on main; this PR supplies the missing regression test for it.
Security risks
None. Test-only change touching a socket test file. No new fixtures, no external network, no credentials, no changes to TLS validation logic — the test uses the harness tls cert already in use by the neighboring test.
Level of scrutiny
Low. This is a coverage addition for behavior already shipped. The new TLS branch is a copy of the pattern used ~40 lines above in the same describe.each (open→greet for tcp, handshake→greet for tls, reject on handshake failure). The PR description documents fail-before (reverting the openssl.c guard fails the tls case; bun 1.3.13 fails 20/20) and pass-after (5/5 on debug main; canary 20/20), which satisfies the "prove the test fails for the right reason" bar.
Other factors
The test follows the repo's conventions: port: 0, using for the listener, awaits observable conditions (accepted, greeted, closedWith) with every failure event wired to reject rather than hang, and asserts the same readReset shape via toEqual. The added error handler on the client is a small hardening over the original tcp-only version. The handshake callback is not invoked for plain tcp sockets, so the tcp path is unchanged in behavior. The PR notes a trivial one-hunk conflict with #39653 on the comment lines, which is expected and not a blocker.
|
Updated 9:19 PM PT - Aug 21st, 2026
❌ @robobun, your commit 8971d7c has 1 failures in
🧪 To try this PR locally: bunx bun-pr 40040That installs a local version of the PR into your bun-40040 --bun |
2 participants
Problem
terminate()on a TLS socket send only an RST, with no close_notify. No test in the tree fails when that change is reverted: the tls reset tests innode-tls-server.test.tsandsocket.test.tspass either way on Linux.Fix
test/js/bun/net/socket.test.ts: the in-processterminate()case in "close() error after the peer resets the connection" now runs for tls as well as tcp. The accepted TLS socket callsterminate(), and the connected peer must getclose(socket, error)withread ECONNRESET.terminate()is not usable here was stale and is removed.LIBUS_SOCKET_CLOSE_CODE_CONNECTION_RESETcheck atpackages/bun-usockets/src/crypto/openssl.c:1960reverted fails the tls case (reported: false, no code). The same build with the line restored passes 5 of 5 runs. Alsobun testwith bun 1.3.13 fails 20 of 20, and the 1.4.0 canary passes 20 of 20.Background
terminate()closes with code 1 (CloseCode::failure):SO_LINGER{1,0}so the kernel sends an RST. For TLS,us_internal_ssl_closeused to runSSL_shutdownfirst for every non-zero code, which wrote a close_notify alert before the RST.SSL_ERROR_ZERO_RETURNand closes cleanly. Only a bare RST reaches it as a read error, which is what node'sresetAndDestroy()produces.Notes
src/orpackages/change. The fix is on main (4199361). The test is the missing proof for it.if (ssl_handle_shutdown(s, code != 0))in place ofif (code == LIBUS_SOCKET_CLOSE_CODE_CONNECTION_RESET || ssl_handle_shutdown(s, code != 0)). The tcp case and the two child-process cases pass both ways.socketblock,localhostresolution and external network), as noted in socket: report a peer reset on Windows as close(socket, ECONNRESET) instead of a code-less error #39615.socket.test.tsand adds no test. Whichever lands second has a one-hunk merge.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/net/socket.test.ts