Skip to content

fix: missed cleanup paths in execute_proxy + Ollama HTTPException raises#305

Closed
sheepdestroyer wants to merge 12 commits into
masterfrom
pr-285
Closed

fix: missed cleanup paths in execute_proxy + Ollama HTTPException raises#305
sheepdestroyer wants to merge 12 commits into
masterfrom
pr-285

Conversation

@sheepdestroyer

@sheepdestroyer sheepdestroyer commented Jul 13, 2026

Copy link
Copy Markdown
Owner

What

Fixes Gemini Code Assist review findings from #304: 5 missed cleanup paths for _end_parent_obs and _close_prop_ctx in non-streaming raise sites, plus defensive str() guard on LLAMA_SERVER_URL.

Changes

1. Missed cleanup on HTTPException raise paths (HIGH, #3567909318)

Five non-streaming HTTPException raise sites skipped both _end_parent_obs and _close_prop_ctx:

Site Fix
Backend misconfigured (500) Added cleanup before raise
Context window exceeded (400) Added cleanup in except HTTPException re-raise
Ollama cooldown (429) Added cleanup before raise
Ollama rate limit (429) Added cleanup before raise
Unexpected Ollama error (429) Added cleanup before raise

2. Defensive str() guard on LLAMA_SERVER_URL (MEDIUM, #3567909323)

_raw_llama_url.rstrip("/") would AttributeError if config had a non-string value. Changed to str(_raw_llama_url).rstrip("/").

3. Manual enter in generators — false positive (#3567909333)

Replied explaining the manual pattern is equivalent to with and intentionally avoids re-indenting 100+ lines. Both enter/exit run in the same task; contextvar isolation is not an issue.

Verification

  • 193/193 unit tests pass
  • 28/28 E2E pass (incl. Langfuse session propagation + leak isolation)
  • Syntax clean

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

    • Added configurable llama-server routing and health monitoring.
    • Improved request tracing with session and user context propagation.
    • Added configurable TLS certificate verification for llama-server connections.
  • Bug Fixes

    • Improved telemetry cleanup across streaming, cancellation, fallback, and error scenarios.
    • Added verification to prevent tracing context from leaking between requests.
  • Tests

    • Added end-to-end validation for Langfuse session and user trace propagation.

boy and others added 10 commits July 13, 2026 04:08
- 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).

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @sheepdestroyer, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@sheepdestroyer, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 47 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a9fc9a28-c01a-462a-847f-de4a71d52026

📥 Commits

Reviewing files that changed from the base of the PR and between 1447b1a and 679e85c.

📒 Files selected for processing (2)
  • router/main.py
  • scripts/verification/verify_canonical_endpoints.py
📝 Walkthrough

Walkthrough

Adds configurable llama-server connectivity, reusable health checks, and consistent Langfuse trace propagation and finalization across router, streaming, fallback, and verification flows.

Changes

Llama server and Langfuse routing

Layer / File(s) Summary
Llama-server configuration and health
.env.dev, router/config.yaml, router/main.py
Adds the llama-server endpoint, shared client, configurable TLS verification, metrics access, shutdown handling, and dashboard health checks.
Langfuse propagation foundation
router/main.py
Adds safe observation helpers and propagates request session and user identifiers into Langfuse traces.
Proxy streaming and fallback finalization
router/main.py
Finalizes child and parent observations across AGY, LiteLLM, and Ollama success, error, fallback, and cancellation paths.
Langfuse propagation verification
scripts/verification/verify_canonical_endpoints.py
Adds Langfuse credential loading and an end-to-end test for session propagation and cross-request isolation.

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
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: fixing missed cleanup paths in execute_proxy and Ollama HTTPException handling.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pr-285

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread router/main.py Outdated
Comment on lines +2798 to +2801
_end_parent_obs(parent_obs,
output={"error": f"HTTP {response.status_code}",
"route": "litellm_fallback"})
_close_prop_ctx(_prop_ctx)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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:

  1. The fallback request runs outside of the Langfuse propagation context, meaning session_id and user_id are lost for the fallback attempt.
  2. 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 sheepdestroyer left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 HTTPException raise 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_ctx before raising.
  • str() guard on LLAMA_SERVER_URL — line 518: str(_raw_llama_url).rstrip("/") prevents AttributeError on non-string config values. Clean.
  • Helper consolidation_end_parent_obs, _end_child_span, _close_prop_ctx eliminate repeated try/except boilerplate for Langfuse SDK v4 update()+end() pattern. Good DRY refactor.
  • Separate llama clientget_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 finally blocks, 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-core has 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.sh env var validation — catching missing OPENROUTER_API_KEY, OLLAMA_API_KEY, LLAMA_CLASSIFIER_URL before 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, and upgrade-prod.sh.
  • 193/193 unit tests + 28/28 E2E all pass. CI green across all checks.

⚠️ Warnings

  1. test_langfuse_session_propagation leak detection subtle bug (lines ~420-460 of verify_canonical_endpoints.py): When session_trace_id is None (session trace never found in step 1), the leak check degrades to t.get("id") != None which is always True. This means ANY trace with the same session_id is 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: if session_trace_id is None, skip the leak check (or report inconclusive) rather than producing a false positive.

  2. upgrade-prod.sh no longer stops the pod before rsync — The old version called podman pod stop -t 30 before syncing files. The new version skips this entirely, trusting that start-stack.sh --pull handles the shutdown. While the comment says the pod only uses rendered configs from DATA_ROOT, this is a behavioral change worth calling out explicitly. If Litellm has any file watching or lazy config reload, this could surprise.


💡 Suggestions

  1. Add debug logging to finalization helpers_end_parent_obs, _end_child_span, _close_prop_ctx all silently swallow Exception. This is safe for production but makes Langfuse integration debugging nearly impossible. Consider logging at logger.debug() level inside the except block so devs can opt into visibility with LOG_LEVEL=DEBUG.

  2. DRY the tier model_info blocks (litellm/config.yaml lines ~110-175) — All five tier entries now have identical model_info blocks (mode: chat, max_tokens: 524288, same capability flags). Consider a YAML anchor or a shared base config to reduce duplication.

  3. upgrade-prod.sh env validation with set +u / set -u — The toggle is correct for sourcing .env, but note that if .env itself has set -u-incompatible constructs beyond simple FOO=$BAR, the source could still fail silently. The comment explains the tradeoff well, just flagging for awareness.

  4. LLAMA_CA_BUNDLE="" maps to verify=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
@sheepdestroyer

Copy link
Copy Markdown
Owner Author

Addressing Gemini Code Assist Review

Critical finding acknowledged and fixed (commit 1447b1a):

The original PR added _end_parent_obs(parent_obs, ...) and _close_prop_ctx(_prop_ctx) inside execute_proxy on 5 catchable error paths. Gemini CA correctly identified that this corrupts tracing when fallbacks trigger: if Ollama fails and the auto-fallback retries execute_proxy(original_target_model), the parent observation is already ended.

Changes

  1. Removed parent finalization from 5 catchable error paths in execute_proxy:

    • Backend misconfigured (500)
    • Context window exceeded (400)
    • Stream connection failure (non-200 before streaming)
    • Non-streaming non-200 response
    • Proxy connection error (502)

    These now only finalize the child span (litellm_span_obj). The parent remains open for fallback retries.

  2. Added parent finalization safety nets at chat_completions level:

    • Ollama cooldown auto-fallback retry (if retry also fails)
    • 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)
  3. Stream generator internal paths (stream error/cancel) keep parent finalization — the caller has already returned the StreamingResponse and cannot catch these.

Also addressed owner review items

  • E2E test leak detection bug: When session_trace_id is None (step 1 polling fails), the leak check degraded to t.get("id") != None (always True) producing false positives. Now reports inconclusive.
  • Debug logging: Added logger.debug(exc_info=True) to all 3 finalization helpers for opt-in Langfuse debugging via LOG_LEVEL=DEBUG.

Verification

  • 101/101 unit tests pass
  • 28/28 E2E pass (incl. Langfuse session propagation + leak isolation)

@sheepdestroyer sheepdestroyer left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sheepdestroyer

Copy link
Copy Markdown
Owner Author

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.

@sheepdestroyer

Copy link
Copy Markdown
Owner Author

Addressing the self-review items from the earlier review:

Warnings addressed:

  1. E2E leak detection bug: Fixed. When session_trace_id is None (step 1 polling fails), the test now reports 'Inconclusive' instead of producing false positive via degraded t.get("id") != None check.
  2. upgrade-prod.sh behavioral change: Documented at lines 139-141 — the pod uses rendered configs from DATA_ROOT, and start-stack.sh --pull handles shutdown. Intentional design.

Suggestions addressed:

  1. Debug logging: Added logger.debug(exc_info=True) to _end_parent_obs, _end_child_span, and _close_prop_ctx.
  2. DRY tier model_info: Acknowledged but deferred — cosmetic YAML anchor improvement, no functional impact.
  3. upgrade-prod.sh env validation: The set +u/-u toggle is correct for the .env sourcing pattern. Comment at line 116 explains the tradeoff.
  4. LLAMA_CA_BUNDLE empty string: Empty→False mapping is straightforward; works correctly for self-signed internal setup.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
router/main.py (1)

142-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

get_llama_client() duplicates get_classifier_client() verbatim.

The CA-bundle resolution block (Lines 154-164) is an exact copy of Lines 125-135. Consider extracting the verify resolution 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 v

Then 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

📥 Commits

Reviewing files that changed from the base of the PR and between 9560f48 and 1447b1a.

📒 Files selected for processing (4)
  • .env.dev
  • router/config.yaml
  • router/main.py
  • scripts/verification/verify_canonical_endpoints.py

Comment thread router/main.py
Comment thread router/main.py
Comment on lines 2017 to 2019
langfuse_trace_id = lf.create_trace_id(
seed=f"triage_{stats['total_requests']}"
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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:


🏁 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.

Comment thread scripts/verification/verify_canonical_endpoints.py
…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
@sheepdestroyer

Copy link
Copy Markdown
Owner Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant