Skip to content

usockets: treat ENOBUFS/ENOMEM from send() as would-block, not fatal - #33803

Merged
Jarred-Sumner merged 2 commits into
mainfrom
claude/farm-ea60c661-net-drain-enobufs-drops-bytes
Jul 9, 2026
Merged

Jarred-Sumner merged 2 commits into
mainfrom
claude/farm-ea60c661-net-drain-enobufs-drops-bytes

Conversation

@robobun

@robobun robobun commented Jul 9, 2026 •

Copy link
Copy Markdown
Collaborator

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):

// 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():

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.


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

us_socket_write_check_error classified every non-EWOULDBLOCK send()
errno as a fatal write error. For the transient resource-exhaustion
errnos (ENOBUFS/ENOMEM on POSIX, WSAENOBUFS on Windows) the connection
is still healthy and a later retry can succeed, so the node:net drain
path (internal_flush in socket_body.rs) must keep the buffered bytes and
re-arm the writable poll instead of dropping them.

Dropping them fired 'drain' on a socket that had silently lost a
contiguous span of the application's byte stream: the next write went
straight to the kernel on the same live connection, so the peer received
the stream with bytes missing from the middle, with no 'error' event, no
failed write callback, and no close.
@coderabbitai

coderabbitai Bot commented Jul 9, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 31 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e347f3bf-3ca9-40f0-b350-dff084cf5590

📥 Commits

Reviewing files that changed from the base of the PR and between b05b4fa and cd450a6.

📒 Files selected for processing (4)
  • packages/bun-usockets/src/bsd.c
  • packages/bun-usockets/src/internal/networking/bsd.h
  • packages/bun-usockets/src/socket.c
  • test/js/node/net/net-syscall-fault.test.ts

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the claude label Jul 9, 2026
@robobun

robobun commented Jul 9, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 11:27 PM PT - Jul 8th, 2026

✅ @robobun, your commit cd450a6155289194324ca1aa55501492a7d062fd passed in Build #70819! 🎉


🧪   To try this PR locally:

bunx bun-pr 33803

That installs a local version of the PR into your bun-33803 executable, so you can run:

bun-33803 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found, but this changes error classification in usockets' core TCP write path (which underpins all networking in Bun), so it's worth a human sanity check.

Extended reasoning...

Overview

Adds bsd_send_is_transient_error() (ENOBUFS/ENOMEM on POSIX, WSAENOBUFS on Windows) and treats it as would-block rather than fatal in us_socket_write_check_error(). Includes a fault-injection test in net-syscall-fault.test.ts covering both errnos. Touches packages/bun-usockets/src/{bsd.c,socket.c,internal/networking/bsd.h}.

Security risks

None identified — this widens the retry set for send() errnos, not the accept set for any input. No auth/crypto/parsing surface.

Level of scrutiny

Moderate-to-high. The diff is small and the reasoning is sound (matches libuv's uv__write retry set), and the new helper is deliberately kept separate from bsd_would_block() so recv-side callers are unaffected. But us_socket_write_check_error sits in the write path shared by every TCP connection (node:net, fetch, Bun.serve, WebSocket), and errno classification here decides whether buffered bytes are retried or silently dropped. A subtle mistake would manifest as rare, hard-to-reproduce data loss or hangs across the whole runtime, so a human should sign off.

Other factors

The bug-hunting system found no issues. The test uses the in-tree fault-injection harness and follows the existing patterns in the file. The only caller of us_socket_write_check_error is the node:net flush path via src/uws_sys/us_socket_t.rs, so the blast radius of the behavior change itself is contained, but the new helper lives in shared bsd.c and the reviewer should confirm the recv-side isolation reasoning holds.

…ted sockets too

SocketBody (and its buffered_data_for_node_net / internal_flush path) is
shared between connecting and net.createServer-accepted sockets, so the
fix in the previous commit already covers both. Parametrise the
drain-path transient-errno test over {client writer, server writer} so
the server surface (every raw-TCP response writer built on node:net) is
proven by the same fail-before/pass-after.
@robobun

robobun commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed cd450a6: the drain-path test now runs against both a net.connect client writer and a net.createServer-accepted server writer.

SocketBody (and its buffered_data_for_node_net / internal_flush / on_writable path) is shared between connecting and accepted sockets, so the fix in d8252c8 already covered both. Without it, the server-writer variant fails identically to the client one ({intact: false, length: 4097}, 199 buffered bytes dropped mid-stream with no 'error' and a normal 'drain'), so any raw-TCP response writer built on net.createServer or Bun.listen was affected. With the fix, all four {ENOBUFS, ENOMEM} × {client, server} cases deliver a byte-exact payload.

The same sweep confirmed Bun.serve is not affected (uws's own buffered writer retries correctly), and node:tls server writes did not reproduce the splice under the same fault plan (TLS buffers in the SSL layer, which does not route through write_check_error).

The complementary "report fatal to JS" half (honouring internal_flush's false return for the connection-fatal errnos) is #33506.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found — the fix looks correct and matches libuv's ENOBUFS handling, but this changes error classification in the core usockets write path (an area with prior Windows regressions per the socket_body.rs comments), so worth a human sign-off.

Extended reasoning...

Overview

Adds bsd_send_is_transient_error() (ENOBUFS/ENOMEM on POSIX, WSAENOBUFS on Windows) and ORs it with bsd_would_block() in us_socket_write_check_error() so transient kernel resource exhaustion is treated as would-block rather than fatal. Prevents internal_flush() in socket_body.rs from silently dropping buffered_data_for_node_net on a healthy connection. ~15 lines of production code across bsd.c/bsd.h/socket.c plus a fault-injection test covering {ENOBUFS, ENOMEM} × {client, server} writers.

Security risks

None identified. This narrows the set of errnos classified as fatal (moving two transient ones to the retry path); it does not relax any validation of untrusted input or expose new surface. EPIPE/ECONNRESET still reach the fatal branch.

Level of scrutiny

High. This is the core usockets TCP write error-classification path that every node:net / Bun.connect socket flows through, and misclassification here causes silent mid-stream data loss. The existing comment at src/runtime/socket/socket_body.rs:904-911 documents a prior regression (f0325bd) where changes to fatal-detection semantics broke Windows FIN-terminated responses, so this area has subtle cross-platform interactions that merit a maintainer's eye.

Other factors

The change is conservative and mirrors libuv's uv__write (EAGAIN || EWOULDBLOCK || ENOBUFS). The new helper is deliberately kept separate from bsd_would_block() so recv() callers are unaffected. Tests use the in-tree fault-injection harness and assert byte-exact delivery. The PR description notes tests were deferred to CI (didn't run locally), and the CI build was still in progress at review time. No CODEOWNERS match for packages/bun-usockets/.

@robobun

robobun commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

CI: 284/284 jobs that ran passed on build #70819, including every Linux, Windows, darwin-x64 and darwin-26-aarch64 test lane. The two darwin-14-aarch64 test-bun shards expired waiting for an agent and are sitting in the retry queue; the same lane is scheduled/waiting on every other PR build right now (70849-70857), so this is fleet-wide agent availability rather than anything in this diff. The only annotation is the known-flaky test/bake/dev-and-prod.test.ts on Windows, which passed on retry.

Ready for review.

@Jarred-Sumner
Jarred-Sumner merged commit fc865b3 into main Jul 9, 2026
78 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the claude/farm-ea60c661-net-drain-enobufs-drops-bytes branch July 9, 2026 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants