timer: assign epoch on heap insert so AbortSignal.timeout fires in creation order - #34895
Conversation
…eation order AbortSignal.timeout(ms) signals created with the same deadline were firing their abort events in reverse creation order, while setTimeout with the same delay fires in creation order. AbortSignal.timeout schedules via All::insert() directly, which did not bump the per-timer epoch, so every signal timer had epoch 0. With all epochs equal, the pairing-heap meld makes each new insert the root, giving LIFO order on pop. setTimeout goes through All::update() which did bump the epoch, so it was unaffected. Move the epoch bump from update() into insert() so every JS-timer heap insertion (setTimeout/setInterval reschedule and AbortSignal.timeout creation) gets a fresh epoch. update() calls insert(), so setTimeout/setInterval behavior is unchanged. Non-JS-timer tags are untouched since js_timer_flags_ptr returns None for them.
|
Reproduced with the reported snippet; the new test in |
|
Updated 2:18 AM PT - Jul 21st, 2026
✅ @robobun, your commit 1080b7c6e9413ed8f32cc653e512eb2bada2243f passed in 🧪 To try this PR locally: bunx bun-pr 34895That installs a local version of the PR into your bun-34895 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesJS timer ordering
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean fix that moves the epoch bump one call level down so AbortSignal.timeout gets a fresh tiebreak epoch on heap insert. One minor test nit inline.
What was reviewed:
- Enumerated every direct
All::insert()caller (DNS, DateHeaderTimer, EventLoopDelayMonitor, Subprocess, GC, Valkey, SQL hooks) — all use non-JS-timer tags, sojs_timer_flags_ptrreturnsNoneand no new epoch bump occurs for them. TimerObjectInternalsreschedule paths (setInterval re-arm, refresh) all go throughupdate()→insert(): still exactly one bump per schedule, so setTimeout/setInterval ordering is unchanged.- Epoch bump now runs before the heap insert in
insert(), soEventLoopTimer::lesssees the fresh epoch during meld — same as before. - No
&mut *timeris live across the newjs_timer_flags_ptrcall ininsert()(SB-clean, mirroring the pattern removed fromupdate()).
Extended reasoning...
Overview
Two files: src/runtime/timer/mod.rs moves the ~6-line epoch-bump block from All::update() into All::insert() (which update() already calls as its final step), and test/js/web/abort/abort.test.ts adds a subprocess test asserting six AbortSignal.timeout(5) handlers plus one setTimeout(_, 5) fire in creation order. The PR description correctly diagnoses the root cause: AbortSignal.timeout schedules via the timer_insert runtime hook → All::insert() directly, bypassing the epoch bump that lived only in update(), so all signal timers had epoch 0 and the pairing-heap tiebreak produced LIFO order.
Security risks
None. This is a scheduling-order fix inside the per-thread timer heap; no untrusted input parsing, no allocation sizing, no cross-thread state. The moved block is byte-identical to what was already running on every setTimeout/setInterval schedule.
Level of scrutiny
Medium — timer heap ordering is core runtime infrastructure, but the change is mechanical: a block moves from a caller into the callee it unconditionally invokes. I verified behavior-preservation by checking every other insert() call site: DateHeaderTimer (tag DateHeaderTimer), EventLoopDelayMonitor (tag EventLoopDelayMonitor), DNS resolver, Subprocess timeout, GC controller, Valkey connection timeout, and the SQL timer_insert hook all use tags for which js_timer_flags_ptr returns None, so the new block is a no-op for them. The only new epoch consumer is AbortSignalTimeout — exactly the fix target. TimerObjectInternals (setTimeout/setInterval init, setInterval re-arm, refresh()) all go through update(), so they still get exactly one bump per schedule.
Other factors
The test follows harness conventions (spawn with -e, bunEnv, await using, concurrent pipe drain, awaits done.promise rather than sleeping) and cites the WPT case it mirrors. robobun confirmed it fails on main with [5,4,3,2,1,0,"t"]. The one nit — expect(stderr).toBe("") can flake on ASAN/debug lanes per REVIEW.md — is already flagged inline and is trivially fixable by dropping the line; the JSON.parse + exitCode assertions already prove correctness. Not blocking on it.
There was a problem hiding this comment.
LGTM — pure code motion of the epoch bump from All::update() into its callee All::insert(), so setTimeout/setInterval are provably unchanged and AbortSignal.timeout now gets the epoch it was missing.
What was reviewed:
- Enumerated every
All::insert()caller (dns, valkey, subprocess, GC, sql, DateHeaderTimer, EventLoopDelayMonitor,timer_inserthook) — all non-JS-timer tags returnNonefromjs_timer_flags_ptr, so no new epoch bumps outside the intendedAbortSignalTimeout. - Confirmed
TimeoutObject/ImmediateObjectreachinsert()only viaupdate()(reschedule()→update()), so still exactly one epoch bump per schedule/refresh — no double-bump. - Stacked-Borrows ordering in
update()still holds:timer_ref's last use precedes the raw-ptr reads insideinsert(). - Test: subprocess pipes drained concurrently, awaits the actual completion event, prior stderr-empty nit addressed in 1080b7c.
Extended reasoning...
Overview
Moves the global-epoch bump + per-timer set_epoch write from All::update() into All::insert() in src/runtime/timer/mod.rs. Since update()'s final step is already self.insert(timer), the setTimeout/setInterval path is byte-for-byte behavior-preserving (one bump per schedule/refresh). The only semantic change is that direct insert() callers whose tag matches js_timer_flags_ptr — which is exclusively AbortSignalTimeout among the direct-insert paths — now receive a fresh epoch and therefore sort in creation order among equal-deadline timers, matching WPT and Node. Adds a subprocess regression test in test/js/web/abort/abort.test.ts.
Security risks
None. No untrusted-input parsing, no allocation sizing, no auth/crypto surface. The change is a 6-line block moved between two functions in the same file.
Level of scrutiny
Timer heap ordering is core runtime code, so I traced every insert() call site: update() (behavior-preserving), DateHeaderTimer::run/enable, EventLoopDelayMonitor::enable/on_fire, and via the timer_insert hook AbortSignal::Timeout, GarbageCollectionController, JSValkeyClient, Subprocess, DNS, and SQL. js_timer_flags_ptr returns Some only for TimeoutObject / ImmediateObject / AbortSignalTimeout; every other caller falls through to None and is unaffected. TimeoutObject/ImmediateObject never call insert() directly (only via update() in reschedule()), so there is no double-bump path. The epoch write happens before the heap insert in both fake- and real-timer branches, matching where EventLoopTimer::less reads it.
Other factors
The Stacked-Borrows note that was on the deleted block ("timer_ref's last use is above so the raw (*timer).tag read inside is SB-clean") still holds after the move — timer_ref.next.nsec = ... remains the last use before self.insert(timer) performs the same raw reads. The prior review comment about expect(stderr).toBe("") was addressed in 1080b7c; stderr is still drained concurrently but no longer asserted empty. The test awaits an actual completion signal (Promise.withResolvers counted down by each callback) rather than sleeping, and drains stdout/stderr/exited concurrently per REVIEW.md.
2 participants
What does this PR do?
AbortSignal.timeout(ms)signals created with the same deadline were firing theirabortevents in (near-)reverse creation order, whilesetTimeoutwith the same delay fires in creation order in the same process. Node and browsers fire both in creation order; WPT covers this indom/abort/timeout.any.js("AbortSignal timeouts fire in order").Repro
Cause
Equal-deadline JS timers are ordered in the pairing heap by a monotonically increasing
epochstored in the timer'sTimerFlags.setTimeout/setIntervalschedule viaAll::update(), which bumped the global epoch and wrote it into the timer before the heap insert.AbortSignal.timeoutschedules viaAll::insert()directly (through thetimer_insertruntime hook), which did not bump the epoch, so every signal timer kept the default epoch0. With all epochs equal,EventLoopTimer::less(a, b)evaluates the tiebreak as(0 - 0) < U25_MAX/2 == truefor every pair, so each newly inserted node becomes the heap root: LIFO order on pop.Fix
Move the epoch bump from
All::update()intoAll::insert().update()already callsinsert()as its final step, so thesetTimeout/setIntervalpath is unchanged (one bump per schedule/refresh, same as before).AbortSignal.timeoutnow gets a fresh epoch on its directinsert()call and sorts alongsidesetTimeoutin creation order. Non-JS-timer tags (SubprocessTimeout,ValkeyConnectionTimeout, GC timers, etc.) are unaffected becausejs_timer_flags_ptrreturnsNonefor them.Verification
bun bd test test/js/web/abort/abort.test.tsNew test fails on
mainwith[5, 4, 3, 2, 1, 0, "t"]and passes with this change.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file