Honour Retry-After on a GitHub rate limit [patch]

BuildMonitor.Test/GitHubRateLimitResetTests.cs

@@ -0,0 +1,95 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.BuildMonitor.Test;
+
+using System;
+using System.Collections.Generic;
+using System.Globalization;
+using System.Net;
+
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+using Octokit;
+
+/// <summary>
+/// Covers when a rate-limited GitHub request may be retried. Only <c>X-RateLimit-Reset</c> was read,
+/// which on a secondary limit is the end of the primary hourly window, so a limit that asked for a
+/// minute paused polling for the 10-minute cap (ktsu-dev/BuildMonitor#385).
+/// </summary>
+[TestClass]
+public sealed class GitHubRateLimitResetTests
+{
+	private static readonly DateTimeOffset Now = new(2026, 10, 29, 12, 0, 0, TimeSpan.Zero);
+
+	private sealed class FakeResponse(HttpStatusCode statusCode, IReadOnlyDictionary<string, string> headers) : IResponse
+	{
+		public object Body => "{}";
+		public IReadOnlyDictionary<string, string> Headers { get; } = headers;
+		public ApiInfo ApiInfo => null!;
+		public HttpStatusCode StatusCode { get; } = statusCode;
+		public string ContentType => "application/json";
+	}
+
+	private static ApiException ApiFailure(HttpStatusCode statusCode, Dictionary<string, string> headers) =>
+		new(new FakeResponse(statusCode, headers));
+
+	private static string UnixSeconds(DateTimeOffset time) =>
+		time.ToUnixTimeSeconds().ToString(CultureInfo.InvariantCulture);
+
+	/// <summary>
+	/// The case from the issue: a secondary limit with most of the hourly budget left.
+	/// </summary>
+	[TestMethod]
+	public void ASecondaryLimitWaitsForItsRetryAfterRatherThanThePrimaryReset()
+	{
+		ApiException secondary = ApiFailure(HttpStatusCode.Forbidden, new()
+		{
+			["X-RateLimit-Remaining"] = "4000",
+			["X-RateLimit-Reset"] = UnixSeconds(Now.AddMinutes(45)),
+			["Retry-After"] = "60",
+		});
+
+		Assert.AreEqual(Now.AddSeconds(60), GitHub.ParseRateLimitResetTime(secondary, Now));
+	}
+
+	[TestMethod]
+	public void ASpentPrimaryBudgetWaitsForTheReset()
+	{
+		DateTimeOffset reset = Now.AddMinutes(45);
+		ApiException primary = ApiFailure(HttpStatusCode.Forbidden, new()
+		{
+			["X-RateLimit-Remaining"] = "0",
+			["X-RateLimit-Reset"] = UnixSeconds(reset),
+		});
+
+		Assert.AreEqual(reset, GitHub.ParseRateLimitResetTime(primary, Now));
+	}
+
+	/// <summary>
+	/// GitHub's guidance for a secondary limit that names no wait is to back off for at least a
+	/// minute, not until the primary window resets.
+	/// </summary>
+	[TestMethod]
+	public void ASecondaryLimitWithNoRetryAfterBacksOffForAMinute()
+	{
+		ApiException secondary = ApiFailure(HttpStatusCode.Forbidden, new()
+		{
+			["X-RateLimit-Remaining"] = "4000",
+			["X-RateLimit-Reset"] = UnixSeconds(Now.AddMinutes(45)),
+		});
+
+		Assert.AreEqual(Now + GitHub.SecondaryRateLimitBackoff, GitHub.ParseRateLimitResetTime(secondary, Now));
+	}
+
+	[TestMethod]
+	public void ARetryAfterGivenAsAnHttpDateIsHonoured()
+	{
+		DateTimeOffset retryAt = Now.AddSeconds(90);
+		ApiException limited = ApiFailure(HttpStatusCode.TooManyRequests, new()
+		{
+			["Retry-After"] = retryAt.ToString("r", CultureInfo.InvariantCulture),
+		});
+
+		Assert.AreEqual(retryAt, GitHub.ParseRateLimitResetTime(limited, Now));
+	}
+}

BuildMonitor/Providers/GitHub.cs

