Skip to content

fix(convo_miner): stop sweeper drawers being purged, abort on failed purge - #2089

Closed
KeilerHirsch wants to merge 2 commits into
MemPalace:developfrom
KeilerHirsch:fix/convo-miner-sweeper-purge-scoping
Closed

fix(convo_miner): stop sweeper drawers being purged, abort on failed purge#2089
KeilerHirsch wants to merge 2 commits into
MemPalace:developfrom
KeilerHirsch:fix/convo-miner-sweeper-purge-scoping

Conversation

@KeilerHirsch

Copy link
Copy Markdown
Contributor

Problem

Two related gaps found in convo_miner.py's purge-before-rebuild path:

1. _metadata_matches_extract_mode's legacy-compat rule sweeps up the sweeper's own drawers. The sweeper is a separate ingest path that writes drawers with no extract_mode field at all — its own metadata schema (ingest_mode="sweep", session_id, role, ...) never included that key, because it was never meant to. _metadata_matches_extract_mode (shared by file_already_mined and convo_miner's own _source_file_delete_ids) treats any drawer with no extract_mode as a legacy exchange-mode row, to stay compatible with genuine pre-schema convo_miner drawers. Since mempalace mine --mode convos defaults --extract to exchange, and both the sweeper and the convo miner are meant to run over the same transcript directory, the very next mine --mode convos re-mine of a session file the sweeper already swept scoops up all of that session's sweeper-written drawers into the purge and deletes them — not because of an exception, but because the selection criterion itself can't tell the two producers apart. Recovery only happens if the sweeper is re-run against the same still-existing file path before it's archived/rotated; otherwise the loss is silent and permanent.

2. Purge-failure swallow (own instance of the same pattern already fixed for miner.py's process_file). _file_chunks_locked wraps its delete-ids lookup and the delete call in one except Exception: logger.debug(...), then proceeds to insert the new chunks regardless of whether the purge succeeded. A transient backend error then leaves stale/duplicate drawers under mixed schema versions, with no operator-visible signal beyond a debug log.

Fix

  • _metadata_matches_extract_mode's legacy-compat fallback now only applies when the drawer is otherwise convo_miner's own — no ingest_mode at all (a genuine pre-ingest_mode-schema convo row), or convo_miner's own "convos" tag. A drawer positively identified as another producer's (ingest_mode="sweep", or any other foreign value) is excluded, using the same ingest_mode discriminator sync.py already relies on for its own registry-row check.
  • A failed purge in _file_chunks_locked now aborts (skipped=True, leaving the old drawers' stored mtime untouched so the next mine retries) and prints a visible warning, instead of falling through to upsert.

Tests

New tests in tests/test_palace.py (direct unit coverage of _metadata_matches_extract_mode's three-way logic, including the exact sweeper-row case) and tests/test_convo_miner_unit.py (an integration-style test of _source_file_delete_ids proving a sweeper row survives a default exchange-mode purge, and a _file_chunks_locked test proving a purge failure aborts before upsert). Each was confirmed to fail against the pre-fix code and pass after the fix. Full existing suite across test_palace.py, test_convo_miner*.py, test_sweeper.py, and test_miner.py still passes — no regressions.

…purge

MemPalace#104 (CRITICAL, data-loss): the sweeper writes drawers with no
extract_mode at all (ingest_mode="sweep"). _metadata_matches_extract_mode's
legacy-compat rule -- "no extract_mode means treat as a legacy exchange
row" -- couldn't tell that apart from a genuine pre-schema convo_miner
row, so mempalace mine --mode convos (default extract=exchange) swept
every sweeper drawer for a shared transcript into its purge scope and
deleted them on the very next re-mine. The legacy-compat fallback now
only applies when the drawer is otherwise convo_miner's own (no
ingest_mode at all, or convo_miner's own "convos" tag) -- a drawer
positively identified as another producer's (sweep, or any other
foreign ingest_mode) never matches, using the same ingest_mode
discriminator sync.py already relies on for its own registry-row check.

MemPalace#105 (MEDIUM, silent-failure): convo_miner's own instance of the
purge-failure swallow already fixed for miner.py at MemPalace#23 -- a failed
purge in _file_chunks_locked was logged at debug level and mining
proceeded anyway, silently producing duplicate/stale drawers under
mixed schema versions. Now aborts (returns skipped=True, leaving the
old drawers' stored mtime untouched so the next mine retries) and
prints a visible warning.

428 tests pass across test_palace.py/test_convo_miner*.py/test_miner.py/
test_sweeper.py/test_hallways.py/test_format_miner.py/test_repair.py, no
regressions.
gnusam pushed a commit to gnusam/mempalace-pgsql that referenced this pull request Jul 29, 2026
Adapts the portable half of upstream PR MemPalace#2088 (four re-mine safety gaps
in process_file) to the PostgreSQL backend. Upstream needs a chunk_total
completion marker and abort-on-failed-purge because ChromaDB commits
batch by batch; here one PalaceDB.replace_file_drawers transaction makes
the whole file atomic instead: stale-row purge and every insert commit
together, so a mine killed mid-file leaves the previous state intact, a
failed purge aborts the attempt, and a shrunk or empty-yield file stops
serving its old chunks.

source_mtime is captured with the read, not a later re-stat (the PR's
TOCTOU gap: an append landing between read and re-stat was stamped as
mined and silently skipped forever). file_already_mined now requires
every drawer in the file's mining scope to carry the current mtime
instead of trusting one arbitrary row, so partial legacy states re-mine
and self-clean. Purge and freshness check are scoped by
(ingest_mode, extract_mode) so convo extract modes cannot delete or
invalidate each other's drawers — the over-match class of upstream
PR MemPalace#2089.

Co-authored-by: KeilerHirsch <KeilerHirsch@users.noreply.github.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
gnusam pushed a commit to gnusam/mempalace-pgsql that referenced this pull request Jul 29, 2026
…ration

Extends the atomic-replace port (upstream PR MemPalace#2088) to the convo path
and closes the fork-side analog of upstream PR MemPalace#2089 (purge deleting
rows another pass owns):

- mine_convos files each transcript through one replace_file_drawers
  transaction scoped to (convos, extract_mode), so re-chunked exchanges
  can no longer leave orphaned tail rows and a mine killed mid-file
  leaves the previous state intact. The freshness check is scoped the
  same way, so exchange-mode and general-mode drawers for one
  transcript track their own staleness.
- source_mtime is captured before normalize() reads the file — Claude
  Code session logs are appended to while being mined, and a later
  re-stat stamps the drawers as covering a tail that was never chunked
  (PR MemPalace#2088's TOCTOU gap).
- register_empty_file now takes that caller-captured mtime and a
  purge_stale flag: genuinely-empty content purges the scope's stale
  drawers in the same transaction as the sentinel upsert, while the
  transient normalize()-failure path keeps registering the sentinel
  without touching mined data.

Co-authored-by: KeilerHirsch <KeilerHirsch@users.noreply.github.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
igorls added a commit that referenced this pull request Aug 2, 2026
fix(convo_miner): stop sweeper drawers being purged (#2089)
@igorls igorls closed this in #2125 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.

1 participant