mirror of
https://github.com/openai/codex.git
synced 2026-10-22 17:58:46 +00:00
Back off failed cloud skill and plugin catalog refreshes (#55968)
## Why Failed cloud discovery previously retried before each model step. Apply a cooldown across steps and turns to avoid repeated discovery requests while a provider remains unavailable. ## What changed - Share per-thread retry backoff logic between cloud skill and plugin catalogs, starting at 30 seconds and doubling to a maximum of 5 minutes. - Scope cooldowns to authentication and MCP resource generations so account changes and reconnects permit immediate recovery. - Clear backoff after successful discovery or configuration resets, while allowing cancelled refreshes to retry immediately. ## Testing Update lifecycle and concurrency tests to cover shared cooldowns, reconnect recovery, and cancellation. Use paused time to verify exponential delays, the 5-minute cap, and backoff across turns. GitOrigin-RevId: 30dab5ceba51e8ad64c0794c1729719e99f577ce
This commit is contained in:
committed by
copyberry
parent
57232f111c
commit
3fd08b5405
@@ -132,7 +132,7 @@ async fn cloud_skills_reuse_cache_and_invalidate_on_connection_or_auth_change()
|
||||
});
|
||||
let test = builder.build_with_auto_env(&server).await?;
|
||||
let mut last_available_skill = calendar;
|
||||
// Retry turn-start failures before each model step.
|
||||
// Back off failures across steps and turns; reconnects allow immediate recovery.
|
||||
for (turn, next_reply, expected_skill, new_account, expected_lists, expected_reads) in [
|
||||
("discovered", Ok(Some(calendar)), Some(calendar), None, 1, 1),
|
||||
(
|
||||
@@ -156,7 +156,7 @@ async fn cloud_skills_reuse_cache_and_invalidate_on_connection_or_auth_change()
|
||||
Err(()),
|
||||
Some(updated_calendar),
|
||||
None,
|
||||
3,
|
||||
1,
|
||||
0,
|
||||
),
|
||||
("removed", Ok(None), None, None, 1, 0),
|
||||
@@ -174,9 +174,10 @@ async fn cloud_skills_reuse_cache_and_invalidate_on_connection_or_auth_change()
|
||||
Err(()),
|
||||
None,
|
||||
Some("account-b"),
|
||||
3,
|
||||
1,
|
||||
0,
|
||||
),
|
||||
("new-account-cooldown", Ok(Some(gmail)), None, None, 0, 0),
|
||||
(
|
||||
"new-account-discovered",
|
||||
Ok(Some(gmail)),
|
||||
@@ -190,7 +191,7 @@ async fn cloud_skills_reuse_cache_and_invalidate_on_connection_or_auth_change()
|
||||
Err(()),
|
||||
Some(gmail),
|
||||
None,
|
||||
3,
|
||||
1,
|
||||
0,
|
||||
),
|
||||
(
|
||||
@@ -207,7 +208,13 @@ async fn cloud_skills_reuse_cache_and_invalidate_on_connection_or_auth_change()
|
||||
*provider.reply.lock().expect("cloud reply lock") = next_reply;
|
||||
let previous_connection_scope = if matches!(
|
||||
turn,
|
||||
"refreshed" | "same-account-failure" | "restored" | "connection-replaced-failure"
|
||||
"refreshed"
|
||||
| "same-account-failure"
|
||||
| "removed"
|
||||
| "restored"
|
||||
| "new-account-discovered"
|
||||
| "connection-replaced-failure"
|
||||
| "replacement-connection-discovered"
|
||||
) {
|
||||
let client = resources
|
||||
.0
|
||||
|
||||
@@ -29,6 +29,7 @@ codex-tools = { workspace = true }
|
||||
codex-utils-absolute-path = { workspace = true }
|
||||
codex-utils-path-uri = { workspace = true }
|
||||
serde_json = { workspace = true }
|
||||
tokio = { workspace = true, features = ["time"] }
|
||||
|
||||
[dev-dependencies]
|
||||
pretty_assertions = { workspace = true }
|
||||
|
||||
@@ -0,0 +1,55 @@
|
||||
//! Thread-owned cloud catalog retry cooldowns scoped to auth and resource generations.
|
||||
|
||||
use codex_mcp::McpResourceClientAuthKey;
|
||||
use codex_mcp::McpResourceServerCacheKey;
|
||||
use std::time::Duration;
|
||||
use tokio::time::Instant;
|
||||
|
||||
/// Store per catalog and thread; clear on success or configuration reset.
|
||||
#[derive(Clone)]
|
||||
pub struct CatalogRefreshBackoff {
|
||||
auth: Option<McpResourceClientAuthKey>,
|
||||
resources: Option<McpResourceServerCacheKey>,
|
||||
delay: Duration,
|
||||
retry_at: Instant,
|
||||
}
|
||||
|
||||
impl std::fmt::Debug for CatalogRefreshBackoff {
|
||||
fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
||||
formatter
|
||||
.debug_struct("CatalogRefreshBackoff")
|
||||
.field("delay", &self.delay)
|
||||
.field("retry_at", &self.retry_at)
|
||||
.finish_non_exhaustive()
|
||||
}
|
||||
}
|
||||
|
||||
impl CatalogRefreshBackoff {
|
||||
pub fn is_pending(
|
||||
&self,
|
||||
auth: Option<&McpResourceClientAuthKey>,
|
||||
resources: Option<&McpResourceServerCacheKey>,
|
||||
) -> bool {
|
||||
self.auth.as_ref() == auth
|
||||
&& self.resources.as_ref() == resources
|
||||
&& Instant::now() < self.retry_at
|
||||
}
|
||||
|
||||
pub fn after_failure(
|
||||
auth: Option<McpResourceClientAuthKey>,
|
||||
resources: Option<McpResourceServerCacheKey>,
|
||||
previous: Option<&Self>,
|
||||
) -> Self {
|
||||
let delay = previous
|
||||
.filter(|previous| previous.auth == auth && previous.resources == resources)
|
||||
.map_or(Duration::from_secs(30), |previous| {
|
||||
(previous.delay * 2).min(Duration::from_secs(300))
|
||||
});
|
||||
Self {
|
||||
auth,
|
||||
resources,
|
||||
delay,
|
||||
retry_at: Instant::now() + delay,
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1,4 +1,5 @@
|
||||
mod capabilities;
|
||||
mod catalog_refresh;
|
||||
mod contributors;
|
||||
mod message_lifecycle;
|
||||
mod registry;
|
||||
@@ -22,6 +23,7 @@ pub use capabilities::NoopExtensionEventSink;
|
||||
pub use capabilities::NoopResponseItemInjector;
|
||||
pub use capabilities::ResponseItemInjectionFuture;
|
||||
pub use capabilities::ResponseItemInjector;
|
||||
pub use catalog_refresh::CatalogRefreshBackoff;
|
||||
pub use codex_context_fragments::ContextualUserFragment;
|
||||
pub use codex_mcp::McpProtocolMode;
|
||||
pub use codex_mcp::ToolInfo as McpToolInfo;
|
||||
|
||||
@@ -40,6 +40,6 @@ codex-login = { workspace = true }
|
||||
futures = { workspace = true }
|
||||
pretty_assertions = { workspace = true }
|
||||
tempfile = { workspace = true }
|
||||
tokio = { workspace = true, features = ["macros", "rt-multi-thread"] }
|
||||
tokio = { workspace = true, features = ["macros", "rt-multi-thread", "test-util"] }
|
||||
tokio-util = { workspace = true, features = ["rt"] }
|
||||
wiremock = { workspace = true }
|
||||
|
||||
@@ -6,6 +6,7 @@ use crate::plugin_contributor::PluginContributor;
|
||||
use codex_core::config::Config;
|
||||
use codex_core_plugins::PluginCatalog;
|
||||
use codex_core_plugins::PluginIdentity;
|
||||
use codex_extension_api::CatalogRefreshBackoff;
|
||||
use codex_extension_api::CatalogRefreshInput;
|
||||
use codex_extension_api::ConfigContributor;
|
||||
use codex_extension_api::ExtensionData;
|
||||
@@ -52,49 +53,60 @@ impl PluginContributor {
|
||||
mcp_resources: input.session_store.get::<McpResourceClient>(),
|
||||
snapshots: Vec::new(),
|
||||
};
|
||||
let (auth_cache_key, catalog_cleared) = {
|
||||
let mut state = state.contributor_state();
|
||||
if !state.plugins_enabled {
|
||||
return false;
|
||||
}
|
||||
let auth = query
|
||||
.mcp_resources
|
||||
.as_ref()
|
||||
.map(|client| client.auth_cache_key_for_server(CODEX_APPS_MCP_SERVER_NAME));
|
||||
if ((waited && *refresh_succeeded)
|
||||
|| input
|
||||
.turn_store
|
||||
.get::<LatestCloudPluginAuth>()
|
||||
.is_some_and(|previous| previous.0 == auth))
|
||||
&& state
|
||||
let (auth_cache_key, resource_cache_key, catalog_cleared) =
|
||||
{
|
||||
let mut state = state.contributor_state();
|
||||
if !state.plugins_enabled {
|
||||
return false;
|
||||
}
|
||||
let auth = query
|
||||
.mcp_resources
|
||||
.as_ref()
|
||||
.map(|client| client.auth_cache_key_for_server(CODEX_APPS_MCP_SERVER_NAME));
|
||||
let resource_cache_key = query
|
||||
.mcp_resources
|
||||
.as_ref()
|
||||
.and_then(|client| client.server_cache_key(CODEX_APPS_MCP_SERVER_NAME));
|
||||
if ((waited && *refresh_succeeded)
|
||||
|| input
|
||||
.turn_store
|
||||
.get::<LatestCloudPluginAuth>()
|
||||
.is_some_and(|previous| previous.0 == auth))
|
||||
&& state
|
||||
.cloud_generation
|
||||
.as_ref()
|
||||
.is_some_and(|generation| generation.auth_cache_key == auth)
|
||||
&& state.cloud_catalog().is_some()
|
||||
{
|
||||
input.turn_store.insert(LatestCloudPluginAuth(auth));
|
||||
return false;
|
||||
}
|
||||
if state.cloud_retry.as_ref().is_some_and(|retry| {
|
||||
retry.is_pending(auth.as_ref(), resource_cache_key.as_ref())
|
||||
}) {
|
||||
return false;
|
||||
}
|
||||
// Waiting callers must not reuse a failed or cancelled refresh.
|
||||
*refresh_succeeded = false;
|
||||
input.turn_store.remove::<LatestCloudPluginAuth>();
|
||||
let previous_catalog_auth = state
|
||||
.cloud_generation
|
||||
.as_ref()
|
||||
.is_some_and(|generation| generation.auth_cache_key == auth)
|
||||
&& state.cloud_catalog().is_some()
|
||||
{
|
||||
input.turn_store.insert(LatestCloudPluginAuth(auth));
|
||||
return false;
|
||||
}
|
||||
// A failed or cancelled refresh must leave waiting callers able to retry.
|
||||
*refresh_succeeded = false;
|
||||
input.turn_store.remove::<LatestCloudPluginAuth>();
|
||||
let previous_catalog_auth = state
|
||||
.cloud_generation
|
||||
.as_ref()
|
||||
.filter(|generation| generation.catalog.is_some())
|
||||
.map(|generation| generation.auth_cache_key.clone());
|
||||
let auth_cache_key =
|
||||
state.prepare_cloud_generation(query.mcp_resources.as_deref().cloned());
|
||||
let catalog_cleared =
|
||||
previous_catalog_auth.is_some_and(|previous| previous != auth_cache_key);
|
||||
(auth_cache_key, catalog_cleared)
|
||||
};
|
||||
.filter(|generation| generation.catalog.is_some())
|
||||
.map(|generation| generation.auth_cache_key.clone());
|
||||
let auth_cache_key =
|
||||
state.prepare_cloud_generation(query.mcp_resources.as_deref().cloned());
|
||||
let catalog_cleared =
|
||||
previous_catalog_auth.is_some_and(|previous| previous != auth_cache_key);
|
||||
(auth_cache_key, resource_cache_key, catalog_cleared)
|
||||
};
|
||||
let result = provider.list(query).await;
|
||||
let mut state = state.contributor_state();
|
||||
let catalog_published = match result {
|
||||
Ok(catalog) => {
|
||||
let published = state.publish_cloud_catalog(auth_cache_key.as_ref(), catalog);
|
||||
if published {
|
||||
state.cloud_retry = None;
|
||||
input
|
||||
.turn_store
|
||||
.insert(LatestCloudPluginAuth(auth_cache_key));
|
||||
@@ -105,6 +117,15 @@ impl PluginContributor {
|
||||
published
|
||||
}
|
||||
Err(_) => {
|
||||
if state.cloud_generation.as_ref().is_some_and(|generation| {
|
||||
generation.auth_cache_key == auth_cache_key && generation.is_auth_current()
|
||||
}) {
|
||||
state.cloud_retry = Some(CatalogRefreshBackoff::after_failure(
|
||||
auth_cache_key,
|
||||
resource_cache_key,
|
||||
state.cloud_retry.as_ref(),
|
||||
));
|
||||
}
|
||||
tracing::warn!("cloud plugin discovery failed");
|
||||
false
|
||||
}
|
||||
@@ -140,9 +161,13 @@ impl ConfigContributor<Config> for PluginContributor {
|
||||
let plugins_enabled = new_config.features.enabled(Feature::Plugins);
|
||||
let state = thread_store.get_or_init(PluginsThreadState::default);
|
||||
let mut state = state.contributor_state();
|
||||
if state.plugins_enabled != plugins_enabled {
|
||||
state.cloud_retry = None;
|
||||
}
|
||||
state.plugins_enabled = plugins_enabled;
|
||||
if self.providers.cloud.is_none() || !plugins_enabled {
|
||||
state.cloud_generation = None;
|
||||
state.cloud_retry = None;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -130,6 +130,7 @@ async fn disabling_plugins_clears_cloud_catalog_and_skips_discovery() -> anyhow:
|
||||
true,
|
||||
Err("discovery unavailable".to_string()),
|
||||
),
|
||||
("failed-plugins-off", false, Ok(cloud_catalog())),
|
||||
("re-enabled", true, Ok(cloud_catalog())),
|
||||
] {
|
||||
let expected_catalog = if plugins_enabled {
|
||||
@@ -245,7 +246,7 @@ async fn account_change_clears_hosted_plugins_before_waiting_for_executor() -> a
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
#[tokio::test(start_paused = true)]
|
||||
async fn cloud_refresh_serializes_waiters_and_retries() {
|
||||
let new_catalog = PluginCatalog::default();
|
||||
for (reply, cancelled) in [
|
||||
@@ -303,13 +304,19 @@ async fn cloud_refresh_serializes_waiters_and_retries() {
|
||||
provider.resume_discovery.notify_one();
|
||||
assert_eq!(turn_start.await, succeeded);
|
||||
}
|
||||
// Reuse a successful refresh; failure/cancellation releases the gate for retry.
|
||||
// Waiting turns share failure cooldowns; cancellation still allows immediate retry.
|
||||
assert_eq!(
|
||||
tokio::time::timeout(Duration::from_secs(5), prewarm)
|
||||
.await
|
||||
.expect("waiting discovery should complete after its predecessor stops"),
|
||||
!succeeded,
|
||||
cancelled,
|
||||
);
|
||||
if !succeeded && !cancelled {
|
||||
assert_eq!(state.cloud_catalog(), None);
|
||||
assert_eq!(provider.calls.load(Ordering::SeqCst), 1);
|
||||
tokio::time::advance(Duration::from_secs(30)).await;
|
||||
assert!(contributor.refresh_catalog_for_step(prewarm_input).await);
|
||||
}
|
||||
assert_eq!(state.cloud_catalog(), Some(expected_catalog.clone()));
|
||||
assert!(!contributor.refresh_catalog_for_step(prewarm_input).await);
|
||||
assert_eq!(
|
||||
@@ -325,7 +332,7 @@ async fn cloud_refresh_serializes_waiters_and_retries() {
|
||||
assert!(!contributor.refresh_catalog_for_step(prewarm_input).await);
|
||||
assert_eq!(state.cloud_catalog(), Some(expected_catalog.clone()));
|
||||
}
|
||||
assert_eq!(provider.calls.load(Ordering::SeqCst), previous_calls + 2);
|
||||
assert_eq!(provider.calls.load(Ordering::SeqCst), previous_calls + 1);
|
||||
|
||||
// Clearing an old auth scope still requires rebuilding, even on failure.
|
||||
session.insert(McpResourceClient::new(Arc::new(
|
||||
@@ -335,6 +342,7 @@ async fn cloud_refresh_serializes_waiters_and_retries() {
|
||||
assert_eq!(state.cloud_catalog(), None);
|
||||
assert!(!contributor.refresh_catalog_for_step(prewarm_input).await);
|
||||
provider.set_reply(Ok(new_catalog.clone()));
|
||||
tokio::time::advance(Duration::from_secs(30)).await;
|
||||
assert!(contributor.refresh_catalog_for_step(prewarm_input).await);
|
||||
}
|
||||
}
|
||||
@@ -660,7 +668,7 @@ async fn run_cloud_catalog_refresh_lifecycle() -> anyhow::Result<()> {
|
||||
),
|
||||
] {
|
||||
let previous_discoveries = provider.calls.load(Ordering::SeqCst);
|
||||
let retry_discovery = reply.is_err() || turn == "in-flight-auth-change";
|
||||
let retry_discovery = turn == "in-flight-auth-change";
|
||||
if let Some(account) = new_account {
|
||||
// Do not manually refresh MCP: the auth transition must pick up this new inventory.
|
||||
*apps_tools
|
||||
@@ -700,6 +708,15 @@ async fn run_cloud_catalog_refresh_lifecycle() -> anyhow::Result<()> {
|
||||
.await?;
|
||||
assert_eq!(provider.calls.load(Ordering::SeqCst), previous_discoveries);
|
||||
}
|
||||
if matches!(
|
||||
turn,
|
||||
"complete-with-warning-disables-plugin" | "new-account-partial"
|
||||
) {
|
||||
// Reconnecting permits recovery before the thread's cooldown expires.
|
||||
test.codex
|
||||
.submit(codex_protocol::protocol::Op::RefreshMcpServers)
|
||||
.await?;
|
||||
}
|
||||
provider.set_reply(reply);
|
||||
let previous_calls = recorded_apps_tool_calls(&server).await.len();
|
||||
let call_id = format!("{turn}-call");
|
||||
|
||||
@@ -5,6 +5,7 @@ use codex_core_plugins::PluginRootOwnership;
|
||||
use codex_core_plugins::catalog_from_snapshots;
|
||||
use codex_exec_server::ExecutorCapabilityDiscoveryV2Snapshot;
|
||||
use codex_exec_server::FileSystemSandboxContext;
|
||||
use codex_extension_api::CatalogRefreshBackoff;
|
||||
use codex_extension_api::SelectedPluginContribution;
|
||||
use codex_mcp::CODEX_APPS_MCP_SERVER_NAME;
|
||||
use codex_mcp::McpResourceClient;
|
||||
@@ -41,6 +42,7 @@ impl PluginsThreadState {
|
||||
pub(crate) struct PluginContributorState {
|
||||
pub(crate) plugins_enabled: bool,
|
||||
pub(crate) cloud_generation: Option<CloudPluginGeneration>,
|
||||
pub(crate) cloud_retry: Option<CatalogRefreshBackoff>,
|
||||
pub(crate) executor_catalog: Option<PluginCatalog>,
|
||||
executor_snapshots: Vec<ExecutorCapabilityDiscoveryV2Snapshot>,
|
||||
pub(crate) executor_cache: Vec<CachedSelectedRoot>,
|
||||
|
||||
@@ -908,16 +908,10 @@ async fn run_cloud_plugin_projection_scenarios() -> anyhow::Result<()> {
|
||||
initialized, installed,
|
||||
"{case}: cloud-only declarations must not start MCPs"
|
||||
);
|
||||
// Failed discovery stays in cooldown across both model steps.
|
||||
assert_eq!(
|
||||
cloud.calls.load(Ordering::SeqCst),
|
||||
if !cloud_enabled {
|
||||
0
|
||||
} else if cloud_available {
|
||||
1
|
||||
} else {
|
||||
// Failed turn-start discovery retries before each model step.
|
||||
1 + requests.len()
|
||||
},
|
||||
usize::from(cloud_enabled),
|
||||
"{case}"
|
||||
);
|
||||
let state = test
|
||||
|
||||
@@ -48,5 +48,5 @@ insta = { workspace = true }
|
||||
opentelemetry_sdk = { workspace = true }
|
||||
pretty_assertions = { workspace = true }
|
||||
tempfile = { workspace = true }
|
||||
tokio = { workspace = true, features = ["macros", "rt-multi-thread"] }
|
||||
tokio = { workspace = true, features = ["macros", "rt-multi-thread", "test-util"] }
|
||||
tracing-subscriber = { workspace = true }
|
||||
|
||||
@@ -7,6 +7,7 @@ use std::sync::Arc;
|
||||
|
||||
use codex_exec_server::ExecutorCapabilityDiscoverySnapshot;
|
||||
use codex_exec_server::ResolvedSelectedCapabilityRoot;
|
||||
use codex_extension_api::CatalogRefreshBackoff;
|
||||
use codex_mcp::McpResourceClient;
|
||||
use codex_mcp::McpResourceClientAuthKey;
|
||||
use codex_mcp::McpResourceServerCacheKey;
|
||||
@@ -33,6 +34,7 @@ pub(crate) struct SkillCatalogState {
|
||||
pub(crate) executor_v2_cache: Option<CachedExecutorV2Catalog>,
|
||||
// Shared by both discovery modes.
|
||||
pub(crate) cloud_generation: Option<Arc<CloudSkillGeneration>>,
|
||||
pub(crate) cloud_retry: Option<CatalogRefreshBackoff>,
|
||||
pub(crate) cloud_resources: CloudResourceCache,
|
||||
}
|
||||
|
||||
|
||||
@@ -19,6 +19,7 @@ use pretty_assertions::assert_eq;
|
||||
use std::sync::atomic::AtomicBool;
|
||||
use std::sync::atomic::AtomicUsize;
|
||||
use std::sync::atomic::Ordering;
|
||||
use std::time::Duration;
|
||||
use tokio::sync::Notify;
|
||||
|
||||
#[derive(Default)]
|
||||
@@ -204,7 +205,7 @@ async fn frozen_cloud_catalog_and_reads_ignore_live_mcp_changes() {
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
#[tokio::test(start_paused = true)]
|
||||
async fn step_cloud_refresh_serializes_waiters_and_retries() {
|
||||
#[derive(Default)]
|
||||
struct WarningCounter(AtomicUsize);
|
||||
@@ -243,13 +244,16 @@ async fn step_cloud_refresh_serializes_waiters_and_retries() {
|
||||
turn_store: &turn_store,
|
||||
};
|
||||
|
||||
// Retry every failure, but warn only once during the turn.
|
||||
for expected_calls in 1..=3 {
|
||||
for (index, delay) in [30, 60, 120, 240, 300, 300].into_iter().enumerate() {
|
||||
cloud.fail_list.store(true, Ordering::SeqCst);
|
||||
assert!(!contributor.refresh_catalog_for_step(refresh_input).await);
|
||||
assert_eq!(thread_state.available_cloud_catalog(), None);
|
||||
assert_eq!(cloud.lists.load(Ordering::SeqCst), expected_calls);
|
||||
assert_eq!(cloud.lists.load(Ordering::SeqCst), index + 1);
|
||||
assert_eq!(warnings.0.load(Ordering::SeqCst), 1);
|
||||
tokio::time::advance(Duration::from_secs(delay - 1)).await;
|
||||
assert!(!contributor.refresh_catalog_for_step(refresh_input).await);
|
||||
assert_eq!(cloud.lists.load(Ordering::SeqCst), index + 1);
|
||||
tokio::time::advance(Duration::from_secs(1)).await;
|
||||
}
|
||||
cloud.pause_list.store(true, Ordering::SeqCst);
|
||||
let mut cancelled = contributor.refresh_catalog_for_step(refresh_input);
|
||||
@@ -262,9 +266,9 @@ async fn step_cloud_refresh_serializes_waiters_and_retries() {
|
||||
Some(SkillCatalog::default());
|
||||
assert!(!contributor.refresh_catalog_for_step(refresh_input).await);
|
||||
assert!(!contributor.refresh_catalog_for_step(refresh_input).await);
|
||||
assert_eq!(cloud.lists.load(Ordering::SeqCst), 5);
|
||||
assert_eq!(cloud.lists.load(Ordering::SeqCst), 8);
|
||||
|
||||
// Waiting callers reuse success, but retry a failed or cancelled discovery.
|
||||
// Waiting callers reuse success and share failures' cooldowns; cancellation allows retry.
|
||||
for (fails, cancelled) in [(false, false), (true, false), (false, true)] {
|
||||
session_store.insert(SkillsSessionState {
|
||||
mcp_resources: Some(client()),
|
||||
@@ -312,6 +316,12 @@ async fn step_cloud_refresh_serializes_waiters_and_retries() {
|
||||
.await
|
||||
.expect("waiting discovery should complete after its predecessor stops")
|
||||
);
|
||||
if fails {
|
||||
assert_eq!(thread_state.available_cloud_catalog(), None);
|
||||
assert_eq!(cloud.lists.load(Ordering::SeqCst), previous_calls + 1);
|
||||
tokio::time::advance(Duration::from_secs(30)).await;
|
||||
assert!(!contributor.refresh_catalog_for_step(prewarm_input).await);
|
||||
}
|
||||
assert_eq!(
|
||||
thread_state.available_cloud_catalog(),
|
||||
Some(if fails || cancelled {
|
||||
@@ -329,22 +339,40 @@ async fn step_cloud_refresh_serializes_waiters_and_retries() {
|
||||
assert_eq!(warnings.0.load(Ordering::SeqCst), 1);
|
||||
}
|
||||
|
||||
// A fresh turn may warn again while discovery remains unavailable.
|
||||
// New turns share the thread's cooldown and failure progression.
|
||||
session_store.insert(SkillsSessionState {
|
||||
mcp_resources: Some(client()),
|
||||
extension_metrics: None,
|
||||
});
|
||||
cloud.fail_list.store(true, Ordering::SeqCst);
|
||||
let next_turn = ExtensionData::new("next turn");
|
||||
assert!(
|
||||
!contributor
|
||||
.refresh_catalog_for_step(CatalogRefreshInput {
|
||||
turn_id: "next turn",
|
||||
turn_store: &next_turn,
|
||||
..refresh_input
|
||||
})
|
||||
.await
|
||||
);
|
||||
let previous_calls = cloud.lists.load(Ordering::SeqCst);
|
||||
for turn_store in [&turn_store, &next_turn] {
|
||||
cloud.fail_list.store(true, Ordering::SeqCst);
|
||||
assert!(
|
||||
!contributor
|
||||
.refresh_catalog_for_step(CatalogRefreshInput {
|
||||
turn_store,
|
||||
..refresh_input
|
||||
})
|
||||
.await
|
||||
);
|
||||
assert_eq!(cloud.lists.load(Ordering::SeqCst), previous_calls + 1);
|
||||
}
|
||||
assert_eq!(warnings.0.load(Ordering::SeqCst), 1);
|
||||
let next_input = CatalogRefreshInput {
|
||||
turn_id: "next turn",
|
||||
turn_store: &next_turn,
|
||||
..refresh_input
|
||||
};
|
||||
tokio::time::advance(Duration::from_secs(30)).await;
|
||||
assert!(!contributor.refresh_catalog_for_step(next_input).await);
|
||||
assert_eq!(cloud.lists.load(Ordering::SeqCst), previous_calls + 2);
|
||||
tokio::time::advance(Duration::from_secs(59)).await;
|
||||
assert!(!contributor.refresh_catalog_for_step(refresh_input).await);
|
||||
assert_eq!(cloud.lists.load(Ordering::SeqCst), previous_calls + 2);
|
||||
tokio::time::advance(Duration::from_secs(1)).await;
|
||||
assert!(!contributor.refresh_catalog_for_step(next_input).await);
|
||||
assert_eq!(cloud.lists.load(Ordering::SeqCst), previous_calls + 3);
|
||||
assert_eq!(warnings.0.load(Ordering::SeqCst), 2);
|
||||
}
|
||||
|
||||
@@ -416,7 +444,7 @@ async fn late_discovery_cannot_publish_into_a_new_generation() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
#[tokio::test(start_paused = true)]
|
||||
async fn partial_cloud_catalog_without_warnings_is_refreshed_next_turn() {
|
||||
let state = state();
|
||||
let provider = Arc::new(Provider::default());
|
||||
@@ -472,6 +500,7 @@ async fn partial_cloud_catalog_without_warnings_is_refreshed_next_turn() {
|
||||
);
|
||||
assert!(state.available_cloud_catalog().is_some());
|
||||
assert!(retry_turn.get::<LatestCloudSkillsAuth>().is_none());
|
||||
tokio::time::advance(Duration::from_secs(30)).await;
|
||||
state
|
||||
.refresh_cloud_catalog(&providers, query(client), &retry_turn)
|
||||
.await
|
||||
|
||||
@@ -7,6 +7,7 @@ use std::sync::Mutex;
|
||||
use tokio::sync::Mutex as AsyncMutex;
|
||||
|
||||
use codex_exec_server::EnvironmentAccessKey;
|
||||
use codex_extension_api::CatalogRefreshBackoff;
|
||||
use codex_extension_api::ExtensionData;
|
||||
use codex_extension_api::ExtensionMetrics;
|
||||
use codex_mcp::McpResourceClient;
|
||||
@@ -97,6 +98,7 @@ impl SkillsThreadState {
|
||||
.lock()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner);
|
||||
catalogs.cloud_generation = None;
|
||||
catalogs.cloud_retry = None;
|
||||
catalogs.cloud_resources = CloudResourceCache::default();
|
||||
}
|
||||
*previous = config;
|
||||
@@ -263,7 +265,7 @@ impl SkillsThreadState {
|
||||
.and_then(|cache| cache.catalog.clone())
|
||||
}
|
||||
|
||||
/// Reuses successful discovery within a turn; retries failures next step.
|
||||
/// Reuses discovery within a turn; backs off failures.
|
||||
#[expect(
|
||||
clippy::await_holding_invalid_type,
|
||||
reason = "cloud discovery must remain serialized through fetching and catalog publication"
|
||||
@@ -304,8 +306,13 @@ impl SkillsThreadState {
|
||||
{
|
||||
return Ok(());
|
||||
}
|
||||
turn_store.remove::<LatestCloudSkillsAuth>();
|
||||
let resource_cache_key = cache.current_resource_cache_key();
|
||||
if catalogs.cloud_retry.as_ref().is_some_and(|retry| {
|
||||
retry.is_pending(cache.auth_cache_key.as_ref(), resource_cache_key.as_ref())
|
||||
}) {
|
||||
return Ok(());
|
||||
}
|
||||
turn_store.remove::<LatestCloudSkillsAuth>();
|
||||
if cache.is_auth_current()
|
||||
&& cache.catalog.as_ref().is_some_and(|catalog| {
|
||||
(waited && *refresh_succeeded)
|
||||
@@ -313,10 +320,11 @@ impl SkillsThreadState {
|
||||
})
|
||||
&& cache.resource_cache_key == resource_cache_key
|
||||
{
|
||||
catalogs.cloud_retry = None;
|
||||
turn_store.insert(LatestCloudSkillsAuth(cache.auth_cache_key.clone()));
|
||||
return Ok(());
|
||||
}
|
||||
// A failed or cancelled refresh must leave waiting callers able to retry.
|
||||
// Waiting callers must not reuse a failed or cancelled refresh.
|
||||
*refresh_succeeded = false;
|
||||
(cache, resource_cache_key)
|
||||
};
|
||||
@@ -338,7 +346,18 @@ impl SkillsThreadState {
|
||||
// Ignore superseded results, including errors.
|
||||
return Ok(());
|
||||
}
|
||||
let mut catalog = result?;
|
||||
let mut catalog = match result {
|
||||
Ok(catalog) => catalog,
|
||||
Err(error) => {
|
||||
catalogs.cloud_retry = Some(CatalogRefreshBackoff::after_failure(
|
||||
cache.auth_cache_key.clone(),
|
||||
resource_cache_key,
|
||||
catalogs.cloud_retry.as_ref(),
|
||||
));
|
||||
return Err(error);
|
||||
}
|
||||
};
|
||||
catalogs.cloud_retry = None;
|
||||
if catalog.is_partial
|
||||
&& let Some(previous) = cache
|
||||
.catalog
|
||||
|
||||
Reference in New Issue
Block a user