fix(acl): shard eviction swaps the shard object so _globalCount cannot over-subtract; concurrency stress test

2026-08-17 03:55:13 -04:00
parent 97902d6879
commit d884934593
2 changed files with 233 additions and 11 deletions
@@ -56,8 +56,10 @@ public sealed class SessionAclDecisionCache
    ///     Enforced against an <see cref="Interlocked"/> counter rather than
    ///     <see cref="ConcurrentDictionary{TKey,TValue}.Count"/>, 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 <see cref="SealedShard"/> for how that survives an eviction
+    ///     racing an insert.
    /// </remarks>
    public const int MaxEntries = 100_000;

@@ -77,6 +79,18 @@ public sealed class SessionAclDecisionCache
    /// </remarks>
    public const int MaxEntriesPerSession = 20_000;

+    /// <summary>
+    ///     Sentinel written over a shard's <see cref="Shard.ApproxCount"/> 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
+    ///     <see cref="Interlocked.Exchange(ref int, int)"/> that captures the count being subtracted from
+    ///     <see cref="_globalCount"/>, so an insert either books its increment <i>before</i> 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.
+    /// </summary>
+    private const int SealedShard = int.MinValue;
+
    private readonly ConcurrentDictionary<string, Shard> _shards = new(StringComparer.Ordinal);
    private long _generation = -1;
    private int _globalCount;
@@ -99,6 +113,29 @@ public sealed class SessionAclDecisionCache
        }
    }

+    /// <summary>
+    ///     The running counter both ceilings are enforced against. Exposed for the concurrency audit only:
+    ///     the property a test wants is almost always <see cref="Count"/>, and the single interesting fact
+    ///     about this one is that the two agree.
+    /// </summary>
+    internal int TrackedCount => Volatile.Read(ref _globalCount);
+
+    /// <summary>
+    ///     Sum of the live shards' own counters — the middle term of the invariant
+    ///     <c>TrackedCount == LiveShardCount == Count</c>. 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.
+    /// </summary>
+    internal int LiveShardCount
+    {
+        get
+        {
+            var total = 0;
+            foreach (var shard in _shards.Values) total += Volatile.Read(ref shard.ApproxCount);
+            return total;
+        }
+    }
+
    /// <summary>
    ///     Looks a decision up without allocating anything — the hot half, and the reason the store half is
    ///     a separate method rather than a <see cref="Func{TResult}"/> 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);
+    }
+
+    /// <summary>
+    ///     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.
+    /// </summary>
+    /// <remarks>
+    ///     A plain <see cref="Interlocked.Increment(ref int)"/> here would race the seal in the other
+    ///     direction: an increment landing after the displacing thread captured the count would leave
+    ///     <see cref="_globalCount"/> 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
+    ///     <see cref="SealedShard"/> and books nothing. The loop retries only against other <i>inserts</i>
+    ///     on the same shard; the sealed case exits.
+    ///     <para>
+    ///         The shard's counter is moved <b>before</b> the global one, and that order matters: a seal
+    ///         landing between the two subtracts an increment the global counter has not received yet, so
+    ///         <see cref="_globalCount"/> can dip transiently below the live total. That is self-healing —
+    ///         the thread in the gap always completes its increment — and it errs towards <i>not</i>
+    ///         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.
+    ///     </para>
+    /// </remarks>
+    /// <param name="shard">The shard the entry was added to.</param>
+    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;
        }
    }

+    /// <summary>
+    ///     Retires a shard that has just been removed from (or replaced in) <see cref="_shards"/> and
+    ///     returns the number of entries it was still accounted for, for the caller to subtract from
+    ///     <see cref="_globalCount"/>.
+    /// </summary>
+    /// <remarks>
+    ///     Only the thread that actually displaced the shard may call this — <see cref="_shards"/>'
+    ///     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.
+    /// </remarks>
+    /// <param name="shard">The displaced shard.</param>
+    /// <returns>The count to subtract from <see cref="_globalCount"/>.</returns>
+    private static int Seal(Shard shard)
+    {
+        var captured = Interlocked.Exchange(ref shard.ApproxCount, SealedShard);
+        return captured < 0 ? 0 : captured;
+    }
+
    /// <summary>
    ///     Returns the memoized decision for this key, computing and storing it when absent or when the
    ///     generation has moved on. A convenience over <see cref="TryGet"/> + <see cref="Set"/>.
@@ -239,24 +342,43 @@ public sealed class SessionAclDecisionCache
    }

    /// <summary>
-    ///     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.
    /// </summary>
+    /// <remarks>
+    ///     Each shard is removed and then <see cref="Seal"/>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 <i>fresh</i> 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.
+    /// </remarks>
    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));
    }

-    /// <summary>One session's decisions, plus the approximate entry count its own ceiling is enforced on.</summary>
+    /// <summary>
+    ///     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
+    ///     <see cref="_shards"/> either still is the live shard or has been sealed — it can never turn
+    ///     into a <i>different</i> generation of the same session's entries under its holder.
+    /// </summary>
    private sealed class Shard
    {
        public readonly ConcurrentDictionary<ShardKey, bool> Entries = new();

-        /// <summary>Approximate by design — read and written with <see cref="Interlocked"/>, never derived
-        ///     from <see cref="ConcurrentDictionary{TKey,TValue}.Count"/> on the store path.</summary>
+        /// <summary>
+        ///     Read and written with <see cref="Interlocked"/>, never derived from
+        ///     <see cref="ConcurrentDictionary{TKey,TValue}.Count"/> on the store path. The name is
+        ///     historical: while the shard is live this tracks <see cref="Entries"/> exactly (entries are
+        ///     only ever added, and every add books here), and once displaced it holds
+        ///     <see cref="SealedShard"/>. It is <i>lagging</i> — an insert is visible in
+        ///     <see cref="Entries"/> a moment before it is booked here — but never wrong.
+        /// </summary>
        public int ApproxCount;
    }

@@ -163,6 +163,106 @@ public sealed class SessionAclDecisionCacheTests
        neighbourCalls.ShouldBe(1);
    }

+    /// <summary>
+    ///     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.
+    /// </summary>
+    [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);
+    }
+
+    /// <summary>
+    ///     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.
+    /// </summary>
+    /// <remarks>
+    ///     <para>
+    ///         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
+    ///         <c>TryAdd</c> for the duration of the <c>Clear</c> 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.
+    ///     </para>
+    ///     <para>
+    ///         ⚠️ <b>The assertion is equality, not a bound.</b> A ceiling check
+    ///         (<c>TrackedCount &lt;= Count</c>, or <c>&gt;= 0</c>) passes against the broken code, because
+    ///         under-counting is exactly what the broken code does. Only <c>TrackedCount == Count</c>
+    ///         falsifies it — verified by reverting the fix, which turns this test red on the first round.
+    ///     </para>
+    /// </remarks>
+    [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");
+        }
+    }
+
    /// <summary>
    ///     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.