diff --git a/archreview/plans/R2-02-resilience-config-hardening-plan.md.tasks.json b/archreview/plans/R2-02-resilience-config-hardening-plan.md.tasks.json index 0417d150..6a0551fc 100644 --- a/archreview/plans/R2-02-resilience-config-hardening-plan.md.tasks.json +++ b/archreview/plans/R2-02-resilience-config-hardening-plan.md.tasks.json @@ -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] }, diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Resilience/DriverResiliencePipelineBuilder.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Resilience/DriverResiliencePipelineBuilder.cs index b1190f86..a0dcc7d9 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Resilience/DriverResiliencePipelineBuilder.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Resilience/DriverResiliencePipelineBuilder.cs @@ -20,6 +20,12 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Resilience; /// Pipeline resolution is lock-free on the hot path: the inner /// caches a per key; /// first-call cost is one .Build. Thereafter reads are O(1). +/// +/// Never-throws invariant (01/S-6): clamps every policy value to +/// before constructing strategy options, so a hostile +/// (constructed directly, bypassing the parser) can never make +/// the memoizing GetOrAdd factory throw Polly's ValidationException — which would +/// otherwise brick every capability call for that key. /// 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(ex => ex is not OperationCanceledException), diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Resilience/DriverResiliencePipelineBuilderTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Resilience/DriverResiliencePipelineBuilderTests.cs index 32b6ea73..ca0b8d14 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Resilience/DriverResiliencePipelineBuilderTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Resilience/DriverResiliencePipelineBuilderTests.cs @@ -325,6 +325,37 @@ public sealed class DriverResiliencePipelineBuilderTests e.Message.Contains("host-9")); } + /// + /// 01/S-6 builder invariant: building a pipeline from a hostile, directly-constructed + /// (bypassing the parser) must NEVER throw Polly's + /// ValidationException. 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. + /// + [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.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"); + } + } + /// Minimal in-memory capturing (level, formatted-message) pairs so the /// resilience logging contract can be asserted without a real logging provider. private sealed class CapturingLogger : ILogger diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Resilience/ResilienceConfigBrickTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Resilience/ResilienceConfigBrickTests.cs new file mode 100644 index 00000000..4b1871d9 --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Resilience/ResilienceConfigBrickTests.cs @@ -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; + +/// +/// 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, +/// "timeoutSeconds": 0 (→ TimeSpan.Zero, outside Polly's [10 ms, 24 h] window) and +/// "breakerFailureThreshold": 1 (below Polly's MinimumThroughput floor of 2) each made +/// throw ValidationException inside the +/// memoizing GetOrAdd 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. +/// +[Trait("Category", "Unit")] +public sealed class ResilienceConfigBrickTests +{ + /// A parsed timeoutSeconds:0 override must not brick a capability call. + [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); + } + + /// A parsed breakerFailureThreshold:1 override must not brick a capability call. + [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); + } +}