Repository navigation
fix(#4866): the disposal drain's NACK now reads transient to the riders it tells to retry - #5272
Conversation
…rs it tells to retry MessageService.Dispose answers every delivery still parked behind an initialization gate, stamps it ErrorType.ShuttingDown, and ends the sentence "retry to get the authoritative answer". It was the one ShuttingDown NACK this service minted without composing through ShutdownNack, so the sentence carried no Banner, no transient marker and no activation tag: Hub recognised/1 was disposed while GatedRequest (id=...) was still deferred; initialization gates closed at deferral: [...] - the message was never processed. The teardown was requested by host/1 ... Retryability is decided by MESSAGE TEXT, not by ErrorType - three classifiers read it (MeshNodeStreamCache.IsTransientOwnerFailure, AreaErrorClassifier.IsTransientHubFailure/IsHubRecycling, OrleansRoutingService.ClassifyRoutedFailure) and a fourth reads the banner (ShutdownNack.IsAnsweredByOwner, deliberately not an ErrorType test because the routing layer mints that classification off the same text). None of them matched, so every consumer with recovery machinery took a recycling hub's answer as FINAL and did not ask again. The envelope said transient; every reader said terminal. That is why #4866's 105 accepted-and-deferred deliveries were lost rather than re-asked when every Store/Plugin-bound hub self-healed at once. Compose it through ShutdownNack.RetryForTheAuthoritativeAnswer - documented for exactly this case, "a gate that can never open" - carrying every fact the old sentence had inside its `what` clause, plus ActivationTag() so a re-probe rider can still tell one wedged activation from a recycle storm (#2025). Not "drain before dispose" and not "re-post to the successor": the third arm, the platform's own ride-out contract, is the one that was broken. Waiting for gates can wait forever (a hub serving a fallback card is exactly the state whose gates may never open - #1814), and re-posting from inside Dispose duplicates the sender's re-ask with a ping-pong hazard. Control on both sides: ADiscardedDeferredDelivery_IsRecognisedAsTheOwnersAnswer discards a real deferred delivery and requires the refusal the sender actually receives to be recognised as the owner's, by Address and by path, with an activation tag - failing on the unfixed tree with the text above printed, passing with the fix. A different address is the negative control; the gate name and the stated reason are positive controls so the rewording cannot trade one fact for another. Full MeshWeaver.Messaging.Hub.Test: 504 passed, 0 failed. OwnerAnswerRecognitionGuard's producer list is hand-written - #3017's own trap one level up - and this drain was the seventh terminal missing from it. Added, floor moved 7 -> 8. FailedGateAnswersBeforeTheTeardownTest asserted the old opening literal positively; it now asserts the drain's own surviving clause. Doc: Riding Out a ShuttingDown Address gains "The promise lives in the TEXT, not in the envelope"; Retiring an Activation's two quotes of the old sentence are corrected, with the note that the two drains are now told apart by their CLAUSE rather than by the banner, which both carry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Reviewer-identified documentation and comment accuracy issues remain unresolved.
Review effort: Lite
Findings: None
What changed in this PR
This PR makes deferred-disposal NACKs use the shared transient ShutdownNack format so affected deliveries remain retryable.
Changes:
- Adds activation-tagged disposal NACKs.
- Expands regression and producer-coverage tests.
- Updates lifecycle and retry-classification documentation.
| File | Summary |
|---|---|
test/MeshWeaver.Messaging.Hub.Test/FailedGateAnswersBeforeTheTeardownTest.cs |
Updates expected disposal wording. |
test/MeshWeaver.Messaging.Hub.Test/DeferredDeliveryNackedOnDisposeTest.cs |
Verifies owner recognition and preserved context. |
test/MeshWeaver.Hosting.Test/OwnerAnswerRecognitionGuard.cs |
Covers the disposal producer seam. |
src/MeshWeaver.Messaging.Hub/MessageService.cs |
Composes structured disposal NACKs. |
src/MeshWeaver.Documentation/Data/Architecture/RidingOutAShuttingDownAddress.md |
Documents classification and retry behavior. |
src/MeshWeaver.Documentation/Data/Architecture/RetiringAnActivation.md |
Updates retirement examples and drain wording. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Test Results (shard 0) 1 files 1 suites 2m 51s ⏱️ Results for commit f74f3b6. |
Test Results (shard 3)429 tests 429 ✅ 1m 1s ⏱️ Results for commit f74f3b6. |
Test Results (shard 1)536 tests 536 ✅ 1m 2s ⏱️ Results for commit f74f3b6. |
Test Results (shard 4) 2 files 2 suites 6m 33s ⏱️ Results for commit f74f3b6. |
Test Results (shard 2)1 579 tests 1 579 ✅ 9m 6s ⏱️ Results for commit f74f3b6. |
Two agents reached the same root independently: #5272 from issue #4866, this branch from #4888. #5272 was first, is narrower, and covers three things this half did not — the 105-discard production measurement, the OwnerAnswerRecognitionGuard extension that closes the enumeration blind spot (the drain was its seventh unlisted terminal), and the Address-vs-path pair in the live test. So it owns MessageService.cs, both Messaging.Hub tests and the RidingOutAShuttingDownAddress page; those four files are restored to main here and there is no overlap left between the two pull requests. What remains on this branch is what is uniquely its own: the OrleansRoutingService pooled-leaf token projection, PooledLeafObservesItsTokenGuard, and the ControlledIoPooling / DisposedScopeAndDyingHubs pages. The one assertion this half had that #5272 lacks — Contain(FormatActivationTag(hub)), the EXACT activation identity rather than 'some hex is present', which is the #2376 drift in the other direction — has been handed to that author rather than merged here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Test Results (shard 5) 5 files 5 suites 14m 27s ⏱️ Results for commit f74f3b6. |
Test Results 13 files 13 suites 35m 2s ⏱️ Results for commit f74f3b6. |
Fixes #4866.
The defect
MessageService.Disposeanswers every delivery still parked behind an initialization gate, stamps itErrorType.ShuttingDown, and ends the sentence "retry to get the authoritative answer". It was theone
ShuttingDownNACK this service minted without composing throughShutdownNack. Capturedfrom the real path by a test run against the unfixed tree:
No
ShutdownNack.Banner, no transient marker, no activation tag.Retryability is decided by MESSAGE TEXT, not by
ErrorType. Three classifiers read it —MeshNodeStreamCache.IsTransientOwnerFailure,AreaErrorClassifier.IsTransientHubFailure/IsHubRecycling,OrleansRoutingService.ClassifyRoutedFailure— and a fourth reads the banner,ShutdownNack.IsAnsweredByOwner, which is deliberately not anErrorTypetest because therouting layer mints that same classification off the same text. None of them matched this sentence.
So the envelope said transient and every reader said terminal: each consumer with recovery machinery
took a recycling hub's answer as FINAL and did not ask again. That is why #4866's 105
accepted-and-deferred
GetDataRequest/SubscribeRequestdeliveries were lost rather than re-askedwhen every
Store/Plugin-bound hub self-healed at once — and it is why that line is anErrorratherthan teardown noise.
It is silent by construction: nothing fails to compile, the delivery is still refused, the log line is
still written.
MessageService's own header already states the rule it broke — "Every ShuttingDownNACK this service mints for a delivery the reader can re-probe must call this" (of
ActivationTag()).The fix
Compose it through
ShutdownNack.RetryForTheAuthoritativeAnswer, documented for exactly this case —"work this hub ACCEPTED and can no longer finish — a queued turn that came too late, machinery that
can no longer be created, a gate that can never open" — carrying every fact the old sentence had
(the gates recorded at deferral, who asked for the teardown, the reason they stated) inside its
whatclause, plus
ActivationTag()so a re-probe rider can still tell one wedged activation from a recyclestorm (#2025).
Not "drain before dispose" and not "re-post to the successor" — the two arms #5053 named. The
third arm, the platform's own ride-out contract, is the one that was broken; restoring it is the root
cause. The other two carry hazards #4866's own "What I did NOT establish" already flagged: waiting
for gates can wait forever (a hub serving a fallback card is exactly the state whose gates may never
open — #1814 is the outage the self-heal recycle exists for), and re-posting from inside
Disposeduplicates the sender's re-ask with a ping-pong hazard.
Control on both sides
DeferredDeliveryNackedOnDisposeTest.ADiscardedDeferredDelivery_IsRecognisedAsTheOwnersAnswerdiscards a REAL deferred delivery and requires the refusal the sender actually receives to be
recognised as the owner's — by
Addressand by path — with an activation tag on it.failed … Expected value to be true …, but found False, with the text aboveprinted by the test.
MeshWeaver.Messaging.Hub.Test: 504 passed, 0failed.
tree:
MeshWeaver.Hosting.Test934 passed / 1 skipped / 0 failed (ownsIsTransientOwnerFailureandOwnerAnswerRecognitionGuard),MeshWeaver.Layout.Test521 /0 (owns
AreaErrorClassifier),MeshWeaver.Data.Test536 / 0 (owns the sync stream'sre-arm latch). 2,495 passed, 0 failed across the four.
satisfied by anything that merely mentions an address.
cannot trade one fact for another.
ErrorTypemust stayShuttingDown— the envelope was never thebroken half.
The guard that should have caught it
OwnerAnswerRecognitionGuardcalls each real producer and runs the real predicate over what it says —but the producer list is hand-written, which is #3017's own trap one level up ("a guard over an
enumeration can only assert the members that were already written down": a fifth terminal was
missing, a sixth was missing at the same time, and the class's own guard passed throughout). This
drain was the seventh, unlisted through both. It is now in the enumeration, and
TheProducerSetCoversEverySeam's floor moves 7 → 8 — which also puts it under that guard'sIsTransientOwnerFailureassertion.FailedGateAnswersBeforeTheTeardownTestasserted the old opening literal positively; it now assertsthe drain's own surviving clause, with the reason recorded in the
because.Docs
envelope" — the four classifiers in a table, the general rule that an owner-side refusal is
COMPOSED by
ShutdownNackand never written at the call site, and the note that the producer guardis an enumeration so adding a seam means adding it there.
the two drains are now told apart by their CLAUSE rather than by the banner, which both carry.
Scope
src/only — this compiles in CI. No in-mesh source, no public surface removed, no interface memberadded, no i18n catalog change.
Pairs-with: none — no public type or member leaves
src/; the change is inside a private method body.🤖 Generated with Claude Code