Skip to content

Fix/issue 465 async sentinel connection pool#522

Open
nkanu17 wants to merge 3 commits intomainfrom
fix/issue-465-async-sentinel-connection-pool
Open

Fix/issue 465 async sentinel connection pool#522
nkanu17 wants to merge 3 commits intomainfrom
fix/issue-465-async-sentinel-connection-pool

Conversation

@nkanu17
Copy link
Collaborator

@nkanu17 nkanu17 commented Mar 2, 2026

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 sync redis.sentinel.Sentinel class. This caused AsyncRedis clients to be backed by a sync SentinelConnectionPool, resulting in runtime failures when async operations were attempted.

Error observed: TypeError: object MasterFor is not callable

Root Cause

The _redis_sentinel_client() method in redisvl/redis/connection.py only imported and used the sync Sentinel class, regardless of whether the client was sync or async.

Solution

  • Import AsyncSentinel from redis.asyncio.sentinel
  • Branch on redis_class to use AsyncSentinel when creating async clients
  • Sync clients continue to use the sync Sentinel class

Changes

File Change
redisvl/redis/connection.py Added AsyncSentinel import; updated _redis_sentinel_client() to use appropriate Sentinel class
tests/unit/test_sentinel_url.py Updated existing tests to mock correct Sentinel class; added edge case tests

Testing

  • All 12 Sentinel tests pass
  • Type checking passes (make check-types)
  • Pre-commit hooks pass

New Test Coverage

  • 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()

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) use redis.asyncio.sentinel.AsyncSentinel rather than the sync Sentinel, 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 SearchIndex and AsyncSearchIndex Sentinel examples and clarifies defaults (service name mymaster).

Written by Cursor Bugbot for commit 9d8f90a. This will update automatically on new commits. Configure here.

nkanu17 added 2 commits March 2, 2026 14:57
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.
@nkanu17 nkanu17 requested review from bsbodden and Copilot and removed request for Copilot March 2, 2026 20:08
@jit-ci
Copy link

jit-ci bot commented Mar 2, 2026

🛡️ Jit Security Scan Results

CRITICAL HIGH MEDIUM

✅ No security findings were detected in this PR


Security scan by Jit

Copilot AI review requested due to automatic review settings March 2, 2026 20:19
@nkanu17 nkanu17 force-pushed the fix/issue-465-async-sentinel-connection-pool branch from 9e69cca to 46922f6 Compare March 2, 2026 20:21
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 as AsyncSentinel) when constructing async Sentinel clients; keep sync clients on redis.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 SearchIndex and AsyncSearchIndex.

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.

Comment on lines +187 to +191
with (
patch("redisvl.redis.connection.Sentinel", side_effect=track_sync_sentinel),
patch(
"redisvl.redis.connection.AsyncSentinel", side_effect=track_async_sentinel
),
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
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

Copilot uses AI. Check for mistakes.
@@ -174,3 +180,4 @@ The Sentinel URL format supports:
- Optional authentication (username:password)
- Service name (required - the name of the Redis master)
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
- Service name (required - the name of the Redis master)
- Optional service name (defaults to `mymaster`; this is the name of the Redis master)

Copilot uses AI. Check for mistakes.
Comment on lines +754 to +757
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).
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
Comment on lines +149 to +154
with (
patch("redisvl.redis.connection.Sentinel", side_effect=track_sync_sentinel),
patch(
"redisvl.redis.connection.AsyncSentinel", side_effect=track_async_sentinel
),
):
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
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)
@nkanu17 nkanu17 force-pushed the fix/issue-465-async-sentinel-connection-pool branch from 46922f6 to 9d8f90a Compare March 3, 2026 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Async Sentinel connections use sync SentinelConnectionPool, causing runtime failures

2 participants