Skip to content

LLM client rate limiting#242

Merged
samuelvkwong merged 24 commits into
mainfrom
feature/llm-client-rate-limiting
Jul 7, 2026
Merged

LLM client rate limiting#242
samuelvkwong merged 24 commits into
mainfrom
feature/llm-client-rate-limiting

Conversation

@mhumzaarain

@mhumzaarain mhumzaarain commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Introduces resilient, rate-limit-aware LLM request handling shared by every LLM
caller in the process (batch extraction/subscriptions and interactive chat), and
adds a proactive per-minute request cap on top of it.

  • Reactive 429 backoff (RateLimitGate): a per-process barrier so that when
    any caller gets rate-limited by the provider, every other in-flight/future LLM
    call backs off behind the same window instead of continuing to hammer the
    provider. Honors Retry-After (seconds, retry-after-ms, or HTTP-date — including
    a naive -0000 date, which is treated as UTC) when present, falls back to
    exponential backoff otherwise, and is capped by a configurable ceiling.
  • Proactive RPM cap (RpmLimiter): a hand-rolled per-process sliding-window
    log
    that caps LLM requests to at most LLM_MAX_RPM in any trailing 60s window,
    matching a provider whose 429 quota is itself a sliding window (not a continuous
    refill). It admits a full burst up to LLM_MAX_RPM, then returns capacity as each
    request ages out 60s after it was sent. Disabled by default (LLM_MAX_RPM=0).
  • Unified wait budget: both barriers share one per-call deadline
    (run_through_gate / run_through_gate_async). If a call can't get past
    either the gate or the RPM limiter within its budget, it's deferred with a
    RateLimited exception rather than blocking indefinitely. After acquiring an RPM
    slot the gate is re-checked, so a 429 arriving from another caller during the wait
    can't slip a request into a freshly-closed window.
  • New shared clients (radis/core/utils/llm_client.py): LLMClient
    (sync, for batch extract_data calls) and AsyncChatClient (async, for
    interactive chat) replace the old radis.chats.utils.chat_client.ChatClient,
    and are now used by extractions/processors.py, subscriptions/processors.py,
    and chats/views.py. Empty/refused completions raise an explicit
    LLMResponseError (with model + finish_reason) instead of a bare assert, so
    the failure is visible even under python -O.
  • Different wait budgets per context: batch jobs (extraction/subscriptions)
    get a long budget (LLM_RATE_LIMIT_MAX_WAIT_SECONDS, default 300s) since
    nothing is waiting on them interactively; chat gets a short budget
    (LLM_RATE_LIMIT_INTERACTIVE_MAX_WAIT_SECONDS, default 20s) since a user is
    waiting in the browser.
  • Interactive UX: chat views catch RateLimited and render a
    "service is busy" warning in the chat template instead of raising a 500. A
    rate-limit on the secondary title generation no longer discards a good answer —
    it falls back to the user prompt as the title and still creates the chat.
  • Transient (connection/5xx) errors get a small local retry
    (with_transient_retries) separate from the 429 gate, since they're
    per-request issues rather than a provider-wide stop signal. (5xx responses and
    timeouts are already covered via openai.InternalServerError /
    openai.APIConnectionError.)

New settings (all documented in example.env)

Setting Default Purpose
LLM_REQUEST_TIMEOUT_SECONDS 60.0 Per-request timeout for the OpenAI client
LLM_EXTRA_BODY disables provider "thinking" Extra body sent with extract_data (Qwen enables "thinking" by default, which corrupts structured output)
LLM_RATE_LIMIT_BACKOFF_BASE_SECONDS 5.0 Base for exponential 429 backoff
LLM_RATE_LIMIT_FALLBACK_MAX_SECONDS 120.0 Cap on header-less exponential backoff
LLM_RATE_LIMIT_HEADER_CEILING_SECONDS 3600.0 Safety cap on an absurd Retry-After
LLM_RATE_LIMIT_MAX_WAIT_SECONDS 300.0 Wait budget for batch calls
LLM_RATE_LIMIT_INTERACTIVE_MAX_WAIT_SECONDS 20.0 Wait budget for interactive chat
LLM_TRANSIENT_RETRY_ATTEMPTS / _BASE_SECONDS 2 / 1.0 Local retry for connection/5xx errors
LLM_MAX_RPM 0 (disabled) Proactive per-process requests/minute cap (sliding window)

Testing

  • Unit tests for RateLimitGate and RpmLimiter (test_rate_limit.py) using a
    deterministic FakeClock, covering backoff math, Retry-After parsing (incl.
    naive -0000 dates), sliding-window behavior (a full burst then a full-window
    wait, and capacity returning in a lump), deadline handling, the post-acquire gate
    re-check, and both sync/async paths.
  • Tests for the shared LLMClient/AsyncChatClient wiring (test_llm_client.py),
    including LLMResponseError on empty/refused completions and RPM deferral, plus
    settings defaults (test_llm_settings.py).
  • uv run cli lint and uv run cli test pass.

Not in scope

  • Cross-process coordination (e.g. multiple llm_worker replicas) — this is a
    per-process, in-memory limiter/gate, consistent with there being no shared
    backend (e.g. Redis) in the stack today. With >1 replica the effective cap is
    LLM_MAX_RPM × replicas; llm_worker runs a single replica by default.

Summary by CodeRabbit

  • New Features
    • Added shared rate-limit coordination and transient retry handling for AI requests across sync and async flows.
    • Expanded LLM configuration via environment settings (timeouts, rate-limit timing, and injected request payload controls).
  • Bug Fixes
    • Chat pages now show a warning banner when AI calls fail; title generation falls back to the user prompt when the service is busy.
    • Chat updates recover gracefully during rate limiting; extraction now uses the updated structured LLM client.
  • Tests
    • Added deterministic test coverage for rate limiting, retry behavior, and default LLM settings.

mhumzaarain and others added 16 commits June 30, 2026 17:41
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
run_through_gate(_async) armed the gate with the clamped pause but made the
defer-vs-wait decision against the raw, unclamped Retry-After. When a server
sent Retry-After above header_ceiling, this caused a spurious RateLimited
even though the clamped window fit the caller's budget. Drop the divergent
early-raise and let the loop's wait_until_open(_async) decide from the
armed (clamped) _blocked_until, as it already did correctly.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR adds shared LLM settings, rate-limit and retry utilities, new sync/async LLM client wrappers, and migration of chat/extraction callers to the new client paths. Chat views now render rate-limit errors, and tests cover the new timing and response handling.

Changes

LLM Resilience Feature

Layer / File(s) Summary
Configuration and environment settings
example.env, radis/settings/base.py, radis/core/tests/test_llm_settings.py
Adds LLM timeout, extra body, rate-limit backoff and wait settings, transient retry settings, and default-value tests.
Rate-limit gate and retry primitives
radis/core/utils/rate_limit.py, radis/core/tests/test_rate_limit.py
Defines the shared rate-limit gate, retry-after parsing, gate wrappers, transient retries, and deterministic tests for headers, deadlines, and retries.
LLM client wrappers and tests
radis/core/utils/llm_client.py, radis/chats/utils/testing_helpers.py, radis/core/tests/test_llm_client.py
Adds the new sync and async OpenAI clients, shared gating, synthetic OpenAI exception helpers, and tests for request shaping, retry behavior, and error handling.
Caller migration and chat error rendering
radis/extractions/processors.py, radis/subscriptions/processors.py, radis/chats/views.py, radis/chats/templates/chats/chat.html
Switches extraction and chat flows to the new LLM clients, handles RateLimited in chat views, and renders chat errors in the template.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

  • openradx/radis#241: Both PRs touch radis/chats/views.py, including chat_update_view, but they change different parts of the view logic.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately summarizes the main change: LLM client rate limiting.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/llm-client-rate-limiting

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces LLM resilience features, including a process-global rate-limit gate, a request-per-minute limiter, and transient retry mechanisms, alongside updates to LLM clients, views, and processors to handle rate limits. The code review identifies several critical improvement opportunities: expanding transient error handling to cover timeout and 5xx status errors, defensively parsing Retry-After headers to prevent timezone-related crashes, and re-verifying the rate-limit gate after acquiring RPM permits to avoid race conditions. Additionally, the feedback suggests converting messages to a list to prevent iterator exhaustion during retries and gracefully falling back to a default title in the chat creation view if title generation is rate-limited.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

