fix(index): use UNLINK instead of DEL in SearchIndex.drop_keys#616
fix(index): use UNLINK instead of DEL in SearchIndex.drop_keys#616Joshuaakaspace wants to merge 1 commit into
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. |
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.
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.