fix: report a workflow action that the GitHub API refused as failed [patch]

BuildMonitor.Test/GitHubRequestOutcomeTests.cs

@@ -0,0 +1,217 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.BuildMonitor.Test;
+
+using System;
+using System.Collections.Generic;
+using System.Net;
+using System.Net.Http;
+using System.Threading.Tasks;
+
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+using Octokit;
+
+/// <summary>
+/// Tests whether one GitHub API request reports having succeeded.
+/// </summary>
+/// <remarks>
+/// <c>MakeGitHubRequestAsync</c> absorbs the failures this provider knows how to handle: it sets the
+/// status and logs, but does not rethrow. That is deliberate -- a polling update should not tear
+/// down over a rate limit -- but it means reaching the line after the call says nothing about
+/// whether the call happened. The three workflow actions assumed it did and returned
+/// <see langword="true"/> unconditionally, so cancelling a workflow while rate limited, or after the
+/// token was revoked, reported success and force-refreshed the build while nothing had happened
+/// server-side.
+///
+/// The request body is a <see cref="Func{TResult}"/> the caller supplies, so the failures can be
+/// injected directly and the real method driven, rather than a rule being lifted out of it.
+/// </remarks>
+[TestClass]
+public sealed class GitHubRequestOutcomeTests
+{
+	private const string RequestName = "test/request";
+
+	/// <summary>
+	/// A response built to order. Octokit's own <c>Response</c> is internal, and the provider reads
+	/// only the status code and the headers off it.
+	/// </summary>
+	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, IReadOnlyDictionary<string, string>? headers = null) =>
+		new(new FakeResponse(statusCode, headers ?? new Dictionary<string, string>()));
+
+	private static Task<bool> RunFailing(GitHub provider, Exception failure) =>
+		provider.MakeGitHubRequestAsync(RequestName, () => Task.FromException(failure));
+
+	[TestMethod]
+	public async Task ASuccessfulRequestReportsSuccess()
+	{
+		GitHub provider = new();
+
+		bool succeeded = await provider.MakeGitHubRequestAsync(RequestName, () => Task.CompletedTask).ConfigureAwait(false);
+
+		Assert.IsTrue(succeeded);
+		Assert.AreEqual(ProviderStatus.OK, provider.Status);
+	}
+
+	/// <summary>
+	/// The headline case from the issue: rate limited, so the request never reached GitHub.
+	/// </summary>
+	[TestMethod]
+	public async Task ARateLimited403ReportsFailure()
+	{
+		GitHub provider = new();
+		ApiException rateLimited = ApiFailure(
+			HttpStatusCode.Forbidden,
+			new Dictionary<string, string> { ["X-RateLimit-Remaining"] = "0" });
+
+		bool succeeded = await RunFailing(provider, rateLimited).ConfigureAwait(false);
+
+		Assert.IsFalse(succeeded, "A request refused for rate limiting did not happen, and must not report success.");
+		Assert.AreEqual(ProviderStatus.RateLimited, provider.Status);
+	}
+
+	/// <summary>
+	/// The other headline case: a plain 403, which is what a just-revoked token looks like.
+	/// </summary>
+	[TestMethod]
+	public async Task APlain403ReportsFailure()
+	{
+		GitHub provider = new();
+
+		bool succeeded = await RunFailing(provider, ApiFailure(HttpStatusCode.Forbidden)).ConfigureAwait(false);
+
+		Assert.IsFalse(succeeded, "A request refused for lack of authorization did not happen, and must not report success.");
+		Assert.AreEqual(ProviderStatus.AuthFailed, provider.Status);
+	}
+
+	[TestMethod]
+	public async Task A429ReportsFailure()
+	{
+		GitHub provider = new();
+
+		bool succeeded = await RunFailing(provider, ApiFailure(HttpStatusCode.TooManyRequests)).ConfigureAwait(false);
+
+		Assert.IsFalse(succeeded);
+		Assert.AreEqual(ProviderStatus.RateLimited, provider.Status);
+	}
+
+	[TestMethod]
+	public async Task AnAuthorizationExceptionReportsFailure()
+	{
+		GitHub provider = new();
+		AuthorizationException unauthorized = new(new FakeResponse(HttpStatusCode.Unauthorized, new Dictionary<string, string>()));
+
+		bool succeeded = await RunFailing(provider, unauthorized).ConfigureAwait(false);
+
+		Assert.IsFalse(succeeded);
+		Assert.AreEqual(ProviderStatus.AuthFailed, provider.Status);
+	}
+
+	[TestMethod]
+	public async Task AConnectionErrorReportsFailure()
+	{
+		GitHub provider = new();
+
+		bool succeeded = await RunFailing(provider, new HttpRequestException("connection reset")).ConfigureAwait(false);
+
+		Assert.IsFalse(succeeded);
+		Assert.AreEqual(ProviderStatus.Error, provider.Status);
+	}
+
+	private static Owner OwnerWithToken() => new()
+	{
+		Name = OwnerName.Create<OwnerName>("alpha"),
+		Token = BuildProviderToken.Create<BuildProviderToken>("alpha-pat"),
+	};
+
+	/// <summary>
+	/// The workflow actions themselves: a refused request must come back as a failure, not as the
+	/// unconditional success they used to report.
+	/// </summary>
+	/// <remarks>
+	/// The API call is a delegate, so the refusal is injected and nothing reaches the network. The
+	/// owner carries its own token, which is what gets past the credential guard without the
+	/// provider needing one.
+	/// </remarks>
+	[TestMethod]
+	public async Task AWorkflowActionRefusedByTheApiReportsFailure()
+	{
+		GitHub provider = new();
+		ApiException rateLimited = ApiFailure(
+			HttpStatusCode.Forbidden,
+			new Dictionary<string, string> { ["X-RateLimit-Remaining"] = "0" });
+
+		bool succeeded = await provider.RunWorkflowActionAsync(
+			OwnerWithToken(), RequestName, () => Task.FromException(rateLimited)).ConfigureAwait(false);
+
+		Assert.IsFalse(succeeded, "A cancel or re-run the API refused must not be reported as having worked.");
+		Assert.AreEqual(ProviderStatus.RateLimited, provider.Status);
+	}
+
+	[TestMethod]
+	public async Task AWorkflowActionThatSucceedsReportsSuccess()
+	{
+		GitHub provider = new();
+
+		bool succeeded = await provider.RunWorkflowActionAsync(
+			OwnerWithToken(), RequestName, () => Task.CompletedTask).ConfigureAwait(false);
+
+		Assert.IsTrue(succeeded);
+	}
+
+	/// <summary>
+	/// A missing NotFound is still a failure, and is caught by the action rather than escaping.
+	/// </summary>
+	[TestMethod]
+	public async Task AWorkflowActionOnAMissingRunReportsFailure()
+	{
+		GitHub provider = new();
+		NotFoundException missing = new(new FakeResponse(HttpStatusCode.NotFound, new Dictionary<string, string>()));
+
+		bool succeeded = await provider.RunWorkflowActionAsync(
+			OwnerWithToken(), RequestName, () => Task.FromException(missing)).ConfigureAwait(false);
+
+		Assert.IsFalse(succeeded);
+	}
+
+	[TestMethod]
+	public async Task AWorkflowActionWithoutCredentialsReportsFailureWithoutCalling()
+	{
+		GitHub provider = new();
+		bool called = false;
+
+		bool succeeded = await provider.RunWorkflowActionAsync(
+			new Owner { Name = OwnerName.Create<OwnerName>("beta") },
+			RequestName,
+			() =>
+			{
+				called = true;
+				return Task.CompletedTask;
+			}).ConfigureAwait(false);
+
+		Assert.IsFalse(succeeded);
+		Assert.IsFalse(called, "With no credentials there is nothing to ask GitHub.");
+	}
+
+	/// <summary>
+	/// A status this provider has no handling for still propagates, so a caller that genuinely
+	/// cannot continue is not quietly handed a <see langword="false"/> instead.
+	/// </summary>
+	[TestMethod]
+	public async Task AnUnhandledStatusStillPropagates()
+	{
+		GitHub provider = new();
+
+		await Assert.ThrowsExactlyAsync<ApiException>(
+			() => RunFailing(provider, ApiFailure(HttpStatusCode.InternalServerError))).ConfigureAwait(false);
+	}
+}

BuildMonitor/Providers/GitHub.cs

@@ -93,7 +93,7 @@
 
 	private Owner? OwnerPendingTokenPopup { get; set; }
 
 	internal override void ShowMenu()
 	{
 		if (Hexa.NET.ImGui.ImGui.BeginMenu(Name))
 		{
@@ -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 = [];
 				}
 			}
 
@@ -652,8 +652,25 @@
 		return [.. errors.Take(10)];
 	}
 
+	/// <summary>
+	/// Runs one GitHub API request, handling the failures this provider knows how to absorb.
+	/// </summary>
+	/// <param name="name">The request name, used for logging and in-flight tracking.</param>
+	/// <param name="action">The API call to make.</param>
+	/// <param name="owner">The owner whose token the call should use, if any.</param>
+	/// <returns>
+	/// <see langword="true"/> if the request completed, <see langword="false"/> if it failed in one
+	/// of the ways handled here. An unhandled <see cref="ApiException"/> status still propagates.
+	/// </returns>
+	/// <remarks>
+	/// The answer matters because the failures handled here are handled silently: the status is set
+	/// and the failure is logged, but nothing is rethrown. A caller that assumed reaching the next
+	/// line meant success therefore reported success for a request that never happened -- which is
+	/// what the workflow actions did, telling the user a re-run or cancel had worked while the
+	/// provider was rate limited or its token had just been revoked.
+	/// </remarks>
 	[System.Diagnostics.CodeAnalysis.SuppressMessage("Style", "IDE0010:Add missing cases", Justification = "<Pending>")]
