perf(pacquet/package-manager): treat unsupported settings as no-opinion in optimistic-repeat-install#12005
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe settings comparison now compares only the fields produced by ChangesSettings comparison and drift detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Review Summary by QodoAdd diagnostic logging for optimistic repeat install rejections
WalkthroughsDescription• Add comprehensive debug logging to check_optimistic_repeat_install function - Logs specific rejection reasons: settings drift, missing modules, stale manifests - Names the camelCase setting that differs (e.g., allowBuilds, peersSuffixMaxLength) - Reports missing directory paths and manifest mtimes for diagnostics • Refactor decision handling in Install::run to match all decision variants - Logs Skipped decisions via tracing::debug! for benchmark analysis - Enables TRACE=pacquet::optimistic_repeat_install=debug to surface rejection gates • Enhance helper functions to return first failing item instead of boolean - first_setting_drift returns setting name causing mismatch - missing_modules_dir returns path of absent directory - first_newer_manifest returns path of stale manifest • All 17 existing tests pass; behavior and error codes unchanged Diagramflowchart LR
A["check_optimistic_repeat_install"] -->|"settings drift"| B["first_setting_drift"]
A -->|"missing modules"| C["missing_modules_dir"]
A -->|"stale manifest"| D["first_newer_manifest"]
A -->|"state load error"| E["tracing::debug!"]
B -->|"setting_name"| F["tracing::debug!"]
C -->|"missing_dir path"| F
D -->|"manifest path"| F
F -->|"Decision::Skipped"| G["Install::run logs reason"]
File Changes1. pacquet/crates/package-manager/src/install.rs
|
Contributor
Micro-Benchmark ResultsLinux |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12005 +/- ##
=======================================
Coverage 88.14% 88.15%
=======================================
Files 228 228
Lines 28523 28547 +24
=======================================
+ Hits 25142 25165 +23
- Misses 3381 3382 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Integrated-Benchmark Report (Linux)Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.94771179524,
"stddev": 0.07859864824905877,
"median": 1.92524792364,
"user": 2.6343886999999997,
"system": 3.3739079599999995,
"min": 1.87842708064,
"max": 2.13156535764,
"times": [
1.92239752364,
1.87842708064,
1.8883684696399998,
1.88149119564,
1.96705957164,
1.89338435664,
2.13156535764,
1.97573479764,
2.01059127564,
1.92809832364
]
},
{
"command": "pacquet@main",
"mean": 1.9229670344399998,
"stddev": 0.06899595435424474,
"median": 1.89669476014,
"user": 2.7607906000000004,
"system": 3.34948596,
"min": 1.8703543946399999,
"max": 2.0770909096400003,
"times": [
2.0770909096400003,
1.88570664564,
1.89336455364,
1.90247817564,
2.0232290436400002,
1.9038754706399998,
1.89454231564,
1.8703543946399999,
1.8801816306399999,
1.89884720464
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6277924196400001,
"stddev": 0.038447326338102446,
"median": 0.6154318489400001,
"user": 0.34941481999999996,
"system": 1.32561286,
"min": 0.60787272694,
"max": 0.7358377059400001,
"times": [
0.7358377059400001,
0.6290754979400001,
0.6157460649400001,
0.60823736394,
0.6139996519400001,
0.61320303594,
0.6193649289400001,
0.6194695869400001,
0.60787272694,
0.6151176329400001
]
},
{
"command": "pacquet@main",
"mean": 0.6392799328400001,
"stddev": 0.027832568260846242,
"median": 0.62888314844,
"user": 0.37956112000000003,
"system": 1.3299999599999999,
"min": 0.61510265894,
"max": 0.7088502079400001,
"times": [
0.7088502079400001,
0.62802468194,
0.6319028349400001,
0.61510265894,
0.6612354139400001,
0.6467627829400001,
0.6222146069400001,
0.62248006594,
0.62974161494,
0.62648445994
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.2216002002399997,
"stddev": 0.03403456122241408,
"median": 2.22481444884,
"user": 3.72304192,
"system": 3.13585354,
"min": 2.16531780234,
"max": 2.28438916834,
"times": [
2.18669146634,
2.21091009434,
2.19531303134,
2.22354602034,
2.2496239513400003,
2.22608287734,
2.23571670734,
2.2384108833400003,
2.16531780234,
2.28438916834
]
},
{
"command": "pacquet@main",
"mean": 2.20457087144,
"stddev": 0.04277838614055283,
"median": 2.20076785084,
"user": 3.79416442,
"system": 3.07185014,
"min": 2.14177991834,
"max": 2.28343286634,
"times": [
2.28343286634,
2.24553734634,
2.1893974533400002,
2.23791242234,
2.20203786034,
2.14840764634,
2.19949784134,
2.20309509034,
2.14177991834,
2.19461026934
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.3782522250000002,
"stddev": 0.02223022254620925,
"median": 1.3833297729,
"user": 1.6360304399999996,
"system": 1.7744545799999998,
"min": 1.3450235399000001,
"max": 1.4085520889,
"times": [
1.3967103959,
1.3450235399000001,
1.3457174179,
1.3898267709,
1.3745033199,
1.3581840999,
1.3948482189,
1.3923236229,
1.3768327749,
1.4085520889
]
},
{
"command": "pacquet@main",
"mean": 1.4176806976,
"stddev": 0.02943639176492092,
"median": 1.4285053999000001,
"user": 1.6960483399999997,
"system": 1.78889248,
"min": 1.3487996439,
"max": 1.4538308469,
"times": [
1.4319838049,
1.4538308469,
1.4343444269,
1.4294731979000002,
1.4024665329000001,
1.4387409619,
1.4014580269,
1.4081719319000001,
1.4275376019000001,
1.3487996439
]
}
]
} |
Contributor
|
Bencher Report| Branch | pr/12005 |
| 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 | 2,221.60 ms(-6.59%)Baseline: 2,378.43 ms | 2,854.12 ms (77.84%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,378.25 ms(-9.31%)Baseline: 1,519.75 ms | 1,823.70 ms (75.57%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 1,947.71 ms(-4.13%)Baseline: 2,031.57 ms | 2,437.88 ms (79.89%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 627.79 ms(-5.21%)Baseline: 662.32 ms | 794.78 ms (78.99%) |
8 tasks
…on in optimistic-repeat-install The `state.settings == current` check compared every field on `WorkspaceStateSettings`, including settings pacquet doesn't yet implement (`peersSuffixMaxLength`, `dedupeDirectDeps`, `packageExtensions`, …). Pnpm's defaults populate those fields when it writes the state, while pacquet's `current_settings` leaves them `None`, so any cross-package-manager scenario (pnpm wrote the workspace state, pacquet runs next) rejected the fast path on iter 1 and fell into the slower frozen no-op short-circuit. That's the 9.09× `babylon × lockfile+node_modules` regression in #11992. Switch `settings_match` to compare only the fields `current_settings` actively writes. Mirrors pnpm's [`Object.entries(workspaceState.settings)`](https://github.com/pnpm/pnpm/blob/72d997cc34/deps/status/src/checkDepsStatus.ts) walk: pnpm iterates the settings present in the state, which by symmetry are the settings the writer cared about. Pacquet's `current_settings` is the symmetric "settings pacquet cares about" set, so comparing against it is the natural way to honour the same contract. Special-case `allowBuilds`: pnpm writes `Some({})` for an empty allow-list, pacquet writes `None` — both mean "no opinion," so treat them as equivalent (mirroring pnpm's `opts.allowBuilds ?? {}` coercion on the read side). The settings *not* compared are tracked at #12009 with a matching skip-list comment in code; each one drops out of the skip list as it's ported end-to-end (yaml plumbing → `Config` field → real install consumer → joined into `current_settings`). Two extra groups (`minimumReleaseAge*`, `trustPolicy*`) hitch a ride: pacquet *does* consume them during install, but they aren't surfaced through the workspace-state crate yet. And two stay outside the comparison permanently — `catalogs` (pnpm itself always ignores) and `workspacePackagePatterns` (covered via `WorkspaceManifest.packages` from `pnpm-workspace.yaml`). End-to-end on `vltpkg/benchmarks/fixtures/babylon`, after pnpm has written the workspace-state file: - before fix: pacquet iter-1 falls through to the frozen no-op short-circuit (~531 ms locally, ~9× pnpm on vlt CI). - after fix: pacquet iter-1 fires the optimistic fast path (~96 ms locally, faster than pnpm's own warm fast path at ~687 ms — same 7-8× lead as iter-2+). The `returns_skipped_when_unported_pnpm_settings_present` test that locked in the conservative posture is replaced with `returns_up_to_date_when_state_carries_unported_pnpm_settings`, which exercises every #12009 setting plus `catalogs` and `workspacePackagePatterns` together. A second new test, `returns_up_to_date_when_state_has_empty_allow_builds_and_current_has_none`, locks in the `allowBuilds` empty-vs-`None` coercion. Closes #11992. Refs: #12009
This was referenced May 28, 2026
This was referenced Jun 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
state.settings == currentinsettings_matchcompared every field onWorkspaceStateSettings, including settings pacquet doesn't yet implement (peersSuffixMaxLength,dedupeDirectDeps,packageExtensions, …). Pnpm's defaults populate those fields when it writes the state; pacquet'scurrent_settingsleaves themNone. So any cross-package-manager scenario — pnpm wrote the workspace-state file, pacquet runs next — rejected the optimistic fast path and fell into the slower frozen no-op short-circuit on iter 1. That's the 9.09×babylon × lockfile+node_modulesregression in #11992.This PR switches
settings_matchto compare only the fieldscurrent_settingsactively writes, mirroring pnpm'sObject.entries(workspaceState.settings)walk semantically. It also special-casesallowBuildsso pnpm'sSome({})and pacquet'sNonematch (both mean "no opinion"), mirroring pnpm'sopts.allowBuilds ?? {}coercion on the read side.What's not compared, and why
Each entry below has a matching line in
settings_match's skip-list comment. As each one is ported end-to-end (yaml plumbing →Configfield → real install consumer → joined intocurrent_settings), it drops out of the skip-list and starts gating the fast path. See #12009 for the porting tracker.Tracked at #12009 (genuinely unimplemented in pacquet today):
dedupeDirectDeps,dedupeInjectedDeps,dedupePeersexcludeLinksFromLockfileinjectWorkspacePackagespackageExtensionspeersSuffixMaxLengthpreferWorkspacePackagesHitch a ride on the same skip-list (pacquet does consume them during install, but they aren't surfaced into the workspace-state crate yet — separate short follow-up):
minimumReleaseAge,minimumReleaseAgeExclude,minimumReleaseAgeIgnoreMissingTime,minimumReleaseAgeStricttrustPolicy,trustPolicyExclude,trustPolicyIgnoreAfterPermanently outside the comparison:
catalogs— pnpm itself always ignores (ignoredSettings.add('catalogs'))workspacePackagePatterns— already tracked viaWorkspaceManifest.packagesfrompnpm-workspace.yamlTest plan
End-to-end on
vltpkg/benchmarks/fixtures/babylon, after pnpm has written the workspace-state file:After the fix, pacquet's iter-1 is 7× faster than pnpm's warm install on the same fixture, matching iter-2's behavior.
cargo nextest run -p pacquet-package-manager optimistic_repeat_install— 18/18 pass (the conservative-posture testreturns_skipped_when_unported_pnpm_settings_presentwas replaced withreturns_up_to_date_when_state_carries_unported_pnpm_settingscovering every Port pnpm settings missing from pacquet #12009 setting +catalogs+workspacePackagePatterns; second new test locks in theallowBuildsempty-vs-Nonecoercion).cargo clippy --locked -p pacquet-package-manager --all-targets -- --deny warnings— cleancargo fmt --check— cleanCloses #11992. Refs #12009.
Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Bug Fixes
Tests