docs(code-reviews): review Secrets, LocalDb, GalaxyRepository, Overview; re-review changed libs (scadaproj#15)
First reviews of Secrets, LocalDb, GalaxyRepository and Overview, and
2026-11-09 re-reviews of Auth, Health, Theme, Telemetry, Audit and
Configuration, all at 91fb2b4. 49 new open findings, each with a P1/P2/P3
priority and a failure scenario: 1 P1, 13 P2, 35 P3. README regenerated;
REVIEW-PROCESS, template and regen script now cover all ten libraries and
the Priority field.
Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV
This commit is contained in:
Joseph Doherty
@@ -0,0 +1,301 @@
|
||||
# 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 | 5 |
|
||||
|
||||
## 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 | Open |
|
||||
| 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**
|
||||
|
||||
_Unresolved._
|
||||
|
||||
### 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 | Open |
|
||||
| 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**
|
||||
|
||||
_Unresolved._
|
||||
|
||||
### 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._
|
||||
Reference in New Issue
Block a user