fix(deps-restorer): forget hoisted locations before replacing them (#17370)

Under nodeLinker: hoisted, both CLIs reuse a package at a location the
previous install's hoistedLocations records. Nothing marked an install in
progress, so an install that stopped after replacing a recorded location
but before writing .modules.yaml left the old record behind. Reverting the
change then reused the replaced files.

Before the linker touches the tree, both CLIs now rewrite .modules.yaml
without every recorded location the install does not reuse as is. A
repeat install that reuses everything writes nothing extra.

A narrow window remains between the final .modules.yaml write and the
current lockfile write, which this change keeps in their existing order.

Closes pnpm/tasks#182

.changeset/hoisted-interrupted-install-reuse.md

@@ -0,0 +1,7 @@
+---
+"@pnpm/installing.deps-restorer": patch
+"pnpm": patch
+"pacquet": patch
+---
+
+With `nodeLinker: hoisted`, an install that stops partway no longer leaves the next install trusting the packages it was replacing. Before, reverting the change that started the stopped install, such as a patch edit, kept the half-replaced package in `node_modules`.

pnpm/crates/cli/tests/suite/hoisted_node_linker/repeat_install.rs

@@ -982,3 +982,66 @@ fn removing_a_linked_dependency_does_not_unlink_through_a_symlinked_scope() {
 
     drop((root, mock_instance));
 }
+
+/// An install that stops while the linker replaces a recorded location must
+/// not leave that location recorded, or the next install would trust whatever
+/// the interrupted import left there.
+#[cfg(unix)]
+#[test]
+fn an_interrupted_hoisted_install_forgets_the_locations_it_replaces() {
+    use std::os::unix::fs::PermissionsExt;
+    const REPLACED: &str = "@pnpm.e2e/dep-of-pkg-with-1-dep";
+    let CommandTempCwd {
+        pacquet,
+        root,
+        workspace,
+        npmrc_info,
+        ..
+    } = CommandTempCwd::init().add_mocked_registry();
+    let AddMockedRegistry { mock_instance, .. } = npmrc_info;
+
+    write_manifest(&workspace, serde_json::json!({ REPLACED: "100.0.0", "is-positive": "1.0.0" }));
+    write_workspace_yaml(&workspace, "nodeLinker: hoisted\noptimisticRepeatInstall: false\n");
+    pacquet
+        .with_args(["install"])
+        .assert()
+        .success();
+    let replaced = DirWitness::take(&workspace.join("node_modules").join(REPLACED));
+
+    // A read-only scope directory stops the linker before it replaces the package.
+    let scope_dir = workspace.join("node_modules/@pnpm.e2e");
+    let set_mode = |mode: u32| {
+        fs::set_permissions(&scope_dir, fs::Permissions::from_mode(mode))
+            .expect("set the scope dir mode");
+    };
+    write_manifest(&workspace, serde_json::json!({ REPLACED: "100.1.0", "is-positive": "1.0.0" }));
+    set_mode(0o555);
+    let output = pacquet_at(&workspace)
+        .with_args(["install"])
+        .output()
+        .expect("run pnpm install");
+    set_mode(0o755);
+    assert!(!output.status.success(), "the install was expected to fail: {output:?}");
+
+    let hoisted_locations = read_modules_manifest::<ModulesHost>(&workspace.join("node_modules"))
+        .expect("read .modules.yaml")
+        .expect(".modules.yaml exists")
+        .hoisted_locations
+        .unwrap_or_default();
+    dbg!(&hoisted_locations);
+    assert!(!hoisted_locations.contains_key(&format!("{REPLACED}@100.0.0")));
+    assert_eq!(
+        hoisted_locations.get("is-positive@1.0.0"),
+        Some(&vec!["node_modules/is-positive".to_string()]),
+    );
+
+    write_manifest(&workspace, serde_json::json!({ REPLACED: "100.0.0", "is-positive": "1.0.0" }));
+    pacquet_at(&workspace)
+        .with_args(["install"])
+        .assert()
+        .success();
+    assert!(!replaced.is_intact(), "the forgotten location was imported again");
+    assert_eq!(read_pkg_version(&workspace, &format!("node_modules/{REPLACED}")), "100.0.0");
+
+    drop((root, mock_instance));
+}

pnpm/crates/deps-restorer/src/install_frozen_lockfile.rs

@@ -184,6 +184,12 @@ pub enum InstallFrozenLockfileError {
     #[diagnostic(transparent)]
     LinkHoistedModules(#[error(source)] LinkHoistedModulesError),
 
+    #[diagnostic(transparent)]
+    ReadModules(#[error(source)] pnpm_modules_yaml::ReadModulesError),
+
+    #[diagnostic(transparent)]
+    WriteModules(#[error(source)] pnpm_modules_yaml::WriteModulesError),
+
     #[display("failed to write package map: {_0}")]
     #[diagnostic(code(ERR_PNPM_PACKAGE_MANAGER_WRITE_PACKAGE_MAP))]
     WritePackageMap(#[error(source)] crate::WritePackageMapError),
@@ -235,6 +241,12 @@ impl From<HoistedLinkerError> for InstallFrozenLockfileError {
             HoistedLinkerError::SymlinkDirectDependencies(error) => {
                 InstallFrozenLockfileError::SymlinkDirectDependencies(error)
             }
+            HoistedLinkerError::ReadModules(error) => {
+                InstallFrozenLockfileError::ReadModules(error)
+            }
+            HoistedLinkerError::WriteModules(error) => {
+                InstallFrozenLockfileError::WriteModules(error)
+            }
             HoistedLinkerError::WritePackageMap(error) => {
                 InstallFrozenLockfileError::WritePackageMap(error)
             }

pnpm/crates/deps-restorer/src/install_frozen_lockfile/hoisted.rs

@@ -10,9 +10,11 @@ pub use hoist_plan::{
 mod direct_links;
 mod hoist_plan;
 mod package_root_links;
+mod replaced_locations;
 mod stale_links;
 mod workspace_links;
 
+use replaced_locations::forget_replaced_hoisted_locations;
 use stale_links::{unlink_prior_workspace_hoists, unlink_stale_direct_links};
 use workspace_links::link_hoisted_workspace_dependencies;
 
@@ -106,6 +108,10 @@ pub enum HoistedLinkerError {
     HoistSymlink(#[error(source)] SymlinkPackageError),
     #[diagnostic(transparent)]
     HoistLinkBins(#[error(source)] pnpm_cmd_shim::LinkBinsError),
+    #[diagnostic(transparent)]
+    ReadModules(#[error(source)] pnpm_modules_yaml::ReadModulesError),
+    #[diagnostic(transparent)]
+    WriteModules(#[error(source)] pnpm_modules_yaml::WriteModulesError),
     #[display("failed to write package map: {_0}")]
     #[diagnostic(code(ERR_PNPM_PACKAGE_MANAGER_WRITE_PACKAGE_MAP))]
     WritePackageMap(#[error(source)] crate::WritePackageMapError),
@@ -131,6 +137,7 @@ pub fn run_hoisted_linker<Reporter: self::Reporter>(
     let build_present = inputs.prior.build_present_packages;
     let unbuilt = inputs.prior.unbuilt_builds;
     let walked = walk_hoisted_graph(inputs, lockfile, skipped)?;
+    forget_replaced_hoisted_locations(inputs, &walked)?;
     unlink_prior_workspace_hoists(inputs)?;
     unlink_stale_direct_links(inputs)?;
     let (mut bins, hoisted_dependencies) =

pnpm/crates/deps-restorer/src/install_frozen_lockfile/hoisted/replaced_locations.rs

@@ -0,0 +1,72 @@
+//! Recorded hoisted locations the linker is about to replace, forgotten
+//! before it starts.
+
+#[cfg(test)]
+mod tests;
+
+use super::{HoistedLinkerError, HoistedLinkerInputs};
+use crate::{DependenciesGraph, HoistedLocations};
+use std::path::Path;
+
+/// Remove from `.modules.yaml` every recorded location this install does not
+/// reuse as it is, before the linker replaces or removes it. The walker trusts
+/// a recorded location, so an install interrupted after this point must not
+/// leave a record of the content it was replacing. Writes nothing when every
+/// recorded location is reused.
+pub(super) fn forget_replaced_hoisted_locations(
+    inputs: &HoistedLinkerInputs<'_>,
+    walked: &crate::hoisted_dep_graph::LockfileToDepGraphResult,
+) -> Result<(), HoistedLinkerError> {
+    let Some(recorded) = inputs.prior.current_hoisted_locations else { return Ok(()) };
+    let Some(kept) =
+        reused_hoisted_locations(recorded, &walked.graph, inputs.projects.walker_lockfile_dir)
+    else {
+        return Ok(());
+    };
+    let modules_dir = &inputs.config.modules_dir;
+    let Some(mut modules) =
+        pnpm_modules_yaml::read_modules_manifest::<pnpm_modules_yaml::Host>(modules_dir)
+            .map_err(HoistedLinkerError::ReadModules)?
+    else {
+        return Ok(());
+    };
+    modules.hoisted_locations = (!kept.is_empty()).then_some(kept);
+    pnpm_modules_yaml::write_modules_manifest::<pnpm_modules_yaml::Host>(modules_dir, modules)
+        .map_err(HoistedLinkerError::WriteModules)
+}
+
+/// The recorded locations `graph` reuses: a present node of the same dep path
+/// at that directory. `None` when that is every recorded location.
+fn reused_hoisted_locations(
+    recorded: &HoistedLocations,
+    graph: &DependenciesGraph,
+    lockfile_dir: &Path,
+) -> Option<HoistedLocations> {
+    let is_reused = |dep_path: &str, location: &str| {
+        graph
+            .get(&lockfile_dir.join(location))
+            .is_some_and(|node| node.present && node.package.dep_path.as_str() == dep_path)
+    };
+    // Checked before building the result, so an install that reuses
+    // everything allocates nothing.
+    let all_reused = recorded
+        .iter()
+        .all(|(dep_path, locations)| {
+            locations.iter().all(|location| is_reused(dep_path, location))
+        });
+    if all_reused {
+        return None;
+    }
+    let mut kept = HoistedLocations::new();
+    for (dep_path, locations) in recorded {
+        let reused: Vec<String> = locations
+            .iter()
+            .filter(|location| is_reused(dep_path, location))
+            .cloned()
+            .collect();
+        if !reused.is_empty() {
+            kept.insert(dep_path.clone(), reused);
+        }
+    }
+    Some(kept)
+}

pnpm/crates/deps-restorer/src/install_frozen_lockfile/hoisted/replaced_locations/tests.rs

@@ -0,0 +1,99 @@
+use super::reused_hoisted_locations;
+use crate::{DependenciesGraph, DependenciesGraphNode, HoistedLocations, HoistedPackageMetadata};
+use pnpm_lockfile::{DirectoryResolution, PkgIdWithPatchHash};
+use pnpm_modules_yaml::DepPath;
+use pretty_assertions::assert_eq;
+use std::{
+    collections::{BTreeMap, BTreeSet},
+    path::{Path, PathBuf},
+};
+
+fn node(dep_path: &str, dir: PathBuf, present: bool) -> DependenciesGraphNode {
+    DependenciesGraphNode {
+        package: HoistedPackageMetadata {
+            dep_path: DepPath::from(dep_path.to_string()),
+            pkg_id_with_patch_hash: PkgIdWithPatchHash::from(dep_path),
+            name: "pkg".to_string(),
+            version: "1.0.0".to_string(),
+            has_bin: false,
+            has_bundled_dependencies: false,
+            patch: None,
+            resolution: DirectoryResolution { directory: "pkg".to_string() }.into(),
+        },
+        alias: None,
+        modules: dir
+            .parent()
+            .expect("dir has a parent")
+            .to_path_buf(),
+        dir,
+        optional: false,
+        optional_dependencies: BTreeSet::new(),
+        present,
+        children: BTreeMap::new(),
+    }
+}
+
+/// Locations are recorded with `/` on every platform, so this also covers
+/// their lookup on Windows.
+fn graph_with(lockfile_dir: &Path, nodes: &[(&str, &str, bool)]) -> DependenciesGraph {
+    nodes
+        .iter()
+        .map(|&(dep_path, location, present)| {
+            let dir = lockfile_dir.join(location);
+            (dir.clone(), node(dep_path, dir, present))
+        })
+        .collect()
+}
+
+fn locations(entries: &[(&str, &[&str])]) -> HoistedLocations {
+    entries
+        .iter()
+        .map(|(dep_path, dirs)| {
+            (
+                dep_path.to_string(),
+                dirs.iter()
+                    .map(ToString::to_string)
+                    .collect(),
+            )
+        })
+        .collect()
+}
+
+#[test]
+fn every_location_reused_needs_no_rewrite() {
+    let lockfile_dir = Path::new("/project");
+    let recorded = locations(&[("a@1.0.0", &["node_modules/a", "node_modules/b/node_modules/a"])]);
+    let graph = graph_with(
+        lockfile_dir,
+        &[("a@1.0.0", "node_modules/a", true), ("a@1.0.0", "node_modules/b/node_modules/a", true)],
+    );
+    assert_eq!(reused_hoisted_locations(&recorded, &graph, lockfile_dir), None);
+}
+
+#[test]
+fn replaced_and_removed_locations_are_dropped() {
+    let lockfile_dir = Path::new("/project");
+    let recorded = locations(&[
+        ("kept@1.0.0", &["node_modules/kept"]),
+        ("reimported@1.0.0", &["node_modules/reimported"]),
+        ("taken-over@1.0.0", &["node_modules/taken-over"]),
+        ("removed@1.0.0", &["node_modules/removed"]),
+        ("moved@1.0.0", &["node_modules/moved", "node_modules/x/node_modules/moved"]),
+    ]);
+    let graph = graph_with(
+        lockfile_dir,
+        &[
+            ("kept@1.0.0", "node_modules/kept", true),
+            ("reimported@1.0.0", "node_modules/reimported", false),
+            ("taken-over@2.0.0", "node_modules/taken-over", true),
+            ("moved@1.0.0", "node_modules/moved", true),
+        ],
+    );
+    assert_eq!(
+        reused_hoisted_locations(&recorded, &graph, lockfile_dir),
+        Some(locations(&[
+            ("kept@1.0.0", &["node_modules/kept"]),
+            ("moved@1.0.0", &["node_modules/moved"]),
+        ])),
+    );
+}

pnpm/crates/deps-restorer/src/linking.rs

@@ -53,6 +53,10 @@ pub enum LinkPhaseError {
     HoistedDepGraph(#[error(source)] crate::HoistedDepGraphError),
     #[diagnostic(transparent)]
     LinkHoistedModules(#[error(source)] crate::LinkHoistedModulesError),
+    #[diagnostic(transparent)]
+    ReadModules(#[error(source)] pnpm_modules_yaml::ReadModulesError),
+    #[diagnostic(transparent)]
+    WriteModules(#[error(source)] pnpm_modules_yaml::WriteModulesError),
     #[display("failed to write package map: {_0}")]
     #[diagnostic(code(ERR_PNPM_PACKAGE_MANAGER_WRITE_PACKAGE_MAP))]
     WritePackageMap(#[error(source)] crate::WritePackageMapError),
@@ -74,6 +78,8 @@ impl From<HoistedLinkerError> for LinkPhaseError {
             HoistedLinkerError::SymlinkDirectDependencies(error) => {
                 LinkPhaseError::SymlinkDirectDependencies(error)
             }
+            HoistedLinkerError::ReadModules(error) => LinkPhaseError::ReadModules(error),
+            HoistedLinkerError::WriteModules(error) => LinkPhaseError::WriteModules(error),
             HoistedLinkerError::WritePackageMap(error) => LinkPhaseError::WritePackageMap(error),
             HoistedLinkerError::PruneStaleLinks(error) => LinkPhaseError::PruneStaleModules(error),
             HoistedLinkerError::HoistSymlink(error) => LinkPhaseError::HoistSymlink(error),

pnpm/crates/package-manager/src/install_with_fresh_lockfile/errors.rs

@@ -64,6 +64,12 @@ pub enum InstallWithFreshLockfileError {
     #[diagnostic(transparent)]
     LinkHoistedModules(#[error(source)] crate::LinkHoistedModulesError),
 
+    #[diagnostic(transparent)]
+    ReadModules(#[error(source)] pnpm_modules_yaml::ReadModulesError),
+
+    #[diagnostic(transparent)]
+    WriteModules(#[error(source)] pnpm_modules_yaml::WriteModulesError),
+
     #[display("failed to write package map: {_0}")]
     #[diagnostic(code(ERR_PNPM_PACKAGE_MANAGER_WRITE_PACKAGE_MAP))]
     WritePackageMap(#[error(source)] crate::WritePackageMapError),
@@ -250,6 +256,12 @@ impl From<crate::install_frozen_lockfile::HoistedLinkerError> for InstallWithFre
             HoistedLinkerError::SymlinkDirectDependencies(error) => {
                 InstallWithFreshLockfileError::SymlinkDirectDependencies(error)
             }
+            HoistedLinkerError::ReadModules(error) => {
+                InstallWithFreshLockfileError::ReadModules(error)
+            }
+            HoistedLinkerError::WriteModules(error) => {
+                InstallWithFreshLockfileError::WriteModules(error)
+            }
             HoistedLinkerError::WritePackageMap(error) => {
                 InstallWithFreshLockfileError::WritePackageMap(error)
             }

pnpm11/installing/deps-installer/test/hoistedNodeLinker/install.ts

@@ -401,3 +401,35 @@ test('installing with hoisted node-linker a package that is a peer dependency of
   const lockfile = project.readLockfile()
   expect(lockfile.packages['@pnpm.e2e/peer-of-itself@1.0.0'].peerDependencies).toBeFalsy()
 })
+
+const testOnPosixAsNonRoot = process.platform === 'win32' || process.getuid?.() === 0 ? test.skip : test
+
+// The next install trusts a recorded location, so an install that stops while
+// replacing one must not leave it recorded.
+testOnPosixAsNonRoot('an interrupted install forgets the hoisted locations it replaces', async () => {
+  prepareEmpty()
+  const manifest = (version: string) => ({
+    dependencies: {
+      '@pnpm.e2e/dep-of-pkg-with-1-dep': version,
+      'is-positive': '1.0.0',
+    },
+  })
+  await install(manifest('100.0.0'), testDefaults({ nodeLinker: 'hoisted' }))
+  fs.writeFileSync('node_modules/@pnpm.e2e/dep-of-pkg-with-1-dep/witness', '')
+
+  // A read-only scope directory stops the linker before it replaces the package.
+  fs.chmodSync('node_modules/@pnpm.e2e', 0o555)
+  try {
+    await expect(install(manifest('100.1.0'), testDefaults({ nodeLinker: 'hoisted' }))).rejects.toThrow()
+  } finally {
+    fs.chmodSync('node_modules/@pnpm.e2e', 0o755)
+  }
+
+  const { hoistedLocations } = loadJsonFileSync<{ hoistedLocations: Record<string, string[]> }>('node_modules/.modules.yaml')
+  expect(hoistedLocations['@pnpm.e2e/dep-of-pkg-with-1-dep@100.0.0']).toBeUndefined()
+  expect(hoistedLocations['is-positive@1.0.0']).toStrictEqual([path.join('node_modules', 'is-positive')])
+
+  await install(manifest('100.0.0'), testDefaults({ nodeLinker: 'hoisted' }))
+  expect(fs.existsSync('node_modules/@pnpm.e2e/dep-of-pkg-with-1-dep/witness')).toBe(false)
+  expect(loadJsonFileSync<{ version: string }>('node_modules/@pnpm.e2e/dep-of-pkg-with-1-dep/package.json').version).toBe('100.0.0')
+})

pnpm11/installing/deps-restorer/src/forgetReplacedHoistedLocations.ts

@@ -0,0 +1,31 @@
+import path from 'node:path'
+
+import type { DependenciesGraph } from '@pnpm/deps.graph-builder'
+import { writeModulesManifest } from '@pnpm/installing.modules-yaml'
+
+import type { HeadlessContext } from './context.js'
+
+/**
+ * Removes from `.modules.yaml` every recorded location this install does not
+ * reuse as it is, before the linker replaces or removes it. The next install
+ * trusts a recorded location, so an install interrupted after this point must
+ * not leave a record of the content it was replacing. Writes nothing when every
+ * recorded location is reused.
+ */
+export async function forgetReplacedHoistedLocations ({ opts, rootModulesDir }: HeadlessContext, graph: DependenciesGraph): Promise<void> {
+  const recorded = opts.modulesFile?.hoistedLocations
+  if (recorded == null) return
+  const isReused = (depPath: string, location: string): boolean => {
+    const node = graph[path.join(opts.lockfileDir, location)]
+    return node != null && node.fetching == null && node.depPath === depPath
+  }
+  const kept: Record<string, string[]> = {}
+  let droppedAny = false
+  for (const [depPath, locations] of Object.entries(recorded)) {
+    const reused = locations.filter((location) => isReused(depPath, location))
+    if (reused.length !== locations.length) droppedAny = true
+    if (reused.length > 0) kept[depPath] = reused
+  }
+  if (!droppedAny) return
+  await writeModulesManifest(rootModulesDir, { ...opts.modulesFile!, hoistedLocations: kept })
+}

pnpm11/installing/deps-restorer/src/linkHoistedLayout.ts

@@ -12,6 +12,7 @@ import type { DependencyManifest, DepPath, HoistedDependencies, ProjectId } from
 
 import type { HeadlessContext, HeadlessDepGraph, LinkedDependencies } from './context.js'
 import { getDirectDependencyAliases } from './directDependencyAliases.js'
+import { forgetReplacedHoistedLocations } from './forgetReplacedHoistedLocations.js'
 import { linkHoistedModules } from './linkHoistedModules.js'
 import { reportDirectDependencyChanges } from './reportDirectDependencyChanges.js'
 import { symlinkDirectDependencies } from './symlinkDirectDependencies.js'
@@ -28,6 +29,7 @@ export async function linkHoistedLayout (ctx: HeadlessContext, depGraph: Headles
   if (ctx.skipPostImportLinking) {
     return { heldBackBinsDirs: [], linkedToRoot: 0, newHoistedDependencies: opts.hoistedDependencies }
   }
+  await forgetReplacedHoistedLocations(ctx, depGraph.graph)
   const hoistsWorkspacePackages = opts.ignorePackageManifest !== true
   if (hoistsWorkspacePackages) {
     // Unlinked before the linker writes the tree: a package that takes over