mirror of
https://github.com/openai/codex.git
synced 2026-10-20 00:18:49 +00:00
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
This commit is contained in:
@@ -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<GuardianReviewError> {
|
||||
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);
|
||||
|
||||
@@ -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]
|
||||
|
||||
Reference in New Issue
Block a user