mirror of
https://github.com/pnpm/pnpm.git
synced 2026-05-24 08:35:19 -04:00
fix(installing.deps-resolver): deterministically order cyclic peer suffixes (#11826)
* 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) <noreply@anthropic.com>
* 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) <noreply@anthropic.com>
* 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) <noreply@anthropic.com>
* 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) <noreply@anthropic.com>
* 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) <noreply@anthropic.com>
* 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) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
6
.changeset/fix-cyclic-peer-determinism.md
Normal file
6
.changeset/fix-cyclic-peer-determinism.md
Normal file
@@ -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).
|
||||
118
installing/deps-installer/test/install/cyclicPeerDeterminism.ts
Normal file
118
installing/deps-installer/test/install/cyclicPeerDeterminism.ts
Normal file
@@ -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<string, string>, peerDeps: Record<string, string>): 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<string[]> {
|
||||
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)
|
||||
}
|
||||
})
|
||||
@@ -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<string, PkgAddress | true> = {}
|
||||
for (const pkgAddress of pkgAddresses) {
|
||||
|
||||
Reference in New Issue
Block a user