Skip to content

fix(proxy): defer async logging until post-call guardrails complete - #23929

Closed
michelligabriele wants to merge 10 commits into
BerriAI:mainfrom
michelligabriele:fix/deferred-logging-post-call-guardrails
Closed

fix(proxy): defer async logging until post-call guardrails complete#23929
michelligabriele wants to merge 10 commits into
BerriAI:mainfrom
michelligabriele:fix/deferred-logging-post-call-guardrails

Conversation

@michelligabriele

@michelligabriele michelligabriele commented Mar 18, 2026

Copy link
Copy Markdown
Contributor

Relevant issues

Related to #23910 (request_data passthrough + full moderation response logging for post-call guardrails)

Pre-Submission checklist

  • I have Added testing in the tests/test_litellm/ directory, Adding at least 1 test is a hard requirement - see details
  • My PR passes all unit tests on make test-unit
  • My PR's scope is as isolated as possible, it only solves 1 specific problem
  • I have requested a Greptile review by commenting @greptileai and received a Confidence Score of at least 4/5 before requesting a maintainer review

Type

🐛 Bug Fix

Changes

Problem: guardrail_information is None in StandardLoggingPayload because logging fires before post-call guardrails have written to metadata.

  • Non-streaming: asyncio.create_task in wrapper_async fires before post_call_success_hook runs
  • Streaming: logging fires at stream exhaustion in CustomStreamWrapper.__anext__ without guardrail data from the assembled response

Non-streaming fix

When post-call guardrails are configured, defer the create_task call by storing a closure on logging_obj._enqueue_deferred_logging. The proxy calls this closure in a try/finally block after post_call_success_hook completes. Sync callbacks (handle_sync_success_callbacks_for_async_calls) fire immediately as before — only the async logging task (which builds StandardLoggingPayload) is deferred.

Files changed:

  • litellm/utils.py — conditional deferral: when _defer_async_logging flag is set, store closure instead of calling create_task
  • litellm/proxy/common_request_processing.py_has_post_call_guardrails() detection, flag setting, try/finally to call deferred closure after post_call_success_hook

Streaming fix

Store a _on_deferred_stream_complete closure on logging_obj that captures proxy context. CustomStreamWrapper.__anext__ checks for this closure at stream end and calls it instead of firing logging directly. The closure runs post_call_success_hook on the assembled response (so guardrails can inspect full content and write guardrail_information to metadata), then fires both logging handlers.

The closure is only attached for CustomStreamWrapper responses (checked via isinstance), not raw async generators from passthrough routes. For routes that consume the CSW via a generator (all current production streaming paths), the closure fires at stream end. If a CSW response falls through to the inline post_call_success_hook without being consumed by a generator (a hypothetical path not exercised by current callers), the closure is cleared and the inline hook runs as before — preserving blocking guardrail behavior with no double invocation.

Files changed:

  • litellm/proxy/common_request_processing.py — closure setup inside the streaming block (only for CSW + post-call guardrails), closure cleared on fallthrough to inline hook
  • litellm/litellm_core_utils/streaming_handler.pyCustomStreamWrapper.__anext__ checks for closure at stream end

Tests (tests/test_litellm/proxy/guardrails/test_deferred_guardrail_logging.py)

Non-streaming (7 tests):

  • _has_post_call_guardrails detection (post_call, pre_call, event_hook=None, string callbacks, empty, list variants)
  • Deferred flag stores and executes closure correctly
  • Regression: without flag, create_task fires normally
  • Exception path: deferred logging fires even if guardrail raises

Streaming (5 tests):

  • Closure defers logging at stream end (called, cleared, response stored)
  • Regression: without closure, logging fires immediately
  • Closure passes guardrail-modified response to logging handlers
  • Exception resilience: logging fires even when guardrail raises HTTPException, guardrail_blocked set in metadata
  • Transient errors do NOT set guardrail_blocked

Companion PR: #23910 fixes what gets written to metadata. This PR fixes when the logging snapshot is taken.

@vercel

vercel Bot commented Mar 18, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
litellm Ready Ready Preview, Comment Mar 19, 2026 2:10pm

Request Review

@codspeed-hq

codspeed-hq Bot commented Mar 18, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 16 untouched benchmarks


Comparing michelligabriele:fix/deferred-logging-post-call-guardrails (a564c1b) with main (e5baa22)

Open in CodSpeed

@greptile-apps

greptile-apps Bot commented Mar 18, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a race condition where guardrail_information was None in StandardLoggingPayload because async logging fired before post-call guardrails had written to metadata. It introduces two deferred-logging mechanisms: a _defer_async_logging flag for non-streaming requests (stores an _enqueue_deferred_logging closure on logging_obj, called in a try/finally after post_call_success_hook), and an _on_deferred_stream_complete closure for streaming CustomStreamWrapper responses (fired at stream exhaustion in __anext__). Test coverage is solid for the detection and deferral mechanics.

Key findings:

  • Behavioral change for non-guardrail callbacks on streaming (common_request_processing.py, line 1077): The deferred streaming closure calls post_call_success_hook, which dispatches to all registered async_post_call_success_hook implementations — not just guardrails. Before this PR, post_call_success_hook was never invoked for streaming requests that returned early (the standard path). Existing custom loggers with async_post_call_success_hook will now unexpectedly run on assembled streaming responses whenever any post-call guardrail is configured. Per the backwards-compatibility rule (b48b7341), this change should be gated or scoped to guardrail callbacks only.
  • Self-referential reference cycle on logging_obj (common_request_processing.py, line 1052): The streaming closure captures _captured_logging_obj (= logging_obj) and is stored on logging_obj._on_deferred_stream_complete, creating a cycle. The cycle is broken when the closure fires, but persists for abandoned (disconnected) streams until GC collects the orphaned CustomStreamWrapper. High client-disconnect rates may produce elevated memory pressure.
  • Fragile asyncio.sleep(0) synchronization in tests (test_deferred_guardrail_logging.py, line 954): Two-level sleep(0) chains for two nested create_task calls work with CPython's default event loop but can produce flaky results under other schedulers or when the event loop has additional pending tasks.

Confidence Score: 3/5

  • The non-streaming fix is correct and well-tested; the streaming fix introduces an unintended backwards-incompatible behavioral change for non-guardrail callbacks that should be addressed before merging.
  • The core deferred-logging mechanism is sound and the test suite covers the main happy-path and exception scenarios. However, the streaming closure calls post_call_success_hook for all callbacks (not just guardrails), which is a behavioral change for existing streaming integrations with async_post_call_success_hook implementations. Additionally, the known limitation that standard OpenAI-compatible streaming doesn't benefit from the fix (noted in a prior review thread) reduces overall confidence. These issues together warrant a score below 4.
  • litellm/proxy/common_request_processing.py — specifically the _on_deferred_stream_complete closure's invocation of post_call_success_hook for all callbacks.

Important Files Changed

Filename Overview
litellm/proxy/common_request_processing.py Core change: adds _has_post_call_guardrails() detection, _defer_async_logging flag for non-streaming, and a streaming closure _on_deferred_stream_complete attached to logging_obj. The deferred closure calls post_call_success_hook for ALL callbacks (not just guardrails), introducing a backwards-incompatible behavioral change for streaming. The try/finally block correctly ensures deferred logging fires even when guardrails raise.
litellm/litellm_core_utils/streaming_handler.py Checks for _on_deferred_stream_complete closure on logging_obj at stream exhaustion. If set, clears it and fires the closure via create_task; otherwise falls back to the original async_success_handler + executor.submit path. The split is clean and the original logging path is fully preserved for non-deferred cases.
litellm/utils.py Defers asyncio.create_task for async logging when _defer_async_logging flag is set, storing a closure instead. Sync callbacks (handle_sync_success_callbacks_for_async_calls) correctly fire immediately outside the if/else — addressing the concern raised in prior reviews. The deferred closure correctly duplicates all parameters from the original create_task call.
tests/test_litellm/proxy/guardrails/test_deferred_guardrail_logging.py Good coverage of detection logic and deferral mechanism. However, most streaming closure tests replicate the production closure in-test rather than exercising the actual base_process_llm_request method. The asyncio.sleep(0) synchronization pattern used for two-level create_task chains can be fragile under alternative event loop schedulers.
docs/my-website/docs/proxy/guardrails/custom_guardrail.md Adds accurate documentation noting that streaming post_call guardrails are audit-only (cannot block delivery), and updates the capability table to reflect that async_post_call_success_hook can only block for non-streaming. No issues found.

Sequence Diagram

sequenceDiagram
    participant Client
    participant ProxyHandler as base_process_llm_request
    participant RouterWrapper as wrapper_async (utils.py)
    participant CSW as CustomStreamWrapper
    participant PostCallHook as post_call_success_hook
    participant Logger as async_success_handler

    Note over ProxyHandler: _has_post_call_guardrails() → True

    alt Non-Streaming Request
        ProxyHandler->>ProxyHandler: logging_obj._defer_async_logging = True
        ProxyHandler->>RouterWrapper: route_request()
        RouterWrapper->>RouterWrapper: store _enqueue_deferred_logging closure (create_task deferred)
        RouterWrapper->>RouterWrapper: handle_sync_success_callbacks immediately
        RouterWrapper-->>ProxyHandler: ModelResponse
        ProxyHandler->>PostCallHook: post_call_success_hook() [writes guardrail_information]
        Note over ProxyHandler: finally block
        ProxyHandler->>RouterWrapper: _enqueue_fn() → asyncio.create_task(logging)
        RouterWrapper->>Logger: _client_async_logging_helper (with guardrail_information)
    else Streaming Request (CustomStreamWrapper)
        ProxyHandler->>ProxyHandler: attach _on_deferred_stream_complete closure to logging_obj
        ProxyHandler->>Client: return StreamingResponse (early return)
        Client->>CSW: consume chunks via __anext__
        CSW-->>Client: stream chunks...
        CSW->>CSW: stream exhausted → check _on_deferred_stream_complete
        CSW->>PostCallHook: _deferred_cb(assembled_response) → post_call_success_hook
        PostCallHook-->>CSW: guardrail_information written to metadata
        CSW->>Logger: asyncio.create_task(async_success_handler)
        CSW->>Logger: executor.submit(success_handler)
    end
Loading

Comments Outside Diff (3)

  1. tests/test_litellm/proxy/guardrails/test_deferred_guardrail_logging.py, line 954-960 (link)

    asyncio.sleep(0) count for task synchronization is fragile

    The comment "Two sleeps: outer create_task … then inner create_task" relies on the asyncio event loop processing exactly one pending task per sleep(0) yield. This is true for CPython's default event loop on a lightly loaded event loop, but can fail if the event loop schedules tasks differently (e.g., multiple tasks enqueued in the same turn, or a different event loop implementation).

    A more robust approach is to drain all pending tasks explicitly:

    # Instead of two sleep(0) calls, drain all pending tasks
    while any(not t.done() for t in asyncio.all_tasks() if t is not asyncio.current_task()):
        await asyncio.sleep(0)

    Or use asyncio.gather to await the task directly if you can capture a reference to it. The same pattern appears in test_closure_logs_even_on_guardrail_exception (lines 1008–1009) and test_production_closure_calls_post_call_success_hook (lines 1141–1142).

  2. litellm/proxy/common_request_processing.py, line 1052-1062 (link)

    Closure creates a self-referential reference cycle on logging_obj

    The closure captures _captured_logging_obj = logging_obj (line ~1059), and the closure itself is stored on logging_obj._on_deferred_stream_complete. This creates a reference cycle:

    logging_obj  →  _on_deferred_stream_complete  →  _captured_logging_obj  →  logging_obj
    

    Python's cyclic GC handles this, and the closure does break the cycle at invocation time via self.logging_obj._on_deferred_stream_complete = None. However, if a client disconnects before the stream is exhausted, __anext__ never reaches the termination branch, the closure never fires, and the cycle persists until the next GC pass collects the orphaned CustomStreamWrapper.

    Consider using a weakref for _captured_logging_obj inside the closure, or explicitly clearing the closure on stream cancellation/error paths, to avoid extended object lifetimes under high client-disconnect rates.

  3. litellm/proxy/common_request_processing.py, line 1077-1090 (link)

    post_call_success_hook now invoked for streaming — behavioral change for non-guardrail callbacks

    The deferred streaming closure calls _captured_proxy_logging_obj.post_call_success_hook(...), which iterates all registered async_post_call_success_hook callbacks — not just CustomGuardrail instances.

    Before this PR, for streaming requests routed through select_data_generator (the standard OpenAI-compatible path), the code returned early and post_call_success_hook was never called. After this PR, when any post-call guardrail is registered, the hook fires on the assembled response for every streaming request.

    This means:

    1. Custom loggers or integrations that implement async_post_call_success_hook will now run for streaming responses when post-call guardrails are configured — even if they were never designed to handle assembled streaming responses.
    2. If an integration implements both standard logging success callbacks (handled by async_success_handler, which the closure also calls) and async_post_call_success_hook, it will be invoked twice on the same assembled response.

    Per the "avoid backwards-incompatible changes without user-controlled flags" guideline, consider scoping the closure's hook invocation to only guardrail callbacks rather than all callbacks via post_call_success_hook, to avoid surprising existing integrations.

