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	3

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	Open
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

Unresolved.

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

Severity	Low
Priority	P3
Category	Error handling & resilience
Status	Open
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

Unresolved.

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	Open
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

Unresolved.