Skip to content

[sqlite store fixes 1/7] Publish vector index files atomically (but not durably) - #1460

Draft
edwinyyyu wants to merge 1 commit into
MemMachine:feat/horizontal-scalingfrom
edwinyyyu:atomic_index_swap
Draft

[sqlite store fixes 1/7] Publish vector index files atomically (but not durably)#1460
edwinyyyu wants to merge 1 commit into
MemMachine:feat/horizontal-scalingfrom
edwinyyyu:atomic_index_swap

Conversation

@edwinyyyu

@edwinyyyu edwinyyyu commented Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Problem

SQLiteVectorStore persists each collection's index by calling the search engine's save(), which writes directly to the final path. A crash partway through leaves a truncated file on disk.

The torn file is only half of it. _save_collection_index trims the applied _PendingOperationRows as soon as save() returns, and that log is the only other copy of those vectors — the records table has no vector column. So the trim is safe at exactly one instant: when the index at the published path holds them. Getting that instant wrong is silent, because a missing vector is indistinguishable from a vector that simply doesn't match.

Fix

A shared index_persistence helper writes the index to a sibling temp file, flushes it, and swaps it into place with os.replace; both engines adopt it and neither gains anything else. load clears a temp left by an interrupted save, so a crash leaks at most one stale file per index.

After this, a save either publishes a complete index or leaves the previous one exactly where it was. Nothing else changes: no schema change, no migration, no rename of index_path, and no new file at the base path.

What a save does not promise

It does not promise that the publication is durable. That is a decision rather than an oversight, so the reasoning is below.

A rename changes a directory entry, not file data, and the two have very different guarantees:

make it durable available
file contents fsync / F_FULLFSYNC / FlushFileBuffers everywhere, documented
directory entry fsync on a directory file descriptor POSIX only, and not on every filesystem
  • POSIX closes the gap with a parent-directory fsync, but only best-effort — network and FUSE filesystems are where that bites.
  • Windows has no equivalent at all. os.fsync is _commit, which is FlushFileBuffers, which is for file data; you cannot open a directory to fsync it. MOVEFILE_WRITE_THROUGH does not help either: its documented guarantee is scoped to "a move performed as a copy and delete operation", the cross-volume path, and says nothing about flushing NTFS metadata for a same-volume rename. The decisive evidence is SQLite's — its VFS threads a directory-sync flag through every commit-relevant directory operation, unixDelete honors it, and winDelete declares the same parameter /* Not used on win32 */. SQLite cares about this more than almost any software and makes no attempt on Windows.

So a power failure can roll the rename back after save returned, while the trim behind it stays committed.

Why that gap stays open

Closing it means never using a directory operation as the commit point: two index slots plus a generation record written into a file that already exists, in the spirit of SQLite's PERSIST journal mode. An earlier revision of this PR implemented exactly that, and it worked. It was still the wrong trade, for four reasons.

The window is narrow, and only one kind of failure lands in it. A clean shutdown, an unhandled exception, an OOM kill, a SIGKILL — none of these lose a rename, because the kernel still owns the page cache and writes it back. Those failures are already covered: the pending log holds everything the engine has not been checkpointed with, and startup replays it. What is left is the machine itself dying between the rename and writeback — a power cut, a host failure, a panic — plus the filesystems where a directory fsync is best-effort or unavailable anyway.

What it costs is recall, not correctness. The records table is the authority on what exists, and every query hit resolves through it, so a reverted publication cannot produce a wrong or stale result — only fewer results. Concretely: the records survive, get still returns them, query stops finding them, and the exposure is bounded by the operations applied since the previous checkpoint, i.e. at most save_threshold. Re-upserting an affected record repairs it. That is the direction this store already tolerates, and it is repairable by the same ingest path that created the record.

Detecting it is not cheap enough to be worth it. Comparing the index's size against the record count is the obvious check and it does not work: row_ids are not reused (#1469 makes that unconditional; before it only the highest row's id can come back), so deleting a record and upserting the same uuid again moves it to a new id. A rollback spanning that pair leaves the index holding the old id and missing the new one — one extra, one missing, identical count — and a re-embedding workload produces that shape constantly. A check that does work needs an id-set digest maintained by every engine, or a full reconcile scan at every boot, which is most expensive precisely where the index is large. Neither earns its keep against a bounded recall gap.

And the guarantee is not free to hold. It cannot be delegated to a rename, so it has to become a layout: two slots and a generation record per index, which every engine has to implement and every future engine has to be audited against. Engines that persist by writing one file — which is what both engines here do, and what most ANN libraries expose — stop being usable as they ship. Existing indexes stop loading until they are cleared and re-ingested, which is a migration this revision no longer asks anyone to perform. Fewer guarantees, less machinery to keep correct, and a wider set of engines that can plug in unmodified.

The direction that would cost — a published index that will not parse — stays closed: the atomic swap prevents it, the pre-swap fsync narrows the window where a rename outlives the data behind it, and a saved-but-unloadable index remains a loud IndexLoadError rather than a silently empty rebuild.

Nothing here detects a lost publication for the caller. A deployment that needs every record searchable after a power failure must be able to re-ingest.

Where the contract is stated

The guarantee is only useful if callers can find it, so it is written where each of them looks:

  • VectorSearchEngine.save — returning means a later load reads this index or the one it replaced, never a mixture; it does not mean the publication survives a power failure.
  • index_persistence — the mechanism, and why a stronger guarantee is not portable.
  • sqlite_vector_store — what survives a process crash, what a power failure costs, and that re-ingest is the repair.
  • _save_collection_index — why the save-then-trim order is the whole protocol.

