Cherry-pick #29311 and #29343 onto patch/v1.84.3 - #29351
Conversation
…9343) * refactor(proxy/auth): normalize Bearer prefix in safe-hash helper UserAPIKeyAuth._safe_hash_litellm_api_key now strips a leading "Bearer "/"bearer " prefix before its existing sk-/JWT classification, so the helper produces the same hashed output regardless of whether the caller stripped the Authorization header prefix or passed the header value through unchanged. * refactor(proxy/auth): make Bearer-prefix strip case-insensitive Per RFC 7235 the HTTP authorization scheme token is case-insensitive. Replace the two-prefix loop with a single case-insensitive check so the helper normalizes "Bearer ", "bearer ", "BEARER ", and any mixed-case variant before classifying the remainder as sk- or JWT. The contract test gains coverage of "BEARER " and "BeArEr ". * test(mcp): align auth-handler test expectations with safe-hash helper The two MCP auth tests asserted that UserAPIKeyAuth(api_key="Bearer ...") retained the raw header bytes on the api_key field. _safe_hash_litellm_api_key now normalizes that input — stripping the Bearer prefix and hashing the resulting sk- key — so the expectations move to the normalized form: the bare token in the parametrize case, and hash_token("sk-...") in the backward-compat assertion. This matches what the real auth flow produces (the builder strips Bearer and the DB stores the hashed token), so the mocks now line up with production rather than with the un-normalized validator output.
|
Wrong base/head. Reopening with base=patch/v1.84.3 and a dedicated feature branch. |
Greptile SummaryThis PR cherry-picks two fixes onto
Confidence Score: 4/5Safe to merge; the logging refactor is well-scoped and all production call sites have been migrated to The litellm/litellm_core_utils/litellm_logging.py — the new
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/litellm_logging.py | Adds dispatch_success_handlers, _is_assembled_stream_success, and _is_sync_litellm_request; refactors dedup guard in async_success_handler to bypass should_run_logging for assembled streams, relying on a new has_dispatched_final_stream_success flag in the new dispatcher instead. |
| litellm/litellm_core_utils/streaming_handler.py | Per-chunk sync logging gated behind _is_sync_litellm_request; final assembled-stream dispatch migrated to dispatch_success_handlers(prefer_async_handlers=True), removing the parallel executor.submit call that caused duplicate traces. |
| litellm/proxy/_types.py | _safe_hash_litellm_api_key now strips a case-insensitive Bearer prefix before sk-/JWT classification, ensuring Bearer-prefixed and bare keys produce the same hash. |
| litellm/proxy/common_request_processing.py | Orphaned and deferred streaming logging paths both migrated from async_success_handler + executor.submit to dispatch_success_handlers(prefer_async_handlers=True), eliminating duplicate trace emission. |
| litellm/proxy/pass_through_endpoints/streaming_handler.py | Streaming pass-through logging migrated to dispatch_success_handlers(prefer_async_handlers=True); removes direct executor.submit call and the executor import. |
| litellm/proxy/pass_through_endpoints/success_handler.py | Non-streaming pass-through _handle_logging migrated to dispatch_success_handlers(prefer_async_handlers=True); removes the thread_pool_executor.submit block and its import. |
| tests/pass_through_unit_tests/test_unit_test_streaming.py | Adds two new async unit tests verifying the async handler fires (and sync does not) for SDK pass-through streaming and non-streaming logging paths. |
| tests/proxy_unit_tests/test_proxy_reject_logging.py | Adds _register_proxy_test_logger to register the test logger on all five global callback lists; changes the embedding model from text-embedding-ada-002 to fake-model to match the test router configuration. |
| tests/test_litellm/litellm_core_utils/test_litellm_logging.py | Adds four focused unit tests for dispatch_success_handlers: dedup on double final-stream dispatch, sync-path dedup, prefer_async_handlers legacy executor path, and pass-through async routing. |
| tests/test_litellm/litellm_core_utils/test_streaming_handler.py | Minor formatting cleanups (assertion style, removed stray print); no logic changes to test assertions. |
| tests/test_litellm/proxy/_experimental/mcp_server/auth/test_user_api_key_auth_mcp.py | Updates two assertions to reflect the Bearer-stripping fix: extracted token is now "test-auth-token" and stored api_key is hash_token("sk-litellm-valid-key"). |
| tests/test_litellm/proxy/guardrails/test_deferred_guardrail_logging.py | Introduces _attach_mock_success_dispatch helper and replaces all direct mock_logging_obj.async_success_handler assignments with it, aligning mocks to the new dispatch_success_handlers production entrypoint. |
| tests/test_litellm/proxy/test_proxy_types.py | Adds test_user_api_key_auth_hashes_authorization_header_form_of_key verifying all Bearer-prefix case variants produce the same api_key and token as the bare key. |
Reviews (1): Last reviewed commit: "refactor(proxy/auth): normalize Bearer p..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if not self._is_assembled_stream_success( | ||
| result | ||
| ) and not self.should_run_logging( | ||
| event_type="async_success" | ||
| ): # prevent double logging | ||
| ): # prevent double logging (non-streaming) | ||
| return |
There was a problem hiding this comment.
async_success_handler for assembled streams
should_run_logging("async_success") previously prevented any re-entrant call to async_success_handler from running (it sets and checks has_logged_async_success). The new logic bypasses that guard whenever _is_assembled_stream_success returns True, so the only remaining dedup for final-stream results is the has_dispatched_final_stream_success flag inside dispatch_success_handlers. Direct callers of async_success_handler (e.g. custom integrations using the object's public API, or future code paths added on the non-dispatch_success_handlers branch) that pass an assembled ModelResponse on a streaming logging object won't be deduplicated. All current production call sites have been migrated, but the behavioral contract change is worth documenting so it isn't re-introduced.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
2 participants
Cherry-picks two commits onto
patch/v1.84.3(base:patch/v1.84.1).Commits
[internal copy of fix: duplicate claude code traces #29089] fix: duplicate claude code traces ([internal copy of #29089] fix: duplicate claude code traces #29311) (
628a65c)dispatch_success_handlers(prefer_async_handlers=True)instead ofasync_success_handler+ a separateexecutor.submitsync call, eliminating duplicate traces.litellm/proxy/pass_through_endpoints/streaming_handler.py: kept this branch's per-endpoint result-building block (ANTHROPIC/VERTEX_AI/OPENAI +Nonefallback) and adopted the newdispatch_success_handlerscall.refactor(proxy/auth): normalize Bearer prefix in safe-hash helper (refactor(proxy/auth): normalize Bearer prefix in safe-hash helper #29343) (
23d9f1f)UserAPIKeyAuth._safe_hash_litellm_api_keystrips a case-insensitive leadingBearerprefix before its sk-/JWT classification, so the hashed output is identical whether or not the caller stripped the Authorization header prefix.Test plan
make test-unit