Skip to content

fix(#1186): a failed read is not an absent node — a short snapshot says so - #5165

Merged
rbuergi merged 1 commit into
mainfrom
fix/1186-a-failed-read-is-not-an-absent-node
Sep 22, 2026
Merged

rbuergi merged 1 commit into
mainfrom
fix/1186-a-failed-read-is-not-an-absent-node

Conversation

@rbuergi

@rbuergi rbuergi commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

A store fault caught inside a query provider arrived downstream as a COMPLETE answer

StorageAdapterMeshQueryProvider catches a store fault, logs it at Warning, drops the rows it could not get, and emits its Initial over what survived. Keeping the survivors is deliberate and stays — one corrupt row must not kill a listing. What was wrong is that the frame then claimed a complete answer, so:

  1. the provider was never recorded in QueryResultChange.SilentProviders (it did emit an Initial);
  2. every consumer that refuses to read absence off a floor could not fire — PathResolutionService's floor refusal guards on exactly that field;
  3. the gap travelled outward as a fact about the node: resolution emitted null, and MessageHubGrain reported "Either the node does not exist or no query provider claims its partition."

Both clauses of that sentence were measured false in the cases that fire (#1186): three of three failing paths read back present on the live mesh, and the provider behind the gap had claimed the partition and answered. The union was asserted as exhaustive while the real cause was a third thing.

The fix: one verdict, folded into the field that already exists

QueryResultChange.SnapshotIncomplete — set by a provider, on its own frame, when it answered over reads it could not complete. MeshQuery folds it into SilentProviders, attaching the provider name it already knows.

That reuse is the point: SilentProviders names a provider that produced no Initial; one that produces a shorter Initial is the same lie with a frame attached. So PathResolutionService, MeshNodeStreamCache and SyncedQueryMeshNodes all cover this case with no change of their own — no second decider over one question.

  • The fold runs on the multi-provider merge and the single-provider fast path, because a lone provider is the shape a Monolith and every test actually run.
  • The completeness sink is per subscription, never a field: a field would be process-wide mutable state shared by every concurrent query on that adapter, so one partition's timeout would mark every other caller's snapshot incomplete and nothing would ever clear it.
  • A teardown cancellation is deliberately not recorded — that query is stopping, not answering; recording it would label every in-flight snapshot at every mesh teardown.

No bound was widened, no retry added, no catch removed, no availability lost. The same rows are served; the frame admits it is short.

With a silent read faulting on its own terminal (#5042) and a short read refused as a floor, the grain's remaining arm is genuinely determinate, so it drops the two-cause union and says what it now knows. Three conditions, three sentences, no union.

Controls — one on each side of the change

case expected with the fix with SnapshotIncomplete reverted to false
store throws on the read that would have found the node incomplete + provider named, survivor still served pass fail
walk (ListChildPaths) throws incomplete + provider named pass fail
healthy store, path nobody wrote complete, SilentProviders empty, present row served pass pass

The third case is not a nicety: without it, an implementation that stamped every frame incomplete passes the first two — and refuses every genuinely absent node in production, stalling every per-node hub activation in the mesh. Both fixtures count and assert the faults they injected, so a refactor that stops taking the faulting leg fails loudly instead of leaving the test vacuous.

Negative control run with the test bin's MeshWeaver.Hosting.dll rebuilt (timestamp checked, not assumed): 2 failed / 1 passed.

Verification

  • dotnet build -c Release -warnaserror, one project per invocation: MeshWeaver.Hosting 0 Error(s) 0 Warning(s); MeshWeaver.Hosting.Orleans 0 Error(s) 0 Warning(s); MeshWeaver.Hosting.Test 0 Error(s) 0 Warning(s); MeshWeaver.Documentation.Test 0 Error(s).
  • New suite: 3 total / 3 succeeded / 0 failed (926 ms).
  • Query layer (PathResolution, QueryFanIn, AFaultedRead, PagedQueryTotalOrder, LiveQuery): 29 total / 29 succeeded / 0 failed.
  • DocumentationLinkIntegrityTest: 1 / 1 passed (it caught a wrong sibling-link form first — fixed and re-run).

Scope statements

  • Pairs-with: none — this diff removes no public type and no public member. It ADDS one initproperty toQueryResultChange` and one private nested class; nothing outside core referenced anything removed, because nothing was removed.
  • Implementers: none — no member was added to a public interface or abstract class. SnapshotIncompleteis aninit` property with a default on a record, so no implementer is obliged.
  • Mirror-sync: none — no strings.{en,de}.json` key is added, removed or re-worded. The two sentences that changed are operator-facing diagnostics in exception text and a log line, English by house rule, and neither is a catalog key.

What this does NOT establish

Refs #1186

🤖 Generated with Claude Code

…ys so

`StorageAdapterMeshQueryProvider` catches a store fault, logs it at Warning,
drops the rows it could not get and emits its `Initial` over what survived.
Keeping the survivors is deliberate and stays — one corrupt row must not kill
a listing — but the frame claimed a COMPLETE answer, so the provider was never
recorded in `SilentProviders`, every consumer that refuses to read absence off
a floor could not fire, and the gap travelled outward as a fact about the NODE:
`PathResolutionService` resolved `null` and `MessageHubGrain` reported "the node
does not exist or no query provider claims its partition" over a node that was
present and a provider that had answered. Both clauses were measured false on
#1186 — three of three failing paths read back present on the live mesh.

The verdict: `QueryResultChange.SnapshotIncomplete`, set by a provider on its
own frame, folded by `MeshQuery` into the `SilentProviders` field every existing
consumer already reads (#4557). No new mechanism, no availability change, no
bound touched: the same rows are served and the frame admits it is short. The
fold runs on the multi-provider merge AND the single-provider fast path, because
a lone provider is the shape a Monolith and every test actually run.

The completeness sink is per SUBSCRIPTION, never a field: a field would be
process-wide mutable state shared by every concurrent query on that adapter, so
one partition's timeout would mark every other caller's snapshot incomplete and
nothing would ever clear it. A teardown cancellation is deliberately not
recorded — that query is stopping, not answering.

With a silent read faulting on its own terminal (#5042) and a short read refused
as a floor, the grain's remaining arm is genuinely determinate, so it drops the
two-cause union it used to offer and says what it now knows.

Controls, both sides of the change: a store that throws on the read that would
have found the node ⇒ incomplete + named provider, survivor still served; a
healthy store asked for a path nobody wrote ⇒ complete, `SilentProviders` empty.
Without the second case an implementation that stamped every frame incomplete
passes — and refuses every genuinely absent node in production. Both fixtures
count and assert the faults they injected, so a refactor that stops taking the
faulting leg fails loudly instead of leaving the test vacuous.

Measured: 3/3 pass with the fix; with `SnapshotIncomplete` reverted to `false`
and the test bin's `MeshWeaver.Hosting.dll` rebuilt, 2 failed / 1 passed — the
two fault cases red, the must-not-move control green. Query layer 29/29.
`MeshWeaver.Hosting`, `MeshWeaver.Hosting.Orleans` and the test project each
build `-c Release -warnaserror` with 0 Error(s), 0 Warning(s).

Closure of #1186 awaits production verification, per sev:H policy.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 22, 2026 03:02
@rbuergi
rbuergi enabled auto-merge September 22, 2026 03:02

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.

Copilot review overview

🟢 Approval recommended

The change is cohesive, reuses the established SilentProviders contract as intended, and is covered by focused regression tests and documentation without introducing async/await into src/.

Review effort: Lite
Findings: None

What changed in this PR

This PR fixes a diagnostic/correctness gap in the query layer where a provider could emit an Initial frame after swallowing a store read fault, but the resulting frame still looked complete downstream—allowing failed reads to be misinterpreted as “absent nodes” (#1186). It introduces a provider-side completeness verdict (SnapshotIncomplete) and folds it into the existing SilentProviders mechanism so existing consumers refuse “floors” without adding a second decision channel.

Changes:

  • Add QueryResultChange<T>.SnapshotIncomplete and thread it through StorageAdapterMeshQueryProvider so an Initial snapshot can explicitly declare it is incomplete due to dropped reads.
  • Fold SnapshotIncomplete into SilentProviders in MeshQuery for both multi-provider merge and single-provider fast path, reusing the existing downstream floor-refusal behavior.
  • Add targeted regression tests and a new architecture doc page describing the failure mode and why the fix is structured as a fold into SilentProviders.
File Description
test/​MeshWeaver.Hosting.Test/​AFaultedReadIsNotAnAbsentNodeTest.cs Adds regression coverage for “faulted read ≠ absent node”, including negative controls.
src/​MeshWeaver.Mesh.Contract/​Query/​QueryResultChange.cs Adds SnapshotIncomplete to carry provider completeness on the wire.
src/​MeshWeaver.Hosting/​Persistence/​Query/​StorageAdapterMeshQueryProvider.cs Records dropped reads per subscription and stamps SnapshotIncomplete on the emitted Initial.
src/​MeshWeaver.Hosting/​Persistence/​Query/​MeshQuery.cs Folds SnapshotIncomplete into SilentProviders (single-provider + multi-provider paths).
src/​MeshWeaver.Hosting.Orleans/​MessageHubGrain.cs Updates the determinate-absence activation error message to avoid an incorrect two-cause union.
src/​MeshWeaver.Documentation/​Data/​Architecture/​AFailedReadIsNotAnAbsentNode.md Documents the end-to-end chain and the “single verdict, folded into existing field” rationale.
src/​MeshWeaver.Documentation/​Data/​Architecture.md Adds the new architecture page to the Architecture index.

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

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 0)

  1 files  ±0    1 suites  ±0   2m 50s ⏱️ +9s
306 tests ±0  306 ✅ ±0  0 💤 ±0  0 ❌ ±0 
308 runs  ±0  308 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 0186e8a. ± Comparison against base commit b9d7fa7.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 3)

429 tests   - 14   429 ✅  - 14   1m 0s ⏱️ +12s
  2 suites  -  1     0 💤 ± 0 
  2 files    -  1     0 ❌ ± 0 

Results for commit 0186e8a. ± Comparison against base commit b9d7fa7.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 1)

533 tests   - 1 057   533 ✅  - 1 057   1m 1s ⏱️ - 2m 4s
  1 suites  -     1     0 💤 ±    0 
  1 files    -     1     0 ❌ ±    0 

Results for commit 0186e8a. ± Comparison against base commit b9d7fa7.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 4)

    2 files   -   1      2 suites   - 1   5m 46s ⏱️ -10s
2 526 tests +591  2 526 ✅ +591  0 💤 ±0  0 ❌ ±0 
2 527 runs  +591  2 527 ✅ +591  0 💤 ±0  0 ❌ ±0 

Results for commit 0186e8a. ± Comparison against base commit b9d7fa7.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 2)

1 550 tests  +830   1 550 ✅ +1 022   7m 36s ⏱️ + 2m 3s
    2 suites  -   1       0 💤  -   192 
    2 files    -   1       0 ❌ ±    0 

Results for commit 0186e8a. ± Comparison against base commit b9d7fa7.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 5)

    4 files   -   1      4 suites   - 1   14m 35s ⏱️ -14s
3 232 tests  - 630  3 232 ✅  - 628  0 💤  - 2  0 ❌ ±0 
3 236 runs   - 630  3 236 ✅  - 628  0 💤  - 2  0 ❌ ±0 

Results for commit 0186e8a. ± Comparison against base commit b9d7fa7.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results

   12 files   -   5     12 suites   - 5   32m 52s ⏱️ -2s
8 576 tests  - 280  8 576 ✅  - 86  0 💤  - 194  0 ❌ ±0 
8 583 runs   - 280  8 583 ✅  - 86  0 💤  - 194  0 ❌ ±0 

Results for commit 0186e8a. ± Comparison against base commit b9d7fa7.

@rbuergi
rbuergi added this pull request to the merge queue Sep 22, 2026
@rbuergi

rbuergi commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor Author

Blast-radius check: all three existing SilentProviders consumers, traced

Folding a new condition into an existing field means every consumer of that field now sees it. I traced all three rather than assuming the reuse is free, because a consumer that retries or refuses on this signal would turn a partial store fault into an availability regression — the opposite of what the change is for.

grep -rn "SilentProviders" src --include='*.cs' gives three readers. None of them retries, and none of them refuses more than it should:

1. PathResolutionService.ResolveSegmentsCore (:684) — the intended target. Refuses to resolve, and refuses to CACHE, only when the hit is not the full requested path:

if (change.SilentProviders is { Count: > 0 } silent
    && !PathsEqual(best?.Path, string.Join("/", segments)))
    throw new InvalidOperationException($"Resolution of '…' is INDETERMINATE: …");

So a read that dropped rows but still found the requested path answers normally. Only the case that would otherwise become a false absence is refused. That is precisely the boundary this change needs and it needed no edit.

2. MeshNodeStreamCache (:2984) — evicts the query chain, and its own comment states the semantics the change wants:

connectionOwner.Add(typeSource.Unanswered
    .Where(silent => silent.Count > 0)
    .Take(1)
    .Subscribe(silent => EvictUnansweredQuery(id, signature, silent), …));

The frame is still DELIVERED — nothing hangs, which is the property the counted-as-empty rule exists for — but the chain is DROPPED from the registry, so the next GetQuery for this (id, query set) builds a fresh chain and asks again. Not an expiry timer and not a per-call-site bypass: the cache simply does not keep what nobody said. A genuine answer keeps its Replay(1).

Extending that to a snapshot shortened by a store fault is the correct reading: a partial answer must not become the durable negative for the life of the process (the #4557 / #1246 outage, where durably present _Access grants read as missing for 24 minutes after a restart). Note .Take(1) — it is a one-shot eviction, not a retry loop.

3. SyncedQueryMeshNodes (:386) — publishes the set on its Unanswered side channel and emits the snapshot regardless. It is the producer for consumer 2 and takes no decision of its own.

Net effect

before after
store fault, requested path not found resolved as absent; frame cached as durable truth resolution refused, naming the provider; chain evicted so the next call re-asks
store fault, requested path still found answered, cached answered, chain evicted so the next call re-asks
healthy read, path genuinely absent answered absent, cached unchanged — answered absent, cached

Nothing hangs, nothing retries in a loop, and no availability is lost: the only behaviour that changes is that a snapshot built over an incomplete read stops being kept as the process's durable answer. The third row is the one the AHealthyStoreMissingThePath_StaysADeterminateAbsence control pins.

What I did not establish: I did not measure the eviction rate this adds under a store partly down. A backend faulting steadily on one partition will now evict that query chain on every frame, so each caller rebuilds rather than replaying — correct, but more work than before, and I have no number for it. If that turns out to matter, the place to bound it is EvictUnansweredQuery, not the naming.

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