One consumer needed more than a docstring. VectorStoreSemanticStorage.update_feature reads a feature's stored embedding back when the caller updates the feature without supplying one — the only place in the server that depends on the index still holding a vector. It reported a lost vector as Vector record not found, naming the feature, which points at the wrong thing and hides the repair; it now separates a record that is genuinely absent from one whose embedding is gone, and the second says to pass a fresh embedding.

Tests

  • test_index_persistence.py: the swap publishes completely or not at all, a failed write leaves the previous index intact and removes the temp, and a stale temp is cleared on load.
  • Per-engine (hnswlib + usearch): a save leaves no temp file behind, and a load clears a stale temp file and still finds every key.
  • Store-level: test_a_reverted_publication_costs_search_not_records reconstructs a lost publication deterministically — restore the previous index bytes after the trim has committed — and pins the direction it fails in: the record still resolves by uuid, and only search loses it. Missing and corrupt indexes each still raise IndexLoadError once index_saved is set.

vector_store suites pass (284 passed, 140 integration deselected) and semantic_memory suites pass (345 passed, 2 skipped, 999 integration deselected); ruff and ty clean.

Changed from the earlier revision

This PR previously shipped the two-slot generation-record protocol and its breaking on-disk change. That guarantee has been dropped in favor of the atomic swap plus an explicit contract, so the breaking-change notice no longer applies: indexes written by the current code keep loading, and there is nothing to migrate.

🤖 Generated with Claude Code

Port

This branch carries this change as merged into speedkick (#1588, 21105d6e6), the one commit cc5311ef9; the branch's earlier commits are replaced. Adaptation: the commit sits on [declared schema 1/2] (#1628), where the engines take a similarity metric, as on speedkick, where #1588 merged before #1598: its engine tests construct the engines with SimilarityMetric.COSINE, the reverted-publication test reads the surviving row through the records table since #1733 removed get, the module docstring says no read reaches a stored vector, and its semantic-storage change (reporting a lost embedding apart from a missing record) has nothing to apply to, since #1733 removed the read-back it reworded.

Stack

24 open PRs: three independent PRs, and the vector store tree of short parallel branches. Every open PR in the tree has feat/horizontal-scaling as its GitHub base, and the independent PRs have main. The branches are in a fork, and a pull request can target only this repository's branches, so the on column gives the order the PRs build on each other. A stacked PR's diff on GitHub includes the PRs under it until they merge.

Independent of the vector store tree, directly on main:

# PR change on
— #1624 Make no memory request create a project main
— #1786 Refuse a filter value of the wrong type for a datetime column, and answer an invalid list filter with 422 (port of #1620) main
— #1792 Refuse property values that some store refuses or alters where an episode enters main

The vector store tree. Each PR builds on the one in its on column; PRs on the same parent are parallel branches and do not depend on each other. #1631 is closed, superseded by #1733–#1736, which hold its changes split in four, with review changes since. #1675 is closed, superseded by #1627. #1841 is closed: its request packing is dropped, and its cancellation contract is in #1840. #1828 and #1839 sit side by side on the branch; #1839 and #1840 are the event memory recovery stack, and #1684 heads the event memory stack, which its own PRs index, listed because it builds on #1840, its branch also carrying #1828's commits until one of the two merges; #1871 keeps its own Stack section. Until the PRs under it merge, their changes show in a stacked PR's diff.

# PR change on
[vector store scale-out 1/6] #1671 (merged) Remove custom sharding from the Qdrant store (port of #1654) main
[vector store scale-out 2/6] #1733 (merged into feat/horizontal-scaling) Answer vector store queries with record UUIDs and scores, and refuse invalid inputs feat/horizontal-scaling
[vector store scale-out 3/6] #1734 (merged into feat/horizontal-scaling) Arbitrate vector store collections in a SQL registry, with an incarnation per collection life and a purge feat/horizontal-scaling
[vector store scale-out 4/6] #1735 (merged into feat/horizontal-scaling) Move the Qdrant store onto the collection registry feat/horizontal-scaling
[vector store scale-out 5/6] #1736 (merged into feat/horizontal-scaling) Move the Milvus store onto the collection registry, against a Milvus server feat/horizontal-scaling
— #1775 (merged into feat/horizontal-scaling) Accept attempts exhausted in the lifecycle churn contract, and say which Qdrant operations filter on the incarnation feat/horizontal-scaling
— #1779 (merged into feat/horizontal-scaling) Classify Qdrant errors by status code alone feat/horizontal-scaling
— #1813 (merged into feat/horizontal-scaling) Create Qdrant collections with strict mode off feat/horizontal-scaling
— #1820 (merged into feat/horizontal-scaling) List only active projects in projects/list feat/horizontal-scaling
[user properties 1/2] #1702 (merged into feat/horizontal-scaling) Keep undeclared properties out of the vector store feat/horizontal-scaling
[user properties 2/2] #1670 (merged into feat/horizontal-scaling) Remove per-project filterable properties (port of #1606) feat/horizontal-scaling
— #1824 (merged into feat/horizontal-scaling) Drop the unread logical key from the purge queues feat/horizontal-scaling
— #1788 Refuse a repeated record UUID or a non-finite property value at upsert, and state the datetime property contract feat/horizontal-scaling
— #1631 (closed) Superseded by #1733–#1736, which hold its changes split in four, with review changes since —
[vector store scale-out 6] #1627 (merged into feat/horizontal-scaling) Give each vector store a fixed schema and string-keyed partitions, built from the deployment's configuration feat/horizontal-scaling
[declared schema 1/2] #1628 (merged into feat/horizontal-scaling) Make a vector store filter only on the properties it declares feat/horizontal-scaling
[sqlite write transactions 1] #1844 (merged into feat/horizontal-scaling) Take SQLite's write lock at BEGIN in every store on a relational SQLite engine feat/horizontal-scaling
[session storage 1/2] #1622 Create a session's storage with the session, never on a request feat/horizontal-scaling
[session storage 2/2] #1625 Look a partition up with get_partition, and remove open-or-create from both stores and close from the segment store #1622
[selective filter 1] #1871 Route filtered queries by selectivity, and search allowlists exhaustively feat/horizontal-scaling
[search results, deferred] #1663 Score every vector search by cosine similarity, and name scores for it (port of #1598's cosine half) #1628
[declared schema 2/2] #1616 Close the filter union, and make negation the complement on every backend feat/horizontal-scaling
[sqlite store fixes 1/7] #1460 (this PR) Publish vector index files atomically (but not durably) feat/horizontal-scaling
[sqlite store fixes 2/7] #1469 Never reuse a row id in SQLiteVectorStore #1460
[sqlite store fixes 3/7] #1672 Own the search engine's concurrency in the store, not in each engine (port of #1612) #1469
[sqlite store fixes 4/7] #1673 Serialize a partition's writes so the engine sees them in order (port of #1607) #1672
[sqlite store fixes 5/7] #1674 Refuse a pending row replay cannot honor, instead of dropping it (port of #1608) #1673
[sqlite store fixes 6/7] #1675 (closed) Superseded by #1627, which takes SQLite's write lock at BEGIN where the engine is created —
[sqlite store fixes 7/7] #1676 Give every write a fresh row id, so a key names one version (port of #1610) #1674
[sqlite store fixes 8] #1843 Hold the engine lock until a cancelled engine call's thread finishes #1676
[qdrant options] #1618 Let a deployment tune a Qdrant collection's HNSW, optimizers and quantization feat/horizontal-scaling
[milvus options] #1741 Let a deployment tune a Milvus collection's vector index and its searches #1618
— #1841 (closed) Its request packing is dropped, and its cancellation contract is in #1840 —
[property keys] #1828 Refuse an illegal metadata key before any store writes, and reserve one key prefix feat/horizontal-scaling
[event memory recovery 1/2] #1839 Give EventMemory a store's shape, with a handle per partition feat/horizontal-scaling
[event memory recovery 2] #1840 Keep event memory's two stores consistent through failed ingests and forgets #1839
[event memory 1/6] #1684 Add time bounds, sources and expansion to EventMemory (port of #1597, without session) #1840
[schema migration 1/2] #1825 Add memmachine-server migrate, which creates every configured store's schema before servers start feat/horizontal-scaling
[schema migration 2/2] #1826 Make startup check what migrate created, and create nothing at startup #1825

This PR is its one commit, b7062f941, directly on feat/horizontal-scaling. #1469 is stacked on it.

Verification

At this PR's head b7062f941, on 2026-10-30, rebased after #1628's squash merge (5c9d3ae99), #1831, and #1858. The chain now starts directly on feat/horizontal-scaling at 690116e37, where [declared schema 1/2] (#1628) is merged, instead of on [sqlite write transactions 1] (#1844): it uses none of #1844's code and shares no file with it. The replay was clean, and each commit's change is the same patch as before. Checks: ruff check and ruff format --check clean on the changed files at every head; ty check as CI runs it and uv lock --check clean at [sqlite store fixes 8] (#1843)'s head, where the server's unit tests pass (2390, 8 skipped). The integration tests did not run locally (Docker was down); CI runs them.

Before the rebase onto 690116e37: At this PR's head 7dc88afbd, on 2026-10-28, rebased onto [sqlite write transactions 1] (#1844) at 4f5d4430c, on [declared schema 1/2] (#1628) at df716e47b, whose tree differs from the previous base by a docstring and the removed extend_existing=True of SQLiteVectorStore._records_table. Each other commit's change is the same patch as before. Checks: ruff check and ruff format --check clean on the changed files at every head; ty check as CI runs it and uv lock --check clean at [sqlite store fixes 8] (#1843)'s head, where the vector store unit tests pass (443, 5 skipped).

Before the rebase onto #1844's 4f5d4430c: At this PR's head 96eca3a81, on 2026-10-27, rebased onto [sqlite write transactions 1] (#1844) at 0bcbf8b3a, after [declared schema 1/2] (#1628)'s second round of review changes (274264523), which move is_valid_identifier and its constants into data_types.py and sort vector_store.py's Raises sections. The replay was clean, and each commit's change is the same patch as before. Checks: ruff check and ruff format --check clean on the changed files at every head; ty check as CI runs it and uv lock --check clean at [sqlite store fixes 8] (#1843)'s head, where the vector store unit tests pass (443, 5 skipped).

Before the rebase onto #1844's 0bcbf8b3a: At this PR's head b3f956943, on 2026-10-27, rebased onto [sqlite write transactions 1] (#1844) at 69e292d8a, after [declared schema 1/2] (#1628)'s review changes (d6972a5fa), which drop supported_filter_nodes and the filter node imports it used, and rename sql_columns.py to sqlite_properties.py. The one conflict was in #1672's commit, at the import lines of sqlite_vector_store.py; it resolves to #1628's FilterExpr import plus that commit's AsyncRWLock import. Otherwise each commit's change is the same as before, with only context lines moved. Checks: ruff check and ruff format --check clean on the changed files at every head; ty check as CI runs it and uv lock --check clean at [sqlite store fixes 8] (#1843)'s head, where the vector store unit tests pass (443, 5 skipped).

Before the rebase onto #1844's 69e292d8a: At this PR's head d5c7e0489, on 2026-10-27, rebased when [declared schema 1/2] (#1628) and [sqlite write transactions 1] (#1844) swapped places: #1628 now sits directly on feat/horizontal-scaling, and #1844 on #1628 at 497a89ef1, whose tree is the previous base 135184cd6's. This PR's tree is byte-identical to its previous head a757d7146's, so the checks and tests below stand.

Before the #1628/#1844 swap: At this PR's head a757d7146, on 2026-10-27, rebased onto [declared schema 1/2] (#1628) at 135184cd6, after #1627 was squash-merged into feat/horizontal-scaling as 805ea2177 and [sqlite write transactions 1] (#1844) was rebased onto it as 2cb305a8e. Its tree is the previous head 8d1622b07's plus #1628's 135184cd6, which gives the Milvus request-size test's store a 120 s request timeout; that test failed on this stack's CI before. Checks at [sqlite store fixes 8] (#1843)'s head: ruff check and ruff format --check clean; ty check clean as CI runs it. No test this PR's verification ran changed, so they were not rerun.

Before the rebase onto #1628's 135184cd6: At this PR's head 8d1622b07, on 2026-10-26, rebuilt on [declared schema 1/2] (#1628) at 4b1210953 for #1627's final rebuild, with #1627 at 8c2351dbe and [sqlite write transactions 1] (#1844) at c9445910f below it. The save's docstring keeps #1627's statement that a save a deletion overtakes leaves no file. The test helper that reads a partition's stored uuids looks its records table up by the partition's incarnation, which names the table since #1627. Checks: ruff check and ruff format --check clean; ty check clean as CI runs it; uv lock --check clean. Tests: the SQLite store's and the search engines' tests, 198 tests. The integration tests did not run locally (Docker was down); CI runs them.

Before the rebuild on #1628's 4b1210953: At this PR's head da68f733a, on 2026-10-20, its tree is the previous head 3debee2f8's with #1627's accuracy fixes, in text only: the partition registry and purge design documents, the database manager's comments and docstrings, the SQLite store's module docstring, the partition registry configuration's descriptions, and other docstrings that still described collections. Checks: ruff check and ruff format --check clean; ty check clean as CI runs it (uv run --frozen --all-extras ty check --project packages/server); uv lock --check clean. No integration tests ran this time, since only comments, docstrings, descriptions, and documentation changed.

Before #1627's accuracy fixes: At this PR's head 3debee2f8, on 2026-10-19, its tree is the previous head 2bd1f490d's with #1627's second review's fixes: the Qdrant and Milvus tests that partitions share the store's collection now write through two partitions and read the store's one native collection; the store tests name their partition fixture partition; and docstrings, comments, and the configuration and upgrade documentation are corrected. Checks: ruff check and ruff format --check clean; ty check clean as CI runs it (uv run --frozen --all-extras ty check --project packages/server); uv lock --check clean.

Before #1627's second review's fixes: At this PR's head 2bd1f490d, on 2026-10-19, its tree is the previous head 9acf51b60's with #1627's review fixes: the database manager answers a repeated (backend, vector store name) with the store it built, without comparing parameters, and keeps a backend's shared SQLite engine until it closes, even when a store's build fails; its test that one SQLite backend's stores share the engine can now fail; the partition registry's parameters check the vector store name with the shared validate_vector_store_name; and semantic memory's local name for the backend key is backend. Checks: ruff check and ruff format --check clean; ty check clean as CI runs it (uv run --frozen --all-extras ty check --project packages/server); uv lock --check clean.

Before #1627's review fixes: At this PR's head 9acf51b60, on 2026-10-19, moved onto [declared schema 1/2] (#1628): its tree is the previous head 17750186c's without [search results] (#1663) beneath it. Its persistence tests construct the engines with SimilarityMetric.COSINE. Checks: ruff check and ruff format --check clean; ty check clean as CI runs it (uv run --frozen --all-extras ty check --project packages/server); uv lock --check clean. The unit tests pass, 364 tests (5 skipped).

Before the move off [search results] (#1663): At this PR's head 17750186c, on 2026-10-19, its tree is the previous head 1aaa99080's with design/vector_store_partition_registry.md listing the lookup as "open", the name it has from #1627 until #1625, where it said "get". Checks: ruff check and ruff format --check clean; ty check clean as CI runs it (uv run --frozen --all-extras ty check --project packages/server); uv lock --check clean. Outside that document the tree is the previous head's, so the test runs below stand.

Before the partition registry document's wording fix: At this PR's head 1aaa99080, on 2026-10-19, its tree is the previous head 7052b4f67's with #1824 beneath it, which drops the purge queues' unread logical key, and #1627 dropping the same key from the partition registry's tombstones and removing the collection registry design document its partition registry document replaces. Checks: ruff check and ruff format --check clean; ty check clean as CI runs it (uv run --frozen --all-extras ty check --project packages/server); uv lock --check clean.

Before the rebase onto #1824's merge: At this PR's head 7052b4f67, on 2026-10-16, its tree is the previous head 1861cf04a's with the configuration's check of a provided tombstone_retention_seconds named _validate_tombstone_retention, where it was _require_retention_floor. Checks: ruff check and ruff format --check clean; ty check clean as CI runs it (uv run --frozen --all-extras ty check --project packages/server); uv lock --check clean.

Before the retention check's rename: At this PR's head 1861cf04a, on 2026-10-16, its tree is the previous head 843ad4bbb's with every vector store check of a value that raises named validate_, as validate_partition_key already was: validate_declared_types, validate_query_vector, validate_score_threshold, validate_limit, and validate_dimensions from the base, and the PRs' own checks in their turn. Checks: ruff check and ruff format --check clean; ty check clean as CI runs it (uv run --frozen --all-extras ty check --project packages/server); uv lock --check clean.

Before the raising checks took the validate_ prefix: At this PR's head 843ad4bbb, on 2026-10-16, its tree is the previous head f5a44a57b's with #1627's changes: the VectorStore interface drops vector_store_name, vector_dimensions, similarity_metric, and indexed_properties, and the partition handle drops partition_key; a partition key may be any string of up to 255 bytes without NUL, and the SQLite stores name a partition's tables and index file by a SHA-256 digest of it; and the checks that return a bool are is_valid_identifier and is_valid_filter. Checks: ruff check and ruff format --check clean; ty check clean as CI runs it (uv run --frozen --all-extras ty check --project packages/server); uv lock --check clean.

Before the store's properties left its interface: At this PR's head f5a44a57b, on 2026-10-15: its tree is the previous head 16a007886's without #1627's per-partition schema record (PartitionSchema, the registry's and the SQLite stores' schema column, and VectorStorePartitionSchemaMismatchError), and with require_partition_key named validate_partition_key; ruff check and ruff format --check clean; ty check clean as CI runs it (uv run --frozen --all-extras ty check --project packages/server); uv lock --check clean.

Before the per-partition schema record went: At this PR's head 16a007886, on 2026-10-15: its tree is the previous head 09641ed71's with the partition lookup named open_partition and the segment store's close_partition kept, since #1625 renames the one and removes the other; ruff check and ruff format --check clean; ty check clean as CI runs it (uv run --frozen --all-extras ty check --project packages/server); uv lock --check clean.

Before the lookup's rename moved into #1625: At this PR's head 09641ed71, on 2026-10-15: its tree is the previous head 970fd7de8's without the segment store's close_partition; ruff check and ruff format --check clean; ty check clean as CI runs it (uv run --frozen --all-extras ty check --project packages/server); uv lock --check clean.

Before #1627's first commit took the removal of close_partition: At this PR's head 970fd7de8, on 2026-10-15, restacked on #1627's rewritten history: its tree is the previous head 2fba83479's; ruff check and ruff format --check clean; ty check clean as CI runs it (uv run --frozen --all-extras ty check --project packages/server); uv lock --check clean.

Before #1627's history was rewritten: At this PR's head 2fba83479, on 2026-10-15, rebased onto feat/horizontal-scaling at cf7ae1579, which carries main through #1774: ruff check and ruff format --check clean; ty check clean as CI runs it (uv run --frozen --all-extras ty check --project packages/server); uv lock --check clean.

Before the rebase onto cf7ae1579: At this PR's head 823ca3187, on 2026-10-15, rebased onto feat/horizontal-scaling with #1702 and #1670 merged (101182c18): ruff check and ruff format --check clean; ty check clean as CI runs it (uv run --frozen --all-extras ty check --project packages/server); uv lock --check clean.

Before the rebase onto #1670's merge: At this PR's head 26d0c33e7, on 2026-10-13, rebuilt on #1702's 6631b5c77: ruff check and ruff format --check clean; ty check clean as CI runs it (uv run --frozen --all-extras ty check --project packages/server); uv lock --check clean.

Before the rebuild on #1702's filter routing: At this PR's head 8b9b271ae, on 2026-10-09, on feat/horizontal-scaling with #1813 merged: ruff check and ruff format --check clean; ty check clean as CI runs it (uv run --frozen --all-extras ty check --project packages/server); uv lock --check clean.

Before the rebase onto #1813's merge: At this PR's head ec03a19be, on 2026-10-09: ruff check and ruff format --check clean; ty check clean as CI runs it (uv run --frozen --all-extras ty check --project packages/server); uv lock --check clean; the SQLite stores' and search engines' tests pass, 233 tests. The full suites last passed at b8f302699, on 2026-10-08, before #1628 moved beneath this PR: the server suite without integration tests, 2108 tests, and the integration tests of the vector stores, the resource manager, episodic memory, and semantic storage, 668 tests, against PostgreSQL 16, Neo4j, Qdrant 1.19.1, and Milvus 2.6.24 in containers..

@edwinyyyu edwinyyyu mentioned this pull request Jul 2, 2026
Closed
12 of 26 tasks
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. If you are still working on this, please push a commit or leave a comment. Reviewers: please respond, or add the keep-open label if this PR should be held open for a longer review cycle.

@github-actions github-actions Bot added the Stale label Aug 3, 2026
@edwinyyyu edwinyyyu added the keep-open Prevents the auto-close task from closing this issue. label Aug 3, 2026
@edwinyyyu edwinyyyu changed the title Atomically swap vector search engine index files on save Atomically and durably swap vector search engine index files on save Aug 5, 2026
@edwinyyyu edwinyyyu changed the title Atomically and durably swap vector search engine index files on save Let the engine own index durability, not the vector store Aug 6, 2026
@edwinyyyu edwinyyyu changed the title Let the engine own index durability, not the vector store Publish vector index files atomically, not durably Aug 12, 2026
@edwinyyyu edwinyyyu mentioned this pull request Aug 13, 2026
Draft
10 of 15 tasks
@edwinyyyu edwinyyyu changed the title Publish vector index files atomically, not durably Publish vector index files atomically, but not durably Sep 8, 2026
@edwinyyyu edwinyyyu changed the title Publish vector index files atomically, but not durably Publish vector index files atomically (but not durably) Sep 8, 2026
This was referenced Sep 8, 2026
Merged
Closed
This was referenced Sep 16, 2026
Merged
Closed
Merged
Closed
Merged
Merged
@edwinyyyu edwinyyyu changed the title Publish vector index files atomically (but not durably) [speedkick port 7/17] Publish vector index files atomically (but not durably) (port of #1588) Sep 16, 2026
This was referenced Sep 16, 2026
Draft
Closed
Draft
Closed
Closed
Closed
Closed
Closed
Merged
This was referenced Oct 14, 2026
Draft
Open
@xiongzubiao
xiongzubiao force-pushed the feat/horizontal-scaling branch from 101182c to cf7ae15 Compare October 15, 2026 15:31
@edwinyyyu
edwinyyyu force-pushed the atomic_index_swap branch 14 times, most recently from 3debee2 to da68f73 Compare October 20, 2026 21:00
This was referenced Oct 20, 2026
Open
Draft
Closed
Draft
Merged
@edwinyyyu edwinyyyu mentioned this pull request Oct 26, 2026
Closed
@edwinyyyu
edwinyyyu force-pushed the atomic_index_swap branch 4 times, most recently from b3f9569 to 96eca3a Compare October 27, 2026 22:48
@edwinyyyu @claude
Publish vector index files atomically (but not durably) (speedkick) (M…
b7062f9 
…emMachine#1588)

* Atomically swap vector search engine index files on save

SQLiteVectorStore persists each collection's index by calling the search
engine's save(), which wrote directly to the final path. A crash mid-write
left a truncated/corrupt file. Because index_saved=True makes the on-disk
index a durable contract (missing/corrupt is a hard IndexLoadError, not a
silent empty rebuild), an interrupted save could render a collection
unrecoverable.

Write the index to a sibling temp file and swap it into place with
os.replace (atomic on POSIX and Windows on the same filesystem), so a reader
sees either the old or new index, never a partial write; a failed save leaves
the previous index intact. Leftover temp files are cleared on load so a crash
does not leak them across restarts.

Implemented in the engines (shared index_persistence helper) rather than in
SQLiteVectorStore/SQLiteVectorStoreCollection, since the index save location
and number of files written differ across engine implementations.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Make the index swap durable, not only atomic

The swap protects a reader from a torn index, but the vector store also
trims its pending-operation log once `save` returns -- and that log is the
only other copy of those vectors, since the records table stores no vector
column. So the swap reaching disk is load-bearing rather than a bonus:

- fsync the parent directory after the replace, since POSIX `rename(2)`
  leaves the new directory entry in the page cache. Best-effort and ignored
  on failure, matching SQLite's `unixSync`; a no-op on Windows, which has no
  equivalent operation.
- stop swallowing a failed fsync of the temp file. SQLite draws the same
  line -- a file fsync failure raises SQLITE_IOERR_FSYNC while a directory
  fsync failure is ignored -- and `EIO` means the writeback already failed
  and the dirty pages were dropped, which is exactly when the save must not
  be reported as committed. The existing cleanup then leaves the previous
  index in place with the log untrimmed, so the next save retries.
- use F_FULLFSYNC on macOS, where plain `fsync` leaves the data in the
  drive's volatile write cache, falling back when a filesystem refuses it.

State the resulting obligation on `VectorSearchEngine.save` itself, since
that is what the store now relies on: replace atomically, then make the
replacement as durable as the platform allows. An engine whose backend
already implements the whole protocol can delegate to it and skip these
helpers; the rest use `atomic_index_write`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Let the engine own index durability, not the vector store

The pending log holds the only durable copy of a vector between
checkpoints -- the records table has no vector column -- so trimming it is
safe only at an instant when the index provably holds those vectors. The
temp-write + rename protocol this PR shipped could not provide that
instant. A rename changes a directory entry, and Windows exposes no way
to flush one: os.fsync is _commit, which is FlushFileBuffers, which is
for file data, and you cannot open a directory to fsync it. The decisive
evidence is SQLite's own -- it threads a directory-sync flag through
every commit-relevant directory operation, honors it in unixDelete, and
declares it /* Not used on win32 */ in winDelete. So os.replace could
return, _save_collection_index could commit its trim durably behind it,
and a power cut could still roll the rename back: records forward, index
back, no copy of the difference left. MOVEFILE_WRITE_THROUGH is not a
fix; its documented guarantee covers copy-and-delete (cross-volume)
moves, not same-volume renames.

Take SQLite's answer, which was not to harden the directory operation
but to stop using one as a commit point (PERSIST commits by zeroing a
header, TRUNCATE by truncating, WAL by appending frames).

A base path now expands into two index slots plus a generation record
each, created once and thereafter only overwritten. A checkpoint writes
the index over the inactive slot and flushes it, then writes that slot's
generation record and flushes that. The record is the commit, and it is
a write into a file that already exists. It holds the generation and its
bitwise complement, so a torn write reads as absent rather than as some
other generation -- all or nothing without needing single-sector
atomicity from the hardware. load takes the highest believable
generation, and deliberately does not fall back to the older slot when
the published index will not parse: the log was trimmed against the
newer one, so the older is stale by exactly the ops that can no longer
be replayed.

Both backends already write straight to the path they are given, which
is what this protocol wants -- verified that repeated saves preserve the
inode and leave no stray files -- so no engine gains a temp file, a
buffer, or a rename.

Durability is entirely the engine's, including which artifact is live.
The store keeps no slot pointer, manifest, or generation, so no schema
change and no migration: what remains is one rule, never trim past what
save says is durable, and _save_collection_index already had that order.
index_path becomes index_base_path since it no longer names a file, and
discarding a collection asks the engine layer which files that covers.

BREAKING CHANGE: an index written by the previous protocol is not
published under the new one, so a collection with index_saved=True
raises IndexLoadError until its index directory is cleared and the
records re-ingested.

Anomaly tests walk every crash point in the publish sequence by
constructing the on-disk state each would leave, plus one that pins the
ordering itself (a failed index write must publish nothing) since
state-based tests cannot observe it. Verified against three deliberate
breaks -- dropping the complement check, writing the record first, and
reusing one slot instead of alternating -- each caught.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Publish the index atomically, and stop promising durability

The two-slot generation-record protocol bought a guarantee we have
decided not to make: that a save survives a power failure. Every engine
would have to implement and maintain that protocol, and the failure it
buys out is bounded -- search recall for the records applied since the
last checkpoint, repaired by re-ingesting them. The direction that
actually costs, a published index that will not parse, is closed by the
atomic swap on its own.

So this returns to the temp-file-plus-rename publication and spends the
difference on stating the contract instead of strengthening it: `save`
publishes atomically, never durably; the store trims the pending log
behind a publication a power failure can revert; a record whose vector
is lost that way still resolves by uuid, is absent from search until it
is upserted again, and nothing here detects the gap for the caller.

Reverts the durability and engine-owned-publication commits, keeps the
atomic swap, and adds a store-level test that reconstructs a reverted
publication deterministically -- restore the previous index bytes after
the trim -- to pin the direction it fails in.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Report a lost embedding as lost, not as a missing feature

`update_feature` reads the stored embedding back when a caller updates a
feature without supplying one, and that is the only place in the server
that depends on the index still holding a vector. With publication now
atomic rather than durable, a power failure can leave a feature whose
row is intact and whose vector is not -- a state this path reported as
"Vector record not found", which points the caller at the wrong thing
and hides the repair.

Split the two cases. A record that is genuinely absent keeps the old
message; a record whose embedding the index no longer holds says so and
names the fix, which is to pass a fresh embedding.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Let a failed fsync fail the save, and cut the essay around it

`_flush_to_disk` wrapped its fsync in `contextlib.suppress(OSError)` and
called itself best-effort. A failed fsync is exactly the evidence that the
bytes are not safe to publish -- on Linux an EIO from fsync means writeback
failed, reported once and then cleared -- so swallowing it and renaming
anyway published a file we had positive evidence was bad. The safeguard
cost something and, in the one case it existed for, guaranteed nothing.
Nothing tested it either.

Let it propagate. `atomic_index_write` already unlinks the temp and
re-raises, so a failed flush now leaves the previously published index
standing, which is the correct outcome. A test pins that.

The fsync is not best-effort, and the docstring should not have said so: it
rules out a class rather than narrowing a window. Because the flush
completes before the rename is issued, and a durable write does not
un-happen, the new name can never appear over incomplete bytes. What the
missing directory fsync costs is the other direction -- the rename may not
survive, so the publish reverts -- and that is the benign one this store
already accepts.

The module docstring was 76 lines against 49 of everything else, most of it
argument rather than documentation: a walk through SQLite's `unixDelete` /
`winDelete` sync-flag handling, and a rejected two-slot commit protocol.
That is the PR's case for the design, not something to re-read every time
someone opens a 20-line module, and the PR body carries it. What a reader
here needs is the guarantee, the non-guarantee, and the cost.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NKmF9xNph9QH3ozNw3ZnJL

* Say why the swap is a rename, and what the reopened fd cannot see

Two things the module was silent on.

Why a rename at all. The stronger answer is to put the commit point inside
the file, where an fsync reaches it portably -- SQLite never renames, and
commits by truncating or zeroing its rollback journal, or in WAL mode by
appending frames whose checksums make a torn tail self-identifying. Both
need the writer to own the file format. A search engine owns its own and
exposes `save(path)`, so above that call a rename is the only atomicity
primitive left, and an engine whose format already commits that way needs
none of this. Worth saying, because "why not do the better thing" is the
first question the module invites.

What the reopened descriptor cannot see. Flushing is fine on a fresh fd --
dirty pages belong to the file, not to the descriptor that dirtied them --
but error reporting is not: Linux hands a writeback error to descriptors
open when it was recorded, so one recorded between the engine's close and
this open is never reported and the save proceeds on bytes already known
bad. Same shape as the 2018 PostgreSQL fsync report. It cannot be closed
from here: the engine writes through its own descriptor and closes it
before returning, and closing the window needs an engine that writes
through a handle the caller supplies.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NKmF9xNph9QH3ozNw3ZnJL

* Fsync the index on a descriptor that predates the write

The fsync was on a descriptor opened after the engine had written and
closed its own, which flushes correctly -- dirty pages belong to the file,
not to the descriptor that dirtied them -- but reports nothing useful.
Linux samples the writeback error sequence when a file is opened, so a
descriptor opened after an error was recorded never learns of it: the fsync
returns success and the save publishes bytes already known bad. Same shape
as the 2018 PostgreSQL fsync report.

Open the temp before yielding it and hold it across the caller's write, so
the descriptor predates the bytes and any error from writing them is
reported here, where it fails the save.

That assumes the caller writes in place. Both engines do -- verified: the
inode is unchanged across `save_index` and `save`, and the held descriptor
sees the written size -- but it is their behaviour, not their contract. An
engine that built a file of its own and renamed it over the temp would
leave this descriptor on an orphaned inode, and the fsync would report on a
file nobody is about to publish. So it is checked before the fsync, and a
mismatch fails the save rather than passing it quietly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NKmF9xNph9QH3ozNw3ZnJL

* State the in-place rule where the caller reads it

The descriptor held across the body only flushes what the body wrote if the
body writes the yielded path in place, and that requirement was recorded in
`_flush_to_disk` -- a private function nobody writing an engine opens. It
belongs on `atomic_index_write`, which is the API they use, alongside what
happens when it is broken: an `OSError` and no publication, so the mistake
surfaces at the first save rather than at a power cut.

`_flush_to_disk` keeps the mechanism -- why the descriptor has to predate
the write -- and now points at the rule instead of restating it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NKmF9xNph9QH3ozNw3ZnJL

* Ask the drive to flush on macOS, where fsync does not

`os.fsync` is not the same guarantee on all three platforms this ships to.
On Linux it flushes to the device, and on Windows `FlushFileBuffers` does
the same. Darwin's `fsync` explicitly does not: it returns once the data
reaches the drive, which may hold it in a volatile write cache. So on macOS
the ordering this module is built on -- data durable before the rename is
issued -- did not hold at the device, which is exactly the case it claims
to rule out.

`F_FULLFSYNC` asks the drive to flush that cache. Filesystems that cannot
refuse it, and there `fsync` is the most that can be asked, so a refusal
falls back; any other error is a write failure and propagates, as before.

The flush-failure test patched `os.fsync`, which Darwin no longer reaches.
It patches the module's own `_fsync` instead, which every platform does.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NKmF9xNph9QH3ozNw3ZnJL

* Stop guessing which errnos mean "this drive cannot do that"

The fallback from `F_FULLFSYNC` to `fsync` was gated on an errno allowlist
-- ENOTSUP, EOPNOTSUPP, EINVAL -- so that a genuine write failure would
propagate rather than be quietly downgraded. Probing the actual returns on
Darwin shows the list is both incomplete and partly invented:

  ENOTSUP=45  EOPNOTSUPP=102   distinct here, so both are needed
  /dev/null   F_FULLFSYNC -> ENODEV(19), while fsync succeeds
  pipe/socket F_FULLFSYNC -> EBADF(9)
  EINVAL      never came from F_FULLFSYNC at all; it came from fsync

So ENODEV -- a real refusal, on a path anyone can reproduce -- would have
raised instead of falling back, and EINVAL was in the list by analogy
rather than evidence. What a network mount answers is not knowable from
here, which makes the whole list a guess that fails closed on whatever it
missed. This codebase does not classify driver errors by guessing, and
this was that.

Fall through on any failure instead. It is not a suppression: `fsync` runs
on the same descriptor and raises in its turn, so a flush that cannot
happen still fails the save. What the fallback gives up is the drive-cache
flush -- the guarantee this had before `F_FULLFSYNC` was asked for at all.
That is also what SQLite does with this same call, for the same reason.

The test drives it through `/dev/null`, which refuses with ENODEV and
accepts `fsync`; it fails against the allowlist and passes without it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NKmF9xNph9QH3ozNw3ZnJL

* Fail the save when the drive cannot be told to flush

`F_FULLFSYNC` failing fell through to `fsync`, on the reasoning that a
refusal is a statement about the filesystem rather than a write failure.
That reasoning does not survive asking what `fsync` alone actually buys on
Darwin.

Against a process or kernel crash it is enough: the data has left the OS
for the drive before the rename is issued, so the rename dies in the page
cache and the old index stands. Against power loss it is not. The data
sits in the drive's volatile cache, the rename's metadata joins it moments
later, and nothing orders them -- and the rename is a few bytes against an
index of megabytes, so a drive flushing as it pleases can easily put the
new name on media while the bytes behind it are still queued. That is the
torn publication this module exists to prevent, in precisely the scenario
its docstring is about.

So the fallback answered a request for ordering with a flush that does not
provide it, and said nothing. A filesystem that cannot order data ahead of
a rename is not one to publish an index onto; raise, and let the operator
point `index_directory` at storage that can.

This is also the simpler code. Refusal and failure now take the same path,
so no errno is inspected -- there is no line to draw and no list to get
wrong, which is what the previous two revisions kept getting wrong in
opposite directions. The `/dev/null` test went with the fallback it pinned.

The earlier defence of falling back rested on network and FUSE mounts
being a realistic home for an index directory. They are not.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NKmF9xNph9QH3ozNw3ZnJL

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
(cherry picked from commit 21105d6)

This branch has not been deployed

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

Reviewers

No reviews

Assignees

No one assigned

Labels

keep-open Prevents the auto-close task from closing this issue. Stale

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@edwinyyyu