From 43486b06e386bacede09403e2d6356b95822f4ec Mon Sep 17 00:00:00 2001 From: Azeem Shaik Date: Wed, 29 Jul 2026 16:37:30 +0530 Subject: [PATCH 1/3] Fail with RetryError: attempts, elapsed, and why it stopped A retry that gave up handed back the last error and nothing else. That error can't tell you whether you burned three retries in 700ms, spent a 30s budget, or had `.when` reject the first error so nothing was ever retried. Three different incidents, identical output. Both drivers now fail with `RetryError`, carrying the last error plus `attempts()`, `elapsed()`, and a `stop_reason()`. Building it lives in one shared `give_up` so the two drivers can't drift on the arithmetic. `impl Error` is bounded on `E: Debug + Display`, not `E: Error + 'static`. The two overlap, so it's one or the other (E0119), and Debug+Display covers strictly more: `String`, `Box`, and `anyhow::Error` all satisfy it and none implement `Error`. Retrying an `anyhow::Result` is about as common as application code gets, and the alternative makes it un-`?`-able. The cost is no `source()`; the inner error's text still rides in `Display`. The async driver now starts its clock on the first poll rather than at `.into_future()`, matching blocking. That gap was invisible until `elapsed()` made it public, and it also stops a future parked in a `FuturesUnordered` from billing the wait to the operation. Escape hatch is one call: `.map_err(RetryError::into_error)`. Clock reads stay O(1): two with no budget regardless of retry count, and the `MaxElapsed` path reuses the read its own budget check just did. The old exact-count test is now a slope assertion over 3 and 30 retries, and the blocking mock gained the same counter so both drivers pin it. Mutation-checked: reporting retries instead of attempts, an ungated give-up event, dropping the MaxElapsed clock reuse, a wrong stop reason in either driver, elapsed measured from zero, and sampling start at conversion time all fail the suite. ADR005 records the always-wrap call, the Error bound with its coverage table, bounded Display, no PartialEq, and the clock-start move. --- CHANGELOG.md | 19 ++++ README.md | 16 +++ docs/adr/ADR006.md | 92 +++++++++++++++++ docs/adr/README.md | 1 + src/blocking/mod.rs | 2 +- src/blocking/retry.rs | 110 +++++++++++++++------ src/error.rs | 212 ++++++++++++++++++++++++++++++++++++++++ src/lib.rs | 18 +++- src/retry.rs | 222 +++++++++++++++++++++++++++++++----------- src/shared.rs | 83 +++++++++++++--- 10 files changed, 674 insertions(+), 101 deletions(-) create mode 100644 docs/adr/ADR006.md create mode 100644 src/error.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 408bc02..29d9ee2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.4.0] - 2026-08-09 + +### Changed +- **Breaking:** `retry(..).await` and `blocking::retry(..).call()` now fail with `RetryError` + instead of `E`. It carries the last error plus `attempts()`, `elapsed()`, and a `stop_reason()` + of `RetriesExhausted` / `NotRetryable` / `MaxElapsed`, so a failed retry says which limit it hit + rather than leaving you to guess. To keep the old signature, add + `.map_err(RetryError::into_error)`. + + `?` into `Box` and `anyhow::Error` keeps working and now covers error types it didn't + before (`String`, `Box`, `anyhow::Error`), because `RetryError` implements + `std::error::Error` for `E: Debug + Display` rather than `E: Error`. The trade is that it has no + `source()`; the inner error's text rides along in `Display`. ADR005 has the reasoning. +- **Breaking:** giving up now emits one more `tracing` event on target `mettle::retry`, with + `attempts`, `elapsed_ms`, and `reason`, but only when at least one retry actually happened. + Anyone counting events on that target sees a change. +- The async driver starts its clock on the first poll rather than at `.into_future()`, matching the + blocking driver. A future parked before its first poll no longer bills that time to the operation. + ## [0.3.0] - 2026-08-09 Adds jitter. Purely additive apart from one collision noted under Upgrading. diff --git a/README.md b/README.md index 77fb583..64e994e 100644 --- a/README.md +++ b/README.md @@ -58,6 +58,22 @@ retrying sooner than `base`, so a dependency that frees up early isn't picked up Both seed from entropy by default and take a fixed seed (`with_seed`) when you want a test to replay the same delays. +When a retry gives up you get a `RetryError`, which says what stopped it: + +```rust +match retry(|| async { fetch(&url).await }).await { + Ok(body) => body, + Err(e) => { + // "max_elapsed after 5 attempts in 29.4s: connection refused" + tracing::error!("{} after {} attempts in {:?}: {}", + e.stop_reason().as_str(), e.attempts(), e.elapsed(), e.error()); + return Err(e.into()); // ?-able into Box / anyhow + } +} +``` + +Only want the underlying error? `.map_err(RetryError::into_error)`. + ## Tools Each tool comes with a runnable example. Start there: diff --git a/docs/adr/ADR006.md b/docs/adr/ADR006.md new file mode 100644 index 0000000..7bafd7e --- /dev/null +++ b/docs/adr/ADR006.md @@ -0,0 +1,92 @@ +# ADR006: `RetryError`, and the bound that decides who can use `?` + +**Status:** Accepted + +## Context + +Through 0.2.0, a retry that gave up handed back the last error and nothing else. That error can't +answer the question you actually have at 3am: did we exhaust three retries in 700 ms, or did we +spend a 30 s budget, or did the `.when` predicate reject the very first error and we never retried +at all? Those are three different incidents and they looked identical. + +Fixing that means changing the error type of the only two functions anyone calls, so it's the kind +of change 0.x exists for and we only get to make cheaply once. + +## Decisions + +**1. Always wrap. `retry` and `blocking::retry` fail with `RetryError`.** +Not an opt-in second terminal like `.detailed()`. That alternative adds a parallel type, a second +documented path that never goes away, and a builder method whose position relative to the other +four matters. It also doesn't save anything: there's one state machine, so it either always +measures at the give-up point or grows a runtime branch to decide whether to bother. + +The escape hatch is one call. `.map_err(RetryError::into_error)` gets you exactly the 0.2.0 +behaviour, and that sentence is in the CHANGELOG, both module docs, and the `into_error` rustdoc. + +**2. `impl Error for RetryError where E: Debug + Display`, and no `source()`.** +This is the consequential one, and it's binary. The alternative is +`where E: Error + 'static` plus a `source()` that returns the inner error. You cannot have both: +they overlap, and the compiler rejects the pair with E0119. + +We took `Debug + Display` because it covers strictly more error types: + +| `E` | `Error + 'static` | `Debug + Display` | +|-----|-------------------|-------------------| +| `io::Error`, `thiserror` enums | works | works | +| `String`, `&str` | no | works | +| `Box` | no | works | +| `anyhow::Error` | no | works | + +`anyhow::Error` does not implement `std::error::Error`, and retrying an `anyhow::Result` is about +as common as application code gets. Choosing `Error + 'static` would mean that code can't use `?` +at all. + +What we give up is the error chain. Anything that walks `source()` (anyhow's `{:#}`, +`std::error::Report`, tracing-error) sees one link where it might have seen two. The inner error's +text is still there, because `Display` includes it, and `downcast_ref::>()` recovers +the whole structure including the attempt count. We think the text is what people read and the +chain is what libraries walk, and the population that can't compile at all is worse off than the +population with a flatter chain. + +Reversing this later is breaking in both directions, so it's written down here rather than left as +an accident of whichever impl got typed first. + +**3. `Display` is bounded on `E: Display` and includes the inner error.** +The alternative, an unbounded `impl Display` printing only the stop reason, would mean +`eprintln!("{e}")` never says what actually failed. For `&str` and `String` errors there's no chain +to recover it from, so that text would be gone. Widening a bounded impl to unbounded later is +non-breaking; narrowing is not, so the bounded version is also the reversible direction. + +**4. No `PartialEq`.** +Downstream can't construct a `RetryError` (private fields, no public constructor), so the +right-hand side of an `assert_eq!` is unbuildable anyway. Where it would work it compares +`elapsed`, which is wall-clock. Adding it later is non-breaking. + +**5. The clock starts at the first poll, not at `.into_future()`.** +The async driver used to sample `start` when the builder was converted. The blocking driver samples +at the first attempt. That difference was invisible while nothing exposed elapsed time; the moment +`elapsed()` became public it would have been two drivers reporting different numbers for the same +work, which ADR001 decision 3 exists to prevent. It also stops a future that sits parked in a +`FuturesUnordered` from billing the parking time to the operation. + +**6. `attempts()` counts attempts, not retries.** +Internally the counter is retries performed. The public number is one more than that, because even +an error rejected by `.when` on the first try means the operation ran once. Returning the raw +counter would compile and quietly report `0` attempts for an operation that demonstrably ran, so +both drivers go through one shared `give_up` and both pin `attempts() == 1` on that path in tests. + +**7. One give-up event, only when a retry happened.** +Giving up emits a single `WARN` on `mettle::retry` with `attempts`, `elapsed_ms`, and `reason`. It +carries no error field: the caller now holds the error, and the last retry event already logged it. +The gate matters. Without it, a `.when(|e| e.is_transient())` in front of an HTTP client would emit +a warning for every 404, on a path that is silent today. + +## Consequences + +Every caller changes their error type, and `?` into a concrete `E` needs +`.map_err(RetryError::into_error)`. In exchange, `?` into `Box` and `anyhow::Error` now +works where it didn't in 0.2.0, and a failed retry can be reported without guessing. + +We're committed to the `Debug + Display` bound and to the absence of `source()`. Both are one-way +after 0.3.0. ADR003 named the `mettle::retry` target as something people build against, and that +target now carries two message shapes, so anyone counting events on it sees a change. diff --git a/docs/adr/README.md b/docs/adr/README.md index 9811e6b..f2acee0 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -11,3 +11,4 @@ changes, edit its file and say what changed and why. | [003](ADR003.md) | Dependencies and Cargo features | Accepted | | [004](ADR004.md) | Jitter, a fourth dependency, and `Clone` | Accepted | | [005](ADR005.md) | Making the tested path as usable as the production path | Accepted | +| [006](ADR006.md) | `RetryError` and the `Error` bound | Accepted | diff --git a/src/blocking/mod.rs b/src/blocking/mod.rs index 88d11fb..486193a 100644 --- a/src/blocking/mod.rs +++ b/src/blocking/mod.rs @@ -8,7 +8,7 @@ //! # fn fetch() -> Result { Ok(1) } //! let value = mettle::blocking::retry(fetch).call()?; //! # let _ = value; -//! # Ok::<(), std::io::Error>(()) +//! # Ok::<(), Box>(()) //! ``` mod clock; diff --git a/src/blocking/retry.rs b/src/blocking/retry.rs index 9fdd664..ba46160 100644 --- a/src/blocking/retry.rs +++ b/src/blocking/retry.rs @@ -2,7 +2,8 @@ use super::clock::{Clock, StdClock}; use crate::backoff::{Backoff, ExponentialBackoff}; -use crate::shared::{should_retry_after, trace_retry}; +use crate::error::RetryError; +use crate::shared::{Decision, give_up, should_retry_after, trace_retry}; use std::time::Duration; /// A configurable blocking retry operation. @@ -30,8 +31,10 @@ pub struct Retry { /// # fn fetch() -> Result { Ok(1) } /// let value = retry(fetch).call()?; /// # let _ = value; -/// # Ok::<(), std::io::Error>(()) +/// # Ok::<(), Box>(()) /// ``` +/// +/// On failure the error is a [`RetryError`]; for the bare error, `.map_err(RetryError::into_error)`. pub fn retry(op: F) -> Retry bool> where F: FnMut() -> Result, @@ -105,10 +108,15 @@ where E: std::fmt::Debug, { /// Run the operation, blocking between attempts, until it succeeds or gives up. - pub fn call(mut self) -> Result { + /// + /// # Errors + /// Returns a [`RetryError`] carrying the last error, the attempt count, the elapsed time, and + /// why it stopped. For the bare error, `.map_err(RetryError::into_error)`. + pub fn call(mut self) -> Result> { let mut backoff = self.backoff; - let mut attempt = 0u32; + let mut retries = 0u32; let start = self.clock.now(); + let elapsed = || self.clock.now().saturating_duration_since(start); loop { let err = match (self.op)() { @@ -116,15 +124,15 @@ where Err(err) => err, }; - match should_retry_after(&err, &self.when, &mut backoff, self.max_elapsed, || { - self.clock.now().saturating_duration_since(start) - }) { - Some(delay) => { - attempt += 1; - trace_retry(attempt, &err, delay); + match should_retry_after(&err, &self.when, &mut backoff, self.max_elapsed, elapsed) { + Decision::Retry(delay) => { + retries += 1; + trace_retry(retries, &err, delay); self.clock.sleep(delay); } - None => return Err(err), + Decision::Stop { reason, elapsed: m } => { + return Err(give_up(err, retries, reason, m, elapsed)); + } } } } @@ -134,6 +142,8 @@ where mod tests { use super::*; use crate::backoff::ExponentialBackoffConfig; + use crate::error::StopReason; + use std::sync::atomic::{AtomicUsize, Ordering::SeqCst}; use std::sync::{Arc, Mutex}; use std::time::Instant; @@ -143,6 +153,7 @@ mod tests { start: Instant, elapsed: Arc>, log: Arc>>, + now_calls: Arc, } impl MockClock { fn new() -> Self { @@ -150,14 +161,19 @@ mod tests { start: Instant::now(), elapsed: Arc::new(Mutex::new(Duration::ZERO)), log: Arc::new(Mutex::new(Vec::new())), + now_calls: Arc::new(AtomicUsize::new(0)), } } fn slept(&self) -> Vec { self.log.lock().unwrap().clone() } + fn now_calls(&self) -> usize { + self.now_calls.load(SeqCst) + } } impl Clock for MockClock { fn now(&self) -> Instant { + self.now_calls.fetch_add(1, SeqCst); self.start + *self.elapsed.lock().unwrap() } fn sleep(&self, dur: Duration) { @@ -182,8 +198,8 @@ mod tests { #[test] fn succeeds_first_try() { let clock = MockClock::new(); - let result: Result = retry(|| Ok(42)).clock(clock.clone()).call(); - assert_eq!(result, Ok(42)); + let result: Result> = retry(|| Ok(42)).clock(clock.clone()).call(); + assert_eq!(result.unwrap(), 42); assert!(clock.slept().is_empty()); } @@ -191,7 +207,7 @@ mod tests { fn retries_then_succeeds() { let clock = MockClock::new(); let mut n = 0; - let result: Result = retry(|| { + let result: Result> = retry(|| { n += 1; if n < 3 { Err("boom") } else { Ok(42) } }) @@ -199,7 +215,7 @@ mod tests { .clock(clock.clone()) .call(); - assert_eq!(result, Ok(42)); + assert_eq!(result.unwrap(), 42); assert_eq!(n, 3); // 2 failures + 1 success assert_eq!(clock.slept(), vec![secs(1), secs(2)]); } @@ -208,7 +224,7 @@ mod tests { fn stops_on_non_retryable() { let clock = MockClock::new(); let mut calls = 0; - let result: Result = retry(|| { + let result: Result> = retry(|| { calls += 1; Err("nope") }) @@ -217,7 +233,11 @@ mod tests { .when(|_e| false) .call(); - assert_eq!(result, Err("nope")); + let err = result.unwrap_err(); + assert_eq!(*err.error(), "nope"); + assert_eq!(err.stop_reason(), StopReason::NotRetryable); + assert_eq!(err.attempts(), 1); // the rejected attempt still ran + assert_eq!(err.elapsed(), Duration::ZERO); assert_eq!(calls, 1); // exactly one attempt assert!(clock.slept().is_empty()); } @@ -225,25 +245,36 @@ mod tests { #[test] fn exhausts_retries() { let clock = MockClock::new(); - let result: Result = retry(|| Err("always")) + let result: Result> = retry(|| Err("always")) .backoff(backoff(3)) .clock(clock.clone()) .call(); - assert_eq!(result, Err("always")); + let err = result.unwrap_err(); + assert_eq!(*err.error(), "always"); + assert_eq!(err.stop_reason(), StopReason::RetriesExhausted); + assert_eq!(err.attempts(), 4); // 3 retries → 4 attempts + assert_eq!(err.elapsed(), secs(7)); // 1 + 2 + 4 on the mock clock assert_eq!(clock.slept().len(), 3); // 3 retries → 3 sleeps, then give up } #[test] fn stops_on_time_budget() { let clock = MockClock::new(); - let result: Result = retry(|| Err("slow")) + let result: Result> = retry(|| Err("slow")) .backoff(backoff(100)) // effectively unlimited retries .clock(clock.clone()) .max_elapsed(secs(10)) .call(); - assert_eq!(result, Err("slow")); + let err = result.unwrap_err(); + assert_eq!(*err.error(), "slow"); + assert_eq!(err.stop_reason(), StopReason::MaxElapsed); + assert_eq!(err.attempts(), 4); + assert!( + err.elapsed() < secs(10), + "elapsed must stay under the budget" + ); // 1 (→1s), 2 (→3s), 4 (→7s); next would be 8 → 7+8=15 ≥ 10 → stop. assert_eq!(clock.slept(), vec![secs(1), secs(2), secs(4)]); } @@ -253,12 +284,12 @@ mod tests { // The async twin of this lives in src/retry.rs. Both drivers take any `Backoff`, so both // have to be shown driving a randomized one (ADR001 decision 3). let clock = MockClock::new(); - let out: Result = retry(|| Err("boom")) + let out: Result> = retry(|| Err("boom")) .backoff(backoff(3).jittered_with_seed(42)) .clock(&clock) .call(); - assert_eq!(out, Err("boom")); + assert_eq!(*out.unwrap_err().error(), "boom"); let slept = clock.slept(); assert_eq!(slept.len(), 3); // Underlying exponential is 1s, 2s, 4s; full jitter can only shrink each one. @@ -272,7 +303,7 @@ mod tests { use crate::backoff::{DecorrelatedBackoff, DecorrelatedBackoffConfig}; let clock = MockClock::new(); - let out: Result = retry(|| Err("boom")) + let out: Result> = retry(|| Err("boom")) .backoff( DecorrelatedBackoff::with_seed( DecorrelatedBackoffConfig { @@ -287,7 +318,7 @@ mod tests { .clock(&clock) .call(); - assert_eq!(out, Err("boom")); + assert_eq!(*out.unwrap_err().error(), "boom"); let slept = clock.slept(); assert_eq!(slept.len(), 4); assert!( @@ -301,15 +332,36 @@ mod tests { // `.clock(c)` takes the clock by value, so without the reference impls a test could hand // over its mock and never read it back. Both forms must reach the same mock. let clock = MockClock::new(); - let _: Result = retry(|| Err("x")).backoff(backoff(2)).clock(&clock).call(); + let _: Result> = retry(|| Err("x")).backoff(backoff(2)).clock(&clock).call(); assert_eq!(clock.slept(), vec![secs(1), secs(2)]); let shared = std::sync::Arc::new(MockClock::new()); - let _: Result = retry(|| Err("x")) + let _: Result> = retry(|| Err("x")) .backoff(backoff(2)) .clock(Arc::clone(&shared)) .call(); assert_eq!(shared.slept(), vec![secs(1), secs(2)]); + + #[test] + fn clock_reads_do_not_scale_with_attempts() { + // The async twin of this lives in src/retry.rs. `elapsed()` is public now, so both + // drivers have to agree on what it costs. + for retries in [3, 30] { + let clock = MockClock::new(); + let _: Result> = retry(|| Err("x")) + .backoff(backoff(retries)) + .clock(clock.clone()) + .call(); + assert_eq!(clock.now_calls(), 2, "with {retries} retries and no budget"); + } + + let clock = MockClock::new(); + let _: Result> = retry(|| Err("x")) + .backoff(backoff(3)) + .clock(clock.clone()) + .max_elapsed(secs(1000)) + .call(); + assert_eq!(clock.now_calls(), 5); // start + 3 decisions + terminal } #[test] @@ -319,7 +371,7 @@ mod tests { let clock = MockClock::new(); let mut n = 0; - let out: Result = retry(|| { + let out: Result> = retry(|| { n += 1; if n < 3 { Err("boom") } else { Ok(42) } }) @@ -327,7 +379,7 @@ mod tests { .clock(clock) .call(); - assert_eq!(out, Ok(42)); + assert_eq!(out.unwrap(), 42); assert_eq!(events.get(), 2); // one event per retry } } diff --git a/src/error.rs b/src/error.rs new file mode 100644 index 0000000..40948c3 --- /dev/null +++ b/src/error.rs @@ -0,0 +1,212 @@ +//! What a retry hands back when it gives up: the last error plus the context you need to tell +//! "the dependency is down" from "our timeout is too tight". + +use std::time::Duration; + +/// Why a retry stopped. Each variant names the knob that produced it. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +#[non_exhaustive] +pub enum StopReason { + /// The backoff ran out of retries (set by `.backoff(..)`). + RetriesExhausted, + /// The `.when(..)` predicate rejected the error, so no retry was attempted for it. + NotRetryable, + /// The next delay wouldn't have fit in the `.max_elapsed(..)` budget. + MaxElapsed, +} + +impl StopReason { + /// A stable snake_case token, for metric labels and log filters. These strings are part of + /// the API and won't change under you: `"retries_exhausted"`, `"not_retryable"`, + /// `"max_elapsed"`. + pub const fn as_str(self) -> &'static str { + match self { + Self::RetriesExhausted => "retries_exhausted", + Self::NotRetryable => "not_retryable", + Self::MaxElapsed => "max_elapsed", + } + } +} + +impl std::fmt::Display for StopReason { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let prose = match self { + Self::RetriesExhausted => "retries exhausted", + Self::NotRetryable => "error was not retryable", + Self::MaxElapsed => "time budget spent", + }; + f.write_str(prose) + } +} + +/// A retry that gave up: the error from the last attempt, plus how many attempts it made, how +/// long it spent, and what stopped it. +/// +/// Both `retry` and `blocking::retry` return this as their error type. The last error alone +/// can't tell you whether you exhausted three retries in 700 ms or burned a 30 s budget, and +/// that difference is usually the whole question during an incident. +/// +/// ``` +/// # use mettle::{RetryError, StopReason}; +/// # fn demo(err: RetryError) { +/// eprintln!( +/// "{} after {} attempts in {:?}: {}", +/// err.stop_reason().as_str(), +/// err.attempts(), +/// err.elapsed(), +/// err.error() +/// ); +/// # } +/// ``` +/// +/// To go back to the bare error and keep an existing `Result` signature, use +/// [`into_error`](RetryError::into_error): +/// +/// ``` +/// # use mettle::RetryError; +/// # fn demo(r: Result>) -> Result { +/// r.map_err(RetryError::into_error) +/// # } +/// ``` +#[derive(Debug, Clone)] +pub struct RetryError { + error: E, + attempts: u32, + elapsed: Duration, + stop_reason: StopReason, +} + +impl RetryError { + pub(crate) fn new(error: E, attempts: u32, elapsed: Duration, stop_reason: StopReason) -> Self { + Self { + error, + attempts, + elapsed, + stop_reason, + } + } + + /// The error from the final attempt. Earlier errors appear only in the per-retry `tracing` + /// events; they aren't collected, so retrying doesn't allocate. + pub fn error(&self) -> &E { + &self.error + } + + /// Take the final error back, dropping the context around it. + pub fn into_error(self) -> E { + self.error + } + + /// How many times the operation actually ran. Always at least 1, including when the very + /// first error was rejected by `.when(..)`. + pub fn attempts(&self) -> u32 { + self.attempts + } + + /// How long it took, measured on the injected `Clock` from the start of the first attempt to + /// giving up. + /// + /// Under [`StopReason::MaxElapsed`] this is always strictly *less* than the budget, since the + /// budget is what stopped the next delay from being taken. Under + /// [`StopReason::NotRetryable`] it's just one call's latency. + pub fn elapsed(&self) -> Duration { + self.elapsed + } + + /// Why the retry stopped. + pub fn stop_reason(&self) -> StopReason { + self.stop_reason + } +} + +impl std::fmt::Display for RetryError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "gave up after {} attempt{} in {:?} ({}): {}", + self.attempts, + if self.attempts == 1 { "" } else { "s" }, + self.elapsed, + self.stop_reason, + self.error + ) + } +} + +// No `source()`. Returning the inner error there would need `E: Error + 'static`, and that impl +// conflicts with this one (E0119), so it's one or the other. `Debug + Display` covers strictly +// more error types: `String`, `Box`, and `anyhow::Error` all satisfy it and none of +// them implement `Error`. The inner error's text still rides along in `Display`, and +// `downcast_ref::>()` recovers the whole structure. +impl std::error::Error for RetryError {} + +#[cfg(test)] +mod tests { + use super::*; + + fn err() -> RetryError<&'static str> { + RetryError::new( + "connection refused", + 4, + Duration::from_millis(7150), + StopReason::RetriesExhausted, + ) + } + + #[test] + fn display_carries_the_inner_error() { + assert_eq!( + err().to_string(), + "gave up after 4 attempts in 7.15s (retries exhausted): connection refused" + ); + } + + #[test] + fn display_does_not_say_one_attempts() { + let e = RetryError::new("nope", 1, Duration::ZERO, StopReason::NotRetryable); + assert!(e.to_string().contains("1 attempt in"), "{e}"); + } + + #[test] + fn stop_reason_tokens_are_stable() { + // These strings end up as metric labels, so pin them. + assert_eq!(StopReason::RetriesExhausted.as_str(), "retries_exhausted"); + assert_eq!(StopReason::NotRetryable.as_str(), "not_retryable"); + assert_eq!(StopReason::MaxElapsed.as_str(), "max_elapsed"); + } + + #[test] + fn accessors_need_no_bounds_on_the_error() { + // `E` here is deliberately neither `Debug` nor `Display`. + struct Opaque; + let e = RetryError::new(Opaque, 2, Duration::from_secs(1), StopReason::MaxElapsed); + assert_eq!(e.attempts(), 2); + assert_eq!(e.elapsed(), Duration::from_secs(1)); + assert_eq!(e.stop_reason(), StopReason::MaxElapsed); + let Opaque = e.into_error(); + } + + #[test] + fn boxes_into_dyn_error_for_types_that_are_not_error() { + // The whole reason `Error` is bounded on `Debug + Display` rather than `Error + 'static`. + // None of these implement `std::error::Error`. + fn boxed( + e: RetryError, + ) -> Box { + Box::new(e) + } + let _ = boxed(err()); + let _ = boxed(RetryError::new( + String::from("oops"), + 1, + Duration::ZERO, + StopReason::NotRetryable, + )); + let _: Box = Box::new(RetryError::new( + Box::::from("inner"), + 1, + Duration::ZERO, + StopReason::NotRetryable, + )); + } +} diff --git a/src/lib.rs b/src/lib.rs index 50ac609..4b8c56a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -50,11 +50,23 @@ //! [`jittered_with_seed`](Backoff::jittered_with_seed) or //! [`DecorrelatedBackoff::with_seed`] when a test needs the delays to repeat. //! +//! # When it fails +//! +//! Both APIs fail with a [`RetryError`](RetryError): the last error, plus how many attempts ran, +//! how long they took, and a [`StopReason`] saying which limit stopped it. During an incident that +//! difference is usually the whole question, since the last error alone can't tell you whether you +//! burned three retries in 700 ms or a 30 s budget. +//! +//! It `?`s straight into `Box` and `anyhow::Error`. To go back to the bare error, use +//! `.map_err(RetryError::into_error)`. +//! //! # Observability //! //! Every retry emits a [`tracing`](https://docs.rs/tracing) event on target `mettle::retry` at -//! `WARN`, carrying the `attempt` number, `delay_ms`, and the `error`. Install any subscriber to -//! see them, filter with `RUST_LOG=mettle::retry=warn`, or silence with `RUST_LOG=mettle=off`. +//! `WARN`, carrying the `attempt` number, `delay_ms`, and the `error`. If at least one retry +//! happened, giving up emits one more on the same target with `attempts`, `elapsed_ms`, and +//! `reason`. Install any subscriber to see them, filter with `RUST_LOG=mettle::retry=warn`, or +//! silence with `RUST_LOG=mettle=off`. #![forbid(unsafe_code)] #![deny(missing_docs)] @@ -64,6 +76,7 @@ compile_error!("enable at least one of the `async` or `blocking` features"); pub mod backoff; +pub mod error; #[cfg(feature = "blocking")] #[cfg_attr(docsrs, doc(cfg(feature = "blocking")))] @@ -87,5 +100,6 @@ pub use backoff::{ }; #[cfg(feature = "async")] pub use clock::Clock; +pub use error::{RetryError, StopReason}; #[cfg(feature = "async")] pub use retry::retry; diff --git a/src/retry.rs b/src/retry.rs index 4cfc6ad..cc79fbf 100644 --- a/src/retry.rs +++ b/src/retry.rs @@ -22,7 +22,8 @@ use pin_project_lite::pin_project; use crate::backoff::{Backoff, ExponentialBackoff}; use crate::clock::{Clock, TokioClock}; -use crate::shared::{should_retry_after, trace_retry}; +use crate::error::RetryError; +use crate::shared::{Decision, give_up, should_retry_after, trace_retry}; /// A configurable retry operation. /// @@ -46,12 +47,25 @@ pub struct Retry { /// /// ```no_run /// # use mettle::retry; -/// # async fn demo() -> Result<(), std::io::Error> { +/// # async fn demo() -> Result<(), Box> { /// let value = retry(|| async { Ok::<_, std::io::Error>(1) }).await?; /// # let _ = value; /// # Ok(()) /// # } /// ``` +/// +/// On failure the error is a [`RetryError`], carrying the last error plus the attempt count, +/// elapsed time, and why it stopped. It `?`s into `Box` and `anyhow::Error`. To get +/// the bare error back and keep a `Result` signature: +/// +/// ```no_run +/// # use mettle::{retry, RetryError}; +/// # async fn demo() -> Result { +/// retry(|| async { Ok::<_, std::io::Error>(1) }) +/// .await +/// .map_err(RetryError::into_error) +/// # } +/// ``` pub fn retry(op: F) -> Retry bool> where F: FnMut() -> Fut, @@ -127,15 +141,18 @@ where B: Backoff, Fut: Future>, { - type Output = Result; + type Output = Result>; type IntoFuture = RetryFuture; fn into_future(self) -> Self::IntoFuture { - let start = self.clock.now(); RetryFuture { - start, + // Sampled on the first poll, not here: a future can sit unpolled (parked in a + // `FuturesUnordered`, say), and that wait isn't time the operation spent. It also + // keeps the async and blocking drivers measuring from the same point, which matters + // now that `elapsed` is something callers can read. + start: None, state: RetryState::Idle, - attempt: 0, + retries: 0, op: self.op, when: self.when, clock: self.clock, @@ -155,8 +172,8 @@ pin_project! { when: P, clock: C, backoff: B, - start: Instant, - attempt: u32, + start: Option, + retries: u32, #[pin] state: RetryState, @@ -186,37 +203,54 @@ where S: Future, Fut: Future>, { - type Output = Result; + type Output = Result>; fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { let mut this = self.project(); loop { let next = match this.state.as_mut().project() { - // Nothing in flight — start an attempt. - RetryStateProj::Idle => RetryState::Attempting { fut: (this.op)() }, + // Nothing in flight — start an attempt. Reached exactly once, so this is where + // the clock starts. + RetryStateProj::Idle => { + *this.start = Some(this.clock.now()); + RetryState::Attempting { fut: (this.op)() } + } // An attempt is in flight — drive it. RetryStateProj::Attempting { fut } => match fut.poll(cx) { Poll::Pending => return Poll::Pending, Poll::Ready(Ok(value)) => return Poll::Ready(Ok(value)), Poll::Ready(Err(err)) => { + let elapsed = || { + this.start.map_or(Duration::ZERO, |s| { + this.clock.now().saturating_duration_since(s) + }) + }; let step = should_retry_after( &err, &*this.when, &mut *this.backoff, *this.max_elapsed, - || this.clock.now().saturating_duration_since(*this.start), + elapsed, ); match step { - Some(delay) => { - *this.attempt += 1; - trace_retry(*this.attempt, &err, delay); + Decision::Retry(delay) => { + *this.retries += 1; + trace_retry(*this.retries, &err, delay); RetryState::Sleeping { delay: this.clock.sleep(delay), } } - None => return Poll::Ready(Err(err)), + Decision::Stop { reason, elapsed: m } => { + return Poll::Ready(Err(give_up( + err, + *this.retries, + reason, + m, + elapsed, + ))); + } } } }, @@ -236,6 +270,7 @@ where mod tests { use super::*; use crate::backoff::{ExponentialBackoff, ExponentialBackoffConfig}; + use crate::error::StopReason; use std::sync::atomic::{AtomicUsize, Ordering::SeqCst}; use std::sync::{Arc, Mutex}; use std::time::Instant; @@ -295,8 +330,9 @@ mod tests { #[tokio::test] async fn succeeds_first_try() { let clock = MockClock::new(); - let result: Result = retry(|| async { Ok(42) }).clock(clock.clone()).await; - assert_eq!(result, Ok(42)); + let result: Result> = + retry(|| async { Ok(42) }).clock(clock.clone()).await; + assert_eq!(result.unwrap(), 42); assert!(clock.slept().is_empty()); // no retries → no sleeps } @@ -305,7 +341,7 @@ mod tests { let clock = MockClock::new(); let attempts = Arc::new(AtomicUsize::new(0)); let a = attempts.clone(); - let result: Result = retry(move || { + let result: Result> = retry(move || { let a = a.clone(); async move { let n = a.fetch_add(1, SeqCst); @@ -316,7 +352,7 @@ mod tests { .clock(clock.clone()) .await; - assert_eq!(result, Ok(42)); + assert_eq!(result.unwrap(), 42); assert_eq!(attempts.load(SeqCst), 3); // 2 failures + 1 success assert_eq!(clock.slept(), vec![secs(1), secs(2)]); // slept after each failure } @@ -326,7 +362,7 @@ mod tests { let clock = MockClock::new(); let attempts = Arc::new(AtomicUsize::new(0)); let a = attempts.clone(); - let result: Result = retry(move || { + let result: Result> = retry(move || { let a = a.clone(); async move { a.fetch_add(1, SeqCst); @@ -338,7 +374,11 @@ mod tests { .when(|_e| false) // nothing is retryable .await; - assert_eq!(result, Err("nope")); + let err = result.unwrap_err(); + assert_eq!(*err.error(), "nope"); + assert_eq!(err.stop_reason(), StopReason::NotRetryable); + assert_eq!(err.attempts(), 1); // the rejected attempt still ran + assert_eq!(err.elapsed(), Duration::ZERO); assert_eq!(attempts.load(SeqCst), 1); // exactly one attempt assert!(clock.slept().is_empty()); } @@ -346,25 +386,35 @@ mod tests { #[tokio::test] async fn exhausts_retries() { let clock = MockClock::new(); - let result: Result = retry(|| async { Err("always") }) + let result: Result> = retry(|| async { Err("always") }) .backoff(backoff(3)) .clock(clock.clone()) .await; - assert_eq!(result, Err("always")); + let err = result.unwrap_err(); + assert_eq!(*err.error(), "always"); + assert_eq!(err.stop_reason(), StopReason::RetriesExhausted); + assert_eq!(err.attempts(), 4); // 3 retries -> 4 attempts + assert_eq!(err.elapsed(), secs(7)); // 1 + 2 + 4 on the mock clock assert_eq!(clock.slept().len(), 3); // 3 retries → 3 sleeps, then give up } #[tokio::test] async fn stops_on_time_budget() { let clock = MockClock::new(); - let result: Result = retry(|| async { Err("slow") }) + let result: Result> = retry(|| async { Err("slow") }) .backoff(backoff(100)) // effectively unlimited retries .clock(clock.clone()) .max_elapsed(secs(10)) .await; - assert_eq!(result, Err("slow")); + let err = result.unwrap_err(); + assert_eq!(*err.error(), "slow"); + assert_eq!(err.stop_reason(), StopReason::MaxElapsed); + assert!( + err.elapsed() < secs(10), + "elapsed must stay under the budget" + ); // 1 (→1s), 2 (→3s), 4 (→7s); next would be 8 → 7+8=15 ≥ 10 → stop. assert_eq!(clock.slept(), vec![secs(1), secs(2), secs(4)]); } @@ -379,7 +429,7 @@ mod tests { let greeting = String::from("hi"); let attempts = AtomicUsize::new(0); - let out: Result = retry(|| async { + let out: Result> = retry(|| async { let n = attempts.fetch_add(1, SeqCst); if n < 2 { Err("transient") @@ -391,7 +441,7 @@ mod tests { .clock(clock.clone()) .await; - assert_eq!(out, Ok("hi!".to_string())); + assert_eq!(out.unwrap(), "hi!"); assert_eq!(attempts.load(SeqCst), 3); let _ = greeting; // still owned here — it was borrowed, not moved } @@ -406,7 +456,7 @@ mod tests { let clock = MockClock::new(); let shared = Rc::new(Cell::new(0)); - let out: Result = retry({ + let out: Result> = retry({ let shared = shared.clone(); move || { let shared = shared.clone(); @@ -424,7 +474,7 @@ mod tests { .clock(clock.clone()) .await; - assert_eq!(out, Ok(2)); + assert_eq!(out.unwrap(), 2); } // --- realistic usage patterns --- @@ -440,7 +490,7 @@ mod tests { let clock = MockClock::new(); let attempts = Arc::new(AtomicUsize::new(0)); let a = attempts.clone(); - let out: Result = retry(move || { + let out: Result> = retry(move || { let a = a.clone(); async move { match a.fetch_add(1, SeqCst) { @@ -454,7 +504,7 @@ mod tests { .when(|e| matches!(e, ApiError::Transient)) .await; - assert_eq!(out, Err(ApiError::Fatal)); + assert_eq!(*out.unwrap_err().error(), ApiError::Fatal); assert_eq!(attempts.load(SeqCst), 3); // transient, transient, fatal → stop assert_eq!(clock.slept(), vec![secs(1), secs(2)]); // slept only after the transients } @@ -464,7 +514,7 @@ mod tests { // The op is `FnMut`, so it can mutate captured state directly — no Arc/atomic needed. let clock = MockClock::new(); let mut calls = 0; - let out: Result = retry(|| { + let out: Result> = retry(|| { calls += 1; let n = calls; async move { if n < 3 { Err("transient") } else { Ok(n) } } @@ -473,29 +523,59 @@ mod tests { .clock(clock.clone()) .await; - assert_eq!(out, Ok(3)); + assert_eq!(out.unwrap(), 3); assert_eq!(calls, 3); // mutated across attempts through a &mut capture } #[tokio::test] - async fn reads_clock_only_when_a_budget_is_set() { - // Perf contract: with no `max_elapsed` we never read the clock in the retry loop — - // only the single `start` read at construction. + async fn clock_reads_do_not_scale_with_attempts() { + // Perf contract: with no `max_elapsed` the retry loop never touches the clock. Two reads + // total, whatever the retry count — `start` on the first poll, and one at the end to + // measure `elapsed`. Asserting both 3 and 30 retries pins the *slope*, which is the part + // that matters; a single count would still pass if the loop started reading per attempt. + for retries in [3, 30] { + let clock = MockClock::new(); + let _: Result> = retry(|| async { Err("x") }) + .backoff(backoff(retries)) + .clock(clock.clone()) + .await; + assert_eq!(clock.now_calls(), 2, "with {retries} retries and no budget"); + } + + // With a budget, one extra read per retry decision, plus start and the terminal read. let clock = MockClock::new(); - let _: Result = retry(|| async { Err("x") }) + let _: Result> = retry(|| async { Err("x") }) .backoff(backoff(3)) .clock(clock.clone()) + .max_elapsed(secs(1000)) .await; - assert_eq!(clock.now_calls(), 1); // just `start` + assert_eq!(clock.now_calls(), 5); // start + 3 decisions + terminal - // With a budget, one extra read per retry decision (3 retries here). + // Stopping *on* the budget reuses the read the budget check just did, so that path costs + // no more than it did before `elapsed` existed. let clock = MockClock::new(); - let _: Result = retry(|| async { Err("x") }) - .backoff(backoff(3)) + let _: Result> = retry(|| async { Err("x") }) + .backoff(backoff(100)) .clock(clock.clone()) - .max_elapsed(secs(1000)) + .max_elapsed(secs(10)) .await; - assert_eq!(clock.now_calls(), 4); // start + 3 + assert_eq!(clock.now_calls(), 5); // start + 4 decisions, no extra terminal read + } + + #[tokio::test] + async fn elapsed_excludes_time_parked_before_the_first_poll() { + // `start` is sampled on the first poll, not at `.into_future()`. A future can sit unpolled + // in a `FuturesUnordered`, and that wait isn't time the operation spent. + let clock = MockClock::new(); + let fut = retry(|| async { Err::("x") }) + .backoff(backoff(1)) + .clock(clock.clone()) + .into_future(); + + clock.sleep(secs(100)).await; // parked; nobody has polled `fut` yet + + let err = fut.await.unwrap_err(); + assert_eq!(err.elapsed(), secs(1)); // only the one backoff delay, not the 100s park } // --- suspension, wakers, and the real Tokio timer (the mock never goes Pending) --- @@ -507,7 +587,7 @@ mod tests { let clock = MockClock::new(); let attempts = Arc::new(AtomicUsize::new(0)); let a = attempts.clone(); - let out: Result = retry(move || { + let out: Result> = retry(move || { let a = a.clone(); async move { tokio::task::yield_now().await; // suspend mid-attempt @@ -522,7 +602,7 @@ mod tests { .clock(clock.clone()) .await; - assert_eq!(out, Ok(7)); + assert_eq!(out.unwrap(), 7); assert_eq!(attempts.load(SeqCst), 2); } @@ -534,7 +614,7 @@ mod tests { let attempts = Arc::new(AtomicUsize::new(0)); let a = attempts.clone(); let start = tokio::time::Instant::now(); - let out: Result = retry(move || { + let out: Result> = retry(move || { let a = a.clone(); async move { if a.fetch_add(1, SeqCst) < 2 { @@ -547,7 +627,7 @@ mod tests { .backoff(backoff(5)) // delays: 1s, 2s .await; - assert_eq!(out, Ok(9)); + assert_eq!(out.unwrap(), 9); assert_eq!(attempts.load(SeqCst), 3); assert_eq!(start.elapsed(), secs(3)); // really waited 1s + 2s of (virtual) time } @@ -558,7 +638,7 @@ mod tests { // passes because `now` and `sleep` share a time source — otherwise it would never stop. let attempts = Arc::new(AtomicUsize::new(0)); let a = attempts.clone(); - let out: Result = retry(move || { + let out: Result> = retry(move || { let a = a.clone(); async move { a.fetch_add(1, SeqCst); @@ -569,7 +649,7 @@ mod tests { .max_elapsed(secs(10)) .await; - assert_eq!(out, Err("slow")); + assert_eq!(*out.unwrap_err().error(), "slow"); assert_eq!(attempts.load(SeqCst), 4); // 1(→1s) 2(→3s) 4(→7s); next 8 → 15 ≥ 10, stop } @@ -615,12 +695,12 @@ mod tests { max_delay: Duration::from_nanos(1), }) .unwrap(); - let out: Result = retry(|| async { Err("always") }) + let out: Result> = retry(|| async { Err("always") }) .backoff(big) .clock(clock.clone()) .await; - assert_eq!(out, Err("always")); + assert_eq!(*out.unwrap_err().error(), "always"); assert_eq!(clock.slept().len(), 5000); // 5000 retries, then give up } @@ -668,7 +748,7 @@ mod tests { let attempts = Arc::new(AtomicUsize::new(0)); let a = attempts.clone(); - let out: Result = retry(move || { + let out: Result> = retry(move || { let a = a.clone(); async move { if a.fetch_add(1, SeqCst) < 2 { @@ -682,10 +762,36 @@ mod tests { .clock(clock) .await; - assert_eq!(out, Ok(42)); + assert_eq!(out.unwrap(), 42); assert_eq!(events.get(), 2); // one event per retry } + #[tokio::test] + async fn give_up_event_fires_once_and_only_after_a_retry() { + // Exhausting retries: one event per retry, plus one for giving up. + let events = crate::test_support::count_retry_events(); + let clock = MockClock::new(); + let _: Result> = retry(|| async { Err("x") }) + .backoff(backoff(3)) + .clock(clock) + .await; + assert_eq!(events.get(), 4); // 3 retries + 1 give-up + } + + #[tokio::test] + async fn non_retryable_first_error_is_silent() { + // A `.when` filter in front of an HTTP client would otherwise WARN on every 404, on a + // path that emitted nothing before the give-up event existed. + let events = crate::test_support::count_retry_events(); + let clock = MockClock::new(); + let _: Result> = retry(|| async { Err("x") }) + .backoff(backoff(3)) + .clock(clock) + .when(|_| false) + .await; + assert_eq!(events.get(), 0); + } + // --- randomized strategies driven through the real driver --- #[tokio::test] @@ -696,7 +802,7 @@ mod tests { use crate::backoff::{DecorrelatedBackoff, DecorrelatedBackoffConfig}; let clock = MockClock::new(); - let out: Result = retry(|| async { Err("boom") }) + let out: Result> = retry(|| async { Err("boom") }) .backoff( DecorrelatedBackoff::with_seed( DecorrelatedBackoffConfig { @@ -711,7 +817,7 @@ mod tests { .clock(clock.clone()) .await; - assert_eq!(out, Err("boom")); + assert_eq!(*out.unwrap_err().error(), "boom"); let slept = clock.slept(); assert_eq!(slept.len(), 4); // max_retries sleeps, then give up assert!( @@ -742,12 +848,12 @@ mod tests { #[tokio::test] async fn drives_a_jittered_backoff() { let clock = MockClock::new(); - let out: Result = retry(|| async { Err("boom") }) + let out: Result> = retry(|| async { Err("boom") }) .backoff(crate::backoff::Jittered::with_seed(backoff(3), 42)) .clock(clock.clone()) .await; - assert_eq!(out, Err("boom")); + assert_eq!(*out.unwrap_err().error(), "boom"); // Underlying exponential is 1s, 2s, 4s; full jitter can only shrink each one. let slept = clock.slept(); assert_eq!(slept.len(), 3); diff --git a/src/shared.rs b/src/shared.rs index f6a73c0..ce1c68a 100644 --- a/src/shared.rs +++ b/src/shared.rs @@ -1,36 +1,82 @@ //! Retry internals shared by the async and blocking drivers: the decision of whether to keep -//! going, and the per-retry `tracing` event. Kept here (not in either driver) so both call the -//! same code and neither depends on the other's feature being enabled. +//! going, how the terminal error is built, and the `tracing` events. Kept here (not in either +//! driver) so both call the same code and neither depends on the other's feature being enabled. use crate::backoff::Backoff; +use crate::error::{RetryError, StopReason}; use std::time::Duration; +/// What to do after a failed attempt. +/// +/// `Stop` carries the reason so the driver doesn't have to re-derive it, and an `elapsed` that is +/// `Some` only on the `MaxElapsed` path, where the budget check already read the clock in this +/// same call. Reusing it there keeps the give-up path at one clock read, never two. +pub(crate) enum Decision { + Retry(Duration), + Stop { + reason: StopReason, + elapsed: Option, + }, +} + /// The retry decision, shared by both drivers so they can't diverge on semantics. /// -/// After a failed attempt, returns `Some(delay)` to wait `delay` and try again, or `None` to -/// give up (non-retryable error, retries exhausted, or the time budget is spent). Pure: -/// `elapsed` is a thunk, so the clock is read only when a budget is actually set. +/// Pure: `elapsed` is a thunk, so the clock is read only when a budget is actually set. pub(crate) fn should_retry_after( err: &E, when: &P, backoff: &mut B, max_elapsed: Option, elapsed: impl FnOnce() -> Duration, -) -> Option +) -> Decision where P: Fn(&E) -> bool, B: Backoff, { if !when(err) { - return None; // non-retryable + return Decision::Stop { + reason: StopReason::NotRetryable, + elapsed: None, + }; } - let delay = backoff.next_delay()?; // None = retries exhausted + let Some(delay) = backoff.next_delay() else { + return Decision::Stop { + reason: StopReason::RetriesExhausted, + elapsed: None, + }; + }; if let Some(budget) = max_elapsed { - if elapsed().saturating_add(delay) >= budget { - return None; // no time for another attempt + let spent = elapsed(); + if spent.saturating_add(delay) >= budget { + // No time for another attempt. We just read the clock, so hand the value on rather + // than making the driver read it again. + return Decision::Stop { + reason: StopReason::MaxElapsed, + elapsed: Some(spent), + }; } } - Some(delay) + Decision::Retry(delay) +} + +/// Build the terminal [`RetryError`] and emit the give-up event. +/// +/// `retries` is how many retries were performed, so the attempt count is one more than that: even +/// a first error rejected by `.when(..)` means the operation ran once. Both drivers go through +/// here so that `+1`, the fallback clock read, and the event gate can't drift apart. +pub(crate) fn give_up( + error: E, + retries: u32, + reason: StopReason, + measured: Option, + measure: impl FnOnce() -> Duration, +) -> RetryError { + let elapsed = measured.unwrap_or_else(measure); + let attempts = retries.saturating_add(1); + if retries > 0 { + trace_give_up(attempts, elapsed, reason); + } + RetryError::new(error, attempts, elapsed, reason) } /// Emit a `tracing` event for one retry. Shared by both drivers so they report identically. @@ -45,3 +91,18 @@ pub(crate) fn trace_retry(attempt: u32, error: &E, delay: Du "retrying after error" ); } + +/// Emit the one event that closes out a retry that gave up, on the same target as the per-retry +/// events. Carries no error: the caller now holds it, and the last retry event already logged it. +/// +/// Only fires when at least one retry happened. Without that gate, putting a `.when(..)` filter in +/// front of an HTTP client would emit a `WARN` for every 404, on a path that is silent today. +fn trace_give_up(attempts: u32, elapsed: Duration, reason: StopReason) { + tracing::warn!( + target: "mettle::retry", + attempts, + elapsed_ms = elapsed.as_millis() as u64, + reason = reason.as_str(), + "gave up" + ); +} From ae3a5d26feb90986d22670cedcb2b4da1b9c89ab Mon Sep 17 00:00:00 2001 From: Azeem Shaik Date: Sun, 9 Aug 2026 17:25:11 +0530 Subject: [PATCH 2/3] Rebuild RetryError on the released 0.3.0 The old branch was based on the pre-review jitter tip, so it was missing the six commits main gained during review, and its ADR005 collided head-on with the ADR005 that shipped in 0.3.0. Rebuilt off main, carrying only the RetryError work. Its ADR is now ADR006; main's ADR005 (testability) keeps the number it shipped under. Four conflicts, all real: both sides had added an ADR005, both had added tests at the same point in the blocking module, and both had written a new crate docs section and a new index row. The blocking tests main added during review are kept and retyped for the new `call()` signature, so the driver parity that release established survives the rebuild: both drivers now carry the same four tests. CHANGELOG entry rewritten in the style 0.3.0 settled on -- what changed and what to do about it, with the reasoning in the ADR. The old branch is preserved as archive/retry-error-pre-rebuild. --- CHANGELOG.md | 43 ++++++++++++++++++++++++++++++------------- Cargo.lock | 2 +- Cargo.toml | 2 +- src/blocking/retry.rs | 6 ++++-- src/retry.rs | 4 ++-- 5 files changed, 38 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 29d9ee2..19d6244 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,23 +9,40 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [0.4.0] - 2026-08-09 +Retry now reports why it gave up, not just what failed last. Breaking. + +### Added +- `RetryError`, with `error()`, `into_error()`, `attempts()`, `elapsed()` and `stop_reason()`. +- `StopReason`: `RetriesExhausted`, `NotRetryable`, `MaxElapsed`, plus `as_str()` for metric labels. + ### Changed -- **Breaking:** `retry(..).await` and `blocking::retry(..).call()` now fail with `RetryError` - instead of `E`. It carries the last error plus `attempts()`, `elapsed()`, and a `stop_reason()` - of `RetriesExhausted` / `NotRetryable` / `MaxElapsed`, so a failed retry says which limit it hit - rather than leaving you to guess. To keep the old signature, add - `.map_err(RetryError::into_error)`. - - `?` into `Box` and `anyhow::Error` keeps working and now covers error types it didn't - before (`String`, `Box`, `anyhow::Error`), because `RetryError` implements - `std::error::Error` for `E: Debug + Display` rather than `E: Error`. The trade is that it has no - `source()`; the inner error's text rides along in `Display`. ADR005 has the reasoning. -- **Breaking:** giving up now emits one more `tracing` event on target `mettle::retry`, with - `attempts`, `elapsed_ms`, and `reason`, but only when at least one retry actually happened. - Anyone counting events on that target sees a change. +- **Breaking:** `retry(..).await` and `blocking::retry(..).call()` fail with `RetryError` instead + of `E`. +- **Breaking:** giving up emits one more `tracing` event on `mettle::retry`, carrying `attempts`, + `elapsed_ms` and `reason`. It fires only when at least one retry happened, so a `.when(..)` filter + rejecting the first error stays as quiet as it was. - The async driver starts its clock on the first poll rather than at `.into_future()`, matching the blocking driver. A future parked before its first poll no longer bills that time to the operation. +### Upgrading + +Wherever you name the error type: + +```diff +-let value: Result = retry(op).await; ++let value: Result> = retry(op).await; +``` + +`?` into `Box` or `anyhow::Error` keeps working, and now covers error types it didn't +before: `String`, `Box` and `anyhow::Error` all satisfy the new bound. To go back to the +bare error and keep an existing signature, add `.map_err(RetryError::into_error)`. + +Matching on the error goes through an accessor, so `match e` becomes `match e.error()`. + +`RetryError` deliberately has no `source()`. Why, and why its `Error` impl is bounded on +`E: Debug + Display` rather than `E: Error`: +[ADR006](https://github.com/azeemshaik025/mettle/blob/main/docs/adr/ADR006.md). + ## [0.3.0] - 2026-08-09 Adds jitter. Purely additive apart from one collision noted under Upgrading. diff --git a/Cargo.lock b/Cargo.lock index 81a7c27..e45640b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10,7 +10,7 @@ checksum = "da7c62ceae207dd37ea5b845da6a0696c799f85e97da1ab5b7910be3c1c80223" [[package]] name = "mettle" -version = "0.3.0" +version = "0.4.0" dependencies = [ "fastrand", "pin-project-lite", diff --git a/Cargo.toml b/Cargo.toml index 5e74194..9f3bc6f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mettle" -version = "0.3.0" +version = "0.4.0" edition = "2024" rust-version = "1.85" description = "A resilience toolkit for Rust: async and blocking retries with configurable backoff and jitter." diff --git a/src/blocking/retry.rs b/src/blocking/retry.rs index ba46160..aa69dd5 100644 --- a/src/blocking/retry.rs +++ b/src/blocking/retry.rs @@ -332,7 +332,8 @@ mod tests { // `.clock(c)` takes the clock by value, so without the reference impls a test could hand // over its mock and never read it back. Both forms must reach the same mock. let clock = MockClock::new(); - let _: Result> = retry(|| Err("x")).backoff(backoff(2)).clock(&clock).call(); + let _: Result> = + retry(|| Err("x")).backoff(backoff(2)).clock(&clock).call(); assert_eq!(clock.slept(), vec![secs(1), secs(2)]); let shared = std::sync::Arc::new(MockClock::new()); @@ -341,9 +342,10 @@ mod tests { .clock(Arc::clone(&shared)) .call(); assert_eq!(shared.slept(), vec![secs(1), secs(2)]); + } #[test] - fn clock_reads_do_not_scale_with_attempts() { + fn clock_reads_do_not_scale_with_attempts() { // The async twin of this lives in src/retry.rs. `elapsed()` is public now, so both // drivers have to agree on what it costs. for retries in [3, 30] { diff --git a/src/retry.rs b/src/retry.rs index cc79fbf..775313b 100644 --- a/src/retry.rs +++ b/src/retry.rs @@ -831,14 +831,14 @@ mod tests { // `.clock(c)` takes the clock by value, so without the reference impls a test could hand // over its mock and never read it back. The blocking twin of this is in blocking/retry.rs. let clock = MockClock::new(); - let _: Result = retry(|| async { Err("x") }) + let _: Result> = retry(|| async { Err("x") }) .backoff(backoff(2)) .clock(&clock) .await; assert_eq!(clock.slept(), vec![secs(1), secs(2)]); let shared = Arc::new(MockClock::new()); - let _: Result = retry(|| async { Err("x") }) + let _: Result> = retry(|| async { Err("x") }) .backoff(backoff(2)) .clock(Arc::clone(&shared)) .await; From 264f1602886a4fce51306985b15596c5b11c5333 Mon Sep 17 00:00:00 2001 From: Azeem Shaik Date: Sun, 9 Aug 2026 17:28:30 +0530 Subject: [PATCH 3/3] Add jitter to the keywords, and the assert_eq upgrade note Outside-in pass on 0.4.0, written as a user following the CHANGELOG. Everything the release claims holds. `?` into anyhow::Result works for io::Error, String, Box and anyhow::Error itself -- the last three do not implement std::error::Error, which is the entire reason ADR006 bounds the impl on Debug + Display. downcast_ref recovers attempts and stop_reason, which is what ADR006 offers in place of source(). Both drivers verified: when() gives not_retryable with attempts 1, max_elapsed gives an elapsed strictly under the budget. Two gaps found. The upgrade guide covered `match e` but not assert_eq! on the whole Result, which is what every existing test does and which fails with a bare E0369 that does not say what to do. And keywords still lacked `jitter` -- swapped for `async`, the most generic of the five and already implied by `tokio`. --- CHANGELOG.md | 11 +++++++++++ Cargo.toml | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 19d6244..254084b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,17 @@ bare error and keep an existing signature, add `.map_err(RetryError::into_error) Matching on the error goes through an accessor, so `match e` becomes `match e.error()`. +Tests that asserted on the whole `Result` need the error unwrapped. `RetryError` is deliberately +not `PartialEq`, and could not usefully be: its fields are private with no public constructor, so +there is no way to build the right-hand side to compare against. + +```diff +-assert_eq!(result, Err(MyError::Timeout)); ++assert_eq!(*result.unwrap_err().error(), MyError::Timeout); +-assert_eq!(result, Ok(42)); ++assert_eq!(result.unwrap(), 42); +``` + `RetryError` deliberately has no `source()`. Why, and why its `Error` impl is bounded on `E: Debug + Display` rather than `E: Error`: [ADR006](https://github.com/azeemshaik025/mettle/blob/main/docs/adr/ADR006.md). diff --git a/Cargo.toml b/Cargo.toml index 9f3bc6f..d5577ca 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,7 +6,7 @@ rust-version = "1.85" description = "A resilience toolkit for Rust: async and blocking retries with configurable backoff and jitter." readme = "README.md" license = "MIT OR Apache-2.0" -keywords = ["retry", "backoff", "resilience", "async", "tokio"] +keywords = ["retry", "backoff", "jitter", "resilience", "tokio"] categories = ["asynchronous", "network-programming"] repository = "https://github.com/azeemshaik025/mettle" documentation = "https://docs.rs/mettle"