Conversation
|
Updated 12:27 PM PT - Mar 25th, 2026
❌ @robobun, your commit 04ec29e has 1 failures in
🧪 To try this PR locally: bunx bun-pr 28425That installs a local version of the PR into your bun-28425 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28422.test.ts`:
- Around line 10-62: The test "isolated install with workspace monorepo
completes without hanging" currently creates only six workspaces which won't
exercise the quadratic resumeUnblockedTasks path; modify the tempDir setup in
this test (the using dir = tempDir(...) call) to programmatically generate a
larger workspace graph (e.g., 50+ packages and several apps) and add
cross-workspace workspace:* dependencies to create higher connectivity (for
example, create packages pkg-0..pkg-49 and apps app-0..app-9 and make each app
depend on many packages and packages depend on a few other packages) so the
installer traverses the expensive resumeUnblockedTasks behavior; keep the
existing assertions but scale the graph generation to be deterministic and fast
(use loops to build the package.json objects and inject them into the tempDir
map).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b8dd7787-848f-4555-8e8f-9da8a3ede0d2
📥 Commits
Reviewing files that changed from the base of the PR and between 7336160 and 2b6073e9e24bfa2ee1333dfe2d89cfeff0e20853.
📒 Files selected for processing (2)
src/install/isolated_install/Installer.zigtest/regression/issue/28422.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The O(N²)→O(B×D) optimization looks algorithmically correct and the threading invariants appear sound, but the change touches concurrent install scheduling code with subtle acquire/release semantics — worth a human eye before landing.
Extended reasoning...
Overview
This PR modifies src/install/isolated_install/Installer.zig to replace a full O(N) scan of all store entries in resumeUnblockedTasks with targeted iteration over a new blocked_entries hashmap. A regression test is added in test/regression/issue/28422.test.ts.
Security Risks
None. This is a pure performance optimization in the package install subsystem with no authentication, permissions, or data-exposure concerns.
Level of Scrutiny
High. The change sits in concurrent install scheduler code where main-thread/worker-thread coordination relies on specific atomic orderings (.monotonic vs .acquire/.release). The blocked_entries field is correctly documented as main-thread-only and is only mutated in onTaskBlocked and resumeUnblockedTasks, both called exclusively from the main thread. The two-phase collect-then-unblock pattern in resumeUnblockedTasks is necessary and correct. However, subtle scheduling bugs in this area could cause tasks to never unblock (deadlock) or be started twice (double-free/corruption), so a human reviewer familiar with the install subsystem should confirm the invariants hold under all code paths — including the onTaskFail path that sets step to .done without touching blocked_entries.
Other Factors
The inline bug comments identify two minor nits in the test file only (unused write/readlinkSync imports, and exitCode asserted before symlink checks contrary to the CLAUDE.md convention). These do not affect correctness. The core Zig logic change is well-reasoned and the described complexity improvement is accurate.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/regression/issue/28422.test.ts (1)
6-52:⚠️ Potential issue | 🟠 MajorRegression load is still likely too small to reliably catch the quadratic path.
With
PKG_COUNT = 20andAPP_COUNT = 5, this fixture can still pass on the pre-fix implementation, so it may not guard against reintroducing the O(N²) behavior.⚙️ Suggested strengthening
-const PKG_COUNT = 20; -const APP_COUNT = 5; +const PKG_COUNT = 75; +const APP_COUNT = 15; -// Generate 50 packages with chained dependencies: pkg-i depends on pkg-(i-1) and pkg-0 +// Generate workspace packages with chained and shared dependencies -// Generate 10 apps, each depending on a spread of packages +// Generate apps with broad cross-workspace connectivity🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/regression/issue/28422.test.ts` around lines 6 - 52, The regression fixture is too small to reliably catch the O(N²) behavior; increase PKG_COUNT and APP_COUNT (e.g., PKG_COUNT from 20 to 100+ and APP_COUNT from 5 to 20+) in the test "isolated install with workspace monorepo completes without hanging" to amplify workload, and make the apps' dependency spread denser (change the step in the for loop that adds every-5th package to every-1-or-2nd package or add additional randomized deps) so connectivity is higher; optionally run the test multiple iterations or add a simple timeout/assertion to fail fast on regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/regression/issue/28422.test.ts`:
- Around line 6-52: The regression fixture is too small to reliably catch the
O(N²) behavior; increase PKG_COUNT and APP_COUNT (e.g., PKG_COUNT from 20 to
100+ and APP_COUNT from 5 to 20+) in the test "isolated install with workspace
monorepo completes without hanging" to amplify workload, and make the apps'
dependency spread denser (change the step in the for loop that adds every-5th
package to every-1-or-2nd package or add additional randomized deps) so
connectivity is higher; optionally run the test multiple iterations or add a
simple timeout/assertion to fail fast on regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e639e040-03e4-4960-9918-28dafab8016a
📥 Commits
Reviewing files that changed from the base of the PR and between 2b6073e9e24bfa2ee1333dfe2d89cfeff0e20853 and 164149473f2bfe17d8fd9f24740f8bd62bca665d.
📒 Files selected for processing (1)
test/regression/issue/28422.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28422.test.ts`:
- Around line 65-79: Add an assertion for stdout and move the exit-code check up
so the test asserts stdout and exitCode immediately after capturing process
output; specifically, call expect(stdout) (e.g., toContain or toBe the expected
output) before calling expect(exitCode).toBe(0), and place these two assertions
right after the Promise.all that produces stdout, stderr, exitCode (before the
filesystem checks that verify APP_COUNT and PKG_COUNT symlinks). This ensures
stdout is checked first and the exit code is validated early for clearer failure
diagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d045201b-7a87-464f-80f1-d50361ee39e2
📥 Commits
Reviewing files that changed from the base of the PR and between 164149473f2bfe17d8fd9f24740f8bd62bca665d and da89abe7de21d85fc23bbe9e7450848098574a76.
📒 Files selected for processing (1)
test/regression/issue/28422.test.ts
There was a problem hiding this comment.
LGTM — all prior feedback addressed, logic is correct.
Extended reasoning...
Overview
Two files changed: src/install/isolated_install/Installer.zig (adds blocked_entries hashmap, updates deinit, onTaskBlocked, and resumeUnblockedTasks) and test/regression/issue/28422.test.ts (new regression test with 20 packages + 5 apps).
Security risks
None. This is a pure algorithmic optimization within the isolated package installer with no security-sensitive code paths touched.
Level of scrutiny
Moderate. The change touches a concurrency-sensitive area (task scheduling in the installer), but the fix is carefully scoped: blocked_entries is only mutated from the main thread (matching the existing thread safety model), the two-phase collect-then-remove pattern correctly avoids hashmap mutation during iteration, and bun.handleOom is used for all allocations. The invariant that an entry is in blocked_entries iff its step is .blocked is maintained by the single add path (onTaskBlocked) and single remove path (resumeUnblockedTasks).
Other factors
All prior inline comments (unused imports, exitCode assertion ordering, workspace graph scaling, stale comments) were addressed across subsequent commits. No bugs were found by the automated bug hunting system. The test uses bunExe/bunEnv from harness and follows repo conventions.
There was a problem hiding this comment.
LGTM — all prior feedback addressed, the optimization is correct.
Extended reasoning...
Overview
This PR adds a blocked_entries hashmap to Installer to avoid an O(N²) scan in resumeUnblockedTasks. Two files changed: Installer.zig (core logic) and isolated_install.zig (call site), plus a regression test.
Security risks
None. This is a pure performance optimization in the package installer with no security-sensitive code paths.
Level of scrutiny
Moderate — touches the isolated linker task scheduler which is production code. The two-phase collect-then-remove pattern to avoid mutating blocked_entries during iteration is the correct approach. Thread safety is preserved: blocked_entries is only ever modified from the main thread.
Other factors
All issues raised in prior review rounds have been resolved: the missing defer installer.deinit() (preventing an ASAN-detectable memory leak) was added, the stale comment referencing the old O(N) code path was updated, and the test was scaled up with PKG_COUNT/APP_COUNT constants. The only open nit is a comment inaccuracy about pkg-1 having one vs. two dependencies — this has no functional impact and the test passes correctly.
There was a problem hiding this comment.
LGTM — all prior feedback addressed, fix is correct.
Extended reasoning...
Overview
Three files changed: src/install/isolated_install/Installer.zig (core fix), src/install/isolated_install.zig (lifecycle callsite), and a new regression test. The fix replaces an O(N) full-store scan in resumeUnblockedTasks with an AutoArrayHashMapUnmanaged that tracks only currently-blocked entries, reducing overall install complexity from O(N²) to O(B×D).
Security risks
None. This is purely an internal data-structure optimization in the package installer. No user input is parsed differently, no permissions change, no auth paths touched.
Level of scrutiny
Moderate — this is install-critical code, but the change is mechanically straightforward: replace a loop over all entries with a loop over a maintained set, plus two-phase collect-then-remove to avoid mutation during iteration. The invariants (main-thread-only mutation of blocked_entries, correct initialization to .empty, freeing in deinit) all check out in the diff.
Other factors
My previous review flagged a 🔴 memory leak (missing defer installer.deinit()) and a 🟡 stale comment; both were fixed in commit 9fbec40. Test scale was raised by CodeRabbit and robobun scaled to 20 packages + 5 apps with chained/cross-workspace deps. The regression test follows repo conventions (bunExe/bunEnv, tempDir, exitCode last, no unused imports). No bugs found by the automated bug hunter.
There was a problem hiding this comment.
The defer installer.deinit() that addressed the blocked_entries leak was added in 9fbec40 but then reverted in 1147848 — so the new allocation introduced by this PR has no corresponding free. The PR description still says it "is now properly called via defer" which is no longer accurate. My previous inline comment (line 31 of Installer.zig) was marked resolved but the underlying issue was reopened by the revert.
Extended reasoning...
Overview
This PR fixes a real O(N²) bug in resumeUnblockedTasks by introducing a blocked_entries: std.AutoArrayHashMapUnmanaged(Store.Entry.Id, void) hashmap on Installer. The algorithmic fix is correct: onTaskBlocked adds entries to the map, resumeUnblockedTasks iterates only the blocked set via a two-phase collect-then-remove pattern, and the stale comment was updated. The regression test (10 packages, 3 apps with cross-workspace deps) validates correctness.
Memory Management Issue
The blocked_entries field allocates from installer.lockfile.allocator via bun.handleOom(this.blocked_entries.put(...)) in onTaskBlocked. Installer.deinit() was updated to free it. However:
- Commit 9fbec40 added
defer installer.deinit()inisolated_install.zigto close the lifecycle gap. - Commit 1147848 (the current HEAD) reverted that defer with message "revert defer installer.deinit() to match pre-existing behavior".
isolated_install.zignow has no call toinstaller.deinit(), whilehoisted_install.zig:197correctly hasdefer installer.deinit().- The PR description still claims "freed in deinit (which is now properly called via defer)" — no longer true.
This introduces a new leak that did not exist before this PR (since blocked_entries is new and is populated in every monorepo install with cross-workspace deps — exactly the target use case). The pre-existing trusted_dependencies_from_update_requests already leaked; this PR adds a second leaked field in the hot path.
Security Risks
None. This is install-time memory management in a short-lived process.
Level of Scrutiny
The algorithmic core is solid and straightforward. The memory lifecycle decision (intentional revert of the deinit call) is the only open question. Given that hoisted_install.zig calls deinit() consistently, the asymmetry should be explained or resolved. The PR description discrepancy (claims the defer is present when it is not) also warrants human verification.
Other Factors
All other inline comments from my previous review were genuinely resolved: stale comments updated, test scale improved, dependency graph fixed, import ordering corrected. The outstanding issue is solely the reverted defer installer.deinit() and the stale PR description.
There was a problem hiding this comment.
LGTM — all prior review feedback has been addressed and the optimization is correct.
Extended reasoning...
Overview
This PR adds a blocked_entries: std.AutoArrayHashMapUnmanaged(Store.Entry.Id, void) field to the isolated Installer struct, and rewrites resumeUnblockedTasks to iterate only over that set instead of all store entries. The three changed files are src/install/isolated_install/Installer.zig (core logic), src/install/isolated_install.zig (lifecycle/cleanup callsite), and test/regression/issue/28422.test.ts (regression test).
Security risks
None. This is a pure performance optimization within the package installer with no user-facing input parsing, auth, crypto, or permission changes.
Level of scrutiny
Moderate. The change touches concurrent install task scheduling with atomic memory ordering. However, the two-phase collect-then-remove pattern is correct: blocked_entries is only ever mutated from the main thread (onTaskBlocked adds, resumeUnblockedTasks removes), so there is no concurrent modification hazard. The .monotonic/.acquire/.release ordering on entry_steps is unchanged from pre-existing patterns.
Other factors
All three rounds of prior inline review feedback were addressed: (1) the missing defer installer.blocked_entries.deinit(lockfile.allocator) was added; (2) the stale comment referencing the removed .monotonic load was updated; (3) the pkg-1 dependency collision in the test was fixed with if (i > 0) / if (i > 1) guards; (4) the double-free risk (blocked_entries freed in both deinit() and the targeted defer) was resolved by removing the field cleanup from deinit() and restoring the *const signature. The one CI failure (v8-heap-snapshot.test.ts SIGKILL) is pre-existing and unrelated. No bugs were found by the automated bug hunting system.
2b81af4 to
01ffc95
Compare
There was a problem hiding this comment.
LGTM — all prior review comments addressed, memory management is correct, and the optimization is sound.
Extended reasoning...
Overview
This PR touches three files: Installer.zig (core logic), isolated_install.zig (call site), and 28422.test.ts (regression test). The change adds a blocked_entries hashmap to track only currently-blocked entries, replacing the O(N) full-scan in resumeUnblockedTasks with an O(B) iteration over the blocked set.
Security risks
None. This is a pure performance/algorithmic fix in the package installer with no security-sensitive code paths.
Level of scrutiny
Medium. The change is in the install critical path and involves memory management and a concurrency invariant (main-thread-only modification of blocked_entries). The logic is straightforward: two-phase collect-then-remove is correct, ownership of blocked_entries is clear (targeted defer in isolated_install.zig only, not deinit()), and the concurrency invariant is documented and structurally enforced.
Other factors
All 15 inline review comments from prior runs were addressed across 6 commits. Final state: deinit() is *const and does not touch blocked_entries; cleanup is solely via defer installer.blocked_entries.deinit(lockfile.allocator) at the call site, eliminating any double-free risk. The regression test uses bunExe/bunEnv from harness, creates a realistic workspace graph, and correctly asserts exit code last per CLAUDE.md. CI failures in bundler_compile.test.ts are pre-existing timeouts unrelated to this PR.
There was a problem hiding this comment.
LGTM — all previously raised concerns have been addressed and the optimization is correct.
Extended reasoning...
Overview
This PR adds a blocked_entries: std.AutoArrayHashMapUnmanaged(Store.Entry.Id, void) field to Installer in src/install/isolated_install/Installer.zig, populates it in onTaskBlocked, and iterates only over it in resumeUnblockedTasks instead of scanning all store entries. A targeted defer installer.blocked_entries.deinit(lockfile.allocator) at the callsite handles cleanup. A regression test with 10 packages + 3 apps exercises the code path.
Security risks
None. This touches the package install/link scheduling logic for the isolated linker — no auth, crypto, or permissions-sensitive code paths.
Level of scrutiny
Moderate: this is performance-critical install code but not security-sensitive. The change is focused and well-bounded — three files, a clear algorithmic improvement, and a regression test. It has been reviewed across many iterations.
Other factors
All inline concerns from prior rounds have been resolved: memory ownership is clean (blocked_entries freed via targeted defer only, not in deinit()), the stale comment was updated, the test was scaled and has correct assertion ordering, and unused imports were removed. The pre-existing trusted_dependencies_from_update_requests leak is unchanged from before this PR — the commit history documents the deliberate choice to preserve that behavior. CI has been verified by robobun across multiple builds.
There was a problem hiding this comment.
LGTM — all prior review concerns resolved.
Extended reasoning...
Overview
Three files changed: Installer.zig adds a blocked_entries hashmap (populated in onTaskBlocked, iterated+removed in resumeUnblockedTasks via two-phase collect-then-remove), isolated_install.zig adds two targeted defers for memory cleanup, and test/regression/issue/28422.test.ts adds a workspace monorepo regression test.
Security risks
None — this is a pure performance optimization to the isolated package linker with no security-sensitive code paths.
Level of scrutiny
This PR went through extensive iterative review: the core O(N²)→O(B×D) algorithm change is correct, the two-phase collect-then-remove pattern is sound (avoids mutating blocked_entries during iteration), and memory management is correctly handled. The split ownership between deinit() (frees trusted_dependencies_from_update_requests) and the targeted defer (frees blocked_entries) is now documented with a comment. All issues raised across multiple review rounds — memory leak, stale comment, test fixture graph density, double-free risk, and the pre-existing trusted_dependencies leak — were resolved.
Other factors
Test creates 10 workspace packages with chained deps + 3 apps with cross-workspace connectivity, correctly asserts symlink creation and exit code 0 last. No unresolved comments remain.
There was a problem hiding this comment.
LGTM — all previously raised issues resolved.
Extended reasoning...
Overview
Three files changed: Installer.zig adds a blocked_entries: std.AutoArrayHashMapUnmanaged(Store.Entry.Id, void) field, records blocked entry IDs in onTaskBlocked, and replaces the O(N) full-store scan in resumeUnblockedTasks with an O(B) iteration over the blocked set using a two-phase collect-then-remove pattern. isolated_install.zig adds two defer statements for cleanup. A regression test creates a workspace monorepo (10 packages, 3 apps) and verifies symlinks + exit code 0.
Security risks
None. This is a pure performance optimization in the package installer. No auth, crypto, or permissions code is touched.
Level of scrutiny
Moderate — this touches install-critical code paths, but the change is well-scoped and the logic is straightforward. The two-phase collect-then-remove pattern correctly avoids hashmap mutation during iteration. The LIFO defer ordering in isolated_install.zig (line 835 runs before 834) ensures blocked_entries is freed before deinit() frees trusted_dependencies_from_update_requests, with no double-free risk. Thread safety is preserved: blocked_entries is only ever modified from the main thread.
Other factors
All issues I previously raised across multiple review rounds have been addressed: missing defer installer.deinit(), stale comment referencing the removed .load(.monotonic) call, double-free risk from blocked_entries.deinit in deinit(), and undocumented split ownership (now documented with a comment in deinit()). The remaining nit from the bug hunter — duplicate dep assignments in the apps loop of the test fixture for a=0 and a=2 — has zero functional impact: all symlink assertions still hold and the O(N²) path is still exercised. CI failure (webview timeout on macOS) is unrelated to this PR.
There was a problem hiding this comment.
LGTM — all review concerns addressed, algorithm and memory management are correct.
Extended reasoning...
Overview
This PR modifies three files: src/install/isolated_install/Installer.zig (adds blocked_entries hashmap, updates resumeUnblockedTasks), src/install/isolated_install.zig (adds two targeted defers for cleanup), and test/regression/issue/28422.test.ts (regression test with 10 workspace packages + 3 apps).
Algorithm Correctness
The O(N²) → O(B×D) change is correct. The old code scanned all store.entries on every task completion; the new code only iterates blocked_entries.keys(). The two-phase collect-then-remove pattern (to_unblock list + swapRemove after iteration) correctly avoids modifying the hashmap during iteration. Thread safety is preserved: blocked_entries is only modified from the main thread (onTaskBlocked adds, resumeUnblockedTasks removes).
Memory Management
The final cleanup scheme is correct and well-documented:
defer installer.deinit()at line 834 freestrusted_dependencies_from_update_requestsdefer installer.blocked_entries.deinit(lockfile.allocator)at line 835 freesblocked_entries- Zig LIFO defer semantics mean line 835 runs first, then line 834 — no double-free
deinit()intentionally excludesblocked_entrieswith an explanatory comment documenting the split ownership
Security Risks
None. This is an internal package installer optimization with no externally-visible security surface.
Level of Scrutiny
This is an install-path performance fix touching non-security Zig code. The algorithm change is well-scoped, the PR went through multiple review rounds (15+ iterations), and all identified issues (memory leaks, double-free risks, stale comments, test quality) were addressed. The regression test exercises the exact code path changed.
Other Factors
All 15 of my prior inline review comments were addressed and resolved. No CHANGES_REQUESTED reviews remain. CI build is in progress with no failures attributable to this change.
8799715 to
ff98f82
Compare
There was a problem hiding this comment.
LGTM — all previously raised issues have been addressed.
Extended reasoning...
Overview
This PR modifies src/install/isolated_install/Installer.zig and src/install/isolated_install.zig to replace an O(N) full-store scan in resumeUnblockedTasks with an O(B) blocked-set iteration, and adds a regression test in test/regression/issue/28422.test.ts.
Security risks
None. This is a pure performance optimization in the package install path with no auth, crypto, or permission changes.
Level of scrutiny
Moderate — touches install-critical code in the isolated linker, but the change is well-scoped: one new hashmap field, additions to two existing methods, and a new test. The logic is straightforward and has been carefully reviewed over multiple iterations.
Other factors
All previously raised issues were resolved across commits: memory leak for trusted_dependencies_from_update_requests fixed by adding defer installer.deinit(), double-free risk with blocked_entries resolved by removing it from deinit() and documenting split ownership, stale comments updated, and test fixture graph density issues corrected. CI failures in the latest build are webview timeouts on macOS — unrelated to isolated install. The two-phase collect-then-remove pattern correctly avoids hashmap mutation during iteration, and thread safety is preserved (main-thread-only map mutations).
…esumeUnblockedTasks resumeUnblockedTasks iterated through ALL store entries on every task completion to find blocked tasks. With large monorepos (many workspace packages × many dependencies), this O(N²) behavior made isolated installs 50x+ slower than hoisted. Add a blocked_entries hashmap to track only blocked entries. Now resumeUnblockedTasks iterates only over actually-blocked entries, reducing complexity from O(N × D) to O(B × D) where B is the number of blocked entries (typically much smaller than N). Also adds defer installer.deinit() to fix a pre-existing leak of trusted_dependencies_from_update_requests in isolated installs. Closes #28422
ff98f82 to
04ec29e
Compare
There was a problem hiding this comment.
LGTM — all issues from prior reviews have been addressed.
Extended reasoning...
Overview
Three files changed: src/install/isolated_install/Installer.zig (core algorithm), src/install/isolated_install.zig (call site cleanup), and a new regression test. The change replaces an O(N) full-store scan in resumeUnblockedTasks with an O(B) iteration over a blocked_entries hashmap, eliminating the O(N²) overall complexity for large monorepos.
Security risks
None. This is a pure performance optimization in the package installer with no security-sensitive code paths.
Level of scrutiny
This required careful scrutiny — it touches concurrent code (task threads + main thread) and memory ownership. Previous review rounds caught a missing defer installer.deinit(), a double-free latent trap in deinit(), a pre-existing trusted_dependencies_from_update_requests leak, a stale concurrency comment, and minor test quality issues. All were addressed across multiple commits.
Other factors
The final state is clean:
deinit()correctly only freestrusted_dependencies_from_update_requests, with a comment explaining thatblocked_entriesis freed via a targeted defer at the call site (preventing future double-free)- The two-phase collect-then-remove pattern in
resumeUnblockedTasksavoids mutating the hashmap during iteration - Thread safety is preserved:
blocked_entriesis only modified from the main thread - The regression test uses
PKG_COUNT/APP_COUNTconstants with a properly constructed dependency graph (chained + cross-workspace) and correct assertion ordering per CLAUDE.md
|
Pegs the CPU to ~100% and eats up excessive memory too (~6gb for a monorepo with 15 packages) |
|
Looks like this was resolved in 1.3.13 🎉 |
…ilter/--catalog, nested overrides, transitive update, and workspace fixes (#38333) Brings Bun's package manager to parity with pnpm for monorepo workflows, and fixes the bugs found while checking every command against pnpm's implementation, pnpm's test suites, pnpm's open issue tracker, pnpm's docs, npm's arborist fixtures, and — for `bun update` — running real pnpm and Bun side by side on the same projects. ### What does this PR do? #### New commands - **`bun dedupe [--check]`** — collapses duplicate versions in `bun.lock` onto the smallest set that still satisfies every dependent's range, using only versions already in the lockfile, then installs. Never downgrades a direct dependency unless that is the only way to drop a version; keeps patched versions (and anything needed to reach them, and says so); refuses to run on a lockfile that is behind `package.json`. `--check` exits 1 without writing. - **`bun prune [--production | --omit=…] [--dry-run] [--filter <ws>]`** — removes everything in `node_modules` that the lockfile does not put there; `--production` leaves exactly what `bun install --production` would. Hoisted and isolated layouts, Windows junctions and shims, workspace links, bundled deps; refuses when `package.json` and `bun.lock` disagree or when `node_modules` was laid out by a different linker; understands turbo-pruned checkouts. - **`bun pm licenses [--json] [--prod|--dev] [--long] [--filter <ws>]`** — installed packages grouped by license, with a `(dev)` marker and `paths`/`license`/`description` in `--json`. - **`bun audit fix [--latest] [--dry-run] [--json]`** — moves each vulnerable package to the lowest safe version its dependents accept, per installed instance; rewrites exact pins when that is the only way; `--latest` also rewrites your own declared ranges (root, workspace, catalog) so a semver-major fix can be taken, and every blocked or unfixable item is followed by the command that resolves it (`bun audit fix --latest`, `bun audit --ignore GHSA-…`); re-audits the tree it actually installed and reports/exits from that second response (npm's `_submitQuickAudit`), so an advisory that starts at the version it moved to is not missed; works across registries; security fixes bypass `minimumReleaseAge` with an annotation. `bun audit --json` honors `--audit-level`/`--ignore` for its exit code; `--omit` is honored by `audit` and `licenses`. #### `bun update` semantics (pnpm's model) - A bare `bun update` re-resolves **transitive** packages too — every edge moves to the newest version its own range (or dist-tag) allows, per dependent, so `bun.lock` no longer stays stale after an update; overrides/catalogs changed since the last install are honored. From a workspace member or `--filter`, only what the selected workspaces reach is re-resolved; from the root, everything. - `bun update <name>` reaches any depth, matches `npm:` aliases by real name, updates in place, never adds to `package.json`, and errors on a name nothing selected depends on. `-r`/`--filter` fan a named update out across workspaces. `--latest` never downgrades a locked version that is ahead of the tag, and `update <name> --latest` also refreshes that package's own dependencies. - Plain updates keep dist-tag literals and non-caret ranges (`*`, `1.x`, `^1 || ^2`) exactly as written and only move the lockfile; `--latest` rewrites them to the resolved version as before. `bun update -i` applies only what you selected. New: positional patterns (`bun update '@types/*'`), `--dev`/`--prod`/`--no-optional`, `-L`, `bun up`. - `package.json` is written after resolution and `bun.lock`'s declared ranges, overrides and catalogs are re-derived from the final `package.json`, replacing the per-command literal rewriting; a no-op update leaves the file byte-identical. #### Overrides - **Nested overrides** (#6608): npm's nested objects, yarn's `a/b` paths and pnpm's `a>b` selectors, applied to the direct parent→child edge; **version-scoped targets** (`"lodash@<4.17.21": "4.17.21"`, the shape `pnpm audit --fix` writes), matched against the dependent's declared range as pnpm does. Rules persist inside the `overrides` section and the file is stamped `lockfileVersion: 3` **only when such rules exist** — existing lockfiles are byte-identical. Flat overrides additionally fix `$ref` to workspace-member deps, catalog-valued rules going stale, and warn on pnpm's `-` / `pkg@` forms. #### Workspaces and filters - `bun add|remove|update … --filter <ws>` (also `-F`, also `bun install <pkg> --filter`) edits the selected workspaces' `package.json` files and **links only those workspaces**, like `bun install --filter`. Filters gain pnpm's relation selectors (`foo...`, `...foo`, `foo^...`, `...^foo`) and `{dir}` subtrees, for the install family **and** `bun run --filter`; `--filter` may precede the subcommand; every command warns about patterns that match nothing; `add`/`remove` no longer select the root implicitly. - `bun add <pkg> --catalog[=name]` reuses an existing catalog entry, keeps a range an explicit version fits, catalogs the range a package already declares, decides per target, and refuses workspace names and local paths; a plain `bun add` uses a default-catalog entry when one exists. A package defined in both `catalog` and `catalogs.default` is an error. `catalog:` peers of registry packages bind to the importer's copy instead of the root catalog. - `--frozen-lockfile` / `bun ci` on turbo-pruned monorepos: pruned-away workspaces are tolerated, a survivor depending on a pruned workspace is an error, catalog subsets are accepted, and an overrides/catalogs change is a frozen failure. #### One output vocabulary Every command here prints the install family's shapes: header, glyph rows (`+`/`-`/`↑`, dedupe's `↳ name old → new`), exactly one noun-first summary line with counts and a duration (`2 duplicate versions removed, 3 packages installed (checked 5 packages) [12ms]`, `N packages removed (checked C) [t]`), no-ops that say what was checked, remedies printed as copy-pasteable command lines, warnings as `warn:`, `--silent` printing nothing, and errors with their remedy together on stderr. Transitive and named updates render as the summary's `↑` rows (once per package; `--dry-run` prints the same rows plus `N packages would be updated`); dedupe reports after the install it triggers, so lifecycle-script output never splits it. A lockfile whose bytes did not change is no longer rewritten (`Saved lockfile` only prints on a real write; `--lockfile-only` no-ops print `Done! Checked N packages (no changes)`). This came out of running every command against fixtures and comparing with `install`/`add`/`remove` (95 findings, all fixed). #### Config precedence A project's `bunfig.toml` now beats any `.npmrc` (project or user-level) for the same key (npmrc files → bunfig's set fields → CLI); npmrc-only settings such as `//host/:_authToken` still attach to bunfig-declared registries, matched by host and path regardless of how either file spells the trailing slash. #### Lockfile migration - `package-lock.json`: rebuilt around a reachability walk that derives each resolution from the entry itself. Fixes `git+https://github.com/…` resolutions being written unparseably (the next install threw the lockfile away), root `bundleDependencies` migrating to an **empty** lockfile, lockfileVersion 1 (and npm's upcoming 4) making `bun install` exit 1 instead of resolving fresh, dependency-level bundles, `dependencies`+`optionalDependencies` double edges, unreferenced entries aborting the migration, duplicate packages for identical `name@version` at nested paths, lost `optionalPeers`, lost integrity when a bundled copy was seen first, and `overrides` not being carried over. All 57 of arborist's v2/v3 fixture projects are vendored and migrated under snapshot. - `pnpm-lock.yaml` v9: bare-hash `patchedDependencies`, snapshot aliases, `catalog:default`, recorded tarball URLs, git `path:`, multi-document files, `runtime:` entries, named registries, peer-suffixed keys chosen per importer, injected workspaces, manifest-only importer deps. #### Isolated linker - An existing store entry whose dependencies re-resolved (override, dedupe, update) now has its links refreshed on the next install (measured cost below). `bun prune` builds the same store the installer builds, so stale `name@version+<peerhash>` variants left by peer bumps are removed and a kept package's real entry never is (whether it was installed with full or `--production` features); on the hoisted linker, dedupe / audit fix / update delete the nested copies whose rows they collapsed instead of leaving the old copy loadable. - Blocked entries resume through per-entry intrusive waiter lists instead of a scan of every store entry after each completion (robobun's #25983/#28425 attempted this). Measured on the reporter's repro from #25799 (2,259 store entries) and a synthetic 6,425-entry monorepo, PR build vs merge-base build: main-thread CPU in the link phase drops 0.85 → 0.33 s and 5.5 → 0.85 s (the removed work grows quadratically); wall time is unchanged with spare cores and 16% / 26% faster pinned to one CPU, the CI/Docker shape in those reports. (The minute-long installs originally reported were peer resolution, fixed before this PR's base.) - Two pre-existing leaks surfaced by the new LSan-enabled tests are fixed: the header buffer of every authenticated registry request, and the per-entry lifecycle-script lists. #### Other bug fixes `bun add x@npm:pkg` writes a range; `bun add --trust a b` no longer drops `b` when `a` was already trusted; `bun add x --dev` no longer rewrote every group in `bun.lock`; `catalog:` peer hoisting; `dependency::Version::eql` treated all `catalog:` specifiers as equal; a `file:` package whose dependencies reach itself (its own name, an `npm:` alias under its own name, two link targets depending on each other — the shapes a `package-lock.json` migration produces, and #25202's `workspace:.` self-reference) hung `bun install` forever in the hoisting tree — the migration shapes now install, and #25202's literal shape now terminates with `Workspace dependency "foo" not found` rather than installing as npm does; a peer of a `file:` package that was only placed nested was also written to `optionalPeers` in a migrated bun.lock, so the next `--frozen-lockfile` failed and a plain install rewrote the lockfile; `catalog:` literals in `bun update`; alias output in the install summary; help/completions for everything above. #### Behavior changes to note in the release notes - `bun update` moves transitive packages; `bun update <name>` no longer adds an undeclared package (exit 1); `--production`/`--prod` on update means "only update `dependencies` and `optionalDependencies`" (a group filter like `--dev`, not the install flag) and `-r`+names with no match is an error; `-i` updates only the selection. - Project `bunfig.toml` overrides any `.npmrc`. - `bun install <pkg> --filter x` edits `x` (not the root); `bun add y --filter x` no longer installs a package named `x`; `add`/`remove --filter '*'` no longer includes the root. - A plain `bun add x` in a workspace whose default catalog lists `x` writes `catalog:`; `audit fix` may rewrite exact pins; `--frozen-lockfile --lockfile-only` writes nothing; overrides/catalog changes fail frozen installs. - One-time lockfile churn after upgrading for projects with `catalog:` peers or dead `pkg@range` override rows; lockfiles that use nested/scoped overrides are v3 and unreadable by older Bun (only when opted in). Turborepo, Nx and Dependabot have been checked; the needed upstream changes are open (nrwl/nx#36666 covers v2 and v3; vercel/turborepo#13740 accepts v3 and preserves the object rows through prune — turborepo main today parses v2 and rejects v3; dependabot needs nothing). Note v2 itself only exists on the 1.4 line. - `bun audit --json` keeps npm's contract: `--audit-level`/`--ignore` decide the exit code, the JSON document is the full registry report (closes #31013 as won't-change). Automatic removal of stale `node_modules` entries on plain `bun install` (#32974) is separate from this PR: `bun prune` is the manual form, and dedupe / audit fix / update now clean up the nested copies they collapse on the hoisted linker; #32974 should reuse prune's planner, and #29512 (sbom) is sequenced after this so it can build on `reachable.rs` instead of carrying its own walk. - Deliberately kept where we differ from pnpm: root `bun update` covers the whole workspace; dependents whose ranges allow follow a moved version (one copy, not two); `--latest` works on transitive names; `--no-save` touches neither file; prune deletion failures exit 1; audit requests stay per-registry. #### Performance Measured on a 1,113-package Next/Prisma/MUI app (PR build vs a PR build of the merge base, interleaved, plus canary and 1.3.14): every hoisted cell is within noise except no-op install, +0.8 ms (+2%, identical syscalls); isolated no-op is +1.9 ms (+3.9%) — the deliberate cost of re-checking existing entries' links every install rather than persisting a stamp file. Everything added is otherwise off the plain-install path (gated on the feature being used or on a diff), and id-indexed sets are bitsets. ### How did you verify your code works? ~1,100 new or ported test cases across the install suites (designed behavior, cases ported from pnpm's suites, pinning tests for pnpm bugs this implementation is immune to, arborist's fixtures, and the CI review findings), all `toStrictEqual`; the whole `test/cli/install` directory passes locally and the existing suites are unchanged except where a pre-existing expectation was deliberately changed (each listed above). `bun update` was additionally verified with a rerunnable differential harness that runs pnpm 11 and this branch on 26 scenario families against one registry and diffs the resulting resolutions edge by edge — after this PR only the deliberate differences above remain. Ecosystem: Turborepo, Nx and Dependabot were checked against the new lockfile output. Co-authored work absorbed with credit: @kjanat's #38190 (alias handling, co-author on the commit), @charpeni's #31143 and @crystalin's #34407 (both superseded), and the tests of the earlier `bun update` PRs (#31752 by @zlotnika, #33127, #36381, #36729, #38224). robobun's #34688 (folder-dependency cycles; its tests are lifted, co-author on the commit) and #37289 (migrated optionalPeers; its test is lifted, co-author on the commit) were fixed independently here and are closed by this PR. #28422's quadratic scan is fixed here as well (already closed). Related but not closed — `bun prune` gives these a manual fix while the automatic-cleanup asks stay open: #8662, #26305, #29793, #21216, #16176. Also related: #10930, #26970, #26751. Fixes #1343 Fixes #3605 Fixes #14719 Fixes #24122 Fixes #18612 Fixes #20238 Fixes #25826 Fixes #23615 Fixes #26973 Fixes #20593 Closes #31013 Fixes #28959 Fixes #28402 Fixes #27897 Fixes #26675 Fixes #10949 Fixes #18504 Fixes #13388 Fixes #24523 Fixes #6608 Fixes #19059 Fixes #16569 Fixes #8262 Fixes #11901 Fixes #13469 Fixes #25202 Closes #29664 Closes #31143 Closes #34407 Closes #34688 Closes #37289 Closes #38190 --------- Co-authored-by: Kaj Kowalski <info@kajkowalski.nl> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
2 participants
Problem
When using
linker = "isolated"in a monorepo with many workspace packages and dependencies,bun installandbun updatebecome extremely slow — 50x+ slower than hoisted installs (issue #28422).Root Cause
resumeUnblockedTasksinsrc/install/isolated_install/Installer.zigiterates through all store entries on every task completion to find blocked tasks:With N packages, this runs O(N) per task completion, and since there are N tasks total, the overall complexity is O(N²). For large monorepos with hundreds or thousands of packages, this quadratic scaling dominates install time.
Fix
Add a
blocked_entrieshashmap that tracks only the entries currently in a blocked state.resumeUnblockedTasksnow iterates only over this set instead of all entries, reducing complexity from O(N × D) to O(B × D) where B is the number of currently blocked entries (typically much smaller than N) and D is the average number of dependencies per entry.blocked_entriesinonTaskBlockedblocked_entrieswhen they become unblocked inresumeUnblockedTasksto_unblocklist collects entries to unblock since we cannot modify the hashmap during iterationblocked_entriesis freed via a targeteddeferat the call siteVerification
bun bd test test/regression/issue/28422.test.ts— passesCloses #28422
Verified by robobun (iteration 17): Squashed commit ff98f82 reviewed. Three files, 105 additions/15 deletions. Algorithm change replaces O(N) full-store scan in resumeUnblockedTasks with O(B) iteration over blocked_entries hashmap — two-phase collect-then-remove avoids mutation during iteration. Memory: blocked_entries freed at line 835 via separate defer, trusted_dependencies freed via installer.deinit() at line 834 — independent fields, no double-free. Thread safety: blocked_entries only modified from main thread (onTaskBlocked adds, resumeUnblockedTasks removes). onTaskFail sets step to .done then calls resumeUnblockedTasks, correctly unblocking dependents. Diff clean (no TODO/FIXME/HACK/XXX). No CHANGES_REQUESTED reviews. Test creates 10 workspace packages with chained deps + 3 apps with cross-workspace deps, runs isolated install, asserts symlinks + exit code 0. Buildkite #41342 pending.