node:tls: report a wrapped socket's connect failure on the TLSSocket

[robobun]

Problem

• tls.connect({ socket }) over a net.Socket whose connect is still in flight (net.connect(...) passed straight in, or a new net.Socket() connected after being wrapped) never reports a failed connect: the plain socket emits error + close, the TLSSocket emits nothing, and a caller waiting on its secureConnect / error / close hangs.
• If the caller only listens on the TLSSocket (the documented usage), the plain socket's ECONNREFUSED is an uncaught exception instead. Same after tls.connect({ socket }).destroy() while the socket was still connecting.
• The connect listener also stays armed after the failure: a net.Socket that is reconnected afterwards (retry logic reusing the socket) is upgraded to TLS by the dead wrap, so a plain server gets a ClientHello and the orphaned TLSSocket reports ERR_SSL_WRONG_VERSION_NUMBER.
• Cause: the wait-for-connect arm of Socket.prototype.connect (src/js/node/net.ts, around line 2030) only registers connection.once("connect", ...). Nothing links the two sockets until that upgrade runs, so the connection's error / close go nowhere. The already-connected arm (upgradeTLSDeferred) and the duplex arm (upgradeDuplexToTLS wires close) do not have this gap.
• Node prints raw error ECONNREFUSED, tls error ECONNREFUSED, raw close, tls close for the repro; bun printed only the two raw lines.

Fix

• While waiting for connect, also listen for the connection's error (re-emitted on the TLSSocket through _emitTLSError) and close (destroys the TLSSocket). connect removes both listeners before upgrading; close removes the connect listener, which is what stops the dead wrap from upgrading a later reconnect.
• Matches node: _init does wrap.on('error', err => this._emitTLSError(err)) and _wrapHandle does wrap.on('close', () => this.destroy()) (lib/internal/tls/wrap.js L977 / L740 in v26.3.0). Going through _emitTLSError keeps node's _controlReleased semantics, and the resulting events (error carrying the connection's own error object, then close with hadError === false) are exactly node's.
• The listeners are scoped to the waiting period because after the upgrade the adopted native handle (or the duplex engine) already reports the connection's errors and close to the TLSSocket itself, and a net.Socket can be reconnected after it closes, so listeners left behind would act on a TLSSocket that has moved on.
• error is ignored once the TLSSocket is already destroyed: bun keeps the connection connecting until connect fires after tls.connect({ socket }).destroy(), and its failure must not surface as an error after the TLSSocket's close. The listener still absorbs it, so it is no longer an uncaught exception either.
• Complementary to node:tls: make client-side new TLSSocket(socket) upgrade the socket it wraps #37664 / node:tls: destroy the wrapped net.Socket when tls.connect({ socket }) is destroyed #34507, which tear the wrapped socket down when the TLSSocket is destroyed (the other direction); neither makes a failed connect reach the TLSSocket. One more neighbour: a client TLSSocket built with new tls.TLSSocket(socket) and then explicitly connect({ socket })ed keeps the stream as its _handle, and destroy() on such a socket throws handle.close is not a function on main today (node:tls: handle destroy() on a TLSSocket wrapping an unconnected stream #35842 fixes that); tls.connect({ socket }), the path this PR is about, has no _handle while waiting.
• Verified with the five new tests in test/js/node/tls/node-tls-connect.test.ts (describe("tls.connect({ socket }) over a net.Socket whose connect has not completed yet")): connect in flight, connect started after wrapping with listeners on the TLSSocket only, raw.destroy() while connecting, tlsSocket.destroy() while connecting, and reconnecting the socket after the failure. All five fail without the src change (two time out, two die on the unhandled ECONNREFUSED, the reconnect one gets ERR_SSL_WRONG_VERSION_NUMBER; checked with USE_SYSTEM_BUN=1 for all five and with a bun bd build of main for the first four), pass with the fix, and stayed green over repeated runs.
• The rest of node-tls-connect.test.ts, node-tls-connect-hostname-verification.test.ts, node-http-connect.test.ts's https CONNECT case (wraps a still-connecting socket and expects the handshake to succeed), and the vendored test-tls-net-socket-keepalive, test-tls-connect-given-socket, test-socket-writes-before-passed-to-tls-socket, test-tls-connect-stream-writes, test-tls-socket-close, test-tls-socket-destroy, test-tls-connect-pipe, test-tls-connect-simple, test-tls-connect-abort-controller, test-tls-socket-allow-half-open-option, test-tls-socket-failed-handshake-emits-error pass. node-net.test.ts has the same 12 failures with and without the change (this container resolves localhost to ::1 for the server and 127.0.0.1 for the client; node behaves the same here).

Background

• tls.connect({ socket }) builds a TLSSocket over a transport the caller owns instead of opening its own TCP connection. In bun the TLS layer is attached in Socket.prototype.connect, in one of three ways: a connected net.Socket has its native handle adopted on the spot (upgradeTLSDeferred), a generic duplex / named pipe gets a stream-level TLS engine (upgradeDuplexToTLS), and a net.Socket that has no handle yet or is still connecting waits for its connect event and then takes one of the first two paths. Only that third, waiting state had no link between the two sockets.
• _emitTLSError is node's routing for errors that happen on a TLSSocket's underlying transport: it emits _tlsError and, once _releaseControl() has handed the socket to the user (which tls.connect() does right away), emits error. It does not destroy the socket; in node the socket is destroyed by the transport's close, which is why the TLSSocket's close carries hadError === false in this flow.

Repro and node / bun output

import net from "node:net";
import tls from "node:tls";
const srv = net.createServer().listen(0, "127.0.0.1", () => {
  const port = srv.address().port;
  srv.close(() => {
    const raw = net.connect({ port, host: "127.0.0.1" }); // still connecting when wrapped
    raw.on("error", e => console.log("raw error", e.code));
    raw.on("close", hadError => console.log("raw close", hadError));
    const c = tls.connect({ socket: raw, rejectUnauthorized: false });
    c.on("error", e => console.log("tls error", e.code));
    c.on("close", hadError => console.log("tls close", hadError));
  });
});

node v26.3.0 and bun with this change:

raw error ECONNREFUSED
tls error ECONNREFUSED
raw close true
tls close false

bun 1.4.0 (and a debug build of main):

raw error ECONNREFUSED
raw close true

The same probe with raw.destroy() right after wrapping prints raw close false / tls close false on node and with this change, only raw close false before. With c.destroy() right after wrapping and no listeners on raw, bun before this change exits with an uncaught ECONNREFUSED; node and bun with this change print tls close false and exit cleanly.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 26 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: 1f649717-cbd6-40f7-aebb-62a471fd727e

📥 Commits

Reviewing files that changed from the base of the PR and between b7a0431 and 9c8e832.

📒 Files selected for processing (2)

• src/js/node/net.ts
• test/js/node/tls/node-tls-connect.test.ts

---

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

[robobun]

^{Updated 2:38 PM PT - Aug 13th, 2026}

✅ @robobun, your commit 9c8e832106d8afac57a1903601520e1227c9946a passed in Build #94795! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 38122

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

bun-38122 --bun

[robobun]

Status: reproduced and fixed, waiting on CI.

• Reproduced on bun 1.4.0 and a debug build of main with the repro in the PR description: tls.connect({ socket: net.connect(closedPort) }) prints only the plain socket's error / close; node also prints them on the TLSSocket. With the caller listening on the TLSSocket alone, the plain socket's ECONNREFUSED is an uncaught exception instead, and a socket reconnected after the failure gets upgraded to TLS by the dead wrap (ERR_SSL_WRONG_VERSION_NUMBER).
• Fix is in Socket.prototype.connect's wait-for-connect arm (src/js/node/net.ts): the connection's error is re-emitted through _emitTLSError and its close destroys the TLSSocket; every listener is removed once connect or close fires.
• Tests: the describe at the end of test/js/node/tls/node-tls-connect.test.ts (five cases); all of them fail without the src change and pass with it.

[claude]

LGTM — focused Node compat fix wiring the wrapped socket's error/close to the waiting TLSSocket.

What was reviewed:

• Traced listener lifecycle: onConnect removes error+close before upgrading; onClose removes connect+error then destroys; onError is absorbed (no re-emit) once the TLSSocket is already destroyed — no leaked listeners on any path.
• Confirmed _emitTLSError is on TLSSocket.prototype and _releaseControl() runs in tls.connect before these can fire, so the error reaches user listeners.
• Checked the four new tests: hermetic (127.0.0.1, refusedPort() holds a live local port so no listen(0) race), await the actual close, and cover connect-in-flight / connect-after-wrap / raw.destroy() / tlssock.destroy() variants.

Extended reasoning...

Overview

Adds error and close listeners on the wrapped net.Socket for the wait-for-connect arm of Socket.prototype.connect in src/js/node/net.ts (~22 lines), so a failed underlying connect reaches the TLSSocket instead of leaving it pending or surfacing as an uncaught exception. Four new tests in test/js/node/tls/node-tls-connect.test.ts assert the exact event sequences node v26.3.0 produces.

Security risks

