net: route throws from socket lifecycle listeners to uncaughtException

[robobun]

Fixes #29761.

Repro

const net = require("net");
const server = net.createServer((socket) => {
  socket.on('data', (data) => {
    console.log('got:', data.toString());
    throw new Error('BOOM');
  });
});
server.listen(0, () => {
  const port = server.address().port;
  const client = net.createConnection({ port }, () => client.write('hi'));
  client.on('error', () => {});
});
process.on('uncaughtException', (err) => {
  console.error('[uncaught]', err.message);
  process.exit(1);
});

• Node: prints [uncaught] BOOM and exits 1.
• Bun (before): prints got: hi and exits on the internal timeout. The throw is silently swallowed.

Cause

Bun's Zig socket layer (onData in src/bun.js/api/bun/socket.zig) wraps callback.call(...) in a try/catch and forwards any thrown exception to Handlers.callErrorHandler, which invokes the JS-side SocketHandlers.error (src/js/node/net.ts). For server-side sockets the handler sets self._hadError = true and re-delegates to the shared SocketHandlers.error, whose _hadError guard trips and the error disappears on the floor. Even when the guard doesn't trip, routing a user-code throw to the socket's 'error' event is wrong — Node re-raises it as 'uncaughtException' and leaves the socket alone.

Fix

Wrap self.push(buffer) in SocketHandlers.data, ServerHandlers.data, and SocketHandlers2.data with try/catch and forward the caught exception through reportError, which surfaces it as 'uncaughtException'. Matches Node's semantics: the socket's own 'error' event doesn't fire for user-listener throws, and the socket stays alive.

Verification

New tests in test/js/node/net/node-net.test.ts cover server-side, client-side, and no-handler scenarios:

• Server-side 'data' throw → uncaughtException fires; socket 'error' does not.
• Client-side 'data' throw → same.
• No handler → Bun crashes with non-zero exit (matches Node default).

All three fail on main (two timeout, one sees the error on the socket's 'error' event instead of uncaughtException) and pass on this branch.

$ bun bd test test/js/node/net/node-net.test.ts -t "uncaughtException in socket listener"
(pass) uncaughtException in socket listener > surfaces a throw from a server-side 'data' listener as uncaughtException
(pass) uncaughtException in socket listener > surfaces a throw from a client-side 'data' listener as uncaughtException
(pass) uncaughtException in socket listener > crashes with a non-zero exit when no uncaughtException handler is set

[robobun]

^{Updated 1:04 PM PT - Jun 18th, 2026}

❌ @robobun, your commit 9063d7f has 4 failures in Build #63365 (All Failures):

• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64
• test/js/node/test/parallel/test-net-connect-memleak.js - code 1 on 🐧 3.23 x64-baseline
• test/js/bun/s3/s3.test.ts - code 1 on 🍎 26 aarch64
• test/js/node/test/parallel/test-tls-client-destroy-soon.js - code 1 on 🍎 14 aarch64
• test/integration/next-pages/test/dev-server.test.ts - code 1 on 🍎 26 aarch64

---

🧪   To try this PR locally:

bunx bun-pr 29762

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

bun-29762 --bun

[coderabbitai]

Walkthrough

Adds a pushDataToSocket wrapper to catch/report exceptions thrown by user 'data' listeners and force successful push() returns, updates all socket read handlers to use it, and changes the options.onread.callback exception path to call reportError(e). Adds tests exercising these thrown-error behaviors.

Changes

Cohort / File(s)	Summary
Socket Data Handler src/js/node/net.ts	Adds pushDataToSocket(buffer) that wraps self.push(buffer) to catch listener-thrown exceptions, calls reportError(e), and forces a true return to avoid native error routing. Updates SocketHandlers.data, ServerHandlers.data, and SocketHandlers2.data to use the wrapper and only pause on backpressure. Changes options.onread.callback exception path to call reportError(e) instead of self.emit("error", e).
Exception Handling Tests test/js/node/net/node-net.test.ts	Adds a concurrent test suite launching a Bun process running Node net servers/clients to validate thrown errors from server-side 'data', client-side 'data', and onread.callback. Tests assert UNCAUGHT:<message> appears when uncaughtException is installed, that socket-level 'error' handlers do not log SOCKET_ERROR: for these throws, and that removing the uncaught handler yields a non-zero exit with the thrown error on stderr.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description comprehensively covers all template sections with detailed repro case, root cause analysis, fix explanation, and verification test results, exceeding basic requirements.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The PR title accurately captures the main objective: routing exceptions from socket data listeners to uncaughtException handling, which matches the core fix described in the PR objectives.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

LGTM — thanks for addressing the describe.concurrent nit and the onread.callback case with a regression test.

Extended reasoning...

Overview

This PR changes error routing for user-code throws on the net.Socket data path. It adds a small pushDataToSocket() helper in src/js/node/net.ts that wraps self.push(buffer) in try/catch and forwards any caught exception through the global reportError() (→ process.on('uncaughtException')), then swaps the three existing data handlers (SocketHandlers, ServerHandlers, SocketHandlers2) to use it. It also updates the onread.callback handler to route throws through reportError instead of self.emit('error', ...). Four subprocess-based regression tests are added in test/js/node/net/node-net.test.ts.

Security risks

None. This is purely error-routing in the Node-compat layer — no auth, crypto, parsing, or untrusted-input handling is touched. The only behavioral change is where an already-thrown user exception surfaces (uncaughtException vs. socket 'error' / silently swallowed), which is strictly an improvement over the prior swallowing behavior.

Level of scrutiny

Low–medium. The diff is ~20 source lines plus tests. The mechanism is straightforward: Readable.push() synchronously emits 'data' in flowing mode, so a listener throw propagates up through it; catching there and calling reportError matches Node's semantics (where the throw bubbles through native onStreamRead and lands in uncaughtException). Returning true from the catch correctly avoids pausing the socket on a user-code bug. reportError is already the established pattern for this in Bun's JS layer (used in diagnostics_channel, _http_client, webstreams_adapters).

Other factors

I reviewed an earlier revision and left two comments (use describe.concurrent; apply the same fix to onread.callback). Both were addressed in ad030db, including a new regression test for the onread path that the author verified against Node first. No CODEOWNERS cover these files. The robobun CI failures are Windows agent-provisioning failures plus an unrelated worker_threads flake, not caused by this change. Test coverage is solid: server-side throw, client-side throw, no-handler crash, and onread.callback throw — each asserts both that uncaughtException fires and that the socket's own 'error' event does not.

[robobun]

Build 48204 completed. 252 jobs finished, 0 test failures. The 6 canceled + 28 blocked jobs are all Windows agent provisioning failures (same Failed to create agent infrastructure issue affecting other PRs in the same window); 2 Darwin 14 aarch64 test-bun jobs expired waiting for agents. Every actual test lane that ran passed. The PR is ready for a retry on Windows when agents come back.

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟣 src/js/node/net.ts:433-439 — 🟣 Pre-existing, same bug class: ffe68f0's safelyInvokeListeners sweep wrapped the main paths of ServerHandlers.open and ServerHandlers.handshake but skipped their early-return branches — the two self.emit('drop', data) calls (blockList / maxConnections) and the ECONNRESET branch's self.emit('_tlsError', err) / self.server.emit('tlsClientError', err, self). A user listener that throws on those events still propagates back through the Zig catch into Handlers.callErrorHandler instead of uncaughtException. Low-traffic edges, but worth wrapping for consistency since the same functions now protect their happy paths a few lines below.

  Extended reasoning...

  What the bug is

  Commit ffe68f0 in this PR introduced safelyInvokeListeners() and applied it to the user-facing emits in ServerHandlers.open (the 'connection' emit) and ServerHandlers.handshake (the tlsClientError/secureConnection/secure/secureConnect block). However, both handlers have early-return branches that emit user-facing events before reaching the wrapped section, and those were left as bare emit calls:

  ServerHandlers.open — two early returns each call self.emit('drop', data):

  • the self.blockList.check(...) branch (around src/js/node/net.ts:398)
  • the self.maxConnections branch (around src/js/node/net.ts:413)

  ServerHandlers.handshake — the ECONNRESET early return (src/js/node/net.ts:443-450):

  if (!success && verifyError?.code === "ECONNRESET") {
    const err = new ConnResetException("socket hang up");
    self.emit("_tlsError", err);
    self.server.emit("tlsClientError", err, self);
    self._hadError = true;
    self.destroy();
    return;
  }

  A throw from a user-registered server.on('drop', ...), server.on('tlsClientError', ...), or socket.on('_tlsError', ...) listener on these paths still escapes the JS handler frame, is caught by the Zig catch |err| in onOpen/onHandshake, and is forwarded to Handlers.callErrorHandler → ServerHandlers.error. That misroutes the user's exception to the socket's 'error'/destroy path (and for onOpen, also calls markInactive() and closes the native socket) instead of surfacing it as process.on('uncaughtException').

  Node's behaviour

  In Node, server.emit('drop', ...) is called from onconnection in lib/net.js, and server.emit('tlsClientError', ...) from _tls_wrap.js, both invoked from native via MakeCallback with no surrounding try/catch. A throw from either listener becomes uncaughtException; the socket-level 'error' event never fires for it.

  Why nothing currently prevents it

  safelyInvokeListeners is only applied to the fall-through path of each handler. The early-return branches return before reaching the wrapped block, so their emit calls run bare inside the Zig-invoked frame. The Zig side wraps the entire JS callback in a single callback.call(...) catch |err| { handlers.callErrorHandler(...) }, so any throw that escapes JS is funneled there.

  Step-by-step proof ('drop' case)

  1. const server = net.createServer(); server.maxConnections = 0; server.on('drop', () => { throw new Error('BOOM') }); process.on('uncaughtException', e => console.log('UNCAUGHT', e.message));
  2. A client connects. Zig onOpen calls ServerHandlers.open.
  3. self._connections (0) >= self.maxConnections (0) is true → socket.end(); self.emit('drop', data);.
  4. The 'drop' listener throws. The throw unwinds out of emit and out of ServerHandlers.open.
  5. Zig's catch |err| fires and calls handlers.callErrorHandler(...) → ServerHandlers.error, which sets data._hadError = true and routes the user's Error('BOOM') through the socket-error path. onOpen additionally calls markInactive() + closes the socket on throw.
  6. Node: step 4's throw reaches MakeCallback's boundary and surfaces as uncaughtException; the 'drop' listener's exception never appears on any socket 'error' event.

  The tlsClientError ECONNRESET case is identical: a TLS server whose handshake is interrupted by ECONNRESET emits 'tlsClientError' from the unwrapped branch, and a throwing listener is misrouted to ServerHandlers.error instead of uncaughtException — while the other server.emit('tlsClientError', ...) call eight lines below (the verify-error branch) is wrapped, so the same event behaves differently depending on which branch fired it.

  Impact

  Low. 'drop' only fires when a server has blockList configured or hits maxConnections; the ECONNRESET-during-handshake path is a TLS edge case; and throwing from these listeners is rare. The practical effect is the same Node-compat divergence this PR fixes elsewhere: the throw is reported on the wrong channel (socket/server error path instead of uncaughtException), and the onOpen Zig path additionally tears down state on throw.

  Severity / scope

  Pre-existing. None of these lines were modified by the PR. Flagging only because ffe68f0 explicitly entered both ServerHandlers.open and ServerHandlers.handshake to apply this exact fix, so the unwrapped branches are now inconsistent with the wrapped ones a few lines below in the very same functions. There are further unwrapped emit sites elsewhere in the file (kAttach's connect/ready, emit('lookup'), emit('connectionAttempt'), etc.) — a single follow-up sweeping all of them is probably more useful than per-site patches.

  How to fix

  Wrap each early-return emit group in safelyInvokeListeners, e.g.:

  socket.end();
  safelyInvokeListeners(() => {
    self.emit("drop", data);
  });
  return;

  and for the handshake ECONNRESET branch:

  if (!success && verifyError?.code === "ECONNRESET") {
    const err = new ConnResetException("socket hang up");
    safelyInvokeListeners(() => {
      self.emit("_tlsError", err);
      self.server.emit("tlsClientError", err, self);
    });
    self._hadError = true;
    self.destroy();
    return;
  }

[robobun]

Thanks for the catch — acknowledging and deferring. Two reasons:

1. The ECONNRESET branch emits tlsClientError, same event-family as the TLS handshake wrapping I just reverted in c959ae5. CI proved that surfacing tlsClientError-adjacent user throws exposes pre-existing Bun TLS bugs (authorized=true default, secureConnection firing on failed handshake) that Node tests rely on being silently swallowed. Wrapping the ECONNRESET path would almost certainly hit the same failure mode.

2. Agreed with your own framing — drop, lookup, connectionAttempt*, kAttach etc. are all the same bug class and warrant a single sweep in a follow-up rather than drip-feeding this PR. This PR is already well past its original scope (data listener throw per node:net does not catch errors inside the 'data' callback #29761 → now also onread.callback, connect/ready/connection/timeout).

Will file a follow-up issue to sweep the remaining sites after this lands.

[robobun]

Rebased onto main (was conflicting). Main refactored ServerHandlers.open into a standalone onconnection function and reworked the TLS handshake/BlockList paths, so the 5 original commits were squashed into one and re-applied cleanly on top:

• pushDataToSocket / safelyInvokeListeners helpers + the data, connect/ready, and onread.callback wrappings are unchanged.
• The connection emit wrapping moved from the old inline ServerHandlers.open block to its new home in onconnection.
• TLS handshake paths remain unwrapped (same scoping as before the rebase).

All 8 uncaughtException in socket listener tests pass; the two TLS Node-compat tests (test-tls-sni-server-client, test-tls-junk-closes-server) pass. Gate verified: the 8 tests fail without the src change, pass with it.

[robobun]

CI note for maintainers: the only failure on the latest build (63155, sha f10ecea) is the :linux: aarch64-android - build-rust lane timing out while fetching the Rust nightly toolchain:

could not download file from 'https://static.rust-lang.org/dist/2026-05-06/channel-rust-nightly.toml'
... tcp connect error: Operation timed out (os error 110)

That is a build-infra network timeout during the Android cross-build toolchain install, before any test runs. It is unrelated to this diff (JS-only change to src/js/node/net.ts). A re-run of that lane should clear it. I already spent my one automated ci: retrigger on an earlier unrelated flake this cycle, so I am not pushing another empty commit; flagging here instead.

[claude]

All my prior feedback has been addressed and I found no remaining issues, but this changes error-routing semantics across several node:net lifecycle events (data/connect/ready/connection/onread) with deliberate scoping carve-outs for TLS and timeout — worth a maintainer's sign-off on those boundaries before merge.

Extended reasoning...

Overview

This PR fixes #29761 by intercepting synchronous throws from user socket-event listeners and re-dispatching them via reportError() so they surface as process.on('uncaughtException') (matching Node), instead of letting them unwind into the native socket layer's call_error_handler where they were either misrouted to the socket's 'error' event or silently swallowed. Two small helpers are added to src/js/node/net.ts (pushDataToSocket, safelyInvokeListeners) plus a module-load capture of reportError, applied at six call sites spanning SocketHandlers/ServerHandlers/SocketHandlers2 data handlers, SocketHandlers.open, onconnection, afterConnect, and the onread.callback path. Eight subprocess tests are added.

Security risks

None. This is purely error-propagation plumbing — no auth, crypto, parsing, or trust-boundary changes. The reportError capture at module load is a hardening (tamper-resistance) improvement, not a new exposure.

Level of scrutiny

High. node:net underlies http/https/tls and essentially every networked Bun program. The change alters when/whether 'error' fires vs. 'uncaughtException', whether 'ready' fires after a throwing 'connect' listener, whether a server socket auto-resumes after a throwing 'connection' listener, and whether the connection survives a throwing 'connect' listener (it now does; previously the native onOpen path tore it down). These are all moves toward Node parity and are well-tested, but they are observable behavior changes in a foundational module.

Other factors

• I reviewed this across ~8 iterations (Apr–Jun); every nit and suggestion was addressed and all threads are resolved. The current bug-hunt pass found nothing.
• Scope grew well past the original issue ('data' only) at my prompting, then was deliberately frozen: TLS handshake emits stay unwrapped (wrapping them surfaced unrelated pre-existing TLS bugs in CI), and the native timeout handlers were intentionally left unwrapped (dead path; user-facing 'timeout' already routes correctly via the JS timer). The author committed to a follow-up sweep for the remaining sites (drop/lookup/connectionAttempt*/kAttach/timeout). A maintainer should confirm they're comfortable with that scoping boundary.
• onconnection now wraps _socket.resume() inside the same try/catch as emit('connection'), so a throwing 'connection' listener leaves the accepted socket paused. This matches Node (the throw unwinds before resume there too), but it's a real behavior change for Bun.
• Tests follow repo conventions (describe.concurrent, three-element Promise.all pipe drain, event-driven rather than time-based). CI is green except an unrelated linux aarch64-android build-rust toolchain-fetch network timeout the author flagged.

Net: I believe the change is correct and well-tested, but it's a semantic change in core networking error handling with explicit scope carve-outs — a human maintainer should give the final nod rather than a bot.

[robobun]

Rebased onto main (6764763). src/js/node/net.ts auto-merged cleanly; the only conflict was a trivial end-of-file adjacency in test/js/node/net/node-net.test.ts where main added a describe("Socket fd adoption") block and this PR added describe.concurrent("uncaughtException in socket listener") at the same spot. Kept both. Verified locally: all 8 uncaughtException tests pass and main's 3 fd-adoption tests pass.

[robobun]

Heads-up: I narrowed this PR after CI surfaced a real regression, and it now needs a maintainer decision on the remaining piece.

What happened

The previous revision wrapped self.push(buffer) in node:net's data handlers (pushDataToSocket) so a throw from a user 'data' listener became uncaughtException. CI (build 63357) showed that breaks node:http:

• test-http-abort-client, test-http-client-aborted-event, test-http-server-capture-rejections: Parse Error: Invalid character in chunk size surfaced as uncaughtException instead of the client res 'error' (ECONNRESET).
• test-http-catch-uncaughtexception: hang/timeout.

All four pass on main and fail with the data-path wrapping.

Root cause

node:http's client socketOnData is itself a socket.on('data') listener, and Bun's native HTTP parser throws parse errors where Node returns them (Node's socketOnData handles a returned Error inline and never throws). Bun's http has been relying on the native socket layer catching that thrown parse error and routing it to the socket 'error' event. Wrapping self.push at the JS layer intercepts the throw first (breaking that routing), and catching mid-Readable.push() also corrupts stream state so the socket never closes (the hang).

There is no way to distinguish a user 'data' listener throw from an internal consumer (http parser) throw at the net push layer. A correct 'data'-path fix needs a coordinated change:

1. native socket onData routing a genuine 'data'-callback throw to uncaughtException (after the Readable fully unwinds, matching Node's C++ boundary) rather than to call_error_handler, and
2. node:http's parser returning parse errors inline (or socketOnData handling the thrown parse error) so http stops depending on the old routing.

That spans native socket code + the http parser error model and also affects the Bun.connect API surface, so it's an architectural call.

What this PR now contains

I reverted the data-path wrapping and kept only the safe, same-bug-class lifecycle fixes that do not touch the http data path:

• 'connect' / 'ready' (client open + afterConnect), 'connection' (server accept), and onread.callback throws now surface as uncaughtException (matching Node), via safelyInvokeListeners + module-captured reportError.
• Short-circuit semantics preserved (a throw from 'connect' aborts the emit group so 'ready' doesn't fire).

Verified: the 4 http tests above pass again, a 40-test node:http parallel sweep is clean (only pre-existing agent-keepalive flake, which also fails on main), and the 5 kept tests fail with net.ts reverted to main and pass with the fix.

The original 'data'-listener case from the issue is intentionally left for the coordinated native+http follow-up described above.

[robobun]

CI note (build 63365): the red lanes are all unrelated flake/infra, not this diff:

• next-pages/dev-server — puppeteer chrome-headless-shell download failure (infra).
• s3.test.ts — R2 upload timeout (infra).
• test/package.json — install timeout, auto-retried (flaky annotation).
• test-net-connect-memleak (alpine x64) and test-tls-client-destroy-soon (darwin aarch64) — GC-timing / timing-sensitive; both pass 3/3 locally against this branch, so not a deterministic regression from the diff.

The change here is JS-only (connect/ready/connection/onread emit wrapping in node:net) and its own tests pass. I have already used this cycle's single automated retrigger, so I am not pushing another empty commit. Flagging here so the red CI is not mistaken for a diff problem.