Skip to content

fix(repair): harden recovery flows and serialize writers - #1407

Open
mjc wants to merge 6 commits into
MemPalace:developfrom
mjc:fix/repair-performance
Open

fix(repair): harden recovery flows and serialize writers#1407
mjc wants to merge 6 commits into
MemPalace:developfrom
mjc:fix/repair-performance

Conversation

@mjc

@mjc mjc commented May 7, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

This PR hardens MemPalace repair and recovery for damaged Chroma palaces and reduces rebuild time on the common recovery path.

Rebased on 2026-05-15 onto upstream/develop at bb31396. Since the original PR, upstream has already landed overlapping repair hardening for rebuild progress/ETA and empty-metadata sanitization. This PR now focuses on the remaining repair-performance and writer-safety pieces that are still unique to this branch.

Current delta after rebase

  • Harden Chroma startup preflight/reconnect behavior so invalid HNSW metadata and stale HNSW state are handled consistently after palace replacement or reconnect.
  • Make repair and recovery honor the configured primary collection name instead of assuming mempalace_drawers.
  • Speed up legacy and from-SQLite rebuilds by reusing stored embeddings when complete, staging into a temporary collection, swapping by rename, and validating source/destination inventory before reporting success.
  • Add a shared per-palace write boundary so repair, mine, repair cleanup, MCP write tools, and direct backend mutators do not race rebuild/swap paths. MemPalace-managed writes against the same palace no longer write the HNSW index concurrently.
  • Fold in review followups around repair-lock release, rebuild/swap failure boundaries, migration no-op behavior, and tests for the above flows.

User-visible outcomes

  • repair/recovery succeeds against more corrupted-palace states without silently dropping rows
  • custom collection names survive rebuilds correctly
  • healthy palaces do not prompt for destructive migration confirmation when no rebuild is needed
  • reconnect paths are more resilient to newly-invalid HNSW metadata
  • live writers no longer race repair/rebuild work
  • rebuilds can avoid expensive re-embedding when stored vectors are complete

Verification

After the rebase:

.venv/bin/python -m ruff check mempalace/config.py mempalace/palace.py mempalace/repair.py mempalace/mcp_server.py tests/test_repair.py
.venv/bin/python -m pytest tests/test_repair.py tests/test_palace_locks.py tests/test_mcp_server.py -q

Result: 272 passed; targeted ruff check clean.

Broader test plan

  • uv run pytest tests/test_backends.py tests/test_searcher.py tests/test_repair.py tests/test_migrate.py tests/test_cli.py tests/test_mcp_server.py tests/test_miner.py -q
  • Optional manual spot checks:
    • run mempalace repair against a palace with a non-default configured collection name
    • run mempalace migrate against a healthy readable palace and confirm it exits successfully without prompting for destructive confirmation
    • verify MCP writes queue/refuse appropriately while repair is active

Checklist

  • Targeted tests pass after rebase
  • Targeted linter check passes after rebase
  • No hardcoded paths

Copilot AI review requested due to automatic review settings May 7, 2026 16:51
@mjc
mjc force-pushed the fix/repair-performance branch from d79acba to fc276a8 Compare May 7, 2026 16:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens MemPalace’s Chroma repair/recovery paths by staging rebuilds into temporary collections (with rename swap + inventory validation), honoring configured collection names, and introducing a shared per-palace write lock to serialize repair/mine/MCP writes and prevent rebuild/write races.

Changes:

  • Repair/recovery: stage into uniquely-named temp collections, swap by rename, reuse stored embeddings when available, and validate inventories/counts (plus new repair-status artifact detection + cleanup).
  • Concurrency: add palace_write_lock and integrate it across miners, repair, MCP tools, and backend write wrappers.
  • Migration & tests: adjust migrate() behavior (no-op on healthy palaces; dry-run avoids probe writes), plus extensive regression/behavioral test coverage.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
mempalace/repair.py Implements staging+swap rebuild flows, SQLite extraction/vector reuse, inventory validation, repair-status artifacts/cleanup, and repair write-locking.
mempalace/palace.py Introduces palace_write_lock (blocking/non-blocking) and maps mine_palace_lock onto it for shared serialization.
mempalace/backends/chroma.py Adds ChromaCollection.modify() wrapper and adjusts quarantine preflight gating semantics.
mempalace/mcp_server.py Uses configured collection name safely and serializes MCP write tools behind the palace write lock.
mempalace/cli.py Adds from-SQLite confirmation helper, repair-status CLI flags, and forwards new repair options.
mempalace/migrate.py Skips write/delete probe in dry-run; avoids prompting on healthy palaces; filters synthetic probe drawers in SQLite fallback.
tests/test_repair.py Adds extensive coverage for new repair/recovery behaviors (temp swap, locking, vectors reuse, inventory validation, artifacts, archive restore).
tests/test_palace_locks.py Covers conflict/compat between mine lock and shared writer lock, plus blocking + Windows behavior.
tests/test_mcp_server.py Verifies MCP tools wait/serialize correctly behind palace write lock.
tests/test_cli.py Adds coverage for new CLI confirmation + repair-status + repair error/lock-handling paths.
tests/test_migrate.py Updates/extends migration behavior tests (healthy no-op, dry-run semantics, probe drawer filtering).
tests/test_backends.py Adds coverage for modify() delegation + lock acquisition; updates quarantine gating tests.
tests/test_searcher.py Adds regression coverage for candidate_strategy API stability and minor assertion formatting.
Comments suppressed due to low confidence (1)

mempalace/repair.py:52

  • REPAIR_TEMP_COLLECTION appears unused and is now misleading since temp collections are generated via _unique_temp_collection_name(...) rather than a fixed ...__repair_tmp name. Consider removing this constant or updating it to reflect the new naming scheme to avoid future callers relying on the old value.
COLLECTION_NAME = "mempalace_drawers"
REPAIR_TEMP_COLLECTION = f"{COLLECTION_NAME}__repair_tmp"


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread mempalace/repair.py Outdated
Comment thread mempalace/repair.py
Comment thread mempalace/mcp_server.py Outdated
Comment thread mempalace/mcp_server.py Outdated
@mjc
mjc force-pushed the fix/repair-performance branch 2 times, most recently from df5a4e9 to b182a4d Compare May 7, 2026 17:22
@igorls igorls added bug Something isn't working storage area/cli CLI commands labels May 8, 2026
@mjc

mjc commented May 9, 2026

Copy link
Copy Markdown
Contributor Author

I'm still experiencing occasional corruption but this cut it down a lot. unsure if its worth taking yet though.

@mjc
mjc force-pushed the fix/repair-performance branch from b182a4d to a54a02f Compare May 15, 2026 21:40
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.

3 participants