[robobun]

Fixes #32239. Fixes #32242. Fixes #40646. Consolidates #32241, #32243 and #32372 (@Sids15, credited as co-author).

Problem

After tls.connect({ socket }) or new tls.TLSSocket(socket, { isServer: true }) puts a TLS layer on a net.Socket, that socket kept receiving the connection's ciphertext as data: a STARTTLS handler still attached re-enters and upgrades again (Invalid socket, #32239); with no listener (postgres.js) the ciphertext piles up in the socket's readable buffer for the life of the connection (4 GB retained on a 4 GB transfer, #32241).

What Node does

new TLSSocket(sock) keeps the original net.Socket alive (tlsSocket._parent / _parentWrap) with its own handle, for close/destroy/address/ref — but TLSWrap does stream->PushStreamListener(this), so every read on that handle goes to the TLS layer and the parent never sees another byte. Output is held while the parent still has writes queued (wrapHasActiveWriteFromPrevOwner). Anything that is not a handle-backed net.Socket is wrapped in a JSStreamSocket fed from data events; initRead drains whatever the parent had already buffered into the engine.

Fix

The upgrade is now modelled the same way, and almost entirely natively:

• The wrapped socket keeps its own handle. upgradeTLS no longer creates a second TLSSocket "raw twin" and swaps it in as connection._handle: the original TCPSocket follows the fd into the adopted us_socket_t (BYPASS_TLS: writes skip SSL; close/writable are chained to it from the TLS half). For the public Bun.Socket.upgradeTLS(), raw === socket now (same handlers, same data), and upgrading a raw half again returns undefined. node:net's kAdoptedTLSRaw/kCloseRawConnection bookkeeping is gone; a fdIsTLS getter tells _destroy/connect() the fd belongs to a TLS layer.
• No ciphertext into the parent. The node:net entry point (jsNodeNetUpgradeTLS) doesn't enable the ssl_raw_tap, so nothing of the TLS stream is dispatched to JS. Whatever the parent had already buffered (every chunk) is handed over as initialData; strings (after setEncoding) fail with ERR_STREAM_WRAP like Node's JSStreamSocket.
• Pending plaintext writes: TLS output held natively (us_socket_hold_tls_output / us_socket_release_tls_output, node's has_active_write_issued_by_prev_listener_): the fd is adopted immediately; whatever SSL emits (the handshake flight) is kept in a per-SSL buffer instead of written, while input is processed as usual (so garbage or a FIN during the hold is an ordinary mid-handshake failure). node:net queues an empty write behind the plaintext and calls startTLSHandshake() from its callback; release writes the buffer out ahead of anything newer. Plaintext still queued natively drains through the raw half first (the TLS half chains writable to it). A close while held reports ECONNRESET like any close before the handshake finished and puts no TLS bytes on the wire. The stream-level engine gets the same hold (held_output) when its handle-backed transport still has JS writes queued, writes its ciphertext into that transport natively (write_from_engine), and a shutdown() with engine output still parked waits for it.
• A still-connecting net.Socket is upgraded on 'connect' (TCP, named pipe, or an outer TLS socket alike), as Node's TLSSocket._start waits for 'connect' before _handle.start(); there is no fd to adopt and no transport to write a ClientHello into before that.
• Stream-level engine transports fed natively. upgradeDuplexToTLS over a handle-backed net.Socket (named pipes, TLS over TLS, Http2SecureServer#emit('connection')) takes transport: connection._handle; NativeCallbacks::TlsTransport routes that socket's data/end/close into the engine (never through JS or the parent's listeners), and the engine's pause/resume reach the transport (backpressure for these paths, previously a TODO). A plain Duplex keeps the data-event wiring, as Node's JSStreamSocket does. kTLSUpgradeSink and the per-chunk sink checks in the four data handlers are gone; internal/net/streamTLS.ts is the one helper for net.ts and _http2_upgrade.ts.
• Lifecycle parity: TLSSocket._parent is set (idle timeouts on the parent are refreshed by TLS activity; ref()/unref() on either socket count); the TLS socket is destroyed when the wrapped stream closes/errors and the wrapped stream is destroyed with the TLS socket (_wrapHandle/doClose); _final waits for a handle that is still on its way; connect() on a socket whose fd a TLS layer owns fails with EISCONN instead of pulling the fd out from under it; the HTTP/2 raw-socket upgrade destroys the raw socket with the session and pauses the native handle for backpressure.
• upgrade_tls_impl: initialData is fed before the tap is enabled, so the public upgradeTLS({ initialData }) no longer shows raw those bytes a second time.

Verification

test/js/node/tls/node-tls-upgrade.test.ts (34 cases: STARTTLS repro, client fd adoption immediate / behind a corked write / behind real backpressure, onread, server adoption immediate / behind a pending write / write issued after the wrap, bytes arriving while writes are pending, ClientHello buffered in one and in several chunks, setEncoding → ERR_STREAM_WRAP, TLS over TLS incl. peer close of the outer layer, no-listener retention, reconnect after TLS / EISCONN while wrapped, parent idle timeout, ref()/unref() through the parent, destroy / end() / peer FIN / peer flood while the handshake is held (client and server side), half-open write not cut off), test/js/bun/net/socket.test.ts (raw === socket, double upgrade, close-handler cross-calls), socket-retention.test.ts (raw halves collectable), node-http2-upgrade.test.mts, cluster.test.ts, node-tls-namedpipes.test.ts (Windows). test/js/node/{tls,net,http2}, test/js/bun/net and all vendored test-tls-*/test-https-*/test-net-*/test-http2-* node tests pass as on main (plus test-tls-connect-allow-half-open-option.js, newly vendored). Scripts driving client/server STARTTLS, TLS-over-TLS, pending-write, destroy-while-flushing, multi-chunk ClientHello, parent-dies-before-adoption and reconnect print the same results under this build and Node 26. TLS bulk-write throughput (1 KiB / 16 KiB writes, release build) is unchanged within noise.

Notes

• State on 2026-09-22: this branch is 425 commits behind main and conflicts in 7 files. node:net: fix tls.connect({ socket }) and onread on a readable: false socket #42265 fixed the fd-adoption path on main and closed tls.connect({ socket }) fails to intercept stream, causing subsequent data to re-trigger cleartext listeners and throw "Invalid socket" #32239 and Bun 1.4 retains grpc-js unary response buffers until remote channel closes #40646. tls.connect({ socket }) over a Windows named pipe can re-emit post-upgrade bytes on the original socket #32242 and the stream-level path (a write pending at upgrade time, TLS over TLS, named pipes) still reproduce on main.
• Related report, tracked as a separate fix on main: a TLS socket over a net.Socket transport never hears that transport's error (node:tls: report a Duplex transport's error on the TLS socket #42235 covers a Duplex transport only). tieToWrappedStream in this PR forwards error and close for every wrapped stream.

---

no test proof · iteration 8 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/tls/node-tls-namedpipes.test.ts, test/js/node/cluster.test.ts, test/js/bun/net/socket.test.ts, test/js/bun/net/socket-retention.test.ts

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

TLS upgrade paths now route incoming socket data through a shared TLS feeder. Buffered and onread data bypass the original stream after takeover. Runtime tap timing prevents duplicate initial bytes. Regression tests cover TCP, named pipes, cluster workers, and HTTP/2.

Changes

Duplex TLS upgrade routing

Layer / File(s)	Summary
Direct TLS feeder routing src/js/internal/net/symbols.ts, src/js/internal/net/tlsFeeder.ts, src/js/node/net.ts	Adds the TLS sink and feeder utilities. Socket handlers send owned bytes to TLS and suppress normal readable delivery.
Upgrade path integration src/js/node/net.ts, src/js/node/_http2_upgrade.ts	Client, server, and HTTP/2 upgrade paths attach the feeder. TLS pair adoption and cleanup update feeder state.
Initial TLS data timing src/runtime/socket/socket_body.rs, src/runtime/socket/mod.rs	Node-specific upgrades use the renamed runtime binding. Public upgrades enable raw TLS tapping after initial_data replay.
Upgrade regression coverage test/js/node/tls/node-tls-upgrade.test.ts, test/js/node/tls/node-tls-namedpipes.test.ts, test/js/node/cluster.test.ts, test/js/node/http2/node-http2-upgrade.test.mts	Tests verify single delivery, buffered input handling, TLS takeover, reconnect behavior, idle timeouts, and no raw-socket emissions or buffered bytes.

Suggested reviewers: alii, cirospaciari, jarred-sumner

Merge Risk: 🔵 Low · up to 139ed

The PR correctly stops the original socket from receiving TLS ciphertext, but a deferred upgrade failure can still surface as an uncaught exception, while one regression test may be flaky or report failures only as timeouts. The change is mergeable with explicit owner follow-up on these bounded risks.

🚥 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 identifies the main change: preventing TLS traffic from surfacing on the original socket after TLS takeover.
Description check	✅ Passed	The description explains the problem, implementation, behavior, and verification. It provides the information required by the template, although it uses equivalent headings instead of the exact templa…

---

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

[github-actions]

Found 1 issue this PR may fix:

1. tls.connect({ socket }) fails to intercept stream, causing subsequent data to re-trigger cleartext listeners and throw "Invalid socket" #32239 - tls.connect({ socket }) fails to intercept stream, causing subsequent data to re-trigger cleartext listeners and throw "Invalid socket"

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

Fixes #32239

🤖 Generated with Claude Code

[github-actions]

This PR may be a duplicate of:

1. net: stop re-emitting post-upgrade bytes on the original socket after tls.connect({ socket }) #32241 - Fixes the same post-upgrade byte re-emission bug on the original socket after tls.connect({ socket })
2. net: stop re-emitting post-upgrade bytes on a named-pipe socket after tls.connect({ socket }) #32243 - Addresses the same re-emission bug on the named-pipe/stream-level TLS path (explicitly superseded by this PR)
3. stop named-pipe tls.connect({ socket }) re-emitting post-upgrade bytes on the original socket #32372 - Same named-pipe TLS upgrade byte re-emission fix (explicitly adopted/extended by this PR)
4. tls: buffer duplex data that arrives before the SSL engine is created #32750 - Fixes the same timing window in the duplex TLS upgrade path where bytes arrive before the SSL engine is ready

🤖 Generated with Claude Code

[robobun]

Relationship to the PRs the bot flagged:

• net: stop re-emitting post-upgrade bytes on the original socket after tls.connect({ socket }) #32241 is the sibling fd-adoption path (socket.upgradeTLS), which replaces the handle and needs suppression, not a feeder. This PR does not touch that path.
• net: stop re-emitting post-upgrade bytes on a named-pipe socket after tls.connect({ socket }) #32243 and stop named-pipe tls.connect({ socket }) re-emitting post-upgrade bytes on the original socket #32372 are earlier attempts at this same bug on the duplex path; both predate tls: close_notify on end(), injected-socket upgrades, reject-handshake wire fix, duplex data-loss, SNI, ALPN (+14 tests, tls 81%→86%) #34598 (the hasUnflushedWrites trigger and server-side upgradeDuplexToTLS sites) and no longer apply cleanly. This PR supersedes them.
• tls: buffer duplex data that arrives before the SSL engine is created #32750 is a different bug: bytes arriving in the window before the SSL engine starts were dropped. That has since been addressed natively (UpgradedDuplex::on_internal_receive_data stages into pending_data and drain_pending replays it). tls: buffer duplex data that arrives before the SSL engine is created #32750 and this PR touch adjacent code but fix different failures.

[claude]

Beyond the inline note on the _http2_upgrade.ts sibling site, I also checked whether attachDuplexTLSFeeder could stall the handshake when the underlying native handle is paused — the four raw data handlers it targets only pause via socket.pause() on push() returning false, which the feeder path now bypasses, so no new stall is introduced. Also confirmed the buffered-bytes drain (connection.read()) only fires when readableLength > 0, so it doesn't flip an otherwise-idle socket into flowing mode.

Extended reasoning...

The PR routes post-upgrade bytes on the duplex-TLS path through a stored feeder instead of the public data event, touching all four raw socket data handlers plus four upgradeDuplexToTLS call sites in src/js/node/net.ts. One same-class sibling in _http2_upgrade.ts:386 remains on the public-event wiring (posted inline as a nit; pre-existing, not a regression). Given this modifies the hot data path for every net.Socket read and sits in TLS-adjacent code, deferring to a human reviewer rather than approving.

[robobun]

^{Updated 9:11 PM PT - Sep 1st, 2026}

❌ @Jarred-Sumner, your commit 6cad8a0 has 1 failures in Build #109207 (All Failures):

• test/cli/run/require-cache.test.ts - code 1: error: Memory leak detected on 🐧 13 x64-asan

---

🧪   To try this PR locally:

bunx bun-pr 36534

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

bun-36534 --bun

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/js/node/net.ts (1)

2402-2431: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Add server-side feeder regression coverage.

Existing server tests wrap TLSSocket instances, but they do not assert that the original raw socket receives no TLS ciphertext. The banner test writes through the wrapped socket, so it does not force the deferred hasUnflushedWrites(connection) branch. Add tests that force both server stream-upgrade branches and assert zero TLS ciphertext on raw-socket data listeners.

🤖 Prompt for 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.

In `@src/js/node/net.ts` around lines 2402 - 2431, Add server-side regression
tests for the stream-upgrade flow around hasUnflushedWrites(connection),
covering both the normal and deferred upgradeDuplexToTLS branches. Attach a data
listener to the original raw connection, write through the wrapped TLSSocket,
and assert the raw socket emits no TLS ciphertext in either branch; explicitly
queue an unflushed write to exercise the deferred branch.

Source: Coding guidelines

🤖 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-upgrade.test.ts`:
- Around line 9-13: Remove the explanatory regression-test comments, leaving
exactly the issue URL comment at each affected site:
test/js/node/tls/node-tls-upgrade.test.ts lines 9-13, 18-20, 37-40, 47-49,
63-66, and 93-94, plus test/js/node/tls/node-tls-namedpipes.test.ts lines
125-128. Preserve the tests and issue URL comments unchanged.
- Around line 45-52: Move the terminal event listeners for tlsSocket before
raw.uncork() so errors, secureConnect, or close triggered by the flush are
always observed and the test promise settles. Ensure every relevant failure
event rejects or otherwise records failure while preserving the existing leaked
result for acceptable terminal outcomes.

---

Outside diff comments:
In `@src/js/node/net.ts`:
- Around line 2402-2431: Add server-side regression tests for the stream-upgrade
flow around hasUnflushedWrites(connection), covering both the normal and
deferred upgradeDuplexToTLS branches. Attach a data listener to the original raw
connection, write through the wrapped TLSSocket, and assert the raw socket emits
no TLS ciphertext in either branch; explicitly queue an unflushed write to
exercise the deferred branch.

🪄 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: 55904209-0e61-4511-9d0e-1829990e8f21

📥 Commits

Reviewing files that changed from the base of the PR and between 21932c3 and fdba7f3.

📒 Files selected for processing (4)

• src/js/node/_http2_upgrade.ts
• src/js/node/net.ts
• test/js/node/tls/node-tls-namedpipes.test.ts
• test/js/node/tls/node-tls-upgrade.test.ts

[robobun]

CI status on build #86132: the two new tests in node-tls-upgrade.test.ts and the new Windows test in node-tls-namedpipes.test.ts pass on every lane, as does node-http2-upgrade.test.mts.

The red on that build is unrelated to this diff: test-fs-read-stream-pos.js (pre-existing timeout on debian aarch64, also red on main), plus [flaky]-tagged retries in napi.test.ts, setInterval.test.js, fetch-tls-abortsignal-timeout.test.ts, bun-security-scanner-matrix, and 08965.test.ts (each passed alone or on retry). None touch node:net, node:tls, or node:http2.

Ready for review.

[claude]

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

• 🔴 src/js/node/_http2_upgrade.ts:390-395 — This site now sets rawSocket[kDuplexTLSFeeder] unconditionally, but Http2SecureServer#emit('connection', socket) (http2.ts:6828) gates only on !(socket instanceof TLSSocket) and per Node's docs accepts any Duplex — for a plain Duplex (or a net.Socket without _handle) nothing ever invokes the feeder after the initial read() drain, so the TLS handshake hangs where the previous rawSocket.on("data", events[0]) worked. attachDuplexTLSFeeder in net.ts already carries the instanceof Socket && connection._handle gate with an .on("data") fallback; this site (and the onTlsClose teardown at line 263) needs the same gate — exporting the helper would keep all five sites in lockstep.

  Extended reasoning...

  What the bug is

  The follow-up commit that covers the fifth upgradeDuplexToTLS site replaced rawSocket.on("data", events[0]) with:

  rawSocket[kDuplexTLSFeeder] = events[0];
  let buffered;
  while ((buffered = rawSocket.read()) !== null) events[0](buffered);

  unconditionally. But kDuplexTLSFeeder is only read by net.ts's four native raw-data handlers (SocketHandlers.data, ServerHandlers.data, SocketHandlers2.data, and the onread variant at net.ts:420/742/1269/1764). Those handlers fire only for a net.Socket backed by a live native _handle. For any other stream, storing the symbol is a no-op — nothing calls it.

  Code path that reaches it

  src/js/node/http2.ts:6825-6829:

  emit(event, ...args) {
    if (event === "connection") {
      const socket = args[0];
      if (socket && !(socket instanceof TLSSocket)) {
        return upgradeRawSocketToH2(connectionListener, this, socket);
      }
    }
    ...
  }

  The gate is only !(socket instanceof TLSSocket). Node's docs for Http2SecureServer's 'connection' event explicitly state: "This event can also be explicitly emitted by users to inject connections into the HTTP server. In that case, any Duplex stream can be passed." The rawSocket: import('node:net').Socket type annotation is compile-time-only and not enforced in built-in modules.

  Why the existing code doesn't cover it

  net.ts's own attachDuplexTLSFeeder (added in this PR at line 1908) handles exactly this distinction:

  function attachDuplexTLSFeeder(connection, onData) {
    if (connection instanceof Socket && connection._handle) {
      connection[kDuplexTLSFeeder] = onData;
      let chunk;
      while ((chunk = connection.read()) !== null) onData(chunk);
    } else {
      connection.on("data", onData);
    }
  }

  The _http2_upgrade.ts site copies only the if-branch body and drops the gate. onTlsClose (line 263) was likewise changed to only clear the symbol, so it also assumes the feeder path.

  Step-by-step proof

  1. User creates h2s = http2.createSecureServer(opts) and a transport that delivers a plain Duplex (e.g. a tunnel, a socket pair, or a net.Socket whose _handle is null because it was constructed from an fd/JSStream wrap).
  2. User calls h2s.emit('connection', duplex) — the documented Node.js pattern.
  3. Http2SecureServer#emit sees !(duplex instanceof TLSSocket) → calls upgradeRawSocketToH2(connectionListener, this, duplex).
  4. upgradeRawSocketToH2 sets duplex[kDuplexTLSFeeder] = events[0] and drains duplex.read() until null (typically nothing yet).
  5. The peer sends its ClientHello. The bytes arrive via duplex.push(buf) → readable buffer. There is no 'data' listener (this PR removed it) and nothing calls read() again, so the stream never flows. net.ts's raw handlers never fire because there is no native _handle.
  6. events[0] is never invoked → the TLS engine receives zero bytes → handshake hangs indefinitely.

  Before this PR, step 4 was rawSocket.on("data", events[0]), which put the stream into flowing mode and delivered every pushed chunk to the feeder — the connection worked.

  Impact

  Regression on a documented Node.js API surface: h2SecureServer.emit('connection', duplex) with a non-native-handle Duplex now hangs where it previously succeeded. Per REVIEW.md's "fix the whole class in the same PR" rule (which this PR already follows at the four net.ts sites), this fifth site should carry the same gate.

  Fix

  Apply the same instanceof Socket && rawSocket._handle gate here (falling back to rawSocket.on("data", events[0])), and mirror it in onTlsClose so the fallback branch removes the listener. The cleanest option is to export attachDuplexTLSFeeder (and a matching detach) from net.ts and reuse it here so all five upgradeDuplexToTLS sites stay in lockstep.

[robobun]

Re the _http2_upgrade.ts finding: addressed in 8c99304. The feeder is now gated on rawSocket._handle (matching attachDuplexTLSFeeder in net.ts), with on("data") as the fallback; onTlsClose undoes both paths.

For what it's worth the specific case cited (Http2SecureServer#emit('connection', plainDuplex)) also hangs on unmodified main and in Node v26.3.0 with the same driver, so that path was not working before either; the gate here just keeps this PR from changing the wiring for non-handle inputs.

[claude]

Both earlier findings (the missing _http2_upgrade.ts site and the dropped _handle gate) are now addressed and this run found no further issues, but the change reroutes raw-socket data delivery across four hot-path handlers and five upgradeDuplexToTLS sites in net.ts/_http2_upgrade.ts, so it's worth a human look.

What was reviewed:

• Feeder short-circuit placement in all four data handlers — bytesRead and _unrefTimer still run before the early return; the onread variant correctly bypasses kOnreadTail.
• Feeder lifetime in net.ts — not cleared on TLS close, but neither was the prior .on('data', events[0]) listener, so no regression; initSocketHandle clears it for reused sockets.
• Symbol.for cross-module sharing matches the existing ::bunUpgradeServerTLS:: pattern in this file.
• The _http2_upgrade.ts inline gates on rawSocket._handle (not instanceof Socket && _handle as in attachDuplexTLSFeeder) — a non-Socket Duplex with a truthy _handle would take the feeder path and stall, but that input is not something Node produces.

Extended reasoning...

Overview

The PR fixes #32242 by rerouting post-upgrade ciphertext on the upgradeDuplexToTLS path so it reaches the TLS engine directly instead of surfacing on the wrapped socket's public data event. It touches src/js/node/net.ts (four raw data handlers, four upgrade call sites, a new attachDuplexTLSFeeder helper, a new kDuplexTLSFeeder symbol, and initSocketHandle) and src/js/node/_http2_upgrade.ts (the fifth upgrade site plus onTlsClose cleanup). Two new tests force the stream-level path platform-independently via cork()+write, and one Windows-only test covers real named pipes.

Security risks

The change is security-adjacent (TLS upgrade data routing) but does not touch crypto, certificate validation, or handshake logic — it only changes which callback receives already-arrived ciphertext bytes. Symbol.for("::bunDuplexTLSFeeder::") is globally reachable, so user code could set it to intercept raw bytes on its own socket, but that grants nothing beyond what a data listener already provides, and the pattern matches existing Symbol.for("::bun...::") keys in this file.

Level of scrutiny

High. net.ts is a large, hot-path core file and the four modified data handlers run for every incoming chunk on every socket. The mechanism is small per-site (a 4-line early return), but correctness depends on the feeder being set/cleared at exactly the right points across five call sites and two modules, and on the _handle gate correctly distinguishing native-backed sockets from plain Duplexes. This PR already went through two rounds of my own findings (missing fifth site; dropped _handle gate at that site), both now fixed.

Other factors

• CI build #86132 passed the new tests on every lane per the author's status comment; unrelated flakes noted.
• All prior review threads (mine, comment-cop, coderabbit) are resolved.
• The _http2_upgrade.ts gate uses rawSocket._handle alone rather than instanceof Socket && _handle; the author's rationale (only Bun-native net.Sockets carry _handle) is reasonable for real-world inputs but is a slight divergence from the shared helper — worth a maintainer glance rather than a blocker.
• Feeder cleanup on the net.ts side mirrors the pre-existing listener lifetime (never removed), so no regression there.

[Jarred-Sumner]

Can we simplify the native code parts of this instead?

[robobun]

This PR is JS-only (no Rust/C++ changes): src/js/node/net.ts, src/js/node/_http2_upgrade.ts, plus tests. The reason it touches four data handlers and five call sites is that each one currently does connection.on("data", events[0]), and the only way to stop post-upgrade bytes from also reaching a user data listener at the JS layer is to route the native handler's buffer to the feeder before push() runs.

A native alternative would be cleaner: have js_upgrade_duplex_to_tls (src/runtime/socket/socket_body.rs) notice when the connection JSValue's _handle is a Bun-native socket and redirect that handle's raw data delivery into UpgradedDuplex::on_internal_receive_data itself, the way upgradeTLS uses the ssl_raw_tap hook. Then no JS-side feeder symbol is needed, the four net.ts data handlers stay untouched, and the five connection.on("data", events[0]) lines can stay as-is (they would only fire for a plain Duplex, which has no native handle).

Happy to redo it that way if you prefer; it trades the JS plumbing here for a change in socket_body.rs / UpgradedDuplex.rs. Let me know which direction you want.

[github-actions]

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

[robobun]

Windows Server 2019 x64 debug build of 7c78dba: node-tls-upgrade.test.ts 35 pass, node-tls-namedpipes.test.ts 8 pass (with --timeout 60000 for the slow debug-only GC loop in the pre-existing test, as before). Linux debug build of a072706: node-tls-upgrade.test.ts 35 pass twice, node-http2-upgrade and socket-retention pass, lint clean. Build 109142 is running on 7c78dba.

[claude]

Code review found no issues

No high-confidence issues detected in this change.

[github-actions]

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

[claude]

Code review found no issues

No high-confidence issues detected in this change.

[robobun]

#32239 and #40646 are fixed on main now, by #42265 (commit 158ff6c). It drops the data of the adopted raw half in the JS handlers, so the wrapped socket sees no TLS bytes.

I verified #40646 on a build of main with a grpc-js 1.14.4 client over TLS (70,000 unary RPCs). readableLength of the wrapped socket stays 0 and heapUsed is flat. Bun 1.4.2 buffers 4.6 MB in the same run. The numbers are in #40646 (comment), and I closed that issue.

After the next rebase, the open issue for this PR is #32242 (the Windows named pipe path), which #42265 does not address. The native rework and the lifecycle parity also remain. The branch conflicts with main in src/js/node/net.ts, src/js/node/tls.ts and test/js/node/tls/node-tls-upgrade.test.ts.

[robobun]

@Jarred-Sumner this PR conflicts with main again, and this time the conflict is in the design, not only in the text. I pushed nothing. I need a decision from you.

State

• Head 6cad8a0 (Sep 2) is 425 commits behind main. A trial merge conflicts in 7 files and 15 hunks: net.ts (5), socket_body.rs (3), us_socket_t.rs (2), node-tls-upgrade.test.ts (2), tls.ts, mod.rs, libusockets.h.
• The branch no longer builds on the current toolchain (it pins clang 21.1.x, main uses 23). Typecheck the built-in modules (src/js) in CI #43649 now typechecks src/js in CI, so internal/net/streamTLS.ts and the new net.ts functions also need types.

Why it is not mechanical. Since Sep 2, main changed the same functions on the old structure:

• node:net: fix tls.connect({ socket }) and onread on a readable: false socket #42265 drops data on the adopted raw half in JS (kAdoptedTLSRaw). It closed tls.connect({ socket }) fails to intercept stream, causing subsequent data to re-trigger cleartext listeners and throw "Invalid socket" #32239, Bun 1.4 retains grpc-js unary response buffers until remote channel closes #40646 was closed the same day, and it added tests to node-tls-upgrade.test.ts.
• node:tls: inherit allowHalfOpen from the wrapped socket #39066 added destroyWhenUpgradedCloses / kOnUpgradedClose. This PR has tieToWrappedStream for the same job.
• node:tls: report a peer reset on a wrap of an accepted socket #42293 keeps the raw half's close callback first and holds the event loop entered across both callbacks (raw error, raw close, tls error, tls close). This PR dispatches the TLS half's close first, then the raw half's.
• node:tls: keep unread data and still emit 'close' when a wrapped transport closes #42176 (today) and the kPreHandshakeWrite rework of TLSSocket._final touch the other two places.

A merge has to pick one design in each place, and main's new tests pin main's choices.

What main still lacks without this PR (measured today)

• tls.connect({ socket }) over a Windows named pipe can re-emit post-upgrade bytes on the original socket #32242 still reproduces on the Windows canary at bf80d21. This branch's two named-pipe tests fail there (surfaced: 2980, upgrades: 2).
• 18 of the 35 tests in this branch's node-tls-upgrade.test.ts fail on the Linux canary (367d939). node:net: fix tls.connect({ socket }) and onread on a readable: false socket #42265 fixed the fd-adoption path only. When the upgrade goes through the stream-level engine (a write still pending at upgrade time, TLS over TLS, named pipes), the TLS traffic still surfaces on the wrapped socket (emitted: 2972). The server-side wrap hands an already buffered ClientHello over twice (2940 against 1470). Also failing: end() right after the upgrade over a flushing socket (hangs), unref() after the upgrade (hangs), EISCONN on reconnect, ERR_STREAM_WRAP. The other 17 pass on main now, the tls.connect({ socket }) fails to intercept stream, causing subsequent data to re-trigger cleartext listeners and throw "Invalid socket" #32239 and Bun 1.4 retains grpc-js unary response buffers until remote channel closes #40646 tests among them.

Related report. Another session reported that a TLS socket over a net.Socket transport never hears that transport's error. I confirmed it on the canary: after raw.destroy(err) the TLS socket gets no error with queued writes, a synthetic "disconnected before secure TLS connection" error otherwise, and the process dies with an uncaught exception when raw has no error listener. Node emits _tlsError and error on the TLS socket. #42235 covers the Duplex case only. tieToWrappedStream in this PR forwards error for every wrapped stream, so the redesign covers it, but I cannot re-run it because the branch no longer builds. I am routing that report to a standalone fix on main, so it does not wait for this PR.

Options

1. I re-integrate the redesign onto main: one merge commit, your commits untouched, verified on Linux and Windows as before. For that I need to know which side wins where the two disagree, first of all the close order from node:tls: report a peer reset on a wrap of an accepted socket #42293.
2. The redesign is parked. Then I close this PR and open small PRs on main's structure for what is still missing, starting with the stream-level leak (tls.connect({ socket }) over a Windows named pipe can re-emit post-upgrade bytes on the original socket #32242).

Which one do you want?

[Jarred-Sumner]

Superseded by #44618, which takes what still reproduced on main: a buffered ClientHello in several chunks, a paused transport, initialData echoed to the parent, unref() after adoption, and _parent, one test each. #32242 stays open as an issue.