Skip to content

fix(migration): support RESP3 key enumeration - #730

Open
Kuang-xianxin wants to merge 1 commit into
redis:mainfrom
Kuang-xianxin:codex/fix-migration-resp3-enumeration
Open

fix(migration): support RESP3 key enumeration#730
Kuang-xianxin wants to merge 1 commit into
redis:mainfrom
Kuang-xianxin:codex/fix-migration-resp3-enumeration

Conversation

@Kuang-xianxin

@Kuang-xianxin Kuang-xianxin commented Sep 6, 2026

Copy link
Copy Markdown

Closes #713
Closes #714

On RESP3 connections, migration key enumeration reads byte-keyed FT.INFO maps as string-keyed dictionaries, treating failed or incomplete indexes as ready. Healthy indexes then hit a KeyError when the aggregate cursor's result map is sliced as a RESP2 row list.

Normalize FT.INFO before checking readiness and extracting SCAN prefixes. Preserve a real zero for percent_indexed instead of treating it as the missing-value default. Share aggregate key extraction between the sync and async executors, handling both RESP2 rows and RESP3 result maps while retaining cursor pagination and cleanup.

Validation:

  • The new regression file fails 16 cases on the original code (12 compatibility/control cases pass).
  • All 120 related enumeration, executor, planner and batch-migration unit tests pass with redis-py 8.1.0 and separately with 6.3.0. Tests cover decoded and byte responses, failed/partial/zero-progress indexes, scoped SCAN fallback, multiple cursor pages, empty pages and cursor cleanup.
  • Full source mypy passes (120 files); repository-wide Black/isort and scoped codespell pass.
  • These tests use protocol-shaped mock responses. Run with pytest --confcutdir=tests/unit to exclude the root Docker startup fixture. The Redis-backed integration suite was not run because Docker is unavailable locally.

Suggested release label: auto:patch.


Note

Medium Risk
Changes core migration enumeration logic; incorrect parsing could skip or mis-count documents during migrations, though behavior is guarded by SCAN fallbacks and new regression tests.

Overview
Fixes migration document enumeration on RESP3 Redis connections where FT.INFO and FT.AGGREGATE return structured maps instead of RESP2-style lists.

FT.INFO responses are normalized with convert_bytes before readiness checks and prefix extraction, so byte-keyed maps behave like string dicts. percent_indexed now treats an explicit 0 as incomplete indexing (SCAN fallback) instead of defaulting missing values to 1.0.

Adds shared _extract_aggregate_keys for sync and async executors to read __key from both RESP2 cursor pages and RESP3 aggregate result maps, with cursor pagination and cleanup unchanged. New unit tests cover RESP2/RESP3, decoded vs byte wire formats, incomplete indexes, aggregate errors, and cursor release.

Reviewed by Cursor Bugbot for commit d55117f. Bugbot is set up for automated code reviews on this repo. Configure here.

Normalize readiness and prefix metadata, preserve zero indexing progress, and share RESP2/RESP3 aggregate parsing across sync and async executors.

Fixes redis#713

Fixes redis#714

Assisted-by: Codex
@vishal-bala
vishal-bala self-requested a review September 9, 2026 16:15
@vishal-bala vishal-bala added the auto:patch Increment the patch version when merged label Sep 9, 2026

@vishal-bala vishal-bala left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for your contribution! This looks good, but it looks like there are a couple more spots where the fix should be applied and a couple details that could be clarified.

Comment on lines +273 to +274
progress = info.get("percent_indexed")
percent_indexed = float(progress) if progress is not None else 1.0

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The same expression survives at three sibling sites, all on the RESP3 path:

  • redisvl/migration/executor.py:1116-1119
  • redisvl/migration/async_executor.py:828-831
  • redisvl/migration/planner.py:173-175

Those read stats_snapshot, which comes from SearchIndex.info() and is already normalised by convert_bytes, so the byte keys are not the problem there. The value type is. Measured on Redis 8.2.7 across all four protocol and decode_responses combinations, percent_indexed is the string '1' on RESP2 and the float 1.0 on RESP3, so a genuine zero is truthy on RESP2 and falsy on RESP3, where or 1.0 reads it as fully built.

executor.py:1119 feeds needs_exact_count, which gates needs_enumeration at :1120-1125; when false, the migration skips key enumeration altogether and validation falls back to the weaker num_docs comparison. That is the outer guard for the same condition this line now catches. planner.py:175 suppresses the "Source index is still building" warning in rvl migrate plan.

progress = snapshot.stats_snapshot.get("percent_indexed")
source_percent_indexed = float(progress) if progress is not None else 1.0


def _extract_prefixes_from_info(info: Any) -> List[str]:
"""Extract Redis Search index prefixes from dict or list FT.INFO shapes."""
info = convert_bytes(info)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is the most consequential change in the diff, and the description does not mention it. Worth adding, because it changes how urgent the fix looks to anyone triaging.

Measured pre-PR on RESP3 with decode_responses=False, _extract_prefixes_from_info returned [] against the byte-keyed FT.INFO. build_scan_match_patterns([]) returns ["*"] (utils.py:86-92), so _enumerate_with_scan enumerated the entire keyspace as the index's documents. _rename_field_in_hash at :640 does not filter the key list it is handed, so an unrelated key carrying a field of the same name gets rewritten:

otherapp:user:9  before: {'email': 'x@y.z', 'title': 'someone elses data'}
                 after : {'email': 'x@y.z', 'headline': 'someone elses data'}

Post-fix, prefix extraction returns the real prefix in all four combinations. Reaching the bad path needs FT.AGGREGATE to fail with a ResponseError first, which the _enumerate_with_aggregate docstring treats as routine cursor expiry rather than an exotic condition.

if isinstance(results_data, dict):
keys = (row["extra_attributes"]["__key"] for row in results_data["results"])
else:
# RESP2 starts with the row count, followed by field/value pairs.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Measured on Redis 8.2.7, the leading element is not the row count: a two-row page came back as [1, [b'__key', b'a:0'], [b'__key', b'a:1']], while the same page reported total_results: 2 on RESP3. [1:] discards it either way, so only the comment is wrong.

Comment on lines +100 to +102
{"hash_indexing_failures": 0, "percent_indexed": 0.0},
],
ids=["failed-documents", "partial-index", "empty-index"],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Measured on Redis 8.2.7, a freshly created index with num_docs: 0 reports percent_indexed of '1' on RESP2 and 1.0 on RESP3, with indexing: 0, so an empty index takes the aggregate fast path rather than the fallback. A percent_indexed of 0.0 means the background build has not started.

Your PR body already has the right word: "zero-progress". The id carries weight here because, with the executors reverted, empty-index is the only readiness value that fails at decode_responses=True, making it the one case that guards this change.

Comment on lines +118 to +119
# The fast path would omit failed/pending documents, even if it did not crash.
client.execute_command.return_value = [[1, [b"__key", b"archive:1"]], 0]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Dead: assert_not_called() at :131 makes this return_value unreachable by construction. Worth dropping the assignment and moving the comment down beside that assertion, where a reader meets it after the thing it justifies.

Two redundant axes while you are here. With the executors reverted, all three readiness values fail at decode_responses=False, all catching the same byte-key bug, so two of the three earn nothing there. And test_closing_enumeration_releases_cursor fails only at protocol=3, where cursor release reads result[1] identically in both response models, so that axis is already covered by test_enumerate_aggregate_cursor_pages.

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

Labels

auto:patch Increment the patch version when merged

Projects

None yet

2 participants