Skip to content

feat: pre-adoption shadow eval for the auto-router (blind pairwise judge, per-tier win rates) - #36250

Closed
tin-berri wants to merge 34 commits into
litellm_internal_stagingfrom
shadow-eval-pre-adoption
Closed

feat: pre-adoption shadow eval for the auto-router (blind pairwise judge, per-tier win rates)#36250
tin-berri wants to merge 34 commits into
litellm_internal_stagingfrom
shadow-eval-pre-adoption

Conversation

@tin-berri

@tin-berri tin-berri commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

What

Pre-adoption shadow evaluation for the auto-router: answer "is it safe to turn the router on?" with evidence from the customer's own live traffic, before any production traffic is routed.

  • A sampled slice (configurable %) of a key's successful requests is duplicated through the auto-router in a detached background task, so there is zero added latency and shadow responses are never served to users
  • An LLM judge compares the real response vs. the router's pick blind, with A/B labels randomized to cancel position bias
  • Verdicts are stratified by the router's own tier classification (SIMPLE / COMPLEX / REASONING), so the result reads: "router's pick judged good-or-better 84% overall, weakest on REASONING at 58%"

How

Backend

  • LiteLLM_ShadowEvalJob + LiteLLM_ShadowEvalVerdict tables, plus migration 20260807000000_add_shadow_eval_job; verdict rows record ids and completion-token counts for BOTH arms, so verdicts can later be checked for judge verbosity bias and joined to the shadow call's spend log
  • ShadowEvalLogger (litellm/integrations/shadow_eval_logger.py): hooks async_log_success_event; the hook is one dict lookup against a job snapshot and never touches the DB; deterministic hash-based sampling, so the same request samples the same way across pods; caps in-flight shadow pipelines at 16 per pod by shedding rather than queueing, so a traffic spike can't build an unbounded task backlog; skips any request stamped internal_call_origin, so it never recurses on its own shadow/judge calls and never samples the auto-router's classifier sub-calls as if a user sent them
  • Job lifecycle runs on a 10s loop owned and started by logger registration itself (no separate startup step to reorder), not on the request path: counter flushes land without waiting for another request, request_count freezes when a job stops (status-guarded writes), and a job whose window or judge-spend cap has passed is completed even if its key goes quiet
  • The judge dispatches through the proxy's router when judge_model is a configured deployment (DB-stored credentials work) and through the SDK for provider-qualified public names; start rejects a judge name neither path can resolve (400) instead of accepting a job that fails every turn, and the most recent shadow/judge failure is persisted as last_error on the job and shown in the UI
  • Internal sub-calls (shadow, judge, classifier) keep billing spend and tokens to the shadowed key, but no longer count as api_requests in the daily rollups, no longer accrue autorouter_savings_spend, and are excluded from the LiteLLM_AutoRouterSession benchmarks rollup, so a running eval cannot inflate the adoption metrics or its own next cost estimate
  • Endpoints in auto_router_endpoints.py:
    • POST /auto_router/shadow_eval/start: validates the router is a configured pre-routing strategy and the judge model is resolvable (a deployment prices by its underlying provider model), rejects a second active job per key (409), returns an upfront judge-cost estimate priced from the key's trailing 7-day user-request volume
    • GET /auto_router/shadow_eval/{job_id}: counters and per-tier win rates (SQL aggregate over verdicts)
    • GET /auto_router/shadow_eval: list jobs
    • POST /auto_router/shadow_eval/{job_id}/stop: halt sampling (~10s propagation), keep verdicts
  • Start and stop require PROXY_ADMIN; the read endpoints also allow PROXY_ADMIN_VIEW_ONLY
  • Logger registered in cost_tracking(), which already owns the same prisma dependency

UI (Cost Optimization -> Auto-Router Usage)

  • ShadowEvalSection: start form (key, router, %, judge model, upfront cost estimate warning), live-polling job card, per-tier results table with low-sample warnings, running judge spend
  • useShadowEval.ts hooks; OpenAPI schema.d.ts regenerated

Scope / known limitations

  • Turn-level only. The shadow never influences the next real turn, so multi-turn compounding effects are out of scope for v1
  • Judge quality gate is a single verdict per turn, no multi-judge panel yet
  • Counters are buffered up to one 10s lifecycle tick, so displayed counts can lag briefly; stopping a job freezes request_count (up to a tick of tail counts on other pods is dropped rather than written after stop)
  • Judge cost estimate uses a flat 4k-prompt/200-output token assumption when the judge model is in the price map, with a $0.01/call fallback otherwise

Test plan

  • 76 unit tests in tests/test_litellm/integrations/test_shadow_eval_logger.py covering deterministic sampling, A/B unmask mapping, fence-tolerant verdict JSON parsing, success-hook skip paths (including classifier sub-calls), request-parameter forwarding, the in-flight backlog cap, router-first judge dispatch, last_error persistence, and the lifecycle-loop flush/finalize/status-guard behaviors, each mutation-checked against the unfixed code
  • Endpoint, rollup, and daily-writer suites cover judge-model validation at the start seam, internal-origin exclusion from LiteLLM_AutoRouterSession, and internal calls keeping spend while not counting as requests
  • 20 UI tests in ShadowEvalSection.test.tsx (failure banner included), plus the existing AutoRouterBenchmarksTab suite
  • Needs a staging pass with a live key and a configured auto-router before merge. Note for existing local DBs: migration 20260807000000_add_shadow_eval_job was amended in place (adds last_error, drops the never-read result_json), so a DB that already applied it needs a re-resolve or db push

Note

High Risk
Background duplicate LLM traffic bills the shadowed key and touches spend rollups, budget checks, and multi-pod job lifecycle; mis-attribution or metric inflation would affect billing and adoption dashboards.

Overview
Adds pre-adoption shadow evaluation so admins can sample a virtual key’s live traffic, replay it through an auto-router in the background, and compare answers with a blind LLM judge—without serving shadow responses to users.

Data & control plane: New LiteLLM_ShadowEvalJob / LiteLLM_ShadowEvalVerdict schema and migration, including a partial unique index so only one active job exists per key across pods. Admin endpoints under /auto_router/shadow_eval start jobs (with cost estimate from daily rollup volume), list/get aggregated win rates by tier and incumbent model, and stop jobs.

Runtime: ShadowEvalLogger registers with cost_tracking(), samples deterministically on successful completions, runs shadow + judge in detached tasks (concurrency cap, budget and per-job spend/time stops), and uses a 10s lifecycle loop for job snapshots, counter flushes, and auto-completion. Shared internal_call_metadata helpers stamp shadow/judge/classifier sub-calls; daily spend rollups and auto-router session benchmarks exclude those calls from request/savings metrics while still billing spend to the shadowed key.

UI: Cost Optimization’s auto-router tab gains ShadowEvalSection (start form, live results, jump link from benchmarks).

Reviewed by Cursor Bugbot for commit e51008f. Bugbot is set up for automated code reviews on this repo. Configure here.

akapur99 and others added 2 commits August 7, 2026 19:52
…eton)

Core components:
- Added LiteLLM_ShadowEvalJob and LiteLLM_ShadowEvalVerdict tables to schema
- Migration: 20260807000000_add_shadow_eval_job (SQL for both tables + indexes)
- Types: StartShadowEvalRequest, GetShadowEvalJobResponse, etc. (pydantic models)
- ShadowEvalLogger: CustomLogger integration that will fire background shadow tasks

TODOs for next phase:
1. Wire config parsing (read shadow_eval from model_info or key settings)
2. Implement router call (_call_router_for_shadow) to get classifier tier + model
3. Build endpoints (POST /auto_router/shadow_eval/start, GET .../job/{id})
4. Implement verdict accumulation + per-tier result binning
5. Hook ShadowEvalLogger into proxy initialization

Still unresolved (implementation details):
- Cost estimation accuracy (judge model pricing per region)
- Sub-sampling strategy for high-traffic keys
- Job lifecycle: incremental verdict writes vs batch
- Retry logic for failed router/judge calls

Co-Authored-By: Claude <noreply@anthropic.com>
Backend:
- ShadowEvalLogger: background task on async_log_success_event, deterministic sampling, blind pairwise judge, verdict persistence + job counter updates
- Endpoints: POST /auto_router/shadow_eval/start (cost estimate), GET /job_id (per-tier results), GET (list), POST /job_id/stop
- Schema + migrations: LiteLLM_ShadowEvalJob, LiteLLM_ShadowEvalVerdict tables
- Types: StartShadowEvalRequest, GetShadowEvalJobResponse, ShadowEvalResult, ShadowEvalTierResult
- Proxy wiring: logger registered in cost_tracking()

UI:
- useShadowEval.ts: hooks for start/stop mutations + job query with live polling
- ShadowEvalSection.tsx: consent-gate form, job status badge, per-tier results table
- Integrated into AutoRouterBenchmarksTab alongside existing benchmarks
- OpenAPI schema regenerated to include shadow_eval endpoints

Tests: unit tests for logger sampling/unmasking/verdict-parsing
Linting: all files python3.11 syntax-check + tsx lint ready

This ships the full pre-adoption evaluation flow:
1. User starts job: specifies key, router, sampling %, gets upfront cost estimate
2. Sampled requests duplicated through router, judged blind, verdicts persisted
3. Dashboard shows per-tier win rates, cost tracking, live progress
4. Ready for Tyler/Tinder/Access Group pre-launch sign-off

Co-Authored-By: Claude <noreply@anthropic.com>
@tin-berri
tin-berri requested a review from a team August 8, 2026 03:24
@greptile-apps

greptile-apps Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR adds pre-adoption auto-router shadow evaluation across persistence, management APIs, detached shadow/judge execution, spend attribution, and the dashboard.

  • Adds shadow-evaluation jobs, verdict aggregation, lifecycle endpoints, and Prisma schema support.
  • Adds sampled background router comparisons with blind judging and per-tier results.
  • Adds dashboard controls and live result polling.
  • The success callback still performs an inline spend lookup that can reach the database.

Confidence Score: 4/5

The PR should not merge until the sampled success callback no longer performs a spend lookup that can reach the database inline.

The active-job lookup was successfully moved to a detached snapshot refresh, but sampled requests still await key and team spend checks whose fallback path can query the database from the success callback.

Files Needing Attention: litellm/integrations/shadow_eval_logger.py

Important Files Changed

Filename Overview
litellm/integrations/shadow_eval_logger.py Implements the detached evaluation pipeline and snapshot lookup, but retains an inline spend check that can reach the database from the success callback.
litellm/litellm_core_utils/internal_call_metadata.py Adds sanitized attribution metadata for internal router and judge calls while removing parent budget reservations.
litellm/proxy/management_endpoints/auto_router_endpoints.py Adds authorized shadow-evaluation lifecycle and result endpoints with cost estimation and aggregate reporting.
litellm-proxy-extras/litellm_proxy_extras/migrations/20260807000000_add_shadow_eval_job/migration.sql Adds job and verdict persistence, including a partial unique index enforcing one active job per key.
ui/litellm-dashboard/src/app/(dashboard)/cost-optimization/_components/AutoRouterBenchmarksTab.tsx Correctly renders the shadow-evaluation section outside benchmark loading, error, and empty-state returns.
ui/litellm-dashboard/src/app/(dashboard)/cost-optimization/_components/ShadowEvalSection.tsx Adds the start form, running-job cards, polling results, tier summaries, and spend presentation.

Reviews (4): Last reviewed commit: "feat(ui): lead shadow eval results with ..." | Re-trigger Greptile

Comment thread litellm/integrations/shadow_eval_logger.py Outdated
Comment thread litellm/integrations/shadow_eval_logger.py Outdated
Comment thread litellm/integrations/shadow_eval_logger.py Outdated
Comment thread litellm/integrations/shadow_eval_logger.py
Comment thread litellm/integrations/shadow_eval_logger.py Outdated
@veria-ai

veria-ai Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

PR overview

All previously flagged issues have been addressed. No open security concerns remain on this pull request.

Security review

No open security issues remain on this pull request.

Fixed/addressed: 4 · PR risk: 0/10

@tin-berri

Copy link
Copy Markdown
Contributor Author

@BugBot

- Registry check now scans all pre-routing strategy registries (auto, complexity,
  adaptive, quality), not just auto_routers. Fixes 400 on adoption for
  complexity-router or adaptive-router users.

- Split auth into _require_admin_viewer (GET) and _require_admin_writer
  (start/stop); view-only admins can no longer initiate paid work (judge calls).

- request_count UPDATE buffering: in-memory counter flushed every 10s instead
  of one UPDATE per request. High-traffic keys now cost one DB op per flush
  interval, not per request.

- Default judge model: anthropic/claude-sonnet-5 (was unmapped claude-3-5-sonnet).
  Cost estimation now prices correctly; fallback is no longer needed.

- completion_cost error handling: try/except around litellm.completion_cost() so
  unmapped judge models don't crash the verdict write.

- UI: ShadowEvalSection now always renders (pre-adoption keys have no router
  sessions yet but still show the start form). Added judge_model parameter to
  the start form. Fixed accessToken undefined in AutoRouterBenchmarksTab.

Tests:
- test_shadow_eval_logger.py (26 tests): sampling, verdict parsing, unmasking,
  skip logic, metadata isolation.
- ShadowEvalSection.test.tsx (7 tests): form, active job display, per-tier
  results, low-sample flagging, completed job handling.
- All existing auto-router endpoint tests (22) and component tests (96) pass.

Co-Authored-By: Claude <noreply@anthropic.com>
Comment thread litellm/integrations/shadow_eval_logger.py Outdated
akapur99 and others added 4 commits August 7, 2026 21:09
- Forward the original request's non-default params (temperature, tools,
  response_format, etc.) to the shadow router call. Previously only model and
  messages were sent, so a request with tools or a non-zero temperature was
  judged against a shadow response generated under totally different sampling
  settings -- an unfair, biased comparison. stream and metadata are still
  stripped: the shadow call needs the full text back and must not leak the
  caller's own metadata.

- Fix unbounded background task backlog: asyncio.create_task() fired
  unconditionally and only the task body waited on a semaphore, so a traffic
  spike queued unlimited tasks (each holding a copy of messages/response)
  before any of them ran. Now the in-flight count is checked and incremented
  before scheduling; over capacity, the sample is dropped instead of queued.

- Fix stopped jobs being silently reactivated: the verdict-write counter
  update unconditionally set status='running', so a pipeline that started
  before stop_shadow_eval_job() marked the job 'completed' could overwrite
  that back to 'running' after the fact. Now it's a conditional update_many
  scoped to status in (pending, running), so a completed job can never
  transition back.

- Strip comments/docstrings added during the previous fix pass per
  CLAUDE.md's no-new-comments rule (flagged by review bot).

Tests: 5 new regression tests (param forwarding, stream/metadata stripping,
backlog cap drops samples under saturation, backlog cap schedules+decrements
under capacity, stop-race status guard). 31/31 passing.

Co-Authored-By: Claude <noreply@anthropic.com>
Resolves two real conflicts (the PR's actual base branch is
litellm_internal_staging, not main):

- litellm/types/management_endpoints/auto_router_endpoints.py: kept both
  the Mapping and Literal imports, both used by pre-existing types.
- tests/e2e/proxy_client.py: kept upstream's more detailed create_model()
  docstring covering multi-replica propagation.

Everything else auto-merged cleanly. Regenerated the OpenAPI schema
(schema.d.ts) to pick up upstream's tier_turns addition to the auto-router
benchmarks response.

Co-Authored-By: Claude <noreply@anthropic.com>
Base branch moved again while resolving the prior merge. Same conflict shape
in litellm/types/management_endpoints/auto_router_endpoints.py: kept both
the Mapping and Literal imports. Regenerated schema.d.ts to pick up
upstream's ModelInfo pricing field changes.

Co-Authored-By: Claude <noreply@anthropic.com>
@codecov

codecov Bot commented Aug 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.00000% with 42 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
litellm/integrations/shadow_eval_logger.py 91.22% 28 Missing ⚠️
...roxy/management_endpoints/auto_router_endpoints.py 92.30% 14 Missing ⚠️

📢 Thoughts on this report? Let us know!

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Live-proxy test: backend flow works. UI unreachable (accessToken undefined; section hidden with zero sessions), judge default lacks provider prefix, cost_actual stays null, complexity routers rejected.

crash
zero sessions
working with local fix
no verdicts

@codspeed-hq

codspeed-hq Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 31 untouched benchmarks


Comparing shadow-eval-pre-adoption (e51008f) with litellm_internal_staging (b0fac57)

Open in CodSpeed

akapur99 and others added 2 commits August 8, 2026 09:30
- Remove unused imports (datetime, timezone, ModelResponse) flagged by ruff.
- Update test_cost_tracking_adds_two_callbacks_when_prisma_set to expect 2
  callbacks on litellm.callbacks (not 1): ShadowEvalLogger now registers
  alongside _ProxyDBLogger in cost_tracking(). Test name already said 'two',
  now it actually tests for the correct count.
- Format ShadowEvalSection.tsx/.test.tsx per prettier.

Tests: 94/94 passing (lifecycle, shadow-eval, auto-router endpoints).
Lint: ruff + prettier all clean.

Co-Authored-By: Claude <noreply@anthropic.com>
The column was declared Float? with no default, so every row started NULL.
The verdict writer increments it, and NULL + x is NULL in Postgres, meaning
judge spend never accumulated and the UI always showed no spend.

Makes the column non-null with a default of 0 across all three schema copies
and the migration, and tightens the response model to a plain float.
@tin-berri

Copy link
Copy Markdown
Contributor Author

All four are fixed now. The first three landed before your run; cost_actual was real, the column now defaults to 0 so increments accumulate.

Comment thread litellm/integrations/shadow_eval_logger.py
akapur99 and others added 4 commits August 8, 2026 10:18
The lint job's strict-rule budget flagged 17 new violations. Rather than
raise the ceiling, this types the code properly:

- validate the judge verdict into a PairwiseVerdict pydantic model at the
  parse boundary instead of dict[str, Any] + cast, which also removes the
  defensive float()/str() coercion downstream
- replace the untyped job dict with a frozen ActiveShadowEvalJob dataclass
- validate the prisma job row into _ShadowEvalJobRow, replacing 11 no-op
  '# type: ignore[attr-defined]' comments
- annotate the success hook and drop Any from the remaining signatures
- mark the genuine third-party dict shapes (prisma filters/payloads, SDK
  message lists) with '# mutable-ok' reasons per the existing convention
Three fixes from the live end-to-end run:

- The judge ran with max_tokens=200, which truncated roughly 12% of
  verdicts mid-JSON so they were lost to failed_count. Raise it to a
  named JUDGE_MAX_OUTPUT_TOKENS=500 and price the upfront cost estimate
  off the same constant, so the estimate can't silently drift from what
  the judge is actually allowed to emit.

- The UI only ever rendered the newest job, so starting a new eval hid
  the results of a populated older one. Prior jobs are now listed in a
  collapsible 'Previous evaluations' card, each expandable to its own
  per-tier results.

- Move the shadow eval section above the benchmarks body: pre-adoption
  keys have no router sessions, so it was buried under an empty state.
  It stays outside BenchmarksBody so it survives that early return.

Co-Authored-By: Claude <noreply@anthropic.com>
Shadow and judge calls were fired with no caller identity on their metadata.
The proxy's cost callback requires user_api_key/_team_id/... to log spend and
apply budget checks, and silently drops the entry without them — so an
admin-enabled eval billed real provider spend that landed on no key, no team,
and no budget counter, invisible to every limit the shadowed key is normally
subject to.

Extract the identity-forwarding rules the auto-router classifier already
implements into a shared litellm/litellm_core_utils/internal_call_metadata.py:
forward the caller's identity subset, strip the parent's budget reservation
(top-level and the copy nested in user_api_key_auth) so a sub-call can't
finalize a reservation that belongs to the parent, and stamp the sub-call's
origin. The classifier now uses this module instead of its own copy.

Wire both shadow eval call sites (_call_router_shadow, _call_judge) through
it, and add a per-job spend cap (job cost_actual >= max(3x the quoted
estimate, $0.50)) so a bad estimate or a traffic spike can't turn a quoted
eval into a runaway bill; a capped job self-completes and is evicted from
cache so it can't be resurrected by an in-flight request.

Also surface api_key_id/team_id on GetShadowEvalJobResponse so an admin
running several jobs can tell which key's traffic a given win rate belongs
to, and regenerate the dashboard's OpenAPI types for the new fields.

Co-Authored-By: Claude <noreply@anthropic.com>
Comment thread litellm/integrations/shadow_eval_logger.py
akapur99 and others added 3 commits August 8, 2026 12:36
A shadow eval samples ongoing traffic, so a job without an end date keeps
billing judge calls until someone remembers to stop it — and the upfront
estimate silently priced exactly one week regardless. Jobs now take a
duration_days (1-30, default 7): the start endpoint stamps ends_at, the
estimate scales trailing volume to the requested window, and the logger
completes a job past its window through the same guarded update + cache
eviction path as the spend cap (generalized into _finalize_job). The
existing shadow eval migration is amended in place since it has not
shipped anywhere yet.

The start form no longer asks anyone to paste a key hash: the key is a
type-to-search combobox backed by /key/list alias substring search that
submits the token, the auto-router is a filter-as-you-type combobox fed
by the configured auto-router deployments, and duration is a select.
Active job cards show when the job will end.

The judge model field is now labelled as such, with guidance: judging
two answers blind needs solid comprehension and reliable JSON, not
frontier reasoning — a mid-tier model (Claude Sonnet / GPT-4o class) is
recommended, nano/mini-class judges give unreliable verdicts, and
frontier reasoning models add cost without changing outcomes. Same
guidance mirrored into the API field description.

Co-Authored-By: Claude <noreply@anthropic.com>
…budget

Adds a "Shadow eval" button next to the Auto-router usage heading that
smooth-scrolls to the shadow eval section, so it's reachable without
scrolling past the benchmarks body first.