Last reviewed commit: "capture proxy_loggin..."

Comment on lines +1085 to +1088
### CALL HOOKS ### - modify outgoing data
response = await proxy_logging_obj.post_call_success_hook(
data=self.data, user_api_key_dict=user_api_key_dict, response=response
)

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.

P1 Standard streaming + post-call guardrails still race

For standard OpenAI-compatible streaming requests (i.e. those that don't hit the allm_passthrough_route, anthropic_messages, or select_data_generator early-returns), control flows straight through the streaming branch and arrives at post_call_success_hook on line 1086. So guardrails DO run. However, because wrapper_async returned the CustomStreamWrapper early (before the closure-storage block in utils.py), _enqueue_deferred_logging is never set.

As a result the per-chunk success logging emitted by CustomStreamWrapper still fires before post_call_success_hook completes, reproducing the original guardrail_information=None bug for streaming requests with post-call guardrails.

The PR description is clear that this fix targets the non-streaming case, but the limitation may be worth documenting with a TODO or follow-up issue so it isn't lost.

Comment on lines 1234 to 1261
return True
return False

@staticmethod
def _has_post_call_guardrails() -> bool:
"""
Check if any registered callback is a post-call guardrail.

Uses the global litellm.callbacks list rather than per-request
should_run_guardrail() — intentionally conservative so that the
check is simple and stateless. The deferral path produces
identical logging output, just fires it slightly later, so
false-positives are harmless.
"""
from litellm.integrations.custom_guardrail import CustomGuardrail
from litellm.types.guardrails import GuardrailEventHooks

for cb in litellm.callbacks:
if (
not isinstance(cb, str)
and isinstance(cb, CustomGuardrail)
and cb._event_hook_is_event_type(GuardrailEventHooks.post_call)
):
return True
return False

async def _handle_llm_api_exception(
self,

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.

P2 Per-request scan of global callbacks list

_has_post_call_guardrails() is a static method that iterates the entire litellm.callbacks list (and re-imports two modules) on every call to base_process_llm_request. For high-throughput deployments with many callbacks this adds avoidable latency in the hot path.

A simple improvement would be to cache the result and invalidate it when litellm.callbacks is mutated (e.g. via a module-level flag), or move the scan to proxy startup/config load time. At minimum, move the imports to module level so the import lookup overhead is paid once:

# at module top
from litellm.integrations.custom_guardrail import CustomGuardrail
from litellm.types.guardrails import GuardrailEventHooks

Comment thread litellm/utils.py
Comment on lines +1951 to +1967
def _enqueue_deferred_logging() -> None:
asyncio.create_task(
_client_async_logging_helper(
logging_obj=logging_obj,
result=result,
start_time=start_time,
end_time=end_time,
is_completion_with_fallbacks=is_completion_with_fallbacks,
)
)
logging_obj.handle_sync_success_callbacks_for_async_calls(
result=result,
start_time=start_time,
end_time=end_time,
)

logging_obj._enqueue_deferred_logging = _enqueue_deferred_logging # type: ignore

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.

P1 handle_sync_success_callbacks_for_async_calls is also deferred

In the original code, handle_sync_success_callbacks_for_async_calls was called synchronously and immediately (before post_call_success_hook ran). In the deferred closure it now fires only after guardrails complete. This is a behavioral change for the sync-callback path that isn't mentioned in the PR description.

If any sync callback relies on being called promptly (e.g. for billing meters, rate-limit counters, or cache warming that must happen before the response is returned to the client), deferring it may cause subtle bugs. Consider whether the sync callbacks should remain immediate and only asyncio.create_task (the async logging) should be deferred:

# Sync callbacks fire immediately; only async logging is deferred
logging_obj.handle_sync_success_callbacks_for_async_calls(
    result=result,
    start_time=start_time,
    end_time=end_time,
)

def _enqueue_deferred_logging() -> None:
    asyncio.create_task(
        _client_async_logging_helper(
            logging_obj=logging_obj,
            result=result,
            start_time=start_time,
            end_time=end_time,
            is_completion_with_fallbacks=is_completion_with_fallbacks,
        )
    )

logging_obj._enqueue_deferred_logging = _enqueue_deferred_logging

Comment on lines +241 to +243
from fastapi import HTTPException

raise HTTPException(status_code=400, detail="Content blocked")

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.

P2 FastAPI import inside test helper

from fastapi import HTTPException is imported inside the guardrail hook method and again at line 254 inside the test function. While test files are generally excluded from the "no FastAPI imports outside proxy/" rule, importing inside function bodies instead of at the top of the file is unusual and makes dependencies harder to track. It also won't help with import-time CI failures.

Consider adding pytest.importorskip("fastapi") at the top of the file or a module-level from fastapi import HTTPException import alongside the other proxy imports already present (e.g. from litellm.proxy._types import UserAPIKeyAuth).

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!

Comment on lines +1251 to +1256
if (
not isinstance(cb, str)
and isinstance(cb, CustomGuardrail)
and cb._event_hook_is_event_type(GuardrailEventHooks.post_call)
):
return True

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.

P2 Redundant isinstance guard

The not isinstance(cb, str) check on line 1251 is redundant: isinstance(cb, CustomGuardrail) already returns False for any str (since str doesn't inherit from CustomGuardrail). Removing it makes the intent clearer.

Suggested change
if (
not isinstance(cb, str)
and isinstance(cb, CustomGuardrail)
and cb._event_hook_is_event_type(GuardrailEventHooks.post_call)
):
return True
for cb in litellm.callbacks:
if (
isinstance(cb, CustomGuardrail)
and cb._event_hook_is_event_type(GuardrailEventHooks.post_call)
):
return True

Comment on lines +89 to +111
class TestHasPostCallGuardrails:
def test_returns_true_for_post_call_guardrail(self):
with patch("litellm.callbacks", [PostCallGuardrail()]):
assert ProxyBaseLLMRequestProcessing._has_post_call_guardrails() is True

def test_returns_true_for_event_hook_none(self):
"""event_hook=None means 'all events', including post_call."""
with patch("litellm.callbacks", [AllEventsGuardrail()]):
assert ProxyBaseLLMRequestProcessing._has_post_call_guardrails() is True

def test_returns_false_for_pre_call_only(self):
with patch("litellm.callbacks", [PreCallGuardrail()]):
assert ProxyBaseLLMRequestProcessing._has_post_call_guardrails() is False

def test_returns_false_for_no_callbacks(self):
with patch("litellm.callbacks", []):
assert ProxyBaseLLMRequestProcessing._has_post_call_guardrails() is False

def test_ignores_non_guardrail_callbacks(self):
"""String callbacks and CustomLogger instances are not guardrails."""
with patch("litellm.callbacks", ["langfuse", CustomLogger()]):
assert ProxyBaseLLMRequestProcessing._has_post_call_guardrails() is False

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.

P2 Missing test for list-typed event_hook

_event_hook_is_event_type has a third branch for isinstance(self.event_hook, list) (see custom_guardrail.py:472), meaning users can write event_hook=["post_call", "pre_call"]. The detection test suite doesn't cover this case — a list containing post_call should return True, a list that only contains pre_call should return False.

Adding these two cases would close the gap:

def test_returns_true_for_list_with_post_call(self):
    class ListGuardrail(CustomGuardrail):
        def __init__(self):
            super().__init__(
                guardrail_name="list-post",
                default_on=True,
                event_hook=["pre_call", "post_call"],
            )
    with patch("litellm.callbacks", [ListGuardrail()]):
        assert ProxyBaseLLMRequestProcessing._has_post_call_guardrails() is True

def test_returns_false_for_list_without_post_call(self):
    class ListGuardrail(CustomGuardrail):
        def __init__(self):
            super().__init__(
                guardrail_name="list-pre",
                default_on=True,
                event_hook=["pre_call"],
            )
    with patch("litellm.callbacks", [ListGuardrail()]):
        assert ProxyBaseLLMRequestProcessing._has_post_call_guardrails() is False

Comment thread litellm/utils.py
Comment on lines +1955 to +1971
def _enqueue_deferred_logging() -> None:
asyncio.create_task(
_client_async_logging_helper(
logging_obj=logging_obj,
result=result,
start_time=start_time,
end_time=end_time,
is_completion_with_fallbacks=is_completion_with_fallbacks,
)
)
logging_obj.handle_sync_success_callbacks_for_async_calls(
result=result,
start_time=start_time,
end_time=end_time,
)

logging_obj._enqueue_deferred_logging = _enqueue_deferred_logging # type: ignore

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.

P2 asyncio.create_task called inside a synchronous closure

_enqueue_deferred_logging is defined as a plain def (synchronous) yet calls asyncio.create_task, which requires a running event loop at call time. This works correctly today because the closure is always invoked from inside the async def base_process_llm_request finally block where an event loop is guaranteed.

However, the type signature () -> None gives no indication of this constraint. If the closure were accidentally called from a sync context (e.g., a cleanup path added in the future), it would raise RuntimeError: no running event loop. Consider documenting this requirement with a comment or an assertion:

def _enqueue_deferred_logging() -> None:
    # Must be called from within a running event loop (async context).
    asyncio.create_task(
        _client_async_logging_helper(
            ...
        )
    )

Comment on lines +1091 to +1101
finally:
# Enqueue deferred logging after post-call guardrails have written
# guardrail_information to metadata. The finally block ensures
# logging fires even if a guardrail raises (matching the current
# behavior where create_task fires before post_call_success_hook).
# For streaming early-returns: no closure is stored (wrapper_async
# returns before line 1946), so _enqueue_fn is None — this is a no-op.
_enqueue_fn = getattr(logging_obj, "_enqueue_deferred_logging", None)
if _enqueue_fn is not None:
logging_obj._enqueue_deferred_logging = None # type: ignore[attr-defined]
_enqueue_fn()

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.

P1 Exception in finally can mask guardrail error

If post_call_success_hook raises (e.g., a guardrail blocks content with HTTPException) and then _enqueue_fn() also raises (e.g., a RuntimeError from asyncio.create_task in a degraded state), Python will replace the original exception with the one from finally. The caller — and the client — will see a logging error instead of the guardrail's blocking response.

Wrapping the closure call in a bare except (log-and-swallow) prevents this:

finally:
    _enqueue_fn = getattr(logging_obj, "_enqueue_deferred_logging", None)
    if _enqueue_fn is not None:
        logging_obj._enqueue_deferred_logging = None  # type: ignore[attr-defined]
        try:
            _enqueue_fn()
        except Exception:
            verbose_proxy_logger.exception(
                "Failed to enqueue deferred async logging"
            )

This mirrors the existing pattern throughout the proxy where logging errors are swallowed so they never surface to clients.

Comment thread litellm/utils.py Outdated
Comment on lines 1949 to 1986
if getattr(logging_obj, "_defer_async_logging", False):
# Proxy has post-call guardrails that must complete before the
# SLO is built. Store a closure the proxy will call after
# post_call_success_hook so guardrail_information is in metadata.
# Only create_task is deferred; sync callbacks fire immediately
# to preserve existing behavior for billing/rate-limiting.
def _enqueue_deferred_logging() -> None:
asyncio.create_task(
_client_async_logging_helper(
logging_obj=logging_obj,
result=result,
start_time=start_time,
end_time=end_time,
is_completion_with_fallbacks=is_completion_with_fallbacks,
)
)

logging_obj._enqueue_deferred_logging = _enqueue_deferred_logging # type: ignore
logging_obj.handle_sync_success_callbacks_for_async_calls(
result=result,
start_time=start_time,
end_time=end_time,
)
else:
asyncio.create_task(
_client_async_logging_helper(
logging_obj=logging_obj,
result=result,
start_time=start_time,
end_time=end_time,
is_completion_with_fallbacks=is_completion_with_fallbacks,
)
)
logging_obj.handle_sync_success_callbacks_for_async_calls(
result=result,
start_time=start_time,
end_time=end_time,
is_completion_with_fallbacks=is_completion_with_fallbacks,
)

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.

P2 Duplicated logging call in else branch

The asyncio.create_task(...) + handle_sync_success_callbacks_for_async_calls(...) pair is now written twice — once inside the deferred closure and once in the else branch. If a new parameter is added to either call in the future, both copies must be updated in sync.

Extracting a small helper avoids this:

async def _make_logging_coro():
    return _client_async_logging_helper(
        logging_obj=logging_obj,
        result=result,
        start_time=start_time,
        end_time=end_time,
        is_completion_with_fallbacks=is_completion_with_fallbacks,
    )

if getattr(logging_obj, "_defer_async_logging", False):
    def _enqueue_deferred_logging() -> None:
        asyncio.create_task(_make_logging_coro())

    logging_obj._enqueue_deferred_logging = _enqueue_deferred_logging  # type: ignore
else:
    asyncio.create_task(_make_logging_coro())

logging_obj.handle_sync_success_callbacks_for_async_calls(
    result=result,
    start_time=start_time,
    end_time=end_time,
)

This also moves handle_sync_success_callbacks_for_async_calls out of the if/else, making it clearer that sync callbacks always fire immediately regardless of the deferral flag.

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!

Comment on lines +256 to +268
with pytest.raises(HTTPException):
try:
await proxy_logging.post_call_success_hook(
data={"model": "gpt-4", "metadata": {}},
response=MagicMock(),
user_api_key_dict=UserAPIKeyAuth(api_key="test"),
)
finally:
# This mirrors the proxy's finally block
_enqueue_fn = getattr(logging_obj, "_enqueue_deferred_logging", None)
if _enqueue_fn is not None:
logging_obj._enqueue_deferred_logging = None
_enqueue_fn()

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.

P2 Test replicates the proxy finally pattern rather than exercising it

test_deferred_logging_fires_on_guardrail_exception manually inlines the try/finally block from base_process_llm_request instead of calling that method. As a result, if the production finally block in common_request_processing.py is ever changed (e.g., the getattr key renamed, the inner try/except removed, or the None-clear logic dropped), this test will continue to pass even though the real proxy path is broken.

Consider adding a separate integration test that calls base_process_llm_request directly with a mocked router and a blocking guardrail, asserting that _enqueue_fn is called after the guardrail raises. The current test is still useful for verifying the helper logic, but a test that exercises the actual proxy method would provide stronger regression protection.

Comment on lines +931 to +934
# Defer async logging when post-call guardrails are configured so the
# StandardLoggingPayload is built after guardrails write to metadata.
if self._has_post_call_guardrails():
logging_obj._defer_async_logging = True # type: ignore

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.

P2 _defer_async_logging flag is set even for requests that will take a streaming early-return

_has_post_call_guardrails() is evaluated unconditionally before route_request, so when post-call guardrails are registered, _defer_async_logging = True is set on every logging_obj — including those for streaming requests that will ultimately return from within the try block (e.g., allm_passthrough_route, anthropic_messages streaming, select_data_generator). For those paths wrapper_async returns a CustomStreamWrapper before reaching the closure-storage code in utils.py, so _enqueue_deferred_logging is never stored and the finally block is a harmless no-op.

However, the _defer_async_logging = True flag persists on logging_obj for the lifetime of the streaming wrapper, and any future code that reads this flag on the streaming logging_obj could behave unexpectedly. A guard like if not (self._is_streaming_request(...) or ...) around the flag-set, or a reset of the flag inside the streaming exit path, would make the invariant explicit.

… simplify guardrail detection, add list event_hook tests
Comment thread litellm/proxy/common_request_processing.py Fixed
@michelligabriele
michelligabriele force-pushed the fix/deferred-logging-post-call-guardrails branch from d7a7058 to b4122c6 Compare March 19, 2026 10:58
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions Bot added the stale label Jun 18, 2026
@github-actions github-actions Bot closed this Jun 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants