From a8bca62978f9fe1f44f261a77eaa6374a6860a2c Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 26 Oct 2026 16:33:31 +0000 Subject: [PATCH] Reduce remote plugin catalog cache probing overhead (#57258) ## Why Cache presence and freshness checks deserialize full plugin records even when callers only need to decide whether a cached catalog is available. ## What changed - Probe cache availability without deserializing plugin records, while still requiring a valid catalog envelope and a `plugins` array. - Let `PreferFreshCache` reject stale catalogs from a 256-byte prefix when the header provides enough information; fall back to a full read otherwise. - Limit discovery cache probes to the user and workspace scopes that need them, skipping the global cache probe. - Serialize cached plugin slices directly without cloning them. ## Testing Add coverage for malformed cache headers and payloads, reordered fields, stale-prefix rejection, and deferring plugin-record validation until a full catalog load. GitOrigin-RevId: 1e5ec95917f38c84bba30dbd3bb8c2f91cc7e3a8 --- codex-rs/core-plugins/src/manager.rs | 9 +- codex-rs/core-plugins/src/remote.rs | 39 +++-- .../core-plugins/src/remote/catalog_cache.rs | 161 ++++++++++++++++-- .../src/remote/catalog_cache_tests.rs | 104 +++++++++-- codex-rs/core-plugins/src/remote_tests.rs | 12 +- 5 files changed, 275 insertions(+), 50 deletions(-) diff --git a/codex-rs/core-plugins/src/manager.rs b/codex-rs/core-plugins/src/manager.rs index 4c299c32fc..ef600e8774 100644 --- a/codex-rs/core-plugins/src/manager.rs +++ b/codex-rs/core-plugins/src/manager.rs @@ -2983,15 +2983,20 @@ impl PluginsManager { if !prefetch_discovery { return; } + let scopes_to_probe: &[RemotePluginScope] = + if config_for_remote_sync.remote_plugin_enabled { + &[RemotePluginScope::User, RemotePluginScope::Workspace] + } else { + &[RemotePluginScope::Workspace] + }; let mut scopes = crate::remote::cached_remote_plugin_catalog_scopes( manager.codex_home.as_path(), &remote_plugin_service_config(&config_for_remote_sync), auth.as_ref(), + scopes_to_probe, ); if config_for_remote_sync.remote_plugin_enabled { scopes.insert(RemotePluginScope::Global); - } else { - scopes.retain(|scope| *scope == RemotePluginScope::Workspace); } manager.maybe_start_remote_catalog_cache_refresh( &config_for_remote_sync, diff --git a/codex-rs/core-plugins/src/remote.rs b/codex-rs/core-plugins/src/remote.rs index 2e3e4ae00a..d51d40a122 100644 --- a/codex-rs/core-plugins/src/remote.rs +++ b/codex-rs/core-plugins/src/remote.rs @@ -541,8 +541,6 @@ enum RemoteInstalledPluginScope { } impl RemotePluginScope { - const CATALOG_CACHE_SCOPES: [Self; 3] = [Self::Global, Self::User, Self::Workspace]; - fn api_value(self) -> &'static str { match self { Self::Global => "GLOBAL", @@ -1155,32 +1153,35 @@ pub(crate) fn has_fresh_cached_remote_plugin_catalog( let Ok(auth) = ensure_chatgpt_auth(auth) else { return false; }; - catalog_cache::load_cached_directory_plugins( - codex_home, config, auth, scope, /*collection*/ None, + catalog_cache::has_cached_directory_plugins( + codex_home, + config, + auth, + scope, + RemotePluginCatalogCacheMode::PreferFreshCache, ) - .is_some_and(|cached| { - matches!( - cached.freshness, - catalog_cache::RemotePluginCatalogCacheFreshness::Fresh - ) - }) } pub(crate) fn cached_remote_plugin_catalog_scopes( codex_home: &Path, config: &RemotePluginServiceConfig, auth: Option<&CodexAuth>, + scopes: &[RemotePluginScope], ) -> BTreeSet { let Ok(auth) = ensure_chatgpt_auth(auth) else { return BTreeSet::new(); }; - RemotePluginScope::CATALOG_CACHE_SCOPES - .into_iter() + scopes + .iter() + .copied() .filter(|scope| { - catalog_cache::load_cached_directory_plugins( - codex_home, config, auth, *scope, /*collection*/ None, + catalog_cache::has_cached_directory_plugins( + codex_home, + config, + auth, + *scope, + RemotePluginCatalogCacheMode::PreferCache, ) - .is_some() }) .collect() } @@ -1196,6 +1197,7 @@ pub fn cached_global_remote_discoverable_plugins( auth, RemotePluginScope::Global, /*collection*/ None, + RemotePluginCatalogCacheMode::PreferCache, ) .map(|cached| cached.plugins) .unwrap_or_default() @@ -2097,13 +2099,10 @@ async fn fetch_directory_plugins_for_scope_with_cache( collection: Option<&str>, cache_mode: RemotePluginCatalogCacheMode, ) -> Result { - if cache_mode != RemotePluginCatalogCacheMode::ForceRefetch - && let Some(codex_home) = codex_home + if let Some(codex_home) = codex_home && let Some(cached) = catalog_cache::load_cached_directory_plugins( - codex_home, config, auth, scope, collection, + codex_home, config, auth, scope, collection, cache_mode, ) - && (cache_mode == RemotePluginCatalogCacheMode::PreferCache - || cached.freshness == catalog_cache::RemotePluginCatalogCacheFreshness::Fresh) { return Ok(DirectoryPluginsFetchOutcome { plugins: cached.plugins, diff --git a/codex-rs/core-plugins/src/remote/catalog_cache.rs b/codex-rs/core-plugins/src/remote/catalog_cache.rs index cf78330e25..cd63578972 100644 --- a/codex-rs/core-plugins/src/remote/catalog_cache.rs +++ b/codex-rs/core-plugins/src/remote/catalog_cache.rs @@ -1,3 +1,4 @@ +use super::RemotePluginCatalogCacheMode; use super::RemotePluginDirectoryItem; use super::RemotePluginScope; use super::RemotePluginServiceConfig; @@ -5,7 +6,18 @@ use chrono::DateTime; use chrono::Utc; use codex_login::CodexAuth; use serde::Deserialize; +use serde::Deserializer; use serde::Serialize; +use serde::de::DeserializeOwned; +use serde::de::DeserializeSeed; +use serde::de::IgnoredAny; +use serde::de::MapAccess; +use serde::de::SeqAccess; +use serde::de::Visitor; +use std::fmt; +use std::fs::File; +use std::io; +use std::io::Read; use std::path::Path; use std::path::PathBuf; use std::time::Duration; @@ -58,12 +70,12 @@ impl RemotePluginCatalogCacheKey { } } -#[derive(Debug, Clone, Serialize, Deserialize)] -struct RemotePluginCatalogDiskCache { +#[derive(Serialize, Deserialize)] +struct RemotePluginCatalogDiskCache { schema_version: u8, #[serde(default, skip_serializing_if = "Option::is_none")] fetched_at: Option>, - plugins: Vec, + plugins: Plugins, } #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -72,23 +84,40 @@ pub(super) enum RemotePluginCatalogCacheFreshness { Stale, } -#[derive(Debug, Clone)] -pub(super) struct CachedDirectoryPlugins { - pub plugins: Vec, +pub(super) struct CachedDirectoryPlugins { + pub plugins: Vec, pub freshness: RemotePluginCatalogCacheFreshness, } -pub(crate) fn load_cached_directory_plugins( +pub(super) fn has_cached_directory_plugins( + codex_home: &Path, + config: &RemotePluginServiceConfig, + auth: &CodexAuth, + scope: RemotePluginScope, + cache_mode: RemotePluginCatalogCacheMode, +) -> bool { + // Ignore each entry so Serde still requires `plugins` to be an array. + load_cached_directory_plugins::( + codex_home, config, auth, scope, /*collection*/ None, cache_mode, + ) + .is_some() +} + +pub(super) fn load_cached_directory_plugins( codex_home: &Path, config: &RemotePluginServiceConfig, auth: &CodexAuth, scope: RemotePluginScope, collection: Option<&str>, -) -> Option { + cache_mode: RemotePluginCatalogCacheMode, +) -> Option> { + if cache_mode == RemotePluginCatalogCacheMode::ForceRefetch { + return None; + } let cache_key = RemotePluginCatalogCacheKey::new(config, auth, scope, collection)?; let cache_path = cache_path(codex_home, &cache_key); - let bytes = match std::fs::read(&cache_path) { - Ok(bytes) => bytes, + let bytes = match read_catalog_bytes(&cache_path, cache_mode) { + Ok(bytes) => bytes?, Err(err) if err.kind() == std::io::ErrorKind::NotFound => return None, Err(err) => { warn!( @@ -98,7 +127,7 @@ pub(crate) fn load_cached_directory_plugins( return None; } }; - let cache: RemotePluginCatalogDiskCache = match serde_json::from_slice(&bytes) { + let cache: RemotePluginCatalogDiskCache> = match serde_json::from_slice(&bytes) { Ok(cache) => cache, Err(err) => { warn!( @@ -116,6 +145,8 @@ pub(crate) fn load_cached_directory_plugins( let freshness = if is_fresh(cache.fetched_at, Utc::now()) { RemotePluginCatalogCacheFreshness::Fresh + } else if cache_mode == RemotePluginCatalogCacheMode::PreferFreshCache { + return None; } else { RemotePluginCatalogCacheFreshness::Stale }; @@ -125,6 +156,112 @@ pub(crate) fn load_cached_directory_plugins( }) } +fn read_catalog_bytes( + path: &Path, + cache_mode: RemotePluginCatalogCacheMode, +) -> io::Result>> { + let mut file = File::open(path)?; + let mut bytes = Vec::new(); + if cache_mode == RemotePluginCatalogCacheMode::PreferFreshCache { + const PREFIX_LEN: usize = 256; + bytes.reserve_exact(PREFIX_LEN); + file.by_ref() + .take(PREFIX_LEN as u64) + .read_to_end(&mut bytes)?; + if bytes.len() < PREFIX_LEN { + return Ok(Some(bytes)); + } + if CatalogHeader::is_stale_prefix(&bytes, Utc::now()) { + return Ok(None); + } + } + file.read_to_end(&mut bytes)?; + Ok(Some(bytes)) +} + +#[derive(Default)] +struct CatalogHeader { + schema_version: Option, + fetched_at: Option>, + metadata_before_plugins: bool, +} + +impl CatalogHeader { + fn is_stale_prefix(bytes: &[u8], now: DateTime) -> bool { + let mut header = Self::default(); + let mut json = serde_json::Deserializer::from_slice(bytes); + // A bounded read can end inside the payload. Only fully decoded fields count. + let parsed = json.deserialize_map(&mut header).and_then(|()| json.end()); + if parsed.is_err_and(|err| !err.is_eof()) { + return false; + } + header.metadata_before_plugins && !is_fresh(header.fetched_at, now) + } +} + +// Only the `plugins` field uses the seed. Serde calls `visit_seq` when it enters +// that array, so a missing or wrongly typed payload still goes through the full reader. +impl<'de> DeserializeSeed<'de> for &mut CatalogHeader { + type Value = (); + + fn deserialize>(self, deserializer: D) -> Result<(), D::Error> { + deserializer.deserialize_seq(self) + } +} + +impl<'de> Visitor<'de> for &mut CatalogHeader { + type Value = (); + + fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str("a plugin catalog object or its plugins array") + } + + fn visit_map>(self, mut map: A) -> Result<(), A::Error> { + #[derive(Deserialize)] + #[serde(field_identifier, rename_all = "snake_case")] + enum Field { + SchemaVersion, + FetchedAt, + Plugins, + #[serde(other)] + Other, + } + + let mut saw_plugins = false; + while let Some(field) = map.next_key()? { + match field { + Field::SchemaVersion if self.schema_version.is_none() => { + self.schema_version = Some(map.next_value()?); + } + Field::FetchedAt if self.fetched_at.is_none() => { + self.fetched_at = Some(map.next_value()?); + } + Field::Plugins if !saw_plugins => { + saw_plugins = true; + map.next_value_seed(&mut *self)?; + } + Field::SchemaVersion => { + return Err(serde::de::Error::duplicate_field("schema_version")); + } + Field::FetchedAt => return Err(serde::de::Error::duplicate_field("fetched_at")), + Field::Plugins => return Err(serde::de::Error::duplicate_field("plugins")), + Field::Other => { + map.next_value::()?; + } + } + } + Ok(()) + } + + fn visit_seq>(self, mut plugins: A) -> Result<(), A::Error> { + self.metadata_before_plugins = self.schema_version + == Some(REMOTE_PLUGIN_CATALOG_DISK_CACHE_SCHEMA_VERSION) + && self.fetched_at.is_some(); + while plugins.next_element::()?.is_some() {} + Ok(()) + } +} + pub(crate) fn write_cached_directory_plugins( codex_home: &Path, config: &RemotePluginServiceConfig, @@ -140,7 +277,7 @@ pub(crate) fn write_cached_directory_plugins( let Ok(contents) = serde_json::to_string_pretty(&RemotePluginCatalogDiskCache { schema_version: REMOTE_PLUGIN_CATALOG_DISK_CACHE_SCHEMA_VERSION, fetched_at: Some(Utc::now()), - plugins: plugins.to_vec(), + plugins, }) else { return; }; diff --git a/codex-rs/core-plugins/src/remote/catalog_cache_tests.rs b/codex-rs/core-plugins/src/remote/catalog_cache_tests.rs index 8ccce5f3c0..9c55d8d649 100644 --- a/codex-rs/core-plugins/src/remote/catalog_cache_tests.rs +++ b/codex-rs/core-plugins/src/remote/catalog_cache_tests.rs @@ -62,15 +62,16 @@ fn global_catalog_cache_reuses_legacy_cache_file() { "schema_version": REMOTE_PLUGIN_CATALOG_DISK_CACHE_SCHEMA_VERSION, "plugins": [], }); - let contents = serde_json::to_string_pretty(&legacy_cache).expect("serialize legacy cache"); - codex_utils_path::write_atomically(&legacy_cache_path, &contents).expect("write legacy cache"); + codex_utils_path::write_atomically(&legacy_cache_path, &legacy_cache.to_string()) + .expect("write legacy cache"); - let cached = load_cached_directory_plugins( + let cached = load_cached_directory_plugins::( codex_home.path(), &config, &auth, RemotePluginScope::Global, /*collection*/ None, + RemotePluginCatalogCacheMode::PreferCache, ) .expect("load legacy global cache"); assert!(cached.plugins.is_empty()); @@ -84,11 +85,90 @@ fn global_catalog_cache_reuses_legacy_cache_file() { /*collection*/ None, &[], ); - let refreshed_cache: RemotePluginCatalogDiskCache = serde_json::from_slice( - &std::fs::read(&legacy_cache_path).expect("read refreshed legacy cache"), - ) - .expect("parse refreshed legacy cache"); - assert!(refreshed_cache.fetched_at.is_some()); + assert!(has_cached_directory_plugins( + codex_home.path(), + &config, + &auth, + RemotePluginScope::Global, + RemotePluginCatalogCacheMode::PreferFreshCache, + )); +} + +#[test] +fn freshness_probe_defers_plugin_validation_until_full_read() { + let codex_home = tempfile::tempdir().expect("create codex home"); + let config = RemotePluginServiceConfig::new( + "https://chatgpt.com/backend-api".to_string(), + crate::test_support::test_http_client_factory(), + /*product_sku*/ None, + ); + let auth = CodexAuth::Headers(AuthHeaders::new(http::HeaderMap::new())); + let scope = RemotePluginScope::Global; + let key = RemotePluginCatalogCacheKey::new(&config, &auth, scope, /*collection*/ None) + .expect("global cache key"); + let path = cache_path(codex_home.path(), &key); + let probe = |mode| has_cached_directory_plugins(codex_home.path(), &config, &auth, scope, mode); + let version = REMOTE_PLUGIN_CATALOG_DISK_CACHE_SCHEMA_VERSION; + let now = serde_json::to_string(&Utc::now()).expect("serialize timestamp"); + let expired_at = "2000-01-01T00:00:00Z"; + let padding = " ".repeat(512); + for contents in [ + serde_json::json!({"schema_version": version + 1, "plugins": []}).to_string(), + serde_json::json!({"schema_version": version}).to_string(), + serde_json::json!({"schema_version": version, "plugins": {}}).to_string(), + format!(r#"{{"schema_version":{version},"plugins":[{{"#), + format!(r#"{{"schema_version":{version},"fetched_at":{now},"plugins":[{padding}"#), + format!( + r#"{{"details":{{"fetched_at":"{expired_at}"}},"schema_version":{version},"plugins":[{padding}"# + ), + format!( + r#"{{"schema_version":{version},"fetched_at":"{expired_at}","fetched_at":{now},"plugins":[{padding}"# + ), + format!( + r#"{{"schema_version":{version},"fetched_at":"{expired_at}","plugins":{{}}}}{padding}"# + ), + ] { + codex_utils_path::write_atomically(&path, &contents).expect("write invalid cache"); + assert!( + !probe(RemotePluginCatalogCacheMode::PreferFreshCache), + "{contents}" + ); + assert!(!path.exists()); + } + + let expired = format!( + r#"{{"note":"[","fetched\u005fat":"{expired_at}","schema_version":{version},"plugins":[{padding}"# + ); + codex_utils_path::write_atomically(&path, &expired).expect("write expired cache"); + assert!(!probe(RemotePluginCatalogCacheMode::PreferFreshCache)); + assert!(path.exists()); + assert!(!probe(RemotePluginCatalogCacheMode::PreferCache)); + assert!(!path.exists()); + + let reordered = + format!(r#"{{"plugins":[{padding}],"schema_version":{version},"fetched_at":{now}}}"#); + codex_utils_path::write_atomically(&path, &reordered).expect("write reordered cache"); + assert!(probe(RemotePluginCatalogCacheMode::PreferFreshCache)); + + let contents = serde_json::json!({ + "schema_version": version, + "fetched_at": Utc::now(), + "plugins": [{}], + }); + codex_utils_path::write_atomically(&path, &contents.to_string()).expect("write invalid plugin"); + assert!(probe(RemotePluginCatalogCacheMode::PreferFreshCache)); + assert!( + load_cached_directory_plugins::( + codex_home.path(), + &config, + &auth, + scope, + /*collection*/ None, + RemotePluginCatalogCacheMode::PreferCache, + ) + .is_none() + ); + assert!(!path.exists()); } #[test] @@ -110,12 +190,13 @@ fn header_auth_does_not_cache_private_catalogs_without_a_stable_identity() { &[], ); assert!( - load_cached_directory_plugins( + load_cached_directory_plugins::( codex_home.path(), &config, &auth, RemotePluginScope::Global, /*collection*/ None, + RemotePluginCatalogCacheMode::PreferCache, ) .is_some() ); @@ -144,19 +225,20 @@ fn header_auth_does_not_cache_private_catalogs_without_a_stable_identity() { let insecure_cache = RemotePluginCatalogDiskCache { schema_version: REMOTE_PLUGIN_CATALOG_DISK_CACHE_SCHEMA_VERSION, fetched_at: Some(Utc::now()), - plugins: Vec::new(), + plugins: Vec::::new(), }; let contents = serde_json::to_string_pretty(&insecure_cache).expect("serialize cache"); codex_utils_path::write_atomically(&insecure_cache_path, &contents) .expect("write insecure cache"); assert!( - load_cached_directory_plugins( + load_cached_directory_plugins::( codex_home.path(), &config, &auth, scope, /*collection*/ None, + RemotePluginCatalogCacheMode::PreferCache, ) .is_none() ); diff --git a/codex-rs/core-plugins/src/remote_tests.rs b/codex-rs/core-plugins/src/remote_tests.rs index 580baabc85..ebf2769002 100644 --- a/codex-rs/core-plugins/src/remote_tests.rs +++ b/codex-rs/core-plugins/src/remote_tests.rs @@ -468,7 +468,8 @@ fn catalog_cache_invalidation_clears_global_collections_and_preserves_other_scop /*product_sku*/ None, ); let auth = CodexAuth::create_dummy_chatgpt_auth_for_testing(); - for scope in [RemotePluginScope::Global, RemotePluginScope::Workspace] { + let scopes = [RemotePluginScope::Global, RemotePluginScope::Workspace]; + for scope in scopes { catalog_cache::write_cached_directory_plugins( codex_home.path(), &config, @@ -480,8 +481,8 @@ fn catalog_cache_invalidation_clears_global_collections_and_preserves_other_scop } assert_eq!( - cached_remote_plugin_catalog_scopes(codex_home.path(), &config, Some(&auth)), - BTreeSet::from([RemotePluginScope::Global, RemotePluginScope::Workspace]) + cached_remote_plugin_catalog_scopes(codex_home.path(), &config, Some(&auth), &scopes), + BTreeSet::from(scopes) ); catalog_cache::write_cached_directory_plugins( codex_home.path(), @@ -500,16 +501,17 @@ fn catalog_cache_invalidation_clears_global_collections_and_preserves_other_scop ); assert_eq!( - cached_remote_plugin_catalog_scopes(codex_home.path(), &config, Some(&auth)), + cached_remote_plugin_catalog_scopes(codex_home.path(), &config, Some(&auth), &scopes), BTreeSet::from([RemotePluginScope::Workspace]), ); assert!( - catalog_cache::load_cached_directory_plugins( + catalog_cache::load_cached_directory_plugins::( codex_home.path(), &config, &auth, RemotePluginScope::Global, Some(OPENAI_CURATED_REMOTE_COLLECTION_KEY), + RemotePluginCatalogCacheMode::PreferCache, ) .is_none() );