feat(client): add option to retry only idempotent requests

What

retryHTTPCheck retries a request on any 5xx (and on status 0) without checking the HTTP method. A non-idempotent request such as a POST is therefore retried, which can duplicate a side effect the server already applied. GitLab can, for example, create a note and still return a 5xx; the retry then posts a second note.

Following review, the fix is opt-in: the default behavior is unchanged, and a new WithOnlyIdempotentRetries client option enables the new behavior. The maintainers plan to make it the default in the 3.0 release.

Change

A new client option, WithOnlyIdempotentRetries, restricts automatic retries to idempotent methods. With the option set:

  • The 5xx / status-0 branch only retries an idempotent method, so POST and PATCH are not retried. A nil Request keeps the previous behavior for callers that synthesize a response without one (for example the status-0 AWS ALB case).
  • The network-error branch uses the same idempotent method set in place of its legacy inline list.

The method set follows RFC 9110 §9.2.2 (idempotent methods: GET, HEAD, OPTIONS, TRACE, PUT, DELETE) plus QUERY, which RFC 10008 defines as safe and idempotent. net/http has no QUERY constant yet, so it is a string literal.

429 Too Many Requests is still retried for every method, with or without the option. A rate-limited request is rejected before the server processes it, so retrying it cannot duplicate a side effect.

Without the option, nothing changes: the 5xx / status-0 branch retries any method, and the network-error branch keeps its previous inline method list (including CONNECT, and excluding PUT and DELETE).

Tests

The by-method retry tests run each case twice, once per mode:

  • TestClient_DefaultRetryPolicy_RetryOn5xxStatus_ByMethod: a 5xx is retried for every method by default; with the option it is retried for idempotent methods and not for POST, PATCH, CONNECT.
  • TestClient_DefaultRetryPolicy_RetryOnIdempotentRequests_ByMethod (network errors): the default keeps the legacy method list; the option applies the idempotent set.
  • TestClient_DefaultRetryPolicy_RetryOnTooManyRequests_AnyMethod: a 429 is retried for GET, POST, and PATCH even with the option set.
  • TestMarkdownUploads_UploadProjectMarkdown_NoRetryOn5xx: with the option, an upload (a POST) is sent once and the error surfaces without a retry.
Edited by Patrick Dawkins

Merge request reports

Loading