fix(llm): set HTTP timeout on Gemini clients#903
Conversation
WalkthroughGemini and Anthropic clients now use explicit, centralized HTTP timeout settings. Gemini embedding and registry construction preserve optional base URLs while always applying the timeout, with tests covering standard, override, and missing-URL cases. ChangesProvider timeout configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
…labs#785) Same wedge-class failure as the LLM client: a stalled Gemini embedding socket hangs the in-process reconciler, which shares the deriver worker's uvloop event loop. Apply the same 10-minute timeout here, in lockstep with src/llm/registry.py's _build_gemini_http_options.
|
Expanded scope after independent code review: a Gemini embedding client in |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/llm/test_registry.py (1)
25-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInitialize settings before importing the registry.
src.configandsrc.llm.registryare imported before this autouse fixture runs, so these environment variables cannot affect settings or the module-levelCLIENTSinitialization. Set them before collection (for example, in a pytest startup hook), or reload config and registry in an isolated test; then assertCLIENTS["gemini"]receives the timeout too.Based on PR objectives: all three Gemini constructors require regression coverage.
🤖 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/llm/test_registry.py` around lines 25 - 31, Move the environment setup from the _patch_settings fixture to pytest startup or another pre-import mechanism so src.config and src.llm.registry read the test values before module-level CLIENTS initialization. Ensure registry tests cover all three Gemini constructors and assert CLIENTS["gemini"] receives the configured timeout.
🤖 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/llm/test_registry.py`:
- Around line 25-31: Move the environment setup from the _patch_settings fixture
to pytest startup or another pre-import mechanism so src.config and
src.llm.registry read the test values before module-level CLIENTS
initialization. Ensure registry tests cover all three Gemini constructors and
assert CLIENTS["gemini"] receives the configured timeout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ca830af-c3ae-451e-96af-0f437671a65a
📒 Files selected for processing (4)
src/embedding_client.pysrc/llm/registry.pytests/llm/test_embedding_client.pytests/llm/test_registry.py
Summary
src/llm/registry.py(default factory, override factory, and module-levelCLIENTS["gemini"]).tests/llm/test_registry.pywith regression coverage on all three Gemini construction sites plus Anthropic-baseline locks.Closes #785.
Why
A stalled Gemini socket can hang the deriver worker indefinitely because the Gemini client is built without an HTTP timeout. All deriver workers share one uvloop event loop, so a single hung call wedges the entire process — representation, dreamer, and reconciler all stop. The Anthropic client already passes
timeout=600.0; this aligns the Gemini client with that behavior.google.genai.HttpOptions.timeoutis anintin milliseconds, so the default is600_000ms (10 min) — same wall-clock budget as Anthropic.Testing
.venv/bin/pytest tests/llm/ -q→ 231 passed.venv/bin/pytest tests/llm/test_registry.py -q→ 7/7 passed (5 red→green for the new Gemini-timeout behavior, 2 baseline Anthropic locks).venv/bin/ruff checkand.venv/bin/basedpyrighton the changed files: no new errorsBroader Postgres-backed deriver queue tests were not run locally (env-level Postgres auth blocker; same gap as PR #901).
Developed with AI assistance; reproduced (unit test) and reviewed locally before submission.
Summary by CodeRabbit
Bug Fixes
Tests