Also fixes a PR review comment (veria-ai): shadow and judge calls ran
outside the normal auth path, so they never went through
reserve_budget_for_request and could push an already-exhausted key or
team further over budget before their own spend was even recorded.
_key_or_team_is_over_budget reads the same cross-pod spend counters
that path reserves against (via the existing get_current_spend) and
skips the shadow/judge pair outright when the shadowed key or its team
is already at or over budget. This is a read-time check, not a
reservation — appropriate for a best-effort background measurement
task, not a billed user request — so it narrows the window rather than
closing it against concurrent bursts, which the response comment
explains.

Co-Authored-By: Claude <noreply@anthropic.com>
akapur99 and others added 7 commits August 10, 2026 19:27
…hadow-eval-pre-adoption

# Conflicts:
#	ui/litellm-dashboard/tsconfig.tsbuildinfo
…task

greptile: _key_or_team_is_over_budget awaited get_current_spend inline in
async_log_success_event, and that read can fall back to an authoritative
DB query — the same request-path DB touch the job-snapshot rework just
removed. The gate now runs first inside _run_shadow_eval, where a
detached task absorbs the latency and the paid shadow/judge calls still
never fire for an over-budget key. The callback is back to zero awaits
beyond its own bookkeeping; a regression test asserts get_current_spend
is never awaited before the callback returns.

Co-Authored-By: Claude <noreply@anthropic.com>
Upstream 255d651 gated the view's tabs behind useCan, which chains
useAuthorized -> useIsOrgAdmin -> useOrganizations -> useQuery, so the
two view test files now die with 'No QueryClient set' before asserting
anything. Mock useIsOrgAdmin (and useAuthorized in the activity test)
at the boundary the component actually reads; the tests assert tab
behavior, not org-admin resolution. These fail on staging's own full
suite runs too — this fixes the slice that our PR's related-tests job
runs.

Co-Authored-By: Claude <noreply@anthropic.com>
…36321)

A shadowed key's real traffic can be a mix of models, and per-tier win
rates blend those incumbents together: if the router beats gpt-4o but
loses to a fine-tune within the same tier, the tier rate hides both
facts. The verdict rollup now groups by (tier, real_model) and the
results carry a second stratification, by_current_model, listing win
rates against each model the key actually uses. The UI renders it as a
second table, only when the traffic really was mixed — a single
incumbent would just repeat the tier table's totals. Tier confidence is
now turn-weighted across the merged rows rather than averaged per row.

Requests already served by the router being shadowed are now skipped
before sampling: duplicating them compares the router to itself —
guaranteed ties, judge spend for zero information. Traffic routed by a
*different* auto-router still samples, which is a meaningful
router-vs-router comparison. Skipped requests still count toward the
job's request_count so the seen/judged ratio stays honest.

Co-authored-by: Abhimanyu Kapur <38531241+akapur99@users.noreply.github.com>
Co-authored-by: Claude <noreply@anthropic.com>
@tin-berri

Copy link
Copy Markdown
Contributor Author

@greptileai can you reply here with a score and why that score?

@tin-berri

Copy link
Copy Markdown
Contributor Author

bugbot run

@greptile-apps

greptile-apps Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Score: 3.5 / 5


What works well

Architecture is sound. The core design decisions hold up under scrutiny:

  • The recursion guard (INTERNAL_CALL_ORIGIN_METADATA_KEY) reliably breaks the shadow→judge→shadow cycle
  • Lifecycle management (counter flushes, snapshot refreshes, job finalization) runs entirely off the request path — the success hook is one dict lookup
  • Deterministic hash-based sampling gives cross-pod consistency without coordination
  • The spend cap (1.5× estimate, $1 floor) and in-flight concurrency cap (16) provide two independent runaway-spend mitigations
  • Budget soft-check before shadow dispatch (_key_or_team_is_over_budget) closes the common case without needing a reservation
  • sanitized_forwardable_call_metadata correctly strips user_api_key_budget_reservation to prevent the double-finalization bug on parent spend
  • Internal call exclusion from api_requests / autorouter_savings_spend / LiteLLM_AutoRouterSession is applied consistently across all three write paths

Real concerns

1. Logger registration is not idempotent (proxy_server.py)

