Repository navigation
fix(data): a remote stream no longer leaves a permanent registrant on the subscribing hub (#3432) - #5206
Conversation
… the subscribing hub (#3432) CreateExternalClient registered the owner-protocol SubscribeRequest subscription twice — on the stream, and again on the subscribing hub as "belt-and-suspenders". A hub's registrant composite is append-only (CompositeDisposable.Add never prunes; nothing in src/ removes an entry), so the second registration held the subscription — and through its closure the whole SynchronizationStream, its store's last snapshot and its sync/ hub until #3321's release ran — for the subscribing hub's entire life. One per remote stream ever opened. It bought nothing: the stream's own composite rides its sync/{id} sub-hub, which is a hosted hub of the subscribing hub and is therefore disposed in the DisposeHostedHubs phase, strictly BEFORE that hub walks its own registrants in ShutDown. Adds MessageHub.DisposalRegistrantCount, printed as Registrants= in both disposal diagnostics — the only reading that can see this retention class, which retains an object graph rather than a hub and is therefore invisible to every MessageHub histogram. Controls measured on both sides (StreamRegistrantsLeaveTheSubscribingHubTest): with the duplicate restored, floor=4 afterOpen=6 afterDispose=6 — RED; fixed, floor=3 afterOpen=3 afterDispose=3. Plus a growing-direction control (one real registration moves the counter 3 to 4) and a coverage control (an undisposed stream is still torn down by the hub's teardown, quiescing clean, registrants to 0). Doc/Architecture/SyncHubPopulation section 8 carries the registry-level account: the one root, the three removals, the (RunLevel, IsDisposing) cross-tab, and both instances of this retention class. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The teardown controls can pass without an outstanding subscription, and the child timeout assertion becomes ineffective after child deregistration.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
This PR removes duplicate remote-stream disposal registration, adds registrant diagnostics, and documents the retention mechanism.
Changes:
- Removes the subscribing-hub registration that retained disposed streams.
- Adds
DisposalRegistrantCountto diagnostics. - Adds regression tests and architecture documentation.
| File | Description |
|---|---|
test/MeshWeaver.Data.Test/StreamRegistrantsLeaveTheSubscribingHubTest.cs |
Regression and teardown tests |
src/MeshWeaver.Messaging.Hub/MessageHub.cs |
Registrant metric and diagnostics |
src/MeshWeaver.Documentation/Data/Architecture/SyncHubPopulation.md |
Retention and registry documentation |
src/MeshWeaver.Data/Serialization/JsonSynchronizationStream.cs |
Removes duplicate registration |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // See DisposalRegistrantCount: append-only composite, so a number that climbs with the | ||
| // traffic this hub has served is one retained object graph per unit of that traffic. | ||
| .Append(" Registrants=").Append(DisposalRegistrantCount); |
There was a problem hiding this comment.
Agreed, and fixed. You are right that nothing held the printed form: every test here read DisposalRegistrantCount directly, so deleting either Registrants= append would have left them all green while the production census — which parses those two lines, not a CLR property — lost its only reading of this retention class.
TheRegistrantCounter_MovesWhenSomethingIsActuallyRegistered now asserts both readers against a NONZERO count, after registering one real cleanup:
- client.GetPendingRequestDiagnostics() must contain
Registrants={after}— the one-line snapshot a live request timeout attaches. - client.GetDisposalDiagnostics().Split('\n')[0] must contain
Registrants={after}— the FIRST line on purpose, which is the root hub's own (depth 0), so this is exact rather than "some hub in the recursive walk happens to carry this number".
It also asserts after > 0 first, because Registrants=0 is what a hub that registered nothing prints too, and an assertion satisfied by that would be the same kind of step that cannot fail.
Measured on the fixed build: DIAG control before=3 after=4, and both lines carry Registrants=4:
Hub client/14444b186624 RunLevel=Started Queue(buffer=0,deferred=0,drainsInFlight=0) PendingCallbacks=0[] Registrants=4
Hub client/14444b186624 RunLevel=Started Disposal= Queue(buffer=0,deferred=0,drainsInFlight=0,openGates=0,draining=False) Registrants=4
And each assertion was checked to be independently live, by removing one append at a time, rebuilding and re-running — not by reading the code:
- both appends removed: RED on the pending-request line —
Expected "... PendingCallbacks=0[<none>]" to contain "Registrants=4". - only the AppendDiagnostics append removed: RED on the disposal line —
Expected "... openGates=0,draining=False)" to contain "Registrants=4".
So neither assertion is carried by the other.
| _ = workspace.GetRemoteStream<InstanceCollection, CollectionReference>( | ||
| CreateHostAddress(), new CollectionReference(nameof(BusinessUnit))); | ||
| (LiveSyncHubs(client) - syncHubFloor).Should().Be(1, | ||
| "the stream under test must exist before its teardown means anything"); |
There was a problem hiding this comment.
You are right, and this is the stronger of the two findings: that control could not fail. StartAndSettleAsync has already waited for the owner's initial snapshot, so observeSubscription is a .Take(1) that has already completed by the time client.Dispose() runs — AnyHubQuiescingTimedOut() == false was therefore true of a teardown with nothing outstanding to release, which is a verification step that reads like a pass.
Replaced with a probe, and running the negative control on it refuted my own first version of the fix as well, so the test now claims something narrower and truer than either.
The test is AnUndisposedRemoteStreamsComposite_IsWalkedByTheSubscribingHubsTeardown: register a probe disposable ON the stream (stream.RegisterForDisposal), never dispose the stream, dispose the subscribing hub, require the probe to come back disposed before DisposalCompleted. It carries a pre-assertion that the probe is UNDISPOSED first, because RegisterForDisposal disposes a registrant on the spot when the stream is already dead and a probe disposed at registration time would satisfy the final assertion with no teardown at all; nothing runs between the two assertions but client.Dispose().
The refutation, because it changes what the test is allowed to say. I first wrote it as proving the sync/ sub-hub route specifically. I then broke that route — removed the hub.RegisterForDisposal(streamDisposables) hook from SynchronizationStream.RegisterForDisposal, rebuilt, re-ran — and the test still PASSED. Workspace.Dispose also disposes every cached remote stream, and its own comment says it exists for exactly this: "Each SynchronizationStream registers its own SubscribeRequest hub.Observe callback for disposal here; without this loop the parent hub's responseSubjects entry for each open SubscribeRequest leaks past the test base's quiescing-budget leak check." So at least TWO routes reach that subscription on a hub teardown, and the line this PR deletes was a THIRD and the latest of them — a hub walks its own registrants in ShutDown, after both. The test now asserts the OUTCOME and says in its remarks that it is over-determined and must not be re-worded into a route claim without breaking every other route first. That is a stronger argument for the deletion than the one I started with, and I would not have had it without running the control.
Why not the unanswered-SubscribeRequest precondition specifically: it would have made the test assert something false on both builds. Both the deleted registration and the surviving one run strictly AFTER Quiescing — the deleted one in ShutDown, the surviving one in DisposeHostedHubs — so neither rescues the quiesce budget, and asserting AnyHubQuiescingTimedOut() == false over a genuinely pending callback would have been wrong rather than merely weak. RemoteStreamDisposeCancelsSubscribeTest remains the right home for that contract: it is about the STREAM-dispose route, where the release is synchronous and where the precondition is load-bearing.
Full MeshWeaver.Data.Test suite on the fixed build: total 536, failed 0.
…nts= contract, and replace a coverage control that could not fail Finding 1: every test read DisposalRegistrantCount directly, so deleting either `Registrants=` append would have left them all green while the production census — which parses those two lines, not a CLR property — lost its only reading of this retention class. Both readers are now asserted against a NONZERO count, the disposal one on the FIRST line so it is the root hub's own rather than "some hub in the walk carries this number". Each assertion was checked to be independently live by removing one append at a time and re-running: both-removed reds the pending-request line, AppendDiagnostics-only reds the disposal line. Finding 2: the old coverage control could not fail. StartAndSettleAsync waits for the owner's initial snapshot, so the .Take(1) subscription had already completed before client.Dispose() and AnyHubQuiescingTimedOut() == false was true of a teardown with nothing to release. Replaced with a probe registered ON the stream, with a pre-assertion that it is undisposed, so the 0 -> 1 transition is caused by the teardown and nothing else. And running the negative control on the replacement refuted the claim I first wrote for it: with the sync/ sub-hub hook removed from SynchronizationStream.RegisterForDisposal the test still passes, because Workspace.Dispose disposes every cached remote stream too — its own comment says it exists to release exactly this SubscribeRequest callback. So the coverage is OVER-DETERMINED: at least two routes reach the subscription on a hub teardown and the deleted line was a third and the latest of them, since a hub walks its own registrants only in ShutDown. The test now asserts the OUTCOME and its remarks say it must not be re-worded into a route claim without breaking every other route first. MeshWeaver.Data.Test: total 536, failed 0. Release -warnaserror clean; the timeout-literal ratchet re-checked after the rewrite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… the control revealed The source comment and the doc page both said the hub teardown reaches the stream's subscription by ONE route, the sync/ sub-hub's own teardown. Running the negative control refuted that: with that hook removed the coverage test still passes, because Workspace.Dispose disposes every cached remote stream and its own comment says it exists to release exactly this SubscribeRequest callback. Both run before a hub walks its own registrants in ShutDown, so the deleted line was the third route and the last to fire. Same argument, stronger evidence — and the correction is recorded where the next reader will look rather than only in the review thread. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributes to #3432 (
sev:H). The issue stays OPEN — this closes one retention root and addsthe reading that can see its whole class; it takes no census and measures no live slope, which is
the closing condition that thread has carried since 2026-09-13.
The root
JsonSynchronizationStream.CreateExternalClientregistered the stream's owner-protocolSubscribeRequestsubscription twice: on the stream (correct) and again onhub, theSUBSCRIBING hub, under "Belt-and-suspenders: dispose the subscription when the HUB tears down too
(idempotent)".
A hub's registrant composite is append-only —
CompositeDisposable.Addnever prunes(
MessageHub.cs:2385,:4218) and no code path insrc/removes an entry — so that secondregistration held the subscription, and through its closure the entire
SynchronizationStream(itsStoreand that store's last snapshot, itsReference, its capturedAccessContext, and itsHubuntil #3321's release ran), for the subscribing hub's whole life. One per remote stream ever
opened. On a portal hub that is one per rendered layout area and per node read.
And it bought nothing — by TWO routes, measured, not one as I first wrote.
ISynchronizationStream.RegisterForDisposalhooks the stream's own composite onto the stream'ssync/{id}sub-hub (SynchronizationStream.cs:1193), a HOSTED hub of the subscribing hub, torndown in its
DisposeHostedHubsphase. AndWorkspace.Disposedisposes every cached remote stream,which its own comment says exists for exactly this — "Each SynchronizationStream registers its own
SubscribeRequest hub.Observe callback for disposal here; without this loop the parent hub's
responseSubjects entry for each open SubscribeRequest leaks". A hub walks its OWN registrants only
later, in
ShutDown. So the deleted line was the third route and the last to fire: it couldonly ever reach an already-disposed subscription, while holding the whole graph behind it forever.
🚨 That second route is a correction I owe to running a negative control rather than reading the
code — see the coverage row below.
The instrument, because this class is invisible to every existing one
It retains an object graph rather than a hub, so no
MessageHubhistogram or heap type tableattributes it.
MessageHub.DisposalRegistrantCountis the reading that can, and it is printed asRegistrants=in bothAppendDiagnosticsandGetPendingRequestDiagnostics— the same lines thedisposal-state census this issue turns on already parses. A hub whose registrant count climbs with
the traffic it has served is retaining one object graph per unit of that traffic.
Controls — measured on both sides
StreamRegistrantsLeaveTheSubscribingHubTest(MeshWeaver.Data.Test), two remote streams opened ondistinct keys then disposed:
floor=4 afterOpen=6 syncHubsOpened=+2→afterDispose=6; RED at "Expected value to be 4 … but found 6"floor=3 afterOpen=3 syncHubsOpened=+2→afterDispose=3— flatTheRegistrantCounter_MovesWhenSomethingIsActuallyRegistered)before=3 after=4on one realRegisterForDisposal— so "the count returned to its floor" is not read off a counter that cannot moveAnUndisposedRemoteStreamsComposite_IsWalkedByTheSubscribingHubsTeardown)DisposalCompleted— so the 0 → 1 transition is caused by the teardown and nothing elseRegistrants=appends reds the pending-request assertion; removing only theAppendDiagnosticsone reds the disposal assertion — so neither is carried by the othersyncHubsOpened=+2is the positive signal that both streams provably existed: a run in whichGetRemoteStreamreturned nothing would otherwise report a flat registrant count and read as a pass.🚨 The review found both of these, and the second finding was right in a way I only saw by running
its control. The first coverage control could not fail —
StartAndSettleAsyncwaits for theowner's initial snapshot, so the
.Take(1)subscription had already completed beforeclient.Dispose()andAnyHubQuiescingTimedOut() == falsewas true of a teardown with nothing torelease. Its replacement then refuted my own first claim for it: with the
sync/sub-hub hookremoved from
SynchronizationStream.RegisterForDisposaland the tree rebuilt, the probe test STILLpasses, because
Workspace.Disposereaches the same subscription. The test therefore asserts theOUTCOME and says in its remarks that it must not be re-worded into a route claim without breaking
every other route first.
Verification
dotnet build -c Release -warnaserror, one project per invocation:MeshWeaver.Messaging.Hub,MeshWeaver.Data,MeshWeaver.Documentation,MeshWeaver.Data.Test,MeshWeaver.Messaging.Hub.Test,MeshWeaver.Documentation.Test— each0 Warning(s) 0 Error(s).MeshWeaver.Messaging.Hub.Test: total 501, failed 0 (3m 34s).MeshWeaver.Data.Test: total 536, failed 0 (1m 11s).MeshWeaver.Documentation.Test(every guard +DocumentationLinkIntegrityTest): total 617, failed 0 (21.8 s).TestTimeoutLiteralRatchetGuardcaught the one30.Seconds()literal the new test first carried (381 vs baseline 380); it is converted toTestTimeouts.Convergence, not re-seeded.What this does NOT do
sync/hubs rooted in their parent'sHostedHubsCollection.messageHubs, which only a hub's own disposal empties. This releases theSTREAM graph a disposed stream left behind, not a hub whose stream was never disposed.
DataExtensionsandMeshDataSourceregister one cleanup per REQUEST SERVED on the owning hub and nothing removes it — measured with
the new instrument at +1 per answered
GetDataRequest(floor 4 → 14 over ten reads, withPendingCallbacks=0). That fix needs a detach-without-dispose registration (several of thoseregistrants NACK when disposed) swept across ~13 sites of the write path's terminal-answer
machinery at once, so it is its own change set. Filed into bug triage as
rbuergi/Feedback/20260922-per-request-registrants-never-leave-the-hub.Docs
Doc/Architecture/SyncHubPopulation§8 — the registry-level account this thread did not have: theone root, the three removals (and which is cannot be removed versus only when X happens), the
(RunLevel, IsDisposing)cross-tab with the cell that is empty by construction, and theregistrant-composite retention class with both instances.
Deploy
Framework code, no node content — nothing to recycle; a roll carries it.
sev:H#3432 stays open andcloses only on post-roll verification.
Pairs-with: none — no public type or member leaves
src/(the diff removes one call and adds oneconcrete property on
MessageHub).Mirror-sync: none — no catalog key added or re-worded.
🤖 Generated with Claude Code