fix: narrow warm install relinking#11169
Conversation
There was a problem hiding this comment.
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.
5e47dd5 to
ebd0186
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughImplements granular child-dependency diffing during warm installs across pnpm and pacquet: pnpm's deps-installer now computes ChangesWarm install relinking fix
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
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Warning Review ran into problems🔥 ProblemsStopped 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 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. Comment |
Code Review by Qodo
1.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
installing/deps-installer/src/install/link.ts (1)
585-591: ⚡ Quick winRefactor
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
📒 Files selected for processing (3)
.changeset/tidy-drinks-help.mdinstalling/deps-installer/src/install/link.tsinstalling/deps-installer/test/install/relinkingScope.test.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).
|
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 '..'.
|
Code review by qodo was updated up to the latest commit ba8d3a9 |
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.
|
Code review by qodo was updated up to the latest commit f08f017 |
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
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
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
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
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
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
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
]
}
]
} |
|
| Branch | pr/11169 |
| Testbed | pacquet |
Click to view all benchmark results
| Benchmark | Latency | Benchmark 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%) |
|
| Branch | pr/11169 |
| Testbed | pnpr |
⚠️ 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-thresholdsflag.
Click to view all benchmark results
| Benchmark | Latency | milliseconds (ms) |
|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot | 2,193.16 ms |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot | 671.43 ms |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot | 661.65 ms |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot | 2,109.16 ms |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot | 661.01 ms |
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:
Verification
COREPACK_ENABLE_AUTO_PIN=0 pnpm exec tsgo --buildCOREPACK_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 --runInBandCOREPACK_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" --runInBandCOREPACK_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" --runInBandSummary by CodeRabbit
Release Notes
Bug Fixes
node_modules, including removal of now-empty scoped directories.Tests