Skip to content

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

Description

@vishal-bala

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions