From 1654584b95f3a082989aa3d0f842b4c007d1cbff Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 14 Aug 2026 01:18:53 +0000 Subject: [PATCH 1/4] Release what a refused or unrun job produced: Bun.write's promise, transform output, readdir and mkdir results WriteFile carried the Box holding Bun.write's promise as a raw ctx pointer in its off-thread half, freed only by the completion. A job released instead of completed (the VM tore down first, or the pool's post was refused) leaked the box and kept the promise rooted. The promise is now the job's Js half (WriteFilePromise), released by the VM with its other live jobs; on Windows WriteFileWindows holds it as a field until it settles it. TransformTask.output_code, StringOrUndefined (ret::Mkdir) and ret::Readdir held bare bun_core::Strings / Dirents that only the to_js path released, so the same release paths leaked them too. output_code and the mkdir path are OwnedStrings now and ret::Readdir releases whatever to_js did not hand over in Drop, which also covers a conversion that fails part-way. The FromAny impl for Box<[bun_core::String]> had no other user and is gone. worker-refused-completion.test.ts gets rows for the four producers, starts each row's work from a microtask so leaksan.supp's module-evaluation entry does not hide what it allocates, and turns LSan on itself on ASAN builds. --- src/jsc/JSValue.rs | 13 - src/runtime/api/JSTranspiler.rs | 14 +- src/runtime/node/node_fs.rs | 54 ++-- src/runtime/webcore/Blob.rs | 27 +- src/runtime/webcore/blob/write_file.rs | 265 +++++++----------- .../workers/worker-refused-completion.test.ts | 98 +++++-- 6 files changed, 220 insertions(+), 251 deletions(-) diff --git a/src/jsc/JSValue.rs b/src/jsc/JSValue.rs index c4b8ab3f5c20..a7bdf686df88 100644 --- a/src/jsc/JSValue.rs +++ b/src/jsc/JSValue.rs @@ -1812,19 +1812,6 @@ impl FromAny for &str { bun_string_jsc::create_utf8_for_js(global, self.as_bytes()) } } -impl FromAny for Box<[bun_core::String]> { - /// The boxed - /// slice is consumed: every element's WTF refcount is dropped and the - /// backing allocation freed via `Box` drop. `bun_core::String` is `Copy` - /// with no `Drop`, so the explicit `deref()` loop is required. - fn into_js_value(self, global: &JSGlobalObject) -> JsResult { - let result = bun_string_jsc::to_js_array(global, &self); - for out in self.iter() { - out.deref(); - } - result - } -} impl FromAny for Option { /// `None` → `undefined`. #[inline] diff --git a/src/runtime/api/JSTranspiler.rs b/src/runtime/api/JSTranspiler.rs index 27b16aa07e26..e0ea565cee70 100644 --- a/src/runtime/api/JSTranspiler.rs +++ b/src/runtime/api/JSTranspiler.rs @@ -654,7 +654,9 @@ impl Config { /// which the job's Js side keeps alive and the pool borrow keeps valid. pub(crate) struct TransformTask { pub input_code: bun_jsc::ThreadSafe, - pub output_code: BunString, + /// Transferred to the promise by `finish`; released with the task on + /// every other path (a log with messages, a VM gone before `then`). + pub output_code: OwnedString, pub transpiler: core::mem::ManuallyDrop>, pub log: bun_ast::Log, pub err: Option, @@ -719,7 +721,7 @@ impl TransformTask { let task = TransformTask { input_code, - output_code: BunString::empty(), + output_code: OwnedString::default(), transpiler: transpiler_copy, macro_map: clone_macro_map(&config.macro_map), tsconfig: config @@ -831,7 +833,6 @@ impl TransformTask { }; if parse_result.empty { - self.output_code = BunString::empty(); return; } @@ -861,9 +862,7 @@ impl TransformTask { buffer_writer = printer.ctx; // `written()` reslices via `written_len`; copy out the printed // bytes, then the local writer is dropped. - self.output_code = BunString::clone_utf8(buffer_writer.written()); - } else { - self.output_code = BunString::empty(); + self.output_code = OwnedString::new(BunString::clone_utf8(buffer_writer.written())); } } @@ -872,8 +871,7 @@ impl TransformTask { promise: &mut JSPromise, global: &JSGlobalObject, ) -> Result<(), bun_jsc::JsTerminated> { - // The job drops this `TransformTask` (running its `Drop`: transpiler - // deref etc.) right after `then` returns. + // The job drops this `TransformTask` right after `then` returns. if self.log.has_any() || self.err.is_some() { let error_value: JsResult = 'brk: { if let Some(err) = &self.err { diff --git a/src/runtime/node/node_fs.rs b/src/runtime/node/node_fs.rs index 6046c4ad2fd5..954337befd4c 100644 --- a/src/runtime/node/node_fs.rs +++ b/src/runtime/node/node_fs.rs @@ -10,7 +10,7 @@ use core::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; use crate::api::bun::process::event_loop_handle_to_ctx; use crate::webcore; use bun_core::Environment; -use bun_core::{String as BunString, ZStr}; +use bun_core::{OwnedString, String as BunString, ZStr}; use bun_event_loop::AnyTaskWithExtraContext::AnyTaskWithExtraContext; use bun_io::KeepAlive; use bun_jsc::AbortSignal; @@ -1197,10 +1197,7 @@ mod _async_tasks { impl FsReturn for ret::Readdir { #[inline] fn fs_to_js(&mut self, global: &JSGlobalObject) -> JsResult { - // `Readdir::to_js` consumes by value (the boxed slices are handed to - // JS). Swap in an empty `Files` payload so `&mut self` stays valid. - let owned = core::mem::replace(self, ret::Readdir::Files(Box::default())); - owned.to_js(global) + self.to_js(global) } } impl FsReturn for StatOrNotFound { @@ -2220,7 +2217,7 @@ mod _async_tasks { } } } else { - let res = match core::mem::replace( + let mut res = match core::mem::replace( &mut this.result_list, ResultListEntryValue::Files(Vec::new()), ) { @@ -4430,8 +4427,11 @@ impl StatOrNotFound { } } +/// The string is owned until `to_js` transfers it; a result dropped +/// undelivered (its worker was gone before the completion could run) +/// releases it. pub enum StringOrUndefined { - String(BunString), + String(OwnedString), None, } impl StringOrUndefined { @@ -4508,15 +4508,18 @@ pub mod ret { Files, } + /// The entries are owned until `to_js` hands them to JS; whatever it did + /// not hand over (a conversion that failed part-way, a result dropped + /// undelivered because its worker was gone) is released by `Drop`. pub enum Readdir { WithFileTypes(Box<[Dirent]>), Buffers(Box<[Buffer]>), Files(Box<[BunString]>), } impl Readdir { - pub fn to_js(self, global_object: &JSGlobalObject) -> JsResult { + pub fn to_js(&mut self, global_object: &JSGlobalObject) -> JsResult { match self { - Readdir::WithFileTypes(mut items) => { + Readdir::WithFileTypes(items) => { let array = JSValue::create_empty_array(global_object, items.len())?; let mut previous_jsstring: *mut bun_jsc::JSString = core::ptr::null_mut(); for (i, item) in items.iter_mut().enumerate() { @@ -4524,14 +4527,12 @@ pub mod ret { item.to_js_newly_created(global_object, Some(&mut previous_jsstring))?; array.put_index(global_object, i as u32, res)?; } - // items dropped here (auto free) Ok(array) } - Readdir::Buffers(mut items) => { + Readdir::Buffers(items) => { // Node returns `Buffer[]` for `{ encoding: "buffer" }`, not // `Uint8Array[]`. Ownership of every `Buffer`'s bytes - // transfers to JSC via `to_node_buffer`; the boxed slice - // itself is freed when `items` drops. + // transfers to JSC via `to_node_buffer`. let array = JSValue::create_empty_array(global_object, items.len())?; for (i, item) in items.iter_mut().enumerate() { let res = item.to_node_buffer(global_object)?; @@ -4539,12 +4540,19 @@ pub mod ret { } Ok(array) } - Readdir::Files(items) => { - // Converted to a JS array, then every element is - // deref'd and the slice freed (handled by the `FromAny - // for Box<[bun_core::String]>` impl). - JSValue::from_any(global_object, items) - } + // The array takes its own refs; ours go in `Drop`. + Readdir::Files(items) => bun_jsc::bun_string_jsc::to_js_array(global_object, items), + } + } + } + impl Drop for Readdir { + fn drop(&mut self) { + match self { + // Transferred entries are empty by now; `deref` on those is a no-op. + Readdir::WithFileTypes(items) => items.iter().for_each(Dirent::deref), + // `Buffer` frees whatever it still owns itself. + Readdir::Buffers(_) => {} + Readdir::Files(items) => items.iter().for_each(BunString::deref), } } } @@ -5711,8 +5719,8 @@ impl NodeFS { if !RETURN_PATH { return Ok(StringOrUndefined::None); } - return Ok(StringOrUndefined::String(BunString::create_from_os_path( - &path[..], + return Ok(StringOrUndefined::String(OwnedString::new( + BunString::create_from_os_path(&path[..]), ))); } } @@ -5881,8 +5889,8 @@ impl NodeFS { if !RETURN_PATH { return Ok(StringOrUndefined::None); } - Ok(StringOrUndefined::String(BunString::create_from_os_path( - &working_mem[..first_match as usize], + Ok(StringOrUndefined::String(OwnedString::new( + BunString::create_from_os_path(&working_mem[..first_match as usize]), ))) } diff --git a/src/runtime/webcore/Blob.rs b/src/runtime/webcore/Blob.rs index eb149d320b7a..27ce747bf960 100644 --- a/src/runtime/webcore/Blob.rs +++ b/src/runtime/webcore/Blob.rs @@ -4634,26 +4634,16 @@ pub(crate) fn write_file_with_source_destination( let source_type = source_store.data.tag(); if destination_type == store::DataTag::File && source_type == store::DataTag::Bytes { - let write_file_promise = bun_core::heap::into_raw(Box::new(WriteFilePromise { - promise: jsc::JSPromiseStrong::default(), - global_this: ctx, - })); - - // The borrowed views below are +0 on the store ref; - // `WriteFile::create` takes its own ref. #[cfg(windows)] { - let promise = JSPromise::create(ctx); - let promise_value = promise.as_value(ctx); + let promise = WriteFilePromise::init(ctx); + let promise_value = promise.value(); promise_value.ensure_still_alive(); - // SAFETY: write_file_promise was just produced by heap::alloc above; sole owner. - unsafe { (*write_file_promise).promise.set(ctx, promise_value) }; match write_file_mod::WriteFileWindows::create( ctx.bun_vm().event_loop(), destination_blob.borrowed_view(), source_blob.borrowed_view(), - write_file_promise, - WriteFilePromise::run, + promise, options.mkdirp_if_not_exists.unwrap_or(true), ) { Err(write_file_mod::WriteFileWindowsError::WriteFileWindowsDeinitialized) => {} @@ -4670,18 +4660,13 @@ pub(crate) fn write_file_with_source_destination( let file_copier = write_file_mod::WriteFile::create( destination_blob.borrowed_view(), source_blob.borrowed_view(), - write_file_promise, - WriteFilePromise::run, options.mkdirp_if_not_exists.unwrap_or(true), ) .expect("unreachable"); - // Defer promise creation until we're just about to schedule the task. - // SAFETY: write_file_promise was just produced by heap::alloc above; sole owner. - unsafe { (*write_file_promise).promise = jsc::JSPromiseStrong::init(ctx) }; - // SAFETY: same `write_file_promise` as above; still solely owned here. - let promise_value = unsafe { (*write_file_promise).promise.value() }; + let promise = WriteFilePromise::init(ctx); + let promise_value = promise.value(); promise_value.ensure_still_alive(); - write_file_mod::WriteFile::schedule(file_copier, ctx); + write_file_mod::WriteFile::schedule(file_copier, promise, ctx); return Ok(promise_value); } } diff --git a/src/runtime/webcore/blob/write_file.rs b/src/runtime/webcore/blob/write_file.rs index 293d064020a5..bb21c3397593 100644 --- a/src/runtime/webcore/blob/write_file.rs +++ b/src/runtime/webcore/blob/write_file.rs @@ -24,30 +24,63 @@ use crate::webcore::body; bun_output::declare_scope!(WriteFile, hidden); -// A tagged result-or-error union. Modeled -// as a plain Rust enum: it only ever travels through the Rust fn-pointer -// callbacks below (`WriteFileOnWriteFileCallback`), never across FFI, so the -// layout is unconstrained. +/// What a finished write settles its promise with. pub enum WriteFileResultType { Result(SizeType), Err(Box), } -pub type WriteFileOnWriteFileCallback = - fn(ctx: *mut c_void, count: WriteFileResultType) -> Result<(), JsTerminated>; +/// The JS side of a file write: the promise `Bun.write()` handed to script. A +/// `WriteFile` job carries it as its `Js` half, so a write whose VM goes away +/// first is released with the VM's other live jobs, on the JS thread, instead +/// of staying rooted; `WriteFileWindows` holds it as a field until it finishes. +#[derive(bun_jsc::JsAffine)] +pub struct WriteFilePromise { + promise: jsc::JSPromiseStrong, +} + +impl WriteFilePromise { + pub(crate) fn init(global: &JSGlobalObject) -> Self { + Self { + promise: jsc::JSPromiseStrong::init(global), + } + } + + pub(crate) fn value(&self) -> JSValue { + self.promise.value() + } + + pub(crate) fn settle( + mut self, + global: &JSGlobalObject, + result: WriteFileResultType, + ) -> Result<(), JsTerminated> { + match result { + WriteFileResultType::Err(err) => { + let err_js = err.to_error_instance(global); + self.promise.reject_with_async_stack(global, Ok(err_js)) + } + WriteFileResultType::Result(wrote) => self + .promise + .resolve(global, JSValue::js_number_from_uint64(wrote as u64)), + } + } +} /// The completion token a `WriteFile` keeps across its async I/O. pub type WriteFileTask = bun_jsc::Completion; -// SAFETY: the two blobs are native values holding store refs (atomic counts); -// io-loop registration state and an opaque completion ctx that only the -// JS-thread completion dereferences — nothing used off-thread is thread-affine. +// SAFETY: the two blobs are native values holding store refs (atomic counts) +// and the rest is io-loop registration state — nothing thread-affine. The +// promise lives in the job's JS side (`WriteFilePromise`), never here. unsafe impl Send for WriteFile {} impl bun_jsc::JobContext for WriteFile { type OffThread = Self; - /// The completion is delivered through `on_complete_callback(ctx, ..)`. - type Js = (); + /// Settled by `then`, or dropped when the VM releases the JS sides of its + /// live jobs at teardown (a refused or unrun write then frees only the + /// off-thread part). + type Js = WriteFilePromise; fn run( this: &mut Self, _vm: &bun_jsc::vm_handle::Borrow, @@ -57,16 +90,21 @@ impl bun_jsc::JobContext for WriteFile { this.run(done); None } - fn then(this: Self, _: (), cx: &bun_jsc::JsThread<'_>) -> jsc::JsResult<()> { - Ok(WriteFile::then(this, cx.global())?) + fn then( + this: Self, + promise: WriteFilePromise, + cx: &bun_jsc::JsThread<'_>, + ) -> jsc::JsResult<()> { + Ok(WriteFile::then(this, promise, cx.global())?) } } +#[cfg(not(windows))] impl WriteFile { - /// JS thread: hand a prepared `WriteFile` to the work pool (the job is - /// its one heap allocation). - pub fn schedule(this: WriteFile, global: &JSGlobalObject) { - bun_jsc::Job::::schedule(&global.js_thread(), this, ()); + /// JS thread: hand a prepared `WriteFile` and the promise it settles to the + /// work pool (the job is its one heap allocation). + pub(crate) fn schedule(this: WriteFile, promise: WriteFilePromise, global: &JSGlobalObject) { + bun_jsc::Job::::schedule(&global.js_thread(), this, promise); } } @@ -85,8 +123,6 @@ pub struct WriteFile { pub(crate) io_request: io::Request, pub(crate) state: AtomicU8, // ClosingState - pub(crate) on_complete_ctx: *mut c_void, - pub(crate) on_complete_callback: WriteFileOnWriteFileCallback, pub(crate) total_written: usize, #[cfg(not(windows))] @@ -230,11 +266,9 @@ impl WriteFile { } #[cfg(not(windows))] - pub(crate) fn create_with_ctx( + pub(crate) fn create( file_blob: Blob, bytes_blob: Blob, - on_write_file_context: *mut c_void, - on_complete_callback: WriteFileOnWriteFileCallback, mkdirp_if_not_exists: bool, ) -> Result { let write_file = WriteFile { @@ -251,39 +285,17 @@ impl WriteFile { io_poll: io::Poll::default(), io_request: io::Request::new(Self::on_request_writable), state: AtomicU8::new(ClosingState::Running as u8), - on_complete_ctx: on_write_file_context, - on_complete_callback, total_written: 0, could_block: false, close_after_io: false, mkdirp_if_not_exists, }; - // No explicit store ref bump: the caller passes a `+1` Blob (via + // No explicit store ref bump: the caller passes `+1` Blobs (via // `borrowed_view()`'s `StoreRef::clone`) and dropping the `WriteFile` - // in `then` runs `StoreRef::drop`, so the ref/deref pair is RAII. + // runs `StoreRef::drop`, so the ref/deref pair is RAII. Ok(write_file) } - #[cfg(not(windows))] - pub(crate) fn create( - file_blob: Blob, - bytes_blob: Blob, - context: *mut C, - callback: WriteFileOnWriteFileCallback, - mkdirp_if_not_exists: bool, - ) -> Result { - // The caller supplies a - // `*mut c_void`-typed callback directly (see `WriteFilePromise::run`), - // so this is just a `.cast()` on `context`. - WriteFile::create_with_ctx( - file_blob, - bytes_blob, - context.cast::(), - callback, - mkdirp_if_not_exists, - ) - } - // reshaped for borrowck — take (off, len) here and re-derive the slice // internally so callers don't hold a borrow of self across the &mut self call. #[cfg(not(windows))] @@ -327,30 +339,19 @@ impl WriteFile { true } - pub(crate) fn then(mut this: WriteFile, _global: &JSGlobalObject) -> Result<(), JsTerminated> { - let cb = this.on_complete_callback; - let cb_ctx = this.on_complete_ctx; - let system_error = this.system_error.take(); - let total_written = this.total_written; - // Cleanup is RAII: dropping the `Box` runs `WriteFile`'s field-drop - // glue, which drops `bytes_blob.store`/`file_blob.store: Option< - // StoreRef>` → `Store::deref()` — exactly one deref each. - // (An earlier explicit `detach()` here was a no-op; the - // bun-write-leak.test.ts failure was the ASAN debug build's ~320 MB - // baseline RSS exceeding the fixture's 256 MB absolute threshold, - // not an unbalanced ref.) + pub(crate) fn then( + mut this: WriteFile, + promise: WriteFilePromise, + global: &JSGlobalObject, + ) -> Result<(), JsTerminated> { + let result = match this.system_error.take() { + Some(err) => WriteFileResultType::Err(Box::new(err)), + None => WriteFileResultType::Result(this.total_written as SizeType), + }; + // Both blobs' store refs go with the drop (`StoreRef::drop`); there is + // nothing to detach by hand. drop(this); - - if let Some(err) = system_error { - cb(cb_ctx, WriteFileResultType::Err(Box::new(err)))?; - return Ok(()); - } - - cb( - cb_ctx, - WriteFileResultType::Result(total_written as SizeType), - )?; - Ok(()) + promise.settle(global, result) } pub(crate) fn run(&mut self, task: WriteFileTask) { @@ -572,8 +573,9 @@ mod windows_impl { pub(crate) io_request: uv::fs_t, pub(crate) file_blob: Blob, pub(crate) bytes_blob: Blob, - pub(crate) on_complete_callback: WriteFileOnWriteFileCallback, - pub(crate) on_complete_ctx: *mut c_void, + /// `Some` until the write finishes; a `WriteFileWindows` freed before + /// then drops it, unrooting the promise. + pub(crate) promise: Option, pub(crate) mkdirp_if_not_exists: bool, pub(crate) uv_bufs: [uv::uv_buf_t; 1], @@ -611,12 +613,13 @@ mod windows_impl { } impl WriteFileWindows { - pub(crate) fn create_with_ctx( + /// Starts the write. `Err(WriteFileWindowsDeinitialized)` means it + /// failed before returning and `promise` has already been rejected. + pub(crate) fn create( + event_loop: *mut EventLoop, file_blob: Blob, bytes_blob: Blob, - event_loop: *mut EventLoop, - on_write_file_context: *mut c_void, - on_complete_callback: WriteFileOnWriteFileCallback, + promise: WriteFilePromise, mkdirp_if_not_exists: bool, ) -> Result<*mut WriteFileWindows, WriteFileWindowsError> { let mkdirp = mkdirp_if_not_exists @@ -632,8 +635,7 @@ mod windows_impl { let write_file = Self::new(WriteFileWindows { file_blob, bytes_blob, - on_complete_ctx: on_write_file_context, - on_complete_callback, + promise: Some(promise), mkdirp_if_not_exists: mkdirp, io_request: bun_core::ffi::zeroed::(), uv_bufs: [uv::uv_buf_t { @@ -1049,28 +1051,29 @@ mod windows_impl { /// `this` must point to a live `WriteFileWindows` allocated via [`Self::new`]. /// On return, `*this` has been freed and must not be accessed again. pub(crate) unsafe fn run_from_js_thread(this: *mut Self) -> WriteFileWindowsError { - // SAFETY: caller contract — `this` is live; copy out everything we + // SAFETY: caller contract — `this` is live; take out everything we // need before `deinit` frees the allocation. - let (cb, cb_ctx) = unsafe { ((*this).on_complete_callback, (*this).on_complete_ctx) }; + let (promise, global, result) = unsafe { + let result = match (*this).to_system_error() { + Some(err) => WriteFileResultType::Err(Box::new(err)), + None => WriteFileResultType::Result((*this).total_written as SizeType), + }; + ( + (*this) + .promise + .take() + .expect("a WriteFileWindows finishes once"), + (*(*this).event_loop).global_ref(), + result, + ) + }; + // SAFETY: caller contract — `this` is live; consumed here. + unsafe { Self::deinit(this) }; - // SAFETY: caller contract — `this` is live. - if let Some(err) = unsafe { (*this).to_system_error() } { - // SAFETY: caller contract — `this` is live; consumed here. - unsafe { Self::deinit(this) }; - if let Err(e) = cb(cb_ctx, WriteFileResultType::Err(Box::new(err))) { - return e.into(); - } - } else { - // SAFETY: caller contract — `this` is live. - let wrote = unsafe { (*this).total_written }; - // SAFETY: caller contract — `this` is live; consumed here. - unsafe { Self::deinit(this) }; - if let Err(e) = cb(cb_ctx, WriteFileResultType::Result(wrote as SizeType)) { - return e.into(); - } + match promise.settle(global, result) { + Ok(()) => WriteFileWindowsError::WriteFileWindowsDeinitialized, + Err(terminated) => terminated.into(), } - - WriteFileWindowsError::WriteFileWindowsDeinitialized } /// # Safety @@ -1200,7 +1203,7 @@ mod windows_impl { aio::Closer::close(Fd::from_uv(fd), (*this).io_request.loop_); } // The store derefs happen via `StoreRef::drop` when the Box is - // reclaimed below (paired with the RAII note in `create_with_ctx`). + // reclaimed below (paired with the RAII note in `create`). (*this).poll_ref.disable(); // (*this).io_request is a valid uv_fs_t embedded in this struct; uv_fs_req_cleanup // is safe on a zeroed or previously-used req. @@ -1209,74 +1212,6 @@ mod windows_impl { drop(bun_core::heap::take(this)); } } - - pub(crate) fn create( - event_loop: *mut EventLoop, - file_blob: Blob, - bytes_blob: Blob, - context: *mut C, - callback: WriteFileOnWriteFileCallback, - mkdirp_if_not_exists: bool, - ) -> Result<*mut WriteFileWindows, WriteFileWindowsError> { - // see `WriteFile::create` — caller supplies an erased - // `*mut c_void` callback directly; `context` is just `.cast()`ed. - WriteFileWindows::create_with_ctx( - file_blob, - bytes_blob, - event_loop, - context.cast::(), - callback, - mkdirp_if_not_exists, - ) - } - } -} - -// ────────────────────────────────────────────────────────────────────────── - -pub struct WriteFilePromise { - pub(crate) promise: jsc::JSPromiseStrong, - pub global_this: *const JSGlobalObject, -} - -impl WriteFilePromise { - pub(crate) fn run( - handler: *mut c_void, - count: WriteFileResultType, - ) -> Result<(), JsTerminated> { - let handler = handler.cast::(); - // SAFETY: handler is the Box-allocated WriteFilePromise created in - // Blob.rs (`heap::into_raw(Box::new(WriteFilePromise { .. }))`); consumed here. - // `swap()` releases the Strong's handle slot and yields a GC-owned `*mut JSPromise`, - // which stays valid past `drop(heap::take(handler))`. - let (promise, global_this): (*mut JSPromise, &JSGlobalObject) = unsafe { - let h = &mut *handler; - let promise = std::ptr::from_mut::(h.promise.swap()); - let global_this = &*h.global_this; - drop(bun_core::heap::take(handler)); - (promise, global_this) - }; - // SAFETY: GC-owned cell (kept alive below); scoped shared access. - let value = unsafe { (*promise).to_js() }; - value.ensure_still_alive(); - match count { - WriteFileResultType::Err(err) => { - // SAFETY: GC-owned cell; the error build's shared borrow ends before the - // scoped exclusive `reject` borrow. - unsafe { - let err_js = err.to_error_instance_with_async_stack(global_this, &*promise); - (*promise).reject(global_this, Ok(err_js))?; - } - } - WriteFileResultType::Result(wrote) => { - // SAFETY: GC-owned cell; exclusive borrow scoped to the call. - unsafe { - (*promise) - .resolve(global_this, JSValue::js_number_from_uint64(wrote as u64))?; - } - } - } - Ok(()) } } diff --git a/test/js/web/workers/worker-refused-completion.test.ts b/test/js/web/workers/worker-refused-completion.test.ts index 20936d3485ed..5c25f0da6f04 100644 --- a/test/js/web/workers/worker-refused-completion.test.ts +++ b/test/js/web/workers/worker-refused-completion.test.ts @@ -7,14 +7,17 @@ // stderr. A row passes only if the named refusal happened (the work really was // on another thread and its release path ran) and the process exited cleanly; // on the ASAN build the release path is also checked for use-after-free and -// leaks. Builds with debug assertions only (debug, ASAN): the gate does not -// exist in release builds. +// leaks (LSan is turned on below, as CI's ASAN lane does for every test). +// Builds with debug assertions only (debug, ASAN): the gate does not exist in +// release builds. import { describe, expect, test } from "bun:test"; -import { bunEnv, bunExe, isASAN, isDebug, isWindows } from "harness"; +import { bunEnv, bunExe, isASAN, isDebug, isWindows, tempDir } from "harness"; +import { join } from "node:path"; type Row = { name: string; - // Runs in the worker before it exits; starts exactly one piece of off-thread work. + // Runs in the worker before it exits (cwd: a scratch directory holding + // SCRATCH_FILES); starts exactly one piece of off-thread work. worker: string; // Substring of the refusal the runtime must log for it. refused: string; @@ -48,6 +51,30 @@ const ROWS: Row[] = [ worker: `new Bun.Image(Bun.file(process.execPath).slice(0, 65536)).metadata().catch(() => {});`, refused: "blob::read_file::ReadFile", }, + { + // A Blob source: a string or buffer this small is written synchronously instead. + name: "Bun.write()", + worker: `Bun.write("written-by-the-pool", new Blob(["refused"]));`, + refused: "blob::write_file::WriteFile", + }, + // The result the pool produced (names / Dirents / the created path / the + // transpiled code) is what must be released along with the refused job. + { name: "fs.promises.readdir", worker: `require("node:fs").promises.readdir(".");`, refused: "args::Readdir" }, + { + name: "fs.promises.readdir withFileTypes", + worker: `require("node:fs").promises.readdir(".", { withFileTypes: true });`, + refused: "args::Readdir", + }, + { + name: "fs.promises.mkdir recursive", + worker: `require("node:fs").promises.mkdir("made/by/the/pool", { recursive: true });`, + refused: "args::Mkdir", + }, + { + name: "Bun.Transpiler transform()", + worker: `new Bun.Transpiler({ loader: "ts" }).transform("export const refused: number = 1;");`, + refused: "TransformTask", + }, { name: "crypto.pbkdf2", worker: `require("node:crypto").pbkdf2("p", "s", 1000, 32, "sha256", () => {});`, @@ -107,11 +134,19 @@ const ROWS: Row[] = [ }, ]; +// The host's cwd: what the readdir rows list, where the write and mkdir rows +// put what the pool produces; thrown away with the row. +const SCRATCH_FILES = { "a.txt": "", "b.txt": "", "c.txt": "", "d.txt": "" }; + // The host: one worker, armed gate. The worker starts its work, reports // "armed" and exits by itself two turns later; the parent never posts anything // the worker waits for (with the gate armed, a parent→worker post is itself a // cross-thread completion that waits for the worker's close). Rows with a // parent side post in response to "armed": that post is what gets refused. +// +// The row's work starts from a microtask: test/leaksan.supp suppresses every +// allocation made under the module evaluation (leak:Bun::evaluateCommonJSModuleOnce), +// which would also hide what a row allocates while starting its work. function host(row: Row) { return ` const { Worker, MessageChannel } = require("node:worker_threads"); @@ -119,7 +154,7 @@ function host(row: Row) { const w = new Worker(\` const { parentPort, workerData } = require("node:worker_threads"); parentPort.on("message", () => {}); - ${row.worker.replace(/`/g, "\\`").replace(/\$\{/g, "\\${")} + queueMicrotask(() => { ${row.worker.replace(/`/g, "\\`").replace(/\$\{/g, "\\${")} }); parentPort.postMessage("armed"); setImmediate(() => setImmediate(() => process.exit(0))); \`, { eval: true, workerData: { port: port2 }, transferList: [port2] }); @@ -129,26 +164,47 @@ function host(row: Row) { `; } +// What a refused job failed to release shows up as an LSan report, and a +// non-zero exit, on the ASAN build (the same options CI's ASAN lane sets). +// LSan cannot see into the JS heap, so the host's own VM has to be torn down +// at exit too, or everything its wrappers still own would be reported. +const LEAK_CHECK_ENV = isASAN + ? { + BUN_DESTRUCT_VM_ON_EXIT: "1", + ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "detect_leaks=1"].filter(Boolean).join(":"), + LSAN_OPTIONS: `print_suppressions=0:suppressions=${join(import.meta.dirname, "../../../leaksan.supp")}`, + } + : {}; + describe.skipIf(!isDebug && !isASAN)( "a completion for a worker that is gone is refused and released by its producer", () => { for (const row of ROWS) { - test.concurrent.skipIf(!!row.skip)(row.name, async () => { - await using proc = Bun.spawn({ - cmd: [bunExe(), "-e", host(row)], - env: { ...bunEnv, ...row.env, BUN_DEBUG_TEST_WORKER_REFUSAL_GATE: "1" }, - stdout: "pipe", - stderr: "pipe", - }); - const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - const refusals = stderr.split("\n").filter(l => l.startsWith("[vm_handle] refused ")); - expect({ - exitCode, - refused: refusals.some(l => l.includes(row.refused)), - // On a failure, everything the host printed. - detail: exitCode === 0 && refusals.some(l => l.includes(row.refused)) ? "" : stdout + stderr, - }).toEqual({ exitCode: 0, refused: true, detail: "" }); - }); + test.concurrent.skipIf(!!row.skip)( + row.name, + async () => { + using scratch = tempDir("worker-refused-completion", SCRATCH_FILES); + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", host(row)], + cwd: String(scratch), + env: { ...bunEnv, ...LEAK_CHECK_ENV, ...row.env, BUN_DEBUG_TEST_WORKER_REFUSAL_GATE: "1" }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + const refusals = stderr.split("\n").filter(l => l.startsWith("[vm_handle] refused ")); + expect({ + exitCode, + refused: refusals.some(l => l.includes(row.refused)), + // On a failure, everything the host printed. + detail: exitCode === 0 && refusals.some(l => l.includes(row.refused)) ? "" : stdout + stderr, + }).toEqual({ exitCode: 0, refused: true, detail: "" }); + }, + // Only debug and ASAN builds get here (see the describe), and on those + // one worker lifecycle is a few seconds of CPU; bun test starts twenty + // of these hosts at once, so the slowest rows pass 5s on a busy box. + 15_000, + ); } }, ); From 15271bf0800f1cceca3903fbb0d54b4136bb2450 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 14 Aug 2026 01:46:25 +0000 Subject: [PATCH 2/4] test: skip the blob read and write rows on Windows, where they are libuv requests rather than pool jobs --- test/js/web/workers/worker-refused-completion.test.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/js/web/workers/worker-refused-completion.test.ts b/test/js/web/workers/worker-refused-completion.test.ts index 5c25f0da6f04..10522dbda60f 100644 --- a/test/js/web/workers/worker-refused-completion.test.ts +++ b/test/js/web/workers/worker-refused-completion.test.ts @@ -39,10 +39,13 @@ const ROWS: Row[] = [ worker: `require("node:fs").realpath(process.execPath, () => {});`, refused: "args::Realpath", }, + // On Windows these three are libuv requests on the worker's own loop + // (ReadFileUV, WriteFileWindows), not pool jobs: nothing gets refused. { name: "Bun.file().text()", worker: `Bun.file(process.execPath).slice(0, 65536).text();`, refused: "blob::read_file::ReadFile", + skip: isWindows, }, { // Same read job, different completion: the image's read chain is handed @@ -50,12 +53,14 @@ const ROWS: Row[] = [ name: "Bun.Image(Bun.file()).metadata()", worker: `new Bun.Image(Bun.file(process.execPath).slice(0, 65536)).metadata().catch(() => {});`, refused: "blob::read_file::ReadFile", + skip: isWindows, }, { // A Blob source: a string or buffer this small is written synchronously instead. name: "Bun.write()", worker: `Bun.write("written-by-the-pool", new Blob(["refused"]));`, refused: "blob::write_file::WriteFile", + skip: isWindows, }, // The result the pool produced (names / Dirents / the created path / the // transpiled code) is what must be released along with the refused job. From 27a670a01b615d8a30e9d7f1158014c9b688b8e8 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 14 Aug 2026 01:54:44 +0000 Subject: [PATCH 3/4] Trim the doc comments on the new ownership types to one line each --- src/runtime/api/JSTranspiler.rs | 2 -- src/runtime/node/node_fs.rs | 7 +------ src/runtime/webcore/blob/write_file.rs | 25 +++++++------------------ 3 files changed, 8 insertions(+), 26 deletions(-) diff --git a/src/runtime/api/JSTranspiler.rs b/src/runtime/api/JSTranspiler.rs index e0ea565cee70..81a721b86d0d 100644 --- a/src/runtime/api/JSTranspiler.rs +++ b/src/runtime/api/JSTranspiler.rs @@ -654,8 +654,6 @@ impl Config { /// which the job's Js side keeps alive and the pool borrow keeps valid. pub(crate) struct TransformTask { pub input_code: bun_jsc::ThreadSafe, - /// Transferred to the promise by `finish`; released with the task on - /// every other path (a log with messages, a VM gone before `then`). pub output_code: OwnedString, pub transpiler: core::mem::ManuallyDrop>, pub log: bun_ast::Log, diff --git a/src/runtime/node/node_fs.rs b/src/runtime/node/node_fs.rs index 954337befd4c..c74a2310e671 100644 --- a/src/runtime/node/node_fs.rs +++ b/src/runtime/node/node_fs.rs @@ -4427,9 +4427,6 @@ impl StatOrNotFound { } } -/// The string is owned until `to_js` transfers it; a result dropped -/// undelivered (its worker was gone before the completion could run) -/// releases it. pub enum StringOrUndefined { String(OwnedString), None, @@ -4508,9 +4505,7 @@ pub mod ret { Files, } - /// The entries are owned until `to_js` hands them to JS; whatever it did - /// not hand over (a conversion that failed part-way, a result dropped - /// undelivered because its worker was gone) is released by `Drop`. + /// `to_js` converts in place; `Drop` releases whatever it did not hand over. pub enum Readdir { WithFileTypes(Box<[Dirent]>), Buffers(Box<[Buffer]>), diff --git a/src/runtime/webcore/blob/write_file.rs b/src/runtime/webcore/blob/write_file.rs index bb21c3397593..907fa5e78b60 100644 --- a/src/runtime/webcore/blob/write_file.rs +++ b/src/runtime/webcore/blob/write_file.rs @@ -30,10 +30,7 @@ pub enum WriteFileResultType { Err(Box), } -/// The JS side of a file write: the promise `Bun.write()` handed to script. A -/// `WriteFile` job carries it as its `Js` half, so a write whose VM goes away -/// first is released with the VM's other live jobs, on the JS thread, instead -/// of staying rooted; `WriteFileWindows` holds it as a field until it finishes. +/// The promise `Bun.write()` returned: a `WriteFile` job's JS half, or a `WriteFileWindows` field. #[derive(bun_jsc::JsAffine)] pub struct WriteFilePromise { promise: jsc::JSPromiseStrong, @@ -70,16 +67,12 @@ impl WriteFilePromise { /// The completion token a `WriteFile` keeps across its async I/O. pub type WriteFileTask = bun_jsc::Completion; -// SAFETY: the two blobs are native values holding store refs (atomic counts) -// and the rest is io-loop registration state — nothing thread-affine. The -// promise lives in the job's JS side (`WriteFilePromise`), never here. +// SAFETY: the two blobs hold store refs (atomic counts) and the rest is io-loop +// registration state; the promise lives on the job's JS side, never here. unsafe impl Send for WriteFile {} impl bun_jsc::JobContext for WriteFile { type OffThread = Self; - /// Settled by `then`, or dropped when the VM releases the JS sides of its - /// live jobs at teardown (a refused or unrun write then frees only the - /// off-thread part). type Js = WriteFilePromise; fn run( this: &mut Self, @@ -101,8 +94,7 @@ impl bun_jsc::JobContext for WriteFile { #[cfg(not(windows))] impl WriteFile { - /// JS thread: hand a prepared `WriteFile` and the promise it settles to the - /// work pool (the job is its one heap allocation). + /// JS thread: hand the write and the promise it settles to the work pool. pub(crate) fn schedule(this: WriteFile, promise: WriteFilePromise, global: &JSGlobalObject) { bun_jsc::Job::::schedule(&global.js_thread(), this, promise); } @@ -348,8 +340,7 @@ impl WriteFile { Some(err) => WriteFileResultType::Err(Box::new(err)), None => WriteFileResultType::Result(this.total_written as SizeType), }; - // Both blobs' store refs go with the drop (`StoreRef::drop`); there is - // nothing to detach by hand. + // Releases both blobs' store refs; nothing to detach by hand. drop(this); promise.settle(global, result) } @@ -573,8 +564,7 @@ mod windows_impl { pub(crate) io_request: uv::fs_t, pub(crate) file_blob: Blob, pub(crate) bytes_blob: Blob, - /// `Some` until the write finishes; a `WriteFileWindows` freed before - /// then drops it, unrooting the promise. + /// `Some` until `run_from_js_thread` settles it. pub(crate) promise: Option, pub(crate) mkdirp_if_not_exists: bool, pub(crate) uv_bufs: [uv::uv_buf_t; 1], @@ -613,8 +603,7 @@ mod windows_impl { } impl WriteFileWindows { - /// Starts the write. `Err(WriteFileWindowsDeinitialized)` means it - /// failed before returning and `promise` has already been rejected. + /// `Err(WriteFileWindowsDeinitialized)`: failed synchronously, `promise` already rejected. pub(crate) fn create( event_loop: *mut EventLoop, file_blob: Blob, From ac8b9666bf94b657928bb8a1a3f3681194655b4d Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 14 Aug 2026 02:09:55 +0000 Subject: [PATCH 4/4] Point on_open's SAFETY comment at create, the constructor that now sets io_request.data --- src/runtime/webcore/blob/write_file.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/runtime/webcore/blob/write_file.rs b/src/runtime/webcore/blob/write_file.rs index 907fa5e78b60..31d20b6f42cb 100644 --- a/src/runtime/webcore/blob/write_file.rs +++ b/src/runtime/webcore/blob/write_file.rs @@ -805,7 +805,7 @@ mod windows_impl { let this: *mut WriteFileWindows = unsafe { WriteFileWindows::from_uv_fs(req) }; debug_assert!(core::ptr::eq( this, - // SAFETY: req == &(*this).io_request; data was set to `this` in create_with_ctx/open. + // SAFETY: req == &(*this).io_request; data was set to `this` in create/open. unsafe { (*req).data }.cast::() )); // SAFETY: `this` is live (libuv invokes us with the req we registered).