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 21b40271..9657ea87 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Security/SessionAclDecisionCache.cs @@ -429,6 +429,8 @@ public sealed class SessionAclDecisionCache /// /// 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. /// /// /// @@ -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. /// /// + /// ⚠️ That guard lives INSIDE the loop, against a freshly re-read stamp — 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. + /// + /// + /// Why a loop rather than one exchange. 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 + /// that follows is dropped by the Generation != generation 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. + /// + /// + /// The elected clear survives the loop. 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. + /// + /// + /// It terminates. Each iteration either returns through the guard or exchanges against a + /// seen strictly larger than the last iteration's (a failed exchange means someone raised + /// the stamp), and _generation only ever rises — so the guard is reached within a bounded + /// number of passes. + /// + /// /// 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. _generation starts at -1, below any real - /// generation, so the first one still passes. + /// generation, so the first one — generation 0 included — still passes. /// /// /// The generation the calling request is being decided under. 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; + } + } } /// 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 4d75d858..d78cffe5 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SessionAclDecisionCacheTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/SessionAclDecisionCacheTests.cs @@ -520,6 +520,116 @@ public sealed class SessionAclDecisionCacheTests cache.LiveShardCount.ShouldBe(cache.Count); } + /// + /// Two lookups arriving together at different generations must leave the higher one stamped, + /// whichever of them wins the compare-exchange. + /// + /// + /// + /// The single-shot-CAS defect: both callers read the same baseline stamp, and if the older + /// 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 TryGet misses and + /// the Set that follows is dropped at the Generation != generation 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. + /// + /// + /// ⚠️ Red is probabilistic per round — 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 + /// no better (its post-phase action releases from inside 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. + /// + /// + /// Every round gets a fresh cache: both racers must start from the -1 seed, or the + /// previous round's stamp would let the <= guard return before either reaches the + /// exchange and the race would never be run at all. + /// + /// + /// It is also the only pin on the retry loop's straggler guard being re-read per attempt. + /// Hoisting that guard to a single check before the loop leaves + /// + /// green — that straggler is sequential, so the one pre-loop check still catches it — while a + /// straggler that lost an exchange spins back, re-reads the newer stamp as its expected + /// value, and rolls it backward. That lands here, as the same stale-stamp failure. + /// + /// + /// Nothing here uses AdvanceGenerationForTest: the stamp must move through the ordinary + /// lookup path, which is the path the defect lives on. + /// + /// + [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,