Repository navigation
fix: keep in-memory BM25 statistics coherent during concurrent mutations - #13674
bogdankostic merged 5 commits into
Conversation
|
@remote-controlled-man is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
Bot
added
topic:tests
type:documentation
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
| "Invalid filter syntax. See https://docs.haystack.deepset.ai/docs/metadata-filtering for details." | ||
| ) | ||
|
|
||
| @_synchronized |
There was a problem hiding this comment.
delete_by_filter and update_by_filter still scan self.storage outside the lock, so an overwrite landing mid scan blows up with RuntimeError: dictionary keys changed during iteration, and if it lands between the scan and delete_documents the new version gets deleted even though it no longer matches the filter. lock is reentrant so just wrapping both is enough
+ @_synchronized
def update_by_filter(self, filters: dict[str, Any], meta: dict[str, Any]) -> int:
@@
+ @_synchronized
def delete_by_filter(self, filters: dict[str, Any]) -> int:fixed this plus a test in this commit, you can just cherry pick it instead of redoing it: its-amann@203399a
There was a problem hiding this comment.
Thanks for catching this and providing the patch! I've cherry-picked your commit, preserving its authorship, and extended the regression to cover update_by_filter as well as delete_by_filter, with both shared and non-shared stores. Both methods now hold the index's reentrant lock throughout selection and mutation, including the nested delete_documents call.
All four new regression cases failed against the previous PR head and pass with the fix. On 1202fc9, the complete in-memory store module passed (416 passed, 8 existing optional skips), as did the BM25 retriever unit tests (19) and pipeline integration tests (5), scoped type/format checks, and pre-commit hooks. The class documentation, release note and PR description now include both filter methods in the synchronization contract. Remote CI remains to run on the updated head.
bogdankostic
left a comment
bogdankostic
left a comment
There was a problem hiding this comment.
Thanks for working on this @remote-controlled-man!
I pushed a commit that also puts the remaining read methods (filter_documents, embedding_retrieval, save_to_disk and the count/metadata methods) under the same lock, since they could still fail when a write landed mid-scan.
Related Issues
No existing issue found for this race.
Proposed Changes:
Concurrent operations on an
InMemoryDocumentStorecan publish a document before its BM25 statistics are ready. An overlapping overwrite can leave the final content asbetawhile its statistics describealpha, so searching for the stored content returns nothing. Deleting or clearing during a write can also leave orphan statistics and change later scores. Concurrent filter-based mutations can raiseRuntimeError: dictionary keys changed during iterationor operate on a replacement document that no longer matches the filter.Serialize
write_documents,delete_documents,delete_all_documents,delete_by_filter,update_by_filterandbm25_retrievalwith a reentrant lock owned by the index. The filter-based operations hold the lock from selection through mutation. Shared store instances use the same lock; non-shared stores keep their own. Reentrancy preserves the existing overwrite-to-delete and filter-to-delete call paths. Their async methods delegate to these protected methods, and independent indexes continue to run concurrently.This incorporates Aman Agnihotri's supplied fix, preserving its authorship, and extends its regression to cover both filter mutation methods.
How did you test it?
087b6fe, all four new filter-mutation cases failed with the dictionary-iteration error. With the follow-up fix, all four passed, covering deletion and metadata updates with both shared and non-shared stores.hatch run test:unit test/document_stores/test_in_memory.py: 416 passed, 8 skipped. The skips are existing optional index-recreation conformance cases; this backend has norecreate_index/recreate_collectionparameter.hatch run test:unit test/components/retrievers/test_in_memory_bm25_retriever.py: 19 passed, 5 integration cases deselected.hatch run test:integration test/components/retrievers/test_in_memory_bm25_retriever.py: 5 passed, 19 unit cases deselected.hatch run test:typesandhatch run fmt-checkon the two Python files passed. All applicable pre-commit hooks passed on all three changed files.All final checks ran on Python 3.12.14 and commit
1202fc9. Full CI matrices, whole-tree checks, API-reference generation and Reno lint remain for CI; no remote CI result is claimed here.Notes for the reviewer
The original code and tests were generated with OpenAI Codex. This follow-up incorporates Aman's supplied patch and a Codex-authored test extension. Codex ran the reported checks; a separate Codex agent reviewed the frozen change.
The lock serializes the six methods listed above for the same index. Other read APIs and direct mutation of returned
Documentobjects are outside this synchronization contract.Checklist