dohertj2/scadalink-design
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(health-monitoring): resolve HealthMonitoring-004,006,010,011,012 — heartbeat-doc accuracy, testable sequence seeding, logged failures, dead-code removal

Browse Source
This commit is contained in:
Joseph Doherty
2026-05-16 22:14:23 -04:00
parent e57ccd78b7
commit 2d7ac5b57f
9 changed files with 260 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files

77
code-reviews/HealthMonitoring/findings.md
View File

@@ -8,7 +8,7 @@
| Last reviewed | 2026-05-16 |
| Reviewer | claude-agent |
| Commit reviewed | `9c60592` |
| Open findings | 5 |
| Open findings | 0 |
## Summary
@@ -187,7 +187,7 @@ updates and no torn snapshots. No further code change was required for this find
|--|--|
| Severity | Low |
| Category | Documentation & comments |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.HealthMonitoring/CentralHealthAggregator.cs:146-148`, `src/ScadaLink.HealthMonitoring/SiteHealthState.cs:21`, `src/ScadaLink.HealthMonitoring/ICentralHealthAggregator.cs:16` |
**Description**
@@ -207,7 +207,17 @@ comments, ideally referencing the owning component rather than restating a magic
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `pending`). Documentation-only — no regression test is
meaningful. Verified the authoritative cadence against the Communication module:
`SiteCommunicationActor.PreStart` schedules the application-level heartbeat to central
at `CommunicationOptions.TransportHeartbeatInterval`, which defaults to **5 seconds**
(`CommunicationOptions.cs:49`). The stale "~2s" in `ICentralHealthAggregator.MarkHeartbeat`
was corrected; all three XML docs (`ICentralHealthAggregator.MarkHeartbeat`,
`SiteHealthState.LastHeartbeatAt`, `CentralHealthAggregator.CheckForOfflineSites`) now
state the single authoritative ~5s figure and reference the owning component
(`Cluster Infrastructure / SiteCommunicationActor` —
`CommunicationOptions.TransportHeartbeatInterval`) rather than restating a bare magic
number, so readers can reason about the 60s offline grace.
### HealthMonitoring-005 — Central self-report site can flap offline; no heartbeat grace like real sites
@@ -259,7 +269,7 @@ offline after 10 minutes) verify the behaviour.
|--|--|
| Severity | Low |
| Category | Design-document adherence |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.HealthMonitoring/HealthReportSender.cs:28`, `src/ScadaLink.HealthMonitoring/CentralHealthReportLoop.cs:32` |
**Description**
@@ -283,7 +293,20 @@ clock dependency is explicit.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `pending`). The `HealthReportSender` XML summary was
rewritten to describe the real strategy — sequence numbers are monotonic and
restart-resetting but explicitly **not** zero/one-based; they are seeded with the
current Unix epoch (ms) so a freshly-active node always sorts after the prior active.
Both `HealthReportSender` and `CentralHealthReportLoop` now accept an optional
`TimeProvider` (defaulting to `TimeProvider.System`) and derive the seed via
`GetUtcNow().ToUnixTimeMilliseconds()` in the constructor body instead of reading
`DateTimeOffset.UtcNow` at field initialization, so the seeding is deterministically
testable and the clock dependency is explicit. `CentralHealthReportLoop` gained a
`CurrentSequenceNumber` test accessor mirroring `HealthReportSender`. Regression tests
`HealthReportSenderTests.SequenceNumberSeed_UsesInjectedTimeProvider` and
`CentralHealthReportLoopTests.SequenceNumberSeed_UsesInjectedTimeProvider` assert the
seed equals the injected provider's Unix-ms instant (these would not compile against
the pre-fix code, which had no `TimeProvider` parameter).
### HealthMonitoring-007 — Heartbeats for not-yet-registered sites are silently dropped
@@ -430,7 +453,7 @@ The HealthMonitoring test suite now stands at 47 passing tests (was 30).
|--|--|
| Severity | Low |
| Category | Error handling & resilience |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.HealthMonitoring/HealthReportSender.cs:70-87` |
**Description**
@@ -451,7 +474,21 @@ the failure so persistent degradation is diagnosable; avoid swallowing
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `pending`). All three bare `catch { }` blocks in
`HealthReportSender.ExecuteAsync` (cluster-nodes refresh, parked-message-count query,
S&F buffer-depth query) were changed to `catch (Exception ex)` and now emit a
`LogWarning(ex, ...)` naming the failed operation and the site, so a persistent
degradation (broken S&F SQLite store, always-throwing `GetClusterNodes()`) is
diagnosable instead of silently shipping stale/zero metrics. On the `OperationCanceledException`
concern: verified the inner calls cannot raise OCE from cancellation —
`IClusterNodeProvider.GetClusterNodes()` is synchronous and takes no token, and
`StoreAndForwardStorage.GetParkedMessageCountAsync()`/`GetBufferDepthByCategoryAsync()`
take no `CancellationToken` parameter, so the only cancellation path is the outer loop's
`PeriodicTimer.WaitForNextTickAsync(stoppingToken)`, which is unaffected. Regression
test `HealthReportSenderTests.ClusterNodeRefreshFailure_IsLoggedNotSwallowed` injects a
throwing `IClusterNodeProvider`, asserts the loop still ships reports, and asserts a
warning carrying the `InvalidOperationException` is logged — confirmed to fail against
the pre-fix bare `catch { }` (logged-entry collection was empty).
### HealthMonitoring-011 — `AddHealthMonitoringActors` is a dead no-op placeholder
@@ -459,7 +496,7 @@ _Unresolved._
|--|--|
| Severity | Low |
| Category | Code organization & conventions |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.HealthMonitoring/ServiceCollectionExtensions.cs:42-46` |
**Description**
@@ -476,15 +513,23 @@ planned, track it in the design doc instead of a half-method.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `pending`). Verified a codebase-wide search — the
`AddHealthMonitoringActors` no-op extension has no production callers (only references
were in the code-review docs themselves). The HealthMonitoring module deliberately
contains no actors (transport is abstracted behind `IHealthReportTransport`; actor-side
wiring lives in the Communication module), so there is no genuine Phase-4 actor work for
this method to host. The dead placeholder was removed from `ServiceCollectionExtensions`
so a caller can no longer be misled into believing actor wiring exists. No regression
test is meaningful for a deleted no-op; removal is verified by the module continuing to
build and all 50 tests passing.
### HealthMonitoring-012 — `SiteHealthState.LatestReport` initialized to `null!`, misrepresenting the contract
| | |
|--|--|
| Severity | Low |
| Severity | Low — re-triaged: already resolved as a side-effect of HealthMonitoring-002. |
| Category | Documentation & comments |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.HealthMonitoring/SiteHealthState.cs:11` |
**Description**
@@ -506,4 +551,12 @@ for HealthMonitoring-007.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `pending`). Re-triaged: verified against the current
source — the root cause was already eliminated by the HealthMonitoring-002 /
HealthMonitoring-007 fixes. `SiteHealthState.LatestReport` is already declared
`SiteHealthReport? LatestReport { get; init; }` (the recommendation's "make it properly
nullable" option) with an XML doc explaining the `null` case ("known only via heartbeats,
has not yet sent a report"). A codebase-wide search confirms no `null!` suppression
remains anywhere in `src/ScadaLink.HealthMonitoring`. This is exactly the change
HealthMonitoring-002 made when converting `SiteHealthState` to an immutable record, so
the contract is now honest and no further code change was required.