fix(r2-02): pipeline Build clamps policy values — building a pipeline can never throw Polly ValidationException (01/S-6 belt-and-braces + brick repro)

2026-07-13 09:55:05 -04:00
parent 294d74c1de
commit 8d306a1bd2
4 changed files with 97 additions and 5 deletions
@@ -1,10 +1,10 @@
 {
  "planPath": "archreview/plans/R2-02-resilience-config-hardening-plan.md",
  "tasks": [
-    { "id": 0, "subject": "S-6 brick-repro test (RED): parsed timeoutSeconds:0 / breakerFailureThreshold:1 must not brick capability calls — ResilienceConfigBrickTests, expect 2 failures on f6eaa267", "status": "pending", "blockedBy": [] },
+    { "id": 0, "subject": "S-6 brick-repro test (RED): parsed timeoutSeconds:0 / breakerFailureThreshold:1 must not brick capability calls — ResilienceConfigBrickTests, expect 2 failures on f6eaa267", "status": "completed", "blockedBy": [] },
    { "id": 1, "subject": "Add ResiliencePolicyRanges constants to Core.Abstractions (single source of truth for parser/builder/AdminUI, derived from Polly.Core 8.6.6 validation)", "status": "completed", "blockedBy": [] },
    { "id": 2, "subject": "S-6 parser: clamp timeout/retry/breaker overrides with appending diagnostics (timeout<=0 -> tier default; breaker==1 -> 2; caps) + 8 parser tests", "status": "completed", "blockedBy": [0, 1] },
-    { "id": 3, "subject": "S-6 builder: belt-and-braces clamp in Build so pipeline construction can never throw ValidationException + Build_NeverThrows_ForHostileDirectOptions (turns task 0 green)", "status": "pending", "blockedBy": [0, 1] },
+    { "id": 3, "subject": "S-6 builder: belt-and-braces clamp in Build so pipeline construction can never throw ValidationException + Build_NeverThrows_ForHostileDirectOptions (turns task 0 green)", "status": "completed", "blockedBy": [0, 1] },
    { "id": 4, "subject": "S-6 production-wiring guard: DriverCapabilityInvokerFactory.Create with hostile JSON yields a working invoker + logged clamp diagnostic; re-run ResilienceInvokerFactoryRegistrationTests", "status": "pending", "blockedBy": [2, 3] },
    { "id": 5, "subject": "S-6 AdminUI: ResilienceFormModel.Validate() range messages (via ResiliencePolicyRanges through the transitive Core.Abstractions ref) + warning block in DriverResilienceSection", "status": "pending", "blockedBy": [1] },
    { "id": 6, "subject": "S-8 parser layer: force Write/AlarmAcknowledge retryCount overrides to 0 with diagnostic (spec invariant, honest dead-knob surfacing) + tests + GetTierDefaults doc", "status": "pending", "blockedBy": [2] },
@@ -20,6 +20,12 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Resilience;
 ///     <para>Pipeline resolution is lock-free on the hot path: the inner
 ///     <see cref="ConcurrentDictionary{TKey,TValue}"/> caches a <see cref="ResiliencePipeline"/> per key;
 ///     first-call cost is one <see cref="ResiliencePipelineBuilder"/>.Build. Thereafter reads are O(1).</para>
+///
+///     <para><b>Never-throws invariant (01/S-6):</b> <see cref="Build"/> clamps every policy value to
+///     <see cref="ResiliencePolicyRanges"/> before constructing strategy options, so a hostile
+///     <see cref="DriverResilienceOptions"/> (constructed directly, bypassing the parser) can never make
+///     the memoizing <c>GetOrAdd</c> factory throw Polly's <c>ValidationException</c> — which would
+///     otherwise brick every capability call for that key.</para>
 /// </remarks>
 public sealed class DriverResiliencePipelineBuilder
 {
@@ -108,16 +114,23 @@ public sealed class DriverResiliencePipelineBuilder
        var policy = options.Resolve(capability);
        var builder = new ResiliencePipelineBuilder { TimeProvider = timeProvider };

+        // Belt-and-braces: clamp the resolved policy to ResiliencePolicyRanges so building a pipeline can
+        // NEVER throw Polly's ValidationException (01/S-6). The parser already clamps every JSON→options
+        // funnel, but DriverResilienceOptions is a public record constructible by paths that never traverse
+        // the parser (tests today; any future caller), so this makes "a pipeline build never throws" a
+        // builder invariant. Three integer ops once per pipeline build (not per call).
+        var timeoutSeconds = Math.Clamp(
+            policy.TimeoutSeconds, ResiliencePolicyRanges.MinTimeoutSeconds, ResiliencePolicyRanges.MaxTimeoutSeconds);
        builder.AddTimeout(new TimeoutStrategyOptions
        {
-            Timeout = TimeSpan.FromSeconds(policy.TimeoutSeconds),
+            Timeout = TimeSpan.FromSeconds(timeoutSeconds),
        });

        if (policy.RetryCount > 0)
        {
            var retryOptions = new RetryStrategyOptions
            {
-                MaxRetryAttempts = policy.RetryCount,
+                MaxRetryAttempts = Math.Min(policy.RetryCount, ResiliencePolicyRanges.MaxRetryCount),
                BackoffType = DelayBackoffType.Exponential,
                UseJitter = true,
                Delay = TimeSpan.FromMilliseconds(100),
@@ -143,7 +156,7 @@ public sealed class DriverResiliencePipelineBuilder
            var breakerOptions = new CircuitBreakerStrategyOptions
            {
                FailureRatio = 1.0,
-                MinimumThroughput = policy.BreakerFailureThreshold,
+                MinimumThroughput = Math.Max(ResiliencePolicyRanges.MinEnabledBreakerFailureThreshold, policy.BreakerFailureThreshold),
                SamplingDuration = TimeSpan.FromSeconds(30),
                BreakDuration = TimeSpan.FromSeconds(15),
                ShouldHandle = new PredicateBuilder().Handle<Exception>(ex => ex is not OperationCanceledException),
@@ -325,6 +325,37 @@ public sealed class DriverResiliencePipelineBuilderTests
            e.Message.Contains("host-9"));
    }

+    /// <summary>
+    ///     01/S-6 builder invariant: building a pipeline from a hostile, directly-constructed
+    ///     <see cref="CapabilityPolicy"/> (bypassing the parser) must NEVER throw Polly's
+    ///     <c>ValidationException</c>. Sweeps the full matrix of {int.MinValue, -1, 0, 1, int.MaxValue}
+    ///     across timeout/retry/breaker and asserts every built pipeline executes a trivial call.
+    /// </summary>
+    [Fact]
+    public async Task Build_NeverThrows_ForHostileDirectOptions()
+    {
+        int[] hostile = { int.MinValue, -1, 0, 1, int.MaxValue };
+        var host = 0;
+        foreach (var t in hostile)
+        foreach (var r in hostile)
+        foreach (var b in hostile)
+        {
+            var options = new DriverResilienceOptions
+            {
+                Tier = DriverTier.A,
+                CapabilityPolicies = new Dictionary<DriverCapability, CapabilityPolicy>
+                {
+                    [DriverCapability.Read] = new(TimeoutSeconds: t, RetryCount: r, BreakerFailureThreshold: b),
+                },
+            };
+            var builder = new DriverResiliencePipelineBuilder();
+            var pipeline = builder.GetOrCreate("hostile", $"h{host++}", DriverCapability.Read, options);
+
+            var result = await pipeline.ExecuteAsync(_ => ValueTask.FromResult(1), CancellationToken.None);
+            result.ShouldBe(1, $"pipeline built from hostile (t={t}, r={r}, b={b}) must execute, not throw");
+        }
+    }
+
    /// <summary>Minimal in-memory <see cref="ILogger"/> capturing (level, formatted-message) pairs so the
    /// resilience logging contract can be asserted without a real logging provider.</summary>
    private sealed class CapturingLogger : ILogger
@@ -0,0 +1,48 @@
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
+using ZB.MOM.WW.OtOpcUa.Core.Resilience;
+
+namespace ZB.MOM.WW.OtOpcUa.Core.Tests.Resilience;
+
+/// <summary>
+///     Load-bearing regression guard for 01/S-6 — an operator-authorable out-of-range
+///     ResilienceConfig value must NEVER brick a healthy driver. Before the R2-02 hardening pass,
+///     <c>"timeoutSeconds": 0</c> (→ <c>TimeSpan.Zero</c>, outside Polly's [10 ms, 24 h] window) and
+///     <c>"breakerFailureThreshold": 1</c> (below Polly's MinimumThroughput floor of 2) each made
+///     <see cref="DriverResiliencePipelineBuilder"/> throw <c>ValidationException</c> inside the
+///     memoizing <c>GetOrAdd</c> factory on EVERY capability call for that (instance, host,
+///     capability) — a permanent brick while the driver is healthy. The parser now clamps and the
+///     builder belt-and-braces clamps, so a pipeline built from a parsed hostile config executes.
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class ResilienceConfigBrickTests
+{
+    /// <summary>A parsed <c>timeoutSeconds:0</c> override must not brick a capability call.</summary>
+    [Fact]
+    public async Task ParsedConfig_WithZeroTimeout_DoesNotBrickCapabilityCalls()
+    {
+        var options = DriverResilienceOptionsParser.ParseOrDefaults(
+            DriverTier.A, "{\"capabilityPolicies\":{\"Read\":{\"timeoutSeconds\":0}}}", out _);
+
+        var pipeline = new DriverResiliencePipelineBuilder()
+            .GetOrCreate("i1", "h1", DriverCapability.Read, options);
+
+        var result = await pipeline.ExecuteAsync(_ => ValueTask.FromResult(42), CancellationToken.None);
+        result.ShouldBe(42);
+    }
+
+    /// <summary>A parsed <c>breakerFailureThreshold:1</c> override must not brick a capability call.</summary>
+    [Fact]
+    public async Task ParsedConfig_WithBreakerThresholdOne_DoesNotBrickCapabilityCalls()
+    {
+        var options = DriverResilienceOptionsParser.ParseOrDefaults(
+            DriverTier.A, "{\"capabilityPolicies\":{\"Read\":{\"breakerFailureThreshold\":1}}}", out _);
+
+        var pipeline = new DriverResiliencePipelineBuilder()
+            .GetOrCreate("i1", "h1", DriverCapability.Read, options);
+
+        var result = await pipeline.ExecuteAsync(_ => ValueTask.FromResult(42), CancellationToken.None);
+        result.ShouldBe(42);
+    }
+}