fix: the resync give-up must not count an ack that overtook its own answer (#3058) by rbuergi · Pull Request #3059 · Systemorph/MeshWeaver

rbuergi · 2026-09-02T06:40:20Z

The hypothesis held — and there were two doors, not one

SubscribeAck was posted from DataExtensions.HandleSubscribeRequest the instant a re-subscribe arrived. But the alreadyServing branch that answers it only queues an UpdateStreamRequest on the stream's action block — so the acknowledgement raced ahead of its own answer, down a different path (a direct hub response) from the one the Full takes (the stream's outbound forwarding subscription). The two are unordered, and the ack always wins.

SynchronizationStream.RequestFreshSnapshot reads an ack as "the owner answered and nothing followed" and faults the mirror past MaxUnansweredResyncs of them. With the ack arriving first, that sentence was true of every ask the moment it was made, so the counter measured the owner's queue depth:

05:14:07.039  Fresh-snapshot request acknowledged by owner   <- re-assert still queued
05:14:07.043  Fresh-snapshot request acknowledged by owner   <- behind the burst's own frames
05:14:07.048  Fresh-snapshot request acknowledged by owner
              Resync gave up: 3 consecutive fresh-snapshot requests to owner
              'FutuRe/AmountType' were acknowledged and none produced a base snapshot

Three asks, one outstanding request, one perfectly healthy Full still in the queue.

But fixing the ordering alone did not close the gate flake — its own control arm re-measured at 2/14, no better than main. Because once an ack means "I sent it", any path that acks without sending is the same defect, and there were two more:

door	why it acked with nothing sent	can it repeat?
the failure arm of the re-assert	a disposed stream / an apply error — nothing was enqueued	no (once per subscribe)
`BuildReassertFrame` returned `null`	the stream has no state to assert yet	yes — for as long as that lasts

The second one is what was actually reddening the gate. During an install the owner is a NodeType hub that cannot emit until its source compiles, so it sits Current-less for the whole compile window and every re-ask inside it was answered "acknowledged" with no Full behind it. Three of those trip the bound on a stream whose owner is merely busy. The measurement names it: after the ordering fix the fault still fired, still named Course/Card — a NodeType — and still fired in the [checks] phase, i.e. at NodeType activation.

The fix

An ack means the snapshot is on the wire, on every path — and every path still answers exactly once.

The ack follows the frame. CreateSynchronizationStream owns the ack and, on the re-assert path, posts it from Update(…, applied:) — the callback that runs in the same turn right after SetCurrent, by which point Store.OnNext has already driven the forwarding subscription and handed the Full to hub.Post.
A failed re-assert refuses instead of acking. It posts a DeliveryFailure, which ResyncRefused already classifies one-policy-per-ErrorType: ObjectDisposedException (incl. HubDisposingException) ⇒ ShuttingDown ⇒ transient, ridden out per fix(routing): pod-hub claim gets a derived lifetime; PodHubNotHere is answered, not published #2745 and not counted; anything else ⇒ terminal, so the mirror faults and re-establishes. ResyncRefused releases the gate as its first, unconditional statement, so the verdict is as prompt as the ack it replaces.
An empty re-assert defers its answer to the stream's first emission — the moment a Full really does go out. One Rx one-shot; nothing polls, nothing retries. If that state never arrives, the subscriber's own request/response terminal fires and ResyncRefused re-opens the gate without counting the ask — an owner with nothing to send has not failed to answer, it has nothing to answer with.

Ack and Refuse share one compare-and-set, so exactly one verdict leaves per subscribe however the arms interleave.

A fresh stream still acks immediately, deliberately: it has no answer to overtake, its first Full comes out of the reduce's own hydration (IO-bound, unbounded), waiting would hold the subscriber's callback across it — and it cannot repeat, because RegisterClientSubscription makes the next re-ask a re-assert.

Nothing waits on a timer. MaxUnansweredResyncs is unchanged at 3.

#1384's termination is intact — the strongest available evidence

StreamResyncGivesUpTest passes unchanged: same drive loop, same bound, same StreamNotConvergingException. An owner that genuinely never answers still faults after three acknowledged-and-lost snapshots, because an ack now means "I sent it".

