fix(logging): classify allm_passthrough_route as async to prevent duplicate success callbacks - #32265
Conversation
|
yucheng seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Greptile SummaryThis PR fixes duplicate success callbacks fired for async LLM passthrough requests by ensuring
Confidence Score: 5/5Safe to merge — the change is narrowly scoped to how the async passthrough flag is propagated and classified, with no side-effects on other call paths. All three layers of the fix (propagation via No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/get_litellm_params.py | Adds allm_passthrough_route as an explicit named parameter and propagates it into the returned litellm_params dict, following the same pattern as acompletion and aembedding. Change is minimal and correct. |
| litellm/litellm_core_utils/litellm_logging.py | Adds allm_passthrough_route check to _is_sync_litellm_request using the existing CallTypes enum (value confirmed in litellm/types/utils.py), consistent with all other async-flag checks in the method. |
| litellm/passthrough/main.py | Removes the now-redundant explicit allm_passthrough_route parameter and reads the flag from **kwargs instead; get_litellm_params(**kwargs) then captures and propagates it. The _is_async derivation and downstream update_environment_variables call are unaffected. |
| tests/test_litellm/litellm_core_utils/test_litellm_logging.py | Three test assertions added/extended: test_is_sync_litellm_request (unit check), test_get_litellm_params_propagates_allm_passthrough_route (propagation check), and test_success_handler_skips_sync_callbacks_for_async_requests gains allm_passthrough_route parametrize case. All cover the regression path cleanly. |
Reviews (3): Last reviewed commit: "test(passthrough): assert allm_passthrou..." | Re-trigger Greptile
Greptile SummaryThis PR fixes duplicate success callbacks (e.g. duplicate LangSmith runs) when using the async Bedrock/LLM passthrough route. The
Confidence Score: 5/5The change is minimal and scoped entirely to the async-vs-sync classifier and its propagation path; no auth, DB, or routing logic is touched. Three files receive targeted, one-line-or-fewer additions that follow established patterns already in the codebase ( No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/get_litellm_params.py | Adds allm_passthrough_route as an explicit parameter and writes it into the returned litellm_params dict, matching the pattern used for acompletion and aembedding; no issues found. |
| litellm/litellm_core_utils/litellm_logging.py | Extends _is_sync_litellm_request to treat allm_passthrough_route=True as async, preventing the sync-callback branch from firing on async passthrough requests; follows the exact same pattern as the existing aembedding/atranscription checks. |
| litellm/passthrough/main.py | Forwards the allm_passthrough_route flag explicitly into the get_litellm_params(...) call so the flag survives into litellm_params; Python correctly de-dupes it since allm_passthrough_route is an explicit named param in both functions. |
| tests/test_litellm/litellm_core_utils/test_litellm_logging.py | Adds three targeted regression guards: one parametrized case in the existing skip-sync-callbacks test, one new unit assertion in test_is_sync_litellm_request, and one new end-to-end propagation test; all are mock-only and strengthen (not weaken) coverage. |
Reviews (1): Last reviewed commit: "fix(logging): classify allm_passthrough_..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…licate success callbacks Async passthrough requests set kwargs["allm_passthrough_route"]=True but that flag is never propagated into litellm_params, and _is_sync_litellm_request only checks acompletion/aresponses/aembedding/aimage_generation/atranscription. Every async passthrough is misclassified as sync, which trips the CustomLogger sync branch in success_handler and fires log_success_event in addition to the async worker's async_log_success_event, causing 2-3 duplicate LangSmith runs per Bedrock passthrough request Propagate allm_passthrough_route through get_litellm_params and teach the classifier about it. /chat/completions and other non-passthrough paths are untouched
b02d694 to
3bf4daf
Compare
…to-end Integration-level guard on top of the unit tests in test_litellm_logging.py: verifies that when kwargs["allm_passthrough_route"]=True enters llm_passthrough_route, the flag survives get_litellm_params(**kwargs), lands in the logging object's litellm_params, and _is_sync_litellm_request reads the request as async
|
Force-pushed once earlier to clean up the propagation path (dropped the redundant @greptileai please re-review the current HEAD |
|
bugbot run |
There was a problem hiding this comment.
✅ 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 335f99e. Configure here.
101f246
into
litellm_internal_staging
Relevant issues
Linear ticket
Resolves LIT-4192
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewScreenshots / Proof of Fix
Repro environment: postgres on
127.0.0.1:5432, litellm proxy on:4000, fake Bedrock on:5091, fake LangSmith capture on:5092, team-level LangSmith success callback registered viaPOST /team/{team_id}/callback, one plain sync callback inlitellm_settings.success_callback(the required trigger; see LIT-4192 "Trigger condition"). The Bedrock request is a real boto3converseagainst the proxy's/bedrock/...passthroughBefore (unfixed, on
origin/litellm_internal_staging@29035c4a99):Matches the fingerprint on the ticket: same
litellm_call_id, differentchatcmpl-response ids, count varying with the thread race,/chat/completionsunaffectedAfter (this branch):
Exactly one LangSmith run per Bedrock passthrough request across 7 consecutive requests,
/chat/completionsunchangedType
Bug Fix
Changes
Logging._is_sync_litellm_request()(litellm/litellm_core_utils/litellm_logging.py) decides sync vs async by inspectinglitellm_paramsforacompletion/aresponses/aembedding/aimage_generation/atranscription. The async passthrough entrypoint inlitellm/passthrough/main.pysetskwargs["allm_passthrough_route"] = True, but that flag was never propagated intolitellm_params, so_is_sync_litellm_requestreturnedTruefor every async passthrough request. That trips the CustomLogger branch inside the syncsuccess_handler(isinstance(callback, CustomLogger) and is_sync_request and call_type != CallTypes.pass_through.value), which callsLangsmithLogger.log_success_event()on top of the async worker'sasync_log_success_event(), producing duplicate LangSmith runsFix at the classifier level so the whole request path treats async passthrough consistently as async:
get_litellm_paramsnow acceptsallm_passthrough_routeand writes it into the returnedlitellm_paramsdict, matching the pattern used foracompletionandaembeddingllm_passthrough_routeforwards itsallm_passthrough_routeargument through theget_litellm_params(...)call so the flag reacheslitellm_params_is_sync_litellm_requestrecognizesallm_passthrough_route=Trueas async, using the same shape as the existing checksRegression tests in
tests/test_litellm/litellm_core_utils/test_litellm_logging.py:test_is_sync_litellm_requestgrows an assertion that{"allm_passthrough_route": True}classifies as asynctest_get_litellm_params_propagates_allm_passthrough_routelocks in thatget_litellm_params(allm_passthrough_route=True)produces params that_is_sync_litellm_requestreads as async, so the flag survives the propagation step end to endtest_success_handler_skips_sync_callbacks_for_async_requestsgainsallm_passthrough_routeas a parametrize case, so a future regression that reintroduces the sync-callback branch for async passthrough fails immediatelyReverted against
litellm_internal_staging, all three of these tests fail; with the fix all pass, and the fulltests/test_litellm/litellm_core_utils/test_litellm_logging.pysuite (109 tests) is greenRegression audit
Full sweep for regressions and backwards-incompatible changes; every claim below is grounded in code evidence
Callers of
_is_sync_litellm_request(four total)litellm_logging.py:1577(dispatch_success_handlers)litellm_logging.py:1971(success_handler)litellm_logging.py:2759(failure_handler)streaming_handler.py:1671(CustomStreamWrapper.run_success_logging_and_cache_storage)_sync_streaming/_async_streaminginpassthrough/main.py, notCustomStreamWrapperWriters of
kwargs["allm_passthrough_route"] = True: exactly one site,passthrough/main.py:63. No other code, tests, or docs set this key, so no non-passthrough request can accidentally flip to asyncCallers of
get_litellm_params: 48 acrosslitellm/andtests/. All use**kwargsor explicit keyword arguments; no positional callers, so addingallm_passthrough_route=Noneas a keyword-only parameter is safeDownstream consumers of the returned
litellm_paramsdict: none read"allm_passthrough_route"as a key outside_is_sync_litellm_request. Not serialized toSpendLogsPayload,StandardLoggingPayload, DB rows, or callback payloads. The newNonevalue on non-passthrough requests is invisible in every observable path@clientdecorator and signature introspection:litellm/utils.pyclient/function_setup/wrapper_asyncdo not useinspect.signature/__signature__. Droppingallm_passthrough_route: bool = Falsefromllm_passthrough_route's signature is safeCallers of
llm_passthrough_route(...): no external caller passesallm_passthrough_route=Trueas a keyword. The only writer is the internalallm_passthrough_routewhich setskwargs["allm_passthrough_route"] = True; the flag now flows through**kwargsnaturallyOrthogonality with the old
pass_through_endpointguard:CallTypes.pass_through.value == "pass_through_endpoint"andCallTypes.allm_passthrough_route.value == "allm_passthrough_route"are distinct strings for distinct code paths; no redundancy, no contradictionEmpirical test-diff (baseline
origin/litellm_internal_stagingvsHEAD)Suite:
tests/test_litellm/litellm_core_utils/+tests/test_litellm/passthrough/+tests/test_litellm/proxy/pass_through_endpoints/+tests/test_litellm/proxy/guardrails/test_deferred_guardrail_logging.py+tests/pass_through_unit_tests/test_unit_test_streaming.pyRegressions introduced by this PR: 0
Newly passing on this branch: 4 — exactly the LIT-4192 tests added or extended here (
test_is_sync_litellm_requestnew assertion,test_get_litellm_params_propagates_allm_passthrough_route,test_success_handler_skips_sync_callbacks_for_async_requests[allm_passthrough_route],test_llm_passthrough_route_propagates_allm_passthrough_route_to_logging_obj); onorigin/litellm_internal_stagingthey fail because the source fix is absent, which serves as the mutation check for this PRNote
Medium Risk
Changes logging callback routing for passthrough requests; scoped fix with tests but affects observability integrations on a hot path.
Overview
Fixes duplicate success callbacks (e.g. LangSmith runs) on async LLM passthrough by treating
allm_passthrough_routelike other async entrypoints.get_litellm_paramsnow accepts and storesallm_passthrough_routeinlitellm_params.Logging._is_sync_litellm_requesttreatsallm_passthrough_route=Trueas async so the syncsuccess_handlerdoes not also fireCustomLogger.log_success_eventon top of the async path.llm_passthrough_routeno longer takesallm_passthrough_routeas a dedicated parameter; it readskwargs["allm_passthrough_route"]for async detection while the flag still flows throughget_litellm_params(**kwargs)into the logging object.Regression tests cover param propagation, sync/async classification, and skipping sync callbacks for
allm_passthrough_route.Reviewed by Cursor Bugbot for commit 335f99e. Bugbot is set up for automated code reviews on this repo. Configure here.