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

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.

src/MarkdownViewr.Windows.Core/AppSettingsStore.cs

@@ -344,10 +344,11 @@ public sealed class AppSettingsStore
 
 
    /// <summary>
-   /// 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.
    /// </summary>
    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
 
 
    /// <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.
+   /// 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.
    /// </summary>
-   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);
 
 
 

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

@@ -878,6 +878,38 @@ public class AppSettingsStoreTests
 
 
 
+   /// <summary>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.</summary>
+   [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<IOException>(faults[1].Exception);
+      Assert.Equal("Dark", store.Load().Theme);
+      Assert.Equal(["settings.json", "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()
@@ -936,9 +968,9 @@ public class AppSettingsStoreTests
 
 
 
-   /// <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>
+   /// <summary>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.</summary>
    [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<UnauthorizedAccessException>(faults[0].Exception);
          Assert.Equal(before, File.ReadAllBytes(temp.File("settings.json")));
@@ -996,24 +1028,6 @@ public class AppSettingsStoreTests
 
 
 
-   /// <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()