Skip to content
This repository was archived by the owner on May 14, 2026. It is now read-only.

feat: partial install with --frozen-lockfile (#433)#442

Merged
zkochan merged 4 commits into
mainfrom
feat/433
May 13, 2026
Merged

feat: partial install with --frozen-lockfile (#433)#442
zkochan merged 4 commits into
mainfrom
feat/433

Conversation

@zkochan

@zkochan zkochan commented May 13, 2026

Copy link
Copy Markdown
Member

Summary

Closes #433. Implements partial install with --frozen-lockfile:

  • Read <virtual_store_dir>/lock.yaml at install start (Lockfile::load_current_from_virtual_store_dir), mirroring upstream pnpm v11's readCurrentLockfile.
  • Skip any snapshot whose dependencies, optionalDependencies, and resolution.integrity match the current lockfile and whose virtual-store slot exists on disk. No fetch, no extract, no relink for skipped snapshots.
  • Write <virtual_store_dir>/lock.yaml at end-of-install (atomically; deletes the file when the lockfile is empty), so the next install sees a fresh diff target.
  • Emit pnpm:_broken_node_modules (BrokenModulesLog) when the slot is gone despite the cache key matching, then fall through to a full install for that snapshot.
  • pnpm:context.currentLockfileExists now reflects reality (was hard-coded false); pnpm:stats.added reports the post-skip delta instead of the total snapshot count, matching upstream.
  • Side-effects-cache prefetch still covers skipped snapshots so BuildModules's is_built gate fires on warm reinstalls — without this, packages with allowBuilds: true re-execute their lifecycle scripts every install (added in 1603f94 after benchmarking caught the regression — see benchmark comment).

Tracks pnpm v11 94240bc046 for readCurrentLockfile / writeCurrentLockfile / lockfileToDepGraph's per-depPath skip and isIntegrityEqual helper.

Performance

Two scenarios benchmarked against a 1352-package fixture, hyperfine --warmup 2 --runs 10.

Pure warm reinstall (every snapshot intact, full lock.yaml on disk):

mean sys time
baseline (main HEAD~2) 969 ms 985 ms
PR 896 ms 349 ms

PR is 1.08× faster, with ~65% less kernel work — the skip filter bypasses the warm-batch's per-snapshot syscalls (1352 → 0). Full breakdown + the regression we caught + fixed.

Non-up-to-date node_modules (N virtual-store slots deleted per iteration):

Damaged slots PR Baseline PR/Baseline
0 581 ms 661 ms 1.14×
1 615 ms 707 ms 1.15×
10 660 ms 707 ms 1.07×
100 2.25 s 2.25 s 1.00×
500 6.13 s 7.93 s 1.29×

PR is consistently a small win and never a loss across the damage spectrum; _broken_node_modules debug events fire correctly for each missing slot. Full sweep + caveats.

Test plan

  • just ready — 633 tests pass, lint clean.
  • just dylint — perfectionist clean.
  • taplo format --check — clean.
  • Unit tests: snapshot_deps_equal, integrity_equal, BrokenModulesLog wire shape, Lockfile::is_empty, save_current_to_virtual_store_dir round-trip / empty-deletes-file / ENOENT-as-none.
  • Integration tests in install/tests.rs:
    • warm_reinstall_skips_snapshot_when_current_lockfile_matches — proves the skip path short-circuits the fetch (the fixture points at a bogus URL; only the skip path makes the install succeed).
    • warm_reinstall_emits_broken_modules_when_dir_is_missing — asserts the _broken_node_modules debug event fires before the fetch.
    • warm_reinstall_reports_added_zero_and_emits_no_imported_events — asserts pnpm:stats.added: 0 and zero pnpm:progress imported events when the skip path drops every snapshot from the install graph.
    • context_log_reflects_current_lockfile_after_first_installpnpm:context.currentLockfileExists flips falsetrue once lock.yaml is on disk, and lock.yaml is asserted present on disk after the first install (exercises the real write path).

Out of scope (tracked in #433)


Written by an agent (Claude Code, claude-opus-4-7).

Read `<virtual_store_dir>/lock.yaml` at install start and diff each
wanted snapshot against it. A snapshot whose `dependencies`,
`optionalDependencies`, and `resolution.integrity` match the current
lockfile *and* whose virtual-store slot exists on disk is dropped
from the install graph entirely — no fetch, no extract, no relink.
When the slot is gone but the cache key still matches, pacquet
emits `pnpm:_broken_node_modules` and falls through to the full
install path for that snapshot. At end-of-install pacquet writes
the wanted lockfile to `<virtual_store_dir>/lock.yaml` (deleting
the file when the lockfile is empty), so the next install sees a
fresh diff target.

Mirrors upstream pnpm's `readCurrentLockfile` / `writeCurrentLockfile`
loop and the per-`depPath` skip in `lockfileToDepGraph` at pnpm v11
`94240bc046`. Closes #433 sections A (read), B (per-snapshot skip),
C (write), and D (reporter signal).

`pnpm:context.currentLockfileExists` now reflects reality (was
hard-coded `false`) and `pnpm:stats.added` reports the post-skip
delta rather than the total snapshot count, matching upstream.
Copilot AI review requested due to automatic review settings May 13, 2026 10:25
@coderabbitai

coderabbitai Bot commented May 13, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Reads/writes a current lockfile at the virtual-store root, adds per-snapshot equality checks to skip unchanged snapshots when possible, emits a BrokenModules event for missing cached slots, and threads current-lockfile state through frozen-lockfile and install flows with tests.

Changes

Virtual-store lockfile persistence and snapshot skip filtering

Layer / File(s) Summary
Lockfile API contracts and empty-check
crates/lockfile/src/lib.rs
Adds CURRENT_FILE_NAME constant for lock.yaml, documents root-importer accessors, and introduces is_empty() to detect lockfiles with no specifiers/dependencies.
Lockfile load/save I/O with atomic writes
crates/lockfile/src/load_lockfile.rs, crates/lockfile/src/save_lockfile.rs
Implements load_current_from_virtual_store_dir to read <virtual_store>/lock.yaml; implements save_current_to_virtual_store_dir which deletes when empty or atomically writes lock.yaml (via write_atomic); extends SaveLockfileError with CreateDir, RemoveFile, and RenameFile.
Lockfile I/O tests
crates/lockfile/src/save_lockfile/tests.rs
Tests round-trip serialization, Ok(None) when missing, deletion when saving empty over an existing file, and no-op when saving empty into an absent virtual-store slot.
CreateVirtualStore inputs and plumbing
crates/package-manager/src/create_virtual_store.rs
Adds current_snapshots/current_packages inputs and wires them into run to drive per-snapshot skip/filter logic.
Snapshot skip pass, helpers, and tests
crates/package-manager/src/create_virtual_store.rs, crates/package-manager/src/create_virtual_store/tests.rs
Introduces a per-snapshot skip pass that uses snapshot_deps_equal and integrity_equal (absent-vs-empty semantics) and checks virtual-store slot existence; includes unit tests for dependency/integrity equality cases.
Warm-install restructuring
crates/package-manager/src/create_virtual_store.rs
Restructures warm-install cache warm-up into two passes: skipped snapshots seed caches first, survivors drive warm/cold partitioning and linking.
BrokenModules reporter event and tests
crates/reporter/src/lib.rs, crates/reporter/src/tests.rs
Adds LogEvent::BrokenModules(BrokenModulesLog) serialized as pnpm:_broken_node_modules with level and missing path, and a test validating the pnpm wire shape.
Install flow: load, propagate, save current lockfile
crates/package-manager/src/install.rs
Install::run reads <virtual_store>/lock.yaml, sets ContextLog.current_lockfile_exists from the load result, forwards current state into frozen-lockfile flows, persists the materialized lockfile after install, and adds LoadCurrentLockfile/SaveCurrentLockfile InstallError variants.
Frozen-lockfile propagation
crates/package-manager/src/install_frozen_lockfile.rs
Adds current_snapshots and current_packages fields and forwards them into CreateVirtualStore to enable per-snapshot skip decisions during frozen installs.
Integration tests for warm reinstall and skip behavior
crates/package-manager/src/install/tests.rs
Integration tests for warm reinstall skipping, BrokenModules emission when slot is missing, context-log flip after first install writes lock.yaml, added: 0 reporting, and suppression of per-snapshot imported events for skipped snapshots.

Sequence Diagram(s)

sequenceDiagram
  participant Installer
  participant CreateVirtualStore
  participant FileSystem
  participant Reporter
  Installer->>CreateVirtualStore: provide current_snapshots/current_packages
  CreateVirtualStore->>CreateVirtualStore: snapshot_deps_equal / integrity_equal checks
  CreateVirtualStore->>FileSystem: stat virtual-store slot path
  FileSystem-->>CreateVirtualStore: exists / missing
  CreateVirtualStore->>Reporter: emit pnpm:_broken_node_modules when missing
  Installer->>FileSystem: save_current_to_virtual_store_dir (atomic write or delete)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

Possibly related PRs

  • pnpm/pacquet#375: Touches reporter event enum changes and pnpm-channel payload additions similar to this PR.
  • pnpm/pacquet#424: Modifies CreateVirtualStore and intersects at package-manager install pipeline changes.

Poem

🐰 I hop through node_modules, neat and spry,

lock.yaml tucked where snapshots lie.
If deps and integrity agree,
I'll skip the fetch and set you free —
atomic writes and carrots in supply.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat: partial install with --frozen-lockfile (#433)' clearly summarizes the main change: implementing partial install capability with the frozen-lockfile mode.
Linked Issues check ✅ Passed The PR successfully implements all core requirements from issue #433: reads current lockfile via load_current_from_virtual_store_dir, skips snapshots when dependencies/integrity match and slot exists, writes current lockfile atomically, emits BrokenModulesLog when slot is missing, and updates statistics/context-logging with accurate counts and existence flags. Comprehensive test coverage validates all scenarios.
Out of Scope Changes check ✅ Passed All changes are directly scoped to implementing partial install with frozen-lockfile (#433). Changes include lockfile reading/writing, skip logic with cache-key matching, broken-modules reporting, and statistics updates—all within PR objectives. Out-of-scope items (workspace filtering, GVS, pruning) are correctly deferred.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description check ✅ Passed The PR description comprehensively addresses all template requirements: it provides a clear summary of changes, references the closed issue (#433), links upstream pnpm commits, confirms pnpm behavior parity, notes testing completion, and includes a detailed rationale with performance benchmarks.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/433

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented May 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.23353% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.49%. Comparing base (b514929) to head (ee80c24).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
crates/lockfile/src/save_lockfile.rs 64.58% 17 Missing ⚠️
crates/package-manager/src/create_virtual_store.rs 89.88% 9 Missing ⚠️
crates/lockfile/src/load_lockfile.rs 77.77% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #442      +/-   ##
==========================================
+ Coverage   86.98%   88.49%   +1.50%     
==========================================
  Files          93       93              
  Lines        6647     6822     +175     
==========================================
+ Hits         5782     6037     +255     
+ Misses        865      785      -80     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements pnpm-style “current lockfile” tracking (<virtual_store_dir>/lock.yaml) to enable warm, partial installs under --frozen-lockfile by skipping snapshots that are unchanged and still present on disk, while preserving pnpm reporter wire compatibility (including _broken_node_modules).

Changes:

  • Add load/save support for the current lockfile (lock.yaml) in pacquet_lockfile, including Lockfile::is_empty and atomic-ish persistence behavior.
  • Thread current-lockfile state through the frozen-lockfile install pipeline to skip unchanged snapshots, emit _broken_node_modules when a slot is missing, and report pnpm:stats.added based on the post-skip delta.
  • Add/extend unit + integration coverage for skip behavior, broken-modules event shape, and current-lockfile read/write semantics.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
crates/reporter/src/tests.rs Adds a serialization test to pin pnpm:_broken_node_modules wire shape.
crates/reporter/src/lib.rs Introduces LogEvent::BrokenModules + payload type for _broken_node_modules.
crates/package-manager/src/install/tests.rs Adds integration tests covering skip path, broken-modules emission, context flag flip, and stats/progress behavior.
crates/package-manager/src/install.rs Loads current lockfile for context + skip inputs; writes current lockfile at end of frozen installs.
crates/package-manager/src/install_frozen_lockfile.rs Threads current snapshots/packages into the frozen-lockfile install runner.
crates/package-manager/src/create_virtual_store/tests.rs Adds unit tests for snapshot dep equality + integrity equality helpers.
crates/package-manager/src/create_virtual_store.rs Implements per-snapshot skip filtering and adjusts pnpm:stats.added based on filtered set.
crates/lockfile/src/save_lockfile/tests.rs Adds tests for current-lockfile round-trip, ENOENT-as-None, and empty-lockfile delete semantics.
crates/lockfile/src/save_lockfile.rs Implements writing/deleting <virtual_store_dir>/lock.yaml with a temp+rename helper.
crates/lockfile/src/load_lockfile.rs Adds load_current_from_virtual_store_dir and refactors shared load-from-path logic.
crates/lockfile/src/lib.rs Adds CURRENT_FILE_NAME constant and Lockfile::is_empty helper for suppressing stale current lockfiles.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/lockfile/src/save_lockfile.rs Outdated
Comment thread crates/package-manager/src/install.rs
@github-actions

github-actions Bot commented May 13, 2026

Copy link
Copy Markdown

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.06     17.3±0.65ms   251.1 KB/sec    1.00     16.2±0.31ms   267.1 KB/sec

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@crates/package-manager/src/create_virtual_store.rs`:
- Around line 252-257: The loop currently calls snapshot_cache_key for every
entry in snapshots before applying the current-lockfile skip filter, causing
UnsupportedResolution for unchanged directory-backed snapshots; change the logic
so you first run the skip filter to produce the subset of snapshots that
survive, then derive cache keys only for those survivors (i.e., apply
snapshot_cache_key only after filtering). Update the code that builds
all_snapshot_entries (the mapping over snapshots and the call to
snapshot_cache_key) to operate on the filtered collection so existing
materialized slots can be reused instead of failing early.

In `@crates/package-manager/src/install.rs`:
- Around line 244-257: The current code persists the current lockfile early (the
if frozen_lockfile && let Some(lockfile) {
lockfile.save_current_to_virtual_store_dir(...) ... }) which can leave a new
lock.yaml on disk if subsequent steps like write_modules_manifest fail; move
this persistence so it runs only after the install is fully committed (i.e.,
after write_modules_manifest returns successfully and any final commit steps
complete) and keep the same error mapping to InstallError::SaveCurrentLockfile;
ensure the condition still checks frozen_lockfile && let Some(lockfile) and
invoke lockfile.save_current_to_virtual_store_dir(&config.virtual_store_dir)
only on the successful path.

In `@crates/package-manager/src/install/tests.rs`:
- Around line 941-1054: The test currently bypasses the write path by using an
empty Lockfile first and then pre-seeding the virtual store before the second
Install::run; change the first run to use a non-empty fixture (e.g., parse
PARTIAL_INSTALL_LOCKFILE into the initial Lockfile instead of the empty one) so
the install must persist a non-empty current lockfile, and remove the manual
call to partial.save_current_to_virtual_store_dir(&virtual_store_dir) (and any
unnecessary seed_placeholder_virtual_store_slot call) so the second Install::run
exercises the read-after-write path rather than relying on pre-seeded state.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9b4f9a4b-6503-4319-941d-2b0e2283b27e

📥 Commits

Reviewing files that changed from the base of the PR and between b514929 and e6fae21.

📒 Files selected for processing (11)
  • crates/lockfile/src/lib.rs
  • crates/lockfile/src/load_lockfile.rs
  • crates/lockfile/src/save_lockfile.rs
  • crates/lockfile/src/save_lockfile/tests.rs
  • crates/package-manager/src/create_virtual_store.rs
  • crates/package-manager/src/create_virtual_store/tests.rs
  • crates/package-manager/src/install.rs
  • crates/package-manager/src/install/tests.rs
  • crates/package-manager/src/install_frozen_lockfile.rs
  • crates/reporter/src/lib.rs
  • crates/reporter/src/tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: Agent
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Lint and Test (macos-latest)
  • GitHub Check: Lint and Test (windows-latest)
  • GitHub Check: Code Coverage
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Preserve existing method chains and pipe-trait chains; do not break them into intermediate let bindings unless there is a concrete justification such as a compilation failure, borrow checker rejection, meaningful performance improvement, or other technical necessity. Refactoring for style alone is not sufficient justification.
Choose owned vs. borrowed parameters to minimize copies; prefer borrowed types (&Path over &PathBuf, &str over &String) when it does not force extra copies.
Prefer Arc::clone(&x) and Rc::clone(&x) over x.clone() for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Write use super::{Foo, bar} instead of use super::*; for any glob whose target is a module you control. External-crate preludes (e.g., use rayon::prelude::*;) and root-of-module re-exports (e.g., pub use submodule::*; in lib.rs) are exceptions.
Follow Rust API Guidelines for naming, as documented in https://rust-lang.github.io/api-guidelines/naming.html.
Declare a newtype wrapper for any branded string type being ported from TypeScript pnpm. Do not collapse the brand into a plain String or &str; give the type its own struct so misuse is a type error.
When porting branded string types where upstream TypeScript always validates before construction, validate in the Rust port too. Construct the wrapper only via TryFrom<String> and/or FromStr; do not provide an infallible public constructor that takes an arbitrary string.
For branded string types where upstream TypeScript never validates (used purely for type-safety to prevent confusion between string slots), expose an infallible From<String> and From<&str> constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bare as assertion), add a from_str_unchecked (or similarly named) constructor on the Rust side. Keep the validating constructor as well; `from_str_u...

Files:

  • crates/reporter/src/lib.rs
  • crates/lockfile/src/lib.rs
  • crates/package-manager/src/install_frozen_lockfile.rs
  • crates/lockfile/src/save_lockfile/tests.rs
  • crates/package-manager/src/create_virtual_store/tests.rs
  • crates/reporter/src/tests.rs
  • crates/package-manager/src/install.rs
  • crates/lockfile/src/load_lockfile.rs
  • crates/lockfile/src/save_lockfile.rs
  • crates/package-manager/src/install/tests.rs
  • crates/package-manager/src/create_virtual_store.rs
🧠 Learnings (3)
📚 Learning: 2026-05-07T23:19:08.272Z
Learnt from: KSXGitHub
Repo: pnpm/pacquet PR: 401
File: tasks/integrated-benchmark/src/work_env.rs:343-344
Timestamp: 2026-05-07T23:19:08.272Z
Learning: When reviewing Rust code in pnpm/pacquet for deprecated API usage, do not automatically treat `serde_saphyr::to_string` as deprecated. In `serde-saphyr` v0.0.25, `serde_saphyr::to_string` has no `#[deprecated]` attribute (the `#[deprecated]` later in `serde-saphyr-0.0.25/src/lib.rs` applies to a different function). Only flag `serde_saphyr::to_string` as deprecated if the resolved dependency version’s source shows `#[deprecated]` on that specific function.

Applied to files:

  • crates/reporter/src/lib.rs
  • crates/lockfile/src/lib.rs
  • crates/package-manager/src/install_frozen_lockfile.rs
  • crates/lockfile/src/save_lockfile/tests.rs
  • crates/package-manager/src/create_virtual_store/tests.rs
  • crates/reporter/src/tests.rs
  • crates/package-manager/src/install.rs
  • crates/lockfile/src/load_lockfile.rs
  • crates/lockfile/src/save_lockfile.rs
  • crates/package-manager/src/install/tests.rs
  • crates/package-manager/src/create_virtual_store.rs
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In Rust test code, follow the repo’s CODE_STYLE_GUIDE test-logging rule: add logging (e.g., `eprintln!`/`eprintln!(...)`) so that useful diagnostic values are printed when a test fails, unless the assertion is `assert_eq!` (where the differing values are already included). Concretely, if you use assertions like `assert!`, `assert_ne!`, etc., ensure the test logs the relevant actual/expected values (or context) before/around the assertion so failures can be diagnosed without rerunning.

Applied to files:

  • crates/lockfile/src/save_lockfile/tests.rs
  • crates/package-manager/src/create_virtual_store/tests.rs
  • crates/reporter/src/tests.rs
  • crates/package-manager/src/install/tests.rs
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In this repo’s Rust tests (run via `cargo nextest`), stdout/stderr from passing tests is captured and is only shown when a test fails. As a result, `eprintln!` (and similar stderr logging) in these tests should generally not be flagged as CI “noise” on the happy path; CI output should only appear for failing tests.

Applied to files:

  • crates/reporter/src/tests.rs
🔇 Additional comments (9)
crates/reporter/src/lib.rs (1)

166-177: LGTM!

Also applies to: 600-608

crates/reporter/src/tests.rs (1)

8-12: LGTM!

Also applies to: 639-658

crates/lockfile/src/lib.rs (1)

78-85: LGTM!

Also applies to: 95-108

crates/lockfile/src/load_lockfile.rs (1)

8-9: LGTM!

Also applies to: 33-34, 36-54, 56-63

crates/lockfile/src/save_lockfile.rs (2)

4-99: LGTM!

Also applies to: 102-117


118-123: std::fs::rename on Windows correctly replaces existing destination files—no issue here.

The Rust standard library explicitly documents that rename(src, dst) replaces dst if it already exists; on Windows this is implemented via MoveFileEx or equivalent APIs with MOVEFILE_REPLACE_EXISTING semantics. Warm installs (when lock.yaml already exists) will succeed as intended. The atomic write pattern used here (write to temp, rename in place) matches best practices and pnpm's approach.

Note: The caveat on Windows is that dst must not be a directory; this is not an issue for a file like lock.yaml.

crates/lockfile/src/save_lockfile/tests.rs (1)

102-175: LGTM!

crates/package-manager/src/install_frozen_lockfile.rs (1)

39-47: LGTM!

Also applies to: 101-103, 128-129

crates/package-manager/src/create_virtual_store/tests.rs (1)

1-4: LGTM!

Also applies to: 6-6, 8-36, 78-168

Comment thread crates/package-manager/src/create_virtual_store.rs Outdated
Comment thread crates/package-manager/src/install.rs Outdated
Comment thread crates/package-manager/src/install/tests.rs Outdated
- Harden `write_atomic` in `save_lockfile.rs`: open the temp file with
  `O_CREAT | O_EXCL` (`create_new(true)`) and retry on AlreadyExists,
  matching the pattern in `pacquet_fs::ensure_file::write_atomic`. Was
  using `fs::write` which followed symlinks and clobbered colliding
  temp files (Copilot review).
- Run the per-snapshot skip filter before deriving cache keys in
  `CreateVirtualStore::run` so unchanged directory-backed snapshots
  don't fail with `UnsupportedResolution` ahead of the skip check.
  Cache-key derivation now only happens for survivors (CodeRabbit).
- Move `save_current_to_virtual_store_dir` after `write_modules_manifest`
  so a manifest-write failure can't leave a fresh current lockfile
  pointing at incomplete install state (CodeRabbit).
- Fix stale comment on `current_lockfile` in `Install::run` that
  referenced a non-existent `current_lockfile_load_error` and claimed
  None-on-corruption; the code actually propagates `LoadLockfileError`
  via `?` and fails the install (Copilot).
- Restructure `context_log_reflects_current_lockfile_after_first_install`
  so the false→true assertion really exercises the install's write
  path: same non-empty fixture across both runs, no manual
  `save_current_to_virtual_store_dir` seeding before the second run,
  plus a positive assertion that `lock.yaml` is on disk after the first
  install. Skip-path side effects (`added: 0`, zero `imported` events)
  are split into their own test (CodeRabbit).
@github-actions

github-actions Bot commented May 13, 2026

Copy link
Copy Markdown

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.563 ± 0.065 2.445 2.712 1.01 ± 0.03
pacquet@main 2.529 ± 0.049 2.465 2.604 1.00
pnpm 6.134 ± 0.039 6.061 6.178 2.43 ± 0.05
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 2.56256568508,
      "stddev": 0.0649922598002834,
      "median": 2.55277171568,
      "user": 2.55223742,
      "system": 3.6237814799999994,
      "min": 2.44498583968,
      "max": 2.71194252668,
      "times": [
        2.57929243468,
        2.55732040168,
        2.54889673568,
        2.58620045168,
        2.71194252668,
        2.55144151668,
        2.55191891268,
        2.55362451868,
        2.54003351268,
        2.44498583968
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 2.52900869648,
      "stddev": 0.049042433018348444,
      "median": 2.5190278686800003,
      "user": 2.5496613200000007,
      "system": 3.5801888799999992,
      "min": 2.46488060768,
      "max": 2.6039298626800003,
      "times": [
        2.51841525568,
        2.51964048168,
        2.5068641076800002,
        2.5719856826800003,
        2.57080810368,
        2.49190751668,
        2.6039298626800003,
        2.46488060768,
        2.57617166268,
        2.46548368368
      ]
    },
    {
      "command": "pnpm",
      "mean": 6.13421727698,
      "stddev": 0.03853284854194687,
      "median": 6.14298718268,
      "user": 8.965209519999998,
      "system": 4.53997508,
      "min": 6.06143088668,
      "max": 6.17841032168,
      "times": [
        6.12231688868,
        6.14744765568,
        6.17412955168,
        6.11826475568,
        6.15252013368,
        6.08328323668,
        6.06143088668,
        6.16584262968,
        6.17841032168,
        6.13852670968
      ]
    }
  ]
}

Scenario: Frozen Lockfile (Hot Cache)

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 700.3 ± 68.9 650.1 884.7 1.01 ± 0.14
pacquet@main 695.9 ± 72.0 649.2 892.0 1.00
pnpm 2701.7 ± 96.5 2554.6 2903.9 3.88 ± 0.43
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.7002537486200001,
      "stddev": 0.06893507490107097,
      "median": 0.6766295647200001,
      "user": 0.37144222000000005,
      "system": 1.48132798,
      "min": 0.6500868692200001,
      "max": 0.8846893432200001,
      "times": [
        0.8846893432200001,
        0.6713135302200001,
        0.6732437792200001,
        0.6629906412200001,
        0.6862435972200001,
        0.6711381622200001,
        0.6800153502200001,
        0.6833265392200001,
        0.6500868692200001,
        0.7394896742200001
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.6958914008200001,
      "stddev": 0.07201546264807367,
      "median": 0.68032391822,
      "user": 0.34649251999999997,
      "system": 1.48458168,
      "min": 0.6491565092200001,
      "max": 0.8920035132200002,
      "times": [
        0.8920035132200002,
        0.6774449012200001,
        0.6518815692200001,
        0.6845115092200001,
        0.7113252122200001,
        0.6533833732200001,
        0.6491565092200001,
        0.6582650392200001,
        0.6977394462200001,
        0.6832029352200001
      ]
    },
    {
      "command": "pnpm",
      "mean": 2.7017339414199997,
      "stddev": 0.09653537302219489,
      "median": 2.7058586492199996,
      "user": 3.16220812,
      "system": 2.27416038,
      "min": 2.55458438822,
      "max": 2.90393533522,
      "times": [
        2.74079652522,
        2.55458438822,
        2.6174519722199996,
        2.7574112172199996,
        2.6231852502199997,
        2.70216760822,
        2.70954969022,
        2.6631325312199996,
        2.90393533522,
        2.74512489622
      ]
    }
  ]
}

Without this, the skip filter bypasses the side-effects-cache
prefetch for any snapshot it skips, so `BuildModules`'s `is_built`
gate misses on warm reinstalls and every package with
`allowBuilds: true` re-executes its lifecycle scripts. On a fixture
with 1352 packages and 3 allowed builds, warm reinstalls regressed
from ~0.9 s (baseline / `main`) to ~3.0 s.

Fix: derive cache keys for *all* snapshots, not just survivors.
Survivors go through the strict path so a malformed wanted snapshot
still surfaces. Skipped snapshots get a lenient pass: any
per-snapshot error collapses to no prefetch entry (same outcome as
if the skip filter had not engaged). The combined cache keys feed
the prefetch and `package_manifests` / `side_effects_maps_by_snapshot`
get populated over the full snapshot set.

Warm/cold partitioning still operates only on survivors — the skip
filter's effect on the link work is unchanged.

After the fix, warm reinstall on a 1352-package fixture:
- PR: 896 ms ± 66 ms (sys 349 ms)
- main: 969 ms ± 39 ms (sys 985 ms)
- PR is 1.08x faster, with ~65% less kernel work
Copilot AI review requested due to automatic review settings May 13, 2026 11:18
@zkochan

zkochan commented May 13, 2026

Copy link
Copy Markdown
Member Author

Benchmark results

Ran a warm-reinstall benchmark to verify the PR's performance impact. Fixture: a 1352-package lockfile (one of the larger real-world fixtures I have), warm CAFS, warm node_modules, hyperfine --warmup 2 --runs 10.

Round 1 — found a regression

mean range sys time
baseline (main HEAD~2) 816 ms 683-1010 ms 770 ms
PR (initial) 2.65 s 2.43-2.88 s 801 ms

PR was 3.25× slower. Bisected the cause:

[BENCH] read_current_lockfile: 12 ms       ← negligible
[BENCH] skip_filter:           7 ms        ← negligible
[BENCH] CreateVirtualStore::run: 8 ms      ← saved 330 ms vs full graph
[BENCH] SymlinkDirectDependencies::run: 14 ms
[BENCH] LinkVirtualStoreBins::run: 22 ms
[BENCH] BuildModules::run:    3,053 ms     ← +2.5 s vs full graph
[BENCH] write_current_lockfile: 19 ms

Root cause: the skip filter ran before snapshot_cache_key derivation, so the side-effects-cache prefetch only covered survivors. With 0 survivors on a warm reinstall, side_effects_maps_by_snapshot was empty, BuildModules's is_built gate missed, and the 3 packages with allowBuilds: true re-executed their lifecycle scripts every install.

Round 2 — after the fix

Commit 1603f94 derives cache keys for all snapshots (lenient for skipped ones), so the prefetch covers everyone the build phase might need to gate on. Warm/cold partitioning still operates only on survivors.

mean range sys time
baseline (main HEAD~2) 969 ms 899-1035 ms 985 ms
PR (fixed) 896 ms 796-1004 ms 349 ms

PR is 1.08× faster than baseline on warm reinstall, with ~65% less kernel work. The big sys-time drop is the skip filter avoiding 1352 per-snapshot syscalls in the warm-batch CAFS link path.

The win is modest on this workload because pacquet's warm-batch was already cheap (~330 ms) when every per-snapshot operation collapses to AlreadyExists/NotFound short-circuits. The skip path's bigger benefit lands when:

  • node_modules has partial damage (skip fires on intact slots, full install on missing ones — covered by the broken-modules test)
  • The store is on a slower volume (per-snapshot syscalls cost more)
  • Workspaces land (more snapshots → bigger absolute saving)

Test suite

All 633 tests still pass, lint + dylint + taplo clean. The fix didn't require any test changes — the existing warm_reinstall_* tests cover the skip-fires scenario; the should_install_dependencies test catches the cold-install path.


Written by an agent (Claude Code, claude-opus-4-7).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Comment thread crates/reporter/src/lib.rs Outdated
Comment thread crates/package-manager/src/create_virtual_store.rs Outdated
Copilot review on PR #442 noted that two doc comments hard-coded
`node_modules/.pnpm/...` even though pacquet's `virtual_store_dir`
is configurable (default `.pnpm` to match pnpm v11, but tests and
custom configs override it). Switch to the `<virtual_store_dir>`
placeholder used elsewhere in the codebase so the inline docs match
the actual on-disk layout regardless of how the user configures it.
@zkochan

zkochan commented May 13, 2026

Copy link
Copy Markdown
Member Author

Additional benchmark: non-up-to-date node_modules

Following up on the question of how the PR behaves when node_modules has partial damage (slots deleted from the virtual store between installs — antivirus quarantine, manual rm -rf, fresh git checkout from a branch that touched fewer packages, etc.). Same 1352-package fixture, hyperfine --warmup 2 --runs 10, --prepare deletes N stride-distributed slots before each run.

Damaged slots PR mean Baseline mean PR/Baseline PR sys time Baseline sys time
0 (pure warm) 581 ms 661 ms 1.14× faster 327 ms 872 ms
1 615 ms 707 ms 1.15× faster 371 ms 769 ms
10 660 ms 707 ms 1.07× faster 428 ms 740 ms
100 2.25 s 2.25 s 1.00× (equivalent) 2.49 s 2.68 s
500 6.13 s 7.93 s 1.29× faster 10.4 s 10.4 s

Interpretation

  • Low damage (N=0-10): PR's skip path bypasses 1342+ unchanged slots' per-snapshot syscalls; baseline walks the full graph and pays ~5× more sys time on the no-op AlreadyExists short-circuits. Wins are small in absolute wall time (50-90 ms) but consistent.
  • Mid damage (N=100): per-slot install work dominates; both paths pay the same actual fetch+link cost for the damaged slots, and the skip savings on the unchanged majority get masked by the absolute install time.
  • High damage (N=500): PR's skip path still saves the ~852 unchanged-slot link attempts the baseline does as no-ops. Even with the install path running for 500 slots, the 1.29× win shows up. Variance is high here (±1.3 s / ±2.5 s), so take with a grain of salt.

Caveats

  • Workload-dependent. On a slower volume (HDD, network FS, container storage) per-snapshot syscalls cost more, so the relative wins should grow.
  • Real-world "non-up-to-date" usually means the wanted lockfile changed, not node_modules has random damage. I can't easily simulate "lockfile changed" without breaking integrity checks, but the broken-modules scenario is the closest stand-in pacquet's frozen-lockfile path will accept.
  • _broken_node_modules debug events fire correctly per the existing test (warm_reinstall_emits_broken_modules_when_dir_is_missing), so a reporter consumer would see one event per damaged slot.

Bottom line: the PR is consistently a small win and never a loss across the damage spectrum, in addition to the pure-warm 1.08× win in the original benchmark.


Written by an agent (Claude Code, claude-opus-4-7).

@zkochan zkochan merged commit 6ca5332 into main May 13, 2026
15 of 16 checks passed
@zkochan zkochan deleted the feat/433 branch May 13, 2026 11:54
zkochan added a commit that referenced this pull request May 13, 2026
Two cleanups while rebasing on top of #442 (partial frozen install):

- `install.rs`: the comment at the `<virtual_store_dir>/lock.yaml`
  write site referenced #431 as future work. With workspace install
  now landing in this PR, refresh the wording to call out that we
  *do* ship every importer's section unchanged, and the remaining
  narrowing (selected importers × engine filter) is gated on
  `--filter` (Stage 2 of #299).
- `root_finder/tests.rs`: rustfmt collapses the if/else closures
  introduced in a0d4df8 to single-line `if … else …` form. Apply
  it once here so the diff doesn't keep returning across runs.

---
Written by an agent (Claude Code, claude-opus-4-7).
zkochan added a commit that referenced this pull request May 13, 2026
After rebasing onto current main (#442 — partial install with
--frozen-lockfile) the CI clippy gate fires two errors I missed
locally:

- `get_subgraph_to_build` had 8 parameters after the `skipped`
  thread-through (clippy::too_many_arguments, threshold 7). Bundle
  the 4 read-only inputs (`children`, `requires_build`, `patches`,
  `skipped`) into a `GetSubgraphCtx` struct; the 3 `&mut`
  accumulators stay as plain params. 5 total now.
- `Some(list) if list.is_empty()` in `platform_axis_meaningful` is
  clippy::redundant_guards. Use the empty-slice pattern
  (`Some([])`) and the single-element pattern (`Some([only]) if
  only == "any"`) so each pattern says what it matches without a
  separate guard clause.

The rebase itself: main's #442 added a per-snapshot "skip
already-installed-and-still-on-disk" pass in
`CreateVirtualStore::run`. Layered the installability skip on top
— it filters BEFORE that pass (installability-skipped snapshots
should never enter the install graph at all, including the
`skipped_entries` cache-key lane that #442 keeps warm for the
build-cache lookup).
zkochan added a commit that referenced this pull request May 13, 2026
Two cleanups while rebasing on top of #442 (partial frozen install):

- `install.rs`: the comment at the `<virtual_store_dir>/lock.yaml`
  write site referenced #431 as future work. With workspace install
  now landing in this PR, refresh the wording to call out that we
  *do* ship every importer's section unchanged, and the remaining
  narrowing (selected importers × engine filter) is gated on
  `--filter` (Stage 2 of #299).
- `root_finder/tests.rs`: rustfmt collapses the if/else closures
  introduced in a0d4df8 to single-line `if … else …` form. Apply
  it once here so the diff doesn't keep returning across runs.

---
Written by an agent (Claude Code, claude-opus-4-7).
zkochan added a commit that referenced this pull request May 13, 2026
After rebasing onto current main (#442 — partial install with
--frozen-lockfile) the CI clippy gate fires two errors I missed
locally:

- `get_subgraph_to_build` had 8 parameters after the `skipped`
  thread-through (clippy::too_many_arguments, threshold 7). Bundle
  the 4 read-only inputs (`children`, `requires_build`, `patches`,
  `skipped`) into a `GetSubgraphCtx` struct; the 3 `&mut`
  accumulators stay as plain params. 5 total now.
- `Some(list) if list.is_empty()` in `platform_axis_meaningful` is
  clippy::redundant_guards. Use the empty-slice pattern
  (`Some([])`) and the single-element pattern (`Some([only]) if
  only == "any"`) so each pattern says what it matches without a
  separate guard clause.

The rebase itself: main's #442 added a per-snapshot "skip
already-installed-and-still-on-disk" pass in
`CreateVirtualStore::run`. Layered the installability skip on top
— it filters BEFORE that pass (installability-skipped snapshots
should never enter the install graph at all, including the
`skipped_entries` cache-key lane that #442 keeps warm for the
build-cache lookup).
zkochan added a commit that referenced this pull request May 13, 2026
Two cleanups while rebasing on top of #442 (partial frozen install):

- `install.rs`: the comment at the `<virtual_store_dir>/lock.yaml`
  write site referenced #431 as future work. With workspace install
  now landing in this PR, refresh the wording to call out that we
  *do* ship every importer's section unchanged, and the remaining
  narrowing (selected importers × engine filter) is gated on
  `--filter` (Stage 2 of #299).
- `root_finder/tests.rs`: rustfmt collapses the if/else closures
  introduced in a0d4df8 to single-line `if … else …` form. Apply
  it once here so the diff doesn't keep returning across runs.

---
Written by an agent (Claude Code, claude-opus-4-7).
zkochan added a commit that referenced this pull request May 13, 2026
After rebasing onto current main (#442 — partial install with
--frozen-lockfile) the CI clippy gate fires two errors I missed
locally:

- `get_subgraph_to_build` had 8 parameters after the `skipped`
  thread-through (clippy::too_many_arguments, threshold 7). Bundle
  the 4 read-only inputs (`children`, `requires_build`, `patches`,
  `skipped`) into a `GetSubgraphCtx` struct; the 3 `&mut`
  accumulators stay as plain params. 5 total now.
- `Some(list) if list.is_empty()` in `platform_axis_meaningful` is
  clippy::redundant_guards. Use the empty-slice pattern
  (`Some([])`) and the single-element pattern (`Some([only]) if
  only == "any"`) so each pattern says what it matches without a
  separate guard clause.

The rebase itself: main's #442 added a per-snapshot "skip
already-installed-and-still-on-disk" pass in
`CreateVirtualStore::run`. Layered the installability skip on top
— it filters BEFORE that pass (installability-skipped snapshots
should never enter the install graph at all, including the
`skipped_entries` cache-key lane that #442 keeps warm for the
build-cache lookup).
zkochan added a commit that referenced this pull request May 13, 2026
Two issues flagged by Copilot on #449:

1. `build_modules::virtual_store_dir_for_key` was calling
   `layout.slot_dir(&bare_key)` after peer-stripping the snapshot
   key. `VirtualStoreLayout` keys its precomputed GVS suffixes by
   the *full* snapshot key (with the peer suffix), so the lookup
   missed for peer-resolved snapshots and fell through to the
   legacy flat-name path — pointing at a directory
   `CreateVirtualDirBySnapshot` never created. Effect: build
   scripts silently skipped for peer-resolved packages. Switched
   the lookup to the full `key`; the package-name segment still
   comes from the peer-stripped key.

2. `CreateVirtualStore::run`'s partial-install skip probe (added
   in #442) still hard-coded
   `config.virtual_store_dir.join(to_virtual_store_name)`, which
   is the legacy layout. Under GVS-on the slot lives at
   `layout.slot_dir(snapshot_key)`, so warm slots were being
   classified as missing and `BrokenModules` emitted for the
   wrong path. Routed through the layout to match what the actual
   install / link steps use.

#442's three partial-install tests
(`warm_reinstall_skips_snapshot_when_current_lockfile_matches`,
`warm_reinstall_emits_broken_modules_when_dir_is_missing`,
`warm_reinstall_reports_added_zero_and_emits_no_imported_events`)
seeded `<virtual_store_dir>/<flat-name>` slots manually — the
legacy shape. With the probe now routed through the layout under
default GVS-on, those legacy seeds no longer match. Each test
sets `enable_global_virtual_store = false` so the seeded layout
matches the probe; the partial-install behaviour under test is
independent of the GVS layout, and the GVS-on dispatch is
exercised by the `frozen_lockfile_*_gvs_*` tests.

All 704 workspace tests pass; taplo / dylint / cargo doc clean.
zkochan added a commit that referenced this pull request May 13, 2026
…incompatibles (#434 slice 1) (#439)

Slice 1 of #434 — foundational installability gate. Ports
[`@pnpm/config.package-is-installable`](https://github.com/pnpm/pnpm/tree/94240bc046/config/package-is-installable)
and threads the resulting skip-set through every phase of the
frozen-lockfile install pipeline.

Closes #266 once shipped (covers the "install respects every snapshot
— no os/cpu/libc filter" gap). Does **not** close #434 — that umbrella
has six more slices to follow.

Upstream reference: pnpm/pnpm@94240bc046.

## What landed

### New crate: `pacquet-package-is-installable`

Ports the upstream `config/package-is-installable` package's three
helpers:

- `check_platform` (`Option<&[String]>` for each `os`/`cpu`/`libc`
  axis, plus a `SupportedArchitectures` override) — returns
  `Option<UnsupportedPlatformError>` matching upstream's
  `ERR_PNPM_UNSUPPORTED_PLATFORM` code, message shape, and JSON
  payload. Handles negation entries (`!foo`), the `any` sentinel,
  the `current` placeholder, and the `currentLibc !== 'unknown'`
  skip from `checkPlatform.ts:38`.
- `check_engine` — evaluates `engines.node` / `engines.pnpm` via
  `node-semver`. Approximates npm-semver's `includePrerelease: true`
  via a strip-prerelease fallback; one over-acceptance edge case
  (`>=X.Y.Z` against `X.Y.Z-rc1`) is pinned in the known-failures
  integration test for follow-up.
- `package_is_installable` — composes the two, returns the
  tri-state verdict matching upstream's `boolean | null` (Installable
  / SkipOptional / ProceedWithWarning), plus an `Err` arm for
  `engine_strict` aborts and `ERR_PNPM_INVALID_NODE_VERSION`.

`InstallabilityOptions<'a>` borrows its host strings so a caller
running through many snapshots in a row can build the host part
once and only toggle `optional` per snapshot. `WantedPlatformRef<'a>`
plays the same role for the manifest axes so `check_platform` runs
the happy path without any allocation.

### New module: `pacquet_package_manager::installability`

`compute_skipped_snapshots` is the per-install entry point. For each
snapshot:

1. Look up the matching `PackageMetadata`.
2. Run `check_package` (cached per peer-stripped `metadata_key` so
   peer-resolved variants of the same package share one verdict).
3. Dispatch on `(verdict, snapshot.optional, host.engine_strict)`:
   - `Installable`: nothing to do.
   - `SkipOptional` + `optional`: add to `SkippedSnapshots`, emit
     `pnpm:skipped-optional-dependency` (deduped per metadata key,
     matching upstream's emit-per-pkgId).
   - `Incompatible` + non-optional + `engine_strict`: abort.
   - `Incompatible` + non-optional + non-strict: `tracing::warn!`
     and proceed. (Upstream's `pnpm:install-check` channel isn't
     wired into pacquet's reporter yet — slice 1 follow-up.)

`any_installability_constraint(packages)` is the caller-side fast
path: if no metadata row declares an `engines.{node,pnpm}` or a
non-empty / non-`["any"]` `cpu`/`os`/`libc`, the entire installability
pass is skipped. The probe runs synchronously in
`install_frozen_lockfile` *before* the `tokio::task::spawn_blocking`
that would invoke `node --version` — so the common no-constraints
lockfile pays nothing for the new pipeline, restoring main's overlap
of node-binary startup with extraction.

### Install-pipeline plumbing

The `SkippedSnapshots` set is threaded into every downstream phase
of `InstallFrozenLockfile::run`:

- `CreateVirtualStore`: installability-skipped snapshots are
  dropped from both `survivors` (no virtual-store slot extracted)
  and `skipped_entries` (no warm-cache row). Layered ahead of
  main's #442 already-installed-and-on-disk skip filter.
- `SymlinkDirectDependencies`: a direct dep whose resolved
  snapshot is in the skip set is omitted from `node_modules/<name>`
  (no symlink, no `pnpm:root added` event, no bin link).
- `LinkVirtualStoreBins`: per-slot bin link skips slots whose
  snapshot is installability-skipped (their virtual-store
  directories don't exist).
- `BuildModules` via `build_sequence`: `get_subgraph_to_build`
  consults `skipped` *before* recursion, so a skipped snapshot's
  subtree doesn't contribute to the build graph via that edge.
  Descendants reachable from a non-skipped root still build
  normally.

### Performance

CI integrated-benchmark on the 1352-package fixture, latest run:

| Scenario | `pacquet@HEAD` | `pacquet@main` | Relative |
|---|---|---|---|
| Frozen Lockfile (cold) | 2.476 ± 0.083 s | 2.442 ± 0.071 s | 1.01 ± 0.05 |
| Frozen Lockfile (Hot Cache) | 685.8 ± 59.3 ms | 700.2 ± 47.4 ms | 1.00 |

Earlier iterations of this PR showed a ~5% cold-install regression
from the `node --version` spawn landing on the extraction critical
path. Closed by hoisting the no-constraints fast-path probe to the
caller (commit `cf47ce51`) so the spawn is gated on actual
constraint presence.

Other perf passes folded in:

- `compute_skipped_snapshots` caches the per-metadata-row check
  verdict so peer-resolved variants share one `check_package` call.
- `check_platform` borrows its three wanted axes through
  `WantedPlatformRef<'a>`; the owned `WantedPlatform` only
  materialises in the error path.

## Tests

| Suite | Count | What it covers |
|---|---|---|
| `pacquet-package-is-installable::tests::check_platform` | 16 | Port of upstream `checkPlatform.ts` — `any`/`current` sentinels, negation, `supportedArchitectures` override, libc unknown-skip |
| `pacquet-package-is-installable::tests::check_engine` | 7 | Port of upstream `checkEngine.ts` — node/pnpm range checks, prerelease cases, `ERR_PNPM_INVALID_NODE_VERSION` |
| `pacquet-package-is-installable::tests::package_is_installable` | 6 | Tri-state verdict + optional/engine-strict dispatch |
| `pacquet-package-is-installable::tests::known_failures` | 1 | The `>=X.Y.Z` vs `X.Y.Z-rc1` over-acceptance, picked up by `just known-failures` |
| `pacquet_package_manager::installability::tests` | 11 | Per-install skip-set computation: skip on bad OS, skip on bad node engine, dedup events across peer variants, fast-path triggers, constraint predicate's edge cases (`engines.npm` only, `["any"]` sentinel, empty lists) |
| `pacquet_package_manager::build_sequence::tests` | 3 (new) | Skipped+patched doesn't enter build queue; skipped parent doesn't drag descendants in; descendant with non-skipped parent still builds |

All ported tests verified to catch regressions by temporarily
breaking the subject under test, observing the failure, then
reverting. The "test the tests" workflow from CLAUDE.md.

## Deferred to follow-up slices

- `.modules.yaml.skipped` write/read + headless re-check
  (slice 3).
- `supportedArchitectures` config + `--cpu` / `--os` / `--libc`
  CLI flags (slice 2).
- `pnpm:install-check` warn channel on the reporter side
  (currently `tracing::warn!`).
- Real libc detection — `host_libc()` returns `"unknown"` today;
  matches non-Linux host behavior, but on Alpine/musl this
  over-installs glibc-only optional packages. Slice 2.
- `engine_strict` config wiring — defaults to false today, so
  the error path is unreachable from production. Wired through
  end-to-end so the slice that flips the config doesn't churn
  the error enum.
zkochan added a commit that referenced this pull request May 13, 2026
Two cleanups while rebasing on top of #442 (partial frozen install):

- `install.rs`: the comment at the `<virtual_store_dir>/lock.yaml`
  write site referenced #431 as future work. With workspace install
  now landing in this PR, refresh the wording to call out that we
  *do* ship every importer's section unchanged, and the remaining
  narrowing (selected importers × engine filter) is gated on
  `--filter` (Stage 2 of #299).
- `root_finder/tests.rs`: rustfmt collapses the if/else closures
  introduced in a0d4df8 to single-line `if … else …` form. Apply
  it once here so the diff doesn't keep returning across runs.

---
Written by an agent (Claude Code, claude-opus-4-7).
zkochan added a commit that referenced this pull request May 13, 2026
Two issues flagged by Copilot on #449:

1. `build_modules::virtual_store_dir_for_key` was calling
   `layout.slot_dir(&bare_key)` after peer-stripping the snapshot
   key. `VirtualStoreLayout` keys its precomputed GVS suffixes by
   the *full* snapshot key (with the peer suffix), so the lookup
   missed for peer-resolved snapshots and fell through to the
   legacy flat-name path — pointing at a directory
   `CreateVirtualDirBySnapshot` never created. Effect: build
   scripts silently skipped for peer-resolved packages. Switched
   the lookup to the full `key`; the package-name segment still
   comes from the peer-stripped key.

2. `CreateVirtualStore::run`'s partial-install skip probe (added
   in #442) still hard-coded
   `config.virtual_store_dir.join(to_virtual_store_name)`, which
   is the legacy layout. Under GVS-on the slot lives at
   `layout.slot_dir(snapshot_key)`, so warm slots were being
   classified as missing and `BrokenModules` emitted for the
   wrong path. Routed through the layout to match what the actual
   install / link steps use.

(`warm_reinstall_skips_snapshot_when_current_lockfile_matches`,
`warm_reinstall_emits_broken_modules_when_dir_is_missing`,
`warm_reinstall_reports_added_zero_and_emits_no_imported_events`)
seeded `<virtual_store_dir>/<flat-name>` slots manually — the
legacy shape. With the probe now routed through the layout under
default GVS-on, those legacy seeds no longer match. Each test
sets `enable_global_virtual_store = false` so the seeded layout
matches the probe; the partial-install behaviour under test is
independent of the GVS layout, and the GVS-on dispatch is
exercised by the `frozen_lockfile_*_gvs_*` tests.

All 704 workspace tests pass; taplo / dylint / cargo doc clean.
zkochan added a commit that referenced this pull request May 13, 2026
Two cleanups while rebasing on top of #442 (partial frozen install):

- `install.rs`: the comment at the `<virtual_store_dir>/lock.yaml`
  write site referenced #431 as future work. With workspace install
  now landing in this PR, refresh the wording to call out that we
  *do* ship every importer's section unchanged, and the remaining
  narrowing (selected importers × engine filter) is gated on
  `--filter` (Stage 2 of #299).
- `root_finder/tests.rs`: rustfmt collapses the if/else closures
  introduced in a0d4df8 to single-line `if … else …` form. Apply
  it once here so the diff doesn't keep returning across runs.

---
Written by an agent (Claude Code, claude-opus-4-7).
zkochan added a commit that referenced this pull request May 13, 2026
Two cleanups while rebasing on top of #442 (partial frozen install):

- `install.rs`: the comment at the `<virtual_store_dir>/lock.yaml`
  write site referenced #431 as future work. With workspace install
  now landing in this PR, refresh the wording to call out that we
  *do* ship every importer's section unchanged, and the remaining
  narrowing (selected importers × engine filter) is gated on
  `--filter` (Stage 2 of #299).
- `root_finder/tests.rs`: rustfmt collapses the if/else closures
  introduced in a0d4df8 to single-line `if … else …` form. Apply
  it once here so the diff doesn't keep returning across runs.

---
Written by an agent (Claude Code, claude-opus-4-7).
zkochan added a commit that referenced this pull request May 13, 2026
Two issues flagged by Copilot on #449:

1. `build_modules::virtual_store_dir_for_key` was calling
   `layout.slot_dir(&bare_key)` after peer-stripping the snapshot
   key. `VirtualStoreLayout` keys its precomputed GVS suffixes by
   the *full* snapshot key (with the peer suffix), so the lookup
   missed for peer-resolved snapshots and fell through to the
   legacy flat-name path — pointing at a directory
   `CreateVirtualDirBySnapshot` never created. Effect: build
   scripts silently skipped for peer-resolved packages. Switched
   the lookup to the full `key`; the package-name segment still
   comes from the peer-stripped key.

2. `CreateVirtualStore::run`'s partial-install skip probe (added
   in #442) still hard-coded
   `config.virtual_store_dir.join(to_virtual_store_name)`, which
   is the legacy layout. Under GVS-on the slot lives at
   `layout.slot_dir(snapshot_key)`, so warm slots were being
   classified as missing and `BrokenModules` emitted for the
   wrong path. Routed through the layout to match what the actual
   install / link steps use.

(`warm_reinstall_skips_snapshot_when_current_lockfile_matches`,
`warm_reinstall_emits_broken_modules_when_dir_is_missing`,
`warm_reinstall_reports_added_zero_and_emits_no_imported_events`)
seeded `<virtual_store_dir>/<flat-name>` slots manually — the
legacy shape. With the probe now routed through the layout under
default GVS-on, those legacy seeds no longer match. Each test
sets `enable_global_virtual_store = false` so the seeded layout
matches the probe; the partial-install behaviour under test is
independent of the GVS layout, and the GVS-on dispatch is
exercised by the `frozen_lockfile_*_gvs_*` tests.

All 704 workspace tests pass; taplo / dylint / cargo doc clean.
zkochan added a commit that referenced this pull request May 13, 2026
Two issues flagged by Copilot on #449:

1. `build_modules::virtual_store_dir_for_key` was calling
   `layout.slot_dir(&bare_key)` after peer-stripping the snapshot
   key. `VirtualStoreLayout` keys its precomputed GVS suffixes by
   the *full* snapshot key (with the peer suffix), so the lookup
   missed for peer-resolved snapshots and fell through to the
   legacy flat-name path — pointing at a directory
   `CreateVirtualDirBySnapshot` never created. Effect: build
   scripts silently skipped for peer-resolved packages. Switched
   the lookup to the full `key`; the package-name segment still
   comes from the peer-stripped key.

2. `CreateVirtualStore::run`'s partial-install skip probe (added
   in #442) still hard-coded
   `config.virtual_store_dir.join(to_virtual_store_name)`, which
   is the legacy layout. Under GVS-on the slot lives at
   `layout.slot_dir(snapshot_key)`, so warm slots were being
   classified as missing and `BrokenModules` emitted for the
   wrong path. Routed through the layout to match what the actual
   install / link steps use.

(`warm_reinstall_skips_snapshot_when_current_lockfile_matches`,
`warm_reinstall_emits_broken_modules_when_dir_is_missing`,
`warm_reinstall_reports_added_zero_and_emits_no_imported_events`)
seeded `<virtual_store_dir>/<flat-name>` slots manually — the
legacy shape. With the probe now routed through the layout under
default GVS-on, those legacy seeds no longer match. Each test
sets `enable_global_virtual_store = false` so the seeded layout
matches the probe; the partial-install behaviour under test is
independent of the GVS layout, and the GVS-on dispatch is
exercised by the `frozen_lockfile_*_gvs_*` tests.

All 704 workspace tests pass; taplo / dylint / cargo doc clean.
zkochan added a commit that referenced this pull request May 13, 2026
## Summary

Threads `VirtualStoreLayout` (from #444) through the frozen-lockfile install pipeline so packages installed with `enableGlobalVirtualStore: true` (pacquet's default since #444) land at the upstream-shaped `<store_dir>/links/<scope>/<name>/<version>/<hash>/node_modules/<pkg>` path. Closes the rest of #432 (Section B + C activation + D entry tests).

## Pipeline changes (frozen-lockfile path)

- `InstallFrozenLockfile::run` constructs the install-scoped `VirtualStoreLayout` from the lockfile + engine name + config, then threads `&VirtualStoreLayout` through `CreateVirtualStore`, `CreateVirtualDirBySnapshot`, `create_symlink_layout`, `SymlinkDirectDependencies`, `InstallPackageBySnapshot`, `BuildModules`, and `LinkVirtualStoreBins`. Every site that used to compute `virtual_store_dir.join(key.to_virtual_store_name())` now goes through one `layout.slot_dir(key)` lookup.
- `Install::run` calls `pacquet_store_dir::register_project` **once per importer** when `enable_global_virtual_store` is on, writing `<store_dir>/projects/<short-hash>` per workspace package so a future `pacquet store prune` can find every reachable consumer of `<store_dir>/links/...`. Best-effort: registry write failures degrade to `tracing::warn!` and the install continues. The walk runs *after* `InstallFrozenLockfile::run` succeeds so importer keys have already been validated.
- `Config::current` now applies `apply_global_virtual_store_derivation` after yaml — pacquet's variant of upstream's [`extendInstallOptions.ts:343-355`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/extendInstallOptions.ts#L343-L355). An explicit `globalVirtualStoreDir:` in yaml wins over the derivation; otherwise the field falls back to the user's pinned `virtualStoreDir` (under GVS-on) or to `<store_dir>/links`.

## Field-split deviation from upstream

Upstream pnpm *mutates* `virtualStoreDir` in place under GVS, so every consumer that reads `virtualStoreDir` sees `<storeDir>/links`. Pacquet keeps `virtual_store_dir` at its project-local value and writes the GVS path into the separate `global_virtual_store_dir` field. The frozen-lockfile path consumes `global_virtual_store_dir` through `VirtualStoreLayout`; the legacy non-frozen `InstallWithoutLockfile` keeps reading `virtual_store_dir` (now via `VirtualStoreLayout::legacy`). The split is a pacquet-only concern: upstream has no without-lockfile install path and so doesn't need the second field. See the doc on `Config::apply_global_virtual_store_derivation` for the reasoning.

## Benchmarks

The integrated-benchmark fixture pins `enableGlobalVirtualStore: false` so existing scenarios continue to compare apples-to-apples against the project-local `<modules>/.pnpm/<flat-name>` shape. Without the pin, every revision newer than this PR would silently switch to the GVS layout, while `pacquet@main` and the pnpm side of the comparison would still use the isolated layout — different shape, unfair comparison. GVS-specific benchmark runs are available by passing `--fixture-dir <dir>` at a copy of the fixture with the flag flipped.

## Tests

- `install::tests::frozen_lockfile_under_gvs_registers_project_and_runs_clean` pins the registry write under default GVS-on (single-project case).
- `install::tests::frozen_lockfile_under_gvs_registers_each_workspace_importer` pins per-importer registration: workspace root + `packages/web` each end up with their own symlink in `<store_dir>/projects/`.
- `install::tests::frozen_lockfile_with_gvs_off_skips_project_registry` pins that GVS-off opts out of the registry write.
- `config::tests::yaml_global_virtual_store_dir_wins_over_derivation` pins the yaml-override precedence.
- All **829 workspace tests** pass under the new layout-threaded path.
- Existing per-snapshot legacy tests construct `VirtualStoreLayout::legacy(...)` explicitly so they keep asserting the flat-name layout regardless of any global default. Partial-install tests from #442 pin `enable_global_virtual_store = false` so their seeded legacy-shape slots match the probe path.

End-to-end port of upstream's [`installing/deps-installer/test/install/globalVirtualStore.ts`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/install/globalVirtualStore.ts) is tracked as a follow-up — those tests need a small frozen-lockfile fixture against the registry-mock that doesn't exist yet.
KSXGitHub pushed a commit that referenced this pull request May 13, 2026
Pull in 28 commits from upstream main, including the
`pacquet-npmrc` → `pacquet-config` rename (#420) plus features:
- supportedArchitectures + --cpu/--os/--libc (#456)
- frozen-lockfile (#442, #443, #447, #450)
- git-fetcher (#436 / #446, #451, #454)
- side-effects cache (#421 / #422, #423, #424)
- real-hoist + global-virtual-store (#432 / #438 / #444, #449, #452)
- patchedDependencies + allow-builds (#425, #427, #428)
- engine/platform installability (#434 / #439)

Conflicts resolved:
- `crates/npmrc/` files migrated under the renamed
  `crates/config/` directory; `Npmrc` → `Config` everywhere
  except `NpmrcAuth` (which keeps the `.npmrc`-domain name).
- `Config::current` reads the env-var DI generic `Api: EnvVar`
  for ${VAR}-substitution in `.npmrc`. Production turbofish in
  `cli_args.rs` is `Config::current::<RealApi, _, _, _, _>(...)`.
- Two-phase `NpmrcAuth::apply_*` retained so default-registry
  creds key at the yaml-resolved registry URL.
- New `Config::auth_headers` field plumbed through
  `install_package_by_snapshot`'s `DownloadTarballToStore`.
- Tests under `crates/config/src/workspace_yaml/tests.rs`
  pick up the new ParseYaml unit test added on this branch.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Partial install with --frozen-lockfile: read+write node_modules/.pnpm/lock.yaml and skip unchanged snapshots

2 participants