Bound background fetches and run at most one per repository [patch]

CLAUDE.md

@@ -26,7 +26,7 @@ Tests live in `ProjectDirector.Test` (MSTest, via `MSTest.Sdk` + `ktsu.Sdk`). Th
 
 `tests/ProjectDirector.UITests` drives the real application headlessly through `ktsu.ImGui.App.Testing` to render the app gallery (`Gallery/`), and exists for that purpose. It reaches the app through `ProjectDirector.BuildConfig()` and the internal options constructor. It replaces the settings file and the secret store with in-memory ones, and points the app at three repositories it seeds offline under `/tmp/projectdirector-gallery`. That path is fixed because it appears in the pictures. Every picture is cropped above the log panel, because the log prints wall-clock timestamps. `PROJECTDIRECTOR_GALLERY_OUT` names the output directory; `.github/workflows/app-gallery.yml` regenerates `docs/gallery/` after CI passes on `main`. A new view or dialog worth showing gets an entry in `Gallery/GalleryCatalog.cs`.
 
-That last point is why the repository actions are shaped the way they are: the part of each with a rule in it is pulled out into a plain method so it can be driven without a live ImGui context or a display. `ProjectDirector.DecidePull` decides whether pulling interrupts the user first (`PullDecisionTests`), and `GitCli.ListPendingChanges` plus `ProjectDirector.DescribePendingChanges` decide what a commit will sweep up and how that is shown (`CommitTests`). `RepoBrowsing` holds the repo and compare browsers' path handling: every entry is relative to the repository root and already includes the browse path, and folders are recognised on disk rather than by a trailing separator (`RepoBrowsingTests`). `DiffTake` builds the file a take arrow in the diff view produces and writes it back with the destination's own line ending and BOM (`DiffTakeTests`). Anything genuinely worth testing that is still tangled up with drawing is usually worth extracting the same way.
+That last point is why the repository actions are shaped the way they are: the part of each with a rule in it is pulled out into a plain method so it can be driven without a live ImGui context or a display. `ProjectDirector.DecidePull` decides whether pulling interrupts the user first (`PullDecisionTests`), and `GitCli.ListPendingChanges` plus `ProjectDirector.DescribePendingChanges` decide what a commit will sweep up and how that is shown (`CommitTests`). `RepoBrowsing` holds the repo and compare browsers' path handling: every entry is relative to the repository root and already includes the browse path, and folders are recognised on disk rather than by a trailing separator (`RepoBrowsingTests`). `DiffTake` builds the file a take arrow in the diff view produces and writes it back with the destination's own line ending and BOM (`DiffTakeTests`). `FetchScheduler` runs the background fetches at most four at a time and never two in one repository, and backdates each fetch time by a random jitter so repositories drift out of lockstep (`FetchSchedulerTests`). Anything genuinely worth testing that is still tangled up with drawing is usually worth extracting the same way.
 
 ```powershell
 dotnet test --configuration Release

ProjectDirector.Test/FetchSchedulerTests.cs

@@ -0,0 +1,119 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.ProjectDirector.Test;
+
+using System;
+using System.IO;
+using System.Linq;
+using System.Threading;
+using System.Threading.Tasks;
+
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+/// <summary>
+/// Covers how background fetches are scheduled.
+/// </summary>
+/// <remarks>
+/// Every stale repository used to start its own <c>git fetch</c> on the same frame, with no bound and
+/// no check for a fetch already running in that repository.
+/// </remarks>
+[TestClass]
+public sealed class FetchSchedulerTests
+{
+	private static FullyQualifiedLocalRepoPath LocalPath(string name) =>
+		FullyQualifiedLocalRepoPath.Create<FullyQualifiedLocalRepoPath>(Path.Join(Path.GetTempPath(), "dev", name));
+
+	[TestMethod]
+	public async Task FortyRepositoriesNeverFetchMoreThanTheBoundAtOnce()
+	{
+		FetchScheduler fetches = new(FetchScheduler.DefaultMaxConcurrent);
+		int running = 0;
+		int mostRunning = 0;
+		int finished = 0;
+
+		Task[] runs = [.. Enumerable.Range(0, 40).Select(i =>
+		{
+			Assert.IsTrue(fetches.TryRun(LocalPath($"repo{i}"), () =>
+			{
+				int now = Interlocked.Increment(ref running);
+				InterlockedMax(ref mostRunning, now);
+				Thread.Sleep(20);
+				_ = Interlocked.Decrement(ref running);
+				_ = Interlocked.Increment(ref finished);
+			}, out Task run));
+			return run;
+		})];
+
+		await Task.WhenAll(runs).ConfigureAwait(false);
+
+		Assert.AreEqual(40, finished);
+		Assert.IsLessThanOrEqualTo(FetchScheduler.DefaultMaxConcurrent, mostRunning);
+		Assert.IsGreaterThan(1, mostRunning, "the bound should still allow fetches to run side by side");
+	}
+
+	[TestMethod]
+	public async Task ASecondFetchInTheSameRepositoryIsRefusedWhileTheFirstRuns()
+	{
+		FetchScheduler fetches = new(FetchScheduler.DefaultMaxConcurrent);
+		using ManualResetEventSlim release = new();
+		int started = 0;
+
+		Assert.IsTrue(fetches.TryRun(LocalPath("A"), () =>
+		{
+			_ = Interlocked.Increment(ref started);
+			release.Wait();
+		}, out Task first));
+
+		Assert.IsTrue(fetches.IsInFlight(LocalPath("A")));
+		Assert.IsFalse(fetches.TryRun(LocalPath("A"), () => Interlocked.Increment(ref started), out Task second));
+		Assert.IsTrue(second.IsCompleted);
+		Assert.IsTrue(fetches.TryRun(LocalPath("B"), () => { }, out Task other));
+
+		release.Set();
+		await Task.WhenAll(first, other).ConfigureAwait(false);
+
+		Assert.AreEqual(1, started);
+		Assert.IsFalse(fetches.IsInFlight(LocalPath("A")));
+	}
+
+	[TestMethod]
+	public async Task ARepositoryCanBeFetchedAgainAfterItsFetchThrows()
+	{
+		FetchScheduler fetches = new(1);
+
+		Assert.IsTrue(fetches.TryRun(LocalPath("A"), () => throw new InvalidOperationException("fetch failed"), out Task failed));
+		_ = await Assert.ThrowsExactlyAsync<InvalidOperationException>(() => failed).ConfigureAwait(false);
+
+		bool ranAgain = false;
+		Assert.IsTrue(fetches.TryRun(LocalPath("A"), () => ranAgain = true, out Task retry));
+		await retry.ConfigureAwait(false);
+
+		Assert.IsTrue(ranAgain, "the failed fetch should have released both its slot and its repository");
+	}
+
+	[TestMethod]
+	public void JitterStaysWithinAQuarterOfTheIntervalAndSpreadsRepositoriesOut()
+	{
+		Random random = new(448);
+		TimeSpan[] jitters = [.. Enumerable.Range(0, 200).Select(_ => FetchScheduler.Jitter(60, random))];
+
+		Assert.IsTrue(jitters.All(j => j >= TimeSpan.Zero && j < TimeSpan.FromSeconds(15)));
+		Assert.IsGreaterThan(100, jitters.Distinct().Count());
+		Assert.AreEqual(TimeSpan.Zero, FetchScheduler.Jitter(0, random));
+	}
+
+	private static void InterlockedMax(ref int target, int value)
+	{
+		int current = Volatile.Read(ref target);
+		while (value > current)
+		{
+			int seen = Interlocked.CompareExchange(ref target, value, current);
+			if (seen == current)
+			{
+				return;
+			}
+
+			current = seen;
+		}
+	}
+}

ProjectDirector/FetchScheduler.cs

@@ -0,0 +1,135 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.ProjectDirector;
+
+using System;
+using System.Collections.Generic;
+using System.Threading;
+using System.Threading.Tasks;
+
+/// <summary>
+/// Runs background fetches a few at a time, and never two in the same repository at once.
+/// </summary>
+/// <remarks>
+/// Every cloned repository used to start its own <c>git fetch</c> the frame it went stale. They all
+/// went stale together, so a user with forty clones got forty concurrent fetches every minute, enough
+/// for GitHub to drop SSH handshakes. A fetch slower than the interval was also started again in the
+/// same repository while the first still ran, and the two fought over ref locks. A fetch waiting for a
+/// slot counts as in flight, so a stale repository is queued at most once.
+/// </remarks>
+/// <param name="maxConcurrent">The most fetches allowed to run at once.</param>
+internal sealed class FetchScheduler(int maxConcurrent)
+{
+	/// <summary>
+	/// The bound used by the application.
+	/// </summary>
+	internal const int DefaultMaxConcurrent = 4;
+
+	private readonly Queue<(FullyQualifiedLocalRepoPath LocalPath, Action Fetch, TaskCompletionSource Done)> waiting = new();
+	private readonly HashSet<FullyQualifiedLocalRepoPath> inFlight = [];
+	private readonly Lock gate = new();
+	private int running;
+
+	/// <summary>
+	/// Gets a value indicating whether a fetch in <paramref name="localPath"/> is running or waiting to run.
+	/// </summary>
+	/// <param name="localPath">The repository to ask about.</param>
+	/// <returns><see langword="true"/> until the fetch ends.</returns>
+	internal bool IsInFlight(FullyQualifiedLocalRepoPath localPath)
+	{
+		lock (gate)
+		{
+			return inFlight.Contains(localPath);
+		}
+	}
+
+	/// <summary>
+	/// Queues <paramref name="fetch"/> to run once a slot is free, unless a fetch in
+	/// <paramref name="localPath"/> is already running or queued.
+	/// </summary>
+	/// <param name="localPath">The repository being fetched.</param>
+	/// <param name="fetch">The fetch itself. It must not touch UI state.</param>
+	/// <param name="run">The queued fetch, or a completed task if the fetch was refused.</param>
+	/// <returns><see langword="true"/> if the fetch was queued.</returns>
+	/// <remarks>
+	/// The fetch releases its slot and its repository however it ends, so a fetch that throws can be retried.
+	/// </remarks>
+	internal bool TryRun(FullyQualifiedLocalRepoPath localPath, Action fetch, out Task run)
+	{
+		TaskCompletionSource done = new(TaskCreationOptions.RunContinuationsAsynchronously);
+		lock (gate)
+		{
+			if (!inFlight.Add(localPath))
+			{
+				run = Task.CompletedTask;
+				return false;
+			}
+
+			waiting.Enqueue((localPath, fetch, done));
+		}
+
+		run = done.Task;
+		StartWaitingFetches();
+		return true;
+	}
+
+	private void StartWaitingFetches()
+	{
+		lock (gate)
+		{
+			while (running < maxConcurrent && waiting.TryDequeue(out (FullyQualifiedLocalRepoPath LocalPath, Action Fetch, TaskCompletionSource Done) next))
+			{
+				running++;
+				_ = Task.Run(() => Execute(next.LocalPath, next.Fetch, next.Done));
+			}
+		}
+	}
+
+	private void Execute(FullyQualifiedLocalRepoPath localPath, Action fetch, TaskCompletionSource done)
+	{
+		Exception? failure = null;
+		try
+		{
+			fetch();
+		}
+		catch (Exception ex) when (ex is not OutOfMemoryException)
+		{
+			failure = ex;
+		}
+		finally
+		{
+			lock (gate)
+			{
+				running--;
+				_ = inFlight.Remove(localPath);
+			}
+		}
+
+		if (failure is null)
+		{
+			done.SetResult();
+		}
+		else
+		{
+			done.SetException(failure);
+		}
+
+		StartWaitingFetches();
+	}
+
+	/// <summary>
+	/// Picks how far to backdate a repository's fetch time, so repositories fetched together go stale
+	/// at different moments rather than all refiring on the same frame.
+	/// </summary>
+	/// <param name="intervalSeconds">The repository's fetch interval.</param>
+	/// <param name="random">The source of randomness.</param>
+	/// <returns>A duration from zero up to, but not including, a quarter of the interval.</returns>
+	[System.Diagnostics.CodeAnalysis.SuppressMessage("Security", "CA5394:Do not use insecure randomness", Justification = "Spreads fetch times; nothing depends on it being unpredictable.")]
+	internal static TimeSpan Jitter(int intervalSeconds, Random random)
+	{
+		Ensure.NotNull(random);
+		return intervalSeconds <= 0
+			? TimeSpan.Zero
+			: TimeSpan.FromSeconds(random.NextDouble() * intervalSeconds / 4);
+	}
+}

ProjectDirector/ProjectDirector.cs

@@ -61,6 +61,11 @@
 	/// </summary>
 	private CloneTracker Clones { get; } = new();
 
+	/// <summary>
+	/// The background fetches, bounded and one per repository, so stale repositories cannot all fetch at once.
+	/// </summary>
+	private FetchScheduler Fetches { get; } = new(FetchScheduler.DefaultMaxConcurrent);
+
 	private static void Main(string[] _) => ImGuiApp.Start(new ProjectDirector().BuildConfig());
 
 	/// <summary>
@@ -422,7 +427,7 @@
 	private void FetchAllRepos() => Options.ClonedRepos.Values.ForEach(FetchRepo);
 	private void PullAllRepos() => Options.ClonedRepos.Values.ForEach(PullRepo);
 
 	private void FetchRepo(FullyQualifiedGitHubRepoName repoName)
 	{
 		if (Options.Repos.TryGetValue(repoName, out GitRepository? repo))
 		{
@@ -430,7 +435,7 @@
 		}
 	}
 
 	private void PullRepo(FullyQualifiedGitHubRepoName repoName)
 	{
 		if (Options.Repos.TryGetValue(repoName, out GitRepository? repo))
 		{
@@ -457,14 +462,16 @@
 	private void FetchRepo(GitRepository repo)
 	{
 		FullyQualifiedLocalRepoPath repoPath = repo.LocalPath;
-		repo.LastFetchTime = DateTime.UtcNow;
-		QueueSaveOptions();
 
 		// Authentication is the platform credential helper's job now, which is also what makes SSH
 		// remotes work without any configuration here.
-		Task task = new(() => QueueGitLog($"Fetching {repo.RemotePath}", GitCli.RunIn(repoPath, "fetch", "origin")));
-
-		task.Start();
+		if (Fetches.TryRun(repoPath, () => QueueGitLog($"Fetching {repo.RemotePath}", GitCli.RunIn(repoPath, "fetch", "origin")), out _))
+		{
+			// Backdated by a random amount so repositories fetched together drift apart instead of
+			// going stale on the same frame again.
+			repo.LastFetchTime = DateTime.UtcNow - FetchScheduler.Jitter(repo.MinFetchIntervalSeconds, Random.Shared);
+			QueueSaveOptions();
+		}
 	}
 
 	/// <summary>
@@ -764,7 +771,7 @@
 						});
 					}
 
 					//int fetchInterval = repo.MinFetchIntervalSeconds;
 					//if (ImGuiWidgets.Knob("Min Fetch Interval", ref fetchInterval, 0, 300, 150))
 					//{
 					//	repo.MinFetchIntervalSeconds = fetchInterval;
@@ -1155,13 +1162,13 @@
 				}
 				else
 				{
 					throw new InvalidOperationException("Only GitHub Repos are supported at this time");
 				}
 			}
 		}
 	}
 
 	private void UpdateClonedStatus()
 	{
 		bool changed = false;
 		foreach ((FullyQualifiedGitHubRepoName _, GitRepository repo) in Options.Repos)
@@ -2138,7 +2145,7 @@
 
 				if (ImGui.TableNextColumn() && existsInA != existsInB)
 				{
 					if (existsInA && ImGui.ArrowButton("##Copy", ImGuiDir.Right))
 					{
 						LogBrowserFailure("Copying", path, RepoBrowsing.Copy(path, baseRoot, compareRoot));
 					}
@@ -2302,7 +2309,7 @@
 
 				if (ImGui.TableNextColumn())
 				{
 					//if (ImGui.Button($"Propagate Directory###Propagate{path.Replace(Path.DirectorySeparatorChar, '.').Replace(Path.AltDirectorySeparatorChar, '.')}"))
 					//{
 					//	shouldOpenPopup |= true;
 					//	Options.PropagatePath = path;
@@ -2311,7 +2318,7 @@
 
 				if (ImGui.TableNextColumn())
 				{
 					//if (ImGui.Button($"X"))
 					//{
 					//	//Directory.Delete(Path.Combine(Options.Repos[Options.BaseRepo].LocalPath, Options.BrowsePath, path));
 					//}
@@ -2350,13 +2357,13 @@
 
 				if (ImGui.TableNextColumn())//&& ImGui.Button($"Propagate###Propagate{path.Replace(Path.DirectorySeparatorChar, '.').Replace(Path.AltDirectorySeparatorChar, '.')}"))
 				{
 					//shouldOpenPopup |= true;
 					//Options.PropagatePath = path;
 				}
 
 				if (ImGui.TableNextColumn())
 				{
 					//if (ImGui.Button($"X"))
 					//{
 					//	//File.Delete(Path.Combine(Options.Repos[Options.BaseRepo].LocalPath, Options.BrowsePath, path));
 					//}