Skip to content

fix: narrow warm install relinking#11169

Merged
zkochan merged 6 commits into
pnpm:mainfrom
vsumner:fix/narrow-warm-install-relinking
Jun 17, 2026
Merged

fix: narrow warm install relinking#11169
zkochan merged 6 commits into
pnpm:mainfrom
vsumner:fix/narrow-warm-install-relinking

Conversation

@vsumner

@vsumner vsumner commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

Problem

During warm installs, pnpm relinked existing packages more broadly than necessary when only some child dependencies changed.

In the narrowed relinking path, removed child aliases could also remain behind as stale links after dependency updates.

Solution

Only pass changed child edges through the relinking path for existing packages.

When a child alias is no longer present in the updated dependency set, remove the obsolete link before relinking. Added regression tests for both cases:

  • unchanged child dependencies are not relinked unnecessarily
  • deleted child dependencies do not remain as stale links after a warm install

Verification

  • COREPACK_ENABLE_AUTO_PIN=0 pnpm exec tsgo --build
  • COREPACK_ENABLE_AUTO_PIN=0 pnpm exec cross-env PNPM_REGISTRY_MOCK_PORT=7780 NODE_OPTIONS="$NODE_OPTIONS --experimental-vm-modules --disable-warning=ExperimentalWarning --disable-warning=DEP0169" jest test/install/relinkingScope.test.ts --runInBand
  • COREPACK_ENABLE_AUTO_PIN=0 pnpm exec cross-env PNPM_REGISTRY_MOCK_PORT=7781 NODE_OPTIONS="$NODE_OPTIONS --experimental-vm-modules --disable-warning=ExperimentalWarning --disable-warning=DEP0169" jest test/install/overrides.ts -t "overrides remove dependencies" --runInBand
  • COREPACK_ENABLE_AUTO_PIN=0 pnpm exec cross-env PNPM_REGISTRY_MOCK_PORT=7782 NODE_OPTIONS="$NODE_OPTIONS --experimental-vm-modules --disable-warning=ExperimentalWarning --disable-warning=DEP0169" jest test/install/globalVirtualStore.ts -t "modules are correctly updated when using a global virtual store" --runInBand

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Optimized warm installs to avoid unnecessary relinking of unchanged child dependencies.
    • Improved warm reinstall behavior to remove stale child symlinks when dependencies are updated or removed.
    • Added safer stale-link cleanup for virtual node_modules, including removal of now-empty scoped directories.
  • Tests

    • Added coverage for warm reinstall relinking paths and for obsolete-child unlinking behavior (including traversal protection cases).

@vsumner vsumner requested a review from zkochan as a code owner April 1, 2026 14:06
Copilot AI review requested due to automatic review settings April 1, 2026 14:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes warm-install relinking behavior so that existing packages only relink the subset of child edges that actually changed, and ensures removed child aliases don’t remain as stale links.

Changes:

  • Narrow relinking scope for already-present packages to only changed/removed child aliases.
  • Proactively remove obsolete child links before relinking updated children.
  • Add regression tests covering both “no unnecessary relinks” and “stale link removal” cases.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
installing/deps-installer/src/install/link.ts Computes changed vs removed child aliases and only relinks those; removes obsolete links before relinking.
installing/deps-installer/test/install/relinkingScope.test.ts Adds tests to validate narrowed relinking and stale-link cleanup during warm installs.
.changeset/tidy-drinks-help.md Publishes a patch changeset describing the behavioral fix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@zkochan zkochan force-pushed the fix/narrow-warm-install-relinking branch from 5e47dd5 to ebd0186 Compare June 17, 2026 06:27
Copilot AI review requested due to automatic review settings June 17, 2026 06:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown

Review Change Stack

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: 80f33884-2efc-4d20-af76-579ffa0359c7

📥 Commits

Reviewing files that changed from the base of the PR and between ebd0186 and f08f017.

📒 Files selected for processing (7)
  • installing/deps-installer/src/install/link.ts
  • installing/deps-installer/test/install/relinkingScope.test.ts
  • pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot.rs
  • pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot/tests.rs
  • pacquet/crates/package-manager/src/create_virtual_store.rs
  • pacquet/crates/package-manager/src/create_virtual_store/tests.rs
  • pacquet/crates/package-manager/src/install_package_by_snapshot.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • installing/deps-installer/test/install/relinkingScope.test.ts
  • installing/deps-installer/src/install/link.ts

📝 Walkthrough

Walkthrough

Implements granular child-dependency diffing during warm installs across pnpm and pacquet: pnpm's deps-installer now computes removedAliases using filesystem and lockfile diffs, removing obsolete child directories before symlinking; pacquet's virtual-store pipeline threads this information through CreateVirtualDirBySnapshot to safely unlink stale child aliases while avoiding unnecessary relinking of unchanged edges.

