review: separate ttl_seconds from window_size_seconds in atomic ARGV

The Lua ARGV layout for `CHECK_AND_INCREMENT_BY_N_SCRIPT` documents two
distinct slots — `ttl_seconds` (counter-key TTL) and `window_size_seconds`
(sliding-window length) — but the Python call site collapsed both to
`int(window_size)`. They happen to be equal today, so the bug was latent,
but a future descriptor carrying a custom counter TTL would have been
silently mis-applied (counter key expiring at the wrong time relative to
the window).

Make the two values separate variables (`ttl_seconds`, `window_size_seconds`)
before the `script_args.extend([...])` call so the ARGV layout matches the
documented contract by construction. The values still flow through to
`per_counter_meta` so the in-memory fallback uses the same source of truth.

Also:
- Document the COUNTER-level (not descriptor-level) parallel-indexing
  invariant on `_build_atomic_response` and `per_counter_meta`. A descriptor
  with both RPM and TPM emits two counters and two meta entries; the Lua
  script's `i` enumerates counters and the over-limit return tuple's index
  maps directly to `per_counter_meta[i - 1]`. Comment surfaces this so a
  future contributor doesn't refactor the arrays apart.
- Annotate the `increments` literal in the batch limiter directly instead
  of routing through `cast(...)`. Drops an unneeded `cast` import.
- Comment on the dynamic limiter's tracking-only branch explaining why
  `should_rate_limit` (non-atomic) is intentional there: we want to bump
  the priority counter so future enforced windows have accurate usage,
  and the OVER_LIMIT response is intentionally ignored.

Tests: 65 passed (1 skipped), 0 regressions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Krrish Dholakia
2026-05-01 12:40:43 -07:00
co-authored by Claude Opus 4.7
parent eba0cdf3f5
commit ca3e659a3c
3 changed files with 43 additions and 21 deletions
+8 -11
View File
@@ -17,7 +17,7 @@ Quick summary:
- async_log_success_event() fires on GET /v1/batches/{id} (batch completion)
"""
from typing import TYPE_CHECKING, Any, Dict, List, Literal, Optional, Union, cast
from typing import TYPE_CHECKING, Any, Dict, List, Literal, Optional, Union
from fastapi import HTTPException
from pydantic import BaseModel
@@ -179,16 +179,13 @@ class _PROXY_BatchRateLimiter(CustomLogger):
model_has_failures=False,
)
increments = cast(
List[Dict[Literal["requests", "tokens"], int]],
[
{
"requests": batch_usage.request_count,
"tokens": batch_usage.total_tokens,
}
for _ in descriptors
],
)
increment: Dict[Literal["requests", "tokens"], int] = {
"requests": batch_usage.request_count,
"tokens": batch_usage.total_tokens,
}
increments: List[Dict[Literal["requests", "tokens"], int]] = [
increment for _ in descriptors
]
rate_limit_response = (
await self.parallel_request_limiter.atomic_check_and_increment_by_n(
@@ -562,6 +562,13 @@ class _PROXY_DynamicRateLimitHandlerV3(CustomLogger):
# If priority is NOT enforced (saturation below threshold) but
# priority_descriptors exist, increment them for tracking only — no
# check, no rollback. This matches the prior tracking semantics.
#
# Using the non-atomic should_rate_limit (instead of
# atomic_check_and_increment_by_n) is intentional here: we don't want
# to enforce the limit, we only want to bump the counter so the
# priority allocation has accurate usage when it later becomes
# enforced. The increment-then-check semantics of should_rate_limit
# are fine because we ignore the OVER_LIMIT response.
if priority_descriptors and not should_enforce_priority:
priority_tracking_response = await self.v3_limiter.should_rate_limit(
descriptors=priority_descriptors,
@@ -754,17 +754,25 @@ class _PROXY_MaxParallelRequestsHandler_v3(CustomLogger):
counter_key = self.create_rate_limit_keys(
descriptor_key, descriptor_value, rlt
)
# Counter-key TTL and window_size are conceptually distinct
# ("how long the counter Redis key lives" vs "how long the
# sliding window is"). They happen to be equal today because
# we have no descriptor type that needs them apart, but they
# are kept as separate variables here so a future custom-TTL
# descriptor doesn't reintroduce a silent expiry bug. Both
# the Lua script and the in-memory fallback read these from
# their respective ARGV / meta slots.
ttl_seconds = int(window_size)
window_size_seconds = int(window_size)
keys.extend([window_key, counter_key])
# Per-descriptor 4-tuple: limit, increment, ttl, window_size.
# window_size is per-descriptor — descriptors may carry custom
# windows distinct from self.window_size, and the Lua script
# uses this slot to evaluate window expiry.
# Per-counter 4-tuple matches the Lua ARGV layout exactly:
# [limit, increment, ttl_seconds, window_size_seconds].
script_args.extend(
[
int(limit_value),
inc_amount,
int(window_size),
int(window_size),
ttl_seconds,
window_size_seconds,
]
)
per_counter_meta.append(
@@ -775,8 +783,8 @@ class _PROXY_MaxParallelRequestsHandler_v3(CustomLogger):
"window_key": window_key,
"counter_key": counter_key,
"increment": inc_amount,
"ttl": int(window_size),
"window_size": int(window_size),
"ttl": ttl_seconds,
"window_size": window_size_seconds,
}
)
@@ -822,13 +830,23 @@ class _PROXY_MaxParallelRequestsHandler_v3(CustomLogger):
raw: List[Any],
per_counter_meta: List[Dict[str, Any]],
) -> RateLimitResponse:
"""Convert Lua script return value to RateLimitResponse."""
"""Convert Lua script return value to RateLimitResponse.
Indexing invariant: `per_counter_meta` and `KEYS` are parallel-indexed
at the COUNTER level, not the descriptor level. A descriptor with both
RPM and TPM limits emits two `(window_key, counter_key)` pairs and
two meta entries — one per counter. The Lua script's loop variable
`i` therefore enumerates counters, and the over-limit return tuple
`{1, i, ...}` carries a counter index that maps directly to
`per_counter_meta[i - 1]`. Keep these arrays parallel at the counter
level when modifying this code.
"""
if not raw:
return RateLimitResponse(overall_code="OK", statuses=[])
status_code = int(raw[0])
if status_code == 1:
# Over limit: { 1, descriptor_index (1-based), current_counter, limit }
# Over limit: { 1, counter_index (1-based), current_counter, limit }
descriptor_index = int(raw[1]) - 1
current_counter = int(raw[2])
limit = int(raw[3])