diff --git a/BuildMonitor.Test/SyncFailureTests.cs b/BuildMonitor.Test/SyncFailureTests.cs new file mode 100644 index 0000000..9e7e044 --- /dev/null +++ b/BuildMonitor.Test/SyncFailureTests.cs @@ -0,0 +1,174 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.BuildMonitor.Test; + +using ktsu.Semantics.Strings; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +/// +/// Covers what happens when a provider update throws — a persistent GitHub 5xx, for example. The +/// sync's timer used to be left running, so ShouldUpdate stayed true and the update loop +/// polled the same item again back to back instead of on its interval, and the exception faulted +/// the whole batch (ktsu-dev/BuildMonitor#299). +/// +[TestClass] +public sealed class SyncFailureTests +{ + /// + /// Long enough for a timer that was not restarted to show a visibly shorter time remaining. + /// + private static readonly TimeSpan Elapse = TimeSpan.FromMilliseconds(1100); + + /// + /// A provider whose build and run updates throw for the items named in + /// and record the rest. + /// + private sealed class FailingProvider : BuildProvider + { + internal override BuildProviderName Name { get; } = "Failing".As(); + + internal HashSet Failing { get; } = []; + + internal List Updated { get; } = []; + + internal override Task UpdateRepositoriesAsync(Owner owner) => Task.CompletedTask; + internal override Task UpdateBuildsAsync(Repository repository) => Task.CompletedTask; + internal override Task UpdateBuildAsync(Build build) => Record(build.Name); + internal override Task UpdateRunAsync(Run run) => Record(run.Name); + + private Task Record(string name) + { + if (Failing.Contains(name)) + { + throw new HttpRequestException("502 Bad Gateway"); + } + + lock (Updated) + { + Updated.Add(name); + } + + return Task.CompletedTask; + } + } + + private static Build AddBuild(Repository repository, string name) + { + Build build = repository.CreateBuild(name.As()); + Assert.IsTrue(repository.Builds.TryAdd(build.Id, build)); + return build; + } + + private static Repository CreateRepository(FailingProvider provider) + { + Owner owner = provider.CreateOwner("owner".As()); + return owner.CreateRepository("repo".As()); + } + + [TestMethod] + public async Task AFailedBuildUpdateStillRestartsItsTimer() + { + // Arrange + FailingProvider provider = new(); + provider.Failing.Add("broken"); + BuildSync sync = new() { Build = AddBuild(CreateRepository(provider), "broken") }; + await Task.Delay(Elapse, TestContext.CancellationToken).ConfigureAwait(false); + TimeSpan remainingBefore = sync.TimeRemaining; + + // Act + await sync.UpdateAsync().ConfigureAwait(false); + + // Assert + Assert.IsGreaterThan(remainingBefore, sync.TimeRemaining, "A failed update should restart the timer, so the build waits out its interval before the next poll"); + Assert.IsFalse(sync.ShouldUpdate); + } + + [TestMethod] + public async Task AFailedBuildUpdateDoesNotStopTheOthersInTheBatch() + { + // Arrange + FailingProvider provider = new(); + provider.Failing.Add("broken"); + Repository repository = CreateRepository(provider); + BuildSync broken = new() { Build = AddBuild(repository, "broken") }; + BuildSync healthy = new() { Build = AddBuild(repository, "healthy") }; + + // Act + Task batch = Task.WhenAll(broken.UpdateAsync(), healthy.UpdateAsync()); + await batch.ConfigureAwait(false); + + // Assert + Assert.IsFalse(batch.IsFaulted, "One failing build should not fault the batch, which skips the timer restarts and pruning after it"); + Assert.HasCount(1, provider.Updated); + Assert.AreEqual("healthy", provider.Updated[0], "The healthy build should still be polled"); + } + + [TestMethod] + public async Task AFailedRunUpdateDoesNotStopTheOthersInTheBatch() + { + // Arrange + FailingProvider provider = new(); + provider.Failing.Add("broken"); + Build build = AddBuild(CreateRepository(provider), "build"); + RunSync broken = new() { Run = AddRun(build, "broken") }; + RunSync healthy = new() { Run = AddRun(build, "healthy") }; + + // Act + Task batch = Task.WhenAll(broken.UpdateAsync(), healthy.UpdateAsync()); + await batch.ConfigureAwait(false); + + // Assert + Assert.IsFalse(batch.IsFaulted, "One failing run should not fault the batch, which skips the pruning after it"); + Assert.HasCount(1, provider.Updated); + Assert.AreEqual("healthy", provider.Updated[0], "The healthy run should still be polled"); + Assert.IsFalse(broken.ShouldUpdate, "The failed run should wait out its interval before the next poll"); + } + + [TestMethod] + public async Task AFailedDiscoveryItemDoesNotStopTheRest() + { + // Arrange + List discovered = []; + + // Act + Task batch = SyncGuard.RunAllAsync( + ["first", "broken", "last"], + name => + { + if (name == "broken") + { + throw new HttpRequestException("503 Service Unavailable"); + } + + lock (discovered) + { + discovered.Add(name); + } + + return Task.CompletedTask; + }, + name => $"discovery for {name}"); + await batch.ConfigureAwait(false); + + // Assert + Assert.IsFalse(batch.IsFaulted, "One failing item should not fault discovery, which would skip the refresh timer restart after it"); + Assert.HasCount(2, discovered); + Assert.Contains("first", discovered); + Assert.Contains("last", discovered); + Assert.Contains(e => e.Message.Contains("discovery for broken", StringComparison.Ordinal), Log.GetEntries(), "The failure should be logged"); + } + + private static Run AddRun(Build build, string name) + { + Run run = build.CreateRun(name.As()); + run.Status = RunStatus.Running; + Assert.IsTrue(build.Runs.TryAdd(run.Id, run)); + return run; + } + + /// + /// Gets or sets the test context MSTest injects, used for its cancellation token. + /// + public TestContext TestContext { get; set; } = null!; +} diff --git a/BuildMonitor/BuildMonitor.cs b/BuildMonitor/BuildMonitor.cs index 04eb97b..1431aa2 100644 --- a/BuildMonitor/BuildMonitor.cs +++ b/BuildMonitor/BuildMonitor.cs @@ -1523,7 +1523,9 @@ private static async Task UpdateAsync() } // Update repositories concurrently (semaphore limits per-provider concurrency) - await Task.WhenAll(allOwners.Select(x => x.Provider.UpdateRepositoriesAsync(x.Owner))).ConfigureAwait(false); + // Each owner is guarded so one that fails does not fault the batch, which would skip the + // ProviderRefreshTimer restart below and re-run discovery straight away (ktsu-dev/BuildMonitor#299). + await SyncGuard.RunAllAsync(allOwners, x => x.Provider.UpdateRepositoriesAsync(x.Owner), x => $"{x.Provider.Name}: repository discovery for {x.Owner.Name}").ConfigureAwait(false); // Gather all repositories across all owners List<(BuildProvider Provider, Repository Repository)> allRepositories = []; @@ -1536,7 +1538,8 @@ private static async Task UpdateAsync() } // Update builds concurrently (semaphore limits per-provider concurrency) - await Task.WhenAll(allRepositories.Select(async x => + // Each repository is guarded for the same reason as each owner above. + await SyncGuard.RunAllAsync(allRepositories, async x => { await x.Provider.UpdateBuildsAsync(x.Repository).ConfigureAwait(false); @@ -1548,7 +1551,7 @@ await Task.WhenAll(allRepositories.Select(async x => Build = build, }); } - })).ConfigureAwait(false); + }, x => $"{x.Provider.Name}: build discovery for {x.Repository.Owner.Name}/{x.Repository.Name}").ConfigureAwait(false); ProviderRefreshTimer.Restart(); } diff --git a/BuildMonitor/BuildSync.cs b/BuildMonitor/BuildSync.cs index a761be4..5a0d8a2 100644 --- a/BuildMonitor/BuildSync.cs +++ b/BuildMonitor/BuildSync.cs @@ -80,6 +80,20 @@ internal RequestPriority Priority internal void ResetTimer() => UpdateTimer.Restart(); internal async Task UpdateAsync() + { + // Restart the timer whether or not the update succeeds. A failed update that left it running + // kept ShouldUpdate true, so the build was polled again back to back (ktsu-dev/BuildMonitor#299). + try + { + _ = await SyncGuard.RunAsync(UpdateBuildAsync, $"BuildSync: {Build.Owner.Name}/{Build.Repository.Name}/{Build.Name}").ConfigureAwait(false); + } + finally + { + UpdateTimer.Restart(); + } + } + + private async Task UpdateBuildAsync() { int runsBefore = Build.Runs.Count; await Build.Owner.BuildProvider.UpdateBuildAsync(Build).ConfigureAwait(false); @@ -101,7 +115,5 @@ internal async Task UpdateAsync() Run = run, }); } - - UpdateTimer.Restart(); } } diff --git a/BuildMonitor/RunSync.cs b/BuildMonitor/RunSync.cs index 3bc9532..baa8f4c 100644 --- a/BuildMonitor/RunSync.cs +++ b/BuildMonitor/RunSync.cs @@ -29,6 +29,21 @@ internal sealed class RunSync internal RunSync() => UpdateTimer.Start(); internal async Task UpdateAsync() + { + // Restart the timer whether or not the update succeeds, so a run whose update keeps failing + // is retried on its interval rather than back to back (ktsu-dev/BuildMonitor#299). A run that + // is no longer ongoing never updates again, so restarting its timer changes nothing. + try + { + _ = await SyncGuard.RunAsync(UpdateRunAsync, $"RunSync: {Run.Owner.Name}/{Run.Repository.Name}/{Run.Build.Name}/{Run.Name}").ConfigureAwait(false); + } + finally + { + UpdateTimer.Restart(); + } + } + + private async Task UpdateRunAsync() { await Run.Owner.BuildProvider.UpdateRunAsync(Run).ConfigureAwait(false); @@ -36,7 +51,6 @@ internal async Task UpdateAsync() { UpdateIntervalCurrent = (int)Run.CalculateETA().TotalSeconds; UpdateIntervalCurrent = Math.Clamp(UpdateIntervalCurrent, UpdateIntervalMin, UpdateIntervalMax); - UpdateTimer.Restart(); } } } diff --git a/BuildMonitor/SyncGuard.cs b/BuildMonitor/SyncGuard.cs new file mode 100644 index 0000000..1a6d697 --- /dev/null +++ b/BuildMonitor/SyncGuard.cs @@ -0,0 +1,58 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.BuildMonitor; + +/// +/// Runs one item of a batched provider update so that its failure is logged instead of faulting +/// the whole batch. +/// +/// +/// The update loop awaits each batch with . A single +/// exception — a persistent 5xx from one repository, say — used to fault that batch, which skipped +/// the timer restarts and pruning after it and made the loop start over straight away instead of on +/// its interval (ktsu-dev/BuildMonitor#299). +/// +internal static class SyncGuard +{ + /// + /// Runs , logging any exception it throws rather than propagating it. + /// + /// The update to run. + /// What is being updated, for the log line. + /// True when the update completed, false when it threw. + internal static async Task RunAsync(Func update, string description) + { + Ensure.NotNull(update); + + try + { + await update().ConfigureAwait(false); + return true; + } +#pragma warning disable CA1031 // Do not catch general exception types - one failing item must not stop the rest of the batch + catch (Exception ex) +#pragma warning restore CA1031 // Do not catch general exception types + { + Log.Error($"{description} failed: {ex.Message}"); + return false; + } + } + + /// + /// Runs for every item concurrently, each guarded by + /// , so one item that throws is logged and the rest still run. + /// + /// The type of item being updated. + /// The items to update. + /// The update to run for each item. + /// Describes an item for the log line when its update fails. + /// A task that completes, without faulting, once every update has finished. + internal static Task RunAllAsync(IEnumerable items, Func update, Func describe) + { + Ensure.NotNull(items); + Ensure.NotNull(update); + Ensure.NotNull(describe); + + return Task.WhenAll(items.Select(item => RunAsync(() => update(item), describe(item)))); + } +}