diff --git a/src/runner.rs b/src/runner.rs index 531f0b6..ec32f41 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -788,10 +788,10 @@ where cfg.rotation.cooldown_jitter_seconds, ); // Personal limits cool this seat only. Credits and spend caps - // are per *workspace*: every seat sharing this seat's - // account_id is equally blocked, so cool them together rather - // than burning an attempt discovering it. - let affected = affected_seats(&cfg, &chosen, reason); + // are per *workspace*: the seats sharing this seat's + // account_id that the blocker really reaches are cooled + // together rather than burning an attempt discovering it. + let affected = affected_seats(&cfg, &state, &chosen, reason, Utc::now()); cool_seats(&mut state, &affected, cd, reason, Utc::now()); let entry = state.entry_mut(&chosen); entry.consecutive_failures = entry.consecutive_failures.saturating_add(1); @@ -1128,14 +1128,24 @@ fn redeem_reset( } } -/// Seats to cool for a failure on `chosen`: just it for a personal limit; -/// every seat in the same workspace for credits / spend caps. -fn affected_seats(cfg: &SeatConfig, chosen: &str, reason: ratelimit::CooldownReason) -> Vec { +/// Seats to cool for a failure on `chosen`: just it for a personal limit. +/// For a workspace blocker, the same-workspace seats the blocker actually +/// reaches — every one of them for a spend cap, and for credits only those +/// with no included quota left (`usage::blocker_targets`). `chosen` is always +/// included: it just failed, whatever its recorded reading says. +fn affected_seats( + cfg: &SeatConfig, + state: &SeatState, + chosen: &str, + reason: ratelimit::CooldownReason, + now: DateTime, +) -> Vec { if reason.is_window_based() { - vec![chosen.to_string()] - } else { - workspace_siblings(cfg, chosen) + return vec![chosen.to_string()]; } + // A workspace blocker does not necessarily block every seat: see + // `usage::blocker_targets`. `chosen` is always included — it just failed. + usage::blocker_targets(state, &workspace_siblings(cfg, chosen), reason, Some(chosen), now) } /// Compose the `Seat:` line: which seat ran, under which strategy (or pin), diff --git a/src/usage.rs b/src/usage.rs index 07d58f8..1b51fb2 100644 --- a/src/usage.rs +++ b/src/usage.rs @@ -1152,6 +1152,47 @@ pub fn quota_state(st: &SeatRuntimeState, now: DateTime) -> QuotaState { } } +/// True when the seat's recorded reading shows it still has included quota to +/// run on. A seat with no reading at all does **not** count as having +/// headroom: with no evidence either way a workspace blocker stands, and the +/// next refresh corrects it. +pub fn has_included_headroom(st: &SeatRuntimeState, now: DateTime) -> bool { + st.usage + .as_ref() + .is_some_and(|snap| matches!(verdict(snap, now), UsageVerdict::Healthy)) +} + +/// Narrow a workspace-wide blocker to the seats it actually blocks. +/// +/// `credits` only bites a seat that has used up its included quota. A sibling +/// still inside its own windows runs for free and was never going to spend +/// credits, so cooling it takes a usable seat out of rotation for nothing — +/// and, because `credits` is neither lifted by a free reset nor given a real +/// reset time, it would stay out for a full `default_cooldown_seconds` and be +/// re-applied on every refresh for as long as the sibling stays blocked. +/// +/// `spend_control` is an admin-set hard stop on the workspace, so it keeps +/// cooling every member regardless of their own headroom. +/// +/// `always` is the seat whose own failure produced the blocker: it is cooled +/// whatever its (possibly stale) reading says. +pub fn blocker_targets( + state: &SeatState, + members: &[String], + reason: CooldownReason, + always: Option<&str>, + now: DateTime, +) -> Vec { + if reason != CooldownReason::Credits { + return members.to_vec(); + } + members + .iter() + .filter(|m| always == Some(m.as_str()) || !has_included_headroom(&state.get(m), now)) + .cloned() + .collect() +} + fn cooldown_until_for( reason: CooldownReason, resets_at: Option>, @@ -1255,12 +1296,17 @@ pub fn reapply_cached_exhaustion( /// 2. A seat's own personal exhaustion (a window at 100% with no credits, a /// per-seat reached flag) cools that seat, through `merge_cooldown`. /// 3. Per workspace, over the fetched members: a workspace-wide blocker -/// (spend cap, or a credits-depleted flag) cools every member of the -/// workspace — **blockers dominate regardless of order**; otherwise, if any +/// (spend cap, or a credits-depleted flag) cools the members it actually +/// blocks — every member for a spend cap, and for credits only those with +/// no included quota left to run on (see [`blocker_targets`]). +/// **Blockers dominate regardless of order**; otherwise, if any /// member shows credits available, cooldowns that credits make moot /// (`rate_limit`, `credits`) are cleared on every member. Clearing never /// spends anything: the credit policy still gates `OnCredits` seats. -/// 4. `needs_login` is never touched. +/// 4. A seat whose fresh reading is `Healthy` while it still carries a +/// `credits` cooldown has that cooldown cleared: credits are never that +/// seat's own block, and nothing else would lift it. +/// 5. `needs_login` is never touched. /// /// Returns `(seat, notice)` pairs for display and the events log. pub fn reconcile_snapshots( @@ -1341,7 +1387,8 @@ pub fn reconcile_snapshots( .max_by_key(|(r, _)| r.strength()); if let Some((reason, resets_at)) = blocker { let until = cooldown_until_for(reason, resets_at, &cfg.rotation, now); - let changed = seat::cool_seats(state, &members, until, reason, now); + let targets = blocker_targets(state, &members, reason, None, now); + let changed = seat::cool_seats(state, &targets, until, reason, now); if !changed.is_empty() { notices.push(( first.clone(), @@ -1402,19 +1449,46 @@ pub fn reconcile_snapshots( continue; } let st = state.get(name); - if let Some(u) = st.cooldown_until.filter(|u| *u > now) { + let Some(u) = st.cooldown_until.filter(|u| *u > now) else { + continue; + }; + let reason = CooldownReason::parse(st.cooldown_reason.as_deref().unwrap_or("")); + // A `credits` cooldown is never about this seat's own limits: it was + // propagated from a workspace sibling, at a moment when this seat's + // own headroom was unknown or stale. A fresh `Healthy` reading proves + // it can run on included quota, and `cool_seats` only ever extends a + // cooldown, so without this nothing would ever lift it and the + // blocked-run probe would reach the same dead end every time. + // + // `Healthy` is the whole condition: an `OnCredits` seat has no + // included quota left and must keep the cooldown. `rate_limit`, + // `model_limit` and `spend_control` are left alone — those are the + // seat's own block, not one inherited from a sibling. + if reason == CooldownReason::Credits && matches!(v, UsageVerdict::Healthy) { + let entry = state.entry_mut(name); + entry.cooldown_until = None; + entry.cooldown_reason = None; notices.push(( name.clone(), format!( - "seat '{}' is cooling until {} ({}) but reports {}; clear with `codex-clean seat status --clear-cooldown {}`", + "seat '{}' has included quota left ({}); cleared the credits cooldown propagated to it", name, - format_local(u), - st.cooldown_reason.as_deref().unwrap_or("rate_limit"), - st.usage.as_ref().map(summarize_usage_short).unwrap_or_else(|| "-".into()), - name + st.usage.as_ref().map(summarize_usage_short).unwrap_or_else(|| "-".into()) ), )); + continue; } + notices.push(( + name.clone(), + format!( + "seat '{}' is cooling until {} ({}) but reports {}; clear with `codex-clean seat status --clear-cooldown {}`", + name, + format_local(u), + st.cooldown_reason.as_deref().unwrap_or("rate_limit"), + st.usage.as_ref().map(summarize_usage_short).unwrap_or_else(|| "-".into()), + name + ), + )); } notices } @@ -2029,7 +2103,10 @@ mod tests { let cfg = cfg_ws(&[("a", "ws"), ("b", "ws")]); let mut blocked = snap_with(&[(10080, 50, Some(60))]); blocked.buckets[0].rate_limit_reached_type = Some("workspace_member_credits_depleted".into()); - let credits = with_credits(snap_with(&[(10080, 50, Some(60))])); + // b has no included quota left, so the workspace blocker reaches it; + // its own reading still claims credits, which must not mask the + // blocker whichever order the pair arrives in. + let credits = with_credits(snap_with(&[(10080, 100, Some(60))])); for order in [ vec![("a".to_string(), blocked.clone()), ("b".to_string(), credits.clone())], vec![("b".to_string(), credits.clone()), ("a".to_string(), blocked.clone())], @@ -2043,6 +2120,83 @@ mod tests { } } + #[test] + fn credits_blocker_spares_a_sibling_that_still_has_included_quota() { + // The workspace has no credits and `b` is flagged for it, but `a` is + // only part-way through its own windows: `a` runs for free and must + // stay usable. Cooling it would take the last good seat out of + // rotation, for a reason no free reset lifts, on every refresh. + let cfg = cfg_ws(&[("a", "ws"), ("b", "ws")]); + let a = snap_with(&[(300, 46, Some(7200)), (10080, 7, Some(600_000))]); + let mut b = snap_with(&[(300, 100, Some(9000)), (10080, 31, Some(600_000))]); + b.buckets[0].rate_limit_reached_type = Some("workspace_member_credits_depleted".into()); + + let mut state = SeatState::default(); + let n = reconcile_snapshots( + &cfg, + &mut state, + vec![("a".into(), a), ("b".into(), b)], + now(), + ); + assert!(state.get("a").cooldown_until.is_none(), "in-quota sibling left usable"); + assert_eq!(state.get("b").cooldown_reason.as_deref(), Some("credits"), "the flagged seat still cools"); + assert!( + n.iter().any(|(_, m)| m.starts_with("credits is workspace-wide; cooling b until ")), + "the notice names only the seat actually cooled: {:?}", + n + ); + assert!( + !n.iter().any(|(_, m)| m.contains("--clear-cooldown a")), + "no 'cooling but reports fine' advice for a seat that was never cooled: {:?}", + n + ); + } + + #[test] + fn credits_blocker_cools_a_sibling_with_no_reading_of_its_own() { + // No evidence of headroom: the blocker stands, and the blocked-run + // probe lifts it again once `a` has a reading. + let cfg = cfg_ws(&[("a", "ws"), ("b", "ws")]); + let mut b = snap_with(&[(300, 100, Some(9000))]); + b.buckets[0].rate_limit_reached_type = Some("workspace_member_credits_depleted".into()); + let mut state = SeatState::default(); + reconcile_snapshots(&cfg, &mut state, vec![("b".into(), b)], now()); + assert_eq!(state.get("a").cooldown_reason.as_deref(), Some("credits"), "unknown sibling cooled"); + + // Now `a` reports headroom. The propagated cooldown must be lifted by + // that evidence alone: cool_seats only ever extends, so if this does + // not clear it, nothing does, and the blocked-run probe dead-ends here + // every time. + let a = snap_with(&[(300, 46, Some(7200))]); + let n = reconcile_snapshots(&cfg, &mut state, vec![("a".into(), a)], now()); + assert!(state.get("a").cooldown_until.is_none(), "fresh headroom lifts the propagated cooldown"); + assert!( + n.iter().any(|(s, m)| s == "a" && m.contains("cleared the credits cooldown propagated to it")), + "{:?}", + n + ); + } + + #[test] + fn spend_control_still_cools_every_member_whatever_their_headroom() { + // An admin-set hard stop is not about included quota, so headroom + // does not spare a sibling the way it does for credits. + let cfg = cfg_ws(&[("a", "ws"), ("b", "ws")]); + let a = snap_with(&[(300, 46, Some(7200))]); + let mut b = snap_with(&[(300, 12, Some(7200))]); + b.spend_control_reached = Some(true); + let mut state = SeatState::default(); + reconcile_snapshots(&cfg, &mut state, vec![("a".into(), a), ("b".into(), b)], now()); + for seat in ["a", "b"] { + assert_eq!( + state.get(seat).cooldown_reason.as_deref(), + Some("spend_control"), + "{} cooled", + seat + ); + } + } + #[test] fn reconcile_snapshot_is_the_probe_after_expiry() { let cfg = cfg_ws(&[("a", "ws")]); diff --git a/tests/seat_integration.rs b/tests/seat_integration.rs index c65621a..5f64bf1 100644 --- a/tests/seat_integration.rs +++ b/tests/seat_integration.rs @@ -1588,6 +1588,55 @@ fn workspace_cooldown_never_shortens_a_siblings_longer_cooldown() { assert_eq!(st.get("main").cooldown_reason.as_deref(), Some("credits")); } +#[test] +fn credits_failure_leaves_a_sibling_with_cached_headroom_runnable() { + // The run path, not the snapshot path: `main` fails with a credits error + // and `backup1` shares its workspace but has a cached reading showing + // plenty of included quota. `backup1` runs on that quota for free, so the + // credits blocker must not reach it. Before the blocker was scoped, it + // was cooled alongside `main` and the run ended 75 with a usable seat + // sitting idle. + let _g = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let env = TestEnv::new(); + let mut cfg = cfg_with_seats(&[("main", "ws-1"), ("backup1", "ws-1")]); + cfg.seats[0].user_id = Some("user-alice".into()); + cfg.seats[1].user_id = Some("user-bob".into()); + env.save_config(&cfg); + let seat_dir = env.clean_home_path.join("seats"); + fs::create_dir_all(seat_dir.join("main")).unwrap(); + fs::create_dir_all(seat_dir.join("backup1")).unwrap(); + fs::write(seat_dir.join("main/auth.json"), seat::fake_auth_json_for_tests("ws-1", "user-alice", "a")).unwrap(); + fs::write(seat_dir.join("backup1/auth.json"), seat::fake_auth_json_for_tests("ws-1", "user-bob", "b")).unwrap(); + + let mut state = SeatState::default(); + // backup1 used more recently, so LRU reaches for main first. + state.entry_mut("backup1").last_used = Some(chrono::Utc::now()); + state.entry_mut("backup1").usage = Some(snapshot(10, 10)); + env.save_state(&state); + + // First attempt (main) fails for credits; the second (backup1) succeeds. + let calls = RefCell::new(0usize); + let attempt = move |_args: &[String], _prompt: &str, _mode: &Mode, _scrub: bool| -> anyhow::Result { + *calls.borrow_mut() += 1; + if *calls.borrow() == 1 { + let mut a = rate_limit_attempt(); + a.output.errors = vec!["Your workspace is out of credits. Add credits to continue.".to_string()]; + return Ok(a); + } + Ok(ok_attempt()) + }; + let exit = runner::run_codex_with(&[], "hi", Mode::Exec, attempt).unwrap(); + + assert_eq!(exit, 0, "the run falls through to the seat that still has quota"); + let st = env.load_state(); + assert_eq!(st.get("main").cooldown_reason.as_deref(), Some("credits"), "the seat that failed is cooled"); + assert!( + st.get("backup1").cooldown_until.is_none(), + "a sibling with included quota left is not cooled by the workspace credits blocker" + ); + assert!(st.get("backup1").is_eligible(chrono::Utc::now()), "and stays eligible for the next run"); +} + #[test] fn status_propagates_workspace_wide_exhaustion_to_siblings() { let _g = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner());