fetch: hold the AbortSignal as Option<AbortSignalRef>, drop SignalRef

[robobun]

What

fetch_impl (src/runtime/webcore/fetch.rs) took a +1 on the request's AbortSignal with hand-written ref_() calls and parked the raw pointer in a private SignalRef(Option<NonNull<AbortSignal>>) Drop guard, whose take() disarmed it when the pointer was moved into FetchOptions.signal: Option<*mut AbortSignal>. FetchTasklet::get moved that pointer into FetchTasklet.signal: Option<*mut AbortSignal> (src/runtime/webcore/fetch/FetchTasklet.rs), every reader went through bun_opaque::opaque_deref, and clear_abort_signal released the ref with an explicit unref() after clean_native_bindings and pending_activity_unref.

All three now hold Option<AbortSignalRef>, the ExternalShared<AbortSignal> that Request, Response::BodyAbortListener, node_fs and node_fs_watcher already use for the same C++ object.

// before
struct SignalRef(Option<NonNull<AbortSignal>>);   // private Drop guard in fetch.rs
signal.0 = 'extract_signal: {
    // options object / request-init object:
    if let Some(signal__) = AbortSignal::from_js(signal_) {
        break 'extract_signal NonNull::new(bun_opaque::opaque_deref(signal__).ref_());
    }
    // fetch(request):
    if let Some(signal_) = req.signal.get() {
        break 'extract_signal NonNull::new(signal_.ref_());
    }
    ..
};
pub(crate) signal: Option<*mut AbortSignal>,       // FetchOptions and FetchTasklet
fn abort_signal(&self) -> Option<&AbortSignal> { self.signal.map(|p| bun_opaque::opaque_deref(p)) }
signal.clean_native_bindings(..); signal.pending_activity_unref(); signal.unref();

// after
let signal: Option<AbortSignalRef> = 'extract_signal: {
    if let Some(signal_ref) = AbortSignal::ref_from_js(signal_) {
        break 'extract_signal Some(signal_ref);
    }
    break 'extract_signal req.signal.get().clone();
    ..
};
pub(crate) signal: Option<AbortSignalRef>,
fn abort_signal(&self) -> Option<&AbortSignal> { self.signal.as_deref() }
let Some(signal) = self.signal.take() else { return };
signal.clean_native_bindings(..); signal.pending_activity_unref();   // dropping `signal` is the unref

In fetch.rs the three extraction sites use AbortSignal::ref_from_js (options object and request-init object; the signal is not of type AbortSignal rejection is unchanged) or clone the Request's own AbortSignalRef, the pre-abort check borrows the Option, and the FetchOptions literal moves it. In FetchTasklet.rs the two fields change type, abort_signal() becomes as_deref(), the listener registration in get borrows the field, and clear_abort_signal drops the taken value instead of calling unref() by hand. SignalRef, its take(), and the three opaque_deref conversions are deleted; the two comments describing the raw-pointer protocol go with them. No unsafe blocks are added or removed.

Why

Who owns the +1 on the signal is now stated by the field types: fetch_impl holds it for the duration of argument processing (every one of the early returns after extraction releases it through AbortSignalRef's Drop, which SignalRef re-implemented locally), FetchOptions carries it, and FetchTasklet owns it until clear_abort_signal takes it out; a FetchTasklet or FetchOptions that is dropped while still holding the pointer can no longer leak it. This is zero-cost: Option<ExternalShared<AbortSignal>> is one word through the NonNull niche where Option<*mut AbortSignal> was two, so both structs shrink; the same ref()/unref()/pendingActivity FFI calls are issued at the same program points (clear_data always runs clear_abort_signal before the tasklet box is freed, so the field's drop glue sees None); and the Deref reads replace opaque_deref calls that each carried a null check, while ref_from_js drops the NonNull::new check on the value ref_() returned.

Part of a series of small type-system hardening changes; each PR stands alone.

Verification

cargo check and cargo clippy are clean for the touched crates. Debug build succeeds; bun bd test test/js/web/fetch/fetch.test.ts fetch-abort-stream-body.test.ts fetch-abort-queued.test.ts fetch-leak.test.ts: 354 pass, 1 skip, 42 fail (fetch-abort-stream-body 6 pass/1 skip and fetch-abort-queued 1 pass are fully green; all AbortSignal tests in fetch.test.ts and the abort/leak tests in fetch-leak.test.ts pass).
The 42 failures (41 in fetch.test.ts: sandbox localhost/IPv6 ConnectionRefused or egress-proxy responses, chmod-000 reads succeeding as root, debug-build "utf16 ... (with gc)" and redirect timeouts; 1 in fetch-leak.test.ts: "Sending URLSearchParams > does not leak" 120s debug timeout) are the identical set that fails on main in this environment, so they are pre-existing and unrelated to this change.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 6 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: df85a78a-2c2d-4fac-ac53-96503d83484c

📥 Commits

Reviewing files that changed from the base of the PR and between 1d28cc4 and b864f76.

📒 Files selected for processing (2)

• src/runtime/webcore/fetch.rs
• src/runtime/webcore/fetch/FetchTasklet.rs

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 4:41 PM PT - Aug 11th, 2026}

✅ @robobun, your commit b864f7638cdc9ed1e1054bd5361b0f6eec7885ec passed in Build #92572! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37549

That installs a local version of the PR into your bun-37549 executable, so you can run:

bun-37549 --bun

[claude]

LGTM — mechanical swap from a hand-rolled SignalRef Drop guard + raw *mut AbortSignal fields to the existing AbortSignalRef (ExternalShared<AbortSignal>) already used by Request, Response::BodyAbortListener, and node_fs.

What was reviewed:

• ref_from_js is from_js + ref_() + adopt, and req.signal.get().clone() on JsCell<Option<AbortSignalRef>> bumps via ExternalShared::clone → same +1 taken at the same three extraction sites.
• clear_abort_signal order preserved (clean_native_bindings → pending_activity_unref → drop = unref); .take() leaves None so field drop-glue is a no-op, no double-unref.
• bun_opaque and NonNull imports remain live in both files; no dead code introduced.

Extended reasoning...

Overview

This PR replaces the private SignalRef(Option<NonNull<AbortSignal>>) Drop guard in src/runtime/webcore/fetch.rs and the raw Option<*mut AbortSignal> fields on FetchOptions/FetchTasklet with Option<AbortSignalRef>, the ExternalShared<AbortSignal> type alias that already models this exact C++ intrusive-refcounted object throughout the codebase (Request.signal, Response::BodyAbortListener, node_fs, node_fs_watcher). Three opaque_deref conversions and the local guard type are deleted; the accessor becomes as_deref().

Refcount equivalence

I traced every changed site against the underlying FFI calls. AbortSignal::ref_from_js (src/jsc/AbortSignal.rs:249) is exactly from_js + ref_() + ExternalShared::adopt, matching the old from_js → opaque_deref → ref_() → NonNull::new. req.signal.get() returns &Option<AbortSignalRef> (JsCell::get at src/jsc/JSCell.rs:104), and .clone() routes through ExternalShared::clone → ext_ref → WebCore__AbortSignal__ref, matching the old signal_.ref_(). ExternalShared::Drop calls ext_deref → WebCore__AbortSignal__unref, matching what SignalRef::drop did via BackRef::unref(). In clear_abort_signal, the taken AbortSignalRef is dropped after clean_native_bindings and pending_activity_unref, so the unref() still fires last; .take() leaves None in the field so the struct's own drop glue does nothing. Neither FetchTasklet nor FetchOptions has a manual Drop impl, so the only new behavior is that a tasklet/options dropped while still holding a signal now releases it instead of leaking — strictly an improvement.

Security risks

None. This is an ownership-encoding refactor over an intrusive C++ refcount; no user input parsing, no new FFI surface, no unsafe blocks added or removed.

Level of scrutiny

Medium — it touches the fetch AbortSignal lifetime, but the change is a type substitution onto an established, already-tested wrapper (ExternalShared) with identical ref/unref semantics. The diff is small, net-negative, and every behavioral claim in the PR description checks out against the source. No dead imports were introduced (NonNull is still used by FetchHeadersRef; bun_opaque is still used at FetchTasklet.rs:2607/2619).

