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 b04bd71d..0417d150 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 @@ -3,7 +3,7 @@ "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": 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": "pending", "blockedBy": [0, 1] }, + { "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": 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] }, diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Resilience/DriverResilienceOptionsParser.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Resilience/DriverResilienceOptionsParser.cs index fa9690b2..a91b0285 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Resilience/DriverResilienceOptionsParser.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Resilience/DriverResilienceOptionsParser.cs @@ -83,15 +83,16 @@ public static class DriverResilienceOptionsParser { if (!Enum.TryParse(capName, ignoreCase: true, out var capability)) { - parseDiagnostic ??= $"Unknown capability '{capName}' in ResilienceConfig; skipped."; + AppendDiagnostic(ref parseDiagnostic, $"Unknown capability '{capName}' in ResilienceConfig; skipped."); continue; } var basePolicy = merged[capability]; - merged[capability] = new CapabilityPolicy( + var mergedPolicy = new CapabilityPolicy( TimeoutSeconds: overridePolicy.TimeoutSeconds ?? basePolicy.TimeoutSeconds, RetryCount: overridePolicy.RetryCount ?? basePolicy.RetryCount, BreakerFailureThreshold: overridePolicy.BreakerFailureThreshold ?? basePolicy.BreakerFailureThreshold); + merged[capability] = ClampPolicy(capability, mergedPolicy, basePolicy, ref parseDiagnostic); } } @@ -102,9 +103,9 @@ public static class DriverResilienceOptionsParser if (shape.RecycleIntervalSeconds is int secs) { if (secs <= 0) - parseDiagnostic ??= $"RecycleIntervalSeconds must be positive; got {secs} — ignoring."; + AppendDiagnostic(ref parseDiagnostic, $"RecycleIntervalSeconds must be positive; got {secs} — ignoring."); else if (tier != DriverTier.C) - parseDiagnostic ??= $"RecycleIntervalSeconds is Tier C only; Tier {tier} driver will not scheduled-recycle."; + AppendDiagnostic(ref parseDiagnostic, $"RecycleIntervalSeconds is Tier C only; Tier {tier} driver will not scheduled-recycle."); else recycleIntervalSeconds = secs; } @@ -119,6 +120,72 @@ public static class DriverResilienceOptionsParser }; } + /// + /// Clamp a merged per-capability override to the legal so a + /// downstream Polly pipeline build can never throw ValidationException (01/S-6). Each clamp + /// appends a diagnostic naming the field + what happened. A non-positive timeout snaps to the + /// capability's (a nonsense value, not merely an extreme — snapping + /// to 1 s could be a 30× surprise on a Discover policy); everything else clamps to the nearest legal + /// value. Preserves maximum operator intent while never bricking. + /// + private static CapabilityPolicy ClampPolicy( + DriverCapability capability, + CapabilityPolicy merged, + CapabilityPolicy tierDefault, + ref string? diagnostic) + { + var timeout = merged.TimeoutSeconds; + if (timeout <= 0) + { + AppendDiagnostic(ref diagnostic, + $"{capability}.timeoutSeconds {timeout} is non-positive; using the tier default ({tierDefault.TimeoutSeconds}s)."); + timeout = tierDefault.TimeoutSeconds; + } + else if (timeout > ResiliencePolicyRanges.MaxTimeoutSeconds) + { + AppendDiagnostic(ref diagnostic, + $"{capability}.timeoutSeconds {timeout} exceeds the max ({ResiliencePolicyRanges.MaxTimeoutSeconds}s); clamped."); + timeout = ResiliencePolicyRanges.MaxTimeoutSeconds; + } + + var retry = merged.RetryCount; + if (retry < ResiliencePolicyRanges.MinRetryCount) + { + AppendDiagnostic(ref diagnostic, $"{capability}.retryCount {retry} is negative; clamped to {ResiliencePolicyRanges.MinRetryCount}."); + retry = ResiliencePolicyRanges.MinRetryCount; + } + else if (retry > ResiliencePolicyRanges.MaxRetryCount) + { + AppendDiagnostic(ref diagnostic, $"{capability}.retryCount {retry} exceeds the cap ({ResiliencePolicyRanges.MaxRetryCount}); clamped."); + retry = ResiliencePolicyRanges.MaxRetryCount; + } + + var breaker = merged.BreakerFailureThreshold; + if (breaker < ResiliencePolicyRanges.MinBreakerFailureThreshold) + { + AppendDiagnostic(ref diagnostic, $"{capability}.breakerFailureThreshold {breaker} is negative; breaker disabled."); + breaker = ResiliencePolicyRanges.MinBreakerFailureThreshold; + } + else if (breaker == 1) + { + AppendDiagnostic(ref diagnostic, + $"{capability}.breakerFailureThreshold 1 is below Polly's minimum throughput; raised to {ResiliencePolicyRanges.MinEnabledBreakerFailureThreshold} (closest to open-on-first-failure intent)."); + breaker = ResiliencePolicyRanges.MinEnabledBreakerFailureThreshold; + } + else if (breaker > ResiliencePolicyRanges.MaxBreakerFailureThreshold) + { + AppendDiagnostic(ref diagnostic, + $"{capability}.breakerFailureThreshold {breaker} exceeds the cap ({ResiliencePolicyRanges.MaxBreakerFailureThreshold}); clamped."); + breaker = ResiliencePolicyRanges.MaxBreakerFailureThreshold; + } + + return merged with { TimeoutSeconds = timeout, RetryCount = retry, BreakerFailureThreshold = breaker }; + } + + /// Append a diagnostic to the running channel (joined with "; ") so multiple clamps all surface. + private static void AppendDiagnostic(ref string? diagnostic, string message) + => diagnostic = diagnostic is null ? message : $"{diagnostic}; {message}"; + private sealed class ResilienceConfigShape { /// Gets or sets the maximum concurrent bulkhead requests. diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Resilience/DriverResilienceOptionsParserTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Resilience/DriverResilienceOptionsParserTests.cs index ae02305f..17f28e84 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Resilience/DriverResilienceOptionsParserTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Resilience/DriverResilienceOptionsParserTests.cs @@ -222,4 +222,116 @@ public sealed class DriverResilienceOptionsParserTests options.RecycleIntervalSeconds.ShouldBeNull(); diag.ShouldContain("must be positive"); } + + // ---- 01/S-6: range clamps (clamp + diagnostic, never brick) ---- + + /// A non-positive timeout is nonsense; it snaps to the capability's tier default with a diagnostic. + [Fact] + public void TimeoutSeconds_Zero_FallsBackToTierDefault_WithDiagnostic() + { + var options = DriverResilienceOptionsParser.ParseOrDefaults( + DriverTier.A, "{\"capabilityPolicies\":{\"Read\":{\"timeoutSeconds\":0}}}", out var diag); + + var tierDefault = DriverResilienceOptions.GetTierDefaults(DriverTier.A)[DriverCapability.Read]; + options.Resolve(DriverCapability.Read).TimeoutSeconds.ShouldBe(tierDefault.TimeoutSeconds); + diag.ShouldNotBeNull(); + diag.ShouldContain("timeoutSeconds"); + } + + /// A timeout above Polly's ceiling clamps to the max with a diagnostic. + [Fact] + public void TimeoutSeconds_AbovePollyMax_Clamped() + { + var options = DriverResilienceOptionsParser.ParseOrDefaults( + DriverTier.A, "{\"capabilityPolicies\":{\"Read\":{\"timeoutSeconds\":999999}}}", out var diag); + + options.Resolve(DriverCapability.Read).TimeoutSeconds.ShouldBe(ResiliencePolicyRanges.MaxTimeoutSeconds); + diag.ShouldNotBeNull(); + diag.ShouldContain("timeoutSeconds"); + } + + /// A negative retry count clamps to zero with a diagnostic. + [Fact] + public void RetryCount_Negative_ClampedToZero() + { + var options = DriverResilienceOptionsParser.ParseOrDefaults( + DriverTier.A, "{\"capabilityPolicies\":{\"Read\":{\"retryCount\":-1}}}", out var diag); + + options.Resolve(DriverCapability.Read).RetryCount.ShouldBe(0); + diag.ShouldNotBeNull(); + diag.ShouldContain("retryCount"); + } + + /// An absurd retry count clamps to the operational cap with a diagnostic. + [Fact] + public void RetryCount_AboveCap_ClampedTo100() + { + var options = DriverResilienceOptionsParser.ParseOrDefaults( + DriverTier.A, "{\"capabilityPolicies\":{\"Read\":{\"retryCount\":2000000000}}}", out var diag); + + options.Resolve(DriverCapability.Read).RetryCount.ShouldBe(ResiliencePolicyRanges.MaxRetryCount); + diag.ShouldNotBeNull(); + diag.ShouldContain("retryCount"); + } + + /// A breaker threshold of 1 (below Polly's MinimumThroughput floor) raises to 2 with a diagnostic. + [Fact] + public void BreakerThreshold_One_ClampedToTwo() + { + var options = DriverResilienceOptionsParser.ParseOrDefaults( + DriverTier.A, "{\"capabilityPolicies\":{\"Read\":{\"breakerFailureThreshold\":1}}}", out var diag); + + options.Resolve(DriverCapability.Read).BreakerFailureThreshold.ShouldBe( + ResiliencePolicyRanges.MinEnabledBreakerFailureThreshold); + diag.ShouldNotBeNull(); + diag.ShouldContain("breakerFailureThreshold"); + } + + /// A negative breaker threshold disables the breaker (0) with a diagnostic. + [Fact] + public void BreakerThreshold_Negative_DisablesBreaker() + { + var options = DriverResilienceOptionsParser.ParseOrDefaults( + DriverTier.A, "{\"capabilityPolicies\":{\"Read\":{\"breakerFailureThreshold\":-5}}}", out var diag); + + options.Resolve(DriverCapability.Read).BreakerFailureThreshold.ShouldBe(0); + diag.ShouldNotBeNull(); + diag.ShouldContain("breakerFailureThreshold"); + } + + /// Multiple clamps in one policy all surface in the (appended) diagnostic string. + [Fact] + public void MultipleClamps_AllSurfaceInDiagnostic() + { + var options = DriverResilienceOptionsParser.ParseOrDefaults( + DriverTier.A, + "{\"capabilityPolicies\":{\"Read\":{\"timeoutSeconds\":0,\"retryCount\":-1,\"breakerFailureThreshold\":1}}}", + out var diag); + + diag.ShouldNotBeNull(); + diag.ShouldContain("timeoutSeconds"); + diag.ShouldContain("retryCount"); + diag.ShouldContain("breakerFailureThreshold"); + + var read = options.Resolve(DriverCapability.Read); + read.TimeoutSeconds.ShouldBe(DriverResilienceOptions.GetTierDefaults(DriverTier.A)[DriverCapability.Read].TimeoutSeconds); + read.RetryCount.ShouldBe(0); + read.BreakerFailureThreshold.ShouldBe(ResiliencePolicyRanges.MinEnabledBreakerFailureThreshold); + } + + /// In-range overrides pass through untouched with no diagnostic. + [Fact] + public void InRangeOverrides_PassThrough_NoDiagnostic() + { + var options = DriverResilienceOptionsParser.ParseOrDefaults( + DriverTier.A, + "{\"capabilityPolicies\":{\"Read\":{\"timeoutSeconds\":5,\"retryCount\":3,\"breakerFailureThreshold\":4}}}", + out var diag); + + diag.ShouldBeNull(); + var read = options.Resolve(DriverCapability.Read); + read.TimeoutSeconds.ShouldBe(5); + read.RetryCount.ShouldBe(3); + read.BreakerFailureThreshold.ShouldBe(4); + } }