Skip to content

fix: tolerate matched docs with missing field payload (Redis 8.8 expiry race) - #653

Open
vishal-bala wants to merge 4 commits into
mainfrom
fix/missing-field-payload-result-parsing
Open

fix: tolerate matched docs with missing field payload (Redis 8.8 expiry race)#653
vishal-bala wants to merge 4 commits into
mainfrom
fix/missing-field-payload-result-parsing

Conversation

@vishal-bala

@vishal-bala vishal-bala commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Summary

Harden RedisVL result parsing so a matched document with a missing field payload is tolerated (skipped) instead of raising or surfacing a partial record. This is client-side future-proofing for a Redis 8.8+ server-side race and the durable fix behind the recent CI flakiness on redis:latest.

Background

Redis 8.8 changed a RediSearch default: the query worker pool now runs a nonzero number of background worker threads. With a background executor active, a document (or an indexed hash field) that expires via TTL/HPEXPIRE at the exact instant a background FT.SEARCH materializes it is returned as a matched id with a nil field array instead of being dropped server-side. redis-py collapses that into a Document carrying only {"id": ..., "payload": None} — no exception — so the malformed record lands in RedisVL, which previously raised KeyError in the vector-normalize branch or leaked a partial record into CacheHit/ChatMessage/RouteMatch. The prior mitigation (--search-workers 0 in CI) only hid this; real users on Redis 8.8 with TTLs are still exposed.

What changed

  • Core parser (redisvl/index/index.py): process_results now skips a matched doc whose field payload is missing, detected only on zero-false-positive signals — a vector/range query missing its always-present vector_distance, or a JSON full-object unpack missing its json key. A plain hash FilterQuery/TextQuery doc is passed through as an id-only dict rather than dropped, so legitimate id-only projections and INDEXMISSING results are never over-skipped. process_aggregate_results and the hybrid/SQL paths drop entirely-empty rows.
  • Extension guards: the semantic cache, message history, and semantic router independently tolerate an id-only/incomplete record (skipping it) for the paths the core rule intentionally doesn't cover.
  • Completeness signal: SearchIndex.query() now returns a SearchResults object — a drop-in list subclass that additionally exposes dropped_count and complete, so callers that need completeness guarantees (audit, eval harnesses, agents) can detect and react to a race-shortened result set. Fully backward compatible: it behaves exactly like a list everywhere else.
  • Observability: skips are logged at WARNING level, aggregated once per query.
  • Docs: a new "Results and expiring documents" section in docs/concepts/queries.md explains the behavior, the detection boundaries, and how to use results.complete / dropped_count.

Behavior and compatibility

Backward compatible for every caller not hitting the race. Under the race, result counts become best-effort: query() may return fewer than num_results, paginate() fewer than page_size, and CountQuery.total can exceed the number of materialized documents (it reflects the server's match count). Silent skip is the default; no config knob is added.

Testing

Deterministic unit tests simulate the malformed response (a Result with a None fields slot / hand-built Document), covering every raise-site and skip branch, an over-skip guard proving legitimate id-only results survive, the SearchResults signal, and the extension guards. CI stays pinned at --search-workers 0; a live REDIS_SEARCH_WORKERS>0 reproduction is documented as a manual step. make check-types and make format are clean; unit and integration query tests pass.

Follow-ups (out of scope)

A process-level skip metric/callback and semantic-cache race-miss vs. real-miss decomposition, a nightly workers>0 integration test to guard the undocumented server shape, optional router detect-and-retry for the top-1 mis-route case, and raw=True message-history hardening.


Note

Medium Risk
Changes default query result handling and extension behavior under a rare server race; mis-detection could drop valid sparse hash results, though guards intentionally keep id-only hash matches.

Overview
Hardens RedisVL against a Redis 8.8+ RediSearch race where background workers can return matched document ids with empty field payloads when TTL/expiry coincides with FT.SEARCH, which previously caused KeyError, validation failures, or partial records downstream.

Core parsing (process_results, aggregates, hybrid/SQL paths) now skips or drops those rows when detection is unambiguous (e.g. missing vector_distance on scored vector queries, missing json on full JSON unpack, empty aggregate/hybrid/SQL rows), logs a single WARNING per query, and returns a SearchResults list subclass with dropped_count and complete so callers can spot shortened result sets without breaking list-shaped APIs. Hash FilterQuery/TextQuery id-only matches are still passed through to avoid false positives.

Extensions add defense-in-depth: semantic cache skips invalid CacheHit rows (no TTL refresh), message history skips bad ChatMessage construction, semantic router drops incomplete aggregate rows before routing.

CI/docs: docker-compose now passes --search-workers on the redis-server command line (fixing a no-op env-based pin); integration tests compare vector results by user key instead of order; new unit tests cover parsers and extensions; queries docs describe expiring-document behavior and SearchResults.

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

@vishal-bala vishal-bala added the auto:patch Increment the patch version when merged label Jul 23, 2026
@vishal-bala
vishal-bala marked this pull request as ready for review July 24, 2026 13:12
vishal-bala and others added 2 commits July 29, 2026 15:12
…ry race)

Redis 8.8 defaults the RediSearch worker pool to a nonzero background
executor, exposing a race where a document expiring (TTL/HPEXPIRE) mid
FT.SEARCH is returned as a matched id with a nil field array. redis-py
collapses this to a Document with only id/payload, so RedisVL previously
raised KeyError in the vector-normalize branch or leaked a partial record
into CacheHit/ChatMessage/RouteMatch.

process_results now skips such documents, detected only on zero-false
positive signals (vector/range query missing vector_distance; JSON
full-object unpack missing json), leaving legitimate id-only and
INDEXMISSING results untouched. process_aggregate_results and the
hybrid/SQL paths drop entirely-empty rows, and the semantic cache, message
history, and router guard the paths the core rule does not cover.

query() now returns a SearchResults list subclass exposing dropped_count
and complete so callers can detect a race-shortened result set; it behaves
exactly like a list otherwise. Skips are logged at WARNING level. Adds
deterministic unit tests and a docs section.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
test_simple compared two independent FT.SEARCH result sets positionally.
john and mary share the query vector (vector_distance == 0.0), so their
relative order can differ between the two calls, making the test flaky
(observed on Python 3.12 / redis-py 6.x / redis:8.4). Compare the result
sets keyed by the unique `user` field instead. Applied to the sync and
async twins.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
@vishal-bala
vishal-bala force-pushed the fix/missing-field-payload-result-parsing branch from c554a07 to d9340cf Compare July 29, 2026 13:57
The CI mitigation pinned --search-workers 0 via the REDIS_ARGS env var, but
the official redis image does not honor REDIS_ARGS (it is a Redis Stack
entrypoint convention). Verified empirically: with REDIS_ARGS set, redis:8.4
reports search-workers 0 (its default anyway) while redis:8.8.0 reports 12 --
the env var is ignored. So the pin was a silent no-op and the background-worker
race stayed live on redis:latest, which is why the mitigation never resolved
the flaky redis:latest runs (e.g. test_simple, test_filter_combinations).

Pass the flags on the server command line instead, which the official image
does honor. Confirmed redis:8.8.0 then reports search-workers 0 and the
integration suite passes against redis:latest. REDIS_SEARCH_WORKERS still
overrides (and now actually takes effect) for reproducing the race locally.

Co-Authored-By: Claude Opus 4.8 <[email protected]>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 6d065e1. Configure here.

Comment thread redisvl/index/index.py Outdated
process_aggregate_results popped __score and then dropped any row that became
an empty dict, so a legitimate scoring-only aggregation row (only __score) was
silently dropped and miscounted as a race-related missing-payload row. Judge
emptiness before stripping __score: drop only rows that came back with no
fields at all; keep a score-only row as {} (matching prior behavior).

Co-Authored-By: Claude Opus 4.8 <[email protected]>
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

Development

Successfully merging this pull request may close these issues.

1 participant