Skip to content

Support Redis OSS Cluster API with redis 1.8.1 and reproducible Flex profiles - #393

Merged
fcostaoliveira merged 5 commits into
masterfrom
fix/msmarco-flex-scalar-schema
Nov 3, 2026
Merged

fcostaoliveira merged 5 commits into
masterfrom
fix/msmarco-flex-scalar-schema

Conversation

@fcostaoliveira

@fcostaoliveira fcostaoliveira commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

Redis uploads to an OSS Cluster API endpoint previously used standalone connections and failed on MOVED. The Redis engine now uses upstream redis 1.8.1 ClusterClient, ClusterConnection and cluster_pipe for keyed commands, redirects and cross-slot pipelines. Enable collection_params.cluster_api=true or REDIS_CLUSTER=1; distributed FT.* commands use the configured database coordinator.

The accompanying text-tag-only profile supports Redis Cloud Flex's reduced MS MARCO suite: TEXT/TAG queries, stored numeric/vector payloads, no numeric or vector index. Profile identity, fresh-index/empty-prefix checks, retained-corpus validation and matching OpenSearch mappings prevent accidental reuse of a different layout. This does not complete the full numeric suite.

Standalone configure/upload connections retain their previous timeout defaults. Shared-prefix scans in both standalone and cluster mode now deduplicate compact SHA-256-derived fingerprints and expose recorded time/key/storage limits. Counting remains proportional to corpus size (roughly 2.3 GB of table storage at 100M keys, plus other process memory); caps are independent. The elapsed limit is cooperative between responses. RedisConfig::try_from_env() uses strict CLI-compatible parsing; deprecated from_env() preserves its legacy coercion.

Validation was performed on tagged AWS compute. Qualified executable source is 4869dc0848958861f53c9907450834cfe6fe6804. PR head 016ac283290820b625bbab3361d08bbfbed00cc4 differs only by seven README clarification lines; no executable source differs. The managed-benchmark PR pins the qualified source.

  • make agent-check, debug/release transport/config guards, five real cluster tests, Redis integration (76 passed, one existing ignored), and OpenSearch integration (41 passed) passed.
  • Six new compatibility mutations produced assertion failures, then restored checks passed. Earlier nine routing/profile mutation proofs remain separately archived as historical evidence.
  • The exact release binary passed live Redis Cloud OSS Cluster API fixtures for memory and streaming uploads, fresh and retained reuse, eight independent TEXT/TAG membership cases at concurrency 1 and 4. The initial streaming attempt hit the retained test-index quota; its failure was preserved, two already-archived fixture indexes were dropped without DD, and a fresh streaming attempt passed.
  • Binary SHA-256: dcc13cbeeae28268061eea478007309135929c7510c6d168d296f91a32bceefe. Identical versioned S3 bytes were verified on both AWS clients.
  • The 68-file private qualification manifest is s3://redis-perf-vector-results-archive/datasets/msmarco/msmarco-388-managed-10m-rcp-20261103/driver-cluster/4869dc0848958861f53c9907450834cfe6fe6804/20261103T184747Z/manifest.json, version cZ7rZSmnV97SCqMOE1N7uvhn7l7eReHn. Encryption, ownership tags and uploaded-version SHA-256 readback were verified. Earlier c53e76c qualification and binary 56640ec… are historical, not the final executable.

The local three-primary fixture runs core Redis without Search modules. Distributed Search validation is against Redis Cloud; independent OSS Search nodes have no equivalent coordinator. Exact-gitlink cluster CI runs in the private orchestration repository on self-hosted AWS compute. Driver-owned persistent cluster CI is still a gap, tracked in #394; this repository currently has no self-hosted runners. These private qualification results are the author's reported evidence, not public GitHub review approvals.

Remaining merge gate: final-head public CI. No release tag or 10M performance result is claimed.

@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

🤖 Automated first-pass review — a human maintainer's review is still required before merge.

