fix(host): run monitored-item revalidation on a low-frequency timer (archreview 03/S19)

2026-08-20 09:55:01 -04:00
parent b09f78396f
commit a4f12bccef
5 changed files with 228 additions and 1 deletions
@@ -687,3 +687,26 @@ for the orchestrator.
 - Two stale remarks corrected, not one: `OnAuthorityChanged`'s "this is the only trigger" **and**
  `IsSubscriptionStillPermitted`'s "a subscription is re-checked at a deployment, never on a timer".
 - OpcUaServer.Tests: 493 passed / 1 skipped.
+
+### T8 — wire the timer into `OtOpcUaServerHostedService`
+
+- **RED observed:** `error CS1061: 'OtOpcUaServerHostedService' does not contain a definition for
+  'StartRevalidationTimer'` and `error CS1729: … does not contain a constructor that takes 13 arguments`
+  (the two new ctor parameters).
+- **DEVIATION (test seam, and the reason for it):** the plan says "extend the hosted-service tests". **There
+  are none — `grep -rln OtOpcUaServerHostedService tests/` matched nothing**, because `StartAsync` boots
+  the real OPC UA SDK (PKI store, listening socket). A new suite,
+  `OtOpcUaServerHostedServiceRevalidationTests`, drives the timer's lifecycle through an **`internal`**
+  `StartRevalidationTimer()` (new `InternalsVisibleTo` on the Host csproj, the convention six driver
+  projects already use). Everything the timer's lifecycle touches — the options, the clock, the disposal —
+  is exercised exactly as production reaches it; only the SDK boot is skipped.
+- **DEVIATION (assertion not made):** the plan asks the test to assert disposal happens **before**
+  `SetDataPlaneAuthorizer(null)`. That is **not observable**: both detaches go through `_server?.…`, and
+  `OtOpcUaSdkServer.NodeManager` is created only by the SDK's own `CreateMasterNodeManager` during a real
+  boot, so with no server both calls are no-ops. The ordering is instead guaranteed structurally — the
+  dispose is the **first statement** of `StopAsync`, with a comment saying why — and what IS asserted is
+  that `StopAsync` (and `DisposeAsync`) dispose the timer, with a non-vacuity check that it was live
+  beforehand. **Reviewer: this is the one plan assertion downgraded rather than met.**
+- The node manager is resolved **per tick** (`() => _server?.NodeManager?.Revalidate…`) rather than
+  captured, so a restart cannot leave the tick walking a dead manager's item map.
+- Host.Tests: 132 green.
@@ -59,7 +59,7 @@
    {
      "id": "T8",
      "subject": "Wire the revalidation timer into OtOpcUaServerHostedService (create after StartAsync, dispose FIRST in StopAsync) + ordering test",
-      "status": "pending",
+      "status": "done",
      "blockedBy": [
        "T6",
        "T7"
@@ -38,6 +38,8 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl
    private readonly IDataPlaneAuthoritySource _dataPlaneAuthority;
    private readonly DataPlaneAuthorizer? _dataPlaneAuthorizer;
    private readonly ServerHistorianOptions _serverHistorianOptions;
+    private readonly AclOptions _aclOptions;
+    private readonly TimeProvider _time;
    private readonly Func<ActorSystem> _actorSystemAccessor;
    private readonly ActorRegistry _actorRegistry;
    private readonly ILoggerFactory _loggerFactory;
@@ -45,6 +47,7 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl

    private OpcUaApplicationHost? _appHost;
    private OtOpcUaSdkServer? _server;
+    private AclRevalidationTimer? _revalidationTimer;

    /// <summary>
    /// Initializes a new instance of the OtOpcUaServerHostedService class.
@@ -77,6 +80,13 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl
    /// <param name="dataPlaneAuthorizer">#520 stage 5 — the node-scoped ACL gate. Nullable, and null means
    /// "not wired" rather than "deny": an unregistered gate must not lock a node out of writes to protect a
    /// feature that is off by default. Once wired AND enabled, every unresolvable case inside it denies.</param>
+    /// <param name="aclOptions">#520 / arch-review 03/S19 — the <c>Security:Acl</c> options, read for
+    /// <see cref="AclOptions.RevalidationIntervalSeconds"/>. Required, not optional: the built-in DI
+    /// container does not fall back to a default for an unregistered service, the same trap the
+    /// <c>IOpcUaUserAuthenticator</c> and <c>IDataPlaneAuthoritySource</c> parameters carry comments
+    /// about.</param>
+    /// <param name="time">The shared clock — the one instance the authorizer and the API-key store bound
+    /// also read. Its <c>CreateTimer</c> arms the revalidation tick.</param>
    /// <param name="loggerFactory">The logger factory for creating loggers.</param>
    public OtOpcUaServerHostedService(
        IOptions<OpcUaApplicationHostOptions> options,
@@ -89,9 +99,16 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl
        Func<ActorSystem> actorSystemAccessor,
        ActorRegistry actorRegistry,
        DataPlaneAuthorizer? dataPlaneAuthorizer,
+        IOptions<AclOptions> aclOptions,
+        TimeProvider time,
        ILoggerFactory loggerFactory)
    {
+        ArgumentNullException.ThrowIfNull(aclOptions);
+        ArgumentNullException.ThrowIfNull(time);
+
        _dataPlaneAuthorizer = dataPlaneAuthorizer;
+        _aclOptions = aclOptions.Value;
+        _time = time;
        _options = options.Value;
        _deferredSink = deferredSink;
        _deferredServiceLevel = deferredServiceLevel;
@@ -269,9 +286,34 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl
                _loggerFactory.CreateLogger<SdkServiceLevelPublisher>()));
        }

+        // arch-review 03/S19 — LAST, after the node manager is known non-null and after the ACL gate is
+        // attached, so the first tick cannot land on a half-wired server.
+        StartRevalidationTimer();
+
        _logger.LogInformation("OtOpcUaServerHostedService: SDK started, address-space + ServiceLevel sinks bound");
    }

+    /// <summary>
+    ///     Arms the clock-driven monitored-item revalidation tick (arch-review 03/S19).
+    /// </summary>
+    /// <remarks>
+    ///     Created unconditionally: the pass it drives early-returns unless the ACL gate is wired AND
+    ///     <c>Security:Acl:Enabled</c> is on, so it stays inert on the default configuration, and
+    ///     <c>RevalidationIntervalSeconds = 0</c> arms no timer at all.
+    ///     <para>
+    ///         The node manager is resolved <b>per tick</b> rather than captured: <see cref="_server"/> is
+    ///         replaced across a restart, and a captured reference would go on revalidating a dead
+    ///         manager's item map — the same detach hazard the authority-source setter documents.
+    ///     </para>
+    ///     <para>Separate from <see cref="StartAsync"/> so the timer's lifecycle is testable without booting the SDK.</para>
+    /// </remarks>
+    internal void StartRevalidationTimer() =>
+        _revalidationTimer = new AclRevalidationTimer(
+            () => _server?.NodeManager?.RevalidateMonitoredItems(),
+            TimeSpan.FromSeconds(_aclOptions.RevalidationIntervalSeconds),
+            _time,
+            _loggerFactory.CreateLogger<AclRevalidationTimer>());
+
    /// <summary>
    /// Stops the OPC UA server asynchronously.
    /// </summary>
@@ -279,6 +321,12 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl
    /// <returns>A task that represents the asynchronous operation.</returns>
    public Task StopAsync(CancellationToken cancellationToken)
    {
+        // FIRST, ahead of every detach below (arch-review 03/S19). A tick landing on a node manager whose
+        // authority and gate have already been cleared is the shape this whole teardown block exists to
+        // prevent — a stopping node sheds the trigger before it sheds what the trigger drives.
+        _revalidationTimer?.Dispose();
+        _revalidationTimer = null;
+
        // Revert to Null adapters so any in-flight writes from a poison-pilled actor don't hit a
        // half-disposed NodeManager.
        _deferredSink.SetSink(null);
@@ -304,6 +352,11 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl
    /// <returns>A task that represents the asynchronous operation.</returns>
    public async ValueTask DisposeAsync()
    {
+        // Before the app host, and idempotent with StopAsync — a host that disposes without stopping must
+        // not leave a tick armed against a server that is about to go away.
+        _revalidationTimer?.Dispose();
+        _revalidationTimer = null;
+
        if (_appHost is not null) await _appHost.DisposeAsync();
    }
 }
@@ -13,6 +13,13 @@
        <NoWarn>$(NoWarn);NU1608</NoWarn>
    </PropertyGroup>

+    <ItemGroup>
+        <!-- OtOpcUaServerHostedService.StartRevalidationTimer is internal: it is a lifecycle seam the
+             tests drive so the ACL revalidation timer (arch-review 03/S19) can be asserted without
+             booting the real OPC UA SDK, not a surface for callers. -->
+        <InternalsVisibleTo Include="ZB.MOM.WW.OtOpcUa.Host.Tests"/>
+    </ItemGroup>
+
    <ItemGroup>
        <PackageReference Include="Akka.Hosting"/>
        <PackageReference Include="Serilog.AspNetCore"/>
@@ -0,0 +1,144 @@
+using Akka.Actor;
+using Akka.Hosting;
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.Logging.Abstractions;
+using Microsoft.Extensions.Options;
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Commons.OpcUa;
+using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
+using ZB.MOM.WW.OtOpcUa.Core.Authorization;
+using ZB.MOM.WW.OtOpcUa.Host.OpcUa;
+using ZB.MOM.WW.OtOpcUa.OpcUaServer;
+using ZB.MOM.WW.OtOpcUa.OpcUaServer.Security;
+
+namespace ZB.MOM.WW.OtOpcUa.Host.Tests.OpcUa;
+
+/// <summary>
+///     Pins the ACL revalidation timer's lifecycle on the hosted service (arch-review 03/S19) — that it is
+///     armed at the configured interval, that <c>0</c> arms nothing, and that stopping disposes it.
+/// </summary>
+/// <remarks>
+///     The timer is armed through <see cref="OtOpcUaServerHostedService.StartRevalidationTimer"/> rather
+///     than by running <c>StartAsync</c>, because <c>StartAsync</c> boots the real OPC UA SDK (certificate
+///     store, listening socket) and none of that is what this is about. Everything the timer's lifecycle
+///     touches — the options, the clock, the disposal — is exercised exactly as production reaches it.
+/// </remarks>
+public sealed class OtOpcUaServerHostedServiceRevalidationTests
+{
+    /// <summary>The configured interval is what the timer is armed at.</summary>
+    [Fact]
+    public void A_configured_interval_arms_one_timer_at_that_period()
+    {
+        var clock = new RecordingTimeProvider();
+        var sut = Service(clock, intervalSeconds: 30);
+
+        sut.StartRevalidationTimer();
+
+        clock.Periods.ShouldBe([TimeSpan.FromSeconds(30)]);
+    }
+
+    /// <summary>
+    ///     <c>RevalidationIntervalSeconds = 0</c> arms nothing — the documented rollback to
+    ///     deploy-triggered-only revalidation.
+    /// </summary>
+    [Fact]
+    public void A_zero_interval_arms_nothing()
+    {
+        var clock = new RecordingTimeProvider();
+        var sut = Service(clock, intervalSeconds: 0);
+
+        sut.StartRevalidationTimer();
+
+        clock.Periods.ShouldBeEmpty();
+    }
+
+    /// <summary>
+    ///     <c>StopAsync</c> disposes the timer, so no tick can land on a node manager that is being torn
+    ///     down.
+    /// </summary>
+    /// <remarks>
+    ///     The disposal is the FIRST thing <c>StopAsync</c> does, ahead of
+    ///     <c>SetDataPlaneAuthority(null)</c> / <c>SetDataPlaneAuthorizer(null)</c> — a stopping node sheds
+    ///     the trigger before it sheds the thing the trigger drives.
+    /// </remarks>
+    [Fact]
+    public async Task StopAsync_disposes_the_timer()
+    {
+        var clock = new RecordingTimeProvider();
+        var sut = Service(clock, intervalSeconds: 30);
+        sut.StartRevalidationTimer();
+
+        clock.Timers[0].Disposed.ShouldBeFalse("non-vacuity: the timer is live before the stop");
+
+        await sut.StopAsync(CancellationToken.None);
+
+        clock.Timers[0].Disposed.ShouldBeTrue();
+    }
+
+    /// <summary>Disposing the service disposes the timer too, for a host that never called StopAsync.</summary>
+    [Fact]
+    public async Task DisposeAsync_disposes_the_timer()
+    {
+        var clock = new RecordingTimeProvider();
+        var sut = Service(clock, intervalSeconds: 30);
+        sut.StartRevalidationTimer();
+
+        await sut.DisposeAsync();
+
+        clock.Timers[0].Disposed.ShouldBeTrue();
+    }
+
+    // ---------- helpers ----------
+
+    private static OtOpcUaServerHostedService Service(TimeProvider clock, int intervalSeconds) => new(
+        Options.Create(new OpcUaApplicationHostOptions()),
+        new DeferredAddressSpaceSink(),
+        new DeferredServiceLevelPublisher(),
+        NullOpcUaUserAuthenticator.Instance,
+        NullHistorianDataSource.Instance,
+        EmptyDataPlaneAuthoritySource.Instance,
+        new ConfigurationBuilder().Build(),
+        () => throw new NotSupportedException("no ActorSystem is needed for the timer lifecycle"),
+        new ActorRegistry(),
+        dataPlaneAuthorizer: null,
+        Options.Create(new AclOptions { RevalidationIntervalSeconds = intervalSeconds }),
+        clock,
+        NullLoggerFactory.Instance);
+
+    /// <summary>Records the timers asked of it instead of arming real ones.</summary>
+    private sealed class RecordingTimeProvider : TimeProvider
+    {
+        /// <summary>Every timer this provider was asked to create, in order.</summary>
+        public List<FakeTimer> Timers { get; } = [];
+
+        /// <summary>The period each of those timers was armed at.</summary>
+        public IReadOnlyList<TimeSpan> Periods => [.. Timers.Select(t => t.Period)];
+
+        /// <inheritdoc />
+        public override ITimer CreateTimer(TimerCallback callback, object? state, TimeSpan dueTime, TimeSpan period)
+        {
+            var timer = new FakeTimer(period);
+            Timers.Add(timer);
+            return timer;
+        }
+
+        /// <summary>A timer that remembers its period and whether it was disposed.</summary>
+        internal sealed class FakeTimer(TimeSpan period) : ITimer
+        {
+            public TimeSpan Period { get; } = period;
+
+            public bool Disposed { get; private set; }
+
+            public bool Change(TimeSpan dueTime, TimeSpan p) => true;
+
+            public void Dispose() => Disposed = true;
+
+            public ValueTask DisposeAsync()
+            {
+                Disposed = true;
+                return ValueTask.CompletedTask;
+            }
+        }
+    }
+}