install: fix spurious --frozen-lockfile rejection when npm aliases duplicate a package name in one tree node - #36578
Conversation
…frozen-lockfile comparison Lockfile::eql sorts hoisted placements by (tree path, package name) before pairing the two lockfiles, but npm: aliases can place several packages with the same real name in one tree node. Those entries tie, sort_unstable pairs them arbitrarily between the loaded and re-hoisted lockfiles, and bun install --frozen-lockfile rejects a lockfile that bun install just wrote. Tie-break on the package resolution so the sort is a total order. Fixes #36577
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Those two are plausibly related (same error, same comparison path) but I could not verify them against this fix: both predate the regression window this PR's repro bisects to, neither mentions npm: aliases (the trigger here), and neither has a runnable reproduction. The unstable tie-pairing this PR fixes is one way Lockfile::eql can report a diff that does not exist, but workspace graphs have other known triggers (see #36303), so I am not adding auto-close lines for them. |
|
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 (1)
WalkthroughLockfile equality sorting now uses package resolutions as a final tie-breaker. Repository ordering compares ChangesLockfile equality and regression coverage
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 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/install/lockfile.rs`:
- Around line 2699-2703: Update the resolution comparison in Repository::order
to compare the resolved Git reference after committish, using the same
empty-value fallback behavior as Repository::eql. Preserve the existing
comparison order and only apply this tie-breaker when earlier fields compare
equal.
In `@test/regression/issue/36577.test.ts`:
- Around line 1-15: Trim the header comment in the regression test by removing
the bug-history and Lockfile::eql root-cause prose, while retaining the issue
URL and the graph-shape rationale explaining the fixture’s detectable signal.
- Around line 70-76: Update makeTarball to use a test-specific tempDir and a
unique tarball filename per invocation, preventing concurrent test collisions.
Cache in-flight Promise<Uint8Array> results by package key so duplicate requests
share one write, and ensure temporary files are removed in a finally block while
retaining Bun.Archive.write with gzip compression.
🪄 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: 6405a831-44e5-4d96-bc0b-78c94b8faf82
📒 Files selected for processing (2)
src/install/lockfile.rstest/regression/issue/36577.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/install_types/resolver_hooks.rs`:
- Around line 1006-1015: Redesign repository identity before applying the
conditional resolved comparison in Repository::order: define a transitive
identity or separate unresolved-repository matching path, then make
Repository::order and EqlSorter use a total ordering consistent with that
identity. Preserve correct lockfile pairing and add coverage for mixed empty and
non-empty resolved values; do not retain the current branch that returns
Ordering::Equal for either empty value.
🪄 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: 25ec6aef-9873-42d8-8522-92cebb12ba2e
📒 Files selected for processing (2)
src/install_types/resolver_hooks.rstest/regression/issue/36577.test.ts
There was a problem hiding this comment.
LGTM — minimal, well-reasoned tie-break that makes the frozen-lockfile sort key total.
What was reviewed
EqlSorter::order: new.then_withonResolution::orderis a strict refinement; the removed "name can't be the same" comment was the incorrect assumption being fixed.Repository::order: unconditionalresolvedcompare is a proper lexicographic extension (transitive); other callers viaResolution::order(Alphabetizer, package_index ordering) only see previously-tied git entries become deterministic.- Test: per-test
tempDirfor tarballs + promise-cachedmakeTarballresolves the earlier concurrency concern; hermetic local registry,port: 0, pipes drained concurrently.
Extended reasoning...
Overview
Three files: src/install/lockfile.rs adds a pkg_resolutions field to EqlSorter and a third .then_with() tie-break on Resolution::order, plus hoists eight items_*() bindings above the sort blocks so the sorter can borrow pkg_resolutions (pure reordering — the bindings are still consumed by the same pairwise loop below). src/install_types/resolver_hooks.rs extends Repository::order from (owner, repo, committish) to (owner, repo, committish, resolved). test/regression/issue/36577.test.ts is a new hermetic regression test that reconstructs the failing graph shape against an in-process registry.
Security risks
None. This is a comparator refinement in the frozen-lockfile equality check and a sort-key extension for git resolutions. No parsing of untrusted input, no new I/O, no auth/crypto surface.
Level of scrutiny
Package-manager lockfile comparison is a correctness-critical path, but the change is mechanically simple: extending a sort key with an additional .then_with() cannot reorder pairs that already compared unequal, and Resolution::order (pub(crate) in the same crate) already exists and is used by Alphabetizer for exactly this purpose. The Repository::order extension is a plain lexicographic append — after 0d3045c dropped the is_empty() guard it is unconditionally transitive, resolving the concern raised in earlier review rounds. I checked the other Resolution::order consumers (Alphabetizer in Package.rs/Yarn.rs, and the package_index ordering documented in bun.lock.rs): they only observe changed output for git/github entries that previously tied on (owner, repo, committish), which now become deterministic rather than sort-stability-dependent — a strict improvement.
Other factors
All prior review threads are resolved. My two earlier findings (test tarball race in os.tmpdir(); non-transitive Repository::order from the either-empty short-circuit) were both fixed (3b61040 and 0d3045c respectively) and the current diff reflects those fixes. The regression test follows harness conventions (tempDir, bunEnv/bunExe, port: 0, concurrent pipe drain, stderr asserted before exit code, test.concurrent with isolated per-case temp dirs). The PR description demonstrates a clear root-cause analysis (pdqsort partitioning of tied keys) and cites passing runs of the adjacent lockfile/hoist/alias suites. The bug-hunting system found nothing on this revision.
|
This test races on a shared cache directory on Windows CI; fix in #36613. |
…#36613) Fixes the Windows CI flake in `test/regression/issue/36577.test.ts` introduced by #36578, and brings the test under the default timeout on debug builds. ### Repro With the CI runner's env on Windows: ```powershell $env:BUN_INSTALL_CACHE_DIR=$tmp; $env:BUN_TMPDIR=$tmp; $env:TEMP=$tmp bun test test/regression/issue/36577.test.ts ``` fails 12/15 runs with either `expect(r.code).toBe(0)` receiving 1, or stderr containing ``` error: failed to verify cache dir for "f012": ENOENT ``` ### Cause The two `test.concurrent` cases install overlapping package names (`f000`-`f023`, `lib`, `carrier`, `pdep`, `zz-late`). The test set `cache = "cache"` in bunfig to give each case an isolated cache, but `scripts/runner.node.mjs` sets `BUN_INSTALL_CACHE_DIR` on the test process, `bunEnv` inherits it, and `fetch_cache_directory_path` consults that env var before the bunfig setting, so both concurrent `bun install` processes shared one cache directory. On Windows, the tarball-extraction retry path handles an occupied cache slot by renaming the existing entry into the temp dir and deleting it before retrying, so one process's post-rename verify can see `ENOENT` on an entry the other process just moved aside. That underlying race is #28062 (fix in progress in #33884). The POSIX path uses `renameat_concurrently_a`, which does not remove the existing entry, so the flake is Windows-only. Separately, under debug+ASAN each case took ~5s (two subprocess installs on 80-104 packages plus ~100 `Bun.Archive.write` calls for tarball synthesis in the registry server), right at the 5s default timeout. ### Fix - Override `BUN_INSTALL_CACHE_DIR` per test case in the spawn env so the intended isolation holds regardless of the ambient environment; drop the now-dead bunfig `cache` key and `mkdirSync`. - Run both installs with `--lockfile-only`. The `Lockfile::eql` comparison under test only needs a lockfile, so tarball download, extraction and linking are irrelevant. With no tarball fetch the registry server no longer needs to synthesize real tarballs, so the `Bun.Archive.write` path and the tarball route are removed (integrity is never verified on this path). ### Verification - Windows with CI-like env: 0/30 failures (was 12/15). - `bun bd test`: 5/5 pass, 0.8-1.8s per case (was timing out at 5s). - With the `src/install/lockfile.rs` change from #36578 reverted, both cases still fail with `lockfile had changes, but lockfile is frozen`, so the test continues to cover the original regression. <!-- robobun:evidence:begin --> --- **[stamp-90s]** gate passed · iteration 1 · 1 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/regression/issue/36577.test.ts' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "test/regression/issue/36577.test.ts" bun test v1.4.0 (6aa1437) test/regression/issue/36577.test.ts: (pass) frozen lockfile accepts a freshly generated lockfile (32 fillers) [2973.65ms] (pass) frozen lockfile accepts a freshly generated lockfile (24 fillers) [3158.88ms] 2 pass 0 fail 8 expect() calls Ran 2 tests across 1 file. [5.50s] Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` test/regression/issue/36577.test.ts | 65 +++++++++---------------------------- 1 file changed, 16 insertions(+), 49 deletions(-) ``` </details> **gate history** · 1 passed · 1 rejected · iteration 1 <details><summary>evidence per changed file</summary> ``` file reads edits tests test/regression/issue/36577.test.ts 6 9 0 ``` </details> **self-review** · no surviving concerns 33 concerns were raised and did not survive verification. <!-- robobun:evidence:end -->
2 participants
Fixes #36577
Repro
The non-frozen install is deterministic (repeated runs write byte-identical
bun.lock); only the frozen verification disagrees with the file it just produced.Cause
The frozen check compares the freshly loaded lockfile against the re-hoisted one with
Lockfile::eql, which collects every hoisted placement, sorts both sides by (tree path, package name), and compares pairwise.That sort key is not a total order:
npm:aliases can place several packages with the same real name in one tree node. In this graph,@asyncapi/multi-parserdepends on@asyncapi/parser,parserapiv1(npm:@asyncapi/parser@^2.1.0), andparserapiv2(npm:@asyncapi/parser@3.0.0-next-major-spec.8), so the root tree holds three placements named@asyncapi/parser(3.6.1, 2.1.2, 3.0.0-next-major-spec.8). The sort ties on them, andsort_unstable(pdqsort) pairs the tied entries by luck of partitioning.The two sides present the tied entries embedded in slightly different sequences: on the loaded side, satisfied optional-peer slots are already resolved when the tree is hoisted, while the post-clean rebuild re-derives them during the walk (since #35681), so a few placements (here
iconv-lite,safer-buffer,has-flag) land at different positions. With ~900 placements in the array, pdqsort then permutes the tied@asyncapi/parserentries differently on each side,eqlpairs 2.1.2 against 3.6.1, and reports a diff that does not exist. That is also why small graphs never hit this: below pdqsort's insertion-sort threshold the sort is effectively stable.Fix
Tie-break
EqlSorter::orderon the package resolution, making the sort key (tree path, package name, resolution) a total order over placements. Two different packages with the same name in the same node always differ in resolution, so pairing no longer depends on sort stability.Verification
test/regression/issue/36577.test.tsrebuilds the failing shape against a local registry: a package name placed at the root three times via twonpm:aliases plus a direct dependency, a satisfied optional peer whose subtree is enqueued at different times on the two sides, and enough filler packages to get past the insertion-sort threshold. Both cases fail on the unfixed build witherror: lockfile had changes, but lockfile is frozenand pass with the fix.bun install --frozen-lockfilereportsChecked 926 installs across 866 packages (no changes).test/cli/install/bun-lock.test.ts,hoist.test.ts,lockfile-version-2.test.ts, and the alias filter ofbun-install-registry.test.tspass with the change.