Add HTTP retry support - #2
Merged
Merged
Conversation
Renamed RetryPolicyOptions.RequestTimeout to AttemptTimeout throughout code and XML docs. Updated DefaultRetryExecutor to use AttemptTimeout for per-attempt timeouts. Refactored and moved tests to DefaultRetryExecutorExecuteAsyncTests.cs, updating method names and assertions to validate AttemptTimeout behavior. Ensured all exception handling and timeout logic references the new property.
Renamed all namespaces from SimpleRetryTools to SimpleRetry for consistency. Updated README.md with introduction, features, configuration, usage examples, backoff strategies, and DI integration. Revised comments and using directives in source and test files to reflect the new namespace.
Implemented StandardResilienceHandler for HTTP retries on transient failures, supporting Retry-After headers, attempt timeouts, and request cloning. Introduced AddHttpSimpleRetry extension methods for IHttpClientBuilder to register SimpleRetry as a delegating handler, with flexible configuration options. Updated Program.cs to use AddHttpSimpleRetry. Removed Polly.Core and Microsoft.Extensions.Http.Resilience dependencies, added Microsoft.Extensions.Http. Refactored ServiceCollectionExtensions for unified AddSimpleRetry logic. Added comprehensive unit tests for retry scenarios and configuration. Improved DefaultRetryExecutor with explicit TimeSpan pattern matching for timeouts.
Renamed sample project from WebApplication1 to SimpleRetrySample and updated all related files and solution references. Refactored DefaultRetryExecutor to use a new RetryOutcome struct, enabling retries based on both exceptions and result values. Updated OnRetryArguments and RetryPolicyOptions.ShouldHandle to work with RetryOutcome, providing richer context and more flexible retry conditions. Enhanced sample Program.cs to demonstrate advanced ShouldHandle logic, including retries for specific exceptions and failed HTTP responses.
Resolve conflicts by combining the per-attempt timeout with the outcome-based retry model. ShouldHandle now receives a RetryOutcome, so StandardResilienceHandler delegates transient status code detection to the policy predicate instead of checking responses manually.
Refactored HTTP retry logic from StandardResilienceHandler into a new HttpRetryDelegatingHandler, encapsulating HTTP-specific retry behavior, request cloning, transient error detection, and Retry-After handling. Updated ServiceCollectionExtensions to register the new handler and wire up HTTP-specific retry options. Enhanced RetryPolicyOptions with RetryDelayGenerator and OnResultDiscarded properties. Improved DefaultRetryExecutor to use linked cancellation tokens for attempt timeouts and custom delay generators. Added/updated unit tests for HTTP retry scenarios and attempt timeout handling. Removed obsolete StandardResilienceHandler and related tests. Made minor code style and API consistency improvements.
Expanded README to cover retrying on results, custom delay logic, and result disposal via `OnResultDiscarded`. Updated `ShouldHandle` to use `RetryOutcome` and added usage examples. Enhanced HTTP retry docs with default behaviors and configuration guidance. Adjusted tests to use `List<Exception?>` for nullability support.
Updated ShouldRetry to use a null-conditional operator when invoking the ShouldHandle delegate from RetryPolicyOptions. Now defaults to retry (returns true) if ShouldHandle is not provided, preventing NullReferenceException and ensuring safer default behavior.
The constructor of RetryOutcome now takes result first, exception second. Updated all factory methods to match this order, ensuring consistent and clear instance creation.
Removed XML documentation inheritance from ExecuteAsync and ExecuteAsync<T> in DefaultRetryExecutor. No functional changes were made.
Project now targets net8.0, net9.0, and net10.0. Set <LangVersion> to latest for newest C# features. Added conditional package references for Microsoft.Extensions.DependencyInjection, Microsoft.Extensions.Http, and Microsoft.Extensions.Logging.Abstractions for net8.0/net9.0. Removed SimpleAuthentication.Abstractions.xml from build output. Minor formatting tweak in <InternalsVisibleTo> (no functional change).
Removed non-generic ExecuteOperationAsync and unified timeout handling in the generic method. All timeout and cancellation logic now resides in ExecuteOperationAsync<T>, ensuring consistent behavior. Updated comments to clarify intent and mechanics.
The test WhenHandledExceptionIsThrownThenRetriesOperation now sets MaxRetryCount to 3, allowing up to three retry attempts instead of one. This change better exercises the retry mechanism.
Added Directory.Build.props to centralize build settings, enable CI flags, embed debug info, and configure repository/source publishing. Integrated Nerdbank.GitVersioning via version.json for version management. Removed direct NSubstitute and xunit.runner.visualstudio references from test project to streamline dependencies.
The retry handler now supports buffering request content and cloning the HttpRequestMessage per attempt. By default, it resends the same request instance, but can be configured to clone requests to prevent mutation leaks. Retries with StreamContent require buffering; otherwise, an InvalidOperationException is thrown. The handler ensures content is not disposed between attempts. Updated README and tests cover these behaviors, including content buffering, request cloning, and content disposal scenarios.
- Updated HttpRetryDelegatingHandler.ShouldHandle to use C# 14 pattern matching: replaced (int)response.StatusCode >= 500 with >= HttpStatusCode.InternalServerError for clarity and type safety. - Added System.Diagnostics.CodeAnalysis using to RetryOutcome.cs. - Annotated RetryOutcome.IsException with [MemberNotNullWhen] to enhance nullability analysis.
Changed AddRetryExecutor from an expression-bodied to a block-bodied method, enclosing the existing logic in curly braces without altering its behavior.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate issues affect compilation, registration, compatibility, cleanup, replayability, documentation, coverage, and test coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds outcome-aware HTTP retry support with timeouts, DI integration, samples, tests, documentation, and coverage tooling.
Changes:
- Adds HTTP retry handling, backoff, outcomes, and cleanup callbacks.
- Multi-targets .NET 8–10 and expands retry configuration.
- Adds samples, testing guidance, and coverage scripts.
File summaries
| File | Review summary |
|---|---|
tests/SimpleRetry.UnitTests/SimpleRetry.UnitTests.csproj |
Configures unit tests. moderate (1 vote): targets only .NET 10, leaving the .NET 8 compatibility branch untested. |
tests/SimpleRetry.UnitTests/ServiceCollectionExtensionsTests.cs |
Tests DI and HTTP registrations. |
src/SimpleRetry/version.json |
Updates package versioning. |
src/SimpleRetry/SimpleRetry.csproj |
Multi-targets .NET 8–10 and adds dependencies. |
src/SimpleRetry/ServiceCollectionExtensions.cs |
Adds HTTP retry registration. critical (3 votes): bufferRequestContent is not publicly configurable, so clients using StreamContent cannot follow the documented fix. |
src/SimpleRetry/RetryTimeoutException.cs |
Adds the timeout exception type. |
src/SimpleRetry/RetryPolicyOptions.cs |
Expands retry configuration. moderate (3 votes): changing ShouldHandle is source-breaking for package version 3.3 consumers. |
src/SimpleRetry/RetryOutcome.cs |
Adds the result/exception outcome model. |
src/SimpleRetry/OnRetryArguments.cs |
Updates callback arguments. moderate (3 votes): removing Exception breaks existing callbacks. |
src/SimpleRetry/HttpRetryDelegatingHandler.cs |
Adds HTTP retry handling. critical (1 vote): the HttpStatusCode relational pattern does not compile. moderate (2 votes): nested non-seekable multipart content can be replayed incorrectly. |
src/SimpleRetry/DefaultRetryExecutor.cs |
Adds outcome and timeout execution. moderate (3 votes): callback failures can bypass disposal of discarded results. |
src/Directory.Build.props |
Adds shared build and version settings. |
SimpleRetry.slnx |
Adds sample and test projects. |
samples/SimpleRetrySample/SimpleRetrySample.csproj |
Adds sample project configuration. |
samples/SimpleRetrySample/Properties/launchSettings.json |
Adds launch profiles. |
samples/SimpleRetrySample/Program.cs |
Demonstrates retry APIs. critical (2 votes): registers System.String as a typed client even though it lacks an HttpClient constructor. |
samples/SimpleRetrySample/appsettings.json |
Adds sample logging configuration. |
samples/SimpleRetrySample/appsettings.Development.json |
Adds development logging configuration. |
README.md |
Documents retry and HTTP APIs. moderate (1 vote): the returned-value example uses an exception-only policy and omits the required result-disposal callback. |
global.json |
Selects the Microsoft Testing Platform. |
.gitignore |
Updates generated-file exclusions. |
.github/skills/test-anti-patterns/skill.md |
Adds test anti-pattern guidance. |
.github/skills/run-tests/skill.md |
Adds test-running guidance. |
.github/skills/run-tests/references/platform-detection.md |
Documents platform detection. |
.github/skills/run-tests/references/filter-syntax.md |
Documents test filtering. |
.github/skills/coverage-analysis/scripts/Extract-MethodCoverage.ps1 |
Adds method coverage extraction. moderate (2 votes): branch counts can double-count across reports. moderate (1 vote): single-item JSON output becomes an object instead of an array. |
.github/skills/coverage-analysis/scripts/Compute-CrapScores.ps1 |
Adds CRAP-score calculation. moderate (3 votes): line aggregates report an average rather than a merged union. moderate (1 vote): branch aggregates can double-count overlaps and inflate coverage. |
.github/skills/coverage-analysis/references/output-format.md |
Defines coverage output format. |
.github/skills/coverage-analysis/references/guidelines.md |
Adds coverage-analysis guidance. |
.github/skills/code-testing-extensions/skill.md |
Adds the testing extension catalog. |
.github/skills/code-testing-extensions/extensions/typescript.md |
Adds TypeScript testing guidance. |
.github/skills/code-testing-extensions/extensions/swift.md |
Adds Swift testing guidance. |
.github/skills/code-testing-extensions/extensions/rust.md |
Adds Rust testing guidance. |
.github/skills/code-testing-extensions/extensions/rust-examples.md |
Adds Rust examples. |
.github/skills/code-testing-extensions/extensions/ruby-examples.md |
Adds Ruby examples. |
.github/skills/code-testing-extensions/extensions/python.md |
Adds Python testing guidance. |
.github/skills/code-testing-extensions/extensions/powershell.md |
Adds PowerShell testing guidance. |
.github/skills/code-testing-extensions/extensions/powershell-examples.md |
Adds PowerShell examples. |
.github/skills/code-testing-extensions/extensions/kotlin.md |
Adds Kotlin testing guidance. |
.github/skills/code-testing-extensions/extensions/kotlin-examples.md |
Adds Kotlin examples. |
.github/skills/code-testing-extensions/extensions/java.md |
Adds Java testing guidance. |
.github/skills/code-testing-extensions/extensions/go.md |
Adds Go testing guidance. |
.github/skills/code-testing-extensions/extensions/dotnet.md |
Adds .NET testing guidance. |
.github/skills/code-testing-extensions/extensions/dotnet-examples.md |
Adds .NET examples. |
.github/skills/code-testing-extensions/extensions/cpp-examples.md |
Adds C++ examples. |
.github/skills/code-testing-agent/unit-test-generation.prompt.md |
Adds the unit-test generation prompt. |
.github/skills/code-testing-agent/skill.md |
Adds the test-generation workflow. |
.github/skills/code-testing-agent/extensions/dotnet.md |
Adds .NET agent guidance. |
Review details
Suppressed comments (5)
.github/skills/coverage-analysis/scripts/Compute-CrapScores.ps1:59
- The branch aggregate is subject to the same double-counting problem as the line aggregate: overlapping branches in multiple reports are added together, so the reported overall branch rate can be inflated. Use a branch union keyed by source/line/branch identity or clearly report an average rather than an aggregate union.
if ($null -ne $cobertura.coverage.'branches-covered' -and $null -ne $cobertura.coverage.'branches-valid') {
$totalBranchesCovered += [double]$cobertura.coverage.'branches-covered'
$totalBranchesValid += [double]$cobertura.coverage.'branches-valid'
} elseif ($cobertura.coverage.'branch-rate') {
$fallbackBranchRates.Add([double]$cobertura.coverage.'branch-rate')
.github/skills/coverage-analysis/scripts/Extract-MethodCoverage.ps1:185
- The script documents a JSON array result, but piping
@($sorted)intoConvertTo-Jsonunwraps a single-item collection and emits a JSON object instead. A caller receiving exactly one filtered method therefore gets a different shape; pass the array as-InputObjectto preserve the contract.
$json = @($sorted) | ConvertTo-Json
README.md:121
- This endpoint resolves the
ExternalApipolicy configured above, whoseShouldHandleonly matchesHttpRequestException. A non-successHttpResponseMessagereturned here therefore is not retried, despite this section claiming returned-value retries; show a response-aware policy (and its result-disposal callback) for this example.
samples/SimpleRetrySample/Program.cs:35 - This registers
System.Stringas a typed client, butstringhas no constructor acceptingHttpClient; resolving the typed client will fail in the typed-client factory. Use a named client or a real typed-client class so the sample's HTTP registration is usable.
tests/SimpleRetry.UnitTests/SimpleRetry.UnitTests.csproj:6 - The production project now targets net8.0, net9.0, and net10.0, but this test project runs only on net10.0. That leaves the net8-specific
LoadIntoBufferAsync/WaitAsyncbranch inHttpRetryDelegatingHandleruntested; multi-target the test project (or add an equivalent test matrix) so the compatibility path is exercised.
- Files reviewed: 53/58 changed files
- Comments generated: 8
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| internal static bool ShouldHandle(RetryOutcome outcome) => outcome switch | ||
| { | ||
| { Exception: HttpRequestException or RetryTimeoutException } => true, | ||
| { Result: HttpResponseMessage response } => response.StatusCode is HttpStatusCode.RequestTimeout or HttpStatusCode.TooManyRequests or >= HttpStatusCode.InternalServerError, |
|
|
||
| AddRetryExecutor(builder.Services); | ||
|
|
||
| builder.AddHttpMessageHandler(services => new HttpRetryDelegatingHandler(services.GetRequiredKeyedService<IRetryExecutor>(builder.Name))); |
Comment on lines
+48
to
+51
| # Prefer aggregate numerator/denominator attributes when present. | ||
| if ($null -ne $cobertura.coverage.'lines-covered' -and $null -ne $cobertura.coverage.'lines-valid') { | ||
| $totalLinesCovered += [double]$cobertura.coverage.'lines-covered' | ||
| $totalLinesValid += [double]$cobertura.coverage.'lines-valid' |
Comment on lines
+126
to
+128
| $mergedTotal = [Math]::Max($existingTotal, $total) | ||
| $mergedCovered = [Math]::Min($existingCovered + $covered, $mergedTotal) | ||
| $methodMap[$key].BranchData[$lineNo] = @{ Covered = $mergedCovered; Total = $mergedTotal } |
Comment on lines
+104
to
+119
| private async Task WaitForNextAttemptAsync(RetryOutcome outcome, int attempt, CancellationToken cancellationToken) | ||
| { | ||
| var retryDelay = options.RetryDelayGenerator?.Invoke(outcome) ?? GetRetryDelay(attempt); | ||
|
|
||
| if (options.OnRetry is not null) | ||
| { | ||
| await options.OnRetry(new(attempt, options.MaxRetryCount, retryDelay, outcome, serviceProvider, loggerFactory)).ConfigureAwait(false); | ||
| } | ||
|
|
||
| if (!outcome.IsException) | ||
| { | ||
| options.OnResultDiscarded?.Invoke(outcome); | ||
| } | ||
|
|
||
| await Task.Delay(retryDelay, cancellationToken).ConfigureAwait(false); | ||
| } |
Comment on lines
+39
to
+43
| if (request.Content is StreamContent content) | ||
| { | ||
| if (!bufferRequestContent) | ||
| { | ||
| throw new InvalidOperationException($"{nameof(StreamContent)} content cannot be cloned, because its source stream is consumed by the first attempt and may not be seekable. Enable request content buffering to retry requests with this kind of content."); |
Comment on lines
+32
to
+34
| /// Gets the outcome that caused the retry, which can be either an exception or a handled result. | ||
| /// </summary> | ||
| public Exception Exception { get; } = exception; | ||
| public RetryOutcome Outcome { get; } = outcome; |
| /// || (outcome.TryGetResult(out HttpResponseMessage? response) && !response.IsSuccessStatusCode); | ||
| /// </code> | ||
| /// </example> | ||
| public Func<RetryOutcome, bool> ShouldHandle { get; set; } = static outcome => outcome.IsException; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.