Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 13499a3f-8ae1-4ff8-8ff3-42269cfee6d1

Reviewing files that changed from the base of the PR and between 2bb1fe7 and 77d6de2.

• pnpr/crates/pnpr/src/config.rs
• pnpr/crates/pnpr/src/config/tests.rs
• pnpr/crates/pnpr/src/error.rs
• pnpr/crates/pnpr/src/server.rs
• pnpr/crates/pnpr/src/upstream.rs
• pnpr/crates/pnpr/src/upstream/tests.rs
• pnpr/crates/pnpr/tests/server.rs

• Create stacked PR
• Commit on current branch

• Create PR with unit tests
• Commit unit tests in branch feat/pnpr-per-uplink-settings

Description

CircuitBreaker::is_available() returns true for all requests once fail_timeout has elapsed, so many
concurrent requests can all re-enter the uplink at once instead of permitting a single half-open
probe. This can overload a recovering upstream and defeats the breaker’s purpose under load.

Code

pnpr/crates/pnpr/src/upstream.rs[R93-102]

+    fn is_available(&self) -> bool {
+        if self.max_fails == 0 || self.failed_requests.load(Ordering::Relaxed) < self.max_fails {
+            return true;
+        }
+        let last_failure = *self.last_failure.lock().expect("circuit breaker poisoned");
+        match last_failure {
+            Some(at) => at.elapsed() >= self.fail_timeout,
+            None => true,
+        }
+    }

Evidence

The breaker’s availability check only tests whether the last failure is older than fail_timeout and
does not reserve a single probe. Because Upstream objects are stored in shared state, concurrent
requests will evaluate is_available() at the same time and all proceed once the cooldown threshold
is crossed.

pnpr/crates/pnpr/src/upstream.rs[89-103]
pnpr/crates/pnpr/src/server.rs[157-170]
pnpr/crates/pnpr/src/server.rs[1756-1764]

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 circuit breaker currently re-opens the uplink to *all* callers once `fail_timeout` has elapsed, which can cause a thundering herd of concurrent retries (probe stampede). The intended behavior is to allow only a single probe request after the cooldown, keeping others short-circuited until that probe succeeds (reset) or fails (re-open cooldown).
### Issue Context
`Upstream` instances are stored in shared app state and accessed concurrently by many requests, so `CircuitBreaker::is_available()` must enforce half-open exclusivity.
### Fix Focus Areas
- pnpr/crates/pnpr/src/upstream.rs[66-113]
### Implementation notes
- Introduce a “half-open probe permit” (e.g., `AtomicBool probe_in_flight` or a mutex-protected state).
- When `failed_requests >= max_fails` and cooldown elapsed, allow exactly one caller to acquire the permit (via `compare_exchange`) and proceed; others should return `UpstreamUnavailable`.
- On `record_success()`, clear failures + timestamp and release the permit.
- On `record_failure()`, update failure state and release the permit while restarting cooldown.

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

Description

The new circuit breaker uses Mutex::lock().expect("circuit breaker poisoned"), which can panic in
the request path if the mutex is poisoned. A panic here can crash request handling and jeopardize
core packument/tarball serving availability.

Code

pnpr/crates/pnpr/src/upstream.rs[R93-112]

+    fn is_available(&self) -> bool {
+        if self.max_fails == 0 || self.failed_requests.load(Ordering::Relaxed) < self.max_fails {
+            return true;
+        }
+        let last_failure = *self.last_failure.lock().expect("circuit breaker poisoned");
+        match last_failure {
+            Some(at) => at.elapsed() >= self.fail_timeout,
+            None => true,
+        }
+    }
+
+    fn record_success(&self) {
+        self.failed_requests.store(0, Ordering::Relaxed);
+        *self.last_failure.lock().expect("circuit breaker poisoned") = None;
+    }
+
+    fn record_failure(&self) {
+        self.failed_requests.fetch_add(1, Ordering::Relaxed);
+        *self.last_failure.lock().expect("circuit breaker poisoned") = Some(Instant::now());
+    }

Evidence

PR Compliance ID 12 requires maintaining core registry behaviors (including serving
packuments/tarballs) without regressions. The new circuit breaker introduces panicking expect()
calls on a mutex lock in the request path, which can crash processing and break those core
behaviors.

