diff --git a/docs/adr/0006-settings-file-protocol.md b/docs/adr/0006-settings-file-protocol.md new file mode 100644 index 0000000..f0a3ae3 --- /dev/null +++ b/docs/adr/0006-settings-file-protocol.md @@ -0,0 +1,95 @@ +# ADR-0006: Settings file protocol + +- **Status:** Accepted +- **Date:** 2026-11-10 + +## Context + +One `settings.json` per edition is shared by every reader window and by every +process that uses the same data folder: an installed copy and a portable copy, or +two builds started from different folders, are separate processes over one file. + +AppKit's `SettingsStore` returns defaults whenever the file cannot be read and writes +a whole snapshot through one fixed temporary name with no lock. With real files and +real processes this lost data in two ways: a load during a sharing violation returned +defaults that the next save wrote back, and two processes saving in turn erased each +other's changes. + +The files beside `settings.json` are now a contract between application versions that +may run at the same time, so the protocol is recorded here. + +## Decision + +`AppSettingsStore` in the core library owns the file. AppKit still defines the shell +preferences (`ShellSettings`); only persistence moved. + +**Change, not snapshot.** `Update` takes a function from the stored settings to the +new settings. It runs under the lock against what is on disk at that moment, so a +caller cannot write back a stale copy of a value it did not change. + +**Lock.** `settings.json.lock` is opened with no sharing for the duration of an +update. It is never deleted: deleting a lock file leaves a window in which a second +opener is refused with access denied. The operating system releases the handle if +the process dies. A lock that stays held for every attempt refuses the update. + +**Read.** The file is opened for reading while denying writers. A program that is +rewriting the file in place therefore reads as busy, not as invalid content, and +during an update no such program can start until the swap has happened. Readers +allow the file to be replaced, so a read never blocks a save. + +**What a read can find.** + +| Found | Used for display | On update | +| --- | --- | --- | +| No file | Defaults | The file is created | +| Valid settings | The stored values | Replaced | +| Some invalid values | Valid values kept, invalid ones as defaults | Replaced; the original is kept aside | +| Not settings at all | Defaults | Replaced; the original is kept aside | +| Unreadable or being written | Defaults | Refused; nothing is written | + +A trailing comma, a comment and a quoted number are valid: they are what a hand edit +commonly leaves behind. A file over 16 MiB is not loaded. + +**Write.** The new content is written to `settings.json.<32 hex digits>.tmp`, +flushed to disk, and swapped in with a replace operation, which also moves an invalid +original to its kept-aside name in the same step. If the file did not exist when it +was read, the temporary file is renamed into place without overwriting. A failed +update leaves the stored file as it was and removes its temporary file. + +**Kept-aside files.** `settings.json.corrupt`, then `settings.json.corrupt-2` to +`settings.json.corrupt-9`; the first free name is used, and the last is reused once +all exist. Nothing reads them back; they exist so a hand edit gone wrong can be +recovered. + +**Abandoned temporary files.** A process killed between its write and its swap +leaves a temporary file. The next update, holding the lock, removes files that match +this store's own name pattern and no others. + +**Waiting.** Only a sharing violation is waited out: at most 20 attempts, 50 ms +apart, for the lock, the read and the swap. Access denied is treated as permanent and +reported at once, so a read-only profile does not stall a window. + +**Reporting.** Faults are reported by kind with the exception type and HRESULT. No +path and no operating-system message text reaches the log. + +## Consequences + +- An update is a synchronous call on the window thread. It normally takes a few + milliseconds and at most about one second per contended step. +- The JSON written is byte-compatible with earlier releases, so a downgrade reads it. +- An earlier release running against the same folder neither takes the lock nor + denies writers. While it is mid-save, this store reads the file as busy and refuses + the update; while this store holds the file, the earlier release's save fails and + it reports that the setting could not be saved. Neither loses stored data, but the + two can refuse each other until both are the same version. +- The store keeps no in-memory copy. Windows hold their own view of the settings and + merge only the fields they changed (ADR-0004). + +## Options considered + +- **A named mutex instead of a lock file.** Rejected: a session-local name does not + cover two sessions of one user on the same profile, and a global name can be + created first by another account on the machine. +- **Fixing AppKit's store instead.** Still worthwhile for other applications, but the + read-modify-write API is a different shape, and this application needed the fix + without waiting for a package release. diff --git a/docs/security/threat-model.md b/docs/security/threat-model.md index 7bbc71a..7a83d6d 100644 --- a/docs/security/threat-model.md +++ b/docs/security/threat-model.md @@ -34,7 +34,7 @@ physical directory. It does not authorize native commands or remote resource acc | Remote resources | Denied by CSP and resource interception | | External links | HTTP(S) only, actual browser user gesture, external Windows handler; no document navigation outside current page | | Heading navigation | Labels extracted after sanitation; authored identity attributes stripped; generated numeric targets only. Fixed host operations reveal/jump/collapse on the current intercepted page; authored scripts, messages and host objects remain disabled | -| Settings / logs | AppKit sanitation; per-user data root; log operational events without document content or paths. Settings changes are read-modify-write under a lock file beside `settings.json` and replace it by rename; an unreadable file is never overwritten, invalid content is kept as `settings.json.corrupt`, a file over 16 MiB is not loaded, and faults are logged by kind, exception type and HRESULT only | +| Settings / logs | AppKit sanitation; per-user data root; log operational events without document content or paths. Settings changes are read-modify-write under a lock file beside `settings.json` and are swapped in atomically; a file that is unreadable or being written by another program is never overwritten, invalid content is kept aside under a bounded set of names, a file over 16 MiB is not loaded, and faults are logged by kind, exception type and HRESULT only (ADR-0006) | | Updates | Repository HTTPS feed, Velopack integrity checks and production signing preflight | | Repository / CI | Protected main and release tags; producer-bound checks; no untrusted PR checkout with privileged review token | diff --git a/docs/user-guide.md b/docs/user-guide.md index 25e0d14..c0c1176 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -73,7 +73,8 @@ Documents are rendered locally. Authored scripts and remote images are blocked; - **Open in editor is disabled:** hover for the reason, then configure an installed editor in Settings. - **The viewer is not a default option:** use Setup rather than a portable copy, then reopen Windows Default Apps. - **The app does not start:** confirm Microsoft Edge WebView2 Runtime is installed and inspect the activity logs if available. -- **A setting could not be saved:** another program is holding `settings.json`, or the folder is read-only. Nothing is overwritten while the file cannot be read; try the change again. If the file was edited by hand and is no longer valid, the app starts with defaults and keeps the invalid file as `settings.json.corrupt` on the next save. +- **A setting could not be saved:** another program is using `settings.json`, or the file or folder is read-only. Nothing is overwritten while the file cannot be read; try the change again. +- **Settings were edited by hand:** a trailing comma, a comment or a quoted number is accepted. A value that is not valid is ignored and the rest are kept. The next save keeps the file as it was beside the new one, as `settings.json.corrupt` (then `.corrupt-2` up to `.corrupt-9`). For help, use [Discussions](https://github.com/KofTwentyTwo/MarkdownViewr.Windows/discussions). Report reproducible problems through [Issues](https://github.com/KofTwentyTwo/MarkdownViewr.Windows/issues/new/choose), including **Help → About** version, Windows version, and relevant sanitized logs. Report vulnerabilities privately using [SECURITY.md](../SECURITY.md). diff --git a/src/MarkdownViewr.Windows.Core/AppSettings.cs b/src/MarkdownViewr.Windows.Core/AppSettings.cs index 2e0b17b..89ce5a0 100644 --- a/src/MarkdownViewr.Windows.Core/AppSettings.cs +++ b/src/MarkdownViewr.Windows.Core/AppSettings.cs @@ -202,8 +202,9 @@ public sealed class AppSettings : ShellSettings _ = Themes.ReaderTheme.ParseCompatible(json); return true; } - catch(JsonException) + catch(Exception exception) when(exception is JsonException or InvalidOperationException) { + // A string the JSON reader cannot decode, such as an unpaired surrogate escape, is invalid content too. return false; } } diff --git a/src/MarkdownViewr.Windows.Core/AppSettingsContext.cs b/src/MarkdownViewr.Windows.Core/AppSettingsContext.cs index 0ad1c6f..d881bbb 100644 --- a/src/MarkdownViewr.Windows.Core/AppSettingsContext.cs +++ b/src/MarkdownViewr.Windows.Core/AppSettingsContext.cs @@ -3,13 +3,17 @@ * SPDX-License-Identifier: MIT */ +using System.Text.Json; using System.Text.Json.Serialization; namespace MarkdownViewr.Windows.Core; -/// Source-generated JSON serializer for , so trimmed builds need no reflection. -[JsonSourceGenerationOptions(WriteIndented = true)] +/// +/// Source-generated JSON serializer for , so trimmed builds need no reflection. +/// Reading tolerates what a hand edit commonly leaves behind: a trailing comma, a comment, a quoted number. +/// +[JsonSourceGenerationOptions(WriteIndented = true, AllowTrailingCommas = true, ReadCommentHandling = JsonCommentHandling.Skip, NumberHandling = JsonNumberHandling.AllowReadingFromString)] [JsonSerializable(typeof(AppSettings))] public sealed partial class AppSettingsContext : JsonSerializerContext { diff --git a/src/MarkdownViewr.Windows.Core/AppSettingsStore.cs b/src/MarkdownViewr.Windows.Core/AppSettingsStore.cs index ee2645a..3978852 100644 --- a/src/MarkdownViewr.Windows.Core/AppSettingsStore.cs +++ b/src/MarkdownViewr.Windows.Core/AppSettingsStore.cs @@ -5,25 +5,36 @@ using System.Text; using System.Text.Json; +using System.Text.Json.Nodes; namespace MarkdownViewr.Windows.Core; /// /// The settings file shared by every window and every process of one edition. A change is -/// applied to the latest stored settings under a cross-process lock and replaces the file -/// atomically, so two instances cannot erase each other's change. A file that cannot be -/// read is never treated as empty: only invalid content is replaced, and it is kept aside. +/// applied to the latest stored settings under a cross-process lock and swapped in atomically, +/// so two instances cannot erase each other's change. A file that cannot be read, or that +/// another program is writing, is never treated as empty; content that is invalid while +/// nobody is writing it is repaired where possible and the original is kept aside. +/// See ADR-0006 for the on-disk protocol. /// public sealed class AppSettingsStore { - /// Attempts for the lock, a read, and the final replace before the operation gives up. + /// Attempts for the lock, a read, and the final swap before the operation gives up. public const int MaximumAttempts = 20; /// A larger file is not this application's settings and is never loaded into memory. public const int MaximumBytes = 16 * 1024 * 1024; - private static readonly TimeSpan s_retryDelay = TimeSpan.FromMilliseconds(50); + /// Invalid files kept aside before the newest slot is reused. + public const int MaximumKeptAside = 9; + + /// The pause between attempts while another handle is in the way. + public static readonly TimeSpan RetryDelay = TimeSpan.FromMilliseconds(50); + + private const int SharingViolation = unchecked((int)0x80070020); + private const int LockViolation = unchecked((int)0x80070021); + private static readonly JsonDocumentOptions s_lenient = new() { AllowTrailingCommas = true, CommentHandling = JsonCommentHandling.Skip }; private readonly Action _report; private readonly Action _wait; @@ -40,7 +51,7 @@ public sealed class AppSettingsStore - /// The settings file. + /// The settings file, as an absolute path. public string Path { get; } @@ -48,7 +59,8 @@ public sealed class AppSettingsStore /// Reads the stored settings for display. A missing, invalid, or unreadable file yields defaults and is left untouched. public AppSettings Load() { - _ = TryRead(out AppSettings settings); + _ = TryRead(out AppSettings settings, out FileStream? stored); + stored?.Dispose(); return settings; } @@ -57,7 +69,8 @@ public sealed class AppSettingsStore /// /// Applies one change to the latest stored settings and saves the result. Returns the saved /// settings, or null when nothing was written: the lock stayed busy, the file could not be - /// read, or the write failed. The previous file is intact in every null case. + /// read or was being written by another program, or the write failed. The stored file is + /// unchanged in every null case. /// public AppSettings? Update(Func change) { @@ -70,20 +83,20 @@ public sealed class AppSettingsStore { return null; } - SettingsRead read = TryRead(out AppSettings current); - if(read == SettingsRead.Unreadable) + SettingsRead read = TryRead(out AppSettings current, out FileStream? stored); + // The read handle denies writers until the swap, so the content judged here is the content replaced. + using(stored) { - return null; + if(read == SettingsRead.Unreadable) + { + return null; + } + AppSettings edited = change(current); + edited.Sanitize(); + RemoveAbandonedTemporaryFiles(); + Replace(JsonSerializer.SerializeToUtf8Bytes(edited, AppSettingsContext.Default.AppSettings), stored is not null, read is SettingsRead.Corrupt or SettingsRead.Repaired); + return edited; } - if(read == SettingsRead.Corrupt) - { - Retry(() => File.Move(Path, Path + ".corrupt", overwrite: true)); - } - AppSettings edited = change(current); - edited.Sanitize(); - RemoveAbandonedTemporaryFiles(); - Replace(JsonSerializer.SerializeToUtf8Bytes(edited, AppSettingsContext.Default.AppSettings)); - return edited; } catch(Exception exception) when(exception is IOException or UnauthorizedAccessException) { @@ -94,7 +107,10 @@ public sealed class AppSettingsStore - /// Holds an exclusive handle beside the settings file; the operating system releases it if the process dies. + /// + /// Holds an exclusive handle beside the settings file; the operating system releases it if the + /// process dies. A lock that stays held, or cannot be opened for another I/O reason, is busy. + /// private FileStream? Acquire() { try @@ -103,7 +119,7 @@ public sealed class AppSettingsStore Retry(() => gate = new FileStream(Path + ".lock", FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None)); return gate; } - catch(IOException exception) when(IsTransient(exception)) + catch(IOException exception) { _report(SettingsFault.Busy, exception); return null; @@ -112,36 +128,27 @@ public sealed class AppSettingsStore - /// Distinguishes a missing file, invalid content, and a file that could not be read; only the last keeps the caller from writing. - private SettingsRead TryRead(out AppSettings settings) + /// + /// Opens the file denying writers, so a program caught rewriting it in place reads as busy + /// rather than as invalid content, then distinguishes missing, valid, repaired and invalid. + /// The open handle is returned so the caller can keep writers out until it has finished. + /// + private SettingsRead TryRead(out AppSettings settings, out FileStream? stored) { settings = new AppSettings(); + stored = null; SettingsRead result; try { - string? json = null; - Retry(() => json = ReadText()); - AppSettings? loaded = json is null ? null : JsonSerializer.Deserialize(json, AppSettingsContext.Default.AppSettings); - if(loaded is null) - { - _report(SettingsFault.Corrupt, null); - result = SettingsRead.Corrupt; - } - else - { - settings = loaded; - result = SettingsRead.Loaded; - } + FileStream? stream = null; + Retry(() => stream = new FileStream(Path, FileMode.Open, FileAccess.Read, FileShare.Read | FileShare.Delete)); + stored = stream; + result = Parse(stream!, ref settings); } catch(Exception exception) when(exception is FileNotFoundException or DirectoryNotFoundException) { result = SettingsRead.Missing; } - catch(JsonException exception) - { - _report(SettingsFault.Corrupt, exception); - result = SettingsRead.Corrupt; - } catch(Exception exception) when(exception is IOException or UnauthorizedAccessException) { _report(SettingsFault.Unreadable, exception); @@ -153,22 +160,75 @@ public sealed class AppSettingsStore - /// Reads without blocking a concurrent replace; an oversized file reads as invalid content instead of exhausting memory. - private string? ReadText() + /// Reads at most ; one invalid value costs only that value, never the whole profile. + private SettingsRead Parse(FileStream stream, ref AppSettings settings) { - using var stream = new FileStream(Path, FileMode.Open, FileAccess.Read, FileShare.ReadWrite | FileShare.Delete); if(stream.Length > MaximumBytes) { - return null; + _report(SettingsFault.Corrupt, null); + return SettingsRead.Corrupt; } - using var reader = new StreamReader(stream, Encoding.UTF8, detectEncodingFromByteOrderMarks: true); - return reader.ReadToEnd(); + using var reader = new StreamReader(stream, Encoding.UTF8, detectEncodingFromByteOrderMarks: true, bufferSize: 4096, leaveOpen: true); + string json = reader.ReadToEnd(); + try + { + if(JsonSerializer.Deserialize(json, AppSettingsContext.Default.AppSettings) is { } loaded) + { + settings = loaded; + return SettingsRead.Loaded; + } + } + catch(JsonException) + { + if(Salvage(json) is { } repaired) + { + settings = repaired; + _report(SettingsFault.Repaired, null); + return SettingsRead.Repaired; + } + } + _report(SettingsFault.Corrupt, null); + return SettingsRead.Corrupt; } - /// Writes beside the file, flushes to disk, then renames over it; a failure leaves the previous file and no temporary file. - private void Replace(byte[] content) + /// Keeps every stored value that is valid on its own and drops the ones that are not; null when the file is not a JSON object at all. + private static AppSettings? Salvage(string json) + { + try + { + if(JsonNode.Parse(json, documentOptions: s_lenient) is not JsonObject stored) + { + return null; + } + foreach(string name in stored.Select(property => property.Key).ToArray()) + { + try + { + _ = new JsonObject { [name] = stored[name]?.DeepClone() }.Deserialize(AppSettingsContext.Default.AppSettings); + } + catch(JsonException) + { + stored.Remove(name); + } + } + return stored.Deserialize(AppSettingsContext.Default.AppSettings); + } + catch(Exception exception) when(exception is JsonException or ArgumentException) + { + // Not JSON, or an object that repeats a property name. + return null; + } + } + + + + /// + /// Writes beside the file and flushes to disk, then swaps it in. A failure leaves the stored + /// file and no temporary file. Invalid content being replaced is kept under a free name. + /// + private void Replace(byte[] content, bool exists, bool keepAside) { string temporary = Path + "." + Guid.NewGuid().ToString("N") + ".tmp"; bool replaced = false; @@ -179,7 +239,16 @@ public sealed class AppSettingsStore stream.Write(content); stream.Flush(flushToDisk: true); } - Retry(() => File.Move(temporary, Path, overwrite: true)); + if(exists) + { + string? aside = keepAside ? KeptAsideName() : null; + Retry(() => File.Replace(temporary, Path, aside, ignoreMetadataErrors: true)); + } + else + { + // No overwrite: a file that appeared since the read is not this instance's to replace. + File.Move(temporary, Path); + } replaced = true; } finally @@ -193,18 +262,36 @@ public sealed class AppSettingsStore - /// A writer that died before its rename left a temporary file; the lock holder is the only writer, so every one found is abandoned. + /// The first free name for an invalid file; once every slot is used the last slot is reused. + private string KeptAsideName() + { + string candidate = Path + ".corrupt"; + for(int slot = 2; slot <= MaximumKeptAside && File.Exists(candidate); slot++) + { + candidate = Path + ".corrupt-" + slot.ToString(System.Globalization.CultureInfo.InvariantCulture); + } + return candidate; + } + + + + /// A writer that died before its swap left a temporary file with this store's own name pattern; under the lock every such file is abandoned. private void RemoveAbandonedTemporaryFiles() { - foreach(string file in Directory.EnumerateFiles(System.IO.Path.GetDirectoryName(Path)!, System.IO.Path.GetFileName(Path) + "*.tmp")) + string prefix = System.IO.Path.GetFileName(Path) + "."; + foreach(string file in Directory.EnumerateFiles(System.IO.Path.GetDirectoryName(Path)!, prefix + "*.tmp")) { - TryDelete(file); + string name = System.IO.Path.GetFileName(file); + if(name.Length == prefix.Length + 36 && Guid.TryParseExact(name.AsSpan(prefix.Length, 32), "N", out _)) + { + TryDelete(file); + } } } - /// Repeats an operation only for a sharing violation or a pending delete, a bounded number of times; the last failure is thrown. + /// Repeats an operation only while another handle is in the way, a bounded number of times; the last failure is thrown. private void Retry(Action operation) { for(int attempt = 1; ; attempt++) @@ -214,24 +301,17 @@ public sealed class AppSettingsStore operation(); return; } - catch(Exception exception) when(attempt < MaximumAttempts && IsTransient(exception)) + catch(IOException exception) when(attempt < MaximumAttempts && IsTransient(exception)) { - _wait(s_retryDelay); + _wait(RetryDelay); } } } - /// Another handle on the file (an editor, a scanner, a second instance) or a delete that has not completed; a missing file is not transient. - private static bool IsTransient(Exception exception) - { - if(exception is UnauthorizedAccessException) - { - return true; - } - return exception is IOException and not (FileNotFoundException or DirectoryNotFoundException) && (exception.HResult & 0xFFFF) is 32 or 33; - } + /// A sharing or byte-range lock violation: another handle on the file that is expected to go away. Access denied is not transient. + private static bool IsTransient(IOException exception) => exception.HResult is SharingViolation or LockViolation; diff --git a/src/MarkdownViewr.Windows.Core/SettingsFault.cs b/src/MarkdownViewr.Windows.Core/SettingsFault.cs index 32a7c5f..501a04b 100644 --- a/src/MarkdownViewr.Windows.Core/SettingsFault.cs +++ b/src/MarkdownViewr.Windows.Core/SettingsFault.cs @@ -9,15 +9,14 @@ namespace MarkdownViewr.Windows.Core; /// Why the settings file could not be used as stored; reported without paths or operating-system message text. public enum SettingsFault { - /// The file exists but does not hold valid settings; defaults are used and the next save keeps the invalid file aside. + /// The file does not hold settings at all; defaults are used and the next save keeps the invalid file aside. Corrupt, - - /// The file could not be read after bounded retries; it is left untouched and nothing is written over it. + /// Some stored values were invalid and read as their defaults; the next save keeps the original file aside. + Repaired, + /// The file could not be read, or another program was writing it; it is left untouched and nothing is written over it. Unreadable, - - /// Another process kept the settings lock for every attempt; the change was not saved. + /// Another instance kept the settings lock for every attempt; the change was not saved. Busy, - - /// The change could not be written; the previous file remains in place. + /// The change could not be written; the stored file is unchanged. WriteFailed, } diff --git a/src/MarkdownViewr.Windows.Core/SettingsRead.cs b/src/MarkdownViewr.Windows.Core/SettingsRead.cs index d68d9a3..1ede827 100644 --- a/src/MarkdownViewr.Windows.Core/SettingsRead.cs +++ b/src/MarkdownViewr.Windows.Core/SettingsRead.cs @@ -11,13 +11,12 @@ internal enum SettingsRead { /// Valid stored settings. Loaded, - /// No file yet; defaults apply and the first save creates it. Missing, - - /// Invalid content; defaults apply and a save keeps the invalid file aside. + /// Valid values were kept and invalid ones dropped; a save keeps the original file aside. + Repaired, + /// Not settings at all; defaults apply and a save keeps the invalid file aside. Corrupt, - /// The content is unknown, so it must not be replaced. Unreadable, } diff --git a/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs index dc60af4..d574216 100644 --- a/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs +++ b/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs @@ -4,13 +4,15 @@ */ using System.Text; +using System.Text.Json.Nodes; namespace MarkdownViewr.Windows.Core.Tests; /// /// The settings file is shared by windows and processes: these tests use real files, real -/// exclusive handles and concurrent store instances, and count attempts instead of time. +/// handles held the way other programs hold them, and concurrent store instances. They count +/// attempts instead of measuring time. /// public class AppSettingsStoreTests { @@ -54,6 +56,18 @@ public class AppSettingsStoreTests + /// The store addresses one file whatever the working folder becomes later. + [Fact] + public void Constructor_RelativePath_IsMadeAbsolute() + { + var store = new AppSettingsStore(Path.Combine("relative-profile", "settings.json")); + Assert.True(Path.IsPathFullyQualified(store.Path)); + Assert.Equal(Path.GetFullPath(Path.Combine("relative-profile", "settings.json")), store.Path); + Assert.Throws(() => new AppSettingsStore(" ")); + } + + + /// A change is applied to what is stored now, not to what this instance read earlier. [Fact] public void Update_AnotherInstanceSavedFirst_ChangesTheLatestSettings() @@ -79,6 +93,39 @@ public class AppSettingsStoreTests + /// Callers that merge return a new object: that object is what is saved and what comes back. + [Fact] + public void Update_ChangeReturnsANewInstance_SavesAndReturnsThatInstance() + { + using var temp = new TempDirectory(); + Seed(temp); + var store = new AppSettingsStore(temp.File("settings.json")); + var replacement = new AppSettings { DocumentTheme = "Dracula", OutlineWidth = 9000 }; + AppSettings? saved = store.Update(_ => replacement); + Assert.Same(replacement, saved); + Assert.Equal(500, replacement.OutlineWidth); + AppSettings loaded = store.Load(); + Assert.Equal("Dracula", loaded.DocumentTheme); + Assert.Equal("System", loaded.Theme); + Assert.Empty(loaded.RecentFiles); + } + + + + /// Stored values outside their range are repaired on every read, not only on save. + [Fact] + public void Load_StoredValuesOutOfRange_AreSanitized() + { + using var temp = new TempDirectory(); + File.WriteAllText(temp.File("settings.json"), """{"OutlineDepth":99,"Theme":"Purple","OutlineWidth":-4}"""); + AppSettings loaded = new AppSettingsStore(temp.File("settings.json")).Load(); + Assert.Equal(6, loaded.OutlineDepth); + Assert.Equal("System", loaded.Theme); + Assert.Equal(140, loaded.OutlineWidth); + } + + + /// A file saved by hand with a byte order mark, in UTF-8 or UTF-16, is still the user's settings. [Theory] [InlineData(false)] @@ -97,24 +144,78 @@ public class AppSettingsStoreTests + /// Without a byte order mark the file is UTF-8, so text outside ASCII reads as written. + [Fact] + public void Load_Utf8WithoutByteOrderMark_ReadsTextOutsideAscii() + { + using var temp = new TempDirectory(); + File.WriteAllBytes(temp.File("settings.json"), new UTF8Encoding(encoderShouldEmitUTF8Identifier: false).GetBytes("""{"GlobalCss":"/* café ☕ */"}""")); + Assert.Equal("/* café ☕ */", new AppSettingsStore(temp.File("settings.json")).Load().GlobalCss); + } + + + + /// What a hand edit commonly leaves behind is still valid settings: a trailing comma, a comment, a quoted number. + [Theory] + [InlineData("""{"Theme":"Dark","OutlineDepth":4,}""")] + [InlineData("""{ /* my settings */ "Theme":"Dark", "OutlineDepth":4 }""")] + [InlineData("""{"Theme":"Dark","OutlineDepth":"4"}""")] + public void Load_HandEditedSyntax_ReadsWithoutAFault(string content) + { + using var temp = new TempDirectory(); + File.WriteAllText(temp.File("settings.json"), content); + var faults = new List(); + AppSettings loaded = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault)).Load(); + Assert.Equal("Dark", loaded.Theme); + Assert.Equal(4, loaded.OutlineDepth); + Assert.Empty(faults); + } + + + + /// The file on disk stays what earlier versions wrote and can read: indented UTF-8 JSON with no byte order mark. + [Fact] + public void Update_WrittenFile_IsIndentedUtf8WithoutByteOrderMarkAndReadableByThePreviousStore() + { + using var temp = new TempDirectory(); + Seed(temp); + byte[] bytes = File.ReadAllBytes(temp.File("settings.json")); + Assert.Equal((byte)'{', bytes[0]); + Assert.Contains(Environment.NewLine + " \"Theme\": \"Dark\"", Encoding.UTF8.GetString(bytes), StringComparison.Ordinal); + var previous = new KofTwentyTwo.AppKit.Settings.SettingsStore(temp.File("settings.json"), AppSettingsContext.Default.AppSettings); + AppSettings read = previous.Load(); + Assert.Equal("Nord", read.DocumentTheme); + Assert.Equal("body { color: red }", read.GlobalCss); + Assert.Equal([KeptPath], read.RecentFiles); + read.Theme = "Light"; + Assert.True(previous.Save(read)); + AppSettings loaded = new AppSettingsStore(temp.File("settings.json")).Load(); + Assert.Equal("Light", loaded.Theme); + Assert.Equal("Nord", loaded.DocumentTheme); + } + + + /// Another handle holding the file for a moment (a scanner, a backup) is waited out, not read as an empty profile. [Fact] public void Load_FileLockedBriefly_RetriesAndReadsTheStoredValues() { using var temp = new TempDirectory(); Seed(temp); - FileStream? exclusive = new(temp.File("settings.json"), FileMode.Open, FileAccess.Read, FileShare.None); - int waits = 0; + using var exclusive = new FileStream(temp.File("settings.json"), FileMode.Open, FileAccess.Read, FileShare.None); + var delays = new List(); var faults = new List(); - var store = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault), _ => + var store = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault), delay => { - if(++waits == 3) + delays.Add(delay); + if(delays.Count == 3) { exclusive.Dispose(); } }); AppSettings loaded = store.Load(); - Assert.Equal(3, waits); + Assert.Equal([AppSettingsStore.RetryDelay, AppSettingsStore.RetryDelay, AppSettingsStore.RetryDelay], delays); + Assert.Equal(TimeSpan.FromMilliseconds(50), AppSettingsStore.RetryDelay); Assert.Equal("Nord", loaded.DocumentTheme); Assert.Equal([KeptPath], loaded.RecentFiles); Assert.Empty(faults); @@ -144,8 +245,8 @@ public class AppSettingsStoreTests /// - /// The data-loss regression: a read failure is not an empty profile. Nothing is written over - /// a file whose content is unknown, so the stored theme, CSS and history survive. + /// A read failure is not an empty profile: nothing is written over a file whose content is + /// unknown, so the stored theme, CSS and history survive. /// [Fact] public void Update_FileLockedThroughout_WritesNothingOverIt() @@ -176,13 +277,102 @@ public class AppSettingsStoreTests - /// Only invalid content is corruption: reading it yields defaults, reports it, and leaves the file where it is. + /// + /// Access denied is permanent, not a moment to wait out: it is reported at once, defaults are + /// shown, and nothing is written. A folder where the file should be answers exactly that way. + /// + [Fact] + public void LoadAndUpdate_AccessDenied_ReportUnreadableWithoutWaitingOrWriting() + { + using var temp = new TempDirectory(); + Directory.CreateDirectory(temp.File("settings.json")); + int waits = 0; + var faults = new List<(SettingsFault Fault, Exception? Exception)>(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, exception) => faults.Add((fault, exception)), _ => waits++); + Assert.Equal("Catppuccin Mocha", store.Load().DocumentTheme); + bool changed = false; + Assert.Null(store.Update(settings => + { + changed = true; + return settings; + })); + Assert.False(changed); + Assert.Equal(0, waits); + Assert.Equal([SettingsFault.Unreadable, SettingsFault.Unreadable], faults.Select(entry => entry.Fault)); + Assert.All(faults, entry => Assert.IsType(entry.Exception)); + Assert.True(Directory.Exists(temp.File("settings.json"))); + Assert.Equal(["settings.json.lock"], FileNames(temp.Path)); + } + + + + /// + /// Another program rewriting the file in place has truncated it and not finished. That is a + /// busy file, not invalid content: it is not set aside, and once the writer finishes the + /// change is applied to what the writer wrote. + /// + [Fact] + public void Update_FileBeingRewrittenInPlace_WaitsForTheWriterAndKeepsItsContent() + { + using var temp = new TempDirectory(); + Seed(temp); + using var writer = new FileStream(temp.File("settings.json"), FileMode.Create, FileAccess.Write, FileShare.ReadWrite | FileShare.Delete); + int waits = 0; + var faults = new List(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault), _ => + { + if(++waits == 2) + { + writer.Write("""{"Theme":"Light","DocumentTheme":"Dracula","GlobalCss":"p { margin: 0 }"}"""u8); + writer.Dispose(); + } + }); + AppSettings? saved = store.Update(settings => + { + settings.OutlineDepth = 5; + return settings; + }); + Assert.NotNull(saved); + Assert.Equal(2, waits); + Assert.Empty(faults); + AppSettings loaded = store.Load(); + Assert.Equal("Light", loaded.Theme); + Assert.Equal("Dracula", loaded.DocumentTheme); + Assert.Equal("p { margin: 0 }", loaded.GlobalCss); + Assert.Equal(5, loaded.OutlineDepth); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); + } + + + + /// A writer that never finishes leaves the save refused and the file exactly as the writer has it. + [Fact] + public void Update_FileStillBeingRewritten_IsRefusedAndNothingIsSetAside() + { + using var temp = new TempDirectory(); + Seed(temp); + var faults = new List(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault), _ => { }); + using(new FileStream(temp.File("settings.json"), FileMode.Create, FileAccess.Write, FileShare.ReadWrite | FileShare.Delete)) + { + Assert.Null(store.Update(settings => settings)); + Assert.Equal("Catppuccin Mocha", store.Load().DocumentTheme); + } + Assert.Equal([SettingsFault.Unreadable, SettingsFault.Unreadable], faults); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); + Assert.Equal(0, new FileInfo(temp.File("settings.json")).Length); + } + + + + /// Content that is not settings at all yields defaults, is reported, and stays where it is until something is saved. [Theory] [InlineData("")] [InlineData("{")] [InlineData("null")] [InlineData("[1]")] - [InlineData("{\"OutlineDepth\":\"deep\"}")] + [InlineData("\"text\"")] + [InlineData("not json")] public void Load_InvalidContent_ReturnsDefaultsAndLeavesTheFile(string content) { using var temp = new TempDirectory(); @@ -197,39 +387,256 @@ public class AppSettingsStoreTests - /// Saving over invalid content keeps that content aside, so a hand edit gone wrong can still be recovered. + /// An invalid value in a file that also has hand-edited syntax still costs only that value. [Fact] - public void Update_InvalidContent_KeepsItAsideAndSavesTheChange() + public void Load_InvalidValueBesideHandEditedSyntax_KeepsEveryOtherValue() + { + using var temp = new TempDirectory(); + File.WriteAllText(temp.File("settings.json"), """{ /* mine */ "Theme":"Dark", "OutlineDepth":"deep", "DocumentTheme":"Nord", }"""); + var faults = new List(); + AppSettings loaded = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault)).Load(); + Assert.Equal([SettingsFault.Repaired], faults); + Assert.Equal("Dark", loaded.Theme); + Assert.Equal("Nord", loaded.DocumentTheme); + Assert.Equal(3, loaded.OutlineDepth); + } + + + + /// One invalid value costs only that value: every other stored preference is kept. + [Fact] + public void Load_OneInvalidValue_KeepsEveryOtherValue() + { + using var temp = new TempDirectory(); + File.WriteAllText(temp.File("settings.json"), """{"Theme":"Dark","OutlineDepth":5.5,"DocumentTheme":"Nord","RecentFiles":["C:\\docs\\a.md",5],"GlobalCss":"body { color: red }","FutureSetting":{"a":1}}"""); + var faults = new List(); + AppSettings loaded = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault)).Load(); + Assert.Equal([SettingsFault.Repaired], faults); + Assert.Equal("Dark", loaded.Theme); + Assert.Equal("Nord", loaded.DocumentTheme); + Assert.Equal("body { color: red }", loaded.GlobalCss); + Assert.Equal(3, loaded.OutlineDepth); + Assert.Empty(loaded.RecentFiles); + } + + + + /// A repeated property beside an invalid value is still read without throwing into startup. + [Fact] + public void Load_RepeatedPropertyAndInvalidValue_DoesNotThrow() + { + using var temp = new TempDirectory(); + File.WriteAllText(temp.File("settings.json"), """{"Theme":"Dark","Theme":"Light","OutlineDepth":"deep"}"""); + var faults = new List(); + AppSettings loaded = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault)).Load(); + Assert.Contains(Assert.Single(faults), new[] { SettingsFault.Corrupt, SettingsFault.Repaired }); + Assert.Equal(3, loaded.OutlineDepth); + } + + + + /// A saved theme whose text the JSON reader cannot decode is discarded like any other invalid theme; it cannot stop the application starting. + [Fact] + public void Load_SavedThemeWithUnpairedSurrogate_DiscardsThatThemeOnly() + { + using var temp = new TempDirectory(); + string valid = Themes.ThemeCatalog.Find("Nord").Json; + string undecodable = valid.Replace("\"Nord\"", "\"\\uD800\"", StringComparison.Ordinal); + Assert.NotEqual(valid, undecodable, StringComparer.Ordinal); + var stored = new JsonObject { ["Theme"] = "Dark", ["UserThemes"] = new JsonArray(undecodable, valid) }; + File.WriteAllText(temp.File("settings.json"), stored.ToJsonString()); + AppSettings loaded = new AppSettingsStore(temp.File("settings.json")).Load(); + Assert.Equal("Dark", loaded.Theme); + Assert.Equal([valid], loaded.UserThemes); + } + + + + /// Saving over invalid content keeps that content under a free name; an earlier kept file is not overwritten. + [Fact] + public void Update_InvalidContent_KeepsItAsideUnderAFreeNameAndSavesTheChange() { using var temp = new TempDirectory(); File.WriteAllText(temp.File("settings.json"), "{ \"Theme\": \"Dark\", "); - File.WriteAllText(temp.File("settings.json.corrupt"), "an older invalid file"); + File.WriteAllText(temp.File("settings.json.corrupt"), "an earlier invalid file"); var store = new AppSettingsStore(temp.File("settings.json")); Assert.NotNull(store.Update(settings => { settings.DocumentTheme = "Nord"; return settings; })); - Assert.Equal("{ \"Theme\": \"Dark\", ", File.ReadAllText(temp.File("settings.json.corrupt"))); + Assert.Equal("an earlier invalid file", File.ReadAllText(temp.File("settings.json.corrupt"))); + Assert.Equal("{ \"Theme\": \"Dark\", ", File.ReadAllText(temp.File("settings.json.corrupt-2"))); Assert.Equal("Nord", store.Load().DocumentTheme); Assert.Equal("System", store.Load().Theme); + Assert.Equal(["settings.json", "settings.json.corrupt", "settings.json.corrupt-2", "settings.json.lock"], FileNames(temp.Path)); } - /// A file far larger than any settings is not loaded into memory, even when it would parse; it reads as invalid content. + /// A repaired file is saved with its valid values and the change, and the original is kept for the value that was dropped. [Fact] - public void Load_OversizedFile_ReadsAsInvalidContent() + public void Update_RepairedContent_SavesTheValidValuesAndKeepsTheOriginalAside() { using var temp = new TempDirectory(); - byte[] valid = Encoding.UTF8.GetBytes("""{"DocumentTheme":"Nord"}"""); - byte[] padded = new byte[AppSettingsStore.MaximumBytes + 1]; + const string Original = """{"Theme":"Dark","OutlineDepth":"deep","DocumentTheme":"Nord"}"""; + File.WriteAllText(temp.File("settings.json"), Original); + var store = new AppSettingsStore(temp.File("settings.json")); + Assert.NotNull(store.Update(settings => + { + settings.GlobalCss = "p { margin: 0 }"; + return settings; + })); + Assert.Equal(Original, File.ReadAllText(temp.File("settings.json.corrupt"))); + var faults = new List(); + AppSettings loaded = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault)).Load(); + Assert.Empty(faults); + Assert.Equal("Dark", loaded.Theme); + Assert.Equal("Nord", loaded.DocumentTheme); + Assert.Equal("p { margin: 0 }", loaded.GlobalCss); + } + + + + /// Kept files are bounded: once every slot is used the newest slot is reused and the older ones stay. + [Fact] + public void Update_EveryKeptSlotUsed_ReusesTheNewestSlot() + { + using var temp = new TempDirectory(); + File.WriteAllText(temp.File("settings.json.corrupt"), "kept 1"); + for(int slot = 2; slot <= AppSettingsStore.MaximumKeptAside; slot++) + { + string number = slot.ToString(System.Globalization.CultureInfo.InvariantCulture); + File.WriteAllText(temp.File("settings.json.corrupt-" + number), "kept " + number); + } + File.WriteAllText(temp.File("settings.json"), "newest invalid"); + Assert.NotNull(new AppSettingsStore(temp.File("settings.json")).Update(settings => settings)); + Assert.Equal("kept 1", File.ReadAllText(temp.File("settings.json.corrupt"))); + Assert.Equal("kept 8", File.ReadAllText(temp.File("settings.json.corrupt-8"))); + Assert.Equal("newest invalid", File.ReadAllText(temp.File("settings.json.corrupt-9"))); + Assert.False(File.Exists(temp.File("settings.json.corrupt-10"))); + Assert.Equal(9, AppSettingsStore.MaximumKeptAside); + } + + + + /// When the save over invalid content fails, the invalid file is still in place: it is never moved away first. + [Fact] + public void Update_InvalidContentAndFailedSave_LeavesTheInvalidFileInPlace() + { + using var temp = new TempDirectory(); + File.WriteAllText(temp.File("settings.json"), "not json"); + var faults = new List(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault), _ => { }); + using(new FileStream(temp.File("settings.json"), FileMode.Open, FileAccess.Read, FileShare.Read)) + { + Assert.Null(store.Update(settings => settings)); + } + Assert.Equal([SettingsFault.Corrupt, SettingsFault.WriteFailed], faults); + Assert.Equal("not json", File.ReadAllText(temp.File("settings.json"))); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); + } + + + + /// A change that throws leaves the file, the lock and the folder as they were; the next save works. + [Fact] + public void Update_ChangeThrows_LeavesTheFileAndReleasesTheLock() + { + using var temp = new TempDirectory(); + File.WriteAllText(temp.File("settings.json"), "not json"); + var store = new AppSettingsStore(temp.File("settings.json")); + Assert.Throws(() => store.Update(_ => throw new InvalidOperationException("caller bug"))); + Assert.Equal("not json", File.ReadAllText(temp.File("settings.json"))); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); + Assert.Throws(() => store.Update(null!)); + Assert.NotNull(store.Update(settings => settings)); + } + + + + /// Reading for display holds nothing afterwards: another program can replace or rewrite the file at once. + [Fact] + public void Load_AfterReading_ReleasesTheFile() + { + using var temp = new TempDirectory(); + Seed(temp); + var store = new AppSettingsStore(temp.File("settings.json")); + Assert.Equal("Nord", store.Load().DocumentTheme); + using(new FileStream(temp.File("settings.json"), FileMode.Open, FileAccess.ReadWrite, FileShare.None)) + { + Assert.Equal("Catppuccin Mocha", new AppSettingsStore(temp.File("settings.json"), wait: _ => { }).Load().DocumentTheme); + } + Assert.Equal("Nord", store.Load().DocumentTheme); + } + + + + /// + /// Between judging the stored content and swapping in the new file, no other program can + /// rewrite it in place: the content that was read is the content that is replaced. + /// + [Fact] + public void Update_WhileTheChangeRuns_TheStoredFileCannotBeRewrittenInPlace() + { + using var temp = new TempDirectory(); + Seed(temp); + var store = new AppSettingsStore(temp.File("settings.json")); + bool refused = false; + Assert.NotNull(store.Update(settings => + { + refused = Record.Exception(() => new FileStream(temp.File("settings.json"), FileMode.Open, FileAccess.Write, FileShare.ReadWrite | FileShare.Delete).Dispose()) is IOException; + settings.Theme = "Light"; + return settings; + })); + Assert.True(refused); + Assert.Equal("Light", store.Load().Theme); + Assert.Equal("Nord", store.Load().DocumentTheme); + } + + + + /// A file that appears after this instance found none was written by someone else and is not overwritten. + [Fact] + public void Update_FileAppearsAfterTheRead_IsNotOverwritten() + { + using var temp = new TempDirectory(); + var faults = new List(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault)); + Assert.Null(store.Update(settings => + { + File.WriteAllText(temp.File("settings.json"), """{"DocumentTheme":"Dracula"}"""); + settings.Theme = "Light"; + return settings; + })); + Assert.Equal([SettingsFault.WriteFailed], faults); + Assert.Equal("""{"DocumentTheme":"Dracula"}""", File.ReadAllText(temp.File("settings.json"))); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); + AppSettings? saved = store.Update(settings => + { + settings.Theme = "Light"; + return settings; + }); + Assert.NotNull(saved); + Assert.Equal("Dracula", saved.DocumentTheme); + } + + + + /// A file is read up to the size bound and not loaded past it, even when it would parse. + [Theory] + [InlineData(0, true)] + [InlineData(1, false)] + public void Load_FileAtAndPastTheSizeBound_IsBoundedExactly(int extraBytes, bool loaded) + { + using var temp = new TempDirectory(); + byte[] padded = new byte[AppSettingsStore.MaximumBytes + extraBytes]; Array.Fill(padded, (byte)' '); - valid.CopyTo(padded, 0); + """{"DocumentTheme":"Nord"}"""u8.CopyTo(padded); File.WriteAllBytes(temp.File("settings.json"), padded); var faults = new List(); - Assert.Equal("Catppuccin Mocha", new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault)).Load().DocumentTheme); - Assert.Equal([SettingsFault.Corrupt], faults); + Assert.Equal(loaded ? "Nord" : "Catppuccin Mocha", new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault)).Load().DocumentTheme); + Assert.Equal(loaded ? [] : [SettingsFault.Corrupt], faults); } @@ -255,13 +662,51 @@ public class AppSettingsStoreTests + /// + /// Mutual exclusion, shown without threads: while one instance is inside its change, a second + /// instance's save is refused for every attempt, and the first instance's change is what ends + /// up stored. + /// + [Fact] + public void Update_WhileAnotherInstanceIsInsideItsChange_IsRefusedAndTheFirstChangeIsSaved() + { + using var temp = new TempDirectory(); + Seed(temp); + int waits = 0; + var faults = new List(); + var first = new AppSettingsStore(temp.File("settings.json")); + var second = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault), _ => waits++); + bool secondChanged = false; + AppSettings? saved = first.Update(settings => + { + Assert.Null(second.Update(other => + { + secondChanged = true; + other.Theme = "Light"; + return other; + })); + settings.DocumentTheme = "Dracula"; + return settings; + }); + Assert.NotNull(saved); + Assert.False(secondChanged); + Assert.Equal(AppSettingsStore.MaximumAttempts - 1, waits); + Assert.Equal([SettingsFault.Busy], faults); + AppSettings loaded = second.Load(); + Assert.Equal("Dracula", loaded.DocumentTheme); + Assert.Equal("Dark", loaded.Theme); + Assert.NotNull(second.Update(other => other)); + } + + + /// A lock released while this instance waits lets the change through. [Fact] public void Update_LockReleasedDuringWait_SavesTheChange() { using var temp = new TempDirectory(); Seed(temp); - FileStream other = new(temp.File("settings.json.lock"), FileMode.Open, FileAccess.ReadWrite, FileShare.None); + using var other = new FileStream(temp.File("settings.json.lock"), FileMode.Open, FileAccess.ReadWrite, FileShare.None); var store = new AppSettingsStore(temp.File("settings.json"), wait: _ => other.Dispose()); AppSettings? saved = store.Update(settings => { @@ -275,9 +720,32 @@ public class AppSettingsStoreTests - /// A handle that forbids replacing the file fails the save cleanly: the previous file stays and no temporary file is left. + /// A reader that allows the file to be replaced, as this store's own reads do, does not stop a save. [Fact] - public void Update_ReplaceBlocked_LeavesPreviousFileAndNoTemporaryFile() + public void Update_FileOpenByAReaderThatAllowsReplacement_StillSaves() + { + using var temp = new TempDirectory(); + Seed(temp); + int waits = 0; + var store = new AppSettingsStore(temp.File("settings.json"), wait: _ => waits++); + using(new FileStream(temp.File("settings.json"), FileMode.Open, FileAccess.Read, FileShare.Read | FileShare.Delete)) + { + Assert.NotNull(store.Update(settings => + { + settings.Theme = "Light"; + return settings; + })); + } + Assert.Equal(0, waits); + Assert.Equal("Light", store.Load().Theme); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); + } + + + + /// A handle that forbids replacing the file fails the save cleanly: the stored file stays and no temporary file is left. + [Fact] + public void Update_ReplaceBlocked_LeavesTheStoredFileAndNoTemporaryFile() { using var temp = new TempDirectory(); Seed(temp); @@ -301,45 +769,106 @@ public class AppSettingsStoreTests + /// A read-only settings file fails the save at once instead of stalling the window for every attempt. + [Fact] + public void Update_ReadOnlyFile_FailsWithoutWaiting() + { + using var temp = new TempDirectory(); + Seed(temp); + byte[] before = File.ReadAllBytes(temp.File("settings.json")); + int waits = 0; + var faults = new List<(SettingsFault Fault, Exception? Exception)>(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, exception) => faults.Add((fault, exception)), _ => waits++); + File.SetAttributes(temp.File("settings.json"), FileAttributes.ReadOnly); + try + { + Assert.Null(store.Update(settings => + { + settings.Theme = "Light"; + return settings; + })); + Assert.Equal("Nord", store.Load().DocumentTheme); + } + finally + { + File.SetAttributes(temp.File("settings.json"), FileAttributes.Normal); + } + Assert.Equal(0, waits); + (SettingsFault fault, Exception? exception) = Assert.Single(faults); + Assert.Equal(SettingsFault.WriteFailed, fault); + Assert.IsType(exception); + Assert.Equal(before, File.ReadAllBytes(temp.File("settings.json"))); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); + } + + + + /// A lock that can never be taken, here a folder with the lock's name, is a failed write reported at once, not a busy lock. + [Fact] + public void Update_LockCannotBeOpened_FailsWithoutWaiting() + { + using var temp = new TempDirectory(); + Directory.CreateDirectory(temp.File("settings.json.lock")); + int waits = 0; + var faults = new List(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault), _ => waits++); + Assert.Null(store.Update(settings => settings)); + Assert.Equal(0, waits); + Assert.Equal([SettingsFault.WriteFailed], faults); + Assert.Empty(FileNames(temp.Path)); + } + + + /// A location that cannot hold a settings folder reports a failed write instead of throwing into the caller. [Fact] public void Update_FolderCannotBeCreated_ReportsWriteFailed() { using var temp = new TempDirectory(); File.WriteAllText(temp.File("blocker"), "a file where the folder should be"); + int waits = 0; var faults = new List(); - var store = new AppSettingsStore(Path.Combine(temp.File("blocker"), "settings.json"), (fault, _) => faults.Add(fault), _ => { }); + var store = new AppSettingsStore(Path.Combine(temp.File("blocker"), "settings.json"), (fault, _) => faults.Add(fault), _ => waits++); Assert.Null(store.Update(settings => settings)); Assert.Equal([SettingsFault.WriteFailed], faults); + Assert.Equal(0, waits); Assert.Equal("Catppuccin Mocha", store.Load().DocumentTheme); } - /// Temporary files left by a writer that died before its rename are removed by the next save; one still in use is left alone. + /// + /// Only this store's own abandoned temporary files are removed by the next save. One still in + /// use, the fixed-name temporary file an older release writes, and anything else ending in + /// .tmp are left alone. + /// [Fact] - public void Update_AbandonedTemporaryFiles_AreRemovedUnlessInUse() + public void Update_AbandonedTemporaryFiles_RemovesOnlyItsOwn() { using var temp = new TempDirectory(); Seed(temp); - File.WriteAllText(temp.File("settings.json.tmp"), "left by the previous store"); - File.WriteAllText(temp.File("settings.json.0123.tmp"), "left by a writer that died"); - File.WriteAllText(temp.File("settings.json.busy.tmp"), "still open elsewhere"); - File.WriteAllText(temp.File("notes.tmp"), "not ours"); + string own = "settings.json." + Guid.NewGuid().ToString("N") + ".tmp"; + string inUse = "settings.json." + Guid.NewGuid().ToString("N") + ".tmp"; + string notHex = "settings.json." + new string('z', 32) + ".tmp"; + foreach(string name in new[] { own, inUse, notHex, "settings.json.tmp", "settings.json.bak.tmp", "settings.json.0123.tmp", "notes.tmp" }) + { + File.WriteAllText(temp.File(name), "left behind"); + } var store = new AppSettingsStore(temp.File("settings.json")); - using(new FileStream(temp.File("settings.json.busy.tmp"), FileMode.Open, FileAccess.Read, FileShare.None)) + using(new FileStream(temp.File(inUse), FileMode.Open, FileAccess.Read, FileShare.None)) { Assert.NotNull(store.Update(settings => settings)); } - Assert.Equal(["notes.tmp", "settings.json", "settings.json.busy.tmp", "settings.json.lock"], FileNames(temp.Path)); + string[] expected = [inUse, notHex, "notes.tmp", "settings.json", "settings.json.0123.tmp", "settings.json.bak.tmp", "settings.json.lock", "settings.json.tmp"]; + Assert.Equal(expected.Order(StringComparer.Ordinal), FileNames(temp.Path), StringComparer.Ordinal); } /// - /// The lost-update regression: instances that each record their own documents at the same - /// time keep every one. A refused save is retried, as a user would; a save that reported - /// success must never be erased by another instance. + /// Instances that each record their own documents at the same time keep every one. A refused + /// save is retried a bounded number of times, as a user would; a save that reported success + /// must never be erased by another instance. /// [Fact] public void Update_ConcurrentInstances_KeepEveryChange() @@ -348,32 +877,18 @@ public class AppSettingsStoreTests const int Instances = 4; const int Each = 5; using var start = new ManualResetEventSlim(); + var failures = new System.Collections.Concurrent.ConcurrentQueue(); var threads = new List(); for(int instance = 0; instance < Instances; instance++) { int id = instance; - var thread = new Thread(() => - { - var store = new AppSettingsStore(temp.File("settings.json")); - start.Wait(); - for(int index = 0; index < Each; index++) - { - string path = $@"C:\docs\instance{id}-{index}.md"; - while(store.Update(settings => - { - settings.RecentFiles = Documents.RecentDocuments.Record(settings.RecentFiles, path); - return settings; - }) is null) - { - Thread.Yield(); - } - } - }); + var thread = new Thread(() => RecordDocuments(temp.File("settings.json"), id, Each, start, failures)); thread.Start(); threads.Add(thread); } start.Set(); threads.ForEach(thread => thread.Join()); + Assert.Empty(failures); string[] recorded = new AppSettingsStore(temp.File("settings.json")).Load().RecentFiles; Assert.Equal(Instances * Each, recorded.Length); Assert.Equal(Instances * Each, recorded.Distinct(StringComparer.Ordinal).Count()); @@ -382,6 +897,39 @@ public class AppSettingsStoreTests + /// One instance's work: a failure is recorded for the test to assert, never thrown on a bare thread. + private static void RecordDocuments(string file, int id, int count, ManualResetEventSlim start, System.Collections.Concurrent.ConcurrentQueue failures) + { + try + { + var store = new AppSettingsStore(file); + start.Wait(); + for(int index = 0; index < count; index++) + { + string path = $@"C:\docs\instance{id}-{index}.md"; + int refusals = 0; + while(store.Update(settings => + { + settings.RecentFiles = Documents.RecentDocuments.Record(settings.RecentFiles, path); + return settings; + }) is null) + { + if(++refusals == 50) + { + failures.Enqueue(path + " was refused 50 times"); + return; + } + } + } + } + catch(Exception exception) when(exception is IOException or UnauthorizedAccessException or InvalidOperationException) + { + failures.Enqueue(exception.ToString()); + } + } + + + /// A profile with non-default values in several unrelated fields, so a wipe is visible. private static void Seed(TempDirectory temp) {