Other factors

The PR follows an established pattern the repo already uses for this object, matching the "grep for the in-tree helper" and "one implementation, in the right place" rules. Existing abort/leak test coverage (fetch-abort-stream-body, fetch-abort-queued, fetch-leak) passes per the PR description. One finder-raised nit about a stale comment listing signal among locals declared above was ruled out — the updated comment still accurately describes the set of owning locals whose Drop runs on early return.

[claude]

LGTM — mechanical swap from a hand-rolled SignalRef Drop guard + raw *mut AbortSignal fields to the canonical Option<AbortSignalRef> (ExternalShared<AbortSignal>) already used by Request, BodyAbortListener, and node_fs.

What was reviewed:

• Refcount balance: ref_from_js/.cloned() take the same +1 the old from_js + ref_() did; every early return in fetch_impl after extraction drops the local (was SignalRef::Drop); FetchOptions move and FetchTasklet::get transfer are 1:1.
• clear_abort_signal ordering unchanged: take() → clean_native_bindings → pending_activity_unref → drop (= unref()), matching the old explicit sequence.
• Checked that Request::abort_signal() returns Option<&AbortSignalRef> so .cloned() bumps the intrusive refcount via ExternalShared::Clone.

Extended reasoning...

Overview

This PR replaces the private SignalRef(Option<NonNull<AbortSignal>>) Drop guard in fetch.rs and the two Option<*mut AbortSignal> fields on FetchOptions/FetchTasklet with Option<AbortSignalRef>, the ExternalShared<AbortSignal> type alias that the rest of the codebase already uses for the same C++ object. Three opaque_deref conversions and one hand-written unref() become as_deref()/Drop; the extraction sites use AbortSignal::ref_from_js and req.abort_signal().cloned().

Security risks

None introduced. The change is refcount plumbing for an intrusively-refcounted C++ WebCore::AbortSignal; no parsing, validation, or trust-boundary logic is touched. The TypeError rejection for non-AbortSignal values and the pre-aborted synchronous rejection are byte-identical.

Level of scrutiny

fetch is a hot, memory-safety-sensitive path, so I traced every ref/unref pair explicitly rather than trusting the shape. The +1 acquisition sites (ref_from_js = from_js + ref_() + adopt; .cloned() = ext_ref) and release sites (local drop on early return, clear_abort_signal's take() → drop) map one-to-one onto the old code. FetchTasklet::get still moves fetch_options.signal into the tasklet field, and clear_data still runs clear_abort_signal before the box is freed, so the field's Drop glue sees None on the normal path — and if a FetchOptions/FetchTasklet were ever dropped early, the ref is now released instead of leaked, which is strictly safer.

Other factors

This is exactly the "use the in-tree helper instead of hand-rolling" pattern REVIEW.md asks for, follows the established AbortSignalRef usage in Request.rs/node_fs, deletes ~40 lines of raw-pointer commentary, and adds no unsafe. The PR is part of a stated series of small type-system hardening changes; cargo check/clippy are clean and the abort-signal test files pass on the debug build.

[robobun]

Closing: main already has this change.

• Refcounted types own their teardown: impl Drop instead of destroy hooks, RefPtr instead of raw holders, rc classes #40516 (49ff888) removed the SignalRef guard from fetch.rs. fetch() and FetchOptions now hold the signal as Option<AbortSignalRef>.
• Bun.ModuleGraph: a context per graph for timers and I/O, per-graph CommonJS #42590 (0d3492e) replaced the tasklet's signal field with an AbortHandle. The handle owns the AbortSignalRef and releases it in unfollow(), so no manual unref() remains.

I compared each hunk of this PR with main at 08a2340. Two small differences remain, and neither changes behavior:

• Main writes the Request branch as Some(signal_.ref_()) (fetch.rs:1022), and this PR used .cloned(). Both take one ref as an AbortSignalRef.
• The accessor comment at FetchTasklet.rs:327 still names signal, a field the tasklet no longer has.