fix(db): serialize SQLite writes to eliminate 'database is locked' (v1.1.3) - #64
fix(db): serialize SQLite writes to eliminate 'database is locked' (v1.1.3)#64jmagar wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR introduces a process-wide reentrant write-lock mechanism for SQLite to serialize concurrent write transactions and eliminate ChangesWrite-lock infrastructure and application
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 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 |
![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: 4fa2505343
ℹ️ 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".
|
|
||
| fn insert_logs_batch_once(pool: &DbPool, entries: &[LogBatchEntry]) -> Result<usize> { | ||
| let mut conn = pool.get()?; | ||
| let _write_guard = crate::db::write_lock(); |
There was a problem hiding this comment.
With the default CORTEX_POOL_SIZE=4, a burst of four writer subsystems can each borrow a pooled connection and then queue on this mutex; the three queued writers hold their connections while waiting, so normal read paths cannot get a connection even though WAL should allow reads during the active writer. This can make MCP/API reads time out under exactly the sustained ingest/maintenance contention this change is meant to fix; acquire write_lock() before pool.get() in the guarded write paths so queued writers do not exhaust the read pool.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR addresses frequent SQLite database is locked errors (and resulting dropped log batches) by introducing a process-wide write-serialization lock and acquiring it across runtime write paths, while keeping reads concurrent via WAL.
Changes:
- Added a process-wide reentrant
db::write_lock()(parking_lot) and guarded write transactions / mutating statements across ingest, scanner, heartbeat, notifications, and maintenance. - Updated maintenance/retention write sites to serialize chunked deletes, vacuum, and merge operations.
- Bumped version to
1.1.3across Cargo + manifests and documented the fix in the changelog.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/db/pool.rs | Introduces write_lock() global reentrant mutex for SQLite write serialization. |
| src/db.rs | Re-exports write_lock for use across crate internals. |
| src/db/ingest.rs | Serializes batch insert transaction with write_lock(). |
| src/db/maintenance.rs | Adds write_lock() around maintenance deletes/vacuum/merge write operations. |
| src/scanner.rs | Serializes scanner write transactions (claim/import + metadata updates). |
| src/scanner/checkpoint.rs | Serializes checkpoint store write transactions. |
| src/notifications/dispatcher.rs | Serializes notification dispatcher write transaction inside spawn_blocking. |
| src/heartbeat.rs | Serializes heartbeat ingest transaction; also touches received_at write path. |
| src/app/error_detection/scanner.rs | Serializes error signature/window write transaction. |
| src/app/service.rs | Updates “syslog-owned” wording and serializes ack/unack write transactions. |
| Cargo.toml | Adds parking_lot dependency and bumps crate version to 1.1.3. |
| Cargo.lock | Updates lockfile for version bump + new dependency. |
| CHANGELOG.md | Documents the write-serialization change under 1.1.3. |
| server.json | Bumps published version to 1.1.3. |
| mcpb/manifest.json | Bumps MCP bundle version to 1.1.3. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| loop { | ||
| let conn = pool.get()?; | ||
| let _write_guard = crate::db::write_lock(); | ||
| let chunk = conn.execute( | ||
| "DELETE FROM logs WHERE id IN ( |
| loop { | ||
| let conn = pool.get()?; | ||
| let _write_guard = crate::db::write_lock(); | ||
| let chunk = conn.execute( | ||
| "DELETE FROM logs WHERE id IN ( |
| let received_at = chrono::Utc::now().format("%Y-%m-%dT%H:%M:%SZ").to_string(); | ||
| let mut conn = pool.get()?; | ||
| let _write_guard = crate::db::write_lock(); | ||
| let tx = conn.transaction()?; |
| loop { | ||
| let conn = pool.get()?; | ||
| let _write_guard = crate::db::write_lock(); | ||
| let chunk = conn.execute( | ||
| "DELETE FROM logs WHERE id IN ( |
| loop { | ||
| let conn = pool.get()?; | ||
| let _write_guard = crate::db::write_lock(); | ||
| let chunk = conn.execute( | ||
| "DELETE FROM logs WHERE id IN ( |
| Ok(conn) => { | ||
| let _write_guard = crate::db::write_lock(); | ||
| match conn.execute_batch(&merge_stmt) { | ||
| Ok(()) => { | ||
| consecutive_failures = 0; |
| @@ -149,6 +149,7 @@ fn insert_heartbeat( | |||
| ) -> anyhow::Result<HeartbeatIngestResponse> { | |||
| let received_at = chrono::Utc::now().format("%Y-%m-%dT%H:%M:%SZ").to_string(); | |||
| Ok(conn) => { | ||
| let _write_guard = crate::db::write_lock(); | ||
| match conn.execute_batch(&merge_stmt) { | ||
| Ok(()) => { | ||
| consecutive_failures = 0; |
| @@ -149,6 +149,7 @@ fn insert_heartbeat( | |||
| ) -> anyhow::Result<HeartbeatIngestResponse> { | |||
| let received_at = chrono::Utc::now().format("%Y-%m-%dT%H:%M:%SZ").to_string(); | |||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server.json (1)
10-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix version/package tag mismatch in release metadata.
"version"is1.1.3but OCIidentifierstill targetsv1.1.2. This can publish/resolve the wrong server artifact for the declared release version.Proposed fix
- "identifier": "ghcr.io/jmagar/cortex:v1.1.2", + "identifier": "ghcr.io/jmagar/cortex:v1.1.3",As per coding guidelines, "Update all version-bearing files with the same version number when bumping."
🤖 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 `@server.json` around lines 10 - 14, The release metadata has a version mismatch: the top-level "version" field is "1.1.3" while the OCI package "identifier" still points to "v1.1.2"; update the OCI identifier string in the packages entry (the "identifier" field) to use "v1.1.3" so it matches the "version" value, and scan other package identifier strings in this file for any remaining old tags to keep all version-bearing fields consistent.src/db/pool.rs (1)
963-1119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
crate::db::write_lock()serialization tobackfill_inventory_stats
src/runtime.rsacquiresmaintenance_permitbefore runningdb::backfill_inventory_stats, butsrc/db/pool.rs::backfill_inventory_statsitself never takes the process-wide SQLite writer mutex (crate::db::write_lock()), while other SQLite writer paths do. If any of those writers run concurrently with this backfill, the missing guard can allow contention duringBEGIN IMMEDIATE/COMMIT. Acquirewrite_lock()once at function entry or per-chunk (e.g., around theBEGIN IMMEDIATE/inserts/COMMIT) to match the other writer subsystems.
[code snippet suggestion]loop { let _write_guard = crate::db::write_lock(); conn.execute_batch("BEGIN IMMEDIATE;")?; // inserts/updates conn.execute_batch("COMMIT;")?; std::thread::sleep(BETWEEN_CHUNKS); }🤖 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 `@src/db/pool.rs` around lines 963 - 1119, The backfill_inventory_stats function is missing the process-wide SQLite writer mutex; acquire crate::db::write_lock() to serialize writer access around the BEGIN IMMEDIATE/INSERTs/COMMIT section (or once at function entry) so the backfill doesn't contend with other writers. Modify backfill_inventory_stats to obtain a _write_guard from crate::db::write_lock() before executing the transactional block that runs BEGIN IMMEDIATE, the INSERT/UPDATE statements, and COMMIT (release after COMMIT/end of scope), ensuring the guard variable lives for the duration of the critical section.
🤖 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 `@CHANGELOG.md`:
- Around line 10-22: Update the CHANGELOG.md footer links so the new release
header [1.1.3] has its own compare link and the [Unreleased] compare range is
rebased to start from v1.1.3; specifically add a markdown link reference for
[1.1.3] pointing to the compare URL between the previous tag and v1.1.3, and
update the existing [Unreleased] link to compare from v1.1.3 (or the
repository's main release tag) to HEAD/main. Ensure the link labels referenced
in the body ([Unreleased] and [1.1.3]) exactly match the new footer entries.
In `@src/app/error_detection/scanner.rs`:
- Around line 186-187: Currently the pooled DB connection (conn) is held across
the global write lock acquisition (crate::db::write_lock()), which can exhaust
the pool; drop or let conn go out of scope before calling write_lock(), then
after acquiring the write guard reopen a new connection and start the
transaction (create tx) inside the locked section; specifically move the
creation of tx (conn.transaction()) to after obtaining the write guard and
re-initialize conn there so only the active writer holds a pool slot.
In `@src/app/service.rs`:
- Around line 2061-2063: The code currently acquires a DB connection via
pool.get() and then calls crate::db::write_lock(), which can hold pooled
connections while waiting on the global write mutex; change both commit paths to
acquire the write mutex first (call crate::db::write_lock() and obtain the
guard) and only then check out a connection (pool.get()) and call
conn.transaction() so the mutex wait does not hold a pool slot; apply the same
flip at the other occurrence referenced (the closure around lines 2130-2132).
In `@src/db/maintenance.rs`:
- Around line 55-56: The write mutex (crate::db::write_lock()) must be acquired
before checking out r2d2 pooled connections on write-only paths to avoid queuing
while holding a pool slot; change the order in the affected helpers so you call
crate::db::write_lock() first, then obtain the pooled connection (the code that
currently creates or checks out conn), and only after that call
conn.execute_batch or other write methods (refer to the current occurrences
using the write_lock() binding and conn.execute_batch/conn.prepare/etc. at the
sites around the existing snippets and the other listed locations).
- Around line 636-637: Both delete_oldest_logs_chunk and
checkpoint_wal_and_incremental_vacuum currently obtain a DB connection (conn)
before calling crate::db::write_lock(), which holds a pool slot while waiting;
change the flow so any preflight/read/checkpoint work uses a transient
connection, then drop that connection before calling write_lock(), and after
acquiring the write lock call the connection-acquisition path again to get a
fresh conn for the mutating step (the DELETE/vacuum execute that assigns
deleted_rows). In short: do not hold conn across write_lock(); perform
reads/checkpoint with one connection, release it, call write_lock(), then open a
new conn to run the execute/delete/vacuum inside the locked section (also apply
same change to the other occurrence around the 809-810 area).
In `@src/notifications/dispatcher.rs`:
- Around line 81-83: The dispatcher still calls outbox_mark_dropped(...) and
outbox_schedule_retry(...) via db_read, causing unprotected writes and potential
SQLITE_BUSY; add a single-statement write helper (e.g., db_write_single<F,
T>(pool: &Pool, f: F) -> Result<T, E>) that acquires crate::db::write_lock(),
gets a connection, performs a one-statement transaction or direct exec, and
returns the result, then replace the db_read call sites in dispatcher.rs for
outbox_mark_dropped and outbox_schedule_retry with this new helper so those
auto-committed writes are serialized under write_lock() just like db_tx paths.
In `@src/scanner/checkpoint.rs`:
- Around line 139-141: The code currently calls self.pool.get() then acquires
the global write lock via crate::db::write_lock(), which can cause pooled
connections to be held while waiting for the mutex; move the write_lock()
acquisition to before calling self.pool.get() so the mutex is taken first, then
checkout the connection. Update both occurrences around the transaction creation
(the block using let mut conn = self.pool.get()?; let _write_guard =
crate::db::write_lock(); let tx = conn.transaction()?) and the second similar
block so the order is let _write_guard = crate::db::write_lock(); let mut conn =
self.pool.get()?; let tx = conn.transaction()?; and limit the guard scope to
only cover the critical section as needed.
---
Outside diff comments:
In `@server.json`:
- Around line 10-14: The release metadata has a version mismatch: the top-level
"version" field is "1.1.3" while the OCI package "identifier" still points to
"v1.1.2"; update the OCI identifier string in the packages entry (the
"identifier" field) to use "v1.1.3" so it matches the "version" value, and scan
other package identifier strings in this file for any remaining old tags to keep
all version-bearing fields consistent.
In `@src/db/pool.rs`:
- Around line 963-1119: The backfill_inventory_stats function is missing the
process-wide SQLite writer mutex; acquire crate::db::write_lock() to serialize
writer access around the BEGIN IMMEDIATE/INSERTs/COMMIT section (or once at
function entry) so the backfill doesn't contend with other writers. Modify
backfill_inventory_stats to obtain a _write_guard from crate::db::write_lock()
before executing the transactional block that runs BEGIN IMMEDIATE, the
INSERT/UPDATE statements, and COMMIT (release after COMMIT/end of scope),
ensuring the guard variable lives for the duration of the critical section.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e0bd9d27-fc4b-4790-be0b-a09afe829730
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lockand included by**/*
📒 Files selected for processing (14)
CHANGELOG.mdCargo.tomlmcpb/manifest.jsonserver.jsonsrc/app/error_detection/scanner.rssrc/app/service.rssrc/db.rssrc/db/ingest.rssrc/db/maintenance.rssrc/db/pool.rssrc/heartbeat.rssrc/notifications/dispatcher.rssrc/scanner.rssrc/scanner/checkpoint.rs
| ## [1.1.3] - 2026-06-01 | ||
|
|
||
| ### Fixed | ||
|
|
||
| - **Serialize SQLite writes to eliminate `database is locked` errors and dropped log batches.** | ||
| cortex ran an r2d2 pool of 4 write-capable connections while 6 writer subsystems | ||
| (syslog/docker ingest, heartbeat, notifications, AI index, retention maintenance) | ||
| raced SQLite's single write lock, exceeding `busy_timeout` (~1000+ lock errors/day and | ||
| silently discarded log batches). Added a process-wide reentrant write-serialization | ||
| lock (`db::write_lock()`) acquired by every write transaction and standalone mutating | ||
| statement (purge DELETEs, VACUUM, incremental_vacuum, merge). Reads stay concurrent on | ||
| the pool (WAL); PASSIVE checkpoints are left unguarded. | ||
|
|
There was a problem hiding this comment.
Update changelog reference links for the new 1.1.3 entry.
The new release section was added, but the footer links are still anchored to older versions ([Unreleased] still compares from v0.32.3, and there’s no [1.1.3] compare link). This breaks release diff navigation.
🤖 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 `@CHANGELOG.md` around lines 10 - 22, Update the CHANGELOG.md footer links so
the new release header [1.1.3] has its own compare link and the [Unreleased]
compare range is rebased to start from v1.1.3; specifically add a markdown link
reference for [1.1.3] pointing to the compare URL between the previous tag and
v1.1.3, and update the existing [Unreleased] link to compare from v1.1.3 (or the
repository's main release tag) to HEAD/main. Ensure the link labels referenced
in the body ([Unreleased] and [1.1.3]) exactly match the new footer entries.
| let _write_guard = crate::db::write_lock(); | ||
| let tx = conn.transaction()?; |
There was a problem hiding this comment.
Release the read connection before waiting on the global write lock.
Because Line 105 keeps the pooled connection alive through the fetch/grouping phase, this new lock is acquired while already holding a pool slot. Under concurrent scan/ingest activity, queued writers can sit on multiple idle connections and block readers despite WAL. Reopen the connection under the locked section so only the active writer holds a pool slot.
♻️ Suggested shape
- let _write_guard = crate::db::write_lock();
- let tx = conn.transaction()?;
+ drop(conn);
+ let _write_guard = crate::db::write_lock();
+ let mut conn = pool.get()?;
+ let tx = conn.transaction()?;🤖 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 `@src/app/error_detection/scanner.rs` around lines 186 - 187, Currently the
pooled DB connection (conn) is held across the global write lock acquisition
(crate::db::write_lock()), which can exhaust the pool; drop or let conn go out
of scope before calling write_lock(), then after acquiring the write guard
reopen a new connection and start the transaction (create tx) inside the locked
section; specifically move the creation of tx (conn.transaction()) to after
obtaining the write guard and re-initialize conn there so only the active writer
holds a pool slot.
| let mut conn = pool.get()?; | ||
| let _write_guard = crate::db::write_lock(); | ||
| let tx = conn.transaction()?; |
There was a problem hiding this comment.
Do not queue ack/unack commits on the mutex while holding pool slots.
Both commit paths check out a connection before waiting on write_lock(). Under concurrent write load, queued acknowledgements can park on the mutex with pooled connections held, shrinking the pool for readers. Flip the order in both closures.
🐛 Minimal change
- let mut conn = pool.get()?;
- let _write_guard = crate::db::write_lock();
+ let _write_guard = crate::db::write_lock();
+ let mut conn = pool.get()?;Also applies to: 2130-2132
🤖 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 `@src/app/service.rs` around lines 2061 - 2063, The code currently acquires a
DB connection via pool.get() and then calls crate::db::write_lock(), which can
hold pooled connections while waiting on the global write mutex; change both
commit paths to acquire the write mutex first (call crate::db::write_lock() and
obtain the guard) and only then check out a connection (pool.get()) and call
conn.transaction() so the mutex wait does not hold a pool slot; apply the same
flip at the other occurrence referenced (the closure around lines 2130-2132).
| let _write_guard = crate::db::write_lock(); | ||
| conn.execute_batch(&format!("PRAGMA incremental_vacuum({pages});"))?; |
There was a problem hiding this comment.
Take the write mutex before checking out pooled connections on the write-only paths.
These helpers now queue on write_lock() while already holding r2d2 connections. A burst of maintenance writers can therefore fill the pool with idle waiters and block reads. Flip the order at these sites so the pool slot is only taken by the writer that is actually entering SQLite.
🐛 Minimal change
- let conn = pool.get()?;
- let _write_guard = crate::db::write_lock();
+ let _write_guard = crate::db::write_lock();
+ let conn = pool.get()?;Also applies to: 62-63, 308-309, 412-413, 529-530, 671-672, 737-738, 768-769
🤖 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 `@src/db/maintenance.rs` around lines 55 - 56, The write mutex
(crate::db::write_lock()) must be acquired before checking out r2d2 pooled
connections on write-only paths to avoid queuing while holding a pool slot;
change the order in the affected helpers so you call crate::db::write_lock()
first, then obtain the pooled connection (the code that currently creates or
checks out conn), and only after that call conn.execute_batch or other write
methods (refer to the current occurrences using the write_lock() binding and
conn.execute_batch/conn.prepare/etc. at the sites around the existing snippets
and the other listed locations).
| let _write_guard = crate::db::write_lock(); | ||
| let deleted_rows = conn.execute( |
There was a problem hiding this comment.
Split the preflight connection from the locked write phase.
delete_oldest_logs_chunk and checkpoint_wal_and_incremental_vacuum both do read/checkpoint work on an already checked-out connection and then wait on write_lock() for the mutating step. That still lets contending maintenance tasks pin pool slots while idle. Drop the preflight connection and reacquire one inside the locked section for the actual DELETE/vacuum.
Also applies to: 809-810
🤖 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 `@src/db/maintenance.rs` around lines 636 - 637, Both delete_oldest_logs_chunk
and checkpoint_wal_and_incremental_vacuum currently obtain a DB connection
(conn) before calling crate::db::write_lock(), which holds a pool slot while
waiting; change the flow so any preflight/read/checkpoint work uses a transient
connection, then drop that connection before calling write_lock(), and after
acquiring the write lock call the connection-acquisition path again to get a
fresh conn for the mutating step (the DELETE/vacuum execute that assigns
deleted_rows). In short: do not hold conn across write_lock(); perform
reads/checkpoint with one connection, release it, call write_lock(), then open a
new conn to run the execute/delete/vacuum inside the locked section (also apply
same change to the other occurrence around the 809-810 area).
| let mut conn = pool.get()?; | ||
| let _write_guard = crate::db::write_lock(); | ||
| let tx = conn.transaction()?; |
There was a problem hiding this comment.
Serialize the auto-committed dispatcher writes too.
db_tx is covered now, but this file still routes outbox_mark_dropped(...) and outbox_schedule_retry(...) through db_read, which executes them without write_lock(). Those paths can still hit SQLITE_BUSY, so the dispatcher is not actually fully covered by the new serialization scheme. Add a dedicated single-statement write helper and move those call sites onto it.
🤖 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 `@src/notifications/dispatcher.rs` around lines 81 - 83, The dispatcher still
calls outbox_mark_dropped(...) and outbox_schedule_retry(...) via db_read,
causing unprotected writes and potential SQLITE_BUSY; add a single-statement
write helper (e.g., db_write_single<F, T>(pool: &Pool, f: F) -> Result<T, E>)
that acquires crate::db::write_lock(), gets a connection, performs a
one-statement transaction or direct exec, and returns the result, then replace
the db_read call sites in dispatcher.rs for outbox_mark_dropped and
outbox_schedule_retry with this new helper so those auto-committed writes are
serialized under write_lock() just like db_tx paths.
| let mut conn = self.pool.get()?; | ||
| let _write_guard = crate::db::write_lock(); | ||
| let tx = conn.transaction()?; |
There was a problem hiding this comment.
Acquire the global write lock before checking out checkpoint connections.
Both transaction paths wait on write_lock() while already holding pooled connections. Under concurrent write traffic, that can fill the pool with idle waiters and stall read requests. Take the mutex first, then call self.pool.get().
🐛 Minimal change
- let mut conn = self.pool.get()?;
- let _write_guard = crate::db::write_lock();
+ let _write_guard = crate::db::write_lock();
+ let mut conn = self.pool.get()?;Also applies to: 307-309
🤖 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 `@src/scanner/checkpoint.rs` around lines 139 - 141, The code currently calls
self.pool.get() then acquires the global write lock via crate::db::write_lock(),
which can cause pooled connections to be held while waiting for the mutex; move
the write_lock() acquisition to before calling self.pool.get() so the mutex is
taken first, then checkout the connection. Update both occurrences around the
transaction creation (the block using let mut conn = self.pool.get()?; let
_write_guard = crate::db::write_lock(); let tx = conn.transaction()?) and the
second similar block so the order is let _write_guard = crate::db::write_lock();
let mut conn = self.pool.get()?; let tx = conn.transaction()?; and limit the
guard scope to only cover the critical section as needed.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 15 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/db/maintenance.rs">
<violation number="1" location="src/db/maintenance.rs:55">
P2: Write lock held across std::thread::sleep in purge_old_logs and purge_by_tag_window, blocking other writers during the inter-chunk delay.
The `_write_guard` acquired inside the chunked-DELETE loop is not dropped until the end of the loop body, which includes `std::thread::sleep(50ms)`. This means the process-wide write lock is held for the entire 50ms sleep, preventing the batch writer (and all other write paths) from acquiring it during that window. The PR's stated design intent is to release the lock between chunks so the batch writer can proceed.
Fix: explicitly drop `_write_guard` before `std::thread::sleep(...)` in both loops.</violation>
<violation number="2" location="src/db/maintenance.rs:55">
P2: Acquire `write_lock()` before `self.pool.get()` in this transaction path so blocked writers don't hold pooled connections while waiting on the global write mutex.</violation>
</file>
<file name="src/db/ingest.rs">
<violation number="1" location="src/db/ingest.rs:36">
P2: Acquire the global write lock before `pool.get()`; current ordering can exhaust the r2d2 pool with waiting writers and block read traffic under write contention.</violation>
</file>
<file name="src/app/service.rs">
<violation number="1" location="src/app/service.rs:2062">
P2: Acquire `write_lock()` before checking out the DB connection in this commit path. Waiting on the mutex while holding a pool slot can exhaust the pool during concurrent ack/unack writes.</violation>
</file>
<file name="CHANGELOG.md">
<violation number="1" location="CHANGELOG.md:10">
P3: Add the `[1.1.3]` footer compare link and update `[Unreleased]` to compare from `v1.1.3`; otherwise changelog diff links for the new release won't resolve correctly.</violation>
</file>
<file name="src/app/error_detection/scanner.rs">
<violation number="1" location="src/app/error_detection/scanner.rs:186">
P2: Don't wait on `write_lock()` while holding this pooled connection. Reacquire the connection for the write transaction so queued writers don't pin pool slots and reduce read availability under contention.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| pub fn db_incremental_vacuum(pool: &DbPool, pages: u32) -> Result<()> { | ||
| let conn = pool.get()?; | ||
| let _write_guard = crate::db::write_lock(); |
There was a problem hiding this comment.
P2: Write lock held across std::thread::sleep in purge_old_logs and purge_by_tag_window, blocking other writers during the inter-chunk delay.
The _write_guard acquired inside the chunked-DELETE loop is not dropped until the end of the loop body, which includes std::thread::sleep(50ms). This means the process-wide write lock is held for the entire 50ms sleep, preventing the batch writer (and all other write paths) from acquiring it during that window. The PR's stated design intent is to release the lock between chunks so the batch writer can proceed.
Fix: explicitly drop _write_guard before std::thread::sleep(...) in both loops.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/db/maintenance.rs, line 55:
<comment>Write lock held across std::thread::sleep in purge_old_logs and purge_by_tag_window, blocking other writers during the inter-chunk delay.
The `_write_guard` acquired inside the chunked-DELETE loop is not dropped until the end of the loop body, which includes `std::thread::sleep(50ms)`. This means the process-wide write lock is held for the entire 50ms sleep, preventing the batch writer (and all other write paths) from acquiring it during that window. The PR's stated design intent is to release the lock between chunks so the batch writer can proceed.
Fix: explicitly drop `_write_guard` before `std::thread::sleep(...)` in both loops.</comment>
<file context>
@@ -52,12 +52,14 @@ pub fn db_wal_checkpoint(pool: &DbPool, mode: &str) -> Result<(i64, i64, i64)> {
pub fn db_incremental_vacuum(pool: &DbPool, pages: u32) -> Result<()> {
let conn = pool.get()?;
+ let _write_guard = crate::db::write_lock();
conn.execute_batch(&format!("PRAGMA incremental_vacuum({pages});"))?;
Ok(())
</file context>
|
|
||
| fn insert_logs_batch_once(pool: &DbPool, entries: &[LogBatchEntry]) -> Result<usize> { | ||
| let mut conn = pool.get()?; | ||
| let _write_guard = crate::db::write_lock(); |
There was a problem hiding this comment.
P2: Acquire the global write lock before pool.get(); current ordering can exhaust the r2d2 pool with waiting writers and block read traffic under write contention.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/db/ingest.rs, line 36:
<comment>Acquire the global write lock before `pool.get()`; current ordering can exhaust the r2d2 pool with waiting writers and block read traffic under write contention.</comment>
<file context>
@@ -33,6 +33,7 @@ pub fn insert_logs_batch(pool: &DbPool, entries: &[LogBatchEntry]) -> Result<usi
fn insert_logs_batch_once(pool: &DbPool, entries: &[LogBatchEntry]) -> Result<usize> {
let mut conn = pool.get()?;
+ let _write_guard = crate::db::write_lock();
let tx = conn.transaction()?;
insert_logs_batch_in_tx(&tx, entries)?;
</file context>
| let hash_clone = hash.clone(); | ||
| self.run_db("ack_error.commit", move |pool| { | ||
| let mut conn = pool.get()?; | ||
| let _write_guard = crate::db::write_lock(); |
There was a problem hiding this comment.
P2: Acquire write_lock() before checking out the DB connection in this commit path. Waiting on the mutex while holding a pool slot can exhaust the pool during concurrent ack/unack writes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/app/service.rs, line 2062:
<comment>Acquire `write_lock()` before checking out the DB connection in this commit path. Waiting on the mutex while holding a pool slot can exhaust the pool during concurrent ack/unack writes.</comment>
<file context>
@@ -2059,6 +2059,7 @@ impl CortexService {
let hash_clone = hash.clone();
self.run_db("ack_error.commit", move |pool| {
let mut conn = pool.get()?;
+ let _write_guard = crate::db::write_lock();
let tx = conn.transaction()?;
crate::db::error_signatures::record_ack_event(
</file context>
|
|
||
| pub fn db_incremental_vacuum(pool: &DbPool, pages: u32) -> Result<()> { | ||
| let conn = pool.get()?; | ||
| let _write_guard = crate::db::write_lock(); |
There was a problem hiding this comment.
P2: Acquire write_lock() before self.pool.get() in this transaction path so blocked writers don't hold pooled connections while waiting on the global write mutex.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/db/maintenance.rs, line 55:
<comment>Acquire `write_lock()` before `self.pool.get()` in this transaction path so blocked writers don't hold pooled connections while waiting on the global write mutex.</comment>
<file context>
@@ -52,12 +52,14 @@ pub fn db_wal_checkpoint(pool: &DbPool, mode: &str) -> Result<(i64, i64, i64)> {
pub fn db_incremental_vacuum(pool: &DbPool, pages: u32) -> Result<()> {
let conn = pool.get()?;
+ let _write_guard = crate::db::write_lock();
conn.execute_batch(&format!("PRAGMA incremental_vacuum({pages});"))?;
Ok(())
</file context>
| } | ||
|
|
||
| // --- Write signatures and windows in a single transaction --- | ||
| let _write_guard = crate::db::write_lock(); |
There was a problem hiding this comment.
P2: Don't wait on write_lock() while holding this pooled connection. Reacquire the connection for the write transaction so queued writers don't pin pool slots and reduce read availability under contention.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/app/error_detection/scanner.rs, line 186:
<comment>Don't wait on `write_lock()` while holding this pooled connection. Reacquire the connection for the write transaction so queued writers don't pin pool slots and reduce read availability under contention.</comment>
<file context>
@@ -183,6 +183,7 @@ pub(crate) fn process_chunk(
}
// --- Write signatures and windows in a single transaction ---
+ let _write_guard = crate::db::write_lock();
let tx = conn.transaction()?;
</file context>
|
|
||
| ## [Unreleased] | ||
|
|
||
| ## [1.1.3] - 2026-06-01 |
There was a problem hiding this comment.
P3: Add the [1.1.3] footer compare link and update [Unreleased] to compare from v1.1.3; otherwise changelog diff links for the new release won't resolve correctly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At CHANGELOG.md, line 10:
<comment>Add the `[1.1.3]` footer compare link and update `[Unreleased]` to compare from `v1.1.3`; otherwise changelog diff links for the new release won't resolve correctly.</comment>
<file context>
@@ -7,6 +7,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [Unreleased]
+## [1.1.3] - 2026-06-01
+
+### Fixed
</file context>
…1.1.3) cortex ran an r2d2 pool of 4 write-capable connections while 6 writer subsystems (syslog/docker ingest, heartbeat, notifications, AI index, retention maintenance) raced SQLite's single write lock, exceeding busy_timeout=5000 — ~1000+ lock errors/day and silently discarded log batches (undercounted a real incident 79 vs 150 entries). Add a process-wide reentrant write-serialization lock (db::write_lock(), parking_lot ReentrantMutex) acquired by every runtime write: 12 conn.transaction() sites + 8 standalone mutating statements (purge DELETEs, VACUUM, incremental_vacuum, merge). Reads stay concurrent on the pool (WAL); PASSIVE checkpoints + init migrations + tests untouched. 922 lib tests pass.
4fa2505 to
6e99453
Compare
|
Superseded — the v1.1.3 serialize-writes change is already on main via commit 4fa2505 (merged as part of the fix/serialize-sqlite-writes fast-forward; the 10 write_lock() guard sites are present on main). This PR's head 6e99453 is a duplicate commit from a scratch worktree and would merge as a no-op. Closing; nothing lost. |
Problem
cortex emits
database is lockedcontinuously (~1000+/day, spiking to 370 in an hour) and silently drops log batches as a result — it undercounted a real SSH incident 79 vs the actual 150 auth.log entries because it was lock-saturated at that moment (Failed to flush full log batch … database is locked … discarded_rows=…, plusheartbeat ingest failed,notification_dispatcher cycle failed).Root cause
init_poolbuilds an r2d2 pool ofpool_size = 4connections, all write-capable. SQLite permits exactly one writer at a time, but 6 writer subsystems — syslog ingest, docker ingest, heartbeat, notifications, AI index, retention maintenance — concurrently grab pooled connections and write. Under sustained load (plus a PASSIVE checkpoint that can't drain a 70 MB WAL) writers exceedbusy_timeout=5000and getSQLITE_BUSY. The PRAGMAs themselves are fine (WAL,synchronous=NORMAL, 5 s busy_timeout, 64 MB cache) — the defect is 4 writers racing 1 write lock.Fix
A process-wide reentrant write-serialization lock (
db::write_lock(),parking_lot::ReentrantMutex) acquired by every runtime write so writers queue in-process instead of colliding at the SQLite layer. Reads are untouched and stay concurrent on the pool (WAL allows many readers).Guarded 20 write sites:
conn.transaction()sites — ingest batch, heartbeat, notifications (db_tx), scanner (×2), app/service (×2), error-detection, checkpoint (×2), maintenance (×2).DELETEs (×4),VACUUM,incremental_vacuum(×2), the merge.Left unguarded by design: PASSIVE
wal_checkpoints (don't take the write lock), init-time migrations (single-threaded startup), and test code. Reentrant mutex prevents any nested write path from deadlocking.Validation
cargo check✅Deploy
cortex prod runs
ghcr.io/jmagar/cortex:${CORTEX_VERSION}— after merge, CI builds the image; then bumpCORTEX_VERSIONto 1.1.3 and recreate the container. Verify by watching thedatabase is lockedcount drop to ~0 in cortex's own logs.Diagnosed while investigating an SSH
MaxSessionsincident on dookie; the dropped-batch undercount is what surfaced it.Summary by cubic
Serialize SQLite writes to stop “database is locked” errors and prevent dropped log batches. A process-wide reentrant mutex queues writers; reads stay concurrent.
Bug Fixes
db::write_lock()usingparking_lot::ReentrantMutex; acquired at write transactions and mutating statements across ingest, heartbeat, notifications, scanner/error detection, service ack/unack paths, and maintenance.wal_checkpoints and startup migrations unguarded.Dependencies
parking_lot.cortexto1.1.3and updated manifests.Written for commit 6e99453. Summary will update on new commits.
Summary by CodeRabbit