fix: retry pacquet metadata fetches with a shared, config-driven retry policy by suzunn · Pull Request #12029 · pnpm/pnpm

suzunn · 2026-05-28T16:12:39Z

Problem

pacquet metadata fetches made a single registry request, so a transient
network failure or a retryable HTTP response (408/429/5xx) aborted
resolution — even though the tarball path already followed pnpm's retry policy.
pnpm wraps every registry request, metadata and tarball, in @zkochan/retry
under one fetch-retries budget; pacquet only retried tarballs, and the
metadata retry that was added in the first cut duplicated the tarball budget and
ignored the user's config.

Fixes #11841.

Solution

A single retry primitive now lives in pacquet-network and is driven by config
on the install paths, matching pnpm exactly.

One shared RetryOpts. The retry budget (fetch-retries /
-factor / -mintimeout / -maxtimeout) plus its exponential-backoff math
moves into pacquet-network, next to should_retry_status and a generic
send_with_retry(client, url, opts, build_request) -> (guard, Response)
helper. pacquet-tarball re-exports RetryOpts; the metadata fetchers use
send_with_retry. This removes the duplicate MetadataRetryOpts.
No parked sockets/permits during backoff. send_with_retry acquires the
network permit per attempt and drops the response and its guard before each
backoff sleep, so a flapping registry can't pin sockets or hold a concurrency
permit while it waits. On the winning attempt the guard rides out with the
Response so the caller keeps the permit through body streaming.
Config-driven retries on every metadata path. A config-sourced
RetryOpts (via the existing retry_opts_from_config) is threaded through the
resolution verifier, NpmResolver, NamedRegistryResolver, and
PickPackageContext, so the metadata and verify paths honor the user's
fetch-retries* settings instead of a hardcoded default — the same way the
tarball path already did.

Parity bug fixed

Because the verifier and resolver hardcoded the default retry budget, a 5xx
flap on the main resolve/verify paths hung for ~70 s (10 s + 60 s default
backoff) regardless of the user's fetch-retries setting — a user who set
fetch-retries=0 to fail fast would still wait. Driving the budget from config
fixes this; test harnesses use a zero-retry budget so failure-path cases don't
wait out the backoff.

Not pnpr

pnpr is intentionally untouched. Retry is an install-time concern: pnpr's
/v1/install accelerator already retries through pacquet's resolver and tarball
fetcher, while the verdaccio-style registry-proxy path must fail fast (and
serve stale on failure) rather than block a client request behind a 70 s
backoff. The shared RetryOpts re-export keeps the accelerator's existing
tarball-download retry compiling unchanged.

Testing

cargo check / cargo clippy --workspace --all-targets -- --deny warnings
cargo fmt --check, taplo format --check
RUSTDOCFLAGS="-D warnings" cargo doc --no-deps
cargo nextest run — network, tarball, resolving-npm-resolver, pnpr, and the
affected package-manager tests all pass with no slow cases.

Written by an agent (Claude Code, claude-opus-4-8).

coderabbitai · 2026-05-28T16:12:59Z

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (1)

