From 17224bb265dce3a187de74b8d9b4ac7a6a9d14d8 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 22 Aug 2026 17:33:57 +1000 Subject: [PATCH 1/2] Replace settings.json rather than rewriting it in place Write deleted the file and then opened it again to serialise into, so between the two there was no settings.json at all, and until the serialise finished there was a partial one. A kill anywhere in there - a logoff, a reboot, task manager - left the tray meeting "Cannot start. Failed to read settings" at every launch afterwards, with deleting the file by hand the only way out. It is now serialised beside the file and moved over it, so the live path only ever holds a whole document. The swap can lose a race with anything holding the file open, which the old delete could not, so it is retried for a fifth of a second before giving up. That path used to throw straight out of an async void click handler. WriteFile is the file half on its own. Write also persists two User scope environment variables, a registry write and a broadcast each, which the test that hammers this would spend all its time on. --- .../SettingsWriteTests.cs | 127 ++++++++++++++++++ src/DiffEngineTray/Settings/SettingsHelper.cs | 49 ++++++- 2 files changed, 173 insertions(+), 3 deletions(-) create mode 100644 src/DiffEngineTray.Tests/SettingsWriteTests.cs diff --git a/src/DiffEngineTray.Tests/SettingsWriteTests.cs b/src/DiffEngineTray.Tests/SettingsWriteTests.cs new file mode 100644 index 00000000..2ccd9ab0 --- /dev/null +++ b/src/DiffEngineTray.Tests/SettingsWriteTests.cs @@ -0,0 +1,127 @@ +using System.Text.Json; + +/// +/// What the settings file looks like from outside while it is being written. The tray reads it at +/// startup and fails to start when it cannot, so a save that leaves it absent or half written for +/// any length of time is a save that can end with a "Cannot start" dialog at every launch. +/// +/// rather than , which +/// also persists two User scope environment variables - a registry write and a broadcast each, +/// which this many iterations of would take far longer than the thing under test. +/// +/// +public class SettingsWriteTests : + IDisposable +{ + [Test] + public async Task The_file_is_never_observed_missing() + { + await SettingsHelper.WriteFile(new()); + + using var cancellation = new CancelSource(); + var reading = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var missing = 0; + var unreadable = new ConcurrentBag(); + var reader = Task.Run( + () => + { + reading.SetResult(true); + while (!cancellation.IsCancellationRequested) + { + string text; + try + { + // Sharing everything, including delete, so that reading the file cannot + // itself be what makes a write fail + using var stream = new FileStream( + file, + FileMode.Open, + FileAccess.Read, + FileShare.ReadWrite | FileShare.Delete); + using var streamReader = new StreamReader(stream); + text = streamReader.ReadToEnd(); + } + catch (FileNotFoundException) + { + Interlocked.Increment(ref missing); + continue; + } + catch (Exception exception) + when (exception is IOException or UnauthorizedAccessException) + { + // The OS asking to come back, rather than the file being damaged + continue; + } + + if (!CanBeRead(text)) + { + unreadable.Add(text); + } + } + }); + + await reading.Task; + + for (var index = 0; index < 200; index++) + { + await SettingsHelper.WriteFile( + new() + { + AlwaysKillLockingProcesses = index % 2 == 0 + }); + } + + await cancellation.CancelAsync(); + await reader; + + await Assert.That(missing).IsEqualTo(0); + await Assert.That(unreadable).IsEmpty(); + } + + [Test] + public async Task Leaves_nothing_beside_the_file() + { + await SettingsHelper.WriteFile(new()); + + var beside = Directory.GetFiles(directory); + + await Assert.That(beside).HasSingleItem(); + await Assert.That(beside[0]).IsEqualTo(file); + } + + static bool CanBeRead(string text) + { + if (text.Length == 0) + { + return false; + } + + try + { + return JsonSerializer.Deserialize(text) != null; + } + catch (JsonException) + { + return false; + } + } + + public SettingsWriteTests() + { + originalPath = SettingsHelper.FilePath; + directory = Path.Combine(Path.GetTempPath(), $"SettingsWriteTests_{Guid.NewGuid()}"); + Directory.CreateDirectory(directory); + file = Path.Combine(directory, "settings.json"); + SettingsHelper.FilePath = file; + } + + public void Dispose() + { + SettingsHelper.FilePath = originalPath; + Directory.Delete(directory, true); + } + + string originalPath; + string directory; + string file; +} diff --git a/src/DiffEngineTray/Settings/SettingsHelper.cs b/src/DiffEngineTray/Settings/SettingsHelper.cs index 5c08cb1f..66f7cc6c 100644 --- a/src/DiffEngineTray/Settings/SettingsHelper.cs +++ b/src/DiffEngineTray/Settings/SettingsHelper.cs @@ -33,8 +33,51 @@ public static async Task Write(Settings settings) { TargetPosition.SetTargetOnLeft(settings.TargetOnLeft); MaxInstance.SetForUser(settings.MaxInstancesToLaunch); - File.Delete(FilePath); - await using var stream = File.OpenWrite(FilePath); - await JsonSerializer.SerializeAsync(stream, settings); + await WriteFile(settings); + } + + /// + /// The file half of , which is the half that has to survive being + /// interrupted. + /// + /// Serialised beside the settings file and moved over it, rather than written in place. The + /// file used to be deleted and then rewritten, so a kill in the window between the two - or + /// part way through serialising - left no file at all, or a truncated one, and every launch + /// after that met "Cannot start. Failed to read settings" until it was deleted by hand. + /// + /// + internal static async Task WriteFile(Settings settings) + { + var temp = $"{FilePath}.tmp"; + + await using (var stream = File.Create(temp)) + { + await JsonSerializer.SerializeAsync(stream, settings); + } + + await Swap(temp); + } + + /// + /// The swap can lose a race with anything holding the settings file open - the tray reading it + /// at startup, a backup, an indexer - and losing it throws out of an async void click handler. + /// Whoever has it will not have it for long, so the save waits rather than failing. + /// + static async Task Swap(string temp) + { + for (var attempt = 0; ; attempt++) + { + try + { + File.Move(temp, FilePath, true); + return; + } + catch (Exception exception) + when (attempt < 10 && + exception is IOException or UnauthorizedAccessException) + { + await Task.Delay(20); + } + } } } \ No newline at end of file From 3d3e61a1df017e8f47a815d8a74381a89968c56b Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 22 Aug 2026 17:48:08 +1000 Subject: [PATCH 2/2] Have the watching reader sample the file rather than spin on it It reopened settings.json the instant it closed it, so it held the file for most of the time it ran. On the two core Windows runner that starved the swap of all eleven attempts it has and failed the test on the writer's side, which is the test being an adversary no real reader is. It now looks once a millisecond, and runs until it has looked a hundred times rather than for a fixed number of writes, so the number of independent samples does not depend on how fast the machine writes. A missing file is counted as a look too - without that the run ended on the write cap with too few looks to assert on, and the unfixed write failed on the guard rather than on what it was there to catch. --- .../SettingsWriteTests.cs | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/src/DiffEngineTray.Tests/SettingsWriteTests.cs b/src/DiffEngineTray.Tests/SettingsWriteTests.cs index 2ccd9ab0..7251c530 100644 --- a/src/DiffEngineTray.Tests/SettingsWriteTests.cs +++ b/src/DiffEngineTray.Tests/SettingsWriteTests.cs @@ -21,6 +21,7 @@ public async Task The_file_is_never_observed_missing() using var cancellation = new CancelSource(); var reading = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); var missing = 0; + var looks = 0; var unreadable = new ConcurrentBag(); var reader = Task.Run( () => @@ -28,7 +29,6 @@ public async Task The_file_is_never_observed_missing() reading.SetResult(true); while (!cancellation.IsCancellationRequested) { - string text; try { // Sharing everything, including delete, so that reading the file cannot @@ -39,30 +39,37 @@ public async Task The_file_is_never_observed_missing() FileAccess.Read, FileShare.ReadWrite | FileShare.Delete); using var streamReader = new StreamReader(stream); - text = streamReader.ReadToEnd(); + var text = streamReader.ReadToEnd(); + if (!CanBeRead(text)) + { + unreadable.Add(text); + } } catch (FileNotFoundException) { Interlocked.Increment(ref missing); - continue; } catch (Exception exception) when (exception is IOException or UnauthorizedAccessException) { // The OS asking to come back, rather than the file being damaged - continue; } - if (!CanBeRead(text)) - { - unreadable.Add(text); - } + Interlocked.Increment(ref looks); + + // Sampling rather than spinning. A reader that reopens the file the instant it + // closes it holds it for most of the time it runs, which on a two core CI + // machine starves the swap of every attempt it has - and that is the test + // being the adversary, not the file being fragile + Thread.Sleep(1); } }); await reading.Task; - for (var index = 0; index < 200; index++) + // Until the reader has looked enough times for the window to have shown itself, rather + // than a count of writes, which on a fast machine all pass between two of its looks + for (var index = 0; Volatile.Read(ref looks) < 100 && index < 5000; index++) { await SettingsHelper.WriteFile( new() @@ -74,6 +81,8 @@ await SettingsHelper.WriteFile( await cancellation.CancelAsync(); await reader; + // That the reader looked at all. Everything below is a statement about what it saw + await Assert.That(looks).IsGreaterThanOrEqualTo(100); await Assert.That(missing).IsEqualTo(0); await Assert.That(unreadable).IsEmpty(); }