fix: Retry transient Docker image pull failures - #1734
Conversation
Bound retries to three attempts, preserve caller cancellation, and avoid retrying permanent Docker API responses. Refs testcontainers#1733
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughDocker image pulls now use a bounded retry policy for transient failures. The policy applies exponential backoff with jitter, logs retry details, honors cancellation, and reports sanitized failure reasons. Unit tests cover retry, failure, delay, and cancellation behavior. ChangesDocker image pull retry flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DockerImageOperations
participant DockerImagePullRetryPolicy
participant DockerAPI
participant ILogger
DockerImageOperations->>DockerImagePullRetryPolicy: ExecuteAsync image pull
DockerImagePullRetryPolicy->>DockerAPI: CreateImageAsync
DockerAPI-->>DockerImagePullRetryPolicy: success or transient failure
DockerImagePullRetryPolicy->>ILogger: log retry attempt and reason
DockerImagePullRetryPolicy->>DockerAPI: retry after delayed backoff
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Testcontainers/Clients/DockerImagePullRetryPolicy.cs (2)
64-67: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider inspecting inner exceptions for transport failures.
The checks match only the outermost exception type.
HttpRequestExceptionoften wraps aSocketException, and Docker.DotNet can surface anIOExceptionnested inside another exception. If the transport failure arrives wrapped in a type that is not listed, the pull is not retried.A small traversal of
InnerExceptionwould cover those cases.♻️ Proposed change
- return exception is HttpRequestException - || exception is IOException - || exception is SocketException - || exception is TimeoutException; + for (var current = exception; current != null; current = current.InnerException) + { + if (current is HttpRequestException || current is IOException || current is SocketException || current is TimeoutException) + { + return true; + } + } + + return false;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Testcontainers/Clients/DockerImagePullRetryPolicy.cs` around lines 64 - 67, Update the retry classification logic in DockerImagePullRetryPolicy to traverse each exception’s InnerException chain and return true when any nested exception is an HttpRequestException, IOException, SocketException, or TimeoutException; otherwise preserve the existing non-retry result.
51-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse one consistent comparison style for the status code.
Line 54 calls
Equalson a boxed enum. Line 55 uses==with a cast. Line 56 uses the int value. The already-computedstatusCodeint makes all three checks uniform.♻️ Proposed simplification
if (exception is DockerApiException dockerApiException) { var statusCode = (int)dockerApiException.StatusCode; - return HttpStatusCode.RequestTimeout.Equals(dockerApiException.StatusCode) - || (HttpStatusCode)429 == dockerApiException.StatusCode - || statusCode >= 500 && statusCode <= 599; + return 408 == statusCode + || 429 == statusCode + || (statusCode >= 500 && statusCode <= 599); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Testcontainers/Clients/DockerImagePullRetryPolicy.cs` around lines 51 - 57, Update the status-code checks in the DockerApiException branch of the retry policy to use the existing statusCode integer consistently: compare it with the numeric values for RequestTimeout and TooManyRequests, while preserving the existing 5xx range behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/Testcontainers/Clients/DockerImagePullRetryPolicy.cs`:
- Around line 64-67: Update the retry classification logic in
DockerImagePullRetryPolicy to traverse each exception’s InnerException chain and
return true when any nested exception is an HttpRequestException, IOException,
SocketException, or TimeoutException; otherwise preserve the existing non-retry
result.
- Around line 51-57: Update the status-code checks in the DockerApiException
branch of the retry policy to use the existing statusCode integer consistently:
compare it with the numeric values for RequestTimeout and TooManyRequests, while
preserving the existing 5xx range behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b3c1feb-bfae-4773-a67b-3b6ddc7850a0
📒 Files selected for processing (4)
src/Testcontainers/Clients/DockerImageOperations.cssrc/Testcontainers/Clients/DockerImagePullRetryPolicy.cssrc/Testcontainers/Logging.cstests/Testcontainers.Tests/Unit/Clients/DockerImagePullRetryPolicyTest.cs
1 participant
What does this PR do?
Adds a bounded retry policy around Docker image pulls:
DockerApiExceptionresponses for HTTP 408, 429, and 5xx statuses;The policy's delay functions are isolated internally so its behavior is deterministic and fast to unit test. Docker.DotNet's
DockerApiExceptionexposes the status code and response body, but not response headers, soRetry-Afteris not available at this layer.Why is it important?
A short-lived registry or network failure currently aborts the image pull and the entire test session. Retrying only clearly transient failures reduces CI flakes without masking invalid images, missing manifests, or authentication/authorization failures.
Related issues
How to test this PR
dotnet build src/Testcontainers/Testcontainers.csproj --configuration Release --no-restore(all five target frameworks; zero warnings)dotnet test tests/Testcontainers.Tests/Testcontainers.Tests.csproj --configuration Release --no-restore(566 passed, 1 unrelated skipped)Summary by CodeRabbit
Reliability
Logging
Testing