install: only re-resolve rows a package still owns when overrides or catalogs change - #38849
install: only re-resolve rows a package still owns when overrides or catalogs change#38849robobun wants to merge 6 commits into
Conversation
…catalogs change When package.json's overrides or catalogs differ from the lockfile, the differ replaces the root's dependency rows and then re-resolves every row in buffers.dependencies that the change could affect. That walk also hit the root's previous rows, which no package owns any more. The resolver treats an unowned row as a transitive dependency, so a root file: path outside the project failed with MissingPackageJSON once its override or catalog entry changed, and every other stale row was resolved again for nothing. Both passes now go through one helper that skips rows outside every package's dependency list.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe installer now uses one traversal for catalog and override dependency invalidation. New tests cover catalog file changes, peer warnings, override removal, file dependencies, lockfiles, and frozen installs. ChangesDependency re-resolution
Possibly related PRs
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change is localized to dependency re-resolution behavior and includes targeted regression coverage; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: fix is up in this PR and ready for review. CI (build 97304): 178 of 179 jobs passed and the new tests passed on every lane, macOS included. The one failed job is a macOS 14 aarch64 shard where Reproduced on bun 1.4.0 three ways, all from the same cause: a root Out of scope here, tracked in the comments below: owned rows these passes still visit twice (unmapped root rows, workspaces about to be re-read, and rows both passes select), which only repeat a peer warning. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. The refactor is clean and the ownership-gated re-resolve matches the existing UpdateScope::walkable_rows pattern, but since it narrows what gets re-resolved on the core install path, a human look at the lockfile/differ invariants would still be worthwhile.
Checked: the catalogs-loop predicate is a correct De Morgan of the old continue guard (empty catalog_overridden still short-circuits via binary_search().is_ok() == false); owned_rows.bit_length() equals the snapshotted buffers.dependencies.len() so the iteration bound and buffer-growth safety are preserved; kept_pruned workspace rows land inside package 0's new (off, len) slice so they remain owned and visited. Tests are hermetic (tempDir, no registry), assert installed versions + lockfile snapshots + a frozen-lockfile pass, and cover both the overrides-removed and catalog-changed variants.
Extended reasoning...
Overview
This PR fixes a bug where bun install fails with "Could not find package.json for file:../x" after removing an override (or changing a catalog entry) for a root file: dependency that points outside the project. The fix extracts the two nearly-identical re-resolve loops in install_with_manager.rs (overrides-changed and catalogs-changed) into a shared reresolve_owned_rows helper that additionally skips rows no package's dependency slice covers — the root's stale rows the differ just replaced. Three regression tests are added across nested-overrides.test.ts and catalogs.test.ts.
Security risks
None. This is package-resolution bookkeeping; no user-facing input parsing, auth, or filesystem-trust boundaries change. The file: trust rule itself (is_workspace_dependency / Folder arm) is untouched — the fix only stops feeding it dead rows.
Level of scrutiny
High. This runs on every bun install where overrides or catalogs changed relative to the lockfile, and it narrows the set of rows that get invalidated and re-enqueued. A missed row here would leave a stale resolution in place. The PR's argument that unowned rows contribute nothing (because Lockfile::clean rebuilds from package lists and the tree builder walks package lists) is sound and matches the precedent of UpdateScope::walkable_rows, update_transitive::set_rows_of, and audit_fix::live_edge — but confirming there's no path where re-resolving a dead row had a load-bearing side effect (e.g. seeding a manifest cache the live row's resolution then reuses in a way that changes outcomes) is the kind of invariant a maintainer who owns this subsystem should sign off on.
Other factors
- The bitset construction is byte-for-byte the same shape as
walkable_rows(src/install/update_scope.rs:162), including theslice.len == 0skip andRange { begin, end }bounds, so no new edge cases in the ownership computation itself. - The catalogs predicate rewrite is a straight De Morgan; verified the empty-
catalog_overriddencase still evaluates to "onlyCatalog-tagged rows". - Iteration bound moved from
dependencies_lentoowned_rows.bit_length(), which is set frombuffers.dependencies.len()at construction — same snapshot semantics, same protection against the buffer growing duringenqueue_dependency_with_main. - The new
?propagatesDynamicBitSet::init_empty's alloc error up throughinstall_with_manager; consistent with surrounding OOM handling. - Tests follow harness conventions (
tempDir,normalizeBunSnapshot,test.concurrent, no network), assert the strongest observable (installed version + full lockfile snapshot +--frozen-lockfilere-read), and the PR states they fail on the released binary.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 10:05 AM PT - Aug 15th, 2026
❌ @robobun, your commit 2701ae8 has some failures in 🧪 To try this PR locally: bunx bun-pr 38849That installs a local version of the PR into your bun-38849 --bun |
|
I was working on a neighbouring symptom of the same dead rows: when a root Three more ways the same two passes visit a row twice survive this change. All of them still print the warning twice with c87bf3e, and the rows involved are owned, so the ownership bitset does not see them:
Branch with the variant I had ready, in case it is useful to fold in: main...farm/1fea27be/skip-replaced-rows-on-override-catalog-change (commit 2621eb9). It keeps one bitset across both passes (the one Not opening a separate PR since this one owns the fix; happy for any of the above to be lifted into it, or it can follow as a small follow-up once this lands. |
|
On the one open question from the review, whether re-resolving the dead rows had any side effect the live rows relied on: I went through the channels and the only one that exists is harmful, so it is now covered by a test.
The PR description now includes the npm case. The two flagged comments were shortened in d1e28ff; the src change is otherwise the same as the first push. |
|
Thanks, that matches what I see. Re-checked the two dead-row peer cases against this branch and lifted them into catalogs.test.ts as "a root peer whose catalog range stops matching the installed version is checked once" (bd02bc7: declared through the catalog, overridden to I am keeping this PR to the unowned rows, so points 1 to 3 stay open: those rows are owned at the time the passes run, and skipping them rests on a different argument (the add/update pass is about to handle them), which needs the differ plumbing from your branch. That is a reasonable change on its own, but it is a separate one from "rows nobody owns resolve as transitive", and the failing install / wrong version here should not wait on it. The description now lists those three cases as out of scope and points at this thread. For the follow-up on top of this branch: the replaced root slice no longer needs seeding ( |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Since it changes resolution semantics in bun install (which rows get re-resolved when overrides/catalogs change), a human look would still be worthwhile before merging.
What was reviewed:
reresolve_owned_rowsmirrors the establishedUpdateScope::walkable_rowspattern; the bitset length correctly bounds the walk to rows that existed before resolution appends more.- The catalogs-changed predicate is semantically identical to the old inverted
continueguard (binary_searchon an empty vec is alwaysErr, so the droppedis_empty()short-circuit was only an optimization). - When both passes run, the second call rebuilds
owned_rowsand picks up rows appended by the first — matching the old per-loopdependencies_lensnapshot for owned rows.
Extended reasoning...
Overview
This PR extracts two near-identical re-resolution loops in install_with_manager.rs (one for overrides_changed, one for catalogs_changed) into a shared reresolve_owned_rows helper. The helper adds one filter: it builds a bitset of dependency rows currently covered by some package's (off, len) slice and skips rows no package owns. Those unowned rows are the root's stale entries from the loaded lockfile, orphaned when the differ appended fresh root rows and repointed package 0's slice. Re-resolving the orphans caused three user-visible bugs (a hard file:../x install failure, a wrong npm version via get_package_id dedupe, and a duplicate peer warning), each now covered by a test in nested-overrides.test.ts or catalogs.test.ts.
Security risks
None. No auth, crypto, network parsing, or path-traversal surface is touched; the change only narrows which already-parsed lockfile rows are re-enqueued for resolution.
Level of scrutiny
Medium-high. The diff itself is small (~50 net src lines) and copies the exact bitset-construction idiom from UpdateScope::walkable_rows, so pattern risk is low. But it sits on the bun install resolution path and is a semantic narrowing rather than a mechanical refactor: correctness rests on the argument that unowned rows contribute nothing useful to resolution. The PR description and follow-up comments walk that argument through every channel (get_package_id dedupe, locked-version helpers, manifest fetches, Lockfile::clean), and the one channel that did leak (dedupe) is the bug being fixed and now has a dedicated test. That analysis reads sound to me, but it is exactly the kind of invariant reasoning a maintainer familiar with the resolver should confirm.
Other factors
- The two comment-cop bot flags were addressed in d1e28ff (comments shortened) and both threads are resolved.
- A second robobun thread proposed folding in three additional owned-row double-visit cases; the author scoped those out with a clear rationale (they need differ plumbing and only affect a warning count), and the description records them as follow-up. That scoping decision seems reasonable but is worth a maintainer's nod.
- Tests follow harness conventions (
tempDir,test.concurrent, VerdaccioRegistry, frozen-lockfile round-trips) and include a baseline case ("inline") that already passed on the released binary.
…one pass Walking the flat buffer without the root's dead rows moved the root's live rows (appended last by the differ) behind every transitive row, so with an unchanged root range they deduped onto whatever same-major version a transitive row had just appended. Walking each package's current list in package order resolves the root first again, as the dead rows used to and as a fresh install does, and leaves the dead rows out without a bitset. The overrides and catalogs selections are folded into one walk so a row matching both is re-resolved once.
| /// Re-resolves the rows `selects`, walking each package's current dependency list in package order. The root's | ||
| /// list (just rebuilt by the differ) goes first because a later row dedupes onto what an earlier one appended | ||
| /// (`Lockfile::get_package_id`); the root's loaded rows, now in no list, would resolve as nobody's and are not | ||
| /// walked, nor are `pinned_rows`, which the update plan just resolved. A workspace the add/update pass is about | ||
| /// to re-read still holds its loaded list here and is walked like any other package. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes resolution ordering in the package manager's overrides/catalogs invalidation pass (buffer order → package-list order, two loops merged into one), a human look would still be worthwhile.
What was reviewed:
reresolve_owned_rows:DependencySlice::begin()/end()returnu32sodep_idmatchesDependencyID; thelistssnapshot bounds the walk against rows/packages appended mid-resolve.- Merged predicate vs. the two old guards:
all_name_hashesis empty unless overrides changed andcatalog_overriddenis only filled whencatalogs_changed, so the union underif invalidates_rowsselects the same rows minus the unowned ones. - Package-order iteration puts the root's new slice first (package 0 was repointed at
(off, len)); the "root's row resolves before a dependency's row" test pins that. - Note: comment-cop still has an open flag on the
reresolve_owned_rowsdoc comment (line 1553).
Extended reasoning...
Overview
The PR replaces two flat 0..dependencies_len loops in install_with_manager.rs (one for overrides_changed, one for catalogs_changed) with a single reresolve_owned_rows helper that iterates packages.items_dependencies() and applies a merged selection predicate. The effect is that the root's previous dependency rows — orphaned when the differ appended fresh ones and repointed package 0 — are no longer re-resolved, and the root's current rows are visited before every other package's. Tests are added to nested-overrides.test.ts (npm range dedupe, file:../ outside the project, removed together with its rule) and catalogs.test.ts (catalog file:../ change, duplicate peer warning).
Security risks
None identified. This is internal resolution bookkeeping; no new inputs are parsed and the file: trust check itself is unchanged (the fix stops feeding it rows with no owner, which is a strictly narrower set of resolves).
Level of scrutiny
High. This is the package manager's core resolve loop, where iteration order is load-bearing (Lockfile::get_package_id dedupes onto packages appended earlier in the session) and row ownership determines whether a file: path escaping the project is accepted. The change is small in line count but alters both which rows are visited and in what order. The PR description argues carefully that dead-row resolution had no beneficial side effects (only the harmful dedupe), and that argument checks out against the code I read, but it is exactly the kind of invariant a maintainer familiar with this subsystem should confirm.
Other factors
- CI is green on the earlier build (178/179, unrelated grpc flake); the latest commit's build (#97917) was still running at review time.
- The candidate finding that "the root's row resolves before a dependency's row" might pass on `USE_SYSTEM_BUN=1" was examined and refuted by verifiers.
- There is an unresolved comment-cop flag on the current HEAD (line 1553) about the doc comment length on
reresolve_owned_rows. The author previously shortened comments in d1e28ff in response to two earlier flags, but the bot fired again after 2701ae8. This is stylistic, not a correctness concern, but it is technically an open inline comment. - The PR thread includes a detailed follow-up plan for owned rows that are still visited twice (unmapped root rows, workspaces about to be re-read, rows selected by both criteria in the old two-loop shape); the author has scoped this PR to unowned rows only, which is a reasonable boundary for a maintainer to sign off on.
2 participants
Problem
"x": "file:../x"installs fine with an override forx, but once that override is removed (or, for"x": "catalog:", once the catalog changes) every followingbun installfails until the lockfile is deleted:"no-deps": "~1.0.0"plus an override, removing the override while changing the range to^1.0.0installs no-deps 1.0.1 (the best match of the range that was just removed) instead of 1.1.0.install_with_managerappends the freshly parsed root rows tobuffers.dependenciesand points package 0 at them (src/install/PackageManager/install_with_manager.rs:353), then re-resolves every row inbuffers.dependencieswhose name is overridden, or that is acatalog:row (the two loops at:531and:555before this change). The root's previous rows are still in the buffer, owned by no package, so they get re-resolved too, ahead of the live rows.file:case: for an unowned rowLockfile::is_workspace_dependencyis false, so the Folder arm ofget_or_put_resolved_package(src/install/PackageManager/PackageManagerEnqueue.rs:2656) treats it as a transitive folder dependency and rejects a path that leaves the package directory unless the name is still overridden. The live row resolves fine; the error comes from the dead one.~1.0.0row appends no-deps@1.0.1, and the live^1.0.0row is then deduped onto that package by the satisfies fallback inLockfile::get_package_id(src/install/lockfile.rs:2034), which accepts a same-major package appended earlier in the session.peerDependenciesentry declared through the catalog (or overridden tocatalog:) is bound twice when the catalog entry stops matching the installed version, sowarn: incorrect peer dependencyis printed twice.Lockfile::cleandrops whatever that produced.Fix
reresolve_owned_rows, which builds a bitset of the rows covered by some package's dependency list and only invalidates and re-resolves those (still skipping the rows a barebun updatepinned). The selection predicates are unchanged.file:/workspace:target from the owning package,locked_version_of_invoking_workspace_rowonly honors rows in the root's current list, and once resolution is done nothing reads an unowned row (Lockfile::cleanand the tree builder both walk package lists). A row no package owns therefore has no owner to be resolved under and nothing to contribute; its live replacement is in the buffer and is visited by the same pass.get_package_iddedupe described above, which is the 1.0.1-instead-of-1.1.0 bug: it let a range that is no longer in package.json pick the version. The locked-version helpers (locked_version_in_lockfileonly looks at lockfile-loaded packages) and manifest fetches (the same manifest is fetched for the live row) are unaffected by whether a dead row was resolved.UpdateScope::walkable_rows,update_transitive::set_rows_of,audit_fix::live_edge); these two loops predate it.catalog:row whose name is also overridden when both changed). Those only repeat a peer warning and are a separate change on top of this one (see the comments below).test/cli/install/nested-overrides.test.ts, "removing a flat rule re-resolves only the root's current rows": the npm range case above, afile:dependency outside the project whose override is removed, and one removed together with its override.test/cli/install/catalogs.test.ts: "changing the entry of a catalog: dependency pointing outside the project", and "a root peer whose catalog range stops matching the installed version is checked once" (declared through the catalog, overridden tocatalog:, plus the inline declaration as the baseline that already warned once).file:ones with the error above, the npm one with 1.0.1, the peer ones with a wrong warning count) and pass with this change; the nested-overrides ones 3 of 3 runs each way.bun bd teston nested-overrides, catalogs, bun-update-transitive, frozen-lockfile-pruned, bun-lock and bun-install: no new failures (the bun-install failures in this sandbox are the tests that need network access).Background
buffers.dependencies/buffers.resolutionsare flat, parallel arrays of dependency rows; each package owns a contiguous(off, len)slice of them. A row is "owned" when some package's slice covers its index.Diff::generate) compares the root package.json against the lockfile's root. When anything relevant changed, the root gets a new slice appended at the end of the buffers rather than having its old rows rewritten, so the old rows stay in the buffer, unowned, untilLockfile::cleanrebuilds the lockfile after resolution.file:) trust: afile:target on a root or workspace row is user-written and may point anywhere; the same target declared by a transitive package is rejected when it escapes that package's directory, except for names listed in the root's overrides, whose values are also user-written. Ownership is how the resolver tells the two apart.Lockfile::get_package_iddedupe: when an npm row resolves, a package of that name already in the lockfile that satisfies the row's range is reused instead of appending the manifest's best match; for packages appended during the current install this is only refused across majors. Resolution order therefore decides which of two overlapping ranges wins, which is why a dead row resolved first could pick for the live one.pinned_rowsis the set of rows a barebun updatealready re-resolved to a chosen version just before these loops run; it was already excluded and still is.