Conversation
Fixes #465 - Async Sentinel connections were using sync SentinelConnectionPool The _redis_sentinel_client() method was only using the sync Sentinel class, which caused AsyncRedis clients to be backed by sync SentinelConnectionPool. This resulted in runtime failures when async operations were attempted. Changes: - Import AsyncSentinel from redis.asyncio.sentinel - Branch on redis_class to use AsyncSentinel for async clients - Sync clients continue to use the sync Sentinel class
Additional tests for issue #465 fix: - Default port (26379) when not specified in URL - Default service name ('mymaster') when path is empty - Password-only authentication (no username) - Custom kwargs passthrough to master_for() Also updated existing tests to mock the appropriate Sentinel class (AsyncSentinel vs Sentinel) based on sync/async context.
🛡️ Jit Security Scan Results✅ No security findings were detected in this PR
Security scan by Jit
|
9e69cca to
46922f6
Compare
There was a problem hiding this comment.
Pull request overview
Fixes async redis+sentinel:// connection construction so async clients are backed by the correct asyncio Sentinel implementation, preventing runtime failures like TypeError: object MasterFor is not callable for async operations.
Changes:
- Use
redis.asyncio.sentinel.Sentinel(aliased asAsyncSentinel) when constructing async Sentinel clients; keep sync clients onredis.sentinel.Sentinel. - Expand/adjust unit tests to validate correct Sentinel class selection and several URL parsing edge cases.
- Update user docs to show Sentinel usage for both
SearchIndexandAsyncSearchIndex.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
redisvl/redis/connection.py |
Selects AsyncSentinel vs Sentinel based on sync/async client type; adds typing/docstrings. |
tests/unit/test_sentinel_url.py |
Updates mocks for async vs sync Sentinel class and adds edge-case parsing/kwargs tests. |
docs/user_guide/installation.md |
Documents Sentinel connections for both sync and async index usage. |
uv.lock |
Updates locked editable redisvl version metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with ( | ||
| patch("redisvl.redis.connection.Sentinel", side_effect=track_sync_sentinel), | ||
| patch( | ||
| "redisvl.redis.connection.AsyncSentinel", side_effect=track_async_sentinel | ||
| ), |
There was a problem hiding this comment.
The with (patch(...), patch(...)): parenthesized context-manager syntax requires Python 3.10+, but this project supports Python >=3.9.2. Please rewrite this to a Python 3.9-compatible form (e.g., with patch(...) as ..., patch(...) as ...: or nested with blocks).
| with ( | |
| patch("redisvl.redis.connection.Sentinel", side_effect=track_sync_sentinel), | |
| patch( | |
| "redisvl.redis.connection.AsyncSentinel", side_effect=track_async_sentinel | |
| ), | |
| with patch( | |
| "redisvl.redis.connection.Sentinel", side_effect=track_sync_sentinel | |
| ), patch( | |
| "redisvl.redis.connection.AsyncSentinel", side_effect=track_async_sentinel |
docs/user_guide/installation.md
Outdated
| @@ -174,3 +180,4 @@ The Sentinel URL format supports: | |||
| - Optional authentication (username:password) | |||
| - Service name (required - the name of the Redis master) | |||
There was a problem hiding this comment.
The list says the Sentinel service name is required, but RedisConnectionFactory._parse_sentinel_url() defaults to "mymaster" when the URL has no path (and there’s now a unit test asserting that behavior). Please update this bullet to match the implementation (service name optional with default mymaster, or clarify that omitting it uses mymaster).
| - Service name (required - the name of the Redis master) | |
| - Optional service name (defaults to `mymaster`; this is the name of the Redis master) |
| Args: | ||
| redis_url: Sentinel URL in the format: | ||
| ``redis+sentinel://[user:pass@]host1:port1[,host2:port2,...]/service[/db]`` | ||
| redis_class: The Redis client class to use (Redis or AsyncRedis). |
There was a problem hiding this comment.
The docstring describes the URL format as requiring /service, but _parse_sentinel_url() (and the tests in this PR) allow the service name to be omitted and default to "mymaster". Please update the format description here to reflect that the service segment is optional (or explicitly document the default).
| with ( | ||
| patch("redisvl.redis.connection.Sentinel", side_effect=track_sync_sentinel), | ||
| patch( | ||
| "redisvl.redis.connection.AsyncSentinel", side_effect=track_async_sentinel | ||
| ), | ||
| ): |
There was a problem hiding this comment.
The with (patch(...), patch(...)): parenthesized context-manager syntax requires Python 3.10+, but this project supports Python >=3.9.2. Please rewrite this to a Python 3.9-compatible form (e.g., with patch(...) as ..., patch(...) as ...: or nested with blocks).
Improvements to Sentinel connection documentation: Code (redisvl/redis/connection.py): - Added comprehensive docstring for _redis_sentinel_client() with Args, Returns, Example - Added comprehensive docstring for _parse_sentinel_url() with Args, Returns, Example - Improved type hint for _parse_sentinel_url() return type (specific tuple) - Added explicit Dict[str, Any] type for sentinel_kwargs - Fixed docstring to clarify service name defaults to 'mymaster' Tests (tests/unit/test_sentinel_url.py): - Added module-level docstring explaining test coverage - Referenced GitHub Issue #465 - Fixed Python 3.9 compatibility (parenthesized context managers) Documentation (docs/user_guide/installation.md): - Added async example using AsyncSearchIndex - Clarified that both sync and async connections are fully supported - Fixed service name description (optional, defaults to mymaster)
46922f6 to
9d8f90a
Compare
Fix: Async Sentinel connections use correct AsyncSentinel class
Fixes #465
Problem
When using
redis+sentinel://URLs with async connections, the_redis_sentinel_client()method was only using the syncredis.sentinel.Sentinelclass. This causedAsyncRedisclients to be backed by a syncSentinelConnectionPool, resulting in runtime failures when async operations were attempted.Error observed:
TypeError: object MasterFor is not callableRoot Cause
The
_redis_sentinel_client()method inredisvl/redis/connection.pyonly imported and used the syncSentinelclass, regardless of whether the client was sync or async.Solution
AsyncSentinelfromredis.asyncio.sentinelredis_classto useAsyncSentinelwhen creating async clientsSentinelclassChanges
redisvl/redis/connection.pyAsyncSentinelimport; updated_redis_sentinel_client()to use appropriate Sentinel classtests/unit/test_sentinel_url.pyTesting
make check-types)New Test Coverage
master_for()Note
Medium Risk
Moderate risk because it changes Redis connection instantiation for Sentinel URLs, which can affect connectivity/HA behavior at runtime; coverage is improved with targeted unit tests.
Overview
Fixes
redis+sentinel://handling so async connections (get_async_redis_connection/AsyncRedis) useredis.asyncio.sentinel.AsyncSentinelrather than the syncSentinel, preventing runtime failures caused by a sync Sentinel connection pool.Adds/updates unit tests to mock the correct Sentinel class for sync vs async, plus additional parsing/kwarg passthrough edge cases and class-selection assertions. Documentation is updated to show both
SearchIndexandAsyncSearchIndexSentinel examples and clarifies defaults (service namemymaster).Written by Cursor Bugbot for commit 9d8f90a. This will update automatically on new commits. Configure here.