fix(whatsapp_data): recover from SQLite corruption instead of re-flooding Sentry (#4733)#4740
Conversation
📝 WalkthroughWalkthroughWhatsApp data writes now detect SQLite corruption, quarantine and rebuild damaged databases, validate recovered schemas, and reset reporting after successful recovery. Observability classifies matching corruption failures as expected warnings, with coverage for recovery and classification boundaries. ChangesWhatsApp SQLite corruption recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WhatsAppIngest
participant WhatsAppStore
participant SQLite
participant Observability
WhatsAppIngest->>WhatsAppStore: upsert chat or message
WhatsAppStore->>SQLite: execute write with retry
SQLite-->>WhatsAppStore: corruption error
WhatsAppStore->>SQLite: quick_check and rebuild schema
WhatsAppStore-->>WhatsAppIngest: recovered write result
WhatsAppStore->>Observability: report corruption breadcrumb once
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
…sai#4733) Add is_sqlite_corrupt to recognise a malformed on-disk image (SQLITE_CORRUPT code 11 / SQLITE_NOTADB code 26), by rusqlite error code with a flattened-string text fallback. This is the signal the store uses to trigger quarantine + rebuild recovery instead of retrying a dead file on every 2-30s scan tick (Sentry TAURI-RUST-KNH: 1,813 escalating events from one host).
…umansai#4733) On a confirmed SQLITE_CORRUPT malformed image, the store now quarantines the damaged whatsapp_data.db (+ WAL/SHM side-files) to a timestamped .corrupt-<ts> copy (preserved, never deleted), rebuilds an empty schema via init_schema, and confirms with PRAGMA integrity_check — mirroring the memory_store/memory_queue pattern. quick_check pre-confirms corruption so a transient mmap fault never destroys good data. The two upsert paths and prune route through write_with_corrupt_recovery, which drives recovery at most once per call then retries once against the rebuilt DB, so ingest self-heals instead of wedging. A process-wide CORRUPT_REPORTED latch reports to Sentry once per corruption episode (reset after a successful recovery) instead of on every scan tick.
…ed (tinyhumansai#4733) Add is_whatsapp_data_sqlite_corrupt_message + a WhatsAppDataSqliteCorrupt kind that demotes '[whatsapp_data] ingest failed: ... database disk image is malformed / file is not a database' out of the Sentry error stream. Defense-in-depth AFTER the store's quarantine+rebuild recovery, not instead of it: it only covers residual noise in the window between detection and a successful rebuild (or a rebuild that keeps failing on a wedged host FS). Scoped to the whatsapp ingest+upsert envelope so unrelated malformed-image errors in other domains still reach Sentry.
upsert_recovers_from_corrupt_database writes a malformed image at the whatsapp_data.db path, runs the upsert path, and asserts recovery quarantines the bad file (preserved, not deleted), rebuilds, and the same upsert SUCCEEDS against the fresh DB — with the report latch reset after recovery (fires at most once per episode). recover_corrupt_db_is_noop_on_healthy_db asserts a healthy DB is never quarantined and its data survives.
oxoxDev
force-pushed
the
fix/4733-whatsapp-sqlite-corruption-recovery
branch
from
250209d to
b478652
Compare
![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: b478652394
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| match retry_on_sqlite_busy(op_name, &f) { | ||
| Ok(val) => Ok(val), | ||
| Err(e) if is_sqlite_corrupt(&e) => { | ||
| self.report_and_recover(op_name, &e); |
There was a problem hiding this comment.
When whatsapp_data.db is already malformed at process startup, WhatsAppDataStore::new() calls init_schema() before any public write can enter this wrapper; that init error makes global::init leave the singleton unset, so subsequent ingest RPCs fail with “store accessed before init” instead of ever reaching the quarantine/rebuild path. This means a corruption that survives an app restart remains wedged and can still repeat on every scanner tick; handle is_sqlite_corrupt around startup schema init as well as write calls.
Useful? React with 👍 / 👎.
| if !(lower.contains("upsert wa_message") || lower.contains("upsert wa_chat")) { | ||
| return false; |
There was a problem hiding this comment.
This classifier only accepts upsert wa_chat/upsert wa_message frames, but the same commit wraps prune_old_messages in corrupt recovery. If corruption is detected from the prune path and recovery/retry still fails, the RPC error is shaped around prune old wa_messages (or the bare SQLite prepare/open error), so this predicate returns false and the residual RPC-path event can still page Sentry every scan tick; include the prune write context in the narrow match.
Useful? React with 👍 / 👎.
![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 (2)
src/openhuman/whatsapp_data/store.rs (2)
304-328: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
quick_check_okandintegrity_check_okare nearly identical (one uses a rawConnection::open+ inlinebusy_timeout, the other reusesopen_conn/configure_connection). Could be collapsed into one helper parameterized by the pragma name, but the current duplication is small and the asymmetry (skipping WAL-mode pragma during the pre-quarantine check) may be intentional.🤖 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/openhuman/whatsapp_data/store.rs` around lines 304 - 328, `quick_check_ok` and `integrity_check_ok` duplicate nearly the same PRAGMA execution flow in `store.rs`; factor the shared logic into a single helper that takes the pragma name and returns the boolean result, then have both methods delegate to it. Keep the existing behavior difference intact by preserving `quick_check_ok`’s direct `Connection::open`/`busy_timeout` path versus `integrity_check_ok`’s `open_conn`/`configure_connection` path if that distinction is intentional, but centralize the common query-and-compare logic to reduce duplication.
24-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGlobal latch is process-wide, not per-store-instance.
CORRUPT_REPORTEDis a single process-wide static. If more than oneWhatsAppDataStoreis ever live in the same process (e.g. multiple workspaces/accounts with separate stores), corruption in one store's DB would suppress the Sentry report for an unrelated corruption episode in a different store. Currently harmless if only one store instance exists per process, but worth confirming that assumption holds.🤖 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/openhuman/whatsapp_data/store.rs` around lines 24 - 31, The CORRUPT_REPORTED latch in WhatsAppDataStore is process-wide, so a corruption in one store can suppress Sentry reporting for a different store instance. Update the dedupe state to be scoped per store (for example, inside WhatsAppDataStore or keyed by the store identity used by the scanner logic) and adjust the recovery/reset path accordingly. Use the CORRUPT_REPORTED symbol and the surrounding WhatsAppDataStore/whatsapp_scanner corruption-handling flow to locate and replace the global behavior.
🤖 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 `@src/openhuman/whatsapp_data/store.rs`:
- Around line 286-302: The recovery path in `report_and_recover` ignores the
result of `integrity_check_ok()` because it always falls through to `Ok(true)`,
so the function reports success even when the rebuilt database is still
unhealthy or the integrity check errors. Update the match in
`report_and_recover` to return the integrity-check outcome explicitly: keep the
success branch as `Ok(true)`, and make the `Ok(false)` and `Err(e)` branches
return `Err(...)` (or otherwise propagate failure) after logging. This keeps the
`CORRUPT_REPORTED` reset and “ingest will resume” behavior in
`report_and_recover` aligned with actual recovery success.
- Around line 256-278: The quarantine loop in `recover_corrupt_db()` is too
strict for `-wal`/`-shm` side-files and can abort before `init_schema()` runs,
which leaves a recreated empty DB behind. Update the `whatsapp_data` quarantine
logic to make side-file moves best-effort after the main database file is
quarantined, or ensure the schema is rebuilt even if a side-file rename fails.
Keep the main file recovery path intact and adjust the
`with_name_suffix`/`std::fs::rename` handling so failures on `-wal` or `-shm` do
not stop the recovery flow.
---
Nitpick comments:
In `@src/openhuman/whatsapp_data/store.rs`:
- Around line 304-328: `quick_check_ok` and `integrity_check_ok` duplicate
nearly the same PRAGMA execution flow in `store.rs`; factor the shared logic
into a single helper that takes the pragma name and returns the boolean result,
then have both methods delegate to it. Keep the existing behavior difference
intact by preserving `quick_check_ok`’s direct `Connection::open`/`busy_timeout`
path versus `integrity_check_ok`’s `open_conn`/`configure_connection` path if
that distinction is intentional, but centralize the common query-and-compare
logic to reduce duplication.
- Around line 24-31: The CORRUPT_REPORTED latch in WhatsAppDataStore is
process-wide, so a corruption in one store can suppress Sentry reporting for a
different store instance. Update the dedupe state to be scoped per store (for
example, inside WhatsAppDataStore or keyed by the store identity used by the
scanner logic) and adjust the recovery/reset path accordingly. Use the
CORRUPT_REPORTED symbol and the surrounding WhatsAppDataStore/whatsapp_scanner
corruption-handling flow to locate and replace the global behavior.
🪄 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
Run ID: 3565bfc0-c4ef-4dda-a458-07a5946def2e
📒 Files selected for processing (4)
src/core/observability.rssrc/openhuman/whatsapp_data/sqlite_retry.rssrc/openhuman/whatsapp_data/store.rssrc/openhuman/whatsapp_data/store_tests.rs
| // 2. Quarantine the main DB + WAL/SHM side-files to `<name>.corrupt-<ts>`. | ||
| let ts = Self::now_secs(); | ||
| let mut quarantined = 0usize; | ||
| for suffix in &["", "-wal", "-shm"] { | ||
| let src = with_name_suffix(&self.db_path, suffix); | ||
| if !src.exists() { | ||
| continue; | ||
| } | ||
| let dst = with_name_suffix(&src, &format!(".corrupt-{ts}")); | ||
| std::fs::rename(&src, &dst).with_context(|| { | ||
| format!( | ||
| "quarantine corrupt whatsapp_data file {} -> {}", | ||
| src.display(), | ||
| dst.display() | ||
| ) | ||
| })?; | ||
| log::warn!( | ||
| "[whatsapp_data] quarantined {} -> {}", | ||
| src.display(), | ||
| dst.display() | ||
| ); | ||
| quarantined += 1; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file structure first
ast-grep outline src/openhuman/whatsapp_data/store.rs --view expanded || true
# Show the relevant sections around quarantine/recovery and read-ops that may race
sed -n '120,320p' src/openhuman/whatsapp_data/store.rs
printf '\n---\n'
sed -n '500,700p' src/openhuman/whatsapp_data/store.rs
# Locate the corruption-detection path
rg -n "is_sqlite_corrupt|report_and_recover|write_with_corrupt_recovery|open_conn|init_schema|quarantine" src/openhuman/whatsapp_data/store.rsRepository: tinyhumansai/openhuman
Length of output: 22882
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the connection/bootstrap path in the target file
sed -n '44,130p' src/openhuman/whatsapp_data/store.rs
printf '\n---\n'
# Inspect the corruption classifier and retry helper definitions
rg -n "fn is_sqlite_corrupt|is_sqlite_corrupt\\(|retry_on_sqlite_busy|SQLITE_CORRUPT|no such table" src/openhuman -g '!target'Repository: tinyhumansai/openhuman
Length of output: 9810
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the SQLite error classifier / retry helper used by whatsapp_data
sed -n '1,260p' src/openhuman/whatsapp_data/sqlite_retry.rs
printf '\n---\n'
# Look for any recovery path that treats "no such table" or schema-missing as corruption
rg -n "no such table|schema-less|recover_corrupt_db|write_with_corrupt_recovery|report_and_recover|init_schema\\(" src/openhuman/whatsapp_dataRepository: tinyhumansai/openhuman
Length of output: 11407
Make side-file quarantine best-effort
A failed -wal/-shm rename currently aborts recover_corrupt_db() before init_schema(), so the retry can recreate an empty DB at self.db_path and later calls will fail with no such table without re-entering corruption recovery. Quarantine the side-files best-effort, or rebuild the schema once the main file has been quarantined.
🤖 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/openhuman/whatsapp_data/store.rs` around lines 256 - 278, The quarantine
loop in `recover_corrupt_db()` is too strict for `-wal`/`-shm` side-files and
can abort before `init_schema()` runs, which leaves a recreated empty DB behind.
Update the `whatsapp_data` quarantine logic to make side-file moves best-effort
after the main database file is quarantined, or ensure the schema is rebuilt
even if a side-file rename fails. Keep the main file recovery path intact and
adjust the `with_name_suffix`/`std::fs::rename` handling so failures on `-wal`
or `-shm` do not stop the recovery flow.
| // 4. Confirm the rebuilt image is structurally sound. | ||
| match self.integrity_check_ok() { | ||
| Ok(true) => log::warn!( | ||
| "[whatsapp_data] corruption recovery complete: quarantined {quarantined} file(s), \ | ||
| rebuilt empty schema, integrity_check=ok at {}", | ||
| self.db_path.display() | ||
| ), | ||
| Ok(false) => log::error!( | ||
| "[whatsapp_data] rebuilt DB still fails integrity_check at {}", | ||
| self.db_path.display() | ||
| ), | ||
| Err(e) => { | ||
| log::error!("[whatsapp_data] integrity_check after rebuild could not run: {e:#}") | ||
| } | ||
| } | ||
| Ok(true) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
integrity_check_ok result is computed but ignored — function always returns Ok(true).
Regardless of whether integrity_check_ok() returns Ok(true), Ok(false), or Err, the function falls through to the unconditional Ok(true) at line 301. If the rebuilt schema somehow still fails integrity_check (or the check can't run), report_and_recover's Ok(true) branch fires anyway: it logs "ingest will resume" and resets CORRUPT_REPORTED, even though recovery wasn't actually confirmed. This re-arms the latch to page again on the very next failure — undermining the "report once per episode" guarantee for this (rare but real) case. The doc comment on lines 211-214 promises Err "when ... the schema rebuild failed," but a rebuild that runs without SQL error yet fails the subsequent integrity check currently still yields Ok(true).
🐛 Proposed fix
- match self.integrity_check_ok() {
- Ok(true) => log::warn!(
- "[whatsapp_data] corruption recovery complete: quarantined {quarantined} file(s), \
- rebuilt empty schema, integrity_check=ok at {}",
- self.db_path.display()
- ),
- Ok(false) => log::error!(
- "[whatsapp_data] rebuilt DB still fails integrity_check at {}",
- self.db_path.display()
- ),
- Err(e) => {
- log::error!("[whatsapp_data] integrity_check after rebuild could not run: {e:#}")
- }
- }
- Ok(true)
+ match self.integrity_check_ok() {
+ Ok(true) => {
+ log::warn!(
+ "[whatsapp_data] corruption recovery complete: quarantined {quarantined} file(s), \
+ rebuilt empty schema, integrity_check=ok at {}",
+ self.db_path.display()
+ );
+ Ok(true)
+ }
+ Ok(false) => {
+ log::error!(
+ "[whatsapp_data] rebuilt DB still fails integrity_check at {}",
+ self.db_path.display()
+ );
+ Err(anyhow::anyhow!(
+ "rebuilt whatsapp_data db still fails integrity_check"
+ ))
+ }
+ Err(e) => {
+ log::error!("[whatsapp_data] integrity_check after rebuild could not run: {e:#}");
+ Err(e.context("integrity_check after rebuild could not run"))
+ }
+ }📝 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.
| // 4. Confirm the rebuilt image is structurally sound. | |
| match self.integrity_check_ok() { | |
| Ok(true) => log::warn!( | |
| "[whatsapp_data] corruption recovery complete: quarantined {quarantined} file(s), \ | |
| rebuilt empty schema, integrity_check=ok at {}", | |
| self.db_path.display() | |
| ), | |
| Ok(false) => log::error!( | |
| "[whatsapp_data] rebuilt DB still fails integrity_check at {}", | |
| self.db_path.display() | |
| ), | |
| Err(e) => { | |
| log::error!("[whatsapp_data] integrity_check after rebuild could not run: {e:#}") | |
| } | |
| } | |
| Ok(true) | |
| } | |
| // 4. Confirm the rebuilt image is structurally sound. | |
| match self.integrity_check_ok() { | |
| Ok(true) => { | |
| log::warn!( | |
| "[whatsapp_data] corruption recovery complete: quarantined {quarantined} file(s), \ | |
| rebuilt empty schema, integrity_check=ok at {}", | |
| self.db_path.display() | |
| ); | |
| Ok(true) | |
| } | |
| Ok(false) => { | |
| log::error!( | |
| "[whatsapp_data] rebuilt DB still fails integrity_check at {}", | |
| self.db_path.display() | |
| ); | |
| Err(anyhow::anyhow!( | |
| "rebuilt whatsapp_data db still fails integrity_check" | |
| )) | |
| } | |
| Err(e) => { | |
| log::error!("[whatsapp_data] integrity_check after rebuild could not run: {e:#}"); | |
| Err(e.context("integrity_check after rebuild could not run")) | |
| } | |
| } |
🤖 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/openhuman/whatsapp_data/store.rs` around lines 286 - 302, The recovery
path in `report_and_recover` ignores the result of `integrity_check_ok()`
because it always falls through to `Ok(true)`, so the function reports success
even when the rebuilt database is still unhealthy or the integrity check errors.
Update the match in `report_and_recover` to return the integrity-check outcome
explicitly: keep the success branch as `Ok(true)`, and make the `Ok(false)` and
`Err(e)` branches return `Err(...)` (or otherwise propagate failure) after
logging. This keeps the `CORRUPT_REPORTED` reset and “ingest will resume”
behavior in `report_and_recover` aligned with actual recovery success.
1 participant
Summary
whatsapp_dataSQLite store now self-heals from an on-disk corruption (SQLITE_CORRUPT/ "database disk image is malformed") instead of re-hitting a dead DB on every 2–30s scan tick.TAURI-RUST-KNH).Problem
A user's
whatsapp_data.dbbecame corrupted (SQLITE_CORRUPT, code 11). Nothing detected or repaired it, so the whatsapp scanner's periodic ingest re-opened the malformed file and re-reported the same failure on every poll — 1,813 events, escalating, from one machine (SentryTAURI-RUST-KNH). A malformed on-disk image never heals on its own: every write fails forever and the store had no recovery, no report latch, and the expected-error filter only recognised "database is locked" (busy), not corruption.The codebase already had a proven recovery pattern for this exact failure in
memory_queue/memory_store(corrupt-detect → quarantine → rebuild → integrity-check + a once-per-process report latch);whatsapp_datasimply never adopted it.Solution
Port the
memory_storerecovery pattern towhatsapp_data:whatsapp_data/sqlite_retry.rs::is_sqlite_corrupt): match rusqliteErrorCode::DatabaseCorrupt/NotADatabase(with a lowercased-text fallback for "disk image is malformed" / "file is not a database") throughanyhowcontext layers.whatsapp_data/store.rs::recover_corrupt_db): re-confirm withPRAGMA quick_checkfirst (a transient mmap fault that now passes is not quarantined — good data is never destroyed), then rename the.db+-wal+-shmside-files to a timestamped.corrupt-<ts>copy (preserved for inspection/salvage), rebuild the schema via the existing init path, andPRAGMA integrity_checkthe result.write_with_corrupt_recovery): at most one recovery + one retry per write call, so a genuinely-wedged filesystem can't loop.CORRUPT_REPORTEDlatch): first detection pages Sentry; the latch clears after a settled recovery so a genuinely-new, later corruption can still page exactly once.core/observability.rs): newExpectedErrorKind::WhatsAppDataSqliteCorrupt+ a tightly-scoped classifier (requires the[whatsapp_data] ingest failed:envelope and anupsert wa_chat/wa_messageframe and malformed-image text) so unrelated corruption elsewhere still reaches Sentry.Wired into all three ingest writes (
upsert_chats,upsert_messages,prune_old_messages) — the full write path is defended consistently. The store opens a fresh connection per call (no cached handle), so recovery needs no handle-drop before the rename; the next open picks up the rebuilt file. Recovery runs under the existing write-lock, so it stays serialized against concurrent writers.RCA-vs-suppress: Bucket = real-defect (data path). The fix is the recovery; the classifier demotion is defense-in-depth after the store self-heals and pages once — never a substitute for the fix.
Submission Checklist
cargo-llvm-cov+diff-cover --compare-branch=upstream/main: 88% on changed lines (185/208). Uncovered lines are recovery logging/error arms (integrity-check-fails, quarantine-rename-fails, recovery-failed logging) that need fault injection; the core corrupt→quarantine→rebuild→success path, the transient-quick_check-noop, and the detection/classifier matrices are all coveredN/A: behaviour-only reliability change to an existing store (no new feature row)## Related—N/A: no matrix feature IDs touchedN/A: internal store reliability, no user-facing surfaceCloses #NNNin## RelatedImpact
.corrupt-<ts>for inspection/salvage; the rebuilt DB starts empty and re-populates from the next scan (WhatsApp data is a local cache re-derivable from the source).Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/4733-whatsapp-sqlite-corruption-recovery(rebased onto currentmain, past the fix(tests): restore test-module imports broken by recent refactors #4759 libtest fix)Validation Run
pnpm --filter openhuman-app format:check— N/A (noapp/changes)pnpm typecheck— N/A (no TS changes)store_tests::39 passed incl.upsert_recovers_from_corrupt_database;sqlite_retry9 passed; observability classifier passed)cargo fmt --checkclean,cargo check --libclean,cargo clippy --libclean on all four changed filesapp/src-taurichanges)Behavior Changes
Parity Contract
quick_check-confirmed corrupt image); busy/locked handling unchanged; no schema change.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Bug Fixes