Walkthrough

The integrated benchmark now plans and spawns per-revision registry mocks with latency proxies, and pnpr’s tarball serving path no longer reads or writes cached-tarball integrity sidecars. Related storage helpers and tests were removed, and server tests were updated for the new cache behavior.

Changes

Per-revision registry mocks in integrated benchmark

Tarball cache integrity sidecar removal

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

• pnpm/pnpm#12166: Refactors pacquet/tasks/integrated-benchmark/src/latency_proxy.rs in the same proxy startup area extended here.
• pnpm/pnpm#12593: Modifies pnpr/crates/pnpr/src/server.rs tarball handling and cache-path logic in the same request flow.
• pnpm/pnpm#12257: Updates integrated-benchmark Verdaccio proxy/registry plumbing related to the revision-specific mock routing added here.

Suggested labels

product: pacquet, product: pnpr

• X
• Mastodon
• Reddit
• LinkedIn

• ❄️ 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.

• Serve cached proxy tarballs directly, avoiding per-request packument parse and SRI re-hash.
• Preserve security by verifying tarballs at cache write-time and rejecting poisoned upstream bytes.
• Update integrated benchmark to use per-revision pnpr registry mocks so serve-path changes are
 measurable.

graph TD
  A["pnpm client"] --> B["pnpr server"] --> C{"Cache hit?"}
  C -->|"hit"| D[("proxy tarball cache")] --> E["stream cached .tgz"]
  C -->|"miss"| F["load packument"] --> G["download + verify"] --> D
  G --> H["upstream registry"]

  subgraph Legend
    direction LR
    _svc(["Service"]) ~~~ _db[("Storage")] ~~~ _dec{"Decision"}
  end

Loading

The following are alternative approaches to this PR:

1. Keep a tarball integrity sidecar for cache hits

• ➕ Retains a cheap on-serve validation gate without re-hashing bytes.
• ➕ Could still detect unexpected cache corruption independent of write-time checks.
• ➖ Extra I/O and metadata management on the hot path.
• ➖ Sidecar can drift or be corrupted, adding complexity and more failure modes.
• ➖ Still doesn’t justify packument lookup on cache hits; only partially addresses regression.

2. Introduce an in-memory packument/integrity lookup cache for serves

• ➕ Avoids packument parsing costs on repeated tarball serves while keeping serve-time binding.
• ➕ Can be tuned/evicted without disk metadata formats.
• ➖ More memory pressure and cache invalidation complexity.
• ➖ Still keeps serve-path work that is redundant given verified writes + client verification.
• ➖ Harder to make benchmark results stable across runs and environments.

Recommendation: Proceed with the PR’s approach: trust cache hits and rely on write-time verification (plus client-side verification) to preserve security while removing per-serve packument parsing and SRI hashing. The alternatives add complexity and/or leave redundant work on the hot path; given the benchmark profile (~1300 cached tarballs per cold-store install), minimizing per-request overhead is the correct trade-off.

Enhancement (1) +40 / -78

server.rsServe cached proxy tarballs without packument lookup or re-hashing +40/-78

Serve cached proxy tarballs without packument lookup or re-hashing

• Moves upstream resolution and cache-read earlier and returns cached tarballs directly on hits, skipping both packument parsing and SRI re-verification. Restricts packument lookup to the cache-miss download path where dist.integrity is required to verify bytes before caching; removes now-dead per-serve integrity sidecar helpers.

pnpr/crates/pnpr/src/server.rs

Refactor (1) +0 / -23

storage.rsRemove cached-tarball integrity sidecar APIs +0/-23

Remove cached-tarball integrity sidecar APIs

• Deletes Storage methods for reading/writing cached tarball integrity markers and removing cached tarballs tied to that sidecar. This matches the new server behavior which no longer depends on per-serve integrity sidecars.

pnpr/crates/pnpr/src/storage.rs

Tests (2) +7 / -48

tests.rsDrop tests covering removed tarball integrity sidecar behavior +0/-43

Drop tests covering removed tarball integrity sidecar behavior

• Removes helper path construction and test cases that validated tarball integrity sidecar round-tripping and sidecar deletion on tarball removal. Remaining tests continue to cover packument caching behavior and other storage semantics.

pnpr/crates/pnpr/src/storage/tests.rs

server.rsAssert poisoned upstream tarballs are rejected before caching +7/-5

Assert poisoned upstream tarballs are rejected before caching

• Updates the tampered-upstream tarball test to assert that mismatched dist.integrity bytes are rejected during cache write, ensuring the poisoned tarball is never written to disk. This aligns the security invariant with the new 'trust cache hits' serve behavior.

pnpr/crates/pnpr/tests/server.rs

Other (2) +204 / -11

work_env.rsAdd per-revision pnpr registry mocks and more robust cleanup +184/-9

Add per-revision pnpr registry mocks and more robust cleanup

• Extends the integrated benchmark harness to plan and spawn a tarball-serving mock per compared pnpr revision, routed via per-revision .npmrc registry URLs to avoid a shared-mock blind spot. Also adds retry logic for directory cleanup on macOS/APFS and threads revision-mock planning through init/benchmark flows.

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

pnpr_command.rsAllow starting registry-mock with an explicit pnpr binary path +20/-2

Allow starting registry-mock with an explicit pnpr binary path

• Refactors pnpr mock command construction so callers can provide a specific pnpr binary (used by the benchmark to run mocks built from each revision). Keeps the existing convenience wrapper that resolves the workspace pnpr binary.

pacquet/tasks/registry-mock/src/pnpr_command.rs

Description

In serve_tarball, a proxy-cache hit returns the cached .tgz before loading the packument, so
pnpr never resolves the tarball’s authoritative version and can skip the second
ensure_osv_allowed() check when that resolved version differs from the filename-derived version.
This can serve a tarball for a now-blocked OSV version after an OSV DB update/restart if the cached
tarball’s filename version is different/non-authoritative.

Code

pnpr/crates/pnpr/src/server.rs[R1188-1215]

+    let upstreams = resolve_upstreams(state, &name);
+    // Serve a cached tarball directly — without re-deriving its version from the
+    // packument or re-hashing the bytes. A tarball only enters the cache through
+    // `download_verified_to_cache` below, which verifies the bytes against the
+    // version's `dist.integrity` as they are written, so nothing unverified is
+    // ever stored; every install client also re-verifies whatever it receives
+    // against that same integrity. So a cache hit needs neither the packument
+    // lookup (the version/integrity binding) nor an SRI pass — both of which
+    // pnpm/pnpm#12570 added to *every* serve, making warm-cache cold-store pay a
+    // per-tarball packument parse plus hash. The OSV screen on the filename's
+    // version already ran above. Read the cache if any uplink in the chain
+    // mirrors tarballs (or there's no upstream left at all — a leftover mirror).
+    let should_read_cache = upstreams.is_empty() || upstreams.iter().any(|u| u.caches());
+    if should_read_cache {
+        match state.inner.storage.open_cached_tarball(&name, &filename).await {
+            Ok(Some((file, len))) => {
+                return tarball_response(streaming::stream_file(file), Some(len));
+            }
+            Ok(None) => {}
+            Err(err) => {
+                tracing::warn!(?err, package = %name.as_str(), %filename, "tarball cache open failed");
+            }
+        }
+    }
+
+    if upstreams.is_empty() {
+        return not_found();
+    }

Evidence

The new cache-hit early return happens before load_packument_bytes()/expected_tarball_dist() and
therefore before the “re-screen when resolved version differs” OSV check, even though the code
documents that the authoritative version is derived by matching the tarball basename in the
packument.

pnpr/crates/pnpr/src/server.rs[1149-1237]
pnpr/crates/pnpr/src/server.rs[1289-1344]

Agent prompt

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

## Issue description
`serve_tarball()` serves cached proxy tarballs before resolving the authoritative version from the packument, which can skip the OSV screen for the resolved version (the one whose `dist.tarball` basename matched). This creates a gap where cached tarballs can bypass OSV blocking if the filename-encoded version is not the true version.
### Issue Context
- The handler explicitly acknowledges that the filename-carried version is best-effort and that the authoritative version comes from packument resolution.
- The “re-screen when resolved version differs” logic currently only runs on cache misses.
### Fix Focus Areas
- pnpr/crates/pnpr/src/server.rs[1149-1237]
- pnpr/crates/pnpr/src/server.rs[1289-1344]
### Implementation guidance
- Ensure OSV enforcement uses the authoritative resolved version even on cache hits.
- Minimal/security-first option: when OSV is enabled (`state.inner.osv_index.is_some()`), resolve `TarballDist` from the packument (or an equivalent lightweight metadata cache) before returning a cached tarball, and run `ensure_osv_allowed()` for the resolved version when it differs from `name_version`.
- Performance-preserving option: persist the resolved version (and optionally integrity) as cache metadata at write time, and read that small metadata on cache hit to run the correct OSV check without loading/parsing the full packument.

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

Description

start_revision_mock() chooses a mock server port via pick_unused_port(), which does not reserve the
port; another local process can claim it before pnpr binds, causing spawn() to fail/panic and making
benchmarks flaky.

Code

pacquet/tasks/integrated-benchmark/src/work_env.rs[R516-545]

+    fn start_revision_mock(&self, revision: &str, registry: RevisionMockRegistry) -> PnprServer {
+        let binary = self.pnpr_server_binary(revision);
+        assert!(
+            binary.is_file(),
+            "pnpr binary not found at {binary:?} — the build step did not produce it",
+        );
+        let mock_port = pick_unused_port().expect("pick a port for the revision mock");
+
+        let bench_dir = self.bench_dir(BenchId::PnprRevision(revision));
+        let stdout = File::create(bench_dir.join("revision-mock.stdout.log"))
+            .expect("create revision mock stdout log");
+        let stderr = File::create(bench_dir.join("revision-mock.stderr.log"))
+            .expect("create revision mock stderr log");
+
+        eprintln!(
+            "Serving {revision}'s tarballs from a mock built from pnpr@{revision} on 127.0.0.1:{mock_port}...",
+        );
+        // The mock advertises its tarball URLs at the client-facing proxy
+        // URL (`registry.url`), not its own loopback port, so downloads cross
+        // the emulated registry link instead of bypassing it.
+        let process = pacquet_registry_mock::pnpr_command_with_binary(
+            &binary,
+            mock_port,
+            Some(&registry.url),
+        )
+        .stdin(Stdio::null())
+        .stdout(stdout)
+        .stderr(stderr)
+        .spawn()
+        .expect("spawn revision mock");

Evidence

The benchmark selects a port with pick_unused_port() and immediately spawns the mock process on
that port, but pick_unused_port() only probes by binding to :0 and returning the port, dropping
the listener right away—so the returned port is not reserved and is subject to TOCTOU races.

pacquet/tasks/integrated-benchmark/src/work_env.rs[516-545]
pacquet/tasks/registry-mock/src/pick_port.rs[1-7]

Agent prompt

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

## Issue description
`start_revision_mock()` selects `mock_port` using `pick_unused_port()`, which returns an unreserved port number. Between selection and the child process binding, another process can bind the port, causing `.spawn()` to fail and the benchmark to panic.
## Issue Context
The PR already reserves the *proxy* port by binding a `TcpListener` during planning time (`plan_revision_mocks()` + `LatencyProxy::spawn_with_listener()`), but the per-revision *mock server* port still uses an unreserved pick.
## Fix Focus Areas
- pacquet/tasks/integrated-benchmark/src/work_env.rs[516-545]
- pacquet/tasks/registry-mock/src/pick_port.rs[1-7]
## Suggested fix
Wrap mock startup in a small retry loop:
- pick a port
- attempt to spawn pnpr
- if spawn fails with `AddrInUse` (or OS-specific equivalent), retry with a new port up to N times
- otherwise fail fast
(Alternative if feasible: add support in pnpr/launcher to bind to `127.0.0.1:0` and report the chosen port back, avoiding TOCTOU entirely.)

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

Description

Per-revision pnpr registry mocks are spawned without overriding pnpr’s upstream/uplink URL, which
defaults to https://registry.npmjs.org, so any cache miss bypasses the benchmark’s intended
local-registry/network-emulation path. This can make results network-dependent/flaky and distort the
performance delta the benchmark is trying to isolate.

Code

pacquet/tasks/integrated-benchmark/src/work_env.rs[R536-545]

+        let process = pacquet_registry_mock::pnpr_command_with_binary(
+            &binary,
+            mock_port,
+            Some(&registry.url),
+        )
+        .stdin(Stdio::null())
+        .stdout(stdout)
+        .stderr(stderr)
+        .spawn()
+        .expect("spawn revision mock");

Evidence

The benchmark spawns per-revision pnpr mocks via pnpr_command_with_binary without any upstream
override. pnpr’s default proxy config routes all packages through the npmjs uplink at
https://registry.npmjs.org, so cache misses will escape the local registry path and its emulated
link characteristics.

pacquet/tasks/integrated-benchmark/src/work_env.rs[516-563]
pacquet/tasks/registry-mock/src/pnpr_command.rs[75-116]
pnpr/crates/pnpr/src/config.rs[1042-1057]

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 integrated benchmark now spawns per-revision `pnpr` proxy mocks, but the spawn path does not override the mock’s uplink/upstream registry. pnpr’s proxy defaults route `**` to the `npmjs` uplink (`https://registry.npmjs.org`), so any cache miss causes real-network fetches that bypass the benchmark’s local Verdaccio/latency/bandwidth model.
### Issue Context
The mock currently configures `--storage`, `--listen`, and `--public-url`, but not the upstream registry/uplink URL. The benchmark uses `RegistryMode::Verdaccio` specifically to keep traffic local and controllable; letting misses escape to npmjs undermines reproducibility.
### Fix Focus Areas
- pacquet/tasks/integrated-benchmark/src/work_env.rs[516-563]
- pacquet/tasks/registry-mock/src/pnpr_command.rs[75-116]
- pnpr/crates/pnpr/src/config.rs[1042-1057]
### What to change
- Extend `pacquet_registry_mock::pnpr_command_with_binary(...)` (or add a new helper) to accept an upstream registry URL (e.g. the benchmark’s `client_registry` or `registry_cache_populator`).
- Generate a small pnpr config file for the revision mock (or modify the default config) that sets the `npmjs` uplink URL to the local registry URL, then pass it via `--config` when spawning the mock.
- Ensure the per-revision mock stays within the same network-emulation topology as other benchmark targets (client→proxy link emulated; proxy→upstream should also be local/controlled).

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

Description

When revision mocks are enabled, pacquet@/pnpr@ targets can be routed to per-revision mock URLs,
but pnpm@ and static targets cannot (no revision key), so a single hyperfine run may compare
installs hitting different registries/network paths. This can invalidate cross-target comparisons
within the same run output.

Code

pacquet/tasks/integrated-benchmark/src/work_env.rs[R808-820]

+    fn registry_for<'a>(
+        &'a self,
+        id: BenchId,
+        client_registry: &'a str,
+        revision_mocks: &'a HashMap<String, RevisionMockRegistry>,
+    ) -> &'a str {
+        if id.is_proxy_cache_populator() {
+            return &self.registry_cache_populator;
+        }
+        if let Some(mock) = id.revision().and_then(|rev| revision_mocks.get(rev)) {
+            return &mock.url;
+        }
+        client_registry

Evidence

registry_for() only redirects targets that have a revision key present in revision_mocks.
BenchId::revision() returns None for pnpm/static ids, so those targets always remain on
client_registry even when other targets are routed to per-revision mocks.

pacquet/tasks/integrated-benchmark/src/work_env.rs[798-821]
pacquet/tasks/integrated-benchmark/src/work_env.rs[1814-1822]

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 benchmark’s new `registry_for()` routing sends only revision-keyed pacquet/pnpr targets to per-revision registry mocks. pnpm and static targets always stay on `client_registry`, which can make a single benchmark run include targets using different registry backends and link models.
### Issue Context
This is a benchmark-fidelity problem: users may interpret rows as comparable across targets in the same report, but registry/backend differences can dominate (or hide) the actual implementation delta.
### Fix Focus Areas
- pacquet/tasks/integrated-benchmark/src/work_env.rs[203-229]
- pacquet/tasks/integrated-benchmark/src/work_env.rs[808-821]
- pacquet/tasks/integrated-benchmark/src/work_env.rs[1814-1822]
### What to change
- Decide on one of:
- (Preferred) When per-revision mocks are planned, require that the run does not include `pnpm@<rev>` or `--with-pnpm` (fail fast with a clear error), OR
- Route pnpm/static targets through an equivalent mock/proxy so all targets share the same registry backend/link model.
- Add an explicit validation step in `run()` after `plan_revision_mocks()` to enforce the chosen invariant (so mismatched runs fail loudly instead of producing misleading results).

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

Description

Each per-revision mock spawn calls pnpr_command_with_binary(), which unconditionally runs
seed_runtime_storage() and walks the full fixture tree, repeating that work once per revision
mock. With many compared revisions, this adds avoidable benchmark startup overhead (not in the timed
loop, but still impacts total runtime/CI latency).

Code

pacquet/tasks/integrated-benchmark/src/work_env.rs[R538-547]

+        let process = pacquet_registry_mock::pnpr_command_with_binary(
+            &binary,
+            mock_port,
+            Some(&registry.url),
+        )
+        .stdin(Stdio::null())
+        .stdout(stdout)
+        .stderr(stderr)
+        .spawn()
+        .expect("spawn revision mock");

Evidence

start_revision_mock() invokes pnpr_command_with_binary() per revision. That function seeds
runtime storage every time, and seed_runtime_storage() traverses all fixture files using
WalkDir::new(src), making repeated spawns repeat the traversal cost.

pacquet/tasks/integrated-benchmark/src/work_env.rs[518-547]
pacquet/tasks/registry-mock/src/pnpr_command.rs[81-100]
pacquet/tasks/registry-mock/src/seed_storage.rs[16-51]

Agent prompt

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

## Issue description
Per-revision mocks cause `seed_runtime_storage()` to run repeatedly (once per mock spawn). `seed_runtime_storage()` performs a `WalkDir` traversal of the fixture storage each time, which is redundant because the runtime storage location is shared.
### Issue Context
The seeding is idempotent, but it still walks the directory tree on every call.
### Fix Focus Areas
- pacquet/tasks/registry-mock/src/pnpr_command.rs[86-101]
- pacquet/tasks/registry-mock/src/seed_storage.rs[16-52]
- pacquet/tasks/integrated-benchmark/src/work_env.rs[518-547]
### Suggested fix
Add a process-wide guard (e.g., `static OnceLock<()>` / `std::sync::Once`) in `pnpr_command_with_binary()` (or inside `seed_runtime_storage()`) so the expensive traversal is performed at most once per orchestrator process, while preserving correctness for concurrent callers.

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

Description

build_cleanup_command() now repeats the full {remove_targets} list multiple times in the
generated hyperfine --prepare string, substantially increasing the command length and making it
harder to maintain. This also increases the chance of hitting OS command-line length limits when
benchmarking many targets/paths.

Code

pacquet/tasks/integrated-benchmark/src/work_env.rs[R1396-1398]

+    let mut command = format!(
+        "rm -rf {remove_targets} || (sleep 0.5; rm -rf {remove_targets}) || (sleep 1; rm -rf {remove_targets})",
+    );

Evidence

The cleanup command constructs a single long remove_targets string by joining paths across all
bench dirs, then formats a rm -rf ... || ... rm -rf ... string that embeds remove_targets
repeatedly. Since the CLI accepts an arbitrary number of targets, the command length can grow
significantly with larger comparisons.

pacquet/tasks/integrated-benchmark/src/work_env.rs[1388-1399]
pacquet/tasks/integrated-benchmark/src/cli_args.rs[107-111]

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 generated `--prepare` shell command embeds the same long `remove_targets` string multiple times for retry behavior, inflating the final command string.
### Issue Context
- `remove_targets` scales with number of targets (`CliArgs.targets`) and per-scenario cleanup paths.
- hyperfine receives this entire string as an argument, so it contributes directly to argv length.
### Fix Focus Areas
- pacquet/tasks/integrated-benchmark/src/work_env.rs[1388-1408]
- pacquet/tasks/integrated-benchmark/src/cli_args.rs[107-111]
### Suggested fix
Emit a small `cleanup.bash` file into the work env root (or each bench dir) that performs the retry loop internally (without duplicating the long path list in the hyperfine argv), and pass `--prepare "bash cleanup.bash"` to hyperfine.

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