fix(query-engine): exact-window queries take a read lock, not write - #611
Conversation
MutableEpoch::exact_query needed &mut self solely to lazily build/cache its window_to_ids offset index, forcing per_key.rs's exact-query call site to take a full write lock on the per-aggregation shard even though the queried data itself isn't mutated. That serialized exact queries against every other concurrent reader/writer of the shard, unlike range queries, which only ever needed a read lock. window_to_ids now lives behind its own RwLock, independent of the outer shard lock, so exact_query can take &self and build the index lazily under double-checked locking. insert/remove_windows still invalidate it at zero cost via get_mut() (no lock acquired, since &mut self already proves exclusive access). per_key.rs's call site drops to .read(); global.rs is updated to compile against the new signature but has no locking change since it uses a single Mutex with no read/write split. Test coverage was designed by a separate agent given only the problem description (not this fix), working from an isolated worktree at the pre-fix commit, to avoid biasing the tests toward this specific implementation: - Correctness tests confirm exact_query's lazy cache never returns stale/incorrect results across inserts, repeated calls, misses, and epoch rotation. - A concurrency test forces an index rebuild on a large shard and measures whether a concurrent range query on the same shard blocks for the rebuild's full duration; pre-fix it reliably did (ratio ~1.00 across repeated runs), post-fix it does not. Fixes #607. Co-Authored-By: Claude Sonnet 5 <[email protected]>
Code ReviewReviewed the core change (scoping Two minor findings on the new inner
🤖 Generated with Claude Code |
Code review on #611 flagged two issues with the RwLock<Option<HashMap>> approach: - It reimplements double-checked locking that std::sync::OnceLock already provides via get_or_init. - It introduces a poisoning-cascade not present before the PR: a panic while holding the inner write lock would poison window_to_ids specifically, and every subsequent exact_query on that epoch would then panic on .unwrap() forever (until the epoch reseals), unlike the outer per-shard lock's poisoning, which call sites already convert gracefully into a Result::Err. OnceLock has no poisoning concept: if the init closure ever panicked, the cell would simply stay uninitialized for the next caller to retry. Switching to it removes both the reimplemented locking logic and the new failure mode in one change. Invalidation on insert/remove_windows now uses OnceLock::take() (still through &mut self, still no synchronization needed) instead of writing None through a lock guard. Co-Authored-By: Claude Sonnet 5 <[email protected]>
- INDEX_DESIGN.md still documented the pre-fix write-lock/&mut-self exact_query design (Option<HashMap>, write lock in the concurrency table and query-mechanics walkthrough). Updated to describe the OnceLock-based design and the read-lock-only concurrency table, with a note on why exact queries no longer need a write lock. - test_exact_query_does_not_block_concurrent_range_queries_per_key computed max_reader_latency_ns but never asserted it was actually touched. If every reader thread got starved of scheduler time during the timed window, the value would stay at its initial 0 and the ratio assertion (0 <= anything) would pass vacuously, having measured no contention at all. Added an explicit assertion that readers recorded at least one iteration each and that max_reader_latency_ns is nonzero before trusting the ratio. Co-Authored-By: Claude Sonnet 5 <[email protected]>
|
lgtm |
Summary
MutableEpoch::exact_queryneeded&mut selfsolely to lazily build/cache itswindow_to_idsoffset index, forcingper_key.rs's exact-query call site to take a full write lock on the per-aggregation shard even though the queried data itself isn't mutated — serializing exact-window queries against every other concurrent reader/writer of that shard.window_to_idsnow lives behind its ownRwLock, independent of the outer shard lock.exact_querytakes&selfand builds the index lazily under double-checked locking (read fast path; write + re-check slow path).insert/remove_windowsstill invalidate it at zero cost viaget_mut()— no lock acquired, since&mut selfalready proves exclusive access there.per_key.rs's call site drops from.write()to.read()— this is the actual concurrency fix (realRwLock-backed shard, the defaultLockStrategy).global.rsis updated to compile against the new signature (get_mut→get) but has no locking change — it uses a singleMutexfor the whole store with no read/write split, so there's nothing to fix there.legacy/store implementations are untouched — confirmed dead code (unreachable viaLockStrategy, no references anywhere outsidelegacy/itself).Testing
Test coverage was designed by a separate agent given only the problem description (not this fix), working from an isolated worktree checked out at the pre-fix commit, specifically to avoid biasing the tests toward this implementation's shape.
run_contract_suite(run against bothLockStrategy::PerKeyand::Global): stability across repeated calls, no shadowing of a later insert by an earlier cache build, miss-then-hit after insert, 900 interleaved insert/query rounds checked by value, and correctness across epoch rotation.test_exact_query_does_not_block_concurrent_range_queries_per_key), written as a TDD red test asserting the desired fixed behavior: a concurrent range query's max latency should stay under 20% of a forced index-rebuild's duration. Pre-fix, this failed cleanly and consistently (reader latency ≈ exact-query duration, ratio ~1.00 across repeated runs) — a reader visibly blocked for the full write-lock hold. Post-fix, it passes.Fixes #607.
Test plan
cargo test -p query_engine_rust --lib— 576 passed, 0 failedcargo test -p query_engine_rust --lib store_correctness_tests—contract_per_key,contract_global, and the new concurrency test all passcargo fmt/cargo clippyclean (pre-commit hooks passed)🤖 Generated with Claude Code