Skip to content

Move WTFTimer out of the shared timer heap to fix a cross-thread race #33131

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Jarred-Sumner merged 2 commits into from
Jul 16, 2026
Merged

Move WTFTimer out of the shared timer heap to fix a cross-thread race #33131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
96345d1
timers: give WTFTimer its own heap so the rest needs no lock
robobun Jul 10, 2026
f0fa434
timers: drop the leftover block around cancel_all_timeout_objects' DFS
robobun Jul 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/runtime/hw_exports.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,8 @@ pub(crate) mod sql_hooks {
unsafe fn timer_insert(heap: *mut c_void, timer: *mut EventLoopTimer) {
// SAFETY: `heap` is `&runtime_state().timer` (live for the VM); `timer`
// is a live intrusive heap node owned by the caller. Route through
// `All::insert` (NOT the raw `.timers` field) so the lock is taken and
// `(*timer).state` / `in_heap` bookkeeping is updated.
// `All::insert` (NOT the raw `.timers` field) so the fake-timers
// routing and the `(*timer).state` / `in_heap` bookkeeping happen.
unsafe { (*heap.cast::<crate::timer::All>()).insert(timer) };
}
unsafe fn timer_remove(heap: *mut c_void, timer: *mut EventLoopTimer) {
Expand Down
12 changes: 6 additions & 6 deletions src/runtime/jsc_hooks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,10 +197,11 @@ pub(crate) fn global_dns_data() -> &'static core::cell::OnceCell<Box<crate::dns_
}

/// Recover the [`RuntimeState`] owned by a specific `vm` (not the calling
/// thread's). `WTFTimer` and the `timer_insert`/`timer_remove` hooks may be
/// invoked off the VM's JS thread (the `All.lock` mutex exists for exactly
/// that), so they must reach the heap through `vm.runtime_state` rather than
/// the thread-local cache.
/// thread's). `WTFTimer` may be entered off the VM's JS thread (the locked
/// `All.wtf_timers` heap exists for exactly that), and the
/// `timer_insert`/`timer_remove` hooks take `vm` from a tier that cannot see
/// `RuntimeState`; both must reach the heap through `vm.runtime_state`
/// rather than the thread-local cache.
///
/// # Safety
/// `vm` must point at a live `VirtualMachine` whose `runtime_state` was set by
Expand Down Expand Up @@ -1211,8 +1212,7 @@ unsafe fn timer_insert(
let state = unsafe { runtime_state_of(vm) };
debug_assert!(!state.is_null(), "timer_insert before init_runtime_state");
// SAFETY: this leaf hook runs no JS, so a short-lived `&mut RuntimeState`
// does not alias anything. `Timer::All::insert` takes its own lock and
// re-derefs `t` per-field.
// does not alias anything. `Timer::All::insert` re-derefs `t` per-field.
unsafe { &mut (*state).timer }.insert(t);
}

Expand Down
183 changes: 34 additions & 149 deletions src/runtime/test_runner/timers/FakeTimers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use bun_core::Environment;
use bun_core::Timespec;
use bun_jsc::{CallFrame, JSFunction, JSGlobalObject, JSHostFn, JSValue, JsResult};
use crate::timer::{
self, ElTimespec, EventLoopTimer, EventLoopTimerState, EventLoopTimerTag, InHeap,
ElTimespec, EventLoopTimer, EventLoopTimerState, EventLoopTimerTag, InHeap,
TimerObjectInternals, TimeoutObject, TimerHeap,
};

Expand Down Expand Up @@ -109,91 +109,44 @@ pub(crate) extern "C" fn Bun__FakeTimers__setSystemTime(ms: f64) {

use crate::jsc_hooks::timer_all;

/// RAII `lock()`/`unlock()` for the per-thread `timer::All.lock`. Centralises
/// the single raw-pointer deref so call sites read `let _g = timers_lock_guard();`
/// with no `unsafe`. The returned [`bun_threading::MutexGuard`] holds the mutex
/// by raw pointer (no borrow), so it does not pin a `&timer::All` across the
/// re-entrant heap accesses documented on `execute_*` below.
#[inline]
fn timers_lock_guard() -> bun_threading::MutexGuard {
// SAFETY: `timer_all()` returns the boxed per-thread `RuntimeState.timer`,
// never null while a VM is installed (asserted above). `lock` is accessed
// via shared `&Mutex` only (interior mutability), so this forms no aliased
// `&mut` with the surrounding `fake_timers` writes.
unsafe { &(*timer_all()).lock }.lock_guard()
}

#[inline]
fn from_el_timespec(t: &ElTimespec) -> Timespec {
Timespec { sec: t.sec, nsec: t.nsec }
}

impl FakeTimers {
fn assert_locked(&self) {
if !Environment::CI_ASSERT {
return;
}
// SAFETY: self points to the `fake_timers` field of `timer::All` (always embedded there)
let owner: &timer::All = unsafe {
&*(bun_core::from_field_ptr!(timer::All, fake_timers, std::ptr::from_ref::<Self>(self)))
};
debug_assert!(!owner.lock.try_lock());
}

pub fn is_active(&self) -> bool {
self.assert_locked();
// validity re-checked at fn exit
let r = self.active;
self.assert_locked();
r
self.active
}

fn activate(&mut self, js_now: f64, global: &JSGlobalObject) {
self.assert_locked();

self.active = true;
CURRENT_TIME.set(global, &Timespec::EPOCH, Some(js_now));

self.assert_locked();
}

fn deactivate(
&mut self,
global: &JSGlobalObject,
) -> Vec<core::ptr::NonNull<TimerObjectInternals>> {
self.assert_locked();

let pinned = self.clear();
CURRENT_TIME.clear(global);
self.active = false;

self.assert_locked();
pinned
}

/// Drain the fake-timer heap. Returns every `TimeoutObject` that was
/// linked so the caller can release the heap's `+1` ref and the `Strong`
/// JS pin via [`TimerObjectInternals::release_heap_pin`] *after* dropping
/// `&mut self` and `All.lock` — that path reaches `&mut All`, which would
/// alias `&mut self.fake_timers` here (same hazard `execute_next` notes).
///
/// Marking `state = CANCELLED` alone strands the `Box<TimeoutObject>`: its
/// refcount sticks at 2 (wrapper +1 from `init_with`, heap +1 from
/// `reschedule`) and `internals.this_value` still GC-roots the wrapper, so
/// neither side ever frees.
#[must_use]
fn clear(&mut self) -> Vec<core::ptr::NonNull<TimerObjectInternals>> {
self.assert_locked();

let mut pinned = Vec::new();
while let Some(timer) = self.timers.delete_min() {
// SAFETY: `delete_min` returns a live `*mut EventLoopTimer` just
// unlinked; for `TimeoutObject` the tag invariant means it IS the
// `event_loop_timer` field of a live `Box<TimeoutObject>` whose
// refcount is ≥ 1 until the caller's release pass.
// SAFETY: `delete_min` returned a live node; the `TimeoutObject`
// it belongs to stays live until the caller's release pass.
unsafe {
(*timer).state = EventLoopTimerState::CANCELLED;
(*timer).in_heap = InHeap::None;
(*timer).state = EventLoopTimerState::CANCELLED;
if (*timer).tag == EventLoopTimerTag::TimeoutObject {
let parent = TimeoutObject::from_timer_ptr(timer);
pinned.push(core::ptr::NonNull::new_unchecked(
Expand All @@ -203,34 +156,15 @@ impl FakeTimers {
}
}

self.assert_locked();
pinned
}

// noalias re-entrancy: `execute_*` / `fire` do NOT take
// `&mut self`. `EventLoopTimer::fire` dispatches into JS; a `setInterval`
// callback's reschedule (`timer::All::update` → `insert_lock_held` →
// `(*timer_all()).fake_timers.timers.insert`) writes back into *this
// same* `FakeTimers::timers` heap through a fresh raw pointer. With a
// live `&mut self` LLVM's `noalias` lets it cache `self.timers.root`
// across the (inlined) `fire` body — `peek()` on the next loop iteration
// then misses the re-inserted interval, so `advanceTimersByTime` /
// `runOnlyPendingTimers` fire each interval at most once per call. Same
// bug class as `TimerObjectInternals::fire` (see dc37f2018b34). Access
// the heap via the raw `timer_all()` pointer instead so every iteration
// reloads from memory.
fn execute_next(global: &JSGlobalObject) -> bool {
let timers = timer_all();

let next = {
let _g = timers_lock_guard();
// SAFETY: `timers` is the boxed per-thread `RuntimeState.timer`;
// single-threaded JS heap so no concurrent `&mut` to `.fake_timers`.
let n = unsafe { (*timers).fake_timers.timers.delete_min() };
match n {
Some(n) => n,
None => return false,
}
// SAFETY: `timer_all()` is the live per-thread `All`; the borrow ends
// at this statement, before `fire` re-enters `All::insert`.
let next = match unsafe { (*timer_all()).fake_timers.timers.delete_min() } {
Some(n) => n,
None => return false,
};

Self::fire(global, next);
Expand All @@ -255,26 +189,22 @@ impl FakeTimers {
}

fn execute_until(global: &JSGlobalObject, until: Timespec) {
let timers = timer_all();

let all = timer_all();
'outer: loop {
let next = 'blk: {
let _g = timers_lock_guard();

// SAFETY: `timers` is the boxed per-thread `RuntimeState.timer`;
// single-threaded JS heap. Re-derive each iteration so the
// re-entrant `insert` from setInterval rescheduling is observed.
let Some(peek) = (unsafe { (*timers).fake_timers.timers.peek() }) else {
// SAFETY: `all` is the live per-thread `All`; each borrow
// lasts one statement and none spans `fire`.
let Some(peek) = (unsafe { (*all).fake_timers.timers.peek() }) else {
break 'outer;
};
// SAFETY: `peek` is the heap root; live while locked.
// SAFETY: `peek` is the heap root; live while linked.
if from_el_timespec(unsafe { &(*peek).next }).greater(&until) {
break 'outer;
}
// bun.assert always evaluates its arg; debug_assert! does NOT in release.
// Hoist the side-effecting delete_min() out so the timer is removed in all builds.
// SAFETY: as above.
let min = unsafe { (*timers).fake_timers.timers.delete_min() }.expect("unreachable");
let min = unsafe { (*all).fake_timers.timers.delete_min() }.expect("unreachable");
debug_assert!(core::ptr::eq(min, peek));
break 'blk min;
};
Expand All @@ -283,20 +213,11 @@ impl FakeTimers {
}

fn execute_only_pending_timers(global: &JSGlobalObject) {
let timers = timer_all();

let until = {
let _g = timers_lock_guard();
// SAFETY: `timers` is the boxed per-thread `RuntimeState.timer`.
let target = unsafe { (*timers).fake_timers.timers.find_max() };
drop(_g);
match target {
Some(t) => {
// SAFETY: `t` was reachable in the heap under the lock.
from_el_timespec(unsafe { &(*t).next })
}
None => return,
}
// SAFETY: `timer_all()` is the live per-thread `All`.
let until = match unsafe { (*timer_all()).fake_timers.timers.find_max() } {
// SAFETY: `t` is reachable in the heap and live while linked.
Some(t) => from_el_timespec(unsafe { &(*t).next }),
None => return,
};
Self::execute_until(global, until);
}
Expand All @@ -311,17 +232,9 @@ impl FakeTimers {
// ===

fn error_unless_fake_timers(global: &JSGlobalObject) -> JsResult<()> {
let timers = timer_all();
// SAFETY: per-thread `timer::All`.
let this = unsafe { &(*timers).fake_timers };

{
let _g = timers_lock_guard();
let active = this.is_active();
drop(_g);
if active {
return Ok(());
}
// SAFETY: per-thread `timer::All`, live for the VM lifetime.
if unsafe { (*timer_all()).fake_timers.is_active() } {
return Ok(());
}
Err(global.throw(format_args!(
"Fake timers are not active. Call useFakeTimers() first."
Expand Down Expand Up @@ -353,10 +266,6 @@ fn set_fake_timer_marker(global: &JSGlobalObject, enabled: bool) {

#[bun_jsc::host_fn]
fn use_fake_timers(global: &JSGlobalObject, frame: &CallFrame) -> JsResult<JSValue> {
let timers = timer_all();
// SAFETY: per-thread `timer::All`.
let this = unsafe { &mut (*timers).fake_timers };

// SAFETY: FFI call into C++ JSMock
let mut js_now = JSMock__getCurrentUnixTimeMs();

Expand All @@ -382,10 +291,8 @@ fn use_fake_timers(global: &JSGlobalObject, frame: &CallFrame) -> JsResult<JSVal
}
}

{
let _g = timers_lock_guard();
this.activate(js_now, global);
}
// SAFETY: per-thread `timer::All`; `activate` does not re-enter `All`.
unsafe { (*timer_all()).fake_timers.activate(js_now, global) };

// Set setTimeout.clock = true to signal that fake timers are enabled.
// This is used by testing-library/react to detect if jest.advanceTimersByTime should be called.
Expand All @@ -396,14 +303,8 @@ fn use_fake_timers(global: &JSGlobalObject, frame: &CallFrame) -> JsResult<JSVal

#[bun_jsc::host_fn]
fn use_real_timers(global: &JSGlobalObject, frame: &CallFrame) -> JsResult<JSValue> {
let timers = timer_all();

let pinned = {
// SAFETY: per-thread `timer::All`.
let this = unsafe { &mut (*timers).fake_timers };
let _g = timers_lock_guard();
this.deactivate(global)
};
// SAFETY: per-thread `timer::All`; the borrow ends before `release_heap_pin`.
let pinned = unsafe { (*timer_all()).fake_timers.deactivate(global) };
let vm = global.bun_vm_ptr();
for p in pinned {
TimerObjectInternals::release_heap_pin(p, vm);
Expand Down Expand Up @@ -479,30 +380,20 @@ fn run_all_timers(global: &JSGlobalObject, frame: &CallFrame) -> JsResult<JSValu

#[bun_jsc::host_fn]
fn get_timer_count(global: &JSGlobalObject, _frame: &CallFrame) -> JsResult<JSValue> {
let timers = timer_all();
// SAFETY: per-thread `timer::All`.
let this = unsafe { &(*timers).fake_timers };
error_unless_fake_timers(global)?;

let count = {
let _g = timers_lock_guard();
this.timers.count()
};
// SAFETY: per-thread `timer::All`, live for the VM lifetime.
let count = unsafe { (*timer_all()).fake_timers.timers.count() };

Ok(JSValue::js_number(count as f64))
}

#[bun_jsc::host_fn]
fn clear_all_timers(global: &JSGlobalObject, frame: &CallFrame) -> JsResult<JSValue> {
let timers = timer_all();
error_unless_fake_timers(global)?;

let pinned = {
// SAFETY: per-thread `timer::All`.
let this = unsafe { &mut (*timers).fake_timers };
let _g = timers_lock_guard();
this.clear()
};
// SAFETY: per-thread `timer::All`; the borrow ends before `release_heap_pin`.
let pinned = unsafe { (*timer_all()).fake_timers.clear() };
let vm = global.bun_vm_ptr();
for p in pinned {
TimerObjectInternals::release_heap_pin(p, vm);
Expand All @@ -513,14 +404,8 @@ fn clear_all_timers(global: &JSGlobalObject, frame: &CallFrame) -> JsResult<JSVa

#[bun_jsc::host_fn]
fn is_fake_timers(_global: &JSGlobalObject, _frame: &CallFrame) -> JsResult<JSValue> {
let timers = timer_all();
// SAFETY: per-thread `timer::All`.
let this = unsafe { &(*timers).fake_timers };

let is_active = {
let _g = timers_lock_guard();
this.is_active()
};
// SAFETY: per-thread `timer::All`, live for the VM lifetime.
let is_active = unsafe { (*timer_all()).fake_timers.is_active() };

Ok(JSValue::from(is_active))
}
Expand Down
6 changes: 3 additions & 3 deletions src/runtime/timer/Timer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,9 +482,9 @@ impl DateHeaderTimer {
unsafe { (*(*vm).uws_loop()).update_date() };

let elt: *mut EventLoopTimer = &raw mut self.event_loop_timer;
// SAFETY: single JS thread; `All::update` only touches `lock`/`timers`/
// `fake_timers`/`epoch`, disjoint from `date_header_timer` which `self`
// aliases (raw-ptr-per-field re-entry pattern, see jsc_hooks.rs).
// SAFETY: single JS thread; nothing `All::update` touches overlaps
// `date_header_timer`, which `self` aliases (raw-ptr-per-field
// re-entry pattern, see jsc_hooks.rs).
unsafe { (*Self::timer_all()).update(elt, &now.add_ms(1000)) };
} else {
// The date was updated recently, just reschedule for the next second
Expand Down
Loading
Loading