event_loop: yield mid-tick thread-pool re-drain to due timers and pending setImmediate by robobun · Pull Request #36479 · oven-sh/bun

robobun · 2026-07-30T18:06:11Z

Repro

const crypto = require('crypto');
let order = '';
setInterval(() => { order += 'T'; }, 1).unref();
setTimeout(() => {
  (function go(i) {
    if (i >= 20) { console.log(order); process.exit(0); }
    crypto.pbkdf2('a', 'b', 1, 8, 'sha256', () => { order += 'R'; go(i + 1); });
    const s = Date.now(); while (Date.now() - s < 3);
  })(0);
}, 5);

node (linux+windows): TTRTRTRTRTRTRTRTRTRTRTRTRTRTRTRTRTRTRTRTR
bun  (linux+windows): TTTTRRRRRRRRRRRRRRRRRRRR

No interval fires between the 20 thread-pool callbacks even though every one of them is overdue. Same shape with setImmediate queued from the first callback: Node runs it before the second callback, Bun runs the whole chain first. The same starvation shows with fs.read on POSIX (on Windows fs.read completes via libuv's own callback and is unaffected).

Cause

EventLoop::tick() re-drains concurrent_tasks (thread-pool completions) inside its while tick_with_count() > 0 loop (src/jsc/event_loop.rs). When a callback submits the next job and the worker thread finishes before the loop cycles, tick_concurrent() promotes the completion and tick_with_count runs it in the same tick(). The chain runs to completion before tick() returns, and auto_tick* (which runs immediates, polls, and drains timers) is never reached.

libuv delivers thread-pool completions once per poll (uv_async_t) and every poll is followed by check and then the timers phase, so Node interleaves. The inner-loop tickConcurrent() dates to 0ce709d96a ("Make new HTTP client more stable", 2022) with no documented invariant; before that it was once per outer iteration.

This is the same mechanism #36314 addresses for MessagePort reschedules and #32807 addresses for setImmediate, but for due JS timers, and applied at the re-drain site rather than per producer.

Fix

Gate the three mid-tick tick_concurrent() calls via tick_concurrent_unless_due():

return early when immediate_tasks is non-empty (same-thread Vec, checked unconditionally so a cross-thread push landing between the concurrent_tasks peek and pop_batch() cannot slip ahead of a pending setImmediate);
return early when concurrent_tasks is non-empty and a JS timer is already due. The concurrent_tasks guard keeps the clock read off the path when nothing arrived (an HTTP server always has DateHeaderTimer armed).

The unconditional tick_concurrent() at the start of tick() stays as the once-per-iteration batch boundary (one poll's worth of completions, as in libuv).

The due-timer probe (timer::All::has_due_regular_timer, reached via a link-time extern "Rust" because the heap lives in bun_runtime) peeks the regular heap and reads the clock only when it is non-empty. tick_with_count drains the whole tasks FIFO in one call, so the probe runs once per thread-pool completion batch, not per task. WTF timers are not checked (would need the wtf_timers lock every iteration; they are at most one loop iteration away once tick() returns).

Applies on both platforms: on Windows on_uv_timer fires inside uv_run in auto_tick*, which is only reached when tick() returns. tick_tasks_only (spawnSync's isolated loop) is unchanged since that context does not run timers.

Verification

Two new tests in test/js/node/timers/node-timers.test.ts use crypto.pbkdf2 (its completion goes through enqueue_task_concurrent on every platform; fs.read on Windows does not):

chained thread-pool callbacks yield to due timers: asserts the longest run of consecutive callbacks between interval firings is at most 3. Fails on main with maxRun:20, passes with the fix (maxRun:1, same as Node). The longest-run metric stays robust under CPU contention, where the unfixed build's bursts stay at 9+ while the fixed build stays at 1.
chained thread-pool callbacks yield to pending setImmediate: asserts order starts RI. Fails on main with RR, passes with the fix.

Both tests fail on the Windows canary build (maxRun:20, RR) and match Node there once the fix applies.

Regression sweep on the debug build: node-timers.test.ts 22/22, worker_threads.test.ts 91/91, node parallel test-timers-* / test-fs-read-stream-pos.js all pass. The setInterval/setTimeout leak tests in test/js/web/timers and the fetch.test.ts connect failures also fail on main in this container.

Surfaced by test-fs-read-stream-pos.js (#36478): after #34834 raised Windows timer resolution to ~1ms, each ReadStream instance's pread chain ran ahead of the 1ms appender and the test's exit-path race rarely hit.

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/node/timers/node-timers.test.ts

…diates EventLoop::tick() re-drained concurrent_tasks (thread-pool completions) inside its inner drain loop, so a chain of fs.read callbacks that each submit the next read ran to completion inside one tick() and due setInterval/setTimeout/setImmediate never fired until the chain stopped. Node/libuv delivers thread-pool completions once per poll and runs the timers/check phases between polls, so timers interleave. Gate the three mid-tick tick_concurrent() calls on 'no setImmediate pending and no JS timer due'. The initial tick_concurrent() at the start of tick() stays as the once-per-iteration batch boundary. The due-timer probe peeks the regular heap and reads the clock only when it is non-empty, reached via a link-time extern into bun_runtime (the timer heap lives there).

coderabbitai · 2026-07-30T18:08:50Z

Walkthrough

Changes

The event loop probes due regular timers before selected concurrent-task drains. Runtime dispatch exposes the timer check, concurrent maintenance is separated from task popping, and Node-style integration tests verify interval and setImmediate callback ordering.

Timer-aware event-loop scheduling

Layer / File(s)	Summary
Due regular timer probe `src/runtime/timer/mod.rs`, `src/runtime/dispatch.rs`	Adds a regular-timer deadline check and exports it through `__bun_has_due_timer()`.
Conditional concurrent task draining `src/jsc/event_loop.rs`	Separates concurrent-task maintenance from popping and gates selected drains on pending immediate tasks and due regular timers.
Timer and immediate ordering coverage `test/js/node/timers/node-timers.test.ts`	Tests interval interleaving and `setImmediate` ordering during chained `pbkdf2` callbacks.

Possibly related PRs

oven-sh/bun#35891: Adds related due-timer probing for event-loop scheduling.
oven-sh/bun#36214: Also changes the concurrent-task draining path in the event loop.
oven-sh/bun#36314: Also adjusts event-loop scheduling around timer and worker work.

Suggested reviewers: jarred-sumner, cirospaciari

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title is concise and accurately summarizes the main event-loop yielding change.
Description check	✅ Passed	The description covers the change and verification, including repro, fix, and test results, matching the template's intent.

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

github-actions · 2026-07-30T18:14:52Z

This PR may be a duplicate of:

event loop: run queued immediates before newly completed async work #32807 - Both modify the same three tick_concurrent() call sites in EventLoop::tick() to prevent mid-tick re-drain starvation; event_loop: yield mid-tick thread-pool re-drain to due timers and pending setImmediate #36479 supersedes event loop: run queued immediates before newly completed async work #32807 by guarding on both pending immediates and due timers

🤖 Generated with Claude Code

robobun · 2026-07-30T18:15:31Z

The three tick_concurrent() call sites overlap with #32807, but the fixes address different symptoms: #32807 is for setImmediate ordering (and also adds auto_tick_startup for the post-entry-point GC window, which this PR does not touch); this PR adds the due-timer check so a chained fs.read yields to an overdue setInterval/setTimeout. The immediate_tasks half of the gate here is a subset of #32807's event_loop.rs change; whichever lands second will need a trivial rebase of that helper.

claude

I didn't find any bugs, but this changes scheduling semantics inside EventLoop::tick() — the hottest and most correctness-sensitive path in the runtime — so it's worth a human look, particularly for the HTTP-throughput case that motivated the original inner-loop tickConcurrent() (0ce709d96a).

What was reviewed:

The immediate_tasks.is_empty() gate matches how enqueue_immediate_task populates the queue during tick(); next_immediate_tasks is only non-empty inside tick_immediate_tasks, so checking only immediate_tasks is correct.
__bun_has_due_timer null-checks timer_all() (can be null before init_runtime_state), and has_due_regular_timer uses ForceRealTime consistent with All::next(); &All and &mut EventLoop are disjoint allocations so no aliasing.
tick_tasks_only (spawnSync) and the leading unconditional tick_concurrent() are correctly left unchanged.
Tests spin-wait to construct the state deterministically rather than racing; assertions are minimal (between > 0, order.slice(0,2) === "RI") so they shouldn't flake under load.

Extended reasoning...

Overview

The PR gates the three mid-tick tick_concurrent() re-drains inside EventLoop::tick() on "no setImmediate pending and no JS timer already due", so a self-feeding chain of thread-pool completions (e.g. fs.read → callback → next fs.read) yields to the timers/check phase instead of running to completion inside a single tick(). It adds a link-time extern "Rust" probe (__bun_has_due_timer → timer::All::has_due_regular_timer) that peeks the regular timer heap and reads the monotonic clock only when the heap is non-empty. Two subprocess tests in node-timers.test.ts cover the setInterval and setImmediate interleaving cases.

Security risks

None. No user input parsing, no auth/crypto, no external I/O beyond reading process.execPath in the test fixture.

Level of scrutiny

High. EventLoop::tick() is the core scheduling loop for every async workload in Bun. The inner-loop tick_concurrent() being gated here was added in 0ce709d96a specifically to make the HTTP client more stable — the PR argues there's no documented invariant, but a maintainer should confirm that yielding mid-batch when a timer is due (e.g. the 1s DateHeaderTimer under an HTTP server, or GcRepeating) doesn't regress request throughput or reintroduce whatever instability that commit fixed. The probe also adds a clock_gettime per inner-loop iteration whenever any timer is scheduled (which is essentially always for a running HTTP server via DateHeaderTimer/GcRepeating); it's vDSO-cheap on Linux but worth a benchmark check.

Other factors

The implementation itself looks correct and well-scoped: the null-guard on timer_all(), the ForceRealTime clock mode matching All::next(), the deliberate exclusion of WTF timers (documented), leaving tick_tasks_only alone, and checking immediate_tasks (not next_immediate_tasks, which is only populated during tick_immediate_tasks) all check out. The tests use busy-spin to force the race deterministically and assert weak lower bounds, so flakiness risk is low. The PR description is thorough and cross-references #36314/#32807/#36478. No prior review comments to address.

Avoids a clock_gettime per inner-loop iteration of tick() when the timer heap is non-empty but nothing arrived from other threads (an HTTP server always has DateHeaderTimer armed). The yield decision only matters when there is something to re-drain.

claude

I didn't find bugs, but this changes the scheduling semantics of EventLoop::tick() — the runtime's hottest path — and overlaps with #32807, so it should get a human sign-off (the CodeRabbit suggestion of Jarred/Ciro seems right).

What was reviewed:

tick_concurrent_unless_due: the !concurrent_tasks.is_empty() guard correctly bounds the clock-read to arriving work, and falling through to tick_concurrent() when empty preserves the update_counts/signal-drain/imminent-GC side effects.
has_due_regular_timer uses ForceRealTime matching All::next/drain_timers, and peeks without heap mutation; __bun_has_due_timer's null check handles the pre-init_runtime_state window.
immediate_tasks (not next_immediate_tasks) is the right field to check — enqueue_immediate_task pushes there during tick().
tick_tasks_only (spawnSync) left on unconditional tick_concurrent() is deliberate and correct.

Extended reasoning...

Overview

Gates the three mid-tick tick_concurrent() re-drain sites in EventLoop::tick() on "no setImmediate pending and no JS timer due", so a chain of thread-pool completions (e.g. fs.read callbacks that each submit the next read) yields to auto_tick* for timers/check instead of running to completion inside one tick(). Adds All::has_due_regular_timer (heap peek + clock_gettime when non-empty) and a link-time __bun_has_due_timer extern following the existing __bun_run_wtf_timer pattern. Two subprocess tests verify interleaving matches Node.

Security risks

None. Pure scheduling; no new inputs, no untrusted-data parsing.

Level of scrutiny

High. EventLoop::tick() is the core dispatch loop — every request, timer, and I/O completion runs through it. The inner-loop tickConcurrent() this gates was added in 0ce709d96a ("Make new HTTP client more stable"); the PR says no invariant is documented, but someone who knows that history should confirm the gate doesn't reintroduce whatever instability it fixed. HTTP workloads keep timeouts in the heap, so the new clock_gettime will fire on many batches — a benchmark eye would help.

Other factors

Overlap with #32807: the PR explicitly notes the immediate_tasks half is a subset of that PR's change and whichever lands second needs a rebase. That's a merge-order decision for a maintainer.
The tests use 2ms busy-spins to construct the race deterministically (commented as such); they assert the weak invariant (between > 0, order.startsWith("RI")) rather than exact counts, which should be robust across CI speeds.
All comment-cop threads are resolved; no human review comments outstanding.

…oss-platform test coverage - Check immediate_tasks unconditionally (same-thread, no race) so a thread-pool push landing between concurrent_tasks.is_empty() and pop_batch() cannot be drained ahead of a pending setImmediate. Only the clock-read timer probe stays guarded on concurrent_tasks non-empty. - Switch the two tests from fs.read to crypto.pbkdf2: fs.read on Windows completes via libuv's callback into enqueue_task() (not the concurrent queue) so the original test never reached tick_concurrent_unless_due there. crypto.pbkdf2 goes through AnyTaskJob -> enqueue_task_concurrent on every platform. - Assert on the longest run of consecutive callbacks (<= 3) instead of total interleaves (>= 1). Under CPU contention the unfixed build breaks the chain occasionally so 'between > 0' could false-pass; the longest burst stays >= 9 there while the fixed build is always 1.

coderabbitai

Actionable comments posted: 2

🤖 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 `@src/jsc/event_loop.rs`:
- Around line 460-474: Update tick_concurrent_unless_due and the related call
sites so pending immediate tasks or due timers suppress only concurrent_tasks
popping, not the maintenance performed by tick_concurrent. Split or parameterize
tick_concurrent to always apply ref-count updates, POSIX signal draining, and
imminent GC handling, while conditionally skipping only the concurrent task
batch.

In `@test/js/node/timers/node-timers.test.ts`:
- Around line 250-254: Remove the historical behavior narrative comments around
the timer/thread-pool tests, including the noted ranges near the test at this
location. Retain only concise, durable rationale or the invariant being
asserted, relying on the test name and assertions for regression context.

🪄 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: 91606edc-cb01-4c98-9a07-06bd49deb2a2

📥 Commits

Reviewing files that changed from the base of the PR and between 95c0034 and 4e4bca9.

📒 Files selected for processing (4)

src/jsc/event_loop.rs
src/runtime/dispatch.rs
src/runtime/timer/mod.rs
test/js/node/timers/node-timers.test.ts

tick_concurrent_with_count is now tick_concurrent_maintenance (ref-count delta, POSIX signal drain, imminent-GC) plus tick_concurrent_pop_batch. tick_concurrent_unless_due always runs the maintenance and gates only the pop, so the yield path no longer defers those side-effects. Also trim the test comments to durable rationale.

claude

No bugs found, but this changes EventLoop::tick() scheduling semantics — the core hot path every async operation flows through — and overlaps with #32807's fix at the same three call sites. Worth a human look at the design choice and the merge order.

What was reviewed:

The tick_concurrent_with_count → maintenance/pop split preserves exact prior behavior; tick_concurrent() and tick_tasks_only() are unchanged.
immediate_tasks check is correct against the swap in tick_immediate_tasks (setImmediate pushes to immediate_tasks, which is what auto_tick reads).
__bun_has_due_timer null-guards timer_all() and matches the neighboring __bun_run_wtf_timer extern pattern; has_due_regular_timer uses ForceRealTime consistent with All::next.
The concurrent_tasks.is_empty() guard keeps the clock read off the no-new-work path; TOCTOU ordering (immediate check before the concurrent guard) is sound.

Extended reasoning...

Overview

This PR fixes timer/setImmediate starvation when a chain of thread-pool callbacks (e.g. crypto.pbkdf2, fs.read) each submit the next job fast enough that tick_concurrent() keeps re-promoting completions inside a single EventLoop::tick(), so auto_tick* (which drains timers and immediates) never runs. The fix gates the three mid-tick tick_concurrent() calls on whether a setImmediate is pending or a JS timer is already due.

Files touched: src/jsc/event_loop.rs (splits tick_concurrent_with_count into maintenance + pop, adds tick_concurrent_unless_due, swaps three call sites in tick()), src/runtime/dispatch.rs (link-time __bun_has_due_timer shim), src/runtime/timer/mod.rs (has_due_regular_timer heap peek), and two new subprocess tests in node-timers.test.ts.

Security risks

None. No user input parsing, no allocation sizing, no auth/crypto surface. The added extern is Rust-ABI link-time (no C FFI), and __bun_has_due_timer is read-only against the per-thread timer heap.

Level of scrutiny

High. EventLoop::tick() is the core scheduling loop; every async completion, microtask drain, and rejected-promise handler flows through it. Changes here can shift ordering for any workload that mixes thread-pool completions with timers, and can regress throughput (the added clock_gettime per re-drain when concurrent work is queued). The PR is careful about this — the clock read is guarded on !concurrent_tasks.is_empty(), and the maintenance/pop split (added after CodeRabbit feedback) ensures signal draining and GC-timer firing are never deferred — but the blast radius of a mistake here is the entire runtime.

Other factors

Overlap with #32807: Both PRs modify the same three tick_concurrent() call sites. The author acknowledged this; whichever lands second needs a rebase, and a maintainer should decide merge order.
Design placement: This fixes the starvation at the re-drain site rather than per-producer (#36314's approach for MessagePort). That's a reasonable choice and the PR description defends it, but it's the kind of architectural placement a maintainer should confirm.
Correctness verified: The refactor of tick_concurrent_with_count is a pure extraction; tick_concurrent() and the unconditional top-of-tick() drain are unchanged; tick_tasks_only (spawnSync) is intentionally left alone. The immediate_tasks non-empty check correctly reflects pending setImmediate work given the swap semantics in tick_immediate_tasks. has_due_regular_timer uses the same ForceRealTime clock and greater comparison as All::next.
Tests: Two new subprocess tests using crypto.pbkdf2 (chosen because it hits enqueue_task_concurrent on all platforms). Both drain pipes concurrently, assert stderr/stdout before exit code, and use robust metrics (max consecutive run length ≤ 3, first two chars of order). The PR description shows both fail on main and on the Windows canary.
All CodeRabbit and comment-cop feedback threads are resolved.

coderabbitai

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/node/timers/node-timers.test.ts`:
- Around line 283-285: Update both subprocess tests in
test/js/node/timers/node-timers.test.ts at lines 283-285 and 309-313 to assert
the complete { stdout, stderr, exitCode } result before parsing stdout or
validating content; then retain the existing maxRun validation at lines 283-285
and RI-prefix validation at lines 309-313.

🪄 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: b0e6ddb9-64e6-48b0-a169-f75af767774b

📥 Commits

Reviewing files that changed from the base of the PR and between 4e4bca9 and 3bb1da9.

📒 Files selected for processing (2)

src/jsc/event_loop.rs
test/js/node/timers/node-timers.test.ts

claude

I didn't find any bugs, but this changes the core EventLoop::tick() scheduling semantics and overlaps with open PR #32807 at the same three call sites — worth a human look to sign off on the fairness/perf trade-off and coordinate the merge order.

What was reviewed:

The maintenance/pop split in tick_concurrent_with_count — verified it's a pure refactor, all prior callers still get maintenance + pop.
The immediate_tasks check reads the right queue (enqueue_immediate_task pushes there; next_immediate_tasks is the recycle buffer).
__bun_has_due_timer null-guards timer_all() and only peeks the heap; the concurrent_tasks.is_empty() guard keeps the clock read off the empty path.
Tests use crypto.pbkdf2 (routes through enqueue_task_concurrent on all platforms) and assert bounded burst length rather than exact order.

Extended reasoning...

Overview

This PR changes when EventLoop::tick() re-drains the cross-thread concurrent_tasks queue mid-tick. It splits tick_concurrent_with_count() into a maintenance half (ref-count deltas, POSIX signals, imminent GC timer) and a pop-batch half, then introduces tick_concurrent_unless_due() which always runs maintenance but skips the pop when a setImmediate is pending or a JS timer is already due. Three inner-loop tick_concurrent() calls in tick() are replaced; the top-of-tick unconditional drain is left as the once-per-iteration batch boundary. Supporting pieces: timer::All::has_due_regular_timer() (peek + clock compare, skips WTF timers to avoid the lock) and a link-time __bun_has_due_timer() extern in dispatch.rs. Two new subprocess tests in node-timers.test.ts cover the timer and setImmediate interleaving.

Security risks

None. No untrusted input, no auth/crypto surface. The change is purely event-loop scheduling order.

Level of scrutiny

High. EventLoop::tick() is the innermost hot loop of the runtime — every request, every callback, every microtask drain passes through it. A change here shifts latency/throughput characteristics for every workload, and the new has_due_regular_timer() adds a clock_gettime on the mid-tick path whenever concurrent_tasks is non-empty and the regular timer heap is non-empty (which the PR notes is always true when Bun.serve has DateHeaderTimer armed). The guard structure is sound and the PR description argues the read is once-per-batch not once-per-task, but the throughput impact on HTTP-server-shaped workloads is a judgment call a maintainer should make.

Other factors

Overlap with #32807: both PRs modify the same three tick_concurrent() sites in tick(). The author acknowledges this and says whichever lands second needs a trivial rebase, but a human should decide sequencing (or whether one supersedes the other — the immediate_tasks half of this PR's gate is a subset of #32807's change).
The CodeRabbit "gate only the pop, not maintenance" concern was addressed in ee7b864 (verified: maintenance now runs unconditionally before the yield check).
tick_tasks_only() (spawnSync's isolated loop) is deliberately left unchanged — correct, since that context doesn't run timers.
The test's maxRun <= 3 bound with order echoed on failure is a reasonable de-flake shape; the second test's slice(0, 2) === "RI" is deterministic given the fix.
The comment-cop bot flagged the doc comments repeatedly; the author pushed back that they match neighboring style and encode non-obvious ordering/perf decisions. That's a style call for the reviewer.

robobun · 2026-07-30T20:25:49Z

CI: build 85914 is 194/196 passed. The one red is worker-transfer-terminate-stress.test.ts on debian 13 x64-asan (ExceptionScope::assertNoException during worker termination), which also fails on main and is unrelated to this change. Everything else is a retry-passed flake; test-fs-read-stream-pos.js (the test that surfaced this bug) passed on every completed lane after a single retry on win2019. An earlier build of this branch with the same fix logic (85897, commit a35828d) was fully green.

The two new node-timers.test.ts tests pass on every lane. Ready for a maintainer to look at the tick() scheduling change and the merge order with #32807.

github-actions Bot added the claude label Jul 30, 2026

claude Bot reviewed Jul 30, 2026

View reviewed changes