None. This is error/close event forwarding during the pre-handshake window; it does not touch certificate verification, rejectUnauthorized, trust stores, or any handshake logic. The only new call is to the existing _emitTLSError, which routes through node's _tlsError/_controlReleased semantics already in tls.ts.

Level of scrutiny

Moderate — TLS-adjacent networking code, but the change is a small, additive event-listener wiring in a single well-understood branch. The already-connected (upgradeTLSDeferred) and duplex (upgradeDuplexToTLS) arms are untouched; the PR description correctly notes those already have equivalent linkage. I traced each path: on connect the new listeners are removed before the (unchanged) upgrade runs; on close the connect/error listeners are removed and this.destroy() is called (idempotent if already destroyed); onError is guarded by !this.destroyed so a post-destroy connect failure is absorbed but not re-emitted, matching node and the fourth test. The arrow functions correctly bind this to the TLSSocket, and this branch is only reachable when bunTlsSymbol is a function (so _emitTLSError exists on the prototype).

Other factors

• Tests follow the repo's guidance: no sleeps, await the observable close, wire failure paths explicitly (closed() helper avoids events.once rejecting on the expected error), await using disposes the port holder. The refusedPort() helper avoids the bind-then-release race by using a held ephemeral local port.
• PR description reports all four tests fail on main / with USE_SYSTEM_BUN=1 and pass with the fix over 10 repeated runs, plus a broad set of adjacent tls/net tests remain green.
• No CODEOWNERS entry for src/js/node/net.ts. Matches local style (plain .on/.once/.removeListener, same as neighboring lines 1999–2002).

[claude]

Automated review ran and found no bugs. The fix is small and well-tested, but it adds interacting event listeners to the TLS wrap's wait-for-connect state machine in src/js/node/net.ts — a human look at the socket-lifecycle interactions would still be worthwhile.

Checked: listener add/remove pairing (onConnect/onClose each remove the others; .once self-removes), _emitTLSError exists on TLSSocket.prototype and matches node's _controlReleased gating, this.destroy() in onClose is idempotent when the TLSSocket was already destroyed, and .on (not .once) for error matches node's _init. The five new tests cover the variant matrix (connect-in-flight, connect-after-wrap, raw.destroy(), client.destroy(), reconnect-after-failure) and each awaits an actual close rather than sleeping.

Extended reasoning...

Overview

The PR fixes tls.connect({ socket }) when the wrapped net.Socket is still connecting: previously only a connect listener was registered on the underlying connection, so a failed connect (ECONNREFUSED) never reached the TLSSocket — callers waiting on secureConnect/error/close would hang, and the plain socket's error could become an uncaught exception. The fix adds error (routed through _emitTLSError) and close (destroys the TLSSocket) listeners for the waiting period, each removing the others when it fires. This mirrors node's internal/tls/wrap.js _init/_wrapHandle. Two files touched: ~15 net lines in src/js/node/net.ts:2030-2091 and five new tests in test/js/node/tls/node-tls-connect.test.ts.

Security risks

None identified. The change is error-propagation plumbing during the pre-handshake wait; it doesn't touch certificate verification, rejectUnauthorized, or any crypto path. The onError guard (if (!this.destroyed)) prevents emitting on a dead socket rather than suppressing anything security-relevant.

Level of scrutiny

Moderate-to-high. src/js/node/net.ts is core networking infrastructure that underpins TLS/HTTP/HTTPS. The change itself is small and the listener add/remove pairing checks out, but socket-lifecycle state machines in the TLS wrap are subtle — the interaction between onConnect removing onClose, onClose removing onConnect, and the this.destroyed guard in onError deserves a look from someone with deep knowledge of bun's upgrade paths (upgradeTLSDeferred / upgradeDuplexToTLS). Not a mechanical config/typo change.

Other factors

• The PR description is unusually thorough: cites the exact node source lines being mirrored, explains why listeners are scoped to the waiting period (post-upgrade the native handle already reports errors), and documents which existing tests were re-run.
• Test coverage is strong: five cases covering connect-in-flight, connect-after-wrap, raw.destroy(), client.destroy(), and socket reuse after failure. Tests await real close events (not sleeps), use a refusedPort() helper that holds a live connection's local port to guarantee ECONNREFUSED, and assert exact event sequences including hadError values matched against node v26.3.0.
• The comment-cop bot's feedback about long comments was addressed in 9c8e832 and both threads are resolved.
• No CODEOWNERS entry covers these paths.
• Verified _emitTLSError exists on TLSSocket.prototype (src/js/node/tls.ts:853) and its semantics (emits _tlsError, then error if _controlReleased) match what the PR description claims.