ModuleUpdateDecision compares version alone, so a consumer can skip an update it needs — the consumer half of Plugins#931 #3163

Activity

1. rbuergi commented on Sep 3, 2026

   

   rbuergi

   on Sep 3, 2026

   ContributorAuthor

   More actions

   This is already implemented — in #3154, which is held as a draft awaiting a maintainer decision

   #3154 ("already landed" means this content against this FRAMEWORK — Plugins#931 consumer half) is
   this exact change: it cites the same site (ModuleUpdateDecision.cs:157), is the same consumer half
   of Plugins#931, and carries the same producer-half context from #3135.

   It is CLEAN and deliberately not marked ready. Its own body states why:

   Opened as a draft on purpose — auto-arm.yml arms every non-draft PR for the merge queue, and
   this one is not to be merged without a maintainer look: it changes what the module updater does on
   every installation, and the fleet-wide consequence of an identity moving is a re-land of every
   affected module.

   So this is not un-started work — it is work waiting on a decision.

   The open question this issue raises is answered there too. This issue asks what a consumer should
   do when the identity is absent from a bundle record (a bundle published before #3135), and reasons
   that fail-closed is "almost certainly right". #3154 should be read for the answer it actually
   implements rather than that decision being re-litigated here.

   Leaving this open rather than closing it: the defect is real and live until #3154 lands. It should be
   closed by that merge, not ahead of it.
2. rbuergi commented on Sep 3, 2026

   

   rbuergi

   on Sep 3, 2026

   ContributorAuthor

   More actions

   Closing — the criterion is met, checked against the criterion rather than the PR title

   #3154 merged to main at 11:08Z today (d51b2971). I read what it changed rather than trusting the name, and this issue's stated need is met end to end.

   1. The comparison this issue asked for

   ModuleUpdateDecision must compare (version, framework identity), not version alone, before answering SkipUpToDate

   src/MeshWeaver.PluginCatalog/ModuleUpdateDecision.cs on main — the landedComparison == 0 branch no longer ends at the version:

   if (servedMvid is not null && !string.Equals(landedMvid, servedMvid, StringComparison.Ordinal))
       return new(ModuleUpdateAction.Land,
           $"version {bundleVersion} is landed, but built against framework "
           + $"{landedMvid ?? "(unrecorded)"} while the registry serves that same version "
           + $"built against {servedMvid} — same content, different platform build; ...");

   The site this issue quoted — $"version {bundleVersion} is already landed" — is gone. The remaining SkipUpToDate reasons both name the framework, or name the fact that no identity was stated.

   2. It is reachable in production, which is the part that was actually missing

   This matters more than the comparison itself, because #3135 had not made the identity reachable at all: BundleRef carried none, and the value the consumer recorded at landing was the identity it had requested on the download URL, not what the module was built against. Comparing those two would never have converged. #3154 closed the whole path:

   • Registry advertises it per bundle — memex/Memex.Portal.Shared/Api/PluginBundleEndpoints.cs emits frameworkMvid = servable?.FrameworkMvid inside the bundles[] array of /api/plugins/bundles/index.json, read off the shelf entry its publisher wrote. Deliberately not the index's top-level identity, which is the registry's own bake.
   • Consumer deserializes it — PluginBundleClient.BundleRef.FrameworkMvid, an init property (not a sixth ctor parameter, so no binary break), read with JsonSerializerDefaults.Web, so frameworkMvid binds.
   • The one production caller passes it — PluginBundleClient.AdoptModule → ModuleUpdateDecision.Decide(..., bundle?.FrameworkMvid). I checked the call sites: AdoptModule is reached from CatalogLayoutAreas (explicit install) and RegistryUpdateReconciler (unattended: true, the lane mw-plugin-test: --allow known-debt ratchet for the plugins gate #723 measured going quiet). There is no other production caller of Decide in either repo — the only remaining callers are tests.
   • Landing writes the compared value back — LandFromBundle(..., advertisedFrameworkMvid) records advertisedFrameworkMvid ?? manifest!.FrameworkMvid on the activation entry, in preference to the archive's requested-lane stamp. That is what makes the two sides converge after one landing instead of re-landing forever.

   3. The open question this issue raised was decided, and not the way this issue guessed

   This issue asked for the absent-identity case to be "a decision rather than a default that falls out of a null check", and reasoned that fail-closed (re-fetch) was "almost certainly right". #3154 decided it explicitly, and split it — fail-closed on one side, fail-open on the other:

   landed	served	verdict
   known	known, different	Land — reason names both identities
   unknown (pre-field entry)	known	Land, once — then it records the identity and settles
   any	unknown	Skip, and the reason says the identity could not be checked
   known	known, equal	Skip — the genuine no-op, with the framework named

   The asymmetry is argued, not defaulted: landing writes the identity back, so an unknown on the landed side heals after one fetch; it can never heal one on the served side, because a registry that states no identity will state none next time either. Answering Land there would re-download every module on every reconcile, forever, on every deployment pointed at a pre-#931 registry.

   So this issue's lean is honoured where it can be (landed-unknown re-fetches, once) and deliberately reversed where it would have bought a permanent loop for a comparison that still has nothing to compare. The verdict there refuses to imply agreement — it says out loud that the registry stated nothing, which is the readable-log half of the original complaint.

   Pinned by test/Memex.Portal.Shared.Test/ModuleUpdateIdentityTest.cs (12 facts + a theory), including SameVersion_DifferentIdentity_Lands (the #723 repro), SameVersion_ServedIdentityUnknown_SkipsAndSaysItCouldNotBeChecked, AfterLanding_TheNextReconcileSkips (convergence), and OmittingTheIdentityArgument_IsTheLegacyAnswer_NotASafetyNet — so nobody reads the optional parameter's default as a safety net.

   What it does not cover, now tracked

   The served-unknown branch is a real blind spot, and #3154 named where it must be closed rather than papering over it at the consumer: a bundle that cannot state what it was built against must become unpublishable. That is still true today — ModulePublish.Accepted.FrameworkMvid is string? and ModulePublish.Validate refuses nothing, so a producer that states no identity shelves a null and every consumer of that bundle sits permanently in skip-and-say-so.

   It was recorded only in Doc/Architecture/ModuleBuildArchitecture's roadmap, and Plugins#931 is closed, so it had no open tracker. Filed as #3211.

   That is producer-side work and outside what this issue asked for. Closing this one as done.
3. rbuergi

   closed this as completedon Sep 3, 2026