_register_shadow_eval_logger() unconditionally creates a new ShadowEvalLogger() and registers it every time cost_tracking() is called. If cost_tracking() is called more than once (config reload, test setUp, etc.), the second instance:

  • handles async_log_success_event independently with a stale/empty _jobs_by_key
  • has its own _pending_seen that no lifecycle loop ever drains (unbounded growth)
  • has its own _inflight_shadow_tasks counter, making the pod-level cap per-instance rather than global

Fix: guard with an isinstance check before registering, similar to what _start_shadow_eval_lifecycle_loop already does:

def _register_shadow_eval_logger() -> None:
    from litellm.integrations.shadow_eval_logger import ShadowEvalLogger
    if any(isinstance(cb, ShadowEvalLogger) for cb in litellm.callbacks):
        return
    litellm.logging_callback_manager.add_litellm_callback(ShadowEvalLogger())

2. _start_shadow_eval_lifecycle_loop() and _register_shadow_eval_logger() have an implicit ordering dependency

_start_shadow_eval_lifecycle_loop is called at startup and searches litellm.callbacks for an already-registered ShadowEvalLogger. It only works if cost_tracking() (which calls _register_shadow_eval_logger) has already run at that point. Nothing in the code enforces this ordering — a future refactor that reorders startup steps silently breaks the lifecycle loop: no error, jobs just never finalize and counters never flush. The lifecycle loop start should live inside _register_shadow_eval_logger itself, called once after registration, rather than being a separate startup step that depends on registration having happened first.

3. Unused schema fields

LiteLLM_ShadowEvalVerdict.shadow_request_id and real_response_tokens are declared in the schema but never written in _run_shadow_eval's prisma.db.litellm_shadowevalverdict.create(data={...}). Dead columns in the migration from day one — either populate them or drop them before the schema is committed.

4. PostgreSQL-specific raw SQL without annotation

::bigint, ::int, ::float casts in _ESTIMATE_VOLUME_SQL and _VERDICT_AGG_SQL are PostgreSQL-only syntax. If litellm's DB support is PostgreSQL-only this is fine, but it should be noted since query_raw bypasses Prisma's portability layer.


Summary

The feature is thoughtfully designed and the critical-path safety properties (no latency added, no recursion, no unbounded backlog) are correct. The 3.5 reflects that the registration/lifecycle wiring has a real correctness bug (non-idempotent registration) and a fragile startup dependency that will silently break if startup order ever changes — both fixable in a few lines before merge.

@cursor cursor 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.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit ec32f70. Configure here.

@tin-berri

Copy link
Copy Markdown
Contributor Author

Addressed the review in 59ae51c. Finding 2: registration now owns the lifecycle loop start and the separate startup step is deleted, so no ordering exists to break. Finding 3: shadow_request_id and real_response_tokens are now written on every verdict. Finding 1 does not reproduce: the callback manager dedups CustomLogger instances by class (_add_custom_logger_to_list), so a second cost_tracking() discards the new instance; pinned by test_cost_tracking_twice_registers_one_shadow_eval_logger rather than duplicating that dedup at the call site. Finding 4: the proxy schema is PostgreSQL-only and AUTOROUTER_BENCHMARKS_SQL in the same file already uses the same casts, so the raw SQL matches repo practice

@tin-berri

Copy link
Copy Markdown
Contributor Author

@greptileai can you reply here with a score and why that score?

@tin-berri

Copy link
Copy Markdown
Contributor Author

bugbot run

@cursor cursor 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.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 59ae51c. Configure here.

@greptile-apps

greptile-apps Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Based on a thorough review of the diff, here's my assessment:


Score: 4 / 5

This is a well-engineered, substantial feature. Here's what earns that score:

