hot: reuse Bun.listen/Bun.serve listeners across --hot reloads - #30906
hot: reuse Bun.listen/Bun.serve listeners across --hot reloads#30906robobun wants to merge 3 commits into
Conversation
|
Updated 9:02 PM PT - Aug 15th, 2026
✅ @robobun, your commit 6e137cb6b08f975c7112d4dabcb571618eeff8e1 passed in 🧪 To try this PR locally: bunx bun-pr 30906That installs a local version of the PR into your bun-30906 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughDetect and reuse existing listening sockets on hot reload: lazily-initialize hot-map, compute per-listen hot_id, register/unregister listeners, swap handlers for matched listeners preserving active connections, register servers under internal config id, add listen id types, and add regression tests. ChangesHot-reload socket reuse
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/socket/Listener.rs (1)
878-901:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the watch-mode fd on the finalize path as well.
listen()registers non-TLS sockets withadd_listening_socket_for_watch_mode(), but this cleanup path only removes the hot-map entry. If an unref'ed listener is GC-finalized instead ofstop()'d, the socket gets closed while its fd stays in the VM watch registry, which can later target an unrelated descriptor once the OS reuses that number. Routefinalize()through the same fd-unregister helper asdo_stop()beforeself.listeneris cleared.Also applies to: 966-969
🤖 Prompt for 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. In `@src/runtime/socket/Listener.rs` around lines 878 - 901, In finalize(), before clearing self.listener (before calling self.listener.replace(...)) call the same fd-unregister helper used by do_stop() (e.g. remove_listening_socket_for_watch_mode or the project's equivalent) to remove the watch-mode fd registration for the socket, then proceed with unlink_unix_socket_path and closing; ensure the unregister call happens while you still have the socket/fd (i.e. before ListenerType::Uws is replaced/consumed). Apply the same change to the other finalize path at the referenced location (lines 966-969) so GC-finalized listeners also unregister from the VM watch registry.
🤖 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 `@test/cli/hot/hot.test.ts`:
- Around line 851-862: The background stderr reader promise (stderrDone) can
reject when runner.kill() aborts runner.stderr; modify the IIFE invocation that
assigns stderrDone so the promise is swallowed by appending .catch(() => {}) to
the IIFE (same pattern used in other hot-reload stream readers), keeping the
existing for-await-of over runner.stderr and the EADDRINUSE/Failed-to checks but
preventing an unhandled rejection after assertions pass.
---
Outside diff comments:
In `@src/runtime/socket/Listener.rs`:
- Around line 878-901: In finalize(), before clearing self.listener (before
calling self.listener.replace(...)) call the same fd-unregister helper used by
do_stop() (e.g. remove_listening_socket_for_watch_mode or the project's
equivalent) to remove the watch-mode fd registration for the socket, then
proceed with unlink_unix_socket_path and closing; ensure the unregister call
happens while you still have the socket/fd (i.e. before ListenerType::Uws is
replaced/consumed). Apply the same change to the other finalize path at the
referenced location (lines 966-969) so GC-finalized listeners also unregister
from the VM watch registry.
🪄 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: 1bb1f6f8-8bd6-4c2d-bcea-1db4464d3fc1
📒 Files selected for processing (4)
src/jsc/VirtualMachine.rssrc/runtime/api/BunObject.rssrc/runtime/socket/Listener.rstest/cli/hot/hot.test.ts
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@packages/bun-types/bun.d.ts`:
- Around line 6477-6488: The id property is currently declared on
UnixSocketOptions<Data> and thus appears on both Bun.connect() and Bun.listen();
update the typings so id is only present on the listen overload: split
UnixSocketOptions<Data> into separate UnixConnectOptions<Data> and
UnixListenOptions<Data> (or make id optional only on the listen overload) and
use UnixConnectOptions<Data> for Bun.connect() and UnixListenOptions<Data> for
Bun.listen(); adjust the type references where UnixSocketOptions<Data> is used
(including any overload signatures) to ensure Bun.connect() no longer exposes id
while Bun.listen() retains it.
In `@src/runtime/socket/Listener.rs`:
- Around line 225-229: The user-provided id is used verbatim which can collide
with Bun.serve keys; update the Some(id) branch in Listener.rs (where hot_id is
set using opts.get and compute_hot_id) to namespace IDs: convert
id.to_slice(...) to a string, detect whether it already starts with the listener
prefix ("[tls]-" when ssl_enabled true or "[tcp]-" when false), and if missing
prepend the correct prefix, then convert back into a Box<[u8]> for hot_id; keep
the existing None/null handling and reuse compute_hot_id only for the default
case.
In `@test/cli/hot/hot.test.ts`:
- Around line 782-802: The test currently resolves the Promise inside the
Bun.connect socket.data callback on the first chunk which is flaky because TCP
can split "v1"/"v2" across chunks; change the client logic created via
Promise.withResolvers and Bun.connect so socket.data appends incoming chunks to
a buffer string and only resolves when the stream ends (socket end/close) or
when the accumulated buffer matches the full expected token (e.g., starts with
"v" followed by digits), and keep socket.error / socket.connectError rejecting
as-is; also ensure you close the socket after resolving (sock.end) as before.
🪄 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: a60d8a76-1120-4de3-9ffd-340e687bd19c
📒 Files selected for processing (3)
packages/bun-types/bun.d.tssrc/runtime/socket/Listener.rstest/cli/hot/hot.test.ts
StatusHead is 6e137cb, rebased onto main a42889a (squashed; rebase notes are in the PR description). All review threads resolved. Since the last status
Reproductionbun bd test test/cli/hot/hot.test.ts -t "should reuse the listening socket"Fails with |
f1153ba to
b7b996a
Compare
6719d39 to
0c8e0ca
Compare
0c8e0ca to
1389d3c
Compare
There was a problem hiding this comment.
No new findings on a735408 — the is_strong() gate addresses the last concern. Deferring to a human for the new public id API and the native listener/hot-map lifecycle wiring.
Extended reasoning...
Overview
This PR wires Bun.listen into the per-VM HotMap so --hot reloads reuse the existing listening socket instead of failing EADDRINUSE. It touches:
src/runtime/socket/Listener.rs(~200 lines): newhot_idfield,compute_hot_id, hot-map lookup withunsafe { &*entry.ptr.cast::<Listener>() }, handler swap via a newcopy_callbacks_from,register/unregister_for_hot_reload, anddo_stop/deinit/watch-mode-fd cleanup wiring.src/runtime/socket/{Handlers.rs, JSSocketHandlers.rs}: newcopy_callbacks_from/callbacks()helpers.src/runtime/api/BunObject.rs: guardsBun.serve'shot.insert_rawagainst duplicate keys and flipsallow_hot = falseon skip.packages/bun-types/bun.d.ts: new publicid?: string | nullonTCPSocketListenOptionsand a newUnixSocketListenOptions.test/cli/hot/hot.test.ts: new describe block driving three--hotreloads for bothBun.listenandBun.serve.
Security risks
No auth/crypto/permissions surface. The main risk class is memory safety: raw-pointer deref from a shared registry, and returning a JS wrapper whose liveness depends on JsRef state. The latter was flagged and fixed in a735408 (gate on is_strong() so a downgraded/possibly-GC-dead Weak wrapper is never returned). The unsafe cast is guarded by a tag check and by unregister_for_hot_reload running in both do_stop and deinit before the allocation is freed.
Level of scrutiny
High. This is native runtime lifecycle code with GC interaction, adds public API surface (id option), and went through 14 iterations with several correctness fixes during review — including a design reversal (verbatim user ids → [listen]- namespaced) and a memory-safety fix in the final commit. That churn is exactly why a human should sign off on the settled shape.
Other factors
All 20 prior review threads (mine and CodeRabbit's) are resolved. The bug-hunting system found nothing on the current head. The test gates the headline fix (same port across reloads) but, as acknowledged in the resolved thread on line 896, the responses assertion doesn't strictly prove handler-swap vs. shared-global read — the author accepted this as a follow-up. One acknowledged-and-deferred limitation remains (TLS toggle with a pinned id keeps the old protocol, matching Bun.serve's existing behavior). None of these block, but they're worth a human being aware of.
a735408 to
ea28168
Compare
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunting pass found no issues on 6e137cb. Because it adds user-facing API (id on Bun.listen) and threads a raw *mut Listener through the shared HotMap with GC-lifetime reasoning around JsRef strong/weak states, a human look is still worthwhile.
What was reviewed:
- The
is_strong()gate on the reuse path — a downgraded/weakthis_valuefalls through todo_stop+ rebind rather than returning a possibly-dead wrapper. insert_rawnow returnsfalseon collision; both callers skip registration andBun.serveflipsallow_hot = falseso itsstop()won't evict the foreign entry.unregister_for_hot_reloadruns on bothdo_stopanddeinitbefore the allocation is freed, andhot_idismem::taken so a second call is a no-op.- The new test drives reloads via stdout events (no sleeps), buffers to newline framing, and wires
close/error/connectErrorto reject.
Extended reasoning...
Overview
Wires Bun.listen into the per-VM HotMap so --hot re-evaluations reuse the bound socket instead of failing EADDRINUSE, mirroring what Bun.serve already does. Touches rare_data.rs (insert_raw panic → bool), BunObject.rs (serve's collision handling), Listener.rs (~130 new lines: key computation, lookup + handler swap, register/unregister, watch-mode fd tracking), Handlers.rs/JSSocketHandlers.rs (a copy_callbacks_from helper and its callbacks() reader), bun.d.ts (new id?: string | null option + UnixSocketListenOptions), and a new describe block in hot.test.ts.
Security risks
None identified. The hot-map is per-VM and only populated under --hot; keys are derived from the caller's own listen options; the tag check gates the pointer cast. No auth/crypto/permission surface.
Level of scrutiny
High. This is not a mechanical change: it introduces a new raw-pointer side channel (HotMapEntry.ptr → &Listener) whose lifetime is decoupled from the JS wrapper's GC lifetime, and gets that right only via the is_strong() gate plus unregister_for_hot_reload running before every free path. A prior revision of this exact PR had a real GC-window bug here (JsRef::Weak returning a dead value), fixed after review. It also adds public API (id on TCPSocketListenOptions/UnixSocketListenOptions) that a maintainer should sign off on.
Other factors
The PR has been through many review rounds; every inline thread is resolved and the head commit addresses the last nit (missing close reject in the test's Bun.connect client). The bug hunter found nothing on the current head. The comment-cop bot fired ~15 identical warnings on comments that are now single-line; those threads are marked resolved. The new test is hermetic (port: 0, local connect, tempDir), asserts port stability across three reloads, and fails on the unfixed build. Given the unsafe-pointer/GC surface and the new public option, I'm deferring rather than approving.
What
Under
--hot, re-evaluating the entry module re-runsBun.listen()on an address that is still bound by the previous evaluation's listener, failing withEADDRINUSE:Bun.serveavoids this via the per-VMHotMap: each server registers under a computed key, and a subsequentBun.serve()with the same address swaps handlers on the live server instead of re-binding.Bun.listenhad noHotMapwiring at all.(Two adjacent bugs this PR originally also fixed,
VirtualMachine::hot_map()never lazy-initializing andBun.serveinserting under the empty key becauseconfig.idwas read afterNewServer::init()took the config, have since landed on main via #31783. This PR now contains only theBun.listenwork plus hardening of the shared registry.)Fix
Bun.listenmirrorsBun.serve:[tcp]-tcp:host:port/[tls]-.../[tcp]-unix:pathkey from the requested address, look it up before binding, and on a match swap handlers/datain place on the existing listener (same swap aslistener.reload()), returning the existing JS wrapper.stop()/deinit(). Non-TLS listen fds are also registered for--watchcleanup, matching the HTTP server path.id?: string | nulloption (matchingBun.serve) disambiguates multiple listeners on the same address;id: null/""opts out. User-supplied ids are namespaced with a[listen]-prefix so listener keys are structurally disjoint fromBun.serve's. A listen and a serve can never reuse each other (different tags), so sharing a key would only leave whichever registers second untracked.HotMap::insert_rawnow returnsboolinstead of panicking on a duplicate key. Its only two callers (Bun.serveand the new listener registration) skip registration onfalse;Bun.serveadditionally flipsallow_hotso itsstop()does not evict the entry that is there. This covers the contrivedBun.serve({id: "[listen]-..."})overlap without a second lookup.JsRefis strong (a registered listener holds it strong while listening); anything else closes the stale socket and rebinds rather than handing back a possibly dead weak value.idadded toTCPSocketListenOptionsand a newUnixSocketListenOptions.Verification
New
describe("should reuse the listening socket on hot reload")intest/cli/hot/hot.test.tsspawnsbun --hotwith aBun.listen/Bun.servefixture onport: 0, drives three reloads, and asserts the same resolved port is reported every time and a client connect succeeds after each reload. On the unfixed build each reload binds a different random port, so the port-stability assertion fails; with the fix all three generations reuse one socket.Also checked manually under
--hot: listen+serve sharing a colliding key in both orders (listener reused, server untracked and rebound,server.idintact, no panic),id: null/""opt-out, distinct-id isolation,stop()-then-relisten, andunref()-then-reload (same port reused).Fixes #26036.
Rebase notes
Rebased onto main after roughly 1100 commits landed; squashed to one commit. Three textual conflicts, all resolved by keeping both sides:
Listener::listennamed-pipe path: main addedactive_handlesregistration next to where this PR addsregister_for_hot_reload.Listener::deinit: main switchedthis_refto a shared borrow;unregister_for_hot_reloadtakes&Selfso it slots in unchanged.hot.test.tsimports: main addedisWindows, this PR addstempDir.Main also changed
Listener::unref()to keepthis_valuestrong while the socket is still listening (it is downgraded indo_stop/mark_inactiveinstead). That makes the non-strong fallback in the reuse path purely defensive. Fullhot.test.tspasses after the rebase.no test proof · iteration 18 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/hot/hot.test.ts