Skip to content

fix: a notification is ADDRESSED — delivered to the person it is for, not filed beside what it is about (#3156, #3216, #3238) - #3252

Merged
meshweaver-cloud[bot] merged 5 commits into
mainfrom
fix/3156-3216-addressed-notifications
Sep 4, 2026
Merged

meshweaver-cloud[bot] merged 5 commits into
mainfrom
fix/3156-3216-addressed-notifications

Conversation

@rbuergi

@rbuergi rbuergi commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Closes #3156
Closes #3216
Closes #3238

The defect, in one sentence

A Notification was a satellite of the entity it was about — {entity}/_Notification/{id}, in
THAT entity's partition — so the bell had no first segment to name, and a query with no first
segment cannot reach the Admin schema at all.

That single fact caused three problems that could only be fixed together, which is why this is one
change set:

#3238 / #3156 the bell UNIONed every partition schema on the server, per circuit, on every notification write anywhere
#3216 Admin is deliberately excluded from public.searchable_schemas, so that fan-out could never read admin.notifications — every platform-admin notification was written, versioned, and shown to nobody
the trap anchoring the bell before moving the write would have silently dropped every notification about anything outside the viewer's partition — the fail-open truncation of #2011

Measured, twice, on memex-cloud

When What Result
2026-09-02, 8 pods [CrossSchema] SLOW unions / 5 min 444, avg 4.0 s, Postgres 94–98 % CPU
2026-09-03 16:47–16:58, one idle replica (0.13 cores, 2 GB) one bell render 4 476 rows across 201 of 201 schemas, 9–10 s, filtered to 0 rows in memory; ~60 lines/min × 7 replicas; memexaks-pg 87–91 % avg CPU

The two measurements agree on mechanism and differ only in magnitude, so #3238 is the current
baseline and this PR closes it. What the newer one adds is what it rules OUT: it was taken on an
idle pod, so it is not a GC artefact, and time-to-first-row ≈ total, so the cost is the scan rather
than the caller's consumption. No disagreement to report.

The fix — the cure that already existed in this repo

PermissionEvaluator hit the identical searchable_schemas blindness for platform-admin GRANTS and
fixed it by path: anchoring, which dissolved the Admin special case rather than adding one.
Same move here.

Write side: a Notification has exactly ONE addressee and is delivered into that addressee's
partition — {addressee}/_Notification/{id}, MainNode = {addressee} — where the addressee is a
user partition or Admin. The entity stays a reference in TargetNodePath, which the bell, the
panel and the React client already group by, so grouping, click target and badge count are
unchanged.

Read side: the bell is NotificationService.BellQuery(addressee), and the Plugins half issues
it once per bell the viewer is entitled to.

🚨 One correction to the committed design, and it matters

Doc/Architecture/AddressedNotifications §3 proposed a single alternation,
namespace:{viewer}/_Notification|Admin/_Notification, measured "Anchored" by
QueryRouteClassifier. That would not have fixed #3216. Classification is not routing:

  • a single concrete namespace: folds into ParsedQuery.Path, so ResolvePinnedPartition pins
    the read to one schema and the fan-out machinery — and therefore searchable_schemas — is never
    consulted;
  • an alternation leaves Path null, takes the fan-out route, and is narrowed by intersection
    with searchable_schemas — deliberately, and the code says why: "it bypasses searchable_schemas,
    whose ExcludedSchemas (auth, admin, …) would then become newly visible to a namespace-anchored
    query."

Admin is one of those excluded schemas, so the alternation would have dropped the platform bell
again, silently, while #3216 read as fixed. Two separately pinned reads is the only shape that
reaches both bells — exactly why PermissionEvaluator combines a partition leg with a root leg
instead of asking for both at once. Pinned by NotificationBellLegsTest, which asserts
ResolvePinnedPartition → rbuergi / admin for the legs and null for the alternation. The doc
page is corrected in this PR.

🚨 The asymmetric risk, and the test that is the point of the change set

Today's bug is "admins see nothing". The careless fix is "everyone sees admin notifications" —
platform notifications carry startup errors, failed reconciles and stuck-instance detail, so that is
the worse outcome. Two independent boundaries:

  1. RLS. Admin/_Notification/{id} resolves through the ordinary path-based fold; a non-admin
    has Permission.None on the Admin partition. NotificationAddressingTest.PlatformNotification_IsNotReadableByANonAdmin
    asserts it as a verdict (GetEffectivePermissions(...) has no Read) and through the bell
    query itself — with the admin's successful read of the SAME row first, so the non-admin's empty
    answer cannot be an artefact of the write not having landed.
  2. The gate. The platform leg is issued only for a viewer hub.IsGlobalAdmin() confirms
    POSITIVELY — the one canonical predicate, never an ad-hoc role-name or root-scope check — seeded
    false, .Catch(... => false), so it fails CLOSED.

🚨 The negative was proven able to fail. Granting the non-admin Viewer on Admin was staged
deliberately and the test went red: "Expected the observable to emit a value matching the predicate
… leaking it to every user is a WORSE outcome than the admin seeing nothing"
. Then reverted.

The design decision, stated: where a platform-collective notification lives

Admin/_Notification — one row, addressed to the operators collectively. Not fanned out per
admin:

Admin, one row per-admin fan-out
Write cost one row per event one row per event per admin
Newly promoted admin sees the history sees nothing before promotion
Demoted admin loses it immediately keeps every copy already written into their own partition — a standing disclosure with no revocation path
Enumeration none the admin set resolved at WRITE time, on the boot-error path

The cost, stated: read is shared — one operator marking a platform notice read marks it read
for all. Right for a shared operations inbox, wrong for personal mail, which is why a personal
notification is never addressed to Admin. It reintroduces no cross-schema union (one pinned
schema) and drops nothing (§6 below).

The migration ruling: neither move nor delete — they age out

Asked for in #3156 and answered in the doc. Moving ~10⁵ rows across 201 schemas would run as a
startup Job whose failure stops every portal from serving (DbVersionGate) — to buy back a class
that is 97 % operator noise which ruling 1 deliberately removes from users' bells, plus a 6 %
thread-completion tail that is the most perishable notification there is. Deleting is cheaper but
irreversible and buys nothing the anchored bell can see anyway. Leaving them costs dead rows a
retention pass reclaims later without a schema migration — filed as #3250.

🚨 This is not the #2011 truncation. Going forward nothing is dropped: a notification about an
entity anywhere in the mesh is delivered into the reader's own partition, which is exactly what
the write side buys. What changes for pre-existing rows is a stated, one-time consequence of ruling
1, announced in What's New — not a silent short read.

Also in this PR

🚨 A bug found on the way. NodeTypeEnrichmentHelpers.ReportStuckOverlayToAdmins composed
NotificationService.Dispatch(...) and never subscribed it. Dispatch is cold, so the "instance
is serving a fallback page" notification was never written — while the log line beside it said
"platform admins notified". Two independent silences over one signal.

Re-addressed emitters: PackageUpdateReconciler → Admin (only an admin can apply an update; its
own refusal text says "Update needs a Global Admin" — 124 of the newest 200 rows were this class, in
every catalog reader's bell), StaticRepoImporter.NotifyStartupFailure → Admin,
NodeTypeCompileParkRegistry's System-driven leg → Admin (it filed a compile failure nobody asked
for under the failing TYPE, "so it is still visible — in every per-user bell that can read the
type"), ModuleDiscoveryService states Admin instead of inheriting it.

CreateNotification's addressee is optional (falling back to the main node's partition): it is
public surface that in-mesh source compiles against at RUNTIME, so making it mandatory would break
callers no compiler here can see. Every caller in both repositories passes it explicitly.

Cross-repo

The Plugins half — the anchored bell, the admin-gated platform leg, the React client, the triage
watch declaring itself, and the deleted grace-list line — is
Systemorph/MeshWeaver.Plugins#1297. Core merges first.

Measured, not assumed: scripts/check-type-forwards.py --surface-json reports 0 removed, 5 added
(Notification.Recipient; NotificationService.{PlatformAddressee,ResolveAddressee,DeliveryNamespace,BellQuery}),
so no Pairs-with: is owed. The declaration set does change, so
Dependent suites (MeshWeaver.Plugins) runs against the merge commit.

Follow-ups filed rather than folded in: Plugins#1294 (narrow the triage watch to users who authored
rules — the last mesh-wide notification read), Plugins#1295 (surface isGlobalAdmin to the React
client so its bell can carry the platform leg), #3250 (notification retention).

Verification

  • dotnet build -c Release -warnaserror, one project per invocation: MeshWeaver.Mesh.Contract,
    MeshWeaver.Graph, MeshWeaver.PluginCatalog, MeshWeaver.Compiler.Pipeline,
    MeshWeaver.Hosting, MeshWeaver.Documentation, MeshWeaver.Graph.Test,
    MeshWeaver.Documentation.Test — 0 Warning(s), 0 Error(s) each.
  • dotnet test test/MeshWeaver.Graph.Test -c Release → 1284 passed, 0 failed (2 m 11 s).
  • dotnet test test/MeshWeaver.Documentation.Test -c Release → 246 passed, 0 failed.
  • Negative control on the access boundary (above): staged, red, reverted.
  • Negative control on link integrity: a deliberate broken link reddened
    DocumentationLinkIntegrityTest naming Doc/Architecture/AddressedNotifications; removed, green
    again. The doc pass is not vacuous.

Effect to expect on the 444-unions / 4 476-rows measurement

The bell's 201-of-201 union is gone — replaced by one pinned schema per bell, and one pinned schema
does not consult searchable_schemas at all. What remains on the notifications table is the triage
service's DECLARED watch: one live subscription per process, re-run on notification writes,
against the bell's one per circuit. Plugins#1294 removes that too.

🚨 Production will not move on its own. memex-cloud's self-update is PAUSED
(Admin/UpdatePolicy policy=None since 09-02) and its serving image predates this change, so the
60 SLOW-lines/min figure stands until that deployment is unpaused. What is established here is the
mechanism and the routing, asserted by ResolvePinnedPartition on each leg — not a production
delta.

🤖 Generated with Claude Code

… not filed beside what it is about (#3156, #3216, #3238)

A Notification used to be a satellite of the ENTITY it concerned —
`{entity}/_Notification/{id}`, in THAT entity's partition — so the bell had no
first segment to name. On Postgres that made it the platform's largest
cross-schema fan-out, and it had a second, silent consequence: `Admin` is
deliberately excluded from `public.searchable_schemas`, so the fan-out could
never read `admin.notifications` at all. Every platform-admin notification —
startup errors, failed reconciles, stuck instances — was written, versioned,
and shown to nobody.

Measured on memex-cloud, twice:
  2026-09-02, 8 pods : 444 `[CrossSchema] SLOW` unions per 5 min, avg 4.0 s,
                       Postgres 94-98% CPU.
  2026-09-03, ONE IDLE replica (0.13 cores): 4 476 rows across 201 of 201
                       schemas, 9-10 s per render, filtered to 0 rows in
                       memory; ~60 lines/min on each of 7 replicas (#3238).

The invariant: a Notification has exactly ONE addressee and is delivered into
that addressee's partition — `{addressee}/_Notification/{id}`,
`MainNode = {addressee}` — where the addressee is a user partition or `Admin`.
The entity stays a reference in `TargetNodePath`, which every reader already
groups by, so grouping, the click target and the badge count are unchanged.

  * `Notification.Recipient` records the addressee, so the invariant is
    checkable without parsing a path.
  * `NotificationService` gains `PlatformAddressee`, `ResolveAddressee`,
    `DeliveryNamespace` and `BellQuery` — one definition of where a
    notification goes and one of how it is read back. `Dispatch` resolves the
    addressee once; `recipient: null` is the PLATFORM, never "wherever the
    entity lives".
  * The addressee stays OPTIONAL on `CreateNotification` (falling back to the
    main node's partition): that method is public surface which in-mesh source
    compiles against at RUNTIME, so making it mandatory would break callers no
    compiler here can see.
  * Mis-addressed core emitters re-addressed to `Admin`:
    `PackageUpdateReconciler` (an update only an admin can apply — 124 of the
    newest 200 rows were this class, in every catalog reader's bell),
    `StaticRepoImporter.NotifyStartupFailure`, and
    `NodeTypeCompileParkRegistry`'s System-driven leg, which filed a compile
    failure nobody asked for under the failing TYPE.
    `ModuleDiscoveryService` now states `Admin` instead of inheriting it.

Found on the way: `NodeTypeEnrichmentHelpers.ReportStuckOverlayToAdmins`
composed `NotificationService.Dispatch(...)` and NEVER SUBSCRIBED it. Dispatch
is cold, so the "instance is serving a fallback page" notification was never
written — while the log line beside it said "platform admins notified".

`BellQuery` builds ONE concrete `namespace:` per addressee, deliberately not an
alternation. A single namespace folds into `ParsedQuery.Path`, which is what
pins the read to one schema and skips `searchable_schemas` entirely — the same
move `SecurityQueries.PartitionAssignments` makes, and the reason the Admin
special case there is GONE rather than duplicated. An alternation classifies as
anchored but leaves `Path` null, takes the fan-out route, and is narrowed by
INTERSECTION with `searchable_schemas` — which excludes `Admin`, so it would
have dropped the platform bell again, silently.

`NotificationAddressingTest` pins the rule, the pinning property, and the
access boundary: a platform notification is readable by a platform admin
(`hub.IsGlobalAdmin`, the one canonical predicate) and by NOBODY else, asserted
as a permission verdict and through the bell query itself. The negative was
proven able to fail — granting the non-admin `Viewer` on `Admin` reddens it.

Doc/Architecture/AddressedNotifications records the rulings taken: operator
notices are addressed to `Admin` COLLECTIVELY (one row, not one per admin — a
per-admin fan-out leaves a demoted admin holding copies with no revocation
path), and the legacy rows are neither migrated nor deleted, they age out.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 3, 2026 17:43
@meshweaver-cloud
meshweaver-cloud Bot enabled auto-merge September 3, 2026 17:44
Dispatch resolved the addressee for the bell write but kept the raw `recipient`
for the preference read and the mailbox lookup. A recipient given as a PATH —
which ResolveAddressee explicitly accepts, and which the test pins — would then
deliver to `rbuergi` while reading settings at
`rbuergi/Documents/spec/_NotificationSettings` (a miss, silently defaulted) and
resolving a document node as a User for the email. Same parameter, two meanings,
in one method.

One `addressee` now feeds both channels. `person` is null exactly when
`recipient` is, so the email leg stays gated on "is there an individual" rather
than on the addressee — a platform notification has no mailbox and no
preferences, and must not acquire one by resolving to Admin.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The diff introduces/retains an unsafe Observable.Using(...ImpersonateAsSystem...) pattern in a modified method and adds new payload casts in tests that should use ContentAs<T>() to avoid silent type-degradation failures.

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

Pull request overview

This PR changes notifications from being stored beside the entity they reference to being delivered into the addressee’s partition (or Admin for platform-operator notices), enabling the notification bell to use pinned, single-partition reads (including Admin, which is excluded from public.searchable_schemas) and eliminating expensive cross-schema fan-out.

Changes:

  • Introduces an addressed notification model (Recipient, addressee-derived paths) and a canonical anchored bell query (NotificationService.BellQuery).
  • Re-addresses several operator-facing emitters to Admin and fixes a cold-observable write that previously never executed due to missing subscription.
  • Adds/updates tests and documentation to pin routing/pinning behavior and the admin/non-admin access boundary, plus a What’s New entry describing the behavior change.
File summaries
File Description
test/MeshWeaver.Graph.Test/NotificationServiceTest.cs Updates unit tests to assert addressed notification node shape and compatibility behavior.
test/MeshWeaver.Graph.Test/NotificationAddressingTest.cs Adds end-to-end tests pinning addressee resolution, pinned bell query shape, and admin access boundary.
src/MeshWeaver.PluginCatalog/PackageUpdateReconciler.cs Re-addresses update-related notifications to the platform addressee (Admin).
src/MeshWeaver.PluginCatalog/ModuleDiscoveryService.cs Makes module discovery announcements explicitly addressed to Admin.
src/MeshWeaver.Mesh.Contract/Notification.cs Documents the addressed model and adds Notification.Recipient.
src/MeshWeaver.Graph/StaticRepoImporter.cs Re-addresses startup-failure notifications to Admin while keeping activity log as click target.
src/MeshWeaver.Graph/NotificationService.cs Implements addressed delivery (ResolveAddressee, DeliveryNamespace, BellQuery) and ensures bell+email channels share one resolved addressee.
src/MeshWeaver.Graph/Configuration/NodeTypeEnrichmentHelpers.cs Fixes a previously non-executing notification by subscribing the cold Dispatch observable.
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-03-notifications-reach-the-person-they-are-for.md Adds a user-facing What’s New entry describing the change and its effects.
src/MeshWeaver.Documentation/Data/Architecture/Notifications.md Updates architecture docs to the anchored, per-addressee bell query shape and addressed delivery.
src/MeshWeaver.Documentation/Data/Architecture/CrossSchemaFanOutElimination.md Updates the census status to reflect the bell/panel fan-out elimination and new anchored shape.
src/MeshWeaver.Documentation/Data/Architecture/AddressedNotifications.md Updates the design doc with measurements, corrected routing approach (two pinned legs, not alternation), and migration ruling.
src/MeshWeaver.Compiler.Pipeline/NodeTypeCompileParkRegistry.cs Adjusts compile-failure notification addressing so system-driven failures go to the platform addressee.
Review details

Suppressed comments (1)

test/MeshWeaver.Graph.Test/NotificationServiceTest.cs:99

  • Same payload-cast trap here: prefer ContentAs<Notification>(Mesh.JsonSerializerOptions) over (Notification)node.Content! so the test doesn’t depend on the CLR instance type on the wire.
        ((Notification)node.Content!).Recipient.Should().Be(TestPartition);
  • Files reviewed: 13/13 changed files
  • Comments generated: 2
  • Review effort level: Lite

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

Comment thread src/MeshWeaver.PluginCatalog/PackageUpdateReconciler.cs Outdated
Comment thread test/MeshWeaver.Graph.Test/NotificationServiceTest.cs Outdated
@github-actions

github-actions Bot commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 0)

246 tests   246 ✅  2m 22s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit 9fe9fb1.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 1)

459 tests   459 ✅  52s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit 9fe9fb1.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 3)

368 tests   366 ✅  40s ⏱️
  3 suites    2 💤
  3 files      0 ❌

Results for commit 9fe9fb1.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 5)

    4 files      4 suites   1m 41s ⏱️
1 440 tests 1 440 ✅ 0 💤 0 ❌
1 441 runs  1 441 ✅ 0 💤 0 ❌

Results for commit 9fe9fb1.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 2)

578 tests   386 ✅  2m 38s ⏱️
  3 suites  192 💤
  3 files      0 ❌

Results for commit 9fe9fb1.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 4)

2 228 tests   2 228 ✅  2m 39s ⏱️
    3 suites      0 💤
    3 files        0 ❌

Results for commit 9fe9fb1.

♻️ This comment has been updated with latest results.

@rbuergi

rbuergi commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Status handoff — one failing test, and it is NOT the "old path" problem it was first taken for

Recording where this stands so the next session does not re-derive it. The work on this branch stopped
mid-flight when the authoring session hit its rate limit.

The failure

MeshWeaver Build and Test → shard 5, run 33786410120:

Memex.Portal.Shared.Test.RegistryReconcileDeferralTest
  .RegistryDownPastTheBudget_DefersTheReconcile_AndTheNextFeedReadDrainsIt

Exactly one failing test. Everything else on the branch is green.

What it is NOT

The last note left on this work said "I missed a test — RegistryReconcileDeferralTest reads the
update reminder at the old path."
That has already been done. The test on this branch reads from
the platform bell, with the reasoning written into the test itself:

