diff --git a/codex-rs/core-plugins/src/manager.rs b/codex-rs/core-plugins/src/manager.rs index 46ebc527ce..845b8f3f32 100644 --- a/codex-rs/core-plugins/src/manager.rs +++ b/codex-rs/core-plugins/src/manager.rs @@ -78,7 +78,6 @@ use std::path::PathBuf; use std::sync::Arc; use std::sync::RwLock; use std::sync::atomic::AtomicBool; -use std::sync::atomic::AtomicU64; use std::sync::atomic::Ordering; use std::time::Instant; use tokio::sync::Semaphore; @@ -405,9 +404,8 @@ pub struct PluginsManager { featured_plugin_ids_cache: RwLock>, configured_marketplace_upgrade_state: RwLock, non_curated_cache_refresh_state: RwLock, - cached_enabled_outcome: RwLock>, + enabled_outcome_cache: RwLock, enabled_outcome_load_semaphore: Semaphore, - enabled_outcome_cache_generation: AtomicU64, remote_installed_plugins_cache: RwLock>>, remote_installed_plugins_cache_refresh_state: RwLock, remote_sync_lock: Semaphore, @@ -421,6 +419,12 @@ struct CachedPluginLoadOutcome { outcome: PluginLoadOutcome, } +#[derive(Default)] +struct EnabledOutcomeCache { + generation: u64, + outcome: Option, +} + #[derive(Clone, PartialEq, Eq)] struct PluginLoadCacheKey { configured_plugins: HashMap, @@ -452,9 +456,8 @@ impl PluginsManager { ConfiguredMarketplaceUpgradeState::default(), ), non_curated_cache_refresh_state: RwLock::new(NonCuratedCacheRefreshState::default()), - cached_enabled_outcome: RwLock::new(None), + enabled_outcome_cache: RwLock::new(EnabledOutcomeCache::default()), enabled_outcome_load_semaphore: Semaphore::new(/*permits*/ 1), - enabled_outcome_cache_generation: AtomicU64::new(0), remote_installed_plugins_cache: RwLock::new(None), remote_installed_plugins_cache_refresh_state: RwLock::new( RemoteInstalledPluginsCacheRefreshState::default(), @@ -513,9 +516,7 @@ impl PluginsManager { if !force_reload && let Some(outcome) = self.cached_enabled_outcome(&cache_key) { return outcome; } - let cache_generation = self - .enabled_outcome_cache_generation - .load(Ordering::Acquire); + let cache_generation = self.enabled_outcome_cache_generation(); let outcome = load_plugins_from_layer_stack( &config.config_layer_stack, self.remote_installed_plugin_configs(), @@ -525,20 +526,7 @@ impl PluginsManager { ) .await; log_plugin_load_errors(&outcome); - if self - .enabled_outcome_cache_generation - .load(Ordering::Acquire) - == cache_generation - { - let mut cache = match self.cached_enabled_outcome.write() { - Ok(cache) => cache, - Err(err) => err.into_inner(), - }; - *cache = Some(CachedPluginLoadOutcome { - key: cache_key, - outcome: outcome.clone(), - }); - } + self.cache_enabled_outcome_if_current(cache_generation, cache_key, outcome.clone()); outcome } @@ -552,13 +540,12 @@ impl PluginsManager { } fn clear_enabled_outcome_cache(&self) { - self.enabled_outcome_cache_generation - .fetch_add(1, Ordering::AcqRel); - let mut cached_enabled_outcome = match self.cached_enabled_outcome.write() { + let mut cache = match self.enabled_outcome_cache.write() { Ok(cache) => cache, Err(err) => err.into_inner(), }; - *cached_enabled_outcome = None; + cache.generation = cache.generation.wrapping_add(1); + cache.outcome = None; } /// Load plugins for a config layer stack without touching the plugins cache. @@ -610,19 +597,43 @@ impl PluginsManager { } fn cached_enabled_outcome(&self, key: &PluginLoadCacheKey) -> Option { - match self.cached_enabled_outcome.read() { + match self.enabled_outcome_cache.read() { Ok(cache) => cache + .outcome .as_ref() .filter(|cached| cached.key == *key) .map(|cached| cached.outcome.clone()), Err(err) => err .into_inner() + .outcome .as_ref() .filter(|cached| cached.key == *key) .map(|cached| cached.outcome.clone()), } } + fn enabled_outcome_cache_generation(&self) -> u64 { + match self.enabled_outcome_cache.read() { + Ok(cache) => cache.generation, + Err(err) => err.into_inner().generation, + } + } + + fn cache_enabled_outcome_if_current( + &self, + generation: u64, + key: PluginLoadCacheKey, + outcome: PluginLoadOutcome, + ) { + let mut cache = match self.enabled_outcome_cache.write() { + Ok(cache) => cache, + Err(err) => err.into_inner(), + }; + if cache.generation == generation { + cache.outcome = Some(CachedPluginLoadOutcome { key, outcome }); + } + } + fn remote_installed_plugin_configs(&self) -> HashMap { let cache = match self.remote_installed_plugins_cache.read() { Ok(cache) => cache, diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index 568b3b2810..f036ed40a5 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -1370,6 +1370,27 @@ async fn plugin_cache_ignores_unrelated_session_overrides() { assert_eq!(second.plugins()[0].mcp_servers.len(), 1); } +#[test] +fn plugin_cache_invalidation_rejects_stale_load_completion() { + let codex_home = TempDir::new().unwrap(); + let manager = PluginsManager::new(codex_home.path().to_path_buf()); + let cache_key = PluginLoadCacheKey { + configured_plugins: HashMap::new(), + skill_config_rules: SkillConfigRules::default(), + remote_plugin_enabled: false, + }; + let stale_generation = manager.enabled_outcome_cache_generation(); + + manager.clear_enabled_outcome_cache(); + manager.cache_enabled_outcome_if_current( + stale_generation, + cache_key.clone(), + PluginLoadOutcome::default(), + ); + + assert_eq!(manager.cached_enabled_outcome(&cache_key), None); +} + #[tokio::test] async fn load_plugins_rejects_invalid_plugin_keys() { let codex_home = TempDir::new().unwrap();