fetch: hold FetchTasklet mutex through deref_from_thread so HTTP thread is never the final deref

[robobun]

Crash

Panic: assertion failed: self.raw_count.load(Ordering::SeqCst) == 0   (macOS arm64, bun 1.4.0)
assert_no_refs<FetchTasklet>          src/ptr/ref_count.rs:603
<FetchTasklet>::deinit                src/runtime/webcore/fetch/FetchTasklet.rs:499
<FetchTasklet>::deinit_callback       src/runtime/webcore/fetch/FetchTasklet.rs:418
<ManagedTask>::run                    src/event_loop/ManagedTask.rs:33
bun_runtime::dispatch::run_task       src/runtime/dispatch.rs:264
tick_queue_with_count                 src/runtime/dispatch.rs:615
<EventLoop>::tick                     src/jsc/event_loop.rs:688
<Run>::start                          src/runtime/cli/run_command.rs:1582

deinit_callback is a ManagedTask enqueued by deref_from_thread on the HTTP thread when release() observes the 1→0 transition. By the time the JS thread runs it, the refcount is nonzero, so the assert_no_refs in deinit fires.

Cause

FetchTasklet::callback runs on the HTTP thread and ends with

Self::enqueue_concurrent(vm, ct);   // enqueue on_progress_update
task_ref.mutex.unlock();
if is_done {
    FetchTasklet::deref_from_thread(task);
}

Between mutex.unlock() and deref_from_thread(), the JS thread can:

1. acquire the mutex (it was parked in on_progress_update at self.mutex.lock()),
2. run on_progress_update to completion with is_done = true,
3. reach cleanup which drops the JS-side initial ref (FetchTasklet::deref(this) → N→1).

The HTTP thread then resumes at deref_from_thread → 1→0, and since the VM is not shutting down it enqueues deinit_callback. That task is the only path to deinit that is not synchronous with the 1→0 transition, so anything that touches the refcount (or the allocation) before it runs trips the assert instead of being blocked by a live ref.

The CAS-fail early return has the same unlock → deref_from_thread ordering. The ignore_data early return had an if is_done { deref_from_thread } that is unreachable (it is inside if has_more { .. }).

Fix

