Make repair_sqlite_db actually repair a damaged database

[rtibblesbot]

Summary

Recovery of a corrupt notifications database has never worked. The repair returned before doing anything, because its guard compared against Django's stock SQLite backend and every Kolibri SQLite deployment sets its own; had it run, it would have dumped the database to a file it never closed and copied that still-empty file over the original. This makes it recover a damaged database instead — the dump is closed and replayed into a fresh file that is swapped over the original, and the connection is opened before it is dumped.

Fixing the guard makes a destructive path live for the first time, so this also narrows what counts as unrecoverable: a lock timeout no longer deletes the database it was asked to repair. The copy the old code took up front is now taken only immediately before the swap or the regeneration — the two points where data can be lost — and a repair that cannot take one is abandoned rather than run blind.

References

Fixes #15108. Defects found while planning #15096, merged as #15107.

Acceptance criteria

All six are met. Two are annotated rather than met verbatim, because they cite kolibri/core/sqlite/test/test_utils.py as an existing file: kolibri/core/sqlite/test/ does not exist on develop — the #15096 characterization tests were dropped before that PR merged — so there was no not_on_windows skip to remove and nothing to replace.

• repair_sqlite_db and common_clean recognise Kolibri's SQLite backend — both call _default_db_is_sqlite() (kolibri/core/sqlite/utils.py:18), the substring form kolibri/core/deviceadmin/utils.py:88 already used.
• The connection is opened before iterdump() — kolibri/core/sqlite/utils.py:97-104, guarded as dbbackup guards it.
• A database with tables survives with its rows intact — the with open(...) block ends before the dump is read back, and the replayed file is os.replaced over the original rather than copyfiled.
• The repair runs to completion on Windows — the dump handle is closed before os.replace and before the finally deletes it, and a sharing violation from os.replace is caught rather than escaping. No skip to remove: the new file carries no platform skip at all, so it runs on the Windows CI axis like every other test.
• A repair that genuinely cannot recover the data still reaches regenerate_database — by the DatabaseError clause and by the zero-tables check. Deliberately narrowed so that a transient sqlite3.OperationalError does not; see below.
• Tests cover a recoverable database and an unrecoverable one — 16 tests driving a real file-backed database. Nothing to replace: written fresh, since those characterization assertions never merged.

Reviewer guidance

Three of these are deliberate additions beyond the acceptance criteria. They exist only because fixing the guard makes the destructive path reachable, and each is a judgement call worth a second opinion:

• kolibri/core/sqlite/utils.py:111-113 — the one that most needs your answer. The rebuild is declined when a journal (-wal or -journal) outlives our close(), which is how we know another connection still holds the database open. Neither ordering around os.replace is safe there: dropping the journal loses every transaction still in it, and keeping it lets the next opener replay the damaged database's pages over the rebuilt one with valid checksums. Declining costs us the repair whenever another thread holds a notifications connection — the normal state in production. Confirm you want that trade rather than swapping anyway.
• kolibri/core/sqlite/utils.py:205 — sqlite3.OperationalError is now treated as "not corruption" and leaves the database alone, split out from the DatabaseError clause that regenerates. Since OperationalError subclasses DatabaseError, check nothing that genuinely is corruption reaches us as an OperationalError and now gets left unrepaired.
• kolibri/core/sqlite/utils.py:164-173 — _kolibri_repairing is set on the Django connection object to stop the reopen at :224 re-entering through connection_created. Check that a connection object is never shared between threads in a way that makes a flag on it the wrong place for this.

Not covered by the tests, so verified by hand against a real KOLIBRI_HOME: the tests drive the "default" alias, so regenerate_database's call_command("migrate", ...) branch — the only branch that runs in production — is exercised only manually. A garbage notifications.sqlite3 regenerated cleanly with no traceback and one repair attempt, and a healthy one with five seeded LearnerProgressNotification rows round-tripped through dump, replay and swap with all five rows, quick_check ok, journal_mode wal and no leftover files.

Known and not fixed here, each a follow-up candidate:

• Five of the six call sites repair without evidence of corruption — only kolibri/core/apps.py:71 checks quick_check first. A quick_check guard inside repair_sqlite_db would make it safe from all of them, and would also bound backups/, which is never pruned.
• regenerate_database runs call_command("migrate") on the default database from inside the transaction.atomic() at kolibri/core/notifications/tasks.py:62. Never executed before, because the guard blocked it.
• A corrupt notifications.sqlite3 crashes the CLI before Django app loading — sqlite_check_foreign_keys (kolibri/utils/database.py:88) lets sqlite3.DatabaseError escape — so this recovery is unreachable from a cold start. Pre-existing, and it defeats the recovery for exactly the case the recovery exists for.

AI usage

I used Claude Code to plan and implement this test-first, prompting it to follow the forms kolibri/core/deviceadmin/utils.py already had right — the substring engine check at :88 and the connection-open guard at :106-111 — rather than inventing new ones. I reviewed each of the three defects against the source before accepting the fix, pushed back on the initial version that swapped the file unconditionally, and verified with the new tests, the notifications/deviceadmin/coach suites on both the SQLite and postgres axes, prek, and the manual runs described above.

---

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

• Ran pre-flight CI checks (lint, format, tests) and verified all pass
• Rebased onto the target branch and resolved any conflicts
• Reorganized commit history into clean, logical commits
• Audited the diff to ensure only issue-relevant files are changed
• Built PR body from the repository's PR template with evidence blocks

Deviations from the issue spec

• Two criteria cite kolibri/core/sqlite/test/test_utils.py as existing — it does not exist on develop, so there was no not_on_windows skip to remove and no characterization assertions to replace. Annotated above rather than ticked verbatim.
• The issue asks only that an unrecoverable database reach regenerate_database; this narrows that so a transient sqlite3.OperationalError does not, because fixing the guard would otherwise let a lock timeout delete the database.
• The rebuild declines the swap when a journal survives our close(), and when no copy of the database can be taken first — neither is in the spec, both bound the destructive path the guard fix opens up. Rationale in Reviewer guidance.

🟡 Waiting for feedback Last updated: 2026-07-30 22:13 UTC

[github-actions]

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.19.6.dev2379_g67b5b8e7b.d20260730.pex
Windows Installer (EXE)	kolibri-setup-0.19.6.dev2379+g67b5b8e7b.d20260730-unsigned.exe
Debian Package	kolibri_0.19.6.dev2379+g67b5b8e7b-0ubuntu1_all.deb
Debian Package	kolibri-server_0.19.6~dev2379+g67b5b8e7b.d20260730-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.19.6.dev2379+g67b5b8e7b.d20260730.dmg
Android Package (APK)	kolibri-0.19.6.dev2379+g67b5b8e7b-debug.apk
Raspberry Pi Image	kolibri-pi-image-0.19.6.dev2379+g67b5b8e7b.zip
TAR file	kolibri-0.19.6.dev2379+g67b5b8e7b.d20260730.tar.gz
WHL file	kolibri-0.19.6.dev2379+g67b5b8e7b.d20260730-py3-none-any.whl

Smoke test screenshot

Download screenshot