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

[davidnmbond]

Fixes #53.

Of the 5xx statuses, only 502, 503 and 504 are now retried, still only for idempotent methods (#43). 408 and 429 are unchanged. 500 and 501 go straight back to the caller.

This reverses a deliberate earlier choice: ODataClientRetryStatusCodeTests previously asserted that 500 is retried. A server that returns 500 for transient faults should send 503 instead.

Tests: 723 unit tests pass. Red-checked: restoring the old rule fails GenuineRejection_IsNotRetried for 500 and 501. The mechanics tests in ODataClientRetryTests now use 503 as their retryable error.

Motivation: MS-26904 in Magic Suite, where a deterministic 500 kept a page's loading overlay up through five retries.

🤖 Generated with Claude Code

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity · 0 duplication

Metric	Results
Complexity	0
Duplication	0

View in Codacy

🟢 Coverage 100.00% diff coverage · +0.01% coverage variation

Metric	Results
Coverage variation	✅ +0.01% coverage variation (-1.00%)
Diff coverage	✅ 100.00% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (9a01941)	3022	2542	84.12%
Head commit (af9f32d)	3023 (+1)	2543 (+1)	84.12% (+0.01%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#54)	2	2	100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

_{TIP This summary will be updated as you push new changes.}

[codacy-production]

Pull Request Overview

This PR successfully implements the logic to treat HTTP 500 and 501 as terminal errors, ensuring they are no longer retried. While the project remains 'up to standards' according to Codacy, the implementation has increased the cyclomatic complexity of ODataClient.cs, specifically within the TrySendRequestAsync method.

A key gap was identified in the test suite: while retries for idempotent methods are verified, there is no explicit test ensuring that transient 5xx errors (502, 503, 504) correctly suppress retries for non-idempotent methods like POST. Addressing this and the complexity issue is recommended to ensure long-term maintainability.

About this PR

• While the code implements idempotency checks for transient 5xx errors (502, 503, 504), there is no explicit test case verifying that these statuses correctly suppress retries for POST or PATCH requests. Adding a test for this scenario is recommended to prevent future regressions.

1 comment outside of the diff

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

_{line 427 ⚪ LOW RISK}
Nitpick: The parameter state is unused in this ILogger mock. To satisfy static analysis while maintaining the interface contract, rename it to _ to explicitly indicate that the value is intentionally ignored.

		public IDisposable? BeginScope<TState>(TState _) where TState : notnull => null;

Test suggestions

• Verify that an HTTP 500 response results in zero retries for a GET request.
• Verify that an HTTP 501 response results in zero retries for a GET request.
• Verify that an HTTP 503 response is retried for a GET request.
• Verify that an HTTP 503 response is NOT retried for a POST request (non-idempotent).
• Verify that an HTTP 429 response is still retried for a POST request.

Prompt proposal for missing tests

Consider implementing these tests if applicable:
1. Verify that an HTTP 503 response is NOT retried for a POST request (non-idempotent).

_{TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback}

[codacy-production]

_{⚪ LOW RISK}

Suggestion: Add HttpStatusCode.BadGateway to the test cases to provide full coverage for all codes mentioned in the summary.

Suggested change

	[InlineData(HttpStatusCode.ServiceUnavailable)]
	[InlineData(HttpStatusCode.BadGateway)]
	[InlineData(HttpStatusCode.ServiceUnavailable)]

[codacy-production]

_{⚪ LOW RISK}

Nitpick: Consider wrapping this long line or breaking it into multiple bullet points to improve readability.