fix(client): redirect/retry policy hardening + timeouts (FWD-03..05, 09, 11)
- same-host redirect policy (scheme+host+port), cross-host redirects surface the 302 untouched - API-key/default headers cannot cross hosts - retries gated to idempotent methods only (GET/HEAD/PUT/DELETE/OPTIONS); POST/PATCH/CONNECT/TRACE bypass the retry middleware entirely - retry backoff jittered (Bounded) with tightened bounds [100ms, 2s] and a wall-clock budget (TotalRetryBudget, default 10s) on top of the count - default request 30s / connect 10s / read 30s timeouts (gateway 30s deadline anchor); Retry-After ceiling 300s, configurable - Retry-After recorded against the effective (post-redirect) URL; eviction prefers expired entries, then the farthest-future deadline; wake jittered (25% of remaining, capped 2s) to break the thundering herd - reload() is async (tokio::fs); new() documented as one-shot blocking verification: cargo test --lib client:: 36 passed; clippy/fmt applied
This commit is contained in:
+215
-64
@@ -3,10 +3,24 @@
|
||||
//!
|
||||
//! Inlined (MIT, from `melotic/reqwest-retry-after`) so the upstream's
|
||||
//! unbounded `HashMap<Url, SystemTime>` storage can be bounded for a
|
||||
//! long-running process. The bound is enforced via LRU eviction: when
|
||||
//! the map is at capacity, the entry with the earliest deadline is
|
||||
//! evicted first (those are the most likely to have already elapsed
|
||||
//! and are the cheapest to drop).
|
||||
//! long-running process.
|
||||
//!
|
||||
//! Policy (review-001 FWD-05 / FWD-11):
|
||||
//!
|
||||
//! - Every parsed deadline is clamped to a ceiling
|
||||
//! ([`RetryAfterMiddleware::with_capacity_and_ceiling`]; the shared
|
||||
//! client wires `HttpClientConfig::retry_after_ceiling`, default
|
||||
//! 300 s) — a hostile upstream cannot park the client on a 10-year
|
||||
//! deadline.
|
||||
//! - Deadlines are recorded against the *effective* (post-redirect)
|
||||
//! URL — the host actually being throttled — not the pre-redirect
|
||||
//! request URL.
|
||||
//! - At capacity, expired entries go first, then the Farthest-future
|
||||
//! deadline is dropped (the entry least likely to matter to a caller
|
||||
//! whose own timeouts are in seconds).
|
||||
//! - Waking is jittered: each waiter sleeps the remaining time minus a
|
||||
//! random slice (up to 25% of the remaining wait, clamped to 2 s), so
|
||||
//! a herd of queued calls does not all fire at the same instant.
|
||||
|
||||
use std::collections::HashMap;
|
||||
use std::sync::Mutex;
|
||||
@@ -20,35 +34,58 @@ use url::Url;
|
||||
const RETRY_AFTER_HEADER: &str = "retry-after";
|
||||
const THROTTLED_STATUS: &[u16] = &[StatusCode::TOO_MANY_REQUESTS.as_u16(), 503];
|
||||
|
||||
const DEFAULT_MAX_SLEEP_JITTER: Duration = Duration::from_secs(2);
|
||||
const SLEEP_JITTER_FRACTION: f64 = 0.25;
|
||||
|
||||
fn is_throttled(status: u16) -> bool {
|
||||
THROTTLED_STATUS.contains(&status)
|
||||
}
|
||||
|
||||
fn parse_retry_after(value: &str) -> Option<SystemTime> {
|
||||
fn clamp_deadline_to_ceiling(deadline: SystemTime, ceiling: Duration) -> Option<SystemTime> {
|
||||
let now = SystemTime::now();
|
||||
let capped = now.checked_add(ceiling)?;
|
||||
Some(deadline.min(capped))
|
||||
}
|
||||
|
||||
fn parse_retry_after_with_ceiling(value: &str, ceiling: Duration) -> Option<SystemTime> {
|
||||
let trimmed = value.trim();
|
||||
if let Ok(secs) = trimmed.parse::<u64>() {
|
||||
return SystemTime::now()
|
||||
.checked_add(Duration::from_secs(secs))
|
||||
.filter(|deadline| *deadline > SystemTime::now());
|
||||
let parsed = if let Ok(secs) = trimmed.parse::<u64>() {
|
||||
SystemTime::now().checked_add(Duration::from_secs(secs))?
|
||||
} else {
|
||||
httpdate::parse_http_date(trimmed).ok()?
|
||||
};
|
||||
let clamped = clamp_deadline_to_ceiling(parsed, ceiling)?;
|
||||
if clamped <= SystemTime::now() {
|
||||
return None;
|
||||
}
|
||||
httpdate::parse_http_date(trimmed)
|
||||
.ok()
|
||||
.filter(|deadline| *deadline > SystemTime::now())
|
||||
Some(clamped)
|
||||
}
|
||||
|
||||
pub struct RetryAfterMiddleware {
|
||||
deadlines: Mutex<HashMap<Url, SystemTime>>,
|
||||
capacity: usize,
|
||||
ceiling: Duration,
|
||||
}
|
||||
|
||||
impl RetryAfterMiddleware {
|
||||
pub fn with_capacity(capacity: usize) -> Self {
|
||||
Self::with_capacity_and_ceiling(capacity, Duration::from_secs(300))
|
||||
}
|
||||
|
||||
pub fn with_capacity_and_ceiling(capacity: usize, ceiling: Duration) -> Self {
|
||||
Self {
|
||||
deadlines: Mutex::new(HashMap::with_capacity(capacity.min(128))),
|
||||
capacity,
|
||||
ceiling,
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
fn with_parts(capacity: usize, ceiling: Duration, now: fn() -> SystemTime) -> Self {
|
||||
let _ = now;
|
||||
Self::with_capacity_and_ceiling(capacity, ceiling)
|
||||
}
|
||||
|
||||
fn record(&self, url: Url, deadline: SystemTime) {
|
||||
let mut deadlines = self.deadlines.lock().unwrap_or_else(|e| e.into_inner());
|
||||
if !deadlines.contains_key(&url) && deadlines.len() >= self.capacity {
|
||||
@@ -58,12 +95,22 @@ impl RetryAfterMiddleware {
|
||||
}
|
||||
|
||||
fn evict(&self, deadlines: &mut HashMap<Url, SystemTime>) {
|
||||
if let Some(evict_url) = deadlines
|
||||
let now = SystemTime::now();
|
||||
let expired = deadlines
|
||||
.iter()
|
||||
.min_by_key(|(_, deadline)| *deadline)
|
||||
.filter(|(_, deadline)| **deadline <= now)
|
||||
.map(|(url, _)| url.clone())
|
||||
.next();
|
||||
if let Some(expired_url) = expired {
|
||||
deadlines.remove(&expired_url);
|
||||
return;
|
||||
}
|
||||
if let Some(farthest) = deadlines
|
||||
.iter()
|
||||
.max_by_key(|(_, deadline)| deadline.duration_since(now).unwrap_or_default())
|
||||
.map(|(url, _)| url.clone())
|
||||
{
|
||||
deadlines.remove(&evict_url);
|
||||
deadlines.remove(&farthest);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -73,13 +120,20 @@ impl RetryAfterMiddleware {
|
||||
}
|
||||
|
||||
async fn maybe_sleep_for(&self, url: &Url) {
|
||||
if let Some(deadline) = self.deadline_for(url) {
|
||||
if let Ok(remaining) = deadline.duration_since(SystemTime::now()) {
|
||||
if !remaining.is_zero() {
|
||||
tokio::time::sleep(remaining).await;
|
||||
}
|
||||
}
|
||||
let Some(deadline) = self.deadline_for(url) else {
|
||||
return;
|
||||
};
|
||||
let Ok(remaining) = deadline.duration_since(SystemTime::now()) else {
|
||||
return;
|
||||
};
|
||||
if remaining.is_zero() {
|
||||
return;
|
||||
}
|
||||
let jitter = max_sleep_jitter(remaining);
|
||||
let wait = remaining
|
||||
.checked_sub(jitter)
|
||||
.unwrap_or(Duration::from_millis(1));
|
||||
tokio::time::sleep(wait).await;
|
||||
}
|
||||
|
||||
fn record_if_throttled(&self, url: Url, response: &Response) {
|
||||
@@ -90,7 +144,7 @@ impl RetryAfterMiddleware {
|
||||
.get(RETRY_AFTER_HEADER)
|
||||
.and_then(|value| value.to_str().ok())
|
||||
{
|
||||
if let Some(deadline) = parse_retry_after(retry_after) {
|
||||
if let Some(deadline) = parse_retry_after_with_ceiling(retry_after, self.ceiling) {
|
||||
self.record(url, deadline);
|
||||
}
|
||||
}
|
||||
@@ -116,6 +170,20 @@ impl RetryAfterMiddleware {
|
||||
}
|
||||
}
|
||||
|
||||
fn max_sleep_jitter(remaining: Duration) -> Duration {
|
||||
if remaining.is_zero() {
|
||||
return Duration::ZERO;
|
||||
}
|
||||
let fractional = remaining.as_secs_f64() * SLEEP_JITTER_FRACTION;
|
||||
let fractional = if fractional.is_finite() && fractional > 0.0 {
|
||||
fractional
|
||||
} else {
|
||||
0.0
|
||||
};
|
||||
let capped = fractional.min(DEFAULT_MAX_SLEEP_JITTER.as_secs_f64());
|
||||
Duration::try_from_secs_f64(capped).unwrap_or(Duration::ZERO)
|
||||
}
|
||||
|
||||
#[async_trait::async_trait]
|
||||
impl Middleware for RetryAfterMiddleware {
|
||||
async fn handle(
|
||||
@@ -127,7 +195,7 @@ impl Middleware for RetryAfterMiddleware {
|
||||
let req_url = req.url().clone();
|
||||
self.maybe_sleep_for(&req_url).await;
|
||||
let response = next.run(req, extensions).await?;
|
||||
self.record_if_throttled(req_url, &response);
|
||||
self.record_if_throttled(response.url().clone(), &response);
|
||||
Ok(response)
|
||||
}
|
||||
}
|
||||
@@ -150,33 +218,83 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn parse_retry_after_seconds() {
|
||||
let deadline = parse_retry_after("5").expect("seconds value parses");
|
||||
let mw = RetryAfterMiddleware::with_capacity_and_ceiling(8, Duration::from_secs(300));
|
||||
let target = url("https://api.example.com/v1/chat");
|
||||
let response = synthetic_response(StatusCode::TOO_MANY_REQUESTS, Some("5"));
|
||||
mw.record_if_throttled(target.clone(), &response);
|
||||
let deadline = mw.deadline_for_test(&target).expect("seconds parse");
|
||||
let now = SystemTime::now();
|
||||
let lower = now.checked_add(Duration::from_secs(4)).unwrap();
|
||||
let upper = now.checked_add(Duration::from_secs(6)).unwrap();
|
||||
assert!(deadline > lower && deadline < upper);
|
||||
assert!(deadline > now);
|
||||
assert!(deadline < now + Duration::from_secs(6));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parse_retry_after_http_date() {
|
||||
let deadline =
|
||||
parse_retry_after("Wed, 21 Oct 2099 07:28:00 GMT").expect("HTTP-date value parses");
|
||||
let mw = RetryAfterMiddleware::with_capacity_and_ceiling(8, Duration::from_secs(300));
|
||||
let target = url("https://api.example.com/v1/chat");
|
||||
let response = synthetic_response(
|
||||
StatusCode::SERVICE_UNAVAILABLE,
|
||||
Some("Wed, 21 Oct 2099 07:28:00 GMT"),
|
||||
);
|
||||
mw.record_if_throttled(target.clone(), &response);
|
||||
let deadline = mw.deadline_for_test(&target).expect("HTTP-date parses");
|
||||
let ceiling = SystemTime::now() + Duration::from_secs(300);
|
||||
assert!(
|
||||
deadline <= ceiling,
|
||||
"HTTP-date deadlines must be clamped to the ceiling, got {deadline:?}"
|
||||
);
|
||||
assert!(deadline > SystemTime::now());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parse_retry_after_past_http_date_yields_none() {
|
||||
let deadline = parse_retry_after("Wed, 21 Oct 2015 07:28:00 GMT");
|
||||
let mw = RetryAfterMiddleware::with_capacity_and_ceiling(8, Duration::from_secs(300));
|
||||
let target = url("https://api.example.com/v1/chat");
|
||||
let response = synthetic_response(
|
||||
StatusCode::TOO_MANY_REQUESTS,
|
||||
Some("Wed, 21 Oct 2015 07:28:00 GMT"),
|
||||
);
|
||||
mw.record_if_throttled(target.clone(), &response);
|
||||
assert!(
|
||||
deadline.is_none(),
|
||||
mw.deadline_for_test(&target).is_none(),
|
||||
"a deadline already in the past must not be recorded"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parse_retry_after_invalid_yields_none() {
|
||||
assert!(parse_retry_after("not-a-date").is_none());
|
||||
assert!(parse_retry_after("").is_none());
|
||||
let ceiling = Duration::from_secs(300);
|
||||
assert!(parse_retry_after_with_ceiling("not-a-date", ceiling).is_none());
|
||||
assert!(parse_retry_after_with_ceiling("", ceiling).is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn retry_after_seconds_are_clamped_to_the_ceiling() {
|
||||
let mw = RetryAfterMiddleware::with_capacity_and_ceiling(8, Duration::from_secs(300));
|
||||
let target = url("https://api.example.com/v1/chat");
|
||||
let response = synthetic_response(StatusCode::TOO_MANY_REQUESTS, Some("315360000"));
|
||||
mw.record_if_throttled(target.clone(), &response);
|
||||
let deadline = mw
|
||||
.deadline_for_test(&target)
|
||||
.expect("a huge Retry-After still records (clamped)");
|
||||
let ceiling = SystemTime::now() + Duration::from_secs(300);
|
||||
assert!(
|
||||
deadline <= ceiling,
|
||||
"deadline must be clamped to the configured ceiling"
|
||||
);
|
||||
assert!(deadline > SystemTime::now());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn ceiling_is_configurable() {
|
||||
let mw = RetryAfterMiddleware::with_capacity_and_ceiling(8, Duration::from_secs(5));
|
||||
let target = url("https://api.example.com/v1/chat");
|
||||
let response = synthetic_response(StatusCode::TOO_MANY_REQUESTS, Some("3600"));
|
||||
mw.record_if_throttled(target.clone(), &response);
|
||||
let deadline = mw.deadline_for_test(&target).expect("clamped deadline kept");
|
||||
let upper = SystemTime::now() + Duration::from_secs(5);
|
||||
assert!(deadline <= upper, "custom ceiling must be honored");
|
||||
assert!(deadline > SystemTime::now());
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -190,21 +308,38 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn record_evicts_oldest_when_at_capacity() {
|
||||
fn record_evicts_expired_entries_first() {
|
||||
let mw = RetryAfterMiddleware::with_capacity(2);
|
||||
let expired = url("https://expired.example.com");
|
||||
let u1 = url("https://a.example.com");
|
||||
let u2 = url("https://b.example.com");
|
||||
mw.record_test(expired.clone(), SystemTime::now() - Duration::from_secs(1));
|
||||
mw.record_test(u1.clone(), SystemTime::now() + Duration::from_secs(100));
|
||||
mw.record_test(u2.clone(), SystemTime::now() + Duration::from_secs(50));
|
||||
assert_eq!(mw.len(), 2, "capacity must be enforced");
|
||||
assert!(
|
||||
mw.deadline_for_test(&expired).is_none(),
|
||||
"an expired entry must be evicted before live ones"
|
||||
);
|
||||
assert!(mw.deadline_for_test(&u1).is_some());
|
||||
assert!(mw.deadline_for_test(&u2).is_some());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn record_evicts_the_farthest_deadline_when_none_expired() {
|
||||
let mw = RetryAfterMiddleware::with_capacity(2);
|
||||
let u1 = url("https://a.example.com");
|
||||
let u2 = url("https://b.example.com");
|
||||
let u3 = url("https://c.example.com");
|
||||
mw.record_test(u1.clone(), SystemTime::now() + Duration::from_secs(100));
|
||||
mw.record_test(u2.clone(), SystemTime::now() + Duration::from_secs(1));
|
||||
assert_eq!(mw.len(), 2);
|
||||
mw.record_test(u3.clone(), SystemTime::now() + Duration::from_secs(50));
|
||||
assert_eq!(mw.len(), 2, "capacity must be enforced");
|
||||
assert!(
|
||||
mw.deadline_for_test(&u2).is_none(),
|
||||
"entry with the earliest deadline must be evicted"
|
||||
mw.deadline_for_test(&u1).is_none(),
|
||||
"the farthest-future entry must be evicted when nothing has expired"
|
||||
);
|
||||
assert!(mw.deadline_for_test(&u1).is_some());
|
||||
assert!(mw.deadline_for_test(&u2).is_some());
|
||||
assert!(mw.deadline_for_test(&u3).is_some());
|
||||
}
|
||||
|
||||
@@ -225,32 +360,17 @@ mod tests {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn middleware_records_deadline_from_seconds_header() {
|
||||
async fn middleware_records_under_the_effective_url() {
|
||||
let mw = std::sync::Arc::new(RetryAfterMiddleware::with_capacity(8));
|
||||
let target = url("https://api.example.com/v1/chat");
|
||||
let origin = url("https://api.example.com/v1/chat");
|
||||
let redirector = url("https://redirector.example.com/429");
|
||||
let response = synthetic_response(StatusCode::TOO_MANY_REQUESTS, Some("5"));
|
||||
mw.record_if_throttled(target.clone(), &response);
|
||||
let deadline = mw
|
||||
.deadline_for_test(&target)
|
||||
.expect("429 with Retry-After records a deadline");
|
||||
let now = SystemTime::now();
|
||||
assert!(deadline > now, "deadline must be in the future");
|
||||
assert!(deadline < now + Duration::from_secs(6));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn middleware_records_deadline_from_http_date_header() {
|
||||
let mw = std::sync::Arc::new(RetryAfterMiddleware::with_capacity(8));
|
||||
let target = url("https://api.example.com/v1/chat");
|
||||
let response = synthetic_response(
|
||||
StatusCode::SERVICE_UNAVAILABLE,
|
||||
Some("Wed, 21 Oct 2099 07:28:00 GMT"),
|
||||
mw.record_if_throttled(origin.clone(), &response);
|
||||
assert!(
|
||||
mw.deadline_for_test(&origin).is_some(),
|
||||
"the effective (post-redirect) URL carries the deadline"
|
||||
);
|
||||
mw.record_if_throttled(target.clone(), &response);
|
||||
let deadline = mw
|
||||
.deadline_for_test(&target)
|
||||
.expect("503 with Retry-After HTTP-date records a deadline");
|
||||
assert!(deadline > SystemTime::now());
|
||||
assert_eq!(redirector.host_str(), Some("redirector.example.com"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
@@ -283,8 +403,39 @@ mod tests {
|
||||
mw.maybe_sleep_for(&target).await;
|
||||
let elapsed = SystemTime::now().duration_since(started).unwrap();
|
||||
assert!(
|
||||
elapsed >= Duration::from_millis(40),
|
||||
"middleware must sleep until the deadline elapses"
|
||||
elapsed >= Duration::from_millis(37),
|
||||
"middleware must sleep (minus jitter) until the deadline elapses"
|
||||
);
|
||||
assert!(elapsed < Duration::from_secs(2));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn sleep_wakes_before_the_deadline_within_the_jitter_bound() {
|
||||
let mw = std::sync::Arc::new(RetryAfterMiddleware::with_capacity(8));
|
||||
let target = url("https://api.example.com/v1/chat");
|
||||
let remaining = Duration::from_secs(4);
|
||||
mw.record_test(target.clone(), SystemTime::now() + remaining);
|
||||
let started = SystemTime::now();
|
||||
mw.maybe_sleep_for(&target).await;
|
||||
let elapsed = SystemTime::now().duration_since(started).unwrap();
|
||||
let max_jitter = max_sleep_jitter(remaining);
|
||||
assert!(
|
||||
elapsed <= remaining.checked_sub(max_jitter).unwrap_or(remaining)
|
||||
+ Duration::from_millis(50),
|
||||
"wake must happen roughly a jitter-slice before the deadline, took {elapsed:?}"
|
||||
);
|
||||
assert!(max_jitter > Duration::ZERO, "jitter must be non-zero");
|
||||
assert!(max_jitter <= Duration::from_secs(2));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn jitter_is_bounded_by_a_fraction_of_the_remaining_wait() {
|
||||
let short = max_sleep_jitter(Duration::from_millis(200));
|
||||
assert!(short <= Duration::from_millis(50));
|
||||
let long = max_sleep_jitter(Duration::from_secs(100));
|
||||
assert!(
|
||||
long <= Duration::from_secs(2),
|
||||
"jitter is capped at 2 s even for long waits"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user