What's done well:

  • Zero-latency design is correct — asyncio.create_task in the success hook, work done in a detached background task
  • Deterministic, hash-based sampling across pods without coordination is the right approach
  • _MAX_CONCURRENT_SHADOW_TASKS = 16 shed-not-queue is correct; it prevents a backlog that never clears under a traffic spike
  • Recursion guard via internal_call_origin is solid and covers all sub-call variants (shadow, judge, classifier)
  • Spend attribution (shadow/judge spend counts toward key budget but not api_requests / autorouter_savings_spend) is correct and consistent across db_spend_update_writer.py and autorouter_session_rollup.py
  • Partial unique index in SQL (correctly calling out that schema.prisma can't express it) is the right place for the one-active-job-per-key guarantee
  • Status-guarded update_many for all counter/finalize writes prevents two pods from resurrecting a stopped job
  • Test coverage is strong — 76 unit tests including mutation-checked edge cases

Concerns that keep this from 5/5:

  1. _run_shadow_eval incurs shadow + judge spend even when prisma is None (checked only after the pipeline runs, for verdict write). If Prisma is unavailable mid-request, both API calls happen but no verdict is stored — spend with no record.
prisma: Final = self._prisma_provider()
# ... budget check uses prisma via get_current_spend ...
shadow: Final = await self._call_router_shadow(...)  # spend incurred
verdict: Final = await self._call_judge(...)          # spend incurred
if prisma is None:  # ← checked too late
    return

The prisma is None guard should move above the shadow/judge dispatch.

  1. list_shadow_eval_jobs has a hard take=50 with no pagination. Operators with many historical jobs will silently get a truncated list.

  2. The spend cap can overshoot by a full lifecycle tick (up to 10s of concurrent judge calls), acknowledged in comments. For a high-volume key with an underestimated cost, this could be material. Worth flagging for awareness even if the tradeoff is intentional.

  3. _register_shadow_eval_logger relies on litellm.callbacks containing the registered instance to call start_lifecycle_loop(). If the callback manager routes async callbacks to a different internal list and litellm.callbacks doesn't include the instance, the lifecycle loop never starts (jobs never finalize, counters never flush) with no error surfaced.

@tin-berri

Copy link
Copy Markdown
Contributor Author

Addressed in e51008f. Concern 1: the prisma gate now sits above the shadow and judge dispatch, so no provider spend happens without a place to record the verdict (pinned by a mutation-checked test). Concern 2: the list endpoint takes a bounded limit query param (default 50, max 200). Concern 3 stands as designed: overshoot is bounded by one 10s tick times the 16-task cap, a few cents at judge prices, and the tick already sets stop propagation. Concern 4 does not reproduce: add_litellm_callback appends to litellm.callbacks via _safe_add_callback_to_list, and test_registration_starts_the_lifecycle_loop_on_the_registered_instance fails if that routing ever changes

@tin-berri

Copy link
Copy Markdown
Contributor Author

@greptileai can you reply here with a score and why that score?

@tin-berri

Copy link
Copy Markdown
Contributor Author

bugbot run

@cursor cursor 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.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit e51008f. Configure here.

@greptile-apps

greptile-apps Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Score: 3/5

The core architecture shows careful engineering — zero-latency detached tasks, hash-based deterministic sampling, recursion guard via internal_call_origin, DB-level one-active-job-per-key uniqueness + race-safe 409, spend-cap with floor, and the clean split between forwarded_internal_call_metadata (in-request) and sanitized_forwardable_call_metadata (detached) to prevent double-finalization of budget reservations. The exclusion of internal calls from daily api_requests / autorouter savings rollups is correct and the reasoning is well-documented. These are all genuinely good decisions.

What pulls it down:

1. Judge prompt can reach ~48k chars (correctness/cost bug)
_MAX_JUDGE_CHARS = 16_000 is applied separately to conversation, response_a, and response_b inside _call_judge. The resulting user_prompt is conversation[-16000] + "\n\nResponse A:\n" + response_a[:16000] + "\n\nResponse B:\n" + response_b[:16000], so the prompt alone can approach 50k characters before the system prompt is added. This will overflow the context window of many judge models and produce expensive, wasted calls that land in failed_count with no clear diagnosis.

2. Lifecycle loop start path is fragile
_register_shadow_eval_logger adds a ShadowEvalLogger() via add_litellm_callback, then scans litellm.callbacks to find the registered instance to call start_lifecycle_loop(). If the callback manager stores the canonical instance somewhere other than litellm.callbacks (e.g. it deduplicates before appending), the loop is never started, jobs never finalize, and counters never flush — silently. This warrants a direct reference or a post-registration check.

3. Migration amended in-place
The PR description says "a DB that already applied it needs a re-resolve or db push." In-place amendment of an already-applied migration corrupts Prisma's migration history checksum. This is a production hazard; a new additive migration is the correct path.

4. Staging pass explicitly not done
The PR description states this is required before merge, which means the review is premature on that axis.

5. Judge model validation is shallow
litellm.get_llm_provider(model=judge_model) only validates the provider prefix is known — it does not verify the model exists or credentials are present. A job started with anthropic/nonexistent-model will pass /start and silently fill failed_count with "judge call failed" errors. A resolve-time credential check or at least a warning in the response would close this gap.

The architecture is solid enough that fixing (1) and (3) + completing staging would make this mergeable. As-is it has a real correctness bug in judge prompt sizing and an operational risk in the migration strategy.

… 0.4.84 -> 0.4.84.post1

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.

4 participants