[Grivn]

Description of change

Fixes #4443.

Draining a cache entry's checked-out owner can consume another command's reply and leave that request waiting. This change coordinates idle-owner invalidation drains with checkout through a per-connection lock, including when pools share a cache. Checkout waits outside the pool lock, so unrelated connections can still be borrowed or returned.

A busy owner can share a cached value for 100 ms after its own invalidation check or cache fill. Peers never drain that socket or extend the window. Pending wire replies, PubSub, blocking commands and packed commands require a fresh read. Idle owners drain invalidations before a hit, and the entry must still be the same object afterward. Checkout and idle AUTH revoke the previous window before becoming busy; checkout processes pending invalidations before opening a new one. This prevents invalidations received while idle from being hidden by a quick reuse, while retaining concurrent cache hits when the value is unchanged.

Ownership also covers idle AUTH, release and maintenance. Cached release listeners run outside the pool lock while the connection remains active and unavailable for checkout. Reentrant release is a no-op even if a listener changes the Sentinel master; final cleanup rechecks ownership and reclaims a demoted connection's slot. Rejected cached releases finish disconnection, clear the ownership flag and release their locks before invoking maintenance completion listeners, in both pool implementations. Idle AUTH preserves newer deferred tokens and releases its claim after socket work, including on errors and cancellation. Free-only disconnect joins drains outside the pool lock and retires a selected idle socket before reuse. Blocking checkout releases the MOVING lock before waiting for a queue slot, preserving custom queue adapters without requiring a mutex attribute.

Idle-owner polls defer maintenance changes to the socket timeout until polling finishes, while updating the parser restore timeout immediately. The reservation and timeout updates share the maintenance state lock, which is released across socket I/O and push handlers. This keeps a concurrent MOVING update from turning a nonblocking poll into a blocking read, including with an unlimited relaxed timeout. Errors and cancellation still release the reservation and apply the latest timeout.

A same-thread cyclic-GC finalizer can close a pool while an idle owner is parsing invalidations. That reentrant close now marks the socket for reconnect and defers teardown until parsing and timeout restoration finish, avoiding both a self-deadlock and a parser being closed mid-read. Other threads still wait for the active drain to complete.

Sentinel discovery during AUTH can request free-socket teardown while AUTH still owns the pool lock. Same-thread cached disconnects wait until the outer AUTH scope releases it. The same deferral covers finalizers during blocking release, free-connection maintenance and the normal pool’s final release cleanup. A release listener may mark the connection for reconnect after the initial check; nested teardown from that final disconnect runs after the pool lock is released, so a peer’s idle drain can finish timeout restoration. The deferral also covers connection snapshot allocation and fallback release of already-rejected cached Sentinel connections. GC finalizers in these scopes queue nested teardown until the pool lock is released. A separate thread-local shutdown guard handles finalizers triggered by disconnect itself, preserving the original non-reentrant owner lock and checkout behavior.

Maintenance events retain emission order but invoke listeners after enclosing connection, pool, handler and topology locks release. If a listener closes its pool and then cancels, ready completion events and other deferred sources still drain before the original cancellation propagates. The cancelled listener is not retried. This lets listeners close pools without deadlocking a concurrent drain. SMIGRATED topology actions run after releasing idle ownership, while connection-local handling and invalidations stay inline. Removed-node cleanup completes deferred disconnect even after parsing errors or cancellation. Nested drains preserve their liveness checks and handler replacement, and maintenance queue allocation stays outside its lock so GC cleanup can enqueue completion. Forked children avoid inherited owner locks and reset event dispatch state. Default-cache invalidation holds its existing lock across holder lookup and removal.

Validation on 342579e (parent 887b027, which includes master ba4c0eb):

• Default stages: 11,104 passed, 27 failed, 2,444 skipped and 8 non-strict xpasses. All 27 failures match the GEO exact-floating-point assertions from the validated 887b027 run on the same Redis 8.10.3 ARM64 services, with parent source hashes verified. No assertions or tolerances changed. Ruff check/format and Vulture pass.
• 2,952 focused cases pass, with 2 skips, covering both parsers and pools, tracking modes, sync/async connection pools, Sentinel, maintenance, fork and MultiDB events.
• Eight new regressions exercise forced and automatic GC during the normal pool's disconnect snapshot and an already-rejected cached Sentinel release. All eight deadlock on published 887b027; all 38 pool-reentry cases pass after repair. They use real Redis connections and a separate pool sharing the cache, and verify returned values, ownership cleanup, slot accounting and pool reuse.
• Python 3.11.15 and 3.12.3 each reproduce four deadlocks on 887b027 and pass all four after-fix probes. Async has no client-side cache proxy or synchronous pool-close finalizer; its related tests remain covered. The full protocol/response matrix, uvloop and external scenario suites were not run. Hosted CI remains subject to maintainer approval.

Prior idle-owner cache-hit check on 887b027 (not rerun for this shutdown/rejected-release repair): five interleaved 1.5-second samples per version, pool and reader count, without simultaneous tests. A released owner in a separate pool warms the shared cache, so public GET includes checkout, peer invalidation polling and release. The merged control is the exact candidate with only the new final-release deferral reverted; that comparison isolates this repair. Comparison with published 7db6bd6 also includes upstream changes. Medians in GET/s:

Pool	Readers	Published 7db6bd6	Merged control	887b027	Repair vs control
ConnectionPool	1	63,767	64,921	59,599	-8.2%
ConnectionPool	2	49,836	50,002	47,027	-5.9%
ConnectionPool	8	16,465	16,493	16,214	-1.7%
ConnectionPool	16	17,080	17,093	16,958	-0.8%
BlockingConnectionPool	1	58,634	58,773	58,129	-1.1%
BlockingConnectionPool	2	46,198	45,758	45,772	+0.0%
BlockingConnectionPool	8	16,211	16,275	16,235	-0.2%
BlockingConnectionPool	16	16,537	16,805	16,225	-3.4%

All 120 samples returned the expected value, ended their threads and made zero server GETs after warming. The table retains measured overhead and scheduling variation. These short samples do not establish zero overhead, allocation cost or a general zero miss rate. Shutdown, AUTH and cancellation interleavings are covered by deterministic regressions.

Historical upstream comparison, measured on e93b9c8 with a shared pool and Redis 8.10.2; these numbers are not measurements of the current commit:

Warmed-cache throughput: five interleaved 1.5-second samples per revision, pool and reader count. The master-merged comparison isolates the PR from upstream changes; the merged-before column isolates the historical deferred-disconnect repair. Medians in GET/s:

Pool	Readers	Master 38233c0	Merged before repair 2eb32d3	e93b9c8
ConnectionPool	1	86,385	87,076	86,751
ConnectionPool	2	38,954	63,170	62,599
ConnectionPool	8	18,103	27,341	27,164
ConnectionPool	16	18,096	33,092	33,138
BlockingConnectionPool	1	82,899	85,070	85,693
BlockingConnectionPool	2	37,721	62,181	62,323
BlockingConnectionPool	8	29,468	26,838	27,126
BlockingConnectionPool	16	29,521	32,601	32,890

All 120 samples made zero server GETs after warming. Single-reader medians relative to master are +0.4% for ConnectionPool and +3.4% for BlockingConnectionPool. Relative to the master merge before this repair, they are -0.4% and +0.7%. BlockingConnectionPool at 8 readers remains 7.9% below master (and +1.1% versus merged-before); the table retains this result. These short samples include scheduling variance and do not establish zero overhead or a zero miss rate outside this workload.

The AUTH/MOVING lock inversion reported in the review thread also reproduces on the uncached merge base 3a6c462 and the tested master 38233c0. It is left for the separate issue requested by the maintainer; this repair does not change that AUTH path.

Default suites for this commit used Python 3.13.18, pytest 9.1.1 and the plain parser on Ubuntu 24.04 with repository-default Redis 8.10.3 ARM64 services (modules, TLS and a six-node cluster). Focused parser coverage and the prior 887b027 throughput comparison used hiredis 3.5.0. The older e93b9c8 upstream comparison used Redis 8.10.2. The current GEO failures were compared to the complete validated 887b027 default-suite record rather than rerunning those unchanged failures separately.

Pull Request check-list

• Do tests and lints pass with this change? (Focused tests and linters pass; default suites retain the 27 reproduced baseline GEO failures above.)
• Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
• Is the new or changed code fully tested? (Focused regressions and all default stages ran; baseline failures and broader matrix limits are listed above.)
• Is a documentation update included (if this change modifies existing APIs, or introduces new ones)? (No public method signatures change.)
• Is there an example added to the examples folder (if applicable)? (Not applicable.)

Prepared with AI assistance; the checks listed above were run locally.

---

Note

High Risk
Large concurrency changes across client-side caching, connection pools, and maintenance/topology handling; incorrect locking could cause stale cache hits, reply mix-ups, or deadlocks under maintenance and Sentinel/cluster events.

Overview
Fixes client-side cache correctness and deadlocks by serializing socket use and invalidation drains on CacheProxyConnection with a per-connection checkout lock, a short busy grace window for peer hits, and cache entries that own the proxy instead of the wrapped connection.

Maintenance and pool teardown now defer event listeners and cluster topology handlers until connection/pool/topology locks are released, with ordered per-source queues (including fork and cancellation behavior). Idle-owner drains can postpone socket timeout updates and flush shared cache on deferred OSS notifications. Pools get cached-specific release/disconnect/re-auth paths that join idle drains outside the pool lock and avoid reentrant GC/Sentinel deadlocks.

Also makes default-cache invalidation deletion atomic under the cache lock and clears deferred re-auth tokens only after successful idle AUTH (sync and async).

^{Reviewed by Cursor Bugbot for commit 342579e. Bugbot is set up for automated code reviews on this repo. Configure here.}

[chatgpt-codex-connector]

Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-11T03:27:13.963403Z	342579e	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[petyaslavova]

Hey @Grivn, thank you for your contribution!

The fix is directionally right. Never draining a checked-out owner closes the reply theft from #4443, and it also covers owners held by PubSub or a blocking command such as BLPOP key 0. Using the owner's own pool lock also fixes the cluster case, where the cache is shared across node pools.

However, I measured a regression that blocks the merge. With two or more threads reading through one client, the cache stops serving hits. Each miss moves ownership to the requesting connection, and the next reader from another thread finds that owner checked out. With no writes at all, 2 readers reached a 99% miss ratio, and 8-16 readers reached 100% on both pool classes, with throughput down 30-55% compared with master. Single-threaded use is unaffected.

Please change the busy-owner path so concurrent readers still get hits. For example, serve the cached value without draining when the owner is checked out, and keep the fresh read only where staleness could be unbounded. Please also add a test that asserts concurrent readers keep hitting the cache. Optionally, the _lock acquire in BlockingConnectionPool.release is not needed just to clear _in_use.

[Grivn]

Thanks @petyaslavova. I reproduced this with a warmed key and no writes: two-second local runs with 2 and 8 readers sent 93–98% of reads to Redis on this branch, versus none on the unchanged base, for both pools. My earlier stress checks covered wrong replies and hangs but missed cache-hit assertions.

I'll revise the busy-owner path and add a concurrent-reader hit test alongside the reply-preservation tests. Serving every busy-owner entry would leave PubSub and indefinite blocking commands without a freshness bound, so those cases still need a fallback. I agree this needs fixing before merge.

[Grivn]

Updated in 56d0728, including the merge with current master.

Busy owners can now serve hits without touching their sockets or moving cache ownership. Sharing has a 100 ms limit since the owner's last invalidation check or cache-fill completion; another reader's hit cannot extend it. This also bounds stale reads when a checked-out connection is left idle. Pending wire replies, PubSub, blocking commands and packed commands keep the fresh-read fallback.

Added concurrent-hit assertions for 2 and 8 readers, plus expiry, PubSub and blocking-command coverage. The original reply-preservation tests are unchanged. With the final commit, 1/2/8/16-reader runs on both pools made zero extra GET requests after warming the key (470,005 reads total). The 12 read/write stress cases also passed.

All three default test stages pass: 8,683 passed, 929 skipped and 8 non-strict xpasses. Ruff and Vulture pass too.

[cursor]

Stale Bugbot comment from a previous run.

[petyaslavova]

Thanks for the update. I re-ran my benchmark on b229c11. Concurrent readers now get cache hits on both pool classes, with zero server GETs on a warmed key at 2, 8 and 16 readers. The #4443 repro also completes without hangs or wrong replies.

One regression remains. BlockingConnectionPool throughput with 8-16 readers is 14-17% below master (about 22.5k vs 26.5k ops/s), while ConnectionPool is faster. Most of the cost comes from the new _lock acquire around _in_use = True in get_connection. Every checkout now waits on the lock that peers hold while draining an idle owner. Please reduce that cost, for example with a per-connection lock shared by the drain and checkout instead of the pool-wide _lock. Please also drop the _lock acquire in BlockingConnectionPool.release, since nothing touches the socket at that point.

A small one: the _BUSY_CACHE_GRACE_SECONDS comment says active readers renew the window, but only the owner's own commands do. Peer hits do not.

[Grivn]

@petyaslavova Fixed in 53b86ce. Checkout and idle-owner drains now share a per-connection lock. Both pools release the pool lock before waiting for a drain, and BlockingConnectionPool.release no longer acquires _lock. The grace-window comment now says only the owner renews it.

Five interleaved 1.5-second runs per case on macOS arm64, Python 3.13.12 and Redis 8.10.2 gave these median warmed-key rates:

Blocking-pool readers	b229c11	53b86ce	master afe7fc7
8	29.1k/s	46.2k/s	33.2k/s
16	29.4k/s	46.9k/s	33.5k/s

All read-only cases had zero extra GETs. Added regressions show unrelated checkout/release can proceed during a drain while checkout of the owner still waits, including shared caches. The default test stages passed 9,216 tests; Ruff, Vulture and all 12 read/write stress cases passed. These are local results; upstream CI still needs to run.

[cursor]

Stale Bugbot comment from a previous run.

[petyaslavova]

Thanks for the update. All three points from my last review are addressed: checkout and the drain share a per-connection lock, BlockingConnectionPool.release no longer takes _lock, and the grace-window comment is accurate.

One regression remains, which Bugbot also flagged. While the drain held the pool lock, it could not overlap the pool paths that use an idle connection's socket under _lock. Now re_auth_callback can send AUTH while a peer drains the same connection, and so can re_auth() from AfterConnectionReleasedEvent, because release clears _in_use before dispatching it. The drain can consume the AUTH reply and leave read_response() blocked while _lock is held, which stalls the pool. disconnect_free_connections on MOVING has the same overlap. A streaming credential provider or maintenance notifications, combined with concurrent cache readers, is enough to hit this.

Please have these paths claim the connection the same way checkout does: set _in_use under _in_use_lock before the socket I/O, without holding that lock during it. In release, clear _in_use only after the event dispatch. Please also add a regression test where a drain overlaps re_auth_callback on an idle owner.

[Grivn]

@petyaslavova Agreed: release must keep _in_use set through the event, and idle AUTH/disconnect need ownership before touching the socket.

I added an overlap regression and checked a local variant that sets _in_use under the connection lock, then releases that lock before AUTH. There is also a lock-order issue to handle: re_auth_callback still holds the pool lock while waiting for an existing peer drain, and that drain can dispatch a maintenance notification needing the pool lock. All 12 parser/tracking/shared-pool cases caught that wait. The MOVING path also enters disconnect_free_connections with outer locks held.

The repair needs to reserve idle connections against checkout before waiting outside the pool lock. I haven't pushed this variant; the reply-safety and lock-order regressions both need to pass first.

[petyaslavova]

Thanks for catching this. Note that the branch is still at 53b86ce, so none of the changes from my last review are on the PR yet. You're right that my suggestion would deadlock: re_auth_callback and the MOVING path (run_proactive_reconnect -> disconnect_free_connections) run with the pool lock held, and an idle-owner drain that holds _in_use_lock can dispatch a MOVING notification whose handler takes that same pool lock.

I don't think these paths need to wait for the drain, though. Under the pool lock, try _in_use_lock.acquire(blocking=False). If it succeeds, set _in_use = True, release the lock, do the socket I/O and clear _in_use afterwards. If a drain holds it, fall back to the deferred handling that in-use connections already get: set_re_auth_token() for re-auth and mark_for_reconnect() for MOVING. Then nothing blocks under the pool lock, and the maintenance handler needs no restructuring, which reserving connections outside the pool lock would require for the MOVING path.

When you push, please also make release clear _in_use only after the event dispatch, and add regressions for a drain overlapping re_auth_callback on an idle owner, and for a drain that dispatches MOVING while disconnect_free_connections runs.

[Grivn]

@petyaslavova Pushed 1afc08d. Idle AUTH and MOVING now try a nonblocking ownership claim and defer through set_re_auth_token() or mark_for_reconnect() if a drain holds the connection. The connection lock is released before socket I/O. Release keeps _in_use set until event dispatch completes, including the error path.

The 60 added overlap cases fail on 53b86ce and pass here. All 228 cache cases pass across both pools, both parsers and all tracking modes; the 108 connection-pool and 80 maintenance-notification tests also pass. The three default test stages and repository linters pass with Python 3.13.18, pytest 9.1.1 and Redis 8.10.2. The full protocol/response matrix has not been run locally; the new head's workflows await approval.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1afc08d2c8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[cursor]

Stale Bugbot comment from a previous run.

[Grivn]

Pushed a follow-up in 4236732. Checkout now waits for idle maintenance, pool disconnect joins drains outside the pool lock, and cache candidates cannot reconnect closed owners or acquire an inherited owner's lock after fork. Shared-cache invalidation is atomic as well; the reproduced KeyError was caught and logged by the parser, rather than directly failing the client read.

All 72 added cases pass; 66 fail on 1afc08d, and six are ordinary-pool controls. The default stages pass 9,171 tests, the focused suite passes 639, and linters and all 12 read/write stress cases pass. Five interleaved throughput runs per case show a 1.9–3.8% cost at 1–2 readers and less than 0.7% difference at 8–16 readers, with zero extra GETs in the final samples. The full protocol/response matrix has not been run locally.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 423673206d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Grivn]

Pushed 4b7c518 to cover three more ownership paths: maintenance reconfiguration now joins idle drains outside the pool lock, completion listeners run after pool disconnect releases the owner lock, and interrupted checkouts restore their reserved slot.

All 60 added cases pass; 48 fail on 4236732. The 9,201 default tests, 699 focused tests, linters and 12 read/write stress cases also pass. The description includes the repeated throughput comparison and the one sample with extra server GETs.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4b7c5181ec

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[petyaslavova]

Thanks for the update. I checked 1afc08d, 4236732 and 4b7c518. All of the points from my last review are addressed: idle AUTH and MOVING claim the connection without blocking and fall back to deferral, release keeps _in_use set through the event dispatch, and the overlap regressions are in place. I re-ran my benchmark against the merge base. With 2-16 readers, both pools are now at or above it, with zero server GETs on a warmed key, and the #4443 repro completes on both pools.

Two items remain:

1. The open Codex comment about custom connection classes is real, and I reproduced it. _disconnect_from_pool calls _reset_maintenance_on_disconnect() without the isinstance(self._conn, MaintNotificationsAbstractConnection) guard that CacheProxyConnection.disconnect uses. So pool.disconnect() and close() raise NotImplementedError and leave the socket open when the connection class does not inherit the maintenance mixin. Please add the same guard and a regression test.
2. With a single reader, ConnectionPool is about 7% slower than the merge base, and BlockingConnectionPool about 3%. When owner is self, the hit path does not need _in_use_lock or the pid and is_connected checks: nothing else touches the socket of a checked-out connection, and send_command has just drained it. Please add a fast path for that case and include a 1-reader comparison against master.

Please also resolve the Codex threads that the latest commits fixed: the shared-cache invalidation deletion, the inherited cache owners after fork, the maintenance-config disconnects, the completion listeners closing the pool, and the interrupted ownership wait. Keep the custom connection class thread open until item 1 is fixed.

After that, the PR needs the upstream CI run.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3c40947efa

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 408c69482a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[cursor]

Stale Bugbot comment from a previous run.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 666f78d04a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 85488e4a4e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 94aafff365

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[cursor]

Stale Bugbot comment from a previous run.

[cursor]

Stale Bugbot comment from a previous run.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 984fe9bbed

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0beab635e4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 943f4907ed

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 426b0f7fc9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7db6bd63f9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 887b0271c6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 342579efcc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Defer cached teardown beyond the outer topology lock

With client-side caching, topology cleanup can still deadlock if GC finalizes an abandoned Redis.from_pool(pool) client while disconnecting a free socket and a peer is draining another idle owner. disconnect_free_connections() defers cached teardown only within its own scope; on exit, it joins the owner's _in_use_lock while this enclosing pool._lock is still held. The peer needs that pool lock to finish timeout restoration, so neither operation can complete, even with maintenance notifications disabled. A fresh automatic-GC/socket-pair probe on Python 3.13 hangs in _defer_cached_disconnects().__exit__ here and completes on the parent; unlike the earlier completion-listener issue, no event listener is involved. Extend cached-disconnect deferral around the outer topology lock as well.

Useful? React with 👍 / 👎.

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

^{Reviewed by Cursor Bugbot for commit 342579e. Configure here.}

[cursor]

Nested free disconnect deadlocks idle drain

High Severity

disconnect_free_connections starts _defer_cached_disconnects while callers such as cluster _free_pool_connections and SMIGRATED already hold pool._lock. Nested pool close then runs _disconnect_from_pool before that outer lock is released. An idle-owner drain already holds _in_use_lock and waits for the same pool lock to restore socket timeouts, so the two threads deadlock.

Additional Locations (2)

• redis/cluster.py#L3486-L3489
• redis/maint_notifications.py#L1570-L1603

 

^{Reviewed by Cursor Bugbot for commit 342579e. Configure here.}