Merge pull request #1697 from margaretjgu/fix/backup-retention

fix(backups): add max_backups retention to bound backup disk usage

Author: igorls

CHANGELOG.md

@@ -6,6 +6,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 
 ---
 
+## [Unreleased]
+
+### Bug Fixes
+
+- **Backup retention to prevent unbounded disk usage.** `mempalace migrate` (full-palace `<palace>.pre-migrate.<timestamp>` copies) and `mempalace repair max-seq-id` (`chroma.sqlite3.max-seq-id-backup-<timestamp>` 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 — one palace was found with hundreds of GB of stale backups beside a few hundred MB of live data, hidden from a normal `du` of the home directory. A new `max_backups` setting (default `10`, env `MEMPALACE_MAX_BACKUPS`, or `config.json`) now prunes the oldest backups after each new one is written. Set it to `0` to keep every backup. Pruning is keyed by filesystem mtime, scoped strictly to each backup's own naming pattern (live data is never touched), and best-effort so a deletion failure can never abort a migration or repair that already succeeded.
+
+---
+
 ## [3.3.6] — 2026-05-24
 
 ### Features

mempalace/backups.py

@@ -0,0 +1,74 @@
+"""Retention pruning for timestamped palace backups.
+
+``mempalace migrate`` and ``mempalace repair max-seq-id`` each write a fresh,
+timestamped backup every time they run and historically never deleted the old
+ones. On a machine that mines or repairs on a schedule those full-size copies
+accumulate silently — a real palace was found with hundreds of gigabytes of
+backups sitting beside only a few hundred megabytes of live data, nearly
+filling the disk. This module prunes the backup set down to a bounded count
+after each new backup is written.
+
+The retention count comes from ``MempalaceConfig.max_backups`` (default 10).
+"""
+
+import glob
+import os
+import shutil
+
+
+def prune_backups(pattern, max_backups, *, log=None):
+    """Delete the oldest backups matching ``pattern`` so at most ``max_backups`` remain.
+
+    Args:
+        pattern: A glob pattern matching the backup paths (files or
+            directories). The caller is responsible for ``glob.escape``-ing
+            any literal, non-wildcard portion that can contain glob
+            metacharacters — palace paths sometimes do (e.g. a ``[``).
+        max_backups: Number of most-recent backups to keep. ``None`` or any
+            value ``<= 0`` disables pruning and returns immediately, so a
+            backup set is never touched when the user has opted out.
+        log: Optional callable (e.g. ``print``) for human-readable progress.
+
+    Returns:
+        The list of paths that were successfully removed.
+
+    Recency is determined by filesystem mtime rather than by parsing the
+    timestamp out of the name, so it stays correct even when two backup
+    producers use different timestamp formats. Deletion failures are logged
+    and skipped: pruning is best-effort cleanup and must never abort the
+    migrate/repair operation that just completed successfully.
+    """
+    if max_backups is None or max_backups <= 0:
+        return []
+
+    scored = []
+    for path in glob.glob(pattern):
+        try:
+            scored.append((os.path.getmtime(path), path))
+        except OSError:
+            # Vanished between glob and stat (concurrent prune / cleanup);
+            # nothing for us to remove.
+            continue
+
+    if len(scored) <= max_backups:
+        return []
+
+    # Newest first; the path breaks mtime ties so ordering is deterministic.
+    scored.sort(key=lambda item: (item[0], item[1]), reverse=True)
+
+    removed = []
+    for _mtime, path in scored[max_backups:]:
+        try:
+            if os.path.isdir(path) and not os.path.islink(path):
+                shutil.rmtree(path)
+            else:
+                os.remove(path)
+        except OSError as exc:
+            if log:
+                log(f"  Backup prune: could not remove {path}: {exc}")
+            continue
+        removed.append(path)
+        if log:
+            log(f"  Backup prune: removed old backup {path}")
+
+    return removed

mempalace/config.py

@@ -198,6 +198,12 @@ def sanitize_content(value: str, max_length: int = 100_000) -> str:
 DEFAULT_COLLECTION_NAME = "mempalace_drawers"
 DEFAULT_BACKEND = "chroma"
 
+# How many timestamped palace backups to retain before the oldest are
+# pruned. Applies to the accumulating backups written by ``mempalace
+# migrate`` and ``mempalace repair max-seq-id`` — see
+# ``MempalaceConfig.max_backups``.
+DEFAULT_MAX_BACKUPS = 10
+
 
 @lru_cache(maxsize=1)
 def get_configured_collection_name() -> str:
@@ -691,6 +697,36 @@ def topic_tunnel_min_count(self):
             parsed = 1
         return max(1, parsed)
 
+    @property
+    def max_backups(self) -> int:
+        """Number of timestamped palace backups to retain before pruning.
+
+        Applies to the accumulating, timestamped backups created by
+        ``mempalace migrate`` (``<palace>.pre-migrate.<timestamp>``) and
+        ``mempalace repair max-seq-id``
+        (``chroma.sqlite3.max-seq-id-backup-<timestamp>``). Each of those
+        commands writes a fresh full-size copy every run and historically
+        never deleted the old ones, so on a machine that mines or repairs on
+        a schedule the backup set could silently grow until it filled the
+        disk. After each backup is written, copies beyond this count (oldest
+        first) are removed.
+
+        Reads ``MEMPALACE_MAX_BACKUPS`` env first, then ``max_backups`` in
+        ``config.json``, then the default of ``10``. A value of ``0`` disables
+        pruning and keeps every backup (use when an external retention policy
+        manages cleanup). Negative or non-numeric values fall back to the
+        default rather than crashing migrate/repair.
+        """
+        env_val = os.environ.get("MEMPALACE_MAX_BACKUPS")
+        if env_val is not None:
+            coerced = self._try_coerce_int(env_val, minimum=0)
+            if coerced is not None:
+                return coerced
+        coerced = self._try_coerce_int(
+            self._file_config.get("max_backups", DEFAULT_MAX_BACKUPS), minimum=0
+        )
+        return DEFAULT_MAX_BACKUPS if coerced is None else coerced
+
     @property
     def hook_silent_save(self):
         """Whether the stop hook saves directly (True) or blocks for MCP calls (False)."""

mempalace/migrate.py

@@ -19,6 +19,7 @@
 """
 
 import errno
+import glob
 import os
 import shutil
 import sqlite3
@@ -28,6 +29,9 @@
 from contextlib import closing
 from datetime import datetime
 
+from .backups import prune_backups
+from .config import MempalaceConfig
+
 
 def _restore_stale_palace(palace_path: str, stale_path: str) -> None:
     """Roll back a failed swap.
@@ -293,6 +297,16 @@ def migrate(palace_path: str, dry_run: bool = False, confirm: bool = False):
     print(f"\n  Backing up to {backup_path}...")
     shutil.copytree(palace_path, backup_path)
 
+    # Enforce backup retention so repeated migrations cannot fill the disk
+    # with full-palace copies. The backup we just created is the newest, so
+    # it survives; only older ``.pre-migrate.*`` siblings beyond the limit
+    # are removed. Best-effort — never let cleanup fail the migration.
+    prune_backups(
+        glob.escape(palace_path) + ".pre-migrate.*",
+        MempalaceConfig().max_backups,
+        log=print,
+    )
+
     # Build fresh palace in a temp directory (avoids chromadb reading old state).
     # Wrap the whole import-and-swap dance in try/finally so the temp dir is
     # cleaned up if any of the chromadb writes, the verify count, or the

mempalace/repair.py

@@ -1529,12 +1529,26 @@ def repair_max_seq_id(
         return result
 
     if backup:
+        import glob
+
+        from .backups import prune_backups
+        from .config import MempalaceConfig
+
         timestamp = datetime.now().strftime("%Y%m%d-%H%M%S")
         backup_path = os.path.join(palace_path, f"chroma.sqlite3.max-seq-id-backup-{timestamp}")
         shutil.copy2(db_path, backup_path)
         result["backup"] = backup_path
         print(f"  Backup:  {backup_path}")
 
+        # Retain only the most recent N backups (the copy just written is the
+        # newest and is kept). Without this, every max-seq-id repair leaves a
+        # full chroma.sqlite3 copy behind that is never cleaned up.
+        prune_backups(
+            os.path.join(glob.escape(palace_path), "chroma.sqlite3.max-seq-id-backup-*"),
+            MempalaceConfig().max_backups,
+            log=print,
+        )
+
     _close_chroma_handles(palace_path)
 
     with sqlite3.connect(db_path) as conn:

tests/test_backups.py

@@ -0,0 +1,157 @@
+"""Tests for backup retention pruning (mempalace.backups.prune_backups).
+
+These guard the fix for unbounded backup growth: ``mempalace migrate`` and
+``mempalace repair max-seq-id`` each drop a fresh full-size, timestamped copy
+every run, and used to never delete the old ones — a palace was found with
+hundreds of GB of stale backups beside a few hundred MB of live data.
+"""
+
+import os
+
+import pytest
+
+from mempalace.backups import prune_backups
+
+
+def _make_backup_dir(parent, name, mtime):
+    """Create a directory backup with a fixed mtime."""
+    path = parent / name
+    path.mkdir()
+    (path / "chroma.sqlite3").write_text("db")
+    os.utime(path, (mtime, mtime))
+    return path
+
+
+def _make_backup_file(parent, name, mtime):
+    """Create a file backup with a fixed mtime."""
+    path = parent / name
+    path.write_text("db")
+    os.utime(path, (mtime, mtime))
+    return path
+
+
+def test_prune_keeps_newest_and_removes_oldest(tmp_path):
+    # 5 backups, mtimes 100..500; keep 2 newest (400, 500).
+    paths = [_make_backup_file(tmp_path, f"b.{i}", mtime=i * 100) for i in range(1, 6)]
+
+    removed = prune_backups(str(tmp_path / "b.*"), max_backups=2)
+
+    surviving = {p.name for p in tmp_path.iterdir()}
+    assert surviving == {"b.4", "b.5"}
+    assert set(removed) == {str(paths[0]), str(paths[1]), str(paths[2])}
+
+
+def test_prune_removes_directory_backups(tmp_path):
+    """migrate writes directory backups (full copytree) — must rmtree them."""
+    _make_backup_dir(tmp_path, "palace.pre-migrate.1", mtime=100)
+    _make_backup_dir(tmp_path, "palace.pre-migrate.2", mtime=200)
+    keep = _make_backup_dir(tmp_path, "palace.pre-migrate.3", mtime=300)
+
+    removed = prune_backups(str(tmp_path / "palace.pre-migrate.*"), max_backups=1)
+
+    assert keep.is_dir()
+    assert len(removed) == 2
+    assert not (tmp_path / "palace.pre-migrate.1").exists()
+    assert not (tmp_path / "palace.pre-migrate.2").exists()
+
+
+def test_prune_noop_when_under_limit(tmp_path):
+    _make_backup_file(tmp_path, "b.1", mtime=100)
+    _make_backup_file(tmp_path, "b.2", mtime=200)
+
+    removed = prune_backups(str(tmp_path / "b.*"), max_backups=10)
+
+    assert removed == []
+    assert len(list(tmp_path.iterdir())) == 2
+
+
+def test_prune_noop_when_exactly_at_limit(tmp_path):
+    _make_backup_file(tmp_path, "b.1", mtime=100)
+    _make_backup_file(tmp_path, "b.2", mtime=200)
+
+    removed = prune_backups(str(tmp_path / "b.*"), max_backups=2)
+
+    assert removed == []
+
+
+@pytest.mark.parametrize("disabled", [0, -1, None])
+def test_prune_disabled_keeps_everything(tmp_path, disabled):
+    for i in range(1, 6):
+        _make_backup_file(tmp_path, f"b.{i}", mtime=i * 100)
+
+    removed = prune_backups(str(tmp_path / "b.*"), max_backups=disabled)
+
+    assert removed == []
+    assert len(list(tmp_path.iterdir())) == 5
+
+
+def test_prune_no_matches(tmp_path):
+    assert prune_backups(str(tmp_path / "nope.*"), max_backups=3) == []
+
+
+def test_prune_only_touches_matching_pattern(tmp_path):
+    """Live data and unrelated files must never be swept up by a backup glob."""
+    _make_backup_file(tmp_path, "chroma.sqlite3.max-seq-id-backup-1", mtime=100)
+    _make_backup_file(tmp_path, "chroma.sqlite3.max-seq-id-backup-2", mtime=200)
+    _make_backup_file(tmp_path, "chroma.sqlite3.max-seq-id-backup-3", mtime=300)
+    # The live database and an unrelated file — must survive.
+    live = _make_backup_file(tmp_path, "chroma.sqlite3", mtime=400)
+    other = _make_backup_file(tmp_path, "tunnels.json", mtime=400)
+
+    prune_backups(
+        str(tmp_path / "chroma.sqlite3.max-seq-id-backup-*"),
+        max_backups=1,
+    )
+
+    assert live.exists()
+    assert other.exists()
+    assert (tmp_path / "chroma.sqlite3.max-seq-id-backup-3").exists()
+    assert not (tmp_path / "chroma.sqlite3.max-seq-id-backup-1").exists()
+    assert not (tmp_path / "chroma.sqlite3.max-seq-id-backup-2").exists()
+
+
+def test_prune_respects_glob_escape_for_metacharacter_paths(tmp_path):
+    """Palace paths can contain glob metacharacters like ``[``.
+
+    Without ``glob.escape`` the pattern would silently match nothing (the
+    bracket is read as a character class), leaving backups unpruned. Callers
+    escape the literal prefix; this confirms the helper prunes correctly once
+    they do.
+    """
+    import glob
+
+    weird = tmp_path / "weird[name]"
+    weird.mkdir()
+    for i in range(1, 4):
+        _make_backup_file(weird, f"chroma.sqlite3.max-seq-id-backup-{i}", mtime=i * 100)
+
+    pattern = os.path.join(glob.escape(str(weird)), "chroma.sqlite3.max-seq-id-backup-*")
+    removed = prune_backups(pattern, max_backups=1)
+
+    assert len(removed) == 2
+    assert (weird / "chroma.sqlite3.max-seq-id-backup-3").exists()
+
+
+def test_prune_is_best_effort_on_delete_failure(tmp_path, monkeypatch):
+    """A failed deletion is logged and skipped, never raised — pruning must
+    not undo a migrate/repair that already succeeded."""
+    for i in range(1, 5):
+        _make_backup_file(tmp_path, f"b.{i}", mtime=i * 100)
+
+    real_remove = os.remove
+
+    def flaky_remove(path):
+        if path.endswith("b.1"):
+            raise OSError("permission denied")
+        return real_remove(path)
+
+    monkeypatch.setattr(os, "remove", flaky_remove)
+
+    logs = []
+    removed = prune_backups(str(tmp_path / "b.*"), max_backups=2, log=logs.append)
+
+    # b.1 and b.2 were over the limit; b.1 failed, b.2 succeeded.
+    assert str(tmp_path / "b.2") in removed
+    assert str(tmp_path / "b.1") not in removed
+    assert (tmp_path / "b.1").exists()
+    assert any("could not remove" in line for line in logs)