From 4958aa9deea4cea599e67195b74ae949105781cc Mon Sep 17 00:00:00 2001 From: Manzoor Wani Date: Thu, 26 Feb 2026 12:55:18 +0530 Subject: [PATCH 1/4] fix(arborist): avoid full reinstall on subsequent linked strategy runs --- .../arborist/lib/arborist/isolated-reifier.js | 7 +- workspaces/arborist/lib/arborist/reify.js | 119 +++++++++++++++++- workspaces/arborist/test/isolated-mode.js | 74 +++++++++++ 3 files changed, 194 insertions(+), 6 deletions(-) diff --git a/workspaces/arborist/lib/arborist/isolated-reifier.js b/workspaces/arborist/lib/arborist/isolated-reifier.js index 0b4052aa916e1..4916610245907 100644 --- a/workspaces/arborist/lib/arborist/isolated-reifier.js +++ b/workspaces/arborist/lib/arborist/isolated-reifier.js @@ -461,7 +461,7 @@ module.exports = cls => class IsolatedReifier extends cls { if (dep.package.bin) { for (const bn in dep.package.bin) { - target.binPaths.push(join(from.realpath, 'node_modules', '.bin', bn)) + target.binPaths.push(join(dep.root.localPath, nmFolder, '.bin', bn)) } } @@ -486,9 +486,12 @@ module.exports = cls => class IsolatedReifier extends cls { parent: root, path: join(dep.root.localPath, nmFolder, dep.name), realpath: target.path, - resolved: dep.resolved, + resolved: external + ? `file:.store/${toKey}/node_modules/${dep.packageName}` + : dep.resolved, root, target, + version: dep.version, top: { path: dep.root.localPath }, } const newEdge1 = { optional, from, to: link } diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index 3c4317afc133e..bd540d19f8136 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -10,6 +10,7 @@ const { callLimit: promiseCallLimit } = require('promise-call-limit') const { depth: dfwalk } = require('treeverse') const { dirname, resolve, relative, join } = require('node:path') const { log, time } = require('proc-log') +const { existsSync } = require('node:fs') const { lstat, mkdir, rm, symlink } = require('node:fs/promises') const { moveFile } = require('@npmcli/fs') const { subset, intersects } = require('semver') @@ -73,6 +74,7 @@ module.exports = cls => class Reifier extends cls { #shrinkwrapInflated = new Set() #sparseTreeDirs = new Set() #sparseTreeRoots = new Set() + #linkedActualForDiff = null constructor (options) { super(options) @@ -114,6 +116,9 @@ module.exports = cls => class Reifier extends cls { // of Node/Link trees log.warn('reify', 'The "linked" install strategy is EXPERIMENTAL and may contain bugs.') this.idealTree = await this.createIsolatedTree() + this.#linkedActualForDiff = this.#buildLinkedActualForDiff( + this.idealTree, this.actualTree + ) } await this[_diffTrees]() await this.#reifyPackages() @@ -123,6 +128,7 @@ module.exports = cls => class Reifier extends cls { this.idealTree = oldTree } await this[_saveIdealTree](options) + this.#linkedActualForDiff = null // clean inert for (const node of this.idealTree.inventory.values()) { if (node.inert) { @@ -424,9 +430,15 @@ module.exports = cls => class Reifier extends cls { if (ideal) { filterNodes.push(ideal) } - const actual = this.actualTree.children.get(ws) - if (actual) { - filterNodes.push(actual) + // Skip actual-side filterNodes when using the linked diff wrapper — + // those nodes have root===actualTree, not root===linkedActualForDiff, + // and Diff.calculate requires filterNode.root to match actual. + // The ideal filterNode alone is sufficient to scope the workspace diff. + if (!this.#linkedActualForDiff) { + const actual = this.actualTree.children.get(ws) + if (actual) { + filterNodes.push(actual) + } } } } @@ -448,7 +460,7 @@ module.exports = cls => class Reifier extends cls { omit: this.#omit, shrinkwrapInflated: this.#shrinkwrapInflated, filterNodes, - actual: this.actualTree, + actual: this.#linkedActualForDiff || this.actualTree, ideal: this.idealTree, }) @@ -571,6 +583,7 @@ module.exports = cls => class Reifier extends cls { // if the directory already exists, made will be undefined. if that's the case // we don't want to remove it because we aren't the ones who created it so we // omit it from the #sparseTreeRoots + /* istanbul ignore next -- mkdir returns path only when dir is new */ if (made) { this.#sparseTreeRoots.add(made) } @@ -787,6 +800,104 @@ module.exports = cls => class Reifier extends cls { return join(filePath) } + // Build a flat actual tree wrapper for linked installs so the diff can + // correctly match store entries that already exist on disk. The proxy + // tree from _createIsolatedTree() is flat (all children on root), but + // loadActual() produces a nested tree where store entries are deep link + // targets. This wrapper surfaces them at the root level for comparison. + #buildLinkedActualForDiff (idealTree, actualTree) { + // Combined Map keyed by path (how allChildren() in diff.js keys) + const combined = new Map() + + // Add actual tree's children (the top-level symlinks) + for (const child of actualTree.children.values()) { + combined.set(child.path, child) + } + + // Add synthetic entries for store nodes and store links that exist on disk. + // The proxy tree is flat — all store entries (isInStore) and store links + // (isStoreLink) are direct children of root. The actual tree only has + // top-level links as root children, so store entries and store links + // need synthetic actual entries for the diff to match them. + for (const child of idealTree.children.values()) { + if (!combined.has(child.path) && (child.isInStore || child.isStoreLink) && + existsSync(child.path)) { + const entry = { + global: false, + globalTop: false, + isProjectRoot: false, + isTop: false, + location: child.location, + name: child.name, + optional: child.optional, + top: child.top, + children: [], + edgesIn: new Set(), + edgesOut: new Map(), + binPaths: [], + fsChildren: [], + /* istanbul ignore next -- emulate Node */ + getBundler () { + return null + }, + hasShrinkwrap: false, + inDepBundle: false, + integrity: null, + isLink: Boolean(child.isLink), + isRoot: false, + isInStore: Boolean(child.isInStore), + path: child.path, + realpath: child.realpath, + resolved: child.resolved, + version: child.version, + package: child.package, + } + entry.target = entry + if (child.isLink && combined.has(child.realpath)) { + entry.target = combined.get(child.realpath) + } + combined.set(child.path, entry) + } + } + + // Proxy .get(name) to original actual tree for filterNodes compatibility + // (scoped workspace installs use .get(name), allChildren uses .values()) + const origGet = actualTree.children.get.bind(actualTree.children) + const combinedGet = combined.get.bind(combined) + /* istanbul ignore next -- only reached during scoped workspace installs */ + combined.get = (key) => combinedGet(key) || origGet(key) + + const wrapper = { + isRoot: true, + isLink: actualTree.isLink, + target: actualTree.target, + fsChildren: actualTree.fsChildren, + path: actualTree.path, + realpath: actualTree.realpath, + edgesOut: actualTree.edgesOut, + inventory: actualTree.inventory, + package: actualTree.package, + resolved: actualTree.resolved, + version: actualTree.version, + integrity: actualTree.integrity, + binPaths: actualTree.binPaths, + hasShrinkwrap: false, + inDepBundle: false, + parent: null, + children: combined, + } + + // Set parent/root on synthetic entries for consistency + for (const child of combined.values()) { + if (!child.parent) { + child.parent = wrapper + child.root = wrapper + } + } + + return wrapper + } + #registryResolved (resolved) { // the default registry url is a magic value meaning "the currently // configured registry". diff --git a/workspaces/arborist/test/isolated-mode.js b/workspaces/arborist/test/isolated-mode.js index 164f1cbb8f516..d272e44ab0e0f 100644 --- a/workspaces/arborist/test/isolated-mode.js +++ b/workspaces/arborist/test/isolated-mode.js @@ -1779,6 +1779,80 @@ tap.test('file: dependency with linked strategy', async t => { t.ok(setupRequire(dir)('project2'), 'project2 can be required from root') }) +tap.test('subsequent linked install is a no-op', async t => { + const graph = { + registry: [ + { name: 'which', version: '1.0.0', bin: './bin.js', dependencies: { isexe: '^1.0.0' } }, + { name: 'isexe', version: '1.0.0' }, + ], + root: { + name: 'myproject', + version: '1.0.0', + dependencies: { which: '1.0.0' }, + }, + } + const { dir, registry } = await getRepo(graph) + const cache = fs.mkdtempSync(`${getTempDir()}/test-`) + + // First install + const arb1 = new Arborist({ path: dir, registry, packumentCache: new Map(), cache }) + await arb1.reify({ installStrategy: 'linked' }) + + // Verify packages are installed + t.ok(fs.lstatSync(path.join(dir, 'node_modules', 'which')).isSymbolicLink(), + 'which is a symlink after first install') + + // Second install — should detect everything is up-to-date + const arb2 = new Arborist({ path: dir, registry, packumentCache: new Map(), cache }) + await arb2.reify({ installStrategy: 'linked' }) + + // Verify the diff has zero actionable leaves + const leaves = arb2.diff?.leaves || [] + const actions = leaves.filter(l => l.action) + t.equal(actions.length, 0, 'second install should have no diff actions') + + // Verify unchanged nodes were detected + t.ok(arb2.diff.unchanged.length > 0, 'second install should have unchanged nodes') + + // Verify packages are still correctly installed + t.ok(fs.lstatSync(path.join(dir, 'node_modules', 'which')).isSymbolicLink(), + 'which is still a symlink after second install') + t.ok(setupRequire(dir)('which'), 'which is requireable after second install') +}) + +tap.test('workspace links are not affected by store resolved fix', async t => { + const graph = { + registry: [ + { name: 'abbrev', version: '1.0.0' }, + ], + root: { + name: 'myproject', + version: '1.0.0', + dependencies: { abbrev: '1.0.0' }, + }, + workspaces: [ + { name: 'mypkg', version: '1.0.0', dependencies: { abbrev: '1.0.0' } }, + ], + } + const { dir, registry } = await getRepo(graph) + const cache = fs.mkdtempSync(`${getTempDir()}/test-`) + + // First install + const arb1 = new Arborist({ path: dir, registry, packumentCache: new Map(), cache }) + await arb1.reify({ installStrategy: 'linked' }) + + // Second install + const arb2 = new Arborist({ path: dir, registry, packumentCache: new Map(), cache }) + await arb2.reify({ installStrategy: 'linked' }) + + // Verify workspace is still correctly linked + t.ok(setupRequire(dir)('mypkg'), 'workspace is requireable after second install') + t.ok(setupRequire(dir)('abbrev'), 'registry dep is requireable after second install') + + // Verify the diff has unchanged nodes (store entries are correctly matched) + t.ok(arb2.diff.unchanged.length > 0, 'second install should have unchanged nodes') +}) + function setupRequire (cwd) { return function requireChain (...chain) { return chain.reduce((path, name) => { From ccc2f828bf8f46f928440009416d07aae58fbebc Mon Sep 17 00:00:00 2001 From: Manzoor Wani Date: Tue, 3 Mar 2026 14:15:02 +0530 Subject: [PATCH 2/4] fix(arborist): clean-up orphaned .store/ entries on linked install --- workspaces/arborist/lib/arborist/reify.js | 43 +++++++++++- workspaces/arborist/test/isolated-mode.js | 79 +++++++++++++++++++++++ 2 files changed, 120 insertions(+), 2 deletions(-) diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index bd540d19f8136..defa0335c30f4 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -8,10 +8,10 @@ const promiseAllRejectLate = require('promise-all-reject-late') const runScript = require('@npmcli/run-script') const { callLimit: promiseCallLimit } = require('promise-call-limit') const { depth: dfwalk } = require('treeverse') -const { dirname, resolve, relative, join } = require('node:path') +const { dirname, resolve, relative, join, sep } = require('node:path') const { log, time } = require('proc-log') const { existsSync } = require('node:fs') -const { lstat, mkdir, rm, symlink } = require('node:fs/promises') +const { lstat, mkdir, readdir, rm, symlink } = require('node:fs/promises') const { moveFile } = require('@npmcli/fs') const { subset, intersects } = require('semver') const { walkUp } = require('walk-up-path') @@ -123,6 +123,7 @@ module.exports = cls => class Reifier extends cls { await this[_diffTrees]() await this.#reifyPackages() if (linked) { + await this.#cleanOrphanedStoreEntries() // swap back in the idealTree // so that the lockfile is preserved this.idealTree = oldTree @@ -1356,6 +1357,44 @@ module.exports = cls => class Reifier extends cls { timeEnd() } + // 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. + async #cleanOrphanedStoreEntries () { + const storeDir = resolve(this.path, 'node_modules', '.store') + let entries + try { + entries = await readdir(storeDir) + } catch { + return + } + + // Collect valid store keys from the isolated ideal tree. + // Store entries have location: node_modules/.store/{key}/node_modules/{pkg} + const validKeys = new Set() + for (const child of this.idealTree.children.values()) { + if (child.isInStore) { + const key = child.location.split(sep)[2] + validKeys.add(key) + } + } + + const orphaned = entries.filter(e => !validKeys.has(e)) + if (!orphaned.length) { + return + } + + log.silly('reify', 'cleaning orphaned store entries', orphaned) + await promiseAllRejectLate( + orphaned.map(e => + rm(resolve(storeDir, e), { recursive: true, force: true }) + .catch(/* istanbul ignore next -- rm with force rarely fails */ + er => log.warn('cleanup', `Failed to remove orphaned store entry ${e}`, er)) + ) + ) + } + // last but not least, we save the ideal tree metadata to the package-lock // or shrinkwrap file, and any additions or removals to package.json async [_saveIdealTree] (options) { diff --git a/workspaces/arborist/test/isolated-mode.js b/workspaces/arborist/test/isolated-mode.js index d272e44ab0e0f..492d467a9dd17 100644 --- a/workspaces/arborist/test/isolated-mode.js +++ b/workspaces/arborist/test/isolated-mode.js @@ -1853,6 +1853,85 @@ tap.test('workspace links are not affected by store resolved fix', async t => { t.ok(arb2.diff.unchanged.length > 0, 'second install should have unchanged nodes') }) +tap.test('orphaned store entries are cleaned up on dependency update', async t => { + const graph = { + registry: [ + { name: 'which', version: '1.0.0', dependencies: { isexe: '^1.0.0' } }, + { name: 'which', version: '2.0.0', dependencies: { isexe: '^1.0.0' } }, + { name: 'isexe', version: '1.0.0' }, + ], + root: { + name: 'myproject', + version: '1.0.0', + dependencies: { which: '1.0.0' }, + }, + } + const { dir, registry } = await getRepo(graph) + const cache = fs.mkdtempSync(`${getTempDir()}/test-`) + const storeDir = path.join(dir, 'node_modules', '.store') + + // First install — which@1.0.0 + const arb1 = new Arborist({ path: dir, registry, packumentCache: new Map(), cache }) + await arb1.reify({ installStrategy: 'linked' }) + + const entriesAfterV1 = fs.readdirSync(storeDir) + t.ok(entriesAfterV1.some(e => e.startsWith('which@1.0.0-')), + 'store has which@1.0.0 entry after first install') + + // Update package.json to depend on which@2.0.0 + const pkgPath = path.join(dir, 'package.json') + const pkg = JSON.parse(fs.readFileSync(pkgPath, 'utf8')) + pkg.dependencies.which = '2.0.0' + fs.writeFileSync(pkgPath, JSON.stringify(pkg)) + + // Second install — which@2.0.0 + const arb2 = new Arborist({ path: dir, registry, packumentCache: new Map(), cache }) + await arb2.reify({ installStrategy: 'linked' }) + + const entriesAfterV2 = fs.readdirSync(storeDir) + t.ok(entriesAfterV2.some(e => e.startsWith('which@2.0.0-')), + 'store has which@2.0.0 entry after update') + t.notOk(entriesAfterV2.some(e => e.startsWith('which@1.0.0-')), + 'old which@1.0.0 store entry is removed after update') +}) + +tap.test('orphaned store entries are cleaned up on dependency removal', async t => { + const graph = { + registry: [ + { name: 'which', version: '1.0.0', dependencies: { isexe: '^1.0.0' } }, + { name: 'isexe', version: '1.0.0' }, + ], + root: { + name: 'myproject', + version: '1.0.0', + dependencies: { which: '1.0.0' }, + }, + } + const { dir, registry } = await getRepo(graph) + const cache = fs.mkdtempSync(`${getTempDir()}/test-`) + const storeDir = path.join(dir, 'node_modules', '.store') + + // First install + const arb1 = new Arborist({ path: dir, registry, packumentCache: new Map(), cache }) + await arb1.reify({ installStrategy: 'linked' }) + + t.ok(fs.readdirSync(storeDir).length > 0, 'store has entries after install') + + // Remove the dependency + const pkgPath = path.join(dir, 'package.json') + const pkg = JSON.parse(fs.readFileSync(pkgPath, 'utf8')) + delete pkg.dependencies + fs.writeFileSync(pkgPath, JSON.stringify(pkg)) + + // Reinstall + const arb2 = new Arborist({ path: dir, registry, packumentCache: new Map(), cache }) + await arb2.reify({ installStrategy: 'linked' }) + + const entriesAfterRemoval = fs.readdirSync(storeDir) + t.equal(entriesAfterRemoval.length, 0, + 'all store entries are removed when dependencies are removed') +}) + function setupRequire (cwd) { return function requireChain (...chain) { return chain.reduce((path, name) => { From 768daae5e2a3284afde0b7af9f000fbfebd4fae1 Mon Sep 17 00:00:00 2001 From: Manzoor Wani Date: Wed, 4 Mar 2026 23:26:45 +0530 Subject: [PATCH 3/4] Unify comment lines into one --- workspaces/arborist/lib/arborist/reify.js | 28 ++++++++--------------- 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index defa0335c30f4..ddc676e5b1d79 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -431,9 +431,8 @@ module.exports = cls => class Reifier extends cls { if (ideal) { filterNodes.push(ideal) } - // Skip actual-side filterNodes when using the linked diff wrapper — - // those nodes have root===actualTree, not root===linkedActualForDiff, - // and Diff.calculate requires filterNode.root to match actual. + // Skip actual-side filterNodes when using the linked diff wrapper. + // Those nodes have root===actualTree, not root===linkedActualForDiff, and Diff.calculate requires filterNode.root to match actual. // The ideal filterNode alone is sufficient to scope the workspace diff. if (!this.#linkedActualForDiff) { const actual = this.actualTree.children.get(ws) @@ -801,11 +800,9 @@ module.exports = cls => class Reifier extends cls { return join(filePath) } - // Build a flat actual tree wrapper for linked installs so the diff can - // correctly match store entries that already exist on disk. The proxy - // tree from _createIsolatedTree() is flat (all children on root), but - // loadActual() produces a nested tree where store entries are deep link - // targets. This wrapper surfaces them at the root level for comparison. + // Build a flat actual tree wrapper for linked installs so the diff can correctly match store entries that already exist on disk. + // The proxy tree from createIsolatedTree() is flat (all children on root), but loadActual() produces a nested tree where store entries are deep link targets. + // This wrapper surfaces them at the root level for comparison. #buildLinkedActualForDiff (idealTree, actualTree) { // Combined Map keyed by path (how allChildren() in diff.js keys) const combined = new Map() @@ -816,10 +813,8 @@ module.exports = cls => class Reifier extends cls { } // Add synthetic entries for store nodes and store links that exist on disk. - // The proxy tree is flat — all store entries (isInStore) and store links - // (isStoreLink) are direct children of root. The actual tree only has - // top-level links as root children, so store entries and store links - // need synthetic actual entries for the diff to match them. + // The proxy tree is flat: all store entries (isInStore) and store links (isStoreLink) are direct children of root. + // The actual tree only has top-level links as root children, so store entries need synthetic actual entries for the diff to match them. for (const child of idealTree.children.values()) { if (!combined.has(child.path) && (child.isInStore || child.isStoreLink) && existsSync(child.path)) { @@ -1357,10 +1352,8 @@ module.exports = cls => class Reifier extends cls { timeEnd() } - // 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. + // 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. async #cleanOrphanedStoreEntries () { const storeDir = resolve(this.path, 'node_modules', '.store') let entries @@ -1370,8 +1363,7 @@ module.exports = cls => class Reifier extends cls { return } - // Collect valid store keys from the isolated ideal tree. - // Store entries have location: node_modules/.store/{key}/node_modules/{pkg} + // Collect valid store keys from the isolated ideal tree (location: node_modules/.store/{key}/node_modules/{pkg}) const validKeys = new Set() for (const child of this.idealTree.children.values()) { if (child.isInStore) { From e0f909f81761d061d411a2daea7a8400bf3a0c09 Mon Sep 17 00:00:00 2001 From: Manzoor Wani Date: Wed, 4 Mar 2026 23:36:46 +0530 Subject: [PATCH 4/4] Update comment for istanbul ignore for made --- workspaces/arborist/lib/arborist/reify.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index ddc676e5b1d79..9b380b38faa08 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -583,7 +583,7 @@ module.exports = cls => class Reifier extends cls { // if the directory already exists, made will be undefined. if that's the case // we don't want to remove it because we aren't the ones who created it so we // omit it from the #sparseTreeRoots - /* istanbul ignore next -- mkdir returns path only when dir is new */ + /* istanbul ignore next -- pre-existing: mkdir returns undefined when dir exists, covered in reify tests but lost in aggregate coverage merge */ if (made) { this.#sparseTreeRoots.add(made) }