logger = logging.getLogger(__name__)

# Transient, usually per-request failures (not rate-limits). Worth a small local retry.
TRANSIENT_ERRORS = (openai.APIConnectionError, openai.InternalServerError)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The TRANSIENT_ERRORS tuple currently only includes openai.APIConnectionError and openai.InternalServerError. It misses openai.APITimeoutError and other 5xx status errors like BadGatewayError (502), ServiceUnavailableError (503), and GatewayTimeoutError (504). Let's update this tuple to include openai.APITimeoutError and handle other 5xx errors dynamically in the retry functions.

Suggested change
TRANSIENT_ERRORS = (openai.APIConnectionError, openai.InternalServerError)
TRANSIENT_ERRORS = (openai.APIConnectionError, openai.APITimeoutError)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. openai.APITimeoutError is already a subclass of openai.APIConnectionError (verified on openai 2.43.0), so timeouts are covered by the current tuple. Also, the suggested diff drops InternalServerError, which would stop us retrying 5xx. All 5xx (502/503/504) map to InternalServerError in the SDK (_make_status_error: if status_code >= 500: return InternalServerError), so they're already retried. Keeping the tuple as-is.

Comment on lines +198 to +204
try:
retry_date = email.utils.parsedate_to_datetime(value)
except (TypeError, ValueError):
return None
if retry_date is None:
return None
return max(0.0, (retry_date - datetime.now(UTC)).total_seconds())

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If the Retry-After header is malformed or does not contain a timezone, email.utils.parsedate_to_datetime might return a naive datetime or raise an error. If it returns a naive datetime, the subtraction retry_date - datetime.now(UTC) will raise a TypeError (can't subtract offset-naive and offset-aware datetimes) which is outside the try-except block and will crash the process. Let's handle naive datetimes defensively and wrap the subtraction inside the try-except block.

Suggested change
try:
retry_date = email.utils.parsedate_to_datetime(value)
except (TypeError, ValueError):
return None
if retry_date is None:
return None
return max(0.0, (retry_date - datetime.now(UTC)).total_seconds())
try:
retry_date = email.utils.parsedate_to_datetime(value)
if retry_date.tzinfo is None:
retry_date = retry_date.replace(tzinfo=UTC)
return max(0.0, (retry_date - datetime.now(UTC)).total_seconds())
except (TypeError, ValueError):
return None

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, confirmed. parsedate_to_datetime returns a naive datetime for an RFC -0000 offset, and the subtraction sits outside the try, so a -0000 Retry-After date raises TypeError and propagates out of the gate. Will wrap the subtraction and normalize naive datetimes to UTC.

Comment thread radis/core/utils/rate_limit.py Outdated
Comment on lines +220 to +224
while True:
if not gate.wait_until_open(deadline):
raise RateLimited() # an earlier 429 armed a window past our budget
if rpm is not None and not rpm.acquire(deadline):
raise RateLimited() # RPM cap can't grant a permit within the budget

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There is a potential race condition here: if rpm.acquire(deadline) sleeps to wait for a permit, the gate might have been closed by another thread during that sleep. Since the gate is not re-verified after acquiring the permit, the request will proceed and hammer the provider while the gate is closed. Let's re-verify the gate immediately after acquiring the RPM permit.

    while True:
        if not gate.wait_until_open(deadline):
            raise RateLimited()  # an earlier 429 armed a window past our budget
        if rpm is not None and not rpm.acquire(deadline):
            raise RateLimited()  # RPM cap can't grant a permit within the budget
        # Re-verify the gate in case it was closed while waiting for the RPM permit
        if not gate.wait_until_open(deadline):
            raise RateLimited()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point — a small TOCTOU: if rpm.acquire sleeps for a permit while another thread arms the gate, we send one request against a freshly-closed gate. Impact is bounded (that request just gets a 429 and re-enters the loop; the gate is best-effort backoff, not a hard lock), and the re-check doesn't fully close the window, but it's cheap and narrows it. Will add the re-check.

Comment thread radis/core/utils/rate_limit.py Outdated
Comment on lines +246 to +250
while True:
if not await gate.wait_until_open_async(deadline):
raise RateLimited()
if rpm is not None and not await rpm.acquire_async(deadline):
raise RateLimited() # RPM cap can't grant a permit within the budget

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There is a potential race condition here: if await rpm.acquire_async(deadline) sleeps to wait for a permit, the gate might have been closed by another thread during that sleep. Since the gate is not re-verified after acquiring the permit, the request will proceed and hammer the provider while the gate is closed. Let's re-verify the gate immediately after acquiring the RPM permit.

    while True:
        if not await gate.wait_until_open_async(deadline):
            raise RateLimited()
        if rpm is not None and not await rpm.acquire_async(deadline):
            raise RateLimited()  # RPM cap can't grant a permit within the budget
        # Re-verify the gate in case it was closed while waiting for the RPM permit
        if not await gate.wait_until_open_async(deadline):
            raise RateLimited()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the sync path — will add the equivalent wait_until_open_async re-check after acquire_async.

Comment on lines +274 to +280
for attempt in range(attempts + 1):
try:
return fn()
except TRANSIENT_ERRORS:
if attempt == attempts:
raise # exhausted -> let the failure path handle it
sleep(base * 2**attempt) # 1s, 2s, ...

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Let's update with_transient_retries to catch any openai.APIStatusError with status_code >= 500 (such as 502 Bad Gateway, 503 Service Unavailable, 504 Gateway Timeout) in addition to connection/timeout errors, ensuring comprehensive transient error resilience.

    for attempt in range(attempts + 1):
        try:
            return fn()
        except TRANSIENT_ERRORS:
            if attempt == attempts:
                raise  # exhausted -> let the failure path handle it
            sleep(base * 2**attempt)  # 1s, 2s, ...
        except openai.APIStatusError as exc:
            if exc.status_code < 500 or attempt == attempts:
                raise
            sleep(base * 2**attempt)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already covered: the SDK raises all status_code >= 500 as openai.InternalServerError (_make_status_error), which is in TRANSIENT_ERRORS, so 502/503/504 are retried today. An explicit APIStatusError branch would be redundant. Leaving as-is.

Comment on lines +291 to +297
for attempt in range(attempts + 1):
try:
return await fn()
except TRANSIENT_ERRORS:
if attempt == attempts:
raise
await sleep(base * 2**attempt)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Let's update with_transient_retries_async to catch any openai.APIStatusError with status_code >= 500 (such as 502 Bad Gateway, 503 Service Unavailable, 504 Gateway Timeout) in addition to connection/timeout errors, ensuring comprehensive transient error resilience.

Suggested change
for attempt in range(attempts + 1):
try:
return await fn()
except TRANSIENT_ERRORS:
if attempt == attempts:
raise
await sleep(base * 2**attempt)
for attempt in range(attempts + 1):
try:
return await fn()
except TRANSIENT_ERRORS:
if attempt == attempts:
raise
await sleep(base * 2**attempt)
except openai.APIStatusError as exc:
if exc.status_code < 500 or attempt == attempts:
raise
await sleep(base * 2**attempt)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reasoning as the sync path — 5xx already retried via InternalServerError. No change.

Comment on lines +51 to +69
async def chat(
self,
messages: Iterable[ChatCompletionMessageParam],
max_completion_tokens: int | None = None,
max_wait: float | None = None,
) -> str:
if max_wait is None:
max_wait = float(settings.LLM_RATE_LIMIT_INTERACTIVE_MAX_WAIT_SECONDS)

return await run_through_gate_async(
_LLM_GATE,
max_wait,
lambda: with_transient_retries_async(
lambda: self._chat(messages, max_completion_tokens),
settings.LLM_TRANSIENT_RETRY_ATTEMPTS,
settings.LLM_TRANSIENT_RETRY_BASE_SECONDS,
),
rpm=_LLM_RPM_LIMITER,
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If messages is passed as a one-time iterator (such as a generator or map object), iterating it during retries will fail because it will be exhausted after the first attempt. Let's convert messages to a list defensively before entering the retry loop.

    async def chat(
        self,
        messages: Iterable[ChatCompletionMessageParam],
        max_completion_tokens: int | None = None,
        max_wait: float | None = None,
    ) -> str:
        if max_wait is None:
            max_wait = float(settings.LLM_RATE_LIMIT_INTERACTIVE_MAX_WAIT_SECONDS)

        # Convert to list to prevent exhaustion of one-time iterators during retries
        messages_list = list(messages)
        return await run_through_gate_async(
            _LLM_GATE,
            max_wait,
            lambda: with_transient_retries_async(
                lambda: self._chat(messages_list, max_completion_tokens),
                settings.LLM_TRANSIENT_RETRY_ATTEMPTS,
                settings.LLM_TRANSIENT_RETRY_BASE_SECONDS,
            ),
            rpm=_LLM_RPM_LIMITER,
        )

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All current callers pass a concrete list (views.py:69,83,190), and the old client forwarded messages the same way, so there's no actual exhaustion today — the retry loop only matters if a caller passes a one-shot generator, which nothing does. Leaving as-is; if we ever want to hard-guarantee it, the cleaner move is narrowing the annotation from Iterable to Sequence rather than defensively copying.

Comment thread radis/chats/views.py
Comment on lines +67 to +101
try:
# Generate an answer for the user prompt
answer = await client.chat(
[
{"role": "system", "content": instructions_system_prompt},
{"role": "user", "content": user_prompt},
],
)

# Generate a title for the chat
title_system_prompt = Template(
settings.CHAT_GENERATE_TITLE_SYSTEM_PROMPT
).substitute(
{"num_words": 6, "user_prompt": user_prompt, "assistant_response": answer}
)

title = await client.chat(
[
{"role": "system", "content": title_system_prompt},
{"role": "user", "content": user_prompt},
],
max_completion_tokens=20,
)
except RateLimited:
return render(
request,
"chats/_chat.html",
{
"chat": None,
"report": report,
"chat_messages": [],
"form": form,
"error": "The LLM service is busy. Please try again in a moment.",
},
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If the answer generation succeeds but the subsequent title generation fails with RateLimited, the entire request fails, discarding the successfully generated answer and forcing the user to re-submit. Let's wrap the title generation in its own try-except block and fallback to a default title (e.g., a truncated version of the user prompt) to preserve the generated answer and improve UX.

            try:
                # Generate an answer for the user prompt
                answer = await client.chat(
                    [
                        {"role": "system", "content": instructions_system_prompt},
                        {"role": "user", "content": user_prompt},
                    ],
                )
            except RateLimited:
                return render(
                    request,
                    "chats/_chat.html",
                    {
                        "chat": None,
                        "report": report,
                        "chat_messages": [],
                        "form": form,
                        "error": "The LLM service is busy. Please try again in a moment.",
                    },
                )

            # Generate a title for the chat
            title_system_prompt = Template(
                settings.CHAT_GENERATE_TITLE_SYSTEM_PROMPT
            ).substitute(
                {"num_words": 6, "user_prompt": user_prompt, "assistant_response": answer}
            )

            try:
                title = await client.chat(
                    [
                        {"role": "system", "content": title_system_prompt},
                        {"role": "user", "content": user_prompt},
                    ],
                    max_completion_tokens=20,
                )
            except RateLimited:
                # Fallback to a truncated version of the user prompt if title generation is rate-limited
                title = user_prompt[:50] + "..." if len(user_prompt) > 50 else user_prompt

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — a rate-limit on the secondary title call shouldn't discard a good answer. Will split the try so the answer path is preserved and fall back to a truncated prompt for the title when title generation is deferred.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (1)
radis/core/tests/test_llm_client.py (1)

30-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add request-contract coverage for AsyncChatClient.

The suite only locks down constructor kwargs on LLMClient. A matching async test would have caught chat-only wiring regressions like missing LLM_EXTRA_BODY forwarding or timeout/max-retries drift.

Also applies to: 83-88

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@radis/core/tests/test_llm_client.py` around lines 30 - 36, Add a matching
request-contract test for AsyncChatClient to cover the async chat path, since
only LLMClient constructor kwargs are currently asserted. In the relevant async
client test, patch the OpenAI async client used by AsyncChatClient, instantiate
it with settings that exercise timeout/max-retries and LLM_EXTRA_BODY, then
assert the created client/request wiring preserves max_retries, timeout, and
forwards the extra body fields. Use AsyncChatClient and the existing LLMClient
test as the reference points for where to mirror the coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@radis/chats/views.py`:
- Around line 67-101: The chat flow in the view handling the answer and title
generation drops a valid `answer` if `client.chat` for the title raises
`RateLimited`. Refactor the `try` around the title generation so the primary
answer path is preserved: keep the first `client.chat` result, catch
`RateLimited` only for the title call, and fall back to a simple/default title
while still continuing to create and render the chat.

In `@radis/core/utils/llm_client.py`:
- Around line 76-85: Stop logging raw LLM payloads in the LLM client paths: the
debug statements around the chat completion request/response in the chat method
and the report/prompt logging in the later prompt-building code should not emit
full messages or model output. Replace those logger.debug calls with
metadata-only logging (for example, counts, lengths, model name, or request ids)
using the existing LLM client methods and prompt/report helpers as anchors. Keep
enough context to trace requests without exposing chat content, generated
prompts, or completion text.
- Around line 81-85: The response handling in LLMClient’s chat completion path
should not rely on assert for runtime validation. In the method that calls
self._client.chat.completions.create and reads
completion.choices[0].message.content, replace the assertion with explicit error
handling that raises a real exception when content is missing or empty so
refusals and failures are surfaced even under optimized Python. Apply the same
pattern anywhere the client uses beta.chat.completions.parse and checks
message.parsed, using the existing LLM client methods/classes to locate the
change and preserve clear logging before raising.
- Around line 39-49: The async chat path in AsyncChatClient._chat is missing the
provider-specific request flags that LLMClient already includes, so chat
requests don’t match extraction behavior. Update the chat request construction
to forward LLM_EXTRA_BODY alongside the existing payload, using the same
request-building approach as LLMClient so both paths send the same provider
options. Locate the AsyncChatClient._chat method and ensure the extra body is
merged into the async chat completion call.

In `@radis/core/utils/rate_limit.py`:
- Around line 67-68: The fallback backoff in RateLimit’s pause calculation can
overflow before min() clamps it because the exponential term is computed first.
Update the logic in the rate limiting path around self._consecutive_429 / pause
so the exponent is bounded before evaluation, or otherwise cap the growth prior
to multiplying, keeping the pause pinned at self._fallback_max without risking
float overflow.

---

Nitpick comments:
In `@radis/core/tests/test_llm_client.py`:
- Around line 30-36: Add a matching request-contract test for AsyncChatClient to
cover the async chat path, since only LLMClient constructor kwargs are currently
asserted. In the relevant async client test, patch the OpenAI async client used
by AsyncChatClient, instantiate it with settings that exercise
timeout/max-retries and LLM_EXTRA_BODY, then assert the created client/request
wiring preserves max_retries, timeout, and forwards the extra body fields. Use
AsyncChatClient and the existing LLMClient test as the reference points for
where to mirror the coverage.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e9ca650f-9c20-4a0c-9e20-9fe8bbe54d1d

📥 Commits

Reviewing files that changed from the base of the PR and between 5cc8a79 and bcf165a.

📒 Files selected for processing (13)
  • example.env
  • radis/chats/templates/chats/chat.html
  • radis/chats/utils/chat_client.py
  • radis/chats/utils/testing_helpers.py
  • radis/chats/views.py
  • radis/core/tests/test_llm_client.py
  • radis/core/tests/test_llm_settings.py
  • radis/core/tests/test_rate_limit.py
  • radis/core/utils/llm_client.py
  • radis/core/utils/rate_limit.py
  • radis/extractions/processors.py
  • radis/settings/base.py
  • radis/subscriptions/processors.py
💤 Files with no reviewable changes (1)
  • radis/chats/utils/chat_client.py

Comment thread radis/chats/views.py
Comment thread radis/core/utils/llm_client.py
Comment thread radis/core/utils/llm_client.py
Comment thread radis/core/utils/llm_client.py
Comment thread radis/core/utils/rate_limit.py Outdated
@mhumzaarain mhumzaarain changed the title Feature/llm client rate limiting llm client rate limiting Jul 1, 2026
@mhumzaarain mhumzaarain changed the title llm client rate limiting LLM client rate limiting Jul 1, 2026
mhumzaarain and others added 8 commits July 1, 2026 14:47
Different LLM providers use different RPM quotas, so a single proactive
per-process cap is awkward to configure meaningfully. Reactive rate
limiting (the shared 429 backoff gate plus transient retries) already
handles provider limits regardless of their configuration, so drop the
proactive RpmLimiter entirely.

Removes the RpmLimiter class, the rpm parameter on run_through_gate/
run_through_gate_async (and the redundant post-acquire gate re-check),
the _LLM_RPM_LIMITER global, the LLM_MAX_RPM setting/env var, and all
related tests.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rename LLM_RATE_LIMIT_FALLBACK_MAX_SECONDS to
LLM_RATE_LIMIT_BACKOFF_MAX_SECONDS (and the gate's fallback_max
param/attr to backoff_max) to better describe what it caps.

Change the header ceiling from a clamp into a sanity threshold: a
Retry-After >= LLM_RATE_LIMIT_HEADER_CEILING_SECONDS is now treated as
absurd and ignored, falling back to the shared exponential backoff
ladder instead of blocking the gate for the full ceiling.
Tests from main (#241) mocked parse() without extra_body and asserted a
system-role message, matching the old ChatClient. The new core LLMClient
always passes extra_body and sends the prompt as a user message.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@samuelvkwong samuelvkwong merged commit 4bfa8e7 into main Jul 7, 2026
2 checks passed
@samuelvkwong samuelvkwong deleted the feature/llm-client-rate-limiting branch July 7, 2026 08:38
samuelvkwong added a commit that referenced this pull request Jul 9, 2026
…limit gate

Drop the DB-backed cross-process backoff (EmbeddingBackoffState singleton,
call_with_429_backoff) in favor of a per-process EMBEDDING_GATE built on the
generic RateLimitGate that main introduced for LLM traffic (#242). The
embedding gateway keeps its own gate so a 429 from one provider never pauses
the other; cross-process coordination isn't worth the model/migration surface
since each container backs off on the 429s it receives itself.

The search path loses its explicit shared-pause bypass and instead runs
through the gate with a short query budget — when the gate is closed beyond
that budget, RateLimited raises and the provider falls back to FTS-only.
Background batches get a long budget; a RateLimited past it escapes stamina
and fails the subjob over to Procrastinate's retry policy.

Migration 0005 is deleted outright (never applied anywhere), consistent with
the branch's earlier never-applied-migration squash.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
mhumzaarain added a commit that referenced this pull request Jul 14, 2026
Adopt main's global LLM rate limiting (PR #242) and drop the branch's
labeling-specific layer it superseded:

- resolve settings/base.py in favor of main's LLM_RATE_LIMIT_* /
  LLM_TRANSIENT_RETRY_* settings; remove the LABELING_RATE_LIMIT_* block
- accept deletion of chats/utils/chat_client.py; remove the branch's
  chats/utils/rate_limit.py and labels/throttled_client.py (+ their tests),
  superseded by core/utils/{llm_client,rate_limit}.py and core tests
- switch labels/labeling.py to radis.core.utils.llm_client.LLMClient
- attach reports to a group in labels search-filter tests to satisfy
  main's group-scoped _build_filter_query

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants