From 3bc8636e62d232f1b928e3c788053f03c0db0ba7 Mon Sep 17 00:00:00 2001 From: stumpylog <797416+stumpylog@users.noreply.github.com> Date: Thu, 30 Jul 2026 07:20:50 -0700 Subject: [PATCH] Fix: guard compact() against unmigrated stores, apply final-review cleanups compact() had no migration guard: on a v1-schema store, document_chunks reads 0 (freshly created empty) while total_inserts reflects the real cumulative count, so the bloat check nearly always rebuilt -- silently losing document_meta (copy_all reads from the empty v1 table) while schema_version copied across unchanged, leaving the store permanently unmigratable. compact() now calls has_pending_migration() and no-ops with a warning instead. Also folds in five minor final-review findings: drop _rebuild_into's unused int return, hoist test-local imports to module level in test_vector_store.py, note in TestMigrations' docstring that its fake structural migrations only exercise dispatch (not full schema correctness), restore the comment explaining why _row() requires document_id, and tighten increment_total_inserts' docstring to not imply general concurrency safety beyond its single atomic statement. Co-Authored-By: Claude Sonnet 5 --- src/paperless_ai/tables.py | 13 +++-- src/paperless_ai/tests/test_vector_store.py | 61 +++++++++++++++++---- src/paperless_ai/vector_store.py | 25 +++++++-- 3 files changed, 77 insertions(+), 22 deletions(-) diff --git a/src/paperless_ai/tables.py b/src/paperless_ai/tables.py index 850c0f862..f3ac5710f 100644 --- a/src/paperless_ai/tables.py +++ b/src/paperless_ai/tables.py @@ -216,11 +216,14 @@ class IndexMetaTable: @staticmethod def increment_total_inserts(conn: sqlite3.Connection, count: int) -> None: - """Atomically add ``count`` to the stored counter in one statement - (INSERT .. ON CONFLICT DO UPDATE with arithmetic), instead of a - separate read-then-write -- called once per add()/upsert_document(), - so halving the statement count here is a real, if small, per-call - saving. index_meta.value has TEXT affinity, so the incremented + """Add ``count`` to the stored counter in one SQL statement (INSERT + .. ON CONFLICT DO UPDATE with arithmetic), instead of a separate + read-then-write -- called once per add()/upsert_document(), so + halving the statement count here is a real, if small, per-call + saving. This only avoids a read-then-write race within this single + statement; it does not make the counter safe against concurrent + writers in general (callers still rely on the write FileLock for + that). index_meta.value has TEXT affinity, so the incremented result is stored as its text representation -- get_total_inserts() already expects that (int(value)), so this is not a behavior change, only fewer statements. diff --git a/src/paperless_ai/tests/test_vector_store.py b/src/paperless_ai/tests/test_vector_store.py index a7defa58d..e4cf883bb 100644 --- a/src/paperless_ai/tests/test_vector_store.py +++ b/src/paperless_ai/tests/test_vector_store.py @@ -1,8 +1,10 @@ +import inspect import sqlite3 from collections.abc import Generator from pathlib import Path import pytest +import sqlite_vec from llama_index.core.schema import TextNode from llama_index.core.vector_stores.types import FilterOperator from llama_index.core.vector_stores.types import MetadataFilter @@ -12,6 +14,9 @@ from pytest_mock import MockerFixture from paperless_ai.migrations import MIGRATIONS from paperless_ai.migrations import Migration +from paperless_ai.migrations import m0001_v1_to_v2 +from paperless_ai.tables import DocumentChunksTable +from paperless_ai.tables import DocumentMetaTable from paperless_ai.vector_store import DB_FILENAME from paperless_ai.vector_store import DEFAULT_TABLE_NAME from paperless_ai.vector_store import SCHEMA_VERSION @@ -534,6 +539,41 @@ class TestCompact: store.compact(force=True) assert store.get_modified_times() == before + def test_compact_on_unmigrated_store_is_noop( + self, + store: PaperlessSqliteVecVectorStore, + mocker: MockerFixture, + ) -> None: + """ + GIVEN: + - A store whose schema_version has been forced behind + SCHEMA_VERSION (has_pending_migration() is True) + WHEN: + - compact(force=True) is called directly, without the caller + having run check_and_run_migrations() first + THEN: + - compact() is a safe no-op: no file-swap rebuild is attempted + at all (asserted via a spy on _rebuild_into, since schema_version + alone is not a reliable signal -- a rebuild would otherwise + copy the stale schema_version across unchanged, making a + before/after equality check pass even when a rebuild *did* + happen). Rebuilding an unmigrated store would silently lose + document_meta and leave the swapped-in file claiming the old + schema_version -- see the class docstring rationale. + """ + store.add([make_node("a1", 1)]) + store.client.execute( + "UPDATE index_meta SET value = '0' WHERE key = 'schema_version'", + ) + assert store.has_pending_migration() is True + rebuild_spy = mocker.spy(PaperlessSqliteVecVectorStore, "_rebuild_into") + store.compact(force=True) + rebuild_spy.assert_not_called() + row = store.client.execute( + "SELECT value FROM index_meta WHERE key = 'schema_version'", + ).fetchone() + assert int(row["value"]) == 0 + class TestDbFile: def test_single_db_file_in_index_dir(self, store, tmp_path: Path) -> None: @@ -556,6 +596,16 @@ class TestMigrations: migration. Test migrations use version numbers starting at SCHEMA_VERSION (2) and above so they never collide with the real from_version=1/to_version=2 migration already registered in MIGRATIONS. + + The fake structural migrations' apply() fixtures (see + test_structural_migration_copies_rows_and_updates_version and + test_stop_at_reembed_boundary below) only populate the rebuilt vec0 + table itself -- they never insert into document_chunks/document_meta on + the destination connection. That's fine here: these tests exist to + verify the generic dispatch mechanism (version bookkeeping, structural- + vs-reembed branching), not full schema correctness of a rebuilt store; + the real migration's data completeness is covered separately by + TestV1ToV2Migration. """ def _schema_version(self, store: PaperlessSqliteVecVectorStore) -> int | None: @@ -815,8 +865,6 @@ class TestV1ToV2Migration: conn = sqlite3.connect(db_path) conn.row_factory = sqlite3.Row conn.enable_load_extension(True) # noqa: FBT003 - import sqlite_vec - sqlite_vec.load(conn) conn.enable_load_extension(False) # noqa: FBT003 conn.execute("PRAGMA journal_mode=WAL") @@ -913,8 +961,6 @@ class TestV1ToV2Migration: """ db_dir = tmp_path self._build_v1_store(str(db_dir / DB_FILENAME), dim=16) - import sqlite_vec - conn = sqlite3.connect(str(db_dir / DB_FILENAME)) conn.enable_load_extension(True) # noqa: FBT003 sqlite_vec.load(conn) @@ -949,9 +995,6 @@ class TestV1ToV2Migration: """ db_dir = tmp_path self._build_v1_store(str(db_dir / DB_FILENAME), dim=16) - from paperless_ai.tables import DocumentChunksTable - from paperless_ai.tables import DocumentMetaTable - chunks_create_spy = mocker.spy(DocumentChunksTable, "create") meta_create_spy = mocker.spy(DocumentMetaTable, "create") vec_table_spy = mocker.spy( @@ -980,10 +1023,6 @@ class TestV1ToV2Migration: # Cheap secondary signal, kept alongside the spy assertions above # (not in place of them): the migration module's source should never # even mention these "current schema" helpers by name. - import inspect - - from paperless_ai.migrations import m0001_v1_to_v2 - source = inspect.getsource(m0001_v1_to_v2) assert "DocumentChunksTable.create" not in source assert "DocumentMetaTable.create" not in source diff --git a/src/paperless_ai/vector_store.py b/src/paperless_ai/vector_store.py index 8b2e2881a..bb3007323 100644 --- a/src/paperless_ai/vector_store.py +++ b/src/paperless_ai/vector_store.py @@ -319,6 +319,14 @@ class PaperlessSqliteVecVectorStore(BasePydanticVectorStore): document_id = node.ref_doc_id or node.metadata.get("document_id") return _Row( chunk_id=node.node_id, + # document_id is required -- int(None) raises TypeError and + # int("not-a-number") raises ValueError, both intentional: + # fail loudly on a malformed/missing document_id rather than + # silently indexing a chunk with no owning document. modified, + # below, still uses the str(x or "") sentinel pattern because a + # missing modified value is legitimate (vec0 no longer even + # stores it -- see document_meta), whereas document_id must + # always be present. document_id=int(document_id), modified=str(node.metadata.get("modified") or ""), node_content=json.dumps(meta), @@ -545,6 +553,12 @@ class PaperlessSqliteVecVectorStore(BasePydanticVectorStore): """ if not self.table_exists(): return + if self.has_pending_migration(): + logger.warning( + "Skipping compact: store has a pending schema migration; " + "run check_and_run_migrations() first", + ) + return live = DocumentChunksTable.count(self._conn) total = IndexMetaTable.get_total_inserts(self._conn) or live if not force and total <= max(live, 1) * COMPACT_BLOAT_RATIO: @@ -566,13 +580,13 @@ class PaperlessSqliteVecVectorStore(BasePydanticVectorStore): src_conn: sqlite3.Connection, dst_conn: sqlite3.Connection, dim: int, - ) -> int: + ) -> None: """Create the vec0 table in ``dst_conn``, copy dim/embed_model from ``src_conn``, and stream every live vec0 row, document_chunks row, - and document_meta row across. Returns the number of vec0 rows - copied. Used by compact() only -- m0001_v1_to_v2 freezes its own - copy loop instead of calling this, since this always reflects the - *current* schema (see the migration DDL-freezing rule in the spec). + and document_meta row across. Used by compact() only -- + m0001_v1_to_v2 freezes its own copy loop instead of calling this, + since this always reflects the *current* schema (see the migration + DDL-freezing rule in the spec). """ PaperlessSqliteVecVectorStore._create_vec_table(dst_conn, dim) dim_value = IndexMetaTable.get_dim(src_conn) @@ -613,7 +627,6 @@ class PaperlessSqliteVecVectorStore(BasePydanticVectorStore): # Reset the cumulative counter: after a rebuild, total_inserts == live. IndexMetaTable.reset_total_inserts(dst_conn, copied) dst_conn.execute("COMMIT") - return copied def _swap_in_compact(self, compact_path: str, db_path: str) -> None: """Atomically replace the live database with the compacted copy."""