bundler: keep the scan counter balanced when an onLoad plugin does not await defer() - #37731
bundler: keep the scan counter balanced when an onLoad plugin does not await defer()#37731robobun wants to merge 6 commits into
Conversation
|
Warning Review limit reached
Next review available in: 7 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 (10)
Comment |
|
Status
|
…e build settles early
|
Updated 3:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit 365aff8 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 37731That installs a local version of the PR into your bun-37731 --bun |
|
Pushed 7b9bd4e (the cancellation fixture rejects on the worker's |
There was a problem hiding this comment.
Thanks for wiring the worker "error" handler in the cancellation fixture — that addresses my earlier note. I re-reviewed after 8ac83ee and didn't find any bugs; because this reworks cross-thread scan-counter bookkeeping and intrusive queue-node ownership in bundle_v2.rs (new unsafe fn post_to_own_loop, second defer_task node, the FIFO-ordering assumption behind the is_linked() check), a maintainer look is still worthwhile.
What was reviewed:
on_load's newdeferredbranch balances againston_notify_deferon both the Mini and JS-loop arms, andfail_outstanding_plugin_requestsnow routing deferred loads throughon_loadreaches the same net counter state as the removed special case.post_to_own_loopis a mechanical dedup of the previouson_load_async/on_resolve_asyncbodies;on_defer_asyncuses the separatedefer_taskoffset so it cannot re-enqueue the answer's node.on_deferreadsself.bv2into a local before posting, soappend_defer_promise(Plugin-only) does not touch theLoadafter the owning loop may have it.- The three new subprocess tests assert the strongest observable (bundle output / event ordering / clean terminate), fail-fast on unexpected build settlement, and cover both loop arms plus cancellation.
Extended reasoning...
Overview
Fixes a scan-counter imbalance in BundleV2 when an onLoad plugin calls args.defer() without awaiting it, which panicked (NegOverflow / unreachable) or hung Bun.build. Touches src/bundler/Graph.rs (adds OutstandingLink::is_linked), src/bundler/bundle_v2.rs (adds Load::defer_task, reworks on_notify_defer to take the Load and ignore late notifications, has on_load reclaim a still-parked unit, extracts post_to_own_loop, simplifies fail_outstanding_plugin_requests), and src/runtime/api/JSBundler.rs (on_defer now delegates to on_defer_async; ~45 lines of hand-rolled loop dispatch removed). Tests added to bundler_defer.test.ts (three subprocess cases) and bake/dev/plugins.test.ts (JS-loop arm).
Security risks
None. No untrusted-input parsing, no auth/crypto/permissions surface. The change is internal bundler bookkeeping.
Level of scrutiny
High. This is concurrency-sensitive native code in the bundler core: pending_items == 0 is what lets wait_for_parse return and linking begin, so a mis-counted unit can start linking while a parse worker is still writing into the graph (a data race, not just a hang). The fix also introduces a second intrusive MPSC queue node on Load and a new unsafe fn post_to_own_loop whose safety contract ("not queued anywhere else, and *request outlives the hop") is discharged by callers via called_defer / arena ownership. The correctness of the is_linked() late-defer check relies on FIFO delivery of the shared queue between the defer notification and the answer. These are exactly the invariants REVIEW.md flags under "Know the thread affinity of every line you touch" and "Never let a pointer or slice outlive the memory it points into".
Other factors
The PR is well-tested: three new subprocess-spawned cases (before-answer, after-answer, cancelled-while-parked) plus a dev-server case for the JS-loop arm, each verified by the author to fail on the unfixed build. My prior feedback (wiring the worker error event so a pre-arm failure surfaces instead of timing out) was addressed in 7b9bd4e, and the comment-cop verbosity flags were trimmed in 8ac83ee. The post_to_own_loop extraction is a straight dedup of the two existing bodies with the task-field offset parameterised. I checked that fail_outstanding_plugin_requests popping (which clears linked) followed by on_load's unlink (no-op on unlinked) then the deferred branch reaches the same counter state as the removed special case, and that on_defer reading bv2 before posting keeps the post-posting append_defer_promise call from touching the Load. No issues found, but the thread-affinity and queue-ordering reasoning is subtle enough that a maintainer familiar with the Mini-loop MPSC semantics should confirm it.
There was a problem hiding this comment.
I re-reviewed after 7b9bd4e (which addressed the earlier note about wiring the worker error event) and 8ac83ee; no bugs found this pass. This is a scan-counter accounting fix in the bundler's cross-thread plugin plumbing — new intrusive queue node, an unsafe fn post_to_own_loop helper, and reworked on_notify_defer/on_load bookkeeping — so a human look at the counter/queue-node invariants would still be worthwhile.
What was reviewed:
on_load's newdeferredbranch and the removed special case infail_outstanding_plugin_requests— the parked unit is moved back exactly once on both the answer and cancel paths.post_to_own_loopagainst the two prior open-coded copies — behavior-preserving;defer_taskis a distinct node so the answer and the notify no longer shareLoad::task.- The
is_linked()guard inon_notify_defer— both hops (Mini and JS) go through the same queue as the answer, so the ordering the guard relies on holds. - The three new
bundler_defer.test.tscases and the bake case — each asserts observable output (bundle contents / event ordering /terminated), not just absence-of-crash.
Extended reasoning...
Overview
Fixes a scan-counter double-decrement / intrusive-queue-node reuse bug when an onLoad plugin calls args.defer() without awaiting it. Touches src/bundler/bundle_v2.rs (adds Load::defer_task, extracts unsafe fn post_to_own_loop, adds on_defer_async, reworks on_notify_defer to take the Load and check is_linked(), moves the parked unit back in on_load, drops the parked-load special case from fail_outstanding_plugin_requests), src/bundler/Graph.rs (adds OutstandingLink::is_linked), and src/runtime/api/JSBundler.rs (deletes on_notify_defer_raw/on_notify_defer_mini_wrap and the hand-rolled loop dispatch in on_defer, now a three-liner). Tests: three new subprocess cases in test/bundler/bundler_defer.test.ts and one dev-server case in test/bake/dev/plugins.test.ts.
Security risks
None. No untrusted-input parsing, auth, or crypto surface; the change is internal bundler bookkeeping.
Level of scrutiny
High. pending_items == 0 is what lets wait_for_parse return and linking begin; miscounting here either hangs or starts linking while a parse worker is still writing into the graph. The fix also introduces a second intrusive MPSC queue node on Load and a new unsafe fn that generalizes the cross-thread hop, both of which carry lifetime/aliasing obligations spelled out in the # Safety doc. The refactor of on_load_async/on_resolve_async into the shared helper is behavior-preserving as far as I can tell (same Posted::Refused handling, same enqueue_task_concurrent_with_extra_ctx shape), but it is a refactor of a hot cross-thread path.
Other factors
My earlier inline note (wire the worker error event and fail fast if the build settles early) was applied in 7b9bd4e; the comment-cop threads were resolved by 8ac83ee and the remaining comments are one/two lines plus the required # Safety block. The PR description states the new tests fail with src/ reverted and pass with the fix on the ASAN debug build, and lists the sibling suites that were run. All review threads are resolved. Given the concurrency-sensitive nature of the counter/queue-node changes I'm deferring rather than approving.
|
Heads up on an overlap with #37729, which makes a kept That affects the two "after answering" tests here (the Whichever of the two lands second will need to drop or rewrite those two tests (the post-answer behaviour is covered by #37729's tests), and both PRs append to the end of |
…ep the drain task out of BundleV2 The task that resolves defer() promises mid-scan was embedded in the BundleV2 it was scheduled from. A plugin that answers without awaiting defer() lets the build finish and free the BundleV2 while that task is still queued on the JS thread, which then read the freed build. The task is now a small heap allocation holding only the plugin handle, which the completion task (Bun.build) or the dev server outlives it with. Promises that were still unsettled when the build completes are resolved from the completion (Bun.build) and from the dev server's bundle cleanup, so a defer() that was not awaited settles deterministically instead of depending on whether a drain happened to run. The mid-scan drain can never reject (the completion's result is not known until after the scan), so the rejected parameter and the result_is_err vtable entry it fed are removed.
…den the drain race window
|
Pushed 757026e and 365aff8. Self-review of the first revision found that the task resolving the defer() promises mid-scan ( |
|
Re the #37729 overlap: as of 365aff8 both "after answering" tests wrap the late |
There was a problem hiding this comment.
I reviewed the changes since my last pass (7b9bd4e addressed the fixture error-wiring; 757026e redesigned DeferredBatchTask to be heap-allocated and added drain_deferred() at build completion; 365aff8 widened the drain-race test window) and didn't find bugs. Given the cross-thread lifetime reasoning in the DeferredBatchTask rewrite and the new unsafe fn post_to_own_loop, a human look is still worthwhile.
What was reviewed:
DeferredBatchTasknow carries onlyNonNull<JSBundlerPlugin>; checked the ordering claim that the plugin handle outlives the queued task (Bun.build: completion task posts to the same queue after; bake: dev-server-lifetime plugin).on_loadreclaiming the parked unit before scheduling parse;fail_outstanding_plugin_requestsnow routes deferred loads throughon_load— traced thaton_load's guard drops the path/namespace on the error arm.on_notify_defer'sis_linked()early-return relies on defer-notify and answer sharing the same queue; verified both arms ofpost_to_own_looppost to the same target ason_load_async.JSBundlerPlugin__drainDeferredlosing therejectedparam — the only caller that passedtruewas the removedresult_is_errpath; new callers (completion, bake cleanup) always want resolve.
Extended reasoning...
Overview
The PR fixes scan-counter underflow/hang when an onLoad plugin calls .defer() without awaiting it. It touches src/bundler/{bundle_v2,Graph,DeferredBatchTask}.rs, src/runtime/api/{JSBundler,js_bundle_completion_task}.rs, src/runtime/bake/DevServer.rs, src/runtime/dispatch.rs, src/jsc/bindings/JSBundlerPlugin.cpp, plus tests in test/bundler/bundler_defer.test.ts and test/bake/dev/plugins.test.ts. Since my previous review, 757026e rewrote DeferredBatchTask from an intrusive BundleV2 field to a heap-allocated task carrying only the plugin handle, and added drain_deferred() at build completion on both the Bun.build and dev-server paths.
Security risks
None identified. This is internal bundler bookkeeping; no user-controlled input reaches new parsing or path handling.
Level of scrutiny
High. The change reasons about cross-thread task lifetimes (the drain task can outlive its BundleV2; the plugin handle it carries must not be destroyed until after the task runs or is released), adds a second intrusive queue node (Load::defer_task) to avoid re-enqueueing the same node, and introduces unsafe fn post_to_own_loop that takes a caller-supplied field offset. pending_items == 0 gates linking, so an accounting error here can start linking while a parse worker is still writing into the graph. This is exactly the class of change REVIEW.md flags for careful human review (intrusive nodes, cross-thread refcounts, "who frees this, when, on which paths").
Other factors
- My earlier feedback (wire the worker
errorevent in the cancellation fixture) was addressed in 7b9bd4e. - The author noted an overlap with #37729 that will require rewriting the two "after answering" tests when whichever lands second — a merge-order coordination point a human should be aware of.
- The seven open comment-cop bot flags on the latest commit target module/doc comments and
# Safetysections; they look like heuristic noise rather than substantive feedback, but they're technically unresolved. - Test coverage is thorough (four new subprocess-spawned cases plus two dev-server cases), and the PR description states they fail with
src/reverted.
Problem
onLoadplugin that callsargs.defer()but answers without awaiting it takesBun.builddown. Debug build:panic: int cast: TryFromIntError(NegOverflow)inBundleV2::on_parse_task_complete; depending on timing it is insteadpanic: internal error: entered unreachable codeinBundleV2::on_load, a hang (what the released build mostly does), or ASANheap-use-after-freeinDeferredBatchTask::run_on_js_thread.defer()parks the load's unit of the scan counter, and an answer that arrived while it was parked did not take it back, so the load was counted twice. The counter then no longer described the in-flight work: underflow, hang, or linking starting while a parse was still writing. On the dev server this showed as other loads'defer()promises resolving before the answer's own imports were loaded.Bun.buildthread thedefer()notification and the answer were queued on the same intrusive queue node, so queuing the second rewrote the first: a lost notification, a dropped neighbouring task, or one node run twice.defer()promises lived inside theBundleV2and walked back to it when run. Without an await the build can finish and free theBundleV2while that task is still queued; whether the promises ever settled depended on the same timing.Fix
defer()notification for a load that was already answered is ignored (bundler: make a kept onLoad args.defer() throw instead of touching the freed build #37729 makes that call throw on the JS side). Property to check: every load puts exactly one unit through the counter, whatever the order of its two messages.defer()notification gets its own queue node, so a load's two messages cannot overwrite each other, and both use the same queue, so they arrive in the order issued.Bun.buildand the dev server; never rejected, since nobody may be awaiting them.Background
Graph::pending_items) counts outstanding loads, resolves and parses; when it reaches zero the scan is over and linking starts, so a counter that is too low starts linking while workers still write into the graph. Each outstandingonLoadrequest holds one unit, and its answer hands the unit to the parse it schedules.args.defer()lets anonLoadcallback wait until every other module has been loaded. It moves the load's unit intoGraph::deferred_pending; whenpending_itemshits zero the parked units move back and a task posted to the JS thread resolves the promises.Bun.buildruns the bundle on its own thread with a Mini event loop, and plugin callbacks post messages back to it through queue nodes embedded in eachLoad; the dev server (bake) runs the bundle on the JS thread itself, so the same messages travel through the JS loop.Bun.buildthe plugin object is destroyed by the completion task, posted to the same JS queue after the drain; the dev server keeps its plugin for its own lifetime. That is what lets the drain task hold only the handle.Original description
What does this PR do?
An
onLoadplugin that callsargs.defer()but answers without waiting for the promise takesBun.builddown. Debug build:Depending on timing the same script instead dies with
panic: internal error: entered unreachable codeinBundleV2::on_load, never finishes (what the released build mostly does), or, once the counters are right, reads a freedBundleV2from the task that resolves the promise (ASAN:heap-use-after-freeinDeferredBatchTask::run_on_js_thread). Repro:Cause
Every outstanding
onLoadrequest holds one unit ofGraph::pending_items..defer()moves that unit intoGraph::deferred_pending(on_notify_defer) anddrain_deferred_tasksmoves it back once the rest of the scan reaches zero; the answer then hands the unit to the parse task (or consumes it on the error paths). Three things went wrong when the answer arrived while the unit was still parked:on_loaddid not know the unit was parked. It scheduled the parse anyway, whose completion decrementedpending_itemsa second time for the same load. From there the counter no longer describes the in-flight work: it underflows (NegOverflow), or a later drain moves the parked unit back with nothing left to consume it (hang). Sincepending_items == 0is what letswait_for_parsereturn and linking start, a counter that is too low can also start linking while a parse worker is still writing into the graph. On the dev server, which runs the bundle on the JS loop, the same mismatch showed up as an early drain: thedefer()promise of a load that was genuinely waiting resolved before the misbehaving load's own imports had been loaded.On the
Bun.buildthread the.defer()notification and the answer were queued on the same intrusive node,Load::task. Queuing a node that is still in the MPSC queue rewrites its callback andnextlink: the notification is replaced by the answer, some other queued task is dropped (hang), or the node runs twice and the secondon_loadhits theunreachable!()on an already consumed value.DeferredBatchTask, the task posted to the JS thread to resolve the promises, was embedded in theBundleV2and walked back to it when run. With a properawaitthat is fine: the pass cannot finish until the resumed callbacks answer, which happens after the task ran. Without one, the answer is already queued when the drain is scheduled, so the pass finishes andinit_and_rundrops theBox<BundleV2>while the task is still queued behind whatever the JS thread is doing; the task then reads the freed pass. Whether the promise ever settled depended on the same timing (in a probe, all promises of a build settled in 3 to 8 percent of builds, none otherwise; onservethe shared plugin handed them to the next build's drain).The
Load::deferredflag that records where a load's unit lives (added in #37075 for cancellation) was also only set on theBun.buildarm; the dev server arm posted the notification without the load.Fix
on_loadtakes the unit back (deferred_pending -= 1,increment_scan_counter()) when the load is stilldeferred, so the answer is accounted for like any other load's.fail_outstanding_plugin_requestsno longer needs its own handling of parked loads and routes them throughon_load(the only difference is a cancellation message per parked load in a log a cancelled build never reports).on_notify_defertakes the load on both arms, sets the flag, and does nothing for a load that has already been answered (no longer inoutstanding_loads,OutstandingLink::is_linked). Both notifications travel through the same queue as the answer, so the check is exact. Callingdefer()after answering is a misuse; bundler: make a kept onLoad args.defer() throw instead of touching the freed build #37729 makes the JS side throw for it, and this check is what keeps a call that does reach the bundler from corrupting the counters.Load::defer_task(called_deferalready limits it to one enqueue); the answer keepsLoad::task. The post-to-the-owning-loop dispatch thaton_load_asyncandon_resolve_asynceach spelled out is one helper,post_to_own_loop, which the newon_defer_asyncuses too;JSBundler::on_deferjust calls it.DeferredBatchTaskis now a small heap allocation holding only the plugin handle, created per drain (Task::from_boxedstyle: the dispatch arm consumes the box,release_unrunfrees it at VM teardown,schedulefrees it if the post is refused, whichenqueue_on_js_loop_for_pluginsnow reports). It never touches the pass. The handle outlives it: forBun.buildthe plugin is destroyed by the completion task, which is posted to the same queue after the drain; bake's plugins live as long as the dev server. The actual drain call moves to the runtime (PluginJscExt::drain_deferred, which does the exception-scope check the dispatch arm used to do), so the bundler no longer declares that FFI entry point.JSBundleCompletionTask::on_complete(Bun.build, including theserveHTML routes that share a plugin) and from the dev server'sfinalize_bundle_cleanup, so adefer()that was not awaited settles exactly once the build is done, which is what the docs promise. They are always resolved, never rejected: rejecting a promise nobody awaited would surface as an unhandled rejection in the user's process. The mid-scan drain could never reject either (the completion's result is not set until after the scan), sodrainDeferred'srejectedparameter and theresult_is_errvtable entry that fed it are removed.Residual, unchanged in kind from before: the production bake CLI also drives the Js arm, and a drain scheduled by a misbehaving plugin that then fails its load could in principle still be queued when that command tears its plugin down. Before this change the same task would have read the freed pass instead.
How did you verify your code works?
test/bundler/bundler_defer.test.ts,describe("defer() that is not awaited"), each case spawning a script because the failures take the process down:onLoadall calldefer()without awaiting it: the bundle contains exactly the plugin-provided modules (the on-disk files throw) and every promise has settled by the timeBun.buildresolves. Unfixed: theNegOverflowpanic (or a hang, or unsettled promises, depending on timing).DeferredBatchTask::run_on_js_thread, 3 of 3 runs; the windows are wide enough that it still fired on a heavily loaded machine.xawaitsdefer(),yanswers synchronously and callsdefer()from a microtask (in a try/catch, since bundler: make a kept onLoad args.defer() throw instead of touching the freed build #37729 makes that call throw),y's answer importsz;zis loaded beforexresumes. Unfixed: hang (node collision) orxresuming early.worker.terminate()) while one load is parked indefer()and another is unanswered. Passes before and after; it guards thefail_outstanding_plugin_requestschange.test/bake/dev/plugins.test.tscovers the dev server arm: the after-answering case (unfixed:xresumes beforezis loaded), and a load that answers without awaitingdefer()whose promise must settle only after the module its own answer imports has been loaded (unfixed: resolved as soon as the scan momentarily looked empty, reported asfalseby the route). The second case ordersw's answer afterx'sdefer()so the scan is never empty at that moment; otherwise resolving immediately would be correct.With the fix:
bundler_defer.test.ts(14),bake/dev/plugins.test.ts(5),bundler_plugin.test.ts(53),bundler_plugin_chain.test.ts(13), thecountedfamily ofworker-terminate-funnels.test.ts(the cancelledBun.buildcase) andtest/internal/source-lintspass on the debug (ASAN) build;cargo fmt,cargo clippy -p bun_bundler -p bun_runtimeandclang-formatare clean. Withsrc/reverted to the merge base, the two before-answering cases and both bake cases fail as described above.Overlap with open PRs: #37709 touches
DeferredBatchTaskand thedrain_deferred_taskscall to stop the old task from forming&mut BundleV2; this PR removes that walk-back altogether, so whichever lands second drops that part. #37729 adds the JS-side guard and its own tests tobundler_defer.test.ts; the tests here already tolerate it.