From 70b6565116cf814adc9cefb2e15d1bdafabcd41b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20B=C3=BCrgi?= <6334612+rbuergi@users.noreply.github.com> Date: Mon, 14 Sep 2026 08:24:03 +0200 Subject: [PATCH 1/2] fix(#4259): an UPDATE restores a declared node that is absent from the mesh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `CatalogLayoutAreas.InstallOrUpdate` has three exits and only two observed the mesh. The incremental one fetched exactly `newManifest.DiffFrom(record.InstalledFiles)` — a comparison of two DECLARATIONS, the candidate's lock against the record the installer itself stamped. A node lost AFTER a previous install has an unchanged file hash, so it never entered the delta, was never fetched, and never reached `DecideAndWrite` — the one place that would have restored it, because it writes whenever `current is null`. The presence-awareness sat one layer below a set the absent file could not enter. And it is the exit an update always takes. #3485's heal lives on the module-hash-EQUAL exit, which an update never takes by definition, and every new publication moves the hash again — so a package that loses a node stays short across arbitrarily many updates, each one green. Measured on memex.meshweaver.cloud 2026-09-14 (read-only): `Hosting/TriageStatus/Release/20260913081235-bylQeIj_`, created on that portal at 08:12:35.446Z with `status: Succeeded`, names all three of `Hosting/Deployment/Source/TriageIntake` and both `Hosting/TriageStatus/Source/*` in its `sourceVersions` — so they landed and compiled. The 22:03:03Z update then wrote exactly the two files whose content had moved (`AksOpsResult` v1 22:02:54.601Z, `PlatformBuildInboxWatcher` v16 22:02:57.255Z) and left eleven declared-and-absent nodes absent, while stamping a 224-file record that asserts every one of them. The fix: derive the candidate manifest's declared node paths through the installer's own file->node rule, read them in ONE batched `IStorageAdapter.ReadMany`, and add back the declared files whose node is absent. `InstallCompleteness.FilesToRestore` is pure so every arm pins offline; `ObservePresent` answers null — never an empty set — when the mesh was not read, and the caller says so at Warning rather than proceeding as if it had checked. A restored SHARED Source/Test file falls back to a full install, by the same rule the existing changed-file guard applies. And a fetch that returns fewer files than it asked for is now named at Error instead of vanishing. Verified by a deterministic repro that is not vacuous: reverting only the `CatalogLayoutAreas.cs` change and rebuilding fails `AnIncrementalUpdateOverAMissingNode_RestoresIt` in 2 m on the `WaitForNode(present: true)` timeout; with the change it passes in 3 s. `InstallCompletenessTest` 21/21, `MeshWeaver.Documentation.Test` 6/6, and every touched project builds `-c Release -warnaserror` at 0 Error(s) 0 Warning(s). Work products: `Doc/Architecture/DeclaredIsNotLanded` carries the three exits, the measured case and the sweep hazard — `Hosting/TriageItem` reads `compilationStatus: Ok` while all six of its `compiledSources` are gone, so `content.compilationStatus:Error` counts only types that ATTEMPTED a compile and failed. `NodeTypeCompilation.md` gains the `shared=@...` decision #4174 left open: `@` is absolute and never rebases, so a tree carrying an absolute cross-reference is not importable under a prefix, and the remedy is at the install boundary. Refs #4259, #4257, #3485, #4208, #4174 Co-Authored-By: Claude Opus 5 (1M context) --- .../Data/Architecture.md | 1 + .../Data/Architecture/DeclaredIsNotLanded.md | 190 +++++++++++++ .../Data/Architecture/NodeTypeCompilation.md | 32 +++ ...date-restores-content-that-went-missing.md | 28 ++ .../CatalogLayoutAreas.cs | 142 +++++++++- .../InstallCompleteness.cs | 101 +++++++ .../InstallCompletenessTest.cs | 250 ++++++++++++++++++ 7 files changed, 734 insertions(+), 10 deletions(-) create mode 100644 src/MeshWeaver.Documentation/Data/Architecture/DeclaredIsNotLanded.md create mode 100644 src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-14-an-update-restores-content-that-went-missing.md diff --git a/src/MeshWeaver.Documentation/Data/Architecture.md b/src/MeshWeaver.Documentation/Data/Architecture.md index ecca308457..e6540ef3ac 100644 --- a/src/MeshWeaver.Documentation/Data/Architecture.md +++ b/src/MeshWeaver.Documentation/Data/Architecture.md @@ -282,6 +282,7 @@ Each theme starts with its introductory page, followed by related architecture t - [Sealed Publication Reads](SealedPublicationReads) — a publication is unreadable for ~90s per target per publish and the `plugins` prefix has two writers; the three answers (404/503/412), the generation that pins one instance, and the mix no reader can detect - [Sealed Publication Generations](SealedPublicationGenerations) — the layout that makes that mix UNREPRESENTABLE (a directory per publication plus a pointer swapped last), the reader contract, the retention rule, and the ordered migration that avoids a new-writer/old-writer half-state - [Install Completeness](InstallCompleteness) — what an install RECORD declares landed, compared against what is actually in the mesh; the comparison nothing made until #3485, and why only one of its five verdicts is a pass +- [Declared Is Not Landed](DeclaredIsNotLanded) — the three exits an install can take and the one that never looked at the mesh: an UPDATE fetched the manifest diff alone, so a node lost after a previous install survived every subsequent update, each reporting success. The measured case, the batched presence read that fixes it, and the sweep hazard that made the damage read smaller than it was - [CI Content Bake](CiContentBake) - [Bundle Delivery Stages](BundleDeliveryStages) — the four independent stages between a merge and a portal serving prebuilt bytes (write · compose · select · deliver), which of #3461 / #3732 / #3768 / #3583 owns each, the instrument that answers for each — and why a reading taken at one stage is not evidence about another - [Framework Identity Churn](FrameworkIdentityChurn) — the identity moves on every core COMMIT, not on every content change (43 merges, 5 touching the full-MVID set, ≥18 identities in 24h); the commit sha compiled into `AssemblyInformationalVersion` is why, the falsification test that refuted the local fix (0 of 22 control, 22 of 22 and 21 of 21), and the four costed options with every runner-hour labelled as arithmetic diff --git a/src/MeshWeaver.Documentation/Data/Architecture/DeclaredIsNotLanded.md b/src/MeshWeaver.Documentation/Data/Architecture/DeclaredIsNotLanded.md new file mode 100644 index 0000000000..c286a1059b --- /dev/null +++ b/src/MeshWeaver.Documentation/Data/Architecture/DeclaredIsNotLanded.md @@ -0,0 +1,190 @@ +--- +Name: Declared Is Not Landed +Category: Architecture +Description: An install record is a declaration, never an observation. Three exits can write it and until now only one of them looked at the mesh — so a node lost after an install could survive every subsequent update, each reporting success. The measured case on memex.meshweaver.cloud, the exit that was blind, and the sweep hazard that made the damage read smaller than it was. +Icon: +--- + +# Declared Is Not Landed + +An install record is a statement about the **source**. It names the files a bundle shipped and the +hash of each. Nothing in it observes the mesh, and nothing in the installer's own counters does +either — `InstallResult(Total, Written)` counts what the installer *decided* to write. + +That gap has been closed one exit at a time, and this page is about the exit that was still open. + +## The three exits an install can take + +`CatalogLayoutAreas.InstallOrUpdate` chooses among three, on two questions: does the record's +`ModuleVersion` equal the candidate's, and does the record carry a file map? + +| Exit | Taken when | Does it look at the mesh? | +|---|---|---| +| **Skip / heal** | the module hash is EQUAL | **Yes**, since MeshWeaver#3485 — `InstallCompleteness.Observe` reads every declared node path and `SkipOrHeal` runs a full install rather than skipping an incomplete one | +| **Full install** | no record, or a record with no file map | **Yes** — `DecideAndWrite` writes whenever `current is null`, so everything absent is restored | +| **Incremental update** | the module hash MOVED and the record has a file map | **It did not.** Fetched exactly `newManifest.DiffFrom(record.InstalledFiles)` | + +The incremental exit's fetch set was a comparison of two *declarations* — the candidate's +`manifest.lock` against the record the installer itself wrote. A node lost after a previous install +has an unchanged file hash, so it never entered `delta.AddedOrChangedFiles`, was never fetched, and +never reached `DecideAndWrite` — the one place that would have restored it. **The +presence-awareness sat one layer below a set the absent file could not enter.** + +## 🚨 And it is the exit an update always takes + +This is what turns a one-off loss into a permanent one. #3485's heal lives on the module-hash-EQUAL +exit. An *update* never takes that exit by definition, and every new publication moves the hash +again. So a package that loses a node stays short across arbitrarily many updates, each one +reporting success, and the only escape is a boot on which the source's hash happens to equal the +record's **and** the install lane happens to run. + +## The measured case + +`Plugins/Hosting` on **memex.meshweaver.cloud**, read 2026-09-14T06:0xZ. All reads through `get` / +`search`; nothing was mutated. + +**The morning: the files were there and they compiled.** The release node +`Hosting/TriageStatus/Release/20260913081235-bylQeIj_` — created on that portal at +2026-09-13T08:12:35.446Z, `status: Succeeded` — names its inputs: + +``` +sourceVersions: + Hosting/Deployment/Source/TriageIntake 639248839464110360 (2026-09-13T08:12:26.411Z) + Hosting/TriageStatus/Source/TriageStatusContent 639248839461605390 (…08:12:26.160Z) + Hosting/TriageStatus/Source/TriageStatusLayoutAreas 639248839464108660 (…08:12:26.410Z) +testVersions: + Hosting/TriageStatus/Test/TriageStatusTests … + Hosting/TriageStatus/Test/TriageStatusTestsArea … +``` + +A release records the source nodes a successful compile actually read. So those five nodes existed +on this portal, nine seconds before the compile that consumed them. + +**The evening: the update ran, and wrote only what had changed.** The record was stamped at +2026-09-13T22:03:03.221Z — `installedNodeCount: 224`, `version 1.19.16`. Two nodes carry write +timestamps from that install: + +| node | version | written | +|---|---|---| +| `Hosting/Deployment/Source/AksOpsResult` | 1 (new file) | 2026-09-13T22:02:54.601Z | +| `Hosting/Deployment/Source/PlatformBuildInboxWatcher` | 16 (changed file) | 2026-09-13T22:02:57.255Z | + +Everything else in `Hosting/Deployment/Source` still carries its older timestamp — 08:09:41, +09-12, 09-09, 09-08, 08-28, 08-19. The update wrote exactly the two files whose content had moved. + +**And eleven declared files had no node at all.** Anchored reads, each with a same-anchor positive +control: + +| probe | result | +|---|---| +| `namespace:Hosting/Deployment/Source scope:subtree nodeType:Code` | **14**, `truncated: false` — no `TriageIntake` (the other 14 are the control: readable, same query shape) | +| `namespace:Hosting/Deployment/Test scope:subtree nodeType:Code` | **3** — no `TriageIntakeTests` | +| `namespace:Hosting/TriageStatus scope:subtree` | **1** — the Release node alone; no `Source/`, no `Test/` | +| `namespace:Hosting/TriageItem scope:subtree` | **1** — the Release node alone | +| `namespace:Hosting scope:children nodeType:NodeType` | **19**, `truncated: false` — the denominator | +| `namespace:Hosting scope:children nodeType:NodeType content.compilationStatus:Error` | **8 of 19** | + +All eleven are named in the record's own `installedFiles` map. The record asserts them; the mesh +does not hold them; the update that stamped the record did not fetch a single one. + +## 🚨 The sweep hazard: 8 of 19 understates it, and the 9th reads green + +`Hosting/TriageItem` is **not** in the Error set. Its own node says: + +``` +compilationStatus: Ok +lastCompileSucceededAt: 2026-09-13T08:12:35.5789283Z +lastCompiledVersion: 6 +requestedReleaseAt: 2026-09-13T21:56:05.9665591Z +lastReleaseRequestHandledAt: 2026-09-13T21:56:05.9665591Z ← handled without a recompile +lastCompileStartedAt: 2026-09-13T08:12:32.047279Z ← still the morning's +compiledSources: 6 entries — TriageActions, TriageItemContent, TriageItemLayoutAreas, + Hosting/Deployment/Source/TriageIntake, and two Test nodes +currentSourceVersions: {} +currentSourceFingerprint: e3b0c44298fc1c14 ← SHA-256 of the EMPTY STRING +``` + +Every one of those six `compiledSources` is now absent from the mesh, and the type serves a cached +assembly at `compilationStatus: Ok`. + +**So `content.compilationStatus:Error` is a sweep for types that have *attempted* a compile and +failed. A type whose sources all vanished but which has not been re-driven since is in the GREEN +population.** The honest statement of this portal's damage is not "8 of 19 are broken" but "8 of 19 +report Error, and at least one more has lost every source it compiled from while reporting Ok". + +This joins the list of reasons a `compilationStatus` sweep's number is not a census — alongside the +RLS filtering and the two-executor split already recorded in +[NodeType Compilation](/Doc/Architecture/NodeTypeCompilation). The cross-check that does see it is +the record-vs-mesh comparison on this page: `compiledSources` (or the release node's +`sourceVersions`) against what the mesh holds. + +## The fix + +`IncrementalUpdate` now asks the mesh before it fetches. + +1. Derive the candidate manifest's declared node paths — `newManifest.Files.Keys` through + `PackageInstaller.NodePathForFile`, the installer's own file→node rule and not a second + implementation of part of it. +2. `InstallCompleteness.ObservePresent` — **one** batched `IStorageAdapter.ReadMany` over that + bounded, known set. Never a query (a stale negative would re-fetch a file that is present) and + never N point reads of possibly-absent paths (which is what opens a storm breaker on the owning + hub). +3. `InstallCompleteness.FilesToRestore` — pure, so every arm is pinnable offline — returns the + declared files the delta is not already fetching whose node is absent. +4. The fetch asks for `delta.AddedOrChangedFiles ∪ restore`. `DecideAndWrite` then writes them, + because `current is null`. + +It costs one batched read per incremental update — the same read +[Install Completeness](/Doc/Architecture/InstallCompleteness)'s post-install verification already +pays on this exit, moved to where it can still change the outcome instead of only reporting it. + +### 🚨 Three answers, never two + +`ObservePresent` returns `null` when the mesh was **not read** — no storage adapter, or a batched +read that faulted — and never an empty set. An empty set would say "the mesh holds none of them", +which on a Postgres read that faulted (a satellite table that does not exist answers `42P01`, not +"absent") would make an install re-fetch every file it ships, every time. `FilesToRestore` maps that +`null` to an empty restore set, and the caller **says so at Warning**: an unobserved mesh is not a +clean one, and the line is spelled so it cannot be read as a pass. + +### The other silent hole in the same method + +`IPackageSource.FetchPackageFiles(package, gitRef, paths)` filters — locally in the interface +default, server-side in `RegistryPackageSource` — and its contract is explicit that *"paths absent +from the package simply don't appear in the result"*. A requested path the source does not serve (a +serving-side casing difference, a stale registry cache, a bundle that disagrees with its own lock) +therefore vanished with no exception and no log line, after which the record was stamped with the +full declared map anyway. The returned set is now compared against the requested one and the +shortfall is named at Error. It reports only; it never fails the update — the install that did run +is a fact, and collapsing "landed short" into "failed" would make a working update a new way to +break. + +## The rule this generalises to + +> **A decision about what the mesh needs may not be taken from a record the writer wrote.** Any +> comparison whose two sides are both declarations — a lock against a record, a hash against a hash, +> a count against a count — can only tell you what the *source* did. If the decision is "does this +> have to be written", exactly one of the two sides must be a read of the mesh. + +The same sentence is why #3485 exists, why the post-install verification exists, and why this page +does. Each of the three closed it on one exit; none of them closed it as a principle, which is how +the third exit stayed open through two fixes aimed at the same defect. + +## What this does NOT do + +- **It does not explain why the nodes went missing.** The release node proves they were written and + compiled; something removed them between 2026-09-13T08:12:35Z and 21:56:07Z, and no instrument a + session can run names it. This change makes the loss *self-correcting on the next update* instead + of permanent — it is not a diagnosis of the loss. +- **It does not re-arm a PARKED NodeType.** A type that failed a compile serves its cache and does + not re-drive itself when sources arrive; that is MeshWeaver#4208's subject. Restoring the content + and recycling the type are two steps, and on an image without #4208 they stay two steps. + +## See also + +- [Install Completeness](/Doc/Architecture/InstallCompleteness) — the five verdicts, only one of + which is a pass, and the post-install read-back that reports the outcome on every writing exit +- [NodeType Compilation](/Doc/Architecture/NodeTypeCompilation) — what a compile reads, what a + release records, and the other reasons a `compilationStatus` sweep is not a census +- [CQRS and Content Access](/Doc/Architecture/CqrsAndContentAccess) — why the presence read here is + a batched store read and never a query diff --git a/src/MeshWeaver.Documentation/Data/Architecture/NodeTypeCompilation.md b/src/MeshWeaver.Documentation/Data/Architecture/NodeTypeCompilation.md index 444addd112..cca5423322 100644 --- a/src/MeshWeaver.Documentation/Data/Architecture/NodeTypeCompilation.md +++ b/src/MeshWeaver.Documentation/Data/Architecture/NodeTypeCompilation.md @@ -314,6 +314,38 @@ behaves identically. `CodeQueryResolver` is the single expansion/grouping implementation, so the files shown in the GUI are exactly the files that compile. +#### 🚨 `@` is ABSOLUTE and never rebases — so a tree carrying one is not relocatable + +The grammar has both forms, and the difference is load-bearing: + +| entry | `CodeQueryResolver.Expand` yields | addressed against | +|---|---|---| +| `namespace:Source scope:subtree` (no `/` in the value) | `namespace:{selfPath}/Source scope:subtree` | **the declaring NodeType** — rebased by `RebaseRelativeNamespace` | +| `shared=@Other/Type/Source` | `path:Other/Type/Source` **and** `namespace:Other/Type/Source scope:subtree` | **the mesh root** — verbatim, never rebased | + +That is deliberate and must stay so. The dominant use of `shared=@…` is a +genuinely cross-partition include — `Store/Plugin` pulls in +`shared=@Store/Coupon/Source`, `@Store/Core/Source`, `@Store/Licensing/Source` +— and rebasing those under the declaring node's prefix would silently redirect +every one of them. + +**The consequence, which is a property to know rather than a bug to fix: a tree +containing an absolute cross-reference cannot be imported under a prefix.** +Import `Northwind/Product` — whose `sources` name +`shared=@Northwind/AnalyticsCatalog/Source/Supplier` — as +`MeshWeaver/samples/Graph/Data/Northwind/Product`, and the reference still +resolves against `Northwind/…`, a root that does not exist on that mesh. The +compile then fails with `CS0246` on the shared types while +`failedSourceQueries` names the two unresolved entries exactly. Nothing is +missing; one reference is un-rebased, and nothing in the compiler can rebase it +without breaking the cross-partition case. + +The remedy is at the INSTALL boundary, not in the resolver: **install a package +at the root its sources were authored for.** MeshWeaver#4174 is the worked +instance — the mirrored `samples/` subtree was retired and `Northwind` installed +at its own root as a `Store/Plugin` catalog entry, after which the same +`shared=@…` values resolve as authored. + --- ## Watching compile progress diff --git a/src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-14-an-update-restores-content-that-went-missing.md b/src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-14-an-update-restores-content-that-went-missing.md new file mode 100644 index 0000000000..6c18678e16 --- /dev/null +++ b/src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-14-an-update-restores-content-that-went-missing.md @@ -0,0 +1,28 @@ +--- +Name: An update restores content that went missing +Category: Fix +Description: Updating a plugin now brings back any of its files whose node has disappeared from the mesh — not only the files that changed since the last install. +Icon: ArrowSync +Order: -20260914 +--- + +# An update restores content that went missing + +Updating an installed plugin used to fetch only the files whose content had changed since the last +install. That is the right thing to do when nothing else moved — but if one of the plugin's nodes +had disappeared from the mesh in the meantime, its file was unchanged, so the update skipped it and +the node stayed gone. Every later update skipped it again, and each one reported success. + +An update now checks the mesh before it fetches. Any file the plugin ships whose node is missing is +re-fetched and written back, alongside the files that genuinely changed — so the ordinary "update +the plugin" action repairs the gap instead of stepping over it. + +Two smaller things travel with it: + +- If the mesh cannot be read at that moment, the update says so and does **not** quietly proceed as + if everything were present. +- If the source returns fewer files than the update asked for, that shortfall is now named instead + of passing unnoticed. + +The symptom this removes: a plugin whose views render empty, or whose node type will not compile, +because source files the install record still lists are not actually there. diff --git a/src/MeshWeaver.PluginCatalog/CatalogLayoutAreas.cs b/src/MeshWeaver.PluginCatalog/CatalogLayoutAreas.cs index fa9da991c4..7e3b6209be 100644 --- a/src/MeshWeaver.PluginCatalog/CatalogLayoutAreas.cs +++ b/src/MeshWeaver.PluginCatalog/CatalogLayoutAreas.cs @@ -1285,7 +1285,37 @@ private static IObservable SkipOrHeal( return withModule(Observable.Return(new InstallResult(0, 0))); } - // The manifest-diff fast path: fetch only manifest.lock, diff, fetch only the changed files. + /// + /// The manifest-diff fast path: fetch only manifest.lock, diff, fetch only the changed + /// files — plus the declared files whose node is ABSENT from the mesh (MeshWeaver#4259). + /// + /// 🚨 The diff alone is a comparison of two DECLARATIONS. + /// newManifest.DiffFrom(record.InstalledFiles) asks what the source changed since the + /// record the installer itself stamped; nothing in it observes the mesh. A node lost AFTER a + /// previous install therefore has an unchanged hash, never enters + /// delta.AddedOrChangedFiles, is never fetched, and never reaches + /// PackageInstaller.DecideAndWrite — which would have restored it, because it writes + /// whenever current is null. The presence-awareness sits one layer below a set the absent + /// file never enters. + /// + /// 🚨 And only the hash-EQUAL path could heal it. #3485 made the up-to-date exit + /// observe the mesh before skipping, so a reinstall at an unchanged module hash repairs. But an + /// UPDATE never takes that exit, and every new publication moves the hash again — so a package + /// that loses a node stays short across arbitrarily many updates, each reporting success. + /// Measured on memex.meshweaver.cloud: Plugins/Hosting took this path at + /// 2026-09-13T22:03Z and wrote exactly the two files whose content had moved + /// (…/Source/AksOpsResult v1 22:02:54.601Z, …/Source/PlatformBuildInboxWatcher v16 + /// 22:02:57.255Z) while eleven declared-and-absent files stayed absent — among them the three + /// the release node Hosting/TriageStatus/Release/20260913081235-bylQeIj_ had compiled + /// from that same morning. + /// + /// The repair costs ONE batched over the candidate + /// manifest's own declared node paths — the same read already pays on + /// this exit, moved to where it can still change the outcome instead of only reporting it. A + /// read that could not run does NOT widen the fetch and says so at Warning: an unobserved mesh + /// is not a clean one, and re-fetching a whole package on every read hiccup would be a new cost + /// nobody asked for. + /// private static IObservable IncrementalUpdate( IMessageHub hub, IPackageSource source, string sourceRef, PackageManifest pkg, PackageManifest record, ILogger? logger, string? authorizingUserId) @@ -1330,19 +1360,111 @@ private static IObservable IncrementalUpdate( .Select(p => p!) .ToHashSet(StringComparer.Ordinal); - logger?.LogInformation( - "Updating {Id} incrementally: {Changed} changed file(s), {Removed} removed → module {ModuleVersion}.", - pkg.Id, delta.AddedOrChangedFiles.Count, delta.RemovedFiles.Count, newManifest.ModuleVersion); + // 🚨 #4259 — the diff is a statement about the SOURCE and the RECORD; ask the MESH + // too. The declared node paths of the candidate manifest are a bounded, KNOWN set, + // so this is one batched ReadMany, never a query (a stale negative here would + // re-fetch a file that is present) and never N point reads of possibly-absent paths + // (which is what opens a storm breaker on the owning hub). + var declaredNodePaths = newManifest.Files.Keys + .Select(f => PackageInstaller.NodePathForFile(f, parsers)) + .Where(p => !string.IsNullOrWhiteSpace(p)) + .Select(p => p!) + .ToImmutableSortedSet(StringComparer.Ordinal); - return (delta.AddedOrChangedFiles.Count == 0 - ? Observable.Return((IReadOnlyList)[]) - : source.FetchPackageFiles(pkg, sourceRef, delta.AddedOrChangedFiles)) - .SelectMany(changedFiles => PackageInstaller.InstallNodeRepoDelta( - hub, pkg, newManifest, changedFiles, removedNodePaths, sourceRef, logger, - authorizingUserId)); + return InstallCompleteness + .ObservePresent( + hub.ServiceProvider.GetService(), + hub.JsonSerializerOptions, declaredNodePaths) + .SelectMany(present => + { + var restore = InstallCompleteness.FilesToRestore( + newManifest.Files, delta.AddedOrChangedFiles, present, parsers); + + // 🚨 The SAME rule the changed-file guard above applies, and for the same + // reason: a package's shared Source/Test are compile inputs for EVERY type + // in it, so the delta's owner-derivation would restore the file and + // recompile nothing but its nominal owner. An absent shared source is + // therefore a full install, whose release-all covers the siblings — the + // caller catches this and falls back, exactly as it does for a changed one. + if (restore.Any(p => sharedPrefixes.Any( + s => p.StartsWith(s, StringComparison.Ordinal)))) + throw new InvalidOperationException( + $"Package '{pkg.Id}' is missing shared Source/Test node(s) " + + $"([{string.Join(", ", restore.Take(InstallCompleteness.MaxNamedInALine))}]); " + + "full install required."); + + if (present is null) + logger?.LogWarning( + "Updating {Id} incrementally: the mesh could NOT be read, so the " + + "{Declared} declared node(s) were not checked for absence. This " + + "is not a pass — a node lost since the last install will not be " + + "restored by this update (MeshWeaver#4259).", + pkg.Id, declaredNodePaths.Count); + else if (restore.Count > 0) + logger?.LogWarning( + "Updating {Id} incrementally: {Restore} of {Declared} declared " + + "node(s) are ABSENT from the mesh although their file hash has " + + "not moved, so the diff alone would have skipped them. Re-fetching " + + "them: [{Files}] (MeshWeaver#4259).", + pkg.Id, restore.Count, declaredNodePaths.Count, + string.Join(", ", restore.Take(InstallCompleteness.MaxNamedInALine))); + + var wanted = delta.AddedOrChangedFiles.Union(restore); + logger?.LogInformation( + "Updating {Id} incrementally: {Changed} changed file(s), {Restored} " + + "restored, {Removed} removed → module {ModuleVersion}.", + pkg.Id, delta.AddedOrChangedFiles.Count, restore.Count, + delta.RemovedFiles.Count, newManifest.ModuleVersion); + + return (wanted.Count == 0 + ? Observable.Return((IReadOnlyList)[]) + : source.FetchPackageFiles(pkg, sourceRef, wanted)) + .Do(fetched => ReportFetchShortfall(pkg.Id, wanted, fetched, logger)) + .SelectMany(changedFiles => PackageInstaller.InstallNodeRepoDelta( + hub, pkg, newManifest, changedFiles, removedNodePaths, sourceRef, + logger, authorizingUserId)); + }); }); } + /// + /// 🚨 A file the fetch was ASKED for and did not get back is a shortfall with no other + /// voice. + /// filters — locally in the interface default, server-side in + /// — and its contract is explicit that *"paths absent from + /// the package simply don't appear in the result"*. So a requested path the source does not + /// serve (a serving-side casing difference, a stale registry cache, a bundle that disagrees with + /// its own lock) vanishes with no exception and no line, the delta installs whatever did arrive, + /// and WriteInstalledRecord then stamps the FULL declared map — a record asserting a file + /// nothing ever wrote. + /// + /// Reports only; it never fails the update. The install that did run is a fact, and + /// collapsing "landed short" into "failed" would make a working update a new way to break — + /// carries the outcome verdict on this same exit. + /// + /// The package being updated. + /// The paths the fetch asked for. + /// What came back. + /// Where the shortfall is named. + private static void ReportFetchShortfall( + string packageId, IReadOnlySet wanted, IReadOnlyList fetched, + ILogger? logger) + { + if (logger is null || wanted.Count == 0) + return; + var missing = wanted + .Except(fetched.Select(f => f.RelativePath), StringComparer.Ordinal) + .ToImmutableSortedSet(StringComparer.Ordinal); + if (missing.Count == 0) + return; + logger.LogError( + "Updating {Id} incrementally: the source returned {Fetched} of the {Wanted} file(s) " + + "asked for — [{Missing}] did NOT travel, so their nodes cannot be written, and the " + + "install record will still declare them (MeshWeaver#4259).", + packageId, fetched.Count, wanted.Count, + string.Join(", ", missing.Take(InstallCompleteness.MaxNamedInALine))); + } + private static ILogger? Logger(LayoutAreaHost host) => host.Hub.ServiceProvider.GetService() ?.CreateLogger("MeshWeaver.PluginCatalog.Catalog"); diff --git a/src/MeshWeaver.PluginCatalog/InstallCompleteness.cs b/src/MeshWeaver.PluginCatalog/InstallCompleteness.cs index 081a047229..9ad0e612d9 100644 --- a/src/MeshWeaver.PluginCatalog/InstallCompleteness.cs +++ b/src/MeshWeaver.PluginCatalog/InstallCompleteness.cs @@ -87,6 +87,107 @@ public static ImmutableSortedSet DeclaredNodePaths( public static ImmutableSortedSet DeclaredNodePaths(PackageManifest? record) => DeclaredNodePaths(record, PackageInstaller.BuiltInParsers); + /// + /// 🚨 What an INCREMENTAL update must add back to its delta — the files a candidate + /// manifest declares, is not already fetching because their hash did not move, and whose node + /// is NOT in the mesh. + /// + /// The defect this closes (MeshWeaver#4259). The incremental path's fetch set is + /// newManifest.DiffFrom(record.InstalledFiles) — a comparison of two DECLARATIONS, the + /// candidate's lock against the record the installer itself stamped. Nothing in it observes the + /// mesh. So a node lost AFTER a previous install is never re-fetched and never re-written: its + /// hash is unchanged, so it is not in the delta, so it never reaches + /// PackageInstaller.DecideAndWrite — which would have written it, because it writes + /// whenever current is null. The presence-awareness exists one layer BELOW a set the + /// absent file never enters. + /// + /// Measured on memex.meshweaver.cloud, 2026-09-14. Plugins/Hosting took the + /// incremental path at 2026-09-13T22:03Z (module 1.18 → 1.19) and wrote exactly the two files + /// whose content had moved — Hosting/Deployment/Source/AksOpsResult (v1, 22:02:54.601Z) + /// and Hosting/Deployment/Source/PlatformBuildInboxWatcher (v16, 22:02:57.255Z). Eleven + /// other declared files whose hashes had NOT moved were absent from the mesh and stayed absent, + /// among them Hosting/Deployment/Source/TriageIntake.cs and both + /// Hosting/TriageStatus/Source/*.cs. Those three had been present that morning — the + /// release node Hosting/TriageStatus/Release/20260913081235-bylQeIj_ names all three in + /// its sourceVersions and its status is Succeeded — so this is the loss-then-update + /// shape, not a delivery that never happened. + /// + /// 🚨 Only the hash-EQUAL path could heal it, and that is the path an update never + /// takes. MeshWeaver#3485 made the up-to-date exit observe the mesh before skipping, so a + /// reinstall at an unchanged module hash repairs. But once the hash has moved, every install + /// takes this path — and each new publication moves it again. A package that loses a node can + /// therefore stay short across arbitrarily many updates while each one reports success. + /// + /// 🚨 Three answers, never two. A null + /// means the mesh was not read — the batched read faulted, or no storage adapter exists — and + /// yields EMPTY. Widening the fetch on an unobserved mesh would re-fetch the whole package every + /// time a read hiccups; refusing to widen is the conservative arm, and the caller is obliged to + /// say that it could not check rather than proceed as if it had. An empty-but-non-null set is a + /// real observation of a partition holding none of the declared nodes. + /// + /// The CANDIDATE manifest's file map — what the source ships now. + /// Deliberately not the record's: the fetch may only ask for files the source still has. + /// The delta's own added/changed files, which the caller is + /// fetching regardless. Never returned, so the caller can union without de-duplicating. + /// The node paths OBSERVED in the mesh, or null when the + /// read did not happen — see the remarks. + /// The parser registry the INSTALL would use; the file→node rule is + /// DI-dependent (#3659), so a second implementation here would answer differently. + /// The declared files to add to the fetch, ordinal-sorted; empty when nothing is + /// missing, when nothing is declared, or when the mesh was not observed. + public static ImmutableSortedSet FilesToRestore( + IReadOnlyDictionary? declaredFiles, + IReadOnlySet? alreadyFetching, + IReadOnlySet? presentNodePaths, + FileFormatParserRegistry parsers) + { + ArgumentNullException.ThrowIfNull(parsers); + var empty = ImmutableSortedSet.Empty.WithComparer(StringComparer.Ordinal); + if (presentNodePaths is null || declaredFiles is not { Count: > 0 }) + return empty; + return declaredFiles.Keys + .Where(f => alreadyFetching?.Contains(f) != true) + .Select(f => (File: f, Node: PackageInstaller.NodePathForFile(f, parsers))) + .Where(x => !string.IsNullOrWhiteSpace(x.Node) && !presentNodePaths.Contains(x.Node!)) + .Select(x => x.File) + .ToImmutableSortedSet(StringComparer.Ordinal); + } + + /// + /// The reactive half of : ONE batched + /// over a bounded, KNOWN set of node paths, answering + /// which of them the mesh holds. + /// + /// 🚨 null on a fault or a missing adapter, never an empty set — the same rule + /// + /// keeps. An empty set would say "the mesh holds none of them", which on a Postgres read that + /// faulted (a satellite table that does not exist answers 42P01, not "absent") would make + /// an install re-fetch every file it ships, every time. + /// + /// The storage adapter; null yields null. + /// Serializer options for the read. + /// The paths to look for. Empty yields an empty OBSERVATION — there was + /// nothing to ask, which is a real answer and not a failed read. + /// A cold observable emitting exactly once. Subscribe to run. + public static IObservable?> ObservePresent( + IStorageAdapter? persistence, + JsonSerializerOptions options, + IReadOnlyCollection nodePaths) + { + ArgumentNullException.ThrowIfNull(nodePaths); + if (persistence is null) + return Observable.Return?>(null); + if (nodePaths.Count == 0) + return Observable.Return?>( + ImmutableHashSet.Empty.WithComparer(StringComparer.Ordinal)); + return persistence.ReadMany(nodePaths, options) + .Select(n => n.Path) + .ToList() + .Select(paths => (IReadOnlySet?)paths.ToImmutableHashSet(StringComparer.Ordinal)) + .Catch?, Exception>(_ => + Observable.Return?>(null)); + } + /// /// The verdict, computed purely — no mesh, no hub, so the falsification tests can drive every /// arm offline. diff --git a/test/Memex.Portal.Shared.Test/InstallCompletenessTest.cs b/test/Memex.Portal.Shared.Test/InstallCompletenessTest.cs index 596ed88892..769371bea6 100644 --- a/test/Memex.Portal.Shared.Test/InstallCompletenessTest.cs +++ b/test/Memex.Portal.Shared.Test/InstallCompletenessTest.cs @@ -1195,4 +1195,254 @@ public IObservable> ListPackages(string gitRef) = public IObservable> FetchPackageFiles( PackageManifest package, string gitRef) => Observable.Return(files); } + + // ── #4259: THE UPDATE PATH, which #3485 did not reach ─────────────────────────────────────── + + private const string Incr = "IncrementalPkg"; + private const string IncrHashV1 = "1111111111111111"; + private const string IncrHashV2 = "2222222222222222"; + private const string IncrGuidePath = $"{Incr}/Guide"; + private const string IncrOtherPath = $"{Incr}/Other"; + + private static PackageManifest IncrCandidate(string moduleVersion) => new() + { + Id = Incr, + Name = Incr, + Kind = PackageKind.NodeRepo, + TargetPartition = Incr, + SourceFolder = Incr, + Version = "1.0.0", + ModuleVersion = moduleVersion, + }; + + /// + /// The package at one generation. / + /// are the ONLY things that move between the two generations — Guide.md's hash is + /// deliberately identical in both locks, which is the whole premise: the diff will not name it, + /// so only a mesh observation can. + /// + private static IReadOnlyList IncrFiles( + string moduleVersion, string otherHash, string otherBody) => + [ + new PackageFile($"{Incr}/{ModuleManifest.FileName}", $$""" + { + "module": "{{Incr}}", + "moduleVersion": "{{moduleVersion}}", + "version": "1.0.0", + "files": { + "{{Incr}}/index.json": "aaa", + "{{Incr}}/Guide.md": "bbb", + "{{Incr}}/Other.md": "{{otherHash}}" + } + } + """), + new PackageFile($"{Incr}/index.json", $$""" + { + "id": "{{Incr}}", + "path": "{{Incr}}", + "nodeType": "Space", + "name": "Incremental package", + "state": "Active" + } + """), + new PackageFile($"{Incr}/Guide.md", "# Guide\n\nThe node this test deletes. Its hash never " + + "moves, so the manifest diff never names it."), + new PackageFile($"{Incr}/Other.md", otherBody), + ]; + + /// + /// 🚨 THE REGRESSION (MeshWeaver#4259). Install, delete one declared node, then run an + /// UPDATE — the module hash MOVED, and the deleted node's own file did not. The node must come + /// back. + /// + /// Fails on main. There, IncrementalUpdate fetches exactly + /// newManifest.DiffFrom(record.InstalledFiles) — a comparison of the candidate's lock + /// against the record the installer itself stamped. Guide.md's hash is identical across + /// the two locks, so it is not in the delta, is never fetched, and never reaches + /// DecideAndWrite — which would have written it, because it writes whenever + /// current is null. The update reports success and the node stays gone. + /// + /// Why this is not covered by . + /// #3485's heal lives on the module-hash-EQUAL exit. An UPDATE never takes that exit, and every + /// new publication moves the hash again — so a package that loses a node can stay short across + /// arbitrarily many updates, each one green. That is memex.meshweaver.cloud's + /// Plugins/Hosting on 2026-09-13T22:03Z: the update wrote the two files whose content had + /// moved and left eleven declared-and-absent ones absent. + /// + /// The changed file is the CONTROL in both directions: it proves the update really ran + /// (so a green result cannot come from an install that did nothing) and that the repair did not + /// work by rewriting the whole package. + /// + [Fact(Timeout = 300_000)] + public async Task AnIncrementalUpdateOverAMissingNode_RestoresIt() + { + var meshService = Mesh.ServiceProvider.GetRequiredService(); + var logger = Mesh.ServiceProvider.GetRequiredService() + .CreateLogger(); + + await CatalogLayoutAreas.InstallOrUpdate( + Mesh, new IncrSource(IncrFiles(IncrHashV1, "ccc", "# Other\n\nGeneration one.")), + "HEAD", IncrCandidate(IncrHashV1), logger) + .Should().Within(180.Seconds()) + .Emit("the first install must land before anything about an update can be measured"); + + (await WaitForNode(IncrGuidePath, present: true)).Should().BeTrue( + "the Guide node is the subject — if the first install never wrote it, the assertion " + + "below would pass for the wrong reason"); + (await WaitForNode(IncrOtherPath, present: true)).Should().BeTrue( + "and the control node has to exist before it can be shown to change"); + + var record = await ReadRecord(Incr); + record.Should().NotBeNull("the installer stamps an install record"); + record!.ModuleVersion.Should().Be(IncrHashV1); + record.InstalledFiles.Should().NotBeNull().And.ContainKey($"{Incr}/Guide.md", + "the record's file map is the incremental path's baseline — with no map the installer " + + "falls back to a FULL install, which repairs by accident and would make this test " + + "vacuous"); + + // ── The loss. + await meshService.DeleteNode(IncrGuidePath) + .Should().Within(60.Seconds()).Emit("the deletion is this test's precondition"); + (await WaitForNode(IncrGuidePath, present: false)).Should().BeTrue( + "the node must actually be gone before the update runs"); + + // ── The UPDATE: module hash moves, Other.md's content moves, Guide.md's does NOT. + var second = await CatalogLayoutAreas.InstallOrUpdate( + Mesh, new IncrSource(IncrFiles(IncrHashV2, "ddd", "# Other\n\nGeneration TWO.")), + "HEAD", IncrCandidate(IncrHashV2), logger) + .Timeout(180.Seconds()) + .Await(); + + second.Written.Should().BeGreaterThan(0, + "the update changed a file, so it must have written something — a zero here would mean " + + "the incremental path never ran and this test is measuring the wrong exit"); + + (await WaitForNode(IncrGuidePath, present: true)).Should().BeTrue( + "THE assertion: an update must restore a declared node that is absent from the mesh, " + + "even though its own file hash did not move. On main it does not — the fetch set is " + + "the manifest diff, which is a statement about the SOURCE and the RECORD and never " + + "about the mesh (MeshWeaver#4259)"); + + (await WaitForNode(IncrOtherPath, present: true)).Should().BeTrue( + "the control: the node that was never deleted is still there, so the repair did not " + + "work by wiping and rewriting the partition"); + + var after = await ReadRecord(Incr); + after!.ModuleVersion.Should().Be(IncrHashV2, + "the update really did move the package forward — if the record still carried the old " + + "hash the install would have taken the up-to-date exit, whose heal is #3485's and not " + + "the one under test here"); + } + + /// + /// The pure arms of — every one pinnable with + /// no mesh, the same split DescribeLanding and NodeTypeBakeStatus.Classify keep. + /// + /// 🚨 The arm that matters most is the null observation. "The mesh was not read" + /// must yield EMPTY and must never be spelled like "nothing is missing" at the call site — an + /// unobserved mesh is not a clean one, and widening the fetch on a read that faulted would + /// re-fetch a whole package on every hiccup. + /// + [Fact] + public void FilesToRestore_NamesTheAbsentOnes_AndRefusesToGuessWhenTheMeshWasNotRead() + { + var parsers = Parsers(); + var declared = ImmutableSortedDictionary.Empty + .Add($"{Incr}/index.json", "aaa") + .Add($"{Incr}/Guide.md", "bbb") + .Add($"{Incr}/Other.md", "ccc") + .Add($"{Incr}/README.md", "ddd"); + var fetching = ImmutableHashSet.Create(StringComparer.Ordinal, $"{Incr}/Other.md"); + + // Everything present ⇒ nothing to add back. + var allPresent = ImmutableHashSet.Create( + StringComparer.Ordinal, Incr, IncrGuidePath, IncrOtherPath, $"{Incr}/README"); + InstallCompleteness.FilesToRestore(declared, fetching, allPresent, parsers) + .Should().BeEmpty("a complete mesh needs no repair — if this were non-empty the " + + "assertion below would prove nothing"); + + // Guide's node gone ⇒ its FILE comes back, and only it. + var guideGone = allPresent.Remove(IncrGuidePath); + InstallCompleteness.FilesToRestore(declared, fetching, guideGone, parsers) + .Should().Equal([$"{Incr}/Guide.md"], + "the absent node's file is what the fetch has to ask for — a node path cannot be " + + "fetched, and naming the wrong half is how a repair asks for something the source " + + "does not serve"); + + // A file already in the delta is never returned twice. + var otherGone = allPresent.Remove(IncrOtherPath); + InstallCompleteness.FilesToRestore(declared, fetching, otherGone, parsers) + .Should().BeEmpty("Other.md is already being fetched by the delta — returning it again " + + "would make the caller's union a lie about how much this repair added"); + + // 🚨 The read did not happen. EMPTY, and never confusable with "nothing is missing". + InstallCompleteness.FilesToRestore(declared, fetching, presentNodePaths: null, parsers) + .Should().BeEmpty("a null observation means the mesh was NOT read; widening the fetch " + + "from it would re-fetch a whole package every time a read hiccups"); + + // Nothing declared ⇒ nothing to restore, whatever the mesh says. + InstallCompleteness.FilesToRestore(null, fetching, guideGone, parsers) + .Should().BeEmpty(); + } + + /// + /// against the live mesh: it reports what is + /// there, it reports an absence as an absence, and with no storage adapter it answers + /// null — "not read" — rather than an empty set. + /// + [Fact(Timeout = 300_000)] + public async Task ObservePresent_ReadsTheMesh_AndSaysNullWhenItCannot() + { + var meshService = Mesh.ServiceProvider.GetRequiredService(); + var logger = Mesh.ServiceProvider.GetRequiredService() + .CreateLogger(); + var persistence = Mesh.ServiceProvider.GetRequiredService(); + + await CatalogLayoutAreas.InstallOrUpdate( + Mesh, new FixedSource(Files()), "HEAD", Candidate(), logger) + .Should().Within(180.Seconds()).Emit("the install is the precondition"); + (await WaitForNode(GuidePath, present: true)).Should().BeTrue(); + + var paths = new[] { GuidePath, OtherPath }; + var present = await InstallCompleteness + .ObservePresent(persistence, Mesh.JsonSerializerOptions, paths) + .Timeout(60.Seconds()).Await(); + present.Should().NotBeNull("the adapter is registered and the paths exist, so this is a " + + "real observation and not a failed read"); + present!.Should().Contain(GuidePath).And.Contain(OtherPath); + + await meshService.DeleteNode(GuidePath).Should().Within(60.Seconds()).Emit("precondition"); + (await WaitForNode(GuidePath, present: false)).Should().BeTrue(); + + var afterLoss = await Observable.Interval(TimeSpan.FromMilliseconds(100)).StartWith(0L) + .SelectMany(_ => InstallCompleteness + .ObservePresent(persistence, Mesh.JsonSerializerOptions, paths)) + .Where(p => p is not null && !p.Contains(GuidePath)) + .FirstAsync().Timeout(120.Seconds()).Await(); + afterLoss!.Should().Contain(OtherPath, + "the control: the surviving node is still observed, so the absence above is about " + + "Guide and not about a read that stopped working"); + + // 🚨 No adapter ⇒ null, NEVER an empty set. An empty set says "the mesh holds none of + // them", which would make every declared file look absent and re-fetch the package. + (await InstallCompleteness + .ObservePresent(null, Mesh.JsonSerializerOptions, paths) + .Timeout(30.Seconds()).Await()) + .Should().BeNull("'there is no adapter' and 'the mesh holds nothing' are different " + + "facts, and spelling them alike is what this whole type exists to prevent"); + } + + /// + /// A package source serving one fixed generation — the same shape as + /// , but listing the INCREMENTAL package so a listing read cannot + /// silently answer about the other fixture's package. + /// + private sealed class IncrSource(IReadOnlyList files) : IPackageSource + { + public IObservable> ListPackages(string gitRef) => + Observable.Return>([IncrCandidate(IncrHashV1)]); + + public IObservable> FetchPackageFiles( + PackageManifest package, string gitRef) => Observable.Return(files); + } } From 2c3e46a4cb766932e22cf83ff35cc961837ed66e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20B=C3=BCrgi?= <6334612+rbuergi@users.noreply.github.com> Date: Mon, 14 Sep 2026 09:12:17 +0200 Subject: [PATCH 2/2] fix(#4259): a fetch shortfall fails the update instead of stamping a false record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Answers the three review findings on #4277 and the ratchet red. 1. 🚨 A fetch shortfall now FAILS the incremental update (Copilot). InstallNodeRepoDelta stamps newManifest as the next InstalledFiles baseline unconditionally, so a file that never travelled — while its OLD node is still present, the ordinary case — left VerifyLanded seeing a present node and reporting completeness. The record then claimed the NEW hash over OLD content and every later update of that module diffed clean and skipped it permanently. ReportFetchShortfall only logged; it is now EnsureFetchComplete and throws "full install required", the same mechanism the absent-shared-source arm already uses, so the caller falls back to a full install and the user still gets the package. The earlier "reports only, an install that ran is a fact" reasoning is corrected in the XML doc: the failure was never "landed short", it was "landed short and then lied about it". The shortfall is also no longer computed only when a logger exists — that made the guard vanish exactly where diagnostics are off. 2. ObservePresent's ReadMany is wrapped in Observable.Defer (Copilot). It was a method CALL evaluated before the Rx chain existed, so an adapter throwing SYNCHRONOUSLY escaped past the null/Warning path and forced a full re-fetch on every such fault. 3. The second full-manifest probe is documented at the call site rather than removed (Copilot). The preflight and VerifyLanded answer different questions on either side of the write; the added cost is one ReadMany over declaredNodePaths. Ratchet: the one 30.Seconds() literal this branch added is now TestTimeouts.Convergence. TestTimeoutLiteralRatchetGuard passes (3/3). The guard is deliberately narrow — it catches only the copied 30 s convention — so the 60/120/180 s bounds here are untouched. EnsureFetchComplete is internal (InternalsVisibleTo already covers this test assembly) so the guard is reachable: three tests pin throw-on-shortfall, no-throw-when-complete, and detection-without-a-logger. Negative control run: with the throw disabled, 2 of the 3 fail and the no-throw control still passes; restored, 3/3 green. Builds: MeshWeaver.PluginCatalog and Memex.Portal.Shared.Test, Release -warnaserror, 0 Error(s) each. Co-Authored-By: Claude Opus 5 (1M context) --- .../CatalogLayoutAreas.cs | 56 ++++++++++++--- .../InstallCompleteness.cs | 7 +- .../InstallCompletenessTest.cs | 70 ++++++++++++++++++- 3 files changed, 122 insertions(+), 11 deletions(-) diff --git a/src/MeshWeaver.PluginCatalog/CatalogLayoutAreas.cs b/src/MeshWeaver.PluginCatalog/CatalogLayoutAreas.cs index 7e3b6209be..82f1f1d696 100644 --- a/src/MeshWeaver.PluginCatalog/CatalogLayoutAreas.cs +++ b/src/MeshWeaver.PluginCatalog/CatalogLayoutAreas.cs @@ -1371,6 +1371,16 @@ private static IObservable IncrementalUpdate( .Select(p => p!) .ToImmutableSortedSet(StringComparer.Ordinal); + // 🚨 COST, stated rather than hidden: this preflight is a SECOND full-manifest + // ReadMany per incremental update — VerifyLanded still runs its own after + // InstallNodeRepoDelta. The two are not redundant and neither can be dropped: this + // one asks "which declared nodes are ABSENT so the delta must re-fetch them" + // BEFORE the write (the whole of #4259), and VerifyLanded asks "did what we just + // wrote land" AFTER it. Collapsing them would mean either re-fetching on a verdict + // computed before the install, or discovering a lost node only on the NEXT update. + // The added cost is one ReadMany over declaredNodePaths — bounded by the manifest, + // not by mesh size — and it is paid only on the incremental path, which exists to + // avoid re-fetching whole packages. return InstallCompleteness .ObservePresent( hub.ServiceProvider.GetService(), @@ -1419,7 +1429,7 @@ private static IObservable IncrementalUpdate( return (wanted.Count == 0 ? Observable.Return((IReadOnlyList)[]) : source.FetchPackageFiles(pkg, sourceRef, wanted)) - .Do(fetched => ReportFetchShortfall(pkg.Id, wanted, fetched, logger)) + .Do(fetched => EnsureFetchComplete(pkg.Id, wanted, fetched, logger)) .SelectMany(changedFiles => PackageInstaller.InstallNodeRepoDelta( hub, pkg, newManifest, changedFiles, removedNodePaths, sourceRef, logger, authorizingUserId)); @@ -1438,31 +1448,59 @@ private static IObservable IncrementalUpdate( /// and WriteInstalledRecord then stamps the FULL declared map — a record asserting a file /// nothing ever wrote. /// - /// Reports only; it never fails the update. The install that did run is a fact, and - /// collapsing "landed short" into "failed" would make a working update a new way to break — - /// carries the outcome verdict on this same exit. + /// 🚨 It fails the incremental update rather than reporting it, and the earlier + /// "reports only" reading of this method was wrong. The objection to failing — that the install + /// which did run is a fact, and collapsing "landed short" into "failed" makes a working update a + /// new way to break — does not survive what happens NEXT: InstallNodeRepoDelta stamps + /// newManifest as the next InstalledFiles baseline unconditionally, so a file that + /// never travelled while its OLD node is still present leaves seeing + /// a present node and reporting completeness. The record then claims the NEW hash over OLD + /// content, and every later update of that module diffs clean and skips it — permanently. The + /// failure is not "landed short", it is "landed short and then lied about it". + /// + /// Nor does throwing cost the user the update. This is the same mechanism the + /// absent-shared-source arm uses a few lines above: the caller catches it and falls back to a + /// FULL install, which rewrites everything and writes a record that is true. The user gets an + /// updated package either way; only the silent-stale path is removed. /// /// The package being updated. /// The paths the fetch asked for. /// What came back. /// Where the shortfall is named. - private static void ReportFetchShortfall( + // Internal for the FetchShortfallFailsClosedTest pin (InternalsVisibleTo): a guard that only + // the incremental-update path can reach is a guard nothing can falsify. + internal static void EnsureFetchComplete( string packageId, IReadOnlySet wanted, IReadOnlyList fetched, ILogger? logger) { - if (logger is null || wanted.Count == 0) + // 🚨 NOT gated on the logger. Computing the shortfall only when someone is listening makes + // the guard disappear exactly where diagnostics are off, which is where a silent stale + // record is least likely to be noticed. + if (wanted.Count == 0) return; var missing = wanted .Except(fetched.Select(f => f.RelativePath), StringComparer.Ordinal) .ToImmutableSortedSet(StringComparer.Ordinal); if (missing.Count == 0) return; - logger.LogError( + logger?.LogError( "Updating {Id} incrementally: the source returned {Fetched} of the {Wanted} file(s) " - + "asked for — [{Missing}] did NOT travel, so their nodes cannot be written, and the " - + "install record will still declare them (MeshWeaver#4259).", + + "asked for — [{Missing}] did NOT travel, so their nodes cannot be written. Falling " + + "back to a full install rather than stamping a record that declares them " + + "(MeshWeaver#4259).", packageId, fetched.Count, wanted.Count, string.Join(", ", missing.Take(InstallCompleteness.MaxNamedInALine))); + // 🚨 THROW, do not merely report. InstallNodeRepoDelta stamps `newManifest` as the next + // InstalledFiles baseline unconditionally. If a requested file is missing while its OLD + // node is still present, VerifyLanded sees a present node and reports completeness — so + // the record would claim the new hash over old content, and the next update of this module + // would diff clean and skip it FOREVER. A shortfall must therefore never reach the write. + // The same mechanism the absent-shared-source arm above uses: the caller catches this and + // falls back to a full install, whose release-all rewrites everything. + throw new InvalidOperationException( + $"Package '{packageId}' asked its source for {wanted.Count} file(s) and received " + + $"{fetched.Count} ([{string.Join(", ", missing.Take(InstallCompleteness.MaxNamedInALine))}] " + + "did not travel); full install required."); } private static ILogger? Logger(LayoutAreaHost host) => diff --git a/src/MeshWeaver.PluginCatalog/InstallCompleteness.cs b/src/MeshWeaver.PluginCatalog/InstallCompleteness.cs index 9ad0e612d9..aea6c0a40d 100644 --- a/src/MeshWeaver.PluginCatalog/InstallCompleteness.cs +++ b/src/MeshWeaver.PluginCatalog/InstallCompleteness.cs @@ -180,7 +180,12 @@ public static ImmutableSortedSet FilesToRestore( if (nodePaths.Count == 0) return Observable.Return?>( ImmutableHashSet.Empty.WithComparer(StringComparer.Ordinal)); - return persistence.ReadMany(nodePaths, options) + // 🚨 Defer is load-bearing, not style. ReadMany is a method CALL: without Defer it runs + // when this method is called, which is before the Catch below exists — so an adapter that + // throws SYNCHRONOUSLY escapes past the null/Warning path and the caller falls back to a + // full package re-fetch on every such fault. Inside Defer, a synchronous throw and an + // asynchronous OnError reach the same conservative outcome. + return Observable.Defer(() => persistence.ReadMany(nodePaths, options)) .Select(n => n.Path) .ToList() .Select(paths => (IReadOnlySet?)paths.ToImmutableHashSet(StringComparer.Ordinal)) diff --git a/test/Memex.Portal.Shared.Test/InstallCompletenessTest.cs b/test/Memex.Portal.Shared.Test/InstallCompletenessTest.cs index 769371bea6..d30db01cd9 100644 --- a/test/Memex.Portal.Shared.Test/InstallCompletenessTest.cs +++ b/test/Memex.Portal.Shared.Test/InstallCompletenessTest.cs @@ -1427,7 +1427,7 @@ await CatalogLayoutAreas.InstallOrUpdate( // them", which would make every declared file look absent and re-fetch the package. (await InstallCompleteness .ObservePresent(null, Mesh.JsonSerializerOptions, paths) - .Timeout(30.Seconds()).Await()) + .Timeout(TestTimeouts.Convergence).Await()) .Should().BeNull("'there is no adapter' and 'the mesh holds nothing' are different " + "facts, and spelling them alike is what this whole type exists to prevent"); } @@ -1445,4 +1445,72 @@ public IObservable> ListPackages(string gitRef) = public IObservable> FetchPackageFiles( PackageManifest package, string gitRef) => Observable.Return(files); } + + // ══════════════════════════════════════════════════════════════════════════ + // A fetch shortfall must never reach the record + // ══════════════════════════════════════════════════════════════════════════ + + /// + /// 🚨 The shortfall FAILS the incremental update; it does not merely report it. + /// InstallNodeRepoDelta stamps the new manifest as the next baseline unconditionally, so + /// a file that never travelled — while its OLD node is still present, which is the ordinary + /// case — would leave the record claiming the NEW hash over OLD content and every later update + /// of that module diffing clean and skipping it forever. The caller turns this throw into a + /// full install, so the user still gets the package. + /// + [Fact(Timeout = 60_000)] + public void AFileThatDidNotTravel_FailsTheIncrementalUpdate_RatherThanStampingTheRecord() + { + var wanted = new[] { "Guide/index.json", "Guide/Overview.md" } + .ToImmutableHashSet(StringComparer.Ordinal); + var fetched = new List { new("Guide/index.json", "{}") }; + + var act = () => CatalogLayoutAreas.EnsureFetchComplete("Acme.Guide", wanted, fetched, null); + + act.Should().Throw( + "a requested file that did not travel must not reach WriteInstalledRecord — the " + + "record would declare content nothing ever wrote, and the next update would " + + "diff clean and skip it permanently") + .WithMessage("*full install required*") + .And.WithMessage("*Guide/Overview.md*", + "the operator has to be told WHICH file did not travel; a shortfall that names " + + "nothing sends them to the whole package"); + } + + /// + /// The control for the case above: when everything asked for arrives, the incremental update + /// proceeds. Without this, a guard that threw unconditionally would pass the test above and + /// turn every update into a full install. + /// + [Fact(Timeout = 60_000)] + public void AFetchThatReturnedEverythingAskedFor_LetsTheIncrementalUpdateProceed() + { + var wanted = new[] { "Guide/index.json" }.ToImmutableHashSet(StringComparer.Ordinal); + var fetched = new List { new("Guide/index.json", "{}") }; + + var act = () => CatalogLayoutAreas.EnsureFetchComplete("Acme.Guide", wanted, fetched, null); + + act.Should().NotThrow( + "the incremental path is the fast path; it must stay available when the source served " + + "every file that was asked for"); + } + + /// + /// 🚨 The shortfall is computed with NO logger. Gating the detection on a logger existing would + /// make the guard vanish exactly where diagnostics are off — which is where a silently stale + /// install record is least likely to be noticed. + /// + [Fact(Timeout = 60_000)] + public void TheShortfallIsDetected_EvenWithNoLogger() + { + var wanted = new[] { "Guide/index.json", "Guide/Gone.md" } + .ToImmutableHashSet(StringComparer.Ordinal); + var fetched = new List { new("Guide/index.json", "{}") }; + + var act = () => CatalogLayoutAreas.EnsureFetchComplete("Acme.Guide", wanted, fetched, logger: null); + + act.Should().Throw( + "detection must not depend on someone listening"); + } + }