Repository navigation
A view catching up got handed a snapshot older than what it had already applied - #975
Merged
Merged
Conversation
…dy applied Fixes #945. `CrossHubPatchAtomicityTest.ConcurrentCrossHubPatches_DoNotDropAQueuedMessage` has been reddening unrelated PRs on loaded CI shards with a handful of the 288 concurrent cross-hub adds missing. The test's own message blamed a non-atomic owner-side merge. It is not that: the owner is correct, and the loss is on the READ side. WHAT THE INSTRUMENTATION SHOWED (not inferred — measured on a local repro) Reproduced 12+ times by running the test under CPU saturation (10 concurrent 2-core-emulated test processes). Three diagnostics, added to the test itself so no log configuration is involved: * WRITE ERRORS (0) — every one of the 288 writes completed successfully. Nothing failed. * OWNER-TRUTH probe (a fresh client opening its own subscription, so it receives the owner's authoritative Current): `version=289 present=288 missing=[]`, in EVERY failing run. The owner-side merge kept all 288 entries. It is atomic and it is not the bug. * MIRROR SEQUENCE — every emission the shared-cache mirror delivered. The failing run shows the mirror going BACKWARD: REGRESSIONS (1): [245@v246 -> 234@v235] tail=[272@v284, 273@v285, ..., 277@v289] The mirror was at 245 entries / node version 246, then dropped to 234 / v235, then resumed applying patches normally (+1 per frame) to the end — permanently 11 entries short. The test reported exactly 11 missing. ROOT CAUSE A write burst makes the mesh change feed announce node versions faster than a loaded mirror applies them, so the mirror's version-gated `Resubscribe` fires. The owner answers on the `alreadyServing` path in `JsonSynchronizationStream.CreateSynchronizationStream` by re-asserting its snapshot onto the stream it is already serving. That re-assert had two defects in three lines: 1. STALE CAPTURE. `Current` was bound OUTSIDE the stream's update turn and written back blindly (`_ => snapshot`). `Update` posts an UpdateStreamRequest; the lambda runs later on the action block, so every frame that committed in between was overwritten — the stream moved backward. Because the owner's outbound JSON cursor is then re-based on the rolled-back state, the erased entries are never re-sent. Later patches keep applying cleanly on top, so the mirror tracks the owner forever at a CONSTANT deficit with no error anywhere. 2. WRONG CLOCK. The frame was stamped `alreadyServing.Hub.Version` — the per-subscriber SYNC hub's message counter — while every content frame on that stream carries the OWNER's data-source clock (`ApplyChanges`, carried through the reduce). Two unrelated counters on one stream is the exact defect `StandardReducers` already documents ("CARRY the base version ... NEVER stream.Hub.Version"). It is what lets a stale Full slip past the receive-side monotonicity guard that exists to drop precisely that; when the sync clock instead runs ahead, every later content patch is dropped as "stale" and the mirror freezes. THE FIX `BuildReassertFrame` reads the snapshot IN-TURN and carries the version that state already has. A re-assert is still a full authoritative snapshot, so an orphaned mirror re-converges (a Full always lands, and a mirror that resubscribed is by definition not ahead) — but it can no longer erase newer state or poison the mirror's version. WHAT THIS IS NOT Not an array-replacement bug. `Thread.PendingUserMessages` is an `ImmutableDictionary`, so RFC 7396 merges it key-by-key; and the owner-truth probe proves the merge kept all 288 entries in every failing run. (`MeshNodePatchMerge.MergeArray` already three-way-merges genuine arrays by element identity for the same reason.) EVIDENCE THE FIX WORKS Same saturation harness, same machine: * before: 7 of 40 runs failed, every failure a permanent deficit (11/18/30/54/70 entries) with the mirror never converging. * after: 39 of 40 passed. The single failure has REGRESSIONS (0) and the mirror reaching 288@v289 — the owner's exact state — just after the 30s settle bound. That is late delivery on a box running ten meshes at once, not a lost write; the test now says so itself via `converged=YES at t...ms`. TESTS * `ResubscribeReassertFrameTest` (new, deterministic) pins both halves: the frame carries the state read at build time, at that state's own version. Negative control: reverting the helper to `stream.Hub.Version` fails it with "Expected 999999 ... but found 4". * `CrossHubPatchAtomicityTest` keeps its assertion unchanged and unweakened; its failure message now reports write errors, owner truth, and the mirror's emission sequence, so a future failure names which layer lost the data instead of asserting a theory. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a cross-hub synchronization defect where a resubscribing mirror could be served a “re-assert” frame built from a stale snapshot and/or stamped with the wrong version clock, allowing a mirror to regress and permanently fall behind under heavy write bursts.
Changes:
- Build resubscribe re-assert frames in-turn from the stream’s current state and carry the state’s own version (not the per-subscriber sync-hub clock).
- Add a deterministic unit test to pin the re-assert frame’s value + version semantics.
- Enhance
CrossHubPatchAtomicityTestfailure diagnostics to distinguish write failures, owner truth, mirror regressions, and late convergence.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/MeshWeaver.Data.Test/ResubscribeReassertFrameTest.cs | New deterministic test pinning resubscribe re-assert frame semantics (value freshness + version clock). |
| test/MeshWeaver.AI.Test/CrossHubPatchAtomicityTest.cs | Adds instrumentation and richer failure diagnostics to localize loss to write/owner/mirror layers. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-08-live-views-no-longer-fall-behind.md | What’s New entry describing the user-visible symptom and the fix at a high level. |
| src/MeshWeaver.Data/Serialization/JsonSynchronizationStream.cs | Implements BuildReassertFrame and uses it to re-assert state correctly for resubscribing subscribers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Contributor
Contributor
…h it exists for Review finding on #975: the mirror-sequence watcher was disposed with a plain `watch.Dispose()` after the try/catch, which `Assert.Fail(...)` throws straight past, and it was subscribed with no OnError handler. Both matter more than hygiene here, because the instrumentation IS this PR's deliverable — the recorded sequence is what makes the regression legible, and it is read only when the test fails: * `using var` — an undisposed subscription on the failure path leaves an Observe subscription pending into teardown. That is a live CI failure mode in this repo right now (SourceDocumentDataLoadingTest, "left Observe subscriptions pending past the Quiescing budget", shard 4); a diagnostic must not add to that pile. * explicit OnError — an unhandled stream fault would be rethrown on the producer's thread and could replace the diagnostics the failure message is built from, leaving the next reader exactly where the previous three investigations were. The fault is now captured and REPORTED ("MIRROR STREAM FAULTED: …") alongside the sequence recorded so far, so a faulted stream is itself a datum rather than a swallowed one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Contributor
Contributor
Contributor
This was referenced Aug 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #945.
CrossHubPatchAtomicityTest.ConcurrentCrossHubPatches_DoNotDropAQueuedMessagehas beenreddening unrelated PRs (#927, #930, #935, #959, #968) with a handful of the 288 concurrent
cross-hub adds missing. The test's failure message blamed a non-atomic owner-side merge.
It is not that — the owner is correct in every failing run, and the loss is on the READ
side.
What was measured, not inferred
Reproduced locally 12+ times by running the test under CPU saturation (10 concurrent
DOTNET_PROCESSOR_COUNT=2test processes — the CI shard condition). Three diagnostics wereadded to the test itself, so no log configuration is involved and they survive to the trx:
Current)version=289 present=288 missing=[]— the owner kept everythingREGRESSIONS (1): [245@v246 -> 234@v235]The shared-cache mirror was at 245 entries / node version 246, went backward to 234 /
v235, then resumed applying patches normally (+1 per frame) to the end — permanently 11
entries short. The test reported exactly 11 missing.
Root cause
A write burst makes the mesh change feed announce node versions faster than a loaded mirror
applies them, so the mirror's version-gated
Resubscribefires. The owner answers on thealreadyServingpath inJsonSynchronizationStream.CreateSynchronizationStreambyre-asserting its snapshot onto the stream it is already serving. That re-assert had two
defects in three lines:
Currentwas bound outside the stream's update turn and written backblindly (
_ => snapshot).Updateposts anUpdateStreamRequest; the lambda runs lateron the action block, so every frame that committed in between was overwritten — the stream
moved backward. The owner's outbound JSON cursor is then re-based on the rolled-back state,
so the erased entries are never re-sent. Later patches keep applying cleanly on top, so
the mirror tracks the owner forever at a constant deficit with no error anywhere.
alreadyServing.Hub.Version— the per-subscribersync hub's message counter — while every content frame on that stream carries the
owner's data-source clock (
ApplyChanges, carried through the reduce). Two unrelatedcounters on one stream is the exact defect
StandardReducersalready documents ("CARRY thebase version … NEVER
stream.Hub.Version"). It is what lets a stale Full slip past thereceive-side monotonicity guard that exists to drop precisely that; when the sync clock
instead runs ahead, every later content patch is dropped as "stale" and the mirror freezes.
The fix
BuildReassertFramereads the snapshot in-turn and carries the version that state alreadyhas. A re-assert is still a full authoritative snapshot, so an orphaned mirror re-converges (a
Full always lands, and a mirror that resubscribed is by definition not ahead) — but it can no
longer erase newer state or poison the mirror's version.
What this is not
Not an array-replacement bug.
Thread.PendingUserMessagesis anImmutableDictionary, soRFC 7396 merges it key-by-key, and the owner-truth probe proves the merge kept all 288 entries
in every failing run. (
MeshNodePatchMerge.MergeArrayalready three-way-merges genuine arraysby element identity for the same reason.)
Evidence the fix works
Same saturation harness, same machine:
entries) with the mirror never converging.
REGRESSIONS (0)and the mirror reaching288@v289— the owner's exact state — just after the 30 s settle bound. That is latedelivery on a box running ten meshes at once, not a lost write, and the test now says so
itself via
converged=YES at t…ms.Tests
ResubscribeReassertFrameTest(new, deterministic) pins both halves: the frame carriesthe state read at build time, at that state's own version. Negative control — reverting the
helper to
stream.Hub.Versionfails it with "Expected 999999 … but found 4".CrossHubPatchAtomicityTestkeeps its assertion unchanged and unweakened — no widenedtimeout, no retry, no reduced concurrency. Its failure message now reports write errors,
owner truth, and the mirror's emission sequence, so a future failure names which layer lost
the data instead of asserting a theory.
Scope note
This is the cross-hub-patch defect only. It does not speak to the separate shard-4 instability
on #968 (
SourceDocumentDataLoadingTestleakedObservesubscriptions past the Quiescingbudget, and one byte-identical run went green) — that is a different failure mode and is not
addressed here.
🤖 Generated with Claude Code