-	internal async Task MakeGitHubRequestAsync(string name, Func<Task> action, Owner? owner = null)
+	internal async Task<bool> MakeGitHubRequestAsync(string name, Func<Task> action, Owner? owner = null)
 	{
 		await RequestSemaphore.WaitAsync().ConfigureAwait(false);
 		try
@@ -679,11 +696,13 @@
 				UpdateRateLimitFromApiInfo();
 
 				ClearStatus();
+				return true;
 			}
 			catch (AuthorizationException)
 			{
 				Log.Error($"{Name}: AuthorizationException for request '{name}'");
 				OnAuthenticationFailure();
+				return false;
 			}
 			catch (ApiException e)
 			{
@@ -711,11 +730,14 @@
 						Log.Error($"{Name}: ApiException ({e.HttpResponse?.StatusCode}) for request '{name}' - {e.Message}");
 						throw;
 				}
+
+				return false;
 			}
 			catch (HttpRequestException ex)
 			{
 				Log.Error($"{Name}: Connection error - {ex.Message}");
 				SetStatus(ProviderStatus.Error, $"{Strings.ConnectionErrorMessage} {ex.Message}");
+				return false;
 			}
 		}
 		finally
@@ -776,21 +798,30 @@
 	}
 
 	/// <summary>
-	/// Re-runs a workflow run.
+	/// Runs one workflow action and answers whether it actually happened.
 	/// </summary>
-	/// <param name="run">The workflow run to re-run.</param>
-	/// <returns>True if the operation was successful, false otherwise.</returns>
-	internal async Task<bool> RerunWorkflowAsync(Run run)
+	/// <param name="owner">The owner whose credentials the action runs under.</param>
+	/// <param name="name">The request name, used for logging and in-flight tracking.</param>
+	/// <param name="action">The API call to make.</param>
+	/// <returns><see langword="true"/> only if the request reached GitHub and succeeded.</returns>
+	/// <remarks>
+	/// The three workflow actions had this guard and these catch blocks copied between them, and all
+	/// three read reaching the line after the request as success. They did not: the failures this
+	/// provider handles are swallowed by <see cref="MakeGitHubRequestAsync"/> without rethrowing, so
+	/// the catch blocks never fired for them and the action reported a success that never happened.
+	/// One copy, returning what the request returned, is what keeps that answer honest -- and taking
+	/// the call as a delegate is what lets a test drive the refusal without reaching the network.
+	/// </remarks>
+	internal async Task<bool> RunWorkflowActionAsync(Owner owner, string name, Func<Task> action)
 	{
-		if (!HasValidCredentials(run.Owner))
+		if (!HasValidCredentials(owner))
 		{
 			return false;
 		}
 
 		try
 		{
-			await MakeGitHubRequestAsync($"{Name}/{run.Owner.Name}/{run.Repository.Name}/rerun/{run.Id}", async () => await GitHubRuns.Rerun(run.Owner.Name, run.Repository.Name, long.Parse(run.Id, CultureInfo.InvariantCulture)).ConfigureAwait(false), run.Owner).ConfigureAwait(false);
-			return true;
+			return await MakeGitHubRequestAsync(name, action, owner).ConfigureAwait(false);
 		}
 		catch (NotFoundException)
 		{
@@ -802,65 +833,25 @@
 		}
 	}
 