Move deref_from_thread before the unlock in both reachable paths. on_progress_update is the only releaser of the initial ref, and it needs the mutex, so while the HTTP thread holds the mutex the initial ref is guaranteed live and the HTTP-side deref is never the 1→0 transition. release() therefore always returns false from callback, and deinit_callback is never scheduled from this path. The only remaining 1→0 callers of deref_from_thread are the shutdown paths (callback's is_shutting_down branch and release_at_shutdown), which route through dealloc_for_shutdown rather than deinit_callback.

A debug_assert!(is_shutting_down()) in deref_from_thread after a 1→0 transition documents the invariant. The dead ignore_data deref is dropped.

After the deref, task is still live (the initial ref is still held), so the subsequent unlock is safe.

Verification

test/js/web/fetch/fetch-tasklet-deref-race.test.ts runs a subprocess fixture with 2000 concurrent fetch/abort cycles against a local server. On the fixed debug+ASAN build: passes in ~4s, 5/5 runs, and the new debug_assert never fires.

Existing fetch.test.ts, fetch-leak.test.ts, fetch-abort-stream-body.test.ts, fetch-abort-queued.test.ts, abort-signal-leak.test.ts on the fixed debug build have identical pass/fail counts to an unfixed baseline build in the same container (the failures are pre-existing debug-timeout and RSS-threshold issues).

Note on fail-before: the race window is the handful of instructions between mutex.unlock() and deref_from_thread(). The JS thread has to acquire the mutex, run all of on_progress_update, and reach its final deref in that gap. A 10× run of the fixture at 5000 iterations against the unfixed release build did not hit the panic once; the same conclusion was reached in #29453 (Zig-era, ~500 attempts). The gate's fail-before check will not be satisfiable without instrumentation in src/. The fix is correct by construction: with the mutex held through deref_from_thread, the HTTP thread categorically cannot observe count == 1.

Related

• fetch: hold tasklet mutex through derefFromThread so HTTP thread never drops last ref #29453 is the Zig-era PR for the same ordering change; it never merged and now conflicts post-Rust-port. This PR is the Rust-port equivalent.
• ai slop #32704 guards the sibling Option::unwrap() on None crash at task_ref.http.as_mut().unwrap() in the same callback, which is the UAF side of this lifecycle race (tasklet already freed when a stale callback runs). That change and this one do not conflict.
• Don't enqueue to a terminated worker's freed event loop from other threads #32071 closes the cross-thread VM-lifetime hole for the worker-termination class of the same UAF.

[coderabbitai]

Warning

Review limit reached

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

More reviews will be available in 5 minutes and 18 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

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 credits.

🚦 How do rate 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 see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 518909a9-bdc8-49b4-b132-61e438a116f6

📥 Commits

Reviewing files that changed from the base of the PR and between 660a330 and 40ca09d.

📒 Files selected for processing (2)

• src/runtime/webcore/fetch/FetchTasklet.rs
• test/js/web/fetch/fetch-tasklet-deref-race.test.ts

Walkthrough

FetchTasklet now asserts the 1→0 dereference path only happens during JavaScript VM shutdown and keeps the HTTP-thread mutex held across selected dereference calls. A new stress fixture and test run concurrent fetch/abort loops against an in-process server and verify clean process completion.

Changes

FetchTasklet deref ordering

Layer / File(s)	Summary
Shutdown invariant src/runtime/webcore/fetch/FetchTasklet.rs	deref_from_thread asserts the 1→0 path only during JavaScript VM shutdown and documents the mutex/refcount invariant.
Callback mutex ordering src/runtime/webcore/fetch/FetchTasklet.rs	callback holds the mutex across selected deref_from_thread calls and unlocks afterward in the early-exit and completion paths.
Race fixture test/js/web/fetch/fetch-tasklet-deref-race-fixture.ts	The fixture runs concurrent fetch and abort loops against an in-process HTTP server, alternates abort scheduling, validates at least one completed fetch, forces garbage collection, and logs ok.
Race test test/js/web/fetch/fetch-tasklet-deref-race.test.ts	The test spawns the fixture process with piped stdout and stderr, waits for exit, and asserts ok output with a zero exit code.

Possibly related PRs

• oven-sh/bun#31325: Modifies src/runtime/webcore/fetch/FetchTasklet.rs fetch progress and shutdown sequencing in the same runtime path.

Suggested reviewers

• Jarred-Sumner
• dylan-conway
• cirospaciari

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main fix: keeping the FetchTasklet mutex held through deref_from_thread on the HTTP thread.
Description check	✅ Passed	The description is detailed and covers the bug, fix, verification, and related context, though it does not use the template headings verbatim.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

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

[robobun]

^{Updated 7:19 PM PT - Jun 25th, 2026}

❌ @robobun, your commit 40ca09d has 4 failures in Build #64771 (All Failures):

• test/js/web/websocket/autobahn.test.ts - code 1 on 🍎 14 aarch64
• test/js/bun/terminal/terminal.test.ts - code 1 on 🍎 14 x64
• test/js/node/test/parallel/test-tls-client-destroy-soon.js - code 1 on 🍎 14 aarch64
• test/napi/napi.test.ts - code 1 on 🪟 2019 x64
• test/napi/napi.test.ts - code 1 on 🪟 2019 x64-baseline
• test/napi/napi.test.ts - code 1 on 🪟 11 aarch64

---

🧪   To try this PR locally:

bunx bun-pr 32707

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

bun-32707 --bun

[github-actions]

Found 1 issue this PR may fix:

1. Doing a large amount of fetch requests results in a memory leak #20912 - Crash in us_internal_ssl_socket_close on the HTTP thread during high-concurrency fetch is consistent with the FetchTasklet refcount race where the HTTP thread's deref becomes the 1-to-0 transition

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #20912

🤖 Generated with Claude Code

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/web/fetch/fetch-tasklet-deref-race-fixture.ts`:
- Around line 25-46: Make the fetch mix deterministic and stop swallowing
unexpected failures in one(): replace the Math.random()-driven abort choice with
a fixed success/abort pattern based on iteration or worker index, and let the
non-abort fetch path surface errors instead of using a blanket catch. Use the
one and worker helpers to locate the fixture logic, and ensure the test only
tolerates the intended abort case while failing loudly if the normal completion
path starts rejecting.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: aeaab5d4-129c-4f8d-bd9a-1e98e7b2d663

📥 Commits

Reviewing files that changed from the base of the PR and between 5621c5d and 95d6550.

📒 Files selected for processing (3)

• src/runtime/webcore/fetch/FetchTasklet.rs
• test/js/web/fetch/fetch-tasklet-deref-race-fixture.ts
• test/js/web/fetch/fetch-tasklet-deref-race.test.ts

[claude]

All my earlier feedback is addressed (including the test timeout in 40ca09d) and I didn't find any remaining issues — but the fix's correctness rests on the non-local invariant that on_progress_update is the sole releaser of the JS-side initial ref and always takes this mutex, plus the post-deref from_raw_ref(task).mutex.unlock() relying on that ref still being live; that's the kind of cross-thread refcount/lifetime reasoning a human owner of this code should sign off on.

Extended reasoning...

Overview

This PR reorders FetchTasklet::deref_from_thread to run before mutex.unlock() at the two reachable non-shutdown exits of FetchTasklet::callback (the HTTP-thread result callback), so the HTTP thread's deref can never be the 1→0 refcount transition. It also drops a dead if is_done { deref } inside the ignore_data && has_more branch, adds a debug_assert!(is_shutting_down()) after a 1→0 in deref_from_thread to document the new invariant, updates three comments to match, and adds a subprocess stress test (2000 fetch/abort iterations at concurrency 64).

The actual code delta is small (~10 lines of reordering in src/runtime/webcore/fetch/FetchTasklet.rs plus comments), and the four 🟡 nits I raised across earlier revisions (two stale comments, one overstated comment, missing test timeout) plus CodeRabbit's deterministic-fixture feedback have all been applied in 95d6550 / 660a330 / 8dd36dd / 40ca09d.

Security risks

None identified. This is internal refcount/mutex lifecycle code with no auth, crypto, parsing, or external-input handling changes. The test fixture binds a server on port 0 (ephemeral) in a subprocess.

Level of scrutiny

High. This is cross-thread refcount + mutex ordering in the core fetch() path — every fetch in Bun goes through FetchTasklet::callback. The fix is argued correct-by-construction rather than empirically: the PR description itself notes the race window is too narrow to reproduce reliably (10× at 5000 iterations against an unfixed build never hit it), so the new test is best-effort and the debug_assert is the real guard. Correctness hinges on two non-local claims that I can't fully verify from the diff alone:

1. on_progress_update is the only path that releases the JS-side initial ref, and it always acquires this mutex first — so while the HTTP thread holds the mutex, refcount ≥ 2.
2. After deref_from_thread(task) drops the HTTP-side ref, dereferencing task again via Self::from_raw_ref(task).mutex.unlock() is safe because the JS-side initial ref is still pinned by (1).

Both look right from the surrounding code and the PR's analysis (which mirrors the unmerged Zig-era #29453), but they depend on the full ~2400-line FetchTasklet lifecycle and its interaction with #32704 / #32071. A human who owns this code should confirm the invariant holds.

Other factors

• No CODEOWNERS entry covers this path.
• Bug-hunting system found nothing.
• All prior review threads are resolved/addressed; the only unresolved inline thread (test timeout) is now fixed in the diff (}, 30_000);).
• CI build #64771 was triggered on the latest commit.

[robobun]

CI status: the remaining red is Windows-only and unrelated to this diff.

Build 64747 and 64771 both failed on:

• test/napi/napi.test.ts on Windows 2019 x64 / x64-baseline (napi_is_arraybuffer output formatting and a gcUntil timing flake)
• test/js/node/net/server.spec.ts on Windows (server.close() ... will not accept new connections, ECONNREFUSED race)
• assorted flaky-tagged Windows retries (hot.test.ts, spawn-stdin-readable-stream.test.ts, bun-install.test.ts EBADF)

None of these touch fetch, FetchTasklet, or the new test. The new fetch-tasklet-deref-race.test.ts passed on every lane. Locally the existing fetch suites (fetch.test.ts, fetch-leak.test.ts, fetch-abort-*) have identical pass/fail counts with and without the change on a debug+ASAN build.

As noted in the PR body, the race is not fail-before provable without src/ instrumentation (the window is mutex.unlock() → deref_from_thread(), a few instructions; a 10× probe at 5000 iterations against the unfixed release build did not fire), so the gate's fail-before check won't be satisfiable. The fix is correct by construction: with the mutex held through deref_from_thread, on_progress_update cannot have released the JS-side initial ref, so the HTTP thread's deref is never the 1→0 transition and deinit_callback is never scheduled from callback.

Ready for review.

[robobun]

Sentry crash cross-reference for the "Related" section:

Covered by this PR

• BUN-3KDD / BUN-3PM8 (assertion failed: self.raw_count.load(Ordering::SeqCst) == 0 in FetchTasklet::deinit via assert_no_refs, src/ptr/ref_count.rs:603): this is the crash the PR body describes. With deref_from_thread moved before unlock, the HTTP-side deref is never 1→0 outside shutdown, so deinit_callback is never scheduled from callback and the deferred assert_no_refs can't observe a nonzero count.

Not covered by this PR

• BUN-3KQ1 (bus error in mi_free from drop_in_place<QueryStringMap> inside AsyncHTTP::sync_progress_from, 28 events, all macOS aarch64): the crash site is at the start of FetchTasklet::callback, right after mutex.lock():

  us_internal_ssl_on_data → ssl_trigger_handshake → Handler::on_handshake
    → close_and_fail → fail → dispatch_result_and_reset
    → on_async_http_callback_raw → FetchTasklet::callback
    → task_ref.http.as_mut().unwrap().sync_progress_from(...)
    → self.client.url = src.client.url.clone()   // drops old self.client.url
    → drop_in_place<Option<QueryStringMap>> → ... → mi_free  (bus error)
  

  URL.search_params is None on every fetch URL (URL::parse never sets it, and no writer exists in src/http/ or src/runtime/webcore/fetch/), so drop_in_place<QueryStringMap> executing at all means the URL struct bytes at task_ref.http.unwrap().client.url are garbage, i.e. the FetchTasklet allocation (or its http: Box<AsyncHTTP>) has already been freed. That requires both refs to have been dropped before this callback was entered, which in turn requires a prior final (has_more=false) dispatch on the same HTTPClient to have already run on_async_http_callback_raw's dealloc. This PR's reorder is at the end of callback and adds no guard at entry, so it does not prevent a second dispatch from reaching sync_progress_from through freed memory.

  Making deinit take the tasklet mutex would not help either: by the time a second dispatch runs, the allocation the mutex lives in is gone, so mutex.lock() is already UB.

  The second-dispatch mechanism is at the src/http/ layer (the HTTPClient embedded in the freed ThreadlocalAsyncHTTP receives another usockets callback). Traced every fail() / dispatch_result_and_reset caller: close_and_fail and the h1 success path both clear/retag the socket ext before the final dispatch; the one exception is HTTPClient::on_timeout (src/http/lib.rs:1931), which calls fail() (dealloc) then terminate_socket, so the socket ext briefly points at a freed client. Also, dispatch_result_and_reset calls state.reset() which returns stage to Pending, so the stage != Done && stage != Fail guard in fail() no longer blocks a re-entry. That's the combination ai slop #32704 went after; it was closed because the test couldn't reproduce on main (Linux).

  I ran three repro variants on a debug+ASAN build (TLS handshake-alert + abort, checkServerIdentity park + abort at varied timings, idle-timeout + abort; ~9k iterations total) and none fired; consistent with the Sentry telemetry being macOS-only and with ai slop #32704's own analysis that every synchronous re-entry path is guarded on Linux (ssl_gone() checks after each dispatch in us_internal_ssl_on_data, us_poll_stop nullifies pending ready_polls entries on close).

  Linking here so the next person looking at BUN-3KQ1 doesn't re-derive the above; it needs a separate fix at the HTTP layer with a macOS repro.

[robobun]

Status check against current main (bdb7382), leaving this open.

test/js/web/fetch/fetch-tasklet-deref-race.test.ts passes on main (5 of 5 runs against a debug build), but as the PR description says, this test is best-effort and did not fail on the unfixed build either, so it passing on main says nothing about whether the ordering change landed.

It has not landed: FetchTasklet::callback in src/runtime/webcore/fetch/FetchTasklet.rs still unlocks the tasklet mutex before calling deref_from_thread on all three exit paths, deref_from_thread still defers deinit to the JS thread on a 1 to 0 transition (now via the FetchTaskletDeinitHop task introduced in #37075), and deinit still asserts a zero refcount. The branch needs a rebase onto the post #37075 code before it can be reviewed.