event loop: run completions left on the macro loop after the macro returned - #38304
event loop: run completions left on the macro loop after the macro returned#38304robobun wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 25 minutes 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)
Comment |
|
Updated 8:49 AM PT - Aug 14th, 2026
✅ @robobun, your commit 73eb0fcde6a901038401b1cd78a030a91ba3ea13 passed in 🧪 To try this PR locally: bunx bun-pr 38304That installs a local version of the PR into your bun-38304 --bun |
|
Status: reproduced and fixed, waiting on CI. Reproduced on canary The new cases in Rebased onto main at 60f6e18: the darwin Rebased again onto main at 7ba276f (after #38299, the ticket model, and #38436) on request; the two commits apply unchanged. Re-verified there: the three in-macro cases hang with main's |
![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 the per-tick concurrent drain in EventLoop::tick_concurrent_with_count and the exit-decision predicate (is_event_loop_alive_excluding_immediates), and introduces cross-loop draining, a human look would still be worthwhile.
What was reviewed:
take_concurrent_batchdrops the deferred-free pattern from the oldtick_concurrent_with_countin favor of the read-then-free pattern thattake_concurrent_tasksalready used; verifiedBatchIterator::nextadvancesfrontpast the node before returning it, so freeing immediately after the raw-pointer reads is sound.finished_macro_loopgate:has_enabled_macro_mode && !macro_mode && ptr::eq(self, ®ular_event_loop)— confirmed the spawnSync isolated loop and the macro loop itself returnNone, andpop_batch/is_emptytake&selfso the&EventLoopreturn is enough.has_concurrent_tasksis wired into bothhas_pending_tasksand theis_event_loop_alivesum, so a stranded macro-loop completion both prevents a blocking poll and keeps the loop alive until it runs.- Tests: three cases (fs.promises in-macro, at module top level, fetch) spawn a child, use a local
Bun.serve({port:0}), drain both pipes concurrently, filter debug[macro]lines, and sort to tolerate ordering — no timing sleeps.
Extended reasoning...
Overview
The PR fixes a hang regression from #37075: async work started inside a macro (un-awaited fs.promises, fetch()) captures LoopKind::Macro at creation and posts its completion to macro_event_loop.concurrent_tasks, but that queue is only drained while wait_for_promise is ticking the macro loop. Once the macro returns, the completion is stranded there while its keep-alive on the shared platform loop keeps is_event_loop_alive() true forever.
The fix has the regular loop's tick_concurrent_with_count also pop the macro loop's concurrent queue, gated by finished_macro_loop() (has_enabled_macro_mode && !macro_mode && self == ®ular_event_loop). has_pending_tasks and is_event_loop_alive_excluding_immediates route through the same gate via a new has_concurrent_tasks(). The batch-transfer body is factored into take_concurrent_batch, which release_queued_tasks now shares (deleting the near-identical take_concurrent_tasks). VmHandle.rs gets only a doc-comment update. Three subprocess tests are added to macro-test.test.ts.
Security risks
None identified. No user-controlled input reaches new parsing/allocation; no auth/crypto/permissions surface. The change is internal event-loop scheduling.
Level of scrutiny
High. tick_concurrent_with_count runs on every tick of every JS VM, and is_event_loop_alive_excluding_immediates decides process exit. The refactoring changes the iteration/free ordering in the hot drain (dropping the one-iteration-deferred free in favor of immediate free after raw-pointer reads — the pattern the deleted take_concurrent_tasks already used, and BatchIterator::next does advance past the node before returning it, so the SAFETY comment is accurate). Cross-loop draining is a new behavior whose correctness depends on the invariant that only one of the two loops ticks at any moment (MacroModeGuard swaps vm.event_loop). The gate correctly excludes the macro loop itself and spawnSync's heap-allocated isolated loop (neither is &vm.regular_event_loop).
Other factors
- Programs that never run a macro pay only one extra bool read (
has_enabled_macro_modeis false), so the hot path is unchanged. - The
&EventLoopreturned byfinished_macro_loopis derived throughvm_ref() -> &'static VirtualMachinewhile&mut self(the regular loop, a sibling field) is held — this follows the existingvm_ref()pattern used throughout the file, andpop_batch/is_emptytake&self. - The tests follow harness conventions (
tempDir,bunEnv, concurrent pipe drain, local server,test.concurrent.each), assert both output lines andexitCode, and handle debug-build[macro]output and non-deterministic line order. - The PR description states the tests time out on an unfixed build and pass with the fix, and that the surrounding macro/spawn/worker suites still pass.
Given the change sits in core event-loop tick/exit machinery with unsafe iteration and cross-loop state, deferring to a human reviewer rather than auto-approving.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?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 how the core event loop drains cross-loop concurrent tasks (unsafe batch iteration, the exit-decision predicate, and a refactor that unifies two previously divergent drain implementations), a human look at the design would still be worthwhile.
What was reviewed:
finished_macro_loop()gating:has_enabled_macro_mode && !macro_modeplus the pointer-equality check correctly excludes the macro loop itself and spawnSync's isolated heap loop.- The deferred-free
to_destroypattern was dropped in favour of the immediate-free pattern the deletedtake_concurrent_tasksalready used;Batch::iterator().next()advances past the node before returning it, so freeing before the nextnext()is sound as the SAFETY comment states. pop_batch(&self)on the macro loop's queue via a shared borrow while holding&mut selfon the regular loop — disjoint VM fields, samevm_ref()pattern used throughout the file.- Tests follow harness conventions (
port: 0, concurrent pipe drain, local server,test.concurrent.each, exit code asserted last).
Extended reasoning...
Overview
Fixes a hang regression from #37075: a macro that starts async work without awaiting it (un-awaited fs.promises.stat(), fetch()) now leaves its completion stranded on macro_event_loop.concurrent_tasks after the macro returns, holding a keep-alive on the shared platform loop so the process never exits. Touches src/jsc/event_loop.rs (adds finished_macro_loop/has_concurrent_tasks/take_concurrent_batch, rewires tick_concurrent_with_count/has_pending_tasks/release_queued_tasks), a one-line change in VirtualMachine.rs (is_event_loop_alive_excluding_immediates), a doc-only change in VmHandle.rs, and three new subprocess tests in macro-test.test.ts.
Security risks
None identified. No untrusted input parsing, no auth/crypto, no new FFI surface. The unsafe blocks are the same batch-node dereference/free the file already had, now in one place instead of two.
Level of scrutiny
High. This is the JS-thread event loop's per-tick drain and the process exit predicate — every bun run goes through it. The refactor unifies two implementations of the concurrent-batch transfer that previously disagreed on whether the ConcurrentTask carrier free needed to be deferred one iteration (tick_concurrent_with_count deferred via to_destroy; take_concurrent_tasks freed immediately). The PR picks the immediate-free variant, which the deleted function's SAFETY comment already justified ("the iterator advanced past it before returning"), and pop_batch takes &self so the cross-loop read is a shared borrow of a disjoint VM field. I did not find a problem with any of this, but it is exactly the class of change where a maintainer should confirm the design choice (regular loop adopting the macro loop's queue vs. re-binding the LoopKind at post time or transferring at disable_macro_mode).
Other factors
- The
core::ptr::eq(self, &raw const vm.regular_event_loop)guard means the spawnSync isolated loop (heap-allocated) and the macro loop itself never adopt the macro queue;has_enabled_macro_modeshort-circuits the whole check to one bool for programs that never use macros. - Teardown already calls
release_queued_taskson both embedded loops (VirtualMachine.rs:1931-1932), so the newrelease_queued_taskscorrectly draining only its own queue (not also the macro loop's) is fine. - All comment-cop bot flags were addressed in 09d148e and the threads are resolved.
- Tests were verified by the author to hang under
USE_SYSTEM_BUN=1and pass with the debug build; they use a localBun.serve({port:0})for the fetch case and follow the harness conventions.
09d148e to
129de6f
Compare
There was a problem hiding this comment.
The concurrent_ref adoption gap I flagged earlier is now covered — tick_concurrent_with_count folds macro_loop.concurrent_ref via apply_concurrent_ref_delta(), has_pending_refs() checks it, and the new crypto.subtle.digest() test case exercises that path. I re-reviewed and found nothing further; given this reworks how the regular loop reaches into the macro loop's concurrent state, a human look at the cross-loop borrow (finished_macro_loop handing out &vm.macro_event_loop while &mut self is the regular loop) and the > 0 check semantics for a pending -1 delta would still be worthwhile.
What was reviewed:
finished_macro_loopgating: only the regular loop adopts, only afterhas_enabled_macro_mode && !macro_mode, so the macro loop and spawnSync's isolated loop never self-adopt.take_concurrent_batchrefactor preserves the old drain semantics (into_taskfrees auto-delete carriers;ensure_unused_capacitybefore the loop);release_queued_tasksnow shares it.has_pending_refschecking> 0on the macro loop'sconcurrent_ref: the off-thread write is-1(unref), which correctly does not keep the loop alive; the stuck+1onplatform_loop.activeis what keeps it alive until the next tick folds the-1.
Extended reasoning...
Overview
This PR fixes a hang regression from #37075: async work started inside a macro without being awaited posts its completion to macro_event_loop.concurrent_tasks (because the ticket captured LoopKind::Macro at creation), but that queue is only drained while a macro is being waited on. After the macro returns nothing pops it, and the work's keep-alive on the shared platform loop keeps the process alive forever. The fix has the regular loop's tick_concurrent_with_count also drain the macro loop's queue and fold its concurrent_ref deltas, gated by finished_macro_loop() (macro mode was ever entered, is not currently active, and self is the regular loop). has_concurrent_tasks() and has_pending_refs() extend the same check so the poll-must-not-block and exit-decision predicates see the adopted work. The batch-transfer loop is factored into take_concurrent_batch, which release_queued_tasks now reuses.
Follow-up on prior finding
My earlier inline comment noted that only concurrent_tasks was adopted, not concurrent_ref, so crypto.subtle.digest() (whose ConcurrentCppTask calls Ticket::unref_keep_alive off-thread → macro_event_loop.concurrent_ref -= 1) would still hang. Commit af84237 addresses this: tick_concurrent_with_count now calls macro_loop.apply_concurrent_ref_delta() before draining, has_pending_refs() also checks the macro loop, and a fourth test case (crypto.subtle.digest on a ≥64-byte input, forcing the WebCrypto work queue) was added. I traced the flow again: the JS-thread ref_keep_alive() folds its +1 into platform_loop.active immediately; the off-thread -1 sits in macro_event_loop.concurrent_ref until the regular loop's next tick folds it. has_pending_refs() checking > 0 is correct — a pending -1 should not itself keep the loop alive; the unfolded +1 in platform_loop.active does that until the fold happens on the next tick.
Security risks
None. No user-controlled input reaches new parsing/allocation; the change is which per-VM queue gets drained on the JS thread.
Level of scrutiny
High. event_loop.rs is the JS-thread run loop; tick_concurrent_with_count runs on every tick, and has_pending_tasks/has_pending_refs gate whether the process blocks or exits. The new finished_macro_loop() forms a shared borrow of vm.macro_event_loop (via vm_ref() -> &'static VirtualMachine) while &mut self aliases vm.regular_event_loop — disjoint fields, and apply_concurrent_ref_delta/pop_batch on the macro loop take &self, so no stacked-borrow overlap, but this is exactly the kind of cross-field access the file's other R-2 mitigations are careful about. The take_concurrent_batch refactor drops the deferred to_destroy pattern in favor of ConcurrentTask::into_task, which per the doc frees auto-delete carriers inline; that behavior change is worth a maintainer's confirmation.
Other factors
Four new test cases in macro-test.test.ts cover fs.promises (in-macro and top-level), fetch, and crypto.subtle.digest; the PR body confirms the in-macro cases hang without the fix on both debug-ASAN and are exercised on release. The two open comment-cop bot notes (VmHandle.rs:78, event_loop.rs:550) are about doc-comment length; the author already argued for keeping them on earlier iterations and the current comments are trimmed to the load-bearing invariant. Not approving because this is a substantive change to event-loop task adoption across two embedded loops — not mechanical.
…turned Off-thread work captures the loop that is current when it is created, so work a macro starts posts its completion to the macro loop. That loop only ticks while the macro is being waited on. A completion arriving after the macro returned (an un-awaited fs.promises call, a fetch()) stayed queued there while still holding its keep-alive on the shared platform loop, so `bun run` of a file using such a macro never exited. The regular loop now also drains the macro loop's concurrent queue whenever no macro is running, and its pending-work / liveness checks look at that queue too. The batch transfer is shared with the teardown release path.
A Macro-kind Ticket releases its keep-alive into macro_event_loop's concurrent_ref (WebCrypto's work queue does this for inputs hashed off thread), and only a tick of that loop folded the delta into the platform loop. So a crypto.subtle call started inside a macro still kept the process alive after its completion had run. The regular loop now applies the macro loop's delta when it services that loop, and the liveness check sees a pending delta there as well.
af84237 to
73eb0fc
Compare
There was a problem hiding this comment.
I reviewed this PR and found no remaining bugs — the concurrent_ref sibling I flagged earlier is fixed in af84237 and covered by the new 4 KiB crypto.subtle.digest case. Because it changes the core tick/drain path and the process-exit liveness checks in event_loop.rs, a human look is still worthwhile.
What was reviewed:
finished_macro_loop()gating: confirmed only the regular loop adopts (ptr-eq againstregular_event_loop), so the macro loop and spawnSync's isolated loop never recurse into it.take_concurrent_batchrefactor vs the old deferred-to_destroyloop:ConcurrentTask::into_taskreadstask/auto_deletebefore freeing the carrier, and the batch iterator has advanced past the node, so the drop-then-next ordering is sound.apply_concurrent_ref_deltaon the macro loop folds into the sharedplatform_loop_opt(), so the delta lands in the same active count either loop uses.- Tests: hermetic (local
Bun.serve({port:0}), tempDir,test.concurrent.each), drain all three pipes, and the sort handles the non-deterministic continuation ordering.
Extended reasoning...
Overview
The PR fixes a hang regression from #37075: async work started inside a macro captures LoopKind::Macro on its ticket, so its completion (and, since #38299, its keep-alive release) lands on macro_event_loop's concurrent channels. That loop only ticks inside wait_for_promise while a macro is pending, so once the macro returns nothing drains it and the shared platform loop's active count never reaches zero. The fix has the regular loop's tick_concurrent_with_count also service the macro loop's concurrent_tasks and fold its concurrent_ref delta, gated by finished_macro_loop() (has_enabled_macro_mode && !macro_mode && self is regular_event_loop). has_pending_tasks, has_concurrent_tasks, has_pending_refs and is_event_loop_alive_excluding_immediates are updated to see the same state so the poll/exit decisions match what the drain will pick up. The two batch-transfer loops (tick vs teardown's release_queued_tasks) are consolidated into take_concurrent_batch, built on the existing ConcurrentTask::into_task.
Security risks
None. This is internal event-loop scheduling; no user-controlled input reaches new code paths, no auth/crypto/permissions surface.
Level of scrutiny
High. tick_concurrent_with_count, has_pending_tasks, has_pending_refs and release_queued_tasks sit on every VM's tick and teardown path — a mistake here hangs or prematurely exits every Bun process, not just macro users. The finished_macro_loop() gate keeps the added work to one bool check when no macro ever ran, which bounds the blast radius, but the refactor of the concurrent-batch drain replaces a deliberate deferred-free pattern with into_task and runs on every tick regardless.
Other factors
- My earlier inline finding (macro loop's
concurrent_refnot folded) was confirmed by the author, fixed in af84237, and a test case (4 KiBcrypto.subtle.digest, which routes through the WebCrypto work queue'sTicket::unref_keep_alive) was added that fails on the first commit and passes on the second. - The gate evidence in the PR description shows the new tests fail (timeout) on both debug-ASAN and release builds without the fix and pass with it; the rest of the macro/transpiler/spawn/worker suites pass.
- The batch-drain refactor is behaviour-preserving:
into_taskreads the payload andauto_deleteflag before freeing the heap carrier (ConcurrentTask.rs:320-328), and the iterator advances past a node before returning it, so freeing node N before fetching N+1 is safe — equivalent to the oldto_destroydeferral. - The comment-cop bot flagged doc-comment length repeatedly; the author trimmed and then justified keeping the remaining two-line docs on
LoopKind::Macroandfinished_macro_loop. All those threads are resolved.
Given the surface area (core tick + exit-liveness), I'm not shadow-approving; a maintainer familiar with the macro-loop / MacroModeGuard interaction should sign off.
2 participants
Problem
bun index.tswhereindex.tscalls a macro that starts async work without awaiting it (an un-awaitedfs.promises.stat(),fetch()orcrypto.subtle.digest()inside the macro; the same at the macro module's top level when the work outlasts loading that module) prints its output and then never exits. 1.3.14 exits 0.Job::scheduletakesvm.ticket()(src/jsc/job.rs:331), which recordscurrent_loop_kind()(src/jsc/VmHandle.rs:519,src/jsc/VirtualMachine.rs:903). While a macro runs that isLoopKind::Macro, so the completion is posted tomacro_event_loop.concurrent_tasks(loop_of+ push,src/jsc/VmHandle.rs:159-179).wait_for_promisewhile a macro is being waited on. Once the macro has returned nothing pops that queue, and the job's keep-alive (taken on the uSockets loop both embedded loops share,job.rs:316, released only when the completion runs) keepsis_event_loop_alive()true forever.Ticket::unref_keep_alive()(VmHandle.rs:245, used by WebCrypto's work queue,src/jsc/CppTask.rs:75, for inputs hashed off thread) adds its-1tomacro_event_loop.concurrent_ref, which only a tick of that loop folds into the platform loop. The WebCrypto result itself is posted to the regular loop and runs; the keep-alive is what stays stuck. This channel exists since One door out of a VM's thread: tickets + a teardown that waits #38299 madeConcurrentCppTaskcarry a ticket instead of always releasing on the regular loop.Fix
EventLoop::tick_concurrent_with_counton the regular loop also services the macro loop: it folds that loop'sconcurrent_refdelta (apply_concurrent_ref_delta, the same function each loop runs on itself) and pops its concurrent queue. Gated byfinished_macro_loop()(src/jsc/event_loop.rs:551): the VM has run a macro (has_enabled_macro_mode), no macro is running right now (!macro_mode), and the loop doing the drain is the regular one (so the macro loop itself and spawnSync's isolated loop never adopt it).has_pending_tasks()(poll must not block) andis_event_loop_alive_excluding_immediates()go throughhas_concurrent_tasks(), andhas_pending_refs()also looks at the macro loop, so work sitting on the macro loop counts the same as work on the regular loop.vm.event_looppoints at the macro loop and it services itself as before (awaited macros are unchanged, and the regular loop's queue still waits until the macro returns); once it returns, the regular loop owns whatever is left or arrives later. A post or unref already wakes the shared platform loop, so a late one is picked up on the next tick. Draining only indisable_macro_mode()would not be enough: in the repro the completion is posted after that runs. Picking the loop at post time instead would put a read of the JS thread's macro state back on the posting thread, which is the race Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown #37075 removed.boolcheck (has_enabled_macro_modeis false). The batch transfer intotasksis factored intotake_concurrent_batch(built onConcurrentTask::into_task), which the teardown release path (release_queued_tasks) now shares instead of carrying its own copy.test/bundler/transpiler/macro-test.test.ts, four new cases that assert the child prints both the macro's value and the un-awaited continuation's output and exits 0. The three cases that start the work inside the macro call (fs.promises,fetch(), a 4 KiBcrypto.subtle.digest()) time out without the fix (the child never exits) and pass with it; the digest case also times out with only the first commit of this PR. The top-levelfs.statcase passes either way on current main, because the stat usually completes while the macro loop is still being ticked to load the macro module; it covers the continuation running from the macro loop. Top-level work that outlasts the module load (a fetch against a server that answers after 300 ms, checked by hand) hangs without the fix and exits with it. The rest of that file,transpiler.test.js, the macro cases inbundler_edgecase/bun-build-api,web-crypto.test.ts,spawn.test.tsand the worker teardown tests pass with the debug build; the file also passes with a release build.Background
EventLoops,regular_event_loopandmacro_event_loop, plusvm.event_loop, a pointer to whichever is current.MacroModeGuardpoints it at the macro loop for the duration of a macro call (and of loading the macro module) so that the user program's queued work does not run in the middle of transpiling; a macro that returns a promise is settled by ticking that loop. Both loops sit on the same uSockets/libuv platform loop, whose active count is what keep-alives adjust and whatis_event_loop_alive()reads.EventLoophas two things other threads write to:concurrent_tasks, the MPSC queue of completions (each tick moves a batch of it into the loop's owntasksFIFO and runs them), andconcurrent_ref, a pending keep-alive delta thatapply_concurrent_ref_deltafolds into the platform loop's active count at the top of each tick. ATicket(VmHandle.rs) is what off-thread work holds to reach either; itsLoopKind, fixed when the work is created, says which loop's pair it writes to.bun run, macros in the entry file (and in every file when transpiling happens on the main thread, e.g. with plugins) execute in the main VM, which is why this hangs the process. Macros in files transpiled on the thread pool and inbun buildrun in per-thread VMs whose regular loop never ticks, so there the same stranded work is only a leak, which is unchanged here.setImmediate()also hangs, on 1.3.14 as well (immediates are per-loop too); that is pre-existing and not touched by this change.[review] gate passed · iteration 2 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 2
evidence per changed file