fix(retriever): rebuild a missing vector store engine on demand - #2357
Merged
lyingbug merged 2 commits intoJul 29, 2026
Conversation
The engine registry is per-process, so a store's engine is missing
whenever this process did not create it. That happens three ways:
- Startup skips a store whose engine fails to construct and never
retries it, so that instance cannot serve the store again -- not
even after a restart, if the backend is slow to come up.
- A store registered on one instance is absent on every other until
they restart, because registration is not broadcast.
- Registration can fail on the instance handling the create, leaving
the store missing even there.
Each one surfaces as "vector store is currently unavailable" and is
cleared today only by restarting the process.
Add GetOrLoadByStoreID, which falls back to building the engine from the
store row when the lookup misses, and route the two bound-store lookups
through it. The method goes on RetrieveEngineRegistry rather than
StoreRegistry because that is the interface the factory functions
receive; the two declare GetByStoreID separately and neither embeds the
other.
Concurrent callers collapse onto one build. That build runs on a context
detached from the caller that started it: callers share the result, so
letting the first one's cancellation abort it would fail everyone else
waiting. A panic inside the build is contained, because singleflight
re-raises it on a goroutine the HTTP recovery middleware cannot reach
and the build calls third-party client constructors.
Publishing is guarded by a per-store generation sampled before the
build, so an engine that finishes after the entry was registered or
removed by someone else does not overwrite it and leave that engine
orphaned with its connections open. A failed build starts a cooldown:
collapsing only helps concurrent callers, so without it a backend that
stays down would cost a full build timeout on every sequential request.
Separate "the store is gone" from "its engine cannot be built right
now". Async delete handlers answer ErrVectorStoreNotFound with
asynq.SkipRetry, so reporting a database blip or a backend outage that
way would permanently discard knowledge-base and index delete tasks and
orphan the vector data they exist to remove. ErrVectorStoreUnavailable
covers the retryable cases; the repository already distinguishes them,
returning (nil, nil) for a missing row and an error for a failure.
For the same reason a caller's own cancellation is never folded into a
store sentinel. The ownership check runs first and queries the database
with that context, so on a shutdown it is usually where cancellation is
noticed.
A search resolves one engine per distinct store, sequentially, and this
server sets no HTTP read or write timeout, so the resolution loop gets a
budget above a single build. An exhausted budget still leaves the
engines warming, since the build is detached, and is reported as an
unavailable store because retrying is likely to succeed.
Build failures are logged where they happen, with the panic stack. The
cause cannot travel back to the caller -- factory errors embed endpoints
and credentials -- so without this the one thing an operator needs when
self-healing fails to heal would be discarded with the error.
Note a deliberate contract change: an ownership lookup failure used to
be reported as not-found, and a test asserted that on purpose. A
database failure says nothing about whether the store exists, and
treating it as permanent is what discards the task, so that assertion is
inverted rather than preserved.
ProcessKBDelete must return retryable engine-resolution failures instead of logging and continuing, which could delete knowledge rows while leaving embeddings behind. Align the handler with ProcessIndexDelete and cover cancellation and ErrVectorStoreUnavailable in tests.
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.
Closes #2356
Overview
The engine registry is per-process, so a store's engine is missing from any process that did not build it. That happens three ways: startup skips a store whose engine fails to construct and never retries it, a store registered on one instance is absent on every other, and registration can fail on the instance handling the create. Each one answers
vector store is currently unavailable(2201) for as long as the process lives, and the first is not fixed by restarting.GetOrLoadByStoreIDfalls back to building the engine from the store row when the lookup misses, and the two bound-store lookups now go through it. Everything it needs was already at hand: the ownership check one line earlier reads the same row, and the engine factory is already in the container.Why
The lookup gave up while holding everything required to recover. The row had just been read to verify ownership, the factory that builds engines from rows was already wired, and the failure was reported as if the store did not exist. The codebase names the consequence in three places —
"will be available after restart", "sibling replicas continue serving the engine from their own caches until process restart", and a test comment describing a row whose "engine initialization failed at app start" — so the behaviour was understood, just never closed.The method goes on
RetrieveEngineRegistryrather thanStoreRegistry, because that is the interface the factory functions receive. The two declareGetByStoreIDseparately and neither embeds the other, so putting it onStoreRegistrydoes not compile at the call site.Changes
internal/types/interfaces/retriever.go(+15):GetOrLoadByStoreIDonRetrieveEngineRegistry, documented as tenant-scoping its own database lookup while still expecting callers to verify ownership first.internal/application/service/retriever/registry.go(+254/-2):GetOrLoadByStoreID— miss → load the row → build → publish. Concurrent callers collapse onto one build viasingleflight.context.WithoutCancel. Callers share its result, so letting the first one's cancellation abort it would fail everyone else waiting on it.singleflightre-raises it on a goroutine of its own (go panic(e); select{}), out of reach of the HTTP recovery middleware, and the build calls third-party client constructors — so without this an availability fix could end the process.RegisterWithStoreIDandUnregisterByStoreIDboth bump it, so a build that finishes late cannot undo a registration or a removal, or leave the engine it replaced orphaned with its connections open.EngineBuildTimeoutis exported so a caller budgeting a sequence of resolutions can size its ceiling above one build instead of guessing.internal/application/service/retriever/factory.go(+56/-10):GetOrLoadByStoreID.ErrVectorStoreUnavailable— "the store exists but its engine cannot be produced right now" — separated fromErrVectorStoreNotFound. The async delete handlers answer not-found withasynq.SkipRetry, so reporting a database blip or a backend outage that way permanently discarded knowledge-base and index delete tasks and orphaned the vector data they exist to remove. The repository already distinguishes the two, returning(nil, nil)for a missing row and an error for a failure.internal/application/service/knowledgebase_search_storegroup.go(+27/-1): a budget over the resolution loop. A search resolves one engine per distinct store, sequentially, and this server sets no HTTP read or write timeout, so nothing bounded the total. Only resolution runs under it — folding the embedding calls into the same ceiling would cut them short for an unrelated reason — and it derives from the caller's context so a client that allows less time still wins. An exhausted budget is reported as an unavailable store rather than an internal error, since the build is detached and keeps warming, so a retry is likely to succeed.internal/application/service/knowledgebase.go(+15/-2),tag.go(+2): the KB delete handler returns on the retryable sentinel instead of logging and continuing, which would have reported success while leaving the embeddings in place.validateVectorStoreBindingmaps the new sentinel to 2201 and lets a context error through rather than answering a client disconnect with a 500.internal/container/container.go(+5/-1): the registry is given the repository and the factory. Both are constructor arguments rather than an optional extra — a registry without them satisfies the interface and serves every lookup, so leaving them out would go unnoticed until a store went missing in production, which is the failure this change removes.Behaviour change worth calling out
An ownership lookup failure was reported as
ErrVectorStoreNotFound, andTestCreateRetrieveEngineForKB_OwnershipLookupErrorasserted that deliberately. A database failure says nothing about whether the store exists, and treating it as permanent is what discards the task, so that assertion is inverted rather than preserved.Core
GetOrLoadByStoreIDinregistry.go— the collapse, the detached build context, the panic guard, and the generation check are each load-bearing for a different failure; the tests below pin them individually.factory.go— this is what decides whether async work is retried or dropped.Test Plan
go build ./...,go vet ./internal/...,gofmtclean on changed files, and no added line over 120 columnsgo test -race ./internal/...— pass. Four suites fail identically onmainand are untouched here:TestTenantAPIKeyServiceAuthenticateThrottlesLastUsedUpdates(data race),internal/datasource/connector/notion,TestEventBus_EmitAndWait(data race), andinternal/infrastructure/docparserTestResolveRemoteImages_*under-count>1.singleflight.Do, and separately when collapsing is removed); concurrent misses build once; a panicking factory fails one request and not the process; an unregistration or a concurrent registration during a build is not overwritten; a failed build enters a cooldown and leaves it; a database failure is retryable while an absent store is not; a cancellation is not reported as a store verdict on either lookup path; the resolution budget applies and never outlives the caller; the container builds a registry that can actually rebuild.mainB answers 2201 on three consecutive attempts; with this change B succeeds. Taking OpenSearch down mid-test shows the rebuild genuinely dials it and logs the transport error it used to swallow, a request inside the cooldown window spends no further attempt, and bringing OpenSearch back lets the same request succeed without restarting anything.No new dependencies —
golang.org/x/syncis already required directly.