feat(ai-hist): read-only reads, lock diagnostics, and write guardrails - #46
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reached
Next review available in: 39 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe core library adds shared SQLite lock timeouts and read-only connections. The CLI adds the ChangesDatabase reliability features
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Poem
🚥 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: a038affc01
ℹ️ 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".
| let conn = if is_read_only(&cli.command) && db_path.exists() { | ||
| open_db_readonly(&db_path)? | ||
| } else { | ||
| open_db(&db_path)? |
There was a problem hiding this comment.
When an existing database was created by an older release, this branch opens it read-only and permanently skips init_db; for example, search_all unconditionally queries session_events/session_events_fts, so a user upgrading from before those tables were added gets no such table on their first search instead of the previous automatic migration. The read-only path needs a schema-version check or a one-time writable migration before serving queries.
Useful? React with 👍 / 👎.
| | Command::Export { .. } | ||
| | Command::Doctor { .. } |
There was a problem hiding this comment.
When ai-hist pack runs while another process holds the database write lock, it still falls through to open_db, which executes schema initialization and can wait or fail on that lock. pack_entries only calls the read-only search path and formats output, so omitting Command::Pack defeats the new noncontending-read behavior for this query command.
Useful? React with 👍 / 👎.
| for holder in holders.iter().filter(|h| h.is_wedged()) { | ||
| problems.push(format!( | ||
| "pid {} is {} and holds the database open; it cannot release the write lock (resume it: kill -CONT {})", | ||
| holder.pid, holder.state, holder.pid | ||
| )); |
There was a problem hiding this comment.
When a stopped process merely has an idle or read-only database handle open, lsof includes it even though probe_write_lock succeeds, yet this loop reports that it cannot release the write lock and recommends kill -CONT. Having the main database file open does not establish ownership of a write transaction, so this produces a contradictory false diagnosis; only associate the holder with a wedged write lock when the probe is blocked and ownership can be established.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Three hardening changes for a database shared by several long-lived processes, each addressing something that made a recent multi-day sync outage harder to survive or to diagnose. Read-only handles for read commands. Every command went through open_db, which runs init_db -- applying the schema is a write, so `search`, `recent`, `stats`, and `pack` were taking a write lock they never needed. They now open with SQLITE_OPEN_READ_ONLY and skip init_db, so a query can neither block the writer nor be blocked behind it. is_read_only is conservative: anything unlisted keeps a writable handle, so a miscategorised command fails safe. Because init_db is also where migrations live, a read-only open is gated on schema_is_current -- a database from an older release falls back to a writable open that migrates it, rather than being served queries against tables it does not have. `ai-hist doctor`. Reports database and WAL size, free space, whether a writer can start, and which processes hold the file open with their process state, flagging holders that are stopped (T) or zombies (Z). Those never run again on their own, so a write transaction they hold is held forever and no busy timeout escapes it. It claims causation only when a writer is actually blocked: holding the file open is not the same as owning a write transaction, and SQLite will not say who holds the lock. Diagnosing exactly this took hours of manual lsof/ps work; it is now one command that also prints the remedy. Write guardrails. Sync refuses to start below a free-space floor rather than failing partway -- an out-of-space write is what truncated .sync-state.json and wedged sync in the first place. After a successful sync it checkpoints the WAL (TRUNCATE, best effort) and warns when the WAL stays large, which indicates a long-lived reader pinning an old snapshot; an unchecked WAL grew to 156MB in practice. Formatting: also applies rustfmt to a handful of pre-existing spots in these files that were already non-conforming, so the crates are now fmt-clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
71f13ec to
42a8685
Compare
![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
🧹 Nitpick comments (3)
crates/ai-hist/src/lib.rs (2)
1706-1709: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueBuild the WAL path from the OS string, not from
Display.
Path::display()replaces invalid UTF-8 withU+FFFD. For a non-UTF-8 database path,wal_paththen returns a path that does not exist, and bothdoctorandsync_basicsilently report a WAL size of 0.♻️ Proposed change
fn wal_path(db_path: &Path) -> PathBuf { - PathBuf::from(format!("{}-wal", db_path.display())) + let mut name = db_path.as_os_str().to_os_string(); + name.push("-wal"); + PathBuf::from(name) }🤖 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 `@crates/ai-hist/src/lib.rs` around lines 1706 - 1709, Update wal_path to construct the “-wal” suffix from the database path’s raw OS-string representation rather than Path::display(), preserving non-UTF-8 bytes and returning the correct sidecar path for doctor and sync_basic.
1855-1914: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a non-zero exit status when
doctorfinds problems.
doctorreturnsOk(())in both branches. A cron job or health check must parse stdout or the JSON payload to learn that the database is wedged. A distinct exit code makes the command usable in a script directly. If you change this, document the codes in the command help, because the change is user-visible.🤖 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 `@crates/ai-hist/src/lib.rs` around lines 1855 - 1914, Update the doctor command’s final status handling so it returns a non-zero exit status whenever the problems collection is non-empty, including the JSON output path, while preserving zero for healthy databases. Document the resulting exit-code meanings in the doctor command’s help text, using the existing doctor implementation and problems handling symbols.crates/ai-hist-core/src/lib.rs (1)
270-280: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop
SQLITE_OPEN_URIfor path consistency.
open_dbopens the user-supplied path as a literal filename, whileopen_db_readonlytreats the same value as a URI when accepted. If it starts withfile:or contains URI query parameters, the two paths behave differently.♻️ Proposed change
let conn = Connection::open_with_flags( path, - OpenFlags::SQLITE_OPEN_READ_ONLY - | OpenFlags::SQLITE_OPEN_NO_MUTEX - | OpenFlags::SQLITE_OPEN_URI, + OpenFlags::SQLITE_OPEN_READ_ONLY | OpenFlags::SQLITE_OPEN_NO_MUTEX, )🤖 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 `@crates/ai-hist-core/src/lib.rs` around lines 270 - 280, Remove OpenFlags::SQLITE_OPEN_URI from the flags used by open_db_readonly, keeping only the read-only and no-mutex flags so user-supplied paths are interpreted consistently with open_db.
🤖 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 `@crates/ai-hist/src/lib.rs`:
- Around line 504-511: The doctor command must not create a missing database. In
crates/ai-hist/src/lib.rs lines 504-511, dispatch Command::Doctor before opening
the connection so open_db is never called; in lines 1793-1798, check db_path
existence and return an explicit “database does not exist” result instead of
calling Connection::open.
- Around line 3298-3322: Update the JSONL read loop around reader.read_line so
an unterminated trailing line is not parsed, counted, committed, or included in
consumed; break before advancing the checkpoint when read_line returns bytes
without a terminating newline, leaving the offset at the start of that partial
record for the next run.
---
Nitpick comments:
In `@crates/ai-hist-core/src/lib.rs`:
- Around line 270-280: Remove OpenFlags::SQLITE_OPEN_URI from the flags used by
open_db_readonly, keeping only the read-only and no-mutex flags so user-supplied
paths are interpreted consistently with open_db.
In `@crates/ai-hist/src/lib.rs`:
- Around line 1706-1709: Update wal_path to construct the “-wal” suffix from the
database path’s raw OS-string representation rather than Path::display(),
preserving non-UTF-8 bytes and returning the correct sidecar path for doctor and
sync_basic.
- Around line 1855-1914: Update the doctor command’s final status handling so it
returns a non-zero exit status whenever the problems collection is non-empty,
including the JSON output path, while preserving zero for healthy databases.
Document the resulting exit-code meanings in the doctor command’s help text,
using the existing doctor implementation and problems handling symbols.
🪄 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: CHILL
Plan: Pro Plus
Run ID: bbced79f-6228-4dfa-a335-952c9ae6697f
📒 Files selected for processing (2)
crates/ai-hist-core/src/lib.rscrates/ai-hist/src/lib.rs
| // Read-only commands get a handle that cannot take the write lock, so a | ||
| // query never contends with the writer. Falls back to a writable open when | ||
| // the database does not exist yet, since that first open has to create it. | ||
| let conn = if is_read_only(&cli.command) && db_path.exists() { | ||
| open_db_readonly(&db_path)? | ||
| } else { | ||
| open_db(&db_path)? | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
doctor creates the database it is supposed to diagnose. Both sites open the database with an opener that creates the file when it is missing, so running ai-hist doctor on a machine with no database reports on a database it just created instead of reporting that none exists.
crates/ai-hist/src/lib.rs#L504-L511: dispatchCommand::Doctorbefore the connection is opened, soopen_dbnever runs for this command.crates/ai-hist/src/lib.rs#L1793-L1798: return an explicit "database does not exist" result whendb_pathdoes not exist, instead of callingConnection::open.
📍 Affects 1 file
crates/ai-hist/src/lib.rs#L504-L511(this comment)crates/ai-hist/src/lib.rs#L1793-L1798
🤖 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 `@crates/ai-hist/src/lib.rs` around lines 504 - 511, The doctor command must
not create a missing database. In crates/ai-hist/src/lib.rs lines 504-511,
dispatch Command::Doctor before opening the connection so open_db is never
called; in lines 1793-1798, check db_path existence and return an explicit
“database does not exist” result instead of calling Connection::open.
| loop { | ||
| line.clear(); | ||
| let read = reader.read_line(&mut line)?; | ||
| if read == 0 { | ||
| break; | ||
| } | ||
| consumed += read as u64; | ||
| if !line.trim().is_empty() { | ||
| match parser(&line) { | ||
| Ok(Some(entry)) => inserted += insert_history(conn, &entry)?, | ||
| Ok(None) => {} | ||
| Err(_) => errors += 1, | ||
| } | ||
| } | ||
| pending += 1; | ||
| if pending >= JSONL_CHUNK_LINES { | ||
| conn.execute_batch("COMMIT")?; | ||
| state.insert(name.to_string(), json!(consumed)); | ||
| checkpoint(state); | ||
| conn.execute_batch("BEGIN")?; | ||
| pending = 0; | ||
| } | ||
| } | ||
| conn.execute_batch("COMMIT")?; | ||
| state.insert(name.to_string(), json!(consumed)); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
A partial trailing line advances the checkpoint and loses the record.
read_line returns the bytes it read even when the file ends without \n. The source files are appended by a live agent, so the last line can be half-written at the moment of the read. The code then counts those bytes in consumed, hands the torn text to parser (which usually increments errors), and commits the advanced offset. The remainder of that record arrives later in the file but is never read, because the offset already moved past its start.
Stop at an unterminated line and leave the offset before it. The next run reads the complete record.
🐛 Proposed fix
let read = reader.read_line(&mut line)?;
if read == 0 {
break;
}
+ // The producer appends live: a line without a terminator may be
+ // half-written. Leave the offset before it and read it next run.
+ if !line.ends_with('\n') {
+ break;
+ }
consumed += read as u64;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| loop { | |
| line.clear(); | |
| let read = reader.read_line(&mut line)?; | |
| if read == 0 { | |
| break; | |
| } | |
| consumed += read as u64; | |
| if !line.trim().is_empty() { | |
| match parser(&line) { | |
| Ok(Some(entry)) => inserted += insert_history(conn, &entry)?, | |
| Ok(None) => {} | |
| Err(_) => errors += 1, | |
| } | |
| } | |
| pending += 1; | |
| if pending >= JSONL_CHUNK_LINES { | |
| conn.execute_batch("COMMIT")?; | |
| state.insert(name.to_string(), json!(consumed)); | |
| checkpoint(state); | |
| conn.execute_batch("BEGIN")?; | |
| pending = 0; | |
| } | |
| } | |
| conn.execute_batch("COMMIT")?; | |
| state.insert(name.to_string(), json!(consumed)); | |
| loop { | |
| line.clear(); | |
| let read = reader.read_line(&mut line)?; | |
| if read == 0 { | |
| break; | |
| } | |
| // The producer appends live: a line without a terminator may be | |
| // half-written. Leave the offset before it and read it next run. | |
| if !line.ends_with('\n') { | |
| break; | |
| } | |
| consumed += read as u64; | |
| if !line.trim().is_empty() { | |
| match parser(&line) { | |
| Ok(Some(entry)) => inserted += insert_history(conn, &entry)?, | |
| Ok(None) => {} | |
| Err(_) => errors += 1, | |
| } | |
| } | |
| pending += 1; | |
| if pending >= JSONL_CHUNK_LINES { | |
| conn.execute_batch("COMMIT")?; | |
| state.insert(name.to_string(), json!(consumed)); | |
| checkpoint(state); | |
| conn.execute_batch("BEGIN")?; | |
| pending = 0; | |
| } | |
| } | |
| conn.execute_batch("COMMIT")?; | |
| state.insert(name.to_string(), json!(consumed)); |
🤖 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 `@crates/ai-hist/src/lib.rs` around lines 3298 - 3322, Update the JSONL read
loop around reader.read_line so an unterminated trailing line is not parsed,
counted, committed, or included in consumed; break before advancing the
checkpoint when read_line returns bytes without a terminating newline, leaving
the offset at the start of that partial record for the next run.
1 participant
Items 2–4 of the hardening plan for a SQLite database shared by several long-lived processes. Follows #44 (corrupt state file) and #45 (checkpointed progress).
Context: a multi-day sync outage traced to an
agent-relayprocess that was SIGSTOP'd mid-write-transaction. A stopped process never runs again on its own, so it held the single WAL write lock permanently. No timeout or retry survives that. These changes reduce the blast radius and make the condition diagnosable in one command.1. Read-only handles for read commands
Every command went through
open_db, which callsinit_db— applying the schema is a write, sosearch,recent, andstatswere acquiring a write lock they never needed.They now open with
SQLITE_OPEN_READ_ONLYand skipinit_db. A read can then neither block the writer nor be blocked behind it; WAL readers proceed against their snapshot regardless of who holds the write lock.is_read_onlyis deliberately conservative — anything not explicitly listed keeps a writable handle, so a miscategorised command fails safe (an unnecessary lock) rather than unsafe (a write on a read-only connection).exportqualifies because it only writes to a separate destination file.Verified against the live wedged database:
stats,recent, andsearchall return normally while the write lock is held.2.
ai-hist doctorReports size, WAL size, free space, whether a writer can start, and who holds the file open — flagging holders in state
T(stopped) orZ(zombie), which can never release.Actual output against the wedged database:
That diagnosis previously took hours of manual
lsof/pswork.--jsonis available for scripting. The probe uses a short 1.5s timeout sodoctorreports promptly rather than inheriting the 30s production wait.3. Write guardrails
.sync-state.jsonand started this whole incident; stopping up front with an actionable message beats discovering it through torn state.PRAGMA wal_checkpoint(TRUNCATE), best effort (a concurrent reader blocks a full checkpoint, which is not a reason to fail a sync that did its work), plus a warning when the WAL stays large. Unchecked it reached 156MB in practice.Testing
66 tests pass. New coverage:
a_read_only_handle_can_read_but_never_write— reads succeed, writes are rejectedonly_non_mutating_commands_get_a_read_only_handle— assertsSync/Tagare not classified read-only, the direction that would break at runtimea_stopped_or_zombie_holder_is_reported_as_wedged—T/Ts/Zflagged,S/R/Ssnothuman_bytes_scales_unitsdoctorwas validated against the real wedged database, not just unit tests.Note
Also applies rustfmt to a few pre-existing non-conforming spots in these files, so both crates are now fmt-clean. Formatting-only hunks, separable in review.
Not included
Item 5 — routing
agent-relayand the MCP server through append-only spools so onlyai-hist syncwrites SQLite. That's the change that makes this class of failure impossible rather than merely survivable, and it deserves its own ADR.🤖 Generated with Claude Code