Skip to content

Commit cb50123

Browse files
offbyoneoffbyone
committed
Finalizing the async retry PR
- suppress the forced wait while testing
1 parent 9c0d243 commit cb50123

4 files changed

Lines changed: 47 additions & 25 deletions

File tree

‎src/Octoshift/Services/GithubApi.cs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -564,11 +564,11 @@ ... on Migration {
564564
var data = await _client.PostGraphQLWithRetryAsync(url, payload);
565565

566566
return (
567-
State: (string)data["data"]["node"]["state"],
568-
RepositoryName: (string)data["data"]["node"]["repositoryName"],
569-
WarningsCount: (int)data["data"]["node"]["warningsCount"],
570-
FailureReason: (string)data["data"]["node"]["failureReason"],
571-
MigrationLogUrl: (string)data["data"]["node"]["migrationLogUrl"]);
567+
State: (string)data["data"]["node"]["state"],
568+
RepositoryName: (string)data["data"]["node"]["repositoryName"],
569+
WarningsCount: (int)data["data"]["node"]["warningsCount"],
570+
FailureReason: (string)data["data"]["node"]["failureReason"],
571+
MigrationLogUrl: (string)data["data"]["node"]["migrationLogUrl"]);
572572
});
573573
}
574574
catch (Exception ex)

‎src/Octoshift/Services/GithubClient.cs‎

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@ public class GithubClient
2525
private const int SECONDARY_RATE_LIMIT_MAX_RETRIES = 3;
2626
private const int SECONDARY_RATE_LIMIT_DEFAULT_DELAY = 60; // 60 seconds default delay
2727

28+
internal int _secondaryRateLimitMaxRetries = SECONDARY_RATE_LIMIT_MAX_RETRIES; // Exposed for testing purposes
29+
internal int _secondaryRateLimitDefaultDelay = SECONDARY_RATE_LIMIT_DEFAULT_DELAY; // Exposed for testing purposes
30+
2831
public GithubClient(OctoLogger log, HttpClient httpClient, IVersionProvider versionProvider, RetryPolicy retryPolicy, DateTimeProvider dateTimeProvider, string personalAccessToken)
2932
{
3033
_log = log;
@@ -99,17 +102,28 @@ public virtual async Task<JToken> PostGraphQLWithRetryAsync(
99102
Dictionary<string, string> customHeaders = null,
100103
int retryCount = 0)
101104
{
102-
var (response, headers) = await PostWithRetry(url, body, customHeaders);
105+
var currentRetryCount = retryCount;
103106

104-
if (IsGraphQLSecondaryRateLimit(response))
107+
while (true)
105108
{
106-
(response, headers) = await HandleSecondaryRateLimit(HttpMethod.Post, url, body, HttpStatusCode.OK, customHeaders, headers, retryCount);
107-
}
109+
var (response, headers) = await PostWithRetry(url, body, customHeaders);
108110

109-
var data = JObject.Parse(response);
110-
EnsureSuccessGraphQLResponse(data);
111+
if (IsGraphQLSecondaryRateLimit(response))
112+
{
113+
(response, _) = await HandleSecondaryRateLimit(HttpMethod.Post, url, body, HttpStatusCode.OK, customHeaders, headers, currentRetryCount);
111114

112-
return data;
115+
if (IsGraphQLSecondaryRateLimit(response))
116+
{
117+
currentRetryCount++;
118+
continue;
119+
}
120+
}
121+
122+
var data = JObject.Parse(response);
123+
EnsureSuccessGraphQLResponse(data);
124+
125+
return data;
126+
}
113127
}
114128

115129
public virtual async IAsyncEnumerable<JToken> PostGraphQLWithPaginationAsync(
@@ -363,7 +377,7 @@ private bool IsGraphQLSecondaryRateLimit(string content)
363377
KeyValuePair<string, IEnumerable<string>>[] headers,
364378
int retryCount = 0)
365379
{
366-
if (retryCount >= SECONDARY_RATE_LIMIT_MAX_RETRIES)
380+
if (retryCount >= _secondaryRateLimitMaxRetries)
367381
{
368382
throw new OctoshiftCliException($"Secondary rate limit exceeded. Maximum retries ({SECONDARY_RATE_LIMIT_MAX_RETRIES}) reached. Please wait before retrying your request.");
369383
}
@@ -401,6 +415,6 @@ private int GetSecondaryRateLimitDelay(KeyValuePair<string, IEnumerable<string>>
401415
}
402416

403417
// Otherwise use exponential backoff: 1m → 2m → 4m
404-
return SECONDARY_RATE_LIMIT_DEFAULT_DELAY * (int)Math.Pow(2, retryCount);
418+
return _secondaryRateLimitDefaultDelay * (int)Math.Pow(2, retryCount);
405419
}
406420
}

‎src/OctoshiftCLI.Tests/Octoshift/Services/GithubApiTests.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1282,7 +1282,7 @@ ... on Migration {
12821282
}}");
12831283

12841284
_githubClientMock
1285-
.Setup(m => m.PostGraphQLAsync(url, It.Is<object>(x => x.ToJson() == payload.ToJson()), null))
1285+
.Setup(m => m.PostGraphQLWithRetryAsync(url, It.Is<object>(x => x.ToJson() == payload.ToJson()), null, 0))
12861286
.ReturnsAsync(response);
12871287

12881288
// Act
@@ -1341,7 +1341,7 @@ ... on Migration {
13411341
}}");
13421342

13431343
_githubClientMock
1344-
.SetupSequence(m => m.PostGraphQLAsync(url, It.Is<object>(x => x.ToJson() == payload.ToJson()), null))
1344+
.SetupSequence(m => m.PostGraphQLWithRetryAsync(url, It.Is<object>(x => x.ToJson() == payload.ToJson()), null, 0))
13451345
.Throws(new HttpRequestException(null, null, statusCode: HttpStatusCode.BadGateway))
13461346
.Throws(new HttpRequestException(null, null, statusCode: HttpStatusCode.BadGateway))
13471347
.ReturnsAsync(response);
@@ -1399,7 +1399,7 @@ ... on Migration {
13991399
}}");
14001400

14011401
_githubClientMock
1402-
.SetupSequence(m => m.PostGraphQLAsync(url, It.Is<object>(x => x.ToJson() == payload.ToJson()), null))
1402+
.SetupSequence(m => m.PostGraphQLWithRetryAsync(url, It.Is<object>(x => x.ToJson() == payload.ToJson()), null, 0))
14031403
.ReturnsAsync(GQL_ERROR_RESPONSE)
14041404
.ReturnsAsync(GQL_ERROR_RESPONSE)
14051405
.ReturnsAsync(response);
@@ -1457,7 +1457,7 @@ ... on Migration {
14571457
}}");
14581458

14591459
_githubClientMock
1460-
.Setup(m => m.PostGraphQLAsync(url, It.Is<object>(x => x.ToJson() == payload.ToJson()), null))
1460+
.Setup(m => m.PostGraphQLWithRetryAsync(url, It.Is<object>(x => x.ToJson() == payload.ToJson()), null, 0))
14611461
.ReturnsAsync(response);
14621462

14631463
// Act

‎src/OctoshiftCLI.Tests/Octoshift/Services/GithubClientTests.cs‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2160,15 +2160,19 @@ public async Task SendAsync_Uses_Exponential_Backoff_When_No_Retry_Headers()
21602160
.ReturnsAsync(CreateHttpResponseFactory(content: "SUCCESS_RESPONSE")());
21612161

21622162
using var httpClient = new HttpClient(handlerMock.Object);
2163-
var githubClient = new GithubClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, _dateTimeProvider.Object, PERSONAL_ACCESS_TOKEN);
2163+
var githubClient = new GithubClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, _dateTimeProvider.Object, PERSONAL_ACCESS_TOKEN)
2164+
{
2165+
// Set short default delay to speed up the test. This is reflected in the logs below.
2166+
_secondaryRateLimitDefaultDelay = 0
2167+
};
21642168

21652169
// Act
21662170
var result = await githubClient.PatchAsync("http://example.com", _rawRequestBody);
21672171

21682172
// Assert
21692173
result.Should().Be("SUCCESS_RESPONSE");
2170-
_mockOctoLogger.Verify(m => m.LogWarning("Secondary rate limit detected (attempt 1/3). Waiting 60 seconds before retrying..."), Times.Once);
2171-
_mockOctoLogger.Verify(m => m.LogWarning("Secondary rate limit detected (attempt 2/3). Waiting 120 seconds before retrying..."), Times.Once);
2174+
_mockOctoLogger.Verify(m => m.LogWarning("Secondary rate limit detected (attempt 1/3). Waiting 0 seconds before retrying..."), Times.Once);
2175+
_mockOctoLogger.Verify(m => m.LogWarning("Secondary rate limit detected (attempt 2/3). Waiting 0 seconds before retrying..."), Times.Once);
21722176
}
21732177

21742178
[Fact]
@@ -2187,7 +2191,11 @@ public async Task SendAsync_Throws_Exception_After_Max_Secondary_Rate_Limit_Retr
21872191
content: "Too many requests")());
21882192

21892193
using var httpClient = new HttpClient(handlerMock.Object);
2190-
var githubClient = new GithubClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, _dateTimeProvider.Object, PERSONAL_ACCESS_TOKEN);
2194+
var githubClient = new GithubClient(_mockOctoLogger.Object, httpClient, null, _retryPolicy, _dateTimeProvider.Object, PERSONAL_ACCESS_TOKEN)
2195+
{
2196+
// Set short default delay to speed up the test. This is reflected in the logs below.
2197+
_secondaryRateLimitDefaultDelay = 0
2198+
};
21912199

21922200
// Act & Assert
21932201
await FluentActions
@@ -2197,9 +2205,9 @@ await FluentActions
21972205
.WithMessage("Secondary rate limit exceeded. Maximum retries (3) reached. Please wait before retrying your request.");
21982206

21992207
// Verify all retry attempts were logged
2200-
_mockOctoLogger.Verify(m => m.LogWarning("Secondary rate limit detected (attempt 1/3). Waiting 60 seconds before retrying..."), Times.Once);
2201-
_mockOctoLogger.Verify(m => m.LogWarning("Secondary rate limit detected (attempt 2/3). Waiting 120 seconds before retrying..."), Times.Once);
2202-
_mockOctoLogger.Verify(m => m.LogWarning("Secondary rate limit detected (attempt 3/3). Waiting 240 seconds before retrying..."), Times.Once);
2208+
_mockOctoLogger.Verify(m => m.LogWarning("Secondary rate limit detected (attempt 1/3). Waiting 0 seconds before retrying..."), Times.Once);
2209+
_mockOctoLogger.Verify(m => m.LogWarning("Secondary rate limit detected (attempt 2/3). Waiting 0 seconds before retrying..."), Times.Once);
2210+
_mockOctoLogger.Verify(m => m.LogWarning("Secondary rate limit detected (attempt 3/3). Waiting 0 seconds before retrying..."), Times.Once);
22032211
}
22042212

22052213
private object CreateRepositoryMigration(string migrationId = null, string state = RepositoryMigrationStatus.Succeeded) => new

0 commit comments

Comments
 (0)