Skip to content

net: fail the write when the kernel rejects the bytes outright - #33506

Closed
robobun wants to merge 4 commits into
mainfrom
farm/6f663ab3/net-fatal-write-error
Closed

robobun wants to merge 4 commits into
mainfrom
farm/6f663ab3/net-fatal-write-error

Conversation

@robobun

@robobun robobun commented Jul 6, 2026 •

Copy link
Copy Markdown
Collaborator

net.Server#maxConnections looked 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

import net from "node:net";
const srv = net.createServer(c => c.write("hi")); // holds the one allowed socket open
srv.maxConnections = 1;
srv.listen(0, "127.0.0.1", async () => {
  const port = srv.address().port;
  const c1 = net.connect(port, "127.0.0.1");
  c1.on("error", () => {});
  await new Promise(r => c1.once("data", r)); // the quota is full

  const dropped = net.connect({ port, host: "127.0.0.1", allowHalfOpen: true });
  let errors = 0;
  dropped.on("error", () => errors++);
  await new Promise(r => dropped.once("end", r)); // the server closed its side

  const buf = Buffer.alloc(65536, 0x41);
  for (let k = 0; k < 8; k++) dropped.write(buf);
  await new Promise(r => setTimeout(r, 900));
  console.log({ errors, writable: dropped.writable, bytesWritten: dropped.bytesWritten });
  process.exit(0);
});
node v26.3.0: { errors: 1, writable: false, bytesWritten: 131072 }
bun 1.4.0:    { errors: 0, writable: true,  bytesWritten: 65536  }

Half a megabyte was handed to write() and silently discarded, the socket stayed writable forever, and its fd was never released. A dropped peer, a blockList rejection, or any peer that closes while we still have a half-open socket to write to hits this.

Cause

us_socket_write_check_error already separates a fatal send() error (EPIPE/ECONNRESET, the peer is gone) from would-block, and write_maybe_corked / internal_flush act on it by dropping the undeliverable bytes. But nothing reported the failure:

  • writeBuffered ($write) returned false, which node:net reads as "would block" and parks the stream callback for a drain.
  • on_writable dispatches the drain callback regardless of the flush result, so the parked callback completed with success, and the next chunk repeated the cycle.
  • Nothing closed the socket. The comment in write_maybe_corked assumed "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 the send() that failed already consumed sk_err, so EPOLLERR never arrives either.

Fix

Record the fatal send error on the socket (Flags::FATAL_WRITE_ERROR) where it is detected, and report it from writeBuffered as -1. node:net's _write and its two drain handlers turn that into errnoException(UV_EPIPE, 'write') on the write callback, which is what makes the Writable destroy the socket -- the same shape Node's afterWrite produces for a failed uv_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 $write callers are deliberately left alone: internal/debugger.ts discards the return value, and _http2_upgrade.ts's handle is an InternalSocket::UpgradedDuplex, whose write_check_error arm cannot report fatal (there is no fd to send() on -- the ciphertext goes back to a JS Duplex), so -1 is unreachable there. That invariant is now stated on the flag itself.

Separately, both onconnection drop branches now close() the refused handle instead of end()-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.ts asserts a dropped peer's writes fail with write EPIPE and the socket is destroyed. It times out on main (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 pass
  • test/js/node/test/parallel/test-tls-*.js, test-http2-*.js: no new failures
  • test/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: localhost resolution and DNS)

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.
@coderabbitai

coderabbitai Bot commented Jul 6, 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: 15 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: 1c26d754-8a28-45b7-86a1-3bdb492333a1

📥 Commits

Reviewing files that changed from the base of the PR and between 48ff9eb and d4ad6aa.

📒 Files selected for processing (3)
  • src/js/node/net.ts
  • src/runtime/socket/socket_body.rs
  • test/js/node/net/node-net-server.test.ts

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

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

robobun commented Jul 6, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 5:07 PM PT - Jul 6th, 2026

❌ @robobun, your commit d4ad6aa has some failures in Build #69185 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 33506

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

bun-33506 --bun

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Found 2 issues this PR may fix:

  1. process EPIPE error event not emitted #7251 - EPIPE error events are not emitted when the read end closes; this PR adds proper UV_EPIPE propagation when the kernel rejects a write
  2. Unable to make socket connections using mssql package #22720 - mssql/tedious reports "Connection lost - socket hang up" consistent with the server dropping the connection and the client socket never receiving error notification

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #7251
Fixes #22720

🤖 Generated with Claude Code

@robobun

robobun commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

Checked both; neither is covered by this change, so I am not adding Fixes lines.

#7251 is process.stdout piped to head. That write goes through the stdio sink, not a us_socket_t, so it never reaches us_socket_write_check_error. Its repro still prints nothing on this branch:

$ bun-debug run bug.js | head -3   # with this PR applied
bun
bun
                                   # no "EPIPE emitted"

#22720 is mssql/tedious with encrypt: true, failing at connect. TLS writes short-circuit past the fatal-error detection (us_socket_write_check_error hands s->ssl sockets straight to us_socket_write), so nothing here touches that path, and "socket hang up" during the handshake is a different failure than a write to an already-dead peer being swallowed.

Comment thread src/runtime/socket/socket_body.rs Resolved
@robobun

robobun commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

Status: the diff is green. Build #69185 shows 281 jobs passing; the one red job (darwin 26 aarch64 - test-bun) failed before running any tests:

Error: buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun'.
Refusing to continue with a partial download (would silently fall back to the wrong binary).

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 serve-body-leak.test.ts timing out on x64-asan, a memory-stress test with a documented ASAN-flake history (a since-reverted "skip on ASAN" PR, and a sibling case already marked todoIf(isFlaky && isLinux)). It did not recur on the re-run. None of these lanes touch the changed code: this PR is node:net write-path plumbing (Rust + JS, platform-independent), and locally all 141 node parallel net tests plus the node tls/http/http2 and test/js/bun/net suites pass with no regressions.

Holding here rather than pushing another empty retrigger commit.

Jarred-Sumner pushed a commit that referenced this pull request Jul 9, 2026
…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 -->
@robobun

robobun commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator Author

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.

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.

1 participant