[BUG] TransientHttpRetryHandler retries non-idempotent POSTs, so a Portainer action can run twice #273

Summary

TransientHttpRetryHandler retries every HTTP method on 408, 429, 5xx, HttpRequestException and TimeoutException, buffering the body so it can be resent. For non-idempotent requests — the Portainer container action (POST /api/endpoints/{id}/docker/containers/{id}/{action}), the Portainer login (POST /api/auth) and the Docker Hub login (POST v2/users/login) — a 5xx/408 answer or a broken connection does not mean the request was not processed. One requested restart/stop can therefore be executed up to RetryCount + 1 times (default 3), and a login can be repeated in a way that trips account lockout or rate limits. Non-idempotent requests must only be retried when the request demonstrably was not processed.

Area

• docs/areas/registries.md (Retry of transient HTTP failures), docs/areas/container-actions.md (Actions, Authentication)
• Security areas (.squad/project.md): Registry and Docker Hub calls (TransientHttpRetryHandler), Container actions (PortainerClient)

Current behavior

src/DockerUpdateGuard/Infrastructure/TransientHttpRetryHandler.cs:181-237:

if (request.Content is not null)
{
    await request.Content.LoadIntoBufferAsync(cancellationToken).ConfigureAwait(false);   // line 186-189: body buffered for resend
}
...
if (IsTransientStatusCode(response.StatusCode) == false || attempt >= retryCount)       // line 201: no method check
...
catch (Exception exception) when (IsTransientException(exception) && ... )            // line 219: any HttpRequestException, also after sending

• IsTransientStatusCode (line 86-91): 408, 429, every status >= 500.
• IsTransientException (line 98-101): any HttpRequestException or TimeoutException — including errors raised after the request was written (e.g. response ended prematurely).
• The handler is attached to the Docker Hub, OCI, .NET/nginx release-feed and Portainer clients (src/DockerUpdateGuard/ServiceCollectionExtensions.cs:72-101).
• POST callers behind it: src/DockerUpdateGuard/Portainer/PortainerClient.cs:154 (login), :336 (container action), src/DockerUpdateGuard/DockerHub/DockerHubClient.cs:969 (Docker Hub login). (Docker Scout's login at DockerScoutVulnerabilityProvider.cs:226 does not use the handler.)
• The existing test TransientHttpRetryHandlerBuffersAndRetriesRequestWithContentAsync (src/Tests/DockerUpdateGuard.Tests/TransientHttpRetryHandlerTests.cs:405) pins the current behavior with a POST .../api/auth.
• Documented as such: docs/areas/registries.md:138 ("Every method is retried, including requests with a body"), docs/areas/container-actions.md:66-67.

Failure scenario: Portainer sits behind a reverse proxy with a 10 s upstream timeout. A restart of a slow-stopping container takes 12 s; the proxy answers 504 while Portainer/Docker carry on. The handler waits ~0.5 s and sends the restart again, then once more after the next 504: the container is restarted three times and the caller finally reports failed.

Expected behavior

Rule (recommended; the Lead may refine it in the plan, but must keep its intent):

• Idempotent methods (GET, HEAD, OPTIONS, TRACE, PUT, DELETE) keep today's retry behavior unchanged.
• Non-idempotent methods (POST, PATCH and any other method) are retried only when the request cannot have been processed:
  1. a transport error raised before the request was sent — HttpRequestException whose HttpRequestError is NameResolutionError, ConnectionError, SecureConnectionError or ProxyTunnelError; and
  2. HTTP 429 Too Many Requests (the server refused the request before processing it), honoring Retry-After as today.
• Non-idempotent requests are not retried on 408, any 5xx, TimeoutException, or any HttpRequestException that may have occurred after sending (ResponseEnded, InvalidResponse, HttpProtocolError, Unknown, …). The response or error is returned to the caller unchanged, so a Portainer action ends failed with the HTTP status / error text after exactly one attempt.
• The retry warning log for a non-retried request is not emitted; optionally a debug entry says the request was not retried because its method is not idempotent.
• The body is buffered only when a retry of that request is possible.

Acceptance criteria

• POST with a 500, 502, 503, 504 or 408 answer is sent exactly once and the response is returned.
• POST failing with HttpRequestException (HttpRequestError.ConnectionError) is retried up to RetryCount times.
• POST failing with HttpRequestException (HttpRequestError.ResponseEnded) or TimeoutException is not retried.
• POST with 429 (with and without Retry-After) is retried as today.
• PATCH behaves like POST; GET, PUT, DELETE behave exactly as before (existing tests unchanged).
• A Portainer action whose action request answers 503 is executed once and reported failed (PortainerClientTests).
• Unit tests (MSTest, naming per docs/UNIT_TESTS.md), e.g. TransientHttpRetryHandlerDoesNotRetryPostOnServerErrorAsync, TransientHttpRetryHandlerRetriesPostOnConnectionErrorAsync, TransientHttpRetryHandlerDoesNotRetryPostOnResponseEndedAsync, TransientHttpRetryHandlerRetriesPostOnTooManyRequestsAsync; the existing TransientHttpRetryHandlerBuffersAndRetriesRequestWithContentAsync is rewritten to the new rule (e.g. a POST with body retried on a connection error, with the body resent intact).
• docs/areas/registries.md (Retry of transient HTTP failures) and docs/areas/container-actions.md (Actions) describe the new rule; the Known gaps entries for this issue (currently registries.md Open questions item 3 and container-actions.md item 3) are removed. docs/ARCHITECTURE.md is updated where it describes the retry policy.

Implementation notes

• HttpRequestException.HttpRequestError is available on the project's target framework (net10.0); SocketsHttpHandler sets ConnectionError/NameResolutionError/SecureConnectionError for failures while establishing the connection, i.e. before any request byte was sent. Treat Unknown conservatively as "may have been sent".
• Options considered:
  1. Recommended – method-based rule inside TransientHttpRetryHandler as above: one place, covers every current and future POST.
  2. Per-request opt-out via HttpRequestOptions set by the callers: more flexible but easy to forget on a new POST.
  3. Excluding only Portainer from the handler: would also lose the safe retries of its GETs and leave the Docker Hub login problem.
• The Docker Hub login (DockerHubClient.cs:969) and Portainer login are effectively safe to repeat, but they are covered by the same rule on purpose (lockout/rate limiting); no exception list.
• Scanning:RetryCount semantics, backoff, jitter, Retry-After handling and the total request timeouts stay unchanged. Retry log entries keep omitting the query string (GetRequestTarget).
• No configuration, README or migration impact (unless README describes the retry policy — then update it).

Related

• F-009 (P1) — DockerHub access token cache tied to scope lifetime #25, F-045 (P6) — Two DockerHubClient instances per scope → duplicated token cache (amplifies F-009) #6 (Docker Hub token caching; the login retried here is part of that flow)
• Portainer container actions feature (UI trigger and action runs) — filed separately; it depends on this fix so a requested action is executed at most once.
• PR [Docs] Describe the current behavior by area #263 (area documents)
• [Feature] Trigger Portainer container actions from the runtime container page and record them as action runs #280 (Portainer container actions feature)

Generated by Claude Code