mirror of
https://github.com/okieselbach/AutopilotMonitor.git
synced 2026-10-24 20:53:06 +00:00
docs(entitlements): MCP quota boundary is deliberately soft - correct claim + pin with tests
Codex follow-up: the previous middleware comment claimed a deterministic limit+1 block, but McpQuotaService caches the per-user decision for 60s - an allowed decision keeps admitting inside that window even after the async increments cross the limit. The soft boundary IS the intended design (bounded overshoot ~TTL x request rate per instance, same posture as the sliding-window rate limiter; one counter read per user per minute instead of per request). - Middleware doc rewritten: check-then-increment fixes the race/inflation, but the boundary is explicitly documented as SOFT with bounded overshoot; warns against re-documenting as exact without reworking the decision cache. - Two regression tests pin the contract: cached allowed decision keeps admitting within the TTL even after counters cross the limit (no storage re-read), and after cache expiry (simulated via eviction) the over-limit counters block. Backend 2658/2658 green.
This commit is contained in:
@@ -69,7 +69,8 @@ public class McpQuotaServiceTests
|
||||
Mock<IUserUsageRepository> usageRepo,
|
||||
string? planDefinitionsJson = null,
|
||||
string? mcpUserPlanOverride = null,
|
||||
TenantEdition edition = TenantEdition.Community)
|
||||
TenantEdition edition = TenantEdition.Community,
|
||||
IMemoryCache? cache = null)
|
||||
{
|
||||
var adminRepo = new Mock<IAdminRepository>();
|
||||
adminRepo.Setup(r => r.GetMcpUserAsync(It.IsAny<string>()))
|
||||
@@ -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<McpUserService>.Instance,
|
||||
globalAdminService: null!, delegatedAdminService: null!, adminConfigService: null!);
|
||||
@@ -225,4 +226,62 @@ public class McpQuotaServiceTests
|
||||
repo.Verify(r => r.GetUsageByUserAsync(It.IsAny<string>(), It.IsAny<string?>(), It.IsAny<string?>()),
|
||||
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<string?>(), It.IsAny<string?>()))
|
||||
.ReturnsAsync(new List<UserUsageRecord>
|
||||
{
|
||||
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<string>(), It.IsAny<string?>(), It.IsAny<string?>()),
|
||||
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<string?>(), It.IsAny<string?>()))
|
||||
.ReturnsAsync(new List<UserUsageRecord>
|
||||
{
|
||||
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);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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).
|
||||
///
|
||||
/// <b>The quota boundary is deliberately SOFT, not exact.</b> 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, <c>Retry-After</c>, and
|
||||
/// <c>X-MCP-Quota-*</c> headers; allowed MCP requests get the quota headers too so the MCP
|
||||
|
||||
Reference in New Issue
Block a user