Repository navigation
Reload SQLite in-memory index state after a rollback (#362) - #367
Open
Bernhard Merkle (bmerkle) wants to merge 2 commits into
Open
Bernhard Merkle (bmerkle) wants to merge 2 commits into
Bernhard Merkle (bmerkle) wants to merge 2 commits into
Conversation
SqliteRelatedTermsFuzzy and SqliteMessageTextIndex mirror table rows in memory (VectorBase, terms list, added-terms set). A transaction rollback restored only the tables, so a retry on the same provider skipped the rolled-back terms as already present and kept vectors for rows that no longer existed. SqliteStorageProvider.__aexit__ now reloads both indexes from their tables after rolling back. The loading code moves from __init__ into a reload_from_db() method on each index. SqliteStorageProvider.clear() and SqliteMessageTextIndex.clear() also reset the in-memory state, which they previously left behind. Fixes microsoft#362 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused recovery changes align cached indexes with database state, with rollback-and-retry regression coverage and no identified blocking issues.
No open issues 🎉
What changed in this PR
Fixes #362 by restoring SQLite-backed in-memory indexes after transaction rollback, allowing retries on the same storage provider to persist and search terms correctly.
Changes:
- Reload fuzzy-term and message-text index state from the database after rollback.
- Clear cached index state alongside table data during clear and message-text deserialization operations.
- Add regression tests for rollback recovery and ingestion retries, and update documentation.
| File | Description |
|---|---|
tests/test_sqlitestore.py |
Tests index restoration and retry after rollback. |
tests/test_add_messages_with_indexing.py |
Tests ingestion retry after a late transaction failure. |
src/typeagent/storage/sqlite/related_terms_index.py |
Extracts reusable state reloading and narrows the fuzzy-index property type. |
src/typeagent/storage/sqlite/provider.py |
Reloads both indexes after rollback and clears fuzzy-index state on reset. |
src/typeagent/storage/sqlite/message_index.py |
Adds ordered vector reloading and clears vectors during reset and deserialization. |
src/typeagent/knowpro/conversation_base.py |
Updates the comment to describe rollback recovery and removes the resolved TODO. |
CHANGELOG.md |
Documents rollback recovery and index-clearing fixes. |
Review effort: Balanced
…contiguity - Tests for SqliteStorageProvider.clear() and SqliteMessageTextIndex.clear() / deserialize() resetting in-memory state, a streaming-path rollback and retry across batches, and a rollback on a fresh provider. - Lookup-based assertions that message text index vectors line up with the persisted index positions after a retry. - SqliteMessageTextIndex.reload_from_db() raises ValueError when index_position values are not contiguous from 0 instead of silently misaligning lookups; the fuzzy index raises instead of asserting on a NULL embedding. - Docstrings note the constructor also reloads and that fuzzy terms reload in table order; CHANGELOG describes both symptoms and the deserialize() change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This branch has not been deployed
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
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.
Fixes #362 (follow-up from the #360 review).
Problem
SqliteRelatedTermsFuzzykeeps_added_terms,_terms_listand aVectorBasenext to theRelatedTermsFuzzytable, andSqliteMessageTextIndexkeeps aVectorBasenext toMessageTextIndex. Both are updated together with the table writes. When the surrounding transaction rolled back,SqliteStorageProvider.__aexit__restored only the tables:lookup_termcould return terms that were not in the DB.Fix
SqliteRelatedTermsFuzzy.reload_from_db()andSqliteMessageTextIndex.reload_from_db(): the loading code that lived in__init__is now a method that first clears the in-memory state.__init__calls it.SqliteStorageProvider.__aexit__calls both afterrollback(). This is the "reload from the table after a rollback" option from the issue: it is always correct, has no extra bookkeeping on the success path, and costs the same as opening the database, only on the failure path.SqliteStorageProvider.clear()now also clears the fuzzy index's in-memory state, andSqliteMessageTextIndex.clear()/deserialize()clear theVectorBasetogether with the table. All three previously left stale vectors behind, so a later lookup could hit a ghost vector that maps to no row.SqliteMessageTextIndex.reload_from_db()orders byindex_positionand raisesValueErrorif the positions are not contiguous from 0. VectorBase row i must hold the embedding atindex_positioni; the old unorderedSELECTrelied on storage order. A database whose positions have gaps can only have been produced by theclear()/deserialize()bug above, and loading it silently misaligned lookups.SqliteRelatedTermsIndex.fuzzy_indexis typed asSqliteRelatedTermsFuzzy(it never returnsNone), so the provider can callreload_from_db()without a cast.conversation_base.pyis resolved and the comment updated.Tests
test_sqlite_rollback_reloads_in_memory_indexes: adds terms and messages inside a transaction that raises; checks the in-memory state matches the tables afterwards, and that a retry persists what the rollback dropped, with lookups confirming vectors line up with rows.test_sqlite_clear_resets_in_memory_indexes,test_sqlite_rollback_on_fresh_provider.test_add_messages_with_indexing_retry_after_rollback: end-to-end throughadd_messages_with_indexing, failing once inupdate_conversation_timestamps(after the indexes were updated), then retrying.test_streaming_retry_after_rollback_of_later_batch: same throughadd_messages_streamingwith the first batch committed and the second rolled back.TestSqliteMessageTextIndexInMemoryState:clear()anddeserialize()reset theVectorBase; non-contiguousindex_positionvalues are rejected.All new tests fail on
mainand pass with this change.makeis green (953 tests).🤖 Generated with Claude Code