From 973e59de8475b50c57baad1141b1587ade1438b7 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 10:53:27 -0400 Subject: [PATCH] fix(options): bind + validate OperationTrackingOptions; register the site IOperationTrackingStore that two comments claimed AddSiteRuntime provides (plan R2-08 T8, arch-review 08r2 NF4) VERIFY-THEN-FIX: the plan-authoring discovery is CONFIRMED. No code anywhere in src/ registered IOperationTrackingStore in DI, yet AkkaHostedService and AuditLog SCE both comment that AddSiteRuntime provides it. Every consumer resolved it via GetService (null-tolerant) and silently ran degraded: cached-drain scheduler never armed, PullSiteCalls reconciliation seam never wired, Tracking.Status degraded to audit-only. This is a FUNCTIONAL FIX riding the hygiene plan: AddSiteRuntime now registers the store (site-only), so site-local cached-call tracking runs in its intended mode. OperationTrackingOptions is now bound + eagerly validated in the Host site-options block. --- .../SiteServiceRegistration.cs | 12 ++++++ .../ServiceCollectionExtensions.cs | 13 ++++++ .../OperationTrackingOptionsValidator.cs | 26 ++++++++++++ .../CompositionRootTests.cs | 20 ++++++++++ .../OperationTrackingOptionsValidatorTests.cs | 40 +++++++++++++++++++ 5 files changed, 111 insertions(+) create mode 100644 src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Tracking/OperationTrackingOptionsValidator.cs create mode 100644 tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Tracking/OperationTrackingOptionsValidatorTests.cs diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/SiteServiceRegistration.cs b/src/ZB.MOM.WW.ScadaBridge.Host/SiteServiceRegistration.cs index b24d0277..b0d33603 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/SiteServiceRegistration.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/SiteServiceRegistration.cs @@ -137,6 +137,18 @@ public static class SiteServiceRegistration services.TryAddEnumerable( ServiceDescriptor.Singleton, SiteRuntimeOptionsValidator>()); + // OperationTrackingOptions for the site-local cached-call tracking store + // (registered by AddSiteRuntime). Bind + eagerly validate here — the store + // opens its SQLite connection string at construction, so a blank connection + // string should fail the host at boot with a key-naming message rather than + // throw opaquely on first resolve (arch-review 08r2 NF4/T8). + services.AddOptions() + .Bind(config.GetSection("ScadaBridge:OperationTracking")) + .ValidateOnStart(); + services.TryAddEnumerable( + ServiceDescriptor.Singleton, + SiteRuntime.Tracking.OperationTrackingOptionsValidator>()); + services.Configure(config.GetSection("ScadaBridge:DataConnection")); services.AddOptions() diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/ServiceCollectionExtensions.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/ServiceCollectionExtensions.cs index 20b612da..7352e990 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/ServiceCollectionExtensions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/ServiceCollectionExtensions.cs @@ -60,6 +60,19 @@ public static class ServiceCollectionExtensions }); services.AddSingleton(sp => sp.GetRequiredService()); + // Site-local cached-operation tracking store — the SQLite source of truth + // that Tracking.Status(TrackedOperationId) reads and the cached-call telemetry + // forwarder writes. Registered HERE (site-only) so the two comments that claim + // "the operational tracking store is registered by AddSiteRuntime" are true + // again (AuditLog SCE + AkkaHostedService). Before this (arch-review 08 round 2 + // NF4/T8) it had no DI registration anywhere in src/, so every consumer resolved + // it as null and silently ran degraded (no cached-drain, no PullSiteCalls + // reconciliation, Tracking.Status audit-only). Central roots never call + // AddSiteRuntime, so this stays absent on central — matching the SITE-ONLY + // contract. OperationTrackingOptions is bound + validated by the Host's + // SiteServiceRegistration site-options block. + services.AddSingleton(); + // Site-local repository implementations backed by SQLite services.AddScoped(); diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Tracking/OperationTrackingOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Tracking/OperationTrackingOptionsValidator.cs new file mode 100644 index 00000000..56ddc469 --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Tracking/OperationTrackingOptionsValidator.cs @@ -0,0 +1,26 @@ +using ZB.MOM.WW.Configuration; + +namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Tracking; + +/// +/// Validates at host startup (arch-review +/// 08 round 2 NF4). opens the SQLite +/// connection string in its constructor, so an empty/blank connection string +/// must fail the host at boot rather than throw opaquely on first resolve. The +/// retention window must be positive — a zero would make the host's terminal-row +/// purge delete every tracking row. +/// +public sealed class OperationTrackingOptionsValidator : OptionsValidatorBase +{ + /// + protected override void Validate(ValidationBuilder builder, OperationTrackingOptions options) + { + builder.RequireThat(!string.IsNullOrWhiteSpace(options.ConnectionString), + $"ScadaBridge:OperationTracking:{nameof(OperationTrackingOptions.ConnectionString)} " + + "must be a non-empty SQLite connection string."); + + builder.RequireThat(options.RetentionDays > 0, + $"ScadaBridge:OperationTracking:{nameof(OperationTrackingOptions.RetentionDays)} " + + $"({options.RetentionDays}) must be > 0."); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs index a1ae5025..0e871b99 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs @@ -339,10 +339,12 @@ public class SiteCompositionRootTests : IDisposable { private readonly WebApplication _host; private readonly string _tempDbPath; + private readonly string _tempTrackingDbPath; public SiteCompositionRootTests() { _tempDbPath = Path.Combine(Path.GetTempPath(), $"scadabridge_test_{Guid.NewGuid()}.db"); + _tempTrackingDbPath = Path.Combine(Path.GetTempPath(), $"scadabridge_track_{Guid.NewGuid()}.db"); var builder = WebApplication.CreateBuilder(); builder.Configuration.Sources.Clear(); @@ -355,6 +357,10 @@ public class SiteCompositionRootTests : IDisposable ["ScadaBridge:Node:RemotingPort"] = "0", ["ScadaBridge:Node:GrpcPort"] = "0", ["ScadaBridge:Database:SiteDbPath"] = _tempDbPath, + // Point the site-local operation-tracking SQLite store (the ctor opens/creates + // the file eagerly) at a temp path so resolving IOperationTrackingStore in the + // composition-root test does not litter site-tracking.db in the test cwd. + ["ScadaBridge:OperationTracking:ConnectionString"] = $"Data Source={_tempTrackingDbPath}", // ClusterOptions requires at least one seed node (ClusterOptionsValidator). ["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:2551", ["ScadaBridge:Cluster:SeedNodes:1"] = "akka.tcp://scadabridge@localhost:2552", @@ -377,6 +383,20 @@ public class SiteCompositionRootTests : IDisposable { (_host as IDisposable)?.Dispose(); try { File.Delete(_tempDbPath); } catch { /* best effort */ } + try { File.Delete(_tempTrackingDbPath); } catch { /* best effort */ } + } + + [Fact] + public void Site_Resolves_IOperationTrackingStore() + { + // Site Call Audit's site-local source of truth. AkkaHostedService and + // ScriptRuntimeContext resolve this with GetService and silently degrade + // when absent (audit-only telemetry, no cached-drain, no PullSiteCalls) — + // so its presence on the SITE composition root must be locked here + // (arch-review 08 round 2 NF4/T8: AddSiteRuntime never registered it, + // despite two comments claiming it did). + var store = _host.Services.GetService(); + Assert.NotNull(store); } // --- Singletons --- diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Tracking/OperationTrackingOptionsValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Tracking/OperationTrackingOptionsValidatorTests.cs new file mode 100644 index 00000000..519fc7a7 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Tracking/OperationTrackingOptionsValidatorTests.cs @@ -0,0 +1,40 @@ +using ZB.MOM.WW.ScadaBridge.SiteRuntime.Tracking; + +namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.Tracking; + +/// +/// Eager startup validation for +/// (arch-review 08 round 2 NF4). The store opens the SQLite connection string at +/// construction, so an empty connection string must fail the host at boot; the +/// retention window must be positive or terminal-row purge would delete +/// everything. +/// +public class OperationTrackingOptionsValidatorTests +{ + [Fact] + public void DefaultOptions_AreValid() + { + var validator = new OperationTrackingOptionsValidator(); + Assert.True(validator.Validate(null, new OperationTrackingOptions()).Succeeded); + } + + [Fact] + public void EmptyConnectionString_Fails() + { + var validator = new OperationTrackingOptionsValidator(); + var result = validator.Validate(null, new OperationTrackingOptions { ConnectionString = "" }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(OperationTrackingOptions.ConnectionString), StringComparison.Ordinal)); + } + + [Fact] + public void ZeroRetentionDays_Fails() + { + var validator = new OperationTrackingOptionsValidator(); + var result = validator.Validate(null, new OperationTrackingOptions { RetentionDays = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(OperationTrackingOptions.RetentionDays), StringComparison.Ordinal)); + } +}