fix: keep the change when a storage write fails

CHANGELOG.md

@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Two storages opened through paths that differ only in form (`a/save.dat` and `./a/save.dat`) no longer publish the same file without serializing against each other.
 - A storage larger than 1 MB no longer re-grows its serialization buffer from 1 MB on every save.
 - A save stopped between the two steps of `File.Replace` on Windows no longer loses the data: the next load reads the new file, or the backup if the new file cannot be read.
+- A failed write no longer loses the change: the next change, `Save()` or `Dispose()` writes the data again.
 
 ## [1.0.5]
 - Baseline of the changelog. See the GitHub Releases page for earlier history.

src/Runtime/BinaryStorage.cs

@@ -663,11 +663,11 @@ public virtual void Dispose()
                 return;
             }
 
-            if (AutoSave && _hasUnsavedChanges)
+            if (AutoSave && (_hasUnsavedChanges || !_persistence.Flush()))
             {
                 SaveDataOnDisk(true);
             }
-            else
+            else if (!AutoSave)
             {
                 _persistence.Flush();
             }
@@ -717,7 +717,15 @@ private void LoadDataFromDisk(KeyLoadFailedBehaviour keyLoadFailedBehaviour)
         private void SaveDataOnDisk(bool waitForDisk)
         {
             ThrowIfDisposed();
-            _persistence.Save(_data, waitForDisk);
+            try
+            {
+                _persistence.Save(_data, waitForDisk);
+            }
+            catch
+            {
+                _hasUnsavedChanges = true;
+                throw;
+            }
             _hasUnsavedChanges = false;
         }
 

src/Runtime/Persistence/BackgroundStorageWriter.cs

@@ -19,6 +19,7 @@ internal sealed class BackgroundStorageWriter : IStorageWriter
         private StorageSnapshot? _pending;
         private bool _isScheduled;
         private bool _isPublishing;
+        private bool _lastWriteFailed;
 
         public BackgroundStorageWriter(StorageFile file)
         {
@@ -32,17 +33,19 @@ public void Write(StorageSnapshot snapshot, bool waitForDisk)
                 Schedule(snapshot);
                 return;
             }
-            TakePending()?.Release();
+            TakePending(out _)?.Release();
             _file.Publish(snapshot);
         }
 
-        public void Flush()
+        public bool Flush()
         {
-            var pending = TakePending();
-            if (pending != null)
+            var pending = TakePending(out var lastWriteFailed);
+            if (pending == null)
             {
-                _file.Publish(pending.Value);
+                return !lastWriteFailed;
             }
+            _file.Publish(pending.Value);
+            return true;
         }
 
         private void Schedule(StorageSnapshot snapshot)
@@ -62,14 +65,16 @@ private void Schedule(StorageSnapshot snapshot)
             replaced?.Release();
         }
 
-        private StorageSnapshot? TakePending()
+        private StorageSnapshot? TakePending(out bool lastWriteFailed)
         {
             lock (_lock)
             {
                 while (_isPublishing)
                 {
                     Monitor.Wait(_lock);
                 }
+                lastWriteFailed = _lastWriteFailed;
+                _lastWriteFailed = false;
                 var pending = _pending;
                 _pending = null;
                 return pending;
@@ -91,18 +96,21 @@ private void PublishScheduled()
                 _isPublishing = true;
             }
 
+            var failed = false;
             try
             {
                 _file.Publish(snapshot);
             }
             catch (Exception exception)
             {
+                failed = true;
                 Debug.LogError($"Failed to save storage '{_file.Main}'. Reason: {exception.Message}");
             }
             finally
             {
                 lock (_lock)
                 {
+                    _lastWriteFailed = failed;
                     _isPublishing = false;
                     Monitor.PulseAll(_lock);
                 }

src/Runtime/Persistence/IStorageWriter.cs

@@ -4,6 +4,6 @@ internal interface IStorageWriter
     {
         void Write(StorageSnapshot snapshot, bool waitForDisk);
 
-        void Flush();
+        bool Flush();
     }
 }

src/Runtime/Persistence/ImmediateStorageWriter.cs

@@ -14,8 +14,9 @@ public void Write(StorageSnapshot snapshot, bool waitForDisk)
             _file.Publish(snapshot);
         }
 
-        public void Flush()
+        public bool Flush()
         {
+            return true;
         }
     }
 }

src/Runtime/Persistence/StoragePersistence.cs

@@ -35,9 +35,9 @@ public void Save(Dictionary<string, Record> data, bool waitForDisk)
             }
         }
 
-        public void Flush()
+        public bool Flush()
         {
-            _writer.Flush();
+            return _writer.Flush();
         }
 
         private void SaveJsonCopy(IReadOnlyDictionary<string, Record> data)

src/Tests/Persistence/FailedWriteTests.cs

@@ -0,0 +1,68 @@
+using System;
+using System.IO;
+using System.Text.RegularExpressions;
+using System.Threading;
+using FluentAssertions;
+using NUnit.Framework;
+using UnityEngine;
+using UnityEngine.TestTools;
+
+namespace Appegy.Storage
+{
+    public class FailedWriteTests : BaseStorageTests
+    {
+        [TearDown]
+        public void UnblockWrites()
+        {
+            if (Directory.Exists(TempPath))
+            {
+                Directory.Delete(TempPath);
+            }
+        }
+
+        [Test]
+        public void WhenBackgroundWriteFails_ThenDisposeWritesTheChange()
+        {
+            var storage = Open(autoSave: true);
+            Directory.CreateDirectory(TempPath);
+            using (var writeFailed = new ManualResetEventSlim())
+            {
+                void OnLog(string message, string stackTrace, LogType type)
+                {
+                    if (type == LogType.Error && message.StartsWith("Failed to save storage"))
+                    {
+                        writeFailed.Set();
+                    }
+                }
+
+                LogAssert.Expect(LogType.Error, new Regex("Failed to save storage"));
+                Application.logMessageReceivedThreaded += OnLog;
+                storage.Set("value", 7);
+                var failed = writeFailed.Wait(TimeSpan.FromSeconds(10));
+                Application.logMessageReceivedThreaded -= OnLog;
+                failed.Should().BeTrue();
+            }
+
+            Directory.Delete(TempPath);
+            storage.Dispose();
+
+            using var reopened = Open();
+            reopened.Get<int>("value").Should().Be(7);
+        }
+
+        [Test]
+        public void WhenAutoSaveWriteFails_ThenDisposeWritesTheChange()
+        {
+            var storage = Open(autoSave: true, saveOnBackgroundThread: false);
+            Directory.CreateDirectory(TempPath);
+
+            storage.Invoking(s => s.Set("value", 7)).Should().Throw<Exception>();
+
+            Directory.Delete(TempPath);
+            storage.Dispose();
+
+            using var reopened = Open();
+            reopened.Get<int>("value").Should().Be(7);
+        }
+    }
+}