fetch: reclaim the tasklet when its last ref drops on the HTTP thread at exit

[robobun]

Fixes a LeakSanitizer SIGABRT at exit in compiled server binaries whose fetches complete right before the process exits. Seen on the debian 13 x64-asan lane in test/bundler/bundler_html_server.test.ts (compile/cli/HTMLServerMultipleRoutes), e.g. build 90301: the binary prints all four expected lines, then aborts at exit with:

==12327==ERROR: LeakSanitizer: detected memory leaks

Indirect leak of 2168 byte(s) in 1 object(s) allocated from:
    ...
    #11 in new<bun_http::async_http::AsyncHTTP>
    #12 in get src/runtime/webcore/fetch/FetchTasklet.rs

7 indirect leaks total, all one cycle: the FetchTasklet, its Box<AsyncHTTP>, the native Response from on_resolve, and the ProxySettings allocations owned by the AsyncHTTP.

Cause

FetchTasklet::callback (HTTP thread) enqueues the final on_progress_update, unlocks the tasklet mutex, and then drops the HTTP-side ref via deref_from_thread. The JS thread is often already parked on that mutex inside on_progress_update, so the unlock hands execution straight over: the JS thread runs the entire final update, drops the JS-side ref, and the HTTP thread's trailing deref then observes the 1→0 transition with the VM still running. In a compiled two-fetch server this handoff ordering happens in roughly 10-15% of runs.

deref_from_thread handled that case by queueing the deinit to the JS event loop as a generic ManagedTask (ConcurrentTask::from_callback, cleanup: None). If the script has already finished, that task never runs:

1. release_queued_tasks_for_shutdown deliberately re-queues ManagedTasks unrun (their owners cancel raw back-pointers from Drop during destructOnExit, so they cannot be freed there), and
2. EventLoop::deinit then frees the queued box without invoking its callback.

The tasklet pointer is discarded with the box. The tasklet ⇄ Box<AsyncHTTP> ⇄ native Response cycle has no other owner visible to LSan (the JS Response wrapper's pointer lives in the JSC heap, which LSan does not scan), so the whole chain is reported as indirect leaks and the process aborts.

Instrumented builds confirm the sequence: every failing run logs the handoff enqueue followed by EventLoop::deinit dropping exactly that task, and no deinit for that tasklet; passing runs show the handoff being dispatched normally.

Fix

Queue the handoff under its own task tag, FetchTaskletDeinit, instead of a generic ManagedTask:

• normal dispatch runs the same deinit the ManagedTask callback ran before, and
• the shutdown release pass (__bun_release_task_at_shutdown, JS thread, before destructOnExit) now reclaims a never-dispatched handoff in the same window the parked HTTP-thread reclaims (dealloc_for_shutdown → shutdown_for_exit drain) already use.

The request-body drain hop queued by on_write_request_data_drain had the same shape (a ManagedTask owning a +1 on the tasklet), with a much narrower trigger (a streaming upload racing a forced exit). It now uses its own tag as well; its shutdown-release arm just drops the queued ref.

Verification

The handoff losing the race against the last event-loop drain is a scheduling coincidence, so the numbers are statistical:

• unfixed: LSan abort in 3/168 and 3/416 compiled-server runs under parallel CPU load; the new test fails 2 of 3 executions (192 child runs each) on an idle 16-core machine.
• fixed: 0 aborts in 640 runs under identical load. One of those runs hit the exact failing interleaving (handoff enqueued after the loop's last drain) and was reclaimed by the shutdown pass, exiting clean. The new test passes 3/3, and test/bundler/bundler_html_server.test.ts passes.

Three tests in test/js/web/fetch/exiting.test.ts, all ASAN-only, all under detect_leaks=1 + BUN_DESTRUCT_VM_ON_EXIT=1:

• a deterministic variant: a test-only fault-injection flag (BUN_INTERNAL_FETCH_DELAY_DEREF_FROM_THREAD) holds the HTTP thread's trailing deref, and a process.on("exit") spin keeps is_shutting_down unset, so the handoff lands in the leak window and is reclaimed through the shutdown release pass on every run (verified via log ordering, 5/5);
• an exit-mid-streaming-upload scenario, the state release_at_shutdown and the drain-hop shutdown arm exist for;
• the original CI shape: the compiled two-fetch HTML server run 192 times in parallel batches. The organic window is a few hundred nanoseconds wide on the HTTP thread, so this one is probabilistic: it reproduced the leak in most, but not all, executions against an unfixed build.

Related: #32707 closes the same unlock window from the other side (holding the mutex through the HTTP thread's deref so it is never the final one) to fix the assert_no_refs panic in the deferred deinit. The two changes compose; this one makes the handoff safe whenever it does happen.

---

no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/fetch/exiting.test.ts

[coderabbitai]

Walkthrough

Changes

Fetch tasklet deinitialization and request-stream resumption now use tagged tasks. Runtime dispatch handles execution and shutdown cleanup. ASAN-only tests cover fetch shutdown races, streaming uploads, and repeated compiled-server exits.

Fetch task lifecycle

Layer / File(s)	Summary
Tagged task dispatch and shutdown cleanup src/event_loop/ConcurrentTask.rs, src/runtime/dispatch.rs	Adds fetch tasklet task tags, dispatch arms, task-count validation, and shutdown cleanup for queued tasks.
Fetch tasklet task scheduling and handlers src/runtime/webcore/fetch/FetchTasklet.rs, src/bun_core/env_var.rs	Replaces callback-based scheduling with tagged tasks. Deinitialization reclaims the tasklet. Request-stream resumption checks abort state, drains the sink when applicable, and releases the tasklet reference. Adds a test-only dereference delay flag.
Fetch exit regression coverage test/js/web/fetch/exiting.test.ts	Adds ASAN-only tests for fetch-thread dereference timing, in-flight streaming uploads, and repeated parallel compiled-server exits.

Possibly related issues

• [CRASH] Worker termination races in-flight fetch(), corrupting the event loop's concurrent task queue (two crash signatures) #33911 — Addresses related races between fetch callbacks and VM or event-loop teardown.

Possibly related PRs

• oven-sh/bun#36571 — Refactors related fetch task scheduling from erased callbacks to tagged task dispatch.
• oven-sh/bun#36983 — Modifies related FetchTasklet shutdown and queued-task cleanup paths.
• oven-sh/bun#36575 — Modifies related FetchTasklet shutdown and cross-thread reclamation paths.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Title check	✅ Passed	The title clearly describes the fetch tasklet reclamation fix that addresses the shutdown leak race.
Description check	✅ Passed	The description explains the problem, cause, fix, and verification results, covering both required template topics.

---

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

[robobun]

^{Updated 8:52 PM PT - Aug 7th, 2026}

✅ @robobun, your commit 0d45fd4566615ea89076b1fe1659319f155133ee passed in Build #90474! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37172

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

bun-37172 --bun

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

test/js/web/fetch/exiting.test.ts (2)

55-63: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Add coverage for the request-body drain handoff.

The fixture sends only GET requests at Line 55 through Line 63. It does not create a request body, but this PR also changes the request-body drain task tag. Add a non-empty POST or streaming request and consume or close it before process exit.

As per coding guidelines, every behavioral change must include an automated regression test in the same change.

🤖 Prompt for 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.

In `@test/js/web/fetch/exiting.test.ts` around lines 55 - 63, Add regression
coverage in the exiting test alongside the existing GET requests by issuing a
non-empty POST or streaming request, then consume or close its response/body
before triggering process exit. Keep the existing home and about assertions, and
ensure the new request exercises the request-body drain handoff affected by the
task-tag change.

Source: Coding guidelines

---

94-108: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Clean up every started child process when the batch fails.

Array.from assigns procs only after all eight callbacks return. A later Bun.spawn failure can therefore orphan earlier children. A rejected pipe read also leaves other children active. Store each Subprocess immediately and kill and await all unfinished children in a finally block.

🤖 Prompt for 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.

In `@test/js/web/fetch/exiting.test.ts` around lines 94 - 108, Update the process
batch in the loop around Bun.spawn and Promise.all so each successfully created
Subprocess is stored immediately, including when a later spawn fails. Add
finally-based cleanup that kills every unfinished process and awaits their
completion, covering both spawn failures and rejected stdout/stderr reads while
preserving normal result collection.

Sources: Coding guidelines, Learnings

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

Outside diff comments:
In `@test/js/web/fetch/exiting.test.ts`:
- Around line 55-63: Add regression coverage in the exiting test alongside the
existing GET requests by issuing a non-empty POST or streaming request, then
consume or close its response/body before triggering process exit. Keep the
existing home and about assertions, and ensure the new request exercises the
request-body drain handoff affected by the task-tag change.
- Around line 94-108: Update the process batch in the loop around Bun.spawn and
Promise.all so each successfully created Subprocess is stored immediately,
including when a later spawn fails. Add finally-based cleanup that kills every
unfinished process and awaits their completion, covering both spawn failures and
rejected stdout/stderr reads while preserving normal result collection.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: aded2bde-7ad4-4562-a93f-223675702920

📥 Commits

Reviewing files that changed from the base of the PR and between 975dec6 and 055d90d.

📒 Files selected for processing (1)

• test/js/web/fetch/exiting.test.ts

[robobun]

Addressed the two review suggestions in f7e6b3f: the fixture now runs a streaming POST upload before the GETs (exercises the request-body drain hop under the leak-checked exit regime, while keeping the GETs last so the exit race shape is unchanged), and the child-process batch is built in a loop with a finally that kills any survivors if a spawn or assertion fails mid-batch.

[claude]

I reviewed this PR and found no bugs; the two nits from earlier passes (the sibling on_write_request_data_drain site and the vacuous buildOut assertion) have both been addressed. Because it changes cross-thread refcount handoff and shutdown-ordering invariants in FetchTasklet, a human look would still be worthwhile.

What was reviewed:

• The two new task tags route to deinit_queued (ref_count already 0 from release()) and deref (drops the drain hop's +1) respectively — matches each enqueue site's ownership.
• resume_request_data_stream signature change: the removed IIFE/Ok(()) was pure ManagedTask-ABI scaffolding; behavior is unchanged.
• ElJsResult alias is dead after the last from_callback site was removed — deletion is correct.
• The test's finally { proc.kill() } now covers mid-batch failures; pipes are drained concurrently.

Extended reasoning...

Overview

The PR replaces two ConcurrentTask::from_callback (ManagedTask) enqueues in FetchTasklet — the last-ref handoff in deref_from_thread and the request-body drain hop in on_write_request_data_drain — with dedicated task tags (FetchTaskletDeinit, FetchTaskletResumeRequestStream). It adds dispatch arms in run_task and shutdown-release arms in __bun_release_task_at_shutdown, bumps the task_tag::COUNT compile-time assert to 114, deletes the now-unused ElJsResult alias, and adds an ASAN-only probabilistic regression test that runs a compiled two-fetch server 192 times under detect_leaks=1.

Security risks

None identified. This is a shutdown-time resource-reclaim fix; no new user-controlled input parsing, no auth/crypto surface. The unsafe blocks are pointer casts under the same tag-identifies-pointee invariant every other arm in dispatch.rs uses.

Level of scrutiny

High. This is cross-thread refcount lifecycle code with unsafe deinit on a raw pointer, and the correctness argument depends on ordering guarantees that live outside the diff: that __bun_release_task_at_shutdown runs on the JS thread after the HTTP daemon has parked and before destructOnExit (so deinit's JSC-handle drops are still valid), and that deref_from_thread only enqueues FetchTaskletDeinit after release() observed the 1→0 transition (so assert_no_refs() inside deinit holds). I verified both against the surrounding code, and the shutdown-release arm for the drain hop correctly uses deref rather than deinit_queued since that enqueue holds a +1, not the last ref. But the PR itself notes this composes with #32707 (which closes the same window from the other side), and the test is probabilistic — a maintainer familiar with the shutdown sequence should confirm the composition and whether the 240s ASAN-only test is acceptable CI cost.

Other factors

Both of my earlier inline findings were addressed in follow-up commits (975dec6, 055d90d, f7e6b3f). The comment-cop bot flags were all resolved by the author with reasonable justifications (the remaining comments document thread/ordering contracts, not workarounds). No human reviewer has weighed in yet.

[claude]

I reviewed this PR and didn't find any bugs on this pass; the three earlier inline findings (the on_write_request_data_drain sibling, the vacuous buildOut assertion, and the ungated fault-injection sleep) have all been addressed. Because this touches cross-thread FetchTasklet refcount lifecycle and the shutdown-release ordering, a human look would still be worthwhile.

What was reviewed:

• The two new task tags' dispatch and shutdown-release arms — each releases exactly the ref its enqueue site takes, on the JS thread, before destructOnExit.
• resume_request_data_stream's refactor is behavior-preserving (aborted → skip on_drain, always deref).
• The ElJsResult alias removal — confirmed no remaining users in the file.
• The fault-injection flag is now #[cfg(debug_assertions)]-gated, matching the BUN_INTERNAL_FAIL_PIPE_READER_START precedent.

Extended reasoning...

Overview

Fixes an LSan-reported leak at exit when a fetch completes right as the process exits: the HTTP thread's last-ref handoff was queued as a generic ManagedTask (cleanup: None), which release_queued_tasks_for_shutdown re-queues unrun and EventLoop::deinit then frees without invoking. The fix gives the handoff (and the sibling request-body drain hop) their own task tags so the shutdown-release pass can reclaim them. Touches ConcurrentTask.rs (two new tags + COUNT bump), dispatch.rs (dispatch arms + shutdown-release arms + the COUNT assert), FetchTasklet.rs (enqueue sites, deinit_queued, refactored resume_request_data_stream, and a debug-only fault-injection delay), env_var.rs (the flag), and adds three ASAN-only tests.

Security risks

None. The change is refcount/cleanup ordering at process exit; no user-facing input parsing, auth, or crypto paths are touched. The new env var is a debug-only fault-injection flag gated behind #[cfg(debug_assertions)].

Level of scrutiny

High. This is cross-thread refcount lifecycle in native code — REVIEW.md's most-blocked category. The correctness argument depends on ordering guarantees between the HTTP thread's trailing deref, the JS thread's is_shutting_down flip, release_queued_tasks_for_shutdown, and destructOnExit. The PR description traces the mechanism carefully and the instrumented verification is convincing, but a maintainer familiar with the shutdown ordering (and with #32707, which closes the same window from the other side) should confirm the two changes compose as claimed.

Other factors

• All three of my earlier inline findings were addressed in follow-up commits (975dec6, 055d90d, 839c57c).
• The comment-cop bot's remaining flags were reasonably declined by the author as lifecycle-contract documentation, matching neighboring arms.
• The third test runs 192 compiled binaries with a 240 s timeout on the ASAN lane; that's a real CI-time cost a maintainer may want to weigh against the deterministic first test's coverage.
• The resume_request_data_stream signature change (drops the ElJsResult<()> return) is safe: the only caller was from_callback, now replaced, and sink.on_drain doesn't propagate a JS error through this path.