Joseph Doherty
3a21e77c03
Corrects the Status Board (PLAN-04 24/26, not 0), and adds a Parallelization Map to 00-MASTER-TRACKER plus a per-plan Parallelization block to PLAN-04/05/06/07/08. Derives ready sets from the live .tasks.json blockedBy graphs and the Files metadata, identifies the initiative-wide serialization mutexes (ManagementActor.cs single-writer, EF migration snapshot, Host/Program.cs, ArtifactDeploymentService.cs, SiteStorageService.cs, NotificationOutboxActor.cs, AlarmSummary.razor), and recommends the safe concurrent subagent lanes per plan. Flags PLAN-07 T33 as a dedupe-skip (CLI.Tests already in slnx).
832 lines
73 KiB
Markdown
832 lines
73 KiB
Markdown
# Edge Integrations Fix Implementation Plan
|
||
|
||
> **For Claude:** REQUIRED SUB-SKILL: Use superpowers-extended-cc:executing-plans to implement this plan task-by-task.
|
||
|
||
**Goal:** Fix every finding in `archreview/06-edge-integrations.md` — make the inbound compiled-handler cache self-healing (revision-checked, failover/direct-SQL/import-safe), close both ESG spec gaps (per-system timeout, path templating), bound outbound response buffering and SMS blast radius, wire the dead SMS retry fields, and clear the report's Medium/Low stability, performance, and convention findings across InboundAPI, ExternalSystemGateway, NotificationService/Outbox delivery, and DelmiaNotifier.
|
||
|
||
**Architecture:** The core stability fix replaces the name-keyed compiled-handler cache in `InboundScriptExecutor` with a script-revision-checked cache (the fresh `ApiMethod` row is already in hand on every request — one ordinal string compare detects staleness and triggers recompile), plus a public `InvalidateMethod` seam that plan 05's "script artifact changed → invalidate" contract will consume. The ESG per-system timeout is a full vertical slice per repo rules: Commons entity → EF mapping + migration → artifact message contract (additive) → site SQLite column → client honor → management command → CLI → UI → design doc. Notification fixes are localized to the delivery adapters and the outbox dispatcher's retry-policy resolution.
|
||
|
||
**Tech Stack:** C#/.NET 9, xUnit + NSubstitute, EF Core migrations (MS SQL central), Microsoft.Data.Sqlite (site), Roslyn scripting, System.CommandLine (CLI), Blazor Server (UI).
|
||
|
||
Build: `dotnet build ZB.MOM.WW.ScadaBridge.slnx`. Test per-project: `dotnet test tests/<project>`. EF migrations: **build first, never `--no-build`**; `dotnet ef migrations add <Name> --project src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase --startup-project src/ZB.MOM.WW.ScadaBridge.Host`. If the migration scaffolds empty, delete the files and rebuild first.
|
||
|
||
## Parallelization (incomplete tasks) — 2026-07-09
|
||
|
||
Nothing complete yet. **Ready now** (`blockedBy` empty): **{1, 6, 7, 8, 17, 23}**.
|
||
|
||
**Concurrent lanes (dispatch as separate implementers):**
|
||
- **Free / file-disjoint:** T1 (`InboundScriptExecutor.cs`, high-risk), T6 (`InboundApiEndpointFilter.cs`, trivial), T17 (`SmsNotificationDeliveryAdapter.cs`), T23 (DelmiaNotifier docs).
|
||
- **Migration holder (single-writer, initiative-wide):** T8 (ExternalSystem `TimeoutSeconds` entity + EF migration) — never run alongside another migration task (PLAN-06 T19, etc.). Unblocks the ExternalSystem chain T9–T16, T19.
|
||
- **`Host/Program.cs` mutex:** T7 — coordinate with PLAN-08 T4 (both edit `Program.cs`).
|
||
|
||
**Downstream chains (blocked until their roots land):** inbound contract T2→T3→T4→T5; ExternalSystem T8→{T9,T10,T11,T12,T19}→…; Notification T18/T19→T20/T21→T22. Several of these (T2, T3, T11, T20, T22) touch **`ManagementActor.cs`** → they enter the initiative-wide ManagementActor mutex when ready. T18/T21 own `NotificationOutboxActor.cs`. See [00-MASTER-TRACKER.md § Parallelization Map](00-MASTER-TRACKER.md#parallelization-map--incomplete-tasks-2026-07-09).
|
||
|
||
## Findings Coverage
|
||
|
||
| # | Finding (report section) | Severity | Task(s) |
|
||
|---|--------------------------|----------|---------|
|
||
| S1 | Compiled inbound handler cache stale across central failover (+ known-bad mirror image) | High | 1, 2, 3 |
|
||
| S2 | Method timeout abandons running script; DI scope disposed underneath (use-after-dispose + orphan accumulation, no counter) | Medium | 4 |
|
||
| S3 | ESG API-key `AuthConfiguration` colon-splitting ambiguous (also silently misparses the JSON shapes the UI placeholder already suggests) | Medium | 15 |
|
||
| S4 | DelmiaNotifier failover can duplicate a processed notification (at-least-once undocumented) | Medium | 23 |
|
||
| S5 | SMS adapter keeps sending after first transient — duplicate amplification on retry | Medium | 17 |
|
||
| S6 | `ErrorClassifier.IsTransient(Exception)` treats any OCE as transient (misuse-prone public predicate) | Low | 16 |
|
||
| S7 | First-of-many SMTP configuration nondeterministic (`FirstOrDefault` in adapter + retry-policy resolver) | Low | 21 |
|
||
| S8 | OAuth2 token cache: unsynchronized field visibility (growth is bounded in practice — accepted) | Low | 21 |
|
||
| S9 | Non-JSON content-type with body yields misleading 400 instead of 415 | Low | 5 |
|
||
| P1 | Outbound HTTP responses buffered unbounded | Medium | 14 |
|
||
| P2 | Serial per-recipient Twilio POSTs inside serial dispatch sweep | Medium | 17 (first-transient short-circuit bounds the black-hole worst case; bounded parallelism **deferred** — at current list sizes short-circuit alone caps the sweep at ~1 timeout) |
|
||
| P3 | Per-delivery TCP+TLS SMTP handshake | Low | **Won't-fix** — deliberate, documented tradeoff with the pooling seam already left open (`MailKitSmtpClientWrapper.cs:12-38`); report itself says "correct tradeoff at current volume" |
|
||
| P4 | Startup compiles every inbound method serially | Low | 7 |
|
||
| C1 | Spec drift: per-system ESG timeout specified but not implemented | High | 8, 9, 10, 11, 12 |
|
||
| C2 | Spec drift: path templates (`/recipes/{id}`) never substituted | Medium | 13 |
|
||
| C3 | Spec drift: inbound error responses lack documented `code` field (incl. SITE_UNREACHABLE surfacing as generic 500) | Low | 5 |
|
||
| C4 | OAuth2 SMTP is Microsoft-365-only despite broader spec | Low | 19, 20 |
|
||
| C5 | `InboundApiEndpointFilter` freezes options at construction | Low | 6 |
|
||
| U1 | Script trust boundary is static-only (no runtime sandbox) | Underdeveloped | **Deferred** — restricted-ALC/out-of-process containment is an architecture change already logged as deferred to v1.x in the codebase's own deferred-work inventory; interacts with plan 05's Script Analysis ownership. Not a bite-sized fix. |
|
||
| U2 | No cache-coherence mechanism for compiled handlers | Underdeveloped | 1, 3 (revision-check fallback works standalone; Task 3 provides the consumer seam for plan 05's invalidation contract) |
|
||
| U3 | Per-SMS retry settings are dead fields (`SmsConfiguration.MaxRetries`/`RetryDelay` never read) | Underdeveloped | 18 |
|
||
| U4 | Twilio accept-only + `AccountSid` interpolated un-escaped into URI | Underdeveloped | 22 (SID format validation at save). Status-callback webhook / per-recipient delivery state: **Deferred** — documented out-of-scope in `Component-NotificationService.md:93,102`; needs an inbound webhook endpoint (new attack surface) that warrants its own design pass. |
|
||
| U5 | DelmiaNotifier duplicate-delivery semantics undocumented; no CI artifact for shipped binary | Underdeveloped | 23 (docs). Windows AOT CI artifact: **Deferred** — repo has no Windows CI runner; the design doc's manual-smoke note stands. |
|
||
| U6 | Test gaps: handler failover/staleness, oversized outbound body, mid-list SMS transient | Underdeveloped | 1, 14, 17 (each task's failing-test step is exactly the missing test) |
|
||
| U7 | API key rotation procedure unwritten | Underdeveloped | 24 |
|
||
|
||
---
|
||
|
||
### Task 1: Revision-checked inbound handler cache (self-healing across failover / direct SQL / known-bad)
|
||
|
||
**Classification:** high-risk (concurrency; changes the serving semantics of every inbound API method)
|
||
**Estimated implement time:** 5 min
|
||
**Parallelizable with:** 6, 7, 8, 17, 18, 23, 24
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs` (lines 26, 38-63, 118-143, 311-331)
|
||
- Test (create): `tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorStalenessTests.cs`
|
||
|
||
Design: the cache value becomes `CachedHandler(string? Script, Func<InboundScriptContext, Task<object?>> Handler)`; `_knownBadMethods` becomes `ConcurrentDictionary<string, string>` (method name → the exact script text that failed). On every `ExecuteAsync`, the freshly-fetched `method.Script` is ordinal-compared against the cached script; mismatch → recompile in place (and purge the known-bad record). A known-bad record only short-circuits when the *current* script is the one that failed — a changed script always gets a fresh compile. `RegisterHandler(name, handler)` (test seam) stores `Script = null`, meaning "no staleness info, serve as-is" — existing tests keep passing; all production compiles flow through `CompileAndRegister`, which always records the script text. This fixes failover staleness, direct-SQL-edit staleness, and stale known-bad in one move; marginal per-request cost is one ordinal string compare of a few-KB script.
|
||
|
||
Behavioral consequence (deliberate, see Task 2): the DB row becomes authoritative. A saved-but-broken script now returns HTTP 500 "Script compilation failed" on every node instead of the old delegate silently serving on the node that compiled it — honest failure replaces node-divergent stale success.
|
||
|
||
**Steps:**
|
||
|
||
1. Write the failing tests:
|
||
|
||
```csharp
|
||
// tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorStalenessTests.cs
|
||
using Microsoft.Extensions.Logging.Abstractions;
|
||
using NSubstitute;
|
||
using ZB.MOM.WW.ScadaBridge.Commons.Entities.InboundApi;
|
||
using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services;
|
||
|
||
namespace ZB.MOM.WW.ScadaBridge.InboundAPI.Tests;
|
||
|
||
/// <summary>
|
||
/// Arch-review S1: the compiled-handler cache must be revision-checked against the
|
||
/// freshly-fetched ApiMethod.Script so a standby node (whose cache was populated at
|
||
/// its own startup) never serves a stale delegate after failover, and a method fixed
|
||
/// out-of-band is never stuck behind a stale known-bad record.
|
||
/// </summary>
|
||
public class InboundScriptExecutorStalenessTests
|
||
{
|
||
private readonly InboundScriptExecutor _executor;
|
||
private readonly RouteHelper _route;
|
||
|
||
public InboundScriptExecutorStalenessTests()
|
||
{
|
||
_executor = new InboundScriptExecutor(
|
||
NullLogger<InboundScriptExecutor>.Instance, Substitute.For<IServiceProvider>());
|
||
_route = new RouteHelper(
|
||
Substitute.For<IInstanceLocator>(), Substitute.For<IInstanceRouter>());
|
||
}
|
||
|
||
private Task<InboundScriptResult> Run(ApiMethod m) => _executor.ExecuteAsync(
|
||
m, new Dictionary<string, object?>(), _route, TimeSpan.FromSeconds(10));
|
||
|
||
[Fact]
|
||
public async Task UpdatedScript_RecompilesInsteadOfServingStaleHandler()
|
||
{
|
||
// Simulates the failover scenario: this node compiled v1 at startup...
|
||
var v1 = new ApiMethod("stale", "return 1;") { Id = 1, TimeoutSeconds = 10 };
|
||
Assert.True(_executor.CompileAndRegister(v1));
|
||
var r1 = await Run(v1);
|
||
Assert.Contains("1", r1.ResultJson);
|
||
|
||
// ...the active node updated the method (this node never heard about it);
|
||
// the per-request DB fetch hands ExecuteAsync the NEW row.
|
||
var v2 = new ApiMethod("stale", "return 2;") { Id = 1, TimeoutSeconds = 10 };
|
||
var r2 = await Run(v2);
|
||
|
||
Assert.True(r2.Success);
|
||
Assert.Contains("2", r2.ResultJson); // old cache would have returned 1
|
||
}
|
||
|
||
[Fact]
|
||
public async Task KnownBadMethod_FixedScript_CompilesInsteadOfFastFailing()
|
||
{
|
||
// The mirror-image variant: broken at this node's startup...
|
||
var broken = new ApiMethod("fixme", "%%% not C# %%%") { Id = 2, TimeoutSeconds = 10 };
|
||
Assert.False(_executor.CompileAndRegister(broken));
|
||
var r1 = await Run(broken);
|
||
Assert.False(r1.Success);
|
||
|
||
// ...fixed via the management path on the OTHER node; this node's fresh
|
||
// DB fetch carries the fixed script.
|
||
var fixedMethod = new ApiMethod("fixme", "return 42;") { Id = 2, TimeoutSeconds = 10 };
|
||
var r2 = await Run(fixedMethod);
|
||
|
||
Assert.True(r2.Success);
|
||
Assert.Contains("42", r2.ResultJson);
|
||
}
|
||
|
||
[Fact]
|
||
public async Task KnownBadMethod_SameScript_StillFastFails()
|
||
{
|
||
var broken = new ApiMethod("stillbad", "%%% not C# %%%") { Id = 3, TimeoutSeconds = 10 };
|
||
Assert.False(_executor.CompileAndRegister(broken));
|
||
|
||
// Same script text → the fast-fail record must hold (no per-request Roslyn).
|
||
var again = new ApiMethod("stillbad", "%%% not C# %%%") { Id = 3, TimeoutSeconds = 10 };
|
||
var r = await Run(again);
|
||
Assert.False(r.Success);
|
||
Assert.Contains("Script compilation failed", r.ErrorMessage);
|
||
}
|
||
|
||
[Fact]
|
||
public async Task TestSeamRegisterHandler_WithoutScript_IsNotRevisionChecked()
|
||
{
|
||
// RegisterHandler (script-less test seam) pins the handler regardless of row content.
|
||
var m = new ApiMethod("pinned", "return 0;") { Id = 4, TimeoutSeconds = 10 };
|
||
_executor.RegisterHandler("pinned", async _ => { await Task.CompletedTask; return 99; });
|
||
var r = await Run(m);
|
||
Assert.True(r.Success);
|
||
Assert.Contains("99", r.ResultJson);
|
||
}
|
||
}
|
||
```
|
||
|
||
2. Run: `dotnet test tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests --filter InboundScriptExecutorStalenessTests` — expect **FAIL** (`UpdatedScript_RecompilesInsteadOfServingStaleHandler` returns 1; `KnownBadMethod_FixedScript…` fast-fails).
|
||
|
||
3. Minimal implementation in `InboundScriptExecutor.cs`:
|
||
- Add `private sealed record CachedHandler(string? Script, Func<InboundScriptContext, Task<object?>> Handler);` and change `_scriptHandlers` to `ConcurrentDictionary<string, CachedHandler>`.
|
||
- Change `_knownBadMethods` to `ConcurrentDictionary<string, string>`; `TryRecordBadMethod(string methodName, string script)` stores the failing script text (cap logic unchanged; `_knownBadMethods[methodName] = script` when already present so a *newer* bad script replaces the record).
|
||
- `RegisterHandler(name, handler)` → `_scriptHandlers[name] = new CachedHandler(null, handler);` (doc-comment it as the script-less test seam, exempt from revision checks).
|
||
- `Register(methodName, handler)` → `Register(ApiMethod method, handler)` storing `new CachedHandler(method.Script, handler)`.
|
||
- In `ExecuteAsync`, replace the lookup block (lines 311-331) with:
|
||
|
||
```csharp
|
||
_scriptHandlers.TryGetValue(method.Name, out var cached);
|
||
// Revision check: a cached handler compiled from different script text is stale
|
||
// (standby-after-failover, direct SQL edit, bundle import). Null Script = test seam, exempt.
|
||
if (cached is { Script: not null }
|
||
&& !string.Equals(cached.Script, method.Script, StringComparison.Ordinal))
|
||
{
|
||
_logger.LogInformation(
|
||
"API method {Method} script changed since it was cached; recompiling", method.Name);
|
||
cached = null;
|
||
}
|
||
|
||
if (cached == null)
|
||
{
|
||
// Fast-fail only when the CURRENT script text is the one already known bad.
|
||
if (_knownBadMethods.TryGetValue(method.Name, out var badScript)
|
||
&& string.Equals(badScript, method.Script, StringComparison.Ordinal))
|
||
return new InboundScriptResult(false, null, "Script compilation failed for this method");
|
||
|
||
var (compiled, _) = Compile(method);
|
||
if (compiled == null)
|
||
{
|
||
TryRecordBadMethod(method.Name, method.Script ?? string.Empty);
|
||
return new InboundScriptResult(false, null, "Script compilation failed for this method");
|
||
}
|
||
|
||
_knownBadMethods.TryRemove(method.Name, out _);
|
||
cached = new CachedHandler(method.Script, compiled);
|
||
_scriptHandlers[method.Name] = cached; // last-write-wins; concurrent first-callers race benignly
|
||
}
|
||
|
||
var result = await cached.Handler(context).WaitAsync(cts.Token);
|
||
```
|
||
|
||
- In `CompileAndRegister` update the failure branch to `TryRecordBadMethod(method.Name, method.Script ?? string.Empty)`.
|
||
|
||
4. Run the new filter → **PASS**, then the whole project: `dotnet test tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests` → all green (existing `RegisterHandler`-based tests keep passing via the null-script seam).
|
||
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorStalenessTests.cs && git commit -m "fix(inbound-api): revision-check compiled handler cache against the fresh ApiMethod row — heals failover/direct-SQL/known-bad staleness"`
|
||
|
||
### Task 2: DB-authoritative compile semantics — management warning text + design doc
|
||
|
||
**Classification:** small
|
||
**Estimated implement time:** 3 min
|
||
**Parallelizable with:** 6, 7, 8, 17, 18, 23
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs` (`TryCompileAndDescribe`, ~lines 2646-2660: warning strings)
|
||
- Modify: `docs/requirements/Component-InboundAPI.md` (the "Direct SQL Warning" section, ~lines 129-131, plus the broken-save behavior description)
|
||
|
||
Task 1 changed the truth: after a broken save, the *old* handler no longer keeps serving (the per-request revision check bypasses it and the method 500s with a compile error on every node consistently). The management warning and the component doc must say so, or they now lie.
|
||
|
||
**Steps:**
|
||
|
||
1. In `TryCompileAndDescribe`, replace the `isUpdate` warning string with: `"Script saved but failed to compile; the method will return a compilation error on every request until a compiling version is saved. Compilation errors: {detail}"` (the create-path string is already correct). Update the `CompileAndRegister` XML doc-comment in `InboundScriptExecutor.cs` (lines 105-114) if Task 1 did not already: the "previously registered handler is left in place" sentence must be replaced with the DB-authoritative behavior.
|
||
2. In `Component-InboundAPI.md`, rewrite the Direct SQL Warning section: direct SQL edits are now picked up on the next request via the per-request script revision check (no restart / no management round-trip needed); note that a broken script serves a 500 compile-failure consistently on all central nodes.
|
||
3. Verify no stale text remains: `grep -rn "previously registered" src/ZB.MOM.WW.ScadaBridge.ManagementService src/ZB.MOM.WW.ScadaBridge.InboundAPI docs/requirements/Component-InboundAPI.md` — every remaining hit must describe the new semantics.
|
||
4. `dotnet build ZB.MOM.WW.ScadaBridge.slnx` → green; `dotnet test tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests` (some tests may assert the old warning text — update them to the new string).
|
||
5. Commit: `git add -A src/ZB.MOM.WW.ScadaBridge.ManagementService src/ZB.MOM.WW.ScadaBridge.InboundAPI tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests docs/requirements/Component-InboundAPI.md && git commit -m "docs(inbound-api): DB row is authoritative for method scripts — update broken-save warning + Direct SQL Warning for the revision-checked cache"`
|
||
|
||
### Task 3: `InvalidateMethod` seam — the Inbound API consumer of plan 05's invalidation contract
|
||
|
||
**Classification:** small
|
||
**Estimated implement time:** 3 min
|
||
**Parallelizable with:** 8, 9, 10, 13-17, 19, 23, 24
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs` (after Task 1)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs` (`HandleDeleteApiMethod`, ~line 2631: swap `RemoveHandler` for `InvalidateMethod`) (after Task 2)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorStalenessTests.cs` (append)
|
||
|
||
Plan 05 designs the general "script artifact changed → invalidate everywhere" contract (bundle import is its consumer to wire). This task gives the Inbound API one obvious, complete entry point: `InvalidateMethod(string methodName)` drops **both** the compiled handler and the known-bad record. Even before plan 05 lands, Task 1's revision check makes invalidation a fast-path optimization rather than a correctness requirement — that is the designed fallback.
|
||
|
||
**Steps:**
|
||
|
||
1. Failing test (append to `InboundScriptExecutorStalenessTests`):
|
||
|
||
```csharp
|
||
[Fact]
|
||
public async Task InvalidateMethod_DropsHandlerAndKnownBadRecord()
|
||
{
|
||
var good = new ApiMethod("inv", "return 5;") { Id = 5, TimeoutSeconds = 10 };
|
||
Assert.True(_executor.CompileAndRegister(good));
|
||
var broken = new ApiMethod("invbad", "%%%") { Id = 6, TimeoutSeconds = 10 };
|
||
Assert.False(_executor.CompileAndRegister(broken));
|
||
|
||
_executor.InvalidateMethod("inv");
|
||
_executor.InvalidateMethod("invbad");
|
||
|
||
Assert.Equal(0, _executor.KnownBadMethodCount);
|
||
// After invalidation the method lazily recompiles from the row — still serves.
|
||
var r = await Run(good);
|
||
Assert.True(r.Success);
|
||
}
|
||
```
|
||
|
||
2. `dotnet test tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests --filter InvalidateMethod_DropsHandlerAndKnownBadRecord` → **FAIL** (no such method).
|
||
3. Implement:
|
||
|
||
```csharp
|
||
/// <summary>
|
||
/// Drops the compiled handler AND any known-bad record for the method. The
|
||
/// consumption seam for the script-artifact invalidation contract (bundle import,
|
||
/// out-of-band updates, failover activation). Safe to call for unknown names.
|
||
/// The per-request revision check in ExecuteAsync remains the correctness
|
||
/// fallback when an invalidation is missed.
|
||
/// </summary>
|
||
public void InvalidateMethod(string methodName)
|
||
{
|
||
_scriptHandlers.TryRemove(methodName, out _);
|
||
_knownBadMethods.TryRemove(methodName, out _);
|
||
}
|
||
```
|
||
|
||
Keep `RemoveHandler` as a one-line delegate to `InvalidateMethod` (public API preserved), and change `HandleDeleteApiMethod` to call `InvalidateMethod(method.Name)`.
|
||
4. Run filter → **PASS**; run full `dotnet test tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests`.
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorStalenessTests.cs && git commit -m "feat(inbound-api): InvalidateMethod seam purging handler + known-bad — consumer entry point for the script-artifact invalidation contract (plan 05)"`
|
||
|
||
### Task 4: Abandoned inbound executions — defer DI-scope disposal + orphan telemetry
|
||
|
||
**Classification:** high-risk (concurrency; per-request lifecycle)
|
||
**Estimated implement time:** 5 min
|
||
**Parallelizable with:** 8-16, 17-22
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs` (`ExecuteAsync` body + finally, lines 333, 388-394) (after Task 3)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.Commons/Observability/ScadaBridgeTelemetry.cs` (add abandoned-execution counter, mirroring `RecordInboundApiRequest`)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs` (append)
|
||
|
||
On timeout, `WaitAsync` returns but the handler task keeps running; the `finally` disposes the DI scope underneath it → `ObjectDisposedException` background faults, and orphans accumulate invisibly (no rate limiting on this API). Fix: capture the handler task; in `finally`, dispose the scope only if the task completed — otherwise increment an orphan gauge, log, and attach a continuation that observes the fault, disposes the scope, and decrements the gauge.
|
||
|
||
**Steps:**
|
||
|
||
1. Failing test:
|
||
|
||
```csharp
|
||
[Fact]
|
||
public async Task TimedOutScript_ScopeDisposalDeferred_NoOrphanLeak()
|
||
{
|
||
// Handler ignores the token and keeps running past the method timeout.
|
||
var release = new TaskCompletionSource();
|
||
_executor.RegisterHandler("slow", async _ => { await release.Task; return 1; });
|
||
var method = new ApiMethod("slow", "unused") { Id = 9, TimeoutSeconds = 1 };
|
||
|
||
var result = await _executor.ExecuteAsync(
|
||
method, new Dictionary<string, object?>(), _route, TimeSpan.FromMilliseconds(50));
|
||
|
||
Assert.False(result.Success);
|
||
Assert.Contains("timed out", result.ErrorMessage);
|
||
Assert.Equal(1, _executor.AbandonedExecutionCount); // orphan is COUNTED
|
||
|
||
release.SetResult(); // orphan finishes; continuation cleans up
|
||
await Task.Delay(200);
|
||
Assert.Equal(0, _executor.AbandonedExecutionCount); // ...and DECREMENTED
|
||
}
|
||
```
|
||
|
||
2. `dotnet test tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests --filter TimedOutScript_ScopeDisposalDeferred_NoOrphanLeak` → **FAIL** (no `AbandonedExecutionCount`).
|
||
3. Implement in `ExecuteAsync`:
|
||
- Add `private long _abandonedExecutions;` and `internal long AbandonedExecutionCount => Interlocked.Read(ref _abandonedExecutions);` (public if the health surface needs it later).
|
||
- Hoist the handler task: `Task<object?>? handlerTask = null;` declared next to `scope`; replace line 333 with `handlerTask = cached.Handler(context); var result = await handlerTask.WaitAsync(cts.Token);`.
|
||
- Replace the `finally` block:
|
||
|
||
```csharp
|
||
finally
|
||
{
|
||
if (handlerTask is null || handlerTask.IsCompleted)
|
||
{
|
||
scope?.Dispose();
|
||
}
|
||
else
|
||
{
|
||
// The handler outlived the request (non-cooperative script after a
|
||
// timeout/client abort). Do NOT dispose the scope underneath it —
|
||
// defer cleanup to the task's own completion, and count the orphan
|
||
// so repeated timeouts of a slow method are visible.
|
||
Interlocked.Increment(ref _abandonedExecutions);
|
||
ScadaBridgeTelemetry.RecordInboundAbandonedExecution(method.Name);
|
||
_logger.LogWarning(
|
||
"Abandoned inbound execution for method {Method} is still running; DI scope disposal deferred", method.Name);
|
||
var abandonedScope = scope;
|
||
_ = handlerTask.ContinueWith(t =>
|
||
{
|
||
_ = t.Exception; // observe the fault so it never surfaces as unobserved
|
||
abandonedScope?.Dispose();
|
||
Interlocked.Decrement(ref _abandonedExecutions);
|
||
}, TaskScheduler.Default);
|
||
}
|
||
}
|
||
```
|
||
|
||
- In `ScadaBridgeTelemetry`, add `RecordInboundAbandonedExecution(string method)` incrementing a counter (`scadabridge.inbound_api.abandoned_executions`) with a bounded `method` tag, copying the existing `RecordInboundApiRequest` pattern.
|
||
4. Run filter → **PASS**; run full project.
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs src/ZB.MOM.WW.ScadaBridge.Commons/Observability/ScadaBridgeTelemetry.cs tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs && git commit -m "fix(inbound-api): defer DI-scope disposal to handler completion and count abandoned executions — closes the timeout use-after-dispose"`
|
||
|
||
### Task 5: Inbound error contract — machine-readable `code` field + SITE_UNREACHABLE + 415 for non-JSON bodies
|
||
|
||
**Classification:** standard
|
||
**Estimated implement time:** 5 min
|
||
**Parallelizable with:** 8-16, 17-22
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.InboundAPI/EndpointExtensions.cs` (all `Results.Json(new { error = … })` sites: lines 111-113, 153-155, 184-194, 212-214, 253-255)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs` (`InboundScriptResult` + catch blocks) (after Task 4)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs` (~line 172: typed exception)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundApiEndpointFilter.cs` (503/413 bodies) (after Task 6)
|
||
- Modify: `docs/requirements/Component-InboundAPI.md` (error-code table, ~lines 91-97)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointExtensionsTests.cs`, `tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointContentTypeTests.cs` (append)
|
||
|
||
Spec documents `{"error": …, "code": "SITE_UNREACHABLE"}`; code emits `{error}` only, and routed site-unreachable failures collapse into a generic 500. Implement the spec (the audience is external integrators). Codes: `UNAUTHORIZED` (401), `NOT_APPROVED` (403 — one shared code for both indistinguishable negatives, preserving enumeration-safety), `INVALID_JSON`, `VALIDATION_FAILED`, `UNSUPPORTED_MEDIA_TYPE` (new 415), `BODY_TOO_LARGE` (413), `STANDBY_NODE` (503), `TIMEOUT`, `SITE_UNREACHABLE`, `SCRIPT_ERROR` (500 catch-all).
|
||
|
||
**Steps:**
|
||
|
||
1. Failing tests (follow the existing patterns in `EndpointExtensionsTests`/`EndpointContentTypeTests`, which drive the endpoint via a test host):
|
||
- `ErrorBodies_CarryMachineReadableCode`: unauthorized request → body has `code == "UNAUTHORIZED"`; in-scope-but-invalid-JSON body → `code == "INVALID_JSON"`; missing required parameter → `code == "VALIDATION_FAILED"`.
|
||
- `NonJsonContentType_WithBody_Returns415`: `POST` with `Content-Type: text/plain` and body `"hello"` → HTTP 415, `code == "UNSUPPORTED_MEDIA_TYPE"`. A request with a body and **no** Content-Type header still parses as JSON (lenient, preserves existing callers).
|
||
- `RoutedSiteUnreachable_Returns500WithSiteUnreachableCode` (unit-level on the executor): stub `IInstanceRouter.RouteToCallAsync` to return `Success=false, ErrorMessage="Site unreachable"`; script does `await Route.To("X").Call("s")`; assert `result.ErrorCode == "SITE_UNREACHABLE"`.
|
||
2. `dotnet test tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests --filter "ErrorBodies_CarryMachineReadableCode|NonJsonContentType_WithBody_Returns415|RoutedSiteUnreachable"` → **FAIL**.
|
||
3. Implement:
|
||
- `RouteHelper.cs`: add `public sealed class SiteUnreachableException : InvalidOperationException` and throw it (instead of bare `InvalidOperationException`) when `RouteToCallAsync` reports failure whose message indicates unreachable/no-contact (and from the site-resolution failure path).
|
||
- `InboundScriptResult` gains `string? ErrorCode = null` (additive positional-with-default). Catch blocks: timeout → `"TIMEOUT"`; add `catch (SiteUnreachableException ex)` before the generic catch → `new InboundScriptResult(false, null, "Site unreachable", "SITE_UNREACHABLE")`; generic → `"SCRIPT_ERROR"`; compile failure → `"SCRIPT_COMPILE_FAILED"`.
|
||
- `EndpointExtensions.cs`: add a local helper `static IResult Error(string code, string message, int status) => Results.Json(new { error = message, code }, statusCode: status);` and route every error return through it. Content-type branch: if `ContentLength > 0` and `ContentType` is non-empty and does not contain `"json"` (ordinal-ignore-case) → `Error("UNSUPPORTED_MEDIA_TYPE", "Content-Type must be application/json", StatusCodes.Status415UnsupportedMediaType)`. Script-failure branch passes `scriptResult.ErrorCode ?? "SCRIPT_ERROR"` through.
|
||
- `InboundApiEndpointFilter.cs`: add `code = "STANDBY_NODE"` / `"BODY_TOO_LARGE"` to its two bodies.
|
||
- Update the doc's failure-body examples/table to the implemented code set.
|
||
4. Run filters → **PASS**; run the full InboundAPI test project (existing error-shape assertions that check only `error` still pass — the field is additive).
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.InboundAPI tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests docs/requirements/Component-InboundAPI.md && git commit -m "feat(inbound-api): machine-readable error codes per spec (incl. SITE_UNREACHABLE) + 415 for non-JSON bodies"`
|
||
|
||
### Task 6: `InboundApiEndpointFilter` hot-reads options via `IOptionsMonitor`
|
||
|
||
**Classification:** trivial
|
||
**Estimated implement time:** 2 min
|
||
**Parallelizable with:** 1, 2, 7, 8, 17, 18, 23, 24
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundApiEndpointFilter.cs` (lines 26-38, 66)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointGatingTests.cs` (append)
|
||
|
||
**Steps:**
|
||
|
||
1. Failing test: `BodySizeCap_HotReloads_WithoutRestart` — construct the filter with a test `IOptionsMonitor<InboundApiOptions>` whose `CurrentValue` is swapped between two invocations (cap 10 bytes → reject; cap 1 MiB → accept); assert the second request passes. (A minimal `TestOptionsMonitor<T>` stub: implements `CurrentValue` from a mutable field, `Get`→CurrentValue, `OnChange`→null.)
|
||
2. Run filter test → **FAIL** (ctor takes `IOptions`).
|
||
3. Change the ctor parameter to `IOptionsMonitor<InboundApiOptions> options`, store the monitor, and read `_options.CurrentValue.MaxRequestBodyBytes` per request (matches the sibling `AuditWriteMiddleware` convention).
|
||
4. Run → **PASS**; `dotnet test tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests`.
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundApiEndpointFilter.cs tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointGatingTests.cs && git commit -m "fix(inbound-api): endpoint filter hot-reads MaxRequestBodyBytes via IOptionsMonitor"`
|
||
|
||
### Task 7: Parallelize startup compile of inbound methods
|
||
|
||
**Classification:** trivial
|
||
**Estimated implement time:** 2 min
|
||
**Parallelizable with:** 1, 2, 6, 8, 17, 18, 23, 24
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.Host/Program.cs` (lines 383-387)
|
||
|
||
`CompileAndRegister` is safe for concurrent callers (ConcurrentDictionary caches, no shared mutable compile state). Replace the foreach with:
|
||
|
||
```csharp
|
||
Parallel.ForEach(methods, method => executor.CompileAndRegister(method));
|
||
```
|
||
|
||
**Steps:** No practical unit test (Host composition root; behavior identical, latency only). 1. Make the edit with a comment citing the review finding (serial Roslyn compiles stretch the failover-recovery window at hundreds of methods). 2. `dotnet build ZB.MOM.WW.ScadaBridge.slnx` → green. 3. `dotnet test tests/ZB.MOM.WW.ScadaBridge.Host.Tests` → green. 4. Commit: `git add src/ZB.MOM.WW.ScadaBridge.Host/Program.cs && git commit -m "perf(host): compile inbound API methods in parallel at startup"`
|
||
|
||
### Task 8: `ExternalSystemDefinition.TimeoutSeconds` — entity + EF mapping + migration
|
||
|
||
**Classification:** high-risk (schema migration)
|
||
**Estimated implement time:** 5 min
|
||
**Parallelizable with:** 1, 2, 6, 7, 17, 23, 24
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.Commons/Entities/ExternalSystems/ExternalSystemDefinition.cs`
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Configurations/ExternalSystemConfiguration.cs`
|
||
- Create: `src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Migrations/<timestamp>_AddExternalSystemTimeoutSeconds.cs` (+ Designer, + snapshot update — generated)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.Commons.Tests` (entity default), `tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests` (mapping round-trip, follow the existing external-system repository test pattern)
|
||
|
||
**Steps:**
|
||
|
||
1. Failing test (Commons.Tests): `ExternalSystemDefinition_TimeoutSeconds_DefaultsToZeroMeaningUseGatewayDefault` — construct the entity, assert `TimeoutSeconds == 0`. Run `dotnet test tests/ZB.MOM.WW.ScadaBridge.Commons.Tests --filter ExternalSystemDefinition_TimeoutSeconds` → **FAIL** (no property).
|
||
2. Add to the entity:
|
||
|
||
```csharp
|
||
/// <summary>
|
||
/// Per-system HTTP round-trip timeout in seconds for all method calls
|
||
/// (spec: Component-ExternalSystemGateway "Timeout"). 0 = unset — the gateway's
|
||
/// configured DefaultHttpTimeout applies.
|
||
/// </summary>
|
||
public int TimeoutSeconds { get; set; }
|
||
```
|
||
|
||
3. EF mapping: in `ExternalSystemConfiguration`, add `builder.Property(e => e.TimeoutSeconds).HasDefaultValue(0);`.
|
||
4. Generate the migration — **build first**: `dotnet build ZB.MOM.WW.ScadaBridge.slnx` then `dotnet ef migrations add AddExternalSystemTimeoutSeconds --project src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase --startup-project src/ZB.MOM.WW.ScadaBridge.Host`. Open the generated file and verify it is NOT empty (one `AddColumn<int>` on the external-systems table, default 0); if empty, delete the generated files, rebuild, regenerate.
|
||
5. Run tests: the Commons filter → **PASS**; `dotnet test tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests` → green.
|
||
6. Commit: `git add src/ZB.MOM.WW.ScadaBridge.Commons/Entities/ExternalSystems/ExternalSystemDefinition.cs src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase && git add tests/ZB.MOM.WW.ScadaBridge.Commons.Tests && git commit -m "feat(esg): per-system TimeoutSeconds column on ExternalSystemDefinition (spec Component-ExternalSystemGateway Timeout) — entity + EF + migration"`
|
||
|
||
### Task 9: Honor the per-system timeout in `ExternalSystemClient.InvokeHttpAsync`
|
||
|
||
**Classification:** standard
|
||
**Estimated implement time:** 4 min
|
||
**Parallelizable with:** 10, 11, 12 (Files disjoint)
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway/ExternalSystemClient.cs` (lines 300-307, 322-323, 345-346)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs` (append)
|
||
|
||
**Steps:**
|
||
|
||
1. Failing test (use the project's existing stub-handler `HttpClient` pattern):
|
||
|
||
```csharp
|
||
[Fact]
|
||
public async Task PerSystemTimeout_OverridesGatewayDefault()
|
||
{
|
||
// System timeout 1s; handler black-holes for 5s; gateway default is 30s.
|
||
var system = new ExternalSystemDefinition("slow", "http://x", "none") { TimeoutSeconds = 1 };
|
||
var method = new ExternalSystemMethod("m", "GET", "/p");
|
||
SetupRepository(system, method); // existing helper pattern
|
||
var handler = new DelegateHandler(async (_, ct) =>
|
||
{
|
||
await Task.Delay(TimeSpan.FromSeconds(5), ct);
|
||
return new HttpResponseMessage(HttpStatusCode.OK);
|
||
});
|
||
_httpClientFactory.CreateClient(Arg.Any<string>()).Returns(new HttpClient(handler));
|
||
var client = new ExternalSystemClient(_httpClientFactory, _repository, NullLogger<ExternalSystemClient>.Instance);
|
||
|
||
var sw = System.Diagnostics.Stopwatch.StartNew();
|
||
var result = await client.CallAsync("slow", "m");
|
||
sw.Stop();
|
||
|
||
Assert.False(result.Success);
|
||
Assert.Contains("Timeout", result.ErrorMessage);
|
||
Assert.True(sw.Elapsed < TimeSpan.FromSeconds(4), "per-system 1s timeout must apply, not the 30s default");
|
||
}
|
||
```
|
||
|
||
2. `dotnet test tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests --filter PerSystemTimeout_OverridesGatewayDefault` → **FAIL** (~30s default would apply; the test waits 5s then sees OK → success).
|
||
3. Implement: before the CTS creation, `var effectiveTimeout = system.TimeoutSeconds > 0 ? TimeSpan.FromSeconds(system.TimeoutSeconds) : _options.DefaultHttpTimeout;` — use `effectiveTimeout` in `new CancellationTokenSource(effectiveTimeout)` and in both timeout error messages (replacing `_options.DefaultHttpTimeout.TotalSeconds`). Update the stale comment block at lines 300-305 ("has no per-system Timeout field yet") to describe the new resolution order.
|
||
4. Run → **PASS**; full project run.
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway/ExternalSystemClient.cs tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs && git commit -m "feat(esg): honor per-system TimeoutSeconds in InvokeHttpAsync (falls back to DefaultHttpTimeout)"`
|
||
|
||
### Task 10: Carry TimeoutSeconds through the artifact pipeline to sites
|
||
|
||
**Classification:** high-risk (message contract + site SQLite schema)
|
||
**Estimated implement time:** 5 min
|
||
**Parallelizable with:** 9, 11, 12
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Artifacts/ExternalSystemArtifact.cs` (additive trailing param `int TimeoutSeconds = 0`)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactDeploymentService.cs` (~line 194: pass `es.TimeoutSeconds`)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Persistence/SiteStorageService.cs` (`MigrateSchemaAsync`: `TryAddColumnAsync(connection, "external_systems", "timeout_seconds", "INTEGER NOT NULL DEFAULT 0")`; `StoreExternalSystemAsync`: new `int timeoutSeconds` param + column in the upsert)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs` (~line 1533: pass `es.TimeoutSeconds`)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Repositories/SiteExternalSystemRepository.cs` (`MapExternalSystem` + its SELECTs: read `timeout_seconds` into `TimeoutSeconds`)
|
||
- Check/Modify: `src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/SiteReplicationActor.cs` (it re-applies `ReplicateArtifacts` — verify its `StoreExternalSystemAsync` call site compiles with the new parameter)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests` (site storage round-trip test — follow the existing `StoreExternalSystemAsync` test if present, else add `SiteStorage_ExternalSystem_TimeoutSeconds_RoundTrips`)
|
||
|
||
The message contract change is **additive** (trailing positional with default), per the repo's additive-only evolution rule — an old central sending to a new site yields 0 (= use default), and vice versa deserializes cleanly.
|
||
|
||
**Steps:**
|
||
|
||
1. Failing test (SiteRuntime.Tests, in-memory/temp-file SQLite): store an external system with `timeoutSeconds: 7` via `SiteStorageService.StoreExternalSystemAsync`, read it back via `SiteExternalSystemRepository.GetExternalSystemByNameAsync`, assert `TimeoutSeconds == 7`. Run `dotnet test tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests --filter TimeoutSeconds_RoundTrips` → **FAIL** (compile error / missing column).
|
||
2. Make the six edits above: add the artifact record's trailing `int TimeoutSeconds = 0`; thread it central-build → site-store → site-read. The SQLite upsert gains `timeout_seconds = excluded.timeout_seconds`.
|
||
3. Run → **PASS**; then `dotnet test tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests` and `dotnet test tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests` (artifact-build assertions may enumerate record fields — update).
|
||
4. `dotnet build ZB.MOM.WW.ScadaBridge.slnx` → green (catches any other `ExternalSystemArtifact` construction site).
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Artifacts/ExternalSystemArtifact.cs src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactDeploymentService.cs src/ZB.MOM.WW.ScadaBridge.SiteRuntime tests/ && git commit -m "feat(esg): TimeoutSeconds travels the artifact pipeline (additive contract) into site SQLite so site-side calls honor it"`
|
||
|
||
### Task 11: TimeoutSeconds management commands + CLI
|
||
|
||
**Classification:** small
|
||
**Estimated implement time:** 4 min
|
||
**Parallelizable with:** 9, 10, 12
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/ExternalSystemCommands.cs` (Create/Update records: trailing `int? TimeoutSeconds = null`)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs` (`HandleCreateExternalSystem` ~1599, `HandleUpdateExternalSystem` ~1612: apply `cmd.TimeoutSeconds ?? 0` / preserve-on-null semantics for update)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.CLI/Commands/ExternalSystemCommands.cs` (create/update: `--timeout-seconds` `Option<int?>`)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests` (append to the existing external-system handler tests); `tests/ZB.MOM.WW.ScadaBridge.CLI.Tests` (option parse test — **note: CLI.Tests is not in the slnx; test the csproj directly**)
|
||
|
||
**Steps:**
|
||
|
||
1. Failing test (ManagementService.Tests): `CreateExternalSystem_PersistsTimeoutSeconds` — send `CreateExternalSystemCommand(..., TimeoutSeconds: 15)`, assert the persisted definition has `TimeoutSeconds == 15`; and `UpdateExternalSystem_NullTimeout_PreservesExisting`. Run `dotnet test tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests --filter TimeoutSeconds` → **FAIL**.
|
||
2. Implement: additive record params; create handler sets `TimeoutSeconds = cmd.TimeoutSeconds ?? 0`; update handler `if (cmd.TimeoutSeconds is { } ts) def.TimeoutSeconds = ts;`. CLI: add the option to both `create` and `update`, pass through, add `--timeout-seconds` to the table projection if the list output enumerates fields.
|
||
3. Run → **PASS**; run `dotnet test tests/ZB.MOM.WW.ScadaBridge.CLI.Tests` **directly** (not via slnx).
|
||
4. Commit: `git add src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/ExternalSystemCommands.cs src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs src/ZB.MOM.WW.ScadaBridge.CLI tests/ && git commit -m "feat(esg): --timeout-seconds on external-system create/update (management command + CLI, additive contract)"`
|
||
|
||
### Task 12: TimeoutSeconds in Central UI form + spec doc sync
|
||
|
||
**Classification:** small
|
||
**Estimated implement time:** 3 min
|
||
**Parallelizable with:** 9, 10, 11
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/ExternalSystemForm.razor` (numeric input next to the existing `_maxRetries`/`_retryDelaySeconds` fields; load in `OnInitializedAsync`, save in both create/update branches)
|
||
- Modify: `docs/requirements/Component-ExternalSystemGateway.md` (line 38 area + §"Call Timeout & Error Handling" ~line 99: document per-system `TimeoutSeconds` with 0 = gateway `DefaultHttpTimeout` fallback and that it now ships to sites in the artifact)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests` (bUnit — append to the existing ExternalSystemForm test if one exists; otherwise assert the rendered form contains the `Timeout (seconds)` field)
|
||
|
||
**Steps:**
|
||
|
||
1. Failing bUnit test: render `ExternalSystemForm` (existing test scaffolding), assert a labeled `Timeout (seconds)` input exists and that saving with value 20 passes `TimeoutSeconds == 20` to the repository/command. Run → **FAIL**.
|
||
2. Implement: `private int _timeoutSeconds;` bound field with helper text "0 = use gateway default"; wire load + both save paths (the form writes the entity directly per the existing pattern). Update the doc.
|
||
3. Run → **PASS**; `dotnet test tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests`.
|
||
4. Commit: `git add src/ZB.MOM.WW.ScadaBridge.CentralUI docs/requirements/Component-ExternalSystemGateway.md tests/ && git commit -m "feat(esg): TimeoutSeconds in ExternalSystem UI form + Component-ExternalSystemGateway doc sync — closes the spec-drift gap"`
|
||
|
||
### Task 13: Path-template substitution (`/recipes/{id}`) in `BuildUrl`
|
||
|
||
**Classification:** standard
|
||
**Estimated implement time:** 5 min
|
||
**Parallelizable with:** 10, 11, 12 (NOT 9/14/15/16 — same file; run after 9)
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway/ExternalSystemClient.cs` (`BuildUrl` lines 431-463, `InvokeHttpAsync` body-serialization branch lines 286-298)
|
||
- Modify: `docs/requirements/Component-ExternalSystemGateway.md` (path/method-definition section: document `{param}` substitution + consumed-parameter removal + missing-parameter error)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs` (append)
|
||
|
||
Design: `BuildUrl` becomes `BuildUrl(baseUrl, path, parameters, httpMethod, out ISet<string> consumedParams)` (or returns a tuple). Regex `\{([A-Za-z_][A-Za-z0-9_]*)\}` over the trimmed path: each placeholder is replaced with `Uri.EscapeDataString(value?.ToString())` from `parameters` and its name recorded as consumed; a placeholder with no matching parameter (or a null value) throws `ArgumentException("Path template parameter '{id}' has no value …")` — a clear authoring error, mirroring `ValidateHttpMethod`. Consumed names are excluded from the GET/DELETE query string AND from the POST/PUT/PATCH JSON body.
|
||
|
||
**Steps:**
|
||
|
||
1. Failing tests:
|
||
- `PathTemplate_SubstitutesAndConsumesParameter`: method path `/recipes/{id}`, GET, parameters `{ id = "R 1", filter = "x" }` → captured request URI is `.../recipes/R%201?filter=x` (no `id` in the query).
|
||
- `PathTemplate_Post_ConsumedParamExcludedFromBody`: POST `/orders/{orderId}/lines`, parameters `{ orderId = 5, qty = 2 }` → body JSON contains `qty` but not `orderId`.
|
||
- `PathTemplate_MissingParameter_ThrowsClearError`: GET `/recipes/{id}` with no `id` → `ArgumentException` naming `id`.
|
||
2. `dotnet test tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests --filter PathTemplate` → **FAIL** (URL contains literal `{id}`).
|
||
3. Implement as designed (a `private static readonly Regex PathParamRegex` + substitution loop; thread `consumedParams` into the query-string LINQ filter and into a `parameters.Where(p => !consumed.Contains(p.Key))` projection before body serialization).
|
||
4. Run → **PASS**; full project run.
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway/ExternalSystemClient.cs docs/requirements/Component-ExternalSystemGateway.md tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs && git commit -m "feat(esg): {param} path-template substitution with consumed-param removal — implements the spec'd /recipes/{id} form"`
|
||
|
||
### Task 14: Bound outbound HTTP response buffering
|
||
|
||
**Classification:** standard
|
||
**Estimated implement time:** 5 min
|
||
**Parallelizable with:** 10, 11, 12 (same-file chain: after 13)
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway/ExternalSystemClient.cs` (line 312 `SendAsync`, lines 334-347 body read)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway/ExternalSystemGatewayOptions.cs` (add `MaxResponseBodyBytes`, default `10 * 1024 * 1024`)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs` (append)
|
||
|
||
Design: `SendAsync(request, HttpCompletionOption.ResponseHeadersRead, linkedCts.Token)`; pre-check `Content-Length` header when present (fail fast); then a bounded read (cap+1 pattern): stream into a `MemoryStream` in 16 KiB chunks, aborting once `MaxResponseBodyBytes` is exceeded. Oversize → `PermanentExternalSystemException` ("response exceeded N bytes" — retrying won't shrink it; buffering it into S&F would defeat the cap).
|
||
|
||
**Steps:**
|
||
|
||
1. Failing test: `OversizedResponseBody_FailsPermanently_WithoutBufferingWholeBody` — options with `MaxResponseBodyBytes = 1024`; stub handler returns 2 MiB of `'x'`; `CallAsync` → `result.Success == false`, error contains `"exceeded"`; and a same-shape `CachedCallAsync` does NOT enqueue to S&F (assert via the existing S&F test double if the suite has one, else assert `WasBuffered == false`).
|
||
2. `dotnet test tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests --filter OversizedResponseBody` → **FAIL** (full body read succeeds).
|
||
3. Implement: add the option; replace `ReadAsStringAsync` with a private `ReadBodyBoundedAsync(HttpContent content, long maxBytes, CancellationToken token)` returning `string` or throwing `PermanentExternalSystemException`; keep the existing ordered OCE catch filters around the read.
|
||
4. Run → **PASS**; full project run.
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs && git commit -m "perf(esg): bound outbound response bodies (ResponseHeadersRead + MaxResponseBodyBytes cap; oversize = permanent failure)"`
|
||
|
||
### Task 15: Structured JSON `AuthConfiguration` with legacy colon fallback
|
||
|
||
**Classification:** high-risk (auth parsing; existing rows must keep working)
|
||
**Estimated implement time:** 5 min
|
||
**Parallelizable with:** 10, 11, 12 (same-file chain: after 14)
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway/ExternalSystemClient.cs` (`ApplyAuth`, lines 465-533)
|
||
- Modify: `docs/requirements/Component-ExternalSystemGateway.md` (auth-configuration format section)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs` (append)
|
||
|
||
The entity doc-comment ("JSON-serialized authentication configuration") and the UI placeholders (`{"key":"xyz"}` / `{"username":"u","password":"p"}`) already promise JSON — but `ApplyAuth` colon-splits, so a JSON-shaped config is silently misparsed **today** (a `{"key":"xyz"}` apikey config produces header name `{"key"`), and any API key containing `:` breaks. Parse order per branch: trimmed value starting with `{` → JSON (`apikey`: `{"header"?: string, "key": string}`, default header `X-API-Key`; `basic`: `{"username": string, "password": string}`); otherwise the legacy colon-split/bare-key behavior, unchanged. Malformed JSON → the existing "malformed config" warning path (send without header), never an exception.
|
||
|
||
**Steps:**
|
||
|
||
1. Failing tests:
|
||
- `ApiKeyAuth_JsonConfig_SetsConfiguredHeaderAndKeepsColons`: `AuthConfiguration = "{\"header\":\"X-Auth\",\"key\":\"ab:cd:ef\"}"` → captured request has header `X-Auth: ab:cd:ef`.
|
||
- `ApiKeyAuth_JsonConfig_DefaultHeader`: `{"key":"xyz"}` → `X-API-Key: xyz` (this is the UI-placeholder shape that is broken today).
|
||
- `BasicAuth_JsonConfig_EncodesUsernamePassword`: `{"username":"u","password":"p:w"}` → `Authorization: Basic` of `u:p:w`.
|
||
- `ApiKeyAuth_LegacyColonConfig_StillWorks`: `"X-Custom:secret"` → `X-Custom: secret` (backward compatibility pinned).
|
||
2. `dotnet test tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests --filter "JsonConfig|LegacyColonConfig"` → **FAIL**.
|
||
3. Implement a private `static bool TryParseJsonAuth(string config, out Dictionary<string, string?> fields)` (System.Text.Json, case-insensitive property names, catch `JsonException` → false) and branch `apikey`/`basic` on it before the legacy split. Log a warning (values never logged) when JSON parsing yields no usable `key`/`username`. Update the doc section with both accepted formats and the deprecation note on the colon form.
|
||
4. Run → **PASS**; full project run.
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway/ExternalSystemClient.cs docs/requirements/Component-ExternalSystemGateway.md tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs && git commit -m "fix(esg): structured JSON AuthConfiguration (matches entity doc + UI placeholders) with legacy colon fallback — keys containing ':' no longer misparse"`
|
||
|
||
### Task 16: Cancellation-aware `ErrorClassifier.IsTransient` overload
|
||
|
||
**Classification:** small
|
||
**Estimated implement time:** 3 min
|
||
**Parallelizable with:** 10, 11, 12, 17-24 (same-file chain: after 15)
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway/ErrorClassifier.cs` (lines 27-38)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway/ExternalSystemClient.cs` (line 325 call site → pass the caller token)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests/ErrorClassifierTests.cs` (append)
|
||
|
||
**Steps:**
|
||
|
||
1. Failing test: `IsTransient_CallerCancelledOce_IsNotTransient` — `var cts = new CancellationTokenSource(); cts.Cancel(); Assert.False(ErrorClassifier.IsTransient(new OperationCanceledException(), cts.Token)); Assert.True(ErrorClassifier.IsTransient(new OperationCanceledException(), CancellationToken.None));` → **FAIL** (no overload).
|
||
2. Implement (mirrors `SmtpErrorClassifier`/`SmsErrorClassifier`):
|
||
|
||
```csharp
|
||
/// <summary>
|
||
/// Cancellation-aware overload: a caller-requested cancellation is NOT transient
|
||
/// (buffering caller-abandoned work into S&F would be wrong). Prefer this overload
|
||
/// at call sites that hold the caller's token.
|
||
/// </summary>
|
||
public static bool IsTransient(Exception exception, CancellationToken cancellationToken)
|
||
{
|
||
if (exception is OperationCanceledException && cancellationToken.IsCancellationRequested)
|
||
return false;
|
||
return IsTransient(exception);
|
||
}
|
||
```
|
||
|
||
Switch the `catch (Exception ex) when (ErrorClassifier.IsTransient(ex))` filter in `InvokeHttpAsync` to `ErrorClassifier.IsTransient(ex, cancellationToken)` (behavior identical — the ordered filters already peel caller-cancel — but the predicate is now misuse-resistant); add a doc-comment on the old overload pointing to the new one.
|
||
3. Run `dotnet test tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests --filter IsTransient_CallerCancelledOce` → **PASS**; full project run.
|
||
4. Commit: `git add src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests/ErrorClassifierTests.cs && git commit -m "fix(esg): cancellation-aware ErrorClassifier.IsTransient overload — caller-cancelled work can never classify transient"`
|
||
|
||
### Task 17: SMS first-transient short-circuit (duplicate amplification + sweep bound)
|
||
|
||
**Classification:** standard
|
||
**Estimated implement time:** 4 min
|
||
**Parallelizable with:** 1-16, 19, 22, 23, 24
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/Delivery/SmsNotificationDeliveryAdapter.cs` (recipient loop, lines 148-172; `RollUp` doc-comment)
|
||
- Modify: `docs/requirements/Component-NotificationService.md` (the re-send-to-all note ~line 102: add the short-circuit)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/Delivery/` (append to the existing SMS adapter test file)
|
||
|
||
Any transient rolls the whole notification up to retry-all, so every recipient accepted *after* the first transient is a guaranteed duplicate text on retry. Breaking the loop on the first transient costs nothing (retry re-attempts everyone) and — because a black-holed endpoint's first request eats the full per-request timeout — also bounds the worst-case sweep occupancy to ~one timeout instead of recipients × timeout, which is the substance of the P2 performance finding at current scale. Bounded parallelism is deferred (see coverage table).
|
||
|
||
**Steps:**
|
||
|
||
1. Failing test: `MidListTransient_ShortCircuits_RemainingRecipientsNotAttempted` — list of 3 numbers; stub handler: first number → 500 (transient), and a request counter. Assert `DeliveryOutcome.Result == Transient` and **exactly 1** HTTP request was made (current code makes 3).
|
||
2. `dotnet test tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests --filter MidListTransient_ShortCircuits` → **FAIL** (3 requests).
|
||
3. Implement: in the `switch`, the `Transient` case sets `firstTransientError ??= attempt.Detail;` then `break`s out of the `foreach` (convert to `goto` -free form: set a flag and `break` the loop after the switch, or restructure to `if/else if`). Add the rationale comment: *"every recipient accepted after the first transient is a guaranteed duplicate on retry (the whole notification re-sends); stopping here also bounds a black-holed endpoint to one timeout per sweep."* Update `RollUp`'s doc-comment and the component doc line.
|
||
4. Run → **PASS**; full `dotnet test tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests`.
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/Delivery/SmsNotificationDeliveryAdapter.cs docs/requirements/Component-NotificationService.md tests/ && git commit -m "fix(notifications): SMS adapter short-circuits on first transient — kills duplicate amplification and bounds the dispatch sweep"`
|
||
|
||
### Task 18: Honor `SmsConfiguration.MaxRetries`/`RetryDelay` — per-type retry policy
|
||
|
||
**Classification:** high-risk (behavioral change to dispatch/parking)
|
||
**Estimated implement time:** 5 min
|
||
**Parallelizable with:** 1-16, 23, 24 (NOT 17/21 — shared test dirs are fine, but 21 edits the same actor file: run 18 before 21)
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/NotificationOutboxActor.cs` (`RunDispatchSweepAsync` ~line 319, `DeliverOneAsync` signature, `ResolveRetryPolicyAsync` lines 370-401)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.Commons/Entities/Notifications/SmsConfiguration.cs` (lines 28-45: delete the RESERVED doc-comments, document the live behavior)
|
||
- Modify: `docs/requirements/Component-NotificationService.md` (~line 65 deferred note), `CLAUDE.md` (the "Notification Outbox retry reuses central SMTP max-retry-count and fixed interval" decision line → "…for Email; Sms uses SmsConfiguration.MaxRetries/RetryDelay, falling back to the SMTP policy when unset")
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/NotificationOutboxActorDispatchTests.cs` (append)
|
||
|
||
Design: `ResolveRetryPolicyAsync` becomes `ResolveRetryPoliciesAsync` returning a small `RetryPolicies(SmtpPolicy, SmsPolicy)` record, resolved once per sweep: the SMTP policy exactly as today; the SMS policy from `INotificationRepository.GetSmsConfigurationAsync()` with the same non-positive-clamp logic, falling back to the SMTP policy when no SMS config row exists. `DeliverOneAsync` selects by `notification.Type` (`Sms` → SMS policy, everything else → SMTP policy). This is plan 08's flagged dead-field finding, owned here.
|
||
|
||
**Steps:**
|
||
|
||
1. Failing test (follow the existing dispatch-test scaffolding, which drives the actor with substitute repositories): `SmsNotification_TransientFailure_UsesSmsRetryPolicy` — SMTP config `MaxRetries = 10`; SMS config `MaxRetries = 1, RetryDelay = 5 min`; an SMS notification with `RetryCount = 1` and a transient adapter outcome → row is **Parked** (SMS policy exhausted) not Retrying; a parallel `EmailNotification_StillUsesSmtpPolicy` asserts an email row with `RetryCount = 1` goes to Retrying with the SMTP delay.
|
||
2. `dotnet test tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests --filter "UsesSmsRetryPolicy|StillUsesSmtpPolicy"` → **FAIL** (SMS row retries under SMTP policy).
|
||
3. Implement as designed; keep the clamp warnings (parameterized by which config produced them). Update the entity doc-comments, component doc, and the CLAUDE.md decision line in the same commit (docs travel with code).
|
||
4. Run → **PASS**; full NotificationOutbox.Tests run.
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/NotificationOutboxActor.cs src/ZB.MOM.WW.ScadaBridge.Commons/Entities/Notifications/SmsConfiguration.cs docs/requirements/Component-NotificationService.md CLAUDE.md tests/ && git commit -m "feat(notifications): SMS notifications retry under SmsConfiguration.MaxRetries/RetryDelay (SMTP policy remains the Email + fallback policy) — wires the dead fields"`
|
||
|
||
### Task 19: Configurable OAuth2 authority/scope — entity + EF + migration + token service
|
||
|
||
**Classification:** high-risk (schema migration + auth flow)
|
||
**Estimated implement time:** 5 min
|
||
**Parallelizable with:** 1-7, 17, 23, 24 (**migration serialization: run after Task 8** — both regenerate the EF model snapshot)
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.Commons/Entities/Notifications/SmtpConfiguration.cs` (add `string? OAuth2Authority`, `string? OAuth2Scope`)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Configurations/NotificationConfiguration.cs` (map both nullable columns)
|
||
- Create: migration `AddSmtpOAuth2AuthorityScope` (same command pattern as Task 8; **build first**, verify non-empty)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.NotificationService/OAuth2TokenService.cs` (`GetTokenAsync` gains optional `string? authority = null, string? scope = null`; defaults preserve today's M365 URL/scope; cache key incorporates authority+scope so two configs with the same credentials but different authorities never share a token)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/Delivery/EmailNotificationDeliveryAdapter.cs` (line 188: pass `smtpConfig.OAuth2Authority`, `smtpConfig.OAuth2Scope`)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.NotificationService.Tests` (append to the OAuth2 token service tests)
|
||
|
||
Authority template: the stored value is the full token-endpoint URL with an optional `{tenant}` placeholder (e.g. `https://login.microsoftonline.com/{tenant}/oauth2/v2.0/token`); null → today's hardcoded M365 endpoint. Scope: null → `https://outlook.office365.com/.default`.
|
||
|
||
**Steps:**
|
||
|
||
1. Failing tests: `GetTokenAsync_CustomAuthorityAndScope_PostsToConfiguredEndpoint` — stub HTTP handler captures the request; call with `authority: "https://idp.example/{tenant}/token", scope: "smtp.send"`; assert the POST went to `https://idp.example/T1/token` with form `scope=smtp.send`. And `GetTokenAsync_DefaultsPreserved_M365` (no authority/scope → today's URL + scope). Run filter → **FAIL** (no parameters).
|
||
2. Implement entity + EF + migration (verify non-empty), then the token-service parameters and the adapter call site; cache key: `CredentialKey($"{credentials}|{authority}|{scope}")`.
|
||
3. Run → **PASS**; `dotnet test tests/ZB.MOM.WW.ScadaBridge.NotificationService.Tests` and `tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests`.
|
||
4. Commit: `git add src/ZB.MOM.WW.ScadaBridge.Commons/Entities/Notifications/SmtpConfiguration.cs src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase src/ZB.MOM.WW.ScadaBridge.NotificationService/OAuth2TokenService.cs src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/Delivery/EmailNotificationDeliveryAdapter.cs tests/ && git commit -m "feat(notifications): configurable OAuth2 authority + scope per SMTP configuration (defaults preserve M365) — entity + EF + migration + token service"`
|
||
|
||
### Task 20: OAuth2 authority/scope surfaces — management command + CLI + UI + doc
|
||
|
||
**Classification:** small
|
||
**Estimated implement time:** 4 min
|
||
**Parallelizable with:** 9-16, 21, 22
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.Commons/Messages/Management/NotificationCommands.cs` (`UpdateSmtpConfigCommand`: trailing `string? OAuth2Authority = null, string? OAuth2Scope = null`)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs` (the SMTP update handler: apply when non-null)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.CLI/Commands/NotificationCommands.cs` (`smtp update`: `--oauth2-authority`, `--oauth2-scope` options, ~line 127+ group)
|
||
- Modify: the Central UI SMTP page (`src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Notifications/` — the SMTP form component; two text inputs shown only when AuthType is OAuth2)
|
||
- Modify: `docs/requirements/Component-NotificationService.md` (~line 48: the "and other modern SMTP providers" sentence now true — document the two fields and their M365 defaults)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests` (handler applies fields), `tests/ZB.MOM.WW.ScadaBridge.CLI.Tests` (option parse — test csproj directly)
|
||
|
||
**Steps:** 1. Failing handler test `UpdateSmtpConfig_AppliesOAuth2AuthorityAndScope` → run filter → **FAIL**. 2. Implement the additive command params, handler application (preserve-on-null), CLI options, UI inputs, doc paragraph. 3. Run filters → **PASS**; run ManagementService.Tests + CLI.Tests (direct). 4. Commit: `git add -A src/ZB.MOM.WW.ScadaBridge.Commons/Messages src/ZB.MOM.WW.ScadaBridge.ManagementService src/ZB.MOM.WW.ScadaBridge.CLI src/ZB.MOM.WW.ScadaBridge.CentralUI docs/requirements/Component-NotificationService.md tests/ && git commit -m "feat(notifications): OAuth2 authority/scope management + CLI + UI surfaces"`
|
||
|
||
### Task 21: Notification low-severity cleanups — deterministic SMTP selection + OAuth2 cache visibility
|
||
|
||
**Classification:** small
|
||
**Estimated implement time:** 4 min
|
||
**Parallelizable with:** 9-16, 20, 22 (after 18 and 19 — shared files)
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/Delivery/EmailNotificationDeliveryAdapter.cs` (lines 75-76)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/NotificationOutboxActor.cs` (retry-policy resolution — post-Task-18 shape)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.NotificationService/OAuth2TokenService.cs` (`CacheEntry`)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/Delivery/` (append)
|
||
|
||
Concrete edits (S7, S8):
|
||
1. **Deterministic SMTP pick**: in both call sites replace `FirstOrDefault()` / `configurations[0]` with `configurations.OrderBy(c => c.Id).FirstOrDefault()` and, when `configurations.Count > 1`, log a warning: `"Multiple SMTP configurations exist; using the lowest-Id row ({Id}). Enforce a single row or delete extras."` Failing test first: `MultipleSmtpConfigs_LowestIdWins` — repository returns rows Id 7 then Id 3 (that order); assert the adapter connects with row 3's server. Run → **FAIL** (row 7 picked) → implement → **PASS**.
|
||
2. **OAuth2 cache visibility**: replace `CacheEntry`'s mutable `Token`/`Expiry` fields with a single `volatile TokenValue? Current;` where `private sealed record TokenValue(string Token, DateTimeOffset Expiry);` — reads outside the lock see a consistent pair; writes inside the lock assign one reference. (Growth stays bounded by distinct credential triples — accepted per report; no eviction added.) Existing OAuth2 tests must stay green.
|
||
|
||
Run `dotnet test tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests` and `tests/ZB.MOM.WW.ScadaBridge.NotificationService.Tests`. Commit: `git add -A src/ZB.MOM.WW.ScadaBridge.NotificationOutbox src/ZB.MOM.WW.ScadaBridge.NotificationService tests/ && git commit -m "fix(notifications): deterministic lowest-Id SMTP config selection (+ multi-row warning); atomic OAuth2 token cache entry"`
|
||
|
||
### Task 22: Validate Twilio `AccountSid` format at config save
|
||
|
||
**Classification:** small
|
||
**Estimated implement time:** 3 min
|
||
**Parallelizable with:** 9, 10, 12, 13-17, 20, 21, 23, 24 (after 11 — same `ManagementActor.cs`)
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs` (the `UpdateSmsConfig` handler: reject a non-matching SID)
|
||
- Modify: the Central UI SMS form (`/notifications/sms` page component: same client-side validation message)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests` (append)
|
||
|
||
`AccountSid` is interpolated un-escaped into the Twilio request URI (`SmsNotificationDeliveryAdapter.cs:131`); a validation regex at save time closes the door on URI-injection via a malformed SID.
|
||
|
||
**Steps:**
|
||
|
||
1. Failing test: `UpdateSmsConfig_MalformedAccountSid_Rejected` — send the SMS-config update command with `AccountSid = "../evil?x="`; assert `ManagementCommandException` containing `"Account SID"`; and `UpdateSmsConfig_ValidSid_Accepted` with `"AC" + new string('a', 32)`.
|
||
2. Run filter → **FAIL**.
|
||
3. Implement in the handler, before persisting: `if (!System.Text.RegularExpressions.Regex.IsMatch(cmd.AccountSid, "^AC[0-9a-fA-F]{32}$")) throw new ManagementCommandException("Account SID must match Twilio's format: 'AC' followed by 32 hex characters.");` Mirror the message in the UI form's validation.
|
||
4. Run → **PASS**; run ManagementService.Tests.
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs src/ZB.MOM.WW.ScadaBridge.CentralUI tests/ && git commit -m "fix(notifications): validate Twilio AccountSid format at save — closes the un-escaped URI interpolation door"`
|
||
|
||
### Task 23: Document DelmiaNotifier at-least-once semantics + idempotency requirement
|
||
|
||
**Classification:** small (documentation)
|
||
**Estimated implement time:** 4 min
|
||
**Parallelizable with:** 1-22
|
||
**Files:**
|
||
- Modify: `docs/plans/2026-06-26-delmia-recipe-notifier-design.md` (after the failover table, ~line 100)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.DelmiaNotifier/README.md` (operator-facing note)
|
||
- Modify: `docs/requirements/Component-InboundAPI.md` (authoring note for the `DelmiaRecipeDownload` method)
|
||
|
||
The behavior is by design (timeout → failover), so no code change — but the consequence (at-least-once: a connection reset *after* the server processed the POST, or a slow response exceeding `TimeoutSeconds`, both roll over to the next URL and re-POST) is nowhere stated, and the deployed inbound method carries the burden.
|
||
|
||
**Steps:**
|
||
|
||
1. Add a "Delivery semantics: at-least-once" section to the design doc: enumerate the two duplicate-producing cases; state the requirement — *the `DelmiaRecipeDownload` inbound method MUST be idempotent on `(MachineCode, DownloadPath, WorkOrderNumber)`; a duplicate must return the same `Result`/`ResultText` as the original, not double-apply side effects.* Cross-link from the failover table row for timeout.
|
||
2. Mirror one paragraph in the DelmiaNotifier README (`## Delivery semantics`) and add the authoring note to `Component-InboundAPI.md` where the method is referenced.
|
||
3. Manual verification step (record the outcome in the commit message): fetch the deployed `DelmiaRecipeDownload` script (`scadabridge api-method get` via the CLI against the target environment, or read the row) and confirm it is idempotent on the natural key; if it is not, file/fix in the environment — this plan only mandates and documents the contract.
|
||
4. `grep -n "at-least-once" docs/plans/2026-06-26-delmia-recipe-notifier-design.md src/ZB.MOM.WW.ScadaBridge.DelmiaNotifier/README.md` → both hit.
|
||
5. Commit: `git add docs/plans/2026-06-26-delmia-recipe-notifier-design.md src/ZB.MOM.WW.ScadaBridge.DelmiaNotifier/README.md docs/requirements/Component-InboundAPI.md && git commit -m "docs(delmia): document at-least-once failover semantics + DelmiaRecipeDownload idempotency requirement"`
|
||
|
||
### Task 24: Document the dual-key API-key rotation procedure
|
||
|
||
**Classification:** trivial (documentation)
|
||
**Estimated implement time:** 3 min
|
||
**Parallelizable with:** 1, 3-22 (after 2 and 5 — same doc file)
|
||
**Files:**
|
||
- Modify: `docs/requirements/Component-InboundAPI.md` (new "### Key rotation" subsection under the API-key auth section, ~line 62+)
|
||
|
||
**Steps:**
|
||
|
||
1. Add the procedure the `sbk_<keyId>_<secret>` design already supports: (1) create a new key with the same scope set (`apikey create`), (2) deliver the new token to the caller and confirm cutover (the audit log's Actor field shows which key serves traffic), (3) disable — don't delete — the old key (`apikey update --enabled false`; reversible), (4) delete after a quarantine period. Note that scopes are copied at creation (scope drift between the pair is an operator error) and that both keys verifying simultaneously is the intended overlap window.
|
||
2. Verify cross-references: the CLI command names used must match `src/ZB.MOM.WW.ScadaBridge.CLI/Commands/SecurityCommands.cs` (adjust wording to the real verbs).
|
||
3. Commit: `git add docs/requirements/Component-InboundAPI.md && git commit -m "docs(inbound-api): dual-key API-key rotation procedure (create -> migrate -> disable -> delete)"`
|
||
|
||
---
|
||
|
||
## Dependencies on other plans
|
||
|
||
- **Plan 05 (Templates/Deployment/Transport)** designs the general "script artifact changed → invalidate everywhere" contract and owns the **bundle-import** side of cache invalidation. This plan's Task 3 provides the Inbound API consumer entry point (`InboundScriptExecutor.InvalidateMethod`) that plan 05's import path must call for every overwritten `ApiMethod`. **Deliberate resilience:** Task 1's per-request revision check is the correctness fallback — inbound methods heal even if an invalidation call is missed or plan 05 lands later, so there is no hard sequencing requirement in either direction.
|
||
- **Plan 04 (Data/Audit backbone)** owns NotificationOutbox dispatcher/audit-row gaps. Tasks 17/18/21 touch `NotificationOutboxActor.cs` and the delivery adapters — if plan 04 is executed concurrently, coordinate on that file (this plan's edits are confined to `RunDispatchSweepAsync`'s policy resolution, `DeliverOneAsync`'s policy selection, and the two adapters).
|
||
- **Plan 08** flagged `SmsConfiguration.MaxRetryCount` unused — ownership of the wiring is **here** (Task 18); plan 08 should not duplicate it.
|
||
- Tasks 8 and 19 both add EF migrations against the shared model snapshot — they are sequenced (19 after 8) within this plan; if any other plan adds migrations in the same window, serialize migration-generation across plans.
|
||
|
||
## Execution order
|
||
|
||
**P0 (start immediately, independent):** Task 1 (the High-severity stale-handler fix) and Task 8 (the High-severity spec-gap migration).
|
||
|
||
- **Inbound chain:** 1 → 2 → 3 → 4 → 6 → 5 → 24 (2/3 also gate 11's ManagementActor edits ordering; 24 last because it shares `Component-InboundAPI.md` with 2 and 5).
|
||
- **ESG chain:** 8 → {9, 10, 11, 12 in parallel} → 13 → 14 → 15 → 16 (9/13/14/15/16 serialize on `ExternalSystemClient.cs`; 10/11/12 are parallel with all of them).
|
||
- **Notifications:** 17 (anytime) ; 18 → 21 ; 19 (after 8, migration serialization) → 20 → 21 ; 22 after 11 (shared `ManagementActor.cs`).
|
||
- **Docs:** 23 anytime; 24 after 2/5.
|
||
|
||
Intra-plan critical path: **1 → 2 → 3 → 4 → 5** (inbound executor file chain) and **8 → 9 → 13 → 14 → 15 → 16** (ESG client file chain) — roughly equal length; run them in parallel lanes. Everything else hangs off those two spines or is free-floating.
|