From 644e76fcf105cfc0c675fbf86081284745386a94 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 17:32:25 +0000 Subject: [PATCH 1/4] Refresh after a clone on the render thread, and allow one clone per folder [patch] The Clone button's continuation ran RefreshPage on the worker thread that did the clone, since the app has no SynchronizationContext and the continuation asked to run synchronously. From there it mutated ClonedRepos, enumerated Repos and replaced the repo browser while the render thread was reading them, and a second click started another clone into the same folder. A new CloneTracker records the clones in flight and a refresh request. The worker only marks its clone complete; Tick takes the request and refreshes on the render thread, and the button is disabled while its clone runs. Fixes #439 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KLpJqJzDAe9WoXTVLt65CJ --- ProjectDirector.Test/CloneTrackerTests.cs | 75 +++++++++++++++++++++++ ProjectDirector/CloneTracker.cs | 71 +++++++++++++++++++++ ProjectDirector/ProjectDirector.cs | 35 +++++++++-- 3 files changed, 175 insertions(+), 6 deletions(-) create mode 100644 ProjectDirector.Test/CloneTrackerTests.cs create mode 100644 ProjectDirector/CloneTracker.cs diff --git a/ProjectDirector.Test/CloneTrackerTests.cs b/ProjectDirector.Test/CloneTrackerTests.cs new file mode 100644 index 0000000..4074f32 --- /dev/null +++ b/ProjectDirector.Test/CloneTrackerTests.cs @@ -0,0 +1,75 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.ProjectDirector.Test; + +using System.IO; +using System.Threading.Tasks; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +/// +/// Covers handing a background clone's completion back to the render thread. +/// +/// +/// The Clone button used to refresh the page from the worker thread that ran the clone, racing the +/// render thread over the repository dictionaries and the repo browser, and nothing stopped a second +/// click from starting another clone into the same folder. +/// +[TestClass] +public sealed class CloneTrackerTests +{ + private static FullyQualifiedLocalRepoPath LocalPath(string name) => + FullyQualifiedLocalRepoPath.Create(Path.Combine(Path.GetTempPath(), "dev", name)); + + [TestMethod] + public void ASecondCloneIntoTheSameFolderIsRefusedWhileTheFirstRuns() + { + CloneTracker clones = new(); + + Assert.IsTrue(clones.TryStart(LocalPath("A"))); + Assert.IsTrue(clones.IsInFlight(LocalPath("A"))); + Assert.IsFalse(clones.TryStart(LocalPath("A"))); + } + + [TestMethod] + public void ACloneIntoAnotherFolderIsNotBlocked() + { + CloneTracker clones = new(); + + Assert.IsTrue(clones.TryStart(LocalPath("A"))); + Assert.IsTrue(clones.TryStart(LocalPath("B"))); + } + + [TestMethod] + public void AFolderCanBeClonedAgainOnceItsCloneCompletes() + { + CloneTracker clones = new(); + Assert.IsTrue(clones.TryStart(LocalPath("A"))); + + clones.Complete(LocalPath("A")); + + Assert.IsFalse(clones.IsInFlight(LocalPath("A"))); + Assert.IsTrue(clones.TryStart(LocalPath("A"))); + } + + [TestMethod] + public void NoRefreshIsRequestedUntilACloneCompletes() + { + CloneTracker clones = new(); + Assert.IsTrue(clones.TryStart(LocalPath("A"))); + + Assert.IsFalse(clones.TakeRefreshRequest()); + } + + [TestMethod] + public async Task ACloneCompletedOnAWorkerThreadIsRefreshedByTheCallerThatTakesTheRequestOnce() + { + CloneTracker clones = new(); + Assert.IsTrue(clones.TryStart(LocalPath("A"))); + + await Task.Run(() => clones.Complete(LocalPath("A"))).ConfigureAwait(false); + + Assert.IsTrue(clones.TakeRefreshRequest()); + Assert.IsFalse(clones.TakeRefreshRequest()); + } +} diff --git a/ProjectDirector/CloneTracker.cs b/ProjectDirector/CloneTracker.cs new file mode 100644 index 0000000..a8cc0e5 --- /dev/null +++ b/ProjectDirector/CloneTracker.cs @@ -0,0 +1,71 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.ProjectDirector; + +using System.Collections.Generic; +using System.Threading; + +/// +/// Tracks the clones running in the background and hands their completion back to the render thread. +/// +/// +/// A clone finishes on a worker thread, and refreshing the page from there mutated +/// , and the +/// repo browser while the render thread was enumerating them. So a finished clone only records that a +/// refresh is wanted, and the tick that runs on the render thread takes the request and refreshes. +/// The same record of what is in flight keeps a second click from starting another clone into the +/// folder the first one is still writing. +/// +internal sealed class CloneTracker +{ + private readonly HashSet inFlight = []; + private readonly Lock gate = new(); + private int refreshRequested; + + /// + /// Records a clone into as started, unless one already is. + /// + /// The folder being cloned into. + /// if the caller should start the clone. + internal bool TryStart(FullyQualifiedLocalRepoPath localPath) + { + lock (gate) + { + return inFlight.Add(localPath); + } + } + + /// + /// Gets a value indicating whether a clone into is running. + /// + /// The folder to ask about. + /// while the clone runs. + internal bool IsInFlight(FullyQualifiedLocalRepoPath localPath) + { + lock (gate) + { + return inFlight.Contains(localPath); + } + } + + /// + /// Records a clone as finished, whether or not it succeeded, and asks for a refresh. Safe to call + /// from any thread. + /// + /// The folder that was cloned into. + internal void Complete(FullyQualifiedLocalRepoPath localPath) + { + lock (gate) + { + _ = inFlight.Remove(localPath); + } + + _ = Interlocked.Exchange(ref refreshRequested, 1); + } + + /// + /// Takes the pending refresh request, if any. Call from the render thread. + /// + /// once for each run of completions since the last call. + internal bool TakeRefreshRequest() => Interlocked.Exchange(ref refreshRequested, 0) != 0; +} diff --git a/ProjectDirector/ProjectDirector.cs b/ProjectDirector/ProjectDirector.cs index 6a244f0..7148052 100644 --- a/ProjectDirector/ProjectDirector.cs +++ b/ProjectDirector/ProjectDirector.cs @@ -56,6 +56,11 @@ internal sealed class ProjectDirector /// private GitHubOwnerName? OwnerPendingTokenPopup { get; set; } + /// + /// The clones running in the background, whose completion the tick hands to . + /// + private CloneTracker Clones { get; } = new(); + // private ChatClient ChatClient { get; init; } private static void Main(string[] _) @@ -681,6 +686,11 @@ private void Tick(float dt) result => QueueLogIfAny(ApplyOwnerToken(owner, result))); } + if (Clones.TakeRefreshRequest()) + { + RefreshPage(); + } + _ = PopupSetDevDirectory.ShowIfOpen(); _ = PopupAddNewGitHubOwner.ShowIfOpen(); _ = PopupSetGitHubOwnerToken.ShowIfOpen(); @@ -703,14 +713,27 @@ private void ShowTopPanel(float dt) { if (!Options.ClonedRepos.ContainsValue(Options.BaseRepo)) { - if (ImGui.Button("Clone", new Vector2(FieldWidth, 0))) + bool cloning = Clones.IsInFlight(repo.LocalPath); + ImGui.BeginDisabled(cloning); + if (ImGui.Button(cloning ? "Cloning..." : "Clone", new Vector2(FieldWidth, 0)) && Clones.TryStart(repo.LocalPath)) { - Task.Run(() => QueueGitLog($"Cloning {repo.RemotePath}", GitCli.Run("clone", repo.RemotePath.ToString(), repo.LocalPath.ToString()))) - .ContinueWith((t) => RefreshPage(), - new CancellationToken(), - TaskContinuationOptions.OnlyOnRanToCompletion | TaskContinuationOptions.ExecuteSynchronously, - TaskScheduler.Current); + GitRemotePath remotePath = repo.RemotePath; + FullyQualifiedLocalRepoPath localPath = repo.LocalPath; + _ = Task.Run(() => + { + try + { + QueueGitLog($"Cloning {remotePath}", GitCli.Run("clone", remotePath.ToString(), localPath.ToString())); + } + finally + { + // The page is refreshed by the next tick, on the render thread, not here. + Clones.Complete(localPath); + } + }); } + + ImGui.EndDisabled(); } else { From b3e51088d5cd01a37e4e870ba4cd61cf406ee1d8 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 17:35:57 +0000 Subject: [PATCH 2/4] Build the test paths with Path.Join Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KLpJqJzDAe9WoXTVLt65CJ --- ProjectDirector.Test/CloneTrackerTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ProjectDirector.Test/CloneTrackerTests.cs b/ProjectDirector.Test/CloneTrackerTests.cs index 4074f32..33831f3 100644 --- a/ProjectDirector.Test/CloneTrackerTests.cs +++ b/ProjectDirector.Test/CloneTrackerTests.cs @@ -19,7 +19,7 @@ namespace ktsu.ProjectDirector.Test; public sealed class CloneTrackerTests { private static FullyQualifiedLocalRepoPath LocalPath(string name) => - FullyQualifiedLocalRepoPath.Create(Path.Combine(Path.GetTempPath(), "dev", name)); + FullyQualifiedLocalRepoPath.Create(Path.Join(Path.GetTempPath(), "dev", name)); [TestMethod] public void ASecondCloneIntoTheSameFolderIsRefusedWhileTheFirstRuns() From b1172b58ac7fb8d605e61657830f2757da973873 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 17:48:23 +0000 Subject: [PATCH 3/4] Run the clone and the refresh handoff through CloneTracker Moves the background clone, its completion and the tick's refresh into CloneTracker.TryRun, CloneTracker.RefreshIfRequested and ProjectDirector.MakeClone, so the parts with a rule in them are tested rather than sitting in the ImGui layer. The Clone button is now hidden while its clone runs rather than disabled. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KLpJqJzDAe9WoXTVLt65CJ --- ProjectDirector.Test/CloneTrackerTests.cs | 99 +++++++++++++++++++++++ ProjectDirector/CloneTracker.cs | 45 +++++++++++ ProjectDirector/ProjectDirector.cs | 38 ++++----- 3 files changed, 159 insertions(+), 23 deletions(-) diff --git a/ProjectDirector.Test/CloneTrackerTests.cs b/ProjectDirector.Test/CloneTrackerTests.cs index 33831f3..5a54e01 100644 --- a/ProjectDirector.Test/CloneTrackerTests.cs +++ b/ProjectDirector.Test/CloneTrackerTests.cs @@ -2,7 +2,9 @@ namespace ktsu.ProjectDirector.Test; +using System; using System.IO; +using System.Threading; using System.Threading.Tasks; using Microsoft.VisualStudio.TestTools.UnitTesting; @@ -72,4 +74,101 @@ public async Task ACloneCompletedOnAWorkerThreadIsRefreshedByTheCallerThatTakesT Assert.IsTrue(clones.TakeRefreshRequest()); Assert.IsFalse(clones.TakeRefreshRequest()); } + + [TestMethod] + public async Task TryRunRefusesASecondCloneWhileTheFirstRunsAndRequestsARefreshWhenItEnds() + { + CloneTracker clones = new(); + using ManualResetEventSlim release = new(); + + Task? first = clones.TryRun(LocalPath("A"), release.Wait); + Assert.IsNotNull(first); + Assert.IsTrue(clones.IsInFlight(LocalPath("A"))); + Assert.IsNull(clones.TryRun(LocalPath("A"), () => Assert.Fail("A duplicate clone ran."))); + + release.Set(); + await first.ConfigureAwait(false); + + Assert.IsFalse(clones.IsInFlight(LocalPath("A"))); + Assert.IsTrue(clones.TakeRefreshRequest()); + } + + [TestMethod] + public async Task ACloneThatThrowsIsStillRecordedAsComplete() + { + CloneTracker clones = new(); + + Task? run = clones.TryRun(LocalPath("A"), () => throw new InvalidOperationException("clone failed")); + Assert.IsNotNull(run); + _ = await Assert.ThrowsExactlyAsync(() => run).ConfigureAwait(false); + + Assert.IsFalse(clones.IsInFlight(LocalPath("A"))); + Assert.IsTrue(clones.TakeRefreshRequest()); + } + + [TestMethod] + public void RefreshIfRequestedRefreshesOnceForACompletedClone() + { + CloneTracker clones = new(); + int refreshes = 0; + + clones.RefreshIfRequested(() => refreshes++); + Assert.AreEqual(0, refreshes); + + clones.Complete(LocalPath("A")); + clones.RefreshIfRequested(() => refreshes++); + clones.RefreshIfRequested(() => refreshes++); + + Assert.AreEqual(1, refreshes); + } + + [TestMethod] + public void MakeCloneClonesTheRemoteIntoTheFolderAndLogsTheResult() + { + string root = Path.Join(Path.GetTempPath(), $"ktsu_pd_{Guid.NewGuid():N}"); + try + { + string origin = Path.Join(root, "origin"); + string clone = Path.Join(root, "clone"); + Assert.IsTrue(GitCli.Run("init", origin).Succeeded, "git init failed."); + string? description = null; + GitResult? result = null; + + ProjectDirector.MakeClone( + GitRemotePath.Create(origin), + FullyQualifiedLocalRepoPath.Create(clone), + (d, r) => (description, result) = (d, r))(); + + Assert.AreEqual($"Cloning {origin}", description); + Assert.IsNotNull(result); + Assert.IsTrue(result.Succeeded); + Assert.IsTrue(GitCli.IsRepository(clone)); + } + finally + { + TryDeleteDirectory(root); + } + } + + private static void TryDeleteDirectory(string path) + { + try + { + // Git marks objects read-only, which blocks a plain recursive delete on Windows. + foreach (string file in Directory.EnumerateFiles(path, "*", SearchOption.AllDirectories)) + { + File.SetAttributes(file, FileAttributes.Normal); + } + + Directory.Delete(path, recursive: true); + } + catch (IOException) + { + // A best-effort cleanup of a temp directory is not worth failing a test over. + } + catch (UnauthorizedAccessException) + { + // As above. + } + } } diff --git a/ProjectDirector/CloneTracker.cs b/ProjectDirector/CloneTracker.cs index a8cc0e5..3588e25 100644 --- a/ProjectDirector/CloneTracker.cs +++ b/ProjectDirector/CloneTracker.cs @@ -2,8 +2,10 @@ namespace ktsu.ProjectDirector; +using System; using System.Collections.Generic; using System.Threading; +using System.Threading.Tasks; /// /// Tracks the clones running in the background and hands their completion back to the render thread. @@ -63,6 +65,49 @@ internal void Complete(FullyQualifiedLocalRepoPath localPath) _ = Interlocked.Exchange(ref refreshRequested, 1); } + /// + /// Runs on the thread pool, unless a clone into + /// is already running. + /// + /// The folder being cloned into. + /// The clone itself. It must not touch UI state. + /// The running clone, or if one was already running. + /// + /// The clone is recorded as complete however it ends, so a clone that throws can be retried. + /// + internal Task? TryRun(FullyQualifiedLocalRepoPath localPath, Action clone) + { + if (!TryStart(localPath)) + { + return null; + } + + return Task.Run(() => + { + try + { + clone(); + } + finally + { + Complete(localPath); + } + }); + } + + /// + /// Calls if a clone has completed since the last call. Call from the + /// render thread. + /// + /// Refreshes the page. + internal void RefreshIfRequested(Action refresh) + { + if (TakeRefreshRequest()) + { + refresh(); + } + } + /// /// Takes the pending refresh request, if any. Call from the render thread. /// diff --git a/ProjectDirector/ProjectDirector.cs b/ProjectDirector/ProjectDirector.cs index 7148052..b32edfd 100644 --- a/ProjectDirector/ProjectDirector.cs +++ b/ProjectDirector/ProjectDirector.cs @@ -686,10 +686,7 @@ private void Tick(float dt) result => QueueLogIfAny(ApplyOwnerToken(owner, result))); } - if (Clones.TakeRefreshRequest()) - { - RefreshPage(); - } + Clones.RefreshIfRequested(RefreshPage); _ = PopupSetDevDirectory.ShowIfOpen(); _ = PopupAddNewGitHubOwner.ShowIfOpen(); @@ -713,27 +710,12 @@ private void ShowTopPanel(float dt) { if (!Options.ClonedRepos.ContainsValue(Options.BaseRepo)) { - bool cloning = Clones.IsInFlight(repo.LocalPath); - ImGui.BeginDisabled(cloning); - if (ImGui.Button(cloning ? "Cloning..." : "Clone", new Vector2(FieldWidth, 0)) && Clones.TryStart(repo.LocalPath)) + // The button is hidden while its clone runs, and the page is refreshed by the next + // tick, on the render thread, rather than by the clone. + if (!Clones.IsInFlight(repo.LocalPath) && ImGui.Button("Clone", new Vector2(FieldWidth, 0))) { - GitRemotePath remotePath = repo.RemotePath; - FullyQualifiedLocalRepoPath localPath = repo.LocalPath; - _ = Task.Run(() => - { - try - { - QueueGitLog($"Cloning {remotePath}", GitCli.Run("clone", remotePath.ToString(), localPath.ToString())); - } - finally - { - // The page is refreshed by the next tick, on the render thread, not here. - Clones.Complete(localPath); - } - }); + _ = Clones.TryRun(repo.LocalPath, MakeClone(repo.RemotePath, repo.LocalPath, QueueGitLog)); } - - ImGui.EndDisabled(); } else { @@ -817,6 +799,16 @@ private void ShowTopPanel(float dt) } } + /// + /// Makes the clone the Clone button runs in the background. + /// + /// The repository to clone. + /// The folder to clone it into. + /// Records the result. Called from the thread the clone runs on. + /// The clone, which touches no UI state. + internal static Action MakeClone(GitRemotePath remotePath, FullyQualifiedLocalRepoPath localPath, Action log) => + () => log($"Cloning {remotePath}", GitCli.Run("clone", remotePath.ToString(), localPath.ToString())); + private void RefreshPage() { if (Options.Repos.TryGetValue(Options.BaseRepo, out GitRepository? repo)) From 75a97dca35c1e1df09eab7ba484bc49c684d7cea Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 17:57:58 +0000 Subject: [PATCH 4/4] Return whether TryRun started the clone instead of a null task Sonar S4586: a method returning Task should never return null. The task is now an out parameter, completed when the clone is refused. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KLpJqJzDAe9WoXTVLt65CJ --- ProjectDirector.Test/CloneTrackerTests.cs | 9 ++++----- ProjectDirector/CloneTracker.cs | 12 ++++++++---- ProjectDirector/ProjectDirector.cs | 2 +- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/ProjectDirector.Test/CloneTrackerTests.cs b/ProjectDirector.Test/CloneTrackerTests.cs index 5a54e01..bf9422e 100644 --- a/ProjectDirector.Test/CloneTrackerTests.cs +++ b/ProjectDirector.Test/CloneTrackerTests.cs @@ -81,10 +81,10 @@ public async Task TryRunRefusesASecondCloneWhileTheFirstRunsAndRequestsARefreshW CloneTracker clones = new(); using ManualResetEventSlim release = new(); - Task? first = clones.TryRun(LocalPath("A"), release.Wait); - Assert.IsNotNull(first); + Assert.IsTrue(clones.TryRun(LocalPath("A"), release.Wait, out Task first)); Assert.IsTrue(clones.IsInFlight(LocalPath("A"))); - Assert.IsNull(clones.TryRun(LocalPath("A"), () => Assert.Fail("A duplicate clone ran."))); + Assert.IsFalse(clones.TryRun(LocalPath("A"), () => Assert.Fail("A duplicate clone ran."), out Task refused)); + Assert.IsTrue(refused.IsCompleted); release.Set(); await first.ConfigureAwait(false); @@ -98,8 +98,7 @@ public async Task ACloneThatThrowsIsStillRecordedAsComplete() { CloneTracker clones = new(); - Task? run = clones.TryRun(LocalPath("A"), () => throw new InvalidOperationException("clone failed")); - Assert.IsNotNull(run); + Assert.IsTrue(clones.TryRun(LocalPath("A"), () => throw new InvalidOperationException("clone failed"), out Task run)); _ = await Assert.ThrowsExactlyAsync(() => run).ConfigureAwait(false); Assert.IsFalse(clones.IsInFlight(LocalPath("A"))); diff --git a/ProjectDirector/CloneTracker.cs b/ProjectDirector/CloneTracker.cs index 3588e25..9cf2fce 100644 --- a/ProjectDirector/CloneTracker.cs +++ b/ProjectDirector/CloneTracker.cs @@ -71,18 +71,20 @@ internal void Complete(FullyQualifiedLocalRepoPath localPath) /// /// The folder being cloned into. /// The clone itself. It must not touch UI state. - /// The running clone, or if one was already running. + /// The running clone, or a completed task if the clone was refused. + /// if the clone was started. /// /// The clone is recorded as complete however it ends, so a clone that throws can be retried. /// - internal Task? TryRun(FullyQualifiedLocalRepoPath localPath, Action clone) + internal bool TryRun(FullyQualifiedLocalRepoPath localPath, Action clone, out Task run) { if (!TryStart(localPath)) { - return null; + run = Task.CompletedTask; + return false; } - return Task.Run(() => + run = Task.Run(() => { try { @@ -93,6 +95,8 @@ internal void Complete(FullyQualifiedLocalRepoPath localPath) Complete(localPath); } }); + + return true; } /// diff --git a/ProjectDirector/ProjectDirector.cs b/ProjectDirector/ProjectDirector.cs index b32edfd..4f3b332 100644 --- a/ProjectDirector/ProjectDirector.cs +++ b/ProjectDirector/ProjectDirector.cs @@ -714,7 +714,7 @@ private void ShowTopPanel(float dt) // tick, on the render thread, rather than by the clone. if (!Clones.IsInFlight(repo.LocalPath) && ImGui.Button("Clone", new Vector2(FieldWidth, 0))) { - _ = Clones.TryRun(repo.LocalPath, MakeClone(repo.RemotePath, repo.LocalPath, QueueGitLog)); + _ = Clones.TryRun(repo.LocalPath, MakeClone(repo.RemotePath, repo.LocalPath, QueueGitLog), out _); } } else