diff --git a/src/MeshWeaver.Documentation/Data/Architecture/AddressedNotifications.md b/src/MeshWeaver.Documentation/Data/Architecture/AddressedNotifications.md index 50c8d38879..95dccdfe26 100644 --- a/src/MeshWeaver.Documentation/Data/Architecture/AddressedNotifications.md +++ b/src/MeshWeaver.Documentation/Data/Architecture/AddressedNotifications.md @@ -252,10 +252,16 @@ Both are filed: MeshWeaver.Plugins#1275 and #3213. `CreatedBy` — which also makes the migration in §6 derivable. `NotificationTriageService.cs:104` derives its recipient the same way and escalates into `{space}/_Triage`, with the same defect. **MeshWeaver.Plugins#1275.** -- **Plugin-update notifications are unbounded.** `PackageUpdateReconciler.Notify` raises a fresh - `Notification` node on every reconcile that still sees an update — measured at four rows for the - same package in one day, 124 of the newest 200 rows overall. There is no "already told you" - suppression. This is the bell's row count, independent of where the rows live. **#3213.** +- ~~**Plugin-update notifications are unbounded.**~~ **FIXED (#3213).** `PackageUpdateReconciler.Notify` + used to raise a fresh `Notification` node on every reconcile that still saw an update — measured + at four rows for the same package in one day, 124 of the newest 200 rows overall. It had no + "already told you" suppression, and it could not have had one: the content-identity gate asks + *is the candidate installed?*, which the reminder path can never satisfy because it installs + nothing. The reminder path now carries its own marker + (`PackageManifest.NotifiedModuleVersion`), and `NotificationService.CreateNotification` accepts a + caller-supplied `identity` that derives a deterministic node id, so a repeat upserts the SAME row + instead of adding one. This was the bell's row count, and it was independent of where the rows + live — the **addressing** half of this emitter (§7 step 1: re-address to `Admin`) is still open. ## 6. The migration diff --git a/src/MeshWeaver.Documentation/Data/Architecture/PluginUpdateOnGreenBuild.md b/src/MeshWeaver.Documentation/Data/Architecture/PluginUpdateOnGreenBuild.md index 30d6b46b32..205eaa2789 100644 --- a/src/MeshWeaver.Documentation/Data/Architecture/PluginUpdateOnGreenBuild.md +++ b/src/MeshWeaver.Documentation/Data/Architecture/PluginUpdateOnGreenBuild.md @@ -227,6 +227,52 @@ present: prune, exactly as the static-repo importer skips it. Claiming is the deliberate act that decouples one node from its package; an unclaimed local edit is overwritten, by design. +## 🚨 A reminder is told ONCE — the gate the content-identity check cannot be (#3213) + +The content-identity gate above is the whole story for the **unattended** path and no part of it +for the **reminder** path, and conflating the two shipped a real defect: on memex.meshweaver.cloud, +**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. + +The gate asks *is the candidate installed?*: + +- On the `AutoUpdate` path that question is **self-silencing**. The apply advances the record's + `ModuleVersion`, so the next reconcile compares equal and says nothing. +- On the reminder path it is **unsatisfiable**. Nothing is installed — by design, the user has not + acted — so `ModuleVersion` never advances and the comparison evaluates 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. + +The notify path needs the *other* question — *have I already told them about this candidate?* — so +it carries its own answer, and the fix is two independent halves. **Either one alone still +duplicates:** + +1. **A satisfiable silence gate.** `PackageManifest.NotifiedModuleVersion` records the candidate + the user was last reminded about, on the durable install record. Writing it is what makes the + gate *become true* — which is exactly 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, which is the symptom itself. +2. **A deterministic notification identity.** `NotificationService.CreateNotification` takes an + optional `identity`; when given, the node id is derived from *(main node, identity)* with the + platform's content-addressing helper and the write becomes an atomic upsert. The reconciler + keys it on *(record, kind, candidate version)*, so a repeat that does reach the notify path — + two reconcile entry points overlapping, a marker write that failed — lands on the **same** node + instead of adding a row. + +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. It is only the *telling* that +is once per candidate. A refusal notification ("Update needs a Global Admin") is gated the same way +and by the same marker. + +Order matters within the notify path: the bell is written **first**, the 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 **new** candidate version changes both the marker comparison and the derived id, so it +gets its own unread bell — the suppression is per candidate, never per package. + ## Configuration ### 1. Register the webhook on the plugin repo @@ -264,7 +310,10 @@ install-time seed. common case and it is supposed to be quiet. - **A module changed** → an "Update available" notification on the install record, naming how many files changed and removed, and the short sha it was built from. Nothing installs — this is the - platform default. + platform default. **Exactly one** notification per candidate version, however many times the + reconcile runs (see *A reminder is told once* below). +- **The SAME module change, seen again** → nothing at all. No new row, no re-raised bell, not one + file fetched. - **A module changed on an opted-in record** → the delta install runs, touching only the changed files — claimed (non-`Include` `SyncBehavior`) nodes excepted. @@ -276,6 +325,7 @@ install-time seed. | A module never updates, and the log says it "has no module content identity" | The module's `manifest.lock` is missing or unparseable, so there is no `ModuleVersion` to compare and "has it changed" is unanswerable. A missing hash is the **absence of evidence**, not evidence of a change: treating it as changed would re-install the module on every green build of the repo *and* on every pod start, which is acting on the event rather than the content. It is refused, loudly, and the catalog card's manual **Update** stays available. Fix the module's CI to emit the sidecar. | | Nothing happens on a green build, **and the log says the delivery "matched NONE of the N sync config(s)"** | No `_GitSync` targets that repository — usually because the repository was **renamed** and the configs still store its old name. The matcher falls back to GitHub's canonical `full_name` (which follows the rename redirect) and repoints the config when it finds one, so this line surviving means the lookup could not be made either: the repository is unreachable with the config creator's credential, or the hook really is installed on a repository this mesh does not sync. The Warning names both sides — the incoming repository and everything it was compared against. | | Build node updates but no installation reacts | The package is not installed on that instance. A catalog lists far more packages than any instance installs; only packages with an install record are considered. | +| The same "Update available" reminder keeps reappearing, or the bell re-lights on one you dismissed | Before #3213 this was the norm — a new row per reconcile, forever. The reminder is now told once per candidate version: the install record carries `NotifiedModuleVersion`, and the notification's id is derived from *(record, kind, candidate)*. Seeing it again means the candidate genuinely moved (`ModuleVersion` differs from the one on the record's marker) — read the record and compare the two, rather than assuming a duplicate. | | The boot log says the feed of a registry could not be read after N attempts and was **recorded as PENDING**, and admins got a bell notification pointing at `Plugins/_RegistryReconcileLedger` | The registry stayed unavailable for longer than the boot's retry budget (#2888). Nothing is lost: the ledger entry for that registry reads `Pending: true`, and the reconcile runs on the next successful feed read — open the catalog page (or install anything from that registry) once the registry is back, then check the entry reads `LastReconciledVia: feed-read`. If the registry answers a *definite* refusal (401/403) instead, the entry is pending too, but no catalog open will drain it until the key or grant is fixed — the `LastFault` names which. | The webhook **never throws** on a write failure: GitHub retries a non-2xx delivery, so an unhandled diff --git a/src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-03-an-update-reminder-is-told-once-not-every-hour.md b/src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-03-an-update-reminder-is-told-once-not-every-hour.md new file mode 100644 index 0000000000..411bfd2206 --- /dev/null +++ b/src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-03-an-update-reminder-is-told-once-not-every-hour.md @@ -0,0 +1,32 @@ +--- +Name: An update reminder is told once, not every hour +Category: Fix +Description: A plugin with an update waiting added a fresh "Update available" notification to your bell every time your installation checked — several a day, per package, for as long as you did not act. You now get one reminder per version, it stays dismissed once you dismiss it, and a genuinely newer build still gets its own. +Icon: Alert +Order: -20260903 +--- + +# An update reminder is told once, not every hour + +When a plugin you have installed publishes a new build, your installation tells you so with an +**Update available** notification on the bell, and leaves the decision to you. That is the intended +behaviour: nothing installs itself unless you asked it to. + +What was not intended is that it told you again on every check. Your installation re-examines what +the plugin catalogue is serving whenever it starts, whenever it reads a catalogue, and whenever a +plugin repository publishes — and each of those checks added a *new* notification, because the +reminder had no memory of having been given. A package you had simply not got round to updating +produced several identical rows a day, for as long as you left it. On our own portal, **124 of the +newest 200 notifications** were this one message about two packages. + +The effect was worse than clutter. Those rows are what the bell has to read every time you open it, +so a reminder you had already seen made the bell slower for everyone who could see it — and a +notification you had dismissed came straight back. + +**A reminder is now a statement about a version, not an event.** The first time a new build of a +package is available, you get one notification. Every later check that still sees the same build +says nothing at all: no new row, no re-lit bell, and nothing fetched. If you dismiss it, it stays +dismissed. + +When the plugin publishes a genuinely newer build, that is a different thing to be told about, and +you get a fresh reminder for it — which is the whole point of the message. diff --git a/src/MeshWeaver.Graph/NotificationService.cs b/src/MeshWeaver.Graph/NotificationService.cs index 7063ba6c2d..036286770c 100644 --- a/src/MeshWeaver.Graph/NotificationService.cs +++ b/src/MeshWeaver.Graph/NotificationService.cs @@ -31,7 +31,30 @@ public static class NotificationService /// Returns an IObservable that emits the created node and completes — /// subscribe to drive the write. Safe to compose inside hub handlers / /// click actions via Subscribe. + /// + /// An EVENT gets a fresh id; a STATE gets an . Left + /// null, every call mints a new GUID, which is right for a notification that reports something + /// that happened once ("your response is ready"). It is WRONG for one that reports a standing + /// condition an emitter re-evaluates on a schedule — "package X has version Y available" — and + /// that difference is the whole of Systemorph/MeshWeaver#3213: a reminder with no identity has + /// nothing for a repeat to collide with, so the reconciler minted a new bell row per poll, + /// forever (124 of the newest 200 rows on memex-cloud). /// + /// + /// A caller-supplied identity for the CONDITION this notification reports (e.g. + /// "package-update|update-available|{moduleVersion}"). When given, the node id is + /// derived deterministically from it plus , and the write is an + /// atomic upsert — so a repeat for the same condition lands on the SAME node and refreshes it + /// instead of adding a row. Change what the condition IS (a newer version) and the id changes + /// with it, so the new state gets its own unread bell. Null = the historic + /// fresh-GUID-per-call behaviour. + /// 🚨 The identity must capture everything that makes two reminders genuinely different. + /// Too coarse and a real change is swallowed into an existing row; the emitter, not this + /// method, owns that judgement. A repeat also refreshes + /// and clears — correct for "this is still true", which is + /// why the emitter should ALSO carry a marker that stops re-raising an unchanged condition + /// rather than relying on the upsert alone. + /// public static IObservable CreateNotification( IMeshService nodeFactory, string mainNodePath, @@ -40,9 +63,18 @@ public static IObservable CreateNotification( NotificationType type, string? targetNodePath = null, string? createdBy = null, - string? icon = null) + string? icon = null, + string? identity = null) { - var notificationId = Guid.NewGuid().AsString(); + var deterministic = !string.IsNullOrEmpty(identity); + // Reuses the platform's ONE content-addressing helper rather than growing a second hash: + // it is exactly the (path, token) → stable-id shape the content-addressed import marker + // (`{Partition}/_Activity/import-{fingerprint}`) already uses, and its output is 16 lower + // hex chars — always a legal node-id segment, whatever characters the caller's identity + // string happens to contain. + var notificationId = deterministic + ? PartitionSourceFingerprint.Compute([(mainNodePath, identity!)]) + : Guid.NewGuid().AsString(); var parentPath = $"{mainNodePath}/{SatelliteSegment}"; var notification = new Notification @@ -67,7 +99,14 @@ public static IObservable CreateNotification( Content = notification }; - return nodeFactory.CreateNode(node); + // 🚨 The upsert is the OWNER's single verb, not a client-side + // CreateNode().Catch(exists → UpdateNode()): two reconcile passes racing on the same + // deterministic path are exactly what this id makes possible, and the hand-rolled split + // races (the create's exists-check lags the concurrent create → the update patches a + // not-yet-materialised node → "NotFound … for patch apply"). + return deterministic + ? nodeFactory.CreateOrUpdateNode(node) + : nodeFactory.CreateNode(node); } private static readonly TimeSpan LookupTimeout = TimeSpan.FromSeconds(10); diff --git a/src/MeshWeaver.PluginCatalog/Package.cs b/src/MeshWeaver.PluginCatalog/Package.cs index c3d9f66473..2c31577371 100644 --- a/src/MeshWeaver.PluginCatalog/Package.cs +++ b/src/MeshWeaver.PluginCatalog/Package.cs @@ -318,6 +318,34 @@ public record PackageManifest /// public bool AutoUpdate { get; init; } + /// + /// The candidate this installation has ALREADY told the user + /// about — the reminder path's own silence gate, and the second half of + /// Systemorph/MeshWeaver#3213. + /// + /// Why alone cannot do this job. The content-identity + /// gate in PackageUpdateReconciler.Decide asks is the candidate INSTALLED?. + /// On the path that is self-silencing: the apply advances + /// , so the next poll compares equal. On the reminder path nothing + /// is installed — by design, the user has not acted — so the same comparison is + /// unsatisfiable and evaluated false on every subsequent poll, forever. The notify path + /// needs the other question, have I already told them about this candidate?, and this + /// is where its answer lives. + /// + /// It is deliberately a fact on the DURABLE record rather than an in-memory de-dup + /// cache: a cache loses its memory on every pod start and re-notifies, which is the symptom + /// itself, and process-wide static state is forbidden here anyway. + /// + /// Written by the reconciler AFTER the reminder is raised (so a failed write costs a + /// harmless repeat, which the notification's deterministic identity then absorbs, rather than + /// a lost reminder). Null on every record that has never been reminded, which is every record + /// written before this field existed — so the first poll after an upgrade reminds once and + /// then goes quiet. A record whose catches up (the user clicked + /// Update) is silenced by the content-identity gate before this one is ever consulted, so a + /// re-stamp that drops this field cannot resurrect the storm. + /// + public string? NotifiedModuleVersion { get; init; } + /// /// The module manifest's per-file hash map as it stands AT THE CATALOG'S REF /// () — the candidate side of the diff, where diff --git a/src/MeshWeaver.PluginCatalog/PackageUpdateReconciler.cs b/src/MeshWeaver.PluginCatalog/PackageUpdateReconciler.cs index a5aaefde0a..759726dffe 100644 --- a/src/MeshWeaver.PluginCatalog/PackageUpdateReconciler.cs +++ b/src/MeshWeaver.PluginCatalog/PackageUpdateReconciler.cs @@ -172,7 +172,7 @@ private static IObservable Decide( ? Apply(hub, meshService, accessService, source, sourceRef, pkg, record, recordPath, provenance, detail, logger) : Notify( - meshService, accessService, recordPath, pkg, + hub, meshService, accessService, recordPath, pkg, record, UpdateAvailableKind, $"Update available: {pkg.Name ?? pkg.Id}", $"A new build of {pkg.Name ?? pkg.Id} is available ({detail}). {provenance}.", logger); @@ -242,7 +242,8 @@ private static IObservable Apply( logger?.LogWarning( "Package update: auto-update of {Id} REFUSED — {Reason}", pkg.Id, ex.Message); return Notify( - meshService, accessService, recordPath, pkg, + hub, meshService, accessService, recordPath, pkg, record, + AuthorizationRequiredKind, $"Update needs a Global Admin: {pkg.Name ?? pkg.Id}", $"A new build of {pkg.Name ?? pkg.Id} is available ({detail}), but it is a " + "commercial package and was not applied automatically. " + ex.Message, @@ -256,21 +257,82 @@ private static IObservable Apply( }); } - /// Raises a system notification on the install record — the user-visible surface of an - /// update reminder, and of a refusal, which must never be a silent skip. + /// The reminder that a changed module is waiting for the user to act. + private const string UpdateAvailableKind = "update-available"; + + /// The refusal that an unattended apply needs a global admin to re-authorize it. + private const string AuthorizationRequiredKind = "authorization-required"; + + /// + /// Raises a system notification on the install record — the user-visible surface of an update + /// reminder, and of a refusal, which must never be a silent skip. + /// + /// 🚨 A reminder is a STATE, not an event, and this method is what makes it behave + /// like one (Systemorph/MeshWeaver#3213). The reconciler re-decides on every poll, so + /// without the two guards below one package with a pending update mints a new bell row per + /// poll, forever — measured on memex-cloud as 124 of the newest 200 notifications, two + /// packages accounting for over half of everything the mesh had written in four days. Both + /// guards are needed, and each one alone still duplicates: + /// + /// A satisfiable silence gate. The content-identity gate in + /// asks "is the candidate installed?" — self-silencing on the + /// AutoUpdate path (the apply advances the record) and UNSATISFIABLE here, because + /// the reminder installs nothing by design. So the notify path carries its own marker, + /// , which answers the question it + /// actually needs: "have I already told them about THIS candidate?". Writing it is what + /// makes the gate become true — which is precisely what the version comparison could + /// never do here. + /// A deterministic identity to be idempotent on. Marker or no marker, the + /// reconcile runs from several entry points (boot, a feed read draining a deferral, a + /// build webhook) that can overlap, and the marker write can fail. Keying the notification + /// on (record, kind, candidate version) makes any such repeat land on the SAME node as an + /// idempotent upsert instead of a new row, so the worst case is a refreshed bell rather + /// than an unbounded one. + /// + /// + /// The gate sits HERE and not before : an unattended apply must keep + /// retrying every poll so a restored admin grant heals itself. It is only the TELLING that is + /// once per candidate. + /// private static IObservable Notify( + IMessageHub hub, IMeshService meshService, AccessService accessService, string recordPath, PackageManifest pkg, + PackageManifest record, + string kind, string title, string body, ILogger? logger) - => Observable.Using( - accessService.ImpersonateAsSystem, - _ => NotificationService.CreateNotification( + { + // Decide() has already refused an empty candidate hash, so this is a real content identity. + var candidate = pkg.ModuleVersion!; + + // ── Guard 1: already told them about this exact candidate ⇒ stay silent, write nothing ── + if (string.Equals(record.NotifiedModuleVersion, candidate, StringComparison.Ordinal)) + return Observable.Return(Unit.Default); + + // RunAsSystem, never Observable.Using (#1790): the install-records partition is + // System-owned, and Rx would otherwise leave the subscribing thread latched as System. + return accessService + .RunAsSystem(() => NotificationService + // ── Guard 2: one node per (record, kind, candidate) ────────────────────────── + .CreateNotification( meshService, recordPath, title, body, - NotificationType.System, targetNodePath: recordPath)) + NotificationType.System, targetNodePath: recordPath, + identity: $"package-update|{kind}|{candidate}") + // Mark AFTER the bell, never before: a marker written first and a notification + // that then failed would silence the reminder permanently, whereas a bell written + // first and a marker that then failed costs one repeat — which guard 2 absorbs + // into the same node. + // + // The TYPED overload, and the lambda reads the CURRENT record rather than the + // snapshot this pass started from: the write is an RFC 7396 merge patch the owning + // hub applies to its own state, so only `notifiedModuleVersion` travels and a + // concurrent install's fields are not clobbered. + .SelectMany(_ => hub.GetMeshNodeStream(recordPath) + .Update(current => current with { NotifiedModuleVersion = candidate }))) .Select(_ => Unit.Default) .Catch((Exception ex) => { @@ -278,4 +340,5 @@ private static IObservable Notify( "Package update: raising the notification for {Id} failed.", pkg.Id); return Observable.Return(Unit.Default); }); + } } diff --git a/test/ImpersonationScopeSites.allow b/test/ImpersonationScopeSites.allow index 1533025529..a0b19d38b8 100644 --- a/test/ImpersonationScopeSites.allow +++ b/test/ImpersonationScopeSites.allow @@ -69,7 +69,7 @@ src/MeshWeaver.PluginCatalog/InstanceAutoRegistrationService.cs 6 src/MeshWeaver.PluginCatalog/InstanceComboReader.cs 1 src/MeshWeaver.PluginCatalog/ModuleDiscoveryService.cs 4 src/MeshWeaver.PluginCatalog/PackageInstaller.cs 10 -src/MeshWeaver.PluginCatalog/PackageUpdateReconciler.cs 2 +src/MeshWeaver.PluginCatalog/PackageUpdateReconciler.cs 1 src/MeshWeaver.PluginCatalog/PluginUpdateWatcher.cs 1 src/MeshWeaver.PluginCatalog/RegistrationKeyService.cs 1 tools/MeshWeaver.PluginTester/PluginGateRunner.cs 1 diff --git a/test/Memex.Portal.Shared.Test/PackageUpdateReminderIdempotenceTest.cs b/test/Memex.Portal.Shared.Test/PackageUpdateReminderIdempotenceTest.cs new file mode 100644 index 0000000000..20638a7da6 --- /dev/null +++ b/test/Memex.Portal.Shared.Test/PackageUpdateReminderIdempotenceTest.cs @@ -0,0 +1,283 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Net; +using System.Net.Http; +using System.Reactive.Linq; +using System.Text; +using System.Threading; +using System.Threading.Tasks; +using MeshWeaver.Data; +using MeshWeaver.Fixture; +using MeshWeaver.Graph; +using MeshWeaver.Hosting.Monolith.TestBase; +using MeshWeaver.Mesh; +using MeshWeaver.Mesh.Security; +using MeshWeaver.Messaging; +using MeshWeaver.PluginCatalog; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Xunit; + +namespace Memex.Portal.Shared.Test; + +/// +/// 🚨 An "Update available" reminder is a STATE, not an event — a second reconcile pass over the +/// same pending update must leave ONE notification node, not two +/// (Systemorph/MeshWeaver#3213). +/// +/// Measured on memex.meshweaver.cloud on 2026-09-03: 124 of the newest 200 notification +/// rows were this one emitter, two packages producing four rows each per day and the same shape all +/// the way back through the window. re-decides on every poll, +/// and nothing about a reminder was idempotent. +/// +/// Two independent halves, and fixing either alone still duplicates — so this fixture +/// exercises both, each with its own arm that fails if that half is missing: +/// +/// A silence gate that can actually become true. The content-identity gate asks "is +/// the candidate installed?", which is self-silencing on the AutoUpdate path and +/// UNSATISFIABLE on the reminder path — nothing is installed there, by design, so it evaluates +/// false on every subsequent poll forever. Arm 2 below runs an IDENTICAL second pass; without +/// it writes a second row. +/// A deterministic identity to be idempotent on. Every notification used to mint a +/// fresh GUID, so a repeat had nothing to collide with. Arm 3 puts the marker back to a stale +/// value — the shape a failed marker write, or a second reconcile entry point racing the first, +/// actually leaves behind — and re-runs: the notify path executes end to end (proved by the +/// marker being rewritten) and still lands on the SAME node. +/// +/// +/// Arm 4 is the falsifier the suppression needs: a genuinely NEW candidate version must still +/// raise its own unread bell. A gate that silenced that would pass arms 2 and 3 while breaking the +/// feature, and it is also what bounds the negative assertions — once the listing shows the v3 +/// reminder, anything an earlier pass wrote is already committed and would be counted. +/// +public class PackageUpdateReminderIdempotenceTest(ITestOutputHelper output) : MonolithMeshTestBase(output) +{ + private const string PackageId = "UpdateReminderIdempotencePkg"; + private const string PackageName = "Update Reminder Idempotence Package"; + private const string InstalledModuleVersion = "mv-installed-3213"; + private const string CandidateModuleVersion = "mv-candidate-3213"; + private const string NextCandidateModuleVersion = "mv-candidate-3213-next"; + + /// A marker value that is neither candidate — what a marker write that never landed, + /// or one left by some other candidate, looks like to the gate. + private const string StaleMarker = "mv-marker-from-some-other-pass"; + + private const string RegistryUrl = "http://registry.reminder-idempotence.test"; + private const string Token = "mwi_reminder_idempotence_test"; + + private static string RecordPath => $"{PackageInstaller.InstalledPartition}/{PackageId}"; + + /// The negative control: the reminder path must not fetch a single file. Every request + /// this handler ever sees is a reconcile that took the APPLY branch it had no business taking. + private readonly CountingRegistry registry = new(); + + private System.Text.Json.JsonSerializerOptions Json => Mesh.JsonSerializerOptions; + + protected override MeshBuilder ConfigureMesh(MeshBuilder builder) + => base.ConfigureMesh(builder) + .AddPluginCatalog() + .ConfigureServices(s => s.AddSingleton(new CountingClientFactory(registry))); + + [Fact] + public async Task ASecondReconcileOverTheSamePendingUpdate_LeavesExactlyOneNotification() + { + await SeedInstallRecord(); + + var source = new RegistryPackageSource(Mesh, RegistryUrl, Token); + var logger = Mesh.ServiceProvider.GetRequiredService() + .CreateLogger(nameof(PackageUpdateReminderIdempotenceTest)); + + // ── 1. The first pass tells the user, once ────────────────────────────────────────────── + await Reconcile(source, CandidateModuleVersion, "Served by registry 'first pass'", logger); + + var first = await Reminders() + .Where(ns => ns.Count == 1) + .FirstAsync().Timeout(TestTimeouts.Convergence).Await(); + var reminderPath = first[0].Path; + var reminder = first[0].ContentAs(Json); + Assert.NotNull(reminder); + Assert.StartsWith("Update available", reminder!.Title, StringComparison.Ordinal); + Assert.Contains(PackageName, reminder.Title); + Assert.False(reminder.IsRead); + + // The gate the reminder path now owns: "have I already told them about THIS candidate?". + // Writing it is what makes the gate become TRUE — which is precisely what comparing the + // record's installed version against the candidate could never do here. + await AwaitMarker(CandidateModuleVersion); + + // ── 2. HALF ONE — an identical second pass says nothing at all ────────────────────────── + // 🚨 Counting rows CANNOT test this half, and asserting only the count is how a fixture + // passes having checked nothing: with the deterministic identity in place, a second pass + // that is NOT suppressed still lands on the SAME node, so the count stays 1 either way. + // What separates "suppressed" from "absorbed" is what the user sees — so dismiss the bell + // first. A pass that re-raises an unchanged reminder un-reads it, which is the defect a + // reader of the bell actually experiences four times a day. + await MarkRead(reminderPath); + await AwaitRead(reminderPath, true); + + // This is the pass that produced a new row per poll, forever. Its own completion is the + // bound: the notification write, if there were one, sits inside the awaited chain. + await Reconcile(source, CandidateModuleVersion, "Served by registry 'second pass'", logger); + + var afterSecond = await Reminders().FirstAsync().Timeout(TestTimeouts.Convergence).Await(); + Assert.Single(afterSecond); + Assert.Equal(reminderPath, afterSecond[0].Path); + var stillDismissed = afterSecond[0].ContentAs(Json); + Assert.NotNull(stillDismissed); + Assert.True(stillDismissed!.IsRead, + "the second pass re-raised a reminder the user had already dismissed — the reminder " + + "path's silence gate did not suppress it"); + Assert.Equal(reminder.CreatedAt, stillDismissed.CreatedAt); + + // ── 3. HALF TWO — a pass that DOES reach the notify path still lands on the same node ─── + // The marker is put back to a value the gate cannot match, so guard one steps aside and the + // notify path runs for real. Only the deterministic identity keeps this from adding a row. + await SetMarker(StaleMarker); + await AwaitMarker(StaleMarker); + + await Reconcile(source, CandidateModuleVersion, "Served by registry 'third pass'", logger); + + // Positive signal that this pass executed the notify path end to end — not that it was + // skipped, which would make the assertion below vacuous. + await AwaitMarker(CandidateModuleVersion); + + var afterThird = await Reminders().FirstAsync().Timeout(TestTimeouts.Convergence).Await(); + Assert.Single(afterThird); + Assert.Equal(reminderPath, afterThird[0].Path); + + // The upsert REFRESHES the row, so the reminder reads as new again. That is the honest + // answer to "this condition is still true and I have no record of having told you", and it + // is precisely why guard one has to exist: on its own, guard two would do this every poll. + var refreshed = afterThird[0].ContentAs(Json); + Assert.NotNull(refreshed); + Assert.False(refreshed!.IsRead); + + // ── 4. A genuinely NEW candidate is still worth telling them about ────────────────────── + // Suppression keyed too coarsely would swallow this, and every arm above would still pass. + await Reconcile(source, NextCandidateModuleVersion, "Served by registry 'new build'", logger); + + var afterNew = await Reminders() + .Where(ns => ns.Any(n => n.Path != reminderPath)) + .FirstAsync().Timeout(TestTimeouts.Convergence).Await(); + Assert.Equal(2, afterNew.Count); + var newReminder = afterNew.Single(n => n.Path != reminderPath).ContentAs(Json); + Assert.NotNull(newReminder); + Assert.StartsWith("Update available", newReminder!.Title, StringComparison.Ordinal); + Assert.False(newReminder.IsRead); + await AwaitMarker(NextCandidateModuleVersion); + + // The listing now contains the newest write, so every earlier one is committed too: two + // candidates, two reminders — not one row per pass. + Assert.Equal(2, (await Reminders().FirstAsync().Timeout(TestTimeouts.Convergence).Await()).Count); + + // …and the reminder path fetched nothing. Four reconciles, zero registry round-trips. + Assert.Equal(0, registry.Attempts); + } + + /// One reconcile pass over one candidate — the exact call every entry point makes + /// (the boot reconcile, a feed read draining a deferral, a build webhook), awaited to + /// completion so the pass's own writes are done before the assertion reads. + private async Task Reconcile( + IPackageSource source, string candidate, string provenance, ILogger logger) + => await PackageUpdateReconciler + .ReconcileInstalled(Mesh, source, "HEAD", [Candidate(candidate)], provenance, logger) + .Timeout(TestTimeouts.Convergence).Await(); + + private static PackageManifest Candidate(string moduleVersion) => new() + { + Id = PackageId, + Name = PackageName, + Version = "1.0.1", + ModuleVersion = moduleVersion, + TargetPartition = PackageId, + }; + + private async Task SeedInstallRecord() + { + var manifest = new PackageManifest + { + Id = PackageId, + Name = PackageName, + Version = "1.0.0", + ModuleVersion = InstalledModuleVersion, + TargetPartition = PackageId, + // The reminder path — the one whose gate could never become true. + AutoUpdate = false, + }; + var record = MeshNode.FromPath(RecordPath) with + { + NodeType = PackageInstaller.PackageNodeType, + Name = manifest.Name, + State = MeshNodeState.Active, + Content = manifest, + }; + var access = Mesh.ServiceProvider.GetRequiredService(); + await access.RunAsSystem(() => NodeFactory.CreateOrUpdateNode(record)) + .Timeout(TestTimeouts.Convergence).Await(); + } + + private async Task SetMarker(string value) + { + var access = Mesh.ServiceProvider.GetRequiredService(); + await access + .RunAsSystem(() => Mesh.GetMeshNodeStream(RecordPath) + .Update(current => current with { NotifiedModuleVersion = value })) + .Timeout(TestTimeouts.Convergence).Await(); + } + + /// The user dismissing the bell — the same field the bell list flips on click. + private async Task MarkRead(string notificationPath) + { + var access = Mesh.ServiceProvider.GetRequiredService(); + await access + .RunAsSystem(() => Mesh.GetMeshNodeStream(notificationPath) + .Update(current => current with { IsRead = true })) + .Timeout(TestTimeouts.Convergence).Await(); + } + + private async Task AwaitRead(string notificationPath, bool expected) => + await Mesh.GetMeshNodeStream(notificationPath) + .Select(n => n.ContentAs(Json)?.IsRead) + .Where(v => v == expected) + .FirstAsync().Timeout(TestTimeouts.Convergence).Await(); + + private async Task AwaitMarker(string expected) => + await Mesh.GetMeshNodeStream(RecordPath) + .Select(n => n.ContentAs(Json)?.NotifiedModuleVersion) + .Where(v => string.Equals(v, expected, StringComparison.Ordinal)) + .FirstAsync().Timeout(TestTimeouts.Convergence).Await(); + + /// The reminder satellites of the install record, read LIVE through a children query + /// (a query never storms on a parent that does not exist yet, and it re-emits on every write). + private IObservable> Reminders() => + Mesh.GetWorkspace() + .GetQuery("notif|pkg|reminder-idempotence", + $"path:{RecordPath}/{NotificationService.SatelliteSegment} scope:children nodeType:Notification") + .Select(ns => (IReadOnlyList)(ns ?? []).ToList()); + + /// Answers nothing and counts every attempt — the reminder path must never reach it. + private sealed class CountingRegistry : HttpMessageHandler + { + private int attempts; + + public int Attempts => Volatile.Read(ref attempts); + + // HttpMessageHandler's contract is Task-shaped; nothing here bridges an observable. + protected override Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) + { + Interlocked.Increment(ref attempts); + return Task.FromResult(new HttpResponseMessage(HttpStatusCode.ServiceUnavailable) + { + Content = new StringContent( + "{\"error\":\"the reminder path must not fetch\"}", Encoding.UTF8, "application/json"), + }); + } + } + + private sealed class CountingClientFactory(HttpMessageHandler handler) : IHttpClientFactory + { + public HttpClient CreateClient(string name) => new(handler, disposeHandler: false); + } +} diff --git a/test/MeshWeaver.Documentation.Test/ImpersonationScopeSiteRatchetGuard.cs b/test/MeshWeaver.Documentation.Test/ImpersonationScopeSiteRatchetGuard.cs index 4a3dc06566..1ffbfe0c60 100644 --- a/test/MeshWeaver.Documentation.Test/ImpersonationScopeSiteRatchetGuard.cs +++ b/test/MeshWeaver.Documentation.Test/ImpersonationScopeSiteRatchetGuard.cs @@ -64,7 +64,7 @@ public class ImpersonationScopeSiteRatchetGuard(ITestOutputHelper output) /// the shape; this stops the list as a WHOLE from growing — including by the trick of adding a /// new file's line. Lower it whenever you delete or lower an entry. /// - private const int TotalBudget = 69; + private const int TotalBudget = 68; /// Production roots. test/ and samples/ are deliberately out of scope — /// the leak there costs test isolation, not a user's permissions, and listing 21 more entries