Reduce SQLite read contention: bound WAL, release vacuum lock, offload integrity check - #1692
Conversation
Investigation of reported DB read contention found two cross-process contention sources in the SQLite layer (single synchronous node:sqlite connection per process, WAL mode): - Unbounded WAL on central-db and archive-db. Neither set journal_size_limit, so their WAL never truncated back down after a checkpoint and every reader paid an ever-growing WAL-index scan. Add journal_size_limit=4MB (matching db.ts) plus explicit synchronous=FULL/wal_autocheckpoint=1000 for intent. central-db is the most cross-process-shared DB; archive-db had the same latent gap. - vacuum() held the EXCLUSIVE lock past its own runtime. Resetting locking_mode to NORMAL does not drop the WAL exclusive lock until the connection next touches the DB, so other processes stayed locked out of reads (SQLITE_BUSY) until some unrelated query ran. A plain read does NOT release it in WAL mode (verified); a PASSIVE checkpoint does. Run one in the finally, guard the locking_mode reset so it can't mask the original error or skip the release, and log swallowed failures. Tests: assert the new PRAGMAs on central-db and archive-db, and that a second connection can read immediately after vacuum() returns. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Ready to review this PR? Stage has broken it down into 2 individual chapters for you:
Chapters generated by Stage for commit 61c29cc on Jun 20, 2026 8:25pm UTC. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThree SQLite databases ( ChangesWAL Durability PRAGMAs and Vacuum Lock Release
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR addresses two cross-process SQLite read contention sources in WAL mode: unbounded WAL growth on
Confidence Score: 5/5Safe to merge. The changes are narrowly scoped to WAL configuration and vacuum lock-release logic, both well-covered by the new tests. The journal_size_limit additions are straightforward and additive — they only take effect at checkpoint time and cannot corrupt existing data. The vacuum() restructure is the most complex change: moving locking_mode=NORMAL and the PASSIVE checkpoint into individually-guarded try/catch blocks is correct, the finally block cannot throw and mask the original error, and the lock-release test with a zero-timeout second connection directly validates the fix. The afterBytes measurement moving after the checkpoint is intentional and covered by the existing test. No files require special attention. The vacuum() control flow in db.ts is the most intricate part of the change but is well-documented and tested. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[vacuum called] --> B[Measure beforeBytes]
B --> C[PRAGMA locking_mode=EXCLUSIVE]
C --> D{walCheckpoint TRUNCATE}
D -->|throws| E[Re-throw wrapped error]
D -->|ok| F{VACUUM exec}
F -->|throws| G[Re-throw wrapped error]
F -->|ok| H[finally block runs]
E --> H
G --> H
H --> I[try: locking_mode=NORMAL]
I -->|ok| K[try: wal_checkpoint PASSIVE]
I -->|throws| J[console.warn and continue]
J --> K
K -->|ok| M[Lock released immediately]
K -->|throws| L[console.warn - lock may linger until next write]
M --> N{Did outer try throw?}
L --> N
N -->|yes| O[Propagate original error to caller]
N -->|no| P[Measure afterBytes post-checkpoint]
P --> Q[Return VacuumResult to caller]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[vacuum called] --> B[Measure beforeBytes]
B --> C[PRAGMA locking_mode=EXCLUSIVE]
C --> D{walCheckpoint TRUNCATE}
D -->|throws| E[Re-throw wrapped error]
D -->|ok| F{VACUUM exec}
F -->|throws| G[Re-throw wrapped error]
F -->|ok| H[finally block runs]
E --> H
G --> H
H --> I[try: locking_mode=NORMAL]
I -->|ok| K[try: wal_checkpoint PASSIVE]
I -->|throws| J[console.warn and continue]
J --> K
K -->|ok| M[Lock released immediately]
K -->|throws| L[console.warn - lock may linger until next write]
M --> N{Did outer try throw?}
L --> N
N -->|yes| O[Propagate original error to caller]
N -->|no| P[Measure afterBytes post-checkpoint]
P --> Q[Return VacuumResult to caller]
Reviews (1): Last reviewed commit: "Reduce SQLite read contention: bound WAL..." | Re-trigger Greptile |
gsxdsm
changed the title
1 participant
What
Investigation of reported database read contention found, and this PR fixes, three sources of cross-process and event-loop contention in the SQLite layer (single synchronous
node:sqliteconnection per process, WAL mode). Not classic row-lock contention — WAL already allows concurrent readers — but three real ways readers get starved.1. Unbounded WAL on the shared DBs (
central-db.ts,archive-db.ts)Neither set
journal_size_limit, so their WAL never truncated back down after a checkpoint and every reader paid an ever-growing WAL-index scan.central-db(fusion-central.db) is shared across all projects and cluster nodes — the most cross-process-contended DB — yet was the least tuned;archive-dbhad the same latent gap.journal_size_limit = 4194304(4 MB), matching the per-projectdb.ts.synchronous = FULL/wal_autocheckpoint = 1000explicitly for intent. These are already SQLite's defaults (verified:node:sqlitekeepssynchronous=FULLunder WAL) — no behavior change.2.
vacuum()held the EXCLUSIVE lock past its own runtime (db.ts)Resetting
locking_modetoNORMALdoes not drop the WAL exclusive lock until the connection next touches the DB — so every other process stayed locked out of reads (SQLITE_BUSY) until some unrelated query ran. A plainSELECTdoes not release it in WAL mode (verified empirically); a checkpoint does.PASSIVEcheckpoint in thefinallyto force the release immediately.locking_mode=NORMALreset so a throw there can't mask the original error or skip the release; log previously-silent checkpoint failures.3. Background
integrity_checkfroze the event loop for seconds (db.ts)scheduleBackgroundIntegrityCheck(~60s after init) ranPRAGMA integrity_checkon the live connection, walking every page and blocking the event loop — the largest single stall in normal operation.sqlite3CLI in a child process (asyncspawn), matching the existing out-of-process pattern (quickCheckSqliteFile,.recover).-readonlyso it can never checkpoint or write the live WAL (works because the live process holds the DB open →-shmexists).verified=false) — same behavior as today on those environments.runBackgroundIntegrityCheck()seam centralizes the offload+fallback policy and gives the scheduler one deterministic, testable point.VACUUM is intentionally not offloaded: the call graph shows it runs only via the
fn db vacuumCLI command and tests — never from the periodic maintenance loop — so it is not a background event-loop stall, and an out-of-process VACUUM on a live WAL DB would add corruption surface for no hot-path benefit.Tests
central-db/archive-db: assert the new PRAGMAs.db.ts: a second connection (busy_timeout=0) can read immediately aftervacuum()returns (fails on pre-fix code); coverage forintegrityCheckSqliteFileAsync; startup-integrity tests updated to the async/offloaded seam (deterministic regardless of whether thesqlite3CLI exists).tsc --noEmitclean.Notes
/ce-code-review(9 reviewers). One finding (a claimedsynchronousNORMAL→FULL regression) was verified to be a false positive and dropped.applyWalPragmas()helper (3 inline copies for now); read-latency instrumentation to confirm steady-state read stalls (the evidence that would gate any larger async/worker investment — which the eval recommends against, as it would reinvite thenode:sqlitecorruption history).🤖 Generated with Claude Code