Skip to content

feat: add HTTP retry logic to update client #49

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hieuck merged 1 commit into from
Jul 8, 2026
Merged

feat: add HTTP retry logic to update client #49

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions src/HttpRetryPolicy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
using System;
using System.Net;
using System.Net.Http;
using System.Threading;
using System.Threading.Tasks;

namespace KeePassAutoReload
{
internal static class HttpRetryPolicy
{
public const int DefaultMaxRetries = 3;

public static async Task<HttpResponseMessage> ExecuteAsync(Func<Task<HttpResponseMessage>> operation, int maxRetries, CancellationToken cancellationToken = default)
{
if (operation == null) throw new ArgumentNullException("operation");
if (maxRetries < 0) throw new ArgumentOutOfRangeException("maxRetries");

cancellationToken.ThrowIfCancellationRequested();

Exception lastException = null;
for (int attempt = 0; attempt <= maxRetries; attempt++)
{
try
{
HttpResponseMessage response = await operation();
if ((int)response.StatusCode >= 500 && (int)response.StatusCode < 600)
{
response.Dispose();
if (attempt == maxRetries)
{
throw new HttpRequestException("Server returned " + (int)response.StatusCode + " after " + maxRetries + " retries.");
}
await DelayAsync(attempt, cancellationToken);
continue;
}
return response;
Comment on lines +25 to +36

@codereviewbot-ai codereviewbot-ai Bot Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential Resource Leak on Exception After Response Creation

If the operation() function returns an HttpResponseMessage, but an exception is thrown after the response is created and before it is returned (for example, in user code that wraps or processes the response), the response may not be disposed, leading to a resource leak. Consider wrapping the response handling in a try/catch/finally block to ensure the response is disposed in all failure scenarios where it is not returned to the caller.

Recommended Solution:

try {
    HttpResponseMessage response = await operation();
    if ((int)response.StatusCode >= 500 && (int)response.StatusCode < 600) {
        response.Dispose();
        // ...
    }
    return response;
} catch (Exception) {
    // If response was created, dispose it here
    throw;
}

Or ensure that the operation function itself is responsible for disposing of the response on failure.

}
catch (HttpRequestException ex) when (attempt < maxRetries)
{
lastException = ex;
await DelayAsync(attempt, cancellationToken);
}
catch (TaskCanceledException ex) when (attempt < maxRetries && !cancellationToken.IsCancellationRequested)
{
lastException = ex;
await DelayAsync(attempt, cancellationToken);
}
}

throw lastException ?? new HttpRequestException("Request failed after retries.");
}

private static async Task DelayAsync(int attempt, CancellationToken cancellationToken)
{
int milliseconds = (int)(100 * Math.Pow(2, attempt));
await Task.Delay(milliseconds, cancellationToken);
}
}
}
1 change: 1 addition & 0 deletions src/KeePassAutoReload.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
<ItemGroup>
<Compile Include="AssetVerifier.cs" />
<Compile Include="AutoSyncPolicy.cs" />
<Compile Include="HttpRetryPolicy.cs" />
<Compile Include="KeePassAutoReloadExt.cs" />
<Compile Include="PluginAboutInfo.cs" />
<Compile Include="PluginPathResolver.cs" />
Expand Down
10 changes: 8 additions & 2 deletions src/UpdateChecker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,10 @@ public HttpUpdateClient()

public async Task<string> DownloadStringAsync(string url, CancellationToken cancellationToken = default)
{
using (HttpResponseMessage response = await _client.GetAsync(url, cancellationToken))
using (HttpResponseMessage response = await HttpRetryPolicy.ExecuteAsync(
() => _client.GetAsync(url, cancellationToken),
HttpRetryPolicy.DefaultMaxRetries,
cancellationToken))
{
response.EnsureSuccessStatusCode();
return await response.Content.ReadAsStringAsync();
Expand All @@ -63,7 +66,10 @@ public async Task<string> DownloadStringAsync(string url, CancellationToken canc

public async Task DownloadFileAsync(string url, string destinationPath, CancellationToken cancellationToken = default)
{
using (HttpResponseMessage response = await _client.GetAsync(url, HttpCompletionOption.ResponseHeadersRead, cancellationToken))
using (HttpResponseMessage response = await HttpRetryPolicy.ExecuteAsync(
() => _client.GetAsync(url, HttpCompletionOption.ResponseHeadersRead, cancellationToken),
HttpRetryPolicy.DefaultMaxRetries,
cancellationToken))
{
response.EnsureSuccessStatusCode();
byte[] data = await response.Content.ReadAsByteArrayAsync();
Comment on lines 66 to 75

@codereviewbot-ai codereviewbot-ai Bot Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential Unhandled IO Exceptions and Path Validation in DownloadFileAsync

The method writes the downloaded bytes directly to the specified destinationPath using File.WriteAllBytes, but does not handle IO exceptions (e.g., disk full, permission denied, invalid path) or validate the path for security (e.g., path traversal). This could result in unhandled exceptions or security vulnerabilities.

Recommended Solution:
Wrap the file write operation in a try-catch block to handle IO exceptions and validate the destination path before writing. For example:

try {
    // Validate destinationPath as needed
    File.WriteAllBytes(destinationPath, data);
} catch (IOException ex) {
    // Handle or log the exception
    throw;
}

Expand Down
1 change: 1 addition & 0 deletions tests/KeePassAutoReload.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
<ItemGroup>
<Compile Include="..\src\AssetVerifier.cs" Link="AssetVerifier.cs" />
<Compile Include="..\src\AutoSyncPolicy.cs" Link="AutoSyncPolicy.cs" />
<Compile Include="..\src\HttpRetryPolicy.cs" Link="HttpRetryPolicy.cs" />
<Compile Include="..\src\PluginAboutInfo.cs" Link="PluginAboutInfo.cs" />
<Compile Include="..\src\PluginPathResolver.cs" Link="PluginPathResolver.cs" />
<Compile Include="..\src\PluginUpdater.cs" Link="PluginUpdater.cs" />
Expand Down
107 changes: 107 additions & 0 deletions tests/Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using System.Collections.Generic;
using System.IO;
using System.Net;
using System.Net.Http;
using System.Security.Authentication;
using System.Threading;
using System.Threading.Tasks;
Expand Down Expand Up @@ -76,6 +77,112 @@ public void AboutTextIncludesVersionAndCurrentSettings()
}
}

public class HttpRetryPolicyTests
{
private static Task<HttpResponseMessage> SuccessResponse(HttpStatusCode statusCode)
{
return Task.FromResult(new HttpResponseMessage(statusCode));
}

[Fact]
public async Task ExecuteAsync_ReturnsResponseOnFirstSuccess()
{
HttpResponseMessage response = await HttpRetryPolicy.ExecuteAsync(
() => SuccessResponse(HttpStatusCode.OK),
3);

Assert.Equal(HttpStatusCode.OK, response.StatusCode);
}

[Fact]
public async Task ExecuteAsync_RetriesOnServerErrorThenSucceeds()
{
int callCount = 0;
HttpResponseMessage response = await HttpRetryPolicy.ExecuteAsync(() =>
{
callCount++;
if (callCount < 3) return SuccessResponse(HttpStatusCode.ServiceUnavailable);
return SuccessResponse(HttpStatusCode.OK);
}, 3);

Assert.Equal(HttpStatusCode.OK, response.StatusCode);
Assert.Equal(3, callCount);
}

[Fact]
public async Task ExecuteAsync_DoesNotRetryOnClientError()
{
int callCount = 0;
HttpResponseMessage response = await HttpRetryPolicy.ExecuteAsync(() =>
{
callCount++;
return SuccessResponse(HttpStatusCode.NotFound);
}, 3);

Assert.Equal(HttpStatusCode.NotFound, response.StatusCode);
Assert.Equal(1, callCount);
}

[Fact]
public async Task ExecuteAsync_ThrowsAfterMaxRetriesOnServerError()
{
int callCount = 0;
await Assert.ThrowsAsync<HttpRequestException>(() => HttpRetryPolicy.ExecuteAsync(() =>
{
callCount++;
return SuccessResponse(HttpStatusCode.InternalServerError);
}, 2));

Assert.Equal(3, callCount);
}
Comment on lines +127 to +137

@codereviewbot-ai codereviewbot-ai Bot Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In ExecuteAsync_ThrowsAfterMaxRetriesOnServerError, the test only asserts that an HttpRequestException is thrown and that the call count is as expected. However, it does not verify the exception message or ensure that the exception is thrown specifically due to exceeding the retry limit. To improve error detection, assert on the exception's message or a custom exception type if available, to avoid false positives from unrelated exceptions.


[Fact]
public async Task ExecuteAsync_RetriesOnTransientExceptionThenSucceeds()
{
int callCount = 0;
HttpResponseMessage response = await HttpRetryPolicy.ExecuteAsync(() =>
{
callCount++;
if (callCount < 2) throw new HttpRequestException("transient");
return SuccessResponse(HttpStatusCode.OK);
}, 3);

Assert.Equal(HttpStatusCode.OK, response.StatusCode);
Assert.Equal(2, callCount);
}

[Fact]
public async Task ExecuteAsync_ThrowsOnCancellationWithoutRetry()
{
int callCount = 0;
using (CancellationTokenSource cts = new CancellationTokenSource())
{
cts.Cancel();
await Assert.ThrowsAsync<OperationCanceledException>(() => HttpRetryPolicy.ExecuteAsync(() =>
{
callCount++;
return SuccessResponse(HttpStatusCode.OK);
}, 3, cts.Token));
}

Assert.Equal(0, callCount);
}
Comment on lines +155 to +169

@codereviewbot-ai codereviewbot-ai Bot Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test ExecuteAsync_ThrowsOnCancellationWithoutRetry asserts that callCount is 0 after cancellation. This assumes that the cancellation token is checked before the operation is invoked. If the implementation changes to increment the counter before checking for cancellation, this test will fail even though the behavior is still correct (i.e., the operation is canceled before any work is done). To make the test more robust, consider only asserting that an OperationCanceledException is thrown, or document the expected order of operations in the implementation.


[Fact]
public void ExecuteAsync_ThrowsWhenOperationIsNull()
{
Assert.Throws<ArgumentNullException>(() =>
HttpRetryPolicy.ExecuteAsync(null, 3).GetAwaiter().GetResult());
}

[Fact]
public void ExecuteAsync_ThrowsWhenMaxRetriesIsNegative()
{
Assert.Throws<ArgumentOutOfRangeException>(() =>
HttpRetryPolicy.ExecuteAsync(() => SuccessResponse(HttpStatusCode.OK), -1).GetAwaiter().GetResult());
}
}

public class UpdateCheckerTests
{
[Fact]
Expand Down
Loading