Do not retry non-idempotent requests the server may have processed (fixes #43)

[davidnmbond]

Closes #43.

The distinction

#42 admitted 408 and 429 alongside 5xx, on the grounds that the server rejected the request without processing it. That reasoning is right, and it is exactly why it does not extend to 5xx — a 504 means the opposite of a 408: the request arrived, work began, and a proxy stopped waiting.

Method	408 / 429	5xx	Client timeout
GET, HEAD, PUT, DELETE, OPTIONS, TRACE	retry	retry	retry
POST, PATCH	retry	no	no

RetryCount keeps its default. What changed is what is eligible for retry, so idempotent traffic — the overwhelming majority — is untouched.

Why it matters

In Magic Suite, one POST starting a two-minute AI investigation, behind a proxy with a 60-second read timeout, ran six times. The user waited six minutes for the error and watched the UI cycle through five investigations nobody asked for. A create retried after a 504 could also duplicate the record.

Deliberately not changed

HttpRequestException still retries for all methods. It is overwhelmingly a connection-level failure where nothing was processed, and narrowing it would cost real resilience for no clear safety gain. Flagging it so the omission reads as a decision.

Verification

Red-checked: with the fix reverted, exactly the four POST/5xx cases fail (4 attempts instead of 1) while the 408/429 and GET guards stay green — confirming they are genuine regression guards rather than co-dependent on the change. 722 unit tests pass, including #42's.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 12 complexity · 0 duplication

Metric	Results
Complexity	12
Duplication	0

View in Codacy

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

The PR correctly restricts retry behavior for non-idempotent HTTP methods (POST, PATCH) to prevent unintended side effects on the server, specifically handling 5xx errors and client-side timeouts. However, the implementation in TrySendRequestAsync has increased the method's complexity beyond recommended limits, and a logging bug was identified where the attempt count is hardcoded to 1 regardless of previous retries for 408/429 status codes. While the logic appears sound, there is a lack of unit test coverage for TaskCanceledException scenarios and PATCH requests, which are central to the PR's intent.

About this PR

• There are no tests covering the PATCH method in the provided diff, although the logic explicitly treats it as non-idempotent. It is recommended to include PATCH in the retry test suite.
• The logic added to handle TaskCanceledException (lines 256-260 in ODataClient.cs) to prevent retries for non-idempotent methods lacks corresponding unit tests. Please add coverage for client-side timeouts to ensure expected behavior.

1 comment outside of the diff

PanoramicData.OData.Client/ODataClient.cs

_{line 201 ⚪ LOW RISK}
Suggestion: The method TrySendRequestAsync currently handles request execution, trace logging, and complex exception-handling logic for both HttpRequestException and TaskCanceledException. To improve maintainability, consider extracting the error-handling blocks or the trace logging into smaller helper methods such as HandleRequestException and HandleTimeoutException.

Test suggestions

• A POST request receiving a 5xx status code (e.g., 504 Gateway Timeout) is attempted exactly once.
• A POST request receiving a 408 Request Timeout or 429 Too Many Requests is retried up to the configured limit.
• A GET request receiving a 5xx status code is retried up to the limit.
• A POST request resulting in a client-side TaskCanceledException is not retried.
• An idempotent request (e.g., GET) resulting in a client-side TaskCanceledException is retried up to the limit.

Prompt proposal for missing tests

Consider implementing these tests if applicable:
1. A POST request resulting in a client-side TaskCanceledException is not retried.
2. An idempotent request (e.g., GET) resulting in a client-side TaskCanceledException is retried up to the limit.

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

[codacy-production]

_{🟡 MEDIUM RISK}

The attempt count should reflect the actual number of attempts made, even when aborting early for non-idempotent requests.

Suggested change

	LoggerMessages.RetriesExhaustedException(_logger, ex, request.RequestUri, "timed out", 1);
	LoggerMessages.RetriesExhaustedException(_logger, ex, request.RequestUri, "timed out", retryCount + 1);