fix: a NodeType assembly can be loaded mid-write and park the type (#1387) by rbuergi · Pull Request #1393 · Systemorph/MeshWeaver

rbuergi · 2026-08-13T07:23:57Z

The defect

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 — TryGetAssemblyPath's result goes 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.

Confirmed non-exotic. Against the unfixed store, 11 of 12 assemblies were observed discoverable at 0 of 4194304 bytes across 726 observations. This is not a rare interleaving; it is the default outcome of a concurrent glob.

Why one mis-read is not transient (issue ask 2)

MeshNodeCompilationService.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 instant 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 (issue ask 3)

On AKS the store is FileSystemAssemblyStore over /data/assembly-cache — a ReadWriteMany Azure Files share (memex-data, storageClassName: azurefile-memex), 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. BlobAssemblyStore had the same shape twice: the local materialisation in PutWithLocation, and DownloadToAsync writing directly into the path the File.Exists fast path discovers by.

Same defect class as #1359 (FileSystemStorageAdapter, FileSystemVersionStore) — third and fourth site, and this one publishes executable bytes.

The fix

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 — the mistake fix: batch the indexing and import progress logs — the O(n²) shape #1341 left behind #1359 had to undo.
PublishBytes deliberately does NOT replace an existing target. Azure Files SMB does not offer 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 on every filesystem. 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 be able to 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-character pattern extension such as *.dll also matches longer extensions that begin with it. Pinned by a deterministic test rather than left as a convention.
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:

	`ADiscoveredAssemblyIsAlwaysComplete`
before	FAIL — 11 of 12 discoverable at 0 of 4194304 bytes (726 observations)
after	PASS — 0 violations

MeshWeaver.Hosting.Test — 161/161 green.
WhatsNewEntryIntegrityTest green.
Release -warnaserror clean, 0 warnings / 0 errors: Mesh.Contract, Graph, Hosting, Hosting.AzureBlob, Hosting.Test, Documentation.Test.

The assertion is on the size of a discovered assembly, never on 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 sampled nothing cannot pass vacuously, and two deterministic assertions run regardless of what the probe saw.

Not merged with #890

#890's NRE is inside Roslyn's MetadataWriter during Emit (the write side); this is a load-side fault with a different mechanism. Co-occurrence is not evidence of a shared root and they are kept separate.

🤖 Generated with Claude Code

…1387) 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) <noreply@anthropic.com>

Copilot

Pull request overview

This PR fixes a race where NodeType assemblies (DLL/PDB) could be discovered and loaded while still being written, causing “format is invalid” load failures that get persisted as CompilationStatus.Error and permanently park the NodeType until manual intervention.

Changes:

Introduces a shared AtomicFileWrite helper in MeshWeaver.Mesh.Contract and removes the hosting-only implementation.
Updates both FileSystemAssemblyStore and BlobAssemblyStore to publish assemblies via temp-file + rename so a DLL is not discoverable until complete (and publishes PDB before DLL).
Adds deterministic atomicity tests for FileSystemAssemblyStore and a “What’s New” entry describing the fix.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
test/MeshWeaver.Hosting.Test/FileSystemAssemblyStoreAtomicityTest.cs	Adds a concurrency probe + deterministic assertions to pin “discoverable implies complete” for cached assemblies.
src/MeshWeaver.Mesh.Contract/Persistence/AtomicFileWrite.cs	Adds a shared atomic publish/write helper (temp sibling + rename) for reuse across assemblies.
src/MeshWeaver.Hosting/Persistence/FileSystemVersionStore.cs	Switches to the shared `AtomicFileWrite` namespace import.
src/MeshWeaver.Hosting/Persistence/FileSystemStorageAdapter.cs	Switches to the shared `AtomicFileWrite` namespace import.
src/MeshWeaver.Hosting/Persistence/AtomicFileWrite.cs	Removes the old hosting-internal `AtomicFileWrite` implementation.
src/MeshWeaver.Hosting.AzureBlob/BlobAssemblyStore.cs	Materializes blob downloads and local cache writes via atomic temp-file publication to avoid torn reads.
src/MeshWeaver.Graph/Configuration/FileSystemAssemblyStore.cs	Publishes PDB then DLL via `AtomicFileWrite.PublishBytes` to ensure glob-based discovery only sees complete assemblies.
src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-13-a-page-no-longer-breaks-because-its-code-was-read-mid-save.md	Documents the user-visible impact and resolution.

Suppressed comments (1)

src/MeshWeaver.Mesh.Contract/Persistence/AtomicFileWrite.cs:163

PublishAsync has the same broad catch (IOException) when (File.Exists(filePath)) filter as PublishBytes, which can misclassify unrelated IO failures as a benign "another writer won" race and return false. Restrict the filter to the specific "already exists" HRESULTs so genuine failures still surface.

        catch (IOException) when (File.Exists(filePath))
        {
            TryDelete(tempPath);
            return false;
        }

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

github-actions · 2026-08-13T07:40:15Z