Skip to content

fix(cli): surface reindex embedding failures and index identity - #1240

Merged
phernandez merged 2 commits into
mainfrom
fix/1237-reindex-error-surfacing
Aug 12, 2026
Merged

fix(cli): surface reindex embedding failures and index identity#1240
phernandez merged 2 commits into
mainfrom
fix/1237-reindex-error-surfacing

Conversation

@phernandez

Copy link
Copy Markdown
Member

Closes #1237. Found live during the Milvus tire-kick (#1235): reindex --embeddings
against a manifest owned by another vector index failed for all 226 entities, printed
"Reindex complete!", and exited 0 — with the instructive guard message visible only in
basic-memory.log.

What changed

  • VectorSyncBatchResult now carries vector_index, embedding_model, and
    sample_errors (first 3 distinct messages, normalized; recorded at every
    continue_on_error catch site including the orphan fail-safe).
  • The CLI embeddings summary states what was indexed and where:
    Embeddings complete (index=sqlite-vec, model=FastEmbedEmbeddingProvider:BAAI/bge-small-en-v1.5): ...
    and prints a single bounded Representative error: line when errors occurred.
  • Exit semantics: when every attempted entity failed across the selected projects, the
    command finishes its remaining phases (search reindex, other projects), prints
    Reindex failed: all vector embedding attempts failed., and exits 1. Partial failure
    stays exit 0 with the sample error shown.

Sample output for the tire-kick scenario:

done Embeddings complete (index=sqlite-vec, model=FastEmbedEmbeddingProvider:BAAI/bge-small-en-v1.5): 0 entities embedded, 0 skipped, 226 errors
Representative error: Cannot mutate vector manifests owned by external indexes ['milvus'] with configured adapter 'sqlite-vec'. Restore the owning adapter and retry.

Reindex failed: all vector embedding attempts failed.

Verification

  • New CLI tests (total failure → exit 1 + representative error; partial failure → exit 0
    • sample; success → index/model identity in output), sync-layer sample-error tests
      (dedupe, cap, orphan fail-safe), service propagation tests: 41 passed focused;
      1,116-test prescribed sweep passed in codex's run.
  • just typecheck, just lint: clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01CPdSXDbYyhyZ1TwgFnpEv8

Signed-off-by: phernandez <paul@basicmachines.co>
@phernandez phernandez added this to the v0.23 milestone Aug 11, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ecc85d2c2d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

# Trigger: every entity attempted across the selected projects failed to embed.
# Why: requested search work and other project summaries must still finish first.
# Outcome: the command preserves useful output but no longer reports false success.
if embedding_entities_total > 0 and embedding_errors_total == embedding_entities_total:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Exclude skipped entities from the all-failed denominator

When a selected project contains any opted-out or already-current entity, total_entities includes it while errors does not, so every entity that actually required embedding can fail and this comparison still exits 0. For example, one embed: false note plus one eligible note that hits the manifest-ownership guard yields total_entities=2, skipped=1, and errors=1, followed by Reindex complete!; base the failure decision on entities that required vector work rather than all database entities.

AGENTS.md reference: AGENTS.md:L132-L133

Useful? React with 👍 / 👎.

Comment on lines 521 to +522
if not entity_ids:
return VectorSyncBatchResult(
entities_total=0,
entities_synced=0,
entities_failed=0,
)
return await self.repository.sync_entity_vectors_batch([])

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid initializing the vector backend for an empty batch

When reindex --embeddings is run before any entities have been indexed, this new call reaches sync_entity_vectors_internal, which invokes _ensure_vector_tables() before checking the empty list; for an external index such as unavailable Milvus, that connects to or even creates the remote collection and can fail before the CLI prints its intended No indexed entities found guidance. Preserve index/model identity without executing backend synchronization when there is no work.

Useful? React with 👍 / 👎.

Signed-off-by: phernandez <paul@basicmachines.co>
@phernandez
phernandez merged commit c4da015 into main Aug 12, 2026
31 of 43 checks passed
@phernandez
phernandez deleted the fix/1237-reindex-error-surfacing branch August 12, 2026 02:48
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.

reindex --embeddings reports success (exit 0) when every entity fails to embed

1 participant