Skip to content

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

Open
TimeToBuildBob wants to merge 8 commits into
ActivityWatch:masterfrom
TimeToBuildBob:fix/sqlite-corrupt-recover
Open

fix(datastore): auto-recover malformed peewee SQLite on startup#154
TimeToBuildBob wants to merge 8 commits into
ActivityWatch:masterfrom
TimeToBuildBob:fix/sqlite-corrupt-recover

Conversation

@TimeToBuildBob

Copy link
Copy Markdown
Contributor

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 ROLLBACKCOMMIT.
    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.

Preserve a corrupt peewee-sqlite.v2.db as <path>.corrupt-<UTC> and replace
it with a recovered copy so aw-server can start instead of restart-looping.

Uses sqlite3 .recover when sqlite_dbpage is available; otherwise a sanitized
.bail-off .dump (Debian/Ubuntu sqlite3 is built without dbpage). Reconstructs
any eventmodel.bucket_id rows missing from bucketmodel. Disable with
AW_SQLITE_AUTO_RECOVER=0.

Git-Session-Id: 08b7
@greptile-apps

greptile-apps Bot commented Sep 1, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds automatic startup recovery for malformed Peewee SQLite databases while preserving the original file and validating recovered state before replacement.

  • Detects malformed databases with PRAGMA quick_check and supports CLI and Python recovery strategies.
  • Reconstructs missing bucket metadata so salvaged events remain accessible.
  • Secures the recovery inode before writing and preserves the original database permissions.
  • Adds coverage for recovery, disabled recovery, partial schemas, and permission handling.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the previously reported schema-reachability and temporary-file permission issues are fixed in the current code.

Important Files Changed

Filename Overview
aw_datastore/storages/peewee.py Invokes malformed-database recovery before initializing the Peewee connection.
aw_datastore/storages/sqlite_recover.py Implements guarded recovery, bucket reconstruction, integrity validation, secured temporary-file handling, and atomic replacement.
tests/test_sqlite_recover.py Covers healthy and corrupt startup paths, fallback recovery, partial schemas, opt-out behavior, and POSIX permission preservation.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[PeeweeStorage startup] --> B{Database healthy?}
    B -->|Yes| C[Open normally]
    B -->|No| D{Auto-recovery enabled?}
    D -->|No| E[Raise recovery instructions]
    D -->|Yes| F[Copy original and sidecars]
    F --> G[Create secured temporary inode]
    G --> H[Try sqlite3 recover]
    H -->|Unavailable or failed| I[Try dump recovery]
    I -->|Failed| J[Try Python row copy]
    H -->|Recovered| K[Reconstruct missing buckets]
    I -->|Recovered| K
    J -->|Recovered| K
    K --> L[Validate integrity and schema]
    L -->|Valid| M[Remove stale journals and replace live DB]
    L -->|Invalid| N[Delete temporary file and preserve original]
Loading

Reviews (4): Last reviewed commit: "fix(datastore): typecheck fchmod on Wind..." | Re-trigger Greptile

Comment on lines +88 to +90
if not is_sqlite_healthy(tmp_dest):
raise SqliteRecoverError("recovered file still fails PRAGMA quick_check")
_replace_live_db(path, tmp_dest)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Incomplete schemas pass recovery

When a corrupt dump retains eventmodel but omits bucketmodel, _reconstruct_missing_buckets returns without repair and quick_check still permits replacement. Peewee then recreates an empty bucket table, leaving the salvaged events unreachable while startup reports a usable datastore.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e93b303. _reconstruct_missing_buckets now creates bucketmodel (and the unique id index) when a dump salvaged eventmodel but omitted the catalog, then inserts recovered-<key> rows. _assert_recovered_schema refuses replacement if any salvaged events would still be unreachable after that.

# Drop WAL/SHM first so SQLite cannot apply the old log to the new file.
for suffix in ("-wal", "-shm", "-journal"):
_remove_if_exists(path + suffix)
os.replace(recovered, path)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 security Replacement drops database permissions

If the original database has stricter permissions than the process's current umask, os.replace installs the newly created recovery file without preserving those restrictions, broadening local read access to activity records. How this was verified: The recovery file is created as a new inode and the replacement path contains no mode-restoration step.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e93b303. _replace_live_db copies the original mode onto the recovered file with os.chmod before os.replace, so a restrictive 0600 database does not become world-readable under a permissive umask.

Windows CI has no sqlite3.exe, so CLI .recover/.dump never ran and the
xor-corrupt fixture failed. Copy schema plus surviving rows through the
stdlib sqlite3 module, reopening poisoned connections after DatabaseError.

Git-Session-Id: 08b7
Create bucketmodel when a corrupt dump salvages eventmodel but omits the
bucket catalog, then refuse replacement if events would still be
unreachable. Preserve the original file mode on os.replace so recovery
does not widen local read access under a permissive umask.
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

@greptileai review

Windows leaves non-empty -wal/-shm next to the .corrupt-<UTC> copy, so
glob(path + '.corrupt-*') matched three files and tripped assert len==1.

Git-Session-Id: 08b7
Comment thread aw_datastore/storages/sqlite_recover.py Outdated
Comment on lines +538 to +539
_preserve_mode(path, recovered)
os.replace(recovered, path)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 security Recovered file permissions remain exposed

If the original database is restrictive but the process umask is permissive, recovery populates .recovered-tmp before applying the original mode, exposing activity data to other local accounts during recovery. If os.chmod fails, _preserve_mode suppresses the error and os.replace installs the permissive inode permanently.

How this was verified: The recovery path writes the temporary database before line 538, and lines 525-539 suppress chmod failures before unconditionally replacing the live database.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ed07f01. Recovery now creates the temporary inode with the live database's mode and applies that exact mode with fchmod before any recovery strategy writes data. Permission setup errors propagate before data is written or replacement begins. Fallbacks truncate the same pre-secured inode instead of deleting and recreating it. Added tests for pre-write mode and fail-closed fchmod failure.

TimeToBuildBob added a commit to TimeToBuildBob/TimeToBuildBob.github.io that referenced this pull request Sep 1, 2026
ActivityWatch peewee SQLite auto-recover: restart loops on SQLITE_CORRUPT
while events are still readable. Portable fallback when sqlite_dbpage is
missing, dump ROLLBACK trap, open PR ActivityWatch/aw-core#154.

Git-Session-Id: e09f
Git-Session-Id: 5f4c1144-c5d9-5e51-a701-52c1c2fa45a9
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

@greptileai review

Git-Session-Id: 5f4c1144-c5d9-5e51-a701-52c1c2fa45a9
Git-Session-Id: 5f4c1144-c5d9-5e51-a701-52c1c2fa45a9
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

@greptileai review

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant