Skip to content

Handles held across delete and recreate resurrect records in both SQLite vector stores #1536

Description

@edwinyyyu

What happened

A VectorStoreCollection handle stays usable after its collection is deleted. If a collection with the same (namespace, name) is then created, writes through the stale handle reach the new collection. Both SQLite-backed vector stores resurrect records this way; Qdrant and Milvus do not.

Everything below reproduces inside a single process — no concurrency across processes is involved.

Measured by: create collection -> open handle -> delete collection -> create again with the same name and config -> write through the stale handle -> query a freshly opened handle.

Store stale write record reaches the recreated collection
QdrantVectorStore succeeds no
MilvusVectorStore succeeds no
SQLiteVecVectorStore succeeds yes, immediately
SQLiteVectorStore succeeds yes, after the next index save

SQLiteVecVectorStore

Records and vector table names derive from (namespace, name) only (_collection_prefix). Delete drops those tables; create rebuilds them under the same names. The stale handle holds records_table and vector_table_name, so its writes land in the new collection's tables and are returned by queries on a freshly opened handle straight away.

SQLiteVectorStore — worse, because it rewrites the live index

SQLiteVectorStoreCollection captures index_path (name-derived) and search_engine at open time, and _maybe_save_index writes that engine to that path. So a handle to a deleted collection can overwrite the index file belonging to its replacement:

index after recreate + legitimate write C: (268 bytes, sha 376ae8b8a2a4)
index after write through the STALE handle: (424 bytes, sha 41d3bffdce74)
stale handle rewrote the live index file:  True
after restart, collection contains: B (stale write) = True, C (legitimate) = True

The stale write's row also lands in the recreated collection's records table, and its _PendingOperationRow is keyed by the same (namespace, name), so the new collection's own pending operations count toward the stale handle's save_threshold. Before a save the record is present in SQL but missing from the index — the two disagree; after a save and restart it is fully queryable.

Why the declared concurrency scope does not settle this

Both stores declare ConcurrencyScope.PROCESS, and that scope is the one place where the application could be the guarantor: inside a single process it can see every handle it holds and every delete it issues, so a lenient reading -- "a handle outliving its collection is the caller's problem" -- is at least coherent there. Above PROCESS it is not coherent at all. Process A holds a handle, process B deletes and recreates, and A has no way to learn of it; only the store can enforce the invariant. Any implementation declaring MACHINE or CLUSTER must therefore provide it outright.

That asymmetry argues for a uniform guarantee rather than a scope-conditional one:

Note that the reproductions above are sequential -- no interleaving is needed to trigger them, so this is a handle-lifetime defect rather than a concurrency defect as such. Interleaving only removes the caller's chance to notice: with one task holding a handle while another deletes and recreates, nothing short of the application serialising its own lifecycle operations avoids it. At that point the declared PROCESS scope is not something the store delivers on its own either.

Proposed solution

Generation-scope the native resources, in the stores themselves rather than by taking a registry dependency.

Why not inject CollectionRegistry. The registry exists to give a backend a transactional metadata authority it does not otherwise have: Qdrant and Milvus cannot make create atomic by themselves. The SQLite stores already are that authority. _CollectionRow lives in the same SQLAlchemy engine as the per-collection tables, and create_collection performs the DDL and the metadata write in one transaction, so a crash cannot leave a half-created collection. Injecting a registry would split one transactional authority into two, reintroduce the native-first/register-last crash window that the registry-backed stores accept only because they have no alternative, and add a required registry_database setting to what is currently the zero-configuration embedded option. The mechanism should be shared as a design, not as a dependency.

The change.

  1. _CollectionRow gains a generation column, minted per creation.
  2. _collection_prefix incorporates the generation, so a recreated collection gets new table names. Everything else currently keyed by (namespace, name) follows: in SQLiteVectorStore the index path, the _search_engines cache key, and _PendingOperationRow's key; in SQLiteVecVectorStore the records and vector table names and the {partition_key}:{uuid} primary ids.
  3. A stale handle then addresses dropped tables and fails loudly rather than silently landing in the replacement. That is permitted by the contract and is more useful than the silent invisibility the registry-backed stores offer, which is the most their shared native containers allow.
  4. _maybe_save_index already opens a session to count pending operations; read the current generation in the same query and skip the save when it does not match the handle's, so a stale handle cannot leave an orphan index file either.

Migration. Default generation to the empty string, and have the prefix append it only when non-empty. Pre-existing collections keep their table names and index paths and keep working; collections created after the change get a generation, and a legacy collection acquires one when it is next deleted and recreated. No renames, no data movement, no startup migration step.

Two adjacent defects worth fixing in the same pass.

  • _PendingOperationRows are replayed on startup "applied or not", so the orphaned row left by a stale write is re-applied into the replacement collection's engine. This is why the stale record survived a restart in the transcript above, and generation-keying the rows fixes it.
  • SQLiteVecVectorStore.create_collection reads, checks, then inserts, even though _CollectionRow's primary key is already (namespace, name) and could arbitrate directly. Mapping IntegrityError onto VectorStoreCollectionAlreadyExistsError is the pattern SQLAlchemyCollectionRegistry.register uses, and would let the store deliver the process-scoped bookkeeping it declares.

Severity note for SQLiteVectorStore: _CollectionRow.index_saved marks the on-disk index as part of the durable contract once set, so a clobbered index is treated as an error rather than silently rebuilt.

Rough cost: ~6 hours -- 1h for the generation column and prefix in both stores, 1.5h for SQLiteVectorStore's index path, engine cache key, pending-operation key and save guard, 1h for SQLiteVecVectorStore plus the create compare-and-set, 1.5h for a shared conformance test across all four stores, 1h for the legacy-generation path and its tests.

Also needed

State the invariant in the VectorStoreCollection contract, unconditionally rather than per concurrency scope, so the fix above is conformance rather than a new behavior and a new backend cannot land without it.

Notes

Pre-existing on main. The open collection-registry PRs (#1526, #1527, #1530, #1531, #1533) leave the mechanism untouched — they only remove open_or_create_collection from these two stores and add a concurrency_scope property. Same store and a related class of defect as #1468.


🤖 Written by Claude Code (Opus 5) on behalf of @edwinyyyu.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions