fix(llm): log expected transient upstream failures at WARNING, not ERROR

[guangyu-reflexio]

Log expected transient upstream LLM failures at WARNING, not ERROR

Problem. A batch Sentry alert fired on ERROR-log volume traced to minimax API
instability — intermittent 529 (server overloaded) + ~25s timeouts in the
document-expansion and profile-dedup paths. Both paths already degrade
gracefully (FTS fallback for expansion; keep profiles un-deduped for
consolidation), so there is no user-facing failure — but the underlying
timeout was logged at ERROR in the LiteLLM client's catch-all and in the
consolidator, flooding error alerts for handled fallbacks.

Fix (log-level only — no behavior change).

• _litellm_text_generation.py: the request-end failure handler now classifies
  the caught exception. Expected transient upstream errors — Timeout,
  APITimeoutError, APIConnectionError, RateLimitError,
  InternalServerError, ServiceUnavailableError, and our own
  LLMHardTimeoutError (a TimeoutError subclass raised when a provider hang is
  killed) — log at WARNING; everything else (bugs, auth, malformed structured
  output) stays ERROR. It still raises LiteLLMClientError, so callers
  continue to own fatality. Classified by exception type name to avoid importing
  the heavy litellm/openai exception hierarchies at module import.
• consolidator.py: the best-effort dedup failure (returns profiles un-deduped)
  is a graceful degradation → WARNING, not ERROR.

Tests. New unit tests assert a transient upstream error (TimeoutError) logs
the request-end failure at WARNING while a genuinely-unexpected error
(RuntimeError) stays ERROR; both still raise. Full test_litellm_client_unit.py

• profile suite green (258 passed); ruff + pyright clean.

Not in scope (documented in the investigation): the document-expansion 25s
timeout is intentionally short (latency-sensitive; FTS fallback by design) and is
not extended here. If minimax overload becomes chronic, demoting it from
primary for these ops is a separate cost/quality decision.

Summary by CodeRabbit

• Bug Fixes

  • Improved logging for temporary upstream AI service failures by reporting them as warnings instead of errors.
  • Unexpected failures continue to be reported as errors for visibility.
  • Profile deduplication now gracefully falls back to retaining incoming profiles when processing fails, while preserving deletion markers.

• Tests

  • Added coverage verifying log severity for transient and unexpected request failures.

[coderabbitai]

📝 Walkthrough

Walkthrough

Transient LiteLLM/provider failures now produce warning-level request-end logs, while unexpected failures remain errors. Profile deduplication failures also log warnings and return the existing best-effort fallback result. Tests cover both transient and unexpected request failures.

Changes

Error observability and fallback handling

Layer / File(s)	Summary
Classify and log transient LiteLLM failures reflexio/server/llm/_litellm_text_generation.py, tests/server/llm/test_litellm_client_unit.py	Transient timeout, connection, rate-limit, overload, and unavailable-provider errors select WARNING-level request-end logging; unexpected exceptions select ERROR-level logging, with tests covering both paths.
Downgrade deduplication fallback logging reflexio/server/services/profile/components/consolidator.py	Deduplication failures log warnings and return incoming profiles with deletion markers removed, plus empty deletion and supersession lists.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• ReflexioAI/reflexio#142: Both changes handle LiteLLM timeout and transient failures with warning-oriented behavior.
• ReflexioAI/reflexio#233: Introduces the ProfileConsolidator component modified by this change.
• ReflexioAI/reflexio#273: Also modifies the shared LiteLLM request path.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 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 main change: transient LLM failures are now logged at WARNING instead of ERROR.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/minimax-transient-loglevel

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (1)

tests/server/llm/test_litellm_client_unit.py (1)

1471-1503: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Type-name matching path is untested.

The transient test only exercises the isinstance(exc, TimeoutError) branch of _is_expected_transient_llm_error. The type(exc).__name__ in _TRANSIENT_LLM_ERROR_NAMES branch — covering APITimeoutError, APIConnectionError, RateLimitError, InternalServerError, ServiceUnavailableError — has no test coverage. Consider adding a parametrized test that raises a dummy exception with one of those type names to verify the WARNING classification holds.

🤖 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 `@tests/server/llm/test_litellm_client_unit.py` around lines 1471 - 1503, The
transient logging tests do not cover the type-name fallback in
_is_expected_transient_llm_error. Add a parametrized test alongside
test_transient_upstream_error_logged_at_warning that raises dummy exceptions
whose class names match each entry in _TRANSIENT_LLM_ERROR_NAMES, then assert
the request-end failure records are WARNING and not ERROR while the client still
raises LiteLLMClientError.

🤖 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 `@tests/server/llm/test_litellm_client_unit.py`:
- Around line 1471-1503: The transient logging tests do not cover the type-name
fallback in _is_expected_transient_llm_error. Add a parametrized test alongside
test_transient_upstream_error_logged_at_warning that raises dummy exceptions
whose class names match each entry in _TRANSIENT_LLM_ERROR_NAMES, then assert
the request-end failure records are WARNING and not ERROR while the client still
raises LiteLLMClientError.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: a8110d89-9fdb-451f-aa0c-0080ea13d63f

📥 Commits

Reviewing files that changed from the base of the PR and between 3a17d44 and 54cd4b4.

📒 Files selected for processing (3)

• reflexio/server/llm/_litellm_text_generation.py
• reflexio/server/services/profile/components/consolidator.py
• tests/server/llm/test_litellm_client_unit.py