Repository navigation
fix(#1174): a write's base is held to the announced version, so a hot path stops rebuilding its mirror per write - #4428
Conversation
… path stops rebuilding its mirror per write
Root cause. Workspace.EvictForPath evicted every cached mirror of an owner on EVERY
change-feed event, including the Updated that the writer's own commit produced. A
write leases its mirror only for its subscription, so eviction + release disposed it
and the NEXT write resolved a brand-new mirror: a SubscribeRequest, an initial-state
round trip and a pair of sync/ hubs, per write. Each of those hydrations had to finish
inside the writer's 30 s BaseStateWaitBound while its predecessor was torn down on the
same owner address. {user}/_UserActivity/{user} is written on every cold page load
(one at version 6498 in production), so it paid that thousands of times; a cold path
pays it once. That is the only mechanism in the code that predicts #1174's
concentration on hot paths (414 timeouts across 57 pods since 2026-08-10).
The eviction could not simply go: it was the writer's freshness barrier against
owner-side writes, and the liveness gate that was tried instead lost a whole
25-message append batch (Doc/Architecture/LiveMirrorsAndTheChangeFeed).
Fix. The change feed already announces the committed Version.
- Workspace.OnOwnerNodeChanged: a versioned Updated on a mirrored owner KEEPS the
mirror and records the version as a high-water mark (AnnouncedVersion). A Deleted,
a Created (recreate), the version-less recycle broadcast, or any event shape without
a version/kind still EVICTS, exactly as before. The mark is dropped with the mirrors
(eviction, and DetachRemoteStreams once no mirror of the owner remains).
- MeshNodeStreamHandle.RebaseSource: the write's base must satisfy
Version > refused && Version >= announced, bounded by ConflictRebaseBound and never
parking. At the bound the writer evicts only on PROOF (the mirror carried a node
below the floor and nothing newer); a mirror that carried nothing is still
hydrating and is not evicted for being slow. No mark => byte-for-byte the old
mirror.Take(1).
Evidence (Release, -warnaserror clean on every touched project):
- HotPathMirrorChurnTest (Data): 5 versioned commits -> 1 mirror, 0 Subscribe,
0 Unsubscribe. Negative control with src set aside: 6 distinct mirrors (red).
- HotPathWritesKeepTheirMirrorTest (Graph, real monolith mesh): 6 sequential
cross-hub writes -> 0 mirrors opened after the first, 0 MIRROR_BEHIND, floors in
force, AccessCount=6. Negative control: 5 opened after the first (red).
- HotPathWriteBaseIsVersionAwareTest (Graph, TestScheduler): 6 seam arms incl.
"still hydrating is not evicted" - red when the proof condition is removed;
the Data "recreate still evicts" arm is red when the kind check is removed.
- StaticRepoImportActivityWriteCountTest at DOTNET_PROCESSOR_COUNT=4 (the
liveness-gate repro): passes, 2000 messages in 80 appends -> version 84.
- Full suites green: Data.Test 523, Graph.Test 1687, Hosting.Test 718,
Messaging.Hub.Test 449, Layout.Test 504, Documentation.Test 476; 38 Orleans
change-feed/routing tests.
EvictedUnleasedStreamRetentionTest pinned the #3432 retention by publishing versioned
Updated events, which no longer evict; its characterization arms now drive the
version-less recycle broadcast (the mechanism is unchanged and still reached there),
and a third arm pins that the per-write shape leaves one stream.
Docs: LiveMirrorsAndTheChangeFeed records the fix and the measurements; the pages that
described the per-event eviction as current point at it; What's New (Fix).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pull request was converted to draft
Test Results (shard 1)525 tests 525 ✅ 48s ⏱️ Results for commit 3d5a70a. ♻️ This comment has been updated with latest results. |
Test Results (shard 0)263 tests 263 ✅ 2m 41s ⏱️ Results for commit 3d5a70a. ♻️ This comment has been updated with latest results. |
Test Results (shard 3)1 333 tests 1 333 ✅ 3m 2s ⏱️ Results for commit 3d5a70a. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate mirror lifecycle and freshness-handling issues remain, along with documentation inconsistencies.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR prevents per-write mirror reconstruction by retaining versioned mirrors and enforcing announced-version freshness.
Changes:
- Adds version-aware mirror retention and proof-based eviction.
- Adds hot-path, recreate, and retention regression tests.
- Updates architecture documentation and What’s New.
File summaries
| File | Summary |
|---|---|
test/MeshWeaver.Graph.Test/HotPathWritesKeepTheirMirrorTest.cs |
Tests repeated cross-hub writes and mirror reuse. |
test/MeshWeaver.Graph.Test/HotPathWriteBaseIsVersionAwareTest.cs |
Tests freshness floors and fallback behavior. |
test/MeshWeaver.Data.Test/HotPathMirrorChurnTest.cs |
Tests mirror reuse and recreate eviction. |
test/MeshWeaver.Data.Test/EvictedUnleasedStreamRetentionTest.cs |
Updates retention characterization for remaining eviction events. |
src/MeshWeaver.Mesh.Contract/MeshNodeStreamExtensions.cs |
Moderate: Address per-mirror eviction, preserve the absolute base-state deadline, track below-announced evidence, and include fallback emissions in stale-mirror detection. |
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-15-writing-the-same-node-again-no-longer-reconnects-every-time.md |
Adds the user-facing fix note. |
src/MeshWeaver.Documentation/Data/DataMesh/SyncedQueryDataSource.md |
Updates stream eviction guidance. |
src/MeshWeaver.Documentation/Data/Architecture/SyncHubPopulation.md |
Nit: Resolve the contradiction about eviction occurring on every change-feed event. |
src/MeshWeaver.Documentation/Data/Architecture/MeshNodeStreamCache.md |
Updates cache behavior documentation. |
src/MeshWeaver.Documentation/Data/Architecture/LiveMirrorsAndTheChangeFeed.md |
Nit: Avoid overstating what timeout evidence proves. |
src/MeshWeaver.Documentation/Data/Architecture/EvictedStreamRetention.md |
Nit: Scope historical retention behavior to events that still evict. |
src/MeshWeaver.Documentation/Data/Architecture/BaseStateTimeoutCensus.md |
Nit: Describe per-write churn as one contributing mechanism rather than definitive timeout proof. |
src/MeshWeaver.Data/Workspace.cs |
Moderate: Synchronize announced-version marks with cache lifecycle, including fault removal and detach races, so orphaned or stale marks are cleared. |
Review details
Suppressed comments (10)
src/MeshWeaver.Data/Workspace.cs:415
- The new mark is also left behind when a cached stream is removed through
DiscardFaultedRemoteStream: that path drops the stream from_remoteStreamCachebut does not clear_announcedVersions. If it was the last mirror and no later change-feed event arrives, the supposedly cache-bounded dictionary retains the path indefinitely and a future acquisition inherits an obsolete floor. Clear the mark when fault removal leaves no mirror for the owner (while preserving it if another mirror remains).
/// this workspace currently mirrors, and removed when those mirrors leave — evicted
/// (<see cref="EvictRemoteStreamsForPath"/>) or detached by the shared mesh-node cache's idle
/// release (<see cref="DetachRemoteStreams"/>, once no mirror of the owner remains). Without
/// the second removal a written-then-idle path would keep its mark for the life of the
/// process.
src/MeshWeaver.Data/Workspace.cs:445
- The mark is removed only after the
_remoteStreamCache.IsEmptyguard. If the detach/feed race leaves an announced version while the last live mirror is already gone, a later delete/recycle or proof-based eviction returns here without clearing it, so_announcedVersionscan retain a path for the workspace lifetime and a future mirror can inherit a stale floor. Remove the mark before checking whether the stream cache is empty.
if (string.IsNullOrEmpty(path) || _remoteStreamCache.IsEmpty)
return;
// The mark describes a mirror that is about to be gone; a fresh stream is authoritative by
// construction and needs no floor.
_announcedVersions.TryRemove(path, out _);
src/MeshWeaver.Documentation/Data/Architecture/BaseStateTimeoutCensus.md:128
- This conclusion is too strong: retaining a hot-path mirror removes the per-write rebuild mechanism, but a first or freshly evicted mirror can still fail to hydrate (the same owner activation/RLS/persistence cases described above). A timeout on a fixed replica therefore does not prove that the mirror was not rebuilt per write; keep the census classification as the evidence and describe the fix as removing one contributing mechanism.
([Live Mirrors and the Change Feed](../LiveMirrorsAndTheChangeFeed)). A base-state timeout on a
replica carrying that fix therefore comes from a mirror that was NOT rebuilt per write, and its
census branch is the next fact to read.
src/MeshWeaver.Documentation/Data/Architecture/EvictedStreamRetention.md:34
- This new paragraph correctly says versioned
Updatedevents no longer enter the retention path, but the section immediately below still states thatWorkspace.EvictForPathruns on every change-feed event. That leaves the page internally contradictory and makes the retained experiment read as if its per-write result still applies; scope the following explanation/code to the delete/recreate/version-less events or explicitly label it pre-#1174.
🚨 **NARROWED 2026-09-15 by [#1174](https://github.com/Systemorph/MeshWeaver/issues/1174).** The
eviction below no longer runs on *every* change-feed event: a versioned `Updated` — one per write —
keeps the mirror and records the version it announced (see
[Live Mirrors and the Change Feed](../LiveMirrorsAndTheChangeFeed)). The retention described here is
unchanged as a mechanism, but it is now reached only by a delete, a recreate or the version-less
recycle broadcast. `EvictedUnleasedStreamRetentionTest` drives that last shape for exactly that
reason, and its third arm pins that the per-write shape leaves one stream, not one per write.
src/MeshWeaver.Documentation/Data/Architecture/LiveMirrorsAndTheChangeFeed.md:187
- This repeats the same overclaim as the census page: the fix stops per-write churn, but a timeout can still arise while the initial or a newly created mirror is hydrating. The post-fix census can distinguish those remaining causes; it cannot establish from the timeout alone that the mirror was or was not rebuilt per write.
code that predicts #1174's concentration on hot paths. The census from #4342 is what will confirm it
on a rolled replica: an occurrence after this fix would have to come from a mirror that was NOT
rebuilt per write. The `Dropping StreamEndedEvent … the target stream is gone` lines of #2776 were the
src/MeshWeaver.Documentation/Data/Architecture/SyncHubPopulation.md:107
- This new scope note says the eviction now runs only for delete, recreate, and version-less recycle events, but section 4 still says
Workspace.EvictForPathruns on every change-feed event. Without marking that block as historical or narrowing it to the events that still evict, the architecture page contradicts the behavior this PR introduces.
🚨 Since [#1174](https://github.com/Systemorph/MeshWeaver/issues/1174) this runs only for change
events no mirror can be held to (a delete, a recreate, a version-less recycle broadcast); a versioned
`Updated` keeps the stream — see [Live Mirrors and the Change Feed](../LiveMirrorsAndTheChangeFeed).
The measurement below predates that.
src/MeshWeaver.Mesh.Contract/MeshNodeStreamExtensions.cs:2071
- The proof is local to the mirror subscribed by this write, but
EvictRemoteStreamsForPathremoves every cached reference and identity for the owner. A slow or RLS-limited mirror can therefore evict healthy mirrors/readers and make them open a fresh subscription on every commit, reintroducing the hot-path churn this change is intended to remove. Evict only the mirror that produced the below-floor node, while retaining the shared floor for any other cached mirrors.
workspace.EvictRemoteStreamsForPath(_path!);
src/MeshWeaver.Mesh.Contract/MeshNodeStreamExtensions.cs:439
- When a floor is present, this
Timeout(ConflictRebaseBound)disposes the firstBaseStateSourcesubscription and switches tomirror.Take(1), which creates a newBaseStateSourceand restarts its 30-second timer. If no usable node has arrived yet, the operation can consequently wait 5 + 30 seconds before failing, even thoughBaseStateWaitBoundand the timeout diagnostic promise a 30-second base-state bound. Carry one absolute deadline through the floor wait and fallback (or apply only the remaining base-state budget).
.Timeout(
ConflictRebaseBound,
Observable.Defer(() =>
{
if (refusedBaseVersion > 0)
src/MeshWeaver.Mesh.Contract/MeshNodeStreamExtensions.cs:410
- This guard is false whenever
refusedBaseVersion >= announcedVersion, so it suppresses eviction for every such conflict re-attempt. YetsawNodeBelowFlooronly meansVersion <= refusedBaseVersion; the mirror may still have emitted a node belowannouncedVersion(e.g. refused=6500, announced=6498, node=6497), which proves it missed the announced commit. Track below-announced evidence separately so conflict-only cases still avoid eviction while this stale case is evicted.
var announcedIsBinding = announcedVersion > 0 && announcedVersion - 1 >= refusedBaseVersion;
src/MeshWeaver.Mesh.Contract/MeshNodeStreamExtensions.cs:444
- If the floor-filter subscription times out before the mirror emits anything, this fallback can then receive a node below
announcedVersion, butsawNodeBelowFloorwas never updated for that subscription.onMirrorBehindAnnouncedis therefore skipped and the stale mirror remains cached, causing every later write to repeat the catch-up bound. Track the fallback emission too and add a delayed-below-floor regression beside the existing delayed-at-floor case.
return mirror.Take(1);
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return; | ||
| } | ||
|
|
||
| var announced = _announcedVersions.AddOrUpdate(path, version, (_, seen) => Math.Max(seen, version)); |
There was a problem hiding this comment.
Fixed in 3d5a70a. The floor is now built so it cannot outlive or precede its mirror; nothing has to keep a second structure in step with the cache.
What changed
- The per-owner
_announcedVersionsmap is gone, along with all of its cleanups: inEvictRemoteStreamsForPath, inDetachRemoteStreams, and the orphan sweep. - The floor is a field of the mirror instance: a new internal
IAnnouncedVersionFloor, implemented bySynchronizationStream<TStream>. The raise is a lock-free monotonic CAS max; nothing waits on it. OnOwnerNodeChangedraises it pair-exact on each materialised instance it finds in_remoteStreamCachefor the owner. A removal that lands between finding an instance and raising its floor takes that instance out of the cache, so the raise lands on an instance no new caller can get. A new mirror is a new instance and starts with no floor.- The write reads the floor off the very stream it acquired,
Workspace.AnnouncedVersionFor(remoteStream), never by path. Every removal path therefore takes the floor with the mirror, including the faulted-stream path you pointed out: idle release, eviction, fault and recycle alike.
Test: AnnouncedFloorLifetimeTest (Data.Test, 2 tests). A workspace test seam, BeforeAnnouncedVersionRecorded (null in production), runs a removal exactly in the window you named. I ran it red against the previous head (fb43c55) with the seam at the same point (between the presence check and the map write), then green:
- Interleaving:
DetachRemoteStreamsinside the window, then a new mirror.floorOnNewMirrorwas 7 (red), now 0. - Faulted mirror, then the rebuilt one:
floorOnRebuiltMirrorwas 5 (red), now 0. - Positive controls: the seam ran and detached this exact mirror, and the live mirror carried floor 5 before the fault.
Suites re-run green: Data.Test 525, Graph.Test 1687 (including HotPathWritesKeepTheirMirrorTest: 0 mirrors opened after the first write, 0 MIRROR_BEHIND), Hosting.Test 718, Documentation.Test 476, the 38 Orleans change-feed/routing tests, and StaticRepoImportActivityWriteCountTest at DOTNET_PROCESSOR_COUNT=4 (2000 messages in 80 appends reach version 84).
Test Results (shard 4) 3 files 3 suites 5m 0s ⏱️ Results for commit 3d5a70a. ♻️ This comment has been updated with latest results. |
Test Results (shard 5) 4 files 4 suites 5m 43s ⏱️ Results for commit 3d5a70a. ♻️ This comment has been updated with latest results. |
Test Results (shard 2)1 643 tests 1 643 ✅ 9m 6s ⏱️ Results for commit 3d5a70a. ♻️ This comment has been updated with latest results. |
Test Results 14 files 14 suites 26m 22s ⏱️ Results for commit 3d5a70a. ♻️ This comment has been updated with latest results. |
…n neither outlive nor precede it Copilot finding (thread 4016282575). The floor was kept in a per-owner map beside the remote-stream cache: OnOwnerNodeChanged checked "does the cache still mirror this owner?" and then AddOrUpdate'd the map. The two steps were not atomic with the cache's removals, so a mirror removed in between (idle release via DetachRemoteStreams, an eviction, a fault) left a floor with no mirror behind it, and the NEXT mirror built for that owner inherited it; the faulted-stream removal path (GetExternalClientSynchronizationStream -> DiscardFaultedRemoteStream) never cleared it at all. A fresh mirror hydrates at or past every announced version, so an inherited floor could only make its first write wait out ConflictRebaseBound for nothing, and the map held entries until a later event swept them. Fix: no map. The floor is a field of the mirror instance. - New internal IAnnouncedVersionFloor, implemented by SynchronizationStream<TStream> with a private long and a lock-free monotonic raise (CAS max; nothing waits). - OnOwnerNodeChanged raises the floor PAIR-EXACT on each materialised instance it finds in _remoteStreamCache for the owner. A removal landing between finding an instance and raising it takes the instance out of the cache, so the raise lands on an instance no new caller can get; a new mirror is a new instance with no floor. - The write reads the floor off the very stream it acquired: Workspace.AnnouncedVersionFor(remoteStream) (static, no path lookup). - Every map cleanup is gone (EvictRemoteStreamsForPath, DetachRemoteStreams, the orphan sweep) - there is nothing left to keep in step with the cache. - A Lazy whose factory has not produced its stream yet carries no floor; that mirror's subscribe is being posted now, which is the exposure a creator had before #1174 and which the owner's merge answers with Conflict + rebase, never a loss. - BeforeAnnouncedVersionRecorded: an instance test seam (null in production) in the window between locating a mirror and raising its floor. Evidence (Release, -warnaserror clean: Data, Mesh.Contract, Documentation, Data.Test, Graph.Test, Hosting.Test, Hosting.Orleans.Test, Documentation.Test): - AnnouncedFloorLifetimeTest (new, 2 tests), run RED first against fb43c55 with the same seam placed between the presence check and the map write: interleaving (DetachRemoteStreams inside the window, then a new mirror): floorOnNewMirror=7 (red) -> 0 (green) faulted mirror, then a rebuilt one: floorOnRebuiltMirror=5 (red) -> 0 (green) Positive controls: the seam ran and detached THIS mirror; the live mirror carried floor 5 before the fault. - Suites: Data.Test 525/525, Graph.Test 1687/1687 (HotPathWritesKeepTheirMirrorTest: openedAfterWarmup=0 keptFloors=10 mirrorBehind=0 accessCount=6), Hosting.Test 718/718, Documentation.Test 476/476, 38/38 Orleans change-feed/routing tests, StaticRepoImportActivityWriteCountTest at DOTNET_PROCESSOR_COUNT=4: 2/2 (2000 messages in 80 appends -> version 84). Docs: LiveMirrorsAndTheChangeFeed says the floor is a field of the mirror, why the map shape could not close the window, and adds the lifetime test to "Measured". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #1174.
Root cause
Workspace.EvictForPathevicted every cached mirror of an owner on every change-feed event, including theUpdatedproduced by the writer's own commit. A write leases its mirror only for the life of its subscription. So after each commit the eviction plus the lease release disposed the mirror, and the next write had to resolve a brand-new one: aSubscribeRequest, an initial-state round trip and a new pair ofsync/hubs, per write. Each of those hydrations had to finish inside the writer's 30 sBaseStateWaitBoundwhile the mirror it replaced was being torn down on the same owner address.{user}/_UserActivity/{user}is written on every cold page load; one such node is at version 6498 in production, so it has paid that cycle thousands of times. A cold path pays it once. This is the only mechanism in the code that predicts the concentration on hot paths recorded in this issue (414 occurrences across 57 pods since 2026-08-10).The eviction could not just be deleted. It was also the writer's freshness barrier against writes the owner makes itself, and the liveness gate tried in its place lost a whole 25-message append batch (
Doc/Architecture/LiveMirrorsAndTheChangeFeed).Fix: the write's base is held to the version the change feed announced
Workspace.OnOwnerNodeChanged: a versionedUpdatedon a mirrored owner keeps the mirror and records the announced version as a high-water mark (AnnouncedVersion). These events still evict, exactly as before:DeletedCreated(a recreate is a new incarnation)The mark leaves with the mirrors: on eviction, and in
DetachRemoteStreamsonce no mirror of that owner remains. Dropping a mark is always safe, because a fresh mirror hydrates at or past any version announced before it.MeshNodeStreamHandle.RebaseSource(the same seam as the ActivityTracking fails to write user activity on optimistic concurrency conflict #1910 conflict rebase): the base must satisfyVersion > refused && Version >= announced.ConflictRebaseBoundand never parks.mirror.Take(1).This is not the liveness gate. Liveness asks how the mirror looks. The floor asks the mirror to prove it carries the announced commit, and the announcement is the same event that used to trigger the eviction.
Relation to the earlier interrupted WIP
This builds on the uncommitted branch work, which I checked before keeping. Three changes were needed:
Createdevicts again (the WIP kept the mirror for any versioned event, which would let a previous incarnation's higher-versioned mirror pass as fresh).DetachRemoteStreams(otherwise it lived for the process lifetime). The WIP's churn test also had a baseline wait that could never fail, which is fixed.Verification
All touched projects build clean with
-c Release -warnaserror.HotPathMirrorChurnTest(Data): 5 versioned commits on one ownerHotPathWritesKeepTheirMirrorTest(Graph, real monolith mesh): 6 sequential cross-hub writesMIRROR_BEHIND, floors in force,AccessCount=6HotPathWriteBaseIsVersionAwareTest(Graph,TestScheduler): 6 seam armsHotPathMirrorChurnTestrecreate armStaticRepoImportActivityWriteCountTestatDOTNET_PROCESSOR_COUNT=4(the liveness-gate repro)Full suites, all green:
EvictedUnleasedStreamRetentionTest(the #3432 characterization) used versionedUpdatedevents, and those no longer evict. Its two characterization arms now drive the version-less recycle broadcast; the retention mechanism is unchanged and is still reached there (+5 unleased, +0 leased, as documented). A third arm pins that the per-write shape leaves one stream.What this does not yet prove
That this is the whole of the production 414 needs a rolled replica. The census from #4342 will say which branch a post-fix occurrence is in. Any such occurrence would come from a mirror that was not rebuilt per write, and that is the next fact to read.
Cross-repo / gates
internal) →Pairs-with: none — only internal members changed (Workspace.EvictForPath private→internal EvictRemoteStreamsForPath, new internal AnnouncedVersion, RebaseSource gains optional parameters, ConflictRebaseBound private→internal); nothing outside core can reference them.Implementers:needed.Mirror-sync: none — no catalog change.Docs:
LiveMirrorsAndTheChangeFeedrecords the fix and its measurements.MeshNodeStreamCache,SyncHubPopulation,EvictedStreamRetention,BaseStateTimeoutCensusandSyncedQueryDataSourcepoint at it where they described the per-event eviction as current. What's New entry (Fix) added.🤖 Generated with Claude Code