redis / redis/redis-vl-python

Migration executor cannot read an FT.AGGREGATE cursor over a RESP3 connection, raising KeyError

Open
#714 0 comments 0 reactions 0 assignees View on GitHub

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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.