Scan skill roots concurrently (#33369)

## What changed

- Scan up to eight skill roots concurrently instead of loading them one at a time.
- Share scan capacity across host-skill and plugin-skill loads to bound aggregate filesystem work.
- Merge completed scans in input order so root precedence and error ordering remain deterministic.
- Skip canonicalization and parsing work for empty roots.

## Testing

- Add coverage for out-of-order scan completion and shared-capacity blocking.

GitOrigin-RevId: 33a47aaa02aee7fe31a050bb04d51c1e2043d6ae
This commit is contained in:
Adam Perry @ OpenAI authored and copyberry committed 2026-07-15 15:56:53 +00:00
1 parent c28770a42f
commit 2e4f55608b
11 files changed
+288 -48

No files matched your search

+10 -1
View File
@@ -53,6 +53,7 @@ use std::path::Path;
use std::process::Command; use std::process::Command;
use std::sync::Arc; use std::sync::Arc;
use tempfile::TempDir; use tempfile::TempDir;
use tokio::sync::Semaphore;
use tracing::instrument; use tracing::instrument;
use tracing::warn; use tracing::warn;
@@ -75,6 +76,7 @@ enum PluginLoadScope<'a> {
restriction_product: Option<Product>, restriction_product: Option<Product>,
skill_config_rules: &'a SkillConfigRules, skill_config_rules: &'a SkillConfigRules,
plugin_skill_snapshots: Option<&'a PluginSkillSnapshots>, plugin_skill_snapshots: Option<&'a PluginSkillSnapshots>,
root_scan_slots: Arc<Semaphore>,
}, },
HooksOnly, HooksOnly,
} }
@@ -118,6 +120,7 @@ pub(crate) async fn load_plugins_from_layer_stack(
plugin_skill_snapshots: Option<&PluginSkillSnapshots>, plugin_skill_snapshots: Option<&PluginSkillSnapshots>,
restriction_product: Option<Product>, restriction_product: Option<Product>,
remote_global_catalog_active: bool, remote_global_catalog_active: bool,
root_scan_slots: Arc<Semaphore>,
) -> Vec<LoadedPlugin<McpServerConfig>> { ) -> Vec<LoadedPlugin<McpServerConfig>> {
let skill_config_rules = skill_config_rules_from_stack(config_layer_stack); let skill_config_rules = skill_config_rules_from_stack(config_layer_stack);
load_plugins_from_layer_stack_with_scope( load_plugins_from_layer_stack_with_scope(
@@ -129,6 +132,7 @@ pub(crate) async fn load_plugins_from_layer_stack(
restriction_product, restriction_product,
skill_config_rules: &skill_config_rules, skill_config_rules: &skill_config_rules,
plugin_skill_snapshots, plugin_skill_snapshots,
root_scan_slots,
}, },
) )
.await .await
@@ -809,6 +813,7 @@ async fn load_plugin(
restriction_product, restriction_product,
skill_config_rules, skill_config_rules,
plugin_skill_snapshots, plugin_skill_snapshots,
root_scan_slots,
} => { } => {
loaded_plugin.manifest_name = Some(manifest.display_name().to_string()); loaded_plugin.manifest_name = Some(manifest.display_name().to_string());
loaded_plugin.manifest_description = manifest.description.clone(); loaded_plugin.manifest_description = manifest.description.clone();
@@ -820,6 +825,7 @@ async fn load_plugin(
*restriction_product, *restriction_product,
skill_config_rules, skill_config_rules,
*plugin_skill_snapshots, *plugin_skill_snapshots,
Arc::clone(root_scan_slots),
) )
.await; .await;
let has_enabled_skills = resolved_skills.has_enabled_skills(); let has_enabled_skills = resolved_skills.has_enabled_skills();
@@ -917,6 +923,7 @@ pub async fn load_plugin_skills(
restriction_product: Option<Product>, restriction_product: Option<Product>,
skill_config_rules: &SkillConfigRules, skill_config_rules: &SkillConfigRules,
plugin_skill_snapshots: Option<&PluginSkillSnapshots>, plugin_skill_snapshots: Option<&PluginSkillSnapshots>,
root_scan_slots: Arc<Semaphore>,
) -> ResolvedPluginSkills { ) -> ResolvedPluginSkills {
load_plugin_skill_inventory( load_plugin_skill_inventory(
plugin_root, plugin_root,
@@ -924,6 +931,7 @@ pub async fn load_plugin_skills(
manifest, manifest,
restriction_product, restriction_product,
plugin_skill_snapshots, plugin_skill_snapshots,
root_scan_slots,
) )
.await .await
.resolve(skill_config_rules) .resolve(skill_config_rules)
@@ -935,6 +943,7 @@ pub(crate) async fn load_plugin_skill_inventory(
manifest: &PluginManifest, manifest: &PluginManifest,
restriction_product: Option<Product>, restriction_product: Option<Product>,
plugin_skill_snapshots: Option<&PluginSkillSnapshots>, plugin_skill_snapshots: Option<&PluginSkillSnapshots>,
root_scan_slots: Arc<Semaphore>,
) -> PluginSkillInventory { ) -> PluginSkillInventory {
let roots = plugin_skill_roots(plugin_root, &manifest.paths) let roots = plugin_skill_roots(plugin_root, &manifest.paths)
.into_iter() .into_iter()
@@ -947,7 +956,7 @@ pub(crate) async fn load_plugin_skill_inventory(
plugin_root: Some(plugin_root.clone()), plugin_root: Some(plugin_root.clone()),
}) })
.collect::<Vec<_>>(); .collect::<Vec<_>>();
let outcome = load_skills_from_roots(roots, plugin_skill_snapshots).await; let outcome = load_skills_from_roots(roots, plugin_skill_snapshots, root_scan_slots).await;
let had_errors = !outcome.errors.is_empty(); let had_errors = !outcome.errors.is_empty();
let skills = outcome let skills = outcome
.skills .skills
@@ -5,6 +5,7 @@ use codex_config::ConfigLayerEntry;
use codex_config::ConfigLayerSource; use codex_config::ConfigLayerSource;
use codex_config::ConfigRequirements; use codex_config::ConfigRequirements;
use codex_config::ConfigRequirementsToml; use codex_config::ConfigRequirementsToml;
use codex_core_skills::loader::MAX_CONCURRENT_ROOT_SCANS;
use codex_plugin::PluginId; use codex_plugin::PluginId;
use pretty_assertions::assert_eq; use pretty_assertions::assert_eq;
use tempfile::TempDir; use tempfile::TempDir;
@@ -163,6 +164,7 @@ enabled = true
/*plugin_skill_snapshots*/ None, /*plugin_skill_snapshots*/ None,
Some(Product::Codex), Some(Product::Codex),
/*remote_global_catalog_active*/ false, /*remote_global_catalog_active*/ false,
Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)),
) )
.await; .await;
let hooks_only = load_plugins_from_layer_stack_with_scope( let hooks_only = load_plugins_from_layer_stack_with_scope(
+12 -1
View File
@@ -70,6 +70,7 @@ use codex_core_skills::PluginSkillSnapshots;
use codex_core_skills::SkillMetadata; use codex_core_skills::SkillMetadata;
use codex_core_skills::config_rules::SkillConfigRules; use codex_core_skills::config_rules::SkillConfigRules;
use codex_core_skills::config_rules::skill_config_rules_from_stack; use codex_core_skills::config_rules::skill_config_rules_from_stack;
use codex_core_skills::loader::MAX_CONCURRENT_ROOT_SCANS;
use codex_hooks::plugin_hook_declarations; use codex_hooks::plugin_hook_declarations;
use codex_login::AuthManager; use codex_login::AuthManager;
use codex_login::CodexAuth; use codex_login::CodexAuth;
@@ -376,6 +377,7 @@ pub struct PluginsManager {
// Keep the cache auth-independent so auth changes only need to resolve capabilities again. // Keep the cache auth-independent so auth changes only need to resolve capabilities again.
loaded_plugins_cache: RwLock<LoadedPluginsCache>, loaded_plugins_cache: RwLock<LoadedPluginsCache>,
loaded_plugins_load_semaphore: Semaphore, loaded_plugins_load_semaphore: Semaphore,
skill_root_scan_slots: Arc<Semaphore>,
tool_suggest_metadata_cache: ToolSuggestMetadataCache, tool_suggest_metadata_cache: ToolSuggestMetadataCache,
remote_installed_plugins_cache: RwLock<Option<Vec<RemoteInstalledPlugin>>>, remote_installed_plugins_cache: RwLock<Option<Vec<RemoteInstalledPlugin>>>,
remote_installed_plugins_cache_refresh_state: RwLock<RemoteInstalledPluginsCacheRefreshState>, remote_installed_plugins_cache_refresh_state: RwLock<RemoteInstalledPluginsCacheRefreshState>,
@@ -452,6 +454,7 @@ impl PluginsManager {
non_curated_cache_refresh_state: RwLock::new(NonCuratedCacheRefreshState::default()), non_curated_cache_refresh_state: RwLock::new(NonCuratedCacheRefreshState::default()),
loaded_plugins_cache: RwLock::new(LoadedPluginsCache::default()), loaded_plugins_cache: RwLock::new(LoadedPluginsCache::default()),
loaded_plugins_load_semaphore: Semaphore::new(/*permits*/ 1), loaded_plugins_load_semaphore: Semaphore::new(/*permits*/ 1),
skill_root_scan_slots: Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)),
tool_suggest_metadata_cache: ToolSuggestMetadataCache::new(), tool_suggest_metadata_cache: ToolSuggestMetadataCache::new(),
remote_installed_plugins_cache: RwLock::new(None), remote_installed_plugins_cache: RwLock::new(None),
remote_installed_plugins_cache_refresh_state: RwLock::new( remote_installed_plugins_cache_refresh_state: RwLock::new(
@@ -585,6 +588,7 @@ impl PluginsManager {
Some(&plugin_skill_snapshots), Some(&plugin_skill_snapshots),
self.restriction_product, self.restriction_product,
remote_global_catalog_active, remote_global_catalog_active,
Arc::clone(&self.skill_root_scan_slots),
) )
.await; .await;
log_plugin_load_errors(&plugins); log_plugin_load_errors(&plugins);
@@ -670,6 +674,7 @@ impl PluginsManager {
/*plugin_skill_snapshots*/ None, /*plugin_skill_snapshots*/ None,
self.restriction_product, self.restriction_product,
self.remote_global_catalog_active(config), self.remote_global_catalog_active(config),
Arc::clone(&self.skill_root_scan_slots),
) )
.await; .await;
self.resolve_loaded_plugins_for_auth(plugins) self.resolve_loaded_plugins_for_auth(plugins)
@@ -1708,7 +1713,12 @@ impl PluginsManager {
) -> Result<PluginCapabilitySummary, MarketplaceError> { ) -> Result<PluginCapabilitySummary, MarketplaceError> {
let fragment = self let fragment = self
.tool_suggest_metadata_cache .tool_suggest_metadata_cache
.metadata_for_plugin(marketplace_name, plugin, self.restriction_product) .metadata_for_plugin(
marketplace_name,
plugin,
self.restriction_product,
Arc::clone(&self.skill_root_scan_slots),
)
.await?; .await?;
Ok(fragment.project(skill_config_rules, self.auth_mode())) Ok(fragment.project(skill_config_rules, self.auth_mode()))
} }
@@ -1890,6 +1900,7 @@ impl PluginsManager {
&config.config_layer_stack, &config.config_layer_stack,
), ),
/*plugin_skill_snapshots*/ None, /*plugin_skill_snapshots*/ None,
Arc::clone(&self.skill_root_scan_slots),
) )
.await; .await;
let plugin_data_root = self.store.plugin_data_root(&plugin_id); let plugin_data_root = self.store.plugin_data_root(&plugin_id);
@@ -1928,6 +1928,7 @@ async fn load_plugin_skills_dedupes_overlapping_manifest_roots() {
/*restriction_product*/ None, /*restriction_product*/ None,
&SkillConfigRules::default(), &SkillConfigRules::default(),
/*plugin_skill_snapshots*/ None, /*plugin_skill_snapshots*/ None,
Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)),
) )
.await; .await;
@@ -5822,6 +5823,7 @@ async fn load_plugins_ignores_project_config_files() {
/*plugin_skill_snapshots*/ None, /*plugin_skill_snapshots*/ None,
Some(Product::Codex), Some(Product::Codex),
/*remote_global_catalog_active*/ false, /*remote_global_catalog_active*/ false,
Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)),
) )
.await; .await;
@@ -118,6 +118,7 @@ impl ToolSuggestMetadataCache {
marketplace_name: &str, marketplace_name: &str,
plugin: &ConfiguredMarketplacePlugin, plugin: &ConfiguredMarketplacePlugin,
restriction_product: Option<Product>, restriction_product: Option<Product>,
root_scan_slots: Arc<Semaphore>,
) -> Result<Arc<ToolSuggestMetadataFragment>, MarketplaceError> { ) -> Result<Arc<ToolSuggestMetadataFragment>, MarketplaceError> {
let artifact = PluginArtifactIdentity { let artifact = PluginArtifactIdentity {
plugin_id: plugin.id.clone(), plugin_id: plugin.id.clone(),
@@ -138,7 +139,13 @@ impl ToolSuggestMetadataCache {
} }
let generation = self.generation(); let generation = self.generation();
let entry = load_plugin_metadata(marketplace_name, plugin, restriction_product).await; let entry = load_plugin_metadata(
marketplace_name,
plugin,
restriction_product,
Arc::clone(&root_scan_slots),
)
.await;
if self.cache_entry_if_current(generation, artifact.clone(), entry.clone()) { if self.cache_entry_if_current(generation, artifact.clone(), entry.clone()) {
return entry.map_err(MarketplaceError::InvalidPlugin); return entry.map_err(MarketplaceError::InvalidPlugin);
} }
@@ -186,6 +193,7 @@ async fn load_plugin_metadata(
marketplace_name: &str, marketplace_name: &str,
plugin: &ConfiguredMarketplacePlugin, plugin: &ConfiguredMarketplacePlugin,
restriction_product: Option<Product>, restriction_product: Option<Product>,
root_scan_slots: Arc<Semaphore>,
) -> ToolSuggestMetadataEntry { ) -> ToolSuggestMetadataEntry {
let plugin_id = PluginId::new(plugin.name.clone(), marketplace_name.to_string()).map_err( let plugin_id = PluginId::new(plugin.name.clone(), marketplace_name.to_string()).map_err(
|err| match err { |err| match err {
@@ -216,6 +224,7 @@ async fn load_plugin_metadata(
&manifest, &manifest,
restriction_product, restriction_product,
/*plugin_skill_snapshots*/ None, /*plugin_skill_snapshots*/ None,
root_scan_slots,
) )
.await; .await;
let mut mcp_server_names = let mut mcp_server_names =
+10 -1
View File
@@ -49,9 +49,13 @@ use std::path::Component;
use std::path::Path; use std::path::Path;
use std::path::PathBuf; use std::path::PathBuf;
use std::sync::Arc; use std::sync::Arc;
use tokio::sync::Semaphore;
use toml::Value as TomlValue; use toml::Value as TomlValue;
use tracing::error; use tracing::error;
// TODO(anp): Tune this eight-scan limit after revisiting byte-based backpressure.
pub const MAX_CONCURRENT_ROOT_SCANS: usize = 8;
#[derive(Debug, Deserialize)] #[derive(Debug, Deserialize)]
struct SkillFrontmatter { struct SkillFrontmatter {
#[serde(default)] #[serde(default)]
@@ -195,12 +199,13 @@ pub struct SkillRoot {
pub async fn load_skills_from_roots<I>( pub async fn load_skills_from_roots<I>(
roots: I, roots: I,
plugin_skill_snapshots: Option<&crate::PluginSkillSnapshots>, plugin_skill_snapshots: Option<&crate::PluginSkillSnapshots>,
root_scan_slots: Arc<Semaphore>,
) -> SkillLoadOutcome ) -> SkillLoadOutcome
where where
I: IntoIterator<Item = SkillRoot> + Send, I: IntoIterator<Item = SkillRoot> + Send,
I::IntoIter: Send, I::IntoIter: Send,
{ {
crate::root_loader::load_and_merge_skill_roots(roots, plugin_skill_snapshots) crate::root_loader::load_and_merge_skill_roots(roots, plugin_skill_snapshots, &root_scan_slots)
.boxed() .boxed()
.await .await
} }
@@ -558,6 +563,10 @@ async fn load_skills_under_root(
for warning in warnings { for warning in warnings {
error!("{warning}"); error!("{warning}");
} }
// With no skills, there is nothing to canonicalize, parse, or namespace-qualify.
if skills.is_empty() {
return;
}
let root_uri = PathUri::from_abs_path(root); let root_uri = PathUri::from_abs_path(root);
let resolved_skills = futures::stream::iter(skills) let resolved_skills = futures::stream::iter(skills)
.map(|skill| async move { .map(|skill| async move {
+164 -2
View File
@@ -29,6 +29,8 @@ use std::path::Path;
use std::path::PathBuf; use std::path::PathBuf;
use std::sync::Arc; use std::sync::Arc;
use std::sync::Mutex; use std::sync::Mutex;
use std::sync::atomic::AtomicUsize;
use std::sync::atomic::Ordering;
use tempfile::TempDir; use tempfile::TempDir;
use tokio::sync::Notify; use tokio::sync::Notify;
use tokio::sync::Semaphore; use tokio::sync::Semaphore;
@@ -44,6 +46,10 @@ struct TestConfig {
struct BlockingRepoSkillRootFileSystem { struct BlockingRepoSkillRootFileSystem {
inner: Arc<dyn ExecutorFileSystem>, inner: Arc<dyn ExecutorFileSystem>,
metadata_calls: Arc<BlockingMetadataCalls>, metadata_calls: Arc<BlockingMetadataCalls>,
blocked_walk_root: Option<PathUri>,
blocked_walk_gate: Semaphore,
walks_started: AtomicUsize,
walk_started: Notify,
} }
struct BlockingMetadataCalls { struct BlockingMetadataCalls {
@@ -149,7 +155,19 @@ impl ExecutorFileSystem for BlockingRepoSkillRootFileSystem {
options: WalkOptions, options: WalkOptions,
sandbox: Option<&'a FileSystemSandboxContext>, sandbox: Option<&'a FileSystemSandboxContext>,
) -> ExecutorFileSystemFuture<'a, WalkOutcome> { ) -> ExecutorFileSystemFuture<'a, WalkOutcome> {
self.inner.walk(path, options, sandbox) self.walks_started.fetch_add(/*val*/ 1, Ordering::AcqRel);
self.walk_started.notify_waiters();
if self.blocked_walk_root.as_ref() != Some(path) {
return self.inner.walk(path, options, sandbox);
}
Box::pin(async move {
self.blocked_walk_gate
.acquire()
.await
.expect("blocked walk gate should remain open")
.forget();
self.inner.walk(path, options, sandbox).await
})
} }
fn remove<'a>( fn remove<'a>(
@@ -276,6 +294,7 @@ async fn load_skills_for_test(config: &TestConfig) -> SkillLoadOutcome {
) )
.await, .await,
/*plugin_skill_snapshots*/ None, /*plugin_skill_snapshots*/ None,
Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)),
) )
.await .await
} }
@@ -463,7 +482,12 @@ async fn loads_skills_from_home_agents_dir_for_user_scope() -> anyhow::Result<()
Some(&home_folder_abs), Some(&home_folder_abs),
) )
.await; .await;
let outcome = load_skills_from_roots(roots, /*plugin_skill_snapshots*/ None).await; let outcome = load_skills_from_roots(
roots,
/*plugin_skill_snapshots*/ None,
Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)),
)
.await;
assert!( assert!(
outcome.errors.is_empty(), outcome.errors.is_empty(),
"unexpected errors: {:?}", "unexpected errors: {:?}",
@@ -552,6 +576,7 @@ async fn load_user_skills_root(root: &Path) -> SkillLoadOutcome {
plugin_root: None, plugin_root: None,
}], }],
/*plugin_skill_snapshots*/ None, /*plugin_skill_snapshots*/ None,
Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)),
) )
.await .await
} }
@@ -1038,6 +1063,7 @@ interface:
plugin_root: Some(plugin_root_abs.clone()), plugin_root: Some(plugin_root_abs.clone()),
}], }],
/*plugin_skill_snapshots*/ None, /*plugin_skill_snapshots*/ None,
Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)),
) )
.await; .await;
@@ -1099,6 +1125,7 @@ interface:
plugin_root: Some(plugin_root.abs()), plugin_root: Some(plugin_root.abs()),
}], }],
/*plugin_skill_snapshots*/ None, /*plugin_skill_snapshots*/ None,
Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)),
) )
.await; .await;
@@ -1275,6 +1302,7 @@ async fn loads_skills_via_symlinked_subdir_for_admin_scope() {
plugin_root: None, plugin_root: None,
}], }],
/*plugin_skill_snapshots*/ None, /*plugin_skill_snapshots*/ None,
Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)),
) )
.await; .await;
@@ -1361,6 +1389,7 @@ async fn system_scope_ignores_symlinked_subdir() {
plugin_root: None, plugin_root: None,
}], }],
/*plugin_skill_snapshots*/ None, /*plugin_skill_snapshots*/ None,
Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)),
) )
.await; .await;
assert!( assert!(
@@ -1399,6 +1428,7 @@ async fn respects_max_scan_depth_for_user_scope() {
plugin_root: None, plugin_root: None,
}], }],
/*plugin_skill_snapshots*/ None, /*plugin_skill_snapshots*/ None,
Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)),
) )
.await; .await;
@@ -1510,6 +1540,7 @@ async fn namespaces_plugin_skills_using_provided_namespace() {
plugin_root: Some(plugin_root.abs()), plugin_root: Some(plugin_root.abs()),
}], }],
/*plugin_skill_snapshots*/ None, /*plugin_skill_snapshots*/ None,
Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)),
) )
.await; .await;
@@ -1757,6 +1788,7 @@ async fn plugin_skill_name_length_limit_allows_max_qualified_name() {
plugin_root: Some(plugin_root.abs()), plugin_root: Some(plugin_root.abs()),
}], }],
/*plugin_skill_snapshots*/ None, /*plugin_skill_snapshots*/ None,
Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)),
) )
.await; .await;
@@ -1806,6 +1838,7 @@ async fn plugin_skill_name_length_limit_rejects_overlong_qualified_name() {
plugin_root: Some(plugin_root.abs()), plugin_root: Some(plugin_root.abs()),
}], }],
/*plugin_skill_snapshots*/ None, /*plugin_skill_snapshots*/ None,
Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)),
) )
.await; .await;
@@ -2234,6 +2267,10 @@ async fn repo_skill_root_search_limits_concurrent_probes_and_preserves_order() {
let fs: Arc<dyn ExecutorFileSystem> = Arc::new(BlockingRepoSkillRootFileSystem { let fs: Arc<dyn ExecutorFileSystem> = Arc::new(BlockingRepoSkillRootFileSystem {
inner: Arc::clone(&LOCAL_FS), inner: Arc::clone(&LOCAL_FS),
metadata_calls: Arc::clone(&metadata_calls), metadata_calls: Arc::clone(&metadata_calls),
blocked_walk_root: None,
blocked_walk_gate: Semaphore::new(/*permits*/ 0),
walks_started: AtomicUsize::new(/*v*/ 0),
walk_started: Notify::new(),
}); });
let assertions = async { let assertions = async {
@@ -2303,6 +2340,130 @@ async fn repo_skill_root_search_limits_concurrent_probes_and_preserves_order() {
); );
} }
#[tokio::test]
async fn merges_root_results_in_input_order_when_scans_finish_out_of_order() {
const ROOT_COUNT: usize = MAX_CONCURRENT_ROOT_SCANS + 1;
let temp = tempfile::tempdir().expect("tempdir");
let roots = (0..ROOT_COUNT)
.map(|index| temp.path().join(format!("root-{index}")))
.collect::<Vec<_>>();
for root in &roots {
fs::create_dir_all(root).expect("create root");
}
let first_skill = roots[0].join("broken/SKILL.md");
let second_skill = roots[1].join("broken/SKILL.md");
for (path, contents) in [
(&first_skill, "missing frontmatter"),
(&second_skill, "also missing frontmatter"),
] {
fs::create_dir_all(path.parent().expect("skill parent")).expect("create skill directory");
fs::write(path, contents).expect("write skill");
}
let blocked_walk_root = PathUri::from_abs_path(&roots[0].abs());
let file_system = Arc::new(BlockingRepoSkillRootFileSystem {
inner: Arc::clone(&LOCAL_FS),
metadata_calls: Arc::new(BlockingMetadataCalls::default()),
blocked_walk_root: Some(blocked_walk_root),
blocked_walk_gate: Semaphore::new(/*permits*/ 0),
walks_started: AtomicUsize::new(/*v*/ 0),
walk_started: Notify::new(),
});
let root_file_system: Arc<dyn ExecutorFileSystem> = file_system.clone();
let skill_roots = roots
.iter()
.enumerate()
.map(|(index, root)| SkillRoot {
path: root.abs(),
scope: if index == 0 {
SkillScope::Repo
} else {
SkillScope::User
},
file_system: Arc::clone(&root_file_system),
plugin_id: None,
plugin_namespace: Some("test".to_string()),
plugin_root: None,
})
.collect::<Vec<_>>();
let root_scan_slots = Semaphore::new(MAX_CONCURRENT_ROOT_SCANS);
let load = tokio::spawn(async move {
crate::root_loader::load_and_merge_skill_roots(
skill_roots,
/*plugin_skill_snapshots*/ None,
&root_scan_slots,
)
.await
});
tokio::time::timeout(std::time::Duration::from_secs(/*secs*/ 5), async {
loop {
let started = file_system.walk_started.notified();
if file_system.walks_started.load(Ordering::Acquire) == ROOT_COUNT {
break;
}
started.await;
}
})
.await
.expect("all skill-root walks should start despite the blocked first root");
file_system.blocked_walk_gate.add_permits(/*n*/ 1);
let outcome = load.await.expect("skill-root load should finish");
assert_eq!(outcome.skills, Vec::new());
assert_eq!(
outcome.errors,
vec![
SkillError {
path: canonicalize_path(first_skill)
.expect("canonical first skill")
.abs(),
message: "missing YAML frontmatter delimited by ---".to_string(),
},
SkillError {
path: canonicalize_path(second_skill)
.expect("canonical second skill")
.abs(),
message: "missing YAML frontmatter delimited by ---".to_string(),
},
]
);
}
#[tokio::test]
async fn skill_root_scans_wait_for_shared_capacity() {
let temp = tempfile::tempdir().expect("tempdir");
let root = temp.path().join("root");
fs::create_dir_all(&root).expect("create root");
let root_scan_slots = Semaphore::new(MAX_CONCURRENT_ROOT_SCANS);
let held_slots = root_scan_slots
.try_acquire_many(
u32::try_from(MAX_CONCURRENT_ROOT_SCANS).expect("root scan limit should fit in u32"),
)
.expect("root scan slots should be available");
let load = crate::root_loader::load_and_merge_skill_roots(
[SkillRoot {
path: root.abs(),
scope: SkillScope::Repo,
file_system: Arc::clone(&LOCAL_FS),
plugin_id: None,
plugin_namespace: Some("test".to_string()),
plugin_root: None,
}],
/*plugin_skill_snapshots*/ None,
&root_scan_slots,
);
tokio::pin!(load);
assert!(futures::poll!(load.as_mut()).is_pending());
drop(held_slots);
let outcome = load.await;
assert_eq!(outcome.skills, Vec::new());
assert_eq!(outcome.errors, Vec::new());
}
#[tokio::test] #[tokio::test]
async fn loads_skills_from_codex_dir_when_not_git_repo() { async fn loads_skills_from_codex_dir_when_not_git_repo() {
let codex_home = tempfile::tempdir().expect("tempdir"); let codex_home = tempfile::tempdir().expect("tempdir");
@@ -2368,6 +2529,7 @@ async fn deduplicates_by_path_preferring_first_root() {
}, },
], ],
/*plugin_skill_snapshots*/ None, /*plugin_skill_snapshots*/ None,
Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)),
) )
.await; .await;
+63 -41
View File
@@ -5,8 +5,11 @@ use std::sync::Arc;
use std::sync::Mutex; use std::sync::Mutex;
use codex_utils_plugins::PluginSkillRoot; use codex_utils_plugins::PluginSkillRoot;
use futures::StreamExt;
use tokio::sync::Semaphore;
use crate::SkillLoadOutcome; use crate::SkillLoadOutcome;
use crate::loader::MAX_CONCURRENT_ROOT_SCANS;
use crate::loader::SkillRoot; use crate::loader::SkillRoot;
use crate::loader::SkillRootSnapshot; use crate::loader::SkillRootSnapshot;
use crate::loader::load_skill_root; use crate::loader::load_skill_root;
@@ -40,52 +43,71 @@ impl fmt::Debug for PluginSkillSnapshots {
pub(crate) async fn load_and_merge_skill_roots<I>( pub(crate) async fn load_and_merge_skill_roots<I>(
roots: I, roots: I,
plugin_skill_snapshots: Option<&PluginSkillSnapshots>, plugin_skill_snapshots: Option<&PluginSkillSnapshots>,
root_scan_slots: &Semaphore,
) -> SkillLoadOutcome ) -> SkillLoadOutcome
where where
I: IntoIterator<Item = SkillRoot>, I: IntoIterator<Item = SkillRoot>,
{ {
let mut root_snapshots = Vec::new(); let mut indexed_root_snapshots = futures::stream::iter(roots.into_iter().enumerate())
for root in roots { .map(|(root_index, root)| async move {
let cache_key = match ( // Bound root scans across all concurrent loads sharing this pool.
root.plugin_id.clone(), let _root_scan_slot = root_scan_slots
root.plugin_namespace.clone(), .acquire()
root.plugin_root.clone(), .await
) { .unwrap_or_else(|_| unreachable!());
(Some(plugin_id), Some(plugin_namespace), Some(plugin_root)) => Some(PluginSkillRoot { let cache_key = match (
path: root.path.clone(), root.plugin_id.clone(),
plugin_id, root.plugin_namespace.clone(),
plugin_namespace, root.plugin_root.clone(),
plugin_root, ) {
}), (Some(plugin_id), Some(plugin_namespace), Some(plugin_root)) => {
_ => None, Some(PluginSkillRoot {
}; path: root.path.clone(),
let cached_snapshot = cache_key.as_ref().and_then(|cache_key| { plugin_id,
let plugin_skill_snapshots = plugin_skill_snapshots?; plugin_namespace,
plugin_skill_snapshots plugin_root,
.snapshots_by_root })
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner)
.get(cache_key)
.cloned()
});
let snapshot = match cached_snapshot {
Some(snapshot) => snapshot,
None => {
let snapshot = load_skill_root(root).await;
if let Some(plugin_skill_snapshots) = plugin_skill_snapshots
&& let Some(cache_key) = cache_key
{
plugin_skill_snapshots
.snapshots_by_root
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner)
.insert(cache_key, snapshot.clone());
} }
snapshot _ => None,
} };
}; let cached_snapshot = cache_key.as_ref().and_then(|cache_key| {
root_snapshots.push(snapshot); let plugin_skill_snapshots = plugin_skill_snapshots?;
} plugin_skill_snapshots
.snapshots_by_root
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner)
.get(cache_key)
.cloned()
});
let snapshot = match cached_snapshot {
Some(snapshot) => snapshot,
None => {
let snapshot = load_skill_root(root).await;
if let Some(plugin_skill_snapshots) = plugin_skill_snapshots
&& let Some(cache_key) = cache_key
{
plugin_skill_snapshots
.snapshots_by_root
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner)
.insert(cache_key, snapshot.clone());
}
snapshot
}
};
(root_index, snapshot)
})
// Keep each load's scan queue bounded while avoiding head-of-line blocking.
.buffer_unordered(MAX_CONCURRENT_ROOT_SCANS)
.collect::<Vec<_>>()
.await;
// Keep every scan slot productive, then restore root precedence for deterministic merging.
indexed_root_snapshots.sort_unstable_by_key(|(root_index, _)| *root_index);
let root_snapshots = indexed_root_snapshots
.into_iter()
.map(|(_, snapshot)| snapshot)
.collect();
merge_skill_root_snapshots(root_snapshots) merge_skill_root_snapshots(root_snapshots)
} }
+11 -1
View File
@@ -9,6 +9,7 @@ use codex_protocol::protocol::Product;
use codex_protocol::protocol::SkillScope; use codex_protocol::protocol::SkillScope;
use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_plugins::PluginSkillRoot; use codex_utils_plugins::PluginSkillRoot;
use tokio::sync::Semaphore;
use tracing::info; use tracing::info;
use tracing::instrument; use tracing::instrument;
use tracing::warn; use tracing::warn;
@@ -20,6 +21,7 @@ use crate::build_implicit_skill_path_indexes;
use crate::config_rules::SkillConfigRules; use crate::config_rules::SkillConfigRules;
use crate::config_rules::resolve_disabled_skill_paths; use crate::config_rules::resolve_disabled_skill_paths;
use crate::config_rules::skill_config_rules_from_stack; use crate::config_rules::skill_config_rules_from_stack;
use crate::loader::MAX_CONCURRENT_ROOT_SCANS;
use crate::loader::SkillRoot; use crate::loader::SkillRoot;
use crate::loader::load_skills_from_roots; use crate::loader::load_skills_from_roots;
use crate::loader::skill_roots; use crate::loader::skill_roots;
@@ -71,6 +73,8 @@ pub struct SkillsService {
extra_roots: RwLock<Vec<AbsolutePathBuf>>, extra_roots: RwLock<Vec<AbsolutePathBuf>>,
cache_by_cwd: RwLock<HashMap<AbsolutePathBuf, HostSkillsSnapshot>>, cache_by_cwd: RwLock<HashMap<AbsolutePathBuf, HostSkillsSnapshot>>,
cache_by_config: RwLock<HashMap<ConfigSkillsCacheKey, HostSkillsSnapshot>>, cache_by_config: RwLock<HashMap<ConfigSkillsCacheKey, HostSkillsSnapshot>>,
// Shared across cwds so root scheduling cannot multiply per-root I/O fanout.
root_scan_slots: Arc<Semaphore>,
} }
impl SkillsService { impl SkillsService {
@@ -89,6 +93,7 @@ impl SkillsService {
extra_roots: RwLock::new(Vec::new()), extra_roots: RwLock::new(Vec::new()),
cache_by_cwd: RwLock::new(HashMap::new()), cache_by_cwd: RwLock::new(HashMap::new()),
cache_by_config: RwLock::new(HashMap::new()), cache_by_config: RwLock::new(HashMap::new()),
root_scan_slots: Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)),
}; };
if !bundled_skills_enabled { if !bundled_skills_enabled {
// The loader caches bundled skills under `skills/.system`. Clearing that directory is // The loader caches bundled skills under `skills/.system`. Clearing that directory is
@@ -213,7 +218,12 @@ impl SkillsService {
roots: Vec<SkillRoot>, roots: Vec<SkillRoot>,
skill_config_rules: &SkillConfigRules, skill_config_rules: &SkillConfigRules,
) -> SkillLoadOutcome { ) -> SkillLoadOutcome {
let outcome = load_skills_from_roots(roots, input.plugin_skill_snapshots.as_ref()).await; let outcome = load_skills_from_roots(
roots,
input.plugin_skill_snapshots.as_ref(),
Arc::clone(&self.root_scan_slots),
)
.await;
let outcome = let outcome =
crate::filter_skill_load_outcome_for_product(outcome, self.restriction_product); crate::filter_skill_load_outcome_for_product(outcome, self.restriction_product);
let disabled_paths = resolve_disabled_skill_paths(&outcome.skills, skill_config_rules); let disabled_paths = resolve_disabled_skill_paths(&outcome.skills, skill_config_rules);
@@ -420,6 +420,7 @@ async fn host_loading_reuses_walk_inventory_for_symlinked_skill_pack() {
use codex_core_skills::SkillMetadata; use codex_core_skills::SkillMetadata;
use codex_core_skills::SkillPolicy; use codex_core_skills::SkillPolicy;
use codex_core_skills::loader::MAX_CONCURRENT_ROOT_SCANS;
use codex_core_skills::loader::SkillRoot; use codex_core_skills::loader::SkillRoot;
use codex_core_skills::loader::load_skills_from_roots; use codex_core_skills::loader::load_skills_from_roots;
use codex_protocol::protocol::SkillScope; use codex_protocol::protocol::SkillScope;
@@ -469,6 +470,7 @@ async fn host_loading_reuses_walk_inventory_for_symlinked_skill_pack() {
plugin_root: None, plugin_root: None,
}], }],
/*plugin_skill_snapshots*/ None, /*plugin_skill_snapshots*/ None,
Arc::new(tokio::sync::Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)),
); );
fn assert_send<T: Send>(_: &T) {} fn assert_send<T: Send>(_: &T) {}
assert_send(&future); assert_send(&future);
@@ -4,6 +4,7 @@ use std::sync::atomic::AtomicUsize;
use std::sync::atomic::Ordering; use std::sync::atomic::Ordering;
use codex_core_skills::HostSkillsSnapshot; use codex_core_skills::HostSkillsSnapshot;
use codex_core_skills::loader::MAX_CONCURRENT_ROOT_SCANS;
use codex_core_skills::loader::SkillRoot; use codex_core_skills::loader::SkillRoot;
use codex_core_skills::loader::load_skills_from_roots; use codex_core_skills::loader::load_skills_from_roots;
use codex_exec_server::CopyOptions; use codex_exec_server::CopyOptions;
@@ -211,6 +212,7 @@ async fn skill_loading_and_reads_use_the_supplied_executor_file_system() {
plugin_root: None, plugin_root: None,
}], }],
/*plugin_skill_snapshots*/ None, /*plugin_skill_snapshots*/ None,
Arc::new(tokio::sync::Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)),
) )
.await; .await;
assert_eq!(outcome.errors, Vec::new()); assert_eq!(outcome.errors, Vec::new());