What I checked: the new src/redis_transport.rs (routing split, cluster_pipe use, scan_prefix and its topology/migration guards, fingerprint budget), every Redis-engine connection site the diff moves onto it (configure, in-memory and streaming upload, search, filter-only, fulltext, mixed), the text-tag-only profile (src/text_tag_profile.rs, Redis configure/delete, OpenSearch mapping and reuse validation, the marker and compound-fingerprint binding), and the three new or extended test suites. I found no silent-wrong-result path in the production code. Several things here close real gaps:

  • count_prefix_keys used to end its loop on any SCAN error (Err(_) => break) and return the partial count. Now the error is fatal. SCAN can also return duplicate keys, so a raw keys.len() sum could over-count a shared corpus to "complete". The fingerprint dedup closes that, and compact_fingerprints_deduplicate_before_applying_key_limit pins it. This is the corpus-count trust bug class, fixed in the direction that fails safe.
  • A partial scan fails closed: the ACL -scan on one non-seed primary in prefix_scan_and_markers_cover_nonseed_primaries_with_resp3. So does migration between two non-seed primaries, which the seed's own CLUSTER NODES view can't see. Both are the right things to pin.
  • The cluster fixture follows the integration_redis FLUSHALLs whatever is on REDIS_TEST_PORT's default 6399, and the old port-override spelling silently no-ops #292 ownership rule: loopback only, port pinned to 17379, and a per-run random fixture- password that a foreign server won't accept. That covers the destructive CLUSTER SETSLOT / ACL SETUSER steps.
  • The reduced profile fails loudly on the cases a quiet path would get wrong. Fresh SKIPINITIALSCAN creation requires an absent index and an empty owned prefix (redis.rs configure). delete() refuses teardown. Shared corpus is refused. A partial retained corpus fails on --skip-upload (test_binary_redis_text_tag_fresh_and_retained_reuse, the DEL redis-text-tag:0 step).

Worth fixing before merge

  1. The validation in the PR body doesn't cover HEAD. The body pins "the final clean commit c53e76c" for make agent-check, cluster integration, the release build, the live Redis Cloud Flex run and the quoted binary SHA-256. Two commits came after it: 4869dc0 ("Preserve standalone timeouts and bound shared-prefix scans") and 016ac28. 4869dc0 is not cosmetic. It appears to add ScanLimits, the SHA-256 fingerprint set, the elapsed-time checks inside scan_prefix, and the standalone-vs-cluster timeout split in Client::get_connection (redis_transport.rs, the if self.cluster_api { … } else { client.get_connection()? } block). Those are the paths the live Flex run exercised: the fresh-prefix check runs scan_prefix → topology() → CLUSTER NODES per primary. Please re-run the live cluster cases and the cluster suite on the actual head, or say plainly in the body which claims hold at HEAD and which only at c53e76c.

  2. Public CI never runs integration_redis_cluster. .github/workflows/ci.yml isn't touched. Its Redis job runs only cargo test --test integration_redis (ci.yml:119), and make agent-check-test runs no integration target. So the cluster-mode half of a 654-line transport has no public gate. That half covers routing, cluster_pipe ordering and error propagation, MOVED/ASK, and the scan refusals. The body says it passes in a private orchestration repo's self-hosted CI, which this repo can't see or re-run. The script needs no Docker, so a CI job that runs make integration-test-redis-cluster looks cheap. Without one, the "break it and confirm it goes red" mutations only hold until the next change to redis_transport.rs. A related point: integration-test: integration-test-redis-cluster (Makefile:199) now makes the local make integration-test download from download.redis.io and compile Redis 8.8.0 from source. That's a new network and C-toolchain requirement on the default local target, and it might be better as its own target or a CI job.

Optional / follow-up (not blockers)

  1. The post-upload check_commandstats guard may be partly blind in cluster mode. coordinator_command sends INFO and CONFIG to the coordinator only. So reset_commandstats and the "no HSET failures during upload" check (redis.rs:2669-2676) read one node's failed_calls. On an OSS core cluster that's the seed only: a failed HSET on the other two primaries wouldn't show up there. This isn't a correctness hole today, because cluster_pipe errors already propagate out of upload_batch_internal and cluster mode doesn't replay them. But it's a guard that looks like it covers the whole cluster and doesn't. I'm not certain whether Redis Cloud's OSS Cluster API endpoint aggregates INFO commandstats across shards. If it does, a one-line comment saying so would do. If it doesn't, either sum over primaries the way scan_prefix iterates them, or note the limit.

  2. Standalone shared-corpus runs now pay for the dedup too. The README says this, but the PR body frames the change as cluster-only. At MS MARCO 100M, hashbrown sizes to 2^27 buckets × (16 B fingerprint + 1 control byte), about 2.3 GB of client RSS. It also hashes 100M keys with SHA-256 on every shared-corpus config's pre-check, where the old loop used O(1) memory. It's outside the timed window as far as I can see, and the dedup is the right fix. A benchmark client sized for the old path could still OOM here, so one sentence in the PR body for standalone users would help.

  3. Scope. Two independent features ship in one PR: the cluster transport plus the redis 1.3.0 → 1.8.1 bump, and the text-tag-only profile. They share no code beyond the Redis engine file, so a regression in either reverts both. Splitting them would keep each revertable on its own. Not a blocker if a maintainer prefers one PR.

  4. Unverifiable claims in the body. "All seven requested reviewers approve", the private CI run and the privately archived qualification logs aren't visible anywhere in this repo's record. Whoever makes the merge call should treat them as the author's statements, not as review evidence.

Net: careful, well-guarded work. The scan path now fails closed where it used to quietly undercount, and the reduced profile refuses every lifecycle that could leave SKIPINITIALSCAN over stale keys. Items 1–2 are about whether HEAD has actually been verified and whether that verification is publicly repeatable, not about defects I found in the code. The merge decision is the maintainer's.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

Docker Build Validation

Platforms tested:

  • linux/amd64 (built and tested)
  • linux/arm64 (build validated)

Tests performed:

  • --help command
  • --describe datasets command
  • Integration test (random-100 dataset + Redis)
  • Multi-platform build validation (amd64 + arm64)

@fcostaoliveira fcostaoliveira changed the title Add reproducible TEXT/TAG-only compound profile for Redis Flex Support Redis OSS Cluster API with redis 1.8.1 and reproducible Flex profiles Nov 3, 2026
@fcostaoliveira

Copy link
Copy Markdown
Collaborator Author

Addressed the requested evidence and partial-upload case. All validation below ran on tagged AWS compute with redis crate 1.8.1.

Guard deliberately disabled Expected regression caught
Fresh index must be absent Redis fresh/reuse integration failed
Owned prefix must be empty Redis stale-key integration failed
OpenSearch dynamic=false OpenSearch reuse integration failed
Foreign indexed scalar property Reduced-schema unit test failed
Excluded field multifield Reduced-schema unit test failed
Redis SORTABLE/UNF storage Reduced-schema unit test failed
Stable slot-map comparison Cluster topology unit test failed
Migration refusal Non-seed migration integration failed
Dedicated cluster pipeline Three-primary pipeline integration failed

All nine mutations produced test assertion failures, not merely build errors, and each source file was restored. The partial-corpus test now deletes an owned hash after successful retained upload, then asserts --skip-upload fails despite the matching marker. The restored final checks are running before merge.

The full upgraded-crate regressions passed: Redis 76 (one pre-existing ignored test), OpenSearch 41, Valkey 29, Dragonfly 15, KiviDB 26, plus the four authenticated cluster transport tests and make agent-check. Live Redis Cloud Flex with OSS Cluster API enabled passed eight independent text/tag membership cases at concurrency 1 and 4, both fresh and retained reuse, with memory and streaming uploads. SCAN now checks every primary and refuses partial scans or migration/topology changes; it no longer assumes a standalone coordinator SCAN covers the cluster.

The reduced delete() refusal remains defense in depth and is not counted as a killed mutation. Existing compound results gain the explicit scalar_indexing: "all" metadata field, while the existing full-profile comparison fingerprint stays unchanged. Reduced-profile identity is distinct. No 10M performance result is claimed.

The final CI-only fix clears Make's exported HDF5 override inside the static cluster-test harness. It does not change the reviewed Rust transport; the self-hosted cluster gate and full PR CI are rerunning on that final commit.

@fcostaoliveira

Copy link
Copy Markdown
Collaborator Author

