Migration executor cannot read an FT.AGGREGATE cursor over a RESP3 connection, raising KeyError
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 427
- Forks
- 101
- Avg merge
- 6d 3h
- Merged PRs (30d)
- 20
Description
MigrationExecutor._enumerate_with_aggregate cannot read an aggregate cursor over a RESP3 connection. It raises KeyError: slice(1, None, None), and because the caller catches only ResponseError the exception escapes the SCAN fallback and aborts the enumeration outright.
Reproduction
Measured on Redis 8.4.6 with redis-py 8.1.0, RedisVL at edcc78e.
KeyError: slice(1, None, None)
redisvl/migration/executor.py:335, in _enumerate_with_aggregate
for item in results_data[1:]:
redisvl/migration/executor.py:317 issues the command through the client rather than through the search helper:
result = client.execute_command(
"FT.AGGREGATE", index_name, "*", "LOAD", "1", "__key",
"WITHCURSOR", "COUNT", str(batch_size), "MAXIDLE", "300000",
)
Raw execute_command on the client gets no Search module response callbacks, because redis-py registers those on the object returned by client.ft(). The reply therefore arrives in whatever shape the protocol delivered it. Measured replies for one index holding three indexed documents:
protocol=2
[[1, [b'__key', b'f:ok0'], [b'__key', b'f:ok1'], [b'__key', b'f:ok2']], 0]
protocol=3, and byte for byte the same with no explicit protocol argument on redis-py 8
[{b'attributes': [], b'format': b'STRING',
b'results': [{b'extra_attributes': {b'__key': b'f:ok0'}, b'values': []}, ...],
b'total_results': 3, b'warning': []}, 0]
Both are two-element lists, so results_data, cursor_id = result at line 332 succeeds either way. The failure lands one line later: under RESP3 results_data is a dict, and dict[1:] raises KeyError with the slice object as the key. redisvl/migration/async_executor.py:184 and :187 are the same code and fail identically.
Worth noting that this is not a TypeError. dict.__getitem__ accepts a slice quite happily and reports it as a missing key, which is a weaker diagnostic than a type error would have been and makes the traceback read as though an index were absent.
Why this is more than a corner case
redis-py 8 negotiates RESP3 by default, so a caller who constructs a client with no protocol argument at all gets the failing shape. That is measured: the second reply above came from Redis.from_url(url) with no protocol keyword.
RedisVL currently masks the problem by forcing kwargs.setdefault("protocol", 2) at five points in redisvl/redis/connection.py (lines 640, 681, 734, 760 and 770), but that default reaches only clients RedisVL builds itself. A caller-supplied client keeps whatever protocol it negotiated. The default is also transitional, and this bug blocks removing it: the moment RedisVL stops pinning RESP2, every migration that reaches aggregate enumeration hits this path.
The fallback does not catch it
_enumerate_indexed_keys wraps the aggregate call in except ResponseError at around line 286 of executor.py. KeyError is not a ResponseError, so the SCAN fallback that exists to rescue a failed aggregate enumeration never runs and the caller sees the raw traceback.
Suggested fix
Issue the command through client.ft(index_name).aggregate(...) so that redis-py's own callbacks normalise the reply. If the raw command must stay, branch on the reply shape and read results and extra_attributes when handed a dict. Widening the caller's except clause beyond ResponseError is worth doing as well, though on its own it would only convert a crash into a quiet fall back to SCAN, which hides a genuine defect rather than fixing it.
Related
The readiness check guarding this same enumeration has an independent RESP3 bug, #713, and the two are worth fixing in one pass. They cannot both surface under RESP2, because a correct readiness check diverts to SCAN before the aggregate code runs. Under RESP3 the first removes the guard and this one then raises.
A smaller adjacent gap
_parse_sentinel_url in redisvl/redis/connection.py:929 reads only netloc and path, never query, so a Sentinel URL that specifies a protocol has it silently ignored:
>>> RedisConnectionFactory._parse_sentinel_url("redis+sentinel://host:26379/mymaster?protocol=3")
([('host', 26379)], 'mymaster', None, None, None)
Measured on the same build. The protocol=3 parameter is dropped without a warning. This is a separate defect from the cursor bug and could reasonably be split out, but it belongs to the same cluster of inconsistent protocol handling across connection paths.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in redisvl/migration/executor.py at _enumerate_with_aggregate and _enumerate_indexed_keys, then compare the matching logic in redisvl/migration/async_executor.py. Reproduce with a RESP3 client and inspect how FT.AGGREGATE replies differ from RESP2; also review the referenced connection defaults. Done means indexed-key enumeration works under RESP2 and RESP3, with the SCAN fallback still available when aggregate enumeration fails.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python, redis
- Domain
- backend, databases, search
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 68/100