diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs index 3f544aee..ea44d403 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs @@ -252,9 +252,16 @@ public sealed class SessionAclDecisionCache // before, and it is accepted — the loss window is a single compare-and-swap wide, and it is // paid only by threads that overflowed the same shard simultaneously, against a refill cost // paid by every eviction. + // + // A shard from a PREVIOUS generation contributes no hint: the fill level it reached belongs to a + // set of grants that no longer exists, and it is about to be displaced by the generation check + // below in any case. Sizing this generation's storage from the last one's crawl would be a guess + // dressed as evidence. shard = Displace( sessionKey, shard, generation, - capacityHint: Math.Clamp(Volatile.Read(ref shard.ApproxCount), 0, MaxEntriesPerSession)); + capacityHint: shard.Generation == generation + ? Math.Clamp(Volatile.Read(ref shard.ApproxCount), 0, MaxEntriesPerSession) + : 0); } // The shard must belong to THIS generation before anything is filed in it. Reached in two ways: a @@ -420,10 +427,33 @@ public sealed class SessionAclDecisionCache internal void AdvanceGenerationForTest(long generation) => Interlocked.Exchange(ref _generation, generation); + /// + /// Advances the cache-wide stamp when this call presents a newer generation, and purges on the way. + /// + /// + /// + /// ⚠️ The comparison is <=, not !=, and that is a straggler guard. The + /// generation only ever increases (DataPlaneAuthorityState counts it up rather than + /// deriving it from the revision hash, precisely so it cannot go backwards), but a request + /// can still present an older one: it captured its authority snapshot before a deploy and then + /// lost the scheduling race to a request that captured after. An inequality test treats that + /// straggler as a deploy, rolls the stamp backward, and purges the new generation's + /// freshly-populated shards — so the post-deploy window, when the cache is coldest and doing the + /// most work, is exactly when it thrashes. Verdicts are safe either way now that each shard + /// carries its own generation; what is at stake here is only whether the cache is any use. + /// + /// + /// The straggler's own lookup is unaffected by returning early: its shard-generation check misses + /// against the newer shards regardless, so it recomputes, which is the correct outcome for a + /// request holding a superseded snapshot. _generation starts at -1, below any real + /// generation, so the first one still passes. + /// + /// + /// The generation the calling request is being decided under. private void InvalidateIfStale(long generation) { var seen = Interlocked.Read(ref _generation); - if (seen == generation) return; + if (generation <= seen) return; // Only the thread that actually moves the stamp clears, so a burst of concurrent requests arriving // just after a deploy repopulates once rather than each clearing the others' fresh entries. diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SessionAclDecisionCacheTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SessionAclDecisionCacheTests.cs index e4aead0d..5bf36a05 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SessionAclDecisionCacheTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SessionAclDecisionCacheTests.cs @@ -263,6 +263,48 @@ public sealed class SessionAclDecisionCacheTests } } + /// + /// A request presenting an OLDER generation — one whose authority snapshot was captured before a + /// deploy and which then lost the scheduling race — must not roll the cache stamp backward, and must + /// not purge the new generation's entries on its way past. + /// + /// + /// + /// The generation itself is monotonic by construction; what is not monotonic is the order + /// requests arrive holding it. Treating a straggler as a deploy inverts the stamp and + /// wipes shards that were populated correctly, so the post-deploy window — the coldest the cache + /// ever is, and the busiest repopulating — is precisely when it would thrash. + /// + /// + /// Nothing here uses AdvanceGenerationForTest: this is the ordinary lookup path, + /// observed. The straggler must miss (its snapshot is superseded, so recomputing is right) AND + /// leave everything else standing — a test asserting only the miss passes against the defect, + /// because the straggler misses either way. + /// + /// + [Fact] + public void A_straggler_from_an_older_generation_neither_clears_nor_rolls_the_stamp_back() + { + var cache = new SessionAclDecisionCache(); + + Ask(cache, generation: 2, compute: () => true).ShouldBeTrue(); + cache.Generation.ShouldBe(2); + cache.Count.ShouldBe(1); + + // The straggler: same question, superseded snapshot. + cache.TryGet(generation: 1, Session, OpcUaOperation.Read, AddressSpaceRealm.Raw, Node, out _) + .ShouldBeFalse("a superseded snapshot must recompute rather than read the newer generation"); + + // Asserted BEFORE the generation-2 lookup below, which would itself restore a rolled-back stamp and + // hide the defect. + cache.Generation.ShouldBe(2, "the straggler must not have rolled the stamp backward"); + cache.Count.ShouldBe(1, "and must not have purged the current generation's entries"); + + cache.TryGet(generation: 2, Session, OpcUaOperation.Read, AddressSpaceRealm.Raw, Node, out var decision) + .ShouldBeTrue("the current generation's decision survived the straggler"); + decision.ShouldBeTrue(); + } + /// /// The store half of the same window: a Set that meets the previous generation's shard must /// displace it, not fill it.