perf(network): schedule tarball downloads by estimated pipeline work#12309
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 (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🧰 Additional context used📓 Path-based instructions (1)pacquet/**/*.rs📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
🧠 Learnings (4)📚 Learning: 2026-05-20T19:40:55.051ZApplied to files:
📚 Learning: 2026-05-22T00:08:44.646ZApplied to files:
📚 Learning: 2026-05-20T23:07:58.444ZApplied to files:
📚 Learning: 2026-06-06T18:58:37.156ZApplied to files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughThis PR threads optional dist hints (unpacked size, file count) from resolvers and pnpr frames into prefetch and download plumbing, computes per-tarball download priority from those hints, replaces the install concurrency semaphore with a priority-aware semaphore, and adds TCP slow-start modeling to integrated benchmarks. ChangesDownload Prioritization and Metadata Propagation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 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 |
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12309 +/- ##
========================================
Coverage 87.69% 87.70%
========================================
Files 289 290 +1
Lines 35564 35809 +245
========================================
+ Hits 31188 31406 +218
- Misses 4376 4403 +27 ☔ 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": 9.969630896879998,
"stddev": 0.1550251834253097,
"median": 9.91901494508,
"user": 3.1808280399999997,
"system": 3.3563805,
"min": 9.84338275358,
"max": 10.338923296579999,
"times": [
10.12603487758,
9.91160025258,
9.92642963758,
9.88032191558,
10.01327919658,
9.86540770558,
9.85983682358,
10.338923296579999,
9.93109250958,
9.84338275358
]
},
{
"command": "pacquet@main",
"mean": 9.911040212980001,
"stddev": 0.10087549915920976,
"median": 9.86913397408,
"user": 3.15854794,
"system": 3.3546278999999997,
"min": 9.82526844058,
"max": 10.11998293158,
"times": [
9.82526844058,
9.84271253358,
10.03274667658,
9.87447499658,
9.990537290579999,
10.11998293158,
9.88285142958,
9.84286022058,
9.86379295158,
9.83517465858
]
},
{
"command": "pnpr@HEAD",
"mean": 5.371782265979999,
"stddev": 0.1347312607048366,
"median": 5.33957084258,
"user": 2.4708283399999997,
"system": 2.9311884999999998,
"min": 5.29770737758,
"max": 5.74711162858,
"times": [
5.29770737758,
5.30151208058,
5.2995622425799995,
5.36685095758,
5.3616914175799995,
5.35957111558,
5.33785943758,
5.34128224758,
5.30467415458,
5.74711162858
]
},
{
"command": "pnpr@main",
"mean": 5.38954751808,
"stddev": 0.15386499620997462,
"median": 5.3138253210799995,
"user": 2.46342144,
"system": 2.9347705999999993,
"min": 5.28322410158,
"max": 5.72925972058,
"times": [
5.29480251358,
5.31782117558,
5.28322410158,
5.29962864458,
5.40878099658,
5.34117657958,
5.72925972058,
5.30316639358,
5.30982946658,
5.60778558858
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.63804429654,
"stddev": 0.01312273825995365,
"median": 0.63941892784,
"user": 0.36738256,
"system": 1.3018755599999998,
"min": 0.62032454634,
"max": 0.65740780734,
"times": [
0.63154657434,
0.65740780734,
0.65053347934,
0.6490301593400001,
0.63924052434,
0.6395973313400001,
0.6210490213400001,
0.62032454634,
0.62490735234,
0.64680616934
]
},
{
"command": "pacquet@main",
"mean": 0.64725917474,
"stddev": 0.021980649152157954,
"median": 0.64245435634,
"user": 0.36698966,
"system": 1.3100240600000002,
"min": 0.61824184234,
"max": 0.69120510634,
"times": [
0.61824184234,
0.6310236823400001,
0.64136744934,
0.63863072334,
0.69120510634,
0.67466185134,
0.6435412633400001,
0.62860940834,
0.65662751434,
0.64868290634
]
},
{
"command": "pnpr@HEAD",
"mean": 0.75722797764,
"stddev": 0.05401708593015897,
"median": 0.7410121483400001,
"user": 0.38395766,
"system": 1.3168432599999997,
"min": 0.72356286234,
"max": 0.90636753934,
"times": [
0.7332289723400001,
0.72356286234,
0.7413390643400001,
0.72869320534,
0.7366668013400001,
0.90636753934,
0.74687054734,
0.74263018134,
0.74068523234,
0.77223537034
]
},
{
"command": "pnpr@main",
"mean": 0.75643438694,
"stddev": 0.029131722306261976,
"median": 0.74579948334,
"user": 0.37631706,
"system": 1.32808996,
"min": 0.73465697634,
"max": 0.82869781434,
"times": [
0.74547729234,
0.74640262234,
0.7424947563400001,
0.73493778034,
0.74612167434,
0.82869781434,
0.78116426134,
0.73465697634,
0.7397316513400001,
0.76465904034
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 8.626537971659998,
"stddev": 0.06463269794798264,
"median": 8.623704886959999,
"user": 3.6306297,
"system": 3.3306958399999993,
"min": 8.54397012296,
"max": 8.74746609396,
"times": [
8.61092881496,
8.56215128296,
8.65877764596,
8.67620212196,
8.55619473996,
8.54397012296,
8.59780633396,
8.74746609396,
8.63648095896,
8.675401600959999
]
},
{
"command": "pacquet@main",
"mean": 9.09042914966,
"stddev": 0.02148900058282124,
"median": 9.08594112146,
"user": 3.5531267,
"system": 3.2954072399999994,
"min": 9.06391381496,
"max": 9.12299277996,
"times": [
9.07081705696,
9.06487900696,
9.08714738596,
9.06391381496,
9.10903610096,
9.12299277996,
9.080627334959999,
9.10307307596,
9.11707008296,
9.084734856959999
]
},
{
"command": "pnpr@HEAD",
"mean": 5.08720982206,
"stddev": 0.057525037338532536,
"median": 5.080348606459999,
"user": 2.3086305,
"system": 2.8134506399999997,
"min": 5.02871959496,
"max": 5.21919866696,
"times": [
5.02871959496,
5.09933843496,
5.21919866696,
5.10356472396,
5.04275320996,
5.1013018379599995,
5.05685536196,
5.12768062096,
5.03132699096,
5.06135877796
]
},
{
"command": "pnpr@main",
"mean": 5.08302569136,
"stddev": 0.04303854095786398,
"median": 5.07179957946,
"user": 2.2999202,
"system": 2.8346637400000003,
"min": 5.03451541896,
"max": 5.16428317596,
"times": [
5.07002000196,
5.06028523796,
5.11144107096,
5.04037138596,
5.16428317596,
5.03451541896,
5.05279955796,
5.14130928696,
5.07357915696,
5.08165261996
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.3820776252,
"stddev": 0.007671434481918599,
"median": 1.3852874867,
"user": 1.5279361199999997,
"system": 1.7606801,
"min": 1.3706358202,
"max": 1.3927707402,
"times": [
1.3872775502,
1.3706358202,
1.3855384592,
1.3720585622,
1.3850365142,
1.3876950062,
1.3753018312,
1.3927707402,
1.3768117522,
1.3876500162
]
},
{
"command": "pacquet@main",
"mean": 1.3733534395,
"stddev": 0.017280290771337716,
"median": 1.3760732772000002,
"user": 1.51642422,
"system": 1.7689315,
"min": 1.3443035722,
"max": 1.3966281142,
"times": [
1.3490424612,
1.3443035722,
1.3790856662,
1.3775876872000001,
1.3921898732,
1.3745588672,
1.3859717972,
1.3966281142,
1.3728251392000002,
1.3613412172000001
]
},
{
"command": "pnpr@HEAD",
"mean": 0.6714117344999999,
"stddev": 0.03934206726725377,
"median": 0.6569698612000001,
"user": 0.32727762,
"system": 1.2669557,
"min": 0.6412001032000001,
"max": 0.7682556322,
"times": [
0.7098454502,
0.6677060162,
0.7682556322,
0.6522660152,
0.6516063022,
0.6693286692,
0.6616737072000001,
0.6483829352,
0.6412001032000001,
0.6438525142
]
},
{
"command": "pnpr@main",
"mean": 0.6797266998,
"stddev": 0.05370529412526567,
"median": 0.6625209797,
"user": 0.33155552,
"system": 1.2685238,
"min": 0.6499310002,
"max": 0.8282276512000001,
"times": [
0.6964217242,
0.6555933052,
0.6638437422000001,
0.8282276512000001,
0.6559935762000001,
0.6558862302,
0.6657754222000001,
0.6499310002,
0.6643961292,
0.6611982172
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 4.84897838282,
"stddev": 0.018190180253305623,
"median": 4.8478441106200005,
"user": 1.7506044599999995,
"system": 1.97602276,
"min": 4.828228184119999,
"max": 4.87798855412,
"times": [
4.83170674112,
4.83594616312,
4.82968436912,
4.84941233412,
4.87798855412,
4.84627588712,
4.85542064812,
4.85962974112,
4.828228184119999,
4.8754912061199995
]
},
{
"command": "pacquet@main",
"mean": 4.86708958012,
"stddev": 0.021770223533961918,
"median": 4.86919556762,
"user": 1.7522661599999996,
"system": 1.9918952599999997,
"min": 4.82722180012,
"max": 4.90507251612,
"times": [
4.90507251612,
4.84782444112,
4.87617657812,
4.8584592851199995,
4.82722180012,
4.85422445012,
4.87656552312,
4.88341226812,
4.87972438212,
4.86221455712
]
},
{
"command": "pnpr@HEAD",
"mean": 0.66868711182,
"stddev": 0.01721045562253051,
"median": 0.6682149831199999,
"user": 0.32710165999999996,
"system": 1.26546166,
"min": 0.65016545512,
"max": 0.70588256712,
"times": [
0.70588256712,
0.68267372812,
0.65016545512,
0.65549087512,
0.67241895212,
0.65551642712,
0.67474340312,
0.66401101412,
0.67435310212,
0.65161559412
]
},
{
"command": "pnpr@main",
"mean": 0.6913218375200001,
"stddev": 0.07657594910365395,
"median": 0.6651158016200001,
"user": 0.32968105999999997,
"system": 1.2766769599999996,
"min": 0.65296042712,
"max": 0.9051864041200001,
"times": [
0.70662358112,
0.67064689112,
0.65296042712,
0.6615024271200001,
0.66318905912,
0.9051864041200001,
0.66704254412,
0.66791626812,
0.66080945612,
0.65734131712
]
}
]
} |
|
| Branch | pr/12309 |
| 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 | 8,626.54 ms(-5.49%)Baseline: 9,127.89 ms | 10,953.47 ms (78.76%) |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot 🚷 view threshold | 4,848.98 ms(-3.39%)Baseline: 5,019.03 ms | 6,022.84 ms (80.51%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,382.08 ms(-3.36%)Baseline: 1,430.11 ms | 1,716.13 ms (80.53%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 9,969.63 ms(-1.28%)Baseline: 10,099.07 ms | 12,118.89 ms (82.27%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 638.04 ms(-2.38%)Baseline: 653.57 ms | 784.29 ms (81.35%) |
|
| Branch | pr/12309 |
| 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 | 5,087.21 ms |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot | 668.69 ms |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot | 671.41 ms |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot | 5,371.78 ms |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot | 757.23 ms |
Code Review by Qodo
Context used 1. Concurrency=1 blocks metadata
|
PR Summary by Qodoperf(network): prioritize tarball downloads by estimated pipeline work WalkthroughsDescription• Add two-class priority scheduling to the shared network connection pool. • Thread dist stats end-to-end to rank tarball downloads by pipeline work. • Extend benchmark proxy with TCP slow-start modeling and fix macOS nonblocking sockets. Diagramgraph TD
A["pnpr server"] --> B["pnpr-client"] --> C["TarballPrefetcher"] --> D["Tarball downloader"] --> E["ThrottledClient"] --> F["PrioritySemaphore"]
M["Metadata fetch"] --> E --> R["Registry"]
E --> R
High-Level AssessmentThe following are alternative approaches to this PR: 1. Two separate semaphores (metadata vs downloads) with fixed partition
2. Weighted fair queue / DRR-style scheduler over all requests
3. Rely on upstream Tokio semaphore + external priority queue wrapper
Recommendation: The PR’s approach (custom, cancel-safe, class-aware PrioritySemaphore with a throughput reserve and FIFO latency queue) is a good tradeoff: it directly encodes the measured non-starvation requirement, remains work-conserving, and keeps the policy small enough to unit test thoroughly. The main review focus should be on permit release correctness under cancellation and on ensuring all metadata-gating paths consistently use UNPRIORITIZED. File ChangesEnhancement (17)
Refactor (2)
Tests (7)
Other (4)
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs (1)
1059-1111: 💤 Low valueWell-structured test coverage for dist stats collection.
The test correctly verifies that the sink is populated when both
unpackedSizeandfileCountare present in the registry response. Consider adding a test case for partial field presence (e.g., onlyunpackedSizepresent, or onlyfileCount), though the filter logic at line 1076 in the main file already handles this correctly.🤖 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 `@pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs` around lines 1059 - 1111, Add additional unit tests alongside binding_check_records_dist_stats_into_the_sink to cover partial dist stat presence: create two new async tests that use observed_dist_stats_sink(), default_opts(®istry) with opts.observed_dist_stats set, create a verifier via create_npm_resolution_verifier, and call verifier.verify with a TarballResolution (LockfileResolution::Tarball) where the packument includes only unpackedSize in one test and only fileCount in the other; assert that the sink records the corresponding Some(unpacked_size) or Some(file_count) while the other field is None, exercising the filter logic used around the existing check (see the filtering around line ~1076).
🤖 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.
Nitpick comments:
In
`@pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs`:
- Around line 1059-1111: Add additional unit tests alongside
binding_check_records_dist_stats_into_the_sink to cover partial dist stat
presence: create two new async tests that use observed_dist_stats_sink(),
default_opts(®istry) with opts.observed_dist_stats set, create a verifier via
create_npm_resolution_verifier, and call verifier.verify with a
TarballResolution (LockfileResolution::Tarball) where the packument includes
only unpackedSize in one test and only fileCount in the other; assert that the
sink records the corresponding Some(unpacked_size) or Some(file_count) while the
other field is None, exercising the filter logic used around the existing check
(see the filtering around line ~1076).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e10ee75c-8014-4985-bc16-068112378cb2
📒 Files selected for processing (30)
pacquet/crates/cli/src/cli_args/install.rspacquet/crates/env-installer/src/install_config_deps.rspacquet/crates/network/src/lib.rspacquet/crates/network/src/priority_semaphore.rspacquet/crates/network/src/priority_semaphore/tests.rspacquet/crates/package-manager/src/build_resolution_verifiers.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install_package_by_snapshot.rspacquet/crates/package-manager/src/install_package_from_registry.rspacquet/crates/package-manager/src/prefetching_resolver.rspacquet/crates/package-manager/src/resolution_observer.rspacquet/crates/package-manager/src/tarball_prefetch.rspacquet/crates/pnpr-client/src/lib.rspacquet/crates/pnpr-client/src/tests.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rspacquet/crates/resolving-npm-resolver/src/lib.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/tarball/src/lib.rspacquet/crates/tarball/src/tests.rspacquet/tasks/integrated-benchmark/src/cli_args.rspacquet/tasks/integrated-benchmark/src/latency_proxy.rspacquet/tasks/integrated-benchmark/src/latency_proxy/tests.rspacquet/tasks/integrated-benchmark/src/main.rspacquet/tasks/integrated-benchmark/src/work_env.rspacquet/tasks/micro-benchmark/src/main.rspnpr/crates/pnpr/src/resolver.rspnpr/crates/pnpr/src/resolver/tests.rspnpr/crates/pnpr/src/upstream.rspnpr/crates/pnpr/src/upstream/tests.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: Log emissions are part of matching pnpm — when porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plainStringor&strin Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too viaTryFrom<String>and/orFromStrand do not provide an infallible public constructor
If upstream TypeScript never validates a branded string, just brand for type-safety in Rust by exposing an infallibleFrom<String>constructor
If upstream TypeScript occasionally constructs a branded string without validation, exposefrom_str_uncheckedin Rust as an escape hatch alongside the validating constructor
Match upstream serde behavior for branded strings crossing JSON, YAML, or INI boundaries by using#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization
Derive simple conversions for branded strings using#[derive(derive_more::From)]and#[derive(derive_more::Into)]instead of handwritingimplblocks; use manualimplonly when conversion needs custom logic
Model TypeScript string literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers, since the set of valid values is closed
Treat TypeScript string template literal types (like`${string}@${string}`) the same as branded string types in Rust, using a newtype wrapper with validation
Follow the code style guide inCODE_STYLE_GUIDE.md— imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Choose owned vs. borrowed parameters to minimize copies; widen to t...
Files:
pacquet/crates/env-installer/src/install_config_deps.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/package-manager/src/install.rspacquet/crates/resolving-npm-resolver/src/lib.rspacquet/crates/cli/src/cli_args/install.rspacquet/tasks/micro-benchmark/src/main.rspacquet/crates/package-manager/src/resolution_observer.rspacquet/crates/pnpr-client/src/tests.rspacquet/crates/package-manager/src/build_resolution_verifiers.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rspacquet/tasks/integrated-benchmark/src/cli_args.rspacquet/tasks/integrated-benchmark/src/main.rspacquet/crates/package-manager/src/prefetching_resolver.rspacquet/crates/network/src/priority_semaphore/tests.rspacquet/crates/pnpr-client/src/lib.rspacquet/crates/package-manager/src/install_package_by_snapshot.rspacquet/tasks/integrated-benchmark/src/latency_proxy.rspacquet/crates/package-manager/src/install_package_from_registry.rspacquet/tasks/integrated-benchmark/src/latency_proxy/tests.rspacquet/crates/package-manager/src/tarball_prefetch.rspacquet/crates/network/src/priority_semaphore.rspacquet/crates/tarball/src/lib.rspacquet/tasks/integrated-benchmark/src/work_env.rspacquet/crates/network/src/lib.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rspacquet/crates/tarball/src/tests.rs
pnpr/**/pnpr/**/*.rs
📄 CodeRabbit inference engine (pnpr/AGENTS.md)
pnpr/**/pnpr/**/*.rs: Follow the pacquet code-style guide (../pacquet/CODE_STYLE_GUIDE.md) for Rust-level conventions including imports, naming, ownership, and error handling
Follow the pacquet contributing guide (../pacquet/CONTRIBUTING.md) for test layout and Rust conventions
Files:
pnpr/crates/pnpr/src/upstream/tests.rspnpr/crates/pnpr/src/resolver/tests.rspnpr/crates/pnpr/src/resolver.rspnpr/crates/pnpr/src/upstream.rs
🧠 Learnings (4)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.
Applied to files:
pacquet/crates/env-installer/src/install_config_deps.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/package-manager/src/install.rspacquet/crates/resolving-npm-resolver/src/lib.rspacquet/crates/cli/src/cli_args/install.rspacquet/crates/package-manager/src/resolution_observer.rspacquet/crates/pnpr-client/src/tests.rspacquet/crates/package-manager/src/build_resolution_verifiers.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rspacquet/crates/package-manager/src/prefetching_resolver.rspacquet/crates/network/src/priority_semaphore/tests.rspacquet/crates/pnpr-client/src/lib.rspacquet/crates/package-manager/src/install_package_by_snapshot.rspacquet/crates/package-manager/src/install_package_from_registry.rspacquet/crates/package-manager/src/tarball_prefetch.rspacquet/crates/network/src/priority_semaphore.rspacquet/crates/tarball/src/lib.rspacquet/crates/network/src/lib.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rspacquet/crates/tarball/src/tests.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/env-installer/src/install_config_deps.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/package-manager/src/install.rspacquet/crates/resolving-npm-resolver/src/lib.rspacquet/crates/cli/src/cli_args/install.rspacquet/crates/package-manager/src/resolution_observer.rspacquet/crates/pnpr-client/src/tests.rspacquet/crates/package-manager/src/build_resolution_verifiers.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rspacquet/crates/package-manager/src/prefetching_resolver.rspacquet/crates/network/src/priority_semaphore/tests.rspacquet/crates/pnpr-client/src/lib.rspacquet/crates/package-manager/src/install_package_by_snapshot.rspacquet/crates/package-manager/src/install_package_from_registry.rspacquet/crates/package-manager/src/tarball_prefetch.rspacquet/crates/network/src/priority_semaphore.rspacquet/crates/tarball/src/lib.rspacquet/crates/network/src/lib.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rspacquet/crates/tarball/src/tests.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.
Applied to files:
pacquet/crates/env-installer/src/install_config_deps.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/package-manager/src/install.rspacquet/crates/resolving-npm-resolver/src/lib.rspacquet/crates/cli/src/cli_args/install.rspacquet/tasks/micro-benchmark/src/main.rspacquet/crates/package-manager/src/resolution_observer.rspacquet/crates/pnpr-client/src/tests.rspacquet/crates/package-manager/src/build_resolution_verifiers.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rspacquet/tasks/integrated-benchmark/src/cli_args.rspacquet/tasks/integrated-benchmark/src/main.rspacquet/crates/package-manager/src/prefetching_resolver.rspacquet/crates/network/src/priority_semaphore/tests.rspacquet/crates/pnpr-client/src/lib.rspacquet/crates/package-manager/src/install_package_by_snapshot.rspacquet/tasks/integrated-benchmark/src/latency_proxy.rspacquet/crates/package-manager/src/install_package_from_registry.rspacquet/tasks/integrated-benchmark/src/latency_proxy/tests.rspacquet/crates/package-manager/src/tarball_prefetch.rspacquet/crates/network/src/priority_semaphore.rspacquet/crates/tarball/src/lib.rspacquet/tasks/integrated-benchmark/src/work_env.rspacquet/crates/network/src/lib.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rspacquet/crates/tarball/src/tests.rs
📚 Learning: 2026-06-06T18:58:37.156Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12243
File: pacquet/crates/package-manager/src/install_package_by_snapshot.rs:319-322
Timestamp: 2026-06-06T18:58:37.156Z
Learning: When reviewing Rust code, do not assume `matches!(expr, Pattern(_))` will move out of `expr` if `Pattern(_)` contains no by-value bindings. `matches!` desugars to a `match` that auto-borrows the scrutinee for discrimination, so even if `expr` is a non-`Copy` value behind a shared reference (e.g., `&T`), the macro should not move-out of the borrowed data purely due to `matches!`. Treat `matches!(&expr, Pattern(_))` as a readability/clarity improvement, not a correctness requirement. Only flag potential move-out-of-borrow risks when the pattern includes by-value bindings that would require moving the matched value.
Applied to files:
pacquet/crates/env-installer/src/install_config_deps.rspnpr/crates/pnpr/src/upstream/tests.rspacquet/crates/resolving-npm-resolver/src/lookup_context.rspacquet/crates/package-manager/src/install.rspacquet/crates/resolving-npm-resolver/src/lib.rspacquet/crates/cli/src/cli_args/install.rspacquet/tasks/micro-benchmark/src/main.rspnpr/crates/pnpr/src/resolver/tests.rspacquet/crates/package-manager/src/resolution_observer.rspacquet/crates/pnpr-client/src/tests.rspacquet/crates/package-manager/src/build_resolution_verifiers.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rspacquet/tasks/integrated-benchmark/src/cli_args.rspacquet/tasks/integrated-benchmark/src/main.rspacquet/crates/package-manager/src/prefetching_resolver.rspacquet/crates/network/src/priority_semaphore/tests.rspacquet/crates/pnpr-client/src/lib.rspacquet/crates/package-manager/src/install_package_by_snapshot.rspacquet/tasks/integrated-benchmark/src/latency_proxy.rspacquet/crates/package-manager/src/install_package_from_registry.rspacquet/tasks/integrated-benchmark/src/latency_proxy/tests.rspacquet/crates/package-manager/src/tarball_prefetch.rspacquet/crates/network/src/priority_semaphore.rspacquet/crates/tarball/src/lib.rspnpr/crates/pnpr/src/resolver.rspnpr/crates/pnpr/src/upstream.rspacquet/tasks/integrated-benchmark/src/work_env.rspacquet/crates/network/src/lib.rspacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rspacquet/crates/tarball/src/tests.rs
🔇 Additional comments (68)
pacquet/tasks/integrated-benchmark/src/cli_args.rs (1)
86-94: LGTM!pacquet/tasks/integrated-benchmark/src/work_env.rs (2)
71-71: LGTM!
553-553: LGTM!Also applies to: 654-654, 682-682
pacquet/tasks/integrated-benchmark/src/main.rs (1)
36-36: LGTM!Also applies to: 106-106, 174-174
pacquet/tasks/integrated-benchmark/src/latency_proxy.rs (4)
46-54: LGTM!
140-146: LGTM!
189-203: LGTM!
219-258: LGTM!pacquet/tasks/integrated-benchmark/src/latency_proxy/tests.rs (2)
25-25: LGTM!Also applies to: 60-60, 100-100
127-177: LGTM!pacquet/crates/network/src/lib.rs (3)
1-156: LGTM!
157-378: LGTM!
380-618: LGTM!pacquet/crates/network/src/priority_semaphore.rs (1)
1-238: LGTM!pacquet/crates/network/src/priority_semaphore/tests.rs (1)
1-148: LGTM!pnpr/crates/pnpr/src/upstream/tests.rs (1)
273-348: LGTM!pacquet/crates/tarball/src/lib.rs (7)
14-14: LGTM!
1317-1322: LGTM!
1804-1822: LGTM!
1508-1537: LGTM!
1834-1862: LGTM!
2099-2231: LGTM!
2289-2325: LGTM!pacquet/crates/tarball/src/tests.rs (1)
172-172: LGTM!Also applies to: 236-236, 272-272, 351-351, 422-422, 653-653, 716-716, 782-782, 858-858, 1138-1138, 1188-1188, 1222-1222, 1265-1265, 1302-1302, 1426-1426, 1500-1500, 1547-1547, 1603-1603, 1679-1679, 1708-1708, 1804-1804, 1842-1842, 1926-1926, 2031-2031, 2123-2123, 2214-2214, 2254-2254, 2336-2336, 2649-2649, 2721-2721
pacquet/tasks/micro-benchmark/src/main.rs (1)
48-48: LGTM!pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs (6)
51-73: LGTM!
142-145: LGTM!
175-175: LGTM!
256-256: LGTM!
454-459: LGTM!
1068-1083: LGTM!pacquet/crates/resolving-npm-resolver/src/lookup_context.rs (1)
48-56: LGTM!pacquet/crates/resolving-npm-resolver/src/lib.rs (1)
41-42: LGTM!pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs (2)
12-12: LGTM!
58-58: LGTM!pnpr/crates/pnpr/src/upstream.rs (1)
344-349: LGTM!pacquet/crates/pnpr-client/src/tests.rs (2)
39-52: LGTM!
54-66: LGTM!pacquet/crates/package-manager/src/build_resolution_verifiers.rs (4)
28-29: LGTM!
72-74: LGTM!
80-80: LGTM!
134-134: LGTM!pacquet/crates/package-manager/src/install.rs (1)
649-649: LGTM!pacquet/crates/package-manager/src/resolution_observer.rs (3)
12-14: LGTM!
37-45: LGTM!
98-99: LGTM!pacquet/crates/package-manager/src/install_package_by_snapshot.rs (3)
305-305: LGTM!
582-591: LGTM!
778-778: LGTM!pnpr/crates/pnpr/src/resolver.rs (7)
58-66: LGTM!
231-242: LGTM!
252-257: LGTM!
391-417: LGTM!
432-476: LGTM!
529-538: LGTM!
570-625: LGTM!pnpr/crates/pnpr/src/resolver/tests.rs (2)
64-94: LGTM!
96-136: LGTM!pacquet/crates/pnpr-client/src/lib.rs (3)
114-122: LGTM!
320-323: LGTM!
266-283: LGTM!pacquet/crates/package-manager/src/install_package_from_registry.rs (2)
150-150: LGTM!Also applies to: 176-176
277-292: LGTM!pacquet/crates/package-manager/src/prefetching_resolver.rs (1)
27-27: LGTM!Also applies to: 201-201, 237-237
pacquet/crates/package-manager/src/tarball_prefetch.rs (2)
50-50: LGTM!Also applies to: 78-78, 91-91
196-207: LGTM!Also applies to: 238-239
pacquet/crates/cli/src/cli_args/install.rs (1)
547-553: LGTM!pacquet/crates/env-installer/src/install_config_deps.rs (1)
186-186: LGTM!
When the connection pool saturates, freed slots now go to the highest-priority waiter instead of FIFO. Tarball downloads pass their unpackedSize as the priority, so the largest pending archives claim slots first and a multi-megabyte tarball can no longer start last and run alone at single-connection throughput while every other slot sits idle (longest-processing-time-first scheduling; see the per-connection bandwidth measurements in #12230). Requests acquired without an explicit priority — packument and other metadata fetches that gate resolution progress — outrank every sized download and stay FIFO among themselves. The size is dist.unpackedSize, threaded in per path: - Local fresh installs already carried it on the resolver result; the download path now uses it as the queueing priority. - The pnpr server ships it in each streamed package frame as an optional unpackedSize field (omitted when the registry never published one; old clients and old servers interoperate unchanged), and the client threads it into the prefetcher — which also gives the prefetch path exact decompression-buffer preallocation. - Frozen restores through pnpr: the lockfile records no sizes, but the server's lockfile verification fan-out fetches each entry's metadata anyway. The npm verifier now records dist.unpackedSize into an optional observed-sizes sink as a side product of the tarball-URL binding check, and the frozen fast path announces every verified tarball as a sized package frame before the done frame. On a whole-lockfile verdict-cache hit no metadata is fetched, so that path keeps the bare done frame. Plain frozen installs without pnpr are unchanged: adding sizes to pnpm-lock.yaml would be a lockfile-format change.
A package's install cost is a two-stage pipeline job — download, then extract (gunzip + SHA-512 + per-file CAS writes) — and the install's fetch phase ends when the last extraction finishes. Ranking the queue by unpackedSize alone lets a many-small-files package slip to the tail, where its extraction extends the makespan past the last byte on the wire. The priority is now the estimated total pipeline work: unpackedSize + 3000 x fileCount, the per-file term being the fixed CAS-write/hash overhead expressed in byte-equivalents. dist.fileCount rides everywhere unpackedSize already did: the local fresh path reads it off the resolver manifest, the pnpr package frame carries it as an optional fileCount field, and the verifier's observed-stats sink (now a DistStats struct) records it during the frozen fast path's verification fan-out. pnpr's abbreviated metadata keeps unpackedSize and fileCount instead of stripping them, since pacquet now reads both for scheduling and preallocation. Resolve-time tarball fetches (a tarball dep's manifest comes from its archive) acquire UNPRIORITIZED like packument fetches: they gate the resolver's walk and must not queue behind sized downloads.
The latency proxy transmitted at the full bandwidth cap from a connection's first byte, overstating real-TCP throughput for small and mid-size tarballs: a transfer under ~10 windows never gets near the link's capacity. With --registry-slow-start, each connection ramps from a ~14.6 KB initial window (RFC 6928) toward the cap, doubling per delivered window, so scheduling effects that depend on per-connection ramp-up become measurable. Requires both --registry-latency-ms and --registry-bandwidth-mbps.
…tadata-first Strictly preferring metadata over sized downloads serialized cold fresh installs: during the resolution burst no tarball ever won a slot, so the download pipeline started only after resolution finished, costing the whole resolve/fetch overlap (measured 2x wall time on a cold loopback install, and multi-second stalls against the real registry). The semaphore now grants by a two-class policy: latency work (metadata, FIFO) and throughput work (downloads, ranked by estimated pipeline work) share the pool, with downloads guaranteed a reserved half so the largest pending archives keep flowing during resolution — and metadata preferred beyond that reserve so a download backlog cannot starve resolution either. Both directions stay work-conserving.
The latency proxy's accept listener is non-blocking, and BSD-derived platforms (macOS) hand accepted sockets the listener's O_NONBLOCK flag, so the pump's blocking reads surfaced spurious WouldBlock errors and dropped every proxied connection before its first byte — clients saw an empty reply on each request through the shaped link. Reset the accepted socket to blocking before pumping.
Thread package_file_count and the download-priority argument through the tarball test fixtures, and assert pnpr's abbreviated metadata now keeps unpackedSize and fileCount (pacquet reads both for decompression preallocation and download scheduling).
Satisfy perfectionist derive_ordering on DistStats and add the new package_file_count field to the micro-benchmark's download literal.
Review findings on #12309: - A single-permit pool computed a throughput reserve equal to the whole pool, so queued downloads blocked metadata outright under networkConcurrency=1 — the inverse of the starvation guarantee. The reserve is now capped at permits-1. - download_priority's saturating arithmetic could reach u64::MAX on absurd registry-published dist stats, colliding with the UNPRIORITIZED latency-class sentinel and reclassifying the download as metadata. Computed priorities now clamp to UNPRIORITIZED - 1.
|
Code review by qodo was updated up to the latest commit 8144b56 |
|
Both review findings are addressed in 8144b56:
Written by an agent (Claude Code, claude-fable-5). |
must_use on the new pub fns (download_priority, observed_dist_stats_sink) and on the ThrottledClient-taking constructors that the pedantic must_use_candidate heuristic now reaches — replacing the tokio semaphore with the Arc-backed priority semaphore made ThrottledClient freeze, which flips the lint's interior-mutability check for downstream signatures. ndjson_frames takes a slice per needless_pass_by_value.
|
Code review by qodo was updated up to the latest commit f2aaee0 |
…erifier API The rebase onto main picked up #12309, which threads an ObservedDistStats sink through build_resolution_verifiers and verify_input_lockfile and adds size-hint parameters to TarballPrefetcher::prefetch. Pass an empty sink where the overlap paths have no use for the stats, and prefetch lockfile tarballs without a work estimate (the lockfile records no dist sizes).
Summary
When the download connection pool saturates, freed slots are granted by a two-class scheduling policy instead of FIFO:
unpackedSize + 3000 × fileCount— so the most expensive download+extract jobs start first (longest-processing-time-first; a large archive that starts last runs alone at single-connection throughput while every other slot idles, see pnpm/pnpm#12230). The per-file term prices the fixed CAS-write/hash overhead, so a many-small-files package ranks as the long job it actually is.How the size hints travel
dist.unpackedSize/dist.fileCountoff the resolver-fetched manifest (also fixes exact decompression-buffer preallocation on the prefetch path, previously hardcodedNone)./v1/resolvepackageframe carries both as optionalunpackedSize/fileCountfields (omitted when the registry never published them; old clients and servers interoperate unchanged).ObservedDistStatssink as a side product of the tarball-URL binding check, and the frozen fast path announces every verified tarball as a sizedpackageframe beforedone(URLs derived by the sametarball_url_and_integritythe client materialization uses). Verdict-cache hits fetch no metadata and keep the baredoneframe.unpackedSize/fileCountinstead of stripping them, since pacquet reads both.Benchmark tooling
--registry-slow-start: per-connection TCP slow start (RFC 6928 initial window, doubling per delivered window toward the bandwidth cap), so scheduling effects that depend on per-connection ramp-up are measurable.O_NONBLOCKand every proxied connection died on its first read — all shaped benchmark traffic silently failed before this.Measurements
Fixture: ~110 direct deps / 1308 packages (~90 MB wire),
isolated-linker.fresh-install.cold-cache.cold-store, local mirror of real npm behind the shaped proxy (30 ms RTT, 80 Mbit/s per-connection cap, TCP slow start).Drift-controlled interleaved comparison (4 alternating blocks x 4 runs each; sequential multi-target sessions on this machine showed up to +75% session-order drift, so block-paired ABAB is the only design we trust):
The PR wins all 4 paired blocks (-0.18 s to -0.50 s, mean -0.30 s, ~2%). A scheduler ablation (reserve+FIFO, smallest-first, unpackedSize-only, work with K=3k and K=10k per file) ordered as the pipeline model predicts, but the per-variant deltas sit inside the session-drift noise, so only the FIFO-vs-full-design pairing is claimed. K in [3000, 10000] is indistinguishable.
The starvation fix is the load-bearing piece, established mechanistically rather than by wall clock: with strict metadata-first priority (an intermediate design), cold-install event timelines showed 4-7 s windows at install start with zero tarball activity - downloads never won a slot during the resolution burst, serializing the resolve and fetch phases. The reserved share removes those gaps entirely and the worst observed cold-install runs with it are within ~1 s of the median, where unreserved variants showed multi-second stragglers.
Real-registry A/B (15 randomized cold-install pairs against npmjs) is noise-bound on a saturated ~100 Mbit link (+/-3 s registry variance), median -0.17 s in this PR's favor - consistent with "never slower."
Verification
cargo nextest runacrosspacquet-network(incl. new reserved-share, priority-ordering, FIFO tie-break, cancelled-waiter tests),pacquet-tarball,pacquet-package-manager,pacquet-resolving-npm-resolver(new sink test),pnpr(frame emission + frozen-frames tests),pacquet-pnpr-client(frame parse tests),pacquet-integrated-benchmark(new slow-start ramp test) — 1060 + 54 tests passingcargo clippy --all-targets -- -D warnings,cargo dylint --all,cargo fmt,RUSTDOCFLAGS="-D warnings" cargo docclean; pre-push hook passedWritten by an agent (Claude Code, claude-fable-5).
Summary by CodeRabbit
New Features
Improvements
Tests