diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/McpQuotaServiceTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/McpQuotaServiceTests.cs index 9d8c32b5..002a27c5 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/McpQuotaServiceTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/McpQuotaServiceTests.cs @@ -69,7 +69,8 @@ public class McpQuotaServiceTests Mock usageRepo, string? planDefinitionsJson = null, string? mcpUserPlanOverride = null, - TenantEdition edition = TenantEdition.Community) + TenantEdition edition = TenantEdition.Community, + IMemoryCache? cache = null) { var adminRepo = new Mock(); adminRepo.Setup(r => r.GetMcpUserAsync(It.IsAny())) @@ -77,7 +78,7 @@ public class McpQuotaServiceTests ? null : new McpUserEntry { Upn = Upn, IsEnabled = true, UsagePlan = mcpUserPlanOverride }); - var cache = new MemoryCache(new MemoryCacheOptions()); + cache ??= new MemoryCache(new MemoryCacheOptions()); var mcpUserService = new McpUserService( adminRepo.Object, cache, NullLogger.Instance, globalAdminService: null!, delegatedAdminService: null!, adminConfigService: null!); @@ -225,4 +226,62 @@ public class McpQuotaServiceTests repo.Verify(r => r.GetUsageByUserAsync(It.IsAny(), It.IsAny(), It.IsAny()), Times.Once); } + + // ── Soft-boundary contract (Codex finding 2026-07-07) ───────────────────────── + // The quota is DELIBERATELY soft: the per-user decision is cached for 60s, so an allowed + // decision keeps admitting requests inside that window even after the fire-and-forget + // increments push the stored counters past the limit. Overshoot is bounded (~TTL × request + // rate per instance) — the same posture as the sliding-window rate limiter. These tests PIN + // that contract; if strict limit+1 blocking is ever wanted, the decision cache must be + // reworked and these tests consciously rewritten. + + [Fact] + public async Task Check_SoftBoundary_CachedAllowedDecision_KeepsAdmitting_WithinTtl() + { + // First read: one request below the daily limit → allowed, decision cached. + var repo = UsageRepo(("20260707", 99)); + var svc = Build(repo); + + var first = await svc.CheckAsync(Oid, Upn, TenantId); + Assert.True(first.Allowed); + + // The async increments land: stored counters are now OVER the limit — but the cached + // allowed decision keeps admitting within the TTL, without re-reading storage. + repo.Setup(r => r.GetUsageByUserAsync(Oid, It.IsAny(), It.IsAny())) + .ReturnsAsync(new List + { + new() { UserId = Oid, Date = "20260707", RequestCount = 150 } + }); + + var second = await svc.CheckAsync(Oid, Upn, TenantId); + + Assert.True(second.Allowed, "soft boundary: cached allowed decision persists within the TTL"); + repo.Verify(r => r.GetUsageByUserAsync(It.IsAny(), It.IsAny(), It.IsAny()), + Times.Once); + } + + [Fact] + public async Task Check_SoftBoundary_AfterCacheEviction_OverLimitCounters_Block() + { + // Same scenario as above, but once the cached decision is gone (TTL expiry — simulated + // via eviction), the next check reads the over-limit counters and blocks. + var cache = new MemoryCache(new MemoryCacheOptions()); + var repo = UsageRepo(("20260707", 99)); + var svc = Build(repo, cache: cache); + + Assert.True((await svc.CheckAsync(Oid, Upn, TenantId)).Allowed); + + repo.Setup(r => r.GetUsageByUserAsync(Oid, It.IsAny(), It.IsAny())) + .ReturnsAsync(new List + { + new() { UserId = Oid, Date = "20260707", RequestCount = 150 } + }); + cache.Remove($"mcp-quota:{Oid}"); // stands in for the 60s TTL elapsing + + var decision = await svc.CheckAsync(Oid, Upn, TenantId); + + Assert.False(decision.Allowed); + Assert.Equal("daily", decision.Scope); + Assert.Equal(150, decision.DailyUsed); + } } diff --git a/src/Backend/AutopilotMonitor.Functions/Middleware/McpQuotaEnforcementMiddleware.cs b/src/Backend/AutopilotMonitor.Functions/Middleware/McpQuotaEnforcementMiddleware.cs index 1c4c2dbb..e2f9289f 100644 --- a/src/Backend/AutopilotMonitor.Functions/Middleware/McpQuotaEnforcementMiddleware.cs +++ b/src/Backend/AutopilotMonitor.Functions/Middleware/McpQuotaEnforcementMiddleware.cs @@ -18,10 +18,19 @@ namespace AutopilotMonitor.Functions.Middleware; /// quota (but their usage is still tracked for the metrics surface). /// /// This middleware also OWNS the usage-counter increment (moved here from AuthenticationMiddleware, -/// Codex finding 2026-07-07): strictly check-then-increment, and only for requests that are -/// actually served — the request that hits the limit is deterministic (requests 1..limit pass, -/// limit+1 blocks) and denied requests (403 upstream, 429 here) no longer inflate the counters. -/// The increment itself stays fire-and-forget (never blocks the request path). +/// Codex finding 2026-07-07): check-then-increment, and only for requests that are actually +/// served — denied requests (403 upstream, 429 here) no longer inflate the counters, and a +/// request can never be blocked by its OWN in-flight increment. The increment stays +/// fire-and-forget (never blocks the request path). +/// +/// The quota boundary is deliberately SOFT, not exact. McpQuotaService caches the +/// per-user decision for 60 seconds (per instance), so an ALLOWED decision keeps admitting +/// requests inside that window even after the async increments push the stored counters past +/// the limit. Worst-case overshoot is bounded: ~60s × the caller's request rate (× instance +/// count on scaled-out Flex Consumption) — the same deliberate posture as the sliding-window +/// rate limiter, trading exactness for one counter read per user per minute instead of per +/// request. Pinned by McpQuotaServiceTests soft-boundary tests; do NOT re-document this as an +/// exact limit without reworking the decision cache. /// /// Over-quota requests get 429 with a structured body, Retry-After, and /// X-MCP-Quota-* headers; allowed MCP requests get the quota headers too so the MCP