fix(#3542): a bookkeeping write that cannot read the policy record refuses it by rbuergi · Pull Request #3619 · Systemorph/MeshWeaver

rbuergi · 2026-09-07T15:27:43Z

Refs #3542 — proposal 3's first part. (The second, "an absent policy must read as None", landed as #3607; nothing here reopens it, and the enum is deliberately not reordered.)

The residue, measured — #3607 did not close it

The brief asked me to check whether #3607 had already closed the clobber. It has not, and here is the measurement, taken on origin/main at 5453be493 (the merge commit of #3607) before any change:

Seed Admin/UpdatePolicy with a real record carrying one field this build cannot deserialize:

{"policy":"None","requireCiGreen":true,"latestAvailableTag":"3.0.0-ci.8009","comboVerifications":"written-by-a-shape-this-build-cannot-read"}

The stream cache says what happens next, in a Warning nobody was reading:

MeshNodeStreamCache.GetStream: Content for Admin/UpdatePolicy stayed an untyped JsonElement
after deserialization — downstream 'Content is X'/'as X' consumers will fail

Then one RecordAvailable-shaped bookkeeping write, which completed silently, leaving:

{"requireCiGreen":true,"latestAvailableTag":"3.0.0-ci.9999","checkedAt":"…","comboVerifications":[]}

policy is gone — the production state #3542 opens on ("a policy record that lost its own policy"), reproduced from a single routine check. Along with it went latestAvailableTag, and in the general case every ComboVerification and any live HeldTag/HeldReason.

Why, and why it is not the field's location

All four writes on the node — RecordCheck, RecordAvailable, RecordHold (SelfUpdateHostedService) and RecordVerification (UpdatePolicyNodeType) — read through UpdatePolicyNodeType.ParseContent, which answers new UpdatePolicyContent() for both "there is no content" and "the content is present and this build cannot read it". Those are different facts. The write then persists that empty record.

So I did not move lastCheckedAt, which is the issue's other suggestion, and the evidence is what decided it: the three sibling writes have the same shape on the same record, so moving one field would leave RecordAvailable, RecordHold and RecordVerification erasing it exactly as before. The defect is a failed read becoming a default that gets written, not the co-location.

The fix invents nothing

The framework already has the primitive built for this exact class, and these four sites had simply never been migrated to it. From MeshNodeStreamHandle.Update<TContent>'s own docs:

🚨 null means ABSENT, never "could not be read". … Conflating the two is what turns "the content did not deserialise" into "the content was empty, write a fresh one" — the write that destroys the real record.

All four now use it. Unreadable content faults with a MeshNodeStreamException naming the path, the runtime type and a JSON excerpt — the write does not happen — and every caller already wraps its bookkeeping write in a .Catch that logs and carries on (#1020: a bookkeeping write may never gate the roll). Refused loudly, record left intact:

MeshNode Deserialization at 'Admin/UpdatePolicy': Update<UpdatePolicyContent> refused to write:
content is untyped JSON (JsonElement) and could not be read as 'UpdatePolicyContent'.
The write was NOT applied — writing a default-valued record here would persist those defaults
over every field the caller never touched.

Also in this change

ParseContent is reimplemented on ObjectAsExtensions.As<T>, so a read additionally covers the as-written JsonNode DOM and a same-named record from another collectible assembly — both of which the hand-rolled JsonElement switch answered with a silent default — and it now documents that it must never feed a write.
UpdatePolicyMcpPatchTest's two "mirrors the production shape exactly" helpers move to the typed write, so the mirror keeps testing the shape it claims to.
AbsentUpdatePolicyFailsClosedTest's prose asserted "None is now the enum's zero value". It is not — Continuous still is, deliberately, because reordering would make an explicit None unwritable under DefaultIgnoreCondition = WhenWritingDefault. Corrected, and the duplicate <summary> block left on DeclaredPolicy is removed.
Work product conserved: Doc/Architecture/CqrsAndContentAccess → "Writes" now teaches the typed overload as the write shape and states the asymmetry plainly — a read may tolerate bad data, a write may not, and building one on the other is what destroys the record. It cites both production instances (AppendUserInput's Status reset, and this one).

Verification

UnreadablePolicyRecordIsNotClobberedTest drives all four production writes against a real mesh — RecordVerification through its public API, the other three through a derived class exposing the poller's own protected virtual members. Each has a positive control on a readable record, because "the write was refused" would otherwise be satisfied by a write that never works.

Falsified. Reverting only the two src files and re-running: exactly the 4 refusal arms fail, the 4 positive controls pass. With the fix: 8/8. Then 93/93 across every self-update suite in the project (ComboGateRollTest, ComboVerdictRecordingTest, SelfUpdateStrandRecoveryTest, UpdatePolicyMcpPatchTest, NonAdminUpdateStatusTest, PlatformUpdateStatusTest, VersionSelectTest, AbsentUpdatePolicyFailsClosedTest).

One thing worth recording, because it wasted a cycle: the three poller arms first failed their positive control with a one-shot read of the node — the write had landed and the read answered with the pre-write snapshot. Fixed by waiting on the condition (.Where(settled).FirstAsync().Timeout(…)), which is the house rule; the refusal arms keep the single read deliberately, and say why in a comment — a refusal happens inside the update lambda, so no patch is posted and there is no later emission to wait for.

dotnet build -c Release -warnaserror, one project per invocation: Memex.Portal.Shared and Memex.Portal.Shared.Test, both 0 Warning(s) 0 Error(s). DocumentationLinkIntegrityTest + WhatsNewEntryIntegrityTest green after the doc edits.

What's New: Category: Fix — "Your update settings survive the portal's own bookkeeping".

#3542 stays open after this: proposal 3 is then complete, but the issue also still carries the ~1300 [MergeGuard] Admin/UpdatePolicy: refused stale/reordered cross-hub write events, which have no fix on main and are a separate root.

🤖 Generated with Claude Code

…fuses it Proposal 3's remaining half. #3607 made an ABSENT `policy` field read as `None` rather than as "enabled" — that fixed what an absent field MEANS, but not the write that makes it absent. All four writes on `Admin/UpdatePolicy` — RecordCheck, RecordAvailable, RecordHold and RecordVerification — read the current content through `UpdatePolicyNodeType.ParseContent`, which answers `new UpdatePolicyContent()` for BOTH "there is no content" and "the content is present and this build cannot read it", and then persisted that empty record. A failed READ became a write that erased the admin's policy, the latest available tag, every combo verdict and any live availability hold. MEASURED on origin/main at 5453be4 (i.e. WITH #3607): seed `{"policy":"None","latestAvailableTag":"3.0.0-ci.8009","comboVerifications":"corrupt"}` — one field this build cannot deserialize, so the record reaches the write lambda as an untyped JsonElement — then one RecordAvailable-shaped write completed SILENTLY and left `{"requireCiGreen":true,"latestAvailableTag":"3.0.0-ci.9999","comboVerifications":[]}`. No `policy` at all: exactly the production state #3542 opens on, from one bookkeeping write. The fix invents nothing. The framework already has the primitive built for this class — `Update<TContent>((node, content) => …)`, whose own docs say "null means ABSENT, never 'could not be read'… Conflating the two is what turns 'the content did not deserialise' into 'the content was empty, write a fresh one' — the write that destroys the real record." The four sites were simply never migrated to it. They are now: unreadable content faults with a MeshNodeStreamException carrying the path, the runtime type and a JSON excerpt, the write does not happen, and every caller's existing `.Catch` logs it and carries on (#1020: a bookkeeping write may never gate the roll). Moving `lastCheckedAt` to a separate node — the other repair the issue proposes — would NOT have closed this: the three sibling writes have the same shape on the same record, so the defect is read-failure-becomes-a-default, not the co-location. Also here: - ParseContent is reimplemented on ObjectAsExtensions.As<T>, so a READ additionally covers the as-written JsonNode DOM and a same-named record from another collectible assembly — both of which the hand-rolled JsonElement switch answered with a silent default — and it now documents that it must never feed a write. - UpdatePolicyMcpPatchTest's two production-shape mirrors move to the typed write, so the mirror keeps testing the shape it claims to. - AbsentUpdatePolicyFailsClosedTest's prose said "None is now the enum's zero value"; it is not, deliberately (reordering would make an explicit None unwritable under DefaultIgnoreCondition = WhenWritingDefault). Corrected, and the duplicate <summary> block #3607 left on DeclaredPolicy is removed. - Doc/Architecture/CqrsAndContentAccess now teaches the typed write as THE write shape, with the read/write asymmetry stated: a read may tolerate bad data, a write may not, and building one on the other is what destroys the record. Verification: UnreadablePolicyRecordIsNotClobberedTest drives all four PRODUCTION writes (RecordVerification is the public API; the other three are the poller's own protected virtuals reached through a derived class) against a real mesh. Falsified by reverting only the src change and re-running: exactly the 4 refusal arms fail and the 4 positive controls pass; with the fix, 8/8. 93/93 across every self-update suite in the project. `dotnet build -c Release -warnaserror` clean, 0 warnings, on Memex.Portal.Shared and Memex.Portal.Shared.Test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🟢 Approval recommended

The change consistently applies the typed-update mechanism across all relevant writers, preserves the “bookkeeping must not gate” behavior via existing .Catch handling, and is backed by a targeted regression test plus positive controls.

Pull request overview

This PR addresses #3542’s remaining “policy clobber” failure mode by ensuring bookkeeping writes to Admin/UpdatePolicy refuse unreadable existing content instead of defaulting it and persisting those defaults back onto the node (which previously erased fields like policy, tags, holds, and combo-verification history).

Changes:

Migrates all Admin/UpdatePolicy bookkeeping writes (RecordCheck, RecordAvailable, RecordHold, RecordVerification) to the typed GetMeshNodeStream(path).Update<TContent>((node, cur) => …) overload so unreadable content faults and the write is not applied.
Reworks UpdatePolicyNodeType.ParseContent to use ObjectAsExtensions.As<T> (broader read tolerance: typed instance, JsonElement, JsonNode, and foreign-assembly same-named types), while documenting it as read-only (must not feed writes).
Adds a regression test that drives all four production write paths against an unreadable record and proves it is refused (and also adds positive controls), plus updates docs/What’s New to teach the typed-write shape and the read/write asymmetry.

File summaries

File	Description
test/Memex.Portal.Shared.Test/UpdatePolicyMcpPatchTest.cs	Updates test helpers to mirror the production typed-write shape for `Admin/UpdatePolicy` mutations.
test/Memex.Portal.Shared.Test/UnreadablePolicyRecordIsNotClobberedTest.cs	New regression test proving unreadable policy content is refused (not clobbered) across all four production write paths, with positive controls.
test/Memex.Portal.Shared.Test/AbsentUpdatePolicyFailsClosedTest.cs	Corrects explanatory prose about enum zero-value and ties the “absent means None” behavior to the new clobber-prevention coverage.
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-07-your-update-settings-survive-the-portals-own-bookkeeping.md	Adds user-facing release note describing the fixed failure mode (unreadable record no longer overwritten).
src/MeshWeaver.Documentation/Data/Architecture/CqrsAndContentAccess.md	Updates guidance to make `Update<TContent>` the primary write pattern and explicitly warns against building writes on bad-data-tolerant reads.
memex/Memex.Portal.Shared/SelfUpdate/UpdatePolicyNodeType.cs	Switches `RecordVerification` to typed update; reimplements `ParseContent` via `ObjectAsExtensions.As<T>` and documents why it must not drive writes.
memex/Memex.Portal.Shared/SelfUpdate/SelfUpdateHostedService.cs	Switches `RecordCheck`, `RecordAvailable`, and `RecordHold` to typed updates; existing call sites continue to treat these as best-effort bookkeeping via `.Catch(...)`.

Review details

Files reviewed: 7/7 changed files
Comments generated: 0
Review effort level: Lite

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

github-actions · 2026-09-07T15:47:46Z