Exit 13 on unsettled top-level await in the main entry instead of spinning - #36051
Exit 13 on unsettled top-level await in the main entry instead of spinning#36051robobun wants to merge 8 commits into
Conversation
…nning
`await new Promise(() => {})` at the top level of the entry module made the
main thread spin `wait_for_promise`'s tick/auto_tick loop forever at ~100%
CPU (strace: epoll fd created but never waited on; `us_loop_run_bun_tick`
early-returns on `num_polls == 0`). Node prints a warning and exits 13.
The worker entry path already handles this: `wait_for_promise_with_termination`
breaks once `!is_event_loop_alive()` and web_worker.rs sets `exit_code = 13`.
Give the main entry the same treatment:
- `load_entry_point` (non-watcher arm) now breaks out of its wait once the
loop has nothing left that could settle the entry promise, returning with
it still Pending.
- After the core run-loop and `on_before_exit()`, `Run::start` checks the
entry promise: if still Pending, print Node's "Detected unsettled
top-level await" warning (entry path, no line number) and set
`exit_code = 13` unless the user already set one.
The check sits after `on_before_exit()` so `beforeExit` fires with 0 first
(Node's order). `--watch`/`--hot` keep their existing spin-forever loop.
Fixes #33283.
|
Reproduced with Latest (4a34c72): entry promise is now |
|
Warning Review limit reached
Next review available in: 12 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling 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)
WalkthroughChangesThe runtime now drains the event loop without indefinitely waiting on unsettled entry promises. The CLI reports unsettled top-level await with exit code 13, while preserving explicit exit codes. New tests cover direct, dependency, timer, microtask, Unsettled top-level await
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
|
Updated 11:23 PM PT - Jul 26th, 2026
✅ @robobun, your commit 4a34c72388d01ccc925b0279a53b4cda0db94dac passed in 🧪 To try this PR locally: bunx bun-pr 36051That installs a local version of the PR into your bun-36051 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Relationships are expected:
Either order works; keeping this one open as the small standalone fix for the plain |
There was a problem hiding this comment.
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 `@test/js/node/process/unsettled-top-level-await.test.ts`:
- Around line 41-44: Update the assertions in the unsettled top-level await test
to verify that stderr contains the reported entry path “entry.mjs” in addition
to the existing warning text and exit status checks.
- Around line 31-33: Reorder assertions in the affected unsettled top-level
await subprocess tests so stderr diagnostics, including the “Detected unsettled
top-level await at” message and module name, are validated before asserting
signalCode and exitCode. Apply this consistently to the referenced assertion
blocks while preserving their existing expected values.
🪄 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: eb026dde-1883-4509-930f-b95e6b5760fd
📒 Files selected for processing (3)
src/jsc/VirtualMachine.rssrc/runtime/cli/run_command.rstest/js/node/process/unsettled-top-level-await.test.ts
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/jsc/VirtualMachine.rs:2448-2462— This drain-aware loop only coversload_entry_point; two sibling non-watcher arms still call the unboundedwait_for_promiseand hit the identical 100%-CPU spin:load_preloadsatsrc/runtime/jsc_hooks.rs:831-837(sobun --preload ./p.mjs a.mjswith an unsettled-TLA preload still hangs — it runs before this loop is ever reached) andload_entry_point_for_test_runneratsrc/jsc/VirtualMachine.rs:4602-4609(sobun teston a file with module-levelawait new Promise(()=>{})still hangs). Per REVIEW.md's "fix the whole class in the same PR", either move the!is_event_loop_alive()break intoEventLoop::wait_for_promiseitself (covers all three at once), apply the same loop to both siblings, or note the intentional exclusion in the PR body.Extended reasoning...
What the bug is
The PR replaces
wait_for_promiseinload_entry_point's non-watcher arm with a drain-aware loop that breaks once!is_event_loop_alive(), so an unsettled top-level await in the main entry surfaces as exit 13 instead of a 100%-CPU busy-spin. But two sibling call sites share the exact same pattern — a non-watcherelse-arm calling the unboundedwait_for_promise(AnyPromise::Internal(promise))on a module-load promise — and neither is updated:load_preloadsatsrc/runtime/jsc_hooks.rs:836— the--preload/-r/ bunfigpreloadpath.load_entry_point_for_test_runneratsrc/jsc/VirtualMachine.rs:4608— thebun testentry loader, a near-byte-identical copy ofload_entry_point.
EventLoop::wait_for_promise(event_loop.rs:957-971) loopstick()/auto_tick()with only anexecution_forbidden()check — nois_event_loop_alive()break — so both siblings still busy-spin exactly as the entry module did before this PR.Code path —
--preload(step-by-step proof)Given
p.mjs = 'await new Promise(() => {});'anda.mjs = 'console.log(1)', runbun --preload ./p.mjs a.mjs:Run::start→vm.load_entry_point(entry)(run_command.rs:1496)- →
reload_entry_point(VirtualMachine.rs:2419), which at line 2342 calls(hooks.load_preloads)(self)before importing the entry module. load_preloads(jsc_hooks.rs:669) iterates each preload, imports it, and in the non-watcherelse-arm at lines 831-837 callsunsafe { (*vm).wait_for_promise(AnyPromise::Internal(promise)) }.wait_for_promiseloopstick()/auto_tick()forever — the promise never settles and there is no liveness check.auto_tick→tick_without_idle()→us_loop_run_bun_tickearly-returns onnum_polls == 0every iteration (the exact busy-spin the PR description diagnoses).load_preloadsnever returns →reload_entry_pointnever returns → the PR's new drain-aware loop at VirtualMachine.rs:2453-2462 is never reached.
Result:
bun --preload ./p.mjs a.mjsstill spins at ~100% CPU, never exits — identical to the bug this PR fixes for the entry module. Node exits 13 with the unsettled-TLA warning fornode --import ./p.mjs a.mjs.Code path —
bun testload_entry_point_for_test_runner(VirtualMachine.rs:4576-4613) is a copy-pasted twin ofload_entry_point. Its non-watcher arm at line 4608 still callsself.wait_for_promise(jsc::AnyPromise::Internal(promise)). A test file whose module body doesawait new Promise(() => {})before anytest()call (so no per-test timeout is armed) hangsbun testin the same busy-spin.Why existing code doesn't prevent it
wait_for_promisehas only anexecution_forbidden()early-out, which is irrelevant on the main thread. The worker path useswait_for_promise_with_termination, which does have the!is_event_loop_alive()break — this PR mirrors that shape forload_entry_pointonly, leaving the two other module-load callers on the unbounded variant.Why this is in scope
REVIEW.md is explicit under "Correctness: the bug class, not the bug":
Fix the whole class in the same PR (same-class sites are ONE concern, not scope creep). Grep for every sibling site sharing the pattern: parallel switch arms, sync/async twins … copy-pasted blocks, every caller of a changed helper. Prefer moving the guard into the shared helper. If a site is intentionally excluded, say so in the PR.
Both sites are the same pattern (
wait_for_promiseon a module-evaluation promise in a non-watcher arm), andload_entry_point_for_test_runneris literally a copy-pasted block. The--preloadcase is on the samebun runcode path this PR targets and reproduces with a one-flag variation of the PR's own repro. The PR description mentions deferring the--printordering /beforeExit-drain / stalled-module walk from #33286, but does not mention excluding these siblingwait_for_promisesites.Impact
bun --preload ./p.mjs a.mjs(and-r, and bunfigpreload = [...]) with an unsettled TLA in the preload → 100%-CPU hang, never exits.bun test file.test.tswherefile.test.tshasawait new Promise(()=>{})at module top → 100%-CPU hang, never exits.
These are pre-existing hangs the PR doesn't worsen — but they are the same bug class the PR claims to fix, reachable via documented CLI flags.
How to fix
Preferred (per REVIEW.md "Prefer moving the guard into the shared helper"): add the
!is_event_loop_alive()break toEventLoop::wait_for_promiseitself (or a newwait_for_module_promisehelper) so all three sites are covered at once and the drain-aware loop isn't open-coded three times.Minimal: apply the same loop shape to
jsc_hooks.rs:831-837andVirtualMachine.rs:4602-4609. The test-runner caller will additionally needtest_command.rshandling to report the file as failed and move on — if that's out of scope for this PR, at minimum note the intentional exclusion in the PR body per REVIEW.md.
… Node parity The drain-aware wait means the entry promise can now transition Pending -> Rejected inside on_before_exit() (e.g. a beforeExit listener rejects the awaited promise). That rejection was falling through the Pending-only check and the process exited 0 with the error swallowed. Add the missing Rejected arm (same shape as the initial-load check and web_worker.rs) so the error is routed through uncaught_exception. spawn.test.ts's "unref() + .exited with nothing else ref'd (Windows)" block asserted the old busy-spin side effect (exit 0, "resolved"). Node exits 13 for all three shapes; update the expectations. The underlying Windows IOCP-unref'd-handle fix (#34478) is still covered by the non-TLA "should not hang" block above it.
…error_counter == 0 The non-watcher reload_entry_point stores pending_internal_promise unprotected (no visitChildren scans it), so once the stack local in Run::start dies the cell can be collected before the post-on_before_exit status read. Root it with a Protected guard that lives through that read. is_event_loop_alive() also returns false when unhandled_error_counter > 0 (a fatal uncaught exception aborted the wait, not a natural drain). Gate the entry-promise check on unhandled_error_counter == 0 so a ref'd timer that would have settled the await is not misreported as an unsettled TLA (matching on_before_exit's own guard and Node's output).
There was a problem hiding this comment.
All four rounds of review feedback have been addressed and the bug hunter found nothing further, but this changes core entry-point/exit semantics and intentionally flips three existing spawn.test.ts cases from "resolves, exit 0" to "exit 13" — a maintainer should sign off on that behavior change and the new GC-rooting of the entry promise across on_before_exit().
What was reviewed:
- The new drain-aware wait loop in
load_entry_pointand the post-on_before_exit()tri-state check — mirrorsweb_worker.rs:1112-1135as claimed. _entry_promise_protectedguard scope: declared before the match, assigned only onOk, lives past the line-1627 deref;vm.pending_internal_promiseis the same pointer that was protected on all non-watcher store paths.- The
unhandled_error_counter == 0andpending_internal_promise_reported_atgates against spurious warnings / double-reporting. - Test file follows harness conventions (
tempDir,bunEnv,test.concurrent, watchdog < per-test timeout, stderr asserted before exit status).
Extended reasoning...
Overview
Touches four files: src/jsc/VirtualMachine.rs (replaces the unbounded wait_for_promise in load_entry_point's non-watcher arm with a loop that breaks once !is_event_loop_alive()), src/runtime/cli/run_command.rs (roots the entry promise with a Protected guard and adds a post-on_before_exit() tri-state check that prints Node's unsettled-TLA warning + exit 13, or routes a late rejection through uncaught_exception), test/js/bun/spawn/spawn.test.ts (flips the "unref() + .exited with nothing else ref'd" block to expect exit 13), and a new 9-case test file.
Security risks
None identified. No parsing of untrusted input, no auth/crypto, no new FFI surface. The one memory-safety concern (unrooted JSInternalPromise deref after GC) was raised in an earlier round and is now addressed with an RAII Protected guard whose lifetime spans the new read.
Level of scrutiny
High. This is the main-thread process lifecycle: it changes when load_entry_point returns, what state the entry promise can be in at each downstream check, how the exit code is derived, and how the entry JSInternalPromise is kept alive across arbitrary user JS in on_before_exit(). It also deliberately changes user-visible behavior — top-level await on an unref'd subprocess now exits 13 (Node parity) instead of eventually resolving via the old busy-spin — and rewrites three existing test assertions to match. Per REVIEW.md, silently weakening/rewriting existing tests needs a stated reason (which the PR gives: the old behavior was an accident of the busy-spin, and the #34478 IOCP concern is still covered by the non-TLA block above), but a maintainer should confirm that trade-off is acceptable.
Other factors
- Four prior inline findings from earlier runs (Pending-only check swallowing late rejections; spurious TLA warning when
unhandled_error_counter > 0; unrooted promise pointer; spawn.test.ts CI failures) were each fixed in follow-up commits with new test coverage, and CodeRabbit's assertion-ordering nits were applied. - The two sibling
wait_for_promisesites (--preload, test runner) are explicitly deferred in the PR description with issue references, satisfying REVIEW.md's "if a site is intentionally excluded, say so". - I checked that
vm.pending_internal_promiseat the new deref is the same cell that_entry_promise_protectedroots on every non-watcher store path inreload_entry_point(lines 2346/2370/2395/2406), and that thereported_at != hot_reload_counterguard correctly skips re-reporting when line 1510 already handled the initial rejection. - Not a simple/mechanical change; deferring rather than approving.
|
Closing as a duplicate of #30551, the PR being kept for this behavior. It makes the same One thing from here worth carrying over when #30551 is rebased: the note that a top-level Still reproducible on main at 165dc9f: the entry case never exits and spins at 100% CPU. #33283 stays open. |
Repro
strace confirms the before state is a pure user-space busy re-tick: the main thread is
R, the startup epoll fd is never waited on, andauto_tick→tick_without_idle()→us_loop_run_bun_tickearly-returns onnum_polls == 0every iteration ofwait_for_promise.Cause
Run::start→VirtualMachine::load_entry_point(non-watcher arm) callswait_for_promise, which loopstick()/auto_tick()until the entry promise settles with no liveness check. When the top-level await can never settle and nothing else refs the loop, this spins forever andload_entry_pointnever returns, so the core run-loop andon_before_exitare never reached.The worker entry path already handles this:
wait_for_promise_with_terminationbreaks once!is_event_loop_alive()with the entry promise still pending, andweb_worker.rssetsexit_code = 13.Fix
Give the main entry the same treatment (mirroring the worker path):
load_entry_point's non-watcher wait now breaks once!is_event_loop_alive()with the promise still Pending, so it returns instead of spinning.on_before_exit(),Run::startcheckspending_internal_promise:Detected unsettled top-level await at <entry path>warning and setexit_code = 13(unless the user already set one);beforeExitlistener) → route throughuncaught_exceptionso the error is reported and exit 1, same as an initial-load rejection. Without this arm a late rejection was swallowed and the process exited 0.The check sits after
on_before_exit()sobeforeExitfires with code 0 first, matching Node's order. The warning names the entry path (JSC does not expose a stalled-module API like V8's, so no:lineyet).--watch/--hotare unchanged.Behavior change: top-level
awaiton an unref'd subprocess (p.unref(); await p.exited) now exits 13 like Node. Previously the busy-spin happened to pick up the exit and resolve.spawn.test.ts's(Windows)block is updated accordingly; the underlying IOCP-unref'd-handle fix (#34478) is still covered by the non-TLA "should not hang" block above it.Intentionally not in this PR (same bug class, pre-existing, left for a follow-up so this stays reviewable):
--preloadwith an unsettled TLA (jsc_hooks.rs:836) andbun teston a file with module-levelawait new Promise(()=>{})(load_entry_point_for_test_runner,VirtualMachine.rs:4608) still call the unboundedwait_for_promise. Those callers each need their own Pending-handling (mark the preload/test file as failed and continue); #33286 / #36052 cover that ground.Verification
test/js/node/process/unsettled-top-level-await.test.ts(8 cases, 3s watchdog):await new Promise(() => {})→ exit 13 + warningsetTimeout(50)(ref'd timer) → exit 0, no warningqueueMicrotask→ exit 0, no warningprocess.exitCode = 7then unsettled TLA → exit 7 (user's code preserved)beforeExitlistener fires with0before the warning → exit 13bun -e 'await new Promise(()=>{})'→ exit 13beforeExithandler rejects the awaited promise → error reported, exit 1All pass with the fix; 6 of them hang (→ SIGTERM at the watchdog) on the released build.
worker-top-level-await.test.ts,dynamic-import-tla-cycle.test.ts,run-eval.test.ts,require-esm-transitive-tla.test.ts, and the updatedspawn.test.tsblock pass.Fixes #33283. Minimal subset of #33286 (which also reworks
--printordering, adds abeforeExit-resolves-TLA drain loop, a C++ stalled-module walk, and covers preload/test-runner); those can follow separately.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/spawn/spawn.test.ts