diff --git a/src/MeshWeaver.Messaging.Hub/MessageService.cs b/src/MeshWeaver.Messaging.Hub/MessageService.cs index d30d3ae3b1..7ce454e624 100644 --- a/src/MeshWeaver.Messaging.Hub/MessageService.cs +++ b/src/MeshWeaver.Messaging.Hub/MessageService.cs @@ -810,10 +810,29 @@ private IMessageDelivery ScheduleNotify(IMessageDelivery delivery, CancellationT // path, and ANY terminal treatment killed the sync stream's resubscribe latch β€” // each wedged every read of the mid-recycle NodeType. fate?.Add($"DROPPED_SHUTTING_DOWN runLevel={hub.RunLevel}", Address); - NackThroughParent(delivery, + + // 🚨 Both halves matter, and this site got both wrong until #2350. + // + // NackThroughParent DECLINES (returns false) when there is no parent, or the parent is + // itself past DisposeHostedHubs. Ignoring the result meant: answered β†’ the reporting + // path NACKed a SECOND time, because only FailedAndNacked sets SenderWasNacked; + // declined β†’ the sender got nothing from here at all. In both cases the delivery + // carried NO classification, so ReportFailure fell back to ErrorType.Unavailable while + // the message still read "Hub is shutting down" β€” a transient race wearing an + // authoritative label. + // + // That defeats every consumer written against the documented contract, which is the + // whole reason Failed(string, ErrorType) exists: SynchronizationStream's resubscribe + // latch, MeshNodeStreamCache's shutdown-drop handling and PackageInstaller's retry all + // ride out ShuttingDown and treat anything else as terminal. Same idiom as + // AnswerUnreleasableDelivery above: honour the return value, and classify either way. + var reason = $"Hub {Address} is shutting down (RunLevel={hub.RunLevel}) β€” cannot process " - + $"{typeName}; the address may reactivate (recycle / restart). Rejecting now."); - return delivery.Failed("Hub is shutting down"); + + $"{typeName}; the address may reactivate (recycle / restart). Rejecting now."; + + return NackThroughParent(delivery, reason) + ? delivery.FailedAndNacked("Hub is shutting down") + : delivery.Failed("Hub is shutting down", ErrorType.ShuttingDown); } // STORM CIRCUIT-BREAKER. Detect an unbounded retry/resubscribe/repost loop β€” @@ -1833,7 +1852,13 @@ private IMessageDelivery PostImplGeneric(TMessage message, PostOptions } postFate?.Add($"POST_REFUSED_SHUTTING_DOWN runLevel={hub.RunLevel}", Address); - return ((IMessageDelivery)delivery).Failed("Hub is shutting down"); + + // Classified, for the same reason as the intake gate (#2350): a refused POST during + // shutdown is transient β€” the address may reactivate β€” and an unclassified failure + // reaches the sender as ErrorType.Unavailable, which its recovery machinery reads as + // terminal. This site does NOT answer the sender itself, so Failed (not + // FailedAndNacked) stays correct; only the classification was missing. + return ((IMessageDelivery)delivery).Failed("Hub is shutting down", ErrorType.ShuttingDown); } // TODO V10: Which cancellation token to pass here? (12.01.2025, Roland BΓΌrgi) diff --git a/test/MeshWeaver.Messaging.Hub.Test/TeardownFailureClassificationGuard.cs b/test/MeshWeaver.Messaging.Hub.Test/TeardownFailureClassificationGuard.cs new file mode 100644 index 0000000000..981a8f3e64 --- /dev/null +++ b/test/MeshWeaver.Messaging.Hub.Test/TeardownFailureClassificationGuard.cs @@ -0,0 +1,94 @@ +using System; +using System.IO; +using System.Linq; +using System.Text.RegularExpressions; +using Xunit; + +namespace MeshWeaver.Messaging.Hub.Test; + +/// +/// Every teardown-time failure MessageService produces must carry a CLASSIFICATION β€” either +/// Failed(message, ErrorType), or FailedAndNacked(message) when the site already +/// answered the sender itself. +/// +/// 🚨 Why a source guard rather than a behavioural test. This file's own +/// DeletedAddressMessage comment states the failure mode: "a drift here is SILENT: it does +/// not fail to compile, it quietly re-opens #1029 at whichever site was missed. Which is exactly +/// what happened β€” the fix landed only on the abandoned-delivery site, and the accepted-then-faulted +/// site kept answering the transient ErrorType.ShuttingDown with the raw exception text." The bug +/// class is a site that LOOKS right and is never exercised by the test that covers its sibling. +/// Nothing about it is racy, so pinning it racily would trade a silent bug for a flaky one. +/// +/// What goes wrong when a classification is missing (#2350). The reporting path reads +/// delivery.GetFailureErrorType(ErrorType.Unavailable) β€” so an unclassified failure reaches +/// the sender as Unavailable while its message still says "shutting down": a transient race +/// wearing an authoritative label. Every consumer written against the documented contract then +/// treats it as terminal β€” SynchronizationStream's resubscribe latch, MeshNodeStreamCache's +/// shutdown-drop handling, PackageInstaller's retry β€” which is the precise outcome +/// Failed(string, ErrorType) exists to prevent, in its own words. +/// +/// The guard is deliberately narrow: it only asks about failures whose MESSAGE mentions +/// shutting down / disposing. A genuine bug must stay terminal and unclassified is right for it. +/// +public class TeardownFailureClassificationGuard +{ + /// Failure text that means "teardown", i.e. transient and retryable. + private static readonly string[] TeardownWords = ["shutting down", "shut down", "disposing"]; + + [Fact] + public void EveryTeardownFailure_InMessageService_CarriesAClassification() + { + var source = File.ReadAllText(Path.Combine( + FindRepoRoot(), "src", "MeshWeaver.Messaging.Hub", "MessageService.cs")); + + // `Failed("...")` with a single STRING-LITERAL argument β€” the overload that records no + // ErrorType. Two args (message, ErrorType) and FailedAndNacked(message) both classify, and + // neither matches: the pattern requires the closing paren straight after the literal. + var unclassified = Regex + .Matches(source, """(? m.Groups[1].Value) + .Where(message => TeardownWords.Any(w => message.Contains(w, StringComparison.OrdinalIgnoreCase))) + .Distinct() + .ToArray(); + + Assert.True(unclassified.Length == 0, + "MessageService produces a teardown failure with NO ErrorType. It will reach the sender " + + "as ErrorType.Unavailable (the reporting fallback) while saying it is shutting down, so " + + "consumers that ride out ShuttingDown will treat a recoverable race as terminal (#2350).\n" + + "Use Failed(message, ErrorType.ShuttingDown), or FailedAndNacked(message) if the site " + + "already answered the sender β€” see AnswerUnreleasableDelivery for the idiom.\n" + + "Unclassified:\n - " + string.Join("\n - ", unclassified)); + } + + /// + /// The interpolated form of the same mistake. Kept separate so the failure message can say which + /// shape it found β€” an interpolated literal is easy to add without noticing it took the + /// single-argument overload. + /// + [Fact] + public void EveryInterpolatedTeardownFailure_InMessageService_CarriesAClassification() + { + var source = File.ReadAllText(Path.Combine( + FindRepoRoot(), "src", "MeshWeaver.Messaging.Hub", "MessageService.cs")); + + var unclassified = Regex + .Matches(source, """(? m.Groups[1].Value) + .Where(message => TeardownWords.Any(w => message.Contains(w, StringComparison.OrdinalIgnoreCase))) + .Distinct() + .ToArray(); + + Assert.True(unclassified.Length == 0, + "MessageService produces an interpolated teardown failure with NO ErrorType (#2350). " + + "Use Failed(message, ErrorType.ShuttingDown) or FailedAndNacked(message).\n" + + "Unclassified:\n - " + string.Join("\n - ", unclassified)); + } + + private static string FindRepoRoot() + { + var dir = new DirectoryInfo(AppContext.BaseDirectory); + while (dir is not null && !File.Exists(Path.Combine(dir.FullName, "MeshWeaver.slnx"))) + dir = dir.Parent; + return dir?.FullName ?? throw new InvalidOperationException("repo root not found"); + } +}