From 6660a512a4bac106bd7b72144948dbef588bc463 Mon Sep 17 00:00:00 2001 From: David Bond Date: Thu, 1 Oct 2026 13:38:03 +0100 Subject: [PATCH] Do not retry HTTP 500 and 501: only 502, 503 and 504 among 5xx (fixes #53) A 500 is the server's own answer, so retrying it only delayed an error the caller was always going to get. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 4 ++++ .../ODataClientRetryIdempotencyTests.cs | 4 ++-- .../UnitTests/ODataClientRetryStatusCodeTests.cs | 3 ++- .../UnitTests/ODataClientRetryTests.cs | 16 ++++++++-------- PanoramicData.OData.Client/ODataClient.cs | 14 ++++++++++++-- 5 files changed, 28 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index af49d36..0cc3463 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,10 @@ 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). +## [Unreleased] + +### Changed +- Stop retrying HTTP 500 (Internal Server Error) and 501 (Not Implemented). Of the 5xx statuses only 502, 503 and 504 are now retried, still only for idempotent methods. A 500 is the server's own answer and asking again gets the same one, so retrying it only kept callers waiting before an error they were always going to get: in Magic Suite, behind a page loading overlay that also blocked Merlin (MS-26904). This reverses the earlier choice to retry 500; a deployment whose server returns 500 for transient faults should return 503 for them instead ## [10.0.109] - 2026-08-07 ### Added diff --git a/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryIdempotencyTests.cs b/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryIdempotencyTests.cs index 99bd9dd..338eaf7 100644 --- a/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryIdempotencyTests.cs +++ b/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryIdempotencyTests.cs @@ -136,11 +136,11 @@ public async Task Post_RejectedWithoutProcessing_IsStillRetried(HttpStatusCode r } /// - /// A GET still retries on 5xx exactly as before. This is the regression guard: the fix must not + /// A GET still retries on 502, 503 and 504. This is the regression guard: the fix must not /// have narrowed retries for idempotent traffic, which is the overwhelming majority. /// [Theory] - [InlineData(HttpStatusCode.InternalServerError)] + [InlineData(HttpStatusCode.ServiceUnavailable)] [InlineData(HttpStatusCode.GatewayTimeout)] public async Task Get_ServerError_IsStillRetried(HttpStatusCode serverError) { diff --git a/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryStatusCodeTests.cs b/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryStatusCodeTests.cs index 88781f9..368fcb1 100644 --- a/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryStatusCodeTests.cs +++ b/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryStatusCodeTests.cs @@ -87,7 +87,6 @@ private static (ODataClient Client, HttpClient HttpClient, Func AttemptCoun [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 @@ -119,6 +118,8 @@ public async Task TransientStatus_IsRetried_AndSucceeds(HttpStatusCode transient [InlineData(HttpStatusCode.NotFound)] // 404 [InlineData(HttpStatusCode.Conflict)] // 409 - routine "already exists"; retrying would be wrong [InlineData(HttpStatusCode.Gone)] // 410 + [InlineData(HttpStatusCode.InternalServerError)] // 500 - the server's own answer; asking again gets the same one + [InlineData(HttpStatusCode.NotImplemented)] // 501 public async Task GenuineRejection_IsNotRetried(HttpStatusCode rejectionStatus) { var (client, httpClient, attemptCount) = CreateClient([rejectionStatus]); diff --git a/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryTests.cs b/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryTests.cs index f65c56e..4d8bae8 100644 --- a/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryTests.cs +++ b/PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryTests.cs @@ -30,7 +30,7 @@ public void Dispose() } /// - /// Tests that client retries on 500 error. + /// Tests that client retries on a 503 error. /// [Fact] public async Task Request_ServerError_Retries() @@ -47,7 +47,7 @@ public async Task Request_ServerError_Retries() callCount++; if (callCount < 3) { - return new HttpResponseMessage(HttpStatusCode.InternalServerError) + return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable) { Content = new StringContent("{}") }; @@ -175,7 +175,7 @@ public async Task Request_MaxRetries_ThrowsException() .ReturnsAsync(() => { callCount++; - return new HttpResponseMessage(HttpStatusCode.InternalServerError) + return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable) { Content = new StringContent("{}") }; @@ -217,7 +217,7 @@ public async Task Request_TransientFailureThatRecovers_LogsAttemptAtDebugAndNoWa callCount++; if (callCount < 2) { - return new HttpResponseMessage(HttpStatusCode.InternalServerError) + return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable) { Content = new StringContent("{}") }; @@ -259,7 +259,7 @@ public async Task Request_RetriesExhaustedOnServerError_LogsSingleWarning() "SendAsync", ItExpr.IsAny(), ItExpr.IsAny()) - .ReturnsAsync(() => new HttpResponseMessage(HttpStatusCode.InternalServerError) + .ReturnsAsync(() => new HttpResponseMessage(HttpStatusCode.ServiceUnavailable) { Content = new StringContent("{}") }); @@ -339,7 +339,7 @@ public async Task Request_RetryAttemptLogLevelWarning_LogsAttemptsAtWarning() callCount++; if (callCount < 2) { - return new HttpResponseMessage(HttpStatusCode.InternalServerError) + return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable) { Content = new StringContent("{}") }; @@ -387,7 +387,7 @@ public async Task Request_RetryAttemptLogLevelNone_LogsNoAttempts() callCount++; if (callCount < 2) { - return new HttpResponseMessage(HttpStatusCode.InternalServerError) + return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable) { Content = new StringContent("{}") }; @@ -450,7 +450,7 @@ public async Task Request_RetryDelay_IsRespected() callTimes.Add(DateTime.UtcNow); if (callTimes.Count < 2) { - return new HttpResponseMessage(HttpStatusCode.InternalServerError) + return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable) { Content = new StringContent("{}") }; diff --git a/PanoramicData.OData.Client/ODataClient.cs b/PanoramicData.OData.Client/ODataClient.cs index 539d328..104c470 100644 --- a/PanoramicData.OData.Client/ODataClient.cs +++ b/PanoramicData.OData.Client/ODataClient.cs @@ -141,7 +141,7 @@ private async Task SendWithRetryAsync( /// the request without processing it - a 408 means it was never fully received, a 429 /// that it was refused outright - so repeating it cannot duplicate any effect. /// - /// 5xx is different, and only safe for an idempotent method (issue #43). A 504 in particular + /// Of the 5xx statuses only 502, 503 and 504 are retried, and only for an idempotent method (issue #43). A 504 in particular /// means the opposite of a 408: the request reached the server, the server began work, and a /// proxy gave up waiting - so the work may still be running. 502 and 503 can likewise be /// returned after an upstream has accepted a request. Retrying a POST in that state either @@ -155,7 +155,17 @@ private async Task SendWithRetryAsync( /// private static bool IsRetryableStatusCode(HttpStatusCode statusCode, HttpMethod method) => statusCode is HttpStatusCode.RequestTimeout or HttpStatusCode.TooManyRequests - || ((int)statusCode >= 500 && IsIdempotent(method)); + || (IsTransientServerError(statusCode) && IsIdempotent(method)); + + /// + /// Whether a 5xx reports an upstream that may recover (502, 503, 504) rather than the server's own answer. + /// + /// + /// A 500 or 501 is the server saying what happened, and asking again gets the same answer: retrying it + /// only keeps the caller waiting, for example behind a loading overlay, before the error it was always going to get. + /// + private static bool IsTransientServerError(HttpStatusCode statusCode) + => statusCode is HttpStatusCode.BadGateway or HttpStatusCode.ServiceUnavailable or HttpStatusCode.GatewayTimeout; /// /// Whether repeating a request with this method is guaranteed to have the same effect as making