node:net: let unhandled socket 'error' throw (stop swallowing ECONNRESET) by robobun · Pull Request #34513 · oven-sh/bun

robobun · 2026-07-17T23:05:36Z

A net.Socket whose peer resets the connection was silently destroyed when no 'error' listener was attached. The process kept running with nothing on 'error', nothing on uncaughtException, and exit code 0.

import net from "node:net";
const srv = net.createServer((c) => setTimeout(() => c.resetAndDestroy(), 60));
srv.listen(0, "127.0.0.1", () => {
  net.connect(srv.address().port, "127.0.0.1"); // NO listeners at all
});
process.on("uncaughtException", (e) => { console.log("UNCAUGHT", e.code); process.exit(7); });
setTimeout(() => { console.log("ALIVE-SWALLOWED"); process.exit(0); }, 1200);

	node v26.3.0	bun main	this PR
output	`UNCAUGHT ECONNRESET`	`ALIVE-SWALLOWED`	`UNCAUGHT ECONNRESET`
exit	7	0	7

The same swallow happened on server-accepted sockets. With an 'error' listener attached both runtimes already delivered ECONNRESET identically; only the unhandled path diverged.

Cause

Three close paths in src/js/node/net.ts gated the destroy(err) on self.listenerCount("error") > 0, falling through to a plain destroy() (or a clean push(null) EOF) when no listener was present:

failWrite (fatal write errno with no write callback)
SocketEmitEndNT (fd-adoption / ServerHandlers.close)
SocketHandlers2.close (client sockets)

Each also attached a no-op once("error", () => {}) on the with-listener branch so a listener detached between close and the deferred 'error' emission could not surface an uncaught exception. Both mechanisms exist specifically to prevent the node-documented behavior (an unhandled 'error' throws).

Node's onStreamRead does stream.destroy(errnoException(nread, 'read')) for any read errno with no listener check; the EventEmitter default-error path does the throwing.

Fix

Drop the three listenerCount("error") gates and the once("error", noop) guards. The existing teardownNoise filter (reset arriving after both sides finished cleanly, which node never observes because the socket is already gone on 'end') already keeps post-exchange loopback RSTs from surfacing; that filter plus _hadError is now applied in SocketHandlers2.close as well so it matches SocketEmitEndNT.

Removing the gates exposed two places Bun generated a reset where Node would not, which now need handling to match Node:

tls.Server accepted sockets. Node's tlsConnectionListener attaches onSocketTLSError to every accepted TLSSocket: before 'secureConnection' it routes the error to 'tlsClientError', and after it the listener is inert but its presence keeps the default EventEmitter throw from firing (user code only sees the socket at 'secureConnection'). Bun had no equivalent, so an accepted TLSSocket that reset before the user's handler ran became an uncaught exception. onconnection now installs the same listener for TLS sockets. Observable as socket.listenerCount("error") === 1 in a 'secureConnection' handler, matching Node.

Happy-eyeballs losing connection. us_internal_socket_after_open() closed each losing parallel connect attempt with SO_LINGER {1,0}, so a server that had already accepted it saw a spurious ECONNRESET on a connection that never sent anything. Every test that ran a net.createServer on all interfaces and fetched via localhost hit this. The losing leg now closes with a normal FIN.

Verification

Four new tests, all failing on main and passing with the fix:

test/js/node/net/node-net.test.ts (3 subprocess tests): client-socket peer RST, server-accepted-socket peer RST, and the no-handler crash case all reach uncaughtException with {code: "ECONNRESET", syscall: "read"}. All three fixtures produce identical output under Node v26.3.0.
test/js/node/tls/node-tls-server.test.ts: a 'secureConnection' handler observes listenerCount("error") === 1 on the accepted TLSSocket.

Regression sweep on Linux (failures identical to main, none new):

suite	with fix	main
`test-net-*.js`	139/141	139/141
`test-tls-*.js`	151/152	151/152
`test-http-.js` + `test-http2-.js`	627/630	627/630
`test-https-*.js`	59/59	59/59

Four test fixtures (fetch-redirect.test.ts, proxy.test.ts, node-fin-fixture.js, node-net.test.ts) gain socket.on("error", () => {}) on accepted sockets that could reset before their late-attached listener ran; Node would crash on those too.

Known Windows-only follow-up

