fix: EmbeddingsCache batch writes silently dropped on async Redis Cluster - #697
Draft
vishal-bala wants to merge 1 commit into
Draft
fix: EmbeddingsCache batch writes silently dropped on async Redis Cluster#697vishal-bala wants to merge 1 commit into
vishal-bala wants to merge 1 commit into
Conversation
…ster Queueing a command on a redis-py async pipeline is synchronous and returns the pipeline itself. Awaiting that return value calls `ClusterPipeline.__await__` -> `initialize()`, which does `self._command_queue = []`, so `execute()` returned `[]` and nothing was ever sent. `amset` still returned every key, so callers believed the write succeeded — silent data loss. `amexists_by_keys` returned `[]` rather than one bool per key. Standalone pipelines tolerate the await, which is why this only broke on Redis Cluster. Removing the bare `# type: ignore` on that line matters as much as the fix itself: it was suppressing the exact mypy error describing the bug, so `make check-types` now catches a reintroduced `await`. Also queue each entry's EXPIRE alongside its HSET in `mset`/`amset`. TTLs were previously applied in a sequential loop after `execute()`, costing N extra round trips and leaving a window where a crash left entries with no expiry — HSET alone does not touch a key's TTL, so nothing would ever have reclaimed them. Verified against a 3-primary Redis 8.4 cluster on redis-py 7.4.0: pre-fix `scan_iter` finds 0 of 10 written keys, post-fix 10. TTLs confirmed for explicit, cache-default, and no-TTL cases, and refreshed on rewrite. The added unit test is hermetic and runs in default CI. That matters because `requires_cluster` tests are skipped in every CI job — two tests in the cluster file had been broken for ~8 months by the #452 `text` -> `content` rename without anyone noticing. Those stale kwargs are fixed here as well.
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
1 participant
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.
Problem
On an async Redis Cluster client,
EmbeddingsCache.amset()returned every cache key while writing nothing. Callers had no way to tell — no error, no partial result.Queueing a command on a redis-py async pipeline is synchronous and returns the pipeline itself. So
await pipeline.hset(...)awaits the pipeline, which callsClusterPipeline.__await__→initialize()→self._command_queue = []. The queue is wiped,execute()returns[], and nothing is ever sent.Standalone pipelines tolerate the
await(their__await__just returnsself), which is why this was cluster-only and went unnoticed.The same pattern in
amexists_by_keys()made it return[]instead of one bool per key.Fix
Drop the
awaiton the two queueing calls; onlyexecute()is awaited.Removing the bare
# type: ignoreon that line matters as much as the fix: it had been suppressing the exact mypy error describing this bug.make check-typesnow fails if theawaitis ever reintroduced.Also included
mset/amset. TTLs were applied in a sequential loop afterexecute()— N extra round trips, plus a window where a crash left entries with no expiry at all.HSETalone does not touch a key's TTL, so nothing would ever have reclaimed them.text=, renamed tocontent=in feat: Add support for multimodal embeddings in vectorizers #452, so they had been raisingTypeErrorfor ~8 months.requires_clustertests run in no CI job, so nobody saw it.CONTRIBUTING.mdnow documents how to run the cluster tests.Testing
Verified against a 3-primary Redis 8.4 cluster on redis-py 7.4.0: pre-fix
scan_iterfinds 0 of 10 written keys, post-fix 10. TTLs confirmed for explicit, cache-default, and no-TTL cases, and refreshed on rewrite.The new unit test is hermetic and runs in default CI — deliberately, since the cluster suite does not. It uses a fake pipeline rather than redis-py internals, because CI matrixes redis-py 5.x/6.x/7.x and the queue attribute differs between them. Both it and the cluster test fail when the
awaitis restored.Net zero new cluster tests: the batch coverage folds into the existing async cluster test.
Follow-up (not in this PR)
No CI job passes
--run-cluster-tests, so ~17 cluster tests have never run. A single non-blocking job would close that gap;tests/cluster-compose.ymlalready exists for it.