Repository navigation
Conversation
A `send()` that fails with EPIPE/ECONNRESET (the peer closed its fd) is detected by `us_socket_write_check_error`, but the failure never reached JS: `write_maybe_corked` returned -1, the undeliverable bytes were dropped, and the drain callback was dispatched anyway, so `node:net` completed the write callback with success. The socket was never closed and never errored. A half-open socket has no read side left to surface the reset on (uws stops polling readable after the peer's FIN), so nothing else could report it either: writes to a dead peer silently vanished and the socket stayed writable forever, holding its fd. Record the failure on the socket and report it from `writeBuffered`, so node:net fails the write callback with `write EPIPE` and the Writable tears the socket down the way Node's afterWrite does. Also close the refused handle in both `onconnection` drop branches instead of half-closing it, like Node, so the connection is released whatever the peer does with the FIN.
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling 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 (3)
Comment |
|
Updated 5:07 PM PT - Jul 6th, 2026
❌ @robobun, your commit d4ad6aa has some failures in 🧪 To try this PR locally: bunx bun-pr 33506That installs a local version of the PR into your bun-33506 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Checked both; neither is covered by this change, so I am not adding #7251 is #22720 is |
|
Status: the diff is green. Build #69185 shows 281 jobs passing; the one red job ( That is an artifact-download timeout on the agent, not a test failure, and its retry on the same lane already passed. The remaining darwin-14 lanes are buildkite auto-retries still queued. The previous build's only hard failure was Holding here rather than pushing another empty retrigger commit. |
…33803) `us_socket_write_check_error` classified every non-`EWOULDBLOCK` `send()` errno as a fatal write error. For transient kernel resource exhaustion (`ENOBUFS`/`ENOMEM` on POSIX, `WSAENOBUFS` on Windows) the connection is still healthy and a later retry can succeed, so the `node:net` drain path must keep the buffered bytes and re-arm the writable poll instead of dropping them. ## Repro Deterministic via the in-tree usockets fault injection (debug/ASan builds): ```js // net.Socket client: a short write leaves bytes in buffered_data_for_node_net, // the drain-path flush (internal_flush) then hits ENOBUFS. fault.set({ syscall: "send", action: "short", bytes: 1, repeat: 1, fd: clientFd }); client.write(head); // send #0: 1 byte accepted, 199 buffered fault.set({ syscall: "send", action: "errno", errno: "ENOBUFS", repeat: 1, fd: clientFd }); // on_writable -> internal_flush -> send() -> ENOBUFS ``` Without the fix the peer receives `head[0]` followed by whatever is written next; the 199 buffered bytes are gone, no `'error'` event, no failed write callback, no close. The short send that leaves data buffered is ordinary kernel backpressure on any real-network upload; a one-off `ENOBUFS`/`ENOMEM` from `send(2)` under memory pressure is ordinary kernel behaviour. ## Cause `packages/bun-usockets/src/socket.c` `us_socket_write_check_error()`: ```c if (bsd_would_block()) { ...retry... } /* errno == EWOULDBLOCK only */ if (fatal_write_error) *fatal_write_error = 1; /* ENOBUFS/ENOMEM land here */ ``` `src/runtime/socket/socket_body.rs` `internal_flush()` then clears `buffered_data_for_node_net` on `fatal`, and `on_writable` dispatches `'drain'` regardless, so JS keeps writing on a connection that has silently lost a span of its byte stream. ## Fix Add `bsd_send_is_transient_error()` (ENOBUFS/ENOMEM on POSIX, WSAENOBUFS on Windows) and treat it the same as would-block in `us_socket_write_check_error`: set `last_write_failed`, re-arm the writable poll, return 0, do not set `fatal_write_error`. This matches libuv's `uv__write`, which retries on `EAGAIN || EWOULDBLOCK || ENOBUFS`. `EPIPE`/`ECONNRESET` still reach the fatal branch; those are connection-fatal and surfaced on the read side. Making the write side itself fail those pending writes is the complementary fix in #33506. ## Verification New `test.each(["ENOBUFS", "ENOMEM"])` case in `test/js/node/net/net-syscall-fault.test.ts`. Without the fix (`git stash -- packages/ && bun bd test ...`): ``` { intact: false, length: 4097 } // 1 head byte + 4096 body; 199 bytes dropped ``` With the fix: ``` { intact: true, length: 4296 } // 200 head + 4096 body, byte-exact ``` The rest of `net-syscall-fault.test.ts`, `tls-syscall-fault.test.ts`, and `socket-syscall-fault.test.ts` pass unchanged. <!-- robobun:evidence:begin --> --- **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/node/net/net-syscall-fault.test.ts <!-- robobun:evidence:end -->
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-06, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
net.Server#maxConnectionslooked like it wasn't capping: a refused connection kept its socket, its fd, and went on writing into the server forever. The cap is actually fine (the server closes the accepted fd), but the peer never found out it was dropped.Repro
Half a megabyte was handed to
write()and silently discarded, the socket stayed writable forever, and its fd was never released. A dropped peer, ablockListrejection, or any peer that closes while we still have a half-open socket to write to hits this.Cause
us_socket_write_check_erroralready separates a fatalsend()error (EPIPE/ECONNRESET, the peer is gone) from would-block, andwrite_maybe_corked/internal_flushact on it by dropping the undeliverable bytes. But nothing reported the failure:writeBuffered($write) returnedfalse, whichnode:netreads as "would block" and parks the stream callback for a drain.on_writabledispatches the drain callback regardless of the flush result, so the parked callback completed with success, and the next chunk repeated the cycle.write_maybe_corkedassumed "the read side surfaces the reset for anyone who is only waiting", but a half-open socket has no read side left: uws switches the poll to writable-only after the peer's FIN, and thesend()that failed already consumedsk_err, soEPOLLERRnever arrives either.Fix
Record the fatal send error on the socket (
Flags::FATAL_WRITE_ERROR) where it is detected, and report it fromwriteBufferedas-1.node:net's_writeand its two drain handlers turn that intoerrnoException(UV_EPIPE, 'write')on the write callback, which is what makes the Writable destroy the socket -- the same shape Node'safterWriteproduces for a faileduv_write. The drain dispatch itself is unchanged (skipping it on fatal is what broke FIN-terminated responses on Windows before); the drain is simply how the failure now reaches JS.The flag is cleared when the wrapper is reused for a fresh
connect().The other two
$writecallers are deliberately left alone:internal/debugger.tsdiscards the return value, and_http2_upgrade.ts's handle is anInternalSocket::UpgradedDuplex, whosewrite_check_errorarm cannot report fatal (there is no fd tosend()on -- the ciphertext goes back to a JS Duplex), so-1is unreachable there. That invariant is now stated on the flag itself.Separately, both
onconnectiondrop branches nowclose()the refused handle instead ofend()-ing it, matching Node.end()only tears the socket down if the flush path's flags happen to line up;close()releases it whatever the peer does with the FIN.Verification
The repro now prints byte-identical output to node, as does the reporter's longer 20-connection version (
{"drops":20,"clientErrors":20,"stillWritableAfterDrop":0,"serverFdDelta":0}).New test in
test/js/node/net/node-net-server.test.tsasserts a dropped peer's writes fail withwrite EPIPEand the socket is destroyed. It times out onmain(no error is ever emitted) and passes with the fix.Suites run against the debug build
test/js/node/test/parallel/test-net-*.js: 141/141 passtest/js/node/test/parallel/test-tls-*.js,test-http2-*.js: no new failurestest/js/node/net/,test/js/node/tls/,test/js/node/http/,test/js/bun/net/: no new failures (the remaining ones fail identically on a stock bun in this container:localhostresolution and DNS)