bundler: run a Bun.build on the bundle thread through the task pointer, not a build-long &mut self - #37740
bundler: run a Bun.build on the bundle thread through the task pointer, not a build-long &mut self#37740robobun wants to merge 5 commits into
Conversation
…r, not &mut self The bundle thread used to hold the dequeued JSBundleCompletionTask as &mut for the whole build (thread_main reborrowed it, and every CompletionStruct method took &mut self, init_and_run's call spanning the entire bundle), while the JS thread still wrote to the task after enqueuing it (poll_ref, promise, html_build_task, started_at_ns) and can cancel it at any point (stop_for_vm_teardown, HTMLBundle State::deinit), and while the bundler wrote the task's log through the pointer create_and_configure_transpiler took out of an earlier &mut self. Each of those is a foreign access into memory a protected reference covers. CompletionStruct now takes the task as this: *mut Self in every method and the impl projects the fields it uses; thread_main and generate_in_new_thread carry the raw pointer. The trait loses the methods only the impl itself called (configure_bundler becomes a free function over the config, plugins/file_map/as_js_bundle_completion_task are inlined) and set_transpiler together with the write-only transpiler field it set. The dispatch vtable thunks project fields too instead of forming a & to the whole task. create_and_schedule_completion_task takes a Deliver (the Bun.build promise or the HTML route) and fills the task in before the enqueue, which is now its last statement; the keep-alive ref moves before it as well. promise, html_build_task and started_at_ns become private so JSBundler::build and HTMLBundle::schedule_bundle cannot write them afterwards. A source lint pins both shapes: no self receivers on CompletionStruct, no reborrow of the dequeued task in BundleThread.rs, nothing touching the task after the enqueue, and the per-owner fields private.
|
Updated 1:28 PM PT - Aug 12th, 2026
❌ @robobun, your commit fb842a0 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 37740That installs a local version of the PR into your bun-37740 --bun |
|
Status: ready for review (head fb842a0, an empty commit on top of 1fadd08); needs a maintainer. Reproduced structurally: the Miri reduction in the description fails on the three shapes Review: Overlaps with #37723 on |
|
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)
WalkthroughChangesThe bundle completion flow now supports promise and HTML-route delivery targets. Task lifecycle callbacks use raw pointers across queue processing, bundle execution, result handoff, and cleanup. JavaScript and HTML callers use the updated scheduler API. Source-lint tests enforce the handoff rules. Bundle completion handoff
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bundler/BundleThread.rs (1)
273-320: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftThe
?on Line 283 skipsast_memory_store.pop()and the teardown at Lines 342-347.
generate_in_new_threadcallsast_memory_store.push()on Line 279 and pairs it withpop()on Line 322.C::create_and_configure_transpiler(completion, bump)?on Line 283 returns early onErrand bypasses bothpop()and thedrop_in_placeblock on Lines 342-347.Consequences on that path:
- The AST-allocator thread-local stays pushed on the bundle thread. That thread is process-lifetime and serves every later build.
ASTMemoryAllocatoris not dropped, so the embeddedmi_heapleaks. This is the exact leak the comment on Lines 324-334 describes.heapstill drops, so the pushed thread-local now refers to reclaimed arena memory.The task itself is still handed back, because
thread_mainrunsset_result(Err)andcomplete_on_bundle_threadon Lines 242-243. So the build fails cleanly from the caller's view, but the bundle thread is left in a corrupted state.Move the fallible section into an inner function or a scope guard so
pop()and the teardown run on every path.The coding guidelines require: "Pair every resource acquisition with release at the acquisition site, including all early-return, error, success, and lifecycle paths." and "Every error, abort, and timeout path must complete the operation: ... mirror success-path cleanup."
🛠️ Sketch: run the fallible section inside a closure so teardown always runs
let bump = &heap; let ast_memory_store: &mut bun_ast::ASTMemoryAllocator = bump.alloc(bun_ast::ASTMemoryAllocator::new(bump)); ast_memory_store.reset(); ast_memory_store.push(); - // Allocate + configure folded — see `create_and_configure_transpiler` doc. - // SAFETY: fn contract. - let transpiler = unsafe { C::create_and_configure_transpiler(completion, bump)? }; + // Allocate + configure folded — see `create_and_configure_transpiler` doc. + // SAFETY: fn contract. + let transpiler = match unsafe { C::create_and_configure_transpiler(completion, bump) } { + Ok(t) => t, + Err(err) => { + // Mirror the success-path teardown before returning. + ast_memory_store.pop(); + // SAFETY: unique `bump.alloc` slot; nothing else references it. + unsafe { + core::ptr::drop_in_place( + std::ptr::from_mut::<bun_ast::ASTMemoryAllocator>(ast_memory_store), + ); + } + return Err(err); + } + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bundler/BundleThread.rs` around lines 273 - 320, Update generate_in_new_thread so every error path after ast_memory_store.push() performs the matching pop() and ASTMemoryAllocator teardown before returning. Encapsulate the fallible transpiler creation and run flow in an inner scope or scope guard, ensuring cleanup executes for both create_and_configure_transpiler errors and successful runs while preserving existing log handling and completion behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/api/js_bundle_completion_task.rs`:
- Around line 1109-1301: Update the doc comment for configure_bundler to
explicitly state that its standalone-HTML branch clears config.compile; preserve
the existing implementation and make clear that the mutable config parameter is
required because this mutation is observed later.
- Around line 988-998: Update create_and_configure_transpiler to project
config.optimize_imports directly from the raw this pointer before
configure_bundler retains the config reference, then store that value in
transpiler.options.optimize_imports. Avoid deriving optimize_imports from the
existing &mut (*this).config borrow; leave the synchronous file_map and
entry-point borrows unchanged.
In `@test/internal/source-lints/bundle-completion-task-handoff.test.ts`:
- Around line 133-135: Strengthen the test around the `reborrows` assertion by
first asserting that the dequeue loop in `BundleThread.rs` still binds the task
as `completion`. Use the existing stripped-source inspection or matching helpers
to make this a positive assertion, while preserving the empty-result assertion
for `reborrows` so renaming the binding fails loudly.
---
Outside diff comments:
In `@src/bundler/BundleThread.rs`:
- Around line 273-320: Update generate_in_new_thread so every error path after
ast_memory_store.push() performs the matching pop() and ASTMemoryAllocator
teardown before returning. Encapsulate the fallible transpiler creation and run
flow in an inner scope or scope guard, ensuring cleanup executes for both
create_and_configure_transpiler errors and successful runs while preserving
existing log handling and completion behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c38366ed-b96b-4426-a5c7-5c21a667de5b
📒 Files selected for processing (5)
src/bundler/BundleThread.rssrc/runtime/api/JSBundler.rssrc/runtime/api/js_bundle_completion_task.rssrc/runtime/server/HTMLBundle.rstest/internal/source-lints/bundle-completion-task-handoff.test.ts
There was a problem hiding this comment.
Beyond the inline nit: I checked the reordering in HTMLBundle::on_plugins_resolved — moving RefCount::<Route>::ref_ before the enqueue is the correct order (the matching ScopedRef::adopt in Route::on_complete now cannot precede it), and state.set(Building) landing after the enqueue is safe because on_complete is posted back to the same JS thread's event loop and cannot interleave. Grep also confirms no remaining callers of the deleted trait methods (set_transpiler / as_js_bundle_completion_task / file_map / plugins / from_completion_handle) beyond the stale comment already flagged.
Extended reasoning...
This PR converts every CompletionStruct method from &mut self to this: *mut Self, reorders create_and_schedule_completion_task so all JS-side writes precede the enqueue, and threads a new Deliver enum through both call sites. It is a large unsafe-Rust change to cross-thread memory-safety-critical code (the Bun.build / HTML-route bundle handoff), so it is not a candidate for auto-approval. Beyond the one nit already reported inline, I verified two things a reviewer would otherwise re-check: (1) in HTMLBundle::on_plugins_resolved, the route's self-ref now precedes the enqueue and state.set(Building) follows it — the former is required because Route::on_complete adopts that +1, and the latter is sound because on_complete_anytask is dispatched via loop_handle.post_task to the same JS thread, so it cannot run before on_plugins_resolved returns; (2) a repo-wide grep for the removed trait surface (set_transpiler, as_js_bundle_completion_task, file_map, plugins, from_completion_handle) and for create_and_schedule_completion_task callers turns up only the two updated call sites and the stale SAFETY comment at BundleThread.rs:337 that the inline nit already covers.
…_run reborrows it shared; pin the task binding the lint scans for
|
Pushed two follow-ups for this review round. e092531: |
There was a problem hiding this comment.
I re-reviewed after e092531 and 882708f — the stale set_transpiler SAFETY comment I flagged is fixed, the configure_bundler doc and the lint's binding-name pin from CodeRabbit are in, and the comment-cop hits were shortened. No bugs found this pass. Given this reworks the cross-thread ownership contract for every Bun.build() (trait-wide &mut self → *mut Self, enqueue reordering, field-visibility changes), a human look at the overall shape is still worthwhile.
What was reviewed:
create_and_schedule_completion_task: verified nothing touches the task afterenqueue,poll_ref.ref_/embedded_work_scheduled/ActiveHandle::registerall moved before it, and the returned pointer is only used for cancel/state-store at the two call sites.init_and_run: checked the new&(*this).configis shared (not&mut) so it coexists with the transpiler's erased&config.optimize_importsfromconfigure_bundler;configure_bundlerbody is byte-identical modulo indent and one SAFETY comment.HTMLBundle::schedule_bundle: the route's self-ref now precedes theDeliver::HtmlRoutehand-off, so theon_completederef contract holds even if the build completes beforestate.set.generate_in_new_threaderror path:set_result/complete_on_bundle_threadordering matches the old code;set_logstill runs on both arms before the hand-back.
Extended reasoning...
Overview
This PR fixes aliasing-model UB in the Bun.build() / HTML-route bundler path by converting every CompletionStruct trait method from &mut self to unsafe fn(this: *mut Self), and by moving all JS-side writes to the JSBundleCompletionTask struct to before it is enqueued to the bundle thread. The change spans src/bundler/BundleThread.rs (trait definition, thread_main, generate_in_new_thread), src/runtime/api/js_bundle_completion_task.rs (trait impl, create_and_schedule_completion_task, new Deliver enum, configure_bundler extracted to a free fn, transpiler field deleted, vtable thunks now project fields), src/runtime/api/JSBundler.rs and src/runtime/server/HTMLBundle.rs (call sites pass Deliver and no longer write to the task after scheduling), plus a new source-lint test that pins the signatures.
Security risks
None identified. This is an internal aliasing-contract change with no user-facing surface, no new input parsing, and no auth/crypto/permission code. The one behavioral difference the PR names (started_at_ns sampled a few µs earlier) affects only a dev-mode timing print.
Level of scrutiny
High. This is native cross-thread ownership code on the hot path of every Bun.build() and every HTML-route request in Bun.serve. REVIEW.md calls memory safety the most-blocked category. The change replaces compiler-enforced borrows with a documented # Safety contract on the trait, deletes a field (transpiler) and four trait methods, and reorders the enqueue relative to poll_ref, embedded_work_scheduled, set_config, and ActiveHandle::register. Each of those reorderings looks correct on inspection (all now happen while the JS thread is still the sole owner), but the blast radius of a mistake here is a use-after-free or hang on every build. The PR is well-argued (Miri reduction for all three shapes, exhaustive verification list), and the earlier review round's nits are all addressed, but the trait-wide design choice — raw-pointer receivers rather than, say, UnsafeCell-wrapping the concurrently-touched fields — is the kind of architectural call a maintainer should sign off on.
Other factors
- My previous inline nit (stale
set_transpilerclause in thedrop_in_placeSAFETY comment) is resolved; the comment now correctly names theBundleV2borrow ending insideinit_and_run. - Both CodeRabbit suggestions (documenting why
configure_bundlertakes&mut config; pinning thecompletionbinding name so the lint can't go vacuous) are applied in e092531. - The comment-cop bot flagged many long comments; 882708f shortened them and the current file reads within the repo's usual density.
- Overlaps with #37723 on
complete_on_bundle_threadand thethread_main/generate_in_new_threadplumbing (noted in the description); whichever lands second needs a small rebase — a human should confirm the intended landing order. - The new source-lint test guards the shape but is not a runtime test of the fix; the actual verification is the existing bundler/HTML/worker suites the description lists, which exercise both
stop_for_vm_teardownbranches and the plugin path.
The BundleThread doc still listed configure_bundler as a trait member, the log-copy comment still spoke of init_and_run owning `this` (which now names the task), generate_in_new_thread claimed to be called from JS, and the two docs describing the hand-off said nothing on the JS thread writes the task after the enqueue, which leaves out the cancel and release-while-queued paths the trait exists for. The optimize_imports SAFETY comment now states the invariant that actually holds (the reference is dead once the bundle is over, before the task is handed back) instead of the task outliving the transpiler, which is not true on the success path.
There was a problem hiding this comment.
I reviewed this PR again after e092531/882708f/1fadd08f and didn't find any bugs; the stale set_transpiler SAFETY clause I flagged earlier is now reworded. Because this reworks the cross-thread JSBundleCompletionTask hand-off (every CompletionStruct receiver, the enqueue ordering, and the vtable thunks) under Stacked/Tree Borrows reasoning, a human look is still worthwhile.
What was reviewed:
create_and_schedule_completion_taskordering —poll_ref.ref_,embedded_work_scheduled,set_config, andActiveHandle::registerall land before the enqueue; the enqueue is the last touch.HTMLBundle::on_plugins_resolved— the route's self-ref is now taken beforeDeliver::HtmlRoute(NonNull::from(self))is handed over;Route's mutable state is allCell/JsCell, so shared-provenance is fine forRoute::on_complete.- Deleted
transpilerfield /set_transpiler— no remaining readers insrc/;_unusedinJSBundlerPlugin__onLoadAsync/onResolveAsyncwas already unused. configure_bundlerbody vs. the old method — checked withgit diff -w; only theoptimize_importsSAFETY comment and the banner/footer comment wording differ.
Extended reasoning...
Overview
This PR converts the CompletionStruct trait (the bundle thread's view of a queued Bun.build()) from &mut self/&self receivers to unsafe fn(this: *mut Self) across all seven methods, removes the &mut *completion reborrow in BundleThread::thread_main, and reorders create_and_schedule_completion_task so every JS-side field write (promise, html_build_task, started_at_ns, keep-alive ref, embedded_work_scheduled, plugin set_config, active-handle registration) happens before the enqueue. A new Deliver enum carries the per-owner delivery target at construction; promise/html_build_task/started_at_ns become module-private so the old post-enqueue writes no longer compile. configure_bundler moves from a trait method to a free fn(&mut Config, &mut Transpiler) (body unchanged per git diff -w), the never-read transpiler: *mut BundleV2 field and set_transpiler are deleted, and the CompletionDispatch vtable thunks project single fields via task_of() returning *mut instead of forming &JSBundleCompletionTask to the whole allocation. A source-lint test pins the receiver shape, the dequeue binding, the post-enqueue tail, and the field visibilities.
Security risks
None identified. This is an internal aliasing-model correctness refactor with no user-facing surface, no new input parsing, and no change to trust boundaries. The order of operations on both success and error paths in generate_in_new_thread/thread_main is preserved.
Level of scrutiny
High. This is ~600 net lines across the bundle-thread hand-off, the JSBundleCompletionTask struct, both call sites (Bun.build and HTMLBundle::Route), and the dispatch vtable — all unsafe Rust reasoning about cross-thread pointer provenance under Stacked/Tree Borrows. REVIEW.md flags native memory safety as the most-blocked category, and this change specifically trades compiler-checked &mut exclusivity for hand-proven per-field projection. The PR description's Miri reduction is thorough and the shape matches the already-landed #37723 family, but the reasoning about which fields the JS thread may touch concurrently (atomics only: cancelled, bundle_loop, stage) versus which the bundle thread owns until hand-back (config, log, result) is subtle enough that a maintainer familiar with this struct's lifecycle should confirm it.
Other factors
My prior inline nit (stale set_transpiler clause in the drop_in_place SAFETY comment) was addressed in e092531. CodeRabbit's optimize_imports retained-reference concern was checked under Miri and withdrawn — the shared reborrow in init_and_run is valid under both models; the &mut self on main was the invalidating operation. CI passed on 882708f. Five comment-cop notices from the 14:22 run remain open on 2-4-line doc/SAFETY comments; the author addressed the substance in the previous round (they state contracts, not workarounds), but they are technically outstanding automated feedback. No human reviewer has weighed in yet.
|
Small overlap with #37709: that PR deletes |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs; my earlier note about the stale set_transpiler reference in the drop_in_place SAFETY comment was addressed in e092531. Because this reworks the cross-thread ownership contract of JSBundleCompletionTask (every CompletionStruct receiver, the enqueue ordering, and the vtable thunks) across ~600 lines of unsafe Rust, a human look is still worthwhile.
What was reviewed:
create_and_schedule_completion_taskordering —poll_ref.ref_,embedded_work_scheduled,set_config, andActiveHandle::registerall now precede the enqueue; the enqueue is the last touch and the tail only returns the pointer.HTMLBundle::schedule_bundle— the route's self-ref is taken beforeDeliver::HtmlRoutehands the pointer over;State::Buildingstill stores the completion pointer forState::deinit's cancel.init_and_run's&(*this).configborrow vs the retained&config.optimize_importsin the transpiler — shared reborrow only, matching the Miri result in the resolved thread.- The deleted
transpilerfield /set_transpiler— grepped for remaining readers; none, and the_unusedparams onJSBundlerPlugin__onLoadAsync/onResolveAsyncwere already unused.
Extended reasoning...
Overview
This PR converts the bundle-thread side of JSBundleCompletionTask's lifecycle from &mut self receivers to raw *mut Self field projections, and reorders the JS-thread hand-off so every field is written before the task is enqueued. Files touched: src/bundler/BundleThread.rs (the CompletionStruct trait and thread_main/generate_in_new_thread), src/runtime/api/js_bundle_completion_task.rs (the impl, create_and_schedule_completion_task, the Deliver enum, the COMPLETION_VTABLE thunks, and configure_bundler moved to a free function), the two callers in JSBundler.rs and HTMLBundle.rs, and a new source-lint test that pins the receiver shapes and the enqueue-is-last invariant.
Security risks
None identified. This is an aliasing-model correctness fix (Miri-diagnosed protected-tag violations under both Stacked and Tree Borrows); no user-facing input handling, no auth/crypto, no new I/O.
Level of scrutiny
High. This is exactly the class REVIEW.md calls out as "the most-blocked category": one allocation shared between the JS thread and the bundle thread for the whole build, with concurrent cancel/release paths (stop_for_vm_teardown, HTMLBundle::State::deinit) racing the bundle thread's try_start/init_and_run. Every reordered statement in create_and_schedule_completion_task and every field projection in the trait impl carries a thread-affinity or lifetime obligation. The PR description is unusually thorough (a runnable Miri reduction for all three failure shapes, and a rationale for pointer receivers over interior mutability), and the extensive comment-cop / CodeRabbit threads on the PR were all resolved with either trimming or a Miri counter-check — but the design choice (pointer receivers on a trait, field-projection discipline enforced only by a source-lint) is one a maintainer should sign off on.
Other factors
- My earlier inline finding (stale
set_transpilermention in a SAFETY comment) was fixed in e092531; the comment now names the actual invariant (BundleV2borrowing the transpiler went away insideinit_and_run). - CodeRabbit's concern about the retained
&config.optimize_importswas withdrawn after robobun's Miri reduction showed the later&(*this).configis a shared reborrow that both models accept; the SAFETY comments at both ends now record that constraint. - The
configure_bundlerbody is unchanged modulo indentation and one SAFETY comment (verified against the diff; it moved from a trait method to a freefn(&mut Config, &mut Transpiler)). - The author noted a small overlap with #37709 (
result_is_errdeletion) and #37723 (complete_on_bundle_thread), so whichever lands second gets a trivial rebase. - The new source-lint test self-checks its regexes against positive/negative fixtures, so it is not vacuous, but it is a text scan rather than a type-level guarantee — that trade-off is called out in the test's header comment.
Problem
JSBundleCompletionTaskallocation is shared by the JS thread and the bundle thread while aBun.build()or HTML route build runs. On main the bundle thread holds it as&mut selffor the whole build, so anything the JS thread does to it meanwhile is a write into memory a protected&mutcovers.promise/started_at_ns/html_build_taskare set after the task is already enqueued (every build);cancelledis stored on VM teardown or route deinit mid-build; and the bundler writes the task'slogthrough a pointer taken out of one&mut selfwhile a later&mut selfis live.write access through <..> is forbidden,the accessed tag is foreign to the protected tag) and passes with pointer receivers.noalias/dereferenceableon the receiver let the compiler assume it. Same family as bundler: hand a finished Bun.build back through its pointer, not a &mut receiver #37723, which converts the hand-back; this covers the rest of the build.Fix
CompletionStructmethod takes the task asthis: *mut Selfand reaches only the fields it uses; the bundle thread no longer reborrows the dequeued task. Trait methods that existed only for the impl to call on itself go away, as does thetranspilerfield, which nothing read. Order of operations is unchanged.Deliver(a promise forBun.build, a route forBun.serve), fills the per-owner fields at construction, and enqueues as its last statement. Those fields are private now, so the old writes do not compile. Only observable difference:started_at_nsis taken a few microseconds earlier.cancelledand the queued-release handshake, and nothing on the bundle thread holds a reference to the whole task. Interior mutability would not do it, since a&mut selfretag claimsUnsafeCellbytes too; the racing fields are already atomics.Background
Bun.build()andBun.serveHTML routes do not bundle on the JS thread. They box aJSBundleCompletionTask, push it onto a queue, and one bundle thread pops it, runs the build, and posts it back to the owning event loop. Until that post, both threads can reach the same allocation.CompletionStructis the trait the generic bundle thread (inbun_bundler) uses to drive a task whose concrete type lives inbun_runtime; most of the diff is changing its receivers and the impl behind them.bun run rust:mirichecks) a&mut selfargument is protected for the whole call. Any access through another pointer during that call is UB, even an atomic store, even to bytes the callee never touches. A raw*mutmakes no such claim, and&raw mut (*this).fieldclaims only that field.Deliveris the new enum naming who gets the result (promise or route). It is consumed at construction, so the per-owner fields are set before the task is ever on the queue.[review] gate passed · iteration 1 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file
Original description
Problem
One
JSBundleCompletionTaskallocation is shared between the JS thread and the bundle thread for as long as aBun.build()(or an HTML route's build) runs. Onmainthe bundle thread holds it as&mutfor the whole build:BundleThread::thread_main(src/bundler/BundleThread.rs) reborrows the dequeued task, and everyCompletionStructmethod takes&mut self,create_and_configure_transpilerfor the option setup andinit_and_runfrom beforeBundleV2::inituntil afterrun_from_js_in_new_threadreturns. A reference argument is protected for the duration of its call, so while those calls are running, three things that happen on every build or on every teardown are accesses into protected memory through a pointer that is not derived from the reference:create_and_schedule_completion_task(src/runtime/api/js_bundle_completion_task.rs) enqueued the task and only then didpoll_ref.ref_(..), and its callers kept writing:JSBundler::buildsetpromise(the comment there, "sole owner on the JS thread until enqueued task runs", was not true, the enqueue had already happened inside the callee) andHTMLBundle::Route::schedule_bundlesetstarted_at_nsandhtml_build_task. By then the bundle thread may already be insidecreate_and_configure_transpiler(&mut self). This is the normal path of every build.stop_for_vm_teardownreadspluginsand storescancelled/ loadsbundle_loop, andHTMLBundle::State::deinitstorescancelled, whileinit_and_run(&mut self)is live on the bundle thread. They are atomics, but under both aliasing models an atomic store through a foreign pointer into memory a protected&mutcovers is still a foreign write.create_and_configure_transpilerhandedTranspiler::inita&raw mut self.logtaken out of its own&mut self, and the bundler writes that log for the whole build, i.e. while the later, sibling&mut selfofinit_and_runis protected. This one is single-threaded.A reduction of exactly these three shapes fails under Miri with Tree Borrows (the model
bun run rust:miriuses) and with Stacked Borrows, in each case pointing at the&mut selfreceiver, and passes once the methods take the pointer (and, for 1, the field is written before the hand-off):Stacked Borrows reports
not granting access to tag .. because that would remove [Unique for ..] which is strongly protectedfor 1 and 2 andthat tag does not exist in the borrow stackfor 3. No crash is known from any of this; ASAN-clean today, because nothing on the bundle thread actually reads the fields the JS side writes. It is the contract that is wrong, and it is whatnoalias/dereferenceableon the receiver let the compiler assume. Same family as #37723, which converts the hand-back (complete_on_bundle_thread) and named the rest of the build's lifetime as a separate change; this is that change. The two overlap oncomplete_on_bundle_threadand on the pointer plumbing inthread_main/generate_in_new_thread, where both end up with the same shape, so whichever lands second has a small rebase.Reduction run under Miri
MIRIFLAGS=-Zmiri-tree-borrows cargo miri run -- before {1,2,3}and the Stacked Borrows default all exit 1 with the errors quoted above;-- after {1,2,3}exit 0 under both models.Fix
CompletionStruct(src/bundler/BundleThread.rs) takes the task asthis: *mut Selfin every method (try_start,free_released_unstarted,complete_on_bundle_thread,set_result,set_log,create_and_configure_transpiler,init_and_run); the trait doc carries the argument above and the one# Safetycontract.thread_mainno longer reborrows the dequeued task andgenerate_in_new_threadtakes the pointer. The impl projects the fields it uses:create_and_configure_transpilerborrowsconfig, takes&raw mut (*this).logand readsenv;init_and_runstoresbundle_loopthrough the atomic, readsplugins, borrowsconfigfor the file map and entry points, and builds the dispatch handle fromthis; the vtable thunks (task_of) projectresult/cancelled/loop_handleinstead of forming a&JSBundleCompletionTaskto the whole task. Same order of operations as before on both the success and the error path.configure_bundleris a free function over&mut Config(its body is unchanged apart from indentation and one SAFETY comment;git diff -wshows it),plugins/file_map/as_js_bundle_completion_taskare inlined intoinit_and_run, andset_transpileris deleted together with thetranspilerfield it wrote, which nothing read (the C++ plugin'sconfigpointer that used to be the other way to reach the task is passed back as_unused).create_and_schedule_completion_tasktakes aDeliver(Promise(JSPromiseStrong)fromBun.build,HtmlRoute(NonNull<Route>)fromBun.serve), fillspromise/html_build_task/started_at_nsin at construction, takes the keep-alive ref before the enqueue, and the enqueue is its last statement.JSBundler::buildcreates the promise and reads its value before scheduling;HTMLBundle::schedule_bundlerefs the route before handing its pointer over and only stores the returned pointer (for the cancel inState::deinit).promise,html_build_taskandstarted_at_nsare private now (started_at_ns()for the dev-mode timing line), so the old writes do not compile.started_at_nsis taken a few microseconds earlier than before, that is the only observable difference.Why pointer receivers rather than interior mutability: the fields the JS thread touches during the build are already atomics, and that does not help, because a
&mut selfretag claims theUnsafeCellbytes too (only shared references leave them out). The interior-mutability version of this fix would therefore have to give the bundle thread a&selfto the whole task and put everything the bundle thread writes (log,result, theconfig.compileclear, the handle wiring) behind cells, touching every JS-side use of those fields as well. Taking the task by pointer and projecting fields is the smaller change and is the shape the rest of this struct's lifecycle already uses (free_released_unstarted,stop_for_vm_teardown,on_complete_anytask, and #37723's hand-back), as do the other conversions in this family.Tests
test/internal/source-lints/bundle-completion-task-handoff.test.ts pins the two signatures the compiler cannot: every method of
CompletionStructtakesthis: *mut Self(and the methods that bracket the build are still there, so the check cannot pass vacuously), BundleThread.rs contains no reborrow of the dequeued task, nothing increate_and_schedule_completion_taskgoes through the task after theenqueuecall, and the three per-owner fields are private. It checks its scans against positive and negative examples. Againstmainit reports the elevenselfreceivers,BundleThread.rs:235: &mut *completion, the(*completion)after the enqueue, andpub(crate)on all three fields; it passes with this branch.Verification
Debug (ASAN) build: test/bundler/bun-build-api.test.ts and bundler_plugin.test.ts (105 pass, including the build-thousands-of-times test), bundler_plugin_chain, bundler_html_server, plugin-error-nested-throw, plugin-sync-exception-fallback, test/js/bun/http/bun-serve-html.test.ts, bun-serve-html-405, bun-serve-html-manifest (the
HTMLBundle::Routepath in development and production mode), the two worker tests in test/js/node/worker_threads/worker_threads.test.ts that terminate workers with builds both queued and mid-plugin (bothstop_for_vm_teardownbranches, racingtry_start/free_released_unstarted), and thecountedfamily of test/js/web/workers/worker-terminate-funnels.test.ts.bun test test/internal/source-lints/(19 files) passes;cargo clippy -p bun_runtime -p bun_bundlerandcargo fmt --checkare clean. bun-serve-html-entry.test.ts cannot connect tolocalhostin this container with the release binary either (already noted on #37723).