Registry core behaviors maintained: packument/tarball serving, local search, atomic writes, on-disk layout, integration tests
pnpr/crates/pnpr/src/upstream.rs[93-112]

Agent prompt

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

## Issue description
`CircuitBreaker` uses `expect()` on `Mutex::lock()`, which will panic if the mutex is poisoned.
## Issue Context
This code runs on hot request paths (`is_available`, `record_success`, `record_failure`). A poisoned mutex should not take down the registry; it should recover (e.g., use `into_inner()`), reset breaker state, and continue.
## Fix Focus Areas
- pnpr/crates/pnpr/src/upstream.rs[93-112]

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

Description

record_failure() increments failed_requests before setting last_failure, while is_available() treats
last_failure == None as available even when failed_requests >= max_fails. Under concurrency this can
briefly allow traffic through an already-tripped breaker, reducing short-circuit effectiveness
during failures.

Code

pnpr/crates/pnpr/src/upstream.rs[R93-112]

+    fn is_available(&self) -> bool {
+        if self.max_fails == 0 || self.failed_requests.load(Ordering::Relaxed) < self.max_fails {
+            return true;
+        }
+        let last_failure = *self.last_failure.lock().expect("circuit breaker poisoned");
+        match last_failure {
+            Some(at) => at.elapsed() >= self.fail_timeout,
+            None => true,
+        }
+    }
+
+    fn record_success(&self) {
+        self.failed_requests.store(0, Ordering::Relaxed);
+        *self.last_failure.lock().expect("circuit breaker poisoned") = None;
+    }
+
+    fn record_failure(&self) {
+        self.failed_requests.fetch_add(1, Ordering::Relaxed);
+        *self.last_failure.lock().expect("circuit breaker poisoned") = Some(Instant::now());
+    }

Evidence

The code explicitly returns true when last_failure is None, and record_failure() performs the
atomic increment before setting last_failure, creating a window where the breaker should be open
but is_available() can still return true.

pnpr/crates/pnpr/src/upstream.rs[93-112]

Agent prompt

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

## Issue description
`CircuitBreaker` splits its state across an atomic counter and a mutex-protected timestamp. Because `record_failure()` updates the counter before the timestamp, and `is_available()` treats a missing timestamp (`None`) as available, concurrent interleavings can observe `failed_requests >= max_fails` but `last_failure == None` and incorrectly allow a request.
### Issue Context
This shows up under concurrency and weakens the breaker’s protection precisely when the uplink is failing.
### Fix Focus Areas
- pnpr/crates/pnpr/src/upstream.rs[93-112]
### Implementation notes
Choose one:
- Treat `last_failure == None` as **unavailable** when `failed_requests >= max_fails` (safer default), and/or
- Update `last_failure` before incrementing `failed_requests`, and/or
- Collapse breaker state into a single lock-protected struct so threshold checks + timestamp updates are consistent.
Also consider eliminating `expect("circuit breaker poisoned")` by recovering from poisoning (`into_inner()`) to avoid panics cascading into process instability.

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

Description

The circuit breaker sets probe_in_flight = true in try_acquire(), but if an admitted request is
cancelled/dropped before any record_*() path runs, the flag never clears and all subsequent
requests will short-circuit indefinitely with UpstreamUnavailable (503). This is reachable in
async request handling (client disconnects/cancellation) because ensure_available() runs before
awaited work in fetch_packument/fetch_tarball_response.

Code

pnpr/crates/pnpr/src/upstream.rs[R124-129]

+        let cooled_down = state.last_failure.is_none_or(|at| at.elapsed() >= self.fail_timeout);
+        if !cooled_down || state.probe_in_flight {
+            return false;
+        }
+        state.probe_in_flight = true;
+        true

Evidence

try_acquire() blocks callers when probe_in_flight is set, but that flag is only cleared by
explicit record calls; both fetch methods call ensure_available() before awaited operations, so
cancellation between admission and recording will leave the breaker stuck.

pnpr/crates/pnpr/src/upstream.rs[110-130]
pnpr/crates/pnpr/src/upstream.rs[245-308]
pnpr/crates/pnpr/src/upstream.rs[321-338]

Agent prompt

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

