bun:test: keep built-in modules' own timers out of fake timers (internal/timers) - #37987
bun:test: keep built-in modules' own timers out of fake timers (internal/timers)#37987robobun wants to merge 2 commits into
Conversation
Add internal/timers setTimeout/setInterval/clearTimeout/clearInterval, backed by TimeoutObjects tagged InternalTimeoutObject: the same Timeout objects the globals create, but never routed into the fake timer heap and always armed from the real clock (Tag::clock). Switch the built-in modules that schedule deadlines for themselves (net, _http_server, http2, child_process, sql, quic, fast-utf8-stream, cluster, readline, repl history, node:test) to them, and add a source lint that rejects bare uses of the timer globals in src/js.
|
Warning Review limit reached
Next review available in: 10 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 (28)
Comment |
|
Updated 12:48 AM PT - Aug 13th, 2026
✅ @robobun, your commit 95e77116f6e4b34490e5721d12d5cc6d4811827f passed in 🧪 To try this PR locally: bunx bun-pr 37987That installs a local version of the PR into your bun-37987 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a cross-cutting change to the timer subsystem — a new EventLoopTimerTag variant threaded through Rust dispatch/refcount/heap paths, a C++ NodeTimers.cpp refactor, and ~12 built-in JS modules switched to internal/timers — a human look would still be worthwhile.
Checked that every tag-dispatch site (__bun_fire_timer, js_timer_flags_ptr, cancel_all_timeout_objects, remove_timer_by_id) accepts InternalTimeoutObject, and that FakeTimers::clear correctly needs no arm since allow_fake_timers() keeps that tag out of the fake heap.
Traced init_with → internals.init() → reschedule() → self.clock(): the tag is written to event_loop_timer before reschedule() reads it, so internal timers arm from the real clock on first insert.
Verified the scheduleTimer() extraction in NodeTimers.cpp preserves the argument-packing / error paths and adds RELEASE_AND_RETURN on the tail call.
Confirmed the new debug_assert!(kind != Kind::SetImmediate) in TimeoutObject::init holds for all callers (sleep, set_timeout_or_interval).
Extended reasoning...
Overview
This PR keeps timers scheduled by Bun's built-in JS modules (net, http, child_process, http2, sql, etc.) out of jest.useFakeTimers(). It adds a second EventLoopTimerTag::InternalTimeoutObject variant for the same TimeoutObject container, exposes native setTimeout/setInterval/clearTimeout/clearInterval bindings via internal/timers, and updates ~12 built-in modules to destructure those instead of using the globals. Supporting changes: Tag::clock() derives the arming clock from allow_fake_timers(); impl_timer_object! drops its Default impl so the tag is passed at construction; NodeTimers.cpp factors the shared setTimeout/setInterval argument packing into scheduleTimer(); two new Rust host-exports (Bun__Timer__set{Timeout,Interval}Internal); a source-lint test that bans bare timer globals in src/js/; 9 new fake-timers integration tests; and a docs paragraph.
Security risks
None identified. The new host functions are only reachable via $newCppFunction from bundled built-in modules, not user code. No untrusted-input parsing, auth, or crypto paths are touched.
Level of scrutiny
High. This touches the timer heap's tag-dispatch invariant (tag → container type recovered via container_of), which is memory-safety-load-bearing: a missed match arm on the new tag would UAF or hit an unreachable!. It also refactors refcounted-object construction (init_with now takes tag), rewrites two hot C++ host functions into a shared helper, and changes user-observable bun:test behavior across a dozen node-compat modules. 28 files across three languages is well past the "simple/mechanical" bar.
Other factors
The change looks correct and well-motivated: it follows #37946's precedent for native timer owners, the tag-dispatch audit is complete (every site that matched TimeoutObject now accepts both tags; FakeTimers::clear is correctly excluded because the new tag never enters the fake heap and its fallback is a debug_assert!). The clock() derivation ties the arming clock to the heap the tag routes to, avoiding the "real-heap timer armed from mocked clock fires immediately" hazard the PR calls out. Test coverage is solid (9 integration tests that time out on main, plus a source-lint that prevents regressions). Still, the breadth (native timer plumbing + FFI + macro refactor + behavioral change) and the fact that a missed dispatch arm here is a memory-safety bug make this a case where a maintainer should sign off.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// A `TimeoutObject` scheduled by a built-in JS module through | ||
| /// `internal/timers` (socket idle timeouts, `child_process` kill timers, | ||
| /// ...): the same container as `TimeoutObject`, but a runtime-internal | ||
| /// timeout as far as `allow_fake_timers` is concerned. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// The clock an owner with this tag arms with (the rule stated on | ||
| /// [`Self::allow_fake_timers`]), for owners that exist under more than one | ||
| /// tag: a real-heap timer armed from the mocked clock is due immediately, | ||
| /// and re-arms due immediately. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The timers built-in modules schedule their own deadlines with; unlike the | ||
| // globals they are not touched by jest.useFakeTimers(). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Timers for the runtime's own deadlines (socket idle timeouts, listen() | ||
| // callbacks, child_process kill timers, ...). The globals belong to user code: | ||
| // jest.useFakeTimers() freezes, counts, advances and clears every timer created | ||
| // through them, and user code may replace them outright. These create the same | ||
| // Timeout objects but never take part in fake timers, like the private timer | ||
| // references Node's lib/ uses. Built-in modules take all four from here | ||
| // (test/internal/source-lints/builtin-timer-globals.test.ts); the global | ||
| // clearTimeout would clear these too, the private one just stays out of reach | ||
| // of replaced globals. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // setTimeout(callback, delay, ...args) / setInterval(callback, delay, ...args). | ||
| // The extra arguments are packed the way Bun__JSTimeout__call (NodeTimerObject.cpp) | ||
| // unpacks them: undefined for none, the value itself for one, a JSCellButterfly | ||
| // for several. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The setTimeout/setInterval that built-in JS modules schedule their own | ||
| // deadlines with (src/js/internal/timers.ts). Same arguments and same Timeout | ||
| // object as the globals, but the timer is never handed to bun:test's fake | ||
| // timers, so socket timeouts, listen() callbacks and the like keep working | ||
| // while a test has jest.useFakeTimers() active. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// `tag` is `TimeoutObject` for the global `setTimeout`/`setInterval`/ | ||
| /// `Bun.sleep` (subject to `jest.useFakeTimers()`) or | ||
| /// `InternalTimeoutObject` for `internal/timers` (always real time); see | ||
| /// [`EventLoopTimerTag::allow_fake_timers`]. Anything else is a bug. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Body of `setTimeout`/`setInterval`; `tag` picks between the global | ||
| /// (fakeable) timers and the `internal/timers` ones, see | ||
| /// [`TimeoutObject::init`]. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// `internal/timers` `setTimeout`: a timer owned by a built-in JS module. | ||
| /// Not affected by `jest.useFakeTimers()` (not counted, advanced or | ||
| /// cleared by it), like the deadlines inside Node's `lib/`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// picks `tag`/`kind`/`interval` and forwards here. The node keeps | ||
| /// `tag` for its whole life (`js_timer_flags_ptr` and the fire | ||
| /// dispatch recover the container from it). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// [`TimerFlags`] slot for the three JS-timer container types | ||
| /// (`TimeoutObject`, under either of its tags / `ImmediateObject` / | ||
| /// `AbortSignalTimeout`), else `None`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Clock the deadlines of this timer are computed from: the one driving | ||
| /// the heap its tag routes it to (`EventLoopTimerTag::clock`), so an | ||
| /// `internal/timers` timer stays on real time under `jest.useFakeTimers()`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It's a carefully constructed change, but it touches core timer infrastructure (a new EventLoopTimerTag variant for the same TimeoutObject container, updates to every tag-dispatch site in dispatch.rs/mod.rs/timer_object_internals.rs, and the impl_timer_object! construction path) plus a dozen built-in modules, so a human look at the approach is warranted. There are also ~11 outstanding comment-cop notes about comment length that haven't been addressed.
What was reviewed:
- Tag-dispatch coverage:
__bun_fire_timer,js_timer_flags_ptr,cancel_all_timeout_objects,remove_timer_by_id, andFakeTimers::clearall handle the new tag (or correctly don't need to). Tag::clock()ties the arming clock to the heap the tag routes to;fire()'s setInterval re-arm andreschedule()both use it, so an internal timer under fake timers doesn't spin onAllowMockedTime.- The
scheduleTimerrefactor in NodeTimers.cpp preserves the old argument-packing and error paths;RELEASE_AND_RETURNon the tail call is correct. - The per-module
require("internal/timers")destructures shadow the globals module-wide, and the new source-lint enforces this going forward.
Extended reasoning...
Overview
This PR keeps timers scheduled by Bun's built-in JS modules (net, http, child_process, http2, sql, etc.) out of jest.useFakeTimers()'s fake heap. It does so by introducing a second EventLoopTimerTag::InternalTimeoutObject for the existing TimeoutObject container, wiring new C++ host functions (functionSetTimeoutInternal/functionSetIntervalInternal) and Rust exports (Bun__Timer__setTimeoutInternal/setIntervalInternal) through to internal/timers.ts, and having 12 built-in modules destructure their timer functions from there instead of using the globals. It also refactors NodeTimers.cpp to share argument packing across the four setTimeout/setInterval variants, replaces the impl_timer_object! Default impl with a tag parameter on init_with, adds a Tag::clock() helper so re-arm/refresh compute deadlines against the correct clock, and updates every tag-dispatch site (__bun_fire_timer, js_timer_flags_ptr, cancel_all_timeout_objects, remove_timer_by_id) to accept both tags. A new source-lint test and 9 integration tests are included.
Security risks
None identified. The new internal timer functions are only reachable via $newCppFunction from bundled built-in modules; user code cannot call them. No untrusted input parsing, auth, or crypto is touched.
Level of scrutiny
High. The timer subsystem is core runtime infrastructure with unsafe Rust throughout: intrusive pairing-heap nodes, tag-keyed container_of recovery in the fire dispatch, intrusive refcounts, and re-entrancy under JS callbacks. Adding a second tag for the same container type means every existing match tag { TimeoutObject => ... } site had to be audited — a missed one is a use-after-free or a wrong-container cast. The PR appears to have covered them consistently (dispatch, flags-ptr, teardown, id-map removal), and the clock() helper correctly ties the arming clock to the heap the tag routes to, but this is exactly the kind of change where a maintainer should confirm the approach (second tag vs. a flag on TimerObjectInternals) and that no dispatch site was missed.
Other factors
- The
github-actionscomment-cop bot left ~11 inline comments flagging paragraph-length code comments across the new/changed files. These are style-only, but they're outstanding automated feedback the author hasn't responded to. - CI (#93915) was still building at review time.
- The change is ~630 insertions / ~120 deletions across 28 files — well above the "simple, mechanical" threshold for auto-approval.
- Test coverage is strong (9 new integration tests exercising net/http/child_process under fake timers, plus a source-lint that fails on any future regression), and the PR description documents extensive local test runs against the affected modules.
2 participants
Problem
jest.useFakeTimers()is active, deadlines that Bun's built-in JS modules schedule for themselves never fire:net.Socket#setTimeout()/httprequest and server timeouts, thetimeoutoption ofchild_process.exec()/execFile()/spawn(),http.Server's headers/request timeout sweep, http2 session timeouts,sql.close({ timeout }), SQL connect retries, QUIC idle timeouts, the readline escape-sequence timeout, and (until node:net, node:http: emit listen() results on nextTick so fake timers do not freeze listen() #37959) thelisten()callback itself. They also show up injest.getTimerCount(), are fired early byjest.runAllTimers()/advanceTimersByTime(), and are cancelled byjest.clearAllTimers()/useRealTimers().setTimeout/setInterval. Fake timers are implemented inside the nativesetTimeout(timer::All::insertroutes everyTimeoutObjectinto the fake heap), so a timer created bysrc/js/node/net.tsis indistinguishable from one created by the test. For the same reason any user code that replacesglobalThis.setTimeout(sinon-style fake timers) breaks these modules today.lib/holds private references to the real timer functions (its eslint config bans the timer globals), and fake timers only replace the globals. bun:test: keep runtime-internal timeouts out of the fake timer heap #37946 gave the native timer owners (Bun.spawn, SQL/Valkey connection timeouts, c-ares, ...) the same treatment; this PR covers the JS-side owners, which bun:test: keep runtime-internal timeouts out of the fake timer heap #37946 and node:net, node:http: emit listen() results on nextTick so fake timers do not freeze listen() #37959 both note as the remaining gap.Fix
src/js/internal/timers.tsnow exportssetTimeout/setInterval/clearTimeout/clearIntervalbound to native functions that user code cannot reach or replace. They create the sameTimeoutobjects as the globals (ref/unref/refresh/clearTimeoutall work, extra arguments are passed through), but the node is taggedEventLoopTimerTag::InternalTimeoutObject.InternalTimeoutObjectis a second tag for theTimeoutObjectcontainer:allow_fake_timers()is false for it, soAll::insertkeeps it in the real heap, andTimerObjectInternals::reschedule()/ thesetIntervalre-arm infire()now take their clock from the tag (Tag::clock(): mocked clock for fakeable tags, real clock otherwise). That keeps the clock a deadline is computed from tied to the heap it is inserted into; a real-heap timer armed from the mocked clock would be due immediately and re-arm due immediately. The tag dispatch sites (__bun_fire_timer,js_timer_flags_ptr,cancel_all_timeout_objects,remove_timer_by_id) accept both tags;FakeTimers::clearneeds no arm because the tag never reaches the fake heap.NodeTimers.cpp: the argument packing shared by the globalsetTimeout/setIntervalis factored intoscheduleTimer()and reused byfunctionSetTimeoutInternal/functionSetIntervalInternal, which call the newBun__Timer__setTimeoutInternal/setIntervalInternalexports. Theimpl_timer_object!Defaultimpl is replaced by passing the tag toinit_with, so everyTimeoutObject/ImmediateObjectstates its tag at construction.internal/timers(one destructure per module, which shadows the globals for the whole module):net,_http_server,http2,child_process,bun:sql+internal/sql/shared,internal/http(Date header cache),internal/quic,internal/streams/fast-utf8-stream,internal/cluster/child,internal/readline/emitKeypressEvents,internal/repl/history, andnode:test's runner timeouts. The user-facingnode:timers,timers/promises,Bun.sleep()andAbortSignal.timeout()are unchanged and still faked.test/internal/source-lints/builtin-timer-globals.test.tsrejects any bare use of the four globals insrc/js(outside the two modules implementing them), so new call sites cannot reintroduce the problem. On main it reports exactly the 68 uses this PR converts.built-in modules are not affected by fake timersblock intest/js/bun/test/fake-timers/fake-timers.test.ts(9 cases: the primitive itself,listen(),socket.setTimeout()survivingclearAllTimers()/useRealTimers(),request.setTimeout(), theheadersTimeoutsweep interval,execFile({ timeout }), andlisten()withglobalThis.setTimeoutreplaced); all 9 fail on main (getTimerCount()is off by one within milliseconds, or the awaited event never comes) and pass with the fix, as does the lint.fake-timers.test.ts(including the bun:test: keep runtime-internal timeouts out of the fake timer heap #37946 cases),test-timers.test.ts,test/js/node/timers,test/js/web/timers,test/internal/source-lints,node-http-server-timeouts,client-timeout-error,node-net,node-net-server,child_process,child-process-exec,sql-connect-error-reporting(retry timer),sql-close-pending-connection, and 31 upstreamtest-{child-process-*-timeout,fastutf8stream-periodicflush,http-*-timeout,http2-*-timeout,net-*timeout,net-autoselectfamily*,cluster-rr-*,readline-keys,repl-*history*}files. The only failures are ones main also has in this container (tests listening onlocalhostwhile connecting to127.0.0.1, leak fixtures keyed on abun-asanbinary name, 500 ms budgets on a debug build).listen()emits toprocess.nextTick, which is independent of this change (different lines; once it lands those sites simply stop being timers, and the rest ofnet.ts/_http_server.tsstill needs this).Background
bun:testare native:jest.useFakeTimers()flips a flag intimer::All, after whichinsert()puts every node whoseTag::allow_fake_timers()is true into a second heap (fake_timers.timers) that onlyadvanceTimersByTime()and friends drain, and makesTimespec::now(AllowMockedTime)return the mocked clock. The real heap is drained againstTimespec::now(ForceRealTime).EventLoopTimeris the intrusive heap node embedded in every timer owner; itsTagsays which owner type embeds it, and the firing code recovers the owner from the node with that tag (__bun_fire_timerinsrc/runtime/dispatch.rs).TimeoutObjectis the owner behindsetTimeout/setInterval/Bun.sleep(); it previously had exactly one tag.src/js/) are bundled into the binary and run against the same globals as user code;$newCppFunction("file.cpp", "symbol", length)gives such a module a JSFunction wrapping a C++ host function, resolved when the module is loaded, which is howinternal/timersgets references user code cannot observe or replace.[review] gate passed · iteration 0 · 28 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file