Conversation
…killing a child
The tcp and tls "accepted socket reports the reset as read ECONNRESET" tests
spawned a peer process, wrote to it, and killed it with SIGKILL so the kernel
would close its socket with unread data and send an RST. Over loopback the
delivery of that last write is asynchronous, and the teardown after the kill
takes about 0.7ms on the Intel CI minis. When delivery is slower the peer's
receive buffer is empty at close, the kernel sends a FIN, and the accepted
socket closes cleanly. The test then fails with no error code.
The peer now resets in-process with terminate(). It closes with SO_LINGER{1,0},
so the kernel sends a bare RST whatever is queued, and a TLS terminate() sends
no close_notify since #39632. The accepted side takes the same path as before:
a poll error, recv() ECONNRESET, close(socket, error) with code ECONNRESET and
syscall read.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughChangesThe TLS/TCP accepted-socket reset test now uses an in-process client. The client waits for the greeting, calls Accepted socket reset test
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only de-flake that replaces a racy SIGKILL-based RST with a deterministic terminate() (SO_LINGER{1,0}) reset.
What was reviewed:
- The assertion is unchanged (
closeErrorShapestill expectsread ECONNRESET), so the test is not weakened — same kernel path (tcp_drop), just without the loopback delivery race. - Failure paths on the new in-process peer are wired:
error/connectError/closeall rejectgreeted, and the server's handshake failure now rejectsclosedWith(the only awaited promise) instead of the removedready. - Checked that the peer's
closefiring afterterminate()rejecting an already-resolvedgreetedis a no-op, not an unhandled rejection. - No production code touched; the removed
tempDir/subprocess machinery is fully replaced, no dead helpers left behind.
Extended reasoning...
Overview
This PR touches a single test in test/js/bun/net/socket.test.ts — the "accepted socket reports the reset as read ECONNRESET" case inside the close() error after the peer resets the connection block. The old test spawned a child process, wrote to it after a ready handshake, then SIGKILL'd it so the kernel would close the peer socket with unread data and emit an RST. The flake root cause (documented in exhaustive detail in the PR description with tcpdump captures and a dummynet repro) is that loopback delivery on macOS is asynchronous through the DLIL input thread: when the write hadn't landed in the peer's receive buffer by the time SIGKILL teardown ran, xnu sent a FIN instead of an RST and the test observed a clean close.
The fix replaces the child process with an in-process Bun.connect peer that calls terminate() after receiving the server's greeting. terminate() closes with SO_LINGER{1,0}, which forces an RST regardless of receive-buffer state — the same tcp_drop kernel path the SIGKILL approach reached, but deterministically. The PR notes this is now viable for the TLS variant because #39632 made TLS terminate() skip the close_notify that previously turned the reset into a clean end on the reading side.
Security risks
None. Test-only change to a socket test file; no production code, no new inputs, no external network.
Level of scrutiny
Low-to-medium. This is a test de-flake with no runtime code changes. The key REVIEW.md concerns for test edits are (1) not weakening the assertion and (2) not routing around a real runtime bug. Both are satisfied: the assertion is byte-identical, and the PR description proves the flake is a test-construction race (async loopback delivery vs. SIGKILL timing), not a runtime defect — the same RST-on-close behavior is exercised via the documented equivalent kernel path. The new test is also strictly better on the harness axes: no subprocess spawn, no temp dir, faster, and every failure event on both sides is wired to reject the awaited promise.
Other factors
- The PR description includes cross-platform verification (darwin-x64 with a 3 ms dummynet delay reproducing the flake 3/3 on the old test and 0/3 on the new; Linux debug; Windows x64 canary 5/5).
- I checked the one subtle interaction: after
peer.terminate(), the peer's ownclosehandler fires and callsgreeted.reject(...), butgreetedwas already resolved bydata— rejecting a settled withResolvers promise is a no-op, not an unhandled rejection. - The removed
readypromise's rejection sites (peer premature exit, early server close) are correctly folded into the remaininggreetedandclosedWithresolvers, so no failure path is left un-awaited. - No prior reviews or comments on the timeline; the bug-hunting system found nothing. The noted overlap with #40040 and #39653 is a one-hunk textual conflict, not a semantic one.
|
Updated 11:27 AM PT - Aug 22nd, 2026
✅ @robobun, your commit 6ed6df6860ff70975306981a2ef3a2aa7eae1bce passed in 🧪 To try this PR locally: bunx bun-pr 40108That installs a local version of the PR into your bun-40108 --bun |
There was a problem hiding this comment.
LGTM — test-only de-flake that replaces a race-prone SIGKILL-based RST with an in-process terminate(), keeping the same assertion.
What was reviewed:
- The new test still asserts the identical
{ reported: true, code: "ECONNRESET", syscall: "read" }shape — no weakening; the kernel path (SO_LINGER{1,0} →tcp_drop→ bare RST) exercises the same usockets poll-error →recv()ECONNRESET →close(socket, error)sequence the original protected. - All failure paths on both sides wire to a reject: server
handshakefailure rejectsclosedWith; clienterror/connectError/closerejectgreeted. The clientclosefiring afterterminate()hits an already-settledgreeted, which is a no-op. - Waiting on the client's
data(the greeting) is the correct readiness signal for both transports — for tls the server writes it only afterhandshake(success), so both handshakes are complete beforeterminate(). - No sleeps, no timeouts,
port: 0,using listenerfor cleanup; removed child-process machinery is fully deleted.
Extended reasoning...
Overview
This PR touches a single test in test/js/bun/net/socket.test.ts: the "accepted socket reports the reset as read ECONNRESET" case inside the close() error after the peer resets the connection block. The old test spawned a child process, wrote data to it, then SIGKILLed it so the kernel would close its socket with unread data and emit an RST. The new test connects a client in-process, waits for the server's greeting, and calls socket.terminate(), which closes with SO_LINGER{1,0} and sends a bare RST. The assertion (closeErrorShape(await closedWith.promise) equals readReset) is unchanged.
Security risks
None. Test-only change with no production code touched. No new external inputs, no network hosts contacted, no new dependencies.
Level of scrutiny
Low-to-medium. This is a de-flake, and the repo's review rules call out "when de-flaking, keep asserting the property the original assertion protected" — this does exactly that. The PR description root-causes the flake precisely (xnu's tcp_disconnect sends FIN when the receive buffer is empty; loopback delivery on macOS is asynchronous through the DLIL input thread; a 3 ms dummynet delay reproduces the CI failure deterministically), and the replacement mechanism reaches the same kernel path (tcp_drop) without depending on write-delivery timing. The comment that was removed (explaining why an in-process terminate wasn't usable for TLS because it wrote close_notify) is correctly obsoleted by #39632, and the new comment cites that.
Other factors
- The test structure follows the file's existing conventions (
Promise.withResolvers,using listener, event-driven awaits, no sleeps). It is now simpler and faster (no subprocess spawn, no tempDir). - Every failure event is wired to reject the awaited promise; the one benign race (client
closefiring aftergreetedis already resolved) is a no-op on a settled promise. - The sibling test in the same block ("a connected socket reports the reset the same way (tcp)") already used in-process
terminate(), so this change also brings the two tests into alignment. - The bug-hunting system found nothing. CodeRabbit found nothing actionable. No prior human review comments to address.
|
CI status: both runs (103621 and 103631) ran |
There was a problem hiding this comment.
LGTM — deterministic de-flake of the accepted-socket ECONNRESET test.
What was reviewed:
- Confirmed the new in-process
terminate()(SO_LINGER{1,0} → bare RST) exercises the same accepted-side path the SIGKILL variant did, and the assertion (read ECONNRESETshape) is unchanged for both tcp and tls. - Checked that every failure path is wired to reject the awaited promise (
greeted: error/connectError/close;closedWith: handshake failure), and that the peer's post-terminatecloserejecting an already-settledgreetedis a no-op. - Verified
tempDir/bunEnvare still used elsewhere in the file (no dead imports), and the sibling connected-socket test already uses the same in-processterminate()pattern.
Extended reasoning...
Overview
This PR rewrites one test in test/js/bun/net/socket.test.ts — "the accepted socket reports the reset as read ECONNRESET" — to trigger the peer's RST via an in-process Bun.connect client calling socket.terminate(), instead of spawning a child process, writing to it, and killing it with SIGKILL. The child-process approach relied on the write reaching the peer's receive buffer before the kill so xnu would send an RST rather than a FIN; over macOS loopback that delivery is asynchronous and raced the process teardown, producing 14 CI failures on darwin x64. terminate() closes with SO_LINGER{1,0}, which sends a bare RST unconditionally, and since #39632 the TLS variant no longer writes a close_notify first — removing the original reason a subprocess was needed.
Security risks
None. Test-only change with no production code touched; no new inputs, no external network, no credentials.
Level of scrutiny
Low-to-moderate. This is a test de-flake, not a behavior change. The key review question is whether the rewrite preserves what the test protects — that the accepted socket's close(socket, error) reports { code: 'ECONNRESET', syscall: 'read' } when the peer resets. The assertion is byte-identical; both trigger paths reach the same kernel tcp_drop and the same usockets poll-error → recv ECONNRESET → close-with-error sequence. The PR description documents a reproduction (dummynet 3ms delay makes the old test fail 3/3 and the new test pass 3/3 on the affected hardware), and CI ran the file green on every lane including both darwin x64 shards across two builds.
Other factors
- All await points have failure paths wired to reject:
greetedis rejected by the client'serror,connectError, andclosehandlers;closedWithis rejected by a server handshake failure. The client'sclosefiring afterterminate()re-rejects an already-resolvedgreeted, which is a no-op. - The subprocess removal drops the write-then-kill dance entirely; the extra
accepted.write(...)existed only to seed unread data for the SIGKILL RST and is not part of the property under test. - The sibling test in the same describe block ("a connected socket reports the reset the same way (tcp)") already used in-process
terminate(), so this change also brings the two into a consistent pattern. tempDirandbunEnvremain used elsewhere in the file (33 occurrences), so no dead imports were introduced.- The PR description flags one-hunk conflicts with #40040 and #39653; those are trivial and expected.
This branch has not been deployed
1 participant
Problem
test/js/bun/net/socket.test.ts>close() error after the peer resets the connection > tcp > the accepted socket reports the reset as read ECONNRESETfails on the darwin x64 lane:closeErrorShape(...)gets{ reported: false, code: undefined, syscall: undefined }instead ofread ECONNRESET. 14 builds between 08:44Z and 13:20Z today, for example 103585.tcp_disconnect), and the accepted socket reads a clean end. The RST that follows arrives too late.Fix
terminate()for tcp and tls. It closes withSO_LINGER{1,0}, so the kernel sends a bare RST whatever is queued. A TLSterminate()sends no close_notify since tls: send a bare RST from terminate(), no close_notify #39632, which is why the child process was needed before.recv()ECONNRESET,close(socket, error)withread ECONNRESET. The kernel path is the same too: a kill with unread data and anSO_LINGERclose both reachtcp_drop.Background
close()sends an RST only if the receive buffer holds unread data orSO_LINGERis set with a zero timeout. Otherwise it sends a FIN.send()returns before the peer's socket has the data. Under load that delay reached 0.8 ms on the CI minis.recv() == 0, which ends the socket cleanly. An RST is reported as a poll error withECONNRESET.Notes
ready, then the peer's FIN 1.0 ms afterready, then the payload 3.5 ms later, then three RSTs from the peer.dummynet out quick on lo0 proto tcp from any to any user root pipe 1,dnctl pipe 1 config delay 3), so CI jobs on the host were not affected. pf was disabled and flushed afterwards.lo_async_probe.py(write, then FIONREAD on the other end) showed the data absent right aftersend()in 30 to 98 percent of 3000 tries, with a delay of 4 to 160 us idle and up to 787 us under 16 CPU burners plus process churn.localhostresolution and external network), as noted in test: cover terminate() on a TLS socket sending a bare RST #40040.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