From a4f12bccef77008fca94a8283bdd6a065c6e494d Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 09:55:01 -0400 Subject: [PATCH] fix(host): run monitored-item revalidation on a low-frequency timer (archreview 03/S19) --- .../plans/R3-05-acl-hot-path-expiry-plan.md | 23 +++ ...-05-acl-hot-path-expiry-plan.md.tasks.json | 2 +- .../OpcUa/OtOpcUaServerHostedService.cs | 53 +++++++ .../ZB.MOM.WW.OtOpcUa.Host.csproj | 7 + ...cUaServerHostedServiceRevalidationTests.cs | 144 ++++++++++++++++++ 5 files changed, 228 insertions(+), 1 deletion(-) create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/OtOpcUaServerHostedServiceRevalidationTests.cs 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 81c1d238..57f5c808 100644 --- a/archreview/plans/R3-05-acl-hot-path-expiry-plan.md +++ b/archreview/plans/R3-05-acl-hot-path-expiry-plan.md @@ -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. 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 b2a67fb8..bbc19b89 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 @@ -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" diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs index d05d5abf..33a0bb70 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/OtOpcUaServerHostedService.cs @@ -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 _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; /// /// Initializes a new instance of the OtOpcUaServerHostedService class. @@ -77,6 +80,13 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl /// #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. + /// #520 / arch-review 03/S19 — the Security:Acl options, read for + /// . Required, not optional: the built-in DI + /// container does not fall back to a default for an unregistered service, the same trap the + /// IOpcUaUserAuthenticator and IDataPlaneAuthoritySource parameters carry comments + /// about. + /// The shared clock — the one instance the authorizer and the API-key store bound + /// also read. Its CreateTimer arms the revalidation tick. /// The logger factory for creating loggers. public OtOpcUaServerHostedService( IOptions options, @@ -89,9 +99,16 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl Func actorSystemAccessor, ActorRegistry actorRegistry, DataPlaneAuthorizer? dataPlaneAuthorizer, + IOptions 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())); } + // 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"); } + /// + /// Arms the clock-driven monitored-item revalidation tick (arch-review 03/S19). + /// + /// + /// Created unconditionally: the pass it drives early-returns unless the ACL gate is wired AND + /// Security:Acl:Enabled is on, so it stays inert on the default configuration, and + /// RevalidationIntervalSeconds = 0 arms no timer at all. + /// + /// The node manager is resolved per tick rather than captured: 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. + /// + /// Separate from so the timer's lifecycle is testable without booting the SDK. + /// + internal void StartRevalidationTimer() => + _revalidationTimer = new AclRevalidationTimer( + () => _server?.NodeManager?.RevalidateMonitoredItems(), + TimeSpan.FromSeconds(_aclOptions.RevalidationIntervalSeconds), + _time, + _loggerFactory.CreateLogger()); + /// /// Stops the OPC UA server asynchronously. /// @@ -279,6 +321,12 @@ public sealed class OtOpcUaServerHostedService : IHostedService, IAsyncDisposabl /// A task that represents the asynchronous operation. 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 /// A task that represents the asynchronous operation. 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(); } } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/ZB.MOM.WW.OtOpcUa.Host.csproj b/src/Server/ZB.MOM.WW.OtOpcUa.Host/ZB.MOM.WW.OtOpcUa.Host.csproj index 1819d602..cc80ef36 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/ZB.MOM.WW.OtOpcUa.Host.csproj +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/ZB.MOM.WW.OtOpcUa.Host.csproj @@ -13,6 +13,13 @@ $(NoWarn);NU1608 + + + + + diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/OtOpcUaServerHostedServiceRevalidationTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/OtOpcUaServerHostedServiceRevalidationTests.cs new file mode 100644 index 00000000..96c989f8 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/OtOpcUaServerHostedServiceRevalidationTests.cs @@ -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; + +/// +/// Pins the ACL revalidation timer's lifecycle on the hosted service (arch-review 03/S19) — that it is +/// armed at the configured interval, that 0 arms nothing, and that stopping disposes it. +/// +/// +/// The timer is armed through rather +/// than by running StartAsync, because StartAsync 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. +/// +public sealed class OtOpcUaServerHostedServiceRevalidationTests +{ + /// The configured interval is what the timer is armed at. + [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)]); + } + + /// + /// RevalidationIntervalSeconds = 0 arms nothing — the documented rollback to + /// deploy-triggered-only revalidation. + /// + [Fact] + public void A_zero_interval_arms_nothing() + { + var clock = new RecordingTimeProvider(); + var sut = Service(clock, intervalSeconds: 0); + + sut.StartRevalidationTimer(); + + clock.Periods.ShouldBeEmpty(); + } + + /// + /// StopAsync disposes the timer, so no tick can land on a node manager that is being torn + /// down. + /// + /// + /// The disposal is the FIRST thing StopAsync does, ahead of + /// SetDataPlaneAuthority(null) / SetDataPlaneAuthorizer(null) — a stopping node sheds + /// the trigger before it sheds the thing the trigger drives. + /// + [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(); + } + + /// Disposing the service disposes the timer too, for a host that never called StopAsync. + [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); + + /// 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 Timers { get; } = []; + + /// The period each of those timers was armed at. + public IReadOnlyList Periods => [.. Timers.Select(t => t.Period)]; + + /// + public override ITimer CreateTimer(TimerCallback callback, object? state, TimeSpan dueTime, TimeSpan period) + { + var timer = new FakeTimer(period); + Timers.Add(timer); + return timer; + } + + /// A timer that remembers its period and whether it was disposed. + 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; + } + } + } +}