fix(index): use UNLINK instead of DEL in SearchIndex.drop_keys - #616
Merged
vishal-bala merged 1 commit intoJun 23, 2026
Merged
Conversation
DEL reclaims memory on the main thread, so a single drop_keys call over a large key set stalls Redis proportionally to the freed keyspace. For SemanticCache use cases where scope-targeted invalidation routinely sweeps 10K to 1M+ keys (for example, a policy version rollover in a multi-tenant deployment), this is a customer-visible latency event on every invalidation. UNLINK has the same return semantics as DEL and is available on Redis 4+, so it is a strict superset for this use case. The only observable difference is that reclaimed memory is reported lazily by MEMORY USAGE, which is the point. Applies to both SearchIndex.drop_keys and AsyncSearchIndex.drop_keys. SemanticCache.drop() flows through this path via the keys= argument. Refs redis#600
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR changes SearchIndex.drop_keys and AsyncSearchIndex.drop_keys to use Redis UNLINK instead of DEL, reducing main-thread blocking during large key invalidations while preserving deletion count semantics.
Changes:
- Replaced sync and async
drop_keysRedis calls fromdeletetounlink. - Updated docstrings to explain the use of
UNLINK. - Added unit regression tests verifying
unlinkis used instead ofdelete.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
redisvl/index/index.py |
Switches sync and async drop_keys implementations to call UNLINK and documents the behavior. |
tests/unit/test_drop_keys_unlink.py |
Adds mock-based tests for single-key and multi-key sync/async drop_keys paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
vishal-bala
approved these changes
Jun 23, 2026
vishal-bala
left a comment
vishal-bala
left a comment
Collaborator
There was a problem hiding this comment.
LGTM 👍
(Sevice tests passed but didn't add the status check to the PR - see results here: https://github.com/redis/redis-vl-python/actions/runs/28022680616)
|
🚀 PR was released in |
applied-ai-release-bot
Bot
added
the
released
vishal-bala
added a commit
that referenced
this pull request
Sep 9, 2026
…ster (#703) Two independent bugs with one root cause: four modules hand-rolled a `SCAN` cursor loop, and a cluster client's `SCAN` reply is not a cursor. `RedisCluster.scan` broadcasts `SCAN` to every primary and replies with a `{node_name: cursor}` mapping of node-local cursors. A single value cannot be broadcast back and the mapping cannot be handed to `scan(cursor=...)`. Every hand-rolled loop in the codebase got this wrong, in one of two ways. ## Why **`BaseCache.clear`/`aclear` spins forever and leaves the cache populated.** `cursor_int == 0` was never true against the mapping, the `Mapping` branch only broke once every node reported 0, and the `else` that advanced the cursor was unreachable — so the cursor stayed 0 and `SCAN 0` was re-issued indefinitely. Affects `SemanticCache` and `EmbeddingsCache`; neither overrides `BaseCache`. The reason this shipped is worth stating, because it is what made the bug invisible to tests. Re-issuing `SCAN 0` *accidentally* makes progress when every key in the DB matches the cache prefix: each round deletes the first page, the keyspace shrinks, and the loop drains. A cache that is the entire keyspace therefore clears fine even with the bug present. The genuine hang needs keys that do **not** match the prefix — the normal case, since RedisVL shares a keyspace between index documents, caches and application data. Then a `SCAN 0` page can match nothing, nothing is deleted, and the loop makes zero progress. Measured against a real 3-primary Redis 8.4 cluster with 50k unrelated keys and 200 cache keys: **20,000 `SCAN` calls without terminating, 197 of 200 cache keys orphaned.** **Six migration `SCAN` loops had the same bug, and worse.** They fed the reply cursor straight back into `client.scan(cursor=...)`, so the second iteration passes a dict and redis-py raises `DataError: Invalid input of type: 'dict'`. Verified against a real cluster. Unlike the cache hang this is a hard crash on the second `SCAN`, not a silent under-count, and it fires for any cluster keyspace regardless of what the keys look like — **arguably the wider blast radius of the two**, even though it was only found while chasing the cache hang. Reachability, most to least exposed: | site | when it runs | | --- | --- | | `validation._count_index_keys` | every `validate` run | | `async_validation._count_index_keys` | every `validate` run | | `async_planner._async_sample_keys` | `count=max(limit, 10)` rarely fills the limit on the first page | | `planner._sample_keys` | `count=max(limit, 1000)` usually returns early, but not when the keyspace is smaller than the limit | | `executor._enumerate_with_scan` | fallback paths only | | `async_executor._enumerate_with_scan` | fallback paths only | ## What changed All seven sites are plain "enumerate keys matching a pattern", so each collapses to redis-py's `scan_iter`, which already drives each primary on its own cursor via `target_nodes`. Cluster cursor semantics move upstream where they belong. That also drops the sync/async duplication in `clear` and both `# type: ignore` comments in `base.py` (a third goes from `planner.py`); none are added back, and `mypy ./redisvl` is clean. | | | | --- | --- | | `redisvl/extensions/cache/base.py` | `clear`/`aclear` become 9 lines each on `scan_iter`. Deletes batch at `CLEAR_BATCH_SIZE = 500` rather than one `DEL` per `SCAN` page. | | `redisvl/migration/{planner,executor,validation}.py` + async twins | Six loops → `scan_iter`. The sample sites gain a bonus: a generator lets the sample limit stop mid-page instead of draining the page first. | | `redisvl/utils/utils.py` | `scan_by_pattern` widened from `Redis` to `SyncRedisClient`. It was already cluster-correct via `scan_iter`; only the annotation said otherwise. | The `clear`/`aclear` docstrings now document what callers actually get: `SCAN` is not a point-in-time snapshot, so this is a best-effort sweep and not an atomic flush — concurrent writers may or may not be swept, duplicate pages are harmless because `DEL` on a gone key is a no-op, and a mid-sweep failure is safe to retry because the operation is idempotent. Worth reviewing as prose, not just as comments: `docs/api/cache.rst` autodocs both caches with `:inherited-members:`, so `BaseCache.clear`'s docstring **is** the published API reference text. ## Commit split Five commits, each independently reviewable, in an order where every fix is separable from its tests: 1. `fix(cache):` the cache hang — `base.py` plus its unit tests. 2. `fix(migration):` the six migration loops — a different subsystem with a different failure mode, and reviewable without an opinion on the cache. 3. `test(cluster):` repairs the pre-existing cluster cache tests, described below. 4. `fix(migration):` an `aclosing` follow-up on `_async_sample_keys` — see below. 5. `test:` the mutation-driven trim of the cache suite and the new migration regression file. Commit 4 exists because `scan_iter` introduced a generator where a `while` loop had none: `_async_sample_keys` returns from inside `async for` once the sample limit is reached, and an async generator abandoned that way is not closed until loop shutdown. That surfaced as `RuntimeWarning: coroutine method 'aclose' ... was never awaited` in the migration tests. Wrapped in `contextlib.aclosing`. The sync planner has the same early return but a plain generator is closed deterministically by refcounting on CPython and emits no warning, so it is left alone. ## Tests **Two pre-existing cluster tests had never executed.** `test_embeddings_cache_cluster_sync`/`_async` passed `text=` to `EmbeddingsCache.set`/`aset`, whose parameter is `content`. They raised `TypeError` on their first statement — which is part of why the cluster clear hang shipped. Both also called `clear()` with no assertion afterward, so even once repaired they would not have caught it. The two new cluster regression tests (sync + async) therefore seed both unrelated keys *and* a cache larger than one `SCAN` page, because of the accidental-progress effect above — 100 keys, what the existing test used, is exactly the size that passes either way. They bound the number of `SCAN` calls `clear()` may issue, since a regression hangs rather than fails and `pytest-timeout` is not installed, and they assert paging actually happened so the test cannot pass vacuously. Key counting goes through `scan_iter`, never `KEYS` or `DBSIZE`: those route to a single node on a cluster and silently report roughly one shard's worth (measured: 201 of 600). **Unit coverage, at both ends.** `tests/unit/test_migration_cluster_scan.py` (4 tests) is new because reverting all six migration modules left 65/65 existing tests green — that fix had no regression coverage at all. `tests/unit/test_cache_clear_cluster_cursor.py` went the other way: mutation testing showed that of 12 cache tests only 3 killed anything no other test killed and two killed nothing, so it is trimmed to 5 with a strictly larger kill set. Now that the cursor walk lives in redis-py, most of what the dropped tests asserted was upstream's behavior. `test_deletes_are_batched` is kept deliberately — it is the only thing standing between us and `client.delete(*list(client.scan_iter(...)))`, which would OOM on a large cache — and now patches `CLEAR_BATCH_SIZE` instead of depending on its value. Both fake clients bind redis-py's *real* `scan_iter` over their own `scan`, so the tests drive the actual library loop rather than a reimplementation of it. The migration fake replies with per-node cursors and raises `DataError` on a dict cursor, so the old loop fails there exactly as it fails against a real cluster. `1411 passed, 1 skipped` on the unit suite; `mypy`, `black` and `isort` clean. ## Not in scope **There is no CI signal for any of this.** `--run-cluster-tests` is passed by no workflow in `.github/workflows/` and `make test` never sets it, so all 20 `requires_cluster` tests — including the 2 added here — are skipped in CI. The unit tests above are what actually guards these fixes on every run; the cluster tests only ran locally. A CI job that passes the flag is the obvious follow-up, but it is a cost/runtime decision (6 containers per run) and is left to a separate PR. **`clear()` uses `DEL` where `drop_keys` uses `UNLINK`.** `SearchIndex.drop_keys` was deliberately moved to `UNLINK` in #616 (issue #600) to avoid stalling the server on bulk deletes. `BaseCache.clear` on a large cache has the same blocking profile and was not changed here, to keep this PR to the cursor bug. Worth its own issue. **`EmbeddingsCache.amset` silently writes nothing on an async cluster client** — it awaits the pipeline object returned by the queueing call, which drains the queue before `execute()`. That is a separate bug being fixed separately. The async cluster test here deliberately seeds with `aset` instead and says so in a comment, since this test is about `aclear`. Adjacent but untouched: #601 (`drop_keys` does not validate cluster hash-tag co-location) is the same "written against standalone, wrong on cluster" family, if you want a theme for a follow-up sweep. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches shared cache clear and migration validation/enumeration paths used in production; behavior changes from broken cluster SCAN to correct iteration, with documented non-atomic clear semantics. > > **Overview** > Fixes **Redis Cluster** breakage from hand-rolled `SCAN` loops: cluster `SCAN` returns per-node cursor maps, not a single integer cursor. > > **`BaseCache.clear` / `aclear`** now enumerate prefix keys with redis-py's **`scan_iter`** and delete in batches of **`CLEAR_BATCH_SIZE` (500)** instead of a broken cursor loop that could spin forever when unrelated keys share the keyspace. Docstrings spell out best-effort, non-atomic clear semantics. > > **Migration** (sync/async planner, executor, validation) replaces six similar loops with **`scan_iter`** so validation key counts, key sampling, and SCAN fallbacks no longer **`DataError`** on the second iteration. **`AsyncMigrationPlanner._async_sample_keys`** wraps early-exit sampling in **`contextlib.aclosing`** to avoid un-awaited `aclose` warnings. > > **`scan_by_pattern`** is typed as **`SyncRedisClient`** (already used `scan_iter`). Tests add cluster-style fakes and multi-page clear regressions; existing cluster embedding cache tests use **`content=`** and assert post-clear emptiness. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit efbd3d0. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Switch
SearchIndex.drop_keysandAsyncSearchIndex.drop_keysfromDELtoUNLINKso memory reclamation runs on a background thread.Refs #600
Motivation
DELreclaims memory on the main thread, so a singledrop_keyscall over a large key set stalls Redis proportionally to the freed keyspace. ForSemanticCacheuse cases where scope-targeted invalidation routinely sweeps 10K to 1M+ keys (for example, a policy version rollover in a multi-tenant deployment), this is a customer-visible latency event on every invalidation. The issue thread on #600 lays this out in more detail, including the path throughSemanticCache.drop()which callsdrop_keysfor thekeys=argument.UNLINKis a strict superset ofDELfor this code path. It returns the same count semantics and has been available since Redis 4, which is well below the supported floor for current RedisVL targets. The only observable difference is that reclaimed memory is reported lazily byMEMORY USAGE, which is the point.Changes
redisvl/index/index.py:SearchIndex.drop_keys(sync) now callsself._redis_client.unlink(...)instead ofself._redis_client.delete(...). Docstring updated to note the choice.redisvl/index/index.py:AsyncSearchIndex.drop_keys(async) now callsclient.unlink(...)instead ofclient.delete(...). Docstring updated to note the choice.tests/unit/test_drop_keys_unlink.py: new mock-based regression tests covering single-key and list-of-keys paths on both sync and async indexes, assertingunlinkis called anddeleteis not.drop_documentsis intentionally left unchanged in this PR. It is a related but separate API (it also applies the index prefix and validates hash-tag co-location on cluster) and #601 is tracking the consistency story there. Keeping this PR scoped to thedrop_keyschange matches the framing in #600.Testing
All run locally with
python -m uv run pytest .... Docker was running fortestcontainers-backed integration tests.tests/unit/test_drop_keys_unlink.py):upstream/mainwithAssertionError: Expected unlink to have been [a]waited once. Called 0 times.on all 4 cases.tests/unit/: 836 passed, 1 skipped on baseline; 840 passed, 1 skipped on this branch. The +4 is the new regression suite. No previously-passing test regressed and no new skips.tests/integration/test_search_index.pyandtests/integration/test_async_search_index.py: 93 passed (covers the originaltest_search_index_drop_keyscases on both sync and async).tests/integration/test_llmcache.pyandtests/integration/test_semantic_router.py: 92 passed, 1 skipped. These exerciseSemanticCache.drop()andSemanticRouterwhich transitdrop_keysunder the hood.make-equivalent linting:isort --check-only,black --check, andmypy redisvlall clean.Verification log tail
Notes
drop_documentsconsistency work from SearchIndex.drop_keys does not validate cluster hash-tag co-location (inconsistent with drop_documents) #601 in a separate PR, or to widen this one if you would prefer a single change for the pair. I left them apart because the cluster hash-tag validation in SearchIndex.drop_keys does not validate cluster hash-tag co-location (inconsistent with drop_documents) #601 is a different shape of fix.Note
Low Risk
Low risk: a small, backwards-compatible change that swaps
DELforUNLINKto reduce Redis blocking during bulk key deletion; behavior differences are mostly limited to asynchronous memory reclamation timing.Overview
Switches
drop_keysto non-blocking deletion.SearchIndex.drop_keysandAsyncSearchIndex.drop_keysnow call RedisUNLINKinstead ofDEL, and their docstrings document the rationale (avoid blocking Redis when dropping large key sets).Adds a unit regression suite (
tests/unit/test_drop_keys_unlink.py) asserting both sync and asyncdrop_keyspaths callunlink(single key and list) and never calldelete.Reviewed by Cursor Bugbot for commit 4fbc650. Bugbot is set up for automated code reviews on this repo. Configure here.