bundler: free the ServerComponentParseTask after it generates its file - #38004
bundler: free the ServerComponentParseTask after it generates its file#38004robobun wants to merge 4 commits into
Conversation
enqueue_server_component_generated_file boxes a ServerComponentParseTask per generated server-component file and hands it to the worker pool, but nothing ever took the box back: the pool callback borrowed it, posted the parse Result, and returned, leaking the task (and whatever its Data owned) once per "use client" module per bundle. The pool callback now reclaims the box and drops it once the generated AST has been built. The two places where the generated AST pointed into the task's Data (the export-name symbols and string literals of a reference proxy, and the import path of a client entry wrapper) now copy those bytes into the worker's AST arena, which is where StoreStr-backed names are required to live anyway, so nothing refers to the task after it is freed.
|
Status: reproduced and fixed; current head is ec7b267. Reproduced with the debug (ASAN) build: a bake dev server using a |
|
Updated 1:10 PM PT - Aug 13th, 2026
✅ @robobun, your commit ec7b26792d1659b62a712e40458c0d83b09ae8f9 passed in 🧪 To try this PR locally: bunx bun-pr 38004That installs a local version of the PR into your bun-38004 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Walkthrough
ChangesServer component task lifetime
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Nothing actionable came out of the automated review. I added two scoping notes to the description: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes unsafe cross-thread ownership (reclaiming the boxed task via heap::take on a worker thread) and repoints lifetime-erased StoreStr slices in generated ASTs, a human look would still be worthwhile.
What was reviewed:
- Thread-pool contract:
ThreadPool.rs:1232never touches the task after the callback returns, matching theOwnedTask::__callbackpattern the fix mirrors. - Provenance: enqueue projects the intrusive field via
addr_of_mut!((*task).task)on the raw box pointer, sofrom_field_ptr!→heap::takerecovers whole-allocation provenance. - Audited every read of
task.datain both generators for slices that outlive the task; the two new arena copies (export keys, entry-wrapper path) cover them, andE::String::init(key)correctly reuses the arena copy instead of double-copying. task.sourceis moved out beforedrop(task), andother_source.path.prettywas already arena-copied; framework config strings live onctx, not the task.
Extended reasoning...
Overview
Fixes a per-generated-file leak of Box<ServerComponentParseTask> in the bundler's server-components path. task_callback_wrap now reclaims the box with heap::take (instead of borrowing it) and drops it after building the AST. Two places where the generated AST previously borrowed slices out of the task's Data — the reference-proxy export names in generate_client_reference_proxy and the import-record path in generate_client_entry_wrapper — now copy into the worker's bump arena, since the task no longer outlives the bundle. Comments at the enqueue site and callback are updated to describe the actual ownership hand-off. A new LSan-gated test in test/bake/deinitialization.test.ts bundles a "use client" module under separateSSRGraph and asserts both the rendered output (catches UAF in the export names) and no LeakSanitizer report.
Security risks
None. This is internal bundler memory management with no user-controlled input reaching the changed code paths beyond what already flowed through them.
Level of scrutiny
High. This is unsafe Rust touching exactly the areas REVIEW.md flags as most-blocked: box reclamation across an FFI-style callback boundary, cross-thread drop (task built on the bundle thread, freed on a worker), and lifetime-erased StoreStr storage where a missed borrow becomes a use-after-free that ASAN may or may not catch depending on allocator reuse. The reasoning in the PR is careful and I verified its load-bearing claims against the source (thread-pool worker loop, OwnedTask::__callback precedent, new_symbol storing the name raw, heap::take = Box::from_raw, the addr_of_mut! provenance path), but this is precisely the class of change where a second pair of eyes on "is there any other slice that escapes the task" is warranted rather than auto-approved.
Other factors
The fix follows the established OwnedTask free-inside-callback pattern from work_pool.rs rather than inventing a new one. The test asserts stdout equals "200 default Alpha Beta\n", which exercises the arena-copied export names through printing and rendering — a UAF there would corrupt the output. The E::String::init(key) change from alloc_slice_copy(key) to bare key is a correct dedup, not a regression: key is already the arena copy at that point. Existing test/bake/dev/bundle.test.ts (which rebuilds proxies across HMR) reportedly still passes. No prior reviews on the PR.
|
For whoever takes the human look the review asks for, the "does anything else escape the task" audit, so it does not have to be redone from scratch. Reads of the task after it is boxed are all in
Nothing else on the task is read, and the thread pool does not touch the intrusive |
ReferenceProxy carried a clone of the client module's Source and of its NamedExports map. The map clone is AstAlloc-backed, and on both paths that produce these tasks (the dev server and `bun build --app`) no AST allocation state is installed when on_parse_task_complete runs, so the clone landed in AstAlloc's never-freed global fallback; freeing the task did not reclaim it. The generator also only ever read the module's path, its source index, and the export names. The task now carries exactly those: the path and index by value and the names copied into the bundle arena by the bundle thread, which is freed with the rest of the bundle. Nothing in the task needs copying on the worker any more, so the generator-side arena copies and the large_enum_variant allowance go away, and the test also checks the path the proxy hands to registerClientReference.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and the automated bug hunt found no issues. Because it changes ownership and lifetime handling in unsafe Rust — reclaiming the boxed task inside the thread-pool callback, erasing bundle-arena lifetimes to 'static for the new ReferenceProxy payload, and relying on the pool's run-once contract for the heap::take — a human look is still warranted.
What was reviewed:
task_callback_wrap'sheap::takepairs with theheap::into_rawat the enqueue site, and the&raw mut (*task).taskprojection preserves whole-allocation provenance for it.copy_export_names_for_reference_proxyfollows the samedetach_lifetime_refarena-erasure convention already used at bundle_v2.rs:1582 and :3341.- Nothing in the posted
parse_task::Resultborrows from the dropped task:sourceis moved out,ctxisCopy, and the generated AST's strings now point at bundle-arena or worker-arena bytes. - The comment-cop flags appear to target the new ownership doc comments and SAFETY blocks rather than actual workarounds.
Extended reasoning...
Overview
This PR fixes a per-"use client"-module leak of ServerComponentParseTask in the bake/server-components bundler path. Two coupled changes: (1) the thread-pool callback now takes back the Box<ServerComponentParseTask> via bun_core::heap::take (paired with the existing heap::into_raw at enqueue) and drops it after generation, and (2) the task's Data payload is restructured so nothing the generated AST points at lives in the task — ReferenceProxy now carries the client module's FsPath, source index, and bundle-arena copies of its export names (via a new copy_export_names_for_reference_proxy helper on BundleV2), replacing the cloned Source and AstAlloc-backed NamedExports map. ClientEntryWrapper::path becomes a bundle-arena &'static [u8]. In on_parse_task_complete, the pre-move named_exports.clone() snapshot is replaced by reading from graph.ast after the AST is moved in. An ASAN-only leak regression test is added.
Security risks
None. This is internal bundler memory management with no user-input parsing, auth, crypto, or network surface.
Level of scrutiny
High. This is native unsafe Rust in a hot bundler path, touching exactly the categories REVIEW.md calls out as most-blocked: cross-thread ownership transfer (box handed to pool, reclaimed in callback), 'static lifetime erasure of arena-backed slices, and pointer-provenance-sensitive projection (from_field_ptr! → heap::take). Getting any of these wrong is a use-after-free or double-free rather than a functional bug. The reasoning in the PR description and the author's escape-analysis comment is thorough and the arena-erasure follows the established interned_slice/detach_lifetime_ref convention in the same file, but the invariants are non-local (thread pool run-once contract, bundle-arena lifetime spanning the worker task, StoreStr's arena-owned contract) and merit a maintainer's confirmation.
Other factors
The new test asserts exact rendered output (path + all three export names in order) plus absence of a LeakSanitizer report and exitCode === 0, and is skipIf(!isASAN). The existing test/bake/dev/bundle.test.ts suite (which regenerates proxies across HMR rebuilds) was reported passing. The comment-cop bot left eight "paragraph-long comment" flags; on inspection these land on doc comments and SAFETY blocks documenting ownership — the kind of non-local-lifetime commentary REVIEW.md requires — not on workaround justifications, so they read as linter noise rather than actionable defects, but the author may want to trim or dismiss them. Given the memory-safety surface, deferring to a human reviewer rather than auto-approving.
|
Update since the reviews above: the second commit (b82c157) changes what the task carries, and 2720ab1 only trims comments. Self-review of the first revision found that freeing the box was not enough. The comment-cop threads are resolved; its rerun on 2720ab1 reports no remaining groups (it cannot resolve threads itself, which is tracked separately). |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It's a cross-thread memory-ownership change in the bundler with several unsafe blocks and erased-'static arena lifetimes, so a human look is still worthwhile.
What was reviewed:
heap::takeintask_callback_wrapreclaims the same allocationenqueue_server_component_generated_fileleaked; the pool's one-shot contract matchesOwnedTask::__callback.- Nothing in the posted
Successborrows the dropped task:sourceismem::taken,client_path/export_namesare bundle-arena copies, andE::String::init(key)/new_symbol(.., key)storeStoreStrs over those arena bytes. copy_export_names_for_reference_proxyallocates on the bundle thread fromgraph.astafter the AST is moved in;FsPath<'static>isCopyand its slices point at arena bytes, notinput_filesvector storage.ClientEntryWrapperhas no producer insrc/, so theBox<[u8]>→&'static [u8]field change has no callers to break.
Extended reasoning...
Overview
The PR fixes a leak of ServerComponentParseTask in the bake/server-components bundler path. Three files touched: src/bundler/ServerComponentParseTask.rs (reclaim the boxed task in the thread-pool callback and drop it after generation; restructure ReferenceProxy/ClientEntryWrapper to hold bundle-arena slices instead of owned clones), src/bundler/bundle_v2.rs (new copy_export_names_for_reference_proxy helper; enqueue site now builds the proxy payload from graph.ast/graph.input_files after the AST is moved in, dropping the pre-move named_exports.clone()), and a new ASAN-gated leak test in test/bake/deinitialization.test.ts.
Security risks
None identified. This is internal bundler memory management with no user-input parsing, auth, or crypto surface.
Level of scrutiny
High. This is native memory-safety work in the bundler — the most-blocked review category — with new unsafe (heap::take on a field-recovered pointer, detach_lifetime_ref on the bundle arena), cross-thread ownership transfer, and 'static-erased slices whose actual lifetimes are the bundle arena's. The PR description's argument is careful and I could not find a hole in it, but correctness rests on several non-local invariants (thread-pool one-shot execution, bundle arena outliving every AST that stores the export-name slices, StoreStr's arena-owned contract, bundle-arena allocation only on the bundle thread) that a maintainer who owns this code should confirm.
Other factors
- I traced every read of the task in
task_callbackand both generators against the new payload shape; nothing surviving thedrop(task)points into it.client_pathisbun_paths::fs::Path<'static>(Copy, holds&'static [u8]arena slices), so copying it out of&graph.input_files[..]beforeenqueue_server_component_generated_filetakes&mut selfis sound and does not alias vector-backed storage. - Grepped for
ClientEntryWrapperacrosssrc/: no constructor exists outside theDefaultplaceholder, so theBox<[u8]>→&'static [u8]change is unreachable today. - The test follows harness conventions (
tempDir,bunEnvspread, concurrent stdout/stderr/exited drain, output asserted before exit code,skipIf(!isASAN)) and asserts the proxy's observable contract (path + export names in export order) in addition to the LSan check. - The eight
comment-copbot comments on the timeline predate/target multi-line comments; commit2720ab17already trimmed the ones this PR added to one line each. The remaining multi-line blocks are pre-existing SAFETY/CONCURRENCY annotations.
|
ec7b267 extends the test rather than changing the fix: the route now also imports a |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it reworks ownership and cross-thread lifetimes in unsafe Rust (reclaiming the task box inside the pool callback, and handing bundle-arena slices with erased 'static lifetimes to worker threads), a human look at the arena/provenance invariants would still be worthwhile — the author's escape audit in the thread is a good starting point.
What was reviewed:
heap::takeintask_callback_wrapmatches theOwnedTask::__callbackcontract inwork_pool.rs(pool fires each task exactly once, never touches it after).copy_export_names_for_reference_proxyfollows the existingdetach_lifetime_ref(self.arena())pattern used elsewhere inbundle_v2.rs; readsgraph.astafter the AST is moved in, so no stale snapshot.- Confirmed
Data::ClientEntryWrapperhas no producer in-tree, so itspath: &'static [u8]change is not exercised but also cannot regress anything. - Test drains stdout/stderr/exited concurrently, is
skipIf(!isASAN), and asserts the proxy's rendered path/export names in addition to the LSan check.
Extended reasoning...
Overview
This PR fixes a per-client-module leak of ServerComponentParseTask in the bake bundler's server-components path. It touches src/bundler/ServerComponentParseTask.rs (task callback now reclaims and drops the boxed task; ReferenceProxy/ClientEntryWrapper payloads reshaped to bundle-arena slices), src/bundler/bundle_v2.rs (new copy_export_names_for_reference_proxy helper; on_parse_task_complete reads named exports from graph.ast after the move instead of cloning them before it), and adds an ASAN-gated regression test to test/bake/deinitialization.test.ts.
Security risks
None. No user-facing input parsing, auth, or network surface is touched — this is internal bundler task lifecycle. The relevant risk class is memory safety (UAF if the box is reclaimed while something still references it, or if arena slices outlive the arena), not security.
Level of scrutiny
High. Per the repo's own review guidance, native memory safety is the most-blocked category. The change adds an unsafe { heap::take(...) } inside a thread-pool callback, an unsafe { detach_lifetime_ref } to hand bundle-arena bytes across threads as &'static, and changes what data crosses the bundle-thread → worker-thread boundary. Each piece follows an established in-tree pattern (OwnedTask::__callback for the box reclaim; multiple existing detach_lifetime_ref(self.arena()) sites in bundle_v2.rs for the arena erasure), and the reasoning in the PR description is thorough, but the invariants ("bundle arena outlives every consumer of these slices", "pool never touches the task after the callback", provenance through from_field_ptr! for Box::from_raw) are exactly the kind a maintainer should sign off on rather than an automated reviewer.
Other factors
- The bug-hunting pass found nothing. I spot-checked that
items_named_exports()is an existing accessor used the same way elsewhere, thatalloc_slice_fill_iteris the standard arena API, and thatClientEntryWrapperhas no producer (so itsBox<[u8]>→&'static [u8]change is inert). - The test follows harness conventions (
tempDir,bunEnvspread, concurrent pipe drain, output asserted before exit code) and asserts functional output — not just "no LeakSanitizer" — so it would catch a UAF that corrupts the export names, and it covers the empty-exports case. - The comment-cop threads on the PR are all resolved; the current revision's comments are one-liners.
- The author explicitly left an escape audit "for whoever takes the human look", which itself signals this was written expecting human sign-off.
|
Reached the same bug independently (handed over from the #37997 work) and am standing down in favour of this PR. Leaving the branch here in case any of it is useful: main...farm/d654bdb9/free-server-component-parse-task
|
|
Note for when this lands: |
1 participant
Problem
"use client"module when the framework uses a separate SSR graph, in dev and inbun build --app) leaks itsServerComponentParseTask. LeakSanitizer on a bake dev server that bundles one client module:BundleV2::enqueue_server_component_generated_fileboxes the task withheap::into_rawand schedules it, and nothing ever takes the box back.task_callback_wrap(src/bundler/ServerComponentParseTask.rs) only borrowed it through the intrusivetaskfield, andparse_worker::on_completefrees theparse_task::Result, not the task. The comment at the enqueue site claimedon_completefrees it.ReferenceProxyheld a clone of the client module'sSourceand of itsNamedExportsmap (bundle_v2.rs,named_exports.clone()inon_parse_task_complete). The map clone isAstAlloc-backed, and on both paths that produce these tasks no AST allocation state is installed whenon_parse_task_completeruns, so it went toAstAlloc's never-freed global fallback (invisible to LSan, one more per"use client"module on every dev-server rebuild). The Zig original shared the AST's map by value; the port deep-cloned it.Fix
task_callback_wrapreclaims theBox<ServerComponentParseTask>withheap::takeand drops it once the generated AST has been built, before theResultis posted. The pool runs a task exactly once and does not touch it after the callback returns (ThreadPool.rsworker loop), the same contractOwnedTask::__callbackandWorkPool::gorely on to free their tasks inside the callback.ReferenceProxynow carries exactly what the generator reads: the client module'sPathand source index by value, and its export names copied into the bundle arena by the bundle thread (copy_export_names_for_reference_proxy, read fromgraph.astafter the AST is moved in, so the pre-move snapshot and the clones are gone). The arena is freed with the bundle, so a dev server reclaims them on every rebuild;ClientEntryWrapper::pathbecomes a bundle-lifetime slice for the same reason.StoreStr's documented "arena-owned" contract; previously the export-name symbols pointed at key boxes inside the task's map and only survived because the task leaked), and thelarge_enum_variantallowance is unnecessary.Data::ClientEntryWrapperhas no producer in the tree, so it is covered but not exercised;client_source_indexis only read by production builds, which a dependency-free fixture cannot currently run (custom frameworks panic later inbun build --app, reported separately).test/bake/deinitialization.test.ts, new case: bake dev server with aseparateSSRGraphframework and two"use client"modules (one with three exports, one with none) underdetect_leaks=1; asserts the proxy passed the module's path and every export name toregisterClientReference, that the export-less module's side effect did not run on the server (its proxy did), no LeakSanitizer report, exit 0. Fails on main with the report above, passes here.skipIf(!isASAN).bun bd test test/bake/dev/bundle.test.ts(includes theseparateSSRGraphclient-component demotion test, which regenerates proxies across HMR rebuilds): 21 pass.Background
separateSSRGraph: when the server graph imports a"use client"module, the module itself is bundled for the browser and SSR graphs, and the server graph gets a generated "reference proxy" whose exports areregisterClientReference(...)calls, one per export of the client module. The proxy is built withAstBuilderon the bundler's thread pool by aServerComponentParseTask, which posts a regularparse_task::Resultback like aParseTaskwould.ParseTask, which lives in the bundle arena and is bulk-freed with it,ServerComponentParseTaskis an individualBoxwhose only owner is the pool task it embeds, so something has to take it back after it runs.BundleV2::arena(),graph.heap): a per-bundle mimalloc heap, allocated from only on the bundle thread and destroyed when the bundle finishes (per rebuild in the dev server). Slices in it are stored with an erased'staticlifetime throughout bundle_v2.rs (interned_slice,path_as_static);copy_export_names_for_reference_proxyuses the same convention.AstAlloc: the allocator behindNamedExports. It allocates from the thread's installed AST allocation state, or from global mimalloc when none is installed, and itsdeallocateis a no-op, so dropping anAstAlloc-backed container frees nothing; only an installed state's owner can reclaim it.Bun.build()installs one for the whole bundle; the bake dev server andbun build --app, the only producers of these tasks, do not have one installed duringon_parse_task_complete.StoreStr(Symbol::original_name,EString) stores a raw(ptr, len)and documents that the bytes must be arena-owned or'static; the bundle arena satisfies that.Earlier revision
The first revision only reclaimed the box in the callback and copied the export names and entry-wrapper path into the worker arena inside the generators, leaving the
SourceandNamedExportsclones in the task. Review pointed out that the map clone's storage is never reclaimed on the bake paths, so the task's payload was still leaking (just not where LSan looks); the second commit replaces the payload instead.[review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file