Speed up the SQLite metadata-cache build (connection reuse + synchronous=NORMAL)#251
Conversation
Paired with the existing WAL journal mode, synchronous=NORMAL skips an fsync on every commit while preserving durability across application crashes (only a power/OS crash can lose the last committed transaction). The SQLite cache is fully regenerable from the JSONL source, so that residual risk is acceptable. Measured: ~6x faster on a commit-bound microbenchmark (300 commits: 363ms -> 57ms). NOTE the real-build impact is modest: a full single-project build issues only ~47 commits (~169ms total fsync), so this saves ~145ms. The dominant cache overhead is connection churn (~189 sqlite3.connect calls per build), addressed separately. Adds a test pinning synchronous=NORMAL alongside the WAL-mode test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A full single-project build issued ~189 sqlite3.connect() calls — every CacheManager method (reads included) opened its own connection. That connection churn, not fsync, is the dominant cache-build cost (~3.9ms/open). Add an opt-in batch() context manager that opens one shared connection, has _get_connection() reuse it, and closes it on scope exit (including on exception, via finally). Wrap the converter's build hotspots in batch(): - ensure_fresh_cache: invalidation reads + populate (load + session/ aggregate writes) share one connection. - load_directory_transcripts: the per-file load loop. - _generate_individual_session_files: the per-session staleness checks, html_cache writes, and per-session renderer cache reads. Nesting is a no-op reuse (inner batch leaves the outer connection's lifecycle to the outermost scope), so these compose safely — the process-hierarchy path drives per-project builds through convert_jsonl_to and inherits the reuse. Result: connection opens for a single-project build drop ~189 -> ~14 (93%). Outside a batch the default is unchanged (connection-per-call), which keeps the Windows-safe no-lingering-handle behaviour: batch() closes the shared connection before any temp-dir teardown / clear_cache rmtree (WinError 32). Integrity tests pin reuse, normal+exception close, nesting, write-persistence, and the rmtree-after-build case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Record the as-built connection lifecycle in the cache deep-dive: connection-per-call default (Windows-safe, no lingering handle), synchronous=NORMAL durability tradeoff, and the batch() connection reuse the converter uses for build hotspots. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Harden the field ordering so _shared_conn always exists before any _get_connection() could run, even if migrations are ever routed through it. Currently safe (migrations use the standalone run_migrations), but this removes a latent AttributeError footgun. Per monk review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
Batched SQLite Connection Reuse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
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 `@claude_code_log/cache.py`:
- Around line 346-347: The SQLite connection setup in `_get_connection()` and
the `batch()` path opens a handle before calling `_configure_connection()`, but
if configuration fails the connection is left open and can lock the DB/WAL
files. Update the setup flow around `sqlite3.connect()` and
`_configure_connection()` to use a close-on-failure path so any exception during
initialization closes the connection before re-raising.
🪄 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: 7faf2dbd-9d8e-457c-85f0-574aeffac017
📒 Files selected for processing (4)
claude_code_log/cache.pyclaude_code_log/converter.pydev-docs/application_model.mdtest/test_cache_sqlite_integrity.py
_get_connection() and batch() opened the SQLite handle before applying pragmas, so a failure in _configure_connection() would leak the connection and could lock the .db/.db-wal/.db-shm files. Add _open_configured_connection(), which closes the handle before re-raising if setup fails, and use it in both paths. batch() now only publishes to _shared_conn after setup succeeds, so a failed configure never leaves a half-initialised handle for reuse. Add a test asserting the connection is closed (and _shared_conn stays None) when _configure_connection raises, in both paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Part of #248.
Problem
Building the SQLite metadata cache dominates per-project processing.
Instrumenting a real (contention-immune) build showed the cost is
connection churn, not fsync: every
CacheManagermethod (readsincluded) opened its own
sqlite3.connect+ PRAGMAs — ~189 connectionopens per single-project build (~3.9ms each), against only 47 commits.
(An earlier "fsync is the bottleneck" reading turned out to be a
measurement artifact from toggling
use_cacheunder load.)Change
PRAGMA synchronous=NORMALon cache connections (WAL). The cacheis regenerable, so the durability trade-off — only an OS/power crash
can lose the last commit, not an app crash — is appropriate.
CacheManager.batch()— an opt-in context that reuses oneconnection for the duration of a build instead of opening ~189. The
converter wraps each per-project build in it. Outside a batch,
behaviour is byte-for-byte unchanged (open-per-call), preserving the
Windows file-lock semantics the per-call design relied on.
nullcontextkeeps the no-cache path identical; nested batches are ano-op reuse).
Why the open-per-call default is preserved
A persistent connection holds
.db/.db-wal/.db-shmopen, which onWindows breaks
TemporaryDirectorycleanup andclear_cachermtree(
WinError 32).batch()therefore closes the shared connection atscope end (and on exception) before any teardown can run. This is
pinned by tests in
test_cache_sqlite_integrity.py, not just asserted:closes
connection leaks the lock)
Results (local Windows)
Cache invalidation (mtime), schema-version rebuild, and concurrent-reader
safety are unchanged and covered by the integrity tests and the existing
cache suite. The connection model is documented in
dev-docs/application_model.md.Unlike the test-only companions in #248, this is a change to production
code that speeds up real cache builds on every platform (Windows most,
since connection/file overhead is heaviest there).
Summary by CodeRabbit
New Features
Bug Fixes
Tests