diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs index bb3154ed..6ebb7626 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/BoundedApiKeyStore.cs @@ -61,10 +61,22 @@ public sealed class BoundedApiKeyStore : IApiKeyStore // R03:3.3 — the known-id set: every ApiKeyId the inner store holds, refreshed every StoreCacheSeconds // (single-flight; a refresh that fails or times out keeps the previous set). Null until the first // successful load, or when the inner store cannot list ids — then the per-id path runs as before. - private volatile IReadOnlySet? _knownIds; - private DateTimeOffset _knownIdsStampedUtc = DateTimeOffset.MinValue; + // The set and the instant it was loaded are published TOGETHER as one immutable snapshot behind a volatile + // reference (fix round 1): a reader on another thread can never pair a new set with an old stamp, or read + // a torn DateTimeOffset (a 16-byte struct, not atomically readable), as the separate fields allowed. + private volatile KnownIdSnapshot? _knownIds; private readonly SemaphoreSlim _knownIdsRefresh = new(1, 1); + // Set when the inner store reports it cannot list ids. Per-instance on purpose: the guard options object is + // shared configuration and is never written (it used to be — `_guard.KnownIdSet = false` — which leaked + // one store's capability into every other consumer of the same options). + private volatile bool _listingUnsupported; + + /// The known-id set and the instant it was loaded, published as one unit. + /// The ids the inner store holds. + /// When they were read. + private sealed record KnownIdSnapshot(IReadOnlySet Ids, DateTimeOffset StampedUtc); + // R03:3.3 — the miss budget: a token bucket on lookups that REACH the inner store and miss. Guards // the path the known-id set cannot (set unavailable), and bounds a flood of unknown ids to the // configured rate instead of one SQL round trip per attempt under the SessionManager lock. @@ -104,7 +116,7 @@ public sealed class BoundedApiKeyStore : IApiKeyStore } /// The known-id set currently in force, or null when none has loaded. Exposed for tests and wiring guards. - public IReadOnlySet? KnownKeyIds => _knownIds; + public IReadOnlySet? KnownKeyIds => _knownIds?.Ids; /// The store being bounded. /// @@ -173,26 +185,25 @@ public sealed class BoundedApiKeyStore : IApiKeyStore /// The set, or null. private async Task?> KnownIdsAsync(TimeSpan ttl, CancellationToken ct) { - if (!_guard.KnownIdSet || ttl <= TimeSpan.Zero) return null; + if (!_guard.KnownIdSet || _listingUnsupported || ttl <= TimeSpan.Zero) return null; var now = _time.GetUtcNow(); var current = _knownIds; - if (current is not null && now - _knownIdsStampedUtc < ttl) return current; + if (current is not null && now - current.StampedUtc < ttl) return current.Ids; - if (!await _knownIdsRefresh.WaitAsync(0, ct).ConfigureAwait(false)) return current; + if (!await _knownIdsRefresh.WaitAsync(0, ct).ConfigureAwait(false)) return current?.Ids; try { // Re-check under the gate: another caller may have refreshed while this one waited. - if (_knownIds is { } refreshed && now - _knownIdsStampedUtc < ttl) return refreshed; + if (_knownIds is { } refreshed && now - refreshed.StampedUtc < ttl) return refreshed.Ids; var ids = await BoundAsync(token => _inner.ListKeyIdsAsync(token), ct).ConfigureAwait(false); if (ids is null) { - // Unsupported by the inner store: say so once and stop asking. - _guard.KnownIdSet = false; + // Unsupported by the inner store: stop asking — for this decorator only. + _listingUnsupported = true; return null; } - _knownIds = ids; - _knownIdsStampedUtc = _time.GetUtcNow(); + _knownIds = new KnownIdSnapshot(ids, _time.GetUtcNow()); return ids; } catch (Exception ex) when (ex is not OperationCanceledException || !ct.IsCancellationRequested) @@ -200,7 +211,7 @@ public sealed class BoundedApiKeyStore : IApiKeyStore _log.LogWarning(ex, "ApiKeyStore: refreshing the known key-id set failed; {Mode}", current is null ? "unknown ids reach the store until it loads" : "the previous set stays in force"); - return current; + return current?.Ids; } finally { diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreKnownIdTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreKnownIdTests.cs index 7051b151..8d08df5e 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreKnownIdTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/OpcUa/BoundedApiKeyStoreKnownIdTests.cs @@ -120,6 +120,36 @@ public sealed class BoundedApiKeyStoreKnownIdTests inner.ListCalls.ShouldBe(0); } + /// + /// Fix round 1 minor — an inner store that cannot list ids switches the known-id set off for THIS + /// decorator only. It used to do so by writing KnownIdSet = false into the guard options it was + /// handed, a shared options object, so one store's capability leaked into every other consumer. + /// Positive control: a second decorator over a listing store on the same options instance still uses + /// the set. + /// + [Fact] + public async Task An_unlisting_store_does_not_mutate_the_shared_guard_options() + { + var guard = new ApiKeyStoreGuardOptions { MissBurst = 100, MissRefillPerSecond = 20 }; + var options = Options.Create(new ApiKeyOptions + { + Username = "apikey", Pepper = "unit-test-pepper-xxxx", StoreTimeoutMs = 0, StoreCacheSeconds = 5, + }); + var clock = new FakeClock(T0); + var unlisting = new ListingStore("k-real") { SupportsListing = false }; + var first = new BoundedApiKeyStore(unlisting, options, NullLogger.Instance, clock, guard); + + (await first.FindAsync("k-guess", T0.UtcDateTime, default)).ShouldBeNull(); + unlisting.FindCalls.ShouldBe(1, "no set: the per-id path ran"); + guard.KnownIdSet.ShouldBeTrue("the shared options object is never written"); + + var listing = new ListingStore("k-real"); + var second = new BoundedApiKeyStore(listing, options, NullLogger.Instance, clock, guard); + (await second.FindAsync("k-guess", T0.UtcDateTime, default)).ShouldBeNull(); + listing.FindCalls.ShouldBe(0, "the second decorator still refuses unknown ids in memory"); + listing.ListCalls.ShouldBe(1); + } + private static BoundedApiKeyStore Bounded( IApiKeyStore inner, TimeProvider clock, int cacheSeconds, int missBurst = 100, double refillPerSecond = 20) => new( inner,