Repository navigation
Bun.listen: don't let GC collect an unref'd listener that is still listening - #36930
Conversation
… listening Listener::unref downgraded this_value to weak while the listen socket was still open. With no other references, GC collected the wrapper and the JSSocketHandlers cell it roots, while the socket kept accepting: the next accepted connection dispatched on_open into the swept cell (SIGABRT or type confusion), and once the finalizer had closed the socket the port refused connections that Node semantics say it must still serve. unref() now only releases the event-loop ref, matching the this_value contract (strong while listening or with connections). The idle downgrade stays where it already lives: do_stop and Handlers::mark_inactive, both gated on the listen socket being closed.
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found, but this touches JSC GC-root lifetime for a live network listener (the exact class REVIEW.md calls the most-blocked), so I'd like a human to confirm.
What was reviewed:
- Verified
do_stop(Listener.rs:838) andHandlers::mark_inactive(Handlers.rs:270-272) both gate thethis_valuedowngrade onListenerType::None, so the removedunref()downgrade is fully covered once the socket actually closes. - Traced
stop()-then-unref()andunref()-then-stop()orderings with and without live connections — every path still reaches a downgrade via one of the two remaining sites. - Test asserts both invariants: unref'd listeners keep serving across GC churn, and the child still exits on its own (poll_ref release intact).
Extended reasoning...
Overview
Removes three lines from Listener::unref() in src/runtime/socket/Listener.rs that downgraded the listener's this_value JsRef to weak whenever active_connections == 0. Adds a comment explaining why the downgrade must not happen here, and a subprocess regression test in test/js/bun/net/socket.test.ts.
The removed downgrade contradicted the field's own doc comment ("Strong while it is listening or has connections"): with zero connections but the listen socket still open, the wrapper became weak, GC swept it and the JSSocketHandlers cell it roots, and the next accepted connection dispatched on_open into a freed cell (or, once the finalizer ran, the port was closed outright). The two legitimate downgrade sites — do_stop and Handlers::mark_inactive — are both already gated on matches!(listener.listener.get(), ListenerType::None), so they only fire after the listen socket is closed. I confirmed this in the current tree.
Security risks
None introduced. The change strictly strengthens a GC root, closing a use-after-free reachable from user JS via Bun.listen(...).unref() + dropped reference + GC. It does not add attack surface, parse untrusted input, or touch auth/crypto.
Level of scrutiny
High. This is JSC GC lifetime management for a live I/O resource — REVIEW.md's most-blocked native category. Even though the diff is a three-line deletion, the correctness of the fix depends on the two remaining downgrade sites covering every stop/drain ordering, and on the strong this_value not preventing process exit (it doesn't — only poll_ref affects loop liveness, and that is still unref'd). I traced these paths and they hold, but a maintainer should validate the reasoning given how load-bearing the strong/weak transition is here.
Other factors
- The test is well-formed per repo conventions: subprocess with
bunEnv/bunExe(), drains stdout/stderr/exit concurrently, asserts a combined{stdout, stderr, exitCode}object, usesport: 0, and covers both the "keeps accepting under GC" and "process exits when unref'd" invariants. PR states it fails underUSE_SYSTEM_BUN=1and passes on the debug build. ref_()still callsset_strongunconditionally (idempotent now thatunref()never downgrades), soref()/unref()symmetry is preserved.- No sibling site shares this exact bug:
UDPSocket::unrefalready only touchespoll_ref; individual-socket downgrades insocket_body.rsall fire on close/error, not onunref(). - Deferring rather than approving because GC-root changes on live sockets are the kind of thing a Bun maintainer should sign off on, not because I found a defect.
|
Updated 8:39 PM PT - Aug 4th, 2026
❌ @robobun, your commit 5bd46d3 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 36930That installs a local version of the PR into your bun-36930 --bun |
The HTTP shape raced on Windows: the server replies and ends without reading the request, and closing with the request bytes unread resets the connection, so fetch saw ECONNRESET. A Bun.connect client that sends nothing has no unread data to reset on.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
CI status: the diff's own coverage is green on every lane, including both Windows lanes after switching the test's client from fetch to a raw TCP socket (the HTTP shape raced on Windows: the server replies and ends without reading the request, and closing with the request unread resets the connection). The remaining red is the debian 13 x64-asan lane only, and both failures are unrelated to this change and present on main: an AsyncLocalStorage-tracking.test.ts leak report pointing into boringssl RSA keygen (node_crypto_binding.rs:85), and a pre-existing worker-transfer-terminate-stress.test.ts SIGABRT. Both have been reported separately. Ready for review. |
There was a problem hiding this comment.
I didn't find any bugs, but this touches GC-rooting semantics for a live listen socket and overlaps with #36181, so it's worth a human look at whether both changes should land together or one supersedes the other.
What was reviewed:
- Verified
Handlers::mark_inactiveanddo_stopare both gated onListenerType::Nonebefore downgrading, so the removed branch was indeed the only site that could weaken a still-listening wrapper. - Checked that
ref_()still callsset_strong— now redundant while listening but idempotent, sounref(); ref()is unchanged. - Test drains stdout/stderr/exited concurrently, asserts the combined object, and covers both the keeps-accepting invariant and the process-exits-on-its-own invariant.
Extended reasoning...
Overview
The Rust change is three lines removed from Listener::unref: it no longer downgrades this_value to weak when active_connections == 0. A three-line comment replaces it, pointing to do_stop / mark_inactive as the owners of the downgrade. The test spawns a child that creates four unref'd listeners, drops their JS references, drives GC with allocation churn, round-trips each port with a raw Bun.connect, and requires the child to exit on its own.
Security risks
None identified. This is a lifetime/GC-rooting change; no input parsing, auth, or crypto is touched.
Level of scrutiny
High. The mechanical diff is tiny, but it changes when a JS wrapper that roots the JSSocketHandlers cell can become collectable. The PR description's contract ("Strong while it is listening or has connections") checks out against the field doc and the two remaining downgrade sites — Handlers::mark_inactive at Handlers.rs:270 only downgrades when listener.listener.get() is None, and do_stop sets it to None first. So a listening-but-unref'd server now stays strongly rooted until stop(), which is the intended Node-compatible behavior, and the poll_ref release still lets the loop drain. I don't see a new leak: the only way to reach the state where the strong ref is never released is to never call stop(), in which case the socket is intentionally still accepting.
Other factors
The duplicate-PR bot flags #36181 as fixing the same root cause at a different site (finalizer deferral), and notes this PR "removes the only trigger #36181's deferral branch has." The author's description argues both stand independently (#36181 covers teardown-during-finalize with live connections; this covers accept-before-finalize with zero connections). That reasoning is plausible, but a maintainer should decide whether to land both, in what order, or whether one obsoletes part of the other. The comment-cop bot fired twice on the new comment and both threads are resolved with an author reply; the final comment is short and load-bearing (it names the two sites that own the invariant), so I don't think it needs further trimming. CI build #89060 was still in progress at review time.
…stening (oven-sh#36930) ## Reproduction ```js function makeUnreffedListener() { const l = Bun.listen({ hostname: "127.0.0.1", port: 0, socket: { open(s) { s.write("HTTP/1.1 200 OK\r\nContent-Length: 2\r\nConnection: close\r\n\r\nok"); s.end(); }, data() {}, }}); const port = l.port; l.unref(); // still listening; only the event-loop hold should drop return port; } const port = makeUnreffedListener(); Bun.gc(true); await Bun.sleep(1); Bun.gc(true); console.log(await fetch("http://127.0.0.1:" + port + "/").then(r => r.text())); ``` Stock bun (release): `ConnectionRefused` once the collected wrapper's finalizer closes the still-listening socket. A fuzz variant that hammers the port while GC is in flight instead lands in the sweep window: 10/10 within 30s as a silent SIGABRT, `TypeError: r.text is not a function` on the fetch result, or another listener's `open` handler answering on the dropped listener's port. Under ASan the dispatch faults in `Bun__SocketHandlers__getField` (`JSSocketHandlers.cpp`) or as a UBSan member call on null ClassInfo in `Bun__JSValue__call`, reached from `NewSocket<false>::on_open` via `us_internal_dispatch_ready_poll`, i.e. a loop dispatch on the swept handlers cell before the finalizer ever runs. ## Cause `Listener::unref` released the event-loop ref and then downgraded `this_value` to weak whenever `active_connections == 0`, while the listen socket was still open. That contradicts the field's own contract ("Strong while it is listening or has connections"): the wrapper's `handlers` slot is the only GC root of the `JSSocketHandlers` cell that every accept dispatches into. With the wrapper weak and the JS reference dropped, GC collects both while the socket keeps accepting, so `on_open` for the next connection reads a swept cell; after the finalizer runs, the port is closed outright, which is not Node semantics either (libuv keeps unref'd servers accepting). ## Fix `unref()` now only does `poll_ref.unref()`. The idle downgrade already lives in the two places that are gated on the listen socket being closed: `do_stop` (close with no connections) and `Handlers::mark_inactive` (last connection drains after a close). A stopped or idle listener is still collected exactly as before; a listening one stays reachable until `stop()`, and the process can still exit because the event-loop ref is released. This is a different site from oven-sh#36181, which defers the finalizer's socket-group close out of the GC sweep when a collected listener still has live connections: that covers the teardown dispatch during finalization, while this bug fires on a fresh accept before any finalizer runs and needs no connections at drop time. Both changes stand on their own. ## Verification New test in `test/js/bun/net/socket.test.ts` spawns a child that drops four unref'd listeners, drives GC with allocation churn, and fetches every port each round; the child must serve every request and then exit on its own (proving `unref()` still releases the loop). `USE_SYSTEM_BUN=1`: child gets `ConnectionRefused` on the first collected listener, test fails (also fails on an unfixed debug build, 1.5s). `bun bd test`: passes (4.1s, ASan). The 30s fuzz hammer on the fixed debug build: 18 rounds, 80/80 requests served, no crash, no wrapper collected while listening. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · 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 <!-- robobun:evidence:end -->
2 participants
Reproduction
Stock bun (release):
ConnectionRefusedonce the collected wrapper's finalizer closes the still-listening socket. A fuzz variant that hammers the port while GC is in flight instead lands in the sweep window: 10/10 within 30s as a silent SIGABRT,TypeError: r.text is not a functionon the fetch result, or another listener'sopenhandler answering on the dropped listener's port. Under ASan the dispatch faults inBun__SocketHandlers__getField(JSSocketHandlers.cpp) or as a UBSan member call on null ClassInfo inBun__JSValue__call, reached fromNewSocket<false>::on_openviaus_internal_dispatch_ready_poll, i.e. a loop dispatch on the swept handlers cell before the finalizer ever runs.Cause
Listener::unrefreleased the event-loop ref and then downgradedthis_valueto weak wheneveractive_connections == 0, while the listen socket was still open. That contradicts the field's own contract ("Strong while it is listening or has connections"): the wrapper'shandlersslot is the only GC root of theJSSocketHandlerscell that every accept dispatches into. With the wrapper weak and the JS reference dropped, GC collects both while the socket keeps accepting, soon_openfor the next connection reads a swept cell; after the finalizer runs, the port is closed outright, which is not Node semantics either (libuv keeps unref'd servers accepting).Fix
unref()now only doespoll_ref.unref(). The idle downgrade already lives in the two places that are gated on the listen socket being closed:do_stop(close with no connections) andHandlers::mark_inactive(last connection drains after a close). A stopped or idle listener is still collected exactly as before; a listening one stays reachable untilstop(), and the process can still exit because the event-loop ref is released.This is a different site from #36181, which defers the finalizer's socket-group close out of the GC sweep when a collected listener still has live connections: that covers the teardown dispatch during finalization, while this bug fires on a fresh accept before any finalizer runs and needs no connections at drop time. Both changes stand on their own.
Verification
New test in
test/js/bun/net/socket.test.tsspawns a child that drops four unref'd listeners, drives GC with allocation churn, and fetches every port each round; the child must serve every request and then exit on its own (provingunref()still releases the loop).USE_SYSTEM_BUN=1: child getsConnectionRefusedon the first collected listener, test fails (also fails on an unfixed debug build, 1.5s).bun bd test: passes (4.1s, ASan). The 30s fuzz hammer on the fixed debug build: 18 rounds, 80/80 requests served, no crash, no wrapper collected while listening.no test proof · iteration 1 · 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