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, }