External/buzz
1
0
Fork 0
mirror of https://github.com/block/buzz.git synced 2026-08-18 06:50:31 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix(cli): floor the 429 retry hint and raise its cap above the quota window

Browse Source 
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 <tlongwell@squareup.com>
Signed-off-by: Tyler Longwell <tlongwell@squareup.com>
This commit is contained in:
Sami
2026-08-05 11:54:08 -04:00
co-authored by Tyler Longwell
parent fd9e1e29ce
commit 186f7e71ad
1 changed files with 81 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/buzz-cli/src/client.rs
+81 -8
View File
@@ -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);