From 5ea6e9d7d9017424d35daf45b8fcba6cf49ff2d5 Mon Sep 17 00:00:00 2001
From: Joseph Doherty <dohejw01@gmail.com>
Date: Thu, 11 Jun 2026 13:09:13 -0400
Subject: [PATCH] fix(historian): validate non-positive
 drain/capacity/retention knobs (review) + log prefix

---
 .../Historian/AlarmHistorianOptions.cs        |  9 ++++++-
 .../ServiceCollectionExtensions.cs            |  2 +-
 .../AlarmHistorianRegistrationTests.cs        | 25 +++++++++++++++++++
 3 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Historian/AlarmHistorianOptions.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Historian/AlarmHistorianOptions.cs
index aec679e5..b99d55fa 100644
--- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Historian/AlarmHistorianOptions.cs
+++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Historian/AlarmHistorianOptions.cs
@@ -1,5 +1,6 @@
 using System.Collections.Generic;
 using System.IO;
+using ZB.MOM.WW.OtOpcUa.Core.AlarmHistorian;
 
 namespace ZB.MOM.WW.OtOpcUa.Runtime.Historian;
 
@@ -38,7 +39,7 @@ public sealed class AlarmHistorianOptions
 
     /// <summary>Maximum queued rows before the sink evicts the oldest. Defaults to 1,000,000
     /// (matches <c>SqliteStoreAndForwardSink</c>'s <c>DefaultCapacity</c>).</summary>
-    public long Capacity { get; init; } = 1_000_000;
+    public long Capacity { get; init; } = SqliteStoreAndForwardSink.DefaultCapacity;
 
     /// <summary>Days to retain dead-lettered rows before purge. Defaults to 30.</summary>
     public int DeadLetterRetentionDays { get; init; } = 30;
@@ -54,6 +55,12 @@ public sealed class AlarmHistorianOptions
             warnings.Add("AlarmHistorian:SharedSecret is empty while the historian is enabled — the Wonderware sidecar Hello frame will carry an empty secret.");
         if (!Path.IsPathRooted(DatabasePath))
             warnings.Add($"AlarmHistorian:DatabasePath '{DatabasePath}' is relative — it resolves against the process working directory (e.g. System32 for a Windows service). Set an absolute path.");
+        if (DrainIntervalSeconds <= 0)
+            warnings.Add($"AlarmHistorian:DrainIntervalSeconds is {DrainIntervalSeconds} — must be > 0; the drain timer will throw or spin at startup.");
+        if (Capacity <= 0)
+            warnings.Add($"AlarmHistorian:Capacity is {Capacity} — must be > 0; the sink constructor will throw at startup.");
+        if (DeadLetterRetentionDays <= 0)
+            warnings.Add($"AlarmHistorian:DeadLetterRetentionDays is {DeadLetterRetentionDays} — must be > 0; dead-lettered rows would be purged on every drain tick.");
         return warnings;
     }
 }
diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/ServiceCollectionExtensions.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/ServiceCollectionExtensions.cs
index 67003c07..fc89162c 100644
--- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/ServiceCollectionExtensions.cs
+++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/ServiceCollectionExtensions.cs
@@ -73,7 +73,7 @@ public static class ServiceCollectionExtensions
         if (opts is not { Enabled: true }) return services; // leave the Null default from AddOtOpcUaRuntime
 
         foreach (var warning in opts.Validate())
-            Serilog.Log.Logger.ForContext<SqliteStoreAndForwardSink>().Warning("{HistorianConfigWarning}", warning);
+            Serilog.Log.Logger.ForContext<SqliteStoreAndForwardSink>().Warning("Historian config: {HistorianConfigWarning}", warning);
 
         services.AddSingleton<IAlarmHistorianSink>(sp =>
         {
diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Historian/AlarmHistorianRegistrationTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Historian/AlarmHistorianRegistrationTests.cs
index b1d0b9e6..de3b44a2 100644
--- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Historian/AlarmHistorianRegistrationTests.cs
+++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Historian/AlarmHistorianRegistrationTests.cs
@@ -137,4 +137,29 @@ public sealed class AlarmHistorianRegistrationTests
     {
         new AlarmHistorianOptions { Enabled = false, SharedSecret = "" }.Validate().ShouldBeEmpty();
     }
+
+    [Fact]
+    public void Validate_warns_on_non_positive_drain_interval()
+    {
+        var opts = new AlarmHistorianOptions { Enabled = true, SharedSecret = "s", DatabasePath = "/abs/h.db", DrainIntervalSeconds = 0 };
+        opts.Validate().ShouldContain(w => w.Contains("DrainIntervalSeconds"));
+    }
+
+    [Fact]
+    public void Validate_warns_on_non_positive_capacity()
+    {
+        var opts = new AlarmHistorianOptions { Enabled = true, SharedSecret = "s", DatabasePath = "/abs/h.db", Capacity = 0 };
+        opts.Validate().ShouldContain(w => w.Contains("Capacity"));
+    }
+
+    [Fact]
+    public void Validate_accumulates_multiple_warnings()
+    {
+        // relative path + empty secret ⇒ both warnings, not short-circuited on the first.
+        var opts = new AlarmHistorianOptions { Enabled = true, SharedSecret = "", DatabasePath = "alarm-historian.db" };
+        var warnings = opts.Validate();
+        warnings.ShouldContain(w => w.Contains("SharedSecret"));
+        warnings.ShouldContain(w => w.Contains("DatabasePath"));
+        warnings.Count.ShouldBeGreaterThanOrEqualTo(2);
+    }
 }