Two upstream Node tests still fail on Windows: test-https-agent-unref-socket.js and test-https-server-close-all.js. Both expose a pre-existing native-layer difference: Bun's process-exit and server.stop(true) close sockets with RST on Windows where libuv sends FIN. The peer's net.Socket now correctly surfaces that RST instead of swallowing it. A separate fix is tracked for the RST source itself; these two are not net.ts bugs.

no test proof · iteration 7 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/net/node-net.test.ts test/js/web/fetch/fetch.test.ts

…SET) A net.Socket whose peer resets the connection was silently destroyed when no 'error' listener was attached. Node delivers the read error to the stream via destroy(err); with no 'error' listener that throws through the default EventEmitter path and reaches uncaughtException. Bun gated the destroy(err) on listenerCount('error') > 0 and fell through to a plain destroy() otherwise, so the failure never surfaced anywhere: no 'error', no uncaughtException, no non-zero exit. Drop the three listenerCount('error') gates (failWrite, SocketEmitEndNT, SocketHandlers2.close) and the once('error', noop) late-detach guards that went with them. The existing teardownNoise filter (a reset that lands after both sides finished cleanly) already keeps post-exchange RSTs from surfacing; that filter is now applied in SocketHandlers2.close as well so it matches SocketEmitEndNT.

robobun · 2026-07-17T23:05:46Z

^{Updated 1:30 AM PT - Jul 18th, 2026}

❌ @robobun, your commit 9e07c6e has 6 failures in Build #75174 (All Failures):

test/js/node/http/node-http-uaf.test.ts - code 1 on 🍎 14 x64
test/js/node/test/sequential/test-gc-http-client-onerror.js - timeout on 🐧 3.23 x64
test/js/node/test/sequential/test-gc-http-client-onerror.js - timeout on 🐧 3.23 x64-baseline
test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js - SIGABRT on 🐧 13 x64-asan
test/js/node/test/parallel/test-https-server-close-all.js - code 1 on 🪟 2019 x64
test/js/node/test/parallel/test-https-server-close-all.js - code 1 on 🪟 2019 x64-baseline
test/js/node/test/parallel/test-https-server-close-all.js - code 1 on 🪟 11 aarch64
test/js/node/test/parallel/test-https-agent-unref-socket.js - code 1 on 🪟 2019 x64-baseline
test/js/node/test/parallel/test-https-agent-unref-socket.js - code 1 on 🪟 2019 x64
test/js/node/test/parallel/test-https-agent-unref-socket.js - code 1 on 🐧 13 x64-asan
test/js/node/test/parallel/test-https-agent-unref-socket.js - code 1 on 🪟 11 aarch64
test/js/web/timers/timer-heap-race.test.ts - pid 6364 sigabrt on 🐧 13 x64-asan

🧪 To try this PR locally:

bunx bun-pr 34513

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

bun-34513 --bun

robobun · 2026-07-17T23:05:46Z

The diff is ready for review. CI is green on all Linux/macOS lanes; the remaining red is:

test/js/node/test/parallel/test-https-agent-unref-socket.js and test-https-server-close-all.js on Windows only. Both are Node upstream tests (cannot be modified) that expose a pre-existing native-layer difference: Bun's process-exit and server.stop(true) close sockets with RST on Windows where libuv sends FIN. The peer's net.Socket now correctly surfaces that RST instead of swallowing it. A separate fix is tracked for the RST source; these are not net.ts bugs.
Occasional unrelated ASAN LeakSanitizer flakes in timer/atomics code (timer-heap-race.test.ts, 30205.test.ts), not reproducible locally on either this branch or main.

The net/tls/http/https/http2 Node parallel suites show no new failures vs main (~980 tests). All review threads are resolved.

coderabbitai · 2026-07-17T23:08:59Z

Walkthrough

Socket write, close, TLS handshake, and accepted-connection paths now use internal error state and explicit cleanup. Tests cover TCP reset propagation, TLS listener setup, fixture error handling, and clean shutdown of losing connection candidates.

Changes

Socket error and TLS handling

