The original sweep in this PR was scoped against develop as of April. Since
then upstream added console output carrying the same characters, so the fix
had drifted into covering roughly half of the affected surface.
Re-measured against current develop and replaced the remaining occurrences of
the same five characters this PR already targets -- U+2014 em-dash, U+2500 box
drawing, U+2192 arrow, U+25CF/U+25CB circles -- on every print(), input() and
logger call reachable from a terminal:
repair.py (10), cli.py (7), format_miner.py (7), mcp_server.py (5),
miner.py (2), onboarding.py (2), migrate.py (2), searcher.py (2),
service.py (1), embedding.py (1), dialect.py (1)
Substitutions match the ones already used in this branch: em-dash -> "--",
box drawing -> "-", arrow -> "->", filled/empty circles -> "#"/".".
Four assertions matched the previous literals and were updated alongside the
strings they cover (test_cli, test_miner x2, test_repair).
Docstrings, comments and stored content are deliberately untouched -- only
what reaches a terminal, where GBK/CP1252 consoles raise UnicodeEncodeError.
The default (legacy) repair path ignored --dry-run and ran the real
rebuild: it deleted any existing <palace>.backup, copied the live palace
over it, re-filed the drawers collection through a staged temp copy, then
rebuilt FTS5 and VACUUMed. #2095 and #2133 fixed this for
--mode from-sqlite only.
The preview now returns before ChromaBackend() is constructed. Opening a
chromadb client is itself a write to chroma.sqlite3, so a preview that
reached one could not be inert; the row count comes from
sqlite_drawer_count instead, the read-only SQLite ground truth
check_extraction_safety already trusts. Staying off the chromadb layer
also keeps a dry run clear of the layer repair is separately reported to
segfault in on a large palace (#2113).
resolve_repair_preflight_errors() decides what a dry run does about the
FTS5 autoheal. The autoheal is a write, so a preview must not run it, but
skipping it routed an isolated inverted-index error into the abort banner
and exit 1 - telling the operator to run offline sqlite3 .recover on a
palace a real run heals by itself (#1596). The dry run now classifies the
errors with the same _errors_are_isolated_fts5 predicate the real path
gates on and continues; broader corruption still aborts in both modes.
It is worded as an attempt rather than a promise, because the real heal
still gives up when another process holds the mine lock, when the rebuild
raises, or when quick_check is still dirty afterwards.
The plan describes the real run in execution order. It names the
live-collection delete the rebuild performs, since "re-file via a staged
temp copy" alone reads as additive; it warns when an existing backup
would be deleted; and it reports a no-op instead of a rebuild when the
collection holds no rows. It states the #1208 truncation guard that can
abort the run, and reports that guard as disabled when
--confirm-truncation-ok is set, because check_extraction_safety returns
immediately then and the promised abort would not happen. An unreadable
count fails closed with a non-zero exit, for parity with the from-sqlite
preview.
from-sqlite ignored --dry-run and performed the real archive+rebuild
(#2095, #2133). Preview after source validation, skip the destructive
confirm, never take the mine-lock or rename the palace, and fail closed
when SQLite row counts are unreadable instead of inventing zeros.
Addresses fatkobra's CHANGES_REQUESTED review on this PR. The prior
truncation-detection fix in _paginate_ids left two loud-failure gaps:
1. When BOTH the offset get() and its no-offset fallback failed, the
`except Exception: break` returned the partial (or empty) ids
collected so far as if the listing were complete. On a mid-run
failure that hands scan_palace/rebuild a truncated palace; on the
first page it returns [] and scan_palace prints "Nothing to scan."
for a palace it never read. Now raises RuntimeError (chained from
the underlying error) instead of breaking.
2. The count()-based truncation disambiguation used the collection-wide
col.count(), but _paginate_ids accepts a where= filter (scan_palace
passes {"wing": only_wing}). A global count greater than the
collected rows does not prove a *filtered* result is truncated -- the
extra rows may belong to other wings. count() takes no where=
argument, so the raise is now gated on `where is None`; filtered
completeness is treated as unknown rather than false-positive-raised.
Adds three regression tests covering the after-progress double failure,
the first-page double failure, and the exactly-page-size filtered case.
hnsw_capacity_status() (chroma.py) exists precisely to preflight the
#1222 SIGSEGV/pyo3-panic class before anything touches the HNSW
segment, but repo-wide it was wired into only 4 call sites while raw
count()/collection.count() is called at 20+ others -- a bare
except Exception around count() cannot catch a native crash, since
the process dies regardless of any Python try/except. This wires the
existing, already-tested probe into the 7 remaining call sites the
audit identified as CRITICAL:
- #89 palace.py::_enforce_embedder_identity -- the universal
get_collection() chokepoint every tool passes through, previously
guarded only by except Exception. Highest leverage: skips this
bookkeeping-only check on divergence instead of risking count().
- #90 migrate.py::migrate -- routes straight to the same
SQLite-extraction fallback the except branch already used, instead
of ever reaching col.count() when diverged.
- #91 repair.py::scan_palace / prune_corrupt -- both abort with the
existing from-sqlite recovery guidance instead of opening the
collection.
- #10 repair.py::rebuild_index -- preflights divergence alongside its
existing sqlite-integrity and poisoned-max-seq-id preflights, before
opening the collection.
- #13 repair.py::rebuild_index never rebuilt or reported on the
closets collection -- now warns when closets is still diverged
after a drawers-only rebuild, pointing at --mode from-sqlite instead
of letting 'Repair complete' stand unqualified.
- #92 dedup.py::get_source_groups -- takes an optional palace_path
(threaded from both callers) to preflight before count(); omitted by
existing tests, which keep their pre-existing behavior.
- #93 miner.py::status -- preflights before the ChromaDB-client
fallback path (used when the direct sqlite read is unavailable).
7 new regression tests, each confirmed failing against the pre-fix
code (via git stash of the source files only) and passing after the
fix. One existing dedup.py test updated for the new palace_path kwarg
in its call-signature assertion. Full suite: 3154 passed, 1 unrelated
pre-existing flake (test_mcp_server.py peer-writer-lock module-global
state leaking across test files in full-suite ordering -- this diff
never touches mcp_server.py).
extract_via_sqlite drove its extraction with an INNER JOIN starting at
embedding_metadata. Any embedding with zero rows there (a sparse
historical write with no chroma:document and no other key -- the same
condition _extract_drawers already sanitizes for the collection-layer
rebuild path, see #1458) never appeared in the join result and was
silently dropped by mempalace repair --mode from-sqlite, with no count
mismatch and no warning.
Drive the query from embeddings (LEFT JOIN embedding_metadata) instead,
defaulting to an empty metadata dict when a row has none. Order by
e.id rather than em.id since em.id is NULL for unmatched rows.
Regression test seeds a real chromadb collection, then strips one
drawer's metadata rows directly via SQLite (current chromadb validates
against empty metadata on write, so this only reproduces on rows
already sitting in an older palace) and asserts the drawer still
comes back through the SQLite bypass.
Bare (#8)/#12: markers were internal finding IDs from a local audit
report, not GitHub references -- but they collide with real closed
issues in this repo (#8, #12) and would misdirect reviewers. Replaced
with plain prose.
Two independent bugs from the 2026-07-28 full-repo audit:
1. _rebuild_collection_via_temp deleted its own verified temp collection
in the except-handler even when the live collection had already been
replaced (live_replaced=True) -- at that point the temp copy is the
ONLY intact data left. Now preserved, and the error message points the
operator at it.
rebuild_index's own recovery path used to 'restore' by copying back a
pre-rebuild chroma.sqlite3 file backup and claim the palace was back
to pre-repair state -- but the live collection's on-disk HNSW segment
directories were already destroyed by the delete, so the restored
sqlite3 referenced segment UUIDs that no longer existed. Replaced with
a new _promote_temp_collection() that recovers by re-extracting from
the verified temp copy and re-uploading into a fresh live collection,
with an honest fallback message if that promotion also fails.
2. _paginate_ids silently returned a truncated ID list when offset-based
pagination failed and the no-offset fallback got structurally stuck
at the first page (1000 results). Callers (scan_palace, rebuild) treat
the result as the complete palace, so this could act on incomplete
data with no warning. Now cross-checks against the collection's own
count() to disambiguate a genuinely-complete page-sized collection
from a truncated larger one, and raises instead of silently truncating
when it can't tell.
Both fixes verified against an independent adversarial review pass
(mutation-tested: confirmed the new tests catch a wrong-source-collection
read and a swapped ids/documents payload in the recovery path, which the
original test set did not).
Deliberately out of scope: none of the rebuild paths hold mine_palace_lock
for their full duration, so a concurrent writer can still interleave with
a rebuild. That's a separate, larger architectural gap (queue-and-replay
vs. block-on-write), tracked for its own follow-up.
shutil.move's fallback for a failed os.rename is copytree + rmtree. On
Windows, when any file inside the palace is held open by another
process (a live MCP server, a running mine, another harness), the
rename fails and shutil.move falls back to deleting the live palace
file-by-file via rmtree -- which itself then fails partway through on
the first locked file, leaving the palace partially gutted next to a
partial (or empty) archive copy.
Reproduced live twice (Windows 11, 2026-07-05 and 2026-07-06): running
`mempalace repair --mode from-sqlite --yes --archive-existing` while
an MCP server / detached mine held palace/*/data_level0.bin open threw
mid-rmtree in both cases. The palace itself survived only because the
specific locked files could not be unlinked -- a different lock
pattern (e.g. a lock on a file rmtree reaches first) would have lost
data with no way back.
os.rename is atomic on both platforms it matters on (POSIX rename(2),
Windows MoveFileEx) -- it either fully succeeds or fails without
touching anything. Catch the failure and abort cleanly with actionable
guidance instead of a raw traceback.
_errors_are_isolated_fts5 gated auto-heal on one specific message shape:
malformed inverted index for FTS5 table
SQLite >= ~3.5x (confirmed on 3.53.2 / Python 3.13.7) reports the same
isolated-FTS5 condition with different wording instead:
fts5: corruption found reading blob N from table "embedding_fulltext_search"
The narrow regex never matched this phrasing, so maybe_autoheal_fts5_index
silently declined to heal on any machine running a recent-enough SQLite,
falling straight through to the hard-abort path -- the exact condition
the whole auto-heal feature (#1926/#1928) exists to avoid. Widened the
pattern to match either wording.
Caught by running this repo's own test suite on this machine:
test_repair.py's two auto-heal tests were failing (not, as assumed
earlier, pre-existing/unrelated flakiness -- that assumption was never
actually verified). Traced to this exact classification gap.
Fixing this correctly also exposed that four tests in
test_miner_fts5_validation.py had been passing for the wrong reason: they
manufacture the exact "reporter-shaped" isolated-FTS5 corruption (#1926's
actual bug shape) and asserted mine() must raise MineValidationError for
it -- true only because the classifier bug prevented auto-heal from ever
engaging. With the classifier fixed, that corruption is now correctly
auto-healed and mine() succeeds instead, so those tests' expectations
were stale, not their fixtures being invalid:
- test_helper_raises_on_fts5_segment_corruption -> renamed
test_helper_auto_heals_fts5_segment_corruption; asserts no raise + a
clean post-heal quick_check, instead of expecting a raise.
- test_full_chain_raises_through_mine_impl and
test_mine_impl_does_not_print_partial_summary_on_validation_error: their
real purpose is exception-passthrough / banner-suppression when the
validator DOES raise, not proving any particular corruption triggers it.
Switched from real file corruption to a monkeypatched raise. (Tried
swapping to _page_mangle's non-isolated corruption first -- that made
ChromaDB's own Rust bindings panic just opening the file for the
re-mine's get_collection() call, a native crash rather than a catchable
Python exception, before the validator ever ran. Different failure mode
than what these tests are about, and not reliable to depend on.)
- test_mine_formats_full_chain_raises_when_fts5_corrupt: same fix, mirrors
the miner-path change for the extract path.
- Added test_full_chain_auto_heals_isolated_fts5_corruption and
test_mine_formats_full_chain_auto_heals_isolated_fts5_corruption as
companions, proving the full mine()/mine_formats() chain -- not just
the standalone validator -- actually auto-heals and succeeds end-to-end
for the isolated case now that it's correctly classified.
- test_errors_are_isolated_fts5_classification: added the new message
wording as an explicit regression fixture (pinned literally, not
dependent on whatever this machine's SQLite happens to emit).
Full suite: 3302 passed, 20 skipped, 0 failed -- first fully clean run
this session. ruff check / ruff format -- clean.
* fix(chroma): require SQLite magic header for ChromaBackend.detect() (#1893)
Closes#1893.
ChromaBackend.detect() was returning True for a 0-byte chroma.sqlite3 file
because the check was just os.path.isfile(...). On a palace that has any
other backend marker alongside a stale 0-byte chroma.sqlite3,
resolve_backend_name then raises BackendMismatchError and the palace becomes
unopenable until the user manually rm's the empty file.
The 0-byte file appears as a side effect of any sqlite3.connect() on a
missing path — Python creates the file immediately but writes the SQLite
header only on the first statement. So any code path that touches the
chroma.sqlite3 path with bare sqlite3.connect(), including chromadb's own
PersistentClient lazy-init (see the comment at backends/chroma.py:2052),
can leave a 0-byte artifact behind.
Fix: detect() now reads the first 16 bytes and compares to the SQLite
magic prefix b"SQLite format 3\x00" instead of relying on file presence
alone. One extra open() + 16-byte read; detect() isn't a hot path.
Properties:
- Rejects 0-byte files (the symptom #1893 is about).
- Rejects non-SQLite garbage at the canonical path (partial writes, etc.).
- Doesn't false-negative on real chroma palaces: any chroma palace whose
PersistentClient has done any work has the magic header on disk
(verified — CREATE TABLE is enough to land the header).
- Doesn't couple detect() to chroma's specific schema; the magic header
is stable across chromadb releases.
Test sweep: many test files used (chroma.sqlite3).touch() or
.write_bytes(b"") as a "fake palace" shortcut, exploiting the loose
isfile() check (one such site even had the comment "# pass the isfile
guard"). After this change, those stand-ins no longer register as chroma
palaces. Introduced tests/_chroma_palace_helper.py::make_minimal_chroma_sqlite
following the existing _backend_conformance.py precedent, and updated 15
call sites across 8 test files to use it. The existing
test_chroma_detect_matches_palace_with_chroma_sqlite (which encoded the
buggy semantics with write_bytes(b"")) is renamed to
test_chroma_detect_matches_palace_with_sqlite_header and now writes a
real SQLite database via the helper. Added two new tests for the
rejection paths (empty file, non-SQLite garbage).
Full env-cleared suite: 3137 passed, 20 skipped, 0 failed. ruff check
and ruff format --check both clean.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KC5Qsknh2zFRtRvVyjXiTA
* fix(sqlite_exact): require SQLite magic header for SQLiteExactBackend.detect()
Per gemini-code-assist review on #1892 PR #1896: SQLiteExactBackend has the
same os.path.isfile() detection pattern as ChromaBackend did, with the same
0-byte-file vulnerability. Mirrors the chroma fix for repo-wide consistency.
- SQLiteExactBackend.detect() now does the same 16-byte SQLite magic-prefix
check as ChromaBackend.detect().
- _chroma_palace_helper.py: factored its body into a private
_write_minimal_sqlite_file() and gained a sibling
make_minimal_sqlite_exact_sqlite() for the sqlite_exact filename. No churn
to any existing chroma call sites.
- test_sqlite_exact_backend.py:426 (the one site that wrote b"" for
sqlite_exact.sqlite3) updated to use the new helper.
- Three new tests in test_sqlite_exact_backend.py mirror the chroma trio:
matches with valid header, rejects empty file, rejects non-SQLite garbage.
Full env-cleared suite: 3140 passed, 20 skipped, 0 failed.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KC5Qsknh2zFRtRvVyjXiTA
---------
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Concurrent killed-mid-write mines can leave embedding_fulltext_search in a
malformed-inverted-index state that fails PRAGMA quick_check while the
underlying rows stay intact (integrity_check ok). The repair preflight then
hard-aborts before reaching the FTS5 rebuild step, so `mempalace repair`
refuses to run and full-text search stays broken — the exact loop #1596
reports. The MineValidationError banner even promises "repair --yes rebuilds
the FTS5 virtual table automatically," which the preflight abort made false.
Add maybe_autoheal_fts5_index(): when every quick_check error is an isolated
"malformed inverted index for FTS5 table" failure, rebuild the index in place
from the intact embedding_fulltext_search_content table
(INSERT ... VALUES('rebuild')) under mine_palace_lock, then re-run quick_check.
The rebuild touches no drawer rows. Wired into both repair preflights
(rebuild_index and cli cmd_repair). Any non-FTS5 error in the set, a lock held
by a live mine, or a rebuild that does not clear quick_check leaves the errors
unchanged so the caller still aborts with the recovery banner — broader
corruption is never silently rebuilt over.
- ruff format llm_client.py and miner.py (lint job)
- _copy_file_no_follow: close src fd if the dst open fails (no leak), and
route the rebuild restore through it so backup + restore share one
no-follow/regular-file path
- update repair tests to assert the unified hardened copy instead of the
removed shutil.copy2 calls; backup paths are now timestamped
- update normalize large-file test to stub fstat (size is checked on the
open fd, not via a pre-open os.path.getsize)
When the chromadb compactor cannot apply the WAL into the drawers HNSW
segment (InternalError: Failed to apply logs to the hnsw segment writer),
the legacy repair paths fail on their first Collection.count() read and
advise re-mining from source files. The drawer rows are intact in
chroma.sqlite3, so repair --mode from-sqlite rebuilds them; re-mining
silently drops drawers added via the MCP server and diary entries that
have no source file.
Both legacy read-failure sites (cmd_repair and rebuild_index) now emit
shared guidance pointing at the from-sqlite recovery, worded conditionally
so it also covers a live server or mine still holding the palace open.
Co-Authored-By: undeadindustries <9536461+undeadindustries@users.noreply.github.com>
mempalace migrate (.pre-migrate.* full-palace copies) and mempalace repair
max-seq-id (chroma.sqlite3.max-seq-id-backup-* DB copies) each wrote a fresh,
full-size, timestamped backup every run and never deleted the old ones. On a
machine that mines or repairs on a schedule, those copies could silently
accumulate until they filled the disk.
Add a configurable max_backups setting (default 10; env MEMPALACE_MAX_BACKUPS
or config.json) and a shared prune_backups helper that trims the oldest copies
after each new backup is written. Pruning is keyed by filesystem mtime, scoped
strictly to each backup's own naming pattern so live data is never touched, and
best-effort so a deletion failure can never abort the migrate/repair that just
succeeded. Set max_backups to 0 to keep every backup.
Repeated `repair --yes` runs leave freed SQLite pages unreclaimed and
can corrupt the FTS5 inverted index. This patch adds
`_vacuum_and_rebuild_fts5()`, called at the end of `rebuild_index()`,
which:
1. Closes all chroma handles (releases PersistentClient's sqlite lock).
2. Rebuilds the FTS5 index via the built-in content-table DDL trick.
3. Runs VACUUM (requires autocommit / isolation_level=None) to reclaim
the freed pages.
The function is a no-op when chroma.sqlite3 is absent or the FTS5
table doesn't exist, so it is safe on every supported backend.
Four new tests cover the happy path, missing FTS5 table, missing file,
and call ordering (close must precede vacuum).
Closes#1516Closes#1517
One-time mechanical reformat so `ruff format --check .` passes under the
newly pinned ruff. Layout only (assert-message parenthesization etc.),
no behavior change. 29 files: 28 under tests/ + 1 tools helper, no core
mempalace/ modules. Produced by `ruff format .`.
`mempalace status`, `search`, and `compress` printed the same misleading
"No palace found / Run: mempalace init" output for three distinct
palace states (no dir / no DB / no collection). Most user-visible on
the first-run state where `init` had run but `mine` had not: the hint
to re-run `init` is a no-op and wastes the user's time.
Backend gets a new typed exception `CollectionNotInitializedError`
(subclass of `PalaceNotFoundError`, transitively `FileNotFoundError`,
so legacy callers keep working). `ChromaBackend.get_collection(
create=False)` wraps chromadb's bare `NotFoundError` as the new typed
exception instead of leaking the chromadb-specific class to callers.
A new internal helper `_open_collection_or_explain` in `palace.py`
runs filesystem-first state checks before the backend call to avoid
chromadb's lazy `chroma.sqlite3` creation as a side-effect of a
read-only inspection, then catches the typed exceptions and prints a
state-specific actionable message. `BackendClosedError` is explicitly
re-raised so a programmer error is not masked as a UX hint.
Two CLI bug sites route through the helper: `miner.status` and
`cli.cmd_compress`. `searcher.search` catches the typed exceptions
directly so it can preserve the cause chain in `SearchError(...) from
e` for programmatic search-API consumers. `cli.cmd_sync` gained an
inline filesystem distinction (no helper needed: it does not use the
collection handle). `repair.status` (capacity check, which by design
must work on corrupted palaces without opening a chromadb client) got
the same distinction via `sqlite_drawer_count`-based empty detection.
The MCP `tool_status` is intentionally left alone: PR #831 already
fixed it there with a `create=True` bootstrap strategy appropriate
for programmatic clients.
Addresses @Copilot's review feedback on #1459. Five tests:
- test_extract_drawers_preserves_valid_metadata: non-empty dict
passes through unchanged (regression guard against breaking happy
path).
- test_extract_drawers_sanitizes_none_metadata: None entries
coerce to {"_repaired_empty_meta": True} (the core fix).
- test_extract_drawers_sanitizes_empty_dict_metadata: empty dict
{} entries also coerce to the sentinel (chromadb 1.5.x rejects
both shapes equally).
- test_extract_drawers_sanitization_preserves_alignment: critical
invariant — ids[i] / documents[i] / metadatas[i] stay in
lockstep through the sanitizer; mis-pairing would silently
corrupt rebuilds.
- test_extract_drawers_multiple_batches: pagination boundary
correctness (sanitizer applied per-batch, no drops/duplicates).
Verified passing locally against mempalace fork main + chromadb
1.5.8 in the palace-daemon venv (5 passed, 67 deselected in 1.88s).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Several tests opened sqlite3 connections without try/finally or
context-manager cleanup, relying on a flat conn.close() after the
work. Any assertion failure or exception between connect and close
leaked the connection until GC, producing
``ResourceWarning: unclosed database`` in CI logs.
On Python 3.13 / macOS the ResourceWarning isn't just noise: a
leaked connection can hold a SQLite advisory lock long enough for
later test setup to block on it, which appears to be the cause of
recent intermittent CI hangs on those two runners.
Wrap each affected ``conn = sqlite3.connect(...)`` block in
``contextlib.closing(...)`` so cleanup runs on the failure path too.
Mirrors the try/finally pattern already used in production code
(searcher.py, repair.py, backends/chroma.py).
No behavior change — same operations, same assertions, just
deterministic cleanup. All 162 affected tests pass locally.
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.
#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.
#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 #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.
Conflicts opened by #1285 (temp-staging rebuild) and #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 #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.
Three conflicts, all from develop landing #1285/#1310/#1312 after this
branch was authored:
- mempalace/cli.py: keep both import sets — this branch's
maybe_repair_poisoned_max_seq_id_before_rebuild plus develop's
RebuildCollectionError / _close_chroma_handles / _extract_drawers /
_rebuild_collection_via_temp added in #1285.
- mempalace/repair.py: keep this branch's
maybe_repair_poisoned_max_seq_id_before_rebuild definition; use
develop's rebuild_index signature with the collection_name parameter
added in #1312. Normalized print indent to 2 spaces matching the
rest of the file.
- tests/test_repair.py: keep both this branch's max_seq_id preflight
tests and develop's rebuild_from_sqlite + configured-collection-name
tests; they exercise distinct code paths and don't overlap.
Local: 1617 tests pass, ruff lint+format clean against 0.4.x CI pin.
The helper opened a chromadb PersistentClient via ChromaBackend and never
closed it, leaving rust-side SQLite/HNSW file locks alive after the
helper returned. On Windows that blocks the in-place archive rename
inside rebuild_from_sqlite with WinError 32 on data_level0.bin,
causing test_rebuild_from_sqlite_in_place_archives_when_opted_in and
test_rebuild_from_sqlite_raises_on_upsert_failure to fail in the
test-windows CI job. No test consumes the returned collection, so
closing the backend in a try/finally is safe and drops the return.
Five small hardening fixes for the from-sqlite rebuild path, all from
mjc's review on #1310:
- repair.py: drawers collection name now resolves from
MempalaceConfig().collection_name via _drawers_collection_name() (closets
stays fixed by design — AAAK index references drawer IDs by string).
Lines up with the broader configured-collection work in #1312 so that
PR can rebase cleanly on top.
- repair.py: create_collection() moved inside the try block in
_rebuild_one_collection so a Chroma "Collection already exists" failure
surfaces as RebuildPartialError with archive_path, not an unstructured
exception that strands the user without recovery instructions.
- repair.py: rebuild_from_sqlite wraps backend lifetime in try/finally
with backend.close() so PersistentClient handles to dest_palace are
released on every exit path. The from-sqlite path post-dates #1285's
lifecycle hardening of the legacy rebuild, so this needed its own
cleanup.
- cli.py: cmd_repair (from-sqlite mode) now exits non-zero when
rebuild_from_sqlite returns {} (validation refusal sentinel), so
unattended scripts/CI distinguish "invalid inputs" from a successful
rebuild that legitimately found zero rows.
- tests/test_repair.py: test_extract_via_sqlite_returns_all_rows_with_metadata
now asserts every backing segment is scope='METADATA', locking in the
segment-layout assumption against future regressions that point the
JOIN at the VECTOR segment.
New test coverage:
- test_rebuild_from_sqlite_honors_configured_drawer_collection_name
- test_cmd_repair_from_sqlite_validation_refusal_exits_nonzero
- test_cmd_repair_from_sqlite_success_does_not_exit
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both `--mode legacy` and the inline `cli.cmd_repair` rebuild path
call `Collection.count()` as their first read — the same 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 new `--mode from-sqlite` reads `(id, document, metadata)` rows
directly from `chroma.sqlite3` via `segments` → `embeddings` →
`embedding_metadata` joins, never opens a chromadb client against
the corrupt palace, and re-upserts everything into a fresh palace.
- `--source PATH` extracts from a corrupt palace already moved aside
- `--archive-existing` handles the in-place case by renaming the
existing palace to `<palace>.pre-rebuild-<timestamp>` first
- Partial-rebuild failures raise `RebuildPartialError` with the
archive path so users can recover; CLI exits non-zero
- In-place mode calls `SharedSystemClient.clear_system_cache()` to
drop chromadb's process-wide System registry (cross-palace use
does not, to limit blast radius for library callers)
- Source validation runs before any destructive moves
Verified end-to-end recovering a 52,300-row real-world corrupt
palace.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Rollback cleanup was instantiating a fresh ChromaBackend, so the live backend that had opened the PersistentClient could keep file handles alive during restore. Close the active backend instance instead so rollback and CLI recovery can release Windows-safe locks before copying the backup back into place.
`_compute_heuristic_seq_id` ran `int(row[0])` directly on the result
of `MAX(e.seq_id)`. On palaces where chromadb 1.5.x has been writing
seq_ids natively (8-byte big-endian uint64 BLOB), that raises
`ValueError: invalid literal for int() with base 10: b'...'` before
the dry-run can print, leaving users with no path through the
recovery feature added in #1135 — the only documented un-poison
route for palaces hit by the original PR #664 shim bug.
Decode BLOB return values via `int.from_bytes(val, "big")` and
keep the existing `int(val)` path for INTEGER rows. Regression
test seeds a BLOB row in `embeddings.seq_id` and asserts the
heuristic surfaces the correct integer.
The BLOB-seq_id migration shim (PR #664) ran int.from_bytes(..., 'big')
over every BLOB in max_seq_id, including chromadb 1.5.x's own native
format (b'\x11\x11' + 6 ASCII digits). That conversion yields a ~1.23e18
integer that silently suppresses every subsequent embeddings_queue write
for the affected segment (queue filter is seq_id > start), causing
silent drawer-write drops after a 1.5.x upgrade.
Two-part fix:
1. Shim narrowing (mempalace/backends/chroma.py)
- Drop max_seq_id from the shim loop. chromadb owns that column's
format; we don't reinterpret it.
- Defense-in-depth: skip rows in embeddings whose seq_id BLOB has the
sysdb-10 b'\x11\x11' prefix rather than misconvert.
2. Recovery command (mempalace/repair.py, mempalace/cli.py)
- mempalace repair --mode max-seq-id [--segment <uuid>]
[--from-sidecar <path>] [--dry-run] [--yes] [--no-backup]
- Detects poisoned rows via threshold (seq_id > 2**53).
- Default heuristic: MAX(embeddings.seq_id) over the collection owning
the poisoned segment. Matches METADATA max exactly; VECTOR segments
get a few seq_ids ahead (queue skips an already-indexed window — an
acceptable loss vs. resetting to 0 and re-processing everything).
- --from-sidecar copies clean values from a pre-corruption sqlite db.
- Backs up chroma.sqlite3, closes chroma handles, atomic UPDATEs,
post-repair verification that raises MaxSeqIdVerificationError if
any row is still above threshold.
Tests: 8 new in tests/test_repair.py (detection, heuristic, sidecar,
dry-run, segment filter, no-op, backup, rollback-on-verify-failure).
3 new in tests/test_backends.py (max_seq_id untouched by shim,
sysdb-10 prefix skipped in embeddings, legacy big-endian u64 BLOBs
still convert). Full suite: 1103 passed.
The user-reported case in #1208: a palace with 67,580 drawers had its
HNSW files manually quarantined to recover from corruption. ``mempalace
repair`` then ran cleanly and reported "Drawers found: 10000 ... Repair
complete. 10000 drawers rebuilt." Backup was the v3.3.3 chroma.sqlite3
that did contain the full 67,580 — but the rebuilt collection only had
the first 10K. 85% data loss, no warning.
Root cause: ChromaDB's collection-layer get() silently caps at
``CHROMADB_DEFAULT_GET_LIMIT = 10_000`` rows when reading from a
collection whose segment metadata is stale (typical post-quarantine
state). col.count() returns the same capped value, so neither the
loop bound nor the extraction count flagged the truncation.
Fix is defense-in-depth, not a recovery mechanism. Repair now:
1. After extraction, queries chroma.sqlite3 directly via a read-only
sqlite3 connection: COUNT(*) FROM embeddings JOIN segments JOIN
collections WHERE name='mempalace_drawers'. If that count exceeds
the extracted count, abort with a clear message before any
destructive operation.
2. Falls back to a weaker check when the SQLite query can't run
(chromadb schema drift, locked file): if extracted exactly equals
CHROMADB_DEFAULT_GET_LIMIT, that's a strong-enough cap signal to
refuse without explicit acknowledgement.
3. Adds ``--confirm-truncation-ok`` (CLI) and ``confirm_truncation_ok``
(rebuild_index kwarg) to override after independent verification.
Useful for the rare case of a palace genuinely sized at exactly
10,000 drawers.
The guard logic lives in ``repair.check_extraction_safety()`` so the
two extraction paths (CLI ``cmd_repair`` and the lower-level
``rebuild_index``) share a single implementation. Raises
``TruncationDetected`` carrying the printable message.
Tests: 9 new cases covering the safe path (counts match, SQLite
unreadable but well under cap), both abort paths (SQLite higher than
extracted, unreadable + at cap), the override flag, and end-to-end
behavior of ``rebuild_index`` with the guard wired in. Plus two
``sqlite_drawer_count`` tests for the missing-file and bad-schema
cases.
What's NOT in this PR: actually recovering the missing 57,580
drawers from the user's case. The on-disk SQLite still holds them;
recovery is a separate flow (direct-extract from chroma.sqlite3,
bypass the chromadb collection layer entirely). This PR's job is
to stop repair from making it worse.
Refs #1208.
Prerequisite for RFC 001 (plugin spec, #743). Removes every direct
`import chromadb` outside the ChromaDB backend itself so the core
modules depend only on the backend abstraction layer.
Extends ChromaBackend with make_client, get_or_create_collection,
delete_collection, create_collection, and backend_version. Adds
update() to the BaseCollection contract. Non-backend callers
(mcp_server, dedup, repair, migrate, cli) now go through the
abstraction; tests patch ChromaBackend instead of chromadb.
With this landed, the RFC 001 spec can be enforced and PalaceStore
(#643) can ship as a plugin without touching core modules.
Arnold Wender
mvalentsev
Igor Lins e Silva
KeilerHirsch
J.R. Murray
Pim Messelink
fatkobra
margaretjgu
William Blair
jp
Mika Cohen
Brian potter
eblander
bensig
MSL