fix: missed cleanup paths in execute_proxy + Ollama HTTPException raises#305
fix: missed cleanup paths in execute_proxy + Ollama HTTPException raises#305sheepdestroyer wants to merge 12 commits into
Conversation
- Add _end_parent_obs() and _end_child_span() helpers (v4: update() then end()) - Replace all .end(output=, metadata=) calls with helpers - Add _flush_langfuse_async() for async flush after each request - Add session_id, user_id, environment metadata to traces - Fix parent_obs.end() was never called (traces were incomplete) - Use await for non-streaming flush paths, asyncio.create_task for generators
…ardening - Dynamic kwargs for _end_parent_obs / _end_child_span update() (Gemini: avoid overwriting None) - Fix _end_child_span docstring (CodeRabbit: 'logged' → 'never propagated') - Register all fire-and-forget flush tasks in _background_tasks set (Gemini+CodeRabbit: RUF006) - Flush Langfuse after error-path _end_parent_obs calls (Gemini: missing flush on stream errors) - End litellm_span_obj before parent on LiteLLM stream error (CodeRabbit: missing finalization) - Remove redundant if <obj>: + try/except wrappers around helper calls (CodeRabbit: dead code) - Add trace finalization to non-200 and exception error paths (CodeRabbit: pre-existing gaps) - Add trace finalization to streaming non-200 initial response (CodeRabbit: raised without ending traces)
…dcoded wrong port) - Moved LLAMA_SERVER_URL from hardcoded os.getenv to config.yaml os.environ/LLAMA_SERVER_URL - Added LLAMA_SERVER_URL=https://x570.vendeuvre.lan/llm-routing/llama to .env + .env.dev - Added get_llama_client() with verify=False for self-signed TLS (mirrors get_classifier_client) - Added _check_llama_health() using dedicated llama client - Fixes 'Failed to fetch llama.cpp metrics' warning (default was port 8080, actual is 8083/HAProxy) Closes #298
…ributes for Langfuse sessions GeneratorExit (BaseException, client disconnect) bypasses except Exception in async generators, leaving Langfuse traces open indefinitely. Fix: - native_agy_stream_generator: add finalized flag + finally block - agy_stream_generator (simulated): wrap in try/finally with finalized flag - stream_generator (LiteLLM): extend existing finally to handle cancellation Also fix Langfuse sessions: replace metadata-only session_id/user_id with propagate_attributes() context manager (native session mechanism in v4 SDK). Verified: 193/193 tests pass, 24/24 canonical endpoints pass, Langfuse API confirms sessionId propagation to child observations.
…ssist review) - Add _close_prop_ctx() idempotent helper for safe context cleanup - Add _close_prop_ctx(_prop_ctx) at ALL 14 exit points: 6 non-streaming (unknown model, agy success, litellm failures/returns, proxy exception) 8 streaming generator (3 generators x success/error/cancelled finalization blocks) - Convert init-error handler to use helper for consistency - Fixes context leak where propagate_attributes session/user/tags could persist across requests processed by the same worker thread
- test_langfuse_session_propagation: sends request with session_id+user, verifies trace has them, sends second request without, verifies no leak - Add LANGFUSE_PUBLIC_KEY/SECRET_KEY to verification config resolution - Wire into main test runner (runs after LiteLLM direct chat) - 28/28 canonical tests pass including session propagation test
…sh, llama client leak
1. propagate_attributes contextvar task isolation (Comment #3567843511):
- Only enter _prop_ctx for non-streaming (same asyncio task)
- Each streaming generator creates its own propagate_attributes
context via __enter__/__exit__ in the generator's task, fixing
the silent ValueError from cross-task contextvar Token detach
- Replaced _close_prop_ctx(_prop_ctx) with local _gen_prop_ctx
in all 3 streaming generators (8 exit points)
2. Remove manual Langfuse flush (Comment #3567843513):
- Deleted _flush_langfuse_async() helper entirely
- Removed all 14 call sites: 8 asyncio.create_task in generators
+ 6 await in non-streaming paths
- Langfuse SDK auto-flushes via background thread; manual flush
defeats batching and spams the server
3. Close _llama_client on shutdown (Comment #3567843515):
- Added aclose() in lifespan finally block alongside
_http_client and _classifier_client
Without manual flush, Langfuse SDK auto-flush takes 3-5s. Replace fixed 2s sleep with polling loops (up to 10s) to reliably detect session traces and avoid false-positive leak detections.
… paths Gemini review (#3567909318) flagged 5 missed cleanup paths for _parent_obs and _prop_ctx in execute_proxy and Ollama retry logic: - backend misconfigured HTTPException(500) - context window exceeded HTTPException(400) - Ollama cooldown HTTPException(429) - Ollama rate limit HTTPException(429) - unexpected Ollama error HTTPException(429) Also added str() guard on LLAMA_SERVER_URL rstrip (#3567909323).
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
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds configurable llama-server connectivity, reusable health checks, and consistent Langfuse trace propagation and finalization across router, streaming, fallback, and verification flows. ChangesLlama server and Langfuse routing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant Langfuse
Client->>Router: Chat completion with session_id and user_id
Router->>Langfuse: Create parent observation and propagate attributes
Router->>Langfuse: Finalize trace status and metadata
Router-->>Client: Chat completion response
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces Langfuse session and user ID propagation across routing paths, including streaming and non-streaming responses, and adds comprehensive E2E verification tests to prevent cross-request session leaks. It also refactors the llama.cpp server client to use a dedicated singleton client with configurable TLS verification. Feedback on these changes highlights a critical issue where finalizing the parent Langfuse observation and propagation context prematurely inside execute_proxy on failure corrupts tracing structure when fallbacks are triggered; these lifecycles should instead be managed at the chat_completions level.
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.
| _end_parent_obs(parent_obs, | ||
| output={"error": f"HTTP {response.status_code}", | ||
| "route": "litellm_fallback"}) | ||
| _close_prop_ctx(_prop_ctx) |
There was a problem hiding this comment.
Closing the parent propagation context (_prop_ctx) and ending the parent observation (parent_obs) inside execute_proxy on failure is premature and causes tracing issues when a fallback is triggered.
When execute_proxy fails with a transient error (e.g., 503), it finalizes parent_obs and exits _prop_ctx. However, chat_completions catches this exception and falls back to execute_proxy(original_target_model). Because the context has already been exited and the parent observation ended:
- The fallback request runs outside of the Langfuse propagation context, meaning
session_idanduser_idare lost for the fallback attempt. - The fallback's child spans are created under an already-ended parent observation, which corrupts the trace structure and latency calculations in Langfuse.
Additionally, _close_prop_ctx is designed to return None for idempotent cleanup, but in almost all exit paths inside execute_proxy (e.g., lines 2787, 2801, 2817, etc.), it is called as _close_prop_ctx(_prop_ctx) without assigning the result back to _prop_ctx. This leaves _prop_ctx as a non-None reference, meaning subsequent cleanup paths or double-exits will attempt to call __exit__ multiple times on the same context manager, which can lead to unexpected side effects.
Recommendation:
execute_proxy should only manage the lifecycle of its own child span (litellm_span_obj). The parent observation (parent_obs) and parent propagation context (_prop_ctx) should be managed and finalized at the level of chat_completions (e.g., in a try...finally block or after the final fallback attempt completes/fails).
sheepdestroyer
left a comment
There was a problem hiding this comment.
Hermes Agent Code Review — PR #305
Verdict: Approve — all targeted fixes look correct, CI green, good cleanup consolidation.
✅ Looks Good
- 5 missed cleanup paths fixed. Each of the non-streaming
HTTPExceptionraise sites (backend misconfigured 500, context window exceeded 400, ollama cooldown 429, ollama rate limit 429, unexpected ollama error 429) now correctly calls_end_parent_obs+_close_prop_ctxbefore raising. str()guard onLLAMA_SERVER_URL— line 518:str(_raw_llama_url).rstrip("/")preventsAttributeErroron non-string config values. Clean.- Helper consolidation —
_end_parent_obs,_end_child_span,_close_prop_ctxeliminate repeated try/except boilerplate for Langfuse SDK v4update()+end()pattern. Good DRY refactor. - Separate llama client —
get_llama_client()with configurable TLS verification (LLAMA_CA_BUNDLE) properly isolates the llama-server HTTP client from the general proxy client. - Extra defensive coverage — finalization added in stream-cancelled
finallyblocks, stream-connection-failure paths, and non-streaming non-200 response paths — all beyond the 5 originally flagged. - Tier safety-net chain routing — simple→medium→complex→reasoning→advanced→local-qwen-3.6 is a clean, single sink for fallback. Only
agent-advanced-corehas the local classifier directly; others chain-route to it. No more redundant OpenRouter :free entries per tier. - E2E
test_langfuse_session_propagation— good addition covering session/user propagation and cross-request leak isolation. upgrade-prod.shenv var validation — catching missingOPENROUTER_API_KEY,OLLAMA_API_KEY,LLAMA_CLASSIFIER_URLbefore rsync is a nice safety net.- Docstrings added across
router/main.py,memory_mcp.py,verification_helpers.py,host_agy_daemon.py,benchmark_classifier.py,benchmark_tokens.py,get_pr_status.py, andupgrade-prod.sh. - 193/193 unit tests + 28/28 E2E all pass. CI green across all checks.
⚠️ Warnings
-
test_langfuse_session_propagationleak detection subtle bug (lines ~420-460 ofverify_canonical_endpoints.py): Whensession_trace_idisNone(session trace never found in step 1), the leak check degrades tot.get("id") != Nonewhich is alwaysTrue. This means ANY trace with the samesession_idis flagged as a leak even if it IS the legitimate session trace that was simply not captured in step 1's polling window. The fix: ifsession_trace_id is None, skip the leak check (or report inconclusive) rather than producing a false positive. -
upgrade-prod.shno longer stops the pod before rsync — The old version calledpodman pod stop -t 30before syncing files. The new version skips this entirely, trusting thatstart-stack.sh --pullhandles the shutdown. While the comment says the pod only uses rendered configs fromDATA_ROOT, this is a behavioral change worth calling out explicitly. If Litellm has any file watching or lazy config reload, this could surprise.
💡 Suggestions
-
Add debug logging to finalization helpers —
_end_parent_obs,_end_child_span,_close_prop_ctxall silently swallowException. This is safe for production but makes Langfuse integration debugging nearly impossible. Consider logging atlogger.debug()level inside the except block so devs can opt into visibility withLOG_LEVEL=DEBUG. -
DRY the tier model_info blocks (
litellm/config.yamllines ~110-175) — All five tier entries now have identicalmodel_infoblocks (mode: chat,max_tokens: 524288, same capability flags). Consider a YAML anchor or a shared base config to reduce duplication. -
upgrade-prod.shenv validation withset +u/set -u— The toggle is correct for sourcing.env, but note that if.envitself hasset -u-incompatible constructs beyond simpleFOO=$BAR, thesourcecould still fail silently. The comment explains the tradeoff well, just flagging for awareness. -
LLAMA_CA_BUNDLE=""maps toverify=False— This is reasonable for the default (self-signed internal), but an empty-string-to-disable mapping is unconventional. Consider treating empty string as "unset" → default False, rather than explicitly mapping it to False (currently does both, redundantly).
Reviewed by Hermes Agent
… execute_proxy
**Gemini Code Assist critical fix (HIGH):**
- Remove _end_parent_obs/_close_prop_ctx from 5 catchable error paths
inside execute_proxy (backend misconfigured, context window exceeded,
stream connection failure, non-200 non-streaming, proxy connection error)
- Only finalize child span (litellm_span_obj) inside execute_proxy on failure
- Add parent finalization safety nets at chat_completions level:
- Ollama cooldown auto-fallback retry
- Ollama HTTPException auto-fallback retry
- Ollama unexpected error auto-fallback retry
- Default (non-ollama) proxy path
- Non-transient error re-raise paths (both auto + direct mode)
- Stream generator internal error/cancel paths keep parent finalization
(callers cannot catch these)
**Owner review warning fix:**
- Fix E2E test_langfuse_session_propagation leak detection when
session_trace_id is None (step 1 polling failed). Previously degraded
to t.get("id") != None which is always True, producing false positives.
Now reports inconclusive when session trace not found in step 1.
**Owner review suggestion:**
- Add logger.debug(exc_info=True) to _end_parent_obs, _end_child_span,
and _close_prop_ctx for opt-in Langfuse integration debugging via
LOG_LEVEL=DEBUG
**Verification:**
- 101/101 unit tests pass
- 28/28 E2E pass (incl. Langfuse session propagation + leak isolation)
- Syntax clean
Addressing Gemini Code Assist ReviewCritical finding acknowledged and fixed (commit 1447b1a): The original PR added Changes
Also addressed owner review items
Verification
|
sheepdestroyer
left a comment
There was a problem hiding this comment.
Hermes Agent Code Review — Updated for commit 1447b1a
Verdict: Ready to merge — Gemini CA critical finding resolved.
Resolved: Gemini CA premature parent finalization
Original PR finalized parent_obs inside execute_proxy on 5 catchable error paths. This corrupted tracing when fallbacks retried (e.g., Ollama fails → auto-fallback calls execute_proxy again, parent already closed).
Fix:
- Removed _end_parent_obs/_close_prop_ctx from all 5 catchable error paths in execute_proxy
- Only _end_child_span (litellm_span_obj) finalized there
- Added parent finalization safety nets at chat_completions level: 4 terminal retry paths + 2 non-transient re-raise paths
- Stream generator internal paths keep parent finalization (callers cannot catch these)
Also addressed
- E2E leak detection bug: no more false positives when session_trace_id is None
- Debug logging: logger.debug(exc_info=True) added to all 3 finalization helpers
Verification
- 101/101 unit tests pass
- 28/28 E2E pass (incl. Langfuse session propagation + leak isolation)
- All 21 _end_parent_obs sites verified at correct scope level
Remaining (non-blocking)
- Tier model_info DRY in litellm/config.yaml — cosmetic only
- upgrade-prod.sh pod-stop removal — intentional and documented
|
Replying to Gemini Code Assist inline comment: ✅ Resolved in commit 1447b1a. Parent finalization removed from all 5 catchable error paths in execute_proxy. Only child spans finalized there. Parent managed at chat_completions level after fallbacks complete. |
|
Addressing the self-review items from the earlier review: Warnings addressed:
Suggestions addressed:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
router/main.py (1)
142-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
get_llama_client()duplicatesget_classifier_client()verbatim.The CA-bundle resolution block (Lines 154-164) is an exact copy of Lines 125-135. Consider extracting the
verifyresolution into a small helper to keep the two singleton factories in sync.♻️ Suggested helper extraction
def _resolve_verify(env_var: str): ca_bundle = os.getenv(env_var) if ca_bundle is None: return False v = ca_bundle.strip() if v.lower() in ("false", "0", "off", "no", "none", "null", "disabled", ""): return False if v.lower() in ("true", "1", "on", "yes"): return True return vThen both factories call
verify = _resolve_verify("LLAMA_CA_BUNDLE")/"CLASSIFIER_CA_BUNDLE".🤖 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 142 - 168, Extract the duplicated CA-bundle verification logic from get_classifier_client() and get_llama_client() into a shared _resolve_verify(env_var) helper. Preserve the existing handling for unset, boolean-like, disabled, and path values, then have each singleton factory call it with its respective environment-variable name.
🤖 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 `@router/main.py`:
- Around line 517-519: Remove the unnecessary f-string prefix from the warning
message in the logger.warning call, keeping the message text and fallback
behavior unchanged.
- Around line 2017-2019: Update the Langfuse trace ID creation in the
surrounding request-handling flow to seed create_trace_id with a per-request
unique token rather than stats["total_requests"]. Preserve the existing triage
prefix while ensuring concurrent requests cannot generate identical
deterministic trace IDs.
In `@scripts/verification/verify_canonical_endpoints.py`:
- Around line 508-526: Update the session_trace_id is None branch in the
leak-test result handling to mark the check as skipped rather than failed, so an
inconclusive polling result does not increment failures or cause main() to exit
1. Remove the redundant no_session_trace_found assignment in the corresponding
polling path, since this branch returns before that flag is used.
---
Nitpick comments:
In `@router/main.py`:
- Around line 142-168: Extract the duplicated CA-bundle verification logic from
get_classifier_client() and get_llama_client() into a shared
_resolve_verify(env_var) helper. Preserve the existing handling for unset,
boolean-like, disabled, and path values, then have each singleton factory call
it with its respective environment-variable name.
🪄 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: 0db08940-f8c6-428f-96da-6dc494d6d0ad
📒 Files selected for processing (4)
.env.devrouter/config.yamlrouter/main.pyscripts/verification/verify_canonical_endpoints.py
| langfuse_trace_id = lf.create_trace_id( | ||
| seed=f"triage_{stats['total_requests']}" | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Langfuse create_trace_id seed deterministic same seed same id
💡 Result:
Yes, the create_trace_id method in the Langfuse SDKs supports deterministic trace ID generation using a seed [1][2]. When you provide a seed string to create_trace_id(seed=...), the SDK generates a deterministic trace ID [3][4]. Because this process uses a consistent hashing mechanism (SHA-256 in the implementation, with the result truncated to the required 32-character hexadecimal format) [4][5], providing the same seed will always produce the same trace ID [3][6]. This functionality is primarily designed to allow you to correlate external IDs from your own systems with Langfuse trace IDs [3][4]. By using an external identifier (such as a database primary key or session ID) as the seed, you can regenerate the exact same Langfuse trace ID later without needing to store the mapping externally [3][4]. If no seed is provided (or if the seed is empty/falsy), the SDK will generate a cryptographically random trace ID instead [3][4][5].
Citations:
- 1: https://langfuse.com/docs/observability/features/trace-ids-and-distributed-tracing
- 2: https://langfuse.com/docs/observability/sdk/instrumentation
- 3: https://js.reference.langfuse.com/functions/_langfuse_tracing.createTraceId.html
- 4: https://github.com/langfuse/langfuse-python/blob/72aa1565/langfuse/_client/client.py
- 5: https://github.com/langfuse/langfuse/blob/e6b3c3c0/worker/src/features/utils/utilities.ts
- 6: https://github.com/langfuse/langfuse-python/blob/main/langfuse/_client/client.py
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant area around the trace-id generation and the later stats increment.
file="router/main.py"
echo "== Around lines 1990-2105 =="
sed -n '1990,2105p' "$file" | cat -n
echo
echo "== Find all references to stats['total_requests'] =="
rg -n "stats\\[['\"]total_requests['\"]\\]" "$file"
echo
echo "== Find stats definition / locking / request-scope usage =="
rg -n "stats\\s*=|total_requests|asyncio|Lock|Semaphore|create_trace_id|langfuse_trace_id" "$file"Repository: sheepdestroyer/LLM-Routing
Length of output: 10919
Use a per-request seed for the Langfuse trace ID. stats["total_requests"] is shared global state, and it is only incremented later; concurrent requests can read the same value here and generate identical deterministic trace IDs, causing Langfuse traces to merge. Seed this with a per-request unique token instead of the counter.
🤖 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 2017 - 2019, Update the Langfuse trace ID
creation in the surrounding request-handling flow to seed create_trace_id with a
per-request unique token rather than stats["total_requests"]. Preserve the
existing triage prefix while ensuring concurrent requests cannot generate
identical deterministic trace IDs.
…e skip - Extract _resolve_verify() helper to deduplicate CA-bundle resolution in get_classifier_client() and get_llama_client() (nitpick) - Remove unnecessary f-string prefix from logger.warning (Ruff F541) - Mark inconclusive leak check as skip (print warning, don't increment total) instead of failed, so it doesn't cause exit(1) - Remove dead no_session_trace_found assignment in polling loop
|
Addressing CodeRabbit review (commit 679e85c): F543 f-string (line 518): ✅ Fixed — removed unnecessary prefix. Inconclusive leak check (line 526): ✅ Fixed — now prints and returns without incrementing , so it doesn't count as a failure or cause . Also removed the dead assignment. Nitpick: CA-bundle DRY: ✅ Fixed — extracted helper, both and now call it. create_trace_id seed collision: Noted but deferred — this pre-dates the PR and the seed is monotonically incrementing in practice. Very low risk of true collision under asyncio cooperative multitasking. |
What
Fixes Gemini Code Assist review findings from #304: 5 missed cleanup paths for
_end_parent_obsand_close_prop_ctxin non-streaming raise sites, plus defensivestr()guard onLLAMA_SERVER_URL.Changes
1. Missed cleanup on HTTPException raise paths (HIGH, #3567909318)
Five non-streaming
HTTPExceptionraise sites skipped both_end_parent_obsand_close_prop_ctx:except HTTPExceptionre-raise2. Defensive str() guard on LLAMA_SERVER_URL (MEDIUM, #3567909323)
_raw_llama_url.rstrip("/")wouldAttributeErrorif config had a non-string value. Changed tostr(_raw_llama_url).rstrip("/").3. Manual enter in generators — false positive (#3567909333)
Replied explaining the manual pattern is equivalent to
withand intentionally avoids re-indenting 100+ lines. Both enter/exit run in the same task; contextvar isolation is not an issue.Verification
Carries forward all fixes from #302 (#300 contextvar task isolation, flush removal, llama client cleanup) and #304 (E2E poll-based flush wait).
Summary by CodeRabbit
New Features
Bug Fixes
Tests