From 7577d47ca7de866fa46ddeb048a8f059676a0ebc Mon Sep 17 00:00:00 2001 From: David Barratt Date: Thu, 21 May 2026 12:06:43 -0400 Subject: [PATCH] fix(installing.deps-resolver): deterministically order cyclic peer suffixes (#8155) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resolveDependencies` was pushing onto `pkgAddresses`, `postponedResolutionsQueue`, and `postponedPeersResolutionQueue` from inside `Promise.all`-spawned callbacks, so the order of items in those arrays reflected completion timing rather than the order of `extendedWantedDeps`. That ordering then flowed downstream into `resolvePeers` and the cyclic-peer suffix assignment, so two packages with transitive peer dependencies on each other (e.g. `@aws-sdk/client-sts` and `@aws-sdk/client-sso-oidc`) flipped between two equally-valid lockfile forms across consecutive installs. The fix awaits `Promise.all` to a temporary array and drains it with `for…of` so the per-edge results land in input order. This matches the existing pattern 200 lines earlier in `resolveDependenciesOfImporters`. End-to-end repro from the issue (`pnpm add @aws-sdk/client-s3@3.588.0` then loop `pnpm dedupe --check`): 33/50 failures without the fix → 0/100 with it. --- Written by an agent (Claude Code, claude-opus-4-7). Co-Authored-By: Claude Opus 4.7 (1M context) --- .changeset/fix-cyclic-peer-determinism.md | 6 + .../test/install/cyclicPeerDeterminism.ts | 119 ++++++++++++++++++ .../deps-resolver/src/resolveDependencies.ts | 44 +++---- 3 files changed, 147 insertions(+), 22 deletions(-) create mode 100644 .changeset/fix-cyclic-peer-determinism.md create mode 100644 installing/deps-installer/test/install/cyclicPeerDeterminism.ts diff --git a/.changeset/fix-cyclic-peer-determinism.md b/.changeset/fix-cyclic-peer-determinism.md new file mode 100644 index 0000000000..76639f7621 --- /dev/null +++ b/.changeset/fix-cyclic-peer-determinism.md @@ -0,0 +1,6 @@ +--- +"@pnpm/installing.deps-resolver": patch +pnpm: patch +--- + +Fixed non-determinism in `pnpm dedupe` and `pnpm install` when a dependency graph contains packages with transitive peer dependencies on each other (e.g. `@aws-sdk/client-sts` and `@aws-sdk/client-sso-oidc`) and `auto-install-peers` is enabled. The lockfile no longer flips between two equally-valid forms across consecutive runs. The root cause was that `resolveDependencies` pushed onto its `pkgAddresses` / `postponedResolutionsQueue` arrays from inside `Promise.all`-spawned callbacks, so completion-order timing leaked into the array order and downstream cyclic-peer suffix assignment. Fixes [#8155](https://github.com/pnpm/pnpm/issues/8155). diff --git a/installing/deps-installer/test/install/cyclicPeerDeterminism.ts b/installing/deps-installer/test/install/cyclicPeerDeterminism.ts new file mode 100644 index 0000000000..3f0292fdb4 --- /dev/null +++ b/installing/deps-installer/test/install/cyclicPeerDeterminism.ts @@ -0,0 +1,119 @@ +import { afterEach, expect, test } from '@jest/globals' +import { type MutatedProject, mutateModules, type MutateModulesOptions, type ProjectOptions } from '@pnpm/installing.deps-installer' +import { prepareEmpty } from '@pnpm/prepare' +import type { PackageMeta } from '@pnpm/resolving.registry.types' +import { getMockAgent, setupMockAgent, teardownMockAgent } from '@pnpm/testing.mock-agent' +import type { ProjectManifest, ProjectRootDir } from '@pnpm/types' + +import { testDefaults } from '../utils/index.js' + +afterEach(async () => { + await teardownMockAgent() +}) + +// Regression test for https://github.com/pnpm/pnpm/issues/8155. +// +// Reproduces the @aws-sdk/client-sts ↔ @aws-sdk/client-sso-oidc situation: a +// "wrapper" package pulls in two transitive deps that declare each other as +// peer dependencies, and `auto-install-peers` hoists those two peers up to the +// workspace root. Before the fix, `resolveDependencies` pushed onto its +// pkgAddresses / postponedResolutionsQueue arrays from inside +// `Promise.all`-spawned callbacks, so completion order leaked into the array +// order and the cyclic-peer suffix flipped between two equally valid forms +// across consecutive installs. +test('cyclic transitive peer dependencies resolve deterministically across installs', async () => { + const rootProject = prepareEmpty() + const lockfileDir = rootProject.dir() + + const wrapperName = '@pnpm.e2e/cyclic-wrapper' + const aName = '@pnpm.e2e/cyclic-a' + const bName = '@pnpm.e2e/cyclic-b' + + const manifest: ProjectManifest = { + name: 'root', + dependencies: { + [wrapperName]: '1.0.0', + }, + } + const allProjects: ProjectOptions[] = [{ + buildIndex: 0, + manifest, + rootDir: lockfileDir as ProjectRootDir, + }] + const options = { + ...testDefaults( + { allProjects, autoInstallPeers: true, forceFullResolution: true }, + { retry: { retries: 0 } } + ), + lockfileDir, + lockfileOnly: true, + } satisfies MutateModulesOptions + + const installProjects: MutatedProject[] = [{ + mutation: 'install', + rootDir: lockfileDir as ProjectRootDir, + manifest, + }] + + const registryUrl = options.registries.default.replace(/\/$/, '') + + function makeMeta (name: string, deps: Record, peerDeps: Record): PackageMeta { + return { + name, + versions: { + '1.0.0': { + name, + version: '1.0.0', + dependencies: deps, + peerDependencies: peerDeps, + dist: { + // Resolver only reads metadata when lockfileOnly is true, so the + // shasum value is never checked against a tarball. + shasum: '0000000000000000000000000000000000000000', + tarball: `${options.registries.default}/${encodeURIComponent(name)}-1.0.0.tgz`, + }, + }, + }, + 'dist-tags': { latest: '1.0.0' }, + } + } + + const metas = { + [wrapperName]: makeMeta(wrapperName, { [aName]: '1.0.0', [bName]: '1.0.0' }, {}), + [aName]: makeMeta(aName, {}, { [bName]: '1.0.0' }), + [bName]: makeMeta(bName, {}, { [aName]: '1.0.0' }), + } + + function metadataPath (name: string): string { + return `/${name.replace('/', '%2F')}` + } + + function arm (): void { + const agent = getMockAgent().get(registryUrl) + for (const [name, meta] of Object.entries(metas)) { + agent.intercept({ path: metadataPath(name), method: 'GET' }).reply(200, meta).persist() + } + } + + // ~30 iterations gives a ≥ (1 − 2⁻²⁹) chance of catching a 50/50 flip if + // the bug returns. The fix makes the walk order canonical so a single run + // would suffice, but iterating cheaply hedges against scheduling drift. + const iterations = 30 + + async function runOnce (): Promise { + await setupMockAgent() + arm() + options.storeController.clearResolutionCache() + await mutateModules(installProjects, options) + const lockfile = rootProject.readLockfile() + const snapshotKeys = Object.keys(lockfile.snapshots ?? {}).sort() + await teardownMockAgent() + return snapshotKeys + } + + const first = JSON.stringify(await runOnce()) + for (let i = 1; i < iterations; i++) { + const subsequent = JSON.stringify(await runOnce()) // eslint-disable-line no-await-in-loop + expect(subsequent).toEqual(first) + } +}) diff --git a/installing/deps-resolver/src/resolveDependencies.ts b/installing/deps-resolver/src/resolveDependencies.ts index 6cb91c7acb..d290affd17 100644 --- a/installing/deps-resolver/src/resolveDependencies.ts +++ b/installing/deps-resolver/src/resolveDependencies.ts @@ -683,29 +683,29 @@ export async function resolveDependencies ( const postponedResolutionsQueue: PostponedResolutionFunction[] = [] const postponedPeersResolutionQueue: PostponedPeersResolutionFunction[] = [] const pkgAddresses: PkgAddress[] = [] - await Promise.all( - extendedWantedDeps.map(async (extendedWantedDep) => { - const { - resolveDependencyResult, - postponedResolution, - postponedPeersResolution, - } = await resolveDependenciesOfDependency( - ctx, - preferredVersions, - options, - extendedWantedDep - ) - if (resolveDependencyResult) { - pkgAddresses.push(resolveDependencyResult as PkgAddress) - } - if (postponedResolution) { - postponedResolutionsQueue.push(postponedResolution) - } - if (postponedPeersResolution) { - postponedPeersResolutionQueue.push(postponedPeersResolution) - } - }) + // Resolve in parallel, then drain the results in input order. Pushing + // from inside the Promise.all callbacks would leak completion-order timing + // into pkgAddresses / postponedResolutionsQueue, which downstream + // determines how cyclic peer suffixes are assigned. See pnpm/pnpm#8155. + const resolvedDependencies = await Promise.all( + extendedWantedDeps.map((extendedWantedDep) => resolveDependenciesOfDependency( + ctx, + preferredVersions, + options, + extendedWantedDep + )) ) + for (const { resolveDependencyResult, postponedResolution, postponedPeersResolution } of resolvedDependencies) { + if (resolveDependencyResult) { + pkgAddresses.push(resolveDependencyResult as PkgAddress) + } + if (postponedResolution) { + postponedResolutionsQueue.push(postponedResolution) + } + if (postponedPeersResolution) { + postponedPeersResolutionQueue.push(postponedPeersResolution) + } + } const newPreferredVersions = Object.create(preferredVersions) as PreferredVersions const currentParentPkgAliases: Record = {} for (const pkgAddress of pkgAddresses) {