Control arms — matched 20 runs each

MeshWeaver.PluginTester.Test, full project per run, same machine, alternating only src/:

arm	runs	`StreamNotConverging`	any failure
`origin/main`	20	5 (25%)	6
ordering fix only (first commit)	14	2	3
both doors closed (this PR)	20	0	0

The control's 25% matches the ~1-in-3 the issue measured. The middle row is the load-bearing one: the first commit alone did not move it — which is why the second exists.

Tests


`StreamResyncAnswerInFlightTest` (new)	The answer is held, not destroyed — the re-assert Full and the ack the owner posts once that frame is on its way, together, because on a real leg they leave through the same queue. The burst's own patches keep flowing. The mirror must ask once, never fault, and converge when the answer is released. It also asserts the invariant as an absence: zero acks may go out ahead of their answer during a resync.
`StreamResyncGivesUpTest`	Unchanged. The genuine wedge still faults.

The new test fails deterministically on unmodified src (full project: 421 pass, 1 fail) in 245 ms:

Expected value to be 1 because the mirror had ONE outstanding re-ask, so it must have
asked ONCE — 4 further frames proving it still has no base are not 4 failures, they are
the same request re-proved […], but found 2.

[Warning] [SYNC_STREAM] Resync has not converged for 1y0zionfsEWuSKXn04tcIQ: asking host/1
  for a fresh snapshot again (attempt 2) — the previous request produced no base snapshot

StreamResyncConvergenceTest and StreamResyncGivesUpTest both carried comments asserting the old ack ordering, and each test's determinism argument rests on it — those rationales are corrected in the same change.

Also green locally, Release: MeshWeaver.Data.Test 422/422, MeshWeaver.Layout.Test 443/443, MeshWeaver.Documentation.Test 176/176 (link integrity + the timeout-literal ratchet), and the full 61-project solution at -c Release -warnaserror with 0 Warning(s), 0 Error(s).

Docs

DataSyncAndCrdt §6 and §9 carried the ordering claim that was wrong — "Because the owner ACKs before its re-assert reaches the wire, the common case costs at most one redundant round trip" — and the "Why 3" reasoning built on it. Corrected, plus the general lesson recorded:

A give-up counter must count outstanding failures, never attempts. The two are the same number only when each attempt has been given its answer's round trip. The moment the counter can be incremented by something that has not yet had a chance to succeed, it stops measuring the condition it names and starts measuring load — so a bound written against "the owner is not answering" fires hardest exactly when the owner is busiest. The cure is never a bigger bound.

Plus a What's New entry (Category: Fix).

Scope

This is question 2 of the issue ("why does an acknowledged re-ask produce no base snapshot"). Question 1 — why a Patch frame is lost at all, 8–15 per samples-gate run on main — is untouched and still open: the give-up was the messenger. That frame loss no longer reds the gate, but it is still there.

Fixes #3058

🤖 Generated with Claude Code

