From f472334f09ce3e85be7baa7f73f4c19d7349d5fa Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 17 Aug 2026 02:23:17 +0000 Subject: [PATCH] fix(config): replace Settings.json atomically instead of truncating it File.WriteAllText truncates the destination before writing, so an interrupted write leaves a half-written or empty Settings.json, and the in-process lock added in the previous commit cannot help a reader in another process - the GUI and the CLI share this file. Route every write through Dinah.Core.IO.AtomicFileWriter, which writes a sibling temp file, flushes to disk and renames it over the destination. Validate the temp file parses as json before the swap, the same way JsonFilePersister already saves AccountsSettings.json, so a bad payload leaves the existing file untouched. Co-authored-by: rmcrackan --- Source/FileManager/PersistentDictionary.cs | 31 +++++++-- .../PersistentDictionaryConcurrencyTests.cs | 66 +++++++++++++++++++ 2 files changed, 92 insertions(+), 5 deletions(-) diff --git a/Source/FileManager/PersistentDictionary.cs b/Source/FileManager/PersistentDictionary.cs index a148abf4..2a40a4fc 100644 --- a/Source/FileManager/PersistentDictionary.cs +++ b/Source/FileManager/PersistentDictionary.cs @@ -1,4 +1,5 @@ -using Newtonsoft.Json; +using Dinah.Core.IO; +using Newtonsoft.Json; using Newtonsoft.Json.Linq; using System; using System.Collections.Generic; @@ -21,6 +22,9 @@ public class PersistentDictionary : IJsonBackedDictionary // the UI thread, BackgroundWorker callbacks and download workers simultaneously. Every cache and // file access below must be serialized: unsynchronized Dictionary writes corrupt the cache, and // unsynchronized file access lets a reader observe a half-written file. + // This lock cannot reach a second process (the GUI and the CLI share Settings.json), which is why + // every write goes through AtomicFileWriter: an outside reader sees either the old file or the new + // one, never a truncated one. private Lock locker { get; } = new(); public PersistentDictionary(string filepath, bool isReadOnly = false) @@ -182,7 +186,7 @@ public class PersistentDictionary : IJsonBackedDictionary var endContents = JsonConvert.SerializeObject(jObject, Formatting.Indented); - File.WriteAllText(Filepath, endContents); + writeFileContents(endContents); success = true; } Serilog.Log.Logger.Information("Removed property. {propertyName}", propertyName); @@ -209,7 +213,7 @@ public class PersistentDictionary : IJsonBackedDictionary if (startContents == endContents) return false; - File.WriteAllText(Filepath, endContents); + writeFileContents(endContents); return true; } @@ -252,7 +256,7 @@ public class PersistentDictionary : IJsonBackedDictionary return false; token[propertyName] = newValue; - File.WriteAllText(Filepath, JsonConvert.SerializeObject(jObject, Formatting.Indented)); + writeFileContents(JsonConvert.SerializeObject(jObject, Formatting.Indented)); } } catch (Exception exDebug) @@ -281,6 +285,23 @@ public class PersistentDictionary : IJsonBackedDictionary : value.Length > 100 ? $"[Length={value.Length}] {value[0..50]}...{value[^50..^0]}" : value; + /// + /// Replaces the settings file in one step, so a concurrent reader - including one in another + /// Libation process - sees either the whole old file or the whole new one. Mirrors how + /// saves AccountsSettings.json. + /// + private void writeFileContents(string contents) + => AtomicFileWriter.WriteAllText(Filepath, contents, validateJsonTempFile); + + /// Throws before the temp file replaces the real one, leaving the real one untouched. + private static void validateJsonTempFile(string tempPath) + { + var contents = File.ReadAllText(tempPath); + if (string.IsNullOrWhiteSpace(contents)) + throw new JsonSerializationException($"Refusing to write an empty settings file to {tempPath}"); + JToken.Parse(contents); + } + /// Caller must hold . private JObject readFile() { @@ -315,7 +336,7 @@ public class PersistentDictionary : IJsonBackedDictionary private void createNewFile() { - File.WriteAllText(Filepath, "{}"); + writeFileContents("{}"); } public JObject GetJObject() diff --git a/Source/_Tests/FileManager.Tests/PersistentDictionaryConcurrencyTests.cs b/Source/_Tests/FileManager.Tests/PersistentDictionaryConcurrencyTests.cs index 215896a0..8e86aa49 100644 --- a/Source/_Tests/FileManager.Tests/PersistentDictionaryConcurrencyTests.cs +++ b/Source/_Tests/FileManager.Tests/PersistentDictionaryConcurrencyTests.cs @@ -4,6 +4,7 @@ using System.Collections.Generic; using System.IO; using System.Threading; using Microsoft.VisualStudio.TestTools.UnitTesting; +using Newtonsoft.Json; using Newtonsoft.Json.Linq; namespace FileManager.Tests; @@ -195,6 +196,71 @@ public class PersistentDictionaryConcurrencyTests } } + /// + /// The lock cannot reach a second process - the GUI and the CLI share one Settings.json - so a + /// write must never leave the file truncated. Reading it straight off disk, bypassing the + /// dictionary, stands in for that outside reader. + /// + [TestMethod] + public void ExternalReaderNeverSeesAPartiallyWrittenFile() + { + var file = createSettingsFile(); + try + { + var dictionary = new PersistentDictionary(file); + + // a payload big enough that a non-atomic write has a window to be caught mid-flight + var padding = new string('x', 64 * 1024); + var done = false; + + runConcurrently(thread => + { + if (thread == 0) + { + try + { + for (var i = 0; i < Keys; i++) + dictionary.SetString("Padded", $"{padding}{i}"); + } + finally + { + Volatile.Write(ref done, true); + } + return; + } + + while (!Volatile.Read(ref done)) + { + string contents; + try + { + // share the file the way a cooperative outside reader would, so an atomic + // replace is never blocked by this test + using var stream = new FileStream(file, FileMode.Open, FileAccess.Read, FileShare.ReadWrite | FileShare.Delete); + using var reader = new StreamReader(stream); + contents = reader.ReadToEnd(); + } + catch (IOException) + { + // a sharing violation is the OS refusing the read, not a corrupt file + continue; + } + + Assert.IsFalse(string.IsNullOrWhiteSpace(contents), "read an empty Settings.json mid-write"); + // throws JsonReaderException on a truncated file + Assert.IsNotNull(JsonConvert.DeserializeObject(contents)); + } + }); + + // no temp files left behind + Assert.AreEqual(1, Directory.GetFiles(Path.GetDirectoryName(file)!).Length); + } + finally + { + deleteSettingsFile(file); + } + } + [TestMethod] public void ReadWhileWriting_DoesNotThrow() {