fix: keep polling intervals when a provider update throws

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;
+
+/// <summary>
+/// Covers what happens when a provider update throws — a persistent GitHub 5xx, for example. The
+/// sync's timer used to be left running, so <c>ShouldUpdate</c> 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).
+/// </summary>
+[TestClass]
+public sealed class SyncFailureTests
+{
+	/// <summary>
+	/// Long enough for a timer that was not restarted to show a visibly shorter time remaining.
+	/// </summary>
+	private static readonly TimeSpan Elapse = TimeSpan.FromMilliseconds(1100);
+
+	/// <summary>
+	/// A provider whose build and run updates throw for the items named in <see cref="Failing"/>
+	/// and record the rest.
+	/// </summary>
+	private sealed class FailingProvider : BuildProvider
+	{
+		internal override BuildProviderName Name { get; } = "Failing".As<BuildProviderName>();
+
+		internal HashSet<string> Failing { get; } = [];
+
+		internal List<string> 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<BuildName>());
+		Assert.IsTrue(repository.Builds.TryAdd(build.Id, build));
+		return build;
+	}
+
+	private static Repository CreateRepository(FailingProvider provider)
+	{
+		Owner owner = provider.CreateOwner("owner".As<OwnerName>());
+		return owner.CreateRepository("repo".As<RepositoryName>());
+	}
+
+	[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<string> 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<RunName>());
+		run.Status = RunStatus.Running;
+		Assert.IsTrue(build.Runs.TryAdd(run.Id, run));
+		return run;
+	}
+
+	/// <summary>
+	/// Gets or sets the test context MSTest injects, used for its cancellation token.
+	/// </summary>
+	public TestContext TestContext { get; set; } = null!;
+}

BuildMonitor/BuildMonitor.cs

@@ -186,7 +186,7 @@
 	/// fields actually occupy tests the bug itself, so a fixed binding is recognised as fixed and
 	/// anything else is refused rather than guessed at.
 	/// </remarks>
 	private static unsafe int? ProbeNativeImGuiTableColumnSize()
 	{
 		int csharpSize = sizeof(ImGuiTableColumn);
 		int widthGivenOffset;
@@ -285,7 +285,7 @@
 		return narrowFieldBytes == NarrowlyBoundColumnFields.Length * 2 ? csharpSize : null;
 	}
 
 	private static unsafe void SaveColumnWidth(string columnName, int columnIndex)
 	{
 		// Null means the layout probe rejected the binding, so there is no address we can trust.
 		if (NativeImGuiTableColumnSize is not int nativeStructSize)
@@ -412,7 +412,7 @@
 		SaveSettingsIfRequired();
 	}
 
 	private static void UpdateOwnerTabs()
 	{
 		// Build expected tabs based on provider type:
 		// - GitHub: one tab per owner (org/user)
@@ -421,7 +421,7 @@
 		HashSet<string> expectedTabIds = [AllOwnersTabId, LogsTabId];
 		Dictionary<string, (string Label, Action Content)> tabsToCreate = [];
 
 		foreach ((BuildProviderName providerName, BuildProvider provider) in AppData.BuildProviders)
 		{
 			if (provider is GitHub)
 			{
@@ -628,7 +628,7 @@
 		return displayName;
 	}
 
 	private static string MakeRepositoryDisplayName(Build build)
 	{
 		string repoName = build.Repository.Name;
 		string ownerName = build.Owner.Name;
@@ -1523,7 +1523,9 @@
 			}
 
 			// 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 @@
 			}
 
 			// 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 @@
 						Build = build,
 					});
 				}
-			})).ConfigureAwait(false);
+			}, x => $"{x.Provider.Name}: build discovery for {x.Repository.Owner.Name}/{x.Repository.Name}").ConfigureAwait(false);
 
 			ProviderRefreshTimer.Restart();
 		}

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

BuildMonitor/RunSync.cs

@@ -29,14 +29,28 @@ 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);
 
 		if (Run.IsOngoing)
 		{
 			UpdateIntervalCurrent = (int)Run.CalculateETA().TotalSeconds;
 			UpdateIntervalCurrent = Math.Clamp(UpdateIntervalCurrent, UpdateIntervalMin, UpdateIntervalMax);
-			UpdateTimer.Restart();
 		}
 	}
 }

BuildMonitor/SyncGuard.cs

@@ -0,0 +1,58 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.BuildMonitor;
+
+/// <summary>
+/// Runs one item of a batched provider update so that its failure is logged instead of faulting
+/// the whole batch.
+/// </summary>
+/// <remarks>
+/// The update loop awaits each batch with <see cref="Task.WhenAll(IEnumerable{Task})"/>. 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).
+/// </remarks>
+internal static class SyncGuard
+{
+	/// <summary>
+	/// Runs <paramref name="update"/>, logging any exception it throws rather than propagating it.
+	/// </summary>
+	/// <param name="update">The update to run.</param>
+	/// <param name="description">What is being updated, for the log line.</param>
+	/// <returns>True when the update completed, false when it threw.</returns>
+	internal static async Task<bool> RunAsync(Func<Task> 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;
+		}
+	}
+
+	/// <summary>
+	/// Runs <paramref name="update"/> for every item concurrently, each guarded by
+	/// <see cref="RunAsync"/>, so one item that throws is logged and the rest still run.
+	/// </summary>
+	/// <typeparam name="T">The type of item being updated.</typeparam>
+	/// <param name="items">The items to update.</param>
+	/// <param name="update">The update to run for each item.</param>
+	/// <param name="describe">Describes an item for the log line when its update fails.</param>
+	/// <returns>A task that completes, without faulting, once every update has finished.</returns>
+	internal static Task RunAllAsync<T>(IEnumerable<T> items, Func<T, Task> update, Func<T, string> describe)
+	{
+		Ensure.NotNull(items);
+		Ensure.NotNull(update);
+		Ensure.NotNull(describe);
+
+		return Task.WhenAll(items.Select(item => RunAsync(() => update(item), describe(item))));
+	}
+}