fix(options): eager startup validation for AuditLog site sub-options — SiteWriter, SiteTelemetry, SiteRetention (plan R2-08 T5, arch-review 08r2 NF4)

2026-07-13 10:36:46 -04:00
parent 8ddcbbd6f4
commit e0dddac0be
7 changed files with 307 additions and 13 deletions
@@ -92,20 +92,23 @@ public static class ServiceCollectionExtensions
        // failures as AuditRedactionFailure on the site health report.
        services.TryAddSingleton<IAuditRedactionFailureCounter, NoOpAuditRedactionFailureCounter>();

-        // Site writer + telemetry options bindings.
-        // BindConfiguration is not used because the configuration root supplied
-        // by the caller may not be the application root — we go through the
-        // section explicitly so a partial IConfiguration (e.g. a test stub
-        // anchored on the AuditLog section's parent) still works.
-        services.AddOptions<SqliteAuditWriterOptions>()
-            .Bind(config.GetSection(SiteWriterSectionName));
-        services.AddOptions<SiteAuditTelemetryOptions>()
-            .Bind(config.GetSection(SiteTelemetrySectionName));
+        // Site writer + telemetry options bindings — now bind-and-validate via
+        // the shared helper so a bad site config fails fast at boot (NF4). The
+        // helper's GetSection(sectionPath) still resolves the section explicitly
+        // (NOT BindConfiguration), preserving the partial-config-root rationale:
+        // the caller's IConfiguration may be a test stub anchored on the AuditLog
+        // section's parent rather than the application root.
+        services.AddValidatedOptions<SqliteAuditWriterOptions, SqliteAuditWriterOptionsValidator>(
+            config, SiteWriterSectionName);
+        services.AddValidatedOptions<SiteAuditTelemetryOptions, SiteAuditTelemetryOptionsValidator>(
+            config, SiteTelemetrySectionName);
        // Retention purge options (the SiteAuditRetentionService hosted job is
-        // registered site-only in AddAuditLogHealthMetricsBridge). Binding here is
-        // inert on central — no hosted service consumes it there.
-        services.AddOptions<SiteAuditRetentionOptions>()
-            .Bind(config.GetSection(SiteAuditRetentionOptions.SectionName));
+        // registered site-only in AddAuditLogHealthMetricsBridge). The binding
+        // here is inert on central — no hosted service consumes it there — but
+        // the defaults validate clean, so eager validation on the central root is
+        // harmless.
+        services.AddValidatedOptions<SiteAuditRetentionOptions, SiteAuditRetentionOptionsValidator>(
+            config, SiteAuditRetentionOptions.SectionName);

        // SqliteAuditWriter is a singleton with a single owned SqliteConnection
        // and a background writer Task; multiple instances would race on the
@@ -0,0 +1,31 @@
+using ZB.MOM.WW.Configuration;
+
+namespace ZB.MOM.WW.ScadaBridge.AuditLog.Site;
+
+/// <summary>
+/// Validates <see cref="SiteAuditRetentionOptions"/> at host startup
+/// (arch-review 08 round 2 NF4). The option class self-clamps at resolve time,
+/// but a plainly-wrong config (zero retention window, zero/negative purge
+/// period, negative initial delay) should still fail the boot so the operator
+/// learns immediately rather than trusting a silent clamp.
+/// <see cref="SiteAuditRetentionOptions.PurgeIntervalOverride"/> is test-only
+/// (per the class remarks) and is deliberately not validated.
+/// </summary>
+public sealed class SiteAuditRetentionOptionsValidator : OptionsValidatorBase<SiteAuditRetentionOptions>
+{
+    /// <inheritdoc />
+    protected override void Validate(ValidationBuilder builder, SiteAuditRetentionOptions options)
+    {
+        builder.RequireThat(options.RetentionDays > 0,
+            $"AuditLog:SiteRetention:{nameof(SiteAuditRetentionOptions.RetentionDays)} " +
+            $"({options.RetentionDays}) must be > 0.");
+
+        builder.RequireThat(options.PurgeInterval > TimeSpan.Zero,
+            $"AuditLog:SiteRetention:{nameof(SiteAuditRetentionOptions.PurgeInterval)} " +
+            $"({options.PurgeInterval}) must be > 0.");
+
+        builder.RequireThat(options.InitialDelay >= TimeSpan.Zero,
+            $"AuditLog:SiteRetention:{nameof(SiteAuditRetentionOptions.InitialDelay)} " +
+            $"({options.InitialDelay}) must be >= 0.");
+    }
+}
@@ -0,0 +1,38 @@
+using ZB.MOM.WW.Configuration;
+
+namespace ZB.MOM.WW.ScadaBridge.AuditLog.Site;
+
+/// <summary>
+/// Validates <see cref="SqliteAuditWriterOptions"/> at host startup
+/// (arch-review 08 round 2 NF4). The channel/batch/flush knobs feed the
+/// background writer task; a zero anywhere stalls it (nothing drains, nothing
+/// flushes), and an empty <c>DatabasePath</c> leaves the SQLite writer with no
+/// backing store. <see cref="SqliteAuditWriterOptions.BacklogPollIntervalSeconds"/>
+/// is intentionally NOT validated — a non-positive value has a documented
+/// fall-back-to-30s contract in <c>SiteAuditBacklogReporter</c>.
+/// </summary>
+public sealed class SqliteAuditWriterOptionsValidator : OptionsValidatorBase<SqliteAuditWriterOptions>
+{
+    /// <inheritdoc />
+    protected override void Validate(ValidationBuilder builder, SqliteAuditWriterOptions options)
+    {
+        builder.RequireThat(!string.IsNullOrWhiteSpace(options.DatabasePath),
+            $"AuditLog:SiteWriter:{nameof(SqliteAuditWriterOptions.DatabasePath)} must be a non-empty path.");
+
+        builder.RequireThat(options.ChannelCapacity > 0,
+            $"AuditLog:SiteWriter:{nameof(SqliteAuditWriterOptions.ChannelCapacity)} " +
+            $"({options.ChannelCapacity}) must be > 0.");
+
+        builder.RequireThat(options.BatchSize > 0,
+            $"AuditLog:SiteWriter:{nameof(SqliteAuditWriterOptions.BatchSize)} " +
+            $"({options.BatchSize}) must be > 0.");
+
+        builder.RequireThat(options.FlushIntervalMs > 0,
+            $"AuditLog:SiteWriter:{nameof(SqliteAuditWriterOptions.FlushIntervalMs)} " +
+            $"({options.FlushIntervalMs}) must be > 0.");
+
+        // BacklogPollIntervalSeconds is deliberately unchecked: a non-positive
+        // value falls back to the reporter's 30s default (see the option's
+        // remarks + register row 21 resolution).
+    }
+}
@@ -0,0 +1,36 @@
+using ZB.MOM.WW.Configuration;
+
+namespace ZB.MOM.WW.ScadaBridge.AuditLog.Site.Telemetry;
+
+/// <summary>
+/// Validates <see cref="SiteAuditTelemetryOptions"/> at host startup
+/// (arch-review 08 round 2 NF4). <c>SiteAuditTelemetryActor</c> uses the two
+/// interval knobs as raw scheduling delays (busy after a productive/faulted
+/// drain, idle after an empty one), so a zero interval spins the drain loop and
+/// a zero batch drains nothing. Idle must be &gt;= busy: a shorter idle interval
+/// inverts the actor's back-off-when-empty intent.
+/// </summary>
+public sealed class SiteAuditTelemetryOptionsValidator : OptionsValidatorBase<SiteAuditTelemetryOptions>
+{
+    /// <inheritdoc />
+    protected override void Validate(ValidationBuilder builder, SiteAuditTelemetryOptions options)
+    {
+        builder.RequireThat(options.BatchSize > 0,
+            $"AuditLog:SiteTelemetry:{nameof(SiteAuditTelemetryOptions.BatchSize)} " +
+            $"({options.BatchSize}) must be > 0.");
+
+        builder.RequireThat(options.BusyIntervalSeconds > 0,
+            $"AuditLog:SiteTelemetry:{nameof(SiteAuditTelemetryOptions.BusyIntervalSeconds)} " +
+            $"({options.BusyIntervalSeconds}) must be > 0.");
+
+        builder.RequireThat(options.IdleIntervalSeconds > 0,
+            $"AuditLog:SiteTelemetry:{nameof(SiteAuditTelemetryOptions.IdleIntervalSeconds)} " +
+            $"({options.IdleIntervalSeconds}) must be > 0.");
+
+        builder.RequireThat(options.IdleIntervalSeconds >= options.BusyIntervalSeconds,
+            $"AuditLog:SiteTelemetry:{nameof(SiteAuditTelemetryOptions.IdleIntervalSeconds)} " +
+            $"({options.IdleIntervalSeconds}) must be >= " +
+            $"{nameof(SiteAuditTelemetryOptions.BusyIntervalSeconds)} ({options.BusyIntervalSeconds}) " +
+            "— the idle drain must back off at least as far as the busy drain.");
+    }
+}
@@ -0,0 +1,53 @@
+using ZB.MOM.WW.ScadaBridge.AuditLog.Site;
+
+namespace ZB.MOM.WW.ScadaBridge.AuditLog.Tests.Site;
+
+/// <summary>
+/// Eager startup validation for <see cref="SiteAuditRetentionOptions"/>
+/// (arch-review 08 round 2 NF4). Although the option class self-clamps at
+/// resolve time, the validator still fails a boot with a plainly-wrong config
+/// (zero retention window, zero/negative purge period, negative initial delay)
+/// so the operator learns at startup rather than trusting a silent clamp.
+/// <c>PurgeIntervalOverride</c> is test-only and left unvalidated.
+/// </summary>
+public class SiteAuditRetentionOptionsValidatorTests
+{
+    [Fact]
+    public void DefaultOptions_AreValid()
+    {
+        var validator = new SiteAuditRetentionOptionsValidator();
+        Assert.True(validator.Validate(null, new SiteAuditRetentionOptions()).Succeeded);
+    }
+
+    [Fact]
+    public void ZeroRetentionDays_Fails()
+    {
+        var validator = new SiteAuditRetentionOptionsValidator();
+        var result = validator.Validate(null, new SiteAuditRetentionOptions { RetentionDays = 0 });
+        Assert.False(result.Succeeded);
+        Assert.Contains(result.Failures!,
+            f => f.Contains(nameof(SiteAuditRetentionOptions.RetentionDays), StringComparison.Ordinal));
+    }
+
+    [Fact]
+    public void ZeroPurgeInterval_Fails()
+    {
+        var validator = new SiteAuditRetentionOptionsValidator();
+        var result = validator.Validate(null,
+            new SiteAuditRetentionOptions { PurgeInterval = TimeSpan.Zero });
+        Assert.False(result.Succeeded);
+        Assert.Contains(result.Failures!,
+            f => f.Contains(nameof(SiteAuditRetentionOptions.PurgeInterval), StringComparison.Ordinal));
+    }
+
+    [Fact]
+    public void NegativeInitialDelay_Fails()
+    {
+        var validator = new SiteAuditRetentionOptionsValidator();
+        var result = validator.Validate(null,
+            new SiteAuditRetentionOptions { InitialDelay = TimeSpan.FromSeconds(-1) });
+        Assert.False(result.Succeeded);
+        Assert.Contains(result.Failures!,
+            f => f.Contains(nameof(SiteAuditRetentionOptions.InitialDelay), StringComparison.Ordinal));
+    }
+}
@@ -0,0 +1,61 @@
+using ZB.MOM.WW.ScadaBridge.AuditLog.Site.Telemetry;
+
+namespace ZB.MOM.WW.ScadaBridge.AuditLog.Tests.Site;
+
+/// <summary>
+/// Eager startup validation for <see cref="SiteAuditTelemetryOptions"/>
+/// (arch-review 08 round 2 NF4). The drain-loop cadences are used as raw
+/// scheduling delays in <c>SiteAuditTelemetryActor</c>; a zero interval spins
+/// the drain and a zero batch drains nothing. Idle must back off at least as
+/// far as busy (idle &lt; busy inverts the actor's intent).
+/// </summary>
+public class SiteAuditTelemetryOptionsValidatorTests
+{
+    [Fact]
+    public void DefaultOptions_AreValid()
+    {
+        var validator = new SiteAuditTelemetryOptionsValidator();
+        Assert.True(validator.Validate(null, new SiteAuditTelemetryOptions()).Succeeded);
+    }
+
+    [Fact]
+    public void ZeroBatchSize_Fails()
+    {
+        var validator = new SiteAuditTelemetryOptionsValidator();
+        var result = validator.Validate(null, new SiteAuditTelemetryOptions { BatchSize = 0 });
+        Assert.False(result.Succeeded);
+        Assert.Contains(result.Failures!,
+            f => f.Contains(nameof(SiteAuditTelemetryOptions.BatchSize), StringComparison.Ordinal));
+    }
+
+    [Fact]
+    public void ZeroBusyInterval_Fails()
+    {
+        var validator = new SiteAuditTelemetryOptionsValidator();
+        var result = validator.Validate(null, new SiteAuditTelemetryOptions { BusyIntervalSeconds = 0 });
+        Assert.False(result.Succeeded);
+        Assert.Contains(result.Failures!,
+            f => f.Contains(nameof(SiteAuditTelemetryOptions.BusyIntervalSeconds), StringComparison.Ordinal));
+    }
+
+    [Fact]
+    public void ZeroIdleInterval_Fails()
+    {
+        var validator = new SiteAuditTelemetryOptionsValidator();
+        var result = validator.Validate(null, new SiteAuditTelemetryOptions { IdleIntervalSeconds = 0 });
+        Assert.False(result.Succeeded);
+        Assert.Contains(result.Failures!,
+            f => f.Contains(nameof(SiteAuditTelemetryOptions.IdleIntervalSeconds), StringComparison.Ordinal));
+    }
+
+    [Fact]
+    public void IdleShorterThanBusy_Fails()
+    {
+        var validator = new SiteAuditTelemetryOptionsValidator();
+        var result = validator.Validate(null,
+            new SiteAuditTelemetryOptions { BusyIntervalSeconds = 30, IdleIntervalSeconds = 5 });
+        Assert.False(result.Succeeded);
+        Assert.Contains(result.Failures!,
+            f => f.Contains(nameof(SiteAuditTelemetryOptions.IdleIntervalSeconds), StringComparison.Ordinal));
+    }
+}
@@ -0,0 +1,72 @@
+using ZB.MOM.WW.ScadaBridge.AuditLog.Site;
+
+namespace ZB.MOM.WW.ScadaBridge.AuditLog.Tests.Site;
+
+/// <summary>
+/// Eager startup validation for <see cref="SqliteAuditWriterOptions"/>
+/// (arch-review 08 round 2 NF4). The site hot-path writer's channel/batch/flush
+/// knobs must be positive or the background writer task cannot make progress;
+/// an empty <c>DatabasePath</c> would leave the SQLite writer with nowhere to go.
+/// <c>BacklogPollIntervalSeconds</c> is deliberately NOT validated — a
+/// non-positive value has a documented fall-back-to-30s contract.
+/// </summary>
+public class SqliteAuditWriterOptionsValidatorTests
+{
+    [Fact]
+    public void DefaultOptions_AreValid()
+    {
+        var validator = new SqliteAuditWriterOptionsValidator();
+        Assert.True(validator.Validate(null, new SqliteAuditWriterOptions()).Succeeded);
+    }
+
+    [Fact]
+    public void EmptyDatabasePath_Fails()
+    {
+        var validator = new SqliteAuditWriterOptionsValidator();
+        var result = validator.Validate(null, new SqliteAuditWriterOptions { DatabasePath = "" });
+        Assert.False(result.Succeeded);
+        Assert.Contains(result.Failures!,
+            f => f.Contains(nameof(SqliteAuditWriterOptions.DatabasePath), StringComparison.Ordinal));
+    }
+
+    [Fact]
+    public void ZeroChannelCapacity_Fails()
+    {
+        var validator = new SqliteAuditWriterOptionsValidator();
+        var result = validator.Validate(null, new SqliteAuditWriterOptions { ChannelCapacity = 0 });
+        Assert.False(result.Succeeded);
+        Assert.Contains(result.Failures!,
+            f => f.Contains(nameof(SqliteAuditWriterOptions.ChannelCapacity), StringComparison.Ordinal));
+    }
+
+    [Fact]
+    public void ZeroBatchSize_Fails()
+    {
+        var validator = new SqliteAuditWriterOptionsValidator();
+        var result = validator.Validate(null, new SqliteAuditWriterOptions { BatchSize = 0 });
+        Assert.False(result.Succeeded);
+        Assert.Contains(result.Failures!,
+            f => f.Contains(nameof(SqliteAuditWriterOptions.BatchSize), StringComparison.Ordinal));
+    }
+
+    [Fact]
+    public void ZeroFlushInterval_Fails()
+    {
+        var validator = new SqliteAuditWriterOptionsValidator();
+        var result = validator.Validate(null, new SqliteAuditWriterOptions { FlushIntervalMs = 0 });
+        Assert.False(result.Succeeded);
+        Assert.Contains(result.Failures!,
+            f => f.Contains(nameof(SqliteAuditWriterOptions.FlushIntervalMs), StringComparison.Ordinal));
+    }
+
+    [Theory]
+    [InlineData(0)]
+    [InlineData(-1)]
+    public void NonPositiveBacklogPollInterval_StillValid(int value)
+    {
+        // Documented fall-back-to-30s contract — must NOT be rejected.
+        var validator = new SqliteAuditWriterOptionsValidator();
+        Assert.True(validator.Validate(null,
+            new SqliteAuditWriterOptions { BacklogPollIntervalSeconds = value }).Succeeded);
+    }
+}