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 <noreply@anthropic.com>
Signed-off-by: James Maes <james@kof22.com>

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.

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).
 

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
 
 
    /// <summary>
-   /// 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.
    /// </summary>
-   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;
+   }
+
+
+
+   /// <summary>
+   /// 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.
+   /// </summary>
+   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
 
 
 
+   /// <summary>A name beside the settings file that only this store uses, so an abandoned one can be recognized and removed.</summary>
+   private string TemporaryName() => Path + "." + Guid.NewGuid().ToString("N") + ".tmp";
+
+
+
    /// <summary>
    /// 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.
    /// </summary>
-   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
 
 
 
-   /// <summary>Repeats an operation only while another handle is in the way, a bounded number of times; the last failure is thrown.</summary>
-   private void Retry(Action operation)
+   /// <summary>
+   /// Repeats an operation only while another handle is in the way, a bounded number of times;
+   /// the last failure is thrown. <paramref name="inTheWay"/> says which failures mean that:
+   /// a sharing violation unless the operation reports an open handle differently.
+   /// </summary>
+   private void Retry(Action operation, Func<Exception, bool>? 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
 
 
    /// <summary>A sharing violation: another handle on the file that is expected to go away. Access denied is not transient.</summary>
-   private static bool IsTransient(IOException exception) => exception.HResult == SharingViolation;
+   private static bool IsTransient(Exception exception) => exception is IOException && exception.HResult == SharingViolation;
+
+
+
+   /// <summary>
+   /// 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.
+   /// </summary>
+   private static bool IsBlockedByAHandle(Exception exception) => exception is UnauthorizedAccessException || IsTransient(exception);
 
 
 

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
    /// </summary>
    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
 
 
 
+   /// <summary>
+   /// 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.
+   /// </summary>
+   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;
+   }
+
+
+
    /// <summary>The errors a volume answers with when it does not implement this kind of rename: invalid function, not supported, invalid parameter.</summary>
    internal static bool IsUnsupported(int error) => error is 1 or 50 or 87;
 

tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs

@@ -850,6 +850,170 @@ public class AppSettingsStoreTests
 
 
 
+   /// <summary>A failed save over invalid content leaves every earlier kept-aside copy as it was, the last one included.</summary>
+   [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<SettingsFault>();
+      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));
+   }
+
+
+
+   /// <summary>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.</summary>
+   [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));
+   }
+
+
+
+   /// <summary>
+   /// 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.
+   /// </summary>
+   [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<TimeSpan>();
+      var faults = new List<SettingsFault>();
+      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));
+   }
+
+
+
+   /// <summary>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.</summary>
+   [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<UnauthorizedAccessException>(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);
+      }
+   }
+
+
+
+   /// <summary>
+   /// 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.
+   /// </summary>
+   [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<SettingsFault>();
+      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));
+   }
+
+
+
+   /// <summary>Without a wait supplied, the store really pauses between attempts instead of spinning through them.</summary>
+   [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 + ".");
+      }
+   }
+
+
+
    /// <summary>A change that throws leaves the file, the lock and the folder as they were; the next save works.</summary>
    [Fact]
    public void Update_ChangeThrows_LeavesTheFileAndReleasesTheLock()

tests/MarkdownViewr.Windows.Core.Tests/FileSwapTests.cs

@@ -9,6 +9,20 @@ namespace MarkdownViewr.Windows.Core.Tests;
 /// <summary>Renaming one real file over another, with other real handles open on the destination.</summary>
 public sealed class FileSwapTests
 {
+   /// <summary>A full path is given the form Windows takes as written: a drive path, a network path, and paths that already have it.</summary>
+   [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));
+   }
+
+
+
    /// <summary>The rename replaces the destination and removes the source name.</summary>
    [Fact]
    public void TryRenameOver_ExistingDestination_ReplacesIt()