From c7880167ab84b11878f48f5938e6777523b7266a Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 10 Nov 2026 15:24:55 -0600 Subject: [PATCH 1/2] fix(accounts): accounts.json is safe across gclo processes; an unreadable file is not a corrupt one The store read accounts.json once and wrote that snapshot back on every change. With the desktop app open, an account added from the CLI was erased by the app's next write (recording a sync result was enough), and its token stayed in the vault with nothing pointing at it. A long `gclo sync --account` did the same to the app, and two app instances to each other. - Every change now takes a lock that every gclo process takes (accounts.json.lock, opened for one process alone), reads the file again, applies its one change by account id to what the file holds at that moment, and writes that. A sync result recorded for an account another process deleted is ignored, not written back. - A file that cannot be read is no longer taken for a corrupt one. Only content that is not a valid list of accounts is moved aside. A file in use is tried again for a short while; one that stays unreadable is left where it is: the store starts empty and logs why, and a change fails without writing anything. - The swap of the new file into place is tried again when something outside gclo holds the live file for a moment. settings.json is written beside the file and moved into place, so a crash mid-write leaves the previous settings. A failed load or save is logged, and Save says whether it succeeded. The "0 means absent" special case for the splash duration described something that does not happen (an absent field keeps the default); the value is simply brought into range. Tests: the three classes that set GCLO_DATA_DIR for the process now share one collection, so they never run beside each other. docs/CLI.md named the wrong folder for accounts.json; corrected. Closes #155 Co-Authored-By: Claude Fable 5.2 Signed-off-by: James Maes --- docs/CLI.md | 15 +- .../AccountsStoreSharedFileTests.cs | 408 ++++++++++++++++++ gclo.Engine.Tests/AppSettingsTests.cs | 85 ++++ gclo.Engine.Tests/CoverageResidueTests.cs | 3 +- gclo.Engine.Tests/GcloPathsTests.cs | 1 + gclo.ViewModels/AccountsStore.cs | 209 ++++++++- gclo.ViewModels/AppSettings.cs | 38 +- gclo/MainWindow.xaml.cs | 2 +- gclo/SettingsDialog.xaml.cs | 2 +- 9 files changed, 722 insertions(+), 41 deletions(-) create mode 100644 gclo.Engine.Tests/AccountsStoreSharedFileTests.cs diff --git a/docs/CLI.md b/docs/CLI.md index 27897fd..bf5daee 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -237,10 +237,17 @@ throttle is reported as such (exit code 4) rather than shrinking the list. An *account* is a saved sync profile: a name, an organization, a target root folder, a parallelism setting, an org-subfolder preference, an optional description, and a token stored in **Windows Credential Manager** (under -`gclo:account:`; the metadata file, `%LOCALAPPDATA%\gclo\accounts.json`, -never contains the token). Accounts are shared between the CLI and the desktop -app: create them with `gclo accounts add` on a headless mirror server, or in -the app's account wizard, and use them from either. +`gclo:account:`; the metadata file, +`%LOCALAPPDATA%\KofTwentyTwo\gclo\accounts.json`, never contains the token). +Accounts are shared between the CLI and the desktop app: create them with +`gclo accounts add` on a headless mirror server, or in the app's account +wizard, and use them from either. + +The CLI and the app can run at the same time, and so can several CLI runs: +each change (adding, editing, or removing an account, recording a sync +result) is applied to what the file holds at that moment, so one never +writes over what another just saved. The app lists an account that was added +from the CLI the next time it starts. > **Windows-only.** Because the token lives in Windows Credential Manager, > `gclo accounts` and `gclo sync --account` exit with code 2 on Linux/macOS. diff --git a/gclo.Engine.Tests/AccountsStoreSharedFileTests.cs b/gclo.Engine.Tests/AccountsStoreSharedFileTests.cs new file mode 100644 index 0000000..2100678 --- /dev/null +++ b/gclo.Engine.Tests/AccountsStoreSharedFileTests.cs @@ -0,0 +1,408 @@ +/* + * Copyright (c) 2026 James Maes (KofTwentyTwo) + * SPDX-License-Identifier: MIT + */ + +using gclo.ViewModels; + + +namespace gclo.Engine.Tests; + + +/// +/// The desktop app and the CLI share accounts.json, and each may run more than once. +/// Two stores over one folder stand for two processes here: a change made through +/// one must survive the next change made through the other, and a file that is in +/// use is waited for, never taken for a corrupt one (#155). +/// +public sealed class AccountsStoreSharedFileTests : IDisposable +{ + private static readonly TimeSpan s_long = TimeSpan.FromSeconds(60); + + private static readonly TimeSpan s_short = TimeSpan.FromMilliseconds(300); + + private readonly string _root = + Path.Combine(Path.GetTempPath(), "gclo-tests", Guid.NewGuid().ToString("N")); + + /// One vault for both stores, as Credential Manager is one for both processes. + private readonly InMemoryVault _vault = new(); + + private readonly RecordingActivityLog _log = new(); + + + + public void Dispose() => GitTestHelpers.TryDeleteDirectory(_root); + + + + private string FilePath => Path.Combine(_root, "accounts.json"); + + + + private string LockPath => Path.Combine(_root, "accounts.json.lock"); + + + + private AccountsStore NewStore(TimeSpan? patience = null) => new(_vault, _root, _log, patience ?? s_long); + + + + private static Account MakeAccount(string name) => new() + { + Id = Guid.NewGuid(), + Name = name, + Organization = "acme", + TargetRoot = @"C:\repos", + }; + + + + private IReadOnlyList NamesOnDisk() + => new AccountsStore(new InMemoryVault(), _root).GetAll().Select(a => a.Name).ToList(); + + + + private IReadOnlyList ErrorsLogged() + => _log.Entries.Where(e => string.Equals(e.Level, "ERROR", StringComparison.Ordinal)).Select(e => e.Message).ToList(); + + + + // ---------------------------------------------------------------- one change at a time, on what the file holds now + + [Fact] + public void AnAccountAddedByTheOtherProcess_SurvivesThisProcessesNextChange() + { + AccountsStore app = NewStore(); + AccountsStore cli = NewStore(); // both started before anything was added + + cli.Save(MakeAccount("From the CLI"), "ghp_cli_token"); + app.Save(MakeAccount("From the app"), null); + + Assert.Equal(["From the app", "From the CLI"], NamesOnDisk(), StringComparer.Ordinal); + // The store that made the later change has seen the earlier one. + Assert.Equal(["From the app", "From the CLI"], app.GetAll().Select(a => a.Name), StringComparer.Ordinal); + } + + + + [Fact] + public void RecordingASyncResult_KeepsWhatTheOtherProcessAddedAndChanged_AndItsTokens() + { + Account work = MakeAccount("Work"); + NewStore().Save(work, "ghp_work_token"); + AccountsStore app = NewStore(); + AccountsStore cli = NewStore(); + Account added = MakeAccount("Added meanwhile"); + + cli.Save(added, "ghp_added_token"); + cli.Save(work with { Description = "edited by the CLI" }, null); + app.RecordSyncResult(work.Id, new DateTimeOffset(2026, 11, 10, 12, 0, 0, TimeSpan.Zero), "42 done"); + + IReadOnlyList onDisk = new AccountsStore(_vault, _root).GetAll(); + Assert.Equal(["Added meanwhile", "Work"], onDisk.Select(a => a.Name), StringComparer.Ordinal); + Account recorded = onDisk.Single(a => a.Id == work.Id); + Assert.Equal("edited by the CLI", recorded.Description); // the other process's change + Assert.Equal("42 done", recorded.LastSyncSummary); // and this one's + Assert.Equal("ghp_added_token", _vault.TryRetrieve(added.Id)); // no token without its account + Assert.Equal("ghp_work_token", _vault.TryRetrieve(work.Id)); + } + + + + [Fact] + public void RecordingASyncResult_ForAnAccountTheOtherProcessDeleted_DoesNotBringItBack() + { + Account work = MakeAccount("Work"); + Account other = MakeAccount("Other"); + AccountsStore setup = NewStore(); + setup.Save(work, "ghp_work_token"); + setup.Save(other, null); + AccountsStore app = NewStore(); + AccountsStore cli = NewStore(); + + cli.Delete(work.Id); + app.RecordSyncResult(work.Id, DateTimeOffset.UtcNow, "done"); + + Assert.Equal(["Other"], NamesOnDisk(), StringComparer.Ordinal); + Assert.Null(_vault.TryRetrieve(work.Id)); + Assert.Contains(ErrorsLogged(), e => e.Contains("no account with id", StringComparison.Ordinal)); + Assert.Equal(["Other"], app.GetAll().Select(a => a.Name), StringComparer.Ordinal); + } + + + + [Fact] + public void Save_UnderANameTheOtherProcessTookMeanwhile_IsRefused_AndWritesNothing() + { + AccountsStore app = NewStore(); + AccountsStore cli = NewStore(); + cli.Save(MakeAccount("Work"), null); + Account clash = MakeAccount("work"); + + ArgumentException ex = Assert.Throws(() => app.Save(clash, "ghp_clash_token")); + + Assert.Contains("already exists", ex.Message); + Assert.Equal(["Work"], NamesOnDisk(), StringComparer.Ordinal); + Assert.Null(_vault.TryRetrieve(clash.Id)); + } + + + + [Fact] + public void Delete_OfAnAccountTheOtherProcessAlreadyDeleted_ChangesNothing_AndKeepsTheRest() + { + Account work = MakeAccount("Work"); + AccountsStore setup = NewStore(); + setup.Save(work, null); + AccountsStore app = NewStore(); + AccountsStore cli = NewStore(); + + cli.Delete(work.Id); + cli.Save(MakeAccount("Added meanwhile"), null); + app.Delete(work.Id); + + Assert.Equal(["Added meanwhile"], NamesOnDisk(), StringComparer.Ordinal); + } + + + + [Fact] + public void ManyChangesThroughTwoStoresAtOnce_AreAllKept() + { + AccountsStore app = NewStore(); + AccountsStore cli = NewStore(); + const int PerStore = 25; + + Parallel.Invoke( + () => + { + for(int i = 0; i < PerStore; i++) + { + app.Save(MakeAccount($"app-{i:D2}"), null); + } + }, + () => + { + for(int i = 0; i < PerStore; i++) + { + cli.Save(MakeAccount($"cli-{i:D2}"), null); + } + }); + + Assert.Equal(2 * PerStore, NamesOnDisk().Count); + } + + + + // ---------------------------------------------------------------- the lock + + [Fact] + public async Task AChange_WhileTheOtherProcessHoldsTheLock_WaitsForIt() + { + AccountsStore store = NewStore(); + Directory.CreateDirectory(_root); + Task save; + + using(new FileStream(LockPath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None)) + { + save = Task.Run(() => store.Save(MakeAccount("Waited"), null)); + Task finished = await Task.WhenAny(save, Task.Delay(TimeSpan.FromMilliseconds(200))); + Assert.NotSame(save, finished); // still waiting for the other process + Assert.False(File.Exists(FilePath)); + } + await save.WaitAsync(s_long); + + Assert.Equal(["Waited"], NamesOnDisk(), StringComparer.Ordinal); + } + + + + [Fact] + public void AChange_WhenTheLockStaysHeld_Fails_AndChangesNothing() + { + Account existing = MakeAccount("Existing"); + NewStore().Save(existing, "ghp_existing_token"); + AccountsStore store = NewStore(s_short); + string before = File.ReadAllText(FilePath); + Account added = MakeAccount("Never saved"); + + using(new FileStream(LockPath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None)) + { + IOException save = Assert.Throws(() => store.Save(added, "ghp_never_stored")); + Assert.Contains("in use by another gclo process", save.Message); + Assert.Throws(() => store.Delete(existing.Id)); + Assert.Throws(() => store.RecordSyncResult(existing.Id, DateTimeOffset.UtcNow, "done")); + } + + Assert.Equal(before, File.ReadAllText(FilePath)); + Assert.Null(_vault.TryRetrieve(added.Id)); + Assert.Equal("ghp_existing_token", _vault.TryRetrieve(existing.Id)); + Assert.Equal(["Existing"], store.GetAll().Select(a => a.Name), StringComparer.Ordinal); + } + + + + [Fact] + public void Construction_WhenTheLockStaysHeld_StartsEmpty_SaysWhy_AndLeavesTheFile() + { + NewStore().Save(MakeAccount("Existing"), null); + string before = File.ReadAllText(FilePath); + + AccountsStore store; + using(new FileStream(LockPath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None)) + { + store = NewStore(s_short); + } + + Assert.Empty(store.GetAll()); + Assert.Contains(ErrorsLogged(), e => e.Contains("Could not read accounts", StringComparison.Ordinal) + && e.Contains("left as it is", StringComparison.Ordinal)); + Assert.Equal(before, File.ReadAllText(FilePath)); + Assert.Empty(Directory.GetFiles(_root, "accounts.json.corrupt-*")); + + // Its next change reads the file again, so what it could not show is not lost. + store.Save(MakeAccount("Added later"), null); + Assert.Equal(["Added later", "Existing"], NamesOnDisk(), StringComparer.Ordinal); + } + + + + [Fact] + public void TheLockFile_StaysBesideTheAccounts_Empty_AndIsNotCreatedByLookingAtNothing() + { + _ = NewStore().GetAll(); + Assert.False(Directory.Exists(_root), "looking at a folder with no accounts creates nothing"); + + NewStore().Save(MakeAccount("First"), null); + + Assert.True(File.Exists(LockPath)); + Assert.Equal(0, new FileInfo(LockPath).Length); + } + + + + // ---------------------------------------------------------------- a file that is in use is not a corrupt file + + [Fact] + public async Task Construction_WhenTheFileIsInUseForAMoment_TriesAgain() + { + if(!OperatingSystem.IsWindows()) + { + return; // file sharing is what keeps a reader out + } + NewStore().Save(MakeAccount("Existing"), null); + Task construction; + + using(new FileStream(FilePath, FileMode.Open, FileAccess.ReadWrite, FileShare.None)) + { + construction = Task.Run(() => NewStore()); + Task finished = await Task.WhenAny(construction, Task.Delay(TimeSpan.FromMilliseconds(200))); + Assert.NotSame(construction, finished); // still waiting for the file + } + AccountsStore store = await construction.WaitAsync(s_long); + + Assert.Equal(["Existing"], store.GetAll().Select(a => a.Name), StringComparer.Ordinal); + Assert.Empty(ErrorsLogged()); + } + + + + [Fact] + public void WhenTheFileStaysUnreadable_ItIsNotMovedAside_AndAChangeFailsWithoutWriting() + { + if(!OperatingSystem.IsWindows()) + { + return; // file sharing is what keeps a reader out + } + NewStore().Save(MakeAccount("Existing"), null); + string before = File.ReadAllText(FilePath); + Account added = MakeAccount("Never saved"); + + AccountsStore store; + using(new FileStream(FilePath, FileMode.Open, FileAccess.ReadWrite, FileShare.None)) + { + store = NewStore(s_short); + Assert.Empty(store.GetAll()); + Assert.Throws(() => store.Save(added, "ghp_never_stored")); + } + + Assert.Equal(before, File.ReadAllText(FilePath)); + Assert.Empty(Directory.GetFiles(_root, "accounts.json.corrupt-*")); + Assert.Null(_vault.TryRetrieve(added.Id)); + Assert.Contains(ErrorsLogged(), e => e.Contains("Could not read accounts", StringComparison.Ordinal)); + Assert.Equal(["Existing"], NewStore().GetAll().Select(a => a.Name), StringComparer.Ordinal); + } + + + + [Fact] + public void ContentThatIsNotAListOfAccounts_IsStillMovedAside_AlsoWhenAChangeFindsIt() + { + AccountsStore store = NewStore(); + store.Save(MakeAccount("Existing"), null); + File.WriteAllText(FilePath, "{ not json"); // damaged after the store read it + + store.Save(MakeAccount("Fresh"), null); + + string aside = Assert.Single(Directory.GetFiles(_root, "accounts.json.corrupt-*")); + Assert.Equal("{ not json", File.ReadAllText(aside)); + Assert.Equal(["Fresh"], NamesOnDisk(), StringComparer.Ordinal); + Assert.Contains(ErrorsLogged(), e => e.Contains("not a valid list of accounts", StringComparison.Ordinal)); + } + + + + // ---------------------------------------------------------------- the swap, when something else holds the live file + + /// Holds the file the way a scanner or a viewer does: it can be read, not replaced. + private FileStream HoldLiveFileAgainstReplacement() + => new(FilePath, FileMode.Open, FileAccess.Read, FileShare.Read); + + + + [Fact] + public async Task AChange_WhenTheLiveFileIsHeldForAMoment_TriesTheSwapAgain() + { + if(!OperatingSystem.IsWindows()) + { + return; // an open file can be replaced elsewhere + } + AccountsStore store = NewStore(); + store.Save(MakeAccount("Existing"), null); + Task save; + + using(HoldLiveFileAgainstReplacement()) + { + save = Task.Run(() => store.Save(MakeAccount("Added"), null)); + Task finished = await Task.WhenAny(save, Task.Delay(TimeSpan.FromMilliseconds(200))); + Assert.NotSame(save, finished); // still trying the swap + } + await save.WaitAsync(s_long); + + Assert.Equal(["Added", "Existing"], NamesOnDisk(), StringComparer.Ordinal); + } + + + + [Fact] + public void AChange_WhenTheLiveFileStaysHeld_Fails_AndTheStoreStillSaysWhatIsOnDisk() + { + if(!OperatingSystem.IsWindows()) + { + return; // an open file can be replaced elsewhere + } + AccountsStore store = NewStore(s_short); + store.Save(MakeAccount("Existing"), null); + Account added = MakeAccount("Never saved"); + + using(HoldLiveFileAgainstReplacement()) + { + Exception ex = Assert.ThrowsAny(() => store.Save(added, "ghp_never_kept")); + Assert.True(ex is IOException or UnauthorizedAccessException, ex.GetType().Name); + } + + Assert.Equal(["Existing"], NamesOnDisk(), StringComparer.Ordinal); + Assert.Equal(["Existing"], store.GetAll().Select(a => a.Name), StringComparer.Ordinal); + Assert.Null(_vault.TryRetrieve(added.Id)); // the vault write was undone + } +} diff --git a/gclo.Engine.Tests/AppSettingsTests.cs b/gclo.Engine.Tests/AppSettingsTests.cs index db944c9..3945745 100644 --- a/gclo.Engine.Tests/AppSettingsTests.cs +++ b/gclo.Engine.Tests/AppSettingsTests.cs @@ -10,6 +10,7 @@ namespace gclo.Engine.Tests; /// Pins defaults and load-time sanitizing of . +[Collection("GCLO_DATA_DIR")] // sets a process-wide variable: never beside another class that does public sealed class AppSettingsTests { [Fact] @@ -90,4 +91,88 @@ public sealed class AppSettingsTests } } } + + + + // ---------------------------------------------------------------- written whole; failures are said (#155) + + private static void WithDataDirectory(Action test) + { + string dir = Path.Combine(Path.GetTempPath(), "gclo-tests", Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(dir); + Environment.SetEnvironmentVariable("GCLO_DATA_DIR", dir); + try + { + test(dir); + } + finally + { + Environment.SetEnvironmentVariable("GCLO_DATA_DIR", null); + GitTestHelpers.TryDeleteDirectory(dir); + } + } + + + + [Fact] + public void Save_SaysItSucceeded_AndLeavesOnlyTheSettingsFile() + => WithDataDirectory(dir => + { + var log = new RecordingActivityLog(); + + bool saved = new AppSettings { Theme = "Dark" }.Save(log); + + Assert.True(saved); + Assert.Equal(["settings.json"], Directory.GetFiles(dir).Select(Path.GetFileName), StringComparer.Ordinal); + Assert.Equal("Dark", AppSettings.Load(log).Theme); + Assert.Empty(log.Entries); + }); + + + + [Fact] + public void Save_WhenItCannotWrite_SaysSo_AndThePreviousSettingsAreStillThere() + => WithDataDirectory(dir => + { + var log = new RecordingActivityLog(); + Assert.True(new AppSettings { Theme = "Dark" }.Save()); + // The new content is written beside the file first; a folder of that name stops it there. + Directory.CreateDirectory(Path.Combine(dir, "settings.json.tmp")); + + bool saved = new AppSettings { Theme = "Light" }.Save(log); + + Assert.False(saved); + (string level, string message) = Assert.Single(log.Entries); + Assert.Equal("ERROR", level); + Assert.StartsWith("Settings could not be saved (", message); + Assert.Equal("Dark", AppSettings.Load().Theme); // not half a file, not the new one: the old one + }); + + + + [Fact] + public void Load_AFileThatCannotBeUsed_GivesTheDefaults_AndSaysSo() + => WithDataDirectory(dir => + { + var log = new RecordingActivityLog(); + File.WriteAllText(Path.Combine(dir, "settings.json"), "{ not json"); + + var loaded = AppSettings.Load(log); + + Assert.Equal("System", loaded.Theme); + (string level, string message) = Assert.Single(log.Entries); + Assert.Equal("ERROR", level); + Assert.StartsWith("Settings could not be loaded (", message); + }); + + + + [Fact] + public void Load_ASplashDurationOfZero_IsBroughtIntoRange_LikeAnyOtherValue() + => WithDataDirectory(dir => + { + File.WriteAllText(Path.Combine(dir, "settings.json"), "{ \"SplashMilliseconds\": 0 }"); + + Assert.Equal(AppSettings.MinSplashMilliseconds, AppSettings.Load().SplashMilliseconds); + }); } diff --git a/gclo.Engine.Tests/CoverageResidueTests.cs b/gclo.Engine.Tests/CoverageResidueTests.cs index 2d9cc7a..65fd395 100644 --- a/gclo.Engine.Tests/CoverageResidueTests.cs +++ b/gclo.Engine.Tests/CoverageResidueTests.cs @@ -14,6 +14,7 @@ namespace gclo.Engine.Tests; /// branches, settings failure fallbacks, store corruption handling, and the /// remaining status-text arms. Part of the 100%-coverage contract. /// +[Collection("GCLO_DATA_DIR")] // sets a process-wide variable: never beside another class that does public sealed class CoverageResidueTests { // ------------------------------------------------- WindowsPathValidator @@ -220,7 +221,7 @@ public sealed class CoverageResidueTests Assert.Empty(store.GetAll()); } - Assert.Contains(log.Errors, e => e.Contains("Failed to load accounts")); + Assert.Contains(log.Errors, e => e.Contains("not a valid list of accounts")); Assert.Contains(log.Errors, e => e.Contains("Could not preserve")); } finally diff --git a/gclo.Engine.Tests/GcloPathsTests.cs b/gclo.Engine.Tests/GcloPathsTests.cs index b87cd3f..1bf500f 100644 --- a/gclo.Engine.Tests/GcloPathsTests.cs +++ b/gclo.Engine.Tests/GcloPathsTests.cs @@ -15,6 +15,7 @@ namespace gclo.Engine.Tests; /// first and deletes that folder, which destroyed accounts.json and settings.json /// in 1.0.2-beta.1 (#91). /// +[Collection("GCLO_DATA_DIR")] // sets a process-wide variable: never beside another class that does public sealed class GcloPathsTests { /// The default root is %LOCALAPPDATA%\KofTwentyTwo\gclo. diff --git a/gclo.ViewModels/AccountsStore.cs b/gclo.ViewModels/AccountsStore.cs index 85f3f77..0c42b13 100644 --- a/gclo.ViewModels/AccountsStore.cs +++ b/gclo.ViewModels/AccountsStore.cs @@ -3,6 +3,7 @@ * SPDX-License-Identifier: MIT */ +using System.Diagnostics; using System.Text.Json; using gclo.Engine; @@ -17,7 +18,7 @@ namespace gclo.ViewModels; /// token. Unlike , accounts are primary user data, so write /// failures in , , and /// propagate instead of being swallowed; only loading -/// is tolerant (a missing or corrupt file yields an empty list). +/// is tolerant (a missing, corrupt, or unreadable file yields an empty list). /// /// /// @@ -44,6 +45,21 @@ namespace gclo.ViewModels; /// /// /// In-memory state changes only after both stores succeeded. Nothing here logs a token. +/// +/// The desktop app and the CLI share the file, and each may run more than once. Every +/// change therefore takes a lock that every gclo process takes (accounts.json.lock, +/// opened for this process alone), reads the file again, applies its one change by +/// to what the file holds now, and writes that: an account +/// another process added, changed, or removed in the meantime stays as that process +/// left it. Reads (, ) answer from what +/// this store last read, at construction or at its latest change. +/// +/// +/// A file that cannot be read is not a corrupt file. Only content that is not valid +/// JSON is moved aside. A file that is in use is tried again for a short while; one +/// that stays unreadable is left exactly where it is: at construction the store then +/// starts empty (and logs why), and a change fails without writing anything. +/// /// public sealed class AccountsStore { @@ -53,8 +69,15 @@ public sealed class AccountsStore private readonly string _filePath; + /// How long a busy file (the lock, or accounts.json itself) is waited for. + private static readonly TimeSpan s_defaultPatience = TimeSpan.FromSeconds(5); + + private static readonly TimeSpan s_pause = TimeSpan.FromMilliseconds(15); + private readonly Lock _gate = new(); + private readonly TimeSpan _patience; + private List _accounts; @@ -66,13 +89,22 @@ public sealed class AccountsStore /// surface that) and treated as empty rather than blocking startup. /// public AccountsStore(ITokenVault vault, string? directory = null, IActivityLog? log = null) + : this(vault, directory, log, s_defaultPatience) + { + } + + + + /// Creates a store with its own patience for a busy file, for tests. + internal AccountsStore(ITokenVault vault, string? directory, IActivityLog? log, TimeSpan patience) { ArgumentNullException.ThrowIfNull(vault); _vault = vault; _log = log ?? new NullActivityLog(); directory ??= GcloPaths.DataRoot; _filePath = Path.Combine(directory, "accounts.json"); - _accounts = Load(); + _patience = patience; + _accounts = LoadAtStart(); } @@ -102,7 +134,7 @@ public sealed class AccountsStore /// /// Inserts or updates (matching by ) and persists - /// immediately. Throws when another account + /// immediately, on top of what the file holds at that moment. Throws when another account /// already uses the name (case-insensitive); nothing is persisted in that case. /// A non-null is written to the vault BEFORE the /// metadata; if the metadata write then fails, the vault is put back the way it @@ -115,6 +147,14 @@ public sealed class AccountsStore ArgumentNullException.ThrowIfNull(account); lock(_gate) { + /////////////////////////////////////////////////////////////////////////// + // Another gclo process may have changed the file since it was last read. // + // The change is applied to what the file holds now, under the lock every // + // gclo process takes, so nothing another process wrote is written over. // + /////////////////////////////////////////////////////////////////////////// + using FileStream _ = AcquireFileLock(); + _accounts = ReadFile(); + bool nameTaken = _accounts.Any(a => a.Id != account.Id && string.Equals(a.Name, account.Name, StringComparison.OrdinalIgnoreCase)); @@ -269,6 +309,14 @@ public sealed class AccountsStore { lock(_gate) { + /////////////////////////////////////////////////////////////////////////// + // Another gclo process may have changed the file since it was last read. // + // The change is applied to what the file holds now, under the lock every // + // gclo process takes, so nothing another process wrote is written over. // + /////////////////////////////////////////////////////////////////////////// + using FileStream _ = AcquireFileLock(); + _accounts = ReadFile(); + var remaining = _accounts.Where(a => a.Id != id).ToList(); if(remaining.Count == _accounts.Count) { @@ -319,13 +367,22 @@ public sealed class AccountsStore /// /// Stamps the outcome of a finished sync onto the account and persists it; every - /// other field is left untouched. An unknown id (account deleted while its sync - /// ran) is logged and ignored. + /// other field is left untouched, including what another process changed on the + /// account while its sync ran. An unknown id (account deleted while its sync ran, + /// here or by another process) is logged and ignored: it is not brought back. /// public void RecordSyncResult(Guid id, DateTimeOffset lastSyncUtc, string summary) { lock(_gate) { + /////////////////////////////////////////////////////////////////////////// + // Another gclo process may have changed the file since it was last read. // + // The change is applied to what the file holds now, under the lock every // + // gclo process takes, so nothing another process wrote is written over. // + /////////////////////////////////////////////////////////////////////////// + using FileStream _ = AcquireFileLock(); + _accounts = ReadFile(); + int index = _accounts.FindIndex(a => a.Id == id); if(index < 0) { @@ -345,35 +402,136 @@ public sealed class AccountsStore - private List Load() + /// + /// What the file holds when the store is created. Never throws: a file that + /// cannot be read leaves the store empty, says why in the log, and is left where + /// it is (the next change reads it again and fails if it still cannot). + /// + private List LoadAtStart() { try { if(!File.Exists(_filePath)) { - return []; + return []; // no accounts yet, and looking creates nothing } - List? loaded = JsonSerializer.Deserialize( - File.ReadAllText(_filePath), AccountsJsonContext.Default.ListAccount); + using FileStream _ = AcquireFileLock(); + return ReadFile(); + } + catch(Exception ex) when(ex is IOException or UnauthorizedAccessException) + { + _log.Error( + $"Could not read accounts from '{_filePath}' ({ex.Message}); no accounts are shown. " + + "The file was left as it is.", + ex); + return []; + } + } + + + + /// + /// The accounts the file holds now. Called with the file lock held. A missing file + /// is no accounts; content that is not a list of accounts is moved aside and is no + /// accounts either. A file that cannot be read at all throws, and is not touched. + /// + private List ReadFile() + { + string json; + try + { + json = WithPatience(() => File.ReadAllText(_filePath), accessDeniedIsBusy: false); + } + catch(Exception ex) when(ex is FileNotFoundException or DirectoryNotFoundException) + { + return []; + } + + try + { + List? loaded = JsonSerializer.Deserialize(json, AccountsJsonContext.Default.ListAccount); if(loaded is not null) { return loaded; } _log.Error($"Accounts file '{_filePath}' deserialized to null; starting with no accounts."); - PreserveCorruptFile(); } catch(Exception ex) { - _log.Error($"Failed to load accounts from '{_filePath}'; starting with no accounts.", ex); - PreserveCorruptFile(); + /////////////////////////////////////////////////////////////////////////// + // The text was read; it is what it says that cannot be used. That, and // + // only that, is a corrupt file. // + /////////////////////////////////////////////////////////////////////////// + _log.Error($"Accounts file '{_filePath}' is not a valid list of accounts; starting with no accounts.", ex); } + PreserveCorruptFile(); return []; } + /// + /// Takes the lock that every gclo process takes before it reads or changes the + /// accounts file, waiting a short while for another process to finish. The lock is + /// a file beside accounts.json, opened for this process alone and held until it is + /// disposed; the file itself stays, empty. + /// + private FileStream AcquireFileLock() + { + string? directory = Path.GetDirectoryName(_filePath); + if(!string.IsNullOrEmpty(directory)) + { + Directory.CreateDirectory(directory); + } + + try + { + return WithPatience( + () => new FileStream(_filePath + ".lock", FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None), + accessDeniedIsBusy: false); + } + catch(IOException ex) when(IsBusy(ex, accessDeniedIsBusy: false)) + { + throw new IOException( + $"The accounts file '{_filePath}' is in use by another gclo process; nothing was changed. Try again in a moment.", + ex); + } + } + + + + /// Runs , trying again for a short while as long as the file is busy. + private T WithPatience(Func action, bool accessDeniedIsBusy) + { + long started = Stopwatch.GetTimestamp(); + while(true) + { + try + { + return action(); + } + catch(Exception ex) when(IsBusy(ex, accessDeniedIsBusy) && Stopwatch.GetElapsedTime(started) < _patience) + { + Thread.Sleep(s_pause); + } + } + } + + + + /// + /// True for a failure that means "in use right now": a sharing violation, and, where + /// the operation reports a busy target that way, a refused access. A missing file + /// or folder is never that. + /// + private static bool IsBusy(Exception exception, bool accessDeniedIsBusy) + => exception is IOException and not (FileNotFoundException or DirectoryNotFoundException or PathTooLongException) + || (accessDeniedIsBusy && exception is UnauthorizedAccessException); + + + /// /// Moves an unreadable accounts file aside before the empty in-memory list can be /// persisted over it: the metadata is what associates vault entries with accounts, @@ -419,13 +577,24 @@ public sealed class AccountsStore stream.Flush(flushToDisk: true); } - if(File.Exists(_filePath)) - { - File.Replace(tempPath, _filePath, _filePath + ".bak"); - } - else - { - File.Move(tempPath, _filePath); - } + /////////////////////////////////////////////////////////////////////////////// + // Something outside gclo (a scanner, a backup, an editor) can hold the live // + // file for a moment; the swap is then refused. It is tried again for a short // + // while before the change is given up. // + /////////////////////////////////////////////////////////////////////////////// + WithPatience( + () => + { + if(File.Exists(_filePath)) + { + File.Replace(tempPath, _filePath, _filePath + ".bak"); + } + else + { + File.Move(tempPath, _filePath); + } + return true; + }, + accessDeniedIsBusy: true); } } diff --git a/gclo.ViewModels/AppSettings.cs b/gclo.ViewModels/AppSettings.cs index 5c96ee4..e102c29 100644 --- a/gclo.ViewModels/AppSettings.cs +++ b/gclo.ViewModels/AppSettings.cs @@ -4,6 +4,7 @@ */ using System.Text.Json; +using gclo.Engine; namespace gclo.ViewModels; @@ -60,8 +61,9 @@ public sealed class AppSettings /// /// Loads settings from disk. Never throws: a missing, corrupt, or unreadable file /// yields defaults, and loaded values are sanitized (concurrency clamped, theme validated). + /// A file that is there but cannot be used is reported to . /// - public static AppSettings Load() + public static AppSettings Load(IActivityLog? log = null) { try { @@ -77,9 +79,10 @@ public sealed class AppSettings } } } - catch + catch(Exception ex) { // Settings must never prevent startup; fall through to defaults. + log?.Error($"Settings could not be loaded ({ex.Message}); the defaults are used.", ex); } return new AppSettings(); @@ -88,11 +91,15 @@ public sealed class AppSettings /// - /// Persists settings to disk, creating the directory if needed. Failures (locked file, - /// read-only profile, full disk, ...) are swallowed: losing a preference write must - /// never crash the app. + /// Persists settings to disk, creating the directory if needed. The new content is + /// written beside the file and then put in its place, so a crash or a full disk in + /// the middle leaves the previous settings, never half a file. Failures (locked + /// file, read-only profile, full disk, ...) do not throw, because losing a + /// preference write must never crash the app; they are reported to + /// and in the result. /// - public void Save() + /// True when the settings are on disk. + public bool Save(IActivityLog? log = null) { try { @@ -103,11 +110,16 @@ public sealed class AppSettings { Directory.CreateDirectory(directory); } - File.WriteAllText(path, JsonSerializer.Serialize(this, AppSettingsJsonContext.Default.AppSettings)); + + string temporary = path + ".tmp"; + File.WriteAllText(temporary, JsonSerializer.Serialize(this, AppSettingsJsonContext.Default.AppSettings)); + File.Move(temporary, path, overwrite: true); + return true; } - catch + catch(Exception ex) { - // Best effort only. + log?.Error($"Settings could not be saved ({ex.Message}); the change is lost when the app closes.", ex); + return false; } } @@ -118,11 +130,9 @@ public sealed class AppSettings // The JSON deserializer can assign null despite the non-nullable annotation. DefaultTargetFolder ??= ""; DefaultMaxConcurrency = Math.Clamp(DefaultMaxConcurrency, MinConcurrency, MaxConcurrency); - // 0 means the field was absent (settings.json written by an older version): - // migrate to the default rather than clamping an unset value to the minimum. - SplashMilliseconds = SplashMilliseconds == 0 - ? DefaultSplashMilliseconds - : Math.Clamp(SplashMilliseconds, MinSplashMilliseconds, MaxSplashMilliseconds); + // A file written before this setting existed has no such field, and the + // property then keeps its default; whatever a file does say is brought into range. + SplashMilliseconds = Math.Clamp(SplashMilliseconds, MinSplashMilliseconds, MaxSplashMilliseconds); if(Theme is not ("System" or "Light" or "Dark")) { Theme = "System"; diff --git a/gclo/MainWindow.xaml.cs b/gclo/MainWindow.xaml.cs index f487c77..21d58ef 100644 --- a/gclo/MainWindow.xaml.cs +++ b/gclo/MainWindow.xaml.cs @@ -142,7 +142,7 @@ public sealed partial class MainWindow : Window } UpdateSyncAllEnabled(); - _settings = AppSettings.Load(); + _settings = AppSettings.Load(_log); // Create and show Quick Sync before selecting it, so NavigationView.Content // is never empty regardless of when SelectionChanged fires. diff --git a/gclo/SettingsDialog.xaml.cs b/gclo/SettingsDialog.xaml.cs index b8af637..adee0ea 100644 --- a/gclo/SettingsDialog.xaml.cs +++ b/gclo/SettingsDialog.xaml.cs @@ -154,7 +154,7 @@ public sealed partial class SettingsDialog : ContentDialog } } - _settings.Save(); + _settings.Save(_log); string typed = DefaultTokenBox.Password; switch(_tokenState.Plan(typed)) -- 2.54.0 From 17955a3c7bd553fd8f6517e86779d1334b154d56 Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 10 Nov 2026 16:06:03 -0600 Subject: [PATCH 2/2] fix(accounts): address the independent review of the shared accounts file One rename puts the new file in place, so it is never absent; start-up reads without the lock. An edit of an account another process removed is refused; a newer recorded sync result is kept; a change that finds the file gone or damaged is applied to what the store last read. Busy means a sharing violation; a read-only file fails at once; a lock held for the whole wait is an AccountsFileBusyException (CLI exit 4). Settings saves retry briefly, use a temporary name of their own and leave nothing behind. Refs #155 Co-Authored-By: Claude Fable 5.2 Signed-off-by: James Maes --- docs/CLI.md | 12 +- docs/security/threat-model.md | 2 +- gclo.Cli/Program.cs | 6 + .../AccountsStoreSharedFileTests.cs | 231 ++++++++++++++++-- gclo.Engine.Tests/AppSettingsTests.cs | 61 ++++- gclo.ViewModels/AccountsFileBusyException.cs | 22 ++ gclo.ViewModels/AccountsStore.cs | 206 ++++++++++++---- gclo.ViewModels/AppSettings.cs | 58 ++++- gclo/SettingsDialog.xaml.cs | 7 +- 9 files changed, 522 insertions(+), 83 deletions(-) create mode 100644 gclo.ViewModels/AccountsFileBusyException.cs diff --git a/docs/CLI.md b/docs/CLI.md index bf5daee..400b9f1 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -245,9 +245,13 @@ wizard, and use them from either. The CLI and the app can run at the same time, and so can several CLI runs: each change (adding, editing, or removing an account, recording a sync -result) is applied to what the file holds at that moment, so one never -writes over what another just saved. The app lists an account that was added -from the CLI the next time it starts. +result) is applied to what the file holds at that moment. An account one of +them added or removed stays added or removed, and a recorded sync result is +kept; when two of them edit the same account, the later edit wins, and an +edit of an account that was removed in the meantime is refused. The app lists +an account that was added from the CLI the next time it starts. If another +gclo process is changing the accounts at that very moment, a command waits a +few seconds and then exits with code 4; run it again. > **Windows-only.** Because the token lives in Windows Credential Manager, > `gclo accounts` and `gclo sync --account` exit with code 2 on Linux/macOS. @@ -411,7 +415,7 @@ its offset from UTC (`2026-11-10 14:03:22.123 -06:00 [INFO] ...`). Text that cam | 1 | The run completed, but some repositories failed or the run was canceled (Ctrl+C). `--json`'s `wasCanceled` tells the two apart. | Inspect the failures; retry them. | | 2 | Fatal: bad arguments, missing/empty token, organization not found, unknown account, missing account token, unusable target folder, or an account command on a non-Windows OS. | Fix the invocation. | | 3 | GitHub rejected the token (401), or refused it access (403: the token may not see the organization, or the organization requires SAML single sign-on and the token is not authorized for it). The message carries GitHub's reason when it gave one. | Fix the token or its access; do not retry as is. | -| 4 | Not your doing: GitHub's primary or secondary rate limit (403 or 429), a temporary throttle, a network failure (DNS, connection, TLS, proxy), a request that timed out, or an error on GitHub's side (HTTP 5xx). | Retry later (for a rate limit, the message says when). | +| 4 | Not your doing: GitHub's primary or secondary rate limit (403 or 429), a temporary throttle, a network failure (DNS, connection, TLS, proxy), a request that timed out, an error on GitHub's side (HTTP 5xx), or the accounts file being changed by another gclo process at that moment. | Retry later (for a rate limit, the message says when). | | 70 | An unexpected error inside gclo. The exception type is printed and the full details are in the activity log. | Please report it. | Codes 3 and 4 were part of code 2 before version 1.1; a script that tested diff --git a/docs/security/threat-model.md b/docs/security/threat-model.md index ee70fbf..4400533 100644 --- a/docs/security/threat-model.md +++ b/docs/security/threat-model.md @@ -61,7 +61,7 @@ flowchart LR | Git HTTPS transport (libgit2) | out/in | Repository objects and trees | PAT as HTTPS basic (`x-access-token`) | `WindowsPathValidator` checks every incoming path against Windows rules before checkout; non-fast-forward is reported, never merged | | PAT entry | in | The token | n/a | App: in-memory or Credential Manager; CLI: env var, file, or stdin only (no `--token `) | | Credential Manager | out/in | Tokens by `gclo[:]:account:` target | Windows user session | `CredentialManagerVault`; scope derived from the data root | -| `settings.json`, `accounts.json` | out/in | Non-secret settings and account metadata | File ACL (user profile) | Atomic writes, sanitized on load, corrupt files preserved as `.bak` | +| `settings.json`, `accounts.json` (with `accounts.json.lock`) | out/in | Non-secret settings and account metadata | File ACL (user profile) | Written beside the file and put in place in one step; sanitized on load; an accounts file whose content is not valid is moved aside (`.corrupt-*`), one that cannot be read is left in place; the previous accounts file is kept as `.bak`. Changes to the accounts are serialized across gclo processes by the lock file: a process of the same user that holds it open can delay account changes (they fail after a few seconds, nothing is written), not read or alter them | | Activity logs | out | Actions and errors; token *lengths* only | File ACL | `IActivityLog` callers never pass tokens | | `.git/gclo-recovery.json`, `gclo.checkoutpending` config key | out/in | Path-recovery plan; checkout-pending marker | File ACL | Written atomically; read only for the repository it belongs to | | Command line (CLI) | in | Options, org, target, filters | n/a | `OptionReader` / `RepoFilterSpec`; parallelism bounded 1..64; token options refused on argv | diff --git a/gclo.Cli/Program.cs b/gclo.Cli/Program.cs index a1651b3..1c043b8 100644 --- a/gclo.Cli/Program.cs +++ b/gclo.Cli/Program.cs @@ -103,6 +103,12 @@ internal static class Program Terminal.Error("Canceled."); return ExitCodes.Partial; } + catch(gclo.ViewModels.AccountsFileBusyException ex) + { + // Another gclo process is changing the accounts: nothing is wrong, try again. + Terminal.Error(ex.Message); + return ExitCodes.Transient; + } catch(Exception ex) { return ReportUnexpected(ex); diff --git a/gclo.Engine.Tests/AccountsStoreSharedFileTests.cs b/gclo.Engine.Tests/AccountsStoreSharedFileTests.cs index 2100678..803ee31 100644 --- a/gclo.Engine.Tests/AccountsStoreSharedFileTests.cs +++ b/gclo.Engine.Tests/AccountsStoreSharedFileTests.cs @@ -228,10 +228,10 @@ public sealed class AccountsStoreSharedFileTests : IDisposable using(new FileStream(LockPath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None)) { - IOException save = Assert.Throws(() => store.Save(added, "ghp_never_stored")); + AccountsFileBusyException save = Assert.Throws(() => store.Save(added, "ghp_never_stored")); Assert.Contains("in use by another gclo process", save.Message); - Assert.Throws(() => store.Delete(existing.Id)); - Assert.Throws(() => store.RecordSyncResult(existing.Id, DateTimeOffset.UtcNow, "done")); + Assert.Throws(() => store.Delete(existing.Id)); + Assert.Throws(() => store.RecordSyncResult(existing.Id, DateTimeOffset.UtcNow, "done")); } Assert.Equal(before, File.ReadAllText(FilePath)); @@ -243,26 +243,40 @@ public sealed class AccountsStoreSharedFileTests : IDisposable [Fact] - public void Construction_WhenTheLockStaysHeld_StartsEmpty_SaysWhy_AndLeavesTheFile() + public void Construction_DoesNotWaitForTheLock_SoAStalledProcessCannotKeepAnotherFromStarting() { NewStore().Save(MakeAccount("Existing"), null); - string before = File.ReadAllText(FilePath); AccountsStore store; + var clock = System.Diagnostics.Stopwatch.StartNew(); using(new FileStream(LockPath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None)) { - store = NewStore(s_short); + store = NewStore(); // a patience of a minute, which it does not use } - Assert.Empty(store.GetAll()); - Assert.Contains(ErrorsLogged(), e => e.Contains("Could not read accounts", StringComparison.Ordinal) - && e.Contains("left as it is", StringComparison.Ordinal)); - Assert.Equal(before, File.ReadAllText(FilePath)); - Assert.Empty(Directory.GetFiles(_root, "accounts.json.corrupt-*")); + Assert.True(clock.Elapsed < TimeSpan.FromSeconds(30), $"construction waited {clock.Elapsed} for the lock"); + Assert.Equal(["Existing"], store.GetAll().Select(a => a.Name), StringComparer.Ordinal); + Assert.Empty(ErrorsLogged()); + } - // Its next change reads the file again, so what it could not show is not lost. - store.Save(MakeAccount("Added later"), null); - Assert.Equal(["Added later", "Existing"], NamesOnDisk(), StringComparer.Ordinal); + + + [Fact] + public void ALockThatCannotBeOpenedAtAll_IsNotTakenForABusyOne_AndDoesNotHideTheAccounts() + { + NewStore().Save(MakeAccount("Existing"), null); + File.Delete(LockPath); + Directory.CreateDirectory(LockPath); // a folder where the lock file belongs + + AccountsStore store = NewStore(); + Assert.Equal(["Existing"], store.GetAll().Select(a => a.Name), StringComparer.Ordinal); + + var clock = System.Diagnostics.Stopwatch.StartNew(); + Exception ex = Assert.ThrowsAny(() => store.Save(MakeAccount("Never saved"), null)); + + Assert.IsNotType(ex); + Assert.True(clock.Elapsed < TimeSpan.FromSeconds(30), $"waited {clock.Elapsed} for something that does not pass"); + Assert.Equal(["Existing"], NamesOnDisk(), StringComparer.Ordinal); } @@ -346,8 +360,195 @@ public sealed class AccountsStoreSharedFileTests : IDisposable string aside = Assert.Single(Directory.GetFiles(_root, "accounts.json.corrupt-*")); Assert.Equal("{ not json", File.ReadAllText(aside)); - Assert.Equal(["Fresh"], NamesOnDisk(), StringComparer.Ordinal); + // The change did not start over from nothing: what the store last read is written back with it. + Assert.Equal(["Existing", "Fresh"], NamesOnDisk(), StringComparer.Ordinal); Assert.Contains(ErrorsLogged(), e => e.Contains("not a valid list of accounts", StringComparison.Ordinal)); + Assert.Contains(ErrorsLogged(), e => e.Contains("missing or damaged", StringComparison.Ordinal)); + } + + + + // ---------------------------------------------------------------- two processes and one account + + [Fact] + public void AnEditOfAnAccountTheOtherProcessRemoved_IsRefused_NotBroughtBackWithoutItsToken() + { + Account work = MakeAccount("Work"); + NewStore().Save(work, "ghp_work_token"); + AccountsStore app = NewStore(); // has Work loaded + AccountsStore cli = NewStore(); + cli.Delete(work.Id); + + InvalidOperationException ex = Assert.Throws( + () => app.Save(work with { Description = "edited in the app" }, null)); + + Assert.Contains("was removed by another gclo process", ex.Message); + Assert.Empty(NamesOnDisk()); + Assert.Null(_vault.TryRetrieve(work.Id)); + Assert.Empty(app.GetAll()); // and the store now knows it is gone + + // Adding it again is a new decision, and works. + app.Save(work, "ghp_new_token"); + Assert.Equal(["Work"], NamesOnDisk(), StringComparer.Ordinal); + } + + + + [Fact] + public void AnEditFromAnOlderCopy_KeepsTheSyncResultTheOtherProcessRecordedSince() + { + Account work = MakeAccount("Work"); + NewStore().Save(work, null); + AccountsStore app = NewStore(); + AccountsStore cli = NewStore(); + var recorded = new DateTimeOffset(2026, 11, 10, 12, 0, 0, TimeSpan.Zero); + cli.RecordSyncResult(work.Id, recorded, "42 done"); + + app.Save(work with { Description = "edited in the app" }, null); // 'work' was read before the sync + + Account onDisk = Assert.Single(new AccountsStore(_vault, _root).GetAll()); + Assert.Equal("edited in the app", onDisk.Description); + Assert.Equal(recorded, onDisk.LastSyncUtc); + Assert.Equal("42 done", onDisk.LastSyncSummary); + } + + + + [Fact] + public void AnEditThatCarriesANewerSyncResult_KeepsItsOwn() + { + Account work = MakeAccount("Work") with + { + LastSyncUtc = new DateTimeOffset(2026, 11, 1, 0, 0, 0, TimeSpan.Zero), + LastSyncSummary = "older", + }; + AccountsStore store = NewStore(); + store.Save(work, null); + var newer = new DateTimeOffset(2026, 11, 10, 0, 0, 0, TimeSpan.Zero); + + store.Save(work with { LastSyncUtc = newer, LastSyncSummary = "newer" }, null); + + Account onDisk = Assert.Single(new AccountsStore(_vault, _root).GetAll()); + Assert.Equal(newer, onDisk.LastSyncUtc); + Assert.Equal("newer", onDisk.LastSyncSummary); + } + + + + // ---------------------------------------------------------------- a file that is gone is not "no accounts" + + [Fact] + public void AChange_ThatFindsTheFileGone_IsAppliedToWhatTheStoreLastRead_WhichPutsTheAccountsBack() + { + Account kept = MakeAccount("Kept"); + Account removed = MakeAccount("Removed"); + AccountsStore store = NewStore(); + store.Save(kept, "ghp_kept_token"); + store.Save(removed, "ghp_removed_token"); + File.Delete(FilePath); + + store.Delete(removed.Id); + + Assert.Equal(["Kept"], NamesOnDisk(), StringComparer.Ordinal); + Assert.Null(_vault.TryRetrieve(removed.Id)); // the delete did what it said, also for the token + Assert.Equal("ghp_kept_token", _vault.TryRetrieve(kept.Id)); + Assert.Contains(ErrorsLogged(), e => e.Contains("missing or damaged", StringComparison.Ordinal)); + } + + + + [Fact] + public void AFirstChange_WhereThereNeverWasAFile_SaysNothingAboutDamage() + { + NewStore().Save(MakeAccount("First"), null); + + Assert.Empty(ErrorsLogged()); + } + + + + [Fact] + public async Task WhileChangesAreBeingSaved_TheFileIsNeverAbsent() + { + // A process that starts while another saves must find the accounts, not "no file yet". + AccountsStore store = NewStore(); + store.Save(MakeAccount("First"), null); + using var stop = new CancellationTokenSource(); + int absences = 0; + int looks = 0; + var watcher = Task.Run(() => + { + while(!stop.IsCancellationRequested) + { + looks++; + if(!File.Exists(FilePath)) + { + absences++; + } + } + }); + + for(int i = 0; i < 150; i++) + { + store.RecordSyncResult(store.GetAll()[0].Id, DateTimeOffset.UtcNow, $"run {i}"); + } + stop.Cancel(); + await watcher; + + Assert.True(looks > 0); + Assert.Equal(0, absences); + } + + + + // ---------------------------------------------------------------- files that will not change by waiting + + [Fact] + public void AReadOnlyAccountsFile_FailsAtOnce_AndSaysSo() + { + AccountsStore store = NewStore(); // a patience of a minute, which it does not use + Account existing = MakeAccount("Existing"); + store.Save(existing, null); + File.SetAttributes(FilePath, FileAttributes.ReadOnly); + Account added = MakeAccount("Never saved"); + try + { + var clock = System.Diagnostics.Stopwatch.StartNew(); + UnauthorizedAccessException ex = Assert.Throws(() => store.Save(added, "ghp_never_kept")); + + Assert.Contains("is read-only", ex.Message); + Assert.True(clock.Elapsed < TimeSpan.FromSeconds(10), $"waited {clock.Elapsed} for a file that stays read-only"); + Assert.Null(_vault.TryRetrieve(added.Id)); + } + finally + { + File.SetAttributes(FilePath, FileAttributes.Normal); + } + Assert.Equal(["Existing"], NamesOnDisk(), StringComparer.Ordinal); + } + + + + [Fact] + public void ABackupThatIsReadOnly_IsReplaced_AndOneThatCannotBeWritten_DoesNotStopTheChange() + { + AccountsStore store = NewStore(); + store.Save(MakeAccount("First"), null); + store.Save(MakeAccount("Second"), null); + string backup = FilePath + ".bak"; + File.SetAttributes(backup, FileAttributes.ReadOnly); + + store.Save(MakeAccount("Third"), null); + Assert.Contains("Second", File.ReadAllText(backup)); // the file as it was before this change + Assert.Empty(ErrorsLogged()); + + File.SetAttributes(backup, FileAttributes.Normal); + File.Delete(backup); + Directory.CreateDirectory(backup); // a folder where the backup belongs + store.Save(MakeAccount("Fourth"), null); + + Assert.Equal(["First", "Fourth", "Second", "Third"], NamesOnDisk(), StringComparer.Ordinal); + Assert.Contains(ErrorsLogged(), e => e.Contains("Could not write the backup", StringComparison.Ordinal)); } diff --git a/gclo.Engine.Tests/AppSettingsTests.cs b/gclo.Engine.Tests/AppSettingsTests.cs index 3945745..012ab3d 100644 --- a/gclo.Engine.Tests/AppSettingsTests.cs +++ b/gclo.Engine.Tests/AppSettingsTests.cs @@ -131,21 +131,74 @@ public sealed class AppSettingsTests [Fact] - public void Save_WhenItCannotWrite_SaysSo_AndThePreviousSettingsAreStillThere() + public void Save_WhenItCannotPutTheFileInPlace_SaysSo_LeavesThePreviousSettings_AndNoTemporaryFile() => WithDataDirectory(dir => { + if(!OperatingSystem.IsWindows()) + { + return; // elsewhere a file that is open can still be replaced + } var log = new RecordingActivityLog(); Assert.True(new AppSettings { Theme = "Dark" }.Save()); - // The new content is written beside the file first; a folder of that name stops it there. - Directory.CreateDirectory(Path.Combine(dir, "settings.json.tmp")); + string path = Path.Combine(dir, "settings.json"); - bool saved = new AppSettings { Theme = "Light" }.Save(log); + bool saved; + using(new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.Read)) + { + saved = new AppSettings { Theme = "Light" }.Save(log, TimeSpan.FromMilliseconds(100)); + } Assert.False(saved); (string level, string message) = Assert.Single(log.Entries); Assert.Equal("ERROR", level); Assert.StartsWith("Settings could not be saved (", message); Assert.Equal("Dark", AppSettings.Load().Theme); // not half a file, not the new one: the old one + Assert.Equal(["settings.json"], Directory.GetFiles(dir).Select(Path.GetFileName), StringComparer.Ordinal); + }); + + + + [Fact] + public async Task Save_WhileTheFileIsOpenForAMoment_TriesAgain() + { + if(!OperatingSystem.IsWindows()) + { + return; // elsewhere a file that is open can still be replaced + } + string dir = Path.Combine(Path.GetTempPath(), "gclo-tests", Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(dir); + Environment.SetEnvironmentVariable("GCLO_DATA_DIR", dir); + try + { + Assert.True(new AppSettings { Theme = "Dark" }.Save()); + Task save; + using(new FileStream(Path.Combine(dir, "settings.json"), FileMode.Open, FileAccess.Read, FileShare.Read)) + { + save = Task.Run(() => new AppSettings { Theme = "Light" }.Save(log: null, TimeSpan.FromSeconds(60))); + Task finished = await Task.WhenAny(save, Task.Delay(TimeSpan.FromMilliseconds(200))); + Assert.NotSame(save, finished); // still trying + } + + Assert.True(await save.WaitAsync(TimeSpan.FromSeconds(60))); + Assert.Equal("Light", AppSettings.Load().Theme); + } + finally + { + Environment.SetEnvironmentVariable("GCLO_DATA_DIR", null); + GitTestHelpers.TryDeleteDirectory(dir); + } + } + + + + [Fact] + public void Save_ByTwoAtOnce_NeverLeavesHalfAFile_OrALeftover() + => WithDataDirectory(dir => + { + Parallel.For(0, 40, i => new AppSettings { Theme = i % 2 == 0 ? "Dark" : "Light" }.Save()); + + Assert.Contains(AppSettings.Load().Theme, new[] { "Dark", "Light" }, StringComparer.Ordinal); + Assert.Equal(["settings.json"], Directory.GetFiles(dir).Select(Path.GetFileName), StringComparer.Ordinal); }); diff --git a/gclo.ViewModels/AccountsFileBusyException.cs b/gclo.ViewModels/AccountsFileBusyException.cs new file mode 100644 index 0000000..5275948 --- /dev/null +++ b/gclo.ViewModels/AccountsFileBusyException.cs @@ -0,0 +1,22 @@ +/* + * Copyright (c) 2026 James Maes (KofTwentyTwo) + * SPDX-License-Identifier: MIT + */ + +namespace gclo.ViewModels; + + +/// +/// A change to the accounts could not be made because another gclo process kept the +/// accounts file's lock for the whole wait. Nothing was changed, and nothing is +/// wrong: the same change succeeds once the other process has finished. An +/// , so everything that already handles one keeps working. +/// +public sealed class AccountsFileBusyException : IOException +{ + /// Creates the exception with its message and the sharing violation that caused it. + public AccountsFileBusyException(string message, Exception innerException) + : base(message, innerException) + { + } +} diff --git a/gclo.ViewModels/AccountsStore.cs b/gclo.ViewModels/AccountsStore.cs index 0c42b13..310d752 100644 --- a/gclo.ViewModels/AccountsStore.cs +++ b/gclo.ViewModels/AccountsStore.cs @@ -44,21 +44,26 @@ namespace gclo.ViewModels; /// the file; Delete tolerates an absent vault entry. /// /// -/// In-memory state changes only after both stores succeeded. Nothing here logs a token. +/// A change shows in only after both stores succeeded. Nothing here logs a token. /// /// The desktop app and the CLI share the file, and each may run more than once. Every /// change therefore takes a lock that every gclo process takes (accounts.json.lock, /// opened for this process alone), reads the file again, applies its one change by /// to what the file holds now, and writes that: an account -/// another process added, changed, or removed in the meantime stays as that process -/// left it. Reads (, ) answer from what -/// this store last read, at construction or at its latest change. +/// another process added or removed in the meantime stays added or removed, and a +/// sync result it recorded is kept. When two processes edit the same account, the +/// later edit wins; an edit of an account that another process removed is refused +/// instead of bringing it back. Reads (, ) +/// answer from what this store last read, at construction or at its latest change; +/// that is also what the store holds after a change that failed. /// /// /// A file that cannot be read is not a corrupt file. Only content that is not valid /// JSON is moved aside. A file that is in use is tried again for a short while; one /// that stays unreadable is left exactly where it is: at construction the store then -/// starts empty (and logs why), and a change fails without writing anything. +/// starts empty (and logs why), and a change fails without writing anything. A +/// change that finds the file gone or damaged does not start over from nothing: it +/// is applied to what this store last read, which puts the accounts back. /// /// public sealed class AccountsStore @@ -69,9 +74,17 @@ public sealed class AccountsStore private readonly string _filePath; - /// How long a busy file (the lock, or accounts.json itself) is waited for. + /// How long the lock another gclo process holds is waited for. private static readonly TimeSpan s_defaultPatience = TimeSpan.FromSeconds(5); + /// + /// The share of the patience spent on a file that something outside gclo holds for + /// a moment (reading it, or swapping the new one into place). Kept well below the + /// wait for the lock, so a process that holds the lock while it waits here does + /// not outlast the patience of the ones waiting for it. + /// + private const int FilePatienceDivisor = 5; + private static readonly TimeSpan s_pause = TimeSpan.FromMilliseconds(15); private readonly Lock _gate = new(); @@ -147,13 +160,17 @@ public sealed class AccountsStore ArgumentNullException.ThrowIfNull(account); lock(_gate) { + bool knownBefore = _accounts.Any(a => a.Id == account.Id); + /////////////////////////////////////////////////////////////////////////// // Another gclo process may have changed the file since it was last read. // // The change is applied to what the file holds now, under the lock every // // gclo process takes, so nothing another process wrote is written over. // /////////////////////////////////////////////////////////////////////////// using FileStream _ = AcquireFileLock(); - _accounts = ReadFile(); + RefreshForAChange(); + + account = ReconcileWithTheFile(account, knownBefore); bool nameTaken = _accounts.Any(a => a.Id != account.Id @@ -315,7 +332,7 @@ public sealed class AccountsStore // gclo process takes, so nothing another process wrote is written over. // /////////////////////////////////////////////////////////////////////////// using FileStream _ = AcquireFileLock(); - _accounts = ReadFile(); + RefreshForAChange(); var remaining = _accounts.Where(a => a.Id != id).ToList(); if(remaining.Count == _accounts.Count) @@ -381,7 +398,7 @@ public sealed class AccountsStore // gclo process takes, so nothing another process wrote is written over. // /////////////////////////////////////////////////////////////////////////// using FileStream _ = AcquireFileLock(); - _accounts = ReadFile(); + RefreshForAChange(); int index = _accounts.FindIndex(a => a.Id == id); if(index < 0) @@ -405,19 +422,16 @@ public sealed class AccountsStore /// /// What the file holds when the store is created. Never throws: a file that /// cannot be read leaves the store empty, says why in the log, and is left where - /// it is (the next change reads it again and fails if it still cannot). + /// it is (the next change reads it again and fails if it still cannot). Read + /// without the lock: a new file is put in place in one step, so what is there is + /// always a whole file, and a stalled process that holds the lock cannot keep + /// another from starting. /// private List LoadAtStart() { try { - if(!File.Exists(_filePath)) - { - return []; // no accounts yet, and looking creates nothing - } - - using FileStream _ = AcquireFileLock(); - return ReadFile(); + return ReadFile() ?? []; } catch(Exception ex) when(ex is IOException or UnauthorizedAccessException) { @@ -432,20 +446,69 @@ public sealed class AccountsStore /// - /// The accounts the file holds now. Called with the file lock held. A missing file - /// is no accounts; content that is not a list of accounts is moved aside and is no - /// accounts either. A file that cannot be read at all throws, and is not touched. + /// Brings what this store holds up to what the file holds now, before a change is + /// applied. Called with the file lock held. A file that is gone or damaged is not + /// taken for "no accounts": the change is then applied to what this store last + /// read, and writing it puts the accounts back. /// - private List ReadFile() + private void RefreshForAChange() + { + List? current = ReadFile(); + if(current is not null) + { + _accounts = current; + } + else if(_accounts.Count > 0) + { + _log.Error( + $"Accounts file '{_filePath}' is missing or damaged; the change is applied to the {_accounts.Count} " + + "account(s) this session last read, which writes them back."); + } + } + + + + /// + /// What a save writes for , given what the file holds + /// now. An edit of an account that another process removed since this store last + /// read the file is refused: saving it would bring the account back, without the + /// token that was removed with it. A sync result another process recorded after + /// the caller's copy was read is kept. + /// + private Account ReconcileWithTheFile(Account account, bool knownBefore) + { + Account? onDisk = _accounts.FirstOrDefault(a => a.Id == account.Id); + if(onDisk is null) + { + return knownBefore + ? throw new InvalidOperationException( + $"Account '{account.Name}' was removed by another gclo process while it was open here; " + + "nothing was saved. Add it again to keep it.") + : account; + } + + return onDisk.LastSyncUtc > (account.LastSyncUtc ?? DateTimeOffset.MinValue) + ? account with { LastSyncUtc = onDisk.LastSyncUtc, LastSyncSummary = onDisk.LastSyncSummary } + : account; + } + + + + /// + /// The accounts the file holds now, or null when it holds none that can be used: + /// the file is missing, or its content is not a list of accounts (it is then moved + /// aside). A file that cannot be read at all throws, and is not touched. + /// + private List? ReadFile() { string json; try { - json = WithPatience(() => File.ReadAllText(_filePath), accessDeniedIsBusy: false); + json = WithPatience(() => File.ReadAllText(_filePath), FilePatience, accessDeniedIsBusy: false); } catch(Exception ex) when(ex is FileNotFoundException or DirectoryNotFoundException) { - return []; + return null; } try @@ -467,17 +530,22 @@ public sealed class AccountsStore } PreserveCorruptFile(); - return []; + return null; } + private TimeSpan FilePatience => _patience / FilePatienceDivisor; + + + /// - /// Takes the lock that every gclo process takes before it reads or changes the - /// accounts file, waiting a short while for another process to finish. The lock is - /// a file beside accounts.json, opened for this process alone and held until it is + /// Takes the lock that every gclo process takes before it changes the accounts + /// file, waiting a short while for another process to finish. The lock is a file + /// beside accounts.json, opened for this process alone and held until it is /// disposed; the file itself stays, empty. /// + /// Another gclo process kept the lock for the whole wait. private FileStream AcquireFileLock() { string? directory = Path.GetDirectoryName(_filePath); @@ -490,11 +558,12 @@ public sealed class AccountsStore { return WithPatience( () => new FileStream(_filePath + ".lock", FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None), + _patience, accessDeniedIsBusy: false); } catch(IOException ex) when(IsBusy(ex, accessDeniedIsBusy: false)) { - throw new IOException( + throw new AccountsFileBusyException( $"The accounts file '{_filePath}' is in use by another gclo process; nothing was changed. Try again in a moment.", ex); } @@ -502,8 +571,8 @@ public sealed class AccountsStore - /// Runs , trying again for a short while as long as the file is busy. - private T WithPatience(Func action, bool accessDeniedIsBusy) + /// Runs , trying again for as long as the file is busy. + private static T WithPatience(Func action, TimeSpan patience, bool accessDeniedIsBusy) { long started = Stopwatch.GetTimestamp(); while(true) @@ -512,7 +581,7 @@ public sealed class AccountsStore { return action(); } - catch(Exception ex) when(IsBusy(ex, accessDeniedIsBusy) && Stopwatch.GetElapsedTime(started) < _patience) + catch(Exception ex) when(IsBusy(ex, accessDeniedIsBusy) && Stopwatch.GetElapsedTime(started) < patience) { Thread.Sleep(s_pause); } @@ -522,13 +591,23 @@ public sealed class AccountsStore /// - /// True for a failure that means "in use right now": a sharing violation, and, where - /// the operation reports a busy target that way, a refused access. A missing file - /// or folder is never that. + /// True for a failure that means "in use right now", which passes: on Windows a + /// sharing or lock violation, and, where the operation reports a busy target that + /// way (replacing a file somebody has open), a refused access. A missing file, a + /// full disk, or a path that is too long is never that, and is not waited for. /// private static bool IsBusy(Exception exception, bool accessDeniedIsBusy) - => exception is IOException and not (FileNotFoundException or DirectoryNotFoundException or PathTooLongException) - || (accessDeniedIsBusy && exception is UnauthorizedAccessException); + { + const int SharingViolation = 32; + const int LockViolation = 33; + if(exception is UnauthorizedAccessException) + { + return accessDeniedIsBusy; + } + + return exception is IOException and not (FileNotFoundException or DirectoryNotFoundException or PathTooLongException) + && (!OperatingSystem.IsWindows() || (exception.HResult & 0xFFFF) is SharingViolation or LockViolation); + } @@ -577,24 +656,57 @@ public sealed class AccountsStore stream.Flush(flushToDisk: true); } + KeepABackup(); + /////////////////////////////////////////////////////////////////////////////// - // Something outside gclo (a scanner, a backup, an editor) can hold the live // - // file for a moment; the swap is then refused. It is tried again for a short // - // while before the change is given up. // + // A file nobody may write to stays that way however long it is tried: say // + // so at once instead of taking the refusal for a file that is busy. // + /////////////////////////////////////////////////////////////////////////////// + if(File.Exists(_filePath) && File.GetAttributes(_filePath).HasFlag(FileAttributes.ReadOnly)) + { + throw new UnauthorizedAccessException($"The accounts file '{_filePath}' is read-only; nothing was changed."); + } + + /////////////////////////////////////////////////////////////////////////////// + // One step puts the new file in place of the old: the name is never absent, // + // so a process that reads without the lock always finds a whole file. // + // Something outside gclo (a scanner, a backup, a viewer) can hold the live // + // file for a moment; the step is then refused, and tried again for a while. // /////////////////////////////////////////////////////////////////////////////// WithPatience( () => { - if(File.Exists(_filePath)) - { - File.Replace(tempPath, _filePath, _filePath + ".bak"); - } - else - { - File.Move(tempPath, _filePath); - } + File.Move(tempPath, _filePath, overwrite: true); return true; }, + FilePatience, accessDeniedIsBusy: true); } + + + + /// + /// Copies the file that is about to be replaced to accounts.json.bak. Best effort: + /// the backup is a convenience for a person, and failing to write it must not + /// stop the change it is a backup for. + /// + private void KeepABackup() + { + try + { + if(File.Exists(_filePath)) + { + string backup = _filePath + ".bak"; + if(File.Exists(backup)) + { + File.SetAttributes(backup, FileAttributes.Normal); + } + File.Copy(_filePath, backup, overwrite: true); + } + } + catch(Exception ex) when(ex is IOException or UnauthorizedAccessException) + { + _log.Error($"Could not write the backup '{_filePath}.bak' ({ex.Message}); the change goes ahead without it.", ex); + } + } } diff --git a/gclo.ViewModels/AppSettings.cs b/gclo.ViewModels/AppSettings.cs index e102c29..320b08a 100644 --- a/gclo.ViewModels/AppSettings.cs +++ b/gclo.ViewModels/AppSettings.cs @@ -90,16 +90,27 @@ public sealed class AppSettings + /// How long a save tries to put the new file in place while something has the old one open. + private static readonly TimeSpan s_savePatience = TimeSpan.FromSeconds(1); + + + /// /// Persists settings to disk, creating the directory if needed. The new content is - /// written beside the file and then put in its place, so a crash or a full disk in - /// the middle leaves the previous settings, never half a file. Failures (locked - /// file, read-only profile, full disk, ...) do not throw, because losing a - /// preference write must never crash the app; they are reported to - /// and in the result. + /// written beside the file and then put in its place in one step, so a crash or a + /// full disk in the middle leaves the previous settings, never half a file. The + /// step is refused while anything has the file open, so it is tried again for a + /// moment. Failures (locked file, read-only profile, full disk, ...) do not throw, + /// because losing a preference write must never crash the app; they are reported + /// to and in the result. /// /// True when the settings are on disk. - public bool Save(IActivityLog? log = null) + public bool Save(IActivityLog? log = null) => Save(log, s_savePatience); + + + + /// Saves with its own patience for a file that is in use, for tests. + internal bool Save(IActivityLog? log, TimeSpan patience) { try { @@ -111,9 +122,18 @@ public sealed class AppSettings Directory.CreateDirectory(directory); } - string temporary = path + ".tmp"; - File.WriteAllText(temporary, JsonSerializer.Serialize(this, AppSettingsJsonContext.Default.AppSettings)); - File.Move(temporary, path, overwrite: true); + // A name of its own: two processes saving at once do not write one file. + string temporary = $"{path}.{Guid.NewGuid():N}.tmp"; + try + { + File.WriteAllText(temporary, JsonSerializer.Serialize(this, AppSettingsJsonContext.Default.AppSettings)); + MoveIntoPlace(temporary, path, patience); + } + finally + { + // Gone already when the move succeeded; otherwise nothing is left behind. + File.Delete(temporary); + } return true; } catch(Exception ex) @@ -125,6 +145,26 @@ public sealed class AppSettings + private static void MoveIntoPlace(string temporary, string path, TimeSpan patience) + { + long started = System.Diagnostics.Stopwatch.GetTimestamp(); + while(true) + { + try + { + File.Move(temporary, path, overwrite: true); + return; + } + catch(Exception ex) when(ex is IOException or UnauthorizedAccessException + && System.Diagnostics.Stopwatch.GetElapsedTime(started) < patience) + { + Thread.Sleep(15); + } + } + } + + + private void Sanitize() { // The JSON deserializer can assign null despite the non-nullable annotation. diff --git a/gclo/SettingsDialog.xaml.cs b/gclo/SettingsDialog.xaml.cs index adee0ea..7167873 100644 --- a/gclo/SettingsDialog.xaml.cs +++ b/gclo/SettingsDialog.xaml.cs @@ -154,7 +154,7 @@ public sealed partial class SettingsDialog : ContentDialog } } - _settings.Save(_log); + bool written = _settings.Save(_log); string typed = DefaultTokenBox.Password; switch(_tokenState.Plan(typed)) @@ -173,9 +173,10 @@ public sealed partial class SettingsDialog : ContentDialog break; } + string outcome = written ? "Settings saved" : "Settings apply until the app closes (they could not be written, see above)"; _log.Info(changes.Count == 0 - ? "Settings saved (no changes)." - : "Settings saved: " + string.Join("; ", changes) + "."); + ? outcome + " (no changes)." + : outcome + ": " + string.Join("; ", changes) + "."); } -- 2.54.0