bake: release the app plugins cell when the dev server is torn down - #37837
bake: release the app plugins cell when the dev server is torn down#37837robobun wants to merge 1 commit into
Conversation
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.
|
Updated 12:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit bcb51e0 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 37837That installs a local version of the PR into your bun-37837 --bun |
|
Warning Review limit reached
Next review available in: 8 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 (8)
Comment |
|
Status: fix pushed, waiting on CI. Reproduced on 1.4.0-canary with the fixture in this PR ( |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks GC-root ownership (protect/unprotect balance via a new RAII type) and reorders the MaybeUninit<DevServer> field-write sequence in init, a human look would still be worthwhile.
What was reviewed:
OwnedPluginDrop pairsPlugin::create's protect withPlugin::destroy's tombstone+unprotect;DerefMutgoes throughopaque_mut.DevServer::initreorder:bundler_optionsstays in the localoptionsacross the fallibleFileSystem::init/Watcher::init/init_transpilercalls, so an earlyErrdrops it (releasing the cell) instead of leaking it in the never-assume_init'd box; nothing else reads(*p).bundler_optionsin that window.on_plugins_resolved'sdebug_assert!(plugin.is_none())matchesensure_route_is_bundled, which only takes the ask-server path when the slot isNone, so aBorrowednever overwrites anOwned.From<bake_body::SplitBundlerOptions>moves theOwnedPlugin(no double-destroy from the removedDrop for UserOptions).
Extended reasoning...
Overview
The PR fixes a leak of the JSBundlerPlugin JSC cell held by Bun.serve({ app: { plugins } }) dev servers. It introduces OwnedPlugin (RAII wrapper whose Drop calls Plugin::destroy, i.e. tombstone + unprotect()) in JSBundler.rs, changes bake_body::SplitBundlerOptions.plugin from Option<NonNull<Plugin>> to Option<OwnedPlugin>, adds bake::DevServerPlugin { Owned, Borrowed } for the dev-server slot (which can also hold a pointer to the server's ServePlugins cell it must not free), and reorders the w!(bundler_options, ...) write in DevServer::init to after the fallible watcher/transpiler steps so an early Err drops the local (and its OwnedPlugin) instead of stranding it in the never-assume_init()'d box. The manual Drop for UserOptions is removed. Three new leak-regression tests cover stopped-server, setup-throws, and init-fails.
Security risks
None in the traditional sense; the risk here is memory safety — a mispaired protect/unprotect is either a leak or a UAF. The change moves toward RAII, which is the right direction. Plugin::destroy tombstones before unprotecting, so late JS-side deliveries are swallowed (matching every other caller). The Borrowed arm carries a raw NonNull<Plugin> owned by ServePlugins, which the same server releases; the dev server is dropped by the server, so the borrow does not dangle.
Level of scrutiny
High. This is native memory-management code intersecting JSC GC roots and a hand-rolled MaybeUninit<DevServer> per-field init. The reorder is subtle: it relies on the documented property that pre-assume_init() writes into the box are never dropped on Err, so ownership must stay in the local until after the last fallible step that reads it. I verified nothing reads (*p).bundler_options between the old and new write positions (only &options.bundler_options is read), and that options — with framework already partially moved out — still drops bundler_options on the earlier FileSystem::init/Watcher::init error paths.
Other factors
The tests are well-constructed (per-process fixture, heapStats().objectTypeCounts.BundlerPlugin after bounded GC polling, cross-checked against getDevServerDeinitCount() and error messages so each path is proven taken). The PR description explicitly demonstrates USE_SYSTEM_BUN=1 failing on all three cases. The debug_assert! in on_plugins_resolved is justified by ensure_route_is_bundled at DevServer.rs:1976 only reaching the ask-server path when plugin.is_none(). The OwnedPlugin type is intentionally identical to #37805's for a clean merge. Given the subtlety of the MaybeUninit interaction and the new Owned/Borrowed ownership split, this warrants a maintainer review even though no defects were found.
Problem
Bun.serve({ app: { framework, plugins } })leaks its native plugin cell (BundlerPlugininheapStats()) after teardown, and with it everysetup()/onLoad/onResolveclosure registered on it. Stopping four such servers leaves four cells on 1.4.0-canary.Bun.serve()throws after creating it: a plugin'ssetup()throwing, or dev server init failing (for exampleEMFILE while initializing file watcher).Fix
Dropreleases it, so whichever struct holds it when it goes away (the parsed options or the dev server) releases it exactly once.Ownedfor the app's own cell,Borrowedfor the server's[serve.static]plugins, which the server releases. The borrowed arm is only filled when the app declared no plugins, so it never displaces an owned cell.Background
app.pluginsandframework.pluginsare collected into one nativeJSBundlerPluginobject (the "cell") that every bundle of the dev server consults. Bunprotect()s it from JSC's GC, so it and everything it references live until something callsdestroy, which tombstones and unprotects it.UserOptionsstruct. Creating a dev server moves the bundler options out of it, so cleanup left inUserOptionsno longer sees them.[serve.static]plugins, a cell the server owns and releases, so the slot has to tell an owned cell from a borrowed one.DevServer::initfills aMaybeUninitbox field by field. On an error before the box is marked initialized, fields already written are not dropped, so anything that must be released on failure has to stay outside the box until the fallible steps are done.Original description
What
The POSIX ready-poll dispatch chain held the poll being dispatched as a reference for the whole time its owner ran (line numbers on main):
Same receiver-shape class as #37681, #37778 and #37803. No crash is known from it.
Why it is wrong
The
&mut selfofon_kqueue_event/on_epoll_eventand ofon_update, and the&mut FilePollargument ofon_dns_poll, are reference arguments: protected until those calls return, and the owner runs inside them. The owner does not use those references. It reaches the same slot through the pointer it keeps itself (FilePollRef,PollerPosix::Fd, the resolver's poll map), on the normal path, not only on teardown:FilePollRef::register_with_fdpassesOneShotFlag::Dispatch), soon_updatesetsNeedsRearmand the owner's re-arm during the dispatch reads and clears it again (register_with_fd_impl,unregister_with_fd_impl) through its own pointer. The flag exists to be passed from this frame to the owner's call underneath it.PollOrFd::close_impl->FilePollRef::deinit_force_unregister), process exit (Process::on_exit->close->PollerPosix), and c-ares closing a socket, which happens inside theChannel::processcall thaton_dns_pollmakes while its&mut FilePollargument is live (on_dns_socket_statealso re-registers the same poll through the map on every read/write transition).Those are reads and writes through a pointer foreign to the protected references further up the stack, which both aliasing models reject (the protector rule the sibling PRs quote; rustc also marks the arguments
noalias).FilePollRef::innerandPollerPosix::fd_poll_mutboth document the&mut FilePollthey hand out as the only live reference to the slot; during a dispatch that was not true. It is latent today because nothing on the chain reads the slot after the owner returns, so there is nothing observable to assert on.Fix
The chain carries the pointer
Pollabledecodes all the way down, the shape__bun_run_file_poll(poll: *mut FilePoll)andPosixBufferedReader::on_poll(this: *mut Self)already have:The entry point reads
IgnoreUpdatesthrough a statement-scoped access and passes the pointer on;update_flags, theNeedsRearmupdate, the kqueuesyslog!and generation-numberdebug_assert!, and__bun_run_file_poll'sowner/hupreads become statement-scoped accesses; the DNS arm passespollitself, andon_dns_pollreadsfd/is_readable()/is_writable()into locals beforeprocessand does not touch the poll afterwards. Same operations in the same order, so no behaviour change.The contract the chain relies on is only that the slot is live when each frame is entered: nothing on the chain reads it after the owner returns, so the comments say that rather than anything about the slot staying allocated for the duration of the call (deferred frees run from the after-tick callback, which a nested tick inside an owner's JS would also run).
Overlap with #37803: that PR converts the owner's side of the same contract (
FilePoll::deinit*and its callers) and, to get there, also convertson_dns_polland the__bun_run_file_pollreads; it leaves the four frames in posix_event_loop.rs to this PR. The two PRs therefore both touch__bun_run_file_pollandon_dns_poll, and a test merge conflicts in exactly those two functions (posix_event_loop.rs auto-merges). That is deliberate: whichever lands second rebases and takes either side of each conflict, and the lint below passes with both (checked with #37803's versions of dispatch.rs and dns.rs dropped onto this branch). An earlier revision of this PR instead allowlisted those two frames in the lint; since #37803 does not touch the lint file, that would have gone red on main after the second merge with nothing forcing anyone to notice, so the overlap is now textual.The five src/io/PipeReader.rs comments about a protector "pre-existing on the parent chain" describe the reader's own
&mut selfentry points (close,finish,start,watch, and the Windowson_read), which parents call directly; converting this chain does not change them.Test
test/internal/source-lints/file-poll-dispatch-raw.test.tslocates every frame of the chain by name (Bun__internal_dispatch_ready_poll,on_kqueue_event,on_epoll_event,on_update,__bun_run_file_pollincluding itsextern "Rust"declaration,on_dns_poll) and checks:*mut FilePoll(aselfreceiver on the FilePoll methods counts as by-reference unless it is a raw receiver), and no parameter takes it as&FilePoll/&mut FilePoll;let x: *mut FilePollbinding). This is what keeps the chain closed: a helper interposed on any edge, whether it takes&mut FilePollor is a&mut selfmethod called as(*this).helper(), and a&mut *thisargument (which coerces and would compile) are all reported;letbinding is a&[mut] FilePollor a fn-long&mut *../unsafe { ..as_mut().. }reborrow.Fixtures cover the main spellings of all six frames (19 findings), the converted spellings (none), and six mutations of the converted chain (interposed
&mut FilePollhelper, interposed&mut selfhelper,&mut *thisargument on the epoll edge,as_mut()binding, untyped entry binding,NonNullparameter), with the expected line numbers written out by hand. On main it reports 13 lines across the three files (the six frames above); on this branch it is clean, and it also fails on the intermediate state where only posix_event_loop.rs is converted. The kqueue function is#[cfg]'d to macOS/FreeBSD, so the lint is also what pins that path from the Linux lanes.A behavioural fail-before test is not possible for the reason given above (the old code never touched the references after the owner ran).
Verification
cargo checkandcargo clippyforbun_ioon x86_64-unknown-linux-gnu and aarch64-apple-darwin (the kqueue function, including thesyslog!/debug_assert!spellings),cargo checkon x86_64-unknown-freebsd;cargo clippy -p bun_runtime;cargo fmt --check;test/internal/source-lints/as a directory (88 pass).test/js/bun/spawn/spawn.test.ts,spawn-many-teardown.test.ts,spawn-pipe-stale-fd-unregister.test.ts(pipe EOF and process exit deinit the poll from inside the dispatch),test/js/node/dns/dns-tcp-bidirectional-poll.test.ts,dns-lookup-keepalive.test.ts,dns-resolver-concurrent-timeout.test.ts(re-registration and deinit of the c-ares poll from insideChannel::process, i.e. theon_dns_pollpath),test/js/node/process/process-memory-pressure.test.ts; earlier revision of the same code also ranspawn-streaming-stdin.test.ts,spawn-stdin-destroy.test.tsand the shell suitespipeline_stack,epipe,shell-blocking-pipe,shell-pipe-read-fault,shell-write-fault.test/js/node/child_process/passes except for the$SHELLtest (the envbun bdruns tests with has noSHELL) and the 20-debug-child GC test hitting its 5 s budget on this machine (its fixture printsOKwhen run directly);spawn-pipe-leak.test.ts(750 debug children per test) hits its 30 s budget here the same way. io: return a FilePoll to its store through the owner's pointer, not a &mut receiver #37803 reports the same local failures.