diff --git a/src/event_loop/lib.rs b/src/event_loop/lib.rs index 35194cd85e6a..cd48a014ae4e 100644 --- a/src/event_loop/lib.rs +++ b/src/event_loop/lib.rs @@ -60,7 +60,6 @@ bun_dispatch::link_interface! { fn exit(); fn enqueue_task(task: Task); fn enqueue_task_concurrent(task: core::ptr::NonNull); - fn concurrent_poster_end(); fn env() -> *mut bun_dotenv::Loader; fn top_level_dir() -> *const [u8]; fn create_null_delimited_env_map() -> Result; diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index aac8df7e6432..fd45bb62f524 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -790,6 +790,13 @@ impl VirtualMachine { unsafe { &*self.event_loop } } + /// Refuse all future work-pool completion posts (see `ConcurrentPosterGate`). Both loops: + /// macro mode can have pointed tasks at either. + pub fn close_concurrent_posters(&mut self) { + self.regular_event_loop.close_concurrent_posters(); + self.macro_event_loop.close_concurrent_posters(); + } + /// Alias for [`Self::event_loop_mut`]. Kept for callers migrated on the /// `runtime-hostfn-safe` branch; both names funnel into the single audited /// `unsafe` deref above. @@ -1638,10 +1645,9 @@ impl VirtualMachine { bun_http::shutdown_for_exit(); // Release tasks the HTTP daemon posted before observing `is_shutting_down` (else the - // tasklet ⇄ `Box` cycle leaks); must precede `destructOnExit`. Wait for - // work-pool fs completions first — they post without a shutdown check, so a post - // landing after the drain leaks. - self.event_loop_mut().wait_for_concurrent_posters(); + // tasklet ⇄ `Box` cycle leaks); must precede `destructOnExit`. Close the + // poster gates first so no work-pool fs completion can post after the drain. + self.close_concurrent_posters(); self.event_loop_mut().release_queued_tasks_for_shutdown(); if let Some(rare) = self.rare_data.as_deref_mut() { diff --git a/src/jsc/event_loop.rs b/src/jsc/event_loop.rs index 14ecee8fafa0..3cab585b44b4 100644 --- a/src/jsc/event_loop.rs +++ b/src/jsc/event_loop.rs @@ -88,10 +88,10 @@ pub struct EventLoop { pub entered_event_loop_count: isize, pub concurrent_ref: AtomicI32, - /// Work-pool tasks that will post to `concurrent_tasks` when they finish (counted on the JS - /// thread before hand-off, decremented on the pool thread after the post). Shutdown waits for - /// zero before the final queue drain so a completion can't land after it and leak. - pub concurrent_posters: AtomicU32, + /// Shared with every scheduled work-pool task whose completion posts back through + /// [`Self::enqueue_task_concurrent`]; see [`ConcurrentPosterGate`]. `None` only after + /// [`Self::deinit`]. + pub concurrent_poster_gate: Option>, /// Atomic nullable pointer to the next-due `WTFTimer`. /// /// Note (§Dispatch): payload is `*mut ()` — the real @@ -132,7 +132,7 @@ impl Default for EventLoop { uws_loop: (), entered_event_loop_count: 0, concurrent_ref: AtomicI32::new(0), - concurrent_posters: AtomicU32::new(0), + concurrent_poster_gate: Some(std::sync::Arc::new(ConcurrentPosterGate::default())), imminent_gc_timer: AtomicPtr::new(core::ptr::null_mut()), #[cfg(unix)] signal_handler: None, @@ -142,6 +142,59 @@ impl Default for EventLoop { } } +/// Gate between work-pool completion posts and event-loop teardown. Posters bracket only the +/// enqueue itself in [`Self::begin_post`]/[`Self::end_post`] — never the fs syscall, which can +/// block forever (a `read()` from a FIFO with no writer) — so shutdown's [`Self::close`] waits +/// only for posts already mid-enqueue and refuses everything later. `Arc`-shared with every +/// in-flight task so a poster whose syscall outlives the VM still reads the closed state from +/// live memory; a refused poster frees its own task without touching the VM or the JS heap +/// (`dispose_without_post` in `node_fs.rs`). +#[derive(Default)] +pub struct ConcurrentPosterGate { + /// Bit 31: closed. Bits 0..31: posters currently between `begin_post` and `end_post`. + state: AtomicU32, +} + +impl ConcurrentPosterGate { + const CLOSED: u32 = 1 << 31; + + /// Work-pool thread: begin a completion post. On `true`, the loop is guaranteed live until + /// the matching [`Self::end_post`] ([`Self::close`] cannot return in between). + #[must_use] + pub fn begin_post(&self) -> bool { + let mut state = self.state.load(Ordering::Relaxed); + loop { + if state & Self::CLOSED != 0 { + return false; + } + match self.state.compare_exchange_weak( + state, + state + 1, + Ordering::Acquire, + Ordering::Relaxed, + ) { + Ok(_) => return true, + Err(actual) => state = actual, + } + } + } + + /// Work-pool thread: the post finished. Last touch of the event loop. + pub fn end_post(&self) { + let prev = self.state.fetch_sub(1, Ordering::Release); + debug_assert!(prev & !Self::CLOSED > 0); + } + + /// JS thread, shutdown: after this returns no further post can land. Bounded by an enqueue, + /// not by the underlying fs operation. + pub fn close(&self) { + self.state.fetch_or(Self::CLOSED, Ordering::AcqRel); + while self.state.load(Ordering::Acquire) & !Self::CLOSED != 0 { + std::thread::yield_now(); + } + } +} + mod drain_result { pub(super) const SUCCESS: u8 = 0; pub(super) const JS_TERMINATED: u8 = 1; @@ -770,6 +823,9 @@ impl EventLoop { } pub fn deinit(&mut self) { + // The VM box is raw-dealloc'd without field `Drop`s, so release the gate `Arc` here; + // stranded posters hold their own clones. + drop(self.concurrent_poster_gate.take()); // Free (don't run — running could re-enter the dying VM) queued // ManagedTask boxes. Other tags are left in place: they were re-queued // by `release_queued_tasks_for_shutdown` because their callback can't @@ -1010,26 +1066,21 @@ impl EventLoop { self.wakeup(); } - /// See `concurrent_posters`. Call on the JS thread before scheduling a - /// work-pool task whose completion posts via `enqueue_task_concurrent`. - pub fn concurrent_poster_begin(&self) { - let _ = self.concurrent_posters.fetch_add(1, Ordering::SeqCst); - } - - /// Pool-thread pair of [`Self::concurrent_poster_begin`]. Must be the - /// poster's last touch of this event loop: once the count hits zero the - /// shutdown thread may drain the queue and tear the VM down. - pub fn concurrent_poster_end(&self) { - let prev = self.concurrent_posters.fetch_sub(1, Ordering::Release); - debug_assert!(prev > 0); + /// Clone of [`Self::concurrent_poster_gate`]. Panics after [`Self::deinit`] — tasks are + /// only created while the VM is live. + pub fn poster_gate(&self) -> std::sync::Arc { + std::sync::Arc::clone( + self.concurrent_poster_gate + .as_ref() + .expect("poster_gate() after EventLoop::deinit()"), + ) } - /// Spin until every counted poster has finished its post. Called on the JS thread during - /// shutdown, after the last JS has run and before the final queue drain. Each pending fs - /// operation is finite, so the wait is bounded by syscall latency. - pub fn wait_for_concurrent_posters(&self) { - while self.concurrent_posters.load(Ordering::Acquire) > 0 { - std::thread::yield_now(); + /// Close [`Self::concurrent_poster_gate`]. Shutdown calls this before the final queue drain + /// so the drain sees every post that will ever land. + pub fn close_concurrent_posters(&self) { + if let Some(gate) = self.concurrent_poster_gate.as_ref() { + gate.close(); } } @@ -1343,7 +1394,6 @@ bun_event_loop::link_impl_JsEventLoop! { exit() => (*this).exit(), enqueue_task(task) => (*this).enqueue_task(task), enqueue_task_concurrent(task) => (*this).enqueue_task_concurrent(task), - concurrent_poster_end() => (*this).concurrent_poster_end(), env() => (*this).vm_ref().transpiler.env, top_level_dir() => core::ptr::from_ref::<[u8]>((*this).vm_ref().top_level_dir()), create_null_delimited_env_map() => diff --git a/src/jsc/lib.rs b/src/jsc/lib.rs index 36b31ec4878a..f21bc8dcf160 100644 --- a/src/jsc/lib.rs +++ b/src/jsc/lib.rs @@ -1359,10 +1359,11 @@ pub use self::event_loop as EventLoop; pub mod any_task_job; pub use self::any_task_job::{AnyTaskJob, AnyTaskJobCtx}; pub use self::event_loop::{ - AnyEventLoop, AnyTaskWithExtraContext, ConcurrentCppTask, ConcurrentPromiseTask, - ConcurrentTask, CppTask, DeferredTaskQueue, EventLoopHandle, EventLoopTask, EventLoopTaskPtr, - GarbageCollectionController, JsTerminated, JsTerminatedResult, ManagedTask, MiniEventLoop, - PosixSignalHandle, PosixSignalTask, Task, WorkPool, WorkPoolTask, WorkTask, WorkTaskContext, + AnyEventLoop, AnyTaskWithExtraContext, ConcurrentCppTask, ConcurrentPosterGate, + ConcurrentPromiseTask, ConcurrentTask, CppTask, DeferredTaskQueue, EventLoopHandle, + EventLoopTask, EventLoopTaskPtr, GarbageCollectionController, JsTerminated, JsTerminatedResult, + ManagedTask, MiniEventLoop, PosixSignalHandle, PosixSignalTask, Task, WorkPool, WorkPoolTask, + WorkTask, WorkTaskContext, }; #[cfg(unix)] pub type PlatformEventLoop = bun_uws::Loop; diff --git a/src/jsc/node_path.rs b/src/jsc/node_path.rs index 84e7d12b6d05..33b776bc4215 100644 --- a/src/jsc/node_path.rs +++ b/src/jsc/node_path.rs @@ -33,6 +33,12 @@ use crate::array_buffer::MarkedArrayBuffer; /// [`ThreadSafe`]. pub trait Unprotect { fn unprotect(&mut self); + + /// The VM died before the JS thread could run [`Self::unprotect`] or `Drop`: clear every + /// cleanup that would touch the JS heap or VM-owned state (buffer pins, AbortSignal refs) + /// so the plain `Drop` is safe off the JS thread. Protect counts died with the heap and + /// need no balancing. Required (no default) so every impl accounts for its fields. + fn disarm_for_dead_vm(&mut self); } /// RAII guard returned by `into_thread_safe()`: a `T` whose JS-backed buffers @@ -51,6 +57,18 @@ impl ThreadSafe { pub fn adopt(value: T) -> Self { Self(value) } + + /// Dispose on a work-pool thread after the VM died: skip [`Unprotect::unprotect`], clear + /// the JS-heap-touching `Drop` behavior via [`Unprotect::disarm_for_dead_vm`], then drop + /// the inner `T` to free its owned Rust payloads. + pub fn dispose_for_dead_vm(self) { + let mut this = core::mem::ManuallyDrop::new(self); + this.0.disarm_for_dead_vm(); + // SAFETY: `this` is `ManuallyDrop`, so `ThreadSafe::drop` (the + // unprotect) never runs and the inner value is read out exactly once; + // its disarmed `Drop` frees the owned payloads. + drop(unsafe { core::ptr::read(&raw const this.0) }); + } } impl core::ops::Deref for ThreadSafe { @@ -225,6 +243,13 @@ impl Unprotect for PathLike { b.buffer.value.unprotect(); } } + + fn disarm_for_dead_vm(&mut self) { + // `Drop` unpins a pinned Buffer path — a JS-cell deref the dead heap can't take. + if let Self::Buffer(b) = self { + b.pinned = false; + } + } } /// `node.PathOrFileDescriptor`. @@ -293,6 +318,12 @@ impl Unprotect for PathOrFileDescriptor { p.unprotect(); } } + + fn disarm_for_dead_vm(&mut self) { + if let Self::Path(p) = self { + p.disarm_for_dead_vm(); + } + } } impl core::fmt::Display for PathOrFileDescriptor { diff --git a/src/jsc/web_worker.rs b/src/jsc/web_worker.rs index 90d0b0daab0b..addd86dd6993 100644 --- a/src/jsc/web_worker.rs +++ b/src/jsc/web_worker.rs @@ -1308,10 +1308,9 @@ impl WebWorker { // teardownJSCVM sets it again. Bun__JSCTaskScheduler__markShuttingDown(vm.global()); // Reclaim queued CppTasks while JSC is still live (after teardownJSCVM the worker VM is - // dealloc'd-without-Drop so anything still in self.tasks leaks). Work-pool fs completions - // post without a shutdown check; wait for in-flight ones so the drain below sees every - // post. - vm.event_loop_mut().wait_for_concurrent_posters(); + // dealloc'd-without-Drop so anything still in self.tasks leaks). Close the poster gates + // first so the drain below sees every work-pool fs completion that will ever land. + vm.close_concurrent_posters(); vm.event_loop_mut().release_queued_tasks_for_shutdown(); if let Some(rare) = vm.rare_data.as_deref_mut() { rare.release_js_handles(); diff --git a/src/runtime/crypto/PBKDF2.rs b/src/runtime/crypto/PBKDF2.rs index fa2468df7bd8..05b9d303ac36 100644 --- a/src/runtime/crypto/PBKDF2.rs +++ b/src/runtime/crypto/PBKDF2.rs @@ -255,6 +255,11 @@ impl bun_jsc::Unprotect for PBKDF2 { self.password.unprotect(); self.salt.unprotect(); } + + fn disarm_for_dead_vm(&mut self) { + self.password.disarm_for_dead_vm(); + self.salt.disarm_for_dead_vm(); + } } pub(crate) struct Pbkdf2Ctx { diff --git a/src/runtime/node/node_crypto_binding.rs b/src/runtime/node/node_crypto_binding.rs index d298afc4247c..c566206802d4 100644 --- a/src/runtime/node/node_crypto_binding.rs +++ b/src/runtime/node/node_crypto_binding.rs @@ -1126,6 +1126,11 @@ mod _impl { bun_jsc::Unprotect::unprotect(&mut self.password); bun_jsc::Unprotect::unprotect(&mut self.salt); } + + fn disarm_for_dead_vm(&mut self) { + bun_jsc::Unprotect::disarm_for_dead_vm(&mut self.password); + bun_jsc::Unprotect::disarm_for_dead_vm(&mut self.salt); + } } impl CryptoJobCtx for Scrypt { diff --git a/src/runtime/node/node_fs.rs b/src/runtime/node/node_fs.rs index ced0457f206e..162a1b114367 100644 --- a/src/runtime/node/node_fs.rs +++ b/src/runtime/node/node_fs.rs @@ -6,6 +6,7 @@ use bun_paths::strings; use core::ffi::{c_char, c_int, c_uint, c_void}; use core::ptr::NonNull; use core::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; +use std::sync::Arc; use crate::api::bun::process::event_loop_handle_to_ctx; use crate::webcore; @@ -18,7 +19,10 @@ use bun_jsc::AbortSignal; use bun_jsc::EventLoopTaskPtr; use bun_jsc::debugger::AsyncTaskTracker; use bun_jsc::virtual_machine::VirtualMachine; -use bun_jsc::{EventLoopHandle, JSGlobalObject, JSValue, JsResult, Task, ThreadSafe, Unprotect}; +use bun_jsc::{ + ConcurrentPosterGate, EventLoopHandle, JSGlobalObject, JSValue, JsResult, Task, ThreadSafe, + Unprotect, +}; use bun_paths::{self as paths, OSPathBuffer, OSPathChar, OSPathSliceZ, PathBuffer}; use bun_sys::FdExt as _; use bun_sys::{self as sys, E, Fd as FD, Maybe, Mode, SystemErrno}; @@ -1012,6 +1016,11 @@ mod _async_tasks { // SAFETY: caller guarantees `this` is the live Box-leaked allocation; // reclaim ownership (paired with the Box::leak in create()). let mut task = unsafe { bun_core::heap::take(this) }; + // `run_from_js_thread` leaves the sentinel error here; only the shutdown-drain path + // still holds a real result, whose payloads only `fs_to_js` would otherwise free. + if let Ok(result) = task.result.as_mut() { + result.fs_discard(); + } // `bun_sys::Error` frees its path on Drop. task.r#ref.unref(bun_io::js_vm_ctx()); } @@ -1067,6 +1076,7 @@ mod _async_tasks { } impl Unprotect for $ty { #[inline] fn unprotect(&mut self) {} + #[inline] fn disarm_for_dead_vm(&mut self) {} } )+ }; } @@ -1155,6 +1165,10 @@ mod _async_tasks { /// Each `ret::*` type implements this by forwarding to its inherent method. pub trait FsReturn { fn fs_to_js(&mut self, global: &JSGlobalObject) -> JsResult; + + /// Release payloads only [`Self::fs_to_js`] would free, for a result that will never + /// reach the JS thread. Types whose `Drop` already frees everything keep the no-op. + fn fs_discard(&mut self) {} } impl FsReturn for JSValue { #[inline] @@ -1191,18 +1205,38 @@ mod _async_tasks { fn fs_to_js(&mut self, global: &JSGlobalObject) -> JsResult { Ok(crate::node::types::FdJsc::to_js(*self, global)) } + fn fs_discard(&mut self) { + // `fs_to_js` would have transferred the descriptor to JS; nothing else closes it. + let fd = core::mem::replace(self, FD::INVALID); + if fd != FD::INVALID { + fd.close(); + } + } } impl FsReturn for StringOrBuffer { #[inline] fn fs_to_js(&mut self, global: &JSGlobalObject) -> JsResult { self.to_js(global) } + fn fs_discard(&mut self) { + // `Drop for StringOrBuffer` deliberately skips `Buffer` (bytes transfer in `to_js`). + if let StringOrBuffer::Buffer(buffer) = self { + buffer.destroy(); + } + } } impl FsReturn for StringOrUndefined { #[inline] fn fs_to_js(&mut self, global: &JSGlobalObject) -> JsResult { self.to_js(global) } + fn fs_discard(&mut self) { + // `transfer_to_js` would have consumed the ref; `BunString` has no `Drop`. + if let StringOrUndefined::String(s) = core::mem::replace(self, StringOrUndefined::None) + { + s.deref(); + } + } } impl FsReturn for ret::Read { #[inline] @@ -1230,6 +1264,27 @@ mod _async_tasks { let owned = core::mem::replace(self, ret::Readdir::Files(Box::default())); owned.to_js(global) } + fn fs_discard(&mut self) { + // Mirrors `ResultListEntryValue::deinit`; no `Drop` releases the entries. + match self { + ret::Readdir::WithFileTypes(items) => { + for item in items.iter() { + item.deref(); + } + } + ret::Readdir::Buffers(items) => { + for item in items.iter_mut() { + item.destroy(); + } + } + ret::Readdir::Files(items) => { + for item in items.iter() { + item.deref(); + } + } + } + *self = ret::Readdir::Files(Box::default()); + } } impl FsReturn for StatOrNotFound { #[inline] @@ -1263,6 +1318,9 @@ mod _async_tasks { pub(crate) result: Maybe, pub(crate) r#ref: KeepAlive, pub(crate) tracker: AsyncTaskTracker, + /// Keeps the loop's [`ConcurrentPosterGate`] readable after the VM is torn down (the + /// blocking syscall can outlive it). + pub(crate) gate: Arc, } bun_threading::intrusive_work_task!([R, A: Unprotect, const F: NodeFSFunctionEnum] AsyncFSTask, task); @@ -1305,16 +1363,14 @@ mod _async_tasks { task: work_pool_task(Self::work_pool_callback), r#ref: KeepAlive::default(), tracker: AsyncTaskTracker::init(vm), + // SAFETY: `event_loop()` is a value field of the live `vm`. + gate: unsafe { (*vm.event_loop()).poster_gate() }, }); // KeepAlive::ref_ now takes the type-erased aio EventLoopCtx; the JS // event loop is the only one that owns AsyncFSTask/UVFSRequest. task.r#ref.ref_(bun_io::js_vm_ctx()); task.tracker.did_schedule(global_object); let promise = task.promise.value(); - // Counted so shutdown's `wait_for_concurrent_posters` covers the - // work-pool completion post; paired in `work_pool_callback`. - // SAFETY: `event_loop()` is a value field of the live `vm`. - unsafe { (*vm.event_loop()).concurrent_poster_begin() }; WorkPool::schedule(&raw mut bun_core::heap::release(task).task); promise } @@ -1334,20 +1390,46 @@ mod _async_tasks { // `sys::Error::path` is `Box<[u8]>` boxed at the // `errno_sys_p` construction site, so no clone is needed — `node_fs` may drop. - // `bun_vm_concurrently()` skips the JS-thread debug assert and is the - // documented accessor for off-thread (work-pool) callers; the - // event-loop's concurrent queue is MPSC-safe. + // Clone, not borrow: the JS thread may free `this` (and its `Arc`) right after the + // enqueue, before `end_post`. // SAFETY: `this` is still exclusively owned here (see above). - let vm = unsafe { (*this).global_object().bun_vm_concurrently() }; - // SAFETY: VirtualMachine and its event loop are process-static - // (LIFETIMES.tsv); the concurrent queue is MPSC-safe. Ownership of - // `this` transfers to the JS thread here — no use after this call. - unsafe { - (*(*vm).event_loop()).enqueue_task_concurrent(ConcurrentTask::create_from(this)); - // Pairs with `concurrent_poster_begin` in `create()`. The JS thread may free `this` - // once popped and tear the VM down at zero — this is the pool thread's last touch. - (*(*vm).event_loop()).concurrent_poster_end(); + let gate = Arc::clone(unsafe { &(*this).gate }); + if gate.begin_post() { + // `bun_vm_concurrently()` skips the JS-thread debug assert and is the + // documented accessor for off-thread (work-pool) callers; the + // event-loop's concurrent queue is MPSC-safe. + // SAFETY: `begin_post()` returned true, so the VM, its event loop, and the + // global object all stay live until the matching `end_post()`. Ownership of + // `this` transfers to the JS thread at the enqueue — no use after it. + unsafe { + let vm = (*this).global_object().bun_vm_concurrently(); + (*(*vm).event_loop()) + .enqueue_task_concurrent(ConcurrentTask::create_from(this)); + } + gate.end_post(); + } else { + // The VM shut down while the syscall was blocked; the completion has nowhere + // to go. SAFETY: the post was refused, so this thread still owns `this`. + unsafe { Self::dispose_without_post(this) }; + } + } + + /// The loop's gate refused the completion post: the VM is torn down (or mid-teardown). + /// Free the task on the work-pool thread without touching the JS heap — the promise + /// `Strong` and the args' protect counts died with it. + /// + /// SAFETY: `this` must be the pointer Box::leak'd in `create()`, exclusively owned by + /// the caller (the post never happened); called at most once. + unsafe fn dispose_without_post(this: *mut Self) { + // SAFETY: caller guarantees `this` is the live Box-leaked allocation. + let mut task = unsafe { bun_core::heap::take(this) }; + if let Ok(result) = task.result.as_mut() { + result.fs_discard(); } + let Self { promise, args, .. } = *task; + // Intentionally never dropped: the Strong handle died with the VM heap. + let _ = core::mem::ManuallyDrop::new(promise); + args.dispose_for_dead_vm(); } pub(crate) fn run_from_js_thread(&mut self) -> Result<(), bun_jsc::JsTerminated> { @@ -1402,6 +1484,10 @@ mod _async_tasks { // SAFETY: caller guarantees `this` is the live Box-leaked allocation; // reclaim ownership (paired with the Box::leak in create()). let mut task = unsafe { bun_core::heap::take(this) }; + // Same as `UVFSRequest::destroy`: only the shutdown-drain path holds a real result. + if let Ok(result) = task.result.as_mut() { + result.fs_discard(); + } // `bun_sys::Error` frees its path on Drop. task.r#ref.unref(bun_io::js_vm_ctx()); } @@ -1452,6 +1538,8 @@ mod _async_tasks { /// outlives this task; `ParentRef` gives a safe `&ShellCpTask` projection /// for `cp_on_copy` and round-trips the `*mut` for `cp_on_finish`. pub(crate) shelltask: Option>, + /// `Some` iff `evtloop` is the JS loop (see [`AsyncFSTask::gate`]). + pub(crate) gate: Option>, } bun_threading::intrusive_work_task!([const IS_SHELL: bool] NewAsyncCpTask, task); @@ -1622,16 +1710,14 @@ mod _async_tasks { // SAFETY: `shelltask` (when non-null) is the live heap-alloc'd `ShellCpTask` // that owns and outlives this task; pointer carries write provenance. shelltask: unsafe { bun_ptr::ParentRef::from_nullable_mut(shelltask) }, + // SAFETY: `event_loop()` is a value field of the live `vm`. + gate: Some(unsafe { (*vm.event_loop()).poster_gate() }), }); if !IS_SHELL { task.r#ref.ref_(event_loop_handle_to_ctx(task.evtloop)); } task.tracker.did_schedule(global_object); - // Counted so shutdown's `wait_for_concurrent_posters` covers the completion post; - // paired in `on_subtask_done`'s Js arm (the mini path never touches the JS event loop). - // SAFETY: `event_loop()` is a value field of the live `vm`. - unsafe { (*vm.event_loop()).concurrent_poster_begin() }; let raw = bun_core::heap::release(task); WorkPool::schedule(&raw mut raw.task); raw @@ -1661,6 +1747,7 @@ mod _async_tasks { // SAFETY: `shelltask` (when non-null) is the live heap-alloc'd `ShellCpTask` // that owns and outlives this task; pointer carries write provenance. shelltask: unsafe { bun_ptr::ParentRef::from_nullable_mut(shelltask) }, + gate: None, }); if !IS_SHELL { task.r#ref.ref_(event_loop_handle_to_ctx(task.evtloop)); @@ -1730,18 +1817,32 @@ mod _async_tasks { // Count reached zero ⇒ exclusive access. `this` carries mutable // provenance from `Box::leak`, so the enqueued callback may safely // form `&mut *this` on the JS thread. - if let EventLoopHandle::Js { owner } = this_ref.evtloop { - this_ref.evtloop.enqueue_task_concurrent(EventLoopTaskPtr { - js: ConcurrentTask::from_callback(this, |p| { - // SAFETY: `p` is the `Box::leak`'d task; subtask count hit zero so this - // JS-thread callback holds the only live reference (exclusive `&mut`). - unsafe { (&mut *p).run_from_js_thread().map_err(Into::into) } - }) - .as_ptr(), - }); - // Pairs with `concurrent_poster_begin` in `create_with_shell_task`. The JS thread - // may free the task once popped and tear the VM down at zero — last touch of loop. - owner.concurrent_poster_end(); + if let EventLoopHandle::Js { .. } = this_ref.evtloop { + // Clone, not borrow: the JS thread may free the task (and its `Arc`) right + // after the enqueue, before `end_post`. + let gate = Arc::clone( + this_ref + .gate + .as_ref() + .expect("JS-loop cp task always carries the poster gate"), + ); + if gate.begin_post() { + // SAFETY (`evtloop` deref): `begin_post()` pins the VM and its event loop + // live until `end_post()`; ownership of `this` transfers at the enqueue. + this_ref.evtloop.enqueue_task_concurrent(EventLoopTaskPtr { + js: ConcurrentTask::from_callback(this, |p| { + // SAFETY: `p` is the `Box::leak`'d task; subtask count hit zero so this + // JS-thread callback holds the only live reference (exclusive `&mut`). + unsafe { (&mut *p).run_from_js_thread().map_err(Into::into) } + }) + .as_ptr(), + }); + gate.end_post(); + } else { + // The VM shut down while a copy was blocked; the completion has nowhere to + // go. SAFETY: the post was refused, so this thread still owns `this`. + unsafe { Self::dispose_without_post(this) }; + } } else { this_ref.evtloop.enqueue_task_concurrent(EventLoopTaskPtr { mini: AnyTaskWithExtraContext::from_callback_auto_deinit( @@ -1837,6 +1938,32 @@ mod _async_tasks { // `to_thread_safe()` when `src`/`dest` are Buffers, so nothing leaks here. } + /// The loop's gate refused the completion post: the VM is torn down (or mid-teardown). + /// Free the task on the work-pool thread without touching the JS heap — the promise + /// `Strong` and the args' protect counts died with it. + /// + /// SAFETY: `this` must be the Box::leak'd task, exclusively owned by the caller (the + /// subtask count hit zero and the post never happened); called at most once. + unsafe fn dispose_without_post(this: *mut Self) { + // SAFETY: caller guarantees `this` is the live Box-leaked allocation. + let task = unsafe { bun_core::heap::take(this) }; + let Self { + promise, + args, + shelltask, + .. + } = *task; + // Intentionally never dropped: the Strong handle died with the VM heap. + let _ = core::mem::ManuallyDrop::new(promise); + args.dispose_for_dead_vm(); + // `cp_on_finish` (JS thread) never runs for a refused post, so reclaim the shell + // parent here: its interpreter died with the VM, its fields are plain heap. + if let Some(parent) = shelltask { + // SAFETY: subtask count hit zero and the interpreter is gone — sole reference. + drop(unsafe { bun_core::heap::take(parent.as_mut_ptr()) }); + } + } + /// Directory scanning + clonefile will block this thread, then each individual file copy (what the sync version /// calls "copy_single_file_sync") will be dispatched as a separate task. pub(crate) fn cp_async(nodefs: &mut NodeFS, this: *mut Self) { @@ -2228,6 +2355,8 @@ mod _async_tasks { pub(crate) pending_err: Option, pub(crate) pending_err_mutex: bun_threading::Mutex, + /// See [`AsyncFSTask::gate`]. + pub(crate) gate: Arc, } bun_threading::intrusive_work_task!(AsyncReaddirRecursiveTask, task); @@ -2405,14 +2534,12 @@ mod _async_tasks { root_fd: FD::INVALID, pending_err: None, pending_err_mutex: bun_threading::Mutex::default(), + // SAFETY: `event_loop()` is a value field of the live `vm`. + gate: unsafe { (*vm.event_loop()).poster_gate() }, }); task.r#ref.ref_(bun_io::js_vm_ctx()); task.tracker.did_schedule(global_object); let promise = task.promise.value(); - // Counted so shutdown's `wait_for_concurrent_posters` covers the - // single CAS-gated completion post; paired in `finish_concurrently`. - // SAFETY: `event_loop()` is a value field of the live `vm`. - unsafe { (*vm.event_loop()).concurrent_poster_begin() }; WorkPool::schedule(&raw mut bun_core::heap::release(task).task); promise } @@ -2584,22 +2711,30 @@ mod _async_tasks { } } - // `bun_vm_concurrently()` skips the JS-thread debug assert and is the - // documented accessor for off-thread (work-pool) callers. - let vm = self.global_object().bun_vm_concurrently(); - // `ConcurrentTask::create` heap-allocates a fresh task; the - // queue takes ownership of it. - // SAFETY: `vm` is the process-singleton VM (LIFETIMES.tsv); the - // concurrent queue is MPSC-safe and the borrow is scoped to the call. - unsafe { - (*vm).enqueue_task_concurrent(ConcurrentTask::create(Task::init( - std::ptr::from_mut::(self), - ))); + // Clone, not borrow: the JS thread may free `self` (and its `Arc`) right after the + // enqueue, before `end_post`. + let gate = Arc::clone(&self.gate); + let this = std::ptr::from_mut::(self); + if gate.begin_post() { + // `bun_vm_concurrently()` skips the JS-thread debug assert and is the + // documented accessor for off-thread (work-pool) callers. + // `ConcurrentTask::create` heap-allocates a fresh task; the + // queue takes ownership of it. + // SAFETY: `begin_post()` returned true, so the VM, its event loop, and the + // global object all stay live until the matching `end_post()`; the concurrent + // queue is MPSC-safe. The JS thread may free `this` once popped — the enqueue + // is this thread's last touch of it. + unsafe { + let vm = (*this).global_object().bun_vm_concurrently(); + (*vm).enqueue_task_concurrent(ConcurrentTask::create(Task::init(this))); + } + gate.end_post(); + } else { + // The VM shut down while the scan was blocked; the completion has nowhere to + // go. SAFETY: the post was refused and the subtask count hit zero, so this + // thread exclusively owns `this`. + unsafe { Self::dispose_without_post(this) }; } - // Pairs with `concurrent_poster_begin` in `create()`. The JS thread may free `self` - // once popped and tear the VM down at zero — last touch of both. - // SAFETY: `event_loop()` is a value field of the process-static VM. - unsafe { (*(*vm).event_loop()).concurrent_poster_end() }; } fn clear_result_list(&mut self) { @@ -2700,6 +2835,26 @@ mod _async_tasks { task.free_root_path(); task.clear_result_list(); } + + /// The loop's gate refused the completion post: the VM is torn down (or mid-teardown). + /// Free the task on the work-pool thread without touching the JS heap — the promise + /// `Strong` and the args' protect counts died with it. Entry disposal + /// (`clear_result_list`) is already off-thread-safe: `finish_concurrently` runs it on + /// pool threads on the error path. + /// + /// SAFETY: `this` must be the pointer Box::leak'd in `create()`, exclusively owned by + /// the caller (the post never happened); called at most once. + unsafe fn dispose_without_post(this: *mut Self) { + // SAFETY: caller guarantees `this` is the live Box-leaked allocation. + let mut task = unsafe { bun_core::heap::take(this) }; + debug_assert!(task.root_fd == FD::INVALID); // closed by finish_concurrently + task.free_root_path(); + task.clear_result_list(); + let Self { promise, args, .. } = *task; + // Intentionally never dropped: the Strong handle died with the VM heap. + let _ = core::mem::ManuallyDrop::new(promise); + args.dispose_for_dead_vm(); + } } /// Maps a readdir element type to its `ResultListEntryValue` variant. @@ -2781,6 +2936,7 @@ pub mod args { ($ty:ident; $($field:ident),+ $(,)?) => { impl Unprotect for $ty { #[inline] fn unprotect(&mut self) { $( self.$field.unprotect(); )+ } + #[inline] fn disarm_for_dead_vm(&mut self) { $( self.$field.disarm_for_dead_vm(); )+ } } impl $ty { pub fn to_thread_safe(&mut self) { $( self.$field.to_thread_safe(); )+ } @@ -2858,6 +3014,9 @@ pub mod args { self.buffers.value.unprotect(); // `self.buffers.buffers`: `Vec` frees on drop. } + + // The element roots/pins are released only in `unprotect`; `Drop` touches no JS state. + fn disarm_for_dead_vm(&mut self) {} } impl FdVectorIo { pub(crate) fn to_thread_safe(&mut self) { @@ -2907,6 +3066,9 @@ pub mod args { impl Unprotect for FTruncate { #[inline] fn unprotect(&mut self) {} + + #[inline] + fn disarm_for_dead_vm(&mut self) {} } impl FTruncate { pub(crate) fn to_thread_safe(&self) {} @@ -3410,6 +3572,11 @@ pub mod args { fn unprotect(&mut self) { self.0.unprotect(); } + + #[inline] + fn disarm_for_dead_vm(&mut self) { + self.0.disarm_for_dead_vm(); + } } impl Rm { pub fn from_js(ctx: &JSGlobalObject, arguments: &mut ArgumentsSlice) -> JsResult { @@ -3806,6 +3973,11 @@ pub mod args { fn unprotect(&mut self) { self.buffer.unprotect(); } + + #[inline] + fn disarm_for_dead_vm(&mut self) { + self.buffer.disarm_for_dead_vm(); + } } impl Write { pub(crate) fn to_thread_safe(&mut self) { @@ -3975,6 +4147,10 @@ pub mod args { } self.buffer.buffer.value.unprotect(); } + + fn disarm_for_dead_vm(&mut self) { + self.pinned = false; + } } impl Read { pub fn from_js(ctx: &JSGlobalObject, arguments: &mut ArgumentsSlice) -> JsResult { @@ -4210,6 +4386,14 @@ pub mod args { self.path.unprotect(); // Signal unref handled by `Drop` (idempotent via `.take()`). } + + fn disarm_for_dead_vm(&mut self) { + self.path.disarm_for_dead_vm(); + // `Drop` would deref the signal's non-atomic WebCore refcount. + if let Some(signal) = self.signal.take() { + let _ = core::mem::ManuallyDrop::new(signal); + } + } } impl ReadFile { pub(crate) fn to_thread_safe(&mut self) { @@ -4301,6 +4485,15 @@ pub mod args { self.data.unprotect(); // Signal unref handled by `Drop` (idempotent via `.take()`). } + + fn disarm_for_dead_vm(&mut self) { + self.file.disarm_for_dead_vm(); + self.data.disarm_for_dead_vm(); + // `Drop` would deref the signal's non-atomic WebCore refcount. + if let Some(signal) = self.signal.take() { + let _ = core::mem::ManuallyDrop::new(signal); + } + } } impl WriteFile { pub fn from_js( @@ -4405,6 +4598,11 @@ pub mod args { fn unprotect(&mut self) { self.0.unprotect(); } + + #[inline] + fn disarm_for_dead_vm(&mut self) { + self.0.disarm_for_dead_vm(); + } } pub struct Exists { @@ -4424,6 +4622,12 @@ pub mod args { p.unprotect(); } } + + fn disarm_for_dead_vm(&mut self) { + if let Some(p) = &mut self.path { + p.disarm_for_dead_vm(); + } + } } impl Exists { pub fn from_js(ctx: &JSGlobalObject, arguments: &mut ArgumentsSlice) -> JsResult { diff --git a/src/runtime/node/types.rs b/src/runtime/node/types.rs index 995e5ca1df13..cb0b2fdbde4c 100644 --- a/src/runtime/node/types.rs +++ b/src/runtime/node/types.rs @@ -289,6 +289,13 @@ impl bun_jsc::Unprotect for BlobOrStringOrBuffer { sob.unprotect(); } } + + fn disarm_for_dead_vm(&mut self) { + // `Blob` never rides a work-pool fs task (no `FsArgument` impl), so no arm for it. + if let Self::StringOrBuffer(sob) = self { + sob.disarm_for_dead_vm(); + } + } } impl bun_jsc::Unprotect for StringOrBuffer { @@ -306,6 +313,12 @@ impl bun_jsc::Unprotect for StringOrBuffer { buffer.buffer.value.unprotect(); } } + + fn disarm_for_dead_vm(&mut self) { + if let Self::Buffer(buffer) = self { + buffer.pinned = false; + } + } } impl StringOrBuffer { diff --git a/test/js/node/worker_threads/worker_threads.test.ts b/test/js/node/worker_threads/worker_threads.test.ts index aa3bf5d714d8..3513ffa7620f 100644 --- a/test/js/node/worker_threads/worker_threads.test.ts +++ b/test/js/node/worker_threads/worker_threads.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, setDefaultTimeout, test } from "bun:test"; -import { bunEnv, bunExe, isDebug, tmpdirSync } from "harness"; +import { bunEnv, bunExe, isDebug, isWindows, tempDir, tmpdirSync } from "harness"; import { once } from "node:events"; import fs from "node:fs"; import { join, relative, resolve } from "node:path"; @@ -1774,3 +1774,122 @@ test("the SHARE_ENV founding thread's process.env stays live after the swap", as expect(stdout.trim()).toBe("yes,unset"); expect(exitCode).toBe(0); }); + +// A pending node:fs thread-pool operation that can never complete (here: a +// FIFO whose open-for-read blocks until a writer appears) must not block +// worker/VM teardown: the completion is instead refused at the event loop's +// poster gate and freed on the pool thread. The fs ops below block forever by +// design, so these tests hang (and time out) if shutdown ever waits on them. +test.concurrent.skipIf(isWindows)("terminate() settles while the worker has an fs read blocked on a FIFO", async () => { + using dir = tempDir("worker-terminate-blocked-read", { + "main.cjs": ` + const { Worker, isMainThread, parentPort, workerData } = require("worker_threads"); + if (isMainThread) { + const w = new Worker(__filename, { workerData: process.argv[2] }); + w.on("exit", () => console.log("exit event")); + w.on("message", () => { + w.terminate().then(() => { + console.log("terminate resolved"); + process.exit(0); + }); + }); + } else { + // Blocks a thread-pool worker forever: a FIFO with no writer. + require("fs").readFile(workerData, () => {}); + parentPort.postMessage("pending"); + } + `, + }); + const fifo = join(String(dir), "pipe.fifo"); + expect(Bun.spawnSync({ cmd: ["mkfifo", fifo] }).exitCode).toBe(0); + await using proc = Bun.spawn({ + cmd: [bunExe(), "main.cjs", fifo], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stdout).toBe("exit event\nterminate resolved\n"); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); +}); + +test.concurrent.skipIf(isWindows)( + "process.exit() with a blocked fs read pending completes under BUN_DESTRUCT_VM_ON_EXIT", + async () => { + using dir = tempDir("exit-blocked-read", { + "main.cjs": `require("fs").readFile(process.argv[2], () => {}); process.exit(0);`, + }); + const fifo = join(String(dir), "pipe.fifo"); + expect(Bun.spawnSync({ cmd: ["mkfifo", fifo] }).exitCode).toBe(0); + await using proc = Bun.spawn({ + cmd: [bunExe(), "main.cjs", fifo], + env: { ...bunEnv, BUN_DESTRUCT_VM_ON_EXIT: "1" }, + cwd: String(dir), + stderr: "pipe", + }); + const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + }, +); + +test.concurrent.skipIf(isWindows)( + "fs op completing after terminate() is discarded without touching the dead worker VM", + async () => { + using dir = tempDir("worker-terminate-late-completion", { + "main.cjs": ` + const { Worker, isMainThread, parentPort, workerData } = require("worker_threads"); + const fs = require("fs"); + if (isMainThread) { + const fifo = process.argv[2]; + const w = new Worker(__filename, { workerData: fifo }); + w.on("message", () => { + w.terminate().then(() => { + console.log("terminate resolved"); + // Unblock the stranded read now that the worker VM is gone; its + // completion must be refused at the gate and freed off-thread. + fs.open(fifo, "w", (err, fd) => { + if (err) throw err; + fs.write(fd, "x", () => { + fs.close(fd, () => { + // The refused completion runs on a detached pool thread in + // the torn-down worker VM; no cross-process signal for it + // exists, so this delay is best-effort scheduling slack + // (an ASAN fault there still fails the run). The + // assertions do not depend on it. + setTimeout(() => { + console.log("done"); + process.exit(0); + }, 250); + }); + }); + }); + }); + }); + } else { + // Buffer path + AbortSignal: their normal teardown cleanup (unpin, + // signal unref) must be skipped by the refused-post disposal, since + // both would touch the dead JS heap. + const controller = new AbortController(); + fs.readFile(Buffer.from(workerData), { signal: controller.signal }, () => {}); + parentPort.postMessage("pending"); + } + `, + }); + const fifo = join(String(dir), "pipe.fifo"); + expect(Bun.spawnSync({ cmd: ["mkfifo", fifo] }).exitCode).toBe(0); + await using proc = Bun.spawn({ + cmd: [bunExe(), "main.cjs", fifo], + // Malloc=1 forces system malloc for the JSC heap so ASAN can see a + // use-after-free if the disposal ever touches it. + env: { ...bunEnv, Malloc: "1" }, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stdout).toBe("terminate resolved\ndone\n"); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + }, +);