diff --git a/BuildMonitor.Test/GitHubRequestOutcomeTests.cs b/BuildMonitor.Test/GitHubRequestOutcomeTests.cs
new file mode 100644
index 0000000..005944c
--- /dev/null
+++ b/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;
+
+///
+/// Tests whether one GitHub API request reports having succeeded.
+///
+///
+/// MakeGitHubRequestAsync 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
+/// 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 the caller supplies, so the failures can be
+/// injected directly and the real method driven, rather than a rule being lifted out of it.
+///
+[TestClass]
+public sealed class GitHubRequestOutcomeTests
+{
+ private const string RequestName = "test/request";
+
+ ///
+ /// A response built to order. Octokit's own Response is internal, and the provider reads
+ /// only the status code and the headers off it.
+ ///
+ private sealed class FakeResponse(HttpStatusCode statusCode, IReadOnlyDictionary headers) : IResponse
+ {
+ public object Body => "{}";
+ public IReadOnlyDictionary Headers { get; } = headers;
+ public ApiInfo ApiInfo => null!;
+ public HttpStatusCode StatusCode { get; } = statusCode;
+ public string ContentType => "application/json";
+ }
+
+ private static ApiException ApiFailure(HttpStatusCode statusCode, IReadOnlyDictionary? headers = null) =>
+ new(new FakeResponse(statusCode, headers ?? new Dictionary()));
+
+ private static Task 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);
+ }
+
+ ///
+ /// The headline case from the issue: rate limited, so the request never reached GitHub.
+ ///
+ [TestMethod]
+ public async Task ARateLimited403ReportsFailure()
+ {
+ GitHub provider = new();
+ ApiException rateLimited = ApiFailure(
+ HttpStatusCode.Forbidden,
+ new Dictionary { ["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);
+ }
+
+ ///
+ /// The other headline case: a plain 403, which is what a just-revoked token looks like.
+ ///
+ [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()));
+
+ 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("alpha"),
+ Token = BuildProviderToken.Create("alpha-pat"),
+ };
+
+ ///
+ /// The workflow actions themselves: a refused request must come back as a failure, not as the
+ /// unconditional success they used to report.
+ ///
+ ///
+ /// 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.
+ ///
+ [TestMethod]
+ public async Task AWorkflowActionRefusedByTheApiReportsFailure()
+ {
+ GitHub provider = new();
+ ApiException rateLimited = ApiFailure(
+ HttpStatusCode.Forbidden,
+ new Dictionary { ["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);
+ }
+
+ ///
+ /// A missing NotFound is still a failure, and is caught by the action rather than escaping.
+ ///
+ [TestMethod]
+ public async Task AWorkflowActionOnAMissingRunReportsFailure()
+ {
+ GitHub provider = new();
+ NotFoundException missing = new(new FakeResponse(HttpStatusCode.NotFound, new Dictionary()));
+
+ 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("beta") },
+ RequestName,
+ () =>
+ {
+ called = true;
+ return Task.CompletedTask;
+ }).ConfigureAwait(false);
+
+ Assert.IsFalse(succeeded);
+ Assert.IsFalse(called, "With no credentials there is nothing to ask GitHub.");
+ }
+
+ ///
+ /// A status this provider has no handling for still propagates, so a caller that genuinely
+ /// cannot continue is not quietly handed a instead.
+ ///
+ [TestMethod]
+ public async Task AnUnhandledStatusStillPropagates()
+ {
+ GitHub provider = new();
+
+ await Assert.ThrowsExactlyAsync(
+ () => RunFailing(provider, ApiFailure(HttpStatusCode.InternalServerError))).ConfigureAwait(false);
+ }
+}
diff --git a/BuildMonitor/Providers/GitHub.cs b/BuildMonitor/Providers/GitHub.cs
index 8d5ffed..b0dd2ab 100644
--- a/BuildMonitor/Providers/GitHub.cs
+++ b/BuildMonitor/Providers/GitHub.cs
@@ -652,8 +652,25 @@ private static List ParseLogForErrors(string logs)
return [.. errors.Take(10)];
}
+ ///
+ /// Runs one GitHub API request, handling the failures this provider knows how to absorb.
+ ///
+ /// The request name, used for logging and in-flight tracking.
+ /// The API call to make.
+ /// The owner whose token the call should use, if any.
+ ///
+ /// if the request completed, if it failed in one
+ /// of the ways handled here. An unhandled status still propagates.
+ ///
+ ///
+ /// 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.
+ ///
[System.Diagnostics.CodeAnalysis.SuppressMessage("Style", "IDE0010:Add missing cases", Justification = "")]
- internal async Task MakeGitHubRequestAsync(string name, Func action, Owner? owner = null)
+ internal async Task MakeGitHubRequestAsync(string name, Func action, Owner? owner = null)
{
await RequestSemaphore.WaitAsync().ConfigureAwait(false);
try
@@ -679,11 +696,13 @@ internal async Task MakeGitHubRequestAsync(string name, Func action, Owner
UpdateRateLimitFromApiInfo();
ClearStatus();
+ return true;
}
catch (AuthorizationException)
{
Log.Error($"{Name}: AuthorizationException for request '{name}'");
OnAuthenticationFailure();
+ return false;
}
catch (ApiException e)
{
@@ -711,11 +730,14 @@ internal async Task MakeGitHubRequestAsync(string name, Func action, Owner
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 @@ private static bool IsRateLimitResponse(ApiException exception)
}
///
- /// Re-runs a workflow run.
+ /// Runs one workflow action and answers whether it actually happened.
///
- /// The workflow run to re-run.
- /// True if the operation was successful, false otherwise.
- internal async Task RerunWorkflowAsync(Run run)
+ /// The owner whose credentials the action runs under.
+ /// The request name, used for logging and in-flight tracking.
+ /// The API call to make.
+ /// only if the request reached GitHub and succeeded.
+ ///
+ /// 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 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.
+ ///
+ internal async Task RunWorkflowActionAsync(Owner owner, string name, Func 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,32 +833,19 @@ internal async Task RerunWorkflowAsync(Run run)
}
}
+ ///
+ /// Re-runs a workflow run.
+ ///
+ /// The workflow run to re-run.
+ /// True if the operation was successful, false otherwise.
+ internal async Task 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);
+
///
/// Cancels a running workflow.
///
/// The workflow run to cancel.
/// True if the operation was successful, false otherwise.
- internal async Task 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 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);
///
/// Triggers a workflow dispatch event.
@@ -835,32 +853,5 @@ internal async Task CancelWorkflowAsync(Run run)
/// The workflow build to trigger.
/// The branch to run the workflow on.
/// True if the operation was successful, false otherwise.
- internal async Task 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()
- };
- 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 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() }).ConfigureAwait(false)).ConfigureAwait(false);
}