From 12d4ca442759c06e4934863ae3e4ae09c2ce366f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 14 Oct 2026 12:39:40 +0000 Subject: [PATCH 1/3] Make file-backed StoreAsync safe under concurrent writers and readers [patch] FileSystemPersistenceProvider and TempPersistenceProvider staged every write in the same ".tmp" and then deleted the target before moving the temp file into place. Overlapping writers to one key moved each other's temp file away and threw. Between the delete and the move, a reader saw the key as missing, so RetrieveOrCreateAsync could store a fresh default over the real record. Both providers now write through Shared/AtomicFileWrite. Each write uses its own ".tmp" in the target's directory, and the swap is a single overwriting move. netstandard2.1 lacks that overload, so it uses File.Replace, or File.Move when the target does not exist yet. A short, bounded retry covers Windows refusing to replace a file another handle has open, and the staging file is always cleaned up. The staging name is not derived from the key, because filenames are budgeted for only the extension plus ".tmp". A longer suffix pushes long keys past the 255-byte name limit. Fixes #66 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01YLmoTmY5N7amW8Z9jazuGV --- CLAUDE.md | 1 + ...als.PersistenceProviders.FileSystem.csproj | 1 + .../FileSystemPersistenceProvider.cs | 11 +-- ...ssentials.PersistenceProviders.Temp.csproj | 1 + .../TempPersistenceProvider.cs | 11 +-- Essentials.Tests/PersistenceProviderTests.cs | 74 +++++++++++++++++ Shared/AtomicFileWrite.cs | 83 +++++++++++++++++++ 7 files changed, 162 insertions(+), 20 deletions(-) create mode 100644 Shared/AtomicFileWrite.cs diff --git a/CLAUDE.md b/CLAUDE.md index 50d5822..d3d69e1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -48,6 +48,7 @@ This is a .NET library (`ktsu.Essentials`) providing high-performance interfaces - `Shared/SpecialFunctions.cs` - Incomplete gamma, incomplete beta, error function, normal quantile and cancellation-free `ExpM1`/`Log1P`, linked into the Normal, LogNormal, Exponential, Binomial, Poisson and Geometric providers rather than placed in the interfaces-only package - `Shared/DistributionArguments.cs` - Parameter validation shared by every distribution provider, linked in the same way - `Shared/PersistenceDefaults.cs` - The `IPersistenceProvider` default `RetrieveOrCreateAsync` body, linked into the FileSystem and Temp providers, which declare that member themselves and so hide the default +- `Shared/AtomicFileWrite.cs` - The FileSystem and Temp providers' write path: each store stages its data in a uniquely named `.tmp` beside the target and publishes it with an overwriting move, retrying briefly while Windows holds the target open, so concurrent writers can't collide and a reader never sees the key missing. Linked into both providers like `PersistenceDefaults.cs` - `Essentials/ISerializationProvider.cs` - Object serialization/deserialization interface - `Essentials/ISerializationOptions.cs` - Configurable serialization options (naming, inclusion, boxing policies) - `Essentials/ICacheProvider.cs` - Generic cache interface with expiration and get-or-add diff --git a/Essentials.PersistenceProviders.FileSystem/Essentials.PersistenceProviders.FileSystem.csproj b/Essentials.PersistenceProviders.FileSystem/Essentials.PersistenceProviders.FileSystem.csproj index 2f1340e..b867528 100644 --- a/Essentials.PersistenceProviders.FileSystem/Essentials.PersistenceProviders.FileSystem.csproj +++ b/Essentials.PersistenceProviders.FileSystem/Essentials.PersistenceProviders.FileSystem.csproj @@ -16,6 +16,7 @@ + diff --git a/Essentials.PersistenceProviders.FileSystem/FileSystemPersistenceProvider.cs b/Essentials.PersistenceProviders.FileSystem/FileSystemPersistenceProvider.cs index c89b913..21099e6 100644 --- a/Essentials.PersistenceProviders.FileSystem/FileSystemPersistenceProvider.cs +++ b/Essentials.PersistenceProviders.FileSystem/FileSystemPersistenceProvider.cs @@ -68,16 +68,7 @@ public async Task StoreAsync(TKey key, T obj, CancellationToken cancellationT _fileSystemProvider.Directory.CreateDirectory(directory); } - // Write to temporary file first, then move for atomic operation - string tempFilePath = filePath + ".tmp"; - await _fileSystemProvider.File.WriteAllTextAsync(tempFilePath, serializedData, cancellationToken).ConfigureAwait(false); - - // Atomic move - if (_fileSystemProvider.File.Exists(filePath)) - { - _fileSystemProvider.File.Delete(filePath); - } - _fileSystemProvider.File.Move(tempFilePath, filePath); + await AtomicFileWrite.WriteAllTextAsync(_fileSystemProvider, filePath, serializedData, cancellationToken).ConfigureAwait(false); } catch (Exception ex) { diff --git a/Essentials.PersistenceProviders.Temp/Essentials.PersistenceProviders.Temp.csproj b/Essentials.PersistenceProviders.Temp/Essentials.PersistenceProviders.Temp.csproj index 2f1340e..b867528 100644 --- a/Essentials.PersistenceProviders.Temp/Essentials.PersistenceProviders.Temp.csproj +++ b/Essentials.PersistenceProviders.Temp/Essentials.PersistenceProviders.Temp.csproj @@ -16,6 +16,7 @@ + diff --git a/Essentials.PersistenceProviders.Temp/TempPersistenceProvider.cs b/Essentials.PersistenceProviders.Temp/TempPersistenceProvider.cs index 3649650..ee62315 100644 --- a/Essentials.PersistenceProviders.Temp/TempPersistenceProvider.cs +++ b/Essentials.PersistenceProviders.Temp/TempPersistenceProvider.cs @@ -65,16 +65,7 @@ public async Task StoreAsync(TKey key, T obj, CancellationToken cancellationT _fileSystemProvider.Directory.CreateDirectory(directory); } - // Write to temporary file first, then move for atomic operation - string tempFilePath = filePath + ".tmp"; - await _fileSystemProvider.File.WriteAllTextAsync(tempFilePath, serializedData, cancellationToken).ConfigureAwait(false); - - // Atomic move - if (_fileSystemProvider.File.Exists(filePath)) - { - _fileSystemProvider.File.Delete(filePath); - } - _fileSystemProvider.File.Move(tempFilePath, filePath); + await AtomicFileWrite.WriteAllTextAsync(_fileSystemProvider, filePath, serializedData, cancellationToken).ConfigureAwait(false); } catch (Exception ex) { diff --git a/Essentials.Tests/PersistenceProviderTests.cs b/Essentials.Tests/PersistenceProviderTests.cs index 696c51e..8dfe3a8 100644 --- a/Essentials.Tests/PersistenceProviderTests.cs +++ b/Essentials.Tests/PersistenceProviderTests.cs @@ -636,6 +636,80 @@ public async Task Store_Null_Removes_The_Key_On_Every_Provider() } } + [TestMethod] + public async Task Concurrent_Stores_To_One_Key_Never_Throw_Or_Hide_The_Key() + { + // Every writer staged its data in the same ".tmp", so overlapping writers moved each other's file + // away and threw, and the delete-then-move left a window in which a reader saw the key as missing + // (issue #66). + string fileSystemDir = Path.Join(Path.GetTempPath(), "PersistenceTests_FS_" + Guid.NewGuid().ToString("N")[..8]); + NativeFileSystemProvider fs = new(); + JsonSerializationProvider serializer = new(); + using TempPersistenceProvider temp = new(fs, serializer, "PersistenceTests_Temp_" + Guid.NewGuid().ToString("N")[..8]); + + try + { + IPersistenceProvider[] providers = + [ + new FileSystemPersistenceProvider(fs, serializer, fileSystemDir), + temp, + ]; + + foreach (IPersistenceProvider persistence in providers) + { + string name = persistence.ProviderName; + const int writers = 200; + + await persistence.StoreAsync("k", new TestData { Name = "initial" }, TestContext.CancellationToken).ConfigureAwait(false); + + using CancellationTokenSource stopReading = new(); + int missing = 0; + Task reader = Task.Run(async () => + { + while (!stopReading.IsCancellationRequested) + { + if (!await persistence.ExistsAsync("k", TestContext.CancellationToken).ConfigureAwait(false) + || await persistence.RetrieveAsync("k", TestContext.CancellationToken).ConfigureAwait(false) is null) + { + Interlocked.Increment(ref missing); + } + } + }, TestContext.CancellationToken); + + Task[] stores = [.. Enumerable.Range(0, writers).Select(i => Task.Run( + () => persistence.StoreAsync("k", new TestData { Name = $"writer {i}" }, TestContext.CancellationToken), + TestContext.CancellationToken))]; + + try + { + await Task.WhenAll(stores).ConfigureAwait(false); + } + finally + { + await stopReading.CancelAsync().ConfigureAwait(false); + await reader.ConfigureAwait(false); + } + + Assert.AreEqual(0, missing, $"{name} reported a stored key as missing during concurrent writes"); + + TestData? final = await persistence.RetrieveAsync("k", TestContext.CancellationToken).ConfigureAwait(false); + Assert.IsNotNull(final); + CollectionAssert.Contains(Enumerable.Range(0, writers).Select(i => $"writer {i}").ToArray(), final.Name, $"{name} published a payload no writer stored"); + + string[] keys = [.. await persistence.GetAllKeysAsync(TestContext.CancellationToken).ConfigureAwait(false)]; + Assert.HasCount(1, keys, $"{name} listed a staging file as a key"); + Assert.AreEqual("k", keys[0]); + } + + Assert.IsEmpty(Directory.GetFiles(fileSystemDir, "*.tmp"), "A staging file was left behind"); + } + finally + { + CleanupDirectory(fileSystemDir); + temp.CleanupDirectory(); + } + } + public record struct ValueData() { public int Value { get; set; } = 50; diff --git a/Shared/AtomicFileWrite.cs b/Shared/AtomicFileWrite.cs new file mode 100644 index 0000000..63dc004 --- /dev/null +++ b/Shared/AtomicFileWrite.cs @@ -0,0 +1,83 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.Essentials; + +using System; +using System.IO; +using System.IO.Abstractions; +using System.Threading; +using System.Threading.Tasks; + +/// +/// Replaces a file's content in one step, so a reader sees either the old content or the new, never neither. +/// +/// +/// Linked into the FileSystem and Temp providers rather than placed in the interfaces package, following +/// PersistenceDefaults. Each write stages its content in a temporary file of its own, so overlapping +/// writers to one key cannot move each other's file away, and publishes it with an overwriting move rather +/// than a delete followed by a move, so the target never disappears in between (issue #66). +/// +internal static class AtomicFileWrite +{ + private const int MaxMoveAttempts = 50; + + /// + /// Writes to , replacing any existing file atomically. + /// + /// The filesystem to write through. + /// The file to create or replace. + /// The text to write. + /// A token to cancel the operation. + /// A task that completes once the new content is in place. + internal static async Task WriteAllTextAsync(IFileSystem fileSystem, string filePath, string content, CancellationToken cancellationToken) + { + // Same directory as the target, so the move is a rename on one volume. The name is not derived from + // the target's, because PersistenceProviderUtilities budgets a key's filename for only the extension + // and ".tmp", and a longer suffix on a long key would exceed the 255-byte name limit. The .tmp + // extension keeps the staging file out of the providers' "*" key listings. + string? directory = fileSystem.Path.GetDirectoryName(filePath); + string tempFileName = $"{Guid.NewGuid():N}.tmp"; + string tempFilePath = string.IsNullOrEmpty(directory) ? tempFileName : fileSystem.Path.Combine(directory, tempFileName); + + try + { + await fileSystem.File.WriteAllTextAsync(tempFilePath, content, cancellationToken).ConfigureAwait(false); + + for (int attempt = 1; ; attempt++) + { + try + { +#if NETSTANDARD2_1 + // No overwriting Move here. Replace swaps an existing target in one step; a target that does + // not exist yet takes a plain Move, and one created by another writer in between makes that + // Move throw, so the retry takes the Replace path. + if (fileSystem.File.Exists(filePath)) + { + fileSystem.File.Replace(tempFilePath, filePath, destinationBackupFileName: null); + } + else + { + fileSystem.File.Move(tempFilePath, filePath); + } +#else + fileSystem.File.Move(tempFilePath, filePath, overwrite: true); +#endif + return; + } + catch (Exception ex) when (attempt < MaxMoveAttempts && ex is IOException or UnauthorizedAccessException) + { + // Windows refuses to replace a file another handle has open without delete sharing, which + // a concurrent reader or a second writer's replace holds for a moment. Give it that moment. + await Task.Delay(Math.Min(attempt * 2, 20), cancellationToken).ConfigureAwait(false); + } + } + } + finally + { + if (fileSystem.File.Exists(tempFilePath)) + { + fileSystem.File.Delete(tempFilePath); + } + } + } +} From 9dfa97f5b62bdab703b581a2aff344e665236053 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 14 Oct 2026 12:56:26 +0000 Subject: [PATCH 2/3] Retry a read that lands on a Windows replace in progress On windows-latest the concurrency test failed in RetrieveAsync: "The process cannot access the file ... k.json because it is being used by another process." Windows holds the target exclusively for the instant an overwriting move swaps it in. A read that lands at that moment is refused, where it would otherwise be served either version. The shared helper, renamed from AtomicFileWrite to AtomicFile, now also provides ReadAllTextAsync, and both providers' RetrieveAsync use it. It retries only ERROR_SHARING_VIOLATION and ERROR_LOCK_VIOLATION, with the same bounded backoff as the write path. A missing file and every other error still surface at once. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01YLmoTmY5N7amW8Z9jazuGV --- CLAUDE.md | 2 +- ...als.PersistenceProviders.FileSystem.csproj | 2 +- .../FileSystemPersistenceProvider.cs | 4 +- ...ssentials.PersistenceProviders.Temp.csproj | 2 +- .../TempPersistenceProvider.cs | 4 +- Shared/{AtomicFileWrite.cs => AtomicFile.cs} | 44 ++++++++++++++++--- 6 files changed, 46 insertions(+), 12 deletions(-) rename Shared/{AtomicFileWrite.cs => AtomicFile.cs} (62%) diff --git a/CLAUDE.md b/CLAUDE.md index d3d69e1..46247ef 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -48,7 +48,7 @@ This is a .NET library (`ktsu.Essentials`) providing high-performance interfaces - `Shared/SpecialFunctions.cs` - Incomplete gamma, incomplete beta, error function, normal quantile and cancellation-free `ExpM1`/`Log1P`, linked into the Normal, LogNormal, Exponential, Binomial, Poisson and Geometric providers rather than placed in the interfaces-only package - `Shared/DistributionArguments.cs` - Parameter validation shared by every distribution provider, linked in the same way - `Shared/PersistenceDefaults.cs` - The `IPersistenceProvider` default `RetrieveOrCreateAsync` body, linked into the FileSystem and Temp providers, which declare that member themselves and so hide the default -- `Shared/AtomicFileWrite.cs` - The FileSystem and Temp providers' write path: each store stages its data in a uniquely named `.tmp` beside the target and publishes it with an overwriting move, retrying briefly while Windows holds the target open, so concurrent writers can't collide and a reader never sees the key missing. Linked into both providers like `PersistenceDefaults.cs` +- `Shared/AtomicFile.cs` - The FileSystem and Temp providers' write and read paths: each store stages its data in a uniquely named `.tmp` beside the target and publishes it with an overwriting move, retrying briefly while Windows holds the target open, so concurrent writers can't collide and a reader never sees the key missing; reads retry only the Windows sharing violation a replace in progress raises. Linked into both providers like `PersistenceDefaults.cs` - `Essentials/ISerializationProvider.cs` - Object serialization/deserialization interface - `Essentials/ISerializationOptions.cs` - Configurable serialization options (naming, inclusion, boxing policies) - `Essentials/ICacheProvider.cs` - Generic cache interface with expiration and get-or-add diff --git a/Essentials.PersistenceProviders.FileSystem/Essentials.PersistenceProviders.FileSystem.csproj b/Essentials.PersistenceProviders.FileSystem/Essentials.PersistenceProviders.FileSystem.csproj index b867528..4e5bf75 100644 --- a/Essentials.PersistenceProviders.FileSystem/Essentials.PersistenceProviders.FileSystem.csproj +++ b/Essentials.PersistenceProviders.FileSystem/Essentials.PersistenceProviders.FileSystem.csproj @@ -16,7 +16,7 @@ - + diff --git a/Essentials.PersistenceProviders.FileSystem/FileSystemPersistenceProvider.cs b/Essentials.PersistenceProviders.FileSystem/FileSystemPersistenceProvider.cs index 21099e6..a3b5b18 100644 --- a/Essentials.PersistenceProviders.FileSystem/FileSystemPersistenceProvider.cs +++ b/Essentials.PersistenceProviders.FileSystem/FileSystemPersistenceProvider.cs @@ -68,7 +68,7 @@ public async Task StoreAsync(TKey key, T obj, CancellationToken cancellationT _fileSystemProvider.Directory.CreateDirectory(directory); } - await AtomicFileWrite.WriteAllTextAsync(_fileSystemProvider, filePath, serializedData, cancellationToken).ConfigureAwait(false); + await AtomicFile.WriteAllTextAsync(_fileSystemProvider, filePath, serializedData, cancellationToken).ConfigureAwait(false); } catch (Exception ex) { @@ -93,7 +93,7 @@ public async Task StoreAsync(TKey key, T obj, CancellationToken cancellationT return default; } - string serializedData = await _fileSystemProvider.File.ReadAllTextAsync(filePath, cancellationToken).ConfigureAwait(false); + string serializedData = await AtomicFile.ReadAllTextAsync(_fileSystemProvider, filePath, cancellationToken).ConfigureAwait(false); if (string.IsNullOrEmpty(serializedData)) { diff --git a/Essentials.PersistenceProviders.Temp/Essentials.PersistenceProviders.Temp.csproj b/Essentials.PersistenceProviders.Temp/Essentials.PersistenceProviders.Temp.csproj index b867528..4e5bf75 100644 --- a/Essentials.PersistenceProviders.Temp/Essentials.PersistenceProviders.Temp.csproj +++ b/Essentials.PersistenceProviders.Temp/Essentials.PersistenceProviders.Temp.csproj @@ -16,7 +16,7 @@ - + diff --git a/Essentials.PersistenceProviders.Temp/TempPersistenceProvider.cs b/Essentials.PersistenceProviders.Temp/TempPersistenceProvider.cs index ee62315..d668b15 100644 --- a/Essentials.PersistenceProviders.Temp/TempPersistenceProvider.cs +++ b/Essentials.PersistenceProviders.Temp/TempPersistenceProvider.cs @@ -65,7 +65,7 @@ public async Task StoreAsync(TKey key, T obj, CancellationToken cancellationT _fileSystemProvider.Directory.CreateDirectory(directory); } - await AtomicFileWrite.WriteAllTextAsync(_fileSystemProvider, filePath, serializedData, cancellationToken).ConfigureAwait(false); + await AtomicFile.WriteAllTextAsync(_fileSystemProvider, filePath, serializedData, cancellationToken).ConfigureAwait(false); } catch (Exception ex) { @@ -90,7 +90,7 @@ public async Task StoreAsync(TKey key, T obj, CancellationToken cancellationT return default; } - string serializedData = await _fileSystemProvider.File.ReadAllTextAsync(filePath, cancellationToken).ConfigureAwait(false); + string serializedData = await AtomicFile.ReadAllTextAsync(_fileSystemProvider, filePath, cancellationToken).ConfigureAwait(false); if (string.IsNullOrEmpty(serializedData)) { diff --git a/Shared/AtomicFileWrite.cs b/Shared/AtomicFile.cs similarity index 62% rename from Shared/AtomicFileWrite.cs rename to Shared/AtomicFile.cs index 63dc004..d0eb3b3 100644 --- a/Shared/AtomicFileWrite.cs +++ b/Shared/AtomicFile.cs @@ -9,7 +9,8 @@ namespace ktsu.Essentials; using System.Threading.Tasks; /// -/// Replaces a file's content in one step, so a reader sees either the old content or the new, never neither. +/// Replaces a file's content in one step, so a reader sees either the old content or the new, never neither, +/// and reads it back without tripping over a replace in progress. /// /// /// Linked into the FileSystem and Temp providers rather than placed in the interfaces package, following @@ -17,9 +18,13 @@ namespace ktsu.Essentials; /// writers to one key cannot move each other's file away, and publishes it with an overwriting move rather /// than a delete followed by a move, so the target never disappears in between (issue #66). /// -internal static class AtomicFileWrite +internal static class AtomicFile { - private const int MaxMoveAttempts = 50; + private const int MaxAttempts = 50; + + // Win32 ERROR_SHARING_VIOLATION and ERROR_LOCK_VIOLATION, carried in the low word of IOException.HResult. + private const int SharingViolation = 32; + private const int LockViolation = 33; /// /// Writes to , replacing any existing file atomically. @@ -64,11 +69,11 @@ internal static async Task WriteAllTextAsync(IFileSystem fileSystem, string file #endif return; } - catch (Exception ex) when (attempt < MaxMoveAttempts && ex is IOException or UnauthorizedAccessException) + catch (Exception ex) when (attempt < MaxAttempts && ex is IOException or UnauthorizedAccessException) { // Windows refuses to replace a file another handle has open without delete sharing, which // a concurrent reader or a second writer's replace holds for a moment. Give it that moment. - await Task.Delay(Math.Min(attempt * 2, 20), cancellationToken).ConfigureAwait(false); + await Task.Delay(Backoff(attempt), cancellationToken).ConfigureAwait(false); } } } @@ -80,4 +85,33 @@ internal static async Task WriteAllTextAsync(IFileSystem fileSystem, string file } } } + + /// + /// Reads , waiting out a concurrent replacing it. + /// + /// The filesystem to read through. + /// The file to read. + /// A token to cancel the operation. + /// The file's content. + internal static async Task ReadAllTextAsync(IFileSystem fileSystem, string filePath, CancellationToken cancellationToken) + { + for (int attempt = 1; ; attempt++) + { + try + { + return await fileSystem.File.ReadAllTextAsync(filePath, cancellationToken).ConfigureAwait(false); + } + catch (IOException ex) when (attempt < MaxAttempts && IsSharingViolation(ex)) + { + // Windows holds the target exclusively for the instant an overwriting move swaps it, so a + // read that lands then is refused rather than served either version. Only that refusal is + // retried; a missing file and every other error surface at once. + await Task.Delay(Backoff(attempt), cancellationToken).ConfigureAwait(false); + } + } + } + + private static bool IsSharingViolation(IOException ex) => (ex.HResult & 0xFFFF) is SharingViolation or LockViolation; + + private static int Backoff(int attempt) => Math.Min(attempt * 2, 20); } From 675bd00d5bc15bad7b8e03b1a8fa22df2b632672 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 14 Oct 2026 13:29:58 +0000 Subject: [PATCH 3/3] Bound the retry loops explicitly and use Assert.Contains SonarCloud S1994 flagged both retry loops in AtomicFile. They were written `for (int attempt = 1; ; attempt++)` with the bound in the catch filter. Each loop now runs `attempt < MaxAttempts` retried attempts, and then makes one final unguarded attempt whose failure surfaces. The swap moves into a Replace helper so the two call sites share it. Behaviour is unchanged. The concurrency test also uses Assert.Contains in place of CollectionAssert.Contains, which MSTEST0068 asked for. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01YLmoTmY5N7amW8Z9jazuGV --- Essentials.Tests/PersistenceProviderTests.cs | 2 +- Shared/AtomicFile.cs | 49 ++++++++++++-------- 2 files changed, 31 insertions(+), 20 deletions(-) diff --git a/Essentials.Tests/PersistenceProviderTests.cs b/Essentials.Tests/PersistenceProviderTests.cs index 8dfe3a8..ac4b73d 100644 --- a/Essentials.Tests/PersistenceProviderTests.cs +++ b/Essentials.Tests/PersistenceProviderTests.cs @@ -694,7 +694,7 @@ public async Task Concurrent_Stores_To_One_Key_Never_Throw_Or_Hide_The_Key() TestData? final = await persistence.RetrieveAsync("k", TestContext.CancellationToken).ConfigureAwait(false); Assert.IsNotNull(final); - CollectionAssert.Contains(Enumerable.Range(0, writers).Select(i => $"writer {i}").ToArray(), final.Name, $"{name} published a payload no writer stored"); + Assert.Contains(final.Name, Enumerable.Range(0, writers).Select(i => $"writer {i}"), $"{name} published a payload no writer stored"); string[] keys = [.. await persistence.GetAllKeysAsync(TestContext.CancellationToken).ConfigureAwait(false)]; Assert.HasCount(1, keys, $"{name} listed a staging file as a key"); diff --git a/Shared/AtomicFile.cs b/Shared/AtomicFile.cs index d0eb3b3..848c795 100644 --- a/Shared/AtomicFile.cs +++ b/Shared/AtomicFile.cs @@ -48,34 +48,23 @@ internal static async Task WriteAllTextAsync(IFileSystem fileSystem, string file { await fileSystem.File.WriteAllTextAsync(tempFilePath, content, cancellationToken).ConfigureAwait(false); - for (int attempt = 1; ; attempt++) + // Every attempt but the last may be refused and retried; the last one's failure surfaces. + for (int attempt = 1; attempt < MaxAttempts; attempt++) { try { -#if NETSTANDARD2_1 - // No overwriting Move here. Replace swaps an existing target in one step; a target that does - // not exist yet takes a plain Move, and one created by another writer in between makes that - // Move throw, so the retry takes the Replace path. - if (fileSystem.File.Exists(filePath)) - { - fileSystem.File.Replace(tempFilePath, filePath, destinationBackupFileName: null); - } - else - { - fileSystem.File.Move(tempFilePath, filePath); - } -#else - fileSystem.File.Move(tempFilePath, filePath, overwrite: true); -#endif + Replace(fileSystem, tempFilePath, filePath); return; } - catch (Exception ex) when (attempt < MaxAttempts && ex is IOException or UnauthorizedAccessException) + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { // Windows refuses to replace a file another handle has open without delete sharing, which // a concurrent reader or a second writer's replace holds for a moment. Give it that moment. await Task.Delay(Backoff(attempt), cancellationToken).ConfigureAwait(false); } } + + Replace(fileSystem, tempFilePath, filePath); } finally { @@ -95,13 +84,13 @@ internal static async Task WriteAllTextAsync(IFileSystem fileSystem, string file /// The file's content. internal static async Task ReadAllTextAsync(IFileSystem fileSystem, string filePath, CancellationToken cancellationToken) { - for (int attempt = 1; ; attempt++) + for (int attempt = 1; attempt < MaxAttempts; attempt++) { try { return await fileSystem.File.ReadAllTextAsync(filePath, cancellationToken).ConfigureAwait(false); } - catch (IOException ex) when (attempt < MaxAttempts && IsSharingViolation(ex)) + catch (IOException ex) when (IsSharingViolation(ex)) { // Windows holds the target exclusively for the instant an overwriting move swaps it, so a // read that lands then is refused rather than served either version. Only that refusal is @@ -109,8 +98,30 @@ internal static async Task ReadAllTextAsync(IFileSystem fileSystem, stri await Task.Delay(Backoff(attempt), cancellationToken).ConfigureAwait(false); } } + + return await fileSystem.File.ReadAllTextAsync(filePath, cancellationToken).ConfigureAwait(false); } +#if NETSTANDARD2_1 + private static void Replace(IFileSystem fileSystem, string tempFilePath, string filePath) + { + // No overwriting Move here. Replace swaps an existing target in one step; a target that does not exist + // yet takes a plain Move, and one created by another writer in between makes that Move throw, so the + // retry takes the Replace path. + if (fileSystem.File.Exists(filePath)) + { + fileSystem.File.Replace(tempFilePath, filePath, destinationBackupFileName: null); + } + else + { + fileSystem.File.Move(tempFilePath, filePath); + } + } +#else + private static void Replace(IFileSystem fileSystem, string tempFilePath, string filePath) + => fileSystem.File.Move(tempFilePath, filePath, overwrite: true); +#endif + private static bool IsSharingViolation(IOException ex) => (ex.HResult & 0xFFFF) is SharingViolation or LockViolation; private static int Backoff(int attempt) => Math.Min(attempt * 2, 20);