fix(logging): route realtime success logging through the bounded worker - #31733
Conversation
Greptile SummaryThis PR routes streaming and realtime success-logging calls through
Confidence Score: 4/5Safe to merge; the bounded-worker dispatch is the correct fix and all three changed sites handle the lazy import correctly. The core fix is straightforward and mechanically consistent — the same one-line swap is applied in three files. The test for The test file (
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/realtime_streaming.py | Replaces bare asyncio.create_task with GLOBAL_LOGGING_WORKER.ensure_initialized_and_enqueue for async success logging in RealTimeStreaming.log_messages; fix is correct and consistent with how the non-streaming path was already handled. |
| litellm/litellm_core_utils/streaming_handler.py | Routes dispatch_success_handlers through GLOBAL_LOGGING_WORKER instead of a bare create_task; the lazy import pattern is correct and no regression test was added for this path. |
| litellm/responses/streaming_iterator.py | Fixes two asyncio.create_task call sites in BaseResponsesAPIStreamingIterator._log_completion (async branch) and ResponsesWebSocketStreaming._log_messages; both changes are correct and no regression tests were added. |
| tests/test_litellm/litellm_core_utils/test_realtime_streaming.py | Adds a regression test for the realtime_streaming.py fix; test logic is sound but assertions are placed outside the with patch block, and no parallel tests exist for the streaming_handler.py or streaming_iterator.py changes. |
Comments Outside Diff (1)
-
litellm/litellm_core_utils/streaming_handler.py, line 2050-2068 (link)No regression test for the
streaming_handler.pyfixThe PR fixes the unbounded-task accumulation in three files but adds a test only for
realtime_streaming.py. The equivalent dispatch path instreaming_handler.py(CustomStreamWrapper's async-for iteration) andstreaming_iterator.py(BaseResponsesAPIStreamingIteratorandResponsesWebSocketStreaming) have no corresponding regression guard. Per the project's review standard, fixes to the unbounded-task issue should be covered by tests at each callsite.Rule Used: What: Ensure that any PR claiming to fix an issue ... (source)
Reviews (1): Last reviewed commit: "fix(logging): route streaming/realtime s..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
81c1f5d to
5757829
Compare
RealTimeStreaming.log_messages dispatched the success handler with a bare asyncio.create_task, bypassing GLOBAL_LOGGING_WORKER (which gives a per-coroutine timeout and a concurrency cap). On a long-lived realtime websocket a slow logging callback left one suspended task per logged turn, each pinning that turn's assembled response, accumulating without bound (~12-15k in-flight under load in a repro) until OOM. Route realtime success logging through the bounded worker so in-flight logging is capped and a hung callback is cancelled at the worker timeout. The chat and responses streaming success-logging paths are intentionally left unchanged: their success callbacks must complete within the call's event-loop run (the non-streaming path pairs the worker with a synchronous callback; the streaming path has no such companion), so deferring them through the worker would drop logs for one-shot SDK calls and breaks test_async_custom_handler_stream. Bounding those paths needs a load-shedding approach and is left to a follow-up.
5757829 to
eb6fc7d
Compare
|
bugbot run |
|
Verdict: Request-changes — PR body lies about scope. The body says it touches realtime_streaming.py and streaming_handler.py and responses/streaming_iterator.py. The actual diff only touches realtime_streaming.py (+ test). The other two leak paths the body claims to fix are still leaking. Either fix all three (matches the body and the "reproduced 14,710 in-flight" claim), or rewrite the body to admit this is realtime-only. Secondary: test monkeypatches the imported GLOBAL_LOGGING_WORKER symbol — works, but [TEST-4] prefers DI. Acceptable for a regression test. Comment on lines 318-321 is fine — that one is load-bearing. feedback from claude. Is it legit? |
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 eb6fc7d. Configure here.
|
rewrote the pr description, this is meant for realtime only. havent seen any reports about this on responses api so ill keep the fix isolated to realtime @yucheng-berri |
|
Verdict: Request-changes — either extend the same one-line worker-routing fix to those two files (preferred, since the pattern is identical) or rewrite the body to admit realtime-only and file a follow-up. Test monkeypatches the module symbol rather than DI'ing the worker, which is a [TEST-4] nit acceptable here; and the pre-existing executor.submit(success_handler(self.messages)) on line 323 looks like it's calling the handler synchronously and submitting the return value — worth flagging while in the area but not introduced by this PR. i think the description is still mentioning 3 files change |
…er (BerriAI#31733) RealTimeStreaming.log_messages dispatched the success handler with a bare asyncio.create_task, bypassing GLOBAL_LOGGING_WORKER (which gives a per-coroutine timeout and a concurrency cap). On a long-lived realtime websocket a slow logging callback left one suspended task per logged turn, each pinning that turn's assembled response, accumulating without bound (~12-15k in-flight under load in a repro) until OOM. Route realtime success logging through the bounded worker so in-flight logging is capped and a hung callback is cancelled at the worker timeout. The chat and responses streaming success-logging paths are intentionally left unchanged: their success callbacks must complete within the call's event-loop run (the non-streaming path pairs the worker with a synchronous callback; the streaming path has no such companion), so deferring them through the worker would drop logs for one-shot SDK calls and breaks test_async_custom_handler_stream. Bounding those paths needs a load-shedding approach and is left to a follow-up.
…to v1.90.3 (#257) This PR contains the following updates: | Package | Update | Change | |---|---|---| | [https://github.com/BerriAI/litellm.git](https://github.com/BerriAI/litellm) | patch | `v1.90.0` → `v1.90.3` | --- ### Release Notes <details> <summary>BerriAI/litellm (https://github.com/BerriAI/litellm.git)</summary> ### [`v1.90.3`](https://github.com/BerriAI/litellm/releases/tag/v1.90.3) [Compare Source](BerriAI/litellm@v1.90.2...v1.90.3) #### Verify Docker Image Signature All LiteLLM Docker images are signed with [cosign](https://docs.sigstore.dev/cosign/overview/). Every release is signed with the same key introduced in [commit `0112e53`](BerriAI/litellm@0112e53). **Verify using the pinned commit hash (recommended):** A commit hash is cryptographically immutable, so this is the strongest way to ensure you are using the original signing key: ```bash cosign verify \ --key https://raw.githubusercontent.com/BerriAI/litellm/0112e53046018d726492c814b3644b7d376029d0/cosign.pub \ ghcr.io/berriai/litellm:v1.90.3 ``` **Verify using the release tag (convenience):** Tags are protected in this repository and resolve to the same key. This option is easier to read but relies on tag protection rules: ```bash cosign verify \ --key https://raw.githubusercontent.com/BerriAI/litellm/v1.90.3/cosign.pub \ ghcr.io/berriai/litellm:v1.90.3 ``` Expected output: ``` The following checks were performed on each of these signatures: - The cosign claims were validated - The signatures were verified against the specified public key ``` *** #### What's Changed - chore(release): backport [#​31923](BerriAI/litellm#31923), [#​31929](BerriAI/litellm#31929), [#​31393](BerriAI/litellm#31393) to stable/1.90.x and cut 1.90.3 by [@​mateo-berri](https://github.com/mateo-berri) in [#​32025](BerriAI/litellm#32025) **Full Changelog**: <BerriAI/litellm@v1.90.2...v1.90.3> ### [`v1.90.2`](https://github.com/BerriAI/litellm/releases/tag/v1.90.2) [Compare Source](BerriAI/litellm@v1.90.1...v1.90.2) #### Verify Docker Image Signature All LiteLLM Docker images are signed with [cosign](https://docs.sigstore.dev/cosign/overview/). Every release is signed with the same key introduced in [commit `0112e53`](BerriAI/litellm@0112e53). **Verify using the pinned commit hash (recommended):** A commit hash is cryptographically immutable, so this is the strongest way to ensure you are using the original signing key: ```bash cosign verify \ --key https://raw.githubusercontent.com/BerriAI/litellm/0112e53046018d726492c814b3644b7d376029d0/cosign.pub \ ghcr.io/berriai/litellm:v1.90.2 ``` **Verify using the release tag (convenience):** Tags are protected in this repository and resolve to the same key. This option is easier to read but relies on tag protection rules: ```bash cosign verify \ --key https://raw.githubusercontent.com/BerriAI/litellm/v1.90.2/cosign.pub \ ghcr.io/berriai/litellm:v1.90.2 ``` Expected output: ``` The following checks were performed on each of these signatures: - The cosign claims were validated - The signatures were verified against the specified public key ``` *** #### What's Changed - chore(release): backport [#​31519](BerriAI/litellm#31519), [#​31733](BerriAI/litellm#31733) to stable/1.90.x and cut 1.90.2 by [@​yuneng-berri](https://github.com/yuneng-berri) in [#​31782](BerriAI/litellm#31782) **Full Changelog**: <BerriAI/litellm@v1.90.1...v1.90.2> ### [`v1.90.1`](https://github.com/BerriAI/litellm/releases/tag/v1.90.1) [Compare Source](BerriAI/litellm@v1.90.0-rc.1...v1.90.1) #### Verify Docker Image Signature All LiteLLM Docker images are signed with [cosign](https://docs.sigstore.dev/cosign/overview/). Every release is signed with the same key introduced in [commit `0112e53`](BerriAI/litellm@0112e53). **Verify using the pinned commit hash (recommended):** A commit hash is cryptographically immutable, so this is the strongest way to ensure you are using the original signing key: ```bash cosign verify \ --key https://raw.githubusercontent.com/BerriAI/litellm/0112e53046018d726492c814b3644b7d376029d0/cosign.pub \ ghcr.io/berriai/litellm:v1.90.1 ``` **Verify using the release tag (convenience):** Tags are protected in this repository and resolve to the same key. This option is easier to read but relies on tag protection rules: ```bash cosign verify \ --key https://raw.githubusercontent.com/BerriAI/litellm/v1.90.1/cosign.pub \ ghcr.io/berriai/litellm:v1.90.1 ``` Expected output: ``` The following checks were performed on each of these signatures: - The cosign claims were validated - The signatures were verified against the specified public key ``` *** #### What's Changed - chore(release): backport [#​31036](BerriAI/litellm#31036), [#​31342](BerriAI/litellm#31342), [#​31653](BerriAI/litellm#31653) to stable/1.90.x and cut 1.90.1 (litellm-enterprise 0.1.43.post1) by [@​yuneng-berri](https://github.com/yuneng-berri) in [#​31667](BerriAI/litellm#31667) **Full Changelog**: <BerriAI/litellm@v1.90.0...v1.90.1> </details> --- ### Configuration 📅 **Schedule**: (UTC) - Branch creation - At any time (no schedule defined) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4yNDkuNSIsInVwZGF0ZWRJblZlciI6IjQzLjI0OS41IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJkZXBlbmRlbmNpZXMiXX0=--> Co-authored-by: Renovate Bot <renovate@bhamm-lab.com> Reviewed-on: https://codeberg.org/blake-hamm/bhamm-lab/pulls/257
Relevant issues
Streaming and realtime success logging accumulates unbounded suspended tasks when a logging callback is slow
Linear ticket
Pre-Submission checklist
@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewScreenshots / Proof of Fix
Streaming, realtime api success logging dispatch the success handler with a bare
asyncio.create_task, which bypassesGLOBAL_LOGGING_WORKER. The non-streaming path already routes through that worker for a per-coroutine timeout and a concurrency cap. When a configured callback is slow (e.g. an external sink that lags), the bare task has neither bound, so each streamed or realtime turn leaves one suspended task pinning that turn's assembled response, and they accumulate without bound until the process OOMsReproduced against a live proxy (single worker) driving sustained streaming load through a deliberately slow success callback (a
CustomLoggerwhoseasync_log_success_eventhangs), comparing the patched and unpatched images. Mock responses were used so the measurement isolates the logging path rather than upstream latency. In-flight success-logging calls were counted in-process:Note: a follow-up will additionally bound the worker's queue (drop-on-full with a counter) to cap the second-order buffer under sustained slow-sink load. This PR addresses the unbounded in-flight task accumulation, which is the primary leak
Type
🐛 Bug Fix
Changes
create_tasksuccess-logging dispatches throughGLOBAL_LOGGING_WORKERinlitellm/litellm_core_utils/realtime_streaming.py,litellm/litellm_core_utils/streaming_handler.py, andtests/test_litellm/litellm_core_utils/test_realtime_streaming.py: regression test asserting realtime success logging routes through the bounded worker and not a barecreate_taskNote
Medium Risk
Changes realtime success-logging dispatch under sustained load; mis-routing could drop or delay spend/audit callbacks, but scope is limited to the logging enqueue path with a targeted regression test.
Overview
Fixes unbounded memory growth when realtime sessions use slow async success logging callbacks (e.g. lagging external sinks).
RealTimeStreaming.log_messagesno longer schedulesasync_success_handlerwith a bareasyncio.create_task. It now enqueues that coroutine onGLOBAL_LOGGING_WORKER, matching the non-streaming path so success logging gets a concurrency cap and per-coroutine timeout instead of piling up suspended tasks that keep each turn’s assembled response in memory.A regression test asserts realtime success logging goes through the worker and does not call
asyncio.create_taskfor that path.Reviewed by Cursor Bugbot for commit eb6fc7d. Bugbot is set up for automated code reviews on this repo. Configure here.