jsc: remove AbortSignal::unref/detach; add abort_signal::PendingActivityRef as the owned hold on a signal

src/jsc/AbortSignal.rs

@@ -1,3 +1,4 @@
+use core::cell::Cell;
 use core::ffi::c_void;
 use core::ptr::NonNull;
 use core::sync::atomic::Ordering;
@@ -47,6 +48,7 @@ unsafe extern "C" {
     safe fn WebCore__AbortSignal__fromJS(value0: JSValue) -> *mut AbortSignal;
     safe fn WebCore__AbortSignal__ref(arg0: &AbortSignal) -> *mut AbortSignal;
     safe fn WebCore__AbortSignal__toJS(arg0: &AbortSignal, arg1: &JSGlobalObject) -> JSValue;
+    // Only called from `ext_deref`; see `ref_`.
     safe fn WebCore__AbortSignal__unref(arg0: &AbortSignal);
     // `*mut Timeout` is round-tripped opaquely through C++ (stored from
     // `AbortSignal__Timeout__create`, never dereferenced on the C++ side), so
@@ -159,19 +161,14 @@ impl AbortSignal {
         ))
     }
 
+    /// Takes a ref; release it by adopting it into an [`AbortSignalRef`]. There
+    /// is no `unref(&self)` on purpose: `&AbortSignal` does not prove ownership
+    /// of a ref (every `AbortSignalRef` derefs to one), so a safe release here
+    /// would let safe code double-release.
     pub fn ref_(&self) -> *mut AbortSignal {
         WebCore__AbortSignal__ref(self)
     }
 
-    pub fn unref(&self) {
-        WebCore__AbortSignal__unref(self)
-    }
-
-    pub fn detach(&self, ctx: *mut c_void) {
-        self.clean_native_bindings(ctx);
-        self.unref();
-    }
-
     /// Lifetime: the returned pointer is borrowed from the JS wrapper and is
     /// valid only while `value` remains reachable. Use [`AbortSignal::ref_from_js`]
     /// to take refcounted ownership instead.
@@ -188,9 +185,11 @@ impl AbortSignal {
         WebCore__AbortSignal__create(global)
     }
 