The bot's updated review identified two backward-compatibility changes that the initial approvals did not explicitly cover. Automatic merge and 10M activation are stopped while these are corrected and revalidated:

  • Restore existing standalone configure/upload timeout defaults; keep cluster routing deadlines and explicit search-worker deadlines. A Linux integration regression observes the actual socket timeout settings without a long sleep.
  • Replace full-key retention during shared-prefix scans with compact SHA-256-derived 128-bit fingerprints and configurable, recorded resource limits. The default admits the realized 100M row count; limits fail closed before excessive fingerprint allocation. The elapsed budget is explicitly cooperative between responses, not a hard interruption of an unbounded standalone socket.
  • Add a strict RedisConfig::try_from_env() matching CLI parsing, while deprecating and preserving the legacy constructor's integer coercion. Document cluster-test build/network prerequisites.

The driver repository currently exposes no self-hosted runners. The exact driver gitlink remains covered by the private repository's self-hosted cluster CI and AWS qualification; adding a driver-owned self-hosted runner is a separate CI infrastructure follow-up. No hosted job is being added.

The new source will receive AWS checks, targeted mutation proofs and renewed reviewer approval before merge. The previously archived c53 qualification remains valid for that earlier revision only; no 10M performance result is claimed.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

Docker Build Validation

Platforms tested:

  • linux/amd64 (built and tested)
  • linux/arm64 (build validated)

Tests performed:

  • --help command
  • --describe datasets command
  • Integration test (random-100 dataset + Redis)
  • Multi-platform build validation (amd64 + arm64)

@fcostaoliveira

Copy link
Copy Markdown
Collaborator Author

The current PR description now names the correct qualification: executable source 4869dc0848958861f53c9907450834cfe6fe6804, binary dcc13cbeeae28268061eea478007309135929c7510c6d168d296f91a32bceefe, five cluster tests and the new 68-file archive. PR head 016ac28 adds only README clarification; the executable source is identical. Both live upload modes and retained reuse passed on the final binary. Historical c53 evidence is explicitly labelled historical.

The public CI ownership gap is real and tracked in #394. No self-hosted runners are currently available in this driver repository; adding hosted compute would violate the requested runner policy. The current revision is covered by the exact-gitlink self-hosted orchestration check and AWS qualification. README now distinguishes core cluster transport coverage from live Redis Cloud Search validation, and describes standalone scan memory and independent limits.

Mutation-to-test mapping (six compatibility mutations were run on the final source; the earlier nine routing/profile mutations are retained as separate historical evidence):

Disabled behavior Test that failed
Scan key cap compact_fingerprints_deduplicate_before_applying_key_limit
Fingerprint allocation cap fingerprint_budget_refuses_growth_before_allocation
Scan elapsed cap scan_limits_are_strict_and_default_covers_full_msmarco
Duplicate handling at the cap compact_fingerprints_deduplicate_before_applying_key_limit
Restored standalone timeout behavior standalone_preserves_default_socket_timeouts_and_explicit_setters
Strict library environment parser strict_cluster_constructor_matches_cli_contract
Fresh index absent / owned prefix empty test_binary_redis_text_tag_fresh_and_retained_reuse
OpenSearch dynamic mapping test_binary_opensearch_text_tag_mapping_and_reuse
Foreign scalar / excluded multifield / SORTABLE-UNF storage old_and_partially_disabled_indexes_are_rejected
Stable slot map topology_requires_every_slot_and_does_not_count_replicas
Migration refusal migration_between_nonseed_nodes_refuses_even_an_empty_prefix
Dedicated cluster pipeline cluster_pipeline_routes_every_primary_and_preserves_payloads_and_errors

Each proof required test result: FAILED, not a build failure, and sources were restored. Coordinator INFO/CONFIG remains coordinator-scoped; we do not claim a cluster-wide commandstats aggregation. Keyed pipeline errors independently propagate and invalidate uploads. Retry-window tuning and test panic cleanup remain follow-ups, not additional correctness claims.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

Docker Build Validation

Platforms tested:

  • linux/amd64 (built and tested)
  • linux/arm64 (build validated)

Tests performed:

  • --help command
  • --describe datasets command
  • Integration test (random-100 dataset + Redis)
  • Multi-platform build validation (amd64 + arm64)

@fcostaoliveira
fcostaoliveira merged commit 42eabd4 into master Nov 3, 2026
38 checks passed
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.

1 participant