Cargo.lock is excluded by !**/*.lock

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3a76b3c4-19f1-48a7-b0b2-9e183585e53f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

📝 Walkthrough

Walkthrough

This PR implements a retry mechanism for npm registry metadata fetches to handle transient network errors and retryable status codes. It adds MetadataRetryOpts with configurable backoff, refactors both full and cached metadata paths to use a shared retry helper send_metadata_request_with_retry, threads retry configuration through all metadata fetch call sites, and adds test coverage for the retry behavior.

Changes

npm metadata fetch retry mechanism

Layer / File(s)	Summary
Retry mechanism foundation `src/fetch_full_metadata.rs`	Introduces `MetadataRetryOpts` struct, exponential backoff helpers (`delay_for`, `should_retry_status`), and `send_metadata_request_with_retry` function that retries GET requests on transient network errors and status codes 408/429/5xx with bounded exponential backoff; adds `retry_opts` field to `FetchFullMetadataOptions`.
Full metadata fetch refactored for retry `src/fetch_full_metadata.rs`	Refactors `fetch_full_metadata` to call `send_metadata_request_with_retry` with a closure that builds the request (including `Accept`, optional `Authorization`, and optional conditional headers); returns `FetchFullMetadataOutcome::NotModified` on 304 response.
Cached metadata fetch integration with retry `src/fetch_full_metadata_cached.rs`	Adds `retry_opts: MetadataRetryOpts` field to `FetchFullMetadataCachedOptions`, updates imports to include retry helpers, and refactors request execution through `send_metadata_request_with_retry` with a closure that preserves cache conditionals and authorization headers.
Test support and retry validation `src/fetch_full_metadata/tests.rs`	Adds `no_retry_opts()` and `fast_retry_opts()` helpers and new async test `fetch_full_metadata_retries_transient_status` that verifies the retry loop succeeds when a 503 is followed by a 200 response; imports `std::time::Duration`.
Full metadata tests updated `src/fetch_full_metadata/tests.rs`	Updates existing full-metadata tests (auth, 5xx error, scoped-name encoding, decode failure, 304 scenarios) to wire `retry_opts: no_retry_opts()` so those tests execute without retries.
Cached metadata tests updated `src/fetch_full_metadata_cached/tests.rs`	Imports `MetadataRetryOpts`, adds `no_retry_opts()` helper, and updates all cache-related tests (cold cache, warm cache, stale cache refresh, no cache_dir, read-only cache_dir) to include `retry_opts: no_retry_opts()`.
Client integration and call sites `src/create_npm_resolution_verifier.rs`, `src/pick_package.rs`	Threads `retry_opts: Default::default()` through metadata fetch call sites in resolution verifier (abbreviated and full metadata paths) and package picker (cached metadata fetch and full metadata upgrade refetch).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

pnpm/pnpm#11931: Updates the npm resolution verifier code paths that fetch metadata in create_npm_resolution_verifier.rs, which this PR threads retry configuration through.

Suggested reviewers

zkochan

Poem

🐰 A hop through the retries so brave,
Where networks once failed, now are saved,
Exponential backoff, delays so sweet,
Transient errors bow in defeat!
The metadata flows without flinch,
No install shall fail by a inch. 🎯

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR implements all key objectives from issue `#11841`: adds retry logic with exponential backoff, restricts retries to transient errors (network errors and 5xx/408/429 responses), applies to both full and cached metadata paths, and includes test coverage.
Out of Scope Changes check	✅ Passed	All changes are directly related to adding retry logic for metadata fetches. No unrelated modifications to pool_idle_timeout, prefetch wiring, or other out-of-scope areas are present.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The title clearly and specifically describes the main change: adding a shared, config-driven retry policy for metadata fetches, which is the core objective of the changeset.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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

qodo-free-for-open-source-projects · 2026-05-28T16:13:18Z

Review Summary by Qodo

Add retry logic for pacquet metadata fetches with exponential backoff

🐞 Bug fix ✨ Enhancement

Walkthroughs

Description

• Add retry logic for pacquet metadata fetches with exponential backoff
  - Retries on transient errors (408, 429, 5xx) and transport failures
  - Uses pnpm-compatible defaults: 2 retries, 10x factor, 10-60s delays
• Refactor metadata request handling into shared send_metadata_request_with_retry helper
  - Preserves 304 Not Modified cache path and fail-fast for non-retryable 4xx
• Update all metadata fetch call sites to pass retry options
• Add comprehensive test coverage for retry behavior with mock registry

Diagram

flowchart LR
  A["Metadata Request"] --> B["send_metadata_request_with_retry"]
  B --> C{"Retryable Status?"}
  C -->|408/429/5xx| D["Exponential Backoff"]
  D --> B
  C -->|304| E["Return Cached"]
  C -->|Other| F["Return Response"]
  B --> G["fetch_full_metadata"]
  B --> H["fetch_full_metadata_cached"]

File Changes

1. pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs ✨ Enhancement +106/-19

Implement metadata request retry helper with exponential backoff

• Add MetadataRetryOpts struct with configurable retry parameters (retries, factor, min/max
 timeouts)
• Implement delay_for() method for exponential backoff calculation
• Add should_retry_status() helper to identify retryable HTTP status codes (408, 429, 5xx)
• Create send_metadata_request_with_retry() async function that handles retry logic with
 exponential backoff
• Refactor fetch_full_metadata() to use the new retry helper instead of direct request sending
• Add retry_opts field to FetchFullMetadataOptions struct

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

2. pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata/tests.rs 🧪 Tests +69/-1

Add retry test coverage and configure test retry options

• Add no_retry_opts() helper function for tests that verify single-request behavior
• Add fast_retry_opts() helper with minimal delays for testing retry paths
• Add new test fetch_full_metadata_retries_transient_status() that verifies 503 retry and eventual
 success
• Update all existing tests to pass retry_opts: no_retry_opts() to preserve original test
 semantics

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

3. pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached.rs ✨ Enhancement +20/-20

Integrate retry logic into cached metadata fetcher

• Import MetadataRetryOpts and send_metadata_request_with_retry from parent module
• Add retry_opts field to FetchFullMetadataCachedOptions struct
• Refactor fetch_full_metadata_cached() to use the new retry helper instead of direct request
 sending
• Preserve 304 Not Modified handling and cache fallback behavior

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

View more (3)

4. pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached/tests.rs 🧪 Tests +10/-1

Configure cached metadata tests with no-retry options
• Add no_retry_opts() helper function for tests
• Update all test cases to pass retry_opts: no_retry_opts() to maintain single-request test
 behavior
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached/tests.rs

5. pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs ✨ Enhancement +2/-0

Enable retry options in resolution verifier
• Add retry_opts: Default::default() to two FetchFullMetadataCachedOptions instantiations
• Enables retry logic for metadata fetches in the resolution verifier
pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs

6. pacquet/crates/resolving-npm-resolver/src/pick_package.rs ✨ Enhancement +2/-0

Enable retry options in package selection logic

• Add retry_opts: Default::default() to FetchFullMetadataCachedOptions in pick_package()
 function
• Add retry_opts: Default::default() to FetchFullMetadataOptions in
 maybe_upgrade_abbreviated_meta_for_release_age() function
• Enables retry logic for metadata fetches in package selection and upgrade paths

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

qodo-free-for-open-source-projects · 2026-05-28T16:13:20Z

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (1)

1. Sleep holds Response open 🐞 Bug ☼ Reliability

Description

In send_metadata_request_with_retry, retryable HTTP responses are kept alive across
tokio::time::sleep, so the underlying connection/body is not released until after the backoff
completes. This can hold idle sockets open during outages and prevents timely connection
reuse/cleanup between attempts.

Code

pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs[R83-101]

Evidence
The retry arm sleeps while response is still alive (not dropped yet), meaning the HTTP
connection/body can remain open during the backoff window.
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs[76-101]

Agent prompt

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

### Issue description
`send_metadata_request_with_retry()` awaits the backoff sleep while the retryable `Response` is still in-scope, so the response (and underlying connection) stays alive during the sleep.

### Issue Context
This happens in the `Ok(response) if should_retry_status(...)` match arm; the `response` value is not dropped until the arm completes, but the sleep occurs before that.

### Fix Focus Areas
- pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs[76-125]

### Suggested change
- Explicitly release the response **before** sleeping (e.g., `drop(response)`), and optionally drain small error bodies (similar to the tarball path) so keep-alive connections can be reused when possible.
- Ensure the retry loop does not retain any connection/body across the backoff delay.

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

2. Backoff parks throttle permit 🐞 Bug ➹ Performance

Description

fetch_full_metadata and fetch_full_metadata_cached acquire a ThrottledClientGuard once and reuse it
across all retry attempts, so the global network semaphore permit is held during backoff sleeps.
This can block other concurrent network operations for up to ~70s per failing request with default
retry settings.

Code

pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs[R201-215]

Evidence
The guard returned by acquire_for_url() holds a semaphore permit until dropped, but the retry call
sites acquire the guard once and then sleep inside the retry helper, keeping the permit held while
waiting. The tarball code documents the intended pattern of acquiring permits inside an attempt to
avoid holding permits during backoff.
pacquet/crates/network/src/lib.rs[65-83]
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs[199-215]
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached.rs[125-143]
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs[83-101]
pacquet/crates/tarball/src/lib.rs[1404-1406]

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 retry implementation acquires `ThrottledClientGuard` outside the retry loop and keeps it alive across retries/backoff, which “parks” a global concurrency permit while no network I/O is happening.

### Issue Context
`ThrottledClientGuard` holds a semaphore permit until dropped. The tarball codebase explicitly documents acquiring permits inside attempts to avoid holding permits during retry sleeps.

### Fix Focus Areas
- pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs[189-230]
- pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached.rs[125-143]
- pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs[76-125]

### Suggested change
Refactor the retry helper/call sites so that:
- A new `ThrottledClientGuard` is acquired **per attempt**.
- The guard is held for `send + body consume` on successful attempts.
- The guard (and any `Response`) is dropped **before** the backoff sleep.

One workable shape is to have the retry helper return an owned result (e.g., `status/headers/body`) or return `(ThrottledClientGuard, Response)` on success so the caller can keep the permit until the body is consumed, while still allowing failed attempts to release the permit before sleeping.

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

3. MetadataRetryOpts duplicates retry logic 📘 Rule violation ⚙ Maintainability

Description

MetadataRetryOpts::delay_for() reimplements the same backoff algorithm and defaults already
present in pacquet_tarball::RetryOpts, creating duplicated retry policy logic across crates. This
increases maintenance risk if pnpm parity values change and only one copy is updated.

Code

pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs[R40-67]

Evidence
PR Compliance ID 4 requires reusing or extracting shared utilities instead of duplicating
non-trivial logic. The PR adds a new retry options struct and backoff calculation in the resolver
crate, while the tarball crate already contains an equivalent retry configuration and delay_for()
implementation for the same pnpm retry policy.
AGENTS.md: Avoid code duplication by reusing or extracting shared utilities
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs[40-67]
pacquet/crates/tarball/src/lib.rs[77-140]

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 resolver crate introduces `MetadataRetryOpts` + `delay_for()` even though an equivalent retry options type and backoff calculation already exist in `pacquet_tarball::RetryOpts`. This duplicates retry policy logic and defaults.

## Issue Context
Because both metadata fetches and tarball fetches aim to mirror pnpm's retry behavior, having two separate implementations makes future parity updates error-prone.

## Fix Focus Areas
- pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs[40-67]
- pacquet/crates/tarball/src/lib.rs[77-140]

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

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs`:
- Around line 85-100: The retry branch in fetch_full_metadata.rs keeps the HTTP
Response alive across tokio::time::sleep, holding sockets; before sleeping in
the branch that matches Ok(response) if should_retry_status(...) { ... },
explicitly drop the response (call drop(response)) or otherwise release its
body/connection before calling tokio::time::sleep(delay).await so the socket is
returned to the pool during backoff; keep the rest of the logic (status, delay,
tracing::warn, attempt increment) unchanged.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 34b9c665-c0c4-4309-acf8-24de18cfc93d

📥 Commits

Reviewing files that changed from the base of the PR and between 7ecaf3d and 4b766a2.

📒 Files selected for processing (6)

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

📜 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). (5)

GitHub Check: Run benchmark on ubuntu-latest
GitHub Check: Lint and Test (macos-latest)
GitHub Check: Lint and Test (windows-latest)
GitHub Check: Code Coverage
GitHub Check: Compile & Lint

🧰 Additional context used

📓 Path-based instructions (1)

pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: When porting a function that fires pnpm:<channel> events through globalLogger, logger.debug(), or streamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plain String or &str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only via TryFrom<String> and/or FromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallible From<String> (and From<&str> when convenient).
If upstream occasionally constructs without validation, expose from_str_unchecked as an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use #[serde(try_from = "String")] for deserialization and #[serde(into = "String")] for serialization.
Use #[derive(derive_more::From)] and #[derive(derive_more::Into)] for mechanical conversion impls. Fall back to manual impl only when conversion needs custom logic.
String-literal unions should become enums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Path over &PathBuf, &str over &String) when it doesn't force extra copies.
Prefer Arc::clone(&x) / Rc::clone(&x) over x.clone() for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Write use super::{Foo, bar} instead of use super::*;. Two forms stay allowed: external-crate preludes like use rayon::prelude::*; and root-of-module re-...

Files:

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

🧠 Learnings (3)

📚 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/resolving-npm-resolver/src/pick_package.rs
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached/tests.rs
pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata/tests.rs
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached.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/resolving-npm-resolver/src/pick_package.rs
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached/tests.rs
pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata/tests.rs
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached.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/resolving-npm-resolver/src/pick_package.rs
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached/tests.rs
pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata/tests.rs
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs
pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata_cached.rs

codecov-commenter · 2026-05-28T16:26:31Z

Codecov Report

❌ Patch coverage is 94.04762% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.61%. Comparing base (1c73e83) to head (bf402b3).

Files with missing lines	Patch %	Lines
.../resolving-npm-resolver/src/fetch_full_metadata.rs	93.84%	4 Missing ⚠️
...ing-npm-resolver/src/fetch_full_metadata_cached.rs	93.33%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #12029   +/-   ##
=======================================
  Coverage   87.60%   87.61%           
=======================================
  Files         267      267           
  Lines       30661    30709   +48     
=======================================
+ Hits        26860    26905   +45     
- Misses       3801     3804    +3

☔ View full report in Codecov by Sentry.
📢 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 · 2026-05-28T16:37:16Z

Integrated-Benchmark Report (Linux)

Scenario: Isolated linker: fresh restore, cold cache + cold store

Command	Mean [s]	Min [s]	Max [s]	Relative
`pacquet@HEAD`	1.701 ± 0.040	1.660	1.769	1.00
`pacquet@main`	1.794 ± 0.211	1.666	2.378	1.05 ± 0.13

BENCHMARK_REPORT.json

{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 1.7008565663800002,
      "stddev": 0.04048105207355616,
      "median": 1.68733072318,
      "user": 2.79752924,
      "system": 2.0523605799999998,
      "min": 1.65988682968,
      "max": 1.76853877568,
      "times": [
        1.72587487968,
        1.67216560168,
        1.66581042368,
        1.71199931568,
        1.76853877568,
        1.67102130268,
        1.76214369668,
        1.66862899368,
        1.70249584468,
        1.65988682968
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 1.7940758516800002,
      "stddev": 0.21079636886243083,
      "median": 1.74607191918,
      "user": 2.77313004,
      "system": 2.0470192799999998,
      "min": 1.66572127968,
      "max": 2.37832811068,
      "times": [
        2.37832811068,
        1.76127987868,
        1.7496689166800001,
        1.66572127968,
        1.7852488016799999,
        1.67206970168,
        1.68756941368,
        1.69422044068,
        1.74247492168,
        1.80417705168
      ]
    }
  ]
}

Scenario: Isolated linker: fresh restore, hot cache + hot store

Command	Mean [ms]	Min [ms]	Max [ms]	Relative
`pacquet@HEAD`	506.8 ± 23.8	491.3	572.2	1.00
`pacquet@main`	509.4 ± 18.9	491.2	557.3	1.01 ± 0.06

BENCHMARK_REPORT.json

{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.50676252356,
      "stddev": 0.02377596452403384,
      "median": 0.49770013116,
      "user": 0.3804862799999999,
      "system": 0.80807026,
      "min": 0.49132852616,
      "max": 0.57222759516,
      "times": [
        0.57222759516,
        0.49668310216,
        0.50928405116,
        0.50242525716,
        0.49588329716,
        0.49474445416,
        0.49860687816000004,
        0.49132852616,
        0.49679338416,
        0.50964869016
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.5094340914600001,
      "stddev": 0.01890631277828608,
      "median": 0.50728175316,
      "user": 0.37489607999999996,
      "system": 0.8132736599999999,
      "min": 0.49124066016,
      "max": 0.55727800516,
      "times": [
        0.55727800516,
        0.50798754516,
        0.50857796516,
        0.49574118616,
        0.51428841016,
        0.50657596116,
        0.49810742516,
        0.49124066016,
        0.49637924716000004,
        0.51816450916
      ]
    }
  ]
}

Scenario: Isolated linker: fresh install, cold cache + cold store

Command	Mean [s]	Min [s]	Max [s]	Relative
`pacquet@HEAD`	2.136 ± 0.030	2.092	2.188	1.00
`pacquet@main`	2.146 ± 0.048	2.090	2.242	1.00 ± 0.03

BENCHMARK_REPORT.json

{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 2.13610412836,
      "stddev": 0.030450503527684827,
      "median": 2.1413204393599994,
      "user": 4.13886742,
      "system": 1.9874393400000003,
      "min": 2.09161885286,
      "max": 2.1884890118599998,
      "times": [
        2.1499522718599997,
        2.1430291098599996,
        2.1593694168599997,
        2.1339080998599997,
        2.1036169648599996,
        2.09161885286,
        2.15358775186,
        2.0978580348599998,
        2.1884890118599998,
        2.1396117688599996
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 2.14563044846,
      "stddev": 0.04797184534212599,
      "median": 2.1359132708599997,
      "user": 4.13206152,
      "system": 2.02120714,
      "min": 2.08951863786,
      "max": 2.24220965386,
      "times": [
        2.1359675988599998,
        2.08951863786,
        2.1358589428599997,
        2.11656991886,
        2.2161868388599997,
        2.24220965386,
        2.1474125148599996,
        2.13987467786,
        2.13173744586,
        2.1009682548599997
      ]
    }
  ]
}

Scenario: Isolated linker: fresh install, hot cache + hot store

Command	Mean [s]	Min [s]	Max [s]	Relative
`pacquet@HEAD`	1.370 ± 0.019	1.349	1.412	1.00
`pacquet@main`	1.384 ± 0.032	1.344	1.466	1.01 ± 0.03

BENCHMARK_REPORT.json

{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 1.3696735833200002,
      "stddev": 0.019117814286176447,
      "median": 1.3695060262200003,
      "user": 1.79752686,
      "system": 1.1512684199999998,
      "min": 1.3486341452200001,
      "max": 1.41160602022,
      "times": [
        1.38575341922,
        1.3562154212200002,
        1.3699812932200002,
        1.3486341452200001,
        1.35263347922,
        1.37278896322,
        1.41160602022,
        1.3690307592200002,
        1.35269014922,
        1.37740218322
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 1.3844166993199998,
      "stddev": 0.032273480836876096,
      "median": 1.3785970602200002,
      "user": 1.8052295599999997,
      "system": 1.15744412,
      "min": 1.34358925022,
      "max": 1.4657012202200002,
      "times": [
        1.36478212522,
        1.3696866872200002,
        1.37408545122,
        1.37021090022,
        1.34358925022,
        1.38310866922,
        1.39690142522,
        1.3864498952200002,
        1.4657012202200002,
        1.38965136922
      ]
    }
  ]
}

github-actions · 2026-05-28T16:37:18Z

Bencher Report

Branch	pr/12029
Testbed	pacquet

Click to view all benchmark results

Benchmark	Latency	Benchmark Result milliseconds (ms) (Result Δ%)	Upper Boundary milliseconds (ms) (Limit %)
isolated-linker.fresh-install.cold-cache.cold-store	📈 view plot 🚷 view threshold	2,136.10 ms (-9.17%) Baseline: 2,351.68 ms	2,822.02 ms (75.69%)
isolated-linker.fresh-install.hot-cache.hot-store	📈 view plot 🚷 view threshold	1,369.67 ms (-10.23%) Baseline: 1,525.81 ms	1,830.97 ms (74.81%)
isolated-linker.fresh-restore.cold-cache.cold-store	📈 view plot 🚷 view threshold	1,700.86 ms (-17.69%) Baseline: 2,066.37 ms	2,479.64 ms (68.59%)
isolated-linker.fresh-restore.hot-cache.hot-store	📈 view plot 🚷 view threshold	506.76 ms (-22.65%) Baseline: 655.16 ms	786.20 ms (64.46%)

🐰 View full continuous benchmarking report in Bencher

…l paths The metadata retry helper duplicated tarball's RetryOpts and hardcoded the default budget at every call site. Consolidate the retry primitive in pacquet-network and drive it from config on the install paths, matching pnpm — which wraps every registry request in @zkochan/retry under one fetch-retries budget. - Move RetryOpts (budget + exponential-backoff math) into pacquet-network alongside should_retry_status and a generic send_with_retry helper that returns (guard, Response). pacquet-tarball re-exports RetryOpts; the metadata fetchers use send_with_retry. Deletes the duplicate MetadataRetryOpts. - send_with_retry acquires the network permit per attempt and drops the response and guard before each backoff sleep, so a flapping registry can't pin sockets or park a concurrency permit during the wait. - Thread a config-sourced RetryOpts (via retry_opts_from_config) through the resolution verifier, NpmResolver, NamedRegistryResolver, and PickPackageContext, so the metadata and verify paths honor fetch-retries / fetch-retry-factor / fetch-retry-mintimeout / fetch-retry-maxtimeout instead of a hardcoded default. This also fixes a parity bug where a 5xx flap hung for 70s (10s + 60s default backoff) regardless of the user's fetch-retries setting. Test harnesses use a zero-retry budget so failure-path cases don't wait out the default backoff. pnpr is intentionally untouched: retry belongs to the install path (its /v1/install accelerator already retries through pacquet's resolver and tarball fetcher), while the verdaccio-style registry-proxy path must fail fast.

welcome · 2026-06-02T22:42:40Z

Congrats on merging your first pull request! 🎉🎉🎉

Copilot AI review requested due to automatic review settings May 28, 2026 16:12

suzunn requested a review from zkochan as a code owner May 28, 2026 16:12

coderabbitai Bot requested changes May 28, 2026

View reviewed changes

Comment thread pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs Outdated

qodo-free-for-open-source-projects Bot reviewed May 28, 2026

View reviewed changes

Comment thread pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs Outdated

Comment thread pacquet/crates/resolving-npm-resolver/src/fetch_full_metadata.rs Outdated

Copilot started reviewing on behalf of suzunn May 28, 2026 16:28 View session

fix: retry pacquet metadata fetches

bf402b3

zkochan force-pushed the fix-pacquet-metadata-retry branch from 4b766a2 to bf402b3 Compare June 2, 2026 20:44

zkochan changed the title ~~fix: retry pacquet metadata fetches~~ fix: retry pacquet metadata fetches with a shared, config-driven retry policy Jun 2, 2026

zkochan force-pushed the fix-pacquet-metadata-retry branch from 7169a3b to 8c8eca5 Compare June 2, 2026 22:26

zkochan force-pushed the fix-pacquet-metadata-retry branch from 8c8eca5 to 4f98ca1 Compare June 2, 2026 22:30

coderabbitai Bot approved these changes Jun 2, 2026

View reviewed changes

zkochan approved these changes Jun 2, 2026

View reviewed changes

zkochan merged commit 8bc25f3 into pnpm:main Jun 2, 2026
22 of 23 checks passed

coderabbitai Bot mentioned this pull request Jun 10, 2026

perf(pacquet): parse packuments off the reactor and release the network permit before CPU work #12318

Merged

coderabbitai Bot mentioned this pull request Jun 25, 2026

fix(network): retry metadata fetches that fail while reading the body #12655

Merged

2 tasks

Uh oh!

Uh oh!

Conversation

suzunn commented May 28, 2026 • edited by zkochan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Parity bug fixed

Not pnpr

Testing

Uh oh!

coderabbitai Bot commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

qodo-free-for-open-source-projects Bot commented May 28, 2026

Review Summary by Qodo

Walkthroughs

File Changes

Uh oh!

qodo-free-for-open-source-projects Bot commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Integrated-Benchmark Report (Linux)

Scenario: Isolated linker: fresh restore, cold cache + cold store

Scenario: Isolated linker: fresh restore, hot cache + hot store

Scenario: Isolated linker: fresh install, cold cache + cold store

Scenario: Isolated linker: fresh install, hot cache + hot store

Uh oh!

github-actions Bot commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bencher Report

Uh oh!

Uh oh!

welcome Bot commented Jun 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

suzunn commented May 28, 2026 •

edited by zkochan

Loading

coderabbitai Bot commented May 28, 2026 •

edited

Loading

qodo-free-for-open-source-projects Bot commented May 28, 2026 •

edited

Loading

codecov-commenter commented May 28, 2026 •

edited

Loading

github-actions Bot commented May 28, 2026 •

edited

Loading

github-actions Bot commented May 28, 2026 •

edited

Loading