Bun.serve: keep the server alive while an HTML route is building - #37813
Conversation
An HTML route served without the DevServer bundles on its first request. The plugin load and the build finish on later event-loop turns and call back into the server through the route's raw back-pointer, but nothing held the server for the route: the waiting clients only count as connections. Once they disconnected, stop() settled, the wrapper became collectable and the NewServer was freed, and the completion then used it (heap-use-after-free in Route::on_complete / Route::on_plugins_resolved). A route in State::Building now counts as a pending request on the server, like an in-flight StaticRoute/FileRoute response does, and releases it when it leaves that state (build complete or plugin load rejected).
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes Bun.serve lifecycle accounting (a UAF fix that also alters when server.stop() settles and what pendingRequests reports), a human look would still be worthwhile.
What was reviewed:
- Traced
on_pending_request/on_request_completebalance across all fourschedule_bundleoutcomes (Err, Ready, Pending→resolved, Pending→rejected) — each path pairs exactly once. - Confirmed
on_plugins_rejected/on_plugins_resolvedare only reached viaserver_body.rsafter thePendingarm took its ref, and the dev-mode state reset only touchesHtml/Err, soBuildingis never dropped withoutfinish_building. - Checked that removing the
Optionguards inon_completeis safe:self.serveris alwaysSomeonceschedule_bundleruns, andfinish_buildingis the last server access beforedeinit_if_we_can. resume_pending_responsesvisibility narrowed tofn— no external callers remain.
Extended reasoning...
Overview
This PR fixes a heap-use-after-free in Bun.serve's non-DevServer HTML route path. html_bundle::Route holds a raw AnyServer back-pointer and bundles lazily; the plugin load and build completion call back through that pointer on later event-loop turns, but nothing kept the server alive on the route's behalf. The fix: entering State::Building bumps the server's pending_requests counter (via on_pending_request()), and both exits from that state (on_complete, on_plugins_rejected) release it via a new finish_building(). This matches how StaticRoute/FileRoute already account for their async work. A three-variant subprocess test asserts the observable contract (pendingRequests == 1 while parked, stop(true) stays pending, wrapper collectable afterwards) and reproduces the ASAN crash on the unfixed build.
Security risks
None. This is an internal lifecycle-accounting fix; no new user-controlled input is parsed and no trust boundaries change.
Level of scrutiny
High. This is memory-safety code in the HTTP server's deinit path — exactly the "most-blocked category" REVIEW.md calls out. The counter must be provably balanced on every terminal path or the server either UAFs (under-count, the bug being fixed) or leaks / never settles stop() (over-count). I traced every entry into and exit from State::Building:
GetOrStartLoadResult::Err— no ref taken, nofinish_building; state goes toErrand the current request handles it synchronously.Ready— ref taken,on_plugins_resolvedsetsBuilding(Some), lateron_complete→finish_building. The?onon_plugins_resolvedonly propagates OOM (caller ishandle_oom), which aborts.Pending— ref taken,Building(None);server_body.rslater invokes exactly one ofon_plugins_resolved(→Building(Some)→on_complete→finish_building) oron_plugins_rejected(→finish_building).- The dev-mode-without-HMR reset in
on_any_requestonly replacesHtml/Err, neverBuilding, so a build in flight cannot lose its ref. Route::dropwhileBuildingcannot happen: the route is intrusively ref'd by the completion task (Building(Some)) or byServePlugins' stored callback (Building(None)).
The removal of the if let Some(server) / else { return } guards in on_complete is justified by the new invariant (the pending-request hold guarantees the server is still allocated), and self.server is never cleared once set. finish_building calls on_request_complete last, after resume_pending_responses, so the server's deinit_if_we_can cannot schedule teardown before the pending clients have been answered.
Other factors
The PR carries a user-visible behavior change the author calls out: server.pendingRequests now reads 1 during an HTML build, and await server.stop() waits for that build. That is consistent with how in-flight fetch handlers already behave, but it is a semantics change a maintainer should sign off on. The author also explicitly scopes out the VM-teardown-cancels-build case, arguing it matches existing fetch-handler-at-exit behavior; that argument reads correctly to me but is another judgment call. The test is well-constructed (fails on the unfixed build per the description, covers all three exit paths, uses test.concurrent, hermetic subprocess), so the mechanical correctness is well-covered — the human review is for the design/semantics call, not for bug-hunting.
|
Updated 9:37 AM PT - Aug 12th, 2026
✅ @robobun, your commit f908be079c293b9ee9230e80546641a7180c55b0 passed in 🧪 To try this PR locally: bunx bun-pr 37813That installs a local version of the PR into your bun-37813 --bun |
Problem
development: falseor{ hmr: false }) bundles on its first request. If the only client disconnects andserver.stop(true)is called while a[serve.static]plugin still has that build parked,stop()settles, the next GC frees the server, and the build then finishes against the freed server.AddressSanitizer: heap-use-after-freeinhtml_bundle::Route::on_complete(parked inonLoad) orRoute::on_plugins_resolved(parked in the plugin'ssetup()). A release build reads the freedNewServerwith no report.Fix
server.pendingRequestsis 1 while an HTML route bundles andawait server.stop()waits for the bundle, as it already does for afetchhandler still running. A build cancelled by VM teardown is not covered; at exit it leaves the same state an in-flightfetchhandler does.stop(), and the first two die with the ASAN reports above. Fixed: all three pass, as do the existing HTML-serve tests that do not need the DevServer.Background
Bun.serve({ routes: { "/": html } })with an imported.htmlfile. Without the DevServer the route bundles the page once, on the first request, registers the outputs as static routes, and holds requests that arrive during the build.[serve.static]plugins: a bunfig entry naming bundler plugins for these routes, loaded on the first request. Both the plugin load and the bundle finish on later event-loop turns and complete by calling back into the server through a raw pointer stored on the route.server.pendingRequests.stop()settles and the server can be torn down only when the count is zero; static and file routes already raise it when a response goes asynchronous.Original description
Repro
HTML route served without the DevServer (
development: falseor{ hmr: false }), with a[serve.static]plugin whoseonLoadparks on a promise. Request the route, drop the client,server.stop(true), drop the server,Bun.gc(true)plus a couple of event-loop turns, then letonLoadresolve. Debug (ASAN) build:Parking in the plugin's
setup()instead (so the route is still waiting for the plugin load when the server goes away) gives the same report one step earlier:On a release build the same sequence reads a freed
NewServer(its config, thenappend_static_route/reload_static_routeson it) without a report.Cause
html_bundle::Routekeeps a rawserverback-pointer and bundles on its first request. Both the plugin load and the build finish on later event-loop turns and call back into the server through that pointer (on_plugins_resolvedreads the config,on_completeregisters the output files as static routes and reloads the route table). While the route is inState::Building, nothing holds the server on its behalf:on_plugins_resolvedonly refs the route itself, and the clients waiting inpending_responsesonly count as connections, which they can drop at any time. So once the last client disconnects and the server is stopped,deinit_if_we_cansees no pending requests, settlesstop(), downgrades the wrapper, and the next GC frees theNewServerwith the build still in flight.StaticRoute/FileRoute/DirectoryRoutealready handle their asynchronous work with the server'spending_requestscounter (on_pending_requestwhen a response goes async,on_static_request_completewhen it finishes); the route's build is the same kind of in-flight work and was the one thing not counted.Fix
schedule_bundlecallsserver.on_pending_request()whenever the route entersState::Building(plugins ready, or plugins still loading), and the two ways out of that state (on_complete,on_plugins_rejected) go through a newfinish_building, which answers the pending responses and then callson_request_complete(). That keeps the server allocated for exactly the window in which the route will call back into it, andon_request_completeruns the idle pass, so a server that was stopped while building is downgraded and freed right after the build lands (thestop()promise now settles then as well, matching what happens for afetchhandler that is still running whenstop()is called). With that invariant,on_completeno longer needs itsOptionhandling of the back-pointer; it takes the server once at the top, the same wayon_plugins_resolvedalready did.A visible consequence:
server.pendingRequestsis 1 while an HTML route is bundling, andawait server.stop()waits for the bundle. A build whose plugin never settles therefore keeps the server allocated, as an unsettledfetchhandler already does. Not covered: a build cancelled by VM teardown never reachesRoute::on_complete(the completion task returns early oncancelled), so at exit the route keeps its ref and, now, its pending request; that is the same state an in-flightfetchhandler leaves a server in at exit and nothing observes it. The DevServer's own plugin wait uses a different back-pointer and is not changed here.Verification
test/js/bun/http/bun-serve-html-build-holds-server.test.ts(separate small file;bun-serve-html.test.tsis too slow under the debug ASAN build for a lifetime test, asbun-serve-html-hot-reload-drop.test.tsnotes). One fixture, parked in turn in the build (onLoad), in the plugin load (setup()), and in a plugin load that then rejects (theon_plugins_rejectedexit has to release the request too). Each child reportsserver.pendingRequestswhile parked, whetherstop(true)settled across ten event-loop turns before the route was released, and whether the wrapper became collectable afterwards; the test expects{ pendingRequestsWhileParked: 1, stopBeforeRelease: "pending", collectedAfterwards: true }plus a clean exit. Ifstop()did settle early, the fixture lets the server get collected before releasing the route, which is the sequence above.Unfixed debug build: all three report
pendingRequestsWhileParked: 0, stopBeforeRelease: "settled", and the first two children die with the ASAN reports above (the rejection case has no use-after-free to hit; it fails on the report). Fixed: the three pass in under a second each. Also run on the fixed debug build:bun-serve-html-405.test.ts,bun-serve-html-hot-reload-drop.test.ts,test/bake/serve-plugins-dev-server.test.ts(all pass), andbun-serve-html.test.ts, where everything that does not need the DevServer passes, includingserve plugins > concurrent requests to multiple routes during plugin load; itsdevelopment: truecases fail in this container withEMFILE while initializing file watcher for development server(inotify instance limit) before reaching any of this code.