mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-13 03:59:12 +00:00
fix(streaming): stop submitting the terminal log from __del__
`tests/unit/test_router_silent_experiment.py` failed on this branch with the
shadow and fallback streaming tests seeing zero success callbacks:
assert 0 == 1 # len(recording_logger.shadow_successes())
Bisected locally against `upstream/main` (26 passed there) — the finalizer is
the cause, not `close()` or `aclose()`. Both of those turned out to be
harmless: removing either one alone left all four tests failing, and removing
the `__del__` flush took the file from 4 failed / 22 passed to 26 passed.
The reason it breaks them is that `__del__` fires at an arbitrary point on an
arbitrary thread. In these tests the wrapper is collected while the recording
logger's bookkeeping is in flight, so the abandoned-stream flush races with
the success callbacks the drain is supposed to produce. It is also unsound in
general: a finalizer can run during interpreter shutdown or after the event
loop it would need is gone, and it cannot report its own failure.
So the flush is now reachable only through `close_stream()` and the context
manager, which is where a caller has actually decided it is abandoning a
stream. The docstrings say so explicitly rather than implying GC coverage.
Verified locally: `test_router_silent_experiment.py` 26/26, the six tests
added here 6/6, and `test_streaming_handler.py` unchanged against baseline
(5 failed / 166 passed both with and without this branch — those five are
pre-existing in this environment).
This commit is contained in:
parent
5643b80340
commit
66495a352c
1 changed files with 8 additions and 4 deletions
|
|
@ -319,6 +319,13 @@ class CustomStreamWrapper:
|
|||
call even though tokens were served. Assemble the response from the
|
||||
chunks received so far and submit the same logging once, mirroring how
|
||||
the proxy bills a partial stream on client disconnect.
|
||||
|
||||
Only reachable through ``close_stream()`` / the context manager. It is
|
||||
deliberately not wired into ``__del__``: a finalizer runs at an
|
||||
arbitrary point on an arbitrary thread, possibly during interpreter
|
||||
shutdown or after the event loop it needs is gone, so submitting
|
||||
logging from there is neither reliable nor testable. A caller that
|
||||
abandons a stream has to say so.
|
||||
"""
|
||||
if self._abandonment_logged or self.sent_last_chunk:
|
||||
return
|
||||
|
|
@ -371,6 +378,7 @@ class CustomStreamWrapper:
|
|||
all miss the call). ``close_stream()`` flushes that log from the chunks
|
||||
received so far and releases the underlying provider stream; safe to
|
||||
call after the stream has been fully consumed (it is a no-op then).
|
||||
This is the only entry point — there is no GC-time fallback.
|
||||
|
||||
Deliberately *not* named ``close``: ``Router.stream_with_fallbacks``
|
||||
duck-types on ``hasattr(model_response, "close")`` and calls it from a
|
||||
|
|
@ -459,10 +467,6 @@ class CustomStreamWrapper:
|
|||
still-active call's context within that same Task if this fires late.
|
||||
"""
|
||||
self._restore_consumer_correlation_context(guarded=True)
|
||||
try:
|
||||
self._flush_abandoned_stream_logging()
|
||||
except Exception: # noqa: BLE001 # __del__ must never raise; logging here is best-effort
|
||||
verbose_logger.debug("abandoned-stream logging failed in __del__", exc_info=True)
|
||||
|
||||
async def aclose(self):
|
||||
# Emit the abandoned-stream log before closing the provider stream:
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue