diff --git a/code-reviews/HealthMonitoring/findings.md b/code-reviews/HealthMonitoring/findings.md index 073cd89..67bdaba 100644 --- a/code-reviews/HealthMonitoring/findings.md +++ b/code-reviews/HealthMonitoring/findings.md @@ -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` diff --git a/src/ScadaLink.CentralUI/Components/Pages/Monitoring/Health.razor b/src/ScadaLink.CentralUI/Components/Pages/Monitoring/Health.razor index 9cb497e..4d829cc 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Monitoring/Health.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Monitoring/Health.razor @@ -71,7 +71,7 @@ @siteName@(isCentral ? "" : $" ({siteId})") - Last report: + Last report: | Last heartbeat: | Seq: @state.LastSequenceNumber diff --git a/src/ScadaLink.CentralUI/Components/Shared/TimestampDisplay.razor b/src/ScadaLink.CentralUI/Components/Shared/TimestampDisplay.razor index b66ed9c..bf5b4df 100644 --- a/src/ScadaLink.CentralUI/Components/Shared/TimestampDisplay.razor +++ b/src/ScadaLink.CentralUI/Components/Shared/TimestampDisplay.razor @@ -1,8 +1,20 @@ -@* Displays a UTC DateTimeOffset formatted for display. Tooltip shows UTC value. *@ +@* Displays a UTC DateTimeOffset formatted for display. Tooltip shows UTC value. + A null Value renders as a plain "never" placeholder — used for timestamps that + have not happened yet (e.g. a heartbeat-only site with no full report). *@ -@Value.LocalDateTime.ToString(Format) +@if (Value is { } value) +{ + @value.LocalDateTime.ToString(Format) +} +else +{ + @NullText +} @code { - [Parameter, EditorRequired] public DateTimeOffset Value { get; set; } + [Parameter, EditorRequired] public DateTimeOffset? Value { get; set; } [Parameter] public string Format { get; set; } = "yyyy-MM-dd HH:mm:ss"; + + /// Text shown when is null. + [Parameter] public string NullText { get; set; } = "never"; } diff --git a/src/ScadaLink.HealthMonitoring/CentralHealthAggregator.cs b/src/ScadaLink.HealthMonitoring/CentralHealthAggregator.cs index f02986e..6d0f22e 100644 --- a/src/ScadaLink.HealthMonitoring/CentralHealthAggregator.cs +++ b/src/ScadaLink.HealthMonitoring/CentralHealthAggregator.cs @@ -118,12 +118,14 @@ public class CentralHealthAggregator : BackgroundService, ICentralHealthAggregat if (!_siteStates.TryGetValue(siteId, out var existing)) { // Unknown site — register it as online, awaiting its first - // full report. LatestReport stays null until ProcessReport runs. + // full report. LatestReport and LastReportReceivedAt both stay + // null until ProcessReport runs — "no report yet" is an explicit + // nullable state, not a year-0001 sentinel the UI must special-case. var registered = new SiteHealthState { SiteId = siteId, LatestReport = null, - LastReportReceivedAt = default, + LastReportReceivedAt = null, LastHeartbeatAt = receivedAt, LastSequenceNumber = 0, IsOnline = true diff --git a/src/ScadaLink.HealthMonitoring/SiteHealthState.cs b/src/ScadaLink.HealthMonitoring/SiteHealthState.cs index 0774105..460d905 100644 --- a/src/ScadaLink.HealthMonitoring/SiteHealthState.cs +++ b/src/ScadaLink.HealthMonitoring/SiteHealthState.cs @@ -21,10 +21,13 @@ public sealed record SiteHealthState public SiteHealthReport? LatestReport { get; init; } /// - /// Time the latest full was processed. - /// Used by the UI to surface report staleness during failover. + /// Time the latest full was processed, or + /// null if the site is known only via heartbeats and has not yet sent + /// a report. Used by the UI to surface report staleness during failover; + /// the null case must be rendered as "no report yet" rather than as a + /// timestamp (a default sentinel would display as year-0001). /// - public DateTimeOffset LastReportReceivedAt { get; init; } + public DateTimeOffset? LastReportReceivedAt { get; init; } /// /// Time the most recent signal of any kind (full report OR heartbeat) was diff --git a/tests/ScadaLink.CentralUI.Tests/Shared/TimestampDisplayTests.cs b/tests/ScadaLink.CentralUI.Tests/Shared/TimestampDisplayTests.cs new file mode 100644 index 0000000..470afad --- /dev/null +++ b/tests/ScadaLink.CentralUI.Tests/Shared/TimestampDisplayTests.cs @@ -0,0 +1,50 @@ +using Bunit; +using ScadaLink.CentralUI.Components.Shared; + +namespace ScadaLink.CentralUI.Tests.Shared; + +/// +/// Regression tests for HealthMonitoring-015. A heartbeat-only registered site has +/// a null LastReportReceivedAt ("no full report yet"). The health +/// dashboard passes that value straight into , so the +/// component's Value must accept DateTimeOffset? and render a +/// null as a human-readable placeholder ("never") instead of the +/// DateTimeOffset.MinValue year-0001 sentinel. Non-null callers must keep +/// rendering the formatted timestamp exactly as before. +/// +public class TimestampDisplayTests : BunitContext +{ + [Fact] + public void Render_NonNullValue_ShowsFormattedTimestamp() + { + var value = new DateTimeOffset(2026, 5, 17, 14, 30, 45, TimeSpan.Zero); + + var cut = Render(parameters => parameters + .Add(p => p.Value, (DateTimeOffset?)value) + .Add(p => p.Format, "HH:mm:ss")); + + var span = cut.Find("span"); + Assert.Equal(value.LocalDateTime.ToString("HH:mm:ss"), span.TextContent.Trim()); + Assert.Contains("2026-05-17 14:30:45 UTC", span.GetAttribute("title")!); + } + + [Fact] + public void Render_NullValue_ShowsNeverPlaceholder() + { + var cut = Render(parameters => parameters + .Add(p => p.Value, (DateTimeOffset?)null) + .Add(p => p.Format, "HH:mm:ss")); + + Assert.Contains("never", cut.Markup, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void Render_NullValue_DoesNotRenderYear0001Sentinel() + { + var cut = Render(parameters => parameters + .Add(p => p.Value, (DateTimeOffset?)null)); + + // The year-0001 DateTimeOffset.MinValue sentinel must never reach the UI. + Assert.DoesNotContain("0001", cut.Markup); + } +} diff --git a/tests/ScadaLink.HealthMonitoring.Tests/CentralHealthAggregatorTests.cs b/tests/ScadaLink.HealthMonitoring.Tests/CentralHealthAggregatorTests.cs index 22b3395..5f3d17f 100644 --- a/tests/ScadaLink.HealthMonitoring.Tests/CentralHealthAggregatorTests.cs +++ b/tests/ScadaLink.HealthMonitoring.Tests/CentralHealthAggregatorTests.cs @@ -240,6 +240,43 @@ public class CentralHealthAggregatorTests Assert.Equal(now, state.LastHeartbeatAt); } + /// + /// Regression test for HealthMonitoring-015. A heartbeat-only registered site + /// has never processed a full report, so + /// must be null — not the DateTimeOffset.MinValue (year-0001) + /// sentinel that the UI would otherwise render as a ~2000-year-stale timestamp. + /// The "no report yet" signal must be an explicit nullable state, consistent + /// with . + /// + [Fact] + public void MarkHeartbeat_RegistersUnknownSite_WithNullLastReportReceivedAt() + { + _aggregator.MarkHeartbeat("site-new", _timeProvider.GetUtcNow()); + + var state = _aggregator.GetSiteState("site-new"); + Assert.NotNull(state); + Assert.Null(state.LastReportReceivedAt); + } + + /// + /// Regression test for HealthMonitoring-015. Once a full report is processed + /// for a heartbeat-registered site, + /// becomes a real (non-null) instant. + /// + [Fact] + public void ProcessReport_SetsLastReportReceivedAt_ForHeartbeatRegisteredSite() + { + _aggregator.MarkHeartbeat("site-new", _timeProvider.GetUtcNow()); + _timeProvider.Advance(TimeSpan.FromSeconds(5)); + var reportTime = _timeProvider.GetUtcNow(); + + _aggregator.ProcessReport(MakeReport("site-new", 1)); + + var state = _aggregator.GetSiteState("site-new"); + Assert.NotNull(state); + Assert.Equal(reportTime, state.LastReportReceivedAt); + } + [Fact] public void MarkHeartbeat_KeepsSiteOnline_BetweenReports() {