PackageUpdateReconciler re-raises "Update available" on every poll — 124 of the newest 200 notifications, and the bulk of the bell's row count #3213

Activity

1. rbuergi

   added a commit that references this issue on Sep 3, 2026

   docs: name the two filed defects (MeshWeaver.Plugins#1275, #3213) at …

   ...

   8b34d42
2. rbuergi commented on Sep 3, 2026

   

   rbuergi

   on Sep 3, 2026

   ContributorAuthor

   More actions

   Root cause — TWO independent halves, and either one alone would still duplicate

   Verified from source. Two agent attempts at this stalled on infrastructure before reporting, so recording the mechanism directly.

   Half 1 — the silence gate asks a question that cannot become true

   PackageUpdateReconciler.cs:161-163:

   // 🚨 THE gate: content identity, not the event that woke us. Equal module hashes ⇒ nothing in
   // this module changed ⇒ stay completely silent — no notification, and not one file fetched.
   if (string.Equals(record.ModuleVersion, pkg.ModuleVersion, StringComparison.Ordinal))
       return Observable.Return(Unit.Default);

   That gate is correct for the AutoUpdate == true path: Apply(...) installs, which advances record.ModuleVersion, so the next poll is silent. It is self-silencing.

   It is unsatisfiable for the AutoUpdate == false path (:174-178). Notify(...) installs nothing, so record.ModuleVersion never advances — by design, because the user has not acted. The gate therefore evaluates false on every subsequent poll and re-notifies forever.

   The gate conflates two different questions:

   • is the candidate installed? — what it actually asks
   • have I already told them about this candidate? — what the notify path needs

   Half 2 — the notification has no identity to be idempotent on

   NotificationService.cs:45:

   var notificationId = Guid.NewGuid().AsString();

   Every call mints a fresh GUID, so each repeat lands at a new node path. There is nothing for a second call to collide with, which is why the duplicates accumulate as separate rows rather than overwriting.

   So even if Half 1 were fixed loosely (say, a poll-interval suppression), any path that re-entered would still create a new node. And conversely, fixing only Half 2 would silence the bell while the reconciler kept re-deciding.

   Two fix shapes

   (a) Reconciler-side — narrow, and correct for this issue. Give the notify path its own gate keyed on the candidate: record the last-notified module version on the install record, and stay silent when it equals pkg.ModuleVersion. Then the notification is idempotent by construction — keyed on (package, candidate identity) — rather than suppressed.

   (b) NotificationService-side — general. Accept an optional caller-supplied deterministic id so a repeat lands on the same node. This repo already has that pattern: content-addressed import markers ({Partition}/_Activity/import-{fingerprint}), where the marker's identity is the thing it records, so a re-run cannot duplicate.

   Recommendation: (a) for #3213, and consider (b) separately. (a) is contained to one file and fixes the reported defect. (b) is a change to a shared service used by at least StartupErrorNotifier, RegistryUpdateReconciler, ModuleDiscoveryService and ContentIndexingActivity — worth having, but its blast radius deserves its own issue and its own measurement of which callers actually re-raise.

   🚨 Do not fix this with a de-dup cache. A static collection is forbidden here (process-wide state bleeds across users and partitions in prod), and an instance cache on a mesh-scoped singleton would still lose its memory on restart and re-notify — which is exactly the symptom. The marker belongs on the node, written through GetMeshNodeStream(path).Update(...).

   Whichever shape is taken, prove it by mutation

   Restore the defect and watch the test redden, with a clean fixture as the control. This fleet produced eight cases today of a check that could not fire — two of them in code written the same day, including a positive test whose fixture could not collide.
3. rbuergi

   added 2 commits that reference this issue on Sep 3, 2026

   test(#3213): bridge every observable wait through .Await(), not Rx's …

   ...

   117a43b

   Merge main: the reminder is now ADDRESSED and IDEMPOTENT, not one or …

   ...

   78f29cc
4. rbuergi commented on Sep 4, 2026

   

   rbuergi

   on Sep 4, 2026

   ContributorAuthor

   More actions

   Fixed on main by #3230 — verified against both halves of the root cause

   Re-read the merged source rather than trusting the PR title. src/MeshWeaver.PluginCatalog/PackageUpdateReconciler.cs on main now closes both halves this issue's root-cause comment named, and the code says so itself in its own doc comment ("two guards are needed, and each one alone still duplicates"):

   Half 1 — a satisfiable silence gate. Notify no longer relies on the content-identity gate that could never become true on the AutoUpdate == false path. It carries its own marker:

   // ── 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);

   and writes NotifiedModuleVersion through hub.GetMeshNodeStream(recordPath).Update<PackageManifest>(...) — the house mutation API, a merge patch, so a concurrent install's fields are not clobbered. Writing it is what makes the gate become true, which is exactly what the version comparison could not do.

   Half 2 — a deterministic identity. The Guid.NewGuid() per call is gone; NotificationService.CreateNotification now takes identity: $"package-update|{kind}|{candidate}", so a repeat lands on the same node as an idempotent upsert instead of minting a new row.

   The ordering is deliberate and documented at the call site: the bell is written before the marker, so a marker write that fails costs one repeat (absorbed by guard 2) rather than silencing the reminder permanently.

   The recommended shape in the root-cause comment was "(a) reconciler-side, and consider (b) separately" — what landed is (a) and (b), with (b) delivered as an optional caller-supplied identity on the shared service exactly as sketched.

   Acceptance test on main: Memex.Portal.Shared.Test.PackageUpdateReminderIdempotenceTest.ASecondReconcileOverTheSamePendingUpdate_LeavesExactlyOneNotification.

   Closing. The bell's row count tail (the ~4 476 rows already written) is #3250, and the addressing half is #3156/#3216 — both tracked separately.
5. rbuergi

   closed this as completedon Sep 4, 2026
6. rbuergi

   added a commit that references this issue on Sep 4, 2026

   test(#3213): the reminder fixture reads the bell it is now DELIVERED to

   ...

   ec3185e