From 835cd7552d6a089bbdfeb21b001cb76c503249c7 Mon Sep 17 00:00:00 2001 From: jif Date: Wed, 14 Oct 2026 13:48:39 +0000 Subject: [PATCH] Skip service backoff for stale Guardian authorization retries (#53987) ## Why Stale authorization requires refreshing local evidence, so service retry backoff unnecessarily delays the next review attempt. ## What changed Yield cooperatively before retrying `StaleAuthorization`, then check cancellation and the shared deadline. Keep retries within the existing attempt budget. ## Testing Extend paused-time tests to cover recovery with fresh evidence and exhaustion of the shared attempt budget, asserting that retries incur no backoff and return the final attempt's evidence. GitOrigin-RevId: fda7ddd040a761d08883740bb5e7aaecacfa6091 --- codex-rs/ext/guardian-reviewer/src/retry.rs | 16 ++++++ .../ext/guardian-reviewer/src/retry_tests.rs | 53 ++++++++++++++----- 2 files changed, 56 insertions(+), 13 deletions(-) diff --git a/codex-rs/ext/guardian-reviewer/src/retry.rs b/codex-rs/ext/guardian-reviewer/src/retry.rs index dd98638fe5..0e2df7e965 100644 --- a/codex-rs/ext/guardian-reviewer/src/retry.rs +++ b/codex-rs/ext/guardian-reviewer/src/retry.rs @@ -1,5 +1,6 @@ //! Applies one review's attempt budget, deadline and cancellation to host-owned sessions. //! Resubmissions are independent; this layer keeps no state between approval requests. +//! Changed local evidence retries cooperatively without service backoff, under the same limits. use crate::GuardianReviewError; use crate::GuardianReviewOutcome; @@ -80,6 +81,21 @@ async fn wait_before_guardian_retry( deadline: Instant, external_cancel: Option<&CancellationToken>, ) -> Option { + if matches!( + outcome, + GuardianReviewOutcome::Error(GuardianReviewError::StaleAuthorization) + ) { + // This is a local evidence refresh, not a failed service request. Yield so + // cancellation and queued context updates can run before capturing it again. + tokio::task::yield_now().await; + return if external_cancel.is_some_and(CancellationToken::is_cancelled) { + Some(GuardianReviewError::Cancelled) + } else if Instant::now() >= deadline { + Some(GuardianReviewError::Timeout) + } else { + None + }; + } let exponential_delay = 200.0 * 2.0_f64.powi(attempt_count.saturating_sub(1) as i32); let jitter = rand::rng().random_range(0.9..1.1); let retry_delay = Duration::from_millis((exponential_delay * jitter) as u64); diff --git a/codex-rs/ext/guardian-reviewer/src/retry_tests.rs b/codex-rs/ext/guardian-reviewer/src/retry_tests.rs index 3ee4d74922..e868debc65 100644 --- a/codex-rs/ext/guardian-reviewer/src/retry_tests.rs +++ b/codex-rs/ext/guardian-reviewer/src/retry_tests.rs @@ -163,32 +163,59 @@ async fn guardian_review_retry_wait_honors_deadline() { assert!(matches!(error, Some(GuardianReviewError::Timeout))); } -#[tokio::test] -async fn stale_authorization_uses_the_shared_attempt_budget() { +#[test_case::test_case(2; "recovers with fresh evidence")] +#[test_case::test_case(4; "exhausts the shared budget")] +#[tokio::test(start_paused = true)] +async fn stale_authorization_uses_the_shared_attempt_budget(success_on_attempt: i64) { let mut attempts = 0; - let (outcome, analytics, _) = run_with_retry( + let started_at = Instant::now(); + let deadline = started_at + Duration::from_secs(/*secs*/ 90); + let (outcome, analytics, evidence) = run_with_retry( GuardianReviewSessionLimits { max_attempts: 3, - deadline: Instant::now() + Duration::from_secs(/*secs*/ 90), + deadline, }, /*external_cancel*/ None, - |_deadline| { + |attempt_deadline| { + assert_eq!(attempt_deadline, deadline); attempts += 1; - async { + let attempt = attempts; + async move { ( - GuardianReviewOutcome::Error(GuardianReviewError::StaleAuthorization), + if attempt == success_on_attempt { + GuardianReviewOutcome::CachedApproval + } else { + GuardianReviewOutcome::Error(GuardianReviewError::StaleAuthorization) + }, GuardianReviewAnalyticsResult::without_session(), - None::<()>, + Some(attempt), ) } }, ) .await; - assert!(matches!( - outcome, - GuardianReviewOutcome::Error(GuardianReviewError::StaleAuthorization) - )); - assert_eq!((attempts, analytics.attempt_count), (3, 3)); + assert_eq!( + started_at.elapsed(), + Duration::ZERO, + "stale authorization retries should not wait for service backoff" + ); + if success_on_attempt <= 3 { + assert!(matches!(outcome, GuardianReviewOutcome::CachedApproval)); + } else { + assert!(matches!( + outcome, + GuardianReviewOutcome::Error(GuardianReviewError::StaleAuthorization) + )); + } + let expected_attempts = success_on_attempt.min(3); + assert_eq!( + (attempts, analytics.attempt_count, evidence), + ( + expected_attempts, + expected_attempts, + Some(expected_attempts) + ) + ); } #[tokio::test]