From cb2fcec88a9706e5da1f5898a0842ab765d7110e Mon Sep 17 00:00:00 2001 From: Dean Tambling Date: Mon, 4 May 2026 23:13:15 -0700 Subject: [PATCH 1/6] WIP: First pass --- RELEASENOTES.md | 1 + src/Octoshift/RetryPolicy.cs | 23 ++- src/Octoshift/Services/GithubClient.cs | 67 +++++++- .../Octoshift/RetryPolicyTests.cs | 153 ++++++++++++++++++ .../Octoshift/Services/GithubClientTests.cs | 145 ++++++++++++++++- 5 files changed, 379 insertions(+), 10 deletions(-) create mode 100644 src/OctoshiftCLI.Tests/Octoshift/RetryPolicyTests.cs diff --git a/RELEASENOTES.md b/RELEASENOTES.md index e69de29bb..fc2fa6ae5 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -0,0 +1 @@ +- Improved retry resilience for transient 5xx server errors (502 Bad Gateway, 503 Service Unavailable, 504 Gateway Timeout) from the GitHub API, with dedicated exponential backoff and Retry-After header support. Non-transient 4xx client errors are no longer needlessly retried. diff --git a/src/Octoshift/RetryPolicy.cs b/src/Octoshift/RetryPolicy.cs index 550619756..452160144 100644 --- a/src/Octoshift/RetryPolicy.cs +++ b/src/Octoshift/RetryPolicy.cs @@ -65,7 +65,7 @@ public async Task> RetryOnResult(Func> func, Func Retry(Func> func) => await CreateRetryPolicyForException().ExecuteAsync(func); private AsyncRetryPolicy CreateRetryPolicyForException() where TException : Exception => Policy - .Handle() + .Handle(ex => IsRetryableException(ex)) .WaitAndRetryAsync(5, retry => retry * TimeSpan.FromMilliseconds(_retryInterval), (Exception ex, TimeSpan ts, Context ctx) => { if (ex is HttpRequestException httpEx) @@ -83,6 +83,27 @@ private AsyncRetryPolicy CreateRetryPolicyForException() where TExce _log?.LogVerbose("Retrying..."); }); + private static bool IsRetryableException(Exception ex) + { + if (ex is OctoshiftCliException) + { + return false; + } + + if (ex is not HttpRequestException httpEx) + { + return true; + } + + if (httpEx.StatusCode == System.Net.HttpStatusCode.Unauthorized) + { + return true; // Let it through so the retry handler can throw OctoshiftCliException + } + + // Retry on server errors (5xx) and network-level failures (null status code) + return httpEx.StatusCode is null || (int)httpEx.StatusCode >= 500; + } + private void ThrowUnauthorizedException(HttpRequestException ex) { var serviceContext = _serviceName is not null ? $" ({_serviceName})" : ""; diff --git a/src/Octoshift/Services/GithubClient.cs b/src/Octoshift/Services/GithubClient.cs index 6d846bc8d..8d833ec86 100644 --- a/src/Octoshift/Services/GithubClient.cs +++ b/src/Octoshift/Services/GithubClient.cs @@ -24,9 +24,13 @@ public class GithubClient private const int MILLISECONDS_PER_SECOND = 1000; private const int SECONDARY_RATE_LIMIT_MAX_RETRIES = 3; private const int SECONDARY_RATE_LIMIT_DEFAULT_DELAY = 60; // 60 seconds default delay + private const int SERVER_ERROR_MAX_RETRIES = 5; + private const int SERVER_ERROR_DEFAULT_DELAY = 5; // 5 seconds default delay internal int _secondaryRateLimitMaxRetries = SECONDARY_RATE_LIMIT_MAX_RETRIES; // Exposed for testing purposes internal int _secondaryRateLimitDefaultDelay = SECONDARY_RATE_LIMIT_DEFAULT_DELAY; // Exposed for testing purposes + internal int _serverErrorMaxRetries = SERVER_ERROR_MAX_RETRIES; // Exposed for testing purposes + internal int _serverErrorDefaultDelay = SERVER_ERROR_DEFAULT_DELAY; // Exposed for testing purposes public GithubClient(OctoLogger log, HttpClient httpClient, IVersionProvider versionProvider, RetryPolicy retryPolicy, DateTimeProvider dateTimeProvider, string personalAccessToken) { @@ -202,7 +206,8 @@ public virtual async Task PatchAsync(string url, object body, Dictionary object body = null, HttpStatusCode expectedStatus = HttpStatusCode.OK, Dictionary customHeaders = null, - int retryCount = 0) + int retryCount = 0, + int serverErrorRetryCount = 0) { await ApplyRetryDelayAsync(); _log.LogVerbose($"HTTP {httpMethod}: {url}"); @@ -245,12 +250,18 @@ public virtual async Task PatchAsync(string url, object body, Dictionary // Check for secondary rate limits before handling primary rate limits if (IsSecondaryRateLimit(response.StatusCode, content)) { - return await HandleSecondaryRateLimit(httpMethod, url, body, expectedStatus, customHeaders, headers, retryCount); + return await HandleSecondaryRateLimit(httpMethod, url, body, expectedStatus, customHeaders, headers, retryCount, serverErrorRetryCount); + } + + // Check for transient server errors (502, 503, 504) + if (IsTransientServerError(response.StatusCode)) + { + return await HandleServerError(httpMethod, url, body, expectedStatus, customHeaders, headers, retryCount, serverErrorRetryCount); } if (response.StatusCode == HttpStatusCode.Forbidden && _retryDelay > 0) { - (content, headers) = await SendAsync(httpMethod, url, body, expectedStatus, customHeaders, retryCount); + (content, headers) = await SendAsync(httpMethod, url, body, expectedStatus, customHeaders, retryCount, serverErrorRetryCount); } else if (expectedStatus == HttpStatusCode.OK) { @@ -375,20 +386,21 @@ private bool IsGraphQLSecondaryRateLimit(string content) HttpStatusCode expectedStatus, Dictionary customHeaders, KeyValuePair>[] headers, - int retryCount = 0) + int retryCount = 0, + int serverErrorRetryCount = 0) { if (retryCount >= _secondaryRateLimitMaxRetries) { - throw new OctoshiftCliException($"Secondary rate limit exceeded. Maximum retries ({SECONDARY_RATE_LIMIT_MAX_RETRIES}) reached. Please wait before retrying your request."); + throw new OctoshiftCliException($"Secondary rate limit exceeded. Maximum retries ({_secondaryRateLimitMaxRetries}) reached. Please wait before retrying your request."); } var delaySeconds = GetSecondaryRateLimitDelay(headers, retryCount); - _log.LogWarning($"Secondary rate limit detected (attempt {retryCount + 1}/{SECONDARY_RATE_LIMIT_MAX_RETRIES}). Waiting {delaySeconds} seconds before retrying..."); + _log.LogWarning($"Secondary rate limit detected (attempt {retryCount + 1}/{_secondaryRateLimitMaxRetries}). Waiting {delaySeconds} seconds before retrying..."); await Task.Delay(delaySeconds * MILLISECONDS_PER_SECOND); - return await SendAsync(httpMethod, url, body, expectedStatus, customHeaders, retryCount + 1); + return await SendAsync(httpMethod, url, body, expectedStatus, customHeaders, retryCount + 1, serverErrorRetryCount); } private int GetSecondaryRateLimitDelay(KeyValuePair>[] headers, int retryCount) @@ -417,4 +429,45 @@ private int GetSecondaryRateLimitDelay(KeyValuePair> // Otherwise use exponential backoff: 1m → 2m → 4m return _secondaryRateLimitDefaultDelay * (int)Math.Pow(2, retryCount); } + + private static bool IsTransientServerError(HttpStatusCode statusCode) => + statusCode is HttpStatusCode.BadGateway + or HttpStatusCode.ServiceUnavailable + or HttpStatusCode.GatewayTimeout; + + private async Task<(string Content, KeyValuePair>[] ResponseHeaders)> HandleServerError( + HttpMethod httpMethod, + string url, + object body, + HttpStatusCode expectedStatus, + Dictionary customHeaders, + KeyValuePair>[] headers, + int retryCount, + int serverErrorRetryCount) + { + if (serverErrorRetryCount >= _serverErrorMaxRetries) + { + throw new HttpRequestException($"Server error persisted after {_serverErrorMaxRetries} retries. Please try again later."); + } + + var delaySeconds = GetServerErrorDelay(headers, serverErrorRetryCount); + + _log.LogWarning($"Server error detected (attempt {serverErrorRetryCount + 1}/{_serverErrorMaxRetries}). Waiting {delaySeconds} seconds before retrying..."); + + await Task.Delay(delaySeconds * MILLISECONDS_PER_SECOND); + + return await SendAsync(httpMethod, url, body, expectedStatus, customHeaders, retryCount, serverErrorRetryCount + 1); + } + + private int GetServerErrorDelay(KeyValuePair>[] headers, int serverErrorRetryCount) + { + var retryAfterHeader = ExtractHeaderValue("Retry-After", headers); + if (!string.IsNullOrEmpty(retryAfterHeader) && int.TryParse(retryAfterHeader, out var retryAfterSeconds)) + { + return retryAfterSeconds; + } + + // Exponential backoff: 5s → 10s → 20s → 40s → 80s + return _serverErrorDefaultDelay * (int)Math.Pow(2, serverErrorRetryCount); + } } diff --git a/src/OctoshiftCLI.Tests/Octoshift/RetryPolicyTests.cs b/src/OctoshiftCLI.Tests/Octoshift/RetryPolicyTests.cs new file mode 100644 index 000000000..c4a9e9a0d --- /dev/null +++ b/src/OctoshiftCLI.Tests/Octoshift/RetryPolicyTests.cs @@ -0,0 +1,153 @@ +using System; +using System.Net; +using System.Net.Http; +using System.Threading.Tasks; +using FluentAssertions; +using OctoshiftCLI.Services; +using Xunit; + +namespace OctoshiftCLI.Tests.Octoshift; + +public sealed class RetryPolicyTests +{ + private readonly RetryPolicy _retryPolicy; + + public RetryPolicyTests() + { + _retryPolicy = new RetryPolicy(null) + { + _retryInterval = 0, + _httpRetryInterval = 0 + }; + } + + [Theory] + [InlineData(HttpStatusCode.InternalServerError)] + [InlineData(HttpStatusCode.BadGateway)] + [InlineData(HttpStatusCode.ServiceUnavailable)] + [InlineData(HttpStatusCode.GatewayTimeout)] + public async Task Retry_Retries_On_5xx_HttpRequestException(HttpStatusCode statusCode) + { + // Arrange + var callCount = 0; + + // Act + var result = await _retryPolicy.Retry(async () => + { + callCount++; + if (callCount == 1) + { + throw new HttpRequestException("server error", null, statusCode); + } + return await Task.FromResult("success"); + }); + + // Assert + result.Should().Be("success"); + callCount.Should().Be(2); + } + + [Theory] + [InlineData(HttpStatusCode.BadRequest)] + [InlineData(HttpStatusCode.NotFound)] + [InlineData(HttpStatusCode.Conflict)] + [InlineData(HttpStatusCode.Forbidden)] + public async Task Retry_Does_Not_Retry_On_4xx_HttpRequestException(HttpStatusCode statusCode) + { + // Arrange + var callCount = 0; + + // Act / Assert + await FluentActions + .Invoking(async () => await _retryPolicy.Retry(async () => + { + callCount++; + throw new HttpRequestException("client error", null, statusCode); + })) + .Should() + .ThrowAsync(); + + callCount.Should().Be(1); + } + + [Fact] + public async Task Retry_Retries_On_Null_StatusCode_HttpRequestException() + { + // Arrange - null status code represents network-level failures + var callCount = 0; + + // Act + var result = await _retryPolicy.Retry(async () => + { + callCount++; + if (callCount == 1) + { + throw new HttpRequestException("network error"); + } + return await Task.FromResult("success"); + }); + + // Assert + result.Should().Be("success"); + callCount.Should().Be(2); + } + + [Fact] + public async Task Retry_Retries_On_Non_Http_Exceptions() + { + // Arrange + var callCount = 0; + + // Act + var result = await _retryPolicy.Retry(async () => + { + callCount++; + if (callCount == 1) + { + throw new TimeoutException("timed out"); + } + return await Task.FromResult("success"); + }); + + // Assert + result.Should().Be("success"); + callCount.Should().Be(2); + } + + [Fact] + public async Task Retry_Does_Not_Retry_On_OctoshiftCliException() + { + // Arrange + var callCount = 0; + + // Act / Assert + await FluentActions + .Invoking(async () => await _retryPolicy.Retry(async () => + { + callCount++; + throw new OctoshiftCliException("terminal error"); + })) + .Should() + .ThrowAsync(); + + callCount.Should().Be(1); + } + + [Fact] + public async Task Retry_Throws_OctoshiftCliException_On_401() + { + // Arrange + var callCount = 0; + + // Act / Assert + await FluentActions + .Invoking(async () => await _retryPolicy.Retry(async () => + { + callCount++; + throw new HttpRequestException("unauthorized", null, HttpStatusCode.Unauthorized); + })) + .Should() + .ThrowAsync() + .WithMessage("*Unauthorized*"); + } +} diff --git a/src/OctoshiftCLI.Tests/Octoshift/Services/GithubClientTests.cs b/src/OctoshiftCLI.Tests/Octoshift/Services/GithubClientTests.cs index 5dbc939c6..8210c6764 100644 --- a/src/OctoshiftCLI.Tests/Octoshift/Services/GithubClientTests.cs +++ b/src/OctoshiftCLI.Tests/Octoshift/Services/GithubClientTests.cs @@ -95,7 +95,100 @@ public async Task GetAsync_Retries_On_Non_Success(HttpStatusCode httpStatusCode) .ReturnsAsync(CreateHttpResponseFactory(content: EXPECTED_RESPONSE_CONTENT)); using var httpClient = new HttpClient(handlerMock.Object); - var githubClient = new GithubClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, _dateTimeProvider.Object, PERSONAL_ACCESS_TOKEN); + var githubClient = new GithubClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, _dateTimeProvider.Object, PERSONAL_ACCESS_TOKEN) + { + _serverErrorDefaultDelay = 0 + }; + + // Act + var returnedContent = await githubClient.GetAsync(URL); + + // Assert + returnedContent.Should().Be(EXPECTED_RESPONSE_CONTENT); + } + + [Theory] + [InlineData(HttpStatusCode.BadGateway)] + [InlineData(HttpStatusCode.ServiceUnavailable)] + [InlineData(HttpStatusCode.GatewayTimeout)] + public async Task GetAsync_Retries_Transient_Server_Errors_With_Dedicated_Handler(HttpStatusCode httpStatusCode) + { + // Arrange + var handlerMock = new Mock(); + handlerMock + .Protected() + .SetupSequence>( + "SendAsync", + ItExpr.Is(req => req.Method == HttpMethod.Get), + ItExpr.IsAny()) + .ReturnsAsync(CreateHttpResponseFactory(statusCode: httpStatusCode, content: "SERVER_ERROR")) + .ReturnsAsync(CreateHttpResponseFactory(statusCode: httpStatusCode, content: "SERVER_ERROR")) + .ReturnsAsync(CreateHttpResponseFactory(content: EXPECTED_RESPONSE_CONTENT)); + + using var httpClient = new HttpClient(handlerMock.Object); + var githubClient = new GithubClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, _dateTimeProvider.Object, PERSONAL_ACCESS_TOKEN) + { + _serverErrorDefaultDelay = 0 + }; + + // Act + var returnedContent = await githubClient.GetAsync(URL); + + // Assert + returnedContent.Should().Be(EXPECTED_RESPONSE_CONTENT); + _mockOctoLogger.Verify(m => m.LogWarning(It.Is(s => s.Contains("Server error detected"))), Times.Exactly(2)); + } + + [Fact] + public async Task GetAsync_Throws_After_Max_Server_Error_Retries() + { + // Arrange + var handlerMock = new Mock(); + handlerMock + .Protected() + .Setup>( + "SendAsync", + ItExpr.Is(req => req.Method == HttpMethod.Get), + ItExpr.IsAny()) + .ReturnsAsync(CreateHttpResponseFactory(statusCode: HttpStatusCode.BadGateway, content: "SERVER_ERROR")); + + using var httpClient = new HttpClient(handlerMock.Object); + var githubClient = new GithubClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, _dateTimeProvider.Object, PERSONAL_ACCESS_TOKEN) + { + _serverErrorDefaultDelay = 0, + _serverErrorMaxRetries = 2 + }; + + // Act / Assert + await FluentActions + .Invoking(async () => await githubClient.GetAsync(URL)) + .Should() + .ThrowAsync() + .WithMessage("*Server error persisted after 2 retries*"); + } + + [Fact] + public async Task GetAsync_Server_Error_Respects_Retry_After_Header() + { + // Arrange + var handlerMock = new Mock(); + handlerMock + .Protected() + .SetupSequence>( + "SendAsync", + ItExpr.Is(req => req.Method == HttpMethod.Get), + ItExpr.IsAny()) + .ReturnsAsync(CreateHttpResponseFactory( + statusCode: HttpStatusCode.ServiceUnavailable, + content: "SERVER_ERROR", + headers: new[] { ("Retry-After", "0") })) + .ReturnsAsync(CreateHttpResponseFactory(content: EXPECTED_RESPONSE_CONTENT)); + + using var httpClient = new HttpClient(handlerMock.Object); + var githubClient = new GithubClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, _dateTimeProvider.Object, PERSONAL_ACCESS_TOKEN) + { + _serverErrorDefaultDelay = 0 + }; // Act var returnedContent = await githubClient.GetAsync(URL); @@ -105,7 +198,55 @@ public async Task GetAsync_Retries_On_Non_Success(HttpStatusCode httpStatusCode) } [Fact] - public async Task GetAsync_Bubbles_UnAuthorized_Error() + public async Task GetAsync_Does_Not_Retry_4xx_Errors() + { + // Arrange + var handlerMock = MockHttpHandler( + req => req.Method == HttpMethod.Get, + CreateHttpResponseFactory(HttpStatusCode.BadRequest, content: "BAD_REQUEST")); + + using var httpClient = new HttpClient(handlerMock.Object); + var githubClient = new GithubClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, _dateTimeProvider.Object, PERSONAL_ACCESS_TOKEN); + + // Act / Assert + await FluentActions + .Invoking(async () => await githubClient.GetAsync(URL)) + .Should() + .ThrowExactlyAsync(); + + // Verify only one attempt was made (no retries) + handlerMock + .Protected() + .Verify("SendAsync", Times.Once(), + ItExpr.IsAny(), + ItExpr.IsAny()); + } + + [Fact] + public async Task GetAsync_500_Uses_Generic_Retry_Not_Dedicated_Handler() + { + // Arrange - 500 should NOT go through the dedicated server error handler + var handlerMock = new Mock(); + handlerMock + .Protected() + .SetupSequence>( + "SendAsync", + ItExpr.Is(req => req.Method == HttpMethod.Get), + ItExpr.IsAny()) + .ReturnsAsync(CreateHttpResponseFactory(statusCode: HttpStatusCode.InternalServerError, content: "INTERNAL_ERROR")) + .ReturnsAsync(CreateHttpResponseFactory(content: EXPECTED_RESPONSE_CONTENT)); + + using var httpClient = new HttpClient(handlerMock.Object); + var githubClient = new GithubClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, _dateTimeProvider.Object, PERSONAL_ACCESS_TOKEN); + + // Act + var returnedContent = await githubClient.GetAsync(URL); + + // Assert + returnedContent.Should().Be(EXPECTED_RESPONSE_CONTENT); + // Should NOT log "Server error detected" since 500 goes through generic retry + _mockOctoLogger.Verify(m => m.LogWarning(It.Is(s => s.Contains("Server error detected"))), Times.Never); + } { // Arrange var handlerMock = new Mock(); From 97a8b443280caffaf5f76d8a4cee7daa4c98eb46 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 5 May 2026 21:44:20 +0000 Subject: [PATCH 2/6] Fix missing test method signature in GithubClientTests Agent-Logs-Url: https://github.com/github/gh-gei/sessions/4798917d-6892-4d45-a06e-4991baeb4f61 Co-authored-by: tambling <1431144+tambling@users.noreply.github.com> --- src/OctoshiftCLI.Tests/Octoshift/Services/GithubClientTests.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/OctoshiftCLI.Tests/Octoshift/Services/GithubClientTests.cs b/src/OctoshiftCLI.Tests/Octoshift/Services/GithubClientTests.cs index 8210c6764..f93353699 100644 --- a/src/OctoshiftCLI.Tests/Octoshift/Services/GithubClientTests.cs +++ b/src/OctoshiftCLI.Tests/Octoshift/Services/GithubClientTests.cs @@ -247,6 +247,9 @@ public async Task GetAsync_500_Uses_Generic_Retry_Not_Dedicated_Handler() // Should NOT log "Server error detected" since 500 goes through generic retry _mockOctoLogger.Verify(m => m.LogWarning(It.Is(s => s.Contains("Server error detected"))), Times.Never); } + + [Fact] + public async Task GetAsync_Bubbles_UnAuthorized_Error() { // Arrange var handlerMock = new Mock(); From 371b15a801ae9984d26b61249e730dc9d21bd266 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 6 May 2026 18:27:59 +0000 Subject: [PATCH 3/6] Fix dotnet format failures in RetryPolicyTests.cs Agent-Logs-Url: https://github.com/github/gh-gei/sessions/e40b8e9c-cfb2-471a-b4ef-28d2afbb267b Co-authored-by: tambling <1431144+tambling@users.noreply.github.com> --- .../Octoshift/RetryPolicyTests.cs | 25 +++++++------------ 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/src/OctoshiftCLI.Tests/Octoshift/RetryPolicyTests.cs b/src/OctoshiftCLI.Tests/Octoshift/RetryPolicyTests.cs index c4a9e9a0d..2087bc014 100644 --- a/src/OctoshiftCLI.Tests/Octoshift/RetryPolicyTests.cs +++ b/src/OctoshiftCLI.Tests/Octoshift/RetryPolicyTests.cs @@ -3,7 +3,6 @@ using System.Net.Http; using System.Threading.Tasks; using FluentAssertions; -using OctoshiftCLI.Services; using Xunit; namespace OctoshiftCLI.Tests.Octoshift; @@ -35,11 +34,9 @@ public async Task Retry_Retries_On_5xx_HttpRequestException(HttpStatusCode statu var result = await _retryPolicy.Retry(async () => { callCount++; - if (callCount == 1) - { - throw new HttpRequestException("server error", null, statusCode); - } - return await Task.FromResult("success"); + return callCount == 1 + ? throw new HttpRequestException("server error", null, statusCode) + : await Task.FromResult("success"); }); // Assert @@ -80,11 +77,9 @@ public async Task Retry_Retries_On_Null_StatusCode_HttpRequestException() var result = await _retryPolicy.Retry(async () => { callCount++; - if (callCount == 1) - { - throw new HttpRequestException("network error"); - } - return await Task.FromResult("success"); + return callCount == 1 + ? throw new HttpRequestException("network error") + : await Task.FromResult("success"); }); // Assert @@ -102,11 +97,9 @@ public async Task Retry_Retries_On_Non_Http_Exceptions() var result = await _retryPolicy.Retry(async () => { callCount++; - if (callCount == 1) - { - throw new TimeoutException("timed out"); - } - return await Task.FromResult("success"); + return callCount == 1 + ? throw new TimeoutException("timed out") + : await Task.FromResult("success"); }); // Assert From 98ea63218f973d8c7ee94c4c8fc0fd0f9b49256f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 8 May 2026 20:40:15 +0000 Subject: [PATCH 4/6] fix: remove async from lambdas that only throw to resolve CS1998 build errors Co-authored-by: tambling <1431144+tambling@users.noreply.github.com> --- src/OctoshiftCLI.Tests/Octoshift/RetryPolicyTests.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/OctoshiftCLI.Tests/Octoshift/RetryPolicyTests.cs b/src/OctoshiftCLI.Tests/Octoshift/RetryPolicyTests.cs index 2087bc014..4783372b0 100644 --- a/src/OctoshiftCLI.Tests/Octoshift/RetryPolicyTests.cs +++ b/src/OctoshiftCLI.Tests/Octoshift/RetryPolicyTests.cs @@ -56,7 +56,7 @@ public async Task Retry_Does_Not_Retry_On_4xx_HttpRequestException(HttpStatusCod // Act / Assert await FluentActions - .Invoking(async () => await _retryPolicy.Retry(async () => + .Invoking(async () => await _retryPolicy.Retry(() => { callCount++; throw new HttpRequestException("client error", null, statusCode); @@ -115,7 +115,7 @@ public async Task Retry_Does_Not_Retry_On_OctoshiftCliException() // Act / Assert await FluentActions - .Invoking(async () => await _retryPolicy.Retry(async () => + .Invoking(async () => await _retryPolicy.Retry(() => { callCount++; throw new OctoshiftCliException("terminal error"); @@ -134,7 +134,7 @@ public async Task Retry_Throws_OctoshiftCliException_On_401() // Act / Assert await FluentActions - .Invoking(async () => await _retryPolicy.Retry(async () => + .Invoking(async () => await _retryPolicy.Retry(() => { callCount++; throw new HttpRequestException("unauthorized", null, HttpStatusCode.Unauthorized); From 2e76b0786ff7fbd7bcdb08f63cfdb5659f032d39 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 8 May 2026 20:47:53 +0000 Subject: [PATCH 5/6] fix: use Task.FromException in non-async retry test lambdas Co-authored-by: tambling <1431144+tambling@users.noreply.github.com> --- src/OctoshiftCLI.Tests/Octoshift/RetryPolicyTests.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/OctoshiftCLI.Tests/Octoshift/RetryPolicyTests.cs b/src/OctoshiftCLI.Tests/Octoshift/RetryPolicyTests.cs index 4783372b0..940f1e415 100644 --- a/src/OctoshiftCLI.Tests/Octoshift/RetryPolicyTests.cs +++ b/src/OctoshiftCLI.Tests/Octoshift/RetryPolicyTests.cs @@ -59,7 +59,7 @@ await FluentActions .Invoking(async () => await _retryPolicy.Retry(() => { callCount++; - throw new HttpRequestException("client error", null, statusCode); + return Task.FromException(new HttpRequestException("client error", null, statusCode)); })) .Should() .ThrowAsync(); @@ -118,7 +118,7 @@ await FluentActions .Invoking(async () => await _retryPolicy.Retry(() => { callCount++; - throw new OctoshiftCliException("terminal error"); + return Task.FromException(new OctoshiftCliException("terminal error")); })) .Should() .ThrowAsync(); @@ -137,7 +137,7 @@ await FluentActions .Invoking(async () => await _retryPolicy.Retry(() => { callCount++; - throw new HttpRequestException("unauthorized", null, HttpStatusCode.Unauthorized); + return Task.FromException(new HttpRequestException("unauthorized", null, HttpStatusCode.Unauthorized)); })) .Should() .ThrowAsync() From 20c461494f95dcb65c22f90e321162eac2b49d94 Mon Sep 17 00:00:00 2001 From: Dean Tambling Date: Fri, 8 May 2026 14:39:46 -0700 Subject: [PATCH 6/6] Add carveout for archive generation --- src/Octoshift/RetryPolicy.cs | 12 ++++ .../Octoshift/RetryPolicyTests.cs | 60 +++++++++++++++++++ .../MigrateRepo/MigrateRepoCommandHandler.cs | 2 +- 3 files changed, 73 insertions(+), 1 deletion(-) diff --git a/src/Octoshift/RetryPolicy.cs b/src/Octoshift/RetryPolicy.cs index 452160144..34c098564 100644 --- a/src/Octoshift/RetryPolicy.cs +++ b/src/Octoshift/RetryPolicy.cs @@ -64,6 +64,10 @@ public async Task> RetryOnResult(Func> func, Func Retry(Func> func) => await CreateRetryPolicyForException().ExecuteAsync(func); + public async Task RetryOnAnyException(Func func) => await CreateRetryPolicyForAllExceptions().ExecuteAsync(func); + + public async Task RetryOnAnyException(Func> func) => await CreateRetryPolicyForAllExceptions().ExecuteAsync(func); + private AsyncRetryPolicy CreateRetryPolicyForException() where TException : Exception => Policy .Handle(ex => IsRetryableException(ex)) .WaitAndRetryAsync(5, retry => retry * TimeSpan.FromMilliseconds(_retryInterval), (Exception ex, TimeSpan ts, Context ctx) => @@ -83,6 +87,14 @@ private AsyncRetryPolicy CreateRetryPolicyForException() where TExce _log?.LogVerbose("Retrying..."); }); + private AsyncRetryPolicy CreateRetryPolicyForAllExceptions() => Policy + .Handle() + .WaitAndRetryAsync(5, retry => retry * TimeSpan.FromMilliseconds(_retryInterval), (Exception ex, TimeSpan ts, Context ctx) => + { + _log?.LogVerbose(ex.ToString()); + _log?.LogVerbose("Retrying..."); + }); + private static bool IsRetryableException(Exception ex) { if (ex is OctoshiftCliException) diff --git a/src/OctoshiftCLI.Tests/Octoshift/RetryPolicyTests.cs b/src/OctoshiftCLI.Tests/Octoshift/RetryPolicyTests.cs index 940f1e415..dac894d48 100644 --- a/src/OctoshiftCLI.Tests/Octoshift/RetryPolicyTests.cs +++ b/src/OctoshiftCLI.Tests/Octoshift/RetryPolicyTests.cs @@ -143,4 +143,64 @@ await FluentActions .ThrowAsync() .WithMessage("*Unauthorized*"); } + + [Fact] + public async Task RetryOnAnyException_Retries_On_OctoshiftCliException() + { + // Arrange + var callCount = 0; + + // Act + var result = await _retryPolicy.RetryOnAnyException(async () => + { + callCount++; + return callCount == 1 + ? throw new OctoshiftCliException("transient error") + : await Task.FromResult("success"); + }); + + // Assert + result.Should().Be("success"); + callCount.Should().Be(2); + } + + [Fact] + public async Task RetryOnAnyException_Retries_On_HttpRequestException() + { + // Arrange + var callCount = 0; + + // Act + var result = await _retryPolicy.RetryOnAnyException(async () => + { + callCount++; + return callCount == 1 + ? throw new HttpRequestException("server error", null, HttpStatusCode.BadRequest) + : await Task.FromResult("success"); + }); + + // Assert + result.Should().Be("success"); + callCount.Should().Be(2); + } + + [Fact] + public async Task RetryOnAnyException_Retries_On_TimeoutException() + { + // Arrange + var callCount = 0; + + // Act + var result = await _retryPolicy.RetryOnAnyException(async () => + { + callCount++; + return callCount == 1 + ? throw new TimeoutException("timed out") + : await Task.FromResult("success"); + }); + + // Assert + result.Should().Be("success"); + callCount.Should().Be(2); + } } diff --git a/src/gei/Commands/MigrateRepo/MigrateRepoCommandHandler.cs b/src/gei/Commands/MigrateRepo/MigrateRepoCommandHandler.cs index 138fc0c99..b7c5445a8 100644 --- a/src/gei/Commands/MigrateRepo/MigrateRepoCommandHandler.cs +++ b/src/gei/Commands/MigrateRepo/MigrateRepoCommandHandler.cs @@ -247,7 +247,7 @@ private string ExtractGhesBaseUrl(string ghesApiUrl) bool keepArchive, bool useGithubStorage) { - var (gitArchiveUrl, metadataArchiveUrl, gitArchiveId, metadataArchiveId) = await _retryPolicy.Retry( + var (gitArchiveUrl, metadataArchiveUrl, gitArchiveId, metadataArchiveId) = await _retryPolicy.RetryOnAnyException( async () => await GenerateArchives(githubSourceOrg, sourceRepo, skipReleases, lockSourceRepo)); if (!useGithubStorage && !blobCredentialsRequired)