From 186f7e71adc269fbf27c043ae05c8c4857a2fadd Mon Sep 17 00:00:00 2001 From: Sami Date: Wed, 5 Aug 2026 11:54:08 -0400 Subject: [PATCH] fix(cli): floor the 429 retry hint and raise its cap above the quota window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CLI honoured a relay `retry in {N}s` hint with `Duration::from_secs(s.min(30))` — no lower bound, and an upper bound below the window it is meant to outlast. Both ends were wrong, in the same direction: they made the client retry sooner than the relay asked. No floor meant a `retry in 0s` hint slept zero and retried instantly. The relay emits the hint precisely to stop that, and each early attempt is not free: it is denied again and still costs a counter increment, so a zero-length sleep converts one rate-limited client into the storm the hint exists to prevent. Sub-second hints now floor to RETRY_IN_MIN_SECS. The 30s cap sat below the relay's longest quota window. Hints of 51s have been observed in production against a 60s window, so any hint above 30s was silently truncated and the client woke inside a window it had been told to sit out — a guaranteed wasted attempt, and another increment. The cap is now 90s, above the window with margin. These sleeps happen between requests, so the cap is independent of the per-request BUZZ_TIMEOUT_SECS budget. Both 429 paths (with_retry_body and the moderation-command path) shared the same open-coded expression; they now share one pure clamp_retry_hint_secs, so the policy cannot drift between them. The existing parse_retry_in_zero_seconds test is kept as-is: a parser returning Some(0) for `retry in 0s` is correct, since it reports what the relay said. That test was not pinning the defect — the missing clamp was. Its docstring now points at the clamp test so the division of responsibility is not rediscovered later. Mutation results: removing the floor (reverting to `.min(MAX)`) fails the clamp test. Restoring the 30s cap fails to compile, caught by the const assertion that the cap outlasts a 60s window; that assertion pins the property rather than the literal, so the cap can be retuned without silently reintroducing the defect. Co-authored-by: Tyler Longwell Signed-off-by: Tyler Longwell --- crates/buzz-cli/src/client.rs | 89 +++++++++++++++++++++++++++++++---- 1 file changed, 81 insertions(+), 8 deletions(-) diff --git a/crates/buzz-cli/src/client.rs b/crates/buzz-cli/src/client.rs index ee8868ad9..c32e04b0f 100644 --- a/crates/buzz-cli/src/client.rs +++ b/crates/buzz-cli/src/client.rs @@ -125,9 +125,29 @@ const RETRY_MAX_ATTEMPTS: u32 = 3; /// `RETRY_BASE_SECS[i]` is the ceiling for attempt `i` before attempt `i+1`. const RETRY_BASE_SECS: [f64; 2] = [0.5, 1.5]; -/// Maximum seconds to honour a relay-provided `retry in Ns` hint from a 429. -/// Defensive cap against pathological hints; real relay hints observed up to ~24 s. -const RETRY_IN_MAX_SECS: u64 = 30; +/// Bounds for honouring a relay-provided `retry in Ns` hint from a 429. +/// +/// The floor exists because the hint is a *window TTL*, not a suggestion: +/// retrying before it expires is guaranteed to be denied again, and each denied +/// attempt still costs a counter increment at the relay. A `retry in 0s` hint +/// (or any sub-second value) would otherwise sleep zero and retry instantly, +/// turning the client into the storm the hint is trying to prevent. +/// +/// The ceiling is a defensive cap against a pathological hint. It must exceed +/// the relay's longest quota window, or the client wakes inside a window it was +/// told to sit out: hints of 51s have been observed in production against a 60s +/// window, so a 30s cap guaranteed a wasted attempt. Sleeps happen between +/// requests, so this is independent of the per-request `BUZZ_TIMEOUT_SECS`. +const RETRY_IN_MIN_SECS: u64 = 1; +const RETRY_IN_MAX_SECS: u64 = 90; + +/// Clamp a relay `retry in Ns` hint into the honoured range. +/// +/// Pure and total, so both call sites share one policy and the endpoints are +/// directly testable. +fn clamp_retry_hint_secs(secs: u64) -> Duration { + Duration::from_secs(secs.clamp(RETRY_IN_MIN_SECS, RETRY_IN_MAX_SECS)) +} /// Returns a full-jitter delay for attempt `i`: a random duration in `[0, RETRY_BASE_SECS[i])`. fn jitter_delay(attempt: u32) -> Duration { @@ -631,7 +651,8 @@ impl BuzzClient { /// failures and mid-body TCP drops. /// - `Err(CliError::Relay { status: 429 | 502 | 503 | 504, .. })` — transient relay /// or proxy errors. For 429 the `retry in Ns` hint from the body is used as the - /// delay (capped at `RETRY_IN_MAX_SECS`); all others use exponential jitter. + /// delay (clamped to `[RETRY_IN_MIN_SECS, RETRY_IN_MAX_SECS]`); all others use + /// exponential jitter. /// /// Use this variant for all operations (reads, writes, uploads); the retry boundary /// covers the entire operation including response body transfer. @@ -659,7 +680,7 @@ impl BuzzClient { } CliError::Relay { status: 429, body } => { let d = parse_retry_hint_text(body) - .map(|s| Duration::from_secs(s.min(RETRY_IN_MAX_SECS))) + .map(clamp_retry_hint_secs) .unwrap_or_else(|| jitter_delay(attempt)); Some(d) } @@ -943,7 +964,7 @@ impl BuzzClient { // timeout/body-loss after the relay may have acted). if !is_last { let delay = parse_retry_hint_text(msg) - .map(|s| Duration::from_secs(s.min(RETRY_IN_MAX_SECS))) + .map(clamp_retry_hint_secs) .unwrap_or_else(|| jitter_delay(attempt)); tokio::time::sleep(delay).await; continue; @@ -1442,8 +1463,9 @@ mod retry_tests { use std::time::Duration; use super::{ - env_duration_secs, is_moderation_kind, jitter_delay, parse_retry_hint_text, - parse_retry_in_secs, RETRY_BASE_SECS, RETRY_IN_MAX_SECS, RETRY_MAX_ATTEMPTS, + clamp_retry_hint_secs, env_duration_secs, is_moderation_kind, jitter_delay, + parse_retry_hint_text, parse_retry_in_secs, RETRY_BASE_SECS, RETRY_IN_MAX_SECS, + RETRY_IN_MIN_SECS, RETRY_MAX_ATTEMPTS, }; // ---- parse_retry_in_secs ---- @@ -1460,12 +1482,63 @@ mod retry_tests { assert_eq!(parse_retry_in_secs(body), Some(3)); } + /// A `retry in 0s` hint parses as `Some(0)`: the parser reports what the + /// relay said, faithfully. Turning 0 into a zero-length sleep is the + /// clamp's job to prevent, not the parser's — see + /// `clamp_retry_hint_secs_floors_zero_and_caps_pathological`. #[test] fn parse_retry_in_zero_seconds() { let body = r#"{"error":"retry in 0s"}"#; assert_eq!(parse_retry_in_secs(body), Some(0)); } + // ---- clamp_retry_hint_secs ---- + + /// The clamp is what stands between a relay hint and a retry storm. + /// + /// Endpoints are asserted exactly, in both directions: + /// + /// * A `retry in 0s` hint previously produced `Duration::from_secs(0)` — an + /// instant retry into a window the relay had just closed, which is the + /// storm the hint exists to prevent. It must now floor to a real sleep. + /// * A hint above the cap must be capped, and the cap must sit above the + /// relay's longest quota window. The old 30s cap was *below* the 51s + /// hints observed in production against a 60s window, so it guaranteed a + /// wasted attempt. Asserting `>= 60` pins the property (outlasts the + /// window) rather than the number, so retuning the cap does not + /// silently reintroduce the defect. + #[test] + fn clamp_retry_hint_secs_floors_zero_and_caps_pathological() { + assert_eq!( + clamp_retry_hint_secs(0), + Duration::from_secs(RETRY_IN_MIN_SECS), + "a `retry in 0s` hint must never sleep zero and retry instantly" + ); + assert_eq!( + clamp_retry_hint_secs(u64::MAX), + Duration::from_secs(RETRY_IN_MAX_SECS), + "a pathological hint must be capped" + ); + + // Values inside the range pass through untouched, including the 51s + // hint that the previous 30s cap silently truncated. + for secs in [RETRY_IN_MIN_SECS, 2, 24, 51, RETRY_IN_MAX_SECS] { + assert_eq!( + clamp_retry_hint_secs(secs), + Duration::from_secs(secs), + "a {secs}s hint is within range and must be honoured exactly" + ); + } + + const { assert!(RETRY_IN_MIN_SECS > 0) }; + const { + assert!( + RETRY_IN_MAX_SECS >= 60, + "the cap must outlast the relay's longest quota window" + ) + }; + } + #[test] fn parse_garbled_body_returns_none() { assert_eq!(parse_retry_in_secs("not json at all"), None);