diff --git a/src/MeshWeaver.Hosting/MeshNodeStreamCache.cs b/src/MeshWeaver.Hosting/MeshNodeStreamCache.cs index b7b29cebc..f093ad960 100644 --- a/src/MeshWeaver.Hosting/MeshNodeStreamCache.cs +++ b/src/MeshWeaver.Hosting/MeshNodeStreamCache.cs @@ -616,9 +616,22 @@ private Entry CreateEntry(string p) // 5 min cap (the atioz `_Activity` / missing-node NotFound resubscribe storm // that saturated the action block and tripped the liveness probe). A real // resolution is a non-null node; nothing else clears the breaker. + // + // 🚨 Record ONLY a genuine missing-node failure — never a TRANSIENT owner miss. + // A per-node grain that Orleans idle-collected answers the next read's + // SubscribeRequest with a transient reactivation reject ("Forwarding failed … + // to invalid activation. Rejecting now.") or a 60 s request timeout. Poisoning + // the negative cache with THAT made the storm-breaker replay the raw Orleans + // reject to every reader for the backoff window AND refuse to re-probe the grain + // that already reactivated — so navigating to a just-idle page crashed until a + // manual reload outlasted the window. The node exists; it is momentarily + // unreachable. Forward the error to THIS subscriber (its caller / the area's + // transient classifier retries) but leave the cache clean so the very next read + // re-probes the fresh activation. Symmetric with the write path (see the + // [UpdateQueue] FAILED branch), which already guards RecordNegative the same way. var bookkeeping = inner.AsObservable().Subscribe( node => { if (node is not null) _negative.TryRemove(p, out _); }, - ex => RecordNegative(p, ex)); + ex => { if (IsMissingNodeFailure(ex)) RecordNegative(p, ex); }); var disposal = new System.Reactive.Disposables.CompositeDisposable(hydrationSub, bookkeeping); // Store the disposal on the Entry so the mesh hub's pre-Quiescing // disposal hook (registered in the ctor) can cancel it — and so the @@ -805,13 +818,70 @@ private void RecordNegative(string path, Exception error) /// /// True when an owner failure means the node/hub does not exist (NotFound / activation - /// failed) — the only failure class the storm-breaker suppresses on the WRITE path. RLS - /// denials and transient routing errors are excluded so an existing-but-busy node is never - /// falsely blocked from writes. + /// failed) — the only failure class the storm-breaker suppresses. RLS denials and + /// transient routing errors are excluded so an + /// existing-but-busy (or a just-idle-collected, mid-reactivation) node is never falsely + /// blocked from reads OR writes. + /// + /// 🚨 Transient takes PRECEDENCE. A grain that idle-collected and is mid- + /// DeactivateOnIdle answers the next delivery with an Orleans forwarding-reject + /// ("Forwarding failed … to invalid activation. Rejecting now.") or, if reactivation is + /// slow, the 60 s hub-request timeout ("… target hub was not found"). Without the + /// transient guard first, that transient miss would be POISONED into the negative cache: + /// the storm-breaker would then replay the raw Orleans reject to every reader for the whole + /// backoff window and refuse to re-probe the grain that already reactivated — the exact + /// "navigating to an idle page intermittently crashes; a manual reload fixes it" bug (the + /// reload just outlasts the 2 s window and the re-probe lands on the fresh activation). + /// The && !IsTransientOwnerFailure guard also protects the "activation failed" + /// substring below — a transient activation miss must never be read as a permanent absence. + /// The node exists; it is transiently unreachable — never suppress it. + /// + internal static bool IsMissingNodeFailure(Exception error) => + !IsTransientOwnerFailure(error) + && (error.Message.Contains("No node found", StringComparison.OrdinalIgnoreCase) + || error.Message.Contains("activation failed", StringComparison.OrdinalIgnoreCase)); + + /// + /// True when an owner failure is TRANSIENT — the node exists but is momentarily + /// unreachable, so a later read/write is likely to succeed and the storm-breaker must + /// NEVER record it as a negative (missing-node) entry. Chiefly the Orleans grain- + /// reactivation window: a per-node grain that Orleans idle-collected answers the next + /// delivery with an OrleansMessageRejectionException ("… to invalid activation. + /// Rejecting now.", surfaced through routing as a DeliveryFailure{ErrorType.Failed} + /// whose message carries "Forwarding failed" / "invalid activation" / "Rejecting now"), + /// or — when reactivation outruns the request budget — a + /// ("No response received in hub …" / "target hub was not found" / "undeliverable"). + /// Each self-heals: the grain reactivates and the very next probe lands on the fresh + /// instance, so the error is forwarded to the current subscriber (whose caller retries) + /// but the cache stays clean. Mirrors RoutingGrain.IsTransientFailure and + /// AreaErrorClassifier.IsTransientHubFailure so all three layers agree on which + /// failures are worth a retry rather than a suppress. /// - private static bool IsMissingNodeFailure(Exception error) => - error.Message.Contains("No node found", StringComparison.OrdinalIgnoreCase) - || error.Message.Contains("activation failed", StringComparison.OrdinalIgnoreCase); + internal static bool IsTransientOwnerFailure(Exception? error) + { + for (var e = error; e != null; e = e.InnerException) + { + if (e is TimeoutException) return true; + // OrleansMessageRejectionException lives in Orleans.Core, which this project does + // not (and must not) reference — match by type name so the classifier stays free + // of the Orleans dependency while still catching the raw grain-reject that reaches + // the cache from the silo-side routing grain. + if (e.GetType().Name == "OrleansMessageRejectionException") return true; + var msg = e.Message ?? string.Empty; + // The routing grain surfaces the exhausted-forward reject as a DeliveryFailure whose + // message is "Delivery to '{path}' failed: Forwarding failed: tried to forward … to + // invalid activation. Rejecting now." — a TRANSIENT reactivation miss, not a real + // missing node. The 60 s hub-request timeout banner adds the "… hub was not found" / + // "undeliverable" / "No response received in hub" forms. + if (msg.Contains("invalid activation", StringComparison.OrdinalIgnoreCase)) return true; + if (msg.Contains("Rejecting now", StringComparison.OrdinalIgnoreCase)) return true; + if (msg.Contains("Forwarding failed", StringComparison.OrdinalIgnoreCase)) return true; + if (msg.Contains("target hub was not found", StringComparison.OrdinalIgnoreCase)) return true; + if (msg.Contains("No response received in hub", StringComparison.OrdinalIgnoreCase)) return true; + if (msg.Contains("undeliverable", StringComparison.OrdinalIgnoreCase)) return true; + } + return false; + } /// /// Returns a per-user access-gated view of the cached shared stream. The diff --git a/test/MeshWeaver.Hosting.Monolith.Test/MeshNodeStreamCacheNegativeClassifierTest.cs b/test/MeshWeaver.Hosting.Monolith.Test/MeshNodeStreamCacheNegativeClassifierTest.cs new file mode 100644 index 000000000..173daa2b7 --- /dev/null +++ b/test/MeshWeaver.Hosting.Monolith.Test/MeshNodeStreamCacheNegativeClassifierTest.cs @@ -0,0 +1,133 @@ +using System; +using MeshWeaver.Messaging; +using Xunit; + +namespace MeshWeaver.Hosting.Monolith.Test; + +/// +/// Deterministic classifier tests for the storm-breaker's +/// TRANSIENT-vs-MISSING decision — the fix for the recurring "navigating to a just-idle page +/// intermittently crashes; a manual reload fixes it" production bug. +/// +/// The bug. A per-node grain that Orleans idle-collects is deactivated. The next +/// read's SubscribeRequest hits the mid-DeactivateOnIdle activation and Orleans, +/// after forwarding MaxForwardCount=2 times, rejects it transiently — surfacing through +/// the silo routing grain as a DeliveryFailure whose message is +/// "Delivery to '{path}' failed: Forwarding failed: tried to forward message … for 2 times +/// after \"DeactivateOnIdle was called.\" to invalid activation. Rejecting now." (or, when +/// reactivation is slow, the 60 s hub-request timeout "… target hub was not found"). The +/// cache's READ path recorded THAT into the negative cache exactly like a genuine missing node, +/// so the storm-breaker replayed the raw Orleans reject to every reader for the whole backoff +/// window AND refused to re-probe the grain that had already reactivated. A manual reload just +/// outlasted the 2 s window so the re-probe landed on the fresh activation. +/// +/// The invariant. Only a GENUINE missing-node failure (NotFound / "No node found") +/// may be recorded as a negative (storm-breaker) entry. A transient reactivation reject / request +/// timeout is NEVER recorded — the node exists and self-heals on the next probe. Mirrors +/// RoutingGrain.IsTransientFailure and AreaErrorClassifier.IsTransientHubFailure so +/// all three layers agree. +/// +public class MeshNodeStreamCacheNegativeClassifierTest +{ + // The exact string the silo routing grain (RoutingGrain.DeliverToGrainWithRetry) posts to the + // sender once the transient Orleans forwarding-reject exhausts its retries — with the verbatim + // Orleans reject text ("Forwarding failed … invalid activation. Rejecting now.") inlined. This + // is the message that reached the browser as a crashed page. + private const string ForwardingRejectMessage = + "Delivery to 'AgenticPension/Statement' failed: Forwarding failed: tried to forward message " + + "Request [S1]->[messagehub/AgenticPension/Statement] DeliverMessage(...) #42[ForwardCount=2] " + + "for 2 times after \"DeactivateOnIdle was called.\" to invalid activation. Rejecting now."; + + // The 60 s hub-request timeout banner (MessageHub.BuildTimeoutMessage) — the "activation timeout" + // symptom. It contains "target hub was not found" / "undeliverable", which the transient + // classifier catches so a slow reactivation is never mistaken for a permanently-absent node. + private const string RequestTimeoutMessage = + "No response received in hub cache/mesh-node-cache within 00:01:00 for request SubscribeRequest " + + "(id=abc) → target AgenticPension/Statement. The request may have been undeliverable or the " + + "target hub was not found."; + + // A locally-defined stand-in with the SAME type name as the real Orleans exception, so the + // classifier's by-name match can be exercised WITHOUT this project referencing Orleans.Core. + private sealed class OrleansMessageRejectionException(string message) : Exception(message); + + private static DeliveryFailureException Nack(string message, ErrorType errorType) + { + var delivery = new MessageDelivery( + new Address("client", "1"), new Address("host", "1"), new object(), new System.Text.Json.JsonSerializerOptions()); + return new DeliveryFailureException(new DeliveryFailure(delivery, message) { ErrorType = errorType }); + } + + // ---- TRANSIENT: must be classified transient and therefore NOT recorded as a negative entry ---- + + [Fact] + public void ForwardingReject_IsTransient_NotMissingNode() + { + var nack = Nack(ForwardingRejectMessage, ErrorType.Failed); + MeshNodeStreamCache.IsTransientOwnerFailure(nack).Should().BeTrue( + "the exhausted-forward Orleans reject ('… invalid activation. Rejecting now.') is a mid-reactivation miss"); + MeshNodeStreamCache.IsMissingNodeFailure(nack).Should().BeFalse( + "a transient reactivation reject must NEVER be recorded as a missing-node negative entry (the bug)"); + } + + [Fact] + public void RawOrleansRejectionException_IsTransient() + { + var ex = new OrleansMessageRejectionException("invalid activation. Rejecting now."); + MeshNodeStreamCache.IsTransientOwnerFailure(ex).Should().BeTrue( + "the raw OrleansMessageRejectionException (matched by type name) is transient"); + MeshNodeStreamCache.IsMissingNodeFailure(ex).Should().BeFalse(); + } + + [Fact] + public void RequestTimeout_TargetHubNotFound_IsTransient() + { + var nack = Nack(RequestTimeoutMessage, ErrorType.Exception); + MeshNodeStreamCache.IsTransientOwnerFailure(nack).Should().BeTrue( + "the 60 s 'target hub was not found' timeout is a slow-reactivation miss, not a permanent absence"); + MeshNodeStreamCache.IsMissingNodeFailure(nack).Should().BeFalse(); + } + + [Fact] + public void RawTimeoutException_IsTransient() + { + MeshNodeStreamCache.IsTransientOwnerFailure(new TimeoutException("no response")).Should().BeTrue(); + } + + [Fact] + public void TransientCause_WrappedAsInnerException_IsStillTransient() + { + var wrapped = new InvalidOperationException("outer", new TimeoutException("target hub was not found")); + MeshNodeStreamCache.IsTransientOwnerFailure(wrapped).Should().BeTrue( + "the transient classifier walks the inner-exception chain, like the sibling classifiers"); + } + + // ---- MISSING NODE: a genuine absence IS recorded (the storm-breaker's legitimate job) ---- + + [Fact] + public void NoNodeFound_IsMissingNode_NotTransient() + { + var nack = Nack( + "No node found at 'rbuergi/_Activity/markdown-abc'. Closest ancestor is 'rbuergi' (remainder='…').", + ErrorType.NotFound); + MeshNodeStreamCache.IsTransientOwnerFailure(nack).Should().BeFalse( + "a genuine 'No node found' is a permanent absence, not a transient reactivation miss"); + MeshNodeStreamCache.IsMissingNodeFailure(nack).Should().BeTrue( + "a genuinely-missing node IS recorded — that is the storm-breaker's legitimate purpose"); + } + + [Fact] + public void UnrelatedError_IsNeitherTransientNorMissing() + { + // An RLS denial or a random processing fault: not transient (don't retry-forever) and not + // missing (don't suppress the node) — recorded by NEITHER predicate, matching prior behaviour. + var nack = Nack("Access denied: user 'x' lacks Read permission on 'y'.", ErrorType.Unauthorized); + MeshNodeStreamCache.IsTransientOwnerFailure(nack).Should().BeFalse(); + MeshNodeStreamCache.IsMissingNodeFailure(nack).Should().BeFalse(); + } + + [Fact] + public void Null_IsNeither() + { + MeshNodeStreamCache.IsTransientOwnerFailure(null).Should().BeFalse(); + } +} diff --git a/test/MeshWeaver.Hosting.Orleans.Test/OrleansIdleReactivationNoWedgeTest.cs b/test/MeshWeaver.Hosting.Orleans.Test/OrleansIdleReactivationNoWedgeTest.cs new file mode 100644 index 000000000..03517b43f --- /dev/null +++ b/test/MeshWeaver.Hosting.Orleans.Test/OrleansIdleReactivationNoWedgeTest.cs @@ -0,0 +1,122 @@ +using System; +using System.Reactive.Linq; +using System.Reactive.Threading.Tasks; +using System.Runtime.CompilerServices; +using System.Threading; +using System.Threading.Tasks; +using MeshWeaver.Data; +using MeshWeaver.Mesh; +using MeshWeaver.Messaging; +using Xunit; + +namespace MeshWeaver.Hosting.Orleans.Test; + +/// +/// End-to-end reactivation smoke test for the production bug where navigating to a page whose +/// per-node grain just went idle intermittently CRASHES, and a manual browser reload "fixes" it. +/// +/// Context. Orleans idle-collects a per-node grain. The next read's +/// SubscribeRequest can hit the mid-DeactivateOnIdle activation; Orleans forwards it +/// up to MaxForwardCount=2 times and then transiently rejects +/// ("Forwarding failed … to invalid activation. Rejecting now."), or — if reactivation is slow — +/// the 60 s hub-request timeout fires. The silo routing grain surfaces that transient failure +/// to the reading cache hub, which (before the fix) recorded it into its storm-breaker negative +/// cache exactly like a genuine missing node — replaying the raw Orleans reject to every reader for +/// the backoff window and refusing to re-probe the grain that had ALREADY reactivated. The manual +/// reload merely outlasted the window. The precise transient-vs-missing decision the fix turns on +/// is pinned deterministically by MeshNodeStreamCacheNegativeClassifierTest (the exact prod +/// message strings); the recording site is now symmetric with the write path. +/// +/// What this test asserts (the smoke test). A read issued right after the owning grain +/// is asked to deactivate must transparently REACTIVATE and return the node — never crash, never +/// wedge. A single in-memory test silo reactivates cleanly enough that it does not reliably hit the +/// two-hop forwarding-reject window, so this is a coverage smoke test for the reactivation path (it +/// would catch a gross regression that broke reactivation-after-idle), NOT the before/after +/// regression pin — that role belongs to the classifier test. A WaitAsync budget turns any +/// wedge into a deterministic (RED); a value inside the budget is +/// GREEN. +/// +public class OrleansIdleReactivationNoWedgeTest(ITestOutputHelper output) : OrleansSharedTestBase(output) +{ + private IMessageHub GetClient([CallerMemberName] string? name = null) + => base.GetClient($"idle-react-{name}-{Guid.NewGuid():N}", "TestUser"); + + private async Task CreateNode(IMessageHub client, string prefix) + { + var id = $"{prefix}-{Guid.NewGuid():N}"; + var response = await client.Observe( + new CreateNodeRequest(new MeshNode(id, "TestUser") { Name = "Original", NodeType = "Markdown" }), + o => o.WithTarget(new Address("TestUser"))) + .FirstAsync().ToTask().WaitAsync(45.Seconds()); + response.Message.Success.Should().BeTrue(response.Message.Error ?? ""); + return response.Message.Node!.Path!; + } + + // Live, CQRS-correct read via the per-node MeshNode stream (the exact path a GUI layout area + // subscription takes — through the cache hub's SubscribeRequest to the node grain). + private IObservable ReadNode(IMessageHub client, string path) + => client.GetWorkspace().GetMeshNodeStream(path).Where(n => n is not null); + + [Fact(Timeout = 120_000)] + public async Task ReadRightAfterGrainDeactivation_TransparentlyReactivates_NeverWedges() + { + var client = GetClient(); + var path = await CreateNode(client, "reactivate"); + + // 1. Warm read — activates the node grain and opens the cache entry. + var warm = await ReadNode(client, path).FirstAsync().ToTask().WaitAsync(45.Seconds()); + warm!.Path.Should().Be(path); + Output.WriteLine($"[warm] {path} read, grain active"); + + // 2. Force the owning per-node grain to deactivate (DeactivateOnIdle), reproducing an + // Orleans idle collection. This is exactly the state in which the next delivery hits a + // mid-deactivation activation and Orleans transiently rejects it. + Fixture.CleanupSiloHubsWithPrefix(path); + Output.WriteLine("[deactivate] requested DeactivateOnIdle on the owning grain"); + + // 3. Read AGAIN, immediately and repeatedly, straight into the deactivation window. Each + // read must transparently reactivate the grain and return the node — never surface the + // Orleans forwarding-reject, never spin to the 60 s timeout, and never get stuck behind a + // poisoned negative-cache window. Several back-to-back reads catch the race regardless of + // exactly when the grain finishes tearing down. + for (var attempt = 1; attempt <= 4; attempt++) + { + var reactivated = await ReadNode(client, path) + .FirstAsync().ToTask() + .WaitAsync(30.Seconds()); // a wedge (poisoned breaker / lost reactivation) trips this + reactivated!.Path.Should().Be(path, + $"read #{attempt} after deactivation must transparently reactivate the grain and return the node"); + Output.WriteLine($"[reactivated] read #{attempt} succeeded — grain came back transparently"); + } + } + + /// + /// A stronger form: after a deactivation-window read, the SAME path must keep answering promptly + /// — proving no negative-cache window was opened by a transient reject (a poisoned window would + /// make this second read fast-fail with the replayed Orleans reject well inside the storm + /// backoff, surfacing as a NON-timeout exception rather than a value). + /// + [Fact(Timeout = 120_000)] + public async Task AfterReactivation_ImmediateReReadStaysHealthy_NoPoisonedNegativeCache() + { + var client = GetClient(); + var path = await CreateNode(client, "healthy"); + + await ReadNode(client, path).FirstAsync().ToTask().WaitAsync(45.Seconds()); + Fixture.CleanupSiloHubsWithPrefix(path); + + // First read into the window: reactivates (or the grain already finished). Either way it + // must produce the node, not throw the transient reject. + var first = await ReadNode(client, path).FirstAsync().ToTask().WaitAsync(30.Seconds()); + first!.Path.Should().Be(path); + + // Immediate re-read — if the first read had poisoned the negative cache with the transient + // reject, THIS read would replay that error instantly (a DeliveryFailureException, NOT a + // TimeoutException) instead of returning the node. + var again = await ReadNode(client, path).FirstAsync().ToTask().WaitAsync(15.Seconds()); + again!.Path.Should().Be(path, + "an immediate re-read after reactivation must return the node — a poisoned negative-cache " + + "window from a transient reject would instead replay the raw Orleans reject"); + Output.WriteLine("[healthy] immediate re-read after reactivation returned the node — breaker clean"); + } +}