From db0285e2ded445fe1e25d4c1066578cce856ad17 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 09:48:48 -0400 Subject: [PATCH 1/6] fix(inbound): wire the compiled-handler cache as the IScriptArtifactChangeBus ApiMethod consumer (plan R2-06 T1) --- src/ZB.MOM.WW.ScadaBridge.Host/Program.cs | 8 +- .../ScriptArtifactChangeSubscriber.cs | 87 ++++++++++++ .../ServiceCollectionExtensions.cs | 6 + .../ScriptArtifactChangeSubscriberTests.cs | 133 ++++++++++++++++++ 4 files changed, 232 insertions(+), 2 deletions(-) create mode 100644 src/ZB.MOM.WW.ScadaBridge.InboundAPI/ScriptArtifactChangeSubscriber.cs create mode 100644 tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/ScriptArtifactChangeSubscriberTests.cs diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs b/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs index 69fb8252..f6b3cc0f 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs @@ -94,8 +94,12 @@ try builder.Services.AddTransport(); // Script-artifact invalidation bus: in-process, per-node pub/sub // for ScriptArtifactsChanged. The Transport bundle importer publishes - // post-commit; the Inbound API compiled-handler cache subscribes as the - // consumer (wired in plan 06). Central-only — it lives beside the importer. + // post-commit; the Inbound API's ScriptArtifactChangeSubscriber (a hosted + // service registered by AddInboundAPI below) consumes ApiMethod notifications + // and invalidates the compiled-handler cache (wired in plan R2-06 T1; the + // per-request revision check remains the correctness fallback). + // SharedScript/Template notifications currently have NO consumer — nothing at + // central caches compiled artifacts of those kinds. Central-only. builder.Services.AddSingleton< ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Scripting.IScriptArtifactChangeBus, ZB.MOM.WW.ScadaBridge.Host.Services.InProcessScriptArtifactChangeBus>(); diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ScriptArtifactChangeSubscriber.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ScriptArtifactChangeSubscriber.cs new file mode 100644 index 00000000..dff2f3cf --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ScriptArtifactChangeSubscriber.cs @@ -0,0 +1,87 @@ +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Scripting; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.Scripting; + +namespace ZB.MOM.WW.ScadaBridge.InboundAPI; + +/// +/// N1 (arch-review 06 round 2): the Inbound API consumer of the script-artifact +/// invalidation contract (docs/plans/2026-07-08-script-artifact-invalidation-contract.md, +/// handoff item (a)). Subscribes to on host +/// start and, for every ApiMethod notification (e.g. a Transport bundle +/// import overwriting method scripts, published post-commit), drops the compiled +/// handler AND the known-bad record via +/// , so the next request +/// recompiles from the fresh row without waiting for the per-request +/// revision-check self-heal. +/// +/// Fast-path only: the revision check in ExecuteAsync remains the +/// correctness fallback — this bus is in-process/per-node and at-least-once +/// (contract §4), so a missed or duplicate notification must always be harmless. +/// The bus is optional (site roles and plain test hosts register none) — the +/// subscriber then no-ops. SharedScript/Template notifications are ignored: this +/// executor caches only ApiMethod handlers. +/// +/// +public sealed class ScriptArtifactChangeSubscriber : IHostedService +{ + private readonly InboundScriptExecutor _executor; + private readonly ILogger _logger; + private readonly IScriptArtifactChangeBus? _bus; + private IDisposable? _subscription; + + /// Initializes the subscriber. + /// The compiled-handler cache to invalidate. + /// Logger instance. + /// The change bus, or null when the host registers none (site roles, tests). + public ScriptArtifactChangeSubscriber( + InboundScriptExecutor executor, + ILogger logger, + IScriptArtifactChangeBus? bus = null) + { + _executor = executor; + _logger = logger; + _bus = bus; + } + + /// + public Task StartAsync(CancellationToken cancellationToken) + { + if (_bus is null) + { + _logger.LogDebug( + "No IScriptArtifactChangeBus registered; inbound handler-cache freshness relies on the per-request revision check alone."); + return Task.CompletedTask; + } + + _subscription = _bus.Subscribe(OnChanged); + return Task.CompletedTask; + } + + /// + public Task StopAsync(CancellationToken cancellationToken) + { + _subscription?.Dispose(); + _subscription = null; + return Task.CompletedTask; + } + + private void OnChanged(ScriptArtifactsChanged notification) + { + if (!string.Equals( + notification.ArtifactKind, ScriptArtifactKinds.ApiMethod, StringComparison.Ordinal)) + { + return; // only ApiMethod handlers are cached by this executor + } + + foreach (var name in notification.Names) + { + _executor.InvalidateMethod(name); + } + + _logger.LogInformation( + "Invalidated {Count} inbound API method handler(s) after a {Source} script-artifact change.", + notification.Names.Count, notification.Source); + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ServiceCollectionExtensions.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ServiceCollectionExtensions.cs index 12df28ac..bf694c50 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ServiceCollectionExtensions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ServiceCollectionExtensions.cs @@ -27,6 +27,12 @@ public static class ServiceCollectionExtensions // body size cap and active-node gating for POST /api/{methodName}. services.AddSingleton(); + // N1: the ApiMethod consumer of the script-artifact invalidation bus. + // The bus parameter is optional — hosts without a registered + // IScriptArtifactChangeBus (site roles, plain test hosts) start it as a no-op + // and the executor's per-request revision check alone keeps handlers fresh. + services.AddHostedService(); + return services; } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/ScriptArtifactChangeSubscriberTests.cs b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/ScriptArtifactChangeSubscriberTests.cs new file mode 100644 index 00000000..37b3076f --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/ScriptArtifactChangeSubscriberTests.cs @@ -0,0 +1,133 @@ +using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.InboundApi; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Scripting; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.Scripting; + +namespace ZB.MOM.WW.ScadaBridge.InboundAPI.Tests; + +/// +/// Arch-review R2 N1: the Inbound API must be a REAL subscriber of +/// IScriptArtifactChangeBus — a bundle-import publish must invalidate a cached +/// handler immediately, without waiting for the per-request revision-check +/// self-heal (which remains the correctness fallback per the invalidation +/// contract's normative semantics). +/// +public class ScriptArtifactChangeSubscriberTests +{ + /// Minimal in-test bus: records subscriptions, delivers synchronously. + private sealed class RecordingBus : IScriptArtifactChangeBus + { + private readonly List> _handlers = new(); + public int SubscriberCount => _handlers.Count; + + public void Publish(ScriptArtifactsChanged notification) + { + foreach (var handler in _handlers.ToArray()) handler(notification); + } + + public IDisposable Subscribe(Action handler) + { + _handlers.Add(handler); + return new Token(() => _handlers.Remove(handler)); + } + + private sealed class Token(Action dispose) : IDisposable + { + public void Dispose() => dispose(); + } + } + + private readonly InboundScriptExecutor _executor = new( + NullLogger.Instance, Substitute.For()); + private readonly RecordingBus _bus = new(); + private readonly RouteHelper _route = new( + Substitute.For(), Substitute.For()); + + private ScriptArtifactChangeSubscriber CreateSubscriber(IScriptArtifactChangeBus? bus) => + new(_executor, NullLogger.Instance, bus); + + private Task Run(ApiMethod m) => _executor.ExecuteAsync( + m, new Dictionary(), _route, TimeSpan.FromSeconds(10)); + + private static ScriptArtifactsChanged ApiMethodChanged(params string[] names) => + new(ScriptArtifactKinds.ApiMethod, names, "BundleImport", DateTimeOffset.UtcNow); + + [Fact] + public async Task ApiMethodPublish_InvalidatesCachedHandler_WithoutRevisionCheckSelfHeal() + { + var subscriber = CreateSubscriber(_bus); + await subscriber.StartAsync(CancellationToken.None); + + // Pin a SCRIPT-LESS handler: the RegisterHandler seam is exempt from the + // per-request revision check, so the ONLY mechanism that can stop it + // serving is an explicit InvalidateMethod — exactly what the bus must trigger. + _executor.RegisterHandler("m", async _ => { await Task.CompletedTask; return 99; }); + var row = new ApiMethod("m", "return 1;") { Id = 1, TimeoutSeconds = 10 }; + + var before = await Run(row); + Assert.Contains("99", before.ResultJson); // pinned handler serves; no self-heal fires + + _bus.Publish(ApiMethodChanged("m")); + + var after = await Run(row); + Assert.True(after.Success); + Assert.Contains("1", after.ResultJson); // recompiled from the fresh row — invalidation, not self-heal + } + + [Fact] + public async Task ApiMethodPublish_PurgesKnownBadRecord() + { + var subscriber = CreateSubscriber(_bus); + await subscriber.StartAsync(CancellationToken.None); + + var broken = new ApiMethod("bad", "%%% not C# %%%") { Id = 2, TimeoutSeconds = 10 }; + Assert.False(_executor.CompileAndRegister(broken)); + Assert.Equal(1, _executor.KnownBadMethodCount); + + _bus.Publish(ApiMethodChanged("bad")); + + Assert.Equal(0, _executor.KnownBadMethodCount); + } + + [Fact] + public async Task NonApiMethodKinds_AreIgnored() + { + var subscriber = CreateSubscriber(_bus); + await subscriber.StartAsync(CancellationToken.None); + + _executor.RegisterHandler("m", async _ => { await Task.CompletedTask; return 99; }); + var row = new ApiMethod("m", "return 1;") { Id = 3, TimeoutSeconds = 10 }; + + _bus.Publish(new ScriptArtifactsChanged( + ScriptArtifactKinds.SharedScript, new[] { "m" }, "BundleImport", DateTimeOffset.UtcNow)); + _bus.Publish(new ScriptArtifactsChanged( + ScriptArtifactKinds.Template, new[] { "m" }, "BundleImport", DateTimeOffset.UtcNow)); + + var result = await Run(row); + Assert.Contains("99", result.ResultJson); // pinned handler untouched + } + + [Fact] + public async Task StopAsync_Unsubscribes() + { + var subscriber = CreateSubscriber(_bus); + await subscriber.StartAsync(CancellationToken.None); + Assert.Equal(1, _bus.SubscriberCount); + + await subscriber.StopAsync(CancellationToken.None); + + Assert.Equal(0, _bus.SubscriberCount); + } + + [Fact] + public async Task NullBus_NoOps() + { + // Site roles / plain test hosts have no bus registered — the hosted + // service must start and stop cleanly (the revision check alone applies). + var subscriber = CreateSubscriber(bus: null); + await subscriber.StartAsync(CancellationToken.None); + await subscriber.StopAsync(CancellationToken.None); + } +} From 9668600c38b2fe2db4afcc9b3ddcbba64f7d77b1 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 09:49:53 -0400 Subject: [PATCH 2/6] docs(inbound): correct bus-consumer claims in tracker + invalidation-contract doc + CLAUDE.md (plan R2-06 T2) --- CLAUDE.md | 2 +- archreview/plans/00-MASTER-TRACKER.md | 4 ++-- ...7-08-script-artifact-invalidation-contract.md | 16 +++++++--------- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index b8ce6646..4b32d017 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -150,7 +150,7 @@ Other peers in the `scadaproj` family (see `scadaproj/CLAUDE.md` for details): ` - Last-write-wins for concurrent template editing (no optimistic concurrency on templates). - Optimistic concurrency on deployment status records. - Naming collisions in composed feature modules are design-time errors. -- Transport (#24, M8): bundles now move site/instance config (`Site`s, site-scoped `DataConnection`s, `Instance`s + override children + `Area` by name), not just central-only config — but still never touch site runtime nodes (imported instances land `NotDeployed`). A `BundleNameMap` name-mapping subsystem (Commons: `SiteMapping`/`ConnectionMapping`/`MappingAction`) reconciles environment-specific identifiers (sites by `SiteIdentifier`, connections by site-qualified `{SiteIdentifier}/{Name}`, instances by `UniqueName`): auto-match → operator override via the import-wizard Map step / CLI `--map-site`/`--map-connection`/`--create-missing-*` → blocker rows for the unresolved. D3 "carry full config (encrypted secrets)": Site addresses travel; `DataConnection` config rides the encrypted `SecretsBlock` (presence-only in diffs). Per-line Myers diff for code fields via the pure `LineDiffer` (`ArtifactDiff` embeds a size-capped structured `lineDiff`; input capped at `MaxInputLines`=4000 → summary-only, so a crafted bundle can't OOM the active node); `ImportResult.StaleInstanceIds` is real via the `IStaleInstanceProbe` seam (Commons; implemented in DeploymentManager). `schemaVersion` 1.0→1.1 (additive); `bundleFormatVersion` stays 1; no new EF tables/columns. **Arch-review 05 (plan 05):** template child fidelity completed (round-trip-guard-verified: `LockedInDerived`, native-alarm-sources, script cadence/timeout, real `AreaName`, ES retry config + methods-on-Add, notification recipients, folder parent edges); import runs the **script trust gate** (template/shared/ApiMethod bodies **and** Expression-trigger bodies) as a hard pre-runtime gate + a name-resolution severity split (template=advisory `ConflictKind.Warning`+`ImportResult.Warnings`, ApiMethod=hard error); `ArtifactDiff` resolves folder/base/composition names (unchanged structured templates classify Identical again); `MaxConcurrentImportSessions`=8 session cap. Rename does NOT rewrite call sites (deploy gate on the target is the real check). Plan-06 handoff: `ScriptArtifactsChanged`/`IScriptArtifactChangeBus` seam + Host in-process bus + Transport post-commit publisher shipped (consumers = plan 06). +- Transport (#24, M8): bundles now move site/instance config (`Site`s, site-scoped `DataConnection`s, `Instance`s + override children + `Area` by name), not just central-only config — but still never touch site runtime nodes (imported instances land `NotDeployed`). A `BundleNameMap` name-mapping subsystem (Commons: `SiteMapping`/`ConnectionMapping`/`MappingAction`) reconciles environment-specific identifiers (sites by `SiteIdentifier`, connections by site-qualified `{SiteIdentifier}/{Name}`, instances by `UniqueName`): auto-match → operator override via the import-wizard Map step / CLI `--map-site`/`--map-connection`/`--create-missing-*` → blocker rows for the unresolved. D3 "carry full config (encrypted secrets)": Site addresses travel; `DataConnection` config rides the encrypted `SecretsBlock` (presence-only in diffs). Per-line Myers diff for code fields via the pure `LineDiffer` (`ArtifactDiff` embeds a size-capped structured `lineDiff`; input capped at `MaxInputLines`=4000 → summary-only, so a crafted bundle can't OOM the active node); `ImportResult.StaleInstanceIds` is real via the `IStaleInstanceProbe` seam (Commons; implemented in DeploymentManager). `schemaVersion` 1.0→1.1 (additive); `bundleFormatVersion` stays 1; no new EF tables/columns. **Arch-review 05 (plan 05):** template child fidelity completed (round-trip-guard-verified: `LockedInDerived`, native-alarm-sources, script cadence/timeout, real `AreaName`, ES retry config + methods-on-Add, notification recipients, folder parent edges); import runs the **script trust gate** (template/shared/ApiMethod bodies **and** Expression-trigger bodies) as a hard pre-runtime gate + a name-resolution severity split (template=advisory `ConflictKind.Warning`+`ImportResult.Warnings`, ApiMethod=hard error); `ArtifactDiff` resolves folder/base/composition names (unchanged structured templates classify Identical again); `MaxConcurrentImportSessions`=8 session cap. Rename does NOT rewrite call sites (deploy gate on the target is the real check). Plan-06 handoff: `ScriptArtifactsChanged`/`IScriptArtifactChangeBus` seam + Host in-process bus + Transport post-commit publisher shipped (ApiMethod consumer = InboundAPI `ScriptArtifactChangeSubscriber`, wired in plan R2-06; SharedScript/Template publish with no consumer — nothing at central caches those kinds). ### Store-and-Forward - Fixed retry interval, no max buffer size. Only transient failures buffered. diff --git a/archreview/plans/00-MASTER-TRACKER.md b/archreview/plans/00-MASTER-TRACKER.md index 02669a0e..e0f90bcb 100644 --- a/archreview/plans/00-MASTER-TRACKER.md +++ b/archreview/plans/00-MASTER-TRACKER.md @@ -84,7 +84,7 @@ Status values: ⬜ Not started · 🟨 In progress · ✅ Complete · ⏸ Blocke **Wave 3 — PLAN-05 COMPLETE (2026-07-10):** all 25 tasks delivered on worktree branch `worktree-archreview-wave3-plan05` and **merged to `main` `ec23db35` via fast-forward** (main was still at PLAN-04's `dbd3ed2f`, zero divergence; worktree removed, branch deleted). Templates/Deployment/Transport hardening across seven report-and-wait waves: import correctness spine (BundleImporter consumes `BaseTemplateName`/composition/inheritance edges + import-time acyclicity, `MinTimeBetweenRuns`/`ExecutionTimeoutSeconds`/`LockedInDerived`/native-alarm-sources/real `AreaName` carried through — T1–T7), a reflection round-trip equivalence guard that surfaced and fixed 5 silent-data-loss holes (ES retry+methods-on-Add, notification recipients, folder parent edges — T8), flattener/collision-detector repeated-composition fixes (T9/T10), revision-hash + DiffService native-source coverage (T11/T12), native-source lock enforcement (T13), the `IScriptArtifactChangeBus` invalidation contract + Host in-process bus + Transport post-commit publisher (T14, plan-06 handoff), compile-verdict cache + read-only-path compile skip + post-success audit isolation + central NotDeployed-delete (T15–T18), and the closing security/quality chain: **import blocker severity split** (template-script findings advisory, ApiMethod hard — T19), **the script trust gate on bundle import** (5th `ScriptTrustValidator` call site, over script bodies *and* Expression-trigger bodies — T20, given a code-reviewer security pass that caught + fixed 2 Important gaps: expression-trigger coverage and per-origin local-declaration scoping), ScriptTrustPolicy deny-list hardening (T21), LineDiffer input cap (T22), ArtifactDiff placeholder-identity resolution (T23), the low-severity batch (session cap / full error lists / leaf-fallback warning — T24), and the design-doc + CLAUDE.md sweep (T25). **Terminal verification:** full slnx build 0 warn/0 err; the entire `dotnet test` sweep green (~5,900 unit + integration tests across every suite) plus a direct `CLI.Tests` run (341). Only residue: the same environmental 8 `CentralUI.PlaywrightTests` Deployment E2E failures (`SQLite Error 14` on `instance deploy` — needs a live docker cluster; PLAN-05 touched zero files in that project's deployment-DB path). Initiative total: **128 of 192** (sum of the Done column). -**Wave 4 — PLAN-06 COMPLETE (2026-07-10):** all 24 tasks delivered and **merged to `main` `3e964ac2` via fast-forward** (main was still at PLAN-05's `9b0cf44c`, zero divergence; worktree removed, branch deleted). Edge Integrations hardening across six report-and-wait waves of file-disjoint parallel subagents: the **InboundAPI** spine — revision-checked compiled-handler cache (heals failover / direct-SQL / known-bad staleness, DB row now authoritative — T1/T2), `InvalidateMethod` seam consuming PLAN-05's `IScriptArtifactChangeBus` contract (T3), deferred DI-scope disposal + orphan-execution telemetry (T4), machine-readable error `code`s incl. `SITE_UNREACHABLE` + 415-on-non-JSON (T5), `IOptionsMonitor` hot-reload (T6), parallel startup compile (T7); the **ESG** per-system `TimeoutSeconds` full vertical slice (entity→EF migration→artifact→site SQLite→client honor→mgmt→CLI→UI→doc — T8–T12) plus `{param}` path-template substitution (T13), bounded outbound response buffering (T14), structured JSON `AuthConfiguration` with legacy-colon fallback (T15), cancellation-aware `IsTransient` (T16); the **Notification** batch — SMS first-transient short-circuit (T17), wired the dead `SmsConfiguration.MaxRetries/RetryDelay` per-type retry policy (T18), configurable OAuth2 authority/scope (entity→EF migration→token service→mgmt/CLI/UI surfaces — T19/T20), deterministic lowest-Id SMTP selection + atomic OAuth2 token cache (T21), Twilio `AccountSid` format validation (T22); and the docs (DelmiaNotifier at-least-once semantics — T23; dual-key API-key rotation procedure — T24). Two EF migrations added (`AddExternalSystemTimeoutSeconds` T8, `AddSmtpOAuth2AuthorityScope` T19, serialized on the shared snapshot). **Terminal verification:** full slnx build 0 warn/0 err; every unit + integration suite green (InboundAPI 253, ESG 132, NotificationOutbox 132, NotificationService 57, ManagementService 245, CLI 346, CentralUI 893, SiteRuntime 494, Commons 597, ConfigDB 338, Host 260, + all others). Only residue: the same environmental 8 `CentralUI.PlaywrightTests` Deployment E2E failures (`SQLite Error 14` on `instance deploy` — needs a live docker cluster; PLAN-06 touched zero files in that project's deployment-DB path). Execution note: `isolation:"worktree"` subagents proved non-isolating here (shared index → commit races); the working model was pre-created dedicated `git worktree`s per task + cherry-pick onto an integration branch + per-wave combined build/impacted-suite barrier. Initiative total: **152 of 192** (sum of the Done column). +**Wave 4 — PLAN-06 COMPLETE (2026-07-10):** all 24 tasks delivered and **merged to `main` `3e964ac2` via fast-forward** (main was still at PLAN-05's `9b0cf44c`, zero divergence; worktree removed, branch deleted). Edge Integrations hardening across six report-and-wait waves of file-disjoint parallel subagents: the **InboundAPI** spine — revision-checked compiled-handler cache (heals failover / direct-SQL / known-bad staleness, DB row now authoritative — T1/T2), `InvalidateMethod` seam (T3; the bus *subscriber* was NOT wired here — arch-review 06 round-2 N1 — and shipped later via PLAN-R2-06 T1), deferred DI-scope disposal + orphan-execution telemetry (T4), machine-readable error `code`s incl. `SITE_UNREACHABLE` + 415-on-non-JSON (T5), `IOptionsMonitor` hot-reload (T6), parallel startup compile (T7); the **ESG** per-system `TimeoutSeconds` full vertical slice (entity→EF migration→artifact→site SQLite→client honor→mgmt→CLI→UI→doc — T8–T12) plus `{param}` path-template substitution (T13), bounded outbound response buffering (T14), structured JSON `AuthConfiguration` with legacy-colon fallback (T15), cancellation-aware `IsTransient` (T16); the **Notification** batch — SMS first-transient short-circuit (T17), wired the dead `SmsConfiguration.MaxRetries/RetryDelay` per-type retry policy (T18), configurable OAuth2 authority/scope (entity→EF migration→token service→mgmt/CLI/UI surfaces — T19/T20), deterministic lowest-Id SMTP selection + atomic OAuth2 token cache (T21), Twilio `AccountSid` format validation (T22); and the docs (DelmiaNotifier at-least-once semantics — T23; dual-key API-key rotation procedure — T24). Two EF migrations added (`AddExternalSystemTimeoutSeconds` T8, `AddSmtpOAuth2AuthorityScope` T19, serialized on the shared snapshot). **Terminal verification:** full slnx build 0 warn/0 err; every unit + integration suite green (InboundAPI 253, ESG 132, NotificationOutbox 132, NotificationService 57, ManagementService 245, CLI 346, CentralUI 893, SiteRuntime 494, Commons 597, ConfigDB 338, Host 260, + all others). Only residue: the same environmental 8 `CentralUI.PlaywrightTests` Deployment E2E failures (`SQLite Error 14` on `instance deploy` — needs a live docker cluster; PLAN-06 touched zero files in that project's deployment-DB path). Execution note: `isolation:"worktree"` subagents proved non-isolating here (shared index → commit races); the working model was pre-created dedicated `git worktree`s per task + cherry-pick onto an integration branch + per-wave combined build/impacted-suite barrier. Initiative total: **152 of 192** (sum of the Done column). **Wave 5 — PLAN-07 COMPLETE (2026-07-10):** all 32 active tasks (T4–T32; T1–T3 landed in P0; **T33 `skipped-duplicate`** — CLI.Tests already in the slnx via PLAN-08 T1) delivered on worktree branch `plan07` and **merged to `main` via fast-forward** (main was still at PLAN-06's `4fb754f9`, zero divergence; all worktrees removed, branch deleted). UI/Management/Security hardening across six report-and-wait waves of file-disjoint parallel subagents against the dominant **ManagementActor single-writer lane** (15 tasks serialized in one integration worktree; the file-disjoint remainder ran in dedicated per-task worktrees, cherry-picked in with a per-wave combined build + impacted-suite barrier): **C1** auth-disabled artifact killed + `DisableLoginGuard` fail-fast (T1–T3, P0); **C2** `DeployToSiteAsync` single-site deploy + fleet-wide site-scope guard (T4/T5); **C3** connection/external-system/DB-connection secret elision via `ConfigSecretScrubber` + sentinel-preserving update (T6–T9); **UA1/C6/C5** any-of role gates + `ListSecuredWrites` gating + area-gate reconciliation (Designer|Deployer) + role-casing canonicalization (T10–T12); **UA2** the frozen `GetRequiredRoles` authorization matrix over all **138** registered commands + dispatch-coverage guard (T13/T24); **S2/P3** real server-side secured-write TTL + expiry-at-approve/reject + opportunistic list sweep + list paging + UI history pager & submission-age dialog (T14–T18); **S1** 5-min transport/deploy Ask timeout + shipped config (T19/T20); **C4** additive `SiteIdentifier` on browse/verify/cert commands + actor dispatch (Designer browse/verify, Admin cert-trust) + dispatch-coverage + CLI parity (T21–T25); **P1/UA5** `LoginThrottle` per-username+IP LDAP-bind lockout wired into every bind surface (T26/T27); **P2** additive Skip/Take paging on template/instance lists (T28); **S3** `PollGate` reentrancy guard on Health/AlarmSummary timers (T29); **S4** transport single-flight + export base64-copy elimination (T30); **S5/C7/C8/P4/UA6** debug-stream fault logging + event-log-filter doc fix + AlarmSummary memoization/a11y + the stale CLAUDE.md `SourceNode` note corrected (C7/C8 both verified true — T31/T32). Two message-contract additions (`AuditKind.SecuredWriteExpire`, secured-write list paging params) landed additively; no EF migrations needed. **Terminal verification:** full slnx build 0 warn/0 err; 28 of 30 test assemblies green (ManagementService 448 incl. the frozen matrix + dispatch-coverage, CentralUI 898, CLI 359, Security 175, Commons 609, Communication 263, Transport 138, ConfigDB 341, SiteRuntime 494, Host 260, + all others), after fixing one stale enum-count lock-in test (`AuditEnumTests` 15→16 for T15's additive `SecuredWriteExpire` — the per-task filtered runs never hit that Commons guard, so the full sweep caught it, exactly as in PLAN-04). Only residue: the same environmental 8 `CentralUI.PlaywrightTests` Deployment E2E failures (`SQLite Error 14` on `instance deploy` — needs a live docker cluster; PLAN-07 touched zero files in that project's deployment-DB path). Initiative total: **181 of 192** (sum of the Done column; PLAN-07's max is 32/33 with T33 a documented skip). @@ -146,7 +146,7 @@ Run up to **five plan-lane executor sessions concurrently** (one each for PLAN-0 - **PLAN-04 Task 13 (additive `RequestedBy` message change) before any PLAN-07 sender refactors** touching the same messages. **Soft seams (either order works, coordinate):** -- **PLAN-05 Task 14 (`IScriptArtifactChangeBus`) ↔ PLAN-06 Task 3 (`InboundScriptExecutor.InvalidateMethod`).** PLAN-06 Task 1's per-request revision check is the designed correctness fallback, so there is no hard sequencing; when both land, wire the bus to the invalidate seam. +- **PLAN-05 Task 14 (`IScriptArtifactChangeBus`) ↔ PLAN-06 Task 3 (`InboundScriptExecutor.InvalidateMethod`).** PLAN-06 Task 1's per-request revision check is the designed correctness fallback, so there is no hard sequencing; when both land, wire the bus to the invalidate seam. **Closed 2026-07-13 (PLAN-R2-06 T1):** `ScriptArtifactChangeSubscriber` (InboundAPI hosted service) subscribes and invalidates on `ApiMethod` notifications. - **PLAN-03's `DeployCompileValidator` should adopt PLAN-05's compile-cache abstraction** once it exists (later refactor, not a blocker). - **PLAN-03 Task 8's stuck-thread watchdog pattern is reusable by PLAN-06 Task 4** (inbound orphan counter). diff --git a/docs/plans/2026-07-08-script-artifact-invalidation-contract.md b/docs/plans/2026-07-08-script-artifact-invalidation-contract.md index 81bce194..8dce8c69 100644 --- a/docs/plans/2026-07-08-script-artifact-invalidation-contract.md +++ b/docs/plans/2026-07-08-script-artifact-invalidation-contract.md @@ -1,6 +1,6 @@ # Script-Artifact Invalidation Contract (`ScriptArtifactsChanged`) -**Status:** Seam + first producer shipped in PLAN-05 (arch-review 05, Task 14). Consumer + additional producers land in **plan 06**. +**Status:** Seam + first producer shipped in PLAN-05 (T14). **Consumer (a) shipped in PLAN-R2-06 T1** (`ScriptArtifactChangeSubscriber`). (b) Management producers remain unwired by design — delete calls `InvalidateMethod` directly; edits self-heal via the revision check. (c) is satisfied by the per-request revision check (PLAN-06 T1). **Problem.** When a script-bearing artifact's definition changes at runtime — an Inbound API method's script, a shared script, or a template's scripts — any node that has already **compiled and cached** a handler/delegate for it keeps serving the stale version until a process restart. This is the root of two recorded project-memory gotchas (a DB-direct edit to a broken inbound method keeps returning "Script compilation failed" from `_knownBadMethods`; a non-compiling update leaves the old delegate serving). A bundle import is a third mutation path with the same hazard. @@ -45,14 +45,12 @@ public interface IScriptArtifactChangeBus - `InProcessScriptArtifactChangeBus` in the Host, registered as a central-role singleton. - The **Transport bundle importer as the first producer**: `BundleImporter.ApplyAsync` collects the Overwrite/Rename resolutions for `ApiMethod` / `SharedScript` / `Template` and publishes one `ScriptArtifactsChanged` per kind **after** `tx.CommitAsync`. The importer's dependency on the bus is optional (`IScriptArtifactChangeBus?`) so Transport-only hosts (and unit tests) run without it. -## Handoff to plan 06 +## Handoff to plan 06 — dispositions -Plan 06 implements the consumer side and the remaining producers: - -| Item | Responsibility | +| Item | Disposition | |---|---| -| **(a) Inbound API consumer** | `InboundScriptExecutor` subscribes to the bus and, on an `ApiMethod` notification, evicts the named entries from `_scriptHandlers` **and** `_knownBadMethods` — closing the "DB-direct edit keeps returning compile error" and "non-compiling update keeps old handler" gotchas. | -| **(b) Management producers** | Publish `ScriptArtifactsChanged` from the `ManagementActor` `UpdateApiMethod` / shared-script / template-script edit paths (`Source = "Management"`), so a UI/CLI edit invalidates the same caches a bundle import does. | -| **(c) Cross-node / failover freshness** | A revision-keyed handler cache (compile keyed by code hash — see PLAN-05 Task 15's `ScriptCompileVerdictCache`) so a node that missed the in-process notification still self-heals on next use, and a newly-active failover node never serves a stale compile. | +| **(a) Inbound API consumer** | **Shipped (PLAN-R2-06 T1).** `ScriptArtifactChangeSubscriber` (an InboundAPI `IHostedService` registered by `AddInboundAPI`) subscribes to the bus and, on an `ApiMethod` notification, calls `InboundScriptExecutor.InvalidateMethod` per name — evicting the named entries from `_scriptHandlers` **and** `_knownBadMethods`, closing the "DB-direct edit keeps returning compile error" and "non-compiling update keeps old handler" gotchas. | +| **(b) Management producers** | **Unwired by design.** The `ManagementActor` delete path calls `InboundScriptExecutor.InvalidateMethod` directly; the update/edit paths self-heal via the per-request revision check, so no notification is needed for correctness. Left unwired deliberately rather than publishing `Source = "Management"` notifications. | +| **(c) Cross-node / failover freshness** | **Satisfied by the per-request revision check (PLAN-06 T1).** The compiled handler is revision-checked against the freshly-fetched `ApiMethod.Script` on every request, so a node that missed the in-process notification still self-heals on next use, and a newly-active failover node never serves a stale compile. | -Until (a)–(c) land, plan 05's publisher is a no-op in practice (no subscribers), but the seam is stable and the producer is correct, so plan 06 is purely additive. +Consumer (a) is now wired, so plan 05's `ApiMethod` publisher reaches a real subscriber. `SharedScript`/`Template` notifications still publish with **no consumer** — that is safe: nothing at central caches compiled artifacts of those kinds, so there is nothing to invalidate. The seam is stable and the producer is correct. From cc94b3a68c204f7b93c04ea43eeea546931b66e6 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 09:51:30 -0400 Subject: [PATCH 3/6] fix(esg): park buffered calls that fail deterministically with ArgumentException (path template/verb) instead of retrying to exhaustion (plan R2-06 T3) --- .../ExternalSystemClient.cs | 21 ++++++++++- .../ExternalSystemClientTests.cs | 36 +++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway/ExternalSystemClient.cs b/src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway/ExternalSystemClient.cs index 9144b0e7..c70920ce 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway/ExternalSystemClient.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway/ExternalSystemClient.cs @@ -166,7 +166,11 @@ public class ExternalSystemClient : IExternalSystemClient /// Delivers a buffered ExternalSystem call during a store-and-forward /// retry sweep. Returns true on success, false on permanent failure (the message /// is parked); throws on a - /// transient failure so the engine retries. + /// transient failure so the engine retries. Deterministic-poison failures against + /// the CURRENT method definition — malformed JSON, or an + /// from an unresolvable {param} path + /// template or an unsupported HTTP verb — return false (park immediately) rather + /// than retry, because re-running the same stored payload can never succeed. /// /// The buffered message to deliver. /// Cancellation token. @@ -221,6 +225,21 @@ public class ExternalSystemClient : IExternalSystemClient _logger.LogError(ex, "Buffered call to '{System}' failed permanently; parking.", payload.SystemName); return false; } + catch (ArgumentException ex) + { + // N2: same poison-message shape as the JsonException catch above. BuildUrl's + // {param} path-template substitution (placeholder with no matching/non-null + // parameter) and ValidateHttpMethod both throw ArgumentException + // deterministically for the SAME stored payload — the method definition + // changed underneath the buffered call, and retrying can never succeed. + // Return false so the S&F engine parks the message instead of counting the + // throw as transient and burning MaxRetries. + _logger.LogError( + ex, + "Buffered call to '{System}'/'{Method}' fails deterministically against the current method definition; parking.", + payload.SystemName, payload.MethodName); + return false; + } // TransientExternalSystemException propagates — the S&F engine retries. } diff --git a/tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs index 514380df..fcbfe7c2 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs @@ -1341,4 +1341,40 @@ public class ExternalSystemClientTests () => client.CallAsync("TestAPI", "getRecipe")); Assert.Contains("id", ex.Message); } + + [Fact] + public async Task DeliverBuffered_PathTemplateNowUnresolvable_ReturnsFalseSoMessageParks() + { + // Arch-review R2 N2: a method whose path gained a {param} placeholder AFTER + // calls were buffered throws ArgumentException deterministically for every + // already-buffered message — it must park immediately (permanent), not burn + // MaxRetries "transient" attempts (the S&F sweep's catch-all retries any throw). + var system = new ExternalSystemDefinition("TestAPI", "https://api.example.com", "none") { Id = 1 }; + var method = new ExternalSystemMethod("getRecipe", "GET", "/recipes/{id}") { Id = 1, ExternalSystemDefinitionId = 1 }; + StubResolution(system, method); + _httpClientFactory.CreateClient(Arg.Any()) + .Returns(new HttpClient(new MockHttpMessageHandler(HttpStatusCode.OK, "{}"))); + var client = new ExternalSystemClient( + _httpClientFactory, _repository, NullLogger.Instance); + + // BufferedCall carries Parameters:null — the {id} placeholder has no value. + var delivered = await client.DeliverBufferedAsync(BufferedCall("TestAPI", "getRecipe")); + + Assert.False(delivered); + } + + [Fact] + public async Task DeliverBuffered_UnsupportedVerbOnStoredMethod_ReturnsFalseSoMessageParks() + { + // ValidateHttpMethod throws the same deterministic ArgumentException shape. + var system = new ExternalSystemDefinition("TestAPI", "https://api.example.com", "none") { Id = 1 }; + var method = new ExternalSystemMethod("oddVerb", "FOO", "/p") { Id = 1, ExternalSystemDefinitionId = 1 }; + StubResolution(system, method); + var client = new ExternalSystemClient( + _httpClientFactory, _repository, NullLogger.Instance); + + var delivered = await client.DeliverBufferedAsync(BufferedCall("TestAPI", "oddVerb")); + + Assert.False(delivered); + } } From b41b3b1b319e59e00d3f7b8a8f807d03b99292ae Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 09:53:00 -0400 Subject: [PATCH 4/6] fix(esg): honor the declared response charset in ReadBodyBoundedAsync with UTF-8 fallback (plan R2-06 T4) --- .../ExternalSystemClient.cs | 34 +++++++++++- .../ExternalSystemClientTests.cs | 54 +++++++++++++++++++ 2 files changed, 87 insertions(+), 1 deletion(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway/ExternalSystemClient.cs b/src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway/ExternalSystemClient.cs index c70920ce..850ed1f9 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway/ExternalSystemClient.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway/ExternalSystemClient.cs @@ -466,6 +466,12 @@ public class ExternalSystemClient : IExternalSystemClient /// into store-and-forward would defeat the cap. Cancellation /// () is allowed to propagate so the /// caller's ordered timeout/cancellation catch filters classify it. + /// + /// N4: the accumulated bytes are decoded using the response's declared charset + /// (Content-Type: …; charset=…) via — parity + /// with the pre-bounded-read ReadAsStringAsync. A null, empty, or + /// unrecognized charset falls back to UTF-8. + /// /// private static async Task ReadBodyBoundedAsync( HttpContent content, string systemName, long maxBytes, CancellationToken token) @@ -498,7 +504,33 @@ public class ExternalSystemClient : IExternalSystemClient } } - return Encoding.UTF8.GetString(buffer.GetBuffer(), 0, (int)buffer.Length); + // N4: honor the response's declared charset (parity with the pre-bounded-read + // ReadAsStringAsync). Unknown/invalid charset → UTF-8 fallback: mojibake beats + // failing the call over a cosmetic header. + return ResolveEncoding(content.Headers.ContentType?.CharSet) + .GetString(buffer.GetBuffer(), 0, (int)buffer.Length); + } + + /// + /// Resolves the declared response charset to an , tolerating + /// the quoted form some servers emit (charset="iso-8859-1"). Null, empty, + /// or unrecognized values fall back to UTF-8. + /// + private static Encoding ResolveEncoding(string? charset) + { + if (string.IsNullOrWhiteSpace(charset)) + { + return Encoding.UTF8; + } + + try + { + return Encoding.GetEncoding(charset.Trim().Trim('"')); + } + catch (ArgumentException) + { + return Encoding.UTF8; + } } private static string Truncate(string value, int maxChars) diff --git a/tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs index fcbfe7c2..206c592f 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs @@ -1,5 +1,6 @@ using System.Net; using System.Net.Http.Headers; +using System.Text; using Microsoft.Data.Sqlite; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; @@ -1377,4 +1378,57 @@ public class ExternalSystemClientTests Assert.False(delivered); } + + [Fact] + public async Task Call_Latin1Charset_DecodesUsingDeclaredCharset() + { + // Arch-review R2 N4: the bounded read must honor the declared charset — + // UTF-8-decoding an iso-8859-1 body turns every non-ASCII byte into U+FFFD. + var system = new ExternalSystemDefinition("TestAPI", "https://api.example.com", "none") { Id = 1 }; + var method = new ExternalSystemMethod("getData", "GET", "/data") { Id = 1, ExternalSystemDefinitionId = 1 }; + StubResolution(system, method); + var handler = new ByteBodyHandler( + HttpStatusCode.OK, + Encoding.Latin1.GetBytes("température 25°C"), + "text/plain; charset=iso-8859-1"); + _httpClientFactory.CreateClient(Arg.Any()).Returns(new HttpClient(handler)); + var client = new ExternalSystemClient( + _httpClientFactory, _repository, NullLogger.Instance); + + var result = await client.CallAsync("TestAPI", "getData"); + + Assert.True(result.Success); + Assert.Contains("température 25°C", result.ResponseJson); + } + + [Fact] + public async Task Call_UnknownCharset_FallsBackToUtf8() + { + var system = new ExternalSystemDefinition("TestAPI", "https://api.example.com", "none") { Id = 1 }; + var method = new ExternalSystemMethod("getData", "GET", "/data") { Id = 1, ExternalSystemDefinitionId = 1 }; + StubResolution(system, method); + var handler = new ByteBodyHandler( + HttpStatusCode.OK, Encoding.UTF8.GetBytes("ok"), "text/plain; charset=not-a-charset"); + _httpClientFactory.CreateClient(Arg.Any()).Returns(new HttpClient(handler)); + var client = new ExternalSystemClient( + _httpClientFactory, _repository, NullLogger.Instance); + + var result = await client.CallAsync("TestAPI", "getData"); + + Assert.True(result.Success); + Assert.Contains("ok", result.ResponseJson); + } + + /// Test helper: returns a fixed byte body with an explicit Content-Type. + private sealed class ByteBodyHandler( + HttpStatusCode statusCode, byte[] body, string contentType) : HttpMessageHandler + { + protected override Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) + { + var response = new HttpResponseMessage(statusCode) { Content = new ByteArrayContent(body) }; + response.Content.Headers.ContentType = MediaTypeHeaderValue.Parse(contentType); + return Task.FromResult(response); + } + } } From fb9dc7ad146b8fae4ed8d767eb7dcb6834cbd329 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 09:57:22 -0400 Subject: [PATCH 5/6] 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" })); + } } From 91bde0fae6e4d17814bb5a16299389bd87b8ee74 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 10:01:22 -0400 Subject: [PATCH 6/6] fix(inbound): 415 guard and lenient JSON parse cover chunked (no Content-Length) bodies (plan R2-06 T6) --- docs/requirements/Component-InboundAPI.md | 2 +- .../EndpointExtensions.cs | 16 +++- .../EndpointContentTypeTests.cs | 86 +++++++++++++++++++ 3 files changed, 100 insertions(+), 4 deletions(-) diff --git a/docs/requirements/Component-InboundAPI.md b/docs/requirements/Component-InboundAPI.md index bd968f18..59c37ba9 100644 --- a/docs/requirements/Component-InboundAPI.md +++ b/docs/requirements/Component-InboundAPI.md @@ -118,7 +118,7 @@ The `sbk__` design already supports zero-downtime rotation — a |--------|-------------|---------| | `UNAUTHORIZED` | 401 | Missing/invalid API key (every auth-stage failure maps here — no stage leak). | | `NOT_APPROVED` | 403 | Method not found **or** key not in scope — indistinguishable by design. | -| `UNSUPPORTED_MEDIA_TYPE` | 415 | Request carried a body with a non-JSON `Content-Type`. A body with **no** `Content-Type` is still parsed leniently as JSON. | +| `UNSUPPORTED_MEDIA_TYPE` | 415 | Request carried a body — declared by `Content-Length` or `Transfer-Encoding` (chunked) — with a non-JSON `Content-Type`. A body with **no** `Content-Type` is still parsed leniently as JSON, and a chunked (no `Content-Length`) body is treated identically to a fixed-length one. | | `INVALID_JSON` | 400 | Request body was not valid JSON. | | `VALIDATION_FAILED` | 400 | Parameter validation failed (missing required field, wrong type, undeclared field). | | `BODY_TOO_LARGE` | 413 | Request body exceeded `MaxRequestBodyBytes`. | diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/EndpointExtensions.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/EndpointExtensions.cs index 85e5237c..0f753ef5 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/EndpointExtensions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/EndpointExtensions.cs @@ -179,12 +179,22 @@ public static class EndpointExtensions httpContext.Items[AuditWriteMiddleware.AuditActorItemKey] = identity.DisplayName; + // N5: a body is present when Content-Length says so, OR when the request is + // chunked — Transfer-Encoding present and Content-Length (necessarily) absent. + // Computed once and used by BOTH the 415 guard and the JSON parse condition so + // chunked and fixed-length bodies behave identically. + var requestHeaders = httpContext.Request.Headers; + var hasBody = httpContext.Request.ContentLength > 0 + || (httpContext.Request.ContentLength is null && requestHeaders.TransferEncoding.Count > 0); + // S9: a body sent with an explicit non-JSON Content-Type is a media-type // mismatch, not malformed JSON — reject it with 415 rather than a misleading // 400 "invalid JSON". A body with NO Content-Type header is still parsed // leniently as JSON below (preserving existing callers). The sniff is - // ordinal-ignore-case so "application/JSON" etc. are treated as JSON. - if (httpContext.Request.ContentLength > 0 + // ordinal-ignore-case so "application/JSON" etc. are treated as JSON. N5: the + // body signal is `hasBody`, so a chunked (no Content-Length) non-JSON body is + // also caught here instead of slipping through to a misleading 400. + if (hasBody && !string.IsNullOrEmpty(httpContext.Request.ContentType) && httpContext.Request.ContentType.Contains("json", StringComparison.OrdinalIgnoreCase) == false) { @@ -203,7 +213,7 @@ public static class EndpointExtensions // `Contains("json")` silently skipped JSON deserialization for any // capitalised value, leaving `body = null` and surfacing required // parameters as 400 "missing" even though the caller sent a valid body. - if (httpContext.Request.ContentLength > 0 + if (hasBody || httpContext.Request.ContentType?.Contains("json", StringComparison.OrdinalIgnoreCase) == true) { using var doc = await JsonDocument.ParseAsync( diff --git a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointContentTypeTests.cs b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointContentTypeTests.cs index 2e5c1cc2..fbf8db84 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointContentTypeTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointContentTypeTests.cs @@ -226,4 +226,90 @@ public sealed class EndpointContentTypeTests : IDisposable // Best-effort cleanup. } } + + [Fact] + public async Task ChunkedNonJsonBody_Returns415() + { + // Arch-review R2 N5: a chunked body has NULL ContentLength — it must not + // slip past the 415 guard into a misleading VALIDATION_FAILED. + const string methodName = "echoChunked"; + var method = new ApiMethod(methodName, "return Parameters[\"value\"];") + { + Id = 1, + TimeoutSeconds = 10, + ParameterDefinitions = """[{"name":"value","type":"Integer","required":true}]""", + }; + var repo = Substitute.For(); + repo.GetMethodByNameAsync(methodName, Arg.Any()).Returns(method); + + using var host = await BuildHostAsync(repo); + var token = await SeedKeyAsync(host, methodName); + var client = host.GetTestClient(); + + var request = new HttpRequestMessage(HttpMethod.Post, "/api/" + methodName) + { + Content = new ChunkedByteContent(Encoding.UTF8.GetBytes("hello")), + }; + request.Content.Headers.ContentType = MediaTypeHeaderValue.Parse("text/plain"); + request.Headers.TransferEncodingChunked = true; + request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", token); + + var response = await client.SendAsync(request); + var body = await response.Content.ReadAsStringAsync(); + + Assert.Equal(HttpStatusCode.UnsupportedMediaType, response.StatusCode); + Assert.Contains("UNSUPPORTED_MEDIA_TYPE", body); + } + + [Fact] + public async Task ChunkedJsonBody_NoContentType_StillParsesLeniently() + { + // The parse condition must also learn about chunked bodies: a chunked JSON + // body with no Content-Type previously skipped parsing entirely (body=null → + // misleading "missing required parameter"). + const string methodName = "echoChunkedJson"; + var method = new ApiMethod(methodName, "return Parameters[\"value\"];") + { + Id = 1, + TimeoutSeconds = 10, + ParameterDefinitions = """[{"name":"value","type":"Integer","required":true}]""", + }; + var repo = Substitute.For(); + repo.GetMethodByNameAsync(methodName, Arg.Any()).Returns(method); + + using var host = await BuildHostAsync(repo); + var token = await SeedKeyAsync(host, methodName); + var client = host.GetTestClient(); + + var request = new HttpRequestMessage(HttpMethod.Post, "/api/" + methodName) + { + Content = new ChunkedByteContent(Encoding.UTF8.GetBytes("{\"value\":42}")), + }; + request.Content.Headers.ContentType = null; + request.Headers.TransferEncodingChunked = true; + request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", token); + + var response = await client.SendAsync(request); + var body = await response.Content.ReadAsStringAsync(); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Contains("42", body); + } + + /// + /// Content whose length is never computable, forcing HttpClient to transmit it + /// chunked (no Content-Length header ever reaches the server). + /// + private sealed class ChunkedByteContent(byte[] bytes) : HttpContent + { + protected override Task SerializeToStreamAsync( + Stream stream, System.Net.TransportContext? context) => + stream.WriteAsync(bytes, 0, bytes.Length); + + protected override bool TryComputeLength(out long length) + { + length = -1; + return false; + } + } }