From 3422cecfd3dbb0a7a3166a660d1761b02ffabea3 Mon Sep 17 00:00:00 2001 From: David Barratt Date: Thu, 21 May 2026 20:20:01 -0400 Subject: [PATCH] fix(installing.deps-resolver): deterministically order cyclic peer suffixes (#11826) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(installing.deps-resolver): deterministically order cyclic peer suffixes (#8155) `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) * test(installing.deps-installer): replace all slashes in mock metadata path Addresses CodeQL incomplete-string-escaping finding: `replace('/', '%2F')` only swaps the first occurrence. Scoped names in this test only have one slash so the behavior is unchanged, but switching to `replaceAll` clears the warning and is more defensible. Co-Authored-By: Claude Opus 4.7 (1M context) * test(installing.deps-installer): assert raw snapshot key order Removed the .sort() applied to the lockfile snapshot keys in the cyclic peer determinism test so the comparison reflects the actual order emitted by the lockfile writer. The deterministic ordering guaranteed by 7577d47 makes the sorted view and the raw view identical today; dropping the sort lets the test fail on any future regression that keeps the key set stable but shuffles the order. Co-Authored-By: Claude Opus 4.7 (1M context) * test(installing.deps-installer): drop stray manifest from MutatedProject literal MutatedProject does not carry a manifest field; it is conveyed via allProjects in MutateModulesOptions. Passing it inside the install project literal triggered TS2353 against the InstallDepsMutation shape. Co-Authored-By: Claude Opus 4.7 (1M context) * test(installing.deps-installer): rename metas to metaByName Clearer name for the map keyed by package name, and avoids tripping cspell on the abbreviation "metas". Co-Authored-By: Claude Opus 4.7 (1M context) * chore: retrigger CI Ubuntu Node.js 22.13.0 hit a transient 404 from Verdaccio's proxy-to-npm while resolving a transitive peer of @medusajs/medusa-js@6.1.7 in the pre-existing "install should not hang on circular peer dependencies" test (installing/deps-installer/test/install/misc.ts:1247). Ubuntu Node 24 and Node 26 ran the same code green. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- .changeset/fix-cyclic-peer-determinism.md | 6 + .../test/install/cyclicPeerDeterminism.ts | 118 ++++++++++++++++++ .../deps-resolver/src/resolveDependencies.ts | 44 +++---- 3 files changed, 146 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..100ae7cbc2 --- /dev/null +++ b/installing/deps-installer/test/install/cyclicPeerDeterminism.ts @@ -0,0 +1,118 @@ +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, + }] + + 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 metaByName = { + [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.replaceAll('/', '%2F')}` + } + + function arm (): void { + const agent = getMockAgent().get(registryUrl) + for (const [name, meta] of Object.entries(metaByName)) { + 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 ?? {}) + 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) {