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

fix(health-monitoring): resolve HealthMonitoring-015 — nullable LastReportReceivedAt

Browse Source 
A heartbeat-registered site that has never sent a full report now has
LastReportReceivedAt = null instead of the year-0001 sentinel. TimestampDisplay
accepts DateTimeOffset? and renders null as a placeholder ('awaiting first
report') rather than a ~2000-year-stale date. Cross-module: HealthMonitoring +
CentralUI.
This commit is contained in:
Joseph Doherty
2026-05-17 05:43:05 -04:00
parent 7da303d7bb
commit e55bd46ca1
7 changed files with 137 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -8,7 +8,7 @@
| Last reviewed | 2026-05-17 |
| Reviewer | claude-agent |
| Commit reviewed | `39d737e` |
| Open findings | 1 |
| Open findings | 0 |
## Summary
@@ -44,8 +44,9 @@ crash-class**. They are residual polish items rather than behaviour regressions:
an inaccurate offline-check-interval comment (HealthMonitoring-013), unvalidated
`HealthMonitoringOptions` intervals that crash the hosted service on
misconfiguration (HealthMonitoring-014), a heartbeat-only registered site left
with a year-0001 `LastReportReceivedAt` that the UI's staleness display must
special-case (HealthMonitoring-015), and `CollectReport` reading
with a year-0001 `LastReportReceivedAt` that the UI's staleness display would
otherwise render verbatim (HealthMonitoring-015 — resolved via a coordinated
HealthMonitoring + CentralUI change), and `CollectReport` reading
`DateTimeOffset.UtcNow` directly instead of the module's now-standard injected
`TimeProvider` (HealthMonitoring-016). The module remains small, readable, and
broadly faithful to the design intent.
@@ -684,7 +685,7 @@ intervals and the `CentralOfflineTimeout < OfflineTimeout` case.
|--|--|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.HealthMonitoring/CentralHealthAggregator.cs:122-130`, `src/ScadaLink.HealthMonitoring/SiteHealthState.cs:27` |
**Description**
@@ -712,25 +713,25 @@ nullable option is safer and matches the existing `LatestReport` treatment.
**Resolution**
_Unresolved — left Open; requires a coordinated cross-module change._ Root cause
confirmed against source: `CentralHealthAggregator.MarkHeartbeat` registers an
unknown site with `LastReportReceivedAt = default` (`0001-01-01`), and the audit of
the single UI reader (`src/ScadaLink.CentralUI/Components/Pages/Monitoring/Health.razor:74`)
confirms the bug is live — it passes `state.LastReportReceivedAt` straight into the
`TimestampDisplay` component, which would render the year-0001 sentinel verbatim for
a heartbeat-only site. The finding's recommended fix — change the field to
`DateTimeOffset?` — is the correct one, but it is a **breaking public-API change**:
`TimestampDisplay.Value` is a non-nullable `DateTimeOffset` parameter, so making
`LastReportReceivedAt` nullable stops `Health.razor` compiling, and the
`CentralUI` module is explicitly outside this task's edit scope. The only fix that
stays module-local would be to stamp the heartbeat time into `LastReportReceivedAt`,
which is semantically dishonest (the field documents "time the latest full report
was processed") and would simply move the bug rather than fix it. The correct
resolution therefore needs the HealthMonitoring change (`DateTimeOffset?`) **and** a
matching `CentralUI` change (a null-checked `TimestampDisplay` or an "awaiting first
report" placeholder) applied together — a design decision spanning two modules.
**Called out for follow-up: open as a coordinated HealthMonitoring + CentralUI work
item.**
Resolved 2026-05-17. Root cause confirmed against source — `CentralHealthAggregator.MarkHeartbeat`
registered a heartbeat-only site with `LastReportReceivedAt = default` (`0001-01-01`),
which `Health.razor:74` passed verbatim into `TimestampDisplay`, rendering a
~2000-year-stale timestamp. Applied the finding's recommended cross-module fix.
**HealthMonitoring:** `SiteHealthState.LastReportReceivedAt` is now `DateTimeOffset?`,
and `MarkHeartbeat`'s unknown-site registration sets it to `null` — "no report yet"
is now an explicit nullable state, consistent with the already-nullable `LatestReport`.
`ProcessReport` continues to set a real instant. **CentralUI:** `TimestampDisplay.Value`
now accepts `DateTimeOffset?` and renders `null` as a plain `text-muted` placeholder
(default "never", configurable via a new `NullText` parameter); existing non-null
callers (`AuditLog`, `EventLogs`, `Deployments`) are unaffected by the implicit
widening. `Health.razor`'s "Last report" cell passes `NullText="awaiting first report"`
so a heartbeat-only site reads cleanly. Regression tests:
`CentralHealthAggregatorTests.MarkHeartbeat_RegistersUnknownSite_WithNullLastReportReceivedAt`
and `ProcessReport_SetsLastReportReceivedAt_ForHeartbeatRegisteredSite` (HealthMonitoring —
the first would not compile against the pre-fix non-nullable field), and
`TimestampDisplayTests.Render_NullValue_ShowsNeverPlaceholder`,
`Render_NullValue_DoesNotRenderYear0001Sentinel`, `Render_NonNullValue_ShowsFormattedTimestamp`
(CentralUI).
### HealthMonitoring-016 — `SiteHealthCollector.CollectReport` reads `DateTimeOffset.UtcNow` directly instead of an injected `TimeProvider`