From 1988f18fbed7a8e7e215f8ddeacbe6d701e90adf Mon Sep 17 00:00:00 2001 From: Rajat Ahuja Date: Fri, 5 Dec 2025 16:29:20 -0500 Subject: [PATCH] fix: coderabbit comments --- src/config.py | 11 +++++++---- src/crud/document.py | 16 +++++++++++----- src/crud/message.py | 4 ++-- src/crud/session.py | 3 +-- ...t_f1a2b3c4d5e6_support_external_embeddings.py | 2 ++ 5 files changed, 23 insertions(+), 13 deletions(-) diff --git a/src/config.py b/src/config.py index f56b893f..5351298a 100644 --- a/src/config.py +++ b/src/config.py @@ -442,15 +442,18 @@ class AppSettings(HonchoSettings): """Propagate top-level NAMESPACE to nested settings if not explicitly set. After this validator runs, CACHE.NAMESPACE, METRICS.NAMESPACE, and - VECTOR_STORE.NAMESPACE are guaranteed to exist. + VECTOR_STORE.NAMESPACE are guaranteed to exist. Explicitly provided + nested namespaces are preserved. """ if self.CACHE.NAMESPACE is None: self.CACHE.NAMESPACE = self.NAMESPACE if self.METRICS.NAMESPACE is None: self.METRICS.NAMESPACE = self.NAMESPACE - # Note: VECTOR_STORE.NAMESPACE has its own default of "honcho", - # but we propagate the top-level NAMESPACE if the user explicitly set it - # and wants consistency across all namespaced services + + vector_namespace_explicit = "NAMESPACE" in self.VECTOR_STORE.model_fields_set + if not vector_namespace_explicit: + self.VECTOR_STORE.NAMESPACE = self.NAMESPACE + return self diff --git a/src/crud/document.py b/src/crud/document.py index de072373..a3fc458c 100644 --- a/src/crud/document.py +++ b/src/crud/document.py @@ -655,11 +655,17 @@ async def is_rejected_duplicate( namespace = vector_store.get_document_namespace( workspace_name, observer, observed ) - await vector_store.delete_many(namespace, [existing_doc.id]) + vector_deleted = False + try: + await vector_store.delete_many(namespace, [existing_doc.id]) + vector_deleted = True + except Exception: + existing_doc.deleted_at = datetime.datetime.now(datetime.timezone.utc) + await db.flush() - # Delete from database after vector store succeeds - await db.delete(existing_doc) - await db.flush() # Flush to make deletion visible in this transaction + if vector_deleted: + await db.delete(existing_doc) + await db.flush() # Flush to make deletion visible in this transaction return False # Don't reject the new document @@ -700,7 +706,7 @@ async def cleanup_soft_deleted_documents( Returns: Count of documents cleaned up (only those where vector deletion succeeded) """ - cutoff = datetime.datetime.now(datetime.UTC) - datetime.timedelta( + cutoff = datetime.datetime.now(datetime.timezone.utc) - datetime.timedelta( minutes=older_than_minutes ) diff --git a/src/crud/message.py b/src/crud/message.py index 5a0aead7..b7566148 100644 --- a/src/crud/message.py +++ b/src/crud/message.py @@ -190,10 +190,10 @@ async def create_messages( ): with attempt: await vector_store.upsert_many(namespace, vector_records) - except Exception as e: + except Exception: # Final attempt failed - log but don't raise # MessageEmbedding records exist in DB, vectors can be added later - logger.error(f"Failed to upsert message vectors after retries: {e}") + logger.exception("Failed to upsert message vectors after retries") except Exception: logger.exception( diff --git a/src/crud/session.py b/src/crud/session.py index dfbd6cf6..967ae3f1 100644 --- a/src/crud/session.py +++ b/src/crud/session.py @@ -432,6 +432,7 @@ async def delete_session( ) ) embeddings = embedding_result.all() + vector_store = get_vector_store() if embeddings: # Build vector IDs: {message_id}_{chunk_index} @@ -439,7 +440,6 @@ async def delete_session( # Try to delete from vector store (best effort) try: - vector_store = get_vector_store() namespace = vector_store.get_message_namespace(workspace_name) await vector_store.delete_many(namespace, vector_ids) logger.debug( @@ -479,7 +479,6 @@ async def delete_session( if documents: # Group document IDs by namespace (observer/observed) docs_by_namespace: dict[str, list[str]] = {} - vector_store = get_vector_store() for doc in documents: namespace = vector_store.get_document_namespace( workspace_name, doc.observer, doc.observed diff --git a/tests/alembic/revisions/test_f1a2b3c4d5e6_support_external_embeddings.py b/tests/alembic/revisions/test_f1a2b3c4d5e6_support_external_embeddings.py index 828648cf..0b8f316d 100644 --- a/tests/alembic/revisions/test_f1a2b3c4d5e6_support_external_embeddings.py +++ b/tests/alembic/revisions/test_f1a2b3c4d5e6_support_external_embeddings.py @@ -11,6 +11,7 @@ def prepare_add_chunk_index_to_message_embeddings( verifier: MigrationVerifier, ) -> None: """Seed state and assertions before upgrading to f1a2b3c4d5e6.""" + verifier.assert_column_exists("message_embeddings", "embedding", nullable=False) # Verify chunk_index column doesn't exist before migration verifier.assert_column_exists("message_embeddings", "chunk_index", exists=False) @@ -22,3 +23,4 @@ def verify_add_chunk_index_to_message_embeddings( """Add assertions validating the effects of f1a2b3c4d5e6.""" # Verify chunk_index column was added with correct properties verifier.assert_column_exists("message_embeddings", "chunk_index", nullable=False) + verifier.assert_column_exists("message_embeddings", "embedding", nullable=True)