From 7d4e272cdf50f173fa97ca4cc024b15a089914ce Mon Sep 17 00:00:00 2001 From: Frowtek Date: Wed, 22 Jul 2026 05:43:49 +0300 Subject: [PATCH] fix(checkpoints): don't prune a project whose volume is merely unmounted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Orphan pruning decides a project is gone from a single probe: if delete_orphans and (not workdir or not Path(workdir).exists()): reason = "orphan" then deletes its ref, index, and metadata — the project's entire checkpoint history. `Path.exists()` is False for a deleted directory, but it is equally False for one whose storage is not attached right now: an unplugged external drive, a share behind a downed VPN, a bind-mount absent from this container, an offline Windows mapped drive. The project is fine; only our view of it is. This is not an opt-in maintenance command. `maybe_auto_prune_checkpoints` runs unattended at startup from both `cli.py` and `gateway/run.py`, with `delete_orphans=True` by default. So starting Hermes once while the drive is unplugged silently destroys the restore points for every project on it — the one thing checkpoints exist to provide, and there is nothing to restore from afterwards. Reproduced against the real store: a project registered under an unmounted path and one on local disk, then a startup prune — prune: {'scanned': 2, 'deleted_orphan': 1} unreachable project index still on disk: False The legacy pre-v2 branch has the same flaw plus a second one: a `HERMES_WORKDIR` marker that exists but cannot be read leaves `workdir = None`, which the same condition treats as an orphan. Failing to read a file is not evidence that a project was deleted. Require corroboration before deleting: the workdir's parent must be present, so its absence is something we actually observed. A missing parent means the volume is not there and we know nothing, so the entry is left alone — and an unreadable marker never deletes at all. Genuinely abandoned projects are still reclaimed, both by the unchanged orphan path (parent present, project gone) and by the retention/stale rule, which runs off `last_touch` rather than a filesystem probe. tests/tools/test_checkpoint_manager.py: a project whose whole mount disappears keeps its history; controls prove a genuinely deleted project is still pruned and a live project is untouched. The data-loss test fails on main; both controls pass there. 81 passed across the checkpoint suites (2 failures in test_checkpoint_manager.py are pre-existing and fail identically on clean main). --- tests/tools/test_checkpoint_manager.py | 87 ++++++++++++++++++++++++++ tools/checkpoint_manager.py | 48 +++++++++++++- 2 files changed, 133 insertions(+), 2 deletions(-) diff --git a/tests/tools/test_checkpoint_manager.py b/tests/tools/test_checkpoint_manager.py index 903f28908c88..6a0e3f6cd4c3 100644 --- a/tests/tools/test_checkpoint_manager.py +++ b/tests/tools/test_checkpoint_manager.py @@ -4,6 +4,7 @@ import argparse import json import logging import os +import shutil import subprocess import time import pytest @@ -1156,3 +1157,89 @@ class TestClearFunctions: result = clear_all() assert result["deleted"] is False assert result["bytes_freed"] == 0 + + +# ========================================================================= +# Orphan pruning must not act on an unreachable volume +# ========================================================================= + +class TestOrphanPruneRequiresObservableDeletion: + """A missing workdir is ambiguous: deleted, or just not mounted right now. + + Orphan pruning deletes a project's whole checkpoint history and runs + unattended at startup (``maybe_auto_prune_checkpoints`` from the CLI and + the gateway), so it must only fire when the deletion is something we + actually observed — the parent directory present, the project gone. An + unplugged drive, a share behind a downed VPN, or a bind-mount absent from + this container must not cost the user their restore points. + """ + + def _project_with_history(self, work_dir, checkpoint_base, monkeypatch): + monkeypatch.setattr( + "tools.checkpoint_manager.CHECKPOINT_BASE", checkpoint_base + ) + m = CheckpointManager(enabled=True, max_snapshots=10) + m.ensure_checkpoint(str(work_dir), "initial") + return m + + def _history_survives(self, checkpoint_base, work_dir): + store = _store_path(checkpoint_base) + return _project_meta_path( + store, _project_hash(str(work_dir)) + ).exists() + + def test_unreachable_volume_keeps_its_checkpoints( + self, tmp_path, checkpoint_base, monkeypatch, + ): + """The whole mount is absent (parent missing too) → keep the history.""" + mount = tmp_path / "mnt" / "nas" + work_dir = mount / "work" / "proj" + work_dir.mkdir(parents=True) + (work_dir / "main.py").write_text("print('x')\n") + self._project_with_history(work_dir, checkpoint_base, monkeypatch) + + # The volume goes away — project AND its parents disappear together. + shutil.rmtree(tmp_path / "mnt") + assert not work_dir.exists() + + prune_checkpoints( + retention_days=0, delete_orphans=True, checkpoint_base=checkpoint_base, + ) + + assert self._history_survives(checkpoint_base, work_dir), ( + "checkpoint history was deleted for a project whose volume was " + "merely unmounted" + ) + + def test_genuinely_deleted_project_is_still_pruned( + self, tmp_path, checkpoint_base, monkeypatch, + ): + """Control: parent present, project gone → a real orphan, still pruned.""" + parent = tmp_path / "projects" + work_dir = parent / "proj" + work_dir.mkdir(parents=True) + (work_dir / "main.py").write_text("print('x')\n") + self._project_with_history(work_dir, checkpoint_base, monkeypatch) + + shutil.rmtree(work_dir) + assert parent.exists() and not work_dir.exists() + + result = prune_checkpoints( + retention_days=0, delete_orphans=True, checkpoint_base=checkpoint_base, + ) + + assert result["deleted_orphan"] >= 1 + assert not self._history_survives(checkpoint_base, work_dir) + + def test_live_project_is_never_pruned_as_orphan( + self, work_dir, checkpoint_base, monkeypatch, + ): + """Control: a present project keeps its history.""" + self._project_with_history(work_dir, checkpoint_base, monkeypatch) + + result = prune_checkpoints( + retention_days=0, delete_orphans=True, checkpoint_base=checkpoint_base, + ) + + assert result["deleted_orphan"] == 0 + assert self._history_survives(checkpoint_base, work_dir) diff --git a/tools/checkpoint_manager.py b/tools/checkpoint_manager.py index b4791fca6e63..6aec5e2e6d8d 100644 --- a/tools/checkpoint_manager.py +++ b/tools/checkpoint_manager.py @@ -564,16 +564,21 @@ def _pre_v2_shadow_repos(base: Path) -> List[Dict]: if not (child / "HEAD").exists(): continue workdir: Optional[str] = None + marker_unreadable = False wd_marker = child / "HERMES_WORKDIR" if wd_marker.exists(): try: workdir = wd_marker.read_text(encoding="utf-8").strip() except (OSError, UnicodeDecodeError): + # The marker is there, we just could not read it. That is + # not evidence the project is gone — never delete on it. workdir = None + marker_unreadable = True out.append({ "path": child, "workdir": workdir, "exists": bool(workdir) and Path(workdir).exists(), + "marker_unreadable": marker_unreadable, }) return out @@ -1288,6 +1293,41 @@ def _delete_ref(store: Path, ref: str) -> bool: return ok +def _workdir_is_observably_gone(workdir: str) -> bool: + """True only when we can positively observe that ``workdir`` was removed. + + ``Path.exists()`` returns False for a deleted directory AND for one whose + storage simply is not attached right now — an unplugged external drive, a + network share behind a downed VPN, a bind-mount absent from this + container, an offline Windows mapped drive. Orphan pruning deletes the + project's entire checkpoint history, so treating that ambiguity as + "deleted" throws away the user's restore points over a transient mount + state, unattended, at startup. + + Require corroboration: the parent directory must be present, so the + absence of the project inside it is something we actually observed. When + the parent is missing too, the volume is not there and we know nothing — + leave the entry alone. Genuinely abandoned projects are still reclaimed by + the retention/stale rule, which runs off ``last_touch`` rather than a + filesystem probe. + """ + if not workdir: + return False + path = Path(workdir) + try: + if path.exists(): + return False + parent = path.parent + # A path whose parent is itself (a filesystem root) gives us nothing + # to corroborate against. + if parent == path: + return False + return parent.is_dir() + except OSError: + # Probe failed (permission, I/O error) — not evidence of deletion. + return False + + def prune_checkpoints( retention_days: int = 7, delete_orphans: bool = True, @@ -1380,7 +1420,11 @@ def prune_checkpoints( reason: Optional[str] = None if ( delete_orphans - and not repo["exists"] + and not repo["marker_unreadable"] + and ( + repo["workdir"] is None + or _workdir_is_observably_gone(repo["workdir"]) + ) and (orphan_allowlist is None or str(child) in orphan_allowlist) ): reason = "orphan" @@ -1423,7 +1467,7 @@ def prune_checkpoints( reason = None if ( delete_orphans - and (not workdir or not Path(workdir).exists()) + and (not workdir or _workdir_is_observably_gone(workdir)) and (orphan_allowlist is None or dir_hash in orphan_allowlist) ): reason = "orphan"