docs(archreview): add architecture-review fix plans (P0 initiative baseline)

archreview/06-edge-integrations.md

@@ -0,0 +1,143 @@
+# Architecture Review 06 — Edge Integrations (Inbound & Outbound Boundaries)
+
+Reviewer domain: the four components where ScadaBridge touches external systems.
+
+## Scope
+
+Deep source review (every non-generated `.cs` file read) of:
+
+- `src/ZB.MOM.WW.ScadaBridge.InboundAPI` — `POST /api/{methodName}`, API-key auth, Roslyn script handlers, parameter/return validation, audit middleware, routed site calls.
+- `src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway` — outbound HTTP client (`Call`/`CachedCall`), `DatabaseGateway` (`Connection`/`CachedWrite`), HTTP + SQL error classifiers, S&F handoff.
+- `src/ZB.MOM.WW.ScadaBridge.NotificationService` + the delivery adapters in `src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/Delivery` — SMTP (MailKit) wrapper, OAuth2 token service, SMTP/SMS error classifiers, credential redactor, Email/SMS adapters.
+- `src/ZB.MOM.WW.ScadaBridge.DelmiaNotifier` — Native-AOT `WWNotifier.exe` client (arg parser, config loader, failover loop, YES/NO reporter).
+
+Specs compared: `docs/requirements/Component-InboundAPI.md`, `Component-ExternalSystemGateway.md`, `Component-NotificationService.md`, `docs/plans/2026-06-26-delmia-recipe-notifier-design.md`. Test projects examined: `tests/ZB.MOM.WW.ScadaBridge.{InboundAPI,ExternalSystemGateway,NotificationService,NotificationOutbox,DelmiaNotifier}.Tests`.
+
+## Maturity Verdict
+
+This is a mature, unusually well-hardened edge layer. The code shows evidence of multiple hardening passes: enumeration-safe auth responses, bounded body capture with UTF-8-safe truncation, ordered cancellation-vs-timeout catch discrimination repeated consistently across HTTP, SQL, SMTP, and SMS paths, poison-message parking on both S&F retry paths, credential redaction on every error string, and secrets encrypted at rest (`ExternalSystemConfiguration.cs:27-29,83-85`). Error classification matches the spec (5xx/408/429/connection = transient) on all four channels and is centralized in dedicated classifiers with tests. The remaining risk is concentrated in three places: **cache coherence of compiled inbound script handlers across central failover** (a silent-stale-code bug, the one genuinely High-severity defect found), **two acknowledged spec-vs-code gaps in the External System Gateway** (per-system timeout and path templating), and **unbounded buffering of outbound HTTP response bodies**. DelmiaNotifier is small, faithful to its design doc, and fully tested at the logic level.
+
+---
+
+## Findings — Dimension 1: Stability
+
+### [High] Compiled inbound handler cache goes stale across central failover — old script serves silently with HTTP 200
+
+`InboundScriptExecutor` is a per-node singleton (`ServiceCollectionExtensions.cs:19`) whose `_scriptHandlers` and `_knownBadMethods` caches (`InboundScriptExecutor.cs:26,39`) are mutated only by:
+
+- the node's own startup compile of all methods (`Host/Program.cs:379-388`), and
+- `ManagementActor.HandleUpdateApiMethod`/`HandleDeleteApiMethod` → `CompileAndRegister`/`RemoveHandler` (`ManagementActor.cs:2631,2650`) — which run **only on the node hosting the ManagementActor singleton (the active node)**.
+
+The standby central node compiles every method at *its* startup and then never hears about updates. `ExecuteAsync` looks the handler up by **name only** (`InboundScriptExecutor.cs:311`) — no revision/hash check — even though the endpoint fetches the fresh `ApiMethod` row (with the current `Script` text) from the DB on every request (`EndpointExtensions.cs:142`).
+
+Concrete failure scenario:
+1. Method `GetOrderStatus` updated via CLI on active node A → A recompiles, serves new logic.
+2. Weeks later A fails over to B. B still holds the delegate compiled from the script text as of B's process start.
+3. B serves the **old** script indefinitely — HTTP 200, correct-looking JSON, `UpdateApiMethod` long ago reported success. Nothing recompiles until B restarts.
+
+The mirror-image variant: a method that was broken at B's startup landed in B's `_knownBadMethods`; it was later fixed via management update (clears the record on A only). After failover, B answers `"Script compilation failed for this method"` for a method that compiles fine.
+
+This is the cluster-wide generalization of the already-known "direct SQL edit doesn't clear the cache" issue (documented as a Direct SQL Warning, `Component-InboundAPI.md:129-131`) — but here the write went through the *supported* management path and is still lost. A per-request script-hash comparison against the freshly-fetched row would fix failover staleness, direct-SQL staleness, and known-bad staleness in one move at negligible cost (the row is already in hand).
+
+Deleted methods are safe (fresh DB lookup returns null → 403); methods *created* after standby startup are safe (lazy compile, `InboundScriptExecutor.cs:314-330`).
+
+### [Medium] Method timeout abandons the running script; its DI scope is disposed underneath it
+
+On timeout, `handler(context).WaitAsync(cts.Token)` (`InboundScriptExecutor.cs:333`) returns control but does not stop the underlying script task — cancellation is cooperative via `ctx.CancellationToken` only. The `finally` block then disposes the per-execution DI scope (`InboundScriptExecutor.cs:389-393`) while a non-cooperative script (e.g. mid-flight in a poorly-authored loop, or blocked in a routed call that ignores the token) may still be running and holding `InboundDatabaseHelper` → the scoped `IDatabaseGateway`. Consequences: `ObjectDisposedException`s surfacing as unobserved background faults, and — since the inbound API deliberately has no rate limiting (`Component-InboundAPI.md:140`) — repeated timeouts of a slow method accumulate orphaned executions with no counter or cap. Recommend at minimum a metric for abandoned executions, and deferring scope disposal until the handler task actually completes (`ContinueWith` on the abandoned task).
+
+### [Medium] ESG API-key `AuthConfiguration` colon-splitting is ambiguous — a key containing `:` silently breaks auth
+
+`ExternalSystemClient.ApplyAuth` (`ExternalSystemClient.cs:489-499`) interprets `AuthConfiguration` as `"HeaderName:KeyValue"` when it contains a colon, else as a bare key with default header `X-API-Key`. An API key whose *value* legitimately contains a colon (not rare for opaque tokens) is silently misparsed: the prefix becomes a bogus header name and the request goes out mis-authenticated with only a remote 401 to debug from. The Basic branch (`:504-519`) is fine (split-2 keeps colons in the password), but the apikey branch has no escape hatch. Structured JSON config (`{"header":"X-API-Key","key":"..."}`) — which the entity doc-comment (`ExternalSystemDefinition.cs:13`, "JSON-serialized authentication configuration") already implies — would eliminate the ambiguity.
+
+### [Medium] DelmiaNotifier failover can duplicate a processed notification
+
+`HttpRecipeSender.cs:36-45` maps **any** `HttpRequestException` and any non-caller timeout to `ConnectFailed`, which the loop rolls over to the next URL (`Notifier.cs`, `continue`). Both cases include "the server received and possibly processed the request, but the response never arrived" (connection reset after send; slow response exceeding `TimeoutSeconds`). The next node then receives a second `DelmiaRecipeDownload` POST for the same recipe. This matches the design table (`2026-06-26-delmia-recipe-notifier-design.md:97-99` lists timeout as a failover trigger) — so this is a *design-level* at-least-once semantics that is nowhere documented as such; the inbound `DelmiaRecipeDownload` method must be idempotent on `(MachineCode, DownloadPath, WorkOrderNumber)` or duplicates will surface downstream. Worth one paragraph in the design doc and a check of the deployed method script.
+
+### [Medium] SMS adapter keeps sending after the first transient failure — inflating duplicate texts on retry
+
+In `SmsNotificationDeliveryAdapter.DeliverAsync` (`SmsNotificationDeliveryAdapter.cs:148-170`), a transient result does not break the loop; the remaining recipients are still attempted. Since *any* transient rolls the **whole notification** up to `Transient` (`RollUp`, `:297-303`) and retry re-texts everyone, each additional number accepted after the first transient is a guaranteed duplicate SMS on the retry. Short-circuiting on the first transient would reduce duplicates at no delivery cost (the retry re-attempts all numbers anyway). The re-send-to-all characteristic itself is documented (`Component-NotificationService.md:102`), but this amplification is avoidable within v1's no-per-recipient-state constraint.
+
+### [Low] `ErrorClassifier.IsTransient(Exception)` treats any `OperationCanceledException` as transient
+
+`ErrorClassifier.cs:32-38` classifies bare `OperationCanceledException`/`TaskCanceledException` as transient. Every current call site is protected by ordered catch filters that peel off caller-cancellation first (`ExternalSystemClient.cs:314-327`), so behaviour is correct today — but the predicate is public and a future call site that skips the ordering would buffer caller-cancelled work into S&F. A `cancellationToken` parameter (as `SmtpErrorClassifier.Classify` and `SmsErrorClassifier.Classify` already take) would make it misuse-resistant.
+
+### [Low] First-of-many SMTP configuration is nondeterministic
+
+`EmailNotificationDeliveryAdapter.cs:75-76` and `NotificationOutboxActor.ResolveRetryPolicyAsync` (`NotificationOutboxActor.cs:373-375`) both use `GetAllSmtpConfigurationsAsync()` + `FirstOrDefault()`. With more than one SMTP row the selected server and the retry policy depend on repository ordering. Either enforce single-row semantics or add an explicit "active" flag.
+
+### [Low] OAuth2 token cache: unsynchronized field visibility and unbounded growth
+
+`OAuth2TokenService.CacheEntry.Token/Expiry` are plain fields read outside the per-entry lock (`OAuth2TokenService.cs:48-51`) and written inside it (`:93-94`). The races are benign (worst case: one redundant token fetch), but `volatile`/`Volatile.Read` would make it airtight. The cache is also never evicted — bounded in practice by the number of distinct SMTP credential triples, so acceptable.
+
+### [Low] Non-JSON content-type with a body yields a misleading 400
+
+`EndpointExtensions.cs:181-194`: `ContentLength > 0 || ContentType contains "json"` means a `text/plain` or `application/x-www-form-urlencoded` body is force-parsed as JSON and rejected as "Invalid JSON in request body" rather than a 415. Cosmetic, but the error steers the caller at the wrong problem.
+
+## Findings — Dimension 2: Performance
+
+### [Medium] Outbound HTTP responses are buffered unbounded
+
+`ExternalSystemClient.InvokeHttpAsync` uses `client.SendAsync(request, token)` with the default `ResponseContentRead` completion option (`ExternalSystemClient.cs:312`) and then `ReadAsStringAsync` (`:337`) — the *entire* response body is materialized in memory with **no size cap**. `MaxErrorBodyChars` (`:386`) truncates only the string embedded into error messages, *after* the full read. The inbound side caps request bodies at 1 MiB precisely because "no rate limiting… an explicit, modest cap bounds per-request allocations" (`InboundApiOptions.cs:16-19`); the outbound side extends no such protection against a misbehaving or compromised external system streaming hundreds of MB into a site node (which also runs the instance actors). Use `HttpCompletionOption.ResponseHeadersRead` plus a bounded read (the audit middleware's cap+1 pattern is directly reusable).
+
+### [Medium] Serial per-recipient Twilio POSTs inside a serial dispatch sweep
+
+`SmsNotificationDeliveryAdapter` sends one POST per phone number sequentially (`SmsNotificationDeliveryAdapter.cs:148-170`), and the outbox dispatcher delivers each due notification sequentially within a sweep (`NotificationOutboxActor.cs:320-345`). Worst case for one 50-recipient list against a black-holing endpoint: 50 × 30 s = 25 minutes inside one sweep, during which *all* other due notifications (including email) queue behind it and appear "stuck" on the KPI tiles. Bounded parallelism per notification (e.g. `Parallel.ForEachAsync` with 4–8 DOP — Twilio tolerates it) and/or first-transient short-circuit (see Stability above) would bound the sweep. At current scale (small lists) this is latent, not live.
+
+### [Low] Per-delivery TCP+TLS SMTP handshake
+
+`MailKitSmtpClientWrapper` is deliberately one-connection-per-delivery, extensively documented with the factory seam left open for pooling (`MailKitSmtpClientWrapper.cs:12-38`). Correct tradeoff at current volume; noted only as the first knob if notification volume grows.
+
+### [Low] Startup compiles every inbound method serially
+
+`Host/Program.cs:383-387` runs Roslyn compilation per method in a foreach before `app.RunAsync()`. Fine at tens of methods; would stretch central startup (and failover-recovery window) at hundreds. `Parallel.ForEach` or lazy-only compilation are cheap options later.
+
+### [Good] The hot-path hygiene elsewhere is exemplary
+
+Bounded audit capture at the source with `ArrayPool` scratch buffers and cap+1 over-read detection (`AuditWriteMiddleware.cs:442-502`), fire-and-forget audit write with fault observation (`:358-359,380-398`), `EnableBuffering` skipped for bodyless requests (`:169-191`), name-keyed indexed repository lookups on the ESG hot path (`ExternalSystemClient.cs:540-550`), known-bad-method cache preventing per-request Roslyn recompiles with a 1000-entry flood cap (`InboundScriptExecutor.cs:38-63`), and per-system `HttpClient` factory clients with `MaxConnectionsPerServer` scoped to gateway-owned names only — explicitly avoiding process-global handler replacement (`ExternalSystemGateway/ServiceCollectionExtensions.cs:28-44,92-105`). No socket-exhaustion patterns anywhere: all four components use `IHttpClientFactory` (DelmiaNotifier's single-shot process makes its one `new HttpClient` correct).
+
+## Findings — Dimension 3: Conventions & Spec Fidelity
+
+### [High] Spec drift: per-system ESG timeout is specified but not implemented
+
+`Component-ExternalSystemGateway.md:38` ("**Timeout**: Per-system timeout for all method calls") and `:101` are explicit, but `ExternalSystemDefinition` has no timeout field (`Commons/Entities/ExternalSystems/ExternalSystemDefinition.cs`) and the client acknowledges it: "ExternalSystemDefinition has no per-system Timeout field yet, so the configured DefaultHttpTimeout is the effective round-trip limit" (`ExternalSystemClient.cs:300-305`). Every external system — a fast local MES and a slow remote recipe manager alike — shares one global 30 s (`ExternalSystemGatewayOptions.cs:9`). Repo rules say the design doc is the spec and doc+code travel together; either add the column (entity + EF config + migration + CLI/UI) or amend the spec to the global-timeout reality.
+
+### [Medium] Spec drift: path templates (`/recipes/{id}`) are never substituted
+
+The spec's method definition shows a templated relative path (`Component-ExternalSystemGateway.md:43`), but `BuildUrl` (`ExternalSystemClient.cs:431-463`) concatenates the path verbatim and routes parameters exclusively to the query string (GET/DELETE) or JSON body (POST/PUT/PATCH). A method authored with `{id}` in its path literally calls `.../recipes/{id}`. Either implement `{param}` substitution (with the consumed parameter removed from body/query) or strike the example from the spec before someone authors against it.
+
+### [Low] Spec drift: inbound error responses lack the documented `code` field
+
+`Component-InboundAPI.md:91-97` documents failure bodies as `{"error": ..., "code": "SITE_UNREACHABLE"}`; every actual error body is `{ error }` only (`EndpointExtensions.cs:111-113,153-155,191-193,212-214,253-255`) and a routed site-unreachable failure surfaces as a generic 500 "Internal script error" (`InboundScriptExecutor.cs:382-387` swallows the `InvalidOperationException` from `RouteTarget.Call`, `RouteHelper.cs:172-174`). Machine-readable error codes matter to exactly the audience of this API (external integrators); implement or de-spec.
+
+### [Low] OAuth2 SMTP is Microsoft-365-only despite a broader spec
+
+`OAuth2TokenService` hardcodes `https://login.microsoftonline.com/{tenant}/oauth2/v2.0/token` and the `outlook.office365.com/.default` scope (`OAuth2TokenService.cs:73,80`); the spec says "For Microsoft 365 **and other modern SMTP providers** that require OAuth2" (`Component-NotificationService.md:48`). A configurable authority/scope on `SmtpConfiguration` is a small change; until then the spec overpromises.
+
+### [Low] `InboundApiEndpointFilter` freezes its options at construction
+
+The filter is a singleton taking `IOptions<InboundApiOptions>` (`InboundApiEndpointFilter.cs:32-38`), so a live change to `MaxRequestBodyBytes` is ignored until restart — while the sibling `AuditWriteMiddleware` deliberately hot-reads `IOptionsMonitor<AuditLogOptions>` per request (`AuditWriteMiddleware.cs:97,149`). Inconsistent; use the monitor.
+
+### [Good] Convention adherence is otherwise strong
+
+Options pattern with startup validators throughout (`SmsOptionsValidator`, `NotificationOptions`); additive message-contract evolution respected and even documented at the parameter level ("this optional parameter trails the CancellationToken because it was appended additively", `InboundScriptExecutor.cs:243-246`); secrets consistently encrypted at rest via `EncryptedStringConverter` (ESG `AuthConfiguration` + DB `ConnectionString`: `Configurations/ExternalSystemConfiguration.cs:27-29,83-85`; SMTP credentials + Twilio AuthToken: `Configurations/NotificationConfiguration.cs:69,118`); connection strings never leak into error text (`DatabaseGateway.cs:298`, enforced in messages); credential scrubbing shared, not duplicated (`CredentialRedactor.cs`); API-key auth delegated to the shared `ZB.MOM.WW.Auth.ApiKeys` package (`InboundAPI.csproj:34`) with peppered-HMAC constant-time verification per spec (`Component-InboundAPI.md:66` — the package itself is outside this repo and was not independently verified), generic 401 messages, and byte-identical 403s for not-found vs not-in-scope with the DB lookup ordered *after* the in-memory scope check to close the timing oracle (`EndpointExtensions.cs:117-156`).
+
+## Underdeveloped Areas
+
+1. **Script trust boundary is static-only.** `ForbiddenApiChecker` is candid: semantic + reflection-gateway hardening, "not a true sandbox… genuine containment needs a runtime boundary" (`ForbiddenApiChecker.cs:24-29`). Inbound scripts run in-process on the central node with `Microsoft.CSharp` dynamic binder referenced (`InboundScriptExecutor.cs:172-184`). The deferred restricted-`AssemblyLoadContext`/out-of-process option remains the single biggest security-hardening gap on this boundary.
+2. **No cache-coherence mechanism for compiled handlers** (the High finding above) — no revision hash, no cluster invalidation message, no recompile-on-activation hook in `IActiveNodeGate`.
+3. **Per-SMS retry settings are dead fields.** `SmsConfiguration.MaxRetries/RetryDelay` are persisted and transported but never read; SMTP config governs retry for *all* notification types (`NotificationOutboxActor.cs:358-401`; documented as deferred in `Component-NotificationService.md:65`).
+4. **Twilio delivery is accept-only.** No status-callback webhook, no per-recipient delivery state (both documented as out-of-scope/future — `Component-NotificationService.md:93,102`). Also `AccountSid` is interpolated un-escaped into the request URI (`SmsNotificationDeliveryAdapter.cs:131`) — harmless for a valid SID, but a validation regex at config-save time would close the door.
+5. **DelmiaNotifier duplicate-delivery semantics undocumented** (Stability finding); AOT publish is Windows-only and the live smoke is manual (`2026-06-26-delmia-recipe-notifier-design.md:133-136,150-151`) — no CI artifact for the actual shipped binary.
+6. **Test gaps** (coverage is otherwise broad and genuinely good across all five test projects — endpoint gating, content-type case-sensitivity, schema-`$ref` runtime resolution, failover loop, both adapters, all four classifiers): nothing exercises the failover/staleness scenario for compiled handlers, nothing exercises an oversized outbound response body, and no test covers a multi-recipient SMS list with a mid-list transient (the duplicate-amplification path).
+7. **API key rotation** is enable/disable + scope-set only (`ManagementActor.cs:2685+`); a documented dual-key rotation procedure (create → migrate caller → disable) would round out the ops story — the `sbk_<keyId>_<secret>` design supports it, it's just unwritten.
+
+## Prioritized Recommendations
+
+1. **[High] Make the inbound handler cache self-healing.** Store the compiled delegate together with a hash of the script text; on each request compare against the freshly-fetched `ApiMethod.Script` and recompile on mismatch (also purging `_knownBadMethods`). One change fixes failover staleness, direct-SQL staleness, and stale-bad-method staleness simultaneously; the DB row is already fetched per request so the marginal cost is one string hash.
+2. **[High] Close the two ESG spec gaps** — per-system `Timeout` column (entity + EF migration + honored in `InvokeHttpAsync`) and either implement `{param}` path templating or amend `Component-ExternalSystemGateway.md`. Per repo rules, doc and code must travel together.
+3. **[Medium] Cap outbound response buffering** in `ExternalSystemClient` (`ResponseHeadersRead` + bounded read, reusing the audit-capture pattern).
+4. **[Medium] Bound the SMS blast radius**: short-circuit the recipient loop on first transient, and add bounded parallelism (or per-notification concurrency in the dispatch sweep) so one dead endpoint cannot stall the whole outbox.
+5. **[Medium] Replace colon-split `AuthConfiguration` parsing with structured JSON** (the entity already claims to be JSON) with a backward-compatible fallback.
+6. **[Medium] Handle abandoned inbound executions**: defer DI-scope disposal to actual handler completion, and add an orphaned-execution counter to the health snapshot.
+7. **[Low] Document DelmiaNotifier's at-least-once failover semantics** and verify the deployed `DelmiaRecipeDownload` script is idempotent.
+8. **[Low] Small consistency fixes**: `IOptionsMonitor` in `InboundApiEndpointFilter`; token parameter on `ErrorClassifier.IsTransient(Exception)`; configurable OAuth2 authority/scope; single-active-SMTP-config enforcement; implement or de-spec the inbound `code` error field.