Exit 13 on an unsettled top-level await instead of hanging - #33286
Exit 13 on an unsettled top-level await instead of hanging#33286robobun wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds unsettled top-level await handling across the VM, CLI, native module scan, and tests. It changes entry-point waiting, emits stalled-module warnings, updates exit handling, and adds coverage for settled and unsettled cases. ChangesUnsettled top-level await detection and reporting
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 1:41 PM PT - Jul 22nd, 2026
❌ @robobun, your commit af8cb03 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 33286That installs a local version of the PR into your bun-33286 --bun |
a7392af to
eff8f48
Compare
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Flagged relationships are expected and called out in the description. This PR is the intended landing for #33283, not a redundant duplicate:
So #33286 supersedes the three rather than duplicating them; the others can be closed once this lands. |
35cb774 to
f92358c
Compare
f92358c to
443c719
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/bindings/ZigGlobalObject.cpp`:
- Around line 699-703: The new header comment in
ZigGlobalObject::findStalledTopLevelAwaitModuleSpecifiers is too long and must
be trimmed to the 3-line limit. Condense the existing explanation so it still
captures the invariant about stalled TLA module specifiers, the
EvaluatingAsync/syntactic TLA condition, and that the result is an empty
BunString when nothing is stalled, while keeping the comment block to at most
three lines.
- Around line 720-722: The module specifier framing in the ZigGlobalObject
builder is unsafe because newline-separated values can be split incorrectly when
a specifier/path contains a newline. Update the code around the builder append
logic to stop using “\n” as the separator and instead emit a delimiter that
cannot appear in filesystem paths, or better, return a structured collection.
Then update the Rust consumer in VirtualMachine.rs to parse the new framing
format using the existing module-specifier handling path so it no longer
generates bogus warnings.
In `@src/jsc/VirtualMachine.rs`:
- Around line 1059-1069: Several newly added doc comments exceed the
repository’s 3-line limit and should be condensed. Shorten the explanatory
blocks around the helper logic in VirtualMachine (including the comments near
wait_for_module_promise and is_event_loop_alive) to no more than 3 lines each,
keeping only the essential rationale; move any extra background to the PR
description. Apply the same trimming to the other flagged comment blocks in this
diff so all comments stay within the repo guideline.
🪄 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: 927daaae-5905-452b-b12f-8d5768c09691
📒 Files selected for processing (4)
src/jsc/VirtualMachine.rssrc/jsc/bindings/ZigGlobalObject.cppsrc/runtime/cli/run_command.rstest/js/node/process/unsettled-top-level-await.test.ts
443c719 to
ea83d0b
Compare
59d26e1 to
10d2dcc
Compare
10d2dcc to
1f6207f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 111-124: The stalled-sibling warning test only checks for the
module names and exit code, but it does not verify the warning’s marker text.
Update the unsettled top-level await test in the relevant
`run`/`expect(r.stderr)` assertions to also check for the “Detected unsettled
top-level await” message, matching the existing sibling test and ensuring the
warning format is covered.
- Around line 4-9: Trim the leading comment in unsettled-top-level-await.test.ts
so it only briefly states the test purpose and expected Node behavior; remove
the issue URL, historical hang narration, and other bug-background details. Keep
the test body focused on setup/action/assertions, and ensure any remaining
comment is at most 3 lines while still pointing to the top-level await behavior
being verified.
- Around line 164-177: The unsettled top-level await test is using a weak stdout
assertion in the Bun.spawn scenario. Update the test in
unsettled-top-level-await.test.ts to assert that stdout is exactly empty for the
bun -p case, using the existing test body around Bun.spawn, proc.stdout.text(),
and the exitCode assertions, instead of only checking that stdout does not
contain "Promise".
🪄 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: 902cb4f9-2840-47e3-8412-8fd2637fa102
📒 Files selected for processing (4)
src/jsc/VirtualMachine.rssrc/jsc/bindings/ZigGlobalObject.cppsrc/runtime/cli/run_command.rstest/js/node/process/unsettled-top-level-await.test.ts
1f6207f to
831adff
Compare
831adff to
731b560
Compare
c0bc147 to
7c94bc0
Compare
7c94bc0 to
603a1c2
Compare
|
Status: ready for review. The diff is green, all review threads are resolved, and the mechanical gate passes (ASAN build runs the new test file: fails without the fix, passes with it). The red CI lanes are pre-existing flake unrelated to this change. Across builds #68097/#68102/#68106/#68121/#68146/#68150 the failures are a rotating set of
|
When the main entry module's top-level await never settles and nothing
else refs the event loop, Bun spun the entry loader forever (100% CPU)
instead of exiting. Node prints a warning and exits with code 13.
Detect the idle-loop + pending-entry-promise condition: the entry loader
(wait_for_module_promise) returns once no active handle, task, or ref
could still settle the promise, and Run::start reports the stalled
module(s) and exits 13. A beforeExit handler still gets a chance to
resolve the await first (Node parity), and --print emits the resolved
value once the entry settles rather than a pending promise. If the
resumed body then throws, the error is reported (running a
process.on('uncaughtException') handler if present, else exit 1),
matching Node. Scoped to the main entry; the test runner and workers
are unchanged.
603a1c2 to
af8cb03
Compare
|
Rebased onto main (af8cb03, was 488 commits behind). Added a 14th test case for |
There was a problem hiding this comment.
No new issues found on 603a1c2 — all prior threads are resolved. Deferring to a human reviewer: this rewrites the main-entry shutdown sequence in Run::start (ordering of --print vs beforeExit, a new resolution loop, and late-rejection reporting), which is core-path enough that the intentional divergences called out in the description (best-effort beforeExit/uncaughtException interleaving, exit_code == 0 vs Node's nullish check, --watch/--preload deferred) deserve a maintainer sign-off.
What was reviewed
wait_for_module_promise/has_pending_loop_work: confirmed a ref'd timer keeps the loop alive (covered by the exit-0 test); the raw-ptrjsc_vmread avoids the borrow conflict as commented.Bun__findStalledTopLevelAwait: no JS entry / no ThrowScope needed;toStringRefownership is released viastalled.deref()on the Rust side.- The
Rejectedarm'spending_internal_promise_reported_atgate — verified it prevents double-reporting an initial-load rejection already handled inload_entry_point. - Test file: hang-guard via
signalCode === null, pipes drained concurrently,describe.concurrentfor the spawn matrix.
Extended reasoning...
Overview
Four files: VirtualMachine.rs adds has_pending_loop_work(), wait_for_module_promise(), entry_point_evaluation_is_pending(), and report_unsettled_top_level_await(), and swaps wait_for_promise → wait_for_module_promise in the non-watcher arm of load_entry_point. ZigGlobalObject.cpp adds Bun__findStalledTopLevelAwait, which walks the JSC module map for EvaluatingAsync records with syntactic TLA and no pending async dependency. run_command.rs restructures the tail of Run::start: the old inline --print block is extracted to print_eval_result() + entry_point_print_ok(), on_before_exit() now fires before the entry-status check, a new resolution loop lets a beforeExit handler settle the TLA, and a status match reports Pending (warn + exit 13) or late Rejected (route through uncaught_exception). A 13-case test file spawns fixtures with a 20s kill timeout and asserts signalCode === null so a reintroduced hang fails cleanly.
Security risks
None identified. The new C++ function only reads the module registry (no JS callouts, no allocation from untrusted input); the NUL-join delimiter is safe because module specifiers cannot contain NUL. The Rust side dereferences jsc_vm/global/pending_internal_promise under documented SAFETY invariants that match existing usage in the same file. BunString returned from C++ is explicitly deref()'d after use.
Level of scrutiny
High. Run::start is the process-lifetime path for every bun <file> / bun -e / bun -p invocation, and this PR reorders --print, beforeExit, and the final drain relative to each other. The review history on this PR already surfaced three separate ordering edge cases (late-throw vs body-scheduled timers, Fulfilled-entry dropping a second beforeExit, nextTick-initialized drain path), one of which led to a revert (c0bc147 → 603a1c2). The author has documented the residual ordering divergence as best-effort, and named --watch/--hot and --preload as intentionally deferred sibling arms — those are exactly the kind of scoping calls a maintainer should ratify rather than a bot.
Other factors
Test coverage is thorough for the stated scope (never-resolving promise, TLA cycle, self-import, unref'd timer, ref'd-timer resolution, beforeExit resolution, leaf-module attribution, sibling listing, late throw with and without an uncaughtException handler, -p/-e). All 13+ prior inline threads on this PR are marked resolved; the last one (explicit process.exitCode = 0 → exit 13 vs Node's exit 0) was closed as an intentional divergence. The bug-hunting pass on the current head found nothing new. Given the surface area and the explicit design trade-offs, this warrants a human look before merge.
|
Closing as a duplicate of #30551. That PR carries the same main-entry change ( Still reproducible on main at 165dc9f: the entry case never exits and spins at 100% CPU instead of exiting 13. #33283 stays open and is listed on #30551. |
Fixes #33283
Repro
The self-import variant (
await import(import.meta.url), orawait import("bun:main")from the entry) is the same spec-level deadlock: the entry awaits its own evaluation promise.Cause
The main-entry loader waits on the entry module's evaluation promise via
wait_for_promise, which loopstick()+auto_tick()until the promise settles. When the top-level await can never settle and no ref'd handle remains,auto_tick()takes the!loop.is_active()branch (non-blockingtick_without_idle), so the wait degenerates into a busy-spin that never returns. Node stops waiting once the loop would otherwise block with the module promise still pending, prints a warning, and exits 13.Fix
Scoped to the main entry (
bun <file>,bun -e,bun -p); the test runner and workers keep their own termination semantics and are unchanged.wait_for_module_promisereplaceswait_for_promiseinload_entry_point. Same tick/auto_tick loop, but it returns with the promise still pending oncehas_pending_loop_work()is false (nothing active, no task/ref/immediate could settle it). A ref'd timer keeps the loop alive, so a TLA that a timer later resolves still works.Run::startdetects the still-pending entry promise afterbeforeExitand reports + exits 13. AbeforeExithandler still gets a chance to resolve the await first (and may schedule more work), matching Node's repeated-beforeExitbehavior.--printemits the resolved value once the entry settles (and nothing for an unsettled one), never a bogusPromise { <pending> }. A late rejection (the resumed body throws) is reported viauncaughtExceptionand exits 1 (or 0 if a handler swallows it).Bun__findStalledTopLevelAwait(C++) walks the module registry to name the actual stalled module(s) (statusEvaluatingAsync, syntactic TLA, not waiting on an async dependency), so the warning points at the leaf rather than the entry.Warning goes to stderr:
Warning: Detected unsettled top-level await at <module>, exit code 13. The source line + caret that Node also prints is omitted; the warning text and exit code are the parity that matters.Intentionally deferred
--preloadscripts with an unsettled top-level await still hang.load_preloadsruns per-preload before the entry and would need its own "still pending" return value propagated back throughreload_entry_point(and ideally name which preload stalled), which is the broader surface #30551 took on. Keeping this PR to the main entry per #33283; preloads are a follow-up.bun --watch/--hotwith an unsettled top-level await also still busy-spins: the watcher arm ofload_entry_pointkeeps its owntick/auto_tickloop, and the right fix there is break-to-idle (return to the watcher's block-wait for file changes), not exit 13, since a watch process should stay alive. Deferred to a follow-up for the same reason.When a
beforeExithandler resolves a top-level await and the resumed body then schedules timers and/or throws, the exact interleaving of those timers vsuncaughtExceptionvs repeatedbeforeExitemissions is best-effort rather than byte-for-byte Node parity: the module can resume insideon_before_exit's own drain (especially onceprocess.nextTickhas initialized its queue), which runs ahead of the report below. The observable outcome that matters (exit code 0/1 and the error message) is correct; matching Node's precise ordering in those multi-way cases would need the drain to be owned by a single loop, which is a follow-up.Verification
test/js/node/process/unsettled-top-level-await.test.ts(14 cases): never-resolving promise, dynamic-import TLA cycle,import(import.meta.url)self-import, and unref'd timer all exit 13; a ref'd timer that settles and abeforeExithandler that settles both exit 0;bun -p/bun -eexit 13 on an unsettled await (and-pprints nothing, notPromise { <pending> }); the warning names the stalled leaf and lists stalled siblings; a resumed body that throws exits 1 (or 0 viauncaughtExceptionhandler). The spin cases hang on the released build and pass after the fix.Prior art
This is the exit-13 half of #14951; the broader CPU-spin fix for condition-gated drive loops is tracked separately in #32014. It supersedes the broader/stale attempts #30551 (which also changed the test runner and preloads) and #30601 (inquirer-specific), and ports @dylan-conway's pre-Rust #29739 to the current tree, scoped to the main entry as #33283 asks.
[review] gate passed · iteration 4 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 4
evidence per changed file
root cause · written by the author bot
The root cause was that Bun's promise-waiting path in load_entry_point kept the event loop ref'd while awaiting the entry module's top-level await, so a promise that never settles left the process parked indefinitely instead of exiting, diverging from Node's behavior of warning and exiting with code 13. The fix introduces a module-specific wait that detects when the entry promise is still pending and nothing else refs the event loop, uses a native module-map scan to identify the stalled async module and its awaiting location, and restructures the CLI shutdown path to emit the unsettled top-…