Changes

Warm install relinking fix

Layer / File(s) Summary
pnpm: ModulesLinkJob type and granular change detection
installing/deps-installer/src/install/link.ts
Adds readModulesDir and rimraf imports; introduces ModulesLinkJob type with optional removedAliases; reworks linkNewPackages to build per-dep entries using getActualChildrenDiff (filesystem) or getChangedChildren (lockfile) instead of pushing whole nodes; adds concurrency limiter for module-directory reads.
pnpm: linkAllModules refactor and helper functions
installing/deps-installer/src/install/link.ts
Replaces linkAllModules to remove obsolete child dirs via removeObsoleteChild and rimraf before symlinking; adds helpers getChangedChildren, getActualChildrenDiff, removeObsoleteChild, getChildrenPaths for optional filtering, link: resolution, non-installable exclusion, and scoped-package cleanup.
pnpm: relinking tests and changeset
installing/deps-installer/test/install/relinkingScope.test.ts, .changeset/tidy-drinks-help.md
Adds relinkingScope.test.ts using jest.unstable_mockModule on symlinkAllModules: test 1 asserts unchanged child edges are skipped after version override; test 2 asserts obsolete child symlinks are deleted after dependency removal. Includes patch-release changeset entry.
Rust: CreateVirtualDirBySnapshot—removed aliases field and helper
pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot.rs, pacquet/crates/package-manager/src/create_virtual_dir_by_snapshot/tests.rs
Adds is_subdir and remove_symlink_dir imports; extends struct with removed_aliases: &[PkgName] field and error variant RemoveObsoleteChild; implements remove_obsolete_child helper with path-traversal guard and scoped-directory cleanup; tests verify safe unlinking of obsolete aliases and prevention of traversal attacks.
Rust: Virtual store pipeline—threading removed aliases
pacquet/crates/package-manager/src/create_virtual_store.rs, pacquet/crates/package-manager/src/create_virtual_store/tests.rs
Computes removed_aliases_by_key from current vs. wanted snapshots; supplies removed_aliases to both warm and cold SlotLink construction; extends SlotLink struct and forwards to CreateVirtualDirBySnapshot; adds helpers removed_aliases_for and removed_child_aliases for set-difference logic; tests validate correct identification of dropped dependencies.
Rust: Single-package install path
pacquet/crates/package-manager/src/install_package_by_snapshot.rs
Passes explicit empty removed_aliases slice to CreateVirtualDirBySnapshot for fresh single-package installs, with comment clarifying no stale aliases exist in new installs.

Sequence Diagram(s)

sequenceDiagram
  participant pnpm as pnpm<br/>deps-installer
  participant pacquet as pacquet<br/>virtual store
  participant cvds as CreateVirtualDir<br/>BySnapshot
  participant fs as Filesystem
  pnpm->>pnpm: linkNewPackages<br/>detect changed children
  pnpm->>pnpm: compute removedAliases<br/>getActualChildrenDiff or<br/>getChangedChildren
  pnpm->>pacquet: create SlotLink<br/>with removed_aliases
  pacquet->>pacquet: removed_aliases_by_key<br/>from current vs wanted
  pacquet->>cvds: removed_aliases slice
  cvds->>fs: rayon::join<br/>CAS import + symlinks
  cvds->>fs: for each removed_aliases<br/>remove_obsolete_child
  fs-->>cvds: obsolete aliases unlinked
  cvds-->>pacquet: complete
  pacquet-->>pnpm: virtual store ready
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • pnpm/pnpm#12249: Modifies the cold/warm virtual-store linking pipeline in pacquet, directly overlapping with this PR's threading of removed_aliases into CreateVirtualDirBySnapshot and per-slot materialization.

Suggested labels

product: pacquet

Suggested reviewers

  • zkochan
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: narrow warm install relinking' follows Conventional Commits specification with the 'fix:' prefix and clearly summarizes the main change: narrowing the relinking scope during warm installs.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration.

Warning

Review ran into problems

🔥 Problems

Stopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a @coderabbit review after the pipeline has finished.


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

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

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Prototype-key removal bypass ✓ Resolved 🐞 Bug ≡ Correctness
Description
getChangedChildren() uses alias in wantedChildren to decide whether an alias was removed, but in
consults the prototype chain. If an alias matches an inherited key (e.g. "toString"/"constructor"),
it can be incorrectly treated as still present and skipped from removedAliases, leaving stale child
links after a warm install.
Code

installing/deps-installer/src/install/link.ts[R616-619]

+  for (const alias of Object.keys(currentChildren)) {
+    if (!(alias in wantedChildren)) {
+      removedAliases.push(alias)
+    }
Evidence
The removal detection currently uses the in operator against a plain object created via object
spread, which can misclassify aliases that collide with prototype keys. The repo already uses
null-prototype records for untrusted key sets to avoid this class of bug.

installing/deps-installer/src/install/link.ts[587-621]
installing/deps-resolver/src/validateDependencyAlias.ts[4-13]
catalogs/config/src/mergeCatalogs.ts[10-23]

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

## Issue description
`getChangedChildren()` determines removed child aliases using `if (!(alias in wantedChildren))`, but `in` checks inherited properties too. This can cause aliases that collide with `Object.prototype` keys (e.g. `toString`, `constructor`) to be treated as present even when they were removed, so the unlink pass won’t run and stale links can remain after warm installs.
### Issue Context
`wantedChildren` is built via object spread into a normal object (default prototype), so prototype-chain membership matters.
### Fix Focus Areas
- installing/deps-installer/src/install/link.ts[587-621]
### Proposed fix
- Replace `alias in wantedChildren` with an own-property check, e.g. `Object.hasOwn(wantedChildren, alias)` (Node 18+), or `Object.prototype.hasOwnProperty.call(wantedChildren, alias)`.
- Optionally build `currentChildren`/`wantedChildren` as null-prototype objects (`Object.create(null)`) to avoid prototype collisions entirely.

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



Remediation recommended

2. Vacuous relinking test ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The new relinkingScope test can pass even when no symlinkAllModules call is observed for the target
package because it asserts every(...) on an array that may be empty. This weakens the regression
coverage and can hide failures in the relinking path or in the worker mock setup.
Code

installing/deps-installer/test/install/relinkingScope.test.ts[R64-71]

+  const pkgCalls = symlinkAllModulesCalls
+    .flat()
+    .filter((dep) => dep.name === '@pnpm.e2e/pkg-with-good-optional')
+
+  // Existing packages with only one changed child edge should not be passed
+  // through the broad worker relinking path with unchanged aliases.
+  expect(pkgCalls.every(({ children }) => !children.includes('is-positive'))).toBe(true)
+})
Evidence
pkgCalls is derived by filtering recorded calls, and the test only asserts pkgCalls.every(...).
In JavaScript, [].every(...) returns true, so the expectation can pass even if
symlinkAllModulesCalls never captured a call for the package under test.

installing/deps-installer/test/install/relinkingScope.test.ts[64-71]
installing/deps-installer/test/install/relinkingScope.test.ts[10-24]

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

## Issue description
The test uses `pkgCalls.every(...)` where `pkgCalls` can be empty; `Array.prototype.every` returns `true` for an empty array, so the assertion can succeed even if no relevant relinking call was captured.
## Issue Context
This test is intended to prove that unchanged child aliases are not passed through the broad relinking path. Without asserting that at least one relinking call was observed for `@pnpm.e2e/pkg-with-good-optional`, the test may become a false positive.
## Fix
Add a non-emptiness assertion (e.g. `expect(pkgCalls.length).toBeGreaterThan(0)`) before the `every(...)` check, or assert more directly that at least one captured call exists and does not include `is-positive`.
## Fix Focus Areas
- installing/deps-installer/test/install/relinkingScope.test.ts[64-71]

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


3. Unbounded relink diff fanout ✓ Resolved 🐞 Bug ➹ Performance
Description
linkNewPackages now runs relink-diff work via an unbounded Promise.all over wantedRelDepPaths
(derived from all wanted lockfile packages), and each task may do filesystem reads (readModulesDir).
On large lockfiles this can significantly increase concurrent I/O during warm installs and may lead
to slowdowns or resource-limit failures in constrained environments.
Code

installing/deps-installer/src/install/link.ts[R386-429]

+    await Promise.all(wantedRelDepPaths.map(async (depPath) => {
+      if (currentPackages[depPath] &&
+        (!equals(currentPackages[depPath].dependencies, wantedPackages[depPath].dependencies) ||
+        !isEmpty(currentPackages[depPath].optionalDependencies ?? {}) ||
+        !isEmpty(wantedPackages[depPath].optionalDependencies ?? {}))
) {
// TODO: come up with a test that triggers the usecase of depGraph[depPath] undefined
// see related issue: https://github.com/pnpm/pnpm/issues/870
if (depGraph[depPath] && !newDepPathsSet.has(depPath)) {
-          existingWithUpdatedDeps.push(depGraph[depPath])
+          const { actualChildrenChanged, removedAliases: actualRemovedAliases } = await getActualChildrenDiff(
+            depGraph[depPath],
+            depGraph,
+            opts.lockfileDir,
+            opts.optional
+          )
+          if (actualChildrenChanged) {
+            existingWithUpdatedDeps.push({
+              children: depGraph[depPath].children,
+              modules: depGraph[depPath].modules,
+              name: depGraph[depPath].name,
+              optionalDependencies: depGraph[depPath].optionalDependencies,
+              removedAliases: actualRemovedAliases,
+            })
+            return
+          }
+          const { changedChildren, removedAliases } = getChangedChildren(
+            currentPackages[depPath].dependencies,
+            currentPackages[depPath].optionalDependencies,
+            wantedPackages[depPath].dependencies,
+            wantedPackages[depPath].optionalDependencies,
+            depGraph[depPath].children
+          )
+          if (!isEmpty(changedChildren) || removedAliases.length > 0) {
+            existingWithUpdatedDeps.push({
+              children: changedChildren,
+              modules: depGraph[depPath].modules,
+              name: depGraph[depPath].name,
+              optionalDependencies: depGraph[depPath].optionalDependencies,
+              removedAliases,
+            })
+          }
}
}
-    }
+    }))
Evidence
The code now processes all wanted package depPaths concurrently and may perform filesystem reads per
package; this is a new scalability risk on pnpm’s warm-install hot path.

installing/deps-installer/src/install/link.ts[361-362]
installing/deps-installer/src/install/link.ts[385-430]
fs/read-modules-dir/src/index.ts[8-34]
installing/deps-installer/src/install/link.ts[497-560]

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

## Issue description
`linkNewPackages()` launches an unbounded number of async tasks (`Promise.all(wantedRelDepPaths.map(...))`) where each task may touch the filesystem (e.g. `readModulesDir()` -> `readdir`). On large lockfiles this can cause high concurrent I/O and memory pressure during warm installs.
### Issue Context
- `wantedRelDepPaths` is computed from `Object.keys(wantedLockfile.packages ?? {})`, i.e. it can be very large.
- The task body may call `getActualChildrenDiff()` which calls `readModulesDir()`.
### Fix Focus Areas
- installing/deps-installer/src/install/link.ts[361-362]
- installing/deps-installer/src/install/link.ts[385-430]
- installing/deps-installer/src/install/link.ts[619-634]
### Suggested fix
- Introduce a `pLimit(N)` (or reuse an existing limiter if appropriate) to cap concurrency for the per-`depPath` work in the `existingWithUpdatedDeps` scan.
- Example pattern:
- `const limitRelinkScan = pLimit(16)`
- `await Promise.all(wantedRelDepPaths.map((depPath) => limitRelinkScan(async () => { ... })))`
- Consider similarly bounding the `removedAliases` cleanup fanout in `linkAllModules()` if it can grow large in practice.

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


Grey Divider

Qodo Logo

Comment thread installing/deps-installer/src/install/link.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
installing/deps-installer/src/install/link.ts (1)

585-591: ⚡ Quick win

Refactor getChangedChildren() to an options object (currently 5 positional args).

This breaks the repo’s TS style rule and makes callsites easier to misorder.

Suggested refactor
-function getChangedChildren (
-  currentDependencies: Record<string, string> | undefined,
-  currentOptionalDependencies: Record<string, string> | undefined,
-  wantedDependencies: Record<string, string> | undefined,
-  wantedOptionalDependencies: Record<string, string> | undefined,
-  allChildren: Record<string, DepPath>
-): { changedChildren: Record<string, DepPath>, removedAliases: string[] } {
+function getChangedChildren (opts: {
+  currentDependencies: Record<string, string> | undefined
+  currentOptionalDependencies: Record<string, string> | undefined
+  wantedDependencies: Record<string, string> | undefined
+  wantedOptionalDependencies: Record<string, string> | undefined
+  allChildren: Record<string, DepPath>
+}): { changedChildren: Record<string, DepPath>, removedAliases: string[] } {
   const currentChildren = {
-    ...currentDependencies,
-    ...currentOptionalDependencies,
+    ...opts.currentDependencies,
+    ...opts.currentOptionalDependencies,
   }
   const wantedChildren = {
-    ...wantedDependencies,
-    ...wantedOptionalDependencies,
+    ...opts.wantedDependencies,
+    ...opts.wantedOptionalDependencies,
   }
-const { changedChildren, removedAliases } = getChangedChildren(
-  currentPackages[depPath].dependencies,
-  currentPackages[depPath].optionalDependencies,
-  wantedPackages[depPath].dependencies,
-  wantedPackages[depPath].optionalDependencies,
-  depGraph[depPath].children
-)
+const { changedChildren, removedAliases } = getChangedChildren({
+  currentDependencies: currentPackages[depPath].dependencies,
+  currentOptionalDependencies: currentPackages[depPath].optionalDependencies,
+  wantedDependencies: wantedPackages[depPath].dependencies,
+  wantedOptionalDependencies: wantedPackages[depPath].optionalDependencies,
+  allChildren: depGraph[depPath].children,
+})

As per coding guidelines: “functions should have ≤2-3 arguments (use options object for more).”

Also applies to: 411-417

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@installing/deps-installer/src/install/link.ts` around lines 585 - 591,
Refactor the getChangedChildren function to accept a single options object
parameter instead of 5 positional arguments (currentDependencies,
currentOptionalDependencies, wantedDependencies, wantedOptionalDependencies,
allChildren). Create a new interface or type to define the shape of this options
object with all these properties, update the function signature to accept this
options object as the single parameter, and then update all callsites throughout
the codebase where getChangedChildren is invoked to pass the arguments as an
object literal instead of positional arguments. Apply the same refactoring
pattern to the other related function mentioned at lines 411-417.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@installing/deps-installer/src/install/link.ts`:
- Around line 636-640: The removeObsoleteChild function is vulnerable to path
traversal attacks because the alias parameter is joined directly into the
deletion path without validation. Before calling rimraf on the path constructed
from modulesDir and alias, add a boundary check to ensure the resolved absolute
path stays within the modulesDir boundaries. Use a path resolution and
validation approach to verify that the target path does not escape the intended
modulesDir directory, rejecting any alias values that would traverse outside it.

---

Nitpick comments:
In `@installing/deps-installer/src/install/link.ts`:
- Around line 585-591: Refactor the getChangedChildren function to accept a
single options object parameter instead of 5 positional arguments
(currentDependencies, currentOptionalDependencies, wantedDependencies,
wantedOptionalDependencies, allChildren). Create a new interface or type to
define the shape of this options object with all these properties, update the
function signature to accept this options object as the single parameter, and
then update all callsites throughout the codebase where getChangedChildren is
invoked to pass the arguments as an object literal instead of positional
arguments. Apply the same refactoring pattern to the other related function
mentioned at lines 411-417.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 10322ef0-3129-455a-8224-12d0691dd997

📥 Commits

Reviewing files that changed from the base of the PR and between 9d79ba1 and ebd0186.

📒 Files selected for processing (3)
  • .changeset/tidy-drinks-help.md
  • installing/deps-installer/src/install/link.ts
  • installing/deps-installer/test/install/relinkingScope.test.ts

Comment thread installing/deps-installer/src/install/link.ts
Addresses PR review feedback:
- Cap the per-package relink-diff scan and obsolete-child cleanup with pLimit
  to avoid unbounded concurrent modules-dir reads/removals on large lockfiles.
- Guard removeObsoleteChild with isValidDependencyAlias so a crafted alias
  cannot escape the modules directory.
- Pass getChangedChildren arguments via an options object (repo style).
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

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

Ports the stale-child-removal behavior from the TypeScript pnpm CLI
(installing/deps-installer linkAllModules / removeObsoleteChild) to
pacquet's isolated linker. When a survivor snapshot's dependency set
shrinks across installs (e.g. via an override), its slot is
re-materialized but the dropped child's symlink was previously left
behind dangling.

CreateVirtualDirBySnapshot now unlinks the obsolete child aliases —
computed from the current-vs-wanted snapshot diff in CreateVirtualStore
and threaded through SlotLink — after the import/symlink join. A
node_modules-boundary guard (is_subdir) skips any alias that would
escape the slot, since PkgName parsing accepts shapes such as '..'.
Copilot AI review requested due to automatic review settings June 17, 2026 08:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Comment thread installing/deps-installer/src/install/link.ts
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

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

zkochan added 2 commits June 17, 2026 11:03
The fixture's '*' range made both installs resolve the
`@pnpm.e2e/dep-of-pkg-with-1-dep` child to the same highest version,
so the package was never relinked and the `every` assertion passed on
an empty array. Pin the child to 100.0.0 on the first install so the
override to 101.0.0 is a real edge change, and assert the package is
actually relinked for the changed child.
getChangedChildren detected removed children with `alias in
wantedChildren`, which walks the prototype chain — a dependency
literally named `constructor`, `toString`, `__proto__`, etc. (all
valid npm names) would match an inherited key and never be flagged for
removal. Build the merged child maps with a null prototype and use
`Object.hasOwn` for every membership and lookup so such aliases are
handled as ordinary children.
Copilot AI review requested due to automatic review settings June 17, 2026 09:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code Review by Qodo

Grey Divider

New Review Started

This review has been superseded by a new analysis

Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

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

@zkochan zkochan enabled auto-merge (squash) June 17, 2026 09:26
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.57143% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.13%. Comparing base (61969fb) to head (f08f017).
⚠️ Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
...kage-manager/src/create_virtual_dir_by_snapshot.rs 83.33% 4 Missing ⚠️
...package-manager/src/install_package_by_snapshot.rs 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11169      +/-   ##
==========================================
+ Coverage   88.09%   88.13%   +0.04%     
==========================================
  Files         310      310              
  Lines       41855    41934      +79     
==========================================
+ Hits        36871    36958      +87     
+ Misses       4984     4976       -8     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

Copy link
Copy Markdown
Contributor

Integrated-Benchmark Report (Linux)

Each scenario reports direct installs and pnpr installs. Bencher consumes pacquet@HEAD and pnpr@HEAD.

Scenario: Isolated linker: fresh restore, cold cache + cold store

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 4.241 ± 0.227 3.966 4.696 2.03 ± 0.17
pacquet@main 4.163 ± 0.115 4.054 4.391 1.99 ± 0.14
pnpr@HEAD 2.109 ± 0.080 1.968 2.197 1.01 ± 0.08
pnpr@main 2.090 ± 0.136 1.916 2.307 1.00
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 4.2412800526000005,
      "stddev": 0.22683239987782308,
      "median": 4.1985795425,
      "user": 4.00660286,
      "system": 3.3817977,
      "min": 3.965574473,
      "max": 4.696096197,
      "times": [
        4.201253573,
        4.109524805,
        4.458828163,
        4.195905512,
        4.696096197,
        4.346571791,
        4.070148228,
        4.004135914,
        3.965574473,
        4.36476187
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 4.1625301827,
      "stddev": 0.11476959046940788,
      "median": 4.126548039499999,
      "user": 3.99637756,
      "system": 3.3926023,
      "min": 4.054485314,
      "max": 4.390529581,
      "times": [
        4.054485314,
        4.117979995,
        4.135116084,
        4.390529581,
        4.223309585,
        4.086389754,
        4.135498656,
        4.076435121,
        4.327839617,
        4.07771812
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 2.1091631237,
      "stddev": 0.08008606814871859,
      "median": 2.1354857215000003,
      "user": 2.71968236,
      "system": 2.8844069,
      "min": 1.967574501,
      "max": 2.197350206,
      "times": [
        2.1821930910000003,
        2.144289389,
        2.006885177,
        2.030381906,
        2.164056144,
        2.166873466,
        2.105345303,
        1.967574501,
        2.126682054,
        2.197350206
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 2.0903032432,
      "stddev": 0.1357221447141442,
      "median": 2.076934926,
      "user": 2.71941096,
      "system": 2.8656549,
      "min": 1.9157453590000002,
      "max": 2.307237798,
      "times": [
        2.252503323,
        2.226972097,
        1.9157453590000002,
        2.104282299,
        2.040700606,
        2.090452941,
        2.063416911,
        1.9285116700000002,
        1.973209428,
        2.307237798
      ]
    }
  ]
}

Scenario: Isolated linker: fresh restore, hot cache + hot store

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 624.1 ± 11.4 606.7 642.7 1.00
pacquet@main 645.1 ± 64.3 613.7 825.6 1.03 ± 0.10
pnpr@HEAD 661.0 ± 16.0 642.8 695.8 1.06 ± 0.03
pnpr@main 706.2 ± 63.0 666.5 864.8 1.13 ± 0.10
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.6240940804999999,
      "stddev": 0.011388424012731069,
      "median": 0.6210787768,
      "user": 0.35933116,
      "system": 1.31089612,
      "min": 0.6067440373,
      "max": 0.6426758043,
      "times": [
        0.6208105873,
        0.6180584483,
        0.6067440373,
        0.6374787113,
        0.6297693383,
        0.6426758043,
        0.6168826623,
        0.6213469663,
        0.6135290663,
        0.6336451833
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.6450907799000001,
      "stddev": 0.06425757609815795,
      "median": 0.6226166298,
      "user": 0.37778765999999997,
      "system": 1.3157603199999999,
      "min": 0.6136716063,
      "max": 0.8255818333,
      "times": [
        0.6136716063,
        0.6157211433,
        0.6167058123,
        0.6195937133,
        0.6290855593,
        0.6230046243,
        0.6430263963,
        0.6222286353,
        0.6422884753,
        0.8255818333
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 0.6610113580999999,
      "stddev": 0.01604148966500461,
      "median": 0.6632414548000001,
      "user": 0.37797826,
      "system": 1.3284559199999997,
      "min": 0.6428457103,
      "max": 0.6957630713,
      "times": [
        0.6501939213,
        0.6430894883,
        0.6478840333,
        0.6695319183,
        0.6658049333,
        0.6615258493,
        0.6649570603,
        0.6428457103,
        0.6957630713,
        0.6685175953
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 0.7061862507,
      "stddev": 0.06302998144988235,
      "median": 0.6789732518,
      "user": 0.38816365999999997,
      "system": 1.33595872,
      "min": 0.6665286883,
      "max": 0.8648377843,
      "times": [
        0.7688807083,
        0.6892394873,
        0.6910094553,
        0.6779961893,
        0.6779599643,
        0.6799503143,
        0.6665286883,
        0.6774985283,
        0.8648377843,
        0.6679613873
      ]
    }
  ]
}

Scenario: Isolated linker: fresh install, cold cache + cold store

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 4.219 ± 0.031 4.165 4.254 1.96 ± 0.11
pacquet@main 4.193 ± 0.060 4.077 4.286 1.95 ± 0.11
pnpr@HEAD 2.193 ± 0.166 1.907 2.454 1.02 ± 0.10
pnpr@main 2.151 ± 0.119 2.023 2.416 1.00
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 4.218699137840001,
      "stddev": 0.030557577329297588,
      "median": 4.22238153584,
      "user": 3.8315508599999992,
      "system": 3.26158536,
      "min": 4.16501844984,
      "max": 4.25397275184,
      "times": [
        4.21100507084,
        4.19738365184,
        4.23375800084,
        4.234947581839999,
        4.25387577484,
        4.19320082784,
        4.16501844984,
        4.19578536584,
        4.25397275184,
        4.24804390284
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 4.19347474684,
      "stddev": 0.06030653401954651,
      "median": 4.19353782284,
      "user": 3.80265506,
      "system": 3.27542296,
      "min": 4.0767121268399995,
      "max": 4.28552269384,
      "times": [
        4.251563175839999,
        4.19202722084,
        4.15000025284,
        4.1950484248399995,
        4.21736794084,
        4.28552269384,
        4.0767121268399995,
        4.2435877158399995,
        4.17205338184,
        4.150864534839999
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 2.19316452274,
      "stddev": 0.16634899457448876,
      "median": 2.2068893998400005,
      "user": 2.5294065599999995,
      "system": 2.80186116,
      "min": 1.90715235684,
      "max": 2.45372377284,
      "times": [
        2.14158143984,
        2.03088174884,
        2.2454749458400003,
        2.28938994184,
        2.3330420538400003,
        2.31867779884,
        2.04341731484,
        2.45372377284,
        2.1683038538400004,
        1.90715235684
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 2.15095910594,
      "stddev": 0.11877262600925553,
      "median": 2.13465084184,
      "user": 2.5746077599999997,
      "system": 2.80619386,
      "min": 2.02311883484,
      "max": 2.4163542228400003,
      "times": [
        2.4163542228400003,
        2.25092656784,
        2.15414508284,
        2.03893589184,
        2.2126366718400003,
        2.05757705284,
        2.02311883484,
        2.1493631208400004,
        2.1199385628400003,
        2.08659505084
      ]
    }
  ]
}

Scenario: Isolated linker: fresh install, hot cache + hot store

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 1.384 ± 0.021 1.362 1.433 2.09 ± 0.10
pacquet@main 1.431 ± 0.019 1.407 1.473 2.16 ± 0.10
pnpr@HEAD 0.662 ± 0.029 0.639 0.737 1.00
pnpr@main 0.673 ± 0.082 0.632 0.905 1.02 ± 0.13
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 1.3839724284600001,
      "stddev": 0.02104932649005873,
      "median": 1.3768413120599998,
      "user": 1.3473578000000002,
      "system": 1.7231082199999999,
      "min": 1.36228591806,
      "max": 1.43304518906,
      "times": [
        1.3725904280599999,
        1.37527881006,
        1.3679285480599999,
        1.43304518906,
        1.36228591806,
        1.37777379006,
        1.37641148206,
        1.40489582706,
        1.37727114206,
        1.3922431500599999
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 1.43073276936,
      "stddev": 0.018515795070857086,
      "median": 1.43049986656,
      "user": 1.3963687999999999,
      "system": 1.78295642,
      "min": 1.40700921306,
      "max": 1.47291201106,
      "times": [
        1.43195694706,
        1.43534634206,
        1.41976568106,
        1.47291201106,
        1.40954287706,
        1.4415718310599999,
        1.42619278406,
        1.42904278606,
        1.40700921306,
        1.43398722106
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 0.6616545621600001,
      "stddev": 0.028683411417776323,
      "median": 0.6517052320600001,
      "user": 0.3329807,
      "system": 1.29043252,
      "min": 0.63884845706,
      "max": 0.7374365290600001,
      "times": [
        0.6443199050600001,
        0.63884845706,
        0.7374365290600001,
        0.65024631606,
        0.6561353770600001,
        0.6753603430600001,
        0.6657659800600001,
        0.6524647470600001,
        0.6450222500600001,
        0.65094571706
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 0.67282716856,
      "stddev": 0.08216456753746776,
      "median": 0.6489194120600001,
      "user": 0.34838269999999993,
      "system": 1.2741561200000002,
      "min": 0.6319427860600001,
      "max": 0.9054231850600001,
      "times": [
        0.6375064110600001,
        0.6483683160600001,
        0.6494705080600001,
        0.6544377710600001,
        0.6585871470600001,
        0.6430526980600001,
        0.6568832480600001,
        0.6425996150600001,
        0.6319427860600001,
        0.9054231850600001
      ]
    }
  ]
}

Scenario: Isolated linker: fresh install, cold cache + hot store

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 3.100 ± 0.052 3.009 3.176 4.62 ± 0.12
pacquet@main 3.117 ± 0.048 3.049 3.233 4.64 ± 0.12
pnpr@HEAD 0.671 ± 0.014 0.652 0.692 1.00
pnpr@main 0.684 ± 0.022 0.658 0.731 1.02 ± 0.04
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 3.1003704451600003,
      "stddev": 0.0523418491425819,
      "median": 3.0892983364599997,
      "user": 1.8378090800000002,
      "system": 1.9949955199999998,
      "min": 3.0089389504599997,
      "max": 3.17598019746,
      "times": [
        3.08481041546,
        3.14592403946,
        3.06835312246,
        3.17598019746,
        3.11561502646,
        3.0937862574599997,
        3.08233902246,
        3.0089389504599997,
        3.16995578346,
        3.05800163646
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 3.1171883088600003,
      "stddev": 0.04825033168834671,
      "median": 3.10443104296,
      "user": 1.8904106799999998,
      "system": 2.0089941199999997,
      "min": 3.04901184646,
      "max": 3.23346684346,
      "times": [
        3.04901184646,
        3.09205403546,
        3.13963915646,
        3.10087969346,
        3.1258851174599998,
        3.1321214934599997,
        3.10734841846,
        3.08996281646,
        3.23346684346,
        3.10151366746
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 0.67143092616,
      "stddev": 0.01357326406895724,
      "median": 0.67110385246,
      "user": 0.35335268,
      "system": 1.30091292,
      "min": 0.65195193646,
      "max": 0.69196074546,
      "times": [
        0.69196074546,
        0.67859688546,
        0.66820039346,
        0.66809097946,
        0.65551191246,
        0.65195193646,
        0.66042127146,
        0.69130472646,
        0.67426309946,
        0.67400731146
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 0.6841348822600001,
      "stddev": 0.022088111195477086,
      "median": 0.67707483246,
      "user": 0.37086318,
      "system": 1.30498682,
      "min": 0.65805183746,
      "max": 0.7306595984600001,
      "times": [
        0.69218709446,
        0.67430227646,
        0.67720214846,
        0.65805183746,
        0.68626264746,
        0.7306595984600001,
        0.67694751646,
        0.67330662346,
        0.71023973146,
        0.66218934846
      ]
    }
  ]
}

@github-actions

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Branchpr/11169
Testbedpacquet
Click to view all benchmark results
BenchmarkLatencyBenchmark Result
milliseconds (ms)
(Result Δ%)
Upper Boundary
milliseconds (ms)
(Limit %)
isolated-linker.fresh-install.cold-cache.cold-store📈 view plot
🚷 view threshold
4,218.70 ms
(+0.28%)Baseline: 4,206.88 ms
5,048.25 ms
(83.57%)
isolated-linker.fresh-install.cold-cache.hot-store📈 view plot
🚷 view threshold
3,100.37 ms
(+2.89%)Baseline: 3,013.43 ms
3,616.12 ms
(85.74%)
isolated-linker.fresh-install.hot-cache.hot-store📈 view plot
🚷 view threshold
1,383.97 ms
(+4.20%)Baseline: 1,328.19 ms
1,593.83 ms
(86.83%)
isolated-linker.fresh-restore.cold-cache.cold-store📈 view plot
🚷 view threshold
4,241.28 ms
(+1.74%)Baseline: 4,168.86 ms
5,002.63 ms
(84.78%)
isolated-linker.fresh-restore.hot-cache.hot-store📈 view plot
🚷 view threshold
624.09 ms
(+1.65%)Baseline: 613.94 ms
736.72 ms
(84.71%)
🐰 View full continuous benchmarking report in Bencher

@github-actions

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Branchpr/11169
Testbedpnpr

⚠️ WARNING: No Threshold found!

Without a Threshold, no Alerts will ever be generated.

Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the --ci-only-thresholds flag.

Click to view all benchmark results
BenchmarkLatencymilliseconds (ms)
isolated-linker.fresh-install.cold-cache.cold-store📈 view plot
⚠️ NO THRESHOLD
2,193.16 ms
isolated-linker.fresh-install.cold-cache.hot-store📈 view plot
⚠️ NO THRESHOLD
671.43 ms
isolated-linker.fresh-install.hot-cache.hot-store📈 view plot
⚠️ NO THRESHOLD
661.65 ms
isolated-linker.fresh-restore.cold-cache.cold-store📈 view plot
⚠️ NO THRESHOLD
2,109.16 ms
isolated-linker.fresh-restore.hot-cache.hot-store📈 view plot
⚠️ NO THRESHOLD
661.01 ms
🐰 View full continuous benchmarking report in Bencher

@zkochan zkochan disabled auto-merge June 17, 2026 11:35
@zkochan zkochan merged commit 1c05876 into pnpm:main Jun 17, 2026
27 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants