uws: drop us_timer_t on epoll/kqueue in favor of bun's timer heap - #33359
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (18)
WalkthroughThis PR replaces epoll/kqueue socket-timeout sweeps with deadline-clamped polling, gates timer APIs and storage by platform, migrates GC timers and idle-loop keepalive behavior, adjusts shutdown ordering, and adds regression tests for timerfd usage and teardown behavior. ChangesTimer and event-loop wiring changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 10:05 PM PT - Jul 9th, 2026
@Jarred-Sumner, your commit bcb8da2 is building: |
|
Found 5 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this rearchitects timer scheduling across the C event loop, the Rust FFI struct mirrors, GC controller lifetime, and the tick_possibly_forever park path on all POSIX targets — worth a human sign-off.
Extended reasoning...
Overview
This PR removes us_timer_t on epoll/kqueue backends and replaces its three remaining users with cheaper mechanisms: the two GarbageCollectionController timers become intrusive EventLoopTimer nodes on the per-VM pairing heap, EventLoop::forever_timer becomes a bare num_polls bump plus a new poll_and_drain_timers runtime hook, and the uSockets socket-timeout sweep becomes an absolute CLOCK_MONOTONIC deadline folded into the epoll_pwait2/kevent64 timeout. It touches 17 files spanning packages/bun-usockets/ (C), src/uws_sys/ (Rust FFI struct mirrors), src/jsc/ and src/runtime/ (event loop, GC controller, timer dispatch), and adds a new test file.
Security risks
None identified. The change is internal scheduling plumbing; no untrusted input parsing, auth, or permission surfaces are touched.
Level of scrutiny
High. This is core event-loop scheduling that every request, socket timeout, GC nudge, and --watch/debugger-wait park runs through, with divergent per-platform code paths (Windows/libuv keeps us_timer_t; Linux/macOS/FreeBSD drop it entirely). It changes a #[repr(C)] struct layout that must stay byte-identical between C and Rust (us_internal_loop_data_t ↔ InternalLoopData), rewires GarbageCollectionController teardown to depend on runtime_state still being installed, and adds a new RuntimeHooks slot. The us_loop_run change also removes the us_loop_integrate call, which is now a no-op but is a semantic change to the public loop entry point.
Other factors
The PR description is thorough, the new test verifies zero timerfds and that idleTimeout still fires, and the author cross-checked the major socket/fetch/worker suites against a main baseline. The bug-hunting system found nothing. The implementation looks careful (e.g., re-arming the sweep before dispatching so a handler that unlinks the last socket doesn't resurrect a dead deadline; field-wise timespec compare to avoid overflow; schedule() handling ACTIVE→remove→insert). Still, the combination of intrusive-heap lifetime management in unsafe Rust, a C/Rust struct-layout change, and altered park semantics on the tick_possibly_forever path is exactly the kind of change a maintainer should read end-to-end.
|
Answering the three bot comments above. #32447 is a real overlap, and this PR now matches it#32447 is @Jarred-Sumner's own open PR doing the I've pushed b218e18 to converge on its shape exactly, so whichever of the two goes in first, the other rebases without conflict:
If you'd rather land #32447 on its own first, this PR rebases down to the uSockets + Re-verified the arming path after the rework, with a temporary
Not adding the
|
There was a problem hiding this comment.
I didn't find any bugs, but this reworks the core event loop (epoll/kqueue poll timeout, GC pacing, tick_possibly_forever, C↔Rust InternalLoopData layout) across three platforms with a lot of unsafe/FFI surface — worth a human pass before merging.
Extended reasoning...
Overview
This PR removes us_timer_t on epoll/kqueue and replaces its three POSIX users with in-process mechanisms: the GC controller's two timers become intrusive EventLoopTimer nodes on the per-VM heap, the socket-timeout sweep becomes a CLOCK_MONOTONIC deadline folded into the epoll_pwait2/kevent64 timeout, and forever_timer becomes a bare num_polls bump plus a new poll_and_drain_timers runtime hook. It touches 17 files spanning the uSockets C backend (epoll_kqueue.c, loop.c, loop_data.h, libusockets.h), the Rust event loop / VM / GC controller, the tag-dispatch table in dispatch.rs, and the #[repr(C)] InternalLoopData mirror.
Security risks
None identified. No user input parsing, auth, or crypto is touched. The change is internal event-loop plumbing.
Level of scrutiny
High. This is production-critical hot-path code with several dimensions that each deserve careful eyes:
- Cross-language struct layout:
us_internal_loop_data_tgained/lost fields under#ifdef LIBUS_USE_LIBUV, and the RustInternalLoopDatamirror must match exactly on every platform or every field after the divergence is misread. - Event-loop parking semantics:
tick_possibly_forevernow goes through a newpoll_and_drain_timershook instead ofloop_.tick(), andus_loop_rundropped itsus_loop_integratecall. Getting this wrong means hangs (--watch, debugger wait) or busy-spins. - GC pacing: the repeating timer moved from kernel-driven to lazy-armed on first
process_gc_timer(), with re-arm ordering that interacts withupdate_gc_repeat_timerbeing called from inside its own fire body. A regression here is silent (memory growth or excess CPU over hours/days). - Memory safety: two new intrusive
EventLoopTimernodes embedded inGarbageCollectionControllerwithcontainer_ofrecovery, raw-pointerarm()/deinit()paths, and a newDropthat reaches for the thread-local VM. - Three-way platform split: Linux epoll, macOS/FreeBSD kqueue, and Windows libuv now diverge more than before; the Windows path is meant to be unchanged but that's only true if every
#[cfg]/#ifdeflines up.
Other factors
The PR description is thorough and the added test does exercise the observable contract (zero timerfds, idle-timeout still fires). The author's manual verification against baseline test suites is a good signal. But this is a ~600-line architectural change to the event loop that also supersedes another open PR (#32447), and it's exactly the kind of change where a maintainer should sanity-check the design choices (e.g., hold_forever_poll never releasing its num_polls bump, poll_and_drain_timers only calling drain_timers on unix, the sweep re-arm ordering in us_internal_sweep_if_due).
The ~2000 asan failures in build 68483 — root cause and fix (e4b2fbe)Real bug, mine. It only fires under Once the GC controller's timers became heap nodes,
That's precisely where Nothing ever touched the heap that late before, because the GC timers were uSockets timers — Fix: move A hardening attempt I backed out, because it was wrongMy first instinct was to also fix the "freed while linked" half at its owning layer: have The VerificationReproduced and confirmed fixed locally with the lane's flag: All clean. Three of the suites the lane failed on ( Added a regression test for it, gated on |
e4b2fbe to
bdc7d16
Compare
Windows regression, and a scope correction (bdc7d16)The asan lane is clean now (zero That left one real failure I'd missed under the asan noise: The test queues 10 Cause: on libuv, Fix: don't. There was never a reason to move them on libuv — a
State
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/bun/event-loop-timers.test.ts`:
- Around line 39-41: The `countTimerFdsIn` subprocess checks in
`event-loop-timers.test.ts` are only asserting `stdout` and `exitCode`, so any
child-process failure will hide useful diagnostics. Update the affected call
sites to also use `stderr`, and insert the house-style guard `if (exitCode !==
0) { expect(stderr).toBe(""); }` immediately before each `exitCode` assertion.
Apply this to both `countTimerFdsIn` usages so failures surface stderr
consistently.
🪄 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: 6f83cab7-4be3-4d30-a0c1-2bad13cb9162
📒 Files selected for processing (18)
packages/bun-usockets/src/eventing/epoll_kqueue.cpackages/bun-usockets/src/internal/eventing/epoll_kqueue.hpackages/bun-usockets/src/internal/internal.hpackages/bun-usockets/src/internal/loop_data.hpackages/bun-usockets/src/libusockets.hpackages/bun-usockets/src/loop.csrc/event_loop/EventLoopTimer.rssrc/jsc/GarbageCollectionController.rssrc/jsc/VirtualMachine.rssrc/jsc/event_loop.rssrc/jsc/web_worker.rssrc/runtime/dispatch.rssrc/runtime/jsc_hooks.rssrc/uws/lib.rssrc/uws_sys/InternalLoopData.rssrc/uws_sys/Timer.rssrc/uws_sys/lib.rstest/js/bun/event-loop-timers.test.ts
💤 Files with no reviewable changes (1)
- packages/bun-usockets/src/internal/eventing/epoll_kqueue.h
Build 68540's reds are all flakes (re-rolled)Four failures, none in this diff's blast radius:
Build 68523, with this diff's runtime code, was 284 passed / 0 failed across the whole matrix. Re-rolled as Where this standsReady for review. Summary of what changed and why is in the PR body; the three things worth a maintainer's eyes:
|
Final status: the diff is green; CI is red on broken agentsBuild 68557 ( The only test annotation is Taken together with build 68523 — 284 passed / 0 failed across the whole matrix, on runtime code identical to what's here — this branch is green. I've spent my one re-roll ( Ready for reviewThe PR body has the full story. Three things worth a maintainer's eyes:
Headline: a |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
This poll_and_drain_timers call seems strange? Don't we already drain timers? Why are we doing it twice? And why is there any unsafe usage?
|
You're right on all three. Deleted it in Why it was there: "Don't we already drain timers?" Not twice in one pass — And my own justification was circular. I wrote that without the drain, an overdue timer would make So
"Why is there any unsafe usage?" The hook body was raw-pointer-per-field ( Re-verified after the deletion: timerfd count still 0, |
|
Done in I'd used The mocked version, meanwhile, put the deadline in mocked-clock units whenever It does reproduce, just not from a synchronous test body — you need real event-loop ticks inside the window so jest.useFakeTimers();
for (let i = 0; i < 40; i++) {
jest.advanceTimersByTime(5_000); // 200s of mocked time
await Bun.file("/etc/hostname").text(); // a real tick
}
jest.useRealTimers();
Deterministic across runs (3/3, 34/35). That also puts the GC timers in line with Verified after: timerfd count still 0, |
Dropping the hook exposed a real hang — fixed in
|
main |
prints handled, exits 0 |
| that commit | prints handled, then hangs forever |
Cause. run_command.rs did this on a rejected entry point:
if vm.hot_reload != 0 || handled {
vm.add_main_to_watcher_if_needed();
vm.event_loop_ref().tick();
vm.event_loop_ref().tick_possibly_forever(); // ← parks on nothing
}With handled == true and no --watch, nothing registered with that loop can ever wake it. It only ever came back because a 1s GC timerfd happened to be sitting in epoll. So this was a latent "park on nothing" bug that main papers over with a timer it doesn't even want — and the accident survived my first pass only because poll_and_drain_timers' get_timeout also returned that 1s GC deadline. Delete the hook, delete the accident, hang.
Fix. Drop the call. It was redundant: the core run-loop immediately below already does the waiting — its watcher arm parks in tick_possibly_forever, and without a watcher it drains until the loop goes quiet and the process exits. Parking there only ever blocked on nothing.
Added a regression test; it times out against the broken code and passes against the fix.
Verified
process.test.js— was a 90s hang, now runs clean (103 pass / 1 fail, and that one isprocess.env.USERmissing in this container; released bun fails it too, 102/2).hot.test.ts— 12/12.--watchstill parks and reloads: 0 voluntary context switches over a 6s idle window, then picks up the change.- timerfd count still 0;
event-loop-timers.test.ts5/0; fake-timers 61/0;test-timers-immediate-queue.jspasses;BUN_DESTRUCT_VM_ON_EXIT=1teardown clean;rust:check-all10/10.
Worth flagging: I'd have missed this if I hadn't gone looking at the superseded build 68779 — it was canceled by my next push, so its failures never surfaced as a CI notification.
Status after
|
|
Addendum on the The correlation is clean:
What it isn't. I replicated the test's exact
What I think it is. A rare lost wakeup somewhere in the watcher → So I'd still rather not guess. Fixing the wake path is the right answer, but it's your code and I don't want to paper over it by quietly reinstating the bound you just asked me to remove. If you'd rather I bound the park as a stopgap, say the word and it's a one-liner — a 2s bound would still leave Everything else on the branch is green. |
|
@robobun could it be that the it's spin-looping instead of blocking in epoll/kqueue? |
Not that I can see. I instrumented So it's ~3 calls per reload cycle, each one actually blocks in What I did findFirst, I was wrong about the scale. Pulling the raw job logs instead of the annotations: it hangs on 6–8 of the 10 Linux lanes on the first attempt, and the runner's retry hides most of them. The 1–2 that show as Second — and this is the interesting bit — the fixture has loop { on_before_exit(); report_exception...(); tick_possibly_forever(); }— So the stall is a reload whose completion needs one more wake that doesn't arrive on a loaded CI box — and on What I'd like to try nextPush instrumentation to CI to see which Or if you'd rather just bound the park while we root-cause it separately — say the word. |
A us_timer_t cost an entire file descriptor on Linux (timerfd) and a pair of kevent64 syscalls per arm on macOS/FreeBSD. Four were live in a normal process: the socket-timeout sweep on the JS thread, the two GC controller timers, and a second sweep on the HTTP client thread. - GarbageCollectionController's two timers become EventLoopTimer nodes embedded in the controller, scheduled on the per-VM timer heap. No new allocation: both nodes are fields, not boxes. - EventLoop.forever_timer only existed to keep num_polls non-zero so us_loop_run_bun_tick would park instead of returning immediately; its callback was a no-op. On posix that is now a plain num_polls bump. tick_possibly_forever() polls bounded by the timer heap's next deadline and drains it afterwards, via a new poll_and_drain_timers runtime hook. - The socket-timeout sweep becomes an absolute deadline in us_internal_loop_data_t, folded into the epoll_pwait2/kevent64 timeout and dispatched from the same tick. This is the existing quic_next_tick_us pattern, and it works on loops that have no timer heap behind them (the HTTP client thread, the CLI mini event loops). us_create_timer/us_timer_set/us_timer_close and friends are now libuv-only, along with the Rust uws::Timer wrapper. No behavior change on Windows.
Converge the GarbageCollectionController half of this change on the shape Jarred already landed in #32447 so whichever goes first rebases cleanly: GcOneShot/GcRepeating tags, arm(), and arming the repeating timer on the first process_gc_timer() tick rather than in init() (keeps the timer heap untouched until the event loop is wired, which matters for Windows' ensure_uv_timer).
The GC controller's timers are now heap nodes, so gc_controller.deinit()
removes them from the per-VM timer heap. Both teardown paths called it
*after* JSC teardown, which is where ~RunLoop::Timer frees the WTFTimer
nodes sharing that heap — and WTFTimer::cancel skips its unlink once the
script execution context is unregistered, so those nodes are freed while
still linked. Removing a GC node afterwards walks into freed siblings:
WRITE of size 8 ... heap-use-after-free
#0 Intrusive::combine_siblings src/io/heap.rs:255
#2 Intrusive::remove src/io/heap.rs:166
#5 All::remove src/runtime/timer/mod.rs:780
#8 GarbageCollectionController::deinit
#9 VirtualMachine::global_exit
freed by:
#7 Box<WTFTimer>::drop
#10 WTFTimer::deinit
#12 WTF::RunLoop::TimerBase::~TimerBase()
Nothing touched the heap that late before, because the nodes were uws
timers. Move deinit() next to cancel_all_timers in both paths, which is
the window the codebase already reserves for exactly this, and make
deinit() terminal so nothing re-arms after the nodes leave the heap.
Only reproduces under BUN_DESTRUCT_VM_ON_EXIT, which the x64-asan lane
sets; add a regression test behind it.
Moving them onto the per-VM timer heap broke Windows: every heap insert runs All::ensure_uv_timer(), which restarts the event loop's one shared uv_timer for the soonest deadline. The GC controller arms often, so a JS timer that is already due keeps getting its wakeup pushed out, and test-timers-immediate-queue starved (hit=930 instead of 10). It failed on all three Windows lanes from the first commit of this branch. There was never a reason to touch libuv here: a us_timer_t there is a uv_timer_t, which costs neither a file descriptor nor a syscall per arm. Only epoll/kqueue pay timerfd/EVFILT_TIMER, and that is what this branch set out to remove. So the scheduling backend is per-platform now, behind arm_one_shot / rearm_repeating / ensure_repeating_armed / unschedule, with the state machine and the fast/slow backoff shared. libuv keeps exactly the code it had on main. poll_and_drain_timers likewise collapses to the tick() the caller used to do inline, since on libuv the heap does not bound uv_run.
Build 68540 went red on four agent-load flakes, none of which touch this diff. v8-heap-snapshot.test.ts was SIGKILL'd on ubuntu 25.04 x64 but passed on that same lane in build 68523, whose runtime code is byte-identical (the only delta is a stderr destructure in the test file), and passed on the aarch64 and x64-baseline lanes of 68540 itself. The two darwin failures are timeouts on tests that run in 1.1s locally, and the Windows one already passed on retry.
epoll/kqueue creates exactly one (wakeup_async); the sweep is a deadline, not a poll.
|
Fair. The bloat was mine, and it was mostly one bad decision.
The +79 that remains is the 137-line test file. What was wasteful. When putting the GC timers on the heap broke The actual fix ( Also deleted All key suites still green locally; |
|
Build 71240: 284 passed / 0 failed.
net +79 (src+packages −58, the rest is the test file). |
| fn arm(vm: *mut VirtualMachine, t: *mut EventLoopTimer, ms: i32) { | ||
| // SAFETY: `t` is one of the two embedded nodes of the per-VM controller, | ||
| // address-stable for the VM lifetime; JS-thread only. | ||
| unsafe { | ||
| if (*t).state == TimerState::ACTIVE { | ||
| VirtualMachine::timer_remove(vm, t); | ||
| } | ||
| (*t).next = Timespec::now(TimespecMockMode::ForceRealTime).add_ms(i64::from(ms)); | ||
| VirtualMachine::timer_insert(vm, t); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🔴 arm() computes the re-arm deadline from Timespec::now(ForceRealTime), but All::drain_timers compares against a cached Timespec::now(AllowMockedTime). Under jest.useFakeTimers() (which seeds the mocked monotonic clock at 0), if advanceTimersByTime(N) pushes mocked time past the machine's CLOCK_MONOTONIC uptime and the loop then reaches drain_timers, on_gc_repeating_timer re-inserts at real-uptime+1s (still < cached mocked now) on every fire and the drain loop never terminates. Re-arm from the passed-in now like EventLoopDelayMonitor::on_fire does (mod.rs:528-539), or compute next as max(real_now, passed_now).add_ms(ms).
Extended reasoning...
What the bug is. GarbageCollectionController::arm() sets the timer's next deadline to Timespec::now(TimespecMockMode::ForceRealTime).add_ms(ms) — i.e. CLOCK_MONOTONIC uptime + interval. on_gc_repeating_timer unconditionally tail-calls Self::arm(vm, &raw mut this.gc_repeating_timer, interval) to re-schedule itself. But All::drain_timers (src/runtime/timer/mod.rs:1030-1049) drives the real timer heap in a loop against a single cached now read once via next() (mod.rs:985-987) as Timespec::now(AllowMockedTime). Under jest.useFakeTimers(), that mocked monotonic clock is seeded at Timespec::EPOCH (0 ns) and advanced only by advanceTimersByTime. When the mocked clock exceeds real CLOCK_MONOTONIC uptime + 1s, every fire re-inserts the node at a deadline still less than the cached now, so drain_timers pops it again immediately and never returns.
Code path. (1) GcRepeating has allow_fake_timers() == false (EventLoopTimer.rs:217), so insert_lock_held puts it into self.timers — the real heap that next() peeks at mod.rs:984 — not the fake heap. (2) FakeTimers::activate() seeds mocked monotonic time at Timespec::EPOCH via mock_time::set(0). (3) drain_timers sets has_set_now = true after the first next() call and never re-reads the clock for the rest of the loop. (4) on_gc_repeating_timer sets state = FIRED, then unconditionally calls Self::arm(...), which re-reads real CLOCK_MONOTONIC, adds interval (1000ms), and re-inserts synchronously — inside the drain loop. (5) The dispatch arm in dispatch.rs discards _now for GcRepeating, so the fire body never sees the drain loop's cached clock and cannot re-arm relative to it.
Why nothing prevents it. The PR author's own analysis (comment 2026-07-06T07:36:10Z, commit b1cd516) reasoned that a ForceRealTime deadline "sits far ahead of the mocked clock and simply never fires" because useFakeTimers() "seeds the clock at Timespec::EPOCH and counts up from zero". That covers only the mocked < real direction. The mocked > real direction — reached the moment advanceTimersByTime crosses machine uptime — was not considered; their 200s repro passed only because their machine had > 200s uptime. On a fresh CI container (uptime seconds to minutes), even modest advances like advanceTimersByTime(10 * 60 * 1000) cross the threshold.
Step-by-step proof. On a machine with 120s of CLOCK_MONOTONIC uptime:
- Test calls
jest.useFakeTimers()→ mocked monotonic = 0 ns. - Test calls
jest.advanceTimersByTime(7 * 24 * 3600 * 1000)→ mocked monotonic = 604800000 ms. - Test does
await Bun.file("/etc/hostname").text()→ real event-loop tick →auto_tick→drain_timers. next()cachesnow = 604800.000s(mocked).has_set_now = true.- The
GcRepeatingnode (armed by an earlierprocess_gc_timer()at real-uptime + 1s ≈ 121s) hasnext ≈ 121.000s< 604800s → popped → fired. on_gc_repeating_timerruns, tail-callsarm(), which setsnext = CLOCK_MONOTONIC (≈120s) + 1s = 121sand re-inserts intoself.timers.- Loop iterates:
next()peeks the same node,has_set_nowis already true sonowstays 604800s, 121s < 604800s → popped → fired → step 6. Infinite loop.
Impact. A hard hang (100% CPU in drain_timers) reachable from any test file that combines fake timers with a large advanceTimersByTime and then awaits real I/O — a common pattern for JWT/session-expiry/cache-TTL tests. Triggers deterministically on fresh CI VMs whose CLOCK_MONOTONIC uptime is less than the advanced interval. This is new to this PR: before, the GC timers were us_timer_t timerfds and were never on the heap drain_timers walks. (The bug class — ForceRealTime deadlines compared against a cached AllowMockedTime — is latent for WTFTimer::update on main, but WTFTimers don't unconditionally re-arm on every fire; GcRepeating does, on every VM, once process_gc_timer() has run once.)
Fix. Match EventLoopDelayMonitor::on_fire (mod.rs:528-539) and DateHeaderTimer: re-arm from the now argument that drain_timers passes into the fire callback, so the re-inserted deadline is always ≥ the cached loop clock. Concretely, plumb now through the GcRepeating dispatch arm (currently |c, _now, vm|) and have arm() compute next as max(Timespec::now(ForceRealTime), passed_now).add_ms(ms) — that keeps real-time pacing when fake timers aren't installed (the property b1cd516 added) while guaranteeing forward progress when they are.
There was a problem hiding this comment.
Addressed in #33896. Turned out to be broader than arm(): the spin reproduces with BUN_GC_TIMER_DISABLE=1 too (WTFTimer hits the same re-arm loop), so the fix moves the comparison in drain_timers/get_timeout to ForceRealTime rather than flooring the re-arm deadline. StatWatcherScheduler gets the matching arm-side change so its deadlines stay in the same units as the heap.
| // The core run-loop below does the actual waiting. | ||
| if vm.hot_reload != 0 || handled { | ||
| vm.add_main_to_watcher_if_needed(); | ||
| // SAFETY: `event_loop` is a self-pointer into this VM; | ||
| // uniquely accessed here. | ||
| vm.event_loop_ref().tick(); |
There was a problem hiding this comment.
🔴 Commit bcb8da2 ("Delete pointless test") removed the entire 137-line test/js/bun/event-loop-timers.test.ts — all 5 tests, not one — leaving the PR with zero automated coverage while the description still lists them. Was that intended to be the whole file? At minimum, the uncaughtException-handler-still-exits test (guards this run_command.rs change; hung forever on the broken commit) and the BUN_DESTRUCT_VM_ON_EXIT ASAN test (guards the gc_controller.deinit() reordering that broke ~2000 tests in build 68483) look worth keeping — those are the crash/UAF repros CLAUDE.md asks for, and they're cross-platform unlike the /proc/self/fd counts.
Extended reasoning...
Commit bcb8da2a (HEAD, authored by Jarred) deleted test/js/bun/event-loop-timers.test.ts in its entirety — 137 lines, 5 tests. git show --stat confirms it's the only change in that commit; the file no longer exists on disk; and the PR's changed-files list (18 files) now contains no test file at all. The PR description's Tests section still lists all four bullet points and the robobun evidence footer still names the file, so the description is now stale.
The commit message says "Delete pointless test" (singular). Given that only one of the five tests could plausibly read as "pointless" — and given that the description wasn't updated and the previous robobun status comment (2h earlier) still counts "the 137-line test file" toward the net diff — this looks like it may have been meant to delete one test rather than the whole file.
Three of the five deleted tests are regression guards for bugs this PR itself introduced and then fixed during development:
- "an uncaughtException handler on a rejected entry point still exits" guards the
run_command.rschange (commit9ffcaf3a, this file at line 1521). Per the timeline,process.on('uncaughtException', ()=>{}); throw new Error()printedhandledand then hung forever on the broken commit, causing the 90s timeout inprocess.test.json build 68779. This is exactly the "crash fixes need the crashing input as a spawned fixture" test CLAUDE.md requires, and the "hang-guard tests assert the process exited on its own (signalCode === null)" pattern it describes. - "destructing the VM on exit does not corrupt the timer heap" guards the
gc_controller.deinit()reordering inVirtualMachine.rs/web_worker.rs(commite8925fd1). Per the timeline, getting this wrong was a heap-use-after-free inIntrusive::combine_siblingsthat broke ~2000 tests on the x64-asan lane in build 68483. CLAUDE.md: "UAF/leak fixes need an ASan repro on the unfixed build." - "Bun.serve idleTimeout still expires an idle connection" verifies the new
sweep_next_tick_nsdeadline mechanism inloop.cactually fires socket timeouts — the direct behavioral test that the sweep-timer replacement works.
The remaining two are the Linux-only /proc/self/fd timerfd-count assertions. Those are the only tests that fail on main and prove the headline change (4 → 0 timerfds), but they're also the only ones one might reasonably call brittle/platform-specific — plausibly the "pointless test" the commit message meant.
Step-by-step proof: (1) git log --oneline shows bcb8da2a is HEAD. (2) git show --stat bcb8da2a shows exactly one hunk: test/js/bun/event-loop-timers.test.ts | 137 --------. (3) ls test/js/bun/event-loop-timers.test.ts → No such file. (4) git show bcb8da2a^:test/js/bun/event-loop-timers.test.ts shows 5 test.concurrent(...) blocks, three of which are cross-platform. (5) The PR diff (18 files changed) touches only src/ and packages/ — zero files under test/. (6) The PR description's ## Tests section still names the file and all four items.
CLAUDE.md is explicit on both counts: "Every behavioral change ships an automated test in the same PR. 'Verified manually' … don't count, even for one-liners" and "Never silently weaken, skip, or delete an existing test or safety net. Every deletion needs a stated reason or replacement." If the deletion was intentional, the description should be updated and the hang / UAF regression guards restored (they're cross-platform, cheap, and each protects a specific commit in this PR). If it was a mistake — git revert bcb8da2a restores the file.
) Follow-up to #33359 ([review](#33359 (comment))). ## Repro ```ts jest.useFakeTimers(); for (let i = 0; i < 100; i++) jest.advanceTimersByTime(40 * 24 * 3600 * 1000); await Bun.file(process.execPath).stat(); // spins at 100% CPU ``` Any `jest.advanceTimersByTime` that pushes the mocked monotonic clock past the machine's `CLOCK_MONOTONIC` uptime, followed by a real I/O await, spins `All::drain_timers` forever. Deterministic on a fresh CI container; on a developer machine it needs an advance larger than uptime. ## Cause `All::drain_timers` and `All::get_timeout` compared `self.timers` against `Timespec::now(AllowMockedTime)`. That heap holds only `allow_fake_timers()==false` nodes (GC controller, `WTFTimer`, `bun:test` timeouts, `StatWatcherScheduler`) plus anything armed before `useFakeTimers()`, and every one of those arms its deadline with `ForceRealTime`. Once the mocked clock exceeds real uptime, every node looks overdue; any that re-arm on fire (`GcRepeating` unconditionally, `WTFTimer` when JSC's GC scheduler has more work) are re-inserted at `real_uptime + interval`, still less than the cached mocked `now`, and the drain loop never returns. The `GcRepeating` case is new to #33359 (the GC timers were kernel timerfds before and fired on real time regardless of fake timers). The `WTFTimer` case was latent before that. Reproduces with `BUN_GC_TIMER_DISABLE=1` too, so the fix has to be at the comparison layer, not in `GarbageCollectionController::arm()`. ## Fix `drain_timers::next()` and `get_timeout` read `ForceRealTime` for `self.timers`. The fake heap is already walked separately by `advanceTimersByTime`, so the two heaps are now ticked against their own clocks. No behavior change when fake timers are not installed (`AllowMockedTime == ForceRealTime` then). `StatWatcherScheduler::set_timer` was the one `allow_fake_timers()==false` tag that still armed with `AllowMockedTime`; flipped to `ForceRealTime` so its deadlines are in the same units as the heap they live in. The Windows path (`ensure_uv_timer`) already used `ForceRealTime`. ## Verification `test/js/bun/test/test-timers.test.ts` spawns a `bun test` child that advances mocked time by ~11 years and then awaits file I/O inside the fake-timer window. Without this change the child spins and is killed by the 20s spawn timeout; with it the child exits in ~35ms. Also ran `test/js/bun/test/fake-timers/` (61 pass), `test/js/node/timers/` (20 pass), `test/js/node/watch/fs.watchFile.test.ts` (9 pass), and `rust:check-all` (10/10). --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Main's #33359 gated the timer-based sweep path behind LIBUS_USE_LIBUV (vs the new folded-timeout path on epoll/kqueue). This branch removed libuv entirely, so LIBUS_USE_LIBUV is never defined and the guards silently took the POSIX #else branch on Windows, dropping sweep_timer and has_added_timer_to_event_loop from the structs and breaking the static_asserts in bun_iocp.h and the Rust mirrors. The IOCP backend uses the same timer-based sweep (us_create_timer / us_timer_set) that libuv did, so the guards become LIBUS_USE_BUN_IOCP. Also re-guards us_timer_close(sweep_timer) in loop_data_free, which the earlier merge had left unconditional (compile error on POSIX).
…#33131) `WTF::RunLoop::TimerBase` (JSC's GC scheduler timers, and the timer behind an `Atomics.waitAsync` timeout) is started and stopped from threads other than the one that owns its run loop: `Atomics.notify` on thread B re-arms thread A's `JSRunLoopTimer` and cancels thread A's `waitAsync` timeout, and `~ArrayBufferContents` does the same from whichever thread drops the last `SharedArrayBuffer` reference. Those timers shared one pairing heap (`All.timers`) with `setTimeout` and every other same-thread timer. `All.lock` existed only because of them, and `get_timeout`'s pre-poll pass peeked and popped that shared heap without taking it, racing the locked `Intrusive::remove` from the other threads. In a debug build that trips ``` assertion failed: self.root == v (src/io/heap.rs:165, Intrusive::remove) ``` and in a release build the check compiles out, so `remove` walks the corrupted links silently. ### Fix Rather than locking the shared heap, take the one cross-thread client out of it: - `All.wtf_timers` (a `Guarded<TimerHeap>`) holds only `WTFTimer` nodes, reached through `All::wtf_arm` and `All::wtf_disarm` from any thread. - `All.lock` is deleted. `insert`, `remove`, `update`, the regular heap, the fake timers, and the epoch are single threaded again, asserted with a `debug_assert!` on the owning thread id, so `setTimeout` no longer takes a mutex per call. The fake-timer lock helper and its `assert_locked` machinery existed only because of that shared lock, so they are deleted too. - `get_timeout` no longer pops and fires inline. It drains the due `WTFTimer`s through their own mutex (dropping the guard across each `fire`, which can synchronously re-enter it), then returns `min(wtf, regular, quic)`. - `drain_timers` drains the `WTFTimer` heap first, preserving both existing firing paths: the pre-poll one only runs when the uws loop is active, and on Windows `drain_timers` runs from `on_uv_timer`. - `WTFTimer.lock` (the per-instance mutex) is deleted; `wtf_timers` owns everything it guarded, and `update` never took it anyway. - `ensure_uv_timer` folds both heaps into the libuv deadline and is now only reachable from the owning thread, which also removes the cross-thread TLS hazard it had. `WTFTimer` never enters the fake-timer heap. Its tag already returned `false` from `allow_fake_timers()`, but it can no longer reach that branch at all. ### Reproducer `test/js/web/timers/timer-heap-race.test.ts` with `timer-heap-atomics-fixture.ts`: four threads each arm batches of short `Atomics.waitAsync` timeouts while the others `Atomics.notify` them, alongside `setTimeout` churn. Under a debug build of `main` this aborts within a few seconds with the assertion above, or with an ASan `heap-use-after-free` in `Intrusive::remove` at `src/io/heap.rs:178`. With this change it runs to completion. A second fixture drives `Bun.gc(true)` in a `setTimeout` loop so the per-VM `JSRunLoopTimer` is re-armed and popped repeatedly on the owning thread. ### Rebased onto #33359, #33623, #33896, #34009 Squashed to one commit and rebased; the earlier commits in the branch history were the two rejected approaches (locking `get_timeout`, then `Guarded<Heaps>`). One conflict had semantic content: #33896 switched the regular heap's clock reads in `get_timeout` and `next` from `AllowMockedTime` to `ForceRealTime` because every node that lands there is armed in real-time units. `WTFTimer` is one of those tags, and its new separate heap is the same case, so the `ForceRealTime` change is also applied in `drain_due_wtf_timers`. The #33359 hunk (skip `uv_timer.start` when already due sooner) and the #33623 hunks (`set_wall_ms`, `clear_wall`, `Bun__FakeTimers__setSystemTime`) applied without semantic interaction. #34009 added a `now_out: &mut Option<Timespec>` out-parameter to `get_timeout` so its caller can reuse the monotonic clock read; the lazy `maybe_now` in the rewritten body becomes a reborrow of that out-parameter, and `drain_due_wtf_timers` fills it the same way the old loop did. ### Verification ``` bun bd test test/js/web/timers/timer-heap-race.test.ts # 2 pass bun bd test test/js/bun/test/test-timers.test.ts # #33896's suite, all pass bun bd test test/js/bun/test/fake-timers/fake-timers.test.ts # #33623's suite, all pass ``` Three `setTimeout doesn't leak when X is called inside its own callback` tests in `test/js/web/timers/` fail under debug + ASan on this machine, and did so identically with an unmodified `src/` at the previous merge base: their fixtures widen the RSS threshold only when `process.execPath.includes("bun-asan")`, which is never true for the `bun-debug` binary name. <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 11 · 10 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 1 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/web/timers/timer-heap-race.test.ts info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05) info: component rust-src is up to date info: checking for self-update (current version: 1.29.0) bun test v1.4.0 (f0fa434) test/js/web/timers/timer-heap-race.test.ts: 19 | 20 | return { stdout, stderr, signal: proc.signalCode, exitCode }; 21 | } 22 | 23 | it("timer heap survives cross-thread Atomics.waitAsync timeout cancellation", async () => { 24 | expect(await runFixture("timer-heap-atomics-fixture.ts")).toEqual({ ^ error: expect(received).toEqual(expected) { - "exitCode": 0, - "signal": null, - "stderr": Any<String>, + "exitCode": 134, + "signal": "SIGABRT", + "stderr": + "============================================================ + Bun Debug v1.4.0 (f0fa434) Linux x64 + Linux Kernel v6.17.0 | glibc v2.41 + CPU: sse42 popcnt avx avx2 avx51 ... (truncated) release without fix: 1 skipped bun test v1.4.0-canary.1 (1498d7b) test/js/web/timers/timer-heap-race.test.ts: (pass) timer heap survives cross-thread Atomics.waitAsync timeout cancellation [3036.31ms] (skip) timer heap stays consistent while GC re-arms the RunLoop timer 1 pass 1 skip 0 fail 1 expect() calls Ran 2 tests across 1 file. [3.21s] __F:0:S:1 ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/web/timers/timer-heap-race.test.ts info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05) info: component rust-src is up to date info: checking for self-update (current version: 1.29.0) bun test v1.4.0 (f0fa434) test/js/web/timers/timer-heap-race.test.ts: (pass) timer heap survives cross-thread Atomics.waitAsync timeout cancellation [3787.70ms] (pass) timer heap stays consistent while GC re-arms the RunLoop timer [2478.37ms] 2 pass 0 fail 2 expect() calls Ran 2 tests across 1 file. [8.23s] __F:0:S:0 release with fix: 1 skipped $ bun scripts/build.ts --profile=release info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu error: could not download file from 'https://static.rust-lang.org/dist/2026-05-06/channel-rust-nightly.toml' to '/root/.rustup/tmp/pg_7uy9hcvyywudd_file.toml': error downloading file: error sending request for url (https://static.rust-lang.org/dist/2026-05-06/channel-rust-nightly.toml): client error (Connect): tls handshake eof [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision f0fa434 features (none) 22 deps, 106 codegen, 1168 objects in 667ms ninja: Entering directory `/workspace/bun/build/release' [1/1231] install /workspace/bun bun install v1.4.0-canary.1 (1498d7b) Checked 124 installs across 170 packages (no changes) [10.00ms] [2/1231] install /workspace/bun/packages/bun-error bun install v1.4.0-canary.1 (1498d7b) Checked 1 install across 2 packages (no changes) [1.00ms] [3/1231] gen bindgenv2 [4/1231] install /workspace/bun/src/node-fallbacks bun install v1.4.0-canary.1 (1498d7b) Checked 129 installs across 147 packages (no changes) [ ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/runtime/hw_exports.rs | 4 +- src/runtime/jsc_hooks.rs | 12 +- src/runtime/test_runner/timers/FakeTimers.rs | 183 ++-------- src/runtime/timer/Timer.rs | 6 +- src/runtime/timer/WTFTimer.rs | 58 +--- src/runtime/timer/mod.rs | 414 ++++++++++++----------- src/runtime/timer/timer_object_internals.rs | 8 +- test/js/web/timers/timer-heap-atomics-fixture.ts | 68 ++++ test/js/web/timers/timer-heap-gc-fixture.ts | 17 + test/js/web/timers/timer-heap-race.test.ts | 43 +++ 10 files changed, 411 insertions(+), 402 deletions(-) ``` </details> **gate history** · 3 passed · 0 rejected · iteration 11 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/runtime/hw_exports.rs 2 3 0 src/runtime/jsc_hooks.rs 2 2 0 src/runtime/test_runner/timers/FakeTimers.rs 9 16 0 src/runtime/timer/Timer.rs 1 1 0 src/runtime/timer/WTFTimer.rs 3 13 0 src/runtime/timer/mod.rs 24 59 0 src/runtime/timer/timer_object_internals.rs 4 3 0 test/js/web/timers/timer-heap-atomics-fixture.ts 2 4 0 test/js/web/timers/timer-heap-gc-fixture.ts 2 3 0 test/js/web/timers/timer-heap-race.test.ts 3 6 0 ``` </details> <!-- robobun:evidence:end -->
…896) Follow-up to #33359 ([review](oven-sh/bun#33359 (comment))). ## Repro ```ts jest.useFakeTimers(); for (let i = 0; i < 100; i++) jest.advanceTimersByTime(40 * 24 * 3600 * 1000); await Bun.file(process.execPath).stat(); // spins at 100% CPU ``` Any `jest.advanceTimersByTime` that pushes the mocked monotonic clock past the machine's `CLOCK_MONOTONIC` uptime, followed by a real I/O await, spins `All::drain_timers` forever. Deterministic on a fresh CI container; on a developer machine it needs an advance larger than uptime. ## Cause `All::drain_timers` and `All::get_timeout` compared `self.timers` against `Timespec::now(AllowMockedTime)`. That heap holds only `allow_fake_timers()==false` nodes (GC controller, `WTFTimer`, `bun:test` timeouts, `StatWatcherScheduler`) plus anything armed before `useFakeTimers()`, and every one of those arms its deadline with `ForceRealTime`. Once the mocked clock exceeds real uptime, every node looks overdue; any that re-arm on fire (`GcRepeating` unconditionally, `WTFTimer` when JSC's GC scheduler has more work) are re-inserted at `real_uptime + interval`, still less than the cached mocked `now`, and the drain loop never returns. The `GcRepeating` case is new to #33359 (the GC timers were kernel timerfds before and fired on real time regardless of fake timers). The `WTFTimer` case was latent before that. Reproduces with `BUN_GC_TIMER_DISABLE=1` too, so the fix has to be at the comparison layer, not in `GarbageCollectionController::arm()`. ## Fix `drain_timers::next()` and `get_timeout` read `ForceRealTime` for `self.timers`. The fake heap is already walked separately by `advanceTimersByTime`, so the two heaps are now ticked against their own clocks. No behavior change when fake timers are not installed (`AllowMockedTime == ForceRealTime` then). `StatWatcherScheduler::set_timer` was the one `allow_fake_timers()==false` tag that still armed with `AllowMockedTime`; flipped to `ForceRealTime` so its deadlines are in the same units as the heap they live in. The Windows path (`ensure_uv_timer`) already used `ForceRealTime`. ## Verification `test/js/bun/test/test-timers.test.ts` spawns a `bun test` child that advances mocked time by ~11 years and then awaits file I/O inside the fake-timer window. Without this change the child spins and is killed by the 20s spawn timeout; with it the child exits in ~35ms. Also ran `test/js/bun/test/fake-timers/` (61 pass), `test/js/node/timers/` (20 pass), `test/js/node/watch/fs.watchFile.test.ts` (9 pass), and `rust:check-all` (10/10). --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
2 participants
us_timer_tis expensive on POSIX: each one holds a file descriptor on Linux (timerfd_create+timerfd_settime) and costs a pair ofkevent64syscalls per arm on macOS/FreeBSD. Bun already has a pairing-heapEventLoopTimerthat costs nothing per timer.A plain
bun runwith oneBun.serve+ onefetchholds four timerfds: the socket-timeout sweep plus the two GC-controller timers on the JS thread, and a second sweep on the HTTP client thread.Sweep — becomes an absolute
CLOCK_MONOTONICdeadline inus_internal_loop_data_t, folded into theepoll_pwait2/kevent64timeout and dispatched from the same tick. Same mechanismquic_next_tick_usalready uses; works on loops that have no timer heap (the HTTP client thread, the CLI mini loops).GC timers — become
EventLoopTimernodes embedded in the controller. Their tags opt out ofjest.useFakeTimers()and they arm on real time (GC pacing is Bun's, not the test's). Doing this hit a latent bug inAll::ensure_uv_timer: it restarts theuv_timeron every insert, and restarting an already-overdue handle shifts its wakeup out by 1ms each time, so the GC controller re-arming on every tick starved the already-due callback forever (test-timers-immediate-queue,hit=930instead of 10). Fixed by skipping the restart when the handle is already armed and due sooner-or-equal.tick_possibly_forever()— stays bounded at ~1s, just without the fd. Its trailingtick()can start work (a--hotreload on a worker thread) whose only wake source is a cross-threadwakeup(), and after a throwing reload the watcher loop degenerates totick_possibly_foreveron repeat.mainnever parked here unbounded anyway: the GC timerfd woke it every second. Theforever_timerbecomes anum_pollsbump on epoll/kqueue.Teardown —
gc_controller.deinit()now removes heap nodes, so it moves next tocancel_all_timers(before JSC teardown, where~RunLoop::Timerfrees theWTFTimernodes sharing the heap). Also fixed: a rejected entry point whoseuncaughtExceptionhandler swallowed the error parked with nothing able to wake it — the core run-loop already does the waiting.us_create_timer/us_timer_set/us_timer_closeand theuws::Timerwrapper are libuv-only now.timerfd/EVFILT_TIMERare gone fromepoll_kqueue.c.src/+packages/: net −58.Tests
test/js/bun/event-loop-timers.test.ts:/proc/self/fdholds zeroanon_inode:[timerfd]entries idle and with a server + the HTTP thread + JS timers live (fails onmain: 3 and 4).Bun.serveidleTimeoutstill expires an idle connection.BUN_DESTRUCT_VM_ON_EXIT=1teardown is ASAN-clean.uncaughtExceptionhandler on a rejected entry point still exits.--watchidle: ~1 wakeup/sec (main: ~2.25/sec).no test proof · iteration 11 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/event-loop-timers.test.ts