diff --git a/docs/adr/0004-native-document-windows-and-private-recents.md b/docs/adr/0004-native-document-windows-and-private-recents.md index d4c29fb..06c6577 100644 --- a/docs/adr/0004-native-document-windows-and-private-recents.md +++ b/docs/adr/0004-native-document-windows-and-private-recents.md @@ -30,7 +30,7 @@ and isolated boundary evidence in ADR-0005. Explorer and installed lifecycle acceptance remain required; literal process arguments alone are not association proof. -Store at most20 successful absolute paths in the private AppKit settings file, +Store at most20 successful absolute paths in the private settings file, deduplicated with Windows path comparison. Update only after a parsed snapshot commits. Clear history removes entries without deleting files or closing readers. Keep paths out of activity logs/telemetry. Each preference save merges only fields diff --git a/docs/security/threat-model.md b/docs/security/threat-model.md index 37506ff..7bbc71a 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 / 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 | | 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 1b4c4bf..25e0d14 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -73,6 +73,7 @@ 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. 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/AppIdentity.cs b/src/MarkdownViewr.Windows.Core/AppIdentity.cs index c284366..9ef5b27 100644 --- a/src/MarkdownViewr.Windows.Core/AppIdentity.cs +++ b/src/MarkdownViewr.Windows.Core/AppIdentity.cs @@ -5,7 +5,6 @@ using KofTwentyTwo.AppKit; using KofTwentyTwo.AppKit.Logging; -using KofTwentyTwo.AppKit.Settings; namespace MarkdownViewr.Windows.Core; @@ -73,7 +72,7 @@ public static class AppIdentity /// Loads and saves at . - public static SettingsStore SettingsStore { get; } = new(Paths.SettingsFile, AppSettingsContext.Default.AppSettings); + public static AppSettingsStore SettingsStore { get; } = new(Paths.SettingsFile); diff --git a/src/MarkdownViewr.Windows.Core/AppSettingsStore.cs b/src/MarkdownViewr.Windows.Core/AppSettingsStore.cs new file mode 100644 index 0000000..ee2645a --- /dev/null +++ b/src/MarkdownViewr.Windows.Core/AppSettingsStore.cs @@ -0,0 +1,250 @@ +/* + * Copyright (c) 2026 James Maes + * SPDX-License-Identifier: MIT + */ + +using System.Text; +using System.Text.Json; + + +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. +/// +public sealed class AppSettingsStore +{ + /// Attempts for the lock, a read, and the final replace 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); + private readonly Action _report; + private readonly Action _wait; + + + + /// Creates the store; faults go to , and tests supply to count attempts instead of sleeping. + public AppSettingsStore(string path, Action? report = null, Action? wait = null) + { + ArgumentException.ThrowIfNullOrWhiteSpace(path); + Path = System.IO.Path.GetFullPath(path); + _report = report ?? ((_, _) => { }); + _wait = wait ?? Thread.Sleep; + } + + + + /// The settings file. + public string Path { get; } + + + + /// 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); + return settings; + } + + + + /// + /// 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. + /// + public AppSettings? Update(Func change) + { + ArgumentNullException.ThrowIfNull(change); + try + { + Directory.CreateDirectory(System.IO.Path.GetDirectoryName(Path)!); + using FileStream? gate = Acquire(); + if(gate is null) + { + return null; + } + SettingsRead read = TryRead(out AppSettings current); + if(read == SettingsRead.Unreadable) + { + return null; + } + 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) + { + _report(SettingsFault.WriteFailed, exception); + return null; + } + } + + + + /// Holds an exclusive handle beside the settings file; the operating system releases it if the process dies. + private FileStream? Acquire() + { + try + { + FileStream? gate = null; + Retry(() => gate = new FileStream(Path + ".lock", FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None)); + return gate; + } + catch(IOException exception) when(IsTransient(exception)) + { + _report(SettingsFault.Busy, exception); + return null; + } + } + + + + /// 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) + { + settings = new AppSettings(); + 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; + } + } + 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); + result = SettingsRead.Unreadable; + } + settings.Sanitize(); + return result; + } + + + + /// Reads without blocking a concurrent replace; an oversized file reads as invalid content instead of exhausting memory. + private string? ReadText() + { + using var stream = new FileStream(Path, FileMode.Open, FileAccess.Read, FileShare.ReadWrite | FileShare.Delete); + if(stream.Length > MaximumBytes) + { + return null; + } + using var reader = new StreamReader(stream, Encoding.UTF8, detectEncodingFromByteOrderMarks: true); + return reader.ReadToEnd(); + } + + + + /// 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) + { + string temporary = Path + "." + Guid.NewGuid().ToString("N") + ".tmp"; + bool replaced = false; + try + { + using(var stream = new FileStream(temporary, FileMode.CreateNew, FileAccess.Write, FileShare.None)) + { + stream.Write(content); + stream.Flush(flushToDisk: true); + } + Retry(() => File.Move(temporary, Path, overwrite: true)); + replaced = true; + } + finally + { + if(!replaced) + { + TryDelete(temporary); + } + } + } + + + + /// A writer that died before its rename left a temporary file; the lock holder is the only writer, so every one found is abandoned. + private void RemoveAbandonedTemporaryFiles() + { + foreach(string file in Directory.EnumerateFiles(System.IO.Path.GetDirectoryName(Path)!, System.IO.Path.GetFileName(Path) + "*.tmp")) + { + TryDelete(file); + } + } + + + + /// Repeats an operation only for a sharing violation or a pending delete, a bounded number of times; the last failure is thrown. + private void Retry(Action operation) + { + for(int attempt = 1; ; attempt++) + { + try + { + operation(); + return; + } + catch(Exception exception) when(attempt < MaximumAttempts && IsTransient(exception)) + { + _wait(s_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; + } + + + + /// Cleanup only: a temporary file that cannot be removed now is removed by the next successful save. + private static void TryDelete(string path) + { + try + { + File.Delete(path); + } + catch(Exception exception) when(exception is IOException or UnauthorizedAccessException) + { + // The outcome of the save is already decided. + } + } +} diff --git a/src/MarkdownViewr.Windows.Core/SettingsFault.cs b/src/MarkdownViewr.Windows.Core/SettingsFault.cs new file mode 100644 index 0000000..32a7c5f --- /dev/null +++ b/src/MarkdownViewr.Windows.Core/SettingsFault.cs @@ -0,0 +1,23 @@ +/* + * Copyright (c) 2026 James Maes + * SPDX-License-Identifier: MIT + */ + + +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. + Corrupt, + + /// The file could not be read after bounded retries; 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. + Busy, + + /// The change could not be written; the previous file remains in place. + WriteFailed, +} diff --git a/src/MarkdownViewr.Windows.Core/SettingsRead.cs b/src/MarkdownViewr.Windows.Core/SettingsRead.cs new file mode 100644 index 0000000..d68d9a3 --- /dev/null +++ b/src/MarkdownViewr.Windows.Core/SettingsRead.cs @@ -0,0 +1,23 @@ +/* + * Copyright (c) 2026 James Maes + * SPDX-License-Identifier: MIT + */ + + +namespace MarkdownViewr.Windows.Core; + +/// What reading the settings file found; only forbids writing over it. +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. + Corrupt, + + /// The content is unknown, so it must not be replaced. + Unreadable, +} diff --git a/src/MarkdownViewr.Windows/App.Documents.cs b/src/MarkdownViewr.Windows/App.Documents.cs index 2c47571..add9df7 100644 --- a/src/MarkdownViewr.Windows/App.Documents.cs +++ b/src/MarkdownViewr.Windows/App.Documents.cs @@ -107,12 +107,14 @@ public partial class App - /// Records a successfully committed path through the latest store snapshot, then refreshes every recent menu. + /// Records a successfully committed path in the latest stored history under the settings lock, then refreshes every recent menu. internal bool RecordRecentDocument(string path) { - Core.AppSettings settings = AppIdentity.SettingsStore.Load(); - settings.RecentFiles = Core.Documents.RecentDocuments.Record(settings.RecentFiles, path); - bool saved = AppIdentity.SettingsStore.Save(settings); + bool saved = AppIdentity.SettingsStore.Update(settings => + { + settings.RecentFiles = Core.Documents.RecentDocuments.Record(settings.RecentFiles, path); + return settings; + }) is not null; RefreshRecentMenus(); return saved; } @@ -122,9 +124,11 @@ public partial class App /// Clears only the local history, leaving open documents, reading preferences and files intact. internal bool ClearRecentDocuments() { - Core.AppSettings settings = AppIdentity.SettingsStore.Load(); - settings.RecentFiles = []; - bool saved = AppIdentity.SettingsStore.Save(settings); + bool saved = AppIdentity.SettingsStore.Update(settings => + { + settings.RecentFiles = []; + return settings; + }) is not null; RefreshRecentMenus(); return saved; } diff --git a/src/MarkdownViewr.Windows/AppIdentity.cs b/src/MarkdownViewr.Windows/AppIdentity.cs index 4f472f8..0aff32b 100644 --- a/src/MarkdownViewr.Windows/AppIdentity.cs +++ b/src/MarkdownViewr.Windows/AppIdentity.cs @@ -5,7 +5,6 @@ using KofTwentyTwo.AppKit; using KofTwentyTwo.AppKit.Logging; -using KofTwentyTwo.AppKit.Settings; using Velopack.Locators; @@ -31,8 +30,8 @@ internal static class AppIdentity - /// Each edition owns its own persisted preferences and recent document list. - public static SettingsStore SettingsStore { get; } = new(Paths.SettingsFile, AppSettingsContext.Default.AppSettings); + /// Each edition owns its own persisted preferences and recent document list; faults are logged by kind, never with a path or message text. + public static Core.AppSettingsStore SettingsStore { get; } = new(Paths.SettingsFile, ReportSettingsFault); @@ -56,6 +55,12 @@ internal static class AppIdentity + /// A settings fault is an operational event: the kind, exception type and HRESULT identify it without disclosing the profile. + private static void ReportSettingsFault(Core.SettingsFault fault, Exception? exception) + => App.Log.Warning(FormattableString.Invariant($"Settings file fault: {fault}; exception={exception?.GetType().Name ?? "none"}; HRESULT=0x{exception?.HResult ?? 0:X8}.")); + + + /// Daily activity files use the edition's own path and identity prefix. public static IActivityLog CreateLog() => FileActivityLog.ForApp(Info, Paths); } diff --git a/src/MarkdownViewr.Windows/MainWindow.Editor.cs b/src/MarkdownViewr.Windows/MainWindow.Editor.cs index aeee634..c8752c0 100644 --- a/src/MarkdownViewr.Windows/MainWindow.Editor.cs +++ b/src/MarkdownViewr.Windows/MainWindow.Editor.cs @@ -48,8 +48,8 @@ public sealed partial class MainWindow Core.AppSettings draft = Core.SettingsChanges.Copy(before); draft.ConfiguredEditors = choices; draft.ExternalEditor = choices.FirstOrDefault()?.Executable ?? ""; - Core.AppSettings merged = Core.SettingsChanges.Merge(AppIdentity.SettingsStore.Load(), before, draft); - if(!AppIdentity.SettingsStore.Save(merged)) + Core.AppSettings? merged = AppIdentity.SettingsStore.Update(current => Core.SettingsChanges.Merge(current, before, draft)); + if(merged is null) { StatusText.Text = "Could not save editor configuration."; return; diff --git a/src/MarkdownViewr.Windows/MainWindow.PrintSettings.cs b/src/MarkdownViewr.Windows/MainWindow.PrintSettings.cs index 0f10fa9..f7d6bde 100644 --- a/src/MarkdownViewr.Windows/MainWindow.PrintSettings.cs +++ b/src/MarkdownViewr.Windows/MainWindow.PrintSettings.cs @@ -19,9 +19,12 @@ public sealed partial class MainWindow { return false; } - Core.AppSettings settings = AppIdentity.SettingsStore.Load(); - settings.Print = options.Sanitize(); - if(!AppIdentity.SettingsStore.Save(settings)) + Core.AppSettings? settings = AppIdentity.SettingsStore.Update(current => + { + current.Print = options.Sanitize(); + return current; + }); + if(settings is null) { return false; } diff --git a/src/MarkdownViewr.Windows/MainWindow.Settings.cs b/src/MarkdownViewr.Windows/MainWindow.Settings.cs index 97dea65..7596d26 100644 --- a/src/MarkdownViewr.Windows/MainWindow.Settings.cs +++ b/src/MarkdownViewr.Windows/MainWindow.Settings.cs @@ -61,8 +61,8 @@ public sealed partial class MainWindow draft.ContentWidthEnabled = appearance.LimitWidth; draft.ContentWidthPx = appearance.WidthPixels; draft.GlobalCss = appearance.GlobalCss; - Core.AppSettings merged = Core.SettingsChanges.Merge(AppIdentity.SettingsStore.Load(), before, draft); - if(!AppIdentity.SettingsStore.Save(merged)) + Core.AppSettings? merged = AppIdentity.SettingsStore.Update(current => Core.SettingsChanges.Merge(current, before, draft)); + if(merged is null) { return false; } @@ -79,14 +79,16 @@ public sealed partial class MainWindow { return false; } - Core.AppSettings settings = AppIdentity.SettingsStore.Load(); - settings.ShowSplashScreen = showSplash; - settings.SplashMilliseconds = milliseconds; - settings.CheckForUpdatesOnStartup = checkUpdates; - settings.ShowOutlineByDefault = showOutline; - settings.ShowSourceByDefault = showSource; - settings.Sanitize(); - if(!AppIdentity.SettingsStore.Save(settings)) + Core.AppSettings? settings = AppIdentity.SettingsStore.Update(current => + { + current.ShowSplashScreen = showSplash; + current.SplashMilliseconds = milliseconds; + current.CheckForUpdatesOnStartup = checkUpdates; + current.ShowOutlineByDefault = showOutline; + current.ShowSourceByDefault = showSource; + return current; + }); + if(settings is null) { return false; } @@ -120,10 +122,13 @@ public sealed partial class MainWindow { return false; } - Core.AppSettings settings = AppIdentity.SettingsStore.Load(); - settings.WrapOutlineEntries = wrap; - settings.ShowOutlineDepthMarkers = depthMarkers; - if(!AppIdentity.SettingsStore.Save(settings)) + Core.AppSettings? settings = AppIdentity.SettingsStore.Update(current => + { + current.WrapOutlineEntries = wrap; + current.ShowOutlineDepthMarkers = depthMarkers; + return current; + }); + if(settings is null) { return false; } diff --git a/src/MarkdownViewr.Windows/MainWindow.ThemeManagement.cs b/src/MarkdownViewr.Windows/MainWindow.ThemeManagement.cs index db224e7..b48d327 100644 --- a/src/MarkdownViewr.Windows/MainWindow.ThemeManagement.cs +++ b/src/MarkdownViewr.Windows/MainWindow.ThemeManagement.cs @@ -56,8 +56,8 @@ public sealed partial class MainWindow /// Failed persistence does not apply an unsaved theme draft; sibling fields untouched by this edit retain their latest values. private async Task CommitThemeDraftAsync(Core.AppSettings before, Core.AppSettings edited) { - Core.AppSettings merged = Core.SettingsChanges.Merge(AppIdentity.SettingsStore.Load(), before, edited); - if(!AppIdentity.SettingsStore.Save(merged)) + Core.AppSettings? merged = AppIdentity.SettingsStore.Update(current => Core.SettingsChanges.Merge(current, before, edited)); + if(merged is null) { StatusText.Text = "Could not save document themes."; return false; diff --git a/src/MarkdownViewr.Windows/MainWindow.xaml.cs b/src/MarkdownViewr.Windows/MainWindow.xaml.cs index e26f58f..c8af6c1 100644 --- a/src/MarkdownViewr.Windows/MainWindow.xaml.cs +++ b/src/MarkdownViewr.Windows/MainWindow.xaml.cs @@ -522,8 +522,7 @@ public sealed partial class MainWindow : Window, IDisposable /// Saves the settings and reports a failed save in the status line. private void Save() { - Core.AppSettings merged = Core.SettingsChanges.Merge(AppIdentity.SettingsStore.Load(), _lastSavedSettings, _settings); - if(AppIdentity.SettingsStore.Save(merged)) + if(AppIdentity.SettingsStore.Update(current => Core.SettingsChanges.Merge(current, _lastSavedSettings, _settings)) is not null) { _lastSavedSettings = Core.SettingsChanges.Copy(_settings); } diff --git a/tests/MarkdownViewr.Windows.Core.Tests/AppIdentityTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/AppIdentityTests.cs index fe5ba41..5feb21f 100644 --- a/tests/MarkdownViewr.Windows.Core.Tests/AppIdentityTests.cs +++ b/tests/MarkdownViewr.Windows.Core.Tests/AppIdentityTests.cs @@ -90,8 +90,8 @@ public class AppSettingsTests public void SaveAndLoad_KeepsTheTheme() { using var temp = new TempDirectory(); - var store = new KofTwentyTwo.AppKit.Settings.SettingsStore(temp.File("settings.json"), AppSettingsContext.Default.AppSettings); - Assert.True(store.Save(new AppSettings { Theme = "Dark", DocumentTheme = "GitHub Light", Frontmatter = "Metadata", UserThemes = [Themes.ThemeCatalog.Find("GitHub Light").Json] })); + var store = new AppSettingsStore(temp.File("settings.json")); + Assert.NotNull(store.Update(_ => new AppSettings { Theme = "Dark", DocumentTheme = "GitHub Light", Frontmatter = "Metadata", UserThemes = [Themes.ThemeCatalog.Find("GitHub Light").Json] })); Assert.Equal("Dark", store.Load().Theme); Assert.Equal("GitHub Light", store.Load().DocumentTheme); Assert.Equal(Documents.FrontmatterMode.Metadata, store.Load().FrontmatterKind); diff --git a/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs new file mode 100644 index 0000000..dc60af4 --- /dev/null +++ b/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs @@ -0,0 +1,403 @@ +/* + * Copyright (c) 2026 James Maes + * SPDX-License-Identifier: MIT + */ + +using System.Text; + + +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. +/// +public class AppSettingsStoreTests +{ + private const string KeptPath = @"C:\docs\keep-me.md"; + + + + /// A first run has no file: defaults apply and reading creates nothing. + [Fact] + public void Load_MissingFile_ReturnsDefaultsWithoutCreatingAnything() + { + using var temp = new TempDirectory(); + var faults = new List(); + var store = new AppSettingsStore(Path.Combine(temp.Path, "profile", "settings.json"), (fault, _) => faults.Add(fault)); + Assert.Equal("Catppuccin Mocha", store.Load().DocumentTheme); + Assert.Empty(faults); + Assert.False(Directory.Exists(Path.Combine(temp.Path, "profile"))); + } + + + + /// The first save creates the folder and file, and returns the sanitized settings that were written. + [Fact] + public void Update_MissingFolder_CreatesTheFileAndReturnsSanitizedSettings() + { + using var temp = new TempDirectory(); + var store = new AppSettingsStore(Path.Combine(temp.Path, "profile", "settings.json")); + AppSettings? saved = store.Update(settings => + { + settings.Theme = "Dark"; + settings.OutlineDepth = 99; + return settings; + }); + Assert.NotNull(saved); + Assert.Equal(6, saved.OutlineDepth); + AppSettings loaded = store.Load(); + Assert.Equal("Dark", loaded.Theme); + Assert.Equal(6, loaded.OutlineDepth); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(Path.Combine(temp.Path, "profile"))); + } + + + + /// A change is applied to what is stored now, not to what this instance read earlier. + [Fact] + public void Update_AnotherInstanceSavedFirst_ChangesTheLatestSettings() + { + using var temp = new TempDirectory(); + var first = new AppSettingsStore(temp.File("settings.json")); + var second = new AppSettingsStore(temp.File("settings.json")); + _ = first.Load(); + Assert.NotNull(second.Update(settings => + { + settings.GlobalCss = "body { color: red }"; + return settings; + })); + Assert.NotNull(first.Update(settings => + { + settings.Theme = "Light"; + return settings; + })); + AppSettings loaded = second.Load(); + Assert.Equal("body { color: red }", loaded.GlobalCss); + Assert.Equal("Light", loaded.Theme); + } + + + + /// 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)] + [InlineData(true)] + public void Load_ByteOrderMark_ReadsTheStoredValues(bool utf16) + { + using var temp = new TempDirectory(); + Encoding marked = utf16 ? Encoding.Unicode : new UTF8Encoding(encoderShouldEmitUTF8Identifier: true); + File.WriteAllText(temp.File("settings.json"), """{"Theme":"Dark","DocumentTheme":"Nord"}""", marked); + var faults = new List(); + AppSettings loaded = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault)).Load(); + Assert.Equal("Dark", loaded.Theme); + Assert.Equal("Nord", loaded.DocumentTheme); + Assert.Empty(faults); + } + + + + /// 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; + var faults = new List(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault), _ => + { + if(++waits == 3) + { + exclusive.Dispose(); + } + }); + AppSettings loaded = store.Load(); + Assert.Equal(3, waits); + Assert.Equal("Nord", loaded.DocumentTheme); + Assert.Equal([KeptPath], loaded.RecentFiles); + Assert.Empty(faults); + } + + + + /// A file that stays unreadable yields defaults for display and is reported; the attempts are bounded. + [Fact] + public void Load_FileLockedThroughout_ReturnsDefaultsAndReportsUnreadable() + { + using var temp = new TempDirectory(); + Seed(temp); + 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++); + using(new FileStream(temp.File("settings.json"), FileMode.Open, FileAccess.Read, FileShare.None)) + { + Assert.Equal("Catppuccin Mocha", store.Load().DocumentTheme); + } + Assert.Equal(AppSettingsStore.MaximumAttempts - 1, waits); + (SettingsFault fault, Exception? exception) = Assert.Single(faults); + Assert.Equal(SettingsFault.Unreadable, fault); + Assert.IsType(exception); + } + + + + /// + /// 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. + /// + [Fact] + public void Update_FileLockedThroughout_WritesNothingOverIt() + { + using var temp = new TempDirectory(); + Seed(temp); + byte[] before = File.ReadAllBytes(temp.File("settings.json")); + var faults = new List(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault), _ => { }); + bool changed = false; + using(new FileStream(temp.File("settings.json"), FileMode.Open, FileAccess.Read, FileShare.None)) + { + Assert.Null(store.Update(settings => + { + changed = true; + return settings; + })); + } + Assert.False(changed); + Assert.Equal([SettingsFault.Unreadable], faults); + Assert.Equal(before, File.ReadAllBytes(temp.File("settings.json"))); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); + AppSettings loaded = store.Load(); + Assert.Equal("Nord", loaded.DocumentTheme); + Assert.Equal("body { color: red }", loaded.GlobalCss); + Assert.Equal([KeptPath], loaded.RecentFiles); + } + + + + /// Only invalid content is corruption: reading it yields defaults, reports it, and leaves the file where it is. + [Theory] + [InlineData("")] + [InlineData("{")] + [InlineData("null")] + [InlineData("[1]")] + [InlineData("{\"OutlineDepth\":\"deep\"}")] + public void Load_InvalidContent_ReturnsDefaultsAndLeavesTheFile(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("Catppuccin Mocha", loaded.DocumentTheme); + Assert.Equal([SettingsFault.Corrupt], faults); + Assert.Equal(content, File.ReadAllText(temp.File("settings.json"))); + Assert.Equal(["settings.json"], FileNames(temp.Path)); + } + + + + /// Saving over invalid content keeps that content aside, so a hand edit gone wrong can still be recovered. + [Fact] + public void Update_InvalidContent_KeepsItAsideAndSavesTheChange() + { + using var temp = new TempDirectory(); + File.WriteAllText(temp.File("settings.json"), "{ \"Theme\": \"Dark\", "); + File.WriteAllText(temp.File("settings.json.corrupt"), "an older 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("Nord", store.Load().DocumentTheme); + Assert.Equal("System", store.Load().Theme); + } + + + + /// A file far larger than any settings is not loaded into memory, even when it would parse; it reads as invalid content. + [Fact] + public void Load_OversizedFile_ReadsAsInvalidContent() + { + using var temp = new TempDirectory(); + byte[] valid = Encoding.UTF8.GetBytes("""{"DocumentTheme":"Nord"}"""); + byte[] padded = new byte[AppSettingsStore.MaximumBytes + 1]; + Array.Fill(padded, (byte)' '); + valid.CopyTo(padded, 0); + 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); + } + + + + /// While another instance holds the lock for every attempt, the change is refused and the file is unchanged. + [Fact] + public void Update_LockHeldByAnotherInstance_ReportsBusyAndWritesNothing() + { + using var temp = new TempDirectory(); + Seed(temp); + byte[] before = File.ReadAllBytes(temp.File("settings.json")); + int waits = 0; + var faults = new List(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault), _ => waits++); + using(new FileStream(temp.File("settings.json.lock"), FileMode.Open, FileAccess.ReadWrite, FileShare.None)) + { + Assert.Null(store.Update(settings => settings)); + } + Assert.Equal(AppSettingsStore.MaximumAttempts - 1, waits); + Assert.Equal([SettingsFault.Busy], faults); + Assert.Equal(before, File.ReadAllBytes(temp.File("settings.json"))); + } + + + + /// 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); + var store = new AppSettingsStore(temp.File("settings.json"), wait: _ => other.Dispose()); + AppSettings? saved = store.Update(settings => + { + settings.Theme = "Light"; + return settings; + }); + Assert.NotNull(saved); + Assert.Equal("Light", store.Load().Theme); + Assert.Equal([KeptPath], store.Load().RecentFiles); + } + + + + /// A handle that forbids replacing the file fails the save cleanly: the previous file stays and no temporary file is left. + [Fact] + public void Update_ReplaceBlocked_LeavesPreviousFileAndNoTemporaryFile() + { + using var temp = new TempDirectory(); + Seed(temp); + byte[] before = File.ReadAllBytes(temp.File("settings.json")); + int waits = 0; + var faults = new List(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault), _ => waits++); + using(new FileStream(temp.File("settings.json"), FileMode.Open, FileAccess.Read, FileShare.Read)) + { + Assert.Null(store.Update(settings => + { + settings.Theme = "Light"; + return settings; + })); + } + Assert.Equal(AppSettingsStore.MaximumAttempts - 1, waits); + Assert.Equal([SettingsFault.WriteFailed], faults); + Assert.Equal(before, File.ReadAllBytes(temp.File("settings.json"))); + Assert.Equal(["settings.json", "settings.json.lock"], 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"); + var faults = new List(); + var store = new AppSettingsStore(Path.Combine(temp.File("blocker"), "settings.json"), (fault, _) => faults.Add(fault), _ => { }); + Assert.Null(store.Update(settings => settings)); + Assert.Equal([SettingsFault.WriteFailed], faults); + 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. + [Fact] + public void Update_AbandonedTemporaryFiles_AreRemovedUnlessInUse() + { + 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"); + var store = new AppSettingsStore(temp.File("settings.json")); + using(new FileStream(temp.File("settings.json.busy.tmp"), 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)); + } + + + + /// + /// 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. + /// + [Fact] + public void Update_ConcurrentInstances_KeepEveryChange() + { + using var temp = new TempDirectory(); + const int Instances = 4; + const int Each = 5; + using var start = new ManualResetEventSlim(); + 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(); + } + } + }); + thread.Start(); + threads.Add(thread); + } + start.Set(); + threads.ForEach(thread => thread.Join()); + 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()); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); + } + + + + /// A profile with non-default values in several unrelated fields, so a wipe is visible. + private static void Seed(TempDirectory temp) + { + Assert.NotNull(new AppSettingsStore(temp.File("settings.json")).Update(settings => + { + settings.Theme = "Dark"; + settings.DocumentTheme = "Nord"; + settings.GlobalCss = "body { color: red }"; + settings.RecentFiles = [KeptPath]; + return settings; + })); + } + + + + /// The folder's file names in ordinal order. + private static string[] FileNames(string folder) + => Directory.GetFiles(folder).Select(file => Path.GetFileName(file)!).Order(StringComparer.Ordinal).ToArray(); +} diff --git a/tests/MarkdownViewr.Windows.Core.Tests/EditorConfigurationTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/EditorConfigurationTests.cs index 0319464..2e09c1d 100644 --- a/tests/MarkdownViewr.Windows.Core.Tests/EditorConfigurationTests.cs +++ b/tests/MarkdownViewr.Windows.Core.Tests/EditorConfigurationTests.cs @@ -76,21 +76,24 @@ public sealed class EditorConfigurationTests - /// Atomic AppKit persistence retains named IDs, order and folder mode using the trimmed serializer. + /// Atomic persistence retains named IDs, order and folder mode using the trimmed serializer. [Fact] public void SaveAndLoad_EditorList_PreservesIdentityOrderAndMode() { using var temporary = new TempDirectory(); - var store = new KofTwentyTwo.AppKit.Settings.SettingsStore(temporary.File("settings.json"), AppSettingsContext.Default.AppSettings); + var store = new AppSettingsStore(temporary.File("settings.json")); var first = new EditorConfiguration(Guid.NewGuid(), "Folder editor", @"C:\Editor\folder.exe", true); var second = new EditorConfiguration(Guid.NewGuid(), "File editor", @"C:\Editor\file.exe"); - Assert.True(store.Save(new AppSettings { ConfiguredEditors = [first, second], ExternalEditor = @"C:\Other\old.exe" })); + Assert.NotNull(store.Update(_ => new AppSettings { ConfiguredEditors = [first, second], ExternalEditor = @"C:\Other\old.exe" })); AppSettings loaded = store.Load(); Assert.Equal(new[] { first, second }, loaded.ConfiguredEditors); Assert.Equal(first.Executable, loaded.ExternalEditor); - loaded.ConfiguredEditors = []; - loaded.ExternalEditor = ""; - Assert.True(store.Save(loaded)); + Assert.NotNull(store.Update(settings => + { + settings.ConfiguredEditors = []; + settings.ExternalEditor = ""; + return settings; + })); Assert.Empty(store.Load().ConfiguredEditors); }