fix(logging_worker): carry queued tasks across event-loop change instead of dropping them - #38144
Conversation
…ead of dropping them LoggingWorker._ensure_queue nulled self._queue on a loop change, discarding every pending LoggingTask (each an un-awaited spend-logging coroutine) with no counter and only a debug log. SDK callers using asyncio.run() per request and mixed sync/async processes rebind the queue's loop and silently lose spend rows and observability events. Drain the stale queue and move the pending tasks onto a fresh queue bound to the new loop, warn with the carried-over count, and keep flush()/join() honest since the queue is no longer thrown away. Adds a regression test that fills the queue before the loop change and asserts every task survives and still executes.
|
bugbot run |
Greptile SummaryThe PR preserves pending logging coroutines when a shared
Confidence Score: 5/5The PR appears safe to merge because the replacement queue retains pending logging work and the normal start path recreates its loop-bound worker state. The changed implementation transfers all bounded pending entries to the new queue, and production enqueue paths initialize a worker on the current loop before adding work; the regression test verifies the intended sequential-loop scenario.
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/logging_worker.py | Migrates queued logging work during event-loop rebinding while preserving the existing worker initialization flow; no actionable changed-code defect was established. |
| tests/test_litellm/litellm_core_utils/test_logging_worker.py | Adds focused regression coverage showing pending coroutines survive a transition between sequential event loops. |
Reviews (1): Last reviewed commit: "fix(logging_worker): carry queued tasks ..." | Re-trigger Greptile
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 12a34a1. Configure here.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
tin-berri
left a comment
There was a problem hiding this comment.
Approving.
Checked the three things that could have gone wrong with the early return:
- It doesn't skip worker startup —
start()calls_ensure_queue()and then sets up_sem/_worker_taskafter it, so the loop-change branch returning early still lands in the same initialization. put_nowaitcan't raiseQueueFullon the carry-over:carried_overis drained from a queue with the samemaxsize, so the new queue can hold all of it.contextvars.Contextisn't loop-bound, sotask["context"].run(...)on the new loop is fine — the carried tasks keep their original context.
The regression test is the right shape: it proves the coroutines actually execute on the second loop rather than just asserting qsize().
code-quality is red on recursive_detector flagging _flatten_form_field / _flatten_form_data_field in litellm/litellm_core_utils/llm_request_utils.py — untouched by this PR, so that's base drift, not you.
…erriAI#38265, BerriAI#37962, and BerriAI#37969 - test_custom_callback_input: audio redaction assertion expects None content (redaction leaves None untouched, gpt-audio-1.5 returns content=None) - local_testing conftest: drain GLOBAL_LOGGING_WORKER in isolate_litellm_state teardown so mocked-router tests stop leaking pending logging tasks into test_gcs_pub_sub - test_together_ai: tools is always a supported param now; only response_format is gated by function-calling support - test_keys: /team/new omits models instead of sending null (422), so the key's team really exists and auth no longer raises TeamNotFoundError - test_team_delete_member_add_race: per-test unique team and user ids so xdist workers sharing one Postgres stop deleting each other's team mid-race
TLDR
Problem this solves:
flush()reports success on a queue whose contents were discardedHow it solves it:
flush()/join()stay honest since nothing is thrown awayUser Flow
Before: a developer using the LiteLLM Python SDK who runs each request in its own
asyncio.run()loop loses the background spend/callback logging for earlier requestsasyncio.run()for request A; its success callback is queued for background logging, but the loop closes before the queue drainsasyncio.run()for request BRuntimeWarning: coroutine 'Logging.async_success_handler' was never awaitedto stderrAfter: the same two requests both log
asyncio.run()for request A; its success callback is queuedasyncio.run()for request BLoggingWorker: event loop changed; carried N pending logging task(s) onto the new loop, and nonever awaitedwarning appearsRelevant issues
Linear ticket
Resolves LIT-6028
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/test_litellm/<your_test_file>.py -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
Shared setup: a standalone script enqueues 5 spend-logging coroutines on one
asyncio.run()loop, then triggers a secondasyncio.run()loop (the exact "fresh loop per request" trigger from the ticket) and reports how many survived, how many ran, and how manynever awaitedwarnings fired.Before (85d5ac2)
python repro_logging_worker.pyAll 5 queued spend-logging coroutines are discarded on the loop change and Python reports each as
coroutine 'spend_log' was never awaited.After (12a34a1)
python repro_logging_worker.pyAll 5 tasks are carried onto the new loop and execute there, with zero
never awaitedwarnings and a single warning-level line reporting the carried-over count.Type
🐛 Bug Fix
Caveats (if any)
Final Attestation
Note
Medium Risk
Touches hot-path queue initialization for background logging; behavior change is scoped to event-loop changes, but incorrect carry-over could affect spend/callback delivery or shutdown semantics.
Overview
Fixes silent loss of background spend/observability logging when the global
LoggingWorkersees a newasyncioevent loop (e.g. repeatedasyncio.run()per request).On loop rebind,
_ensure_queueno longer discards the old queue. It drains pendingLoggingTaskentries via new_drain_pending, re-enqueues them on a fresh loop-bound queue, resets semaphore/worker state, and logs a warning with the carried-over count when anything was pending.Adds regression test
test_event_loop_change_carries_pending_tasks_over(LIT-6028) so queued coroutines still run after rebind instead of triggeringnever awaitedwarnings.Reviewed by Cursor Bugbot for commit 12a34a1. Bugbot is set up for automated code reviews on this repo. Configure here.