Skip to content

perf(network): schedule tarball downloads by estimated pipeline work#12309

Merged
zkochan merged 12 commits into
mainfrom
pnpr-perf-opt
Jun 11, 2026
Merged

perf(network): schedule tarball downloads by estimated pipeline work#12309
zkochan merged 12 commits into
mainfrom
pnpr-perf-opt

Conversation

@zkochan

@zkochan zkochan commented Jun 10, 2026

Copy link
Copy Markdown
Member

Summary

When the download connection pool saturates, freed slots are granted by a two-class scheduling policy instead of FIFO:

  • Latency class (packument/metadata fetches, which gate resolution progress): served FIFO.
  • Throughput class (tarball downloads): ranked by estimated total pipeline workunpackedSize + 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.
  • Neither class can starve the other: downloads are guaranteed a reserved half of the pool (strict metadata-first was measured to serialize cold installs — no tarball got a slot until resolution drained, costing the whole resolve/fetch overlap), and metadata wins beyond that reserve (a download backlog can't stall resolution). Both directions are work-conserving.

How the size hints travel

  • Local fresh installs read dist.unpackedSize / dist.fileCount off the resolver-fetched manifest (also fixes exact decompression-buffer preallocation on the prefetch path, previously hardcoded None).
  • The pnpr /v1/resolve package frame carries both as optional unpackedSize / fileCount fields (omitted when the registry never published them; old clients and servers interoperate unchanged).
  • pnpr frozen restores: the lockfile records no sizes, but the verification fan-out fetches each entry's metadata anyway — the npm verifier records both stats into an optional ObservedDistStats 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 done (URLs derived by the same tarball_url_and_integrity the client materialization uses). Verdict-cache hits fetch no metadata and keep the bare done frame.
  • pnpr's abbreviated metadata now preserves unpackedSize/fileCount instead of stripping them, since pacquet reads both.
  • Resolve-time tarball fetches (tarball deps' manifests come from their archives) acquire in the latency class — they gate the resolver's walk.

Benchmark tooling

  • The integrated benchmark's latency proxy gained --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.
  • Fixed a macOS bug where the proxy's accepted sockets inherited the listener's O_NONBLOCK and 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):

target mean +/- sd (n=16)
baseline FIFO 14.36 s +/- 0.54
this PR 14.06 s +/- 0.70

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 run across pacquet-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 passing
  • cargo clippy --all-targets -- -D warnings, cargo dylint --all, cargo fmt, RUSTDOCFLAGS="-D warnings" cargo doc clean; pre-push hook passed

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

Summary by CodeRabbit

  • New Features

    • Priority-based network queuing so large downloads don’t block metadata/latency work.
    • Tarball downloads and resolver streaming now include unpacked-size and file-count hints.
    • Frozen-lockfile streaming emits package frames with dist metadata.
    • TCP slow-start simulation added to integrated benchmarks.
  • Improvements

    • Resolver/registry and verifiers collect and emit per-package dist statistics for better planning.
    • Prefetch/download scheduling uses these hints for smarter queuing.
  • Tests

    • New tests for priority scheduling, cancellation behavior, and dist-stat instrumentation.

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 228198fe-070e-4abc-b67c-1f9a52ba4747

📥 Commits

Reviewing files that changed from the base of the PR and between 8144b56 and f2aaee0.

📒 Files selected for processing (5)
  • pacquet/crates/engine-runtime-node-resolver/src/node_resolver.rs
  • pacquet/crates/resolving-git-resolver/src/runners.rs
  • pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs
  • pacquet/crates/tarball/src/lib.rs
  • pnpr/crates/pnpr/src/resolver.rs
✅ Files skipped from review due to trivial changes (1)
  • pacquet/crates/resolving-git-resolver/src/runners.rs
🚧 Files skipped from review as they are similar to previous changes (3)
  • pnpr/crates/pnpr/src/resolver.rs
  • pacquet/crates/tarball/src/lib.rs
  • pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs
📜 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)
  • GitHub Check: Code Coverage
  • GitHub Check: Lint and Test (windows-latest)
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Dylint
  • GitHub Check: Lint and Test (macos-latest)
  • GitHub Check: Lint and Test (ubuntu-latest)
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: Log emissions are part of matching pnpm — when porting a function that fires pnpm:<channel> events through globalLogger, logger.debug(...), or streamParser.write(...), mirror the call site, payload, and ordering so @pnpm/cli.default-reporter parses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plain String or &str in Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too via TryFrom<String> and/or FromStr and 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 infallible From<String> constructor
If upstream TypeScript occasionally constructs a branded string without validation, expose from_str_unchecked in 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 handwriting impl blocks; use manual impl only when conversion needs custom logic
Model TypeScript string literal unions (like 'auto' | 'always' | 'never') as Rust enums 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 in CODE_STYLE_GUIDE.md — imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching, pipe-trait, error handling, test layout, and cloning of Arc and Rc
Choose owned vs. borrowed parameters to minimize copies; widen to t...

Files:

  • pacquet/crates/engine-runtime-node-resolver/src/node_resolver.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/engine-runtime-node-resolver/src/node_resolver.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/engine-runtime-node-resolver/src/node_resolver.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/engine-runtime-node-resolver/src/node_resolver.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/engine-runtime-node-resolver/src/node_resolver.rs
🔇 Additional comments (1)
pacquet/crates/engine-runtime-node-resolver/src/node_resolver.rs (1)

99-102: LGTM!


📝 Walkthrough

Walkthrough

This 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.

Changes

Download Prioritization and Metadata Propagation

Layer / File(s) Summary
Priority semaphore foundation
pacquet/crates/network/src/{lib.rs, priority_semaphore.rs, priority_semaphore/tests.rs}
Replaces tokio semaphore with a two-class PrioritySemaphore, updates ThrottledClient to use it, and adds scheduling and cancellation tests.
Metadata collection and verifier plumbing
pacquet/crates/resolving-npm-resolver/src/{create_npm_resolution_verifier.rs, lookup_context.rs, lib.rs}, pacquet/crates/package-manager/src/build_resolution_verifiers.rs
Adds DistStats and ObservedDistStats sink, exposes a sink constructor, projects per-version dist stats into abbreviated metadata, and wires the sink into verifier options.
Resolver and pnpr frame changes
pnpr/crates/pnpr/src/{resolver.rs, upstream.rs, resolver/tests.rs, upstream/tests.rs}, pacquet/crates/pnpr-client/src/{lib.rs, tests.rs}
pnpr now emits enriched package NDJSON frames with optional unpackedSize/fileCount; client parsing accepts those fields and forwards them into ResolvedPackage; frozen-fast-path emits per-tarball frames when lockfile verification yields observed stats.
Prefetch and install metadata threading
pacquet/crates/package-manager/src/{resolution_observer.rs, prefetching_resolver.rs, tarball_prefetch.rs, install_package_from_registry.rs, install_package_by_snapshot.rs}, pacquet/crates/cli/src/cli_args/install.rs, pacquet/crates/env-installer/src/install_config_deps.rs
Extracts dist.fileCount/dist.unpackedSize from manifests, extends ResolvedPackageHint and TarballPrefetcher::prefetch to accept them, and threads package_file_count through DownloadTarballToStore at multiple spawn sites.
Tarball download priority and execution
pacquet/crates/tarball/src/{lib.rs, tests.rs}, pacquet/tasks/micro-benchmark/src/main.rs
Adds download_priority() (bytes + per-file overhead), passes priority into ThrottledClient::acquire_for_url_with_priority through retry boundaries, introduces DownloadTarballToStore.package_file_count, and updates tests/bench to match new call shapes.
Benchmark TCP slow-start modeling
pacquet/tasks/integrated-benchmark/src/{cli_args.rs, latency_proxy.rs, latency_proxy/tests.rs, main.rs, work_env.rs}
Adds --registry-slow-start flag, LinkProfile.slow_start, a per-connection congestion-window slow-start model and tests, and threads the flag through work env wiring.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

Possibly related PRs

  • pnpm/pnpm#12237: Related changes to pnpr streaming install callback and prefetcher invocation.
  • pnpm/pnpm#12163: Related upstream packument/trim_dist_fields edits affecting fileCount/unpackedSize.
  • pnpm/pnpm#12251: Also touches the pnpr streaming install prefetch call site.

"A rabbit builds a semaphore so neat,
Size and files guide which fetch gets a seat;
Frames carry hints, verifiers keep score,
Prefetch queues smarter than before;
Slow-start hums and benchmarks beat. 🐇"

🚥 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 accurately describes the main change: implementing priority-based scheduling for tarball downloads based on estimated pipeline work, which is the core feature across all modified files.
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 docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pnpr-perf-opt

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.

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.00      7.6±0.18ms   572.6 KB/sec    1.01      7.7±0.39ms   566.9 KB/sec

@codecov-commenter

codecov-commenter commented Jun 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.04594% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.70%. Comparing base (ac367fc) to head (f2aaee0).

Files with missing lines Patch % Lines
pnpr/crates/pnpr/src/resolver.rs 78.20% 17 Missing ⚠️
pacquet/crates/network/src/priority_semaphore.rs 87.37% 13 Missing ⚠️
...package-manager/src/install_package_by_snapshot.rs 66.66% 1 Missing ⚠️
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.
📢 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

github-actions Bot commented Jun 10, 2026

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 9.970 ± 0.155 9.843 10.339 1.86 ± 0.05
pacquet@main 9.911 ± 0.101 9.825 10.120 1.85 ± 0.05
pnpr@HEAD 5.372 ± 0.135 5.298 5.747 1.00
pnpr@main 5.390 ± 0.154 5.283 5.729 1.00 ± 0.04
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

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 638.0 ± 13.1 620.3 657.4 1.00
pacquet@main 647.3 ± 22.0 618.2 691.2 1.01 ± 0.04
pnpr@HEAD 757.2 ± 54.0 723.6 906.4 1.19 ± 0.09
pnpr@main 756.4 ± 29.1 734.7 828.7 1.19 ± 0.05
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

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 8.627 ± 0.065 8.544 8.747 1.70 ± 0.02
pacquet@main 9.090 ± 0.021 9.064 9.123 1.79 ± 0.02
pnpr@HEAD 5.087 ± 0.058 5.029 5.219 1.00 ± 0.01
pnpr@main 5.083 ± 0.043 5.035 5.164 1.00
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

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 1.382 ± 0.008 1.371 1.393 2.06 ± 0.12
pacquet@main 1.373 ± 0.017 1.344 1.397 2.05 ± 0.12
pnpr@HEAD 0.671 ± 0.039 0.641 0.768 1.00
pnpr@main 0.680 ± 0.054 0.650 0.828 1.01 ± 0.10
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

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 4.849 ± 0.018 4.828 4.878 7.25 ± 0.19
pacquet@main 4.867 ± 0.022 4.827 4.905 7.28 ± 0.19
pnpr@HEAD 0.669 ± 0.017 0.650 0.706 1.00
pnpr@main 0.691 ± 0.077 0.653 0.905 1.03 ± 0.12
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
      ]
    }
  ]
}

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Branchpr/12309
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
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%)
🐰 View full continuous benchmarking report in Bencher

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Branchpr/12309
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
5,087.21 ms
isolated-linker.fresh-install.cold-cache.hot-store📈 view plot
⚠️ NO THRESHOLD
668.69 ms
isolated-linker.fresh-install.hot-cache.hot-store📈 view plot
⚠️ NO THRESHOLD
671.41 ms
isolated-linker.fresh-restore.cold-cache.cold-store📈 view plot
⚠️ NO THRESHOLD
5,371.78 ms
isolated-linker.fresh-restore.hot-cache.hot-store📈 view plot
⚠️ NO THRESHOLD
757.23 ms
🐰 View full continuous benchmarking report in Bencher

@zkochan zkochan changed the title perf(network): start the largest pending tarball downloads first perf(network): schedule tarball downloads by estimated pipeline work Jun 10, 2026
@zkochan zkochan marked this pull request as ready for review June 10, 2026 22:32
@qodo-free-for-open-source-projects

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

Copy link
Copy Markdown

Code Review by Qodo

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

Context used

Grey Divider


Action required

1. Concurrency=1 blocks metadata 🐞 Bug ≡ Correctness
Description
With network_concurrency = 1, throughput_reserve becomes 1 (permits.div_ceil(2)), and
next_waiter() will always select a queued throughput waiter on every release when any downloads
are queued. This forces queued metadata requests to wait until the download queue drains, delaying
resolution progress under a supported configuration.
Code

pacquet/crates/network/src/priority_semaphore.rs[R120-192]

+    pub(crate) fn new(permits: usize) -> Self {
+        PrioritySemaphore {
+            state: Arc::new(Mutex::new(SemState {
+                free: permits,
+                latency_in_flight: 0,
+                throughput_in_flight: 0,
+                throughput_reserve: permits.div_ceil(2),
+                next_seq: 0,
+                latency_waiters: VecDeque::new(),
+                throughput_waiters: BinaryHeap::new(),
+            })),
+        }
+    }
+
+    /// Wait for a permit. Free permits are claimed immediately; when
+    /// the pool is saturated the caller queues in its class and is
+    /// woken per the grant policy in the module docs.
+    pub(crate) async fn acquire(&self, priority: u64) -> Permit {
+        let class = Class::of(priority);
+        let rx = {
+            let mut state = self.state.lock().expect("priority semaphore lock poisoned");
+            if state.free > 0 {
+                state.free -= 1;
+                *state.count_mut(class) += 1;
+                return Permit { state: Arc::clone(&self.state), class, armed: true };
+            }
+            let (tx, rx) = oneshot::channel();
+            let seq = state.next_seq;
+            state.next_seq += 1;
+            let waiter = Waiter { priority, seq, tx };
+            match class {
+                Class::Latency => state.latency_waiters.push_back(waiter),
+                Class::Throughput => state.throughput_waiters.push(waiter),
+            }
+            rx
+        };
+        rx.await.expect("priority semaphore state dropped while a permit was awaited")
+    }
+
+    #[cfg(test)]
+    pub(crate) fn queued_waiters(&self) -> usize {
+        let state = self.state.lock().expect("priority semaphore lock poisoned");
+        state.latency_waiters.len() + state.throughput_waiters.len()
+    }
+
+    #[cfg(test)]
+    pub(crate) fn available_permits(&self) -> usize {
+        self.state.lock().expect("priority semaphore lock poisoned").free
+    }
+}
+
+impl SemState {
+    fn count_mut(&mut self, class: Class) -> &mut usize {
+        match class {
+            Class::Latency => &mut self.latency_in_flight,
+            Class::Throughput => &mut self.throughput_in_flight,
+        }
+    }
+
+    /// Pop the waiter the grant policy picks next, or `None` when both
+    /// queues are empty: throughput work below its reserve goes first,
+    /// then queued latency work, then throughput work above the
+    /// reserve.
+    fn next_waiter(&mut self) -> Option<(Waiter, Class)> {
+        if self.throughput_in_flight < self.throughput_reserve
+            && let Some(waiter) = self.throughput_waiters.pop()
+        {
+            return Some((waiter, Class::Throughput));
+        }
+        if let Some(waiter) = self.latency_waiters.pop_front() {
+            return Some((waiter, Class::Latency));
+        }
+        self.throughput_waiters.pop().map(|waiter| (waiter, Class::Throughput))
Evidence
The semaphore reserves permits.div_ceil(2) for throughput, which equals 1 when permits=1. The
grant policy checks throughput_in_flight < throughput_reserve before considering latency waiters,
and because release decrements in-flight first, each freed permit in a 1-slot pool makes
throughput_in_flight 0 and thus always prefers queued throughput. Since pacquet only rejects
network_concurrency==0, a user-configured value of 1 is valid and exposed.

pacquet/crates/network/src/priority_semaphore.rs[119-193]
pacquet/crates/network/src/lib.rs[538-545]
pacquet/crates/config/src/lib.rs[896-903]

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

## Issue description
When `network_concurrency` is configured to 1, the new two-class scheduling policy effectively becomes "downloads always win while any are queued" because the throughput reserve is computed as 1 and the grant rule checks `throughput_in_flight < throughput_reserve` after decrementing in-flight on release. This can delay metadata fetches behind downloads and slow/serialize resolution.
### Issue Context
- `network_concurrency` values >= 1 are allowed (only 0 is rejected).
- For `permits=1`, the current `throughput_reserve` math makes the reserve equal to the whole pool.
### Fix Focus Areas
- Ensure `permits=1` cannot reserve the entire pool for throughput (e.g., set `throughput_reserve = 0` when `permits == 1`, or compute reserve as `permits / 2` for small permit counts).
- Add a focused unit test for the `permits=1` case with both latency and throughput queued.
- file/path[start_line-end_line]
- pacquet/crates/network/src/priority_semaphore.rs[120-193]
- pacquet/crates/network/src/lib.rs[538-545]
- pacquet/crates/config/src/lib.rs[896-903]

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



Remediation recommended

2. Slow-start over-penalizes chunks 🐞 Bug ≡ Correctness ⭐ New
Description
SlowStart::effective_rate() applies the current cwnd-derived rate to the entire chunk and only
then grows cwnd, so a single chunk larger than cwnd is delayed as if it were all sent at the
smaller window. With the pump’s 32KiB read chunks and INITIAL_CWND_BYTES ≈14.6KiB, this skews
--registry-slow-start benchmark timings by adding extra ramp time that the module docs say should
be amortized as cwnd grows per delivered window.
Code

pacquet/tasks/integrated-benchmark/src/latency_proxy.rs[R245-256]

+    /// The rate (bytes/sec) at which the next `len`-byte chunk
+    /// serializes, advancing the window-growth bookkeeping.
+    fn effective_rate(&mut self, cap: f64, len: usize) -> f64 {
+        let rate = (self.cwnd / self.rtt_secs).min(cap);
+        self.bytes_in_round += len as f64;
+        if self.bytes_in_round >= self.cwnd {
+            self.bytes_in_round = 0.0;
+            // Stop growing once the window sustains the cap
+            // (`cap × RTT` is the bandwidth-delay product).
+            self.cwnd = (self.cwnd * 2.0).min((cap * self.rtt_secs).max(INITIAL_CWND_BYTES));
+        }
+        rate
Evidence
The module claims the rate should be cwnd ÷ RTT and cwnd “doubles per delivered window”, but the
writer computes delay for the full 32KiB chunk using a single effective_rate() call and only then
increments cwnd once, meaning bytes beyond the first window are still charged at the earlier
(too-low) rate.

pacquet/tasks/integrated-benchmark/src/latency_proxy.rs[46-54]
pacquet/tasks/integrated-benchmark/src/latency_proxy.rs[170-204]
pacquet/tasks/integrated-benchmark/src/latency_proxy.rs[219-256]

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 slow-start proxy models cwnd growth only after an entire read chunk is accounted for, but serialization delay for that chunk is computed using the pre-growth cwnd-derived rate. When the read chunk exceeds cwnd (default 32KiB > 14.6KiB), the proxy delays the full chunk at the slower initial rate, overstating slow-start cost vs the intended “double per delivered window” behavior.

## Issue Context
This impacts integrated benchmark fidelity (not production networking). The goal is to approximate that transfers ramp up toward the cap over successive windows/RTTs; the current implementation effectively treats each read chunk as a single window regardless of size.

## Fix Focus Areas
- pacquet/tasks/integrated-benchmark/src/latency_proxy.rs[167-258]

Suggested implementation directions:
- In `pump()`, when slow-start is enabled, either:
 - split `bytes` into sub-slices sized to the remaining cwnd in the current round, updating cwnd and accumulating the serialization delay piecewise, or
 - change the reader buffer size to be <= `INITIAL_CWND_BYTES` (or dynamically <= current cwnd) so one chunk never spans multiple windows.
- Update `SlowStart::effective_rate` (or introduce a `serialize_delay(len, cap) -> Duration`) to advance cwnd multiple times when `len` spans multiple windows, rather than a single `if bytes_in_round >= cwnd` step.

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


3. NDJSON frames concat copies 🐞 Bug ➹ Performance ⭐ New
Description
The frozen-lockfile fast path builds a Vec<Vec<u8>> of pre-serialized frames and then
ndjson_frames() uses frames.concat(), allocating and copying the entire payload into a second
buffer. For large lockfiles (many package frames plus done), this creates avoidable CPU and
peak-memory overhead on the server.
Code

pnpr/crates/pnpr/src/resolver.rs[R529-537]

+/// A 200 NDJSON response carrying several already-serialized frames in
+/// one fixed body. Used by the frozen fast path, where every frame is
+/// known up front — no channel to stream from.
+fn ndjson_frames(frames: &[Vec<u8>]) -> Response {
+    Response::builder()
+        .status(StatusCode::OK)
+        .header(header::CONTENT_TYPE, NDJSON_CONTENT_TYPE)
+        .body(Body::from(frames.concat()))
+        .expect("binary response is always valid")
Evidence
The frozen fast path explicitly constructs a frames vector (potentially many entries) and then
builds the HTTP body via frames.concat(), which necessarily allocates and copies all bytes into a
new buffer.

pnpr/crates/pnpr/src/resolver.rs[244-257]
pnpr/crates/pnpr/src/resolver.rs[529-537]

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

## Issue description
`ndjson_frames()` currently materializes the full response body by calling `frames.concat()`, which allocates a new `Vec<u8>` and copies all per-frame bytes into it. Since the frozen path can produce one frame per tarball, this scales peak memory and adds an O(total_bytes) copy.

## Issue Context
The streaming path already uses `Body::from_stream(...)`. The frozen path is “known up front”, but it can still be emitted as a stream/iterator to avoid the extra aggregation copy.

## Fix Focus Areas
- pnpr/crates/pnpr/src/resolver.rs[244-257]
- pnpr/crates/pnpr/src/resolver.rs[529-552]

Suggested implementation directions:
- Replace `ndjson_frames(frames: &[Vec<u8>])` with a streaming body over an iterator (e.g., `stream::iter(frames.iter().cloned().map(Bytes::from))`) and `Body::from_stream`.
- Alternatively, if a fixed body is required, precompute total length and build a single `Vec<u8>` with `Vec::with_capacity(total)` + `extend_from_slice` to avoid `concat()`’s intermediate behavior.

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


4. ZIP downloads wrong class 🐞 Bug ➹ Performance
Description
ThrottledClient::acquire_for_url() now always acquires permits at UNPRIORITIZED (latency class),
but the ZIP/binary archive download path still uses acquire_for_url, so those large transfers are
scheduled as latency work instead of throughput work. This can cause binary ZIP downloads to be
preferred over prioritized tarball downloads and to contend with metadata FIFO, reducing the
intended scheduling behavior under pool saturation.
Code

pacquet/crates/network/src/lib.rs[R354-358]

   /// just to satisfy the type signature — the lookup itself works
   /// on the raw string form.
   pub async fn acquire_for_url(&self, url: &str) -> ThrottledClientGuard<'_> {
-        let permit =
-            self.semaphore.acquire().await.expect("semaphore shouldn't have been closed this soon");
+        self.acquire_for_url_with_priority(url, UNPRIORITIZED).await
+    }
Evidence
The network crate documents UNPRIORITIZED as the latency-class marker and makes
acquire_for_url() default to that value; the ZIP/binary archive downloader still calls
acquire_for_url() before performing a full archive GET+extract, so it will be scheduled as
latency-class work under saturation.

pacquet/crates/network/src/lib.rs[42-49]
pacquet/crates/network/src/lib.rs[354-378]
pacquet/crates/tarball/src/lib.rs[2364-2414]

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

## Issue description
`ThrottledClient::acquire_for_url()` now always queues work at `UNPRIORITIZED` (latency class), but at least one non-metadata *download* path still uses it: the ZIP/binary archive fetch+extract code. This misclassifies those large downloads as latency-class work under the new two-class policy.
## Issue Context
- `UNPRIORITIZED` is explicitly documented as the latency-class sentinel meant for metadata/packument requests.
- The ZIP path does a full-body download and extraction, so it should be treated like throughput-class work (even if its priority is unknown/0).
## Fix Focus Areas
- pacquet/crates/tarball/src/lib.rs[2388-2414]
- pacquet/crates/network/src/lib.rs[42-49]
- pacquet/crates/network/src/lib.rs[354-378]
## Implementation notes
- Switch the ZIP download path to use `acquire_for_url_with_priority(package_url, <priority>)`.
- If you don’t have size/file hints for ZIPs, use `0` to place them in the throughput class without outranking sized tarballs, or derive a priority from `content-length` when available.

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


View more (1)
5. Priority sentinel collision 🐞 Bug ≡ Correctness
Description
UNPRIORITIZED is u64::MAX and is used as a latency-class sentinel, but download_priority() can
also saturate to u64::MAX on 64-bit targets from extreme dist.unpackedSize/dist.fileCount
values. When that happens, a tarball download is misclassified as latency-class, breaking the
intended scheduling behavior.
Code

pacquet/crates/network/src/lib.rs[R42-49]

+/// Permit priority used by [`ThrottledClient::acquire`] /
+/// [`ThrottledClient::acquire_for_url`] for callers that don't pass an
+/// explicit one. Marks the request as latency class — packument and
+/// other metadata fetches that gate resolution progress — served FIFO
+/// and preferred over size-prioritized downloads beyond the downloads'
+/// reserved share of the pool (see the `priority_semaphore` module
+/// docs for the two-class grant policy).
+pub const UNPRIORITIZED: u64 = u64::MAX;
Evidence
The latency/throughput class is selected solely by priority == UNPRIORITIZED, so any throughput
priority equal to u64::MAX becomes latency-class. download_priority() uses saturating operations
and dist hints are parsed into usize, allowing extreme values on 64-bit that can saturate to
u64::MAX and trigger the collision.

pacquet/crates/network/src/lib.rs[42-55]
pacquet/crates/network/src/priority_semaphore.rs[52-55]
pacquet/crates/tarball/src/lib.rs[1817-1822]
pacquet/crates/package-manager/src/install_package_from_registry.rs[287-292]

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

## Issue description
`UNPRIORITIZED` is a sentinel value (`u64::MAX`) that maps to the latency class, but the tarball `download_priority()` computation can also yield `u64::MAX` via saturating arithmetic. This causes throughput-class tarball downloads to be enqueued as latency-class, bypassing size-based ordering and the reserve policy.
### Issue Context
- `UNPRIORITIZED` is used as a *class discriminator* (equality check), so any collision changes class.
- `dist` hints are parsed into `usize` and can be extremely large on 64-bit.
### Fix Focus Areas
- Ensure no throughput priority can ever equal `UNPRIORITIZED` (e.g., clamp computed download priorities to `UNPRIORITIZED - 1`, or change the sentinel scheme so the latency class is not represented by an in-range numeric priority).
- file/path[start_line-end_line]
- pacquet/crates/network/src/lib.rs[42-49]
- pacquet/crates/network/src/priority_semaphore.rs[52-55]
- pacquet/crates/tarball/src/lib.rs[1817-1822]

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


Grey Divider

Previous review results

Review updated until commit f2aaee0

Results up to commit 8144b56


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

Context used

Action required
1. Concurrency=1 blocks metadata 🐞 Bug ≡ Correctness
Description
With network_concurrency = 1, throughput_reserve becomes 1 (permits.div_ceil(2)), and
next_waiter() will always select a queued throughput waiter on every release when any downloads
are queued. This forces queued metadata requests to wait until the download queue drains, delaying
resolution progress under a supported configuration.
Code

pacquet/crates/network/src/priority_semaphore.rs[R120-192]

+    pub(crate) fn new(permits: usize) -> Self {
+        PrioritySemaphore {
+            state: Arc::new(Mutex::new(SemState {
+                free: permits,
+                latency_in_flight: 0,
+                throughput_in_flight: 0,
+                throughput_reserve: permits.div_ceil(2),
+                next_seq: 0,
+                latency_waiters: VecDeque::new(),
+                throughput_waiters: BinaryHeap::new(),
+            })),
+        }
+    }
+
+    /// Wait for a permit. Free permits are claimed immediately; when
+    /// the pool is saturated the caller queues in its class and is
+    /// woken per the grant policy in the module docs.
+    pub(crate) async fn acquire(&self, priority: u64) -> Permit {
+        let class = Class::of(priority);
+        let rx = {
+            let mut state = self.state.lock().expect("priority semaphore lock poisoned");
+            if state.free > 0 {
+                state.free -= 1;
+                *state.count_mut(class) += 1;
+                return Permit { state: Arc::clone(&self.state), class, armed: true };
+            }
+            let (tx, rx) = oneshot::channel();
+            let seq = state.next_seq;
+            state.next_seq += 1;
+            let waiter = Waiter { priority, seq, tx };
+            match class {
+                Class::Latency => state.latency_waiters.push_back(waiter),
+                Class::Throughput => state.throughput_waiters.push(waiter),
+            }
+            rx
+        };
+        rx.await.expect("priority semaphore state dropped while a permit was awaited")
+    }
+
+    #[cfg(test)]
+    pub(crate) fn queued_waiters(&self) -> usize {
+        let state = self.state.lock().expect("priority semaphore lock poisoned");
+        state.latency_waiters.len() + state.throughput_waiters.len()
+    }
+
+    #[cfg(test)]
+    pub(crate) fn available_permits(&self) -> usize {
+        self.state.lock().expect("priority semaphore lock poisoned").free
+    }
+}
+
+impl SemState {
+    fn count_mut(&mut self, class: Class) -> &mut usize {
+        match class {
+            Class::Latency => &mut self.latency_in_flight,
+            Class::Throughput => &mut self.throughput_in_flight,
+        }
+    }
+
+    /// Pop the waiter the grant policy picks next, or `None` when both
+    /// queues are empty: throughput work below its reserve goes first,
+    /// then queued latency work, then throughput work above the
+    /// reserve.
+    fn next_waiter(&mut self) -> Option<(Waiter, Class)> {
+        if self.throughput_in_flight < self.throughput_reserve
+            && let Some(waiter) = self.throughput_waiters.pop()
+        {
+            return Some((waiter, Class::Throughput));
+        }
+        if let Some(waiter) = self.latency_waiters.pop_front() {
+            return Some((waiter, Class::Latency));
+        }
+        self.throughput_waiters.pop().map(|waiter| (waiter, Class::Throughput))
Evidence
The semaphore reserves permits.div_ceil(2) for throughput, which equals 1 when permits=1. The
grant policy checks throughput_in_flight < throughput_reserve before considering latency waiters,
and because release decrements in-flight first, each freed permit in a 1-slot pool makes
throughput_in_flight 0 and thus always prefers queued throughput. Since pacquet only rejects
network_concurrency==0, a user-configured value of 1 is valid and exposed.

pacquet/crates/network/src/priority_semaphore.rs[119-193]
pacquet/crates/network/src/lib.rs[538-545]
pacquet/crates/config/src/lib.rs[896-903]

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

## Issue description
When `network_concurrency` is configured to 1, the new two-class scheduling policy effectively becomes "downloads always win while any are queued" because the throughput reserve is computed as 1 and the grant rule checks `throughput_in_flight < throughput_reserve` after decrementing in-flight on release. This can delay metadata fetches behind downloads and slow/serialize resolution.
### Issue Context
- `network_concurrency` values >= 1 are allowed (only 0 is rejected).
- For `permits=1`, the current `throughput_reserve` math makes the reserve equal to the whole pool.
### Fix Focus Areas
- Ensure `permits=1` cannot reserve the entire pool for throughput (e.g., set `throughput_reserve = 0` when `permits == 1`, or compute reserve as `permits / 2` for small permit counts).
- Add a focused unit test for the `permits=1` case with both latency and throughput queued.
- file/path[start_line-end_line]
- pacquet/crates/network/src/priority_semaphore.rs[120-193]
- pacquet/crates/network/src/lib.rs[538-545]
- pacquet/crates/config/src/lib.rs[896-903]

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



Remediation recommended
2. ZIP downloads wrong class 🐞 Bug ➹ Performance ⭐ New
Description
ThrottledClient::acquire_for_url() now always acquires permits at UNPRIORITIZED (latency class),
but the ZIP/binary archive download path still uses acquire_for_url, so those large transfers are
scheduled as latency work instead of throughput work. This can cause binary ZIP downloads to be
preferred over prioritized tarball downloads and to contend with metadata FIFO, reducing the
intended scheduling behavior under pool saturation.
Code

pacquet/crates/network/src/lib.rs[R354-358]

    /// just to satisfy the type signature — the lookup itself works
    /// on the raw string form.
    pub async fn acquire_for_url(&self, url: &str) -> ThrottledClientGuard<'_> {
-        let permit =
-            self.semaphore.acquire().await.expect("semaphore shouldn't have been closed this soon");
+        self.acquire_for_url_with_priority(url, UNPRIORITIZED).await
+    }
Evidence
The network crate documents UNPRIORITIZED as the latency-class marker and makes
acquire_for_url() default to that value; the ZIP/binary archive downloader still calls
acquire_for_url() before performing a full archive GET+extract, so it will be scheduled as
latency-class work under saturation.

pacquet/crates/network/src/lib.rs[42-49]
pacquet/crates/network/src/lib.rs[354-378]
pacquet/crates/tarball/src/lib.rs[2364-2414]

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

## Issue description
`ThrottledClient::acquire_for_url()` now always queues work at `UNPRIORITIZED` (latency class), but at least one non-metadata *download* path still uses it: the ZIP/binary archive fetch+extract code. This misclassifies those large downloads as latency-class work under the new two-class policy.

## Issue Context
- `UNPRIORITIZED` is explicitly documented as the latency-class sentinel meant for metadata/packument requests.
- The ZIP path does a full-body download and extraction, so it should be treated like throughput-class work (even if its priority is unknown/0).

## Fix Focus Areas
- pacquet/crates/tarball/src/lib.rs[2388-2414]
- pacquet/crates/network/src/lib.rs[42-49]
- pacquet/crates/network/src/lib.rs[354-378]

## Implementation notes
- Switch the ZIP download path to use `acquire_for_url_with_priority(package_url, <priority>)`.
- If you don’t have size/file hints for ZIPs, use `0` to place them in the throughput class without outranking sized tarballs, or derive a priority from `content-length` when available.

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


3. Priority sentinel collision 🐞 Bug ≡ Correctness
Description
UNPRIORITIZED is u64::MAX and is used as a latency-class sentinel, but download_priority() can
also saturate to u64::MAX on 64-bit targets from extreme dist.unpackedSize/dist.fileCount
values. When that happens, a tarball download is misclassified as latency-class, breaking the
intended scheduling behavior.
Code

pacquet/crates/network/src/lib.rs[R42-49]

+/// Permit priority used by [`ThrottledClient::acquire`] /
+/// [`ThrottledClient::acquire_for_url`] for callers that don't pass an
+/// explicit one. Marks the request as latency class — packument and
+/// other metadata fetches that gate resolution progress — served FIFO
+/// and preferred over size-prioritized downloads beyond the downloads'
+/// reserved share of the pool (see the `priority_semaphore` module
+/// docs for the two-class grant policy).
+pub const UNPRIORITIZED: u64 = u64::MAX;
Evidence
The latency/throughput class is selected solely by priority == UNPRIORITIZED, so any throughput
priority equal to u64::MAX becomes latency-class. download_priority() uses saturating operations
and dist hints are parsed into usize, allowing extreme values on 64-bit that can saturate to
u64::MAX and trigger the collision.

pacquet/crates/network/src/lib.rs[42-55]
pacquet/crates/network/src/priority_semaphore.rs[52-55]
pacquet/crates/tarball/src/lib.rs[1817-1822]
pacquet/crates/package-manager/src/install_package_from_registry.rs[287-292]

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

## Issue description
`UNPRIORITIZED` is a sentinel value (`u64::MAX`) that maps to the latency class, but the tarball `download_priority()` computation can also yield `u64::MAX` via saturating arithmetic. This causes throughput-class tarball downloads to be enqueued as latency-class, bypassing size-based ordering and the reserve policy.
### Issue Context
- `UNPRIORITIZED` is used as a *class discriminator* (equality check), so any collision changes class.
- `dist` hints are parsed into `usize` and can be extremely large on 64-bit.
### Fix Focus Areas
- Ensure no throughput priority can ever equal `UNPRIORITIZED` (e.g., clamp computed download priorities to `UNPRIORITIZED - 1`, or change the sentinel scheme so the latency class is not represented by an in-range numeric priority).
- file/path[start_line-end_line]
- pacquet/crates/network/src/lib.rs[42-49]
- pacquet/crates/network/src/priority_semaphore.rs[52-55]
- pacquet/crates/tarball/src/lib.rs[1817-1822]

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


Results up to commit e450e63


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

Context used

Action required
1. Concurrency=1 blocks metadata 🐞 Bug ≡ Correctness
Description
With network_concurrency = 1, throughput_reserve becomes 1 (permits.div_ceil(2)), and
next_waiter() will always select a queued throughput waiter on every release when any downloads
are queued. This forces queued metadata requests to wait until the download queue drains, delaying
resolution progress under a supported configuration.
Code

pacquet/crates/network/src/priority_semaphore.rs[R120-192]

+    pub(crate) fn new(permits: usize) -> Self {
+        PrioritySemaphore {
+            state: Arc::new(Mutex::new(SemState {
+                free: permits,
+                latency_in_flight: 0,
+                throughput_in_flight: 0,
+                throughput_reserve: permits.div_ceil(2),
+                next_seq: 0,
+                latency_waiters: VecDeque::new(),
+                throughput_waiters: BinaryHeap::new(),
+            })),
+        }
+    }
+
+    /// Wait for a permit. Free permits are claimed immediately; when
+    /// the pool is saturated the caller queues in its class and is
+    /// woken per the grant policy in the module docs.
+    pub(crate) async fn acquire(&self, priority: u64) -> Permit {
+        let class = Class::of(priority);
+        let rx = {
+            let mut state = self.state.lock().expect("priority semaphore lock poisoned");
+            if state.free > 0 {
+                state.free -= 1;
+                *state.count_mut(class) += 1;
+                return Permit { state: Arc::clone(&self.state), class, armed: true };
+            }
+            let (tx, rx) = oneshot::channel();
+            let seq = state.next_seq;
+            state.next_seq += 1;
+            let waiter = Waiter { priority, seq, tx };
+            match class {
+                Class::Latency => state.latency_waiters.push_back(waiter),
+                Class::Throughput => state.throughput_waiters.push(waiter),
+            }
+            rx
+        };
+        rx.await.expect("priority semaphore state dropped while a permit was awaited")
+    }
+
+    #[cfg(test)]
+    pub(crate) fn queued_waiters(&self) -> usize {
+        let state = self.state.lock().expect("priority semaphore lock poisoned");
+        state.latency_waiters.len() + state.throughput_waiters.len()
+    }
+
+    #[cfg(test)]
+    pub(crate) fn available_permits(&self) -> usize {
+        self.state.lock().expect("priority semaphore lock poisoned").free
+    }
+}
+
+impl SemState {
+    fn count_mut(&mut self, class: Class) -> &mut usize {
+        match class {
+            Class::Latency => &mut self.latency_in_flight,
+            Class::Throughput => &mut self.throughput_in_flight,
+        }
+    }
+
+    /// Pop the waiter the grant policy picks next, or `None` when both
+    /// queues are empty: throughput work below its reserve goes first,
+    /// then queued latency work, then throughput work above the
+    /// reserve.
+    fn next_waiter(&mut self) -> Option<(Waiter, Class)> {
+        if self.throughput_in_flight < self.throughput_reserve
+            && let Some(waiter) = self.throughput_waiters.pop()
+        {
+            return Some((waiter, Class::Throughput));
+        }
+        if let Some(waiter) = self.latency_waiters.pop_front() {
+            return Some((waiter, Class::Latency));
+        }
+        self.throughput_waiters.pop().map(|waiter| (waiter, Class::Throughput))
Evidence
The semaphore reserves permits.div_ceil(2) for throughput, which equals 1 when permits=1. The
grant policy checks throughput_in_flight < throughput_reserve before considering latency waiters,
and because release decrements in-flight first, each freed permit in a 1-slot pool makes
throughput_in_flight 0 and thus always prefers queued throughput. Since pacquet only rejects
network_concurrency==0, a user-configured value of 1 is valid and exposed.

pacquet/crates/network/src/priority_semaphore.rs[119-193]
pacquet/crates/network/src/lib.rs[538-545]
pacquet/crates/config/src/lib.rs[896-903]

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

### Issue description
When `network_concurrency` is configured to 1, the new two-class scheduling policy effectively becomes "downloads always win while any are queued" because the throughput reserve is computed as 1 and the grant rule checks `throughput_in_flight < throughput_reserve` after decrementing in-flight on release. This can delay metadata fetches behind downloads and slow/serialize resolution.

### Issue Context
- `network_concurrency` values >= 1 are allowed (only 0 is rejected).
- For `permits=1`, the current `throughput_reserve` math makes the reserve equal to the whole pool.

### Fix Focus Areas
- Ensure `permits=1` cannot reserve the entire pool for throughput (e.g., set `throughput_reserve = 0` when `permits == 1`, or compute reserve as `permits / 2` for small permit counts).
- Add a focused unit test for the `permits=1` case with both latency and throughput queued.
- file/path[start_line-end_line]
 - pacquet/crates/network/src/priority_semaphore.rs[120-193]
 - pacquet/crates/network/src/lib.rs[538-545]
 - pacquet/crates/config/src/lib.rs[896-903]

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



Remediation recommended
2. Priority sentinel collision 🐞 Bug ≡ Correctness
Description
UNPRIORITIZED is u64::MAX and is used as a latency-class sentinel, but download_priority() can
also saturate to u64::MAX on 64-bit targets from extreme dist.unpackedSize/dist.fileCount
values. When that happens, a tarball download is misclassified as latency-class, breaking the
intended scheduling behavior.
Code

pacquet/crates/network/src/lib.rs[R42-49]

+/// Permit priority used by [`ThrottledClient::acquire`] /
+/// [`ThrottledClient::acquire_for_url`] for callers that don't pass an
+/// explicit one. Marks the request as latency class — packument and
+/// other metadata fetches that gate resolution progress — served FIFO
+/// and preferred over size-prioritized downloads beyond the downloads'
+/// reserved share of the pool (see the `priority_semaphore` module
+/// docs for the two-class grant policy).
+pub const UNPRIORITIZED: u64 = u64::MAX;
Evidence
The latency/throughput class is selected solely by priority == UNPRIORITIZED, so any throughput
priority equal to u64::MAX becomes latency-class. download_priority() uses saturating operations
and dist hints are parsed into usize, allowing extreme values on 64-bit that can saturate to
u64::MAX and trigger the collision.

pacquet/crates/network/src/lib.rs[42-55]
pacquet/crates/network/src/priority_semaphore.rs[52-55]
pacquet/crates/tarball/src/lib.rs[1817-1822]
pacquet/crates/package-manager/src/install_package_from_registry.rs[287-292]

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

### Issue description
`UNPRIORITIZED` is a sentinel value (`u64::MAX`) that maps to the latency class, but the tarball `download_priority()` computation can also yield `u64::MAX` via saturating arithmetic. This causes throughput-class tarball downloads to be enqueued as latency-class, bypassing size-based ordering and the reserve policy.

### Issue Context
- `UNPRIORITIZED` is used as a *class discriminator* (equality check), so any collision changes class.
- `dist` hints are parsed into `usize` and can be extremely large on 64-bit.

### Fix Focus Areas
- Ensure no throughput priority can ever equal `UNPRIORITIZED` (e.g., clamp computed download priorities to `UNPRIORITIZED - 1`, or change the sentinel scheme so the latency class is not represented by an in-range numeric priority).
- file/path[start_line-end_line]
 - pacquet/crates/network/src/lib.rs[42-49]
 - pacquet/crates/network/src/priority_semaphore.rs[52-55]
 - pacquet/crates/tarball/src/lib.rs[1817-1822]

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


Qodo Logo

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

Copy link
Copy Markdown

PR Summary by Qodo

perf(network): prioritize tarball downloads by estimated pipeline work
✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

Walkthroughs

Description
• 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.
Diagram
graph TD
  A["pnpr server"] --> B["pnpr-client"] --> C["TarballPrefetcher"] --> D["Tarball downloader"] --> E["ThrottledClient"] --> F["PrioritySemaphore"]
  M["Metadata fetch"] --> E --> R["Registry"]
  E --> R
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Two separate semaphores (metadata vs downloads) with fixed partition
  • ➕ Simpler to reason about than a shared scheduler
  • ➕ Hard isolation prevents class interference
  • ➖ Not work-conserving unless you add borrowing logic
  • ➖ Borrowing reintroduces scheduling complexity
  • ➖ Harder to guarantee FIFO for metadata while still picking largest download
2. Weighted fair queue / DRR-style scheduler over all requests
  • ➕ More principled fairness model and tunable weights
  • ➕ Can approximate reserved share without discrete threshold
  • ➖ Significantly more complex implementation and testing burden
  • ➖ Harder to explain and validate under cancellation/races
  • ➖ More knobs without clear evidence they outperform the simpler two-class policy
3. Rely on upstream Tokio semaphore + external priority queue wrapper
  • ➕ Less custom concurrency code to maintain
  • ➕ Potential reuse of existing abstractions
  • ➖ Still need custom cancel-safety and class-reserve logic
  • ➖ Integrating priorities with permit drop/release semantics is non-trivial
  • ➖ Likely ends up similar in complexity to the bespoke implementation

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.

Grey Divider

File Changes

Enhancement (17)
install.rs Pass dist stats from pnpr frames into tarball prefetch +7/-1

Pass dist stats from pnpr frames into tarball prefetch

• Extends pnpr install flow to pass 'unpacked_size' and 'file_count' into the prefetcher so downloads can be prioritized and buffers sized using resolver-provided hints.

pacquet/crates/cli/src/cli_args/install.rs


install_config_deps.rs Initialize package file-count hint in materialization options +1/-0

Initialize package file-count hint in materialization options

• Adds 'package_file_count: None' to tarball download options wiring for env-installer materialization.

pacquet/crates/env-installer/src/install_config_deps.rs


lib.rs Replace Tokio Semaphore with class-aware PrioritySemaphore in ThrottledClient +48/-13

Replace Tokio Semaphore with class-aware PrioritySemaphore in ThrottledClient

• Introduces 'UNPRIORITIZED' for latency-class requests and swaps the concurrency limiter to a priority-ordered semaphore. Adds 'acquire_for_url_with_priority' for tarball downloads while keeping metadata calls on the latency path.

pacquet/crates/network/src/lib.rs


priority_semaphore.rs Add PrioritySemaphore with two-class, non-starving scheduling policy +238/-0

Add PrioritySemaphore with two-class, non-starving scheduling policy

• Implements a cancel-safe semaphore that schedules latency (FIFO) vs throughput (max-priority with FIFO tie-break) with a reserved throughput share. Permit drop hands off to the next waiter and skips cancelled waiters without leaking permits.

pacquet/crates/network/src/priority_semaphore.rs


build_resolution_verifiers.rs Plumb optional observed dist-stats sink into verifier construction +8/-1

Plumb optional observed dist-stats sink into verifier construction

• Extends 'build_resolution_verifiers' to accept an 'ObservedDistStats' sink so verifiers can record 'dist' work statistics as a side effect of verification.

pacquet/crates/package-manager/src/build_resolution_verifiers.rs


install_package_by_snapshot.rs Expose tarball URL derivation and add file-count field to download options +12/-2

Expose tarball URL derivation and add file-count field to download options

• Adds 'package_file_count: None' where download options are assembled. Makes 'tarball_url_and_integrity' public so the pnpr server can derive byte-identical tarball URLs for client cache key alignment.

pacquet/crates/package-manager/src/install_package_by_snapshot.rs


install_package_from_registry.rs Extract dist.fileCount and factor dist-field parsing helper +14/-5

Extract dist.fileCount and factor dist-field parsing helper

• Adds 'manifest_file_count' and a shared 'manifest_dist_field' helper to read 'dist' numeric hints. Threads 'file_count' into tarball download options alongside 'unpacked_size'.

pacquet/crates/package-manager/src/install_package_from_registry.rs


prefetching_resolver.rs Prefetch tarballs with both unpackedSize and fileCount hints +3/-1

Prefetch tarballs with both unpackedSize and fileCount hints

• Collects 'dist.fileCount' from resolver manifests and passes both size hints into the tarball download request configuration.

pacquet/crates/package-manager/src/prefetching_resolver.rs


resolution_observer.rs Include unpackedSize and fileCount in resolved tarball hints +14/-1

Include unpackedSize and fileCount in resolved tarball hints

• Extends 'ResolvedPackageHint' with optional 'unpacked_size' and 'file_count' parsed from the manifest so the fetch side can prioritize and preallocate accurately.

pacquet/crates/package-manager/src/resolution_observer.rs


tarball_prefetch.rs Carry fileCount through TarballPrefetcher and spawned downloads +17/-3

Carry fileCount through TarballPrefetcher and spawned downloads

• Extends 'TarballDownload' and 'TarballPrefetcher::prefetch' to include 'unpacked_size' and 'file_count', ensuring downstream tarball fetching has the inputs for priority and decompression sizing.

pacquet/crates/package-manager/src/tarball_prefetch.rs


lib.rs Parse optional unpackedSize/fileCount from pnpr package frames +34/-2

Parse optional unpackedSize/fileCount from pnpr package frames

• Extends the NDJSON 'package' frame schema and 'ResolvedPackage' to carry optional dist stats, defaulting to 'None' for older servers or missing registry fields.

pacquet/crates/pnpr-client/src/lib.rs


create_npm_resolution_verifier.rs Record dist stats during verification and preserve them in abbreviated metadata +50/-0

Record dist stats during verification and preserve them in abbreviated metadata

• Adds 'DistStats' and an 'ObservedDistStats' sink collected opportunistically during tarball URL binding checks. Updates abbreviated metadata projection to retain 'unpackedSize' and 'fileCount' per version when present.

pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs


lookup_context.rs Extend abbreviated metadata projection with per-version dist stats +9/-0

Extend abbreviated metadata projection with per-version dist stats

• Adds 'version_dist_stats' to the abbreviated metadata projection so the verifier can surface work estimates without additional metadata fetches.

pacquet/crates/resolving-npm-resolver/src/lookup_context.rs


lib.rs Prioritize tarball acquires by estimated pipeline work (unpackedSize + K×fileCount) +45/-8

Prioritize tarball acquires by estimated pipeline work (unpackedSize + K×fileCount)

• Adds 'package_file_count', computes 'download_priority', and acquires network permits via 'acquire_for_url_with_priority' to start the most expensive tarballs first. Marks resolver-gating tarball-manifest fetches as 'UNPRIORITIZED' so they don't queue behind downloads.

pacquet/crates/tarball/src/lib.rs


latency_proxy.rs Simulate TCP slow start and fix macOS accepted-socket nonblocking inheritance +63/-1

Simulate TCP slow start and fix macOS accepted-socket nonblocking inheritance

• Adds optional slow-start rate ramping per connection to the proxy’s throughput cap logic. Ensures accepted sockets are set back to blocking mode to avoid 'WouldBlock' read failures on macOS/BSD.

pacquet/tasks/integrated-benchmark/src/latency_proxy.rs


resolver.rs Stream dist stats in package frames and announce sized tarballs for verified frozen lockfiles +127/-27

Stream dist stats in package frames and announce sized tarballs for verified frozen lockfiles

• Adds 'unpackedSize'/'fileCount' to streamed 'package' frames (omitted when unknown for compatibility). On verified frozen-lockfile fast path, emits deduplicated package frames with sizes derived from verifier-collected dist stats before the final 'done' frame.

pnpr/crates/pnpr/src/resolver.rs


upstream.rs Preserve dist.unpackedSize and dist.fileCount in abbreviated packuments +6/-7

Preserve dist.unpackedSize and dist.fileCount in abbreviated packuments

• Stops stripping 'unpackedSize' and 'fileCount' from abbreviated metadata so downstream components (pacquet) can use the hints for buffer sizing and download scheduling.

pnpr/crates/pnpr/src/upstream.rs


Refactor (2)
install.rs Pass no dist-stats sink for non-pnpr install path +1/-0

Pass no dist-stats sink for non-pnpr install path

• Updates the standard install path to pass 'None' for 'observed_dist_stats', limiting collection to call sites that need it (e.g., pnpr verification).

pacquet/crates/package-manager/src/install.rs


lib.rs Re-export dist-stats types and sink constructor +2/-1

Re-export dist-stats types and sink constructor

• Exports 'DistStats', 'ObservedDistStats', and 'observed_dist_stats_sink' for upstream consumers (pnpr server and package-manager wiring).

pacquet/crates/resolving-npm-resolver/src/lib.rs


Tests (7)
tests.rs Unit tests for priority ordering, FIFO ties, cancellation, and reserve policy +148/-0

Unit tests for priority ordering, FIFO ties, cancellation, and reserve policy

• Adds coverage for priority ordering, equal-priority FIFO behavior, cancellation handoff correctness, and the throughput-reserve vs metadata preference rules.

pacquet/crates/network/src/priority_semaphore/tests.rs


tests.rs Test pnpr package-frame parsing with and without dist stats +18/-2

Test pnpr package-frame parsing with and without dist stats

• Updates the package-frame parsing test to include the new fields and adds a compatibility test ensuring frames without dist stats still parse cleanly.

pacquet/crates/pnpr-client/src/tests.rs


tests.rs Test that verifier binding check records dist stats into sink +58/-1

Test that verifier binding check records dist stats into sink

• Adds a tokio test that mocks a registry response with dist stats and asserts the sink receives the correct '(name, version) -> DistStats' entry after verification.

pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs


tests.rs Update tarball tests for fileCount and explicit download priority argument +30/-0

Update tarball tests for fileCount and explicit download priority argument

• Adjusts test struct initializers and helper calls for the new 'package_file_count' field and the additional 'download_priority' parameter in internal fetch helpers.

pacquet/crates/tarball/src/tests.rs


tests.rs Extend proxy tests for slow-start behavior and new profile field +57/-3

Extend proxy tests for slow-start behavior and new profile field

• Updates existing tests to include the new 'slow_start' field and adds a test asserting slow start increases transfer time vs a flat-rate cap.

pacquet/tasks/integrated-benchmark/src/latency_proxy/tests.rs


tests.rs Test package-frame dist stat omission and frozen lockfile tarball announcements +74/-0

Test package-frame dist stat omission and frozen lockfile tarball announcements

• Adds tests verifying sized vs unsized package frame serialization and that frozen verified lockfile emits tarball frames with sizes and skips non-tarball resolutions.

pnpr/crates/pnpr/src/resolver/tests.rs


tests.rs Update abbreviation tests to expect size-hint fields preserved +5/-4

Update abbreviation tests to expect size-hint fields preserved

• Adjusts upstream abbreviation test expectations to keep 'fileCount' and 'unpackedSize' while still dropping legacy 'npm-signature' and 'shasum' when appropriate.

pnpr/crates/pnpr/src/upstream/tests.rs


Other (4)
cli_args.rs Add benchmark flag to enable per-connection TCP slow-start simulation +10/-0

Add benchmark flag to enable per-connection TCP slow-start simulation

• Introduces '--registry-slow-start' to model TCP ramp-up behavior when bandwidth caps and latency are configured.

pacquet/tasks/integrated-benchmark/src/cli_args.rs


main.rs Wire registry slow-start CLI flag into proxy link profile +3/-0

Wire registry slow-start CLI flag into proxy link profile

• Threads 'registry_slow_start' from CLI args into the registry proxy’s 'LinkProfile' and into the constructed 'WorkEnv'.

pacquet/tasks/integrated-benchmark/src/main.rs


work_env.rs Persist slow-start setting and apply it to registry proxy spawning +4/-0

Persist slow-start setting and apply it to registry proxy spawning

• Adds 'registry_slow_start' to 'WorkEnv' and uses it when constructing the registry proxy profile; other proxies explicitly disable slow start.

pacquet/tasks/integrated-benchmark/src/work_env.rs


main.rs Update micro-benchmark tarball download options for fileCount +1/-0

Update micro-benchmark tarball download options for fileCount

• Extends benchmark setup to include 'package_file_count: None' in 'DownloadTarballToStore' initialization.

pacquet/tasks/micro-benchmark/src/main.rs


Grey Divider

Qodo Logo

Comment thread pacquet/crates/network/src/priority_semaphore.rs

@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.

🧹 Nitpick comments (1)
pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs (1)

1059-1111: 💤 Low value

Well-structured test coverage for dist stats collection.

The test correctly verifies that the sink is populated when both unpackedSize and fileCount are present in the registry response. Consider adding a test case for partial field presence (e.g., only unpackedSize present, or only fileCount), 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(&registry) 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(&registry) 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

📥 Commits

Reviewing files that changed from the base of the PR and between d2125b8 and e450e63.

📒 Files selected for processing (30)
  • pacquet/crates/cli/src/cli_args/install.rs
  • pacquet/crates/env-installer/src/install_config_deps.rs
  • pacquet/crates/network/src/lib.rs
  • pacquet/crates/network/src/priority_semaphore.rs
  • pacquet/crates/network/src/priority_semaphore/tests.rs
  • pacquet/crates/package-manager/src/build_resolution_verifiers.rs
  • pacquet/crates/package-manager/src/install.rs
  • pacquet/crates/package-manager/src/install_package_by_snapshot.rs
  • pacquet/crates/package-manager/src/install_package_from_registry.rs
  • pacquet/crates/package-manager/src/prefetching_resolver.rs
  • pacquet/crates/package-manager/src/resolution_observer.rs
  • pacquet/crates/package-manager/src/tarball_prefetch.rs
  • pacquet/crates/pnpr-client/src/lib.rs
  • pacquet/crates/pnpr-client/src/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs
  • pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/lib.rs
  • pacquet/crates/resolving-npm-resolver/src/lookup_context.rs
  • pacquet/crates/tarball/src/lib.rs
  • pacquet/crates/tarball/src/tests.rs
  • pacquet/tasks/integrated-benchmark/src/cli_args.rs
  • pacquet/tasks/integrated-benchmark/src/latency_proxy.rs
  • pacquet/tasks/integrated-benchmark/src/latency_proxy/tests.rs
  • pacquet/tasks/integrated-benchmark/src/main.rs
  • pacquet/tasks/integrated-benchmark/src/work_env.rs
  • pacquet/tasks/micro-benchmark/src/main.rs
  • pnpr/crates/pnpr/src/resolver.rs
  • pnpr/crates/pnpr/src/resolver/tests.rs
  • pnpr/crates/pnpr/src/upstream.rs
  • pnpr/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 fires pnpm:<channel> events through globalLogger, logger.debug(...), or streamParser.write(...), mirror the call site, payload, and ordering so @pnpm/cli.default-reporter parses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plain String or &str in Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too via TryFrom<String> and/or FromStr and 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 infallible From<String> constructor
If upstream TypeScript occasionally constructs a branded string without validation, expose from_str_unchecked in 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 handwriting impl blocks; use manual impl only when conversion needs custom logic
Model TypeScript string literal unions (like 'auto' | 'always' | 'never') as Rust enums 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 in CODE_STYLE_GUIDE.md — imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching, pipe-trait, error handling, test layout, and cloning of Arc and Rc
Choose owned vs. borrowed parameters to minimize copies; widen to t...

Files:

  • pacquet/crates/env-installer/src/install_config_deps.rs
  • pacquet/crates/resolving-npm-resolver/src/lookup_context.rs
  • pacquet/crates/package-manager/src/install.rs
  • pacquet/crates/resolving-npm-resolver/src/lib.rs
  • pacquet/crates/cli/src/cli_args/install.rs
  • pacquet/tasks/micro-benchmark/src/main.rs
  • pacquet/crates/package-manager/src/resolution_observer.rs
  • pacquet/crates/pnpr-client/src/tests.rs
  • pacquet/crates/package-manager/src/build_resolution_verifiers.rs
  • pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs
  • pacquet/tasks/integrated-benchmark/src/cli_args.rs
  • pacquet/tasks/integrated-benchmark/src/main.rs
  • pacquet/crates/package-manager/src/prefetching_resolver.rs
  • pacquet/crates/network/src/priority_semaphore/tests.rs
  • pacquet/crates/pnpr-client/src/lib.rs
  • pacquet/crates/package-manager/src/install_package_by_snapshot.rs
  • pacquet/tasks/integrated-benchmark/src/latency_proxy.rs
  • pacquet/crates/package-manager/src/install_package_from_registry.rs
  • pacquet/tasks/integrated-benchmark/src/latency_proxy/tests.rs
  • pacquet/crates/package-manager/src/tarball_prefetch.rs
  • pacquet/crates/network/src/priority_semaphore.rs
  • pacquet/crates/tarball/src/lib.rs
  • pacquet/tasks/integrated-benchmark/src/work_env.rs
  • pacquet/crates/network/src/lib.rs
  • pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs
  • pacquet/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.rs
  • pnpr/crates/pnpr/src/resolver/tests.rs
  • pnpr/crates/pnpr/src/resolver.rs
  • pnpr/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.rs
  • pacquet/crates/resolving-npm-resolver/src/lookup_context.rs
  • pacquet/crates/package-manager/src/install.rs
  • pacquet/crates/resolving-npm-resolver/src/lib.rs
  • pacquet/crates/cli/src/cli_args/install.rs
  • pacquet/crates/package-manager/src/resolution_observer.rs
  • pacquet/crates/pnpr-client/src/tests.rs
  • pacquet/crates/package-manager/src/build_resolution_verifiers.rs
  • pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs
  • pacquet/crates/package-manager/src/prefetching_resolver.rs
  • pacquet/crates/network/src/priority_semaphore/tests.rs
  • pacquet/crates/pnpr-client/src/lib.rs
  • pacquet/crates/package-manager/src/install_package_by_snapshot.rs
  • pacquet/crates/package-manager/src/install_package_from_registry.rs
  • pacquet/crates/package-manager/src/tarball_prefetch.rs
  • pacquet/crates/network/src/priority_semaphore.rs
  • pacquet/crates/tarball/src/lib.rs
  • pacquet/crates/network/src/lib.rs
  • pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs
  • pacquet/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.rs
  • pacquet/crates/resolving-npm-resolver/src/lookup_context.rs
  • pacquet/crates/package-manager/src/install.rs
  • pacquet/crates/resolving-npm-resolver/src/lib.rs
  • pacquet/crates/cli/src/cli_args/install.rs
  • pacquet/crates/package-manager/src/resolution_observer.rs
  • pacquet/crates/pnpr-client/src/tests.rs
  • pacquet/crates/package-manager/src/build_resolution_verifiers.rs
  • pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs
  • pacquet/crates/package-manager/src/prefetching_resolver.rs
  • pacquet/crates/network/src/priority_semaphore/tests.rs
  • pacquet/crates/pnpr-client/src/lib.rs
  • pacquet/crates/package-manager/src/install_package_by_snapshot.rs
  • pacquet/crates/package-manager/src/install_package_from_registry.rs
  • pacquet/crates/package-manager/src/tarball_prefetch.rs
  • pacquet/crates/network/src/priority_semaphore.rs
  • pacquet/crates/tarball/src/lib.rs
  • pacquet/crates/network/src/lib.rs
  • pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs
  • pacquet/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.rs
  • pacquet/crates/resolving-npm-resolver/src/lookup_context.rs
  • pacquet/crates/package-manager/src/install.rs
  • pacquet/crates/resolving-npm-resolver/src/lib.rs
  • pacquet/crates/cli/src/cli_args/install.rs
  • pacquet/tasks/micro-benchmark/src/main.rs
  • pacquet/crates/package-manager/src/resolution_observer.rs
  • pacquet/crates/pnpr-client/src/tests.rs
  • pacquet/crates/package-manager/src/build_resolution_verifiers.rs
  • pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs
  • pacquet/tasks/integrated-benchmark/src/cli_args.rs
  • pacquet/tasks/integrated-benchmark/src/main.rs
  • pacquet/crates/package-manager/src/prefetching_resolver.rs
  • pacquet/crates/network/src/priority_semaphore/tests.rs
  • pacquet/crates/pnpr-client/src/lib.rs
  • pacquet/crates/package-manager/src/install_package_by_snapshot.rs
  • pacquet/tasks/integrated-benchmark/src/latency_proxy.rs
  • pacquet/crates/package-manager/src/install_package_from_registry.rs
  • pacquet/tasks/integrated-benchmark/src/latency_proxy/tests.rs
  • pacquet/crates/package-manager/src/tarball_prefetch.rs
  • pacquet/crates/network/src/priority_semaphore.rs
  • pacquet/crates/tarball/src/lib.rs
  • pacquet/tasks/integrated-benchmark/src/work_env.rs
  • pacquet/crates/network/src/lib.rs
  • pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs
  • pacquet/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.rs
  • pnpr/crates/pnpr/src/upstream/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/lookup_context.rs
  • pacquet/crates/package-manager/src/install.rs
  • pacquet/crates/resolving-npm-resolver/src/lib.rs
  • pacquet/crates/cli/src/cli_args/install.rs
  • pacquet/tasks/micro-benchmark/src/main.rs
  • pnpr/crates/pnpr/src/resolver/tests.rs
  • pacquet/crates/package-manager/src/resolution_observer.rs
  • pacquet/crates/pnpr-client/src/tests.rs
  • pacquet/crates/package-manager/src/build_resolution_verifiers.rs
  • pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier/tests.rs
  • pacquet/tasks/integrated-benchmark/src/cli_args.rs
  • pacquet/tasks/integrated-benchmark/src/main.rs
  • pacquet/crates/package-manager/src/prefetching_resolver.rs
  • pacquet/crates/network/src/priority_semaphore/tests.rs
  • pacquet/crates/pnpr-client/src/lib.rs
  • pacquet/crates/package-manager/src/install_package_by_snapshot.rs
  • pacquet/tasks/integrated-benchmark/src/latency_proxy.rs
  • pacquet/crates/package-manager/src/install_package_from_registry.rs
  • pacquet/tasks/integrated-benchmark/src/latency_proxy/tests.rs
  • pacquet/crates/package-manager/src/tarball_prefetch.rs
  • pacquet/crates/network/src/priority_semaphore.rs
  • pacquet/crates/tarball/src/lib.rs
  • pnpr/crates/pnpr/src/resolver.rs
  • pnpr/crates/pnpr/src/upstream.rs
  • pacquet/tasks/integrated-benchmark/src/work_env.rs
  • pacquet/crates/network/src/lib.rs
  • pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs
  • pacquet/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!

zkochan added 11 commits June 11, 2026 01:13
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.
@qodo-free-for-open-source-projects

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

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 8144b56

@zkochan

zkochan commented Jun 10, 2026

Copy link
Copy Markdown
Member Author

Both review findings are addressed in 8144b56:

  1. Concurrency=1 blocks metadata — the throughput reserve is now capped at permits - 1, so a single-permit pool reserves nothing for downloads and queued metadata is served first (covered by single_permit_pool_still_serves_metadata_first).
  2. Priority sentinel collisiondownload_priority now clamps to UNPRIORITIZED - 1, so saturated dist stats from a hostile registry can never reclassify a download as latency-class (covered by download_priority_never_reaches_the_latency_sentinel).

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.
@qodo-free-for-open-source-projects

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

Copy link
Copy Markdown

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

@zkochan zkochan merged commit 657d322 into main Jun 11, 2026
28 checks passed
@zkochan zkochan deleted the pnpr-perf-opt branch June 11, 2026 00:58
zkochan added a commit that referenced this pull request Jun 11, 2026
…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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants