diff --git a/CLAUDE.md b/CLAUDE.md index 0eb5a16..501ef95 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -465,7 +465,9 @@ same 403 to `GitHostingAuthenticationException`. `RateLimitExceededException`, `SecondaryRateLimitExceededException`, and `AbuseException` all derive from `ForbiddenException`, so **arm order is load-bearing**: the three specific ones must precede it. Only `AbuseException` carries a relative delay (`RetryAfterSeconds`) rather than an absolute instant, so it is anchored to -`DateTimeOffset.UtcNow`; `SecondaryRateLimitExceededException` carries no reset time at all. +`DateTimeOffset.UtcNow`. `SecondaryRateLimitExceededException` has no reset property of its own, so +its reset time comes from the response's `Retry-After` header when GitHub sends one, read the same +way as the bare-429 arm below, and is `null` otherwise. **A bare `429` needs a fourth arm, matched on status rather than on type.** Octokit has no dedicated exception for it: a `429` surfaces as a plain `ApiException`, so it reached diff --git a/GitIntegration.Test/Hosting/GitHubProviderTests.cs b/GitIntegration.Test/Hosting/GitHubProviderTests.cs index 6f8ea5b..5caf3fd 100644 --- a/GitIntegration.Test/Hosting/GitHubProviderTests.cs +++ b/GitIntegration.Test/Hosting/GitHubProviderTests.cs @@ -771,7 +771,7 @@ public async Task TranslatesASecondaryRateLimitResponseToGitHostingRateLimitExce // Octokit routes a 403 whose body names a secondary rate limit to // SecondaryRateLimitExceededException, which derives from ForbiddenException. It must // therefore be matched before the ForbiddenException arm, or it would be reported as an - // authentication failure. GitHub sends no reset time with it, so ResetsAt is null. + // authentication failure. This response carries no Retry-After, so ResetsAt is null. using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() .Respond( (HttpStatusCode)403, @@ -788,6 +788,35 @@ public async Task TranslatesASecondaryRateLimitResponseToGitHostingRateLimitExce Assert.IsNull(exception.ResetsAt); } + [TestMethod] + public async Task ReadsResetsAtFromRetryAfterOnASecondaryRateLimitResponseAsync() + { + // SecondaryRateLimitExceededException has no reset property of its own, but GitHub may send + // Retry-After with it. Without that delay a caller can only guess when to retry, and GitHub + // escalates the limit for clients that retry too early. Bracketed rather than exact, because + // the mapping anchors to UtcNow. + using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + .Respond( + (HttpStatusCode)403, + "{\"message\":\"You have exceeded a secondary rate limit\"}", + ("Content-Type", "application/json"), + ("Retry-After", "60")); + GitHubProvider provider = new() { Owner = "contoso".As(), Handler = handler }; + + DateTimeOffset before = DateTimeOffset.UtcNow; + + GitHostingRateLimitException exception = await Assert.ThrowsExactlyAsync( + async () => await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) + .ConfigureAwait(false); + + DateTimeOffset after = DateTimeOffset.UtcNow; + + Assert.AreEqual((HttpStatusCode)403, exception.StatusCode); + StringAssert.Contains(exception.ResponseBody, "secondary rate limit"); + Assert.IsNotNull(exception.ResetsAt); + Assert.IsInRange(before.AddSeconds(60), after.AddSeconds(60), exception.ResetsAt.Value); + } + [TestMethod] public async Task TranslatesAnAbuseDetectionResponseToGitHostingRateLimitExceptionAsync() { diff --git a/GitIntegration/GitHubProvider.cs b/GitIntegration/GitHubProvider.cs index 9d61bf5..7d2f4a7 100644 --- a/GitIntegration/GitHubProvider.cs +++ b/GitIntegration/GitHubProvider.cs @@ -589,7 +589,7 @@ private GitHostingException Translate(ApiException exception) return exception switch { RateLimitExceededException rateLimit => new GitHostingRateLimitException(exception.Message, Name, exception.StatusCode, responseBody, rateLimit.Reset, exception), - SecondaryRateLimitExceededException => new GitHostingRateLimitException(exception.Message, Name, exception.StatusCode, responseBody, resetsAt: null, exception), + SecondaryRateLimitExceededException => new GitHostingRateLimitException(exception.Message, Name, exception.StatusCode, responseBody, ToResetTime(TryGetRetryAfterSeconds(exception)), exception), AbuseException abuse => new GitHostingRateLimitException(exception.Message, Name, exception.StatusCode, responseBody, ToResetTime(abuse.RetryAfterSeconds), exception), { StatusCode: HttpStatusCode.TooManyRequests } => new GitHostingRateLimitException(exception.Message, Name, exception.StatusCode, responseBody, ToResetTime(TryGetRetryAfterSeconds(exception)), exception), AuthorizationException => new GitHostingAuthenticationException(authenticationMessage, Name, exception.StatusCode, responseBody, exception), @@ -600,11 +600,12 @@ private GitHostingException Translate(ApiException exception) } /// - /// Converts an abuse-detection Retry-After delay into the absolute reset time + /// Converts a Retry-After delay into the absolute reset time /// reports. /// /// - /// is a relative delay, and + /// and the Retry-After header a secondary + /// rate limit or a bare 429 may carry are relative delays, and /// is an absolute instant, because that is /// what and Azure DevOps's /// X-RateLimit-Reset both report. Anchoring to loses