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

fix(acl): make InvalidateIfStale a monotonic-max CAS loop — stamp publication is never deferred

Browse Source
This commit is contained in:
Joseph Doherty
2026-08-17 07:15:30 -04:00
parent 4c16118105
commit 441b34a391
2 changed files with 151 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs
+41 -6
View File
@@ -429,6 +429,8 @@ public sealed class SessionAclDecisionCache
/// <summary>
/// Advances the cache-wide stamp when this call presents a newer generation, and purges on the way.
/// The stamp ends up holding the highest generation any caller has presented — never a lower one,
/// and never one call late.
/// </summary>
/// <remarks>
/// <para>
@@ -443,21 +445,54 @@ public sealed class SessionAclDecisionCache
/// carries its own generation; what is at stake here is only whether the cache is any use.
/// </para>
/// <para>
/// ⚠️ <b>That guard lives INSIDE the loop, against a freshly re-read stamp</b> — hoisting it to a
/// single check before the loop reintroduces exactly the backward roll it exists to prevent: a
/// straggler that lost its exchange would spin back round and re-attempt against the newer stamp
/// it just lost to, and eventually succeed in rolling it back. Re-reading means the straggler
/// sees the newer value on its second pass and leaves.
/// </para>
/// <para>
/// <b>Why a loop rather than one exchange.</b> A single shot defers publication by a call: two
/// callers read the same baseline, the older one's exchange lands first, and the newer one's
/// fails and falls out having published nothing. Its own lookup then misses and the
/// <see cref="Set"/> that follows is dropped by the <c>Generation != generation</c> guard, which
/// costs a store in the post-deploy window where the cache is coldest. Retrying against the moved
/// stamp closes that: the higher generation is published on the call that presents it.
/// </para>
/// <para>
/// <b>The elected clear survives the loop.</b> Only the thread whose exchange actually moved the
/// stamp clears, so a burst of concurrent requests arriving just after a deploy still repopulates
/// once rather than each clearing the others' fresh entries. A retry can now clear too, but only
/// by winning a move to a strictly higher generation — so clears stay bounded by the number of
/// distinct generations presented, not by request volume.
/// </para>
/// <para>
/// <b>It terminates.</b> Each iteration either returns through the guard or exchanges against a
/// <c>seen</c> strictly larger than the last iteration's (a failed exchange means someone raised
/// the stamp), and <c>_generation</c> only ever rises — so the guard is reached within a bounded
/// number of passes.
/// </para>
/// <para>
/// 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. <c>_generation</c> starts at <c>-1</c>, below any real
/// generation, so the first one still passes.
/// generation, so the first one — generation <c>0</c> included — still passes.
/// </para>
/// </remarks>
/// <param name="generation">The generation the calling request is being decided under.</param>
private void InvalidateIfStale(long generation)
{
var seen = Interlocked.Read(ref _generation);
if (generation <= seen) return;
while (true)
{
var seen = Interlocked.Read(ref _generation);
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.
if (Interlocked.CompareExchange(ref _generation, generation, seen) == seen) ClearAll();
if (Interlocked.CompareExchange(ref _generation, generation, seen) == seen)
{
ClearAll();
return;
}
}
}
/// <summary>
tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SessionAclDecisionCacheTests.cs
+110
View File
@@ -520,6 +520,116 @@ public sealed class SessionAclDecisionCacheTests
cache.LiveShardCount.ShouldBe(cache.Count);
}
/// <summary>
/// Two lookups arriving together at different generations must leave the <b>higher</b> one stamped,
/// whichever of them wins the compare-exchange.
/// </summary>
/// <remarks>
/// <para>
/// The single-shot-CAS defect: both callers read the same baseline stamp, and if the <i>older</i>
/// generation's exchange lands first, the newer one's exchange fails against the moved stamp and
/// falls out with no retry. Its generation is never published, so its own <c>TryGet</c> misses and
/// the <c>Set</c> that follows is dropped at the <c>Generation != generation</c> guard — a lost
/// store in exactly the post-deploy window where the cache is coldest. Verdict safety is not at
/// stake (the per-shard generation check decides that); cache usefulness is.
/// </para>
/// <para>
/// ⚠️ <b>Red is probabilistic per round</b> — it needs both racers to read the baseline stamp
/// before either exchanges, a window a few instructions wide. Everything about the harness
/// exists to buy those odds, and the measurements are why it looks the way it does: fresh
/// threads per round land red about once in twenty thousand, a two-party <see cref="Barrier"/>
/// no better (its post-phase action releases from <i>inside</i> one racer, handing it a head
/// start), while the long-lived racers plus a third-thread starter used below turn roughly a
/// quarter to a half of all rounds red. So a round or two is enough in practice, and the
/// twenty thousand are for the machine that is slower or busier than this one.
/// </para>
/// <para>
/// Every round gets a <b>fresh cache</b>: both racers must start from the <c>-1</c> seed, or the
/// previous round's stamp would let the <c>&lt;=</c> guard return before either reaches the
/// exchange and the race would never be run at all.
/// </para>
/// <para>
/// It is also the only pin on the retry loop's <b>straggler guard being re-read per attempt</b>.
/// Hoisting that guard to a single check before the loop leaves
/// <see cref="A_straggler_from_an_older_generation_neither_clears_nor_rolls_the_stamp_back"/>
/// green — that straggler is sequential, so the one pre-loop check still catches it — while a
/// straggler that <i>lost an exchange</i> spins back, re-reads the newer stamp as its expected
/// value, and rolls it backward. That lands here, as the same stale-stamp failure.
/// </para>
/// <para>
/// Nothing here uses <c>AdvanceGenerationForTest</c>: the stamp must move through the ordinary
/// lookup path, which is the path the defect lives on.
/// </para>
/// </remarks>
[Fact]
public void Concurrent_lookups_at_mixed_generations_always_publish_the_highest()
{
const int rounds = 20_000;
const int spinsBeforeYield = 4_000;
// The cache under race, handed over through a slot the starter rewrites between rounds.
var slot = new SessionAclDecisionCache[1];
var gate = 0; // starter's release signal: the round number both racers are cleared to run
var finished = 0; // racers' completion count for the current round
var firstStaleRound = -1;
var staleRounds = 0;
// Raw threads rather than the thread pool: the pool may serialise two work items onto one thread,
// which would make the race unreachable. Held for the whole run so the release is the only skew.
var low = new Thread(() => Race(1L)) { IsBackground = true };
var high = new Thread(() => Race(2L)) { IsBackground = true };
low.Start();
high.Start();
for (var round = 0; round < rounds; round++)
{
Volatile.Write(ref slot[0], new SessionAclDecisionCache());
Volatile.Write(ref finished, 0);
Volatile.Write(ref gate, round + 1); // both racers are spinning on this one store
for (var spins = 0; Volatile.Read(ref finished) != 2; spins++)
if (spins < spinsBeforeYield) Thread.SpinWait(1);
else Thread.Yield();
if (Volatile.Read(ref slot[0]).Generation != 2L)
{
if (firstStaleRound < 0) firstStaleRound = round;
staleRounds++;
}
}
low.Join();
high.Join();
firstStaleRound.ShouldBe(
-1,
$"round {firstStaleRound} (of {staleRounds} stale in {rounds}): the newer generation must " +
"publish even when it loses the CAS race");
void Race(long generation)
{
for (var round = 0; round < rounds; round++)
{
// Tight spin on the starter's store, not a park: both racers take the same invalidation from
// a third core and so leave the spin within a few nanoseconds of each other. Releasing from
// one of the racers instead (a Barrier's post-phase action) hands the releaser a head start
// of roughly the other's wake-up latency, which is orders of magnitude wider than the
// read-then-exchange window and drives the hit rate to near zero. The yield after
// spinsBeforeYield is only a liveness backstop for a machine with fewer cores than this
// needs threads — the release normally lands long before it.
for (var spins = 0; Volatile.Read(ref gate) != round + 1; spins++)
if (spins < spinsBeforeYield) Thread.SpinWait(1);
else Thread.Yield();
Volatile.Read(ref slot[0])
.TryGet(generation, Session, OpcUaOperation.Read, AddressSpaceRealm.Raw, Node, out _);
Interlocked.Increment(ref finished);
}
}
}
private static bool Ask(
SessionAclDecisionCache cache,
long generation,