Yurii bart feat add retryable policy 01

[r4m-semeyon]

No description provided.

[github-actions]

Gemini AI Code Risk & Compatibility Analysis

Status: [OK] Analysis completed successfully

Files analyzed:

• route4me-csharp-sdk/Route4MeSDKLibrary/Managers/Route4MeManagerBase.cs
• route4me-csharp-sdk/Route4MeSDKLibrary/Resilience/HttpResiliencePolicy.cs
• route4me-csharp-sdk/Route4MeSDKLibrary/Route4MeConfig.cs
• route4me-csharp-sdk/Route4MeSDKTest/Examples/Optimizations/BatchOptimizationWithRetry.cs
• route4me-csharp-sdk/Route4MeSdkV5UnitTest/V5/ResilienceTests.cs

Analysis Results

[STARTUP] StartupProfiler.flush() called with 9 phases
[STARTUP] Recording metric for phase: cli_startup duration: 60.92870199999993
[STARTUP] Recording metric for phase: load_settings duration: 1.0328839999999673
[STARTUP] Recording metric for phase: migrate_settings duration: 1.3446349999999256
[STARTUP] Recording metric for phase: parse_arguments duration: 18.61390499999993
[STARTUP] Recording metric for phase: load_cli_config duration: 22.70555300000001
[STARTUP] Recording metric for phase: initialize_app duration: 5.835804000000053
[STARTUP] Recording metric for phase: authenticate duration: 0.07456900000011046
[STARTUP] Recording metric for phase: discover_tools duration: 249.815652
[STARTUP] Recording metric for phase: initialize_mcp_clients duration: 0.4011270000000877
(node:2230) MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal]. MaxListeners is 10. Use events.setMaxListeners() to increase limit
(Use node --trace-warnings ... to show where the warning was created)
Here is the analysis of the code changes:

Summary

The submitted code introduces resilience and fault-tolerance mechanisms (retry and circuit breaker patterns) into the HTTP client logic using the Polly library. This is a significant feature addition that improves the robustness of the SDK. While the implementation is generally good, there are potential risks related to synchronous-over-asynchronous programming patterns that could lead to deadlocks, and changes in exception handling that could affect existing clients.

---

1. Potential Backward Compatibility Risks

• Severity: Medium
• File: route4me-csharp-sdk/Route4MeSDKLibrary/Managers/Route4MeManagerBase.cs
• Issue: The introduction of new catch blocks for HttpRequestException and BrokenCircuitException alters the error handling flow. Previously, a failed HTTP request would likely throw an HttpRequestException. Now, if retries are enabled (RetryCount > 0), the same failure will be caught internally and result in a ResultResponse object with Status = false and a "RetryExhausted" message.
• Risk: This is a behavioral breaking change for consumers who have enabled the new retry feature and have existing logic that catches HttpRequestException. Their catch blocks will no longer execute, and they must instead inspect the returned ResultResponse object to detect the failure.
• Suggestion: This change is acceptable for a new feature, but it should be prominently documented in the release notes. Clearly state that when RetryCount is greater than 0, transient HttpRequestExceptions are no longer thrown and are instead reported through the ResultResponse object.

---

2. Performance Risks & Coding Standard Violations

• Severity: High
• File: route4me-csharp-sdk/Route4MeSDKLibrary/Managers/Route4MeManagerBase.cs
• Lines: 457-461, 492-540
• Issue: The synchronous overload of RunApiRequest uses blocking calls like .Wait() and .Result on asynchronous methods (e.g., httpClientHolder.HttpClient.GetAsync(...).Wait()). This is a well-known anti-pattern known as "sync-over-async".
• Risk: This can lead to thread pool starvation and, more critically, deadlocks in applications with a synchronization context (like ASP.NET Classic, Windows Forms, or WPF). The application could hang indefinitely.
• Suggestion:
  • Primary Recommendation: Deprecate and eventually remove the synchronous RunApiRequest method. Encourage all consumers to use the asynchronous RunApiRequestAsync which is safer and more efficient.
  • Alternative (If sync is required): While no sync-over-async implementation is perfectly safe, a slightly better pattern would involve using a private async method and blocking it with GetAwaiter().GetResult(), though this does not eliminate the risk of deadlocks entirely. The fundamental issue is mixing blocking and async code.

// Problematic code in synchronous GET
var httpResponse = HttpResiliencePolicy.GetSyncPolicy()
    .Execute(() =>
    {
        var task = httpClientHolder.HttpClient.GetAsync(uri.PathAndQuery);
        task.Wait(); // <-- Potential deadlock
        return task.Result;
    });

// Problematic code in synchronous POST/PUT etc.
var response = HttpResiliencePolicy.GetSyncPolicy()
    .Execute(() =>
    {
        // ...
        task.Wait(); // <-- Potential deadlock
        return task.Result;
    });

---

3. Other Improvement Suggestions

• Severity: Low
• File: route4me-csharp-sdk/Route4MeSDKLibrary/Managers/Route4MeManagerBase.cs
• Issue: The new catch (HttpRequestException hre) block swallows the original exception details. It creates a generic message that loses valuable context from the original exception (like the status code or specific error message), which is crucial for debugging.
• Suggestion: Include details from the original exception in the response message to aid in diagnostics.

// Current implementation
resultResponse = new ResultResponse
{
    Status = false,
    Messages = new Dictionary<string, string[]>
    {
        {"RetryExhausted", new[] {
            $"Request failed after {Route4MeConfig.RetryCount} retry attempts: {hre.Message}"
        }}
    }
};

// Suggested improvement
resultResponse = new ResultResponse
{
    Status = false,
    Messages = new Dictionary<string, string[]>
    {
        {"RetryExhausted", new[] {
            $"Request failed after {Route4MeConfig.RetryCount} retry attempts. Last exception: {hre.Message}"
            // Consider also logging hre.ToString() for full details if a logging framework is used
        }}
    }
};

• Severity: Low
• File: route4me-csharp-sdk/Route4MeSDKLibrary/Resilience/HttpResiliencePolicy.cs
• Issue: The file introduces a new helper method IsTransientError, but it is not used anywhere in the provided code changes.
• Suggestion: If this method is intended for use, it should be integrated into the policy definitions. If it is dead code, it should be removed to keep the codebase clean.