mirror of
https://github.com/oven-sh/bun
synced 2026-02-19 07:12:24 +00:00
7848648e09
## 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: ```zig 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 - [x] New regression test passes (`node-tls-upgrade-leak.test.ts`) - [x] All existing TLS tests pass (upgrade, connect, server, cert, renegotiation) - [x] MongoDB TLS scenario shows zero `TLSSocket` accumulation - [x] Node.js control confirms leak is Bun-specific - [ ] CI passes 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>