Layer / File(s)	Summary
Socket error and teardown handling `src/js/node/net.ts`	Write and close paths now surface or suppress errors using internal teardown state rather than listener counts.
TLS handshake error routing `src/js/node/net.ts`, `src/js/node/http2.ts`, `test/js/node/tls/node-tls-server.test.ts`	Pre-handshake TLS errors are routed through `tlsClientError` once, with updated handshake comments and internal-listener coverage.
Accepted connection cleanup `src/js/node/net.ts`, `packages/bun-usockets/src/context.c`	Dropped accepted sockets are destroyed explicitly, and losing DNS candidates close with clean shutdown.
Socket error regression coverage `test/js/node/net/node-net.test.ts`, `test/js/node/net/node-fin-fixture.js`	Subprocess tests cover reset errors, uncaught exceptions, default process failure, and max-connection drops.
Fixture updates `test/js/bun/http/proxy.test.ts`, `test/js/web/fetch/*`, `test/js/node/tls/renegotiation.test.ts`, `test/js/third_party/body-parser/express-memory-leak-fixture.mjs`	Network fixtures add no-op error handlers, the renegotiation fixture handles raw socket errors, and the body-parser fixture binds to `127.0.0.1`.

Possibly related PRs

oven-sh/bun#33256: Updates related node:net socket close and teardown error handling.
oven-sh/bun#33963: Modifies related TLS handshake and tlsClientError emission paths.
oven-sh/bun#34285: Updates related SocketEmitEndNT EOF and error synthesis logic.

Suggested reviewers: cirospaciari, jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
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 title clearly summarizes the main change: unhandled socket errors now throw instead of being swallowed.
Description check	✅ Passed	The description covers the change, cause, fix, and verification, though it uses different headings than the template.

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

