Skip to content

Commit 1c05876

Browse files
vsumnerzkochan
vsumner
and
zkochan
authored
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 <z@kochan.io>
1 parent c86559c commit 1c05876

8 files changed

Lines changed: 534 additions & 34 deletions

File tree

‎.changeset/tidy-drinks-help.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
"@pnpm/installing.deps-installer": patch
3+
"pnpm": patch
4+
---
5+
6+
Avoid relinking unchanged child dependencies and remove stale child links during warm installs.

‎installing/deps-installer/src/install/link.ts‎

Lines changed: 144 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,13 @@ import {
77
statsLogger,
88
} from '@pnpm/core-loggers'
99
import { calcDepState, type DepsStateCache, findRuntimeNodeVersion } from '@pnpm/deps.graph-hasher'
10+
import { readModulesDir } from '@pnpm/fs.read-modules-dir'
1011
import { symlinkDependency } from '@pnpm/fs.symlink-dependency'
11-
import type {
12-
DependenciesGraph,
13-
DependenciesGraphNode,
14-
LinkedDependency,
12+
import {
13+
type DependenciesGraph,
14+
type DependenciesGraphNode,
15+
isValidDependencyAlias,
16+
type LinkedDependency,
1517
} from '@pnpm/installing.deps-resolver'
1618
import type { InstallationResultStats } from '@pnpm/installing.deps-restorer'
1719
import { linkDirectDeps } from '@pnpm/installing.linking.direct-dep-linker'
@@ -33,6 +35,7 @@ import type {
3335
SupportedArchitectures,
3436
} from '@pnpm/types'
3537
import { symlinkAllModules } from '@pnpm/worker'
38+
import { rimraf } from '@zkochan/rimraf'
3639
import pLimit from 'p-limit'
3740
import { pathExists } from 'path-exists'
3841
import { difference, equals, isEmpty, pick, pickBy, props } from 'ramda'
@@ -346,6 +349,10 @@ interface LinkNewPackagesResult {
346349
added: number
347350
}
348351

352+
type ModulesLinkJob = Pick<DependenciesGraphNode, 'children' | 'modules' | 'name' | 'optionalDependencies'> & {
353+
removedAliases?: string[]
354+
}
355+
349356
async function linkNewPackages (
350357
currentLockfile: LockfileObject,
351358
wantedLockfile: LockfileObject,
@@ -372,23 +379,55 @@ async function linkNewPackages (
372379
prefix: opts.lockfileDir,
373380
})
374381

375-
const existingWithUpdatedDeps = []
382+
const existingWithUpdatedDeps: ModulesLinkJob[] = []
376383
if (!opts.force && (currentLockfile.packages != null) && (wantedLockfile.packages != null)) {
384+
const currentPackages = currentLockfile.packages
385+
const wantedPackages = wantedLockfile.packages
377386
// add subdependencies that have been updated
378-
// TODO: no need to relink everything. Can be relinked only what was changed
379-
for (const depPath of wantedRelDepPaths) {
380-
if (currentLockfile.packages[depPath] &&
381-
(!equals(currentLockfile.packages[depPath].dependencies, wantedLockfile.packages[depPath].dependencies) ||
382-
!isEmpty(currentLockfile.packages[depPath].optionalDependencies ?? {}) ||
383-
!isEmpty(wantedLockfile.packages[depPath].optionalDependencies ?? {}))
387+
await Promise.all(wantedRelDepPaths.map((depPath) => limitModulesDirReads(async () => {
388+
if (currentPackages[depPath] &&
389+
(!equals(currentPackages[depPath].dependencies, wantedPackages[depPath].dependencies) ||
390+
!isEmpty(currentPackages[depPath].optionalDependencies ?? {}) ||
391+
!isEmpty(wantedPackages[depPath].optionalDependencies ?? {}))
384392
) {
385393
// TODO: come up with a test that triggers the usecase of depGraph[depPath] undefined
386394
// see related issue: https://github.com/pnpm/pnpm/issues/870
387395
if (depGraph[depPath] && !newDepPathsSet.has(depPath)) {
388-
existingWithUpdatedDeps.push(depGraph[depPath])
396+
const { actualChildrenChanged, removedAliases: actualRemovedAliases } = await getActualChildrenDiff(
397+
depGraph[depPath],
398+
depGraph,
399+
opts.lockfileDir,
400+
opts.optional
401+
)
402+
if (actualChildrenChanged) {
403+
existingWithUpdatedDeps.push({
404+
children: depGraph[depPath].children,
405+
modules: depGraph[depPath].modules,
406+
name: depGraph[depPath].name,
407+
optionalDependencies: depGraph[depPath].optionalDependencies,
408+
removedAliases: actualRemovedAliases,
409+
})
410+
return
411+
}
412+
const { changedChildren, removedAliases } = getChangedChildren({
413+
currentDependencies: currentPackages[depPath].dependencies,
414+
currentOptionalDependencies: currentPackages[depPath].optionalDependencies,
415+
wantedDependencies: wantedPackages[depPath].dependencies,
416+
wantedOptionalDependencies: wantedPackages[depPath].optionalDependencies,
417+
allChildren: depGraph[depPath].children,
418+
})
419+
if (!isEmpty(changedChildren) || removedAliases.length > 0) {
420+
existingWithUpdatedDeps.push({
421+
children: changedChildren,
422+
modules: depGraph[depPath].modules,
423+
name: depGraph[depPath].name,
424+
optionalDependencies: depGraph[depPath].optionalDependencies,
425+
removedAliases,
426+
})
427+
}
389428
}
390429
}
391-
}
430+
})))
392431
}
393432

394433
if (!newDepPathsSet.size && (existingWithUpdatedDeps.length === 0)) return { newDepPaths: [], added }
@@ -457,6 +496,7 @@ async function selectNewFromWantedDeps (
457496
}
458497

459498
const limitLinking = pLimit(16)
499+
const limitModulesDirReads = pLimit(16)
460500

461501
async function linkAllPkgs (
462502
storeController: StoreController,
@@ -525,33 +565,109 @@ async function linkAllPkgs (
525565
}
526566

527567
async function linkAllModules (
528-
depNodes: DependenciesGraphNode[],
568+
depNodes: ModulesLinkJob[],
529569
depGraph: DependenciesGraph,
530570
opts: {
531571
lockfileDir: string
532572
optional: boolean
533573
}
534574
): Promise<void> {
575+
await Promise.all(depNodes.flatMap((depNode) => (depNode.removedAliases ?? []).map(async (alias) => limitModulesDirReads(async () => removeObsoleteChild(depNode.modules, alias)))))
535576
await symlinkAllModules({
536577
deps: depNodes.map((depNode) => {
537-
const children = opts.optional
538-
? depNode.children
539-
: pickBy((_, childAlias) => !depNode.optionalDependencies.has(childAlias), depNode.children)
540-
const childrenPaths: Record<string, string> = {}
541-
for (const [alias, childDepPath] of Object.entries(children ?? {})) {
542-
if (childDepPath.startsWith('link:')) {
543-
childrenPaths[alias] = path.resolve(opts.lockfileDir, childDepPath.slice(5))
544-
} else {
545-
const pkg = depGraph[childDepPath]
546-
if (!pkg || !pkg.installable && pkg.optional || alias === depNode.name) continue
547-
childrenPaths[alias] = pkg.dir
548-
}
549-
}
550578
return {
551-
children: childrenPaths,
579+
children: getChildrenPaths(depNode, depGraph, opts.lockfileDir, opts.optional),
552580
modules: depNode.modules,
553581
name: depNode.name,
554582
}
555583
}),
556584
})
557585
}
586+
587+
function getChangedChildren (
588+
opts: {
589+
currentDependencies: Record<string, string> | undefined
590+
currentOptionalDependencies: Record<string, string> | undefined
591+
wantedDependencies: Record<string, string> | undefined
592+
wantedOptionalDependencies: Record<string, string> | undefined
593+
allChildren: Record<string, DepPath>
594+
}
595+
): { changedChildren: Record<string, DepPath>, removedAliases: string[] } {
596+
const { currentOptionalDependencies, wantedOptionalDependencies, allChildren } = opts
597+
// Use null-prototype maps so a child literally named `constructor`,
598+
// `toString`, `__proto__`, etc. is treated as a normal alias instead
599+
// of colliding with an inherited `Object.prototype` key during the
600+
// `in`/index lookups below.
601+
const currentChildren: Record<string, string> = Object.assign(Object.create(null), opts.currentDependencies, currentOptionalDependencies)
602+
const wantedChildren: Record<string, string> = Object.assign(Object.create(null), opts.wantedDependencies, wantedOptionalDependencies)
603+
const changedChildren: Record<string, DepPath> = {}
604+
const removedAliases: string[] = []
605+
for (const [alias, wantedChildDepPath] of Object.entries(wantedChildren)) {
606+
const optionalityChanged = hasOwn(wantedOptionalDependencies, alias) !== hasOwn(currentOptionalDependencies, alias)
607+
if (currentChildren[alias] !== wantedChildDepPath || optionalityChanged) {
608+
const resolvedChildDepPath = hasOwn(allChildren, alias) ? allChildren[alias] : undefined
609+
if (resolvedChildDepPath != null) {
610+
changedChildren[alias] = resolvedChildDepPath
611+
}
612+
}
613+
}
614+
for (const alias of Object.keys(currentChildren)) {
615+
if (!hasOwn(wantedChildren, alias)) {
616+
removedAliases.push(alias)
617+
}
618+
}
619+
return { changedChildren, removedAliases }
620+
}
621+
622+
function hasOwn (obj: Record<string, unknown> | undefined, key: string): boolean {
623+
return obj != null && Object.hasOwn(obj, key)
624+
}
625+
626+
async function getActualChildrenDiff (
627+
depNode: ModulesLinkJob,
628+
depGraph: DependenciesGraph,
629+
lockfileDir: string,
630+
optional: boolean
631+
): Promise<{ actualChildrenChanged: boolean, removedAliases: string[] }> {
632+
if (depNode.optionalDependencies.size === 0) {
633+
return { actualChildrenChanged: false, removedAliases: [] }
634+
}
635+
const currentAliases = new Set((await readModulesDir(depNode.modules) ?? []).filter((alias) => alias !== depNode.name))
636+
const nextAliases = new Set(Object.keys(getChildrenPaths(depNode, depGraph, lockfileDir, optional)))
637+
const removedAliases = Array.from(currentAliases).filter((alias) => !nextAliases.has(alias))
638+
const actualChildrenChanged = removedAliases.length > 0 ||
639+
Array.from(nextAliases).some((alias) => !currentAliases.has(alias))
640+
return { actualChildrenChanged, removedAliases }
641+
}
642+
643+
async function removeObsoleteChild (modulesDir: string, alias: string): Promise<void> {
644+
// Guard against an alias that would escape the modules directory (e.g. `../../x`).
645+
if (!isValidDependencyAlias(alias)) return
646+
await rimraf(path.join(modulesDir, alias))
647+
if (alias[0] === '@') {
648+
await fs.rmdir(path.join(modulesDir, alias.split('/')[0])).catch(() => {})
649+
}
650+
}
651+
652+
function getChildrenPaths (
653+
depNode: ModulesLinkJob,
654+
depGraph: DependenciesGraph,
655+
lockfileDir: string,
656+
optional: boolean
657+
): Record<string, string> {
658+
const children = optional
659+
? depNode.children
660+
: pickBy((_, childAlias) => !depNode.optionalDependencies.has(childAlias), depNode.children)
661+
const childrenPaths: Record<string, string> = {}
662+
for (const [alias, childDepPath] of Object.entries(children ?? {})) {
663+
if (alias === depNode.name) continue
664+
if (childDepPath.startsWith('link:')) {
665+
childrenPaths[alias] = path.resolve(lockfileDir, childDepPath.slice(5))
666+
continue
667+
}
668+
const pkg = depGraph[childDepPath]
669+
if (!pkg || !pkg.installable && pkg.optional) continue
670+
childrenPaths[alias] = pkg.dir
671+
}
672+
return childrenPaths
673+
}

‎installing/deps-installer/test/install/relinkingScope.test.ts‎

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
import fs from 'node:fs'
2+
import path from 'node:path'
3+
4+
import { expect, jest, test } from '@jest/globals'
5+
import { prepare } from '@pnpm/prepare'
6+
import type { ProjectManifest, ProjectRootDir } from '@pnpm/types'
7+
8+
import { testDefaults } from '../utils/index.js'
9+
10+
const symlinkAllModulesCalls: Array<Array<{ name: string, children: string[] }>> = []
11+
12+
const originalWorker = await import('@pnpm/worker')
13+
jest.unstable_mockModule('@pnpm/worker', () => ({
14+
...originalWorker,
15+
symlinkAllModules: jest.fn(async (opts: Parameters<typeof originalWorker.symlinkAllModules>[0]) => {
16+
symlinkAllModulesCalls.push(
17+
opts.deps.map((dep) => ({
18+
name: dep.name,
19+
children: Object.keys(dep.children).sort(),
20+
}))
21+
)
22+
return originalWorker.symlinkAllModules(opts)
23+
}),
24+
}))
25+
26+
const { mutateModulesInSingleProject } = await import('@pnpm/installing.deps-installer')
27+
28+
test('relinks only changed child edges for existing packages after dependency updates', async () => {
29+
const manifest: ProjectManifest = {
30+
dependencies: {
31+
'@pnpm.e2e/pkg-with-good-optional': '1.0.0',
32+
},
33+
}
34+
const project = prepare(manifest)
35+
36+
// Pin the child to an older version first so the update below is a
37+
// real edge change. With the fixture's `*` range both installs would
38+
// otherwise resolve to the same highest version and nothing relinks.
39+
const initialOpts = testDefaults({
40+
overrides: {
41+
'@pnpm.e2e/dep-of-pkg-with-1-dep': '100.0.0',
42+
},
43+
})
44+
await mutateModulesInSingleProject({
45+
manifest,
46+
mutation: 'install',
47+
rootDir: process.cwd() as ProjectRootDir,
48+
}, initialOpts)
49+
50+
symlinkAllModulesCalls.length = 0
51+
52+
const updatedOpts = testDefaults({
53+
overrides: {
54+
'@pnpm.e2e/dep-of-pkg-with-1-dep': '101.0.0',
55+
},
56+
storeDir: initialOpts.storeDir,
57+
})
58+
await mutateModulesInSingleProject({
59+
manifest,
60+
mutation: 'install',
61+
rootDir: process.cwd() as ProjectRootDir,
62+
}, updatedOpts)
63+
64+
const lockfile = project.readLockfile()
65+
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')
66+
expect(lockfile.snapshots['@pnpm.e2e/pkg-with-good-optional@1.0.0'].optionalDependencies?.['is-positive']).toBe('1.0.0')
67+
68+
const pkgModulesDir = path.resolve('node_modules/.pnpm/@pnpm.e2e+pkg-with-good-optional@1.0.0/node_modules')
69+
expect(fs.realpathSync(path.join(pkgModulesDir, '@pnpm.e2e/dep-of-pkg-with-1-dep'))).toContain('101.0.0')
70+
71+
const pkgCalls = symlinkAllModulesCalls
72+
.flat()
73+
.filter((dep) => dep.name === '@pnpm.e2e/pkg-with-good-optional')
74+
75+
// The package must actually be relinked for the changed child edge,
76+
// otherwise the `every` assertion below would pass vacuously on an
77+
// empty array and prove nothing.
78+
expect(pkgCalls.some(({ children }) => children.includes('@pnpm.e2e/dep-of-pkg-with-1-dep'))).toBe(true)
79+
80+
// Existing packages with only one changed child edge should not be passed
81+
// through the broad worker relinking path with unchanged aliases.
82+
expect(pkgCalls.every(({ children }) => !children.includes('is-positive'))).toBe(true)
83+
})
84+
85+
test('removes obsolete child links for existing packages after dependency updates', async () => {
86+
const manifest: ProjectManifest = {
87+
dependencies: {
88+
'@pnpm.e2e/pkg-with-good-optional': '1.0.0',
89+
},
90+
}
91+
prepare(manifest)
92+
93+
const initialOpts = testDefaults()
94+
await mutateModulesInSingleProject({
95+
manifest,
96+
mutation: 'install',
97+
rootDir: process.cwd() as ProjectRootDir,
98+
}, initialOpts)
99+
100+
const obsoleteChildPath = path.resolve('node_modules/.pnpm/@pnpm.e2e+pkg-with-good-optional@1.0.0/node_modules/is-positive')
101+
expect(fs.existsSync(obsoleteChildPath)).toBe(true)
102+
103+
const updatedOpts = testDefaults({
104+
overrides: {
105+
'@pnpm.e2e/pkg-with-good-optional>is-positive': '-',
106+
},
107+
storeDir: initialOpts.storeDir,
108+
})
109+
await mutateModulesInSingleProject({
110+
manifest,
111+
mutation: 'install',
112+
rootDir: process.cwd() as ProjectRootDir,
113+
}, updatedOpts)
114+
115+
expect(fs.existsSync(obsoleteChildPath)).toBe(false)
116+
})

0 commit comments

Comments
 (0)