### Issue description
`CircuitBreaker::try_acquire()` marks a half-open probe as `probe_in_flight = true`, but that flag is only cleared by explicit `record_success`/`record_failure`/`record_neutral` calls. If the async task is cancelled/dropped after admission (e.g., client disconnect) before those methods run, the probe slot leaks and the breaker stays permanently “probe in flight”, short-circuiting all future requests.

### Issue Context
This can happen because `ensure_available()` is called before the first `.await` in both fetch paths, so cancellation can occur at any later await point.

### Fix Focus Areas
- pnpr/crates/pnpr/src/upstream.rs[110-149]
- pnpr/crates/pnpr/src/upstream.rs[245-338]

### Suggested fix approach
- Change `try_acquire()` to return an RAII “permit” (e.g., `BreakerPermit`) when it sets `probe_in_flight = true`.
- Implement `Drop` for the permit to clear `probe_in_flight` (equivalent to `record_neutral`) if the request exits early/cancels.
- Thread the permit through `fetch_packument` / `fetch_tarball_response` so that success/failure paths “commit” the permit (preventing the Drop handler from running twice) and update counters appropriately.

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

Description

Upstream::checked() records a circuit-breaker failure for any non-2xx response (except 404 handled
earlier), so repeated upstream 401/403/429 responses can open the breaker and then all subsequent
requests to that uplink short-circuit to UpstreamUnavailable (503), enabling per-uplink denial of
service by triggering expected auth/rate-limit errors.

Code

pnpr/crates/pnpr/src/upstream.rs[R302-314]

+    /// Pass a successful response through; map any non-success status to
+    /// [`RegistryError::UpstreamStatus`] and count it against the breaker
+    /// (a 5xx/4xx-other upstream is a failure; a 404 is handled by the
+    /// callers before reaching here).
+    async fn checked(&self, response: reqwest::Response, url: &str) -> Result<reqwest::Response> {
+        if response.status().is_success() {
+            return Ok(response);
+        }
+        self.breaker.record_failure();
+        let status = response.status();
+        let body = response.text().await.unwrap_or_default();
+        Err(RegistryError::UpstreamStatus { url: url.to_string(), status: status.as_u16(), body })
   }
-    let body = response.text().await.unwrap_or_default();
-    Err(RegistryError::UpstreamStatus { url: url.to_string(), status: status.as_u16(), body })

Evidence

checked() counts any non-2xx response as a breaker failure; once failures reach the threshold,
ensure_available() returns UpstreamUnavailable before sending requests, and that error maps to
HTTP 503.

pnpr/crates/pnpr/src/upstream.rs[302-314]
pnpr/crates/pnpr/src/upstream.rs[283-291]
pnpr/crates/pnpr/src/upstream.rs[89-102]
pnpr/crates/pnpr/src/error.rs[173-186]

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 circuit breaker currently increments its failure counter for **all** non-success upstream HTTP statuses (via `Upstream::checked()`), which means repeated upstream 401/403/429 responses can trip the breaker and cause subsequent requests to be short-circuited with `UpstreamUnavailable` (503). This makes it possible to take an uplink “down” without any actual upstream outage.
### Issue Context
- `ensure_available()` short-circuits requests when the breaker is open.
- `checked()` currently calls `record_failure()` for any non-2xx response, and its comment explicitly treats `4xx` (non-404) as failures.
- `UpstreamUnavailable` maps to HTTP 503.
### Fix Focus Areas
- Update breaker failure classification to only count **transport errors and server-side errors (5xx)** as circuit-breaker failures (optionally include 429 if you intentionally want rate-limits to open the circuit), while leaving other 4xx responses as non-breaker failures.
- Add/adjust unit tests to assert that repeated upstream 401/403 do **not** open the breaker and do not cause `UpstreamUnavailable` short-circuiting.
- pnpr/crates/pnpr/src/upstream.rs[283-314]
- pnpr/crates/pnpr/src/upstream/tests.rs[446-509]

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

Description

parse_interval converts attacker-controlled config strings to Duration via
Duration::from_secs_f64 without checking the representable upper bound, which can panic on
extremely large but finite values. This can crash pnpr during config load, preventing safe schema
validation diagnostics.

Code

pnpr/crates/pnpr/src/config.rs[R571-606]

