fix(_memory): make interrupted reindex resumable, verified by model identity - #7
Closed
omar-nahhas wants to merge 2 commits into
Closed
fix(_memory): make interrupted reindex resumable, verified by model identity#7omar-nahhas wants to merge 2 commits into
omar-nahhas wants to merge 2 commits into
Conversation
…dentity Memory.initialize() used to embed every document in one pass when a reindex was needed (embedding model changed) and only persist the result at the very end. Interrupting that pass -- a crash, a restart, a slow or shared embeddings backend timing out mid-batch -- threw away every already-embedded document; the next attempt started over from zero. On a large memory store against a slow backend this can cost hours of redone work every time the process is interrupted. Reindexing now embeds in batches (Memory._REBUILD_BATCH_SIZE) and checkpoints to disk after each one (index.rebuilding.faiss/.pkl), so an interrupted rebuild resumes from where it left off instead of restarting. The checkpoint is not trusted blindly: it's tagged with the embedding model it was built under (index.rebuilding.json), and a resume is only used if that matches the model we're about to (re)index with. If the target model changed again while a rebuild was interrupted, the stale checkpoint is discarded and a fresh rebuild starts -- otherwise resuming would add new-model vectors into an old-model FAISS index, which fails a dimension assertion on the very next batch. Adds tests/test_memory_rebuild_resume.py covering the checkpoint save/load/clear helpers directly and Memory.initialize() end-to-end for both the resume-under-same-model and discard-under-changed-model cases.
…e ordering Two gaps flagged by review on the resumable-reindex fix: 1. The rebuild checkpoint's model-identity check only compared provider/name. A kwarg that changes the embedder's actual output (e.g. an OpenAI-style `dimensions` override, or an api_base pointed at a different backend) could change while provider/name stayed the same, letting a checkpoint built under the old kwargs get resumed into under the new ones. The signature now includes build_kwargs() (minus api_key, which is deliberately excluded since it's persisted to disk in the memory dir and a credential rotation alone isn't "a different model"). 2. The rebuild checkpoint was cleared right after the batch loop, before the final index/embedding.json were durably written. A crash in that gap left neither a valid final index nor a resumable checkpoint, reintroducing the full re-embed-from-scratch failure mode the checkpoint exists to prevent. Clearing now happens strictly after the final save succeeds. Adds 3 tests: api_key exclusion, kwarg-change rejection, and checkpoint-survives-a-crash-during-final-save.
Author
|
Closing — superseded by the equivalent PR against the true upstream (agent0ai#1792) and the merged fix in our private fork (NuevaNext/agent-zero#88, agent0ai#89). |
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
2 participants
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.
Summary
Fixes a real production issue hit today: a memory reindex (triggered when the configured embedding model changes) embedded every document in a single pass and only persisted the result at the very end. Interrupting that pass — a crash, a pod restart, a slow/shared embeddings backend timing out mid-batch — threw away every already-embedded document, forcing the next attempt to start over from zero. Against a slow or shared embeddings backend, this cost hours of redone work on every interruption.
Memory._REBUILD_BATCH_SIZE, 20) and checkpoints to disk after each batch (index.rebuilding.faiss/.pkl), so an interrupted rebuild resumes where it left off.index.rebuilding.json) and is only trusted if that matches the model we're about to (re)index with. If the target model changed again while a rebuild was interrupted, the stale checkpoint is discarded and a fresh rebuild starts — otherwise resuming would add new-model vectors into an old-model FAISS index, failing a dimension assertion on the next batch.Test plan
tests/test_memory_rebuild_resume.py— 9 tests covering the checkpoint save/load/clear helpers directly, plusMemory.initialize()end-to-end for both the resume-under-same-model and discard-under-changed-model cases. All pass.main:email_parser_test.pyhas a stale import,rate_limiter_test.pyneeds live OpenRouter credentials) — no regressions introduced by this change.🤖 Generated with Claude Code