From f74f3b68acd4e266d2ec0a0da7e60bd184e339bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20B=C3=BCrgi?= <6334612+rbuergi@users.noreply.github.com> Date: Tue, 22 Sep 2026 19:50:13 +0200 Subject: [PATCH] fix(#4866): the disposal drain's NACK now reads transient to the riders 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) --- .../Data/Architecture/RetiringAnActivation.md | 17 +++- .../RidingOutAShuttingDownAddress.md | 58 ++++++++++++ .../MessageService.cs | 30 +++++-- .../OwnerAnswerRecognitionGuard.cs | 17 +++- .../DeferredDeliveryNackedOnDisposeTest.cs | 89 +++++++++++++++++++ .../FailedGateAnswersBeforeTheTeardownTest.cs | 8 +- 6 files changed, 207 insertions(+), 12 deletions(-) diff --git a/src/MeshWeaver.Documentation/Data/Architecture/RetiringAnActivation.md b/src/MeshWeaver.Documentation/Data/Architecture/RetiringAnActivation.md index e93f6067af..7d414655dd 100644 --- a/src/MeshWeaver.Documentation/Data/Architecture/RetiringAnActivation.md +++ b/src/MeshWeaver.Documentation/Data/Architecture/RetiringAnActivation.md @@ -39,8 +39,8 @@ thread**. So on the `DataContext` seam the two live on different threads, and both of them drain the *same* deferred backlog through `MessageService.DrainDeferredDeliveries`: -- the teardown's drain answers *"Hub X was disposed while <message> was still deferred — the - message was never processed"*; +- the teardown's drain answers *"Hub X is shutting down (…) — <message> was still deferred; + initialization gates closed at deferral: […] — the message was never processed"*; - `FailGate` → `FailDeferredBacklog` answers with the transient cause. `DrainDeferredDeliveries` claims each entry with `TryRemove`, so exactly one of them answers any @@ -118,8 +118,17 @@ Hub host/1 is shutting down — its DataContext initialization met a transient i reactivate (recycle / restart); retry to get the authoritative answer. ``` -If instead you read *"…was disposed while X was still deferred … the message was never processed"*, -the retirement lost its own race: something failed the gate after the teardown had begun. +If instead you read *"…X was still deferred; initialization gates closed at deferral: […] — the +message was never processed"*, the retirement lost its own race: something failed the gate after the +teardown had begun. + +🚨 **Tell them apart by the CLAUSE, never by the banner.** Since #4866 the teardown's drain composes +through `ShutdownNack.RetryForTheAuthoritativeAnswer` too — it was the last owner-side refusal that +did not, and the omission made it unrecognisable to every transient classifier, which is +[the defect that page records](/Doc/Architecture/RidingOutAShuttingDownAddress). So +`IsAnsweredByOwner` is now true of *both* answers, as it should be: both really are this owner +speaking. What separates them is what they say — a named fault, or a delivery that was still parked +behind its gates. ## Related diff --git a/src/MeshWeaver.Documentation/Data/Architecture/RidingOutAShuttingDownAddress.md b/src/MeshWeaver.Documentation/Data/Architecture/RidingOutAShuttingDownAddress.md index 39ebb5731a..75c070d100 100644 --- a/src/MeshWeaver.Documentation/Data/Architecture/RidingOutAShuttingDownAddress.md +++ b/src/MeshWeaver.Documentation/Data/Architecture/RidingOutAShuttingDownAddress.md @@ -224,6 +224,64 @@ answer that was already in the log. It is #1029's silence on the door #1029 did intermittent only because the owner's own teardown notice usually reaches the reader before the change-feed event latches the re-ask. +## 🚨 The promise lives in the TEXT, not in the envelope (#4866) + +Everything above assumes the rider can *tell* that it was handed the promise. It cannot do that from +`ErrorType`. Every classifier that decides "re-probe the fresh activation" against "take this answer +as final" reads the **message text**: + +| Classifier | Where | Reads | +|---|---|---| +| `MeshNodeStreamCache.IsTransientOwnerFailure` | `MeshWeaver.Hosting` | `"is shutting down"`, `"Rejecting now"`, `"invalid activation"`, … | +| `AreaErrorClassifier.IsTransientHubFailure` / `IsHubRecycling` | `MeshWeaver.Layout` | the same markers | +| `OrleansRoutingService.ClassifyRoutedFailure` | `MeshWeaver.Connection.Orleans` | the same markers | +| `ShutdownNack.IsAnsweredByOwner` | `MeshWeaver.Messaging.Contract` | `ShutdownNack.Banner` — *"Hub X is shutting down"* | + +`ShutdownNack.IsAnsweredByOwner` is deliberately **not** an `ErrorType` test, and its own doc comment +says why: the routing layer mints that same classification off the very same text, so an +`ErrorType` test answers the question with the routing layer's echo of it. The banner is the only +evidence that names the speaker. + +So an owner-side refusal that is *stamped* `ErrorType.ShuttingDown` but *worded* without the banner +is a promise nobody can collect. The envelope is transient, every reader is terminal, and there is +nothing to grep: no compiler error, no exception, no log line — just a consumer that quietly stops +asking. + +**That is what the disposal drain did.** The seam that answers deliveries still parked behind an +initialization gate when the hub goes down hand-wrote its sentence — *"Hub X **was disposed** while +GetDataRequest (id=…) was still deferred …"* — instead of composing it through `ShutdownNack`. It +carried no banner and matched no marker, so `IsAnsweredByOwner` reported *"the routing layer refused +you"* and every rider took a recycling hub's answer as final. Measured in #4866: one pod, one +sub-second window, **105** accepted-and-deferred `GetDataRequest`/`SubscribeRequest` deliveries +discarded when every hub bound to a `Store/Plugin` overlay self-healed at once — each one answered +with a sentence that told the sender to *"retry to get the authoritative answer"* in wording the +sender could not recognise as retryable. The recycle itself was correct; the answer to the work it +displaced was not. + +The fix is the seam, not a new drain: `ShutdownNack.RetryForTheAuthoritativeAnswer` is 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**"* — and it carries the +banner and the activation tag by construction. Every fact the old sentence had (the gates recorded at +deferral, who asked for the teardown, the reason they stated) is preserved inside its `what` clause. + +**The general rule: an owner-side refusal is COMPOSED by `ShutdownNack`, never written at the call +site.** A hand-written one looks right in review — it names the address, it explains itself, it even +ends with the retry sentence — and it is invisible to every reader that has to act on it. + +**And the guard over the producers is an enumeration, which is the same trap one level up.** +`OwnerAnswerRecognitionGuard` calls each producer and runs the real predicate over what it says, but +the producer *list* is written by hand: #3017 was filed because a fifth terminal was missing from +such a list, a sixth was missing at the same time, and the class's own guard passed throughout. This +drain was the seventh, and it stayed unlisted through both. Adding a refusal seam means adding it +there — and moving the floor in `TheProducerSetCoversEverySeam`, so the shrink is a decision rather +than an accident. + +Pinned live by `DeferredDeliveryNackedOnDisposeTest.ADiscardedDeferredDelivery_IsRecognisedAsTheOwnersAnswer`, +which 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 on it — with a different +address as the negative control, and the gate name and stated reason as positive controls so a +rewording cannot trade one fact for another. + Since #3498 the re-ask arm mirrors the initial one: `ShuttingDown` is still the promise it rides out; any other classification faults the stream's subscribers with the classified error and disposes the keep-alive. A reader of a deleted node learns "gone" milliseconds after the delete, and a stream never diff --git a/src/MeshWeaver.Messaging.Hub/MessageService.cs b/src/MeshWeaver.Messaging.Hub/MessageService.cs index c59df11a8c..09b0254580 100644 --- a/src/MeshWeaver.Messaging.Hub/MessageService.cs +++ b/src/MeshWeaver.Messaging.Hub/MessageService.cs @@ -3345,11 +3345,31 @@ public void Dispose() + "this hub went down with work still parked behind its gates.", Address, delivery.Message.GetType().Name, delivery.Id, delivery.Sender, gatesAtDeferral, hub.RunLevel, disposal); - NackThroughParent(delivery, - $"Hub {Address} was disposed while {delivery.Message.GetType().Name} " - + $"(id={delivery.Id}) was still deferred; initialization gates closed at deferral: " - + $"[{gatesAtDeferral}] — the message was never processed. The teardown was {disposal}. " - + "The address may reactivate (recycle / restart); retry to get the authoritative answer."); + // 🚨 COMPOSED THROUGH ShutdownNack, never hand-written (#4866). This was the one + // ShuttingDown NACK this service minted without the seam, and the cost was invisible: + // the ENVELOPE said transient (NackThroughParent stamps ErrorType.ShuttingDown) while + // the TEXT said nothing of the kind, and the three classifiers that decide "re-probe + // the fresh activation" vs "take this answer as final" read the TEXT — + // MeshNodeStreamCache.IsTransientOwnerFailure, AreaErrorClassifier.IsTransientHubFailure + // /IsHubRecycling and OrleansRoutingService.ClassifyRoutedFailure. "Hub X was disposed + // while …" matches none of their markers and carries no ShutdownNack.Banner, so + // ShutdownNack.IsAnsweredByOwner answered "the routing layer refused you". Every + // consumer with recovery machinery therefore took a recycling hub's answer as terminal + // and did NOT ask again — so a delivery this hub ACCEPTED and threw away was genuinely + // lost, which is what makes the line above an Error rather than teardown noise. The + // sentence ends "retry to get the authoritative answer" and the readers it says that to + // could not tell it was retryable. + // + // 🚨 ActivationTag() rides here for the same reason it rides on the late-turn site: a + // consumer re-probing this address must tell ONE hub wedged in teardown from a recycle + // storm (#2025), and the per-DELIVERY id right after it varies on every retry against + // the SAME activation. + NackThroughParent(delivery, ShutdownNack.RetryForTheAuthoritativeAnswer( + Address, + $"RunLevel={hub.RunLevel}, {ActivationTag()}", + $"{delivery.Message.GetType().Name} (id={delivery.Id}) was still deferred; " + + $"initialization gates closed at deferral: [{gatesAtDeferral}] — the message was " + + $"never processed. The teardown was {disposal}")); }); // No buffers to Complete — ScheduleNotify drops post-shutdown messages and the diff --git a/test/MeshWeaver.Hosting.Test/OwnerAnswerRecognitionGuard.cs b/test/MeshWeaver.Hosting.Test/OwnerAnswerRecognitionGuard.cs index 5cd5ae748b..7d8cea3721 100644 --- a/test/MeshWeaver.Hosting.Test/OwnerAnswerRecognitionGuard.cs +++ b/test/MeshWeaver.Hosting.Test/OwnerAnswerRecognitionGuard.cs @@ -66,6 +66,21 @@ private static ObjectDisposedException DisposedScope() => "GetDataRequest (id=abc) was accepted before disposal began and its turn came " + "too late to process")); + // The SEVENTH terminal, and the one #4866 was filed on: the disposal drain, which answers + // every delivery still parked behind an initialization gate when the hub goes down. It + // hand-wrote its sentence ("Hub X was disposed while …") instead of composing it here, so + // it carried no banner and matched no transient marker — the envelope said ShuttingDown + // while the text read terminal, and the consumers decide on the text. Listed here so the + // shrink that hid a producer for six terminals cannot hide it for a seventh. + yield return ("MessageService disposal drain (ShutdownNack.RetryForTheAuthoritativeAnswer)", + ShutdownNack.RetryForTheAuthoritativeAnswer( + Owner, + $"RunLevel=ShutDown, {ShutdownNack.ActivationMarker}DEADBEEF", + "GetDataRequest (id=abc) was still deferred; initialization gates closed at " + + "deferral: [DataContextInit,MeshNodeInit] — the message was never processed. " + + "The teardown was requested by mesh/1 (routed DisposeRequest); why: " + + "Overlay self-heal")); + // The typed refusal a handler throws, both constructors — they build the sentence // independently of each other. yield return ("HubDisposingException(address, what)", @@ -173,7 +188,7 @@ public void ARoutingLayerFailure_IsNotTheOwnersAnswer(string message, string bec [Fact] public void TheProducerSetCoversEverySeam() { - OwnerAnswers().Should().HaveCountGreaterThanOrEqualTo(7, + OwnerAnswers().Should().HaveCountGreaterThanOrEqualTo(8, "one case per owner-side seam, plus the relayed shape — if a producer was genuinely " + "consolidated away, lower this number deliberately rather than letting the coverage " + "drop unnoticed"); diff --git a/test/MeshWeaver.Messaging.Hub.Test/DeferredDeliveryNackedOnDisposeTest.cs b/test/MeshWeaver.Messaging.Hub.Test/DeferredDeliveryNackedOnDisposeTest.cs index 9fba5760e6..a03799e26e 100644 --- a/test/MeshWeaver.Messaging.Hub.Test/DeferredDeliveryNackedOnDisposeTest.cs +++ b/test/MeshWeaver.Messaging.Hub.Test/DeferredDeliveryNackedOnDisposeTest.cs @@ -360,6 +360,95 @@ public async Task ATeardownWhoseReasonIsBlank_ReportsItAsUNSTATED_NotAsNothing() "normalising the reason must not cost the sender the attribution it travels with"); } + private static readonly Address RecognisedAddress = new("recognised", "1"); + + /// + /// 🚨 Systemorph/MeshWeaver#4866 — the discard NACK told the sender to "retry to get the + /// authoritative answer" in a sentence no reader could tell was retryable. + /// + /// The defect. This site hand-wrote its refusal ("Hub X was disposed + /// while …") instead of composing it through , so it opened with + /// no . Three independent classifiers decide retryability by + /// MESSAGE TEXT rather than by — + /// MeshNodeStreamCache.IsTransientOwnerFailure, + /// AreaErrorClassifier.IsTransientHubFailure/IsHubRecycling and + /// OrleansRoutingService.ClassifyRoutedFailure — and none of their markers appears in + /// that sentence. So the transient the test above pins was + /// carried by an envelope the consumers do not read, and every consumer with recovery + /// machinery took the corpse's answer as FINAL: the cache did not re-probe, the layout area did + /// not re-arm, and the accepted work was genuinely lost rather than re-asked. That is the + /// difference between the 105 discards of #4866 being retryable noise and being 105 requests + /// nobody ever answers. + /// + /// Why this is not the ErrorType assertion again. + /// is deliberately NOT an ErrorType test — the + /// routing layer mints that same classification off the same text, so an ErrorType test answers + /// the question with the routing layer's echo of it. The banner is the only evidence that + /// identifies the speaker, and the drain was the one producer in this service not emitting it. + /// OwnerAnswerRecognitionGuard pins the same contract over every producer's sentence; + /// this test is the LIVE arm, over the delivery the production path actually discards. + /// + [Fact] + public async Task ADiscardedDeferredDelivery_IsRecognisedAsTheOwnersAnswer() + { + var host = GetHost(); + + var gated = host.GetHostedHub( + RecognisedAddress, + c => c.WithTypes(typeof(GatedRequest), typeof(GatedResponse)) + .WithInitializationGate("recognised-gate-never-opens", _ => false) + .WithHandler((h, d) => + { + h.Post(new GatedResponse(), o => o.ResponseFor(d)); + return d.Processed(); + })); + gated.Should().NotBeNull(); + + var response = host + .Observe(new GatedRequest(), o => o.WithTarget(RecognisedAddress)) + .FirstAsync() + .Await(TestContext.Current.CancellationToken); + + await WaitForDeferredBacklog(host); + + host.Post(new DisposeRequest { Reason = TheStatedReason }, + o => o.WithTarget(RecognisedAddress)); + + var failure = await Assert.ThrowsAsync(() => response); + failure.Failure.Should().NotBeNull(); + Output.WriteLine(failure.Failure!.Message ?? "(no message)"); + + ShutdownNack.IsAnsweredByOwner(failure.Failure.Message, RecognisedAddress).Should().BeTrue( + "the hub that discarded the delivery IS the speaker, and every consumer that decides " + + "'retry the fresh activation' vs 'take this as final' reads the banner to know it — " + + "compose the refusal through ShutdownNack rather than widening the predicate"); + ShutdownNack.IsAnsweredByOwner(failure.Failure.Message, RecognisedAddress.Path) + .Should().BeTrue( + "a reader that resolved the node by PATH holds nothing else, and the two halves " + + "must answer identically or recognition is a coin toss on which one is at hand"); + + ShutdownNack.ExtractActivationTag(failure.Failure.Message).Should().NotBeNullOrEmpty( + "a consumer re-probing this address cannot otherwise tell ONE hub wedged in teardown " + + "from a recycle storm — those have opposite fixes (#2025), and every ShuttingDown " + + "NACK minted for a delivery the reader can re-probe carries the activation tag"); + + // NEGATIVE CONTROL — the predicate must still be able to say no, or the assertions above + // are satisfied by anything that mentions an address. + ShutdownNack.IsAnsweredByOwner(failure.Failure.Message, new Address("recognised", "2")) + .Should().BeFalse( + "a DIFFERENT address is not this owner answering; if this passes the refusal is " + + "being recognised by something other than its own banner"); + + // POSITIVE CONTROLS — the facts the refusal already carried are ADDED to, never traded + // against, which is how a rewrite of this sentence reads as a fix while losing evidence. + failure.Failure.Message.Should().Contain("recognised-gate-never-opens", + "the gate that actually held the delivery (#3712/#3789) survives the rewording"); + failure.Failure.Message.Should().Contain(TheStatedReason, + "and so does WHY the teardown happened, as its poster stated it"); + failure.Failure.ErrorType.Should().Be(ErrorType.ShuttingDown, + "the envelope was never the broken half — it must stay transient too"); + } + /// /// Polls the public disposal diagnostics (which report deferred=<N> per hub, /// walking hosted hubs) until something is parked. The gated hub is the only hub in this test diff --git a/test/MeshWeaver.Messaging.Hub.Test/FailedGateAnswersBeforeTheTeardownTest.cs b/test/MeshWeaver.Messaging.Hub.Test/FailedGateAnswersBeforeTheTeardownTest.cs index b71ab28a06..4da8312046 100644 --- a/test/MeshWeaver.Messaging.Hub.Test/FailedGateAnswersBeforeTheTeardownTest.cs +++ b/test/MeshWeaver.Messaging.Hub.Test/FailedGateAnswersBeforeTheTeardownTest.cs @@ -107,9 +107,13 @@ public async Task AfterTheTeardown_FailGateCanNoLongerNameTheCause() failure.Failure!.Message.Should().NotContain("transient infrastructure fault", "this is the loss #4261 names: the requester is told the generic disposal sentence " + "instead of the cause the retirement exists to carry"); - failure.Failure.Message.Should().Contain("was disposed while", + failure.Failure.Message.Should().Contain("was still deferred", "the generic sentence is what it does get — asserted positively so 'the cause is " - + "missing' cannot be satisfied by the requester hearing nothing at all"); + + "missing' cannot be satisfied by the requester hearing nothing at all. The clause " + + "is the drain's own ('… was still deferred; initialization gates closed at " + + "deferral: …'), not its opening: since #4866 the drain composes through " + + "ShutdownNack, so the sentence now OPENS with the owner banner every transient " + + "classifier reads, and the old literal 'was disposed while' is gone"); } ///