diff --git a/AppDataStorage.Test/AppDataTests.cs b/AppDataStorage.Test/AppDataTests.cs index b147f03..b63803e 100644 --- a/AppDataStorage.Test/AppDataTests.cs +++ b/AppDataStorage.Test/AppDataTests.cs @@ -260,6 +260,54 @@ public void TestReadTextRestoresFromBackupIfMainFileMissing() AssertFileExists(appData.FilePath, "Main file should be restored from backup."); } + // The on-disk state left by a process killed between WriteText's Delete of the main file and its + // Move of the temp file into place: main gone, temp holding the save that was in flight, backup + // holding the content before it. + private static void SetUpInterruptedWrite(TestAppData appData, string inFlight, string previous) + { + AppData.FileSystem.File.WriteAllText(AppData.MakeTempFilePath(appData.FilePath), inFlight); + AppData.FileSystem.File.WriteAllText(AppData.MakeBackupFilePath(appData.FilePath), previous); + AppData.FileSystem.File.Delete(appData.FilePath); + } + + [TestMethod] + public void TestReadTextRecoversTheInterruptedWriteRatherThanTheStaleBackup() + { + const string inFlight = "Newest data that was being saved"; + const string previous = "Original data"; + using TestAppData appData = CreateTestAppDataWithContent(TestDataString); + AppData.WriteText(appData, TestDataString); + SetUpInterruptedWrite(appData, inFlight, previous); + + string text = AppData.ReadText(appData); + + Assert.AreEqual(inFlight, text, "The newest save should be recovered, not the stale backup."); + AssertFileExists(appData.FilePath, "Main file should be restored from the interrupted write."); + } + + [TestMethod] + public void TestReadTextArchivesTheRecoveredTempFileRatherThanLeavingItOrphaned() + { + using TestAppData appData = CreateTestAppDataWithContent(TestDataString); + AppData.WriteText(appData, TestDataString); + SetUpInterruptedWrite(appData, "Newest data that was being saved", "Original data"); + AbsoluteFilePath tempFilePath = AppData.MakeTempFilePath(appData.FilePath); + + AppData.ReadText(appData); + + Assert.IsFalse( + AppData.FileSystem.File.Exists(tempFilePath), + "The consumed temp file should not be left orphaned on disk."); + + string directory = AppData.FileSystem.Path.GetDirectoryName(appData.FilePath)!; + string tempFileName = AppData.FileSystem.Path.GetFileName(tempFilePath); + Assert.IsTrue( + Array.Exists( + AppData.FileSystem.Directory.GetFiles(directory), + file => AppData.FileSystem.Path.GetFileName(file).StartsWith(tempFileName + ".", StringComparison.Ordinal)), + "The recovered temp file should be archived under a timestamped name, not deleted."); + } + [TestMethod] public void TestQueueSaveSetsSaveQueuedTime() { diff --git a/AppDataStorage/AppData.cs b/AppDataStorage/AppData.cs index 9eeaa5f..52290b3 100644 --- a/AppDataStorage/AppData.cs +++ b/AppDataStorage/AppData.cs @@ -186,22 +186,14 @@ internal static void EnsureDirectoryExists(AbsoluteDirectoryPath path) } catch (FileNotFoundException) { - AbsoluteFilePath bkFilePath = MakeBackupFilePath(appData.FilePath); - if (FileSystem.File.Exists(bkFilePath)) + // WriteText deletes the main file before moving the temp file into its place. A + // process killed in that window leaves the main file gone, the temp file holding the + // content of the save that was in flight, and the backup holding the content before + // it. The temp file is therefore the newer of the two candidates and is tried first; + // recovering the backup instead silently discards the newest save. + if (TryRestoreFrom(MakeTempFilePath(appData.FilePath), appData.FilePath) + || TryRestoreFrom(MakeBackupFilePath(appData.FilePath), appData.FilePath)) { - FileSystem.File.Copy(bkFilePath, appData.FilePath); - - // Create a unique timestamped backup filename - string timestamp = DateTime.Now.ToString("yyyyMMdd_HHmmss"); - AbsoluteFilePath timestampedBackup = bkFilePath.WithSuffix($".{timestamp}"); - int counter = 0; - while (FileSystem.File.Exists(timestampedBackup)) - { - counter++; - timestampedBackup = bkFilePath.WithSuffix($".{timestamp}_{counter}"); - } - - FileSystem.File.Move(bkFilePath, timestampedBackup); return ReadText(appData); } } @@ -210,6 +202,39 @@ internal static void EnsureDirectoryExists(AbsoluteDirectoryPath path) } } + /// + /// Restores from if the candidate exists, + /// then archives the candidate under a unique timestamped name. + /// + /// The recovery candidate to restore from. + /// The app data file path to restore. + /// True if the candidate existed and the file was restored; otherwise false. + private static bool TryRestoreFrom(AbsoluteFilePath candidate, AbsoluteFilePath filePath) + { + if (!FileSystem.File.Exists(candidate)) + { + return false; + } + + FileSystem.File.Copy(candidate, filePath); + + // Archiving rather than deleting keeps the recovered content for inspection, and moving it + // out of the way is what stops a candidate that turns out to be unreadable from being + // promoted again on the next attempt: LoadOrCreate deletes a file it cannot deserialize and + // reads again, which would otherwise restore the same bad content forever. + string timestamp = DateTime.Now.ToString("yyyyMMdd_HHmmss"); + AbsoluteFilePath archived = candidate.WithSuffix($".{timestamp}"); + int counter = 0; + while (FileSystem.File.Exists(archived)) + { + counter++; + archived = candidate.WithSuffix($".{timestamp}_{counter}"); + } + + FileSystem.File.Move(candidate, archived); + return true; + } + /// /// Queues a save operation for the current app data instance. ///