docs: search-index lock-acquisition order and threading model#138
docs: search-index lock-acquisition order and threading model#138clean6378-max-it wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDocuments ChangesFTS search documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🤖 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 `@docs/architecture.md`:
- Line 156: Update the Publish description for _publish_active_index so it does
not claim unconditional atomic pointer replacement; explicitly mention the
pointer.write_text fallback and qualify the reader guarantee to acknowledge
possible incomplete pointers, or remove that fallback from the implementation if
atomic publication is required.
- Line 158: Update the architecture documentation to distinguish rebuild
isolation from index_locked classification: explain that rebuilding a new SQLite
file does not normally lock the old active database, while query_index_hits
still classifies sqlite3.OperationalError as index_locked for fallback handling.
Avoid implying that rebuild concurrency inherently causes the error, and clarify
this distinction for follow-up tests.
🪄 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: 315eb35c-5452-4512-8803-b4baaaa035be
📒 Files selected for processing (2)
README.mddocs/architecture.md
e4d8f3c to
10e6b86
Compare
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 `@README.md`:
- Line 84: Update the README guidance for
CLAUDE_CODE_CHAT_BROWSER_NO_SEARCH_INDEX so it states that disabling the index
avoids using or building index files, rather than implying that existing index
files are removed.
🪄 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: 9b567a1f-22b7-4ca1-ac0c-47b5289233d3
📒 Files selected for processing (2)
README.mddocs/architecture.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/architecture.md
|
|
||
| The design targets **one WSGI worker process** (for example `python app.py` or `gunicorn --workers 1`). That gives one in-process writer thread, one advisory-lock owner, and consistent in-memory usability cache per operator session. | ||
|
|
||
| See [Deployment — WSGI workers](../README.md#deployment-wsgi-workers) in the README for multi-worker warnings. |
There was a problem hiding this comment.
Broken anchor. This links to #deployment-wsgi-workers, but the README heading is #### Deployment - WSGI workers, which Github slugs to deployment--wsgi-workers. The single-hyphen link won't resolve.
Simplest fix: rename the README heading to #### Deployment: WSGI workers - it slugs to deployment-wsgi-workers, matching this link and drops the em-dash.
| > The server **refuses to start** if `--debug` is combined with a non-loopback `--host` (e.g. `0.0.0.0`). | ||
| > That check runs only when you start the app with **`python app.py`** (not via `flask run` or other WSGI entrypoints). | ||
|
|
||
| #### Deployment — WSGI workers |
There was a problem hiding this comment.
Em-dash in the heading, and it's why the cross-link at docs/architecture.md:194 is broken.
Suggest #### Deployment: WSGI workers - fixes both at once.
| - `integration-tests` — API integration subset + coverage artifact | ||
| - `js-tests` — `npm ci` + vitest | ||
|
|
||
| ## Search index (FTS5) — locks and threading |
There was a problem hiding this comment.
em-dash in the heading.
This one is internally consistent - the links at architecture.md:69 and README.md:78 already use the matching double-hyphen slug search-index-fts5--lock-and-threading - so if you de-em-dash this heading, update both of those links in the same pass or they'll break.
2 participants
Summary
docs/architecture.md.README.mdwarning against multi-worker Gunicorn/uWSGI on a shared index directory; point to single-worker orCLAUDE_CODE_CHAT_BROWSER_NO_SEARCH_INDEX=1.Docs-only (Week 30 item #6); defines the invariant for the follow-up concurrency suite (item #7).
Test plan
utils/search_index.py(lines 57–63, 187–219, 521, 641–693)Summary by CodeRabbit
GET /api/search?q=..., describing SQLite FTS5 usage when available and a live JSONL scan fallback when the index is missing, stale, or temporarily locked.