fix(datastore): auto-recover malformed peewee SQLite on startup

[TimeToBuildBob]

Problem

aw-server (Peewee/SQLite) can enter a restart loop when the on-disk
peewee-sqlite.v2.db is SQLITE_CORRUPT (database disk image is malformed).
PRAGMA quick_check fails, the process exits, and the bundled restart action
hits the same file again. The database is often still partially readable, and
SQLite's recovery tools can salvage events.

Change

On PeeweeStorage init, if the file fails PRAGMA quick_check:

1. Copy it aside as <path>.corrupt-<UTC> (plus -wal/-shm if present).
2. Try sqlite3 .recover when sqlite_dbpage is available (typical on macOS).
3. Otherwise use sqlite3 .bail off .dump and rewrite ROLLBACK → COMMIT.
   Debian/Ubuntu sqlite3 is commonly built without SQLITE_ENABLE_DBPAGE_VTAB,
   so .recover fails there with no such table: sqlite_dbpage.
4. Reconstruct any eventmodel.bucket_id rows missing from bucketmodel as
   recovered-<key> so the datastore can open after a dump that salvaged events
   but not bucket metadata.
5. Replace the live file only after the recovered copy itself passes
   PRAGMA quick_check. WAL/SHM on the live path are removed first so the old
   log cannot be applied to the new file.

Disable with AW_SQLITE_AUTO_RECOVER=0. If recovery cannot run (no sqlite3
CLI, or both strategies fail), the original file is left in place and the error
includes the manual commands.

Tests

tests/test_sqlite_recover.py:

• xor-corrupt a real Peewee DB after WAL checkpoint; startup recovers 30/30 events
• healthy DB is a no-op (no sidecar)
• AW_SQLITE_AUTO_RECOVER=0 raises without copying
• dump sanitizer rewrites ROLLBACK and drops corruption markers

Full suite: 201 passed, 2 skipped.

Notes

This does not change the on-disk schema of a healthy database. Recovered bucket
rows may be named recovered-<key> when the bucket page was the corrupt one;
events are kept.

[greptile-apps]

Confidence Score: 5/5

The PR appears safe to merge.

Summary

The PR adds automatic startup recovery for malformed Peewee SQLite databases while preserving the original database and validating the recovered copy before replacement.

• Attempts SQLite .recover, dump-based recovery, and a Python row-copy fallback.
• Reconstructs missing bucket metadata so salvaged events remain reachable.
• Preserves restrictive POSIX permissions before recovered activity data is written.
• Adds corruption, schema-reconstruction, fallback, and permission tests.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[PeeweeStorage startup] --> B{PRAGMA quick_check passes?}
    B -- Yes --> C[Open database normally]
    B -- No --> D[Copy database and sidecars aside]
    D --> E[Create secured recovery inode]
    E --> F[Try sqlite3 .recover]
    F -->|Failure| G[Try sanitized sqlite3 dump]
    G -->|Failure| H[Try Python row copy]
    F -->|Success| I[Reconstruct missing buckets]
    G -->|Success| I
    H -->|Success| I
    I --> J[Validate integrity and event reachability]
    J -->|Valid| K[Remove stale journals and replace live DB]
    J -->|Invalid| L[Preserve original and report recovery instructions]

Loading

_{Reviews (5) · Last reviewed commit: "fix(datastore): unique corrupt sidecars;..."}

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

CI-green and mergeable (Greptile 5/5) — waiting only on a maintainer click.

This PR is ready to merge, but the bot has pull-only access to this repo and can't self-merge — surfacing it here so it isn't lost. The monitoring loop will stop re-flagging it now that this note is posted.

[TimeToBuildBob]

🤖 AI code review

Safe to merge — 2 findings disposed (rejected)

_{Updated after inline dispositions on finding threads — this is the current state; the verdict below is frozen at review time and is kept as the historical record of that pass.}

Finding disposition

Finding	Severity	State
aw_datastore/storages/sqlite_recover.py:517	P1	fixed in 9f76f5b
aw_datastore/storages/sqlite_recover.py:272	P1	rejected — The verification said AutoField uses sqlite_sequence and that skipping sqlite_% tables therefore resets the counter. Tha
aw_datastore/storages/sqlite_recover.py:448	P1	rejected — The verification step claims peewee DateTimeField "will raise ValueError when converting to a datetime object" and there
aw_datastore/storages/sqlite_recover.py:341	P2	accepted-tradeoff
aw_datastore/storages/sqlite_recover.py:107	P2	fixed in 9f76f5b
aw_datastore/storages/sqlite_recover.py:124	P2	accepted-tradeoff
aw_datastore/storages/sqlite_recover.py:191	P2	rejected
aw_datastore/storages/sqlite_recover.py:287	P2	fixed in 1756bf9
aw_datastore/storages/sqlite_recover.py:351	P2	rejected
aw_datastore/storages/sqlite_recover.py:415	P2	fixed in 1756bf9
aw_datastore/storages/sqlite_recover.py:124	P2	accepted-tradeoff
aw_datastore/storages/sqlite_recover.py:190	P2	fixed in 74a59eb
aw_datastore/storages/sqlite_recover.py:348	P2	accepted-tradeoff
aw_datastore/storages/sqlite_recover.py:122	P2	superseded by latest review (not reproduced)

Not safe to merge — 2 P1 open

Confidence 2/5

_{ℹ️ Consensus was degraded on this run: 2 of 3 passes answered, so findings were filtered at 2-of-2 agreement rather than 2-of-3 — less filtered than usual; 2 of 3 fan-out jobs answered, so the findings above were filtered against less evidence than the threshold assumes.}

3 findings · ❌ 2 P1 · ⚠️ 1 P2

❌ P1 high — aw_datastore/storages/sqlite_recover.py:448

In _reconstruct_missing_buckets, when bucketmodel is missing, the code creates it with _ensure_bucketmodel and then runs the LEFT JOIN to find missing buckets. However, the created bucketmodel has a UNIQUE index on id, and the reconstructed rows use id = f"recovered-{key}". If the same key appears multiple times in the missing list (it is SELECT DISTINCT, so no), but if the eventmodel has multiple distinct bucket_id values, each gets a unique id. That is fine. However, the _assert_recovered_schema function is called after _reconstruct_missing_buckets in maybe_recover_malformed_sqlite, and it checks for missing buckets again. Since _reconstruct_missing_buckets inserted rows for all missing keys, the second check should pass. But there is a subtle bug: _reconstruct_missing_buckets only inserts rows for bucket_id values that are not NULL. If eventmodel has a NULL bucket_id, the LEFT JOIN condition e.bucket_id IS NOT NULL excludes it, so it is not reconstructed, and _assert_recovered_schema also excludes NULL bucket_id, so it passes. But peewee's EventModel.bucket is a ForeignKeyField with NOT NULL? In the schema, bucket_id is INTEGER NOT NULL, so NULL cannot happen. Not a real issue. Another issue: _reconstruct_missing_buckets uses datetime.now(timezone.utc).isoformat() for the created field, which produces a string like '2026-01-01T12:00:00.123456+00:00'. Peewee's DateTimeField expects a datetime object or a string in a specific format? When reading, BucketModel.json calls iso8601.parse_date(self.created), which can parse ISO 8601 with offset, so that is fine. But when peewee writes a DateTimeField, it stores a string in the format '%Y-%m-%d %H:%M:%S.%f+00:00' (see dt_plus_duration). The reconstructed created string has a 'T' separator and a '+00:00' offset, which iso8601 can parse, but peewee's DateTimeField may not be able to read it back? Actually peewee's DateTimeField uses its own format for storage and retrieval; when reading, it parses the string with datetime.strptime using the format '%Y-%m-%d %H:%M:%S.%f' or similar. If the string has a 'T' and '+00:00', peewee may fail to parse it, causing BucketModel.json to raise. Let me check: peewee's DateTimeField stores datetime objects as strings using the database's date format, which for SQLite is typically 'YYYY-MM-DD HH:MM:SS.SSS'. When reading, it uses the same format. The reconstructed created string '2026-01-01T12:00:00.123456+00:00' does not match that format, so peewee's DateTimeField will raise ValueError when converting to a datetime object. This would break bucket.json() for reconstructed buckets, causing the API to fail when listing buckets. The test test_reconstruct_creates_bucketmodel_when_missing does not read the created field through peewee; it only checks the raw row. The test test_peewee_startup_recovers_xor_corrupt_db uses a bucket that is not reconstructed (the original bucket row survives), so it does not hit this. This is a real bug: reconstructed buckets have a created timestamp in a format peewee cannot parse, so any code that calls bucket.json() on a reconstructed bucket will crash. The fix is to format the created timestamp as peewee expects, e.g., datetime.now(timezone.utc).strftime('%Y-%m-%d %H:%M:%S.%f+00:00').

Consider using CAST(e.bucket_id AS INTEGER) or a more lenient match, or reconstruct buckets based on distinct bucket_id values regardless of type.

_{How this was verified: Checked peewee's DateTimeField storage format in peewee.py: dt_plus_duration uses '%Y-%m-%d %H:%M:%f+00:00'. The reconstructed created uses isoformat() with 'T' and '+00:00', which peewee's DateTimeField will not parse back. BucketModel.json calls iso8601.parse_date, but peewee's model loading happens before that, and peewee's DateTimeField.db_value/ python_value will raise on the 'T' format.}

_{Consensus: 2/2 passes agreed}
Distinct keys: 1 (general)

❌ P1 high — aw_datastore/storages/sqlite_recover.py:342

