fix(#3623): five more writes refuse a record they cannot read instead of overwriting it by rbuergi · Pull Request #3631 · Systemorph/MeshWeaver

rbuergi · 2026-09-07T16:35:15Z

Closes #3623.

The class, and why it is silent

A read of MeshNode.Content must stay bad-data tolerant: ContentAs<T> / As<T> answer null
rather than throw, because a settings tab that throws is worse than one showing a fail-closed
default. That same tolerance destroys data on a write. Inside stream.Update(node => …),
node.ContentAs<T>(opts) ?? new T() makes "there is nothing here" and "I could not read what is
here" the same answer, and the write then persists a default-valued record over every field the
caller never touched. The write completes, nothing is logged, and the loss surfaces later as a field
that "went missing" — which is how Admin/UpdatePolicy lost its own policy ahead of a portal
rolling itself onto a withdrawn version line (#3542), and how ThreadInput.AppendUserInput reset
Status to Idle (the CheckInbox flake).

All three unreadable shapes happen in a running mesh and none of them throws: untyped JSON whose
$type will not resolve, the as-written JsonObject DOM before the materialisation pipeline
re-types it, and a same-named record from another collectible assembly — every NodeType recompile
mints one.

Established per site, before changing anything

The issue named five. Each was read through to the write it feeds; two of them are worse than the
issue states and one is narrower, and both corrections are in the table.

site	content	does the write actually happen?
`PresentationLayoutArea.cs`	`User`	Yes, in BOTH directions. `new User().HiddenPaths` is `null`; `ApplyMark(null, path, hide:true)` returns a fresh list and `hide:false` returns `ImmutableList.Empty` — neither is reference-equal to `null`, so the guard `ReferenceEquals(marks, user.HiddenPaths)` passes them both through. A viewer's whole account record replaced by defaults on a hide or a show click. The issue said "on a hide/show mark"; it is in fact not idempotent-safe in either direction.
`GitHubWebhookProcessor.cs`	`GitHubSyncConfig`	Yes, unconditionally — the token reference, branch, path mapping and `LastSyncCommitSha` of a repo whose only sin was being renamed.
`BuildCoordinationExtensions.cs`	`BuildState`	Yes. An empty state fails both early-outs, so the write lands: every OTHER candidate's registration, the live `ClaimedBy`/`Status`, the `Ready` map and every `StoodDown` mark, replaced by defaults plus this one registration.
`BuildNodeType.CommitGrant`	`BuildState`	Yes, and this one is DURABLE. Not a `stream.Update` at all — it decides on the claim LOCK read from storage and commits with `WriteIfVersion`. Deciding on an empty state means "nobody holds the build, nothing is stale", and the compare-and-set then succeeds because it diffs against the real row's version. The live holder is evicted from the one witness every cluster's arbiter reads, and a second full bake starts on a build already running.
`BuildNodeType.ApplyGrant`	`BuildState`	Narrower than the issue states, and worth saying so. With a holder-carrying grant the candidate guard happens to refuse — the right outcome for the wrong reason, and silently. It becomes a write when `granted.ClaimedBy` is `null`, and every guard in between is meanwhile evaluating a fiction. Measured by falsification below, not by argument.
`ActivityLogLogger.cs`	`ActivityLog`	Yes, on every 100 ms flush. Exactly the fields the lambda does NOT set are the ones lost: the `Id`, `HubPath`, `Start` and `User` the dispatcher stamped, and any `RequestedStatus` a concurrent cancel had just written. The comment above that write claims those "ride through untouched"; on an unreadable record it was false.

The fix

Four sites take the framework primitive. Update<TContent>((node, content) => …) — null means
ABSENT and only absent, so create-on-absent is preserved exactly, while present-but-unreadable
content faults with a MeshNodeStreamException naming the path, the runtime type and a JSON
excerpt, with the write not applied. Every one of them already had an onError arm, so the
refusal is reported where the failure was already reported.

Two sites do not fit it, and here is why. CommitGrant is not a stream.Update — it decides on
a storage read and commits a compare-and-set — and ApplyGrant is a pure decision function called
from inside an untyped lambda. Both now do by hand what the primitive does: null when Content is null, and refuse loudly when it is present and unreadable. Both refuse by writing nothing and
logging what was withheld, never by throwing — a throw at ApplyGrant would strand the lock the
pass just took, because a refused publication is precisely what HandBackAStoodDownGrant reads in
order to release it. Refusing this way is also what the three sibling deciders on that node already
do (StandDown, ReleaseStoodDownClaim, Arbitrate); they were simply silent about it.

The nullable-backing-field shape from #3607 does not apply here and was deliberately not
reached for. That shape solves a different problem — an enum whose zero value is the permissive
state, so an absent field and a chosen value are indistinguishable within one record. Nothing
here has a defaulted member standing in for absence; the ambiguity is between "no content" and
"unreadable content", one level up, and the type argument is what resolves it.

ApplyGrant gains an optional ILogger? (internal, so no public surface moves) and
CommitGrant's lock read is extracted as ReadLockStateOrRefuse — a decision, testable the same
way ApplyGrant and StandDown already are, which is the pattern this file states for itself.

The control that covers the denominator, not the five

DefaultedContentReadInsideAnUpdateLambdaGuard scans src/ and memex/ for an untyped .Update(x => …) lambda containing ContentAs<T>(…) ?? new or … .Content as T ?? new, and holds it at
zero. Five bespoke tests would cover five sites; this covers the 72 untyped lambdas that exist
today and every one added tomorrow. It scans comment-masked source (the fixed files quote the old
shape to explain why it went), and it asserts its own denominator — fewer than 40 lambdas found
means the regex stopped matching, not that the tree got clean.

There is no allow file, deliberately. The correction is mechanical and preserves behaviour
exactly when the record is readable, so an exemption could only ever mean "this one may keep
destroying records".

🚨 Falsified — every new assertion made to fail against the pre-fix code

Reverted the fixes, rebuilt, re-ran; exit codes read directly, never through a pipe.

reverted	result
`BuildCoordinationExtensions` back to the untyped lambda	the guard fails, naming `src/MeshWeaver.Graph/BuildCoordinationExtensions.cs:99 — ContentAs<BuildState>(hub.JsonSerializerOptions) ?? new`
`ApplyGrant` back to `?? new BuildState()`	`AGrantWithNoHolder_NeverOverwritesAMirrorThisBuildCannotRead` fails — "Expected value to refer to the same instance … but it did not", i.e. it wrote; and `TheRefusalIsRecorded_NamingTheNodeAndTheHolder` fails with an empty log
`ReadLockStateOrRefuse` back to `?? new BuildState()`	`AnUnreadableLockRow_RefusesTheWholePass` fails

Each new arm is paired with a non-vacuity control on the same input — a readable mirror still
publishes, an absent lock row still inserts, a readable one comes back holder and all — because
"the write was refused" is otherwise satisfied by an arbiter that never grants anything, and the
whole protocol would be dead with every test green. The unreadable fixture is itself asserted to be
unreadable (TheUnreadableFixture_IsActuallyUnreadable): a JSON shape the record happened to
tolerate would make every arm pass against the very defect they refuse.

🚨 One thing the next person must know, and it touches #3619

These tests model "present but unreadable" as malformed JSON, which is safe here because they
are pure decisions that never reach MeshNodeStreamCache. A test that drives a REAL mesh must not
use that shape. Once #3625 lands, an unreadable node read through the stream cache emits a
degradation record that reaches the trace log, and check-untyped-content.sh reds the shard on it —
correctly: the fixture really is content nothing can read.

The shape that works on a real mesh, and is a better model of production, is to seed a value of a
different, registered type: the cache types it happily, so nothing degrades and nothing is
recorded, while ContentAs<T> still answers null because it recovers a foreign runtime type only
when the short name matches. That is the same-named-record-from-another-assembly case, which is the
one that actually happens. Written up in Doc/Architecture/ReadingCiSignals by #3625.

#3619's UnreadablePolicyRecordIsNotClobberedTest uses the malformed-JSON shape on a real mesh
and will red its shard once both land. It needs the other fixture shape before #3625 merges.

Verification

dotnet build -c Release -warnaserror, one project per invocation, exit read directly:
MeshWeaver.Graph, MeshWeaver.GitSync, MeshWeaver.Kernel.Hub, MeshWeaver.Graph.Test,
MeshWeaver.Documentation.Test — all 0 Warning(s) 0 Error(s).
BuildGrantPublicationTest 27/27 (20 pre-existing + 7 new).
MeshWeaver.Graph.Test filtered to Presentation|Build — 172/172.
MeshWeaver.Documentation.Test 323/323 — the whole guard wall, including the new ratchet and
DocumentationLinkIntegrityTest / WhatsNewEntryIntegrityTest after the doc edits.

Work product conserved

Doc/Architecture/CqrsAndContentAccess → "Writes" now teaches the typed overload as the write
shape, states the read/write asymmetry plainly, cites both production instances, and names the rule
for the writes that are not stream.Update (refuse by writing nothing and logging, not by throwing,
wherever a throw would strand a lock the caller must hand back). This is the same section #3619
rewrites — whichever lands second resolves; the content agrees.

What's New: Category: Fix — "A record the portal cannot read is no longer a record it overwrites".

🤖 Generated with Claude Code

… of overwriting it A read of MeshNode.Content stays bad-data tolerant on purpose. That same tolerance destroys the record on a WRITE: `node.ContentAs<T>(opts) ?? new T()` inside an untyped `Update(node => …)` makes "there is nothing here" and "I could not read what is here" the same answer, and the write persists a default-valued record over every field the caller never touched — silently. Established per site before changing anything, and two of the five are worse than #3623 states: PresentationLayoutArea replaces a viewer's whole ACCOUNT in BOTH mark directions (ApplyMark returns Empty, never null, so the reference-equality guard passes it through), and CommitGrant's write is DURABLE — it decides on the claim LOCK read from storage and commits with WriteIfVersion, which succeeds because it diffs the real row's version. One is narrower: ApplyGrant's defaulted state only becomes a write when granted.ClaimedBy is null, and until then it refuses for the wrong reason, silently. Four sites take the framework primitive, Update<TContent>, whose null means ABSENT and only absent. Two cannot: CommitGrant is not a stream.Update, and ApplyGrant is a pure decision inside one. Both now refuse by writing nothing and LOGGING what they withheld — never by throwing, because a throw at ApplyGrant would strand the lock the pass just took (a refused publication is what HandBackAStoodDownGrant reads to release it). #3607's nullable-backing-field shape solves a different problem and was deliberately not reached for: nothing here has a defaulted member standing in for absence. DefaultedContentReadInsideAnUpdateLambdaGuard holds the shape at ZERO across src/ and memex/ — the 72 untyped lambdas that exist and every one added tomorrow, not just the five — asserting its own denominator, with no allow file. Falsified: reverting each of the three fixes with new assertions makes exactly those assertions fail (the guard naming BuildCoordinationExtensions.cs:99; ApplyGrant's no-holder arm reporting that it wrote; the lock-row arm finding a default BuildState). Every arm is paired with a non-vacuity control on the same input. Refs #3623 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…ntent-writes # Conflicts: # src/MeshWeaver.Documentation/Data/Architecture/CqrsAndContentAccess.md

Copilot

🟡 Changes recommended

The new deserialization-refusal Warning in ActivityLogLogger can log at 100ms cadence while the record remains unreadable, which risks operational log flooding and should be throttled (e.g., warn once per activity/path).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR prevents silent data loss caused by defaulting unreadable MeshNode.Content inside untyped stream.Update(node => …) lambdas by switching affected writes to typed Update<TContent>((node, content) => …) (or explicitly refusing when typed update isn’t usable), and it adds a guard/test coverage plus docs/What’s New to keep the fix from regressing.

Changes:

Migrate several write sites to typed Update<TContent> so “absent content” and “present-but-unreadable content” are no longer conflated during writes.
Add regression tests around build-grant publication/lock-row refusal and a repository-wide guard test to ratchet the problematic pattern to zero.
Update documentation and add a What’s New entry describing the user-visible impact.

File summaries

File	Description
test/MeshWeaver.Graph.Test/BuildGrantPublicationTest.cs	Adds targeted tests for refusal behavior when build state content is present but unreadable, including logging assertions.
test/MeshWeaver.Documentation.Test/DefaultedContentReadInsideAnUpdateLambdaGuard.cs	Introduces a repo-wide guard test that fails if an untyped `Update` lambda defaults a failed content read to `new …`.
src/MeshWeaver.Kernel.Hub/ActivityLogLogger.cs	Switches activity log snapshot writes to typed `Update<ActivityLog>` and distinguishes deserialization refusals in logging.
src/MeshWeaver.Graph/PresentationLayoutArea.cs	Switches user record updates to typed `Update<User>` to avoid clobbering unreadable user content on hide/show clicks.
src/MeshWeaver.Graph/Configuration/BuildNodeType.cs	Refactors build-lock reading into `ReadLockStateOrRefuse`, adds refusal logging, and threads optional logger through `ApplyGrant`.
src/MeshWeaver.Graph/BuildCoordinationExtensions.cs	Switches build-claim registration to typed `Update<BuildState>` to avoid overwriting unreadable build coordination state.
src/MeshWeaver.GitSync/GitHubWebhookProcessor.cs	Switches GitHub sync config repointing to typed `Update<GitHubSyncConfig>` to avoid erasing unreadable config.
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-07-a-record-the-portal-cannot-read-is-no-longer-a-record-it-overwrites.md	Adds a user-facing release note describing the fixed clobbering behavior and affected areas.
src/MeshWeaver.Documentation/Data/Architecture/CqrsAndContentAccess.md	Updates the “Writes” guidance to recommend typed `Update<TContent>` and documents the failure mode being prevented.

Review details

Files reviewed: 9/9 changed files
Comments generated: 1
Review effort level: Lite

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

+                            if (ex is MeshNodeStreamException { Error.Code: MeshNodeErrorCode.Deserialization })
+                                _diagnostics?.LogWarning(ex,
+                                    "ActivityLogLogger: REFUSED to publish the log snapshot for {Path} "
+                                    + "— the activity record is present and could not be read as "
+                                    + "ActivityLog. NOTHING was written: a default-valued record would "


github-actions · 2026-09-07T17:32:44Z