mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
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.
This commit is contained in:
committed by
GitHub
Unverified
parent
9dbe098349
commit
2c67a27a71
Generated
-2
@@ -2451,8 +2451,6 @@ version = "0.0.0"
|
||||
dependencies = [
|
||||
"codex-utils-absolute-path",
|
||||
"codex-utils-plugins",
|
||||
"serde",
|
||||
"serde_json",
|
||||
"thiserror 2.0.18",
|
||||
]
|
||||
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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<Option<String>>,
|
||||
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() {
|
||||
|
||||
Reference in New Issue
Block a user