fix(host): publish the known-id set and its stamp atomically; never write the shared guard options
BoundedApiKeyStore kept the set (volatile) and its DateTimeOffset stamp (non-volatile, 16 bytes) in separate fields, so a reader could pair a new set with an old stamp or read a torn stamp; they are now one immutable snapshot behind a volatile reference. An inner store that cannot list ids now switches the set off with a per-instance flag instead of writing KnownIdSet = false into the shared ApiKeyStoreGuardOptions. Refs #733 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT
This commit is contained in:
@@ -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<string>? _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;
|
||||
|
||||
/// <summary>The known-id set and the instant it was loaded, published as one unit.</summary>
|
||||
/// <param name="Ids">The ids the inner store holds.</param>
|
||||
/// <param name="StampedUtc">When they were read.</param>
|
||||
private sealed record KnownIdSnapshot(IReadOnlySet<string> 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
|
||||
}
|
||||
|
||||
/// <summary>The known-id set currently in force, or null when none has loaded. Exposed for tests and wiring guards.</summary>
|
||||
public IReadOnlySet<string>? KnownKeyIds => _knownIds;
|
||||
public IReadOnlySet<string>? KnownKeyIds => _knownIds?.Ids;
|
||||
|
||||
/// <summary>The store being bounded.</summary>
|
||||
/// <remarks>
|
||||
@@ -173,26 +185,25 @@ public sealed class BoundedApiKeyStore : IApiKeyStore
|
||||
/// <returns>The set, or null.</returns>
|
||||
private async Task<IReadOnlySet<string>?> 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
|
||||
{
|
||||
|
||||
@@ -120,6 +120,36 @@ public sealed class BoundedApiKeyStoreKnownIdTests
|
||||
inner.ListCalls.ShouldBe(0);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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 <c>KnownIdSet = false</c> 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.
|
||||
/// </summary>
|
||||
[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<BoundedApiKeyStore>.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<BoundedApiKeyStore>.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,
|
||||
|
||||
Reference in New Issue
Block a user