test_runner: gate event-loop drain and on_exit to node:test, drop BUN_TEST_DRAIN_EVENT_LOOP - #35401
test_runner: gate event-loop drain and on_exit to node:test, drop BUN_TEST_DRAIN_EVENT_LOOP#35401robobun wants to merge 5 commits into
Conversation
…_TEST_DRAIN_EVENT_LOOP
The env var was an opt-in for the vendored node test runner and run()
children. Replace it with a TestRunner flag set the first time node:test
registers something in the process, and gate both the post-file
on_before_exit() drain and the shutdown on_exit() call on that flag.
bun:test-only runs revert to setting is_shutting_down directly, so
process.on('exit') handlers are not dispatched for them.
Also call handle_rejected_promises() before on_exit() on the node:test
path, matching bun run.
|
Updated 5:44 AM PT - Jul 24th, 2026
❌ @robobun, your commit 3b9df9f has 1 failures in 🧪 To try this PR locally: bunx bun-pr 35401That installs a local version of the PR into your bun-35401 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
WalkthroughChangesThe test runner removes Node test exit lifecycle
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@src/runtime/cli/test_command.rs`:
- Around line 3049-3064: Update the early bail exit path in TestCommand::run
before the direct global_exit() call so node:test runs use the same
reporter.jest.node_test_module_used-gated shutdown routine as the shown block,
including handle_rejected_promises() and on_exit(). Preserve the existing
non-Node shutdown behavior and ensure global_exit() remains after the shared
shutdown processing.
🪄 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: 5a288423-9563-4578-9fd1-bdb1deed41d6
📒 Files selected for processing (6)
scripts/runner.node.mjssrc/bun_core/env_var.rssrc/js/node/test.tssrc/runtime/cli/test_command.rssrc/runtime/test_runner/jest.rstest/cli/test/bun-test.test.ts
💤 Files with no reviewable changes (2)
- scripts/runner.node.mjs
- src/bun_core/env_var.rs
|
Neither linked issue is fixed here:
|
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #35137. That PR adds a per-file |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
on_before_exit() spins while is_event_loop_alive(), which cannot tell one file's handles from another's. Running it per file meant a handle leaked by an earlier bun:test file would wedge a later node:test file's drain in a multi-file run. Move the drain to the end-of-run node:test block and gate it on single-file runs, matching what the removed env var covered: the vendored node suite and run() children each spawn one file per process.
The end-of-run drain runs after active_file is cleared and after the exit-code decision, so an uncaught throw surfaced there routes through run_error_handler (print only) and left the run at exit 0. Re-check unhandled_error_counter after the drain/handle_rejected_promises/on_exit block and set exit_code = 1 when it grew.
…ture Set exit_code before on_exit() reads it so 'exit' listeners see the propagated code (common.mustCall()'s checker early-returns on nonzero). Keep throws from inside 'exit' listeners out of the check: on main those print but do not fail the run, and the vendored test-sqlite-* files hit EBUSY on Windows from tmpdir's exit-time cleanup with databases still open, which otherwise turns a passing run into exit 1. Arm the drain-throw fixture's setTimeout from inside the test body at 20ms instead of 500ms at module scope so it is due after the per-file loop exits without spinning the drain for half a second.
There was a problem hiding this comment.
No new issues found — all four earlier findings (multi-file drain hang, drain-throw exit-0, exit-handler seeing stale code, 500ms fixture) are addressed in 6621cb6. Deferring rather than approving because this reworks the bun test shutdown sequence (drain / handle_rejected_promises / on_exit / exit-code ordering) and reverts 051952d's unconditional on_exit() for bun:test-only runs, which is a user-visible behavior call worth a maintainer sign-off.
What was reviewed:
- Shutdown ordering in
test_command.rs:prev_unhandledcapture → drain →handle_rejected_promises→ exit-code propagation →on_exit, withis_shutting_downset on both branches beforeglobal_exit. node_test_module_usedwrite injs_file_generation: disjoint from the live&BunTestRootborrow, JS-thread-only.summary.files <= 1gate covers the removed env var's callers (vendored runner,run()children); multi-file no-hang test guards it.- Acknowledged gap left as follow-up:
run()children whose target never importsnode:testno longer drain.
Extended reasoning...
Overview
Removes BUN_TEST_DRAIN_EVENT_LOOP and replaces it with a TestRunner.node_test_module_used flag set from node:test's existing js_file_generation hook. At end-of-run, when the flag is set: drain the event loop (single-file runs only), flush pending rejections, propagate any drain-surfaced uncaught error to the exit code, then dispatch process.on('exit'). bun:test-only runs take the else branch and just set is_shutting_down. Touches test_command.rs (shutdown sequence), jest.rs (flag + write site), env_var.rs / runner.node.mjs / node/test.ts (env-var removal), and adds/rewrites five tests in bun-test.test.ts.
Security risks
None identified. No untrusted-input parsing, no auth/crypto, no network. The unsafe blocks are pre-existing raw-pointer reborrow patterns on the single-threaded JS path; the one new unsafe write ((*runner).node_test_module_used = true) is a disjoint field projection with the aliasing invariant documented at the site.
Level of scrutiny
High. This is the bun test shutdown path — event-loop drain, exit-handler dispatch, exit-code computation, and GC-root release are all ordering-sensitive, and this PR went through three prior review rounds each of which surfaced a real ordering bug (multi-file hang, drain-throw exiting 0, exit handlers seeing a stale code). That history is exactly why I'm not auto-approving: the area has proven subtle enough that a maintainer should confirm the final shape.
Other factors
- Behavior reversion: 051952d made
bun testalways callon_exit(); this PR reverts that forbun:test-only runs. Issue #8434 asks for the opposite. The author explicitly declined to fix #8434 here — that's a reasonable scope call, but a maintainer should confirm the direction. - Deliberately-preserved gap: throws from inside
'exit'listeners still print-but-exit-0 (the author cited Windowstest-sqlite-*EBUSY as the reason not to widen the check). Documented in the code comment. - Acknowledged follow-up:
run()children whose target file never importsnode:testno longer drain (theNODE_TEST_CONTEXTenv var is not consulted natively). Author marked this as low-value and left it. - Overlap with #35137: touches adjacent lines; whichever lands second needs a small rebase.
- Test coverage is good for the paths implemented (both branches of the gate, multi-file no-hang, drain-throw exit code + handler-visible code, exit-handler-fails-run).
There was a problem hiding this comment.
The fixes in 6621cb6 look right — the unhandled-counter check now sits between handle_rejected_promises() and on_exit() so exit handlers see the propagated code, and arming the 20ms timer from inside the test body keeps the drain-throw fixture fast without racing the per-file tick loop. Deferring for a maintainer to confirm the user-facing choice that plain bun:test runs revert to not dispatching process.on('exit') (scoping back 051952d), since #8434 requests the opposite.
Also checked this pass:
fileGeneration()is reached only fromgetRootNode()insrc/js/node/test.ts, so bun:test-only files cannot set the flag.reporter.summary().files <= 1matches the removed env var's coverage (both former setters spawn one file per process); multi-file node:test runs still geton_exit()without the drain.- The raw-pointer write to
node_test_module_usedinjs_file_generationis disjoint from the caller's live&BunTestRootborrow.
Extended reasoning...
Overview
Replaces the BUN_TEST_DRAIN_EVENT_LOOP opt-in (added one commit earlier in 051952d) with automatic detection: a node_test_module_used flag on TestRunner, set the first time node:test's getRootNode() calls the native js_file_generation hook. At end-of-run, when the flag is set, bun test drains the event loop (single-file only), flushes pending rejections, propagates any drain-surfaced uncaught error to the exit code, and dispatches process.on('exit'). bun:test-only runs take the else branch and skip both the drain and on_exit(). Touches test_command.rs (shutdown sequence), jest.rs (flag + setter), src/js/node/test.ts and scripts/runner.node.mjs (drop the env var), env_var.rs (drop the declaration), and adds/rewrites five tests in bun-test.test.ts.
Security risks
None identified. No untrusted-input parsing, no auth/crypto/permissions surface. The two unsafe blocks are pre-existing patterns: test_command.rs reborrows the live &mut VirtualMachine through a raw pointer inside run_with_api_lock (single-threaded, same as the code it replaces), and jest.rs writes a disjoint bool field via runner_ptr() to avoid invalidating the caller's live &BunTestRoot — the SAFETY comment names the invariant and the field projection does not overlap.
Level of scrutiny
High. This is the test-runner shutdown path — event-loop drain, exit-handler dispatch, and exit-code propagation — and three prior review rounds on this PR each surfaced a real bug (multi-file hang from a leaked handle; drain-surfaced throw exiting 0; exit handlers seeing a stale code === 0). Each was fixed with a regression test. That history is exactly why I'm not auto-approving: the area is subtle enough that a maintainer should sign off on the final shape, particularly the summary.files <= 1 heuristic and the decision to keep bun:test runs off the on_exit() path.
Other factors
All prior inline findings are resolved and covered by new tests (bun:test does not run exit handlers, node:test runs exit handlers, multi-file no-hang, drain-throw fails with code === 1 seen by the handler, exit handler can fail the run). The one open design question is user-facing: 051952d made bun test fire process.on('exit') unconditionally; this PR scopes that back to node:test only, which is the pre-051952d75 behavior but is also what issue #8434 is asking to change. The author already stated on the thread that #8434 is intentionally out of scope here — that's reasonable, but it's a product call a human should confirm rather than a bot. The remaining acknowledged gap (a run() child whose target never imports node:test no longer drains) was left as a follow-up with stated reasoning; I agree it's not merge-blocking.
|
CI status: the diff is green on the lanes it touches. Remaining failures across builds 79499 and 79508 are unrelated:
The 9 |
|
Delete it from runner too |
|
Already removed from both |
2 participants
What
Removes
BUN_TEST_DRAIN_EVENT_LOOP(added in 051952d) and replaces it with automatic detection: whennode:testregisters anything in the process,bun testdrains the event loop before exit (single-file runs only) and dispatchesprocess.on('exit')handlers at shutdown, matching how Node runs test files.bun:test-only runs keep the prior exit-after-tests behavior.Why
051952d made
bun testalways callvm.on_exit()(dispatchingprocess.on('exit')handlers) so Node'scommon.mustCall()exit-time checks would run. That is a behavior change for plainbun:testsuites. This PR gates both theon_before_exit()drain and the shutdownon_exit()to processes that actually usenode:test, and drops the env-var opt-in that the vendored-test runner andrun()children set.Also adds
handle_rejected_promises()beforeon_exit()on thenode:testpath, matchingbun run.How
node:testalready calls a nativejsFileGenerationhook on every registration (viagetRootNode()); that now setsTestRunner.node_test_module_used. At end of run, when the flag is set: drain (single-file only), flush pending rejections, dispatch'exit'. The drain is once-per-process rather than per-file, and gated onsummary.files <= 1, becauseon_before_exit()spins whileis_event_loop_alive()and cannot tell one file's handles from another's; the vendored node suite andrun()children each spawn one file per process, so this matches what the removed env var covered without letting a leaked handle from an earlier file wedge a mixed multi-file run.Verification
With
src/stashed (current main),bun:test does not run process.on('exit') handlersfails because main dispatches exit handlers unconditionally.test/js/node/test_runner/node-test.test.ts(45 tests) andtest/js/node/test/parallel/test-file-write-stream5.js(node:test +common.mustCall()) still pass without the env var.no test proof · iteration 2 · 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