From 8bd66b2574bed0a4e8cc1c748d5f57da313def7a Mon Sep 17 00:00:00 2001 From: Manzoor Wani Date: Thu, 25 Jun 2026 01:33:15 +0530 Subject: [PATCH] fix(arborist): clean up stale .store and hoisted dirs on strategy switch (#9647) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In continuation of our exploration of using `install-strategy=linked` in the [Gutenberg monorepo](https://github.com/WordPress/gutenberg/pull/75814), which powers the WordPress Block Editor. Switching `install-strategy` in the same project directory left behind the previous strategy's layout. Going hoisted → linked kept the stale real top-level transitive directories alongside the new `.store/` and symlinks; going linked → hoisted kept the entire `node_modules/.store/` directory. A fresh install of either strategy was already clean — only the switch was affected. ## Why Under the linked strategy the actual tree the diff compares against is synthesized from the ideal tree (`#buildLinkedActualForDiff`), so real directories left over from a prior hoisted layout are never seen, and `#cleanOrphanedTopLevelLinks` only removed symlinks. In the other direction `load-actual` ignores dot-directories, so the hoisted diff never sees `node_modules/.store` and never removes it. ## How `reify.js` now removes the leftover `.store` on a non-linked reify via `#removeStaleStoreDir`. The store lives only at the project root and is exclusively a linked artifact, so a single removal covers the project. It runs only for a full-project install — a workspace-filtered or `--workspaces=false` install is skipped, because out-of-scope workspaces may still link into the store. `#cleanOrphanedTopLevelLinks` (run only under linked) additionally removes stale real package directories — a directory containing a `package.json` that is not in the ideal tree's valid top-level set — and prunes an emptied `@scope` directory afterward. Non-package real directories and symlinks pointing outside the project are still preserved. The valid-top-level collection in `#cleanOrphanedStoreEntries` no longer skips non-link nodes, so the root's bundled dependencies — materialized as real top-level directories under linked — are recorded as valid and never swept as stale. ## References Fixes #9615 Part of #9608 (cherry picked from commit ca923237e973313b8b1cdf8c9e0dcfdb2ee875a6) --- workspaces/arborist/lib/arborist/reify.js | 66 +++++++++++++--- workspaces/arborist/test/arborist/reify.js | 92 ++++++++++++++++++++++ 2 files changed, 147 insertions(+), 11 deletions(-) diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index 0c7b4fd0acfbf..1f90c117a45cd 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -144,6 +144,14 @@ module.exports = cls => class Reifier extends cls { if (!this.options.dryRun && !this.options.packageLockOnly) { await this.#cleanOrphanedStoreEntries() } + } else if (!this.options.dryRun && !this.options.packageLockOnly) { + // The .store directory is exclusively a linked-strategy artifact, and load-actual ignores dot-directories, so the diff never sees it. + // Remove it here when switching away from linked so it does not linger under hoisted/nested. + // Only do this for a full-project install: a workspace-filtered or --workspaces=false install may leave out-of-scope workspaces with links still pointing into the store. + const filtered = this.options.workspaces.length > 0 || !this.options.workspacesEnabled + if (!filtered) { + await this.#removeStaleStoreDir() + } } } finally { // Restore the non-isolated tree so the lockfile is preserved and a reused Arborist never sees the isolated tree, even if reify throws. @@ -1438,6 +1446,18 @@ module.exports = cls => class Reifier extends cls { timeEnd() } + // Remove the root .store left behind by a previous linked install when reifying under a non-linked strategy. + async #removeStaleStoreDir () { + const storeDir = resolve(this.path, 'node_modules', '.store') + if (!existsSync(storeDir)) { + return + } + log.silly('reify', 'removing stale .store from a previous linked install') + await rm(storeDir, { recursive: true, force: true }) + .catch(/* istanbul ignore next -- rm with force rarely fails */ + er => log.warn('cleanup', 'Failed to remove stale .store directory', er)) + } + // After a linked install, scan node_modules/.store/ and remove any directories that are not referenced by the current ideal tree. // Store entries become orphaned when dependencies are updated or removed, because the diff never sees the old store keys. // Then sweep the top-level node_modules/ for orphaned symlinks (e.g. an uninstalled dep whose store entry was just removed) so we don't leave dangling links. @@ -1476,7 +1496,7 @@ module.exports = cls => class Reifier extends cls { // Locations are normalized to forward slashes here because IsolatedNode/IsolatedLink locations are built with path.join, which uses backslashes on Windows. const validKeys = new Set() const nmDirs = new Map() - // Valid bin shim names per node_modules dir, collected from each top-level link's package.bin so the .bin sweep keeps only shims a still-linked package provides. + // Valid bin shim names per node_modules dir, collected from each top-level entry's package.bin so the .bin sweep keeps only shims a still-installed package provides. const binsByDir = new Map() const NM_PREFIX = 'node_modules/' const STORE_MARKER = '/.store/' @@ -1490,13 +1510,11 @@ module.exports = cls => class Reifier extends cls { validKeys.add(key) continue } - if (!child.isLink) { - continue - } // Tree-only Links never exist on disk; skipping them lets the sweep remove any stale self-link left by an older npm version. - if (child.isUndeclaredWorkspaceLink) { + if (child.isLink && child.isUndeclaredWorkspaceLink) { continue } + // Real top-level Nodes (e.g. the root's bundled deps) fall through here too, so they are recorded as valid and never swept as stale. const nmIdx = loc.lastIndexOf(NM_PREFIX) if (nmIdx === -1 || loc.includes(STORE_MARKER)) { continue @@ -1640,8 +1658,10 @@ module.exports = cls => class Reifier extends cls { // Remove node_modules/ entries that aren't represented in the ideal tree. // Run for the project root and each workspace's node_modules. // The linked diff path can't see these because #buildLinkedActualForDiff derives the actual tree from the ideal, so removed deps are never compared. - // Only symlinks whose target resolves inside the project root are removed — that covers store links (node_modules/.store/...) and workspace self-links (e.g. node_modules/ -> ../packages/) that npm itself created. - // Symlinks pointing outside the project (e.g. `npm link foo` without --save targeting the global prefix, or hand-made `ln -s` to an external path) and real directories are preserved. + // Two kinds of stale entry are removed: + // - symlinks whose target resolves inside the project root — store links (node_modules/.store/...) and workspace self-links (e.g. node_modules/ -> ../packages/) that npm itself created. + // - real package directories — hoisted-layout deps left behind when switching from the hoisted strategy to linked, where every valid top-level entry is a symlink. + // Symlinks pointing outside the project (e.g. `npm link foo` without --save targeting the global prefix, or hand-made `ln -s` to an external path) and non-package real directories are preserved. async #cleanOrphanedTopLevelLinks (nmDir, validTopLevel) { const projectPrefix = resolve(this.path) + sep let dirents @@ -1662,7 +1682,15 @@ module.exports = cls => class Reifier extends cls { return resolve(dirname(linkPath), target).startsWith(projectPrefix) } + // A real directory is stale only when it is an actual package (has a package.json), so unrelated user directories are never touched. + const isStaleRealPkg = (dirent, entPath) => + dirent.isDirectory() && existsSync(resolve(entPath, 'package.json')) + + const isOrphan = async (dirent, entPath) => + (dirent.isSymbolicLink() && await isOurOrphan(entPath)) || isStaleRealPkg(dirent, entPath) + const orphaned = [] + const scopes = new Set() for (const ent of dirents) { // skip npm-managed entries (.bin, .store, .package-lock.json, etc) if (ent.name.startsWith('.')) { @@ -1678,11 +1706,12 @@ module.exports = cls => class Reifier extends cls { } for (const pkgEnt of scoped) { const key = `${ent.name}${sep}${pkgEnt.name}` - if (!validTopLevel.has(key) && pkgEnt.isSymbolicLink() && await isOurOrphan(resolve(nmDir, key))) { + if (!validTopLevel.has(key) && await isOrphan(pkgEnt, resolve(nmDir, key))) { orphaned.push(key) + scopes.add(ent.name) } } - } else if (!validTopLevel.has(ent.name) && ent.isSymbolicLink() && await isOurOrphan(resolve(nmDir, ent.name))) { + } else if (!validTopLevel.has(ent.name) && await isOrphan(ent, resolve(nmDir, ent.name))) { orphaned.push(ent.name) } } @@ -1691,14 +1720,29 @@ module.exports = cls => class Reifier extends cls { return } - log.silly('reify', 'cleaning orphaned top-level links', orphaned) + log.silly('reify', 'cleaning orphaned top-level entries', orphaned) await promiseAllRejectLate( orphaned.map(name => rm(resolve(nmDir, name), { recursive: true, force: true }) .catch(/* istanbul ignore next -- rm with force rarely fails */ - er => log.warn('cleanup', `Failed to remove orphaned link ${name}`, er)) + er => log.warn('cleanup', `Failed to remove orphaned entry ${name}`, er)) ) ) + + // Removing the last package under a scope leaves an empty @scope directory behind, so prune any scope directory that is now empty. + await promiseAllRejectLate( + [...scopes].map(async scope => { + const scopeDir = resolve(nmDir, scope) + try { + const remaining = await readdir(scopeDir) + if (!remaining.length) { + await rm(scopeDir, { recursive: true, force: true }) + } + } catch { + /* istanbul ignore next -- readdir of a scope dir we just listed should not fail */ + } + }) + ) } // last but not least, we save the ideal tree metadata to the package-lock diff --git a/workspaces/arborist/test/arborist/reify.js b/workspaces/arborist/test/arborist/reify.js index 42bd8f5941aa5..bfc8fd5937a3e 100644 --- a/workspaces/arborist/test/arborist/reify.js +++ b/workspaces/arborist/test/arborist/reify.js @@ -4512,6 +4512,98 @@ t.test('install strategy linked', async (t) => { t.ok(fs.lstatSync(scmd), 'surviving semver.cmd kept') t.ok(fs.lstatSync(sps1), 'surviving semver.ps1 kept') }) + + t.test('switching hoisted -> linked removes stale real top-level dirs', async t => { + // Regression test for https://github.com/npm/cli/issues/9615 + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'sw', version: '1.0.0', dependencies: { minimatch: '3.0.4' }, + }), + }) + createRegistry(t, true) + + // A hoisted install lays the transitive deps out as real top-level dirs. + await reify(path, { installStrategy: 'hoisted' }) + const nm = resolve(path, 'node_modules') + for (const dep of ['balanced-match', 'brace-expansion', 'concat-map']) { + t.ok(fs.statSync(resolve(nm, dep)).isDirectory(), `${dep} is a real dir under hoisted`) + } + + // Plant a stale scoped real package to cover the scoped removal and empty-scope pruning path. + const scopedPkg = resolve(nm, '@scope/stale') + fs.mkdirSync(scopedPkg, { recursive: true }) + fs.writeFileSync(resolve(scopedPkg, 'package.json'), + JSON.stringify({ name: '@scope/stale', version: '1.0.0' })) + // A non-package real dir must be preserved. + fs.mkdirSync(resolve(nm, 'not-a-package'), { recursive: true }) + + // Switching to linked must remove those stale real dirs, leaving only the symlink + .store. + await reify(path, { installStrategy: 'linked' }) + for (const dep of ['balanced-match', 'brace-expansion', 'concat-map']) { + t.notOk(fs.existsSync(resolve(nm, dep)), `${dep} stale real dir removed after switch to linked`) + } + t.notOk(fs.existsSync(scopedPkg), 'stale scoped real package removed') + t.notOk(fs.existsSync(resolve(nm, '@scope')), 'emptied scope dir pruned') + t.ok(fs.existsSync(resolve(nm, 'not-a-package')), 'non-package real dir preserved') + t.ok(fs.lstatSync(resolve(nm, 'minimatch')).isSymbolicLink(), 'minimatch is a store symlink') + t.ok(fs.statSync(resolve(nm, '.store')).isDirectory(), '.store created') + }) + + t.test('switching linked -> hoisted removes the stale .store dir', async t => { + // Regression test for https://github.com/npm/cli/issues/9615 + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'sw', version: '1.0.0', dependencies: { minimatch: '3.0.4' }, + }), + }) + createRegistry(t, true) + + await reify(path, { installStrategy: 'linked' }) + const nm = resolve(path, 'node_modules') + t.ok(fs.statSync(resolve(nm, '.store')).isDirectory(), '.store created under linked') + + // A hoisted install must not leave the linked store behind. + await reify(path, { installStrategy: 'hoisted' }) + t.notOk(fs.existsSync(resolve(nm, '.store')), '.store removed after switch to hoisted') + t.ok(fs.statSync(resolve(nm, 'balanced-match')).isDirectory(), 'transitive dep hoisted to a real dir') + }) + + t.test('a partial hoisted install does not wipe a still-referenced linked .store', async t => { + // Regression test for https://github.com/npm/cli/issues/9615 + // A workspace-filtered or --workspaces=false hoisted install must not remove the root .store, since out-of-scope workspaces still link into it. + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'root', version: '1.0.0', workspaces: ['packages/*'], + }), + packages: { + // a is the out-of-scope workspace that keeps a live link into the store. + a: { 'package.json': JSON.stringify({ name: 'a', version: '1.0.0', dependencies: { minimatch: '3.0.4' } }) }, + b: { 'package.json': JSON.stringify({ name: 'b', version: '1.0.0' }) }, + }, + }) + createRegistry(t, true) + const nm = resolve(path, 'node_modules') + const aLink = resolve(path, 'packages/a/node_modules/minimatch') + // a's dep resolves through the root .store, so deleting the store would break it. + const stillLinked = msg => t.ok( + fs.lstatSync(aLink).isSymbolicLink() && fs.existsSync(fs.realpathSync(aLink)), msg) + + await reify(path, { installStrategy: 'linked' }) + t.ok(fs.statSync(resolve(nm, '.store')).isDirectory(), '.store created under linked') + t.match(fs.realpathSync(aLink), /node_modules[\\/]\.store[\\/]/, 'workspace a links into the store') + + // Filter to workspace b: a is out of scope and must keep its live store link. + await reify(path, { installStrategy: 'hoisted', workspaces: ['b'] }) + t.ok(fs.existsSync(resolve(nm, '.store')), '.store kept during a workspace-filtered install') + stillLinked('workspace a still resolves through the store after the filtered install') + + await reify(path, { installStrategy: 'hoisted', workspacesEnabled: false }) + t.ok(fs.existsSync(resolve(nm, '.store')), '.store kept during a --workspaces=false install') + stillLinked('workspace a still resolves through the store after the --workspaces=false install') + + await reify(path, { installStrategy: 'hoisted' }) + t.notOk(fs.existsSync(resolve(nm, '.store')), '.store removed by a full hoisted install') + }) }) t.test('linked strategy --workspaces=false and --include-workspace-root do not crash', async t => {