From 81caee3400528d6a9d591f6939bc82cc9960732e Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Thu, 8 Jan 2026 18:06:10 -0800 Subject: [PATCH] Add 5s timeout to models list call + integration test (#8942) - Enforce a 5s timeout around the remote models refresh to avoid hanging /models calls. --- codex-rs/core/src/models_manager/manager.rs | 14 ++-- codex-rs/core/tests/common/responses.rs | 19 ++++++ codex-rs/core/tests/suite/remote_models.rs | 72 +++++++++++++++++++++ 3 files changed, 101 insertions(+), 4 deletions(-) diff --git a/codex-rs/core/src/models_manager/manager.rs b/codex-rs/core/src/models_manager/manager.rs index 0b388d133..862f5584c 100644 --- a/codex-rs/core/src/models_manager/manager.rs +++ b/codex-rs/core/src/models_manager/manager.rs @@ -12,6 +12,7 @@ use std::sync::Arc; use std::time::Duration; use tokio::sync::RwLock; use tokio::sync::TryLockError; +use tokio::time::timeout; use tracing::error; use super::cache; @@ -21,6 +22,7 @@ use crate::api_bridge::map_api_error; use crate::auth::AuthManager; use crate::config::Config; use crate::default_client::build_reqwest_client; +use crate::error::CodexErr; use crate::error::Result as CoreResult; use crate::features::Feature; use crate::model_provider_info::ModelProviderInfo; @@ -29,6 +31,7 @@ use crate::models_manager::model_presets::builtin_model_presets; const MODEL_CACHE_FILE: &str = "models_cache.json"; const DEFAULT_MODEL_CACHE_TTL: Duration = Duration::from_secs(300); +const MODELS_REFRESH_TIMEOUT: Duration = Duration::from_secs(5); const OPENAI_DEFAULT_API_MODEL: &str = "gpt-5.1-codex-max"; const OPENAI_DEFAULT_CHATGPT_MODEL: &str = "gpt-5.2-codex"; const CODEX_AUTO_BALANCED_MODEL: &str = "codex-auto-balanced"; @@ -105,10 +108,13 @@ impl ModelsManager { let client = ModelsClient::new(transport, api_provider, api_auth); let client_version = format_client_version_to_whole(); - let (models, etag) = client - .list_models(&client_version, HeaderMap::new()) - .await - .map_err(map_api_error)?; + let (models, etag) = timeout( + MODELS_REFRESH_TIMEOUT, + client.list_models(&client_version, HeaderMap::new()), + ) + .await + .map_err(|_| CodexErr::Timeout)? + .map_err(map_api_error)?; self.apply_remote_models(models.clone()).await; *self.etag.write().await = etag.clone(); diff --git a/codex-rs/core/tests/common/responses.rs b/codex-rs/core/tests/common/responses.rs index 458d04335..710d03fc7 100644 --- a/codex-rs/core/tests/common/responses.rs +++ b/codex-rs/core/tests/common/responses.rs @@ -1,5 +1,6 @@ use std::sync::Arc; use std::sync::Mutex; +use std::time::Duration; use anyhow::Result; use base64::Engine; @@ -674,6 +675,24 @@ pub async fn mount_models_once(server: &MockServer, body: ModelsResponse) -> Mod models_mock } +pub async fn mount_models_once_with_delay( + server: &MockServer, + body: ModelsResponse, + delay: Duration, +) -> ModelsMock { + let (mock, models_mock) = models_mock(); + mock.respond_with( + ResponseTemplate::new(200) + .insert_header("content-type", "application/json") + .set_body_json(body.clone()) + .set_delay(delay), + ) + .up_to_n_times(1) + .mount(server) + .await; + models_mock +} + pub async fn mount_models_once_with_etag( server: &MockServer, body: ModelsResponse, diff --git a/codex-rs/core/tests/suite/remote_models.rs b/codex-rs/core/tests/suite/remote_models.rs index 71a3106bc..6dfdc4467 100644 --- a/codex-rs/core/tests/suite/remote_models.rs +++ b/codex-rs/core/tests/suite/remote_models.rs @@ -9,6 +9,7 @@ use codex_core::ModelProviderInfo; use codex_core::ThreadManager; use codex_core::built_in_model_providers; use codex_core::config::Config; +use codex_core::error::CodexErr; use codex_core::features::Feature; use codex_core::models_manager::manager::ModelsManager; use codex_core::protocol::AskForApproval; @@ -32,6 +33,7 @@ use core_test_support::responses::ev_completed; use core_test_support::responses::ev_function_call; use core_test_support::responses::ev_response_created; use core_test_support::responses::mount_models_once; +use core_test_support::responses::mount_models_once_with_delay; use core_test_support::responses::mount_sse_once; use core_test_support::responses::mount_sse_sequence; use core_test_support::responses::sse; @@ -45,6 +47,7 @@ use tempfile::TempDir; use tokio::time::Duration; use tokio::time::Instant; use tokio::time::sleep; +use tokio::time::timeout; use wiremock::BodyPrintLimit; use wiremock::MockServer; @@ -442,6 +445,75 @@ async fn remote_models_preserve_builtin_presets() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn remote_models_request_times_out_after_5s() -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_sandbox!(Ok(())); + + let server = MockServer::start().await; + let remote_model = test_remote_model("remote-timeout", ModelVisibility::List, 0); + let models_mock = mount_models_once_with_delay( + &server, + ModelsResponse { + models: vec![remote_model], + }, + Duration::from_secs(6), + ) + .await; + + let codex_home = TempDir::new()?; + let mut config = load_default_config_for_test(&codex_home).await; + config.features.enable(Feature::RemoteModels); + + let auth = CodexAuth::create_dummy_chatgpt_auth_for_testing(); + let provider = ModelProviderInfo { + base_url: Some(format!("{}/v1", server.uri())), + ..built_in_model_providers()["openai"].clone() + }; + let manager = ModelsManager::with_provider( + codex_home.path().to_path_buf(), + codex_core::auth::AuthManager::from_auth_for_testing(auth), + provider, + ); + + let start = Instant::now(); + let refresh = timeout( + Duration::from_secs(7), + manager.refresh_available_models_with_cache(&config), + ) + .await; + let elapsed = start.elapsed(); + let err = refresh + .expect("refresh should finish") + .expect_err("refresh should time out"); + let request_summaries: Vec = server + .received_requests() + .await + .expect("mock server should capture requests") + .iter() + .map(|req| format!("{} {}", req.method, req.url.path())) + .collect(); + assert!( + elapsed >= Duration::from_millis(4_500), + "expected models call to block near the timeout; took {elapsed:?}" + ); + assert!( + elapsed < Duration::from_millis(5_800), + "expected models call to time out before the delayed response; took {elapsed:?}" + ); + match err { + CodexErr::Timeout => {} + other => panic!("expected timeout error, got {other:?}; requests: {request_summaries:?}"), + } + assert_eq!( + models_mock.requests().len(), + 1, + "expected a single /models request" + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn remote_models_hide_picker_only_models() -> Result<()> { skip_if_no_network!(Ok(()));