Repository navigation
Conversation
|
Updated 1:00 PM PT - Oct 23rd, 2026
✅ @robobun, your commit c413bf1a151ce441ef709bc15a286b9bd7b89569 passed in 🧪 To try this PR locally: bunx bun-pr 39175That installs a local version of the PR into your bun-39175 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
Walkthrough
ChangesPrint entry-point promise handling
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly summarizes the main change: removing promise reactions and the extra event-loop turn used by bun --print for pending results. |
| Description check | |
The description provides detailed problem, fix, verification, tradeoffs, platform limitations, and related issues. It does not use the exact template headings, but it includes the required information… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Description check
-
Explanation
The description provides detailed problem, fix, verification, tradeoffs, platform limitations, and related issues. It does not use the exact template headings, but it includes the required information and is substantially complete.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
Status: merged main and reworked (c413bf1). Ready for review.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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
robobun
changed the title
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.
…ending-result-exit-path # Conflicts: # mordant-baseline.toml # src/jsc/bindings/ZigGlobalObject.h # src/jsc/bindings/headers.h # src/runtime/hw_exports.rs
A result promise that is still pending once the event loop is done prints
as `Promise { <pending> }`. The extra loop turn that #9407 added for
`Bun.$` results is gone: that shape settles in the main loop now, and
the turn only ran unref'd work that was already due, which node -p does
not run either.
robobun
changed the title
There was a problem hiding this comment.
Beyond the inline findings, I also checked the removal of the two PromiseFunctions slots and host functions: ThenablesArray is sized from PromiseFunctions::Count_ (src/jsc/bindings/ZigGlobalObject.h:424), and no remaining reference to Bun__onResolveEntryPointResult / Bun__onRejectEntryPointResult exists anywhere in the tree, so the deletion is clean.
Extended reasoning...
The change deletes the --print pending-promise reactions and extra loop turn in src/runtime/cli/run_command.rs and src/runtime/hw_exports.rs, with matching removals in the C++ bindings and the mordant baseline; it touches no security-sensitive surface. The inline findings (Windows stdin-derived results and beforeExit-settled results now printing as pending, plus missing ESM coverage) are behavior regressions a maintainer should weigh against the ASAN/exit-sequence motivation, so a human look is warranted.
Findings marked 🟡 are optional suggestions and need no follow-up push.
| let to_print: JSValue = match result.as_any_promise() { | ||
| Some(promise) if promise.status() != PromiseStatus::Pending => { | ||
| promise.result(vm.jsc_vm()) | ||
| } | ||
| result | ||
| _ => result, | ||
| }; |
There was a problem hiding this comment.
🔴 Windows users running bun --print on a result derived from a stdin or file read now get Promise { <pending> } instead of the value the base branch printed. On Windows the libuv read callback settles the source promise without a microtask checkpoint, so the derived .then() promise is still pending at src/runtime/cli/run_command.rs:1664 when the loop is done; the base's extra vm.tick() drained that microtask before printing. The new test certifies this with test.failingIf(isWindows) rather than guarding it. Fix: before reading the result, drain already-queued microtasks once (e.g. vm.drain_microtasks()), without a new loop turn or timer pass, so a result whose reactions are merely queued prints its value on every platform; then drop the failingIf marker. [also at: test/cli/run/run-eval.test.ts:241 - Windows users running bun --print on a promise derived from a stdin or Bun.file read now get Promise { <pending> } where the base printed the value.]
Why this was flagged
Trigger: bun --print 'Bun.stdin.text().then(t => t.trim())' with piped stdin on Windows (the PR's own stdin test, test/cli/run/run-eval.test.ts:241, is marked failingIf(isWindows) for this and cites issue #41850; the same applies to a rejecting Bun.file() read per #39787). The source promise is resolved from a libuv callback with no microtask checkpoint, so the .then() continuation has not run when while vm.is_event_loop_alive() at run_command.rs:1652 exits. The new match at run_command.rs:1663-1668 sees PromiseStatus::Pending and prints the raw promise, so stdout is Promise { <pending> }. On the base branch the deleted result.then2(...) plus vm.tick() ran a microtask checkpoint that settled the derived promise and printed the value. The PR text acknowledges the hazard and the test encodes the regression as expected on Windows, so CI will not catch it.
Verification: test/cli/run/run-eval.test.ts:239-241 marks the stdin test test.failingIf(isWindows) with the comment "On Windows the .then() of a stdin read has not run when the loop ends". New code at run_command.rs:1663-1668: Some(promise) if promise.status() != PromiseStatus::Pending => promise.result(...), _ => result — the derived promise is Pending, so Promise { <pending> } is printed.
| if ctx.runtime_options.eval.eval_and_print { | ||
| let to_print: JSValue = 'brk: { | ||
| let result = vm | ||
| .entry_point_result | ||
| .value | ||
| .get() | ||
| .unwrap_or(JSValue::UNDEFINED); | ||
| if let Some(promise) = result.as_any_promise() { | ||
| match promise.status() { | ||
| PromiseStatus::Pending => { | ||
| // C-ABI shims are emitted by | ||
| // `generate-host-exports.ts` into | ||
| // `crate::generated_host_exports` under their | ||
| // link name (`Bun__on…EntryPointResult`). | ||
| result.then2( | ||
| vm.global(), | ||
| JSValue::UNDEFINED, | ||
| crate::generated_host_exports::Bun__onResolveEntryPointResult, | ||
| crate::generated_host_exports::Bun__onRejectEntryPointResult, | ||
| ); | ||
| vm.tick(); | ||
| vm.auto_tick_active(); | ||
| while vm.is_event_loop_alive() { | ||
| vm.tick(); | ||
| vm.auto_tick_active(); | ||
| } | ||
| break 'brk result; | ||
| } | ||
| _ => break 'brk promise.result(vm.jsc_vm()), | ||
| } | ||
| let result = vm | ||
| .entry_point_result | ||
| .value | ||
| .get() | ||
| .unwrap_or(JSValue::UNDEFINED); | ||
| let to_print: JSValue = match result.as_any_promise() { | ||
| Some(promise) if promise.status() != PromiseStatus::Pending => { | ||
| promise.result(vm.jsc_vm()) | ||
| } | ||
| result | ||
| _ => result, | ||
| }; | ||
| // SAFETY: `vals[..1]` is the single stack `to_print`; null | ||
| // `ctype` routes to the VM's stdout/stderr default. |
There was a problem hiding this comment.
🟡 (optional) Users whose --print result is settled by work a 'beforeExit' listener starts now never see the value; the base printed it. The print at src/runtime/cli/run_command.rs:1671 runs before vm.on_before_exit() at :1683, so the promise is still pending and prints as Promise { <pending> }. On the base the reaction attached by then2 still fired during on_before_exit's drain (src/jsc/VirtualMachine.rs:2257) and printed the settled value before exiting. Fix: read the result again after on_before_exit returns (print at exit time, or print once more when a result that was pending has since settled) so a late settlement is never dropped.
Why this was flagged
Trigger: bun -p 'new Promise(r => process.on("beforeExit", () => setTimeout(() => r(5), 1)))' or any script whose beforeExit listener starts the work that settles the printed promise. Entry: Run::start non-watcher arm, src/runtime/cli/run_command.rs:1652-1683. The new code reads the promise once at :1658-1668 while the loop is dead and prints Promise { <pending> } at :1671, then calls vm.on_before_exit() at :1683, which dispatches beforeExit (src/jsc/VirtualMachine.rs:2246) and drains the listener's work at :2249-2263; the settlement happens there, after the print. On the base branch the pending arm attached Bun__onResolveEntryPointResult via result.then2 and that reaction (base src/runtime/hw_exports.rs:293-314) console.log'd the value when the drain inside on_before_exit settled it, so stdout contained the value. Remedy: re-check the promise after on_before_exit rather than reading it once before listeners run.
Verification: New code, src/runtime/cli/run_command.rs:1658-1668, reads entry_point_result once; a pending promise is printed at :1671-1680 as Promise { <pending> }. Only afterwards does :1683 call vm.on_before_exit(), which drains the loop at src/jsc/VirtualMachine.rs:2249-2263. The promise settles during that drain, but nothing re-reads or re-prints it, so the value is never shown.
| // `--print` reads the result once the event loop is done. A promise that is | ||
| // settled by then prints its value. One that is still pending prints as | ||
| // `Promise { <pending> }`: nothing more runs for it, neither unref'd work nor | ||
| // anything after an unhandled error. Either way the process leaves through the | ||
| // regular exit path ('exit' listeners, exit code). | ||
| // | ||
| // A script whose timer must not run blocks in Bun.sleepSync after it arms the | ||
| // timer, so the timer is overdue by the time the loop is done. |
There was a problem hiding this comment.
🟡 nit (optional): maintainers get no coverage of the ESM producer of the printed result, and the block's stated contract does not hold there. Every script in the new block is evaluated as CJS; an entry with import or top-level await stores its value through EvalGlobalObject::moduleLoaderEvaluate (src/jsc/bindings/ZigGlobalObject.cpp:4428) and, with TLA, is driven by wait_for_promise (src/jsc/event_loop.rs:1141), which keeps ticking after unhandled_error_counter is non-zero. Fix: add ESM variants (an import and a TLA form) of the pending, unhandled-rejection and unref'd-timer cases alongside the CJS ones, and pin or document that a TLA entry still runs its timer and prints the settled value after an unhandled error (pre-existing).
Why this was flagged
The new describe block at test/cli/run/run-eval.test.ts:214 only passes scripts with no import/export and no top-level await, so they are evaluated as CJS and the result reaches vm.entry_point_result through Bun__VM__setEntryPointEvalResultCJS (src/jsc/bindings/JSCommonJSModule.cpp:247). The ESM…
Verification: nit. Triggering condition: a --print entry that parses as ESM, which the new block never spawns. Every script in the new block at test/cli/run/run-eval.test.ts:214-321 is evaluated as CJS. For TLA entries wait_for_promise (src/jsc/event_loop.rs:1141-1152) never consults unhandled_error_counter, so the block's comment does not hold. That is pre-existing behavior unchanged by this PR.
|
Answers to the three review findings. None of them changes the code in this PR. Windows stdin result (run_command.rs:1668). The missing microtask checkpoint is in the Windows read completion. Result settled from a 'beforeExit' listener (run_command.rs:1670). Listed under "Not changed here" in Notes. On main this case prints
ESM coverage (run-eval.test.ts:213). The new block pins the CJS arm that the deleted reactions served. A settled ESM result is covered by the |
Problem
--printresult is a promise still pending after the event loop,Run::startattaches two reactions and runs one more loop turn. A reaction that fires prints the value and callsGlobal::exit.exitlisteners, exit code 1 after an unhandled rejection,--cpu-profoutput and VM teardown. The ASAN lane reportsERROR: LeakSanitizer: detected memory leaks, exit 134.Fix
src/runtime/hw_exports.rs:295) and the turn (src/runtime/cli/run_command.rs:1666).Run::startreads the promise once: settled prints its result, pending printsPromise { <pending> }. Its normal exit sequence follows.test/cli/run/run-eval.test.ts, 11 tests. A debug build of main fails 7, the 4 controls pass.Background
Run::startloads the script, runs the event loop until no ref'd work is left, then runs the exit sequence.--printwith promises #9407 added both so thatBun.$`cmd`.text()printed. That result settles in the main loop today.unhandled_error_counter == 0check (f319da0). It only runs due unref'd work, a race thatnode -pdoes not have.Downsides
AbortSignal.timeout()) now printsPromise { <pending> }.-presult derived from a stdin read should now print as pending until Drain microtasks after a Windows Bun.file() read completes #39793 lands. The stdin test isfailingIf(isWindows), so CI checks this.Notes
Question for a maintainer. #9407 added the extra turn. This PR removes it. If the turn is a wanted default, commit f319da0 on this branch keeps it behind an
unhandled_error_counter == 0check and passes its own tests. Say so and I will restore that shape.Before and after (Linux x64. main = release 1.4.3-canary.1, same
--printarm as main 5c6d4e2. Each script registers anexitlistener.)-presultPromise { <pending> }, listener, exit 0timer ran,late, no listener, exit 0Promise { <pending> }, listener, exit 1settled late, no listener, exit 1Promise { <pending> }, listener, exit 1--preloadwithPromise.reject(new Error("boom")), resultBun.sleep(10).then(() => 5)5, no listener, exit 0Promise { <pending> }, listener, exit 1--import: pending, listener, exit 1queueMicrotask(() => { throw new Error("boom") })5, no listener, exit 1Promise { <pending> }, listener, exit 1Bun.$, a timer, a stdin read,Bun.file,fetch,Bun.spawn().exited, top-level awaitbun --cpu-prof -p 'new Promise(r => { setTimeout(() => r(1), 1).unref(); Bun.sleepSync(10) })'writes 0 profiles on main and 1 with this PR.-pshapes were compared on both builds. 17 print the same: results that ref'd work settles, results settled before the loop, and two that never settle. The other 11 are the paths in the table.ASAN lane. Debug build, with
BUN_DESTRUCT_VM_ON_EXIT=1,ASAN_OPTIONS=...detect_leaks=1:abort_on_error=1andLSAN_OPTIONS=...suppressions=test/leaksan.supp(whatscripts/runner.node.tssets):bun-debug --preload ./p.mjs --print 'Bun.sleep(10).then(() => 5)', withqueueMicrotask(() => { throw new Error("boom"); })inp.mjs, prints5, thenERROR: LeakSanitizer: detected memory leaks,SUMMARY: AddressSanitizer: 262 byte(s) leaked in 2 allocation(s), exit 134. The leaked objects are theArc<ParsedSourceMap>fromSavedSourceMap::get_with_contentand itsInternalSourceMapbuffer. The reaction exits beforeglobal_exit()runs the teardown that frees them.Promise { <pending> }, exit 1, no leak report. 10 of 10 runs for each preload kind.debian 13 x64-asan). That PR dropped the test and left this path to this PR.Tests. Block
--print of a promise once the event loop is done, 11 tests:src/change: a debug build of main 5c6d4e2 (plain environment and ASAN lane environment) and the release build.test.failingIf(isWindows): see Windows below.Bun.sleepSync(10). The timer is overdue when the loop is done, so any extra turn fires it. Three of the scripts (unref'd timer, early rejection, timer throw) were also run directly 120 times each under load: all print the pending promise.drain_timers(src/runtime/timer/mod.rs) reads the clock once per pass, so the timer that the throwing callback arms is not due in that pass. On Windows the pass runs fromon_uv_timer, and libuv calls a restarted timer handle in the next pass only. This is read from the source. It was not run on Windows.Other suites on the debug build.
exit-code.test.ts,preload-test.test.js,globals.test.js,no-addons.test.ts,process-args.test.jsanddisallow-code-generation-from-strings.test.ts(these two with--timeout 120000: at the 5 s default this debug build is too slow for tests that spawn many processes),html-rewriter.test.jsandin-process-cron.test.ts(users ofPromiseFunctionsslots behind the two removed ones, run before the turn was removed), the vendoredtest-timers-{timeout,interval,immediate}-promisified.js,test-child-process-promisified.js,test-cli-options-precedence.js, andtest/internal/source-lints/(3 of its tests time out at 5 s on the debug build and pass on the release build).Not changed here, with the owner.
.then()-derived result that rejects still prints its reason on stdout and exits 0:bun -p 'Bun.sleep(1).then(() => { throw new Error("boom") })'.promise.result()marks the promise handled before the exit sequence reports rejections. That arm is main's. Report unhandled rejections made in the event loop's last turn #45582 reports the rejection at the end of the loop turn.bun -p 'setTimeout(() => process.exit(0), 5); 5'prints nothing (node prints5). A result that abeforeExitlistener settles prints as pending. After a fatal error the result prints beforeexitlisteners (node prints it after). Node v26 CLI compatibility: make node:cli tests pass (+9 upstream tests) #32622 moves the print to exit time (src/js/internal/eval_print.ts).exitlisteners get code 0 after an unhandled rejection: process: 'exit' listeners see exit code 1 after an unhandled rejection #45575.Windows.
Bun.stdin.text()and a rejectingBun.file()read settle their promise from a libuv callback with no microtask checkpoint (#41850, #39787, fix in #39793). A.then()on such a promise has not run when the loop ends. For a-presult derived that way, the deleted turn'stick()ran the checkpoint. So main prints the value, and this PR should printPromise { <pending> }.bun -ealready loses that continuation on Windows. This is inferred from #39793. It was not run. The stdin control is thereforetest.failingIf(isWindows). If the Windows lanes report that it passed, the inference is wrong: remove the marker and this paragraph. Whichever of this PR and #39793 lands second must remove the marker.Overlap with open PRs.
entry_cut_shortarm to this block. It is not needed once this lands: a pending result never gets a turn.Earlier revisions. 82b9967 and f319da0 kept the extra turn when the loop drained on its own and printed the result after it. The self-review found that the turn only serves unref'd work that is already due when the loop ends, so its outcome depends on timing. This revision removes it.
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