From 58838dfa5ba1d4b5383898cc2d08f270323d62d0 Mon Sep 17 00:00:00 2001
From: jif <jif@openai.com>
Date: Thu, 24 Sep 2026 14:38:24 +0000
Subject: [PATCH] Validate loaded plugins outside the cache lock (#47858)

## Why

Checking installed plugin roots performs filesystem work while holding the loaded plugins cache mutex, blocking lookups for other cache keys.

## What changed

Store cache entries in `Arc`s and release the mutex before validating installation paths. After reacquiring the lock, check the cache generation and entry identity to reject entries invalidated or replaced during validation. Preserve stale-entry eviction and most-recently-used ordering.

GitOrigin-RevId: cb5a5f7ec52cba852e6bb9da03a8e214cc19727d
---
 codex-rs/core-plugins/src/manager.rs | 51 +++++++++++++++++-----------
 1 file changed, 32 insertions(+), 19 deletions(-)

diff --git a/codex-rs/core-plugins/src/manager.rs b/codex-rs/core-plugins/src/manager.rs
index dbe52a569b..3312b40120 100644
--- a/codex-rs/core-plugins/src/manager.rs
+++ b/codex-rs/core-plugins/src/manager.rs
@@ -564,20 +564,26 @@ struct LoadedPluginsCacheEntry {
 struct LoadedPluginsCache {
     generation: u64,
     // Most recently used first.
-    entries: VecDeque<LoadedPluginsCacheEntry>,
+    entries: VecDeque<Arc<LoadedPluginsCacheEntry>>,
 }
 
 impl LoadedPluginsCache {
     fn get(
-        &mut self,
+        cache: &Mutex<Self>,
         key: &PluginLoadCacheKey,
         store: &PluginStore,
-    ) -> Option<&LoadedPluginsCacheEntry> {
-        let index = self.entries.iter().position(|entry| &entry.key == key)?;
-        let entry = self.entries.remove(index)?;
+    ) -> Option<Arc<LoadedPluginsCacheEntry>> {
+        let (generation, entry) = {
+            let cache = cache
+                .lock()
+                .unwrap_or_else(std::sync::PoisonError::into_inner);
+            let entry = cache.entries.iter().find(|entry| &entry.key == key)?;
+            (cache.generation, Arc::clone(entry))
+        };
         // Another process can replace installed versions without invalidating this manager.
         // Keep the parsed skills paired with the installation whose paths they advertise.
-        if entry.plugins.iter().any(|plugin| {
+        // Validate outside the cache lock so filesystem work does not block other cache keys.
+        let stale = entry.plugins.iter().any(|plugin| {
             let Ok(plugin_id) = PluginId::parse(&plugin.config_name) else {
                 return false;
             };
@@ -585,11 +591,24 @@ impl LoadedPluginsCache {
                 .active_plugin_root(&plugin_id)
                 .unwrap_or_else(|| store.plugin_base_root(&plugin_id));
             installed_root != plugin.root
-        }) {
+        });
+        let mut cache = cache
+            .lock()
+            .unwrap_or_else(std::sync::PoisonError::into_inner);
+        if cache.generation != generation {
             return None;
         }
-        self.entries.push_front(entry);
-        self.entries.front()
+        // A load can replace this key without changing the cache generation.
+        let index = cache
+            .entries
+            .iter()
+            .position(|current| Arc::ptr_eq(current, &entry))?;
+        let current = cache.entries.remove(index)?;
+        if stale {
+            return None;
+        }
+        cache.entries.push_front(current);
+        Some(entry)
     }
 }
 
@@ -757,10 +776,7 @@ impl PluginsManager {
             self.remote_global_catalog_active(config),
             RemoteInstalledPluginsAuthIdentity::from_auth(auth.as_ref()),
         );
-        self.loaded_plugins_cache
-            .lock()
-            .unwrap_or_else(std::sync::PoisonError::into_inner)
-            .get(&key, &self.store)
+        LoadedPluginsCache::get(&self.loaded_plugins_cache, &key, &self.store)
             .map(|cached| cached.plugin_skill_snapshots.clone())
     }
 
@@ -975,10 +991,7 @@ impl PluginsManager {
     }
 
     fn cached_loaded_plugins(&self, key: &PluginLoadCacheKey) -> Option<Vec<LoadedPlugin>> {
-        self.loaded_plugins_cache
-            .lock()
-            .unwrap_or_else(std::sync::PoisonError::into_inner)
-            .get(key, &self.store)
+        LoadedPluginsCache::get(&self.loaded_plugins_cache, key, &self.store)
             .map(|cached| cached.plugins.clone())
     }
 
@@ -1004,11 +1017,11 @@ impl PluginsManager {
             return;
         }
         cache.entries.retain(|entry| entry.key != key);
-        cache.entries.push_front(LoadedPluginsCacheEntry {
+        cache.entries.push_front(Arc::new(LoadedPluginsCacheEntry {
             key,
             plugins,
             plugin_skill_snapshots,
-        });
+        }));
         let evicted = cache.entries.len() > LOADED_PLUGINS_CACHE_CAPACITY;
         cache.entries.truncate(LOADED_PLUGINS_CACHE_CAPACITY);
         drop(cache);