mirror of
https://github.com/openai/codex.git
synced 2026-06-01 19:02:59 +00:00
core: fix stale curated plugin cache refresh races (#16126)
## Why The `plugin/list` force-sync path can race app-server startup's curated plugin cache refresh. Startup was capturing the configured curated plugin IDs from the initial config snapshot. If `plugin/list` with `forceRemoteSync` removed curated plugin entries from `config.toml` while that background refresh was still in flight, the startup task could recreate cache directories for plugins that had just been uninstalled. That leaves the `plugin/list` response logically correct but the on-disk cache stale, which matches the flaky Ubuntu arm failure seen in `codex-app-server::all suite::v2::plugin_list::plugin_list_force_remote_sync_reconciles_curated_plugin_state` while validating [#16047](https://github.com/openai/codex/pull/16047). ## What - change `codex-rs/core/src/plugins/manager.rs` so startup curated-repo refresh rereads the current user `config.toml` before deciding which curated plugin cache entries to refresh - factor the configured-plugin parsing so the same logic can be reused from either the config layer stack or the persisted user config value - add a regression test that verifies curated plugin IDs are read from the latest user config state before cache refresh runs ## Testing - `cargo test -p codex-core configured_curated_plugin_ids_from_codex_home_reads_latest_user_config -- --nocapture` - `cargo test -p codex-app-server suite::v2::plugin_list::plugin_list_force_remote_sync_reconciles_curated_plugin_state -- --nocapture` - `just argument-comment-lint`
This commit is contained in:
@@ -29,6 +29,7 @@ use super::sync_openai_plugins_repo;
|
||||
use crate::AuthManager;
|
||||
use crate::SkillMetadata;
|
||||
use crate::auth::CodexAuth;
|
||||
use crate::config::CONFIG_TOML_FILE;
|
||||
use crate::config::Config;
|
||||
use crate::config::ConfigService;
|
||||
use crate::config::ConfigServiceError;
|
||||
@@ -1008,28 +1009,7 @@ impl PluginsManager {
|
||||
auth_manager: Arc<AuthManager>,
|
||||
) {
|
||||
if config.features.enabled(Feature::Plugins) {
|
||||
let mut configured_curated_plugin_ids =
|
||||
configured_plugins_from_stack(&config.config_layer_stack)
|
||||
.into_keys()
|
||||
.filter_map(|plugin_key| match PluginId::parse(&plugin_key) {
|
||||
Ok(plugin_id)
|
||||
if plugin_id.marketplace_name == OPENAI_CURATED_MARKETPLACE_NAME =>
|
||||
{
|
||||
Some(plugin_id)
|
||||
}
|
||||
Ok(_) => None,
|
||||
Err(err) => {
|
||||
warn!(
|
||||
plugin_key,
|
||||
error = %err,
|
||||
"ignoring invalid configured plugin key during curated sync setup"
|
||||
);
|
||||
None
|
||||
}
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
configured_curated_plugin_ids.sort_unstable_by_key(PluginId::as_key);
|
||||
self.start_curated_repo_sync(configured_curated_plugin_ids);
|
||||
self.start_curated_repo_sync();
|
||||
start_startup_remote_plugin_sync_once(
|
||||
Arc::clone(self),
|
||||
self.codex_home.clone(),
|
||||
@@ -1054,7 +1034,7 @@ impl PluginsManager {
|
||||
}
|
||||
}
|
||||
|
||||
fn start_curated_repo_sync(self: &Arc<Self>, configured_curated_plugin_ids: Vec<PluginId>) {
|
||||
fn start_curated_repo_sync(self: &Arc<Self>) {
|
||||
if CURATED_REPO_SYNC_STARTED.swap(true, Ordering::SeqCst) {
|
||||
return;
|
||||
}
|
||||
@@ -1065,6 +1045,8 @@ impl PluginsManager {
|
||||
.spawn(
|
||||
move || match sync_openai_plugins_repo(codex_home.as_path()) {
|
||||
Ok(curated_plugin_version) => {
|
||||
let configured_curated_plugin_ids =
|
||||
configured_curated_plugin_ids_from_codex_home(codex_home.as_path());
|
||||
match refresh_curated_plugin_cache(
|
||||
codex_home.as_path(),
|
||||
&curated_plugin_version,
|
||||
@@ -1333,7 +1315,13 @@ fn configured_plugins_from_stack(
|
||||
let Some(user_layer) = config_layer_stack.get_user_layer() else {
|
||||
return HashMap::new();
|
||||
};
|
||||
let Some(plugins_value) = user_layer.config.get("plugins") else {
|
||||
configured_plugins_from_user_config_value(&user_layer.config)
|
||||
}
|
||||
|
||||
fn configured_plugins_from_user_config_value(
|
||||
user_config: &toml::Value,
|
||||
) -> HashMap<String, PluginConfig> {
|
||||
let Some(plugins_value) = user_config.get("plugins") else {
|
||||
return HashMap::new();
|
||||
};
|
||||
match plugins_value.clone().try_into() {
|
||||
@@ -1345,6 +1333,60 @@ fn configured_plugins_from_stack(
|
||||
}
|
||||
}
|
||||
|
||||
fn configured_curated_plugin_ids(
|
||||
configured_plugins: HashMap<String, PluginConfig>,
|
||||
) -> Vec<PluginId> {
|
||||
let mut configured_curated_plugin_ids = configured_plugins
|
||||
.into_keys()
|
||||
.filter_map(|plugin_key| match PluginId::parse(&plugin_key) {
|
||||
Ok(plugin_id) if plugin_id.marketplace_name == OPENAI_CURATED_MARKETPLACE_NAME => {
|
||||
Some(plugin_id)
|
||||
}
|
||||
Ok(_) => None,
|
||||
Err(err) => {
|
||||
warn!(
|
||||
plugin_key,
|
||||
error = %err,
|
||||
"ignoring invalid configured plugin key during curated sync setup"
|
||||
);
|
||||
None
|
||||
}
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
configured_curated_plugin_ids.sort_unstable_by_key(PluginId::as_key);
|
||||
configured_curated_plugin_ids
|
||||
}
|
||||
|
||||
fn configured_curated_plugin_ids_from_codex_home(codex_home: &Path) -> Vec<PluginId> {
|
||||
let config_path = codex_home.join(CONFIG_TOML_FILE);
|
||||
let user_config = match fs::read_to_string(&config_path) {
|
||||
Ok(user_config) => user_config,
|
||||
Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Vec::new(),
|
||||
Err(err) => {
|
||||
warn!(
|
||||
path = %config_path.display(),
|
||||
error = %err,
|
||||
"failed to read user config while refreshing curated plugin cache"
|
||||
);
|
||||
return Vec::new();
|
||||
}
|
||||
};
|
||||
|
||||
let user_config = match toml::from_str::<toml::Value>(&user_config) {
|
||||
Ok(user_config) => user_config,
|
||||
Err(err) => {
|
||||
warn!(
|
||||
path = %config_path.display(),
|
||||
error = %err,
|
||||
"failed to parse user config while refreshing curated plugin cache"
|
||||
);
|
||||
return Vec::new();
|
||||
}
|
||||
};
|
||||
|
||||
configured_curated_plugin_ids(configured_plugins_from_user_config_value(&user_config))
|
||||
}
|
||||
|
||||
fn load_plugin(
|
||||
config_name: String,
|
||||
plugin: &PluginConfig,
|
||||
|
||||
@@ -2207,6 +2207,43 @@ fn refresh_curated_plugin_cache_reinstalls_missing_configured_plugin_with_curren
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn configured_curated_plugin_ids_from_codex_home_reads_latest_user_config() {
|
||||
let tmp = tempfile::tempdir().unwrap();
|
||||
write_file(
|
||||
&tmp.path().join(CONFIG_TOML_FILE),
|
||||
r#"[features]
|
||||
plugins = true
|
||||
|
||||
[plugins."slack@openai-curated"]
|
||||
enabled = true
|
||||
|
||||
[plugins."sample@debug"]
|
||||
enabled = true
|
||||
"#,
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
configured_curated_plugin_ids_from_codex_home(tmp.path())
|
||||
.into_iter()
|
||||
.map(|plugin_id| plugin_id.as_key())
|
||||
.collect::<Vec<_>>(),
|
||||
vec!["slack@openai-curated".to_string()]
|
||||
);
|
||||
|
||||
write_file(
|
||||
&tmp.path().join(CONFIG_TOML_FILE),
|
||||
r#"[features]
|
||||
plugins = true
|
||||
"#,
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
configured_curated_plugin_ids_from_codex_home(tmp.path()),
|
||||
Vec::<PluginId>::new()
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn refresh_curated_plugin_cache_returns_false_when_configured_plugins_are_current() {
|
||||
let tmp = tempfile::tempdir().unwrap();
|
||||
|
||||
Reference in New Issue
Block a user