ENH: use SQLite for the directory index map - #764
Conversation
|
@codex review |
📝 WalkthroughWalkthroughThe directory index-location cache now uses SQLite instead of JSON. The indexer initializes and repairs corrupted databases, performs transactional upserts, handles concurrent recovery, and reads external changes. Configuration, fixtures, and tests now target SQLite behavior. ChangesSQLite index-map cache
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c12800141
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dascore/io/index/indexer.py (1)
158-166: 🚀 Performance & Scalability | 🔵 TrivialExclusive recovery lock serializes all reads, not just recovery.
Every
_get_index_map/_update_index_mapacquires the in-process_INDEX_MAP_RECOVERY_LOCKand an exclusive OS file lock through_index_map_recovery_guard, so concurrent reads are fully serialized across threads and processes even when no recovery is in progress. The inline comment (Lines 162-165) documents this is deliberate to keep readers away from the unlink/rebuild window, and given how infrequently the map is touched this is a reasonable safety-over-throughput tradeoff.If contention ever shows up on a hot path, consider a shared-lock-for-normal-access / exclusive-lock-for-recovery scheme (lock upgrade) so reads can proceed concurrently. No change needed for now.
🤖 Prompt for 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. In `@dascore/io/index/indexer.py` around lines 158 - 166, The existing exclusive locking in _run_index_map_operation and _index_map_recovery_guard is intentional; make no code changes. Preserve serialization of reads and updates across threads and processes to protect the database recovery window.
🤖 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.
Nitpick comments:
In `@dascore/io/index/indexer.py`:
- Around line 158-166: The existing exclusive locking in
_run_index_map_operation and _index_map_recovery_guard is intentional; make no
code changes. Preserve serialization of reads and updates across threads and
processes to protect the database recovery window.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1790dad0-8e48-4452-a5e2-7509f11fc87d
📒 Files selected for processing (5)
dascore/config.pydascore/io/index/indexer.pytests/conftest.pytests/test_io/test_index/test_index_edge_cases.pytests/test_io/test_indexer.py
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ac739f63c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a187cbf2d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #764 +/- ##
===========================================
+ Coverage 48.75% 99.91% +51.16%
===========================================
Files 163 163
Lines 17081 17330 +249
===========================================
+ Hits 8327 17316 +8989
+ Misses 8754 14 -8740
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Superseded by #772, which takes a simpler approach: one small JSON file per data directory instead of a SQLite database with corruption-recovery locks and |
|
superseded by #772 |
1 participant
Description
This is the first focused extraction from the free-threaded integration PR #763. It replaces the unreleased JSON directory index map on
devwith a small SQLite database.The map now uses transactional upserts with
BEGIN IMMEDIATE, per-operation connections, and a busy timeout so concurrent writers preserve every directory entry. Corrupt disposable maps are rebuilt under a content-independent advisory lock that coordinates threads and processes around destructive recovery.The indexer retains its local-index fallback when the optional global map is unavailable. Read-only shared caches can reuse existing mappings through SQLite read-only mode, while corrupt read-only optional maps degrade safely for writable data directories. POSIX at-fork callbacks also wait for an active recovery guard to finish before forking, so a child cannot inherit process-local or advisory file-lock state. Platforms without
fcntl, including relevant Emscripten builds, retain the single-process lock path.This is an intentional clean break from the unreleased JSON format. Sibling
cache_paths.jsonfiles are ignored and left untouched; the configured path always names the SQLite database.PR #763 remains open as the integration reference and is not closed by this extraction.
Validation
PYTHON_GIL=0: 145 affected tests passedPYTHON_GIL=1: 145 affected tests passedTestIndexMapwithDeprecationWarningtreated as an error: 18 passed in each GIL modeChecklist
I have (if applicable):