scadaproj/code-reviews/Health/findings.md

Code Review — Health

Field	Value
Library	ZB.MOM.WW.Health/
Packages	ZB.MOM.WW.Health, ZB.MOM.WW.Health.Akka, ZB.MOM.WW.Health.EntityFrameworkCore
Component spec	components/health/spec/SPEC.md
Shared contract	components/health/shared-contract/ZB.MOM.WW.Health.md
Status	Reviewed
Last reviewed	2026-11-09 (re-review; baseline 2026-06-01 at 5f75cd4)
Reviewer	Claude (2026-11-09 re-review, scadaproj#15; baseline: Claude automated)
Commit reviewed	91fb2b4
Open findings	0

Summary

2026-11-09 re-review (91fb2b4, scadaproj#15). Re-reviewed at 91fb2b4 (2026-11-09) against the 2026-06-01 baseline 5f75cd4; the library is now 0.3.0. The headline change, the active tier now meaning "oldest Up member" through the new ClusterActiveNode helper instead of Leader/RoleLeader, is correct. Only MemberStatus.Up members are eligible, so Joining, WeaklyUp, Leaving, Exiting, Down and Removed are all excluded. Ties fall through to Akka's Member.IsOlderThan, which breaks an equal up-number on address ordering, so the choice is deterministic. Self-identity is compared on UniqueAddress, so a restarted incarnation on the same host and port is never mistaken for the old one. An Up-but-unreachable oldest member is still named active, which matches singleton placement until the split-brain resolver downs it. A role-scoped node that carries none of the preferred roles answers NoActiveRoleStatus (default Unhealthy, i.e. 503). Both startup-safety paths (no ActorSystem, cluster not yet accessible) still return Degraded (200), and a container-built AddCheck<ActiveNodeHealthCheck> probe confirmed that ActivatorUtilities picks the right constructor out of the three. The status mapping, the data JSON shape (emitted only when the check supplies data, description always present) and the composite convention builder are correct. All 76 tests pass (Health.Tests 25, Health.Akka.Tests 45, Health.EntityFrameworkCore.Tests 6). ClusterActiveNodeTests runs a real two-node cluster whose age order and address order diverge, and covers role scoping, role preference and the gate. The non-Up exclusions (Leaving/WeaklyUp/ Joining) and the restart-incarnation UniqueAddress comparison are verified by reading the code only: no test exercises them. That is a test gap, not a defect. There is one new Low finding: the AkkaActiveNodeGate can only be scoped to a single role, while the probe takes an ordered role-preference list. So the documented "gate and probe can never disagree" guarantee does not hold for the fused-node configuration that OtOpcUa actually runs (Health-007). It is latent, because no family app uses AkkaActiveNodeGate today.

2026-06-01 baseline (5f75cd4). The library is small, cohesive, and well-documented, and it tracks the normalized spec closely: the three-tier endpoint convention, the canonical JSON writer, the IActiveNodeGate seam, the configurable AkkaClusterStatusPolicy presets, and the role-filtered ActiveNodeHealthCheck all match the contract. The package split is clean — Akka and EF Core stay out of the core package, so MxGateway can take core only (category 8: no leakage). The decision logic is factored into pure, exhaustively table-tested functions (AkkaClusterStatusPolicy, ActiveNodeDecision), and the test suite (58 tests) exercises the public surface through a real TestServer / real SQLite / real probe delegates rather than mocks.

The findings cluster around error-handling completeness in the health-check probes (category 4), the highest-risk area for this library: a probe that throws past the IHealthCheck boundary, or that returns a status the spec says it should not, degrades the value of every consuming app's /health/* endpoints. The two Akka checks and the gRPC check do not guard the cluster-state / probe call the way the spec requires (return Degraded on inaccessible cluster) or the way the sibling DatabaseHealthCheck does (catch-all → Unhealthy). The framework's HealthCheckService catches escaping exceptions, so none of these crash an endpoint — that caps their severity at Medium — but they produce a result that disagrees with the documented contract. The remainder are Low: a JSON shape nuance (description key omitted vs. emitted-null), recommended-tag drift in XML docs, a return-value footgun in MapZbHealth, and missing GenerateDocumentationFile so the (otherwise excellent) XML docs do not ship in the nupkgs.

Checklist coverage

2026-11-09 re-review at 91fb2b4:

#	Category	Examined	Notes
1	Correctness & logic bugs	☑	OldestUpMember/SelfIsActive/ResolveActiveRole are correct for ties, non-Up states, role scoping, the no-Up-member case (Unhealthy) and restart incarnations (UniqueAddress). Gate/probe scope mismatch → Health-007.
2	Public API surface & compatibility	☑	0.3.0 is a deliberate, documented behaviour break (standby 503, role-lacking node Unhealthy). ActiveNodeDecision was removed but was internal. New public ClusterActiveNode and ActiveNodeHealthCheckOptions are sealed/static and nullable-correct. The gate offers a single role where the probe offers RolePreference → Health-007.
3	Concurrency & thread safety	☑	Checks and gate hold no mutable state. Membership is read from a single ClusterReadView snapshot per call. No issues found.
4	Error handling & resilience	☑	Health-001/002 fixes hold: cluster access is wrapped and maps to Degraded (OCE excluded); the gate swallows to false. No issues found.
5	Security & secret handling	☑	Endpoints are anonymous by design. The data payload carries node addresses, roles and counts only. No issues found.
6	Performance & resource management	☑	One linear member scan per probe. No allocations of note and nothing disposable. No issues found.
7	Spec & shared-contract adherence	☑	Three tiers, status strings and JSON envelope match SPEC §3. The oldest-Up rule matches both Akka apps (OtOpcUa RedundancyStateActor and ScadaBridge ActiveNodeEvaluator delegate to it). No issues found.
8	Packaging, dependencies & project layout	☑	Version 0.3.0; Akka stays out of the core package. The NU1510 warning (explicit HealthChecks.Abstractions reference is now prunable) is cosmetic. No issues found.
9	Testing coverage	☑	76 tests pass, including real-cluster ClusterActiveNodeTests (age order diverging from address order), startup safety, role resolution and the data writer shape. Untested: the Leaving/WeaklyUp/Joining exclusion and same-address restart incarnations (correct by code reading). A container-built AddCheck<ActiveNodeHealthCheck> was probed in the scratchpad and resolves correctly (Degraded with no ActorSystem).
10	Documentation & XML docs	☑	Thorough and accurate, apart from the gate's "can never disagree" claim (Health-007).

2026-06-01 baseline checklist (5f75cd4)

#	Category	Examined	Notes
1	Correctness & logic bugs	☑	Tier predicates, policy presets, and ActiveNodeDecision matrices are correct and match the spec tables. No logic defects found.
2	Public API surface & compatibility	☑	Health-005. Surface is minimal, sealed, well-named; nullable annotations correct; no internal types leak (ActiveNodeDecision is internal).
3	Concurrency & thread safety	☑	Options are mutable POCOs but per-registration; checks hold no shared mutable state; cluster-state reads are snapshot reads; static writer options are immutable. No issues found.
4	Error handling & resilience	☑	Health-001, Health-002. Akka checks don't guard cluster-state access; gRPC check lets non-Rpc/non-OCE exceptions escape.
5	Security & secret handling	☑	No secrets/PII. Exception objects attached to HealthCheckResult (standard); endpoints AllowAnonymous by design. No issues found.
6	Performance & resource management	☑	DatabaseHealthCheck is pool-safe and eagerly releases the timeout timer; scopes/contexts disposed via await using; channel not owned (correctly not disposed). No issues found.
7	Spec & shared-contract adherence	☑	Health-001, Health-003, Health-004. Endpoint convention + status strings otherwise match §3.
8	Packaging, dependencies & project layout	☑	Health-006. Dependency split correct; central versions; net10.0; correct package ids.
9	Testing coverage	☑	Strong: TestServer tier tests, real SQLite EF tests, table-driven policy/decision tests, gRPC probe + timeout + cancellation. Gaps noted inline in findings, not raised separately.
10	Documentation & XML docs	☑	Health-004, Health-006. Public API XML docs are thorough and accurate; the issues are recommended-tag drift and docs not being packed.

Findings

Health-001 — Akka health checks throw (instead of returning Degraded) when cluster state is inaccessible

Severity	Medium
Category	Error handling & resilience
Status	Resolved
Location	ZB.MOM.WW.Health/src/ZB.MOM.WW.Health.Akka/AkkaClusterHealthCheck.cs:45, ZB.MOM.WW.Health/src/ZB.MOM.WW.Health.Akka/ActiveNodeHealthCheck.cs:106

Description

Both Akka checks guard only the null ActorSystem case: _serviceProvider.GetService<ActorSystem>() returns null → Degraded. But once a non-null ActorSystem is resolved they call Cluster.Get(system) and read .SelfMember / .State.Leader with no try/catch. The spec is explicit that this path must be Degraded, not an exception:

"in both modes, if the ActorSystem is not yet ready or cluster state is inaccessible (e.g. during startup), the check returns Degraded (startup-safety rule)." — SPEC.md §2.2 note.

Cluster.Get(system) throws (ConfigurationException) when the Akka.Cluster extension is not configured on the resolved ActorSystem, and SelfMember can throw during the window where the ActorSystem object exists but the cluster has not finished initialising — exactly the startup race the spec calls out. The ActorSystem being present-but-not-yet-clustered is a very real ordering in a Microsoft.Extensions.Hosting app (the ActorSystem is registered in DI before Cluster has joined).

The escaping exception is caught by ASP.NET Core's HealthCheckService, which records the entry as Unhealthy — so the endpoint does not crash. But Unhealthy on the ready tier returns 503 and removes the node from load-balancing, whereas the spec's intended Degraded returns 200 (still in rotation). A transient startup window therefore yanks a healthy-but-starting node out of rotation, the opposite of the "startup-safe" guarantee the XML docs advertise.

Recommendation

Wrap the cluster-state read in a try/catch and return HealthCheckResult.Degraded(...) on failure, so both the null-ActorSystem and inaccessible-cluster paths converge on Degraded as the spec requires: resolve Cluster.Get(system) and read membership inside a try, catching the cluster extension's startup exceptions and mapping them to Degraded with a "cluster not yet ready" description.

Resolution

Resolved in 544a6dd, 2026-06-01 — wrapped the Cluster.Get(system)/SelfMember/leader reads in AkkaClusterHealthCheck and ActiveNodeHealthCheck in try/catch (Exception when not OCE) returning Degraded("Akka cluster state not yet accessible."); tests add a plain non-clustered ActorSystem to assert Degraded instead of the escaping ConfigurationException.

Health-002 — GrpcDependencyHealthCheck lets non-RpcException/non-OperationCanceledException errors escape

Severity	Medium
Category	Error handling & resilience
Status	Resolved
Location	ZB.MOM.WW.Health/src/ZB.MOM.WW.Health/GrpcDependencyHealthCheck.cs:54

Description

CheckHealthAsync catches only RpcException and OperationCanceledException. Any other exception thrown by the probe escapes the method. This is reachable on both paths:

• Default probe: GrpcChannel.ConnectAsync can throw exceptions other than RpcException — e.g. InvalidOperationException (channel/socket misuse, shutdown) or HttpRequestException / SocketException surfaced from the transport before a gRPC status is produced.
• Custom probe (GrpcDependencyOptions.Probe): a caller-supplied delegate (e.g. a grpc.health.v1.Health/Check RPC built on a non-gRPC client) can throw anything.

The XML docs and shared contract describe the result as Unhealthy "when it returns false, throws an RpcException, or times out" — they do not promise to handle arbitrary exceptions, and the code does not. By contrast the sibling DatabaseHealthCheck has a catch-all catch (Exception ex) → Unhealthy("Database connection failed.", ex) (DatabaseHealthCheck.cs:78), so the two probes are asymmetric. As with Health-001 the framework's HealthCheckService records the escaping exception as Unhealthy, so no endpoint crashes — but the dependency-named, controlled description ("{name} probe failed…") is lost, and the library violates its own "must not throw past the IHealthCheck boundary" intent.

Recommendation

Add a trailing catch (Exception ex) returning HealthCheckResult.Unhealthy($"{name} probe failed: {ex.Message}", ex), after the existing OCE/Rpc handlers (so external cancellation still propagates). This brings the gRPC probe in line with DatabaseHealthCheck and the documented contract.

Resolution

Resolved in 544a6dd, 2026-06-01 — added a trailing catch (Exception ex) to GrpcDependencyHealthCheck.CheckHealthAsync returning Unhealthy("{name} probe failed: {ex.Message}", ex) after the OCE/Rpc external-cancellation handlers; new test asserts a probe throwing InvalidOperationException maps to Unhealthy.

Health-003 — Null description is omitted from the JSON body instead of emitted as null

Severity	Low
Category	Spec & shared-contract adherence
Status	Resolved
Location	ZB.MOM.WW.Health/src/ZB.MOM.WW.Health/ZbHealthWriter.cs:32

Description

SerializerOptions sets DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, so a check that produces no description renders an entry with no description key at all: { "status": "Healthy", "durationMs": 0.1 }. The spec models description as a string? that "may be null" and both the canonical example (SPEC.md §3) and the HealthChecks.UI.Client shape the writer claims to mirror emit the key with a null value rather than dropping it. Consumers/dashboards parsing the body and reading entries.<name>.description must therefore handle a missing property, not just a null one. The deviation is undocumented and untested — ResponseWriterTests only asserts the present-description case.

Low because the aggregate status/HTTP-code contract that orchestrators key on is unaffected; only the descriptive sub-field shape drifts.

Recommendation

Either remove WhenWritingNull (so description: null is emitted, matching the spec example and the UI-client shape), or document explicitly in the writer's XML docs and SPEC.md §3 that absent descriptions are omitted, and add a test pinning the chosen behaviour.

Resolution

Resolved in 544a6dd, 2026-06-01 — removed DefaultIgnoreCondition = WhenWritingNull from ZbHealthWriter.SerializerOptions so a null description renders as "description": null (matching the spec example / UI-client shape); writer XML doc now states the key is always present, and a new ResponseWriterTests case pins present-and-null.

Health-004 — XML docs recommend the active tag for ready-tier probes, contradicting the spec

Severity	Low
Category	Documentation & XML docs
Status	Resolved
Location	ZB.MOM.WW.Health/src/ZB.MOM.WW.Health.Akka/AkkaClusterHealthCheck.cs:11, ZB.MOM.WW.Health/src/ZB.MOM.WW.Health/GrpcDependencyHealthCheck.cs:21

Description

AkkaClusterHealthCheck's summary says "Register to the ZbHealthTags.Ready tag (recommended [ready, active])", and GrpcDependencyHealthCheck likewise recommends tagging [ready, active]. The spec is unambiguous that both probes belong to the ready tier only: §2.2 "Registered to the ready tag" (Akka cluster) and §2.4 "Registered to the ready tag" (gRPC dependency). The active tier is reserved by §1 / §2.3 for the leader/active-singleton probe (ActiveNodeHealthCheck); putting cluster-membership or gRPC-reachability checks on the active tier pollutes the active-node tier with non-leadership concerns and contradicts the convergence convention the library exists to enforce.

Recommendation

Bring the XML-doc recommendations in line with the spec — recommend ZbHealthTags.Ready only for both probes (drop the active suggestion); or, if the dual-tag recommendation is intentional, reconcile it back into SPEC.md §2.2/§2.4 so code and spec agree.

Resolution

Resolved in 544a6dd, 2026-06-01 — updated the XML summaries on AkkaClusterHealthCheck and GrpcDependencyHealthCheck to recommend ZbHealthTags.Ready only (dropped the [ready, active] suggestion), aligning the docs with SPEC §2.2/§2.4.

Health-005 — MapZbHealth returns only the readiness builder, silently dropping conventions for the active/live tiers

Severity	Low
Category	Public API surface & compatibility
Status	Resolved
Location	ZB.MOM.WW.Health/src/ZB.MOM.WW.Health/ZbHealthEndpointExtensions.cs:64

Description

MapZbHealth maps three endpoints but returns the IEndpointConventionBuilder for /health/ready only. Any convention a caller chains onto the result — .RequireHost(...), .RequireAuthorization(), .WithMetadata(...), .CacheOutput() — applies to the readiness endpoint alone; the active and liveness endpoints silently do not receive it. The behaviour is documented in the XML <returns>, so this is a deliberate, disclosed trade-off rather than a bug, but it is a real footgun: the most natural reading of endpoints.MapZbHealth().RequireHost("…") is "gate all three health endpoints", and the result type gives the caller no signal that two of the three are excluded.

Recommendation

Consider returning a composite builder that fans conventions out to all three endpoints (collect the three IEndpointConventionBuilders and wrap them) so chained conventions behave as a caller expects. If the single-builder return is kept for simplicity, strengthen the <returns> remark to a warning and note it in the README's MapZbHealth example.

Resolution

Resolved in 544a6dd, 2026-06-01 — MapZbHealth now returns a private CompositeEndpointConventionBuilder that fans Add/Finally to all three endpoint builders (readiness, active, liveness); <returns> docs updated, and a new TierMappingTests case proves .RequireHost(...) gates all three tiers.

Health-006 — XML documentation is not emitted into the packed nupkgs

Severity	Low
Category	Packaging, dependencies & project layout
Status	Resolved
Location	ZB.MOM.WW.Health/Directory.Build.props:3, ZB.MOM.WW.Health/src/ZB.MOM.WW.Health/ZB.MOM.WW.Health.csproj:3

Description

Every public type and member in all three packages carries thorough, accurate XML documentation, but no project (and neither Directory.Build.props nor the three .csproj files) sets <GenerateDocumentationFile>true</GenerateDocumentationFile>. As a result dotnet pack produces nupkgs that contain the DLLs but no .xml doc files, so consumers of ZB.MOM.WW.Health / .Akka / .EntityFrameworkCore get no IntelliSense summaries or parameter docs for the API. For a shared library whose value proposition is a documented common surface across three apps, this discards the documentation effort at the package boundary. (It also means the compiler does not enforce the "public members are documented" invariant via CS1591.)

Recommendation

Set <GenerateDocumentationFile>true</GenerateDocumentationFile> once in Directory.Build.props so all three packable projects emit and pack their XML docs. Optionally pair with targeted <NoWarn> if any CS1591 warnings surface on currently-undocumented members.

Resolution

Resolved in 544a6dd, 2026-06-01 — set <GenerateDocumentationFile>true</GenerateDocumentationFile> (plus NoWarn=CS1591 for test/non-packed members) in Directory.Build.props; verified dotnet pack -c Release now ships lib/net10.0/*.xml in all three nupkgs.

Health-007 — AkkaActiveNodeGate cannot express the probe's RolePreference, so gate and /health/active can disagree despite the documented guarantee

Severity	Low
Priority	P3
Category	Public API surface & compatibility
Status	Resolved
Location	ZB.MOM.WW.Health/src/ZB.MOM.WW.Health.Akka/AkkaActiveNodeGate.cs:46 (ctor string? role), :14-17 (doc claim), vs ZB.MOM.WW.Health/src/ZB.MOM.WW.Health.Akka/ActiveNodeHealthCheck.cs:114-115

Description

In 0.3.0 ActiveNodeHealthCheck resolves its scope from an ordered RolePreference list through ClusterActiveNode.ResolveActiveRole(self.Roles, …), so a fused node answers for admin and a driver-only node answers for driver. AkkaActiveNodeGate accepts only a single fixed string? role and calls ClusterActiveNode.SelfIsActive(cluster, _role) directly. Its XML remarks still promise that "an endpoint gated by this type and the /health/active probe an orchestrator routes by can never disagree about which node is in charge". That holds only when the probe is configured with zero or one role. For the multi-role preference (the exact configuration OtOpcUa's HealthEndpoints uses, [admin, driver]) no gate configuration matches the probe on every node.

Failure scenario

An app wires /health/active with RolePreference = ["admin","driver"] and gates its RequireActiveNode() endpoints with new AkkaActiveNodeGate(sp, "admin"). On a driver-only pair, the oldest driver member answers /health/active 200, so the load balancer routes traffic to it. The gate evaluates the admin scope: the node has no admin role, so SelfIsActive returns false and every gated request gets 503 from ActiveNodeGateEndpointFilter. The pair serves nothing. Using new AkkaActiveNodeGate(sp) (unscoped) instead makes the gate follow the oldest member of the whole mesh, which on a mixed mesh can be a different node from the probe's role-scoped answer. Today this is latent: no family app constructs AkkaActiveNodeGate, because ScadaBridge and OtOpcUa use their own evaluators over ClusterActiveNode.

Recommendation

Give AkkaActiveNodeGate an ActiveNodeHealthCheckOptions constructor, or factor the "resolve role, then SelfIsActive" step into one shared ClusterActiveNode method that both the probe and the gate call, so they share one scope definition. Until then, qualify the remarks: the guarantee holds only when both are configured with the same single role or both unscoped.

Resolution

Resolved 2026-11-09 in the commit tagged (scadaproj#48) (Health 0.3.1, unpublished) — added AkkaActiveNodeGate.ForOptions(sp, ActiveNodeHealthCheckOptions) and a shared ClusterActiveNode.SelfIsActiveForPreference(cluster, rolePreference) (resolve the first preferred role the node carries, then SelfIsActive; a node with none of the roles is never active), so the gate and /health/active share one scope definition. Chose a static factory over a second public constructor so an existing new AkkaActiveNodeGate(sp, null) call is not made ambiguous. Class remarks qualified. Tests Gate_ForOptions_AgreesWithTheProbeOnEveryNode_ForAMultiRolePreference and ..._NodeCarryingNoPreferredRole_IsNeverActive_AndEmptyPreferenceIsUnscoped (new API, so they cannot compile without the fix).