Skip to content

Commit 1ea4040

Browse files
JuliaEdomclaude
andcommitted
fix(batch): meter each sub-item against the rate limit (S-4)
`/v1/batch` accepts up to 20 items and runs each as a full engine op (entity / metric / NL->SQL), but the auth middleware only charged the single HTTP request one rate-limit token. An authenticated tenant could therefore drive up to 20x its per-minute budget, concentrated on the expensive NL path (audit S-4, P3, metering bypass). Add `AuthManager.charge_rate_limit(tenant_key, units)` that debits `units` extra tokens against the same bucket via the existing `check_rate_limit`, and have the batch handler charge `len(requests) - 1` before executing, rejecting the whole batch with 429 if the budget cannot absorb it. Skipped when auth is disabled (no tenant_key), so per-key isolation and the fail-closed core are untouched. Tests: unit for `charge_rate_limit` (within/over/zero budget) and an integration test that a 5-item batch under a 3/min key returns 429. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 639d31a commit 1ea4040

4 files changed

Lines changed: 93 additions & 1 deletion

File tree

src/serving/api/auth/manager.py

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -421,6 +421,24 @@ async def check_rate_limit(self, tenant_key: TenantKey) -> tuple[bool, int, int]
421421
return True, max(0, tenant_key.rate_limit_rpm - len(window)), reset_at
422422
return is_allowed, remaining, reset_at
423423

424+
async def charge_rate_limit(self, tenant_key: TenantKey, units: int) -> bool:
425+
"""Debit ``units`` *additional* rate-limit tokens for one request that does
426+
more than one unit of metered work — a ``/v1/batch`` of N items runs N
427+
engine ops but the middleware only charged the single HTTP request.
428+
429+
Each unit maps to one ``check_rate_limit`` call against the same bucket,
430+
so a batch cannot bypass the per-minute budget by bundling many (notably
431+
the expensive NL→SQL) engine ops under one token. Every unit is debited
432+
(the request costs its full N even when over budget); returns ``False`` if
433+
the bucket could not absorb all of them, so the caller can reject before
434+
doing the work. (audit S-4)
435+
"""
436+
allowed = True
437+
for _ in range(max(0, units)):
438+
ok, _remaining, _reset_at = await self.check_rate_limit(tenant_key)
439+
allowed = allowed and ok
440+
return allowed
441+
424442
def is_failed_auth_limited(self, client_ip: str) -> bool:
425443
now = self.time_source()
426444
cutoff = now - FAILED_AUTH_WINDOW_SECONDS

src/serving/api/routers/batch.py

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
from typing import Any, Literal
55

66
import structlog
7-
from fastapi import APIRouter, Request
7+
from fastapi import APIRouter, HTTPException, Request
88
from pydantic import BaseModel, Field
99

1010
from src.serving.api.routers.agent_query import (
@@ -186,6 +186,26 @@ async def _execute_query_item(item: BatchItem, req: Request) -> dict[str, Any]:
186186

187187
@router.post("/batch", response_model=BatchResponse)
188188
async def batch_query(request: BatchRequest, req: Request) -> BatchResponse:
189+
# A batch runs one engine op per item but the auth middleware only metered the
190+
# single HTTP request, so a tenant could drive up to 20x its per-minute budget
191+
# (concentrated on the expensive NL path) for one token. Charge the remaining
192+
# items against the same rate-limit bucket and reject the whole batch if the
193+
# budget cannot absorb them. Skipped when auth is disabled (no tenant_key).
194+
# (audit S-4)
195+
tenant_key = getattr(req.state, "tenant_key", None)
196+
auth_manager = getattr(req.app.state, "auth_manager", None)
197+
extra_units = len(request.requests) - 1
198+
if tenant_key is not None and auth_manager is not None and extra_units > 0:
199+
within_budget = await auth_manager.charge_rate_limit(tenant_key, extra_units)
200+
if not within_budget:
201+
raise HTTPException(
202+
status_code=429,
203+
detail=(
204+
f"Rate limit exceeded: a {len(request.requests)}-item batch costs "
205+
f"{len(request.requests)} of {tenant_key.rate_limit_rpm} requests/minute."
206+
),
207+
)
208+
189209
started_at = time.monotonic()
190210
outcomes = await asyncio.gather(
191211
*[_execute_item(item, req) for item in request.requests],

tests/integration/test_batch.py

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,40 @@ def test_batch_rejects_more_than_twenty_requests(client):
167167
assert response.status_code == 422
168168

169169

170+
def test_batch_meters_each_item_against_rate_limit(client):
171+
# (audit S-4) a batch of N items runs N engine ops, so it must cost N
172+
# rate-limit tokens, not one. A batch that exceeds the tenant's per-minute
173+
# budget is rejected (429) instead of bypassing it ~20x on the expensive path.
174+
manager = client.app.state.auth_manager
175+
key = "batch-rpm-key"
176+
manager.keys_by_value = {
177+
key: TenantKey(
178+
key=key,
179+
name="batch-agent",
180+
tenant="default",
181+
rate_limit_rpm=3,
182+
allowed_entity_types=None,
183+
created_at=datetime.now(UTC).date(),
184+
)
185+
}
186+
manager._rate_windows.clear()
187+
manager.rate_limiter._windows.clear()
188+
189+
response = client.post(
190+
"/v1/batch",
191+
headers={"X-API-Key": key},
192+
json={
193+
"requests": [
194+
{"id": f"m{index}", "type": "metric", "params": {"name": "x", "window": "1h"}}
195+
for index in range(5) # 1 (request) + 4 extra = 5 > rpm 3
196+
]
197+
},
198+
)
199+
200+
assert response.status_code == 429
201+
assert "Rate limit exceeded" in response.json()["detail"]
202+
203+
170204
def test_batch_requires_api_key_when_auth_is_configured(client):
171205
_set_auth(client)
172206

tests/unit/test_auth_manager_mutation.py

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -561,6 +561,26 @@ def test_bucket_key_is_deterministic_and_per_key_distinct(self) -> None:
561561
assert a1 == a2
562562
assert a1 != b
563563

564+
@pytest.mark.asyncio
565+
async def test_charge_rate_limit_within_budget_returns_true(self) -> None:
566+
# A batch of 4 items (3 extra units) fits under a 5/min budget. (audit S-4)
567+
m = _build_manager()
568+
assert await m.charge_rate_limit(_key(rate_limit_rpm=5), 3) is True
569+
570+
@pytest.mark.asyncio
571+
async def test_charge_rate_limit_over_budget_returns_false(self) -> None:
572+
# 5 extra units cannot fit a 2/min budget -> the batch is rejected.
573+
m = _build_manager()
574+
assert await m.charge_rate_limit(_key(rate_limit_rpm=2), 5) is False
575+
576+
@pytest.mark.asyncio
577+
async def test_charge_rate_limit_zero_units_is_noop(self) -> None:
578+
# A single-item batch (0 extra units) debits nothing beyond the request.
579+
m = _build_manager()
580+
tenant_key = _key(rate_limit_rpm=1)
581+
assert await m.charge_rate_limit(tenant_key, 0) is True
582+
assert await m.charge_rate_limit(tenant_key, 1) is True # budget untouched
583+
564584
@pytest.mark.asyncio
565585
async def test_check_rate_limit_applies_local_window_when_redis_reports_full(self) -> None:
566586
m = _build_manager(

0 commit comments

Comments
 (0)