dohertj2/lmxopcua
1
0
Fork 0
Code Issues 32 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(acl): stamp the generation on Shard so a rollover racer cannot serve a stale-generation verdict; eviction-swap capacity hint

Browse Source
This commit is contained in:
Joseph Doherty
2026-08-17 05:37:56 -04:00
parent 8946644bb1
commit 1979cf4119
2 changed files with 299 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs
+111 -15
View File
@@ -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.
/// </para>
/// <para>
/// ⚠️ <b>The invariant that makes that safe is per-shard, not global: a shard can only ever answer for
/// the generation it was built under.</b> The wholesale drop is necessarily two steps — publish the new
/// generation, then purge the shards — because the publish <i>is</i> 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.
/// </para>
/// <para>
/// <b>Storage is sharded per calling KEY, under a two-level bound.</b> The shard key is
/// <c>ApiKeyId ?? Principal</c>, so <i>sessions sharing one API key share one shard and one
/// <see cref="MaxEntriesPerSession"/> cap</i> — 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);
}
/// <summary>
/// Replaces a session's live shard with a fresh one for <paramref name="generation"/> and returns
/// whatever shard is live afterwards.
/// </summary>
/// <remarks>
/// This REPLACES the shard object rather than emptying the dictionary in place — the move
/// <see cref="ClearAll"/> makes, and for the same reason. Emptying in place left the clear and the
/// counter reset as two steps over storage a racing <see cref="Set"/> was still inserting into: the
/// racer could land in the just-cleared dictionary and increment both counters between them, and the
/// <see cref="Interlocked.Exchange(ref int, int)"/> then subtracted an amount that included the
/// racer's fresh entry, under-counting <see cref="_globalCount"/> for good. Swapping the object makes
/// the displaced shard unreachable in one atomic step, after which <see cref="Seal"/> 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.
/// </remarks>
/// <param name="sessionKey">The key whose shard is being displaced.</param>
/// <param name="shard">The shard the caller found live.</param>
/// <param name="generation">The generation the replacement is built for.</param>
/// <param name="capacityHint">Initial entry capacity for the replacement; zero leaves it at the default.</param>
/// <returns>The live shard after the swap.</returns>
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);
}
/// <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
@@ -339,6 +405,21 @@ public sealed class SessionAclDecisionCache
/// <summary>Drops every memoized decision. Called when the authorization generation advances.</summary>
public void Clear() => ClearAll();
/// <summary>
/// Moves the generation stamp <b>without</b> clearing — reproducing, deterministically, the exact
/// view a racer gets inside <see cref="InvalidateIfStale"/>'s rollover window, where the new
/// generation is already published but the winner's <see cref="ClearAll"/> has not run yet.
/// </summary>
/// <remarks>
/// 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 — <i>a shard can only serve the
/// generation it was built under</i> — is what makes the window harmless, so the pin has to be able
/// to stand inside it.
/// </remarks>
/// <param name="generation">The generation to publish.</param>
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
/// <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
/// <param name="generation">The authorization generation this shard's entries were decided under.</param>
/// <param name="capacity">
/// Initial entry capacity. Zero leaves <see cref="Entries"/> 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.
/// </param>
private sealed class Shard(long generation, int capacity = 0)
{
public readonly ConcurrentDictionary<ShardKey, bool> Entries = new();
public readonly ConcurrentDictionary<ShardKey, bool> Entries = capacity > 0
? new ConcurrentDictionary<ShardKey, bool>(Environment.ProcessorCount, capacity)
: new ConcurrentDictionary<ShardKey, bool>();
/// <summary>
/// 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.
/// </summary>
public readonly long Generation = generation;
/// <summary>
/// Read and written with <see cref="Interlocked"/>, never derived from
tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SessionAclDecisionCacheTests.cs
+188
View File
@@ -263,6 +263,123 @@ public sealed class SessionAclDecisionCacheTests
}
}
/// <summary>
/// The store half of the same window: a <c>Set</c> that meets the previous generation's shard must
/// displace it, not fill it.
/// </summary>
/// <remarks>
/// Filing into an orphaned shard is never <i>wrong</i> — 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.
/// </remarks>
[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);
}
/// <summary>
/// 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.
/// </summary>
/// <remarks>
/// <para>
/// The generation is <b>encoded in the stored verdict</b> (<c>gen % 2 == 0</c>), 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.
/// </para>
/// <para>
/// 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.
/// </para>
/// </remarks>
[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");
}
}
/// <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.
@@ -289,6 +406,77 @@ public sealed class SessionAclDecisionCacheTests
cache.Count.ShouldBe(1);
}
/// <summary>
/// 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.
/// </summary>
/// <remarks>
/// <para>
/// <c>AdvanceGenerationForTest</c> 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 <i>grant walk</i>
/// rather than a usable revoked key — real, but narrower than it first looks.
/// </para>
/// <para>
/// ⚠️ The assertion is a <b>miss</b>, not a corrected value: serving <c>false</c> from a shard
/// built under the old generation would be just as wrong, and a value assertion would pass on it.
/// </para>
/// </remarks>
[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");
}
/// <summary>
/// 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.
/// </summary>
/// <remarks>
/// <b>Pin, not red-first.</b> 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.
/// </remarks>
[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,