Repository navigation
fix(#4259): an UPDATE restores a declared node that is absent from the mesh - #4277
Conversation
…e mesh `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) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
A critical baseline-stamping issue and several moderate review findings remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR repairs incremental plugin updates by detecting and restoring declared mesh nodes that went missing.
Changes:
- Adds batched presence checks and restoration logic.
- Reports package-source fetch shortfalls.
- Adds regression tests and installation documentation.
File summaries
| File | Review findings |
|---|---|
test/Memex.Portal.Shared.Test/InstallCompletenessTest.cs |
Moderate (1 vote): the test does not prove the incremental narrow fetch path; capture and assert requested paths. |
src/MeshWeaver.PluginCatalog/InstallCompleteness.cs |
Moderate (2 votes): synchronous ReadMany failures can bypass Catch; wrap the call in Observable.Defer. |
src/MeshWeaver.PluginCatalog/CatalogLayoutAreas.cs |
Critical (1 vote): fetch shortfalls can still advance the install baseline and preserve stale content. Moderate findings cover duplicated full-manifest reads, missing incremental shortfall coverage, and untested shared Source/Test fallback. Nits cover file/node count units and content-asset diagnostics. |
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-14-an-update-restores-content-that-went-missing.md |
No review comments. |
src/MeshWeaver.Documentation/Data/Architecture/NodeTypeCompilation.md |
No review comments. |
src/MeshWeaver.Documentation/Data/Architecture/DeclaredIsNotLanded.md |
Nit (1 vote): 2026-09-14T06:0xZ is not a valid ISO-8601 timestamp. |
src/MeshWeaver.Documentation/Data/Architecture.md |
No review comments. |
Review details
Suppressed comments (6)
src/MeshWeaver.Documentation/Data/Architecture/DeclaredIsNotLanded.md:44
2026-09-14T06:0xZis not a valid ISO-8601 timestamp, so readers cannot tell whether thexis an intentional redaction or a typo. Use a valid timestamp, or state the date/time was intentionally omitted.
`Plugins/Hosting` on **memex.meshweaver.cloud**, read 2026-09-14T06:0xZ. All reads through `get` /
`search`; nothing was mutated.
src/MeshWeaver.PluginCatalog/CatalogLayoutAreas.cs:1422
- The existing shortfall test exercises a full install, not this new incremental
FetchPackageFiles(..., wanted)branch. A regression that removes or miscomputesReportFetchShortfallcould therefore still pass the suite; add an incremental source that omits one requested path and assert that the Error log names that path.
: source.FetchPackageFiles(pkg, sourceRef, wanted))
.Do(fetched => ReportFetchShortfall(pkg.Id, wanted, fetched, logger))
src/MeshWeaver.PluginCatalog/CatalogLayoutAreas.cs:1408
restore.Countcounts manifest files, whiledeclaredNodePaths.Countcounts distinct node paths and the message labels both as nodes.NodeFileMapperdeliberately maps bothA.jsonandA/index.jsonto the same node, so a missing node can produce a misleading line such as “2 of 1 declared node(s)”. Report these as files (or compute a distinct restored-node count) so the warning's numerator and denominator have the same units.
"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).",
src/MeshWeaver.PluginCatalog/CatalogLayoutAreas.cs:1463
wantedcontains every manifest file, includingcontent/**assets and other non-node files, whileNodePathForFileexplicitly excludes those. If a content asset is omitted by the source, this new error says its node cannot be written even though no node is expected, which misdiagnoses the operational failure. Make the message type-neutral or report node and non-node shortfalls separately.
"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).",
src/MeshWeaver.PluginCatalog/CatalogLayoutAreas.cs:1394
- The new missing-node path deliberately falls back to a full install when an absent file is under the package-level
Source/orTest/prefix, because an incremental owner release would leave sibling NodeTypes stale. The added tests cover ordinary missing-node restoration and the pure helper arms, but none exercises this branch or verifies that a sibling is recompiled; a future change could remove this safety rule while the current regression test still passes.
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.");
test/Memex.Portal.Shared.Test/InstallCompletenessTest.cs:1318
Written.Should().BeGreaterThan(0)does not prove that the incremental path or its narrow fetch set was used: a regression that falls back toFull()can produce the same result, andIncrSourcedoes not record the requested paths. Since this test claims to protect hash-based fetch efficiency, make the source capture the secondpathsargument and assert that the update requested only the changed file plus the missingGuide.md, not the whole package.
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");
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| : source.FetchPackageFiles(pkg, sourceRef, wanted)) | ||
| .Do(fetched => ReportFetchShortfall(pkg.Id, wanted, fetched, logger)) | ||
| .SelectMany(changedFiles => PackageInstaller.InstallNodeRepoDelta( |
| return InstallCompleteness | ||
| .ObservePresent( | ||
| hub.ServiceProvider.GetService<IStorageAdapter>(), | ||
| hub.JsonSerializerOptions, declaredNodePaths) |
| if (nodePaths.Count == 0) | ||
| return Observable.Return<IReadOnlySet<string>?>( | ||
| ImmutableHashSet<string>.Empty.WithComparer(StringComparer.Ordinal)); | ||
| return persistence.ReadMany(nodePaths, options) |
Pull request was converted to draft
Held as draft — author terminated, CI red, three review findings standingI have converted this to draft, which disabled auto-merge ( Why it is held. This PR was armed with 1. CI red — one failing test, and it is a rule this repo enforces deliberately
This diff added hand-written 2. Three Copilot findings, none answered
The first one is worth restating: it is the same shape as the defect this PR exists to fix (#4259 — "three declared source nodes never landed and the install logged nothing"). Shipping it as-is would replace a silent miss with a silent miss that is also durable. ResumingThe branch is 🤖 Generated with Claude Code |
…false record 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) <noreply@anthropic.com>
All three findings answered, ratchet fixed, undraftingPicked this up after its author was terminated by the rate limit. 1. The install baseline advancing through a fetch shortfall — fixed, and you were rightThis was the real one. It is now I also corrected the XML doc, which argued the opposite: "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." That reasoning does not survive what happens next — the failure was never "landed short", it was "landed short and then lied about it", and the fallback means nobody loses an update. Second defect found while there: the shortfall was computed only 2.
|
An UPDATE never looked at the mesh — so a lost node survived every update, each reporting success
Closes the last open half of #4259.
The defect
CatalogLayoutAreas.InstallOrUpdatehas three exits. Two of them observe the mesh; the third did not.InstallCompleteness.Observe+SkipOrHealDecideAndWritewrites whenevercurrent is nullnewManifest.DiffFrom(record.InstalledFiles)The incremental fetch set was a comparison of two declarations: the candidate's
manifest.lockagainst the record the installer itself stamped. A node lost after a previous install has an
unchanged file hash, so it never entered
delta.AddedOrChangedFiles, was never fetched, and neverreached
DecideAndWrite— the one place that would have restored it. The presence-awareness sat onelayer 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.
The measured case —
Plugins/Hostingon memex.meshweaver.cloudRead 2026-09-14T06:0xZ, read-only (
get/search); portal at3.0.0+c84c6c05503228860df03c4a8b596e497e6d218c.The files were there and they compiled.
Hosting/TriageStatus/Release/20260913081235-bylQeIj_(created on this portal 2026-09-13T08:12:35.446Z,
status: Succeeded) names its inputs insourceVersions:Hosting/Deployment/Source/TriageIntake,Hosting/TriageStatus/Source/TriageStatusContent,…/TriageStatusLayoutAreas(ticks =2026-09-13T08:12:26Z), plus two
Test/nodes. A release records the source nodes a successfulcompile actually read.
The update wrote only what had changed. Record stamped 2026-09-13T22:03:03.221Z,
installedNodeCount: 224. Exactly two nodes carry that install's timestamps:Hosting/Deployment/Source/AksOpsResultHosting/Deployment/Source/PlatformBuildInboxWatcherEleven declared files have no node. Anchored probes, each with a same-anchor positive control:
namespace:Hosting/Deployment/Source scope:subtree nodeType:Codetruncated: false— noTriageIntake; the 14 are the controlnamespace:Hosting/Deployment/Test scope:subtree nodeType:CodeTriageIntakeTestsnamespace:Hosting/TriageStatus scope:subtreenamespace:Hosting/TriageItem scope:subtreenamespace:Hosting scope:children nodeType:NodeTypetruncated: false— the denominatornamespace:Hosting scope:children nodeType:NodeType content.compilationStatus:ErrorAll eleven are in the record's own
installedFilesmap. Control instance, same probes:0 of 19 at Error, and its
Hosting/Deployment/Sourceholds 15 includingTriageIntake—same package, same declaration, one instance short.
The fix
IncrementalUpdateasks the mesh before it fetches:PackageInstaller.NodePathForFile—the installer's own file→node rule, not a second implementation of part of it.
InstallCompleteness.ObservePresent— one batchedIStorageAdapter.ReadManyover thatbounded, known set. Never a query (a stale negative would re-fetch a present file) and never N
point reads of possibly-absent paths (which opens a storm breaker on the owning hub).
InstallCompleteness.FilesToRestore— pure, so every arm is pinnable offline — returns thedeclared files the delta is not already fetching whose node is absent.
delta.AddedOrChangedFiles ∪ restore.One batched read per incremental update — the same read the 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.
ObservePresentreturnsnullwhen the mesh was NOT read (noadapter, or a faulted batched read) and never an empty set — an empty set would say "the mesh holds
none of them", which on a Postgres read that faulted (
42P01, not "absent") would re-fetch everyfile the package ships, every time. That
nullyields an empty restore set and the caller says soat Warning, spelled so it cannot be read as a pass.
A restored SHARED
Source//Test/file falls back to a full install, by the same rule theexisting changed-file guard applies and for the same reason: those are compile inputs for every type
in the package, so the delta's owner-derivation would recompile only the nominal owner.
And the other silent hole in the same method.
IPackageSource.FetchPackageFiles(…, paths)filters, 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 vanished with no exception and no 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 named at Error. It reports only; it never fails
the update.
Verification
AnIncrementalUpdateOverAMissingNode_RestoresItinstalls a package, deletes one node, then runs anupdate in which the module hash moves and the deleted node's own file hash does not.
CatalogLayoutAreas.cschange and rebuilding: FAILED in 2 m (theWaitForNode(present: true)timeout — the node stays gone)Memex.Portal.Shared.Test.InstallCompletenessTest— 21 of 21 passed, 27 s. The class held 18[Fact]s at the branch base and holds 21 here: the behavioural repro plus two arms pinningFilesToRestore(every branch, offline) andObservePresent(live read, absence, and theno-adapter
nullthat must never be an empty set).MeshWeaver.Documentation.Testlink integrity / topic map / conflict markers — 6 of 6 passed.dotnet build -c Release -warnaserror, one project per invocation:MeshWeaver.PluginCatalog0 Error(s) 0 Warning(s),
MeshWeaver.Documentation0/0,Memex.Portal.Shared.Test0/0.Work products conserved
the three exits, the measured case, the batched presence read, and the sweep hazard:
Hosting/TriageItemreadscompilationStatus: Okwhile all six of itscompiledSourcesareabsent, because it has not been re-driven since its last successful compile. So
content.compilationStatus:Erroris a sweep for types that attempted a compile and failed —"8 of 19 report Error" understates the damage, and the cross-check that sees it is
compiledSources(or a release node'ssourceVersions) against the mesh.shared=@…decision that Two sample NodeTypes fail on every boot of memex-cloud because of the samples MIRROR, not their source — a pre-rename Type/article leftover and Product's unrebased shared=@Northwind refs #4174 deliberately left open:@is absolute and never rebases (CodeQueryResolver.Expand), because the dominant use is agenuinely cross-partition include; the consequence is that a tree carrying an absolute
cross-reference is not importable under a prefix, and the remedy is at the install boundary
(install the package at the root its sources were authored for), not in the resolver.
Category: FixWhat's New entry.What this does NOT do
node proves they were written and compiled; no instrument a session can run names what removed
them. This change makes the loss self-correcting on the next update instead of permanent.
stay two steps on an image without it.
Refs #4259, #4257, #3485, #4208, #4174
🤖 Generated with Claude Code