From 6918584b27f1df291804f258d06892b82533d33d Mon Sep 17 00:00:00 2001 From: Francesco Vadicamo Date: Tue, 14 Jul 2026 17:48:51 +0000 Subject: [PATCH 1/3] feat(index): reclaim superseded index versions, and refuse to delete the live one REQ-064 asks for "cleanup of old version afterwards". Nothing implemented it: reindex wrote a second copy of every chunk under the target version and left both in the store forever, while reindex.py's docstring told the operator it could "trigger cleanup of old-version chunks". There was nothing to trigger. The gap became live only when BUG-023 was fixed: until reindex actually wrote, there was never an old version to clean up. VectorStoreProvider gains delete_index_version(namespace, index_version), implemented by Qdrant and pgvector and exposed as DELETE /api/v1/index-versions/{version} (admin scope). The guard sits in the providers, not the endpoint. A store is the only component that knows which version it reads, so it raises ActiveIndexVersionError (409) rather than trusting each caller to check first. The failure mode this exists for is not "an old version survives", it is "the live index is emptied": the hand-written delete it replaces has no idea which version is live and is run at the moment the operator is least sure. scripts/reindex.sh --cleanup OLD_VERSION completes the lifecycle as a second run. It cannot be one run: the switch sits between reindex and cleanup, and before it the API correctly refuses. Namespace binding is enforced as on DELETE /documents/{id} (403 on mismatch, not a silent retarget): this deletes by filter, so a retargeted namespace would drop an entire version of a namespace the caller never named. Verified on the dev stack (Qdrant): reindexing `default` to v2 doubled it 12 -> 24 points, the switch left live search byte-identical, cleanup reclaimed exactly the 12 old points, deleting the *active* version was refused with 409, and the six other namespaces (all at version 1) lost nothing. Refs: REQ-064, ARCH-045, ADR-0026, DEBT-032 --- .s2s/BACKLOG.md | 11 +- docs/reference/api.md | 69 +++- scripts/reindex.sh | 148 ++++++-- vektra-index/README.md | 2 + vektra-index/src/vektra_index/adapters.py | 21 ++ .../src/vektra_index/providers/pgvector.py | 34 ++ .../src/vektra_index/providers/qdrant.py | 43 +++ vektra-index/src/vektra_index/reindex.py | 79 ++++- .../tests/test_index_version_cleanup.py | 316 ++++++++++++++++++ vektra-shared/src/vektra_shared/errors.py | 24 ++ vektra-shared/src/vektra_shared/protocols.py | 14 + 11 files changed, 714 insertions(+), 47 deletions(-) create mode 100644 vektra-index/tests/test_index_version_cleanup.py diff --git a/.s2s/BACKLOG.md b/.s2s/BACKLOG.md index f52f8b4b..2af1793b 100644 --- a/.s2s/BACKLOG.md +++ b/.s2s/BACKLOG.md @@ -899,8 +899,9 @@ Swagger at `/docs` is auto-generated and complete, but the markdown reference do ### DEBT-032: no way to clean up an old index version after a reindex -**Status**: planned | **Priority**: high | **Created**: 2026-07-14 | **Raised to high**: 2026-07-14 +**Status**: completed (2026-07-14) | **Priority**: high | **Created**: 2026-07-14 | **Raised to high**: 2026-07-14 | **PR**: #108 **Origin**: DOCS-009 (2026-07-14), found while documenting the reindex flow end to end. +**Resolution**: `VectorStoreProvider` gained `delete_index_version(namespace, index_version)`, implemented by Qdrant and pgvector, exposed as `DELETE /api/v1/index-versions/{version}` (admin). The guard lives in the providers, not the endpoint: a store is the only component that knows which version it reads, so it raises `ActiveIndexVersionError` (409) rather than trusting callers to check first. `scripts/reindex.sh --cleanup OLD_VERSION` completes the lifecycle as a second run, because the switch sits between the two and the API correctly refuses a cleanup before it. Verified on the dev stack: reindexing `default` to v2 doubled the namespace (12 -> 24 points), the switch left live search byte-identical, the cleanup reclaimed exactly the 12 old points, and the same call against the *active* version was refused with 409 while the six other namespaces (all at version 1) lost nothing. **Why high, and why this is not really "debt"**: REQ-064 spells the cleanup out as part of the requirement ("new chunks created with incremented version alongside old, atomic switch via config change, **cleanup of old version afterwards**"). So this is not a suboptimal-but-working solution: it is an acceptance criterion of a shipped requirement that was never built, while the module docstring tells the operator it exists. Code that promises a capability it does not have is the same disease as BUG-023, one level up. @@ -913,10 +914,10 @@ The module docstring in `vektra-index/src/vektra_index/reindex.py` promises the Consequences: every reindex permanently doubles the storage for that namespace, and the only way to reclaim it is a hand-written delete against the store (a Qdrant filter delete, or `DELETE FROM document_chunks WHERE index_version = N`). That is an irreversible operation with no namespace guard, run by hand, at exactly the moment the operator is least sure of what is live — the failure mode being the deletion of the *active* version, which empties the live index. `docs/reference/api.md` documents the manual procedure with the warnings it needs, but the procedure should not be manual. **Acceptance criteria**: -- [ ] `VectorStoreProvider` gains a version-scoped delete, implemented by both pgvector and Qdrant -- [ ] An admin endpoint exposes it and **refuses to delete the version the system is currently serving**, with a test that proves the refusal: the destructive failure mode here is not "an old version survives", it is "the live index is emptied" -- [ ] `scripts/reindex.sh` can complete the lifecycle -- [ ] `docs/reference/api.md` replaces the manual store-level procedure with the endpoint +- [x] `VectorStoreProvider` gains a version-scoped delete, implemented by both pgvector and Qdrant +- [x] An admin endpoint exposes it and **refuses to delete the version the system is currently serving**, with a test that proves the refusal: the destructive failure mode here is not "an old version survives", it is "the live index is emptied" +- [x] `scripts/reindex.sh` can complete the lifecycle +- [x] `docs/reference/api.md` replaces the manual store-level procedure with the endpoint **Traceability**: REQ-064 (unimplemented acceptance criterion), ARCH-045 (index versioning), ADR-0026 (the Protocol that needs the version-scoped delete), BUG-023 (whose fix made this live) diff --git a/docs/reference/api.md b/docs/reference/api.md index bf234476..93b04792 100644 --- a/docs/reference/api.md +++ b/docs/reference/api.md @@ -941,7 +941,7 @@ Re-embed a namespace under a **new index version** without downtime (REQ-064, AR The mechanism: chunks are versioned. Every read (search, stats, chunk listing) filters on the **active** index version, set by `VEKTRA_ACTIVE_INDEX_VERSION` (default `1`). A reindex writes a second copy of the chunks under the target version, **alongside** the live ones, which keep serving traffic. The new version becomes live only when the operator changes the env var and restarts. Nothing is deleted along the way, so a bad reindex is rolled back by simply not switching. -The switch and the cleanup are manual. There is **no** API to activate a version or to delete an old one: activation is an env var plus a restart, and cleanup is a store-level delete (see [Step 5](#step-5-clean-up-the-old-version)). +The switch is manual: there is **no** API to activate a version, it is an env var plus a restart. The cleanup afterwards is an API call, `DELETE /api/v1/index-versions/{version}` (see [Step 5](#step-5-clean-up-the-old-version)), which refuses to delete whichever version is currently being served. ### POST /api/v1/reindex @@ -1022,9 +1022,44 @@ Errors: `404` if the job id is unknown, or belongs to a different namespace than **Why `chunks_reindexed` matters.** Until BUG-023 the job read chunks from a Postgres table that is empty in Qdrant mode, re-embedded nothing, wrote nothing, and still reported `completed`. An operator polling `status` alone would have seen a green result and switched the active version to an **empty index**. The job now refuses to report success when it wrote nothing: a namespace with documents but zero reindexed chunks fails with `error_message` explaining that the store returned no chunks at the source version. `chunks_reindexed` makes the distinction visible rather than implied — treat a completed job whose `chunks_reindexed` is 0 (against a non-empty namespace) as a bug, not a no-op. +### DELETE /api/v1/index-versions/{index_version} + +Reclaim the storage held by a superseded index version (REQ-064). Reindex leaves both versions in the store; this is the only thing that removes one. + +**Scopes**: `admin` + +```bash +curl -s -X DELETE \ + -H "Authorization: Bearer $VEKTRA_API_KEY" \ + "http://localhost:8000/api/v1/index-versions/1?namespace=default" | python3 -m json.tool +``` + +| Parameter | In | Default | Description | +|-----------|-----|---------|-------------| +| `index_version` | path | (required) | The version to delete. Must be `>= 1`, and must **not** be the active one. | +| `namespace` | query | `default` | Namespace to clean up. A namespace-bound key may only name its own; naming another returns `403`. | + +```json +{ + "namespace": "default", + "index_version": 1, + "chunks_removed": 12 +} +``` + +Errors: + +- **`409`** if `index_version` is the version the system is currently serving. **This is the guard that matters.** The store refuses before deleting anything, so the request is a no-op, not a partial wipe. The failure mode it exists to prevent is not "an old version survives", it is "the live index is emptied": get the version number wrong by one and every query stops finding anything. +- `400` if `index_version` is below 1. +- `403` for a namespace-bound key naming another namespace. The delete is by filter, so a silently retargeted namespace would drop an entire version of a namespace the caller never named. + +The call is **idempotent**: deleting a version that is already gone returns `200` with `chunks_removed: 0`. That is the cheapest way to confirm a cleanup really happened, and it is safe to repeat. + +It is also the one **irreversible** step in the whole flow. Everything before it can be undone by not switching, or by switching back. Once the old version is deleted, rolling back means reindexing again from scratch. + ### End-to-end operator flow -`scripts/reindex.sh TARGET_VERSION [NAMESPACE]` automates steps 1 and 2. The steps below are what it does, plus the manual part it deliberately stops short of. +`scripts/reindex.sh TARGET_VERSION [NAMESPACE]` automates steps 1 and 2; `scripts/reindex.sh --cleanup OLD_VERSION [NAMESPACE]` does step 5. They are two separate runs on purpose: between them sits the switch, which only a human can do, and until it happens the API will (correctly) refuse the cleanup. #### Step 1: trigger @@ -1083,24 +1118,30 @@ To roll back, set the variable back to `1` and recreate again: version 1 is stil #### Step 5: clean up the old version -**There is no API for this.** The old version's chunks stay in the store, invisible to reads but occupying space, until they are removed at the store level. Once the new version is verified in production, delete them directly: +Until this step, the old version is still in the store: invisible to reads, but occupying exactly as much space as the live one. Every reindex you never clean up doubles the namespace again. + +Do it **after** step 4, once the new version has served real traffic and you are no longer going to roll back: ```bash -# Qdrant -curl -s -X POST "http://localhost:6333/collections/vektra/points/delete?wait=true" \ - -H "Content-Type: application/json" \ - -d '{"filter":{"must":[ - {"key":"namespace_id","match":{"value":"default"}}, - {"key":"index_version","match":{"value":1}}]}}' +curl -s -X DELETE \ + -H "Authorization: Bearer $VEKTRA_API_KEY" \ + "http://localhost:8000/api/v1/index-versions/1?namespace=default" +# {"namespace":"default","index_version":1,"chunks_removed":12} ``` -```sql --- pgvector -DELETE FROM document_chunks -WHERE namespace_id = 'default' AND index_version = 1; +Or: `scripts/reindex.sh --cleanup 1 default`. + +Ordering is not left to your memory. If you run this **before** the switch, version 1 is still the active one and the API refuses with `409` — the request deletes nothing: + +```json +{"detail": "Refusing to delete index version 1 of namespace 'default': it is the version currently being served. Switch VEKTRA_ACTIVE_INDEX_VERSION to the new version and restart before cleaning up the old one."} ``` -Both are irreversible and neither is namespace-safe by default: **always filter on both `namespace_id` and `index_version`**, and count the matching rows/points before deleting. Deleting the version that is currently active empties the live index. +That refusal is the whole reason this is an endpoint rather than a store-level delete. Reclaiming space is the small half of the job; the large half is that the obvious hand-written version of it — a Qdrant filter delete, or `DELETE FROM document_chunks WHERE index_version = 1` — has no idea which version is live, and is run by an operator at precisely the moment they are least sure. One wrong number and the live index is empty, with no error and nothing to roll back to. + +To confirm it is really gone, run it again: a second call returns `chunks_removed: 0`. + +This is the one irreversible step in the flow. Everything before it is undone by not switching, or by switching back. ## Analytics diff --git a/scripts/reindex.sh b/scripts/reindex.sh index 6905bf5f..f1c86426 100755 --- a/scripts/reindex.sh +++ b/scripts/reindex.sh @@ -1,53 +1,100 @@ #!/usr/bin/env bash # Vektra zero-downtime reindex script # Usage: scripts/reindex.sh TARGET_VERSION [NAMESPACE] +# scripts/reindex.sh --cleanup OLD_VERSION [NAMESPACE] +# +# The two modes are deliberately two runs. A reindex writes the new version +# alongside the old one and does not switch to it: until the operator sets +# VEKTRA_ACTIVE_INDEX_VERSION and restarts, the old version is the one serving +# traffic, and the API refuses to delete it (409). So the cleanup cannot happen +# in the same run as the reindex -- it happens after the switch, and refusing to +# fake that is what keeps the script from deleting a live index. set -euo pipefail BASE_URL="${VEKTRA_API_URL:-http://localhost:${VEKTRA_PORT:-8000}}" POLL_INTERVAL=5 POLL_TIMEOUT=600 +usage() { + echo "Usage: scripts/reindex.sh TARGET_VERSION [NAMESPACE]" + echo " scripts/reindex.sh --cleanup OLD_VERSION [NAMESPACE]" + echo "" + echo "Trigger a zero-downtime reindex job, or reclaim a superseded version." + echo "" + echo "Arguments:" + echo " TARGET_VERSION Target index version (required, integer >= 1)" + echo " OLD_VERSION With --cleanup: the superseded version to delete" + echo " NAMESPACE Target namespace (default: 'default')" + echo "" + echo "Options:" + echo " --cleanup Delete a superseded index version. Run it only after" + echo " switching VEKTRA_ACTIVE_INDEX_VERSION to the new one:" + echo " the API refuses to delete the version it is serving." + echo " --dry-run Print what would be called, then exit" + echo "" + echo "Environment:" + echo " VEKTRA_API_URL Base URL (default: http://localhost:8000)" + echo " VEKTRA_API_KEY Bearer token (required)" +} + # ---- arg parsing ---- +MODE="reindex" +DRY_RUN="false" +VERSION="" +NAMESPACE="default" +POSITIONAL=0 + for arg in "$@"; do case "$arg" in --dry-run) - echo "[dry-run] Would trigger reindex at ${BASE_URL}/api/v1/reindex" - exit 0 + DRY_RUN="true" + ;; + --cleanup) + MODE="cleanup" ;; -h|--help) - echo "Usage: scripts/reindex.sh TARGET_VERSION [NAMESPACE]" - echo "" - echo "Trigger a zero-downtime reindex job." - echo "" - echo "Arguments:" - echo " TARGET_VERSION Target index version (required, integer >= 1)" - echo " NAMESPACE Target namespace (default: 'default')" - echo "" - echo "Environment:" - echo " VEKTRA_API_URL Base URL (default: http://localhost:8000)" - echo " VEKTRA_API_KEY Bearer token (required)" + usage exit 0 ;; - --invalid-*|--*) + --invalid-*|--*|-*) echo "Error: unknown argument '$arg'" >&2 - echo "Usage: scripts/reindex.sh TARGET_VERSION [NAMESPACE]" >&2 + usage >&2 exit 1 ;; + *) + if [ "$POSITIONAL" -eq 0 ]; then + VERSION="$arg" + else + NAMESPACE="$arg" + fi + POSITIONAL=$((POSITIONAL + 1)) + ;; esac done -TARGET_VERSION="${1:-}" -NAMESPACE="${2:-default}" +if [ "$DRY_RUN" = "true" ]; then + if [ "$MODE" = "cleanup" ]; then + echo "[dry-run] Would delete index version ${VERSION:-} of namespace '${NAMESPACE}'" \ + "at ${BASE_URL}/api/v1/index-versions/${VERSION:-}" + else + echo "[dry-run] Would trigger reindex at ${BASE_URL}/api/v1/reindex" + fi + exit 0 +fi # ---- validation ---- -if [ -z "$TARGET_VERSION" ]; then - echo "Error: TARGET_VERSION argument is required" >&2 - echo "Usage: scripts/reindex.sh TARGET_VERSION [NAMESPACE]" >&2 +if [ -z "$VERSION" ]; then + if [ "$MODE" = "cleanup" ]; then + echo "Error: OLD_VERSION argument is required with --cleanup" >&2 + else + echo "Error: TARGET_VERSION argument is required" >&2 + fi + usage >&2 exit 1 fi -if ! [[ "$TARGET_VERSION" =~ ^[0-9]+$ ]] || [ "$TARGET_VERSION" -lt 1 ]; then - echo "Error: TARGET_VERSION must be an integer >= 1" >&2 +if ! [[ "$VERSION" =~ ^[0-9]+$ ]] || [ "$VERSION" -lt 1 ]; then + echo "Error: version must be an integer >= 1" >&2 exit 1 fi @@ -56,6 +103,49 @@ if [ -z "${VEKTRA_API_KEY:-}" ]; then exit 1 fi +# ---- cleanup mode ---- +if [ "$MODE" = "cleanup" ]; then + echo "Deleting index version ${VERSION} of namespace '${NAMESPACE}'..." + + # No -f: a 409 means the API refused because that version is the one being + # served, and that body is the most useful thing the operator can read. + CLEANUP_RESP=$(curl -s -w "\n%{http_code}" \ + -X DELETE \ + -H "Authorization: Bearer ${VEKTRA_API_KEY}" \ + "${BASE_URL}/api/v1/index-versions/${VERSION}?namespace=${NAMESPACE}") || { + echo "Error: cleanup request failed" >&2 + exit 1 + } + + CLEANUP_CODE=$(echo "$CLEANUP_RESP" | tail -1) + CLEANUP_BODY=$(echo "$CLEANUP_RESP" | sed '$d') + + case "$CLEANUP_CODE" in + 200) + REMOVED=$(echo "$CLEANUP_BODY" | python3 -c "import sys,json; print(json.load(sys.stdin)['chunks_removed'])") + echo "Removed ${REMOVED} chunk(s) at index version ${VERSION}." + if [ "$REMOVED" -eq 0 ]; then + echo "Nothing was left at that version: it had already been cleaned up." + fi + exit 0 + ;; + 409) + echo "Error: refused. Index version ${VERSION} is the one currently being served." >&2 + echo "$CLEANUP_BODY" >&2 + echo "" >&2 + echo "Switch VEKTRA_ACTIVE_INDEX_VERSION to the new version and restart first." >&2 + exit 1 + ;; + *) + echo "Error: unexpected HTTP ${CLEANUP_CODE}" >&2 + echo "$CLEANUP_BODY" >&2 + exit 1 + ;; + esac +fi + +TARGET_VERSION="$VERSION" + # ---- trigger reindex ---- echo "Triggering reindex for namespace '${NAMESPACE}' (target version: ${TARGET_VERSION})..." JSON_BODY=$(python3 -c " @@ -98,8 +188,18 @@ while [ "$ELAPSED" -lt "$POLL_TIMEOUT" ]; do printf " [%3ds] status: %s progress: %s\n" "$ELAPSED" "$STATUS" "$PROCESSED" case "$STATUS" in completed|complete|done) - echo "Reindex completed successfully." - echo "To activate the new index: set VEKTRA_ACTIVE_INDEX_VERSION=${TARGET_VERSION} and restart." + CHUNKS=$(echo "$STATUS_RESP" | python3 -c "import sys,json; print(json.load(sys.stdin)['chunks_reindexed'])") + echo "Reindex completed: ${CHUNKS} chunk(s) written at version ${TARGET_VERSION}." + if [ "$CHUNKS" -eq 0 ]; then + echo "Warning: it wrote nothing. Do not switch: the new version is empty." >&2 + exit 1 + fi + SOURCE_VERSION=$(echo "$STATUS_RESP" | python3 -c "import sys,json; print(json.load(sys.stdin)['source_index_version'])") + echo "" + echo "Next:" + echo " 1. Check that ${CHUNKS} matches what you expected before switching." + echo " 2. Set VEKTRA_ACTIVE_INDEX_VERSION=${TARGET_VERSION} and restart." + echo " 3. Reclaim the old version: scripts/reindex.sh --cleanup ${SOURCE_VERSION} ${NAMESPACE}" exit 0 ;; failed|error) diff --git a/vektra-index/README.md b/vektra-index/README.md index 6231d0b7..8fb8e422 100644 --- a/vektra-index/README.md +++ b/vektra-index/README.md @@ -24,4 +24,6 @@ The provider also exposes a `raw_filters` escape hatch for backend-specific meta Operator script `scripts/reindex.sh` triggers a full re-embedding into a new index version while serving from the active one. After completion, `VEKTRA_ACTIVE_INDEX_VERSION` is bumped atomically (provider-specific transaction). +Both versions are then in the store. `scripts/reindex.sh --cleanup OLD_VERSION` (`DELETE /api/v1/index-versions/{version}`) reclaims the superseded one; skipping it leaves the namespace's storage permanently doubled. The store refuses to delete the version it is currently serving, so the cleanup cannot be run before the switch. + See [architecture.md](../.s2s/architecture.md) for the component specification. diff --git a/vektra-index/src/vektra_index/adapters.py b/vektra-index/src/vektra_index/adapters.py index 3960f1d0..8258b0a4 100644 --- a/vektra-index/src/vektra_index/adapters.py +++ b/vektra-index/src/vektra_index/adapters.py @@ -177,6 +177,27 @@ async def delete(self, namespace: str, ids: list[str]) -> int: raise return total + async def delete_index_version(self, namespace: str, index_version: int) -> int: + """Delete a namespace's chunks at one index version (REQ-064). + + The active-version refusal is PgvectorProvider's, not this wrapper's: + keeping it below the session boundary means it holds for every caller + of the provider, not just the ones that come through here. + """ + factory = self._get_session_factory() + pgvector = self._get_pgvector() + + async with factory() as session: + try: + removed = await pgvector.delete_index_version( + session, namespace, index_version + ) + await session.commit() + return removed + except Exception: + await session.rollback() + raise + async def health_check(self) -> HealthStatus: factory = self._get_session_factory() pgvector = self._get_pgvector() diff --git a/vektra-index/src/vektra_index/providers/pgvector.py b/vektra-index/src/vektra_index/providers/pgvector.py index 2bdd955e..4275c53e 100644 --- a/vektra-index/src/vektra_index/providers/pgvector.py +++ b/vektra-index/src/vektra_index/providers/pgvector.py @@ -20,6 +20,7 @@ from sqlalchemy import Float, delete, func, literal_column, select, text from sqlalchemy.ext.asyncio import AsyncSession +from vektra_shared.errors import ActiveIndexVersionError from vektra_shared.types import ( ChunkEmbedding, HealthStatus, @@ -547,6 +548,39 @@ async def count_chunks( return int((await session.execute(stmt)).scalar_one()) + async def delete_index_version( + self, + session: AsyncSession, + namespace: str, + index_version: int, + ) -> int: + """Delete a namespace's chunks at one index version (REQ-064). + + Refuses the active version: that DELETE is the one that empties the + live index. Counts before deleting, so chunks_removed is the truth + rather than an assumption, and a second call returning 0 is how the + operator confirms the old version is really gone. + """ + from vektra_index.models import DocumentChunkOrm + + if index_version == self._active_index_version: + raise ActiveIndexVersionError(namespace, index_version) + + where_clause = ( + DocumentChunkOrm.namespace_id == namespace, + DocumentChunkOrm.index_version == index_version, + ) + + count_result = await session.execute( + select(func.count()).select_from(DocumentChunkOrm).where(*where_clause) + ) + chunks_count = count_result.scalar_one() + + await session.execute(delete(DocumentChunkOrm).where(*where_clause)) + + await session.flush() + return chunks_count + async def health_check(self, session: AsyncSession) -> HealthStatus: """Quick connectivity check via SELECT 1 FROM document_chunks.""" try: diff --git a/vektra-index/src/vektra_index/providers/qdrant.py b/vektra-index/src/vektra_index/providers/qdrant.py index e0ddbfec..015518e0 100644 --- a/vektra-index/src/vektra_index/providers/qdrant.py +++ b/vektra-index/src/vektra_index/providers/qdrant.py @@ -20,6 +20,7 @@ from typing import Any from uuid import UUID, uuid4 +from vektra_shared.errors import ActiveIndexVersionError from vektra_shared.types import ( ChunkEmbedding, HealthStatus, @@ -546,6 +547,48 @@ async def delete(self, namespace: str, ids: list[str]) -> int: ) return int(count_result.count) + async def delete_index_version(self, namespace: str, index_version: int) -> int: + """Delete a namespace's points at one index version (REQ-064). + + Refuses the active version: both versions live in this one collection + and are told apart only by payload, so a filter that got the version + wrong would delete the points that are serving traffic. + + Counts before deleting, as delete() does: Qdrant's UpdateResult does not + carry a count, and a second call returning 0 is how the operator + confirms the old version is really gone. + """ + if index_version == self._active_index_version: + raise ActiveIndexVersionError(namespace, index_version) + + from qdrant_client import models + + selector = models.Filter( + must=[ + models.FieldCondition( + key="namespace_id", + match=models.MatchValue(value=namespace), + ), + models.FieldCondition( + key="index_version", + match=models.MatchValue(value=index_version), + ), + ] + ) + + count_result = await self._client.count( + collection_name=self._collection_name, + count_filter=selector, + exact=True, + ) + + await self._client.delete( + collection_name=self._collection_name, + points_selector=models.FilterSelector(filter=selector), + wait=True, + ) + return int(count_result.count) + async def health_check(self) -> HealthStatus: """Check Qdrant connectivity via get_collections().""" try: diff --git a/vektra-index/src/vektra_index/reindex.py b/vektra-index/src/vektra_index/reindex.py index cf290eb9..b77cfbd1 100644 --- a/vektra-index/src/vektra_index/reindex.py +++ b/vektra-index/src/vektra_index/reindex.py @@ -7,8 +7,11 @@ GET /api/v1/reindex/{job_id}/status returns current progress. The active index version switch is manual: the operator sets -VEKTRA_ACTIVE_INDEX_VERSION after reindex completes, then triggers -cleanup of old-version chunks. +VEKTRA_ACTIVE_INDEX_VERSION after reindex completes and restarts. Only then +does DELETE /api/v1/index-versions/{version} reclaim the old version, which +until that moment is the one serving traffic. That ordering is not a +convention the operator is asked to honour: the store refuses to delete the +version it is reading (DEBT-032). """ from __future__ import annotations @@ -18,13 +21,14 @@ from typing import Any from uuid import UUID, uuid4, uuid5 -from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException, Request +from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException, Query, Request from pydantic import BaseModel, Field from sqlalchemy import func, select, update from sqlalchemy.ext.asyncio import AsyncSession from vektra_shared.auth import ApiKeyInfo, require_scope from vektra_shared.db import get_session +from vektra_shared.errors import ActiveIndexVersionError logger = logging.getLogger(__name__) @@ -64,6 +68,12 @@ class ReindexStatusResponse(BaseModel): completed_at: str | None = None +class DeleteIndexVersionResponse(BaseModel): + namespace: str + index_version: int + chunks_removed: int + + # --------------------------------------------------------------------------- # Background job # --------------------------------------------------------------------------- @@ -103,7 +113,8 @@ async def run_reindex( The original chunks (source_version) are preserved for zero-downtime operation. Once the operator verifies the new index and switches - VEKTRA_ACTIVE_INDEX_VERSION, old-version chunks can be cleaned up. + VEKTRA_ACTIVE_INDEX_VERSION, the old version is reclaimed with + DELETE /api/v1/index-versions/{source_version}. This function runs outside the request lifecycle. It creates its own DB sessions as needed. @@ -390,3 +401,63 @@ async def reindex_status( created_at=job.created_at.isoformat(), completed_at=job.completed_at.isoformat() if job.completed_at else None, ) + + +@router.delete( + "/index-versions/{index_version}", response_model=DeleteIndexVersionResponse +) +async def delete_index_version( + index_version: int, + request: Request, + namespace: str | None = Query(None), + key: ApiKeyInfo = Depends(require_scope("admin")), +) -> DeleteIndexVersionResponse: + """Reclaim the storage of a superseded index version (REQ-064, DEBT-032). + + The last step of the reindex lifecycle, and the only irreversible one. Run + it once the new version has been switched in and verified: reindex leaves + both versions in the store, and nothing else removes the loser. + + Refuses to delete the version the store is currently reading, with 409. The + refusal lives in the provider, not here, so it also covers callers that are + not this endpoint. Idempotent: a second call returns chunks_removed=0, which + is how the operator confirms the old version is really gone. + """ + from vektra_index.api import _namespace_scope_violation + + if index_version < 1: + raise HTTPException( + status_code=400, detail="index_version must be an integer >= 1" + ) + + vector_store = request.app.state.registry.get("vector_store", "default") + + # Namespace binding (H5), as on DELETE /documents/{id}: refuse a mismatch + # rather than silently retargeting it. This endpoint deletes by filter, so a + # silent override would delete a whole version of a namespace the caller + # never named. + if key.namespace_id and namespace and namespace != key.namespace_id: + raise _namespace_scope_violation() + effective_ns = key.namespace_id or namespace or "default" + + try: + chunks_removed = await vector_store.delete_index_version( + effective_ns, index_version + ) + except ActiveIndexVersionError as exc: + raise HTTPException(status_code=409, detail=str(exc)) from exc + + logger.info( + "index_version_deleted", + extra={ + "namespace": effective_ns, + "index_version": index_version, + "chunks_removed": chunks_removed, + }, + ) + + return DeleteIndexVersionResponse( + namespace=effective_ns, + index_version=index_version, + chunks_removed=chunks_removed, + ) diff --git a/vektra-index/tests/test_index_version_cleanup.py b/vektra-index/tests/test_index_version_cleanup.py new file mode 100644 index 00000000..2e73ba18 --- /dev/null +++ b/vektra-index/tests/test_index_version_cleanup.py @@ -0,0 +1,316 @@ +"""Cleanup of a superseded index version (REQ-064, DEBT-032). + +Reindex writes a second copy of every chunk under the target version and leaves +both in the store. Nothing used to remove the loser, so every reindex doubled a +namespace's storage forever and the only exit was a hand-written delete against +the store. + +The destructive failure mode of that hand-written delete is not "an old version +survives", it is "the live index is emptied": one wrong version number and the +chunks that are answering queries are gone. So the guard these tests exist for +is the refusal to delete the active version, and they check the refusal at the +level where it is enforced (the store, which is the only thing that knows what +it is serving) as well as through the endpoint. + +The endpoint tests wire a real provider with a stubbed client rather than an +AsyncMock store: a mocked guard proves nothing about the guard. +""" + +from __future__ import annotations + +import sys +from types import ModuleType +from typing import Any +from unittest.mock import AsyncMock, MagicMock +from uuid import uuid4 + +import pytest +from fastapi import FastAPI +from httpx import ASGITransport, AsyncClient + +from vektra_shared.auth import ApiKeyInfo +from vektra_shared.errors import ActiveIndexVersionError +from vektra_shared.registry import ProviderRegistry + +# --------------------------------------------------------------------------- +# Fake qdrant_client +# --------------------------------------------------------------------------- +# +# qdrant-client is not installed for unit tests (CI runs `uv sync --dev` with no +# extras), so the module has to be stubbed. test_qdrant_provider.py stubs it with +# a bare MagicMock, which cannot answer "what filter did we build?": every +# attribute is another MagicMock. Half of what matters here is precisely the +# filter -- a delete that pins the version but forgets the namespace wipes that +# version for every tenant -- so these stand-ins keep their arguments readable. +# +# The provider imports qdrant_client lazily inside each method, so what counts is +# what sits in sys.modules when the method *runs*, not when this file is imported. +# pytest imports test_qdrant_provider.py after this one and its bare MagicMock +# would otherwise be the module in force by then. Hence an autouse fixture, which +# installs the fake per test and restores the previous entry afterwards. + + +class _MatchValue: + def __init__(self, value: Any) -> None: + self.value = value + + +class _FieldCondition: + def __init__(self, key: str, match: Any) -> None: + self.key = key + self.match = match + + +class _Filter: + def __init__( + self, must: list[Any] | None = None, must_not: list[Any] | None = None + ) -> None: + self.must = must or [] + self.must_not = must_not or [] + + +class _FilterSelector: + def __init__(self, filter: Any) -> None: + self.filter = filter + + +@pytest.fixture(autouse=True) +def fake_qdrant(monkeypatch: pytest.MonkeyPatch) -> None: + models = ModuleType("qdrant_client.models") + models.Filter = _Filter # type: ignore[attr-defined] + models.FieldCondition = _FieldCondition # type: ignore[attr-defined] + models.MatchValue = _MatchValue # type: ignore[attr-defined] + models.FilterSelector = _FilterSelector # type: ignore[attr-defined] + + qdrant = ModuleType("qdrant_client") + qdrant.models = models # type: ignore[attr-defined] + qdrant.AsyncQdrantClient = MagicMock # type: ignore[attr-defined] + + monkeypatch.setitem(sys.modules, "qdrant_client", qdrant) + monkeypatch.setitem(sys.modules, "qdrant_client.models", models) + + +def _selector_conditions(client: MagicMock) -> dict[str, Any]: + """The namespace/version pair the delete filter actually pinned.""" + selector = client.delete.await_args.kwargs["points_selector"] + return {c.key: c.match.value for c in selector.filter.must} + + +# --------------------------------------------------------------------------- +# Qdrant provider +# --------------------------------------------------------------------------- + + +def _make_qdrant_client(count: int = 7) -> MagicMock: + client = MagicMock() + client.count = AsyncMock(return_value=MagicMock(count=count)) + client.delete = AsyncMock() + return client + + +class TestQdrantDeleteIndexVersion: + @pytest.mark.asyncio + async def test_deletes_a_superseded_version_and_reports_the_count(self) -> None: + from vektra_index.providers.qdrant import QdrantVectorStoreProvider + + client = _make_qdrant_client(count=7) + provider = QdrantVectorStoreProvider(active_index_version=2, _client=client) + + removed = await provider.delete_index_version("default", 1) + + assert removed == 7 + client.delete.assert_awaited_once() + + # The filter must pin both namespace and version: pinning only the + # version would delete that version across every tenant. + assert _selector_conditions(client) == { + "namespace_id": "default", + "index_version": 1, + } + + @pytest.mark.asyncio + async def test_refuses_the_active_version_and_deletes_nothing(self) -> None: + """The whole point of DEBT-032: this call would empty the live index.""" + from vektra_index.providers.qdrant import QdrantVectorStoreProvider + + client = _make_qdrant_client() + provider = QdrantVectorStoreProvider(active_index_version=1, _client=client) + + with pytest.raises(ActiveIndexVersionError): + await provider.delete_index_version("default", 1) + + client.delete.assert_not_awaited() + + @pytest.mark.asyncio + async def test_second_cleanup_reports_nothing_left(self) -> None: + """Idempotent: chunks_removed=0 is how the operator confirms it is gone.""" + from vektra_index.providers.qdrant import QdrantVectorStoreProvider + + client = _make_qdrant_client(count=0) + provider = QdrantVectorStoreProvider(active_index_version=2, _client=client) + + assert await provider.delete_index_version("default", 1) == 0 + + +# --------------------------------------------------------------------------- +# Pgvector provider +# --------------------------------------------------------------------------- + + +class TestPgvectorDeleteIndexVersion: + @staticmethod + def _make_session(count: int = 5) -> AsyncMock: + session = AsyncMock() + session.execute = AsyncMock( + return_value=MagicMock(**{"scalar_one.return_value": count}) + ) + session.flush = AsyncMock() + return session + + @pytest.mark.asyncio + async def test_deletes_a_superseded_version_and_reports_the_count(self) -> None: + from vektra_index.providers.pgvector import PgvectorProvider + + session = self._make_session(count=5) + provider = PgvectorProvider(active_index_version=2) + + removed = await provider.delete_index_version(session, "default", 1) + + assert removed == 5 + # COUNT then DELETE. + assert session.execute.await_count == 2 + + @pytest.mark.asyncio + async def test_refuses_the_active_version_and_deletes_nothing(self) -> None: + from vektra_index.providers.pgvector import PgvectorProvider + + session = self._make_session() + provider = PgvectorProvider(active_index_version=1) + + with pytest.raises(ActiveIndexVersionError): + await provider.delete_index_version(session, "default", 1) + + session.execute.assert_not_awaited() + + +# --------------------------------------------------------------------------- +# DELETE /api/v1/index-versions/{index_version} +# --------------------------------------------------------------------------- + + +def _make_app( + active_index_version: int, + client: MagicMock, + namespace_id: str | None = None, +) -> FastAPI: + """App wired with a real Qdrant provider over a stubbed client. + + The guard under test is the provider's, so the provider has to be real. + """ + from vektra_index.providers.qdrant import QdrantVectorStoreProvider + from vektra_index.reindex import router + + key_store = AsyncMock() + key_store.lookup_by_token = AsyncMock( + return_value=ApiKeyInfo( + key_id=uuid4(), scopes=["admin"], namespace_id=namespace_id + ) + ) + + app = FastAPI() + app.state.registry = ProviderRegistry() + app.state.registry.register("key_store", "default", key_store) + app.state.registry.register( + "vector_store", + "default", + QdrantVectorStoreProvider( + active_index_version=active_index_version, _client=client + ), + ) + app.include_router(router) + return app + + +async def _delete_version( + app: FastAPI, version: int, params: dict | None = None +) -> tuple[int, dict]: + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://test" + ) as http: + resp = await http.delete( + f"/api/v1/index-versions/{version}", + params=params or {}, + headers={"Authorization": "Bearer test-token"}, + ) + return resp.status_code, resp.json() + + +@pytest.mark.asyncio +async def test_endpoint_removes_the_old_version() -> None: + client = _make_qdrant_client(count=12) + app = _make_app(active_index_version=2, client=client) + + status, body = await _delete_version(app, 1, {"namespace": "default"}) + + assert status == 200 + assert body["chunks_removed"] == 12 + assert body["index_version"] == 1 + assert body["namespace"] == "default" + client.delete.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_endpoint_refuses_to_delete_the_version_being_served() -> None: + """The criterion that matters: this request must not empty the live index. + + Version 1 is active, so deleting it is the operator error that takes the + system down. It has to fail loudly, and it has to fail *before* the store + is touched. + """ + client = _make_qdrant_client(count=12) + app = _make_app(active_index_version=1, client=client) + + status, body = await _delete_version(app, 1, {"namespace": "default"}) + + assert status == 409 + assert "currently being served" in body["detail"] + client.delete.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_endpoint_rejects_version_zero() -> None: + client = _make_qdrant_client() + app = _make_app(active_index_version=1, client=client) + + status, _ = await _delete_version(app, 0) + + assert status == 400 + client.delete.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_endpoint_refuses_a_namespace_outside_the_key_binding() -> None: + """A bound key must not clean up another tenant's version (H5). + + This deletes by filter, so a silently retargeted namespace would drop a + whole version of a namespace the caller never named. + """ + client = _make_qdrant_client() + app = _make_app(active_index_version=2, client=client, namespace_id="tenant-a") + + status, _ = await _delete_version(app, 1, {"namespace": "tenant-b"}) + + assert status == 403 + client.delete.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_endpoint_uses_the_key_namespace_when_none_is_given() -> None: + client = _make_qdrant_client(count=3) + app = _make_app(active_index_version=2, client=client, namespace_id="tenant-a") + + status, body = await _delete_version(app, 1) + + assert status == 200 + assert body["namespace"] == "tenant-a" + assert _selector_conditions(client)["namespace_id"] == "tenant-a" diff --git a/vektra-shared/src/vektra_shared/errors.py b/vektra-shared/src/vektra_shared/errors.py index f7510b06..78cc1e80 100644 --- a/vektra-shared/src/vektra_shared/errors.py +++ b/vektra-shared/src/vektra_shared/errors.py @@ -61,6 +61,30 @@ def to_envelope(self) -> dict[str, Any]: return {"error": data} +# --------------------------------------------------------------------------- +# Domain exceptions +# --------------------------------------------------------------------------- + + +class ActiveIndexVersionError(Exception): + """Raised when something tries to delete the index version being served. + + The guard behind REQ-064's cleanup step. A vector store refuses this from + inside delete_index_version() rather than trusting callers to check first, + because it is the only component that knows which version it reads. + """ + + def __init__(self, namespace: str, index_version: int) -> None: + self.namespace = namespace + self.index_version = index_version + super().__init__( + f"Refusing to delete index version {index_version} of namespace " + f"'{namespace}': it is the version currently being served. Switch " + "VEKTRA_ACTIVE_INDEX_VERSION to the new version and restart before " + "cleaning up the old one." + ) + + # --------------------------------------------------------------------------- # Normative error codes (REQ-011) # HTTP status mapping: diff --git a/vektra-shared/src/vektra_shared/protocols.py b/vektra-shared/src/vektra_shared/protocols.py index 88ac41ad..45561803 100644 --- a/vektra-shared/src/vektra_shared/protocols.py +++ b/vektra-shared/src/vektra_shared/protocols.py @@ -148,6 +148,20 @@ async def count_chunks(self, namespace: str | None = None) -> int: async def delete(self, namespace: str, ids: list[str]) -> int: ... + async def delete_index_version(self, namespace: str, index_version: int) -> int: + """Delete every chunk of a namespace at one index version (REQ-064). + + The counterpart of store(index_version=...): reindex writes a second + version alongside the live one, and this reclaims the version that lost. + Returns chunks_removed. + + Implementations MUST refuse to delete their own active index version and + raise ActiveIndexVersionError. The destructive failure mode here is not + "an old version survives", it is "the live index is emptied": the store + is the only component that knows which version it is serving, so the + refusal belongs here rather than in whichever caller happens to ask.""" + ... + async def health_check(self) -> HealthStatus: ... From a832d87abcf410b0bdb0175c8f18cf6eea7c241c Mon Sep 17 00:00:00 2001 From: Francesco Vadicamo Date: Tue, 14 Jul 2026 18:08:13 +0000 Subject: [PATCH 2/3] fix(index): put the cleanup endpoint's errors in the REQ-010 envelope (review) Gemini flagged that the 400 and 409 raised bare HTTPException strings while the repo style guide requires ErrorResponse.to_envelope(). It was right about the smell: the endpoint was internally inconsistent (its 403 already used the envelope, via _namespace_scope_violation) and, being new code, it was adding to DEBT-033 rather than staying clear of it. It was wrong about the fix. Its suggestion constructed ERR-INDEX-001/002 inline, and those codes did not exist: not in the errors.py registry, not in the status map, not in error-codes.md. Registering them is the actual work. - ERR-INDEX-001 (409): refused, that version is the one being served - ERR-INDEX-002 (400): index version below 1 Both are declared in vektra_shared.errors with their status overrides, and documented in docs/reference/error-codes.md. The 409 carries the refused namespace and index_version in details: an operator tool has to tell "wrong version, nothing happened" apart from "the store is down", and a bare string does not let it. DEBT-033's scope shrinks accordingly: this endpoint is already enveloped, so what remains is the pre-existing set (POST /reindex, GET status, conversations, admin conversation turns). Noted in its backlog entry. scripts/reindex.sh now prints the refusal message out of the envelope instead of dumping the raw JSON, falling back to the raw body if the shape is unexpected rather than dying on a KeyError and hiding why the delete was refused. Verified against the live stack: the 409 and 400 return the documented envelope, and the script renders the refusal as one readable line. Refs: #108, REQ-010, REQ-011, DEBT-032, DEBT-033 --- .s2s/BACKLOG.md | 2 + docs/reference/api.md | 23 +++++--- docs/reference/error-codes.md | 2 + scripts/reindex.sh | 15 +++++- vektra-index/src/vektra_index/reindex.py | 53 +++++++++++++++++-- .../tests/test_index_version_cleanup.py | 13 ++++- vektra-shared/src/vektra_shared/errors.py | 6 +++ 7 files changed, 99 insertions(+), 15 deletions(-) diff --git a/.s2s/BACKLOG.md b/.s2s/BACKLOG.md index 2af1793b..1a490c21 100644 --- a/.s2s/BACKLOG.md +++ b/.s2s/BACKLOG.md @@ -930,6 +930,8 @@ Consequences: every reindex permanently doubles the storage for that namespace, **Context**: REQ-010 defines a single error envelope (`{"error": {category, code, message, remediation, request_id, details}}`), and `docs/reference/api.md` presented it as universal. It is not. Reindex (`400`, `404`), conversations (`404`, `503`) and admin conversation turns (`404`, `501`, `503`) raise `HTTPException` with a plain string detail, so they return FastAPI's bare `{"detail": "..."}` — no code, no remediation, no request id. +**Scope reduced (2026-07-14, DEBT-032 / #108)**: the new `DELETE /api/v1/index-versions/{version}` was built enveloped from the start (`ERR-INDEX-001`, `ERR-INDEX-002`), so it is *not* part of this cleanup. What remains is the pre-existing set: `POST /reindex`, `GET /reindex/{job_id}/status`, conversations, admin conversation turns. + A client cannot branch on an error code for these endpoints, and the operator-facing reindex failures are exactly the ones where a machine-readable code would be worth having. The doc now warns that both shapes exist; the fix is to make the envelope actually universal. **Acceptance criteria**: diff --git a/docs/reference/api.md b/docs/reference/api.md index 93b04792..7da8e342 100644 --- a/docs/reference/api.md +++ b/docs/reference/api.md @@ -1047,11 +1047,11 @@ curl -s -X DELETE \ } ``` -Errors: +Errors (all in the REQ-010 envelope): -- **`409`** if `index_version` is the version the system is currently serving. **This is the guard that matters.** The store refuses before deleting anything, so the request is a no-op, not a partial wipe. The failure mode it exists to prevent is not "an old version survives", it is "the live index is emptied": get the version number wrong by one and every query stops finding anything. -- `400` if `index_version` is below 1. -- `403` for a namespace-bound key naming another namespace. The delete is by filter, so a silently retargeted namespace would drop an entire version of a namespace the caller never named. +- **`409` `ERR-INDEX-001`** if `index_version` is the version the system is currently serving. **This is the guard that matters.** The store refuses before deleting anything, so the request is a no-op, not a partial wipe. The failure mode it exists to prevent is not "an old version survives", it is "the live index is emptied": get the version number wrong by one and every query stops finding anything. `details` carries the `namespace` and `index_version` that were refused. +- `400` `ERR-INDEX-002` if `index_version` is below 1. +- `403` `ERR-AUTH-003` for a namespace-bound key naming another namespace. The delete is by filter, so a silently retargeted namespace would drop an entire version of a namespace the caller never named. The call is **idempotent**: deleting a version that is already gone returns `200` with `chunks_removed: 0`. That is the cheapest way to confirm a cleanup really happened, and it is safe to repeat. @@ -1134,7 +1134,16 @@ Or: `scripts/reindex.sh --cleanup 1 default`. Ordering is not left to your memory. If you run this **before** the switch, version 1 is still the active one and the API refuses with `409` — the request deletes nothing: ```json -{"detail": "Refusing to delete index version 1 of namespace 'default': it is the version currently being served. Switch VEKTRA_ACTIVE_INDEX_VERSION to the new version and restart before cleaning up the old one."} +{ + "error": { + "category": "PERMANENT", + "code": "ERR-INDEX-001", + "message": "Refusing to delete index version 1 of namespace 'default': it is the version currently being served. Switch VEKTRA_ACTIVE_INDEX_VERSION to the new version and restart before cleaning up the old one.", + "remediation": "Switch VEKTRA_ACTIVE_INDEX_VERSION to the new version and restart, then delete the old one. To check which version is live, see VEKTRA_ACTIVE_INDEX_VERSION in the running configuration.", + "request_id": "550e8400-e29b-41d4-a716-446655440000", + "details": {"namespace": "default", "index_version": 1} + } +} ``` That refusal is the whole reason this is an endpoint rather than a store-level delete. Reclaiming space is the small half of the job; the large half is that the obvious hand-written version of it — a Qdrant filter delete, or `DELETE FROM document_chunks WHERE index_version = 1` — has no idea which version is live, and is run by an operator at precisely the moment they are least sure. One wrong number and the live index is empty, with no error and nothing to roll back to. @@ -1323,10 +1332,10 @@ Most errors follow a standard envelope (REQ-010): See [error codes reference](error-codes.md) for the complete list. -Not every endpoint uses the envelope. Reindex, conversations and admin conversation turns return FastAPI's bare shape instead: +Not every endpoint uses the envelope. `POST /api/v1/reindex`, `GET /api/v1/reindex/{job_id}/status`, conversations and admin conversation turns return FastAPI's bare shape instead: ```json {"detail": "Reindex job not found"} ``` -Clients that parse errors must handle both. The envelope is the intended contract; the bare form is a gap, not a second contract to rely on. +Clients that parse errors must handle both. The envelope is the intended contract; the bare form is a gap (DEBT-033), not a second contract to rely on. `DELETE /api/v1/index-versions/{version}` uses the envelope throughout, including its `409`. diff --git a/docs/reference/error-codes.md b/docs/reference/error-codes.md index 9eb598fb..3b5bd6a2 100644 --- a/docs/reference/error-codes.md +++ b/docs/reference/error-codes.md @@ -92,6 +92,8 @@ These codes are used by specific components and are not part of the REQ-011 regi | ERR-LEARN-004 | vektra-learn | 409 | Duplicate enrollment (student already enrolled in course) | | ERR-LEARN-005 | vektra-learn | 404 | Conversation not found (GET conversations/turns) | | ERR-LEARN-006 | vektra-learn | 403 | Conversation belongs to a different course/namespace | +| ERR-INDEX-001 | vektra-index | 409 | Refused: the index version named for deletion is the one currently being served. Nothing was deleted (REQ-064) | +| ERR-INDEX-002 | vektra-index | 400 | Invalid index version (below 1) | ## Adding new error codes diff --git a/scripts/reindex.sh b/scripts/reindex.sh index f1c86426..19af1d62 100755 --- a/scripts/reindex.sh +++ b/scripts/reindex.sh @@ -130,8 +130,21 @@ if [ "$MODE" = "cleanup" ]; then exit 0 ;; 409) + # The refusal (ERR-INDEX-001) is the one message the operator must read, so + # dig it out of the envelope rather than dumping JSON at them. Falls back to + # the raw body if the shape is not what we expect, instead of dying on a + # KeyError and hiding the reason the delete was refused. echo "Error: refused. Index version ${VERSION} is the one currently being served." >&2 - echo "$CLEANUP_BODY" >&2 + echo "$CLEANUP_BODY" | python3 -c " +import sys, json +raw = sys.stdin.read() +try: + body = json.loads(raw) + err = body.get('detail', body).get('error', {}) + print(err.get('message') or err.get('remediation') or raw) +except Exception: + print(raw) +" >&2 echo "" >&2 echo "Switch VEKTRA_ACTIVE_INDEX_VERSION to the new version and restart first." >&2 exit 1 diff --git a/vektra-index/src/vektra_index/reindex.py b/vektra-index/src/vektra_index/reindex.py index b77cfbd1..38fc22ca 100644 --- a/vektra-index/src/vektra_index/reindex.py +++ b/vektra-index/src/vektra_index/reindex.py @@ -28,7 +28,14 @@ from vektra_shared.auth import ApiKeyInfo, require_scope from vektra_shared.db import get_session -from vektra_shared.errors import ActiveIndexVersionError +from vektra_shared.errors import ( + ERR_INDEX_001, + ERR_INDEX_002, + ActiveIndexVersionError, + ErrorCategory, + ErrorResponse, + http_status_for, +) logger = logging.getLogger(__name__) @@ -74,6 +81,44 @@ class DeleteIndexVersionResponse(BaseModel): chunks_removed: int +# --------------------------------------------------------------------------- +# Errors (REQ-010 envelope) +# --------------------------------------------------------------------------- + + +def _active_index_version_refusal(exc: ActiveIndexVersionError) -> HTTPException: + """409 for a cleanup aimed at the version being served. + + The one error on this endpoint a client would genuinely branch on, so it + carries a code rather than a bare string: an operator tool needs to tell + "wrong version, nothing happened" apart from "the store is down". + """ + err = ErrorResponse( + category=ErrorCategory.PERMANENT, + code=ERR_INDEX_001, + message=str(exc), + remediation=( + "Switch VEKTRA_ACTIVE_INDEX_VERSION to the new version and restart, " + "then delete the old one. To check which version is live, see " + "VEKTRA_ACTIVE_INDEX_VERSION in the running configuration." + ), + details={"namespace": exc.namespace, "index_version": exc.index_version}, + ) + return HTTPException(status_code=http_status_for(err), detail=err.to_envelope()) + + +def _invalid_index_version(index_version: int) -> HTTPException: + """400 for an index version below 1.""" + err = ErrorResponse( + category=ErrorCategory.PERMANENT, + code=ERR_INDEX_002, + message=f"index_version must be an integer >= 1, got {index_version}.", + remediation="Pass the index version you want to delete, as an integer >= 1.", + details={"index_version": index_version}, + ) + return HTTPException(status_code=http_status_for(err), detail=err.to_envelope()) + + # --------------------------------------------------------------------------- # Background job # --------------------------------------------------------------------------- @@ -426,9 +471,7 @@ async def delete_index_version( from vektra_index.api import _namespace_scope_violation if index_version < 1: - raise HTTPException( - status_code=400, detail="index_version must be an integer >= 1" - ) + raise _invalid_index_version(index_version) vector_store = request.app.state.registry.get("vector_store", "default") @@ -445,7 +488,7 @@ async def delete_index_version( effective_ns, index_version ) except ActiveIndexVersionError as exc: - raise HTTPException(status_code=409, detail=str(exc)) from exc + raise _active_index_version_refusal(exc) from exc logger.info( "index_version_deleted", diff --git a/vektra-index/tests/test_index_version_cleanup.py b/vektra-index/tests/test_index_version_cleanup.py index 2e73ba18..bd2607ce 100644 --- a/vektra-index/tests/test_index_version_cleanup.py +++ b/vektra-index/tests/test_index_version_cleanup.py @@ -273,18 +273,27 @@ async def test_endpoint_refuses_to_delete_the_version_being_served() -> None: status, body = await _delete_version(app, 1, {"namespace": "default"}) assert status == 409 - assert "currently being served" in body["detail"] client.delete.assert_not_awaited() + # REQ-010 envelope with a code: an operator tool has to tell "wrong version, + # nothing happened" apart from "the store is down", and cannot do that from + # a bare string. + err = body["detail"]["error"] + assert err["code"] == "ERR-INDEX-001" + assert err["details"] == {"namespace": "default", "index_version": 1} + assert "currently being served" in err["message"] + assert err["remediation"] + @pytest.mark.asyncio async def test_endpoint_rejects_version_zero() -> None: client = _make_qdrant_client() app = _make_app(active_index_version=1, client=client) - status, _ = await _delete_version(app, 0) + status, body = await _delete_version(app, 0) assert status == 400 + assert body["detail"]["error"]["code"] == "ERR-INDEX-002" client.delete.assert_not_awaited() diff --git a/vektra-shared/src/vektra_shared/errors.py b/vektra-shared/src/vektra_shared/errors.py index 78cc1e80..a1fc649a 100644 --- a/vektra-shared/src/vektra_shared/errors.py +++ b/vektra-shared/src/vektra_shared/errors.py @@ -137,6 +137,10 @@ def __init__(self, namespace: str, index_version: int) -> None: ERR_LEARN_005 = "ERR-LEARN-005" # Conversation not found (WI-1) ERR_LEARN_006 = "ERR-LEARN-006" # Conversation belongs to another course (WI-1) +# Index errors (ARCH-045, index versioning) +ERR_INDEX_001 = "ERR-INDEX-001" # Refused: that index version is the one being served +ERR_INDEX_002 = "ERR-INDEX-002" # Invalid index version (< 1) + # --------------------------------------------------------------------------- # HTTP status mapping helpers @@ -163,6 +167,8 @@ def __init__(self, namespace: str, index_version: int) -> None: ERR_LEARN_004: 409, ERR_LEARN_005: 404, ERR_LEARN_006: 403, + ERR_INDEX_001: 409, # Conflict: the version is live, deleting it is refused + ERR_INDEX_002: 400, } From af1312f24cfc2466897f618eed6d4fbb888f2c7c Mon Sep 17 00:00:00 2001 From: Francesco Vadicamo Date: Tue, 14 Jul 2026 18:15:27 +0000 Subject: [PATCH 3/3] docs(changelog): log the index-version cleanup endpoint (DEBT-032) Refs: #108, DEBT-032, REQ-064 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d1cd8f3e..f2fa76f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ Convention (Keep a Changelog 1.1.0): - **ci**: publish versioned container images to GHCR on tag push (INFRA-007). `v*` tags trigger `.github/workflows/publish.yml`, which builds and pushes `ghcr.io/vektralabs/vektra:{version}` and `:{version}-ocr` (GHA build cache, OCI version/revision labels, built-in `GITHUB_TOKEN`, no new secrets). Deployment docs and a new `deploy/docker-compose.image.yml.example` overlay cover the resulting `docker compose pull && docker compose up -d` flow as an alternative to building from source. No `latest` tag. The manual tagging flow is unchanged. - **index**: `GET /api/v1/documents/{document_id}/chunks` lists a document's stored chunks (text, position, parent link, metadata) at the active index version, read from the active vector store. Any valid scope. - **index**: `reindex_jobs.chunks_reindexed` (migration `0007`) records how many chunks a reindex actually re-embedded and wrote, exposed on `GET /api/v1/reindex/{job_id}/status`. A job that walked every document and stored nothing used to be indistinguishable from a real one. +- **index**: `DELETE /api/v1/index-versions/{version}` (admin) reclaims a superseded index version, completing the reindex lifecycle REQ-064 always specified ("cleanup of old version afterwards") and nothing implemented (DEBT-032). A reindex writes a second copy of every chunk under the target version and leaves both in the store; until now nothing removed the loser, so **every reindex doubled a namespace's storage permanently** and the only way back was a hand-written delete against the store. The gap was live only because BUG-023 was fixed: while reindex wrote nothing, there was never an old version to clean up. `VectorStoreProvider` grows `delete_index_version(namespace, index_version)`, implemented by both Qdrant and pgvector, and `scripts/reindex.sh --cleanup OLD_VERSION [NAMESPACE]` drives it. **The store refuses to delete the version it is currently serving** (409, `ERR-INDEX-001`), because the failure mode that matters here is not "an old version survives" but "the live index is emptied": the hand-written delete this replaces has no idea which version is live and is run at precisely the moment the operator is least sure. The refusal lives in the providers, not the endpoint, since a store is the only component that knows which version it reads — so it holds for every caller, and it is why the cleanup is a second run of the script rather than a flag on the first: the switch sits between them. The call is idempotent (a second one returns `chunks_removed: 0`) and namespace-bound keys cannot clean up another namespace's version (403). Verified against the live Qdrant stack: reindexing a namespace to v2 doubled it (12 → 24 points), the switch left search byte-identical, the cleanup reclaimed exactly the 12 old points, deleting the *active* version was refused, and the six other namespaces — all sitting at version 1 — lost nothing. ### Fixed