fix: clean up raw TCP socket on TLS upgrade close

Merged
AL
9mo ago
+72-0

Summary

Fixes #12117, #24118, #25948

When a TCP socket is upgraded to TLS via tls.connect({ socket }), upgradeTLS() creates two TLSSocket structs — a TLS wrapper and a raw TCP wrapper. Both are markActive()'d and ref()'d. On close, uws fires onClose through the TLS context only, so the TLS wrapper is properly cleaned up, but the raw TCP wrapper's onClose never fires. Its has_pending_activity stays true forever and its ref_count is never decremented, leaking one raw TLSSocket per upgrade cycle.

This affects any code using the tls.connect({ socket }) "starttls" pattern:

  • MongoDB Node.js driver — SDAM heartbeat connections cycle TLS upgrades every ~10s, causing unbounded memory growth in production
  • mysql2 TLS upgrade path
  • Any custom starttls implementation

The fix

Adds a defer block in NewWrappedHandler(true).onClose that cleans up the raw TCP socket when the TLS socket closes:

defer {
    if (!this.tcp.socket.isDetached()) {
        this.tcp.socket.detach();
        this.tcp.has_pending_activity.store(false, .release);
        this.tcp.deref();
    }
}
  • isDetached() guard — skips cleanup if the raw socket was already closed through another code path (e.g., JS-side handle.close())
  • socket.detach() — marks InternalSocket as .detached so isClosed() returns true safely (the underlying us_socket_t is freed when uws closes the TLS context)
  • has_pending_activity.store(false) — allows JSC GC to collect the raw socket's JS wrapper
  • deref() — balances the ref() from upgradeTLS; the remaining ref is the implicit one from JSC (ref_count.init() == 1). When GC later calls finalize() → deref(), ref_count hits 0 and deinit() runs the full cleanup chain (markInactive, handlers, poll_ref, socket_context)

markInactive() is intentionally not called in the defer — it must run inside deinit() to avoid double-freeing the handlers struct.

Why Node.js doesn't have this bug

Node.js implements TLS upgrades purely in JavaScript/C++ with OpenSSL, where the TLS wrapper takes ownership of the underlying socket. There is no separate "raw socket wrapper" that needs independent cleanup.

Test Results

Regression test

$ bun test test/js/node/tls/node-tls-upgrade-leak.test.ts
 1 pass, 0 fail

Creates 20 TCP→TLS upgrade cycles, closes all connections, runs GC, asserts TLSSocket count stays below 10.

Existing TLS test suite (all passing)

node-tls-upgrade.test.ts      1 pass
node-tls-connect.test.ts     24 pass, 1 skip
node-tls-server.test.ts      21 pass
node-tls-cert.test.ts        25 pass, 3 todo
renegotiation.test.ts          6 pass

MongoDB TLS scenario (patched Bun, 4 minutes)

Baseline: RSS=282.4 MB | Heap Used=26.4 MB
Check #4:  RSS=166.7 MB | Heap Used=24.2 MB — No TLSSocket growth. RSS DECREASED.

Test plan

  • New regression test passes (node-tls-upgrade-leak.test.ts)
  • All existing TLS tests pass (upgrade, connect, server, cert, renegotiation)
  • MongoDB TLS scenario shows zero TLSSocket accumulation
  • Node.js control confirms leak is Bun-specific
  • CI passes

🤖 Generated with Claude Code