From 2bb454bc55de6e450c72335cc3ae68e7419ec6e1 Mon Sep 17 00:00:00 2001 From: Jonathan Liebig Date: Sun, 12 Jul 2026 21:31:45 +0200 Subject: [PATCH] fix(login): require weekly reset drift before auto-start --- .../codex_plus_plus/weekly_window_state.rs | 148 +++++++----------- .../weekly_window_state_tests.rs | 146 ++++++++++++----- 2 files changed, 161 insertions(+), 133 deletions(-) diff --git a/codex-rs/login/src/codex_plus_plus/weekly_window_state.rs b/codex-rs/login/src/codex_plus_plus/weekly_window_state.rs index 8fbe5f1112bc..c60d3e161042 100644 --- a/codex-rs/login/src/codex_plus_plus/weekly_window_state.rs +++ b/codex-rs/login/src/codex_plus_plus/weekly_window_state.rs @@ -14,8 +14,8 @@ const STATE_FILE: &str = "weekly-window-state.json"; const LOCK_FILE: &str = "weekly-window.lock"; const SCAN_LOCK_FILE: &str = "weekly-window-scan.lock"; const MAX_STATE_BYTES: u64 = 4 * 1024; -const SUPPRESSION_SECONDS: i64 = 7 * 24 * 60 * 60; const MAX_FAILURE_COUNT: u8 = 8; +const STATE_VERSION: u8 = 2; #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum WeeklyWindowUsage { Missing, @@ -108,7 +108,7 @@ struct State { impl State { fn new() -> Self { Self { - version: 1, + version: STATE_VERSION, ..Self::default() } } @@ -143,8 +143,14 @@ impl AccountStore { let state_path = account_home.join(STATE_FILE); let mut state = match read_state(&state_path)? { StateRead::Ready(state) => state, + StateRead::Legacy => { + let mut state = State::new(); + observe_after_completion(&mut state, usage); + write_state(&state_path, &state)?; + return Ok(WeeklyWindowAttemptDecision::NotDue); + } StateRead::Corrupt => { - let state = quarantine_state(usage, now); + let state = quarantine_state(usage); write_state(&state_path, &state)?; return Ok(WeeklyWindowAttemptDecision::StateUnavailable); } @@ -155,6 +161,7 @@ impl AccountStore { let dispatched_at = state.last_attempt_at; close_attempt(&mut state, now, Some(WeeklyWindowError::Ambiguous)); state.last_attempt_at = dispatched_at; + observe_after_completion(&mut state, usage); write_state(&state_path, &state)?; return Ok(WeeklyWindowAttemptDecision::NotDue); } @@ -186,6 +193,7 @@ impl AccountStore { retry_not_before: state.retry_not_before, recovery_not_before: state.recovery_not_before, }, + StateRead::Legacy => WeeklyWindowStatus::default(), StateRead::Corrupt | StateRead::Incompatible => WeeklyWindowStatus { last_error: Some(WeeklyWindowError::StateQuarantined), ..WeeklyWindowStatus::default() @@ -237,102 +245,57 @@ fn due_identity(state: &mut State, usage: WeeklyWindowUsage, now: i64) -> Option close_attempt(state, now, /*error*/ None); } state.last_observed_active = true; - state.last_observed_reset_at = resets_at.or(state.last_observed_reset_at); return None; } + let resets_at = resets_at?; - if let Some(until) = state.recovery_not_before { - if now < until && !state.last_observed_active { - baseline(state, resets_at); + if state.attempt_status == Some(AttemptStatus::Retryable) { + let Some(AttemptIdentity::ResetAt(attempt_reset_at)) = state.attempt_identity else { + close_attempt(state, now, /*error*/ None); + return None; + }; + let latest_reset_at = state.last_observed_reset_at.unwrap_or(attempt_reset_at); + if resets_at != attempt_reset_at && resets_at < latest_reset_at { return None; } - state.recovery_not_before = None; - state.last_error = None; - } - - let identity = if let Some(resets_at) = resets_at { - let identity = AttemptIdentity::ResetAt(resets_at); - if state.attempt_status == Some(AttemptStatus::Retryable) - && state.attempt_identity == Some(identity) - { - if state - .retry_not_before - .is_some_and(|retry_at| now < retry_at) - { - return None; - } - identity - } else { - if state.attempt_status == Some(AttemptStatus::Closed) - && state.attempt_identity == Some(identity) - { - return None; - } - let moved_forward = state + state.last_observed_reset_at = Some( + state .last_observed_reset_at - .is_some_and(|previous| resets_at > previous); - if resets_at > now && !moved_forward { - baseline(state, Some(resets_at)); - return None; - } - identity - } - } else if state.attempt_status == Some(AttemptStatus::Retryable) { + .unwrap_or(resets_at) + .max(resets_at), + ); if state .retry_not_before .is_some_and(|retry_at| now < retry_at) { return None; } - state.attempt_identity? - } else { - if state.attempt_status == Some(AttemptStatus::Closed) - && matches!(state.attempt_identity, Some(AttemptIdentity::ResetAt(_))) - && inside_suppression(state, now) - && !state.last_observed_active - { - state.last_observed_active = false; - return None; - } - let generation = if state.last_observed_active - || state.attempt_status == Some(AttemptStatus::Closed) - && !inside_suppression(state, now) - { - state.missing_reset_generation.saturating_add(1).max(1) - } else if state.missing_reset_generation == 0 { - 1 - } else if state.attempt_status.is_none() { - state.missing_reset_generation - } else { - return None; - }; - state.missing_reset_generation = generation; - AttemptIdentity::MissingReset(generation) - }; - - if state.attempt_status == Some(AttemptStatus::Closed) - && state.attempt_identity != Some(identity) - && !state.last_observed_active - && inside_suppression(state, now) - { - baseline(state, resets_at); - return None; + return Some(AttemptIdentity::ResetAt(attempt_reset_at)); } - state.last_observed_active = false; - Some(identity) -} -fn baseline(state: &mut State, resets_at: Option) { - state.last_observed_reset_at = resets_at.or(state.last_observed_reset_at); + let previous = state.last_observed_reset_at; + let was_active = state.last_observed_active; state.last_observed_active = false; - if resets_at.is_none() && state.missing_reset_generation == 0 { - state.missing_reset_generation = 1; + state.last_observed_reset_at = Some(previous.map_or(resets_at, |value| value.max(resets_at))); + if previous.is_none() || was_active { + return None; } + + let identity = AttemptIdentity::ResetAt(resets_at); + (resets_at > previous? + && !(state.attempt_status == Some(AttemptStatus::Closed) + && state.attempt_identity == Some(identity))) + .then_some(identity) } fn close_attempt(state: &mut State, now: i64, error: Option) { if let Some(AttemptIdentity::ResetAt(resets_at)) = state.attempt_identity { - state.last_observed_reset_at = Some(resets_at); + state.last_observed_reset_at = Some( + state + .last_observed_reset_at + .unwrap_or(resets_at) + .max(resets_at), + ); } state.attempt_status = Some(AttemptStatus::Closed); state.last_attempt_at = Some(now); @@ -346,34 +309,32 @@ fn close_attempt(state: &mut State, now: i64, error: Option) fn observe_after_completion(state: &mut State, usage: WeeklyWindowUsage) { if let WeeklyWindowUsage::Present { unused, resets_at } = usage { state.last_observed_active = !unused; - state.last_observed_reset_at = resets_at.or(state.last_observed_reset_at); + if let Some(resets_at) = resets_at { + state.last_observed_reset_at = Some( + state + .last_observed_reset_at + .unwrap_or(resets_at) + .max(resets_at), + ); + } } } -fn inside_suppression(state: &State, now: i64) -> bool { - state - .last_attempt_at - .is_some_and(|at| now < at.saturating_add(SUPPRESSION_SECONDS)) -} - -fn quarantine_state(usage: WeeklyWindowUsage, now: i64) -> State { +fn quarantine_state(usage: WeeklyWindowUsage) -> State { let mut state = State { - recovery_not_before: Some(now.saturating_add(SUPPRESSION_SECONDS)), last_error: Some(WeeklyWindowError::StateQuarantined), ..State::new() }; if let WeeklyWindowUsage::Present { unused, resets_at } = usage { state.last_observed_active = !unused; state.last_observed_reset_at = resets_at; - if unused { - baseline(&mut state, resets_at); - } } state } enum StateRead { Ready(State), + Legacy, Corrupt, Incompatible, } @@ -397,7 +358,10 @@ fn read_state(path: &Path) -> io::Result { let Some(version) = value.get("version").and_then(serde_json::Value::as_u64) else { return Ok(StateRead::Corrupt); }; - if version != 1 { + if version == 1 { + return Ok(StateRead::Legacy); + } + if version != u64::from(STATE_VERSION) { return Ok(StateRead::Incompatible); } Ok(serde_json::from_value(value).map_or(StateRead::Corrupt, StateRead::Ready)) diff --git a/codex-rs/login/src/codex_plus_plus/weekly_window_state_tests.rs b/codex-rs/login/src/codex_plus_plus/weekly_window_state_tests.rs index 7faec93dffcc..f9e8331bf7dc 100644 --- a/codex-rs/login/src/codex_plus_plus/weekly_window_state_tests.rs +++ b/codex-rs/login/src/codex_plus_plus/weekly_window_state_tests.rs @@ -4,7 +4,6 @@ use tempfile::TempDir; const ACTIVE: bool = false; const UNUSED: bool = true; -const RESETLESS: Option = None; fn test_store(automation_enabled: bool) -> (TempDir, AccountStore, AccountId) { let home = TempDir::new().expect("tempdir"); @@ -46,12 +45,6 @@ fn assert_unavailable(store: &AccountStore, id: &AccountId, usage: WeeklyWindowU )); } -fn finish_completed(attempt: WeeklyWindowAttempt, _now: i64) { - let refreshed_usage = usage(UNUSED, Some(i64::MAX)); - let outcome = WeeklyWindowAttemptOutcome::Completed { refreshed_usage }; - attempt.finish(outcome, _now).unwrap(); -} - fn finish_rejected(attempt: WeeklyWindowAttempt, _now: i64) { let error = WeeklyWindowRetryableError::Rejected; let outcome = WeeklyWindowAttemptOutcome::Retryable { error }; @@ -59,30 +52,45 @@ fn finish_rejected(attempt: WeeklyWindowAttempt, _now: i64) { } #[test] -fn reset_windows_baseline_dedupe_and_recover_dropped_dispatch() { +fn dated_unused_window_requires_outward_movement_and_recovers_dropped_dispatch() { let (_home, store, id) = test_store(/*automation_enabled*/ true); assert_not_due(&store, &id, usage(ACTIVE, Some(100)), 50); - let attempt = ready(&store, &id, usage(UNUSED, Some(200)), 60); + assert_not_due(&store, &id, usage(UNUSED, Some(200)), 60); + assert_not_due(&store, &id, usage(UNUSED, Some(200)), 61); + let attempt = ready(&store, &id, usage(UNUSED, Some(201)), 62); assert!(matches!( - store.begin_weekly_window_attempt(&id, usage(UNUSED, Some(200)), /*now*/ 60), + store.begin_weekly_window_attempt(&id, usage(UNUSED, Some(201)), /*now*/ 62), Ok(WeeklyWindowAttemptDecision::Locked) )); drop(attempt); - let stale_at = 60 + SUPPRESSION_SECONDS; - assert_not_due(&store, &id, usage(UNUSED, Some(300)), stale_at); + assert_not_due(&store, &id, usage(UNUSED, Some(202)), 63); let status = store.weekly_window_status(&id).unwrap(); assert_eq!(status.last_error, Some(WeeklyWindowError::Ambiguous)); - let attempt = ready(&store, &id, usage(UNUSED, Some(300)), stale_at + 1); - finish_completed(attempt, stale_at + 1); - assert_not_due(&store, &id, usage(UNUSED, Some(300)), stale_at + 2); - assert_not_due(&store, &id, usage(ACTIVE, RESETLESS), stale_at + 3); - let attempt = ready(&store, &id, usage(UNUSED, RESETLESS), stale_at + 4); - finish_completed(attempt, stale_at + 4); - let retry_at = stale_at + 4 + SUPPRESSION_SECONDS; - assert_not_due(&store, &id, usage(UNUSED, Some(i64::MAX)), retry_at - 1); - drop(ready(&store, &id, usage(UNUSED, Some(i64::MAX)), i64::MAX)); + assert_not_due(&store, &id, usage(UNUSED, Some(202)), 64); + let attempt = ready(&store, &id, usage(UNUSED, Some(203)), 65); + attempt + .finish( + WeeklyWindowAttemptOutcome::Completed { + refreshed_usage: usage(ACTIVE, Some(203)), + }, + /*now*/ 66, + ) + .unwrap(); + assert_not_due(&store, &id, usage(UNUSED, Some(300)), 67); + assert_not_due(&store, &id, usage(UNUSED, Some(300)), 68); + drop(ready(&store, &id, usage(UNUSED, Some(301)), 69)); + + let (_home, store, id) = test_store(/*automation_enabled*/ true); + assert_not_due(&store, &id, usage(UNUSED, Some(100)), 100); + drop(ready(&store, &id, usage(UNUSED, Some(101)), 101)); + assert_not_due(&store, &id, usage(ACTIVE, None), 102); + assert_not_due(&store, &id, usage(UNUSED, Some(200)), 103); + assert_not_due(&store, &id, usage(UNUSED, Some(200)), 104); + drop(ready(&store, &id, usage(UNUSED, Some(201)), 105)); + let (_home, disabled, id) = test_store(/*automation_enabled*/ false); assert_not_due(&disabled, &id, usage(UNUSED, Some(1)), 1); + assert_not_due(&disabled, &id, usage(UNUSED, Some(2)), 2); } #[test] @@ -97,6 +105,7 @@ fn weekly_scan_lease_is_nonblocking_and_released_on_drop() { #[test] fn retry_backoff_reuses_identity_and_caps() { let (home, store, id) = test_store(/*automation_enabled*/ true); + assert_not_due(&store, &id, usage(UNUSED, Some(9)), 9); let mut now = 10; for failure in 0..=MAX_FAILURE_COUNT { finish_rejected(ready(&store, &id, usage(UNUSED, Some(10)), now), now); @@ -121,16 +130,76 @@ fn retry_backoff_reuses_identity_and_caps() { } #[test] -fn resetless_windows_rearm_after_activity_and_seven_days() { +fn retry_tracks_reset_drift_and_resetless_activity() { let (_home, store, id) = test_store(/*automation_enabled*/ true); - finish_rejected(ready(&store, &id, usage(UNUSED, RESETLESS), 0), 0); - assert_not_due(&store, &id, usage(ACTIVE, RESETLESS), 1); - finish_completed(ready(&store, &id, usage(UNUSED, RESETLESS), 2), 2); - assert_not_due(&store, &id, usage(UNUSED, RESETLESS), 3); - assert!(matches!( - store.begin_weekly_window_attempt(&id, usage(UNUSED, RESETLESS), 2 + SUPPRESSION_SECONDS), - Ok(WeeklyWindowAttemptDecision::Ready(_)) - )); + assert_not_due(&store, &id, usage(UNUSED, Some(9)), 9); + finish_rejected(ready(&store, &id, usage(UNUSED, Some(10)), 10), 10); + assert_not_due(&store, &id, usage(UNUSED, Some(12)), 309); + assert_not_due(&store, &id, usage(UNUSED, Some(11)), 310); + finish_rejected(ready(&store, &id, usage(UNUSED, Some(12)), 311), 311); + assert_eq!( + store.weekly_window_status(&id).unwrap().retry_not_before, + Some(911) + ); + ready(&store, &id, usage(UNUSED, Some(12)), 911) + .finish( + WeeklyWindowAttemptOutcome::Completed { + refreshed_usage: WeeklyWindowUsage::Missing, + }, + /*now*/ 911, + ) + .unwrap(); + assert_not_due(&store, &id, usage(UNUSED, Some(12)), 912); + drop(ready(&store, &id, usage(UNUSED, Some(13)), 913)); + + let (_home, store, id) = test_store(/*automation_enabled*/ true); + assert_not_due(&store, &id, usage(UNUSED, Some(9)), 9); + finish_rejected(ready(&store, &id, usage(UNUSED, Some(10)), 10), 10); + assert_not_due(&store, &id, usage(ACTIVE, None), 11); + assert_not_due(&store, &id, usage(UNUSED, Some(11)), 12); + drop(ready(&store, &id, usage(UNUSED, Some(12)), 13)); + + let (_home, store, id) = test_store(/*automation_enabled*/ true); + assert_not_due(&store, &id, usage(UNUSED, Some(9)), 9); + finish_rejected(ready(&store, &id, usage(UNUSED, Some(10)), 10), 10); + assert_not_due(&store, &id, usage(UNUSED, Some(8)), 310); + drop(ready(&store, &id, usage(UNUSED, Some(10)), 311)); +} + +#[test] +fn reset_regression_does_not_reopen_a_closed_identity() { + let (_home, store, id) = test_store(/*automation_enabled*/ true); + assert_not_due(&store, &id, usage(UNUSED, Some(10)), 10); + ready(&store, &id, usage(UNUSED, Some(11)), 11) + .finish( + WeeklyWindowAttemptOutcome::Completed { + refreshed_usage: WeeklyWindowUsage::Missing, + }, + /*now*/ 11, + ) + .unwrap(); + assert_not_due(&store, &id, usage(UNUSED, Some(9)), 12); + assert_not_due(&store, &id, usage(UNUSED, Some(11)), 13); + drop(ready(&store, &id, usage(UNUSED, Some(12)), 14)); +} + +#[test] +fn legacy_retry_state_is_rebaselined() { + let (home, store, id) = test_store(/*automation_enabled*/ true); + let path = home.path().join("accounts/acct_test").join(STATE_FILE); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + let state = State { + version: 1, + attempt_identity: Some(AttemptIdentity::ResetAt(10)), + attempt_status: Some(AttemptStatus::Retryable), + retry_not_before: Some(0), + ..State::default() + }; + std::fs::write(&path, serde_json::to_vec(&state).unwrap()).unwrap(); + + assert_not_due(&store, &id, usage(UNUSED, Some(10)), 10); + assert_not_due(&store, &id, usage(UNUSED, Some(10)), 11); + drop(ready(&store, &id, usage(UNUSED, Some(11)), 12)); } #[test] @@ -145,23 +214,18 @@ fn bad_state_is_quarantined_or_preserved_without_credentials() { WeeklyWindowStatus { last_error: Some(WeeklyWindowError::StateQuarantined), retry_not_before: None, - recovery_not_before: Some(100 + SUPPRESSION_SECONDS), + recovery_not_before: None, } ); - drop(ready(&store, &id, usage(UNUSED, RESETLESS), 101)); - std::fs::write(&path, b"{broken").unwrap(); - assert_unavailable(&store, &id, usage(UNUSED, RESETLESS), 200); - let recovery_at = 200 + SUPPRESSION_SECONDS; - let reset_at = recovery_at + 100; - assert_not_due(&store, &id, usage(UNUSED, Some(reset_at)), recovery_at); - let rolled_usage = usage(UNUSED, Some(reset_at + 100)); - let attempt = ready(&store, &id, rolled_usage, recovery_at + 1); + assert_not_due(&store, &id, usage(UNUSED, Some(200)), 101); + assert_not_due(&store, &id, usage(UNUSED, Some(200)), 102); + let attempt = ready(&store, &id, usage(UNUSED, Some(201)), 103); assert_eq!(store.weekly_window_status(&id).unwrap().last_error, None); drop(attempt); - for incompatible in [br#"{"version":2}"#.to_vec(), vec![b'x'; 4097]] { + for incompatible in [br#"{"version":3}"#.to_vec(), vec![b'x'; 4097]] { std::fs::write(&path, &incompatible).unwrap(); - assert_unavailable(&store, &id, usage(UNUSED, RESETLESS), 101); + assert_unavailable(&store, &id, usage(UNUSED, None), 104); assert_eq!(std::fs::read(&path).unwrap(), incompatible); } }