mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
login: treat provider auth refresh_interval_ms=0 as no auto-refresh (#16480)
## Why Follow-up to #16288: the new dynamic provider auth token flow currently defaults `refresh_interval_ms` to a non-zero value and rejects `0` entirely. For command-backed bearer auth, `0` should mean "never auto-refresh". That lets callers keep using the cached token until the backend actually returns `401 Unauthorized`, at which point Codex can rerun the auth command as part of the existing retry path. ## What changed - changed `ModelProviderAuthInfo.refresh_interval_ms` to accept `0` and documented that value as disabling proactive refresh - updated the external bearer token refresher to treat `refresh_interval_ms = 0` as an indefinitely reusable cached token, while still rerunning the auth command during unauthorized recovery - regenerated `core/config.schema.json` so the schema minimum is `0` and the new behavior is described in the field docs - added coverage for both config deserialization and the no-auto-refresh plus `401` recovery behavior ## How tested - `cargo test -p codex-protocol` - `cargo test -p codex-login` - `cargo test -p codex-core test_deserialize_provider_auth_config_`
This commit is contained in:
committed by
GitHub
Unverified
parent
1b711a5501
commit
f83f3fa2a6
@@ -845,9 +845,9 @@
|
||||
},
|
||||
"refresh_interval_ms": {
|
||||
"default": 300000,
|
||||
"description": "Maximum age for the cached token before rerunning the command.",
|
||||
"description": "Maximum age for the cached token before rerunning the command. Set to `0` to disable proactive refresh and only rerun after a 401 retry path.",
|
||||
"format": "uint64",
|
||||
"minimum": 1.0,
|
||||
"minimum": 0.0,
|
||||
"type": "integer"
|
||||
},
|
||||
"timeout_ms": {
|
||||
|
||||
@@ -151,8 +151,29 @@ args = ["--format=text"]
|
||||
command: "./scripts/print-token".to_string(),
|
||||
args: vec!["--format=text".to_string()],
|
||||
timeout_ms: NonZeroU64::new(5_000).unwrap(),
|
||||
refresh_interval_ms: NonZeroU64::new(300_000).unwrap(),
|
||||
refresh_interval_ms: 300_000,
|
||||
cwd: AbsolutePathBuf::resolve_path_against_base(".", base_dir.path()).unwrap(),
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_deserialize_provider_auth_config_allows_zero_refresh_interval() {
|
||||
let base_dir = tempdir().unwrap();
|
||||
let provider_toml = r#"
|
||||
name = "Corp"
|
||||
|
||||
[auth]
|
||||
command = "./scripts/print-token"
|
||||
refresh_interval_ms = 0
|
||||
"#;
|
||||
|
||||
let provider: ModelProviderInfo = {
|
||||
let _guard = AbsolutePathBufGuard::new(base_dir.path());
|
||||
toml::from_str(provider_toml).unwrap()
|
||||
};
|
||||
|
||||
let auth = provider.auth.expect("auth config should deserialize");
|
||||
assert_eq!(auth.refresh_interval_ms, 0);
|
||||
assert_eq!(auth.refresh_interval(), None);
|
||||
}
|
||||
|
||||
@@ -174,8 +174,8 @@ $lines | Select-Object -Skip 1 | Set-Content -Path tokens.txt
|
||||
ModelProviderAuthInfo {
|
||||
command: self.command.clone(),
|
||||
args: self.args.clone(),
|
||||
timeout_ms: non_zero_u64(/*value*/ 1_000),
|
||||
refresh_interval_ms: non_zero_u64(/*value*/ 60_000),
|
||||
timeout_ms: NonZeroU64::new(/*value*/ 1_000).unwrap(),
|
||||
refresh_interval_ms: 60_000,
|
||||
cwd: match codex_utils_absolute_path::AbsolutePathBuf::try_from(self.tempdir.path()) {
|
||||
Ok(cwd) => cwd,
|
||||
Err(err) => panic!("tempdir should be absolute: {err}"),
|
||||
@@ -184,13 +184,6 @@ $lines | Select-Object -Skip 1 | Set-Content -Path tokens.txt
|
||||
}
|
||||
}
|
||||
|
||||
fn non_zero_u64(value: u64) -> NonZeroU64 {
|
||||
match NonZeroU64::new(value) {
|
||||
Some(value) => value,
|
||||
None => panic!("expected non-zero value: {value}"),
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Default)]
|
||||
struct TagCollectorVisitor {
|
||||
tags: BTreeMap<String, String>,
|
||||
|
||||
@@ -220,7 +220,7 @@ $lines | Select-Object -Skip 1 | Set-Content -Path tokens.txt
|
||||
command: self.command.clone(),
|
||||
args: self.args.clone(),
|
||||
timeout_ms: non_zero_u64(/*value*/ 1_000),
|
||||
refresh_interval_ms: non_zero_u64(/*value*/ 60_000),
|
||||
refresh_interval_ms: 60_000,
|
||||
cwd: match codex_utils_absolute_path::AbsolutePathBuf::try_from(self.tempdir.path()) {
|
||||
Ok(cwd) => cwd,
|
||||
Err(err) => panic!("tempdir should be absolute: {err}"),
|
||||
|
||||
@@ -287,6 +287,26 @@ async fn external_bearer_only_auth_manager_uses_cached_provider_token() {
|
||||
assert_eq!(manager.get_api_auth_mode(), Some(ApiAuthMode::ApiKey));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn external_bearer_only_auth_manager_disables_auto_refresh_when_interval_is_zero() {
|
||||
let script = ProviderAuthScript::new(&["provider-token", "next-token"]).unwrap();
|
||||
let mut auth_config = script.auth_config();
|
||||
auth_config.refresh_interval_ms = 0;
|
||||
let manager = AuthManager::external_bearer_only(auth_config);
|
||||
|
||||
let first = manager
|
||||
.auth()
|
||||
.await
|
||||
.and_then(|auth| auth.api_key().map(str::to_string));
|
||||
let second = manager
|
||||
.auth()
|
||||
.await
|
||||
.and_then(|auth| auth.api_key().map(str::to_string));
|
||||
|
||||
assert_eq!(first.as_deref(), Some("provider-token"));
|
||||
assert_eq!(second.as_deref(), Some("provider-token"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn external_bearer_only_auth_manager_returns_none_when_command_fails() {
|
||||
let script = ProviderAuthScript::new_failing().unwrap();
|
||||
@@ -298,7 +318,9 @@ async fn external_bearer_only_auth_manager_returns_none_when_command_fails() {
|
||||
#[tokio::test]
|
||||
async fn unauthorized_recovery_uses_external_refresh_for_bearer_manager() {
|
||||
let script = ProviderAuthScript::new(&["provider-token", "refreshed-provider-token"]).unwrap();
|
||||
let manager = AuthManager::external_bearer_only(script.auth_config());
|
||||
let mut auth_config = script.auth_config();
|
||||
auth_config.refresh_interval_ms = 0;
|
||||
let manager = AuthManager::external_bearer_only(auth_config);
|
||||
let initial_token = manager
|
||||
.auth()
|
||||
.await
|
||||
|
||||
@@ -35,18 +35,24 @@ impl ExternalAuth for BearerTokenRefresher {
|
||||
async fn resolve(&self) -> io::Result<Option<ExternalAuthTokens>> {
|
||||
let access_token = {
|
||||
let mut cached = self.state.cached_token.lock().await;
|
||||
if let Some(cached_token) = cached.as_ref()
|
||||
&& cached_token.fetched_at.elapsed() < self.state.config.refresh_interval()
|
||||
{
|
||||
cached_token.access_token.clone()
|
||||
} else {
|
||||
let access_token = run_provider_auth_command(&self.state.config).await?;
|
||||
*cached = Some(CachedExternalBearerToken {
|
||||
access_token: access_token.clone(),
|
||||
fetched_at: Instant::now(),
|
||||
});
|
||||
access_token
|
||||
if let Some(cached_token) = cached.as_ref() {
|
||||
let should_use_cached_token = match self.state.config.refresh_interval() {
|
||||
Some(refresh_interval) => cached_token.fetched_at.elapsed() < refresh_interval,
|
||||
None => true,
|
||||
};
|
||||
if should_use_cached_token {
|
||||
return Ok(Some(ExternalAuthTokens::access_token_only(
|
||||
cached_token.access_token.clone(),
|
||||
)));
|
||||
}
|
||||
}
|
||||
|
||||
let access_token = run_provider_auth_command(&self.state.config).await?;
|
||||
*cached = Some(CachedExternalBearerToken {
|
||||
access_token: access_token.clone(),
|
||||
fetched_at: Instant::now(),
|
||||
});
|
||||
access_token
|
||||
};
|
||||
Ok(Some(ExternalAuthTokens::access_token_only(access_token)))
|
||||
}
|
||||
|
||||
@@ -283,8 +283,9 @@ pub struct ModelProviderAuthInfo {
|
||||
pub timeout_ms: NonZeroU64,
|
||||
|
||||
/// Maximum age for the cached token before rerunning the command.
|
||||
/// Set to `0` to disable proactive refresh and only rerun after a 401 retry path.
|
||||
#[serde(default = "default_provider_auth_refresh_interval_ms")]
|
||||
pub refresh_interval_ms: NonZeroU64,
|
||||
pub refresh_interval_ms: u64,
|
||||
|
||||
/// Working directory used when running the token command.
|
||||
#[serde(default = "default_provider_auth_cwd")]
|
||||
@@ -297,8 +298,8 @@ impl ModelProviderAuthInfo {
|
||||
Duration::from_millis(self.timeout_ms.get())
|
||||
}
|
||||
|
||||
pub fn refresh_interval(&self) -> Duration {
|
||||
Duration::from_millis(self.refresh_interval_ms.get())
|
||||
pub fn refresh_interval(&self) -> Option<Duration> {
|
||||
NonZeroU64::new(self.refresh_interval_ms).map(|value| Duration::from_millis(value.get()))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -309,11 +310,8 @@ fn default_provider_auth_timeout_ms() -> NonZeroU64 {
|
||||
)
|
||||
}
|
||||
|
||||
fn default_provider_auth_refresh_interval_ms() -> NonZeroU64 {
|
||||
non_zero_u64(
|
||||
DEFAULT_PROVIDER_AUTH_REFRESH_INTERVAL_MS,
|
||||
"model_providers.<id>.auth.refresh_interval_ms",
|
||||
)
|
||||
fn default_provider_auth_refresh_interval_ms() -> u64 {
|
||||
DEFAULT_PROVIDER_AUTH_REFRESH_INTERVAL_MS
|
||||
}
|
||||
|
||||
fn non_zero_u64(value: u64, field_name: &str) -> NonZeroU64 {
|
||||
|
||||
Reference in New Issue
Block a user