Retry BMC reads once after refreshing credentials on 401 - #4216
Retry BMC reads once after refreshing credentials on 401#4216wminckler wants to merge 3 commits into
Conversation
BmcClient refreshed credentials after an authentication error but returned the original failure without replaying the request. Since BMC credentials rotate underneath a long-lived client, every rotation burned one request per in-flight resource. For the metric collectors that failure is not benign: a fetch that fails emits no samples for the interval, so the sink sweeps that resource's series and they reappear on the next interval. The result is stable hw_metric series churning between collection intervals and an incomplete metric catalog. Replace refresh_auth_if_needed with read_with_auth_retry, which takes the operation as a re-runnable closure and replays it once after a successful refresh. Only the idempotent reads (get/expand/filter/stream) use it, so mutating calls still fail fast. The retry is single-shot: it runs with credentials newer than the rejected ones, so a second 401 means the credentials are wrong rather than stale. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughBMC read operations now share centralized credential validation, authentication-error refresh, and one-time retry behavior. Stream establishment uses the same path while item-level errors remain unchanged. Tests cover retry limits, refresh failures, and credential-generation races. ChangesBMC authentication retry
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BmcClient
participant read_with_auth_retry
participant Credentials
participant CircuitBreaker
BmcClient->>read_with_auth_retry: establish read or stream
read_with_auth_retry->>Credentials: ensure credentials and capture generation
read_with_auth_retry->>CircuitBreaker: execute guarded operation
CircuitBreaker-->>read_with_auth_retry: authentication error
read_with_auth_retry->>Credentials: refresh when generation is unchanged
read_with_auth_retry->>CircuitBreaker: retry operation once
CircuitBreaker-->>BmcClient: return result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/health/src/bmc.rs (1)
239-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid retry/generation logic — consider logging the exhausted-retry path too.
Traced this against all five new tests (
read_retries_once_after_refreshing_on_auth_error,read_does_not_retry_non_auth_errors,read_retries_at_most_once,read_surfaces_original_error_when_refresh_fails,read_retries_when_a_concurrent_caller_already_refreshed) — the generation-gated refresh, single-shot retry, and "no-op refresh still counts as success" semantics all check out correctly.One asymmetry: refresh failure (line 276) and retry success (line 286) are both logged, but a second auth failure on the retried
guarded(op())call (line 285) — which per your own doc comment "means the credentials themselves are wrong, not stale" — passes through silently. That's exactly the signal an operator would want when debugging a BMC that keeps rejecting rotated credentials.♻️ Proposed addition to log the exhausted-retry case
- self.guarded(op()).await.inspect(|_| { - tracing::debug!( - original_error = ?error, - endpoint = ?self.addr, - "Retry after BMC credential refresh succeeded" - ); - }) + self.guarded(op()).await.inspect(|_| { + tracing::debug!( + original_error = ?error, + endpoint = ?self.addr, + "Retry after BMC credential refresh succeeded" + ); + }).inspect_err(|retry_error| { + tracing::warn!( + error = ?retry_error, + original_error = ?error, + endpoint = ?self.addr, + "Retry after BMC credential refresh also failed" + ); + })🤖 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 `@crates/health/src/bmc.rs` around lines 239 - 292, Update read_with_auth_retry so the second guarded(op()) attempt logs an error when it fails, including the retry error, original authentication error, and endpoint context; preserve the existing success debug log and return the retry result unchanged.
🤖 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 `@crates/health/src/bmc.rs`:
- Around line 239-292: Update read_with_auth_retry so the second guarded(op())
attempt logs an error when it fails, including the retry error, original
authentication error, and endpoint context; preserve the existing success debug
log and return the retry result unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 40b67280-32e5-4bfa-be16-cf5246dcf95f
📒 Files selected for processing (1)
crates/health/src/bmc.rs
Address review on the retry change: a retry that fails after the refresh returned silently, which is the one outcome an operator needs. It now warns, carrying both the retry error and the original one. Also drop the pre-existing refresh warning to debug. Its only production caller is now the retry path, so it fires on a first auth failure that the very next line recovers from — an expected consequence of credential rotation, not an operator signal. Warning there while the genuine failure stayed silent had the levels exactly backwards, and since auth errors do not trip the circuit breaker it also meant a misconfigured endpoint warned once per resource per interval with no backoff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The five existing tests drive `read_with_auth_retry` directly, so nothing pinned the four call sites to it: reverting `get` to its old non-retrying body left every one of them passing. Verified by doing exactly that — only the new test caught it. Serve a scripted 401-then-200 from a local listener and point a real BmcClient at it through `proxy_url`, which `bmc_url` returns verbatim, so the client speaks plain HTTP and no TLS setup is needed. Counting connections (each response closes) makes "the request was replayed on the wire" a direct assertion rather than an inference from the credential provider call count. The companion test pins the read-only boundary: a mutating call must not be replayed, which is a safety property rather than just a comment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Two notes for reviewers on things that aren't obvious from the diff. Log levels moved.
Nothing in this repo greps for the downgraded message, but anything external keying on it at default level will go quiet. Known limitation — auth failures still have no backoff. |
|
Two more review points, both considered and deliberately not changed. The per-attempt No metric was added for retries. A counter distinguishing "read succeeded on the replay" from "read succeeded outright" would make this fix observable in the field, and today a successful retry is indistinguishable from a clean read. It is not a small addition, for three reasons:
Both are recorded here so the reasoning is visible rather than re-derived at the next review. |
1 participant
BMC credentials rotate underneath the long-lived
BmcClientthat the hardware-health collectors share. When a request lands on the wrong side of a rotation, the BMC answers 401.BmcClientalready noticed this and refreshed its credentials — but it then returned the original failure to the caller without replaying the request. Every rotation therefore burned one request per resource that happened to be in flight.For the metric collectors that failure is not benign. Each collection interval is bracketed by
MetricCollectionStart/MetricCollectionEnd, and a fetch that fails emits no samples, so the sink sweeps that resource's series for the interval. On the next interval the refreshed credentials work and the series come back. The observable symptom is stablehw_metricseries churning between consecutive intervals — series counts moving 152 → 142 → 142 and 126 → 142 with different identity hashes across a short window, alongside a large fetch-failure count — which stops the collector from reliably publishing the complete default metric catalog.This replaces
refresh_auth_if_neededwithread_with_auth_retry, which takes the operation as a re-runnable closure and, after a successful refresh, replays it once.A few deliberate constraints:
get,expand,filter,stream) route through the retry. Mutating calls (create,update,delete,action,multipart_update,create_session) are unchanged and still fail fast, so there is no risk of a replayed write.refresh_credentialsreturns early when a concurrent caller has already rotated past the observed generation. That still means the credentials now in place are newer than the rejected ones, so the replay is warranted — and no extra provider fetch happens.Circuit-breaker semantics are unchanged: both attempts run through
guarded, and an open-circuit fast-fail is not an auth error, so it never triggers a refresh.Related issues
Type of Change
Breaking Changes
Testing
Five new cases in
bmc::testsdriveread_with_auth_retrywith a scripted operation that records attempt counts:read_retries_once_after_refreshing_on_auth_error— the regression itself: 401 → refresh → retry succeeds, in exactly 2 attempts and 2 provider calls.read_does_not_retry_non_auth_errors— a 404 surfaces immediately, with no credential refresh.read_retries_at_most_once— a persistent 401 stops at 2 attempts and 1 refresh.read_surfaces_original_error_when_refresh_fails— no retry, and the original 401 (not the refresh error) surfaces.read_retries_when_a_concurrent_caller_already_refreshed— the no-op refresh path still retries, without re-fetching credentials.Full
carbide-healthlib suite passes (398 tests), plus clippy and rustfmt.Additional Notes
This implements the retry half of the fix, not the "preserve last known series" half. If a BMC resource is genuinely unreadable for an entire interval — a real outage rather than a credential rotation — its series will still be swept. Whether the sink should hold last-known series across a failed interval is a separate design question about staleness handling, and is intentionally out of scope here.
expandandfilternow clone theirExpandQuery/FilterQueryper attempt, since those are consumed by value and the closure has to be callable twice.🤖 Generated with Claude Code