Walk the timer pairing heap without recursion

[robobun]

Problem

• jest.getTimerCount() and jest.runOnlyPendingTimers() die with a silent Segmentation fault (exit 139, no crash banner) once a few hundred thousand fake timers are pending. An ASAN build reports AddressSanitizer: stack-overflow in Intrusive::count_internal (src/io/heap.rs:94) and Intrusive::find_max_internal (src/io/heap.rs:124).
• Both walkers recursed once per node. Timers inserted with decreasing deadlines build one chain of child links in the pairing heap, so the recursion depth equals the timer count. With increasing deadlines the chain runs along next links instead. The release build overflows the 8 MB main stack at about 240k timers.

Fix

• count() and find_max() now share one iterative depth-first walk, Intrusive::for_each. It descends through child and next, and climbs back through prev links. The walk uses constant stack space.
• The climb is correct because every non-root node's prev points at its left sibling, or at its parent when it is the leftmost child, and meld, combine_siblings, and remove keep that invariant. So prev.child == node tells the walk that it reached the parent.
• Verified: test/js/bun/test/fake-timers/fake-timers.test.ts (new test: 60k decreasing timers under a 1 MB stack, stock bun segfaults). Also test/js/bun/test/test-timers.test.ts, test/js/node/timers, and the sinonjs fake-timers port.
• Overlaps timer: remove unsafe from the timer module #40187, which rewrites the same heap.rs region with a Vec-stack for_each. Either walk fixes the crash. This PR is the small step: if timer: remove unsafe from the timer module #40187 lands first, this one reduces to its test. If this lands first, timer: remove unsafe from the timer module #40187 takes its own for_each on rebase and keeps the test.

Background

• src/io/heap.rs is an intrusive pairing heap. Each node embeds an IntrusiveField with child, next, and prev pointers. child is the leftmost child, next the right sibling, prev the left sibling or (for the leftmost child) the parent.
• The fake-timer clock in src/runtime/test_runner/timers/FakeTimers.rs keeps pending timers in this heap. getTimerCount calls count(). runOnlyPendingTimers calls find_max() to find the deadline it must run up to.
• The real event-loop timer heap (src/runtime/timer/mod.rs) is the same type, so it gets the same walk.

Notes

• Stock bun (1.4.3-canary f42e980) with the ledger repro: 1e6 decreasing timers, getTimerCount() → SIGSEGV. runOnlyPendingTimers() with 300k decreasing timers → SIGSEGV. Increasing order survives on the release build because the next leg of the old recursion is a tail position that the optimizer turned into a loop.
• Release overflow thresholds measured with ulimit -s: at 512 KB between 14k and 16k timers (about 35 bytes per frame). The debug ASAN build overflows between 100k and 200k timers at 8 MB.
• The test spawns through sh -c 'ulimit -s 1024 && exec ...' so that 60k timers are enough. 60k timers take about 8 s on the debug ASAN build, almost all of it in setTimeout creation and firing. A 256 KB stack is too small for the debug build to boot the VM.
• insert, meld, delete_min, remove, and combine_siblings are already loop based. Nothing else in the file recurses.
• Self-reviewed: 9 concerns raised. The one that survived is the overlap with timer: remove unsafe from the timer module #40187, addressed by the cross-reference above. Rejected: making for_each public and retargeting the two teardown walks in src/runtime/timer/mod.rs onto it (those walks belong to timer: remove unsafe from the timer module #40187's refactor, and this PR stays a crash fix).
• The walker was checked in a throwaway crate against a brute-force expectation after every insert, every third remove, and ten delete_min calls, in decreasing, increasing, and scrambled insert orders, plus a 2M-node chain. That test cannot live in bun_io because the crate does not link standalone under cargo test.

---

no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/test/fake-timers/fake-timers.test.ts

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{On-demand reviews are free for the next 14 days. After that, they cost $0.25 per reviewed file.}

Or wait 2 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 57b2e4dd-759e-41f8-a273-00fc908cd5d3

📥 Commits

Reviewing files that changed from the base of the PR and between d316760 and 2809034.

📒 Files selected for processing (2)

• src/io/heap.rs
• test/js/bun/test/fake-timers/fake-timers.test.ts

---

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

[robobun]

^{Updated 10:33 AM PT - Sep 6th, 2026}

❌ @robobun, your commit 2809034 has 2 failures in Build #111382 (All Failures):

• test/js/node/test/parallel/test-crypto-dh-leak.js - code 1 on 🐧 13 x64-asan
• ❌ CPU instruction violation on Linux x64 — 1 check(s) failed

The baseline build contains instructions not available on Nehalem (SSE4.2, no AVX/AVX2/AVX512).

• Static instruction scan

Static scan violations

llint_op_wide16  [INVLPGB]  (1 insns)
If these are runtime-dispatched behind a CPUID gate: add each symbol to
scripts/verify-baseline-static/allowlist-x64.txt with a comment pointing at the gate.
If there's no gate: this is a real bug — a -march leaked into a subbuild.
❌ CPU instruction violation on Linux x64 — 1 check(s) failed
The baseline build contains instructions not available on Nehalem (SSE4.2, no AVX/AVX2/AVX512).
Static instruction scan
Static scan violations
llint_op_wide16  [INVLPGB]  (1 insns)
If these are runtime-dispatched behind a CPUID gate: add each symbol to
scripts/verify-baseline-static/allowlist-x64.txt with a comment pointing at the gate.
If there's no gate: this is a real bug — a -march leaked into a subbuild.

---

🧪   To try this PR locally:

bunx bun-pr 41584

That installs a local version of the PR into your bun-41584 executable, so you can run:

bun-41584 --bun

[robobun]

Status

Reproduced on bun 1.4.3-canary (f42e980): 1e6 fake timers with decreasing deadlines, then jest.getTimerCount(), exits 139 with no crash banner. jest.runOnlyPendingTimers() with 300k such timers does the same. Both walks in src/io/heap.rs recursed once per node.

Fix: one iterative walk shared by count() and find_max(). The new test in test/js/bun/test/fake-timers/fake-timers.test.ts fails on stock bun and passes with this branch.

Reviewed: this PR should stay open as the small crash fix. It overlaps #40187 on the same heap.rs hunk, and the body says how the two combine.

CI: the diff is green. The new test passes on every lane in builds 111095, 111372, and 111382. The remaining red lanes are unrelated to this change and also fail on main: x64 and x64-musl verify-baseline (the static instruction scan flags llint_op_wide16, same as main build 110907) and test-crypto-dh-leak.js on x64-asan. The other failures passed on retry. Ready for a maintainer.

[claude]

I reviewed this and didn't find any bugs. The iterative walk relies on the same prev invariant that remove() already depends on (line 188's (*prev).heap().child == v check), and I traced it against single-node, sibling-chain, child-chain, and mixed shapes — each node is visited exactly once and the null-root early return is preserved. Because this is an unsafe-Rust rewrite of a traversal that also backs the real event-loop timer heap, a human look at the prev-climb reasoning would still be worthwhile.

What was reviewed

• for_each correctness: verified the prev.child == node parent-detection matches the invariant meld/remove/combine_siblings maintain, and that root.prev == null / root.next == null hold on every mutation path so the climb terminates without missing nodes.
• find_max semantics: null root still returns null (for_each early-returns before the closure runs); the redundant less(root, root) on the first visit matches the old code's first call.
• Dead code: count_internal/find_max_internal have no remaining references.
• Test: sh -c 'ulimit -s … && exec "$0" -e "$1"' avoids shell-escaping the script, pipes are drained concurrently, stdout is asserted before exitCode, and the Windows skip is scoped to the POSIX-only ulimit.

Extended reasoning...

Overview

The PR replaces two recursive helpers in the intrusive pairing heap (src/io/heap.rs) — count_internal and find_max_internal — with a single iterative for_each that walks the tree in constant stack space by descending through child, moving across next, and climbing back via prev until prev.child == node identifies the parent. count() and find_max() now both delegate to it. A regression test in fake-timers.test.ts spawns a child under ulimit -s 1024, inserts 60k decreasing-deadline fake timers, and asserts getTimerCount() and runOnlyPendingTimers() complete with correct counts.

Security risks

None. This is an internal data-structure traversal with no user-controlled input beyond timer count; the change removes an unbounded-recursion DoS vector rather than adding attack surface. No parsing, no allocation-size arithmetic, no boundary crossings.

Level of scrutiny

High. This is unsafe Rust in a shared data structure used not only by fake timers but by the real event-loop timer heap (src/runtime/timer/mod.rs). A traversal that skips or double-visits nodes would corrupt getTimerCount and runOnlyPendingTimers semantics for every consumer. I walked the algorithm against the invariants the existing meld/combine_siblings/remove code maintains: every non-root node's prev points either to its left sibling (where prev.child != node) or to its parent (where prev.child == node) — the exact discriminant remove() already uses at line 188 — and the root's prev and next are always null across insert/delete_min/remove. Under those invariants the walk visits each node exactly once and terminates at the root. The find_max null-root path and the redundant less(root, root) first comparison both match the old code's behavior.

Other factors

The test follows harness conventions (bunExe/bunEnv, Promise.all drain, stdout-before-exitCode assertion order, narrow test.skipIf(isWindows) for the POSIX-only ulimit), and the "$0" -e "$1" argv-passing pattern sidesteps shell-escaping the inline script. The 60s per-test timeout is a generous ceiling for an ~8s debug-ASAN run per the PR notes. The old recursive helpers are fully deleted with no stragglers. The PR author reports out-of-tree brute-force verification across insert/remove/delete_min sequences and a 2M-node chain, which the in-tree test cannot host because bun_io doesn't link under cargo test. Given the unsafe rewrite sits on the real timer path, a maintainer sign-off on the prev-chain reasoning is the right bar even though I found no defects.

[claude]

Code review found no issues

No high-confidence issues detected in this change.