coderabbitai

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/js/node/net.ts`:
- Around line 191-195: Add a subprocess test covering the no-callback
write-failure path around the stream write handling in net.ts. Force a
write-side failure before the peer receives data, omit both the write callback
and an 'error' listener, and assert the resulting uncaught error and process
behavior. Ensure the fixture does not reset before failWrite is exercised.

🪄 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: 05874f39-bce8-4faf-b319-92d329c54fd6

📥 Commits

Reviewing files that changed from the base of the PR and between 91460bc and 5675683.

📒 Files selected for processing (2)

src/js/node/net.ts
test/js/node/net/node-net.test.ts

…e happy-eyeballs loser CI fallout from the net.ts listenerCount removal broke into two classes: 1. tls.Server accepted sockets: Node attaches onSocketTLSError to every accepted TLSSocket in tlsConnectionListener, routing a pre-handshake error to 'tlsClientError' and (by its presence) keeping a post- handshake unhandled 'error' from throwing. Bun had no equivalent, so an accepted TLSSocket that reset before the user's 'secureConnection' listener ran surfaced as uncaughtException. Install the same listener in onconnection for TLS sockets. 2. Happy-eyeballs losing attempt: us_internal_socket_after_open() closed each losing connect attempt with LIBUS_SOCKET_CLOSE_CODE_CONNECTION_ RESET (SO_LINGER {1,0}), so a server that had already accepted it saw a spurious ECONNRESET on a connection that never sent anything. Every test that ran a net.createServer on all interfaces and fetched via 'localhost' hit this. Close the losing leg cleanly instead. Also: - SocketHandlers2.close: when the teardownNoise/_hadError guard filters an error, destroy() quietly so 'close' still fires (matches SocketEmitEndNT's fall-through). - Add 'error' listeners to accepted sockets in four test fixtures that fetch or write into a net.createServer without one.

…ntErrorEmitted on handshake timeout

claude

No new issues found — the three earlier inline comments (fall-through stale-handle guard, ktlsClientErrorEmitted on handshake timeout, and the SocketHandlers2.close filtered-error destroy) are all addressed in f5c6b1b. Deferring to a human because this changes default net.Socket error semantics (previously-silent ECONNRESET now throws), touches native usockets close behavior, and carries a known Windows follow-up.

What was reviewed:

The three listenerCount("error") gate removals and their fall-throughs now mirror SocketEmitEndNT; _hadError is set before destroy(er) at each site.
onServerSocketTLSError double-emit guard: all four tlsClientError emit sites checked — the two post-_secureEstablished sites (net.ts:845, 896) are inert via that flag, and the two pre-handshake sites now set ktlsClientErrorEmitted.
context.c FIN-instead-of-RST for happy-eyeballs losers: losers are still POLL_TYPE_SEMI_SOCKET at close so on_close never dispatches for them; only the wire-level close code changes.
Test-fixture .on("error", () => {}) additions: each is on a server-accepted socket that Node would also crash on under the same race, not masking a Bun-specific regression.

Extended reasoning...

Overview

Removes three listenerCount("error") > 0 gates in src/js/node/net.ts (failWrite, SocketEmitEndNT, SocketHandlers2.close) so an unhandled socket 'error' throws via the EventEmitter default path, matching Node's onStreamRead. To keep that from surfacing spurious errors it also (a) attaches a Node-equivalent onServerSocketTLSError listener to every accepted TLSSocket with a ktlsClientErrorEmitted once-guard, and (b) changes the happy-eyeballs losing-leg close in packages/bun-usockets/src/context.c from LIBUS_SOCKET_CLOSE_CODE_CONNECTION_RESET to CLEAN_SHUTDOWN so a peer that already accepted it sees FIN, not RST. Three new subprocess tests plus a TLS listener-count test; seven existing test fixtures gain .on("error", () => {}) on accepted sockets.

Security risks

None identified. No auth/crypto/permissions surface. The usockets change only affects how a losing parallel-connect fd is closed; the TLS attach for the winner is unchanged.

Level of scrutiny

High. This is a Node-compat default-behavior change: sockets that previously closed silently on peer RST will now surface an uncaught exception, which is correct per Node but has broad blast radius (the PR itself had to patch four unrelated test files, and the description notes two Windows upstream tests still fail as a known follow-up). It also edits native usockets. That combination — compat-layer semantics change + native edit + known platform follow-up — is exactly the kind of change a maintainer should sign off on rather than a bot.

Other factors

All three of my earlier inline findings were addressed in 8108029 and f5c6b1b, and I re-verified them against the current diff (the fall-through at net.ts:1212 now carries socket === self._handle, and net.ts:1053 sets ktlsClientErrorEmitted). The regression-sweep table in the description shows parity with main across the net/tls/http/https suites on Linux, but the Windows-only follow-up is explicitly deferred. The bug-hunting pass on this revision found nothing new.

…balls The aborted-requests stress test pushed 50000 fetches to 'localhost', which races IPv4 and IPv6 and leaves the losing leg (now closed with a FIN instead of a RST) in TIME_WAIT. On Windows the accumulated TIME_WAIT sockets pushed the test past its 40s budget. Binding the fixture to 127.0.0.1 sends fetch() to one address and removes the extra leg.

…hat the client resets The invalid-Content-Length and duplicate-header tests deliberately hand fetch() a response it rejects; the client resets the connection it can no longer trust, and the server's accepted socket now sees that reset instead of a swallowed close.

…update stale comment onconnection() kAttach'es the wrapper before checking blockList / maxConnections, so a connection dropped by either leaves a live Socket with no 'error' listener. With the listenerCount gate removed, a RST from the dropped peer would surface as uncaughtException. destroy() the wrapper before returning so SocketEmitEndNT short-circuits on destroyed. Also update the now-stale comment at the requestCert/rejectUnauthorized destroy(): onServerSocketTLSError is attached before handshake runs so the socket does have an 'error' listener there.

claude

Both findings from the last pass are addressed in dc4420f — _socket.destroy() now runs in the blockList/maxConnections drop branches (with a covering test), and the stale comment at the rejectUnauthorized destroy site is updated. No new issues this round.

Deferring to a human for sign-off given the scope: this changes unhandled-'error' semantics across every net.Socket consumer (net/tls/http/http2), adds the onServerSocketTLSError routing, and flips the happy-eyeballs loser close code in usockets, with two known Windows-only Node test regressions called out as follow-up.

Checked this pass: the new _socket.destroy() in both drop branches makes SocketEmitEndNT short-circuit on self.destroyed so a late RST is inert; the maxConnections subprocess test exercises exactly that. The happy-eyeballs CLEAN_SHUTDOWN swap was examined for TIME_WAIT accumulation on the loser and ruled out as a concern.

Extended reasoning...

Overview

The PR removes three listenerCount('error') > 0 gates in src/js/node/net.ts (failWrite, SocketEmitEndNT, SocketHandlers2.close) so an unhandled socket 'error' throws via the EventEmitter default path, matching Node. To keep that from surfacing Bun-generated resets that Node never produces, it also (a) attaches an onServerSocketTLSError listener to every accepted TLSSocket in onconnection, guarded by a new ktlsClientErrorEmitted symbol, (b) switches the happy-eyeballs loser close in packages/bun-usockets/src/context.c from RST to FIN, and (c) — after my previous finding — destroys the dropped socket wrapper in the blockList/maxConnections early-returns. Seven test files gain socket.on('error', () => {}) on accepted sockets that could reset, and four new subprocess tests plus one TLS listener-count test are added.

Security risks

The maxConnections/blockList DoS I flagged last round is fixed: _socket.destroy() before the early return means SocketEmitEndNT short-circuits on self.destroyed when the peer's RST arrives, and the new subprocess test in node-net.test.ts covers it. onServerSocketTLSError is inert post-_secureEstablished and guarded by ktlsClientErrorEmitted at all three pre-handshake tlsClientError emit sites (handshake failure, handshake timeout, and the listener itself), so no double-emission or swallowed error paths remain that I can see. No injection/auth surface is touched.

Level of scrutiny

High. src/js/node/net.ts is load-bearing for every node:net/node:tls/node:http/node:http2 consumer, and this is a deliberate behavior change (previously-swallowed errors now throw). The PR has already been through three review rounds with real issues found each time (fall-through mismatch, handshake-timeout guard, stale-handle guard, blockList/maxConnections crash, stale comment) — all addressed — which is exactly why a human should give it a final look before merge. The usockets change is one line but alters wire behavior for every dual-stack connect.

Other factors

The PR description notes two Windows-only Node test regressions (test-https-agent-unref-socket.js, test-https-server-close-all.js) as a known follow-up rooted in a separate native-layer RST-vs-FIN difference. The regression sweep table shows parity with main on Linux across ~976 net/tls/http/https tests. All prior inline threads are resolved and the corresponding fixes are visible in the diff. Given the breadth of downstream consumers and the acknowledged platform-specific follow-up, this is over my bar for auto-approval.

…ST test On macOS the connect callback could fire before the server's accept had fully processed, so the RST arrived before the accepted socket was established and was delivered as a clean close. Having the server write a byte and the client reset only after receiving it makes the RST land on an established server socket on every platform.

coderabbitai

Actionable comments posted: 3

🤖 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 `@test/js/bun/http/proxy.test.ts`:
- Line 390: Remove the duplicate no-op "error" listener registration on
clientSocket, keeping the earlier listener before the first request and deleting
only the later registration in the surrounding test flow.

In `@test/js/node/net/node-net.test.ts`:
- Around line 1262-1264: Update the subprocess output assertion in the test to
compare stdout directly instead of calling trim(), preserving the exact expected
"DROPPED\nDONE" value so trailing whitespace or extra blank lines cause the test
to fail.

In `@test/js/node/tls/node-tls-server.test.ts`:
- Around line 1326-1329: Update the test cleanup in the finally block to await
the server’s asynchronous close completion rather than calling server.close()
and returning immediately. Preserve client?.destroy(), and resolve cleanup only
after the server close event confirms the TLS server and its handles are fully
released.

🪄 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: e2e879c1-95c4-4827-96da-1c3320e04f5e

📥 Commits

Reviewing files that changed from the base of the PR and between 5675683 and 6d03b77.

📒 Files selected for processing (9)

packages/bun-usockets/src/context.c
src/js/node/net.ts
test/js/bun/http/proxy.test.ts
test/js/node/net/node-fin-fixture.js
test/js/node/net/node-net.test.ts
test/js/node/tls/node-tls-server.test.ts
test/js/third_party/body-parser/express-memory-leak-fixture.mjs
test/js/web/fetch/fetch-redirect.test.ts
test/js/web/fetch/fetch.test.ts

…duplex test When the attacker process is torn down the peer close can arrive as a RST on Windows; the raw net.Socket had no 'error' listener.

coderabbitai

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 `@test/js/node/tls/renegotiation.test.ts`:
- Line 265: Update the raw socket error handler in the renegotiation test to
ignore only the expected peer-teardown reset, such as ECONNRESET, and route all
other errors to the test’s rejection or failure path. Add or adjust assertions
so unexpected TLS, protocol, or transport errors fail the test rather than being
swallowed.

🪄 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: afeb872d-9aa1-406b-97bb-eb91e78abe5a

📥 Commits

Reviewing files that changed from the base of the PR and between 6d03b77 and c7fb4fd.

📒 Files selected for processing (1)

test/js/node/tls/renegotiation.test.ts

…etTLSError test

…nSocketTLSError test

coderabbitai

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 `@test/js/node/tls/node-tls-server.test.ts`:
- Around line 1322-1325: Update the client connection flow around secureConn so
client errors reject the awaited operation instead of being swallowed.
Coordinate the client error handler with the secureConnection promise, remove
the error listener once secureConnection succeeds, and preserve propagation of
the original connection error to the test.

🪄 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: 1cab0eeb-5a77-4837-9c22-4280ac389d6e

📥 Commits

Reviewing files that changed from the base of the PR and between c7fb4fd and 9e07c6e.

📒 Files selected for processing (2)

src/js/node/http2.ts
test/js/node/tls/node-tls-server.test.ts

claude

No new findings after 9e07c6e — all earlier inline comments have been addressed. Deferring to a human for the final sign-off given the blast radius: this flips core net.Socket error semantics (unhandled ECONNRESET now crashes the process, matching Node), changes the native happy-eyeballs close code in context.c, and carries two acknowledged Windows-only Node-test regressions.

What was reviewed:

The three listenerCount("error") gate removals and their fall-through paths in net.ts — SocketHandlers2.close now mirrors SocketEmitEndNT's teardownNoise/_hadError guards and quiet-destroy fall-through, including the socket === self._handle check.
onServerSocketTLSError + ktlsClientErrorEmitted: all three tlsClientError emit sites set the guard before emitting; the blockList/maxConnections drop path now destroys the wrapper so a late RST can't crash.
context.c happy-eyeballs FIN-vs-RST: the losing SEMI_SOCKET has no s->ssl yet, so us_socket_close with CLEAN_SHUTDOWN is a plain shutdown()+close, no TLS deferral.

Extended reasoning...

Overview

The PR removes three listenerCount("error") > 0 gates in src/js/node/net.ts (failWrite, SocketEmitEndNT, SocketHandlers2.close) so an unhandled socket 'error' reaches uncaughtException as in Node, instead of being silently swallowed. To keep that from surfacing spurious errors it also: attaches Node's onSocketTLSError equivalent to every TLS-server-accepted socket (guarded by a new ktlsClientErrorEmitted symbol so tlsClientError isn't double-reported); destroys the pre-'connection' wrapper in the blockList/maxConnections drop path; and changes the happy-eyeballs losing-leg close in packages/bun-usockets/src/context.c from RST to FIN. Seven test fixtures gain early .on("error", () => {}) on accepted sockets, and four new subprocess tests cover the client/server/uncaught/maxConnections cases plus a listenerCount test for the TLS accept path.

Security risks

None identified. The change tightens error reporting rather than loosening any check. onServerSocketTLSError matches Node's _tls_wrap.js behavior and only routes pre-handshake errors to tlsClientError; post-handshake it is inert.

Level of scrutiny

High. This is a deliberate behavioral change to core node:net error semantics: user code that previously ran cleanly on Bun (because ECONNRESET was swallowed) will now crash with uncaughtException, matching Node. The native context.c change alters what a net.Server observes for every dual-stack localhost connect. The PR description also flags two Windows-only upstream Node tests (test-https-agent-unref-socket.js, test-https-server-close-all.js) that now fail because Bun's native layer sends RST where libuv sends FIN — a pre-existing native issue this PR exposes rather than introduces, but still a CI-visible regression a maintainer should sign off on.

Other factors

I reviewed this across four earlier rounds and every inline finding was addressed in follow-up commits (8108029, 42ec6b4, dc4420f, 8bfef4a, 9e07c6e): the SocketHandlers2.close fall-through now matches SocketEmitEndNT including the socket === self._handle guard; the handshake-timeout timer sets ktlsClientErrorEmitted; the maxConnections/blockList drop path destroys the wrapper (with a dedicated test); the two stale why-comments in net.ts:848 and http2.ts:6808 were updated; and the new node-tls-server.test.ts test registers secureConnection before connect() and attaches a client error listener. The regression sweep in the description (net/tls/http/http2/https suites unchanged vs main on Linux) is reassuring, but the combination of core-semantics change + native-layer change + known Windows follow-ups is exactly the kind of thing a maintainer should look at before merge.

robobun · 2026-09-13T18:14:43Z

Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-18, 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.

github-actions Bot added the claude label Jul 17, 2026

coderabbitai Bot reviewed Jul 17, 2026

View reviewed changes

Comment thread src/js/node/net.ts

claude Bot reviewed Jul 17, 2026

View reviewed changes

Comment thread src/js/node/net.ts