scadaproj/code-reviews/Overview/findings.md

# Code Review — Overview

| Field | Value |
|-------|-------|
| Library | `ZB.MOM.WW.Overview/` (hosted application, not a packed library) |
| Packages | none (app; `IsPackable=false`) |
| Component spec | `docs/plans/2026-07-22-overview-dashboard-design.md` (+ `docs/plans/2026-07-22-overview-dashboard-impl-plan.md`) |
| Shared contract | Consumes the `ZB.MOM.WW.Health` `ZbHealthWriter` JSON body (`/health/ready`, `/health/active`); mirrored in `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/ZbHealthReport.cs` |
| Status | Reviewed |
| Last reviewed | 2026-11-09 |
| Reviewer | Claude |
| Commit reviewed | `91fb2b4` |
| Open findings | 0 |

## Summary

The dashboard meets its core requirements:

- **No authentication, no credentials, no mutating endpoint.** The only interaction is a per-circuit
  pause toggle.
- **No SSRF surface.** Probe URLs come only from validated configuration (absolute http/https).
- **Health-check JSON cannot inject markup.** The one `MarkupString` (cluster evidence line) is built
  from `WebUtility.HtmlEncode`-escaped values. Everything else renders through Razor encoding.
- **Blazor state is marshalled and cleaned up.** Store-change notifications go through `InvokeAsync`,
  and the page unsubscribes and disposes its timer.
- **Sweeps cannot overlap.** There is a single writer and an immutable snapshot swap.
- **DI is covered.** `BootTests` boots the real host through `WebApplicationFactory`, so the DI
  wiring has container-level coverage.

The suite passes: 188/188 tests.

The one serious defect is error handling at the probe boundary, Overview-001. `ZbHealthReportClient`
catches only `HttpRequestException` and `OperationCanceledException`. Any other exception from one
instance's response faults the poller `BackgroundService`, and the default `StopHost` policy then
takes down the whole dashboard. This was verified end-to-end: a single registered endpoint answering
with an unknown `charset` stopped the host within one sweep.

The other findings cover:

- leader-chip mis-attribution when two cluster groups of one application share hosts, which is the
  QA ScadaBridge layout (Overview-002);
- a status-only active probe that reads any 200, including a followed login redirect, as Active
  (Overview-003);
- one slow instance gating the whole sweep (Overview-004);
- stale docs (Overview-005).

Known open item, not a code finding: scadaproj #14 (re-run the two OtOpcUa-blocked Phase 4
live-acceptance checks).

## Checklist coverage

| # | Category | Examined | Notes |
|---|----------|----------|-------|
| 1 | Correctness & logic bugs | ☑ | Status derivation, damping and staleness are correct and table-tested. See Overview-002 (leader resolution scans the whole application) and Overview-003 (any 200 = Active). |
| 2 | Public API surface & compatibility | ☑ | App, not a library. The `ZbHealthReport` mirror is tolerant (case-insensitive, open `data` as `JsonElement`). No issues found. |
| 3 | Concurrency & thread safety | ☑ | Single-writer poller. `PollTarget.Last` is written only after `Task.WhenAll`. `Volatile` snapshot swap. Blazor updates go through `InvokeAsync`. The `_paused` read off the render thread is benign. No issues found. |
| 4 | Error handling & resilience | ☑ | See Overview-001 (one bad response kills the host) and Overview-004 (one slow instance stales every card). |
| 5 | Security & secret handling | ☑ | Anonymous and read-only by design. No credentials in registry or code. `MarkupString` input is HTML-encoded. Management links and probe URLs come from config only and are scheme-validated. No issues found. |
| 6 | Performance & resource management | ☑ | One pooled `IHttpClientFactory` client per sweep. Per-request linked CTS disposed. Page timer and event handler released in `Dispose`. No issues found beyond Overview-004. |
| 7 | Spec & shared-contract adherence | ☑ | Matches design §2 (anonymous, cache-first) and the Health 0.3.0 `activeNode`/`leader` data keys. See Overview-005: the design and options docs say `/health/active` is probed only when `HasActiveRole`, but the code probes it unconditionally. |
| 8 | Packaging, dependencies & project layout | ☑ | Inline pins (family app convention). No Auth/Audit/Secrets references, as required. Package source mapping restricts the Gitea feed to `ZB.MOM.WW.*`. No issues found. |
| 9 | Testing coverage | ☑ | Strong: poller, client, derivation, leader resolver, page and card via bUnit, boot. Gaps: no test for a non-`HttpRequestException` failure from a probe (Overview-001), no cross-group same-host leader test (Overview-002), no redirect test (Overview-003). |
| 10 | Documentation & XML docs | ☑ | See Overview-005. The `deployment.md` §7 claim that QA's registry resolves leaders correctly is undermined by Overview-002. |

## Findings

### Overview-001 — Any non-`HttpRequestException` from one instance faults the poller and stops the whole dashboard

| | |
|--|--|
| Severity | High |
| Priority | P1 |
| Category | Error handling & resilience |
| Status | Resolved |
| Location | `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/ZbHealthReportClient.cs:99` (catch list `:122-134`; unguarded loop `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/OverviewPollerService.cs:86-95`) |

**Description**

The method promises it "never throws for an unreachable endpoint". It catches only
`OperationCanceledException`, `HttpRequestException` and, around deserialization, `JsonException`.

Other exceptions reachable from a remote response escape `ProbeAsync`. The confirmed case is
`response.Content.ReadAsStringAsync` (`:99`), which throws **`InvalidOperationException`** when the
`Content-Type` charset is not a known encoding.

From there the exception propagates up through each layer, none of which catches it:

1. The probe's `ProbeAsync` task faults.
2. `Task.WhenAll` in `SweepAsync` rethrows it.
3. `ExecuteAsync` catches only `OperationCanceledException` when stopping.
4. The `BackgroundService` faults.
5. The host applies the default `HostOptions.BackgroundServiceExceptionBehavior = StopHost`.

A malformed answer from any *one* registered instance therefore terminates the entire dashboard. This
contradicts design §5, which classes a non-JSON or failed answer as an **Unreachable** instance,
not a fatal error.

**Failure scenario**

A registered `BaseUrl` points at something that is not a healthy family app: a port reused by another
service, a misconfigured reverse proxy, or an app that emits `Content-Type: application/json;
charset=<unsupported>`. Verified end-to-end with `WebApplicationFactory<Program>` and a local
`HttpListener` answering `charset=bogus-xyz` for one of two instances. Within the first sweep the log
showed:

- `[ERR] BackgroundService failed — System.InvalidOperationException: The character set provided in
  ContentType is invalid`
- `[FTL] ... BackgroundServiceExceptionBehavior is configured to StopHost ... the IHost instance is
  stopping`

`ApplicationStopping` fired within 256 ms. On wonder, NSSM restarts the service, and it dies again on
its first sweep: a crash loop. Operators lose the fleet view because of a single bad entry.

**Recommendation**

- In `ZbHealthReportClient.SendAsync`, after the `OperationCanceledException` filters, add a final
  `catch (Exception ex)` that returns `Failed(startedAt, statusCode?, ex.Message)`.
- Alternatively, read the body as bytes and decode as UTF-8 yourself, which removes the charset
  dependency.
- Defence in depth: wrap each per-target `ProbeAsync` in `SweepAsync` so a throwing probe becomes an
  `Unreachable` result for that instance only, and catch-and-log per sweep in `ExecuteAsync` so one
  bad sweep cannot end the loop.
- Add a test with a handler returning an invalid charset, and one whose handler throws an arbitrary
  exception.

**Resolution**

Resolved 2026-11-09 in the commit tagged `(scadaproj#29)` — `ZbHealthReportClient.SendAsync` now ends with a catch-all that returns an Unreachable result (keeping the status code when one was received), so an unknown `charset` or any other non-shutdown exception costs only that instance. Defence in depth: `OverviewPollerService` wraps each per-target probe (`Guarded`) and catches-and-logs per sweep in `ExecuteAsync`, so neither a throwing probe nor a throwing sweep can fault the `BackgroundService`. Regression tests: `ZbHealthReportClientTests.Probe_UnknownCharset_IsUnreachableNotThrown` (both tiers), `Probe_ArbitraryHandlerException_IsUnreachableNotThrown`, `OverviewPollerServiceTests.Sweep_OneInstanceWithAnUnreadableBody_DoesNotFaultTheSweep` and `ExecuteLoop_SurvivesAnInstanceWithAnUnreadableBody` — all five fail without the fix (the loop test with the same `InvalidOperationException` from the finding); the poller tests also pass with the client fix reverted, proving the per-target guard independently.

### Overview-002 — Leader address is resolved against every instance of the application, not the group, so co-located groups steal each other's chip

| | |
|--|--|
| Severity | Medium |
| Priority | P2 |
| Category | Correctness & logic bugs |
| Status | Resolved |
| Location | `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/LeaderResolver.cs:85` (scan at `:150-166`) |

**Description**

`ResolveGroup` passes `allInstances` (every instance in the application) to
`ResolveToInstanceName`. That method returns the **first** instance in registry order whose
`BaseUrl` host equals the Akka address host.

A group's leader is by definition one of that group's members, but the members are never searched
first. Whenever two groups of the same application share hosts, the group listed later gets the
earlier group's instance name on its "Leader ·" chip. No card in the right group then gets the Leader
badge: `IsLeader` compares within the group, and the resolved name belongs to the other group.

`ZB.MOM.WW.Overview/CLAUDE.md` and `deployment.md` §7 describe this as a single-box problem (worked
around on VD03 with `127.0.0.1` BaseUrls) and state that "QA's registry can use real FQDNs" because
the hosts are distinct. On QA, however, ScadaBridge **central-a and site-a both run on vq03, and
central-b and site-b on vq04** (`deployment.md:52-53`), all inside one `ScadaBridge` application.

**Failure scenario**

On the QA dashboard (`vq03:5320` / `vq04:5320`), ScadaBridge's central group is listed first and the
site cluster's leader is on vq03. A scratch test of that exact layout gave:

- `central: leaderInstance=central-a`
- `site: leaderInstance=central-a`

So the site group header reads "Leader · central-a" (a node in a different Akka cluster), and neither
site card shows the Leader chip. This is the only environment with a real HA pair, which is where
operators read leadership during failover. This is inferred from the documented QA layout; the QA
overlay is deployed-only and was not read.

**Recommendation**

In `ResolveGroup`, resolve against `members` first and fall back to `allInstances` only if no member
matches. Add a `LeaderResolverTests` case with two co-located groups. Then revisit the CLAUDE.md /
`deployment.md` guidance: with the fix, the VD03 `127.0.0.1` workaround also becomes unnecessary.

**Resolution**

Resolved 2026-11-09 in the commit tagged `(scadaproj#43)` — `LeaderResolver.ResolveGroup` now resolves the winning address against the group's own members only. Deliberately **no** fall-back to the whole application (a deviation from the recommendation): the fall-back reintroduces the bug whenever a group's member does not match by host but another group's does (e.g. a `127.0.0.1` member next to an FQDN member of another group); an unmatched address keeps its raw form, as before. Regression tests `LeaderResolverTests.Resolve_CoLocatedGroups_ResolveTheLeaderWithinTheirOwnGroup` (the QA central/site layout; failed with `site` → `central-a`) and `Resolve_LeaderHostOnlyInAnotherGroup_IsNotAttributedAcrossGroups` both fail without the fix. `ZB.MOM.WW.Overview/CLAUDE.md` corrected; the matching `deployment.md` §7 note (root doc, outside this change) is still stale.

### Overview-003 — Status-only active probe treats any 200, including a followed redirect to a login page, as Active

| | |
|--|--|
| Severity | Low |
| Priority | P3 |
| Category | Correctness & logic bugs |
| Status | Resolved |
| Location | `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/ZbHealthReportClient.cs:112` (client config `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/PollingServiceCollectionExtensions.cs:25`) |

**Description**

`ProbeStatusAsync` (`bodyRequired: false`) returns `Reachable = true` with the HTTP status even when
the body is not health JSON. `StatusDerivation.ObserveActive` then maps 200 to `Active`.

The named `HttpClient` keeps the default `AllowAutoRedirect = true`. If `/health/active` 302s to a
login page, which is how ASP.NET cookie auth answers an unauthenticated request, the client follows
the redirect and the final 200 HTML page is reported as **Active**.

The ready tier is protected, because its body must parse. The active tier is not.

**Failure scenario**

A family app is redeployed with a fallback authorization policy that accidentally covers
`/health/active`. This shape has happened before: OtOpcUa's LocalDb sync POST was once answered by
the login 302. Both nodes of the pair now show the **Active** chip, and the raw-signals footer reads
`active 200`, while their true role is unknown.

Scratch test: a handler returning `200 text/html <html>login</html>` gives
`ObserveActive(true, probe) == Active`.

**Recommendation**

Configure the probe handler with `AllowAutoRedirect = false`
(`ConfigurePrimaryHttpMessageHandler(() => new SocketsHttpHandler { AllowAutoRedirect = false })`)
so a 3xx maps to `Unknown`. Optionally, for a non-empty body that is not health JSON, treat 200 as
`Unknown`: the comment's legitimate case is an *empty* body.

**Resolution**

Resolved 2026-11-09 in the commit tagged `(scadaproj#48)` — the named probe `HttpClient` now uses `SocketsHttpHandler { AllowAutoRedirect = false }`, so a 3xx on `/health/active` surfaces as its own status code and `ObserveActive` maps it to Unknown (the ready tier already requires a parseable body). Regression test `ProbeRedirectTests.ActiveProbe_AuthRedirectToLoginPage_IsNotReportedAsActive` (real loopback listener, 302 to a 200 login page; failed without the fix). Not done: the optional 'non-empty non-JSON 200 body is Unknown' rule — a plain-text 200 from a non-ZbHealth app on the active tier is a legitimate Active signal, so that needs an owner decision if wanted.

### Overview-004 — One slow instance gates the single sweep, so every other instance goes Stale

| | |
|--|--|
| Severity | Low |
| Priority | P3 |
| Category | Error handling & resilience |
| Status | Resolved |
| Location | `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/OverviewPollerService.cs:120` (sequential ready + active at `:155-171`; validator `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Registry/OverviewOptionsValidator.cs:69-76`) |

**Description**

All due targets are probed in one `Task.WhenAll`, and the next sweep cannot start until the slowest
target finishes. `PeriodicTimer` coalesces missed ticks. Each target makes up to two sequential
requests (ready, then active), so a sweep can last up to `2 × max(effective Timeout)`.

The validator checks `StaleAfter > PollInterval` per instance. It does not check that any instance's
`StaleAfter` exceeds the worst-case sweep duration imposed by *other* instances' timeouts. Per-instance
`TimeoutSeconds` overrides are explicitly supported.

**Failure scenario**

An operator gives a slow WAN site a `TimeoutSeconds: 30` override. That site then hangs, accepting
TCP but not answering. Every sweep now takes about 60 s. Every other instance, with the default
`StaleAfterSeconds: 45`, is re-polled only every ~60 s, so the whole dashboard flips to **Stale**
between polls even though everything else is healthy.

Scratch test: with one instance whose handler delays 4 s, `SweepAsync` took **8.0 s** while the other
instance answered instantly.

**Recommendation**

Decouple targets: run each target on its own loop or task, or let the sweep publish as results
arrive and not wait for laggards past their own interval. At minimum, extend the validator: reject a
configuration where any instance's effective `StaleAfter` is ≤ `2 × max effective Timeout` across the
registry, naming the offending override.

**Resolution**

Resolved 2026-11-09 in the commit tagged `(scadaproj#48)` — chose decoupling over a validator rule. The tick loop no longer awaits the sweep; each target is claimed (`TryClaim`) while its probe is in flight, so later ticks skip a hung target, and each probe records and publishes its own result the moment it completes (publish under a lock). `SweepAsync` still awaits its own batch and returns the final snapshot, so existing semantics for callers and tests hold. Regression test `Sweep_HungInstanceDoesNotHoldUpResultsOrLaterSweepsForTheOthers` (failed without the fix). Side effect: the heartbeat/sweep metric now describe the batch started on each tick, and a tick whose targets are all still in flight stamps the heartbeat immediately (the loop is alive; the probes are bounded by their timeouts).

### Overview-005 — Docs say `/health/active` is probed only when `HasActiveRole`; the code probes it for every reachable instance

| | |
|--|--|
| Severity | Low |
| Priority | P3 |
| Category | Documentation & XML docs |
| Status | Resolved |
| Location | `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Registry/OverviewOptions.cs:9` (also `:76-80`) |

**Description**

Two places say the active tier is gated on the role:

- The `OverviewOptions` remarks (`:8-10`) say "GET `{BaseUrl}/health/ready`, plus
  `{BaseUrl}/health/active` when `HasActiveRole`".
- `InstanceEntry.HasActiveRole` (`:76-80`) says single-instance gateways "are never probed for
  `/health/active`".

Design doc §3 (line 81) and §5 ("only when `HasActiveRole`") say the same. `OverviewPollerService.ProbeAsync` (`:162-171`) deliberately
probes the active tier **unconditionally** whenever ready answered. Only the *interpretation* is gated
on `HasActiveRole`, so that MxAccessGateway's session counts on the active tier are visible.

**Failure scenario**

An operator or reviewer relies on the documented behaviour, for example when sizing the probe load,
or when deciding that a gateway's active-tier auth or rate limit is irrelevant because "Overview never
calls it". They are surprised by twice the requests per gateway, or by gateway access-log noise. A
future maintainer may also "fix" the code to match the doc and silently remove the MXAccess session
row.

**Recommendation**

Update the `OverviewOptions` remarks, the `HasActiveRole` XML doc and design §3: the active tier is
probed whenever ready answers, and `HasActiveRole` controls only whether an Active/Standby badge is
shown.

**Resolution**

Resolved 2026-11-09 in the commit tagged `(scadaproj#48)` — corrected the `OverviewOptions` remarks, the `InstanceEntry.HasActiveRole` XML doc, design doc §3/§5 and the Overview CLAUDE.md: the active tier is probed whenever ready answered and `HasActiveRole` only gates the Active/Standby badge.