+    // A bare number is seconds, matching verdaccio (`interval * 1000` ms).
+    if let Ok(seconds) = raw.parse::<f64>() {
+        return (seconds.is_finite() && seconds >= 0.0).then(|| Duration::from_secs_f64(seconds));
+    }
+    let mut total_seconds = 0f64;
+    let bytes = raw.as_bytes();
+    let mut index = 0;
+    while index < bytes.len() {
+        if bytes[index].is_ascii_whitespace() {
+            index += 1;
+            continue;
+        }
+        let number_start = index;
+        while index < bytes.len() && (bytes[index].is_ascii_digit() || bytes[index] == b'.') {
+            index += 1;
+        }
+        if index == number_start {
+            return None;
+        }
+        let number: f64 = raw[number_start..index].parse().ok()?;
+        let unit_start = index;
+        while index < bytes.len() && bytes[index].is_ascii_alphabetic() {
+            index += 1;
+        }
+        let seconds = match &raw[unit_start..index] {
+            "ms" => number / 1000.0,
+            "s" | "" => number,
+            "m" => number * 60.0,
+            "h" => number * 3600.0,
+            "d" => number * 86_400.0,
+            "w" => number * 604_800.0,
+            _ => return None,
+        };
+        total_seconds += seconds;
+    }
+    total_seconds.is_finite().then(|| Duration::from_secs_f64(total_seconds))

Evidence

PR Compliance ID 8 requires safe config handling with actionable validation diagnostics. The new
interval parser calls Duration::from_secs_f64 in two places without an upper-bound check, which
can panic for very large inputs instead of producing a validation error.

Config parity: ENV substitution plus schema validation diagnostics and hot reload
pnpr/crates/pnpr/src/config.rs[571-606]

Agent prompt

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

## Issue description
`parse_interval` calls `Duration::from_secs_f64` on values that may be finite but out of range for `Duration`, which can panic and crash the registry during config parsing.
## Issue Context
This code parses operator-provided YAML config; invalid values should surface as a named `InvalidConfig` error (diagnostic), not trigger a panic.
## Fix Focus Areas
- pnpr/crates/pnpr/src/config.rs[571-606]

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

Description

The new interval fields (maxage, timeout, fail_timeout) are deserialized as Option<String>,
which makes the advertised “bare number means seconds” YAML syntax unreliable unless users quote
numbers as strings. This conflicts with parse_interval() explicitly supporting bare numeric values
and can break verdaccio-shaped configs that use numeric scalars.

Code

pnpr/crates/pnpr/src/config.rs[R415-428]

+    /// Verdaccio interval strings (`"2m"`, `"30s"`, `"1h30m"`) or a bare
+    /// number of seconds; parsed by [`parse_interval`] in
+    /// [`resolve_uplink`]. Kept as raw strings here so an unparsable
+    /// value surfaces as a config error rather than a serde failure.
+    #[serde(default)]
+    maxage: Option<String>,
+    #[serde(default)]
+    timeout: Option<String>,
+    #[serde(default)]
+    max_fails: Option<u32>,
+    #[serde(default)]
+    fail_timeout: Option<String>,
+    #[serde(default)]
+    cache: Option<bool>,

Evidence

UplinkFile defines these knobs as strings, while parse_interval() is intentionally written to
accept bare numeric input; the string-only deserialization shape means “bare number” acceptance only
works when the YAML value is already a string.

pnpr/crates/pnpr/src/config.rs[404-429]
pnpr/crates/pnpr/src/config.rs[554-573]

Agent prompt

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

### Issue description
`UplinkFile` stores `maxage`/`timeout`/`fail_timeout` as `Option<String>`, but `parse_interval()` supports bare numbers (seconds). With a string-only serde shape, YAML numeric scalars (e.g. `timeout: 30`) are not guaranteed to deserialize into these fields, forcing users to quote values and undermining the intended verdaccio compatibility.

### Issue Context
The code comment on `UplinkFile` and the parser behavior both claim bare numbers are accepted.

### Fix Focus Areas
- pnpr/crates/pnpr/src/config.rs[404-429]
- pnpr/crates/pnpr/src/config.rs[524-552]
- pnpr/crates/pnpr/src/config.rs[554-608]

