Repository navigation
fix(#2756): a transient unserved verdict must not evict the owner's server-side stream - #2758
Merged
Merged
Conversation
…erver-side stream #2745 widened HandleTargetUnservedFailure's gate to the TargetUnserved stamp alone, on the reasoning that keeping the ErrorType test would make the router's new verdict inert. An inert eviction is the CORRECT answer for that verdict: ShuttingDown is the platform's transient, ride-it-out classification, and JsonSynchronizationStream handles it by RE-ARMING rather than re-subscribing (JsonSynchronizationStream.cs:473-484, :648-652). So an owner that evicts on it disposes the server-side half of a subscription whose other half is deliberately waiting for it. Main went red on 3376da2 with ObservableQueryTests.ObserveQuery_EmitsRemovedOnDeletedNode timing out, where both halves share a process and the eviction wins the race — but the defect is not co-location, it is the two sides disagreeing about what the verdict means. The stamp still decides whether the delivery is OURS to act on (that half of #2745 stays and is its real insight); the verdict now decides whether the action is to evict or to leave the stream alone. The guard is an explicit ShuttingDown early-return, commented so a future transient ErrorType is added there rather than falling through — the default is to evict, and eviction is irreversible for a subscriber that never re-asks. UnservedVerdictEvictionTest's transient case inverts accordingly, and its assertion is now a POSITIVE reactive signal instead of an elapsed window: after the verdict the owner writes a change, and the test waits for that change to reach the client's mirror — which can only happen over the stream the eviction must not have destroyed. The previous helper bridged Observable.Timer with .ToTask() and would have passed before the verdict was even handled. ATerminalUnservedVerdict_StillEvicts is untouched. Non-vacuity: removing the guard fails 1 of 3; restoring it passes 3/3. Reported by meshweaver-61 as #2756. Closes #2756 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EPCWramjFYquX5SaMdeJKW
Contributor
There was a problem hiding this comment.
Pull request overview
Adjusts owner-side handling of DeliveryFailure.TargetUnserved so that transient “unserved” verdicts (ErrorType.ShuttingDown) no longer evict the owner’s server-side stream, preventing subscribers that re-arm (rather than re-subscribe) from getting stuck waiting on a stream the owner destroyed.
Changes:
- Update
HandleTargetUnservedFailureto treatErrorType.ShuttingDownas transient and avoid eviction. - Update
UnservedVerdictEvictionTesttransient case to assert survivability via a positive “change reaches mirror” signal rather than a timing window.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/MeshWeaver.Data/DataExtensions.cs |
Avoids owner-side stream eviction on transient TargetUnserved verdicts (ShuttingDown). |
test/MeshWeaver.Data.Test/UnservedVerdictEvictionTest.cs |
Reworks the transient-verdict test to prove the stream remains usable by verifying a post-verdict update reaches the client mirror. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Test Results (shard 4) 10 files ±0 10 suites ±0 5m 44s ⏱️ +5s Results for commit afec19f. ± Comparison against base commit 6551e64. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both. |
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
Test Results 41 files ±0 41 suites ±0 34m 11s ⏱️ - 1m 36s Results for commit afec19f. ± Comparison against base commit 6551e64. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both. |
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.
Closes #2756. Main is red on
3376da2a2and CD is publishing nothing, so this is the unblock.What broke
#2745 (mine) widened
DataExtensions.HandleTargetUnservedFailure's gate fromTargetUnserved && ErrorType == NotFoundto the stamp alone, reasoning that keeping the ErrorType test would make the router's new verdict inert here and re-open the #2426/#2546 fan-out-to-a-corpse leak.That reasoning is backwards for this verdict. An inert eviction is the correct answer for a transient one:
ShuttingDownis the platform's ride-it-out classification — the router now stampsTargetUnservedbeside it when the owner is mid-roll or its pod-hub claim has not landed;JsonSynchronizationStreamhandlesShuttingDownby RE-ARMING, not re-subscribing (JsonSynchronizationStream.cs:473-484,:648-652);ObservableQueryTests.ObserveQuery_EmitsRemovedOnDeletedNodeis where it surfaced — both halves share a process there, so the eviction wins the race — but co-location is only what made it deterministic, not the defect.The fix
The stamp still decides whether the delivery is ours to act on — that half of #2745 is its real insight and stays. The verdict now decides whether the right action is to evict or to leave the stream alone: an explicit
ErrorType.ShuttingDownearly-return, commented so a future transient type must be added there rather than falling through. The default stays evict, and eviction is the irreversible branch for a subscriber that never re-asks.The leak #2426/#2546 closes is a subscriber whose process is gone — the terminal verdict — and only that one may evict.
Tests
UnservedVerdictEvictionTest's transient case inverts toATransientUnservedVerdict_LeavesTheOwnersServerSideStreamAlone, and its assertion is a positive reactive signal rather than an elapsed window: after the verdict the owner writes a change and the test waits for it to reach the client's mirror — which can only happen over the stream the eviction must not have destroyed. The previous helper bridgedObservable.Timerwith.ToTask()and would have passed before the verdict was even delivered.ATerminalUnservedVerdict_StillEvictsis untouched.Non-vacuity: removing the guard fails 1 of 3; restoring it passes 3/3.
dotnet build -c Release -warnaserror→0 Warning(s), 0 Error(s).Reported by a peer session as #2756 with the correct mechanism; the
JsonSynchronizationStreamre-arm evidence is what settles which side is right.🤖 Generated with Claude Code
https://claude.ai/code/session_01EPCWramjFYquX5SaMdeJKW