fix: prevent a pinned locked peer provider from leaking to sibling nodes by davidbarratt · Pull Request #12320 · pnpm/pnpm

davidbarratt · 2026-06-10T22:38:35Z

Problem

pnpm dedupe --check fails intermittently in CI on a large workspace, while running pnpm dedupe locally on the same commit reports no changes. The diff that dedupe prints when it fails shows a dependency instance flipping an optional peer in its suffix between runs, for example next@16.2.6(@opentelemetry/api@1.9.1)... versus next@16.2.6(@babel/core@7.29.0)(@opentelemetry/api@1.9.1).... Both variants legitimately exist in the lockfile (the flipping package itself never appears in the added/removed package list, only its dependents do); what changes between runs is which variant a dependent gets linked against.

The failures correlate with cache state: runs that download even a few packages (cold CI cache) can fail, runs that reuse everything (warm local cache) pass. That pointed at resolution order, since the dependency tree's child-map insertion order follows async resolution completion order.

Root cause

The locked-peer-context pinning introduced in #12083 (1c73e8303c) mutates parentPkgs when it reuses a provider recorded in the lockfile:

resolvePeers.ts#L514: parentPkgs[peerName] = lockedPeer

That write is intended to be scoped to the node being resolved. It isn't, because a node with no child dependencies never copies parentPkgs — it aliases its parent's object:

resolvePeers.ts#L451-L456

Childless nodes are common in exactly the pinning scenario: peer dependencies are stripped from a snapshot's dependencies when building the tree, so any package whose dependencies are all peers has zero children. When such a node pins a provider, the provider lands in the parent's parentPkgs and becomes visible to every sibling resolved after it. A sibling with an optional peer of that name then resolves the peer it was never supposed to see, and gets a different dep path.

Whether a given sibling runs before or after the leaking node depends on the iteration order of the children map, which is insertion order, which is network completion order. Hence: deterministic-looking locally, flaky in CI.

Reproduction

The added unit test (installing/deps-resolver/test/resolvePeers.ts, "a provider pinned for a childless consumer does not leak to the consumer's siblings") resolves the same tree twice, with the childless pinning consumer and a sibling (victim, which has an optional peer on the pinned provider) in both insertion orders. Before the fix, the consumer-first order produces victim/1.0.0(peer/2.0.0) while the victim-first order produces victim/1.0.0 — the output depends on sibling order. After the fix both orders produce victim/1.0.0.

Fix

Copy parentPkgs before the pinning loop when it aliases the parent's object, so the pin stays scoped to the node and its own subtree.

Verification

New test fails before the fix and passes after; full @pnpm/installing.deps-resolver suite passes (109 tests).
The guards from fix(deps-resolver): prefer locked peer contexts during resolution by default #12083 still hold: both "wins over a locked context" tests in installing/deps-installer/test/install/peerDependencies.ts pass, and pnpm/test/install/preferLockedPeerContexts.ts passes against a freshly rebuilt bundle.
@pnpm/installing.dedupe.check tests pass.

No pacquet change is needed: its lockfile-reuse path rebuilds children from the manifest and lets the peer pass derive each instance's context (see the pacquet section of 1c73e8303c), so it has no pinning write to leak. This fix brings the TypeScript side back in line with what pacquet already produces.

Written by an agent (Claude Code, claude-fable-5).

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Fixed intermittent CI failures of pnpm dedupe --check caused by nondeterministic lockfile generation.
- Improved peer dependency resolution isolation so pinned peer providers no longer leak into sibling package resolutions.
Tests
- Added a regression test ensuring pinned peer provider instances remain scoped and do not appear in sibling dependency-tree variants.
Chores
- Updated patch versions for the involved packages.

qodo-free-for-open-source-projects · 2026-06-10T22:38:42Z

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0)

1. Unnecessary parentPkgs cloning 🐞 Bug ➹ Performance

Description

In resolvePeersOfNode, parentPkgs is cloned for childless nodes whenever lockedPeerContext is
present, even if no pinned provider is actually applied because the loop later hits only
early-continue guards. This can add avoidable allocation/property-copy overhead in the
resolvedPeerProviderPaths (second-pass) resolution hot path on large graphs.

Code

installing/deps-resolver/src/resolvePeers.ts[R489-491]
+    if (parentPkgs === parentParentPkgs) {
+      parentPkgs = { ...parentParentPkgs }
+    }

Evidence
Childless nodes intentionally alias parentParentPkgs, so pinning mutations would otherwise affect
siblings; the new code fixes that by cloning. But the clone currently happens before the loop, while
the loop has multiple early-continue guards that can result in zero pinning writes, making the clone
unnecessary in those cases.
installing/deps-resolver/src/resolvePeers.ts[451-456]
installing/deps-resolver/src/resolvePeers.ts[483-491]
installing/deps-resolver/src/resolvePeers.ts[492-510]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`resolvePeersOfNode()` now clones `parentPkgs` up-front for childless nodes when `lockedPeerContext` is present, but the subsequent pinning loop can skip all entries due to guard conditions. This means the clone can be wasted work.
### Issue Context
The clone is necessary when a pin actually occurs because childless nodes alias `parentParentPkgs`, and pinning writes into `parentPkgs`. However, if no pin occurs, the clone can be avoided.
### Fix Focus Areas
- installing/deps-resolver/src/resolvePeers.ts[483-523]
### Suggested approach
Move the `parentPkgs = { ...parentParentPkgs }` operation so it happens only immediately before the first actual write (e.g. right before `parentPkgs[peerName] = lockedPeer`), guarded by `if (parentPkgs === parentParentPkgs)` (or a `didCopy` boolean) to ensure at most one copy.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Previous review results

Review updated until commit 09051c4

Results up to commit facb729

🐞 Bugs (1) 📘 Rule violations (0)

1. Unnecessary parentPkgs cloning 🐞 Bug ➹ Performance

Description

In resolvePeersOfNode, parentPkgs is cloned for childless nodes whenever lockedPeerContext is
present, even if no pinned provider is actually applied because the loop later hits only
early-continue guards. This can add avoidable allocation/property-copy overhead in the
resolvedPeerProviderPaths (second-pass) resolution hot path on large graphs.

Code

installing/deps-resolver/src/resolvePeers.ts[R489-491]
+    if (parentPkgs === parentParentPkgs) {
+      parentPkgs = { ...parentParentPkgs }
+    }

Evidence
Childless nodes intentionally alias parentParentPkgs, so pinning mutations would otherwise affect
siblings; the new code fixes that by cloning. But the clone currently happens before the loop, while
the loop has multiple early-continue guards that can result in zero pinning writes, making the clone
unnecessary in those cases.
installing/deps-resolver/src/resolvePeers.ts[451-456]
installing/deps-resolver/src/resolvePeers.ts[483-491]
installing/deps-resolver/src/resolvePeers.ts[492-510]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`resolvePeersOfNode()` now clones `parentPkgs` up-front for childless nodes when `lockedPeerContext` is present, but the subsequent pinning loop can skip all entries due to guard conditions. This means the clone can be wasted work.

### Issue Context
The clone is necessary when a pin actually occurs because childless nodes alias `parentParentPkgs`, and pinning writes into `parentPkgs`. However, if no pin occurs, the clone can be avoided.

### Fix Focus Areas
- installing/deps-resolver/src/resolvePeers.ts[483-523]

### Suggested approach
Move the `parentPkgs = { ...parentParentPkgs }` operation so it happens only immediately before the first actual write (e.g. right before `parentPkgs[peerName] = lockedPeer`), guarded by `if (parentPkgs === parentParentPkgs)` (or a `didCopy` boolean) to ensure at most one copy.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Results up to commit 107e935

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Results up to commit e81ca6e

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

coderabbitai · 2026-06-10T22:38:50Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 6b99734d-98bd-4c2f-a60c-42f310a5b312

📥 Commits

Reviewing files that changed from the base of the PR and between 07290e3 and 09051c4.

📒 Files selected for processing (1)

installing/deps-resolver/src/resolvePeers.ts

📝 Walkthrough

Walkthrough

This PR fixes a nondeterministic lockfile issue in the peer dependency resolver by preventing locked peer provider pinning from leaking into sibling resolutions. The fix includes a defensive clone operation, a regression test covering two processing orders, and changeset metadata.

Changes

Peer Context Leak Prevention

Layer / File(s)	Summary
Locked peer context scoping fix `installing/deps-resolver/src/resolvePeers.ts`, `.changeset/locked-peer-pin-no-sibling-leak.md`	`resolvePeersOfNode` clones `parentPkgs` when it is the same object as `parentParentPkgs` to scope peer provider pinning from `lockedPeerContext` to the current node only. Changeset documents patch version bumps for `@pnpm/installing.deps-resolver` and `pnpm`.
Regression test for peer context leak prevention `installing/deps-resolver/test/resolvePeers.ts`	Test constructs two dependency-tree processing orders, pins a peer provider for one consumer via `lockedPeerContext`, then verifies the pinned instance is scoped to that consumer and does not leak to its sibling.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

pnpm/pnpm#12081 — Both PRs change installing/deps-resolver/src/resolvePeers.ts to prevent inherited peer-resolution context from propagating incorrectly to sibling or diamond dependency nodes.
pnpm/pnpm#12273 — Both PRs modify peer-resolution logic to fix parent-context propagation issues, preventing peer-provider "leakage" during peer walk with updated resolver tests.

Suggested labels

area: lockfile

Suggested reviewers

zkochan

Poem

🐰 A peer pinned down, but oh what strife!
It leaked to siblings, caused lockfile strife.
A clone, a scope, a test so keen—
Now all resolutions stay pristine! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: preventing a locked peer provider from leaking to sibling nodes, which is the core fix in this PR.
Linked Issues check	✅ Passed	The PR fixes nondeterministic resolution of optional peer dependencies by preventing pinned peers from affecting sibling resolutions, directly supporting the objective to handle optional dependencies correctly.
Out of Scope Changes check	✅ Passed	All changes (changeset, source fix, and regression test) are directly related to fixing the locked peer provider leak issue described in the PR objectives.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

qodo-free-for-open-source-projects · 2026-06-10T22:39:29Z

PR Summary by Qodo

Fix locked peer provider pinning leaking into sibling resolution
🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Walkthroughs

Description

• Prevent locked-peer provider pinning from mutating shared parent scope for childless nodes.
• Eliminate nondeterministic peer suffix resolution that flaked pnpm dedupe --check in CI.
• Add regression test covering both sibling insertion orders to ensure deterministic output.

Diagram

graph TD
  A["Dependencies tree"] --> B["resolvePeersOfNode()"] --> C{"lockedPeerContext present?"}
  C -- "yes" --> D["Copy parentPkgs if aliased"] --> E["Pin locked peer provider"] --> F["Resolve siblings' peers"] --> G["Deterministic dep paths"]
  C -- "no" --> F --> G

High-Level Assessment

The following are alternative approaches to this PR:

1. Always clone parentPkgs for every node

➕ Eliminates all aliasing risks without special-casing childless nodes
➕ Simplifies reasoning about mutation scope
➖ Potential performance/memory regression on large trees due to frequent copies
➖ Unnecessary work for nodes that never pin or mutate parentPkgs

2. Make parentPkgs immutable (persistent map / overlay chain)

➕ Prevents mutation leaks by construction
➕ Makes scoping explicit (each node gets an overlay)
➖ Larger refactor with more risk and review surface area
➖ May introduce runtime overhead and wider API changes

3. Record pinned providers in a separate per-node structure

➕ Avoids mutating shared parentPkgs altogether
➕ Keeps pinning semantics explicit and auditable
➖ Requires changes to downstream resolution logic to consult two sources
➖ More complex data-flow than the minimal fix

Recommendation: The PR’s approach (copy-on-alias only when entering the pinning path) is the best tradeoff: it fixes the leak with minimal behavior change and minimal overhead. Broader immutability or always-clone strategies could further harden the logic but would significantly increase runtime cost or refactor scope.

File Changes

Bug fix (1)

resolvePeers.ts Prevent locked-peer pinning from mutating aliased parentPkgs +8/-0
Prevent locked-peer pinning from mutating aliased parentPkgs
• Adds a guard to copy 'parentPkgs' when it aliases 'parentParentPkgs' (childless node case) before applying locked-peer-context pinning. This keeps pinned providers scoped to the current node/subtree and removes sibling-order-dependent peer resolution.
installing/deps-resolver/src/resolvePeers.ts

Tests (1)

resolvePeers.ts Regression test for sibling-order independence with childless pinned consumer +72/-0
Regression test for sibling-order independence with childless pinned consumer
• Introduces a unit test that resolves the same wrapper subtree twice with opposite sibling insertion orders. Verifies that a consumer with 'lockedPeerContext' does not cause a sibling with an optional peer to pick up the pinned provider.
installing/deps-resolver/test/resolvePeers.ts

Documentation (1)

locked-peer-pin-no-sibling-leak.md Add changeset for nondeterministic dedupe/locked-peer pin leak fix +6/-0
Add changeset for nondeterministic dedupe/locked-peer pin leak fix
• Adds a patch changeset for '@pnpm/installing.deps-resolver' and 'pnpm'. Documents the CI flake caused by a pinned locked peer provider leaking into a shared parent scope for childless nodes.
.changeset/locked-peer-pin-no-sibling-leak.md

qodo-free-for-open-source-projects · 2026-06-11T12:03:15Z

Code review by qodo was updated up to the latest commit 107e935

qodo-free-for-open-source-projects · 2026-06-14T16:50:14Z

Code review by qodo was updated up to the latest commit facb729

When the locked-peer-context pinning introduced in #12083 runs for a node that has no child dependencies, parentPkgs aliases the parent's object, so writing the pinned provider into it exposed the provider to every sibling resolved afterwards. Sibling order follows resolution completion order, so optional peers of siblings resolved nondeterministically and "pnpm dedupe --check" failed intermittently in CI. Copy parentPkgs before pinning so the pin stays scoped to the node and its own subtree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

qodo-free-for-open-source-projects · 2026-06-14T21:40:32Z

Code Review by Qodo

New Review Started

This review has been superseded by a new analysis

qodo-free-for-open-source-projects · 2026-06-14T21:43:03Z

Code review by qodo was updated up to the latest commit 09051c4

davidbarratt requested a review from zkochan as a code owner June 10, 2026 22:38

coderabbitai Bot approved these changes Jun 10, 2026

View reviewed changes

zkochan mentioned this pull request Jun 12, 2026

Resolution: a shared package's children context is decided by an async race (nondeterministic lockfiles) #12358

Closed

zkochan force-pushed the fix/locked-peer-pin-sibling-leak branch from facb729 to 07290e3 Compare June 14, 2026 21:40

perf: copy parentPkgs only before the first pin write

09051c4

zkochan approved these changes Jun 14, 2026

View reviewed changes

zkochan merged commit 3a27141 into pnpm:main Jun 14, 2026
9 of 10 checks passed

Uh oh!

Uh oh!

Conversation

davidbarratt commented Jun 10, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Root cause

Reproduction

Fix

Verification

Summary by CodeRabbit

Uh oh!

qodo-free-for-open-source-projects Bot commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Previous review results

Great, no issues found!

Great, no issues found!

Uh oh!

coderabbitai Bot commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

qodo-free-for-open-source-projects Bot commented Jun 10, 2026

PR Summary by Qodo

Walkthroughs

File Changes

Uh oh!

qodo-free-for-open-source-projects Bot commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qodo-free-for-open-source-projects Bot commented Jun 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qodo-free-for-open-source-projects Bot commented Jun 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

New Review Started

Uh oh!

qodo-free-for-open-source-projects Bot commented Jun 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

davidbarratt commented Jun 10, 2026 •

edited by coderabbitai Bot

Loading

qodo-free-for-open-source-projects Bot commented Jun 10, 2026 •

edited

Loading

coderabbitai Bot commented Jun 10, 2026 •

edited

Loading

qodo-free-for-open-source-projects Bot commented Jun 11, 2026 •

edited

Loading

qodo-free-for-open-source-projects Bot commented Jun 14, 2026 •

edited

Loading

qodo-free-for-open-source-projects Bot commented Jun 14, 2026 •

edited

Loading

qodo-free-for-open-source-projects Bot commented Jun 14, 2026 •

edited

Loading