Repository navigation
Conversation
|
Updated 10:30 PM PT - Jul 25th, 2026
❌ @robobun, your commit 112f49c has 2 failures in
Add 🧪 To try this PR locally: bunx bun-pr 33271That installs a local version of the PR into your bun-33271 --bun |
||||||||||||||||||||||
WalkthroughAdds two new JSC host promise handlers for JS bundler plugin setup, extends the plugin setup pipeline to suspend on pending promises, and updates Bun.build to resume or reject the deferred build later. Adds tests for async setup ordering, suspension, and failure handling. ChangesAsync plugin setup suspension
Sequence Diagram(s)sequenceDiagram
participant Build as build()
participant ConfigFromJs as Config::from_js
participant DeferredBuild
participant SetupPromise as pending setup promise
participant OnResolve as Bun__JSBundler__onResolvePluginSetup
Build->>ConfigFromJs: from_js(..., &mut suspended)
ConfigFromJs-->>Build: Ok(None) + SuspendedPluginSetup
Build->>DeferredBuild: create, attach .then handlers
Build-->>Build: return build promise immediately
SetupPromise-->>OnResolve: resolved
OnResolve->>DeferredBuild: resume(next_index)
DeferredBuild->>ConfigFromJs: run_one_plugin_setup / finish_from_js
DeferredBuild-->>DeferredBuild: schedule bundling completion
Related PRs: None identified. Suggested labels: bundler, javascript, async Suggested reviewers: None identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/api/JSBundler.rs`:
- Around line 459-476: The SuspendedPluginSetup state keeps pending as a bare
JSValue, which leaves the promise unrooted during the gap before .then() is
attached in the JSBundler setup flow. Change SuspendedPluginSetup to store
pending as a jsc::Strong, initialize it before allocating
DeferredBuild/JSPromiseStrong state, and use pending.get() when wiring the
.then(...) callback so the promise stays GC-rooted across the suspend/resume
path.
In `@test/bundler/bun-build-plugin-async-setup.test.ts`:
- Around line 4-15: Trim the long comment in the test to only durable,
non-obvious guidance: keep the reference to Bun.build(plugin) async setup
behavior and the invariant that setup() must not synchronously block config
parsing, while removing the bug history, event-loop mechanism details, and
issue-link narrative. Update the comment near Bun.build and setup() so it stays
within 3 lines and leave the rest of the context in the PR description.
- Around line 16-190: The tests under Bun.build with an async plugin setup() are
all isolated and should run concurrently, but they are currently defined with a
plain describe/test structure. Update the suite to use describe.concurrent or
mark each test as test.concurrent so the tempDir-backed file writes and the
Bun.spawn subprocess do not serialize unnecessarily. Keep the existing test
bodies and unique scenarios (like the gated build, sleepy setup, starter
onStart, and explosive rejection) unchanged while switching the test
declarations to concurrent form.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a83dac9e-f170-417d-87be-caa7559cad8a
📒 Files selected for processing (5)
src/jsc/bindings/ZigGlobalObject.cppsrc/jsc/bindings/ZigGlobalObject.hsrc/jsc/bindings/headers.hsrc/runtime/api/JSBundler.rstest/bundler/bun-build-plugin-async-setup.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find bugs, but this restructures Bun.build's plugin-setup path around a heap-leaked DeferredBuild with manual lifetime, GC rooting across async boundaries, and a documented behavior change (async setup() rejection now rejects the returned promise instead of throwing synchronously) — worth a human pass on the ownership transfers and the CodeRabbit rooting note before merging.
Extended reasoning...
Overview
This PR removes the waitForPromise call from Config::from_js in src/runtime/api/JSBundler.rs, which previously spun the event loop synchronously while waiting for an async plugin setup() to settle. It replaces that with a suspend/resume model: from_js now returns Ok(None) plus a SuspendedPluginSetup when a step yields a pending promise, and build() boxes that state into a leaked DeferredBuild whose .then reactions (registered as two new PromiseFunctions in ZigGlobalObject.h/.cpp/headers.h) continue the plugin chain, finish the config parse, and schedule the bundle. A new test file exercises the non-blocking return, chained plugins, onStart ordering, post-await config mutation, and rejection.
Security risks
None identified — no auth/crypto/permissions/parsing-of-untrusted-input surface is touched. The change is confined to bundler config parsing and promise plumbing.
Level of scrutiny
High. Bun.build is a core, widely-used API and the change introduces:
- Manual heap ownership via
bun_core::heap::into_raw/takeofDeferredBuild, reclaimed by exactly one of two promise reactions (or re-leaked on re-suspension). Any path where neither reaction runs, or both run, is a leak/UAF. - JS values held across an async boundary:
config_js,plugins_array, andresultarejsc::Strong, butSuspendedPluginSetup::pendingis a bareJSValue(CodeRabbit flagged this). It's consumed within the same synchronousbuild()frame before.then()attaches, so it's likely fine, but that's exactly the kind of GC-window reasoning a maintainer should confirm. - Ownership transfer of the
Pluginregistry between the scopeguard,SuspendedPluginSetup,DeferredBuild, and the completion task, with disarm points on each path. - A user-visible behavior change: rejecting async
setup()(or a config-parse error after an async step) now rejects the build promise rather than throwing synchronously fromBun.build(...).
Other factors
The PR description is thorough, tests look well-targeted (including a subprocess hang guard verified against USE_SYSTEM_BUN=1), and no regressions are reported across the existing plugin suites. CodeRabbit left one substantive inline (pending rooting) and two style nits that are unaddressed. Given the memory/GC sensitivity and the API behavior change, this exceeds the bar for auto-approval.
|
Checked #14539 before adding a |
There was a problem hiding this comment.
No issues found, but this reworks Bun.build's plugin setup control flow with manual heap lifecycle (into_raw/take) for DeferredBuild, GC rooting across suspend/resume, and a documented behavior change (async setup() rejection now rejects instead of throwing) — worth a human look.
Extended reasoning...
Overview
This PR eliminates a waitForPromise call in Config::from_js that made Bun.build synchronously spin the event loop when a plugin's async setup() returned a still-pending promise. The fix converts the plugin setup chain into a proper promise continuation: when a step is pending, config parsing suspends into a heap-allocated DeferredBuild, Bun.build returns its result promise immediately, and the remaining plugins plus finish_from_js run from the .then reaction. The C++ side (ZigGlobalObject.{h,cpp}, headers.h) is mechanical — two new PromiseFunctions enum entries and host-function declarations. The substantive change is ~300 net lines in src/runtime/api/JSBundler.rs introducing SetupStep, SuspendedPluginSetup, DeferredBuild, run_one_plugin_setup, finish_from_js, and two host callbacks.
Security risks
None identified. No auth, crypto, permissions, or untrusted-input parsing surfaces are touched.
Level of scrutiny
This deserves careful human review rather than auto-approval:
- Manual memory lifecycle:
DeferredBuildis leaked viaheap::into_rawand reclaimed viaheap::takein exactly one of two promise reactions, then re-leaked if the chain suspends again. The invariant that exactly one reaction fires per.thenis load-bearing for both UAF and leak avoidance. ThePluginregistry ownership also transfers between the scopeguard,SuspendedPluginSetup,DeferredBuild, and finally the completion task orPlugin::destroyon failure — several handoff points to verify. - GC rooting across async boundaries:
config_js,plugins_array, the pending promise, and the result promise are all held across event-loop turns viajsc::Strong/JSPromiseStrong. CodeRabbit already caught one unrootedJSValue(fixed in 78a2c43), which suggests this area benefits from a second pair of eyes. - Behavior change: On the previously-blocking path, a rejecting async
setup()(or a config parse error surfaced after an async setup settles) now rejects the returned promise instead of throwing synchronously from theBun.build(...)call site. This is documented in the PR description but is a user-visible semantic shift. - Control-flow refactor of a core public API:
from_jsis split intofrom_js+run_one_plugin_setup+finish_from_js, with the plugin loop now callable both synchronously and from a promise reaction. Theis_lastcomputation changed fromi == length.saturating_sub(1)toindex + 1 == length, and iteration switched fromarray_iteratorto indexedget_index— both look correct but are the kind of refactor detail worth confirming.
Other factors
Test coverage is good (6 new tests covering the hang repro via subprocess with SIGKILL guard, I/O-suspended setup, onStart ordering, multi-plugin resume, post-await config mutation, and rejection propagation), and the PR description reports the relevant existing plugin test suites still pass. All prior bot review comments (GC rooting, comment length, test concurrency) are marked addressed in 78a2c43. The bug-hunting pass found nothing. Still, the combination of manual heap ownership, GC-rooting subtlety, and a public-API behavior change puts this outside the auto-approve threshold.
Config::from_js ran each plugin's setup() during synchronous config parsing and, when runSetupFunction returned a promise that was still pending, called waitForPromise to spin the event loop until it settled. That re-enters the event loop from whatever callback called Bun.build (the bug class tracked in #33261) and also deadlocks deterministically when the code that would settle the promise is later on the same stack. Bun.build already returns a promise, and the runSetupFunction builtin already folds a pending async setup() together with any pending onStart() promises into the one promise it hands back to Rust. So: split Config::from_js at the plugin loop, and when a step's promise is still pending, park the parse state in a heap DeferredBuild and finish the chain, the rest of the config parse, and the bundle scheduling from that promise's .then continuation. The synchronous path (no plugins, or every setup() and onStart() settled before returning) is unchanged. The only observable difference is on the path that previously blocked: a config-parse error raised after an async setup() settles now rejects Bun.build's promise instead of throwing synchronously, and a rejecting async setup() does the same.
SuspendedPluginSetup held the still-pending promise runSetupFunction returned as a bare JSValue across the gap between Config::from_js recording the suspension and build() attaching the .then reactions. Hold it in a jsc::Strong instead so the rooting is explicit rather than relying on the JS settling chain and the conservative stack scan; this is also what ServePlugins does for the analogous promise. Also make the new test suite concurrent and trim its header comment.
78a2c43 to
14ca1fc
Compare
|
Rebased onto main (was 594 commits behind, applied cleanly) and added a test for the Local verification after rebase: Regression suites unchanged: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
comment-cop flags every added multi-line comment on claude-labeled PRs. The flagged blocks were ordinary doc comments, not workaround justifications, but they were also more verbose than they needed to be: several restated the same #33261 invariant. Each block is now a single line; the SAFETY comments on unsafe blocks are exempt and unchanged.
|
Trimmed every flagged doc comment to one line in 112f49c (comment-cop fires on any added 2+-line comment on |
There was a problem hiding this comment.
I didn't find any bugs, but this reworks Bun.build's plugin-setup control flow around a heap-leaked DeferredBuild that owns a raw *mut Plugin across suspension points, adds two native promise reactions, and changes user-visible behavior (async setup() rejection now rejects the returned promise instead of throwing synchronously). Worth a human look at the ownership/GC contracts.
What was reviewed:
*mut Pluginownership across all terminal paths (sync scopeguard, suspended →DeferredBuild, resume success/fail/re-suspend, reject reaction) — every path either moves it to the completion task or callsPlugin::destroy.- GC-rooting of the pending promise:
SuspendedPluginSetup.pending: Strongstays alive throughBox::new(DeferredBuild)and drops after.thenattaches; the re-suspend path inon_deferred_build_resolvehaspendingon-stack with no allocation before.then. Config::from_jshas a single caller (build()), so the signature change is contained;is_last = index + 1 == lengthmatches the oldi == length.saturating_sub(1).promiseFunctionsSizebumped 42→44 to match the two new enum variants.
Extended reasoning...
Overview
This PR removes the waitForPromise call from Config::from_js in src/runtime/api/JSBundler.rs and replaces it with a promise-chaining continuation. When a plugin setup() returns a still-pending promise, from_js now records where it stopped in a SuspendedPluginSetup, build() heap-allocates a DeferredBuild owning the partial state, and the pending promise's .then reactions (two new PromiseFunctions entries wired through ZigGlobalObject.{cpp,h} and headers.h) resume the chain one plugin at a time. The post-plugin config parsing was extracted into finish_from_js, and the per-plugin loop body into run_one_plugin_setup, both shared between the sync and deferred paths. A new 7-test file exercises the gated-hang repro, chained plugins, onStart() ordering, config mutation after await, Promise.race escape, and async rejection.
Security risks
None identified. No new untrusted-input parsing; the plugin array and config object were already read from user JS by the same accessors. No auth/crypto/permission surface.
Level of scrutiny
High. This is core Bun.build runtime code with manual memory ownership: a Box<DeferredBuild> is leaked via heap::into_raw and reclaimed in exactly one of two native .then reactions (or re-leaked on further suspension), and it holds a raw *mut Plugin whose destruction must happen on every failure path. It also holds three jsc::Strongs and a JSPromiseStrong that must not leak or be dropped early. The PR description explicitly calls out a user-visible behavior change (async setup() rejection → promise rejection instead of synchronous throw), and this is part of a larger event-loop-reentry audit (#33261). REVIEW.md's memory-safety section — "every acquisition paired with release at the acquisition site", "exactly one named owner, released exactly once" — applies directly here, and a maintainer should confirm the ownership graph.
Other factors
The bug-hunting system found nothing. All prior review threads (CodeRabbit's GC-rooting concern, comment-cop's doc-comment length flags, describe.concurrent) are resolved and the fixes are visible in the diff. Tests look solid: subprocess fixtures for the hang cases with SIGKILL guards, and the author verified fail-on-system-bun / pass-on-debug plus the four adjacent regression suites. The is_last computation, scopeguard disarm placement, and single-caller signature change all check out. But the combination of heap-leaked FFI context, raw plugin pointer ownership across async boundaries, new C++ enum wiring, and an intentional API behavior change puts this well outside the "simple/mechanical" bar for auto-approval.
|
CI on 112f49c (build 81943) is red on infra, not the diff:
Happy to re-roll once the fleet recovers, but doing it now would discard 85 green test lanes for the same starved queue. |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-26, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
Part of the #33261 audit (work item 3).
Repro
An async plugin
setup()whose promise has not settled by the timesetup()returns makesBun.buildspin the event loop at 100% CPU. When the thing that settles it is later on the same stack, that is a permanent hang:Hangs on 1.3.x, every time.
Cause
Config::from_js(src/runtime/api/JSBundler.rs) runs each plugin'ssetup()during synchronous config parsing and, when therunSetupFunctionbuiltin hands back a promise that is still pending, callswaitForPromise, which runs the whole event loop until it settles.Bun.buildis reachable from any JS, including an I/O completion callback, so this is the synchronous re-entry #33261 forbids: a nestedepoll_wait/keventinside a poll-dispatch callback overwrites the shared ready-poll batch and loses one-shot events. It is also the deterministic hang above, since the nested loop blocks the JS frame that would have settled the promise.That one call was also the only implementation of the documented "the bundler waits until all
onStart()callbacks have completed before continuing" guarantee: the builtin folds a pending asyncsetup()together with the last plugin's pendingonStart()promises into the single promise it hands back.Fix
Chain instead of blocking.
Bun.buildalready returns its own promise, so:Config::from_jsno longer blocks. When a step's promise is still pending it records where the chain stopped (SuspendedPluginSetup) and returns early. The chain has to advance one plugin at a time becauserunSetupFunctionneeds the previous plugin's settledonStart()array.build()creates the result promise immediately and parks the parse state in a heapDeferredBuildowned by the pending promise's.thenreactions (registered inPromiseFunctions, like every other native promise reaction). The resolve reaction runs the remaining plugins, re-suspending on each further pending promise, then the rest of the config parse, then schedules the bundle, moving the already-returned promise into the completion task. The reject reaction rejects it.finish_from_jsand defers with the chain.The synchronous path (no plugins, or every
setup()andonStart()settled before returning) is unchanged. TheonStart()-gates-the-bundle guarantee is preserved: the bundle task is not created until the chain settles.Behavior change
Only on the path that used to block. A rejecting async
setup()(oronStart()) now rejects the returned promise instead of throwing synchronously from theBun.build(...)call, and so does a config parse error raised after an asyncsetup()settles. Synchronoussetup()errors still throw synchronously.Verification
test/bundler/bun-build-plugin-async-setup.test.ts.No regressions:
bun-build-api.test.ts(48 pass), andbundler_plugin.test.ts+bundler_plugin_chain.test.ts+plugin-error-nested-throw.test.ts+plugin-sync-exception-fallback.test.ts(69 pass across the four).The audit of the other non-top-level
waitForPromisecallers (the rest of work item 3) is on #33261. The remaining violation,bake_body.rs:378viaBun.serve({ app: { plugins } }), needs a different shape (Bun.servereturns aServer, not a promise) and is left for a follow-up.[review] gate passed · iteration 1 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file