From 1b89beea82ae60e0dbeacb38f1ad2eae9dfed377 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 19 Oct 2026 11:43:05 +0000 Subject: [PATCH] Avoid cloning the full catalog for model metadata lookups (#55265) ## Why `get_model_info` clones the entire model catalog before selecting metadata for a single model. ## What changed Look up metadata directly in the active catalog for both remote and static model managers. Share catalog selection across remote catalog accessors and metadata lookup, preserving auth identity checks, API-key discovery policy, provider matching rules, and configuration overrides. ## Testing Extend coverage for custom catalog configuration overrides, metadata fallback after disabling API-key discovery, and fallback after an auth identity change. GitOrigin-RevId: 7d0bc4c1ccd2b56e16c7a17c1dbdca05a5c14a7b --- .../model-provider/src/models_endpoint.rs | 6 ++ .../src/api_key_discovery_tests.rs | 8 ++ codex-rs/models-manager/src/manager.rs | 80 +++++++++++-------- codex-rs/models-manager/src/manager_tests.rs | 15 ++++ 4 files changed, 76 insertions(+), 33 deletions(-) diff --git a/codex-rs/model-provider/src/models_endpoint.rs b/codex-rs/model-provider/src/models_endpoint.rs index 6986451a8c..780ae934bb 100644 --- a/codex-rs/model-provider/src/models_endpoint.rs +++ b/codex-rs/model-provider/src/models_endpoint.rs @@ -762,6 +762,12 @@ mod tests { let bundled = codex_models_manager::bundled_models_response().unwrap(); assert_eq!(manager.get_remote_models().await, bundled.models); assert_eq!(manager.try_get_remote_models().unwrap(), bundled.models); + assert_eq!( + manager + .get_model_info(&model.slug, &Default::default()) + .await, + model_info_from_slug(&model.slug) + ); assert_eq!( manager .raw_model_catalog( diff --git a/codex-rs/models-manager/src/api_key_discovery_tests.rs b/codex-rs/models-manager/src/api_key_discovery_tests.rs index 518a5a4868..6c3695c555 100644 --- a/codex-rs/models-manager/src/api_key_discovery_tests.rs +++ b/codex-rs/models-manager/src/api_key_discovery_tests.rs @@ -42,6 +42,7 @@ async fn api_key_discovery_flag_controls_fetches_and_cached_catalogs() { let manager = OpenAiModelsManager::new(home.path().into(), endpoint.clone(), Some(auth.clone())); let bundled = load_remote_models_from_file().unwrap(); + let config = ModelsManagerConfig::default(); for strategy in [ RefreshStrategy::Online, @@ -69,6 +70,7 @@ async fn api_key_discovery_flag_controls_fetches_and_cached_catalogs() { models ); assert_eq!(endpoint.fetch_count(), 1); + assert_eq!(manager.get_model_info("dynamic", &config).await, models[0]); // A runtime rollback must also suppress the already-loaded catalog. manager.set_api_key_model_discovery_enabled(/*enabled*/ false); @@ -80,6 +82,12 @@ async fn api_key_discovery_flag_controls_fetches_and_cached_catalogs() { bundled ); assert_eq!(manager.try_get_remote_models().unwrap(), bundled); + assert!( + manager + .get_model_info("dynamic", &config) + .await + .used_fallback_model_metadata + ); assert_eq!(endpoint.fetch_count(), 1); // A new session must honor its startup flag even when a matching disk cache exists. diff --git a/codex-rs/models-manager/src/manager.rs b/codex-rs/models-manager/src/manager.rs index 2a0b98091f..2b5ea22bf4 100644 --- a/codex-rs/models-manager/src/manager.rs +++ b/codex-rs/models-manager/src/manager.rs @@ -15,6 +15,7 @@ use codex_protocol::openai_models::ModelInfo; use codex_protocol::openai_models::ModelPreset; use codex_protocol::openai_models::ModelVisibility; use codex_protocol::openai_models::ModelsResponse; +use std::borrow::Cow; use std::fmt; use std::future::Future; use std::path::PathBuf; @@ -430,53 +431,38 @@ impl ModelsManager for OpenAiModelsManager { fn get_remote_models(&self) -> ModelsManagerFuture<'_, Vec> { Box::pin(async move { let entry = self.remote_models.read().await; - if !self.api_key_discovery_disabled() - && entry.identity.is_some() - && entry.identity == self.endpoint_client.identity() - { - entry.models.clone() - } else { - self.catalog_source.fallback_models().unwrap_or_default() - } + self.models_for_entry(&entry).into_owned() }) } fn try_get_remote_models(&self) -> Result, TryLockError> { let entry = self.remote_models.try_read()?; - Ok( - if !self.api_key_discovery_disabled() - && entry.identity.is_some() - && entry.identity == self.endpoint_client.identity() - { - entry.models.clone() - } else { - self.catalog_source.fallback_models().unwrap_or_default() - }, - ) + Ok(self.models_for_entry(&entry).into_owned()) } fn auth_manager(&self) -> Option<&AuthManager> { self.auth_manager.as_deref() } - #[tracing::instrument(skip_all, fields(model))] + #[tracing::instrument(skip_all, fields(model = model))] fn get_model_info<'a>( &'a self, model: &'a str, config: &'a ModelsManagerConfig, ) -> ModelsManagerFuture<'a, ModelInfo> { Box::pin(async move { - let models = self.get_remote_models().await; - match &self.catalog_source { - CatalogSource::Default => { - construct_model_info_from_candidates(model, &models, config) + let remote = { + let entry = self.remote_models.read().await; + let models = self.models_for_entry(&entry); + match &self.catalog_source { + CatalogSource::Default => find_model_from_candidates(model, &models), + CatalogSource::ExplicitProvider(_) => models + .iter() + .find(|candidate| candidate.slug == model) + .cloned(), } - CatalogSource::ExplicitProvider(_) => construct_model_info( - model, - models.into_iter().find(|candidate| candidate.slug == model), - config, - ), - } + }; + construct_model_info(model, remote, config) }) } @@ -498,6 +484,18 @@ impl ModelsManager for OpenAiModelsManager { } impl OpenAiModelsManager { + /// Selects the catalog available under the current provider and auth identity. + fn models_for_entry<'a>(&self, entry: &'a ModelsCacheEntry) -> Cow<'a, [ModelInfo]> { + if !self.api_key_discovery_disabled() + && entry.identity.is_some() + && entry.identity == self.endpoint_client.identity() + { + Cow::Borrowed(&entry.models) + } else { + Cow::Owned(self.catalog_source.fallback_models().unwrap_or_default()) + } + } + async fn raw_model_catalog( &self, refresh_strategy: RefreshStrategy, @@ -766,6 +764,17 @@ impl OpenAiModelsManager { } impl ModelsManager for StaticModelsManager { + #[tracing::instrument(skip_all, fields(model = model))] + fn get_model_info<'a>( + &'a self, + model: &'a str, + config: &'a ModelsManagerConfig, + ) -> ModelsManagerFuture<'a, ModelInfo> { + Box::pin( + async move { construct_model_info_from_candidates(model, &self.remote_models, config) }, + ) + } + fn get_default_model<'a>( &'a self, model: &'a Option, @@ -900,15 +909,20 @@ fn find_model_by_namespaced_suffix(model: &str, candidates: &[ModelInfo]) -> Opt find_model_by_longest_prefix(suffix, candidates) } +/// Selects owned model metadata without applying configuration overrides. +fn find_model_from_candidates(model: &str, candidates: &[ModelInfo]) -> Option { + // First use the normal longest-prefix match. If that misses, allow a narrowly scoped + // retry for namespaced slugs like `custom/gpt-5.3-codex`. + find_model_by_longest_prefix(model, candidates) + .or_else(|| find_model_by_namespaced_suffix(model, candidates)) +} + pub(crate) fn construct_model_info_from_candidates( model: &str, candidates: &[ModelInfo], config: &ModelsManagerConfig, ) -> ModelInfo { - // First use the normal longest-prefix match. If that misses, allow a narrowly scoped - // retry for namespaced slugs like `custom/gpt-5.3-codex`. - let remote = find_model_by_longest_prefix(model, candidates) - .or_else(|| find_model_by_namespaced_suffix(model, candidates)); + let remote = find_model_from_candidates(model, candidates); construct_model_info(model, remote, config) } diff --git a/codex-rs/models-manager/src/manager_tests.rs b/codex-rs/models-manager/src/manager_tests.rs index 6533129b53..3bb8d71821 100644 --- a/codex-rs/models-manager/src/manager_tests.rs +++ b/codex-rs/models-manager/src/manager_tests.rs @@ -811,6 +811,21 @@ async fn get_model_info_uses_custom_catalog() { assert_eq!(model_info.context_window, Some(272_000)); assert!(model_info.supports_image_detail_original); assert!(!model_info.used_fallback_model_metadata); + + let config = ModelsManagerConfig { + model_context_window: Some(128_000), + ..Default::default() + }; + let expected = ModelInfo { + context_window: Some(128_000), + ..model_info + }; + assert_eq!( + manager + .get_model_info("gpt-overlay-experiment", &config) + .await, + expected + ); } #[tokio::test]