test(opcua): pin clock-driven expiry in the revalidation pass; correct the only-trigger remark

archreview/plans/R3-05-acl-hot-path-expiry-plan.md

@@ -669,3 +669,21 @@ for the orchestrator.
 - 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.
+
+### T7 — node-manager expiry tests + xmldoc correction
+
+- **RED: the three new cases PASSED on first run — as the plan predicted** ("the expiry case may already
+  PASS … that is the point: it pins the property the timer will rely on before the timer exists"). The
+  pass already reads the authorizer's `TimeProvider`, so no production change was needed.
+- **Falsification run (recorded, since "it passed immediately" proves nothing on its own):** shortening the
+  clock advance in `An_expired_key_stands_down_a_live_item_with_no_deploy` from 20 min to 5 min (i.e.
+  short of `ExpiresUtc`) turned it **`[FAIL]`**, then restored. So the assertion tracks the clock rather
+  than a blanket deny.
+- `A_renewed_key_re_attaches_the_item_on_the_next_pass` observes the restore **indirectly and
+  falsifiably**: the pass skips an already-suppressed item, so standing the item down a second time can
+  only succeed if it was genuinely re-attached in between. (`NodeHandle.MonitoredNode` is null on a
+  hand-tracked stub item, so attach/detach is not directly observable at this seam — it is proven over the
+  wire in T9.)
+- 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.

archreview/plans/R3-05-acl-hot-path-expiry-plan.md.tasks.json

@@ -51,7 +51,7 @@
     {
       "id": "T7",
       "subject": "Node-manager expiry tests (FakeTimeProvider advanced past ExpiresUtc, no deploy) + correct the ':316-328' only-trigger xmldoc",
-      "status": "pending",
+      "status": "done",
       "blockedBy": [
         "T1"
       ]

src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs

@@ -317,12 +317,21 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
     ///     Re-authorizes live subscriptions when a deployment installs a new authorization generation.
     /// </summary>
     /// <remarks>
-    ///     ⚠️ <b>This is the only trigger, and the pass's memo depends on that.</b>
-    ///     <see cref="IsSubscriptionStillPermitted"/> memoizes key revocation and expiry for the span of one
-    ///     pass — safe only while revalidation is deploy-triggered, because a pass is then a momentary
-    ///     event whose verdicts cannot go stale within it. A future periodic trigger must give each tick a
-    ///     fresh memo (it already does — the dictionary is pass-local) and must not be tempted to reuse one
-    ///     across ticks, or an expiry landing between ticks would be served the previous tick's allow.
+    ///     <para>
+    ///         <b>There are TWO triggers</b> (arch-review 03/S19): this one, the deployment axis; and the
+    ///         host's low-frequency <c>AclRevalidationTimer</c>, the clock axis — a key's
+    ///         <c>ExpiresUtc</c> passes with the clock, not with a deployment, so a deploy-only trigger
+    ///         left an expired key streaming to a live subscription indefinitely on a fleet that had been
+    ///         deployed once and left running.
+    ///     </para>
+    ///     <para>
+    ///         ⚠️ <b>The second trigger is safe only because the memo is pass-local.</b>
+    ///         <see cref="IsSubscriptionStillPermitted"/> memoizes key revocation and expiry for the span
+    ///         of one pass, which is sound because a pass is a momentary event whose verdicts cannot go
+    ///         stale within it — its dictionary is constructed per call. Hoisting it across ticks, to save
+    ///         the walk, would serve an expiry that landed between two ticks the earlier tick's allow, and
+    ///         re-open exactly the gap the timer closes.
+    ///     </para>
     /// </remarks>
     /// <param name="snapshot">The newly published snapshot; unused — the current state is re-read per item.</param>
     private void OnAuthorityChanged(DataPlaneAuthoritySnapshot snapshot) => RevalidateMonitoredItems();
@@ -3311,9 +3320,10 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
     ///     </para>
     ///     <para>
     ///         This memo does cover the key expiry check that the authorizer's own cache excludes, but only
-    ///         for the duration of one pass — and a subscription is re-checked at a deployment, never on a
-    ///         timer, so an expiry that lands inside a single pass was already going to be caught by the
-    ///         next deployment rather than by this one.
+    ///         for the duration of one pass. Since arch-review 03/S19 a subscription is re-checked on the
+    ///         host's low-frequency timer as well as at a deployment, so an expiry that lands inside a
+    ///         single pass is caught by the next TICK — one interval later, not one deployment later. What
+    ///         makes that hold is that the memo is constructed per call and never reused across passes.
     ///     </para>
     /// </remarks>
     private bool IsSubscriptionStillPermitted(

tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerStage6EnforcementTests.cs

@@ -615,8 +615,156 @@ public sealed class NodeManagerStage6EnforcementTests : IDisposable
         finally { await host.DisposeAsync(); }
     }
 
+    /// <summary>
+    ///     A key that expires by the CLOCK stands its live monitored item down, with no deployment
+    ///     anywhere in the test (arch-review 03/S19).
+    /// </summary>
+    /// <remarks>
+    ///     <para>
+    ///         The first pass is the falsification control, and it is not optional: without it a build
+    ///         whose pass detached everything it saw — unconditionally, or because the authorizer denied
+    ///         for some unrelated reason — would pass the second half and prove nothing about expiry.
+    ///     </para>
+    ///     <para>
+    ///         <c>DataPlaneAuthorityState.Apply</c> is called exactly once here, in setup. Nothing
+    ///         advances the generation between the two passes, so the only thing that changed is the
+    ///         clock — which is the whole claim.
+    ///     </para>
+    /// </remarks>
+    [Fact]
+    public async Task An_expired_key_stands_down_a_live_item_with_no_deploy()
+    {
+        var (host, server) = await BootAsync();
+        try
+        {
+            var nm = server.NodeManager!;
+            var clock = new FakeTimeProvider(new DateTime(2026, 8, 20, 12, 0, 0, DateTimeKind.Utc));
+            nm.Authorizer = KeyedSubscribeAuthorizer(
+                clock, out _, expiresUtc: new DateTime(2026, 8, 20, 12, 10, 0, DateTimeKind.Utc));
+
+            var item = TrackItem(nm, Variable(nm, "Plant/Modbus/dev1/Speed"), KeyedIdentity());
+
+            nm.RevalidateMonitoredItems();
+            item.Denials.ShouldBe(0, "the key is still valid, so nothing may stand down");
+
+            clock.Advance(TimeSpan.FromMinutes(20));
+            nm.RevalidateMonitoredItems();
+
+            item.Denials.ShouldBe(1);
+        }
+        finally { await host.DisposeAsync(); }
+    }
+
+    /// <summary>
+    ///     A key renewed with a later expiry re-attaches its item on the next pass — the clock axis
+    ///     inherits the deploy axis's reversibility.
+    /// </summary>
+    /// <remarks>
+    ///     The restore is asserted by standing the item down a SECOND time: the pass skips an item that is
+    ///     already suppressed, so a second denial can only be queued if the item was genuinely restored
+    ///     in between. That is why this pins the design decision not to close the session — a client that
+    ///     renews its key recovers with no action of its own.
+    /// </remarks>
+    [Fact]
+    public async Task A_renewed_key_re_attaches_the_item_on_the_next_pass()
+    {
+        var (host, server) = await BootAsync();
+        try
+        {
+            var nm = server.NodeManager!;
+            var clock = new FakeTimeProvider(new DateTime(2026, 8, 20, 12, 0, 0, DateTimeKind.Utc));
+            nm.Authorizer = KeyedSubscribeAuthorizer(
+                clock, out var state, expiresUtc: new DateTime(2026, 8, 20, 12, 10, 0, DateTimeKind.Utc));
+
+            var item = TrackItem(nm, Variable(nm, "Plant/Modbus/dev1/Speed"), KeyedIdentity());
+
+            clock.Advance(TimeSpan.FromMinutes(20));
+            nm.RevalidateMonitoredItems();
+            item.Denials.ShouldBe(1, "non-vacuity: the item really was stood down before the renewal");
+
+            // Renew: same key id, a later expiry.
+            state.Apply(
+                Cluster, "rev-2",
+                [Key(new DateTime(2026, 8, 20, 14, 0, 0, DateTimeKind.Utc))],
+                [GrantOn(NodePermissions.Subscribe)],
+                Scopes());
+            nm.RevalidateMonitoredItems();
+
+            // Expire it again. A still-suppressed item is skipped, so a second denial proves the restore.
+            clock.Advance(TimeSpan.FromHours(3));
+            nm.RevalidateMonitoredItems();
+
+            item.Denials.ShouldBe(2);
+        }
+        finally { await host.DisposeAsync(); }
+    }
+
+    /// <summary>A clock that moves but does not reach the expiry stands nothing down.</summary>
+    [Fact]
+    public async Task An_unexpired_key_is_untouched_by_a_clock_only_pass()
+    {
+        var (host, server) = await BootAsync();
+        try
+        {
+            var nm = server.NodeManager!;
+            var clock = new FakeTimeProvider(new DateTime(2026, 8, 20, 12, 0, 0, DateTimeKind.Utc));
+            nm.Authorizer = KeyedSubscribeAuthorizer(
+                clock, out _, expiresUtc: new DateTime(2026, 8, 20, 13, 0, 0, DateTimeKind.Utc));
+
+            var item = TrackItem(nm, Variable(nm, "Plant/Modbus/dev1/Speed"), KeyedIdentity());
+
+            clock.Advance(TimeSpan.FromMinutes(30));
+            nm.RevalidateMonitoredItems();
+
+            item.Denials.ShouldBe(0);
+        }
+        finally { await host.DisposeAsync(); }
+    }
+
     // ───────────────────────────── helpers ─────────────────────────────
 
+    /// <summary>The public id of the key the clock-driven expiry cases authenticate with.</summary>
+    private const string ExpiringKeyId = "k-expiry";
+
+    /// <summary>
+    ///     A cluster-wide Subscribe grant PLUS a key that expires at <paramref name="expiresUtc"/>, so the
+    ///     verdict moves with the clock rather than with a deployment.
+    /// </summary>
+    /// <param name="clock">The authorizer's clock — the one <c>Evaluate</c> checks expiry against.</param>
+    /// <param name="state">The authority state, so a test can renew the key without a second helper.</param>
+    /// <param name="expiresUtc">When the key stops being usable.</param>
+    /// <returns>The gate, enforcing.</returns>
+    private static DataPlaneAuthorizer KeyedSubscribeAuthorizer(
+        TimeProvider clock, out DataPlaneAuthorityState state, DateTime expiresUtc)
+    {
+        state = new DataPlaneAuthorityState();
+        state.Apply(Cluster, "rev-1", [Key(expiresUtc)], [GrantOn(NodePermissions.Subscribe)], Scopes());
+        return new DataPlaneAuthorizer(
+            state, new TriePermissionEvaluator(state.Tries), enabled: true, NullLogger.Instance, clock);
+    }
+
+    /// <summary>The expiring key, at whatever expiry a case needs.</summary>
+    /// <param name="expiresUtc">When the key stops being usable.</param>
+    /// <returns>The key row.</returns>
+    private static ApiKey Key(DateTime expiresUtc) => new()
+    {
+        ApiKeyId = ExpiringKeyId,
+        Name = ExpiringKeyId,
+        SecretHash = "hash",
+        Principal = Principal,
+        Enabled = true,
+        CreatedUtc = new DateTime(2026, 8, 20, 0, 0, 0, DateTimeKind.Utc),
+        ExpiresUtc = expiresUtc,
+    };
+
+    /// <summary>
+    ///     An identity carrying the key id, which is what makes the per-call expiry re-check apply — an
+    ///     identity with no <c>ApiKeyId</c> skips it entirely.
+    /// </summary>
+    /// <returns>The identity.</returns>
+    private static RoleCarryingUserIdentity KeyedIdentity() =>
+        new(new UserNameIdentityToken { UserName = "apikey" }, [], Principal, ExpiringKeyId);
+
     /// <summary>A cluster-wide Subscribe grant — what <c>CreateMonitoredItems</c> resolves to.</summary>
     private static DataPlaneAuthorizer SubscribeAuthorizer(TimeProvider clock)
     {