From 2c67a27a719d6db46ea29e3a4f306d34db52ace2 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Wed, 25 Mar 2026 16:03:53 -0600 Subject: [PATCH] Avoid duplicate auth refreshes in `getAuthStatus` (#15798) I've seen several intermittent failures of `get_auth_status_returns_token_after_proactive_refresh_recovery` today. I investigated, and I found a couple of issues. First, `getAuthStatus(refreshToken=true)` could refresh twice in one request: once via `refresh_token_if_requested()` and again via the proactive refresh path inside `auth_manager.auth()`. In the permanent-failure case this produced an extra `/oauth/token` call and made the app-server auth tests flaky. Use `auth_cached()` after an explicit refresh request so the handler reuses the post-refresh auth state instead of immediately re-entering proactive refresh logic. Keep the existing proactive path for `refreshToken=false`. Second, serialize auth refresh attempts in `AuthManager` have a startup/request race. One proactive refresh could already be in flight while a `getAuthStatus(refreshToken=false)` request entered `auth().await`, causing a second `/oauth/token` call before the first failure or refresh result had been recorded. Guarding the refresh flow with a single async lock makes concurrent callers share one refresh result, which prevents duplicate refreshes and stabilizes the proactive-refresh auth tests. --- codex-rs/Cargo.lock | 2 -- codex-rs/app-server/src/codex_message_processor.rs | 7 ++++++- codex-rs/login/src/auth/manager.rs | 13 ++++++++++++- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 5e3619137..fb1710987 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2451,8 +2451,6 @@ version = "0.0.0" dependencies = [ "codex-utils-absolute-path", "codex-utils-plugins", - "serde", - "serde_json", "thiserror 2.0.18", ] diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 22122a799..4687ec5ba 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -1403,7 +1403,12 @@ impl CodexMessageProcessor { requires_openai_auth: Some(false), } } else { - match self.auth_manager.auth().await { + let auth = if do_refresh { + self.auth_manager.auth_cached() + } else { + self.auth_manager.auth().await + }; + match auth { Some(auth) => { let permanent_refresh_failure = self.auth_manager.refresh_failure_for_auth(&auth).is_some(); diff --git a/codex-rs/login/src/auth/manager.rs b/codex-rs/login/src/auth/manager.rs index 94d546a56..34c0e7e27 100644 --- a/codex-rs/login/src/auth/manager.rs +++ b/codex-rs/login/src/auth/manager.rs @@ -12,6 +12,7 @@ use std::path::PathBuf; use std::sync::Arc; use std::sync::Mutex; use std::sync::RwLock; +use tokio::sync::Mutex as AsyncMutex; use codex_app_server_protocol::AuthMode as ApiAuthMode; use codex_protocol::config_types::ForcedLoginMethod; @@ -1038,6 +1039,7 @@ pub struct AuthManager { enable_codex_api_key_env: bool, auth_credentials_store_mode: AuthCredentialsStoreMode, forced_chatgpt_workspace_id: RwLock>, + refresh_lock: AsyncMutex<()>, } impl AuthManager { @@ -1067,6 +1069,7 @@ impl AuthManager { enable_codex_api_key_env, auth_credentials_store_mode, forced_chatgpt_workspace_id: RwLock::new(None), + refresh_lock: AsyncMutex::new(()), } } @@ -1084,6 +1087,7 @@ impl AuthManager { enable_codex_api_key_env: false, auth_credentials_store_mode: AuthCredentialsStoreMode::File, forced_chatgpt_workspace_id: RwLock::new(None), + refresh_lock: AsyncMutex::new(()), }) } @@ -1100,6 +1104,7 @@ impl AuthManager { enable_codex_api_key_env: false, auth_credentials_store_mode: AuthCredentialsStoreMode::File, forced_chatgpt_workspace_id: RwLock::new(None), + refresh_lock: AsyncMutex::new(()), }) } @@ -1307,6 +1312,7 @@ impl AuthManager { /// can assume that some other instance already refreshed it. If the persisted /// token is the same as the cached, then ask the token authority to refresh. pub async fn refresh_token(&self) -> Result<(), RefreshTokenError> { + let _refresh_guard = self.refresh_lock.lock().await; let auth_before_reload = self.auth_cached(); if auth_before_reload .as_ref() @@ -1323,7 +1329,7 @@ impl AuthManager { tracing::info!("Skipping token refresh because auth changed after guarded reload."); Ok(()) } - ReloadOutcome::ReloadedNoChange => self.refresh_token_from_authority().await, + ReloadOutcome::ReloadedNoChange => self.refresh_token_from_authority_impl().await, ReloadOutcome::Skipped => { Err(RefreshTokenError::Permanent(RefreshTokenFailedError::new( RefreshTokenFailedReason::Other, @@ -1338,6 +1344,11 @@ impl AuthManager { /// observe refreshed token. If the token refresh fails, returns the error to /// the caller. pub async fn refresh_token_from_authority(&self) -> Result<(), RefreshTokenError> { + let _refresh_guard = self.refresh_lock.lock().await; + self.refresh_token_from_authority_impl().await + } + + async fn refresh_token_from_authority_impl(&self) -> Result<(), RefreshTokenError> { tracing::info!("Refreshing token"); let auth = match self.auth_cached() {