Skip to content

fix(repair): preflight SQLite integrity before rebuild - #1364

Merged
igorls merged 3 commits into
MemPalace:developfrom
fatkobra:fix/1362-repair-sqlite-integrity-preflight
May 7, 2026
Merged

fix(repair): preflight SQLite integrity before rebuild#1364
igorls merged 3 commits into
MemPalace:developfrom
fatkobra:fix/1362-repair-sqlite-integrity-preflight

Conversation

@fatkobra

@fatkobra fatkobra commented May 5, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #1362.

This makes mempalace repair --yes run a SQLite PRAGMA quick_check before it reaches Chroma's destructive delete_collection() rebuild step.

If SQLite reports corruption, repair now stops with a clear recovery message instead of surfacing an opaque Chroma/SQLite code 779 traceback from inside delete_collection().

What changed

  • Added sqlite_integrity_errors().
    • Opens chroma.sqlite3 read-only.
    • Runs PRAGMA quick_check.
    • Returns any non-ok messages as repair-blocking errors.
  • Added print_sqlite_integrity_abort().
    • Prints a clear explanation that SQLite-layer corruption was detected.
    • Points the user toward offline SQLite recovery before re-running repair.
  • Wired the preflight into repair.rebuild_index() after extraction/truncation safety and before:
    • backing up chroma.sqlite3
    • backend.delete_collection(...)
    • collection recreation
  • Added regression tests for:

Why

#1362 reports a palace where HNSW was not the corrupt layer. The HNSW segment looked healthy, but repair --yes crashed inside Chroma's delete_collection() with:

(code: 779) database disk image is malformed

## How to test
ruff format mempalace/repair.py tests/test_repair.py
ruff check mempalace/repair.py tests/test_repair.py
python -m pytest tests/test_repair.py -q
python -m pytest tests/ -v

## Checklist
- [x] Tests pass (`python -m pytest tests/ -v`)
- [x] No hardcoded paths
- [x] Linter passes (`ruff check .`)

Conflicts opened by MemPalace#1285 (temp-staging rebuild) and MemPalace#1312
(collection_name in recovery paths) merging after this branch was
authored.

mempalace/repair.py:
- Kept this branch's sqlite_integrity_errors() and
  print_sqlite_integrity_abort() helpers; took develop's rebuild_index
  signature with the collection_name parameter from MemPalace#1312. Normalized
  the helper's print indent to 2 spaces to match the rest of the file.

tests/test_repair.py:
- Kept both this branch's sqlite_integrity_errors tests and develop's
  rebuild_from_sqlite + configured-collection coverage.
- Replaced 7 sites of sqlite_path.write_text("fake") with
  sqlite3.connect(...).close() — write_text("fake") fails PRAGMA
  quick_check, so the new preflight aborts before the rebuild logic
  the tests actually exercise. An empty real SQLite DB passes
  quick_check and lets the tests run as intended.
- Took develop's temp-staging assertion shape (delete/create the
  __repair_tmp collection in addition to the live drawers collection)
  for the existing test_rebuild_index_success test.

Local: 1618 tests pass, ruff lint+format clean against 0.4.x CI pin.
@igorls igorls added this to the v3.3.5 milestone May 7, 2026
@igorls

igorls commented May 7, 2026

Copy link
Copy Markdown
Member

Maintainer-edit: merged develop in to resolve conflicts opened by #1285 (temp-staging rebuild) and #1312 (collection_name plumbing) after this branch was authored.

  • `mempalace/repair.py`: kept this branch's `sqlite_integrity_errors()` and `print_sqlite_integrity_abort()`; took develop's `rebuild_index` signature with the `collection_name` parameter. Normalized the helper's print indent to 2 spaces to match the rest of the file.
  • `tests/test_repair.py`: kept both this branch's preflight tests and develop's `rebuild_from_sqlite` / configured-collection coverage. Took develop's temp-staging assertion shape for `test_rebuild_index_success`.
  • Test fixture update: 7 `test_rebuild_index_*` tests used `sqlite_path.write_text("fake")` — that fails PRAGMA quick_check, so the new preflight aborted before the rebuild logic the tests actually exercise. Replaced with `sqlite3.connect(str(sqlite_path)).close()` (empty real SQLite DB that passes quick_check), preserving the tests' intent.

Local: 1618 tests pass, ruff lint+format clean against the 0.4.x CI pin.

MemPalace#1357 (max_seq_id preflight) merged into develop while this branch
was in CI, opening a fresh conflict between the two preflight helpers.

mempalace/repair.py:
- Kept both: this branch's sqlite_integrity_errors() / print_sqlite_
  integrity_abort() AND develop's maybe_repair_poisoned_max_seq_id_
  before_rebuild() from MemPalace#1357. They check for distinct corruption
  classes and run as separate preflights.

tests/test_repair.py:
- Kept both this branch's sqlite_integrity_errors test group and
  develop's max_seq_id preflight test group; non-overlapping coverage.

Local: 1623 tests pass, ruff lint+format clean against 0.4.x CI pin.
@igorls

igorls commented May 7, 2026

Copy link
Copy Markdown
Member

Round-2 merge: #1357 (max_seq_id preflight) merged into develop while this branch was in CI, opening a fresh conflict between the two preflight helpers. Kept both — they check distinct corruption classes:

Both run as separate preflights inside `rebuild_index`; non-overlapping coverage in tests too. Local: 1623 tests pass on the merge state, ruff lint+format clean.

@igorls
igorls merged commit f38d9eb into MemPalace:develop May 7, 2026
6 checks passed
igorls added a commit that referenced this pull request May 7, 2026
fix(repair): run SQLite integrity preflight before chromadb open (follow-up to #1364)
mjc pushed a commit to mjc/mempalace that referenced this pull request May 7, 2026
MemPalace#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 MemPalace#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/cli CLI commands bug Something isn't working storage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

repair --yes aborts on SQLite-layer index corruption (code 779) before reaching HNSW rebuild

2 participants