fix(caching): store task references in LLMClientCache._remove_key#22143
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryFixes a bug where
Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| litellm/caching/llm_caching_handler.py | Stores asyncio task references in a class-level _background_tasks set with add_done_callback cleanup, following the recommended asyncio pattern to prevent GC from destroying unawaited coroutines. Clean, minimal fix. |
| tests/test_litellm/caching/test_llm_caching_handler.py | Adds 6 mock-only tests covering async/sync client close, eviction, no-event-loop safety, and background task cleanup. No network calls — compliant with mock-only test requirement. |
Sequence Diagram
sequenceDiagram
participant Caller
participant LLMClientCache
participant EventLoop
participant _background_tasks as _background_tasks Set
participant AsyncClient
Caller->>LLMClientCache: _remove_key(key)
LLMClientCache->>LLMClientCache: value = cache_dict.get(key)
LLMClientCache->>LLMClientCache: super()._remove_key(key)
alt value has async close_fn
LLMClientCache->>EventLoop: create_task(close_fn())
EventLoop-->>LLMClientCache: task
LLMClientCache->>_background_tasks: add(task)
LLMClientCache->>EventLoop: task.add_done_callback(discard)
EventLoop->>AsyncClient: await close()
AsyncClient-->>EventLoop: done
EventLoop->>_background_tasks: discard(task)
else value has sync close_fn
LLMClientCache->>AsyncClient: close()
end
Last reviewed commit: ba849c1
| # Background tasks must be stored to prevent garbage collection, which would | ||
| # trigger "coroutine was never awaited" warnings. See: | ||
| # https://docs.python.org/3/library/asyncio-task.html#creating-tasks | ||
| _background_tasks: Set[asyncio.Task] = set() |
There was a problem hiding this comment.
Class-level mutable shared state
_background_tasks is a class variable, meaning it's shared across all LLMClientCache instances. While this works fine in production (where there's typically a single global instance), it could cause subtle issues if multiple instances are created — e.g., flush_cache() on one instance won't clear tasks created by another. Consider making this an instance variable initialized in __init__ instead:
| _background_tasks: Set[asyncio.Task] = set() | |
| _background_tasks: Set[asyncio.Task] | |
| def __init__(self, *args, **kwargs): | |
| super().__init__(*args, **kwargs) | |
| self._background_tasks: Set[asyncio.Task] = set() |
There was a problem hiding this comment.
Thanks for the suggestion! The shared task registry is intentional — create_task() needs a strong reference to prevent GC from triggering "coroutine was never awaited" warnings (as recommended in the asyncio docs). I added a comment to make the class-level scope explicit.
I avoided adding an __init__ override to keep this fix minimal, since LLMClientCache currently relies on the base class init path. If maintainers would prefer instance-level tracking or a module-level registry instead, I'm happy to adjust.
843df9f to
ba849c1
Compare
🚅 Shin's PR Review1. Does this PR fix the issue it describes? 2. Has this issue already been solved elsewhere? 3. Are there other PRs addressing the same problem? 4. Other issues this could close? Summary: Well-structured fix with 6 comprehensive tests covering sync/async clients, eviction scenarios, and cleanup verification. The implementation follows Python's official asyncio documentation. LGTM. 🚅 |
|
The CI lint failure ( git rebase main
git push --force-with-lease |
|
The MCP e2e test failures ( |
|
The passthrough test failures ( |
|
All 12 test failures (health check git fetch origin main
git rebase origin/main
git push --force-with-leaseThis single rebase will resolve all CI failures: the lint error, MCP tests, passthrough tests, and these 12 failures. |
|
The 11 failures (guardrail endpoints, key management) also pass on current main. Same root cause — branch needs rebasing. All CI failures on this PR are due to the branch being behind main, not due to the caching changes. |
|
The UI build failure (merge conflict markers in |
…ove_key to prevent unawaited coroutine warnings Fixes BerriAI#22128
ba849c1 to
fb72979
Compare
Summary
LLMClientCache._remove_keycallscreate_task(close_fn())when evicting async HTTP clients, but discards the task reference. Python's GC destroys the unreferenced task before it runs, producingRuntimeWarning: coroutine 'AsyncHTTPHandler.close' was never awaitedon every cache eviction.Store task references in a class-level
_background_tasksset and useadd_done_callbackto clean up completed tasks, following the recommended pattern from the asyncio docs.Fixes #22128
Type
🐛 Bug Fix
Testing
Added 6 tests in
tests/test_litellm/caching/test_llm_caching_handler.py:.close()is actually called.close()is calledset_cacheoverflow closes async clients without warnings_remove_keydoesn't raise when no event loop is running