fix(nodetype): the collision overlay's self-heal watcher stops re-emitting the #2231 fingerprint by rbuergi · Pull Request #5308 · Systemorph/MeshWeaver

rbuergi · 2026-09-22T19:22:55Z

Refs #5264 · refs #2231 · follows #5008

What was still wrong on `main`

#5008 made an instance naming an OCCUPIED NodeType path (the Store plugin root at Feedback, declaration at Feedback/Feedback) a terminal refusal with one Error naming both sides, and took the logger off the predicates on that route because a non-declaration occupant is their normal input. It missed one caller: ArmOverlaySelfHeal, which both refusals (ProbeCollision's branch and NonDeclarationOverlay) arm with a null gate over the occupant's stream. Its Evaluate called ContentAs<NodeTypeDefinition>(jsonOptions, logger) on every emission, so the plugin root produced As<NodeTypeDefinition> for Feedback could not recover value: JsonException at Error once per emission for the watcher's life. That is the second and third line of #5264's burst (17:48:06Z and 17:48:51Z) — the fingerprint that kept #2231 reopening.

The change

Evaluate first applies the same one-sided NodeTypeDeclarationProbe.IsProvablyNotADeclaration; a proven occupant is "not usable" without a log line. A real declaration, or degraded content that might be one, still goes through ContentAs with the logger, so a genuine conversion fault on a NodeType stays loud. No bound, no catch, no behaviour change to when the watcher recycles.

Controls

OverlaySelfHealWatcherTest.CollisionOverlay_OccupantEmissions_AreNotLoggedAsConversionFaults_AndARealDeclarationStillHeals:

unfixed watcher: failed, found 3 item(s) (one Error per occupant emission);
fixed: 14/14 in OverlaySelfHealWatcherTest; NodeTypeOccupiedPathActivationTest + NodeTypePathOccupancyTest 11/11.
the positive half: after the occupant emissions, a usable declaration still recycles exactly once.

Builds: MeshWeaver.Graph, MeshWeaver.Compiler.Pipeline.Test, MeshWeaver.Graph.Test — -c Release -warnaserror, 0 Warning(s), 0 Error(s).

What this does NOT do

It does not repair the stranded instance. rbuergi/Feedback/20260922-per-request-registrants-never-leave-the-hub (control instance) names the bare Feedback, and the overlay it gets is the correct answer. The repair is a full-node update rewriting its nodeType to Feedback/Feedback — an owner act on that partition.
Not established: whether that instance was created on an image carrying Feedback NodeType resolution lands on the plugin node's PluginContent when the definition is absent from the tenant partition #5008's write-side refusal (merged 11:59Z on 2026-09-22; the control instance was on c55301f3, pre-Feedback NodeType resolution lands on the plugin node's PluginContent when the definition is absent from the tenant partition #5008, at 12:05Z, and I could not read it afterwards — it answers 503 from here right now). If it was created AFTER Feedback NodeType resolution lands on the plugin node's PluginContent when the definition is absent from the tenant partition #5008 was live, the write boundary has a gap and that is a separate defect.

Doc: Doc/Architecture/DanglingNodeTypes → The activation half (the missed caller, the control, what is not repaired).

Pairs-with: none — no public type or member removed; the change is inside an internal static method.
Implementers: none — no interface member added.
Mirror-sync: none — no localization key added or changed.

Recycle after deploy: none needed for the fix itself (it is framework code; a roll replaces every activation).

🤖 Generated with Claude Code

…tting the #2231 fingerprint (refs #5264) #5008 took the logger off every predicate on the occupied-path route because a non-declaration occupant is their NORMAL input — but missed ArmOverlaySelfHeal, which both refusals arm over the OCCUPANT's stream. Every emission of the Store plugin root at `Feedback` went through ContentAs<NodeTypeDefinition> WITH the logger, so each one logged `As<NodeTypeDefinition> for Feedback could not recover value: JsonException` at Error for the watcher's whole life — the line in #5264's burst 45 s after the collision line. The watcher now applies the same one-sided IsProvablyNotADeclaration first; a real declaration (or degraded content that might be one) still converts with the logger. Control: 3 Error lines against the unfixed watcher, 0 with the fix; the positive half proves it still recycles once a declaration lands. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Copilot

Copilot review overview

🟡 Changes recommended

Two moderate findings remain regarding duplicate conversion and missing failure-path test coverage.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

Avoid duplicate NodeTypeDefinition conversion per watcher emission · New

What changed in this PR

Updates collision-overlay self-healing to suppress conversion errors for occupied paths while preserving recovery for valid declarations.

Changes:

Skip conversion logging for proven non-declaration occupants.
Add regression coverage for logging suppression and recovery.
Document the activation behavior and repair scope.

File	Summary	Review findings
`test/MeshWeaver.Compiler.Pipeline.Test/OverlaySelfHealWatcherTest.cs`	Tests silent occupant emissions and successful healing.	Moderate (1 vote): add coverage for conversion failure on a real declaration. Nit (1 vote): use immutable accumulation instead of `ConcurrentQueue`.
`src/MeshWeaver.Graph/Configuration/NodeTypeEnrichmentHelpers.cs`	Filters occupied paths before conversion.	Moderate (3 votes): avoid converting each emission twice. Nit (1 vote): reuse the existing helper.
`src/MeshWeaver.Documentation/Data/Architecture/DanglingNodeTypes.md`	Documents the missed watcher path and remediation scope.	No final review comments.

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

rbuergi · 2026-09-22T19:31:41Z

+            if (t is not null && NodeTypeDeclarationProbe.IsProvablyNotADeclaration(t, jsonOptions))
+                return (null, false);
            var def = t?.ContentAs<NodeTypeDefinition>(jsonOptions, logger);


Right — fixed in a664c89. Evaluate no longer calls the probe: it splits on the probe's own first clause (the node DECLARES a NodeType other than NodeType) and converts exactly once, passing the logger only when the node does not declare another type. Outcomes are unchanged: a convertible occupant (untyped JSON) still yields its definition, an unconvertible one is silent (the probe's conviction, now without a second pass), and a real declaration still converts with the logger. OverlaySelfHealWatcherTest 14/14, builds clean under -warnaserror.

Half right, and the half that is wrong is the half that would have driven the fix. IsProvablyNotADeclaration does deserialize — its last clause is candidate.ContentAs<NodeTypeDefinition>(options) is null — so the concern is real in principle. But the predicate is a short-circuiting && chain, and the order decides who actually pays:

!string.IsNullOrEmpty(candidate.NodeType) && !string.Equals(candidate.NodeType, MeshNode.NodeTypePath, StringComparison.OrdinalIgnoreCase) && candidate.ContentAs<NodeTypeDefinition>(options) is null;

Case by case, which is what decides whether this is a hot-path regression:

candidate where it exits passes

a real NodeType declaration clause 2 — its NodeType is MeshNode.NodeTypePath 1 (Evaluate's own)

empty NodeType clause 1 1

degraded JSON / the Feedback occupant clause 3 returns null ⇒ proven ⇒ the new early return fires, and Evaluate never reaches its own ContentAs 1 (the probe's)

same-named foreign content that DOES deserialize into NodeTypeDefinition clause 3 returns non-null ⇒ predicate false ⇒ Evaluate converts again 2

So the review's two named examples point the wrong way. Degraded JSON is the case the early return short-circuits, not one that doubles — it exits at clause 3 with the conversion already done and never reaches the second call. And it is not "every candidate that is not proven to be an occupant": a genuine declaration never reaches clause 3 at all, so the watcher's actual steady-state input — the one this PR exists for — is unchanged at one pass.

The double conversion is confined to exactly one case: foreign content whose NodeType is set, is not the NodeType path, and whose payload nonetheless deserializes into a NodeTypeDefinition. That is worth tidying, and the suggested shape is the right one — have the probe hand back what it converted (TryGetDefinition(node, options, out var def)) so the predicate and the conversion are one pass by construction. But it is a narrow correctness-of-effort issue, not the "every long-lived watcher emission" regression described, and the "Deserialize ONCE per node" contract is not broken on any path this watcher normally sees.

Worth stating because it is the reason this PR exists: the early return is what stops As<NodeTypeDefinition> for Feedback could not recover value: JsonException being logged at Error once per emission for the watcher's life. An error logged before its classifier can never go quiet, so the fingerprint could not stop recurring however the instance was repaired. Keeping the logger-bearing ContentAs for everything that might really be a declaration preserves the loud path where it belongs.

🤖 Generated with Claude Code

github-actions · 2026-09-22T19:28:43Z

Test Results (shard 0)

1 files ±0 1 suites ±0 2m 40s ⏱️ -1s
320 tests ±0 320 ✅ ±0 0 💤 ±0 0 ❌ ±0
324 runs ±0 324 ✅ ±0 0 💤 ±0 0 ❌ ±0