merge(r2): r2-plan06
# Conflicts: # CLAUDE.md
This commit is contained in:
Joseph Doherty
@@ -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, template script + alarm Expression-trigger bodies, **and instance alarm-override trigger expressions** — R2-05) 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 (startup-validated `>0`, R2-05). **R2-05 also:** the post-commit `ScriptArtifactsChanged` publisher now covers `Add` resolutions (not just Overwrite/Rename), import warns on instance overrides targeting locked template members (advisory, rows still written), and the Expression-trigger syntax check is surface-keyed-cached + skipped on read-only staleness/comparison paths (`validateScriptCompilation:false`). 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, template script + alarm Expression-trigger bodies, **and instance alarm-override trigger expressions** — R2-05) 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 (startup-validated `>0`, R2-05). **R2-05 also:** the post-commit `ScriptArtifactsChanged` publisher now covers `Add` resolutions (not just Overwrite/Rename), import warns on instance overrides targeting locked template members (advisory, rows still written), and the Expression-trigger syntax check is surface-keyed-cached + skipped on read-only staleness/comparison paths (`validateScriptCompilation:false`). 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.
|
||||
|
||||
@@ -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).
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -118,13 +118,13 @@ The `sbk_<keyId>_<secret>` 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`. |
|
||||
| `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). |
|
||||
|
||||
|
||||
@@ -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 <see cref="TransientExternalSystemException"/> 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
|
||||
/// <see cref="ArgumentException"/> from an unresolvable <c>{param}</c> path
|
||||
/// template or an unsupported HTTP verb — return false (park immediately) rather
|
||||
/// than retry, because re-running the same stored payload can never succeed.
|
||||
/// </summary>
|
||||
/// <param name="message">The buffered message to deliver.</param>
|
||||
/// <param name="cancellationToken">Cancellation token.</param>
|
||||
@@ -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.
|
||||
}
|
||||
|
||||
@@ -447,6 +466,12 @@ public class ExternalSystemClient : IExternalSystemClient
|
||||
/// into store-and-forward would defeat the cap. Cancellation
|
||||
/// (<see cref="OperationCanceledException"/>) is allowed to propagate so the
|
||||
/// caller's ordered timeout/cancellation catch filters classify it.
|
||||
/// <para>
|
||||
/// N4: the accumulated bytes are decoded using the response's declared charset
|
||||
/// (<c>Content-Type: …; charset=…</c>) via <see cref="ResolveEncoding"/> — parity
|
||||
/// with the pre-bounded-read <c>ReadAsStringAsync</c>. A null, empty, or
|
||||
/// unrecognized charset falls back to UTF-8.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
private static async Task<string> ReadBodyBoundedAsync(
|
||||
HttpContent content, string systemName, long maxBytes, CancellationToken token)
|
||||
@@ -479,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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Resolves the declared response charset to an <see cref="Encoding"/>, tolerating
|
||||
/// the quoted form some servers emit (<c>charset="iso-8859-1"</c>). Null, empty,
|
||||
/// or unrecognized values fall back to UTF-8.
|
||||
/// </summary>
|
||||
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)
|
||||
|
||||
@@ -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>();
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// C3: a routed call failed because the target site could not be reached (no contact /
|
||||
/// unreachable). Derives from <see cref="InvalidOperationException"/> so existing
|
||||
/// callers that catch the base type are unaffected, while
|
||||
/// <see cref="InboundScriptExecutor"/> catches this more specific type to surface the
|
||||
/// spec's <c>SITE_UNREACHABLE</c> error code instead of a generic <c>SCRIPT_ERROR</c>.
|
||||
/// A routing failure whose message does <em>not</em> indicate unreachability stays a
|
||||
/// C3/N3: a routed call failed because the target site never answered — the routed
|
||||
/// Ask <b>expired</b>. Unreachability is typed at the Ask boundary from
|
||||
/// <see cref="AskTimeoutException"/> (the communication layer's own signal: an
|
||||
/// unreachable/contactless site has its enveloped message dropped so the Ask times
|
||||
/// out caller-side) — it is <em>never</em> inferred from an error-message substring.
|
||||
/// Derives from <see cref="InvalidOperationException"/> so existing callers that
|
||||
/// catch the base type are unaffected, while <see cref="InboundScriptExecutor"/>
|
||||
/// catches this more specific type to surface the spec's <c>SITE_UNREACHABLE</c>
|
||||
/// error code instead of a generic <c>SCRIPT_ERROR</c>. A <c>Success=false</c>
|
||||
/// <em>response</em> means the site answered (reachable by definition) and stays a
|
||||
/// plain <see cref="InvalidOperationException"/>.
|
||||
/// </summary>
|
||||
public sealed class SiteUnreachableException : InvalidOperationException
|
||||
@@ -18,6 +23,11 @@ public sealed class SiteUnreachableException : InvalidOperationException
|
||||
/// <summary>Initializes a new <see cref="SiteUnreachableException"/> with the given message.</summary>
|
||||
/// <param name="message">The routing-level failure message that indicated the site was unreachable.</param>
|
||||
public SiteUnreachableException(string message) : base(message) { }
|
||||
|
||||
/// <summary>Initializes a new <see cref="SiteUnreachableException"/> wrapping the underlying Ask-timeout.</summary>
|
||||
/// <param name="message">The routing-level failure message.</param>
|
||||
/// <param name="innerException">The underlying <see cref="AskTimeoutException"/> from the routed Ask.</param>
|
||||
public SiteUnreachableException(string message, Exception innerException) : base(message, innerException) { }
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
@@ -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
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// C3: classifies a routing-level failure message. When the message indicates the
|
||||
/// site could not be reached (unreachable / no contact) the failure is a
|
||||
/// <see cref="SiteUnreachableException"/> — so <see cref="InboundScriptExecutor"/>
|
||||
/// can surface the spec's <c>SITE_UNREACHABLE</c> code — otherwise a plain
|
||||
/// <see cref="InvalidOperationException"/>.
|
||||
/// 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).
|
||||
/// </summary>
|
||||
private static InvalidOperationException RoutingFailure(string message) =>
|
||||
IsUnreachable(message)
|
||||
? new SiteUnreachableException(message)
|
||||
: new InvalidOperationException(message);
|
||||
|
||||
private static bool IsUnreachable(string message) =>
|
||||
message.Contains("unreachable", StringComparison.OrdinalIgnoreCase)
|
||||
|| message.Contains("no contact", StringComparison.OrdinalIgnoreCase)
|
||||
|| message.Contains("no-contact", StringComparison.OrdinalIgnoreCase);
|
||||
private static async Task<TResponse> RouteOrUnreachable<TResponse>(Task<TResponse> 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);
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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 <see cref="InvalidOperationException"/>
|
||||
/// (SCRIPT_ERROR at the endpoint). Unreachability is typed at the Ask boundary
|
||||
/// by <c>RouteOrUnreachable</c> (AskTimeoutException → SiteUnreachableException),
|
||||
/// replacing the old message-substring sniff that a harmless rewording in the
|
||||
/// communication layer could silently defeat.
|
||||
/// </summary>
|
||||
private static InvalidOperationException RoutingFailure(string message) => new(message);
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// 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 <see cref="IScriptArtifactChangeBus"/> on host
|
||||
/// start and, for every <c>ApiMethod</c> notification (e.g. a Transport bundle
|
||||
/// import overwriting method scripts, published post-commit), drops the compiled
|
||||
/// handler AND the known-bad record via
|
||||
/// <see cref="InboundScriptExecutor.InvalidateMethod"/>, so the next request
|
||||
/// recompiles from the fresh row without waiting for the per-request
|
||||
/// revision-check self-heal.
|
||||
/// <para>
|
||||
/// Fast-path only: the revision check in <c>ExecuteAsync</c> 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.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
public sealed class ScriptArtifactChangeSubscriber : IHostedService
|
||||
{
|
||||
private readonly InboundScriptExecutor _executor;
|
||||
private readonly ILogger<ScriptArtifactChangeSubscriber> _logger;
|
||||
private readonly IScriptArtifactChangeBus? _bus;
|
||||
private IDisposable? _subscription;
|
||||
|
||||
/// <summary>Initializes the subscriber.</summary>
|
||||
/// <param name="executor">The compiled-handler cache to invalidate.</param>
|
||||
/// <param name="logger">Logger instance.</param>
|
||||
/// <param name="bus">The change bus, or null when the host registers none (site roles, tests).</param>
|
||||
public ScriptArtifactChangeSubscriber(
|
||||
InboundScriptExecutor executor,
|
||||
ILogger<ScriptArtifactChangeSubscriber> logger,
|
||||
IScriptArtifactChangeBus? bus = null)
|
||||
{
|
||||
_executor = executor;
|
||||
_logger = logger;
|
||||
_bus = bus;
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
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;
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
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);
|
||||
}
|
||||
}
|
||||
@@ -27,6 +27,12 @@ public static class ServiceCollectionExtensions
|
||||
// body size cap and active-node gating for POST /api/{methodName}.
|
||||
services.AddSingleton<InboundApiEndpointFilter>();
|
||||
|
||||
// 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<ScriptArtifactChangeSubscriber>();
|
||||
|
||||
return services;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
@@ -1341,4 +1342,93 @@ 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<string>())
|
||||
.Returns(new HttpClient(new MockHttpMessageHandler(HttpStatusCode.OK, "{}")));
|
||||
var client = new ExternalSystemClient(
|
||||
_httpClientFactory, _repository, NullLogger<ExternalSystemClient>.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<ExternalSystemClient>.Instance);
|
||||
|
||||
var delivered = await client.DeliverBufferedAsync(BufferedCall("TestAPI", "oddVerb"));
|
||||
|
||||
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<string>()).Returns(new HttpClient(handler));
|
||||
var client = new ExternalSystemClient(
|
||||
_httpClientFactory, _repository, NullLogger<ExternalSystemClient>.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<string>()).Returns(new HttpClient(handler));
|
||||
var client = new ExternalSystemClient(
|
||||
_httpClientFactory, _repository, NullLogger<ExternalSystemClient>.Instance);
|
||||
|
||||
var result = await client.CallAsync("TestAPI", "getData");
|
||||
|
||||
Assert.True(result.Success);
|
||||
Assert.Contains("ok", result.ResponseJson);
|
||||
}
|
||||
|
||||
/// <summary>Test helper: returns a fixed byte body with an explicit Content-Type.</summary>
|
||||
private sealed class ByteBodyHandler(
|
||||
HttpStatusCode statusCode, byte[] body, string contentType) : HttpMessageHandler
|
||||
{
|
||||
protected override Task<HttpResponseMessage> 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);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<IInboundApiRepository>();
|
||||
repo.GetMethodByNameAsync(methodName, Arg.Any<CancellationToken>()).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<IInboundApiRepository>();
|
||||
repo.GetMethodByNameAsync(methodName, Arg.Any<CancellationToken>()).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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Content whose length is never computable, forcing HttpClient to transmit it
|
||||
/// chunked (no Content-Length header ever reaches the server).
|
||||
/// </summary>
|
||||
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;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<IInstanceLocator>();
|
||||
locator.GetSiteIdForInstanceAsync("X", Arg.Any<CancellationToken>()).Returns("SiteA");
|
||||
var router = Substitute.For<IInstanceRouter>();
|
||||
router.RouteToCallAsync("SiteA", Arg.Any<RouteToCallRequest>(), Arg.Any<CancellationToken>())
|
||||
.Returns(ci => new RouteToCallResponse(
|
||||
((RouteToCallRequest)ci[1]).CorrelationId,
|
||||
Success: false, ReturnValue: null, ErrorMessage: "Site unreachable",
|
||||
DateTimeOffset.UtcNow));
|
||||
.Returns(Task.FromException<RouteToCallResponse>(
|
||||
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<IInstanceLocator>();
|
||||
locator.GetSiteIdForInstanceAsync("X", Arg.Any<CancellationToken>()).Returns("SiteA");
|
||||
var router = Substitute.For<IInstanceRouter>();
|
||||
router.RouteToCallAsync("SiteA", Arg.Any<RouteToCallRequest>(), Arg.Any<CancellationToken>())
|
||||
.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<string, object?>(), route, TimeSpan.FromSeconds(10));
|
||||
|
||||
Assert.False(result.Success);
|
||||
Assert.Equal("SCRIPT_ERROR", result.ErrorCode);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task HandlerTimesOut_ReturnsTimeoutError()
|
||||
{
|
||||
|
||||
@@ -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<RouteToCallRequest>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Task.FromException<RouteToCallResponse>(
|
||||
new Akka.Actor.AskTimeoutException("Timeout after 00:00:30")));
|
||||
|
||||
await Assert.ThrowsAsync<SiteUnreachableException>(
|
||||
() => CreateHelper().To("inst").Call("s"));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task GetAttributes_AskTimeout_ThrowsSiteUnreachableException()
|
||||
{
|
||||
SiteResolves("inst", "SiteA");
|
||||
_router.RouteToGetAttributesAsync("SiteA", Arg.Any<RouteToGetAttributesRequest>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Task.FromException<RouteToGetAttributesResponse>(
|
||||
new Akka.Actor.AskTimeoutException("Timeout after 00:00:30")));
|
||||
|
||||
await Assert.ThrowsAsync<SiteUnreachableException>(
|
||||
() => CreateHelper().To("inst").GetAttributes(new[] { "a" }));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// 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).
|
||||
/// </summary>
|
||||
public class ScriptArtifactChangeSubscriberTests
|
||||
{
|
||||
/// <summary>Minimal in-test bus: records subscriptions, delivers synchronously.</summary>
|
||||
private sealed class RecordingBus : IScriptArtifactChangeBus
|
||||
{
|
||||
private readonly List<Action<ScriptArtifactsChanged>> _handlers = new();
|
||||
public int SubscriberCount => _handlers.Count;
|
||||
|
||||
public void Publish(ScriptArtifactsChanged notification)
|
||||
{
|
||||
foreach (var handler in _handlers.ToArray()) handler(notification);
|
||||
}
|
||||
|
||||
public IDisposable Subscribe(Action<ScriptArtifactsChanged> 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<InboundScriptExecutor>.Instance, Substitute.For<IServiceProvider>());
|
||||
private readonly RecordingBus _bus = new();
|
||||
private readonly RouteHelper _route = new(
|
||||
Substitute.For<IInstanceLocator>(), Substitute.For<IInstanceRouter>());
|
||||
|
||||
private ScriptArtifactChangeSubscriber CreateSubscriber(IScriptArtifactChangeBus? bus) =>
|
||||
new(_executor, NullLogger<ScriptArtifactChangeSubscriber>.Instance, bus);
|
||||
|
||||
private Task<InboundScriptResult> Run(ApiMethod m) => _executor.ExecuteAsync(
|
||||
m, new Dictionary<string, object?>(), _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);
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user