Do not retry HTTP 500 and 501 (fixes #53)

CHANGELOG.md

@@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [vNext]
 
 ### 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 server that returns 500 for transient faults should return 503 for them instead (#53)
 - Rename four builder parameters that duplicated their method name, so that they read
   distinctly in IntelliSense: `Key(key)` to `Key(keyValue)`, `Filter(filter)` to
   `Filter(filterExpression)`, `OrderBy(orderBy)` to `OrderBy(orderByExpression)` and

PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryIdempotencyTests.cs

@@ -136,11 +136,11 @@ public async Task Post_RejectedWithoutProcessing_IsStillRetried(HttpStatusCode r
 	}
 
 	/// <summary>
-	/// 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.
 	/// </summary>
 	[Theory]
-	[InlineData(HttpStatusCode.InternalServerError)]
+	[InlineData(HttpStatusCode.ServiceUnavailable)]
 	[InlineData(HttpStatusCode.GatewayTimeout)]
 	public async Task Get_ServerError_IsStillRetried(HttpStatusCode serverError)
 	{

PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryStatusCodeTests.cs

@@ -87,7 +87,6 @@ private static (ODataClient Client, HttpClient HttpClient, Func<int> 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]);

PanoramicData.OData.Client.Test/UnitTests/ODataClientRetryTests.cs

@@ -8,7 +8,7 @@ namespace PanoramicData.OData.Client.Test.UnitTests;
 public class ODataClientRetryTests : MockedODataClientTestBase
 {
 	/// <summary>
-	/// Tests that client retries on 500 error.
+	/// Tests that client retries on a 503 error.
 	/// </summary>
 	[Fact]
 	public async Task Request_ServerError_Retries()
@@ -21,7 +21,7 @@ public async Task Request_ServerError_Retries()
 				callCount++;
 				if (callCount < 3)
 				{
-					return new HttpResponseMessage(HttpStatusCode.InternalServerError)
+					return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable)
 					{
 						Content = new StringContent("{}")
 					};
@@ -137,7 +137,7 @@ public async Task Request_MaxRetries_ThrowsException()
 			.ReturnsAsync(() =>
 			{
 				callCount++;
-				return new HttpResponseMessage(HttpStatusCode.InternalServerError)
+				return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable)
 				{
 					Content = new StringContent("{}")
 				};
@@ -175,7 +175,7 @@ public async Task Request_TransientFailureThatRecovers_LogsAttemptAtDebugAndNoWa
 				callCount++;
 				if (callCount < 2)
 				{
-					return new HttpResponseMessage(HttpStatusCode.InternalServerError)
+					return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable)
 					{
 						Content = new StringContent("{}")
 					};
@@ -213,7 +213,7 @@ public async Task Request_RetriesExhaustedOnServerError_LogsSingleWarning()
 	{
 		// Arrange
 		SetupSendAsync()
-			.ReturnsAsync(() => new HttpResponseMessage(HttpStatusCode.InternalServerError)
+			.ReturnsAsync(() => new HttpResponseMessage(HttpStatusCode.ServiceUnavailable)
 			{
 				Content = new StringContent("{}")
 			});
@@ -285,7 +285,7 @@ public async Task Request_RetryAttemptLogLevelWarning_LogsAttemptsAtWarning()
 				callCount++;
 				if (callCount < 2)
 				{
-					return new HttpResponseMessage(HttpStatusCode.InternalServerError)
+					return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable)
 					{
 						Content = new StringContent("{}")
 					};
@@ -329,7 +329,7 @@ public async Task Request_RetryAttemptLogLevelNone_LogsNoAttempts()
 				callCount++;
 				if (callCount < 2)
 				{
-					return new HttpResponseMessage(HttpStatusCode.InternalServerError)
+					return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable)
 					{
 						Content = new StringContent("{}")
 					};
@@ -388,7 +388,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("{}")
 					};

PanoramicData.OData.Client/ODataClient.cs

@@ -142,7 +142,7 @@ private async Task<HttpResponseMessage> SendWithRetryAsync(
 	/// the request <em>without processing it</em> - 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
@@ -156,7 +156,17 @@ private async Task<HttpResponseMessage> SendWithRetryAsync(
 	/// </remarks>
 	private static bool IsRetryableStatusCode(HttpStatusCode statusCode, HttpMethod method)
 		=> statusCode is HttpStatusCode.RequestTimeout or HttpStatusCode.TooManyRequests
-			|| ((int)statusCode >= 500 && IsIdempotent(method));
+			|| (IsTransientServerError(statusCode) && IsIdempotent(method));
+
+	/// <summary>
+	/// Whether a 5xx reports an upstream that may recover (502, 503, 504) rather than the server's own answer.
+	/// </summary>
+	/// <remarks>
+	/// 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.
+	/// </remarks>
+	private static bool IsTransientServerError(HttpStatusCode statusCode)
+		=> statusCode is HttpStatusCode.BadGateway or HttpStatusCode.ServiceUnavailable or HttpStatusCode.GatewayTimeout;
 
 	/// <summary>
 	/// Whether repeating a request with this method is guaranteed to have the same effect as making