### Suggested fix approach
- Replace `Option<String>` with an untagged scalar enum, e.g.:
 - `#[serde(untagged)] enum IntervalScalar { Str(String), Int(i64), Float(f64) }`
- Normalize `IntervalScalar` to a string (or directly to seconds) before calling `parse_interval`.
- Keep the current “invalid interval becomes InvalidConfig naming the field” behavior.

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

Description

fetch_tarball_response() records circuit-breaker success after only validating response headers,
so mid-stream body errors/timeouts will not increment max_fails and the uplink may never trip to
UpstreamUnavailable. This weakens the per-uplink reliability controls and can cause repeated
failing upstream fetches under load.

Code

pnpr/crates/pnpr/src/upstream.rs[R275-280]

+        let response = self.checked(response, &url).await?;
+        // Success here covers only headers; a mid-stream body failure is
+        // the caller's to observe. Recording success on a clean status is
+        // what verdaccio does too.
+        self.breaker.record_success();
      Ok(FetchOutcome::Ok(response))

Evidence

Rule 9 requires max_fails/fail_timeout to influence runtime behavior by tracking failures. The
new tarball path records record_success() immediately after checked() (status-only), while the
actual response body is streamed later via response.bytes_stream() in serve_tarball, so stream
errors will not be counted as breaker failures.

Per-uplink reliability controls implemented (timeouts, max_fails, fail_timeout, cache disable)
pnpr/crates/pnpr/src/upstream.rs[261-280]
pnpr/crates/pnpr/src/server.rs[659-683]

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 circuit breaker is updated as a success before tarball streaming completes, so body-stream failures (including request timeouts during streaming) do not count toward `max_fails`.
## Issue Context
Per-uplink reliability controls (rule 9) require failures to be tracked so the breaker can open and short-circuit requests. In the tarball path, `reqwest::Response` is returned for streaming and errors can occur after headers are read.
## Fix Focus Areas
- pnpr/crates/pnpr/src/upstream.rs[261-280]
- pnpr/crates/pnpr/src/server.rs[659-683]

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

Description

Upstream::ensure_available() constructs RegistryError::UpstreamUnavailable with `uplink:
self.base.clone() (the upstream URL), and error_response() returns err.to_string()` in the HTTP
body. This can disclose internal upstream endpoints to clients and reports the failing uplink as a
URL rather than the configured uplink name.

Code

pnpr/crates/pnpr/src/upstream.rs[R286-291]

+    fn ensure_available(&self) -> Result<()> {
+        if self.breaker.is_available() {
+            return Ok(());
+        }
+        Err(RegistryError::UpstreamUnavailable { uplink: self.base.clone() })
+    }

Evidence

ensure_available() uses self.base (configured upstream URL) as the uplink field, and
error_response() serializes errors with err.to_string() directly into the HTTP response body;
the UpstreamUnavailable Display format includes {uplink}.

pnpr/crates/pnpr/src/upstream.rs[286-291]
pnpr/crates/pnpr/src/upstream.rs[170-184]
pnpr/crates/pnpr/src/error.rs[22-32]
pnpr/crates/pnpr/src/server.rs[1975-1979]

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 the circuit breaker is open, pnpr returns a 503 with a message containing the upstream base URL (because `UpstreamUnavailable.uplink` is set from `self.base`). Since `server::error_response` returns `err.to_string()` to the client, this leaks upstream endpoint details and produces less actionable errors than identifying the configured uplink name.
## Issue Context
- `Upstream` currently does not retain the configured uplink name; it only stores `base` (URL).
- `UpstreamUnavailable` is intended to identify the uplink, but it is currently populated with the URL.
## Fix Focus Areas
- Add an `uplink_name: String` (or similar) field to `Upstream` and populate it when building upstreams in the router.
- Change `ensure_available()` to emit `UpstreamUnavailable { uplink: uplink_name.clone() }` (or change the error variant to carry both name+url but only display the name).
- Keep the URL for logs/spans if needed, but avoid returning it in the client-visible error string.
### Code pointers
- pnpr/crates/pnpr/src/upstream.rs[170-184]
- pnpr/crates/pnpr/src/upstream.rs[283-291]
- pnpr/crates/pnpr/src/server.rs[160-166]
- pnpr/crates/pnpr/src/error.rs[22-32]

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