hot: stop Bun.serve instances the new generation did not re-adopt - #32957
hot: stop Bun.serve instances the new generation did not re-adopt#32957robobun wants to merge 3 commits into
Conversation
Under --hot, each Bun.serve registers in the per-VM HotMap keyed by its computed host:port id. On soft reload, a Bun.serve call with a matching id adopts the existing server (swapping its handlers in place), but if the reloaded module never calls Bun.serve again, or binds a different address, nothing ever stops the previous generation's server. Its listener stays bound and keeps invoking the old generation's fetch closure indefinitely, and the port can never be reclaimed. Stamp every HotMap entry with the hot_reload_counter at the time it is inserted or adopted. Once the reloaded entry point's internal promise fulfills, sweep the map and gracefully stop (same semantics as server.stop()) every server whose stamp predates the current counter. The sweep is skipped when the reload throws, so the last good generation keeps serving, matching the existing error-recovery behavior.
|
Warning Review limit reached
More reviews will be available in 6 minutes and 40 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
|
Updated 12:53 AM PT - Jun 28th, 2026
❌ @robobun, your commit 25c574a has 2 failures in
🧪 To try this PR locally: bunx bun-pr 32957That installs a local version of the PR into your bun-32957 --bun |
/^gen [2-9]\d*$/ rejected gen 10-19, gen 100-199, etc., contradicting the "any generation past 1" comment and the waitFor predicate above it.
There was a problem hiding this comment.
Thanks for fixing the regex — no further issues from me. This changes user-visible --hot lifecycle semantics for Bun.serve (stopping servers the new generation didn't re-adopt), so I'll leave the design sign-off to a human.
Extended reasoning...
Overview
This PR adds an orphan-sweep step to --hot reloads: HotMapEntry gains a generation stamp, Bun.serve stamps it on insert/adopt, and after the reloaded entry-point promise fulfills, hot_stop_orphaned_servers (wired through a new RuntimeHooks slot) gracefully stops any Bun.serve instance whose stamp predates the current hot_reload_counter. Touches src/jsc/VirtualMachine.rs, src/jsc/rare_data.rs, src/runtime/api/BunObject.rs, src/runtime/jsc_hooks.rs, plus four new tests in test/cli/hot/hot.test.ts.
Prior feedback
My earlier inline comment about /^gen [2-9]\d*$/ rejecting gen 10..19 was addressed in 3101795 — the test now uses /^gen ([2-9]|[1-9]\d+)$/, which correctly matches any integer ≥ 2. The bug-hunting pass on the updated revision found nothing.
Security risks
None identified. The change is gated on hot_reload == HOT_RELOAD_HOT, only narrows what stays listening (it stops servers, never starts or exposes anything), and reuses the existing server.stop(false) graceful-stop path.
Level of scrutiny
Medium-high. The implementation is small and follows the established RuntimeHooks / cron_clear_all_reload pattern, and the four tests cover adopt / no-adopt / different-id / throw-during-load. But it is a deliberate change to --hot's observable lifecycle: servers that previously kept running will now be stopped. That's a product/semantics decision (e.g., interaction with servers created asynchronously after the entry-point promise fulfills, or with allow_hot/reusePort edge cases) that a maintainer should sign off on rather than a bot.
Other factors
The sweep snapshots stale entries before iterating (avoiding iterator invalidation when stop removes its own key), runs at most once per generation via hot_reload_orphan_swept_at, and is skipped on rejection so the last good generation keeps serving — all of which look correct. I'm deferring purely on scope, not on any concrete concern with the code.
Re-run CI. Build 65927 had no real failures: the linux x64-asan build-rust, freebsd x64 build-rust, and linux x64-baseline build-bun jobs expired in the agent queue without running, and the same steps passed on every lane that got an agent.
|
Update now that build 66050 has finished: 240 jobs passed, and Full accounting for build 66050:
The previous build, 65927 (3101795), was red for the same class of reason: three different jobs ( Verification, against the local ASAN debug build:
I already pushed one empty |
What
Under
--hot, aBun.serve()from a previous module generation is never shut down when the reloaded code no longer callsBun.serve(or moves to a different host/port). The orphaned listener keeps serving the old generation'sfetchhandler indefinitely, and nothing else can ever bind the port. There is also no handle to close it from the new generation.This contradicts
docs/runtime/watch-mode.mdx("your HTTP server will be reloaded with the updated code without the process being restarted"): when the updated code has no server at all, one keeps running with the old handler.Reproduction
After the reload the source on disk has no
Bun.serve, but the port still answers withgen 1.Cause
Each
Bun.serveunder--hotregisters in the per-VMHotMapkeyed by its computedhost:portid. When the next generation callsBun.servewith a matching id, the entry is found and the existing server is adopted (handlers swapped in place). That is the only path that touches the entry. If the new generation never callsBun.serveagain, or binds a different address (a different id), nothing removes or stops the old server.VirtualMachine::reload()has no symmetric teardown for it.Fix
HotMapEntrygains ageneration: u32stamped fromvm.hot_reload_counterwhenever an entry is inserted or adopted byBun.serve.server.stop(): close the listener, let in-flight requests drain) every server whose stamp predates the current counter.--hot, at most once per generation, and is skipped when the reloaded module throws, so the last good generation keeps serving. That matches the existing error-recovery behavior.The sweep body lives in
bun_runtimeand is reached frombun_jscthrough a newRuntimeHooks::hot_stop_orphaned_serversslot, the same forward-dep patterncron_clear_all_reloadalready uses on this code path.Verification
Four new tests in
test/cli/hot/hot.test.tsunder--hot Bun.serve orphaned server:Bun.serve: the old port stops answeringThe first two fail on the unfixed build (the old port keeps answering
gen 1); all four pass with the fix. The last two also pass without the fix and guard against the sweep being over-eager.The probes send
Connection: closeso each opens a fresh TCP connection. The sweep closes the listener, not sockets that are already established, and a pooled keep-alive socket from an earlier fetch would otherwise still get answers.Note: this is specific to
Bun.serve.Bun.listennever hadHotMapwiring at all; that is #30906, which is the inverse problem (the old listener is never found, so the new one getsEADDRINUSE).Behavior notes for review
These are the deliberate semantic edges of the sweep, for the lifecycle sign-off:
awaitbeforeBun.serveis covered (the promise stays pending until it resolves). ABun.servedeferred past module evaluation (for example inside asetTimeout) now gets a fresh server instead of adopting the previous generation's, because the sweep already stopped it. That matches the report's framing ("after the new generation finishes evaluating") and the server still ends up listening; I don't think anyone is relying on adopting a listener from a timer callback, but it is a behavior change for that pattern.server.stop()'s semantics. The listener closes (no new connections can reach the old handler), in-flight requests drain, and idle keep-alive sockets are reaped by the server's idle timeout. Full teardown happens once GC finalizes the now-unreferenced JSServerobject, the same lifecycle as callingserver.stop()and dropping the reference.allow_hot: falseservers never enter theHotMap, so they are invisible to both the existing adoption path and this sweep; their (pre-existing) behavior is unchanged. The sweep covers exactly the set of servers the adoption path covers.