# Code Review Report: Feature 21 — Document Management & Vector Cleanup

This report performs a deep-dive audit of the Hub's document lifecycle service in `document.py`, focusing on **Data Integrity**, **Vector Purging**, and **Database Stewardship**.

---

## 🏗️ 12-Factor App Compliance Audit

| Factor | Status | Observation |
| :--- | :--- | :--- |
| **IV. Backing Services** | 🔴 **Major Issue** | **Orphaned Vector Metadata**: The `delete_document` method (Line 51) correctly removes the primary document record but **comments out** the deletion of associated vector metadata (Line 62). This leads to "Ghost Vectors" accumulating in the database, breaking referential integrity and causing vector searches to return pointers to non-existent documents. |
| **VI. Processes** | 🟡 **Warning** | **Vector Search Drift**: Deleting a document from the DB does not trigger a removal or rebuild of the FAISS in-memory index. As a result, the RAG search results will continue to identify the deleted document as a "top match," but subsequent content fetching will fail with `404 Not Found`. |

---

## 🔍 File-by-File Diagnostic

### 1. `app/core/services/document.py`
The service managing document ingestion and retrieval for the Hub's RAG system.

> [!CAUTION]
> **Resource Leak (Incomplete Cleanup)**
> Line 64: `db.delete(doc_to_delete)`
> While the ORM handles the primary record, the lack of an explicit cleanup for the `VectorMetadata` table and the FAISS index itself represents a persistent storage leak. In a high-churn RAG environment (frequent document updates/deletes), the vector storage will grow indefinitely regardless of actual content size.
> 
> **Fix**: Uncomment the `VectorMetadata` deletion and implement a `vector_store.remove_document(id)` method to ensure the FAISS index is updated or rebuilt periodically to purge deleted IDs.

**Identified Problems**:
*   **Synchronous Commit Overhead**: `add_document` (Line 18) performs synchronous commits. For bulk ingestion (e.g., uploading a 500-page PDF), this will block the API worker threads sequentially for every page chunk rather than using a batched approach.
*   **Mock Fallback Logic**: The embedding model naming logic (Line 29) is brittle and hardcoded. It should be derived dynamically from a `settings` property or a class attribute on the `embedder` itself.

---

## 🛠️ Summary Recommendations

1.  **Enforce Referential Integrity**: Enable the commented-out `VectorMetadata` deletion to prevent database bloat and "Ghost Result" hazards in the RAG pipeline.
2.  **Implement Vector Invalidation**: Add a "soft-delete" or "purged" flag to vector metadata if FAISS rebuilds are too expensive, ensuring the search loop filters out invalid results.
3.  **Scoped Error Handling**: Update `delete_document` to return a boolean status rather than a potentially null ID, improving the caller's ability to handle missing records.

---

**This concludes Feature 21. I have persisted this report to `/app/docs/reviews/feature_review_document_vector_cleanup.md`. Should I apply the fix to enable vector metadata purging now?**
