Skip to content

RedisCacheLayer has no event-loop ownership contract across its six redis_pool call sites #1162

Description

@groupthinking

A redis.asyncio.ConnectionPool caches connections whose transports are bound to the event loop that opened them. RedisCacheLayer holds one pool in self.redis_pool and uses it from six call sites in src/youtube_extension/backend/services/intelligent_cache.py:

  • connect() (~L346)
  • get() (~L364)
  • set() (~L403)
  • delete() (~L467)
  • clear() (~L490)
  • invalidate_by_tags() (~L515)

None of them assert which loop owns the pool. The module also builds an IntelligentCacheSystem() singleton at import time (~L721), outside any loop, so the layer can be reached from more than one loop in the same process.

Problem

There is no declared, enforced contract for which event loop may use a layer. Reusing a pool from a second loop can surface as connection errors or cross-loop transport misuse. Upstream context: redis/redis-py#3351.

Why not fixed in #1152

#1152 replaced a sequential sadd loop with a bounded gather and added a per-instance tag-write semaphore for the aggregate-budget regression that change introduced. A loop-ownership guard was briefly added there but covered only the tagged-set() path, i.e. one of six. A partial guard implies a safety property the layer does not have, so it was reverted and the concern split out here.

Options (pick one and apply to every pool call site)

  1. Reject use from any loop other than the owning one. Smallest safe contract.
  2. One RedisCacheLayer + pool per event loop.
  3. Allow sequential reuse only if the pool is closed on its owner loop before that loop ends, then recreated for the next loop.

Acceptance criteria

  • One contract chosen and documented on RedisCacheLayer.
  • Enforced uniformly across all six call sites, not just set().
  • Regression tests covering the chosen contract, including the import-time singleton reached from a second loop.
  • Existing suite still green (pytest gives each async test a fresh loop, so option 1 needs a per-loop construction story for tests).

Scope

src/youtube_extension/backend/services/intelligent_cache.py, tests/unit/test_intelligent_cache.py.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions