Repository navigation
fix(sync): a resync that never converges faults the mirror instead of asking forever (#1384) - #3040
Merged
Merged
Conversation
… asking forever (#1384) A Patch whose `BasedOnVersion` does not chain onto the last applied frame proves a frame was lost in transport, and the mirror asks the owner for a fresh snapshot. #2654 made that recovery survive ONE lost answer: the resync gate is released by the re-ask's round trip, so the next proven gap earns one new re-ask. What it could not do is TERMINATE. When the owner→subscriber leg keeps eating this stream's snapshots, every re-ask is acknowledged, every acknowledgement releases the gate, every answering Full dies on the way back, and the mirror re-asks for the rest of the process's life — at Warning in a log, and at nothing at all in the API a consumer can act on. A log line is not an API: the subscriber holds "awaiting first data" forever, with no error and no completion to re-establish from. Measured live on memex-cloud, 2026-09-01, on Event/SavGeneralversammlung2026/Talk: [SYNC_STREAM] Frame loss detected for G4LJWZjBXkWdULLsT-5N5g: incoming Patch v13 chains onto v12 but the last applied frame is v11 — a frame was lost in transport Layout area 'Present' on Event/SavGeneralversammlung2026/Talk was torn down having never rendered — the subscriber only ever saw the "awaiting first data" placeholder Plain node reads on that path answered instantly, and recycling the pod holding the activation did not clear it: the owner was healthy, and the wedge lived entirely in a subscriber nobody had told anything was wrong. So the count of consecutive ACKNOWLEDGED-and-unanswered re-asks is bounded, and at MaxUnansweredResyncs (3) the mirror stops asking and OnErrors with a StreamNotConvergingException. That is the one signal a subscriber can act on: StreamLiveness.IsUsable already refuses to serve a faulted stream (#2387), so the workspace cache evicts and closes it and the next natural caller opens a fresh one that subscribes from scratch. Three things this deliberately is not: * NOT a timer, poller or watchdog. Nothing retries and nothing polls. An increment costs a full round trip to the owner — the gate suppresses every re-ask while one is outstanding, and only the answer releases it — PLUS a subsequent owner frame proving the mirror still has no base. It counts EVIDENCE, not effort; raising the bound buys a longer silence, not a better chance. * NOT a count of ASKS. The increment lives in the ack arm and nowhere else, because a re-ask refused TRANSIENTLY (ShuttingDown — the rolling-deploy overlap window, #2745) is deliberately ridden out, and counting it would fault every mirror in that window. A re-ask that gets no verdict at all stays recoverable and loud, as before. * NOT applied to a self-owned stream. PatchDataChangeRequest reaches UpdateStream on the owner's server-side stream too; there the counter never resets (the Full that would reset it is one that stream SENDS), so it measures nothing. The latch ordering the RCA also flagged — nulling the cache and latching before the `Reference is WorkspaceReference` check — is already correct since be39fc6 (#2654), and would not have prevented this incident anyway: LayoutAreaReference IS a WorkspaceReference, so the Present-area stream took the ordinary path, asked, was acknowledged, and was never answered. Pinned by StreamResyncGivesUpTest: one mid-burst Patch eaten to start the resync, then EVERY fresh snapshot eaten, one write per proven gap. With the bound inverted to int.MaxValue the test reproduces the incident's exact log signature (Frame loss detected … then "Resync has not converged" attempts 2, 3, 4) and hangs; with the bound in place the mirror faults and a fresh subscriber converges on the owner's complete state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core synchronization/convergence behavior in a highly concurrent hot path, so it should get final human review despite good test coverage and documentation.
Pull request overview
This PR hardens MeshWeaver’s data synchronization “mirror resync” path so a subscriber stream that repeatedly detects frame loss and repeatedly re-requests a snapshot will stop looping silently and instead fault the mirror stream, enabling downstream consumers (and the workspace cache) to recover by re-subscribing.
Changes:
- Add a bounded “acknowledged-but-unanswered resync” counter to
SynchronizationStream.RequestFreshSnapshot()that faults the mirror withStreamNotConvergingExceptionafterMaxUnansweredResyncs. - Introduce
StreamNotConvergingExceptionas the explicit terminal signal for non-converging mirrors. - Add a deterministic regression test covering both the fault and the “fresh subscriber recovers” behavior, plus documentation/What’s New updates describing the termination contract and operator signal.
File summaries
| File | Description |
|---|---|
src/MeshWeaver.Data/Serialization/SynchronizationStream.cs |
Implements the bounded non-convergence behavior (count acked-but-unanswered resyncs, reset on Full, fault after threshold, suppress repeat reporting). |
src/MeshWeaver.Data/Serialization/StreamNotConvergingException.cs |
Adds a dedicated exception type to represent “mirror cannot re-establish base snapshot despite acknowledged resyncs”. |
test/MeshWeaver.Data.Test/StreamResyncGivesUpTest.cs |
Adds a deterministic test that injects frame loss (one Patch + all Full snapshots) and asserts fault + subsequent recovery via a new stream instance. |
src/MeshWeaver.Documentation/Data/Architecture/DataSyncAndCrdt.md |
Documents the termination contract, rationale for counting ACKs, mirrors-only qualifier, and the operational signal. |
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-02-a-view-that-never-loads-now-says-so.md |
Adds a user-facing Fix entry describing the new “report failure instead of waiting forever” behavior. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
…eview) Copilot review on #3040. The give-up branch read `unansweredResyncs` three times, so the Warning and the exception message were two separately-sampled numbers. They are provably equal today — the gate is HELD from the check onwards, so the ack arm cannot increment, and the reset runs on this very hub turn — but a diagnostic that says "gave up after 3" beside an exception that says "after 4" would cost the next reader more than it tells them, and nobody should have to reconstruct that argument to trust the number. One local, used everywhere; the owner comes off the `Owner` property rather than a second local. (The review's stated mechanism — a concurrent reset from the Full branch — is not the reachable one: `UpdateStream`'s Full reset and `RequestFreshSnapshot` run on the same sync-hub action block. The ack arm is the genuinely cross-thread writer, and the gate excludes it here. The change is right regardless of which argument gets you there.) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 2, 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.
Closes the residual of #1384 (RCA in this comment) — defence in depth for a frame that IS lost, so the recovery cannot become a silent permanent wedge.
The residual #2654 left behind
A Patch whose
BasedOnVersiondoes not chain onto the last applied frame proves a frame was lost in transport, so the mirror asks the owner for a fresh snapshot. #2654 made that recovery survive ONE lost answer: the resync gate is released by the re-ask's round trip, so the next proven gap earns one new re-ask.What it could not do is terminate. When the owner→subscriber leg keeps eating this stream's snapshots:
Resync has not convergedat Warning is the only trace, and a log line is not an API: the subscriber is handed neither a value nor a terminal, so nothing downstream can re-establish.The live incident — memex-cloud, 2026-09-01,
Event/SavGeneralversammlung2026/TalkPlain node reads on that path answered instantly, and recycling the pod holding the activation did not clear it — the owner was healthy, and the wedge lived entirely in a subscriber nobody had told anything was wrong. Repro from the user: present the deck, page to the last slide, press Esc.
The root cause of the frame loss is being handled elsewhere (#2976 blocked from prod by #1043 ⇒ null
AccessContext⇒ cross-schema fan-out ⇒ 6–9 s queries ⇒ dropped frames). This PR is not about that. It is about what happens when a frame is lost anyway, and it holds while the portal is slow.The fix
Bound the count of consecutive acknowledged-and-unanswered re-asks. At
MaxUnansweredResyncs(3) the mirror stops asking andOnErrors with a newStreamNotConvergingException.That fault is the recovery signal.
StreamLiveness.IsUsablealready refuses to serve a faulted stream (#2387), soWorkspaceevicts and closes it and the next natural caller opens a fresh one that subscribes from scratch;OnErroralso opensSynchronizationGate, releasing whatever was deferred behind it.Three things this deliberately is NOT
ShuttingDown— the rolling-deploy overlap window, #2745) is deliberately ridden out, and counting it would fault every mirror in that window, whichResyncRefusedalready calls "a worse outcome than the bug this ticket fixes". A re-ask with no verdict at all stays recoverable and loud, unchangedPatchDataChangeRequestreachesUpdateStreamon the owner's server-side stream too, where the counter never resets (the Full that would reset it is one that stream sends), so it measures nothing. Guarded with the sameOwner.Equals(Host.Address)predicateOwnerVersion()usesThe latch ordering the RCA also flagged
RequestFreshSnapshotnulling the cache and latching before itsReference is WorkspaceReferencecheck was fixed by be39fc6 (#2654) and is already correct on main. It would not have prevented this incident:LayoutAreaReference : WorkspaceReference<EntityStore>, so the Present-area stream took the ordinary path — it asked, was acknowledged, and was never answered.Verification — deterministic repro, and it can fail
StreamResyncGivesUpTestinjects two losses on the owner's post pipeline, both scoped to the one mirror stream: one mid-burst Patch (starts the resync), then everyFullon that stream. One write per proven gap, each gated on the event that the previous answer was destroyed — never a delay.Inverted probe (
MaxUnansweredResyncs = int.MaxValue, i.e. pre-fix "never give up") — the whole project,DOTNET_PROCESSOR_COUNT=4, Release:That is the production signature reproduced in a test, and then the silence.
Restored — same project, same flags:
Also green:
MeshWeaver.Documentation.Test172/172, and a deliberately-broken-link probe confirmedDocumentationLinkIntegrityTestnames the edited page (Doc/Architecture/DataSyncAndCrdt: … does not exist) rather than passing vacuously.dotnet build -c Release -warnaserror, one project per invocation:0 Warning(s) / 0 Error(s)onMeshWeaver.Data,MeshWeaver.Documentation,MeshWeaver.Data.Test,MeshWeaver.Documentation.Test.check-record-signatures.pyandcheck-type-forwards.pyboth clean against current main.Work products conserved
Doc/Architecture/DataSyncAndCrdt→ new section "The termination contract — when the mirror stops asking and faults (Six of the last ten CI failures were unrelated single integration tests — a flake population taxing every second PR #1384)": the incident, why the count is of acks, the mirrors-only qualifier, the bound-too-low/too-high asymmetry, the operator signal, and the deliberate boundary at "no verdict at all".Category: Fix) — a user notices a view that never loads.🤖 Generated with Claude Code