From d884934593430cde1ef0624388489d9ec5b71832 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 17 Aug 2026 03:55:13 -0400 Subject: [PATCH] fix(acl): shard eviction swaps the shard object so _globalCount cannot over-subtract; concurrency stress test --- .../Security/SessionAclDecisionCache.cs | 144 ++++++++++++++++-- .../SessionAclDecisionCacheTests.cs | 100 ++++++++++++ 2 files changed, 233 insertions(+), 11 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 f9e1fbc4..1a5f3c75 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs @@ -56,8 +56,10 @@ public sealed class SessionAclDecisionCache /// Enforced against an counter rather than /// , which takes every lock in the table — a /// price the store path would pay on every miss to answer a question that only matters at the - /// ceiling. The counter is approximate under concurrency by design, and is reset by every clear so - /// drift cannot ratchet permanently. + /// ceiling. The counter lags — an insert is in its shard's dictionary a moment before it is booked + /// — but it does not drift: every entry is booked exactly once and unbooked exactly once, when the + /// shard holding it is sealed. See for how that survives an eviction + /// racing an insert. /// public const int MaxEntries = 100_000; @@ -77,6 +79,18 @@ public sealed class SessionAclDecisionCache /// public const int MaxEntriesPerSession = 20_000; + /// + /// Sentinel written over a shard's the instant it stops being the + /// live shard for its key. It is the whole of the eviction race fix: it is set by the same + /// that captures the count being subtracted from + /// , so an insert either books its increment before that capture + /// (and is therefore included in the subtraction) or sees the sentinel and books nothing at all. + /// There is no third outcome, which is what makes the counter exact rather than approximate. + /// Negative so it can never be mistaken for a real count, and far enough from zero that the + /// increment path cannot walk it back to a positive value. + /// + private const int SealedShard = int.MinValue; + private readonly ConcurrentDictionary _shards = new(StringComparer.Ordinal); private long _generation = -1; private int _globalCount; @@ -99,6 +113,29 @@ public sealed class SessionAclDecisionCache } } + /// + /// The running counter both ceilings are enforced against. Exposed for the concurrency audit only: + /// the property a test wants is almost always , and the single interesting fact + /// about this one is that the two agree. + /// + internal int TrackedCount => Volatile.Read(ref _globalCount); + + /// + /// Sum of the live shards' own counters — the middle term of the invariant + /// TrackedCount == LiveShardCount == Count. Reading the three separately is what tells a + /// failing audit whether a shard's counter drifted from its dictionary or the global counter + /// drifted from the shards. + /// + internal int LiveShardCount + { + get + { + var total = 0; + foreach (var shard in _shards.Values) total += Volatile.Read(ref shard.ApproxCount); + return total; + } + } + /// /// Looks a decision up without allocating anything — the hot half, and the reason the store half is /// a separate method rather than a parameter. @@ -177,19 +214,85 @@ public sealed class SessionAclDecisionCache } else if (Volatile.Read(ref shard.ApproxCount) >= MaxEntriesPerSession) { - shard.Entries.Clear(); - Interlocked.Add(ref _globalCount, -Interlocked.Exchange(ref shard.ApproxCount, 0)); + // 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)); + + // Whoever won the swap, the live shard is now a different object than the one held above. + shard = _shards.GetOrAdd(sessionKey, static _ => new Shard()); } // TryAdd, not an indexer assignment: only a genuine insert may move the counters, or a racing pair // of walks over the same key would inflate them and pull the ceilings in for good. if (shard.Entries.TryAdd(new ShardKey(operation, realm, nodeIdentifier), decision)) + TryBookInsert(shard); + } + + /// + /// 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 + /// and must not be counted. + /// + /// + /// A plain here would race the seal in the other + /// direction: an increment landing after the displacing thread captured the count would leave + /// carrying an entry no live shard holds. The compare-exchange is what + /// linearises the two — either it wins while the shard is still live (and the seal that follows + /// captures and subtracts this increment along with the rest), or it observes + /// and books nothing. The loop retries only against other inserts + /// on the same shard; the sealed case exits. + /// + /// The shard's counter is moved before the global one, and that order matters: a seal + /// landing between the two subtracts an increment the global counter has not received yet, so + /// can dip transiently below the live total. That is self-healing — + /// the thread in the gap always completes its increment — and it errs towards not + /// clearing, which is the harmless direction. The reverse order cannot self-heal: it would book + /// globally before the seal could see the entry, leaving a permanent over-count. + /// + /// + /// The shard the entry was added to. + private void TryBookInsert(Shard shard) + { + while (true) { - Interlocked.Increment(ref shard.ApproxCount); + var seen = Volatile.Read(ref shard.ApproxCount); + + // Displaced between the TryAdd and here: the entry is lost with the shard, and counting it + // would be exactly the over-count this method exists to prevent. + if (seen < 0) return; + + if (Interlocked.CompareExchange(ref shard.ApproxCount, seen + 1, seen) != seen) continue; + Interlocked.Increment(ref _globalCount); + return; } } + /// + /// Retires a shard that has just been removed from (or replaced in) and + /// returns the number of entries it was still accounted for, for the caller to subtract from + /// . + /// + /// + /// Only the thread that actually displaced the shard may call this — ' + /// compare-and-swap removal/replacement elects exactly one, so no shard is ever sealed twice. The + /// already-sealed branch is defence in depth: it returns zero rather than subtracting the + /// sentinel, which would drive the global counter deeply negative and disable both ceilings. + /// + /// The displaced shard. + /// The count to subtract from . + private static int Seal(Shard shard) + { + var captured = Interlocked.Exchange(ref shard.ApproxCount, SealedShard); + return captured < 0 ? 0 : captured; + } + /// /// Returns the memoized decision for this key, computing and storing it when absent or when the /// generation has moved on. A convenience over + . @@ -239,24 +342,43 @@ public sealed class SessionAclDecisionCache } /// - /// Drops every shard and resets the global counter. Dropping the shard objects rather than emptying + /// Drops every shard and settles the global counter. Dropping the shard objects rather than emptying /// them also sheds the shards of sessions that have since closed. A walk racing this holds a shard /// that is no longer in the table; its entry is simply lost, which is the same outcome the clear /// itself produces. /// + /// + /// Each shard is removed and then ed, and only its sealed count is subtracted — + /// rather than the whole counter being slammed to zero. Zeroing is wrong in the other direction + /// from the eviction race: a walk that mints a fresh shard while this is running is a live + /// entry, and resetting the counter after it books its increment would lose it. Subtracting exactly + /// what is being dropped leaves the counter equal to what remains, whatever arrives mid-clear. + /// private void ClearAll() { - _shards.Clear(); - Interlocked.Exchange(ref _globalCount, 0); + foreach (var key in _shards.Keys) + if (_shards.TryRemove(key, out var shard)) + Interlocked.Add(ref _globalCount, -Seal(shard)); } - /// One session's decisions, plus the approximate entry count its own ceiling is enforced on. + /// + /// One session's decisions, plus the entry count its own ceiling is enforced on. A shard is + /// immutable in identity: it is never emptied, only replaced, so a reference obtained from + /// 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 { public readonly ConcurrentDictionary Entries = new(); - /// Approximate by design — read and written with , never derived - /// from on the store path. + /// + /// Read and written with , never derived from + /// on the store path. The name is + /// historical: while the shard is live this tracks exactly (entries are + /// only ever added, and every add books here), and once displaced it holds + /// . It is lagging — an insert is visible in + /// a moment before it is booked here — but never wrong. + /// public int ApproxCount; } 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 7e595806..6062c6b7 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SessionAclDecisionCacheTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SessionAclDecisionCacheTests.cs @@ -163,6 +163,106 @@ public sealed class SessionAclDecisionCacheTests neighbourCalls.ShouldBe(1); } + /// + /// After a shard is evicted the global counter equals what actually survived — the arithmetic of + /// the swap, pinned single-threaded where it is exact and every off-by-a-shard is visible. + /// + [Fact] + public void An_eviction_subtracts_exactly_what_it_displaced() + { + var cache = new SessionAclDecisionCache(); + const int overshoot = 25; + + for (var i = 0; i < SessionAclDecisionCache.MaxEntriesPerSession + overshoot; i++) + Ask(cache, generation: 1, session: "crawler-key", node: $"Plant/dev1/Tag{i}", compute: () => true); + + // The cap is reached on the entry that would have been the 20,001st; everything after it is the + // refilled shard. Subtracting anything but the displaced shard's own count would show up here. + cache.Count.ShouldBe(overshoot); + cache.TrackedCount.ShouldBe(cache.Count); + cache.LiveShardCount.ShouldBe(cache.Count); + } + + /// + /// The eviction race. Many writers hammer ONE shard past its ceiling while others insert into it, + /// so evictions and inserts interleave repeatedly; at quiescence the global counter must equal the + /// entries that actually survived. + /// + /// + /// + /// The defect this pins is not reachable single-threaded. Eviction used to empty the shard's + /// dictionary and then reset the counters as two steps: a writer blocked inside + /// TryAdd for the duration of the Clear resumes the moment the table's locks are + /// released — squarely in the window before the counter reset — books an increment, and has it + /// subtracted a second time by the reset that follows. The drift is negative-only, so it never + /// changed a verdict; it made the cache trim progressively earlier than it should. + /// + /// + /// ⚠️ The assertion is equality, not a bound. A ceiling check + /// (TrackedCount <= Count, or >= 0) passes against the broken code, because + /// under-counting is exactly what the broken code does. Only TrackedCount == Count + /// falsifies it — verified by reverting the fix, which turns this test red on the first round. + /// + /// + [Fact] + public void Concurrent_eviction_and_insertion_leave_the_counter_exact() + { + var cache = new SessionAclDecisionCache(); + const string crawler = "crawler-key"; + const int crawlers = 8; + const int neighbours = 2; + const int perCrawler = 40_000; + const int perNeighbour = 500; + + // Sized for exposure, not for coverage: one round drives the shard past its ceiling ~16 times, and + // the interleaving that used to corrupt the counter needs a writer to land inside the few + // nanoseconds between the clear and the counter reset. Fewer rounds let the reverted code pass. + const int rounds = 8; + + for (var round = 0; round < rounds; round++) + { + var currentRound = round; + var start = new Barrier(crawlers + neighbours); + var workers = new Thread[crawlers + neighbours]; + + for (var w = 0; w < workers.Length; w++) + { + var worker = w; + + // The first `crawlers` threads share ONE shard and drive it past its ceiling many times + // over; the rest hold their own shards, so the audit also covers the shards an eviction + // must not touch. + var session = worker < crawlers ? crawler : $"neighbour-{worker}"; + var writes = worker < crawlers ? perCrawler : perNeighbour; + + workers[w] = new Thread(() => + { + start.SignalAndWait(); + + // Node identifiers are unique per (round, worker, index), so every store is a genuine + // insert and nothing is lost to a TryAdd collision instead of to the race. + for (var i = 0; i < writes; i++) + cache.GetOrAdd( + generation: 1, session, OpcUaOperation.Read, AddressSpaceRealm.Raw, + $"Plant/dev1/R{currentRound}-W{worker}-T{i}", static () => true); + }) { IsBackground = true }; + } + + foreach (var worker in workers) worker.Start(); + foreach (var worker in workers) worker.Join(); + start.Dispose(); + + var survived = cache.Count; + + cache.TrackedCount.ShouldBeGreaterThanOrEqualTo( + 0, $"round {currentRound}: the counter must never be driven negative"); + cache.LiveShardCount.ShouldBe( + survived, $"round {currentRound}: a shard's own counter drifted from its entries"); + cache.TrackedCount.ShouldBe( + survived, $"round {currentRound}: 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.