Skip to content

fix(repair): honor --dry-run in repair --mode from-sqlite - #1654

Closed
potterdigital wants to merge 1 commit into
MemPalace:developfrom
potterdigital:fix/dry-run-from-sqlite
Closed

fix(repair): honor --dry-run in repair --mode from-sqlite#1654
potterdigital wants to merge 1 commit into
MemPalace:developfrom
potterdigital:fix/dry-run-from-sqlite

Conversation

@potterdigital

Copy link
Copy Markdown
Contributor

What does this PR do?

repair --mode from-sqlite silently ignored --dry-run. cmd_repair's from-sqlite branch never forwarded args.dry_run to rebuild_from_sqlite (the max-seq-id branch does), and rebuild_from_sqlite had no dry_run parameter. So mempalace repair --mode from-sqlite --dry-run did one of two non-preview things:

  • with --archive-existing (or an existing dest) it ran the real rebuild — archiving the live palace and re-embedding every row;
  • otherwise it aborted at the destructive-action confirmation prompt.

Neither previews. This matters most for the case from-sqlite exists to handle: a large corrupt palace where a real rebuild re-embeds 100k+ rows and runs for a long time — exactly when you want to look first.

This PR:

  • adds a dry_run parameter to rebuild_from_sqlite; after the normal source validation it prints per-collection row counts and returns them without archiving the existing palace, creating collections, or re-embedding. Counts come from sqlite_drawer_count — the same SQLite ground-truth helper repair already uses — so the preview matches what a real rebuild upserts.
  • wires dry_run through cmd_repair and skips confirm_destructive_action under --dry-run (a read-only preview shouldn't require --yes or block on a prompt).
  • preserves the return contract: a dry run returns a populated {collection: count} dict (CLI treats as success); validation refusals still return {} exactly as before.

No new dependencies. No behavior change for runs without --dry-run.

How to test

mempalace repair --mode from-sqlite --archive-existing --dry-run
# → "DRY RUN — no changes will be made", per-collection would-upsert counts, palace untouched
python -m pytest tests/test_repair.py tests/test_cli.py -k "dry_run or from_sqlite" -v

New tests:

  • test_rebuild_from_sqlite_dry_run_cross_palace_writes_nothing — dry run creates nothing at dest and reports the counts the existing round-trip test proves a real rebuild produces.
  • test_rebuild_from_sqlite_dry_run_in_place_does_not_archive — in-place dry run does not move the live palace aside.
  • test_cmd_repair_from_sqlite_dry_run_passes_through_and_skips_confirm — CLI forwards dry_run=True and never calls confirm_destructive_action.

Checklist

  • Tests pass (python -m pytest tests/ -v) — 2270 passed, 3 skipped
  • No hardcoded paths
  • Linter passes (ruff check .)

`cmd_repair`'s from-sqlite branch never forwarded `args.dry_run` to
`rebuild_from_sqlite` (the max-seq-id branch does), and the function had
no `dry_run` parameter. As a result `repair --mode from-sqlite --dry-run`
ignored the flag entirely: it either ran the real destructive rebuild or
aborted at the confirmation prompt, instead of previewing.

Add a `dry_run` parameter to `rebuild_from_sqlite`. After source
validation it prints the per-collection row counts — read from
`sqlite_drawer_count`, the same SQLite ground-truth helper repair uses
elsewhere, so the preview matches what a real rebuild upserts — and
returns those would-be counts without archiving the existing palace,
creating collections, or re-embedding. Wire it through `cmd_repair` and
skip the destructive-action confirmation under `--dry-run`, since a
preview is read-only.

Tests:
- cross-palace dry-run writes nothing and reports the counts a real
  rebuild produces (verified against the existing round-trip test);
- in-place dry-run does not archive the live palace;
- CLI forwards dry_run=True and does not call confirm_destructive_action.

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces a '--dry-run' option to the 'repair' command's 'from-sqlite' mode, allowing users to preview rebuild plans and expected row counts without executing destructive actions or archiving existing palaces. Corresponding unit tests have been added to verify this behavior. The review feedback highlights an issue where using 'or 0' for unreadable collections during a dry run could silently mask database corruption or schema mismatches as empty collections, and suggests explicitly checking for 'None' to abort and signal a preflight failure.

Comment thread mempalace/repair.py
Comment on lines +1194 to +1198
counts = {}
for cname in _recoverable_collections():
n = sqlite_drawer_count(source_palace, cname) or 0
counts[cname] = n
print(f" [{cname}] would re-embed and upsert {n} rows")

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.

medium

Using or 0 when sqlite_drawer_count returns None silently conflates an unreadable/corrupt database (or schema mismatch) with a genuinely empty collection. If the database is unreadable, the dry run will misleadingly report 0 rows and succeed, whereas a real run would fail.

We should explicitly check if n is None and return an empty dict {} (which indicates a validation/preflight failure to the CLI) to prevent silent failures during dry runs.

        counts = {}
        for cname in _recoverable_collections():
            n = sqlite_drawer_count(source_palace, cname)
            if n is None:
                print(f"  ERROR: [{cname}] count is unreadable (corrupt database or schema mismatch).")
                return {}
            counts[cname] = n
            print(f"  [{cname}] would re-embed and upsert {n} rows")

@fatkobra

fatkobra commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

This issue is still present on current develop and has now been independently reported again in #2095 and #2133.

The existing implementation here appears to cover the correct contract:

  • forward dry_run from cmd_repair
  • do not require destructive confirmation for a read-only preview
  • validate the source before reporting a plan
  • report per-collection rows without archiving, creating the destination, or re-embedding

The earlier review concern also looks valid: sqlite_drawer_count(...) or 0 would conflate an unreadable/corrupt collection with a genuinely empty collection.

That path should instead fail explicitly, for example:

count = sqlite_drawer_count(source_palace, collection_name)
if count is None:
    print(
        f"  ERROR: [{collection_name}] row count is unreadable; "
        "dry-run cannot produce a trustworthy rebuild plan."
    )
    return {}

@igorls

igorls commented Aug 2, 2026

Copy link
Copy Markdown
Member

Superseded by #2138, which re-applies this dry-run approach on current develop (after single-writer mine-lock wrapping) and fails closed when sqlite_drawer_count is unreadable rather than using or 0.

@igorls

igorls commented Aug 2, 2026

Copy link
Copy Markdown
Member

Superseded by #2138 (merged).

@igorls igorls closed this Aug 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants