-
-
Notifications
You must be signed in to change notification settings - Fork 263
fix(rag): roll back + surface a document that fails to embed (no silent dead KB entries) #452
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -63,6 +63,9 @@ class RagService { | |||||||||||||||||||||
| await embeddingService.load(); | ||||||||||||||||||||||
| const texts = chunks.map(c => c.content); | ||||||||||||||||||||||
| const embeddings = await embeddingService.embedBatch(texts); | ||||||||||||||||||||||
| if (embeddings.length !== rowIds.length) { | ||||||||||||||||||||||
| throw new Error(`embedding count ${embeddings.length} does not match ${rowIds.length} chunks`); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| const entries = rowIds.map((rowId, i) => ({ | ||||||||||||||||||||||
| chunkRowid: rowId, | ||||||||||||||||||||||
| docId, | ||||||||||||||||||||||
|
|
@@ -71,7 +74,15 @@ class RagService { | |||||||||||||||||||||
| ragDatabase.insertEmbeddingsBatch(entries); | ||||||||||||||||||||||
| logger.log(`[RAG] Generated ${embeddings.length} embeddings for ${fileName}`); | ||||||||||||||||||||||
| } catch (err) { | ||||||||||||||||||||||
| logger.error('[RAG] Embedding generation failed (non-fatal):', err); | ||||||||||||||||||||||
| // A document with no embeddings is NOT searchable — semantic search skips it — | ||||||||||||||||||||||
| // yet the KB would show it as "added". That is the "looks indexed but doesn't | ||||||||||||||||||||||
| // work" lie. backfillEmbeddings has no auto-trigger, so a swallowed failure | ||||||||||||||||||||||
| // strands the doc permanently. Roll back the just-inserted doc + chunks and | ||||||||||||||||||||||
| // surface the failure so the user sees the real error (KnowledgeBaseScreen | ||||||||||||||||||||||
| // already alerts on an index throw) and can retry — never a silent dead entry. | ||||||||||||||||||||||
| ragDatabase.deleteDocument(docId); | ||||||||||||||||||||||
| logger.error('[RAG] Embedding generation failed — rolled back document:', err); | ||||||||||||||||||||||
| throw new Error(`Could not index "${fileName}": embeddings failed to generate. ${err instanceof Error ? err.message : String(err)}`); | ||||||||||||||||||||||
|
Comment on lines
+83
to
+85
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win Guard the rollback delete so it can't mask the original failure. If 🛡️ Proposed fix- ragDatabase.deleteDocument(docId);
- logger.error('[RAG] Embedding generation failed — rolled back document:', err);
+ try {
+ ragDatabase.deleteDocument(docId);
+ logger.error('[RAG] Embedding generation failed — rolled back document:', err);
+ } catch (rollbackErr) {
+ logger.error('[RAG] Embedding generation failed AND rollback failed:', rollbackErr, err);
+ }
throw new Error(`Could not index "${fileName}": embeddings failed to generate. ${err instanceof Error ? err.message : String(err)}`);📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Comment on lines
+84
to
+85
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 4. Error cause not preserved The new catch block throws a fresh Error string, which drops the original error object/stack as a cause. This reduces debuggability (especially differentiating load vs native embedding failures) even though the UI surfaces indexErr.message. Agent Prompt
|
||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| onProgress?.({ stage: 'done', message: 'Done' }); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3. Backfill mismatch not handled
🐞 Bug≡ CorrectnessAgent Prompt
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools