fix: hand redis.asyncio.Redis the async Retry, not the sync one - #29
Merged
Conversation
build_redis_retry built redis.retry.Retry for both factories, and the async factory handed that to redis.asyncio.Redis. The sync class's call_with_retry does not await, so the client awaited the coroutine outside the retry loop and retry.enabled=True, max_attempts=N never retried on an async client: 0.50 s to fail against a paused Redis with socket_timeout=0.5, where the sync client took 3.42 s. Both helpers take asyncio=False now; the async factory passes True and gets redis.asyncio.retry.Retry, for the exponential object and for the zero-retry one alike. The sync path is unchanged. The API rows and rule 5 on the agents page and the retry section of the configuration guide say so.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
build_redis_retrybuiltredis.retry.Retryfor both factories, andcreate_async_redis_clienthanded that toredis.asyncio.Redis. The sync class'scall_with_retryis a plain function: it callsdo()once and returns the coroutine it got, so the client awaited that coroutine outside the retry loop, andretry.enabled=True, max_attempts=Nnever retried on an async client — single node on every redis-py in the range, and the async cluster's pipeline path with it. The zero-retry object from #27 was the same class, which happened not to matter: no retries need no loop.What changed
build_redis_retry(settings, *, asyncio=False)andbuild_base_redis_kwargs(settings, *, asyncio=False)buildredis.retry.Retryby default, exactly as before, andredis.asyncio.retry.Retrywithasyncio=True— the exponential object and the zero-retry one alike. The async factory passesasyncio=Truefor single and cluster; the sync factory is untouched.build_redis_retrycarriesLiteraloverloads, so a sync caller still seesredis.retry.Retryfrom the type checker and an async caller sees the async class, with abooloverload for code that passes the flag through.The two API rows, rule 5 and a "common mistakes" block on the agents page, and the retry section of the configuration guide say what the code does.
What I checked in redis-py
One venv per version, the same ten as for #25, reading the classes off the source.
redis.asyncio.retry.Retryis there from 4.5.0 through 8.1.0, never a subclass ofredis.retry.Retry, always withasync def call_with_retryand the same(backoff, retries, supported_errors)constructor. On every one of those versions the asyncConnection.connectandRedis.execute_commandbothawait ...retry.call_with_retry(...), so the fix behaves the same across the range and moves no floor.What I rejected
Swapping the class inside the async factory alone.
build_base_redis_kwargsandbuild_redis_retryare exported from the root, and anyone building aredis.asyncio.Redisfrom them by hand would keep the trap — the reason #27 put the zero-retry object in the helper rather than in the factory.A second pair of helpers for the async side, mirroring the
aio/syncsplit. Two more root exports and two more rows on the agents page for one bit of information, andbuild_base_redis_kwargswould either be duplicated or wrap the sync one and patchretry, which is the rejected shape one level down.Tests
Unit: the async factory's
retry, single and cluster, isredis.asyncio.retry.Retrywith retries enabled (test_resilience.py, now parametrized over cluster) and with retries off (test_client.py); both helpers build the async class withasyncio=Trueand the sync class by default (test_utils.py). Integration: an async client withmax_attempts=3, backoff_base=0.1, backoff_cap=1.0andsocket_timeout=0.5against a pausedredis:7-alpineraisesTimeoutErrorafter 3.0 to 6.0 s — four attempts plus 0.2 + 0.4 + 0.8 s of backoff — with a sync twin. The sync unit tests are unchanged and still pinredis.retry.Retry.With master's
utils.pyandaio/factory.pyswapped back in under the new tests, those eight fail — the three helper tests, the two enabled and two disabled async factory tests, and the async integration test at 0.50 s — and the ten sync and retries-off ones pass.Before and after
One GET per kit-built client, retries enabled as above,
socket_timeout=0.5, against the paused container, redis-py 8.1.0.master:
this branch:
Gate
make check: ruff checkAll checks passed!, ruff format54 files already formatted, mypySuccess: no issues found in 25 source files.make test:144 passed, coverage 99.64% against the 90% floor.uv.lockuntouched.CHANGELOG.mdis release-please's.Closes #26