diff --git a/CHANGELOG.md b/CHANGELOG.md index 04ce27e..b4a9a3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,14 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [vNext] + +### Added +- Add `MaximumRetryAfterDelay` option to `ODataClientOptions` (default 30 seconds). A server-supplied `Retry-After` header is now honoured in preference to `RetryDelay`, bounded by this value so that a large or malformed header cannot stall the caller. Set to `TimeSpan.Zero` to ignore `Retry-After` entirely and always use `RetryDelay` + +### Fixed +- Retry HTTP 408 (Request Timeout) and 429 (Too Many Requests) alongside 5xx. Previously any status below 500 was returned to the caller immediately, so a 408 from an intervening proxy or a 429 from a rate limiter was never retried. Both are cases where the server rejected the request without processing it, so retrying is safe even for methods that are not idempotent. Other 4xx statuses remain non-retryable, in particular 409, which is a routine "already exists" outcome for callers that create-or-overwrite + ## [10.0.106] - 2026-07-08 ### Fixed diff --git a/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryStatusCodeTests.cs b/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryStatusCodeTests.cs new file mode 100644 index 0000000..88781f9 --- /dev/null +++ b/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryStatusCodeTests.cs @@ -0,0 +1,273 @@ +using System.Diagnostics; + +namespace PanoramicData.OData.Client.Test.UnitTests; + +/// +/// Tests for which HTTP status codes are treated as transient and retried, and for how long the client +/// waits between attempts. +/// +/// +/// 408 and 429 were previously not retried: the client returned immediately for anything below 500. Both +/// are cases where the server rejected the request without processing it, so retrying is safe even for +/// methods that are not idempotent, and a caller behind a proxy that emits 408 had no recourse. +/// +/// The negative cases matter at least as much as the positive ones. A 409 is a routine "already exists" +/// outcome for callers that create-or-overwrite, and retrying it would be actively wrong. +/// +public class ODataClientRetryStatusCodeTests : TestBase +{ + private const string ProductsJson = """ + { + "@odata.context": "https://test.odata.org/$metadata#Products", + "value": [ { "ID": 1, "Name": "Widget", "Price": 9.99 } ] + } + """; + + private static HttpResponseMessage Ok() + => new(HttpStatusCode.OK) { Content = new StringContent(ProductsJson, Encoding.UTF8, "application/json") }; + + private static ODataQueryBuilder ProductsQuery() => new("Products", NullLogger.Instance); + + /// + /// Builds a client whose transport returns the given statuses in order, then 200 for every later call, + /// recording how many requests were actually attempted. + /// + private static (ODataClient Client, HttpClient HttpClient, Func AttemptCount) CreateClient( + HttpStatusCode[] failuresThenSuccess, + TimeSpan? retryDelay = null, + TimeSpan? retryAfter = null, + TimeSpan? maximumRetryAfterDelay = null) + { + var attempts = 0; + + var handler = new MockHttpMessageHandler(_ => + { + var index = attempts++; + + if (index >= failuresThenSuccess.Length) + { + return Ok(); + } + + var response = new HttpResponseMessage(failuresThenSuccess[index]) + { + Content = new StringContent("{}", Encoding.UTF8, "application/json") + }; + + if (retryAfter is { } delta) + { + response.Headers.RetryAfter = new System.Net.Http.Headers.RetryConditionHeaderValue(delta); + } + + return response; + }); + + var httpClient = new HttpClient(handler) { BaseAddress = new Uri("https://test.odata.org/") }; + + var options = new ODataClientOptions + { + BaseUrl = "https://test.odata.org/", + HttpClient = httpClient, + Logger = NullLogger.Instance, + RetryCount = 3, + RetryDelay = retryDelay ?? TimeSpan.FromMilliseconds(1) + }; + + if (maximumRetryAfterDelay is { } maximum) + { + options.MaximumRetryAfterDelay = maximum; + } + + return (new ODataClient(options), httpClient, () => attempts); + } + + /// + /// A transient status is retried and the call then succeeds. + /// + [Theory] + [InlineData(HttpStatusCode.RequestTimeout)] // 408 - the case that prompted this + [InlineData(HttpStatusCode.TooManyRequests)] // 429 + [InlineData(HttpStatusCode.InternalServerError)] // 500 - must not regress + [InlineData(HttpStatusCode.BadGateway)] // 502 + [InlineData(HttpStatusCode.ServiceUnavailable)] // 503 + [InlineData(HttpStatusCode.GatewayTimeout)] // 504 + public async Task TransientStatus_IsRetried_AndSucceeds(HttpStatusCode transientStatus) + { + var (client, httpClient, attemptCount) = CreateClient([transientStatus]); + + try + { + var response = await client.GetAsync(ProductsQuery(), TestContext.Current.CancellationToken); + + response.Value.Should().ContainSingle("the retry should have succeeded on the second attempt"); + attemptCount().Should().Be(2, "one failure then one success"); + } + finally + { + client.Dispose(); + httpClient.Dispose(); + } + } + + /// + /// A genuine rejection is returned to the caller on the first attempt, never retried. + /// + [Theory] + [InlineData(HttpStatusCode.BadRequest)] // 400 + [InlineData(HttpStatusCode.Unauthorized)] // 401 + [InlineData(HttpStatusCode.Forbidden)] // 403 + [InlineData(HttpStatusCode.NotFound)] // 404 + [InlineData(HttpStatusCode.Conflict)] // 409 - routine "already exists"; retrying would be wrong + [InlineData(HttpStatusCode.Gone)] // 410 + public async Task GenuineRejection_IsNotRetried(HttpStatusCode rejectionStatus) + { + var (client, httpClient, attemptCount) = CreateClient([rejectionStatus]); + + try + { + // The status surfaces to the caller (as a result or an exception); what matters here is that + // it was not attempted a second time. + try + { + _ = await client.GetAsync(ProductsQuery(), TestContext.Current.CancellationToken); + } + catch (Exception) + { + // Expected for several of these; the assertion below is the point of the test. + } + + attemptCount().Should().Be(1, "a genuine rejection must be returned to the caller, not retried"); + } + finally + { + client.Dispose(); + httpClient.Dispose(); + } + } + + /// + /// Retries stop at RetryCount rather than continuing indefinitely. + /// + [Fact] + public async Task TransientStatus_IsRetriedUpToRetryCount_ThenGivesUp() + { + // Four failures against RetryCount = 3 means the last attempt still fails. + var failures = new[] + { + HttpStatusCode.RequestTimeout, + HttpStatusCode.RequestTimeout, + HttpStatusCode.RequestTimeout, + HttpStatusCode.RequestTimeout + }; + + var (client, httpClient, attemptCount) = CreateClient(failures); + + try + { + try + { + _ = await client.GetAsync(ProductsQuery(), TestContext.Current.CancellationToken); + } + catch (Exception) + { + // The failure surfacing is expected; the attempt count is what is under test. + } + + attemptCount().Should().Be(4, "the initial attempt plus RetryCount (3) retries, and no more"); + } + finally + { + client.Dispose(); + httpClient.Dispose(); + } + } + + /// + /// A server-supplied Retry-After is used in preference to the configured RetryDelay. + /// + [Fact] + public async Task RetryAfter_IsPreferredOverRetryDelay() + { + // A long RetryDelay with a short Retry-After: honouring the header should complete quickly. + var (client, httpClient, _) = CreateClient( + [HttpStatusCode.TooManyRequests], + retryDelay: TimeSpan.FromSeconds(30), + retryAfter: TimeSpan.FromMilliseconds(50)); + + try + { + var stopwatch = Stopwatch.StartNew(); + _ = await client.GetAsync(ProductsQuery(), TestContext.Current.CancellationToken); + stopwatch.Stop(); + + stopwatch.Elapsed.Should().BeLessThan( + TimeSpan.FromSeconds(10), + "the 50 ms Retry-After should be used rather than the 30 second RetryDelay"); + } + finally + { + client.Dispose(); + httpClient.Dispose(); + } + } + + /// + /// An excessive Retry-After is bounded by MaximumRetryAfterDelay rather than honoured in full. + /// + [Fact] + public async Task RetryAfter_IsBoundedByMaximumRetryAfterDelay() + { + // A hostile Retry-After of an hour, bounded to 50 ms, must not stall the caller. + var (client, httpClient, _) = CreateClient( + [HttpStatusCode.TooManyRequests], + retryDelay: TimeSpan.FromMilliseconds(1), + retryAfter: TimeSpan.FromHours(1), + maximumRetryAfterDelay: TimeSpan.FromMilliseconds(50)); + + try + { + var stopwatch = Stopwatch.StartNew(); + _ = await client.GetAsync(ProductsQuery(), TestContext.Current.CancellationToken); + stopwatch.Stop(); + + stopwatch.Elapsed.Should().BeLessThan( + TimeSpan.FromSeconds(10), + "a one hour Retry-After must be capped rather than honoured"); + } + finally + { + client.Dispose(); + httpClient.Dispose(); + } + } + + /// + /// Setting MaximumRetryAfterDelay to zero opts out of Retry-After entirely. + /// + [Fact] + public async Task RetryAfter_IsIgnoredWhenMaximumIsZero() + { + // Opting out: Retry-After of an hour, maximum of zero, so RetryDelay applies instead. + var (client, httpClient, _) = CreateClient( + [HttpStatusCode.TooManyRequests], + retryDelay: TimeSpan.FromMilliseconds(1), + retryAfter: TimeSpan.FromHours(1), + maximumRetryAfterDelay: TimeSpan.Zero); + + try + { + var stopwatch = Stopwatch.StartNew(); + _ = await client.GetAsync(ProductsQuery(), TestContext.Current.CancellationToken); + stopwatch.Stop(); + + stopwatch.Elapsed.Should().BeLessThan( + TimeSpan.FromSeconds(10), + "a zero maximum should disable Retry-After entirely and fall back to RetryDelay"); + } + finally + { + client.Dispose(); + httpClient.Dispose(); + } + } +} diff --git a/PanoramicData.OData.Client/ODataClient.cs b/PanoramicData.OData.Client/ODataClient.cs index 466c0f1..e889dde 100644 --- a/PanoramicData.OData.Client/ODataClient.cs +++ b/PanoramicData.OData.Client/ODataClient.cs @@ -58,10 +58,7 @@ public ODataClient(ODataClientOptions options) _ownsHttpClient = false; // If the provided HttpClient has no BaseAddress, set it from options so relative URLs resolve correctly - if (_httpClient.BaseAddress is null) - { - _httpClient.BaseAddress = new Uri(options.BaseUrl.TrimEnd('/') + "/"); - } + _httpClient.BaseAddress ??= new Uri(options.BaseUrl.TrimEnd('/') + "/"); LoggerMessages.UsingProvidedHttpClient(_logger, _httpClient.BaseAddress); } @@ -128,13 +125,58 @@ private async Task SendWithRetryAsync( if (retryCount <= _options.RetryCount) { - await Task.Delay(_options.RetryDelay, cancellationToken).ConfigureAwait(false); + await Task.Delay(GetRetryDelay(lastResponse), cancellationToken).ConfigureAwait(false); } } return lastResponse ?? throw new ODataClientException("Request failed after all retries"); } + /// + /// Whether a status code represents a transient failure that is worth retrying. + /// + /// + /// 408 and 429 join 5xx here because in both cases the server rejected the request without + /// processing it - a 408 means it was never fully received, a 429 that it was refused outright - + /// so retrying is safe even for methods that are not idempotent. + /// + /// Every other 4xx is a genuine rejection and must not be retried. 409 in particular is a routine + /// "already exists" outcome for callers that create-or-overwrite, and retrying it would be wrong. + /// + private static bool IsRetryableStatusCode(HttpStatusCode statusCode) + => statusCode is HttpStatusCode.RequestTimeout or HttpStatusCode.TooManyRequests + || (int)statusCode >= 500; + + /// + /// How long to wait before the next attempt, honouring a Retry-After header when the server sends one. + /// + /// + /// Retrying a 429 on a fixed delay while ignoring Retry-After amplifies the very condition that + /// produced it, so the server's own figure wins where it gives one, bounded by + /// . + /// + private TimeSpan GetRetryDelay(HttpResponseMessage? response) + { + if (_options.MaximumRetryAfterDelay <= TimeSpan.Zero) + { + return _options.RetryDelay; + } + + var serverRequestedDelay = response?.Headers.RetryAfter switch + { + { Delta: { } delta } => delta, + { Date: { } date } => date - DateTimeOffset.UtcNow, + _ => (TimeSpan?)null + }; + + if (serverRequestedDelay is not { } requested || requested <= TimeSpan.Zero) + { + return _options.RetryDelay; + } + + return requested > _options.MaximumRetryAfterDelay ? _options.MaximumRetryAfterDelay : requested; + } + private async Task<(bool ShouldReturn, HttpResponseMessage? Response)> TrySendRequestAsync( HttpRequestMessage request, int retryCount, @@ -156,7 +198,7 @@ private async Task SendWithRetryAsync( // Log full response details at Trace level await LogResponseTraceAsync(response, cancellationToken).ConfigureAwait(false); - if (response.IsSuccessStatusCode || (int)response.StatusCode < 500) + if (response.IsSuccessStatusCode || !IsRetryableStatusCode(response.StatusCode)) { return (true, response); } @@ -265,12 +307,10 @@ private async Task LogRequestTraceAsync(HttpRequestMessage request, Cancellation sb.AppendLine(body); } - #pragma warning disable CA1873 // Method is already guarded by IsEnabled check at method entry - LoggerMessages.LogRequestTrace(_logger, sb.ToString()); - #pragma warning restore CA1873 - } + LoggerMessages.LogRequestTrace(_logger, sb.ToString()); + } - private async Task LogResponseTraceAsync(HttpResponseMessage response, CancellationToken cancellationToken) + private async Task LogResponseTraceAsync(HttpResponseMessage response, CancellationToken cancellationToken) { if (!_logger.IsEnabled(LogLevel.Trace)) { @@ -295,13 +335,10 @@ private async Task LogResponseTraceAsync(HttpResponseMessage response, Cancellat sb.AppendLine("--- Response Body ---"); var body = await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false); sb.AppendLine(body); + LoggerMessages.LogResponseTrace(_logger, sb.ToString()); + } - #pragma warning disable CA1873 // Method is already guarded by IsEnabled check at method entry - LoggerMessages.LogResponseTrace(_logger, sb.ToString()); - #pragma warning restore CA1873 - } - - private static async Task CloneRequestAsync(HttpRequestMessage request) + private static async Task CloneRequestAsync(HttpRequestMessage request) { var clone = new HttpRequestMessage(request.Method, request.RequestUri); diff --git a/PanoramicData.OData.Client/ODataClientOptions.cs b/PanoramicData.OData.Client/ODataClientOptions.cs index 60196a1..7e2ffa8 100644 --- a/PanoramicData.OData.Client/ODataClientOptions.cs +++ b/PanoramicData.OData.Client/ODataClientOptions.cs @@ -33,8 +33,26 @@ public class ODataClientOptions /// /// Delay between retry attempts. Default is 1 second. /// + /// + /// Used when the server does not supply a Retry-After header. When it does, that value is preferred, + /// bounded by . + /// public TimeSpan RetryDelay { get; set; } = TimeSpan.FromSeconds(1); + /// + /// Upper bound on a delay requested by a server via a Retry-After header. Default is 30 seconds. + /// + /// + /// A Retry-After is honoured in preference to , because retrying a 429 on a + /// fixed delay while ignoring the server's own figure amplifies the condition that produced it. + /// + /// It is bounded so that a large or malformed header cannot stall the caller for as long as it asks: + /// a client that hangs for an hour is worse than one that asks again too soon and is refused a second + /// time. Set to to ignore Retry-After entirely and always use + /// . + /// + public TimeSpan MaximumRetryAfterDelay { get; set; } = TimeSpan.FromSeconds(30); + /// /// Gets or sets the level at which individual failed attempts that will be retried are logged. /// diff --git a/README.md b/README.md index cccf488..533b9bf 100644 --- a/README.md +++ b/README.md @@ -391,9 +391,13 @@ var client = new ODataClient(new ODataClientOptions // Optional: Request timeout (default: 5 minutes) Timeout = TimeSpan.FromMinutes(5), - // Optional: Retry configuration for transient failures + // Optional: Retry configuration for transient failures (408, 429 and 5xx) RetryCount = 3, RetryDelay = TimeSpan.FromSeconds(1), + + // Optional: upper bound on a server-supplied Retry-After header, which is honoured in + // preference to RetryDelay. TimeSpan.Zero ignores Retry-After entirely. (default: 30 seconds) + MaximumRetryAfterDelay = TimeSpan.FromSeconds(30), // Optional: Provide your own HttpClient HttpClient = existingHttpClient,