From 2cae3a1cac87f7b632052a1a43243f5ba205ea90 Mon Sep 17 00:00:00 2001 From: Xin Lin Date: Thu, 18 Jun 2026 19:10:56 -0700 Subject: [PATCH] Avoid skill filesystem scans on cache hits --- codex-rs/core-skills/src/service.rs | 124 ++++++++++++--- codex-rs/core-skills/src/service_tests.rs | 179 ++++++++++++++++++++++ 2 files changed, 279 insertions(+), 24 deletions(-) diff --git a/codex-rs/core-skills/src/service.rs b/codex-rs/core-skills/src/service.rs index 1c956d79b9..9e2f896730 100644 --- a/codex-rs/core-skills/src/service.rs +++ b/codex-rs/core-skills/src/service.rs @@ -1,9 +1,13 @@ use std::collections::HashMap; use std::collections::HashSet; +use std::hash::Hash; +use std::hash::Hasher; use std::sync::Arc; use std::sync::RwLock; +use codex_app_server_protocol::ConfigLayerSource; use codex_config::ConfigLayerStack; +use codex_config::ConfigLayerStackOrdering; use codex_exec_server::ExecutorFileSystem; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; @@ -128,13 +132,16 @@ impl SkillsService { input: &SkillsLoadInput, fs: Option>, ) -> HostSkillsSnapshot { - let roots = self.skill_roots_for_config(input, fs).await; - let skill_config_rules = skill_config_rules_from_stack(&input.config_layer_stack); - let cache_key = config_skills_cache_key(&roots, &skill_config_rules); + let extra_roots = self.extra_roots(); + let cache_key = config_skills_cache_key(input, &extra_roots, fs.as_ref()); if let Some(snapshot) = self.cached_snapshot_for_config(&cache_key) { return snapshot; } + let roots = self + .skill_roots_for_config_with_extra_roots(input, fs, extra_roots) + .await; + let skill_config_rules = skill_config_rules_from_stack(&input.config_layer_stack); let snapshot = HostSkillsSnapshot::new(Arc::new( self.build_skill_outcome(input, roots, &skill_config_rules) .await, @@ -151,13 +158,23 @@ impl SkillsService { &self, input: &SkillsLoadInput, fs: Option>, + ) -> Vec { + self.skill_roots_for_config_with_extra_roots(input, fs, self.extra_roots()) + .await + } + + async fn skill_roots_for_config_with_extra_roots( + &self, + input: &SkillsLoadInput, + fs: Option>, + extra_roots: Vec, ) -> Vec { let mut roots = skill_roots( fs, &input.config_layer_stack, &input.cwd, input.effective_skill_roots.clone(), - self.extra_roots(), + extra_roots, ) .await; if !input.bundled_skills_enabled { @@ -268,10 +285,42 @@ impl SkillsService { } } -#[derive(Debug, Clone, PartialEq, Eq, Hash)] +/// Skill-relevant inputs that can be compared before root discovery touches the filesystem. +#[derive(Clone, PartialEq, Eq, Hash)] struct ConfigSkillsCacheKey { - roots: Vec<(AbsolutePathBuf, u8, Option, Option)>, - skill_config_rules: SkillConfigRules, + cwd: AbsolutePathBuf, + config_layers: Vec, + effective_skill_roots: Vec, + bundled_skills_enabled: bool, + extra_roots: Vec, + file_system: Option, +} + +#[derive(Clone, PartialEq, Eq, Hash)] +struct ConfigLayerSkillsCacheKey { + source: std::mem::Discriminant, + config_folder: Option, + disabled: bool, + skills_config: Option, + project_root_markers: Option, +} + +// Snapshots retain filesystem-bound skill paths, so cache entries must distinguish instances. +#[derive(Clone)] +struct ExecutorFileSystemCacheKey(Arc); + +impl PartialEq for ExecutorFileSystemCacheKey { + fn eq(&self, other: &Self) -> bool { + Arc::ptr_eq(&self.0, &other.0) + } +} + +impl Eq for ExecutorFileSystemCacheKey {} + +impl Hash for ExecutorFileSystemCacheKey { + fn hash(&self, state: &mut H) { + std::ptr::hash(Arc::as_ptr(&self.0), state); + } } pub fn bundled_skills_enabled_from_stack( @@ -297,28 +346,55 @@ pub fn bundled_skills_enabled_from_stack( } fn config_skills_cache_key( - roots: &[SkillRoot], - skill_config_rules: &SkillConfigRules, + input: &SkillsLoadInput, + extra_roots: &[AbsolutePathBuf], + fs: Option<&Arc>, ) -> ConfigSkillsCacheKey { ConfigSkillsCacheKey { - roots: roots - .iter() - .map(|root| { - let scope_rank = match root.scope { - SkillScope::Repo => 0, - SkillScope::User => 1, - SkillScope::System => 2, - SkillScope::Admin => 3, + cwd: input.cwd.clone(), + config_layers: input + .config_layer_stack + .get_layers( + ConfigLayerStackOrdering::LowestPrecedenceFirst, + /*include_disabled*/ true, + ) + .into_iter() + .filter_map(|layer| { + let config_folder = layer.config_folder(); + let skills_config = if matches!( + layer.name, + ConfigLayerSource::User { .. } | ConfigLayerSource::SessionFlags + ) { + layer.config.get("skills").map(ToString::to_string) + } else { + None }; - ( - root.path.clone(), - scope_rank, - root.plugin_id.clone(), - root.plugin_namespace.clone(), - ) + let project_root_markers = if !layer.is_disabled() + && !matches!(layer.name, ConfigLayerSource::Project { .. }) + { + layer + .config + .get("project_root_markers") + .map(ToString::to_string) + } else { + None + }; + (config_folder.is_some() + || skills_config.is_some() + || project_root_markers.is_some()) + .then(|| ConfigLayerSkillsCacheKey { + source: std::mem::discriminant(&layer.name), + config_folder, + disabled: layer.is_disabled(), + skills_config, + project_root_markers, + }) }) .collect(), - skill_config_rules: skill_config_rules.clone(), + effective_skill_roots: input.effective_skill_roots.clone(), + bundled_skills_enabled: input.bundled_skills_enabled, + extra_roots: extra_roots.to_vec(), + file_system: fs.cloned().map(ExecutorFileSystemCacheKey), } } diff --git a/codex-rs/core-skills/src/service_tests.rs b/codex-rs/core-skills/src/service_tests.rs index 23a15f7955..6b08dd9801 100644 --- a/codex-rs/core-skills/src/service_tests.rs +++ b/codex-rs/core-skills/src/service_tests.rs @@ -7,10 +7,20 @@ use codex_config::CONFIG_TOML_FILE; use codex_config::ConfigLayerEntry; use codex_config::ConfigLayerStack; use codex_config::ConfigRequirementsToml; +use codex_exec_server::CopyOptions; +use codex_exec_server::CreateDirectoryOptions; +use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::ExecutorFileSystemFuture; +use codex_exec_server::FileMetadata; +use codex_exec_server::FileSystemReadStream; +use codex_exec_server::FileSystemSandboxContext; use codex_exec_server::LOCAL_FS; +use codex_exec_server::ReadDirectoryEntry; +use codex_exec_server::RemoveOptions; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::test_support::PathBufExt; use codex_utils_absolute_path::test_support::PathExt; +use codex_utils_path_uri::PathUri; use codex_utils_plugins::PluginSkillRoot; use pretty_assertions::assert_eq; use std::collections::HashSet; @@ -18,8 +28,121 @@ use std::fs; use std::path::Path; use std::path::PathBuf; use std::sync::Arc; +use std::sync::atomic::AtomicUsize; +use std::sync::atomic::Ordering; use tempfile::TempDir; +struct CountingFileSystem { + delegate: Arc, + operation_count: AtomicUsize, +} + +impl CountingFileSystem { + fn new(delegate: Arc) -> Self { + Self { + delegate, + operation_count: AtomicUsize::new(0), + } + } + + fn operation_count(&self) -> usize { + self.operation_count.load(Ordering::Relaxed) + } + + fn record_operation(&self) { + self.operation_count.fetch_add(1, Ordering::Relaxed); + } +} + +impl ExecutorFileSystem for CountingFileSystem { + fn canonicalize<'a>( + &'a self, + path: &'a PathUri, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, PathUri> { + self.record_operation(); + self.delegate.canonicalize(path, sandbox) + } + + fn read_file<'a>( + &'a self, + path: &'a PathUri, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, Vec> { + self.record_operation(); + self.delegate.read_file(path, sandbox) + } + + fn read_file_stream<'a>( + &'a self, + path: &'a PathUri, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, FileSystemReadStream> { + self.record_operation(); + self.delegate.read_file_stream(path, sandbox) + } + + fn write_file<'a>( + &'a self, + path: &'a PathUri, + contents: Vec, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, ()> { + self.record_operation(); + self.delegate.write_file(path, contents, sandbox) + } + + fn create_directory<'a>( + &'a self, + path: &'a PathUri, + options: CreateDirectoryOptions, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, ()> { + self.record_operation(); + self.delegate.create_directory(path, options, sandbox) + } + + fn get_metadata<'a>( + &'a self, + path: &'a PathUri, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, FileMetadata> { + self.record_operation(); + self.delegate.get_metadata(path, sandbox) + } + + fn read_directory<'a>( + &'a self, + path: &'a PathUri, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, Vec> { + self.record_operation(); + self.delegate.read_directory(path, sandbox) + } + + fn remove<'a>( + &'a self, + path: &'a PathUri, + options: RemoveOptions, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, ()> { + self.record_operation(); + self.delegate.remove(path, options, sandbox) + } + + fn copy<'a>( + &'a self, + source_path: &'a PathUri, + destination_path: &'a PathUri, + options: CopyOptions, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, ()> { + self.record_operation(); + self.delegate + .copy(source_path, destination_path, options, sandbox) + } +} + fn write_user_skill(codex_home: &TempDir, dir: &str, name: &str, description: &str) { let skill_dir = codex_home.path().join("skills").join(dir); fs::create_dir_all(&skill_dir).unwrap(); @@ -228,6 +351,62 @@ async fn skills_for_config_reuses_cache_for_same_effective_config() { assert_eq!(outcome2.skills, outcome1.skills); } +#[tokio::test] +async fn skills_for_config_cache_hit_avoids_environment_filesystem_operations() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + let config_layer_stack = config_stack(&codex_home, ""); + let skills_service = SkillsService::new( + codex_home.path().abs(), + /*bundled_skills_enabled*/ true, + ); + let skills_input = SkillsLoadInput::new( + cwd.path().abs(), + Vec::new(), + config_layer_stack, + /*bundled_skills_enabled*/ true, + ); + let counting_file_system = Arc::new(CountingFileSystem::new(Arc::clone(&LOCAL_FS))); + let file_system: Arc = counting_file_system.clone(); + + skills_service + .snapshot_for_config(&skills_input, Some(Arc::clone(&file_system))) + .await; + let operations_after_cache_miss = counting_file_system.operation_count(); + assert!(operations_after_cache_miss > 0); + + skills_service + .snapshot_for_config(&skills_input, Some(Arc::clone(&file_system))) + .await; + assert_eq!( + counting_file_system.operation_count(), + operations_after_cache_miss + ); + + let unrelated_session_flag_input = SkillsLoadInput::new( + cwd.path().abs(), + Vec::new(), + config_stack_with_session_flags(&codex_home, "", "model = 'gpt-5'"), + /*bundled_skills_enabled*/ true, + ); + skills_service + .snapshot_for_config( + &unrelated_session_flag_input, + Some(Arc::clone(&file_system)), + ) + .await; + assert_eq!( + counting_file_system.operation_count(), + operations_after_cache_miss + ); + + skills_service.clear_cache(); + skills_service + .snapshot_for_config(&skills_input, Some(file_system)) + .await; + assert!(counting_file_system.operation_count() > operations_after_cache_miss); +} + #[tokio::test] async fn set_extra_roots_replaces_runtime_roots_and_clears_cache() { let codex_home = tempfile::tempdir().expect("tempdir");