From ea0475f08fcc334f7a7150df2239914414ce8d7a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 19 Oct 2026 16:36:29 +0000 Subject: [PATCH] Fall back to X-RateLimit-Reset for a GitHub 429 or secondary limit with no Retry-After [patch] A primary rate limit answered with a 429 arrives from Octokit as a plain ApiException, because RateLimitExceededException is built only for a 403, so the bare-429 arm found no Retry-After and reported ResetsAt = null while the reset instant sat in X-RateLimit-Reset. The 429 and secondary arms now use Retry-After first and then, when X-RateLimit-Remaining is 0, the X-RateLimit-Reset instant, read case-insensitively and range-checked as the Azure DevOps provider reads it. Fixes #268 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KqPHUs2CJGAWm9XxdTQbwC --- CLAUDE.md | 11 ++- .../Hosting/GitHubProviderTests.cs | 91 +++++++++++++++++++ GitIntegration/GitHubProvider.cs | 56 +++++++++++- 3 files changed, 152 insertions(+), 6 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 501ef95..1737de6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -466,8 +466,7 @@ same 403 to `GitHostingAuthenticationException`. `RateLimitExceededException`, **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` 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. +its reset time is read the same way as the bare-429 arm below. **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 @@ -478,8 +477,12 @@ specific claim and a 429 carried by one of those subtypes should still be answer Its reset time comes from the response's `Retry-After` header, read case-insensitively rather than by key: HTTP header names are case-insensitive and Octokit's header dictionary compares ordinally, so a keyed lookup would work only because `HttpResponseMessage` happens to canonicalise this header's -casing. A `Retry-After` carrying an HTTP date rather than seconds yields `null`, since an unset -`ResetsAt` is better than an invented one. +casing. A `Retry-After` carrying an HTTP date rather than seconds is not used, since an unset +`ResetsAt` is better than an invented one. Without a usable `Retry-After`, a response reporting its +quota spent (`X-RateLimit-Remaining: 0`) gives `X-RateLimit-Reset`, GitHub's documented fallback, read +case-insensitively and range-checked as Azure DevOps reads it. That is how a primary limit answered +with a `429` arrives, because Octokit builds `RateLimitExceededException` only for a `403`. With +neither header usable, `ResetsAt` is `null`. **A repository from a hosting provider carries no `LocalPath`, and how it is addressed is the provider's own decision.** `GitRepository.LocalPath` is nullable because a repository a provider enumerated has never been diff --git a/GitIntegration.Test/Hosting/GitHubProviderTests.cs b/GitIntegration.Test/Hosting/GitHubProviderTests.cs index 5caf3fd..ba3b251 100644 --- a/GitIntegration.Test/Hosting/GitHubProviderTests.cs +++ b/GitIntegration.Test/Hosting/GitHubProviderTests.cs @@ -899,6 +899,97 @@ public async Task LeavesResetsAtNullWhenATooManyRequestsResponseCarriesNoRetryAf Assert.IsNull(exception.ResetsAt); } + [TestMethod] + public async Task ReadsResetsAtFromXRateLimitResetOnATooManyRequestsResponseWithASpentQuotaAsync() + { + // GitHub may answer a spent primary quota with a 429 rather than a 403. Octokit builds + // RateLimitExceededException, which carries Reset, only for a 403, so this arrives as a plain + // ApiException with no Retry-After. The reset instant is still in the headers. + using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + .Respond( + HttpStatusCode.TooManyRequests, + "{\"message\":\"You have exceeded a rate limit\"}", + ("Content-Type", "application/json"), + ("X-RateLimit-Remaining", "0"), + ("X-RateLimit-Reset", "1798800000")); + GitHubProvider provider = new() { Owner = "contoso".As(), Handler = handler }; + + GitHostingRateLimitException exception = await Assert.ThrowsExactlyAsync( + async () => await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) + .ConfigureAwait(false); + + Assert.AreEqual(HttpStatusCode.TooManyRequests, exception.StatusCode); + Assert.AreEqual(DateTimeOffset.FromUnixTimeSeconds(1798800000), exception.ResetsAt); + } + + [TestMethod] + public async Task ReadsResetsAtFromXRateLimitResetOnASecondaryRateLimitResponseWithASpentQuotaAsync() + { + // GitHub's documented order for a secondary limit is Retry-After, then X-RateLimit-Reset when + // X-RateLimit-Remaining is 0. + using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + .Respond( + (HttpStatusCode)403, + "{\"message\":\"You have exceeded a secondary rate limit\"}", + ("Content-Type", "application/json"), + ("x-ratelimit-remaining", "0"), + ("x-ratelimit-reset", "1798800000")); + GitHubProvider provider = new() { Owner = "contoso".As(), Handler = handler }; + + GitHostingRateLimitException exception = await Assert.ThrowsExactlyAsync( + async () => await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) + .ConfigureAwait(false); + + StringAssert.Contains(exception.ResponseBody, "secondary rate limit"); + Assert.AreEqual(DateTimeOffset.FromUnixTimeSeconds(1798800000), exception.ResetsAt); + } + + [TestMethod] + public async Task PrefersRetryAfterOverXRateLimitResetAsync() + { + using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + .Respond( + HttpStatusCode.TooManyRequests, + "{\"message\":\"You have exceeded a rate limit\"}", + ("Content-Type", "application/json"), + ("Retry-After", "90"), + ("X-RateLimit-Remaining", "0"), + ("X-RateLimit-Reset", "1798800000")); + 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.IsNotNull(exception.ResetsAt); + Assert.IsInRange(before.AddSeconds(90), after.AddSeconds(90), exception.ResetsAt.Value); + } + + [TestMethod] + public async Task LeavesResetsAtNullWhenTheQuotaIsNotReportedSpentAsync() + { + // GitHub sends X-RateLimit-Reset on every response. The reset of a quota that still has + // requests left says nothing about when this one may be retried. + using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + .Respond( + HttpStatusCode.TooManyRequests, + "{\"message\":\"You have exceeded a rate limit\"}", + ("Content-Type", "application/json"), + ("X-RateLimit-Remaining", "4999"), + ("X-RateLimit-Reset", "1798800000")); + GitHubProvider provider = new() { Owner = "contoso".As(), Handler = handler }; + + GitHostingRateLimitException exception = await Assert.ThrowsExactlyAsync( + async () => await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) + .ConfigureAwait(false); + + Assert.IsNull(exception.ResetsAt); + } + [TestMethod] public async Task LeavesResetsAtNullWhenRetryAfterCarriesAnHttpDateAsync() { diff --git a/GitIntegration/GitHubProvider.cs b/GitIntegration/GitHubProvider.cs index 7d2f4a7..d5d2e15 100644 --- a/GitIntegration/GitHubProvider.cs +++ b/GitIntegration/GitHubProvider.cs @@ -589,9 +589,9 @@ 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, ToResetTime(TryGetRetryAfterSeconds(exception)), exception), + SecondaryRateLimitExceededException => new GitHostingRateLimitException(exception.Message, Name, exception.StatusCode, responseBody, TryGetResetTime(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), + { StatusCode: HttpStatusCode.TooManyRequests } => new GitHostingRateLimitException(exception.Message, Name, exception.StatusCode, responseBody, TryGetResetTime(exception), exception), AuthorizationException => new GitHostingAuthenticationException(authenticationMessage, Name, exception.StatusCode, responseBody, exception), ForbiddenException => new GitHostingAuthenticationException(authenticationMessage, Name, exception.StatusCode, responseBody, exception), NotFoundException => new GitHostingNotFoundException(exception.Message, Name, exception.StatusCode, responseBody, exception), @@ -618,6 +618,58 @@ private GitHostingException Translate(ApiException exception) private static DateTimeOffset? ToResetTime(int? retryAfterSeconds) => retryAfterSeconds is int seconds ? DateTimeOffset.UtcNow.AddSeconds(seconds) : null; + /// + /// Reads when a rate-limited response says the limit lifts, for the arms whose exception carries + /// no reset of its own. + /// + /// + /// The order is GitHub's documented one. A Retry-After delay comes first. Failing that, a + /// response that reports its quota spent (X-RateLimit-Remaining: 0) carries the instant it + /// refills in X-RateLimit-Reset. That is how a primary limit answered with a 429 + /// arrives: Octokit builds , with its + /// , only for a 403, so without the fallback the + /// same exhausted quota reported a reset time at one status and none at the other. + /// + /// The failure Octokit reported. + /// The absolute reset time, or when neither header is usable. + private static DateTimeOffset? TryGetResetTime(ApiException exception) => + ToResetTime(TryGetRetryAfterSeconds(exception)) ?? TryGetRateLimitReset(exception); + + /// + /// Reads the instant a spent quota refills from a failed response's X-RateLimit-Reset header. + /// + /// + /// Read only when X-RateLimit-Remaining is 0, because GitHub sends both headers on + /// every response and the reset time of a quota that is not spent says nothing about when this + /// request may be retried. Scanned case-insensitively for the reason + /// gives, and range-checked as Azure DevOps's reading of the + /// same header is, so a value cannot hold yields + /// rather than an exception thrown out of the translation of another one. + /// + /// The failure Octokit reported. + /// The reset instant, or when the quota is not reported spent or the header is absent or unusable. + private static DateTimeOffset? TryGetRateLimitReset(ApiException exception) + { + if (exception.HttpResponse?.Headers is not IReadOnlyDictionary headers) + { + return null; + } + + string? HeaderValue(string name) => headers + .FirstOrDefault(candidate => candidate.Key.Equals(name, StringComparison.OrdinalIgnoreCase)) + .Value; + + bool spent = long.TryParse(HeaderValue("X-RateLimit-Remaining"), NumberStyles.Integer, CultureInfo.InvariantCulture, out long remaining) + && remaining == 0; + + return spent + && long.TryParse(HeaderValue("X-RateLimit-Reset"), NumberStyles.Integer, CultureInfo.InvariantCulture, out long epochSeconds) + && epochSeconds >= DateTimeOffset.MinValue.ToUnixTimeSeconds() + && epochSeconds <= DateTimeOffset.MaxValue.ToUnixTimeSeconds() + ? DateTimeOffset.FromUnixTimeSeconds(epochSeconds) + : null; + } + /// /// Reads a failed response's Retry-After delay, in seconds. ///