From 4ddd13269abc193b42468eb02e725cf496d6ae65 Mon Sep 17 00:00:00 2001 From: Manzoor Wani Date: Wed, 24 Jun 2026 23:19:17 +0530 Subject: [PATCH] fix(arborist): record the linked .store layout in the hidden lockfile (#9630) 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. Under `install-strategy=linked`, the hidden lockfile `node_modules/.package-lock.json` recorded the hoisted logical layout (`node_modules/`) instead of the actual on-disk `.store`/symlink layout. The hidden lockfile is meant to cache what `loadActual()` finds on disk so the actual tree can be validated cheaply, but because it recorded the wrong layout it was rejected on every reload, so it never served as a cache and misrepresented the installed layout. A linked reify swaps `idealTree` for the isolated tree, materializes the `.store`/symlink layout, then swaps the logical tree back before saving. The hidden lockfile was serialized from that logical tree, so it listed packages at their hoisted paths. On the next load, `assertNoNewer()` walked the real `node_modules` (the root symlink plus `.store/`) and could not reconcile it with the hoisted entries, throwing `missing from lockfile`, so `loadActual()` always fell back to a full filesystem scan. `reify.js` serializes the hidden lockfile from the isolated tree, which mirrors the on-disk layout, while `package-lock.json` still comes from the logical tree. It records every store package directory and symlink, adds an entry for each `.store/` container directory (these are the fsParents `loadVirtual()` needs so a store package can resolve its sibling deps), includes the workspace directories, and skips tree-only undeclared-workspace self-links that are never materialized on disk. `assertNoNewer()` additionally validates the directories the plain `node_modules` walk cannot reach under the linked strategy: a store package's deps live as symlinked siblings under `.store//node_modules` (and `.store` is skipped as a dot-dir), and an undeclared workspace is not symlinked into the root `node_modules` at all. These directories are derived from the lockfile entries. A workspace directory is only walked when it is not the target of a link entry, so the hoisted strategy keeps its existing, stricter validation unchanged — a stale workspace symlink that points at the wrong target still surfaces as a missing entry and rejects the cache. Fixes #9612 Part of #9608 --- workspaces/arborist/lib/arborist/reify.js | 66 ++++++++++-- workspaces/arborist/lib/shrinkwrap.js | 29 ++++++ workspaces/arborist/test/arborist/reify.js | 113 +++++++++++++++++++++ 3 files changed, 201 insertions(+), 7 deletions(-) diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index 960510a86531e..4b10e78ff1c3f 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -26,7 +26,8 @@ const optionalSet = require('../optional-set.js') const relpath = require('../relpath.js') const retirePath = require('../retire-path.js') const treeCheck = require('../tree-check.js') -const { defaultLockfileVersion } = require('../shrinkwrap.js') +const Shrinkwrap = require('../shrinkwrap.js') +const { defaultLockfileVersion } = Shrinkwrap const { saveTypeMap, hasSubKey } = require('../add-rm-pkg-deps.js') const { IsolatedNode, IsolatedLink } = require('../isolated-classes.js') @@ -115,12 +116,15 @@ module.exports = cls => class Reifier extends cls { await this[_loadTrees](options) const oldTree = this.idealTree + // Kept to serialize the hidden lockfile from the on-disk .store/symlink layout. + let isolatedTree = null if (linked) { // swap out the tree with the isolated tree // this is currently technical debt which will be resolved in a refactor // of Node/Link trees log.warn('reify', 'The "linked" install strategy is EXPERIMENTAL and may contain bugs.') this.idealTree = await this.createIsolatedTree() + isolatedTree = this.idealTree if (this.actualTree) { this.#linkedActualForDiff = this.#buildLinkedActualForDiff( this.idealTree, this.actualTree @@ -241,17 +245,24 @@ module.exports = cls => class Reifier extends cls { calcDepFlags(this.idealTree) } - // save the ideal's meta as a hidden lockfile after we actualize it - this.idealTree.meta.filename = - this.idealTree.realpath + '/node_modules/.package-lock.json' - this.idealTree.meta.hiddenLockfile = true - this.idealTree.meta.lockfileVersion = defaultLockfileVersion + // save the ideal's meta as a hidden lockfile after we actualize it. + // Under linked the logical tree is the hoisted layout, so the hidden lockfile is serialized from the isolated tree instead. + if (!linked) { + this.idealTree.meta.filename = + this.idealTree.realpath + '/node_modules/.package-lock.json' + this.idealTree.meta.hiddenLockfile = true + this.idealTree.meta.lockfileVersion = defaultLockfileVersion + } this.actualTree = this.idealTree this.idealTree = null if (!this.options.global && !this.options.dryRun) { - await this.actualTree.meta.save() + if (linked) { + await this.#saveLinkedHiddenLockfile(isolatedTree) + } else { + await this.actualTree.meta.save() + } const ignoreScripts = !!this.options.ignoreScripts // if we aren't doing a dry run or ignoring scripts and we actually made changes to the dep // tree, then run the dependencies scripts @@ -834,6 +845,47 @@ module.exports = cls => class Reifier extends cls { return join(filePath) } + // Serialize the hidden lockfile from the isolated tree, which mirrors the on-disk .store/symlink layout. + // Its children are every materialized node_modules entry: store package dirs and all symlinks. + async #saveLinkedHiddenLockfile (isolatedTree) { + const path = isolatedTree.realpath + const meta = new Shrinkwrap({ + path, + hiddenLockfile: true, + lockfileVersion: defaultLockfileVersion, + resolveOptions: this.options, + }) + meta.reset() + meta.filename = resolve(path, 'node_modules/.package-lock.json') + const storeRe = /^(.*\/\.store\/.+?)\/node_modules\// + const containers = new Set() + const nodes = new Set() + for (const node of isolatedTree.children.values()) { + // Tree-only undeclared workspace self-links aren't on disk. + if (node.isUndeclaredWorkspaceLink) { + continue + } + nodes.add(node) + // Record the enclosing .store/ dir so loadVirtual can resolve a store package's sibling deps. + // node.location uses the platform separator; lockfile keys are posix. + const m = node.location.replace(/\\/g, '/').match(storeRe) + if (m) { + containers.add(m[1]) + } + } + // Workspace dirs hold their own dep symlinks; record them so the cache can validate those subtrees. + for (const ws of isolatedTree.fsChildren) { + nodes.add(ws) + } + for (const node of nodes) { + meta.add(node) + } + for (const loc of containers) { + meta.data.packages[loc] = {} + } + await meta.save() + } + // 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. diff --git a/workspaces/arborist/lib/shrinkwrap.js b/workspaces/arborist/lib/shrinkwrap.js index cf27bcf4ac726..03cc86edbd4e9 100644 --- a/workspaces/arborist/lib/shrinkwrap.js +++ b/workspaces/arborist/lib/shrinkwrap.js @@ -172,6 +172,35 @@ const assertNoNewer = async (path, data, lockTime, dir, seen) => { return } + // The walk above can't reach two linked-strategy layouts: a store package's sibling deps under .store//node_modules (.store is a skipped dot-dir), and an undeclared workspace not symlinked into root node_modules. Walk those dirs, derived from the lockfile entries. + // A dir reachable through a link entry is skipped: it was already reached (or, if the symlink is stale, correctly stays unseen so the lockfile is rejected). This keeps the hoisted strategy's stale-symlink detection intact, since there every workspace is a link target. + const linkTargets = new Set() + for (const loc in data.packages) { + const { link, resolved } = data.packages[loc] + if (link && resolved) { + linkTargets.add(resolved.replace(/\\/g, '/')) + } + } + const extraDirs = new Set() + for (const loc in data.packages) { + const store = loc.match(/^(.*\/\.store\/.+?)\/node_modules\//) + if (store) { + // .store/ is never walked but has no entry, so mark it seen. + seen.add(store[1]) + extraDirs.add(`${store[1]}/node_modules`) + continue + } + // A workspace/fsChild dir outside node_modules, e.g. packages/a. + const i = loc.indexOf('/node_modules/') + const root = i === -1 ? loc : loc.slice(0, i) + if (root && !/(^|\/)node_modules(\/|$)/.test(root) && !linkTargets.has(root)) { + extraDirs.add(root) + } + } + for (const rel of extraDirs) { + await assertNoNewer(path, data, lockTime, resolve(path, rel), seen) + } + // assert that all the entries in the lockfile were seen for (const loc in data.packages) { if (!seen.has(loc)) { diff --git a/workspaces/arborist/test/arborist/reify.js b/workspaces/arborist/test/arborist/reify.js index becde0b6fa4c2..e16563a7722bf 100644 --- a/workspaces/arborist/test/arborist/reify.js +++ b/workspaces/arborist/test/arborist/reify.js @@ -4333,6 +4333,119 @@ t.test('install strategy linked', async (t) => { t.ok(abbrev.isSymbolicLink(), 'abbrev got installed') }) + t.test('hidden lockfile records the linked .store layout and round-trips', async t => { + // Regression for #9612: the hidden lockfile must record the on-disk .store/symlink layout so it round-trips as a valid cache. + const Shrinkwrap = require('../../lib/shrinkwrap.js') + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'root', + version: '1.0.0', + // once depends on wrappy, so the store has a transitive symlink to validate + dependencies: { once: '1.4.0' }, + }), + }) + + createRegistry(t, true) + await reify(path, { installStrategy: 'linked' }) + + const hidden = require(resolve(path, 'node_modules/.package-lock.json')) + const locs = Object.keys(hidden.packages) + // the layout is recorded at .store paths, not hoisted node_modules/ + t.ok(locs.some(l => /^node_modules\/\.store\/once@/.test(l)), + 'once is recorded under .store') + t.ok(locs.some(l => /^node_modules\/\.store\/.*\/node_modules\/wrappy$/.test(l)), + 'the transitive wrappy symlink is recorded inside the store') + t.notOk(locs.includes('node_modules/wrappy'), + 'wrappy is not recorded at a hoisted path') + + // the cache is accepted on reload: assertNoNewer matches it against the real disk layout + const meta = await Shrinkwrap.load({ path, hiddenLockfile: true }) + t.equal(meta.loadedFromDisk, true, 'hidden lockfile is a valid cache of the disk layout') + + // loadActual must reconstruct the tree from the cache with once->wrappy resolved through the store. + const actual = await newArb({ path, installStrategy: 'linked' }).loadActual() + const onceNode = [...actual.inventory.values()].find(n => n.name === 'once' && !n.isLink) + t.ok(onceNode, 'once is in the cached actual tree') + const wrappyEdge = onceNode.edgesOut.get('wrappy') + t.ok(wrappyEdge && !wrappyEdge.missing, 'once resolves its wrappy dep through the cached store layout') + }) + + t.test('hidden lockfile round-trips with an undeclared workspace', async t => { + // Regression for #9612: an undeclared workspace materializes deps in its own node_modules but isn't linked into root, and the cache must still validate that subtree. + const Shrinkwrap = require('../../lib/shrinkwrap.js') + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'host', + version: '1.0.0', + workspaces: ['packages/a'], + // root does not depend on the workspace, so it stays undeclared + }), + packages: { + a: { + 'package.json': JSON.stringify({ + name: 'a', + version: '1.0.0', + dependencies: { once: '1.4.0' }, + }), + }, + }, + }) + + createRegistry(t, true) + await reify(path, { installStrategy: 'linked' }) + + // the workspace's dep is materialized under its own node_modules, not the root's + t.ok(fs.lstatSync(resolve(path, 'packages/a/node_modules/once')).isSymbolicLink(), + 'once is symlinked into the workspace node_modules') + t.notOk(fs.existsSync(resolve(path, 'node_modules/a')), + 'the undeclared workspace is not symlinked into the root node_modules') + + const meta = await Shrinkwrap.load({ path, hiddenLockfile: true }) + t.equal(meta.loadedFromDisk, true, 'hidden lockfile validates the undeclared workspace subtree') + }) + + t.test('hidden lockfile round-trips with an undeclared workspace and no store entries', async t => { + // Regression for #9612: with only local deps there is no .store, so the cache must still walk the undeclared workspace subtree to validate it. + const Shrinkwrap = require('../../lib/shrinkwrap.js') + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'host', + version: '1.0.0', + workspaces: ['packages/w', 'packages/a', 'packages/b'], + // only w is declared; a and b stay undeclared, and a depends on b locally + dependencies: { w: '1.0.0' }, + }), + packages: { + w: { 'package.json': JSON.stringify({ name: 'w', version: '1.0.0' }) }, + a: { + 'package.json': JSON.stringify({ + name: 'a', + version: '1.0.0', + dependencies: { b: '1.0.0' }, + }), + }, + b: { 'package.json': JSON.stringify({ name: 'b', version: '1.0.0' }) }, + }, + }) + + createRegistry(t, false) + await reify(path, { installStrategy: 'linked' }) + + t.notOk(fs.existsSync(resolve(path, 'node_modules/.store')), + 'no store is created for an all-local graph') + t.ok(fs.lstatSync(resolve(path, 'packages/a/node_modules/b')).isSymbolicLink(), + 'the undeclared workspace links its local dep') + + const meta = await Shrinkwrap.load({ path, hiddenLockfile: true }) + t.equal(meta.loadedFromDisk, true, 'hidden lockfile validates the subtree without any store entry') + + // loadActual must reconstruct the undeclared workspace from the cache with its local dep resolved. + const actual = await newArb({ path, installStrategy: 'linked' }).loadActual() + const aNode = [...actual.inventory.values()].find(n => n.name === 'a' && !n.isLink) + const bEdge = aNode && aNode.edgesOut.get('b') + t.ok(bEdge && !bEdge.missing, 'the undeclared workspace resolves its local dep through the cache') + }) + t.test('does not re-create a workspace dir removed from manifest', async t => { // Regression test for https://github.com/npm/cli/issues/9331 const path = t.testdir({