Exit on unsettled top-level await instead of hanging (@inquirer/prompts)#30601
Exit on unsettled top-level await instead of hanging (@inquirer/prompts)#30601robobun wants to merge 7 commits into
Conversation
Fixes the @inquirer/prompts hang in #17636. Three pieces, all needed for Node parity on the reported flow (stdin closes -> readline closes -> top-level await never settles -> signal-exit rejects the prompt): 1. loadEntryPoint(): break out of the TLA wait once the event loop has nothing left that could settle it, instead of busy-spinning on tickWithoutIdle at 100% CPU forever. 2. Run.start(): fold onBeforeExit into the drain loop so a beforeExit listener that settles the TLA can resume it; otherwise map a still-pending entry promise to exit code 13 with a warning. 3. BunProcess.cpp: when the user has replaced process.emit as an own property (signal-exit does this), dispatch 'beforeExit' and 'exit' through that override so it can observe shutdown. Drain microtasks once after 'exit' so a promise rejected from an exit handler reaches its .catch() before the process dies.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughReworks VM shutdown to tick-drain unsettled top-level await, inspects pending/rejected TLA to set exit codes (13/1), routes beforeExit/exit through a user-installed process.emit override when present, drains microtasks after exit, and adds regression tests for the behaviors. ChangesTop-Level Await Shutdown Behavior
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 10:27 PM PT - May 12th, 2026
❌ @autofix-ci[bot], your commit 74f9aa4 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 30601That installs a local version of the PR into your bun-30601 --bun |
|
Found 5 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/bindings/BunProcess.cpp (1)
892-905:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrain
nextTickwhenever a userprocess.emitoverride runs for"beforeExit"
wrapped().emit()returningfalsemeans no listeners ran, but an arbitrary JS override does not carry that guarantee. A monkey-patchedprocess.emitcan enqueueprocess.nextTick(...)during"beforeExit"and still returnfalse/undefined, so the currentif (fired)gate can skip queued work and prevent the pending TLA from ever resuming on shutdown.Suggested change
- bool fired; + bool shouldDrainNextTick = false; if (JSC::JSValue userEmit = userEmitOverride(vm, process)) { - fired = callUserEmitOverride(globalObject, process, userEmit, "beforeExit"_s, jsNumber(exitCode)); + callUserEmitOverride(globalObject, process, userEmit, "beforeExit"_s, jsNumber(exitCode)); + shouldDrainNextTick = true; } else { MarkedArgumentBuffer arguments; arguments.append(jsNumber(exitCode)); - fired = process->wrapped().emit(Identifier::fromString(vm, "beforeExit"_s), arguments); + shouldDrainNextTick = process->wrapped().emit(Identifier::fromString(vm, "beforeExit"_s), arguments); } - if (fired) { + if (shouldDrainNextTick) { if (globalObject->m_nextTickQueue) { auto nextTickQueue = globalObject->m_nextTickQueue.get(); nextTickQueue->drain(vm, globalObject); } }🤖 Prompt for 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. In `@src/jsc/bindings/BunProcess.cpp` around lines 892 - 905, The current logic only drains globalObject->m_nextTickQueue when the emit call reports listeners ran (fired), but a user-provided override (userEmitOverride / callUserEmitOverride) can enqueue nextTick callbacks even when it returns false/undefined; change the control flow so that when userEmitOverride(vm, process) is used (i.e., you call callUserEmitOverride), you always drain the nextTick queue afterwards regardless of the returned fired value, while preserving the existing behavior for the wrapped().emit path; use the same nextTickQueue->drain(vm, globalObject) call and m_nextTickQueue check to perform the drain.
🤖 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.
Outside diff comments:
In `@src/jsc/bindings/BunProcess.cpp`:
- Around line 892-905: The current logic only drains
globalObject->m_nextTickQueue when the emit call reports listeners ran (fired),
but a user-provided override (userEmitOverride / callUserEmitOverride) can
enqueue nextTick callbacks even when it returns false/undefined; change the
control flow so that when userEmitOverride(vm, process) is used (i.e., you call
callUserEmitOverride), you always drain the nextTick queue afterwards regardless
of the returned fired value, while preserving the existing behavior for the
wrapped().emit path; use the same nextTickQueue->drain(vm, globalObject) call
and m_nextTickQueue check to perform the drain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c744462e-ec44-4931-930a-02eed28f2bbe
📒 Files selected for processing (4)
src/bun.js.zigsrc/jsc/VirtualMachine.zigsrc/jsc/bindings/BunProcess.cpptest/regression/issue/17636.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Node drains Promise microtasks after the 'exit' event (so that .catch()/.finally() reactions on promises rejected by exit handlers run), but never process.nextTick — nextTick() is a no-op once _exiting is set and anything already queued is dropped. The previous commit used GlobalObject::drainMicrotasks() which also drains the nextTick queue, causing a nextTick queued from handleRejectedPromises() (which runs at the very end of tick()) to fire with _exiting=true. That broke test/js/node/test/parallel/test-event-capture-rejections.js whose common.mustCall() guards on process._exiting. Also: when a user process.emit override runs for 'beforeExit', drain nextTick regardless of its return value — an override's falsy return does not mean it did no work.
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 `@test/regression/issue/17636.test.ts`:
- Around line 71-73: The test currently uses loose contains checks on the parsed
stdout array (`seen` from JSON.parse(stdout.trim())) which allows wrong order or
extra events; replace the two expect(...toContain(...)) assertions with a single
exact equality assertion that `seen` equals the precise ordered array of
shutdown events (e.g., ["beforeExit:0","exit:0"]) so the test enforces both
order and no extra emissions; update the assertions around the `seen` variable
accordingly.
🪄 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: 620e09b7-4211-44b3-89b3-305c4047fd6f
📒 Files selected for processing (2)
src/jsc/bindings/BunProcess.cpptest/regression/issue/17636.test.ts
Six concurrent ASAN debug-build subprocesses contend enough that the readline/stdin cases exceed the default 5s timeout. Use 30s under debug builds, 10s otherwise.
| while (true) { | ||
| while (vm.isEventLoopAlive()) { | ||
| vm.tick(); | ||
| vm.eventLoop().autoTickActive(); | ||
| } | ||
|
|
||
| vm.onBeforeExit(); | ||
|
|
||
| // loadEntryPoint() may have returned with the module's | ||
| // evaluation promise still pending (nothing left in the | ||
| // event loop that could settle it). A beforeExit listener | ||
| // may have just scheduled work that will settle it — | ||
| // drain once and re-enter if so. Otherwise fall through | ||
| // and the still-pending promise becomes exit code 13. | ||
| if (vm.pending_internal_promise) |p| { | ||
| if (p.status() == .pending) { | ||
| vm.tick(); | ||
| if (vm.isEventLoopAlive()) continue; | ||
| } | ||
| } | ||
| break; |
There was a problem hiding this comment.
🟡 Moving vm.onBeforeExit() into the outer while (true) (and removing the trailing call after the eval_and_print block) flips the output order for bun --print when a beforeExit listener writes to stdout: bun -p '(process.on("beforeExit", () => console.log("BE")), 42)' now prints BE\n42 instead of Node's / pre-PR Bun's 42\nBE. Very minor (--print + a beforeExit listener is uncommon and run-eval.test.ts doesn't cover it), but worth noting in case you want to keep a trailing onBeforeExit() after the eval_and_print block.
Extended reasoning...
What changed
Pre-PR ordering in the non-watcher branch of Run.start() was:
- main drain loop (
while (vm.isEventLoopAlive()) { tick; autoTickActive; }) eval_and_printblock (prints the-presult, with its own drain loop for a pending result promise)vm.onBeforeExit()
The PR moves vm.onBeforeExit() inside the new outer while (true) loop (src/bun.js.zig:504) and deletes the trailing call after the eval_and_print block (the old line at the end of that block was replaced by the late-TLA / exit-13 check). So beforeExit now fires before the eval_and_print block runs.
Observable consequence
For bun --print <expr>, any beforeExit listener that writes to stdout now interleaves before the printed result instead of after. This diverges from Node's -p semantics, where the expression result is printed synchronously, then the event loop runs, then beforeExit fires on natural shutdown.
Step-by-step proof
bun -p '(process.on("beforeExit", () => console.log("BE")), 42)'Node / pre-PR Bun:
- Expression evaluates synchronously → result is
42,beforeExitlistener registered. - Main drain loop: nothing alive → exits immediately.
eval_and_printblock:vm.entry_point_result.valueis42(not a promise) →to_print.print(...)writes42\n.vm.onBeforeExit()fires → listener writesBE\n.- Output:
42\nBE\n.
Post-PR Bun:
- Expression evaluates synchronously → result is
42. - Outer
while (true): inner drain loop exits immediately, thenvm.onBeforeExit()fires → listener writesBE\n.pending_internal_promiseis fulfilled (the-esource has no TLA), so the loopbreaks. eval_and_printblock prints42\n.- Output:
BE\n42\n.
Why existing code/tests don't catch it
run-eval.test.ts (33/33 pass per the PR description) doesn't combine --print with a beforeExit listener, and the new regression tests in 17636.test.ts all use -e, not -p. The reordering is a side-effect of folding onBeforeExit() into the drain loop (which is necessary for the TLA fix) without re-adding a trailing dispatch after eval_and_print.
Impact and fix
Impact is very low — bun --print + a registered beforeExit listener is an uncommon combination, and one could argue beforeExit-before-print is semantically defensible since the print is a runtime post-shutdown step. But it is an undocumented Node-parity ordering change introduced by this PR. If you want to preserve the old ordering, the simplest fix is to add a single trailing vm.onBeforeExit() after the eval_and_print block (before the late-TLA check), so that any side effects of printing — and the print itself — happen before the final beforeExit round. Not a blocker.
|
CI status (build #53948): the diff is green — my regression test The two red lanes are pre-existing Windows flakes unrelated to anything this PR touches:
This PR only touches Ready for review/merge. |
|
Verified this also fixes #6592 ( Repro: in a PTY, run
|
1 participant
Fixes #17636
Fixes #14951
Fixes #29194
Fixes #12918
Repro
Root cause
When stdin closes, readline closes, and nothing is left in the event loop that could settle the prompt's top-level
await. Three things then go wrong compared to Node:loadEntryPointbusy-spins forever. It waits on the entry module's evaluation promise viawaitForPromise, which loopstick(); autoTick();until the promise settles. With an idle loopautoTickfalls intotickWithoutIdle()(zero-timeout poll) → 100% CPU spin. Node instead stops waiting, firesbeforeExit, and maps the still-pending promise to exit code 13.process.emitoverrides are bypassed forbeforeExit/exit.signal-exit(pulled in by inquirer) monkey-patchesprocess.emitto observe shutdown and reject the pending prompt. Bun dispatched these two events through the C++EventEmitter::emitonprocess->wrapped()directly, so the override never ran and signal-exit never fired its callback.Microtasks aren't drained after
'exit'. Even once signal-exit rejects the prompt, the.catch(() => process.exit(0))is a microtask that Bun never ran before dying. Node drains once after'exit'.Fix
src/jsc/VirtualMachine.zig—loadEntryPoint: replace the unconditionalwaitForPromisewith a loop that also breaks whenisEventLoopAlive()is false, so an idle loop returns to the caller with a still-pending promise instead of spinning.src/bun.js.zig— main run loop: foldonBeforeExit()into the drain loop so abeforeExitlistener that settles the TLA can resume it; after the loop fully drains, map a still-.pendingentry promise to a warning + exit 13 (or report a late.rejected).src/jsc/bindings/BunProcess.cpp—dispatchExitInternal/Process__dispatchOnBeforeExit: ifprocess.emitwas replaced as an own property, call it (matching Node). Setprocess._exitingunconditionally. Drain microtasks once after'exit'so shutdown-time promise reactions reach their handlers.await new Promise(() => {})process.exitCode = 42; await new Promise(() => {})beforeExithandler settles the TLAprocess.emit = fnoverride observesbeforeExit/exit'exit'listener runssearch()on stdin close.catch()runs, exit 0.catch()runs, exit 0Verification
bun bd test test/regression/issue/17636.test.ts— 6 passUSE_SYSTEM_BUN=1 bun test test/regression/issue/17636.test.ts— 6 fail (hang/timeout on unfixed bun)@inquirer/promptsrepro now exits 0 with the sameCAUGHT:message Node printstest/fixtures/es-modules/tla/{resolved,unresolved,unresolved-withexitcode,unresolved-with-listener,rejected,rejected-withexitcode,process-exit}.mjs— all match Node's exit codes and listener outputbun bd test test/js/node/process/process.test.js -t onBeforeExit— 3 passbun bd test test/js/node/process/process.test.js -t exitCode— 22 passbun bd test test/js/bun/resolve/{require-esm-transitive-tla,dynamic-import-tla-cycle}.test.ts— 7 passbun bd test test/cli/run/run-eval.test.ts— 33 passbun bd test/js/node/test/parallel/test-process-beforeexit*.js— passbun run zig:check-all— passRelated
Overlaps with / supersedes the stale #29196 (conflicts with main) and the TLA portion of #29739 / #29549 / #27215. This PR bundles the three pieces that together produce Node-equivalent behaviour for the @inquirer/prompts flow and adds a regression test for the end-to-end signal-exit pattern.