+	/// <summary>
+	/// Re-runs a workflow run.
+	/// </summary>
+	/// <param name="run">The workflow run to re-run.</param>
+	/// <returns>True if the operation was successful, false otherwise.</returns>
+	internal async Task<bool> RerunWorkflowAsync(Run run) => await RunWorkflowActionAsync(run.Owner, $"{Name}/{run.Owner.Name}/{run.Repository.Name}/rerun/{run.Id}", async () => await GitHubRuns.Rerun(run.Owner.Name, run.Repository.Name, long.Parse(run.Id, CultureInfo.InvariantCulture)).ConfigureAwait(false)).ConfigureAwait(false);
+
 	/// <summary>
 	/// Cancels a running workflow.
 	/// </summary>
 	/// <param name="run">The workflow run to cancel.</param>
 	/// <returns>True if the operation was successful, false otherwise.</returns>
-	internal async Task<bool> CancelWorkflowAsync(Run run)
-	{
-		if (!HasValidCredentials(run.Owner))
-		{
-			return false;
-		}
-
-		try
-		{
-			await MakeGitHubRequestAsync($"{Name}/{run.Owner.Name}/{run.Repository.Name}/cancel/{run.Id}", async () => await GitHubRuns.Cancel(run.Owner.Name, run.Repository.Name, long.Parse(run.Id, CultureInfo.InvariantCulture)).ConfigureAwait(false), run.Owner).ConfigureAwait(false);
-			return true;
-		}
-		catch (NotFoundException)
-		{
-			return false;
-		}
-		catch (ApiException)
-		{
-			return false;
-		}
-	}
+	internal async Task<bool> CancelWorkflowAsync(Run run) => await RunWorkflowActionAsync(run.Owner, $"{Name}/{run.Owner.Name}/{run.Repository.Name}/cancel/{run.Id}", async () => await GitHubRuns.Cancel(run.Owner.Name, run.Repository.Name, long.Parse(run.Id, CultureInfo.InvariantCulture)).ConfigureAwait(false)).ConfigureAwait(false);
 
 	/// <summary>
 	/// Triggers a workflow dispatch event.
 	/// </summary>
 	/// <param name="build">The workflow build to trigger.</param>
 	/// <param name="branch">The branch to run the workflow on.</param>
 	/// <returns>True if the operation was successful, false otherwise.</returns>
-	internal async Task<bool> TriggerWorkflowAsync(Build build, BranchName branch)
-	{
-		if (!HasValidCredentials(build.Owner))
-		{
-			return false;
-		}
-
-		try
-		{
-			await MakeGitHubRequestAsync($"{Name}/{build.Owner.Name}/{build.Repository.Name}/dispatch/{build.Name}", async () =>
-			{
-				CreateWorkflowDispatch createWorkflowDispatch = new(branch)
-				{
-					Inputs = new Dictionary<string, object>()
-				};
-				await GitHubActions.Workflows.CreateDispatch(build.Owner.Name, build.Repository.Name, long.Parse(build.Id, CultureInfo.InvariantCulture), createWorkflowDispatch).ConfigureAwait(false);
-			}, build.Owner).ConfigureAwait(false);
-			return true;
-		}
-		catch (NotFoundException)
-		{
-			return false;
-		}
-		catch (ApiException)
-		{
-			return false;
-		}
-	}
+	internal async Task<bool> TriggerWorkflowAsync(Build build, BranchName branch) => await RunWorkflowActionAsync(build.Owner, $"{Name}/{build.Owner.Name}/{build.Repository.Name}/dispatch/{build.Name}", async () => await GitHubActions.Workflows.CreateDispatch(build.Owner.Name, build.Repository.Name, long.Parse(build.Id, CultureInfo.InvariantCulture), new CreateWorkflowDispatch(branch) { Inputs = new Dictionary<string, object>() }).ConfigureAwait(false)).ConfigureAwait(false);
 }