fix(convo_miner): stop sweeper drawers being purged (#2089) - #2125
Merged
Conversation
…purge #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. #105 (MEDIUM, silent-failure): convo_miner's own instance of the purge-failure swallow already fixed for miner.py at #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.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR merges and finalizes the fix from #2089 onto develop, tightening convo_miner’s purge/idempotency behavior so it no longer deletes sweeper-created drawers and so it fails safely (skips) when a purge operation errors, avoiding mixed stale/new drawer states.
Changes:
- Refines
_metadata_matches_extract_modeto treat missingextract_modeas “legacy exchange” only for convo-miner-owned drawers (ingest_modeNone/"convos"), excluding other producers like the sweeper ("sweep"). - Updates
convo_miner._file_chunks_lockedto abort the mine attempt when stale-drawer purge fails, emitting a visible warning and returningskipped=True. - Adds targeted unit tests covering the new extract-mode scoping logic and the purge-failure abort behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
mempalace/palace.py |
Narrows legacy exchange-mode matching so missing extract_mode no longer sweeps up non-convo producers (e.g., sweeper). |
mempalace/convo_miner.py |
Aborts/returns skipped when stale-drawer purge fails instead of proceeding to upsert into a half-purged state. |
tests/test_palace.py |
Adds direct unit coverage for _metadata_matches_extract_mode including the sweeper metadata case. |
tests/test_convo_miner_unit.py |
Adds coverage for purge-failure abort behavior and for excluding sweeper rows from exchange-mode delete-id selection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Conflict-resolved merge of #2089 onto current
develop.tests/test_palace.pyimport conflict with single-writer tests from fix(storage): enforce local single-writer ownership #2079 (keeps both test suites)Closes #2089.
Test plan
uv run pytest tests/test_palace.py tests/test_convo_miner_unit.py