Skip to content

bun test: run one bounded macrotask pass after the last test - #35896

Open
robobun wants to merge 2 commits into
mainfrom
farm/34c01618/test-exit-drain
Open

robobun wants to merge 2 commits into
mainfrom
farm/34c01618/test-exit-drain

Conversation

@robobun

@robobun robobun commented Jul 26, 2026 •

Copy link
Copy Markdown
Collaborator

Repro

import { test } from "bun:test";
test("arms a late timer", () => {
  setTimeout(() => { throw new Error("late macrotask failure"); }, 0);
});
$ bun test late.test.ts
(pass) arms a late timer

 1 pass
 0 fail
$ echo $?
0

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::run does one tick_immediate_tasks, an optional on_before_exit() behind BUN_TEST_DRAIN_EVENT_LOOP, one handle_rejected_promises, and returns to the summary/exit path. Pending timers are never given a turn.

Fix

Replace the single tick_immediate_tasks with two auto_tick + tick passes 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 a setTimeout(fn, 0) (clamped to 1ms) become due and fire. Both polls are bounded by the GC controller's 16ms timer, which process_gc_timer arms inside auto_tick before 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.exit laundering pieces of the same analysis are already covered by #33372 and #35139.

Verification

test/cli/test/bun-test.test.ts gains a bun test post-run drain describe that spawns bun test on five fixtures:

  • setTimeout(0) throw, setTimeout(0) rejection: each prints Unhandled error between tests and exits 1
  • setImmediate throw: still caught (regression guard for the replaced tick_immediate_tasks)
  • setTimeout(60000): prints the note and exits 0 in well under 30s (no wait)
  • a clean file: no note

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.ts all 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

@coderabbitai

coderabbitai Bot commented Jul 26, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

Your included review limit has been reached.

You’re in a promotional period — use the checkbox below to run this review for free:

  • Run 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 @coderabbitai review --use-credits.

You can also wait for the limit to reset (next review available in 6 minutes), then comment @coderabbitai review or push new commits to the PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7b3b0822-bd8c-4e56-9538-fdfc3ca2e5cb

📥 Commits

Reviewing files that changed from the base of the PR and between abe2ad4 and ac98c10.

📒 Files selected for processing (4)
  • src/jsc/VirtualMachine.rs
  • src/runtime/cli/test_command.rs
  • test/cli/test/bun-test.test.ts
  • test/integration/next-pages/test/dev-server-ssr-100.test.ts

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

@robobun

robobun commented Jul 26, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 4:57 AM PT - Aug 22nd, 2026

✅ @robobun, your commit ac98c10db8e03e1b9b293f469dca846f86120050 passed in Build #103561! 🎉


🧪   To try this PR locally:

bunx bun-pr 35896

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

bun-35896 --bun

@github-actions

Copy link
Copy Markdown
Contributor

Found 3 issues this PR may fix:

  1. Allow asserting uncaught errors in the Bun's test runner #10226 - Requests the ability to catch uncaught errors from setImmediate callbacks after a test returns; this PR's macrotask drain pass now catches exactly those errors
  2. Top level await causes promise rejections inside global.setTimeout to be ignored, allowing execution to continue #22546 - Promise rejections inside global.setTimeout are silently ignored with top-level await; the bounded drain now lets those timers fire and catches their rejections
  3. bun test swallows unhandled rejections from a script file's async IIFE #34859 - bun test swallows unhandled rejections from a script file's async IIFE; the post-test macrotask drain may now surface these

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #10226
Fixes #22546
Fixes #34859

🤖 Generated with Claude Code

@robobun

robobun commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

None of the three suggested issues are fixed here:

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. bun test: surface unhandled errors that fire after the final test settles #35891 - Addresses the exact same bug (late macrotask errors after the final test are silently orphaned) with the same fix approach: a bounded event-loop drain after the phase loop exits, catching setTimeout/promise-rejection throws via on_uncaught_exception.

🤖 Generated with Claude Code

@robobun

robobun commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

Related: #35891 targets the same bug with a different shape. That one adds a dedicated drain_for_late_errors with a 1s wall-clock cap, checks active_timer_count/immediate_ref_count directly, and gates the drain on first_last.last (only the final file). This one replaces the existing tick_immediate_tasks with two auto_tick passes on every file, bounded by the GC controller's 16ms timer, and adds the note: line for still-pending work.

