From 1c0587698d0d869ea3fed0eee2fdace8d73cf695 Mon Sep 17 00:00:00 2001 From: Victor Sumner <308886+vsumner@users.noreply.github.com> Date: Wed, 17 Jun 2026 07:36:01 -0400 Subject: [PATCH] fix: narrow warm install relinking (#11169) ## Problem During warm installs, pnpm relinked existing packages more broadly than necessary when only some child dependencies changed. In the narrowed relinking path, removed child aliases could also remain behind as stale links after dependency updates. ## Solution Only pass changed child edges through the relinking path for existing packages. When a child alias is no longer present in the updated dependency set, remove the obsolete link before relinking. Added regression tests for both cases: - unchanged child dependencies are not relinked unnecessarily - deleted child dependencies do not remain as stale links after a warm install --------- Co-authored-by: Zoltan Kochan --- .changeset/tidy-drinks-help.md | 6 + installing/deps-installer/src/install/link.ts | 172 +++++++++++++++--- .../test/install/relinkingScope.test.ts | 116 ++++++++++++ .../src/create_virtual_dir_by_snapshot.rs | 76 +++++++- .../create_virtual_dir_by_snapshot/tests.rs | 77 +++++++- .../src/create_virtual_store.rs | 70 ++++++- .../src/create_virtual_store/tests.rs | 47 ++++- .../src/install_package_by_snapshot.rs | 4 + 8 files changed, 534 insertions(+), 34 deletions(-) create mode 100644 .changeset/tidy-drinks-help.md create mode 100644 installing/deps-installer/test/install/relinkingScope.test.ts diff --git a/.changeset/tidy-drinks-help.md b/.changeset/tidy-drinks-help.md new file mode 100644 index 0000000000..3648649cc2 --- /dev/null +++ b/.changeset/tidy-drinks-help.md @@ -0,0 +1,6 @@ +--- +"@pnpm/installing.deps-installer": patch +"pnpm": patch +--- + +Avoid relinking unchanged child dependencies and remove stale child links during warm installs. diff --git a/installing/deps-installer/src/install/link.ts b/installing/deps-installer/src/install/link.ts index 9f86709fe8..654649e63d 100644 --- a/installing/deps-installer/src/install/link.ts +++ b/installing/deps-installer/src/install/link.ts @@ -7,11 +7,13 @@ import { statsLogger, } from '@pnpm/core-loggers' import { calcDepState, type DepsStateCache, findRuntimeNodeVersion } from '@pnpm/deps.graph-hasher' +import { readModulesDir } from '@pnpm/fs.read-modules-dir' import { symlinkDependency } from '@pnpm/fs.symlink-dependency' -import type { - DependenciesGraph, - DependenciesGraphNode, - LinkedDependency, +import { + type DependenciesGraph, + type DependenciesGraphNode, + isValidDependencyAlias, + type LinkedDependency, } from '@pnpm/installing.deps-resolver' import type { InstallationResultStats } from '@pnpm/installing.deps-restorer' import { linkDirectDeps } from '@pnpm/installing.linking.direct-dep-linker' @@ -33,6 +35,7 @@ import type { SupportedArchitectures, } from '@pnpm/types' import { symlinkAllModules } from '@pnpm/worker' +import { rimraf } from '@zkochan/rimraf' import pLimit from 'p-limit' import { pathExists } from 'path-exists' import { difference, equals, isEmpty, pick, pickBy, props } from 'ramda' @@ -346,6 +349,10 @@ interface LinkNewPackagesResult { added: number } +type ModulesLinkJob = Pick & { + removedAliases?: string[] +} + async function linkNewPackages ( currentLockfile: LockfileObject, wantedLockfile: LockfileObject, @@ -372,23 +379,55 @@ async function linkNewPackages ( prefix: opts.lockfileDir, }) - const existingWithUpdatedDeps = [] + const existingWithUpdatedDeps: ModulesLinkJob[] = [] if (!opts.force && (currentLockfile.packages != null) && (wantedLockfile.packages != null)) { + const currentPackages = currentLockfile.packages + const wantedPackages = wantedLockfile.packages // add subdependencies that have been updated - // TODO: no need to relink everything. Can be relinked only what was changed - for (const depPath of wantedRelDepPaths) { - if (currentLockfile.packages[depPath] && - (!equals(currentLockfile.packages[depPath].dependencies, wantedLockfile.packages[depPath].dependencies) || - !isEmpty(currentLockfile.packages[depPath].optionalDependencies ?? {}) || - !isEmpty(wantedLockfile.packages[depPath].optionalDependencies ?? {})) + await Promise.all(wantedRelDepPaths.map((depPath) => limitModulesDirReads(async () => { + if (currentPackages[depPath] && + (!equals(currentPackages[depPath].dependencies, wantedPackages[depPath].dependencies) || + !isEmpty(currentPackages[depPath].optionalDependencies ?? {}) || + !isEmpty(wantedPackages[depPath].optionalDependencies ?? {})) ) { // TODO: come up with a test that triggers the usecase of depGraph[depPath] undefined // see related issue: https://github.com/pnpm/pnpm/issues/870 if (depGraph[depPath] && !newDepPathsSet.has(depPath)) { - existingWithUpdatedDeps.push(depGraph[depPath]) + const { actualChildrenChanged, removedAliases: actualRemovedAliases } = await getActualChildrenDiff( + depGraph[depPath], + depGraph, + opts.lockfileDir, + opts.optional + ) + if (actualChildrenChanged) { + existingWithUpdatedDeps.push({ + children: depGraph[depPath].children, + modules: depGraph[depPath].modules, + name: depGraph[depPath].name, + optionalDependencies: depGraph[depPath].optionalDependencies, + removedAliases: actualRemovedAliases, + }) + return + } + const { changedChildren, removedAliases } = getChangedChildren({ + currentDependencies: currentPackages[depPath].dependencies, + currentOptionalDependencies: currentPackages[depPath].optionalDependencies, + wantedDependencies: wantedPackages[depPath].dependencies, + wantedOptionalDependencies: wantedPackages[depPath].optionalDependencies, + allChildren: depGraph[depPath].children, + }) + if (!isEmpty(changedChildren) || removedAliases.length > 0) { + existingWithUpdatedDeps.push({ + children: changedChildren, + modules: depGraph[depPath].modules, + name: depGraph[depPath].name, + optionalDependencies: depGraph[depPath].optionalDependencies, + removedAliases, + }) + } } } - } + }))) } if (!newDepPathsSet.size && (existingWithUpdatedDeps.length === 0)) return { newDepPaths: [], added } @@ -457,6 +496,7 @@ async function selectNewFromWantedDeps ( } const limitLinking = pLimit(16) +const limitModulesDirReads = pLimit(16) async function linkAllPkgs ( storeController: StoreController, @@ -525,33 +565,109 @@ async function linkAllPkgs ( } async function linkAllModules ( - depNodes: DependenciesGraphNode[], + depNodes: ModulesLinkJob[], depGraph: DependenciesGraph, opts: { lockfileDir: string optional: boolean } ): Promise { + await Promise.all(depNodes.flatMap((depNode) => (depNode.removedAliases ?? []).map(async (alias) => limitModulesDirReads(async () => removeObsoleteChild(depNode.modules, alias))))) await symlinkAllModules({ deps: depNodes.map((depNode) => { - const children = opts.optional - ? depNode.children - : pickBy((_, childAlias) => !depNode.optionalDependencies.has(childAlias), depNode.children) - const childrenPaths: Record = {} - for (const [alias, childDepPath] of Object.entries(children ?? {})) { - if (childDepPath.startsWith('link:')) { - childrenPaths[alias] = path.resolve(opts.lockfileDir, childDepPath.slice(5)) - } else { - const pkg = depGraph[childDepPath] - if (!pkg || !pkg.installable && pkg.optional || alias === depNode.name) continue - childrenPaths[alias] = pkg.dir - } - } return { - children: childrenPaths, + children: getChildrenPaths(depNode, depGraph, opts.lockfileDir, opts.optional), modules: depNode.modules, name: depNode.name, } }), }) } + +function getChangedChildren ( + opts: { + currentDependencies: Record | undefined + currentOptionalDependencies: Record | undefined + wantedDependencies: Record | undefined + wantedOptionalDependencies: Record | undefined + allChildren: Record + } +): { changedChildren: Record, removedAliases: string[] } { + const { currentOptionalDependencies, wantedOptionalDependencies, allChildren } = opts + // Use null-prototype maps so a child literally named `constructor`, + // `toString`, `__proto__`, etc. is treated as a normal alias instead + // of colliding with an inherited `Object.prototype` key during the + // `in`/index lookups below. + const currentChildren: Record = Object.assign(Object.create(null), opts.currentDependencies, currentOptionalDependencies) + const wantedChildren: Record = Object.assign(Object.create(null), opts.wantedDependencies, wantedOptionalDependencies) + const changedChildren: Record = {} + const removedAliases: string[] = [] + for (const [alias, wantedChildDepPath] of Object.entries(wantedChildren)) { + const optionalityChanged = hasOwn(wantedOptionalDependencies, alias) !== hasOwn(currentOptionalDependencies, alias) + if (currentChildren[alias] !== wantedChildDepPath || optionalityChanged) { + const resolvedChildDepPath = hasOwn(allChildren, alias) ? allChildren[alias] : undefined + if (resolvedChildDepPath != null) { + changedChildren[alias] = resolvedChildDepPath + } + } + } + for (const alias of Object.keys(currentChildren)) { + if (!hasOwn(wantedChildren, alias)) { + removedAliases.push(alias) + } + } + return { changedChildren, removedAliases } +} + +function hasOwn (obj: Record | undefined, key: string): boolean { + return obj != null && Object.hasOwn(obj, key) +} + +async function getActualChildrenDiff ( + depNode: ModulesLinkJob, + depGraph: DependenciesGraph, + lockfileDir: string, + optional: boolean +): Promise<{ actualChildrenChanged: boolean, removedAliases: string[] }> { + if (depNode.optionalDependencies.size === 0) { + return { actualChildrenChanged: false, removedAliases: [] } + } + const currentAliases = new Set((await readModulesDir(depNode.modules) ?? []).filter((alias) => alias !== depNode.name)) + const nextAliases = new Set(Object.keys(getChildrenPaths(depNode, depGraph, lockfileDir, optional))) + const removedAliases = Array.from(currentAliases).filter((alias) => !nextAliases.has(alias)) + const actualChildrenChanged = removedAliases.length > 0 || + Array.from(nextAliases).some((alias) => !currentAliases.has(alias)) + return { actualChildrenChanged, removedAliases } +} + +async function removeObsoleteChild (modulesDir: string, alias: string): Promise { + // Guard against an alias that would escape the modules directory (e.g. `../../x`). + if (!isValidDependencyAlias(alias)) return + await rimraf(path.join(modulesDir, alias)) + if (alias[0] === '@') { + await fs.rmdir(path.join(modulesDir, alias.split('/')[0])).catch(() => {}) + } +} + +function getChildrenPaths ( + depNode: ModulesLinkJob, + depGraph: DependenciesGraph, + lockfileDir: string, + optional: boolean +): Record { + const children = optional + ? depNode.children + : pickBy((_, childAlias) => !depNode.optionalDependencies.has(childAlias), depNode.children) + const childrenPaths: Record = {} + for (const [alias, childDepPath] of Object.entries(children ?? {})) { + if (alias === depNode.name) continue + if (childDepPath.startsWith('link:')) { + childrenPaths[alias] = path.resolve(lockfileDir, childDepPath.slice(5)) + continue + } + const pkg = depGraph[childDepPath] + if (!pkg || !pkg.installable && pkg.optional) continue + childrenPaths[alias] = pkg.dir + } + return childrenPaths +} diff --git a/installing/deps-installer/test/install/relinkingScope.test.ts b/installing/deps-installer/test/install/relinkingScope.test.ts new file mode 100644 index 0000000000..92f71670c7 --- /dev/null +++ b/installing/deps-installer/test/install/relinkingScope.test.ts @@ -0,0 +1,116 @@ +import fs from 'node:fs' +import path from 'node:path' + +import { expect, jest, test } from '@jest/globals' +import { prepare } from '@pnpm/prepare' +import type { ProjectManifest, ProjectRootDir } from '@pnpm/types' + +import { testDefaults } from '../utils/index.js' + +const symlinkAllModulesCalls: Array> = [] + +const originalWorker = await import('@pnpm/worker') +jest.unstable_mockModule('@pnpm/worker', () => ({ + ...originalWorker, + symlinkAllModules: jest.fn(async (opts: Parameters[0]) => { + symlinkAllModulesCalls.push( + opts.deps.map((dep) => ({ + name: dep.name, + children: Object.keys(dep.children).sort(), + })) + ) + return originalWorker.symlinkAllModules(opts) + }), +})) + +const { mutateModulesInSingleProject } = await import('@pnpm/installing.deps-installer') + +test('relinks only changed child edges for existing packages after dependency updates', async () => { + const manifest: ProjectManifest = { + dependencies: { + '@pnpm.e2e/pkg-with-good-optional': '1.0.0', + }, + } + const project = prepare(manifest) + + // Pin the child to an older version first so the update below is a + // real edge change. With the fixture's `*` range both installs would + // otherwise resolve to the same highest version and nothing relinks. + const initialOpts = testDefaults({ + overrides: { + '@pnpm.e2e/dep-of-pkg-with-1-dep': '100.0.0', + }, + }) + await mutateModulesInSingleProject({ + manifest, + mutation: 'install', + rootDir: process.cwd() as ProjectRootDir, + }, initialOpts) + + symlinkAllModulesCalls.length = 0 + + const updatedOpts = testDefaults({ + overrides: { + '@pnpm.e2e/dep-of-pkg-with-1-dep': '101.0.0', + }, + storeDir: initialOpts.storeDir, + }) + await mutateModulesInSingleProject({ + manifest, + mutation: 'install', + rootDir: process.cwd() as ProjectRootDir, + }, updatedOpts) + + const lockfile = project.readLockfile() + expect(lockfile.snapshots['@pnpm.e2e/pkg-with-good-optional@1.0.0'].dependencies?.['@pnpm.e2e/dep-of-pkg-with-1-dep']).toBe('101.0.0') + expect(lockfile.snapshots['@pnpm.e2e/pkg-with-good-optional@1.0.0'].optionalDependencies?.['is-positive']).toBe('1.0.0') + + const pkgModulesDir = path.resolve('node_modules/.pnpm/@pnpm.e2e+pkg-with-good-optional@1.0.0/node_modules') + expect(fs.realpathSync(path.join(pkgModulesDir, '@pnpm.e2e/dep-of-pkg-with-1-dep'))).toContain('101.0.0') + + const pkgCalls = symlinkAllModulesCalls + .flat() + .filter((dep) => dep.name === '@pnpm.e2e/pkg-with-good-optional') + + // The package must actually be relinked for the changed child edge, + // otherwise the `every` assertion below would pass vacuously on an + // empty array and prove nothing. + expect(pkgCalls.some(({ children }) => children.includes('@pnpm.e2e/dep-of-pkg-with-1-dep'))).toBe(true) + + // Existing packages with only one changed child edge should not be passed + // through the broad worker relinking path with unchanged aliases. + expect(pkgCalls.every(({ children }) => !children.includes('is-positive'))).toBe(true) +}) + +test('removes obsolete child links for existing packages after dependency updates', async () => { + const manifest: ProjectManifest = { + dependencies: { + '@pnpm.e2e/pkg-with-good-optional': '1.0.0', + }, + } + prepare(manifest) + + const initialOpts = testDefaults() + await mutateModulesInSingleProject({ + manifest, + mutation: 'install', + rootDir: process.cwd() as ProjectRootDir, + }, initialOpts) + + const obsoleteChildPath = path.resolve('node_modules/.pnpm/@pnpm.e2e+pkg-with-good-optional@1.0.0/node_modules/is-positive') + expect(fs.existsSync(obsoleteChildPath)).toBe(true) + + const updatedOpts = testDefaults({ + overrides: { + '@pnpm.e2e/pkg-with-good-optional>is-positive': '-', + }, + storeDir: initialOpts.storeDir, + }) + await mutateModulesInSingleProject({ + manifest, + mutation: 'install', + rootDir: process.cwd() as ProjectRootDir, + }, updatedOpts) + + expect(fs.existsSync(obsoleteChildPath)).toBe(false) +}) diff --git a/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot.rs b/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot.rs index 0b0fa6076c..91635e66b1 100644 --- a/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot.rs +++ b/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot.rs @@ -5,12 +5,18 @@ use crate::{ use derive_more::{Display, Error}; use miette::Diagnostic; use pacquet_config::PackageImportMethod; -use pacquet_lockfile::{PackageKey, SnapshotEntry}; +use pacquet_fs::{is_subdir, remove_symlink_dir}; +use pacquet_lockfile::{PackageKey, PkgName, SnapshotEntry}; use pacquet_reporter::{ LogEvent, LogLevel, PackageImportMethod as WireImportMethod, ProgressLog, ProgressMessage, Reporter, }; -use std::{collections::HashMap, fs, io, path::PathBuf, sync::atomic::AtomicU8}; +use std::{ + collections::HashMap, + fs, io, + path::{Path, PathBuf}, + sync::atomic::AtomicU8, +}; /// This subroutine creates the virtual-store slot for one package and then /// runs the two post-extraction tasks — CAS file import and intra-package @@ -55,6 +61,16 @@ pub struct CreateVirtualDirBySnapshot<'a> { /// `linkAllModules` at /// . pub skipped: &'a SkippedSnapshots, + /// Child aliases that were linked by a previous install but are no + /// longer in this snapshot's dependency set. Their stale symlinks + /// are unlinked from the slot before the progress event fires, so + /// a warm reinstall that drops a dependency (e.g. via an override) + /// doesn't leave a dangling child behind. Mirrors upstream's + /// `removeObsoleteChild` pass in `linkAllModules` at + /// . + /// Empty for fresh packages and for survivors whose dependency set + /// only changed by addition. + pub removed_aliases: &'a [PkgName], #[cfg(test)] pub(crate) link_concurrency_probe: Option<&'a tests::LinkConcurrencyProbe>, } @@ -75,6 +91,14 @@ pub enum CreateVirtualDirError { #[diagnostic(transparent)] SymlinkPackage(#[error(source)] SymlinkPackageError), + + #[display("Failed to remove obsolete child link at {path:?}: {error}")] + #[diagnostic(code(pacquet_package_manager::remove_obsolete_child))] + RemoveObsoleteChild { + path: PathBuf, + #[error(source)] + error: io::Error, + }, } impl CreateVirtualDirBySnapshot<'_> { @@ -90,6 +114,7 @@ impl CreateVirtualDirBySnapshot<'_> { package_key, snapshot, skipped, + removed_aliases, #[cfg(test)] link_concurrency_probe, } = self; @@ -141,6 +166,20 @@ impl CreateVirtualDirBySnapshot<'_> { cas_result?; symlink_result?; + // Unlink children the package no longer depends on. Run after + // the join rather than before it: the removed aliases are + // disjoint from the wanted set `create_symlink_layout` just + // linked, and from the package's own `node_modules/` + // directory the CAS import populates, so the end state is the + // same as upstream's remove-then-link ordering without racing + // either parallel task. + for alias in removed_aliases { + if *alias == package_key.name { + continue; + } + remove_obsolete_child(&virtual_node_modules_dir, alias)?; + } + // `pnpm:progress imported` mirrors pnpm's emit at // : // one event per (resolved + fetched) package once its CAFS @@ -181,5 +220,38 @@ pub(crate) fn optimistic_wire_method(method: PackageImportMethod) -> WireImportM } } +/// Unlink one obsolete child from a slot's `node_modules`, mirroring +/// pnpm's `removeObsoleteChild` at +/// . +/// +/// Removes the `/` symlink and, for a scoped +/// alias, drops the now-empty `@scope` directory (ignoring the error +/// when another scoped sibling keeps it populated). `remove_symlink_dir` +/// unlinks the symlink itself, never its target package. +/// +/// `is_subdir` is the traversal guard: `PkgName` parsing accepts shapes +/// such as `..` that would resolve outside the slot, so an alias that +/// doesn't stay within `node_modules` is skipped rather than removed. +fn remove_obsolete_child( + virtual_node_modules_dir: &Path, + alias: &PkgName, +) -> Result<(), CreateVirtualDirError> { + let child_path = virtual_node_modules_dir.join(alias.to_string()); + if !is_subdir(virtual_node_modules_dir, &child_path) { + return Ok(()); + } + match remove_symlink_dir(&child_path) { + Ok(()) => {} + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(error) => { + return Err(CreateVirtualDirError::RemoveObsoleteChild { path: child_path, error }); + } + } + if let Some(scope) = &alias.scope { + let _ = fs::remove_dir(virtual_node_modules_dir.join(format!("@{scope}"))); + } + Ok(()) +} + #[cfg(test)] pub(crate) mod tests; diff --git a/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot/tests.rs b/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot/tests.rs index 19fccd0cc6..248d6954a0 100644 --- a/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot/tests.rs +++ b/pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot/tests.rs @@ -1,6 +1,7 @@ -use super::{CreateVirtualDirBySnapshot, optimistic_wire_method}; +use super::{CreateVirtualDirBySnapshot, optimistic_wire_method, remove_obsolete_child}; use pacquet_config::PackageImportMethod; -use pacquet_lockfile::{PackageKey, SnapshotEntry}; +use pacquet_fs::force_symlink_dir; +use pacquet_lockfile::{PackageKey, PkgName, SnapshotEntry}; use pacquet_reporter::{ LogEvent, PackageImportMethod as WireImportMethod, ProgressMessage, Reporter, }; @@ -149,6 +150,7 @@ async fn run_emits_imported_event_after_import_indexed_dir() { package_key: &package_key, snapshot: &snapshot, skipped: &skipped, + removed_aliases: &[], link_concurrency_probe: None, } .run::() @@ -179,3 +181,74 @@ async fn run_emits_imported_event_after_import_indexed_dir() { "imported.to suffix must mirror the virtual-store layout; got {to}", ); } + +/// A warm reinstall that drops a child dependency unlinks the stale +/// symlink (and its now-empty `@scope` directory) while leaving the +/// children it still depends on in place. Ports the "removes obsolete +/// child links" case from upstream's `relinkingScope.test.ts` at +/// . +#[tokio::test] +async fn run_removes_obsolete_child_links() { + use pacquet_reporter::SilentReporter; + + let dir = tempdir().expect("tempdir"); + let layout = crate::VirtualStoreLayout::legacy( + dir.path().to_path_buf(), + pacquet_config::default_virtual_store_dir_max_length() as usize, + ); + let package_key: PackageKey = "react@18.0.0".parse().expect("valid snapshot key"); + let node_modules = layout.slot_dir(&package_key).join("node_modules"); + std::fs::create_dir_all(&node_modules).expect("create slot node_modules"); + + let target = dir.path().join("target"); + std::fs::create_dir_all(&target).expect("create symlink target"); + for alias in ["is-positive", "@scope/old", "keep-me"] { + force_symlink_dir(&target, &node_modules.join(alias)).expect("create stale child symlink"); + } + + let cas_paths: HashMap = HashMap::new(); + let logged_methods = AtomicU8::new(0); + let snapshot = SnapshotEntry::default(); + let skipped = crate::SkippedSnapshots::default(); + let removed_aliases = + [PkgName::parse("is-positive").unwrap(), PkgName::parse("@scope/old").unwrap()]; + CreateVirtualDirBySnapshot { + layout: &layout, + cas_paths: &cas_paths, + import_method: PackageImportMethod::Hardlink, + logged_methods: &logged_methods, + requester: "/proj", + package_id: "react@18.0.0", + package_key: &package_key, + snapshot: &snapshot, + skipped: &skipped, + removed_aliases: &removed_aliases, + link_concurrency_probe: None, + } + .run::() + .expect("run should succeed"); + + assert!(!node_modules.join("is-positive").exists(), "obsolete child must be unlinked"); + assert!(!node_modules.join("@scope").exists(), "now-empty scope directory must be removed"); + assert!( + node_modules.join("keep-me").symlink_metadata().is_ok(), + "children not in removed_aliases must be left untouched", + ); +} + +/// The traversal guard refuses to unlink an alias that resolves +/// outside the slot's `node_modules`. `PkgName` parsing accepts `..`, +/// so without the guard the join would escape the directory. +#[test] +fn remove_obsolete_child_skips_path_traversal() { + let dir = tempdir().expect("tempdir"); + let node_modules = dir.path().join("slot").join("node_modules"); + std::fs::create_dir_all(&node_modules).expect("create node_modules"); + let sibling = dir.path().join("slot").join("sibling"); + std::fs::create_dir_all(&sibling).expect("create sibling dir"); + + remove_obsolete_child(&node_modules, &PkgName::parse("..").unwrap()) + .expect("traversal alias is skipped, not an error"); + + assert!(sibling.exists(), "a `..` alias must not delete a sibling of node_modules"); +} diff --git a/pacquet/crates/package-manager/src/create_virtual_store.rs b/pacquet/crates/package-manager/src/create_virtual_store.rs index d09b59874c..5c191e97db 100644 --- a/pacquet/crates/package-manager/src/create_virtual_store.rs +++ b/pacquet/crates/package-manager/src/create_virtual_store.rs @@ -8,7 +8,7 @@ use miette::Diagnostic; use pacquet_config::{Config, NodeLinker, PackageImportMethod}; use pacquet_deps_path::get_pkg_id_with_patch_hash; use pacquet_lockfile::{ - LockfileResolution, PackageKey, PackageMetadata, PkgIdWithPatchHash, PkgNameVerPeer, + LockfileResolution, PackageKey, PackageMetadata, PkgIdWithPatchHash, PkgName, PkgNameVerPeer, SnapshotEntry, select_platform_variant, }; use pacquet_network::ThrottledClient; @@ -704,6 +704,27 @@ impl CreateVirtualStore<'_> { map }); + // Per-slot obsolete child aliases for the link pass. Only + // survivors that already existed in `current_snapshots` and + // dropped a child contribute an entry; fresh packages and + // addition-only changes map to the empty slice. Computed once + // here so both the warm and cold `SlotLink` batches can borrow + // it. Mirrors the `removedAliases` upstream derives in + // `getChangedChildren` at + // . + let removed_aliases_by_key: HashMap> = match current_snapshots { + Some(current_snapshots) => snapshot_entries + .iter() + .filter_map(|(snapshot_key, snapshot, _)| { + let current_snapshot = current_snapshots.get(*snapshot_key)?; + let removed = + removed_child_aliases(current_snapshot, snapshot, &snapshot_key.name); + (!removed.is_empty()).then(|| ((*snapshot_key).clone(), removed)) + }) + .collect(), + None => HashMap::new(), + }; + let import_method = config.package_import_method; if is_hoisted { // Hoisted still wants the progress reporter to fire so @@ -733,6 +754,7 @@ impl CreateVirtualStore<'_> { snapshot, cas_paths: cas_paths.as_ref(), warm_cache_key: Some(cache_key), + removed_aliases: removed_aliases_for(&removed_aliases_by_key, snapshot_key), }) .collect(); link_slots_parallel::(LinkSlotsParallel { @@ -883,6 +905,7 @@ impl CreateVirtualStore<'_> { snapshot, cas_paths, warm_cache_key: None, + removed_aliases: removed_aliases_for(&removed_aliases_by_key, snapshot_key), }) .collect(); link_slots_parallel::(LinkSlotsParallel { @@ -958,6 +981,46 @@ impl CreateVirtualStore<'_> { } } +/// Look up the obsolete child aliases for a slot, defaulting to an +/// empty slice. The extra indirection lets the `SlotLink` builders +/// pass their multiply-borrowed `snapshot_key` straight through — +/// deref coercion narrows it to `&PackageKey` at the call site. +fn removed_aliases_for<'a>( + removed_aliases_by_key: &'a HashMap>, + snapshot_key: &PackageKey, +) -> &'a [PkgName] { + removed_aliases_by_key.get(snapshot_key).map_or(&[], Vec::as_slice) +} + +/// Child aliases linked by the previous install (`current`) that are +/// absent from the wanted snapshot's `dependencies ∪ +/// optional_dependencies`. The slot's own name is excluded so a +/// self-referential dependency never targets `node_modules/`, +/// the directory the CAS import owns. +fn removed_child_aliases( + current: &SnapshotEntry, + wanted: &SnapshotEntry, + self_name: &PkgName, +) -> Vec { + fn child_aliases(snapshot: &SnapshotEntry) -> impl Iterator { + let deps = snapshot.dependencies.iter().flatten(); + let opt_deps = snapshot.optional_dependencies.iter().flatten(); + deps.chain(opt_deps).map(|(alias, _)| alias) + } + let wanted_aliases: HashSet<&PkgName> = child_aliases(wanted).collect(); + let mut seen: HashSet<&PkgName> = HashSet::new(); + let mut removed = Vec::new(); + for alias in child_aliases(current) { + if alias == self_name || wanted_aliases.contains(alias) { + continue; + } + if seen.insert(alias) { + removed.push(alias.clone()); + } + } + removed +} + fn requires_build_from_cas_paths(cas_paths: &HashMap) -> bool { if files_include_install_scripts(cas_paths.keys()) { return true; @@ -975,6 +1038,10 @@ struct SlotLink<'a> { snapshot: &'a SnapshotEntry, cas_paths: &'a HashMap, warm_cache_key: Option<&'a str>, + /// Child aliases dropped since the previous install, threaded into + /// [`crate::CreateVirtualDirBySnapshot::removed_aliases`] so their + /// stale symlinks are unlinked during the link pass. + removed_aliases: &'a [PkgName], } #[derive(Clone, Copy)] @@ -1032,6 +1099,7 @@ fn link_slots_parallel( package_key: slot.snapshot_key, snapshot: slot.snapshot, skipped, + removed_aliases: slot.removed_aliases, #[cfg(test)] link_concurrency_probe, } diff --git a/pacquet/crates/package-manager/src/create_virtual_store/tests.rs b/pacquet/crates/package-manager/src/create_virtual_store/tests.rs index 4b4de09bfb..0b02e04584 100644 --- a/pacquet/crates/package-manager/src/create_virtual_store/tests.rs +++ b/pacquet/crates/package-manager/src/create_virtual_store/tests.rs @@ -1,6 +1,7 @@ use super::{ CreateVirtualStore, CreateVirtualStoreError, InstallPackageBySnapshotError, - emit_warm_snapshot_progress, integrity_equal, snapshot_cache_key, snapshot_deps_equal, + emit_warm_snapshot_progress, integrity_equal, removed_child_aliases, snapshot_cache_key, + snapshot_deps_equal, }; use pacquet_lockfile::{ GitResolution, LockfileResolution, PackageKey, PackageMetadata, PkgName, PkgVerPeer, @@ -44,6 +45,50 @@ fn snapshot_with_dep(child: &str, ref_str: &str) -> SnapshotEntry { } } +fn dep_map(children: &[&str]) -> Option> { + if children.is_empty() { + return None; + } + // The ref value is irrelevant to `removed_child_aliases`; only the + // alias keys matter. A bare version is the simplest valid ref. + Some(children.iter().map(|child| (name(child), "1.0.0".parse().expect("ref"))).collect()) +} + +fn snapshot(deps: &[&str], optional: &[&str]) -> SnapshotEntry { + SnapshotEntry { + dependencies: dep_map(deps), + optional_dependencies: dep_map(optional), + ..Default::default() + } +} + +#[test] +fn removed_child_aliases_reports_dropped_children_only() { + let self_name = name("host"); + let current = snapshot(&["kept", "dropped"], &["opt-dropped"]); + let wanted = snapshot(&["kept", "added"], &[]); + + let mut removed: Vec = removed_child_aliases(¤t, &wanted, &self_name) + .iter() + .map(PkgName::to_string) + .collect(); + removed.sort(); + + assert_eq!(removed, vec!["dropped".to_string(), "opt-dropped".to_string()]); +} + +#[test] +fn removed_child_aliases_excludes_self_and_unchanged_sets() { + let self_name = name("host"); + // The slot lists itself as a dependency and is otherwise unchanged. + let current = snapshot(&["host", "kept"], &[]); + let wanted = snapshot(&["kept"], &[]); + + let removed = removed_child_aliases(¤t, &wanted, &self_name); + + assert!(removed.is_empty(), "self and still-present children must not be removed: {removed:?}"); +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn cold_batch_links_slots_in_parallel() { use crate::{AllowBuildPolicy, SkippedSnapshots, VirtualStoreLayout}; diff --git a/pacquet/crates/package-manager/src/install_package_by_snapshot.rs b/pacquet/crates/package-manager/src/install_package_by_snapshot.rs index 33c0813e2f..636aabd635 100644 --- a/pacquet/crates/package-manager/src/install_package_by_snapshot.rs +++ b/pacquet/crates/package-manager/src/install_package_by_snapshot.rs @@ -567,6 +567,10 @@ impl InstallPackageBySnapshot<'_> { package_key, snapshot, skipped, + // The non-deferred slot link runs only on the fresh + // single-package path (no previous install to diff + // against), so there are never obsolete children here. + removed_aliases: &[], #[cfg(test)] link_concurrency_probe, }