fix(repair): run SQLite integrity preflight before chromadb open (follow-up to #1364) - #1403
Conversation
#1364 added the SQLite quick_check preflight to rebuild_index, but placed it AFTER backend.get_collection(...). On a SQLite-corrupt palace, chromadb's rust binding raises pyo3_runtime.PanicException — which is not a regular Exception subclass — so it propagates past the existing `except Exception` handlers and the user sees a 30-line stack trace instead of the friendly abort message #1364 was designed to deliver. Reproduced with `mempalace repair --yes` against a palace whose chroma.sqlite3 has 4 mangled pages: pre-fix, panic; post-fix, the clean abort message and exit code 1. Two changes: - mempalace/cli.py cmd_repair: run sqlite_integrity_errors() right after the basic palace-existence check, BEFORE the max_seq_id preflight (which itself opens sqlite3) and BEFORE backend = ChromaBackend(). Exit non-zero so unattended scripts and CI gates see the failure. - mempalace/repair.py rebuild_index: same move at the function level for direct callers (tests, MCP) that bypass cmd_repair. The new test test_rebuild_index_runs_sqlite_preflight_before_chromadb_open uses a real chromadb-built palace (no ChromaBackend mock) plus a real corrupt SQLite (16 KB of mangled pages) so the ordering is exercised end-to-end. The previously-shipping test for the abort path mocked both the backend and sqlite_integrity_errors, which is why the ordering bug shipped CI-green. Six existing test_cli.py cmd_repair tests used `(palace_dir / "chroma.sqlite3").write_text("db")` to fake the SQLite file. The new preflight correctly fails quick_check on those 2-byte stubs, so the tests now create empty real SQLite DBs the same way the test_repair.py fixtures already do.
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression in the repair flow where the SQLite integrity preflight (PRAGMA quick_check) ran after opening ChromaDB, allowing Chroma’s Rust binding to panic on corrupted SQLite pages before MemPalace could print the intended friendly abort message.
Changes:
- Move SQLite
quick_checkpreflight earlier inrepair.rebuild_index()(before any ChromaDB open). - Add the same earlier preflight to
cli.cmd_repair, exiting with code 1 on corruption so scripts/CI can detect failure. - Add a regression test that uses a real ChromaDB palace + deliberately corrupted SQLite DB; update CLI tests to create real empty SQLite files instead of 2-byte stubs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
mempalace/repair.py |
Runs SQLite integrity preflight before any ChromaDB client open in rebuild_index(). |
mempalace/cli.py |
Adds early SQLite integrity preflight to cmd_repair() and exits non-zero on corruption. |
tests/test_repair.py |
Adds regression test ensuring preflight runs before ChromaDB open using real corruption. |
tests/test_cli.py |
Updates repair CLI tests to create valid SQLite DB files so the new preflight doesn’t fail on stubs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert pre_size > 16384, "need a multi-page sqlite db to mangle" | ||
|
|
||
| with open(sqlite_path, "r+b") as f: | ||
| f.seek(40960) # page 10 |
There was a problem hiding this comment.
Good catch — fixed in 7b15103.
Replaced the unconditional f.seek(40960) with a page-aligned offset computed from pre_size. Now there are explicit asserts that the file holds at least HEADER_GUARD + CORRUPT_BYTES (12 KB), the offset is rounded down to a 4 KB page boundary, and it never lands within the first two pages so the file still opens. If a future SQLite/chromadb version returns a smaller initial DB the test fails with a clear assertion instead of silently extending the file.
Address Copilot review on #1403: the test seeked unconditionally to offset 40960 with only `pre_size > 16384` as a guard. If pre_size sat between 16384 and 40960 + 16384 = 57344 (e.g., on a chromadb version that allocated fewer pages on init, or a future schema change), the seek would extend the file with zero-padding and the original pages would stay intact — quick_check would still pass on the (untouched) real data, and the regression guard would silently skip detecting a preflight-ordering regression. Compute the offset from pre_size, page-aligned, with explicit asserts that the file is large enough to mangle 4 pages without truncating the header or extending past EOF.
Follow-up to #1364 found via manual smoke test of v3.3.5 develop tip.
What does this PR do?
#1364 added the SQLite `PRAGMA quick_check` preflight to `rebuild_index`, but placed it AFTER `backend.get_collection(...)`. ChromaDB's rust binding raises `pyo3_runtime.PanicException` on a malformed page — and PanicException is not a regular `Exception` subclass, so it propagates past the existing `except Exception` handlers. The user gets a 30-line stack trace instead of the friendly abort message #1364 was designed to deliver.
Repro
Pre-fix: `pyo3_runtime.PanicException: Expect it to be fetched: Database(SqliteError { code: 11, message: "database disk image is malformed" })` and a long Python traceback.
Post-fix: the `ABORT: SQLite-layer corruption detected before repair rebuild` message with the documented recovery steps, and exit code 1.
What changed
Why the bug shipped CI-green
The previously-shipping abort-path test (`test_rebuild_index_aborts_on_sqlite_integrity_errors_before_delete_collection`) mocks both `ChromaBackend` and `sqlite_integrity_errors`. With both mocked, the ordering of the preflight relative to the real chromadb open never gets exercised — the test passes regardless of whether the preflight runs before or after the (mocked, never-crashing) `get_collection` call.
New test
`test_rebuild_index_runs_sqlite_preflight_before_chromadb_open` uses:
I verified the test fails on the pre-fix code (panic) and passes on the fixed code (clean abort).
Test fixture cleanup
Six existing `test_cli.py` `cmd_repair` tests used `(palace_dir / "chroma.sqlite3").write_text("db")` to stub the SQLite file. The new preflight correctly fails quick_check on those 2-byte stubs, so the tests now create empty real SQLite DBs the same way the `test_repair.py` fixtures already do.
Local verification
```
1624 passed, 1 skipped (was 1623 before this PR's new test)
ruff lint + format clean against the 0.4.x CI pin
```
Closes nothing on its own — fixes a regression in already-merged #1364. Fits naturally in v3.3.5.