fix(asyncio): convert synchronous retry policies

[AG0708]

Description of change

Passing redis.retry.Retry to an asyncio client currently appears valid but bypasses the retry loop and failure callback because its synchronous wrapper returns the coroutine before observing failures. This converts synchronous retry policies at the asyncio connection, pool, cluster, and MultiDB boundaries while preserving their backoff, retry count, and supported errors.

The regression coverage verifies both the stored policy type and the actual three-attempt/three-callback behavior.

Fixes #4262.

Pull Request check-list

• Do tests and lints pass with this change?
• Do the CI tests pass with this change (CI is starting on this PR)
• Is the new or changed code fully tested?
• Is a documentation update included (not applicable; no public API is added or changed)
• Is there an example added to the examples folder (not applicable)

---

Note

Medium Risk
Changes how retry policies are applied on asyncio connections and cluster/MultiDB clients, which can alter reconnect behavior. Conversion is compatibility-oriented and covered by tests, but custom sync retry implementations are intentionally dropped.

Overview
Fixes a bug where passing redis.retry.Retry into asyncio clients looked valid but skipped the async retry loop (the sync wrapper returned a coroutine without observing failures).

Async connection, pool, cluster, and MultiDB now convert sync policies via _to_async_retry, keeping backoff, retry count, and supported errors, and warning that custom call_with_retry is not preserved. Already-async or duck-typed retry objects are left unchanged. Docs describe the conversion and recommend configuring redis.asyncio.retry.Retry directly.

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a9bbb55b12

ℹ️ 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]

Hey @AG0708, thank you for your contribution!

The problem in #4262 is real and normalizing at the asyncio boundary sounds like a fair choice - repairing existing configurations beats crashing on upgrade. The current guard, though, classifies retry objects by class rather than by shape, and that might break users who work fine today.

There is no isinstance check on a retry object anywhere in the library right now: the asyncio stack only ever calls await call_with_retry(...), get_retries(), update_supported_errors(), and deep-copies the object. Any custom policy matching that shape is supported in practice, and _to_async_retry currently mishandles three such cases:

1. A custom AbstractRetry subclass with an async def call_with_retry works today, but is silently rebuilt as a plain Retry — the user's retry logic is discarded with no error and no warning.
2. A duck-typed policy that does not inherit from AbstractRetry works today, but now raises TypeError at construction — Connection(retry=…), ConnectionPool(retry=…), RedisCluster(retry=…), set_retry(…) and MultiDBClient. That removes an input the released API accepts.
3. A subclass of the synchronous Retry that overrides call_with_retry is genuinely broken today, so converting it is an improvement — but the result silently runs our policy instead of theirs.

Could you switch the check to detect a synchronous-shaped policy and pass everything else through untouched, so no working configuration changes behavior?

def _to_async_retry(retry):
    # Already async-shaped: asyncio Retry, its subclasses, custom AbstractRetry
    # implementations with an async call_with_retry, and duck-typed policies.
    if iscoroutinefunction(getattr(type(retry), "call_with_retry", None)):
        return retry
    if not isinstance(retry, AbstractRetry):
        return retry  # unknown shape - keep today's behavior instead of raising
    warnings.warn(
        "A synchronous redis.retry.Retry was passed to an asyncio client and has "
        "been converted to redis.asyncio.retry.Retry. A custom call_with_retry "
        "implementation is not preserved - please use redis.asyncio.retry.Retry.",
        UserWarning,
        stacklevel=2,
    )
    return Retry(
        backoff=retry._backoff,
        retries=retry._retries,
        supported_errors=retry._supported_errors,
    )

Two more things before we can merge. First, please keep the UserWarning above: without it the fix silently changes runtime behavior on upgrade, since code that makes one attempt today will start retrying with backoff and running the disconnect-on-error callbacks. Second, please add a note to docs/retry.rst — that page documents only redis.retry.Retry and has no asyncio example, which is what leads users into this in the first place.

For tests, please cover the two pass-through cases above (a custom async AbstractRetry subclass and a duck-typed policy must come out unchanged), and add a regression test through the entry point from the report — redis.asyncio.ConnectionPool.from_url(retry=…, retry_on_error=[…]) followed by await pool.get_connection() — plus one that goes through a real seam such as Connection.connect() or check_health() rather than calling retry.call_with_retry directly.

Thanks again for digging into this — with those changes it should be ready for another review.

[AG0708]

Addressed in adee4bc. The conversion now detects coroutine-shaped policies and preserves custom async AbstractRetry implementations and duck-typed policies unchanged; only synchronous AbstractRetry policies are converted, with the requested UserWarning. I also added the asyncio Retry documentation, the from_url/get_connection regression, a Connection.connect retry-path regression, and retained the uncopyable-backoff coverage. The focused retry/MultiDB suite passes (91 passed; one live-Redis integration test deselected), and Ruff, format, Vulture, RST syntax, and diff checks are clean.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b01690c17d

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[cursor]

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

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

[petyaslavova]

Hey @AG0708,

This covers everything from the previous round: the guard classifies by shape, the UserWarning is kept, docs/retry.rst has the asyncio section, and the new tests go through Connection.connect() and ConnectionPool.from_url(...) + get_connection() instead of calling call_with_retry directly.

Two small items before merge:

1. In test_async_shaped_retry_is_preserved, both policies define call_with_retry as async def, so both take the iscoroutinefunction branch. Please add a duck-typed policy with a plain def call_with_retry that returns an awaitable, so the not isinstance(retry, AbstractRetry) pass-through - the branch that keeps today's behavior for unknown shapes - is covered as well.
2. ConnectionPool.set_retry now also stores the converted policy in connection_kwargs, matching the sync pool. Please add a regression test that a connection created after a direct pool.set_retry(...) uses the new policy.

One non-blocking note: Redis.set_retry still writes the unconverted object into connection_kwargs before pool.set_retry overwrites it, so converting there too (or dropping that write) would remove the ordering dependence. You can also disregard the Bugbot deepcopy finding - Connection.__init__ and Redis.__init__ already deep-copied any retry policy on master, so this PR does not introduce that constraint.

CI has not run on this PR yet, so we will need a green run after these changes.

[petyaslavova]

Hey @AG0708,

Both items from the last round are covered - DuckTypedAwaitableRetry exercises the not isinstance(retry, AbstractRetry) pass-through, and test_pool_set_retry_applies_to_new_connection is a real base-failing regression, since on master the new connection fell back to the Retry(NoBackoff(), 0) default. ConnectionPool.set_retry now matches the sync pool too.

One thing left before merge. Please convert also in Redis.set_retry rather than relying on pool.set_retry to overwrite the raw write:

def set_retry(self, retry: Retry) -> None:
    retry = _to_async_retry(retry)
    self.get_connection_kwargs().update({"retry": retry})
    self.connection_pool.set_retry(retry)

ConnectionPool.set_retry then passes the already-async policy straight through, so there is no second warning. The invariant worth holding is that connection_kwargs["retry"] only ever contains an async-shaped policy, because make_connection builds every connection from that dict - so if an unconverted policy is left there, the conversion branch runs and warns on each new connection instead of once at init. That is reachable today: ConnectionPoolInterface.set_retry is abstract with a pass body, so a custom pool that honors the interface without writing connection_kwargs leaves the raw object in place, and get_retry() also hands back a policy no connection uses.

Only set_retry needs this - Redis.__init__ writes into the same dict the pool then converts, so there is no window there. For the regression test, note that the existing test_pool_converts_sync_retry passes either way; it needs a pool whose set_retry does not write connection_kwargs to fail on the base.