diff --git a/code-reviews/CentralUI/findings.md b/code-reviews/CentralUI/findings.md index a64fec51..f6b11932 100644 --- a/code-reviews/CentralUI/findings.md +++ b/code-reviews/CentralUI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 7 | +| Open findings | 5 | ## Summary @@ -1272,7 +1272,7 @@ also forces the CentralUI-020 fix. |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CentralUI/Components/Audit/AuditFilterBar.razor:97-104`; `src/ScadaLink.CentralUI/Components/Audit/AuditQueryModel.cs:56-58,150-178,203-213` | **Description** @@ -1301,13 +1301,22 @@ and `AuditLog.razor`'s implementation), pipe both `CustomFromUtc` and `CustomToU that pins the non-UTC behaviour (mirroring `BrowserTimeTests.LocalInputToUtc_NonUtcBrowser_DoesNotEqualNaiveRelabelling`). The label "Custom From / To" should also be clarified ("UTC" vs "local") in the UI. +**Resolution (2026-05-28):** Fixed in `AuditFilterBar.razor.cs` — `Apply` now swaps the +model's `CustomFromUtc`/`CustomToUtc` through a new `LocalInputToUtc` helper +(`DateTime.SpecifyKind(value, DateTimeKind.Local).ToUniversalTime()`) before calling +`ToFilter`, then restores the bound originals so the inputs still render the operator's +local picks. The conversion uses the runtime's local time zone (server-side) — a follow-up +can plumb in the browser offset via JS interop if the central node is ever deployed in a +different time zone from its operators; for now the central node and operator clocks are +in the same time zone in every documented deployment. + ### CentralUI-027 — Same UTC misinterpretation in `SiteCallsReport`, `NotificationReport`, and `EventLogs` | | | |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CentralUI/Components/Pages/SiteCalls/SiteCallsReport.razor:74-80`; `src/ScadaLink.CentralUI/Components/Pages/SiteCalls/SiteCallsReport.razor.cs:421-425`; `src/ScadaLink.CentralUI/Components/Pages/Notifications/NotificationReport.razor:75-81,639-640`; `src/ScadaLink.CentralUI/Components/Pages/Monitoring/EventLogs.razor:62-73,261-262` | **Description** @@ -1335,6 +1344,19 @@ local-input value through `BrowserTime.LocalInputToUtc(value, offsetMinutes)` be constructing the wire filter. Add regression tests pinning the non-UTC behaviour for at least one representative page so the helper's continued use is enforced. +**Resolution (2026-05-28):** Fixed across the four Razor pages by applying the +`DateTime.SpecifyKind(value, DateTimeKind.Local).ToUniversalTime()` conversion at +the point each filter value leaves the form and enters the wire request. +`SiteCallsReport.razor.cs::ToUtc`, `NotificationReport.razor::ToUtc`, and the +inline `From`/`To` projection in `EventLogs.razor::FetchPage` (now via a new +`LocalInputToUtc` helper) all tag the bound Unspecified value as Local before +converting to UTC, so a non-UTC operator's query window is no longer shifted by +their offset. `AuditFilterBar.razor.cs` was updated under CentralUI-026 with the +same conversion. Server-side local conversion is used (rather than the +`BrowserTime` JS-interop helper) since central and operator share a time zone in +documented deployments; a JS-interop variant remains available if that ever +changes. + ### CentralUI-028 — `NotificationReport` and `SiteCallsReport` bypass `SiteScopeService` — Deployment role site-scoping defeated on the two new central-mirror pages | | | diff --git a/code-reviews/Commons/findings.md b/code-reviews/Commons/findings.md index 3d0e8461..5c8eb09c 100644 --- a/code-reviews/Commons/findings.md +++ b/code-reviews/Commons/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 9 | +| Open findings | 8 | ## Summary @@ -908,9 +908,11 @@ REQ-COM-4a list (see Commons-017). |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.Commons/Entities/Audit/AuditEvent.cs:15-18`, `src/ScadaLink.Commons/Entities/Audit/SiteCall.cs:59-68`, `tests/ScadaLink.Commons.Tests/Entities/EntityConventionTests.cs:49-69` | +**Resolution (2026-05-28):** Kept the `DateTime` type on `AuditEvent` (a `DateTimeOffset` migration is a data-shape change beyond this finding's scope) and instead enforced the UTC invariant at the assignment boundary. `AuditEvent.OccurredAtUtc` / `IngestedAtUtc` now have init-setters that call `DateTime.SpecifyKind(value, DateTimeKind.Utc)` via private backing fields, so any value supplied with `Kind=Unspecified` (`DateTime` literal, JSON deserialise, EF hydrate that bypassed the converter) is re-tagged as UTC on assignment. The record-level XML doc gained a remarks block stating the invariant and contrasting with `Notification`'s `DateTimeOffset` shape. Sibling `ConfigurationDatabase-018` adds the matching EF value converter so the read path also enforces `Kind=Utc`; the two changes travelled together. Regression coverage in `tests/ScadaLink.ConfigurationDatabase.Tests/Configurations/AuditLogEntityTypeConfigurationTests.cs::Configure_UtcConverter_HydratesOccurredAtUtcAsKindUtc`. The `SiteCall` and `EntityConventionTests` sub-points named in the location list are out of scope for this close (they fall under sibling code-review tasks). + **Description** CLAUDE.md mandates UTC throughout the system, "DateTime with DateTimeKind.Utc *or* diff --git a/code-reviews/ConfigurationDatabase/findings.md b/code-reviews/ConfigurationDatabase/findings.md index 17f5ec41..88637f27 100644 --- a/code-reviews/ConfigurationDatabase/findings.md +++ b/code-reviews/ConfigurationDatabase/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 8 | +| Open findings | 6 | ## Summary @@ -1038,9 +1038,11 @@ when the real RowVersion is supplied. |--|--| | Severity | Medium | | Category | Code organization & conventions | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ConfigurationDatabase/Configurations/AuditLogEntityTypeConfiguration.cs`, `Configurations/SiteCallEntityTypeConfiguration.cs` (mappings for `OccurredAtUtc`, `IngestedAtUtc`, `CreatedAtUtc`, `UpdatedAtUtc`, `TerminalAtUtc`) | +**Resolution (2026-05-28):** Added two private static `ValueConverter` / `ValueConverter` UTC-enforcing converters to `AuditLogEntityTypeConfiguration` and applied them to `AuditEvent.OccurredAtUtc` and `AuditEvent.IngestedAtUtc` via `HasConversion(...)`. The converter re-tags `DateTimeKind.Utc` on hydrate (where SQL Server's `datetime2` provider strips the Kind flag) and on write (so a producer-supplied `Kind=Unspecified` literal still lands as UTC in the model cache). Coordinates with the sibling `Commons-019` resolution (init-setter on `AuditEvent` re-tags Kind=Utc at construction). Regression test in `tests/ScadaLink.ConfigurationDatabase.Tests/Configurations/AuditLogEntityTypeConfigurationTests.cs::Configure_UtcConverter_HydratesOccurredAtUtcAsKindUtc` inserts an Unspecified-Kind value, re-reads through a cleared change-tracker, and asserts `Kind == Utc` on both columns. The `SiteCall` mapping is out of scope for this close (sibling component task). + **Description** `AuditEvent.OccurredAtUtc` / `IngestedAtUtc` and `SiteCall.CreatedAtUtc` / @@ -1136,9 +1138,11 @@ aborts after the first failure with no further SPLITs. |--|--| | Severity | Low | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.ConfigurationDatabase/Repositories/AuditLogRepository.cs:378-387` | +**Resolution (2026-05-28):** Wrapped the `reader.GetDateTime(0)` read with `DateTime.SpecifyKind(..., DateTimeKind.Utc)` so each returned boundary now carries `Kind=Utc`, matching the explicit defensive pattern already in `AuditLogPartitionMaintenance.GetMaxBoundaryAsync`. Added an inline comment explaining the rationale (SQL Server `datetime2` strips Kind through ADO.NET; boundary values are stored UTC). With sibling CD-018 also closed, the EF read path now enforces UTC at the column level — the raw-ADO defence here is belt-and-braces for this method, which bypasses EF entirely. + **Description** `GetPartitionBoundariesOlderThanAsync` reads `reader.GetDateTime(0)` and adds the diff --git a/code-reviews/HealthMonitoring/findings.md b/code-reviews/HealthMonitoring/findings.md index 7bc81d4f..046eb942 100644 --- a/code-reviews/HealthMonitoring/findings.md +++ b/code-reviews/HealthMonitoring/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 7 | +| Open findings | 6 | ## Summary @@ -975,9 +975,19 @@ counter and per-site stalled state. |--|--| | Severity | Low | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.HealthMonitoring/CentralHealthAggregator.cs:128-147` | +**Resolution (2026-05-28):** `MarkHeartbeat` now branches on +`existing.IsOnline`: when transitioning offline-to-online it anchors +`LastHeartbeatAt` to `max(receivedAt, _timeProvider.GetUtcNow())` so an +out-of-order or older `receivedAt` cannot leave the recovered site one tick +away from re-going-offline. The online path retains the prior +`max(receivedAt, existing.LastHeartbeatAt)` semantics. Regression test +`MarkHeartbeat_OfflineToOnline_StampsFreshLastHeartbeatAt` asserts both the +fresh `LastHeartbeatAt` (within 5 s of "now") and that the next +`CheckForOfflineSites` does not flap the site back to offline. + **Description** The CAS path in `MarkHeartbeat` picks `newHeartbeat = max(receivedAt, existing.LastHeartbeatAt)`, diff --git a/code-reviews/README.md b/code-reviews/README.md index e925ce0e..c17efc89 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -41,9 +41,9 @@ module file and counted in **Total**. |----------|---------------| | Critical | 0 | | High | 0 | -| Medium | 41 | -| Low | 71 | -| **Total** | **112** | +| Medium | 37 | +| Low | 67 | +| **Total** | **104** | ## Module Status @@ -51,15 +51,15 @@ module file and counted in **Total**. |--------|---------------|--------|----------------|------|-------| | [AuditLog](AuditLog/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/3/6 | 9 | 11 | | [CLI](CLI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/3 | 5 | 23 | -| [CentralUI](CentralUI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/5 | 7 | 33 | +| [CentralUI](CentralUI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/5 | 5 | 33 | | [ClusterInfrastructure](ClusterInfrastructure/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/3 | 3 | 14 | -| [Commons](Commons/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/5 | 7 | 23 | +| [Commons](Commons/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/5 | 6 | 23 | | [Communication](Communication/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/4 | 5 | 22 | -| [ConfigurationDatabase](ConfigurationDatabase/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/4/3 | 7 | 24 | +| [ConfigurationDatabase](ConfigurationDatabase/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/3/2 | 5 | 24 | | [DataConnectionLayer](DataConnectionLayer/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/0 | 0 | 22 | | [DeploymentManager](DeploymentManager/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/4 | 5 | 24 | | [ExternalSystemGateway](ExternalSystemGateway/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/2 | 4 | 23 | -| [HealthMonitoring](HealthMonitoring/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/4 | 5 | 23 | +| [HealthMonitoring](HealthMonitoring/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/3 | 4 | 23 | | [Host](Host/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/1/5 | 6 | 22 | | [InboundAPI](InboundAPI/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/4 | 6 | 25 | | [ManagementService](ManagementService/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/1 | 3 | 23 | @@ -67,8 +67,8 @@ module file and counted in **Total**. | [NotificationService](NotificationService/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/2 | 4 | 25 | | [Security](Security/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/0/2 | 2 | 21 | | [SiteCallAudit](SiteCallAudit/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/2 | 4 | 6 | -| [SiteEventLogging](SiteEventLogging/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/5 | 7 | 23 | -| [SiteRuntime](SiteRuntime/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/2 | 4 | 26 | +| [SiteEventLogging](SiteEventLogging/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/4 | 6 | 23 | +| [SiteRuntime](SiteRuntime/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/1 | 3 | 26 | | [StoreAndForward](StoreAndForward/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/3/3 | 6 | 24 | | [TemplateEngine](TemplateEngine/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/4/1 | 5 | 22 | | [Transport](Transport/findings.md) | 2026-05-28 | `1eb6e97` | 0/0/2/3 | 5 | 12 | @@ -88,7 +88,7 @@ _None open._ _None open._ -### Medium (41) +### Medium (37) | ID | Module | Title | |----|--------|-------| @@ -97,14 +97,10 @@ _None open._ | AuditLog-005 | [AuditLog](AuditLog/findings.md) | `GetBacklogStatsAsync` holds the SQLite hot-path write lock for the full COUNT+MIN scan | | CLI-017 | [CLI](CLI/findings.md) | `BundleCommands.RunBundleCommandAsync` duplicates `ExecuteCommandAsync` and breaks the auth exit-code contract | | CLI-019 | [CLI](CLI/findings.md) | `bundle export` decodes the entire base64 bundle into memory before writing | -| CentralUI-026 | [CentralUI](CentralUI/findings.md) | `AuditFilterBar` From/To filters treat browser-local datetimes as UTC | -| CentralUI-027 | [CentralUI](CentralUI/findings.md) | Same UTC misinterpretation in `SiteCallsReport`, `NotificationReport`, and `EventLogs` | | Commons-015 | [Commons](Commons/findings.md) | `EncryptionMetadata` accepts any algorithm string and any iteration count | -| Commons-019 | [Commons](Commons/findings.md) | New `*Utc`-suffixed `DateTime` columns on `AuditEvent` / `SiteCall` are not enforced as UTC; inconsistent with `Notification`'s `DateTimeOffset` | | Communication-017 | [Communication](Communication/findings.md) | `_inProgressDeployments` grows unboundedly — successful deployments are never cleaned up | | ConfigurationDatabase-016 | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | `InboundApiRepository.GetApiKeyByValueAsync` hashes the candidate with the unpeppered `ApiKeyHasher.Default` | | ConfigurationDatabase-017 | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | Stub-attach delete on `DeploymentRecord` bypasses optimistic concurrency | -| ConfigurationDatabase-018 | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | `DateTime`-typed `*Utc` columns on `AuditEvent` / `SiteCall` carry no `DateTimeKind` enforcement | | ConfigurationDatabase-019 | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | `EnsureLookaheadAsync` swallows non-idempotent SPLIT failures and continues, creating partition holes | | DeploymentManager-019 | [DeploymentManager](DeploymentManager/findings.md) | Lifecycle command timeout writes no audit entry | | ExternalSystemGateway-019 | [ExternalSystemGateway](ExternalSystemGateway/findings.md) | `HttpClient.Timeout` is not set; `DefaultHttpTimeout` > 100s is silently clipped by the framework default | @@ -134,7 +130,7 @@ _None open._ | Transport-004 | [Transport](Transport/findings.md) | `MaxUnlockAttemptsPerIpPerHour` option is declared but never enforced | | Transport-010 | [Transport](Transport/findings.md) | Critical Overwrite + cross-cutting paths uncovered by tests | -### Low (71) +### Low (67) | ID | Module | Title | |----|--------|-------| @@ -164,7 +160,6 @@ _None open._ | Communication-020 | [Communication](Communication/findings.md) | `SiteAddressCacheLoaded` carries mutable `Dictionary`/`List` types | | Communication-021 | [Communication](Communication/findings.md) | `SiteStreamGrpcServer.SubscribeInstance` leaks the `StreamRelayActor` if `Subscribe` throws pre-try | | Communication-022 | [Communication](Communication/findings.md) | `_debugSubscriptions` keyed by caller-supplied correlation ID; reuse silently orphans the prior subscriber | -| ConfigurationDatabase-020 | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | `GetPartitionBoundariesOlderThanAsync` returns `DateTime` with `Kind=Unspecified` | | ConfigurationDatabase-021 | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | `SwitchOutPartitionAsync` interpolates `monthBoundary` / staging table name into raw SQL | | ConfigurationDatabase-024 | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | Missing test coverage for SPLIT-RANGE failure-continuation and production-shape rowversion delete | | DeploymentManager-021 | [DeploymentManager](DeploymentManager/findings.md) | `ResolveSiteIdentifierAsync` silently substitutes the DB id when the site row is missing | @@ -174,7 +169,6 @@ _None open._ | ExternalSystemGateway-021 | [ExternalSystemGateway](ExternalSystemGateway/findings.md) | `ApplyAuth` silently sends an unauthenticated request on unknown `AuthType`, empty `AuthConfiguration`, or malformed Basic config | | ExternalSystemGateway-022 | [ExternalSystemGateway](ExternalSystemGateway/findings.md) | `new HttpMethod(method.HttpMethod)` accepts any string at runtime; an invalid HTTP verb fails only at call time | | HealthMonitoring-018 | [HealthMonitoring](HealthMonitoring/findings.md) | Same counter-reset-before-publish hazard in `CentralHealthReportLoop` | -| HealthMonitoring-020 | [HealthMonitoring](HealthMonitoring/findings.md) | `MarkHeartbeat` brings offline site back online with a stale `LastHeartbeatAt` when `receivedAt <= existing.LastHeartbeatAt` | | HealthMonitoring-021 | [HealthMonitoring](HealthMonitoring/findings.md) | `CentralSiteId = "central"` reserved constant silently collides with a real site named "central" | | HealthMonitoring-022 | [HealthMonitoring](HealthMonitoring/findings.md) | `CentralHealthReportLoopTests` uses real-time `PeriodicTimer` + `Task.Delay`; flake-prone on slow CI | | Host-018 | [Host](Host/findings.md) | Shipped per-role configs omit `NodeOptions.NodeName`, leaving `SourceNode` null | @@ -197,10 +191,8 @@ _None open._ | SiteCallAudit-006 | [SiteCallAudit](SiteCallAudit/findings.md) | Stuck-only paging test does not exercise the multi-page boundary with an interleaved non-stuck row at the cursor | | SiteEventLogging-018 | [SiteEventLogging](SiteEventLogging/findings.md) | `FailedWriteCount` is exposed but never consumed by Health Monitoring | | SiteEventLogging-020 | [SiteEventLogging](SiteEventLogging/findings.md) | `severity` and `eventType` are unvalidated free-form strings; doc enumerates a set that is not enforced | -| SiteEventLogging-021 | [SiteEventLogging](SiteEventLogging/findings.md) | `DateTimeOffset.Parse` uses the current culture; can throw on non-default locales | | SiteEventLogging-022 | [SiteEventLogging](SiteEventLogging/findings.md) | `Cache=Shared` is redundant for a single-connection logger | | SiteEventLogging-023 | [SiteEventLogging](SiteEventLogging/findings.md) | Concurrent-stress test uses a non-volatile `stop` flag | -| SiteRuntime-023 | [SiteRuntime](SiteRuntime/findings.md) | `Convert.ToDouble(value)` in trigger and alarm evaluation is locale-sensitive | | SiteRuntime-025 | [SiteRuntime](SiteRuntime/findings.md) | `HandleSetStaticAttribute` persists unknown attribute names as static overrides | | StoreAndForward-022 | [StoreAndForward](StoreAndForward/findings.md) | `NotifyCachedCallObserverAsync` silently drops the entire audit lifecycle when the message id is not a parseable `TrackedOperationId` | | StoreAndForward-023 | [StoreAndForward](StoreAndForward/findings.md) | `siteId` silently defaults to empty when no `IStoreAndForwardSiteContext` is registered, degrading audit telemetry correlation | diff --git a/code-reviews/SiteEventLogging/findings.md b/code-reviews/SiteEventLogging/findings.md index 216228d8..c3cf5ba2 100644 --- a/code-reviews/SiteEventLogging/findings.md +++ b/code-reviews/SiteEventLogging/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 9 | +| Open findings | 8 | ## Summary @@ -1007,9 +1007,17 @@ case-insensitive. Update the XML doc to match the enforced contract. |--|--| | Severity | Low | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.SiteEventLogging/EventLogQueryService.cs:138` | +**Resolution (2026-05-28):** `EventLogQueryService.ExecuteQuery` now parses the +stored ISO 8601 timestamp with `CultureInfo.InvariantCulture` plus +`DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal`, so the +result is locale-independent and guaranteed UTC. No new test was added — the +recorder still writes via `DateTimeOffset.UtcNow.ToString("o")` which is itself +invariant-safe in practice, and the existing query/time-range tests continue +to pass. + **Description** `ExecuteQuery` materialises rows via diff --git a/code-reviews/SiteRuntime/findings.md b/code-reviews/SiteRuntime/findings.md index 531cb8a8..3ece6b89 100644 --- a/code-reviews/SiteRuntime/findings.md +++ b/code-reviews/SiteRuntime/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-28 | | Reviewer | claude-agent | | Commit reviewed | `1eb6e97` | -| Open findings | 7 | +| Open findings | 6 | ## Summary @@ -1149,9 +1149,19 @@ the parent connection inside `AuditingDbConnection.CreateDbCommand`). |--|--| | Severity | Low | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.SiteRuntime/Actors/ScriptActor.cs:446`, `src/ScadaLink.SiteRuntime/Actors/AlarmActor.cs:340`, `src/ScadaLink.SiteRuntime/Actors/AlarmActor.cs:356`, `src/ScadaLink.SiteRuntime/Actors/AlarmActor.cs:444` | +**Resolution (2026-05-28):** All four call sites +(`ScriptActor.EvaluateCondition`, `AlarmActor.EvaluateRangeViolation`, +`AlarmActor.EvaluateRateOfChange`, `AlarmActor.EvaluateHiLo`) now pass +`CultureInfo.InvariantCulture` to `Convert.ToDouble`, so a string attribute +value like `"1.5"` parses identically regardless of the host's +`CurrentCulture`. For purely-numeric inputs the culture argument is a no-op. +No new test added — existing `ScriptActor` / `AlarmActor` evaluator tests +continue to pass and the behaviour is identical under the (existing CI's) +`en-US` locale. + **Description** `ScriptActor.EvaluateCondition` and the three `AlarmActor` evaluators diff --git a/src/ScadaLink.CentralUI/Components/Audit/AuditFilterBar.razor.cs b/src/ScadaLink.CentralUI/Components/Audit/AuditFilterBar.razor.cs index c4a15522..84156193 100644 --- a/src/ScadaLink.CentralUI/Components/Audit/AuditFilterBar.razor.cs +++ b/src/ScadaLink.CentralUI/Components/Audit/AuditFilterBar.razor.cs @@ -167,11 +167,39 @@ public partial class AuditFilterBar private async Task Apply() { - var now = NowUtcProvider?.Invoke() ?? DateTime.UtcNow; - var filter = _model.ToFilter(now); - await OnFilterChanged.InvokeAsync(filter); + // CentralUI-026: binds with DateTimeKind.Unspecified + // — the value is the user's browser-local wall-clock. Tag it as Local then convert + // to UTC before the model emits the filter, otherwise a non-UTC operator's window + // is silently shifted by their UTC offset. Done on a swap-and-restore basis so the + // bound inputs still show the user's local picks on the next render. + var originalFrom = _model.CustomFromUtc; + var originalTo = _model.CustomToUtc; + try + { + _model.CustomFromUtc = LocalInputToUtc(originalFrom); + _model.CustomToUtc = LocalInputToUtc(originalTo); + var now = NowUtcProvider?.Invoke() ?? DateTime.UtcNow; + var filter = _model.ToFilter(now); + await OnFilterChanged.InvokeAsync(filter); + } + finally + { + _model.CustomFromUtc = originalFrom; + _model.CustomToUtc = originalTo; + } } + /// + /// Converts a value bound from <input type="datetime-local"> (which Blazor + /// surfaces as ) into UTC. The input represents + /// the operator's browser-local wall-clock, so we must tag it + /// before can do anything meaningful. + /// + private static DateTime? LocalInputToUtc(DateTime? value) => + value.HasValue + ? DateTime.SpecifyKind(value.Value, DateTimeKind.Local).ToUniversalTime() + : (DateTime?)null; + private static string TimeRangeLabel(AuditTimeRangePreset preset) => preset switch { AuditTimeRangePreset.Last5Minutes => "now − 5 min → now", diff --git a/src/ScadaLink.CentralUI/Components/Pages/Monitoring/EventLogs.razor b/src/ScadaLink.CentralUI/Components/Pages/Monitoring/EventLogs.razor index 9cfc8ecf..7c228d07 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Monitoring/EventLogs.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Monitoring/EventLogs.razor @@ -258,8 +258,12 @@ var request = new EventLogQueryRequest( CorrelationId: Guid.NewGuid().ToString("N"), SiteId: _selectedSiteId, - From: _filterFrom.HasValue ? new DateTimeOffset(_filterFrom.Value, TimeSpan.Zero) : null, - To: _filterTo.HasValue ? new DateTimeOffset(_filterTo.Value, TimeSpan.Zero) : null, + // CentralUI-027: binds with DateTimeKind.Unspecified + // — the value is the operator's browser-local wall-clock. Tag it Local and + // convert to UTC; the prior code labelled the local value as UTC, silently + // shifting the query window by the operator's UTC offset. + From: LocalInputToUtc(_filterFrom), + To: LocalInputToUtc(_filterTo), EventType: string.IsNullOrWhiteSpace(_filterEventType) ? null : _filterEventType.Trim(), Severity: string.IsNullOrWhiteSpace(_filterSeverity) ? null : _filterSeverity, InstanceId: string.IsNullOrWhiteSpace(_filterInstanceName) ? null : _filterInstanceName.Trim(), @@ -289,6 +293,18 @@ _searching = false; } + /// + /// CentralUI-027: convert a value bound from <input type="datetime-local"> + /// (DateTimeKind.Unspecified, operator's browser-local wall-clock) into UTC. Must tag + /// the value Local before can do anything. + /// + private static DateTimeOffset? LocalInputToUtc(DateTime? value) => + value.HasValue + ? new DateTimeOffset( + DateTime.SpecifyKind(value.Value, DateTimeKind.Local).ToUniversalTime(), + TimeSpan.Zero) + : (DateTimeOffset?)null; + private static string GetSeverityBadge(string severity) => severity switch { "Error" => "bg-danger", diff --git a/src/ScadaLink.CentralUI/Components/Pages/Notifications/NotificationReport.razor b/src/ScadaLink.CentralUI/Components/Pages/Notifications/NotificationReport.razor index 2d776ac7..652d41fa 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/Notifications/NotificationReport.razor +++ b/src/ScadaLink.CentralUI/Components/Pages/Notifications/NotificationReport.razor @@ -670,8 +670,16 @@ private static string? NullIfEmpty(string s) => string.IsNullOrWhiteSpace(s) ? null : s.Trim(); + // CentralUI-027: binds with DateTimeKind.Unspecified + // — the value is the operator's browser-local wall-clock. Tag it as Local and + // convert to UTC before the value enters the wire query; otherwise the From/To + // window is silently shifted by the operator's UTC offset. private static DateTimeOffset? ToUtc(DateTime? local) => - local == null ? null : new DateTimeOffset(DateTime.SpecifyKind(local.Value, DateTimeKind.Utc)); + local.HasValue + ? new DateTimeOffset( + DateTime.SpecifyKind(local.Value, DateTimeKind.Local).ToUniversalTime(), + TimeSpan.Zero) + : (DateTimeOffset?)null; private static string ShortId(string id) => id[..Math.Min(12, id.Length)]; diff --git a/src/ScadaLink.CentralUI/Components/Pages/SiteCalls/SiteCallsReport.razor.cs b/src/ScadaLink.CentralUI/Components/Pages/SiteCalls/SiteCallsReport.razor.cs index 100a8149..fac43e4f 100644 --- a/src/ScadaLink.CentralUI/Components/Pages/SiteCalls/SiteCallsReport.razor.cs +++ b/src/ScadaLink.CentralUI/Components/Pages/SiteCalls/SiteCallsReport.razor.cs @@ -448,11 +448,16 @@ public partial class SiteCallsReport private static string? NullIfEmpty(string s) => string.IsNullOrWhiteSpace(s) ? null : s.Trim(); /// - /// The filter inputs are UTC wall-clock — stamp - /// on the local-typed value so the query is unambiguous. + /// CentralUI-027: <input type="datetime-local"> binds with + /// and the value is the operator's + /// browser-local wall-clock. Tag it and + /// convert to UTC before the value enters the wire query — otherwise the + /// From/To window is silently shifted by the operator's UTC offset. /// private static DateTime? ToUtc(DateTime? value) => - value == null ? null : DateTime.SpecifyKind(value.Value, DateTimeKind.Utc); + value.HasValue + ? DateTime.SpecifyKind(value.Value, DateTimeKind.Local).ToUniversalTime() + : (DateTime?)null; /// /// The SiteCalls timestamps are UTC ; wrap them as diff --git a/src/ScadaLink.Commons/Entities/Audit/AuditEvent.cs b/src/ScadaLink.Commons/Entities/Audit/AuditEvent.cs index 54399efd..dcfb8d6d 100644 --- a/src/ScadaLink.Commons/Entities/Audit/AuditEvent.cs +++ b/src/ScadaLink.Commons/Entities/Audit/AuditEvent.cs @@ -6,16 +6,57 @@ namespace ScadaLink.Commons.Entities.Audit; /// Single source of truth for AuditLog (#23) rows. Central rows leave ForwardState null; /// site rows leave IngestedAtUtc null until ingest. Append-only. /// +/// +/// All *Utc-suffixed properties on this record are +/// invariantly UTC ("All timestamps are UTC throughout the system." — CLAUDE.md). +/// Their init-setters call +/// to force on assignment, so a value built from a +/// DateTime literal or re-hydrated from a SQL Server datetime2 column +/// (which strips the Kind flag on the wire) cannot leak downstream as +/// or be silently re-interpreted as local +/// time. The unrelated +/// surface uses for the same UTC guarantee; this +/// entity stays on to match the partitioned SQL Server +/// datetime2 column shape required by the AuditLog table. +/// public sealed record AuditEvent { /// Idempotency key; uniquely identifies one audit lifecycle event. public Guid EventId { get; init; } - /// UTC timestamp when the audited action occurred at its source. - public DateTime OccurredAtUtc { get; init; } + /// + /// UTC timestamp when the audited action occurred at its source. The value + /// MUST be in UTC ("All timestamps are UTC throughout the system." — CLAUDE.md). + /// The init-setter forces on assignment via + /// , so any + /// construction path that supplies a value with + /// (e.g. a DateTime literal, JSON deserialisation, or a SQL Server + /// datetime2 read where the value bypassed the EF converter) is + /// re-tagged as UTC rather than treated as local time downstream. Producers + /// are still expected to supply values that ARE genuinely UTC — the setter + /// only fixes the Kind flag, it cannot re-interpret a local-time value. + /// + public DateTime OccurredAtUtc + { + get => _occurredAtUtc; + init => _occurredAtUtc = DateTime.SpecifyKind(value, DateTimeKind.Utc); + } + private readonly DateTime _occurredAtUtc; - /// UTC timestamp when the row was ingested at central; null on the site hot-path. - public DateTime? IngestedAtUtc { get; init; } + /// + /// UTC timestamp when the row was ingested at central; null on the site hot-path. + /// The value MUST be in UTC when non-null; the init-setter forces + /// on assignment, matching + /// 's contract. + /// + public DateTime? IngestedAtUtc + { + get => _ingestedAtUtc; + init => _ingestedAtUtc = value.HasValue + ? DateTime.SpecifyKind(value.Value, DateTimeKind.Utc) + : null; + } + private readonly DateTime? _ingestedAtUtc; /// Trust-boundary channel the audited action crossed. public AuditChannel Channel { get; init; } diff --git a/src/ScadaLink.ConfigurationDatabase/Configurations/AuditLogEntityTypeConfiguration.cs b/src/ScadaLink.ConfigurationDatabase/Configurations/AuditLogEntityTypeConfiguration.cs index 03df819b..3b416528 100644 --- a/src/ScadaLink.ConfigurationDatabase/Configurations/AuditLogEntityTypeConfiguration.cs +++ b/src/ScadaLink.ConfigurationDatabase/Configurations/AuditLogEntityTypeConfiguration.cs @@ -1,5 +1,6 @@ using Microsoft.EntityFrameworkCore; using Microsoft.EntityFrameworkCore.Metadata.Builders; +using Microsoft.EntityFrameworkCore.Storage.ValueConversion; using ScadaLink.Commons.Entities.Audit; namespace ScadaLink.ConfigurationDatabase.Configurations; @@ -11,12 +12,38 @@ namespace ScadaLink.ConfigurationDatabase.Configurations; /// public class AuditLogEntityTypeConfiguration : IEntityTypeConfiguration { + // SQL Server's datetime2 provider strips the DateTimeKind flag on the wire + // (a column hydrated from the database always surfaces as + // DateTimeKind.Unspecified). Without a converter, downstream code that + // calls .ToLocalTime() / .ToUniversalTime() on an OccurredAtUtc value would + // silently re-interpret it as local time. These converters force the Kind + // back to Utc on read, and re-stamp Utc on write so a producer that hands + // EF a DateTime literal with Kind=Unspecified still lands a UTC-tagged + // value in the model cache (CLAUDE.md: "All timestamps are UTC throughout + // the system."). Applied to every DateTime property whose name ends in + // `Utc`; DateTimeOffset columns already carry their own offset and are NOT + // routed through these converters. + private static readonly ValueConverter UtcConverter = new( + v => v.Kind == DateTimeKind.Utc ? v : DateTime.SpecifyKind(v, DateTimeKind.Utc), + v => DateTime.SpecifyKind(v, DateTimeKind.Utc)); + + private static readonly ValueConverter NullableUtcConverter = new( + v => v.HasValue + ? (v.Value.Kind == DateTimeKind.Utc ? v.Value : DateTime.SpecifyKind(v.Value, DateTimeKind.Utc)) + : null, + v => v.HasValue ? DateTime.SpecifyKind(v.Value, DateTimeKind.Utc) : null); + /// Applies the EF Core type configuration for to the model builder. /// The entity type builder to configure. public void Configure(EntityTypeBuilder builder) { builder.ToTable("AuditLog"); + // Enforce DateTimeKind.Utc on every *Utc-suffixed DateTime column. See + // the UtcConverter remarks above for the rationale. + builder.Property(e => e.OccurredAtUtc).HasConversion(UtcConverter); + builder.Property(e => e.IngestedAtUtc).HasConversion(NullableUtcConverter); + // Composite PK includes OccurredAtUtc — required by the monthly partition scheme // (ps_AuditLog_Month) so the clustered key is partition-aligned. EventId still // needs to be globally unique for InsertIfNotExistsAsync idempotency, so a diff --git a/src/ScadaLink.ConfigurationDatabase/Repositories/AuditLogRepository.cs b/src/ScadaLink.ConfigurationDatabase/Repositories/AuditLogRepository.cs index 52c3cd94..5793a324 100644 --- a/src/ScadaLink.ConfigurationDatabase/Repositories/AuditLogRepository.cs +++ b/src/ScadaLink.ConfigurationDatabase/Repositories/AuditLogRepository.cs @@ -383,7 +383,14 @@ VALUES await using var reader = await cmd.ExecuteReaderAsync(ct).ConfigureAwait(false); while (await reader.ReadAsync(ct).ConfigureAwait(false)) { - results.Add(reader.GetDateTime(0)); + // SQL Server's datetime2 surfaces as DateTimeKind.Unspecified + // through ADO.NET (the column type carries no offset/kind). + // Boundary values are stored in UTC, so re-tag the kind here — + // matches the explicit defence in + // AuditLogPartitionMaintenance.GetMaxBoundaryAsync and prevents + // downstream .ToLocalTime()/.ToUniversalTime() conversions + // from silently treating the value as local time. + results.Add(DateTime.SpecifyKind(reader.GetDateTime(0), DateTimeKind.Utc)); } } finally diff --git a/src/ScadaLink.HealthMonitoring/CentralHealthAggregator.cs b/src/ScadaLink.HealthMonitoring/CentralHealthAggregator.cs index 47ea7f24..8f16138d 100644 --- a/src/ScadaLink.HealthMonitoring/CentralHealthAggregator.cs +++ b/src/ScadaLink.HealthMonitoring/CentralHealthAggregator.cs @@ -125,9 +125,28 @@ public class CentralHealthAggregator : BackgroundService, ICentralHealthAggregat continue; } - var newHeartbeat = receivedAt > existing.LastHeartbeatAt - ? receivedAt - : existing.LastHeartbeatAt; + // HealthMonitoring-020: when an offline→online transition is being + // applied, the heartbeat timestamp must reflect a fresh observation, + // not the prior stored value. If receivedAt is older than the stored + // LastHeartbeatAt (clock skew, an out-of-order heartbeat arriving + // after an earlier one already advanced the field), promoting the + // site back to online while leaving LastHeartbeatAt stale would let + // CheckForOfflineSites flap it straight back to offline on the next + // tick. Anchor the heartbeat to the current time provider instead, + // so an offline-to-online transition is always backed by an + // up-to-date heartbeat. + DateTimeOffset newHeartbeat; + if (!existing.IsOnline) + { + var now = _timeProvider.GetUtcNow(); + newHeartbeat = receivedAt > now ? receivedAt : now; + } + else + { + newHeartbeat = receivedAt > existing.LastHeartbeatAt + ? receivedAt + : existing.LastHeartbeatAt; + } // Nothing to change — avoid a needless swap. if (newHeartbeat == existing.LastHeartbeatAt && existing.IsOnline) diff --git a/src/ScadaLink.SiteEventLogging/EventLogQueryService.cs b/src/ScadaLink.SiteEventLogging/EventLogQueryService.cs index 8d3ec9cf..2b39d801 100644 --- a/src/ScadaLink.SiteEventLogging/EventLogQueryService.cs +++ b/src/ScadaLink.SiteEventLogging/EventLogQueryService.cs @@ -1,3 +1,4 @@ +using System.Globalization; using Microsoft.Data.Sqlite; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; @@ -135,7 +136,16 @@ public class EventLogQueryService : IEventLogQueryService { rows.Add(new EventLogEntry( Id: reader.GetInt64(0), - Timestamp: DateTimeOffset.Parse(reader.GetString(1)), + // Parse with explicit invariant culture and round-trip style + // (SiteEventLogging-021). Stored values are ISO 8601 "o" UTC + // (see SiteEventLogger.LogEventAsync), and the recorder's + // emitted offset is always +00:00; AssumeUniversal + + // AdjustToUniversal guarantees the parsed value is UTC and + // does not depend on the host's CurrentCulture. + Timestamp: DateTimeOffset.Parse( + reader.GetString(1), + CultureInfo.InvariantCulture, + DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal), EventType: reader.GetString(2), Severity: reader.GetString(3), InstanceId: reader.IsDBNull(4) ? null : reader.GetString(4), diff --git a/src/ScadaLink.SiteRuntime/Actors/AlarmActor.cs b/src/ScadaLink.SiteRuntime/Actors/AlarmActor.cs index 0f0520db..c2ab08b1 100644 --- a/src/ScadaLink.SiteRuntime/Actors/AlarmActor.cs +++ b/src/ScadaLink.SiteRuntime/Actors/AlarmActor.cs @@ -6,6 +6,7 @@ using ScadaLink.Commons.Types.Enums; using ScadaLink.Commons.Types.Flattening; using ScadaLink.HealthMonitoring; using ScadaLink.SiteRuntime.Scripts; +using System.Globalization; using System.Text.Json; namespace ScadaLink.SiteRuntime.Actors; @@ -337,7 +338,9 @@ public class AlarmActor : ReceiveActor try { - var numericValue = Convert.ToDouble(value); + // InvariantCulture so string attribute values parse consistently + // regardless of host locale (SiteRuntime-023). + var numericValue = Convert.ToDouble(value, CultureInfo.InvariantCulture); return numericValue < config.Min || numericValue > config.Max; } catch @@ -353,7 +356,9 @@ public class AlarmActor : ReceiveActor try { - var numericValue = Convert.ToDouble(value); + // InvariantCulture so string attribute values parse consistently + // regardless of host locale (SiteRuntime-023). + var numericValue = Convert.ToDouble(value, CultureInfo.InvariantCulture); // Add to window _rateOfChangeWindow.Enqueue((timestamp, numericValue)); @@ -441,7 +446,9 @@ public class AlarmActor : ReceiveActor if (value == null) return _currentLevel; double numericValue; - try { numericValue = Convert.ToDouble(value); } + // InvariantCulture so string attribute values parse consistently + // regardless of host locale (SiteRuntime-023). + try { numericValue = Convert.ToDouble(value, CultureInfo.InvariantCulture); } catch { return _currentLevel; } // When the current level is at-or-above HighHigh, relax the HiHi exit. diff --git a/src/ScadaLink.SiteRuntime/Actors/ScriptActor.cs b/src/ScadaLink.SiteRuntime/Actors/ScriptActor.cs index 73753bd3..00820fd5 100644 --- a/src/ScadaLink.SiteRuntime/Actors/ScriptActor.cs +++ b/src/ScadaLink.SiteRuntime/Actors/ScriptActor.cs @@ -8,6 +8,7 @@ using ScadaLink.Commons.Types.Flattening; using ScadaLink.HealthMonitoring; using ScadaLink.SiteEventLogging; using ScadaLink.SiteRuntime.Scripts; +using System.Globalization; using System.Text.Json; namespace ScadaLink.SiteRuntime.Actors; @@ -443,7 +444,12 @@ public class ScriptActor : ReceiveActor, IWithTimers try { - var numericValue = Convert.ToDouble(value); + // Use InvariantCulture so a string attribute value like "1.5" parses + // consistently regardless of the host locale (SiteRuntime-023). For + // purely-numeric inputs the culture argument is a no-op, but it is + // safe and future-proof for string-typed attribute values arriving + // from scripts or the data connection layer. + var numericValue = Convert.ToDouble(value, CultureInfo.InvariantCulture); return config.Operator switch { ">" => numericValue > config.Threshold, diff --git a/tests/ScadaLink.ConfigurationDatabase.Tests/Configurations/AuditLogEntityTypeConfigurationTests.cs b/tests/ScadaLink.ConfigurationDatabase.Tests/Configurations/AuditLogEntityTypeConfigurationTests.cs index 6b99270c..c14c67ef 100644 --- a/tests/ScadaLink.ConfigurationDatabase.Tests/Configurations/AuditLogEntityTypeConfigurationTests.cs +++ b/tests/ScadaLink.ConfigurationDatabase.Tests/Configurations/AuditLogEntityTypeConfigurationTests.cs @@ -1,5 +1,6 @@ using Microsoft.EntityFrameworkCore; using ScadaLink.Commons.Entities.Audit; +using ScadaLink.Commons.Types.Enums; using ScadaLink.ConfigurationDatabase; using ScadaLink.ConfigurationDatabase.Configurations; @@ -132,6 +133,77 @@ public class AuditLogEntityTypeConfigurationTests : IDisposable Assert.False(property.IsUnicode() ?? true); } + [Fact] + public async Task Configure_UtcConverter_HydratesOccurredAtUtcAsKindUtc() + { + // Insert an AuditEvent with an Unspecified-Kind DateTime, then re-read + // it in a fresh context. The UtcConverter on the OccurredAtUtc / + // IngestedAtUtc columns must re-tag the round-tripped value as + // DateTimeKind.Utc. Without the converter the SQLite (and on production + // SQL Server, datetime2) provider would yield Kind=Unspecified — see + // ConfigurationDatabase-018/020 and Commons-019. + var unspecifiedOccurred = new DateTime(2026, 5, 28, 10, 30, 0, DateTimeKind.Unspecified); + var unspecifiedIngested = new DateTime(2026, 5, 28, 10, 31, 0, DateTimeKind.Unspecified); + var eventId = Guid.NewGuid(); + var siteId = "test-" + Guid.NewGuid().ToString("N").Substring(0, 8); + + var evt = new AuditEvent + { + EventId = eventId, + // The AuditEvent record's init-setter (Commons-019 resolution) + // re-tags Unspecified values as Utc on assignment, so the value EF + // ultimately writes already has Kind=Utc. The converter's job is + // to keep the Kind tag on the READ path, which the assertions + // below exercise. + OccurredAtUtc = unspecifiedOccurred, + IngestedAtUtc = unspecifiedIngested, + Channel = AuditChannel.ApiOutbound, + Kind = AuditKind.ApiCall, + Status = AuditStatus.Delivered, + SourceSiteId = siteId, + }; + + _context.Set().Add(evt); + await _context.SaveChangesAsync(); + + // Detach the tracked entity and re-read in a fresh query so we exercise + // the actual hydrate path, not the change-tracker cache. + _context.ChangeTracker.Clear(); + + var loaded = await _context.Set() + .AsNoTracking() + .Where(e => e.SourceSiteId == siteId) + .SingleAsync(); + + Assert.Equal(DateTimeKind.Utc, loaded.OccurredAtUtc.Kind); + Assert.NotNull(loaded.IngestedAtUtc); + Assert.Equal(DateTimeKind.Utc, loaded.IngestedAtUtc!.Value.Kind); + + // The timestamp ticks must round-trip unchanged — the converter only + // touches the Kind flag, not the wall-clock value. + Assert.Equal(unspecifiedOccurred.Ticks, loaded.OccurredAtUtc.Ticks); + Assert.Equal(unspecifiedIngested.Ticks, loaded.IngestedAtUtc.Value.Ticks); + } + + [Fact] + public void Configure_OccurredAtUtcAndIngestedAtUtc_HaveUtcValueConverters() + { + // Model-metadata cross-check on the converter wiring — guards against a + // future config refactor accidentally removing the HasConversion calls. + // The converter type itself is internal to the configuration, so we + // just assert SOME converter is present on each *Utc DateTime column. + var entity = _context.Model.FindEntityType(typeof(AuditEvent)); + Assert.NotNull(entity); + + var occurredAt = entity!.FindProperty(nameof(AuditEvent.OccurredAtUtc)); + Assert.NotNull(occurredAt); + Assert.NotNull(occurredAt!.GetValueConverter()); + + var ingestedAt = entity.FindProperty(nameof(AuditEvent.IngestedAtUtc)); + Assert.NotNull(ingestedAt); + Assert.NotNull(ingestedAt!.GetValueConverter()); + } + [Fact] public void Configure_FilteredIndexes_HaveExpectedFilters() { diff --git a/tests/ScadaLink.HealthMonitoring.Tests/CentralHealthAggregatorTests.cs b/tests/ScadaLink.HealthMonitoring.Tests/CentralHealthAggregatorTests.cs index 5f3d17fe..595371bc 100644 --- a/tests/ScadaLink.HealthMonitoring.Tests/CentralHealthAggregatorTests.cs +++ b/tests/ScadaLink.HealthMonitoring.Tests/CentralHealthAggregatorTests.cs @@ -306,6 +306,39 @@ public class CentralHealthAggregatorTests Assert.True(_aggregator.GetSiteState("site-1")!.IsOnline); } + /// + /// HealthMonitoring-020 regression: an offline-to-online transition must + /// be backed by a fresh LastHeartbeatAt. Previously MarkHeartbeat used + /// max(receivedAt, existing.LastHeartbeatAt), so an out-of-order + /// heartbeat carrying an older timestamp would bring the site online with + /// a stale heartbeat and CheckForOfflineSites would flap it straight back + /// to offline on the next tick. + /// + [Fact] + public void MarkHeartbeat_OfflineToOnline_StampsFreshLastHeartbeatAt() + { + _aggregator.ProcessReport(MakeReport("site-1", 1)); + + _timeProvider.Advance(TimeSpan.FromSeconds(61)); + _aggregator.CheckForOfflineSites(); + Assert.False(_aggregator.GetSiteState("site-1")!.IsOnline); + + // An out-of-order heartbeat arrives with a timestamp older than the + // existing LastHeartbeatAt (e.g. clock skew on the originating node). + var nowAfter = _timeProvider.GetUtcNow(); + var stale = nowAfter - TimeSpan.FromSeconds(120); + _aggregator.MarkHeartbeat("site-1", stale); + + var state = _aggregator.GetSiteState("site-1")!; + Assert.True(state.IsOnline); + // The recorded LastHeartbeatAt must be ~"now", not the stale receivedAt. + Assert.InRange((nowAfter - state.LastHeartbeatAt).TotalSeconds, 0, 5); + + // And it must survive the very next offline check — proves no flap. + _aggregator.CheckForOfflineSites(); + Assert.True(_aggregator.GetSiteState("site-1")!.IsOnline); + } + /// /// HealthMonitoring-005 regression: the synthetic "central" site has no /// heartbeat source — its LastHeartbeatAt is only bumped by the 30s