Guard against re-entrant ValkeyCache context use - #1080
pctablet505 wants to merge 2 commits into
Conversation
Nesting the same ValkeyCache instance in multiple `async with` blocks silently replaced `self.client` with a new GlideClient on each entry, so the outer block's exit closed the inner client (or double-closed it) while the original connection could leak. `__aenter__` now raises RuntimeError if a client is already active, and `__aexit__` resets `self.client` to None after closing so the instance can still be reused sequentially. Fixes aio-libs#1023.
There was a problem hiding this comment.
Please use create_autospec(..., spec_set=True, instance=True) and patch(..., autospec=True, spec_set=True).
|
|
||
| async def __aexit__(self, *args, **kwargs) -> None: | ||
| await self.client.close() | ||
| if self.client is not None: |
There was a problem hiding this comment.
This should be an assert, it should never be false.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1080 +/- ##
==========================================
- Coverage 98.85% 98.83% -0.02%
==========================================
Files 32 32
Lines 3579 3607 +28
Branches 125 129 +4
==========================================
+ Hits 3538 3565 +27
Misses 41 41
- Partials 0 1 +1
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Build the fake client with create_autospec(GlideClient, spec_set=True, instance=True) and patch GlideClient.create with autospec=True, spec_set=True, so the new tests fail if ValkeyCache ever touches an attribute the real client does not have. __aexit__ only runs when __aenter__ has already succeeded, so self.client can never be None there; assert it instead of branching.
Confidence Score: 4/5Merge is blocked because concurrent use of one ValkeyCache instance can leak a connection and fail cleanup. A deterministic asynchronous reproduction confirmed one blocking lifecycle failure. No security issue was found. Files Needing Attention: aiocache/backends/valkey.py
 T-Rex LogsWhat T-Rex did
Reviews (1): Last reviewed commit: "Use autospec mocks and assert the client..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| async def __aenter__(self) -> Self: | ||
| if self.client is not None: | ||
| raise RuntimeError( | ||
| "ValkeyCache context is not reentrant. " | ||
| "Nesting the same instance in multiple 'async with' blocks " | ||
| "is not supported." | ||
| ) | ||
| self.client = await GlideClient.create(self.config) |
There was a problem hiding this comment.
Two tasks can both pass the self.client is None check before either GlideClient.create() call completes. Each creates a different client, but the later assignment replaces the first; when both contexts exit, only the later client is closed, the first client is leaked, and the other exit raises AssertionError. Reserve or serialize lifecycle entry before awaiting client creation so an overlapping entry is rejected.
Knowledge Base Used: Valkey cache backend
Artifacts
Concurrent Valkey entry reproduction
- Deterministic mocked asynchronous reproduction that forces two context entries to overlap before client creation completes.
Concurrent Valkey entry output
- The output shows two created clients, one unclosed client, and one failing exit, confirming unreliable concurrent cleanup.
2 participants
What do these changes do?
ValkeyCache.__aenter__always created a brand newGlideClientandoverwrote
self.client, even if the instance was already inside anasync withblock. Nesting the same instance (e.g.async with cache: async with cache: ...) silently replaced the connection, so the innerblock's exit closed the newer client while the outer block's exit then
closed it again (or the original connection leaked, never being closed).
__aenter__now raisesRuntimeErrorif a client is already active,and
__aexit__clearsself.clientback toNoneafter closing sothe same instance can still be used again sequentially (just not
nested).
Are there changes in behavior for the user?
Nesting the same
ValkeyCacheinstance in overlappingasync withblocks now raises
RuntimeErrorinstead of silently leaking/double-closingconnections. Normal (non-nested) usage is unaffected.
Related issue number
Fixes #1023
Checklist