openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-11-07 20:30:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

Harden remote installed plugin cache reconciliation (#40015)

Browse Source 
## What changed

- Scope remote installed-plugin and loaded-plugin snapshots to the active account, and discard in-flight loads when the account changes.
- Serialize bundle reconciliation with direct installs and uninstalls, and use cache generations to prevent stale refreshes from overwriting newer state.
- Validate the complete installed-plugin snapshot before downloading bundles or removing stale cache entries, while retaining valid installed metadata when materialization fails.

## Testing

- Cover account changes during plugin loading, incomplete snapshots, refresh/reconciliation races, and reconciliation cancellation recovery.

GitOrigin-RevId: 694c25577b420b93dacc6c65e3c0ecb7b8cc64c3
This commit is contained in:
Matthew Zeng
2026-08-21 21:28:16 +00:00
committed by copyberry
parent 9949c9eafa
commit e6a3877e95
8 changed files with 986 additions and 161 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core-plugins/src/remote/remote_installed_plugin_sync.rs
+150 -57
View File
@@ -4,6 +4,7 @@ use super::REMOTE_WORKSPACE_MARKETPLACE_NAME;
use super::REMOTE_WORKSPACE_SHARED_WITH_ME_MARKETPLACE_NAME;
use super::REMOTE_WORKSPACE_SHARED_WITH_ME_PRIVATE_MARKETPLACE_NAME;
use super::REMOTE_WORKSPACE_SHARED_WITH_ME_UNLISTED_MARKETPLACE_NAME;
use super::RemoteInstalledPlugin;
use super::RemoteInstalledPluginScope;
use super::RemotePluginCatalogError;
use super::RemotePluginScope;
@@ -11,7 +12,6 @@ use super::RemotePluginServiceConfig;
use super::RemotePluginShareDiscoverability;
use super::ensure_chatgpt_auth;
use super::fetch_installed_plugins;
use super::remote_plugin_canonical_marketplace_name;
use crate::store::PLUGINS_CACHE_DIR;
use crate::store::PluginStore;
use crate::store::PluginStoreError;
@@ -51,6 +51,13 @@ pub struct RemoteInstalledPluginBundleSyncOutcome {
pub materialized_remote_plugins: Vec<RemotePluginMaterialization>,
pub removed_cache_plugin_ids: Vec<String>,
pub failed_remote_plugin_ids: Vec<String>,
/// Failures that leave an otherwise valid installed plugin unavailable locally.
pub failed_materialization_remote_plugin_ids: Vec<String>,
}
pub(crate) struct RemoteInstalledPluginBundleSyncResult {
pub(crate) outcome: RemoteInstalledPluginBundleSyncOutcome,
pub(crate) installed_plugins: Vec<RemoteInstalledPlugin>,
}
impl RemoteInstalledPluginBundleSyncOutcome {
@@ -66,6 +73,12 @@ pub enum RemoteInstalledPluginBundleSyncError {
#[error("{0}")]
Store(#[from] PluginStoreError),
#[error("timed out waiting for another remote plugin cache mutation; retry")]
LockTimeout,
#[error("remote plugin state changed during reconciliation; retry reconciliation")]
Superseded,
}
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
@@ -100,15 +113,44 @@ pub async fn sync_remote_installed_plugin_bundles_once(
config: &RemotePluginServiceConfig,
auth: Option<&CodexAuth>,
) -> Result<RemoteInstalledPluginBundleSyncOutcome, RemoteInstalledPluginBundleSyncError> {
let result =
sync_remote_installed_plugin_bundles_once_with_snapshot(codex_home, config, auth).await?;
Ok(result.outcome)
}
pub(crate) async fn sync_remote_installed_plugin_bundles_once_with_snapshot(
codex_home: PathBuf,
config: &RemotePluginServiceConfig,
auth: Option<&CodexAuth>,
) -> Result<RemoteInstalledPluginBundleSyncResult, RemoteInstalledPluginBundleSyncError> {
let auth = ensure_chatgpt_auth(auth)?;
let authenticated_account_id = auth.get_account_id();
let installed_plugins = fetch_installed_plugins(
let fetched_installed_plugins = fetch_installed_plugins(
config,
auth,
RemoteInstalledPluginScope::All,
/*include_download_urls*/ true,
)
.await?;
// `/installed` is an authoritative full snapshot. If any row cannot be
// canonicalized to a local cache key, omitting it makes an installed plugin
// indistinguishable from an uninstall, so reject the pass before downloads,
// publication, or stale cleanup.
let mut validated_installed_plugins = Vec::with_capacity(fetched_installed_plugins.len());
for installed_plugin in fetched_installed_plugins {
let cached_plugin = super::remote_installed_plugin_to_cache_entry(&installed_plugin)?;
let plugin_id = PluginId::new(
installed_plugin.plugin.name.clone(),
cached_plugin.marketplace_name.clone(),
)
.map_err(|err| {
RemotePluginCatalogError::UnexpectedResponse(format!(
"remote installed plugin `{}` has an invalid local cache id: {err}",
installed_plugin.plugin.id
))
})?;
validated_installed_plugins.push((installed_plugin, cached_plugin, plugin_id));
}
let store = PluginStore::try_new(codex_home.clone())?;
let mut installed_plugin_names_by_marketplace =
BTreeMap::<String, BTreeSet<String>>::from_iter([
@@ -136,37 +178,27 @@ pub async fn sync_remote_installed_plugin_bundles_once(
]);
let mut materialized_remote_plugins = BTreeMap::new();
let mut failed_remote_plugin_ids = BTreeSet::new();
let mut failed_materialization_remote_plugin_ids = BTreeSet::new();
let mut installed_plugins = Vec::new();
for installed_plugin in installed_plugins {
for (installed_plugin, cached_plugin, plugin_id) in validated_installed_plugins {
let marketplace_name = cached_plugin.marketplace_name.clone();
let plugin = installed_plugin.plugin;
let scope = plugin.scope;
let discoverability = plugin.discoverability;
let marketplace_name = remote_plugin_canonical_marketplace_name(&plugin)?.to_string();
installed_plugin_names_by_marketplace
.entry(marketplace_name.clone())
.or_default()
.insert(plugin.name.clone());
let plugin_id = match PluginId::new(plugin.name.clone(), marketplace_name.clone()) {
Ok(plugin_id) => plugin_id,
Err(err) => {
warn!(
remote_plugin_id = %plugin.id,
plugin = %plugin.name,
marketplace = %marketplace_name,
error = %err,
"skipping remote installed plugin with invalid local cache id"
);
failed_remote_plugin_ids.insert(plugin.id);
continue;
}
};
let release_version = plugin
.release
.version
.as_deref()
.map(str::trim)
.filter(|version| !version.is_empty());
if store.active_plugin_version(&plugin_id).as_deref() == release_version {
if let Some(release_version) = release_version
&& store.active_plugin_version(&plugin_id).as_deref() == Some(release_version)
{
if let Err(err) = store.write_remote_plugin_id(&plugin_id, &plugin.id) {
warn!(
remote_plugin_id = %plugin.id,
@@ -177,6 +209,7 @@ pub async fn sync_remote_installed_plugin_bundles_once(
);
failed_remote_plugin_ids.insert(plugin.id);
}
installed_plugins.push(cached_plugin);
continue;
}
@@ -197,7 +230,9 @@ pub async fn sync_remote_installed_plugin_bundles_once(
error = %err,
"skipping remote installed plugin bundle download"
);
failed_remote_plugin_ids.insert(plugin.id);
failed_remote_plugin_ids.insert(plugin.id.clone());
failed_materialization_remote_plugin_ids.insert(plugin.id);
installed_plugins.push(cached_plugin);
continue;
}
};
@@ -229,34 +264,51 @@ pub async fn sync_remote_installed_plugin_bundles_once(
error = %err,
"failed to download remote installed plugin bundle"
);
failed_remote_plugin_ids.insert(plugin.id);
failed_remote_plugin_ids.insert(plugin.id.clone());
failed_materialization_remote_plugin_ids.insert(plugin.id);
}
}
installed_plugins.push(cached_plugin);
}
installed_plugins.sort_by(|left, right| {
left.marketplace_name
.cmp(&right.marketplace_name)
.then_with(|| left.id.cmp(&right.id))
});
let stale_cache_cleanup = tokio::task::spawn_blocking(move || {
remove_stale_remote_plugin_caches(
let mut removed_cache_plugin_ids = Vec::new();
let cleanup_error = remove_stale_remote_plugin_caches(
codex_home.as_path(),
&installed_plugin_names_by_marketplace,
&mut removed_cache_plugin_ids,
)
.err();
removed_cache_plugin_ids.sort();
(removed_cache_plugin_ids, cleanup_error)
})
.await;
let removed_cache_plugin_ids = match stale_cache_cleanup {
Ok(Ok(removed_cache_plugin_ids)) => removed_cache_plugin_ids,
Ok(Err(err)) => {
warn!(error = %err, "failed to remove stale remote plugin cache entries");
Vec::new()
}
let (removed_cache_plugin_ids, cleanup_error) = match stale_cache_cleanup {
Ok(result) => result,
Err(err) => {
warn!(error = %err, "failed to join stale remote plugin cache cleanup task");
Vec::new()
(Vec::new(), Some(err.to_string()))
}
};
if let Some(err) = cleanup_error {
warn!(error = %err, "failed to remove stale remote plugin cache entries");
}
Ok(RemoteInstalledPluginBundleSyncOutcome {
materialized_remote_plugins: materialized_remote_plugins.into_values().collect(),
removed_cache_plugin_ids,
failed_remote_plugin_ids: failed_remote_plugin_ids.into_iter().collect(),
Ok(RemoteInstalledPluginBundleSyncResult {
outcome: RemoteInstalledPluginBundleSyncOutcome {
materialized_remote_plugins: materialized_remote_plugins.into_values().collect(),
removed_cache_plugin_ids,
failed_remote_plugin_ids: failed_remote_plugin_ids.into_iter().collect(),
failed_materialization_remote_plugin_ids: failed_materialization_remote_plugin_ids
.into_iter()
.collect(),
},
installed_plugins,
})
}
@@ -301,8 +353,8 @@ impl Drop for RemotePluginCacheMutationGuard {
fn remove_stale_remote_plugin_caches(
codex_home: &Path,
installed_plugin_names_by_marketplace: &BTreeMap<String, BTreeSet<String>>,
) -> Result<Vec<String>, String> {
let mut removed_cache_plugin_ids = Vec::new();
removed_cache_plugin_ids: &mut Vec<String>,
) -> Result<(), String> {
for marketplace_name in [
REMOTE_GLOBAL_MARKETPLACE_NAME,
REMOTE_CREATED_BY_ME_MARKETPLACE_NAME,
@@ -369,8 +421,7 @@ fn remove_stale_remote_plugin_caches(
}
}
removed_cache_plugin_ids.sort();
Ok(removed_cache_plugin_ids)
Ok(())
}
fn remote_plugin_cache_root(codex_home: &Path) -> PathBuf {
@@ -410,7 +461,7 @@ mod tests {
use wiremock::matchers::query_param_is_missing;
#[tokio::test]
async fn sync_same_version_backfills_metadata_without_materialization() {
async fn sync_same_version_backfills_metadata_and_missing_version_requires_materialization() {
let server = MockServer::start().await;
let codex_home = tempfile::tempdir().expect("create codex home");
let cached_manifest = codex_home
@@ -432,21 +483,37 @@ mod tests {
.and(query_param("limit", "200"))
.and(query_param("includeDownloadUrls", "true"))
.respond_with(ResponseTemplate::new(200).set_body_json(json!({
"plugins": [{
"id": remote_plugin_id,
"name": "linear",
"scope": "GLOBAL",
"installation_policy": "AVAILABLE",
"authentication_policy": "ON_USE",
"status": "ENABLED",
"release": {
"version": "1.2.3",
"display_name": "Linear",
"description": "Track work",
"interface": {},
"plugins": [
{
"id": remote_plugin_id,
"name": "linear",
"scope": "GLOBAL",
"installation_policy": "AVAILABLE",
"authentication_policy": "ON_USE",
"status": "ENABLED",
"release": {
"version": "1.2.3",
"display_name": "Linear",
"description": "Track work",
"interface": {},
},
"enabled": true,
},
"enabled": true,
}],
{
"id": "plugins~Plugin_missing_version",
"name": "missing-version",
"scope": "GLOBAL",
"installation_policy": "AVAILABLE",
"authentication_policy": "ON_USE",
"status": "ENABLED",
"release": {
"display_name": "Missing version",
"description": "Needs materialization",
"interface": {},
},
"enabled": true,
},
],
"pagination": {"next_page_token": null},
})))
.expect(1)
@@ -466,7 +533,16 @@ mod tests {
.await
.expect("sync current remote plugin bundle");
assert_eq!(outcome, RemoteInstalledPluginBundleSyncOutcome::default());
assert_eq!(
outcome,
RemoteInstalledPluginBundleSyncOutcome {
failed_remote_plugin_ids: vec!["plugins~Plugin_missing_version".to_string()],
failed_materialization_remote_plugin_ids: vec![
"plugins~Plugin_missing_version".to_string()
],
..RemoteInstalledPluginBundleSyncOutcome::default()
}
);
let plugin_id = PluginId::new(
"linear".to_string(),
REMOTE_GLOBAL_MARKETPLACE_NAME.to_string(),
@@ -486,6 +562,14 @@ mod tests {
"remote_plugin_id": remote_plugin_id,
})
);
assert!(
!codex_home
.path()
.join(PLUGINS_CACHE_DIR)
.join(REMOTE_GLOBAL_MARKETPLACE_NAME)
.join("missing-version")
.exists()
);
}
#[tokio::test]
@@ -611,6 +695,7 @@ mod tests {
materialized_remote_plugins: Vec::new(),
removed_cache_plugin_ids,
failed_remote_plugin_ids: Vec::new(),
failed_materialization_remote_plugin_ids: Vec::new(),
}
);
assert_eq!(
@@ -701,27 +786,33 @@ mod tests {
REMOTE_GLOBAL_MARKETPLACE_NAME,
"linear",
);
let removed = remove_stale_remote_plugin_caches(
let mut removed = Vec::new();
remove_stale_remote_plugin_caches(
codex_home.path(),
&installed_plugin_names_by_marketplace,
&mut removed,
)
.expect("cleanup while install is guarded");
assert_eq!(removed, Vec::<String>::new());
assert!(cached_manifest.is_file());
drop(guard);
let removed = remove_stale_remote_plugin_caches(
let mut removed = Vec::new();
remove_stale_remote_plugin_caches(
codex_home.path(),
&installed_plugin_names_by_marketplace,
&mut removed,
)
.expect("cleanup while second install guard is still active");
assert_eq!(removed, Vec::<String>::new());
assert!(cached_manifest.is_file());
drop(second_guard);
let removed = remove_stale_remote_plugin_caches(
let mut removed = Vec::new();
remove_stale_remote_plugin_caches(
codex_home.path(),
&installed_plugin_names_by_marketplace,
&mut removed,
)
.expect("cleanup after install guard is dropped");
assert_eq!(removed, vec!["linear@openai-curated-remote".to_string()]);
@@ -799,9 +890,11 @@ mod tests {
),
]);
let removed = remove_stale_remote_plugin_caches(
let mut removed = Vec::new();
remove_stale_remote_plugin_caches(
codex_home.path(),
&installed_plugin_names_by_marketplace,
&mut removed,
)
.expect("cleanup private shared-with-me cache");