mirror of
https://github.com/openai/codex.git
synced 2026-11-13 06:45:07 +00:00
Refresh plugin skills after out-of-process version changes (#42284)
## Why Another process sharing `CODEX_HOME` can replace an installed plugin version without invalidating the current process's caches. This could leave skill listings tied to stale plugin paths and retain obsolete plugin generations. ## What changed - Reject cached plugin loads when their roots no longer match the active installation, so skills are reloaded after upgrades or rollbacks. - Keep the 32 most recently used configuration-based skill snapshots while allowing callers to continue using snapshots that have been evicted. ## Testing - Cover external plugin upgrades and rollbacks through `skills/list`, including a subsequent warm-cache read. - Cover cache eviction, reuse, and the lifetime of caller-held snapshots. GitOrigin-RevId: ca00f9539c01461e3945d340bf63f8436665220b
This commit is contained in:
1 parent
8d32abcd01
commit
50fffd5ed3
4 files changed
+218
-37
No files matched your search
@@ -1,4 +1,5 @@
|
||||
use std::collections::HashMap;
|
||||
use std::collections::VecDeque;
|
||||
use std::hash::Hash;
|
||||
use std::hash::Hasher;
|
||||
use std::sync::Arc;
|
||||
@@ -37,6 +38,13 @@ use crate::loader::MAX_CONCURRENT_ROOT_SCANS;
|
||||
use crate::loader::load_and_merge_host_skill_roots;
|
||||
use crate::loader::load_and_merge_host_skill_roots_with_request_snapshots;
|
||||
|
||||
const CONFIG_SKILLS_CACHE_CAPACITY: usize = 32;
|
||||
|
||||
struct ConfigSkillsCacheEntry {
|
||||
key: ConfigSkillsCacheKey,
|
||||
snapshot: Arc<OnceCell<HostSkillsSnapshot>>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct HostSkillsLoadInput {
|
||||
cwd: AbsolutePathBuf,
|
||||
@@ -77,7 +85,7 @@ pub struct HostSkillsService {
|
||||
restriction_product: Option<Product>,
|
||||
extra_roots: RwLock<Vec<AbsolutePathBuf>>,
|
||||
cache_by_cwd: RwLock<HashMap<AbsolutePathBuf, HostSkillsSnapshot>>,
|
||||
cache_by_config: RwLock<HashMap<ConfigSkillsCacheKey, Arc<OnceCell<HostSkillsSnapshot>>>>,
|
||||
cache_by_config: RwLock<VecDeque<ConfigSkillsCacheEntry>>,
|
||||
// Shared across cwds so root scheduling cannot multiply per-root I/O fanout.
|
||||
root_scan_slots: Arc<Semaphore>,
|
||||
}
|
||||
@@ -124,7 +132,7 @@ impl HostSkillsService {
|
||||
restriction_product,
|
||||
extra_roots: RwLock::new(Vec::new()),
|
||||
cache_by_cwd: RwLock::new(HashMap::new()),
|
||||
cache_by_config: RwLock::new(HashMap::new()),
|
||||
cache_by_config: RwLock::new(VecDeque::new()),
|
||||
root_scan_slots: Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)),
|
||||
};
|
||||
// The cache is shared by every process using this CODEX_HOME. Disabled services filter
|
||||
@@ -178,10 +186,6 @@ impl HostSkillsService {
|
||||
&skill_config_rules,
|
||||
input.plugin_skill_snapshots.as_ref(),
|
||||
);
|
||||
if let Some(snapshot) = self.cached_snapshot_for_config(&cache_key) {
|
||||
return snapshot;
|
||||
}
|
||||
|
||||
self.snapshot_for_skill_roots(
|
||||
input,
|
||||
roots,
|
||||
@@ -310,17 +314,21 @@ impl HostSkillsService {
|
||||
.cache_by_config
|
||||
.write()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner);
|
||||
if force_reload {
|
||||
let snapshot_cell = Arc::new(OnceCell::new());
|
||||
cache.insert(cache_key, Arc::clone(&snapshot_cell));
|
||||
snapshot_cell
|
||||
} else {
|
||||
Arc::clone(
|
||||
cache
|
||||
.entry(cache_key)
|
||||
.or_insert_with(|| Arc::new(OnceCell::new())),
|
||||
)
|
||||
}
|
||||
let snapshot_cell = cache
|
||||
.iter()
|
||||
.position(|entry| entry.key == cache_key)
|
||||
.and_then(|index| cache.remove(index))
|
||||
.filter(|_| !force_reload)
|
||||
.map(|entry| entry.snapshot)
|
||||
.unwrap_or_else(|| Arc::new(OnceCell::new()));
|
||||
cache.push_front(ConfigSkillsCacheEntry {
|
||||
key: cache_key,
|
||||
snapshot: Arc::clone(&snapshot_cell),
|
||||
});
|
||||
// Keys retain parsed plugin generations; eviction releases obsolete catalogs while
|
||||
// callers and in-flight loads keep their own snapshots alive.
|
||||
cache.truncate(CONFIG_SKILLS_CACHE_CAPACITY);
|
||||
snapshot_cell
|
||||
};
|
||||
|
||||
snapshot_cell
|
||||
@@ -394,23 +402,6 @@ impl HostSkillsService {
|
||||
}
|
||||
}
|
||||
|
||||
fn cached_snapshot_for_config(
|
||||
&self,
|
||||
cache_key: &ConfigSkillsCacheKey,
|
||||
) -> Option<HostSkillsSnapshot> {
|
||||
match self.cache_by_config.read() {
|
||||
Ok(cache) => cache
|
||||
.get(cache_key)
|
||||
.and_then(|snapshot| snapshot.get())
|
||||
.cloned(),
|
||||
Err(err) => err
|
||||
.into_inner()
|
||||
.get(cache_key)
|
||||
.and_then(|snapshot| snapshot.get())
|
||||
.cloned(),
|
||||
}
|
||||
}
|
||||
|
||||
fn extra_roots(&self) -> Vec<AbsolutePathBuf> {
|
||||
match self.extra_roots.read() {
|
||||
Ok(roots) => roots.clone(),
|
||||
|
||||
@@ -207,6 +207,88 @@ async fn skills_for_config_reuses_cache_for_same_effective_config() {
|
||||
assert_eq!(outcome2.skills, outcome1.skills);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn skills_for_config_bounds_plugin_generations_and_preserves_live_snapshots() {
|
||||
let codex_home = tempfile::tempdir().expect("tempdir");
|
||||
let cwd = tempfile::tempdir().expect("tempdir");
|
||||
let skill_path = write_plugin_skill(
|
||||
&codex_home,
|
||||
"test",
|
||||
"sample",
|
||||
"search",
|
||||
"search",
|
||||
"initial description",
|
||||
);
|
||||
let plugin_root = plugin_skill_root_for_skill_path(&skill_path, "sample@test", "sample");
|
||||
let base_input = HostSkillsLoadInput::new(
|
||||
cwd.path().abs(),
|
||||
vec![plugin_root],
|
||||
config_stack(&codex_home, "[skills.bundled]\nenabled = false\n"),
|
||||
);
|
||||
let skills_service = HostSkillsService::new(
|
||||
codex_home.path().abs(),
|
||||
/*bundled_skills_enabled*/ false,
|
||||
);
|
||||
let mut generations = Vec::new();
|
||||
let mut recent = None;
|
||||
let mut held_snapshot = None;
|
||||
let mut held_skills = Vec::new();
|
||||
|
||||
// Reloads allocate new handles even when roots repeat, as they do after a rollback.
|
||||
for generation in 0..CONFIG_SKILLS_CACHE_CAPACITY + 2 {
|
||||
fs::write(
|
||||
&skill_path,
|
||||
format!("---\nname: search\ndescription: generation {generation}\n---\n\n# Body\n"),
|
||||
)
|
||||
.expect("write updated skill");
|
||||
let owner = Arc::new(TestPluginSkillSnapshotCache::default());
|
||||
generations.push(Arc::downgrade(&owner));
|
||||
let input = base_input
|
||||
.clone()
|
||||
.with_plugin_skill_snapshots(Some(SkillRootSnapshots::new(owner)));
|
||||
let snapshot = skills_service
|
||||
.snapshot_for_config(&input, Some(Arc::clone(&LOCAL_FS)))
|
||||
.await;
|
||||
|
||||
if generation == 0 {
|
||||
recent = Some((input, snapshot));
|
||||
} else if generation == 1 {
|
||||
held_skills = snapshot.outcome().skills.clone();
|
||||
held_snapshot = Some(snapshot);
|
||||
}
|
||||
if generation == CONFIG_SKILLS_CACHE_CAPACITY - 1 {
|
||||
let (input, snapshot) = recent.as_ref().expect("first generation");
|
||||
let reused = skills_service
|
||||
.snapshot_for_config(input, Some(Arc::clone(&LOCAL_FS)))
|
||||
.await;
|
||||
assert!(std::ptr::eq(snapshot.outcome(), reused.outcome()));
|
||||
}
|
||||
}
|
||||
drop(recent);
|
||||
|
||||
let retained_generations = generations
|
||||
.iter()
|
||||
.enumerate()
|
||||
.filter_map(|(generation, owner)| owner.upgrade().map(|_| generation))
|
||||
.collect::<Vec<_>>();
|
||||
assert_eq!(
|
||||
retained_generations,
|
||||
std::iter::once(/*value*/ 0)
|
||||
.chain(3..CONFIG_SKILLS_CACHE_CAPACITY + 2)
|
||||
.collect::<Vec<_>>()
|
||||
);
|
||||
assert_eq!(
|
||||
held_snapshot
|
||||
.expect("evicted live snapshot")
|
||||
.outcome()
|
||||
.skills,
|
||||
held_skills
|
||||
);
|
||||
|
||||
skills_service.clear_cache();
|
||||
assert_eq!(generations.iter().filter_map(Weak::upgrade).count(), 0);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn watchable_skill_root_paths_exclude_plugin_and_system_roots() {
|
||||
let codex_home = tempfile::tempdir().expect("tempdir");
|
||||
|
||||
Reference in new issue
Block a user