Skip to content
Open
26 changes: 22 additions & 4 deletions codex-rs/core/src/models_manager/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use std::collections::HashSet;
use std::path::PathBuf;
use std::sync::Arc;
use std::time::Duration;
use tokio::sync::Mutex;
use tokio::sync::RwLock;
use tokio::sync::TryLockError;
use tracing::error;
Expand Down Expand Up @@ -39,6 +40,7 @@ pub struct ModelsManager {
// todo(aibrahim) merge available_models and model family creation into one struct
local_models: Vec<ModelPreset>,
remote_models: RwLock<Vec<ModelInfo>>,
refresh_lock: Mutex<()>,
auth_manager: Arc<AuthManager>,
etag: RwLock<Option<String>>,
codex_home: PathBuf,
Expand All @@ -53,6 +55,7 @@ impl ModelsManager {
Self {
local_models: builtin_model_presets(auth_manager.get_auth_mode()),
remote_models: RwLock::new(Self::load_remote_models_from_file().unwrap_or_default()),
refresh_lock: Mutex::new(()),
auth_manager,
etag: RwLock::new(None),
codex_home,
Expand All @@ -68,6 +71,7 @@ impl ModelsManager {
Self {
local_models: builtin_model_presets(auth_manager.get_auth_mode()),
remote_models: RwLock::new(Self::load_remote_models_from_file().unwrap_or_default()),
refresh_lock: Mutex::new(()),
auth_manager,
etag: RwLock::new(None),
codex_home,
Expand All @@ -78,6 +82,7 @@ impl ModelsManager {

/// Fetch the latest remote models, using the on-disk cache when still fresh.
pub async fn refresh_available_models_with_cache(&self, config: &Config) -> CoreResult<()> {
let _refresh_guard = self.refresh_lock.lock().await;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't this lock in the same place we do the network call?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's purpose is to avoid multiple parallel network calls?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to attempt a refresh after we have cache as well

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but that's already handled with if self.try_load_cache().await { no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if two instances hit refresh_available_models_with_cache at time 0 and 1.

Then, both won't find cache.
both will do a call.
both will update cache

What I want is, if one is already updating, wait then read the cache because it will exist.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if !config.features.enabled(Feature::RemoteModels)
|| self.auth_manager.get_auth_mode() == Some(AuthMode::ApiKey)
{
Expand All @@ -104,10 +109,23 @@ 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 remote_models = tokio::time::timeout(
Duration::from_secs(5),
Copy link
Collaborator

@pakrym-oai pakrym-oai Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit add const

client.list_models(&client_version, HeaderMap::new()),
)
.await;

let (models, etag) = match remote_models {
Ok(Ok((models, etag))) => (models, etag),
Ok(Err(err)) => {
error!("failed to refresh remote models: {}", map_api_error(err));
return Ok(());
}
Err(_) => {
error!("timed out refreshing remote models after 5s");
return Ok(());
}
};

self.apply_remote_models(models.clone()).await;
*self.etag.write().await = etag.clone();
Expand Down
Loading
Loading