#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 .unref() edit. Either closes the exit-before-drain gap; happy to close whichever is less wanted.

Comment thread src/runtime/cli/test_command.rs Resolved
Comment thread src/runtime/cli/test_command.rs Resolved
Comment thread src/runtime/cli/test_command.rs Resolved
Comment thread src/runtime/cli/test_command.rs Resolved
Comment thread src/runtime/cli/test_command.rs Resolved
Comment thread src/runtime/cli/test_command.rs Resolved
Comment thread src/runtime/cli/test_command.rs Resolved
Comment thread src/runtime/cli/test_command.rs Resolved
Comment thread src/runtime/cli/test_command.rs Resolved
Comment thread src/runtime/cli/test_command.rs Resolved

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 with self.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 with FirstLast { first: true, last: true } (parallel/runner.rs:606-609) while the worker's own IPC channel keeps platform_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 on reporter.worker_ipc_file_idx.is_none().

    Extended reasoning...

    What the bug is

    Both new gates at test_command.rs:3408 and :3427 call vm.is_event_loop_alive(). That predicate (VirtualMachine.rs:1044-1049) delegates to is_event_loop_alive_excluding_immediates(), whose very first clause is self.unhandled_error_counter == 0 && (…) (VirtualMachine.rs:1036), OR'd only with the setImmediate queues. It was designed for bun_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_counter short-circuits both gates

    unhandled_error_counter is bumped at VirtualMachine.rs:1356/1402/3296 on every "Unhandled error between tests" and is reset only in swap_global_for_test_isolation (:4727) — i.e. under --isolate. In the default non-isolate mode it is monotonic across all files and --rerun-each iterations; test_command.rs:3383 reads it as a baseline but never resets it.

    So once any earlier file (or the first auto_tick at :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 setTimeout does not populate immediate_tasks/next_immediate_tasks (those are for setImmediate), so the OR fallback at :1048-1049 does not save it.

    False positive — --parallel worker IPC keeps the loop alive

    Under bun test --parallel:

    • Every file dispatched to a worker gets FirstLast { first: true, last: true } (parallel/runner.rs:606-609), so first_last.last at :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::begin at runner.rs:580-585 idle-loops on auto_tick() blocking, which requires is_active() true. The isolation swap explicitly skips test_parallel_ipc_group (Channel.rs:114-115), and close_isolation_handles runs at runner.rs:613 after TestCommand::run returns.
    • should_drain_event_loop() reads BUN_TEST_DRAIN_EVENT_LOOP, which parallel workers do not set, so control reaches the else if at :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 on first_last.last doesn't help here because every worker file is last.

    Step-by-step proof

    False negative: bun test a.test.ts b.test.ts (no --isolate). a.test.ts throws from a setTimeout(0) → caught by the new drain, unhandled_error_counter = 1. b.test.ts also arms setTimeout(() => { throw … }, 0) and leaves a ref'd setTimeout(() => {}, 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.ts where both files contain only test("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 for c and again for d, 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-worker mode 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 --parallel is 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. read platform_loop.is_active() / has_pending_refs() / timer-heap peek directly), and additionally gate the note on reporter.worker_ipc_file_idx.is_none() so the runner's own IPC socket is never attributed to user code.

Comment thread src/runtime/cli/test_command.rs Resolved
Comment thread src/runtime/cli/test_command.rs Resolved
Comment thread src/runtime/cli/test_command.rs Resolved
Comment thread src/runtime/cli/test_command.rs Resolved
@robobun

robobun commented Jul 26, 2026 •

Copy link
Copy Markdown
Collaborator Author

CI status (build #103544 on the rebased commit ca24db3, complete): 179/181 lanes passed. The two hard failures are not this diff:

  • test/cli/install/GHSA-pfwx-36v6-832x.test.ts on win11-aarch64: GET https://api.github.com/.../tarball/98e8ff1 - 504, a GitHub API outage during the run (reported to main-break triage).
  • test/js/node/http2/h2-conformance.test.ts on darwin-aarch64: marked [pre-existing], fails on main too.

The remaining reds are [flaky] retries or parallel-batch-only failures that passed alone. The six new post-run drain tests and bun-test.test.ts (104 pass) / isolation.test.ts (30 pass) pass on every lane.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 to false once unhandled_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 on has_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:3404 and the note gate at :3422 — use vm.is_event_loop_alive() as a proxy for "is there still pending work?". But is_event_loop_alive() (VirtualMachine.rs:1044-1049) is is_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 false while 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 to platform_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:

    1. Sync test body arms the timer at now+1ms (0 clamps to 1ms) and unrefs it. platform_loop.active == 0.
    2. phase = Done; the run loop exits.
    3. First auto_tick() at :3402: is_active() is false → tick_without_idle() branch (non-blocking; process_gc_timer/get_timeout skipped). drain_timers runs but only microseconds have elapsed since arming, so the 1ms deadline hasn't passed.
    4. 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.
    5. 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, so is_event_loop_alive() is still true there.

    Axis 2 — unhandled_error_counter > 0

    Once any unhandled error has occurred in the process, is_event_loop_alive() returns false regardless of ref'd timers or open handles (unless an immediate is queued, which won't be the case after tick() at :3403 drained them). unhandled_error_counter is bumped on the isBunTest path for both uncaught exceptions (VirtualMachine.rs:1356) and unhandled rejections (:3296), and is only reset in swap_global_for_test_isolation (:4727) — i.e. under --isolate, never between files in default mode.

    Effects:

    • At :3404 — if the first auto_tick() fires a setTimeout(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 the bun run / on_before_exit drain 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'd setTimeout(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. The unhandled_error_counter case 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.

Comment thread src/runtime/cli/test_command.rs Resolved
Comment thread src/runtime/cli/test_command.rs Resolved
@robobun

robobun commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

On the latest additional-findings comment (unref'd timers + unhandled_error_counter): the error-counter axis was already addressed in 6a35425 by replacing is_event_loop_alive() with has_pending_loop_work(), which reads the raw loop constituents without that gate.

On the unref'd-timer axis: leaving it as-is. .unref() is the user explicitly saying this timer should not keep the process alive, and under bun run an unref'd timer that has not fired by exit is silently dropped. Giving it an extra turn under bun test would diverge from that, and the ref'd variant (the one that would have kept the process alive and so whose drop is a real exit-code hazard) is what this PR targets.

Comment thread src/runtime/cli/test_command.rs Resolved
Comment thread src/runtime/cli/test_command.rs Resolved
Comment thread src/jsc/VirtualMachine.rs Resolved

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_immediates refactor: same four constituents, same unhandled_error_counter == 0 gate — behavior-identical.
  • The two-pass drain's bounding: both polls now guarded by has_timer_due_within(20) || wakeup(); residual TOCTOU only bites under BUN_GC_TIMER_DISABLE=1 + a contrived setImmediate-clears-timer race (declined, reasonable).
  • note: gating: first_last.last && last-repeat && !test_isolation_enabled covers --isolate and --parallel workers (whose IPC pipe would false-positive on Windows).
  • dev-server-ssr-100 onEmpty→onIdle: correct fix — onEmpty resolves with concurrency tasks still in flight, and the new drain would surface their post-dispose ConnectionRefused rejections.
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.

@robobun

robobun commented Aug 22, 2026 •

Copy link
Copy Markdown
Collaborator Author

#45109 fixed the dev-server-ssr-100.test.ts race this PR also touches: main now waits for queue.onIdle(). When this PR rebases, the hunk for that file can be dropped.

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.
@robobun
robobun force-pushed the farm/34c01618/test-exit-drain branch from fe92657 to ca24db3 Compare August 22, 2026 11:02
@robobun

robobun commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto main (ca24db3, squashed to one commit). Two small conflicts resolved:

bun-test.test.ts (104 pass) and isolation.test.ts (30 pass) pass on the rebased tree.

Comment thread src/runtime/cli/test_command.rs Resolved
Comment thread src/runtime/cli/test_command.rs 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).
Comment thread src/runtime/cli/test_command.rs Resolved
Comment thread src/runtime/cli/test_command.rs Resolved

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@robobun

robobun commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator Author

Note on the dev-server-ssr-100.test.ts hunk: #40061 fixes the same race more thoroughly (awaits every queue.add() promise and asserts all 100 completed). If #40061 lands first, this PR's one-line onEmpty to onIdle change conflicts trivially and should be dropped in favor of it. If this lands first, #40061 still applies cleanly on top. Either order works.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants