From 1979cf4119c3388570920a9229d491f653add22a Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 17 Aug 2026 05:37:56 -0400 Subject: [PATCH] fix(acl): stamp the generation on Shard so a rollover racer cannot serve a stale-generation verdict; eviction-swap capacity hint --- .../Security/SessionAclDecisionCache.cs | 126 ++++++++++-- .../SessionAclDecisionCacheTests.cs | 188 ++++++++++++++++++ 2 files changed, 299 insertions(+), 15 deletions(-) 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 fa190a03..3f544aee 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs @@ -32,6 +32,16 @@ namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Security; /// deploy costs one repopulation and cannot be wrong. Deployments are rare relative to reads. /// /// +/// ⚠️ The invariant that makes that safe is per-shard, not global: a shard can only ever answer for +/// the generation it was built under. The wholesale drop is necessarily two steps — publish the new +/// generation, then purge the shards — because the publish is the compare-exchange that elects +/// the single thread allowed to purge. Between them the stamp already matches while the previous +/// generation's shards are still in the table, so comparing the stamp alone would let a caller arriving +/// in that window read a verdict the new trie never made. Each shard therefore carries its own +/// generation: a mismatched shard is a miss on the read path and is displaced on the store path, which +/// makes the window inert rather than merely narrow. +/// +/// /// Storage is sharded per calling KEY, under a two-level bound. The shard key is /// ApiKeyId ?? Principal, so sessions sharing one API key share one shard and one /// cap — the isolation is across distinct keys, not across @@ -178,7 +188,14 @@ public sealed class SessionAclDecisionCache // TryGetValue rather than GetOrAdd: a session that only ever hits must not mint a shard, and a // session whose shard was just cleared must not resurrect one until it actually stores something. + // + // The shard's OWN generation decides, not the stamp InvalidateIfStale just compared: inside the + // rollover window the stamp already matches and the purge has not run, so the stamp cannot + // distinguish this generation's shard from the last one's. A mismatch is treated as a plain miss and + // is NOT displaced from here — this path stays allocation-free and non-mutating, and the shard is + // reclaimed a moment later by the purging thread or by the next Set on that key. return _shards.TryGetValue(sessionKey, out var shard) + && shard.Generation == generation && shard.Entries.TryGetValue(new ShardKey(operation, realm, nodeIdentifier), out decision); } @@ -205,7 +222,7 @@ public sealed class SessionAclDecisionCache // trie never made. Dropping it costs one recomputation on the next call. if (Generation != generation) return; - var shard = _shards.GetOrAdd(sessionKey, static _ => new Shard()); + var shard = _shards.GetOrAdd(sessionKey, static (_, gen) => new Shard(gen), generation); if (Volatile.Read(ref _globalCount) >= MaxEntries) { @@ -218,22 +235,41 @@ public sealed class SessionAclDecisionCache // re-enumerate an emptying dictionary. The redundancy costs an enumeration, not correctness, // and a CAS-elected clear would add hot-path state for a branch that is a rare fallback. ClearAll(); - shard = _shards.GetOrAdd(sessionKey, static _ => new Shard()); + shard = _shards.GetOrAdd(sessionKey, static (_, gen) => new Shard(gen), generation); } else if (Volatile.Read(ref shard.ApproxCount) >= MaxEntriesPerSession) { - // The inner bound. This REPLACES the shard object rather than emptying the dictionary in - // place — the same move ClearAll makes, and for the same reason. Emptying in place left the - // clear and the counter reset as two steps over storage a racing Set was still inserting - // into: the racer could land in the just-cleared dictionary and increment both counters - // between them, and the Exchange then subtracted an amount that included the racer's fresh - // entry, under-counting _globalCount for good. Swapping the object makes the displaced - // shard unreachable in one atomic step, after which Seal settles its count exactly. - if (_shards.TryUpdate(sessionKey, new Shard(), shard)) - Interlocked.Add(ref _globalCount, -Seal(shard)); + // The inner bound. The replacement is pre-sized to what the shard it displaces was holding: this + // key has just demonstrated it fills its whole budget, and it is about to do so again, so the + // refill would otherwise walk the dictionary's whole growth cascade — a rehash under every one of + // the table's locks at each doubling — to arrive back at a size already known. Clamped to the + // ceiling because the count read here is the pre-seal one — a thread that lost the swap reads + // the sentinel and clamps to no hint at all, which is the right answer for it. Only this site + // hints; a shard minted by GetOrAdd has no such evidence and stays at the default capacity. + // + // The trade: the replacement is built before the swap is attempted, so a thread that loses + // discards a pre-sized dictionary rather than an empty one. That is a bigger throwaway than + // 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. + shard = Displace( + sessionKey, shard, generation, + capacityHint: Math.Clamp(Volatile.Read(ref shard.ApproxCount), 0, MaxEntriesPerSession)); + } - // Whoever won the swap, the live shard is now a different object than the one held above. - shard = _shards.GetOrAdd(sessionKey, static _ => new Shard()); + // The shard must belong to THIS generation before anything is filed in it. Reached in two ways: a + // deploy landed since the shard was minted (the common case — one displacement per key per deploy, + // and the read path has been missing on it since the stamp moved), or the rollover window closed + // between the check at the top of this method and here. + if (shard.Generation != generation) + { + shard = Displace(sessionKey, shard, generation, capacityHint: 0); + + // Still mismatched means a racer re-minted the shard under a generation that has since moved + // again. Drop the decision rather than loop: filing this generation's verdict in another + // generation's shard is the same error in the other direction, and the line above already + // dropped a verdict for the identical reason. + if (shard.Generation != generation) return; } // TryAdd, not an indexer assignment: only a genuine insert may move the counters, or a racing pair @@ -242,6 +278,36 @@ public sealed class SessionAclDecisionCache TryBookInsert(shard); } + /// + /// Replaces a session's live shard with a fresh one for and returns + /// whatever shard is live afterwards. + /// + /// + /// This REPLACES the shard object rather than emptying the dictionary in place — the move + /// makes, and for the same reason. Emptying in place left the clear and the + /// counter reset as two steps over storage a racing was still inserting into: the + /// racer could land in the just-cleared dictionary and increment both counters between them, and the + /// then subtracted an amount that included the + /// racer's fresh entry, under-counting for good. Swapping the object makes + /// the displaced shard unreachable in one atomic step, after which settles its + /// count exactly — and because only the thread that wins the compare-and-swap seals, no shard is + /// sealed twice however many threads arrive here at once. The returned shard is re-read from the + /// table rather than assumed to be the one just built: whoever won the swap, the live shard is now a + /// different object than the caller held. + /// + /// The key whose shard is being displaced. + /// The shard the caller found live. + /// The generation the replacement is built for. + /// Initial entry capacity for the replacement; zero leaves it at the default. + /// The live shard after the swap. + private Shard Displace(string sessionKey, Shard shard, long generation, int capacityHint) + { + if (_shards.TryUpdate(sessionKey, new Shard(generation, capacityHint), shard)) + Interlocked.Add(ref _globalCount, -Seal(shard)); + + return _shards.GetOrAdd(sessionKey, static (_, gen) => new Shard(gen), generation); + } + /// /// Books a just-completed insert against the shard's own counter and the global one, unless the /// shard was displaced first — in which case the entry lives in storage nothing can reach any more @@ -339,6 +405,21 @@ public sealed class SessionAclDecisionCache /// Drops every memoized decision. Called when the authorization generation advances. public void Clear() => ClearAll(); + /// + /// Moves the generation stamp without clearing — reproducing, deterministically, the exact + /// view a racer gets inside 's rollover window, where the new + /// generation is already published but the winner's has not run yet. + /// + /// + /// Tests only. That window is a few instructions wide, so a concurrency test can only make it + /// probable; this makes it certain. The invariant it exercises — a shard can only serve the + /// generation it was built under — is what makes the window harmless, so the pin has to be able + /// to stand inside it. + /// + /// The generation to publish. + internal void AdvanceGenerationForTest(long generation) => + Interlocked.Exchange(ref _generation, generation); + private void InvalidateIfStale(long generation) { var seen = Interlocked.Read(ref _generation); @@ -375,9 +456,24 @@ public sealed class SessionAclDecisionCache /// either still is the live shard or has been sealed — it can never turn /// into a different generation of the same session's entries under its holder. /// - private sealed class Shard + /// The authorization generation this shard's entries were decided under. + /// + /// Initial entry capacity. Zero leaves at the default — the right choice + /// everywhere except the per-shard eviction swap, which alone has evidence of the size the + /// replacement is about to grow back to. + /// + private sealed class Shard(long generation, int capacity = 0) { - public readonly ConcurrentDictionary Entries = new(); + public readonly ConcurrentDictionary Entries = capacity > 0 + ? new ConcurrentDictionary(Environment.ProcessorCount, capacity) + : new ConcurrentDictionary(); + + /// + /// The generation this shard's entries were decided under, fixed for its lifetime. It is what the + /// read path compares — the cache-wide stamp cannot be trusted alone, because it is published one + /// step before the shards it invalidates are purged. + /// + public readonly long Generation = generation; /// /// Read and written with , never derived from 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 6062c6b7..e4aead0d 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,123 @@ public sealed class SessionAclDecisionCacheTests } } + /// + /// The store half of the same window: a Set that meets the previous generation's shard must + /// displace it, not fill it. + /// + /// + /// Filing into an orphaned shard is never wrong — the read path's generation check means + /// nothing stored there can ever be served — which is exactly why it needs its own pin: the symptom + /// is silent, and it is a dead cache rather than a leaking one. Every store for that key would land + /// somewhere unreadable, consuming the key's whole per-shard budget before an eviction swap finally + /// minted a shard for the current generation. Twenty thousand wasted inserts per key per deployment, + /// with a hit rate of zero throughout and nothing in the counters to say so. + /// + [Fact] + public void A_store_meeting_the_previous_generations_shard_displaces_it() + { + var cache = new SessionAclDecisionCache(); + + Ask(cache, generation: 1, compute: () => true).ShouldBeTrue(); + cache.AdvanceGenerationForTest(2); + + // Inside the window: the stamp says 2, the shard in the table was built for 1. + cache.Set(generation: 2, Session, OpcUaOperation.Read, AddressSpaceRealm.Raw, Node, decision: false); + + cache.TryGet(generation: 2, Session, OpcUaOperation.Read, AddressSpaceRealm.Raw, Node, out var decision) + .ShouldBeTrue("the store must have landed in a shard the current generation can read"); + decision.ShouldBeFalse("and it must be the new verdict, not the one it replaced"); + + // The displaced shard was sealed and unbooked, so the survivor is the single new entry. + cache.Count.ShouldBe(1); + cache.TrackedCount.ShouldBe(1); + cache.LiveShardCount.ShouldBe(1); + } + + /// + /// The same rollover, arrived at the way production does — a generation moving under live traffic + /// rather than through the test seam. No hit may ever return a verdict stored under a different + /// generation, and the counters must still settle exactly. + /// + /// + /// + /// The generation is encoded in the stored verdict (gen % 2 == 0), which is what + /// makes a leak visible at all: a cache that answers with the right shape but the wrong + /// generation's data is indistinguishable from a correct one unless the data says which + /// generation it came from. A miss recomputes the encoding for the generation it was asked + /// about, so only a hit can ever disagree. + /// + /// + /// The bumps come from inside the worker mix rather than from a spinning thread of their own, + /// and are spaced far enough apart for entries to accumulate between them — a generation moving + /// faster than a lookup completes makes every call a miss, which is a cache that is never read + /// and therefore a test that pins nothing. + /// + /// + [Fact] + public void No_hit_ever_returns_a_verdict_from_another_generation() + { + var cache = new SessionAclDecisionCache(); + const int workers = 6; + const int writesEach = 30_000; + const int bumpEvery = 500; + const int rounds = 4; + + // A deliberately narrow node and key space: workers must keep landing on entries their peers + // stored, and on shards a bump has just orphaned. Unique identifiers would make every call a miss. + const int nodes = 64; + const int keys = 2; + + for (var r = 0; r < rounds; r++) + { + var round = r; + var generation = (round * 100L) + 1L; + var mismatches = 0; + var start = new Barrier(workers); + var threads = new Thread[workers]; + + for (var w = 0; w < workers; w++) + { + var worker = w; + + threads[w] = new Thread(() => + { + start.SignalAndWait(); + + for (var i = 0; i < writesEach; i++) + { + // One worker advances the generation, so the bump lands in the middle of the others' + // traffic rather than beside it. + if (worker == 0 && i % bumpEvery == 0) Interlocked.Increment(ref generation); + + var gen = Interlocked.Read(ref generation); + var expected = gen % 2 == 0; + + var decided = cache.GetOrAdd( + gen, $"key-{worker % keys}", OpcUaOperation.Read, AddressSpaceRealm.Raw, + $"Plant/dev1/Tag{i % nodes}", () => expected); + + if (decided != expected) Interlocked.Increment(ref mismatches); + } + }) { IsBackground = true }; + } + + foreach (var thread in threads) thread.Start(); + foreach (var thread in threads) thread.Join(); + start.Dispose(); + + mismatches.ShouldBe( + 0, $"round {round}: a hit served a verdict taken under a different generation"); + + var survived = cache.Count; + + cache.LiveShardCount.ShouldBe( + survived, $"round {round}: a shard's own counter drifted from its entries"); + cache.TrackedCount.ShouldBe( + survived, $"round {round}: the global counter drifted from the live shards"); + } + } + /// /// A decision taken while a deployment lands mid-walk is returned but NOT stored, so the new /// generation never inherits a verdict its own trie did not make. @@ -289,6 +406,77 @@ public sealed class SessionAclDecisionCacheTests cache.Count.ShouldBe(1); } + /// + /// The generation-rollover window, made deterministic. Publishing the new generation and purging the + /// shards are two steps, and a racer arriving between them sees a generation that already matches + /// and shards that have not been dropped yet — so it must not be able to read one. + /// + /// + /// + /// AdvanceGenerationForTest is the racer's view exactly: the stamp moves, nothing clears. + /// Before the fix this test served the generation-1 verdict to a generation-2 caller. Key + /// revocation and expiry are re-checked above the cache, so the leak was a stale grant walk + /// rather than a usable revoked key — real, but narrower than it first looks. + /// + /// + /// ⚠️ The assertion is a miss, not a corrected value: serving false from a shard + /// built under the old generation would be just as wrong, and a value assertion would pass on it. + /// + /// + [Fact] + public void A_shard_built_under_the_previous_generation_is_never_served() + { + var cache = new SessionAclDecisionCache(); + + // GetOrAdd, not a bare Set: Set drops a decision whose generation is not already the stamped one, + // and a fresh cache is stamped -1. The lookup inside GetOrAdd is what publishes generation 1. + Ask(cache, generation: 1, compute: () => true).ShouldBeTrue(); + cache.TryGet(generation: 1, Session, OpcUaOperation.Read, AddressSpaceRealm.Raw, Node, out var stored) + .ShouldBeTrue("the entry must exist before the window opens, or the test proves nothing"); + stored.ShouldBeTrue(); + + // Stand inside the window: the new generation is published, the shards are not yet purged. + cache.AdvanceGenerationForTest(2); + + cache.TryGet(generation: 2, Session, OpcUaOperation.Read, AddressSpaceRealm.Raw, Node, out _) + .ShouldBeFalse("a shard may only answer for the generation it was built under"); + } + + /// + /// A shard displaced by the per-shard eviction swap keeps taking inserts correctly, with the counters + /// still exact — the behaviour pin behind the capacity hint the swap now passes. + /// + /// + /// Pin, not red-first. The hint sizes a dictionary's initial buckets, which is unobservable + /// through the public surface: no assertion here can distinguish a hinted shard from a default one, + /// and none tries to. What it pins is that passing it changed nothing about the refill — that the + /// replacement shard still accepts entries and still books them exactly once. + /// + [Fact] + public void A_shard_replaced_by_the_eviction_swap_still_refills_exactly() + { + var cache = new SessionAclDecisionCache(); + const int overshoot = 12; + + for (var i = 0; i < SessionAclDecisionCache.MaxEntriesPerSession + overshoot; i++) + Ask(cache, generation: 1, session: "crawler-key", node: $"Plant/dev1/Tag{i}", compute: () => true); + + cache.Count.ShouldBe(overshoot); + cache.TrackedCount.ShouldBe(overshoot); + cache.LiveShardCount.ShouldBe(overshoot); + + // The refilled shard serves what it was given, and a fresh insert into it still books once. + Ask(cache, generation: 1, session: "crawler-key", node: "Plant/dev1/Tag20011", + compute: () => throw new InvalidOperationException("this decision was already memoized")) + .ShouldBeTrue(); + Ask(cache, generation: 1, session: "crawler-key", node: "Plant/dev1/Fresh", compute: () => false) + .ShouldBeFalse(); + + cache.Count.ShouldBe(overshoot + 1); + cache.TrackedCount.ShouldBe(cache.Count); + cache.LiveShardCount.ShouldBe(cache.Count); + } + private static bool Ask( SessionAclDecisionCache cache, long generation,