threading: finish a WaitGroup without holding a reference into it past the release

[robobun]

Problem

• WaitGroup::wait() returning is what lets the owner free the group, so once the last finish() has published the count and released the mutex, the finishing thread must not touch the group at all. threading: don't touch WaitGroup after publishing count==0 in finish() #34458 moved the last real access in front of that release. What is left is that finish(&self), Mutex::unlock(&self), the per-OS unlock impls, and the contended-path Futex::wake(&self.state) all still hold references into the group through and after the release, and a reference argument asserts the memory for the whole call.
• Miri rejects the waiter's free for exactly that reason. bun run rust:miri -p bun_threading (the crate is not in the Miri set yet) fails the crate's own test, wait_group::tests::wait_returning_means_finish_is_done_with_self, under Tree Borrows with
  Undefined Behavior: deallocation through <tag> at alloc[0xc] is forbidden ... transitioned due to a protector release --> src/threading/Mutex.rs:193 (end of DebugImpl::unlock; offset 0xc is padding inside the mutex, so this hits even though every field is an atomic), and under the default Stacked Borrows with ... would remove [SharedReadOnly ...] which is strongly protected. Every seed I tried (12) fails within 2..200 iterations.
• It is not only padding: when the waiter re-locked while the finisher still held the mutex, FutexImpl::unlock (src/threading/Mutex.rs:397 on main) forms &self.state for the wake after the releasing swap, i.e. possibly after the waiter has freed the group. In an instrumented Miri run that happened in 6 of 1500 iterations. On a real kernel the wake is harmless (a private FUTEX_WAKE only uses the address as a key), but the reference is still formed on freed memory.
• In-tree callers with that lifetime: LinkerContext's two source-map groups. generate_chunks_in_parallel frees the task slab on the line after wait() (src/bundler/linker_context/generateChunksInParallel.rs:78), and the Bun.build error path waits on both groups and tears down the whole BundleV2 right after (src/runtime/api/js_bundle_completion_task.rs:1267). That is the path threading: don't touch WaitGroup after publishing count==0 in finish() #34458's ASAN report came from. ThreadPool's group is joined before the pool drops and the Windows install queue's group is a static, so those two are fine with &self.

Fix

• WaitGroup::finish_raw(this: *const Self) does the work through raw pointers, fast path included (a fast-path CAS can be the second-to-last decrement, with another finisher letting the waiter free the group while this frame is still live). Its last access to the group is the store that releases the mutex.
• Mutex::unlock_raw(this: *const Self) and raw-pointer unlock_raw impls for the futex, Darwin and Windows backends, so no frame between finish_raw and the releasing store holds a reference. unlock(&self) delegates to it; Bun__unlock calls the impl directly; the os_unfair_lock_unlock extern takes the address (the Windows one already did).
• Futex::wake_raw(*const AtomicU32) for the unlock tail; wake(&AtomicU32) delegates to it, so the other callers are unchanged. It is a safe fn because every backend's wake side only keys on the address and never reads the word (Linux get_futex_key for private futexes, __ulock_wake, RtlWakeAddress*, _umtx_op private wake, memory.atomic.notify); the worst a freed or reused address can produce is a spurious wakeup, which every wait loop already tolerates.
• finish(&self) stays for groups kept alive by something other than wait() and delegates to finish_raw; its doc says when it is and is not allowed. The two LinkerContext finishers use finish_raw through ParentRef::as_const_ptr, so the finish is the last statement in the task that touches the context.
• Why this is the right shape: the memory-level ordering from threading: don't touch WaitGroup after publishing count==0 in finish() #34458 is unchanged; this only changes which pointers are live across the release, which is the thing both aliasing models (and LLVM's dereferenceable on reference arguments) reason about. It is the same shape as UnboundedQueue::push_raw in event_loop: post to a MiniEventLoop through raw pointers and publish last #37883 and the tree's this: *mut Self convention for functions that end in a free (test/internal/source-lints/self-receiver-reclaim.test.ts).
• Linux Futex wake no longer panics on EFAULT (the FreeBSD backend already tolerated it). A real kernel only returns it for an address outside user space, which no caller can produce (every caller just did an atomic op on the same word), while Miri returns it for a word that has since been freed, which is now a documented-legal input; with the panic kept, the fixed test fails under Miri about once per 250 iterations (the 6/1500 above). The futex_3arg SAFETY comment in bun_sys is corrected to match (a WAKE only uses uaddr as a key).
• Verified with:
  • The crate's own test (wait_group::tests::wait_returning_means_finish_raw_is_done_with_the_group), which now finishes through finish_raw. bun_threading is added to MIRI_CRATES and src/threading/** to the Miri workflow's paths, so the existing cargo miri test CI job is the automated check: it fails on main's src/threading (the diagnostic above) and passes here. Under cfg(miri) the test runs 500 iterations (about 15s; the unfixed shape fails within 200 on every seed tried), natively still 10,000. Passes on 12 seeds plus a 10,000-iteration run on the default seed, under both Tree Borrows and Stacked Borrows; the full bun run rust:miri set passes (bun_threading takes 15s, bun_paths takes 75s for comparison).
  • cargo check -p bun_threading --tests on linux-gnu, linux-musl, android, both darwin and both windows-msvc triples and freebsd, plus --release (the ReleaseImpl-direct path) on linux, darwin and windows; cargo check --workspace on the host; cargo clippy --no-deps on bun_threading and bun_bundler; the Windows-target clippy finding set is identical to main's.
  • bun bd test test/bundler/bun-build-api.test.ts (52 pass) and a 20-round loop of a 200-module sourcemap: "external" build plus a failing sourcemap build under the ASAN debug build, which exercises both LinkerContext call sites and the error-path teardown.
• Overlap with open PRs: event_loop: post to a MiniEventLoop through raw pointers and publish last #37883 adds bun_threading to the Miri set with this test #[ignore]d under Miri; this PR makes it pass, so whichever lands second drops the ignore (the MIRI_CRATES and workflow lines are identical). bundler: scope thread-pool join barriers to the batch, not the whole pool #36481 introduces stack-scoped groups whose BatchDone drop calls (*ptr).finish() relying on this property; with this change that should be WaitGroup::finish_raw(ptr), noted there.
• Deliberately not in this PR: the same shape exists with other primitives in two places outside bun_threading, SingleHTTPChannel::write_item(&self) in src/http/AsyncHTTP.rs (send_sync frees the channel right after read_item returns) and process_http_callback(&mut self) in src/runtime/webcore/s3/download_stream.rs (on_response frees the task once that unlock lands). Both need a raw-unlock path for the guard types rather than WaitGroup changes, so they are tracked separately. ResetEvent::set(&self) has the same tail but its only user is a process-lifetime BundleThread, and the Condvar notifiers I looked at (VmHandle, HTTPThread shutdown) hold an Arc or a static.

Background

• Protectors: in Rust's aliasing models (Stacked Borrows, and Tree Borrows, which rust:miri uses) a reference passed as a function argument is "protected" for the duration of the call: the callee may assume the memory stays valid and unchanged by others until it returns, and freeing memory that a protected reference covers is undefined behavior even if the callee never touches it again. This is what lets rustc mark reference arguments dereferenceable for LLVM. A raw pointer argument carries no such assertion, which is why a function whose job ends by letting another thread free the object takes *const Self.
• Padding: a &Mutex covers the struct's padding bytes too, and padding is not interior-mutable, so a struct made only of atomics still gets the strict treatment for those bytes. That is what the 0xc in the diagnostic is.
• Futex wake is address-keyed: wait sleeps on an address after comparing the word; wake looks the address up in the kernel's (or runtime's) waiter table without reading user memory. This is the property every futex-based mutex relies on so that the thread that acquires the lock next may free it; it is also why a wake on a freed address is harmless and why wake_raw needs no unsafe.
• WaitGroup::finish publishes the final decrement under the group's mutex (since threading: don't touch WaitGroup after publishing count==0 in finish() #34458), so wait(), which checks the count under the same mutex, cannot return before the finisher's unlock; the releasing store inside that unlock is therefore the exact point after which the group may be gone.

Miri diagnostic on main, and the iteration counts

$ bun run rust:miri -p bun_threading
test wait_group::tests::wait_returning_means_finish_is_done_with_self ... error: Undefined Behavior: deallocation through <620185> at alloc217837[0xc] is forbidden
    --> library/alloc/src/boxed.rs:2002:17
     = help: the accessed tag <620185> has state Reserved (conflicted) which forbids this deallocation (acting as a child write access)
help: the accessed tag <620185> was created here, in the initial state Reserved
    --> src/threading/WaitGroup.rs:115:17      drop(Box::from_raw(wg));
help: the accessed tag <620185> later transitioned to Reserved (conflicted) due to a protector release (acting as a foreign read access) on every location previously accessed by this tag
    --> src/threading/Mutex.rs:193:6           (end of DebugImpl::unlock)

Depending on the interleaving the same test also fails as
deallocation ... is forbidden ... the accessed tag is foreign to the protected tag <..> (currently Frozen) ... protected tag was created here: Mutex.rs:66 pub fn unlock(&self),
and under Stacked Borrows (MIRIFLAGS="") as
not granting access to tag <..> because that would remove [SharedReadOnly for <..>] which is strongly protected.

Iteration at which the unfixed shape fails under Miri, by -Zmiri-seed:
0: 2, 1: 199, 2: 75, 3: 16, 4: 75, 5: 10, 6: 10, 7: 113, 8: 113, 9: 71, 10: 36, 11: 51.

Instrumented run of the fixed code, 1500 iterations, default seed: 2051 contended unlocks, 6 of whose wakes ran after the waiter had already freed the group (Miri returned EFAULT), 0 aliasing reports.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 30 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: 830a0b59-f862-4130-a55c-c8ad88f00896

📥 Commits

Reviewing files that changed from the base of the PR and between e697804 and cd200b0.

📒 Files selected for processing (7)

• .github/workflows/miri.yml
• scripts/rust-miri.ts
• src/bundler/LinkerContext.rs
• src/sys/lib.rs
• src/threading/Futex.rs
• src/threading/Mutex.rs
• src/threading/WaitGroup.rs