// 🚨 It is read from the PLATFORM bell, not from the install record. Since
// MeshWeaver#3156 a notification is delivered to its ADDRESSEE, and only a platform admin
// can apply an update … so it is addressed to `Admin` instead of being filed under
// `Plugins/{package}/_Notification` where every catalog reader saw it.
var reminder = await AdminNotifications()
    .Select(ns => ns.FirstOrDefault(n => n.Title.StartsWith("Update available", StringComparison.Ordinal)))
    .Where(n => n is not null)
    .FirstAsync().Timeout(TestTimeouts.Convergence);

with AdminNotifications() anchored per the #3216 design:

.GetQuery("notif|Admin|reconcile",
    $"path:{StartupErrorNotifier.AdminPartition}/_Notification scope:children nodeType:Notification")

So the read side is already migrated. Do not "fix" this by pointing the test back at the old path —
that would assert the defect #3156/#3216 exist to remove.

What the evidence actually says

The test took 1m 48s before failing. That is the shape of the .Timeout(TestTimeouts.Convergence)
on the reactive wait expiring, not an assertion comparing two values. So the reminder never arrived at
the Admin bell at all
— the read is anchored correctly and there is nothing there to read.

That points at the write side, not the test:

  1. Does the emitter actually stamp Recipient = StartupErrorNotifier.AdminPartition and write under
    Admin/_Notification, or is it still writing to Plugins/{package}/_Notification?
  2. If it does write there — is the write subscribed? A composed write that is never subscribed
    silently does nothing, which produces exactly this signature: no error, no row, a reactive wait that
    never completes.
  3. Admin is excluded from searchable_schemas, so confirm the path:-anchored read genuinely reaches
    admin.notifications in the test's backend rather than only in production Postgres.

The three assertions immediately after the wait (Served by registry '{RegistryName}' in the message,
TargetNodePath still pointing at the install record, Recipient == AdminPartition) have never been
reached, so nothing is known about whether they hold.

Also on this branch

gen-manifests.py --check was red on the sibling Plugins branches for the same change set; both were
regenerated and pushed (fix/3060-per-tab-composer, fix/3216-bell-anchored-admin-leg). A src/ edit
moves a module version, so that gate needs re-running after any further source change here.

Related, filed while this was in flight

#3238 — unanchored nodeType:Notification fan-out, measured at 4,476 rows × 201 schemas with
Postgres at 90%. It is the same defect measured again, larger and more recently than the numbers in this
PR's description (444 unions / 5 min across 8 pods). Whoever finishes this should decide explicitly
whether this change set closes #3238, and say so rather than closing it optimistically.

@github-actions

github-actions Bot commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Test Results

   15 files     15 suites   10m 55s ⏱️
5 319 tests 5 125 ✅ 194 💤 0 ❌
5 320 runs  5 126 ✅ 194 💤 0 ❌

Results for commit 9fe9fb1.

♻️ This comment has been updated with latest results.

@rbuergi

rbuergi commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Addendum — measured. It is CI-contention-sensitive, and I could not reproduce it.

Following up my earlier comment with actual measurements rather than reasoning.

The change is coherent: write and read moved together

writes the reminder to test reads the reminder from
main Plugins/{package}/_Notification (no recipient passed) path:Plugins/{PackageId}/_Notification scope:children
this branch Admin/_Notification (recipient: NotificationService.PlatformAddressee) path:Admin/_Notification scope:children

So the branch moves both halves in step, which is correct. The reconciler diff is small — it adds
recipient: NotificationService.PlatformAddressee to the CreateNotification call and documents why
(only a platform admin can apply a plugin update; before the addressed model this class was 124 of the
newest 200 rows on memex-cloud, in every catalog reader's bell).

Local: passes 4/4

Built Debug, ran the single test by filter in this worktree (b7595bd7):

Passed! - Failed: 0, Passed: 1  Duration: 659 ms

Then three more runs under DOTNET_PROCESSOR_COUNT=4, the sanctioned way to reproduce CI races here:

Passed! - 543 ms
Passed! - 608 ms
Passed! - 612 ms

4/4 green, ~600 ms each. CI failed it after 1m 48s — i.e. the reactive wait normally completes
almost instantly and in CI it never completed at all. That is a contention signature, not a logic error.

On main: green across 12 runs

Last 12 MeshWeaver Build and Test runs on main: zero failed jobs in total. So this test has not
failed on main in that window.

🚨 The detector was validated before that null was believed. My first scan grepped only
.message and returned 0 hits on the job I know failed with this exact test — a false null of
precisely the shape #890 has been burned by twice. The test name is in the annotation .title, not
.message. Re-run over title + message, the positive control returns 1. The main result above
is from the corrected detector.

What this does and does not establish

  • The change is not categorically broken — it passes deterministically on a quiet machine, including
    under constrained CPU.
  • It is not a standing failure on main.
  • One CI occurrence, not reproduced. n=1 cannot distinguish "this branch introduced a timing
    sensitivity" from "a rare pre-existing race that this branch's run happened to hit".

🚨 What NOT to do next

Do not re-run the job to see whether it goes green. A passing re-run is the control arm of an
experiment, not an all-clear, and re-running to dismiss a flake is how a real race gets hidden. If a
further CI data point is wanted it should come from a run that carries an actual change, or from
measuring the base rate across a larger population with the corrected (title+message) detector.

The open question is narrow and worth stating plainly for whoever picks this up: under CI contention,
does the path:Admin/_Notification scope:children query emit the reminder within
TestTimeouts.Convergence?
Both the registry-down notification (step 2b, which passes) and this
reminder land in the same partition through the same query, so an answer has to explain why only the
second one is affected.

@rbuergi

rbuergi commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

🚨 Correcting myself twice: this is STALENESS, not a flake — and my proposed "swallow fix" was wrong

I said earlier that the failure was probably a timing sensitivity, and then proposed splitting out a
fix for Notify's .Catch swallow as the way to make it diagnosable. Both were wrong. Reading the
code settles it, and the branch needs a real rebase rather than a re-run.

Why the swallow theory is refuted

RegistryUpdateReconciler — the code the passing step-2b assertion exercises — wraps its Admin
notification in an identical .Catch + LogWarning (RegistryUpdateReconciler.cs:471). Both
notifications are swallowed the same way, so a swallow cannot explain why one arrives and the other
does not. That hypothesis is dead.

What is actually true: the branch predates ~10 merged commits

main moved substantially while this sat. Measured against origin/main today:

this branch main
NotifiedModuleVersion guard (#3213) absent — 0 occurrences present, load-bearing
deterministic identity: on CreateNotification absent identity: $"package-update|{kind}|{candidate}" → CreateOrUpdateNode
system identity in Notify Observable.Using(ImpersonateAsSystem) accessService.RunAsSystem(...)
addressee (recipient: → ResolveAddressee) this branch's own contribution absent

That third row matters on its own: main's Notify carries an explicit warning against the shape this
branch still uses —

"RunAsSystem, never Observable.Using (#1790): the install-records partition is System-owned, and
Rx would otherwise leave the subscribing thread latched as System."

So the rebase is a MERGE OF TWO DESIGNS, not a conflict resolution

Both changes address #3213/#3156 territory from different angles and they interlock:

  • main made the reminder idempotent — a deterministic id keyed on (record, kind, candidate) plus
    a NotifiedModuleVersion marker, so a repeat upserts the same node instead of minting a row.
  • this branch makes it addressed — delivered to Admin rather than filed under
    Plugins/{package}/_Notification where every catalog reader saw it.

Merged correctly, the notification is both addressed to Admin and deterministic. The identity
key must stay (record, kind, candidate) while the addressee becomes Admin — i.e.
CreateNotification needs recipient: and identity: together, and the deterministic id is computed
from (mainNodePath, identity) where mainNodePath is now the addressee. That interaction is
untested by either side
and is exactly where a careless rebase silently reverts one of the two.

Why the test times out rather than asserting

Nothing on this branch writes NotifiedModuleVersion, so the reminder is minted per poll under a fresh
GUID — but it is minted into Admin/_Notification, which the branch's test does read. The timeout
therefore is not explained by staleness alone, and I am not claiming the rebase will fix it. What
staleness does establish is that the current red is not attributable to a race, and that re-running it
or widening its wait would be measuring a tree nobody intends to ship.

Recommendation, revised

  1. Rebase onto main and resolve Notify deliberately — keep main's RunAsSystem, marker guard
    and deterministic identity; keep this branch's addressee. Neither side wins wholesale.
  2. Add the missing coverage for the interaction: a reminder that is addressed to Admin AND
    idempotent across two reconcile passes
    — one node, in the Admin bell, after two passes.
  3. Only then judge the test. If it still times out on a rebased tree, that is a real defect worth
    chasing with the writer-identity diagnostic; on the current tree the question is not well posed.

I am not merging this, and I am not re-running it.

…the other

This branch and main each solved half of the same problem, blind to the other,
and both halves are load-bearing:

  main   (#3213) the reminder is IDEMPOTENT — a deterministic id keyed on the
         condition plus a NotifiedModuleVersion marker, so a repeat upserts the
         same node instead of minting a bell row per poll (124 of the newest 200
         rows on memex-cloud were this one class).
  branch (#3156/#3216) the reminder is ADDRESSED — delivered to the platform
         admins who can actually act on it, instead of being filed under
         Plugins/{package}/_Notification where every catalog reader saw it.

Resolution keeps both. NotificationService.CreateNotification now takes
`recipient` AND `identity`: recipient decides WHERE it is delivered, identity
decides WHETHER a repeat is a new row or the same one refreshed.

PackageUpdateReconciler.Notify keeps main's structure wholesale — RunAsSystem
(this branch still had Observable.Using(ImpersonateAsSystem), which #1790
forbids: Rx leaves the subscribing thread latched as System), the marker guard,
and marker-written-after-the-bell ordering — and gains only the addressee.

THE ONE JUDGEMENT CALL: the deterministic id is keyed on the ADDRESSEE, not on
mainNodePath as main had it. The node lives at {addressee}/_Notification/{id},
so keying on the entity lets two addressees told about the SAME entity+condition
derive the SAME id in different partitions — harmless today (only the platform
is addressed for a package update) and a silent cross-partition collision the
moment a per-user reminder about a shared entity exists.

Adds AnAddressedNotification_IsIdempotentPerAddressee_NotPerEntity, covering the
interaction neither side tested and which a careless rebase silently reverts.

🚨 That test was WRONG on its first attempt and the mutation control caught it.
It asserted `other.Path != first.Path`, but the path is
{addressee}/_Notification/{id} — two addressees always differ in the first
segment whatever the keying, so it passed under the mutation and pinned nothing.
Corrected to assert the ID, it fails on the reverted keying with a concrete
collision: both addressees derive "0b7be12a45527090".

Verified: MeshWeaver.Graph Release -warnaserror 0 Warning(s) 0 Error(s);
NotificationServiceTest 6/6; and RegistryReconcileDeferralTest — the test that
timed out at 20s in CI and blocked this PR — passes in 619 ms. The staleness WAS
the cause; I had explicitly not claimed the rebase would fix it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@rbuergi

rbuergi commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Rebased — and the prediction I hedged on came good: the failing test now passes in 619 ms

Following up my staleness diagnosis. I said "I am not claiming the rebase will fix it". It did.
RegistryDownPastTheBudget_DefersTheReconcile_AndTheNextFeedReadDrainsIt, which timed out at 20 s in
CI and blocked this PR all evening, now passes in 619 ms.

The mechanism is visible in hindsight: this branch addressed the reminder to Admin but predated
NotifiedModuleVersion and the deterministic identity, so the reminder was minted per-poll under a
fresh GUID while the test waited on a specific one. Merged, it is both addressed and idempotent —
exactly what the test asserts.

What the merge decided

Two conflicts, both real design merges rather than textual ones:

this branch main resolved
delivery recipient: → ResolveAddressee → Admin none kept
idempotence none identity: → deterministic id + CreateOrUpdateNode kept
marker guard absent NotifiedModuleVersion kept (main's)
system identity Observable.Using(ImpersonateAsSystem) RunAsSystem main's — #1790 forbids the former (Rx leaves the subscribing thread latched as System)

CreateNotification now takes both recipient and identity: recipient decides WHERE it is
delivered, identity decides WHETHER a repeat is a new row or the same one refreshed.

The one judgement call: the deterministic id is keyed on the addressee, not on mainNodePath
as main had it. The node lives at {addressee}/_Notification/{id}, so keying on the entity lets two
addressees told about the SAME entity+condition derive the SAME id in different partitions — harmless
today (only the platform is addressed for a package update), a silent cross-partition collision the
moment a per-user reminder about a shared entity exists.

🚨 The test that pins this was WRONG on its first attempt, and the mutation control caught it

AnAddressedNotification_IsIdempotentPerAddressee_NotPerEntity covers the interaction neither side
tested. My first version asserted other.Path != first.Path — and passed under the mutation,
pinning nothing: the path is {addressee}/_Notification/{id}, so two addressees always differ in the
first segment whatever the keying.

Corrected to assert the id, the reverted keying fails with the collision named concretely:

AssertionException : Did not expect value to be "0b7be12a45527090" because the id is keyed on the
ADDRESSEE, so telling two people about one entity must not derive one shared id …
Failed! - Failed: 1, Passed: 0

Recording the miss because it is the third check tonight whose output did not depend on what it
claimed to check — after an attribution grep that read .message when the test name is in .title,
and classify-test-run.py printing a hard-coded remedy for a bucket that conflates two mechanisms. A
test named IsIdempotentPerAddressee_NotPerEntity that verifies neither half of its own name is worse
than no test, because the next reader trusts it.

Verification

  • MeshWeaver.Graph Release -warnaserror: 0 Warning(s), 0 Error(s)
  • MeshWeaver.PluginCatalog Release -warnaserror: clean
  • NotificationServiceTest: 6/6
  • RegistryReconcileDeferralTest: passes, 619 ms

Not re-run to check for a flake; the pass is on a changed tree, which is the only sanctioned reason to
re-measure.

rbuergi and others added 2 commits September 4, 2026 07:46
`PackageUpdateReminderIdempotenceTest` still listed the install record's own
satellite collection, `Plugins/{package}/_Notification`. Since this branch
addresses a package-update reminder to the PLATFORM operators — only a platform
admin can apply a plugin update, so an ordinary catalog reader can do nothing
with it — nothing is written there any more. The `.Where(ns => ns.Count == 1)`
never emitted and the fixture died on its 90 s hard deadline, 108 s in.

The fixture was measuring the right property in the wrong place: it asserts
idempotence, not location. It now reads `Admin/_Notification` through
`NotificationService.DeliveryNamespace(PlatformAddressee)` — path-anchored,
which is also the only shape that reaches that schema at all, since `Admin` is
excluded from `searchable_schemas` (#3216).

Narrowed to `TargetNodePath == RecordPath`, and that narrowing is load-bearing:
the platform bell is a SHARED operations inbox, so counting the namespace raw
would have made every assertion depend on whatever else the boot happened to
report — a startup error, a discovery status change. Counting a shared inbox is
how this fixture would have started passing for the wrong reason.

Verified locally: Release + -warnaserror, 0 Error(s); the filtered listing
resolves to exactly 1 row after the first pass and exactly 2 after the new
candidate, so both the positive and the discriminating arms still bite.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HhT5AXByEKo2fRQtWH2wCW
Addresses the review finding on NotificationServiceTest.

`((Notification)node.Content!)` is the trap-door the house rules name: it is
correct only when the value already happens to be the CLR type, and yields a
silent null in exactly the three cases a running mesh produces — untyped JSON
whose `$type` did not resolve, the as-written JsonObject DOM before the
materialization pipeline re-types it, and a same-named type from another
collectible assembly. In a test that is worse than in production: the assertion
does not fail, it throws an InvalidCastException that reads as a harness fault,
or it passes for the wrong reason.

Five sites now read `node.ContentAs<Notification>(Mesh.JsonSerializerOptions)`,
which is also what the production readers of these nodes use — so the fixture
asserts against the materialized shape the bell actually sees.

Verified: Release + -warnaserror, 0 Error(s); NotificationServiceTest 6/6 green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HhT5AXByEKo2fRQtWH2wCW
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants