diff --git a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md index 1220328e..d6f82684 100644 --- a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md +++ b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md @@ -657,3 +657,15 @@ for the orchestrator. remark is now qualified by node role and points at the bound, and `ConfigDbApiKeyStore` records both that it is wrapped in production and that "revocation on the next request" is **activation**-scoped. - Host.Tests: 123 green (was 106 at base). + +### T6 — `AclRevalidationTimer` + +- **RED observed:** `error CS0246: The type or namespace name 'AclRevalidationTimer' could not be found`. +- **DEVIATION (visibility):** the plan declares the class `internal sealed`. `ZB.MOM.WW.OtOpcUa.Host` has + **no `InternalsVisibleTo`**, so the tests could not reach it. Made `public sealed`, matching + `BoundedApiKeyStore` and every other type in `Host/OpcUa/`. +- Added beyond the plan's list: `A_tick_runs_the_pass` — the control for the two "nothing happened" cases + (a `Tick()` that silently did nothing would satisfy the throwing-pass and dispose cases on its own). +- 5 tests, green. Also corrected the T4 `MeterRecorder` remark: the assembly declares + `CollectionBehavior(DisableTestParallelization = true)`, which is a stronger and simpler reason the + name-only filter is safe than the one first written. diff --git a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json index 1928adba..db16b8fb 100644 --- a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json +++ b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json @@ -43,7 +43,7 @@ { "id": "T6", "subject": "AclRevalidationTimer (pure, TimeProvider-driven, non-throwing tick) + unit tests (03/S19)", - "status": "pending", + "status": "done", "blockedBy": [ "T1" ] diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/AclRevalidationTimer.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/AclRevalidationTimer.cs new file mode 100644 index 00000000..233c9ddb --- /dev/null +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/AclRevalidationTimer.cs @@ -0,0 +1,99 @@ +using Microsoft.Extensions.Logging; +using ZB.MOM.WW.OtOpcUa.Core.Authorization; + +namespace ZB.MOM.WW.OtOpcUa.Host.OpcUa; + +/// +/// Runs the node manager's monitored-item revalidation pass on a low-frequency tick, so a key that +/// expires by the CLOCK reaches a live subscription without waiting for a deployment (arch-review +/// 03/S19). +/// +/// +/// +/// Every request-shaped operation re-checks the calling key's usability per call and so honours +/// ExpiresUtc the moment it passes. A subscription is authorized once and then streams, and +/// streaming is how machine consumers actually read this server — so before this the only thing +/// that could stand a subscription down was a deployment. +/// +/// +/// ⚠️ The tick must never throw. An unhandled exception on a timer callback thread takes +/// the process down. Every tick is wrapped; a faulting pass is logged and the timer keeps running, +/// because the next tick is the recovery. +/// +/// +/// ⚠️ Each tick gets a fresh memo, and that is the pass's own constraint, not this class's. +/// RevalidateMonitoredItems allocates its verdict dictionary per call — see the remarks on +/// OtOpcUaNodeManager.OnAuthorityChanged. Nothing here may hoist it across ticks, or an +/// expiry landing between two ticks would be served the earlier tick's allow. +/// +/// +/// It adds no decision logic: the verdict comes from the authorizer's own +/// , and the stand-down (queue BadUserAccessDenied, then detach) +/// and its restore are the pass's, unchanged. The pass early-returns unless ACL enforcement is on, +/// so this is inert on the default configuration. +/// +/// +public sealed class AclRevalidationTimer : IDisposable +{ + private readonly Action _revalidate; + private readonly ILogger _log; + private readonly ITimer? _timer; + private volatile bool _disposed; + + /// Initializes a new instance of the class. + /// The revalidation pass to run on each tick. + /// + /// How often to run it. Non-positive arms no timer at all — the deploy-triggered-only behaviour + /// that predates 03/S19. + /// + /// The shared clock; its CreateTimer is what arms the tick. + /// The logger. + public AclRevalidationTimer(Action revalidate, TimeSpan interval, TimeProvider time, ILogger log) + { + ArgumentNullException.ThrowIfNull(revalidate); + ArgumentNullException.ThrowIfNull(time); + ArgumentNullException.ThrowIfNull(log); + + _revalidate = revalidate; + _log = log; + + if (interval <= TimeSpan.Zero) + { + // Say so once. A disabled guard that logs nothing is indistinguishable from a guard that is + // running and finding nothing, which is precisely the confusion 03/S19 is about. + _log.LogInformation( + "AclRevalidationTimer: disabled — {Section}:RevalidationIntervalSeconds is not positive, " + + "so live monitored items are re-authorized only when a deployment lands", + AclOptions.SectionName); + return; + } + + _timer = time.CreateTimer(_ => Tick(), state: null, interval, interval); + _log.LogInformation( + "AclRevalidationTimer: live monitored items will be re-authorized every {Interval}", interval); + } + + /// Runs one revalidation pass, absorbing anything it throws. + /// Exposed so a test can drive the callback body deterministically rather than by waiting. + public void Tick() + { + if (_disposed) return; + + try + { + _revalidate(); + } + catch (Exception ex) + { + _log.LogError(ex, "AclRevalidationTimer: the monitored-item revalidation pass threw; " + + "the timer keeps running and the next tick is the recovery"); + } + } + + /// + public void Dispose() + { + _disposed = true; + _timer?.Dispose(); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/AclRevalidationTimerTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/AclRevalidationTimerTests.cs new file mode 100644 index 00000000..fbe57857 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/AclRevalidationTimerTests.cs @@ -0,0 +1,151 @@ +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Host.OpcUa; + +namespace ZB.MOM.WW.OtOpcUa.Host.Tests.OpcUa; + +/// +/// Covers — the low-frequency trigger that makes key expiry reach +/// a live subscription without waiting for a deployment (arch-review 03/S19). +/// +public sealed class AclRevalidationTimerTests +{ + /// A positive interval arms one timer whose due time and period are both that interval. + [Fact] + public void A_positive_interval_creates_a_timer_with_that_period() + { + var clock = new RecordingTimeProvider(); + + using var _ = new AclRevalidationTimer( + () => { }, TimeSpan.FromSeconds(30), clock, NullLogger.Instance); + + clock.Created.Count.ShouldBe(1); + clock.Created[0].DueTime.ShouldBe(TimeSpan.FromSeconds(30)); + clock.Created[0].Period.ShouldBe(TimeSpan.FromSeconds(30)); + } + + /// + /// A non-positive interval arms nothing at all, and says so once — a disabled guard should be + /// visible in the log rather than inferred from an absence of activity. + /// + [Fact] + public void A_zero_interval_creates_no_timer() + { + var clock = new RecordingTimeProvider(); + var log = new CapturingLogger(); + + using var _ = new AclRevalidationTimer(() => { }, TimeSpan.Zero, clock, log); + + clock.Created.ShouldBeEmpty(); + log.Lines.ShouldContain(l => l.Contains("RevalidationIntervalSeconds", StringComparison.Ordinal)); + } + + /// + /// A pass that throws must not escape the tick. + /// + /// + /// An unhandled exception on a timer callback thread takes the process down. The next tick is the + /// recovery, so the fault is logged and the timer keeps running. + /// + [Fact] + public void A_throwing_pass_does_not_escape_the_tick() + { + var log = new CapturingLogger(); + using var timer = new AclRevalidationTimer( + () => throw new InvalidOperationException("pass blew up"), + TimeSpan.FromSeconds(30), + new RecordingTimeProvider(), + log); + + Should.NotThrow(timer.Tick); + log.Lines.ShouldContain(l => l.Contains("revalidation", StringComparison.OrdinalIgnoreCase)); + } + + /// A tick runs the pass — the control for the two "nothing happened" cases above. + [Fact] + public void A_tick_runs_the_pass() + { + var runs = 0; + using var timer = new AclRevalidationTimer( + () => runs++, TimeSpan.FromSeconds(30), new RecordingTimeProvider(), NullLogger.Instance); + + timer.Tick(); + timer.Tick(); + + runs.ShouldBe(2); + } + + /// Disposing disposes the underlying timer and stops further ticks. + [Fact] + public void Dispose_stops_further_ticks() + { + var clock = new RecordingTimeProvider(); + var runs = 0; + var timer = new AclRevalidationTimer( + () => runs++, TimeSpan.FromSeconds(30), clock, NullLogger.Instance); + + timer.Dispose(); + + clock.Created[0].Disposed.ShouldBeTrue(); + timer.Tick(); + runs.ShouldBe(0, "a disposed timer must not run the pass again"); + } + + // ---------- helpers ---------- + + /// + /// A that records the timers asked of it instead of arming real ones. + /// + private sealed class RecordingTimeProvider : TimeProvider + { + /// Every timer this provider was asked to create, in order. + public List Created { get; } = []; + + /// + public override ITimer CreateTimer(TimerCallback callback, object? state, TimeSpan dueTime, TimeSpan period) + { + var timer = new FakeTimer(dueTime, period); + Created.Add(timer); + return timer; + } + + /// A timer that does nothing but remember how it was asked for, and whether it was disposed. + internal sealed class FakeTimer(TimeSpan dueTime, TimeSpan period) : ITimer + { + public TimeSpan DueTime { get; } = dueTime; + + public TimeSpan Period { get; } = period; + + public bool Disposed { get; private set; } + + public bool Change(TimeSpan due, TimeSpan p) => true; + + public void Dispose() => Disposed = true; + + public ValueTask DisposeAsync() + { + Disposed = true; + return ValueTask.CompletedTask; + } + } + } + + /// Collects rendered log messages so a "said so once" claim can be asserted. + private sealed class CapturingLogger : ILogger + { + public List Lines { get; } = []; + + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) => Lines.Add(formatter(state, exception)); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreTests.cs index 74210583..fd521810 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreTests.cs @@ -421,11 +421,11 @@ public sealed class BoundedApiKeyStoreTests /// Collects otopcua.opcua.apikey_store.bounded measurements by their outcome tag. /// /// ⚠️ Filtered by instrument name only, which is normally the metrics-test leak (a listener sees - /// a PARALLEL test's measurements too). It is safe HERE for a checkable reason: + /// a PARALLEL test's measurements too). It is safe HERE for two checkable reasons: /// is the only thing in the product that writes this instrument, - /// and this class is the only one in the assembly that constructs one — and xUnit never runs two - /// tests of the same class in parallel. Adding a class that exercises the decorator means adding - /// a discriminating tag here. + /// and this assembly declares CollectionBehavior(DisableTestParallelization = true) in + /// AssemblyInfo.cs, so there is no parallel test to leak from. Removing that attribute + /// means adding a discriminating tag here. /// private sealed class MeterRecorder : IDisposable {