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); }