chore(followups): reviewer-recommended tests, comments, and hardening from the remediation reviews
The remediation reviews approved every task but left a tail of small notes. This lands the gateway-side half of them. Hardening (behavior changes, all narrow): - BuildFilteredWriteBulkCommand's unreachable default case failed OPEN: a fifth bulk-write kind added upstream without a filter case here would have shipped the DENIED entries to the worker while reporting them denied to the caller. It now throws UnreachableException. - SqliteCanonicalAuditStore.ListRecentAsync no longer throws on a row it cannot date. The retention sweep deliberately preserves such rows (SQLite's datetime() yields NULL, so the DELETE never matches), which guaranteed the dashboard's recent-audit view would meet one eventually and lose the whole page to it. The row is now reported at DateTimeOffset.MinValue with every other column intact, behind an optional logger. - The audit drain loop's finally now completes the channel writer alongside detaching the drain, so a producer that raced past the attached check takes the write-through branch instead of stranding its event in a buffer nobody reads until shutdown. TryComplete is idempotent, so StopAsync is unaffected. Tests: - MapCommandReply ownership (Assert.Same on the inner reply), mirroring the existing MapEvent ownership test. - Redactor key-id length boundary at exactly 64 and 65 characters, pinning which way it fails. Nothing validates key-id length at creation, so docs/Diagnostics.md's "which no issued key id does" is now stated as the heuristic it is. - ApiKeyFailureLimiter.Reset with a PartitionResolution whose partition was evicted between the Check and the Reset: inert, and clears nobody else's block. - Constraint-cache concurrency stress: the cap is enforced by the inserting thread, so overshoot must be transient and proportional to the in-flight inserters, and the cache must settle at or under the cap. - ListRecentAsync against a raw-SQL undateable row. Comment/doc accuracy: - EventsHubViewerRegistry.ReleaseConnection records that it relies on SignalR's default sequential per-connection dispatch (MaximumParallelInvocationsPerClient = 1). - A PERF(followup) note on Invoke's double session resolve and why removing it needs a SessionManager overload. - SessionEventDistributor: the volatile-field comment named the pump as the lock-free reader, but the pump's single capture point is inside _replayLock; the genuinely lock-free reader is SubscriberCount. OnSubscriberOverflow's "cannot be observed here" now excepts the DisposeAsync abandon path. The churn test names its ConcurrentDictionary bucket-order assumption and that a violation surfaces as a read timeout, not a silent pass. - The two "restores the sequential drain's behavior" claims (SessionManager, docs/Sessions.md) were wrong: the sequential drain leaked too, because KillWorkerAsync's entry ThrowIfCancellationRequested aborted the whole loop on the first session for zero kills. Reworded to "fixes a leak the sequential drain also had", with the sweep-bound/shutdown-unbound ParallelOptions asymmetry explained. - ISessionManager.ShutdownAsync's token doc: it degrades the drain to a kill sweep rather than cancelling it, with the bounded overrun stated. SessionShutdownHostedService.StopAsync records that its cancellation-logging branch is now unreachable.
This commit is contained in:
@@ -219,6 +219,33 @@ public sealed class ChannelAuditWriterTests : IDisposable
|
||||
(await ListActionsAsync(factory)).OrderBy(action => action, StringComparer.Ordinal));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The sweep above deliberately preserves rows it cannot date, so the dashboard's recent-audit
|
||||
/// view is guaranteed to meet one eventually. Reading must therefore be defensive: the
|
||||
/// undateable row is reported at <see cref="DateTimeOffset.MinValue"/> with every other column
|
||||
/// intact, rather than one bad row throwing the whole page away.
|
||||
/// </summary>
|
||||
/// <returns>A task that represents the asynchronous operation.</returns>
|
||||
[Fact]
|
||||
public async Task SqliteStore_ListRecent_WithUnparseableRow_ReportsMinValueInsteadOfThrowing()
|
||||
{
|
||||
(SqliteCanonicalAuditStore store, AuthSqliteConnectionFactory factory) = CreateStore();
|
||||
|
||||
DateTimeOffset occurred = new(2026, 5, 18, 0, 0, 0, TimeSpan.Zero);
|
||||
await store.InsertAsync(MakeEvent("dateable", occurred), CancellationToken.None);
|
||||
await InsertRawRowAsync(factory, "undateable", "0000-not-a-timestamp");
|
||||
|
||||
IReadOnlyList<AuditEvent> recent = await store.ListRecentAsync(10, CancellationToken.None);
|
||||
|
||||
Assert.Equal(2, recent.Count);
|
||||
Assert.Equal(occurred, recent.Single(auditEvent => auditEvent.Action == "dateable").OccurredAtUtc);
|
||||
|
||||
AuditEvent undateable = recent.Single(auditEvent => auditEvent.Action == "undateable");
|
||||
Assert.Equal(DateTimeOffset.MinValue, undateable.OccurredAtUtc);
|
||||
Assert.Equal("operator01", undateable.Actor);
|
||||
Assert.Equal(AuditOutcome.Denied, undateable.Outcome);
|
||||
}
|
||||
|
||||
private static (ChannelAuditWriter Writer, AuditDrainService Drain) CreateWriterAndDrain(
|
||||
IAuditEventSink sink,
|
||||
SecurityOptions? security = null,
|
||||
@@ -265,7 +292,8 @@ public sealed class ChannelAuditWriterTests : IDisposable
|
||||
await command.ExecuteNonQueryAsync(CancellationToken.None);
|
||||
}
|
||||
|
||||
// Reads actions straight from SQL: ListRecentAsync would throw on the unparseable timestamp.
|
||||
// Reads actions straight from SQL so the sweep assertion depends on the DELETE alone, with no
|
||||
// opinion from the store's read path about how an undateable row is surfaced.
|
||||
private static async Task<List<string>> ListActionsAsync(AuthSqliteConnectionFactory factory)
|
||||
{
|
||||
await using SqliteConnection connection = await factory.OpenConnectionAsync(CancellationToken.None);
|
||||
|
||||
@@ -281,6 +281,55 @@ public sealed class CachingApiKeyVerifierTests
|
||||
Assert.Same(reparsed, MapConstraints(firstJson));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Stress: the constraint cache's bound is enforced by the inserting thread itself
|
||||
/// (<c>GetOrAdd</c>, then enqueue, then evict), so concurrent inserters can each land an
|
||||
/// entry before any of them reaches the eviction step. That overshoot is real but must be
|
||||
/// transient and proportional to the in-flight inserters — not unbounded growth — and once
|
||||
/// the churn stops the cache must be back at or under the cap. Hammered on both the
|
||||
/// converging path (every iteration maps the same blob, so all but one <c>GetOrAdd</c>
|
||||
/// loses) and the growth path (a distinct blob per iteration, which is what forces
|
||||
/// eviction).
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void ToGatewayIdentity_ConcurrentBlobs_OvershootIsTransientAndCacheSettlesUnderCap()
|
||||
{
|
||||
const int cap = GatewayApiKeyIdentityMapper.MaxCachedConstraintBlobs;
|
||||
|
||||
// Overshoot is bounded by how many inserters can sit between their GetOrAdd and their own
|
||||
// EvictIfOverCapacity, so scale the allowance with the available parallelism rather than
|
||||
// pinning a magic number. Generous on purpose: the assertion under test is "bounded", not
|
||||
// "bounded by exactly this".
|
||||
int transientAllowance = (Environment.ProcessorCount * 8) + 64;
|
||||
string sharedJson = ConstraintsJson("Area_StressShared");
|
||||
int peak = 0;
|
||||
|
||||
Parallel.For(0, (cap * 2) + 64, index =>
|
||||
{
|
||||
MapConstraints(sharedJson);
|
||||
MapConstraints(ConstraintsJson($"Area_Stress_{index}"));
|
||||
|
||||
int size = GatewayApiKeyIdentityMapper.CurrentCacheSize;
|
||||
int seen = Volatile.Read(ref peak);
|
||||
while (size > seen && Interlocked.CompareExchange(ref peak, size, seen) != seen)
|
||||
{
|
||||
seen = Volatile.Read(ref peak);
|
||||
}
|
||||
});
|
||||
|
||||
Assert.True(
|
||||
peak <= cap + transientAllowance,
|
||||
$"cache peaked at {peak} entries, past the {cap} cap plus the {transientAllowance} transient allowance");
|
||||
|
||||
// The cache is process-wide static and other test classes in this assembly map identities
|
||||
// too, so poll rather than asserting on the instant the loop returns.
|
||||
Assert.True(
|
||||
SpinWait.SpinUntil(
|
||||
() => GatewayApiKeyIdentityMapper.CurrentCacheSize <= cap,
|
||||
TimeSpan.FromSeconds(5)),
|
||||
$"cache settled at {GatewayApiKeyIdentityMapper.CurrentCacheSize} entries, past the {cap} cap");
|
||||
}
|
||||
|
||||
private static ApiKeyConstraints MapConstraints(string constraintsJson) =>
|
||||
GatewayApiKeyIdentityMapper.ToGatewayIdentity(new LibApiKeyIdentity(
|
||||
KeyId: "operator01",
|
||||
|
||||
@@ -414,6 +414,46 @@ public sealed class ApiKeyFailureLimiterTests
|
||||
Assert.True(limiter.IsTracked(arriving));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The <c>PartitionResolution</c> handed back by <c>Check</c> is carried across an await (the
|
||||
/// inner verification) before <c>Reset</c> consumes it, so the partition it names can be evicted
|
||||
/// in between. Applying a stale resolution must be inert — never throw, and never clear a
|
||||
/// partition or aggregate that belongs to somebody else. The failure direction that matters is
|
||||
/// "clears too little", which costs the caller a probe wait; "clears somebody else's block"
|
||||
/// would be a throttle bypass.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void Reset_WithStaleResolutionForEvictedPartition_IsInertAndClearsNoOtherBlock()
|
||||
{
|
||||
ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch);
|
||||
ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 3, aggregateLimit: 3, maxPartitions: 2);
|
||||
ApiKeyThrottlePartition victim = new("ipv4:10.0.0.1:1", "victim");
|
||||
ApiKeyThrottlePartition other = new("ipv4:10.0.0.2:1", "other");
|
||||
ApiKeyThrottlePartition arriving = new("ipv4:10.0.0.3:1", "arriving");
|
||||
|
||||
RecordFailures(limiter, victim, 3);
|
||||
Assert.Equal(
|
||||
ApiKeyThrottleDecision.ThrottledByPeer,
|
||||
limiter.Check(victim, out ApiKeyFailureLimiter.PartitionResolution stale));
|
||||
Assert.True(stale.IsResolved);
|
||||
|
||||
// Age the victim's window out, then push the map past its cap so eviction takes it —
|
||||
// expired windows are the first eviction preference, so this is deterministic. The
|
||||
// resolution captured above now names a partition that no longer exists.
|
||||
clock.Advance(Window + TimeSpan.FromSeconds(1));
|
||||
RecordFailures(limiter, other, 3);
|
||||
RecordFailures(limiter, arriving, 3);
|
||||
Assert.False(limiter.IsTracked(victim));
|
||||
|
||||
limiter.Reset(victim, stale);
|
||||
|
||||
Assert.True(limiter.IsTracked(other));
|
||||
Assert.True(limiter.IsTracked(arriving));
|
||||
Assert.Equal(2, limiter.TrackedAggregateCount);
|
||||
Assert.Equal(ApiKeyThrottleDecision.ThrottledByPeer, limiter.Check(other));
|
||||
Assert.Equal(ApiKeyThrottleDecision.ThrottledByPeer, limiter.Check(arriving));
|
||||
}
|
||||
|
||||
private static void RecordFailures(ApiKeyFailureLimiter limiter, ApiKeyThrottlePartition partition, int count)
|
||||
{
|
||||
for (int i = 0; i < count; i++)
|
||||
|
||||
Reference in New Issue
Block a user