Don't enqueue to a terminated worker's freed event loop from other threads - #32071
Don't enqueue to a terminated worker's freed event loop from other threads#32071robobun wants to merge 23 commits into
Conversation
…reads A worker's VirtualMachine (with its EventLoop and MPSC concurrent task queue embedded in it) is freed by WebWorker::shutdown() when the worker is terminated. Cross-thread producers that captured a raw pointer to the VM or its event loop at schedule time (the HTTP client thread for fetch/S3 completions, the work pool for fs/crypto/zlib/napi completions, watcher threads, napi addon threads, the process waiter thread) had no way to observe that free: a completion landing after terminate() read the freed VM and pushed into its freed queue. The corrupted queue memory then surfaced on whichever live worker reused it, as "Panic: invalid enum value" in EventLoop.tickQueueWithCount (Sentry BUN-2VPE, 546 events, Windows-dominant, always on a worker thread with workers_spawned + workers_terminated set). Fix: a process-global registry of live (VirtualMachine, EventLoop) addresses, mirroring the fence the C++ postTaskTo path already has with allScriptExecutionContextsMap. VMs register in VirtualMachine::init(), workers unregister at the top of shutdown() before anything is freed, and the boxed spawnSync loop registers/unregisters around its lifetime. Cross-thread producers now go through checked entry points (VirtualMachine::try_enqueue_task_concurrent / with_live_vm / is_shutting_down_or_freed, EventLoop::try_enqueue_task_concurrent) that hold the registry lock across the push, so teardown cannot free the VM mid-enqueue; the immortal main-thread VM takes a lock-free address- compare fast path. When the target is gone the task is dropped: the node is freed if auto_delete and the payload is left to leak, matching the fate of tasks already sitting in a terminated worker's never-drained queue. Converted producers: FetchTasklet (the shutting-down check on the HTTP thread raced the same free), S3 simple/download tasks, WorkTask, ConcurrentPromiseTask, AnyTaskJob (also stops reading vm.global on the pool thread), node:fs AsyncFSTask and readdir-recursive (now capture the VM at schedule instead of deriving it from the freed JSGlobalObject at completion), node:zlib native streams (same capture), napi async_work / ThreadSafeFunction / NapiFinalizerTask, fs watchers and stat watcher, Archive, Bun.password, RuntimeTranspilerStore, JSBundler completion, DevServer hot reload events, JSC deferred work scheduler and the concurrent keep-alive counters, and every EventLoopHandle user (shell tasks, the POSIX waiter thread) via the vtable arm. The regression test terminates a worker while a fetch is in flight and releases the response only after the worker VM is freed; on an unfixed ASAN build the HTTP thread trips heap-use-after-free in FetchTasklet::callback -> VirtualMachine::is_shutting_down deterministically.
|
Updated 7:34 PM PT - Jul 13th, 2026
✅ @robobun, your commit 99fb435c328c228c637f9b85bcd7a1744bb89b0b passed in 🧪 To try this PR locally: bunx bun-pr 32071That installs a local version of the PR into your bun-32071 --bun |
|
Found 7 issues this PR may fix:
🤖 Generated with Claude Code |
Accepting a possibly dangling pointer is these functions' contract; no dereference happens until the live-VM registry proves the pointee alive and holds off its free. Same pattern as FetchTasklet::deref_from_thread.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a live-VM registry and VM-level fallible concurrency helpers, registers/unregisters VMs at init/shutdown, implements EventLoop try-enqueue/discard helpers, and migrates many cross-thread enqueue/ref/unref callsites to the new checked APIs to avoid using freed worker VMs. ChangesWorker VM lifetime safety for concurrent task enqueueing
Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/jsc/ConcurrentPromiseTask.rs (2)
111-121:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCapture the owning VM here, not just the
EventLoop*.These two wrappers still route completion through
EventLoop::try_enqueue_task_concurrent(event_loop.as_ptr(), task), so the admission check is only as strong as loop-address liveness. The PR’s stated root cause is freed worker memory being reused by a new worker; if a new worker gets the sameEventLoopaddress, a stale completion can be accepted into the wrong VM and then run with deadglobal_this/ promise state. These task types should capture*mut VirtualMachineat creation time and useVirtualMachine::try_enqueue_task_concurrent(...)instead, then sweep the siblingEventLoop::try_enqueue_task_concurrentsites that also only carry loop identity. As per coding guidelines, "fix the whole bug class in the same PR." Based on PR summary, the original crash involved stale producers targeting memory reused by a new worker.🤖 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/jsc/ConcurrentPromiseTask.rs` around lines 111 - 121, The current code only captures EventLoop pointer (`event_loop`) and calls EventLoop::try_enqueue_task_concurrent(event_loop.as_ptr(), task), which can accept completions into the wrong VM if a new VM reuses the same EventLoop address; instead capture the owning VirtualMachine pointer at task creation and call VirtualMachine::try_enqueue_task_concurrent(...) so admission checks use VM liveness. Update the code that constructs the task (referencing this_ref.event_loop and this_ref.concurrent_task.from(...)) to also capture the owning `*mut VirtualMachine` and replace the EventLoop::try_enqueue_task_concurrent call with VirtualMachine::try_enqueue_task_concurrent, and sweep other sites that only pass an EventLoop pointer to use the VM-based enqueue API.Source: Coding guidelines
111-121:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe new “target VM is gone” paths lose the only cleanup owner.
ConcurrentPromiseTaskandWorkTaskboth buildAutoDeinit::ManualDeinitnodes, so a failed checked enqueue strands the heap owner: neitherrun_from_js()nordestroy()runs, and the task’s context / promise state / keepalive cleanup is lost.TranspilerJob::dispatch_to_main_thread()has the same shape: whenwith_live_vm()returns false,run_from_js_thread()never runs, sopromise.deinit(),reset_for_pool(), and slot recycling are all skipped even though this file documents those as the non-Drop teardown path. Please add an explicit failure teardown/recycle path for the “VM already gone” case instead of silently dropping these completions. As per coding guidelines, "pair every acquisition with its release at the acquisition site" and "every allocation must have exactly one named owner, released exactly once." Based on PR summary, target-gone enqueue currently drops payloads when the VM is missing.🤖 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/jsc/ConcurrentPromiseTask.rs` around lines 111 - 121, The checked enqueue call (EventLoop::try_enqueue_task_concurrent) can fail when the target VM is gone, which currently drops the ManualDeinit-owned payload (ConcurrentPromiseTask / WorkTask) and leaks or leaves promise/context in an inconsistent state; change the branch that handles a false return from try_enqueue_task_concurrent to perform the same explicit teardown/recycle currently done in the normal completion path: invoke the task’s failure teardown (call the equivalent of run_from_js()/run_from_js_thread() or destroy() logic that performs promise.deinit(), reset_for_pool(), and releases any keepalive/context), and return the AutoDeinit owner back to its pool/slot so the allocation is recycled; ensure this cleanup code is added at the enqueue site (next to where AutoDeinit::ManualDeinit is created) so every acquisition is paired with a single release even when with_live_vm()/try_enqueue_task_concurrent() reports the VM is gone.Source: Coding guidelines
src/runtime/napi/napi_body.rs (1)
2711-2723:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't leave the TSFN stuck in
Pendingwhen the checked enqueue is rejected.
schedule_dispatch()flipsdispatch_statetoPendingbefore the enqueue attempt, but it ignores thefalsereturn. Once the worker VM has been unregistered,enqueue()still appendsctxpointers and returnsok, while no JS task can ever drain the queue or queue the finalizer. For unbounded TSFNs that becomes an unbounded leak after worker termination; for bounded ones it degrades into a permanentqueue_full/stuck state.This needs to surface the dead-VM result back into the TSFN state machine: either gate the queue write on VM liveness before
write_item(), or transition to a closing state on failed enqueue so futureenqueue()calls fail loudly instead of accepting undeliverable work. Based on the PR context, ThreadSafeFunction calls may originate from addon threads that outlive a terminated worker loop.🤖 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/runtime/napi/napi_body.rs` around lines 2711 - 2723, schedule_dispatch currently sets dispatch_state to Pending and ignores a false return from EventLoop::try_enqueue_task_concurrent, leaving the TSFN stuck; update schedule_dispatch (and related state transitions) so that when EventLoop::try_enqueue_task_concurrent(self.event_loop.as_ptr(), ConcurrentTask::create_from(self_ptr)) returns false you immediately update dispatch_state back to Idle or to a Closing/Terminated state (instead of leaving it Pending) and ensure enqueue()/write_item() checks that state and fails fast when the VM is dead; in practice revert or move from DispatchState::Pending to DispatchState::Idle (or a new Closing state) on failed enqueue, and make enqueue()/write_item() treat non-Idle/non-Pending (or Closing) as an error so future calls do not accept undeliverable work.
🤖 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/jsc/any_task_job.rs`:
- Around line 147-155: The new try_enqueue path drops errors and leaks the
resources acquired for the task; when
VirtualMachine::try_enqueue_task_concurrent(job.vm.as_ptr(),
ConcurrentTask::create(job.any_task.task())) returns an error you must perform
the same teardown the JS-thread callback would have done: release the VM
keepalive/refcount that was pre-acquired and free/drop the ConcurrentTask/boxed
task returned by ConcurrentTask::create (i.e., call the same cleanup functions
or code-path used by the original JS-callback), rather than ignoring the Err, so
every acquisition at create/enqueue is balanced by a release on the failure
path.
In `@src/jsc/event_loop.rs`:
- Around line 1077-1085: The current address-only liveness check using
live_vm_registry::REGISTRY.iter().any(|e| e.loop_ == loop_ as usize) allows ABA
reuse; change the check to use a generation/token paired with the EventLoop
pointer: store a generation field in the registry entry and in the handle that
callers carry, capture that generation under the REGISTRY lock alongside the
pointer, verify both pointer and generation match (e.g., entry.loop_ == loop_ as
usize && entry.gen == captured_gen) before dropping the lock, and only then call
unsafe { (*loop_).enqueue_task_concurrent(task) }; if the generation doesn’t
match, call discard_unqueued_concurrent_task(task) and return false. Ensure the
code paths around live_vm_registry::REGISTRY, the creation of loop handles, and
enqueue_task_concurrent use and propagate the generation token consistently.
- Around line 1061-1086: The exported safe function try_enqueue_task_concurrent
(and the helper discard_unqueued_concurrent_task) currently accepts forgeable
raw pointers and dereferences task, causing UB on dangling pointers; make the
API safe by either marking try_enqueue_task_concurrent and
discard_unqueued_concurrent_task as unsafe and document the caller liveness
requirements, or change the signatures to take an owned/liveness-guaranteeing
type (e.g., NonNull<ConcurrentTaskItem> wrapped in a newtype guaranteeing
ownership or Arc/Handle) so you never call task.as_ref().auto_delete() on a
potential dangling pointer; update all call sites and tests that call
EventLoop::try_enqueue_task_concurrent, and keep the existing use of
enqueue_task_concurrent and live_vm_registry checks but ensure callers uphold
the safety contract if you choose the unsafe route.
In `@src/jsc/VirtualMachine.rs`:
- Around line 489-490: The registry currently keys VMs by raw address (usize),
which allows ABA/stale-pointer reuse; update the VM registration to include a
monotonically-incremented generation/token and return a non-reused handle to
producers (e.g., change the register/unregister path such as
register_vm/unregister_vm to maintain a generation counter per slot and store
(addr,generation) together). Propagate that handle into producer-side operations
(e.g., where Producer or enqueue_from_producer looks up a
VirtualMachine/EventLoop) and make lookup verify both address and generation
match before enqueuing; increment generation on unregister so reused addresses
never validate with older producers. Ensure the generation is stored atomically
(e.g., in the Registry/Entry) so concurrent register/unregister/lookup are safe.
- Around line 3692-3715: The helper VirtualMachine::with_live_vm currently
exposes a safe &VirtualMachine across threads (via the unsafe { &*vm } call)
which violates the Sync safety contract; change with_live_vm to either be
non-public (remove pub) or mark it unsafe (pub unsafe fn with_live_vm) so
callers must uphold cross-thread safety, or better change its callback signature
to only accept a concurrency-safe handle (e.g. &self.event_loop_shared() or an
explicit thread-safe VM handle) instead of &VirtualMachine; update all
cross-thread call sites such as try_enqueue_task_concurrent and
is_shutting_down_or_freed to use the new unsafe API or to extract and pass the
thread-safe subset, and keep the existing MAIN_THREAD_VM fast-path and
live_vm_registry::REGISTRY / unregister_vm locking semantics intact to preserve
correctness.
In `@src/runtime/node/node_fs_stat_watcher.rs`:
- Around line 683-685: The call to VirtualMachine::try_enqueue_task_concurrent
in the helper currently drops its Result, causing leaked StatWatcher/scheduler
refs when enqueue fails; change the helper to return the enqueue result
(bool/Result) instead of ignoring it, propagate that boolean to the callers
(restat() and InitialStatTask::run_owned()), and update those sites to
balance/release the pre-taken StatWatcher ref when the helper returns false so
the ref-transfer is completed on failure; reference the symbols
try_enqueue_task_concurrent, restat, InitialStatTask::run_owned, StatWatcher,
and ctx when locating and updating the code paths.
- Around line 333-338: The try_enqueue call can fail and currently the
heap-allocated Holder at holder_ptr is only reclaimed in update_timer(), causing
a leak if enqueue is rejected; change the error branch after
VirtualMachine::try_enqueue_task_concurrent to reclaim the Holder immediately
(e.g., convert holder_ptr back into a Box and drop it: Box::from_raw(holder_ptr)
or call the Holder's proper destructor) so the allocation is freed on Err,
leaving the successful path unchanged where ownership is transferred to the
enqueued task; ensure you do this at the same call site (around
VirtualMachine::try_enqueue_task_concurrent) to pair acquisition and release.
In `@src/runtime/node/node_fs_watcher.rs`:
- Around line 101-103: The call to VirtualMachine::try_enqueue_task_concurrent
currently swallows a false return which leaks the FSWatchTaskPosix clone and
never calls unref_task(); change FSWatchTaskPosix::enqueue to check the boolean
result of VirtualMachine::try_enqueue_task_concurrent(self.ctx, task) and, if it
returns false, call FSWatchTaskPosix::deinit(self) (the clone that was moved)
and invoke unref_task() before returning an error/early-exit; ensure no
successful-path signals are sent when enqueue fails so run()/deinit() balance is
preserved.
In `@src/runtime/node/node_zlib_binding.rs`:
- Around line 479-490: The enqueue-failure path for
VirtualMachine::try_enqueue_task_concurrent currently returns false without
releasing the write-time references acquired in write() (the intrusive self-ref
and the poll_ref keepalive), leaking the native stream payload; fix by balancing
those refs when enqueue fails — either explicitly call the same deref/release
operations that run_from_js_thread() performs (release the intrusive self-ref
and the poll_ref) in the false branch after
ConcurrentTask::create(Task::init(this)) is rejected, or move that release logic
into the Task/ConcurrentTask Drop implementation so Task::init(this) and the
queue node always release their refs on failure. Ensure the chosen fix uses the
same release mechanism as run_from_js_thread() to avoid double-free.
- Around line 253-255: The VM identity is being matched only by raw address via
CompressionStreamImpl::vm() which lets a reused VM address be mis-associated;
update CompressionStreamImpl::vm() (and any place passing the VM to
VirtualMachine::try_enqueue_task_concurrent / with_live_vm) to pass a stable
identity (e.g., a unique VM handle or include the VM instance pointer plus an
owning marker used by live_vm_registry) so live_vm_registry checks cannot
collide on recycled addresses, and ensure
try_enqueue_task_concurrent/with_live_vm use that stable identity for gating.
Also fix async_job_run to handle the false return from
try_enqueue_task_concurrent: when enqueue fails call
discard_unqueued_concurrent_task and then perform the same cleanup that would
run in run_from_js_thread (call poll_ref().unref(vm), call T::deref(this_ptr),
and release/unpin any pinned buffers) so no refs/keepalives or payloads are
leaked; keep the discard_unqueued_concurrent_task behavior but add explicit
payload/unref cleanup on the enqueue-fail path.
In `@src/runtime/webcore/blob/copy_file.rs`:
- Around line 1834-1842: CopyFileWindows currently holds a borrowed &EventLoop
(`pub event_loop: &'a jsc::event_loop::EventLoop`) which is converted to a raw
pointer with core::ptr::from_ref in on_mkdirp_complete_concurrent before calling
jsc::event_loop::EventLoop::try_enqueue_task_concurrent, risking UB if the
VM/worker was terminated; change CopyFileWindows to store a raw pointer (*mut or
*const jsc::event_loop::EventLoop) like WriteFileWindows does, update its
constructor and all uses (including the field name event_loop and any places
that take &self.event_loop) so on_mkdirp_complete_concurrent passes that raw
pointer directly into try_enqueue_task_concurrent, and ensure any unsafe
dereferences are done only where correctness/liveness is already guarded.
In `@src/runtime/webcore/fetch/FetchTasklet.rs`:
- Around line 382-386: FetchTasklet currently stores javascript_vm as a &'static
VirtualMachine which can become a dangling Rust reference; change the field type
(FetchTasklet.javascript_vm) to a raw pointer (e.g., *mut VirtualMachine or
NonNull<VirtualMachine>) and update all sites that do
core::ptr::from_ref(...).cast_mut() to use that raw pointer directly, call
VirtualMachine::is_shutting_down_or_freed(vm_ptr) and/or use
with_live_vm/live_vm_registry to prove liveness first, and only create a safe
reference (dereference) after liveness is confirmed; update any helper functions
and pattern matches that assume a &'static to accept the raw pointer type
instead.
In `@test/js/web/workers/worker-terminate-lifetime.test.ts`:
- Around line 183-189: Replace the unconditional discard of stderr (the "void
stderr;" line) with an unconditional assertion that proc.stderr (the captured
stderr string) does not contain AddressSanitizer/ASAN sanitizer report markers
before any other assertions; specifically, read stderr from the Promise result
(the stderr variable alongside stdout/exitCode), assert it does not match common
sanitizer signatures (e.g., "AddressSanitizer" / "ERROR: AddressSanitizer" /
"heap-use-after-free") and only then proceed to assert stdout, exitCode and
proc.signalCode; update the test in worker-terminate-lifetime.test.ts to check
stderr first and remove the silent discard.
---
Outside diff comments:
In `@src/jsc/ConcurrentPromiseTask.rs`:
- Around line 111-121: The current code only captures EventLoop pointer
(`event_loop`) and calls
EventLoop::try_enqueue_task_concurrent(event_loop.as_ptr(), task), which can
accept completions into the wrong VM if a new VM reuses the same EventLoop
address; instead capture the owning VirtualMachine pointer at task creation and
call VirtualMachine::try_enqueue_task_concurrent(...) so admission checks use VM
liveness. Update the code that constructs the task (referencing
this_ref.event_loop and this_ref.concurrent_task.from(...)) to also capture the
owning `*mut VirtualMachine` and replace the
EventLoop::try_enqueue_task_concurrent call with
VirtualMachine::try_enqueue_task_concurrent, and sweep other sites that only
pass an EventLoop pointer to use the VM-based enqueue API.
- Around line 111-121: The checked enqueue call
(EventLoop::try_enqueue_task_concurrent) can fail when the target VM is gone,
which currently drops the ManualDeinit-owned payload (ConcurrentPromiseTask /
WorkTask) and leaks or leaves promise/context in an inconsistent state; change
the branch that handles a false return from try_enqueue_task_concurrent to
perform the same explicit teardown/recycle currently done in the normal
completion path: invoke the task’s failure teardown (call the equivalent of
run_from_js()/run_from_js_thread() or destroy() logic that performs
promise.deinit(), reset_for_pool(), and releases any keepalive/context), and
return the AutoDeinit owner back to its pool/slot so the allocation is recycled;
ensure this cleanup code is added at the enqueue site (next to where
AutoDeinit::ManualDeinit is created) so every acquisition is paired with a
single release even when with_live_vm()/try_enqueue_task_concurrent() reports
the VM is gone.
In `@src/runtime/napi/napi_body.rs`:
- Around line 2711-2723: schedule_dispatch currently sets dispatch_state to
Pending and ignores a false return from EventLoop::try_enqueue_task_concurrent,
leaving the TSFN stuck; update schedule_dispatch (and related state transitions)
so that when EventLoop::try_enqueue_task_concurrent(self.event_loop.as_ptr(),
ConcurrentTask::create_from(self_ptr)) returns false you immediately update
dispatch_state back to Idle or to a Closing/Terminated state (instead of leaving
it Pending) and ensure enqueue()/write_item() checks that state and fails fast
when the VM is dead; in practice revert or move from DispatchState::Pending to
DispatchState::Idle (or a new Closing state) on failed enqueue, and make
enqueue()/write_item() treat non-Idle/non-Pending (or Closing) as an error so
future calls do not accept undeliverable work.
🪄 Autofix (Beta)
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: 3882630f-b7ea-449e-ae75-17faf431c1d0
📒 Files selected for processing (29)
src/jsc/AsyncModule.rssrc/jsc/ConcurrentPromiseTask.rssrc/jsc/CppTask.rssrc/jsc/JSCScheduler.rssrc/jsc/RuntimeTranspilerStore.rssrc/jsc/VirtualMachine.rssrc/jsc/WorkTask.rssrc/jsc/any_task_job.rssrc/jsc/event_loop.rssrc/jsc/web_worker.rssrc/runtime/api/Archive.rssrc/runtime/api/js_bundle_completion_task.rssrc/runtime/bake/dev_server/mod.rssrc/runtime/crypto/PasswordObject.rssrc/runtime/napi/napi_body.rssrc/runtime/node/node_fs.rssrc/runtime/node/node_fs_stat_watcher.rssrc/runtime/node/node_fs_watcher.rssrc/runtime/node/node_zlib_binding.rssrc/runtime/node/zlib/NativeBrotli.rssrc/runtime/node/zlib/NativeZlib.rssrc/runtime/node/zlib/NativeZstd.rssrc/runtime/shell/shell_body.rssrc/runtime/webcore/blob/copy_file.rssrc/runtime/webcore/blob/write_file.rssrc/runtime/webcore/fetch/FetchTasklet.rssrc/runtime/webcore/s3/download_stream.rssrc/runtime/webcore/s3/simple_request.rstest/js/web/workers/worker-terminate-lifetime.test.ts
…bility - FetchTasklet.javascript_vm and CopyFileWindows.event_loop become BackRef instead of Rust references (same pattern as the S3 tasks): a worker VM can be freed while these structs are still referenced from the HTTP/pool threads, so holding a reference would dangle. JS-thread paths get() them; cross-thread paths only pass as_ptr() to the checked accessors. Resolves the struct's own TODO in copy_file.rs. - with_live_vm becomes pub(crate): the closure receives &VirtualMachine on a non-JS thread and must stick to the documented thread-safe subset, so don't expose it outside the crate. - StatWatcherScheduler frees its timer-update Holder when the checked enqueue is rejected (plain ParentRef + AnyTask, safe off-thread). - FSWatchTaskPosix::enqueue reclaims the cloned task box and balances the pending-activity ref when the checked enqueue is rejected (both are watcher-thread-safe: plain heap entries, atomic counter). - Document the residual address-reuse (ABA) window on the registry: pre-existing, strictly narrower than the bug this fixes, and closing it needs schedule-time generation tokens in every producer struct (follow-up). - Regression test asserts stderr contains no AddressSanitizer report so failures surface the report text.
|
Responses to the three outside-diff findings from the review (no inline threads to resolve for these): ConcurrentPromiseTask/WorkTask should capture the VM instead of the EventLoop pointer: declined as a no-op today. The ConcurrentPromiseTask/WorkTask/TranspilerJob teardown on rejected enqueue: declined for the same reason as the resolved any_task_job thread. Their cleanup paths ( ThreadSafeFunction left |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/runtime/webcore/fetch/FetchTasklet.rs (2)
404-409:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the shutdown-reclaim comment.
dealloc_for_shutdown()no longer "reclaims only the Rust-side boxes" on the HTTP thread. It parks the intact box and later runsdeinit()on the JS thread, so this comment now describes the old behavior and weakens the safety story around the fallback path. As per coding guidelines, "Comments must be load-bearing and true."🤖 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/runtime/webcore/fetch/FetchTasklet.rs` around lines 404 - 409, The shutdown comment is out of date: update the block above the unsafe call to FetchTasklet::dealloc_for_shutdown(this) to accurately state that dealloc_for_shutdown no longer reclaims only Rust-side boxes on the HTTP thread but instead parks the intact box and schedules deinit() to run later on the JS thread (with destructOnExit still freeing the HandleSet), and adjust the safety rationale to explain why calling dealloc_for_shutdown here is safe (i.e., it does not touch JS HandleSet, it only parks state for JS-thread deinit(), and any fallback path that runs deinit() on exit happens on the JS thread), referencing FetchTasklet::dealloc_for_shutdown, deinit(), clear_data(), and destructOnExit to make the updated behavior and thread-safety guarantees explicit.Source: Coding guidelines
390-400:⚠️ Potential issue | 🟠 MajorRaw-address VM liveness can still cause ABA mis-enqueue for stale
FetchTasklets.In
FetchTasklet::deref_from_thread, the enqueue is gated byVirtualMachine::is_shutting_down_or_freed(vm_ptr)and thenVirtualMachine::try_enqueue_task_concurrent(vm_ptr, ...), wherevm_ptris taken fromself_.javascript_vm.as_ptr()and the registry liveness check is keyed byvm as usizeonly (src/jsc/VirtualMachine.rs/live_vm_registry).
src/jsc/VirtualMachine.rs’s own doc comment notes that to eliminate mis-delivery on address reuse you need a schedule-time generation token carried by producers; the current registry has no such token. If a worker VM is destroyed and a new VM reuses the same address before this HTTP-thread completion runs,try_enqueue_task_concurrentcan accept the stale pointer and enqueue into the new VM’s loop, after whichFetchTasklet::deinit()can drop JSCStrong/Weakfields under the wrong VM/handle-set/threading assumptions.Fix by carrying a non-reused identity (generation/epoch token) from schedule-time into the queued task and validating it in the enqueue/liveness fast-path, not just “vm address is currently registered”.
🤖 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/runtime/webcore/fetch/FetchTasklet.rs` around lines 390 - 400, The enqueue uses a raw vm pointer and can mis-deliver tasks when an address is reused; capture a non-reused generation token at schedule-time and validate it during enqueue. Modify FetchTasklet::deref_from_thread to read both self_.javascript_vm.as_ptr() and a VM generation token (e.g. VirtualMachine::generation(vm_ptr) or add a getter) and pass that token into ConcurrentTask::from_callback (augment ConcurrentTask payload to carry vm_generation). Update VirtualMachine::try_enqueue_task_concurrent to check both the vm_ptr and the carried vm_generation against the live_vm_registry entry (reject if generation mismatches) before accepting the task; ensure FetchTasklet::deinit_callback still receives the token and refuses to operate if the token no longer matches.
♻️ Duplicate comments (2)
src/runtime/node/node_fs_stat_watcher.rs (1)
683-690:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate failed enqueue back to the ref-transfer sites.
This helper still swallows
try_enqueue_task_concurrent()returningfalse.restat()takes a watcher ref at Line 946, andInitialStatTask::run_owned()transfers the ref taken increate_and_schedule()at Lines 1218-1231; when the VM is already gone, neither main-thread callback runs to release that ref, so theStatWatcherand its scheduler ref leak on worker termination. Make this returnbool/#[must_use]and have those callsites balance their pre-taken ref on failure.As per coding guidelines, “every error/abort/timeout path actively completes the operation” and “pair every acquisition with its release at the acquisition site using Drop/RAII guards.”
🤖 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/runtime/node/node_fs_stat_watcher.rs` around lines 683 - 690, The helper enqueue_task_concurrent currently swallows try_enqueue_task_concurrent() failures causing leaked StatWatcher/scheduler refs; change enqueue_task_concurrent to return bool and mark it #[must_use], have it return the boolean result of VirtualMachine::try_enqueue_task_concurrent(self.ctx.as_ptr(), task), and update callers (restat and InitialStatTask::run_owned / create_and_schedule) to check the return value and explicitly release/balance the pre-taken watcher ref when it returns false (i.e., perform the same ref-drop/cleanup that would have happened if the task had been enqueued) so every acquisition is paired with a release at the acquisition site.Source: Coding guidelines
src/jsc/VirtualMachine.rs (1)
494-502:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftAddress-only liveness still allows stale producers to target the wrong worker.
Entry { vm: usize, .. }plusreg.iter().any(|e| e.vm == vm as usize)only proves that some live VM currently occupies that address. If worker A dies and worker B is later allocated at A’s address, a stale producer for A will passwith_live_vm()and enqueue/ref/unref against B. That turns the original UAF into cross-worker task misdelivery/state corruption instead of actually fencing stale pointers. Please switch this registry to a non-reused identity (generation/token/handle) captured at schedule time and validate both pointer and generation here.Also applies to: 512-549, 3709-3732
🤖 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/jsc/VirtualMachine.rs` around lines 494 - 502, The registry currently uses address-only identity (Entry { vm: usize } and checks like reg.iter().any(|e| e.vm == vm as usize)) which allows stale producers to match a newly allocated VM at the same address; change the registry Entry to include a non-reused generation/token (e.g., add a generation: u64 or Handle type), ensure producers capture that generation at schedule time, and update validation sites (notably with_live_vm, the checks around reg.iter().any(|e| e.vm == vm as usize) and other uses in the ranges referenced) to require both vm pointer and generation to match before enqueuing/ref/unref so stale pointers are correctly fenced. Ensure creation/alloc increments or creates unique generation values and that producer structs store the generation alongside the vm pointer captured at schedule time.
🤖 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.
Outside diff comments:
In `@src/runtime/webcore/fetch/FetchTasklet.rs`:
- Around line 404-409: The shutdown comment is out of date: update the block
above the unsafe call to FetchTasklet::dealloc_for_shutdown(this) to accurately
state that dealloc_for_shutdown no longer reclaims only Rust-side boxes on the
HTTP thread but instead parks the intact box and schedules deinit() to run later
on the JS thread (with destructOnExit still freeing the HandleSet), and adjust
the safety rationale to explain why calling dealloc_for_shutdown here is safe
(i.e., it does not touch JS HandleSet, it only parks state for JS-thread
deinit(), and any fallback path that runs deinit() on exit happens on the JS
thread), referencing FetchTasklet::dealloc_for_shutdown, deinit(), clear_data(),
and destructOnExit to make the updated behavior and thread-safety guarantees
explicit.
- Around line 390-400: The enqueue uses a raw vm pointer and can mis-deliver
tasks when an address is reused; capture a non-reused generation token at
schedule-time and validate it during enqueue. Modify
FetchTasklet::deref_from_thread to read both self_.javascript_vm.as_ptr() and a
VM generation token (e.g. VirtualMachine::generation(vm_ptr) or add a getter)
and pass that token into ConcurrentTask::from_callback (augment ConcurrentTask
payload to carry vm_generation). Update
VirtualMachine::try_enqueue_task_concurrent to check both the vm_ptr and the
carried vm_generation against the live_vm_registry entry (reject if generation
mismatches) before accepting the task; ensure FetchTasklet::deinit_callback
still receives the token and refuses to operate if the token no longer matches.
---
Duplicate comments:
In `@src/jsc/VirtualMachine.rs`:
- Around line 494-502: The registry currently uses address-only identity (Entry
{ vm: usize } and checks like reg.iter().any(|e| e.vm == vm as usize)) which
allows stale producers to match a newly allocated VM at the same address; change
the registry Entry to include a non-reused generation/token (e.g., add a
generation: u64 or Handle type), ensure producers capture that generation at
schedule time, and update validation sites (notably with_live_vm, the checks
around reg.iter().any(|e| e.vm == vm as usize) and other uses in the ranges
referenced) to require both vm pointer and generation to match before
enqueuing/ref/unref so stale pointers are correctly fenced. Ensure
creation/alloc increments or creates unique generation values and that producer
structs store the generation alongside the vm pointer captured at schedule time.
In `@src/runtime/node/node_fs_stat_watcher.rs`:
- Around line 683-690: The helper enqueue_task_concurrent currently swallows
try_enqueue_task_concurrent() failures causing leaked StatWatcher/scheduler
refs; change enqueue_task_concurrent to return bool and mark it #[must_use],
have it return the boolean result of
VirtualMachine::try_enqueue_task_concurrent(self.ctx.as_ptr(), task), and update
callers (restat and InitialStatTask::run_owned / create_and_schedule) to check
the return value and explicitly release/balance the pre-taken watcher ref when
it returns false (i.e., perform the same ref-drop/cleanup that would have
happened if the task had been enqueued) so every acquisition is paired with a
release at the acquisition site.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 670b0257-d77f-4bc3-968a-1b4e200e34e5
📒 Files selected for processing (6)
src/jsc/VirtualMachine.rssrc/runtime/node/node_fs_stat_watcher.rssrc/runtime/node/node_fs_watcher.rssrc/runtime/webcore/blob/copy_file.rssrc/runtime/webcore/fetch/FetchTasklet.rstest/js/web/workers/worker-terminate-lifetime.test.ts
When the HTTP thread dropped the last tasklet reference after a worker was terminated, the previous code parked the box via dealloc_for_shutdown, whose global_exit drain runs deinit() on the main thread: that drops the tasklet's Strong/Weak handles into the dead worker VM's freed HandleSet (invisible to ASAN because the JSC heap is bmalloc-backed). Split the two cases with VirtualMachine::live_shutting_down_state: a VM observed registered and shutting down can only be the never-freed main VM (workers unregister before setting the flag), so the park-for-deinit path stays for process exit; a freed worker's tasklet is parked permanently in a reachable static instead, so nothing ever touches its dead JSC state and LSan-enabled CI lanes don't report it. Also rewrites the stale comments flagged in review that still described dealloc_for_shutdown as reclaiming Rust-side boxes on the HTTP thread.
|
Responses to the review-body findings on 007f64b (no inline threads for these): FetchTasklet: stale shutdown-reclaim comment and the deeper issue behind it: fixed in 64bafc1. The old path parked a dead worker's tasklet via FetchTasklet ABA on the raw vm pointer and registry address-only identity (duplicate): same residual as the resolved threads, documented on the registry and tracked in #32073 (producer-carried generation tokens). Not re-litigating here. StatWatcher ref propagation (duplicate): still declined, now with the concrete evidence from |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/jsc/VirtualMachine.rs (2)
2231-2234:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRegister the VM only after the last fallible init step.
register_vm(vm)runs beforeinit_runtime_state(...)?. If that hook errors,init()returns with a stale live-registry entry for a VM that never finished initialization, so laterwith_live_vm/try_enqueue_task_concurrentcalls can incorrectly treat it as live.🩹 Minimal fix
- // Make this VM reachable for checked cross-thread enqueues. - // Worker VMs are unregistered in `WebWorker::shutdown()` before - // the allocation is freed; the main VM stays registered forever. - live_vm_registry::register_vm(vm); - // `source_mappings.map` is a sibling-field backref onto // `saved_source_map_table`. addr_of_mut!((*vm).saved_source_map_table) .write(crate::saved_source_map::HashTable::default()); @@ if let Some(hooks) = runtime_hooks() { // SAFETY: hook contract — `vm` is the unique live VM on this // thread. Write through the raw `vm` ptr (not `vm_ref`) so no // `&mut VirtualMachine` is held live across the hook call — the // hook body itself dereferences `vm`. @@ unsafe { (*vm).runtime_state = (hooks.init_runtime_state)(vm, &mut opts)? }; } + + // Make this VM reachable for checked cross-thread enqueues only after + // all fallible init work above has succeeded. + live_vm_registry::register_vm(vm);As per coding guidelines, "Pair every acquisition with its release at the acquisition site" and "Prefer validate-first-allocate-last so error paths have nothing to clean up."
Also applies to: 2254-2266
🤖 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/jsc/VirtualMachine.rs` around lines 2231 - 2234, The VM is being registered too early (live_vm_registry::register_vm(vm)) before the last fallible initialization init_runtime_state(...)?, which can leave a stale live entry on error; move the register_vm call to after init_runtime_state(...) completes successfully (i.e., at the end of init()) so acquisition is paired with its release and errors won’t leave the VM visible to with_live_vm / try_enqueue_task_concurrent; apply the same change to the other similar block referenced around lines 2254-2266 and ensure WebWorker::shutdown() still unregisters worker VMs as before.Source: Coding guidelines
3750-3752:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftAvoid reading JS-thread-owned VM fields from these cross-thread helpers.
These closures run on producer threads, but
vm.event_loop_shared()reads the mutableevent_loopselector andvm.is_shutting_down()reads a plainbool. Both are written on the JS thread (enable_macro_mode/disable_macro_mode, shutdown), so this introduces unsynchronized cross-thread reads and violates theSynccontract. The concurrent path needs a stable enqueue target plus an atomic shutdown bit instead of borrowing&VirtualMachinehere.As per coding guidelines, "Know the thread affinity of every line you touch" and "benign same-value races are still UB."
Also applies to: 3776-3778, 3783-3788
🤖 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/jsc/VirtualMachine.rs` around lines 3750 - 3752, The closure passed to Self::with_live_vm is performing unsynchronized cross-thread reads (vm.event_loop_shared() and vm.is_shutting_down()) which are JS-thread owned; change the concurrent path to capture a thread-safe enqueue target and an atomic shutdown bit instead of borrowing &VirtualMachine. Add or use a method that returns a cloned Arc/handle to the EventLoopShared (e.g., an Arc<EventLoopShared> or dedicated enqueue_target) and expose an AtomicBool shutdown flag or accessor, then call enqueue_task_concurrent on that cloned handle from producer threads and check the atomic shutdown bit; update places using with_live_vm (including enqueue_task_concurrent call sites around with_live_vm and occurrences at 3776-3778, 3783-3788) to use the new thread-safe enqueue API rather than calling vm.event_loop_shared() or vm.is_shutting_down() inside the producer-thread closure.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/webcore/fetch/FetchTasklet.rs`:
- Around line 401-411: The code trusts javascript_vm.as_ptr() and
VirtualMachine::live_shutting_down_state/try_enqueue_task_concurrent to identify
a live VM by raw address, which allows address-reuse corruption; change the VM
liveness checks and task enqueue path to use a non-reusable identity (e.g., a
generation-stable VM id or handle) rather than the bare pointer: add a
monotonically-incremented generation id on VM creation, include that id in the
live registry entry and in ConcurrentTask::from_callback payload, and update
VirtualMachine::live_shutting_down_state and
VirtualMachine::try_enqueue_task_concurrent to validate both pointer and
generation (or consult the handle) before enqueuing/deinit (including for
FetchTasklet::deinit_callback) so late completions cannot act on a different VM
allocated at the same address.
- Around line 27-35: DEAD_VM_TASKLETS is append-only and makes retained
FetchTasklet graphs leak for the process lifetime; change the strategy so parked
tasklets are not permanently retained: modify where FetchTasklet::deinit() and
dealloc_for_shutdown()/global_exit drain would park items to instead store
either weak references/IDs plus metadata (or a small fixed-size ring buffer) in
DEAD_VM_TASKLETS and implement a cleanup/prune step that removes entries when
their VM is confirmed freed or on shutdown; ensure the prune runs from
dealloc_for_shutdown/global_exit and add bounded-cap semantics (or
timestamp-based eviction) so repeated worker churn cannot grow unbounded.
---
Outside diff comments:
In `@src/jsc/VirtualMachine.rs`:
- Around line 2231-2234: The VM is being registered too early
(live_vm_registry::register_vm(vm)) before the last fallible initialization
init_runtime_state(...)?, which can leave a stale live entry on error; move the
register_vm call to after init_runtime_state(...) completes successfully (i.e.,
at the end of init()) so acquisition is paired with its release and errors won’t
leave the VM visible to with_live_vm / try_enqueue_task_concurrent; apply the
same change to the other similar block referenced around lines 2254-2266 and
ensure WebWorker::shutdown() still unregisters worker VMs as before.
- Around line 3750-3752: The closure passed to Self::with_live_vm is performing
unsynchronized cross-thread reads (vm.event_loop_shared() and
vm.is_shutting_down()) which are JS-thread owned; change the concurrent path to
capture a thread-safe enqueue target and an atomic shutdown bit instead of
borrowing &VirtualMachine. Add or use a method that returns a cloned Arc/handle
to the EventLoopShared (e.g., an Arc<EventLoopShared> or dedicated
enqueue_target) and expose an AtomicBool shutdown flag or accessor, then call
enqueue_task_concurrent on that cloned handle from producer threads and check
the atomic shutdown bit; update places using with_live_vm (including
enqueue_task_concurrent call sites around with_live_vm and occurrences at
3776-3778, 3783-3788) to use the new thread-safe enqueue API rather than calling
vm.event_loop_shared() or vm.is_shutting_down() inside the producer-thread
closure.
🪄 Autofix (Beta)
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: 25de61b0-7923-41fb-8217-ef38f89b820c
📒 Files selected for processing (2)
src/jsc/VirtualMachine.rssrc/runtime/webcore/fetch/FetchTasklet.rs
…rking - VirtualMachine::init now registers in the live-VM registry as its final step, so an init_runtime_state error cannot leave a stale entry. - is_shutting_down becomes AtomicBool and the event_loop selector becomes AtomicPtr: the checked cross-thread enqueue helpers read both from producer threads while the JS thread writes them. - FetchTasklet frees its plain-heap buffers (response/header/URL) on the HTTP thread before parking in DEAD_VM_TASKLETS; only the handles whose teardown would touch the dead JSC heap stay parked.
|
Both outside-diff findings from the latest review are addressed in 575acf4:
Unsynchronized cross-thread reads in
Everything else the closures touch was already thread-safe: the MPSC queue push takes Verified: |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/jsc/VirtualMachine.rs (1)
2154-2156:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPublish
MAIN_THREAD_VMonly after the main VM is fully live.Line 2154 stores
MAIN_THREAD_VMbefore Line 2254 initializesevent_loop, and long before Line 2349 registers the VM as live. Butwith_live_vm()treatsMAIN_THREAD_VMas an immortal fast path and dereferences it without the registry lock. That means a cross-thread producer can bypass the new liveness fence and touch a partially initialized main VM; the worst case here is loading a nullevent_loopfrom the zeroed allocation and dereferencing it.Suggested fix
- if opts.is_main_thread { - MAIN_THREAD_VM.store(vm, core::sync::atomic::Ordering::Release); - } ... if opts.smol { // SAFETY: written once during init. IS_SMOL_MODE.store(true, core::sync::atomic::Ordering::Relaxed); } // Make this VM reachable for checked cross-thread enqueues. Last step // of `init` so the fallible ones above (`init_runtime_state`) cannot // leave a stale entry behind on an `Err` return. Worker VMs are // unregistered in `WebWorker::shutdown()` before the allocation is // freed; the main VM stays registered forever. + if opts.is_main_thread { + MAIN_THREAD_VM.store(vm, core::sync::atomic::Ordering::Release); + } live_vm_registry::register_vm(vm);Also applies to: 3736-3751
🤖 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/jsc/VirtualMachine.rs` around lines 2154 - 2156, The publish of MAIN_THREAD_VM is happening too early; change the flow so that MAIN_THREAD_VM.store(...) is called only after the VM is fully initialized and registered live (i.e., after the event_loop is set up and after whatever function registers the VM as live), so that with_live_vm() cannot observe a partially-initialized VM; locate the early store in VirtualMachine::... (the block that checks opts.is_main_thread) and move it to follow the event_loop initialization and the VM liveness registration call (and apply the same fix in the other occurrence around the 3736-3751 region) so MAIN_THREAD_VM is written after the registry lock/fence path is complete.src/runtime/webcore/fetch/FetchTasklet.rs (1)
1763-1765:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate this stale lifetime invariant.
This safety comment still describes the old
'staticworld (“the VM outlives this tasklet”, “process-lifetime singleton”), but worker VMs are exactly the case that can disappear first. Leaving that invariant here makes the surrounding unsafe/lifetime reasoning backwards.As per coding guidelines, comments must be “load-bearing and true,” and a comment contradicting the current code is a correctness bug.
🤖 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/runtime/webcore/fetch/FetchTasklet.rs` around lines 1763 - 1765, The safety comment above the BackRef creation is stale and asserts the VM outlives the tasklet; update it to reflect the current lifetime invariant: explain that bun_vm() returns an FFI *mut VirtualMachine which must remain valid for the duration of the BackRef usage, and that worker VMs can be destroyed before tasklets—so the code relies on external synchronization/drop ordering to ensure the VM is alive while this BackRef is used. Edit the comment near the FetchTasklet code that calls global_this.bun_vm() / bun_ptr::BackRef::new to state the correct, load-bearing invariant (VM must be kept alive for the scope of this BackRef) rather than claiming process-lifetime/static longevity.Source: Coding guidelines
src/runtime/node/node_fs.rs (1)
1327-1330:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRejected dead-VM enqueues still leak the full FS payload.
When
VirtualMachine::try_enqueue_task_concurrent()returnsfalse, only the queue node is discarded. These two call sites then leak the already-produced task payloads:AsyncFSTask::resultcan hold largereadFile/readdiroutputs, andAsyncReaddirRecursiveTaskhas already drained its recursiveresult_liston the worker thread before this enqueue attempt. Repeated worker-termination races can therefore accumulate large, user-controlled memory leaks. Please reclaim the pure-data result state on thefalsepath here and apply the same policy across both task types, leaving only JS-thread-only state parked if it truly cannot be torn down safely. As per coding guidelines, "pair every acquisition with its release at the acquisition site" and "fix the whole bug class in the same PR".Also applies to: 2567-2570
🤖 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/runtime/node/node_fs.rs` around lines 1327 - 1330, The enqueue call using VirtualMachine::try_enqueue_task_concurrent with ConcurrentTask::create_from currently drops only the queue node when it returns false, leaking large payloads held by AsyncFSTask::result and AsyncReaddirRecursiveTask::result_list; modify the false-return branch immediately after the try_enqueue_task_concurrent call to explicitly reclaim/free the task payload data (clear or take AsyncFSTask::result and drain/free AsyncReaddirRecursiveTask::result_list) before abandoning the task, ensuring any JS-thread-only handles remain if needed but all pure-data buffers are released; apply the same explicit reclamation at both call sites (the shown site and the one around lines 2567-2570) so acquisitions are paired with releases.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/webcore/fetch/FetchTasklet.rs`:
- Around line 448-466: The dead-VM cleanup in free_native_data_for_dead_vm
currently skips tearing down request_body which leaks HTTPRequestBody::Sendfile
fds; update free_native_data_for_dead_vm to inspect and take self.request_body
(or call its detach path) and explicitly close the sendfile fd and zero offsets
for the Sendfile variant before abandoning the tasklet (mirror
HTTPRequestBody::detach behavior), ensuring the operation is safe to run on the
native side and idempotent if already closed; reference the request_body field,
the free_native_data_for_dead_vm method, and the
HTTPRequestBody::Sendfile/HTTPRequestBody::detach logic when implementing the
fix.
---
Outside diff comments:
In `@src/jsc/VirtualMachine.rs`:
- Around line 2154-2156: The publish of MAIN_THREAD_VM is happening too early;
change the flow so that MAIN_THREAD_VM.store(...) is called only after the VM is
fully initialized and registered live (i.e., after the event_loop is set up and
after whatever function registers the VM as live), so that with_live_vm() cannot
observe a partially-initialized VM; locate the early store in
VirtualMachine::... (the block that checks opts.is_main_thread) and move it to
follow the event_loop initialization and the VM liveness registration call (and
apply the same fix in the other occurrence around the 3736-3751 region) so
MAIN_THREAD_VM is written after the registry lock/fence path is complete.
In `@src/runtime/node/node_fs.rs`:
- Around line 1327-1330: The enqueue call using
VirtualMachine::try_enqueue_task_concurrent with ConcurrentTask::create_from
currently drops only the queue node when it returns false, leaking large
payloads held by AsyncFSTask::result and AsyncReaddirRecursiveTask::result_list;
modify the false-return branch immediately after the try_enqueue_task_concurrent
call to explicitly reclaim/free the task payload data (clear or take
AsyncFSTask::result and drain/free AsyncReaddirRecursiveTask::result_list)
before abandoning the task, ensuring any JS-thread-only handles remain if needed
but all pure-data buffers are released; apply the same explicit reclamation at
both call sites (the shown site and the one around lines 2567-2570) so
acquisitions are paired with releases.
In `@src/runtime/webcore/fetch/FetchTasklet.rs`:
- Around line 1763-1765: The safety comment above the BackRef creation is stale
and asserts the VM outlives the tasklet; update it to reflect the current
lifetime invariant: explain that bun_vm() returns an FFI *mut VirtualMachine
which must remain valid for the duration of the BackRef usage, and that worker
VMs can be destroyed before tasklets—so the code relies on external
synchronization/drop ordering to ensure the VM is alive while this BackRef is
used. Edit the comment near the FetchTasklet code that calls
global_this.bun_vm() / bun_ptr::BackRef::new to state the correct, load-bearing
invariant (VM must be kept alive for the scope of this BackRef) rather than
claiming process-lifetime/static longevity.
🪄 Autofix (Beta)
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: dc3380dc-99cf-4990-a864-bafaa97edb54
📒 Files selected for processing (8)
src/jsc/VirtualMachine.rssrc/jsc/event_loop.rssrc/jsc/web_worker.rssrc/runtime/cli/test/parallel/runner.rssrc/runtime/cli/test_command.rssrc/runtime/jsc_hooks.rssrc/runtime/node/node_fs.rssrc/runtime/webcore/fetch/FetchTasklet.rs
Also close the Sendfile request-body fd before parking a dead-VM FetchTasklet (fds are process-wide, unlike the parked heap bytes), and fix the stale lifetime comment at the javascript_vm BackRef creation.
|
The three outside-diff findings from the latest review:
Stale lifetime comment at the Rejected dead-VM enqueues leak the FS task payload (node_fs.rs 1327-1330, 2567-2570): declined, because the payloads are not safe to free on the pool thread. |
|
Current state: the change is complete and review-clean (all review threads resolved, including Jarred's "the VM handle must be the only source of truth": no cross-thread producer stores a raw VM or EventLoop pointer, and the C++ captures carry the generation next to Verification: the worker-lifetime test file (6 tests) passes on the ASAN debug build and fail-before holds for the UAF repro; CI on the latest full run (build 71966, d680781): 284 jobs passed, 2 red, all three failures unrelated to this diff:
The earlier open question (a single unattributed Pre-existing findings surfaced during review/CI, each reproduced or verified and tracked separately rather than widening this PR: #32176 (test-runner exit allocations vs LSan on the asan lane), #32179 (serve-body-leak's zero-headroom 512 MB cap under ASAN), #33313 ( This is ready for maintainer review. The permitted CI re-roll for this branch is long spent, so I will not push empty retriggers for known-unrelated red lanes. |
Fixes #32073. Stacked on #32071 (base branch is that PR's branch; retargets to main when it lands). ### Problem The live-VM registry from #32071 keys liveness by address only. Its known residual, documented in the registry's doc comment: if a new VM is allocated at a dead worker VM's address, a stale producer that captured the old pointer passes the check and its completion is delivered to the new VM instead of being dropped. A registry-side counter alone cannot close this, because producers only bring an address to the lookup; the token has to be captured at schedule time and carried by the producer. ### Fix - Every registration in `live_vm_registry` is stamped with a process-unique generation (`AtomicU64`, starts at 1; 0 is the never-matches value of placeholder handles). The generation is also stamped into `VirtualMachine.live_generation` and each `EventLoop.live_generation` so handles can be built without the lock. `enable_macro_mode` re-stamps the macro loop it recreates. - New `Copy` handle types: `VmHandle { addr, generation }` (from `VirtualMachine::concurrent_handle()`) and `LoopHandle` (from `EventLoop::concurrent_handle()`, for producers that must deliver to the exact captured loop: regular vs macro, or the boxed spawnSync loop, which gets its own generation in `register_extra_loop`). - The checked entry points (`VirtualMachine::try_enqueue_task_concurrent`, `EventLoop::try_enqueue_task_concurrent`, `with_live_vm`, `is_shutting_down_or_freed`, `try_ref/unref_concurrently`) take the handle and verify `(addr, generation)` under the registry lock. The lock-free main-VM fast paths compare the generation too; a stale pre-registration read only causes a spurious miss into the locked path. - Every Rust producer that stored `*mut VirtualMachine` / `BackRef<EventLoop>` for completion delivery now stores the handle: FetchTasklet, S3 simple/list/download tasks, WorkTask, ConcurrentPromiseTask, AnyTaskJob, TranspilerJob dispatch, node:fs async tasks, zlib native streams (NativeZlib/NativeBrotli/NativeZstd), napi async_work and TSFN dispatch, FSWatcher/StatWatcher, PasswordObject, Archive async tasks, DevServer watcher events, `Bun.build` completion, and blob copy_file/write_file on Windows. - `EventLoopHandle::Js` / `AnyEventLoop::Js` (bun_event_loop) gain a `generation` field captured at construction, and the `JsEventLoop::enqueue_task_concurrent` dispatch carries it, covering the process waiter thread, shell tasks, and the bundler plugin/parse-task completions in one place. `EventLoopHandle::from_tag_ptr` re-reads the generation under its existing still-live contract. - The package manager's `WakeHandler` carries the generation next to its context pointer so the auto-install wake (`AsyncModule::on_wake_handler`) can reassemble the handle. Structs whose VM backref is also used on the JS thread (FSWatcher, StatWatcher/scheduler, DevServer, TSFN) keep that pointer and add the handle for the one cross-thread access; single-purpose fields were converted outright. `GlobalJS::enqueue_task_concurrent_wait_pid` (shell) is deleted: it had no callers and computed the VM from the global object at call time on non-JS threads, which cannot carry a schedule-time handle. ### Remaining address-only checks (explicitly named `*_addr_only`) Pointers captured by C++ cannot carry a generation without C++-side plumbing, so `JSVMClientData::bunVM` (JSCScheduler: `Bun__queueJSCDeferredWorkTaskConcurrently`, `Bun__eventLoop__incrementRefConcurrently`), `EventLoopTaskNoContext` (webcrypto CppTask ref/unref), and the napi finalizer's env-derived VM stay address-checked, which is exactly #32071's behavior for them. They are named `*_addr_only` so the residual is greppable. TSFN acceptance-after-death and napi env teardown remain tracked in #15964 / #30286. ### Verification - `bun bd test test/js/web/workers/worker-terminate-lifetime.test.ts`: 6/6 pass under the ASAN debug build, including #32071's fetch-in-flight ASAN test and two new tests: - "cross-thread completions are delivered to live worker VMs" exercises fetch, Bun.spawn exit, node:fs, zlib, and Bun.password inside a worker; a broken generation capture silently drops the completion, so each would hang. - "terminating a worker with a subprocess in flight drops the waiter-thread completion" covers the waiter-thread path (EventLoopHandle generation dispatch) against a freed worker loop. - Debug-build suites for the converted producers pass: spawn, password, zlib (2 failures are container-speed artifacts: the tests' data generation alone takes 40s under this ASAN build vs their 15s cap), Bun.build API, fs.watch (2 failures are run-as-root artifacts, identical on release main), worker.test.ts (2 failures are a pre-existing 1s hardcoded budget; timed identically on the base branch build). - `cargo check -p bun_bin` on linux-x64, windows-x64 (covers the copy_file/write_file changes), darwin-arm64; clippy clean on all touched crates. The ABA mis-delivery itself is not deterministically reproducible from JS (it requires the allocator to reuse a dead VM's exact address for a new VM while a stale producer is in flight), so there is no fail-before test for it; the new tests pin the delivery and drop behavior of the handle plumbing on both sides.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
There shouldn't be ANY pointers to the VM if there is als oa VM handle. The VM handle must be the only source of truth.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Every C++ site that stored a bare bunVM pointer for later concurrent use now captures the VM's registry generation next to it (JSVMClientData, Zig::GlobalObject, EventLoopTaskNoContext, NapiEnv) and passes both back across the ABI, where VmHandle::from_raw_parts reassembles the handle. The address-only fallback entry points are deleted. The generation is stamped early in VirtualMachine::init (before Zig__GlobalObject__create and init_runtime_state) instead of at registration, so creation-time captures see the real value; registration stays the final init step. This also fixes the resolver WakeHandler capturing generation 0, which made the checked enqueue drop every auto-install wake. NapiFinalizerTask::schedule no longer dereferences the env's global on non-JS threads; it uses the env's creation-time handle capture.
|
Done in c417173. The remaining raw-pointer surface was the C++ side plus the address-only fallback APIs that existed for it; both are gone:
To make creation-time capture possible, the generation is stamped early in Verified: the worker-terminate regression suite (6 tests, fails on the unfixed build), worker/message-channel/atomics/napi-30205 suites, a live auto-install through |
|
Issue #33911 is another report of this crash class, with a fast reproduction: workers firing un-awaited failing fetches while the parent recycles them via terminate(). It segfaults release builds in under a second and trips the same ASAN signature this PR's description quotes (heap-use-after-free read of VirtualMachine::is_shutting_down from FetchTasklet::callback on the HTTP Client thread, freed by WebWorker::shutdown). I independently root-caused it to the same defect and have a narrow fetch-only alternative on https://github.com/oven-sh/bun/branches/all?query=farm%2F3bc34bb6 (branch One piece that may be worth lifting regardless of which fix lands: the branch adds a deterministic regression test in test/js/web/workers/worker-terminate-lifetime.test.ts that holds the worker's fetch connections open on a local Bun.listen server, terminates the worker, then RSTs the sockets, which reproduces the UAF 5/5 on an unfixed ASAN debug build without needing a stress loop. |
…terminate-concurrent-queue-uaf
…terminate-concurrent-queue-uaf
…gets, and generation stamping Bun__setDefaultGlobalObject publishes MAIN_THREAD_VM during Zig__GlobalObject__create (after the lock-free readers' fields are initialized), so the end-of-init store is a re-store, not the first publish. vm.event_loop only ever points at the two embedded sibling loops (spawnSync swaps event_loop_handle, not this field). Generation stamping moved from register_vm to stamp_generation in c417173; update the field docs that still cited register_vm.
The event_loop() accessor body still mentioned the boxed spawnSync loop after d1c2a20 corrected the field doc; EventLoopHandle::init and from_tag_ptr's Safety doc still claimed the constructor does not dereference the pointer, but both read the loop's registration generation at construction.
… arm Every other arm dereferences this on the JS thread; the checked enqueue_task_concurrent arm runs on producer threads and never dereferences this, which the block header previously contradicted.
…terminate-concurrent-queue-uaf # Conflicts: # src/jsc/bindings/webcore/MessagePort.cpp # src/runtime/webcore/fetch/FetchTasklet.rs
…terminate-concurrent-queue-uaf # Conflicts: # src/runtime/napi/napi_body.rs
There was a problem hiding this comment.
No new issues in this pass; all prior inline findings from earlier rounds (the WakeHandler generation-0 capture, the stale VM-lifetime doc sweeps across VirtualMachine.rs / event_loop.rs / AnyEventLoop.rs / node_fs / S3, the DEAD_VM_TASKLETS placement, the event_loop_handle non-atomic read tracked in #33313, and the TranspilerJob inline-slot structural limitation tracked in #33936) have been addressed or filed. This is a 63-file architectural change to cross-thread VM lifetime management — the live_vm_registry design, the VmHandle/LoopHandle producer-carried generation model, the C++ ABI additions, and the deliberate-leak policy on rejected enqueues all warrant maintainer sign-off.
Checked the registry lock as a leaf (critical sections only push to the MPSC queue + wakeup()), the stamp_generation → Zig__GlobalObject__create → register_vm ordering against every C++ (bunVM, generation) capture site, and the ~30 producer conversions for consistent schedule-time handle capture.
Confirmed the rejected-enqueue paths that intentionally leak (zlib, any_task_job, node_fs) do so because their teardown is JS-thread-only (CellRefCounted, JSC Strong drops) — matching the documented discard_unqueued_concurrent_task policy.
Verified WebWorker::shutdown() unregisters before any free and before set_shutting_down(), so Some(true) from live_shutting_down_state is main-VM-only as FetchTasklet::deref_from_thread relies on.
Extended reasoning...
Overview
This PR introduces a process-global live_vm_registry (a lock-guarded Vec<(vm_addr, loop_addr, generation)>) plus VmHandle / LoopHandle schedule-time identity tokens, and converts every cross-thread producer in the runtime — HTTP client thread (fetch, S3), work pool (node:fs, crypto, zlib, napi async work, Archive, Bun.build), watcher threads (fs.watch, fs.watchFile, DevServer), the POSIX process waiter thread, napi threadsafe functions, and JSC's own deferred-work scheduler — from raw *mut VirtualMachine / *mut EventLoop captures to registry-checked try_enqueue_task_concurrent / try_ref_concurrently entry points. C++ captures (JSVMClientData, Zig::GlobalObject, EventLoopTaskNoContext, NapiEnv) gain a bunVMGeneration field passed back across the ABI. VirtualMachine.event_loop and is_shutting_down become atomic. 63 files, ~1600 lines added.
Security risks
None in the traditional sense — no user-input parsing, auth, or crypto. This is memory-safety infrastructure: the fix itself is the security boundary. The risk profile is (a) a producer conversion that captures the wrong generation and silently drops legitimate completions (I caught one such bug — the WakeHandler generation-0 capture — in an earlier round; fixed), (b) a lock-ordering deadlock with the new registry lock (verified as a leaf), or (c) the main-VM lock-free fast path racing init (verified: stamp_generation and event_loop store happen before the first MAIN_THREAD_VM publish via Bun__setDefaultGlobalObject).
Level of scrutiny
High. This touches the core of Bun's concurrency model: VM lifetime, worker termination, and the event-loop task queue that every async operation flows through. It introduces a new cross-cutting abstraction (the registry + generation-carrying handles) that every future cross-thread producer must use correctly, and encodes a policy decision (rejected enqueues leak their payload rather than attempting off-thread teardown) that maintainers should explicitly ratify. The PR has been through 10 gate iterations, extensive CodeRabbit review, and multiple rounds of my own inline review — but the sheer breadth (30+ producer sites, C++ ABI changes, Windows-specific paths in blob copy/write) and the fact that it changes what "holding a VM pointer" means throughout the codebase puts it well outside bot-approval territory.
Other factors
- All 13 of my prior inline threads are resolved (fixes landed or findings filed as #33313 / #33936).
- The ASAN regression test demonstrably fails on unfixed builds and passes with the fix; a positive-delivery test guards against generation-capture regressions.
- The author's CI assessment on build #71966 attributes the two remaining red lanes to unrelated main-wide breakage (#33966/#33418) and macOS agent infra, with evidence.
- Two known pre-existing limitations this PR narrows but does not close (#33313
event_loop_handleatomic, #33936TranspilerJobinline-slot storage) are tracked separately with the author's agreement. - The deliberate-leak policy on rejected enqueues is well-reasoned (JSC handle teardown is JS-thread-only) and documented on
discard_unqueued_concurrent_task, but it is a design choice a maintainer should see. - No human maintainer has reviewed yet; the PR description explicitly requests it ("ready for maintainer review").
|
The specific |
2 participants
Crash
Sentry BUN-2VPE:
Panic: invalid enum valueinEventLoop.tickQueueWithCountat theswitch (task.tag())that drains the task queue. 546 events, ~37/day on 1.3.14, Windows x86_64 dominant, all compiled executables. Always on a Worker thread, and every event carriesworkers_spawned+workers_terminated.Repro
Terminate a worker while a fetch is in flight, and have the server respond only after the worker's VM has been freed:
On an unfixed ASAN debug build this aborts deterministically:
Cause
A worker's
VirtualMachine(with itsEventLoopand the MPSC concurrent task queue embedded in it) is freed byWebWorker::shutdown()on terminate. Cross-thread producers capture a raw*mut VirtualMachine/*mut EventLoopwhen work is scheduled and push completions later: the HTTP client thread (fetch, S3), the work pool (node:fs, crypto, zlib, napi async work), watcher threads, napi addon threads (threadsafe functions), the POSIX process waiter thread. Nothing told them the worker died;enqueue_task_concurrenthad only a debug assert. A late completion reads the freed VM and pushes into its freed queue. When that memory has been reused (commonly by the next worker, since worker VM allocations are same-sized), the stale write corrupts the new owner; a task slot read back with garbage in the tag bits is exactly "invalid enum value" intickQueueWithCount, on a live worker that did nothing wrong. That also explains the tag pairing: you need one worker terminated (stale producer) and another running (victim).The parent->worker postMessage path does not have this bug: C++
ScriptExecutionContext::postTaskToholdsallScriptExecutionContextsMapLockacross lookup+enqueue and the worker removes itself from that map before the VM is freed. This PR gives the Rust-side producers the same fence.Fix
VmHandle/LoopHandle: the schedule-time identity cross-thread producers hold instead of*mut VirtualMachine/*mut EventLoop. A handle is plain data,(address, generation); the generation is minted per VM (stamped early ininit, never reused) and holding a handle neither keeps the VM alive nor permits dereferencing it. The handle is the only cross-thread VM identity; there are no raw-pointer entry points.live_vm_registry(VirtualMachine.rs): a process-global lock + list of live(vm, loop, generation)registrations (addresses stored asusize, never dereferenced). Every VM registers as the final step ofVirtualMachine::init(); workers unregister at the top ofshutdown(), before anything is freed; the boxed spawnSync event loop registers around its lifetime.VirtualMachine::{with_live_vm, try_enqueue_task_concurrent, is_shutting_down_or_freed, try_ref_concurrently, try_unref_concurrently}andEventLoop::try_enqueue_task_concurrent, all keyed by handle. The(address, generation)pair must match a live registration, which closes both the freed-memory race and address reuse (a new VM at a dead VM's address has a different generation). For non-main VMs the push happens while holding the registry lock, so teardown (which takes the same lock to unregister) cannot free the VM mid-enqueue. The immortal main-thread VM takes a lock-free fast path.auto_delete, the payload is leaked. That is the same fate as a task that made it into the queue moments earlier, since a terminated worker's queue is never drained.JSVMClientData,Zig::GlobalObject,EventLoopTaskNoContext, andNapiEnvstore the generation next to theirbunVMpointer (viaBun__getVmGeneration, captured at creation on the JS thread) and pass both back across the ABI, whereVmHandle::from_raw_partsreassembles the handle. Producers that previously derived the VM from theJSGlobalObjectat completion time (node:fs tasks, node:zlib streams,AnyTaskJob, napi finalizers) capture the handle at schedule time instead.EventLoopHandle(shell tasks, the waiter thread, fs cp subtasks) are covered centrally by the vtable arm in event_loop.rs, which carries the loop generation.Known not-converted:
Bun__queueTaskConcurrently(fenced by the C++ map lock as described above) and the dev-only hot reloader (main VM only).Verification
test/js/web/workers/worker-terminate-lifetime.test.ts: "terminating a worker with a fetch in flight does not touch the freed VM" (ASAN-gated withskipIf(!isASAN)) fails on an unfixed ASAN debug build (AddressSanitizer abort) and passes with the fix; the file also covers positive delivery to live workers and the subprocess waiter-thread race (6 tests total).import "left-pad"through the registry download andPackageManager::wake()); this path hangs if the WakeHandler's generation capture is wrong.bun run rust:check-all: 10/10 target combos OK (the diff touches Windows-gated code in blob copy_file/write_file).Relationship to other PRs
Supersedes #31692 (robobun): that PR gated only
FetchTaskletwith a per-VM refcountedConcurrentEnqueueGateand explicitly left S3 and the other cross-thread producers ungated. This PR covers the whole producer class with one registry; the crash-side coverage for fetch is equivalent (same ASAN repro and test file). One piece of #31692 is intentionally not carried over: draining the worker's own concurrent queue at shutdown to release already-queued task refs. That is a leak fix, not a crash fix (a queued task keeps its payload alive; it just never runs), and the pre-existing behavior for a terminated worker's queue is unchanged here. It can land separately if wanted. The address-reuse window of an address-only registry is closed in this PR by the producer-carried generation tokens (#32073, implemented via #32082 which was merged into this branch).#29331 (Zig-era, pre-port) fixed the fetch slice of the same bug by validating a
VirtualMachine.Handleagainst theScriptExecutionContextmap before enqueueing - the same fence idea this PR applies registry-wide on the Rust side.Related issues (not auto-closed)
The issue-matcher suggested several candidates. Assessment against the actual mechanism (cross-thread enqueue/read into a freed worker VM):
Worker&worker_threadsstability tracking issue #15964: TODO item 2 ("make all usages of the event loop use a weak pointer or context id so we do not reference an event loop of a closed Worker") is what the registry implements; tracking issue, stays open.Merge notes
Merging main at 73b6c14 brought in #34067, which overlaps this PR inside
ThreadSafeFunction: it makesevent_loopanOption<BackRef<EventLoop>>thatenv_teardownclears, while this PR routes the addon-thread dispatch through the generation-checkedLoopHandle. The resolution keeps both: the struct carries theOptionfield (cleared at env teardown, used by the JS-thread-only accessors #34067 added) plus the schedule-timeloop_handle, andschedule_dispatchfirst bails if the env is torn down, then enqueues throughEventLoop::try_enqueue_task_concurrentinstead of dereferencing the loop pointer from an addon thread. Verified with the worker-lifetime test file (6/6) and the napi threadsafe-function suite (the one timeout,frees an orphaned threadsafe function whose last reference a call consumed, is the 5s test timeout vs a debug+ASAN build: the fixture itself completes with the exact expected output in ~10s).[decide:webkit] gate passed · iteration 12 · 63 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 1 rejected · iteration 12
evidence per changed file
root cause · written by the author bot
The crash was a use-after-free race on worker termination: producers on other threads could call enqueueTaskConcurrent, ref, or unref against a worker's VirtualMachine and event loop after the worker had already torn them down, leaving dangling task pointers in the queue that later surfaced as invalid tag panics when the loop drained them. The fix introduces a process-global registry of live VMs, with workers unregistering early during shutdown, and routes all cross-thread enqueue, ref, and unref operations through fallible try variants that first confirm the target VM is still registered u…