-    pub fn new(global: &JSGlobalObject) -> *mut AbortSignal {
+    pub fn new(global: &JSGlobalObject) -> AbortSignalRef {
         crate::mark_binding!();
-        WebCore__AbortSignal__new(global)
+        // SAFETY: C++ returns `leakRef()` of a fresh signal, i.e. the `+1`
+        // adopted here.
+        unsafe { AbortSignalRef::adopt(WebCore__AbortSignal__new(global)) }
     }
 
     /// Returns a borrowed handle to the internal Timeout, or null.
@@ -199,9 +198,9 @@ impl AbortSignal {
     ///
     /// Thread-safety: not thread-safe; call only on the owning thread/loop.
     ///
-    /// Usage: if you need to operate on the Timeout (run/cancel/deinit), hold a ref
-    /// to `this` for the duration (e.g., `this.ref_(); defer this.unref();`) and avoid
-    /// caching the pointer across turns.
+    /// Usage: if you need to operate on the Timeout (run/cancel/deinit), hold an
+    /// [`AbortSignalRef`] to `self` for the duration and avoid caching the
+    /// pointer across turns.
     pub fn get_timeout(&self) -> Option<&Timeout> {
         let ptr = WebCore__AbortSignal__getTimeout(self);
         // SAFETY: returned Timeout is owned by `self` and valid while `self` is held
@@ -256,6 +255,66 @@ impl AbortSignal {
     }
 }
 
+/// What a native operation holds on a signal while it is in flight: a ref, a
+/// pending-activity count (keeps the JS wrapper and its `abort` listeners
+/// alive), and at most one native listener. `Drop` gives all three back, so
+/// holders keep an `Option<PendingActivityRef>` and release by taking it out.
+pub struct PendingActivityRef {
+    signal: AbortSignalRef,
+    /// Null until [`Self::add_listener`].
+    listener_ctx: Cell<*mut c_void>,
+}
+
+impl PendingActivityRef {
+    pub fn new(signal: AbortSignalRef) -> Self {
+        signal.pending_activity_ref();
+        Self {
+            signal,
+            listener_ctx: Cell::new(core::ptr::null_mut()),
+        }
+    }
+
+    /// [`AbortSignal::add_listener`], but removed again when `self` drops.
+    /// Takes `&self` so it shadows the `Deref`'d original for every receiver.
+    pub fn add_listener(
+        &self,
+        ctx: *mut c_void,
+        callback: unsafe extern "C" fn(*mut c_void, JSValue),
+    ) {
+        debug_assert!(
+            self.listener_ctx.get().is_null(),
+            "PendingActivityRef already has a listener"
+        );
+        self.listener_ctx.set(ctx);
+        self.signal.add_listener(ctx, callback);
+    }
+
+    pub fn signal_ref(&self) -> AbortSignalRef {
+        self.signal.clone()
+    }
+}
+
+impl core::ops::Deref for PendingActivityRef {
+    type Target = AbortSignal;
+    #[inline]
+    fn deref(&self) -> &AbortSignal {
+        &self.signal
+    }
+}
+
+impl Drop for PendingActivityRef {
+    fn drop(&mut self) {
+        let ctx = self.listener_ctx.get();
+        if !ctx.is_null() {
+            self.signal.clean_native_bindings(ctx);
+        }
+        // Second on purpose: `cleanNativeBindings` cancels an `AbortSignal.timeout`
+        // timer once nothing observes the signal, and pending activity counts
+        // as observing it. The ref itself goes when `self.signal` drops.
+        self.signal.pending_activity_unref();
+    }
+}
+
 pub enum AbortReason {
     Common(CommonAbortReason),
     Js(JSValue),

src/runtime/api/bun/h2_frame_parser.rs

@@ -32,7 +32,8 @@ use bun_jsc::abort_signal::AbortListener;
 use bun_jsc::array_buffer::BinaryType;
 use bun_jsc::virtual_machine::VirtualMachine;
 use bun_jsc::{
-    CallFrame, GlobalRef, JSGlobalObject, JSValue, JsCell, JsClass, JsRef, JsResult, StrongOptional,
+    AbortSignalRef, CallFrame, GlobalRef, JSGlobalObject, JSValue, JsCell, JsClass, JsRef,
+    JsResult, StrongOptional,
 };
 use bun_ptr::IntrusiveRc;
 
@@ -1549,14 +1550,7 @@ pub struct Stream {
 }
 
 pub(crate) struct SignalRef {
-    // LIFETIMES.tsv: SHARED — AbortSignal is intrusively refcounted across FFI/codegen.
-    // `AbortSignal` is an opaque C++ type whose ref/unref go through
-    // `WebCore__AbortSignal__ref/unref`; it does not (and cannot) implement
-    // `bun_ptr::RefCounted`, so balance refs by hand in `attach_signal` /
-    // `Drop`. `BackRef` captures the backref invariant
-    // (signal is `ref_()`'d in `attach_signal` and outlives this struct until
-    // `Drop` calls `detach()`/`unref()`), so reads go through safe `Deref`.
-    signal: bun_ptr::BackRef<AbortSignal>,
+    signal: AbortSignalRef,
     // LIFETIMES.tsv: SHARED — H2FrameParser carries an intrusive RefCount and is
     // recovered via `from_field_ptr!` from the auto-flusher. It uses a hand-rolled
     // `Cell<u32>` ref count (not `bun_ptr::RefCount<Self>`), so `IntrusiveRc`'s
@@ -1570,7 +1564,6 @@ pub(crate) struct SignalRef {
 
 impl SignalRef {
     pub(crate) fn is_aborted(&self) -> bool {
-        // BackRef invariant: signal kept alive via .ref_() in attach_signal.
         self.signal.aborted()
     }
 
@@ -1594,12 +1587,9 @@ impl SignalRef {
 
 impl Drop for SignalRef {
     fn drop(&mut self) {
-        // BackRef invariant: `signal` is the C++-refcounted AbortSignal we
-        // ref_()'d in `attach_signal`; valid until this `detach` releases our
-        // listener and unrefs. Copy the `BackRef` out first so the `&mut self`
-        // taken by `from_mut` doesn't overlap the receiver borrow.
-        let signal = self.signal;
-        signal.detach(std::ptr::from_mut(self).cast::<c_void>());
+        // `attach_signal` registered the listener with `self`'s address as ctx.
+        let ctx = std::ptr::from_mut(self).cast::<c_void>();
+        self.signal.clean_native_bindings(ctx);
         // ParentRef backref — parser outlives every SignalRef (ref()'d in
         // `attach_signal`); release that ref now via the inherent `deref()`.
         H2FrameParser::deref(self.parser.get());
@@ -2120,20 +2110,19 @@ impl Stream {
             .unwrap_or_else(|| JSValue::js_number(self.id as f64))
     }
 
-    pub fn attach_signal(&mut self, parser: &H2FrameParser, signal: &mut AbortSignal) {
-        // `ref_()` bumps the C++ intrusive refcount and returns the same live
-        // `self` pointer with FFI (wildcard) provenance — store *that* in the
-        // `BackRef` so its validity is tied to the refcount, not to the
-        // borrowed `&mut AbortSignal` parameter's lifetime.
-        let refed = core::ptr::NonNull::new(signal.ref_()).expect("AbortSignal::ref_");
+    pub fn attach_signal(&mut self, parser: &H2FrameParser, signal: &AbortSignal) {
+        // SAFETY: `signal` is live (borrowed from the JS wrapper the caller is
+        // holding); `ref_()` returns the same pointer carrying a `+1`, which
+        // the `AbortSignalRef` now owns.
+        let owned = unsafe { AbortSignalRef::adopt(signal.ref_()) };
         // we need a stable pointer to know what signal points to what stream_id + parser
         let mut signal_ref = Box::new(SignalRef {
-            signal: bun_ptr::BackRef::from(refed),
+            signal: owned,
             parser: bun_ptr::ParentRef::new(parser),
             stream_id: self.id,
         });
         // `signal_ref` is heap-allocated and outlives the listener registration
-        // (cleared via `detach` in `Drop for SignalRef`).
+        // (removed in `Drop for SignalRef`).
         signal.listen(&raw mut *signal_ref);
         // TODO: We should not need this ref counting here, since Parser owns Stream
         parser.ref_();
@@ -9360,8 +9349,7 @@ impl H2FrameParser {
 
             if let Some(signal_arg) = options.get(global_object, "signal")? {
                 if let Some(signal_ptr) = AbortSignal::from_js(signal_arg) {
-                    // SAFETY: `from_js` returns a live *mut AbortSignal owned by JSC; rooted via `signal_arg` on the stack.
-                    let signal_ = unsafe { &mut *signal_ptr };
+                    let signal_ = AbortSignal::opaque_ref(signal_ptr);
                     if signal_.aborted() {
                         stream.state = StreamState::IDLE;
                         let wrapped =

src/runtime/api/bun/js_bun_spawn_bindings.rs

@@ -367,22 +367,13 @@ fn spawn_maybe_sync<const IS_SYNC: bool>(
     let mut windows_hide: bool = false;
     #[cfg(windows)]
     let mut windows_verbatim_arguments: bool = false;
-    let mut abort_signal: Option<*mut WebCore::AbortSignal> = None;
+    let mut abort_signal: Option<jsc::AbortSignalRef> = None;
     let mut terminal_info: Option<TerminalCreateResult> = None;
     let mut existing_terminal: Option<bun_ptr::BackRef<Terminal, bun_ptr::Mut>> = None; // Existing terminal passed by user
     let mut terminal_js_value: JSValue = JSValue::ZERO;
     let mut defer_guard = scopeguard::guard(
-        (&mut abort_signal, &mut terminal_info),
-        |(abort_signal, terminal_info): (
-            &mut Option<*mut WebCore::AbortSignal>,
-            &mut Option<TerminalCreateResult>,
-        )| {
-            if let Some(signal) = abort_signal.take() {
-                // signal was ref()'d when stored; unref releases that ref.
-                // `AbortSignal` is an `opaque_ffi!` ZST handle; `opaque_ref` is
-                // the centralised non-null deref proof.
-                WebCore::AbortSignal::opaque_ref(signal).unref();
-            }
+        &mut terminal_info,
+        |terminal_info: &mut Option<TerminalCreateResult>| {
             // If we created a new terminal but spawn failed, close it. The
             // writer/reader/finalize deref paths release the remaining refs.
             // Downgrade the JSRef so the wrapper is GC-eligible, and mark
@@ -395,8 +386,8 @@ fn spawn_maybe_sync<const IS_SYNC: bool>(
             }
         },
     );
-    // Note: reshaped for borrowck — re-borrow through the guard tuple.
-    let (abort_signal, terminal_info) = &mut *defer_guard;
+    // Note: reshaped for borrowck — re-borrow through the guard.
+    let terminal_info = &mut *defer_guard;
 
     // Owned ZBox for `cwd` held here so the `&[u8]` borrow stays valid until
     // `spawn_process` returns.
@@ -511,15 +502,11 @@ fn spawn_maybe_sync<const IS_SYNC: bool>(
             }
 
             if let Some(signal_val) = args.get_truthy(global_this, "signal")? {
-                if let Some(signal) = WebCore::AbortSignal::from_js(signal_val) {
-                    // `from_js` returns a live FFI handle owned by JS.
-                    // `AbortSignal` is an `opaque_ffi!` ZST handle; `opaque_ref`
-                    // is the centralised non-null deref proof.
-                    let sig = WebCore::AbortSignal::opaque_ref(signal);
-                    if let Some(abort_error) = sig.node_abort_error_if_aborted(global_this) {
+                if let Some(signal) = WebCore::AbortSignal::ref_from_js(signal_val) {
+                    if let Some(abort_error) = signal.node_abort_error_if_aborted(global_this) {
                         return Err(global_this.throw_value(abort_error));
                     }
-                    **abort_signal = Some(sig.ref_());
+                    abort_signal = Some(signal);
                 } else {
                     return Err(global_this.throw_invalid_argument_type_value(
                         b"signal",
@@ -1336,7 +1323,7 @@ fn spawn_maybe_sync<const IS_SYNC: bool>(
         closed: Default::default(),
         this_value: Default::default(),
         weak_file_sink_stdin_ptr: Cell::new(None),
-        abort_signal: Cell::new(None),
+        abort_signal: JsCell::new(None),
         event_loop_timer_refd: Cell::new(false),
         event_loop_timer: JsCell::new(crate::timer::EventLoopTimer::init_paused(
             crate::timer::EventLoopTimerTag::SubprocessTimeout,
@@ -1801,16 +1788,14 @@ fn spawn_maybe_sync<const IS_SYNC: bool>(
     // Adding the abort listener may call the onAbortSignal callback immediately if it was already aborted
     // Therefore, we must do this at the very end.
     if let Some(signal) = abort_signal.take() {
-        // SAFETY: `signal` is a live *mut AbortSignal carrying the +1 ref taken
-        // above; ownership of that ref transfers to `subprocess.abort_signal`.
+        let signal = jsc::abort_signal::PendingActivityRef::new(signal);
         // `add_listener` may synchronously fire `on_abort_signal` (already
-        // aborted), which re-enters via `subprocess_ptr` — write through the
-        // raw pointer so no `&mut Subprocess` is held across the call.
-        unsafe {
-            (*signal).pending_activity_ref();
-            let _ = (*signal).add_listener(subprocess_ptr.cast(), Subprocess::on_abort_signal);
-            (*subprocess_ptr).abort_signal.set(NonNull::new(signal));
-        }
+        // aborted), which re-enters via `subprocess_ptr`, so the store below
+        // goes through the raw pointer rather than a `&mut Subprocess`.
+        signal.add_listener(subprocess_ptr.cast(), Subprocess::on_abort_signal);
+        // SAFETY: `subprocess_ptr` is the live Subprocess allocated above;
+        // `clear_abort_signal` drops the hold.
+        unsafe { (*subprocess_ptr).abort_signal.set(Some(signal)) };
     }
 
     if !IS_SYNC {
@@ -1841,24 +1826,8 @@ fn spawn_maybe_sync<const IS_SYNC: bool>(
         // watchOrReap will handle the already exited case for us.
     }
 
-    match subprocess.process_mut().watch_or_reap() {
-        sys::Result::Ok(_) => {
-            // Once everything is set up, we can add the abort listener
-            // Adding the abort listener may call the onAbortSignal callback immediately if it was already aborted
-            // Therefore, we must do this at the very end.
-            if let Some(signal) = abort_signal.take() {
-                // SAFETY: see the matching block above.
-                unsafe {
-                    (*signal).pending_activity_ref();
-                    let _ =
-                        (*signal).add_listener(subprocess_ptr.cast(), Subprocess::on_abort_signal);
-                    (*subprocess_ptr).abort_signal.set(NonNull::new(signal));
-                }
-            }
-        }
-        sys::Result::Err(_) => {
-            subprocess.process_mut().wait(true);
-        }
+    if subprocess.process_mut().watch_or_reap().is_err() {
+        subprocess.process_mut().wait(true);
     }
 
     if !subprocess.has_exited() {

src/runtime/api/bun/subprocess.rs

@@ -29,7 +29,7 @@ use crate::api::bun_process::{Process, Rusage, Status};
 use crate::ipc as IPC;
 use crate::node::node_cluster_binding;
 use crate::timer::{EventLoopTimer, EventLoopTimerState};
-use crate::webcore::{self, AbortSignal, FileSink};
+use crate::webcore::{self, FileSink};
 #[cfg(windows)]
 use bun_libuv_sys::UvHandle as _;
 
@@ -147,10 +147,8 @@ pub struct Subprocess<'a> {
     /// Weak observer of the stdin `FileSink` — holds no ownership/ref. `onStdinDestroyed`
     /// nulls this before the sink is freed, so it is never dereferenced after the sink dies.
     pub(crate) weak_file_sink_stdin_ptr: Cell<Option<NonNull<FileSink>>>,
-    /// +1 C++-intrusive ref held; released in `clear_abort_signal` via
-    /// `AbortSignal::unref()`. Not `Arc` — `AbortSignal` is an opaque FFI
-    /// handle whose refcount lives on the C++ side.
-    pub(crate) abort_signal: Cell<Option<NonNull<AbortSignal>>>,
+    /// The `signal` spawn option, with our abort listener registered on it.
+    pub(crate) abort_signal: JsCell<Option<jsc::abort_signal::PendingActivityRef>>,
 
     pub(crate) event_loop_timer_refd: Cell<bool>,
     /// Intrusive timer node. `JsCell` so `&self` can hand `*mut EventLoopTimer`
@@ -363,16 +361,10 @@ bun_spawn::link_impl_ProcessExit! {
 }
 
 impl Subprocess<'_> {
-    /// Shared borrow of the attached `AbortSignal`, if any.
-    ///
-    /// `abort_signal` holds a +1 C++-intrusive ref taken in
-    /// `spawn_maybe_sync`; the pointee is therefore live for as long as the
-    /// cell is `Some` (it is `take`n *before* `unref()` in
-    /// [`clear_abort_signal`](Self::clear_abort_signal)) — i.e. the
-    /// owner-outlives-holder `BackRef` invariant holds.
+    /// Owned, so it stays valid if `clear_abort_signal` runs meanwhile.
     #[inline]
-    pub(crate) fn abort_signal_ref(&self) -> Option<bun_ptr::BackRef<AbortSignal>> {
-        self.abort_signal.get().map(bun_ptr::BackRef::from)
+    pub(crate) fn abort_signal_ref(&self) -> Option<jsc::AbortSignalRef> {
+        self.abort_signal.get().as_ref().map(|s| s.signal_ref())
     }
 
     #[bun_jsc::host_fn(method)]
@@ -1279,14 +1271,7 @@ impl Subprocess<'_> {
     }
 
     fn clear_abort_signal(&self) {
-        if let Some(signal) = self.abort_signal.replace(None).map(bun_ptr::BackRef::from) {
-            // `signal` was stored with a +1 C++ intrusive ref (taken in
-            // `spawn_maybe_sync`); it stays live until `unref()` below, so the
-            // `BackRef` invariant (pointee outlives holder) holds for this scope.
-            signal.pending_activity_unref();
-            signal.clean_native_bindings(self.as_ctx_ptr().cast::<c_void>());
-            signal.unref();
-        }
+        drop(self.abort_signal.replace(None));
     }
 
     pub fn finalize(self: Box<Self>) {