From bcb51e01244d6cf8862f340873d92fea96ffb095 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 13:17:36 +0000 Subject: [PATCH] bake: release the app plugins cell when the dev server is torn down Bun.serve({ app }) parses framework.plugins / app.plugins into one JSBundlerPlugin cell, which Plugin::create protects from GC. The only release was UserOptions' Drop, but NewServer::init mem::take()s the bundler options out of UserOptions and hands them to the DevServer, whose Drop never called Plugin::destroy. Every dev server created with plugins therefore kept its cell, and every closure the plugins had registered, alive for the rest of the process. The same happened when UserOptions::from_js failed after creating the cell (a plugin's setup() throwing) and when DevServer::init failed after taking the options over. Hold the cell in an OwnedPlugin whose Drop releases it, so whichever struct currently holds it releases it: the bake_body options on the production CLI and on rejected serve options, the DevServer otherwise. The DevServer's slot also stores the pointer to the server's [serve.static] plugins, which it must not release, so it becomes a DevServerPlugin::{Owned, Borrowed} enum. DevServer::init now writes the options into the box only once the fallible steps that read them are done, so an error before assume_init() drops them too. --- src/runtime/api/JSBundler.rs | 46 +++++++ src/runtime/bake/DevServer.rs | 36 ++++-- src/runtime/bake/bake_body.rs | 52 ++------ src/runtime/bake/mod.rs | 32 ++++- src/runtime/bake/production.rs | 6 +- test/bake/app-plugins-release.test.ts | 57 +++++++++ .../app-plugins-release.ts | 114 ++++++++++++++++++ .../fixtures/app-plugins-release/server.ts | 5 + 8 files changed, 291 insertions(+), 57 deletions(-) create mode 100644 test/bake/app-plugins-release.test.ts create mode 100644 test/bake/fixtures/app-plugins-release/app-plugins-release.ts create mode 100644 test/bake/fixtures/app-plugins-release/server.ts diff --git a/src/runtime/api/JSBundler.rs b/src/runtime/api/JSBundler.rs index 248176152965..cb920ba447cf 100644 --- a/src/runtime/api/JSBundler.rs +++ b/src/runtime/api/JSBundler.rs @@ -1804,6 +1804,51 @@ pub mod js_bundler { } } + /// Owner of a `JSBundlerPlugin` from [`PluginJscExt::create`]. The handle + /// is a `protect()`ed JSCell, not a heap allocation: a `Box` around + /// it frees nothing (`Plugin` is a ZST opaque), so the only thing that + /// releases the cell is this type's `Drop` calling [`PluginJscExt::destroy`]. + /// JS thread only. + pub struct OwnedPlugin(core::ptr::NonNull); + + impl OwnedPlugin { + pub fn create(global: &JSGlobalObject, target: jsc::BunPluginTarget) -> Self { + Self( + core::ptr::NonNull::new(Plugin::create(global, target)) + .expect("JSBundlerPlugin__create returns a non-null cell"), + ) + } + + /// The handle itself, for holders that only borrow the plugin (the + /// bundle thread, routes and the dev server sharing a server's plugins). + /// Valid until `self` drops. + #[inline] + pub fn as_non_null(&self) -> core::ptr::NonNull { + self.0 + } + } + + impl core::ops::Deref for OwnedPlugin { + type Target = Plugin; + #[inline] + fn deref(&self) -> &Plugin { + Plugin::opaque_ref(self.0.as_ptr()) + } + } + + impl core::ops::DerefMut for OwnedPlugin { + #[inline] + fn deref_mut(&mut self) -> &mut Plugin { + Plugin::opaque_mut(self.0.as_ptr()) + } + } + + impl Drop for OwnedPlugin { + fn drop(&mut self) { + Plugin::destroy(self.0.as_ptr()); + } + } + /// Convert a JS exception value into a `logger.Msg`. If the conversion itself /// throws (e.g. `Symbol.toPrimitive` on the thrown object throws), clear that /// secondary exception and return a generic fallback message so @@ -1880,6 +1925,7 @@ pub mod js_bundler { pub use js_bundler as JSBundler; pub use js_bundler::Config; +pub(crate) use js_bundler::OwnedPlugin; /// `jsc.API.JSBundler.Plugin` — re-exported for `crate::bake` (`SplitBundlerOptions.plugin`). pub use js_bundler::Plugin; pub(crate) use js_bundler::PluginJscExt; diff --git a/src/runtime/bake/DevServer.rs b/src/runtime/bake/DevServer.rs index c8f326837b28..e83b33a2f673 100644 --- a/src/runtime/bake/DevServer.rs +++ b/src/runtime/bake/DevServer.rs @@ -198,8 +198,8 @@ pub enum Magic { #[derive(Copy, Clone, Eq, PartialEq)] pub enum PluginState { - /// Should ask server for plugins. Once plugins are loaded, the plugin - /// pointer is written into `server_transpiler.options.plugin` + /// Should ask server for plugins (unless the app brought its own). Once + /// plugins are loaded, the cell is stored in `bundler_options.plugin`. Unknown, // These two states mean that `server.getOrLoadPlugins()` was called. Pending, @@ -528,7 +528,6 @@ pub(crate) fn init(options: Options) -> JsResult> { w!(generation, 0); w!(graph_safety_lock, ThreadLock::init_unlocked()); w!(framework, options.framework); - w!(bundler_options, options.bundler_options); w!(emit_incremental_visualizer_events, 0); w!(emit_memory_visualizer_events, 0); w!( @@ -650,8 +649,11 @@ pub(crate) fn init(options: Options) -> JsResult> { // // SAFETY: `init_transpiler` writes the slot via `MaybeUninit::write` (see // `bake_body.rs`), so the previous (uninitialized) bytes are never dropped. - // `framework`/`log`/`bundler_options` were written above; reborrowing each - // individually via `addr_of_mut!` is sound because no `&mut DevServer` exists. + // `framework`/`log` were written above; reborrowing each individually via + // `addr_of_mut!` is sound because no `&mut DevServer` exists. + // `bundler_options` stays in `options` until the transpilers exist: it owns + // the app's plugin cell, and an `Err` before `assume_init()` drops nothing + // that was already written into the box. // Note: `Transpiler<'static>` erases the arena lifetime — `options.arena` // is the `UserOptions.arena` which is moved into / outlives the `DevServer` // box. Widen `'a → 'static` here once. @@ -666,7 +668,7 @@ pub(crate) fn init(options: Options) -> JsResult> { unsafe { let framework = &mut *addr_of_mut!((*p).framework); let log = &mut *addr_of_mut!((*p).log); - let bundler_options = &mut *addr_of_mut!((*p).bundler_options); + let bundler_options = &options.bundler_options; match framework.init_transpiler( arena, @@ -719,6 +721,7 @@ pub(crate) fn init(options: Options) -> JsResult> { } w!(bundler_framework_views, bundler_framework_views); + w!(bundler_options, options.bundler_options); } // ── every field is now written ─────────────────────────────────────────── @@ -2010,8 +2013,11 @@ fn ensure_route_is_bundled( } crate::server::GetOrStartLoadResult::Ready(ready) => { dev.plugin_state = PluginState::Loaded; - dev.bundler_options.plugin = - ready.map(::core::ptr::NonNull::from); + dev.bundler_options.plugin = ready.map(|plugin| { + bake::DevServerPlugin::Borrowed( + ::core::ptr::NonNull::from(plugin), + ) + }); } } } @@ -3217,7 +3223,11 @@ impl DevServer { ssr_transpiler: unsafe { ::core::ptr::NonNull::from((*self_ptr).ssr_transpiler.assume_init_mut()) }, - plugins: self.bundler_options.plugin, + plugins: self + .bundler_options + .plugin + .as_ref() + .map(bake::DevServerPlugin::as_non_null), }), // SAFETY: see `heap_ptr` note above. unsafe { &*heap_ptr }, @@ -6128,7 +6138,13 @@ impl DevServer { &mut self, plugins: Option<*mut crate::api::js_bundler::Plugin>, ) -> crate::Result<()> { - self.bundler_options.plugin = plugins.and_then(::core::ptr::NonNull::new); + // Only reached when the app declared no plugins of its own + // (`ensure_route_is_bundled` asks the server for plugins just then), + // so this never replaces an `Owned` cell. + debug_assert!(self.bundler_options.plugin.is_none()); + self.bundler_options.plugin = plugins + .and_then(::core::ptr::NonNull::new) + .map(bake::DevServerPlugin::Borrowed); self.plugin_state = PluginState::Loaded; self.start_next_bundle_if_present(); Ok(()) diff --git a/src/runtime/bake/bake_body.rs b/src/runtime/bake/bake_body.rs index ded4de92aea2..2926aacb9f1a 100644 --- a/src/runtime/bake/bake_body.rs +++ b/src/runtime/bake/bake_body.rs @@ -5,7 +5,6 @@ #![allow(unexpected_cfgs)] // `bun_codegen_embed` is set via RUSTFLAGS (scripts/build/rust.ts) for release/CI builds. use bun_alloc::ArenaVecExt as _; -use core::ptr::NonNull; use bun_alloc::Arena; // = bumpalo::Bump use bun_collections::ArrayHashMap; @@ -15,10 +14,7 @@ use bun_jsc::{JSGlobalObject, JSValue, JsError, JsResult, ZigStringSlice}; use bun_options_types::schema as bun_schema; use bun_paths::{self as paths, PathBuffer}; -// `jsc.API.JSBundler.Plugin` — opaque FFI handle for the C++ JSBundlerPlugin. -// Re-exported from `crate::api::js_bundler` so `SplitBundlerOptions.plugin` -// shares the same type the bundler pipeline uses. -pub(crate) use crate::api::js_bundler::Plugin; +use crate::api::js_bundler::OwnedPlugin; use crate::api::js_bundler::js_bundler::PluginJscExt as _; // Note: parent `mod.rs` already declares `dev_server` / `framework_router` @@ -145,20 +141,6 @@ pub struct UserOptions { pub(crate) bundler_options: SplitBundlerOptions, } -impl Drop for UserOptions { - fn drop(&mut self) { - // arena: dropped by Bump's Drop - // allocations: dropped by StringRefList's Drop - if let Some(p) = self.bundler_options.plugin { - // `p` is the FFI handle returned by `Plugin::create` in - // `parse_plugin_array`; `PluginJscExt::destroy` is its paired - // (safe) destructor — it null-checks via `opaque_ref` and - // unprotect()s the JSCell / tombstones the C++ object. - Plugin::destroy(p.as_ptr()); - } - } -} - impl UserOptions { /// Currently, this function must run at the top of the event loop. pub fn from_js(config: JSValue, global: &JSGlobalObject) -> JsResult { @@ -296,7 +278,11 @@ impl StringRefList { #[derive(Default)] pub struct SplitBundlerOptions { - pub plugin: Option>, + /// One cell holding both `framework.plugins` and `app.plugins`. Released + /// when this drops (`bun build --app`, or `Bun.serve` rejecting its + /// options) unless `NewServer::init` moves it into the `DevServer`, which + /// then owns it (`bake::DevServerPlugin::Owned`). + pub plugin: Option, pub client: BuildConfigSubset, pub server: BuildConfigSubset, pub ssr: BuildConfigSubset, @@ -312,18 +298,11 @@ impl SplitBundlerOptions { plugin_array: JSValue, global: &JSGlobalObject, ) -> JsResult<()> { - // Create the Plugin and assign it to `opts.plugin` BEFORE iterating, + // Create the Plugin and assign it to `self.plugin` BEFORE iterating, // so `plugins: []` still leaves `self.plugin = Some(_)`. - let plugin: NonNull = match self.plugin { - Some(p) => p, - None => { - let p = Plugin::create(global, bun_jsc::BunPluginTarget::Bun); - let p = NonNull::new(p) - .expect("JSBundlerPlugin__create returns a non-null protected JSCell"); - self.plugin = Some(p); - p - } - }; + let plugin: &mut OwnedPlugin = self + .plugin + .get_or_insert_with(|| OwnedPlugin::create(global, bun_jsc::BunPluginTarget::Bun)); let empty_object = JSValue::create_empty_object(global, 0); let mut iter = plugin_array.array_iterator(global)?; @@ -356,15 +335,8 @@ impl SplitBundlerOptions { } }; - // `Plugin` is an `opaque_ffi!` ZST — `opaque_mut` is the safe - // deref. Handle held live in `self.plugin` (protected JSCell). - let plugin_result = Plugin::opaque_mut(plugin.as_ptr()).add_plugin( - function, - empty_object, - JSValue::NULL, - false, - true, - )?; + let plugin_result = + plugin.add_plugin(function, empty_object, JSValue::NULL, false, true)?; if let Some(promise) = plugin_result.as_any_promise() { promise.set_handled(global.vm()); diff --git a/src/runtime/bake/mod.rs b/src/runtime/bake/mod.rs index 12136842c27d..d4287c30113f 100644 --- a/src/runtime/bake/mod.rs +++ b/src/runtime/bake/mod.rs @@ -11,6 +11,8 @@ use core::ptr::NonNull; use std::borrow::Cow; +use crate::api::js_bundler::OwnedPlugin; + // ─── Submodule bodies ──────────────────────────────────────────────────────── // `bake_body.rs` carries the Framework/UserOptions/BuildConfigSubset `from_js` // impls plus the `init_server_runtime`/`get_hmr_runtime` host fns. @@ -463,12 +465,32 @@ impl Framework { } } +/// The plugin cell every bundle of a `DevServer` runs against, and whether +/// the dev server is the one releasing it. +pub(crate) enum DevServerPlugin { + /// `framework.plugins` / `app.plugins`: created for this dev server by + /// `UserOptions::from_js` and released when the dev server drops. + Owned(OwnedPlugin), + /// The server's `[serve.static]` plugins, used when the app declares + /// none. The cell belongs to the server's `ServePlugins`, released by the + /// same server that owns this dev server; `DevServer::on_plugins_resolved` + /// only stores the pointer. + Borrowed(NonNull), +} + +impl DevServerPlugin { + pub(crate) fn as_non_null(&self) -> NonNull { + match self { + DevServerPlugin::Owned(plugin) => plugin.as_non_null(), + DevServerPlugin::Borrowed(plugin) => *plugin, + } + } +} + /// `bake.SplitBundlerOptions` — per-graph bundler config + shared plugin. #[derive(Default)] pub struct SplitBundlerOptions { - /// FFI: `jsc.API.JSBundler.Plugin` (`JSBundlerPlugin__create`); deinit - /// goes through the C++ side. See LIFETIMES.tsv. - pub(crate) plugin: Option>, + pub(crate) plugin: Option, pub(crate) client: BuildConfigSubset, pub(crate) server: BuildConfigSubset, pub(crate) ssr: BuildConfigSubset, @@ -561,9 +583,7 @@ impl From for BuildConfigSubset { impl From for SplitBundlerOptions { fn from(src: bake_body::SplitBundlerOptions) -> Self { Self { - // `bake_body::Plugin` and keystone `jsc::Plugin` both alias - // `crate::api::js_bundler::Plugin` — same nominal type, no cast. - plugin: src.plugin, + plugin: src.plugin.map(DevServerPlugin::Owned), client: src.client.into(), server: src.server.into(), ssr: src.ssr.into(), diff --git a/src/runtime/bake/production.rs b/src/runtime/bake/production.rs index 95aa242fdfa6..f88cbb369a86 100644 --- a/src/runtime/bake/production.rs +++ b/src/runtime/bake/production.rs @@ -630,7 +630,11 @@ fn build_with_vm(ctx: Context, cwd: &[u8], pt: &mut PerThread) -> crate::Result< framework: bundler_framework, client_transpiler: NonNull::new(client_ptr).expect("stack-owned transpiler"), ssr_transpiler: NonNull::new(ssr_ptr).expect("stack-owned transpiler"), - plugins: options.bundler_options.plugin, + plugins: options + .bundler_options + .plugin + .as_ref() + .map(|plugin| plugin.as_non_null()), }, &options.arena, Some(NonNull::from(&mut any_loop)), diff --git a/test/bake/app-plugins-release.test.ts b/test/bake/app-plugins-release.test.ts new file mode 100644 index 000000000000..a66ef76cea95 --- /dev/null +++ b/test/bake/app-plugins-release.test.ts @@ -0,0 +1,57 @@ +import { expect, test } from "bun:test"; +import { bunEnv, bunExe } from "harness"; +import path from "node:path"; + +// `app.plugins` / `framework.plugins` are held by one native BundlerPlugin cell +// that the dev server takes over from the parsed serve options. The dev server +// used to drop its handle to that cell without releasing it, so every dev +// server created with plugins kept its cell, and every closure the plugins had +// registered, rooted for the rest of the process; options rejected after the +// cell had been created leaked it the same way. The fixture runs one of these +// paths per process and reports how many cells survive a full GC afterwards. +// +// The cases run one at a time: each dev server needs a file watcher instance, +// which Linux hands out per user, and a process only gives it back on exit. +// The fixture waits for one when the machine is out of them (hence the +// timeouts), and the debug build's startup plus full collections are slow. +async function runCase(name: string) { + await using proc = Bun.spawn({ + cmd: [bunExe(), "app-plugins-release.ts", name], + cwd: path.join(import.meta.dir, "fixtures/app-plugins-release"), + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // stderr is not asserted: "init-fails" makes Bun print the framework file it + // could not resolve there. + expect(stdout, stderr).toStartWith("{"); + return { ...JSON.parse(stdout), exitCode }; +} + +test("a stopped dev server releases the cell holding its app plugins", async () => { + expect(await runCase("stopped-server")).toEqual({ + cellsWhileServing: 1, + devServersDeinitialized: 1, + leakedCells: 0, + exitCode: 0, + }); +}, 60_000); + +test("serve options rejected by a plugin's setup() release the cell", async () => { + expect(await runCase("setup-throws")).toEqual({ + error: "setup failed on purpose", + devServersDeinitialized: 0, + leakedCells: 0, + exitCode: 0, + }); +}, 60_000); + +test("a dev server that fails to initialize releases the cell", async () => { + expect(await runCase("init-fails")).toEqual({ + error: "Framework is missing required files!", + devServersDeinitialized: 1, + leakedCells: 0, + exitCode: 0, + }); +}, 60_000); diff --git a/test/bake/fixtures/app-plugins-release/app-plugins-release.ts b/test/bake/fixtures/app-plugins-release/app-plugins-release.ts new file mode 100644 index 000000000000..87da9923c269 --- /dev/null +++ b/test/bake/fixtures/app-plugins-release/app-plugins-release.ts @@ -0,0 +1,114 @@ +// Spawned by test/bake/app-plugins-release.test.ts, once per case, with this +// directory as cwd. +// +// `Bun.serve({ app: { plugins } })` creates one native BundlerPlugin cell (the +// JSC class name of JSBundlerPlugin) holding every setup()/onLoad/onResolve +// closure, and protects it from GC until whoever owns it releases it. Each case +// below makes the cell's owner go away in a different way and prints, as one +// JSON line, how many cells are still alive after a full GC: the number of +// cells that path leaked. Every case runs in its own process because a dev +// server's file watcher instance is only given back when the process exits. +import { getDevServerDeinitCount } from "bun:internal-for-testing"; +import { heapStats } from "bun:jsc"; + +function liveCells(): number { + return heapStats().objectTypeCounts.BundlerPlugin ?? 0; +} + +// A released cell is only unprotected: it still has to be collected, and a +// stale pointer to it on the native stack can keep it alive for a collection, +// so alternate collections with turns of the event loop. A released cell is +// normally gone after the first collection; a protected one survives all of +// them, which is what a leak looks like. +async function liveCellsAfterGC(expected: number): Promise { + for (let attempt = 0; attempt < 10 && liveCells() !== expected; attempt++) { + Bun.gc(true); + await new Promise(resolve => setTimeout(resolve, 0)); + } + return liveCells(); +} + +function serveOptions(setup: (build: Bun.PluginBuilder) => void, serverEntryPoint = "./server.ts") { + return { + port: 0, + development: true, + fetch: () => new Response("unused"), + app: { + framework: { + fileSystemRouterTypes: [{ root: "routes", style: "nextjs-pages", serverEntryPoint }], + }, + plugins: [{ name: "probe", setup }], + }, + } as Bun.Serve.Options; +} + +function registerOnLoad(build: Bun.PluginBuilder) { + build.onLoad({ filter: /\.probe$/ }, () => ({ contents: "", loader: "js" })); +} + +// Every dev server needs a file watcher instance (inotify on Linux), which is +// a per-user limit shared with every other process on the machine. When the +// machine is out of them, wait for one instead of failing on the spot. (Each +// attempt that fails this way parses the options, so it creates and rejects a +// cell of its own.) +async function serve(options: Bun.Serve.Options): Promise> { + const deadline = Date.now() + 30_000; + while (true) { + try { + return Bun.serve(options); + } catch (error) { + if (!(error as Error).message.startsWith("EMFILE") || Date.now() > deadline) throw error; + await Bun.sleep(100); + } + } +} + +async function serveError(options: Bun.Serve.Options): Promise { + try { + (await serve(options)).stop(true); + return "did not throw"; + } catch (error) { + return (error as Error).message; + } +} + +let report: Record; +switch (process.argv[2]) { + // The dev server takes the cell over from the parsed options and has to + // release it when the stopped server tears the dev server down. + case "stopped-server": { + const server = await serve(serveOptions(registerOnLoad)); + // The running dev server's cell is the one that must survive a collection. + const cellsWhileServing = await liveCellsAfterGC(1); + server.stop(true); + report = { cellsWhileServing }; + break; + } + // The cell already exists when a plugin's setup() makes Bun.serve() throw, + // so the rejected options have to release it. No dev server is involved. + case "setup-throws": { + const error = await serveError( + serveOptions(() => { + throw new Error("setup failed on purpose"); + }), + ); + report = { error }; + break; + } + // The dev server has already taken the cell over when it fails to resolve + // the framework's files, so the dev server that never finished initializing + // has to release it as it is torn down. + case "init-fails": { + const error = await serveError(serveOptions(registerOnLoad, "./does-not-exist.ts")); + report = { error }; + break; + } + default: + throw new Error(`unknown case ${JSON.stringify(process.argv[2])}`); +} + +report.leakedCells = await liveCellsAfterGC(0); +// Read after the collections above gave a stopped server its turns of the +// event loop to tear the dev server down. +report.devServersDeinitialized = getDevServerDeinitCount(); +console.log(JSON.stringify(report)); diff --git a/test/bake/fixtures/app-plugins-release/server.ts b/test/bake/fixtures/app-plugins-release/server.ts new file mode 100644 index 000000000000..e134fa8c1731 --- /dev/null +++ b/test/bake/fixtures/app-plugins-release/server.ts @@ -0,0 +1,5 @@ +// Framework server entry point for app-plugins-release.ts. Never bundled: the +// probe starts and stops dev servers without ever requesting a route. +export function render() { + return new Response("unused"); +}