Bun.listen: defer the Listener finalizer's socket-group close to the event loop

[robobun]

Reproduction

const socket = { data() {}, open() {}, error() {}, close() {}, handshake() {} };
const conns = [];
async function makeDroppedTlsListenerWithLiveConn() {
  const srv = Bun.listen({ hostname: '127.0.0.1', port: 0, tls: { key, cert }, socket });
  const port = srv.port;
  srv.unref();
  conns.push(await Bun.connect({ hostname: '127.0.0.1', port, tls: { rejectUnauthorized: false }, socket }));
  void srv.port;
}
for (let i = 0; i < 3; i++) await makeDroppedTlsListenerWithLiveConn();
await Bun.sleep(100);
for (let i = 0; i < 8; i++) { Bun.gc(true); await Bun.sleep(20); }
console.log('survived');

Stock bun (release): SIGABRT, exit 134, 3/3. The plain-TCP shape (no tls: on either side) crashes the same way.
release-asan: ASSERTION FAILED: vm().currentThreadIsHoldingAPILock() => vm().heap.mutatorState() != MutatorState::Sweeping (JSCell.cpp:179).

Backtrace (plain-TCP leg): Heap::sweepInFinalize -> JSDestructibleObjectDestroyFunc -> Listener::finalize -> Listener::deinit -> us_socket_group_close_all -> NewSocket::on_close -> Bun__SocketHandlers__getField -> JSCell::classInfo -> validateIsNotSweeping.
TLS leg: same chain via us_internal_ssl_close -> ssl_trigger_handshake_econnreset -> us_dispatch_handshake -> NewSocket::on_handshake -> Bun__SocketHandlers__getField(index=8). Which leg trips first is ordering-dependent.

Cause

When a dropped, unref()'d Bun.listen() server is collected while it still has live accepted connections, Listener::finalize calls deinit, which force-closes the embedded socket group. us_socket_group_close_all synchronously dispatches each accepted socket's on_close, and for TLS sockets us_internal_ssl_close additionally fires a synthetic on_handshake(0, ECONNRESET) for any accepted socket whose server-side handshake has not yet completed. Both dispatchers read their JS handler from the JSSocketHandlers cell; the classInfo read trips JSC's validateIsNotSweeping assertion.

Fix

The accepted sockets are not themselves being finalized (only the Listener wrapper is), so their handlers should still fire. Defer deinit (and with it close_all()) to the event loop via a ManagedTask when there are live connections; the dispatch then runs outside the sweep and the user's close / handshake callbacks fire normally. The swept JSListener wrapper's JsRef and the Handlers back-pointer are retired inline (plain Rust state, no JSC heap access) so nothing between the sweep and the deferred task can observe the dead wrapper via socket.listener. The accepted sockets' own Strong-held wrappers keep the handlers cell alive across the deferral.

When the VM is shutting down the event loop won't tick again; in that case the Listener is leaked and its sockets are reclaimed by process exit or by the worker's close_all_socket_groups.

Verification

Two tests in test/js/bun/net/socket.test.ts, one plain-TCP and one TLS, each spawn a child that drops three such listeners and drives GC until both sides observe the close.

USE_SYSTEM_BUN=1: both children SIGABRT (exitCode: 134, empty stdout), both tests fail.
bun bd: both print serverClose=3 clientClose=3, exit 0, both pass (10/10 stable).

---

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

[coderabbitai]

Walkthrough

Changes

Listener GC teardown

Layer / File(s)	Summary
Deferred listener finalization src/runtime/socket/Listener.rs	Listener::finalize defers teardown for active connections, skips it during VM shutdown after clearing the listener reference, and preserves immediate teardown when no connections remain.
TCP and TLS GC regression coverage test/js/bun/net/socket.test.ts	Subprocess tests force GC for unref’d TCP and TLS listeners and verify all server and client close callbacks complete successfully.

Suggested reviewers: jarred-sumner

🚥 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 is clear, concise, and accurately summarizes the main change: deferring Listener finalization to the event loop.
Description check	✅ Passed	The description includes the reproduction, cause, fix, and verification details, which cover the required information despite different section headings.

---

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

[claude]

Beyond the inline test nits, I checked: enqueue_task from inside the sweep only writes to the task fifo (event_loop.rs:682) — no JS-heap access; the JSSocketHandlers cell survives the Listener wrapper's collection because each accepted socket's Strong-held wrapper independently caches it (socket_body.rs:1563/1579); and mark_inactive reaching back through handlers.listener between enqueue and the deferred deinit sees a still-valid heap allocation with ListenerType::None already set.

Extended reasoning...

This is a GC-finalizer / raw-pointer-ownership change on a memory-safety-critical path, so I'm not auto-approving. The mechanism looks sound and matches the ManagedTask deferral pattern already used in server/mod.rs:1799. The deliberate leak-on-is_shutting_down and the two test-quality nits are worth a human sign-off.

[robobun]

^{Updated 2:20 AM PT - Jul 29th, 2026}

❌ @robobun, your commit ac620b4 has 1 failures in Build #84753 (All Failures):

• step failed outside runner - exit 1 on 🐧 aarch64-musl - build-bun
• step failed outside runner - exit 1 on 🐧 x64-asan - build-bun
• step failed outside runner - exit 1 on 🐧 aarch64-android - build-bun
• step failed outside runner - exit 1 on 🍎 x64 - build-bun
• step failed outside runner - exit 1 on 🐧 x64-musl - build-bun
• step failed outside runner - exit 1 on 🍎 aarch64 - build-bun
• step failed outside runner - exit 1 on 🪟 aarch64 - build-bun
• step failed outside runner - exit 1 on 🐧 aarch64 - build-bun
• step failed outside runner - exit 1 on :freebsd: x64 - build-bun
• step failed outside runner - exit 1 on 🍎 26 aarch64 - test-bun

---

🧪   To try this PR locally:

bunx bun-pr 36181

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

bun-36181 --bun

[claude]

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

• 🔴 src/runtime/socket/socket_body.rs:2016-2022 — on_handshake (socket_body.rs:1634) has no FINALIZING || handlers.gc_closing guard, but for a TLS listener group.close_all() synchronously dispatches it: us_socket_close → us_internal_ssl_close (openssl.c:1848) sees ssl_handshake_state != HANDSHAKE_COMPLETED → ssl_trigger_handshake_econnreset → us_dispatch_handshake → on_handshake reads handlers.on_handshake() from the JSSocketHandlers cell mid-sweep — the exact validateIsNotSweeping SIGABRT this PR fixes for on_close/on_data/on_timeout. So an unref'd Bun.listen({ tls }) collected while an accepted socket is mid-handshake still aborts; on_handshake needs the same early return before the cell read (REVIEW.md: "Fix the whole class … SSL/non-SSL variants"), and the new test only exercises plain TCP.

  Extended reasoning...

  What the bug is

  The PR adds handlers.gc_closing guards to on_close, on_data, and on_timeout so accepted sockets bail before reading the JSSocketHandlers cell when Listener::deinit force-closes the socket group from inside the GC sweep. But on_handshake (socket_body.rs:1634-1762) has no such guard — no Flags::FINALIZING check and no handlers.gc_closing.get() check anywhere before line 1752's handlers.on_handshake(). For a TLS listener, us_socket_group_close_all synchronously dispatches on_handshake before on_close for any accepted socket whose handshake hasn't completed, so the TLS variant of the exact assertion this PR is fixing is still reachable.

  Code path that triggers it

  Traced end to end:

  1. Listener::deinit (Listener.rs:904, GC-finalizer-only caller) sets gc_closing = true, then calls this_ref.group.with_mut(|g| g.close_all()) when active_connections > 0.
  2. us_socket_group_close_all_ex (context.c:113) iterates head_sockets and, for each non-SEMI socket, calls us_socket_close(s, LIBUS_SOCKET_CLOSE_CODE_CLEAN_SHUTDOWN, 0).
  3. us_socket_close (socket.c:353): if (s->ssl && !us_socket_is_closed(s)) return us_internal_ssl_close(s, code, reason); — an accepted TLS socket has s->ssl set, so it routes to the SSL close path.
  4. us_internal_ssl_close (openssl.c:1808): after ssl_update_handshake(s) at :1845, line 1848 checks if (s->ssl_handshake_state != HANDSHAKE_COMPLETED) → line 1851 ssl_trigger_handshake_econnreset(s).
  5. ssl_trigger_handshake_econnreset (openssl.c:1701-1713) calls us_dispatch_handshake(s, 0, {ECONNRESET, ...}).
  6. us_dispatch_handshake routes via the BunSocketTls vtable (the accepted socket's kind was set in Listener::on_create) to NewSocket<true>::on_handshake.
  7. on_handshake at socket_body.rs:1634: has_handlers() at :1645 is true (the accepted socket holds an Rc<Handlers>); this.socket.set(s) at :1649 re-arms the handle so is_detached() at :1650 is false; then line 1752 handlers.on_handshake() → self.cell.on_handshake() → Bun__SocketHandlers__getField → JSCell::classInfo → validateIsNotSweeping — the exact assertion in the PR's backtrace.

  Note that ssl_update_handshake at openssl.c:1845 can also independently reach us_dispatch_handshake on the same call (openssl.c:1698), so there are two entry points to on_handshake inside the same synchronous close_all() dispatch.

  Why the existing guards don't cover it

  • The per-socket Flags::FINALIZING is not set — it's the Listener being swept, not the accepted NewSocket. That's exactly why the PR added gc_closing to the shared Handlers for on_close/on_data/on_timeout.
  • has_handlers() (line 1645) only checks that the Rc<Handlers> exists — it does, the accepted socket holds it.
  • is_detached() (line 1650) is false because line 1649 just set this.socket to the live us_socket_t passed in.
  • on_close is now guarded, but for a mid-handshake TLS socket on_handshake fires first in the same synchronous us_socket_close call, so the guarded on_close never gets a chance to save it.

  Step-by-step proof (concrete trigger)

  The direct TLS sibling of the PR's own repro:

  1. const srv = Bun.listen({ hostname: '127.0.0.1', port: 0, tls: { key, cert }, socket: {...} }); srv.unref(); — unref() before any accept downgrades this_value to weak so GC can collect the Listener wrapper.
  2. A TLS client connects but stalls mid-handshake (e.g. a raw TCP Bun.connect to the TLS port that never sends a ClientHello, or a TLS client that connects and then does nothing). Server-side accept fires on_open → active_connections becomes 1; ssl_handshake_state stays HANDSHAKE_PENDING.
  3. Bun.gc(true) collects the Listener wrapper → Listener::finalize → deinit (mid-sweep). active_connections > 0 → group.close_all().
  4. Steps 2-7 of the code path above execute synchronously inside the sweep; line 1752 reads the cell → validateIsNotSweeping SIGABRT (exit 134 in release, ASSERTION FAILED in release-asan).

  Impact

  Same as the bug the PR fixes — a release-build SIGABRT — but for Bun.listen({ tls }) instead of plain TCP. The mid-handshake window is narrower than TCP-open, but it's a real steady-state condition (any client that connects and never completes the handshake leaves the accepted socket in this state indefinitely). REVIEW.md's "Fix the whole class in the same PR … SSL/non-SSL variants" names this exact sibling-site pattern as required scope, and the new test in socket.test.ts only exercises plain TCP so it does not cover this variant.

  Fix

  Add the same guard at the top of on_handshake, before ref_guard() and before any handlers.* cell read:

  pub fn on_handshake(...) -> JsResult<()> {
      jsc::mark_binding!();
      if !this.has_handlers() {
          return Ok(());
      }
      let handlers = this.get_handlers();
      if this.flags.get().contains(Flags::FINALIZING) || handlers.gc_closing.get() {
          return Ok(());
      }
      this.update_flags(|f| f.insert(Flags::HANDSHAKE_COMPLETE));
      ...

  (hoist get_handlers() above the flag update so the guard runs before any state mutation, matching on_close's ordering). Ideally also add a TLS variant of the new test — same shape with tls: { key, cert } on the listener and a raw-TCP client that connects without handshaking — to cover it.

• 🔴 src/runtime/socket/Listener.rs:903-905 — Setting gc_closing = true unconditionally on the shared Rc<Handlers> in Listener::deinit regresses the Windows named-pipe path: accepted named-pipe connections are not in the embedded SocketGroup (that branch of listen() never calls group.init(); accepts go through WindowsNamedPipeContext with their own uv_pipe), so close_all() is a no-op and those connections stay open with gc_closing permanently true. The new guards at socket_body.rs:2106 / :2019 / :978 then silently drop every subsequent data, close, and timeout event on connections that are still receiving bytes — before this PR they were unaffected by the listener being GC'd. Consider scoping gc_closing to bracket only the synchronous close_all() dispatch (set → close_all() → clear), or gating the .set(true) on the non-named-pipe path — REVIEW.md's "fix the whole class in the same PR / POSIX/Windows branches" applies here.

  Extended reasoning...

  What the bug is

  Listener::deinit (Listener.rs:904) now sets this_ref.handlers.gc_closing.set(true) unconditionally on the shared Rc<Handlers>, and the flag is never cleared. For a uws/TCP listener this is fine: group.close_all() on the next line synchronously closes every accepted socket, so no further events can arrive on that Handlers after deinit returns.

  But on the Windows named-pipe listener path (ListenerType::NamedPipe), accepted connections are not members of the embedded SocketGroup:

  • The named-pipe branch of Listener::listen() constructs group: JsCell::new(uws::SocketGroup::default()) and returns before ever reaching the group.init() / register_root_region calls (Listener.rs:212-290).
  • Accepts go through WindowsNamedPipeListeningContext::on_client_connect → Listener::on_name_pipe_created → WindowsNamedPipeContext::create, which drives its own uv_pipe handle and never links the connection into listener.group.
  • Each accepted named-pipe socket clones Rc::clone(&listener.handlers) (Listener.rs:527) — the same Handlers instance whose gc_closing cell deinit just flipped.

  So in deinit, group.close_all() iterates a default/empty group (a no-op), and finalize's close_pipe_and_deinit closes only the listening uv_pipe, not the accepted connections. The accepted named-pipe sockets remain open with their shared Handlers permanently marked gc_closing == true.

  The code path that triggers it

  WindowsNamedPipeContext::on_data (WindowsNamedPipeContext.rs:184-191), on_timeout (:254-260), and on_close (:263-278) all forward via match_socket! to NewSocket::on_data / on_timeout / on_close — the exact functions this PR added || handlers.gc_closing.get() guards to (socket_body.rs:2106, :978, :2019). Every subsequent data payload, timeout, and peer-disconnect event on those still-live connections is silently dropped before the callback read.

  Why existing code doesn't prevent it

  Before this PR, named-pipe connections were unaffected by the listener being GC'd. The crash the PR fixes is uws-group-specific: us_socket_group_close_all synchronously dispatches on_close from inside the sweep, and that mid-sweep cell read is what trips validateIsNotSweeping. Named-pipe events fire from the libuv event loop on a normal tick, not from inside the sweep, so the assertion never applied there — and each accepted socket's own JS wrapper roots the JSSocketHandlers cell via get_this_value → handlers_set_cached (socket_body.rs:1566), so the cell read was safe. The new guard suppresses a path that was already sound.

  Step-by-step proof

  1. On Windows: const srv = Bun.listen({ unix: '\\\\.\\pipe\\foo', socket: { data(s, d) { got += d.length }, ... } }); srv.unref(); — unref() with active_connections == 0 downgrades this_value to weak (Listener.rs:1000). Drop the JS reference to srv.
  2. A client connects: on_name_pipe_created clones Rc::clone(&listener.handlers) into the accepted NewSocket and increments active_connections. Nothing re-upgrades the listener's this_value.
  3. GC runs: the Listener wrapper is collected → finalize → ListenerType::NamedPipe arm calls close_pipe_and_deinit (closes only the listening handle) → Self::deinit(this).
  4. deinit sets gc_closing = true on the shared Handlers; active_connections > 0 so it calls group.close_all(), but the group was never init()'d and holds no sockets — no-op. The Listener frees itself.
  5. The client writes bytes. libuv delivers them → WindowsNamedPipeContext::on_data → NewSocket::on_data → handlers.gc_closing.get() is true → early return. The server data callback never fires; bytes are silently discarded on an open, receiving socket.
  6. When the client eventually disconnects, on_close hits the same guard at socket_body.rs:2019 → the server close callback never fires either.

  This is the same unref()'d-listener-with-live-connections timeline the PR's own TCP test exercises, just with unix: '\\\\.\\pipe\\...' on Windows.

  Impact

  Silent data loss on live connections — a Windows-only behavior regression on the sibling transport of the exact scenario this PR fixes for TCP. The PR's rationale ("firing the user's close handler when GC happens to collect it is not useful") justifies suppressing on_close during the synchronous sweep dispatch, but silently dropping on_data on a connection that is still open and receiving bytes is a different, user-visible failure. REVIEW.md: "Fix the whole class in the same PR … POSIX/Windows branches, SSL/non-SSL variants."

  How to fix

  Scope gc_closing to the synchronous close_all() dispatch only — that is the one path that runs from inside the sweep:

  this_ref.handlers.set_listener(None);
  if this_ref.handlers.active_connections.get() > 0 {
      this_ref.handlers.gc_closing.set(true);
      this_ref.group.with_mut(|g| g.close_all());
      this_ref.handlers.gc_closing.set(false);
  }

  This preserves the fix for the uws path (the guarded on_close calls happen synchronously inside close_all()) while leaving named-pipe connections — whose events arrive later from the libuv loop, outside the sweep — able to read the cell as before. Alternatively, gate the set(true) on !matches!(listener, ListenerType::NamedPipe(_)), but bracketing the flag around the synchronous dispatch is the more precise expression of the invariant.

[robobun]

Addressed both review findings:

• on_handshake guard: already added in fb8895c (also on_end, for the TLS close-notify path). The guard sits after HANDSHAKE_COMPLETE/socket.set(s) but before the first cell read at handlers.on_handshake(); those two writes are plain Rust state, not JSC heap accesses.
• Named-pipe regression: 8d8d5a7 brackets gc_closing around the synchronous close_all() dispatch only, so Windows named-pipe connections (which are not in the embedded group and keep dispatching from the libuv loop) are unaffected.

The TLS mid-handshake shape is tracked as #6530 per the thread; the on_handshake guard here covers it for the listener-finalizer path.

[Jarred-Sumner]

Not correct. The listener finalizing does not mean the sockets within are finalizing.

[robobun]

7d7e35a reverts to the event-loop deferral: the accepted sockets are not being finalized, so their close/handshake handlers now fire normally from the deferred task instead of being suppressed. The gc_closing flag and the dispatch-handler guards are gone; src/ diff is back to just Listener::finalize. Both TCP and TLS tests assert serverClose=3.

[claude]

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

• 🔴 src/runtime/socket/Listener.rs:900-902 — Setting gc_closing unconditionally in Listener::deinit regresses the Windows named-pipe path: accepted named-pipe sockets are InternalSocket::Pipe (never linked into the us_socket_group, which is left at SocketGroup::default()), so close_all() is a no-op and the connections stay open with gc_closing stuck true on their shared Handlers — from then on on_data/on_end/on_handshake/on_timeout/on_close all bail silently and incoming bytes are dropped on the floor. Pre-PR this scenario worked (nothing on the named-pipe finalizer path dispatches synchronously mid-sweep, so the assertion being fixed was never reachable there); either gate the flag on ListenerType::Uws / a non-empty group, or also close the accepted named-pipe connections so gc_closing is only ever observed by sockets actually being torn down.

  Extended reasoning...

  What the bug is

  Listener::deinit (Listener.rs:900) now does this_ref.handlers.gc_closing.set(true) unconditionally, regardless of ListenerType. On the TCP/unix-socket path this is correct: every accepted socket is a us_socket_t linked into the embedded us_socket_group, so the immediately-following group.close_all() synchronously force-closes each one, and gc_closing is only observed by on_close during that sweep-driven teardown.

  On the Windows named-pipe path the picture is different. The named-pipe branch of Listener::listen (Listener.rs:~208-320) constructs the listener with group: JsCell::new(uws::SocketGroup::default()) and returns before ever calling group.init() — the group stays all-zeroed (SocketGroup.rs:78-81, head_sockets == null). Each accepted connection is created via on_name_pipe_created → WindowsNamedPipeContext::create, and its NewSocket wrapper stores an InternalSocket::Pipe, not a us_socket_t linked into the group. And close_pipe_and_deinit (called from finalize for ListenerType::NamedPipe) only schedules an async uv_close of the listening uv_pipe; the accepted client pipe handles are independent libuv handles and stay open.

  The specific code path

  1. Bun.listen({ unix: '\\\\.\\pipe\\foo', socket: {...} }); srv.unref() before the first accept. unref() (Listener.rs:993-996) sees active_connections == 0 and downgrades this_value to weak.
  2. A client connects. on_client_connect → on_name_pipe_created clones Rc::clone(&listener.handlers) into the accepted NewSocket (Listener.rs:557), and NewSocket::on_open → handlers.mark_active() bumps active_connections to 1 — but nothing re-upgrades the Listener's this_value, so it remains weakly held.
  3. The user drops the JS reference to srv. GC sweeps the Listener wrapper.
  4. Listener::finalize runs: listener.replace(None) → close_pipe_and_deinit(named_pipe) (async close of the listening handle only) → Self::deinit(Box::into_raw(self)).
  5. deinit: set_listener(None); gc_closing.set(true); active_connections == 1 > 0 so group.close_all() — but on the never-init()'d default group this walks a null head_sockets and does nothing. The Listener is freed.
  6. The accepted NewSocket still holds its Rc<Handlers> clone, now with gc_closing == true permanently set. Its libuv pipe handle is still open and reading.

  From this point on, every dispatch from WindowsNamedPipeContext (WindowsNamedPipeContext.rs:187/212/223/257/272) into the shared NewSocket::on_* hits the new guard:

  • on_data (socket_body.rs:2115): handlers.gc_closing.get() → return — incoming bytes silently dropped.
  • on_end (socket_body.rs:1611): bails after unref + mark_inactive, never calls the user's end handler.
  • on_handshake / on_timeout: return silently.
  • on_close (socket_body.rs:2028): bails through CloseTeardown without ever invoking the user's close handler.

  The connection is neither closed nor reported closed — it just goes deaf.

  Why existing code doesn't prevent it

  The PR's rationale — "a dropped, never-stop()'d listener is a leak scenario; firing the user's close handler when GC happens to collect it is not useful behaviour to preserve" — is about closing connections. It does not cover a connection that is not being closed and continues receiving data. The Flags::FINALIZING half of the guard is per-socket and correctly scoped; gc_closing is per-Handlers and now leaks onto sockets the finalizer never touches.

  Nothing on the named-pipe finalizer path dispatches synchronously into JS mid-sweep: close_pipe_and_deinit schedules an async uv_close and close_all() is a no-op. So the validateIsNotSweeping assertion this PR fixes was never reachable on the named-pipe path — meaning the flag guards nothing there and only breaks previously-working behavior.

  Step-by-step proof it worked pre-PR

  Before this PR, in the same scenario, deinit did set_listener(None) and called close_all() on the empty group (no-op) — gc_closing did not exist. The accepted named-pipe socket kept dispatching normally: its own Rc<Handlers> keeps the Handlers alive, and its JS wrapper's visited handlers slot (set via get_this_value → handlers_set_cached, socket_body.rs:1566, called from on_open) keeps the JSSocketHandlers cell alive independently of the freed Listener wrapper. on_data read handlers.on_data() from that cell and called the user's handler; on_close eventually fired the user's close. All of that ran on the event loop, not inside the sweep.

  Impact

  Windows-only silent-data-loss regression on the sibling transport of the path this PR fixes. The trigger (unref-before-first-accept + drop + GC) is exactly the scenario the PR's own new test exercises for TCP. Per REVIEW.md's "fix the whole class in the same PR … POSIX/Windows branches, SSL/non-SSL variants" this sibling must be handled.

  How to fix

  Two options:

  1. Gate the flag — only set gc_closing when the group actually has sockets that close_all() will synchronously dispatch (e.g. only on the ListenerType::Uws path, or only when group was init()'d / head_sockets is non-null). There is nothing to guard on the named-pipe path — no synchronous dispatch happens from the finalizer there.
  2. Also close the accepted named-pipe connections in deinit so gc_closing is only ever observed by sockets actually being torn down (matching the TCP semantics the flag was designed for).

[robobun]

The named-pipe gc_closing finding above is against a superseded commit; 7d7e35a dropped the flag entirely. The current src/ diff is only the ManagedTask deferral in Listener::finalize, so named-pipe connections are untouched: when the deferred deinit runs, close_all() on the empty group is a no-op and those connections keep dispatching normally from libuv (their own wrappers root the handlers cell).

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@src/runtime/socket/socket_body.rs`:
- Around line 1665-1667: Remove the handlers.gc_closing early returns in
src/runtime/socket/socket_body.rs lines 1665-1667 and 2020-2022, retaining only
socket finalization protection so on_handshake and on_close still execute after
deferred listener teardown. Update test/js/bun/net/socket.test.ts lines
3377-3384 to assert normal server-side close delivery, and lines 3429-3437 to
verify server acceptance/handshake before asserting the deferred TLS
close/handshake callbacks; ensure both tests validate their setup preconditions.

🪄 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: e4e4a720-aca9-4c03-858f-cce4528d6ff9

📥 Commits

Reviewing files that changed from the base of the PR and between 8d8d5a7 and c271b8c.

📒 Files selected for processing (4)

• src/runtime/socket/Handlers.rs
• src/runtime/socket/Listener.rs
• src/runtime/socket/socket_body.rs
• test/js/bun/net/socket.test.ts

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@src/runtime/socket/Listener.rs`:
- Line 870: In the deinit ownership round-trip, replace the standard
Box::into_raw conversion with bun_core::heap::into_raw so it matches the
existing bun_core::heap::take reclamation; preserve the current raw-pointer flow
and use the established heap ownership helpers.

In `@test/js/bun/net/socket.test.ts`:
- Line 3366: Replace the TCP polling loop at test/js/bun/net/socket.test.ts:3366
and the corresponding TLS loop at test/js/bun/net/socket.test.ts:3415 with
event-driven synchronization: await each connection’s open event, trigger GC,
and await promises resolved when the expected server and client close counts are
reached. Remove timing-based await sleep(N) waits while preserving the existing
close-count expectations.

🪄 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: bb1920cf-7c34-4948-86c5-122a99f0ad3a

📥 Commits

Reviewing files that changed from the base of the PR and between c271b8c and cf694b2.

📒 Files selected for processing (2)

• src/runtime/socket/Listener.rs
• test/js/bun/net/socket.test.ts

[robobun]

Rebased onto 0316b6f and squashed to one commit (23aee8d). Current state per the review: deinit is deferred to the event loop via ManagedTask when there are live connections, so the accepted sockets' close/handshake handlers fire normally on the next tick instead of being suppressed. No per-dispatcher guards; socket_body.rs is unchanged from main.

Both the plain-TCP and TLS tests assert serverClose=3 clientClose=3; both SIGABRT on stock and pass on the debug build (10/10).

[claude]

No new findings after the rebase; all earlier review threads are addressed. Deferring to a human because this touches GC-finalizer / raw-pointer lifetime in Listener::finalize with a deferred heap::take and an intentional leak on the shutdown path — worth a maintainer's eyes even though the mechanism looks correct.

What was reviewed: the hoisted this_value.finalize() / set_listener(None) close the stale-socket.listener window before the ManagedTask runs; the updated Handlers::listener() SAFETY block names that as the invariant; the is_shutting_down() leak branch's fds are reclaimed by close_all_socket_groups on worker teardown; both tests gate on server open and poll (serverClose < 3 || clientClose < 3) so neither can pass vacuously.

Extended reasoning...

Overview

The PR fixes a SIGABRT (validateIsNotSweeping) when a dropped, unref()'d Bun.listen() server is GC'd while it still has live accepted connections. Listener::finalize previously ran deinit inline from inside the JSC sweep; deinit's close_all() synchronously dispatches each accepted socket's on_close (and for TLS, a synthetic on_handshake), which reads the JSSocketHandlers cell — a cell read mid-sweep trips JSC's assertion. The fix: inside finalize, hoist this_value.finalize() and handlers.set_listener(None) (plain Rust writes, no JSC heap access), then when active_connections > 0 enqueue deinit as a ManagedTask so close_all() runs from the event loop instead of the sweep. On vm.is_shutting_down() the box is intentionally leaked (event loop won't tick; fds are reclaimed by process exit or the worker's close_all_socket_groups). Files: src/runtime/socket/Listener.rs (finalize logic), src/runtime/socket/Handlers.rs (SAFETY-comment update only), two new subprocess tests in test/js/bun/net/socket.test.ts (TCP + TLS).

Security risks

None identified. This is an internal lifecycle/GC-safety fix; no user-controlled input reaches the changed code, no auth/crypto/permissions surface.

Level of scrutiny

High. This is native memory-safety code in a GC finalizer: raw Box::into_raw ownership handoff to a deferred task, an intentional leak branch, and a raw-pointer SAFETY invariant (Handlers::listener()) whose justification changed. Prior review rounds already caught and fixed a real stale-JsRef::Weak window (the un-hoisted this_value.finalize()), a stale SAFETY comment, and test-flake shapes (missing accept-gate, poll condition not covering clientClose). The remaining acknowledged trade-offs — a small heap-box leak on worker termination and the ManagedTask having no cleanup fn on the enqueue-then-shutdown TOCTOU — match the existing server/mod.rs schedule_deinit precedent and are not regressions (pre-PR this path SIGABRTed).

Other factors

The bug hunter found nothing new on the squashed commit. All eight of my earlier inline findings are resolved (one nit — the TCP fixture's srv-reachability shape — was reasonably declined as zero practical risk). The PR description was re-synced to the shipped approach. The two tests spawn subprocesses, drain pipes concurrently, gate on the actual observable condition (server open count, then bounded poll on both close counters), and assert a strict {stdout, stderr, exitCode} object; the author reports 10/10 stable and SIGABRT on USE_SYSTEM_BUN=1. Given the domain (GC finalizer + deferred native teardown + intentional leak), a maintainer sign-off is still appropriate rather than a bot approval.

[claude]

No new findings after the rebase/squash; all earlier threads are resolved. This is a GC-finalizer / raw-pointer-ownership change (deferring deinit to a ManagedTask from inside the sweep, with an intentional leak on the is_shutting_down() path), so it warrants a human sign-off on the memory-safety reasoning.

Checked this pass: the hoisted this_value.finalize() / set_listener(None) close the socket.listener stale-wrapper window; enqueue_task from a finalizer matches the server/mod.rs schedule_deinit precedent; the updated Handlers::listener() SAFETY block names the new invariant; both TCP and TLS tests gate on server open and poll (serverClose < 3 || clientClose < 3).

Extended reasoning...

Overview

The PR fixes a SIGABRT when a dropped, unref()'d Bun.listen() server is GC'd while it still has live accepted connections. Pre-PR, Listener::finalize called deinit inline from the sweep; deinit → close_all() synchronously dispatched each accepted socket's on_close (and for TLS, a synthetic on_handshake), which read the JSSocketHandlers cell and tripped JSC's validateIsNotSweeping assertion. The fix: inside finalize, hoist this_value.finalize() + handlers.set_listener(None) (plain Rust writes, no JSC heap access), then when active_connections > 0 enqueue deinit as a ManagedTask so close_all() runs from the event loop. On vm.is_shutting_down() the Listener box is leaked (fds reclaimed by process exit or the worker's close_all_socket_groups). Handlers::listener()'s SAFETY block is reworded to cite the new invariant. Two subprocess tests (plain-TCP and TLS) exercise the crash and assert the deferred handlers fire (serverClose=3 clientClose=3).

Security risks

None identified. No user-controlled input parsing, no auth/crypto/permissions surface. The change is confined to finalizer scheduling and back-pointer clearing.

Level of scrutiny

High. This is native memory-safety code at the JSC-GC boundary: a finalizer running inside the sweep hands a raw *mut Listener to the event loop, changes the ownership contract documented in an unsafe SAFETY block, and deliberately leaks on one branch. The reasoning is sound and matches the existing server/mod.rs::schedule_deinit pattern, but this is exactly the class of change ("never let a pointer or slice outlive the memory it points into", "reference counts provably balanced on every terminal path") that REVIEW.md calls out for careful human review.

Other factors

I left four rounds of inline feedback earlier (stale-JsRef window, TLS accept-gate, poll-condition, SAFETY-comment sync, PR-description sync); all are addressed and resolved. One consistency nit (mirroring the TCP fixture's srv reachability guard) was declined with a reasonable justification (no auto-GC fires in the connect window of a fresh bun -e process). The remaining acknowledged follow-up — the worker-termination heap leak and the ManagedTask::new (no cleanup fn) TOCTOU on enqueue-then-shutdown — is not a regression (pre-PR that path SIGABRTed) and matches existing precedent, but a maintainer should confirm they're comfortable landing it as-is with the follow-up deferred. The bug-hunting system found nothing this run.

[robobun]

CI is blocked on build-cpp agent availability, not this diff: in builds 84660 and 84753 the Rust compile succeeded on every lane that ran, but 7+ build-cpp jobs expired or stayed scheduled under the current queue backlog, so the downstream build-bun jobs fail with "Sibling step ...-build-cpp errored, nothing to link" and ~150 test jobs never run. The test lanes that did run show only parallel-batch flakes (08965, spawn-streaming-stdout, svelte, bun-lock, bun-install-streaming-extract), all of which passed alone; socket.test.ts is green. Locally both new tests pass on bun bd and SIGABRT on USE_SYSTEM_BUN=1. Ready for review; CI will need the backlog to clear.