Fixes to locking mechanism#3
Merged
Merged
Conversation
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
1 participant
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.
Fix TOCTOU race and double lock acquisition in conversation query path
process_query_locally acquired state.index.read() twice for conversation queries that missed the conversation namespace — once to check the conv namespace, then again for the base fallback. Between the two acquisitions the lock was fully released, allowing the WAL flush task to write to the index between the two reads. This meant the two lookups could observe the index at different points in time: a concurrent insert into the conversation namespace could land in the window and be silently missed, returning scope: "global" when the entry was already in scope: "conversation".
Adds SemanticIndex::query_two_level, which runs both namespace lookups under a single &self borrow. The caller holds the read guard for the duration, so both checks see the same consistent snapshot. As a side effect this halves the lock acquisition count on the conversation miss path — under WAL flush write pressure each extra read().await is a scheduling stall.
The process_query_locally handler is updated to call query_two_level when a conversation_id is present. Six deterministic unit tests cover the new method: conv hit, base fallback, both miss, conv priority, exact-match through the two-level path, and dimension mismatch error propagation.