Repository navigation
Reclaim change-feed-evicted mirrors instead of parking them forever (#1324) - #1415
Merged
Merged
Conversation
`Workspace.EvictForPath` fires on EVERY mesh change event — including the echo of the writer's own write — and parked the evicted `ISynchronizationStream` in `_evictedRemoteStreams` without disposing it. A continuously-written path therefore minted a fresh client `sync/` hub (and, via its SubscribeRequest, a matching `sync/` hub on the owner) per write and never gave one back. Instrumented over the NodeType recompile repro the parked set grew 1 → 23 monotonically; its only reaper is the shared mesh-node cache's idle sweep, which needs zero subscribers AND ten minutes untouched — a condition a continuously-written path never meets. Part of #1324 (the residual left after #1345). The eviction is load-bearing (disabled, the next write diffs against a stale snapshot and the owner's MergeGuard refuses it, so a compile never settles) and a "dispose when the Rx subscriber count hits zero" rule cannot work: the reduce chain `CreateExternalClient` builds subscribes to the stream ITSELF, so an evicted stream sits at 2–3 subscribers forever. Both were measured and are recorded in the test's comment. So holders DECLARE themselves. `Workspace.AcquireRemoteStreamUnchecked` resolves the stream and hands back a lease; an evicted stream with no remaining declared holder is disposed at once (UnsubscribeRequest reaches the owner, both `sync/` hubs die). Leaseholders: - the shared mesh-node cache's per-entry hydration — for the entry's whole life, which is what identifies the ONE live mirror per path; - each cross-hub write (`UpdateRemote`, `WriteViaSyncStream`) — for its subscription. A stream nobody leased is never in the registry and keeps the old conservative parking, so undeclared holders (the MeshDataSource / SyncedQueryDataSource reduce callbacks) are unaffected. `DetachRemoteStreams` drops the bookkeeping when it hands ownership out. Measured on `NodeTypeRecompileAlcLeakTest` (Release, three recompiles of a `config => config` NodeType), before → after: hubs/recompile 22.0 → 12.0 / 12.3 / 12.0 managed 8 MB → 6–7 MB cache/<mesh> +18 → +3 compile-state +23 → +12 mirrors 24 opened, 23 evicted, 0 reclaimed → 18 reclaimed The 6 unreclaimed survivors are exactly the one live hydration mirror per path. Ratchet tightened 26 → 15 hubs and 16 → 12 MB per recompile. Two mechanisms still retain and are named in the test comment for the follow-up: the per-compile `_Activity/compile-<ts>` NODE hubs (now the largest share), and owner-side intermediate reduced streams — `MeshDataSource`'s own-node factory builds `primary.Reduce<InstanceCollection>(...)` uncached on every call that passes a configuration (once per inbound SubscribeRequest) and `CreateReducedStream` registers each child on its PARENT, which is the hub-lifetime primary stream. Same shape as #1345, one layer down at `stream.Reduce`; correlating surviving hubs against client stream ids shows 27 of 34 match no client mirror. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #1324’s “EvictForPath parking” retainer by introducing explicit remote-stream holder leases so change-feed-evicted remote mirrors can be reclaimed promptly instead of being parked indefinitely, reducing retained hubs/memory during repeated NodeType recompiles and busy-path writes.
Changes:
- Introduces remote stream acquisition with a disposable lease so evicted streams can be disposed once no declared holders remain.
- Updates the ALC leak repro test thresholds/notes to reflect the improved steady-state and remaining retainers.
- Documents the new lease-based reclamation behavior and adds a “What’s New” entry describing the user-visible impact.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/MeshWeaver.Hosting.Monolith.Test/NodeTypeRecompileAlcLeakTest.cs | Updates measurement commentary and tightens hub/memory retention bounds consistent with the new reclaim behavior. |
| src/MeshWeaver.Mesh.Contract/MeshNodeStreamExtensions.cs | Switches remote stream usage to acquire-and-lease semantics so subscriptions/writes declare holding and release reliably. |
| src/MeshWeaver.Data/Workspace.cs | Implements lease tracking and reclaim-on-eviction logic; adds debug logging for remote-stream cache misses. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-13-writing-to-a-page-no-longer-leaves-a-connection-behind.md | Adds a release note describing the reclaimed-mirror fix and its impact on long-lived portals. |
| src/MeshWeaver.Documentation/Data/DataMesh/SyncedQueryDataSource.md | Documents the new “declared holder” lease model for evicted streams and the rationale. |
| src/MeshWeaver.Documentation/Data/Architecture/MeshNodeStreamCache.md | Expands cache lifecycle documentation to include lease-driven disposal of evicted upstreams. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+148
to
+151
| return (_workspace.GetStream(new MeshNodeReference()) | ||
| ?? throw new InvalidOperationException( | ||
| "MeshNode stream is not available — the workspace has no MeshNodeReference reducer."), | ||
| Disposable.Empty); |
Comment on lines
574
to
576
| // Check if cached stream is still alive. Hub.RunLevel alone is not | ||
| // sufficient because hub shutdown is async: right after stream.Dispose() | ||
| // is called, RunLevel is still Running even though disposal was triggered. |
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
This was referenced Aug 13, 2026
This was referenced Sep 6, 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.
Part of #1324 — closes retainer #1 (the
EvictForPathparking). Does not close the issue: two other mechanisms remain and are named below and in the test's comment.The retainer, and why the two obvious cures were already dead
Workspace.EvictForPathfires on every mesh change event — including the echo of the writer's own write — takes the mirror out of_remoteStreamCacheand parks it in_evictedRemoteStreamswithout disposing it. So a continuously-written path mints a fresh clientsync/hub (and, via itsSubscribeRequest, a matchingsync/hub on the owner) per write and never gives one back. Instrumented over the recompile repro the parked set grew 1 → 23 monotonically; its only reaper is the shared mesh-node cache's idle sweep, which needs zero subscribers and ten minutes untouched — a condition a continuously-written path never meets.Both easy answers were measured and rejected in earlier passes on this issue, and both are now recorded in the test comment so nobody re-derives them:
MergeGuardrefuses it (refused stale/reordered cross-hub write to 'compilationStatus'); the compile never settles. The eviction is load-bearing; the parking is the defect.JsonSynchronizationStream.CreateExternalClientbuilds subscribes to the stream itself (the outbound change-notification pipeline, the version-gate observer), so an evicted stream sits at 2–3 subscribers forever.The fix: holders declare themselves
Subscriber counts cannot separate consumers from the stream's own plumbing — so consumers say so explicitly.
Workspace.AcquireRemoteStreamUncheckedresolves the stream and returns a lease; an evicted stream with no remaining declared holder is disposed at once, which postsUnsubscribeRequestto the owner and kills bothsync/hubs.MeshNodeStreamHandle.Subscribe)UpdateRemote,WriteViaSyncStream)Observable.CreatesubscriptionMeshDataSource,SyncedQueryDataSourceExtensions)A stream nobody leased is never in the registry, so this cannot pull a mirror out from under an undeclared reader.
DetachRemoteStreamsdrops the bookkeeping when it hands ownership out; the acquire re-checks liveness so a stream reclaimed between resolve and lease is released and re-resolved fresh.EvictForPathalso gained the Debug counterpart of its eviction line — "opened remote stream {StreamId} for {Owner} as {Identity}" on a genuine cache miss — which is what made this measurable without a heap dump, and what the attribution below is built from.Measured —
NodeTypeRecompileAlcLeakTest, Release, three recompiles ofconfig => configcache/<mesh>_Activity/compile-state_Activity/compile-<ts>(3 new node hubs)Directly on the mechanism, from the Debug trace over one run: 24 mirrors opened, 23 evicted, 18 reclaimed. The 6 survivors are exactly the one live hydration mirror per path — the correct steady state.
Ratchet tightened: 26 → 15 hubs and 16 → 12 MB per recompile.
What still retains (for the follow-up, not this PR)
_Activity/compile-<ts>NODE hubs (~6 sync hubs apiece) — created per compile, never reclaimed. Now the largest single share of the residual. A node-lifecycle question, not a stream one.compile-state.MeshDataSource's own-nodeAddWorkspaceReferenceStream<MeshNode>factory buildsprimary.Reduce<InstanceCollection>(CollectionReference("MeshNode"))and then reduces that to the node, andWorkspaceStreams.CreateReducedStreamregisters each child for disposal on its PARENT — here the data source's primary stream, i.e. hub lifetime. The factory runs uncached on every call that passes aconfiguration, which is once per inboundSubscribeRequest, so an unsubscribe reaps the subscription's own hub and leaves the intermediates behind. Exactly fix: share a workspace's local reduced streams instead of minting a hub per call (#1324) #1345's defect shape, one layer down atstream.Reduce. Correlating the surviving hubs against the client stream ids says this is whatcompile-state's +12 now is: 27 of the 34 survivingsync/hubs match no client mirror at all.Verification
dotnet restore+dotnet build --no-restore -c Release -p:CIRun=true -warnaserror(CI's exact flags) — 0 errors.MeshWeaver.Hosting.Monolith.Test653/653 ·MeshWeaver.Query.Test390/390 (incl.RemoteStreamCacheTest) ·MeshWeaver.Data.Test344/344 ·MeshWeaver.Security.Test407/407 (incl.RemoteStreamCacheIdentityTest) ·MeshWeaver.Documentation.Test20/20.What's New entry included (
Category: Fix— this is the OOM curve users see).🤖 Generated with Claude Code