From fb9dc7ad146b8fae4ed8d767eb7dcb6834cbd329 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 09:57:22 -0400 Subject: [PATCH] fix(inbound): type SITE_UNREACHABLE on AskTimeoutException at the router await instead of message-substring sniffing (plan R2-06 T5) --- docs/requirements/Component-InboundAPI.md | 2 +- .../RouteHelper.cs | 80 +++++++++++++------ .../InboundScriptExecutorTests.cs | 41 ++++++++-- .../RouteHelperTests.cs | 26 ++++++ 4 files changed, 114 insertions(+), 35 deletions(-) diff --git a/docs/requirements/Component-InboundAPI.md b/docs/requirements/Component-InboundAPI.md index c861f914..bd968f18 100644 --- a/docs/requirements/Component-InboundAPI.md +++ b/docs/requirements/Component-InboundAPI.md @@ -124,7 +124,7 @@ The `sbk__` design already supports zero-downtime rotation — a | `BODY_TOO_LARGE` | 413 | Request body exceeded `MaxRequestBodyBytes`. | | `STANDBY_NODE` | 503 | Request hit a standby central node; the inbound API serves only the active node. | | `TIMEOUT` | 500 | The method script exceeded its execution timeout. | -| `SITE_UNREACHABLE` | 500 | A routed `Route.To(...).Call(...)` could not reach the target site. | +| `SITE_UNREACHABLE` | 500 | The target site did not respond to the routed request within the routing timeout (no contact / down / partitioned) — typed from the routed Ask expiring (`AskTimeoutException`), never inferred from an error-message substring. A `Success=false` response means the site answered and is classified `SCRIPT_ERROR`. | | `SCRIPT_COMPILE_FAILED` | 500 | The method's script failed to compile (returned consistently on every node until a compiling version is saved). | | `SCRIPT_ERROR` | 500 | Catch-all for any other script-execution failure (details are logged centrally, never leaked to the caller). | diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs index dca01913..63872f72 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs @@ -1,3 +1,4 @@ +using Akka.Actor; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; using ZB.MOM.WW.ScadaBridge.Commons.Messages.InboundApi; using ZB.MOM.WW.ScadaBridge.Commons.Types; @@ -5,12 +6,16 @@ using ZB.MOM.WW.ScadaBridge.Commons.Types; namespace ZB.MOM.WW.ScadaBridge.InboundAPI; /// -/// C3: a routed call failed because the target site could not be reached (no contact / -/// unreachable). Derives from so existing -/// callers that catch the base type are unaffected, while -/// catches this more specific type to surface the -/// spec's SITE_UNREACHABLE error code instead of a generic SCRIPT_ERROR. -/// A routing failure whose message does not indicate unreachability stays a +/// C3/N3: a routed call failed because the target site never answered — the routed +/// Ask expired. Unreachability is typed at the Ask boundary from +/// (the communication layer's own signal: an +/// unreachable/contactless site has its enveloped message dropped so the Ask times +/// out caller-side) — it is never inferred from an error-message substring. +/// Derives from so existing callers that +/// catch the base type are unaffected, while +/// catches this more specific type to surface the spec's SITE_UNREACHABLE +/// error code instead of a generic SCRIPT_ERROR. A Success=false +/// response means the site answered (reachable by definition) and stays a /// plain . /// public sealed class SiteUnreachableException : InvalidOperationException @@ -18,6 +23,11 @@ public sealed class SiteUnreachableException : InvalidOperationException /// Initializes a new with the given message. /// The routing-level failure message that indicated the site was unreachable. public SiteUnreachableException(string message) : base(message) { } + + /// Initializes a new wrapping the underlying Ask-timeout. + /// The routing-level failure message. + /// The underlying from the routed Ask. + public SiteUnreachableException(string message, Exception innerException) : base(message, innerException) { } } /// @@ -182,7 +192,7 @@ public class RouteTarget correlationId, _instanceCode, scriptName, ScriptArgs.Normalize(parameters), DateTimeOffset.UtcNow, _parentExecutionId); - var response = await _instanceRouter.RouteToCallAsync(siteId, request, token); + var response = await RouteOrUnreachable(_instanceRouter.RouteToCallAsync(siteId, request, token)); if (!response.Success) { @@ -230,7 +240,7 @@ public class RouteTarget correlationId, _instanceCode, attributeNames.ToList(), DateTimeOffset.UtcNow, _parentExecutionId); - var response = await _instanceRouter.RouteToGetAttributesAsync(siteId, request, token); + var response = await RouteOrUnreachable(_instanceRouter.RouteToGetAttributesAsync(siteId, request, token)); if (!response.Success) { @@ -294,7 +304,7 @@ public class RouteTarget AttributeValueCodec.Encode(targetValue), timeout, DateTimeOffset.UtcNow, _parentExecutionId); - var response = await _instanceRouter.RouteToWaitForAttributeAsync(siteId, request, token); + var response = await RouteOrUnreachable(_instanceRouter.RouteToWaitForAttributeAsync(siteId, request, token)); if (!response.Success) { @@ -345,7 +355,7 @@ public class RouteTarget correlationId, _instanceCode, attributeValues, DateTimeOffset.UtcNow, _parentExecutionId); - var response = await _instanceRouter.RouteToSetAttributesAsync(siteId, request, token); + var response = await RouteOrUnreachable(_instanceRouter.RouteToSetAttributesAsync(siteId, request, token)); if (!response.Success) { @@ -366,9 +376,9 @@ public class RouteTarget var siteId = await _instanceLocator.GetSiteIdForInstanceAsync(_instanceCode, cancellationToken); if (siteId == null) { - // Route the site-resolution failure through the same classifier so a - // message indicating the site is unreachable surfaces as SITE_UNREACHABLE; - // a plain not-found / no-assigned-site stays a generic InvalidOperationException. + // A null site id is a plain not-found / no-assigned-site — never + // unreachability (nothing was routed), so it stays a generic + // InvalidOperationException (SCRIPT_ERROR). throw RoutingFailure( $"Instance '{_instanceCode}' not found or has no assigned site"); } @@ -377,19 +387,37 @@ public class RouteTarget } /// - /// C3: classifies a routing-level failure message. When the message indicates the - /// site could not be reached (unreachable / no contact) the failure is a - /// — so - /// can surface the spec's SITE_UNREACHABLE code — otherwise a plain - /// . + /// N3: awaits a routed Ask and TYPES unreachability. AskTimeoutException means + /// the site never answered (no ClusterClient contact, site down, partition) — + /// the communication layer's own typed signal (CentralCommunicationActor drops + /// envelopes for contactless sites so the Ask expires caller-side) — and + /// surfaces as SiteUnreachableException so the executor maps it to the spec's + /// SITE_UNREACHABLE code. A Success=false RESPONSE means the site answered and + /// is therefore reachable; it flows through RoutingFailure as a plain + /// InvalidOperationException (SCRIPT_ERROR). /// - private static InvalidOperationException RoutingFailure(string message) => - IsUnreachable(message) - ? new SiteUnreachableException(message) - : new InvalidOperationException(message); + private static async Task RouteOrUnreachable(Task routedAsk) + { + try + { + return await routedAsk; + } + catch (AskTimeoutException ex) + { + throw new SiteUnreachableException( + "Site did not respond to the routed request within the routing timeout — unreachable or not connected.", + ex); + } + } - private static bool IsUnreachable(string message) => - message.Contains("unreachable", StringComparison.OrdinalIgnoreCase) - || message.Contains("no contact", StringComparison.OrdinalIgnoreCase) - || message.Contains("no-contact", StringComparison.OrdinalIgnoreCase); + /// + /// C3/N3: a routing-level failure REPORTED BY the site or the locator. The + /// remote end answered (or the instance simply has no site), so this is never + /// unreachability — always a plain + /// (SCRIPT_ERROR at the endpoint). Unreachability is typed at the Ask boundary + /// by RouteOrUnreachable (AskTimeoutException → SiteUnreachableException), + /// replacing the old message-substring sniff that a harmless rewording in the + /// communication layer could silently defeat. + /// + private static InvalidOperationException RoutingFailure(string message) => new(message); } diff --git a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs index 9a929c4a..2334d070 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs @@ -98,19 +98,17 @@ public class InboundScriptExecutorTests } [Fact] - public async Task RoutedSiteUnreachable_Returns500WithSiteUnreachableCode() + public async Task RoutedAskTimeout_Returns500WithSiteUnreachableCode() { - // C3: a routed call whose site is unreachable must surface the machine-readable - // SITE_UNREACHABLE code (spec Component-InboundAPI failure body) rather than - // collapsing into a generic SCRIPT_ERROR. + // N3: unreachability is the Ask EXPIRING (site never answered) — typed as + // AskTimeoutException by the communication layer — not a substring in a + // failure message. Pre-fix this fell into the generic catch as SCRIPT_ERROR. var locator = Substitute.For(); locator.GetSiteIdForInstanceAsync("X", Arg.Any()).Returns("SiteA"); var router = Substitute.For(); router.RouteToCallAsync("SiteA", Arg.Any(), Arg.Any()) - .Returns(ci => new RouteToCallResponse( - ((RouteToCallRequest)ci[1]).CorrelationId, - Success: false, ReturnValue: null, ErrorMessage: "Site unreachable", - DateTimeOffset.UtcNow)); + .Returns(Task.FromException( + new Akka.Actor.AskTimeoutException("Timeout after 00:00:30"))); var route = new RouteHelper(locator, router); var method = new ApiMethod("routed", "return await Route.To(\"X\").Call(\"s\");") @@ -123,6 +121,33 @@ public class InboundScriptExecutorTests Assert.Equal("SITE_UNREACHABLE", result.ErrorCode); } + [Fact] + public async Task SiteRespondedFailure_MessageMentioningUnreachable_IsScriptErrorNotSiteUnreachable() + { + // N3 anti-sniff: a site that RESPONDED with Success=false is reachable by + // definition — even when its error text happens to contain "unreachable". + // Pre-fix the substring sniff misclassified this as SITE_UNREACHABLE. + var locator = Substitute.For(); + locator.GetSiteIdForInstanceAsync("X", Arg.Any()).Returns("SiteA"); + var router = Substitute.For(); + router.RouteToCallAsync("SiteA", Arg.Any(), Arg.Any()) + .Returns(ci => new RouteToCallResponse( + ((RouteToCallRequest)ci[1]).CorrelationId, + Success: false, ReturnValue: null, + ErrorMessage: "PLC device unreachable behind the site gateway", + DateTimeOffset.UtcNow)); + var route = new RouteHelper(locator, router); + + var method = new ApiMethod("routed2", "return await Route.To(\"X\").Call(\"s\");") + { Id = 8, TimeoutSeconds = 10 }; + + var result = await _executor.ExecuteAsync( + method, new Dictionary(), route, TimeSpan.FromSeconds(10)); + + Assert.False(result.Success); + Assert.Equal("SCRIPT_ERROR", result.ErrorCode); + } + [Fact] public async Task HandlerTimesOut_ReturnsTimeoutError() { diff --git a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/RouteHelperTests.cs b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/RouteHelperTests.cs index f8f0b36c..70712c81 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/RouteHelperTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/RouteHelperTests.cs @@ -595,4 +595,30 @@ public class RouteHelperTests Assert.NotNull(captured); Assert.Equal(inboundExecutionId, captured!.ParentExecutionId); } + + // --- N3: typed unreachability at the Ask boundary --- + + [Fact] + public async Task Call_AskTimeout_ThrowsSiteUnreachableException() + { + SiteResolves("inst", "SiteA"); + _router.RouteToCallAsync("SiteA", Arg.Any(), Arg.Any()) + .Returns(Task.FromException( + new Akka.Actor.AskTimeoutException("Timeout after 00:00:30"))); + + await Assert.ThrowsAsync( + () => CreateHelper().To("inst").Call("s")); + } + + [Fact] + public async Task GetAttributes_AskTimeout_ThrowsSiteUnreachableException() + { + SiteResolves("inst", "SiteA"); + _router.RouteToGetAttributesAsync("SiteA", Arg.Any(), Arg.Any()) + .Returns(Task.FromException( + new Akka.Actor.AskTimeoutException("Timeout after 00:00:30"))); + + await Assert.ThrowsAsync( + () => CreateHelper().To("inst").GetAttributes(new[] { "a" })); + } }