Skip to content

fix(llm): stop paging on exhausted-retry LLM failures; retry hard timeouts; per-model timeout floors - #142

Merged
yyiilluu merged 1 commit into
mainfrom
fix/sentry-warn-expected-failures
Jun 10, 2026
Merged

fix(llm): stop paging on exhausted-retry LLM failures; retry hard timeouts; per-model timeout floors#142
yyiilluu merged 1 commit into
mainfrom
fix/sentry-warn-expected-failures

Conversation

@yyiilluu

@yyiilluu yyiilluu commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Three LLM-reliability fixes that stop expected failure modes from surfacing as Sentry errors and reduce spurious hard-timeout failures:

  • Tool loop: run_tool_loop now catches LiteLLMClientError ahead of the generic except Exception handler and logs it at WARNING (event=tool_loop_llm_error, no traceback). By the time this exception reaches the loop, the client has already exhausted its retries and fallbacks — it is a known failure mode (provider errors, timeouts), not a code bug. The result is unchanged: finished_reason="error" with the accumulated messages, so downstream behavior is identical. The generic handler keeps logger.exception for truly unexpected errors.
  • Hard-timeout retry: the hard timeout kills the litellm subprocess, so litellm's num_retries never gets a chance to fire. The client now performs one explicit retry at its own level after LLMHardTimeoutError, covering transient provider hangs.
  • Per-model timeout floors: reasoning models routinely exceed the default 120s provider timeout on large extraction contexts. _MODEL_TIMEOUT_FLOOR_SECONDS raises the effective timeout to max(configured, floor) (currently minimax/MiniMax-M3 → 240s). Floors never lower a configured timeout, and an explicit per-call timeout kwarg bypasses them entirely.

Also includes minor cleanups: ruff formatting in setup_cmd.py/llm_utils.py and a docstring correction in extractor_interaction_utils.py (defaults come from config_schema constants, not hardcoded values).

Changes

  • reflexio/server/llm/tools.py — catch LiteLLMClientError in the native tool loop, log at WARNING, return graceful ToolLoopResult
  • reflexio/server/llm/litellm_client.py — one explicit retry after LLMHardTimeoutError; per-model timeout floor via _effective_timeout_for_model
  • reflexio/cli/commands/setup_cmd.py, reflexio/server/llm/llm_utils.py — formatting only
  • reflexio/server/services/extractor_interaction_utils.py — docstring fix
  • Tests: caplog assertions that LiteLLMClientError produces no ERROR-level records, hard-timeout retry coverage, timeout-floor coverage, and test updates for constants-based defaults

Test Plan

  • ruff check / ruff format clean on all changed files
  • pyright introduces no new errors vs base (identical error distribution before/after)
  • All 7 touched test files pass: 440 passed

Summary by CodeRabbit

  • New Features

    • Improved timeout handling for LLM providers that exceed default request timeouts, enhancing reliability for certain models.
  • Bug Fixes

    • Enhanced error handling and logging for LLM operations within tool execution to improve error visibility.
  • Configuration Changes

    • Updated default retrieval floor values and stride size defaults for improved performance tuning.

…eouts; per-model timeout floors

- run_tool_loop now catches LiteLLMClientError ahead of the generic
  handler and logs at WARNING: the client already exhausted its retries
  and fallbacks, so this is a known failure mode, not a code bug. Same
  finished_reason='error' result, so downstream behavior is unchanged.
- A hard timeout kills the litellm subprocess before num_retries can
  fire, so the client now owes one explicit retry at that level.
- Add per-model timeout floors (minimax/MiniMax-M3 -> 240s): reasoning
  models routinely exceed the default 120s on large extraction
  contexts. Floors never lower a configured timeout, and explicit
  per-call timeouts bypass them.
@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR introduces timeout resilience improvements to LLM request handling and error recovery. It adds per-model timeout floors to prevent providers that routinely exceed defaults from blocking, implements client-level retry on hard timeouts, adds graceful error handling in the tool loop, and updates configuration default expectations.

Changes

LLM Timeout Resilience & Error Handling

Layer / File(s) Summary
Model Timeout Floor Calculation & Integration
reflexio/server/llm/litellm_client.py, tests/server/llm/test_litellm_client_unit.py
Adds _MODEL_TIMEOUT_FLOOR_SECONDS mapping models to minimum timeout thresholds and _effective_timeout_for_model() helper that returns max(config.timeout, floor). Integrates into _build_completion_params() to enforce floors on all requests. Unit tests validate floor enforcement, override behavior, and per-call timeout precedence.
Hard Timeout Exception Retry at Client Level
reflexio/server/llm/litellm_client.py, tests/server/llm/test_litellm_client_unit.py
_make_request() now catches LLMHardTimeoutError, logs a warning, and automatically retries the completion once. If retry succeeds, result is returned; if it also times out, LiteLLMClientError is raised. Integration tests validate transient timeout is retried once then succeeds, and consecutive timeouts fail after two total attempts.
Tool Loop Dedicated LLM Error Handling
reflexio/server/llm/tools.py, tests/server/llm/test_tools.py
run_tool_loop() now catches LiteLLMClientError separately with WARNING-level structured logging, sets trace.finished = False, and returns error result with accumulated messages and pending tool calls. Test confirms error is logged at WARNING and trace reflects error state.
Configuration Default Updates
reflexio/server/services/extractor_interaction_utils.py, tests/models/test_retrieval_floor_config.py, tests/server/services/test_base_generation_service.py, tests/server/services/test_extractor_interaction_utils.py
Updates stride size default from 5 to 8, retrieval floor config defaults from -5.0 to -3.0, and docstring in get_extractor_window_params() to reference DEFAULT_WINDOW_SIZE / DEFAULT_STRIDE_SIZE from config_schema. Related test setup and assertions updated accordingly.
Code Formatting & Minor Style Adjustments
reflexio/cli/commands/setup_cmd.py, reflexio/server/llm/llm_utils.py, tests/server/llm/test_model_defaults.py, tests/server/services/playbook/test_playbook_aggregator.py
Reformats CLI provider dict to multi-line layout, adds spacing in utilities, and adjusts test assertion formatting for consistency. Converts playbook test request instantiations to single-line form.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • ReflexioAI/reflexio#124: Refactors _build_completion_params and _make_request timeout/retry logic using native LiteLLM num_retries/fallbacks, overlapping with this PR's model floor and hard-timeout retry mechanisms.
  • ReflexioAI/reflexio#136: Introduces LLMHardTimeoutError via hard client-side timeout wrapper; this PR explicitly catches that error and implements client-level retry.
  • ReflexioAI/reflexio#62: Modifies _build_completion_params for seed and temperature=0.0 opt-in logic, sharing the same completion-params integration point as this PR's timeout floor changes.

Poem

🐰 A timeout floor, a retry beat,
Makes stubborn providers play it neat,
Hard timeouts caught, one chance to try,
Tool loops dance on, they'll not crash and die,
Defaults renewed, the config flows true!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 65.71% 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 accurately summarizes the three main changes: stopping on exhausted-retry LLM failures (tool loop error handling), retrying hard timeouts (client-level retry), and per-model timeout floors (timeout configuration).
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/sentry-warn-expected-failures

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 and usage tips.

@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.

🧹 Nitpick comments (1)
reflexio/server/llm/tools.py (1)

581-582: 💤 Low value

Comment is slightly misleading about "type-only dependency".

The comment states the local import keeps litellm_client a "type-only dependency," but this is a runtime import needed to catch the exception. The intent appears to be minimizing runtime coupling by importing only in the native tool-calling path, but "type-only" specifically refers to imports under if TYPE_CHECKING:. Consider rephrasing to something like "Local import defers the runtime dependency to the native tool-calling path only."

🤖 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 `@reflexio/server/llm/tools.py` around lines 581 - 582, The comment near the
local import of LiteLLMClientError is misleading—it's a runtime import, not a
type-only import—so update the comment to reflect that this local import defers
the runtime dependency to the native tool-calling path instead of claiming it is
"type-only"; locate the import of LiteLLMClientError in
reflexio.server.llm.tools and replace the comment text to something like "Local
import defers the runtime dependency to the native tool-calling path only" (or
equivalent wording) so it accurately describes intent without implying
TYPE_CHECKING semantics.
🤖 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.

Nitpick comments:
In `@reflexio/server/llm/tools.py`:
- Around line 581-582: The comment near the local import of LiteLLMClientError
is misleading—it's a runtime import, not a type-only import—so update the
comment to reflect that this local import defers the runtime dependency to the
native tool-calling path instead of claiming it is "type-only"; locate the
import of LiteLLMClientError in reflexio.server.llm.tools and replace the
comment text to something like "Local import defers the runtime dependency to
the native tool-calling path only" (or equivalent wording) so it accurately
describes intent without implying TYPE_CHECKING semantics.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b05c8763-52df-48fa-87ab-db470b279500

📥 Commits

Reviewing files that changed from the base of the PR and between b079a28 and a14cca3.

📒 Files selected for processing (12)
  • reflexio/cli/commands/setup_cmd.py
  • reflexio/server/llm/litellm_client.py
  • reflexio/server/llm/llm_utils.py
  • reflexio/server/llm/tools.py
  • reflexio/server/services/extractor_interaction_utils.py
  • tests/models/test_retrieval_floor_config.py
  • tests/server/llm/test_litellm_client_unit.py
  • tests/server/llm/test_model_defaults.py
  • tests/server/llm/test_tools.py
  • tests/server/services/playbook/test_playbook_aggregator.py
  • tests/server/services/test_base_generation_service.py
  • tests/server/services/test_extractor_interaction_utils.py

@yyiilluu
yyiilluu merged commit bfa916a into main Jun 10, 2026
1 check passed
@yilu331
yilu331 deleted the fix/sentry-warn-expected-failures branch June 12, 2026 07:11
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.

1 participant