Skip to content

fix: the NodeType overlay could never be delivered — two equal timeouts - #2992

Merged
github-actions[bot] merged 1 commit into
mainfrom
fix/nodetype-overlay-budget
Sep 1, 2026
Merged

github-actions[bot] merged 1 commit into
mainfrom
fix/nodetype-overlay-budget

Conversation

@rbuergi

@rbuergi rbuergi commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

The diagnostic that names the culprit has never once been delivered

NodeTypeEnrichmentHelpers already builds exactly the right message when an instance references a
NodeType that resolves to nothing:

NodeType 'X' is not registered (referenced by instance '…')

It is correct, it is specific, and nobody has ever seen it in production. Two bounds are equal:

NodeTypeEnrichmentHelpers.SlowPathTimeout 60 s — inner
MessageHubConfiguration.RequestTimeout 60 s — outer

An inner bound that cannot fire first cannot deliver anything. So every one of these activations
surfaced as the ceiling's generic "No response received in hub … for request SubscribeRequest",
which names neither the culprit nor the remedy — and, through NodeReadOutcome.FromReadFailure,
reaches an operator as "Unavailable: … this read reached no verdict".

This is the rule the codebase already writes down for itself, in ReadBudget:

"a bound nested inside another bound must be able to fire FIRST, because it is the only one that
knows WHICH read starved."

ReadBudget.Default (10 s) is documented as "comfortably inside the 60 s hub RequestTimeout so
this bound is the one that fires"
. SlowPathTimeout never got that margin, and ReadBudget's own
remarks cite three production incidents of this exact shape (#1563, #1693, #1748).

It has already been paid for once, from the test side. OrleansDynamicCompilationTest carries
this note about the identical collision:

"At 60s the Fact timeout exactly equalled NodeTypeEnrichmentHelpers.SlowPathTimeout and
MessageHubConfiguration.RequestTimeout, so it preempted the framework's own graceful sink … and
every failure of this class reported a bare 'Test execution timed out' with nothing to go on —
five sessions theorised from that silence."

That fix raised the test's budget. The collision underneath it was never addressed; this PR
addresses it.

Why lowering it costs nothing

SlowPathTimeout never bounded a legitimate compile. WaitForCompileSettled disarms it
(Observable.Never) for as long as a compile is genuinely in flight, and in-flight visibility is
InFlightOverlayGrace (5 s to a live progress overlay that self-heals onto the real hub). So the
constant governs exactly one case — nothing compiling and the type stream still unsettled, i.e. a
dangling or unreachable NodeType — where the whole remaining wait is dead time.

30 s restores the margin the surrounding design has always argued for and leaves a full 30 s for the
overlay to be built and delivered inside the caller's ceiling.

What's in this PR

  1. SlowPathTimeout 60 s → 30 s, with the invariant stated at the constant.
  2. MessageHubConfiguration.DefaultRequestTimeout is now public. It was a bare literal invisible
    outside its assembly, so every nested bound that had to stay under it hand-copied the number into
    a comment
    — which is precisely how it drifted into collision. Behaviour is unchanged; the
    relationship is now single-sourced and referenceable.
  3. NodeTypeOverlayBudgetTest pins the ordering invariant, the headroom, and
    InFlightOverlayGrace < SlowPathTimeout. Verified to fail on reintroduction:
    Expected 00:01:00 to be less than 00:01:00. A comment cannot fail; this can.
  4. Doc drift fixed at the same site. The XML doc argued "30 s leaves budget" directly above a
    constant of 60, and StaticNodeProviders.md said 60 s. A comment that contradicts its constant is
    how the next reader concludes the margin exists. (UnregisteredNodeTypeTest in the Plugins repo
    also says 30 s — that becomes correct again with this change, no edit needed.)
  5. NodeUpdatePipeline stops reporting a non-verdict as a verdict. It mapped both a real
    DeliveryFailureException and its own 10 s TimeoutException to "Node not found: {path}".
    The timeout case is exactly how a node whose NodeType no longer resolves fails — so the write path
    told callers an existing node was absent, and the only repair that reads as sensible from
    there is delete-and-recreate, on live content. This is the same lie the read path was fixed for
    (Three more sites report an availability failure as a definitive negative (permission-fold fault → "Access denied", read timeout → "Not found", cold cache → "user unknown") #974). Genuine not-found is byte-identical; only the timeout branch changes.

How this was found

Auditing two production meshes for instances whose nodeType resolves to nothing. One real
instance on memex.systemorph.com (rbuergi/_Draft/ClientA_EslProposalQA, nodeType: EmailDraft)
reads as Unavailable and never reaches a verdict. Chasing why that produced no diagnosis is what
surfaced this.

Three measurement traps worth recording, because each one produces a confident wrong answer:

  • search nodeType:NodeType cannot enumerate the valid type set. Most built-ins leave NodeType
    null and the AddMeshNodes seed bucket is suppressed under context:search. The rule actually
    enforced is FindStaticNode(nodeType) is not null || persistence.Exists(nodeType).
  • nodeType wildcards work in a positive filter but are silently ignored in negation —
    -nodeType:Edu/* does nothing.
  • get_diagnostics is not a registration oracle — it returns the same trivial Ok for any
    static type, healthy or not.

Deliberately NOT in this PR — two holes that need a decision, not an implementation

Both are real and both have a real counterparty, so each is a maintainer call rather than a patch:

A. update accepts a nodeType that does not exist. Create validates it
(MeshExtensions.cs:756-786, NodeCreationRejectionReason.InvalidNodeType); update does not —
UpdateAccordingToSourceNode copies NodeType through unvalidated, and no registered
INodeValidator ever produces InvalidNodeType for NodeOperation.Update. So update is a supported
route to create the orphan condition.
Counterparty: StaticRepoImporter deliberately relies on update skipping this check as an
ordering escape hatch for cases ImportWriteOrder cannot sequence. A naive validator would break the
importer, which is worse than the hole.

B. Pruning a NodeType strands its instances. ComputePrunableNodes:412 and
PackageInstaller.PruneRemovedNodes:2915 delete a NodeType with no instance check; a rename
(HandleMoveNodeRequest:4697) never rewrites instances.
Counterparty: WhatsNew/2026-08-28-retired-node-prune.md documents pruning retired NodeTypes as
intended. So the answer is probably not refusal but a loud, actionable report — and the detector
already exists as a UI-only query (MeshNodeLayoutAreas.cs:817, nodeType:{name} scope:subtree)
that nobody wired into the delete path. The policy is also already written down, in
V53_RetypeBuiltinSlideDeckToPublish: "every install must retype first — the deletion lands only a
release AFTER it has run everywhere."
That is exactly the rule prune violates.

Worth stating plainly while both are open: there is currently no safe supported way to repair a
mistyped node.
patch refuses nodeType outright (PatchableFields, MeshOperations.cs:4221),
so the only route is update — which is hole A.

Fixing either before this PR would have left them invisible anyway. That is the argument for landing
the instrument first.

🤖 Generated with Claude Code

NodeTypeEnrichmentHelpers.SlowPathTimeout (60s) exactly equalled
MessageHubConfiguration.RequestTimeout (60s). An inner bound that cannot fire
first cannot deliver anything, so the overlay's correct and specific message —
"NodeType 'X' is not registered (referenced by instance '…')" — has never once
reached a caller. Every occurrence surfaced as the ceiling's generic
"No response received in hub …", naming neither the culprit nor the remedy.

This is the rule ReadBudget states for exactly this pair ("a bound nested inside
another bound must be able to fire FIRST, because it is the only one that knows
WHICH read starved"), with three cited incidents. The identical collision is
already recorded from the test side in OrleansDynamicCompilationTest, where it
cost "five sessions theorised from that silence" — that fix raised the test's
budget and left the collision underneath untouched.

Lowering it costs nothing: WaitForCompileSettled disarms this budget while a
compile is in flight, and in-flight visibility is InFlightOverlayGrace (5s to a
self-healing progress overlay). It governs only the dead-wait case.

- SlowPathTimeout 60s -> 30s, invariant stated at the constant
- MessageHubConfiguration.DefaultRequestTimeout made public: it was a bare
  literal invisible outside its assembly, so nested bounds hand-copied the
  number into comments — which is how it drifted into collision
- NodeTypeOverlayBudgetTest pins ordering, headroom and grace ordering;
  verified to fail on reintroduction
- Doc drift fixed: the XML doc argued "30 s leaves budget" above a constant of
  60, and StaticNodeProviders.md said 60 s
- NodeUpdatePipeline no longer reports its own 10s timeout as "Node not found".
  That conflated a non-verdict with a verdict and invited delete-and-recreate on
  live content — the same lie the read path was fixed for (#974). Genuine
  not-found is unchanged.

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

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 0)

216 tests   216 ✅  2m 32s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit f9062a5.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 3)

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

Results for commit f9062a5.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 1)

419 tests   419 ✅  1m 27s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit f9062a5.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 2)

529 tests   527 ✅  1m 31s ⏱️
  4 suites    2 💤
  4 files      0 ❌

Results for commit f9062a5.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 5)

    3 files      3 suites   58s ⏱️
1 194 tests 1 002 ✅ 192 💤 0 ❌
1 195 runs  1 003 ✅ 192 💤 0 ❌

Results for commit f9062a5.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a production diagnostic gap where the NodeType “slow path” overlay could never surface its specific error message because its timeout equaled the hub request timeout ceiling, causing the outer ceiling to preempt the inner diagnostic.

Changes:

  • Reduced NodeTypeEnrichmentHelpers.SlowPathTimeout from 60s to 30s to restore headroom under the hub request ceiling.
  • Exposed MessageHubConfiguration.DefaultRequestTimeout publicly to avoid hand-copied timeout literals drifting into collisions.
  • Added NodeTypeOverlayBudgetTest and updated documentation to pin and explain the timeout ordering invariant.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/MeshWeaver.Graph.Test/NodeTypeOverlayBudgetTest.cs Adds a regression test that enforces timeout ordering/headroom invariants for overlay delivery.
src/MeshWeaver.Messaging.Hub/MessageHubConfiguration.cs Single-sources the default request timeout via a new public DefaultRequestTimeout.
src/MeshWeaver.Hosting/NodeUpdatePipeline.cs Improves update-path error reporting by distinguishing timeout “unavailable” from definitive not-found.
src/MeshWeaver.Graph/Configuration/NodeTypeEnrichmentHelpers.cs Lowers slow-path timeout and documents the nested-timeout invariant.
src/MeshWeaver.Documentation/Data/Architecture/StaticNodeProviders.md Updates doc values/wording to match the new 30s slow-path timeout rationale.

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

Comment on lines +49 to +53
Observable.Throw<MeshNode>(ex switch
{
DeliveryFailureException => new InvalidOperationException(
$"Node not found: {node.Path}"),
TimeoutException => new InvalidOperationException(
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results (shard 4)

1 671 tests   1 671 ✅  1m 49s ⏱️
    3 suites      0 💤
    3 files        0 ❌

Results for commit f9062a5.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Test Results

   14 files     14 suites   9m 22s ⏱️
4 615 tests 4 421 ✅ 194 💤 0 ❌
4 616 runs  4 422 ✅ 194 💤 0 ❌

Results for commit f9062a5.

@github-actions
github-actions Bot merged commit 861f1a0 into main Sep 1, 2026
27 checks passed
@rbuergi
rbuergi deleted the fix/nodetype-overlay-budget branch October 10, 2026 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants