diff --git a/workspaces/arborist/lib/place-dep.js b/workspaces/arborist/lib/place-dep.js index 34a62144e9be4..f2710e0f4ce05 100644 --- a/workspaces/arborist/lib/place-dep.js +++ b/workspaces/arborist/lib/place-dep.js @@ -298,41 +298,49 @@ class PlaceDep { // because we're copying rather than moving them out of the virtual root, // otherwise they'd be gone and the peer set would change throughout // this loop. - for (const peerEdge of this.placed.edgesOut.values()) { - if (peerEdge.valid || !peerEdge.peer || peerEdge.peerConflicted) { - continue - } + // The dep we're placing may have been evicted from its virtual root while + // a later peer edge was being resolved (a recursive #loadPeerSet can + // detach the original node when a newer copy takes its place in the + // virtual root's inventory). In that case there is no sibling peer set + // left to place here; the placed node's unmet peers are re-resolved when + // it is processed from the deps queue. + if (virtualRoot) { + for (const peerEdge of this.placed.edgesOut.values()) { + if (peerEdge.valid || !peerEdge.peer || peerEdge.peerConflicted) { + continue + } - const peer = virtualRoot.children.get(peerEdge.name) + const peer = virtualRoot.children.get(peerEdge.name) - // Note: if the virtualRoot *doesn't* have the peer, then that means - // it's an optional peer dep. If it's not being properly met (ie, - // peerEdge.valid is false), then this is likely heading for an - // ERESOLVE error, unless it can walk further up the tree. - if (!peer) { - continue - } + // Note: if the virtualRoot *doesn't* have the peer, then that means + // it's an optional peer dep. If it's not being properly met (ie, + // peerEdge.valid is false), then this is likely heading for an + // ERESOLVE error, unless it can walk further up the tree. + if (!peer) { + continue + } - // peerConflicted peerEdge, just accept what's there already - if (!peer.satisfies(peerEdge)) { - continue - } + // peerConflicted peerEdge, just accept what's there already + if (!peer.satisfies(peerEdge)) { + continue + } - this.children.push(new PlaceDep({ - auditReport: this.auditReport, - explicitRequest: this.explicitRequest, - force: this.force, - installLinks: this.installLinks, - installStrategy: this.installStrategy, - legacyPeerDeps: this.legacyPeerDeps, - preferDedupe: this.preferDedupe, - strictPeerDeps: this.strictPeerDeps, - updateNames: this.updateName, - parent: this, - dep: peer, - node: this.placed, - edge: peerEdge, - })) + this.children.push(new PlaceDep({ + auditReport: this.auditReport, + explicitRequest: this.explicitRequest, + force: this.force, + installLinks: this.installLinks, + installStrategy: this.installStrategy, + legacyPeerDeps: this.legacyPeerDeps, + preferDedupe: this.preferDedupe, + strictPeerDeps: this.strictPeerDeps, + updateNames: this.updateName, + parent: this, + dep: peer, + node: this.placed, + edge: peerEdge, + })) + } } } diff --git a/workspaces/arborist/test/arborist/build-ideal-tree.js b/workspaces/arborist/test/arborist/build-ideal-tree.js index 400ff7b238a69..64482b0f9e04f 100644 --- a/workspaces/arborist/test/arborist/build-ideal-tree.js +++ b/workspaces/arborist/test/arborist/build-ideal-tree.js @@ -4959,6 +4959,62 @@ t.test('circular peer back-off does not crash when node is detached mid-resoluti 'backs off to plugin@1.0.0 to satisfy the optional peer instead of crashing') }) +t.test('peer task dep evicted from its virtual root by a later peer edge does not crash (#9911)', async t => { + // A global install queues tui's peer edges as problem edges resolved one at + // a time. Resolving the @t/agent edge places agent@2.0.0 into the virtual + // root, then resolving @t/y's peer set replaces it with agent@1.0.0 in the + // same virtual root (the canReplace path in #loadPeerSet), detaching the + // node the agent task still holds. Building that task's PlaceDep used to + // dereference the now-null virtual root while trying to place agent's + // invalid peer edge llm@^2.0.0, crashing with "Cannot read properties of + // null (reading 'children')". The unmet peers are instead re-resolved from + // the deps queue, so the tree backs off to the versions the peer set wants. + const registry = createRegistry(t, false) + + const tuiPack = registry.packument({ + name: '@t/tui', + version: '1.0.0', + peerDependencies: { '@t/agent': '*', '@t/y': '1.0.0' }, + }) + const tuiManifest = registry.manifest({ name: '@t/tui', packuments: [tuiPack] }) + await registry.package({ manifest: tuiManifest }) + + const agentPacks = [ + registry.packument({ name: '@t/agent', version: '1.0.0', peerDependencies: { '@t/llm': '^1.0.0' } }), + registry.packument({ name: '@t/agent', version: '2.0.0', peerDependencies: { '@t/llm': '^2.0.0' } }), + ] + const agentManifest = registry.manifest({ name: '@t/agent', packuments: agentPacks }) + await registry.package({ manifest: agentManifest, times: 2 }) + + const yPack = registry.packument({ + name: '@t/y', + version: '1.0.0', + peerDependencies: { '@t/agent': '^1.0.0', '@t/llm': '^1.0.0' }, + }) + const yManifest = registry.manifest({ name: '@t/y', packuments: [yPack] }) + await registry.package({ manifest: yManifest }) + + const llmPacks = [ + registry.packument({ name: '@t/llm', version: '1.0.0' }), + registry.packument({ name: '@t/llm', version: '2.0.0' }), + ] + const llmManifest = registry.manifest({ name: '@t/llm', packuments: llmPacks }) + await registry.package({ manifest: llmManifest, times: 2 }) + + const path = t.testdir({ + 'package.json': JSON.stringify({ name: 'test-9911' }), + }) + const arb = newArb(path, { global: true }) + const tree = await arb.buildIdealTree({ add: ['@t/tui@1.0.0'] }) + + const tui = tree.children.get('@t/tui') + t.ok(tui, 'tui is installed at top level') + t.equal(tui.children.get('@t/agent').version, '1.0.0', + 'backs off to agent@1.0.0 to satisfy the peer set instead of crashing') + t.equal(tui.children.get('@t/llm').version, '1.0.0', + 'llm backs off to the version the peer set can satisfy') +}) + t.test('does not fetch packuments for peerOptional deps that will not be installed', async t => { const registry = createRegistry(t, false)