@@ -93,7 +93,7 @@
 
 	private Owner? OwnerPendingTokenPopup { get; set; }
 
 	internal override void ShowMenu()
 	{
 		if (Hexa.NET.ImGui.ImGui.BeginMenu(Name))
 		{
@@ -204,7 +204,7 @@
 			}
 
 			// Add all organizations the user belongs to
 			IReadOnlyList<Organization> organizations = await CurrentClient!.Organization.GetAllForCurrent().ConfigureAwait(false);
 			foreach (Organization org in organizations)
 			{
 				OwnerName orgName = org.Login.As<OwnerName>();
@@ -219,7 +219,7 @@
 		}).ConfigureAwait(false);
 	}
 
 	internal override async Task UpdateRepositoriesAsync(Owner owner)
 	{
 		if (!HasValidCredentials(owner))
 		{
@@ -247,7 +247,7 @@
 				// Filter to only repos owned by this user (GetAllForCurrent returns repos from all orgs the user has access to)
 				IReadOnlyList<Octokit.Repository> currentUserRepos = await GitHubRepository.GetAllForCurrent().ConfigureAwait(false);
 				int totalCount = currentUserRepos.Count;
 				foreach (Octokit.Repository repo in currentUserRepos)
 				{
 					if (repo.Owner.Login.Equals(owner.Name.ToString(), StringComparison.OrdinalIgnoreCase))
 					{
@@ -268,7 +268,7 @@
 				catch (NotFoundException)
 				{
 					// Owner might be an org-only account, try org repos instead
 					userRepositories = [];
 				}
 
 				// Try to get organization repositories - this only works for orgs
@@ -281,7 +281,7 @@
 				catch (NotFoundException)
 				{
 					// Owner is not an organization, that's fine
 					organizationRepositories = [];
 				}
 			}
 
@@ -306,13 +306,13 @@
 
 				// Get existing repository or create new one
 				bool isNew = false;
 				Repository repository = owner.Repositories.GetOrAdd(repositoryId, _ =>
 				{
 					isNew = true;
 					return owner.CreateRepository(repositoryName, repositoryId);
 				});
 
 				if (isNew)
 				{
 					newRepos++;
 					Log.Info($"GitHub: Discovered repository: {owner.Name}/{repositoryName}");
@@ -347,7 +347,7 @@
 				}
 			}
 
 			if (newRepos > 0 || updatedRepos > 0 || archivedRepos > 0)
 			{
 				Log.Info($"GitHub: Repository update for {owner.Name}: {newRepos} new, {updatedRepos} updated, {archivedRepos} archived removed, {owner.Repositories.Count} total");
 			}
@@ -373,7 +373,7 @@
 
 					// Get existing build or create new one
 					bool isNew = false;
 					Build build = repository.Builds.GetOrAdd(buildId, _ =>
 					{
 						isNew = true;
 						return repository.CreateBuild(buildName, buildId);
@@ -854,22 +854,62 @@
 		}
 	}
 
-	private static DateTimeOffset? ParseRateLimitResetTime(ApiException exception)
+	private static DateTimeOffset? ParseRateLimitResetTime(ApiException exception) =>
+		ParseRateLimitResetTime(exception, DateTimeOffset.UtcNow);
+
+	/// <summary>
+	/// Works out when a rate-limited request may be retried, following GitHub's guidance: honour
+	/// <c>Retry-After</c> when it is sent, wait for <c>X-RateLimit-Reset</c> only when the primary
+	/// budget is spent, and otherwise back off for a minute.
+	/// </summary>
+	/// <remarks>
+	/// A secondary limit can be hit with most of the hourly budget left, and its
+	/// <c>X-RateLimit-Reset</c> is still the end of the primary window, often most of an hour away.
+	/// Reading only that header paused polling for the 10-minute cap rather than the minute or so
+	/// GitHub asked for (ktsu-dev/BuildMonitor#385).
+	/// </remarks>
+	/// <param name="exception">The rate-limited response.</param>
+	/// <param name="now">The current time, which a relative <c>Retry-After</c> counts from.</param>
+	/// <returns>When to retry, or <see langword="null"/> when the response carried no headers.</returns>
+	internal static DateTimeOffset? ParseRateLimitResetTime(ApiException exception, DateTimeOffset now)
 	{
-		if (exception.HttpResponse?.Headers == null)
+		IReadOnlyDictionary<string, string>? headers = exception.HttpResponse?.Headers;
+		if (headers == null)
 		{
 			return null;
 		}
 
-		if (exception.HttpResponse.Headers.TryGetValue("X-RateLimit-Reset", out string? resetValue) &&
+		if (headers.TryGetValue("Retry-After", out string? retryAfterValue))
+		{
+			if (int.TryParse(retryAfterValue, NumberStyles.None, CultureInfo.InvariantCulture, out int retryAfterSeconds))
+			{
+				return now + TimeSpan.FromSeconds(retryAfterSeconds);
+			}
+
+			if (DateTimeOffset.TryParseExact(retryAfterValue, "r", CultureInfo.InvariantCulture, DateTimeStyles.AssumeUniversal, out DateTimeOffset retryAt))
+			{
+				return retryAt;
+			}
+		}
+
+		if (headers.TryGetValue("X-RateLimit-Remaining", out string? remainingValue) &&
+			int.TryParse(remainingValue, CultureInfo.InvariantCulture, out int remaining) &&
+			remaining == 0 &&
+			headers.TryGetValue("X-RateLimit-Reset", out string? resetValue) &&
 			long.TryParse(resetValue, CultureInfo.InvariantCulture, out long unixTimestamp))
 		{
 			return DateTimeOffset.FromUnixTimeSeconds(unixTimestamp);
 		}
 
-		return null;
+		return now + SecondaryRateLimitBackoff;
 	}
 
+	/// <summary>
+	/// How long to back off from a secondary rate limit that names no <c>Retry-After</c>. GitHub asks
+	/// for at least a minute.
+	/// </summary>
+	internal static TimeSpan SecondaryRateLimitBackoff { get; } = TimeSpan.FromMinutes(1);
+
 	private static bool IsRateLimitResponse(ApiException exception)
 	{
 		if (exception.HttpResponse?.Headers == null)