Repository navigation
fix(data): a lease and a mirror reclaim are one atomic decision (#5087) - #5640
Conversation
ReclaimIfUnheld read the lease count, then removed the entry and disposed.
A writer that resolved the mirror before its eviction could lease in that
window, pass its IsUsable re-check, and have the mirror disposed under it —
its base read completed empty ("this hub's mirror ended without ever
carrying the node's state").
The reclaim now claims the stream by compare-exchanging its lease count
0 -> -1 (ReclaimingSentinel); a lease is taken only by compare-exchange from
a non-negative count and is refused against the sentinel, so the acquirer
resolves a fresh mirror. The sentinel is dropped only after Dispose.
ReclaimLeaseAtomicityTest drives the window deterministically through the
ReclaimDecided seam: on the old code B's lease was granted on a disposed
mirror (leaseBGranted=True mirrorUsable=False); now it is refused.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved moderate and critical lifecycle/concurrency findings remain in Workspace.cs.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Fixes a remote-stream lease/reclaim race using atomic sentinel-based coordination.
Changes:
- Updates lease and reclaim handling in
Workspace. - Adds deterministic race and control tests.
- Documents lifecycle behavior and remaining failure producers.
| File | Summary |
|---|---|
test/MeshWeaver.Data.Test/ReclaimLeaseAtomicityTest.cs |
Adds race and control tests. |
src/MeshWeaver.Documentation/Data/Architecture/WriteVerdictTotality.md |
Updates write-verdict documentation. |
src/MeshWeaver.Documentation/Data/Architecture/EvictedStreamRetention.md |
Documents the reclaim protocol. |
src/MeshWeaver.Data/Workspace.cs |
Implements atomic lease/reclaim coordination. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (!_remoteStreamLeases.TryUpdate(stream, ReclaimingSentinel, 0)) | ||
| return; | ||
| if (!_evictedRemoteStreams.TryRemove(stream, out _)) | ||
| { | ||
| // Another edge (DetachRemoteStreams, Dispose) took ownership after the claim; give the | ||
| // slot back so the bookkeeping does not keep refusing a stream we no longer reclaim. | ||
| _remoteStreamLeases.TryUpdate(stream, 0, ReclaimingSentinel); |
There was a problem hiding this comment.
Agreed, and reproduced. Fixed in 1f90603cb3.
With one shared -1 the reclaim could not tell the re-parked ownership from its own claim: DetachRemoteStreams dropped the sentinel, ParkRemoteStreams re-added the parking entry, and the reclaim's TryRemove then took that new entry and disposed the stream.
The fix:
- Per-claim token. Each claim is now a fresh negative token (
-Interlocked.Increment(ref _reclaimClaims)). - Re-read after taking the entry. After it takes the parking entry, the reclaim re-reads the lease slot. Its own token means no handover happened, so it disposes. Any other value (absent,
0, a count, another token) means the claim was superseded, so it restores_evictedRemoteStreams[stream]and returns. - No gap before the dispose. Once the re-read passes, the stream is out of
_evictedRemoteStreams.DetachRemoteStreamscan no longer reach it before the dispose.
Test. I added ReclaimLeaseAtomicityTest.ADetachAndRePark_InsideTheClaim_SupersedesTheReclaim for exactly this interleaving. The seam (ReclaimClaimed) now runs right after the claim, and the test runs DetachRemoteStreams + ParkRemoteStreams there. It failed on the sentinel version: the re-parked mirror was disposed. It passes with the token. The other arms (the lease race and the control) and the related Evicted*/*Mirror* classes are still green locally. EvictedStreamRetention §10 now documents the token.
…ark supersedes it (#5087) Review finding on #5640: with one shared -1 sentinel, DetachRemoteStreams (which drops the lease entry) followed by ParkRemoteStreams inside the claim left the reclaim unable to tell the re-parked ownership from its own claim, so it took the new parking entry and disposed a stream a consumer had just re-attached to. The claim is now a fresh negative token per reclaim. After taking the parking entry the reclaim re-reads the slot: its own token means no handover; anything else means the claim was superseded, and it restores the parking entry and leaves the stream alone. The seam moves to right after the claim (ReclaimClaimed) and a new arm drives the handover; it failed on the sentinel version and passes now. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Refs #5087
The defect
Workspace.ReclaimIfUnheldREAD the lease count, then removed the lease entry unconditionally and disposed the evicted mirror. A writer that resolved the mirror before its eviction could take its lease between the read and the dispose: its count went 0 → 1 after the reclaim had read 0, its post-leaseIsUsablere-check still passed, and the reclaim then dropped its entry and disposed the mirror under it. Its base read completed empty —RequireBaseState's "Update aborted: this hub's mirror ended without ever carrying the node's state", the terminal on #5087.The fix
Lease and reclaim now decide over the same dictionary slot by compare-exchange:
TryUpdate(stream, claim, 0), whereclaimis a fresh negative token per reclaim — only an evicted stream, only at zero holders;AcquireRemoteStreamUncheckedresolves again and builds a fresh mirror (the reclaimed one is out of the cache);DetachRemoteStreams+ParkRemoteStreamshandover superseded the claim (review finding), so it restores the parking entry and leaves the stream alone;Dispose; a claim that loses_evictedRemoteStreams.TryRemoveto another owner hands the slot back.No lock, no gate, no bound changed. Unleased streams keep today's conservative parking (the claim fails on a missing entry), so #3432's behaviour is unchanged.
Evidence
test/MeshWeaver.Data.Test/ReclaimLeaseAtomicityTest.cs— deterministic, via theReclaimDecidedseam (runs when the reclaim has decided "unheld"; writer B leases there, same thread):DIAG leaseBGranted=True mirrorUsable=FalseADetachAndRePark_InsideTheClaim_SupersedesTheReclaim, from the review): failed on the shared-sentinel version (re-parked mirror disposed), passes with the tokenEvicted*3/3,*Mirror*17/17,AnnouncedFloor*2/2 (Data.Test),WriteBaseStateTotality*3/3 (Graph.Test)dotnet build -c Release -warnaserror: MeshWeaver.Data, MeshWeaver.Data.Test, MeshWeaver.Documentation — 0 warnings, 0 errorsNot established
The same message has two other producers (a ReplaySubject that never carried a value; the write path's
Where(Value is not null)), byte-identical in the log. This closes the lease/reclaim producer only, so it staysRefs, notFixes: #5087's two production samples cannot be attributed to this producer from the log alone.Docs:
Doc/Architecture/EvictedStreamRetention§10 (new),WriteVerdictTotalitytable row.Pairs-with: none — only internal members of
MeshWeaver.Datachange (LeaseRemoteStream→TryLeaseRemoteStream, new internalTryLeaseResolved/ReclaimClaimed); no public surface.Recycle: none needed — the fix is in the portal binary; takes effect on the next roll.
🤖 Generated with Claude Code