fix(logging): dedupe async CustomLogger hooks with an explicit sweep flag - #33577
Conversation
…m_logger_hooks sweep flag The async success/failure paths run the sync handler only to service legacy sync-only callbacks (langfuse, s3, sync-only CustomLoggers), so thread run_custom_logger_hooks=False from the two async sync-sweep sites (dispatch_success_handlers, handle_sync_success_callbacks_for_async_calls) and the async wrapper failure path. That suppresses the sync CustomLogger hook and the standard/openmeter emitters the async handler already ran, closing the anthropic_messages double-log without depending on the a* request classifier Replaces the is_async_entrypoint wrapper stamp, which required mutable per-request state and broke duck-typed logging stubs in CI Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
c1a0c96
into
litellm_lit4447_dd_dedupe
Greptile SummaryThis PR is an alternative to #33481 for fixing double-logging (LIT-4447 / LIT-4475): instead of stamping
Confidence Score: 3/5The success-path fix is clean and well-tested, but The callable-callback branch in
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/litellm_logging.py | Adds run_custom_logger_hooks parameter to success_handler and failure_handler, removes the is_async_entrypoint stamp, and makes _is_sync_litellm_request a static method. The success path correctly gates both CustomLogger and callable callbacks on is_sync_request, but failure_handler's callable-callback branch at line 2879 is not gated on is_sync_request, creating an asymmetry with success_handler and leaving callable callbacks potentially double-firing on the async failure path. |
| litellm/utils.py | Removes the is_async_entrypoint stamp assignments from the sync and async @client wrappers, and passes run_custom_logger_hooks=False to failure_handler in the async exception path. Changes are minimal and targeted. |
| tests/test_litellm/litellm_core_utils/test_litellm_logging.py | Test suite updated to remove is_async_entrypoint stamps and pass run_custom_logger_hooks=False at call sites. The nested async-to-sync test weakens its assertion from channel-specific (events == ["async"]) to count-only (len(events) == 1), intentional per PR description but reduces regression protection for the delivery channel. |
Comments Outside Diff (1)
-
litellm/litellm_core_utils/litellm_logging.py, line 2879-2890 (link)Callable callbacks not gated on
is_sync_requestinfailure_handlerUnlike
success_handler(line 2370:callable(callback) is True and is_sync_request and customLogger is not None), the callable-callback branch infailure_handlerruns unconditionally regardless ofis_sync_request. Withrun_custom_logger_hooks=Falsethe handler intends to suppress the callable callbacks (the PR description says "the handler skips the CustomLogger, openmeter, and standard-payload work the async handler already performed"), but the missingis_sync_requestguard means callable callbacks still fire here. If a callable is registered in bothlitellm.failure_callbackandlitellm._async_failure_callback, it will double-fire on every async failure, the exact problem this PR is trying to close forsuccess_handler.
Reviews (1): Last reviewed commit: "fix(logging): dedupe async CustomLogger ..." | Re-trigger Greptile
| @@ -1174,12 +1161,7 @@ async def fake_outer_async(model: str, messages=None, **kwargs): | |||
| litellm.success_callback = original_success | |||
There was a problem hiding this comment.
Test assertion weakened — delivery channel no longer verified
The previous assertion was assert events == ["async"], which confirmed the delivery arrived via the async hook. This is replaced by assert len(events) == 1, which only confirms count. The PR description acknowledges the behavioral change (nested async-to-sync delivers via sync hook now), so the new assertion is technically correct for the new behavior. However, the change means that a future regression where the delivery silently flips back to the async hook (or swaps channel unexpectedly) would not be caught. Consider asserting the specific channel, e.g. assert events == ["sync"], so the delivery path is locked.
Rule Used: What: Flag any modifications to existing tests and... (source)
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!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Relevant issues
Linear ticket
Resolves LIT-4447
Resolves LIT-4475
Stacking note
This PR stacks on #33481 (base branch
litellm_lit4447_dd_dedupe). It is an alternative implementation of the same fix that removes theis_async_entrypointwrapper stamp in favor of an explicit call-site flag. If this approach is preferred it replaces #33481; the diff shown here is exactly the delta from the stamp approach to the flag approach.Pre-Submission checklist
Screenshots / Proof of Fix
Live-proxy capture is pending on this commit. The observable DataDog behavior is identical to #33481 (exactly one intake event per async call), since both suppress the second CustomLogger delivery. See the internal-hook caveat under Changes for the one behavioral difference from the stamp approach.
Type
🐛 Bug Fix
Changes
Async requests fire
async_log_success_eventand then run the syncsuccess_handleronly to service legacy sync-only callbacks (langfuse,s3, and CustomLoggers whose only delivery path is the sync hook). On entrypoints that plant noa*flag inlitellm_params(anthropic_messages,agenerate_content),_is_sync_litellm_requestmisclassified the request as sync, so that compatibility sweep also firedCustomLogger.log_success_eventon top of the async hook, shipping two DataDog events with onelitellm_call_id.Instead of stamping request async-ness at the
@clientwrapper (#33481) and re-deriving the decision from classification, this makes the decision explicit at the two async sweep sites, extending the existingprefer_async_handlerscall-site-override convention one level down onto the handlers themselves:success_handlerandfailure_handlertakerun_custom_logger_hooks: Optional[bool] = None. The two async sweep sites (the post-asyncexecutor.submitindispatch_success_handlersandhandle_sync_success_callbacks_for_async_calls) plus the async wrapper's failure path passrun_custom_logger_hooks=False.False, the handler skips the CustomLogger, openmeter, and standard-payload work the async handler already performed, while legacy sync-only string callbacks still run._custom_logger_has_only_sync_success_hooks/_custom_logger_has_only_sync_failure_hook, and_should_run_sync_callbacks_for_async_callsstill opens the sweep for them._is_sync_litellm_requestreverts to the base classifier; theis_async_entrypointinstance stamp and its duck-typed-stubgetattrguard are removed.This closes the double-log for every async path that reaches the sweep, including paths that build a
Loggingoutside@client(proxy passthrough, realtime, MCP), without depending on the fragilea*classifier and without adding mutable per-request state.Behavioral caveat versus #33481: for the nested async-to-sync case (
agenerate_contentdelegating to the syncgenerate_contenton a shared logging object), a both-hook CustomLogger is now delivered once via the sync hook rather than the async hook. It is still exactly one delivery and identical from the DataDog intake's perspective; the inner sync@clientwrapper cannot know it is nested without the shared stamp this PR removes. Direct async entrypoints such as/v1/messagesare unaffected and deliver via the async hook exactly once.Tests updated to the flag mechanism, covering: flagless
anthropic_messagesthrough the real@clientwrapper firing the async hook once; both-hook CustomLogger not double-firing under the open sweep; sync-only CustomLogger still delivered once; nested async-to-sync delivered once; and the dispatch gate opening for sync-only loggers.Link to Devin session: https://app.devin.ai/sessions/f79a6ba850c74a1c8d2bfe3f9aa3665c
Requested by: @yucheng-berri