…nswer Fixes #3058 SubscribeAck was posted from HandleSubscribeRequest the instant a re-subscribe arrived, while the re-assert answering it only QUEUES an UpdateStreamRequest on the stream's action block. So the acknowledgement raced ahead of its own answer, down a different path (a direct hub response) from the one the Full takes (the stream's outbound forwarding subscription). SynchronizationStream.RequestFreshSnapshot reads an ack as "the owner answered and nothing followed" and faults the mirror past MaxUnansweredResyncs of them. With the ack arriving first, that sentence was true of every ask the moment it was made, so the counter measured the owner's QUEUE DEPTH, not non-convergence: during a bulk install the re-assert waits behind the burst's own frames, each of those frames re-proves the mirror has no base and earns a fresh re-ask, and three acks landed inside 9 ms (05:14:07.039 -> .043 -> .048, run 33593426491) while the first, entirely healthy Full was still queued. Mirrors faulted mid-install and the samples content gate went red on roughly a third of all runs, on main. The ack now follows the frame it acknowledges. CreateSynchronizationStream owns it and, on the re-assert path, posts it from Update(..., applied:) — the callback that runs in the same turn right after SetCurrent, by which point Store.OnNext has already driven the forwarding subscription and handed the Full to hub.Post. Ack and frame leave through the same outbound queue, in that order, so an ack with no snapshot behind it means what the exception says it means. A fresh stream still acks immediately and the asymmetry is deliberate: that subscribe has no answer to overtake, its first Full comes out of the reduce's own hydration (IO-bound, unbounded), and waiting would hold the subscriber's pending callback across it — risking the very RequestTimeout the ack exists to prevent. It cannot produce the miscount either: RegisterClientSubscription makes the next re-ask a re-assert. Every path acks exactly once, failure arms included. Not a threshold bump: MaxUnansweredResyncs is unchanged at 3, and #1384's termination is intact — StreamResyncGivesUpTest passes UNCHANGED. Tests - StreamResyncAnswerInFlightTest (new): the answer is HELD, not destroyed — the re-assert Full and the ack the owner posts once that frame is on its way, together, because on a real leg they leave through the same queue. The burst's own patches keep flowing. The mirror must ask ONCE, never fault, and converge when the answer is released. On the pre-fix tree the ack escapes the hold and the mirror asks per frame: fails deterministically in 245 ms at "Expected value to be 1 ... but found 2", with "Resync has not converged ... attempt 2" in the log. - StreamResyncGivesUpTest / StreamResyncConvergenceTest: unchanged behaviour; their comments asserting the OLD ack ordering are corrected, since both tests' determinism arguments rest on it. Measured, both arms on this machine, MeshWeaver.PluginTester.Test (14 runs each, 6 plain + 8 under DOTNET_PROCESSOR_COUNT=4): before 1/14 failed (GateInstallsUpstreamPackagesTest, StreamNotConverging on Course/Card; 2 StreamNotConverging log lines) after 0/14 failed (0 StreamNotConverging log lines) Docs: DataSyncAndCrdt sections 6 and 9 carried the ordering claim that was wrong ("the owner ACKs before its re-assert reaches the wire") and the reasoning built on it. Corrected, with the general lesson recorded: a give-up counter must count outstanding FAILURES, never attempts — the two are the same number only when each attempt has had its answer's round trip, and a bound that can be incremented by something which has not yet had a chance to succeed measures LOAD, so it fires hardest exactly when the system is busiest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SD7aiLSo59xng2TzU32xEa

github-actions · 2026-09-02T06:45:07Z

Test Results (shard 3)

602 tests ±0 602 ✅ ±0 54s ⏱️ +2s
2 suites ±0 0 💤 ±0
2 files ±0 0 ❌ ±0

Results for commit 76b5168. ± Comparison against base commit 7186907.

♻️ This comment has been updated with latest results.

github-actions · 2026-09-02T06:45:31Z

Test Results (shard 5)

3 files ±0 3 suites ±0 1m 11s ⏱️ -4s
1 226 tests ±0 1 034 ✅ ±0 192 💤 ±0 0 ❌ ±0
1 227 runs ±0 1 035 ✅ ±0 192 💤 ±0 0 ❌ ±0

Results for commit 76b5168. ± Comparison against base commit 7186907.

♻️ This comment has been updated with latest results.

github-actions · 2026-09-02T06:45:33Z

Test Results (shard 1)

422 tests +1 422 ✅ +1 1m 46s ⏱️ +27s
1 suites ±0 0 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit 76b5168. ± Comparison against base commit 7186907.

♻️ This comment has been updated with latest results.

Copilot

🟡 Changes recommended

The re-assert failure path in JsonSynchronizationStream.CreateSynchronizationStream currently sends a SubscribeAck even when no answering Full was enqueued, which contradicts the new “ack implies snapshot sent” contract and can reintroduce miscounting.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes a synchronization resync give-up false positive where a SubscribeAck could overtake the Full snapshot it was implicitly acknowledging, causing mirrors to “give up” based on the owner’s queue depth rather than genuine non-convergence.

Changes:

