feat: add Valkey as a vector database provider - #1
Conversation
Add Valkey (with the valkey-search module) as a first-class vector
database provider, selectable via VECTOR_DB=valkey. The change is purely
additive: default behavior is unchanged and the provider is only used
when explicitly configured.
- New Valkey provider extending VectorDatabase, registered in
getVectorDbClass(); mirrors the PGVector/Qdrant providers.
- Uses the official @valkey/valkey-glide client (server only, pinned)
and valkey-search (FT.CREATE/FT.SEARCH, HNSW, COSINE) with one index
per namespace (allm_idx_{ns}) over allm:{ns}: hash keys.
- Full parity: add/delete document, delete-namespace, reset, similarity
search with threshold + topN, filterIdentifiers exclusion, vector
cache path, namespace stats, optional TLS/auth, configurable timeout.
- Settings plumbing (updateENV, systemSettings with password masked),
env examples, commented opt-in docker-compose service, admin UI option.
- Unit tests (mocked client) and gated live integration tests against
valkey/valkey-bundle.
daric93
left a comment
There was a problem hiding this comment.
Review of the Valkey vector DB provider, focused on correctness against the base contract and the qdrant/pgvector reference providers. Ten inline findings below, roughly most-severe first. The top four (stale static client, raw-vs-normalized prefix mismatch, missing dimension check, and the never-wired validateConnection) are worth addressing before merge. Lower-priority cleanup not inlined: curateSources/distanceToSimilarity are byte-copies of pgvector that could live on the base class; the GLIDE-record branches in the _infoValue/_fieldsToObject/_parseSearchReply parsers are dead under the hardcoded RESP2 protocol; and ValkeyVectorDBRequestTimeout is registered in KEY_MAPPING but has no settings form field or systemSettings exposure (env-only).
Generated with Claude Code
| if (process.env.VECTOR_DB !== "valkey") | ||
| throw new Error("Valkey::Invalid ENV settings"); | ||
|
|
||
| if (!Valkey.#client) |
There was a problem hiding this comment.
Stale static client — connection setting changes need a process restart to take effect.
#client is created once from process.env and only rebuilt after an explicit disconnect(). When an admin changes VALKEY_VECTOR_DB_HOST / PASSWORD / USE_TLS via the settings UI, updateENV mutates process.env but never calls disconnect() (only a VECTOR_DB provider switch triggers resetAllVectorStores). So connect() keeps returning the cached client pointed at the old endpoint — every embed/search hits the wrong instance or fails auth until the server is restarted. Worse, validateConnection() builds a fresh probe client, so the admin "test connection" passes against the new config while live traffic still uses the old one.
pgvector and qdrant build a fresh client on every connect() and avoid this entirely.
Suggested fix: either drop the static cache and connect per call, or add the Valkey connection keys to a postUpdate hook in updateENV.js that calls new Valkey().disconnect() so the next connect() rebuilds from current env.
There was a problem hiding this comment.
Fixed — added a handleValkeyConnectionReset postUpdate hook on every Valkey connection key in updateENV.js that calls new Valkey().disconnect(), so the next connect() rebuilds the cached client from the updated env.
| // The HASH key prefix for all chunks in a namespace. Uses the raw namespace so | ||
| // the FT.CREATE PREFIX and the stored keys always line up exactly. | ||
| keyPrefix(namespace = "") { | ||
| return `allm:${namespace}:`; |
There was a problem hiding this comment.
keyPrefix uses the raw namespace while indexName uses the normalized one — they can diverge and silently lose data.
indexName() normalizes (allm_idx_${this.normalize(namespace)}) but keyPrefix() uses the raw namespace. Two namespaces that differ only by non-[a-zA-Z0-9_] characters (e.g. my-ws and my_ws) both map to index allm_idx_my_ws but keep distinct prefixes allm:my-ws: vs allm:my_ws:. Whichever calls getOrCreateIndex first wins the FT.CREATE PREFIX; the second sees the index already exists and returns early, so its chunks are written under a prefix the index never watches — permanently unsearchable, while addDocumentToNamespace still returns vectorized: true.
Derive the prefix from the same normalized token so the two can never diverge (all callers go through keyPrefix(), so reset()'s allm:* scan still matches):
| return `allm:${namespace}:`; | |
| return `allm:${this.normalize(namespace)}:`; |
There was a problem hiding this comment.
Fixed — keyPrefix() now returns allm:${this.normalize(namespace)}:, so it shares the exact normalized token with indexName() and can never diverge. All callers go through keyPrefix() and reset()'s allm:* scan still matches.
| * @param {number|null} dimensions | ||
| */ | ||
| async getOrCreateIndex(client, namespace, dimensions = null) { | ||
| if (await this.namespaceExists(client, namespace)) return; |
There was a problem hiding this comment.
No dimension check when the index already exists — an embedder change silently drops vectors.
getOrCreateIndex returns early if the namespace index exists and never compares the existing index DIM to the incoming vector dimension. If the embedding model changes (e.g. 384 → 1536), _upsertChunk HSETs wrong-length FLOAT32 buffers that valkey-search silently refuses to index. addDocumentToNamespace reports vectorized: true, but the chunks never become searchable, and a later FT.SEARCH with the new-dim BLOB errors or returns nothing — data loss with no surfaced error.
Suggested fix: FT.INFO already exposes the dimension (you parse other fields via _infoValue), so validate it here and throw a clear error on mismatch. Better still, add valkey to the pgvector branch of resetAllVectorStores (server/utils/vectorStore/resetAllVectorStores.js:33) so a provider/embedder reset drops all allm_idx_* indexes — Valkey indexes have a fixed per-index dimension just like the pgvector column.
There was a problem hiding this comment.
Fixed both ways — getOrCreateIndex now reads the existing index dimension from FT.INFO (via a defensive _indexDimension walker) and throws a clear error on mismatch, and valkey was added to the pgvector branch of resetAllVectorStores so a provider/embedder reset drops all allm_idx_* indexes via Valkey.reset().
| }, | ||
|
|
||
| // Valkey Vector DB Options (valkey-search module) | ||
| ValkeyVectorDBEndpoint: { |
There was a problem hiding this comment.
Valkey.validateConnection() is defined but never invoked — bad config is accepted silently.
All ValkeyVectorDB* entries use empty checks: [] and no preUpdate, unlike PGVector which wires validatePGVectorConnectionString via preUpdate. As a result the Valkey.validateConnection() method (valkey/index.js:761) is dead code: an admin can enter an unreachable host or wrong password, click Save, and updateENV persists it and reports success with no connection test. The failure only surfaces later at embed/search time as opaque connect errors.
Suggested fix: add a preUpdate validator (mirroring the PGVector pattern) on at least one Valkey key that calls Valkey.validateConnection(overrides) with the pending form values and surfaces the error to the UI.
There was a problem hiding this comment.
Fixed — added a validateValkeyConnection preUpdate (mirroring validatePGVectorConnectionString) on the Valkey connection keys; it calls Valkey.validateConnection(overrides) with the pending form value layered over current env and surfaces the error to the UI when Valkey is the active provider.
| try { | ||
| await client.customCommand(["FT.INFO", this.indexName(namespace)]); | ||
| return true; | ||
| } catch (e) { |
There was a problem hiding this comment.
namespaceExists treats every FT.INFO error as "namespace does not exist", masking real outages.
A connection refusal, auth failure, or a missing valkey-search module all make FT.INFO throw, and the catch returns false. Consequences: performSimilaritySearch returns the benign "Invalid query - no documents found for workspace!" message, and deleteDocumentFromNamespace silently no-ops. Users see empty-but-normal chat results and admins see deletes "succeed" while the database is actually down or misconfigured — only heartbeat surfaces the error.
Suggested fix: distinguish "index not found" from infrastructure errors. valkey-search returns a specific message for an unknown index; only treat that case as false and re-throw everything else so the real failure propagates.
There was a problem hiding this comment.
Fixed — namespaceExists/_namespaceInfo now only return false for an unknown-index error (_isUnknownIndexError) and re-throw everything else. Verified against live valkey/valkey-bundle:8.1: the real message is Index: with name '...' not found, which the matcher catches, so genuine outages now propagate instead of looking like an empty namespace.
| // we never leak a dead client object. | ||
| async disconnect() { | ||
| try { | ||
| if (Valkey.#client) Valkey.#client.close(); |
There was a problem hiding this comment.
client.close() is not awaited — a rejected close promise escapes the try/catch as an unhandled rejection.
GLIDE's close() returns a promise. If it rejects (e.g. socket already broken), the synchronous try block completes, #client is nulled in finally, and the rejection surfaces later as an unhandledRejection rather than hitting the catch(e) logger. Same pattern in validateConnection (lines 767 and 774).
| if (Valkey.#client) Valkey.#client.close(); | |
| if (Valkey.#client) await Valkey.#client.close(); |
There was a problem hiding this comment.
Fixed — close() is now awaited in disconnect() and in both validateConnection paths, so a rejected close lands in the catch logger instead of escaping as an unhandled rejection.
| requestTimeout, | ||
| protocol: ProtocolVersion.RESP2, | ||
| }; | ||
| if (password) |
There was a problem hiding this comment.
Username-only ACL connections never authenticate.
The whole credentials block is gated on if (password), so a configured VALKEY_VECTOR_DB_USERNAME with no password (valid for some ACL setups) is dropped and the client connects as the unauthenticated default user, producing NOAUTH/NOPERM errors instead of authenticating as the requested user.
Suggested fix: attach credentials when either is present:
| if (password) | |
| if (username || password) | |
| config.credentials = { | |
| username: username || undefined, | |
| password: password || undefined, | |
| }; |
There was a problem hiding this comment.
Fixed — credentials are now attached when username || password is set, with each field passed as value || undefined, so username-only ACL users authenticate instead of falling back to the default user.
|
|
||
| const useTLS = | ||
| overrides.useTLS ?? | ||
| String(process.env.VALKEY_VECTOR_DB_USE_TLS ?? "false") === "true"; |
There was a problem hiding this comment.
A rediss:// endpoint URL does not auto-enable TLS.
When VALKEY_VECTOR_DB_ENDPOINT is parsed (lines 64-74), the scheme is used for host/port/credentials but ignored for TLS — useTLS comes only from VALKEY_VECTOR_DB_USE_TLS (default false). A user who sets rediss://host:6379 and leaves USE_TLS unset connects in plaintext to a TLS-only port and the handshake fails, with no hint that the scheme implied TLS.
Suggested fix: infer TLS from the URL scheme when an endpoint is provided, e.g. set a local let useTLSFromUrl = url.protocol === "rediss:" inside the parse block and OR it into the final useTLS.
There was a problem hiding this comment.
Fixed — the parse block now sets useTLSFromUrl = url.protocol === "rediss:" and ORs it into the final useTLS, so a rediss:// endpoint enables TLS even when VALKEY_VECTOR_DB_USE_TLS is unset.
| const reply = await client.customCommand([ | ||
| "FT.SEARCH", | ||
| this.indexName(namespace), | ||
| `*=>[KNN ${topN} @vector $BLOB AS score]`, |
There was a problem hiding this comment.
FT.SEARCH KNN query has no SORTBY, so results may not be ordered best-first.
The query aliases the distance AS score but never sorts by it. valkey-search may return the KNN docs in internal/index order rather than ascending distance. Because similarityResponse filters each match independently and never sorts, contextTexts/scores can come back unordered, so the highest-relevance chunk is not guaranteed to be first in the context handed to the LLM — silent retrieval-quality degradation.
Suggested fix: add "SORTBY", "score" (ascending, since this is COSINE distance) to the FT.SEARCH args, or sort result by similarity descending before returning.
There was a problem hiding this comment.
Fixed, but not via SORTBY — live testing against valkey/valkey-bundle:8.1 showed valkey-search rejects SORTBY on a KNN query (Unexpected: argument SORTBY). Took the alternative instead: similarityResponse now sorts matches by similarity descending (lowest COSINE distance first) before filtering, so the most relevant chunk is always first.
| const { client } = await this.connect(); | ||
| if (!(await this.namespaceExists(client, namespace))) | ||
| throw new Error("Namespace by that name does not exist."); | ||
| const stats = await this.namespace(client, namespace); |
There was a problem hiding this comment.
Redundant FT.INFO round-trips on metadata and query paths.
namespace() calls namespaceExists (FT.INFO) then _namespaceCount (FT.INFO) on the same index, and a single FT.INFO reply already carries both existence and num_docs. namespace-stats and delete-namespace then add their own namespaceExists on top, so each issues 2-3 FT.INFO calls for one index. Separately, performSimilaritySearch does an FT.INFO existence pre-check on every query even though FT.SEARCH against a missing index already errors — an extra round-trip on the hottest (RAG) path.
Suggested fix: issue one FT.INFO and read both existence (did it throw) and num_docs from that reply; drop the pre-check in performSimilaritySearch and treat the FT.SEARCH "unknown index" error as the empty-namespace case.
There was a problem hiding this comment.
Fixed — added _namespaceInfo which issues a single FT.INFO and returns both existence and num_docs; namespace(), namespace-stats, and delete-namespace now go through it instead of stacking separate existence + count probes. performSimilaritySearch drops the pre-check entirely and treats an unknown-index FT.SEARCH error as the empty-namespace case.
- Rebuild cached client on connection setting changes (postUpdate disconnect) - Normalize keyPrefix to match indexName (prevent prefix/index divergence) - Guard getOrCreateIndex against dimension mismatch; reset valkey on embedder change - Wire validateConnection via preUpdate so bad config is rejected at save time - namespaceExists: only treat unknown-index as missing; propagate real outages - await client.close() in disconnect/validateConnection (no unhandled rejection) - Authenticate username-only ACL connections - Infer TLS from rediss:// endpoint scheme - Sort KNN results by similarity descending (valkey-search rejects SORTBY on KNN) - Collapse redundant FT.INFO round-trips; drop per-query existence pre-check
Jonathan-Improving
left a comment
There was a problem hiding this comment.
QA Review: Valkey Vector Database Provider
Verdict: COMMENT — 2 findings (1M, 1L). GLIDE API usage verified correct.
GLIDE API Verification (@valkey/valkey-glide 2.4.1, JS client)
All customCommand calls verified correct:
- ✅
FT.CREATE— ON HASH PREFIX 1 SCHEMA VECTOR HNSW 6 TYPE FLOAT32 DIM DISTANCE_METRIC COSINE - ✅
FT.SEARCH— index query PARAMS 2 BLOB DIALECT 2 RETURN 3 text metadata score LIMIT 0 - ✅
FT.INFO— correct 2-arg structure - ✅
FT.DROPINDEX— correct 2-arg structure - ✅
FT._LIST— correct no-arg structure - ✅
SCAN— cursor MATCH pattern COUNT n - ✅ Vector encoding:
Buffer.from(Float32Array.from(values).buffer)— correct LE float32 - ✅ RESP2 protocol selection for predictable array responses
- ✅
GlideClient.createClient(config)— correct factory pattern - ✅ KNN query
*=>[KNN N @vector $BLOB AS score]with PARAMS binding — no string interpolation
Key Positives
- Excellent dimension mismatch detection in
getOrCreateIndex(prevents silent data loss on embedder change) - Bounded SCAN loop with
_MAX_SCAN_ITERATIONScap - Comprehensive namespace normalization (mirrors Milvus pattern)
filterIdentifiersexclusion works correctly with source pinning- Connection validation with explicit cleanup (
validateConnection) - Thorough test suite: 28 unit tests (hermetic) + 9 integration tests (live server)
distanceToSimilaritycorrectly clamps and inverts COSINE distance- Named constants throughout (no magic numbers)
- Admin UI, env plumbing, docker-compose all properly wired
Findings
See inline comments below.
| const info = await client.customCommand(["FT.INFO", index]); | ||
| total += Number(this._infoValue(info, "num_docs") ?? 0) || 0; | ||
| } catch { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[Suggestion — Medium] Silent error swallowing in catch blocks
Several bare catch blocks discard error context entirely, making production debugging difficult:
- Line 457:
totalVectorssilently reports 0 during a connection outage (FT.INFO failure →continue) - Line 738: Malformed metadata JSON → silently replaced with
{}— the search result is returned with empty metadata instead of being flagged as degraded - Line 75: Malformed endpoint URL silently falls through
In production, these make it impossible to distinguish "no data" from "connection failure" from "parse error."
Suggested fix: At minimum, log at debug/warning level in these paths:
} catch (e) {
this.logger("totalVectors - FT.INFO failed for %s: %s", index, e.message);
continue;
}For the metadata parse (line 738), consider including a marker in the returned result so downstream consumers know it is degraded:
} catch {
metadata = { _parseError: true };
}There was a problem hiding this comment.
Good catch. Added logger calls to all three sites in e07d258d — the per-index FT.INFO failure in totalVectors, the malformed-endpoint parse in connection() (via new Valkey().logger, mirroring validateConnection), and the metadata JSON parse in performSimilaritySearch. I skipped the _parseError metadata marker since that object is spread into sourceDocuments and would leak into LLM context/citations — a debug log keeps the degraded case visible without polluting results.
| // we never leak a dead client object. | ||
| async disconnect() { | ||
| try { | ||
| if (Valkey.#client) await Valkey.#client.close(); |
There was a problem hiding this comment.
[Observation — Low] await client.close() — close() is synchronous in JS GLIDE
The @valkey/valkey-glide Node.js client's close() method is synchronous (returns void, not a Promise). Awaiting it is harmless (awaiting a non-thenable resolves immediately) but semantically misleading — it suggests the operation is async when it is not.
This appears on lines 349, 884, and 891.
Not blocking — just a correctness note. If the GLIDE team ever makes close() async, the existing await will already be correct.
There was a problem hiding this comment.
Confirmed — BaseClient.d.ts in 2.4.1 declares close(errorMessage?: string): void, so it's synchronous as you noted. Keeping the await intentionally: it's a no-op today and future-proofs the call sites if GLIDE ever makes close() async, which matches your closing note.
Signed-off-by: Daria Korenieva <daric2612@gmail.com>
|
Opened public PR: Mintplex-Labs#5929 |
Summary
Adds Valkey (with the
valkey-searchmodule) as a first-class vector database provider, selectable viaVECTOR_DB=valkey. The change is purely additive: default behavior is unchanged and the provider is only exercised when explicitly configured.Valkeyprovider extending theVectorDatabasebase contract, registered ingetVectorDbClass()exactly like the PGVector/Qdrant providers.@valkey/valkey-glideclient (server-only, pinned exact2.4.1) andvalkey-search(FT.CREATE/FT.SEARCH, HNSW, COSINE) with one index per namespace (allm_idx_{ns}) overallm:{ns}:hash keys. Vectors stored as FLOAT32 little-endian; KNN query vector bound viaPARAMS(no string interpolation).updateENVKEY_MAPPING,systemSettingswith the password surfaced as a boolean only),.env.exampleentries, a commented opt-invalkeyservice indocker-compose.yml, and an admin UI config option + provider-privacy entry.Capability parity
Full parity (or greater) with the existing reference providers — nothing stubbed or deferred:
delete-namespace,reset, namespace stats / existence checkssimilarityThreshold+topN,filterIdentifiers(pinned-source) exclusionskipCacheembed path, with partial-failure surfacingrerankedSimilarityResponseis correctly out of scope (not part of the base contract)What was tested
filterIdentifiersfiltering, orphan cleanup, and selector registration.valkey/valkey-bundle:8.1(real Valkey ops; only the embedder, on-disk vector cache, and PrismaDocumentVectorsstubbed). Poll-based index waits, no blind sleeps — heartbeat, index create + ingest, KNN retrieval,filterIdentifiersexclusion, document delete, namespace ops,totalVectorsacross namespaces,delete-namespace(no orphan keys),reset.eslintclean on all changed server source files.Configuration / opt-in
Disabled by default; activate by setting
VECTOR_DB=valkeyplus connection config:Requires a Valkey server with the
valkey-searchmodule (valkey/valkey-bundle:8.1). A commented opt-in service is included indocker/docker-compose.yml.How to test manually
Related ticket: AEA-534.