bun --print: exit 1 and run exit listeners when the result promise is still pending after the event loop stops - #39175
Conversation
|
Updated 12:04 PM PT - Aug 15th, 2026
🔄 @robobun, the build for your commit |
|
Warning Review limit reached
Next review available in: 12 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. 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 (7)
Comment |
|
Status: reproduced and fixed in this PR; ready for review.
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes process exit semantics for --print (exit code, exit listeners, unhandled-rejection reporting) and the description flags coordination with several open PRs (#39128, #37173, #36448, #38116, #38524), a human look would still be worthwhile.
What was reviewed:
run_command.rs: the new status-re-read replaces thethen2reactions and falls through to the existing exit sequence;resultstays rooted viaentry_point_result.valueacross the extra tick.- Dead-code removal is complete: no remaining references to
on(Resolve|Reject)EntryPointResultanywhere; theHOST_EXPORTmarker deletion meansgenerate-host-exports.tsno longer emits the thunks. PromiseFunctionsenum now has 46 entries, matchingpromiseFunctionsSize = 46;promiseHandlerIDarms updated consistently.- Tests: six cases cover fulfil/reject × (unref'd timer / early rejection / early throw) plus a never-settling guard; each uses
Bun.sleepSyncso the settling timer is overdue but not runnable in the main loop.
Extended reasoning...
Overview
The PR removes the then2-based promise reactions (Bun__onResolveEntryPointResult / Bun__onRejectEntryPointResult) that bun --print attached to a still-pending result promise after the main loop drained. Those reactions called Global::exit() from inside the promise job, bypassing on_before_exit(), handle_rejected_promises(), on_exit(), the ANY_UNHANDLED → exit 1 step, and global_exit(). The fix re-reads promise.status() after the extra tick and prints from the same site as the already-settled case, then falls through to the one exit sequence. Dead code is deleted across hw_exports.rs, ZigGlobalObject.{h,cpp}, headers.h, and mordant-baseline.toml. Six new tests in run-eval.test.ts cover the variant matrix.
Security risks
None. This is CLI exit-path control flow; no untrusted input parsing, auth, crypto, or network surface is touched.
Level of scrutiny
Medium-high. The logic change itself is small and clearly a simplification (two exit paths → one), and the test coverage is thorough with a careful anti-flake construction (Bun.sleepSync makes the settling timer overdue but not main-loop-runnable). However, process exit semantics — exit code, exit listener firing, whether a late rejection is reported — are user-observable and load-bearing for scripts and CI. The PromiseFunctions enum index shift is internal-only (indices into m_thenables, never persisted), and the author verified the other slot users (html-rewriter, cron, etc.) still pass; I confirmed the new count (46) matches the enum length and that promiseHandlerID's if-chain was updated to match.
Other factors
The description flags interactions with five open PRs. In particular #38524 would change what the error-case tests here print (Promise { <pending> } instead of the settled value), and #39128 is superseded by this change. That coordination is a maintainer decision, not something the automated review can settle. Given the semantic surface (exit codes for -p) and the open-PR interactions, deferring rather than shadow-approving.
…ding When the --print result was still pending after the event loop was done, Run::start attached two native reactions to it and ticked once more; the reaction that ran printed the settled value and called Global::exit from inside the promise job, skipping on_before_exit, handle_rejected_promises, on_exit, the unhandled-error exit code and global_exit. Attaching them also marked the result promise handled, so its own late rejection was never reported. Re-read the promise's status after the extra turn instead and print from the one existing site, so the run ends through the regular exit sequence. The extra turn is now only taken when the loop drained on its own. After an unhandled error stopped it, the turn ran whatever of the script's timers came due before the next internal wakeup, so what --print printed (and, via the reactions, the exit code) depended on the timer's length; as in on_before_exit, nothing of the script runs after such an error. Removes the reactions, their PromiseFunctions slots and the mordant baseline entry for them.
dbdd9bb to
82b9967
Compare
There was a problem hiding this comment.
LGTM — targeted bugfix that removes the second --print exit path so the result promise's late settlement goes through the normal Run::start exit sequence.
What was reviewed:
run_command.rs: the extra turn is now gated onunhandled_error_counter == 0(same guard ason_before_exit), status is re-read after, and control falls through to the existing print + exit sequence — no path skipson_exit/ANY_UNHANDLED/global_exitanymore.- Dead-code removal is complete: grep confirms no remaining references to
Bun__on{Resolve,Reject}EntryPointResult;PromiseFunctionsenum count matches the updatedpromiseFunctionsSize = 46; theHOST_EXPORTmarkers are gone so codegen drops the thunks. - Tests use
Bun.sleepSyncto make the settling timer overdue before the loop is inspected, so the unref'd-timer and error-stopped cases are deterministic; the libuv-timers-run-twice case correctly relaxes its stdout assertion for Windows.
Extended reasoning...
Overview
This PR fixes bun --print to route through the normal exit sequence when the result promise is still pending after the main event loop finishes. Previously, Run::start attached native then2 reactions (Bun__onResolveEntryPointResult / Bun__onRejectEntryPointResult) that called Global::exit() directly from inside the promise job — bypassing on_before_exit, handle_rejected_promises, on_exit (the 'exit' event), the ANY_UNHANDLED → exit 1 step, and VM teardown. The fix drops the reactions entirely, keeps the one extra loop turn only when the loop drained cleanly (unhandled_error_counter == 0), re-reads the promise status afterward, and lets the single existing print + exit path handle every case. Files touched: run_command.rs (the logic), hw_exports.rs (delete the two reaction bodies), ZigGlobalObject.{h,cpp} + headers.h (remove the enum entries, size constant, and declarations), mordant-baseline.toml (drop the now-gone lint entry), and run-eval.test.ts (six new tests).
Security risks
None. No parsing of untrusted input, no auth/crypto/permissions surface. The change narrows control flow on a CLI-only path.
Level of scrutiny
Medium. This is the process exit path for bun --print, so incorrect ordering could skip 'exit' listeners or misreport exit codes — but the change is a strict simplification that removes a divergent exit path rather than adding one. The new flow is: the same tick/auto_tick_active/while-alive shape the main loop already uses, then the same status→result unwrap the already-settled case always had, then fall through. The unhandled_error_counter == 0 guard matches the precedent in on_before_exit (#34639) and is_event_loop_alive itself.
Other factors
- Verified via grep that no references to the removed symbols remain anywhere in the tree;
generate-host-exports.tsscrapes// HOST_EXPORTmarkers so the generated thunks disappear with the source functions. - Counted the remaining
PromiseFunctionsenum entries againstpromiseFunctionsSize = 46— they match;ThenablesArraysizes off that constant so no OOB. - The tests are placed in the existing
run-eval.test.ts, usedescribe.concurrentwith async spawns, drain both pipes andexitedtogether, and are deterministic by construction (each script blocks inBun.sleepSyncpast the timer's due time so the extra turn either fires it or provably doesn't). The Windows/libuv edge case (timers run again after poll in the same turn) is called out and the affected test asserts only what holds on every platform. - User-visible behavior change (after a fatal error,
--printnow showsPromise { <pending> }and does not run the settling timer) is a deliberate alignment withbun -eandnode -p, and is what makes the exit code deterministic. - The PR description documents that the whole file plus consumers of the shifted
PromiseFunctionsindices (globals.test.js,html-rewriter.test.js,in-process-cron.test.ts,no-addons.test.ts) were run green, and the new block was verified to fail on main and under the ASAN lanes' LSan settings.
Problem
bun --print(-p) exits 0 after reporting an unhandled rejection when the result promise is settled by a timer that comes due shortly after.bun -ewith the same script exits 1.Promise { <pending> }and exits 1, because an internal wakeup (the GC timer, about 100ms in) ends the wait first; withBUN_GC_TIMER_DISABLE=1a 500ms timer runs too and the exit code is 0 again. So after a fatal error, what--printprints, whether more of the script runs, and the exit code all depend on timing.process.on("exit")listeners never run (also after an uncaught exception thrown from a timer, which does exit 1), and a result promise that rejects late is printed like a value and exits 0 instead of being reported.new Promise(r => { setTimeout(() => r(1), 1).unref(); Bun.sleepSync(10) })) takes the same path, so its exit listeners are skipped too, and under the ASAN lanes' settings (BUN_DESTRUCT_VM_ON_EXIT=1plus LSan) every script on this path aborts at exit with a leak report because the VM teardown is skipped as well (why --print: share one body between the entry point promise reactions #39128 had to run its test withdetect_leaks=0).Run::start(src/runtime/cli/run_command.rs) attached two native reactions to it (then2withBun__onResolveEntryPointResult/Bun__onRejectEntryPointResult,src/runtime/hw_exports.rs) and ran one more turn of the loop, whether the loop had drained or an unhandled error had stopped it. Each reactionconsole.logs the settled value and callsGlobal::exit(exit_handler.exit_code)from inside the promise job, skipping everythingRun::startdoes after printing:on_before_exit(),handle_rejected_promises(),on_exit()(theexitevent), theANY_UNHANDLED-> exit code 1 step andglobal_exit(). Attaching reactions also marks the promise handled, which is why its own late rejection was never reported. The Zig version had the same shape; not a port regression.Fix
PromiseFunctionsslots, header declarations and the mordant baseline entry that pointed at them). After the extra turn,Run::startreads the promise's status again and prints its result if it settled, or the promise itself if not, the handling the already-settled case always had; then it continues into the same exit sequence as every other--print/-erun. One print site instead of three.unhandled_error_counter == 0, the checkon_before_exituses since process: skip 'beforeExit' after a fatal uncaught exception #34639 to skipbeforeExitafter a fatal error). After an unhandled error--printnow printsPromise { <pending> }and exits 1 at once, whatever the timer, which is what-edoes with the same script (the timer does not run there either) and whatnode -pprints for a pending promise. An error that is not fatal (anunhandledRejectionlistener,--unhandled-rejections=warn) does not touch the counter, so those runs still wait for the result as before (probed). Every other run loop (Run::start's own,on_before_exit, workers, the REPL) ticks insidewhile is_event_loop_alive()and so already stops on the counter; this turn was the one that did not. event loop: report immediates' rejections before polling, and stop polling once a fatal error ended the run #38524, which makesauto_tick_active()itself return early in that state, composes with this: after an error the turn is no longer taken, and in the drained case the counter is 0.exitevent, the report for a late rejection and the teardown all come from the one existing exit path instead of a second one that had to reproduce it and did not. What the drained case prints is unchanged; a result that rejects in the extra turn is now handled like one that rejects during the loop (reported on stderr, exit 1).test/cli/run/run-eval.test.ts, new block--print with a result promise still pending when the event loop is done: unref'd timer fulfilling / rejecting the result, unhandled rejection with a resolve / reject timer that must not run, uncaught exception from a timer, and a never-settling result as a guard. Five of the six fail on the release binary and on a debug build of main (value printed, noexit listener ranline; the error cases printtimer ran/lateand exit 0); all six pass with the change. 20 runs each way, plus the whole file and the block again under the ASAN lanes' environment (BUN_DESTRUCT_VM_ON_EXIT=1,detect_leaks=1:abort_on_error=1,test/leaksan.supp, exception-check validation).Bun.sleepSync, so by the time the loop is looked at the timer is overdue. In the unref cases it therefore fires in the extra turn on every platform (the turn drains due timers whether or not they are ref'd, throughus_loop_pumpon libuv); in the rejection cases the error is counted before any timer drain, so the callback not having run holds on every platform; in the uncaught case the settling timer is armed by the throwing callback, and because libuv runs timers again after its poll that turn may still fire it on Windows, so that test only asserts the error, the listener line and the code. The rejected-unref test does not assert what the rejected result is printed as (cli: don't print a rejected promise's reason twice inbun -p#36448's subject).run-eval.test.ts,globals.test.js,no-addons.test.ts,workerd/html-rewriter.test.jsandcron/in-process-cron.test.ts(users of the otherPromiseFunctionsslots, whose indices shift), and the dead-symbol source lints.hw_exports.rshalf of bun -p/--print: report a throw raised while formatting the result #37173 (formatter throw while printing; one site left for it). cli: don't print a rejected promise's reason twice inbun -p#36448 (what a rejected result prints) applies unchanged to the late-settled case now. process: report exit code 1 to 'exit' listeners on an unhandled rejection #38116 is why the rejection tests only check that theexitlistener ran, not the code it received. Node v26 CLI compatibility: make node:cli tests pass #32622 rewrites--printto print from JS on exit, Node's model, and deletes the same reactions; this is the targeted subset of that.Background
--printresult: the C++ module loader stores the eval entry's completion value invm.entry_point_result(Bun__VM__setEntryPointEvalResult*). After the main loop,Run::startprints it, unwrapping a promise to its result.VirtualMachine::is_event_loop_alive()is false onceunhandled_error_counteris non-zero (an error nothing handled was reported; this is howbun runexits 1 after one), and once nothing ref'd is left (an unref'd timer does not count). One turn of the loop istick()(queued tasks, microtasks) plusauto_tick_active()(immediates, a poll that ends at the next timer or wakeup, then the timers that are due).Run::start:on_before_exit()emitsbeforeExit(skipped after a fatal error),handle_rejected_promises()reports rejections left by the last turn,on_exit()emitsexit,ANY_UNHANDLED(set by the run command's unhandled-error handler) becomes exit code 1,global_exit()tears the VM down when asked to and exits withexit_handler.exit_code.then2: attaches two native functions as a promise's fulfil/reject reactions.GlobalObject::thenablecaches oneJSFunctionper native function in a table indexed by thePromiseFunctionsenum (promiseFunctionsSizeis its length), which is why removing a pair of reactions touchesZigGlobalObject.h/.cpp. A promise with reactions attached counts as handled for unhandled-rejection tracking.mordant-baseline.toml: per-(lint, file) counts of pre-existing findings from the advisorymordantjob; the removed line was thereimplemented_helperfinding for the two identical reactions.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/run/run-eval.test.ts