Repository navigation
Conversation
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 29 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 6 minutes), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (4)
Comment |
|
Updated 4:57 AM PT - Aug 22nd, 2026
✅ @robobun, your commit ac98c10db8e03e1b9b293f469dca846f86120050 passed in 🧪 To try this PR locally: bunx bun-pr 35896That installs a local version of the PR into your bun-35896 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
None of the three suggested issues are fixed here:
|
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Related: #35891 targets the same bug with a different shape. That one adds a dedicated #35891 is more explicit about what it waits on and has the longer grace window; this one is smaller and doesn't need the isolation.test.ts |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/runtime/cli/test_command.rs:3408-3417—is_event_loop_alive()answers "should the process keep draining", not "does user code have pending work", so it misfires in both directions here: it starts withself.unhandled_error_counter == 0 && …(VirtualMachine.rs:1036), so once any prior file in a non-isolate run has produced an unhandled error, the second drain pass at :3408 and the note at :3427 are silently disabled for the rest of the run; and under--parallel, every worker file is dispatched withFirstLast { first: true, last: true }(parallel/runner.rs:606-609) while the worker's own IPC channel keepsplatform_loop.is_active()true (Channel.rs:244-250, explicitly skipped by the isolation swap), so the note prints spuriously after every clean file. Consider checking user-scoped liveness directly (e.g.platform_loop.is_active() || has_pending_refs()without the error-counter gate) and additionally gating the note onreporter.worker_ipc_file_idx.is_none().Extended reasoning...
What the bug is
Both new gates at
test_command.rs:3408and:3427callvm.is_event_loop_alive(). That predicate (VirtualMachine.rs:1044-1049) delegates tois_event_loop_alive_excluding_immediates(), whose very first clause isself.unhandled_error_counter == 0 && (…)(VirtualMachine.rs:1036), OR'd only with thesetImmediatequeues. It was designed forbun_main/on_before_exit, where a nonzero error counter means "stop draining and exit". Reusing it here as "is there pending work?" conflates those semantics and misfires in both directions.False negative —
unhandled_error_countershort-circuits both gatesunhandled_error_counteris bumped at VirtualMachine.rs:1356/1402/3296 on every "Unhandled error between tests" and is reset only inswap_global_for_test_isolation(:4727) — i.e. under--isolate. In the default non-isolate mode it is monotonic across all files and--rerun-eachiterations;test_command.rs:3383reads it as a baseline but never resets it.So once any earlier file (or the first
auto_tickat :3406 for the current file) has produced an unhandled error:- :3408 — the second drain pass is skipped. The PR's own comment at :3403-3405 says a leftover wakeup can consume the first poll and the second is what lets
setTimeout(fn, 0)(clamped to 1ms) become due; without it, a later file's late-macrotask throw can be re-orphaned — the exact bug this PR fixes, hidden behind a prior error. - :3427 — the pending-work note is suppressed for the rest of the run, even if the last file leaves a ref'd 60s timer.
A ref'd
setTimeoutdoes not populateimmediate_tasks/next_immediate_tasks(those are forsetImmediate), so the OR fallback at :1048-1049 does not save it.False positive —
--parallelworker IPC keeps the loop aliveUnder
bun test --parallel:- Every file dispatched to a worker gets
FirstLast { first: true, last: true }(parallel/runner.rs:606-609), sofirst_last.lastat :3425 is always satisfied. - The worker's IPC channel to the coordinator is a ref'd loop handle — Channel.rs:244-250 explicitly documents "We keep the pipe ref'd… unref'ing here makes autoTick() take the tickWithoutIdle (NOWAIT) path" on Windows, and on POSIX
WorkerLoop::beginat runner.rs:580-585 idle-loops onauto_tick()blocking, which requiresis_active()true. The isolation swap explicitly skipstest_parallel_ipc_group(Channel.rs:114-115), andclose_isolation_handlesruns at runner.rs:613 afterTestCommand::runreturns. should_drain_event_loop()readsBUN_TEST_DRAIN_EVENT_LOOP, which parallel workers do not set, so control reaches theelse ifat :3425.
So after every clean file (
unhandled_error_counter == 0),is_event_loop_alive()is true purely because of the runner's own socket, and the note "a test left a timer or open handle…" prints once per file per worker — misattributing an internal handle to user code. The d1492e9 fix that gated onfirst_last.lastdoesn't help here because every worker file islast.Step-by-step proof
False negative:
bun test a.test.ts b.test.ts(no--isolate).a.test.tsthrows from asetTimeout(0)→ caught by the new drain,unhandled_error_counter = 1.b.test.tsalso armssetTimeout(() => { throw … }, 0)and leaves a ref'dsetTimeout(() => {}, 60_000). At :3408,is_event_loop_alive()=(0 == 1) && … || <empty immediate queues>=false→ second pass skipped; if the first poll at :3406 was consumed by a leftover wakeup, b's throw is orphaned. At :3427 the same predicate is false → note suppressed despite the pending 60s timer.False positive:
bun test --parallel c.test.ts d.test.tswhere both files contain onlytest("clean", () => {}). Each worker:first_last.last = true,repeat_index + 1 == repeat_count,unhandled_error_counter == 0,platform_loop.is_active() == true(IPC socket) → note prints forcand again ford, both spurious.Why existing tests don't catch it
The new tests spawn single-file processes or a two-file run with no errors and no
--parallel. Neither the cross-file error-counter state nor--test-workermode is exercised.Impact and fix
Neither direction changes pass/fail counts or exit codes — the false-negative case only loses an additional diagnostic in a run already exiting nonzero, and the false-positive case is advisory spam — so this is a nit. But
--parallelis a documented user-facing flag, and a suite with hundreds of files will emit hundreds of misleading notes.Fix: replace
vm.is_event_loop_alive()at both sites with a raw liveness check that omits the error-counter gate (e.g. readplatform_loop.is_active()/has_pending_refs()/ timer-heap peek directly), and additionally gate the note onreporter.worker_ipc_file_idx.is_none()so the runner's own IPC socket is never attributed to user code. - :3408 — the second drain pass is skipped. The PR's own comment at :3403-3405 says a leftover wakeup can consume the first poll and the second is what lets
|
CI status (build #103544 on the rebased commit ca24db3, complete): 179/181 lanes passed. The two hard failures are not this diff:
The remaining reds are |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/runtime/cli/test_command.rs:3404-3412—is_event_loop_alive()is the wrong predicate for these two gates: it ignores unref'd timers (they sit in the heap but contribute to none of its inputs) and short-circuits tofalseonceunhandled_error_counter > 0(VirtualMachine.rs:1036). So the PR's own repro with.unref()appended —setTimeout(() => { throw ... }, 0).unref()— skips the second pass and exits 0, and once the first pass catches one throw (or any earlier file produced an unhandled error in a non-isolate run), a second near-due timer never gets its turn and the note at :3422 is suppressed for the rest of the run. Gating :3404 onhas_timer_due_within(20) || vm.is_event_loop_alive()(and mirroring the heap peek at :3422) fixes both axes with machinery this PR already added.Extended reasoning...
What
Both new call sites — the second-pass gate at
test_command.rs:3404and the note gate at:3422— usevm.is_event_loop_alive()as a proxy for "is there still pending work?". Butis_event_loop_alive()(VirtualMachine.rs:1044-1049) isis_event_loop_alive_excluding_immediates() || immediate-queues-nonempty, and the former (:1030-1042) is defined as:self.unhandled_error_counter == 0 && (platform_loop.is_active() as usize + active_tasks + el.tasks.readable_length() + el.has_pending_refs() as usize > 0)
That gives two independent ways for it to return
falsewhile a due (or near-due) timer sits in the heap.Axis 1 — unref'd timers
An unref'd timer lives in
timer::All's heap but does not contribute toplatform_loop.is_active()(the ref'd-handle count),active_tasks,tasks.readable_length(),has_pending_refs(), or the immediate queues. So the PR's exact motivating repro with.unref()appended:test("arms a late timer", () => { setTimeout(() => { throw new Error("late"); }, 0).unref(); });
Step-by-step:
- Sync test body arms the timer at now+1ms (0 clamps to 1ms) and unrefs it.
platform_loop.active == 0. phase = Done; the run loop exits.- First
auto_tick()at :3402:is_active()is false →tick_without_idle()branch (non-blocking;process_gc_timer/get_timeoutskipped).drain_timersruns but only microseconds have elapsed since arming, so the 1ms deadline hasn't passed. is_event_loop_alive()at :3404 → false (nothing ref'd, no tasks, no immediates). The second pass at :3405-3411 — the one whose stated purpose per the comment is to let the 1ms clamp elapse — is skipped entirely.- Note check at :3422 → false. Throw orphaned; run exits 0.
has_timer_due_within(20)at :3405 would see the unref'd timer (same heap), but it's only reached when :3404 is true. The five new tests all use ref'd timers; the "not-yet-due timer" fixture's.unref()line is paired with a second ref'd 60s timer, sois_event_loop_alive()is still true there.Axis 2 —
unhandled_error_counter > 0Once any unhandled error has occurred in the process,
is_event_loop_alive()returnsfalseregardless of ref'd timers or open handles (unless an immediate is queued, which won't be the case aftertick()at :3403 drained them).unhandled_error_counteris bumped on theisBunTestpath for both uncaught exceptions (VirtualMachine.rs:1356) and unhandled rejections (:3296), and is only reset inswap_global_for_test_isolation(:4727) — i.e. under--isolate, never between files in default mode.Effects:
- At :3404 — if the first
auto_tick()fires asetTimeout(0)that throws, the guard evaluates false and a second still-pending near-due timer (e.g.setTimeout(() => { throw A }, 0); setTimeout(() => { throw B }, 5);) never gets its turn. The run still exits 1 (A was caught), so this is diagnostic-completeness only. - At :3422 — in a non-isolate multi-file run, once file A produces an "Unhandled error between tests",
is_event_loop_alive()is false for the rest of the run, so a ref'd 60s timer left by file B never triggers the note.
Why it's the wrong predicate
is_event_loop_alive()was designed for thebun run/on_before_exitdrain loop, where "stop draining once an error surfaces" and "unref'd handles don't keep the process alive" are both intended semantics. Reusing it here as a plain "is there pending work?" check silently inherits both. But this PR frames the drain as "one bounded macrotask pass", not "drain until idle" — a due timer's throw is a failure whether or not it would have kept the process alive, and whether or not something else already threw.Fix
Gate :3404 on the timer heap directly:
if has_timer_due_within(20) || vm.is_event_loop_alive() {
This uses machinery the PR already added, sees unref'd heap entries, and doesn't depend on
unhandled_error_counter. For the note at :3422, either OR in a heap peek /active_timer_count()check, or read the loop-liveness inputs directly (platform_loop.is_active() || active_tasks > 0 || …) rather than the error-gated wrapper.Severity
Nit. Neither axis is a regression — before this PR only
tick_immediate_tasks()ran here, so both the ref'd and unref'dsetTimeout(0)variants were orphaned. The unref'd case is REVIEW.md "Cover the variant matrix": the ref'd variant of the PR's own repro is caught, its.unref()sibling is not. Theunhandled_error_countercase only changes which diagnostics print in a run already exiting non-zero. Both are worth fixing since the change is one line and reuses code already in the diff. - Sync test body arms the timer at now+1ms (0 clamps to 1ms) and unrefs it.
|
On the latest additional-findings comment (unref'd timers + On the unref'd-timer axis: leaving it as-is. |
There was a problem hiding this comment.
No new issues after fe92657 — the has_pending_loop_work move is behavior-preserving and my earlier round's findings are addressed or reasonably declined. Deferring to a human because this is a user-facing bun test behavior change (previously-silent late macrotask errors now fail the run, and a new note: line is emitted), and #35891 implements the same fix with a different shape — a maintainer should pick which lands.
What was reviewed:
is_event_loop_alive_excluding_immediatesrefactor: same four constituents, sameunhandled_error_counter == 0gate — behavior-identical.- The two-pass drain's bounding: both polls now guarded by
has_timer_due_within(20) || wakeup(); residual TOCTOU only bites underBUN_GC_TIMER_DISABLE=1+ a contrivedsetImmediate-clears-timer race (declined, reasonable). note:gating:first_last.last && last-repeat && !test_isolation_enabledcovers--isolateand--parallelworkers (whose IPC pipe would false-positive on Windows).dev-server-ssr-100onEmpty→onIdle: correct fix —onEmptyresolves withconcurrencytasks still in flight, and the new drain would surface their post-disposeConnectionRefusedrejections.
Extended reasoning...
Overview
The PR replaces bun test's post-phase-Done tick_immediate_tasks() with a two-pass auto_tick()+tick() drain so a setTimeout(0) throw/rejection armed by the last test books as "Unhandled error between tests" instead of being silently dropped. It adds a note: when the loop is still alive after the drain (default serial mode only), factors the raw liveness predicate onto VirtualMachine::has_pending_loop_work(), adds a has_timer_due_within(ms) heap-peek helper, ships six subprocess tests in bun-test.test.ts, and fixes a latent onEmpty/onIdle race in dev-server-ssr-100.test.ts that the new drain surfaced.
Security risks
None. No untrusted input parsing, no auth/crypto, no network. The unsafe blocks in has_timer_due_within dereference the per-thread timer heap on the JS thread after a null check — same access pattern auto_tick's get_timeout already uses.
Level of scrutiny
High. This runs on every bun test invocation and changes exit-code semantics: files that previously exited 0 with an orphaned late-timer error will now exit 1. That is the correct behavior, but it is a breaking observable change and the dev-server-ssr-100 fix in this same PR demonstrates it can flip existing suites. The note: line is new user-facing output whose wording and gating (suppressed under --isolate/--parallel) are policy calls. And #35891 solves the same bug with a different design (explicit 1s wall-clock cap, last-file-only, direct active_timer_count check) — a maintainer should decide which approach to take rather than having a bot pick one.
Other factors
The PR has been through four review rounds; each finding I raised was either addressed (both polls now guarded, has_pending_loop_work moved onto VirtualMachine and is_event_loop_alive_excluding_immediates rewritten to delegate — verified byte-equivalent semantics, note gated on !test_isolation_enabled) or declined with a defensible rationale (the setImmediate-clears-near-timer TOCTOU only unbounds under BUN_GC_TIMER_DISABLE=1, a debug-only knob). The comment-cop bot's repeated "paragraph-long comment" flags were consciously kept — the comments document non-obvious event-loop mechanics, which I agree is warranted here. Test coverage is good: throw + rejection variants, setImmediate regression guard, far-future timer noted-not-waited with an elapsed-time assertion, clean-file negative, and multi-file once-only. CI on the earlier commits was green on the new tests; the only hard failure was binary-size against a stale canary baseline.
|
#45109 fixed the |
A failure surfaced from a setTimeout(fn, 0) armed by the last test in a file was silently dropped: the per-file loop exited at Phase::Done, ran one setImmediate batch, printed the summary and exited before the timer could fire. The unhandled-rejection handler stays armed the whole run, so a late microtask rejection was already caught; anything behind a macrotask was orphaned. Replace the single tick_immediate_tasks with two auto_tick+tick passes so a 1ms-clamped setTimeout(0) can become due. Each poll is bounded: when the nearest heap deadline is within 20ms it bounds the poll itself, otherwise wakeup() keeps it non-blocking so a far-future timer or open socket is never waited on. A throw/rejection from a now-due timer books as 'Unhandled error between tests' and fails the run. After the bounded pass, if the loop still has pending work at the end of a default serial run, print a note rather than waiting on it. The check is skipped under --isolate/--parallel (per-file teardown; a worker's IPC socket would count). VirtualMachine gains has_pending_loop_work(), the raw liveness check that is_event_loop_alive() gates on unhandled_error_counter == 0. dev-server-ssr-100.test.ts: await PQueue onIdle, not onEmpty. onEmpty resolves while the last `concurrency` tasks are in flight, so the using-devServer dispose raced their fetches; the new drain surfaced the resulting ConnectionRefused rejections.
fe92657 to
ca24db3
Compare
|
Rebased onto main (ca24db3, squashed to one commit). Two small conflicts resolved:
|
…parallel workers Under GITHUB_ACTIONS the note was emitted inside the last file's log group, which the UI collapses for a passing file. Record it during the drain and print after the group closes. A --parallel --no-isolate worker has test_isolation_enabled = false, so the isolation gate did not cover it; add reporter.worker_ipc_file_idx to the gate (the worker's own IPC socket keeps the loop active).
There was a problem hiding this comment.
Both nits from my last pass are addressed in ac98c10 — the note now prints after ::endgroup:: via the pending_work_note flag, the gate carries reporter.worker_ipc_file_idx.is_none(), and two new tests cover each. No further findings this run.
This still changes what bun test considers a passing run (a late setTimeout(0) throw now exits 1) and the two-pass drain's bound rests on the wakeup/GC-timer interaction, so a human sign-off on the exit-semantics change is worthwhile.
What was reviewed: the VirtualMachine refactor is behavior-identical to is_event_loop_alive_excluding_immediates on main (yield_tasks/has_concurrent_tasks included); has_timer_due_within's two unsafe derefs are single-JS-thread and null-checked; the --isolate/--parallel --no-isolate/GHA note-gate paths were traced and each is now covered by a fixture.
Extended reasoning...
Overview
The PR replaces the single tick_immediate_tasks after bun test reaches Phase::Done with a two-pass auto_tick + tick bounded drain, so a setTimeout(fn, 0) armed by the last test can fire and its throw/rejection books as "Unhandled error between tests" (exit 1) instead of being silently orphaned (exit 0). It also adds an advisory note: when the loop is still alive after the drain, refactors VirtualMachine::is_event_loop_alive_excluding_immediates to derive from a new has_pending_loop_work[_excluding_immediates] pair, adds has_timer_due_within (unsafe timer-heap peek) to bound each poll, and swaps onEmpty → onIdle in dev-server-ssr-100.test.ts. Eight new subprocess-spawning tests in bun-test.test.ts cover the throw/rejection/setImmediate/far-timer/clean/multi-file/GHA/parallel-worker cases.
Since my last review
At 2026-08-22T11:35:02Z I left two 🟡 nits: (1) the note printed inside the last file's ::group:: under GITHUB_ACTIONS=true, and (2) under --parallel --no-isolate the worker's ref'd IPC pipe made has_pending_loop_work() true so the note printed for every clean file. Commit ac98c10 addresses both exactly as suggested (flag hoisted past ::endgroup::; && reporter.worker_ipc_file_idx.is_none() added) and adds a test for each. The two open comment-cop bot flags at :3403 and :3421 are the same automated "paragraph-long comment" heuristic the author has repeatedly (and reasonably) declined — the comments are 2-5 lines documenting non-obvious event-loop mechanics, not workaround justifications.
Security risks
None. No untrusted input parsing, no auth/crypto, no filesystem/network surface change. The two new unsafe blocks in has_timer_due_within deref the per-thread timer heap on the single JS thread after a null check; peek() returns a live intrusive-heap node.
Level of scrutiny
High. This is a user-visible behavioral change to bun test's exit code: any existing suite whose last test arms a due macrotask that throws will go from green to red. The bounding argument for each poll is subtle (nearest heap deadline ≤20ms, else vm.wakeup(); the GC repeating timer as backstop under the declined TOCTOU nit). The change has been iterated ~8 times through review and every raised concern has either been fixed or explicitly reasoned about, but the design decision itself — how much of the loop to drain after the last test, and whether to fail the run for it — is one a maintainer should sign off on.
Other factors
CI on the pre-ac98c10d rebase was 179/181 with the two reds documented as unrelated (GitHub API 504, pre-existing h2-conformance flake). robobun noted #40061 supersedes the dev-server-ssr-100.test.ts hunk; it's still here but is a correct standalone fix (onEmpty resolves before the last concurrency fetches finish). The VirtualMachine.rs refactor was diffed against main and preserves the exact liveness-constituent set including the #38299 additions.
|
Note on the |
Repro
The timer never runs and the file is reported green. A late microtask rejection is already caught (the unhandled-rejection handler stays armed through Phase::Done); anything behind a macrotask is orphaned.
Cause
After the per-file loop reaches
Phase::Done,TestCommand::rundoes onetick_immediate_tasks, an optionalon_before_exit()behindBUN_TEST_DRAIN_EVENT_LOOP, onehandle_rejected_promises, and returns to the summary/exit path. Pending timers are never given a turn.Fix
Replace the single
tick_immediate_taskswith twoauto_tick+tickpasses while the file's unhandled-rejection handler is still armed. A wakeup left over from the run loop consumes the first poll; the second lets asetTimeout(fn, 0)(clamped to 1ms) become due and fire. Both polls are bounded by the GC controller's 16ms timer, whichprocess_gc_timerarms insideauto_tickbefore computing the poll deadline, so neither waits on a far-future user timer or an open socket. A throw or rejection from a now-due timer books as "Unhandled error between tests" and fails the run.After the bounded pass, if the loop is still alive (a future-deadline timer or a ref'd handle), surface a
note:rather than waiting on it.The thenable-await and
process.exitlaundering pieces of the same analysis are already covered by #33372 and #35139.Verification
test/cli/test/bun-test.test.tsgains abun test post-run draindescribe that spawnsbun teston five fixtures:setTimeout(0)throw,setTimeout(0)rejection: each printsUnhandled error between testsand exits 1setImmediatethrow: still caught (regression guard for the replacedtick_immediate_tasks)setTimeout(60000): prints the note and exits 0 in well under 30s (no wait)With
src/stashed the first two and the note test fail (1 pass / 0 fail, exit 0); with the change all five pass.test/cli/test/bun-test.test.ts,test-test.test.ts,isolation.test.ts,test-timeout-behavior.test.ts,rerun-each.test.ts,test/js/node/test_runner/node-test.test.tsall pass unchanged.no test proof · iteration 10 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/test/bun-test.test.ts