Repository navigation
fix(data): announce an owner recycle once per subscriber, not once per stream - #6539
Merged
Merged
Conversation
RecycleCascadeAnnouncementFanOutTest: one subscriber holds 20 streams on each of 3 owners, and all 3 owners are recycled. The test counts what the router's carrier posts to the subscriber and asserts at most one message per owner. On main the carrier posts one StreamEndedEvent per stream (60), from Workspace.AnnounceRecycleToClientSubscriptions. Control: every mirror still re-asks its owner once. Test only, pushed alone so that its red run is the negative control. Refs #5532
Workspace.AnnounceRecycleToClientSubscriptions posted one StreamEndedEvent per client subscription through one carrier (portal/nodeops-{meshId} on a mesh); a recycle cascade put every stream on that queue (memex-cloud 2026-10-11T19:21:35Z, depth 30907). Subscribers declaring AcceptsCoalescedStreamEnd now get one end per (owner, subscriber) naming the rest in AlsoEndedStreamIds; RouteStreamMessage splits it per stream. Others unchanged.
Refs #5532
Contributor
Test Results 20 files - 5 20 suites - 5 55m 21s ⏱️ - 1m 33s Results for commit 1658104. ± Comparison against base commit 3182d81. This pull request removes 356 and adds 20 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The regression test can pass on duplicate re-asks and does not cover legacy subscribers that reject coalescing.
3 open findings
What changed in this PR
Coalesces recycle notifications per capable subscriber while preserving per-stream mirror behavior and legacy-client compatibility.
Changes:
- Adds negotiated coalesced stream-end messages.
- Splits coalesced messages at subscribers.
- Adds regression coverage and architecture documentation.
| File | Description |
|---|---|
src/MeshWeaver.Data.Contract/Messages.cs |
Adds capability flag and additional stream IDs. |
src/MeshWeaver.Data/DataExtensions.cs |
Splits coalesced notifications before sync-hub routing. |
src/MeshWeaver.Data/Serialization/JsonSynchronizationStream.cs |
Advertises and propagates capability support. |
src/MeshWeaver.Data/Workspace.cs |
Coalesces recycle announcements per subscriber. |
src/MeshWeaver.Documentation/Data/Architecture/LiveMirrorsAndTheChangeFeed.md |
Documents the revised recycle flow. |
test/MeshWeaver.Data.Test/RecycleCascadeAnnouncementFanOutTest.cs |
Exercises fan-out reduction and resubscription. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+288
to
+292
| if (subscription.AcceptsCoalescedEnd) | ||
| coalesced.Add(key.StreamId); | ||
| else | ||
| ends.Add((subscriber, new StreamEndedEvent(key.StreamId))); | ||
| } |
Contributor
There was a problem hiding this comment.
Agreed, the end-to-end test only mints declaring subscribers, so it could not see this branch. Fixed in 1658104: CoalesceRecycleEnds now takes plain (subscriber, stream id, declared) rows and is internal. The new unit test AnUndeclaredSubscriberKeepsOneEndPerStream_ADeclaredOneGetsOne pins both branches through the one seam. An undeclared subscriber gets one plain end per stream with no AlsoEndedStreamIds, and a declared one gets a single end naming all its streams. Negative control: with the declaration ignored (if (acceptsCoalescedEnd || true)), the test fails with Expected collection {"l1"} to equal {"l1", "l2", "l3"}.
Comment on lines
+171
to
+175
| .Select(_ => Volatile.Read(ref reAsksAfterRecycle)) | ||
| .Should().Within(TestTimeouts.Convergence) | ||
| .Match(n => n >= Owners * StreamsPerOwner, | ||
| "every mirror of a recycled owner must still re-ask it once -- an announcement that " | ||
| + "reaches fewer mirrors strands the rest on a dead activation (#2533, #3986)"); |
Contributor
There was a problem hiding this comment.
Right, a total can be met by a subset looping. Fixed in 1658104: the route records each re-asking mirror as (owner address, stream id), and the control waits for Owners x StreamsPerOwner distinct mirrors. It also checks that the re-asks reach all Owners owners. Local run: re-asks that crossed the router after the recycle: 60 from 60 distinct mirrors, carrier posts client/…=3.
Comment on lines
+307
to
+311
| /// <summary> | ||
| /// The second negotiated capability: this subscriber splits a <see cref="StreamEndedEvent"/> | ||
| /// carrying <see cref="StreamEndedEvent.AlsoEndedStreamIds"/> into one end per stream, so a | ||
| /// recycled owner may tell it ONCE about every stream it held there instead of once per stream | ||
| /// (Systemorph/MeshWeaver#5532). Default <c>false</c>: a subscriber that does not say so (a JS or |
Contributor
There was a problem hiding this comment.
Fixed in 1658104: AcceptsStringSplice now calls itself "THE FIRST NEGOTIATED WIRE CAPABILITY", so the two adjacent contracts no longer contradict each other.
StreamEndedEvent now carries both AlsoEndedStreamIds (this branch, #5532) and Activation (main, #6602). The coalesced announcement is stamped with the owner activation's token, and the subscriber's split copies it onto every per-stream end, so the re-arm budget still counts one teardown once. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…the recycle coalescing (#5532) Answers the three review findings on #6539: - the end-to-end control counts DISTINCT re-asking mirrors (owner, stream id), so a retry loop on a subset of mirrors can no longer satisfy it while the rest stay stranded; - CoalesceRecycleEnds takes plain (subscriber, stream id, declared) rows and is internal, and a unit test pins the rolling-deploy contract: an undeclared subscriber keeps one plain end per stream with no AlsoEndedStreamIds, a declared one gets one end naming all its streams. Negative control: with the declaration ignored, it fails with {"l1"} vs {"l1","l2","l3"}; - AcceptsStringSplice's doc calls itself the FIRST negotiated capability, not THE ONE. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
rbuergi
enabled auto-merge
October 13, 2026 15:08
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.
A recycled owner told its subscribers one
StreamEndedEventper live client subscription, all through one carrier. A subscriber that declares the newSubscribeRequest.AcceptsCoalescedStreamEnd(every C# subscriber, throughMintSubscribeRequest) now gets ONEStreamEndedEventper (owner, subscriber), whose newAlsoEndedStreamIdsnames its other streams; the subscriber'sRouteStreamMessagesplits it back into one end per stream before anything reaches async/{id}sub-hub, so every mirror still re-asks exactly once. Every other subscriber (JS/Python clients, pods on the previous image) keeps the per-stream ends. No timeout, retry, pacing, shed policy or watermark changes.Root cause
Workspace.AnnounceRecycleToClientSubscriptions(src/MeshWeaver.Data/Workspace.cs) rancarrier.Post(new StreamEndedEvent(streamId), o => o.WithTarget(subscriber))once per (subscriber, stream) pair. For a hub under the mesh router the carrier is the router'sRouterCarrierspokesman,portal/nodeops-{meshId}, which is also the hub that issues a recycle cascade. The owner keys each subscription on the subscribing workspace hub (Subscriber = delivery.SenderinJsonSynchronizationStream.CreateSynchronizationStream), so a circuit or cache hub holding many streams of one owner was told once per stream, and a cascade over many owners put the sum of all their streams on that one single-threaded queue.Evidence, memex-cloud, pod
memex-portal-deployment-b79b66b68-w488f, 2026-10-11:[DISPOSE-DISCARD]lines show a RecycleCascade requested byportal/nodeops-ZjzXQ4RCv0K2l430tkdI5A(ModuleLiveUpdate of MeshWeaver.OgCard) recycling LinkedIn, RemoteControl, Edu, Reinsurance, Hosting and more (Ops/Logs/memex-cloud-1791746495042810736-b66b68-w488f,Ops/Logs/memex-cloud-1791746495108529539-b66b68-w488f, landed byOps/Actions/logs-memex-cloud-20261011-5718-getdata-discards).portal/nodeops-ZjzXQ4RCv0K2l430tkdI5Aat inbound depth 30907, shedding StreamEndedEvent; 19:21:36.553Z the same inmesh/ZjzXQ4RCv0K2l430tkdI5Aat depth 44118 (Ops/Logs/memex-cloud-1791746495706784813-b66b68-w488f,Ops/Logs/memex-cloud-1791746496553186738-b66b68-w488f).Ops/Actions/logs-memex-cloud-20261011-5532-p2-unserved-eviction), the healthy-owner end (silent while its owner tears down), a pod stop (Ops/Actions/logs-memex-cloud-20261011-5532-pod-stops).Negative control: commit 0a9e866 is the test alone. Against the unfixed per-stream loop quoted above, the carrier posts one StreamEndedEvent per stream, 3 owners x 20 streams = 60 posts to the one subscriber, which violates the test's bound
perTarget.Max(kv => kv.Value) <= Owners(3).How it was tested
RecycleCascadeAnnouncementFanOutTest.ARecycleTellsEachSubscriberOncePerOwner_NotOncePerStream(test/MeshWeaver.Data.Test): one subscriber holds 20 streams on each of 3 router-hosted owners, and all 3 are recycled with routed DisposeRequests. It counts what the router's carrier posts per target (bound: 3) and, as its control, requires all 60 mirrors to re-ask their owner. This pull request's CI is its first run, together with the existing recycle tests (RecycleAnnouncementTest,OwnerDeactivationTellsItsLiveSubscribersTest,SubscriberReleaseIsNotAnnouncedBackTest).Outside this change: burst A of the same day (19:16:10Z, pod fcnw2,
mesh/QkJnM_x6wUmSDasRo6cINQ, shedding HeartBeatEvent) and the 2026-09-23 window, whose producers have not been read; and the re-asks themselves, which still cross the router one per stream. The issue stays open until a bug.verify on memex-cloud. The finding is conserved insrc/MeshWeaver.Documentation/Data/Architecture/LiveMirrorsAndTheChangeFeed.md.Refs #5532
Bug-Thread: Hosting/Triage/_Thread/bug-systemorph-meshweaver-5532
Opened by Dispatch's control plane on behalf of
Essentials/Agent/bug-triage(auto · Provider/OpenRouterEU/anthropic/claude-opus-5.5), from the bug's own branchbugfix/systemorph-meshweaver-5532. It is never merged by Dispatch: the merge is the governed activitydev.merge, signed by a person once the checks and the internal review are green.