tls: root the duplex origin and listener thunks through the wrapper instead of as Strong handles

[robobun]

What

UpgradedDuplex (the TLS engine that drives tls.connect({ socket: <Duplex> }) and the equivalent server wrap) held five StrongOptional handles per connection: the origin stream plus the four native listener thunks created in get_js_handlers. Each Strong is an independent HandleSet entry whose lifetime is tied to the native struct rather than to the JS wrapper's reachability.

Move all five onto the JSTLSSocket wrapper as visited values: slots (duplexOrigin, duplexOnData, duplexOnEnd, duplexOnWritable, duplexOnClose). UpgradedDuplex keeps plain JSValue shadows for its own reads and stack-roots the wrapper across on_close so the slots stay valid through the close handler's downgrade + re-entry into JS. teardown still neuters the thunks' function data via the shadows; the slots themselves are dropped with the wrapper.

Same pattern as #31859 (socket handlers into a visited cell) and #34346 (server callbacks traced from the JS wrapper).

Why

heapStats().protectedObjectTypeCounts before/after, 20 live upgrades:

                         before     after
Function (protected)       +80         0
Object   (protected)       +20         0
TLSSocket (protected)      +20       +20   // the wrapper's own self-reference, unchanged

There is no correctness bug to point at here: teardown releases all five Strongs and the existing tests exercise that path. The change is about the rooting model. A Strong is a VM-global root, so these five per connection are invisible to the GC's reachability graph and live exactly as long as the native struct does, no longer and no shorter. Holding them on the wrapper makes them ordinary traced edges: alive while the wrapper is, collected when it isn't, and reported in heap snapshots as edges from the TLSSocket that owns them.

Verification

• New test in test/js/bun/net/socket-retention.test.ts asserts the Function/Object protected-count delta across 20 live upgrades stays below N (was 4N and N respectively).
• Existing duplex coverage passes: node-tls-connect.test.ts (TLS over Duplex, session/keylog, destroy-in-data), node-tls-duplex-close-throw-uaf.test.ts (close/error ordering), renegotiation.test.ts (duplex path), socket-retention.test.ts (wrapper lifetime).
• rust:check-all clean across all targets.

---

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

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 3 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: 75b3b189-702c-4568-a779-4662aa3fe8f6

📥 Commits

Reviewing files that changed from the base of the PR and between 20b4a0b and 99107ff.

📒 Files selected for processing (4)

• src/runtime/socket/UpgradedDuplex.rs
• src/runtime/socket/socket_body.rs
• src/runtime/socket/sockets.classes.ts
• test/js/bun/net/socket-retention.test.ts

---

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

[robobun]

^{Updated 9:31 PM PT - Jul 18th, 2026}

❌ @robobun, your commit 99107ff has 3 failures in Build #75596 (All Failures):

• test/js/node/http/node-http-server-socket-end-drain.test.ts - code 1 on 🍎 26 aarch64
• 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/node/net/node-net.test.ts - code 1 on 🐧 3.23 aarch64

---

🧪   To try this PR locally:

bunx bun-pr 34672

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

bun-34672 --bun

[robobun]

Status: ready for review.

• Reproduced on main: 20 live tls.connect({socket: duplex}) upgrades add +80 Function and +20 Object to heapStats().protectedObjectTypeCounts; with this change both deltas are 0.
• Fail-before / pass-after covered by the new test in socket-retention.test.ts, green on every CI lane.
• Existing duplex coverage (node-tls-connect, node-tls-duplex-close-throw-uaf, renegotiation over duplex, socket-retention) passes.
• rust:check-all green.
• Self-review clean: every lifetime/ordering hazard probed (pre-open error, StartTLS failure, on_close downgrade-then-reenter) is covered by the early teardown() before handle_connect_error and the on_close stack-root.

CI (build 75596): the remaining red is unrelated to this diff:

• test/js/node/net/node-net.test.ts ("connect({path}) reused handle", mimalloc page delta 11 vs <10, alpine aarch64 only) — plain net.Socket unix path, no TLS; pre-existing on main.
• test/js/node/test/parallel/test-net-connect-memleak.js (alpine x64) — net.createConnection FinalizationRegistry timing; pre-existing on main.
• test/js/node/http/node-http-server-socket-end-drain.test.ts (darwin aarch64) — EPIPE race; pre-existing on main.
• darwin 26 aarch64 infra: Tart VM boot failed with "number of VMs exceeds the system limit".

All three test files use plain TCPSocket (whose values: list is unchanged), and all are already being tracked as main breaks.

[claude]

No bugs found this pass; the earlier UAF window and the stderr nit are both addressed. Deferring to a human because this swaps Strong GC roots for wrapper-visited slots + unrooted JSValue shadows in the TLS-over-Duplex path — a lifetime-model change where a missed ordering is a UAF under GC pressure.

What was reviewed:

• Traced every path into teardown(): on_close (stack-roots the wrapper), Drop via deinit from StartTLS (tls.is_none() and start_tls failure — both now neuter shadows before handle_connect_error downgrades), and EventState::Close (shadows already zeroed by the preceding on_close).
• Confirmed the wrapper's JSRef is strong (via mark_active()) for the entire window the shadows are read outside those brackets, so origin/thunks stay rooted through internal_write/call_write_or_end.
• Verified the codegen emits duplex_*_set_cached for the new values: entries and that TCPSocket keeps the plain ["handlers"] list.

Extended reasoning...

Overview

The PR moves five StrongOptional handles on UpgradedDuplex (the origin Duplex and four native listener thunks) onto the owning JSTLSSocket wrapper as visited values: slots, keeping plain JSValue shadows on the Rust struct for reads. teardown() still neuters the thunks' function data via the shadows and now also runs eagerly on both connect-error branches so the shadows are cleared before the wrapper's JSRef is downgraded. Files: UpgradedDuplex.rs, two call sites in socket_body.rs, the values: list in sockets.classes.ts, and a new protected-count regression test.

Security risks

No new attack surface. The risk is memory safety: a JSValue shadow that outlives its GC root is a use-after-free when set_function_data writes into a swept cell. My previous review identified exactly that on the pre-open-error → queued-StartTLS path; the fix (call upgrade.teardown() before handle_connect_error in both the !is_open on_error branch and the start_tls-failure branch) is in place and I re-traced it as correct — the wrapper's JSRef is still strong at both call sites, so the thunk cells are live when neutered, and the later Drop-path teardown() sees zeroed shadows.

Level of scrutiny

High. This is a GC-rooting refactor in TLS socket code, precisely the category REVIEW.md's "Root or copy every JSValue held beyond the current call" section is written for. There is no correctness bug being fixed (the PR says so), so the bar is that the new rooting model is at least as safe as the old one on every path. I traced the four teardown() entry points and the origin reads in call_write_or_end/on_received_data against the wrapper's JSRef state machine and found no window where a shadow is read after the wrapper can have been swept — but this is subtle enough (the first review pass found a real hole) that a human should confirm the reasoning, particularly the on_close → deinit_in_next_tick → EventState::Close sequence and whether any other caller of deinit_in_next_tick could exist.

Other factors

Both prior inline findings are resolved (the UAF fix in the second commit, the void stderr; fix in the third). The new test asserts protectedObjectTypeCounts deltas rather than absolute counts, uses the sibling tests' subprocess pattern, and would fail on main (fn≈4N, obj≈N). rust:check-all reported clean and the existing duplex/renegotiation/UAF regression tests are cited as passing. Given the GC-lifetime subtlety and that this is not a bug fix, I'm deferring rather than approving.