Honor each route's retry policy in the Rust client

Merged
JE
2mo ago
+623-49

Card: hey-sdk Rust: honor per-operation retry policy

What changes

The generator has always written every route's policy into the route table (routes::LIST_BOXES.retry = Retry { max: 3, base_delay_ms: 1000, retry_on: &[429, 503] }, from behavior-model.json), and the client never read it: attempt() ran on the client-wide max_retries, gated only by idempotency, and resent on a fixed [429, 500, 502, 503, 504]. Go has honoured the policy since #94, with a Go-only fixture.

  • Operation carries its &'static Route; Page carries it to every page read after the first, so next_page/each_page resend under the first page's policy rather than as anonymous requests.
  • attempt() spends what the policy allows. max is Smithy's maxAttempts — the sends in all — so max: 3 is three requests, max: 2 (DeleteBoxDesignation and 61 others) is two.
  • The client's settings only make that gentler: max_retries is a ceiling on the sends (max_retries(1) on a max: 3 route → 2 requests; max_retries(10) → still 3), base_delay a floor under the first wait, max_delay a ceiling over every wait. base_delay becomes Option internally so a client that never set one starts from the route's base_delay_ms; the public builder is unchanged.
  • retry_on replaces the fixed status list for modelled routes: a 500 on ListBoxes is the answer, not a retry. RequestResult::retryable (what hooks see) now says whether the policy would resend the status.
  • A route the model gives no policy (retry: { max: 0 } — the four non-idempotent POSTs) is sent once, whatever the client allows. A path the caller wrote (client.get("/custom"), form requests) has no policy and runs on the client's settings as before, including the old status list.
  • Retry-After on a 429 is honoured as given (seconds or HTTP-date, rounded up per #151) when the policy names 429; a 429 on a route whose policy does not name it is not waited out.
  • conformance/tests/go/retry-policy.json moves to conformance/tests/retry-policy.json and the go/ directory is gone; the Go runner loses its private glob. The Rust runner already had the DeleteBoxDesignation arm and the retryable assertion; at the generated layer it now carries a repeatOperation case past a run that failed with an HTTP status (the Go runner's generated client counts such a run as answered, not failed), which is what the case's first 500 needs. The HEY layer stops at every failure, as Go's does.
  • max_delay is now the ceiling it is documented as: it is no longer lifted to a larger base_delay at build time, and the jitter goes under it too. The wait a Retry-After names is still honoured as given.

Second commit is a one-line rewrite of the cache-key check that clippy 1.97 flags (nonminimal_bool); it was blocking make rs-lint locally and is unrelated.

Go

Go's hey.Client passes WithRetryConfig, which replaces every operation's attempt count with MaxRetries + 1 and never reads baseDelayMs; the shared fixture passes there only because the runner drives the raw generated client. This PR is Rust-only. A separate PR is making the Go client honour the per-operation policy with the client-wide config as the same ceiling, and will split the shared fixture into status-list and attempt-count cases; the fixture's move out of conformance/tests/go/ lands here first.

Gate

On the branch as pushed (rebased onto main after #153):

  • make rs-check (fmt-check, clippy --all-targets -D warnings, cargo test --workspace, doc -D warnings): green
  • make rs-check-drift: green
  • make conformance-rs: 191/191
  • make conformance-go: 191/191 (the moved fixture runs from the shared directory)

New tests: rust/hey-sdk/tests/retry_policy.rs (15 cases from literal wiremock bodies — policy count, ceiling, per-route count, status outside the policy, raw path on client defaults, 429 not named by the policy, no-policy route, non-idempotent, policy delay with client floor, max_delay over the policy's wait and over the client's own floor and jitter, Retry-After kept above the ceiling, Retry-After HTTP-date on a modelled route, later pages under the first page's policy with a route budget told from a raw one, route-table pin), plus client_pipeline::resending_stops_at_the_retry_limit corrected from 4 to the model's 3.

Adversarial review

Besides Copilot and the Codex GitHub reviewer, the diff was reviewed by Codex CLI (gpt-6-astra, reasoning xhigh). Findings and what was done:

  • max_delay lifted to base_delay at build time, jitter added above the cap — fixed (also Copilot's).
  • Runner continuation applied to HEY-layer cases too — fixed, scoped to the generated layer (also Copilot's).
  • max_retries + 1 overflows for u32::MAX on the path that used to skip it — fixed with saturating_add.
  • The pagination test passed with route propagation removed (3 sends fit both budgets) — fixed: page two now answers an undeclared 500 under max_retries(10), which a raw budget would resend.
  • Timing tests with tight upper bounds and an HTTP-date computed before mock setup — fixed: the policy delay is 1.5 s so the bounds are wide, the date is computed after the mocks are mounted.
  • README overstated "sends in all": the ordinary budget is min(max, max_retries + 1), the refresh resend is granted past a spent budget, Retry-After: 0 falls back to backoff — fixed in the README.
  • follow_pagination builds later pages with Operation::at and so loses the policy — declined: a Response names no route, giving it one means a new field on a public all-pub struct, and no service uses follow_pagination (only get_all-style raw walks do). Documented on the method and in the README; next_page/each_page carry the policy.
  • The shared fixture cannot tell the correct policy from retry_on: [500] (both give three requests and a final 503) — declined here: per-invocation assertions need a fixture-format change, and the fixture split is being done with the Go change.