socket: report a peer reset on Windows as close(socket, ECONNRESET) instead of a code-less error - #39615
Conversation
|
Updated 12:10 AM PT - Aug 19th, 2026
⏳ @robobun, your commit 6bc25bc is still building in
|
|
Status: reproduced on a Windows x64 machine. With the released binary, Proof on a Windows x64 debug build of the current branch (6bc25bc, rebased on main after #39610 landed): the three new tests in CI on the pre-rebase sha aad7e50 (build 101072) passed on every Windows lane. Its two red lanes were the darwin paused-reset test that #39610 has since fixed and the asan |
|
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 (4)
Included review availability: Your plan provides up to 5 included reviews per hour; 3 remain after this review. WalkthroughThis change standardizes socket error fallbacks across platforms, converts close codes into normalized read errors, and adds TCP and TLS regression tests for peer resets. ChangesSocket error normalization
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — the previous nit (wiring peer.exited to ready.reject) is addressed in e378ee3.
What was reviewed:
- Verified the C macro changes are byte-identical on POSIX (
LIBUS_ECONNRESET→ECONNRESET,LIBUS_ERR→errno) and only shift Windows to WSA numbering. - Checked
us_socket_get_error(libuv.c) — Winsockgetsockoptfailure reports viaWSAGetLastError(), noterrno, so the swap is correct; all other callers only compare!= 0. - Confirmed
read_error_from_close_codemirrors the existing connect-error mapping (SystemErrno::init(u32)at socket_body.rs:1163) andsys::Error::new(SystemErrno, Tag)exists; the POSIX arm is identical to the old inline call.
Extended reasoning...
Overview
Six files touched: a new LIBUS_ECONNRESET macro in internal.h, three C sites in usockets swapped from bare ECONNRESET/errno to the platform-numbered LIBUS_ECONNRESET/LIBUS_ERR, a new read_error_from_close_code helper in socket_body.rs that maps WSA codes to SystemErrno on Windows only, and three regression tests in socket.test.ts.
Security risks
None. This only changes which errno constant a peer-initiated close carries into JS. No parsing of untrusted input, no allocation, no auth/crypto.
Level of scrutiny
The diff sits in the socket event-loop hot path, so I checked each change against both platforms. On POSIX every C change is a macro that expands to the exact token it replaced (LIBUS_ERR ≡ errno, LIBUS_ECONNRESET ≡ ECONNRESET), and the Rust POSIX arm is the previous body verbatim — so POSIX behaviour is provably unchanged. On Windows: us_socket_resume's failure branch is unreachable (libuv us_poll_change always returns 0), the SEMI_SOCKET connect fallback already had the #ifdef for WSAECONNRESET, and the remaining two sites (us_socket_get_error failure return and the poll-error close fallback) fix real numbering mismatches the PR describes. The Rust Windows arm follows the same SystemErrno::init(u32) pattern already used for connect errors at line 1163, and folds WSAECONNABORTED into ECONNRESET per libuv's uv__process_tcp_read_req.
Other factors
The tests are well-constructed: the child-process kill guarantees an RST with nothing queued ahead (avoiding the TLS close_notify clean-end problem the comment explains), every failure event including peer.exited is wired to reject the awaited promise, resources use using/await using, and describe.concurrent matches the file's existing convention. The author verified fail-without/pass-with on a Windows debug build and 25/25 flake-free repeats on Linux; CI on aad7e50 was green on all Windows lanes with the two red lanes attributed to unrelated known issues. The PR description enumerates every other consumer of the close code and confirms none read it as an error. My earlier review's only nit was addressed and the thread is resolved.
|
@robobun fix conflicts |
…ies ECONNRESET On Windows usockets closes a reset socket with the WSA code (WSAECONNRESET, 10054) from recv() or SO_ERROR. on_close stored that value as a SystemErrno discriminant, so the error reached JS without a code. Map it through the WSA table the way the connect error path does. WSAECONNABORTED is reported as ECONNRESET like libuv does on its read path, and a code the table does not name is reported as ECONNRESET as well. The fallbacks usockets uses when SO_ERROR reports nothing used the CRT's ECONNRESET (108 on Windows), which reads as ESHUTDOWN in the SystemErrno numbering. They now use LIBUS_ECONNRESET, which is WSAECONNRESET on Windows, and us_socket_get_error reports a failed getsockopt with LIBUS_ERR.
e378ee3 to
6bc25bc
Compare
|
Rebased on main (6bc25bc). The conflict was in |
Problem
close(socket, error)after a peer reset gets an error with nocode(errno: -10054, messageUnknown Error, read). Linux and macOS reportECONNRESET.WSAECONNRESET= 10054).on_closeinsrc/runtime/socket/socket_body.rsstores it unmapped withsys::Error::from_code_int, which on Windows holdsSystemErrnodiscriminants. 10054 is not one.loop.cis the CRT'sECONNRESET, 108 on Windows. Discriminant 108 isESHUTDOWN.Fix
on_closemaps the code through the WSA table, as the connect error path does.WSAECONNABORTEDbecomesECONNRESET, as in libuv's read path (uv__process_tcp_read_req), so the code matches node. An unknown code becomesECONNRESETtoo: the connection is gone either way.loop.candus_socket_resumeuse the newLIBUS_ECONNRESET(WSAECONNRESETon Windows), and the libuvus_socket_get_errorreturnsLIBUS_ERRwhengetsockoptfails. All close codes now share one numbering per platform.test/js/bun/net/socket.test.ts, plus the two paused-reset tests from usockets: report a peer reset on a paused socket on kqueue #39610, which now check the code on Windows too. A Windows x64 debug build fails them without thesrcandpackagesdiff and passes with it. Linux passes both ways. The net and tls reset suites pass on both platforms (notes).Background
recv()fails or the poll reports an error, and the close code is then the error (LIBUS_ERR,SO_ERROR, or a fallback). Codes 0 to 2 are closes that Bun started.NewSocket::on_closepasses a larger code to the JSclosehandler aserror.SystemErrnouses Linux numbering.SystemErrno::initmaps a Win32 or WSA code onto it, andsys::Errorstores the mapped value.Notes
Test design: the peer is a child process killed while it has unread data, so the kernel sends an RST with nothing queued ahead of it. An in-process TLS
terminate()sends a close_notify first, which a reading POSIX peer consumes as a clean end, so it cannot stand in for the reset in the tls case. The third test resets a plain tcp connection from the server side in-process and checks theBun.connectsocket, which shareson_close.Error shape on Windows x64 (
Bun.listensocket, clientterminate()), tcp and tls give the same result:{ code: undefined, errno: -10054, syscall: "read", message: "Unknown Error, read" }{ code: "ECONNRESET", errno: -4077, syscall: "read", message: "ECONNRESET: connection reset by peer, read" }netserver socket:{ code: 'ECONNRESET', errno: -4077, syscall: 'read' }The
ESHUTDOWNflavor: the poll-error close inloop.cis reached on Windows when libuv reports an error status for the poll. Windows does not reliably latch a received RST inSO_ERROR(seeus_internal_libuv_peer_reset_probe), so the fallback is taken there. #37104 observed it aserror=ESHUTDOWNin its test matrix. From JS, a reset on a reading or paused socket goes through therecv()close on Windows (the paused probe inpoll_cbadds READABLE, and Windows discards the receive queue on a reset), so the new tests cover therecv()flavor and the fallback is fixed by inspection. A reset that arrives after the accepted socket consumed the peer's FIN (half open, polling nothing) is not reported at all on Windows, with or without this change. That is a separate defect and is not touched here.src/js/node/net.tskeeps itscode === undefinedbranches. With this change it takes thecode === "ECONNRESET"branch on Windows and reportserrno: -4077like node, instead of-10054.Other consumers of the close code were checked: the HTTP client, WebSocket client, Postgres, MySQL, Valkey, IPC and uWS ignore the value (uWS WebSocket uses it as a reason length for its own closes).
NewSocket::on_closeis the only consumer that reads it as an error.Related PRs: #39579 adds
LIBUS_ECONNABORTEDandLIBUS_ECONNREFUSEDto the same block ofinternal.hfor the connect path, andLIBUS_ECONNRESETfollows that shape. Whichever lands second has a small merge ininternal.h. #39610 landed first. Its paused-reset tests insocket.test.tschecked the code on POSIX only because of this bug, and the rebase removes that guard, so they are part of the proof now (they observedcode: undefinedon both Windows lanes in that PR's CI).Suites run on the Windows x64 debug build:
test/js/bun/net/socket.test.ts(82 pass, 9 skip),test/js/node/net/node-net.test.ts(78 pass, 1 fail: "should allow reconnecting after end()" is a 3 ms timer race with no reset in it, and it passes in 2 of 4 runs on this build), the 4 reset tests intest/js/node/tls/node-tls-server.test.ts, and the 17 files intest/js/node/test/parallelwhose names match reset, econnreset or error-twice. On the Linux debug build:socket.test.ts(the same 9 failures as the released binary in this container:localhostresolution and external network), thenode-tls-serverreset tests,test-net-error-twice,test-net-server-reset,test-net-socket-reset-send,test-net-socket-reset-twice,test-tls-econnreset,test-tls-wrap-econnreset,test-tls-wrap-econnreset-socketandtest-http-conn-reset. The new tests passed 25 of 25 repeated runs on Linux with the released binary.