test: search-index concurrency for rebuild, queries, and lock order#139
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds concurrency-focused search-index tests and updates background-worker lifecycle and query error handling. The suite covers rebuilds, background refreshes, lock ordering, locked-index reporting, legitimate empty results, live-scan fallback, and partial indexed results. ChangesSearch index concurrency validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_search_index_concurrency.py`:
- Around line 182-246: Update test_queries_during_background_refresh to use a
poll interval that guarantees multiple background refresh opportunities during
the mutator loop, and ensure the test cleanup stops and joins the background
worker before returning. Extend reset_background_for_tests or add test-local
cleanup using the existing background lifecycle symbols, while preserving the
reader/mutator assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6072df99-3db2-4c96-854c-cc61c7a49a60
📒 Files selected for processing (1)
tests/test_search_index_concurrency.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@utils/search_index.py`:
- Around line 814-837: The reset logic in the background worker lifecycle must
retain ownership while termination is unconfirmed. In utils/search_index.py
lines 814-837, when the thread remains alive after joining, keep
_background_started, _background_thread, and _background_lock_fd unchanged,
leave _background_stop set, and prevent replacement startup; only clear
ownership and release the lock after termination is confirmed. In
utils/search_index.py lines 702-711, synchronize thread startup with reset so a
published but not-yet-started worker cannot be orphaned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f938e7d1-064b-4bf7-803c-3631b5f7a3c8
📒 Files selected for processing (4)
tests/conftest.pytests/test_search_index.pytests/test_search_index_concurrency.pyutils/search_index.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_search_index_concurrency.py
|
"Tests-only goal" / "does not change production search-index code" is inaccurate: utils/search_index.py is +58/-53 with a real background-worker lifecycle change (cooperative stop Event + thread creation moved inside _index_lock). Please reword to reflect the production race fix. |
updated PR description now |
3 participants
Closes #136
Sprint #7 adds tests/test_search_index_concurrency.py and changes utils/search_index.py so the background refresh worker can shut down cleanly in tests. The worker gets a cooperative stop event, reset_background_for_tests joins the daemon thread, and startup runs under _index_lock. Without that, full pytest could leave a search-index-refresh thread running. We hit that on Windows: lock-order tracing picked up events from a stray worker.
The suite exercises rebuild plus query overlap. One test runs multiple readers against a writer that keeps calling build_search_index(..., force=True), with a barrier each round. Another runs start_search_index_background while queries run and new session files appear; it checks that mutator-only terms show up in the index and that more than one refresh cycle runs. Lock nesting from #138 is recorded during a forced rebuild and checked against the doc (no index lock on the build path, usability only under the build lock), with a small negative control on the checker. Other cases cover index_locked on OperationalError, empty vs locked, live-scan fallback when the index is locked with no hits, and partial hits when a later batch sees lock contention. The stress test also counts reader rounds where the sentinel term actually appears in hits and fails if that count is zero, so it cannot pass as a no-crash test when every round is locked or query_ok is false.
After a join attempt (and a warning if the thread is still alive past the timeout), reset_background_for_tests clears _background_started, releases the background lock fd, and clears _background_stop so the next test can start a worker. write_session and index_patches moved into tests/conftest.py for reuse from test_search_index.py and the concurrency file.
Quick check: pytest tests/test_search_index_concurrency.py -q --no-cov (eight tests). CI full pytest is the real gate. Builds on #138; the suite should fail if lock nesting regresses.
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes / Improvements
Tests