Hand pool-finished transpiler and patch jobs back through their pointer, not a &mut receiver - #37778
Hand pool-finished transpiler and patch jobs back through their pointer, not a &mut receiver#37778robobun wants to merge 6 commits into
Conversation
…not &mut self TranspilerJob::dispatch_to_main_thread(&mut self) pushed the job slot onto the store queue from inside run(&mut self)'s scope guard, and PatchTask::run_from_thread_pool_impl(&mut self) pushed its task onto the manager's queue the same way. From the push on, the consuming thread recycles or frees the object while those receivers are still live arguments of the functions that pushed. dispatch_to_main_thread now takes the slot pointer; run_from_worker_thread calls run() through a statement-scoped reborrow and dispatches afterwards on both paths, which also replaces the scope guard. PatchTask's push moves into run_from_thread_pool, next to the raw pointer recovered from the pool task. A source lint bans pushing a receiver's address onto a queue, and the import and worker tests exercise the hive, heap-spilled and teardown hand-back paths.
|
Updated 4:54 PM PT - Aug 12th, 2026
❌ @robobun, your commit 207f02b has 2 failures in
🧪 To try this PR locally: bunx bun-pr 37778That installs a local version of the PR into your bun-37778 --bun |
|
Warning Review limit reached
Next review available in: 1 minute 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 (5)
Comment |
|
Status: ready for review. Reproduced how: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
TranspilerJob::run_from_js_thread(&mut self) ended with store.put(self), which drops the slot in place, or frees it when it spilled past the hive, while that receiver is still a live argument. The result is now moved out by a &mut self helper (take_completion), the put goes through the pointer the queue handed back, and fulfill runs on the moved-out values as before. The lint covers .put( alongside .push( (self-receiver-push-put.test.ts); against main it reports the three converted sites. FilePoll's own put of its slot is allowlisted with an exact count and tracked separately.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Two follow-up pushes since the PR opened:
Description and status comment are updated; unresolved review threads from the comment linter are answered above. |
There was a problem hiding this comment.
My earlier note on run_from_js_thread was addressed in f9bfdec (now takes the popped pointer, take_completion(&mut self) moves the result out before put(this)), and the comment-cop threads were trimmed in 16ba6ac. No further issues found.
Checked: run()'s sole caller is run_from_worker_thread and the workspace is panic = "abort", so the post-call dispatch covers every path the removed defer! did; the borrow_if_running() guard now dropping before dispatch is covered by the embedded-work count as described. PatchTask::run_from_thread_pool_impl is now private with the one caller. The lint's allowlist entries match the two FilePoll sites.
Given this reworks cross-thread ownership hand-over on the transpiler hot path and worker teardown, a human look is still worthwhile.
Extended reasoning...
Overview
Converts three &mut self methods that hand their own storage to another thread (or back to a hive that may free it) into unsafe fn(this: *mut Self) with statement-scoped reborrows: TranspilerJob::dispatch_to_main_thread, TranspilerJob::run_from_js_thread, and the push half of PatchTask::run_from_thread_pool. Removes the scopeguard::defer! from TranspilerJob::run() in favour of an unconditional dispatch in the caller. Adds a source lint (self-receiver-push-put.test.ts) covering .push( and .put( of the receiver's address, with a ratcheted allowlist for the two FilePoll::deinit_possibly_defer instances, plus runtime coverage of hive overflow (96 concurrent imports) and worker-teardown-mid-transpile.
Security risks
None identified. No user-controlled input parsing, auth, or crypto. The change tightens an aliasing-model contract (protected &mut argument vs. cross-thread free/write) rather than introducing new unsafe surface.
Level of scrutiny
High. This is memory-safety-critical unsafe Rust on a hot path (every async import()) with cross-thread ownership reasoning: it changes when the borrow_if_running() guard is released relative to the queue push, replaces a scope guard with post-call sequencing under panic = "abort", and reorders run_from_js_thread around a new Completion struct. Each step is argued in the description and matches the sibling PRs (#37681/#37703/#37723/#37768), but the reasoning about embedded-work counts keeping the VM handle open across the now-unguarded dispatch, and HiveArrayFallback::put's drop-in-place vs. Box-free split, deserves a maintainer's eyes.
Other factors
My prior review flagged the run_from_js_thread put(ptr::from_mut(self)) sibling; f9bfdec folded it in and extended the lint to .put(. The comment-cop bot's verbosity flags were trimmed in 16ba6ac. All inline threads are resolved. Tests are thorough: the lint self-tests its own patterns, has a non-vacuous scan guard, and ratchets the allowlist; the 96-import test covers both hive and heap-spilled slots plus the parse-error early return; the worker test exercises release_queued_jobs_for_teardown. Verified run() has no other callers and run_from_thread_pool_impl is now private.
|
Heads up: #37803 converts the two FilePoll sites this PR's |
There was a problem hiding this comment.
I re-reviewed after f9bfdec and 16ba6ac and didn't find new issues — the run_from_js_thread put now goes through the pointer as flagged, and the lint covers .put( too. Given this is unsafe aliasing-model surgery on the transpiler store hot path and shifts the running-branch dispatch outside the borrow_if_running() guard onto the embedded-work count, a human look is still worthwhile.
What was reviewed:
run()'sdefer!removal vs. caller-side dispatch — every early return inrun()still reaches the dispatch; workspace ispanic = "abort"so the guard bought nothing extra.- Dispatch-after-guard-drop on the running branch — covered by the embedded-work count as the not-running branch already was;
post_taskstill hitsPosted::Queued. take_completionpreserves the old order (reset → put → fulfill) and moves out the same fields;run_from_thread_pool_implgoing private has no other callers.
Extended reasoning...
Overview
The PR converts three &mut self methods that hand their own storage away (via .push() onto a cross-thread queue or .put() back into a hive) to take *mut Self instead: TranspilerJob::dispatch_to_main_thread, TranspilerJob::run_from_js_thread, and PatchTask::run_from_thread_pool. It removes the scopeguard::defer! inside TranspilerJob::run() in favor of an unconditional dispatch in the caller, adds a Completion struct and take_completion() helper to move the result out before the slot is recycled, and adds a source lint (self-receiver-push-put.test.ts) plus two behavioral tests (96-way concurrent dynamic import overflowing the 64-slot hive; worker terminate mid-transpile).
Security risks
None. This is an internal aliasing-model correctness refactor with no user-facing API surface, no parsing of untrusted input, and no auth/crypto/permissions code touched.
Level of scrutiny
High. The transpiler store is on the hot path of every async module load, and the change alters where the running-branch dispatch happens relative to the borrow_if_running() guard — previously the dispatch ran under the guard (via defer! inside run()), now it runs after the guard drops, relying on the embedded-work count for VM lifetime. The PR description argues this correctly (the not-running branch already relied on the same count), and the post_task result is still asserted Queued, but this is exactly the kind of synchronization-ordering change where a maintainer's eye on the VM-teardown sequencing is valuable. The unsafe-Rust reasoning about protectors is subtle by nature.
Other factors
My earlier review flagged that run_from_js_thread had the same shape as dispatch_to_main_thread — that was folded in with f9bfdec, and the lint was extended to cover .put(. The comment-cop feedback on comment length was addressed in 16ba6ac; the remaining comments are Safety contracts and one- or two-line ownership notes. I confirmed panic = "abort" is set in the workspace Cargo.toml (so removing the defer! in run() for its early-return coverage is sound), and that run_from_thread_pool_impl (now private) has no other callers. The lint's allowlist for the two FilePoll sites is ratcheted with an exact count and #37803 is queued to convert those. The new tests exercise both the hive-inline and heap-spilled slot paths, the parse-error early return, and the teardown release path. No bugs were found by the bug-hunting system on this pass.
RuntimeTranspilerStore::run_from_js_thread pops a whole batch of finished jobs off the queue and returns as soon as draining microtasks reports that script was terminated. The jobs it had not reached yet were already off the queue, so release_queued_jobs_for_teardown never saw them: their path buffer, module promise, transpiled source and (for slots spilled past the hive) the slot itself leaked on every worker.terminate() that landed mid-batch. The drain now releases the remainder through the same helper the teardown uses. The worker test keeps a stream of imports in flight and terminates once the first one has evaluated, so the termination always lands mid-batch; under ASAN the host is leak-checked with the transpile-wide suppression removed, which is what fails without this change. Both new tests also check, in debug builds, that their imports really went through the store.
| /// their source, log and module promise here instead of running. (A batch | ||
| /// that `run_from_js_thread` had already popped when script was terminated | ||
| /// is released there.) |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Releases `job` and whatever is left in `iter`: jobs whose completion | ||
| /// will not run drop their module promise here, on the JS thread, and go | ||
| /// back to the pool. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Script was terminated. The rest of this batch is already off | ||
| // the queue, so the teardown would never see it: release it now. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
1 participant
Problem
&mut selfmethod: the transpiler job pushes itself onto the store queue and laterputs itself back into its hive, and the patch task pushes itself onto the package manager's queue.putitself drops the slot in place (and frees it when more than 64 jobs are in flight). The calling method's&mut selfis still live while that happens.&mutargument is protected until its call returns, so the compiler may re-read through it after the push, andbun run rust:mirirejects the free outright ("the strongly protected tag disallows deallocations"). A read after the push is what crashed the Zig version of this code (fix(runtime-transpiler): don't readthisafter publishing TranspilerJob to main thread #29128).push,put) that none of those lints cover.Fix
&mutto the object ends with its own statement, so none exists at the moment the other side takes it.defer!dispatch becomes an unconditional dispatch by the worker afterrun(); the workspace ispanic = "abort", so the same early returns are covered, and the push stays under the embedded-work count teardown waits on. The finished result is moved out into a struct so the slot still goes back before script runs; a batch cut short by VM termination now releases the jobs it had already popped..push/.put; it reports exactly these three sites on main and passes here (twoFilePollsites are allowlisted at an exact count until io: return a FilePoll to its store through the owner's pointer, not a &mut receiver #37803). Behaviour is unchanged, so the rest is coverage under the ASAN debug build: a new 96-way concurrentimport()test that overflows the hive and includes parse failures, a new workerterminate()-during-transpile test, and the existing patch tests.Background
import()s are transpiled on the thread pool in per-VM job slots (64 inline, heap boxes past that). The pool pushes a finished slot onto a queue; the JS thread pops it, fulfils the module promise andputs the slot back, which drops it in place or frees a heap spill.bun installcomputes and applies package patches on the thread pool; the worker pushes the finished task onto the package manager's queue and the main thread takes it and drops it.&mutargument isnoaliasanddereferenceable. Writing to or freeing that memory before the call returns, from any thread, is undefined behaviour even if the method never reads it again; Miri's Tree Borrows mode reports it, and codegen relies on it.schedulebumps and the worker releases after dispatch, which is what makes pushing and posting outside theborrow_if_running()guard safe.test/internal/source-lints/holds tests that scan the Rust tree for a banned shape and pin allowlisted leftovers to an exact count, so a converted site cannot be replaced by a new one.Original description
Problem
Three places give an object's storage away from inside a
&mut selfmethod on that object:TranspilerJob::dispatch_to_main_thread(&mut self)(src/jsc/RuntimeTranspilerStore.rs)queue.push(NonNull::from(&mut *self)), run from ascopeguard::defer!insiderun(&mut self), whichrun_from_worker_threadenters through(*this).run(); the not-running branch calls(*this).dispatch_to_main_thread()directlyTranspilerJob::run_from_js_thread(&mut self)(same file)store.put(ptr::from_mut(self)), entered through(*job).run_from_js_thread()PatchTask::run_from_thread_pool_impl(&mut self)(src/install/patch_install.rs)patch_task_queue.push(NonNull::from(&mut *self)), entered through&mut *PatchTask::from_task_ptr(task)The push is the hand-over: as soon as it lands, the JS thread pops the transpiler job, writes its fields (
run_from_js_thread, orrelease_queued_jobs_for_teardownduring teardown) andput()s it, and the package manager's main threadheap::takes the patch task and drops it (src/install/PackageManager/runTasks.rs). Theputis the hive return itself:HiveArrayFallback::putdrops the slot in place, and when more thanTRANSPILER_JOB_HIVE_CAP(64) jobs are in flight the slot is a heapBoxthatputfrees. In all three cases this happens while the&mut selfof the method that made the call (and, for the transpiler's push, ofrunone frame up) is still a live argument.A reference argument is protected until its call returns: rustc emits it as
noalias+dereferenceablefor the whole call, so the compiler may re-read through it after the call instead of keeping the copies taken before it (a source-level read after the push is what crashed the Zig version of this code, #29128), and under the aliasing modelbun run rust:mirichecks (Tree Borrows) freeing theBoxis rejected outright ("the strongly protected tag disallows deallocations") and the other thread's writes race with the protector's release. The comment indispatch_to_main_threaddescribed the hazard, but the receiver type contradicted it. No crash is known from any of the three today; this is the same contract bug as #37681, #37703, #37723 and #37768, in two spellings (push,put) none of those lints cover.Fix
All three go through the pointer the caller already holds, the shape
ShellTask::on_finish,NetworkTask::notifyandPackageManagerTask::Task::callbackalready use:dispatch_to_main_threadbecomesunsafe fn(this: *mut Self): it copiesvmand clonesloop_handlethrough accesses that end before the push, pushesthis, and posts the store.run_from_worker_threadrunsrun()through a statement-scoped reborrow (only while the VM is running, as before) and then dispatches unconditionally, which replaces thedefer!inrun: that guard only existed to coverrun's early returns, and the workspace ispanic = "abort", so a call after the scopedrun()covers the same paths. The dispatch now happens after theborrow_if_running()guard is released on the running path too; the push and post are covered by the embedded-work count (scheduleincrements it,embedded_work_finishedfollows the dispatch, and teardown waits for it beforeclose()), which is what the not-running branch already relied on.run_from_js_threadbecomesunsafe fn(this: *mut Self). The body that moves the result out of the slot and resets it is nowtake_completion(&mut self) -> Completion, invoked as a statement-scoped(*this).take_completion(); theputthen takesthis, andAsyncModule::fulfillruns on the moved-out values, in the same order as before (slot back first, then script). The two callers inRuntimeTranspilerStore::run_from_js_threadpass the popped pointer.PatchTask::run_from_thread_poolkeeps the raw pointer it recovers from the pool task, runs the body through(*this).run_from_thread_pool_impl(), and pushesthis; the push and wake move out of the&mut selfmethod.Out of scope, on purpose:
TranspilerJob::schedulein the same file is theWorkPool::scheduleinstance of this and is converted by #37768 (the hunks do not overlap).FilePoll::deinit_possibly_defer(src/io/posix_event_loop.rs and the Windows twin) puts its own slot back the same way; everyFilePoll::deinit*entry point is&mut selfwith many owners, so it is allowlisted in the lint below with an exact count; #37803 converts those two sites and carries the same lint file with the allowlist turned around, so whichever of the two PRs lands second deletes the entries that remain (the ratchet test fails until it does). quic'sENDPOINT_REGISTRYpushesfrom_ref(self).cast_mut()from a&selfmethod into a same-thread registry whose consumer neither writes nor frees; the lint leaves the shared spellings out rather than allowlisting it.Verification
test/internal/source-lints/self-receiver-push-put.test.tsbans passing the receiver's address (NonNull::from(&mut *self),from_mut(self),&raw mut *self,self as *mut _, wrapped inNonNull::new*or not, inline or through a local of the same function) to a.push(..)or.put(..), with a self-test of the spellings it does and does not match and an exact-count ratchet for the twoFilePollentries. Against main it reports exactly the three sites above:and passes with this branch.
Behaviour is unchanged, so the rest is coverage of the converted paths, run against the debug (ASAN) build:
test/js/bun/resolve/concurrent-dynamic-import.test.ts: new test issues 96import()s at once, 12 of which fail to parse, and checks every one settles with its own module's result. All 96 jobs are claimed before the first is handed back, so this covers the hive slots, the heap-spilled slots thatput()frees, and both the success and the early-return paths ofrun()reaching the caller-side dispatch. An interleaved comparison of the per-import round trip between this branch and a main build of the same tree showed no difference (3.8 to 6.6 ms per import on a loaded debug box, both builds alike).test/js/web/workers/worker.test.ts: new test terminates workers while their imports are still on the pool. WithBUN_DEBUG_Worker=1the teardown log shows between 1 and 57 jobs still out on the pool in most of the teardowns, so the not-running branch andrelease_queued_jobs_for_teardownare exercised; the test asserts a clean exit. About 1.2s locally under ASAN. Three older tests in the same block fail locally on this machine exactly the same way without this change (two time out at 5s, one expects a worker to have posted within 30ms; debug build speed), so they are not touched here.test/cli/install/bun-install-patch.test.ts(18 pass) andtest/cli/install/bun-patch.test.ts(31 pass) for the patch task hand-over.cargo buildof the touched crates is warning-free;rustfmt --checkis clean.