mirror of
https://github.com/openai/codex.git
synced 2026-10-18 17:18:52 +00:00
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
This commit is contained in:
@@ -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);
|
||||
|
||||
Reference in New Issue
Block a user