feat(index): reclaim superseded index versions, and refuse to delete the live one#108
Conversation
…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
fvadicamo
added
documentation
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds guarded deletion of superseded index versions through storage providers, an admin API endpoint, CLI cleanup mode, tests, and updated reindex documentation. ChangesIndex version cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant reindex.sh
participant IndexVersionAPI
participant VectorStoreProvider
Operator->>reindex.sh: Run reindex or --cleanup
reindex.sh->>IndexVersionAPI: DELETE superseded index version
IndexVersionAPI->>VectorStoreProvider: delete_index_version(namespace, version)
VectorStoreProvider-->>IndexVersionAPI: chunks_removed or active-version error
IndexVersionAPI-->>reindex.sh: HTTP response
reindex.sh-->>Operator: Cleanup result and next-step guidance
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the cleanup of superseded index versions (DEBT-032 / REQ-064) by adding a delete_index_version method to the vector store providers (Qdrant and pgvector) and exposing it via a new admin endpoint DELETE /api/v1/index-versions/{index_version}. The providers correctly refuse to delete the active index version being served, raising an ActiveIndexVersionError (409). Additionally, scripts/reindex.sh has been updated with a --cleanup option, and comprehensive unit tests have been added. The review feedback correctly identifies that the new endpoint raises bare HTTPExceptions with string details, violating the repository style guide which mandates using ErrorResponse.to_envelope() for the detail field.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/reference/api.md (1)
944-944: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse 'afterward' instead of 'afterwards'.
As indicated by static analysis, "afterward" is the preferred variant in American English.
📝 Proposed fix
-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. +The switch is manual: there is **no** API to activate a version, it is an env var plus a restart. The cleanup afterward 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.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/reference/api.md` at line 944, Update the documentation sentence in the version-switching procedure to use the American English adverb “afterward” instead of “afterwards,” without changing the surrounding API or cleanup instructions.Source: Linters/SAST tools
vektra-index/src/vektra_index/reindex.py (1)
433-433: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSafely retrieve the provider registry and vector store.
request.app.state.registrymay raise anAttributeErrorif it is not set, andregistry.get()may returnNoneif the provider isn't registered. Similar to the existingtrigger_reindexfunction, safely retrieve these objects and raise an explicit 500 error if they are missing to prevent unhandled exception tracebacks.🛡️ Proposed fixes for safe retrieval
- vector_store = request.app.state.registry.get("vector_store", "default") + registry = getattr(request.app.state, "registry", None) + if registry is None: + raise HTTPException(status_code=500, detail="ProviderRegistry is not configured") + + vector_store = registry.get("vector_store", "default") + if vector_store is None: + raise HTTPException(status_code=500, detail="No vector store registered")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vektra-index/src/vektra_index/reindex.py` at line 433, Update the code around the vector_store retrieval to safely access request.app.state.registry and handle a missing registry or unregistered vector store. Follow the existing trigger_reindex function’s retrieval pattern, and raise an explicit HTTP 500 error when either dependency is unavailable instead of allowing AttributeError or None to propagate.vektra-index/tests/test_index_version_cleanup.py (1)
160-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPgvector test coverage gap: no namespace-scoping or idempotent-cleanup assertion.
Unlike
TestQdrantDeleteIndexVersion, which asserts the delete filter pins bothnamespace_idandindex_version(via_selector_conditions) and has a dedicated idempotent second-cleanup-returns-zero test,TestPgvectorDeleteIndexVersiononly checkssession.execute.await_count == 2and doesn't verify the WHERE clause scoping or add a parity test for the "second call returns 0" behavior called out in the PR objectives. Given namespace isolation is the core safety invariant this feature depends on, the pgvector suite should mirror that coverage.♻️ Suggested additions
`@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() + + `@pytest.mark.asyncio` + async def test_second_cleanup_reports_nothing_left(self) -> None: + from vektra_index.providers.pgvector import PgvectorProvider + + session = self._make_session(count=0) + provider = PgvectorProvider(active_index_version=2) + + assert await provider.delete_index_version(session, "default", 1) == 0Verifying the WHERE clause pins
namespace_id(not justindex_version) would require inspecting the compiled statement passed tosession.execute, since the fake session here doesn't capture the SQL construct the way the Qdrant fake captures the filter object.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vektra-index/tests/test_index_version_cleanup.py` around lines 160 - 193, Expand TestPgvectorDeleteIndexVersion to inspect the compiled statements passed to session.execute and assert the delete WHERE clause scopes both namespace_id and index_version, matching the existing _selector_conditions coverage in TestQdrantDeleteIndexVersion. Add an idempotent cleanup test that performs a second deletion and verifies it returns 0, while preserving the active-version refusal behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/reindex.sh`:
- Line 125: In scripts/reindex.sh at lines 125-125 and 191-197, replace direct
Python dictionary-key access with explicit JSON parsing and contract validation
for chunks_removed, chunks_reindexed, and source_index_version. Validate that
the response is valid JSON, each required field exists and has the expected
value type, and fail loudly with a clear field-specific error message when
validation fails; apply the same handling at both sites.
---
Nitpick comments:
In `@docs/reference/api.md`:
- Line 944: Update the documentation sentence in the version-switching procedure
to use the American English adverb “afterward” instead of “afterwards,” without
changing the surrounding API or cleanup instructions.
In `@vektra-index/src/vektra_index/reindex.py`:
- Line 433: Update the code around the vector_store retrieval to safely access
request.app.state.registry and handle a missing registry or unregistered vector
store. Follow the existing trigger_reindex function’s retrieval pattern, and
raise an explicit HTTP 500 error when either dependency is unavailable instead
of allowing AttributeError or None to propagate.
In `@vektra-index/tests/test_index_version_cleanup.py`:
- Around line 160-193: Expand TestPgvectorDeleteIndexVersion to inspect the
compiled statements passed to session.execute and assert the delete WHERE clause
scopes both namespace_id and index_version, matching the existing
_selector_conditions coverage in TestQdrantDeleteIndexVersion. Add an idempotent
cleanup test that performs a second deletion and verifies it returns 0, while
preserving the active-version refusal behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c4b76c1-1849-4112-abe1-34268017dfda
⛔ Files ignored due to path filters (1)
.s2s/BACKLOG.mdis excluded by!.s2s/**
📒 Files selected for processing (10)
docs/reference/api.mdscripts/reindex.shvektra-index/README.mdvektra-index/src/vektra_index/adapters.pyvektra-index/src/vektra_index/providers/pgvector.pyvektra-index/src/vektra_index/providers/qdrant.pyvektra-index/src/vektra_index/reindex.pyvektra-index/tests/test_index_version_cleanup.pyvektra-shared/src/vektra_shared/errors.pyvektra-shared/src/vektra_shared/protocols.py
… (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
Refs: #108, DEBT-032, REQ-064
1 participant
Summary
Closes DEBT-032. REQ-064 spells out "cleanup of old version afterwards" as part of the requirement. Nothing ever implemented it: reindex writes a second copy of every chunk under the target version and leaves 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 only became live when BUG-023 was fixed (#102). Until reindex actually wrote chunks, there was never an old version to clean up. The moment it did, every reindex started doubling a namespace's storage permanently, and the only exit was a hand-written delete against the store.
What changed
VectorStoreProvider.delete_index_version(namespace, index_version) -> intQdrantVectorStoreProviderandPgvectorProvider(+VectorStoreServiceAdapter)DELETE /api/v1/index-versions/{version}(admin scope)scripts/reindex.sh --cleanup OLD_VERSION [NAMESPACE]docs/reference/api.mdstep 5 replaces the manual store-level delete with the endpointThe guard, and where it lives
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 (a Qdrant filter delete, or
DELETE FROM document_chunks WHERE index_version = N) has no idea which version is live, and is run by an operator at exactly the moment they are least sure.So the refusal sits in the providers, not in the endpoint: a store is the only component that knows which version it reads. It raises
ActiveIndexVersionError(->409) rather than trusting each caller to check first, which means the guard also covers callers that are not this endpoint.Two consequences worth calling out:
VEKTRA_ACTIVE_INDEX_VERSION+ restart) sits between them, and before it the API correctly refuses.scripts/reindex.shis therefore two invocations, on purpose, and says so.DELETE /documents/{id}:403on mismatch, not a silent retarget. This endpoint deletes by filter, so a silently retargeted namespace would drop an entire version of a namespace the caller never named.Idempotent: a second call returns
chunks_removed: 0, which is how an operator confirms the old version is really gone.Proof the guard holds
Removing the guard was tried on purpose, and the tests go red:
test_endpoint_refuses_to_delete_the_version_being_servedfails withassert 200 == 409(i.e. the live index would have been emptied), plus the provider-level test.DELETEwould have executed).Endpoint tests wire a real provider over a stubbed client, not an
AsyncMockstore: a mocked guard proves nothing about the guard.Verified on the live dev stack (Qdrant, 1323 points)
DELETE /index-versions/1while v1 is activedefault-> v2--cleanup 1before the switch--cleanup 1after the switchchunks_removed: 0(idempotent)Stack restored to its exact initial state: 1323 points, all at version 1, original chunk ids,
.envbyte-identical.Notes for the reviewer
409/400on this endpoint use FastAPI's bare{"detail": ...}shape, matching the rest of the reindex router. That is DEBT-033, queued next, which will convert the whole family to the REQ-010 envelope at once. The403already uses the envelope (it reuses_namespace_scope_violation).DELETE, Qdrant is a filtered points delete.Checks
make lint- clean (ruff, mypy on 76 files, 8/8 import-linter contracts)make test- 765 passed, 3 skippedshellcheck scripts/*.sh- clean (run in a container; same check CI runs)Refs: REQ-064, ARCH-045, ADR-0026, BUG-023 (whose fix made this live)
Summary by CodeRabbit
New Features
Documentation