Skip to content

fix: reject non-finite FAISS vectors and preserve upload diagnostics - #10139

Open
KITE919 wants to merge 5 commits into
AstrBotDevs:masterfrom
KITE919:fix/10109-knowledge-base-index-error
Open

KITE919 wants to merge 5 commits into
AstrBotDevs:masterfrom
KITE919:fix/10109-knowledge-base-index-error

Conversation

@KITE919

@KITE919 KITE919 commented Sep 19, 2026 •

Copy link
Copy Markdown

Related to #10109: knowledge-base uploads can surface a generic index-write error with little diagnostic information. This PR rejects malformed embeddings and preserves storage diagnostics. The original issue does not include enough evidence to confirm NaN/Inf as its root cause.

Modifications

  • Reject NaN/Inf embeddings in FaissVecDB.insert_batch() before document or FAISS writes; do not silently replace values with zero.
  • Validate vectors at the EmbeddingStorage.insert(), insert_batch(), and search() boundaries.
  • Roll back partial document/vector inserts when internal-ID counts mismatch or FAISS insertion fails.
  • Preserve the underlying exception and traceback in structured KnowledgeBaseUploadError diagnostics.
  • Add regression coverage for rejected vectors, mismatched internal-ID counts (empty, too few, and too many), and cleanup/diagnostics on FAISS failure.
  • This is not a breaking change. Valid-vector behavior and public signatures are unchanged; malformed vectors now fail explicitly.

Verification Steps and Test Results

Validated locally on macOS with Python 3.12 on October 21, 2026:

uv run pytest tests/unit/test_faiss_vec_db.py -q
15 passed

uv run ruff check .
All checks passed!

uv run ruff format --check .
513 files already formatted

git diff --check
Passed

These are focused local tests; the full application, dashboard, and remote CI suites have not been validated by this run. The local test run reports dependency deprecation/syntax warnings.

The five previous GitHub Actions workflows did not execute: their summaries report This workflow run required approval but was not approved before it expired. Repository-maintainer approval is required for remote workflow execution. The previous Sourcery review approved commit 06710a2; it is automated review, not human approval.

Checklist

  • No new feature is introduced; feature-discussion requirement is not applicable.
  • Focused regression tests and repository-wide Ruff checks pass; commands/results are listed above. Full remote CI remains pending maintainer approval.
  • No WebUI navigation, page structure, or labels change, so no WebUI documentation or screenshot update is needed.
  • No new project dependencies are introduced.
  • Changes do not introduce malicious code.

AI assistance was used for implementation review and regression-test preparation; the listed checks were executed locally.

Fixes AstrBotDevs#10109: wrap FAISS insert failures as structured KnowledgeBaseUploadError
with cause + traceback; clamp non-finite embedding values (NaN/Inf) to 0.0
before passing to FAISS C++ backend.
fix: reject non-finite vectors early in EmbeddingStorage

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 2 issues

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="astrbot/core/db/vec_db/faiss_impl/vec_db.py" line_range="220-221" />
<code_context>
             await self.embedding_storage.insert_batch(vectors_array, int_ids)
-        except Exception:
-            await self._rollback_partial_insert(ids=ids, int_ids=int_ids)
+        except KnowledgeBaseUploadError:
             raise
+        except Exception as _faiss_err:
+            # Low-level FAISS errors (index corruption, resource exhaustion,
</code_context>
<issue_to_address>
**issue (bug_risk):** The internal-ID count mismatch raises `KnowledgeBaseUploadError` inside the transaction-compensation block, but the new `except KnowledgeBaseUploadError` re-raises it without calling `_rollback_partial_insert`. Document rows already committed by `insert_documents_batch` remain orphaned whenever the returned ID count is wrong.

**Triggers:** When `DocumentStorage.insert_documents_batch()` returns fewer or more IDs than the content count.

**Suggested fix:** Roll back partial inserts before re-raising this error, or move the ID-count validation outside the `try` and explicitly compensate on failure.

```suggestion
        except KnowledgeBaseUploadError:
            await self._rollback_partial_insert(ids=ids, int_ids=int_ids)
            raise
```
</issue_to_address>

### Comment 2
<location path="astrbot/core/db/vec_db/faiss_impl/embedding_storage.py" line_range="149-153" />
<code_context>
             )
+        # Validate vector values before FAISS write; non-finite values cause
+        # C++ segfaults or silent index corruption.
+        if not np.all(np.isfinite(vectors)):
+            nan_count = int(np.sum(~np.isfinite(vectors)))
+            await self.save_index()
+            raise RuntimeError(
+                f"向量包含 {nan_count} 个非有限值 (NaN/Inf),无法写入 FAISS 索引。请检查嵌入模型配置。"
+            )
         if vectors.shape[1] != self.dimension:
</code_context>
<issue_to_address>
**issue (broader_impact):** Finite-value validation is added only to `insert_batch`; `EmbeddingStorage.search()` still passes NaN/Inf query vectors directly to FAISS and returns invalid distances or neighbor IDs instead of rejecting the malformed vector. `EmbeddingStorage.insert()` likewise remains an unvalidated FAISS boundary for single-vector inserts.

**Triggers:** When a query or single-document embedding contains NaN or Inf.

**Suggested fix:** Apply the same `np.isfinite()` validation to `search()` and `insert()` before invoking FAISS.
</issue_to_address>

Sourcery assessment

Needs a human reviewer. 2 findings to address first, and if malformed embeddings are clamped incorrectly, zero-valued vectors can be persisted in the FAISS index and produce bad retrieval results even after reverting the code. The affected index can be rebuilt or the documents re-embedded, so the damage is bounded and repairable.

Blocking findings: astrbot/core/db/vec_db/faiss_impl/vec_db.py:221, astrbot/core/db/vec_db/faiss_impl/embedding_storage.py:153


Sourcery is free for open source - if you like our reviews please consider sharing them ✨

Comment thread astrbot/core/db/vec_db/faiss_impl/vec_db.py
Comment thread astrbot/core/db/vec_db/faiss_impl/embedding_storage.py
- Roll back partial inserts in except KnowledgeBaseUploadError block to
  prevent orphaned document rows when DocumentStorage returns mismatched ID count
- Add isfinite() validation to EmbeddingStorage.insert() and search() to
  reject NaN/Inf vectors before FAISS C++ layer sees them
sourcery-ai[bot]
sourcery-ai Bot previously approved these changes Sep 20, 2026

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sourcery assessment

Approved.

@KITE919

KITE919 commented Sep 20, 2026

Copy link
Copy Markdown
Author

Follow-up fix pushed in 06710a2: non-finite embeddings are now rejected before document/FAISS writes instead of being silently clamped to zero; single insert, batch insert, and search remain guarded, and focused regression tests were added. The unnecessary save_index call on rejected batch input was also removed.

@sourcery-ai
sourcery-ai Bot dismissed their stale review October 21, 2026 10:23

Sourcery withdrew this approval because the latest commits introduced blocking findings.

@KITE919 KITE919 changed the title fix: robust error handling and NaN clamping in FAISS vector store fix: reject non-finite FAISS vectors and preserve upload diagnostics Oct 21, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant