fix(rag): roll back + surface a document that fails to embed (no silent dead KB entries)#452
Closed
alichherawalla wants to merge 1 commit into
Closed
fix(rag): roll back + surface a document that fails to embed (no silent dead KB entries)#452alichherawalla wants to merge 1 commit into
alichherawalla wants to merge 1 commit into
Conversation
…nt dead entries) indexDocument caught embedding failure non-fatally and still returned success, so a document with ZERO embeddings showed as 'added' in the Knowledge Base but was not searchable (semantic search skips it) — 'looks indexed but doesn't actually work'. backfillEmbeddings has no auto-trigger, so the swallowed failure stranded the doc permanently. Now: on any embedding failure (load throw, embedBatch throw/OOM, or an embedding-count mismatch), roll back the just-inserted document + chunks and throw. KnowledgeBaseScreen already alerts on an index throw, so the user sees the real error and can retry instead of a silent non-functional entry. Tests: replace the 'continues without embeddings' test (which encoded the lie) with three fails-before/passes-after cases — load failure, embedBatch failure, and embedding-count mismatch all roll back (deleteDocument called) and throw, with insertEmbeddingsBatch never called. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughRagService.indexDocument now validates that the number of generated embeddings matches chunk count and, on any embedding-related failure or mismatch, deletes the just-inserted document and rethrows an error instead of silently continuing. Unit tests updated to verify rollback behavior in these failure scenarios. ChangesEmbedding Rollback in RagService
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant RagService
participant EmbeddingService
participant RagDatabase
Caller->>RagService: indexDocument(doc)
RagService->>RagDatabase: insert document
RagService->>EmbeddingService: embedBatch(chunks)
EmbeddingService-->>RagService: embeddings or error
alt embedding failure or count mismatch
RagService->>RagDatabase: deleteDocument(docId)
RagService-->>Caller: throw indexing error
else success
RagService->>RagDatabase: insertEmbeddingsBatch(embeddings)
RagService-->>Caller: success
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Collaborator
Author
Self-auditSOLID / abstraction
Tests — no false green
Provit (on-device E2E)
Platform parity
Standards
|
PR Summary by QodoFix RAG indexing: rollback and throw on embedding failures (no dead KB entries)
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
![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
🤖 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 `@src/services/rag/index.ts`:
- Around line 83-85: The rollback in the RAG indexing failure path can mask the
original embedding error if `ragDatabase.deleteDocument(docId)` throws,
preventing `logger.error` and the wrapped `throw` in the `index` flow from
running. Update the error handling around `ragDatabase.deleteDocument` in
`src/services/rag/index.ts` so rollback failures are caught and logged
separately, while the original embedding failure is always the one rethrown from
the `index` function with its cause preserved.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1296a0b5-1d3d-44c4-9e9f-5eee5d982f02
📒 Files selected for processing (2)
__tests__/unit/services/rag/index.test.tssrc/services/rag/index.ts
Comment on lines
+83
to
+85
| 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)}`); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the rollback delete so it can't mask the original failure.
If ragDatabase.deleteDocument(docId) throws, the DB error propagates instead of the embedding error, and logger.error / the wrapped throw never run — leaving exactly the stranded, undiagnosable entry this change is meant to prevent. Wrap the rollback so the underlying embedding cause is always surfaced.
🛡️ 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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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)}`); | |
| 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)}`); |
🤖 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 `@src/services/rag/index.ts` around lines 83 - 85, The rollback in the RAG
indexing failure path can mask the original embedding error if
`ragDatabase.deleteDocument(docId)` throws, preventing `logger.error` and the
wrapped `throw` in the `index` flow from running. Update the error handling
around `ragDatabase.deleteDocument` in `src/services/rag/index.ts` so rollback
failures are caught and logged separately, while the original embedding failure
is always the one rethrown from the `index` function with its cause preserved.
Code Review by Qodo
Context used✅ Compliance rules (platform):
25 rules 1. Backfill mismatch not handled
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+66
to
+68
| if (embeddings.length !== rowIds.length) { | ||
| throw new Error(`embedding count ${embeddings.length} does not match ${rowIds.length} chunks`); | ||
| } |
There was a problem hiding this comment.
3. Backfill mismatch not handled 🐞 Bug ≡ Correctness
indexDocument now rejects when the embedding count doesn’t match chunk count, but backfillEmbeddings still blindly uses embeddings[i] for each chunk. If an embedding-count mismatch ever occurs (as the new guard suggests is possible), backfill can store empty vectors and semantic retrieval can produce NaN scores.
Agent Prompt
### Issue description
`indexDocument()` now enforces `embeddings.length === rowIds.length`, but `backfillEmbeddings()` does not validate the returned embedding count before indexing `embeddings[i]`. If `embedBatch()` returns fewer embeddings than chunks, `embeddingToBlob(undefined)` will produce an empty embedding blob, and semantic scoring can become `NaN`.
### Issue Context
- `backfillEmbeddings()` builds entries with `embedding: embeddings[i]` without bounds checks.
- `embeddingToBlob()` uses `new Float32Array(embedding)`; when `embedding` is `undefined` it results in a zero-length buffer.
- `cosineSimilarity()` multiplies `a[i] * b[i]` for `i < a.length`; with `b` shorter, this yields `NaN` and unstable sorting.
### Fix Focus Areas
- src/services/rag/index.ts[93-119]
- src/services/rag/database.ts[114-116]
- src/services/rag/vectorMath.ts[9-21]
### Suggested change
Apply the same length check used in `indexDocument()` inside `backfillEmbeddings()`. On mismatch, skip inserting embeddings (and consider logging + continuing), or delete any partially inserted embeddings for that doc if you ever change insertion semantics.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+84
to
+85
| 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)}`); |
There was a problem hiding this comment.
4. Error cause not preserved 🐞 Bug ◔ Observability
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
### Issue description
`indexDocument()` rethrows embedding failures with a new `Error(...)` message, losing the original error as a structured cause. This makes diagnosing failures harder (stack traces, error typing) even though the message is preserved.
### Issue Context
Callers (e.g. KnowledgeBaseScreen) display `indexErr.message`, but logs/telemetry benefit from retaining the original error via `cause`.
### Fix Focus Areas
- src/services/rag/index.ts[76-86]
- src/screens/KnowledgeBaseScreen.tsx[94-100]
### Suggested change
Use `throw new Error(msg, { cause: err })` (if supported by your TS/JS target) or attach `(e as any).cause = err` before throwing. Keep the user-facing message, but preserve the original error for diagnostics.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Collaborator
Author
|
Closing — bundling the fix into #510 (refactor/parse-once-boundary). A falsifying red-flow test for this bug is committed on that branch (integration-level, mocks only the native boundary; verified red on HEAD, flips green when the fix lands). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
indexDocumentcaught embedding failure non-fatally and still returned success. A document with zero embeddings then showed as "added" in the Knowledge Base but was not searchable (semantic search skips it) — the "looks indexed but doesn't actually work" lie.backfillEmbeddingshas no auto-trigger, so the swallowed failure stranded the doc permanently.Fix
On any embedding failure (load throw,
embedBatchthrow/OOM, or an embedding-count vs chunk-count mismatch), roll back the just-inserted document + chunks and throw.KnowledgeBaseScreenalready alerts on an index throw, so the user sees the real error and can retry — never a silent non-functional entry.Tests (jest, fails-before / passes-after)
Replaced the old
continues without embeddings if embedding failstest (which encoded the lie) with three cases: load failure,embedBatchfailure (OOM), and embedding-count mismatch — each assertsindexDocumentthrows and rolls back (deleteDocumentcalled), withinsertEmbeddingsBatchnever called.Provit (on-device E2E)
Pending — a KB round-trip journey (index a real doc → confirm searchable; feed a doc that fails to embed → confirm the error surfaces, no silent dead entry). jest mocks the sqlite/native boundary, so the true round-trip proof is on-device. Self-audit comment below.
Scope
Fixes the "not actually working" half of the KB honesty issue. The truncation-honesty half (>500KB silently cut) is a separate follow-up PR.
🤖 Generated with Claude Code
Summary by CodeRabbit