fix: de-duplicate cursor keys in update_by_filter

[vishal-bala]

Problem

tests/integration/test_bulk_operations.py was intermittently failing in CI across four matrix cells on code that does not touch bulk operations, blocking unrelated PRs. The failures all had matched=60 correct but processed varying non-deterministically — assert 87 == 60, assert 107 == 60, assert 25 == 60.

This is a real bug, not test isolation. Each xdist worker already gets its own Redis container via a per-worker COMPOSE_PROJECT_NAME, so cross-test contamination was never possible. The bug reproduces serially against a single Redis in under a minute.

Root cause

An FT.AGGREGATE ... WITHCURSOR cursor is a position in an ascending walk of internal document ids, not a snapshot of the match set, so it can return the same key on more than one page. Confirmed at raw protocol level on Redis 8.4.4 (search-workers=0) — a 60-document match set comes back as 61-72 rows, with the extra rows arriving on the final page:

pages(header,rows)=[(1, 25), (1, 25), (1, 21)]   total=71  distinct=60  -> 11 duplicates

update_by_filter wrote and counted each emission, so documents were written twice and processed was inflated. matched stayed correct because the count query is a single round-trip with no cursor.

Two mechanisms, both measured:

RediSearch has no in-place update. Any write to a document the index covers tombstones its old document id and appends a brand-new, higher one. max_doc_id climbed 12→13→14→15 across three single-field HSETs while num_docs held at 10, and the touched document moved to the end of the row order each time — true even when the written field is not in the schema, and even writing the identical value back. So a document rewritten while a cursor is open is removed from behind the cursor and re-added in front of it.

A plain pipelined bulk load creates tombstones on its own. Loading 120 fresh documents gave max_doc_id=168 for num_docs=120 — 48 documents were re-indexed during ingestion (num_records corroborates exactly: 504 = 360 live + 48×3 orphaned). One-by-one loading gave only 4. Duplication is therefore a transient post-write settling effect:

when the cursor reads	runs with duplicates
immediately after the load	7/15
after a 2-second pause	0/15

Which is precisely the shape of the failing test: create(overwrite=True, drop=True) → load 120 documents → immediately update_by_filter. Severity tracks outstanding tombstone churn — load only 4/15, load + updates 7/15, load + prior delete/reload 11/15, settled 0/15.

Fix

De-duplicate keys across cursor pages in both the sync and async _iter_keys_by_filter, via a shared _dedupe_keys helper that also collapses repeats within a page.

harness	before	after
sync update_by_filter × N	11/30 bad	0/40
async test path × N	26/60 bad	0/80
test_bulk_operations.py -n 4 × 5	—	all green

The de-duplication is load-bearing beyond the count: because every write tombstones a document id, redundant writes manufacture the very index churn that provokes further re-emission. drop_by_filter needs none of this — it re-queries from offset 0 and counts actual UNLINK returns, so repeats are absorbed, because deletes shrink their own match set. An update does not, which is why the same strategy doesn't transfer.

Alternatives considered and rejected

Every memory-free alternative relocates O(n) state from the client's private heap onto a shared resource. For a library embedded in many applications, the client heap is the only O(n) budget it legitimately owns.

• Streaming (write what you read, O(batch) memory) — does not terminate. Each write re-appends the document ahead of the cursor, which reads it again: 3,025 writes for 60 documents across 121 pages, still going when capped. In any streaming variant the seen set stops being a counting nicety and becomes the termination guarantee — and it is still O(match count), so there was never an O(batch_size) option to trade for.
• Keyset/seek pagination (FILTER @__key > '<last>' + SORTBY @__key) — verified working and duplicate-free by construction, but FILTER is post-query and documented as "not indexed", and LOAD @__key is documented as "the equivalent of HMGET against a Redis key" per record. Keyset pays that full scan per page: ~10⁹ record-visits at 1M documents versus ~10⁶ for one cursor walk. Also PARAMS does not reach FILTER expressions, so the watermark must be string-interpolated into an expression DSL with unspecified escaping — on the one code path whose job is mutating data, with keys that embed user-controlled ids. Worth revisiting only as an opt-in resumability feature.
• Lua-side SADD dedup — exact with O(1) client memory, but the marker set is an eligible eviction victim under allkeys-*, so dedup would silently stop exactly under memory pressure. A TTL for crash cleanup makes it a victim under volatile-* too. Plus CROSSSLOT on Cluster.
• Server-side GROUPBY @__key — a blocking stage that materialises the whole match set server-side before emitting row one.

Memory cost of the shipped fix, measured (40-char keys): the seen set adds 32 MB per million keys on top of ~84 MB the keys list and its strings already occupied — a ~38% constant, no change in complexity class. The 1M-match case is the scenario the docs already tell users to partition.

Docstring corrections

Two pre-existing claims that measurement disproved:

• "the aggregation cursor cannot safely be read while the index is being written" — it can: it neither errors nor loses rows. It simply doesn't terminate. The phase split is a termination requirement, not a safety one, and now says so.
• processed is at-most-once, not exactly-once. A cursor may also return fewer rows than matched (Redis documents both a partial result when it reports cursor id 0, and missing rows under load rebalancing), so a completed run is not proof that every matching document was seen. Corrected in the docstring and in docs/concepts/search-and-indexing.md.

Tests

Two unit tests replay canned cursor pages, and one integration test covers the end-to-end contract with a real count query and real writes. Canning is deliberate: real re-emission reproduces only ~4/15 of the time, so a live-server test would carry a ~60% false-pass rate.

The test set was chosen with a mutation matrix rather than by taste. Seven candidate tests were dropped for adding no unique coverage — including one that asserted order preservation and still passed when the implementation was replaced with an order-destroying set comprehension, and one whose only mutation was already caught by two pre-existing tests. The surviving three each catch a defect nothing else does (sync-only regression, async-only regression, end-to-end processed accounting), and the reduced set has zero uncaught mutations where the larger set had one.

Existing assertions were not loosened — processed == 60 is the documented contract.

Follow-ups (not in this PR)

• completed=True is returned unconditionally by update_by_filter even though processed can silently undercount. completed = len(keys) >= matched would close that for free, but it changes a documented contract.
• Related bugs in the same family found while auditing, each spun off separately: BaseCache.clear never advancing its SCAN cursor on Cluster; paginate treating an empty page as exhaustion; delete_route_references raising on duplicate SCAN keys; migration validation comparing indexed-document enumeration against a SCAN-by-prefix count.

---

Note

Medium Risk
Changes bulk partial-update behavior on a data-mutation path; risk is mitigated by focused unit/integration tests and clearer processed semantics, but callers relying on the old inflated counts would see different numbers.

Overview
Fixes update_by_filter inflating BulkResult.processed when RediSearch’s FT.AGGREGATE ... WITHCURSOR returns the same document key on multiple pages (cursor walks internal doc ids, not a snapshot; writes can re-append ids ahead of the cursor).

Adds _dedupe_keys and a per-iteration seen set in sync and async _iter_keys_by_filter, so each matching key is written and counted at most once. Docstrings and docs/concepts/search-and-indexing.md now describe at-most-once processed, possible processed vs matched gaps, extra client memory for the dedup set (~120 MB/M keys guidance), and why read-then-write must stay separated.

Tests: new unit coverage in tests/unit/test_bulk_cursor_dedup.py and an integration test that replays canned aggregate pages to assert single writes per document.

^{Reviewed by Cursor Bugbot for commit 04c7d2a. Bugbot is set up for automated code reviews on this repo. Configure here.}

[tylerhutcherson]

Wouldn't this be a bug in our cursor processing logic in Redis Search?

[tylerhutcherson]

I think this is a worthwhile workaround, but I would make sure we flag the issue to the search team on the cursor duplicates. That seems like it should not be a thing and we are having to absorb it with some less than ideal patterns that could leak memory to client application to a level that is not transparent at scale.

[vishal-bala]

I don't know that it qualifies as a bug - it's kind of a natural side-effect of the way that cursors work in Redis and us faking a single client-side op with several server-side ops. It's definitely something that isn't documented so clearly as a possible behaviour of SCAN, so that's something worth flagging.

[applied-ai-release-bot]

🚀 PR was released in v0.25.1 🚀