fix(repair): add --mode from-sqlite to recover palaces with corrupt HNSW (#1308)

[potterdigital]

What does this PR do?

Closes #1308.

Both mempalace repair --mode legacy and the inline cli.cmd_repair rebuild path call Collection.count() as their first read — exactly the call that raises chromadb.errors.InternalError: Failed to apply logs to the hnsw segment writer on the corruption class reported in #1308. Repair would print "Cannot recover — palace may need to be re-mined from source files" even though the underlying SQLite tables were fully intact (the corruption lives in the on-disk HNSW index files, not the data layer).

This PR adds --mode from-sqlite that reads (id, document, metadata) rows directly from chroma.sqlite3 via a segments → embeddings → embedding_metadata join, never opens a chromadb client against the corrupt palace, and re-upserts everything into a fresh palace under the user's configured embedding function.

CLI surface

mempalace repair --mode from-sqlite                                  # in-place; refuses to clobber dest
mempalace repair --mode from-sqlite --archive-existing               # archive then rebuild in place
mempalace repair --mode from-sqlite --source PATH                    # extract from PATH into --palace
mempalace repair --mode from-sqlite --source PATH --palace OTHER     # explicit cross-palace rebuild

--archive-existing renames the existing palace to <palace>.pre-rebuild-<timestamp> before reading from it. The original is never deleted.

Safety scaffolding

A reviewer scrutiny pass surfaced several issues that all needed handling before this could land safely:

• Confirm gate: any path that touches an existing palace dir is gated behind confirm_destructive_action (matching the legacy mode's existing protection). Pure cross-palace extraction into a non-existent dir is the only non-destructive path and skips the prompt.
• Source-validate-before-archive: an earlier draft archived the dest first and validated source second, leaving users with a renamed empty dir to manually undo when --source pointed at a non-palace dir. Validation now runs first.
• Partial-rebuild rollback: if a chromadb upsert fails mid-batch, the function raises RebuildPartialError carrying the failed collection name, partial counts, dest path, and archive path. CLI catches it and exits non-zero with the recovery instructions printed inline. No silent partial palace; no exit-zero on data loss.
• SharedSystemClient.clear_system_cache() blast radius: chromadb's process-wide System registry holds onto pre-archive state, so in-place mode would otherwise fail on create_collection with "Collection already exists". The call is now scoped to in-place mode only — cross-palace rebuilds never touch it. Docstring carries an explicit warning against calling rebuild_from_sqlite from inside a long-running mempalace process holding live PersistentClient references for other palaces.
• Re-embedding correctness: documents are re-embedded under the user's configured EF (the original HNSW vectors live in the corrupt data_level0.bin and cannot be recovered). The model is deterministic so search results remain semantically equivalent. Round-trip test asserts both drawer and closet document text + metadata come back intact.

How to test

Recovery was verified end-to-end on a 52,300-row real-world corrupt palace (48,199 drawers + 4,101 closets) before this PR was opened. After rebuild, mempalace status returned the same wing/room breakdown, and mempalace search "supabase migration" returned cosine-scored hits.

Automated:

pytest tests/test_repair.py -v
ruff check .

11 new tests in tests/test_repair.py, all using real tmp_path chromadb fixtures rather than mocks (the bug class is "extraction sees different rows than chromadb stored" — only an end-to-end fixture proves that). Coverage:

Test	What it catches
extract_via_sqlite_returns_all_rows_with_metadata	broken JOIN, wrong segment scope, dropped chroma:document key
extract_via_sqlite_preserves_typed_metadata	column-resolution rule picking the wrong typed column for int/float/bool
extract_via_sqlite_unknown_collection_yields_nothing	dropped WHERE c.name=? filter (seeds two collections, queries a third)
extract_via_sqlite_missing_palace_yields_nothing	regression on the missing-file early-return
rebuild_from_sqlite_roundtrips_via_real_chromadb	row drops, mangled metadata, swapped doc bodies, closet text re-embedding
rebuild_from_sqlite_refuses_existing_dest	silent overwrite of an existing palace
rebuild_from_sqlite_in_place_archives_when_opted_in	empty rebuild from reading the wrong path post-archive; archive content preservation
rebuild_from_sqlite_in_place_refuses_without_archive_flag	catastrophic silent deletion of the only data copy
rebuild_from_sqlite_source_missing_chroma_db	partial state at dest on bad input
rebuild_from_sqlite_in_place_validates_source_before_archiving	archive-before-validate ordering bug
rebuild_from_sqlite_raises_on_upsert_failure	exit-zero on partial rebuild — uses monkeypatch to inject a chromadb upsert failure

Full suite: 1481 passed, 1 skipped on python -m pytest tests/ --ignore=tests/benchmarks.

Checklist

• Tests pass (python -m pytest tests/ -v)
• No hardcoded paths
• Linter passes (ruff check .)
• CHANGELOG entry under [3.3.5] — unreleased (v3.3.4 was already tagged from release/3.3.4)

🤖 Generated with Claude Code

[mjc]

Small compatibility thought: this hardcodes mempalace_drawers, which may need to line up with the configured collection-name work in #1312.

Would it be safer for the drawer collection to come from MempalaceConfig().collection_name, with mempalace_closets appended if closets are
intentionally fixed? A regression test with collection_name = "custom_drawers" would make this pretty clear.

[potterdigital]

Good call. Pulled drawers through MempalaceConfig().collection_name via a small _drawers_collection_name() helper. Closets stays hardcoded since the AAAK layer references drawer IDs by string and isn't per-deployment configurable. Tried to mirror #1312's pattern so it should rebase cleanly when that lands. Added a regression test that monkeypatches the resolver and seeds custom_drawers_xyz end-to-end.

[mjc]

This looks worth tightening: create_collection() happens before the try, so failures here won’t be wrapped as RebuildPartialError.

That matters for the in-place flow because the PR description calls out a possible Collection already exists failure if Chroma’s process cache
still sees the pre-archive collection. If that happens after the original palace was moved aside, the CLI would not get the structured
archive_path / partial-count recovery message.

Would it make sense to move create_collection() inside the try so collection-create failures are reported the same way as upsert failures?

[potterdigital]

Fair point, moved it. Now a create_collection failure (including "Collection already exists" from a stale System cache) surfaces as RebuildPartialError with archive_path instead of a bare exception.

[mjc]

Should this backend be closed in a finally?

This path creates a ChromaBackend() and can open a PersistentClient for the destination palace. Since #1285 had to be careful about active Chroma
handles during repair rollback/restore, it seems safer to make the lifecycle explicit here too:

backend = ChromaBackend()
try:
    ...
finally:
    backend.close()

[potterdigital]

Done — wrapped the backend in try/finally backend.close() covering all exit paths (success, RebuildPartialError, unexpected). Went with try/finally over with since ChromaBackend doesn't implement the context manager protocol. The from-sqlite path post-dates #1285's lifecycle hardening so it needed its own cleanup here.

[mjc]

One CLI behavior question: rebuild_from_sqlite() returns {} for validation/refusal cases like missing source DB or existing destination, and this
command then returns normally with exit code 0.

For an explicitly requested repair command, would those cases be better as non-zero exits? That would make scripts/CI distinguish “nothing was
rebuilt because inputs were invalid” from a successful recovery.

This looks like it applies to the other validation returns just below this one too.

[potterdigital]

Agreed. Library still returns {} for validation refusal vs. {name: 0, ...} for a legitimately empty rebuild — that contract is documented now — and the CLI converts {} to sys.exit(1). Two new tests pin both behaviors so the distinction can't silently regress.

[mjc]

Small test-clarity thought: since this recovery path depends on Chroma’s internal SQLite segment layout, could one of the real-Chroma tests assert
which segment scope the recovered embeddings rows are attached to?

I initially wondered whether this should use the VECTOR segment because the HNSW repair/status code finds the vector segment to inspect the on-disk
HNSW files. Looking more closely, though, the document/metadata rows may correctly live under Chroma’s METADATA segment while the HNSW files live
under VECTOR.

A concrete assertion in test_extract_via_sqlite_returns_all_rows_with_metadata would make that assumption durable, e.g. query segments.scope for
the extracted row IDs and assert it is the expected scope. That would help future repair work avoid accidentally changing this to the wrong segment.

[potterdigital]

Done. Added a JOIN against segments/collections for the extracted row IDs that asserts scope == {'METADATA'}. You read the layout exactly right — METADATA holds doc/metadata rows, VECTOR holds the HNSW files. If anyone ever points the extraction JOIN at VECTOR, this fails loudly instead of silently regressing recovery.

[mjc]

This is a really thoughtful recovery path, and I like the shape of it a lot. Bypassing Chroma entirely when the HNSW side is the thing that’s broken feels like the right answer for this class of failure, especially since the user’s verbatim data is still sitting intact in chroma.sqlite3.

I’m not a maintainer here, just some person reading along, so please take these as questions rather than gatekeeping. I left a few inline thoughts, but they’re mostly small “can we make this even harder to misuse or regress?” questions around the edges: configured collection names, early collection-creation failures, backend handle cleanup, validation exit behavior, and making the Chroma segment-layout assumption explicit in tests.

Overall, this seems like a strong direction.

[igorls]

Hey @mjc please check the failing tests on Windows

[mjc]

The failing tests are hitting a handle-lifecycle gap in the in-place rebuild_from_sqlite() path, not a rebuild-data-path issue. On Windows, data_level0.bin stays locked if the active Chroma backend / shared system cache is still holding the palace open during rollback / restore cleanup.

The relevant fix is already in #1285, specifically commit f57f300 (fix(repair): close active backend before rollback restore). That change fixes the rollback path so we close the active backend instance before restoring the archived palace, instead of instantiating a fresh backend while the real one can still hold file handles open. That is the exact class of failure showing up here with WinError 32 on data_level0.bin.

There is also a separate write-boundary issue here: repair/rebuild paths should take the palace write lock around the destructive replace / restore portion so Chroma and SQLite state cannot change across the swap boundary. I’m working on that separately; it is related to repair safety, but distinct from this Windows handle-release fix.

[igorls]

Rebased onto develop 6741b69. Two text-collision conflicts, both list-position rather than semantic:

• CHANGELOG.md: combined the v3.3.5 bug-fix list (HEAD already had today's seven-PR batch entries; appended this PR's entry for fix(repair): rebuild_index bails on col.count() — exactly the call HNSW corruption breaks #1308)
• tests/test_cli.py: both branches added new test sections at end-of-file — kept HEAD's stdio-reconfigure tests (from fix(cli, fact-checker): reconfigure stdio to UTF-8 on Windows #1282) and this PR's cmd_repair_from_sqlite exit-code tests side-by-side

mempalace/cli.py and mempalace/repair.py auto-merged.

Verified locally:

• uvx ruff@0.4 check . and ruff format --check . clean
• Targeted: pytest tests/test_repair.py tests/test_cli.py — 107 passed
• Full suite: pytest tests/ --ignore=tests/benchmarks — 1566 passed, 1 skipped in 59s

The Windows test failure on the prior CI run should also clear: today's #1105 (Chroma backend now releases the SQLite file lock on close_palace/close) addressed the same class of file-handle leak that was tripping Windows on rmtree-then-reopen flows. CI re-run on this push will confirm.

[igorls]

Maintainer-edit pushed to unblock for v3.3.5 (due tomorrow):

1. Merged develop — branch was 6 commits behind and was about to revert the auto-save tools shipped in docs: add 30-day expiry callout + ship 4 auto-save tools #1391.
2. Fixed test-windows failures in test_rebuild_from_sqlite_in_place_archives_when_opted_in and test_rebuild_from_sqlite_raises_on_upsert_failure.

Root cause: the _seed_palace test helper opened a ChromaBackend and never closed it. chromadb's rust-side SQLite/HNSW file locks survived the helper's return, so on Windows the in-place archive rename inside rebuild_from_sqlite hit WinError 32 on data_level0.bin. Linux/macOS tolerate the open handles; Windows does not.

Fix wraps the seed in try/finally: backend.close(). No test was consuming the returned collection, so dropping the return is safe.

Linux full suite: 1568 passed, ruff lint/format clean against the 0.4.x CI pin. Watching CI for the Windows job.