Provider error taxonomy + retry/backoff ([A6] / #13)

core/src/agent/provider/gemini.rs

@@ -77,13 +77,14 @@ impl LlmProvider for GeminiProvider {
             .map_err(|e| ProviderError::Transport(e.to_string()))?;
 
         let status = resp.status();
+        let retry_after = parse_retry_after(resp.headers());
         let text = resp
             .text()
             .await
             .map_err(|e| ProviderError::Transport(e.to_string()))?;
 
         if !status.is_success() {
-            return Err(classify_http_error(status.as_u16(), &text));
+            return Err(classify_http_error(status.as_u16(), &text, retry_after));
         }
 
         let wire: GeminiResponseBody = serde_json::from_str(&text)
@@ -93,19 +94,40 @@ impl LlmProvider for GeminiProvider {
 }
 
 /// Map a non-2xx HTTP response to a typed error, pulling Gemini's
-/// `error.message` when present.
-fn classify_http_error(status: u16, body: &str) -> ProviderError {
+/// `error.message` when present. The retry classifier ([A6]) decides
+/// what's worth a second attempt — this just picks the variant; see
+/// [`ProviderError::is_retryable`].
+fn classify_http_error(
+    status: u16,
+    body: &str,
+    retry_after: Option<std::time::Duration>,
+) -> ProviderError {
     let message = serde_json::from_str::<GeminiErrorEnvelope>(body)
         .ok()
         .map(|e| e.error.message)
         .unwrap_or_else(|| body.to_string());
     match status {
         400 => ProviderError::InvalidRequest(message),
         401 | 403 => ProviderError::Auth(message),
+        429 => ProviderError::RateLimit {
+            message,
+            retry_after,
+        },
+        500..=599 => ProviderError::Transient(format!("HTTP {status}: {message}")),
         _ => ProviderError::Provider(format!("HTTP {status}: {message}")),
     }
 }
 
+/// Parse a `Retry-After` header value as seconds. Gemini sends an integer
+/// seconds count rather than the HTTP-date alternative the spec permits,
+/// so we only handle the seconds case — an unparseable header yields
+/// `None` and the retry loop falls back to exponential backoff.
+fn parse_retry_after(headers: &reqwest::header::HeaderMap) -> Option<std::time::Duration> {
+    let raw = headers.get(reqwest::header::RETRY_AFTER)?.to_str().ok()?;
+    let secs: u64 = raw.trim().parse().ok()?;
+    Some(std::time::Duration::from_secs(secs))
+}
+
 // ---------------------------------------------------------------------------
 // Neutral -> Gemini wire
 // ---------------------------------------------------------------------------
@@ -511,9 +533,70 @@ mod tests {
     #[test]
     fn http_errors_are_classified() {
         let body = r#"{"error": {"code": 403, "message": "API key not valid", "status": "PERMISSION_DENIED"}}"#;
-        assert!(matches!(classify_http_error(403, body), ProviderError::Auth(m) if m.contains("not valid")));
-        assert!(matches!(classify_http_error(400, body), ProviderError::InvalidRequest(_)));
-        assert!(matches!(classify_http_error(503, body), ProviderError::Provider(_)));
+        assert!(matches!(
+            classify_http_error(403, body, None),
+            ProviderError::Auth(m) if m.contains("not valid")
+        ));
+        assert!(matches!(
+            classify_http_error(400, body, None),
+            ProviderError::InvalidRequest(_)
+        ));
+    }
+
+    /// 5xx maps to `Transient` so the retry loop ([A6]) backs off and
+    /// tries again; the message preserves the HTTP status for debugging.
+    #[test]
+    fn five_hundreds_classify_as_transient() {
+        let body = r#"{"error": {"code": 503, "message": "backend overloaded"}}"#;
+        for status in [500, 502, 503, 504] {
+            let err = classify_http_error(status, body, None);
+            assert!(
+                matches!(&err, ProviderError::Transient(m) if m.contains(&status.to_string())),
+                "expected Transient for {status}, got {err:?}"
+            );
+            assert!(err.is_retryable());
+        }
+    }
+
+    /// 429 maps to `RateLimit`; a `Retry-After` header threads through
+    /// to the variant so the retry helper honours it verbatim.
+    #[test]
+    fn rate_limited_passes_retry_after_through() {
+        let body = r#"{"error": {"code": 429, "message": "quota exceeded"}}"#;
+        let hint = std::time::Duration::from_secs(7);
+        let err = classify_http_error(429, body, Some(hint));
+        let ProviderError::RateLimit { retry_after, message } = err else {
+            panic!("expected RateLimit");
+        };
+        assert_eq!(retry_after, Some(hint));
+        assert_eq!(message, "quota exceeded");
+    }
+
+    /// 4xx codes we don't model explicitly stay in the unclassified
+    /// `Provider` bucket (not retried).
+    #[test]
+    fn unmodeled_4xx_stays_in_provider_bucket() {
+        let body = r#"{"error": {"code": 404, "message": "model not found"}}"#;
+        let err = classify_http_error(404, body, None);
+        assert!(matches!(&err, ProviderError::Provider(m) if m.contains("404")));
+        assert!(!err.is_retryable());
+    }
+
+    #[test]
+    fn parse_retry_after_handles_integer_seconds() {
+        use reqwest::header::{HeaderMap, HeaderValue, RETRY_AFTER};
+        let mut h = HeaderMap::new();
+        h.insert(RETRY_AFTER, HeaderValue::from_static("12"));
+        assert_eq!(parse_retry_after(&h), Some(std::time::Duration::from_secs(12)));
+
+        // Non-numeric (e.g. HTTP-date format) yields None — caller falls
+        // back to exponential backoff.
+        let mut h = HeaderMap::new();
+        h.insert(RETRY_AFTER, HeaderValue::from_static("Wed, 21 Oct 2026 07:28:00 GMT"));
+        assert!(parse_retry_after(&h).is_none());
+
+        // Missing header.
+        assert!(parse_retry_after(&HeaderMap::new()).is_none());
     }
 
     /// Live smoke test against the real Gemini API. Ignored by default; run with

core/src/agent/provider/mod.rs

@@ -23,7 +23,9 @@ use async_trait::async_trait;
 use serde::{Deserialize, Serialize};
 
 pub mod gemini;
+pub mod retry;
 pub use gemini::GeminiProvider;
+pub use retry::{retry_with_backoff, RetryConfig};
 
 /// Who authored a message in the conversation.
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
@@ -251,27 +253,78 @@ pub struct ChatResponse {
     pub finish_reason: FinishReason,
 }
 
-/// Errors a provider can surface. Intentionally small for `#8`; `#13`
-/// extends this with rate-limit/transient classification and backoff.
+/// Errors a provider can surface. Classification is what the retry loop
+/// (`#13`) branches on — see [`ProviderError::is_retryable`] and
+/// [`ProviderError::retry_after`].
 #[derive(Debug, thiserror::Error)]
 pub enum ProviderError {
-    /// Authentication failed (missing/invalid API key). Not retryable.
+    /// Authentication failed (missing/invalid API key). Not retryable —
+    /// retrying will not change the answer; surface to the UI immediately.
     #[error("provider auth failed: {0}")]
     Auth(String),
-    /// The request was rejected as malformed (bad model id, unsupported field).
+    /// The request was rejected as malformed (bad model id, unsupported
+    /// field). Not retryable — retrying will not change the answer.
     #[error("invalid request: {0}")]
     InvalidRequest(String),
-    /// Transport / network failure talking to the provider.
+    /// Transport / network failure talking to the provider. Retryable —
+    /// a TCP reset, DNS hiccup, or proxy blip should not bubble up as a
+    /// hard failure to the user the first time.
     #[error("transport error: {0}")]
     Transport(String),
-    /// The provider returned a response we couldn't parse into neutral types.
+    /// The provider returned a response we couldn't parse into neutral
+    /// types. Not retryable — the bug is in our decoder or the provider's
+    /// wire format, not in the network.
     #[error("could not parse provider response: {0}")]
     Decode(String),
-    /// Provider-side error not otherwise classified (5xx, quota, etc.).
+    /// Provider-side rate limit / quota response (typically HTTP 429).
+    /// Retryable. `retry_after` carries the server's hint (the
+    /// `Retry-After` header) when present; the retry loop should honour
+    /// it instead of its own backoff schedule.
+    #[error("rate limited: {message}")]
+    RateLimit {
+        message: String,
+        retry_after: Option<std::time::Duration>,
+    },
+    /// Transient provider failure (typically HTTP 5xx, timeouts).
+    /// Retryable with exponential backoff.
+    #[error("transient provider error: {0}")]
+    Transient(String),
+    /// Provider-side error not otherwise classified — bucket for shapes
+    /// we don't model explicitly. Not retried by default to avoid
+    /// hammering a provider with a request it has already rejected for a
+    /// non-transient reason.
     #[error("provider error: {0}")]
     Provider(String),
 }
 
+impl ProviderError {
+    /// True if the retry loop should attempt this call again. Drives the
+    /// `retry_with_backoff` decision tree alongside the configured retry
+    /// budget.
+    ///
+    /// Retryable: [`Self::RateLimit`], [`Self::Transient`], [`Self::Transport`].
+    /// Not retryable: [`Self::Auth`], [`Self::InvalidRequest`],
+    /// [`Self::Decode`], [`Self::Provider`].
+    pub fn is_retryable(&self) -> bool {
+        matches!(
+            self,
+            ProviderError::RateLimit { .. }
+                | ProviderError::Transient(_)
+                | ProviderError::Transport(_)
+        )
+    }
+
+    /// The provider's explicit "wait at least this long" hint, if any.
+    /// Currently only [`Self::RateLimit`] carries this — `Retry-After`
+    /// headers are the only place providers reliably set it.
+    pub fn retry_after(&self) -> Option<std::time::Duration> {
+        match self {
+            ProviderError::RateLimit { retry_after, .. } => *retry_after,
+            _ => None,
+        }
+    }
+}
+
 /// The contract every LLM provider implements. One method: take a neutral
 /// [`ChatRequest`], return a neutral [`ChatResponse`]. The API key is passed
 /// per call (never stored on the provider) so it can come from the OS
@@ -389,6 +442,36 @@ mod tests {
         }
     }
 
+    #[test]
+    fn is_retryable_matches_classification_table() {
+        // Retryable.
+        assert!(ProviderError::Transport("eof".into()).is_retryable());
+        assert!(ProviderError::Transient("5xx".into()).is_retryable());
+        assert!(ProviderError::RateLimit {
+            message: "slow down".into(),
+            retry_after: None,
+        }
+        .is_retryable());
+        // Not retryable.
+        assert!(!ProviderError::Auth("bad key".into()).is_retryable());
+        assert!(!ProviderError::InvalidRequest("bad model".into()).is_retryable());
+        assert!(!ProviderError::Decode("bad json".into()).is_retryable());
+        assert!(!ProviderError::Provider("safety".into()).is_retryable());
+    }
+
+    #[test]
+    fn retry_after_only_set_for_rate_limit() {
+        let d = std::time::Duration::from_secs(2);
+        let rl = ProviderError::RateLimit {
+            message: "wait".into(),
+            retry_after: Some(d),
+        };
+        assert_eq!(rl.retry_after(), Some(d));
+
+        assert!(ProviderError::Transient("x".into()).retry_after().is_none());
+        assert!(ProviderError::Transport("x".into()).retry_after().is_none());
+    }
+
     #[tokio::test]
     async fn provider_trait_is_object_safe_and_callable() {
         let provider: std::sync::Arc<dyn LlmProvider> = std::sync::Arc::new(EchoProvider);