diff --git a/tasks/client/review-001-client-timeout-retry.md b/tasks/client/review-001-client-timeout-retry.md index 933ca63..3fff3c4 100644 --- a/tasks/client/review-001-client-timeout-retry.md +++ b/tasks/client/review-001-client-timeout-retry.md @@ -1,7 +1,7 @@ --- id: review-001-client-timeout-retry name: Client redirect/Retry-After policy — idempotency, timeouts, caps (FWD-03, FWD-04, FWD-05, FWD-11, FWD-09) -status: pending +status: completed depends_on: [] scope: moderate risk: medium @@ -43,13 +43,13 @@ shape the shared client's request policy: ## Acceptance Criteria -- [ ] Cross-host redirect test with an API-key credential header — key must not reach the redirect target -- [ ] Non-idempotent method is never retried (test); total retry duration bounded (test) -- [ ] Default request + connect timeouts exist in `HttpClientConfig::default()`; `Retry-After` capped (tests) -- [ ] Retry-After keyed post-redirect; eviction and wake behavior fixed or documented (tests) -- [ ] No blocking fs reads on the async path (FWD-09) -- [ ] `HttpClientConfig` defaults documented; review-001-forward-url-safety and this task together close the deployment-facing gate -- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass +- [x] Cross-host redirect test with an API-key credential header — key must not reach the redirect target +- [x] Non-idempotent method is never retried (test); total retry duration bounded (test) +- [x] Default request + connect timeouts exist in `HttpClientConfig::default()`; `Retry-After` capped (tests) +- [x] Retry-After keyed post-redirect; eviction and wake behavior fixed or documented (tests) +- [x] No blocking fs reads on the async path (FWD-09) +- [x] `HttpClientConfig` defaults documented; review-001-forward-url-safety and this task together close the deployment-facing gate +- [x] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass ## References @@ -62,6 +62,72 @@ shape the shared client's request policy: > review-001-forward-url-safety (different file); they form the > deployment gate together. +Policy values chosen (documented in the `http_client.rs` module docs and +`HttpClientConfig` field docs): + +- Redirect policy: same-host only (scheme + host + port must match), 10-hop + cap; cross-host redirects surface the 302 response untouched — headers + never travel to another host. Rationale: same-host hops preserve the + legitimate redirect-following convenience (oauth-ish flows behind one + origin) while structurally ruling out credential exfiltration; the + alternative (redirects none) is a one-line config swap for callers who + want stricter behavior. +- Retries: method-gated (GET/HEAD/PUT/DELETE/OPTIONS only; POST/PATCH/ + CONNECT/TRACE bypass the retry middleware entirely) + attempt count 3 + + backoff bounds [100 ms, 2 s] with `Jitter::Bounded` + wall-clock + budget `max_total_retry_duration` (default 10 s) enforced by + `TotalRetryBudget` (stops retries and clamps scheduled retries to the + budget deadline). +- Default timeouts: request 30 s (gateway deadline anchor), connect + 10 s, read 30 s; all configurable, `None` disables. +- Retry-After ceiling: 300 s default, configurable per client + (`retry_after_ceiling`) / per middleware instance; applies to both + seconds and HTTP-date forms; past deadlines still rejected. +- FWD-11: deadlines recorded under the effective (post-redirect) URL + (`response.url()`); eviction prefers expired entries, then the + farthest-future deadline (least actionable first); wakes jittered by + 25% of the remaining wait, capped at 2 s. +- FWD-09: `SharedHttpClient::reload` is now async (`tokio::fs::read` — + `spawn_blocking`-backed); PEM reads factored into read-first + pure + builder. `SharedHttpClient::new` remains sync and is documented as + one-shot blocking construction at assembly time (never on a request or + hot-reload path) — kept sync deliberately to avoid churning ~15 + constructor call sites in adapter tests; reload (the documented + hot-reload path) is fully non-blocking. + ## Summary -> Filled on completion. \ No newline at end of file +Implemented in `src/client/http_client.rs` + `src/client/retry_after.rs` +(commits 015b241, b1529dd, 4a557a0): + +- **FWD-03**: `same_host_redirect_policy()` — reqwest `Policy::custom` + following only scheme+host+port matches (10-hop cap via + `attempt.error`); everything else `attempt.stop()`. Tests: + `cross_host_redirect_does_not_leak_api_key_header` (raw-TCP attacker + + redirector listeners assert the key never reaches the target and no + request arrives at all), `same_host_redirect_is_still_followed`. +- **FWD-04**: `RetryGateMiddleware` gates idempotent methods into + `RetryTransientMiddleware` wrapped in `TotalRetryBudget` (wall-clock + cap; also clamps scheduled retry times into the budget). Tests: + `non_idempotent_post_gets_one_attempt_then_the_429_is_surfaced` (500 on + POST → exactly 1 upstream hit), `idempotent_get_is_retried_on_a_ + transient_failure` (500,500,200 → 3 hits), policy unit tests. +- **FWD-05**: `HttpClientConfig::default()` → request 30 s / connect + 10 s / read 30 s; `retry_after_ceiling` (default 300 s) clamps both + numeric and HTTP-date `Retry-After` values in + `parse_retry_after_with_ceiling`. Tests: default-config assertions, + 10-year Retry-After clamped, custom 5 s ceiling honored. +- **FWD-11**: record under `response.url()` (effective URL); eviction = + expired-first then farthest-future; wake jitter (25% capped 2 s). + Tests: `middleware_records_under_the_effective_url`, + `record_evicts_expired_entries_first`, + `record_evicts_the_farthest_deadline_when_none_expired`, + `sleep_wakes_before_the_deadline_within_the_jitter_bound`, + `jitter_is_bounded_by_a_fraction_of_the_remaining_wait`. +- **FWD-09**: `build_client` split into read (async `tokio::fs` for + `reload`, sync for one-shot `new`) + pure `build_client_with_pems`; + `reload` documented and implemented as non-blocking. + +36 client unit tests (was 20) + full-suite green; `cargo clippy +--all-targets -- -D warnings` and `cargo fmt --check` clean; +`cargo test --all-features` green (289 tests). \ No newline at end of file