From 6919d3ce4a6b04cafc2db9bed668ff504f394e4e Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 10 Nov 2026 16:09:55 -0600 Subject: [PATCH 1/5] fix(settings): keep stored preferences when the file is shared or unreadable A load that hit a sharing violation returned defaults, and the next save wrote them back over the stored themes, CSS, editors and recent files. Two processes over one data folder also erased each other's changes: each did load, modify, save with no lock and one shared temporary file name. AppSettingsStore applies one change to the latest stored settings under a lock file beside settings.json and replaces the file by rename. A file that cannot be read after bounded retries is never overwritten; only invalid content counts as corruption and is kept as settings.json.corrupt. A file over 16 MiB is not loaded, abandoned temporary files are removed, and faults are logged by kind, exception type and HRESULT only. Every save in the app now goes through Update. Tests use real exclusive handles and concurrent instances and count attempts instead of time. Closes #46 Co-Authored-By: Claude Fable 5.2 Signed-off-by: James Maes --- ...ve-document-windows-and-private-recents.md | 2 +- docs/security/threat-model.md | 2 +- docs/user-guide.md | 1 + src/MarkdownViewr.Windows.Core/AppIdentity.cs | 3 +- .../AppSettingsStore.cs | 250 +++++++++++ .../SettingsFault.cs | 23 + .../SettingsRead.cs | 23 + src/MarkdownViewr.Windows/App.Documents.cs | 18 +- src/MarkdownViewr.Windows/AppIdentity.cs | 11 +- .../MainWindow.Editor.cs | 4 +- .../MainWindow.PrintSettings.cs | 9 +- .../MainWindow.Settings.cs | 33 +- .../MainWindow.ThemeManagement.cs | 4 +- src/MarkdownViewr.Windows/MainWindow.xaml.cs | 3 +- .../AppIdentityTests.cs | 4 +- .../AppSettingsStoreTests.cs | 403 ++++++++++++++++++ .../EditorConfigurationTests.cs | 15 +- 17 files changed, 763 insertions(+), 45 deletions(-) create mode 100644 src/MarkdownViewr.Windows.Core/AppSettingsStore.cs create mode 100644 src/MarkdownViewr.Windows.Core/SettingsFault.cs create mode 100644 src/MarkdownViewr.Windows.Core/SettingsRead.cs create mode 100644 tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs 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); } -- 2.54.0 From 600825030a3175e75ff2c57979bf5d78db657474 Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 10 Nov 2026 16:57:37 -0600 Subject: [PATCH 2/5] fix(settings): never set aside a file another program is writing Reading while denying writers makes a file that is being rewritten in place read as busy instead of as invalid content, and the read handle is held until the swap, so the content judged is the content replaced. The swap uses a replace operation: an invalid original moves to its kept-aside name in the same step, a failed save leaves it in place, and a reader that allows replacement no longer blocks a save. Access denied is permanent and reported at once; only a sharing violation is waited out, so a read-only profile no longer stalls a window. One invalid value costs only that value, a trailing comma, a comment and a quoted number are accepted, and a saved theme the JSON reader cannot decode is discarded instead of stopping startup. Kept-aside files use the first free of nine names. Only this store's own temporary files are removed. ADR-0006 records the on-disk protocol. Tests cover each of these with real handles held the way other programs hold them, a deterministic interleaving of two instances, the on-disk format against the previous store, and bounded retries in the concurrent test. Refs #46 Co-Authored-By: Claude Fable 5.2 Signed-off-by: James Maes --- docs/adr/0006-settings-file-protocol.md | 95 +++ docs/security/threat-model.md | 2 +- docs/user-guide.md | 3 +- src/MarkdownViewr.Windows.Core/AppSettings.cs | 3 +- .../AppSettingsContext.cs | 8 +- .../AppSettingsStore.cs | 212 ++++-- .../SettingsFault.cs | 13 +- .../SettingsRead.cs | 7 +- .../AppSettingsStoreTests.cs | 654 ++++++++++++++++-- 9 files changed, 862 insertions(+), 135 deletions(-) create mode 100644 docs/adr/0006-settings-file-protocol.md 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) { -- 2.54.0 From 80e00b3ed1013118e94707b08d77197f8d9b161c Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 10 Nov 2026 17:59:39 -0600 Subject: [PATCH 3/5] fix(settings): replace the settings file in one rename The stored file is now replaced with a single rename that works while other handles are open on it, so there is never a moment without a settings file and a process killed mid-save leaves the old file or the new one. Volumes without that rename fall back to a move. Content the JSON reader cannot decode is treated as invalid content rather than an error, the read handle is released on every path, and a kept-aside copy that cannot be made no longer blocks the save. Clearing recent files also deletes kept-aside copies. ADR-0006, the user guide and the threat model describe the protocol as implemented. Refs #46 Co-Authored-By: Claude Fable 5.2 Signed-off-by: James Maes --- docs/adr/0006-settings-file-protocol.md | 74 ++-- docs/security/threat-model.md | 2 +- docs/user-guide.md | 2 +- .../AppSettingsStore.cs | 302 +++++++++++----- src/MarkdownViewr.Windows.Core/FileSwap.cs | 90 +++++ .../MarkdownViewr.Windows.Core.csproj | 2 + .../SettingsFault.cs | 6 +- .../SettingsRead.cs | 6 +- src/MarkdownViewr.Windows/App.Documents.cs | 5 + .../AppSettingsStoreTests.cs | 323 +++++++++++++++++- .../FileSwapTests.cs | 120 +++++++ 11 files changed, 820 insertions(+), 112 deletions(-) create mode 100644 src/MarkdownViewr.Windows.Core/FileSwap.cs create mode 100644 tests/MarkdownViewr.Windows.Core.Tests/FileSwapTests.cs diff --git a/docs/adr/0006-settings-file-protocol.md b/docs/adr/0006-settings-file-protocol.md index f0a3ae3..085d3af 100644 --- a/docs/adr/0006-settings-file-protocol.md +++ b/docs/adr/0006-settings-file-protocol.md @@ -32,42 +32,64 @@ update. It is never deleted: deleting a lock file leaves a window in which a sec 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. +**Read for an update.** The file is opened for reading while denying writers, and +that handle is held until the rename. A program rewriting the file in place therefore +reads as busy, not as invalid content, and none can start between the read and the +rename. An empty file is waited on for the same attempts, because a program that +clears a file before rewriting it leaves it empty for a moment. + +**Read for display.** The same open is tried first. If another handle has write +access, the file is read beside it instead of waiting, since such a handle is usually +idle. Content read that way which does not parse is retried and, if it never parses, +reported as unreadable rather than corrupt: it cannot be judged while it can still be +written. An empty file reads as defaults at once. **What a read can find.** | Found | Used for display | On update | | --- | --- | --- | | No file | Defaults | The file is created | +| Empty file | Defaults | Replaced after the wait; nothing kept aside | | 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 | +| Some invalid properties | Valid properties kept, invalid ones as defaults | Replaced; a copy of the original is kept aside | +| Not settings at all | Defaults | Replaced; a copy of 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. +commonly leaves behind. Text the JSON reader cannot decode, such as an unpaired +surrogate escape, makes its property invalid. Repair works per top-level property: +one bad entry in a list costs that list. 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 +**Write.** The new content is written to `settings.json.<32 hex digits>.tmp` and +flushed to disk. That file is then renamed over `settings.json` in one step with the +Windows rename that replaces an open file (`FileRenameInfoEx` with POSIX semantics). +At no point is there no `settings.json`: a reader sees the old file or the new one, +and a process killed at any moment leaves one of them. Handles that were open on the +old file and allow deletion go on reading it. `File.Replace` is not used because it +is two renames with the file missing between them, and `File.Move` with overwrite is +not used because it refuses while any handle is open on the destination. + +On a volume that does not implement that rename, the store closes its own read +handle and falls back to `File.Move` with overwrite. 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. +**Kept-aside copies.** Before invalid content is replaced it is copied to +`settings.json.corrupt`, or to the first free of `settings.json.corrupt-2` to +`settings.json.corrupt-9`; the last name is overwritten once all exist. The copy is a +courtesy for recovering a hand edit: if it cannot be made the fault is reported and +the save proceeds, because refusing would block every later change too. A save that +then fails removes the copy it made. Nothing reads the copies back. They contain +whatever the file contained, including recent-document paths, so clearing recent +files deletes them. -**Abandoned temporary files.** A process killed between its write and its swap +**Abandoned temporary files.** A process killed between its write and its rename 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. +apart, for the lock, the read and the rename. 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. @@ -77,11 +99,14 @@ path and no operating-system message text reaches the log. - 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 rename needs Windows 10 version 1607 or later and a file system that supports + it; NTFS does. Other volumes use the fallback, which is not safe against a kill + between closing the file and the move completing. +- An earlier release running against the same data folder still has its own defect: + it takes no lock, and when a read fails it falls back to defaults and may save + them. This store cannot prevent that. It also makes that release's saves fail more + often while both run, because the earlier release cannot replace a file this store + has open. Run one version per data folder. - The store keeps no in-memory copy. Windows hold their own view of the settings and merge only the fields they changed (ADR-0004). @@ -90,6 +115,9 @@ path and no operating-system message text reaches the log. - **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. +- **Renaming the invalid file away instead of copying it.** Rejected: between that + rename and the new file arriving there is no settings file, and a kill there loses + the profile. - **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 7a83d6d..9a6e7e3 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 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) | +| 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 renamed into place in one step, so the file is never absent; a file that is unreadable or being written by another program is never overwritten, a copy of invalid content is kept aside under a bounded set of names and removed when recent files are cleared, 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 c0c1176..9db21e5 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -74,7 +74,7 @@ Documents are rendered locally. Authored scripts and remote images are blocked; - **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 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`). +- **Settings were edited by hand:** a trailing comma, a comment or a quoted number is accepted; the app rewrites the file in its own format on the next save, so comments are not kept. A setting whose value is not valid is ignored and the other settings are kept. When that happens, the next save first copies the file as it was to `settings.json.corrupt` (then `.corrupt-2` up to `.corrupt-9`). Those copies contain everything the file contained; **File → Recent files → Clear recent files** deletes them. 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/AppSettingsStore.cs b/src/MarkdownViewr.Windows.Core/AppSettingsStore.cs index 3978852..85d87ad 100644 --- a/src/MarkdownViewr.Windows.Core/AppSettingsStore.cs +++ b/src/MarkdownViewr.Windows.Core/AppSettingsStore.cs @@ -3,6 +3,7 @@ * SPDX-License-Identifier: MIT */ +using System.Globalization; using System.Text; using System.Text.Json; using System.Text.Json.Nodes; @@ -12,41 +13,50 @@ 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 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. +/// applied to the latest stored settings under a cross-process lock and renamed into place in +/// one step, so two instances cannot erase each other's change and the file is never absent. +/// 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 a copy of +/// the original is kept aside. ADR-0006 describes the files involved. /// public sealed class AppSettingsStore { - /// Attempts for the lock, a read, and the final swap before the operation gives up. + /// Attempts for the lock, a read, and the final rename 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; - /// Invalid files kept aside before the newest slot is reused. + /// Copies of invalid files kept aside before the last name 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; + private readonly Func _renameOver; /// 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) + : this(path, report, wait, FileSwap.TryRenameOver) + { + } + + + + /// Creates the store over a given rename, so tests can stand in for a volume that cannot rename over an open file. + internal AppSettingsStore(string path, Action? report, Action? wait, Func renameOver) { ArgumentException.ThrowIfNullOrWhiteSpace(path); Path = System.IO.Path.GetFullPath(path); _report = report ?? ((_, _) => { }); _wait = wait ?? Thread.Sleep; + _renameOver = renameOver; } @@ -56,11 +66,14 @@ public sealed class AppSettingsStore - /// Reads the stored settings for display. A missing, invalid, or unreadable file yields defaults and is left untouched. + /// + /// Reads the stored settings for display. A missing, empty, invalid or unreadable file yields + /// defaults and is left untouched. A handle that another program merely holds with write + /// access does not delay the read. + /// public AppSettings Load() { - _ = TryRead(out AppSettings settings, out FileStream? stored); - stored?.Dispose(); + _ = TryRead(forUpdate: false, out AppSettings settings, out _); return settings; } @@ -75,6 +88,7 @@ public sealed class AppSettingsStore public AppSettings? Update(Func change) { ArgumentNullException.ThrowIfNull(change); + FileStream? stored = null; try { Directory.CreateDirectory(System.IO.Path.GetDirectoryName(Path)!); @@ -83,26 +97,42 @@ public sealed class AppSettingsStore { return null; } - 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) + // The read handle denies writers until the rename, so the content judged here is the content replaced. + SettingsRead read = TryRead(forUpdate: true, out AppSettings current, out stored); + if(read == SettingsRead.Unreadable) { - 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; + return null; } + AppSettings edited = change(current); + edited.Sanitize(); + RemoveAbandonedTemporaryFiles(); + string? aside = read is SettingsRead.Corrupt or SettingsRead.Repaired ? KeepAside(stored!) : null; + Replace(JsonSerializer.SerializeToUtf8Bytes(edited, AppSettingsContext.Default.AppSettings), stored, aside); + return edited; } catch(Exception exception) when(exception is IOException or UnauthorizedAccessException) { _report(SettingsFault.WriteFailed, exception); return null; } + finally + { + stored?.Dispose(); + } + } + + + + /// + /// Removes the copies of invalid files that saves have kept aside. They hold whatever the + /// file held, including the recent-document history, so clearing that history removes them. + /// + public void DeleteKeptAside() + { + for(int slot = 1; slot <= MaximumKeptAside; slot++) + { + TryDelete(KeptAsideName(slot)); + } } @@ -129,43 +159,122 @@ public sealed class AppSettingsStore /// - /// 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. + /// Reads the file, waiting out other handles a bounded number of times, and reports what it + /// found. For an update the open handle is returned so writers stay out until the rename. /// - private SettingsRead TryRead(out AppSettings settings, out FileStream? stored) + private SettingsRead TryRead(bool forUpdate, out AppSettings settings, out FileStream? stored) { settings = new AppSettings(); stored = null; - SettingsRead result; - try + SettingsRead? read = null; + for(int attempt = 1; read is null; attempt++) { - FileStream? stream = null; - Retry(() => stream = new FileStream(Path, FileMode.Open, FileAccess.Read, FileShare.Read | FileShare.Delete)); - stored = stream; - result = Parse(stream!, ref settings); + if(attempt > 1) + { + _wait(RetryDelay); + } + read = Attempt(forUpdate, attempt == MaximumAttempts, ref settings, ref stored); } - catch(Exception exception) when(exception is FileNotFoundException or DirectoryNotFoundException) + if(read is SettingsRead.Corrupt or SettingsRead.Repaired) { - result = SettingsRead.Missing; - } - catch(Exception exception) when(exception is IOException or UnauthorizedAccessException) - { - _report(SettingsFault.Unreadable, exception); - result = SettingsRead.Unreadable; + _report(read == SettingsRead.Corrupt ? SettingsFault.Corrupt : SettingsFault.Repaired, null); } settings.Sanitize(); - return result; + return read.Value; } - /// Reads at most ; one invalid value costs only that value, never the whole profile. - private SettingsRead Parse(FileStream stream, ref AppSettings settings) + /// + /// One read. Returns null to try again: another handle is in the way, the file is empty while + /// a save is waiting for whoever cleared it, or the content read beside a writer did not parse. + /// On the last attempt it always answers. + /// + private SettingsRead? Attempt(bool forUpdate, bool last, ref AppSettings settings, ref FileStream? stored) { + FileStream? stream = null; + bool keep = false; + try + { + stream = Open(forUpdate, out bool writersExcluded); + SettingsRead read = Parse(stream, ref settings); + if(read == SettingsRead.Empty && forUpdate && !last) + { + // A program that clears the file before rewriting it is about to write. + return null; + } + if(read == SettingsRead.Corrupt && !writersExcluded) + { + // Content read while another program can write may simply be half written. + return last ? Unreadable(null) : null; + } + keep = forUpdate; + stored = keep ? stream : null; + return read; + } + catch(Exception exception) when(exception is FileNotFoundException or DirectoryNotFoundException) + { + return SettingsRead.Missing; + } + catch(IOException exception) when(IsTransient(exception) && !last) + { + return null; + } + catch(Exception exception) when(exception is IOException or UnauthorizedAccessException) + { + return Unreadable(exception); + } + finally + { + if(!keep) + { + stream?.Dispose(); + } + } + } + + + + /// Reports a file whose content could not be established and answers that it must not be replaced. + private SettingsRead Unreadable(Exception? exception) + { + _report(SettingsFault.Unreadable, exception); + return SettingsRead.Unreadable; + } + + + + /// + /// Opens the file denying writers, so a program rewriting it in place reads as busy rather + /// than as invalid content. A read for display falls back to reading beside a handle that has + /// write access, because such a handle is usually idle; the caller then knows the content may + /// be half written. + /// + private FileStream Open(bool forUpdate, out bool writersExcluded) + { + writersExcluded = true; + try + { + return new FileStream(Path, FileMode.Open, FileAccess.Read, FileShare.Read | FileShare.Delete); + } + catch(IOException exception) when(IsTransient(exception) && !forUpdate) + { + writersExcluded = false; + return new FileStream(Path, FileMode.Open, FileAccess.Read, FileShare.ReadWrite | FileShare.Delete); + } + } + + + + /// Reads at most and classifies the content; an invalid property costs that property, never the whole profile. + private static SettingsRead Parse(FileStream stream, ref AppSettings settings) + { + if(stream.Length == 0) + { + return SettingsRead.Empty; + } if(stream.Length > MaximumBytes) { - _report(SettingsFault.Corrupt, null); return SettingsRead.Corrupt; } using var reader = new StreamReader(stream, Encoding.UTF8, detectEncodingFromByteOrderMarks: true, bufferSize: 4096, leaveOpen: true); @@ -178,22 +287,20 @@ public sealed class AppSettingsStore return SettingsRead.Loaded; } } - catch(JsonException) + catch(Exception exception) when(IsInvalidContent(exception)) { if(Salvage(json) is { } repaired) { settings = repaired; - _report(SettingsFault.Repaired, null); return SettingsRead.Repaired; } } - _report(SettingsFault.Corrupt, null); return SettingsRead.Corrupt; } - /// 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. + /// Keeps every stored property 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 @@ -208,14 +315,14 @@ public sealed class AppSettingsStore { _ = new JsonObject { [name] = stored[name]?.DeepClone() }.Deserialize(AppSettingsContext.Default.AppSettings); } - catch(JsonException) + catch(Exception exception) when(IsInvalidContent(exception)) { stored.Remove(name); } } return stored.Deserialize(AppSettingsContext.Default.AppSettings); } - catch(Exception exception) when(exception is JsonException or ArgumentException) + catch(Exception exception) when(IsInvalidContent(exception) || exception is ArgumentException) { // Not JSON, or an object that repeats a property name. return null; @@ -225,10 +332,54 @@ public sealed class AppSettingsStore /// - /// 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. + /// What the JSON reader throws for content that is not valid settings: a syntax or type error, + /// or text it cannot decode, such as an unpaired surrogate escape. /// - private void Replace(byte[] content, bool exists, bool keepAside) + private static bool IsInvalidContent(Exception exception) => exception is JsonException or InvalidOperationException; + + + + /// + /// Copies an invalid file to the first free kept-aside name before it is replaced, or over the + /// last name once all are taken, and returns the copy's path. The copy is a courtesy: failing + /// to make it is reported and does not stop the save, since refusing would block every later + /// change as well; null is returned then. + /// + private string? KeepAside(FileStream stored) + { + int slot = 1; + while(slot < MaximumKeptAside && (File.Exists(KeptAsideName(slot)) || Directory.Exists(KeptAsideName(slot)))) + { + slot++; + } + try + { + using var copy = new FileStream(KeptAsideName(slot), FileMode.Create, FileAccess.Write, FileShare.None); + stored.Position = 0; + stored.CopyTo(copy); + copy.Flush(flushToDisk: true); + return KeptAsideName(slot); + } + catch(Exception exception) when(exception is IOException or UnauthorizedAccessException) + { + _report(SettingsFault.NotKeptAside, exception); + return null; + } + } + + + + /// The kept-aside names: .corrupt, then .corrupt-2 to .corrupt-9. + private string KeptAsideName(int slot) => slot == 1 ? Path + ".corrupt" : Path + ".corrupt-" + slot.ToString(CultureInfo.InvariantCulture); + + + + /// + /// Writes beside the file and flushes to disk, then renames over it in one step. A failure + /// leaves the stored file as it was, and neither the temporary file nor the copy just kept + /// aside behind. + /// + private void Replace(byte[] content, FileStream? stored, string? aside) { string temporary = Path + "." + Guid.NewGuid().ToString("N") + ".tmp"; bool replaced = false; @@ -239,16 +390,22 @@ public sealed class AppSettingsStore stream.Write(content); stream.Flush(flushToDisk: true); } - if(exists) - { - string? aside = keepAside ? KeptAsideName() : null; - Retry(() => File.Replace(temporary, Path, aside, ignoreMetadataErrors: true)); - } - else + if(stored is null) { // No overwrite: a file that appeared since the read is not this instance's to replace. File.Move(temporary, Path); } + else + { + bool renamed = false; + Retry(() => renamed = _renameOver(temporary, Path)); + if(!renamed) + { + // This volume cannot rename over an open file, so the file is closed first. + stored.Dispose(); + Retry(() => File.Move(temporary, Path, overwrite: true)); + } + } replaced = true; } finally @@ -256,26 +413,17 @@ public sealed class AppSettingsStore if(!replaced) { TryDelete(temporary); + if(aside is not null) + { + TryDelete(aside); + } } } } - /// 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. + /// A writer that died before its rename left a temporary file with this store's own name pattern; under the lock every such file is abandoned. private void RemoveAbandonedTemporaryFiles() { string prefix = System.IO.Path.GetFileName(Path) + "."; @@ -310,12 +458,12 @@ public sealed class AppSettingsStore - /// 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; + /// A sharing 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 == SharingViolation; - /// Cleanup only: a temporary file that cannot be removed now is removed by the next successful save. + /// Removes a file if it is there and can be removed; a folder with that name, or a file in use, is left. private static void TryDelete(string path) { try @@ -324,7 +472,7 @@ public sealed class AppSettingsStore } catch(Exception exception) when(exception is IOException or UnauthorizedAccessException) { - // The outcome of the save is already decided. + // Cleanup only: the operation this was part of has already succeeded or failed. } } } diff --git a/src/MarkdownViewr.Windows.Core/FileSwap.cs b/src/MarkdownViewr.Windows.Core/FileSwap.cs new file mode 100644 index 0000000..6e33b8c --- /dev/null +++ b/src/MarkdownViewr.Windows.Core/FileSwap.cs @@ -0,0 +1,90 @@ +/* + * Copyright (c) 2026 James Maes + * SPDX-License-Identifier: MIT + */ + +using System.ComponentModel; +using System.Runtime.InteropServices; +using System.Text; +using Microsoft.Win32.SafeHandles; + + +namespace MarkdownViewr.Windows.Core; + +/// +/// Replaces a file by renaming another over it in a single step. The destination is never +/// absent, not even for an instant, and the rename works while the destination is open through +/// handles that allow deletion; those handles go on reading the content they opened. +/// refuses when any handle is open on the +/// destination, and is two renames with the +/// destination missing between them, so neither can do this. +/// +internal static partial class FileSwap +{ + private const uint DeleteAccess = 0x00010000; + private const uint ShareAll = 0x00000007; + private const uint OpenExisting = 3; + private const int FileRenameInfoEx = 22; + private const uint ReplaceIfExists = 0x1; + private const uint PosixSemantics = 0x2; + private const int AccessDenied = 5; + + + + /// + /// Renames over . Returns false when the + /// volume cannot rename over an open file, as file systems other than NTFS may not. Throws + /// with the Windows error as its HRESULT when the rename is refused, + /// for example by a handle on the destination that does not allow deletion, and + /// when access is denied. + /// + public static bool TryRenameOver(string source, string destination) + { + using SafeFileHandle handle = CreateFile(source, DeleteAccess, ShareAll, 0, OpenExisting, 0, 0); + if(handle.IsInvalid) + { + throw Failure(Marshal.GetLastPInvokeError()); + } + // FILE_RENAME_INFO: flags, root directory handle, name length in bytes, then the name itself. + byte[] name = Encoding.Unicode.GetBytes(destination); + int lengthOffset = 2 * IntPtr.Size; + int nameOffset = lengthOffset + sizeof(int); + byte[] rename = new byte[nameOffset + name.Length + sizeof(char)]; + BitConverter.TryWriteBytes(rename, ReplaceIfExists | PosixSemantics); + BitConverter.TryWriteBytes(rename.AsSpan(lengthOffset), name.Length); + name.CopyTo(rename, nameOffset); + if(SetFileInformationByHandle(handle, FileRenameInfoEx, rename, rename.Length)) + { + return true; + } + int error = Marshal.GetLastPInvokeError(); + return IsUnsupported(error) ? false : throw Failure(error); + } + + + + /// The errors a volume answers with when it does not implement this kind of rename: invalid function, not supported, invalid parameter. + internal static bool IsUnsupported(int error) => error is 1 or 50 or 87; + + + + /// The exception a managed file operation would throw for the same Windows error. + private static Exception Failure(int error) + { + string message = new Win32Exception(error).Message; + return error == AccessDenied ? new UnauthorizedAccessException(message) : new IOException(message, unchecked((int)0x80070000) | error); + } + + + + /// Opens the file to be renamed with delete access, which a managed file stream cannot request. + [LibraryImport("kernel32.dll", EntryPoint = "CreateFileW", SetLastError = true, StringMarshalling = StringMarshalling.Utf16)] + private static partial SafeFileHandle CreateFile(string path, uint access, uint share, nint security, uint disposition, uint flags, nint template); + + + + /// Applies the rename to the open file. + [LibraryImport("kernel32.dll", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + private static partial bool SetFileInformationByHandle(SafeFileHandle file, int informationClass, byte[] information, int size); +} diff --git a/src/MarkdownViewr.Windows.Core/MarkdownViewr.Windows.Core.csproj b/src/MarkdownViewr.Windows.Core/MarkdownViewr.Windows.Core.csproj index 317c5d4..e4eab67 100644 --- a/src/MarkdownViewr.Windows.Core/MarkdownViewr.Windows.Core.csproj +++ b/src/MarkdownViewr.Windows.Core/MarkdownViewr.Windows.Core.csproj @@ -8,6 +8,8 @@ win-x86;win-x64;win-arm64 true + + true diff --git a/src/MarkdownViewr.Windows.Core/SettingsFault.cs b/src/MarkdownViewr.Windows.Core/SettingsFault.cs index 501a04b..5f321c0 100644 --- a/src/MarkdownViewr.Windows.Core/SettingsFault.cs +++ b/src/MarkdownViewr.Windows.Core/SettingsFault.cs @@ -9,9 +9,9 @@ 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 does not hold settings at all; 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 a copy of the file aside. Corrupt, - /// Some stored values were invalid and read as their defaults; the next save keeps the original file aside. + /// Some stored properties were invalid and read as their defaults; the next save keeps a copy of the original 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, @@ -19,4 +19,6 @@ public enum SettingsFault Busy, /// The change could not be written; the stored file is unchanged. WriteFailed, + /// The invalid original could not be copied aside; the change was saved without the copy. + NotKeptAside, } diff --git a/src/MarkdownViewr.Windows.Core/SettingsRead.cs b/src/MarkdownViewr.Windows.Core/SettingsRead.cs index 1ede827..b741223 100644 --- a/src/MarkdownViewr.Windows.Core/SettingsRead.cs +++ b/src/MarkdownViewr.Windows.Core/SettingsRead.cs @@ -13,9 +13,11 @@ internal enum SettingsRead Loaded, /// No file yet; defaults apply and the first save creates it. Missing, - /// Valid values were kept and invalid ones dropped; a save keeps the original file aside. + /// A file with no content, as a program that clears a file before rewriting it leaves it; defaults apply and a save replaces it. + Empty, + /// Valid properties were kept and invalid ones dropped; a save keeps a copy of the original aside. Repaired, - /// Not settings at all; defaults apply and a save keeps the invalid file aside. + /// Not settings at all; defaults apply and a save keeps a copy of the 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 add9df7..4ac93a8 100644 --- a/src/MarkdownViewr.Windows/App.Documents.cs +++ b/src/MarkdownViewr.Windows/App.Documents.cs @@ -129,6 +129,11 @@ public partial class App settings.RecentFiles = []; return settings; }) is not null; + if(saved) + { + // Copies of an invalid settings file kept beside it would still list the history. + AppIdentity.SettingsStore.DeleteKeptAside(); + } RefreshRecentMenus(); return saved; } diff --git a/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs index d574216..ebcfcc5 100644 --- a/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs +++ b/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs @@ -196,6 +196,24 @@ public class AppSettingsStoreTests + /// A change that throws leaves the stored file as it was and open to every other program at once. + [Fact] + public void Update_ChangeThrows_ReleasesTheStoredFile() + { + using var temp = new TempDirectory(); + Seed(temp); + byte[] before = File.ReadAllBytes(temp.File("settings.json")); + var store = new AppSettingsStore(temp.File("settings.json")); + Assert.Throws(() => store.Update(_ => throw new InvalidOperationException("change failed"))); + using(new FileStream(temp.File("settings.json"), FileMode.Open, FileAccess.ReadWrite, FileShare.None)) + { + } + Assert.Equal(before, File.ReadAllBytes(temp.File("settings.json"))); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); + } + + + /// 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() @@ -358,7 +376,7 @@ public class AppSettingsStoreTests Assert.Null(store.Update(settings => settings)); Assert.Equal("Catppuccin Mocha", store.Load().DocumentTheme); } - Assert.Equal([SettingsFault.Unreadable, SettingsFault.Unreadable], faults); + Assert.Equal([SettingsFault.Unreadable], faults); Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); Assert.Equal(0, new FileInfo(temp.File("settings.json")).Length); } @@ -367,7 +385,6 @@ public class AppSettingsStoreTests /// 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]")] @@ -403,24 +420,181 @@ public class AppSettingsStoreTests - /// One invalid value costs only that value: every other stored preference is kept. + /// An invalid property costs only that property: every other stored preference, before and after it, is kept. [Fact] - public void Load_OneInvalidValue_KeepsEveryOtherValue() + public void Load_InvalidProperties_KeepsEveryOtherProperty() { 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}}"""); + 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},"WrapOutlineEntries":true,"ContentWidthPx":1234,"Frontmatter":"Metadata","RenderHighlight":false,"ShowSplashScreen":false,"OutlineWidth":300,"Print":{"Theme":"Nord","Borders":true}}"""); 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.True(loaded.WrapOutlineEntries); + Assert.Equal(1234, loaded.ContentWidthPx); + Assert.Equal("Metadata", loaded.Frontmatter); + Assert.False(loaded.RenderHighlight); + Assert.False(loaded.ShowSplashScreen); + Assert.Equal(300, loaded.OutlineWidth); + Assert.Equal("Nord", loaded.Print.Theme); + Assert.True(loaded.Print.Borders); Assert.Equal(3, loaded.OutlineDepth); Assert.Empty(loaded.RecentFiles); } + /// + /// Text the JSON reader cannot decode, an unpaired surrogate escape, is an invalid property + /// like any other: it is dropped, the rest load, and nothing is thrown into startup. + /// + [Fact] + public void LoadAndUpdate_UndecodableText_DropsThatPropertyAndSavesTheRest() + { + using var temp = new TempDirectory(); + File.WriteAllText(temp.File("settings.json"), "{\"DocumentTheme\":\"Nord\",\"GlobalCss\":\"a\\uD800b\",\"Theme\":\"Dark\"}"); + var faults = new List(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault)); + AppSettings loaded = store.Load(); + Assert.Equal("Nord", loaded.DocumentTheme); + Assert.Equal("Dark", loaded.Theme); + Assert.Equal("", loaded.GlobalCss); + Assert.NotNull(store.Update(settings => settings)); + Assert.Equal([SettingsFault.Repaired, SettingsFault.Repaired], faults); + Assert.Equal("Dark", store.Load().Theme); + Assert.Equal(["settings.json", "settings.json.corrupt", "settings.json.lock"], FileNames(temp.Path)); + } + + + + /// A file with no content is not invalid settings: it reads as defaults, without a fault and without a wait. + [Fact] + public void Load_EmptyFile_ReturnsDefaultsWithoutAFaultOrAWait() + { + using var temp = new TempDirectory(); + File.WriteAllBytes(temp.File("settings.json"), []); + int waits = 0; + var faults = new List(); + Assert.Equal("Catppuccin Mocha", new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault), _ => waits++).Load().DocumentTheme); + Assert.Empty(faults); + Assert.Equal(0, waits); + } + + + + /// + /// A program that clears the file, closes it, and then writes it again leaves it empty for a + /// moment. A save waits for the content instead of filling the gap with defaults. + /// + [Fact] + public void Update_FileClearedThenRewritten_WaitsForTheContent() + { + using var temp = new TempDirectory(); + Seed(temp); + File.WriteAllBytes(temp.File("settings.json"), []); + int waits = 0; + var faults = new List(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault), _ => + { + if(++waits == 2) + { + File.WriteAllText(temp.File("settings.json"), """{"Theme":"Light","DocumentTheme":"Dracula"}"""); + } + }); + AppSettings? saved = store.Update(settings => + { + settings.OutlineDepth = 5; + return settings; + }); + Assert.NotNull(saved); + Assert.Equal(2, waits); + Assert.Empty(faults); + Assert.Equal("Dracula", store.Load().DocumentTheme); + Assert.Equal("Light", store.Load().Theme); + Assert.Equal(5, store.Load().OutlineDepth); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); + } + + + + /// A file that stays empty is replaced after the wait, and nothing is kept aside: there is nothing to keep. + [Fact] + public void Update_FileStaysEmpty_SavesAfterTheWaitWithoutKeepingAnythingAside() + { + using var temp = new TempDirectory(); + File.WriteAllBytes(temp.File("settings.json"), []); + int waits = 0; + var faults = new List(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault), _ => waits++); + Assert.NotNull(store.Update(settings => + { + settings.Theme = "Dark"; + return settings; + })); + Assert.Equal(AppSettingsStore.MaximumAttempts - 1, waits); + Assert.Empty(faults); + Assert.Equal("Dark", store.Load().Theme); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); + } + + + + /// + /// A handle another program merely holds with write access does not delay a read for display: + /// the stored values are read beside it. + /// + [Fact] + public void Load_FileHeldWithWriteAccess_ReadsTheStoredValuesWithoutWaiting() + { + using var temp = new TempDirectory(); + Seed(temp); + 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.ReadWrite, FileShare.ReadWrite | FileShare.Delete)) + { + Assert.Equal("Nord", store.Load().DocumentTheme); + } + Assert.Equal(0, waits); + Assert.Empty(faults); + } + + + + /// + /// Content read beside a writer that does not parse may simply be half written. The read waits + /// for the writer; if the content never becomes valid it is reported as unreadable, not as + /// corrupt, because it cannot be judged while someone is writing it. + /// + [Theory] + [InlineData(true)] + [InlineData(false)] + public void Load_HalfWrittenContentBesideAWriter_WaitsAndNeverCallsItCorrupt(bool writerFinishes) + { + using var temp = new TempDirectory(); + using var writer = new FileStream(temp.File("settings.json"), FileMode.Create, FileAccess.Write, FileShare.ReadWrite | FileShare.Delete); + writer.Write("""{"DocumentTheme":"Dra"""u8); + writer.Flush(); + int waits = 0; + var faults = new List(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault), _ => + { + if(++waits == 2 && writerFinishes) + { + writer.Write("""cula"}"""u8); + writer.Flush(); + } + }); + AppSettings loaded = store.Load(); + Assert.Equal(writerFinishes ? "Dracula" : "Catppuccin Mocha", loaded.DocumentTheme); + Assert.Equal(writerFinishes ? 2 : AppSettingsStore.MaximumAttempts - 1, waits); + Assert.Equal(writerFinishes ? [] : [SettingsFault.Unreadable], faults); + } + + + /// A repeated property beside an invalid value is still read without throwing into startup. [Fact] public void Load_RepeatedPropertyAndInvalidValue_DoesNotThrow() @@ -520,7 +694,144 @@ public class AppSettingsStoreTests - /// When the save over invalid content fails, the invalid file is still in place: it is never moved away first. + /// A folder where a kept-aside copy would go is skipped; the next name is used. + [Fact] + public void Update_KeptAsideNameIsAFolder_UsesTheNextName() + { + using var temp = new TempDirectory(); + Directory.CreateDirectory(temp.File("settings.json.corrupt")); + File.WriteAllText(temp.File("settings.json"), "not json"); + var faults = new List(); + Assert.NotNull(new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault)).Update(settings => settings)); + Assert.Equal("not json", File.ReadAllText(temp.File("settings.json.corrupt-2"))); + Assert.Equal([SettingsFault.Corrupt], faults); + } + + + + /// When no copy can be made the save still goes through, and the missing copy is reported. + [Fact] + public void Update_CopyCannotBeKeptAside_SavesAndReportsIt() + { + using var temp = new TempDirectory(); + for(int slot = 2; slot <= AppSettingsStore.MaximumKeptAside; slot++) + { + Directory.CreateDirectory(temp.File("settings.json.corrupt-" + slot.ToString(System.Globalization.CultureInfo.InvariantCulture))); + } + Directory.CreateDirectory(temp.File("settings.json.corrupt")); + File.WriteAllText(temp.File("settings.json"), "not json"); + var faults = new List<(SettingsFault Fault, Exception? Exception)>(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, exception) => faults.Add((fault, exception))); + Assert.NotNull(store.Update(settings => + { + settings.Theme = "Dark"; + return settings; + })); + Assert.Equal([SettingsFault.Corrupt, SettingsFault.NotKeptAside], faults.Select(entry => entry.Fault)); + Assert.IsType(faults[1].Exception); + Assert.Equal("Dark", store.Load().Theme); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); + } + + + + /// Kept-aside copies hold whatever the file held, so they can be removed on request; folders with those names are left. + [Fact] + public void DeleteKeptAside_CopiesAndAFolder_RemovesOnlyTheCopies() + { + using var temp = new TempDirectory(); + Seed(temp); + File.WriteAllText(temp.File("settings.json.corrupt"), "copy 1"); + File.WriteAllText(temp.File("settings.json.corrupt-2"), "copy 2"); + File.WriteAllText(temp.File("settings.json.corrupt-9"), "copy 9"); + File.WriteAllText(temp.File("settings.json.corrupt-10"), "not a kept-aside name"); + Directory.CreateDirectory(temp.File("settings.json.corrupt-3")); + new AppSettingsStore(temp.File("settings.json")).DeleteKeptAside(); + Assert.Equal(["settings.json", "settings.json.corrupt-10", "settings.json.lock"], FileNames(temp.Path)); + Assert.True(Directory.Exists(temp.File("settings.json.corrupt-3"))); + } + + + + /// + /// The stored file is in place at the moment the new one is renamed over it and immediately + /// after: there is no step at which the folder has no settings file. + /// + [Fact] + public void Update_AtTheRename_TheStoredFileIsStillInPlace() + { + using var temp = new TempDirectory(); + Seed(temp); + string path = temp.File("settings.json"); + var observed = new List(); + var store = new AppSettingsStore(path, null, null, (source, destination) => + { + observed.Add(File.Exists(destination) ? "stored file present before" : "stored file missing before"); + observed.Add(File.Exists(source) ? "new file written" : "new file missing"); + bool renamed = FileSwap.TryRenameOver(source, destination); + observed.Add(File.Exists(destination) ? "stored file present after" : "stored file missing after"); + return renamed; + }); + Assert.NotNull(store.Update(settings => + { + settings.Theme = "Light"; + return settings; + })); + Assert.Equal(["stored file present before", "new file written", "stored file present after"], observed); + Assert.Equal("Light", store.Load().Theme); + } + + + + /// On a volume that cannot rename over an open file the store closes its own handle and replaces the file the older way. + [Fact] + public void Update_VolumeCannotRenameOverAnOpenFile_ReplacesAfterClosingItsHandle() + { + using var temp = new TempDirectory(); + Seed(temp); + int attempts = 0; + var store = new AppSettingsStore(temp.File("settings.json"), null, _ => { }, (_, _) => + { + attempts++; + return false; + }); + Assert.NotNull(store.Update(settings => + { + settings.Theme = "Light"; + return settings; + })); + Assert.Equal(1, attempts); + Assert.Equal("Light", store.Load().Theme); + Assert.Equal("Nord", store.Load().DocumentTheme); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); + } + + + + /// Only a sharing violation is waited out at the rename; any other I/O failure fails the save at once. + [Theory] + [InlineData(unchecked((int)0x80070020), 3, true)] + [InlineData(unchecked((int)0x80070021), 0, false)] + [InlineData(unchecked((int)0x8007001F), 0, false)] + [InlineData(0x00000020, 0, false)] + public void Update_RenameFails_WaitsOnlyForASharingViolation(int result, int expectedWaits, bool saved) + { + using var temp = new TempDirectory(); + Seed(temp); + int waits = 0; + int attempts = 0; + var faults = new List(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault), _ => waits++, (source, destination) => + ++attempts <= 3 ? throw new IOException("refused", result) : FileSwap.TryRenameOver(source, destination)); + Assert.Equal(saved, store.Update(settings => settings) is not null); + Assert.Equal(expectedWaits, waits); + Assert.Equal(saved ? [] : [SettingsFault.WriteFailed], faults); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); + } + + + + /// When the save over invalid content fails, the invalid file is still in place and the copy made for this save is removed again. [Fact] public void Update_InvalidContentAndFailedSave_LeavesTheInvalidFileInPlace() { diff --git a/tests/MarkdownViewr.Windows.Core.Tests/FileSwapTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/FileSwapTests.cs new file mode 100644 index 0000000..2b5c013 --- /dev/null +++ b/tests/MarkdownViewr.Windows.Core.Tests/FileSwapTests.cs @@ -0,0 +1,120 @@ +/* + * Copyright (c) 2026 James Maes + * SPDX-License-Identifier: MIT + */ + + +namespace MarkdownViewr.Windows.Core.Tests; + +/// Renaming one real file over another, with other real handles open on the destination. +public sealed class FileSwapTests +{ + /// The rename replaces the destination and removes the source name. + [Fact] + public void TryRenameOver_ExistingDestination_ReplacesIt() + { + using var temp = new TempDirectory(); + File.WriteAllText(temp.File("stored.json"), "old"); + File.WriteAllText(temp.File("new.tmp"), "new"); + Assert.True(FileSwap.TryRenameOver(temp.File("new.tmp"), temp.File("stored.json"))); + Assert.Equal("new", File.ReadAllText(temp.File("stored.json"))); + Assert.False(File.Exists(temp.File("new.tmp"))); + } + + + + /// A reader that allows deletion does not stop the rename, and goes on reading the content it opened. + [Fact] + public void TryRenameOver_DestinationOpenByAReaderThatAllowsDeletion_ReplacesItUnderTheReader() + { + using var temp = new TempDirectory(); + File.WriteAllText(temp.File("stored.json"), "old content"); + File.WriteAllText(temp.File("new.tmp"), "new"); + using(var reader = new StreamReader(new FileStream(temp.File("stored.json"), FileMode.Open, FileAccess.Read, FileShare.Read | FileShare.Delete))) + { + Assert.True(FileSwap.TryRenameOver(temp.File("new.tmp"), temp.File("stored.json"))); + Assert.Equal("new", File.ReadAllText(temp.File("stored.json"))); + Assert.Equal("old content", reader.ReadToEnd()); + } + Assert.Equal(["stored.json"], Directory.GetFiles(temp.Path).Select(file => Path.GetFileName(file)!), StringComparer.Ordinal); + } + + + + /// A handle that does not allow deletion refuses the rename with a sharing violation, and both files stay as they were. + [Fact] + public void TryRenameOver_DestinationOpenWithoutDeleteSharing_IsASharingViolation() + { + using var temp = new TempDirectory(); + File.WriteAllText(temp.File("stored.json"), "old"); + File.WriteAllText(temp.File("new.tmp"), "new"); + using(new FileStream(temp.File("stored.json"), FileMode.Open, FileAccess.Read, FileShare.Read)) + { + IOException refused = Assert.Throws(() => FileSwap.TryRenameOver(temp.File("new.tmp"), temp.File("stored.json"))); + Assert.Equal(unchecked((int)0x80070020), refused.HResult); + } + Assert.Equal("old", File.ReadAllText(temp.File("stored.json"))); + Assert.Equal("new", File.ReadAllText(temp.File("new.tmp"))); + } + + + + /// A read-only destination is access denied, as it is for a managed move. + [Fact] + public void TryRenameOver_ReadOnlyDestination_IsAccessDenied() + { + using var temp = new TempDirectory(); + File.WriteAllText(temp.File("stored.json"), "old"); + File.WriteAllText(temp.File("new.tmp"), "new"); + File.SetAttributes(temp.File("stored.json"), FileAttributes.ReadOnly); + try + { + Assert.Throws(() => FileSwap.TryRenameOver(temp.File("new.tmp"), temp.File("stored.json"))); + } + finally + { + File.SetAttributes(temp.File("stored.json"), FileAttributes.Normal); + } + Assert.Equal("old", File.ReadAllText(temp.File("stored.json"))); + } + + + + /// A source that does not exist is reported with the Windows error for a missing file. + [Fact] + public void TryRenameOver_MissingSource_IsAnIoFailure() + { + using var temp = new TempDirectory(); + File.WriteAllText(temp.File("stored.json"), "old"); + IOException missing = Assert.Throws(() => FileSwap.TryRenameOver(temp.File("absent.tmp"), temp.File("stored.json"))); + Assert.Equal(unchecked((int)0x80070002), missing.HResult); + Assert.Equal("old", File.ReadAllText(temp.File("stored.json"))); + } + + + + /// A destination that does not exist yet is simply created by the rename. + [Fact] + public void TryRenameOver_NoDestinationYet_RenamesTheSource() + { + using var temp = new TempDirectory(); + File.WriteAllText(temp.File("new.tmp"), "new"); + Assert.True(FileSwap.TryRenameOver(temp.File("new.tmp"), temp.File("stored.json"))); + Assert.Equal("new", File.ReadAllText(temp.File("stored.json"))); + } + + + + /// Only the errors a volume gives for an unimplemented rename mean "use another way"; every other error is a failure. + [Theory] + [InlineData(1, true)] + [InlineData(50, true)] + [InlineData(87, true)] + [InlineData(0, false)] + [InlineData(2, false)] + [InlineData(5, false)] + [InlineData(32, false)] + [InlineData(17, false)] + public void IsUnsupported_WindowsError_SeparatesAnUnimplementedRenameFromAFailure(int error, bool unsupported) + => Assert.Equal(unsupported, FileSwap.IsUnsupported(error)); +} -- 2.54.0 From 34b6aba0ff5cc2a04084a7ffb340f5d939202451 Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 10 Nov 2026 18:43:34 -0600 Subject: [PATCH 4/5] fix(settings): save to long paths and place kept copies after the save The rename that replaces the settings file now takes the path in the form Windows reads as written, so a data folder past 260 characters, or one whose name is not well-formed UTF-16, saves like any other. Before, every save after the first was refused there. The copy of an invalid file is written to a temporary name and given its kept-aside name only after the save succeeds, so a failed save can no longer overwrite and then delete an earlier copy, and a killed save leaves no duplicate copies. A file over 16 MiB is replaced without a copy. On a volume that needs the fallback move, a reader's open handle is waited out instead of failing the save at once. Refs #46 Co-Authored-By: Claude Fable 5.2 Signed-off-by: James Maes --- docs/adr/0006-settings-file-protocol.md | 38 ++-- docs/user-guide.md | 2 +- .../AppSettingsStore.cs | 93 +++++++--- src/MarkdownViewr.Windows.Core/FileSwap.cs | 22 ++- .../AppSettingsStoreTests.cs | 164 ++++++++++++++++++ .../FileSwapTests.cs | 14 ++ 6 files changed, 292 insertions(+), 41 deletions(-) diff --git a/docs/adr/0006-settings-file-protocol.md b/docs/adr/0006-settings-file-protocol.md index 085d3af..d996d2f 100644 --- a/docs/adr/0006-settings-file-protocol.md +++ b/docs/adr/0006-settings-file-protocol.md @@ -52,7 +52,7 @@ written. An empty file reads as defaults at once. | Empty file | Defaults | Replaced after the wait; nothing kept aside | | Valid settings | The stored values | Replaced | | Some invalid properties | Valid properties kept, invalid ones as defaults | Replaced; a copy of the original is kept aside | -| Not settings at all | Defaults | Replaced; a copy of the original is kept aside | +| Not settings at all | Defaults | Replaced; a copy of the original is kept aside, unless it is over 16 MiB | | 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 @@ -69,27 +69,41 @@ old file and allow deletion go on reading it. `File.Replace` is not used because is two renames with the file missing between them, and `File.Move` with overwrite is not used because it refuses while any handle is open on the destination. +The rename is given the path in the form Windows takes as written, so a data folder +whose path is longer than 260 characters, or whose name is not well-formed UTF-16, +behaves like any other. + On a volume that does not implement that rename, the store closes its own read handle and falls back to `File.Move` with overwrite. 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 copies.** Before invalid content is replaced it is copied to -`settings.json.corrupt`, or to the first free of `settings.json.corrupt-2` to -`settings.json.corrupt-9`; the last name is overwritten once all exist. The copy is a -courtesy for recovering a hand edit: if it cannot be made the fault is reported and -the save proceeds, because refusing would block every later change too. A save that -then fails removes the copy it made. Nothing reads the copies back. They contain -whatever the file contained, including recent-document paths, so clearing recent -files deletes them. +**Kept-aside copies.** Before invalid content is replaced it is copied to a temporary +file. Once the save has succeeded the copy is renamed to `settings.json.corrupt`, or +to the first free of `settings.json.corrupt-2` to `settings.json.corrupt-9`; the last +name is overwritten once all exist. A save that fails removes its temporary copy and +leaves every earlier copy as it was, and the invalid file is still in place. The copy +is a courtesy for recovering a hand edit, with these limits: + +- a file over 16 MiB is replaced without a copy, since it was never this + application's settings and copying it would stall the window; +- a copy that cannot be given its name is removed and reported, and the save stands, + because refusing would block every later change too; +- a process killed between the save and the rename of the copy loses that copy. + +Nothing reads the copies back. They contain whatever the file contained, including +recent-document paths, so clearing recent files deletes them. **Abandoned temporary files.** A process killed between its write and its rename -leaves a temporary file. The next update, holding the lock, removes files that match -this store's own name pattern and no others. +leaves a temporary file, and possibly a temporary copy. 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 rename. Access denied is treated as permanent -and reported at once, so a read-only profile does not stall a window. +and reported at once, so a read-only profile does not stall a window. The fallback +move is the exception: it answers access denied while any handle is open on the +file, a reader's included, so there access denied is waited out as well, and a +read-only file costs the full wait before the save is refused. **Reporting.** Faults are reported by kind with the exception type and HRESULT. No path and no operating-system message text reaches the log. diff --git a/docs/user-guide.md b/docs/user-guide.md index 9db21e5..7e47870 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -74,7 +74,7 @@ Documents are rendered locally. Authored scripts and remote images are blocked; - **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 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; the app rewrites the file in its own format on the next save, so comments are not kept. A setting whose value is not valid is ignored and the other settings are kept. When that happens, the next save first copies the file as it was to `settings.json.corrupt` (then `.corrupt-2` up to `.corrupt-9`). Those copies contain everything the file contained; **File → Recent files → Clear recent files** deletes them. +- **Settings were edited by hand:** a trailing comma, a comment or a quoted number is accepted; the app rewrites the file in its own format on the next save, so comments are not kept. A syntax error such as a missing comma or quote makes the whole file unreadable: every setting starts from its default, and the next save keeps a copy of your file as described below. A setting whose value is not valid is ignored and the other settings are kept. When either happens, the next save keeps a copy of the file as it was, named `settings.json.corrupt` (then `.corrupt-2` up to `.corrupt-9`). Those copies contain everything the file contained; **File → Recent files → Clear recent files** deletes them. 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/AppSettingsStore.cs b/src/MarkdownViewr.Windows.Core/AppSettingsStore.cs index 85d87ad..c09b20b 100644 --- a/src/MarkdownViewr.Windows.Core/AppSettingsStore.cs +++ b/src/MarkdownViewr.Windows.Core/AppSettingsStore.cs @@ -106,8 +106,12 @@ public sealed class AppSettingsStore AppSettings edited = change(current); edited.Sanitize(); RemoveAbandonedTemporaryFiles(); - string? aside = read is SettingsRead.Corrupt or SettingsRead.Repaired ? KeepAside(stored!) : null; - Replace(JsonSerializer.SerializeToUtf8Bytes(edited, AppSettingsContext.Default.AppSettings), stored, aside); + string? copy = read is SettingsRead.Corrupt or SettingsRead.Repaired ? CopyAside(stored!) : null; + Replace(JsonSerializer.SerializeToUtf8Bytes(edited, AppSettingsContext.Default.AppSettings), stored, copy); + if(copy is not null) + { + KeepAside(copy); + } return edited; } catch(Exception exception) when(exception is IOException or UnauthorizedAccessException) @@ -340,12 +344,35 @@ public sealed class AppSettingsStore /// - /// Copies an invalid file to the first free kept-aside name before it is replaced, or over the - /// last name once all are taken, and returns the copy's path. The copy is a courtesy: failing - /// to make it is reported and does not stop the save, since refusing would block every later - /// change as well; null is returned then. + /// Copies an invalid file to a temporary name before it is replaced and returns that name. A + /// file past the size bound is not this application's settings and is not worth a copy: that + /// is reported and null returned. A copy left behind by a save that never finished is removed + /// with the other abandoned temporary files. /// - private string? KeepAside(FileStream stored) + private string? CopyAside(FileStream stored) + { + if(stored.Length > MaximumBytes) + { + _report(SettingsFault.NotKeptAside, null); + return null; + } + string temporary = TemporaryName(); + using var copy = new FileStream(temporary, FileMode.CreateNew, FileAccess.Write, FileShare.None); + stored.Position = 0; + stored.CopyTo(copy); + copy.Flush(flushToDisk: true); + return temporary; + } + + + + /// + /// Gives the copy of a replaced invalid file its lasting name: the first free kept-aside name, + /// or the last one once all are taken. This happens after the save, so a save that fails + /// never disturbs an earlier copy. The copy is a courtesy: one that cannot be placed is + /// removed and reported, and the save stands. + /// + private void KeepAside(string copy) { int slot = 1; while(slot < MaximumKeptAside && (File.Exists(KeptAsideName(slot)) || Directory.Exists(KeptAsideName(slot)))) @@ -354,16 +381,12 @@ public sealed class AppSettingsStore } try { - using var copy = new FileStream(KeptAsideName(slot), FileMode.Create, FileAccess.Write, FileShare.None); - stored.Position = 0; - stored.CopyTo(copy); - copy.Flush(flushToDisk: true); - return KeptAsideName(slot); + File.Move(copy, KeptAsideName(slot), overwrite: true); } catch(Exception exception) when(exception is IOException or UnauthorizedAccessException) { + TryDelete(copy); _report(SettingsFault.NotKeptAside, exception); - return null; } } @@ -374,14 +397,19 @@ public sealed class AppSettingsStore + /// A name beside the settings file that only this store uses, so an abandoned one can be recognized and removed. + private string TemporaryName() => Path + "." + Guid.NewGuid().ToString("N") + ".tmp"; + + + /// /// Writes beside the file and flushes to disk, then renames over it in one step. A failure - /// leaves the stored file as it was, and neither the temporary file nor the copy just kept - /// aside behind. + /// leaves the stored file as it was, and neither the temporary file nor the copy of the + /// invalid original behind. /// - private void Replace(byte[] content, FileStream? stored, string? aside) + private void Replace(byte[] content, FileStream? stored, string? copy) { - string temporary = Path + "." + Guid.NewGuid().ToString("N") + ".tmp"; + string temporary = TemporaryName(); bool replaced = false; try { @@ -401,9 +429,10 @@ public sealed class AppSettingsStore Retry(() => renamed = _renameOver(temporary, Path)); if(!renamed) { - // This volume cannot rename over an open file, so the file is closed first. + // This volume cannot rename over an open file, so the file is closed first. This + // move is refused while any handle is open on the file, a reader's included. stored.Dispose(); - Retry(() => File.Move(temporary, Path, overwrite: true)); + Retry(() => File.Move(temporary, Path, overwrite: true), IsBlockedByAHandle); } } replaced = true; @@ -413,9 +442,9 @@ public sealed class AppSettingsStore if(!replaced) { TryDelete(temporary); - if(aside is not null) + if(copy is not null) { - TryDelete(aside); + TryDelete(copy); } } } @@ -439,9 +468,14 @@ public sealed class AppSettingsStore - /// 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) + /// + /// Repeats an operation only while another handle is in the way, a bounded number of times; + /// the last failure is thrown. says which failures mean that: + /// a sharing violation unless the operation reports an open handle differently. + /// + private void Retry(Action operation, Func? inTheWay = null) { + inTheWay ??= IsTransient; for(int attempt = 1; ; attempt++) { try @@ -449,7 +483,7 @@ public sealed class AppSettingsStore operation(); return; } - catch(IOException exception) when(attempt < MaximumAttempts && IsTransient(exception)) + catch(Exception exception) when(attempt < MaximumAttempts && inTheWay(exception)) { _wait(RetryDelay); } @@ -459,7 +493,16 @@ public sealed class AppSettingsStore /// A sharing 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 == SharingViolation; + private static bool IsTransient(Exception exception) => exception is IOException && exception.HResult == SharingViolation; + + + + /// + /// What a move over an existing file fails with while another handle is open on it. Unlike + /// an open or a rename, that move answers access denied, which therefore has to be waited out + /// there even though a read-only file gives the same answer. + /// + private static bool IsBlockedByAHandle(Exception exception) => exception is UnauthorizedAccessException || IsTransient(exception); diff --git a/src/MarkdownViewr.Windows.Core/FileSwap.cs b/src/MarkdownViewr.Windows.Core/FileSwap.cs index 6e33b8c..f2c9eae 100644 --- a/src/MarkdownViewr.Windows.Core/FileSwap.cs +++ b/src/MarkdownViewr.Windows.Core/FileSwap.cs @@ -5,7 +5,6 @@ using System.ComponentModel; using System.Runtime.InteropServices; -using System.Text; using Microsoft.Win32.SafeHandles; @@ -40,13 +39,14 @@ internal static partial class FileSwap /// public static bool TryRenameOver(string source, string destination) { - using SafeFileHandle handle = CreateFile(source, DeleteAccess, ShareAll, 0, OpenExisting, 0, 0); + using SafeFileHandle handle = CreateFile(Verbatim(source), DeleteAccess, ShareAll, 0, OpenExisting, 0, 0); if(handle.IsInvalid) { throw Failure(Marshal.GetLastPInvokeError()); } // FILE_RENAME_INFO: flags, root directory handle, name length in bytes, then the name itself. - byte[] name = Encoding.Unicode.GetBytes(destination); + // The name is copied as stored: a file name may hold text that is not well-formed UTF-16. + byte[] name = MemoryMarshal.AsBytes(Verbatim(destination).AsSpan()).ToArray(); int lengthOffset = 2 * IntPtr.Size; int nameOffset = lengthOffset + sizeof(int); byte[] rename = new byte[nameOffset + name.Length + sizeof(char)]; @@ -63,6 +63,22 @@ internal static partial class FileSwap + /// + /// The form of a full path that Windows takes as written, without the 260-character limit the + /// plain form has in a process that has not opted in to long paths. The managed file calls add + /// it themselves; a direct Windows call has to. + /// + internal static string Verbatim(string fullPath) + { + if(fullPath.StartsWith(@"\\?\", StringComparison.Ordinal) || fullPath.StartsWith(@"\\.\", StringComparison.Ordinal)) + { + return fullPath; + } + return fullPath.StartsWith(@"\\", StringComparison.Ordinal) ? string.Concat(@"\\?\UNC\", fullPath.AsSpan(2)) : @"\\?\" + fullPath; + } + + + /// The errors a volume answers with when it does not implement this kind of rename: invalid function, not supported, invalid parameter. internal static bool IsUnsupported(int error) => error is 1 or 50 or 87; diff --git a/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs index ebcfcc5..05b1e48 100644 --- a/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs +++ b/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs @@ -850,6 +850,170 @@ public class AppSettingsStoreTests + /// A failed save over invalid content leaves every earlier kept-aside copy as it was, the last one included. + [Fact] + public void Update_InvalidContentAndFailedSaveWithEveryNameUsed_KeepsTheEarlierCopies() + { + 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"); + string[] before = FileNames(temp.Path); + 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("kept 9", File.ReadAllText(temp.File("settings.json.corrupt-9"))); + Assert.Equal("kept 1", File.ReadAllText(temp.File("settings.json.corrupt"))); + Assert.Equal("newest invalid", File.ReadAllText(temp.File("settings.json"))); + Assert.Equal([.. before, "settings.json.lock"], FileNames(temp.Path)); + } + + + + /// A file past the size bound is replaced without being copied: copying it would stall the window and fill the disk with something that was never settings. + [Fact] + public void Update_OversizedFile_IsReplacedWithoutACopy() + { + using var temp = new TempDirectory(); + using(var oversized = new FileStream(temp.File("settings.json"), FileMode.CreateNew)) + { + oversized.SetLength(AppSettingsStore.MaximumBytes + 1); + } + var faults = new List<(SettingsFault Fault, Exception? Exception)>(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, exception) => faults.Add((fault, exception))); + Assert.NotNull(store.Update(settings => + { + settings.Theme = "Dark"; + return settings; + })); + Assert.Equal([SettingsFault.Corrupt, SettingsFault.NotKeptAside], faults.Select(entry => entry.Fault)); + Assert.Null(faults[1].Exception); + Assert.Equal("Dark", store.Load().Theme); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); + } + + + + /// + /// On a volume that cannot rename over an open file, the move that replaces it is refused with + /// access denied while a reader has the file open. That is waited out like any other handle, + /// each pause being the store's retry delay. + /// + [Fact] + public void Update_FallbackMoveBlockedByAReader_WaitsForIt() + { + using var temp = new TempDirectory(); + Seed(temp); + FileStream? reader = new(temp.File("settings.json"), FileMode.Open, FileAccess.Read, FileShare.Read | FileShare.Delete); + var delays = new List(); + var faults = new List(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault), delay => + { + delays.Add(delay); + if(delays.Count == 2) + { + reader.Dispose(); + } + }, (_, _) => false); + Assert.NotNull(store.Update(settings => + { + settings.Theme = "Light"; + return settings; + })); + Assert.Equal([AppSettingsStore.RetryDelay, AppSettingsStore.RetryDelay], delays); + Assert.Empty(faults); + Assert.Equal("Light", store.Load().Theme); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); + } + + + + /// The same move on a read-only file is refused for good: the store gives up after its bounded wait and leaves the file and the folder as they were. + [Fact] + public void Update_FallbackMoveOnAReadOnlyFile_GivesUpAfterTheWait() + { + using var temp = new TempDirectory(); + Seed(temp); + byte[] before = File.ReadAllBytes(temp.File("settings.json")); + File.SetAttributes(temp.File("settings.json"), FileAttributes.ReadOnly); + try + { + 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++, (_, _) => false); + Assert.Null(store.Update(settings => settings)); + Assert.Equal(AppSettingsStore.MaximumAttempts - 1, waits); + Assert.Equal(SettingsFault.WriteFailed, Assert.Single(faults).Fault); + Assert.IsType(faults[0].Exception); + Assert.Equal(before, File.ReadAllBytes(temp.File("settings.json"))); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); + } + finally + { + File.SetAttributes(temp.File("settings.json"), FileAttributes.Normal); + } + } + + + + /// + /// The data folder can be anywhere the person points it. A path past the classic 260-character + /// limit, and a folder whose name is not well-formed UTF-16 (which the file system allows), + /// are saved to again and again like any other. + /// + [Theory] + [InlineData(true)] + [InlineData(false)] + public void Update_UnusualFolderPath_SavesRepeatedly(bool longPath) + { + using var temp = new TempDirectory(); + string folder = longPath + ? Path.Combine(temp.Path, new string('a', 120), new string('b', 120), new string('c', 60)) + : Path.Combine(temp.Path, "profile \uD800 name"); + var faults = new List(); + var store = new AppSettingsStore(Path.Combine(folder, "settings.json"), (fault, _) => faults.Add(fault)); + Assert.Equal(longPath, store.Path.Length > 260); + foreach(string theme in new[] { "Dark", "Light", "Dark" }) + { + Assert.NotNull(store.Update(settings => + { + settings.Theme = theme; + return settings; + })); + Assert.Equal(theme, store.Load().Theme); + } + Assert.Empty(faults); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(folder)); + } + + + + /// Without a wait supplied, the store really pauses between attempts instead of spinning through them. + [Fact] + public void Update_LockHeld_DefaultWaitPausesBetweenAttempts() + { + using var temp = new TempDirectory(); + Seed(temp); + var store = new AppSettingsStore(temp.File("settings.json")); + using(new FileStream(temp.File("settings.json.lock"), FileMode.Open, FileAccess.ReadWrite, FileShare.None)) + { + long started = System.Diagnostics.Stopwatch.GetTimestamp(); + Assert.Null(store.Update(settings => settings)); + TimeSpan elapsed = System.Diagnostics.Stopwatch.GetElapsedTime(started); + Assert.True(elapsed >= AppSettingsStore.RetryDelay * (AppSettingsStore.MaximumAttempts - 1) / 2, "The store waited only " + elapsed + "."); + } + } + + + /// 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() diff --git a/tests/MarkdownViewr.Windows.Core.Tests/FileSwapTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/FileSwapTests.cs index 2b5c013..4868c21 100644 --- a/tests/MarkdownViewr.Windows.Core.Tests/FileSwapTests.cs +++ b/tests/MarkdownViewr.Windows.Core.Tests/FileSwapTests.cs @@ -9,6 +9,20 @@ namespace MarkdownViewr.Windows.Core.Tests; /// Renaming one real file over another, with other real handles open on the destination. public sealed class FileSwapTests { + /// A full path is given the form Windows takes as written: a drive path, a network path, and paths that already have it. + [Theory] + [InlineData(@"C:\data\settings.json", @"\\?\C:\data\settings.json")] + [InlineData(@"\\server\share\settings.json", @"\\?\UNC\server\share\settings.json")] + [InlineData(@"\\?\C:\data\settings.json", @"\\?\C:\data\settings.json")] + [InlineData(@"\\?\UNC\server\share\settings.json", @"\\?\UNC\server\share\settings.json")] + [InlineData(@"\\.\C:\data\settings.json", @"\\.\C:\data\settings.json")] + public void Verbatim_FullPath_IsPrefixedOnce(string path, string expected) + { + Assert.Equal(expected, FileSwap.Verbatim(path)); + } + + + /// The rename replaces the destination and removes the source name. [Fact] public void TryRenameOver_ExistingDestination_ReplacesIt() -- 2.54.0 From 181950b73a546229a20cb3b6d592d022f9f40b27 Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 10 Nov 2026 19:07:46 -0600 Subject: [PATCH 5/5] fix(settings): save when the invalid file cannot be copied A copy of an invalid settings file that cannot be written, for example on a full disk, is now reported and the save goes ahead, as it does when the copy cannot be given its name. Before, the save was refused. On a volume that needs the fallback move, a read-only settings file is recognized by its attribute and refused at once instead of after the full wait. Refs #46 Co-Authored-By: Claude Fable 5.2 Signed-off-by: James Maes --- docs/adr/0006-settings-file-protocol.md | 9 +-- .../AppSettingsStore.cs | 39 ++++++++----- .../AppSettingsStoreTests.cs | 56 ++++++++++++------- 3 files changed, 66 insertions(+), 38 deletions(-) diff --git a/docs/adr/0006-settings-file-protocol.md b/docs/adr/0006-settings-file-protocol.md index d996d2f..bb76851 100644 --- a/docs/adr/0006-settings-file-protocol.md +++ b/docs/adr/0006-settings-file-protocol.md @@ -87,8 +87,9 @@ is a courtesy for recovering a hand edit, with these limits: - a file over 16 MiB is replaced without a copy, since it was never this application's settings and copying it would stall the window; -- a copy that cannot be given its name is removed and reported, and the save stands, - because refusing would block every later change too; +- a copy that cannot be written, for example on a full disk, or cannot be given its + name, is removed and reported, and the save goes ahead, because refusing would + block every later change too; - a process killed between the save and the rename of the copy loses that copy. Nothing reads the copies back. They contain whatever the file contained, including @@ -102,8 +103,8 @@ lock, removes files that match this store's own name pattern and no others. apart, for the lock, the read and the rename. Access denied is treated as permanent and reported at once, so a read-only profile does not stall a window. The fallback move is the exception: it answers access denied while any handle is open on the -file, a reader's included, so there access denied is waited out as well, and a -read-only file costs the full wait before the save is refused. +file, a reader's included, so there access denied is waited out as well, unless the +file carries the read-only attribute. **Reporting.** Faults are reported by kind with the exception type and HRESULT. No path and no operating-system message text reaches the log. diff --git a/src/MarkdownViewr.Windows.Core/AppSettingsStore.cs b/src/MarkdownViewr.Windows.Core/AppSettingsStore.cs index c09b20b..feeaf90 100644 --- a/src/MarkdownViewr.Windows.Core/AppSettingsStore.cs +++ b/src/MarkdownViewr.Windows.Core/AppSettingsStore.cs @@ -344,10 +344,11 @@ public sealed class AppSettingsStore /// - /// Copies an invalid file to a temporary name before it is replaced and returns that name. A - /// file past the size bound is not this application's settings and is not worth a copy: that - /// is reported and null returned. A copy left behind by a save that never finished is removed - /// with the other abandoned temporary files. + /// Copies an invalid file to a temporary name before it is replaced and returns that name. + /// The copy is a courtesy, so the save goes ahead without one, with the reason reported and + /// null returned, when the file is past the size bound (it was never this application's + /// settings) or the copy cannot be written. A copy left behind by a save that never finished + /// is removed with the other abandoned temporary files. /// private string? CopyAside(FileStream stored) { @@ -357,11 +358,22 @@ public sealed class AppSettingsStore return null; } string temporary = TemporaryName(); - using var copy = new FileStream(temporary, FileMode.CreateNew, FileAccess.Write, FileShare.None); - stored.Position = 0; - stored.CopyTo(copy); - copy.Flush(flushToDisk: true); - return temporary; + try + { + using(var copy = new FileStream(temporary, FileMode.CreateNew, FileAccess.Write, FileShare.None)) + { + stored.Position = 0; + stored.CopyTo(copy); + copy.Flush(flushToDisk: true); + } + return temporary; + } + catch(Exception exception) when(exception is IOException or UnauthorizedAccessException) + { + TryDelete(temporary); + _report(SettingsFault.NotKeptAside, exception); + return null; + } } @@ -498,11 +510,12 @@ public sealed class AppSettingsStore /// - /// What a move over an existing file fails with while another handle is open on it. Unlike - /// an open or a rename, that move answers access denied, which therefore has to be waited out - /// there even though a read-only file gives the same answer. + /// What a move over the settings file fails with while another handle is open on it. Unlike + /// an open or a rename, that move answers access denied, which therefore has to be waited + /// out. A read-only file gives the same answer for good, so it is told apart by its attribute. /// - private static bool IsBlockedByAHandle(Exception exception) => exception is UnauthorizedAccessException || IsTransient(exception); + private bool IsBlockedByAHandle(Exception exception) + => IsTransient(exception) || (exception is UnauthorizedAccessException && !new FileInfo(Path).IsReadOnly); diff --git a/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs index 05b1e48..86d831e 100644 --- a/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs +++ b/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs @@ -878,6 +878,38 @@ public class AppSettingsStoreTests + /// When the invalid file cannot be copied, here because another program has locked its bytes, the save still goes through, the missing copy is reported and no partial copy is left. + [Fact] + [System.Runtime.Versioning.SupportedOSPlatform("windows")] + public void Update_InvalidFileCannotBeRead_SavesWithoutACopyAndReportsIt() + { + using var temp = new TempDirectory(); + File.WriteAllText(temp.File("settings.json"), "not json"); + var faults = new List<(SettingsFault Fault, Exception? Exception)>(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, exception) => faults.Add((fault, exception))); + FileStream? locker = null; + try + { + Assert.NotNull(store.Update(settings => + { + locker = new FileStream(temp.File("settings.json"), FileMode.Open, FileAccess.Read, FileShare.ReadWrite | FileShare.Delete); + locker.Lock(0, locker.Length); + settings.Theme = "Dark"; + return settings; + })); + } + finally + { + locker?.Dispose(); + } + Assert.Equal([SettingsFault.Corrupt, SettingsFault.NotKeptAside], faults.Select(entry => entry.Fault)); + Assert.IsType(faults[1].Exception); + Assert.Equal("Dark", store.Load().Theme); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); + } + + + /// A file past the size bound is replaced without being copied: copying it would stall the window and fill the disk with something that was never settings. [Fact] public void Update_OversizedFile_IsReplacedWithoutACopy() @@ -936,9 +968,9 @@ public class AppSettingsStoreTests - /// The same move on a read-only file is refused for good: the store gives up after its bounded wait and leaves the file and the folder as they were. + /// The same move on a read-only file is refused for good, which the store sees from the attribute: it gives up at once and leaves the file and the folder as they were. [Fact] - public void Update_FallbackMoveOnAReadOnlyFile_GivesUpAfterTheWait() + public void Update_FallbackMoveOnAReadOnlyFile_FailsWithoutWaiting() { using var temp = new TempDirectory(); Seed(temp); @@ -950,7 +982,7 @@ public class AppSettingsStoreTests var faults = new List<(SettingsFault Fault, Exception? Exception)>(); var store = new AppSettingsStore(temp.File("settings.json"), (fault, exception) => faults.Add((fault, exception)), _ => waits++, (_, _) => false); Assert.Null(store.Update(settings => settings)); - Assert.Equal(AppSettingsStore.MaximumAttempts - 1, waits); + Assert.Equal(0, waits); Assert.Equal(SettingsFault.WriteFailed, Assert.Single(faults).Fault); Assert.IsType(faults[0].Exception); Assert.Equal(before, File.ReadAllBytes(temp.File("settings.json"))); @@ -996,24 +1028,6 @@ public class AppSettingsStoreTests - /// Without a wait supplied, the store really pauses between attempts instead of spinning through them. - [Fact] - public void Update_LockHeld_DefaultWaitPausesBetweenAttempts() - { - using var temp = new TempDirectory(); - Seed(temp); - var store = new AppSettingsStore(temp.File("settings.json")); - using(new FileStream(temp.File("settings.json.lock"), FileMode.Open, FileAccess.ReadWrite, FileShare.None)) - { - long started = System.Diagnostics.Stopwatch.GetTimestamp(); - Assert.Null(store.Update(settings => settings)); - TimeSpan elapsed = System.Diagnostics.Stopwatch.GetElapsedTime(started); - Assert.True(elapsed >= AppSettingsStore.RetryDelay * (AppSettingsStore.MaximumAttempts - 1) / 2, "The store waited only " + elapsed + "."); - } - } - - - /// 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() -- 2.54.0