Joseph Doherty
5bbd7689fa
Re-ran all 8 domain reviews at HEAD8c888f13against theb910f5ebbaseline: every round-1 finding source-verified (168 fixed, 0 regressions, 0 false claims); 56 new findings (1 Critical / 4 High / 15 Medium / 36 Low), concentrated in post-baseline code (anti-entropy resync, KPI rollup backfill, live alarm stream) and seams the fixes exposed. Headliners: S&F resync predicate inversion can wipe the delivering node's buffer (02-N1 Critical); resync snapshot exceeds the Akka remoting frame size (02-N2); failover drill kills the one node keep-oldest can't survive (01-N1); unbounded rollup backfill per failover (04-R1); live production API key in untracked test.txt (08-NF1). Adds PLAN-R2-01..08 + .tasks.json manifests and the Round-2 board, P0 list, cross-plan mutexes, and wave order in 00-MASTER-TRACKER.
810 lines
51 KiB
Markdown
810 lines
51 KiB
Markdown
# PLAN-R2-06 — Edge Integrations Round-2 Fixes
|
||
|
||
> **For Claude:** REQUIRED SUB-SKILL: Use superpowers-extended-cc:executing-plans to implement this plan task-by-task.
|
||
|
||
**Goal:** Close every NEW finding in `archreview/06-edge-integrations.md` (round 2, 2026-07-12): wire the Inbound API compiled-handler cache as the real `IScriptArtifactChangeBus` consumer the Host comment and master tracker already claim exists (N1, the round-1 U2 residue) and sweep the false claims out of the docs; park (not retry-to-exhaustion) buffered ESG calls that fail deterministically with `ArgumentException` (N2); replace `SITE_UNREACHABLE` message-substring sniffing with a typed signal at the Ask boundary (N3); honor the declared response charset in the bounded body read (N4); and close the chunked-body bypass of the 415 guard (N5).
|
||
|
||
**Architecture:** The N1 consumer is a small `IHostedService` (`ScriptArtifactChangeSubscriber`) owned by the InboundAPI component and registered in `AddInboundAPI`: on start it calls `IScriptArtifactChangeBus.Subscribe` and, for every `ApiMethod` notification, calls the existing `InboundScriptExecutor.InvalidateMethod` seam (`InboundScriptExecutor.cs:136-140`) per name — the per-request revision check stays the correctness fallback per the contract's normative semantics (`docs/plans/2026-07-08-script-artifact-invalidation-contract.md` §4), so the subscriber is a fast-path, never a correctness dependency. The bus dependency is optional (`IScriptArtifactChangeBus? = null`, mirroring `BundleImporter.cs:134`) so site roles and plain test hosts no-op. N3 deliberately chooses the **typed-exception** form over an additive error-code field: unreachability manifests at the communication layer as `Akka.Actor.AskTimeoutException` (the site never answered — see `CentralCommunicationActor.HandleSiteEnvelope`'s no-ClusterClient path, which deliberately lets the Ask expire), so `RouteHelper` types it into `SiteUnreachableException` at the four router awaits and the message sniff (`RouteHelper.cs:391-395`) is deleted — a `Success=false` response means the site *answered*, which is by definition not unreachability. **No Commons message contract changes anywhere in this plan** — the PLAN-08 contract-lock suite (`ClusterClientContractLockTests.cs`) is untouched; the `RouteTo*Response` records are not among the locked seven in any case, and this plan does not modify them. N2/N4/N5 are each one-file classification/decoding/guard fixes with the failing test being exactly the report's scenario.
|
||
|
||
**Tech Stack:** C#/.NET 9, xUnit + NSubstitute, ASP.NET Core TestServer (inbound endpoint tests), Roslyn scripting (real compiles in executor tests), Akka.NET (`AskTimeoutException`, transitively referenced via Communication). No EF migrations, no `ManagementActor.cs` edits, no Commons message-contract edits.
|
||
|
||
Build: `dotnet build ZB.MOM.WW.ScadaBridge.slnx`. Test per-project: `dotnet test tests/<project>` — targeted filters only, per task.
|
||
|
||
## Parallelization (incomplete tasks) — 2026-07-13
|
||
|
||
Nothing complete yet. **Ready now** (`blockedBy` empty): **{1, 3, 5, 6}**.
|
||
|
||
**Concurrent lanes (dispatch as separate implementers, fully file-disjoint):**
|
||
- **Inbound consumer lane:** T1 (`ScriptArtifactChangeSubscriber.cs` new + `ServiceCollectionExtensions.cs` + the `Host/Program.cs:95-98` comment) → T2 (docs/tracker truth sweep — no code).
|
||
- **ESG client lane (single-writer on `ExternalSystemClient.cs` + `ExternalSystemClientTests.cs`):** T3 → T4, serial.
|
||
- **Inbound routing lane:** T5 (`RouteHelper.cs` + executor/route tests).
|
||
- **Inbound endpoint lane:** T6 (`EndpointExtensions.cs` + `EndpointContentTypeTests.cs`).
|
||
|
||
**Cross-plan file notes:** T1 touches `Host/Program.cs` (comment only, lines 95-98) — coordinate if another active plan holds the `Program.cs` mutex. Nothing here touches `ManagementActor.cs`, `BundleImporter.cs`, or the EF model snapshot.
|
||
|
||
---
|
||
|
||
### Task 1: `ScriptArtifactChangeSubscriber` — wire the Inbound API as the bus's `ApiMethod` consumer
|
||
|
||
**Classification:** standard (new consumer wiring on the hot handler cache; safe-by-design — the per-request revision check remains the correctness fallback, so a missed/duplicate notification is harmless)
|
||
**Estimated implement time:** ~5 min
|
||
**Parallelizable with:** 3, 4, 5, 6
|
||
**Files:**
|
||
- Create: `src/ZB.MOM.WW.ScadaBridge.InboundAPI/ScriptArtifactChangeSubscriber.cs`
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.InboundAPI/ServiceCollectionExtensions.cs` (lines 12-31: register the hosted service)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.Host/Program.cs` (lines 95-98: rewrite the factually-wrong comment)
|
||
- Test (create): `tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/ScriptArtifactChangeSubscriberTests.cs`
|
||
|
||
The bus contract shipped in PLAN-05 T14 (`IScriptArtifactChangeBus`, `InProcessScriptArtifactChangeBus`, the `BundleImporter` post-commit publisher at `BundleImporter.cs:1811`), and PLAN-06 T3 shipped the `InvalidateMethod` seam — but a repo-wide search confirms **zero `Subscribe` call sites**: the bus publishes into the void while `Host/Program.cs:96-98` claims "the Inbound API compiled-handler cache subscribes as the consumer (wired in plan 06)". This task ships the one real consumer: an `IHostedService` in the InboundAPI project (component-owned, unit-testable — unlike a lambda in the Host composition root). `SharedScript`/`Template` notifications are ignored by this consumer (the executor caches only ApiMethod handlers); their no-consumer status is documented honestly in Task 2.
|
||
|
||
**Steps:**
|
||
|
||
1. Write the failing tests:
|
||
|
||
```csharp
|
||
// tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/ScriptArtifactChangeSubscriberTests.cs
|
||
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);
|
||
}
|
||
}
|
||
```
|
||
|
||
2. Run: `dotnet test tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests --filter ScriptArtifactChangeSubscriberTests` — expect **FAIL** (compile error: no `ScriptArtifactChangeSubscriber` type).
|
||
|
||
3. Implement `src/ZB.MOM.WW.ScadaBridge.InboundAPI/ScriptArtifactChangeSubscriber.cs`:
|
||
|
||
```csharp
|
||
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);
|
||
}
|
||
}
|
||
```
|
||
|
||
In `ServiceCollectionExtensions.AddInboundAPI` (after the `InboundApiEndpointFilter` registration):
|
||
|
||
```csharp
|
||
// 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>();
|
||
```
|
||
|
||
In `Host/Program.cs` replace the comment at lines 95-98 with the truth:
|
||
|
||
```csharp
|
||
// Script-artifact invalidation bus: in-process, per-node pub/sub
|
||
// for ScriptArtifactsChanged. The Transport bundle importer publishes
|
||
// 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.
|
||
```
|
||
|
||
4. Run the filter → **PASS**, then the whole project: `dotnet test tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests` → all green (the existing TestServer-based hosts call `AddInboundAPI` + `StartAsync` with no bus registered — the optional-parameter default keeps them green; this is the same optional-dependency pattern `BundleImporter` already uses). Then `dotnet test tests/ZB.MOM.WW.ScadaBridge.Host.Tests` → green (Program.cs change is comment-only).
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.InboundAPI/ScriptArtifactChangeSubscriber.cs src/ZB.MOM.WW.ScadaBridge.InboundAPI/ServiceCollectionExtensions.cs src/ZB.MOM.WW.ScadaBridge.Host/Program.cs tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/ScriptArtifactChangeSubscriberTests.cs && git commit -m "fix(inbound): wire the compiled-handler cache as the IScriptArtifactChangeBus ApiMethod consumer (plan R2-06 T1)"`
|
||
|
||
### Task 2: Truth sweep — tracker / contract-doc / CLAUDE.md claims about the bus consumer
|
||
|
||
**Classification:** small (documentation)
|
||
**Estimated implement time:** ~4 min
|
||
**Parallelizable with:** 3, 4, 5, 6 (after 1)
|
||
**Files:**
|
||
- Modify: `archreview/plans/00-MASTER-TRACKER.md` (line 36 Wave-4 summary: the "`InvalidateMethod` seam consuming PLAN-05's `IScriptArtifactChangeBus` contract (T3)" claim; line 98 cross-plan handoff bullet)
|
||
- Modify: `docs/plans/2026-07-08-script-artifact-invalidation-contract.md` (line 3 status; §"Handoff to plan 06" rows (a)/(b)/(c), line 58 "no subscribers" sentence)
|
||
- Modify: `CLAUDE.md` (Templates & Deployment section, the "Plan-06 handoff: … (consumers = plan 06)" sentence)
|
||
|
||
N1's second half: the report calls the Host comment (fixed in Task 1) *and* the tracker/doc claims factually wrong — "will misdirect the next person debugging cache coherence." After Task 1 the consumer genuinely exists, so this task makes every claim match reality, including the honest residue: **(b) Management producers are still unwired by design** (the delete path calls `InvalidateMethod` directly at `ManagementActor.cs:2953`; update/edit paths self-heal via the per-request revision check — no notification needed for correctness), and **(c) is satisfied by the revision check** (PLAN-06 T1). SharedScript/Template kinds still publish with no consumer — that is now *documented* as safe (nothing at central caches compiled artifacts of those kinds) instead of silently misdescribed.
|
||
|
||
**Steps:**
|
||
|
||
1. `00-MASTER-TRACKER.md` line 36: change "`InvalidateMethod` seam consuming PLAN-05's `IScriptArtifactChangeBus` contract (T3)" to "`InvalidateMethod` seam (T3; the bus *subscriber* was NOT wired here — arch-review 06 round-2 N1 — and shipped later via PLAN-R2-06 T1)". Line 98: append to the PLAN-05 T14 ↔ PLAN-06 T3 bullet: "**Closed 2026-07-13 (PLAN-R2-06 T1):** `ScriptArtifactChangeSubscriber` (InboundAPI hosted service) subscribes and invalidates on `ApiMethod` notifications."
|
||
2. `docs/plans/2026-07-08-script-artifact-invalidation-contract.md`: line 3 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)." Update the handoff table rows to the same dispositions and rewrite line 58 ("Until (a)–(c) land, plan 05's publisher is a no-op in practice…") to past tense with the shipped/unwired split.
|
||
3. `CLAUDE.md`: change "(consumers = plan 06)" in the Transport bullet to "(ApiMethod consumer = InboundAPI `ScriptArtifactChangeSubscriber`, wired in plan R2-06; SharedScript/Template publish with no consumer — nothing at central caches those kinds)".
|
||
4. Verify no stale claim remains: `grep -rn "wired in plan 06\|consumers = plan 06" src docs CLAUDE.md archreview/plans/00-MASTER-TRACKER.md` → zero hits (Task 1 already rewrote the Program.cs comment).
|
||
5. Commit: `git add archreview/plans/00-MASTER-TRACKER.md docs/plans/2026-07-08-script-artifact-invalidation-contract.md CLAUDE.md && git commit -m "docs(inbound): correct bus-consumer claims in tracker + invalidation-contract doc + CLAUDE.md (plan R2-06 T2)"`
|
||
|
||
### Task 3: `DeliverBufferedAsync` parks deterministic `ArgumentException` failures (path template / verb)
|
||
|
||
**Classification:** small (classification-only change on the buffered path, mirroring the existing `JsonException` poison fix in the same method)
|
||
**Estimated implement time:** ~4 min
|
||
**Parallelizable with:** 1, 2, 5, 6
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway/ExternalSystemClient.cs` (`DeliverBufferedAsync`, lines 214-225: add the catch)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs` (append)
|
||
|
||
N2: `BuildUrl`'s `{param}` substitution throws `ArgumentException` for a placeholder with no matching/non-null parameter (`ExternalSystemClient.cs:535-537`) and `ValidateHttpMethod` throws the same type (`:423-438`). Neither is caught in `DeliverBufferedAsync` (its try at `:214-224` catches only `PermanentExternalSystemException`), and the S&F retry sweep's catch-all treats **any** thrown exception as transient (`StoreAndForwardService.cs:822-830`) — so a method path edited to add `{id}` while calls are buffered burns the full default 50 retries (`StoreAndForwardOptions.cs:21`) with a misleading "transient" `LastError` before parking. Re-running the same substitution against the same stored payload throws deterministically — same poison shape as the `JsonException` fix at `:184-195`. No `StoreAndForwardService` change: the classification decision belongs to the delivery handler, exactly like the existing cases.
|
||
|
||
**Steps:**
|
||
|
||
1. Failing tests (append; reuse the existing `StubResolution`/`BufferedCall`/`MockHttpMessageHandler` helpers — `BufferedCall` builds a payload with `Parameters: null`, which is exactly the unresolvable-placeholder case):
|
||
|
||
```csharp
|
||
[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);
|
||
}
|
||
```
|
||
|
||
2. Run: `dotnet test tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests --filter "DeliverBuffered_PathTemplateNowUnresolvable_ReturnsFalseSoMessageParks|DeliverBuffered_UnsupportedVerbOnStoredMethod_ReturnsFalseSoMessageParks"` → **FAIL** (both tests die on the propagating `ArgumentException` instead of returning `false`).
|
||
3. Implement — in `DeliverBufferedAsync`, after the `catch (PermanentExternalSystemException …)` branch (line 223), add:
|
||
|
||
```csharp
|
||
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;
|
||
}
|
||
```
|
||
|
||
Also extend the method's `<summary>` doc-comment (lines 165-173) to name `ArgumentException` alongside malformed JSON as a park-immediately case.
|
||
4. Run the filter → **PASS**; then the full project `dotnet test tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests` → green; consumer-suite check (no S&F files touched, classification contract only): `dotnet test tests/ZB.MOM.WW.ScadaBridge.StoreAndForward.Tests` → green.
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway/ExternalSystemClient.cs tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs && git commit -m "fix(esg): park buffered calls that fail deterministically with ArgumentException (path template/verb) instead of retrying to exhaustion (plan R2-06 T3)"`
|
||
|
||
### Task 4: Honor the declared response charset in `ReadBodyBoundedAsync`
|
||
|
||
**Classification:** small (decode-only; restores round-1 `ReadAsStringAsync` parity)
|
||
**Estimated implement time:** ~4 min
|
||
**Parallelizable with:** 1, 2, 5, 6 (NOT 3 — same file; run after 3)
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway/ExternalSystemClient.cs` (`ReadBodyBoundedAsync`, line 482 + new private helper)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs` (append)
|
||
|
||
N4: the bounded read ends with `Encoding.UTF8.GetString(...)` (`:482`) regardless of `Content-Type: …; charset=iso-8859-1` — a legacy external system replying in a non-UTF-8 single-byte encoding becomes mojibake in script-visible response text and error bodies. The pre-P1 `ReadAsStringAsync` honored the charset; restore parity: read `content.Headers.ContentType?.CharSet`, `Encoding.GetEncoding` it (strip optional quotes), fall back to UTF-8 on unknown/invalid values (mojibake beats a hard failure for a cosmetic header).
|
||
|
||
**Steps:**
|
||
|
||
1. Failing tests (append; add the small byte-body handler beside `MockHttpMessageHandler` at line ~1100):
|
||
|
||
```csharp
|
||
[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);
|
||
}
|
||
}
|
||
```
|
||
|
||
2. Run: `dotnet test tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests --filter "Call_Latin1Charset_DecodesUsingDeclaredCharset|Call_UnknownCharset_FallsBackToUtf8"` → **FAIL** (`Call_Latin1Charset…`: UTF-8 decode yields U+FFFD for `é`/`°`; the fallback test passes already — it pins the tolerance so the fix cannot regress it into a throw).
|
||
3. Implement — replace line 482 with:
|
||
|
||
```csharp
|
||
// 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);
|
||
```
|
||
|
||
and add below `ReadBodyBoundedAsync`:
|
||
|
||
```csharp
|
||
/// <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;
|
||
}
|
||
}
|
||
```
|
||
|
||
Also update the `ReadBodyBoundedAsync` doc-comment (lines 440-450) to state the charset behavior.
|
||
4. Run the filter → **PASS**; full project run `dotnet test tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests` → green (the oversized-body and timeout tests at `:1047` are decode-independent).
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway/ExternalSystemClient.cs tests/ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests/ExternalSystemClientTests.cs && git commit -m "fix(esg): honor the declared response charset in ReadBodyBoundedAsync with UTF-8 fallback (plan R2-06 T4)"`
|
||
|
||
### Task 5: Typed `SITE_UNREACHABLE` — `AskTimeoutException` classification replaces message sniffing
|
||
|
||
**Classification:** standard (reclassifies the routed-failure taxonomy for every inbound routed call; the change also makes SITE_UNREACHABLE fire for the *real* unreachable path for the first time)
|
||
**Estimated implement time:** ~5 min
|
||
**Parallelizable with:** 1, 2, 3, 4, 6
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs` (lines 16-21: inner-exception ctor; 185, 233, 297, 348: the four router awaits; 364-377: `ResolveSiteAsync` comment; 379-395: `RoutingFailure` + delete `IsUnreachable`)
|
||
- Modify: `docs/requirements/Component-InboundAPI.md` (error-code table: `SITE_UNREACHABLE` = "the site did not respond to the routed request (no contact / down / partitioned)")
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs` (rewrite the sniff-pinning test at lines 100-124, add the anti-sniff test), `tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/RouteHelperTests.cs` (append)
|
||
|
||
N3: `RouteHelper.IsUnreachable` (`:391-395`) matches `"unreachable"`/`"no contact"` substrings in `ErrorMessage` text produced by another component — a harmless rewording silently downgrades `SITE_UNREACHABLE` to `SCRIPT_ERROR` with no test failing at the boundary. The typed signal already exists in the communication layer: an unreachable site means the ClusterClient Ask **expires** (`CentralCommunicationActor.HandleSiteEnvelope` deliberately drops enveloped messages for sites with no client so "the Ask will timeout on the caller side"; `CommunicationService.RouteTo*Async` are plain bounded Asks), surfacing as `Akka.Actor.AskTimeoutException` — which today falls into the executor's generic catch as `SCRIPT_ERROR`. Conversely, a `Success=false` *response* means the site **answered** — by definition not unreachability. So: type `AskTimeoutException → SiteUnreachableException` at the four router awaits, and make `RoutingFailure` always a plain `InvalidOperationException`. **No Commons message-contract change** (the typed-exception form was chosen over an additive error-code field on the `RouteTo*Response` records, so the PLAN-08 contract-lock suite needs no update; those records are not among the locked seven anyway, and additive-only + default(T)-safe rules would apply if a future change adds such a field).
|
||
|
||
**Steps:**
|
||
|
||
1. Failing tests. In `InboundScriptExecutorTests.cs`, **rewrite** `RoutedSiteUnreachable_Returns500WithSiteUnreachableCode` (lines 100-124 — it currently pins the sniff by stubbing `ErrorMessage: "Site unreachable"`) and add the anti-sniff case:
|
||
|
||
```csharp
|
||
[Fact]
|
||
public async Task RoutedAskTimeout_Returns500WithSiteUnreachableCode()
|
||
{
|
||
// 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(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\");")
|
||
{ Id = 7, TimeoutSeconds = 10 };
|
||
|
||
var result = await _executor.ExecuteAsync(
|
||
method, new Dictionary<string, object?>(), route, TimeSpan.FromSeconds(10));
|
||
|
||
Assert.False(result.Success);
|
||
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);
|
||
}
|
||
```
|
||
|
||
In `RouteHelperTests.cs` (uses the existing `_locator`/`_router`/`CreateHelper`/`SiteResolves` scaffolding), append:
|
||
|
||
```csharp
|
||
[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" }));
|
||
}
|
||
```
|
||
|
||
2. Run: `dotnet test tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests --filter "RoutedAskTimeout_Returns500WithSiteUnreachableCode|SiteRespondedFailure_MessageMentioningUnreachable|AskTimeout_ThrowsSiteUnreachableException"` → **FAIL** (Ask-timeout cases surface `SCRIPT_ERROR`/`AskTimeoutException`; the anti-sniff case returns `SITE_UNREACHABLE`).
|
||
3. Implement in `RouteHelper.cs` (add `using Akka.Actor;`):
|
||
- `SiteUnreachableException` gains `public SiteUnreachableException(string message, Exception innerException) : base(message, innerException) { }` and its class doc-comment is rewritten: thrown when the routed Ask **expires** (typed `AskTimeoutException` from the communication layer) — never inferred from message text.
|
||
- Add to `RouteTarget`:
|
||
|
||
```csharp
|
||
/// <summary>
|
||
/// 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 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);
|
||
}
|
||
}
|
||
```
|
||
|
||
- Wrap the four router awaits, e.g. line 185: `var response = await RouteOrUnreachable(_instanceRouter.RouteToCallAsync(siteId, request, token));` (same at 233, 297, 348).
|
||
- Replace `RoutingFailure` + delete `IsUnreachable` (lines 379-395):
|
||
|
||
```csharp
|
||
/// <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);
|
||
```
|
||
|
||
- Fix the now-stale comment inside `ResolveSiteAsync` (lines 368-371) — a null site id is a plain not-found, never sniffed.
|
||
- Update the `Component-InboundAPI.md` error-code table row for `SITE_UNREACHABLE` to the typed semantics ("the target site did not respond to the routed request within the routing timeout").
|
||
4. Run the filter → **PASS**; full project `dotnet test tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests` → green (no other test stubs unreachable-flavored `ErrorMessage` text).
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs docs/requirements/Component-InboundAPI.md tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/RouteHelperTests.cs && git commit -m "fix(inbound): type SITE_UNREACHABLE on AskTimeoutException at the router await instead of message-substring sniffing (plan R2-06 T5)"`
|
||
|
||
### Task 6: 415 guard covers chunked (no Content-Length) bodies
|
||
|
||
**Classification:** small (guard-condition fix on the endpoint; also fixes lenient parsing of chunked bodies)
|
||
**Estimated implement time:** ~4 min
|
||
**Parallelizable with:** 1, 2, 3, 4, 5
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.InboundAPI/EndpointExtensions.cs` (lines 182-212: the 415 guard at 187-194 and the parse condition at 206-207)
|
||
- Modify: `docs/requirements/Component-InboundAPI.md` (the 415 row: "a body — declared by Content-Length or Transfer-Encoding (chunked)")
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointContentTypeTests.cs` (append)
|
||
|
||
N5: the 415 guard keys on `ContentLength > 0` (`:187`); a chunked non-JSON body has null `ContentLength`, skips the 415, then also skips JSON parsing (the parse condition `:206-207` is `ContentLength > 0 || ContentType contains json`), so `body = null` and the caller gets a misleading `VALIDATION_FAILED` — the same error class S9 set out to fix, now confined to the chunked edge. Fix: compute `hasBody` once (Content-Length positive, OR Content-Length absent with a `Transfer-Encoding` header present) and use it in **both** the 415 guard and the parse condition — so a chunked JSON/no-Content-Type body also parses leniently, matching the fixed-length behavior. The round-1 leniencies are preserved: no body → no 415; body with no Content-Type → lenient JSON parse.
|
||
|
||
**Steps:**
|
||
|
||
1. Failing tests (append to `EndpointContentTypeTests`, reusing `BuildHostAsync`/`SeedKeyAsync`; add the non-seekable content helper so the client can never auto-compute a Content-Length):
|
||
|
||
```csharp
|
||
[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;
|
||
}
|
||
}
|
||
```
|
||
|
||
2. Run: `dotnet test tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests --filter "ChunkedNonJsonBody_Returns415|ChunkedJsonBody_NoContentType_StillParsesLeniently"` → **FAIL** (first: 400 `VALIDATION_FAILED` instead of 415; second: 400 "missing required parameter" instead of 200).
|
||
3. Implement in `EndpointExtensions.cs` — above the S9 block (line ~182), compute the body signal once, then use it in both places:
|
||
|
||
```csharp
|
||
// 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);
|
||
```
|
||
|
||
The 415 guard (lines 187-189) becomes `if (hasBody && !string.IsNullOrEmpty(httpContext.Request.ContentType) && httpContext.Request.ContentType.Contains("json", StringComparison.OrdinalIgnoreCase) == false)`; the parse condition (lines 206-207) becomes `if (hasBody || httpContext.Request.ContentType?.Contains("json", StringComparison.OrdinalIgnoreCase) == true)`. Update the S9 comment block (lines 182-186) to name the chunked case. Update the doc's 415 row.
|
||
4. Run the filter → **PASS**; full project `dotnet test tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests` → green (`NonJsonContentType_WithBody_Returns415` and the lenient no-Content-Type case at `EndpointContentTypeTests.cs:96-146` still pass — fixed-length behavior is unchanged, and a zero-length body still never 415s).
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.InboundAPI/EndpointExtensions.cs docs/requirements/Component-InboundAPI.md tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/EndpointContentTypeTests.cs && git commit -m "fix(inbound): 415 guard and lenient JSON parse cover chunked (no Content-Length) bodies (plan R2-06 T6)"`
|
||
|
||
---
|
||
|
||
## Dependencies on other plans
|
||
|
||
- **PLAN-R2-05 owns the publisher-side Add-skip fix.** `BundleImporter.PublishScriptArtifactChanges` (`BundleImporter.cs:1777-1817`, publish at `:1811`) deliberately excludes `Add` resolutions ("nothing is cached under a brand-new name yet"); revisiting that rationale is PLAN-R2-05's finding. Task 1's subscriber has **no `blockedBy`** on it — the bus contract shipped in PLAN-05 T14, the subscriber consumes whatever is published, and `InvalidateMethod` is safe for unknown names, so when PLAN-R2-05 broadens the publisher the consumer needs zero changes. Files are disjoint (`BundleImporter.cs` vs `InboundAPI/*`) — safe to run both plans concurrently.
|
||
- **Contract-lock (PLAN-08 T9).** This plan touches no Commons message contracts, so `ClusterClientContractLockTests.cs` (locked records: `CreateDataConnectionCommand`, `DeployInstanceCommand`, `NotificationSubmit`, `AttributeValueChanged`, `GetAttributeRequest`, `SetStaticAttributeCommand`, `ManagementEnvelope`) needs no update. Task 5 deliberately chose the typed-exception form over an additive error-code field on the `RouteTo*Response` records; those records are not among the locked seven, but any future additive field there must still be safe at `default(T)` (the locked reality: Akka's Newtonsoft serializer ignores C# optional-parameter defaults on missing fields).
|
||
- **`Host/Program.cs` mutex.** Task 1's edit is comment-only (lines 95-98) — coordinate only if another active plan is editing `Program.cs` in the same window.
|
||
|
||
## Execution order
|
||
|
||
**P0 (start immediately, independent, file-disjoint):** Tasks 1, 3, 5, 6 in parallel lanes.
|
||
|
||
- **Inbound consumer lane:** 1 → 2 (docs truth sweep last, so it records what actually shipped).
|
||
- **ESG client lane:** 3 → 4 (both edit `ExternalSystemClient.cs` + the same test file — serial).
|
||
- **Inbound routing lane:** 5 (free-floating).
|
||
- **Inbound endpoint lane:** 6 (free-floating).
|
||
|
||
Intra-plan critical path: **3 → 4** and **1 → 2** (both two deep); everything else is single-task. No EF migrations, no `ManagementActor.cs`, no initiative-wide mutexes beyond the `Program.cs` comment note above.
|
||
|
||
## Findings Coverage
|
||
|
||
| # | Finding (report section) | Severity | Task(s) |
|
||
|---|--------------------------|----------|---------|
|
||
| N1 | Script-artifact invalidation bus publishes into the void; Host comment + master tracker claim a consumer that does not exist (the open PLAN-05→PLAN-06 handoff) | Medium | 1 (subscriber + Program.cs comment), 2 (tracker/contract-doc/CLAUDE.md truth sweep) |
|
||
| N2 | Now-unresolvable path template (or bad verb) on a buffered message classified transient and retried to exhaustion (50 retries) before parking with a misleading `LastError` | Low | 3 |
|
||
| N3 | `SITE_UNREACHABLE` classification is substring-sniffing on another component's error-message text; a rewording silently downgrades it to `SCRIPT_ERROR` | Low | 5 (typed `AskTimeoutException → SiteUnreachableException` at the router await; sniff deleted; no Commons contract change → contract-lock suite untouched) |
|
||
| N4 | Bounded response read decodes UTF-8 unconditionally, dropping the round-1 charset honoring (mojibake for non-UTF-8 legacy systems) | Low | 4 |
|
||
| N5 | 415 guard keyed on `ContentLength > 0` is bypassed by chunked non-JSON bodies, resurfacing the misleading-error class S9 fixed | Low | 6 |
|
||
| U2 (round-1 partial) | No cache-coherence mechanism for compiled handlers — the revision check shipped but the claimed bus consumer was never wired | Underdeveloped | Same as N1 → 1, 2 (the report folds the U2 residue into N1) |
|
||
| U4 (round-1 partial) | Twilio accept-only: status-callback webhook / per-recipient delivery state | Underdeveloped | **Deferred (unchanged)** — stays out of scope per the round-1 ruling (documented in `Component-NotificationService.md:95`): an inbound webhook endpoint is new attack surface warranting its own design pass. The residual minor (SID rows saved before validation are not retro-checked) is accepted. No task. |
|