fix(httpclient): retry timed-out requests, stop replaying POST and PATCH

[vdekrijger]

Fixes #156. The reporter's diagnosis is correct on both counts, and this takes the per-attempt-context direction they proposed.

1. A timed-out request could never be retried

Timeout: 30s sat on the http.Client while the retry loop lived in its Transport. Client.Timeout bounds the whole Do() — every attempt and every backoff — so a request that failed by timing out had already spent the budget. The first backoff returned instantly with an expired context and the loop died at (attempt: 1) sleeping: context deadline exceeded.

flowchart LR
  subgraph before["Before — one 30s budget for the whole loop"]
    B1["attempt 1<br/>burns all 30s"] --> B2["backoff<br/>context already expired"] --> B3["give up"]
  end
  subgraph after["After — 30s per attempt"]
    A1["attempt 1 · own 30s"] --> A2["backoff<br/>caller's context, alive"] --> A3["attempt 2 · own 30s"] --> A4["… up to 4"]
  end

Loading

The transport now owns the deadline (AttemptTimeout, 30s); the client carries none.

The part worth reviewing closely: that deadline has to cover reading the response body, which happens after RoundTrip returns. Cancelling when RoundTrip returns — as the issue's sketch does — kills the body for every successful request. The cancel func is handed to the body's Close() instead. TestRetryTransport_ResponseBodyOutlivesRoundTrip fails with context canceled against the naive placement.

2. Retries replayed POST and PATCH, duplicating resources

The decision read the error or status alone, with no method check, while client.go sets req.GetBody. A create PostHog committed but failed to acknowledge was sent again, leaving a resource Terraform never records and will not clean up. The rule now:

• 429 — replay any method; it was rejected before processing.
• Idempotent method (GET HEAD PUT DELETE OPTIONS TRACE) or an Idempotency-Key header — replay.
• Failed dial — replay any method; nothing was written.
• Anything else on POST/PATCH — surface the error rather than risk a duplicate.

On net/http already doing this

Raised in the issue's third comment. Request.isReplayable, nothingWrittenError and shouldRetryRequest are unexported and sit below this middleware, so the rule cannot literally be reused — but its Idempotency-Key handling now is.

The useful half: shouldRetryRequest checks "nothing written" before the method, so the transport already replays a POST on a dead pooled connection when GetBody is set. Verified by closing a pooled connection under it — one extra dial, server saw the POST once. The layers do not overlap: it refuses fresh-connection failures, which is exactly where our dial case starts.

One correction for the record — the comment gives the stdlib's replayable set as including PUT/DELETE; it does not (GET HEAD OPTIONS TRACE). We are deliberately wider, per RFC 9110, because unlike the transport's invisible connection-level retry ours is explicit and logged.

Also fixed, and why they belong here

WithNoRetry/WithRetryConfig stacked rather than replaced the retry transport, so WithNoRetry did not disable retries — and removing the redundant MaxRetries == 0 passthrough made that stacked pair reproduce this issue's own shape. NewRetryTransport now fills every tunable whose zero is not a meaningful choice. Retry and give-up decisions log at debug level with a reason.

Not included

Provider-schema knobs (request_timeout, max_retries) — a workaround for the bug being fixed. Full httptrace byte-tracking. Retry-After is still clamped to MaxBackoff (pre-existing), and there is no total request budget: the ~2 minute per-request ceiling is in the upgrade note, and it is spent again per page and per redirect hop.

Test plan

go test -cover ./internal/... green over repeated consecutive runs and under -race; golangci-lint reports 0 issues. Middleware coverage 91% → 96.9%.

Both defects were reproduced before being fixed: the pre-fix shape gives the reporter's exact symptom, and reverting the method rule fails all six duplicate-producing cases. TestRetryTransport_WithBody moved from POST to PUT — it was the test asserting the buggy behaviour.

Two pre-existing flaky timing assertions were rewritten as floor-plus-ceiling (TestSleep/completes_after_duration failed 12 of 20 runs on main); they were blocking the local gate.