Move SubscribeAck emission from DataExtensions.HandleSubscribeRequest into JsonSynchronizationStream.CreateSynchronizationStream, ensuring the ack is queued after the re-assert Full (for the re-subscribe path).
Add a deterministic regression test (StreamResyncAnswerInFlightTest) covering the “answer is delayed/held but in-flight” case to ensure the mirror re-asks only once and does not fault.
Update documentation and test rationales to reflect the corrected ack/answer ordering and the “count failures, not attempts” principle.

File summaries

File	Description
test/MeshWeaver.Data.Test/StreamResyncGivesUpTest.cs	Updates test commentary to match the new ack-after-frame ordering.
test/MeshWeaver.Data.Test/StreamResyncConvergenceTest.cs	Updates test commentary to match the new ack-after-frame ordering.
test/MeshWeaver.Data.Test/StreamResyncAnswerInFlightTest.cs	Adds regression test for “answer in flight” scenario to prevent false give-up under burst.
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-02-a-busy-install-no-longer-breaks-live-views.md	Adds “What’s New” entry describing the user-visible fix.
src/MeshWeaver.Documentation/Data/Architecture/DataSyncAndCrdt.md	Documents the ordering rule and why the give-up counter must count failures, not attempts.
src/MeshWeaver.Data/Serialization/SynchronizationStream.cs	Updates comments in resync logic to reflect ack semantics as evidence post-fix.
src/MeshWeaver.Data/Serialization/JsonSynchronizationStream.cs	Implements deferred ack on re-assert path and immediate ack on fresh stream path.
src/MeshWeaver.Data/DataExtensions.cs	Removes immediate ack posting from `HandleSubscribeRequest`, delegating it to the stream implementation.

Review details

Files reviewed: 8/8 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.

github-actions · 2026-09-02T06:45:46Z

Test Results (shard 0)

231 tests ±0 231 ✅ ±0 2m 19s ⏱️ -21s
1 suites ±0 0 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit 76b5168. ± Comparison against base commit 7186907.

♻️ This comment has been updated with latest results.

github-actions · 2026-09-02T06:46:27Z

Test Results (shard 4)

1 780 tests +5 1 780 ✅ +5 2m 6s ⏱️ -3s
3 suites ±0 0 💤 ±0
3 files ±0 0 ❌ ±0

Results for commit 76b5168. ± Comparison against base commit 7186907.

♻️ This comment has been updated with latest results.

github-actions · 2026-09-02T06:46:33Z

Test Results (shard 2)

539 tests ±0 537 ✅ ±0 1m 35s ⏱️ ±0s
4 suites ±0 2 💤 ±0
4 files ±0 0 ❌ ±0

Results for commit 76b5168. ± Comparison against base commit 7186907.

♻️ This comment has been updated with latest results.

github-actions · 2026-09-02T06:46:54Z

Test Results

14 files ±0 14 suites ±0 9m 54s ⏱️ -1s
4 800 tests +6 4 606 ✅ +6 194 💤 ±0 0 ❌ ±0
4 801 runs +6 4 607 ✅ +6 194 💤 ±0 0 ❌ ±0

Results for commit 76b5168. ± Comparison against base commit 7186907.

♻️ This comment has been updated with latest results.

Review found the new invariant violated one branch over, and following it to the end found the branch that was still reddening the gate. 1. The failure arm of the re-assert now REFUSES instead of acking. It ran when nothing had been sent (a disposed stream, an apply error), so acking there fed an OWNER-side failure into the mirror's acknowledged-and-unanswered count — a channel built to measure LEG loss. It now posts a DeliveryFailure, which SynchronizationStream.ResyncRefused already classifies: ObjectDisposed (the stream or its hub tearing down, incl. HubDisposingException) is ShuttingDown and therefore TRANSIENT — ridden out per #2745, gate re-opened, NOT counted; anything else is terminal, so the mirror faults and re-establishes instead of waiting. ResyncRefused releases the gate unconditionally as its first statement, so the verdict is as prompt as the ack it replaces. Ack and Refuse now share ONE latch (ClaimTheAnswer), so exactly one verdict leaves per subscribe whichever arms are reached. 2. THE BRANCH THAT STILL REDDENED THE GATE. BuildReassertFrame returns null when the stream has nothing to assert, and that branch acked immediately — with nothing sent, and REPEATEDLY. During an install the owner is a NodeType hub that cannot emit until its source compiles, so it sits Current-less for the whole compile window and every re-ask in it was answered "acknowledged" with no Full behind it. Three of those trip MaxUnansweredResyncs on a stream whose owner is merely busy. That is what survived the ordering fix, and the measurement says so: the fault still fired, still named 'Course/Card' — a NodeType — and still fired in the [checks] phase, i.e. at NodeType activation. With the ack now provably ordered after any frame, an ack with no Full can only come from a path that sends none, and this is the only such path that can repeat (the fresh-stream path registers its subscription, so the next re-ask is a re-assert). Such a re-assert now defers its answer to the stream's FIRST emission — the moment a Full really does go out. Nothing polls and nothing retries: one Rx one-shot, disposed by Take(1) when it fires and by the stream when it does not. If the state never arrives, the subscriber's own request/response terminal fires and ResyncRefused re-opens the gate WITHOUT counting the ask, which is the correct reading — an owner with nothing to send has not failed to answer, it has nothing to answer with. Measured, MeshWeaver.PluginTester.Test, full project per run, this machine: origin/main 1 StreamNotConverging in 14 runs ordering fix only (1st commit) 2 StreamNotConverging in 14 runs both doors closed (this commit) 0 StreamNotConverging in 20 runs, 0 failures The first commit alone did NOT move the gate flake — its own arm re-measured at 2/14 — because the counter had a second, repeating source of false evidence. Only both together close it. StreamResyncAnswerInFlightTest additionally asserts the invariant as an ABSENCE: zero acks may go out ahead of their answer during a resync, not merely "some ack rode behind one". Full Data.Test 422/422 and Layout.Test 443/443 in Release. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SD7aiLSo59xng2TzU32xEa

Copilot AI lite review requested due to automatic review settings September 2, 2026 06:40

Copilot started reviewing on behalf of rbuergi September 2, 2026 06:40

View session

Copilot AI reviewed Sep 2, 2026

View reviewed changes

Comment thread src/MeshWeaver.Data/Serialization/JsonSynchronizationStream.cs Resolved

This was referenced Sep 2, 2026

Resync give-up (#1384) fires on a routine bulk-install frame loss: ~1-in-3 flake on main, reproducible in MeshWeaver.PluginTester.Test #3058

Closed

fix: the report, the ack and the log about a delivery stop carrying it (#3044, #3045) #3056

Merged

rbuergi merged commit 804b9be into main Sep 2, 2026
25 of 26 checks passed

rbuergi deleted the fix/3058-resync-giveup-counts-inflight branch September 13, 2026 10:18

Navigation Menu

Repository navigation

fix: the resync give-up must not count an ack that overtook its own answer (#3058) - #3059

fix: the resync give-up must not count an ack that overtook its own answer (#3058)#3059
rbuergi merged 2 commits into
mainfrom
fix/3058-resync-giveup-counts-inflight

rbuergi commented Sep 2, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 2, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 2, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 2, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

github-actions Bot commented Sep 2, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 2, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 2, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 2, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

rbuergi commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The hypothesis held — and there were two doors, not one

The fix

#1384's termination is intact — the strongest available evidence

Control arms — matched 20 runs each

Tests

Docs

Scope

Uh oh!

github-actions Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results (shard 3)

Uh oh!

github-actions Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results (shard 5)

Uh oh!

github-actions Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results (shard 1)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

github-actions Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results (shard 0)

Uh oh!

github-actions Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results (shard 4)

Uh oh!

github-actions Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results (shard 2)

Uh oh!

github-actions Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rbuergi commented Sep 2, 2026 •

edited

Loading

github-actions Bot commented Sep 2, 2026 •

edited

Loading

github-actions Bot commented Sep 2, 2026 •

edited

Loading

github-actions Bot commented Sep 2, 2026 •

edited

Loading

github-actions Bot commented Sep 2, 2026 •

edited

Loading

github-actions Bot commented Sep 2, 2026 •

edited

Loading

github-actions Bot commented Sep 2, 2026 •

edited

Loading

github-actions Bot commented Sep 2, 2026 •

edited

Loading