Floor the Go retry cap at one attempt so WithMaxRetries(0) still sends the request

Merged
JE
2mo ago
+169-19

The failure

WithMaxRetries(0) on the Go client never sent a request. doRequestURL's loop is for attempt = 1; attempt <= MaxRetries, a pre-check loop, so a cap of 0 ran it zero times and every GET returned

request failed after 0 retries: %!w(<nil>)

— no request on the wire, and the wrapped error was nil. NewClient only rejects a negative cap, so 0 was accepted and then silently did nothing. (Noticed while porting the URL-redaction fix in #175.)

The contract

MaxRetries is the total attempt count, floored at one. That is how every other Fizzy SDK with a numeric cap already counts it — Kotlin's attempt < maxAttempts, Rust's max_attempts(n).max(1) (whose doc says it counts "as the Go client counts its MaxRetries"), the behavior model's retry.max: 1 for single-attempt operations — and it is the settled basecamp-sdk contract (SPEC §2 validation step 4, basecamp-sdk#718): a cap of 0 is the spelling for "no retries — exactly one attempt", not a request to send nothing.

The change

  • doRequestURL runs on maxAttempts := max(MaxRetries, 1). The floor lives in the loop rather than only at construction because the loop is pre-check and a directly-struct-built Client never passes through NewClient's validation.
  • After the final attempt there is no retry, so the loop no longer sleeps the backoff or fires OnRetry announcing an attempt that will never be made (before: WithMaxRetries(2) on a 503 stream fired OnRetry for attempts 2 and 3, and slept once more before returning).
  • The final-attempt check runs before any delay is resolved, and backoffDelay returns the bare exponential term when MaxJitter <= 0: WithMaxJitter(0) is an accepted option and rand.Int63n(0) panics, so a zero-jitter client used to panic on every retry.
  • The exhausted-budget error counts attempts, matching what it wraps: request failed after 2 attempts: Gateway error (503).
  • HTTPOptions.MaxRetries and WithMaxRetries docs say what the number counts.

Tests

client_retry_zero_cap_test.go pins, against an httptest server counting hits:

  • WithMaxRetries(0) → exactly one request and the server's real response (fails on main with the wrapped-nil error above);
  • WithMaxRetries(0) with WithMaxJitter(0) on a 503 → exactly one request, the 503 *Error in the chain, after 1 attempt: in the message, no OnRetry, no panic;
  • backoffDelay with zero jitter is the deterministic exponential series;
  • WithMaxRetries(2) on 503s → exactly two requests, after 2 attempts:, OnRetry announcing attempt 2 only.

Gates: make go-check, Go conformance runner 127/127. No fixture added: the shared suite's configOverrides has no retry-cap knob and the other runners would not honor one, so a zero-cap fixture would fail there rather than pass vacuously; that is a suite-wide addition, not this PR's.

Sibling SDKs

Checked for the same off-by-one: basecamp-sdk Go already floors both loops at one (max(MaxRetries, 1), #718, with its own zero-cap tests and conformance fixtures); hey-sdk Go budgets attempts = MaxRetries + 1 on the hand-written path and max(min(op.max-1, MaxRetries), 0) + 1 on the per-operation policy path, so 0 is one attempt there too. Neither needs a change.

Out of scope here: the Fizzy Ruby SDK's request_with_retry has the same pre-check shape (break if attempt > max_retries) and raises Request failed after 0 retries without sending on max_retries: 0.