fix: give RedisCacheLayer an event-loop ownership contract (#1162) - #1179
fix: give RedisCacheLayer an event-loop ownership contract (#1162)#1179groupthinking wants to merge 1 commit into
Conversation
A redis.asyncio ConnectionPool caches connections whose transports are bound to the loop that opened them (redis/redis-py#3351), but RedisCacheLayer held one pool in self.redis_pool and reached it from six methods with no notion of which loop owned it. This module also builds an IntelligentCacheSystem singleton at import time, outside any loop, so one layer instance really is reachable from several loops in a single process. #1152 added a partial loop guard covering only the tagged-set() path and then reverted it, because guarding one of six call sites implies a safety property the layer does not have. This adds the layer-wide contract instead. The layer and its pool are now owned by exactly one event loop. Ownership is claimed the first time the pool is touched and verified at every call site that reaches it: connect, disconnect, get, set, delete, clear and invalidate_by_tags. - non-owning loop, owner alive -> raise CacheLoopOwnershipError - non-owning loop, owner closed -> the pool is unusable by anyone, so drop it, mark the layer disconnected, log a WARNING, and let the new loop re-claim via connect(). We deliberately do not await pool.disconnect() here: that would touch the very transports bound to the dead loop. - disconnect() is the supported clean handoff, with IntelligentCacheSystem.shutdown() as its facade counterpart to initialize(). Without that the recovery path documented on the class would only be reachable by indexing into system.layers. Every guard runs outside its method's `except Exception` block. Those blocks return None/False/0 on failure, so a guard placed inside one would disguise cross-loop transport misuse as an ordinary cache miss. The closed-owner-loop discard is a judgement call: #1162 left the recovery behaviour open, and a pure hard reject would permanently brick the import-time singleton for any process that calls asyncio.run() more than once. Discarding is strictly safer than the status quo, which silently reuses a dead pool, and it is loud. Tests: 17 added, covering rejection from a second live loop at all seven call sites, non-swallowing of the error through both the layer and the IntelligentCacheSystem facade, the closed-owner discard and its warning, the disconnect()/connect() handoff, and the import-time singleton. Verified non-vacuous: with the guard neutered, 12 of them fail (the other 5 exercise disconnect()/shutdown(), which do not exist without this fix). test_tag_write_semaphore_is_replaced_after_its_loop_closes now reconnects between loops. Its docstring already disclaimed cross-loop pool safety and cited this issue; under the contract the second loop must re-establish the pool before writing, which is exactly why the semaphore must be replaced. Closes #1162 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Agent Completion Truth Gate: NOT_APPLICABLEEvidence agrees. Machine-readable verdict{
"details": {},
"reasons": [],
"verdict": "not_applicable"
} |
There was a problem hiding this comment.
Pull request overview
Adds event-loop ownership enforcement for Redis connection pools to prevent unsafe cross-loop reuse.
Changes:
- Adds ownership validation and closed-loop recovery.
- Introduces
disconnect()and system-levelshutdown(). - Adds comprehensive ownership regression tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/youtube_extension/backend/services/intelligent_cache.py |
Implements pool ownership and lifecycle APIs. |
tests/unit/test_intelligent_cache.py |
Tests rejection, recovery, handoff, and singleton behavior. |
| if owner is None: | ||
| # A pool assigned without going through connect() is claimed by the | ||
| # first loop that touches it. With no pool there is nothing to own. | ||
| if self.redis_pool is not None: | ||
| self._pool_loop = loop |
| pool = self.redis_pool | ||
| self.redis_pool = None | ||
| self._connected = False | ||
| self._pool_loop = None | ||
|
|
||
| if pool is None: | ||
| return | ||
|
|
||
| try: | ||
| await pool.disconnect() | ||
| except Exception as e: # pragma: no cover - defensive | ||
| logger.warning(f"Redis pool disconnect error: {e}") |
| Redis fault, so it is deliberately *not* folded into the ``None``/``False``/ | ||
| ``0`` fallbacks that the layer returns for connection failures -- a silent | ||
| fallback here would hide cross-loop transport misuse behind what looks like | ||
| an ordinary cache miss. |
Automated remediation review — not a sign-offIndependent read of the diff (contract + all 7 call sites + the 17 tests). No blocking findings. This is an automated review; it does not stand in for the human sign-off you asked for on the recovery choice. Contract verification
On your judgement call (closed-owner path → "discard and release")Endorsed. A pure hard-reject would permanently brick the import-time One caveat worth stating plainly so the sign-off is informed: the discarded pool's connections leak their sockets until GC, because they can't be closed from a foreign loop. That leak is unavoidable on the crash path (owner died without CI realityAll 15 Recommendation: ready to merge on your sign-off of the recovery choice above. Not auto-merging: protected branch + you explicitly requested human review. Generated by Claude Code |
groupthinking
left a comment
groupthinking
left a comment
There was a problem hiding this comment.
Review — event-loop ownership contract (#1162)
Reviewed the full diff and traced _require_pool_loop() through all seven call sites. The contract is correct and the enforcement is placed where the PR body claims. Findings below.
What holds up
- No interleaving window in
connect().self.redis_pool = from_url(...)andself._pool_loop = loophave noawaitbetween them, so a pool can never be visible with_pool_loopstillNoneinsideconnect(). Ownership is claimed atomically with pool creation. - Guard precedes the
if not self._connectedearly-out onget/set/delete/clear/invalidate_by_tags. Confirmed in the diff — this is the load-bearing invariant, and aCacheLoopOwnershipErrorcannot be swallowed into aNone/False/0fallback. - Closed-owner path is internally consistent. It nils
redis_pool/_connected/_pool_loopand deliberately does notawait pool.disconnect()on the dead loop. Downstream:get()then falls through to a clean miss, anddisconnect()reads the now-Nonepool and no-ops — so the same reset can't double-free. - Pool-less layer claims nothing (
owner is None and redis_pool is None→ return without setting_pool_loop), so a read on an unconnected layer can't lock out a laterconnect()from another loop. The parametrized live-foreign-loop tests plus the neutered-guard non-vacuity run (12 fail without the guard) make this convincing rather than asserted.
Non-blocking (pre-existing, out of this PR's declared scope)
connect()re-entry on the owning loop still leaks the previous pool. Callingconnect()twice on the same loop overwritesself.redis_poolwith a freshfrom_url(...)without awaiting the old pool'sdisconnect(). This behavior predates the PR and isn't in scope here, but now thatdisconnect()exists as the clean handoff, a future hardening could haveconnect()early-return (orawait self.disconnect()first) when already connected on the same loop. Flagging only so it's on record — not a reason to hold this change.
On the sign-off you flagged
The "discard and release" choice on the closed-owner path is, in my read, the right call: it is strictly safer than the status quo (silently reusing a dead pool), it is loud, and the socket-leak caveat is spelled out in the WARNING. That said — you explicitly asked for human sign-off on this behavioural change, and converting a silent degradation into a raised CacheLoopOwnershipError is exactly the kind of blast-radius decision an unattended reviewer shouldn't rubber-stamp. Recording this as COMMENT (technically LGTM), not APPROVE: the sign-off is a human's to give, and the merge itself is a human gate (draft PR → protected main).
Merge readiness
The four posted statuses (CodeRabbit, Vercel ×2, agent-completion/truth-gate) are green. mergeable_state is unstable, consistent with the two repo-wide red check-runs you pre-disclosed (Agent completion enforcement #1160, gitleaks FP #1165) rather than anything in this diff — a human should confirm those two are the only remaining reds before merging.
Generated by Claude Code
|
@coderabbitai review Exact-head independent review requested for |
|
✅ Action performedReview finished.
|
Closing — this work is already the tip of
|
Canonical issue
Closes #1162
Outcome
RedisCacheLayernow has an explicit event-loop ownership contract, so cross-loop misuse of its connection pool fails loudly at the call site instead of silently degrading into a cache miss.The bug. A
redis.asyncio.ConnectionPoolcaches connections whose transports are bound to the loop that opened them (redis/redis-py#3351).RedisCacheLayerheld one pool inself.redis_pooland reached it from six methods with no notion of which loop owned it.This is not theoretical for this module, because it builds an
IntelligentCacheSystemsingleton at import time, outside any running loop:so a single layer instance genuinely is reachable from several loops in one process — anything calling
asyncio.run()more than once (the repo's own benchmark scripts do) reaches it.Why it was invisible. Every pool method wraps its body in
except Exceptionand returnsNone/False/0. A cross-loop transport error therefore surfaced as an ordinary cache miss. The cache appeared to work, just with a mysteriously poor hit rate.The contract. The layer and its pool are owned by exactly one event loop. Ownership is claimed the first time the pool is touched and verified at every call site that reaches it:
connect,disconnect(new),get,set,delete,clear,invalidate_by_tags— seven in total._require_pool_loop()resolves four cases:None→ claim it, but only if a pool actually exists. A pool-less layer owns nothing; otherwise aget()on an unconnected layer would lock out a laterconnect()from a different loop.CacheLoopOwnershipError.connect().We deliberately do not
await pool.disconnect()on that last path — that would touch the very transports bound to the dead loop. The warning says so and points atdisconnect()as the clean path.Load-bearing invariant. Every guard runs outside its method's
except Exceptionblock and before theif not self._connected: return <fallback>early-out. A guard inside thetrywould disguise cross-loop misuse as a cache miss — reintroducing the exact failure mode this PR exists to remove.CacheLoopOwnershipErroris a programming error and is deliberately not folded into the connection-failure fallbacks. Placing it before the_connectedcheck also avoids a bug present in my first draft: the closed-owner path resets_connected = False, so a guard placed after the early-out would let the method sail on and use aNonepool.New handoff API.
disconnect()is the supported clean handoff, withIntelligentCacheSystem.shutdown()as its facade counterpart toinitialize(). Withoutshutdown()the recovery path documented on the class would only be reachable by indexing intosystem.layers[...], since no consumer holds aRedisCacheLayerdirectly.Background. #1152 added a loop guard covering only the tagged-
set()path, then reverted it on the grounds that guarding one of six call sites advertises a safety property the layer does not have. #1162 was carved out to add the layer-wide contract instead. This PR is that follow-up.Scope
src/youtube_extension/backend/services/intelligent_cache.py; the newRedisCacheLayer.disconnect()andIntelligentCacheSystem.shutdown(); regression tests.Risk
CacheLoopOwnershipErrorinstead of aNone/Falsefallback. That is the intended point of the change, but it converts a silent misbehaviour into a loud one, so a latent misuse anywhere in the codebase would surface as a new exception rather than a degraded cache. I found no such caller: all production entry points drive the singleton from one loop. Everything else is additive — the single-loop path is byte-for-byte the same code as before, just preceded by a check that returns immediately.Judgement call requiring reviewer sign-off
#1162 left the recovery behaviour open between three options. I chose "discard and release" on the closed-owner path.
A pure hard reject with no escape hatch would permanently brick the import-time singleton for any process that calls
asyncio.run()more than once. Discarding is strictly safer than the status quo — which silently reuses a dead pool — and it is loud, with the socket-leak caveat spelled out in the WARNING. But it is a choice, and I would rather it get explicit sign-off than slide through unexamined.Verification
All checks below run against head
b60beb927945c7844157435a2c631efd26f1c90d.Focused tests —
tests/unit/test_intelligent_cache.py,tests/unit/test_intelligent_cache_models.py,tests/unit/test_comprehensive_benchmarking.py:main(ad7e2c10, baseline)Same single pre-existing collection error in both runs (
test_comprehensive_benchmarking.pyimportspsutil, absent from the local venv), so the delta is clean: +17 tests, zero regressions. Baseline was captured by stashing this branch's diff and re-running the identical command, not from memory.17 tests added, covering:
IntelligentCacheSystemfacadecaplog)connect()succeeding on a new loop after the owner closeddisconnect()→connect()handoff, anddisconnect()with no pool being a no-opfinallyIntelligentCacheSystem.shutdown()releasing the layerThe foreign loop is a real second event loop on a daemon thread (
run_forever()+run_coroutine_threadsafe), not a mock. A mock would not reproduce transport binding and the test would prove nothing.Non-vacuity proof. Guard neutered with an early
return asyncio.get_running_loop(), suite re-run:Failures: all 7
test_live_foreign_loop_is_rejectedparams, plusnot_swallowed,leaves_the_layer_untouched,closed_owner_loop_releases_the_pool,import_time_singleton,facade_does_not_swallow. The 5 that still pass exercisedisconnect()/shutdown(), which do not exist in pre-fix code at all. Source restored afterwards and the restoration verified.Lint.
ruff checkclean on the changed source file. The singleI001in the test file was verified pre-existing onmain(git show HEAD:… | ruff check --stdin-filename) and left untouched. Black was deliberately not run: the file is not black-formatted atmain, so running it would bury this diff in unrelated reflowing.One existing test modified.
test_tag_write_semaphore_is_replaced_after_its_loop_closesnowconnect()s on the second loop. Its docstring already disclaimed cross-loop pool safety and cited #1162 by name; under the contract the second loop must re-establish the pool before writing, which is precisely why the semaphore must be replaced. The assertion it makes is unchanged.ruffclean on changed sourceProduction evidence
Not applicable, and deliberately so. This is a backend Python correctness fix with no user-facing surface: no Vercel preview exercises it, and there is no runtime deployment evidence to attach.
More importantly, the bug is unobservable in production telemetry by construction — that is the whole finding. Every affected method swallows the failure into a
None/False/0fallback, so a cross-loop violation renders as a cache miss, not an error. There is no log line, metric, or trace onmainthat would distinguish "the pool is bound to a dead loop" from "this key genuinely was not cached". Waiting for production evidence would mean waiting for a signal the current code cannot emit.The evidence that is available is therefore the executable kind, above: a real second event loop reproducing the violation, and a neutered-guard run proving those tests fail without the fix.
Post-merge, the change is self-evidencing. The
CacheLoopOwnershipErrorand the closed-owner WARNING are the first observability this failure mode has ever had. If either appears in logs it is a real cross-loop misuse that was previously invisible.Agent handoff
Note on the two repo-wide red gates
Agent completion enforcementcurrently fails on every PR in this repo (tracked in #1160, remediation in PR #1151), andgitleaks (working tree)has a known false positive (#1165, #1142). Neither is caused by this change; both are red on unrelated PRs at the same time. Flagging so they are not misread as breakage from this branch.