From f978f2384a05ec39ab61b0449e5896ffabf4071c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20B=C3=BCrgi?= Date: Thu, 13 Aug 2026 09:23:22 +0200 Subject: [PATCH 1/2] fix: a NodeType assembly can be loaded mid-write and park the type (#1387) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both IAssemblyStore implementations published a freshly-emitted assembly with File.WriteAllBytes / DownloadToAsync straight onto the name readers discover by. FileMode.Create creates the target FIRST and streams the bytes afterwards, so the DLL exists at 0 bytes under its final, globbable name while it fills in. The reader is not reading JSON here — it hands that path to AssemblyLoadContext.LoadFromAssemblyPath. A truncated PE image still loads (the header is intact) and the first Assembly.GetTypes() throws ReflectionTypeLoadException: Could not load type 'HighWaterLayoutAreas' from assembly 'DynamicNode_TestData_HighWaterType' because the format is invalid which reads as a source defect and is nothing of the kind. Why one mis-read is not transient. CompileResultFromAssembly catches that load fault and records CompilationStatus.Error; the first-build kickoff is gated on Status == null, so it never retries. The bytes on disk heal the moment the write finishes — the VERDICT does not. One torn read parks the NodeType until someone rebuilds it by hand, and a parked NodeType refuses portal readiness. Where it bites hardest: on AKS the store is FileSystemAssemblyStore over /data/assembly-cache, a ReadWriteMany Azure Files share, so the racing reader is usually a different POD. FileSystemAssemblyStore's own first-write-wins probe uses the same glob, so it too could observe a half-written file. Same defect class as #1359 (FileSystemStorageAdapter, FileSystemVersionStore), third and fourth site — and this one publishes executable bytes. - AtomicFileWrite moves from MeshWeaver.Hosting.Persistence (internal) to MeshWeaver.Mesh.Contract (public, MeshWeaver.Mesh.Persistence) so all three assemblies share ONE implementation rather than growing a second copy, which is the mistake #1359 had to undo. - PublishBytes deliberately does NOT replace an existing target. Azure Files SMB does not give the POSIX guarantee that rename-over-an-existing-name is a single operation, so relying on replace would re-open the window on the very volume this is closing. Creating a name that does not yet exist is one directory-entry insert everywhere. FileSystemAssemblyStore loses nothing: its target name embeds the content hash, so a target of that name already holds exactly these bytes. BlobAssemblyStore's local materialisation keeps replace semantics (its key carries no framework tag, so a new image must overwrite) — that cache is a process-local directory where rename-replace IS atomic. - The staging name is a dot-prefixed sibling ending .tmp, so no caller's discovery glob can match it — including under Windows' DOS 8.3 quirk, where a three-char pattern extension like *.dll also matches longer extensions beginning with it. - The PDB is published before the DLL: the DLL is the discovery key, so anything a reader can see is complete AND already has its symbols. No retry, no sleep, no widened budget: the bytes are either complete when published or they are not. Verification (fail-before / pass-after, DOTNET_PROCESSOR_COUNT=4): before — 11 of 12 assemblies discoverable at 0 of 4194304 bytes, 726 observations after — 0 violations; MeshWeaver.Hosting.Test 161/161 green Release -warnaserror clean: Mesh.Contract, Graph, Hosting, Hosting.AzureBlob, Hosting.Test, Documentation.Test. Co-Authored-By: Claude Opus 5 (1M context) --- ...eaks-because-its-code-was-read-mid-save.md | 28 +++ .../Configuration/FileSystemAssemblyStore.cs | 38 +++- .../BlobAssemblyStore.cs | 30 ++- .../Persistence/AtomicFileWrite.cs | 55 ----- .../Persistence/FileSystemStorageAdapter.cs | 1 + .../Persistence/FileSystemVersionStore.cs | 1 + .../Persistence/AtomicFileWrite.cs | 189 ++++++++++++++++++ .../FileSystemAssemblyStoreAtomicityTest.cs | 184 +++++++++++++++++ 8 files changed, 463 insertions(+), 63 deletions(-) create mode 100644 src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-13-a-page-no-longer-breaks-because-its-code-was-read-mid-save.md delete mode 100644 src/MeshWeaver.Hosting/Persistence/AtomicFileWrite.cs create mode 100644 src/MeshWeaver.Mesh.Contract/Persistence/AtomicFileWrite.cs create mode 100644 test/MeshWeaver.Hosting.Test/FileSystemAssemblyStoreAtomicityTest.cs diff --git a/src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-13-a-page-no-longer-breaks-because-its-code-was-read-mid-save.md b/src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-13-a-page-no-longer-breaks-because-its-code-was-read-mid-save.md new file mode 100644 index 0000000000..7ef853df92 --- /dev/null +++ b/src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-13-a-page-no-longer-breaks-because-its-code-was-read-mid-save.md @@ -0,0 +1,28 @@ +--- +Name: A page no longer breaks because its code was read while still being saved +Category: Fix +Description: A freshly built page could be picked up a moment before it had finished saving, which made it look permanently broken even though nothing was wrong with it. Builds now become visible only once they are complete. +Icon: Bug +Order: -20260813 +--- + +# A page no longer breaks because its code was read while still being saved + +When you edit the code behind a page, the portal builds it and saves the result so every part +of the portal can use it. The saved file was being created first and filled in immediately +afterwards — a gap of milliseconds, but a real one. Anything that picked the build up inside +that gap got half of it. + +Half a build is not a build, so the portal reported it the only way it could: as a failure to +compile. That was the damaging part. A page that had actually built perfectly well was marked +broken, and because a compile failure is treated as a settled answer, it was never retried — +the page stayed broken until someone rebuilt it by hand, and the error message pointed at the +page's code, where there was nothing to find. + +It was easiest to hit exactly where it hurt most: on a busy portal, and on installations that +run several copies of the portal sharing one location for builds, where one copy would read +what another was still writing. A page marked broken also holds up the portal reporting itself +as ready, so a single mis-read could slow a restart for everyone. + +A build is now written aside and moved into place in one step, so it becomes visible only once +it is whole. There is no longer a moment in which a page can be read half-built. diff --git a/src/MeshWeaver.Graph/Configuration/FileSystemAssemblyStore.cs b/src/MeshWeaver.Graph/Configuration/FileSystemAssemblyStore.cs index d25c633194..ccb88f4070 100644 --- a/src/MeshWeaver.Graph/Configuration/FileSystemAssemblyStore.cs +++ b/src/MeshWeaver.Graph/Configuration/FileSystemAssemblyStore.cs @@ -1,5 +1,6 @@ using System.Reactive.Linq; using System.Security.Cryptography; +using MeshWeaver.Mesh.Persistence; using MeshWeaver.Mesh.Services; using Microsoft.Extensions.Logging; @@ -140,11 +141,40 @@ public IObservable PutWithLocation(string nodeTypePath, l var pdbPath = Path.ChangeExtension(dllPath, ".pdb"); var relativeContentPath = Path.GetRelativePath(rootDirectory, dllPath).Replace('\\', '/'); - File.WriteAllBytes(dllPath, assemblyBytes); + // 🚨 ATOMIC PUBLICATION — never File.WriteAllBytes on dllPath (MeshWeaver#1387). + // The DLL's NAME is its publication: both TryGetAssemblyPath above and the + // first-write-wins probe a few lines up discover it by globbing + // `v{version}-{tag}-*.dll`, and the winner's path goes straight to + // AssemblyLoadContext.LoadFromAssemblyPath. FileMode.Create (what WriteAllBytes uses) + // creates the target FIRST and streams the bytes afterwards, so a reader that globs + // inside that window loads a TRUNCATED PE image. The header is intact, so the load + // itself succeeds and the first Assembly.GetTypes() throws + // ReflectionTypeLoadException: Could not load type 'X' from assembly + // 'DynamicNode_…' because the format is invalid + // which CompileResultFromAssembly records as a compile failure — and that failure is + // TERMINAL: it writes CompilationStatus.Error and the first-build kickoff (gated on + // Status == null) never retries, so one transient torn read PARKS the NodeType until + // someone deletes the file. A parked NodeType refuses portal readiness. + // + // The reader is not even necessarily in this process: on AKS this directory is + // /data/assembly-cache, a ReadWriteMany Azure Files share, so every replica globs the + // bytes another replica is mid-write on. + // + // Publish the PDB before the DLL: the DLL is the discovery key, so anything visible to + // a reader is complete AND already has its symbols. if (pdbBytes is { Length: > 0 }) - File.WriteAllBytes(pdbPath, pdbBytes); - logger.LogInformation( - "Cached assembly at {DllPath} ({Bytes} bytes)", dllPath, assemblyBytes.Length); + AtomicFileWrite.PublishBytes(pdbPath, pdbBytes); + var published = AtomicFileWrite.PublishBytes(dllPath, assemblyBytes); + if (published) + logger.LogInformation( + "Cached assembly at {DllPath} ({Bytes} bytes)", dllPath, assemblyBytes.Length); + else + // Another writer (or another replica through the shared volume) published the same + // content-hashed name first. The bytes are identical by construction — the hash IS + // the name — so this is a no-op, not a conflict. + logger.LogDebug( + "Assembly already published at {DllPath} by a concurrent writer — kept theirs " + + "(identical bytes: the content hash is the file name)", dllPath); return Observable.Return(new AssemblyStoreLocation(dllPath, FileSystemCollectionName, relativeContentPath)); } diff --git a/src/MeshWeaver.Hosting.AzureBlob/BlobAssemblyStore.cs b/src/MeshWeaver.Hosting.AzureBlob/BlobAssemblyStore.cs index 4838bd0a3a..7c47b25033 100644 --- a/src/MeshWeaver.Hosting.AzureBlob/BlobAssemblyStore.cs +++ b/src/MeshWeaver.Hosting.AzureBlob/BlobAssemblyStore.cs @@ -1,6 +1,7 @@ using System.Reactive.Linq; using Azure; using Azure.Storage.Blobs; +using MeshWeaver.Mesh.Persistence; using MeshWeaver.Mesh.Services; using MeshWeaver.Mesh.Threading; using Microsoft.Extensions.Logging; @@ -84,9 +85,20 @@ public BlobAssemblyStore( try { Directory.CreateDirectory(Path.GetDirectoryName(localPath)!); - await dllBlob.DownloadToAsync(localPath, ct).ConfigureAwait(false); - try { await pdbBlob.DownloadToAsync(Path.ChangeExtension(localPath, ".pdb"), ct).ConfigureAwait(false); } + // 🚨 Materialise through a temp file + rename (MeshWeaver#1387). localPath is the + // exact name the File.Exists fast path above discovers by, and the winner's path + // goes to AssemblyLoadContext.LoadFromAssemblyPath — so downloading straight into + // it publishes a growing, partially-written PE image under its final name. The + // pdb is published first so a discoverable DLL always has its symbols. + try + { + await AtomicFileWrite.PublishAsync( + Path.ChangeExtension(localPath, ".pdb"), + temp => pdbBlob.DownloadToAsync(temp, ct)).ConfigureAwait(false); + } catch (RequestFailedException rfe) when (rfe.Status == 404) { /* pdb optional */ } + await AtomicFileWrite.PublishAsync( + localPath, temp => dllBlob.DownloadToAsync(temp, ct)).ConfigureAwait(false); logger.LogInformation( "Hydrated {NodeTypePath}@v{Version} from blob to {LocalPath}", nodeTypePath, version, localPath); @@ -115,10 +127,20 @@ public IObservable PutWithLocation(string nodeTypePath, l // Write to local cache first so the caller can load immediately without waiting // on the upload round-trip. + // + // 🚨 Through a temp file + rename (MeshWeaver#1387): localPath is the exact name + // TryGetAssemblyPath's File.Exists fast path discovers by, and the discovered path is + // handed to AssemblyLoadContext.LoadFromAssemblyPath. File.WriteAllBytesAsync opens + // FileMode.Create — target first, bytes after — so writing it directly publishes a + // truncated PE image under its final name for the duration of the write. Replace + // semantics are correct here (and atomic): this cache is a process-local directory, + // and the key carries no framework tag, so a new image must be able to overwrite the + // previous image's bytes for the same version. Directory.CreateDirectory(Path.GetDirectoryName(localPath)!); - await File.WriteAllBytesAsync(localPath, assemblyBytes, ct).ConfigureAwait(false); if (pdbBytes is { Length: > 0 }) - await File.WriteAllBytesAsync(Path.ChangeExtension(localPath, ".pdb"), pdbBytes, ct).ConfigureAwait(false); + await AtomicFileWrite.ReplaceAllBytesAsync( + Path.ChangeExtension(localPath, ".pdb"), pdbBytes, ct).ConfigureAwait(false); + await AtomicFileWrite.ReplaceAllBytesAsync(localPath, assemblyBytes, ct).ConfigureAwait(false); // Then upload. Overwrite=true is safe because two replicas compiling the same // version produce (near-)identical bytes. diff --git a/src/MeshWeaver.Hosting/Persistence/AtomicFileWrite.cs b/src/MeshWeaver.Hosting/Persistence/AtomicFileWrite.cs deleted file mode 100644 index 46068b46ea..0000000000 --- a/src/MeshWeaver.Hosting/Persistence/AtomicFileWrite.cs +++ /dev/null @@ -1,55 +0,0 @@ -namespace MeshWeaver.Hosting.Persistence; - -/// -/// The one way this assembly publishes a file: write a uniquely-named temp file, then rename it -/// over the target. The rename is what makes the target's APPEARANCE and its CONTENT a single -/// event. -/// -/// 🚨 Why it must not be . -/// That opens with FileMode.Create — it creates (or truncates) the target FIRST and streams -/// the bytes afterwards. So between the two, the target exists at zero length under its final, -/// publicly discoverable name. Anything that finds files by name is then looking at a file that is -/// not there yet: -/// -/// FileSystemVersionStore.GetVersions globs {id}_*.json, so the -/// file name IS the publication of a version. A reader that listed and then read inside that -/// window got "" and JsonSerializer.Deserialize threw "The input does not -/// contain any JSON tokens" — how -/// VersionHistoryTest.VersionQuery_GetVersionBeforeAsync_FindsPreChangeState failed on CI -/// with a version list that was otherwise exactly right. -/// A cancellation landing in that same window leaves the truncated target on -/// disk permanently, with the previous content already gone — the 0-byte SamplesGraph corruption -/// pattern. -/// -/// -/// Both failures are the same defect: writing to the name readers watch. The cure is to write -/// somewhere readers do not look and rename, which is atomic on every filesystem this runs on. The -/// temp name deliberately keeps the target's full name as a prefix and appends .tmp.{guid}, -/// so it can never match a *.json glob, and two concurrent writers of the same target cannot -/// collide on the temp. -/// -internal static class AtomicFileWrite -{ - /// - /// Writes so that only ever exists - /// complete. On any failure the target is left exactly as it was and no temp file survives. - /// - /// is nullable to match - /// , which writes an - /// empty file for null; callers that must never publish empty content check that - /// themselves. - public static async Task WriteAllTextAsync(string filePath, string? content, CancellationToken ct) - { - var tempPath = filePath + ".tmp." + Guid.NewGuid().ToString("N"); - try - { - await File.WriteAllTextAsync(tempPath, content, ct).ConfigureAwait(false); - File.Move(tempPath, filePath, overwrite: true); - } - catch - { - try { if (File.Exists(tempPath)) File.Delete(tempPath); } catch { /* best effort */ } - throw; - } - } -} diff --git a/src/MeshWeaver.Hosting/Persistence/FileSystemStorageAdapter.cs b/src/MeshWeaver.Hosting/Persistence/FileSystemStorageAdapter.cs index 43c0c083fe..27c87143f1 100644 --- a/src/MeshWeaver.Hosting/Persistence/FileSystemStorageAdapter.cs +++ b/src/MeshWeaver.Hosting/Persistence/FileSystemStorageAdapter.cs @@ -4,6 +4,7 @@ using MeshWeaver.Hosting.Persistence.Parsers; using MeshWeaver.Markdown; using MeshWeaver.Mesh; +using MeshWeaver.Mesh.Persistence; using MeshWeaver.Mesh.Services; using MeshWeaver.Mesh.Threading; diff --git a/src/MeshWeaver.Hosting/Persistence/FileSystemVersionStore.cs b/src/MeshWeaver.Hosting/Persistence/FileSystemVersionStore.cs index a81fa3c5ff..4d011a2755 100644 --- a/src/MeshWeaver.Hosting/Persistence/FileSystemVersionStore.cs +++ b/src/MeshWeaver.Hosting/Persistence/FileSystemVersionStore.cs @@ -1,6 +1,7 @@ using System.Reactive.Linq; using System.Text.Json; using MeshWeaver.Mesh; +using MeshWeaver.Mesh.Persistence; using MeshWeaver.Mesh.Services; using MeshWeaver.Mesh.Threading; diff --git a/src/MeshWeaver.Mesh.Contract/Persistence/AtomicFileWrite.cs b/src/MeshWeaver.Mesh.Contract/Persistence/AtomicFileWrite.cs new file mode 100644 index 0000000000..f1e006b264 --- /dev/null +++ b/src/MeshWeaver.Mesh.Contract/Persistence/AtomicFileWrite.cs @@ -0,0 +1,189 @@ +namespace MeshWeaver.Mesh.Persistence; + +/// +/// The one way this framework publishes a file: write a uniquely-named temp file, then rename it +/// into place. The rename is what makes the target's APPEARANCE and its CONTENT a single event. +/// +/// 🚨 Why it must not be +/// or . Both open with FileMode.Create — they +/// create (or truncate) the target FIRST and stream the bytes afterwards. So between the two, the +/// target exists at zero (or partial) length under its final, publicly discoverable name. Anything +/// that finds files by name is then looking at a file that is not there yet: +/// +/// FileSystemVersionStore.GetVersions globs {id}_*.json, so the +/// file name IS the publication of a version. A reader that listed and then read inside that +/// window got "" and JsonSerializer.Deserialize threw "The input does not +/// contain any JSON tokens" — how +/// VersionHistoryTest.VersionQuery_GetVersionBeforeAsync_FindsPreChangeState failed on CI +/// with a version list that was otherwise exactly right. +/// FileSystemAssemblyStore globs v{version}-{tag}-*.dll, so the +/// DLL name IS the publication of a compiled NodeType assembly — and the reader hands that path +/// straight to AssemblyLoadContext.LoadFromAssemblyPath. A partial read there is not an +/// empty string, it is a truncated PE image: the header loads, and the first +/// Assembly.GetTypes() throws ReflectionTypeLoadException — "Could not load type +/// 'X' from assembly 'DynamicNode_…' because the format is invalid" — which the compile +/// pipeline records as a compile failure and PARKS the NodeType (MeshWeaver#1387). On AKS the +/// cache is a ReadWriteMany Azure Files share, so the racing reader is usually a DIFFERENT +/// POD. +/// A cancellation landing in that same window leaves the truncated target on +/// disk permanently, with the previous content already gone — the 0-byte SamplesGraph corruption +/// pattern. +/// +/// +/// All three failures are the same defect: writing to the name readers watch. The cure is to +/// write somewhere readers do not look and rename. +/// +/// The temp name. It is a sibling of the target (rename is only atomic within one +/// filesystem) named .tmp-{guid}-{targetName}.tmp. The LEADING DOT and the trailing +/// .tmp are both load-bearing: a caller's discovery glob is anchored on the target's own +/// prefix (v{version}-…, {id}_…) and on its real extension, and a name that starts +/// with neither cannot match one — including under Windows' DOS 8.3 quirk, where a three-character +/// pattern extension such as *.dll also matches longer extensions that begin with it. The +/// guid makes two concurrent writers of the same target unable to collide on the temp. +/// +/// Replace semantics. deliberately does NOT replace an +/// existing target, and that is what keeps it atomic on the AKS assembly-cache share. Azure Files +/// SMB does not offer the POSIX guarantee that rename() over an EXISTING name is a single +/// operation, so a publisher that relied on replace would be re-introducing the very window it is +/// closing. Creating a name that does not yet exist is a single directory-entry insert on every +/// filesystem this runs on. Callers whose target names embed a content hash (the assembly store) +/// lose nothing: an existing target of that name already holds exactly these bytes. +/// +public static class AtomicFileWrite +{ + /// + /// Writes so that only ever exists + /// complete. On any failure the target is left exactly as it was and no temp file survives. + /// + /// Final path the content is published under. + /// Text to write. + /// Cancellation token. + /// is nullable to match + /// , which writes an + /// empty file for null; callers that must never publish empty content check that + /// themselves. + public static async Task WriteAllTextAsync(string filePath, string? content, CancellationToken ct) + { + var tempPath = TempPathFor(filePath); + try + { + await File.WriteAllTextAsync(tempPath, content, ct).ConfigureAwait(false); + File.Move(tempPath, filePath, overwrite: true); + } + catch + { + TryDelete(tempPath); + throw; + } + } + + /// + /// Publishes at so the target only ever + /// exists complete, WITHOUT replacing an existing target. + /// + /// Final path the bytes are published under. + /// Bytes to write. + /// true when this call published the file; false when a target of that + /// name already existed (another writer, or another replica through a shared volume, won the + /// race) — in which case nothing was overwritten and no temp file survives. + /// + /// First-publish-wins is the contract, not an optimisation: see the type remarks on why + /// replace-on-rename is not something the AKS assembly-cache share guarantees. + /// + public static bool PublishBytes(string filePath, byte[] bytes) + { + var tempPath = TempPathFor(filePath); + try + { + File.WriteAllBytes(tempPath, bytes); + // overwrite:false → File.Move throws IOException when the target already exists. + // That is the race being handled, not an error: the winner's bytes stay published. + File.Move(tempPath, filePath, overwrite: false); + return true; + } + catch (IOException) when (File.Exists(filePath)) + { + TryDelete(tempPath); + return false; + } + catch + { + TryDelete(tempPath); + throw; + } + } + + /// + /// Writes so that only ever exists + /// complete, REPLACING any existing target. Use this only where the target lives on a local + /// filesystem — see the type remarks on why a shared SMB volume must use + /// instead. + /// + /// Final path the bytes are published under. + /// Bytes to write. + /// Cancellation token. + public static async Task ReplaceAllBytesAsync(string filePath, byte[] bytes, CancellationToken ct) + { + var tempPath = TempPathFor(filePath); + try + { + await File.WriteAllBytesAsync(tempPath, bytes, ct).ConfigureAwait(false); + File.Move(tempPath, filePath, overwrite: true); + } + catch + { + TryDelete(tempPath); + throw; + } + } + + /// + /// Publishes the file produced by at , + /// so the target only ever exists complete. The callback is handed the temp path and must fill + /// it; the rename happens only after it returns. Use this when the bytes are produced by a + /// stream-to-path API (an SDK download) rather than held in memory. + /// + /// Final path the file is published under. + /// Fills the supplied temp path. + /// true when this call published the file; false when a target of that + /// name already existed. + public static async Task PublishAsync(string filePath, Func writeTemp) + { + var tempPath = TempPathFor(filePath); + try + { + await writeTemp(tempPath).ConfigureAwait(false); + File.Move(tempPath, filePath, overwrite: false); + return true; + } + catch (IOException) when (File.Exists(filePath)) + { + TryDelete(tempPath); + return false; + } + catch + { + TryDelete(tempPath); + throw; + } + } + + /// + /// The staging name a publication of uses — a sibling that no + /// caller's discovery glob can match. Exposed so tests can assert the invariant rather than + /// re-derive the convention. + /// + /// Final path being published. + /// A unique sibling path to stage the bytes in. + public static string TempPathFor(string filePath) + { + var directory = Path.GetDirectoryName(filePath); + var name = $".tmp-{Guid.NewGuid():N}-{Path.GetFileName(filePath)}.tmp"; + return string.IsNullOrEmpty(directory) ? name : Path.Combine(directory, name); + } + + private static void TryDelete(string path) + { + try { if (File.Exists(path)) File.Delete(path); } catch { /* best effort */ } + } +} diff --git a/test/MeshWeaver.Hosting.Test/FileSystemAssemblyStoreAtomicityTest.cs b/test/MeshWeaver.Hosting.Test/FileSystemAssemblyStoreAtomicityTest.cs new file mode 100644 index 0000000000..6283e0db47 --- /dev/null +++ b/test/MeshWeaver.Hosting.Test/FileSystemAssemblyStoreAtomicityTest.cs @@ -0,0 +1,184 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Reactive.Linq; +using System.Threading; +using System.Threading.Tasks; +using MeshWeaver.Graph.Configuration; +using MeshWeaver.Mesh.Persistence; +using Microsoft.Extensions.Logging.Abstractions; +using Xunit; + +namespace MeshWeaver.Hosting.Test; + +/// +/// Pins the one property 's two halves have to agree on: +/// a compiled assembly must not be DISCOVERABLE until it is COMPLETE. +/// +/// finds an assembly by globbing +/// v{version}-{frameworkTag}-*.dll, so the DLL's file name IS its publication — and the +/// path it returns is handed straight to AssemblyLoadContext.LoadFromAssemblyPath. +/// File.WriteAllBytes opens with FileMode.Create: it creates the target FIRST and +/// streams the bytes afterwards, so writing straight to the final name publishes a 0-byte file and +/// fills it in later. A reader that globs inside that window does not get an empty string, it gets +/// a truncated PE image — the header is intact, the load succeeds, and the first +/// Assembly.GetTypes() throws +/// "Could not load type 'X' from assembly 'DynamicNode_…' because the format is invalid". +/// +/// That is not hypothetical: it is MeshWeaver#1387, seen on a CI shard as +/// ReflectionTypeLoadException … NodeType 'TestData/HighWaterType' PARKED after compile +/// failure. And the consequence is terminal, not transient — +/// MeshNodeCompilationService.CompileResultFromAssembly records that load fault as +/// CompilationStatus.Error and the first-build kickoff (gated on Status == null) does +/// not retry, so ONE torn read parks the NodeType until the file is deleted by hand, and a parked +/// NodeType refuses portal readiness. On AKS the cache is /data/assembly-cache, a +/// ReadWriteMany Azure Files share, so the racing reader is usually a different POD. +/// +/// 🚨 The assertion is on the SIZE of a DISCOVERED assembly, never on write throughput or +/// timing, so it cannot be satisfied by making anything faster — only by making publication atomic. +/// The observation count is asserted non-zero so a probe that never sampled a published assembly +/// cannot pass vacuously, and two deterministic assertions run afterwards regardless of what the +/// probe saw. +/// +public class FileSystemAssemblyStoreAtomicityTest : IDisposable +{ + private const string NodeTypePath = "AtomicType"; + private const int Versions = 12; + + /// + /// Big enough that the byte stream needs several chunks, which is what opens the window between + /// "file exists under its final, globbable name" and "file holds the whole image". Calibrated + /// the same way as 's payload, and sized so the + /// whole test writes ~48 MB. + /// + private const int PayloadBytes = 4 * 1024 * 1024; + + private readonly string _root = Path.Combine( + Path.GetTempPath(), "MeshWeaverAssemblyAtomicity", $"t_{Guid.NewGuid():N}"); + + public FileSystemAssemblyStoreAtomicityTest() => Directory.CreateDirectory(_root); + + public void Dispose() + { + GC.SuppressFinalize(this); + try { if (Directory.Exists(_root)) Directory.Delete(_root, recursive: true); } catch { /* best effort */ } + } + + /// Distinct bytes per version so a reader cannot pass by reading someone else's file. + private static byte[] Payload(long version) + { + var bytes = new byte[PayloadBytes]; + Array.Fill(bytes, (byte)version); + return bytes; + } + + private FileSystemAssemblyStore NewStore() => + new(_root, NullLogger.Instance); + + /// The exact glob discovers by. + private static string DiscoveryGlob(long version) => + $"v{version}-{FileSystemAssemblyStore.FrameworkTag}-*.dll"; + + [Fact(Timeout = 120000)] + public async Task ADiscoveredAssemblyIsAlwaysComplete() + { + var store = NewStore(); + var typeDirectory = Path.Combine(_root, NodeTypePath); + + var writesDone = false; + var observations = 0; + var failures = new List(); + + // Watches exactly the name space TryGetAssemblyPath globs, with nothing in the loop but the + // two syscalls, so it samples the publication window rather than spending it elsewhere. A + // DLL the glob can see must already hold its whole image; any short length IS the truncated + // PE one step before AssemblyLoadContext meets it. + var probe = Task.Run(() => + { + while (!Volatile.Read(ref writesDone)) + { + if (!Directory.Exists(typeDirectory)) continue; + for (var v = 1; v <= Versions; v++) + { + FileInfo[] listed; + try { listed = new DirectoryInfo(typeDirectory).GetFiles(DiscoveryGlob(v)); } + catch { continue; } + + foreach (var f in listed) + { + long length; + try { length = new FileInfo(f.FullName).Length; } catch { continue; } + Interlocked.Increment(ref observations); + if (length != PayloadBytes) + lock (failures) + failures.Add( + $"{f.Name} was discoverable at {length} of {PayloadBytes} bytes"); + } + } + } + }); + + for (var v = 1; v <= Versions; v++) + await store.Put(NodeTypePath, v, Payload(v), pdbBytes: null).FirstAsync(); + + Volatile.Write(ref writesDone, true); + await probe; + + Assert.True(Volatile.Read(ref observations) > 0, + "the probe never discovered a published assembly at all, so it proved nothing"); + Assert.True(failures.Count == 0, + "an assembly must not become discoverable before its bytes are on disk " + + $"({observations} observations, {failures.Distinct().Count()} distinct violations):" + + Environment.NewLine + string.Join(Environment.NewLine, failures.Distinct())); + + // …and every version the writes leave behind must round-trip through the store's own API. + // Deterministic, so a probe that happened to sample nothing still cannot make this vacuous. + for (var v = 1; v <= Versions; v++) + { + var path = await store.TryGetAssemblyPath(NodeTypePath, v).FirstAsync(); + Assert.NotNull(path); + var bytes = await File.ReadAllBytesAsync(path!); + Assert.Equal(PayloadBytes, bytes.Length); + Assert.Equal((byte)v, bytes[^1]); + } + } + + /// + /// The staging name must be invisible to the store's own discovery glob — a temp that still + /// matched v{version}-{tag}-*.dll would be handed to the assembly loader as if it were a + /// published assembly, which is the very failure this is preventing. Deterministic: it asserts + /// the naming convention itself, not a sampled race. + /// + [Fact] + public void TheStagingNameCannotMatchTheDiscoveryGlob() + { + var typeDirectory = Path.Combine(_root, NodeTypePath); + Directory.CreateDirectory(typeDirectory); + + var target = Path.Combine( + typeDirectory, $"v7-{FileSystemAssemblyStore.FrameworkTag}-0123456789ab.dll"); + var temp = AtomicFileWrite.TempPathFor(target); + File.WriteAllBytes(temp, new byte[16]); + + Assert.Empty(new DirectoryInfo(typeDirectory).GetFiles(DiscoveryGlob(7))); + Assert.Empty(new DirectoryInfo(typeDirectory).GetFiles("*.dll")); + } + + /// + /// A completed publication leaves no residue: exactly the DLL and its PDB, nothing else. + /// + [Fact(Timeout = 60000)] + public async Task PublishingLeavesNoTemporaryFileBehind() + { + var store = NewStore(); + await store.Put(NodeTypePath, 1, new byte[1024], new byte[512]).FirstAsync(); + + var files = Directory.GetFiles(Path.Combine(_root, NodeTypePath)) + .Select(Path.GetFileName).Order().ToArray(); + Assert.Equal(2, files.Length); + Assert.All(files, f => Assert.StartsWith("v1-", f)); + Assert.Contains(files, f => f!.EndsWith(".dll", StringComparison.Ordinal)); + Assert.Contains(files, f => f!.EndsWith(".pdb", StringComparison.Ordinal)); + } +} From fc948bf1e3e9985374b8df4745d00126b675e7af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20B=C3=BCrgi?= Date: Thu, 13 Aug 2026 09:45:13 +0200 Subject: [PATCH 2/2] review: narrow the already-exists filter so a real IO fault cannot be swallowed (#1387) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review on #1393: PublishBytes/PublishAsync caught any IOException as the 'another writer won' race whenever File.Exists(target) held, which could hide a genuine fault (denied, full disk, bad path) behind a false 'already published'. Fixed by checking the target BEFORE any IO. Having established there that it did not exist, the only way it can exist when the move fails is a concurrent writer — a real fault leaves the target absent, so it falls through and rethrows. Portable, no HResult/errno sniffing, and it skips the write entirely in the common already-published case. Two tests pin both halves: AGenuineIoFailureIsNotSwallowedAsTheAlreadyPublishedRace (missing parent dir → DirectoryNotFoundException propagates, target still absent) PublishingOverAnExistingTargetKeepsTheIncumbentAndReportsFalse MeshWeaver.Hosting.Test 163/163. Release -warnaserror clean: Mesh.Contract, Hosting.Test. Co-Authored-By: Claude Opus 5 (1M context) --- .../Persistence/AtomicFileWrite.cs | 13 +++++++ .../FileSystemAssemblyStoreAtomicityTest.cs | 37 +++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/src/MeshWeaver.Mesh.Contract/Persistence/AtomicFileWrite.cs b/src/MeshWeaver.Mesh.Contract/Persistence/AtomicFileWrite.cs index f1e006b264..7ae8002bba 100644 --- a/src/MeshWeaver.Mesh.Contract/Persistence/AtomicFileWrite.cs +++ b/src/MeshWeaver.Mesh.Contract/Persistence/AtomicFileWrite.cs @@ -92,6 +92,14 @@ public static async Task WriteAllTextAsync(string filePath, string? content, Can /// public static bool PublishBytes(string filePath, byte[] bytes) { + // Checked BEFORE any IO, which is what makes the catch filter below precise rather than a + // blanket "an IOException with the target present must have been the race". Having + // established here that the target did NOT exist, the only way it can exist by the time + // the move fails is that another writer created it in between — a genuine fault (denied, + // full disk, bad path) leaves the target absent, so it falls through and rethrows. + if (File.Exists(filePath)) + return false; + var tempPath = TempPathFor(filePath); try { @@ -149,6 +157,11 @@ public static async Task ReplaceAllBytesAsync(string filePath, byte[] bytes, Can /// name already existed. public static async Task PublishAsync(string filePath, Func writeTemp) { + // Same pre-check as PublishBytes, and for the same reason: it is what narrows the catch + // filter below to the concurrent-writer race. See there. + if (File.Exists(filePath)) + return false; + var tempPath = TempPathFor(filePath); try { diff --git a/test/MeshWeaver.Hosting.Test/FileSystemAssemblyStoreAtomicityTest.cs b/test/MeshWeaver.Hosting.Test/FileSystemAssemblyStoreAtomicityTest.cs index 6283e0db47..cd06c891c5 100644 --- a/test/MeshWeaver.Hosting.Test/FileSystemAssemblyStoreAtomicityTest.cs +++ b/test/MeshWeaver.Hosting.Test/FileSystemAssemblyStoreAtomicityTest.cs @@ -165,6 +165,43 @@ public void TheStagingNameCannotMatchTheDiscoveryGlob() Assert.Empty(new DirectoryInfo(typeDirectory).GetFiles("*.dll")); } + /// + /// 🚨 A genuine IO fault must NOT be mistaken for the benign "another writer won" race and + /// swallowed into a false return. The two are told apart by the pre-check: having + /// established the target did not exist, the only way it can exist when the move fails is a + /// concurrent writer — a real fault leaves it absent, so it propagates. + /// + /// Here the parent directory does not exist, so the write throws + /// (an ) with no target + /// created. Returning false would tell FileSystemAssemblyStore the assembly was + /// already published and hand back a path to nothing. + /// + [Fact] + public void AGenuineIoFailureIsNotSwallowedAsTheAlreadyPublishedRace() + { + var missingDirectory = Path.Combine(_root, "no-such-dir", "v1-tag-hash.dll"); + + Assert.ThrowsAny(() => AtomicFileWrite.PublishBytes(missingDirectory, new byte[8])); + Assert.False(File.Exists(missingDirectory)); + } + + /// + /// The other half of that contract: when the target genuinely IS already there, publishing + /// reports false and leaves the incumbent's bytes untouched. + /// + [Fact] + public void PublishingOverAnExistingTargetKeepsTheIncumbentAndReportsFalse() + { + var typeDirectory = Path.Combine(_root, NodeTypePath); + Directory.CreateDirectory(typeDirectory); + var target = Path.Combine(typeDirectory, "v1-tag-hash.dll"); + File.WriteAllBytes(target, [1, 2, 3]); + + Assert.False(AtomicFileWrite.PublishBytes(target, [9, 9, 9, 9])); + Assert.Equal(new byte[] { 1, 2, 3 }, File.ReadAllBytes(target)); + Assert.Single(Directory.GetFiles(typeDirectory)); + } + /// /// A completed publication leaves no residue: exactly the DLL and its PDB, nothing else. ///