From a540b24e993b4e47833feb5910f780772de50de0 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Wed, 5 Aug 2026 01:08:40 +0200 Subject: [PATCH] refactor(deps-restorer): split create_virtual_store, and fix stale `file:` dependency copies (#13649) Split `create_virtual_store::run` into per-phase submodules, and fix `file:` dependencies not being re-copied when their source directory changed. Both the optimistic repeat-install short-circuit and the import completion-marker check treated an unchanged lockfile as evidence that a mutable-source slot was current. --- .changeset/directory-dep-recopy.md | 5 + pnpm/crates/cli/tests/hoisted_node_linker.rs | 43 ++ pnpm/crates/cli/tests/repeat_install.rs | 54 +++ .../src/create_virtual_dir_by_snapshot.rs | 12 +- .../create_virtual_dir_by_snapshot/tests.rs | 4 + .../deps-restorer/src/create_virtual_store.rs | 411 ++++-------------- .../src/create_virtual_store/partition.rs | 168 +++++++ .../src/create_virtual_store/snapshot_plan.rs | 170 ++++++++ .../src/install_frozen_lockfile.rs | 11 +- .../src/install_package_by_snapshot.rs | 33 +- .../src/install_package_by_snapshot/tests.rs | 55 ++- pnpm/crates/deps-restorer/src/linking.rs | 2 +- .../deps-restorer/src/materialization_plan.rs | 27 +- .../src/install/prepare_modules_state.rs | 20 + .../src/install_with_fresh_lockfile.rs | 6 +- 15 files changed, 661 insertions(+), 360 deletions(-) create mode 100644 .changeset/directory-dep-recopy.md create mode 100644 pnpm/crates/deps-restorer/src/create_virtual_store/partition.rs create mode 100644 pnpm/crates/deps-restorer/src/create_virtual_store/snapshot_plan.rs diff --git a/.changeset/directory-dep-recopy.md b/.changeset/directory-dep-recopy.md new file mode 100644 index 0000000000..ed345e0476 --- /dev/null +++ b/.changeset/directory-dep-recopy.md @@ -0,0 +1,5 @@ +--- +"pacquet": patch +--- + +Fixed `file:` dependencies not being re-copied when their source directory changed. A `file:` dependency is copied into the store at install time rather than symlinked, so editing the local package's files and running `pnpm install` again left the previous copy in place — the lockfile is unchanged by such an edit, so the install treated the tree as up to date. diff --git a/pnpm/crates/cli/tests/hoisted_node_linker.rs b/pnpm/crates/cli/tests/hoisted_node_linker.rs index d7478a5b96..f90b09a542 100644 --- a/pnpm/crates/cli/tests/hoisted_node_linker.rs +++ b/pnpm/crates/cli/tests/hoisted_node_linker.rs @@ -977,3 +977,46 @@ fn hoist_patterns_are_inert_under_the_hoisted_linker() { drop((root, mock_instance)); } + +/// The hoisted linker imports directly into the flat `node_modules/` +/// rather than into a virtual-store slot, so it needs its own guard +/// that a `file:` dependency's copy is retaken when the source moves. +#[test] +fn a_directory_dependency_is_recopied_under_the_hoisted_linker() { + let CommandTempCwd { pacquet, root, workspace, npmrc_info, .. } = + CommandTempCwd::init().add_mocked_registry(); + let AddMockedRegistry { mock_instance, .. } = npmrc_info; + write_workspace_yaml(&workspace, "nodeLinker: hoisted\n"); + + let local = workspace.join("local-pkg"); + fs::create_dir_all(&local).expect("create the local package dir"); + let write_local = |marker: &str| { + fs::write( + local.join("package.json"), + serde_json::json!({ "name": "local-pkg", "version": "1.0.0" }).to_string(), + ) + .expect("write the local package.json"); + fs::write(local.join("marker.txt"), marker).expect("write the marker"); + }; + write_local("first"); + write_manifest(&workspace, serde_json::json!({ "local-pkg": "file:./local-pkg" })); + + pacquet.with_arg("install").assert().success(); + let installed_marker = workspace.join("node_modules/local-pkg/marker.txt"); + assert_eq!( + fs::read_to_string(&installed_marker).expect("read the installed marker"), + "first", + "the first install should materialize the directory dependency", + ); + + write_local("second"); + pacquet_in(&workspace).with_arg("install").assert().success(); + + assert_eq!( + fs::read_to_string(&installed_marker).expect("read the installed marker"), + "second", + "the second install should re-copy the directory into node_modules", + ); + + drop((root, mock_instance)); +} diff --git a/pnpm/crates/cli/tests/repeat_install.rs b/pnpm/crates/cli/tests/repeat_install.rs index e82123e8b2..f5c4b6048b 100644 --- a/pnpm/crates/cli/tests/repeat_install.rs +++ b/pnpm/crates/cli/tests/repeat_install.rs @@ -306,3 +306,57 @@ fn available_packages_are_relinked_during_forced_install() { drop((root, mock_instance)); } + +/// pnpm's `file:` is a copy taken at install time, not a symlink, so +/// each install must re-copy: the source can change with no lockfile +/// change to signal it. +#[test] +fn a_directory_dependency_is_recopied_when_its_source_changes() { + let CommandTempCwd { pacquet, root, workspace, npmrc_info, .. } = + CommandTempCwd::init().add_mocked_registry(); + let AddMockedRegistry { mock_instance, .. } = npmrc_info; + + let local = workspace.join("local-pkg"); + fs::create_dir_all(&local).expect("create the local package dir"); + let write_local = |marker: &str| { + fs::write( + local.join("package.json"), + serde_json::json!({ "name": "local-pkg", "version": "1.0.0" }).to_string(), + ) + .expect("write the local package.json"); + fs::write(local.join("marker.txt"), marker).expect("write the marker"); + }; + write_local("first"); + + fs::write( + workspace.join("package.json"), + serde_json::json!({ + "name": "root", + "private": true, + "dependencies": { "local-pkg": "file:./local-pkg" }, + }) + .to_string(), + ) + .expect("write the root package.json"); + + pacquet.with_arg("install").assert().success(); + let linked_marker = workspace.join("node_modules/local-pkg/marker.txt"); + assert_eq!( + fs::read_to_string(&linked_marker).expect("read the linked marker"), + "first", + "the first install should materialize the directory dependency", + ); + + // The version is left alone so nothing in the lockfile changes and + // only a re-copy of the source can surface the edit. + write_local("second"); + pacquet_in(&workspace).with_arg("install").assert().success(); + + assert_eq!( + fs::read_to_string(&linked_marker).expect("read the linked marker"), + "second", + "the second install should re-copy the directory into its slot", + ); + + drop((root, mock_instance)); +} diff --git a/pnpm/crates/deps-restorer/src/create_virtual_dir_by_snapshot.rs b/pnpm/crates/deps-restorer/src/create_virtual_dir_by_snapshot.rs index f8bf2366e0..108d195c0b 100644 --- a/pnpm/crates/deps-restorer/src/create_virtual_dir_by_snapshot.rs +++ b/pnpm/crates/deps-restorer/src/create_virtual_dir_by_snapshot.rs @@ -54,6 +54,15 @@ pub struct CreateVirtualDirBySnapshot<'a> { pub package_id: &'a str, pub package_key: &'a PackageKey, pub snapshot: &'a SnapshotEntry, + /// Whether this package's file map points at mutable local source + /// (a `file:` / [`pacquet_lockfile::LockfileResolution::Directory`] + /// resolution) rather than immutable CAS entries. pnpm's `file:` is + /// a copy taken at install time — unlike `link:`, which symlinks — + /// so the slot has to be rebuilt on every install: the source can + /// change without the lockfile changing, and the completion-marker + /// short-circuit in [`fn@crate::import_indexed_dir`] would otherwise + /// leave the previous install's copy in place forever. + pub source_is_mutable: bool, /// Whether dependency links inside the slot should be created. /// `symlink: false` still imports the package itself but leaves its /// `node_modules` free of graph links for `PnP` resolution. @@ -122,6 +131,7 @@ impl CreateVirtualDirBySnapshot<'_> { package_id: _package_id, package_key, snapshot, + source_is_mutable, symlink, skipped, removed_aliases, @@ -163,7 +173,7 @@ impl CreateVirtualDirBySnapshot<'_> { } else { cas_paths }; - let import_opts = if interrupted_build { + let import_opts = if interrupted_build || source_is_mutable { ImportIndexedDirOpts { force: true, keep_modules_dir: true } } else { ImportIndexedDirOpts::default() diff --git a/pnpm/crates/deps-restorer/src/create_virtual_dir_by_snapshot/tests.rs b/pnpm/crates/deps-restorer/src/create_virtual_dir_by_snapshot/tests.rs index 4c03ec2ace..87d1b485a1 100644 --- a/pnpm/crates/deps-restorer/src/create_virtual_dir_by_snapshot/tests.rs +++ b/pnpm/crates/deps-restorer/src/create_virtual_dir_by_snapshot/tests.rs @@ -150,6 +150,7 @@ async fn run_emits_imported_event_after_import_indexed_dir() { package_id: "react@18.0.0", package_key: &package_key, snapshot: &snapshot, + source_is_mutable: false, symlink: true, skipped: &skipped, removed_aliases: &[], @@ -215,6 +216,7 @@ fn run_imports_needs_build_marker_with_a_fresh_package() { package_id: "react@18.0.0", package_key: &package_key, snapshot: &snapshot, + source_is_mutable: false, symlink: true, skipped: &skipped, removed_aliases: &[], @@ -256,6 +258,7 @@ fn run_rejects_traversal_package_name() { package_id: "../../escaped@1.0.0", package_key: &package_key, snapshot: &snapshot, + source_is_mutable: false, symlink: true, skipped: &skipped, removed_aliases: &[], @@ -307,6 +310,7 @@ async fn run_removes_obsolete_child_links() { package_id: "react@18.0.0", package_key: &package_key, snapshot: &snapshot, + source_is_mutable: false, symlink: true, skipped: &skipped, removed_aliases: &removed_aliases, diff --git a/pnpm/crates/deps-restorer/src/create_virtual_store.rs b/pnpm/crates/deps-restorer/src/create_virtual_store.rs index 04e1a5bfd3..2e2a507281 100644 --- a/pnpm/crates/deps-restorer/src/create_virtual_store.rs +++ b/pnpm/crates/deps-restorer/src/create_virtual_store.rs @@ -19,11 +19,10 @@ use pacquet_package_manifest::{ files_include_install_scripts, manifest_requires_build, parse_manifest, }; use pacquet_reporter::{ - BrokenModulesLog, LogEvent, LogLevel, ProgressLog, ProgressMessage, Reporter, StatsLog, - StatsMessage, + LogEvent, LogLevel, ProgressLog, ProgressMessage, Reporter, StatsLog, StatsMessage, }; use pacquet_store_dir::{SharedVerifiedFilesCache, StoreIndex, StoreIndexWriter, store_index_key}; -use pacquet_tarball::{MemCache, PrefetchResult, SharedReportedProgressKeys, prefetch_cas_paths}; +use pacquet_tarball::{MemCache, SharedReportedProgressKeys, prefetch_cas_paths}; use pipe_trait::Pipe; use std::{ collections::{HashMap, HashSet}, @@ -74,6 +73,11 @@ pub type SideEffectsMapsBySnapshot = /// avoid re-inspecting every package directory after materialization. pub type RequiresBuildBySnapshot = HashMap; +/// A snapshot paired with the store-index cache key it is looked up +/// by. `None` for a resolution that never goes through the CAFS +/// (directory and git), which therefore has no row to prefetch. +pub(crate) type SnapshotWithCacheKey<'a> = (&'a PackageKey, &'a SnapshotEntry, Option); + /// Output of [`CreateVirtualStore::run`]. Bundles the bin-link /// manifest cache, the per-snapshot side-effects-cache overlays the /// build-phase needs, and the per-install fetch-failure set. @@ -360,174 +364,21 @@ impl CreateVirtualStore<'_> { // Sort + dedup the prefetch input so `prefetch_cas_paths` // doesn't redo identical SELECT + integrity-check work for // every peer variant. - // Per-snapshot skip pass: drop snapshots that don't need - // installing. - // - // Two reasons a snapshot can be dropped from the install graph: - // - // 1. **Installability skip (this PR)** — `SkippedSnapshots` - // contains it because the host's `engines` / `cpu` / `os` - // / `libc` don't satisfy the package's constraints and the - // snapshot is `optional`. A skipped depPath is dropped from - // the install graph. These snapshots also stay out of the - // `skipped_entries` cache-key pass — they were never - // supposed to be installed, so there are no store-index - // rows to keep alive. - // - // 2. **Current-lockfile skip (main )** — the previous - // install also installed this snapshot (`current_snapshots`) - // with the same dependency wiring + integrity, AND its - // virtual-store slot still exists on disk. These DO land in - // `skipped_entries` so `BuildModules`'s `is_built` cache - // lookup can short-circuit re-runs of allowed-build scripts - // on warm reinstalls. - // - // Run this *before* deriving cache keys so unchanged - // directory-backed snapshots aren't tripped by - // `snapshot_cache_key`'s `UnsupportedResolution`. - // - // Route the slot-existence probe through `layout.slot_dir` so - // GVS-on installs check the correct path. Under GVS, slots live - // at `////`, - // not `/`; probing the - // latter would find an empty path, so the skip gate would - // incorrectly mark every warm slot as "broken" and emit - // `BrokenModules` for the wrong path. - let mut marker_probe_keys = HashSet::new(); - let mut marker_rebuilds = HashSet::new(); - let survivors = snapshots - .iter() - // Reason 1: installability skip. Drop entirely. - .filter(|(snapshot_key, _)| !skipped.contains(snapshot_key)) - // Reason 2: current-lockfile skip. Drop survivors that - // already match the previous install. - .filter(|(snapshot_key, snapshot)| { - let Some(current_snapshots) = current_snapshots else { return true }; - let Some(current_snapshot) = current_snapshots.get(*snapshot_key) else { - return true; - }; - let wanted_metadata = packages.get(&snapshot_key.without_peer()); - // Directory-typed snapshots carry mutable local - // source: the user can edit `file:./local-pkg` files - // between installs and pacquet must re-walk them on - // every install, otherwise the slot drifts. Directory - // snapshots are forced through the cold path for this - // reason. Without this carve-out - // both `current` and `wanted` resolutions report - // `integrity() == None`, `integrity_equal` returns - // true, the slot directory check passes, and the - // directory-fetcher never runs on the second install. - if matches!( - wanted_metadata.map(|meta| &meta.resolution), - Some(LockfileResolution::Directory(_)), - ) { - return true; - } - if !snapshot_deps_equal(current_snapshot, snapshot) { - return true; - } - let current_metadata = - current_packages.and_then(|p| p.get(&snapshot_key.without_peer())); - if !integrity_equal(current_metadata, wanted_metadata) { - return true; - } - let dir = layout - .slot_dir(snapshot_key) - .join("node_modules") - .join(snapshot_key.name.to_string()); - if dir.is_dir() { - let needs_rebuild = - gvs_slot_needs_rebuild(layout, allow_build_policy, snapshot_key); - marker_probe_keys.insert((*snapshot_key).clone()); - if needs_rebuild { - marker_rebuilds.insert((*snapshot_key).clone()); - } - needs_rebuild - } else { - Reporter::emit(&LogEvent::BrokenModules(BrokenModulesLog { - level: LogLevel::Debug, - missing: dir.to_string_lossy().into_owned(), - })); - true - } - }); - // Validate every surviving snapshot upfront so a malformed - // lockfile (missing metadata, missing tarball integrity, - // currently-unsupported directory / git resolution) errors - // out *before* we start the warm batch — otherwise the warm - // rayon batch runs to completion (~6 s on `alot7`) before the - // actual error fires. - // - // Cache-key derivation runs in two passes: - // - // - *Survivors* go through the strict path (this `?`). Their - // resolutions have to be valid because the install will - // actually fetch + link them. - // - *Skipped* snapshots get a lenient pass below: cache keys - // are derived if possible, and any per-snapshot error is - // swallowed. Reason: skipped snapshots aren't being - // re-installed, but their store-index rows still need to - // land in `side_effects_maps_by_snapshot` so - // [`crate::BuildModules`]'s `is_built` gate can skip - // re-running build scripts on warm reinstalls (review on - // — without this, allowed-build packages re-execute - // their scripts every install, costing seconds on the - // warm-reinstall path). - type SnapshotWithCacheKey<'a> = (&'a PackageKey, &'a SnapshotEntry, Option); - let snapshot_entries: Vec> = survivors - .map(|(snapshot_key, snapshot)| { - snapshot_cache_key( - snapshot_key, - packages, - config.ignore_scripts, - &runtime_platform_selector, - ) - .map(|key| (snapshot_key, snapshot, key)) - }) - .collect::>()?; - marker_rebuilds.extend( - snapshot_entries - .iter() - .filter(|(snapshot_key, _, _)| !marker_probe_keys.contains(*snapshot_key)) - .filter(|(snapshot_key, _, _)| { - gvs_slot_needs_rebuild(layout, allow_build_policy, snapshot_key) - }) - .map(|(snapshot_key, _, _)| (*snapshot_key).clone()), - ); - - // Cache keys for the *skipped* snapshots (i.e. snapshots - // present in `snapshots` but absent from `snapshot_entries`). - // Derived leniently so an unsupported / malformed skipped - // entry doesn't fail the install — it just contributes no - // prefetch row, which is the same outcome as if the skip - // filter had not engaged. Built as a parallel `Vec` so the - // downstream `package_manifests` / - // `side_effects_maps_by_snapshot` loop sees the full snapshot - // set, not just survivors. - let survivor_keys: std::collections::HashSet<&PackageKey> = - snapshot_entries.iter().map(|(k, _, _)| *k).collect(); - let skipped_entries: Vec> = snapshots - .iter() - .filter(|(snapshot_key, _)| !survivor_keys.contains(snapshot_key)) - // Installability-skipped snapshots are excluded from - // `skipped_entries` too — they were never installed, so - // there's no store-index row to keep warm for the - // build-cache lookup. Only the current-lockfile-skip - // path (`snapshot_entries` filtered above) should contribute - // here. - .filter(|(snapshot_key, _)| !skipped.contains(snapshot_key)) - .map(|(snapshot_key, snapshot)| { - let cache_key = snapshot_cache_key( - snapshot_key, - packages, - config.ignore_scripts, - &runtime_platform_selector, - ) - .ok() - .flatten(); - (snapshot_key, snapshot, cache_key) - }) - .collect(); + let snapshot_plan::SnapshotPlan { + survivors: snapshot_entries, + skipped_entries, + marker_rebuilds, + } = snapshot_plan::plan_snapshots::(&snapshot_plan::SnapshotPlanInputs { + snapshots, + packages, + current_snapshots, + current_packages, + layout, + allow_build_policy, + skipped, + ignore_scripts: config.ignore_scripts, + runtime_platform_selector: &runtime_platform_selector, + })?; // `pnpm:stats added` fires one event per project once the // orchestrator has decided how many packages will land in the @@ -559,12 +410,7 @@ impl CreateVirtualStore<'_> { cache_key_refs.sort_unstable(); cache_key_refs.dedup(); let cache_keys: Vec = cache_key_refs.into_iter().map(String::from).collect(); - let PrefetchResult { - cas_paths: prefetched, - manifests: prefetched_manifests, - side_effects_maps: prefetched_side_effects, - requires_build: prefetched_requires_build, - } = prefetch_cas_paths( + let prefetch = prefetch_cas_paths( store_index.clone(), store_dir, cache_keys, @@ -572,137 +418,18 @@ impl CreateVirtualStore<'_> { SharedVerifiedFilesCache::clone(&verified_files_cache), ) .await; - - // Partition snapshots by whether the prefetch covered them. The - // warm batch — every snapshot whose tarball is already in the - // CAFS — runs entirely on rayon: no tokio futures, no - // `try_join_all` polling overhead, no `spawn_blocking` round-trip - // per snapshot. The cold batch (cache miss → download needed) - // keeps the existing `try_join_all` + download path. - // - // **Why this beats per-snapshot tokio futures:** profiling at - // 1352 prefetched / 0 cold on a 10-core Mac showed `sum-of-link - // ≈ wall` (~10 s sum on a 10 s wall, i.e. effectively 1× - // parallelism) even though `try_join_all` was meant to fan - // futures across tokio's 10 worker threads. Each future's sync - // `rayon::join` pinned one tokio worker; with up to 10 such - // futures progressing concurrently, each one's inner par_iter - // saturated rayon's pool, and the pool ended up processing one - // snapshot at a time. Going straight to rayon via a single - // `par_iter` lets the pool schedule across all 1352 snapshots - // as one work-stealing graph — the shape pnpm's piscina pool - // gives implicitly. On the same benchmark, wall dropped from - // ~10 s to ~6.5 s. - // - // The `par_iter` blocks the calling thread for the duration of - // the warm batch. The cold-batch fetches run *after* this - // returns; that ordering is intentional — warm-cache work has - // no network dependency, so we'd be racing a cold download - // against a CPU/syscall-bound rayon batch for nothing. - // Element types are inferred from the push calls below — no - // explicit alias, so the warm tuple's third field stays bound - // to whatever value type `pacquet_tarball::PrefetchedCasPaths` - // exposes. A future change there propagates here without a - // local alias drifting (Copilot review on ). - let mut warm = Vec::with_capacity(snapshot_entries.len()); - let mut cold: Vec<(&PackageKey, &SnapshotEntry)> = Vec::new(); - // Build a `metadata_key -> manifest` lookup from the prefetched - // index rows. Snapshot keys differ across peer-resolved - // variants of the same package (`react-dom@17.0.2(react@...)`), - // but the bundled manifest is identical across variants - // because every variant resolves to the same tarball. Keying - // by [`PkgNameVerPeer::without_peer`] collapses the variants - // to one entry: same shape as - // [`pacquet_lockfile::Lockfile::packages`], which is what the - // bin linker already looks up by. - let mut package_manifests: PackageManifests = - HashMap::with_capacity(prefetched_manifests.len()); - let mut side_effects_maps_by_snapshot: SideEffectsMapsBySnapshot = - HashMap::with_capacity(prefetched_side_effects.len()); - let mut requires_build_by_snapshot: RequiresBuildBySnapshot = - HashMap::with_capacity(prefetched_requires_build.len()); - - // First pass: process *skipped* snapshots into the bin- - // manifest cache and the side-effects map. They don't enter - // the warm/cold partition (no link work to do), but their - // store-index rows are needed downstream so - // [`crate::BuildModules`]'s `is_built` gate can fire — without - // these entries, packages with `allowBuilds: true` would - // re-execute their lifecycle scripts on every warm reinstall. - for (snapshot_key, _snapshot, cache_key) in &skipped_entries { - if let Some(cache_key) = cache_key.as_deref() - && let Some(manifest) = prefetched_manifests.get(cache_key) - { - package_manifests - .entry(snapshot_key.without_peer()) - .or_insert_with(|| std::sync::Arc::clone(manifest)); - } - if !marker_rebuilds.contains(*snapshot_key) - && let Some(cache_key) = cache_key.as_deref() - && let Some(maps) = prefetched_side_effects.get(cache_key) - { - side_effects_maps_by_snapshot - .insert((*snapshot_key).clone(), std::sync::Arc::clone(maps)); - } - if let Some(cache_key) = cache_key.as_deref() - && let Some(&requires_build) = prefetched_requires_build.get(cache_key) - { - requires_build_by_snapshot.insert((*snapshot_key).clone(), requires_build); - } - } - - // Second pass: survivors. Same loop as above plus the - // warm/cold partition that decides which snapshots run the - // link work. - for (snapshot_key, snapshot, cache_key) in &snapshot_entries { - if let Some(cache_key) = cache_key.as_deref() - && let Some(manifest) = prefetched_manifests.get(cache_key) - { - package_manifests - .entry(snapshot_key.without_peer()) - .or_insert_with(|| std::sync::Arc::clone(manifest)); - } - // Peer-variants of the same package share the same - // store-index row → the same `Arc<_>`. Cheap to share. - if !marker_rebuilds.contains(*snapshot_key) - && let Some(cache_key) = cache_key.as_deref() - && let Some(maps) = prefetched_side_effects.get(cache_key) - { - side_effects_maps_by_snapshot - .insert((*snapshot_key).clone(), std::sync::Arc::clone(maps)); - } - if let Some(cache_key) = cache_key.as_deref() - && let Some(&requires_build) = prefetched_requires_build.get(cache_key) - { - requires_build_by_snapshot.insert((*snapshot_key).clone(), requires_build); - } - // Carry the cache key alongside the warm entry so the - // reporter can skip a duplicate package-status event when - // a resolve-time prefetch already emitted it. - match cache_key.as_deref().and_then(|key| prefetched.get(key).map(|paths| (key, paths))) - { - Some((key, cas_paths)) => warm.push(( - snapshot_key, - snapshot, - cas_paths, - key, - snapshot_needs_build_marker( - snapshot_key, - requires_build_by_snapshot.get(*snapshot_key).copied().unwrap_or(false), - ), - )), - None => cold.push((snapshot_key, snapshot)), - } - } - tracing::info!( - target: "pacquet::install::phase", - phase = "create_virtual_store_partition", - warm = warm.len(), - cold = cold.len(), - skipped = skipped_entries.len(), - total = snapshot_entries.len(), - node_linker = ?node_linker, - "phase complete", + let partition::Partition { + warm, + cold, + package_manifests, + side_effects_maps_by_snapshot, + mut requires_build_by_snapshot, + } = partition::partition_snapshots( + &snapshot_entries, + &skipped_entries, + &prefetch, + &marker_rebuilds, + node_linker, ); // Hoisted-mode CAS index assembly. Collected here, *before* @@ -773,6 +500,11 @@ impl CreateVirtualStore<'_> { snapshot, cas_paths: cas_paths.as_ref(), warm_cache_key: Some(cache_key), + // A cache key means the file map is CAS-backed, + // and `snapshot_cache_key` yields none for a + // directory resolution, so a warm slot's source + // is immutable by construction. + source_is_mutable: false, needs_build_marker_source: needs_build_marker .then_some( needs_build_marker_source @@ -819,16 +551,12 @@ impl CreateVirtualStore<'_> { // future returns; under hoisted no slot was written and the // CAS index is the only output. let mut fetch_failed: HashSet = HashSet::new(); - let mut cold_cas_paths: Vec<(&PackageKey, &SnapshotEntry, HashMap, bool)> = - Vec::new(); + let mut cold_cas_paths: Vec> = Vec::new(); if !cold.is_empty() { - let prefetched_ref = Some(&prefetched); + let prefetched_ref = Some(&prefetch.cas_paths); let verified_files_cache_ref = &verified_files_cache; let runtime_platform_selector_ref = &runtime_platform_selector; - type ColdOutcome<'a> = ( - Option, - Option<(&'a PackageKey, &'a SnapshotEntry, HashMap, bool)>, - ); + type ColdOutcome<'a> = (Option, Option>); let outcomes: Vec> = cold .iter() .map(|(snapshot_key, snapshot)| async move { @@ -870,9 +598,20 @@ impl CreateVirtualStore<'_> { .run::() .await; match result { - Ok(cas_paths) => { + Ok(installed) => { + let crate::InstalledPackage { cas_paths, source_is_mutable } = + installed; let requires_build = requires_build_from_cas_paths(&cas_paths); - Ok((None, Some((*snapshot_key, *snapshot, cas_paths, requires_build)))) + Ok(( + None, + Some(ColdCapture { + snapshot_key, + snapshot, + cas_paths, + requires_build, + source_is_mutable, + }), + )) } Err(err) if snapshot.optional && is_fetch_side_failure(&err) => { // Silent swallow. `tracing::warn!` gives @@ -910,7 +649,8 @@ impl CreateVirtualStore<'_> { fetch_failed.insert(key); } if let Some(captured) = captured { - requires_build_by_snapshot.insert((*captured.0).clone(), captured.3); + requires_build_by_snapshot + .insert((*captured.snapshot_key).clone(), captured.requires_build); cold_cas_paths.push(captured); } } @@ -928,20 +668,24 @@ impl CreateVirtualStore<'_> { if !is_hoisted && !cold_cas_paths.is_empty() { let cold_slots: Vec> = cold_cas_paths .iter() - .map(|(snapshot_key, snapshot, cas_paths, requires_build)| SlotLink { - snapshot_key, - snapshot, - cas_paths, + .map(|capture| SlotLink { + snapshot_key: capture.snapshot_key, + snapshot: capture.snapshot, + cas_paths: &capture.cas_paths, warm_cache_key: None, + source_is_mutable: capture.source_is_mutable, needs_build_marker_source: snapshot_needs_build_marker( - snapshot_key, - *requires_build, + capture.snapshot_key, + capture.requires_build, ) .then_some( needs_build_marker_source.as_ref().map(tempfile::NamedTempFile::path), ) .flatten(), - removed_aliases: removed_aliases_for(&removed_aliases_by_key, snapshot_key), + removed_aliases: removed_aliases_for( + &removed_aliases_by_key, + capture.snapshot_key, + ), }) .collect(); link_slots_parallel::(LinkSlotsParallel { @@ -986,7 +730,7 @@ impl CreateVirtualStore<'_> { // real install. if let Some(map) = cas_paths_by_pkg_id.as_mut() { map.reserve(cold_cas_paths.len()); - for (snapshot_key, _snapshot, paths, _requires_build) in cold_cas_paths { + for ColdCapture { snapshot_key, cas_paths: paths, .. } in cold_cas_paths { // `get_pkg_id_with_patch_hash` strips the peer-graph // suffix but keeps `(patch_hash=...)` so patched // packages share one CAS-paths entry across their peer @@ -1089,11 +833,22 @@ fn gvs_slot_needs_rebuild( .is_file() } +/// A cold snapshot whose CAS paths are staged and whose slot link was +/// deferred to [`link_slots_parallel`]. +struct ColdCapture<'a> { + snapshot_key: &'a PackageKey, + snapshot: &'a SnapshotEntry, + cas_paths: HashMap, + requires_build: bool, + source_is_mutable: bool, +} + struct SlotLink<'a> { snapshot_key: &'a PackageKey, snapshot: &'a SnapshotEntry, cas_paths: &'a HashMap, warm_cache_key: Option<&'a str>, + source_is_mutable: bool, needs_build_marker_source: Option<&'a Path>, /// Child aliases dropped since the previous install, threaded into /// [`crate::CreateVirtualDirBySnapshot::removed_aliases`] so their @@ -1157,6 +912,7 @@ fn link_slots_parallel( package_id: &package_id, package_key: slot.snapshot_key, snapshot: slot.snapshot, + source_is_mutable: slot.source_is_mutable, symlink, skipped, removed_aliases: slot.removed_aliases, @@ -1402,5 +1158,8 @@ fn emit_warm_snapshot_progress( } } +mod partition; +mod snapshot_plan; + #[cfg(test)] mod tests; diff --git a/pnpm/crates/deps-restorer/src/create_virtual_store/partition.rs b/pnpm/crates/deps-restorer/src/create_virtual_store/partition.rs new file mode 100644 index 0000000000..161857c48b --- /dev/null +++ b/pnpm/crates/deps-restorer/src/create_virtual_store/partition.rs @@ -0,0 +1,168 @@ +//! Split the snapshots this install materializes into the warm batch — +//! already in the CAFS, so the prefetch covered them — and the cold +//! batch that still needs downloading, and collect the store-index rows +//! later phases read. +//! +//! Runs immediately after the prefetch, whose results it consumes. + +use super::{ + PackageManifests, RequiresBuildBySnapshot, SideEffectsMapsBySnapshot, SnapshotWithCacheKey, + snapshot_needs_build_marker, +}; +use pacquet_config::NodeLinker; +use pacquet_lockfile::{PackageKey, SnapshotEntry}; +use pacquet_tarball::PrefetchResult; +use std::collections::{HashMap, HashSet}; + +/// One warm entry: the snapshot, its prefetched CAS paths, the cache key +/// that found them, and whether its slot needs a build marker. +pub(super) type WarmEntry<'a> = ( + &'a PackageKey, + &'a SnapshotEntry, + &'a std::sync::Arc>, + &'a str, + bool, +); + +pub(super) struct Partition<'a> { + pub warm: Vec>, + pub cold: Vec<(&'a PackageKey, &'a SnapshotEntry)>, + /// Bundled manifests recovered from the store index, so the bin + /// linker need not re-read each child's `package.json`. + pub package_manifests: PackageManifests, + pub side_effects_maps_by_snapshot: SideEffectsMapsBySnapshot, + pub requires_build_by_snapshot: RequiresBuildBySnapshot, +} + +/// Assign every snapshot to the warm or cold batch, and fold the +/// prefetched store-index rows into the maps the build and bin phases +/// read. +/// +/// Skipped snapshots are walked too, and contribute their rows without +/// entering either batch: they have no link work, but omitting their +/// side-effects entries would make the build phase re-run approved +/// scripts on every warm reinstall. +/// +/// `marker_rebuilds` withholds the side-effects row for a slot whose +/// global-virtual-store build marker says it must be rebuilt — keeping +/// the row would let the `is_built` gate skip the very build the marker +/// is asking for. +pub(super) fn partition_snapshots<'a>( + snapshot_entries: &'a [SnapshotWithCacheKey<'a>], + skipped_entries: &'a [SnapshotWithCacheKey<'a>], + prefetch: &'a PrefetchResult, + marker_rebuilds: &HashSet, + node_linker: NodeLinker, +) -> Partition<'a> { + let PrefetchResult { cas_paths: prefetched, .. } = prefetch; + + // The warm batch runs on rayon rather than per-snapshot tokio + // futures. Profiled at 1352 prefetched / 0 cold on a 10-core Mac: + // each future's sync `rayon::join` pinned a tokio worker and + // saturated the pool, so `sum-of-link ~= wall` — effectively 1x + // parallelism. One `par_iter` over every snapshot lets the pool + // work-steal across all of them; wall dropped ~10 s to ~6.5 s. + // + // Cold fetches deliberately run after this returns rather than + // alongside: racing a network download against a CPU-bound rayon + // batch buys nothing. + let mut warm = Vec::with_capacity(snapshot_entries.len()); + let mut cold: Vec<(&PackageKey, &SnapshotEntry)> = Vec::new(); + // Keyed peer-stripped: every peer variant of a package resolves to + // the same tarball, so they share one bundled manifest, and this is + // the shape the bin linker looks up by. + let mut rows = IndexRows::with_capacity_for(prefetch); + + for (snapshot_key, _snapshot, cache_key) in skipped_entries { + rows.absorb(snapshot_key, cache_key.as_deref(), prefetch, marker_rebuilds); + } + + // Second pass: survivors, which additionally take the warm/cold + // partition that decides which snapshots run the link work. + for (snapshot_key, snapshot, cache_key) in snapshot_entries { + rows.absorb(snapshot_key, cache_key.as_deref(), prefetch, marker_rebuilds); + // Carry the cache key alongside the warm entry so the + // reporter can skip a duplicate package-status event when + // a resolve-time prefetch already emitted it. + match cache_key.as_deref().and_then(|key| prefetched.get(key).map(|paths| (key, paths))) { + Some((key, cas_paths)) => warm.push(( + *snapshot_key, + *snapshot, + cas_paths, + key, + snapshot_needs_build_marker( + snapshot_key, + rows.requires_build_by_snapshot.get(*snapshot_key).copied().unwrap_or(false), + ), + )), + None => cold.push((*snapshot_key, *snapshot)), + } + } + tracing::info!( + target: "pacquet::install::phase", + phase = "create_virtual_store_partition", + warm = warm.len(), + cold = cold.len(), + skipped = skipped_entries.len(), + total = snapshot_entries.len(), + node_linker = ?node_linker, + "phase complete", + ); + let IndexRows { package_manifests, side_effects_maps_by_snapshot, requires_build_by_snapshot } = + rows; + Partition { + warm, + cold, + package_manifests, + side_effects_maps_by_snapshot, + requires_build_by_snapshot, + } +} + +/// The store-index rows the build and bin phases read, accumulated +/// across both partition passes. +struct IndexRows { + package_manifests: PackageManifests, + side_effects_maps_by_snapshot: SideEffectsMapsBySnapshot, + requires_build_by_snapshot: RequiresBuildBySnapshot, +} + +impl IndexRows { + fn with_capacity_for(prefetch: &PrefetchResult) -> Self { + IndexRows { + package_manifests: HashMap::with_capacity(prefetch.manifests.len()), + side_effects_maps_by_snapshot: HashMap::with_capacity(prefetch.side_effects_maps.len()), + requires_build_by_snapshot: HashMap::with_capacity(prefetch.requires_build.len()), + } + } + + /// Fold one snapshot's prefetched rows in. + /// + /// Both passes share this so a skipped snapshot and a survivor can + /// never diverge in what they contribute. + fn absorb( + &mut self, + snapshot_key: &PackageKey, + cache_key: Option<&str>, + prefetch: &PrefetchResult, + marker_rebuilds: &HashSet, + ) { + let Some(cache_key) = cache_key else { return }; + if let Some(manifest) = prefetch.manifests.get(cache_key) { + self.package_manifests + .entry(snapshot_key.without_peer()) + .or_insert_with(|| std::sync::Arc::clone(manifest)); + } + // Peer-variants of the same package share the same store-index + // row → the same `Arc<_>`. Cheap to share. + if !marker_rebuilds.contains(snapshot_key) + && let Some(maps) = prefetch.side_effects_maps.get(cache_key) + { + self.side_effects_maps_by_snapshot + .insert(snapshot_key.clone(), std::sync::Arc::clone(maps)); + } + if let Some(&requires_build) = prefetch.requires_build.get(cache_key) { + self.requires_build_by_snapshot.insert(snapshot_key.clone(), requires_build); + } + } +} diff --git a/pnpm/crates/deps-restorer/src/create_virtual_store/snapshot_plan.rs b/pnpm/crates/deps-restorer/src/create_virtual_store/snapshot_plan.rs new file mode 100644 index 0000000000..5ee8ac7588 --- /dev/null +++ b/pnpm/crates/deps-restorer/src/create_virtual_store/snapshot_plan.rs @@ -0,0 +1,170 @@ +//! Decide which snapshots this install must materialize, and derive the +//! store-index cache key each one is looked up by. +//! +//! Runs before the prefetch, whose input is the cache keys produced +//! here. + +use super::{ + CreateVirtualStoreError, SnapshotWithCacheKey, gvs_slot_needs_rebuild, integrity_equal, + snapshot_cache_key, snapshot_deps_equal, +}; +use crate::{SkippedSnapshots, VirtualStoreLayout}; +use pacquet_lockfile::{ + LockfileResolution, PackageKey, PackageMetadata, PlatformSelector, SnapshotEntry, +}; +use pacquet_reporter::{BrokenModulesLog, LogEvent, LogLevel, Reporter}; +use std::collections::{HashMap, HashSet}; + +pub(super) struct SnapshotPlanInputs<'a> { + pub snapshots: &'a HashMap, + pub packages: &'a HashMap, + /// What the previous install materialized. `None` on a first + /// install; when present, a snapshot whose wiring and integrity are + /// unchanged *and* whose slot is still on disk is left alone. + pub current_snapshots: Option<&'a HashMap>, + pub current_packages: Option<&'a HashMap>, + pub layout: &'a VirtualStoreLayout, + pub allow_build_policy: &'a crate::AllowBuildPolicy, + /// Snapshots the installability pass ruled out on this host. + pub skipped: &'a SkippedSnapshots, + pub ignore_scripts: bool, + pub runtime_platform_selector: &'a PlatformSelector, +} + +/// The snapshots to install, the ones deliberately left alone, and the +/// slots whose global-virtual-store build marker forces a rebuild. +pub(super) struct SnapshotPlan<'a> { + /// Snapshots this install materializes. + pub survivors: Vec>, + /// Snapshots the current-lockfile check skipped. They contribute no + /// link work, but their store-index rows still feed the build + /// phase's `is_built` gate, so a warm reinstall does not re-run + /// approved build scripts. + pub skipped_entries: Vec>, + pub marker_rebuilds: HashSet, +} + +/// Partition the lockfile's snapshots into what this install must do +/// and what it may leave alone. +/// +/// Validation is deliberately asymmetric: survivors go through the +/// strict cache-key derivation, because the install will actually fetch +/// and link them, so a malformed resolution must fail before the warm +/// batch starts rather than several seconds into it. Skipped snapshots +/// get a lenient pass — they are not being installed, and swallowing a +/// per-snapshot error there costs only a prefetch row. +pub(super) fn plan_snapshots<'a, Reporter: self::Reporter>( + inputs: &SnapshotPlanInputs<'a>, +) -> Result, CreateVirtualStoreError> { + let &SnapshotPlanInputs { + snapshots, + packages, + current_snapshots, + current_packages, + layout, + allow_build_policy, + skipped, + ignore_scripts, + runtime_platform_selector, + } = inputs; + + // The slot probe goes through `layout.slot_dir` because under GVS + // the slot lives at `/...`, and probing + // `/` would find nothing and report + // every warm slot as broken. See pnpm/pacquet#442 for why the + // current-lockfile skip keeps its store-index rows. + let mut marker_probe_keys = HashSet::new(); + let mut marker_rebuilds = HashSet::new(); + let survivors = snapshots + .iter() + // Reason 1: installability skip. Drop entirely. + .filter(|(snapshot_key, _)| !skipped.contains(snapshot_key)) + // Reason 2: current-lockfile skip. Drop survivors that + // already match the previous install. + .filter(|(snapshot_key, snapshot)| { + let Some(current_snapshots) = current_snapshots else { return true }; + let Some(current_snapshot) = current_snapshots.get(*snapshot_key) else { + return true; + }; + let wanted_metadata = packages.get(&snapshot_key.without_peer()); + // A `file:` dependency's source is mutable, so an + // unchanged lockfile is no evidence its slot is current. + if matches!( + wanted_metadata.map(|meta| &meta.resolution), + Some(LockfileResolution::Directory(_)), + ) { + return true; + } + if !snapshot_deps_equal(current_snapshot, snapshot) { + return true; + } + let current_metadata = + current_packages.and_then(|p| p.get(&snapshot_key.without_peer())); + if !integrity_equal(current_metadata, wanted_metadata) { + return true; + } + let dir = layout + .slot_dir(snapshot_key) + .join("node_modules") + .join(snapshot_key.name.to_string()); + if dir.is_dir() { + let needs_rebuild = + gvs_slot_needs_rebuild(layout, allow_build_policy, snapshot_key); + marker_probe_keys.insert((*snapshot_key).clone()); + if needs_rebuild { + marker_rebuilds.insert((*snapshot_key).clone()); + } + needs_rebuild + } else { + Reporter::emit(&LogEvent::BrokenModules(BrokenModulesLog { + level: LogLevel::Debug, + missing: dir.to_string_lossy().into_owned(), + })); + true + } + }); + let snapshot_entries: Vec> = survivors + .map(|(snapshot_key, snapshot)| { + snapshot_cache_key(snapshot_key, packages, ignore_scripts, runtime_platform_selector) + .map(|key| (snapshot_key, snapshot, key)) + }) + .collect::>()?; + marker_rebuilds.extend( + snapshot_entries + .iter() + .filter(|(snapshot_key, _, _)| !marker_probe_keys.contains(*snapshot_key)) + .filter(|(snapshot_key, _, _)| { + gvs_slot_needs_rebuild(layout, allow_build_policy, snapshot_key) + }) + .map(|(snapshot_key, _, _)| (*snapshot_key).clone()), + ); + + // A parallel `Vec` rather than a filter later: the partition's + // manifest and side-effects loop has to see the full snapshot set, + // not just survivors. + let survivor_keys: std::collections::HashSet<&PackageKey> = + snapshot_entries.iter().map(|(k, _, _)| *k).collect(); + let skipped_entries: Vec> = snapshots + .iter() + .filter(|(snapshot_key, _)| !survivor_keys.contains(snapshot_key)) + // Installability-skipped snapshots are excluded from + // `skipped_entries` too — they were never installed, so + // there's no store-index row to keep warm for the + // build-cache lookup. Only the current-lockfile-skip + // path (`snapshot_entries` filtered above) should contribute + // here. + .filter(|(snapshot_key, _)| !skipped.contains(snapshot_key)) + .map(|(snapshot_key, snapshot)| { + let cache_key = snapshot_cache_key( + snapshot_key, + packages, + ignore_scripts, + runtime_platform_selector, + ) + .ok() + .flatten(); + (snapshot_key, snapshot, cache_key) + }) + .collect(); + Ok(SnapshotPlan { survivors: snapshot_entries, skipped_entries, marker_rebuilds }) +} diff --git a/pnpm/crates/deps-restorer/src/install_frozen_lockfile.rs b/pnpm/crates/deps-restorer/src/install_frozen_lockfile.rs index fd8bbf15df..28bf6d8ffa 100644 --- a/pnpm/crates/deps-restorer/src/install_frozen_lockfile.rs +++ b/pnpm/crates/deps-restorer/src/install_frozen_lockfile.rs @@ -717,11 +717,8 @@ where supported_architectures, ) .await; - // Keep `node_detected` + the version for the engine-name - // derivation below; the rest of the host is dropped here. - let host_node = installability_host - .as_ref() - .map(|host| (host.node_detected, host.node_version.clone())); + let host_node = + installability_host.as_ref().map(crate::materialization_plan::HostNode::from); let closure_importer_ids: std::collections::HashSet = importers.keys().cloned().collect(); @@ -1202,7 +1199,7 @@ pub struct HoistedLinkerInputs<'a> { /// `(node_detected, node_version)` from the installability host /// probe. `None` when no installability check ran (the fresh /// path, and constraint-free frozen lockfiles). - pub host_node: Option<&'a (bool, String)>, + pub host_node: Option<&'a crate::materialization_plan::HostNode>, pub supported_architectures: Option<&'a pacquet_package_is_installable::SupportedArchitectures>, /// Per-package CAS index produced by [`crate::CreateVirtualStore`] /// under `node_linker == Hoisted`. The linker imports files from @@ -1317,7 +1314,7 @@ pub fn run_hoisted_linker( // mismatch on a required package is a hard error under strict, // otherwise a skip-optional / warning. engine_strict: config.engine_strict, - current_node_version: host_node.map(|(_, ver)| ver.clone()).unwrap_or_default(), + current_node_version: host_node.map(|host| host.version.clone()).unwrap_or_default(), current_os: pacquet_graph_hasher::host_platform().to_string(), current_cpu: pacquet_graph_hasher::host_arch().to_string(), current_libc: pacquet_graph_hasher::host_libc().to_string(), diff --git a/pnpm/crates/deps-restorer/src/install_package_by_snapshot.rs b/pnpm/crates/deps-restorer/src/install_package_by_snapshot.rs index 6440b182d8..e59483b03c 100644 --- a/pnpm/crates/deps-restorer/src/install_package_by_snapshot.rs +++ b/pnpm/crates/deps-restorer/src/install_package_by_snapshot.rs @@ -262,12 +262,25 @@ pub enum InstallPackageBySnapshotError { }, } +/// What installing one package produced. +#[derive(Debug)] +pub struct InstalledPackage { + pub cas_paths: HashMap, + /// Whether [`Self::cas_paths`] points at mutable local source + /// rather than immutable content-addressed entries. See + /// [`crate::CreateVirtualDirBySnapshot::source_is_mutable`]. + pub source_is_mutable: bool, +} + impl InstallPackageBySnapshot<'_> { - /// Execute the subroutine. Returns the CAS file index for the - /// fetched package — the map relative-archive-path → - /// absolute-store-path that downstream consumers use to either - /// populate a virtual-store slot (isolated) or import into a - /// hoisted `node_modules//` directly (hoisted). + /// Execute the subroutine. Returns the fetched package's CAS file + /// index — the map relative-archive-path → absolute-store-path + /// that downstream consumers use to either populate a + /// virtual-store slot (isolated) or import into a hoisted + /// `node_modules//` directly (hoisted) — together with + /// whether that map points at mutable local source, which only + /// this function can tell because a custom fetcher's `delegate` + /// can replace the lockfile's resolution. /// /// Under [`NodeLinker::Isolated`] the slot has already been /// materialized by the time this returns (via @@ -279,7 +292,7 @@ impl InstallPackageBySnapshot<'_> { /// gets, and it's threaded into [`crate::link_hoisted_modules()`]. pub async fn run( self, - ) -> Result, InstallPackageBySnapshotError> { + ) -> Result { let InstallPackageBySnapshot { http_client, config, @@ -374,6 +387,11 @@ impl InstallPackageBySnapshot<'_> { None }; let resolution = effective_resolution.as_ref().unwrap_or(&metadata.resolution); + // Derived from the effective resolution, not the lockfile's: a + // custom fetcher's `delegate` can resolve to a directory, and + // then the file map points at mutable source even though the + // lockfile entry says otherwise. + let source_is_mutable = matches!(resolution, LockfileResolution::Directory(_)); let cas_paths: HashMap = match resolution { LockfileResolution::Tarball(_) | LockfileResolution::Registry(_) => { @@ -636,6 +654,7 @@ impl InstallPackageBySnapshot<'_> { package_id: &package_id, package_key, snapshot, + source_is_mutable, symlink: config.symlink, skipped, // The non-deferred slot link runs only on the fresh @@ -650,7 +669,7 @@ impl InstallPackageBySnapshot<'_> { .map_err(InstallPackageBySnapshotError::CreateVirtualDir)?; } - Ok(cas_paths) + Ok(InstalledPackage { cas_paths, source_is_mutable }) } } diff --git a/pnpm/crates/deps-restorer/src/install_package_by_snapshot/tests.rs b/pnpm/crates/deps-restorer/src/install_package_by_snapshot/tests.rs index e5487c5e21..e3b8184948 100644 --- a/pnpm/crates/deps-restorer/src/install_package_by_snapshot/tests.rs +++ b/pnpm/crates/deps-restorer/src/install_package_by_snapshot/tests.rs @@ -548,7 +548,7 @@ async fn cold_batch_reuses_in_flight_prefetch_from_mem_cache() { .await .expect("cold batch must reuse the prefetched download instead of fetching"); - assert_eq!(cas_paths, seeded); + assert_eq!(cas_paths.cas_paths, seeded); drop(store_tmp); } @@ -757,7 +757,7 @@ async fn run_snapshot_install_with_picker( picker: &std::sync::Arc, tarball_mem_cache: Option<&std::sync::Arc>, workspace_root: &std::path::Path, -) -> Result, InstallPackageBySnapshotError> { +) -> Result { let package_key: PackageKey = "foo@1.0.0".parse().expect("parse key"); let layout = crate::VirtualStoreLayout::legacy(workspace_root.join("vstore"), 120); let allow_build_policy = crate::AllowBuildPolicy::new( @@ -841,7 +841,7 @@ async fn custom_fetcher_delegate_rewrites_the_resolution() { .await .expect("the delegated registry resolution must drive the fetch"); - assert_eq!(cas_paths, seeded); + assert_eq!(cas_paths.cas_paths, seeded); drop(store_tmp); } @@ -885,7 +885,7 @@ async fn custom_fetcher_declining_falls_through_to_the_original_resolution() { .await .expect("a declined package must take the built-in path unchanged"); - assert_eq!(cas_paths, seeded); + assert_eq!(cas_paths.cas_paths, seeded); drop(store_tmp); } @@ -1126,7 +1126,10 @@ async fn installing_a_runtime_persists_the_synthesized_manifest_into_the_store_i .run::() .await .expect("cold runtime install"); - assert!(cold_cas_paths.contains_key("package.json"), "the cold slot gets the manifest"); + assert!( + cold_cas_paths.cas_paths.contains_key("package.json"), + "the cold slot gets the manifest", + ); // Flush the store-index writer so the row is durable before read-back. drop(writer); @@ -1186,7 +1189,7 @@ async fn installing_a_runtime_persists_the_synthesized_manifest_into_the_store_i .await .expect("warm runtime reinstall reads the store, not the network"); assert!( - warm_cas_paths.contains_key("package.json"), + warm_cas_paths.cas_paths.contains_key("package.json"), "the warm reinstall re-materializes the manifest from the persisted row", ); @@ -1242,7 +1245,7 @@ async fn custom_typed_resolution_installs_via_delegating_fetcher() { .await .expect("a delegating fetcher must install a custom-typed resolution"); - assert_eq!(cas_paths, seeded); + assert_eq!(cas_paths.cas_paths, seeded); drop(store_tmp); } @@ -1278,3 +1281,41 @@ async fn custom_typed_resolution_without_a_claiming_fetcher_fails() { drop(store_tmp); } + +/// A custom fetcher may delegate to a directory resolution, in which +/// case the file map points at mutable local source even though the +/// lockfile entry does not say so. Reporting that from the effective +/// resolution is what lets the slot be re-copied on the next install. +#[tokio::test] +async fn a_delegated_directory_resolution_reports_mutable_source() { + let store_tmp = tempfile::tempdir().expect("create the store dir"); + let source = store_tmp.path().join("local-src"); + std::fs::create_dir_all(&source).expect("create the source dir"); + std::fs::write( + source.join("package.json"), + serde_json::json!({ "name": "foo", "version": "1.0.0" }).to_string(), + ) + .expect("write the source manifest"); + + let config = leaked_offline_config("https://registry.test", store_tmp.path()); + let metadata = registry_metadata(); + + let picker = scripted_picker( + true, + Ok(serde_json::json!({ + "delegate": { "type": "directory", "directory": "local-src" }, + })), + ); + + let installed = + run_snapshot_install_with_picker(config, &metadata, &picker, None, store_tmp.path()) + .await + .expect("the delegated directory resolution must drive the fetch"); + + assert!( + installed.source_is_mutable, + "a delegated directory resolution must report mutable source", + ); + + drop(store_tmp); +} diff --git a/pnpm/crates/deps-restorer/src/linking.rs b/pnpm/crates/deps-restorer/src/linking.rs index ffe298530c..77fa5d2e9c 100644 --- a/pnpm/crates/deps-restorer/src/linking.rs +++ b/pnpm/crates/deps-restorer/src/linking.rs @@ -123,7 +123,7 @@ pub struct LinkPhaseInputs<'a> { pub is_hoisted: bool, pub prune_orphans: bool, pub prior_hoisted_dependencies: Option<&'a crate::HoistedDependencies>, - pub host_node: Option<&'a (bool, String)>, + pub host_node: Option<&'a crate::materialization_plan::HostNode>, pub supported_architectures: Option<&'a pacquet_package_is_installable::SupportedArchitectures>, pub logged_methods: &'a AtomicU8, } diff --git a/pnpm/crates/deps-restorer/src/materialization_plan.rs b/pnpm/crates/deps-restorer/src/materialization_plan.rs index 0c2e52a9c6..3c2ea577d4 100644 --- a/pnpm/crates/deps-restorer/src/materialization_plan.rs +++ b/pnpm/crates/deps-restorer/src/materialization_plan.rs @@ -22,6 +22,23 @@ use std::{ path::Path, }; +/// The host Node, reduced to what the phases after the installability +/// check still need. +#[derive(Debug, Clone)] +pub struct HostNode { + pub version: String, + /// `false` when detection fell back to the synthetic host. A + /// synthetic version must not key the store: it would poison both + /// the side-effects cache and the global-virtual-store hash. + pub detected: bool, +} + +impl From<&InstallabilityHost> for HostNode { + fn from(host: &InstallabilityHost) -> Self { + HostNode { version: host.node_version.clone(), detected: host.node_detected } + } +} + /// Detect the host an install's `os` / `cpu` / `libc` / `engines` /// constraints are checked against. /// @@ -185,14 +202,10 @@ pub fn compute_skip_set( /// [`JoinHandle`][tokio::task::JoinHandle] so it overlaps the /// virtual-store I/O — except under the global virtual store, whose /// layout needs the name synchronously. -/// -/// A synthetic host (`node_detected == false`) yields `None` rather than -/// a `99999.0.0`-derived key, which would poison both the cache and the -/// GVS hash. pub async fn resolve_engine_name( enable_global_virtual_store: bool, snapshots: Option<&HashMap>, - host_node: Option<&(bool, String)>, + host_node: Option<&HostNode>, ) -> (Option, Option>>) { fn probe() -> Option { pacquet_graph_hasher::detect_node_major() @@ -203,12 +216,12 @@ pub async fn resolve_engine_name( return (Some(pacquet_graph_hasher::engine_name(major, None, None)), None); } match host_node { - Some((true, version)) => ( + Some(HostNode { version, detected: true }) => ( parse_major_from_version(version) .map(|major| pacquet_graph_hasher::engine_name(major, None, None)), None, ), - Some((false, _)) => (None, None), + Some(HostNode { detected: false, .. }) => (None, None), None if enable_global_virtual_store => { (tokio::task::spawn_blocking(probe).await.ok().flatten(), None) } diff --git a/pnpm/crates/package-manager/src/install/prepare_modules_state.rs b/pnpm/crates/package-manager/src/install/prepare_modules_state.rs index 493bfa9039..dcf40d0233 100644 --- a/pnpm/crates/package-manager/src/install/prepare_modules_state.rs +++ b/pnpm/crates/package-manager/src/install/prepare_modules_state.rs @@ -292,6 +292,15 @@ pub(super) async fn prepare_modules_state<'install, Reporter: self::Reporter + ' && let Some(wanted_lockfile) = lockfile && let Some(current) = current_lockfile && wanted_lockfile == current + // A `file:` dependency resolves to a directory whose + // contents can change with nothing in the lockfile or + // `.modules.yaml` moving, so an equal-lockfile tree is not + // evidence that its slot is current. pnpm's `file:` is a + // copy taken at install time, not a symlink, so the copy + // has to be retaken; the TypeScript CLI has no gate at this + // level at all and instead forces every directory dep + // through materialization in `lockfileToDepGraph`. + && !has_directory_snapshot(wanted_lockfile) && let Some(modules) = modules_manifest.as_ref() && modules_consistent_with(modules, config, node_linker, included) // A `supportedArchitectures` change alters the skip set @@ -399,6 +408,17 @@ pub(super) async fn prepare_modules_state<'install, Reporter: self::Reporter + ' })) } +/// Whether any package in the lockfile resolves to a local directory. +/// +/// Such a package's source is mutable between installs, so its +/// materialized copy can go stale while every install-state artifact +/// still says the tree is current. +fn has_directory_snapshot(lockfile: &Lockfile) -> bool { + lockfile.packages.iter().flat_map(|packages| packages.values()).any(|metadata| { + matches!(metadata.resolution, pacquet_lockfile::LockfileResolution::Directory(_)) + }) +} + fn is_safe_modules_purge_target(modules_dir: &Path, workspace_root: &Path) -> bool { modules_dir != workspace_root && modules_dir.starts_with(workspace_root) } diff --git a/pnpm/crates/package-manager/src/install_with_fresh_lockfile.rs b/pnpm/crates/package-manager/src/install_with_fresh_lockfile.rs index 3f3eefdab2..c16649305c 100644 --- a/pnpm/crates/package-manager/src/install_with_fresh_lockfile.rs +++ b/pnpm/crates/package-manager/src/install_with_fresh_lockfile.rs @@ -1258,11 +1258,9 @@ impl InstallWithFreshLockfile<'_, DependencyGroupList> { supported_architectures, ) .await; - // Detect the host node once and reuse it for both the engine-name - // cache key and installability check, mirroring the frozen path. - let host_node: Option<(bool, String)> = installability_host + let host_node = installability_host .as_ref() - .map(|host| (host.node_detected, host.node_version.clone())); + .map(pacquet_deps_restorer::materialization_plan::HostNode::from); let (engine_name, deferred_engine_handle) = pacquet_deps_restorer::materialization_plan::resolve_engine_name(