Add 5s timeout to models list call + integration test (#8942)
- Enforce a 5s timeout around the remote models refresh to avoid hanging /models calls.
This commit is contained in:
parent
51dd5af807
commit
81caee3400
3 changed files with 101 additions and 4 deletions
|
|
@ -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();
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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<String> = 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(()));
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue