diff --git a/docs/CLI.md b/docs/CLI.md index 27897fd..400b9f1 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -237,10 +237,21 @@ 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. 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. @@ -404,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 new file mode 100644 index 0000000..803ee31 --- /dev/null +++ b/gclo.Engine.Tests/AccountsStoreSharedFileTests.cs @@ -0,0 +1,609 @@ +/* + * 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)) + { + 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.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_DoesNotWaitForTheLock_SoAStalledProcessCannotKeepAnotherFromStarting() + { + NewStore().Save(MakeAccount("Existing"), null); + + AccountsStore store; + var clock = System.Diagnostics.Stopwatch.StartNew(); + using(new FileStream(LockPath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None)) + { + store = NewStore(); // a patience of a minute, which it does not use + } + + 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()); + } + + + + [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); + } + + + + [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)); + // 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)); + } + + + + // ---------------------------------------------------------------- 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..012ab3d 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,141 @@ 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_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()); + string path = Path.Combine(dir, "settings.json"); + + 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); + }); + + + + [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/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 85f3f77..310d752 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). /// /// /// @@ -43,7 +44,27 @@ 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 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. 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 { @@ -53,8 +74,23 @@ public sealed class AccountsStore private readonly string _filePath; + /// 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(); + private readonly TimeSpan _patience; + private List _accounts; @@ -66,13 +102,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 +147,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 +160,18 @@ 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(); + RefreshForAChange(); + + account = ReconcileWithTheFile(account, knownBefore); + bool nameTaken = _accounts.Any(a => a.Id != account.Id && string.Equals(a.Name, account.Name, StringComparison.OrdinalIgnoreCase)); @@ -269,6 +326,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(); + RefreshForAChange(); + var remaining = _accounts.Where(a => a.Id != id).ToList(); if(remaining.Count == _accounts.Count) { @@ -319,13 +384,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(); + RefreshForAChange(); + int index = _accounts.FindIndex(a => a.Id == id); if(index < 0) { @@ -345,31 +419,194 @@ 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). 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 []; - } + 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 []; + } + } - List? loaded = JsonSerializer.Deserialize( - File.ReadAllText(_filePath), AccountsJsonContext.Default.ListAccount); + + + /// + /// 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 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), FilePatience, accessDeniedIsBusy: false); + } + catch(Exception ex) when(ex is FileNotFoundException or DirectoryNotFoundException) + { + return null; + } + + 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); } - return []; + PreserveCorruptFile(); + return null; + } + + + + private TimeSpan FilePatience => _patience / FilePatienceDivisor; + + + + /// + /// 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); + if(!string.IsNullOrEmpty(directory)) + { + Directory.CreateDirectory(directory); + } + + try + { + 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 AccountsFileBusyException( + $"The accounts file '{_filePath}' is in use by another gclo process; nothing was changed. Try again in a moment.", + ex); + } + } + + + + /// 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) + { + 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", 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) + { + 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); } @@ -419,13 +656,57 @@ public sealed class AccountsStore stream.Flush(flushToDisk: true); } - if(File.Exists(_filePath)) + KeepABackup(); + + /////////////////////////////////////////////////////////////////////////////// + // 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)) { - File.Replace(tempPath, _filePath, _filePath + ".bak"); + throw new UnauthorizedAccessException($"The accounts file '{_filePath}' is read-only; nothing was changed."); } - else + + /////////////////////////////////////////////////////////////////////////////// + // 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( + () => + { + 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 { - File.Move(tempPath, _filePath); + 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 5c96ee4..320b08a 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(); @@ -87,12 +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. 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 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. /// - public void Save() + /// True when the settings are on disk. + 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 { @@ -103,11 +121,45 @@ public sealed class AppSettings { Directory.CreateDirectory(directory); } - File.WriteAllText(path, JsonSerializer.Serialize(this, AppSettingsJsonContext.Default.AppSettings)); + + // 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 + 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; + } + } + + + + 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); + } } } @@ -118,11 +170,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..7167873 100644 --- a/gclo/SettingsDialog.xaml.cs +++ b/gclo/SettingsDialog.xaml.cs @@ -154,7 +154,7 @@ public sealed partial class SettingsDialog : ContentDialog } } - _settings.Save(); + 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) + "."); }