bundler: abort on allocation failure when enqueuing a plugin-resolved entry point - #41041
bundler: abort on allocation failure when enqueuing a plugin-resolved entry point#41041robobun wants to merge 1 commit into
Conversation
… entry point
on_resolve returned silently when enqueue_entry_item failed. That call
increments the scan counter before its fallible allocations, so a failure
left pending_items one too high and the build never finished. Use the
same .expect("oom") the Success arm uses for the same failure.
Clears the stale defaulted_failure baseline entry for bundle_v2.rs.
|
Warning Review limit reached
On-demand reviews are free for the next 20 days. After that, they cost $0.25 per reviewed file. Or wait 10 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 4:39 AM PT - Aug 31st, 2026
❌ @robobun, your commit 953005f has 1 failures in
🧪 To try this PR locally: bunx bun-pr 41041That installs a local version of the PR into your bun-41041 --bun |
There was a problem hiding this comment.
LGTM — converts a hang-on-OOM into the file's standard controlled abort.
What was reviewed:
- Traced
enqueue_entry_item:increment_scan_counter()at :2819 runs before the two?sites (:2828, :2867), so the old silentreturndid strandpending_items— the hang analysis holds. .expect("oom")matches the file's local convention (40 occurrences, 0 ofunwrap_or_oom()) and mirrors theSuccessarm's handling of the samepath_with_pretty_initializedcall at :4937.- Baseline removal is self-verifying via the mordant CI check; no CODEOWNERS cover the touched paths.
Extended reasoning...
Overview
This PR changes one call site in src/bundler/bundle_v2.rs inside on_resolve's NoMatch → EntryPointBuild arm, replacing let Ok(source_index) = this.enqueue_entry_item(..) else { return; } with .expect("oom"), and drops the corresponding defaulted_failure entry from mordant-baseline.toml. I verified the PR's core claim against source: enqueue_entry_item calls self.increment_scan_counter() (which bumps self.graph.pending_items) at line 2819, before its two fallible ? operations at lines 2828 (path_with_pretty_initialized) and 2867 (input_files.append). Both are arena/store allocation paths. If either failed, the previous code returned silently with pending_items one too high and no parse task enqueued to pay it back, so is_done() would never fire — a hang, exactly as described.
Security risks
None. This is bundler-internal control flow around allocation failure; no user-input parsing, no auth/crypto/permissions, no boundary validation is touched. The change tightens behavior (hang → controlled abort) on a path that only runs when the arena cannot allocate.
Level of scrutiny
Low-to-moderate. The change is a single expression rewrite plus a lint-baseline line removal. The .expect("oom") choice matches the file's overwhelming local convention (40 occurrences vs. 0 of unwrap_or_oom()), and the sibling Success arm of the same match already handles the identical path_with_pretty_initialized failure with .expect("oom") at line 4937 — so this brings the two arms into agreement rather than introducing a new pattern. REVIEW.md's "Route allocation failure through bun_core::handle_oom" and src/CLAUDE.md's unwrap_or_oom() guidance are satisfied in spirit (controlled crash on OOM), and the "match the exact file's local conventions" rule takes precedence for the specific spelling.
Other factors
The three-line comment explains a non-local invariant (the callee has already incremented a counter, so bailing out hangs) — that is information a reader cannot derive from the call site, so it earns its place despite the one-line preference. No new test is added, but the changed path is reachable only on allocator failure and the fix converts undefined-wait into abort; there is no observable behavior to assert against short of fault injection, and the author ran the relevant bundler plugin suites for regression coverage. The baseline count discrepancy (2 recorded, 1 site fixed) is addressed in the PR description as a stale count with a cross-reference to #40309, and is mechanically enforced by the mordant CI check — a wrong removal would fail CI rather than merge silently. No CODEOWNERS entries cover src/bundler/ or the baseline file, and there are no prior reviews or outstanding objections on the timeline.
Problem
BundleV2::on_resolve(src/bundler/bundle_v2.rs:4818) handles a pluginonResolvethat returned no match for an entry point by resolving it from disk and callingenqueue_entry_item. The call waslet Ok(source_index) = this.enqueue_entry_item(..) else { return; }.enqueue_entry_itemcallsincrement_scan_counter()(line 2819) before its two fallible steps,path_with_pretty_initialized(..)?(line 2828) andinput_files.append(..)?(line 2867). Both fail only on allocation. When one fails,pending_itemsstays one too high with no parse task to pay it back,is_done()never fires, and the build waits forever instead of failing. The mordant lintdefaulted_failurereports this site fordefaulted_failure:src/bundler/bundle_v2.rs.Fix
.expect("oom"). This is what theSuccessarm of the same function does for the samepath_with_pretty_initializedfailure (line 4934), and what the workspace does for allocation failure in general (bun_core::handle_oom): a controlled abort, not a hang."defaulted_failure:src/bundler/bundle_v2.rs" = 2line frommordant-baseline.toml. The count was stale: the pinned mordant reports one site on main, this one. Verified locally:bun_bundlerreports no finding over the baseline. bundler: narrow the generated source index once with Index::init #40309 lowers the same line to 1 for the stale half, so one of the two PRs will need a trivial rebase on this file.test/bundler/bundler_plugin.test.ts,bundler_plugin_chain.test.ts,bundler_edgecase.test.tswith the debug build. No new test: the changed path runs only when the arena or the filename store cannot allocate.Background
pending_itemsis the bundler's count of work still in flight. Every enqueued parse task adds one, every completed task subtracts one, and the bundle finishes when it reaches zero.on_resolveruns when a JS plugin'sonResolvecallback settles.EntryPointBuildrecords are the entry points themselves. A plugin that does not claim one lets the file resolver handle it.mordant-baseline.tomlis a ratchet of lint findings accepted as pre-existing. CI fails only when a (lint, file) pair grows past its count. Fixing a site lets its line be removed.Notes
CI on this PR: 180 of 181 jobs pass. The one red job is
test/cli/run/require-cache.test.tson debian 13 x64-asan, a require.cache leak test that times out. It is marked pre-existing (also red on main) and is with main-break triage. The other failures in that build passed on retry.bundler: resolve in-memory entry points when an onResolve plugin declines them #38600 (open, 2026-08-14) reshapes the same
on_resolveblock for a different reason (in-memory entry points that anonResolveplugin declines) and is behind main on this function's signature. Whichever lands second needs a small rebase here.Findings on main at the pinned mordant (
12d2797), with an empty baseline:defaulted_failurereports one site inbundle_v2.rs, line 4818. The baseline's count of 2 was recorded at an older mordant revision. Other stale entries found the same way, left alone because open PRs already touch those lines:[bun_ast]and[bun_sql_jsc]empty sections,h2_frame_parser.rs32 (31 real),reimplemented_helper:Terminal.rs(gone).resolve_entry_pointon the line above keeps itslet Ok(..) else { return }: it logs the resolution error into the build log before returningErr, so the build fails with a message.