---

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

[robobun]

^{Updated 1:05 AM PT - Aug 14th, 2026}

@robobun, your commit cd200b0 is building: #95391

[robobun]

Status: reproduced on main with bun run rust:miri -p bun_threading (fails within 2..200 iterations on each of 12 seeds, diagnostic in the description); with this branch the same command passes on those seeds and on a 10,000-iteration run, under Tree Borrows and Stacked Borrows. The check is the crate's wait_group test under the existing Miri CI job, which this PR enables for bun_threading (the test/ wrapper was removed per review). Follow-up commits only shorten comments and drop the unreachable EFAULT panic in the Linux wake. Waiting on CI.

[claude]

I reviewed this PR and the automated bug-hunting pass found no issues. The reasoning about Tree Borrows protectors, the raw-pointer chain from finish_raw → Mutex::unlock_raw → Futex::wake_raw, and the per-backend wake signature changes all look correct to me. Because it rewrites core synchronization primitives across five OS backends with subtle aliasing-model reasoning, and coordinates with two other open PRs (#37883, #36481), a human look is still warranted.

What was reviewed:

• Traced the raw-pointer chain: no &T is formed past the releasing store in any backend; (*this).mutex.lock() and .cond.signal() form references only before the count reaches 0.
• Checked ctx.mark_pending_task_done() in the defer! blocks — it runs before finish_raw, so its &LinkerContext borrow is dropped while the group is still live; ctx itself is a ParentRef (Copy wrapper), not a reference.
• Verified .cast_mut() was added for the FreeBSD _umtx_op and wasm memory_atomic_notify calls whose FFI signatures take *mut; ParentRef::as_const_ptr exists.
• Confirmed the remaining WaitGroup::finish(&self) callers (ThreadPool, Windows install queue) are the ones the PR names as kept-alive-by-other-means.

Extended reasoning...

Overview

This PR fixes a Rust aliasing-model violation in WaitGroup::finish: once the last finisher publishes count=0 and releases the mutex, wait() on another thread may return and the owner may free the group, but the finishing thread's call frames (finish(&self), Mutex::unlock(&self), per-OS unlock, contended-path Futex::wake(&self.state)) still hold &T references into the group. Under Stacked/Tree Borrows a reference argument is protected for the whole call, so freeing memory it covers is UB even without a subsequent access — Miri rejects the crate's own test on main. The fix introduces finish_raw(*const Self), Mutex::unlock_raw(*const Self) (with per-backend impls for Futex/Darwin/Windows), and Futex::wake_raw(*const AtomicU32), threading raw pointers from the finish call down to the releasing store so no reference outlives it. The two LinkerContext source-map task finishers are switched to finish_raw; bun_threading is added to the Miri crate set and workflow paths; a new test/internal/threading-miri.test.ts runs cargo miri test -p bun_threading.

Security risks

None. This is internal synchronization plumbing with no user-facing input surface. The change does not weaken any check — the Linux EFAULT panic is preserved for real builds and only relaxed under cfg(miri), where Miri models a freed-address wake differently from a real kernel.

Level of scrutiny

High. This is the repo's most-blocked review category (native memory safety): it rewrites the unlock path of the process-wide Mutex and Futex primitives across five OS backends (Linux, Darwin, Windows, FreeBSD, wasm), changes an extern "C" signature (os_unfair_lock_unlock), and rests on subtle reasoning about Tree Borrows protectors, padding bytes, and futex-wake address-keying semantics. Per the approval guidelines, changes to critical code paths with cross-platform unsafe Rust should get human sign-off even when the automated pass is clean.

Other factors

• The PR description is unusually thorough (mechanism, Miri diagnostics per seed, cross-target cargo check on 8 triples including --release, ASAN loop of the affected bundler path), and the shape matches the tree's existing *const Self convention for functions that end by letting another thread free the object.
• I verified the intermediate reference-forming calls inside finish_raw ((*this).mutex.lock(), (*this).cond.signal()) all happen strictly before the releasing store, so their short-lived &T borrows are sound; and that the LinkerContext defer! blocks call mark_pending_task_done() before finish_raw, so its &LinkerContext is dropped while the group is still live. ctx is a ParentRef (NonNull wrapper), not a reference, so holding it as a local past the finish creates no protector.
• The remaining .finish() (&self) callers — ThreadPool::wait_group and the Windows install NewTaskQueue — are the ones the PR documents as kept alive by join/static, matching the new doc comment on finish.
• Two open PRs (#37883, #36481) overlap with this change per the description; whichever lands second needs a small adjustment. That coordination is another reason for a maintainer to be in the loop.

[Jarred-Sumner]

Remove the typescript test. miri is already run in CI.

[robobun]

Removed in c2f93e1. The check is the crate's own wait_group test under the existing cargo miri test job, which this PR turns on for bun_threading (MIRI_CRATES plus the workflow's src/threading/** path). That job passed on the first push and is rerunning for the follow-ups, which only shorten comments and drop the unreachable EFAULT panic; the same command fails on main's src/threading.