From a1b31e89cd662b7b0373340e7a5f5551ea3f8811 Mon Sep 17 00:00:00 2001
From: Destynova2 <cliard@a00.fr>
Date: Tue, 27 Oct 2026 16:19:22 +0100
Subject: [PATCH] fix(oauth): clear refresh marker on connection failures

A refresh that fails before the TCP/TLS connection is established never
reaches the issuer, so the stored refresh token is neither spent nor
rotated. refresh_token left the .refresh.pending uncertainty marker in
place on any error from do_refresh, so one transient connect failure
wedged every later refresh with "Previous OAuth refresh outcome is
unknown" until a manual re-auth, even though the refresh token was still
valid.

Clear the marker when do_refresh fails with a reqwest connection-phase
error (is_connect()); timeouts and post-send failures still keep it,
since there the outcome is genuinely unknown.

Fixes #663
---
 src/auth/oauth.rs | 69 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 68 insertions(+), 1 deletion(-)

diff --git a/src/auth/oauth.rs b/src/auth/oauth.rs
index 917112c7..94c8cde5 100644
--- a/src/auth/oauth.rs
+++ b/src/auth/oauth.rs
@@ -439,7 +439,23 @@ impl OAuthClient {
         }
 
         self.token_store.begin_refresh(&existing_token)?;
-        let response = self.do_refresh(&existing_token).await?;
+        let response = match self.do_refresh(&existing_token).await {
+            Ok(response) => response,
+            Err(error) => {
+                // A connection that never opened proves the issuer never received
+                // this request, so the refresh token was neither spent nor rotated.
+                // Clear the uncertainty marker so the next attempt can reuse the
+                // still-valid token, instead of wedging every future refresh on
+                // "Previous OAuth refresh outcome is unknown".
+                if error
+                    .downcast_ref::<reqwest::Error>()
+                    .is_some_and(reqwest::Error::is_connect)
+                {
+                    self.token_store.finish_refresh(provider_id)?;
+                }
+                return Err(error);
+            }
+        };
 
         if !response.status().is_success() {
             // A server/transport failure can occur after rotation. Only explicit
@@ -778,4 +794,55 @@ mod tests {
         let cfg = OAuthConfig::anthropic().with_callback_port(9999);
         assert_eq!(cfg.redirect_uri, original);
     }
+
+    /// A connection that never opens must not poison later refreshes.
+    ///
+    /// Regression test for the permanent wedge: a transient connect failure
+    /// (brief outage, DNS hiccup, daemon waking from sleep) used to leave the
+    /// `.refresh.pending` marker behind, so every subsequent refresh aborted
+    /// with "Previous OAuth refresh outcome is unknown" even though the stored
+    /// refresh token was still valid and had never reached the issuer.
+    #[tokio::test]
+    async fn connect_failure_does_not_wedge_future_refreshes() {
+        use crate::storage::GrobStore;
+        use chrono::Utc;
+        use secrecy::SecretString;
+        use std::sync::Arc;
+
+        let dir = tempfile::tempdir().unwrap();
+        let store = Arc::new(GrobStore::open(&dir.path().join("grob.db")).unwrap());
+        store
+            .save_oauth_token(&OAuthToken {
+                provider_id: "provider".into(),
+                access_token: SecretString::from("synthetic-access"),
+                refresh_token: SecretString::from("synthetic-refresh"),
+                expires_at: Utc::now() - chrono::Duration::minutes(1),
+                enterprise_url: None,
+                project_id: None,
+                needs_reauth: None,
+            })
+            .unwrap();
+
+        // Loopback port 1 has no listener: reqwest fails at connect, before any
+        // request bytes could reach an issuer, so the refresh token is provably
+        // neither spent nor rotated.
+        let mut config = OAuthConfig::openai_codex();
+        config.token_url = "http://127.0.0.1:1/token".into();
+        let client = OAuthClient::new(config, TokenStore::with_store(store).unwrap());
+
+        let first = client.refresh_token("provider").await.unwrap_err();
+        assert!(
+            !first.to_string().contains("outcome is unknown"),
+            "first refresh should surface the connect failure, not the wedge: {first:#}"
+        );
+
+        // Before the fix, the marker left by the first attempt made this second
+        // refresh fail with the uncertainty guard instead of retrying the
+        // still-valid token.
+        let second = client.refresh_token("provider").await.unwrap_err();
+        assert!(
+            !second.to_string().contains("outcome is unknown"),
+            "a transient connect failure must not wedge later refreshes: {second:#}"
+        );
+    }
 }