Repository navigation
perf(embeddings-cache): refresh and set TTLs in the same round trip as each lookup and write - #764
Open
vishal-bala wants to merge 3 commits into
Open
vishal-bala wants to merge 3 commits into
vishal-bala wants to merge 3 commits into
Conversation
mget_by_keys() and amget_by_keys() fetched a batch in one pipeline, then refreshed each hit's TTL with its own EXPIRE call, so N hits cost 1 + N round trips whenever a TTL was configured. This sits on the hot path of every vectorizer with an EmbeddingsCache attached. Queue EXPIRE next to each HGETALL in the existing non-transactional pipeline and keep only the HGETALL replies. EXPIRE on a missing key returns 0 without creating it, so misses need no special case. The TTL is resolved once and the EXPIREs are skipped when none is set. The pipeline stays non-transactional so cluster keys may span slots. Ten hits on Redis 8.4 over localhost: median 3.99 ms before, 0.93 ms after (500 runs each).
Address review feedback on the pipelined TTL refresh: - The fake pipeline now records the EXPIRE TTL, so the unit tests catch a wrong TTL value as well as a refresh outside the lookup pipeline. - Merge the sync and async integration tests into one parametrised test and inline its single-use helper. - Describe the whole unit module in its docstring, and note why the fake client defaults to transaction=True. - Trim the pipeline comments and drop the async-only note on awaiting, which amset already explains. EXPIRE is now also sent for misses. It is a no-op on a missing key, but it means a TTL-configured lookup needs EXPIRE permission even when the whole batch misses; before, only a hit needed it. Such a cache already needs EXPIRE for every write and every hit. A non-hash key in the batch also gets the TTL before HGETALL's WRONGTYPE error is raised.
…lines With a TTL configured, a get_by_key()/aget_by_key() hit ran HGETALL and then a separate EXPIRE, and set()/aset() ran HSET and then a separate EXPIRE: two round trips each. set() also briefly left a fresh entry with no expiry, the window mset() already closes. Delegate them to mget_by_keys()/amget_by_keys() and mset()/amset(), which queue the EXPIRE in the same non-transactional pipeline. Each call now costs one round trip and four duplicated bodies go away. get_by_key() and set() keep their own async-only warning, so the hint still names the method the caller used. As with the batch read, get_by_key() and aget_by_key() now send EXPIRE on a miss too when a TTL is set. It leaves a missing key untouched, but a credential without write access to the key now fails on a miss as well as a hit; the installation guide's ACL section now says so. Sync timings on Redis 8.4 over Docker Desktop localhost: get() 381 us before, 249 us after; set() 505 us before, 248 us after.
vishal-bala
marked this pull request as ready for review
October 15, 2026 08:38
This branch has not been deployed
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.
Motivation
When a TTL is configured, every
EmbeddingsCachelookup that hits refreshes the entry's expiry, and every write sets one. Until now each refresh was its own round trip.mget_by_keys()fetched the batch in one pipeline and then sent oneEXPIREper hit, so N hits cost 1 + N round trips;get_by_key()andset()cost two each. This is on the hot path of every vectorizer constructed withcache=, becauseembed()andembed_many()look up throughget()andmget()and write back throughset()andmset().set()also left a short window in which a new entry existed with no expiry, whichmset()already avoided.Measured on Redis 8.4.7 in Docker on localhost (about 280 µs per
PING), with 768-dimension embeddings and a 600 s TTL, median of 1,000 calls each:mainmget_by_keys(), 10 hitsget_by_key(), hitset()On a real network the saving grows with the round-trip time.
Changes
TTL refresh in the lookup pipeline
mget_by_keys()andamget_by_keys()now queueEXPIRE key ttlimmediately after eachHGETALLin the existing pipeline and keep every other reply, so the lookup and the refresh share one round trip. The TTL is resolved once, and with no TTL configured the pipeline is the same as before.EXPIREon a missing key returns 0 without creating the key, so misses need no special case. The pipeline stays non-transactional, so on Redis Cluster a batch can span slots.Single-key methods delegate to the batch methods
get_by_key()andaget_by_key()now callmget_by_keys()andamget_by_keys()with one key, andset()andaset()callmset()andamset()with one item. Each call costs one round trip, the write and its expiry are sent together, and four duplicated method bodies go away.get_by_key()andset()keep their own async-only warning, so the hint still names the method the caller used.Tests
EXPIREcarries. Cluster tests do not run in CI, so this is the guard against a transactional pipeline.mget_by_keys()andamget_by_keys(), and asserts that every hit's TTL is reset and that a missing key is not created. It fails if either path refreshes keys one at a time.test_sync_methods_warn_with_async_only_clientnow mocks the pipeline rather thanhgetall, and asserts that the warning namesget_by_key.Docs
EmbeddingsCachegetters fail on a miss, as well as a hit, under a lookup-only credential.Notes
EXPIREis now sent for misses as well as hits whenever a TTL is configured. That is a no-op on the data, but it means a credential without write access to the cache's keys now fails on a miss too, where previously it failed only on a hit. Such a credential could already never read a hit or write an entry, so no working setup breaks; the failure simply arrives earlier. The same reasoning covers a key under the cache prefix that is not a hash: it now receives the TTL beforeHGETALLraisesWRONGTYPE.SemanticCacheis unaffected here; its hit refresh is handled separately in #753, which touches different files and merges cleanly with this branch.Release Notes
EmbeddingsCachelookups and writes with a TTL configured, including those a vectorizer makes throughcache=, now refresh or set the expiry in the same round trip, so cached embedding calls are faster. With a TTL configured, a credential that cannot runEXPIREon the cache's keys now fails on a cache miss as well as on a hit.Note
Medium Risk
Hot-path Redis behavior changes when TTL is configured (including earlier ACL failures on cache misses); performance and semantics are intentional but affect production embedding caches.
Overview
EmbeddingsCachebatches TTL work with lookups and writes so vectorizercache=paths need fewer Redis round trips when a TTL is set.mget_by_keys/amget_by_keysnow queueEXPIREright after eachHGETALLin one non-transactional pipeline (including on misses, whereEXPIREis a no-op). Single-keyget_by_key,set, and async equivalents delegate to the batchmget/msethelpers so reads/writes and expiry share oneexecute().The installation guide ACL section notes that getters pipeline
EXPIREwith the lookup, so read-only credentials can fail on a miss as well as a hit when TTL refresh is enabled.Tests add pipeline unit coverage (sync + async), an integration check that TTL refresh stays inside the lookup pipeline, and updated async-only sync-method warning mocks.
Reviewed by Cursor Bugbot for commit dbc3172. Bugbot is set up for automated code reviews on this repo. Configure here.