fix(#3213): an update reminder is a STATE — told once per candidate, not once per poll by rbuergi · Pull Request #3230 · Systemorph/MeshWeaver

rbuergi · 2026-09-03T17:01:42Z

The defect

PackageUpdateReconciler.Notify minted a fresh Notification node on every reconcile that
still saw a pending update. Measured on memex.meshweaver.cloud on 2026-09-03: 124 of the newest
200 notification rows were this one emitter — four rows for the same package in a single day, two
packages accounting for over half of everything the mesh had written in four days. Those rows are
the bell's row count for everyone who can read the plugin catalogue, so it was a cost paid on every
bell render, not just clutter.

Two independent halves — fixing either alone still duplicates

Both were confirmed against the source before changing anything.

Half 1 — the silence gate asked a question that could never become true

Decide gates on content identity: record.ModuleVersion == pkg.ModuleVersion, i.e. "is the
candidate INSTALLED?".

On the AutoUpdate path that is self-silencing — Apply installs, the record's
ModuleVersion advances, the next reconcile compares equal.
On the notify path it is unsatisfiable. Notify installs nothing — by design, the user has
not acted — so ModuleVersion never advances and the comparison evaluated false on every
subsequent reconcile, forever. And the reconcile runs often: at boot, whenever a feed read drains
a deferred registry, and on every build webhook.

Fix: PackageManifest.NotifiedModuleVersion — the candidate this installation has already told
the user about, on the durable install record. Notify compares against it and then writes it, so
writing it is what makes the gate become true, which is precisely what the version comparison
could never do here. It lives on the node rather than in a cache because a cache loses its memory on
every pod start and re-notifies — that is the symptom.

Half 2 — the notification had no identity for a repeat to collide with

NotificationService.CreateNotification minted Guid.NewGuid() per call, so a repeat landed at a
new path rather than overwriting one.

Fix: an optional identity parameter. When given, the node id is derived deterministically from
(mainNodePath, identity) through PartitionSourceFingerprint — the platform's existing
content-addressing helper, the same shape as the {Partition}/_Activity/import-{fingerprint} marker
— and the write becomes an atomic CreateOrUpdateNode (the owner's single upsert verb, not a
client-side create-then-update split, which races). The reconciler keys it on (record, kind,
candidate version). Default null keeps every existing caller on the historic behaviour.

Why the gate is inside `Notify` and not in front of `Apply`

An unattended apply must keep retrying every reconcile so a restored admin grant heals itself.
Only the telling is once per candidate — which is also why the commercial-refusal notification
("Update needs a Global Admin") is gated by the same marker while Apply still runs each pass.

Order within the notify path is deliberate: bell first, marker second. A marker written first
whose notification then failed would silence the reminder permanently; a bell written first whose
marker then failed costs one repeat, which the deterministic identity absorbs into the same node.

A genuinely newer candidate changes both the comparison and the derived id, so it still gets its own
unread bell. No poll-count cap, no time window, no de-dup cache.

Verification — proved by mutation, both halves

test/Memex.Portal.Shared.Test/PackageUpdateReminderIdempotenceTest.cs drives the real reconciler
against a real monolith mesh and a real RegistryPackageSource over four reconcile passes.

Mutation	Result
marker gate disabled (`if (false && …)`)	❌ "the second pass re-raised a reminder the user had already dismissed — the reminder path's silence gate did not suppress it"
deterministic id disabled (`var deterministic = false && …`)	❌ `Assert.Single() Failure: The collection contained 2 items`
both in place	✅

🚨 Counting rows cannot test half 1, and a fixture that only counted would pass having checked
nothing: with the deterministic identity in place an unsuppressed second pass lands on the same
node, so the count stays 1 either way. What separates suppressed from absorbed is what the user
sees — so the fixture dismisses the bell first and asserts it stays dismissed (that re-lit bell
is the defect a reader experiences four times a day). The first attempt at this test passed under
the half-1 mutation for exactly that reason; it was rewritten rather than accepted.

A fourth arm is the falsifier the suppression needs: a genuinely new candidate version must
still raise its own unread reminder — a gate keyed too coarsely would pass every other arm while
breaking the feature. The negative control asserts zero registry round-trips across all four
reconciles ("no notification, and not one file fetched").

Local runs, all green:

dotnet build -c Release -warnaserror (whole solution) — 0 Warning(s), 0 Error(s)
Memex.Portal.Shared.Test — 698/698 (includes RegistryReconcileDeferralTest, which exercises
this same notify path)
MeshWeaver.Graph.Test — 1269/1269 (includes NotificationServiceTest, NotificationDispatchTest,
NotificationDispatchIdentityLatchTest)
ImpersonationScopeSiteRatchetGuard, WhatsNewEntryIntegrityTest,
DocumentationLinkIntegrityTest, NoConflictMarkersGuard — 6/6

Also in this change

Notify's Observable.Using(ImpersonateAsSystem, …) → RunAsSystem (ActivityLogLogger.SealOverflowLocked latches system-security on the script thread (same shape as #1784's review finding) #1790 — the subscribing
thread was left latched as System). The impersonation ratchet shrinks:
PackageUpdateReconciler.cs 2 → 1, TotalBudget 69 → 68.
Docs: a new section in
PluginUpdateOnGreenBuild
("A reminder is told ONCE — the gate the content-identity check cannot be"), a failure-mode row,
and the AddressedNotifications §5 bullet marked fixed. The addressing half of this emitter
(Deliver notifications to the recipient's partition so the bell stops fanning out over all schemas (444 unions / 5 min on memex-cloud) #3156 §7 step 1 — re-address to Admin) is deliberately still open; it is independent of
suppression.
What's New entry, Category: Fix.

Pairs-with: none — this change removes no public type or member; it only adds
PackageManifest.NotifiedModuleVersion and an optional parameter on
NotificationService.CreateNotification (source-compatible for every existing caller).

🤖 Generated with Claude Code

…not once per poll `PackageUpdateReconciler.Notify` minted a fresh `Notification` node on every reconcile that still saw a pending update. Measured on memex.meshweaver.cloud 2026-09-03: 124 of the newest 200 notification rows were this one emitter, four rows for the same package in a day, two packages accounting for over half of everything the mesh had written in four days — and those rows are the bell's row count for every viewer who can read the plugin catalogue. TWO independent halves, and fixing either alone still duplicates: 1. The silence gate asked a question that could never become true. `Decide`'s content-identity gate asks "is the candidate INSTALLED?" — self-silencing on the AutoUpdate path (the apply advances `record.ModuleVersion`) and UNSATISFIABLE on the notify path, which installs nothing by design. So it evaluated false on every subsequent reconcile, forever, and the reconcile runs at boot, on every feed read that drains a deferred registry, and on every build webhook. → `PackageManifest.NotifiedModuleVersion` records the candidate the user was last reminded about, on the durable install record; `Notify` compares against it and writes it. Writing it is what makes the gate BECOME true. On the record rather than in a cache because a cache loses its memory on every pod start and re-notifies — which is the symptom itself. 2. The notification had no identity for a repeat to collide with. Every `NotificationService.CreateNotification` call minted a fresh GUID, so a repeat landed at a new path rather than overwriting. → an optional `identity` derives the node id deterministically from (mainNodePath, identity) via `PartitionSourceFingerprint` — the platform's existing content-addressing helper, the same shape as the `_Activity/import-{fingerprint}` marker — and makes the write an atomic `CreateOrUpdateNode`. The reconciler keys it on (record, kind, candidate version). The gate sits inside `Notify`, deliberately not in front of `Apply`: an unattended apply must keep retrying every reconcile so a restored admin grant heals itself. Only the TELLING is once per candidate. The bell is written first and the marker second — a marker written first whose notification then failed would silence the reminder permanently, whereas a bell written first whose marker then failed costs one repeat, which the deterministic identity absorbs into the same node. A genuinely newer candidate changes both the comparison and the derived id, so it still gets its own unread bell. No poll-count cap, no time window, no de-dup cache. Verified by mutation, both halves, each with its own failing arm: - disable the marker gate → "the second pass re-raised a reminder the user had already dismissed" (counting rows CANNOT catch this half — the deterministic upsert keeps the count at one either way, so the fixture dismisses the bell and asserts it stays dismissed) - disable the deterministic id → Assert.Single failure, 2 rows A fourth arm proves the suppression is not too coarse: a new candidate version still raises its own unread reminder. And the negative control asserts zero registry round-trips across four reconciles. Also converts `Notify`'s `Observable.Using(ImpersonateAsSystem, …)` to `RunAsSystem` (#1790) and lowers the impersonation ratchet: PackageUpdateReconciler.cs 2 → 1, TotalBudget 69 → 68. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🟡 Changes recommended

The new test awaits IObservable chains directly; per MeshWeaver.Messaging.ObservableAwait it should use .Await() to avoid Rx inline-resumption continuation hazards.

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

Pull request overview

This PR fixes a notification storm in the plugin update reconciler by making “Update available” reminders idempotent per candidate version, so a standing update condition produces one durable reminder (state) rather than a new notification node on every reconcile poll.

Changes:

Adds a durable “already notified” marker (PackageManifest.NotifiedModuleVersion) and uses it in PackageUpdateReconciler.Notify to suppress repeat reminders for the same candidate.
Extends NotificationService.CreateNotification with an optional deterministic identity that switches the write to an atomic upsert (stable node id per condition) instead of minting a new GUID each time.
Adds an idempotence regression test and updates documentation/What’s New to capture the new “reminder is state” behavior.

File summaries

File	Description
test/MeshWeaver.Documentation.Test/ImpersonationScopeSiteRatchetGuard.cs	Lowers impersonation-site budget after reducing a production root.
test/Memex.Portal.Shared.Test/PackageUpdateReminderIdempotenceTest.cs	New regression test exercising both suppression (marker) and deterministic identity behaviors.
test/ImpersonationScopeSites.allow	Updates allow-list counts for `PackageUpdateReconciler`.
src/MeshWeaver.PluginCatalog/PackageUpdateReconciler.cs	Adds marker-based silence gate + deterministic notification identity; replaces `Observable.Using(ImpersonateAsSystem, …)` with `RunAsSystem`.
src/MeshWeaver.PluginCatalog/Package.cs	Adds `PackageManifest.NotifiedModuleVersion` marker field.
src/MeshWeaver.Graph/NotificationService.cs	Adds optional `identity` parameter and uses `PartitionSourceFingerprint` + `CreateOrUpdateNode` for deterministic, idempotent notifications.
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-03-an-update-reminder-is-told-once-not-every-hour.md	What’s New entry describing the user-visible behavior change.
src/MeshWeaver.Documentation/Data/Architecture/PluginUpdateOnGreenBuild.md	Documents why the content-identity gate cannot silence reminders and how the new gate works.
src/MeshWeaver.Documentation/Data/Architecture/AddressedNotifications.md	Marks the unbounded plugin-update notification issue as fixed (suppression half).

Review details

Suppressed comments (10)

test/Memex.Portal.Shared.Test/PackageUpdateReminderIdempotenceTest.cs:123

Avoid awaiting an IObservable directly here; use ObservableAwait.Await() to prevent inline continuation resumption on the signalling thread.

        var afterSecond = await Reminders().FirstAsync().Timeout(TestTimeouts.Convergence);

test/Memex.Portal.Shared.Test/PackageUpdateReminderIdempotenceTest.cs:145

Avoid awaiting an IObservable directly here; use ObservableAwait.Await() to prevent inline continuation resumption on the signalling thread.

        var afterThird = await Reminders().FirstAsync().Timeout(TestTimeouts.Convergence);

test/Memex.Portal.Shared.Test/PackageUpdateReminderIdempotenceTest.cs:162

This awaits an IObservable directly; prefer ...Timeout(...).Await() (MeshWeaver.Messaging.ObservableAwait) so the continuation is scheduled asynchronously.

        var afterNew = await Reminders()
            .Where(ns => ns.Any(n => n.Path != reminderPath))
            .FirstAsync().Timeout(TestTimeouts.Convergence);

test/Memex.Portal.Shared.Test/PackageUpdateReminderIdempotenceTest.cs:217

This awaits an IObservable directly; use .Await() to bridge the observable to a Task without inline continuation resumption on the signalling thread.

        await access.RunAsSystem(() => NodeFactory.CreateOrUpdateNode(record))
            .Timeout(TestTimeouts.Convergence);

test/Memex.Portal.Shared.Test/PackageUpdateReminderIdempotenceTest.cs:236

This awaits an IObservable directly; use .Await() to bridge the observable to a Task without inline continuation resumption on the signalling thread.

        await access
            .RunAsSystem(() => Mesh.GetMeshNodeStream(notificationPath)
                .Update<Notification>(current => current with { IsRead = true }))
            .Timeout(TestTimeouts.Convergence);

test/Memex.Portal.Shared.Test/PackageUpdateReminderIdempotenceTest.cs:249

This awaits an IObservable directly; use .Await() (MeshWeaver.Messaging.ObservableAwait) to avoid Rx's inline-resumption awaiter.

    private async Task AwaitMarker(string expected) =>
        await Mesh.GetMeshNodeStream(RecordPath)
            .Select(n => n.ContentAs<PackageManifest>(Json)?.NotifiedModuleVersion)
            .Where(v => string.Equals(v, expected, StringComparison.Ordinal))
            .FirstAsync().Timeout(TestTimeouts.Convergence);

test/Memex.Portal.Shared.Test/PackageUpdateReminderIdempotenceTest.cs:185

PackageUpdateReconciler.ReconcileInstalled(...).Timeout(...) is an IObservable; awaiting it directly uses Rx's awaiter. Use .Await() to avoid inline continuation resumption (same defect class as .ToTask()).

        => await PackageUpdateReconciler
            .ReconcileInstalled(Mesh, source, "HEAD", [Candidate(candidate)], provenance, logger)
            .Timeout(TestTimeouts.Convergence);

test/Memex.Portal.Shared.Test/PackageUpdateReminderIdempotenceTest.cs:226

This awaits an IObservable directly; use .Await() to bridge the observable to a Task without inline continuation resumption on the signalling thread.

        await access
            .RunAsSystem(() => Mesh.GetMeshNodeStream(RecordPath)
                .Update<PackageManifest>(current => current with { NotifiedModuleVersion = value }))
            .Timeout(TestTimeouts.Convergence);

test/Memex.Portal.Shared.Test/PackageUpdateReminderIdempotenceTest.cs:243

This awaits an IObservable directly; use .Await() (MeshWeaver.Messaging.ObservableAwait) to avoid Rx's inline-resumption awaiter.

    private async Task AwaitRead(string notificationPath, bool expected) =>
        await Mesh.GetMeshNodeStream(notificationPath)
            .Select(n => n.ContentAs<Notification>(Json)?.IsRead)
            .Where(v => v == expected)
            .FirstAsync().Timeout(TestTimeouts.Convergence);

test/Memex.Portal.Shared.Test/PackageUpdateReminderIdempotenceTest.cs:172

This awaits an IObservable directly; prefer ...Timeout(...).Await() (MeshWeaver.Messaging.ObservableAwait) so the continuation is scheduled asynchronously.

        Assert.Equal(2, (await Reminders().FirstAsync().Timeout(TestTimeouts.Convergence)).Count);

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.

github-actions · 2026-09-03T17:10:49Z