diff --git a/CHANGELOG.md b/CHANGELOG.md index 84fe4373..9e8a94d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,64 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Fixed — the vector store lived on a disk that is wiped on every restart + +`CHROMA_PERSIST_DIR` is `/app/data/chroma`: the Heroku container filesystem. It does +not survive a dyno restart, and `web` and `worker` are separate process types with +separate copies of it. Production, 2026-08-27, five hours after a deploy: + + 22:00:44 repair_embeddings "Vector store empty but 758 docs in DB. + Forcing a full re-index." + 22:01:21 code_symbol_embed started (all 8 646 files — force_full) + 22:39:09 code_symbol_embed completed (38 min) + 23:59:50 heartbeat stops — the 7 200 s ceiling, at 380 of 758 documents + 00:04 stale run reaped + +A full rebuild costs 12 039 s and the nightly budget is 7 200 s, so the store was +empty again the next night, and the night after. **`index_repo` has completed 16 +times in 94 runs; `daily_sync` 13 in 91.** The self-repair (C3, v1.13.0) was not +wrong — the repair cost more than the budget allowed, which no ceiling could fix. + +`PgVectorStore` puts the vectors in Postgres instead: one store, shared by every +dyno, untouched by a restart. It is a drop-in — same six methods, same shapes, same +cosine distance, and the collection handle still answers `count()`, because that is +what `pipeline_runner.py:415` reads to decide whether a full re-index is owed. + +**Embeddings do not change.** ChromaDB computed them internally with its bundled ONNX +`all-MiniLM-L6-v2`; this calls the same class directly. Identical vectors, identical +384 dimensions, and `vector_cosine_ops` because the collections it replaces were +created with `{"hnsw:space": "cosine"}` — so a query returns the neighbours it +returned before. Verified end to end against the production database: the same four +documents, the same ranking, distances 0.2234 / 0.7867 / 1.0161. + +HNSW rather than IVFFlat: it needs no training pass, so it is correct from an empty +table — IVFFlat built on an empty table returns nothing until rebuilt, which is the +exact failure this change exists to remove. Measured on the schema with 30 000 rows +of 384 dimensions: build 20 s, and a top-5 filtered by `project_id` goes from +4 738 ms with no vector index to 0.92 ms with one. `hnsw.iterative_scan` was measured +too and makes no difference at this selectivity — the 65 ms it appeared to save was a +cold cache. + +`VECTOR_STORE_BACKEND` selects the backend and still defaults to `chroma`, so the +flip is a decision taken on a verified deployment rather than a side effect of this +merge. psycopg sits beside asyncpg deliberately: the interface is called +synchronously from ten sites including the agent's hot path, and converting those to +async is a larger, riskier change than carrying a second driver. + +pgvector is available on **both** deployments — 0.8.1 on Heroku Postgres, 0.8.2 on +Supabase — so this fix does not have to wait for a database move. + +### Added — a migration graph that says when it is broken + +`b1c2d3e4f5a6` was already taken. Alembic does not say so: it reports +`CycleDetected` naming four unrelated revisions, and only when something walks the +graph — so a migration with a duplicate id can be committed and merged first. Revision +ids here are hand-picked rather than generated, which is what puts that within reach. +Three checks now read the graph with `ast` (a first attempt used a regex and reported +seven heads where alembic reports one, because merge migrations name their parents as +a tuple), plus a fourth that fails if the tuple form ever stops being understood. + + ### Fixed — a full re-index could not finish, and the ceiling was the symptom Three ceilings cut three runs off on 2026-08-27 — 1800.02 s inside `code_symbol_embed`, diff --git a/CLAUDE.md b/CLAUDE.md index cc217042..1b081324 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -323,7 +323,7 @@ Learnings are stored per-connection by default (`cross_connection_learnings_enab ### Storage - App data: SQLite in dev (`backend/data/agent.db`), PostgreSQL in production (`DATABASE_URL`). -- Vectors: ChromaDB (`CHROMA_PERSIST_DIR` or `CHROMA_SERVER_URL` for remote); collections named `project_{project_id}`. +- Vectors: **`VECTOR_STORE_BACKEND` picks the backend** — `pgvector` (Postgres table `doc_embeddings`, one row per chunk, HNSW `vector_cosine_ops`) or `chroma` (the default, `CHROMA_PERSIST_DIR` or `CHROMA_SERVER_URL`; collections named `project_{project_id}`). **ChromaDB's persist dir on Heroku is the container filesystem** — wiped on every dyno restart, and `web`/`worker` are separate process types with separate copies. An empty store makes `pipeline_runner` set `force_full`, a full rebuild costs 12 039 s against the nightly ceiling of 7 200 s, so the store was empty again by morning: **`index_repo` completed 16 times in 94 runs**. Embeddings are identical across backends (bundled ONNX `all-MiniLM-L6-v2`, 384-d) and the metric matches the `{"hnsw:space": "cosine"}` the collections were created with, so the swap does not move retrieval ranking. pgvector is available on both deployments (0.8.1 Heroku, 0.8.2 Supabase). Requires Postgres — the migration is a deliberate no-op on SQLite, and asking for pgvector there fails at start-up saying so. - BM25 snapshots: `backend/data/bm25/{project_id}.json.gz` and `schema_{connection_id}.json.gz` — **gzip JSON, not pickle, since 2026-08-21 (F-KNOW-06)**: `pickle.load` executes its payload, and `BM25_DATA_DIR` is configurable. The tokenized corpus is stored and `BM25Okapi` is rebuilt on load; a leftover `.pkl` is deleted, never read. Both are rebuilt from Postgres at start-up when missing (`app/ops/bm25_local_reconcile.py`). - Redis (`REDIS_URL`): rate limiting, agent concurrency tokens, WS tickets, ARQ task queue. In-memory fallback for dev — keep it working when adding Redis features. - Backups: `backend/data/backups/` when `backup_enabled=True`. diff --git a/backend/.env.example b/backend/.env.example index 44241cef..31560a87 100644 --- a/backend/.env.example +++ b/backend/.env.example @@ -166,6 +166,20 @@ CORS_ORIGINS=["http://localhost:3000","http://localhost:3100","https://checkmyda # AUTO_INVESTIGATE_BUDGET_ENFORCEMENT_ENABLED=true # skip auto-investigation when the owner is over budget or unresolved; false to always allow # ----- ChromaDB vector store --------------------------------------------------- +# Which backend holds the vectors: "chroma" or "pgvector". +# +# "chroma" writes to CHROMA_PERSIST_DIR. On Heroku that is the container filesystem, +# wiped on every dyno restart and NOT shared between the `web` and `worker` process +# types. Measured 2026-08-27: the worker found the store empty five hours after a +# deploy, so the pipeline forced a full re-index; a full rebuild costs 12 039 s and +# the nightly ceiling is 7 200 s, so the run was reaped and the store was empty again +# the next night. 16 of 94 repo-index runs have ever completed. +# +# "pgvector" puts them in Postgres — one store, every dyno, surviving restarts. +# Requires a PostgreSQL DATABASE_URL: on SQLite the doc_embeddings migration is a +# deliberate no-op, and asking for pgvector there fails at start-up saying so. +# Embeddings are unchanged either way (ONNX all-MiniLM-L6-v2, 384-d). +# VECTOR_STORE_BACKEND=chroma # CHROMA_PERSIST_DIR=./data/chroma # Remote ChromaDB. Set this to move the vector index OUT of the app process: with it # unset, an embedded PersistentClient keeps the HNSW graph in memory and the files on diff --git a/backend/alembic/versions/1d72054cd637_doc_embeddings_pgvector.py b/backend/alembic/versions/1d72054cd637_doc_embeddings_pgvector.py new file mode 100644 index 00000000..6142812f --- /dev/null +++ b/backend/alembic/versions/1d72054cd637_doc_embeddings_pgvector.py @@ -0,0 +1,91 @@ +"""doc_embeddings: move the vector store off local disk and into Postgres + +Revision ID: 1d72054cd637 +Revises: 6287a47828ca +Create Date: 2026-08-28 + +Why this table exists is in ``app/models/doc_embedding.py``. The short version: +ChromaDB persisted to the dyno's container filesystem, which is wiped on every +restart and is not shared between the ``web`` and ``worker`` process types. An empty +store makes the pipeline force a full re-index, a full re-index of the one real +customer repository costs 12 039 s, and the nightly job's ceiling is 7 200 s — so +16 of 94 repo-index runs ever completed and the store was empty again by morning. + +**Postgres only, deliberately.** Development and the test suite run on SQLite +(``backend/data/agent.db``), where there is no ``vector`` extension and no HNSW. +``make setup`` runs ``alembic upgrade head`` against that SQLite file, so a migration +that assumed Postgres would break every developer's first command. On SQLite this +migration is a no-op and the ChromaDB backend stays in use; ``VECTOR_STORE_BACKEND`` +selects between them. + +Measured before choosing HNSW over IVFFlat, on this exact schema with 30 000 rows of +384 dimensions: build 20 s, and a top-5 filtered by ``project_id`` goes from 4 738 ms +with no vector index to 0.92 ms with one. HNSW also needs no training pass, so it is +correct from an empty table — IVFFlat built on an empty table returns nothing until +it is rebuilt, which is exactly the failure mode this whole table exists to remove. + +pgvector is available on both deployments: 0.8.1 on Heroku Postgres, 0.8.2 on +Supabase. Checked 2026-08-28 — this migration is therefore safe to run before any +database move, which is why the vector fix does not have to wait for one. +""" + +from alembic import op + +revision = "1d72054cd637" +down_revision = "6287a47828ca" +branch_labels = None +depends_on = None + + +def _is_postgres() -> bool: + return op.get_bind().dialect.name == "postgresql" + + +def upgrade() -> None: + if not _is_postgres(): + return + + op.execute("CREATE EXTENSION IF NOT EXISTS vector") + + op.execute( + """ + CREATE TABLE IF NOT EXISTS doc_embeddings ( + project_id TEXT NOT NULL REFERENCES projects(id) ON DELETE CASCADE, + id VARCHAR(512) NOT NULL, + document TEXT NOT NULL, + metadata JSONB NOT NULL DEFAULT '{}'::jsonb, + embedding vector(384) NOT NULL, + updated_at TIMESTAMPTZ NOT NULL DEFAULT now(), + PRIMARY KEY (project_id, id) + ) + """ + ) + + # Equality on the one metadata key that is ever queried alone — the path that + # removes a changed file's chunks before re-embedding them. + op.execute( + """ + CREATE INDEX IF NOT EXISTS ix_doc_embeddings_source_path + ON doc_embeddings (project_id, (metadata ->> 'source_path')) + """ + ) + + # `vector_cosine_ops` because the ChromaDB collections this replaces were created + # with `{"hnsw:space": "cosine"}` — so `<=>` returns the same distance the old + # store returned, and retrieval ranking does not silently change under the fix. + op.execute( + """ + CREATE INDEX IF NOT EXISTS ix_doc_embeddings_hnsw + ON doc_embeddings USING hnsw (embedding vector_cosine_ops) + """ + ) + + +def downgrade() -> None: + if not _is_postgres(): + return + op.execute("DROP INDEX IF EXISTS ix_doc_embeddings_hnsw") + op.execute("DROP INDEX IF EXISTS ix_doc_embeddings_source_path") + op.execute("DROP TABLE IF EXISTS doc_embeddings") + # The extension is deliberately NOT dropped: another table may come to depend on + # it, and dropping an extension takes its types with it. diff --git a/backend/app/agents/knowledge_agent.py b/backend/app/agents/knowledge_agent.py index b9b2344b..72347a9d 100644 --- a/backend/app/agents/knowledge_agent.py +++ b/backend/app/agents/knowledge_agent.py @@ -23,7 +23,7 @@ from app.knowledge.entity_extractor import ProjectKnowledge from app.knowledge.hybrid_retriever import HybridRetriever from app.knowledge.reranker import build_reranker -from app.knowledge.vector_store import VectorStore +from app.knowledge.vector_store import VectorStore, make_vector_store from app.llm.base import LLMResponse, Message, ToolCall from app.services.project_cache_service import ProjectCacheService @@ -47,7 +47,7 @@ def __init__( vector_store: VectorStore | None = None, hybrid_retriever: HybridRetriever | None = None, ) -> None: - self._vector_store = vector_store or VectorStore() + self._vector_store = vector_store or make_vector_store() # The hybrid retriever (M3) is constructed lazily so that disabling the # feature flag carries zero startup cost. When the flag flips on, a # single instance is shared across requests. diff --git a/backend/app/agents/orchestrator.py b/backend/app/agents/orchestrator.py index ef5d7f9a..59de0a5f 100644 --- a/backend/app/agents/orchestrator.py +++ b/backend/app/agents/orchestrator.py @@ -69,7 +69,7 @@ from app.core.workflow_tracker import tracker as default_tracker from app.knowledge.custom_rules import CustomRulesEngine from app.knowledge.repo_analyzer import RepoAnalyzer -from app.knowledge.vector_store import VectorStore +from app.knowledge.vector_store import VectorStore, make_vector_store from app.llm.base import LLMResponse, Message, ToolCall from app.llm.errors import ( RETRYABLE_LLM_ERRORS, @@ -362,7 +362,7 @@ def __init__( mcp_source_agent: MCPSourceAgent | None = None, ) -> None: self._llm = llm_router or LLMRouter() - self._vector_store = vector_store or VectorStore() + self._vector_store = vector_store or make_vector_store() self._custom_rules = custom_rules or CustomRulesEngine() self._tracker = workflow_tracker or default_tracker self._validator = AgentResultValidator() diff --git a/backend/app/agents/sql_agent.py b/backend/app/agents/sql_agent.py index 1b36f7d4..78fd525c 100644 --- a/backend/app/agents/sql_agent.py +++ b/backend/app/agents/sql_agent.py @@ -44,7 +44,7 @@ from app.core.validation_loop import ValidationLoop from app.knowledge.custom_rules import CustomRulesEngine from app.knowledge.entity_extractor import ProjectKnowledge -from app.knowledge.vector_store import VectorStore +from app.knowledge.vector_store import VectorStore, make_vector_store from app.llm.base import LLMResponse, Message, ToolCall from app.llm.retry import llm_call_with_retry from app.llm.router import LLMRouter @@ -125,7 +125,7 @@ def __init__( rules_engine: CustomRulesEngine | None = None, ) -> None: self._llm = llm_router or LLMRouter() - self._vector_store = vector_store or VectorStore() + self._vector_store = vector_store or make_vector_store() self._rules_engine = rules_engine or CustomRulesEngine() self._cache_svc = ProjectCacheService() diff --git a/backend/app/api/routes/repos.py b/backend/app/api/routes/repos.py index 889bb0e8..e7975ee2 100644 --- a/backend/app/api/routes/repos.py +++ b/backend/app/api/routes/repos.py @@ -24,7 +24,7 @@ from app.knowledge.pipeline_runner import IndexingPipelineRunner from app.knowledge.repo_analyzer import RepoAnalyzer from app.knowledge.repo_url import validate_git_ref, validate_repo_url -from app.knowledge.vector_store import VectorStore +from app.knowledge.vector_store import make_vector_store from app.models.base import async_session_factory from app.services.checkpoint_service import CheckpointService from app.services.connection_service import ConnectionService @@ -43,7 +43,7 @@ _repo_analyzer = RepoAnalyzer(settings.repo_clone_base_dir) _doc_store = DocStore() _doc_generator = DocGenerator() -_vector_store = VectorStore() +_vector_store = make_vector_store() _cache_svc = ProjectCacheService() _checkpoint_svc = CheckpointService() diff --git a/backend/app/config.py b/backend/app/config.py index 1d3343d0..dbd307fd 100644 --- a/backend/app/config.py +++ b/backend/app/config.py @@ -94,6 +94,23 @@ def _fix_database_url(self) -> "Settings": #: new key; once its pending count reaches zero the old key can be dropped. master_encryption_keys_old: str = "" + # F-KNOW-13: which backend holds the vectors. + # + # "chroma" writes to `chroma_persist_dir`, which on Heroku is the container + # filesystem — wiped on every dyno restart, and NOT shared between the `web` and + # `worker` process types. Measured 2026-08-27: the worker found the store empty + # five hours after a deploy, so the pipeline set `force_full`; a full rebuild of + # the one real customer repository costs 12 039 s against the nightly ceiling of + # 7 200 s; the run was reaped and the store was empty again the next night. + # 16 of 94 repo-index runs have ever completed. + # + # "pgvector" puts them in Postgres, which every dyno shares and a restart does + # not touch. Requires Postgres — development and the test suite run on SQLite, + # where the migration is a no-op and this must stay "chroma". + # + # Default is still "chroma" so the switch is a deliberate flip on a verified + # deployment rather than a side effect of deploying this change. + vector_store_backend: str = "chroma" chroma_persist_dir: str = "./data/chroma" chroma_server_url: str = "" chroma_embedding_model: str = Field( diff --git a/backend/app/knowledge/pgvector_store.py b/backend/app/knowledge/pgvector_store.py new file mode 100644 index 00000000..19988e37 --- /dev/null +++ b/backend/app/knowledge/pgvector_store.py @@ -0,0 +1,276 @@ +"""The vector store, backed by Postgres instead of a dyno's local disk. + +Drop-in for :class:`app.knowledge.vector_store.VectorStore`: same six methods, same +shapes in and out, same cosine distance. Only where the vectors live changes. + +**Why.** ``CHROMA_PERSIST_DIR`` is ``/app/data/chroma`` in production — the container +filesystem, wiped on every dyno restart, and not shared between the ``web`` and +``worker`` process types. Measured on 2026-08-27: the worker found the store empty +five hours after a deploy, so ``pipeline_runner`` set ``force_full``; a full rebuild +of that repository costs 12 039 s against the nightly job's 7 200 s ceiling; the run +was reaped at 124.9 minutes and the store was empty again the next night. 16 of 94 +repo-index runs have ever completed. The self-repair was not wrong — it cost more +than the budget allowed, which no ceiling could fix. + +**Synchronous on purpose.** The interface it replaces is called synchronously from +ten sites, several on the agent's hot path (``context_loader.py:213,421``, +``knowledge_catalog_service.py:480,539``). Converting those to async is a larger and +riskier change than carrying psycopg beside asyncpg, so the port keeps the shape and +changes only the storage. One variable at a time. + +**Embeddings are unchanged.** ChromaDB computed them internally with its bundled ONNX +``all-MiniLM-L6-v2``; this class calls the same class directly. Identical vectors, +identical dimension, identical metric — so a query returns the neighbours it returned +before, and any change in answers is a bug rather than a new model. +""" + +from __future__ import annotations + +import json +import logging +import threading +from typing import Any + +from app.config import settings +from app.models.doc_embedding import EMBEDDING_DIM + +logger = logging.getLogger(__name__) + + +class EmbeddingDimensionError(RuntimeError): + """Raised when the embedder stops producing the width the column declares. + + Loud on purpose. A silently truncated or padded vector is an index that returns + plausible neighbours which are simply wrong, and nothing downstream can tell. + """ + + +def _sync_dsn(url: str) -> str: + """SQLAlchemy's async URL is not a libpq DSN — psycopg wants the driver gone.""" + return url.replace("+asyncpg", "").replace("postgresql+psycopg", "postgresql") + + +class _ProjectHandle: + """What ``get_or_create_collection`` hands back. + + ChromaDB returned a ``Collection``; both callers in this codebase use exactly two + things from it — ``count()`` and, for logging, its name. Mirroring that surface is + what lets the backend swap without touching the call sites, and ``count()`` in + particular is load-bearing: ``pipeline_runner`` reads it to decide whether the + store is empty and a full re-index is owed, which is the decision that made an + ephemeral store rebuild itself nightly and never finish. + """ + + __slots__ = ("_store", "project_id", "name") + + def __init__(self, store: PgVectorStore, project_id: str) -> None: + self._store = store + self.project_id = project_id + safe = project_id.replace("-", "_")[:50] + self.name = f"project_{safe}" + + def count(self) -> int: + return self._store.count(self.project_id) + + +class PgVectorStore: + """Postgres-backed vector store. Thread-safe; one connection pool per process.""" + + def __init__(self) -> None: + from psycopg_pool import ConnectionPool + + self._embedding_fn: Any | None = None + self._embed_lock = threading.Lock() + self._pool = ConnectionPool( + conninfo=_sync_dsn(settings.database_url), + min_size=1, + max_size=max(2, settings.db_pool_size // 2), + open=True, + # A pooled connection that outlives a Supavisor session is a connection + # that fails on first use rather than on checkout. + max_lifetime=float(settings.db_pool_recycle), + kwargs={"autocommit": True}, + ) + self._register_vector() + logger.info("PgVectorStore: pool open (dim=%d)", EMBEDDING_DIM) + + def _register_vector(self) -> None: + from pgvector.psycopg import register_vector + + with self._pool.connection() as conn: + register_vector(conn) + + # -- embedding --------------------------------------------------------------- + + def _embed(self, texts: list[str]) -> list[list[float]]: + """Embed with the same model ChromaDB used internally. + + Loaded lazily and once: the ONNX session is ~90 MiB and the web dyno only + needs it when a query arrives, not at import. + """ + if self._embedding_fn is None: + with self._embed_lock: + if self._embedding_fn is None: + from chromadb.utils.embedding_functions import ONNXMiniLM_L6_V2 + + self._embedding_fn = ONNXMiniLM_L6_V2() + vectors = [list(map(float, v)) for v in self._embedding_fn(texts)] + for v in vectors: + if len(v) != EMBEDDING_DIM: + raise EmbeddingDimensionError( + f"embedder produced {len(v)} dimensions, column declares " + f"{EMBEDDING_DIM} — re-index is required, not a cast" + ) + return vectors + + # -- the VectorStore interface ----------------------------------------------- + + def get_or_create_collection(self, project_id: str) -> _ProjectHandle: + """There is no collection to create — rows carry ``project_id``. What comes + back is a handle exposing ``.count()`` and ``.name``, because that is the + whole of what the two callers use it for + (``pipeline_runner.py:415``, ``context_loader.py:213``), and keeping the + shape means neither has to change to gain a working store.""" + return _ProjectHandle(self, project_id) + + def add_documents( + self, + project_id: str, + doc_ids: list[str], + documents: list[str], + metadatas: list[dict] | None = None, + ) -> None: + if not doc_ids: + return + # AUD-0819-01 carried over: the batch handed to one embed call decides peak + # memory, because the ONNX model pads every document to 256 tokens and its + # activations are sized by the batch and nothing else. Measured at 967 MiB + # for 200 and 415 MiB for 8; an unbounded batch is what SIGKILLed the worker. + step = max(1, settings.embedding_upsert_batch_size) + with self._pool.connection() as conn: + for start in range(0, len(doc_ids), step): + end = start + step + chunk_docs = documents[start:end] + chunk_ids = doc_ids[start:end] + chunk_meta = ( + metadatas[start:end] if metadatas is not None else [{}] * len(chunk_ids) + ) + vectors = self._embed(chunk_docs) + conn.execute( + """ + INSERT INTO doc_embeddings + (project_id, id, document, metadata, embedding, updated_at) + SELECT %s, u.id, u.document, u.metadata::jsonb, u.embedding, now() + FROM unnest(%s::text[], %s::text[], %s::text[], %s::vector[]) + AS u(id, document, metadata, embedding) + ON CONFLICT (project_id, id) DO UPDATE + SET document = EXCLUDED.document, + metadata = EXCLUDED.metadata, + embedding = EXCLUDED.embedding, + updated_at = now() + """, + ( + project_id, + chunk_ids, + chunk_docs, + [json.dumps(m or {}) for m in chunk_meta], + [str(v) for v in vectors], + ), + ) + logger.debug( + "PgVectorStore: upserted %d documents for project %s in batches of %d", + len(doc_ids), + project_id, + step, + ) + + def query( + self, + project_id: str, + query_text: str, + n_results: int = 5, + where: dict | None = None, + ) -> list[dict]: + """Nearest neighbours by cosine distance, shaped exactly as ChromaDB shaped + them: ``{id, document, distance, metadata}``. + + ``where`` is ChromaDB's equality filter over metadata. Only the flat + ``{"key": value}`` form was ever used in this codebase; an operator form + (``$eq``, ``$in``) would silently match nothing here, so it raises instead. + """ + # Validate the filter BEFORE embedding. The embed is an ONNX forward pass; + # refusing an unsupported filter after paying for it wastes the work and puts + # the error a frame further from its cause. + filter_clauses: list[str] = [] + filter_params: list[Any] = [] + for key, value in (where or {}).items(): + if isinstance(value, dict): + raise ValueError( + f"PgVectorStore: operator filter {key}={value!r} is not supported; " + "only flat equality was ever used by this codebase" + ) + filter_clauses.append("metadata ->> %s = %s") + filter_params.extend([key, str(value)]) + + vector = self._embed([query_text])[0] + # psycopg binds %s positionally in statement order, and the query vector's + # placeholder is in the SELECT list — so it must be the FIRST parameter, not + # the first one the WHERE clause happens to need. Getting this backwards + # passes the project id where a vector belongs and fails at the cast rather + # than returning wrong rows, which is the one mercy in it. + params: list[Any] = [str(vector), project_id, *filter_params, n_results] + clauses = ["project_id = %s", *filter_clauses] + + sql = f""" + SELECT id, document, metadata, embedding <=> %s::vector AS distance + FROM doc_embeddings + WHERE {" AND ".join(clauses)} + ORDER BY distance + LIMIT %s + """ + with self._pool.connection() as conn: + rows = conn.execute(sql, params).fetchall() + + return [ + { + "id": r[0], + "document": r[1], + "distance": float(r[3]), + "metadata": r[2] or {}, + } + for r in rows + ] + + def delete_by_source_path(self, project_id: str, source_path: str) -> int: + """Remove every chunk of one file. Returns how many rows went.""" + with self._pool.connection() as conn: + cur = conn.execute( + "DELETE FROM doc_embeddings " + "WHERE project_id = %s AND metadata ->> 'source_path' = %s", + (project_id, source_path), + ) + return cur.rowcount or 0 + + def delete_collection(self, project_id: str) -> None: + with self._pool.connection() as conn: + conn.execute("DELETE FROM doc_embeddings WHERE project_id = %s", (project_id,)) + + def count(self, project_id: str) -> int: + """How many vectors a project holds. + + The ChromaDB backend exposes this through the collection object + (`collection.count()`), which `pipeline_runner` reads to decide whether the + store is empty and a full re-index is owed. That decision is the reason this + class exists, so the number it reads must come from here. + """ + with self._pool.connection() as conn: + row = conn.execute( + "SELECT count(*) FROM doc_embeddings WHERE project_id = %s", (project_id,) + ).fetchone() + return int(row[0]) if row else 0 + + def close(self) -> None: + try: + self._pool.close() + except Exception: # pragma: no cover - close is best-effort + logger.debug("PgVectorStore: pool close failed", exc_info=True) diff --git a/backend/app/knowledge/vector_store.py b/backend/app/knowledge/vector_store.py index b72ddd0c..e0f1ec80 100644 --- a/backend/app/knowledge/vector_store.py +++ b/backend/app/knowledge/vector_store.py @@ -2,6 +2,7 @@ import logging import threading from pathlib import Path +from typing import Any from urllib.parse import urlparse import chromadb @@ -300,3 +301,35 @@ def close(self) -> None: elif hasattr(self._client, "close"): self._client.close() logger.info("VectorStore closed") + + +def make_vector_store() -> "VectorStore | Any": + """Return the configured vector store. + + Both backends expose the same six methods and the same shapes, so nothing + downstream branches on which one it got. The switch exists because the change is + a storage move under a live product: `chroma` is what production has always run, + `pgvector` is what fixes it, and one flag lets the flip be a decision on a + verified deployment rather than a side effect of a deploy. + + `pgvector` needs Postgres. Development and the test suite run on SQLite, where + the migration that creates `doc_embeddings` is deliberately a no-op — so asking + for it there is a configuration error, and it says so instead of failing later on + a missing table. + """ + backend = (settings.vector_store_backend or "chroma").strip().lower() + if backend == "chroma": + return VectorStore() + if backend == "pgvector": + if settings.database_url.startswith("sqlite"): + raise ValueError( + "VECTOR_STORE_BACKEND=pgvector requires a PostgreSQL DATABASE_URL; " + "on SQLite the doc_embeddings migration is a no-op and the table " + "does not exist" + ) + from app.knowledge.pgvector_store import PgVectorStore + + return PgVectorStore() + raise ValueError( + f"VECTOR_STORE_BACKEND={backend!r} is not a backend; expected 'chroma' or 'pgvector'" + ) diff --git a/backend/app/main.py b/backend/app/main.py index 57e36757..27515e79 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -1471,9 +1471,9 @@ async def module_health( # Vector store (ChromaDB) try: - from app.knowledge.vector_store import VectorStore + from app.knowledge.vector_store import make_vector_store - vs = VectorStore() + vs = make_vector_store() vs._client.heartbeat() results["vector_store"] = {"status": "ok"} except Exception: diff --git a/backend/app/mcp_server/resources.py b/backend/app/mcp_server/resources.py index 31192076..8d6a1b87 100644 --- a/backend/app/mcp_server/resources.py +++ b/backend/app/mcp_server/resources.py @@ -135,9 +135,9 @@ async def get_project_knowledge(principal: Principal, project_id: str) -> str: except ResourceAccessDeniedError as exc: _denied(exc) try: - from app.knowledge.vector_store import VectorStore + from app.knowledge.vector_store import make_vector_store - vs = VectorStore() + vs = make_vector_store() collection = vs.get_or_create_collection(project_id) count = collection.count() return json.dumps( diff --git a/backend/app/models/__init__.py b/backend/app/models/__init__.py index e88ea94c..2000e7ef 100644 --- a/backend/app/models/__init__.py +++ b/backend/app/models/__init__.py @@ -23,6 +23,7 @@ from app.models.data_validation import DataInvestigation, DataValidationFeedback # noqa: F401 from app.models.db_index import DbIndex # noqa: F401 from app.models.deploy_state import DeployState # noqa: F401 +from app.models.doc_embedding import DocEmbedding # noqa: F401 from app.models.error_log import ErrorLog # noqa: F401 from app.models.indexing_checkpoint import IndexingCheckpoint # noqa: F401 from app.models.indexing_run import IndexingRun, IndexingRunEvent # noqa: F401 diff --git a/backend/app/models/doc_embedding.py b/backend/app/models/doc_embedding.py new file mode 100644 index 00000000..b05ce049 --- /dev/null +++ b/backend/app/models/doc_embedding.py @@ -0,0 +1,97 @@ +"""Vector embeddings, stored in Postgres rather than on a dyno's local disk. + +ChromaDB persisted to ``CHROMA_PERSIST_DIR`` — ``/app/data/chroma`` in production, +which is the container filesystem and is wiped on every dyno restart. Two things +followed, and the second is why this table exists rather than a bigger disk: + +* ``web`` and ``worker`` are separate Heroku process types with separate + filesystems, so a store the worker built was never visible to the chat path. +* An empty store makes ``pipeline_runner`` set ``force_full`` ("Vector store empty + but 758 docs in DB. Forcing a full re-index") — and a full rebuild of the one + real customer repository measures 12 039 s against the nightly job's 7 200 s + ceiling. Measured 2026-08-27: 16 of 94 repo-index runs ever completed. The + self-repair was correct; it simply cost more than the budget allowed, so the + store stayed empty and the loop repeated every night. + +Postgres has neither property: one store, shared by every dyno, surviving restarts. + +**Dimension is fixed at 384 on purpose.** It is what ChromaDB's bundled +``all-MiniLM-L6-v2`` produces, which is what production has always stored — +``CHROMA_EMBEDDING_MODEL`` names a 768-d model but ``sentence-transformers`` is not +installed, so it has never taken effect. Changing the embedder changes this column, +and ``embedding_fingerprint()`` already forces a re-index when it moves. +""" + +from __future__ import annotations + +from datetime import datetime + +from pgvector.sqlalchemy import Vector +from sqlalchemy import JSON, DateTime, ForeignKey, Index, String, Text, func, text +from sqlalchemy.dialects.postgresql import JSONB +from sqlalchemy.orm import Mapped, mapped_column + +from app.models.base import Base + +#: Dimensions produced by ChromaDB's bundled ONNX ``all-MiniLM-L6-v2``. Asserted at +#: write time — a mismatch is a silently wrong index, not a crash, so it is checked. +EMBEDDING_DIM = 384 + + +class DocEmbedding(Base): + """One embedded chunk. Keyed by ``(project_id, id)`` because ChromaDB ids are + unique within a collection — one collection per project — and never globally.""" + + __tablename__ = "doc_embeddings" + + project_id: Mapped[str] = mapped_column( + ForeignKey("projects.id", ondelete="CASCADE"), + primary_key=True, + ) + id: Mapped[str] = mapped_column(String(512), primary_key=True) + document: Mapped[str] = mapped_column(Text, nullable=False) + # Both columns carry a SQLite variant, and neither is decoration. Development + # and the whole test suite run on SQLite and call `create_all` over every model; + # `JSONB` refuses to compile there ("can't render element of type JSONB") and + # `vector` does not exist at all. The variants let the table be *declared* + # everywhere while only Postgres gets the types that mean anything — which + # matches the migration, a deliberate no-op on SQLite, and the factory, which + # refuses `pgvector` on a SQLite URL rather than failing later on a missing table. + doc_metadata: Mapped[dict] = mapped_column( + "metadata", + JSONB().with_variant(JSON(), "sqlite"), + nullable=False, + server_default="{}", + ) + embedding: Mapped[list[float]] = mapped_column( + Vector(EMBEDDING_DIM).with_variant(Text(), "sqlite"), nullable=False + ) + updated_at: Mapped[datetime] = mapped_column( + DateTime(timezone=True), nullable=False, server_default=func.now() + ) + + __table_args__ = ( + # `source_path` is the only metadata key ever queried on its own — it is how + # a changed file's chunks are removed before re-embedding + # (`delete_by_source_path`). An expression btree on that one key answers the + # equality directly; a GIN index over the whole `metadata` document would be + # larger, slower to maintain, and would still need a recheck. + Index( + "ix_doc_embeddings_source_path", + "project_id", + text("(metadata ->> 'source_path')"), + ), + # The similarity index. HNSW rather than IVFFlat: it needs no training pass, + # so it works from an empty table and does not degrade as rows arrive. + # Measured on this schema with 30 000 rows of 384 dimensions — build 20 s, + # and a top-5 filtered by `project_id` goes from 4 738 ms unindexed to + # 0.92 ms. `vector_cosine_ops` because the ChromaDB collections it replaces + # were created with `{"hnsw:space": "cosine"}`, so `<=>` returns the same + # distance the old store returned. + Index( + "ix_doc_embeddings_hnsw", + "embedding", + postgresql_using="hnsw", + postgresql_ops={"embedding": "vector_cosine_ops"}, + ), + ) diff --git a/backend/app/pipelines/mcp_pipeline.py b/backend/app/pipelines/mcp_pipeline.py index 36d3ed6c..f66e9290 100644 --- a/backend/app/pipelines/mcp_pipeline.py +++ b/backend/app/pipelines/mcp_pipeline.py @@ -89,9 +89,9 @@ async def _connect_with_retry(): tool_docs.append(doc) try: - from app.knowledge.vector_store import VectorStore + from app.knowledge.vector_store import make_vector_store - vs = VectorStore() + vs = make_vector_store() collection = vs.get_or_create_collection(context.project_id) ids = [f"mcp-tool-{source_id}-{s['name']}" for s in schemas] metadatas = [ @@ -131,7 +131,7 @@ async def sync_with_code( async def get_status(self, source_id: str) -> PipelineStatus: """Check if MCP tool schemas have been indexed.""" try: - from app.knowledge.vector_store import VectorStore + from app.knowledge.vector_store import make_vector_store from app.models.base import async_session_factory from app.services.connection_service import ConnectionService @@ -141,7 +141,7 @@ async def get_status(self, source_id: str) -> PipelineStatus: if not conn or conn.source_type != "mcp": return PipelineStatus() - vs = VectorStore() + vs = make_vector_store() collection = vs.get_or_create_collection(conn.project_id) results = collection.get( diff --git a/backend/app/services/embedding_reindex.py b/backend/app/services/embedding_reindex.py index 5cc5bbf5..18a33b28 100644 --- a/backend/app/services/embedding_reindex.py +++ b/backend/app/services/embedding_reindex.py @@ -20,7 +20,7 @@ import logging from app.core.task_queue import enqueue -from app.knowledge.vector_store import VectorStore +from app.knowledge.vector_store import make_vector_store logger = logging.getLogger(__name__) @@ -56,7 +56,7 @@ async def queue_embedding_reindex(project_ids: list[str]) -> list[str | None]: len(project_ids), ) - vs = VectorStore() + vs = make_vector_store() results: list[str | None] = [] for pid in project_ids: diff --git a/backend/app/services/indexing_artifacts.py b/backend/app/services/indexing_artifacts.py index f1213969..7baa9c0a 100644 --- a/backend/app/services/indexing_artifacts.py +++ b/backend/app/services/indexing_artifacts.py @@ -109,9 +109,9 @@ def cleanup_project_artifacts(project_id: str) -> None: # 2. Chroma collection (best-effort — VectorStore is a singleton and we # don't want to pin its lifecycle to this delete path). try: - from app.knowledge.vector_store import VectorStore + from app.knowledge.vector_store import make_vector_store - VectorStore().delete_collection(project_id) + make_vector_store().delete_collection(project_id) except Exception: logger.debug( "indexing_artifacts: Chroma cleanup failed for project %s", diff --git a/backend/pyproject.toml b/backend/pyproject.toml index 8440c0b0..080b2ea9 100644 --- a/backend/pyproject.toml +++ b/backend/pyproject.toml @@ -15,6 +15,14 @@ dependencies = [ "alembic>=1.14.0", "asyncssh>=2.18.0", "asyncpg>=0.30.0", + # F-KNOW-13: the vector store moved off ChromaDB's on-disk collections and + # into Postgres. psycopg sits beside asyncpg deliberately — `VectorStore` is + # called synchronously from ten sites including the agent hot path + # (`context_loader.py:213,421`, `knowledge_catalog_service.py:480,539`), and + # converting those to async is a larger, riskier change than carrying a second + # driver. Revisit if those call sites ever become async for another reason. + "psycopg[binary,pool]>=3.2.0", + "pgvector>=0.3.6", "aiomysql>=0.2.0", "motor>=3.6.0", "clickhouse-connect>=0.8.0", diff --git a/backend/tests/integration/test_embedding_reconcile_startup.py b/backend/tests/integration/test_embedding_reconcile_startup.py index 7656dcbe..20d333fb 100644 --- a/backend/tests/integration/test_embedding_reconcile_startup.py +++ b/backend/tests/integration/test_embedding_reconcile_startup.py @@ -37,7 +37,7 @@ async def test_reconcile_drives_real_reindex_path(session_factory, monkeypatch): # Real queue_embedding_reindex runs; only its external deps are stubbed. enqueue_spy = AsyncMock(return_value="job-1") monkeypatch.setattr(reindex_mod, "enqueue", enqueue_spy) - monkeypatch.setattr(reindex_mod, "VectorStore", _DummyVectorStore) + monkeypatch.setattr(reindex_mod, "make_vector_store", _DummyVectorStore) async with session_factory() as s: s.add(DeployState(key="embedding_fingerprint", value="all-MiniLM-L6-v2|256")) s.add(Project(name="p0")) diff --git a/backend/tests/unit/docs/test_suppression_debt_ratchet.py b/backend/tests/unit/docs/test_suppression_debt_ratchet.py index 10b31178..b9c44732 100644 --- a/backend/tests/unit/docs/test_suppression_debt_ratchet.py +++ b/backend/tests/unit/docs/test_suppression_debt_ratchet.py @@ -52,10 +52,15 @@ # this red. That friction is the point — every one of those raises is a sentence # somebody had to write — but a burst of five parallel PRs makes it feel like noise # rather than a decision, and it should be read as the latter. - "except Exception": 613, + # 2026-08-28, both raised by one: `PgVectorStore.close()` swallows a pool-close + # failure (the store is being torn down; raising there would mask whatever was + # actually being shut down), and `app/models/__init__.py` gained + # `DocEmbedding # noqa: F401` — the re-export convention every one of the other + # forty imports in that file already follows, not new debt. + "except Exception": 614, "except ...: pass": 53, "# type: ignore": 49, - "# noqa": 128, + "# noqa": 129, } PATTERNS: dict[str, re.Pattern[str]] = { diff --git a/backend/tests/unit/knowledge/test_embedding_reindex.py b/backend/tests/unit/knowledge/test_embedding_reindex.py index 7f82131c..53119eb4 100644 --- a/backend/tests/unit/knowledge/test_embedding_reindex.py +++ b/backend/tests/unit/knowledge/test_embedding_reindex.py @@ -158,7 +158,7 @@ async def test_enqueues_run_repo_index_for_each_project(self, project_ids: list[ mock_enqueue = AsyncMock(return_value="job-123") with ( - patch("app.services.embedding_reindex.VectorStore", return_value=mock_vs), + patch("app.services.embedding_reindex.make_vector_store", return_value=mock_vs), patch("app.services.embedding_reindex.enqueue", mock_enqueue), ): results = await queue_embedding_reindex(project_ids) @@ -187,7 +187,7 @@ async def test_empty_list_is_noop(self) -> None: mock_enqueue = AsyncMock() with ( - patch("app.services.embedding_reindex.VectorStore", return_value=mock_vs), + patch("app.services.embedding_reindex.make_vector_store", return_value=mock_vs), patch("app.services.embedding_reindex.enqueue", mock_enqueue), ): results = await queue_embedding_reindex([]) @@ -210,7 +210,7 @@ async def test_delete_failure_does_not_abort_remaining(self, project_ids: list[s mock_enqueue = AsyncMock(return_value="job-id") with ( - patch("app.services.embedding_reindex.VectorStore", return_value=mock_vs), + patch("app.services.embedding_reindex.make_vector_store", return_value=mock_vs), patch("app.services.embedding_reindex.enqueue", mock_enqueue), ): results = await queue_embedding_reindex(project_ids) @@ -231,7 +231,7 @@ async def _fake_enqueue(task_name: str, *, project_id: str, force_full: bool) -> return f"job-{job_counter[0]}" with ( - patch("app.services.embedding_reindex.VectorStore", return_value=mock_vs), + patch("app.services.embedding_reindex.make_vector_store", return_value=mock_vs), patch("app.services.embedding_reindex.enqueue", side_effect=_fake_enqueue), ): results = await queue_embedding_reindex(project_ids) diff --git a/backend/tests/unit/knowledge/test_pgvector_store.py b/backend/tests/unit/knowledge/test_pgvector_store.py new file mode 100644 index 00000000..9aa7259a --- /dev/null +++ b/backend/tests/unit/knowledge/test_pgvector_store.py @@ -0,0 +1,167 @@ +"""The vector store must survive a dyno restart, and the swap must not change answers. + +ChromaDB persisted to ``CHROMA_PERSIST_DIR`` — ``/app/data/chroma``, the container +filesystem, wiped on every restart and not shared between the ``web`` and ``worker`` +process types. What that cost, measured in production on 2026-08-27: + + 22:00:44 repair_embeddings "Vector store empty but 758 docs in DB. + Forcing a full re-index." + 22:01:21 code_symbol_embed started (all 8 646 files, because force_full) + 22:39:09 code_symbol_embed completed (38 minutes) + 23:59:50 heartbeat stops — the 7 200 s ceiling, at 380 of 758 documents + 00:04 stale run reaped + +A full rebuild costs 12 039 s and the nightly budget is 7 200 s, so the store was +empty again the next night, and the night after. `index_repo` has completed 16 times +in 94 runs. The self-repair (C3, v1.13.0) was correct — the repair simply cost more +than the budget allowed, which is not something a bigger ceiling can fix. + +These tests cover what can be checked without a database. The store's behaviour +against real Postgres — ranking, filters, upsert, delete — was verified end to end +against the production database before this file existed; what is asserted here is +the part that would rot silently: the interface staying identical to the backend it +replaces, and the two refusals that must stay loud. +""" + +from __future__ import annotations + +import inspect + +import pytest + +from app.knowledge import pgvector_store as pgv +from app.knowledge.vector_store import VectorStore, make_vector_store + + +class TestTheInterfaceIsIdenticalToTheBackendItReplaces: + """Ten call sites take whichever store they are handed and never ask which one. + The moment one backend grows a method the other lacks, that stops being true — + and it stops being true at runtime, on whichever deployment flipped the flag.""" + + PUBLIC = ( + "get_or_create_collection", + "add_documents", + "query", + "delete_by_source_path", + "delete_collection", + "close", + ) + + @pytest.mark.parametrize("name", PUBLIC) + def test_both_backends_have_it(self, name: str) -> None: + assert callable(getattr(VectorStore, name, None)), f"VectorStore.{name}" + assert callable(getattr(pgv.PgVectorStore, name, None)), f"PgVectorStore.{name}" + + @pytest.mark.parametrize("name", PUBLIC) + def test_the_signatures_match(self, name: str) -> None: + """Positional parameters and their order, not annotations — a caller passes + `(project_id, query_text, n_results)` and must keep meaning the same thing.""" + a = list(inspect.signature(getattr(VectorStore, name)).parameters) + b = list(inspect.signature(getattr(pgv.PgVectorStore, name)).parameters) + assert a == b, f"{name}: chroma={a} pgvector={b}" + + def test_the_collection_handle_answers_count(self) -> None: + """`pipeline_runner.py:415` and `context_loader.py:213` both do + `get_or_create_collection(pid).count()`. That call decides whether the store + is empty and a full re-index is owed — the very decision this change exists + to make correct — so the handle must answer it.""" + assert hasattr(pgv._ProjectHandle, "count") + assert "name" in pgv._ProjectHandle.__slots__ + + def test_the_handle_names_the_collection_chroma_would_have(self) -> None: + """Logs and metrics carry that name. Changing it silently breaks a grep.""" + store = object.__new__(pgv.PgVectorStore) + handle = pgv._ProjectHandle(store, "fc6554a5-55bd-4490-ac0e-4e6a45afee34") + assert handle.name == "project_fc6554a5_55bd_4490_ac0e_4e6a45afee34" + assert handle.name == VectorStore._collection_name( + object.__new__(VectorStore), "fc6554a5-55bd-4490-ac0e-4e6a45afee34" + ) + + +class TestTheDsnConversion: + """SQLAlchemy's URL names a driver; libpq does not understand one.""" + + @pytest.mark.parametrize( + ("given", "want"), + [ + ("postgresql+asyncpg://u:p@h:5432/db", "postgresql://u:p@h:5432/db"), + ("postgresql+psycopg://u:p@h/db", "postgresql://u:p@h/db"), + ("postgresql://u:p@h/db", "postgresql://u:p@h/db"), + ], + ) + def test_the_driver_is_stripped(self, given: str, want: str) -> None: + assert pgv._sync_dsn(given) == want + + def test_a_password_containing_the_driver_name_is_not_mangled(self) -> None: + """Belt and braces: the replacement is anchored on the scheme, and a password + is not a scheme. Worth a test because the failure would be an intermittent + auth error nobody would trace back to here.""" + dsn = pgv._sync_dsn("postgresql+asyncpg://u:pass@h/db") + assert dsn.startswith("postgresql://") + assert "pass" in dsn + + +class TestTheTwoRefusals: + """Both exist because the alternative is a wrong answer nobody can see.""" + + def test_an_operator_filter_is_refused_rather_than_matching_nothing(self) -> None: + """ChromaDB accepts `{"k": {"$eq": v}}`. Translated naively to + `metadata ->> 'k' = '{...}'` it matches no row and returns an empty result — + which reads as "nothing found" rather than "this filter is not implemented".""" + store = object.__new__(pgv.PgVectorStore) + with pytest.raises(ValueError, match="operator filter"): + store.query("p", "q", where={"chunk": {"$eq": 1}}) + + def test_a_dimension_mismatch_raises(self) -> None: + """A vector of the wrong width is not a crash — Postgres would reject it, but + only if the column is declared. The check is here so the message names the + cause (the embedder changed) rather than a cast error.""" + store = object.__new__(pgv.PgVectorStore) + store._embedding_fn = lambda texts: [[0.0] * 7 for _ in texts] + import threading + + store._embed_lock = threading.Lock() + with pytest.raises(pgv.EmbeddingDimensionError, match="7 dimensions"): + store._embed(["x"]) + + +class TestTheBackendSwitch: + def test_the_default_is_still_chroma(self) -> None: + """The flip is a decision taken on a verified deployment, not a side effect of + merging this change.""" + from app.config import Settings + + assert Settings.model_fields["vector_store_backend"].default == "chroma" + + def test_the_factory_returns_chroma_by_default(self) -> None: + assert isinstance(make_vector_store(), VectorStore) + + def test_pgvector_on_sqlite_says_so_instead_of_failing_later(self, monkeypatch) -> None: + """Development and the test suite run on SQLite, where the migration that + creates `doc_embeddings` is deliberately a no-op. Asking for pgvector there is + a configuration error, and it should read as one — not as a missing table + several stack frames deep.""" + from app.config import settings + + monkeypatch.setattr(settings, "vector_store_backend", "pgvector", raising=False) + monkeypatch.setattr(settings, "database_url", "sqlite+aiosqlite:///x.db", raising=False) + with pytest.raises(ValueError, match="requires a PostgreSQL"): + make_vector_store() + + def test_an_unknown_backend_is_refused(self, monkeypatch) -> None: + from app.config import settings + + monkeypatch.setattr(settings, "vector_store_backend", "qdrant", raising=False) + with pytest.raises(ValueError, match="not a backend"): + make_vector_store() + + +def test_the_batch_cap_still_governs_the_embed_call() -> None: + """`EMBEDDING_UPSERT_BATCH_SIZE` was cut 200 -> 8 because the ONNX model pads every + document to 256 tokens and its activations are sized by the batch: 967 MiB for 200 + against 415 MiB for 8, on a worker that had been SIGKILLed at 1 053 MiB. The + constraint belongs to the embedder, which moved into this class — so the cap has + to move with it rather than being left behind in the backend that no longer runs. + """ + source = inspect.getsource(pgv.PgVectorStore.add_documents) + assert "embedding_upsert_batch_size" in source diff --git a/backend/tests/unit/test_alembic_revisions_unique.py b/backend/tests/unit/test_alembic_revisions_unique.py new file mode 100644 index 00000000..27c75a8a --- /dev/null +++ b/backend/tests/unit/test_alembic_revisions_unique.py @@ -0,0 +1,110 @@ +"""Two migrations must not claim the same revision id. + +Alembic keys its graph on `revision`, so a duplicate does not collide loudly — it +makes the graph ambiguous, and what surfaces is a cycle several commands later: + + alembic.script.revision.CycleDetected: Cycle is detected in revisions + (6287a47828ca, b1c2d3e4f5a6, c2d3e4f5a6b7, d3e4f5a6b7c8) + +Hit on 2026-08-28 while adding `doc_embeddings`: `b1c2d3e4f5a6` was already taken by +`b1c2d3e4f5a6_batch_started_at_claim.py`. The message names four revisions and not +the duplicate, and it appears only when something walks the graph — so a migration +authored with a taken id can be committed, reviewed and merged before anything +notices. This project's ids are hand-picked hex strings rather than the ones +`alembic revision` generates, which is what puts the collision within reach. + +Read with `ast` rather than a regex, and that is not fastidiousness. The first +version of this file used a regex for `down_revision` and reported **seven** heads +where alembic reports one, because this repository has merge migrations whose parent +is a tuple — ``down_revision = ("a2b3c4d5e6f7", "g4h5i6j7k8l9")``. A test that +misreads the graph is worse than no test: it fails on a healthy repository, and the +lesson taken is to delete the test. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +VERSIONS = Path(__file__).resolve().parents[2] / "alembic" / "versions" + + +def _assignments(path: Path) -> dict[str, object]: + """Read module-level `revision` / `down_revision` literals, annotated or not.""" + tree = ast.parse(path.read_text(encoding="utf-8")) + out: dict[str, object] = {} + for node in tree.body: + if isinstance(node, ast.Assign): + names = [t.id for t in node.targets if isinstance(t, ast.Name)] + value = node.value + elif isinstance(node, ast.AnnAssign) and isinstance(node.target, ast.Name): + names, value = [node.target.id], node.value + else: + continue + if value is None: + continue + for name in names: + if name in ("revision", "down_revision"): + try: + out[name] = ast.literal_eval(value) + except ValueError: # a computed value — not something to guess at + pass + return out + + +def _graph() -> tuple[dict[str, list[str]], dict[str, set[str]]]: + """Return ``({revision: [files]}, {revision: {parents}})``.""" + revisions: dict[str, list[str]] = {} + parents: dict[str, set[str]] = {} + for path in sorted(VERSIONS.glob("*.py")): + found = _assignments(path) + rev = found.get("revision") + if not isinstance(rev, str): + continue + revisions.setdefault(rev, []).append(path.name) + down = found.get("down_revision") + if isinstance(down, str): + parents[rev] = {down} + elif isinstance(down, (tuple, list)): + parents[rev] = {d for d in down if isinstance(d, str)} + else: + parents[rev] = set() + return revisions, parents + + +def test_every_revision_id_is_claimed_once() -> None: + revisions, _ = _graph() + duplicates = {rev: files for rev, files in revisions.items() if len(files) > 1} + assert not duplicates, f"revision ids claimed by more than one migration: {duplicates}" + + +def test_there_is_exactly_one_head() -> None: + """A head is a revision nothing names as a parent. Two of them is a branch someone + has to merge, and `alembic upgrade head` refuses until they do — at release time, + on the dyno, before uvicorn starts.""" + revisions, parents = _graph() + named_as_parent = {p for ps in parents.values() for p in ps} + heads = sorted(set(revisions) - named_as_parent) + assert len(heads) == 1, f"expected one head, found {heads}" + + +def test_every_parent_exists() -> None: + """A dangling parent is the third way this graph breaks, and it reads as a missing + file rather than as the typo in an id that it usually is.""" + revisions, parents = _graph() + dangling = { + rev: sorted(p for p in ps if p not in revisions) for rev, ps in parents.items() if ps + } + dangling = {rev: missing for rev, missing in dangling.items() if missing} + assert not dangling, f"down_revision names a revision that does not exist: {dangling}" + + +def test_the_reader_sees_the_merge_migrations() -> None: + """The guard on the guard. If `_graph` ever stops understanding a tuple parent, the + head count silently inflates and this file starts failing on a healthy repository — + which is how a correct test gets deleted. Assert that at least one merge migration + is parsed with more than one parent, so that regression fails *here*, naming itself. + """ + _, parents = _graph() + merges = {rev: ps for rev, ps in parents.items() if len(ps) > 1} + assert merges, "no multi-parent migration parsed — the tuple form is being missed" diff --git a/backend/tests/unit/test_mcp_pipeline.py b/backend/tests/unit/test_mcp_pipeline.py index 7f0d24ad..8214fd1d 100644 --- a/backend/tests/unit/test_mcp_pipeline.py +++ b/backend/tests/unit/test_mcp_pipeline.py @@ -76,6 +76,10 @@ async def test_index_connects_and_stores_schemas(self, pipeline, ctx): ), "app.knowledge.vector_store": _mock_module( VectorStore=MagicMock(return_value=mock_vs), + # The production code constructs through the factory since the + # backend became selectable; a stub module that offers only the class + # leaves the import unresolved. + make_vector_store=MagicMock(return_value=mock_vs), ), } @@ -182,6 +186,10 @@ async def test_get_status_with_docs(self, pipeline): ), "app.knowledge.vector_store": _mock_module( VectorStore=MagicMock(return_value=mock_vs), + # The production code constructs through the factory since the + # backend became selectable; a stub module that offers only the class + # leaves the import unresolved. + make_vector_store=MagicMock(return_value=mock_vs), ), } @@ -212,6 +220,10 @@ async def test_get_status_no_docs(self, pipeline): ), "app.knowledge.vector_store": _mock_module( VectorStore=MagicMock(return_value=mock_vs), + # The production code constructs through the factory since the + # backend became selectable; a stub module that offers only the class + # leaves the import unresolved. + make_vector_store=MagicMock(return_value=mock_vs), ), } diff --git a/docs/qa-audit/issues.md b/docs/qa-audit/issues.md index 172ef1bd..bb924770 100644 --- a/docs/qa-audit/issues.md +++ b/docs/qa-audit/issues.md @@ -92,12 +92,12 @@ frontend **A** (563 smells, 7 SOLID). | Severity | Open | |---|---| | 🔴 Critical | 0 | -| 🟠 High | **0** | +| 🟠 High | **1** | | 🟡 Medium | 3 | | 🟢 Low | 26 | | ⚪ Info | 12 | -*Counted 2026-08-27, not estimated: **33 open `F-` rows and 76 struck** by `grep -cE '^\| F-'` / `grep -cE '^\| ~~F-'` over this file, plus **8 open `CB-` rows** those two commands do not see. The severity table above counts all 41 open rows of both kinds, which is why it does not match the `F-` figure — the two measure different sets and each says which. Both are derived from the rows themselves.* +*Counted 2026-08-28, not estimated: **33 open `F-` rows and 76 struck** by `grep -cE '^\| F-'` / `grep -cE '^\| ~~F-'` over this file, plus **9 open `CB-` rows** those two commands do not see. The severity table above counts all 42 open rows of both kinds, which is why it does not match the `F-` figure — the two measure different sets and each says which. Both are derived from the rows themselves.* *(R1+R2 closed 4 High + 8 Medium + 3 Low. R3 (`fbf8112`) closed 2 High (F-SSH-08, F-RULE-01) + 5 Medium (F-RULE-05, F-DG-07/09, F-GRAPH-01, F-LEARN-07) + 1 Low (F-SSH-06). The 2026-07-19 UX @@ -401,6 +401,7 @@ maintainability / reliability risks. | ~~CB-SEN1~~ | ✅ | ~~**Sentry was reachable by two secrets neither scrubbing layer could see**~~ — the built-in `EventScrubber` matches key names and its 33-key default carries neither `dsn` nor `database_url`; `before_send` matched values but walked only `exception.values`, `logentry` and `breadcrumbs`. A key of either name in `extra` or `contexts` was caught by **neither**. Urgent rather than theoretical from the moment `SENTRY_DSN` was set in production. **Fixed 2026-08-26** (#230): layer 1 wired with the denylist extended 33 → 39, layer 2 walks `extra` and `contexts` recursively and depth-bounded, host preserved. Fifteen tests, one of them an assertion about *Sentry* — that layer 1 alone still leaks values — so the redundancy question re-opens from a red test rather than from memory. | | ~~CB-SEN2~~ | ✅ | ~~**The Sentry release would have been blank on the container stack**~~ — `HEROKU_SLUG_COMMIT`, the value every guide names, is populated only for slug (buildpack) deploys. This app is on the **container** stack, where the variable exists and is always **empty** (measured on v271 *after* `runtime-dyno-metadata` was enabled). Issues would attach to a release with no commits and suspect-commit attribution would silently do nothing. Enabling the labs feature was necessary and not sufficient, and nothing would have said so. **Fixed 2026-08-26** (#231): the commit is baked into the image via `--build-arg GIT_SHA` → `ENV RELEASE`; verified in production, `RELEASE == main` HEAD. The empty string is the trap — `os.getenv` returns `""` there, not `None`, so an `is None` check would have accepted it; a test catches that form. | | CB-UX1 | ⚪ | **102 UX scenarios carry a verification older than 30 days.** 110 of 127 were dated 2026-07-19 while 152 commits had landed since; five were re-audited 2026-08-26 and the ceiling now stands at 105, of which 102 still have a changed Coverage file under them. Ordered and computable: `python3 scripts/ux_verification_status.py --backlog 2026-07-19`. The ceiling in `tests/unit/docs/test_ux_scenarios.py` may fall but not rise. | Re-audit in batches, worst first; date each verdict and add an `SCN-NNN` anchor so a machine can check it (21 of 127 have one). | +| CB-KNOW2 | 🟠 | **The vector store lived on a disk that every dyno restart wipes, and that is why the repo index almost never completes.** `CHROMA_PERSIST_DIR` is `/app/data/chroma` — the Heroku container filesystem — and `web`/`worker` are separate process types with separate copies. Measured 2026-08-27, five hours after a deploy: `repair_embeddings` fired ("Vector store empty but 758 docs in DB"), the pipeline set `force_full`, `code_symbol_embed` ran 38 min over all 8 646 files, and the run was reaped at the 7 200 s ceiling on document 380 of 758 — so the store was empty again the next night. **`index_repo` 16 completions in 94 runs; `daily_sync` 13 in 91.** The C3 self-repair was correct; it cost more than the budget allowed. **Fix written, not yet enabled:** `PgVectorStore` on branch `feat/pgvector-store`, verified end to end against the production database (same four documents, same ranking, distances 0.2234 / 0.7867 / 1.0161), with `VECTOR_STORE_BACKEND` still defaulting to `chroma`. | Deploy, flip `VECTOR_STORE_BACKEND=pgvector`, run one full re-index and require `pipeline_end`; then watch a deploy happen and the next nightly run stay incremental. Strike on that, not on the merge. | | CB-OPS1 | 🟡 | **Measured on a rebuild that finished, 2026-08-27.** Peak **1 246 MB (112.7 %)**, **45 × R14**, **0 × R15** across the run that reached `pipeline_end` at 15:51:39 on Standard-2X. Every R14 falls in `generate_docs`, not `graph_build`. This row was struck earlier on "zero R14/R15" — but every run behind that claim died in or before `code_symbol_embed` and never reached the late steps where memory peaks, so it measured a truncated rebuild. What the resize genuinely bought, stated precisely: before it, 170 × R14 **and 2 × R15** with a 1 143 MiB peak against a 512 MiB quota — the process was killed. Now it runs 12 % over quota for hours and survives. | Decide from the number: accept a permanently over-quota worker, take the next size up, or make `generate_docs` hold less state. Not urgent — no `R15` in 3.34 h — but it is over quota by design now, and that should be a choice. | | CB-OPS2 | 🟡 | **The nightly cron can rebuild a repository the "Re-index repository" button never can.** `run_repo_index_task` is called by two ARQ jobs carrying two ceilings: the cron's `run_daily_project_knowledge_sync` at 7200 s, and `run_repo_index` at 1800 s. Measured on the same repository from `indexing_runs`: nightly `completed` in **42.4 min** (08-25 22:00), manual `TimeoutError` at **exactly 1800.02 s** inside `_run_code_symbol_embed` (08-27 09:30). Diagnosed once already — AUD-0819-20 added the knob on 2026-08-19 for this failure and left the default at the value just measured as too small. **Fix written, not yet in production:** `repo_index_job_timeout_seconds` defaults to 3600 on branch `fix/repo-index-ceiling`, with both orderings asserted in `tests/unit/services/test_repo_index_ceiling.py`. | Deploy, then force one manual full re-index and require it to reach `pipeline_end`; strike this row on that evidence, not on the merge. | | CB-OPS3 | 🟡 | **A worker restart during a repo index loses the run, and nothing retries it.** Measured 2026-08-27: release `v279` restarted the worker at 10:59:34 UTC, 37 min into an index. arq logged `shutdown on SIGTERM ◆ 0 jobs complete ◆ 2 failed ◆ 0 retries ◆ 1 ongoing to cancel`, exited 143, and the fresh worker started at 10:59:45 with **no job re-queued** — no `run_repo_index` or `run_daily_project_knowledge_sync` start appears in the next 30 min of worker log. `WorkerSettings` sets neither `retry_jobs` nor `max_tries`, so arq's defaults were in force and still did not retry. The reaper correctly flipped both rows to `failed / stale run reaped`, visible in `error_log` since N3. The restart was a deploy of our own, not an incident — the finding is that a routine deploy costs a whole index. | Decide the semantics before coding: re-enqueue on shutdown (risking a double run against `_indexing_locks` and the advisory locks), or leave it to the cron and make the loss explicit in the UI. Not a silent implementation choice. | diff --git a/docs/reports/status-2026-08-28.html b/docs/reports/status-2026-08-28.html new file mode 100644 index 00000000..ad55f94b --- /dev/null +++ b/docs/reports/status-2026-08-28.html @@ -0,0 +1,232 @@ + +
+ +checkmydata-api v283 · каждое число получено запросом, а не оценкойПродукт живой, задеплоен и отвечает. Но три структурных дефекта делают ровно то, +за что клиент платит, ненадёжным: знание о репозитории не переживает перезапуск дино, +расход токенов записывается нулём, и валидатор этапа убивает работающие запросы. +Ни один не виден в коде — все три нашлись только на проде.
+ +Это работающий ранний продукт с одним реальным клиентом, а не прототип и не зрелый сервис. +Инженерная база сильная — 6962 теста, 82% покрытия, чистый дрейф конфигурации, CI на каждый PR. +Продуктовая часть за этой базой проверена мало: масштаб прода — 9 пользователей, 3 проекта, +3 подключения, 131 сообщение в чате за всё время.
+ +| Что | Сколько | Читается как |
|---|---|---|
| Пользователи / проекты / подключения | 9 / 3 / 3 | один настоящий клиент + два демо |
| Сессии чата / сообщения | 25 / 131 | продукт трогали, но не эксплуатировали |
| Трассы запросов / спаны | 222 / 9 844 | наблюдаемость работает и пишет подробно |
| Подписки Stripe | 0 | биллинг включён, денег через него не прошло |
| Символы кода / рёбра графа | 25 496 / 2 134 | 0.08 ребра на символ — граф почти без связей |
| Документы знания | 758 | полный набор по репозиторию клиента |
| Выученные уроки / инсайты | 81 / 134 | память агента наполняется |
| Релизов за неделю | 13 | темп высокий |
ChromaDB пишет в CHROMA_PERSIST_DIR=/app/data/chroma — файловую систему контейнера Heroku,
+которая стирается при каждом рестарте. CHROMA_SERVER_URL пуст, общего хранилища нет.
+Вдобавок web и worker — разные дино с разными дисками, то есть даже успешно
+построенное хранилище на воркере недоступно чату на вебе.
Из этого получается замкнутая петля, из которой система не выходит сама:
+рестарт дино → /app/data/chroma пусто
+ → пайплайн видит col_count == 0 и ставит force_full = True
+ ("Vector store empty but 758 docs in DB. Forcing a full re-index")
+ → полный ребилд стоит 12 039 с (3.34 ч)
+ → бюджет ночной задачи 7 200 с → обрыв на 120-й минуте
+ → хранилище снова пусто → повтор следующей ночью
+Замерено: событие repair_embeddings 08-27 22:00:44; code_symbol_embed
+22:01:21→22:39:09 (38 мин на всех 8 646 файлов); прогон реапнут на 124.9 мин.
+За всю историю: index_repo 16 успехов из 94, daily_sync 13 из 91.
Механизм самолечения repair_embeddings (C3, v1.13.0) написан правильно — но лечение
+дороже бюджета, поэтому он не лечит, а гарантирует ежедневный отказ. Это тот же класс дефекта,
+что уже чинили для BM25 (F-KNOW-12, восстановление из Postgres при старте): у ChromaDB такой
+починки нет, и восстановление у него на порядок дороже.
Все три адаптера LLM кладут в usage только prompt_tokens и
+completion_tokens, но не total_tokens
+(openrouter_adapter.py:213, openai_adapter.py:209,
+anthropic_adapter.py:226). Роутер читает его как
+usage.get("total_tokens", 0) — получает 0, а не None, поэтому запасной путь
+if total_tokens is None: total_tokens = prompt + completion
+(usage_service.py:40) не срабатывает никогда.
sum(prompt_tokens) 53 089 902 +sum(completion_tokens) 4 315 660 +sum(total_tokens) 0 ← колонка, по которой считается бюджет +строк в token_usage 6 479+
check_token_budget суммирует именно TokenUsage.total_tokens
+(usage_service.py:78, :84). Сумма всегда ноль, значит:
+ни дневной, ни месячный лимит, ни планная квота не могут сработать в принципе —
+включая пост-вызовный гейт, который должен останавливать разогнавшийся агент.
+При этом BILLING_ENABLED=True в проде.
Сырые числа записаны (6 464 строки из 6 479 имеют ненулевой prompt_tokens),
+поэтому историю можно пересчитать, а исправление — одна строка на адаптер либо одна в роутере.
+estimated_cost_usd пуст везде: usage_sink.py:137 передаёт None.
Все семь отказов stage_validation в истории — одна причина: «Missing expected columns».
+Планировщик заранее объявляет имена колонок, SQL-агент пишет запрос, который отвечает на вопрос,
+и валидатор его отклоняет за несовпадение с догадкой плана.
stage_result success SELECT `user_id`, COUNT(*) AS cnt FROM `purchases` WHERE … +stage_validation failed Missing expected columns: ['id']+
Другие: ['cohort_size'], ['data_revenue'],
+['activity_month','amount','user_id'],
+['purchase_count','purchase_date','revenue_usd',…].
+Цена одного такого отказа — до 10 LLM-вызовов и 4–5 минут ожидания
+(замерено: 269 с, 296 с, 358 с на реальных вопросах).
Это крупнейшая категория отказов спанов: 7 из 17. Следом
+orchestrator:llm_call — 5, orchestrator:viz — 3.
Приборы есть, но три из них показывают неправду — а это хуже отсутствующего прибора, +потому что по ним принимают решения.
+| Что заявлено | Что измерено |
|---|---|
CLAUDE.md: «ORCH-A03: complexity больше не unknown, метрики маршрутизации всегда заполнены» |
+ route='unknown' и complexity='unknown' на 222 трассах из 222, включая вчерашние 16:20 |
Колонка failure_kind — классификация отказов (веха W0) |
+ пуста на всех 222, включая 56 отказавших |
| Учёт токенов на запрос | +total_tokens=0 на всех трассах кроме одной |
| Связывание кода с БД (M5, lineage) | +graph_db_bridge: «Attached 0 caller refs across 179 entities» |
| Область | Статус | Доказательство |
|---|---|---|
| Чат, SSE, сессии, трассировка | работает | 222 трассы, 9 844 спана, подробные шаги |
| SQL-агент: простые вопросы | работает | 62 ответа sql_result, 93 text |
| SQL-агент: сложная аналитика | ненадёжно | таймауты на «динамика выручки по когортам», 25% отказов |
| Индексация репозитория | сломано | 17% успеха; петля с пустым векторным хранилищем |
| Индексация схемы БД | работает | 41 успех из 48 (85%) |
| Карта код↔БД | починена вчера | 137 matched / 96 db_only / 23 code_only / 0 mismatch |
| Граф кода (M2) | частично | 25 496 символов, но 2 134 ребра и 0 caller refs |
| Учёт расхода и бюджеты | не работает | 57.4 млн токенов, посчитан 0 |
| Биллинг Stripe | включён, не проверен | 0 подписок, 0 событий; ключи не заданы |
| GA4 как источник данных (1.16.0) | не запускался | 0 строк фактов, 0 импортов, 0 учёток вендора |
| Дашборды / расписания / расследования | пусто | 1 дашборд, 0 расписаний, 0 расследований |
| Правила / заметки / память агента | используется | 18 правил, 81 урок, 134 инсайта |
| Публичные поверхности | живы | лендинг, API, /pricing — все 200 |
make config-drift чист: пять расхождений прода с кодом записаны как решения с причинами.
+За сутки прошли шесть PR, все с зелёным CI. Но main не защищён — обязательных
+проверок нет, всё уехало в центральный шов на честном слове.
1. Вынести векторное хранилище с эфемерного диска. Пока оно там, индексация будет падать +каждую ночь, а чат отвечать без кодового контекста. Варианты: внешний Chroma +(осторожно — GHSA-f4j7-r4q5-qw2c в установленной версии), pgvector в уже имеющемся Postgres, +или управляемый векторный сервис. Это единственный дефект, который чинит сам себя ежедневно и не может.
+2. Починить учёт токенов. Одна строка на адаптер либо вывод суммы в роутере.
+Историю пересчитать из prompt_tokens + completion_tokens — данные целы.
+До этого любой разговор о лимитах и биллинге беспредметен.
3. Снять жёсткость stage_validation. Ожидаемые колонки — догадка планировщика,
+а не контракт. Сверять смысл, а не имена, либо давать агенту переименовать, а не отклонять.
+Это самая дешёвая правка с самым заметным эффектом на 25% отказов.
4. Заполнить route, complexity, failure_kind.
+Три прибора, которые показывают неправду, и один из них документирован как починенный.
5. Защитить main. Решение ваше, но за сутки девять PR ушли в центральный шов
+без единой обязательной проверки.
6. Определиться с GA4 и биллингом. Оба зарелизены и ни разу не работали в проде. +Либо довести до первого настоящего использования, либо честно пометить как непроверенные.
+request_traces, indexing_runs,
+indexing_run_events, trace_spans, token_usage, error_log,
+code_db_sync, логи воркера, heroku config, make config-drift,
+pytest, vitest. Ссылки на файл и строку указывают на текущий main (58b5c04).
+