Bun.connect: native write() returns -1 (not raw -errno) on a peer RST and close carries ECONNRESET - #36422
Bun.connect: native write() returns -1 (not raw -errno) on a peer RST and close carries ECONNRESET#36422robobun wants to merge 12 commits into
Conversation
…se carries ECONNRESET When the peer RSTs while JS is in a synchronous write burst, the first failing send() is observed by write_maybe_corked (ECONNRESET, then EPIPE). Since #32488 the native write()/end() host functions returned that raw negative errno to JS, contradicting the documented -1 sentinel, and the errno was then discarded: the next poll reported EOF, so the socket fired 'end' and close(undefined), indistinguishable from a graceful FIN. Record the first fatal send errno on the socket. The native write()/end() return is clamped to -1. on_end skips the bogus FIN dispatch when a fatal send was already observed, and on_close reports the recorded errno as the close error (syscall 'write') when the loop itself saw only a clean HUP. node:net already guards its close-error path with _hadError, so the write_buffered caller that consumes the negative errno is unaffected.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesFatal write errno state is added to socket objects, initialized across construction and reuse paths, and propagated through close handling. Write results are clamped to Socket write error handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/socket/Listener.rs`:
- Line 1537: Reset fatal_write_errno whenever a socket wrapper is reused for a
new connection. In src/runtime/socket/Listener.rs:1537, clear
prev.fatal_write_errno in the maybe_previous branch; apply the same reset in the
prev_maybe_tls reuse branch at src/runtime/socket/Listener.rs:1207 and the
prev_maybe_tcp reuse branch at src/runtime/socket/Listener.rs:1294, while
preserving errno persistence until each transport closes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2da05713-59e9-4394-83c6-261d5d4d92a2
📒 Files selected for processing (4)
src/runtime/node/node_net_binding.rssrc/runtime/socket/Listener.rssrc/runtime/socket/socket_body.rstest/js/bun/net/socket.test.ts
…tarts clean on_close consumes it with replace(0) before any early return, and detach_for_reconnect (the node:net reconnect-while-live path, whose close dispatch is suppressed) clears it alongside the other per-transport state.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/socket/socket_body.rs (1)
2467-2472: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRecord fatal errno in every flush path.
This records only immediate
write_maybe_corkedfailures. Buffered/drain writes useinternal_flush, whose fatal branch at Lines 2975-2982 returns the errno without settingfatal_write_errno;on_openandflushdiscard that return, whileon_writablecloses with the field still zero. Consequently, a peer reset during a buffered write can still produce a cleanclose/endpath without the promisedsyscall: "write"error.Add the same first-error recording before returning from
internal_flush.Proposed fix
if fatal_errno != 0 { + if self.fatal_write_errno.get() == 0 { + self.fatal_write_errno.set(fatal_errno); + } self.buffered_data_for_node_net .with_mut(|b| b.clear_and_free()); return fatal_errno; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/socket/socket_body.rs` around lines 2467 - 2472, Update the fatal-error branch of internal_flush to record fatal_errno in fatal_write_errno before returning, using the same first-error-only guard as the immediate write_maybe_corked path. Preserve the existing errno return behavior so buffered and drain flushes expose the write failure through the later close/end handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/runtime/socket/socket_body.rs`:
- Around line 2467-2472: Update the fatal-error branch of internal_flush to
record fatal_errno in fatal_write_errno before returning, using the same
first-error-only guard as the immediate write_maybe_corked path. Preserve the
existing errno return behavior so buffered and drain flushes expose the write
failure through the later close/end handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 86ca97a2-8ccf-41a1-b5b1-6ebaeb13c375
📒 Files selected for processing (1)
src/runtime/socket/socket_body.rs
… its return still surface the reset at close
|
Took the outside-diff suggestion in 6b03bd2: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/socket/socket_body.rs (1)
2459-2461: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRecord fatal errors from buffered flushes too.
The new state is recorded only in
write_maybe_corked, butinternal_flush()can detect the same fatalwrite_check_errorresult and return the errno without settingfatal_write_errno. After a buffered retry,on_writable()closes the socket, buton_close()then sees zero and cannot emit the promisedsyscall: "write"close error. Store the first errno ininternal_flush()before returning it; also ensure the publicflush()path does not discard this fatal result.Proposed fix
if fatal_errno != 0 { + if self.fatal_write_errno.get() == 0 { + self.fatal_write_errno.set(fatal_errno); + } self.buffered_data_for_node_net .with_mut(|b| b.clear_and_free()); return fatal_errno; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/socket/socket_body.rs` around lines 2459 - 2461, Update internal_flush() to store the first fatal errno in fatal_write_errno before returning a fatal write_check_error result, matching write_maybe_corked’s existing behavior. Ensure the public flush() path propagates that fatal result instead of discarding it, so on_close() can emit the promised write syscall error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/runtime/socket/socket_body.rs`:
- Around line 2459-2461: Update internal_flush() to store the first fatal errno
in fatal_write_errno before returning a fatal write_check_error result, matching
write_maybe_corked’s existing behavior. Ensure the public flush() path
propagates that fatal result instead of discarding it, so on_close() can emit
the promised write syscall error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2f5538d9-ce08-4ece-9a8d-29c5bc1c1c8b
📒 Files selected for processing (1)
src/runtime/socket/socket_body.rs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…s it With allow_half_open (every node:net socket), loop.c only re-arms WRITABLE after dispatching on_end, and the failing send() has already consumed sk_err so no EPOLLERR follows. Suppressing the dispatch without closing left the socket spinning on level-triggered EPOLLHUP (test-net-GH-5504 hang). Closing here routes to on_close, which reports the recorded errno; the non-half-open path's second close is a no-op on an already-closed socket. Also branch the macOS assertion: XNU's send() returns EPIPE without clearing so_error, so kqueue's recv() reports ECONNRESET first and the close error is tagged syscall=read.
…rror so on_close does not re-report it internal_flush latches the errno for callers that discard its return (on_open, flush, end). on_writable handles the return itself via the error callback, so the synchronous on_close that follows its close() must not deliver the same error a second time; the client-side net.ts close handler has no _hadError guard and would reshape the write error as a read.
…sting on_writable quarantine write_check_error's fatal classification is unverified on Windows (a5e7ba5, test-http-no-content-length): on_writable already discards its return there. Latching via the new side channel would let on_end/on_close act on a signal that path deliberately ignores. Gate the two producers so the field stays 0 on Windows; the consumers are then no-ops.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/socket/socket_body.rs (1)
909-928: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear
fatal_write_errnobefore re-entering JS. If theerrorhandler synchronously closes/destroys the socket,on_close()reads the still-set errno at entry and can emit the same write failure again. Movingset(0)ahead ofcall_error_handler()prevents that duplicate report.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/socket/socket_body.rs` around lines 909 - 928, In the fatal send-error path, clear this.fatal_write_errno before invoking handlers.call_error_handler so synchronous socket destruction cannot re-report the same errno through on_close(). Keep the existing error conversion, handler invocation, close behavior, and return flow unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/runtime/socket/socket_body.rs`:
- Around line 909-928: In the fatal send-error path, clear
this.fatal_write_errno before invoking handlers.call_error_handler so
synchronous socket destruction cannot re-report the same errno through
on_close(). Keep the existing error conversion, handler invocation, close
behavior, and return flow unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e7a661f-e765-4c34-9bd3-54399f2fa19b
📒 Files selected for processing (2)
src/runtime/socket/socket_body.rstest/js/bun/net/socket.test.ts
There was a problem hiding this comment.
All five of my earlier findings look addressed — the latest commits (3dcac50 gates both latch producers with #[cfg(not(windows))] symmetric with the on_writable quarantine; c27ef43 moves the on_writable latch-clear before call_error_handler so a handler-driven synchronous close doesn't re-report). No new issues found this pass.
That said, this reworks the native socket close/end/write-error state machine across five dispatch sites with cross-platform errno semantics and node:net-layer interaction, and it's taken several correction rounds to converge. Worth a human look before landing, and macOS CI should be confirmed green (the test's Linux/macOS branch was not exercised locally per the PR body).
What was reviewed: latch lifecycle across all reuse paths (on_close replace(0), detach_for_reconnect, Windows named-pipe reuse is moot since producers are cfg-gated); on_end→close under allow_half_open no longer spins and ref_guard() keeps this alive across the synchronous on_close; on_writable no longer double-reports via on_close on either the handler-destroys or the explicit-close branch.
Extended reasoning...
Overview
Adds a per-transport fatal_write_errno: Cell<i32> on NewSocket so a peer RST observed by a synchronous send() (before the loop can poll it) is (a) clamped to the documented -1 sentinel at the native write()/end() return, (b) suppresses the spurious end dispatch, and (c) surfaces as {code, syscall:'write'} on close. Touches socket_body.rs (field, on_writable/on_end/on_close, write_maybe_corked, internal_flush, both host-fn return sites, detach_for_reconnect), Listener.rs + node_net_binding.rs (all 8 struct-literal init sites), and adds a spawned-server RST test.
Security risks
None. Errno propagation and event-ordering only; no auth/crypto/parsing/permissions.
Level of scrutiny
High. This is the core Bun.connect/Bun.listen and node:net socket lifecycle, with cross-platform kernel-behavior dependencies (Linux sk_err consumption vs XNU SS_CANTSENDMORE-first, the documented Windows fatal-write quarantine from a5e7ba5), an allow_half_open path that node:net always sets, and node:net's SocketHandlers2.close reshaping contract. Getting the latch's set/clear ordering wrong produces spins, double-error emission, or stale state on wrapper reuse — four of my five prior findings were exactly those classes.
Other factors
- Five substantive review-driven fixes already landed on this branch (allowHalfOpen spin, macOS test assertions, reconnect-reuse reset, on_writable double-report, Windows quarantine bypass). All appear correctly applied in the final diff; the last two commits since my previous review (3dcac50, c27ef43) match what was described and I re-traced the on_writable path to confirm the clear now precedes JS re-entry.
- Test is
skipIf(isWindows)(correct — producers are cfg-gated there) and branches Linux vs macOS assertions; macOS was not run locally per the PR body, so CI is the first check. - The residual node:net edge case I noted while re-reading — on_open's post-connect
internal_flushlatching, then on_end→close→SocketHandlers2.closereshaping the write-tagged error to'read ECONNRESET'— is a strict improvement over pre-PR (which swallowed the error entirely) and was explicitly acknowledged by the author as the intended remaining latch consumer, so I'm not flagging it as a bug. - Given the number of iterations needed and the criticality of the code path, deferring to human review rather than approving.
There was a problem hiding this comment.
No new findings this pass — all five prior concerns (reconnect state leak, allowHalfOpen spin, macOS errno shape, node:net client double-report, Windows quarantine bypass) are addressed in the current diff. Deferring to a human because this threads new per-transport state through on_end/on_close/on_writable with platform- and caller-specific ordering, and each prior review round found a real interaction bug; someone familiar with the usockets loop.c dispatch and the node:net close contract should sign off.
Checked this round: the #[cfg(not(windows))] gates on both latch producers keep the field at 0 on Windows so the on_end/on_close consumers are no-ops there; on_close consumes the latch via replace(0) before its early return so a reused wrapper starts clean; on_writable clears the latch before re-entering JS so a handler-driven close cannot re-report it; on_end's close under allow_half_open takes a ref_guard and us_internal_socket_close_raw guards the non-half-open double-close.
Extended reasoning...
Overview
Adds fatal_write_errno: Cell<i32> to NewSocket so a peer RST observed by a synchronous send() (before the loop can poll it) is recorded, clamps native write()/end() returns to the documented -1 sentinel, suppresses the spurious end dispatch when the HUP is not a clean FIN, and surfaces the recorded errno as the close error. Touches socket_body.rs (on_writable, on_end, on_close, write_maybe_corked, internal_flush, detach_for_reconnect, all constructor sites), Listener.rs and node_net_binding.rs (constructor sites only), plus a new subprocess-based test in socket.test.ts.
Security risks
None identified. This is error-reporting semantics on an already-failed connection; no new untrusted input parsing, no auth/crypto surface.
Level of scrutiny
High. The new state interacts with: loop.c's half-open vs. auto-close dispatch, the node:net JS close handler's _hadError/destroyed contract, the reconnect/reuse path (MongoDB driver pattern), and the documented Windows quarantine of fatal-write detection. Five prior review rounds each surfaced a distinct real bug in one of those interactions, all now fixed — but that history is itself the argument for a human pass.
Other factors
The test is skipIf(isWindows) and branches Linux vs. BSD errno shape; it was not run locally on macOS (per the PR body). The on_writable clear-before-close ordering (25a60a5 → c27ef43) and the on_end force-close under allow_half_open are the two spots where a reviewer with loop.c context should confirm the synchronous re-entry is sound.
2 participants
What
Native
Bun.connect/Bun.listensocket.write()was leaking the raw negative errno (-104ECONNRESET, then-32EPIPE) to JS when the peer RSTs while the caller is in a synchronous write burst, instead of the documented-1sentinel. The RST was then reported as a clean peer FIN:endfired andclosereceivedundefined, so a peer abort mid-upload was indistinguishable from a graceful close.Repro
Server in a child process RSTs (
terminate(), SO_LINGER=0) 100 ms after accept while the client is inside a synchronouswrite()loop. Onmain:With this change:
The read-side detection of the same RST (a single write 80 ms after the RST) already produced
write() -> -1+close(err=ECONNRESET), so the error's identity depended on which syscall observed it first.Cause
write_maybe_corkedreturns-fatal_errnoon a rejectedsend()so node:net's buffered-write caller can fail the write like Node'sonWriteComplete(added in #32488). The nativewrite()/end()host functions returned that value verbatim, and nothing recorded it: when the loop next polled it saw only HUP, dispatchedon_end, and closed with the clean-shutdown code.Fix
Record the first fatal send errno on the socket. Native
write()/end()clamp their return to-1.on_endskips its dispatch when a fatal send was already observed (it is not a peer FIN), andon_closereports the recorded errno as the close error (syscall: "write") when the loop itself delivered a clean close code. node:net already guards its close-error path with_hadError, so thewrite_bufferedcaller that consumes the negative errno is unaffected.Verification
New test in
test/js/bun/net/socket.test.tsspawns the RST server in a child process, does a synchronous write burst, and assertswrite()returns-1,enddoes not fire, andclosereceives anECONNRESETerror withsyscall: "write". Fails onmainwith[-104, -32, -32]; passes with this change. node:net reset tests (test-net-*-reset*.js,test-net-error-twice.js) pass.no test proof · iteration 1 · 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