diff --git a/CLAUDE.md b/CLAUDE.md index abc29b8..561449a 100644 --- a/CLAUDE.md +++ b/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 diff --git a/ProjectDirector.Test/FetchSchedulerTests.cs b/ProjectDirector.Test/FetchSchedulerTests.cs new file mode 100644 index 0000000..cc84b8e --- /dev/null +++ b/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; + +/// +/// Covers how background fetches are scheduled. +/// +/// +/// Every stale repository used to start its own git fetch on the same frame, with no bound and +/// no check for a fetch already running in that repository. +/// +[TestClass] +public sealed class FetchSchedulerTests +{ + private static FullyQualifiedLocalRepoPath LocalPath(string name) => + FullyQualifiedLocalRepoPath.Create(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(() => 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; + } + } +} diff --git a/ProjectDirector/FetchScheduler.cs b/ProjectDirector/FetchScheduler.cs new file mode 100644 index 0000000..9eaf9dd --- /dev/null +++ b/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; + +/// +/// Runs background fetches a few at a time, and never two in the same repository at once. +/// +/// +/// Every cloned repository used to start its own git fetch 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. +/// +/// The most fetches allowed to run at once. +internal sealed class FetchScheduler(int maxConcurrent) +{ + /// + /// The bound used by the application. + /// + internal const int DefaultMaxConcurrent = 4; + + private readonly Queue<(FullyQualifiedLocalRepoPath LocalPath, Action Fetch, TaskCompletionSource Done)> waiting = new(); + private readonly HashSet inFlight = []; + private readonly Lock gate = new(); + private int running; + + /// + /// Gets a value indicating whether a fetch in is running or waiting to run. + /// + /// The repository to ask about. + /// until the fetch ends. + internal bool IsInFlight(FullyQualifiedLocalRepoPath localPath) + { + lock (gate) + { + return inFlight.Contains(localPath); + } + } + + /// + /// Queues to run once a slot is free, unless a fetch in + /// is already running or queued. + /// + /// The repository being fetched. + /// The fetch itself. It must not touch UI state. + /// The queued fetch, or a completed task if the fetch was refused. + /// if the fetch was queued. + /// + /// The fetch releases its slot and its repository however it ends, so a fetch that throws can be retried. + /// + 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(); + } + + /// + /// 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. + /// + /// The repository's fetch interval. + /// The source of randomness. + /// A duration from zero up to, but not including, a quarter of the interval. + [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); + } +} diff --git a/ProjectDirector/ProjectDirector.cs b/ProjectDirector/ProjectDirector.cs index 82b3ad9..6523620 100644 --- a/ProjectDirector/ProjectDirector.cs +++ b/ProjectDirector/ProjectDirector.cs @@ -61,6 +61,11 @@ internal sealed class ProjectDirector /// private CloneTracker Clones { get; } = new(); + /// + /// The background fetches, bounded and one per repository, so stale repositories cannot all fetch at once. + /// + private FetchScheduler Fetches { get; } = new(FetchScheduler.DefaultMaxConcurrent); + private static void Main(string[] _) => ImGuiApp.Start(new ProjectDirector().BuildConfig()); /// @@ -457,14 +462,16 @@ private void FetchRepoIfStale(GitRepository repo) 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(); + } } ///