From b592898fcb6e928e9323e15d35009eece2146896 Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 10 Nov 2026 15:07:55 -0600 Subject: [PATCH 1/2] fix(engine): small correctness fixes found in the 1.1.1 review - A target folder that is a drive root ('C:\', '/') rejected every repository: the root keeps its separator when trimmed, and a second one was appended. The root now ends in exactly one. - The Windows-invalid character set was asked of the running platform, so the Windows rules answered differently elsewhere. It is written out. - SyncSummary.WasCanceled reported the token at the end of the run, so a cancellation that arrived after the last repository finished marked a complete run as canceled (and the CLI exited 1). It is now true exactly when a repository was canceled. - A concurrency read from accounts.json was used as stored. Account now keeps it between 1 and 64 however it is set. The engine's handling of a value below one has a test. - ProcessRunner wrote the child's input outside the block that ends the child, and a child that exited before reading its input failed the call with an I/O error. Every way out now ends a child that is still alive, and such a child answers with its exit code and error output. - The gateway cache kept up to eight clients, keyed by the token, and emptied itself when full. GatewayCache keeps four, keyed by a hash of the token, and drops only the one unused the longest. - The activity log: timestamps carry their offset from UTC; the clock is read once per entry, so the timestamp and the daily file agree at midnight; daily files older than 30 days are removed; a write that finds the file held by another process (the app and the CLI share it) waits briefly and tries again instead of dropping the entry. - The invalid-paths message ended with "Nothing was checked out.", which is wrong for a repository that already has a working tree. It now ends with "No files were written.", which holds for a clone and a pull. docs/CLI.md named the wrong log folder; corrected with the above. Closes #151 Co-Authored-By: Claude Fable 5.2 Signed-off-by: James Maes --- README.md | 2 +- docs/CLI.md | 11 +- gclo.Engine.Tests/AccountsStoreTests.cs | 33 +++ gclo.Engine.Tests/ActivityLogTests.cs | 276 +++++++++++++++++- gclo.Engine.Tests/GatewayCacheTests.cs | 152 ++++++++++ .../InvalidRepositoryPathsExceptionTests.cs | 4 +- gclo.Engine.Tests/OrgSyncEngineTests.cs | 61 ++++ gclo.Engine.Tests/ProcessRunnerTests.cs | 164 +++++++++++ .../RepositoryPathResolverTests.cs | 43 +++ .../WindowsPathValidationTests.cs | 20 ++ gclo.Engine/FileActivityLog.cs | 149 +++++++++- gclo.Engine/GatewayCache.cs | 95 ++++++ .../InvalidRepositoryPathsException.cs | 2 +- gclo.Engine/OctokitGatewayCache.cs | 25 +- gclo.Engine/OrgSyncEngine.cs | 6 +- gclo.Engine/ProcessRunner.cs | 61 +++- gclo.Engine/RepositoryPathResolver.cs | 16 +- gclo.Engine/SyncRequest.cs | 4 +- gclo.Engine/SyncSummary.cs | 6 +- gclo.Engine/WindowsPathValidator.cs | 13 +- gclo.ViewModels/Account.cs | 13 +- 21 files changed, 1091 insertions(+), 65 deletions(-) create mode 100644 gclo.Engine.Tests/GatewayCacheTests.cs create mode 100644 gclo.Engine.Tests/ProcessRunnerTests.cs create mode 100644 gclo.Engine/GatewayCache.cs diff --git a/README.md b/README.md index 5e5e848..214bb4b 100644 --- a/README.md +++ b/README.md @@ -116,7 +116,7 @@ On launch, gclo shows a brief branded splash, then the **Quick Sync** connect ca **Settings** (File > Settings…) let you set a default GitHub token (stored in the Windows Credential Manager), the default target folder (with a folder picker), the default parallelism, and the theme (System, Light, or Dark); an Advanced section toggles the startup splash and its duration. -**Activity log:** View > Activity log… shows the tail of today's log and links to the logs folder. Logs live under `%LOCALAPPDATA%\KofTwentyTwo\gclo\logs`, one file per day; they record run parameters and per-repo failures and never contain your token (see [SECURITY.md](SECURITY.md)). +**Activity log:** View > Activity log… shows the tail of today's log and links to the logs folder. Logs live under `%LOCALAPPDATA%\KofTwentyTwo\gclo\logs`, one file per day, kept for 30 days; they record run parameters and per-repo failures and never contain your token (see [SECURITY.md](SECURITY.md)). **Updates:** Help > Check for updates… downloads and applies the latest release for your channel. Self-update is only available in installed builds (not the portable zip or a local debug build). diff --git a/docs/CLI.md b/docs/CLI.md index 7c8d988..8639a1e 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -381,13 +381,18 @@ still wins for that run (the stored token is left untouched). ## Activity log Every invocation appends to a daily activity log file, `gclo-yyyy-MM-dd.log`, -under `%LOCALAPPDATA%\gclo\logs` (on Linux/macOS: the platform's local -application data folder). It records the run's parameters, per-repository +under `%LOCALAPPDATA%\KofTwentyTwo\gclo\logs` (on Linux/macOS: the platform's +local application data folder). It records the run's parameters, per-repository failures, and path-sanitization notes — the same log the desktop app shows under **View > Activity log**. Tokens are never written to the log, or anywhere else on disk; see [SECURITY.md](../SECURITY.md). -Each entry is one line that starts with its timestamp. Text that came from a repository, such as a file path, is written with control characters shown as `\uXXXX`, and the lines of an exception are indented under their entry, so nothing in a repository can add or imitate an entry. Everything the CLI prints as text shows such characters the same way, and on a console whose code page cannot represent a character (code page 437 or 850, for example) that character is shown in the same form too; the JSON modes use JSON's own escaping. +Daily files are kept for 30 days, today included; older ones are removed the +first time something is logged on a given day. The desktop app and the CLI +write to the same file, and can do so at the same time without losing entries. + +Each entry is one line that starts with its timestamp: local time, followed by +its offset from UTC (`2026-11-10 14:03:22.123 -06:00 [INFO] ...`). Text that came from a repository, such as a file path, is written with control characters shown as `\uXXXX`, and the lines of an exception are indented under their entry, so nothing in a repository can add or imitate an entry. Everything the CLI prints as text shows such characters the same way, and on a console whose code page cannot represent a character (code page 437 or 850, for example) that character is shown in the same form too; the JSON modes use JSON's own escaping. ## Exit codes diff --git a/gclo.Engine.Tests/AccountsStoreTests.cs b/gclo.Engine.Tests/AccountsStoreTests.cs index c978ea0..95d0948 100644 --- a/gclo.Engine.Tests/AccountsStoreTests.cs +++ b/gclo.Engine.Tests/AccountsStoreTests.cs @@ -363,4 +363,37 @@ public sealed class AccountsStoreTests : IDisposable Assert.False(File.Exists(Path.Combine(_root, "accounts.json.tmp"))); Assert.Equal(2, NewStore().GetAll().Count); } + + + + // ---------------------------------------------------------------- a concurrency read from disk (#151) + + [Theory] + [InlineData(0, AppSettings.MinConcurrency)] + [InlineData(-7, AppSettings.MinConcurrency)] + [InlineData(5000, AppSettings.MaxConcurrency)] + [InlineData(12, 12)] + public void Load_AConcurrencyOutsideTheSupportedRange_IsBroughtBackIntoIt(int stored, int expected) + { + Directory.CreateDirectory(_root); + File.WriteAllText( + Path.Combine(_root, "accounts.json"), + $$"""[{"Id":"{{Guid.NewGuid()}}","Name":"Edited","Organization":"acme","TargetRoot":"C:\\repos","MaxConcurrency":{{stored}}}]"""); + + Account loaded = Assert.Single(NewStore().GetAll()); + + Assert.Equal(expected, loaded.MaxConcurrency); + } + + + + [Fact] + public void Account_AConcurrencyOutsideTheSupportedRange_IsBroughtBackIntoIt_HoweverItIsSet() + { + Account account = MakeAccount("Bounds") with { MaxConcurrency = 0 }; + + Assert.Equal(AppSettings.MinConcurrency, account.MaxConcurrency); + Assert.Equal(AppSettings.MaxConcurrency, (account with { MaxConcurrency = int.MaxValue }).MaxConcurrency); + Assert.Equal(AppSettings.DefaultConcurrency, MakeAccount("Default").MaxConcurrency); + } } diff --git a/gclo.Engine.Tests/ActivityLogTests.cs b/gclo.Engine.Tests/ActivityLogTests.cs index b35dd11..c592039 100644 --- a/gclo.Engine.Tests/ActivityLogTests.cs +++ b/gclo.Engine.Tests/ActivityLogTests.cs @@ -3,6 +3,7 @@ * SPDX-License-Identifier: MIT */ +using System.Diagnostics; using System.Globalization; using System.Text.RegularExpressions; @@ -17,9 +18,9 @@ namespace gclo.Engine.Tests; /// public sealed class ActivityLogTests : IDisposable { - /// "yyyy-MM-dd HH:mm:ss.fff [LEVEL] " prefix every entry line must carry. + /// "yyyy-MM-dd HH:mm:ss.fff +hh:mm [LEVEL] " prefix every entry line must carry. private static readonly Regex s_linePattern = new( - @"^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}\.\d{3} \[(INFO|ERROR)\] ", RegexOptions.Compiled | RegexOptions.ExplicitCapture, TimeSpan.FromSeconds(1)); + @"^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}\.\d{3} [+-]\d{2}:\d{2} \[(INFO|ERROR)\] ", RegexOptions.Compiled | RegexOptions.ExplicitCapture, TimeSpan.FromSeconds(1)); private readonly string _root = Path.Combine(Path.GetTempPath(), "gclo-tests", Guid.NewGuid().ToString("N")); @@ -87,14 +88,14 @@ public sealed class ActivityLogTests : IDisposable var log = new FileActivityLog(_root); // What a repository can put into a message: a file path with a newline that // imitates the start of another entry, and a terminal escape sequence. - string forged = "repo failed: bad\n2026-01-01 00:00:00.000 [INFO] all repositories synced" + (char)0x1B + "[2J"; + string forged = "repo failed: bad\n2026-01-01 00:00:00.000 +00:00 [INFO] all repositories synced" + (char)0x1B + "[2J"; log.Error(forged); string line = Assert.Single(File.ReadAllLines(log.CurrentLogFilePath)); Assert.Matches(s_linePattern, line); Assert.Contains("[ERROR] repo failed: bad", line); - Assert.Contains(Visible(0x0A) + "2026-01-01 00:00:00.000 [INFO] all repositories synced", line); + Assert.Contains(Visible(0x0A) + "2026-01-01 00:00:00.000 +00:00 [INFO] all repositories synced", line); Assert.EndsWith(Visible(0x1B) + "[2J", line, StringComparison.Ordinal); Assert.DoesNotContain((char)0x1B, line); } @@ -106,7 +107,7 @@ public sealed class ActivityLogTests : IDisposable { var log = new FileActivityLog(_root); var exception = new InvalidOperationException( - "bad path 'a\n2026-01-01 00:00:00.000 [INFO] forged entry' in tree" + (char)0x1B + "[0m"); + "bad path 'a\n2026-01-01 00:00:00.000 +00:00 [INFO] forged entry' in tree" + (char)0x1B + "[0m"); log.Error("sync blew up", exception); log.Info("next entry"); @@ -120,7 +121,7 @@ public sealed class ActivityLogTests : IDisposable // The indent is what keeps a continuation line from matching an entry's start, // for this pattern and for the Log window's filter alike. Assert.All(lines[1..^1], l => Assert.DoesNotMatch(s_linePattern, l)); - Assert.Contains(lines, l => l.Contains("2026-01-01 00:00:00.000 [INFO] forged entry", StringComparison.Ordinal)); + Assert.Contains(lines, l => l.Contains("2026-01-01 00:00:00.000 +00:00 [INFO] forged entry", StringComparison.Ordinal)); Assert.DoesNotContain(lines, l => l.Contains((char)0x1B)); } @@ -267,6 +268,269 @@ public sealed class ActivityLogTests : IDisposable + // ---------------------------------------------------------------- the clock (#151) + + /// A clock the test sets; it counts how often it is read. + private sealed class SetClock(DateTimeOffset now) : TimeProvider + { + public DateTimeOffset Now { get; set; } = now; + + public int Reads { get; private set; } + + /// What every reading adds to the clock afterwards. + public TimeSpan StepPerRead { get; set; } + + public override TimeZoneInfo LocalTimeZone + => TimeZoneInfo.CreateCustomTimeZone("test", Now.Offset, "test", "test"); + + + + public override DateTimeOffset GetUtcNow() + { + Reads++; + DateTimeOffset reading = Now; + Now += StepPerRead; + return reading.ToUniversalTime(); + } + } + + + + [Fact] + public void Entry_CarriesItsOffsetFromUtc_AndGoesToTheFileOfItsDay() + { + var clock = new SetClock(new DateTimeOffset(2026, 3, 4, 5, 6, 7, 89, TimeSpan.FromHours(-6))); + var log = new FileActivityLog(_root, clock, TimeSpan.Zero); + + log.Info("hello"); + + string file = Path.Combine(_root, "gclo-2026-03-04.log"); + Assert.Equal("2026-03-04 05:06:07.089 -06:00 [INFO] hello", Assert.Single(File.ReadAllLines(file))); + Assert.Equal(file, log.CurrentLogFilePath); + } + + + + [Fact] + public void Entry_ReadsTheClockOnce_SoItsTimestampAndItsFileAgreeAtMidnight() + { + // Every reading moves this clock on by a millisecond, starting at the last one of a day. + var clock = new SetClock(new DateTimeOffset(2026, 3, 4, 23, 59, 59, 999, TimeSpan.FromHours(2))) + { + StepPerRead = TimeSpan.FromMilliseconds(1), + }; + var log = new FileActivityLog(_root, clock, TimeSpan.Zero); + + log.Info("last of the day"); + log.Info("first of the next"); + + Assert.Equal(2, clock.Reads); + Assert.Equal( + "2026-03-04 23:59:59.999 +02:00 [INFO] last of the day", + Assert.Single(File.ReadAllLines(Path.Combine(_root, "gclo-2026-03-04.log")))); + Assert.Equal( + "2026-03-05 00:00:00.000 +02:00 [INFO] first of the next", + Assert.Single(File.ReadAllLines(Path.Combine(_root, "gclo-2026-03-05.log")))); + } + + + + // ---------------------------------------------------------------- old files (#151) + + private string Touch(string name) + { + Directory.CreateDirectory(_root); + string path = Path.Combine(_root, name); + File.WriteAllText(path, "old"); + return path; + } + + + + [Fact] + public void Write_RemovesDailyFilesOlderThanTheRetention_AndNothingElse() + { + var clock = new SetClock(new DateTimeOffset(2026, 3, 31, 12, 0, 0, TimeSpan.Zero)); + var log = new FileActivityLog(_root, clock, TimeSpan.Zero); + string tooOld = Touch("gclo-2026-03-01.log"); + string muchTooOld = Touch("gclo-2019-12-31.log"); + string[] kept = + [ + Touch("gclo-2026-03-02.log"), // the thirtieth day, today included + Touch("gclo-notes.log"), + Touch("gclo-2020-13-45.log"), + Touch("other-2020-01-01.log"), + Touch("gclo-2020-01-01.log.bak"), + Touch("gclo-2020-01-01-copy.log"), + ]; + + log.Info("today"); + + Assert.Equal(30, FileActivityLog.RetentionDays); + Assert.False(File.Exists(tooOld)); + Assert.False(File.Exists(muchTooOld)); + Assert.All(kept, file => Assert.True(File.Exists(file), file)); + Assert.Single(File.ReadAllLines(Path.Combine(_root, "gclo-2026-03-31.log"))); + } + + + + [Fact] + public void Write_LooksForOldFilesOnceADay() + { + var clock = new SetClock(new DateTimeOffset(2026, 3, 31, 12, 0, 0, TimeSpan.Zero)); + var log = new FileActivityLog(_root, clock, TimeSpan.Zero); + log.Info("first of the day"); + string old = Touch("gclo-2020-01-01.log"); + + log.Info("later the same day"); + Assert.True(File.Exists(old)); + + clock.Now = clock.Now.AddDays(1); + log.Info("the next day"); + Assert.False(File.Exists(old)); + } + + + + [Fact] + public void Write_AnOldFileThatCannotBeRemoved_IsLeft_AndTheRestOfTheWorkIsDone() + { + if(!OperatingSystem.IsWindows()) + { + return; // elsewhere a file that is open can still be removed + } + var clock = new SetClock(new DateTimeOffset(2026, 3, 31, 12, 0, 0, TimeSpan.Zero)); + var log = new FileActivityLog(_root, clock, TimeSpan.Zero); + string open = Touch("gclo-2020-01-01.log"); + string free = Touch("gclo-2020-01-02.log"); + + using(new FileStream(open, FileMode.Open, FileAccess.Read, FileShare.Read)) + { + log.Info("today"); + } + + Assert.True(File.Exists(open)); + Assert.False(File.Exists(free)); + Assert.Single(File.ReadAllLines(log.CurrentLogFilePath)); + } + + + + // ---------------------------------------------------------------- two writers of one file (#151) + + /// Holds the file the way another process in the middle of an entry does. + private static FileStream HoldAsAnotherWriter(string path) + => new(path, FileMode.Append, FileAccess.Write, FileShare.Read); + + + + [Fact] + public async Task Write_WhileAnotherWriterHoldsTheFile_WaitsAndThenGetsThrough() + { + if(!OperatingSystem.IsWindows()) + { + return; // file sharing is what keeps the other writer out + } + var log = new FileActivityLog(_root, TimeProvider.System, TimeSpan.FromSeconds(60)); + log.Info("first"); + Task write; + + using(HoldAsAnotherWriter(log.CurrentLogFilePath)) + { + write = Task.Run(() => log.Info("second")); + Task finished = await Task.WhenAny(write, Task.Delay(TimeSpan.FromMilliseconds(200))); + Assert.NotSame(write, finished); // still waiting for the file + } + await write.WaitAsync(TimeSpan.FromSeconds(60)); + + string[] lines = File.ReadAllLines(log.CurrentLogFilePath); + Assert.Equal(2, lines.Length); + Assert.EndsWith("first", lines[0]); + Assert.EndsWith("second", lines[1]); + } + + + + [Fact] + public void Write_WhenTheFileStaysHeld_GivesUpAfterItsPatience_ThenTriesOnceUntilAWriteGetsThrough() + { + if(!OperatingSystem.IsWindows()) + { + return; // file sharing is what keeps the other writer out + } + var patience = TimeSpan.FromMilliseconds(600); + var log = new FileActivityLog(_root, TimeProvider.System, patience); + log.Info("first"); + string path = log.CurrentLogFilePath; + + using(HoldAsAnotherWriter(path)) + { + var clock = Stopwatch.StartNew(); + log.Info("lost after waiting"); + TimeSpan waited = clock.Elapsed; + clock.Restart(); + log.Info("lost without waiting"); + TimeSpan notWaited = clock.Elapsed; + + Assert.True(waited >= patience, $"gave up after {waited}"); + Assert.True(notWaited < patience / 2, $"waited again for {notWaited}"); + } + + // A write that gets through brings the patience back. + log.Info("back"); + using(HoldAsAnotherWriter(path)) + { + var clock = Stopwatch.StartNew(); + log.Info("lost after waiting again"); + Assert.True(clock.Elapsed >= patience, $"gave up after {clock.Elapsed}"); + } + + string[] lines = File.ReadAllLines(path); + Assert.Equal(2, lines.Length); + Assert.EndsWith("first", lines[0]); + Assert.EndsWith("back", lines[1]); + } + + + + [Fact] + public void TwoWritersOfOneFile_LoseNoEntry_AndWriteOnlyWholeLines() + { + if(!OperatingSystem.IsWindows()) + { + return; // file sharing is what keeps the other writer out + } + // Two logs over one folder stand for the desktop app and the CLI. + var app = new FileActivityLog(_root, TimeProvider.System, TimeSpan.FromSeconds(60)); + var cli = new FileActivityLog(_root, TimeProvider.System, TimeSpan.FromSeconds(60)); + const int PerWriter = 150; + + Parallel.Invoke( + () => + { + for(int i = 0; i < PerWriter; i++) + { + app.Info($"app entry {i}"); + } + }, + () => + { + for(int i = 0; i < PerWriter; i++) + { + cli.Info($"cli entry {i}"); + } + }); + + string[] lines = File.ReadAllLines(app.CurrentLogFilePath); + Assert.Equal(2 * PerWriter, lines.Length); + Assert.All(lines, line => Assert.Matches(s_linePattern, line)); + Assert.Equal(PerWriter, lines.Count(l => l.Contains(" app entry ", StringComparison.Ordinal))); + Assert.Equal(PerWriter, lines.Count(l => l.Contains(" cli entry ", StringComparison.Ordinal))); + } + + + // ---------------------------------------------------------------- NullActivityLog [Fact] diff --git a/gclo.Engine.Tests/GatewayCacheTests.cs b/gclo.Engine.Tests/GatewayCacheTests.cs new file mode 100644 index 0000000..9299d38 --- /dev/null +++ b/gclo.Engine.Tests/GatewayCacheTests.cs @@ -0,0 +1,152 @@ +/* + * Copyright (c) 2026 James Maes (KofTwentyTwo) + * SPDX-License-Identifier: MIT + */ + +namespace gclo.Engine.Tests; + + +/// +/// decides which API clients are kept: one per token, a +/// small fixed number of them, the longest unused dropped first, and no token held +/// by the cache itself (#151). +/// +public sealed class GatewayCacheTests +{ + /// A gateway that only has an identity; the cache never calls it. + private sealed class Gateway(string token) : IGitHubGateway + { + public string Token { get; } = token; + + public Task> GetOrganizationRepositoriesPageAsync(string organization, int page) + => throw new NotSupportedException(); + + public Task> GetOwnRepositoriesPageAsync(int page) => throw new NotSupportedException(); + + public Task> GetUserRepositoriesPageAsync(string user, int page) + => throw new NotSupportedException(); + + public Task GetCurrentUserLoginAsync() => throw new NotSupportedException(); + + public Task> GetOrganizationLoginsAsync() => throw new NotSupportedException(); + + public Task GetAccountKindAsync(string login) => throw new NotSupportedException(); + } + + + + private readonly List _created = []; + + + + private GatewayCache NewCache(int capacity) + => new(capacity, token => + { + lock(_created) + { + _created.Add(token); + } + return new Gateway(token); + }); + + + + [Fact] + public void Get_TheSameToken_GivesTheSameGateway_MadeOnce() + { + GatewayCache cache = NewCache(capacity: 2); + + IGitHubGateway first = cache.Get("token-a"); + IGitHubGateway second = cache.Get("token-a"); + + Assert.Same(first, second); + Assert.Equal(["token-a"], _created, StringComparer.Ordinal); + Assert.Equal(1, cache.Count); + } + + + + [Fact] + public void Get_AnotherToken_GivesAnotherGateway_MadeForThatToken() + { + GatewayCache cache = NewCache(capacity: 2); + + var a = (Gateway)cache.Get("token-a"); + var b = (Gateway)cache.Get("token-b"); + + Assert.NotSame(a, b); + Assert.Equal("token-a", a.Token); + Assert.Equal("token-b", b.Token); + Assert.Equal(2, cache.Count); + } + + + + [Fact] + public void Get_WhenFull_DropsOnlyTheGatewayUnusedTheLongest() + { + GatewayCache cache = NewCache(capacity: 2); + IGitHubGateway a = cache.Get("token-a"); + IGitHubGateway b = cache.Get("token-b"); + Assert.Same(a, cache.Get("token-a")); // a is now the more recently used of the two + + cache.Get("token-c"); // full: b goes, a stays + + Assert.Equal(2, cache.Count); + Assert.Same(a, cache.Get("token-a")); + Assert.NotSame(b, cache.Get("token-b")); // made again + Assert.Equal(["token-a", "token-b", "token-c", "token-b"], _created, StringComparer.Ordinal); + } + + + + [Fact] + public void Get_NeverHoldsMoreThanItsCapacity() + { + GatewayCache cache = NewCache(capacity: 3); + + for(int i = 0; i < 50; i++) + { + cache.Get($"token-{i}"); + } + + Assert.Equal(3, cache.Count); + } + + + + [Fact] + public void Get_FromManyThreads_MakesOneGatewayPerToken() + { + GatewayCache cache = NewCache(capacity: 4); + var seen = new System.Collections.Concurrent.ConcurrentBag(); + + Parallel.For(0, 200, i => seen.Add(cache.Get($"token-{i % 4}"))); + + Assert.Equal(4, seen.Distinct().Count()); + Assert.Equal(4, _created.Count); + } + + + + [Fact] + public void KeyOf_IsAHashOfTheToken_NotTheToken() + { + string key = GatewayCache.KeyOf("ghp_exampleTokenValue"); + + Assert.Equal(64, key.Length); // SHA-256, as hexadecimal + Assert.DoesNotContain("ghp_exampleTokenValue", key, StringComparison.OrdinalIgnoreCase); + Assert.Equal(key, GatewayCache.KeyOf("ghp_exampleTokenValue")); + Assert.False(string.Equals(key, GatewayCache.KeyOf("ghp_exampleTokenValuf"), StringComparison.Ordinal)); + } + + + + [Fact] + public void Constructor_RefusesACapacityBelowOne_AndAMissingFactory() + { + Assert.Throws(() => new GatewayCache(0, token => new Gateway(token))); + Assert.Throws(() => new GatewayCache(1, null!)); + Assert.Throws(() => GatewayCache.KeyOf(null!)); + } +} diff --git a/gclo.Engine.Tests/InvalidRepositoryPathsExceptionTests.cs b/gclo.Engine.Tests/InvalidRepositoryPathsExceptionTests.cs index 6b94f86..e3ce475 100644 --- a/gclo.Engine.Tests/InvalidRepositoryPathsExceptionTests.cs +++ b/gclo.Engine.Tests/InvalidRepositoryPathsExceptionTests.cs @@ -17,7 +17,7 @@ public sealed class InvalidRepositoryPathsExceptionTests Assert.StartsWith("1 path in this repository cannot be created on Windows:", ex.Message); Assert.Contains("'aux' ('aux' is a reserved Windows device name)", ex.Message); - Assert.EndsWith("Nothing was checked out.", ex.Message); + Assert.EndsWith("No files were written.", ex.Message); } @@ -35,6 +35,6 @@ public sealed class InvalidRepositoryPathsExceptionTests Assert.StartsWith("5 paths in this repository cannot be created on Windows:", ex.Message); Assert.Contains(" and 2 more", ex.Message); // examples cap at three Assert.DoesNotContain("bad:4", ex.Message); - Assert.EndsWith("Nothing was checked out.", ex.Message); + Assert.EndsWith("No files were written.", ex.Message); } } diff --git a/gclo.Engine.Tests/OrgSyncEngineTests.cs b/gclo.Engine.Tests/OrgSyncEngineTests.cs index 126ca08..0f37cce 100644 --- a/gclo.Engine.Tests/OrgSyncEngineTests.cs +++ b/gclo.Engine.Tests/OrgSyncEngineTests.cs @@ -533,4 +533,65 @@ public sealed class OrgSyncEngineTests : IDisposable Assert.Equal(1, summary.Total); Assert.Single(_git.CloneCalls); } + + + + // ---------------------------------------------------------------- concurrency bounds, cancellation at the end (#151) + + [Theory] + [InlineData(0)] + [InlineData(-3)] + public async Task Sync_AConcurrencyBelowOne_RunsOneRepositoryAtATime(int maxConcurrency) + { + _lister.Repositories = Repos("alpha", "bravo", "charlie", "delta"); + var gate = new Lock(); + int running = 0; + int mostAtOnce = 0; + _git.CloneHandler = async (_, _, _, _, _) => + { + lock(gate) + { + running++; + mostAtOnce = Math.Max(mostAtOnce, running); + } + await Task.Delay(20, CancellationToken.None); + lock(gate) + { + running--; + } + }; + + SyncSummary summary = await CreateEngine().SyncAsync(CreateRequest(maxConcurrency), _progress); + + Assert.Equal(new SyncSummary(Total: 4, Cloned: 4, Updated: 0, Failed: 0, Canceled: 0, WasCanceled: false), summary); + Assert.Equal(1, mostAtOnce); + } + + + + /// Cancels as soon as a repository reports that it is done. + private sealed class CancelWhenDone(CancellationTokenSource source) : IProgress + { + public void Report(RepoProgress value) + { + if(value.Status == SyncStatus.Done) + { + source.Cancel(); + } + } + } + + + + [Fact] + public async Task Sync_CancellationThatArrivesAfterTheLastRepositoryFinished_LeavesACompleteRun() + { + _lister.Repositories = Repos("solo"); + using var cts = new CancellationTokenSource(); + + SyncSummary summary = await CreateEngine().SyncAsync(CreateRequest(), new CancelWhenDone(cts), cts.Token); + + Assert.True(cts.IsCancellationRequested); + Assert.Equal(new SyncSummary(Total: 1, Cloned: 1, Updated: 0, Failed: 0, Canceled: 0, WasCanceled: false), summary); + } } diff --git a/gclo.Engine.Tests/ProcessRunnerTests.cs b/gclo.Engine.Tests/ProcessRunnerTests.cs new file mode 100644 index 0000000..6543aba --- /dev/null +++ b/gclo.Engine.Tests/ProcessRunnerTests.cs @@ -0,0 +1,164 @@ +/* + * Copyright (c) 2026 James Maes (KofTwentyTwo) + * SPDX-License-Identifier: MIT + */ + +using System.Diagnostics; +using System.Globalization; + + +namespace gclo.Engine.Tests; + + +/// +/// against real, local child processes (the Windows +/// command interpreter and PowerShell; nothing leaves the machine). What matters is +/// what the caller gets back and that no child is left behind (#151). +/// +public sealed class ProcessRunnerTests : IDisposable +{ + private readonly string _root = + Path.Combine(Path.GetTempPath(), "gclo-tests", Guid.NewGuid().ToString("N")); + + private readonly ProcessRunner _runner = new(); + + + + public void Dispose() => GitTestHelpers.TryDeleteDirectory(_root); + + + + [Fact] + public async Task Run_ReturnsTheExitCodeAndWhatTheChildPrinted() + { + if(!OperatingSystem.IsWindows()) + { + return; // drives cmd.exe + } + + ProcessResult result = await _runner.RunAsync( + "cmd.exe", ["/d", "/c", "echo out& echo err 1>&2& exit 3"], environment: null, standardInput: null, CancellationToken.None); + + Assert.Equal(3, result.ExitCode); + Assert.Equal("out", result.StandardOutput.Trim()); + Assert.Equal("err", result.StandardError.Trim()); + } + + + + [Fact] + public async Task Run_PassesStandardInputAndTheEnvironmentToTheChild() + { + if(!OperatingSystem.IsWindows()) + { + return; // drives cmd.exe + } + + ProcessResult result = await _runner.RunAsync( + "cmd.exe", + ["/d", "/c", "echo %GCLO_TEST_VALUE%& more"], + new Dictionary(StringComparer.Ordinal) { ["GCLO_TEST_VALUE"] = "from-the-environment" }, + "from standard input", + CancellationToken.None); + + Assert.Equal(0, result.ExitCode); + Assert.Contains("from-the-environment", result.StandardOutput, StringComparison.Ordinal); + Assert.Contains("from standard input", result.StandardOutput, StringComparison.Ordinal); + } + + + + [Fact] + public async Task Run_AChildThatExitsWithoutReadingItsInput_AnswersWithItsExitCodeAndErrorOutput() + { + if(!OperatingSystem.IsWindows()) + { + return; // drives cmd.exe + } + // Far more than a pipe holds, so the write is still under way when the child is gone. + string input = new('x', 4 * 1024 * 1024); + + ProcessResult result = await _runner.RunAsync( + "cmd.exe", ["/d", "/c", "echo not today 1>&2& exit 7"], environment: null, input, CancellationToken.None) + .WaitAsync(TimeSpan.FromSeconds(60)); + + Assert.Equal(7, result.ExitCode); + Assert.Equal("not today", result.StandardError.Trim()); + } + + + + [Fact] + public async Task Run_Canceled_EndsTheChild_AndThrowsCancellation() + { + if(!OperatingSystem.IsWindows()) + { + return; // drives powershell.exe + } + Directory.CreateDirectory(_root); + string idFile = Path.Combine(_root, "child.id"); + using var cts = new CancellationTokenSource(); + + // The child says who it is, then sleeps far longer than the test runs. + Task run = _runner.RunAsync( + "powershell.exe", + ["-NoProfile", "-NonInteractive", "-Command", "Set-Content -LiteralPath $env:GCLO_TEST_ID_FILE -Value $PID; Start-Sleep -Seconds 600"], + new Dictionary(StringComparer.Ordinal) { ["GCLO_TEST_ID_FILE"] = idFile }, + standardInput: null, + cts.Token); + int childId = await ReadChildIdAsync(idFile, run); + + cts.Cancel(); + + await Assert.ThrowsAnyAsync(() => run.WaitAsync(TimeSpan.FromSeconds(60))); + Assert.True(await IsGoneAsync(childId), $"child process {childId} is still running after cancellation"); + } + + + + private static async Task ReadChildIdAsync(string idFile, Task run) + { + var clock = Stopwatch.StartNew(); + while(clock.Elapsed < TimeSpan.FromSeconds(60)) + { + Assert.False(run.IsCompleted, "the child ended before it said who it is"); + try + { + if(File.Exists(idFile) && int.TryParse(File.ReadAllText(idFile).Trim(), NumberStyles.Integer, CultureInfo.InvariantCulture, out int id)) + { + return id; + } + } + catch(IOException) + { + // Still being written. + } + await Task.Delay(50); + } + throw new TimeoutException("the child never said who it is"); + } + + + + private static async Task IsGoneAsync(int processId) + { + var clock = Stopwatch.StartNew(); + while(clock.Elapsed < TimeSpan.FromSeconds(30)) + { + try + { + using var process = Process.GetProcessById(processId); + if(process.HasExited) + { + return true; + } + } + catch(ArgumentException) + { + return true; // no such process any more + } + await Task.Delay(50); + } + return false; + } +} diff --git a/gclo.Engine.Tests/RepositoryPathResolverTests.cs b/gclo.Engine.Tests/RepositoryPathResolverTests.cs index 17a32fe..1e2d3c5 100644 --- a/gclo.Engine.Tests/RepositoryPathResolverTests.cs +++ b/gclo.Engine.Tests/RepositoryPathResolverTests.cs @@ -187,4 +187,47 @@ public sealed class RepositoryPathResolverTests Assert.EndsWith(Path.DirectorySeparatorChar + "alpha", resolved); } + + + + // ---------------------------------------------------------------- a drive root as the target (#151) + + [Fact] + public void Resolve_TargetIsTheRootOfADrive_ReturnsTheFolderDirectlyUnderIt() + { + // 'C:\' on Windows, '/' elsewhere: a root keeps its separator when it is trimmed. + string driveRoot = Path.GetPathRoot(Path.GetFullPath(s_root))!; + + string resolved = RepositoryPathResolver.Resolve(driveRoot, "gclo"); + + Assert.Equal(driveRoot + "gclo", resolved); + Assert.Equal(driveRoot, Path.GetDirectoryName(resolved)); + } + + + + [Fact] + public void Resolve_TargetIsTheRootOfADrive_StillRefusesANameThatLeavesIt() + { + string driveRoot = Path.GetPathRoot(Path.GetFullPath(s_root))!; + + Assert.Throws(() => RepositoryPathResolver.Resolve(driveRoot, "..")); + Assert.Throws(() => RepositoryPathResolver.Resolve(driveRoot, "a/b")); + } + + + + [Theory] + [InlineData("a?b")] + [InlineData("a*b")] + [InlineData("a|b")] + [InlineData("ab")] + [InlineData("a\"b")] + [InlineData("a\tb")] + public void TryValidateName_ACharacterWindowsRefuses_IsRefusedOnEveryPlatform(string name) + { + Assert.False(RepositoryPathResolver.TryValidateName(name, out string? reason)); + Assert.Equal("it contains characters that are not allowed in a folder name", reason); + } } diff --git a/gclo.Engine.Tests/WindowsPathValidationTests.cs b/gclo.Engine.Tests/WindowsPathValidationTests.cs index 8aa8b9f..1102467 100644 --- a/gclo.Engine.Tests/WindowsPathValidationTests.cs +++ b/gclo.Engine.Tests/WindowsPathValidationTests.cs @@ -473,4 +473,24 @@ public sealed class WindowsPathValidationTests : IDisposable repo.Commit("long path commit", signature, signature); return path; } + + + + // ---------------------------------------------------------------- the fixed character set (#151) + + [Fact] + public void InvalidNameCharacters_AreTheWindowsSet_WhereverTheRulesRun() + { + char[] characters = WindowsPathValidator.InvalidNameCharacters(); + + // The 32 control characters and nine more; no duplicates. + Assert.Equal(41, characters.Length); + Assert.Equal(41, characters.Distinct().Count()); + Assert.All(Enumerable.Range(0, 32), i => Assert.Contains((char)i, characters)); + Assert.All("\"<>|:*?\\/", c => Assert.Contains(c, characters)); + if(OperatingSystem.IsWindows()) + { + Assert.Equal(Path.GetInvalidFileNameChars().Order(), characters.Order()); + } + } } diff --git a/gclo.Engine/FileActivityLog.cs b/gclo.Engine/FileActivityLog.cs index e318f86..b67a159 100644 --- a/gclo.Engine/FileActivityLog.cs +++ b/gclo.Engine/FileActivityLog.cs @@ -3,7 +3,9 @@ * SPDX-License-Identifier: MIT */ +using System.Diagnostics; using System.Globalization; +using System.Text; namespace gclo.Engine; @@ -13,17 +15,51 @@ namespace gclo.Engine; /// An that appends timestamped lines to a daily file, /// gclo-yyyy-MM-dd.log. Writes are serialized by a lock and each one opens, /// appends, and closes the file, so every entry reaches disk even if the process -/// dies right after. Logging must never take the application down: all IO -/// failures (unwritable directory, locked file, full disk) are swallowed. The -/// directory is created lazily on the first write, never in the constructor. +/// dies right after. The desktop app and the CLI write to the same file: a write +/// that finds the file held by the other waits briefly and tries again. Files +/// older than days are removed. Logging must never +/// take the application down: all IO failures (unwritable directory, locked file, +/// full disk) are swallowed. The directory is created lazily on the first write, +/// never in the constructor. /// public sealed class FileActivityLog : IActivityLog { + /// + /// How many days of log files are kept, today included. A daily file older than + /// that is removed the first time an entry is written on a given day. + /// + public const int RetentionDays = 30; + /// What every line of an exception's text starts with, under its entry. private const string ContinuationIndent = " "; + private const string FilePrefix = "gclo-"; + + private const string FileDateFormat = "yyyy-MM-dd"; + + /// How long one write waits for a file another process is writing to. + private static readonly TimeSpan s_defaultContentionBudget = TimeSpan.FromMilliseconds(200); + + private static readonly TimeSpan s_retryPause = TimeSpan.FromMilliseconds(1); + + private static readonly UTF8Encoding s_utf8 = new(encoderShouldEmitUTF8Identifier: false); + private readonly Lock _gate = new(); + private readonly TimeProvider _time; + + private readonly TimeSpan _contentionBudget; + + /// The day old files were last removed for; the work is done once a day. + private DateOnly _prunedFor; + + /// + /// True after a write gave up on a file it could not open. Until a write gets + /// through again, each one tries once and does not wait, so a file that stays + /// locked costs the operations being logged nothing. + /// + private bool _held; + /// Creates a log that writes under . @@ -33,12 +69,22 @@ public sealed class FileActivityLog : IActivityLog /// install root %LOCALAPPDATA%\gclo, which Setup.exe deletes, #91). /// public FileActivityLog(string? directory = null) + : this(directory, TimeProvider.System, s_defaultContentionBudget) + { + } + + + + /// Creates a log with its own clock and its own patience, for tests. + internal FileActivityLog(string? directory, TimeProvider time, TimeSpan contentionBudget) { LogDirectory = string.IsNullOrWhiteSpace(directory) ? Path.Combine( Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData), "KofTwentyTwo", "gclo", "logs") : directory; + _time = time; + _contentionBudget = contentionBudget; } @@ -49,10 +95,7 @@ public sealed class FileActivityLog : IActivityLog /// Today's log file; entries roll over to a new file at midnight. - public string CurrentLogFilePath - => Path.Combine( - LogDirectory, - "gclo-" + DateTime.Now.ToString("yyyy-MM-dd", CultureInfo.InvariantCulture) + ".log"); + public string CurrentLogFilePath => PathFor(_time.GetLocalNow()); @@ -66,16 +109,30 @@ public sealed class FileActivityLog : IActivityLog + private string PathFor(DateTimeOffset moment) + => Path.Combine( + LogDirectory, + FilePrefix + moment.ToString(FileDateFormat, CultureInfo.InvariantCulture) + ".log"); + + + private void Write(string level, string message, Exception? exception) { try { lock(_gate) { + /////////////////////////////////////////////////////////////////////////// + // The clock is read once per entry, so the timestamp and the file the // + // entry goes to always agree, across midnight too. The timestamp // + // carries its offset from UTC: a log that spans a daylight-saving // + // change, or is read on another machine, still says when each thing // + // happened. // + /////////////////////////////////////////////////////////////////////////// + DateTimeOffset now = _time.GetLocalNow(); Directory.CreateDirectory(LogDirectory); - string timestamp = DateTime.Now.ToString( - "yyyy-MM-dd HH:mm:ss.fff", CultureInfo.InvariantCulture); + string timestamp = now.ToString("yyyy-MM-dd HH:mm:ss.fff zzz", CultureInfo.InvariantCulture); /////////////////////////////////////////////////////////////////////////// // An entry is one line that starts with its timestamp; readers (the Log // // window's error filter among them) rely on that. Messages carry text // @@ -89,7 +146,8 @@ public sealed class FileActivityLog : IActivityLog entry += Environment.NewLine + DisplayText.EscapeLines(exception.ToString(), ContinuationIndent); } - File.AppendAllText(CurrentLogFilePath, entry + Environment.NewLine); + Append(PathFor(now), entry + Environment.NewLine); + RemoveOldFiles(DateOnly.FromDateTime(now.DateTime)); } } catch @@ -97,4 +155,75 @@ public sealed class FileActivityLog : IActivityLog // A logging failure must never take the operation it describes down. } } + + + + /// + /// Appends as one write to a file opened for this writer + /// alone, so entries from two processes never interleave. Another process in the + /// middle of its own entry holds the file for a moment: the write is tried again + /// until it gets through or its patience runs out. + /// + private void Append(string path, string text) + { + byte[] bytes = s_utf8.GetBytes(text); + long started = Stopwatch.GetTimestamp(); + while(true) + { + try + { + using(var file = new FileStream(path, FileMode.Append, FileAccess.Write, FileShare.Read)) + { + file.Write(bytes); + } + _held = false; + return; + } + catch(IOException) when(!_held && Stopwatch.GetElapsedTime(started) < _contentionBudget) + { + Thread.Sleep(s_retryPause); + } + catch(IOException) + { + _held = true; + throw; + } + } + } + + + + /// + /// Removes the daily files older than days, once per + /// day. Only files named exactly like a daily log are touched; one that cannot be + /// removed (open in a viewer, read-only) is left for another day. + /// + private void RemoveOldFiles(DateOnly today) + { + if(_prunedFor == today) + { + return; + } + _prunedFor = today; + + DateOnly oldestKept = today.AddDays(1 - RetentionDays); + foreach(string file in Directory.EnumerateFiles(LogDirectory, FilePrefix + "*.log")) + { + ReadOnlySpan date = Path.GetFileNameWithoutExtension(file.AsSpan())[FilePrefix.Length..]; + if(!DateOnly.TryParseExact(date, FileDateFormat, CultureInfo.InvariantCulture, DateTimeStyles.None, out DateOnly day) + || day >= oldestKept) + { + continue; + } + + try + { + File.Delete(file); + } + catch(Exception ex) when(ex is IOException or UnauthorizedAccessException) + { + // Left for another day. + } + } + } } diff --git a/gclo.Engine/GatewayCache.cs b/gclo.Engine/GatewayCache.cs new file mode 100644 index 0000000..db0fdc8 --- /dev/null +++ b/gclo.Engine/GatewayCache.cs @@ -0,0 +1,95 @@ +/* + * Copyright (c) 2026 James Maes (KofTwentyTwo) + * SPDX-License-Identifier: MIT + */ + +using System.Security.Cryptography; +using System.Text; + + +namespace gclo.Engine; + + +/// +/// Keeps the few most recently used gateways, one per token, so successive lookups +/// with one token share a client and its warm connection. Bounded: when a new token +/// arrives and the cache is full, the gateway that has gone unused the longest is +/// dropped, and only that one. Entries are keyed by a hash of the token, so the +/// cache itself holds no token; a dropped gateway is not disposed, because a request +/// that started before it was dropped may still be using it, and it is released +/// with its connections once nothing refers to it. +/// +internal sealed class GatewayCache +{ + private readonly Lock _gate = new(); + + private readonly int _capacity; + + private readonly Func _create; + + /// Most recently used first. + private readonly LinkedList<(string Key, IGitHubGateway Gateway)> _entries = new(); + + + + /// How many gateways are kept; at least one. + /// Makes the gateway for a token that has none. + public GatewayCache(int capacity, Func create) + { + ArgumentOutOfRangeException.ThrowIfLessThan(capacity, 1); + ArgumentNullException.ThrowIfNull(create); + _capacity = capacity; + _create = create; + } + + + + /// How many gateways are held right now. + public int Count + { + get + { + lock(_gate) + { + return _entries.Count; + } + } + } + + + + /// The gateway for : the one in use for it, or a new one. + public IGitHubGateway Get(string token) + { + string key = KeyOf(token); + lock(_gate) + { + for(LinkedListNode<(string Key, IGitHubGateway Gateway)>? node = _entries.First; node is not null; node = node.Next) + { + if(string.Equals(node.Value.Key, key, StringComparison.Ordinal)) + { + _entries.Remove(node); + _entries.AddFirst(node); + return node.Value.Gateway; + } + } + + IGitHubGateway gateway = _create(token); + _entries.AddFirst((key, gateway)); + if(_entries.Count > _capacity) + { + _entries.RemoveLast(); + } + return gateway; + } + } + + + + /// What an entry is filed under: a SHA-256 of the token, never the token. + internal static string KeyOf(string token) + { + ArgumentNullException.ThrowIfNull(token); + return Convert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(token))); + } +} diff --git a/gclo.Engine/InvalidRepositoryPathsException.cs b/gclo.Engine/InvalidRepositoryPathsException.cs index f6d96b0..6fa1427 100644 --- a/gclo.Engine/InvalidRepositoryPathsException.cs +++ b/gclo.Engine/InvalidRepositoryPathsException.cs @@ -33,6 +33,6 @@ public sealed class InvalidRepositoryPathsException : Exception string suffix = paths.Count > 3 ? $" and {paths.Count - 3} more" : ""; string subject = paths.Count == 1 ? "1 path" : $"{paths.Count} paths"; return $"{subject} in this repository cannot be created on Windows: {examples}{suffix}. " + - "Nothing was checked out."; + "No files were written."; } } diff --git a/gclo.Engine/OctokitGatewayCache.cs b/gclo.Engine/OctokitGatewayCache.cs index c3fee61..4f3a023 100644 --- a/gclo.Engine/OctokitGatewayCache.cs +++ b/gclo.Engine/OctokitGatewayCache.cs @@ -10,29 +10,20 @@ namespace gclo.Engine; /// -/// One per token, shared by both listers for the life of -/// the process, so successive org lookups and repository loads ride one warm HTTP -/// connection instead of paying a DNS + TCP + TLS handshake to api.github.com on -/// every interactive action (#30). Octokit clients are thread-safe for concurrent -/// requests with fixed credentials. Bounded: tokens are rotated rarely, so when the -/// cache grows past a handful of entries it is simply emptied. +/// One per token, shared by both listers, so successive +/// org lookups and repository loads ride one warm HTTP connection instead of paying +/// a DNS + TCP + TLS handshake to api.github.com on every interactive action (#30). +/// Octokit clients are thread-safe for concurrent requests with fixed credentials. +/// Which gateways are kept, and for how long, is 's rule. /// [ExcludeFromCodeCoverage(Justification = "Holds live Octokit clients; the offline suite injects fake gateways.")] internal static class OctokitGatewayCache { - private const int MaxEntries = 8; + private const int MaxEntries = 4; - private static readonly System.Collections.Concurrent.ConcurrentDictionary s_gateways = - new(StringComparer.Ordinal); + private static readonly GatewayCache s_gateways = new(MaxEntries, static token => new OctokitGateway(token)); - public static IGitHubGateway Get(string token) - { - if(s_gateways.Count >= MaxEntries && !s_gateways.ContainsKey(token)) - { - s_gateways.Clear(); - } - return s_gateways.GetOrAdd(token, static t => new OctokitGateway(t)); - } + public static IGitHubGateway Get(string token) => s_gateways.Get(token); } diff --git a/gclo.Engine/OrgSyncEngine.cs b/gclo.Engine/OrgSyncEngine.cs index eaf5d43..38c1d1d 100644 --- a/gclo.Engine/OrgSyncEngine.cs +++ b/gclo.Engine/OrgSyncEngine.cs @@ -184,6 +184,10 @@ public sealed class OrgSyncEngine } } - return new SyncSummary(repos.Count, cloned, updated, failed, canceled, cancellationToken.IsCancellationRequested); + /////////////////////////////////////////////////////////////////////////////// + // Cut short means a repository did not get its turn or did not finish. A // + // cancellation that arrives after the last one finished cut nothing short. // + /////////////////////////////////////////////////////////////////////////////// + return new SyncSummary(repos.Count, cloned, updated, failed, canceled, WasCanceled: canceled > 0); } } diff --git a/gclo.Engine/ProcessRunner.cs b/gclo.Engine/ProcessRunner.cs index a6042c8..a9647b7 100644 --- a/gclo.Engine/ProcessRunner.cs +++ b/gclo.Engine/ProcessRunner.cs @@ -50,30 +50,59 @@ public sealed class ProcessRunner : IProcessRunner using var process = new Process { StartInfo = start }; process.Start(); - using var stdout = new MemoryStream(); - using var stderr = new MemoryStream(); - Task copyOut = process.StandardOutput.BaseStream.CopyToAsync(stdout, cancellationToken); - Task copyErr = process.StandardError.BaseStream.CopyToAsync(stderr, cancellationToken); - if(standardInput is not null) - { - await process.StandardInput.WriteAsync(standardInput.AsMemory(), cancellationToken).ConfigureAwait(false); - } - process.StandardInput.Close(); - + /////////////////////////////////////////////////////////////////////////////// + // From here on the child exists, so every way out of this method, not only // + // cancellation, goes through the finally that ends it if it is still alive. // + /////////////////////////////////////////////////////////////////////////////// try { + using var stdout = new MemoryStream(); + using var stderr = new MemoryStream(); + Task copyOut = process.StandardOutput.BaseStream.CopyToAsync(stdout, cancellationToken); + Task copyErr = process.StandardError.BaseStream.CopyToAsync(stderr, cancellationToken); + try + { + if(standardInput is not null) + { + await process.StandardInput.WriteAsync(standardInput.AsMemory(), cancellationToken).ConfigureAwait(false); + } + process.StandardInput.Close(); + } + catch(IOException) + { + //////////////////////////////////////////////////////////////////////// + // The child closed its input, or exited, before it had read all of // + // it. That is its answer, not a failure to run it: its exit code and // + // what it printed say why, and they are what the caller gets. // + //////////////////////////////////////////////////////////////////////// + } + await process.WaitForExitAsync(cancellationToken).ConfigureAwait(false); await Task.WhenAll(copyOut, copyErr).ConfigureAwait(false); + return new ProcessResult(process.ExitCode, Decode(stdout.ToArray()), Decode(stderr.ToArray())); } - catch(OperationCanceledException) + finally { - try - { process.Kill(entireProcessTree: true); } - catch(InvalidOperationException) { /* already exited */ } - throw; + EndIfStillRunning(process); } + } - return new ProcessResult(process.ExitCode, Decode(stdout.ToArray()), Decode(stderr.ToArray())); + + + private static void EndIfStillRunning(Process process) + { + try + { + if(!process.HasExited) + { + process.Kill(entireProcessTree: true); + } + } + catch(Exception ex) when(ex is InvalidOperationException or System.ComponentModel.Win32Exception + or NotSupportedException or AggregateException) + { + // It exited in between, or it (or one of its children) cannot be ended from here. + } } diff --git a/gclo.Engine/RepositoryPathResolver.cs b/gclo.Engine/RepositoryPathResolver.cs index 4194870..466d65b 100644 --- a/gclo.Engine/RepositoryPathResolver.cs +++ b/gclo.Engine/RepositoryPathResolver.cs @@ -18,7 +18,9 @@ public static class RepositoryPathResolver { private static readonly char[] s_separatorChars = ['/', '\\']; - private static readonly char[] s_invalidNameChars = Path.GetInvalidFileNameChars(); + // The Windows set on every platform, so a name's validity does not depend on + // where it is checked. + private static readonly char[] s_invalidNameChars = WindowsPathValidator.InvalidNameCharacters(); // Windows paths compare case-insensitively; everywhere else the file system is // (by default) case-sensitive and so is the containment check. @@ -50,7 +52,14 @@ public static class RepositoryPathResolver // level below the root. GetFullPath collapses '.'/'..' segments and, on // Windows, trims trailing dots and spaces — so a name that survived the // textual checks but still lands on or outside the root is caught here. - string root = Path.TrimEndingDirectorySeparator(Path.GetFullPath(targetRoot)) + Path.DirectorySeparatorChar; + // The root ends in exactly one separator. A drive root ('C:\', or '/') keeps + // the one it has: trimming leaves it in place, and a second one would put every + // candidate outside the root. + string root = Path.TrimEndingDirectorySeparator(Path.GetFullPath(targetRoot)); + if(!Path.EndsInDirectorySeparator(root)) + { + root += Path.DirectorySeparatorChar; + } string candidate = Path.GetFullPath(Path.Combine(root, repoName)); if(!candidate.StartsWith(root, s_pathComparison) || candidate.Length <= root.Length) @@ -68,7 +77,8 @@ public static class RepositoryPathResolver /// /// True when is a single, safe folder-name segment: /// not blank, not . or .., no directory separators (so never rooted - /// or UNC), no drive or stream colon, and no characters a file system rejects in a name. + /// or UNC), no drive or stream colon, and no characters Windows rejects in a name + /// (the same answer on every platform). /// /// The candidate name. /// Why the name was rejected; null when it is valid. diff --git a/gclo.Engine/SyncRequest.cs b/gclo.Engine/SyncRequest.cs index 6a0993c..13859b7 100644 --- a/gclo.Engine/SyncRequest.cs +++ b/gclo.Engine/SyncRequest.cs @@ -10,7 +10,9 @@ namespace gclo.Engine; /// GitHub organization login name. /// GitHub Personal Access Token used for both the API and git transport. /// Local folder under which each repository is placed as a subfolder. -/// Upper bound on simultaneous git operations. +/// +/// Upper bound on simultaneous git operations. A value below one runs one at a time. +/// public sealed record SyncRequest(string Organization, string Token, string TargetRoot, int MaxConcurrency = 8) { /// diff --git a/gclo.Engine/SyncSummary.cs b/gclo.Engine/SyncSummary.cs index b3f3e03..f842e96 100644 --- a/gclo.Engine/SyncSummary.cs +++ b/gclo.Engine/SyncSummary.cs @@ -12,5 +12,9 @@ namespace gclo.Engine; /// Existing repositories fetched and fast-forwarded. /// Repositories that failed; a failure never aborts the others. /// Repositories canceled before or during processing. -/// Whether the run was cut short by cancellation. +/// +/// Whether the run was cut short by cancellation: true exactly when +/// is above zero. A cancellation that arrives after the +/// last repository finished leaves a complete run. +/// public sealed record SyncSummary(int Total, int Cloned, int Updated, int Failed, int Canceled, bool WasCanceled); diff --git a/gclo.Engine/WindowsPathValidator.cs b/gclo.Engine/WindowsPathValidator.cs index b50191f..36f90d8 100644 --- a/gclo.Engine/WindowsPathValidator.cs +++ b/gclo.Engine/WindowsPathValidator.cs @@ -20,7 +20,7 @@ public static class WindowsPathValidator { private const int MaxSegmentLength = 255; - private static readonly char[] s_invalidChars = Path.GetInvalidFileNameChars(); + private static readonly char[] s_invalidChars = InvalidNameCharacters(); private static readonly HashSet s_reservedNames = new(StringComparer.OrdinalIgnoreCase) { @@ -31,6 +31,17 @@ public static class WindowsPathValidator + /// + /// The characters Windows refuses in a file or folder name: the control characters + /// U+0000 to U+001F, and " < > | : * ? \ /. Written out here and not + /// asked of the running platform, so these rules give the same answer wherever + /// they run. + /// + internal static char[] InvalidNameCharacters() + => [.. Enumerable.Range(0, 32).Select(i => (char)i), '"', '<', '>', '|', ':', '*', '?', '\\', '/']; + + + /// /// Returns every Windows-invalid path in ; empty when all /// are fine. Flattens the tree to its leaf paths and runs them through diff --git a/gclo.ViewModels/Account.cs b/gclo.ViewModels/Account.cs index facefd1..1c260fa 100644 --- a/gclo.ViewModels/Account.cs +++ b/gclo.ViewModels/Account.cs @@ -52,8 +52,17 @@ public sealed record Account /// public bool CreateOrgSubfolder { get; init; } - /// Parallel clone/pull count used when syncing this account. - public int MaxConcurrency { get; init; } = AppSettings.DefaultConcurrency; + /// + /// Parallel clone/pull count used when syncing this account. Always between + /// and : + /// a value outside that range (an accounts.json edited by hand) is brought back + /// into it when it is read, so every user of the account gets a usable number. + /// + public int MaxConcurrency + { + get; + init => field = Math.Clamp(value, AppSettings.MinConcurrency, AppSettings.MaxConcurrency); + } = AppSettings.DefaultConcurrency; /// When the account's last sync finished, or null if it never ran. public DateTimeOffset? LastSyncUtc { get; init; } -- 2.54.0 From 61fb8c253b52204022d6d0b2ecbc556b2dbed17a Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 10 Nov 2026 15:32:31 -0600 Subject: [PATCH 2/2] fix(engine): address the independent review of the small fixes An absent concurrency is the default, not the minimum; the invalid-paths sentence is about the working tree; a log that gave up on a held file does not wait again before a cool-down; the cache keeps eight clients and hashes the token as it is; the CLI marks a run as canceled when cancellation kept a path recovery from happening; tests no longer depend on the wall clock. Refs #151 Co-Authored-By: Claude Fable 5.2 Signed-off-by: James Maes --- docs/CLI.md | 6 +- gclo.Cli.Tests/SyncCommandTests.cs | 2 + gclo.Cli/SyncCommand.cs | 52 ++++++++++--- gclo.Engine.Tests/AccountsStoreTests.cs | 32 +++++++- gclo.Engine.Tests/ActivityLogTests.cs | 76 ++++++++++++++----- gclo.Engine.Tests/GatewayCacheTests.cs | 12 +++ .../InvalidRepositoryPathsExceptionTests.cs | 4 +- gclo.Engine.Tests/ProcessRunnerTests.cs | 12 ++- gclo.Engine/FileActivityLog.cs | 42 ++++++---- gclo.Engine/GatewayCache.cs | 10 ++- .../InvalidRepositoryPathsException.cs | 2 +- gclo.Engine/OctokitGatewayCache.cs | 3 +- gclo.Engine/ProcessRunner.cs | 6 +- gclo.Engine/SyncSummary.cs | 8 +- gclo.ViewModels/Account.cs | 23 ++++-- 15 files changed, 224 insertions(+), 66 deletions(-) diff --git a/docs/CLI.md b/docs/CLI.md index 8639a1e..1648af5 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -388,8 +388,10 @@ under **View > Activity log**. Tokens are never written to the log, or anywhere else on disk; see [SECURITY.md](../SECURITY.md). Daily files are kept for 30 days, today included; older ones are removed the -first time something is logged on a given day. The desktop app and the CLI -write to the same file, and can do so at the same time without losing entries. +first time something is logged on a given day. When the desktop app and the +CLI log to the same file (they do unless `GCLO_DATA_DIR` moves the app's data), +an entry that finds the file in use is tried again for a moment rather than +dropped; if the file stays in use, entries are dropped until it is free again. Each entry is one line that starts with its timestamp: local time, followed by its offset from UTC (`2026-11-10 14:03:22.123 -06:00 [INFO] ...`). Text that came from a repository, such as a file path, is written with control characters shown as `\uXXXX`, and the lines of an exception are indented under their entry, so nothing in a repository can add or imitate an entry. Everything the CLI prints as text shows such characters the same way, and on a console whose code page cannot represent a character (code page 437 or 850, for example) that character is shown in the same form too; the JSON modes use JSON's own escaping. diff --git a/gclo.Cli.Tests/SyncCommandTests.cs b/gclo.Cli.Tests/SyncCommandTests.cs index db75a9b..a1102f4 100644 --- a/gclo.Cli.Tests/SyncCommandTests.cs +++ b/gclo.Cli.Tests/SyncCommandTests.cs @@ -295,6 +295,8 @@ public sealed class SyncCommandTests : IDisposable Assert.Equal(1, code); Assert.Empty(git.RecoveredRepoNames); // recovery was never attempted + // The engine finished its one repository; what was cut short is the recovery, and the run says so. + Assert.Contains("Canceled: 0 cloned, 0 updated, 1 failed, 0 canceled of 1.", console.Out); } diff --git a/gclo.Cli/SyncCommand.cs b/gclo.Cli/SyncCommand.cs index 5c2fcb8..fd1d41c 100644 --- a/gclo.Cli/SyncCommand.cs +++ b/gclo.Cli/SyncCommand.cs @@ -389,19 +389,20 @@ internal static class SyncCommand // --sanitize-paths: repositories whose only problem was Windows-invalid // paths were held back by the progress handler; retry each with an // automatic recovery. A recovered repository counts as cloned. - int sanitized = 0; - foreach(PendingRecovery pending in progress.DrainPendingRecoveries()) - { - if(await TrySanitizeAsync(git, progress, printer, log, targetRoot, pending, announce: !machineReadable, cancellationToken) - .ConfigureAwait(false)) - { - sanitized++; - } - } + (int sanitized, bool sanitizationCutShort) = await SanitizeHeldBackAsync( + git, progress, printer, log, targetRoot, announce: !machineReadable, cancellationToken).ConfigureAwait(false); if(sanitized > 0) { summary = summary with { Cloned = summary.Cloned + sanitized, Failed = summary.Failed - sanitized }; } + if(sanitizationCutShort) + { + //////////////////////////////////////////////////////////////////////// + // The engine had finished every repository, so its summary says the // + // run was complete; the recovery that was asked for was not. // + //////////////////////////////////////////////////////////////////////// + summary = summary with { WasCanceled = true }; + } log.Info( $"sync finished: total={summary.Total}, cloned={summary.Cloned}, updated={summary.Updated}, " @@ -518,6 +519,39 @@ internal static class SyncCommand + /// + /// Sanitizes every repository the progress handler held back, one after the other. + /// Says how many now count as cloned, and whether cancellation kept one of them + /// from being sanitized. + /// + private static async Task<(int Sanitized, bool CutShort)> SanitizeHeldBackAsync( + IGitClient git, + SyncProgressHandler progress, + ProgressPrinter printer, + IActivityLog log, + string targetRoot, + bool announce, + CancellationToken cancellationToken) + { + int sanitized = 0; + bool cutShort = false; + foreach(PendingRecovery pending in progress.DrainPendingRecoveries()) + { + if(await TrySanitizeAsync(git, progress, printer, log, targetRoot, pending, announce, cancellationToken) + .ConfigureAwait(false)) + { + sanitized++; + } + else if(cancellationToken.IsCancellationRequested) + { + cutShort = true; + } + } + return (sanitized, cutShort); + } + + + /// /// Applies an automatic (rename to the suggested name, /// skip what has none) to one repository that failed with Windows-invalid paths. diff --git a/gclo.Engine.Tests/AccountsStoreTests.cs b/gclo.Engine.Tests/AccountsStoreTests.cs index 95d0948..7e1047a 100644 --- a/gclo.Engine.Tests/AccountsStoreTests.cs +++ b/gclo.Engine.Tests/AccountsStoreTests.cs @@ -369,9 +369,10 @@ public sealed class AccountsStoreTests : IDisposable // ---------------------------------------------------------------- a concurrency read from disk (#151) [Theory] - [InlineData(0, AppSettings.MinConcurrency)] - [InlineData(-7, AppSettings.MinConcurrency)] + [InlineData(0, AppSettings.DefaultConcurrency)] + [InlineData(-7, AppSettings.DefaultConcurrency)] [InlineData(5000, AppSettings.MaxConcurrency)] + [InlineData(1, 1)] [InlineData(12, 12)] public void Load_AConcurrencyOutsideTheSupportedRange_IsBroughtBackIntoIt(int stored, int expected) { @@ -387,13 +388,38 @@ public sealed class AccountsStoreTests : IDisposable + [Fact] + public void Load_AnAccountWithoutTheOptionalFields_GetsTheirDefaults_AndKeepsThemWhenSaved() + { + // The reader hands an absent field over as zero or null; neither is a setting. + Directory.CreateDirectory(_root); + var id = Guid.NewGuid(); + File.WriteAllText( + Path.Combine(_root, "accounts.json"), + $$"""[{"Id":"{{id}}","Name":"Bare","Organization":"acme","TargetRoot":"C:\\repos"}]"""); + AccountsStore store = NewStore(); + + Account loaded = Assert.Single(store.GetAll()); + store.RecordSyncResult(id, DateTimeOffset.UtcNow, "done"); + Account saved = Assert.Single(NewStore().GetAll()); + + Assert.Equal(AppSettings.DefaultConcurrency, loaded.MaxConcurrency); + Assert.Equal("", loaded.Description); + Assert.Equal(AppSettings.DefaultConcurrency, saved.MaxConcurrency); + Assert.Equal("", saved.Description); + } + + + [Fact] public void Account_AConcurrencyOutsideTheSupportedRange_IsBroughtBackIntoIt_HoweverItIsSet() { Account account = MakeAccount("Bounds") with { MaxConcurrency = 0 }; - Assert.Equal(AppSettings.MinConcurrency, account.MaxConcurrency); + Assert.Equal(AppSettings.DefaultConcurrency, account.MaxConcurrency); Assert.Equal(AppSettings.MaxConcurrency, (account with { MaxConcurrency = int.MaxValue }).MaxConcurrency); + Assert.Equal(AppSettings.MinConcurrency, (account with { MaxConcurrency = AppSettings.MinConcurrency }).MaxConcurrency); Assert.Equal(AppSettings.DefaultConcurrency, MakeAccount("Default").MaxConcurrency); + Assert.Equal("", (account with { Description = null! }).Description); } } diff --git a/gclo.Engine.Tests/ActivityLogTests.cs b/gclo.Engine.Tests/ActivityLogTests.cs index c592039..a0ff765 100644 --- a/gclo.Engine.Tests/ActivityLogTests.cs +++ b/gclo.Engine.Tests/ActivityLogTests.cs @@ -3,7 +3,6 @@ * SPDX-License-Identifier: MIT */ -using System.Diagnostics; using System.Globalization; using System.Text.RegularExpressions; @@ -419,6 +418,11 @@ public sealed class ActivityLogTests : IDisposable // ---------------------------------------------------------------- two writers of one file (#151) + /// A clock that stands still, so a test's file name does not depend on when it runs. + private static SetClock Noon() => new(new DateTimeOffset(2026, 3, 4, 12, 0, 0, TimeSpan.Zero)); + + + /// Holds the file the way another process in the middle of an entry does. private static FileStream HoldAsAnotherWriter(string path) => new(path, FileMode.Append, FileAccess.Write, FileShare.Read); @@ -432,7 +436,7 @@ public sealed class ActivityLogTests : IDisposable { return; // file sharing is what keeps the other writer out } - var log = new FileActivityLog(_root, TimeProvider.System, TimeSpan.FromSeconds(60)); + var log = new FileActivityLog(_root, Noon(), TimeSpan.FromSeconds(60)); log.Info("first"); Task write; @@ -453,37 +457,42 @@ public sealed class ActivityLogTests : IDisposable [Fact] - public void Write_WhenTheFileStaysHeld_GivesUpAfterItsPatience_ThenTriesOnceUntilAWriteGetsThrough() + public void Write_WhenTheFileStaysHeld_GivesUpAfterItsPatience_AndDoesNotWaitAgainBeforeTheCoolDownIsOver() { if(!OperatingSystem.IsWindows()) { return; // file sharing is what keeps the other writer out } - var patience = TimeSpan.FromMilliseconds(600); - var log = new FileActivityLog(_root, TimeProvider.System, patience); + int waits = 0; + var log = new FileActivityLog(_root, Noon(), TimeSpan.FromMilliseconds(300), TimeSpan.FromHours(1)) + { + Pause = pause => + { + waits++; + Thread.Sleep(pause); + }, + }; log.Info("first"); string path = log.CurrentLogFilePath; using(HoldAsAnotherWriter(path)) { - var clock = Stopwatch.StartNew(); log.Info("lost after waiting"); - TimeSpan waited = clock.Elapsed; - clock.Restart(); + int waitsOfTheFirst = waits; log.Info("lost without waiting"); - TimeSpan notWaited = clock.Elapsed; - Assert.True(waited >= patience, $"gave up after {waited}"); - Assert.True(notWaited < patience / 2, $"waited again for {notWaited}"); + Assert.True(waitsOfTheFirst > 0, "the first write did not wait for the file"); + Assert.Equal(waitsOfTheFirst, waits); } - // A write that gets through brings the patience back. + // A write that gets through does not bring the waiting back: a file that is + // held again and again must not stall its writers once per release. log.Info("back"); using(HoldAsAnotherWriter(path)) { - var clock = Stopwatch.StartNew(); - log.Info("lost after waiting again"); - Assert.True(clock.Elapsed >= patience, $"gave up after {clock.Elapsed}"); + int before = waits; + log.Info("lost without waiting, again"); + Assert.Equal(before, waits); } string[] lines = File.ReadAllLines(path); @@ -494,6 +503,39 @@ public sealed class ActivityLogTests : IDisposable + [Fact] + public void Write_OnceTheCoolDownIsOver_WaitsForAHeldFileAgain() + { + if(!OperatingSystem.IsWindows()) + { + return; // file sharing is what keeps the other writer out + } + int waits = 0; + var coolDown = TimeSpan.FromMilliseconds(100); + var log = new FileActivityLog(_root, Noon(), TimeSpan.FromMilliseconds(100), coolDown) + { + Pause = pause => + { + waits++; + Thread.Sleep(pause); + }, + }; + log.Info("first"); + + using(HoldAsAnotherWriter(log.CurrentLogFilePath)) + { + log.Info("lost after waiting"); + int waitsOfTheFirst = waits; + Thread.Sleep(coolDown + TimeSpan.FromMilliseconds(100)); + log.Info("lost after waiting again"); + + Assert.True(waitsOfTheFirst > 0, "the first write did not wait for the file"); + Assert.True(waits > waitsOfTheFirst, "the write after the cool-down did not wait for the file"); + } + } + + + [Fact] public void TwoWritersOfOneFile_LoseNoEntry_AndWriteOnlyWholeLines() { @@ -502,8 +544,8 @@ public sealed class ActivityLogTests : IDisposable return; // file sharing is what keeps the other writer out } // Two logs over one folder stand for the desktop app and the CLI. - var app = new FileActivityLog(_root, TimeProvider.System, TimeSpan.FromSeconds(60)); - var cli = new FileActivityLog(_root, TimeProvider.System, TimeSpan.FromSeconds(60)); + var app = new FileActivityLog(_root, Noon(), TimeSpan.FromSeconds(60)); + var cli = new FileActivityLog(_root, Noon(), TimeSpan.FromSeconds(60)); const int PerWriter = 150; Parallel.Invoke( diff --git a/gclo.Engine.Tests/GatewayCacheTests.cs b/gclo.Engine.Tests/GatewayCacheTests.cs index 9299d38..1dbf237 100644 --- a/gclo.Engine.Tests/GatewayCacheTests.cs +++ b/gclo.Engine.Tests/GatewayCacheTests.cs @@ -142,6 +142,18 @@ public sealed class GatewayCacheTests + [Fact] + public void KeyOf_TwoStringsThatEncodeToTheSameBytes_StillHaveDifferentKeys() + { + // Half of a surrogate pair cannot be encoded; an encoder turns each into the same replacement. + string one = "ghp_" + (char)0xD800 + "abc"; + string other = "ghp_" + (char)0xD801 + "abc"; + + Assert.False(string.Equals(GatewayCache.KeyOf(one), GatewayCache.KeyOf(other), StringComparison.Ordinal)); + } + + + [Fact] public void Constructor_RefusesACapacityBelowOne_AndAMissingFactory() { diff --git a/gclo.Engine.Tests/InvalidRepositoryPathsExceptionTests.cs b/gclo.Engine.Tests/InvalidRepositoryPathsExceptionTests.cs index e3ce475..086f2de 100644 --- a/gclo.Engine.Tests/InvalidRepositoryPathsExceptionTests.cs +++ b/gclo.Engine.Tests/InvalidRepositoryPathsExceptionTests.cs @@ -17,7 +17,7 @@ public sealed class InvalidRepositoryPathsExceptionTests Assert.StartsWith("1 path in this repository cannot be created on Windows:", ex.Message); Assert.Contains("'aux' ('aux' is a reserved Windows device name)", ex.Message); - Assert.EndsWith("No files were written.", ex.Message); + Assert.EndsWith("Nothing was written to the working tree.", ex.Message); } @@ -35,6 +35,6 @@ public sealed class InvalidRepositoryPathsExceptionTests Assert.StartsWith("5 paths in this repository cannot be created on Windows:", ex.Message); Assert.Contains(" and 2 more", ex.Message); // examples cap at three Assert.DoesNotContain("bad:4", ex.Message); - Assert.EndsWith("No files were written.", ex.Message); + Assert.EndsWith("Nothing was written to the working tree.", ex.Message); } } diff --git a/gclo.Engine.Tests/ProcessRunnerTests.cs b/gclo.Engine.Tests/ProcessRunnerTests.cs index 6543aba..25dab09 100644 --- a/gclo.Engine.Tests/ProcessRunnerTests.cs +++ b/gclo.Engine.Tests/ProcessRunnerTests.cs @@ -106,9 +106,15 @@ public sealed class ProcessRunnerTests : IDisposable new Dictionary(StringComparer.Ordinal) { ["GCLO_TEST_ID_FILE"] = idFile }, standardInput: null, cts.Token); - int childId = await ReadChildIdAsync(idFile, run); - - cts.Cancel(); + int childId; + try + { + childId = await ReadChildIdAsync(idFile, run); + } + finally + { + cts.Cancel(); // also when the child never said who it is: it must not outlive the test + } await Assert.ThrowsAnyAsync(() => run.WaitAsync(TimeSpan.FromSeconds(60))); Assert.True(await IsGoneAsync(childId), $"child process {childId} is still running after cancellation"); diff --git a/gclo.Engine/FileActivityLog.cs b/gclo.Engine/FileActivityLog.cs index b67a159..e40aee1 100644 --- a/gclo.Engine/FileActivityLog.cs +++ b/gclo.Engine/FileActivityLog.cs @@ -15,8 +15,9 @@ namespace gclo.Engine; /// An that appends timestamped lines to a daily file, /// gclo-yyyy-MM-dd.log. Writes are serialized by a lock and each one opens, /// appends, and closes the file, so every entry reaches disk even if the process -/// dies right after. The desktop app and the CLI write to the same file: a write -/// that finds the file held by the other waits briefly and tries again. Files +/// dies right after. The desktop app and the CLI can write to the same file: a write +/// that finds the file held by the other waits briefly and tries again, and one that +/// gave up does not make later writes wait until a cool-down is over. Files /// older than days are removed. Logging must never /// take the application down: all IO failures (unwritable directory, locked file, /// full disk) are swallowed. The directory is created lazily on the first write, @@ -40,6 +41,9 @@ public sealed class FileActivityLog : IActivityLog /// How long one write waits for a file another process is writing to. private static readonly TimeSpan s_defaultContentionBudget = TimeSpan.FromMilliseconds(200); + /// How long after a write gave up no write waits for the file again. + private static readonly TimeSpan s_defaultCoolDown = TimeSpan.FromSeconds(30); + private static readonly TimeSpan s_retryPause = TimeSpan.FromMilliseconds(1); private static readonly UTF8Encoding s_utf8 = new(encoderShouldEmitUTF8Identifier: false); @@ -50,15 +54,18 @@ public sealed class FileActivityLog : IActivityLog private readonly TimeSpan _contentionBudget; + private readonly TimeSpan _coolDown; + /// The day old files were last removed for; the work is done once a day. private DateOnly _prunedFor; /// - /// True after a write gave up on a file it could not open. Until a write gets - /// through again, each one tries once and does not wait, so a file that stays - /// locked costs the operations being logged nothing. + /// When a write last gave up on a file it could not open (a stopwatch reading; + /// zero when none has). Until the cool-down after it is over, each write tries + /// once and does not wait: a file that stays locked, or is locked again and again, + /// costs the operations being logged one wait per cool-down, not one per entry. /// - private bool _held; + private long _gaveUpAt; @@ -75,8 +82,8 @@ public sealed class FileActivityLog : IActivityLog - /// Creates a log with its own clock and its own patience, for tests. - internal FileActivityLog(string? directory, TimeProvider time, TimeSpan contentionBudget) + /// Creates a log with its own clock, patience, and cool-down, for tests. + internal FileActivityLog(string? directory, TimeProvider time, TimeSpan contentionBudget, TimeSpan? coolDown = null) { LogDirectory = string.IsNullOrWhiteSpace(directory) ? Path.Combine( @@ -85,10 +92,16 @@ public sealed class FileActivityLog : IActivityLog : directory; _time = time; _contentionBudget = contentionBudget; + _coolDown = coolDown ?? s_defaultCoolDown; } + /// How a write waits between two tries; tests count the waits through it. + internal Action Pause { get; init; } = Thread.Sleep; + + + /// public string LogDirectory { get; } @@ -162,11 +175,13 @@ public sealed class FileActivityLog : IActivityLog /// Appends as one write to a file opened for this writer /// alone, so entries from two processes never interleave. Another process in the /// middle of its own entry holds the file for a moment: the write is tried again - /// until it gets through or its patience runs out. + /// until it gets through or its patience runs out. A write that ran out of + /// patience is the last one to wait until the cool-down is over. /// private void Append(string path, string text) { byte[] bytes = s_utf8.GetBytes(text); + bool patient = _gaveUpAt == 0 || Stopwatch.GetElapsedTime(_gaveUpAt) >= _coolDown; long started = Stopwatch.GetTimestamp(); while(true) { @@ -176,16 +191,15 @@ public sealed class FileActivityLog : IActivityLog { file.Write(bytes); } - _held = false; return; } - catch(IOException) when(!_held && Stopwatch.GetElapsedTime(started) < _contentionBudget) + catch(IOException) when(patient && Stopwatch.GetElapsedTime(started) < _contentionBudget) { - Thread.Sleep(s_retryPause); + Pause(s_retryPause); } - catch(IOException) + catch(IOException) when(patient) { - _held = true; + _gaveUpAt = Stopwatch.GetTimestamp(); throw; } } diff --git a/gclo.Engine/GatewayCache.cs b/gclo.Engine/GatewayCache.cs index db0fdc8..09c78d2 100644 --- a/gclo.Engine/GatewayCache.cs +++ b/gclo.Engine/GatewayCache.cs @@ -3,8 +3,8 @@ * SPDX-License-Identifier: MIT */ +using System.Runtime.InteropServices; using System.Security.Cryptography; -using System.Text; namespace gclo.Engine; @@ -86,10 +86,14 @@ internal sealed class GatewayCache - /// What an entry is filed under: a SHA-256 of the token, never the token. + /// + /// What an entry is filed under: a SHA-256 of the token, never the token. Taken + /// over the string exactly as it is (its UTF-16 code units), so two different + /// strings never share a key by being encoded to the same bytes. + /// internal static string KeyOf(string token) { ArgumentNullException.ThrowIfNull(token); - return Convert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(token))); + return Convert.ToHexString(SHA256.HashData(MemoryMarshal.AsBytes(token.AsSpan()))); } } diff --git a/gclo.Engine/InvalidRepositoryPathsException.cs b/gclo.Engine/InvalidRepositoryPathsException.cs index 6fa1427..b4df226 100644 --- a/gclo.Engine/InvalidRepositoryPathsException.cs +++ b/gclo.Engine/InvalidRepositoryPathsException.cs @@ -33,6 +33,6 @@ public sealed class InvalidRepositoryPathsException : Exception string suffix = paths.Count > 3 ? $" and {paths.Count - 3} more" : ""; string subject = paths.Count == 1 ? "1 path" : $"{paths.Count} paths"; return $"{subject} in this repository cannot be created on Windows: {examples}{suffix}. " + - "No files were written."; + "Nothing was written to the working tree."; } } diff --git a/gclo.Engine/OctokitGatewayCache.cs b/gclo.Engine/OctokitGatewayCache.cs index 4f3a023..6f85d63 100644 --- a/gclo.Engine/OctokitGatewayCache.cs +++ b/gclo.Engine/OctokitGatewayCache.cs @@ -19,7 +19,8 @@ namespace gclo.Engine; [ExcludeFromCodeCoverage(Justification = "Holds live Octokit clients; the offline suite injects fake gateways.")] internal static class OctokitGatewayCache { - private const int MaxEntries = 4; + // One per saved account that has a token of its own, for a handful of accounts. + private const int MaxEntries = 8; private static readonly GatewayCache s_gateways = new(MaxEntries, static token => new OctokitGateway(token)); diff --git a/gclo.Engine/ProcessRunner.cs b/gclo.Engine/ProcessRunner.cs index a9647b7..12bae88 100644 --- a/gclo.Engine/ProcessRunner.cs +++ b/gclo.Engine/ProcessRunner.cs @@ -48,6 +48,10 @@ public sealed class ProcessRunner : IProcessRunner } using var process = new Process { StartInfo = start }; + // Declared before the child is started, so they are released after it has been + // ended: what still copies into them on a failure is not cut off under it. + using var stdout = new MemoryStream(); + using var stderr = new MemoryStream(); process.Start(); /////////////////////////////////////////////////////////////////////////////// @@ -56,8 +60,6 @@ public sealed class ProcessRunner : IProcessRunner /////////////////////////////////////////////////////////////////////////////// try { - using var stdout = new MemoryStream(); - using var stderr = new MemoryStream(); Task copyOut = process.StandardOutput.BaseStream.CopyToAsync(stdout, cancellationToken); Task copyErr = process.StandardError.BaseStream.CopyToAsync(stderr, cancellationToken); try diff --git a/gclo.Engine/SyncSummary.cs b/gclo.Engine/SyncSummary.cs index f842e96..b5101a9 100644 --- a/gclo.Engine/SyncSummary.cs +++ b/gclo.Engine/SyncSummary.cs @@ -13,8 +13,10 @@ namespace gclo.Engine; /// Repositories that failed; a failure never aborts the others. /// Repositories canceled before or during processing. /// -/// Whether the run was cut short by cancellation: true exactly when -/// is above zero. A cancellation that arrives after the -/// last repository finished leaves a complete run. +/// Whether the run was cut short by cancellation. From the engine it is true exactly +/// when is above zero: a cancellation that arrives after +/// the last repository finished leaves a complete run. A caller that does more work +/// around the engine (listing first, recovering paths afterwards) sets it when that +/// work was cut short. /// public sealed record SyncSummary(int Total, int Cloned, int Updated, int Failed, int Canceled, bool WasCanceled); diff --git a/gclo.ViewModels/Account.cs b/gclo.ViewModels/Account.cs index 1c260fa..d0b16ce 100644 --- a/gclo.ViewModels/Account.cs +++ b/gclo.ViewModels/Account.cs @@ -37,8 +37,15 @@ public sealed record Account /// Display name; unique across accounts (case-insensitive). public required string Name { get; init; } - /// Optional free-form note about what the account is for. - public string Description { get; init; } = ""; + /// + /// Optional free-form note about what the account is for. Never null: a file + /// that has no such field is read as an empty note. + /// + public string Description + { + get; + init => field = value ?? ""; + } = ""; /// GitHub organization (or user) whose repositories are synced. public required string Organization { get; init; } @@ -54,14 +61,18 @@ public sealed record Account /// /// Parallel clone/pull count used when syncing this account. Always between - /// and : - /// a value outside that range (an accounts.json edited by hand) is brought back - /// into it when it is read, so every user of the account gets a usable number. + /// and , + /// so every user of the account gets a usable number. A value below the minimum + /// is not a setting anyone chose (an accounts.json edited by hand, or one without + /// the field, which is read as zero): it becomes the default. A value above the + /// maximum becomes the maximum. /// public int MaxConcurrency { get; - init => field = Math.Clamp(value, AppSettings.MinConcurrency, AppSettings.MaxConcurrency); + init => field = value < AppSettings.MinConcurrency + ? AppSettings.DefaultConcurrency + : Math.Min(value, AppSettings.MaxConcurrency); } = AppSettings.DefaultConcurrency; /// When the account's last sync finished, or null if it never ran. -- 2.54.0