Skip to content

Commit 73d700b

Browse files
RatnamOjhaclaude
andcommitted
fix(backend): keep Developer API conversation reads under one shared ceiling
Splitting conversation list and detail reads into separately tunable policies (#8713) gave each route its own bucket, so a single API key can make 60 list reads *and* 60 detail reads per hour where it previously made 60 in total. The practical exposure delta is small -- detail reads return one conversation each, ~1% of what the list endpoint's 100-record pages already allow, and the polling vector from the original incident is still capped at 60/hr. The reason to fix it is structural: nothing bounds the policy set, so every future dev:conversation_*_read policy silently raises the aggregate again. Add a "dev:conversation_reads_total" policy that every conversation read charges before its per-route budget. The aggregate returns to 60/hr while list, detail and transcript budgets stay independently tunable underneath it, which also settles the review thread asking why detail was given the same value as list. Costs one extra Redis round trip on the two conversation read routes; the existing transcript sub-budget already charges two buckets on the same request. The added test drives both routes in alternation and asserts the aggregate, not a per-route budget, is what rejects the caller. It fails on the prior wiring with "120 != 60". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 99dbbb2 commit 73d700b

4 files changed

Lines changed: 88 additions & 2 deletions

File tree

backend/dependencies.py

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -339,12 +339,29 @@ def _require_conversations_read_scope(auth: ApiKeyAuth):
339339
)
340340

341341

342+
async def _check_conversation_read_budgets_async(
343+
*,
344+
request: Optional[Request],
345+
auth: ApiKeyAuth,
346+
route_policy_name: str,
347+
) -> None:
348+
"""Charge a conversation read against the shared ceiling, then its per-route budget.
349+
350+
The shared ceiling is checked first so sustained polling is rejected on the
351+
aggregate budget regardless of which read route it targets. Without it, adding a
352+
per-route policy would hand each key a fresh bucket and raise the total number of
353+
conversation reads it can make -- the opposite of what these limits are for.
354+
"""
355+
await _check_dev_api_key_rate_limit_async(request=request, auth=auth, policy_name="dev:conversation_reads_total")
356+
await _check_dev_api_key_rate_limit_async(request=request, auth=auth, policy_name=route_policy_name)
357+
358+
342359
async def get_auth_with_conversations_read(
343360
auth: ApiKeyAuth = Depends(get_api_key_auth),
344361
request: Request = None,
345362
) -> ApiKeyAuth:
346363
_require_conversations_read_scope(auth)
347-
await _check_dev_api_key_rate_limit_async(request=request, auth=auth, policy_name="dev:conversations_read")
364+
await _check_conversation_read_budgets_async(request=request, auth=auth, route_policy_name="dev:conversations_read")
348365
return auth
349366

350367

@@ -353,7 +370,9 @@ async def get_auth_with_conversation_detail_read(
353370
request: Request = None,
354371
) -> ApiKeyAuth:
355372
_require_conversations_read_scope(auth)
356-
await _check_dev_api_key_rate_limit_async(request=request, auth=auth, policy_name="dev:conversation_detail_read")
373+
await _check_conversation_read_budgets_async(
374+
request=request, auth=auth, route_policy_name="dev:conversation_detail_read"
375+
)
357376
return auth
358377

359378

backend/tests/unit/test_dependency_async_boundaries.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -282,7 +282,10 @@ async def exercise() -> None:
282282
assert policies == [
283283
'mcp:memories_read',
284284
'mcp:memories_write',
285+
# Each conversation read charges the shared ceiling before its per-route budget.
286+
'dev:conversation_reads_total',
285287
'dev:conversations_read',
288+
'dev:conversation_reads_total',
286289
'dev:conversation_detail_read',
287290
'dev:conversations',
288291
'dev:memories_read',

backend/tests/unit/test_rate_limiting.py

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -489,6 +489,7 @@ def test_all_router_policies_exist(self):
489489
"goals:advice",
490490
"goals:extract",
491491
"dev:conversations",
492+
"dev:conversation_reads_total",
492493
"dev:conversations_read",
493494
"dev:conversation_detail_read",
494495
"dev:conversation_transcript_read",
@@ -561,6 +562,7 @@ def test_developer_dependencies_have_read_rate_limits(self):
561562
for policy in [
562563
"dev:memories_read",
563564
"dev:action_items_read",
565+
"dev:conversation_reads_total",
564566
"dev:conversations_read",
565567
"dev:conversation_detail_read",
566568
"dev:conversation_transcript_read",
@@ -592,6 +594,62 @@ def test_developer_conversation_reads_emit_sanitized_audit_logs(self):
592594
self.assertNotIn("request.headers.get('Authorization'", dependencies_source)
593595
self.assertNotIn('request.headers.get("Authorization"', dependencies_source)
594596

597+
def test_conversation_reads_share_an_aggregate_ceiling(self):
598+
"""Per-route read policies must not raise the total reads a key can make.
599+
600+
Before list and detail were split into separate policies they shared one
601+
60/hr bucket. Giving detail its own 60/hr policy without a shared ceiling
602+
would let one key make 120 conversation reads an hour -- a loosening of the
603+
exact limit #8713 asked to tighten. The shared ceiling is what prevents that,
604+
so this drives both routes and asserts the aggregate, not the per-route, cap
605+
is what stops the caller.
606+
"""
607+
dependencies = importlib.import_module("dependencies")
608+
609+
auth = dependencies.ApiKeyAuth(
610+
uid="uid1",
611+
scopes=["conversations:read"],
612+
app_id="test-app",
613+
key_id="test-key",
614+
)
615+
616+
counters: dict[str, int] = {}
617+
618+
def counting_limiter(*, prefix, uid, app_id, key_id, policy_name):
619+
max_requests, _window = RATE_POLICIES[policy_name]
620+
counters[policy_name] = counters.get(policy_name, 0) + 1
621+
if counters[policy_name] > max_requests:
622+
raise HTTPException(status_code=429, detail="Rate limit exceeded")
623+
624+
async def drive() -> int:
625+
served = 0
626+
# Alternate routes so neither per-route bucket can be what stops us.
627+
for i in range(500):
628+
dep = (
629+
dependencies.get_auth_with_conversations_read
630+
if i % 2 == 0
631+
else dependencies.get_auth_with_conversation_detail_read
632+
)
633+
try:
634+
await dep(auth)
635+
except HTTPException as exc:
636+
self.assertEqual(exc.status_code, 429)
637+
break
638+
served += 1
639+
return served
640+
641+
with patch.object(dependencies, "check_api_key_rate_limit", counting_limiter):
642+
served = asyncio.run(drive())
643+
644+
umbrella_max, _window = RATE_POLICIES["dev:conversation_reads_total"]
645+
split_total = RATE_POLICIES["dev:conversations_read"][0] + RATE_POLICIES["dev:conversation_detail_read"][0]
646+
647+
self.assertEqual(served, umbrella_max)
648+
self.assertLess(served, split_total, "per-route budgets must not sum into a higher effective ceiling")
649+
# The shared ceiling, not a per-route budget, is what rejected the caller.
650+
self.assertLessEqual(counters["dev:conversations_read"], RATE_POLICIES["dev:conversations_read"][0])
651+
self.assertLessEqual(counters["dev:conversation_detail_read"], RATE_POLICIES["dev:conversation_detail_read"][0])
652+
595653
def test_developer_rate_limit_failures_log_without_request(self):
596654
dependencies = importlib.import_module("dependencies")
597655
auth = dependencies.ApiKeyAuth(

backend/utils/rate_limit_config.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,12 @@
118118
# MCP API-key contexts are keyed by app/key identity when available.
119119
"dev:memories_read": (120, 3600),
120120
"dev:action_items_read": (120, 3600),
121+
# Conversation reads are limited in two tiers. Every conversation read consumes
122+
# the shared "reads_total" ceiling plus its per-route budget, so splitting list
123+
# and detail into separately tunable policies cannot raise the aggregate number
124+
# of conversation reads one key can make. Transcript reads consume a third,
125+
# stricter bucket on top of the other two.
126+
"dev:conversation_reads_total": (60, 3600),
121127
"dev:conversations_read": (60, 3600),
122128
"dev:conversation_detail_read": (60, 3600),
123129
"dev:conversation_transcript_read": (25, 3600),

0 commit comments

Comments
 (0)