In _recover_with_python, the function copies rows from the source to the destination using INSERT OR IGNORE. However, the destination table is created from the source's CREATE TABLE DDL, which may include constraints such as NOT NULL, UNIQUE, or foreign keys. If the source has a UNIQUE index on a column and the recovery copies rows that violate uniqueness (e.g., due to corruption causing duplicate keys), INSERT OR IGNORE will silently skip those rows, losing data. More importantly, the function does not disable foreign keys on the destination connection. If the source has foreign key constraints (peewee's eventmodel has a foreign key to bucketmodel), and the recovery copies eventmodel rows before bucketmodel rows, the INSERT OR IGNORE will fail on foreign key violations and skip all event rows, resulting in a recovered database with no events. The order of table creation and row copying is based on sqlite_master order, which is typically table creation order, but bucketmodel is created before eventmodel in peewee, so that may be fine. However, if the source is corrupt and the bucketmodel table is unreadable, the eventmodel rows will be copied first and fail FK constraints. The code does not set PRAGMA foreign_keys=OFF on the destination, so this is a real risk. The test test_recover_with_python_skips_uncreatable_tables does not cover FK constraints.

try:
                    mx = src_con.execute(f"SELECT max({pk_ident}) FROM {ident}").fetchone()
                    ids = list(range(1, int(mx[0]) + 1)) if mx and mx[0] else []
                except (sqlite3.Error, ValueError):
                    return 0

_{How this was verified: Checked the destination connection creation: dest_con = sqlite3.connect(dest) without PRAGMA foreign_keys=OFF. Peewee's eventmodel has a ForeignKeyField, so the DDL includes a foreign key constraint. If bucketmodel is missing or unreadable, eventmodel inserts will fail.}

_{Consensus: 2/2 passes agreed}
Distinct keys: 1 (general)

⚠️ P2 medium — aw_datastore/storages/sqlite_recover.py:204

In _recover_with_dbpage, the timeout handling kills both processes and then calls communicate(timeout=10) on each, but the first communicate() call on dump already timed out with RECOVER_TIMEOUT_SEC=300, and the second communicate() on dump uses timeout=30. If the dump process is stuck ignoring SIGKILL (uninterruptible sleep) or the pipe is full, the bounded reaps can still hang, but more importantly the code does not handle the case where load.communicate(timeout=300) times out while dump.communicate(timeout=30) is still running. The except block kills both, but if dump.communicate(timeout=30) itself raises TimeoutExpired, it is caught by the same except, and the loop then calls communicate(timeout=10) on both, which is bounded. However, the initial load.communicate(timeout=300) may have already consumed the timeout, and the subsequent dump.communicate(timeout=30) is a second timeout window, so total time can be 300+30+10+10 = 350 seconds, which is bounded but long. The real issue is that if the dump process produces a huge output and the load process is slow, the dump.communicate(timeout=30) can time out even though the dump is still producing, and the code then kills both and returns False, discarding a potentially recoverable database. This is a trade-off, not a bug, because the timeout is intentional. However, there is a more concrete bug: after the timeout, the code calls _truncate(dest) only in the returncode branches, not in the timeout branch. In the timeout branch, it returns False without truncating dest, so dest may contain a partially loaded database. Then _try_recover sees _recover_with_dbpage returned False, and proceeds to _recover_with_dump, which calls _truncate(dest) at the start? Actually _recover_with_dump does not truncate dest before loading; it runs sqlite3 dest with input=sql, which will append to the existing partial file. If dest already has a partial schema, the dump load may fail with 'table already exists' or similar, causing recovery to fail. The fix is to truncate dest in the timeout branch before returning False. This is a real bug: a timeout in .recover leaves a partial dest that poisons the subsequent .dump attempt.

After kill(), use proc.wait(timeout=10) in a loop, and if still alive, consider escalating or logging. Alternatively, use subprocess.run with timeout for the whole pipeline.

_{How this was verified: Traced _recover_with_dbpage: timeout branch returns False without _truncate(dest). _try_recover then calls _recover_with_dump, which does not truncate dest before loading. A partial dest from .recover can cause the dump load to fail with 'table already exists'.}

_{Consensus: 2/2 passes agreed}
Distinct keys: 1 (general)

Previous review passes

commit	score	findings	engine	when
c544da7d2614	2/5	6	llm	2026-09-16 09:46 UTC
9f76f5b8eb97	4/5	4	llm	2026-09-16 12:01 UTC
1756bf932b62	4/5	3	llm	2026-09-16 13:22 UTC

_{Reviewed 74a59ebae7b9 · openrouter/deepseek/deepseek-v4-flash-0731 · llm (in-band) engine · 308s · about this reviewer}

_{Maintainer commands}

_{@TimeToBuildBob review (own line) — fresh review · @TimeToBuildBob fix — a worker acts on the findings. Once per comment; 👀 = received.}

[TimeToBuildBob]

@greptileai review