mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-25 17:18:11 +00:00
fix(checkpoints): an empty surviving mount point is not evidence of deletion
Addresses @egilewski's review: the parent-directory check still deleted
checkpoint history for the most common unmount layout.
Detaching storage removes the parent outright in some layouts
(`/Volumes/Ext/proj` on macOS, `/media/<user>/<label>/proj`), which the first
commit handles. But in the classic static layout — `/mnt/volume/proj`, an
fstab entry, a container bind-mount — unmounting removes the contents and
leaves the mount point behind as an empty directory. `parent.is_dir()` is then
true, the project is absent, and the startup sweep deletes its ref, index and
metadata: exactly the case this PR set out to protect.
Reproduced against the real predicate before this commit:
mount root vanished (macOS) -> False ok
empty surviving mount point -> True <-- history deleted
really deleted (siblings) -> True ok
An empty parent carries no information: it looks identical whether the volume
was detached or the project was deleted. So require the parent to actually say
something — it holds some other entry (we observed a populated directory that
does not contain the project), or it is itself a live mount point (the volume
is attached right now and demonstrably does not hold the project).
The cost is that a project deleted out of an otherwise-empty parent is no
longer reclaimed by the orphan rule. It is not leaked: the retention rule
reads `last_touch` rather than probing the filesystem and still collects it,
so reclamation is deferred, not lost. That is the right direction for a
predicate whose false positive destroys a user's restore points unattended.
`_dir_has_any_entry` stops at the first entry via `os.scandir` instead of
materializing a listing, since a project root can hold a large tree.
tests/tools/test_checkpoint_manager.py: `test_surviving_empty_mountpoint_
keeps_its_checkpoints` pins the reviewed case, and `test_empty_parent_project_
is_still_reclaimed_by_retention` pins the deferral above so the safety valve
cannot silently regress into a leak. Both fail on the previous commit. The
real-orphan control now seeds a sibling so it exercises a populated parent
rather than the ambiguous empty one. 80 passed in the checkpoint suite; the 2
remaining failures (`TestGitEnvIsolation`, `TestClearFunctions`) fail
identically on clean main.
This commit is contained in:
parent
7d4e272cdf
commit
1fc338a4ec
2 changed files with 105 additions and 8 deletions
|
|
@ -1211,14 +1211,78 @@ class TestOrphanPruneRequiresObservableDeletion:
|
|||
"merely unmounted"
|
||||
)
|
||||
|
||||
def test_surviving_empty_mountpoint_keeps_its_checkpoints(
|
||||
self, tmp_path, checkpoint_base, monkeypatch,
|
||||
):
|
||||
"""The mount point outlives the volume as an empty dir → keep history.
|
||||
|
||||
The classic static layout (``/mnt/volume/proj``, an fstab entry, a
|
||||
container bind-mount) does not remove the mount point on unmount: the
|
||||
project disappears but ``/mnt/volume`` stays behind as an empty
|
||||
directory. The parent being present is therefore not evidence on its
|
||||
own — an empty parent looks exactly the same whether the volume was
|
||||
detached or the project was deleted.
|
||||
"""
|
||||
mount_point = tmp_path / "mnt" / "volume"
|
||||
work_dir = mount_point / "proj"
|
||||
work_dir.mkdir(parents=True)
|
||||
(work_dir / "main.py").write_text("print('x')\n")
|
||||
self._project_with_history(work_dir, checkpoint_base, monkeypatch)
|
||||
|
||||
# Unmount: contents go, the mount point itself remains.
|
||||
shutil.rmtree(work_dir)
|
||||
assert mount_point.is_dir() and not any(mount_point.iterdir())
|
||||
|
||||
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 mount point "
|
||||
"merely survived the unmount as an empty directory"
|
||||
)
|
||||
|
||||
def test_empty_parent_project_is_still_reclaimed_by_retention(
|
||||
self, tmp_path, checkpoint_base, monkeypatch,
|
||||
):
|
||||
"""Skipping an ambiguous parent defers reclamation, it does not lose it.
|
||||
|
||||
A project deleted out of an otherwise-empty parent is indistinguishable
|
||||
from an unmounted volume, so orphan pruning leaves it alone — but the
|
||||
retention rule, which reads ``last_touch`` instead of probing the
|
||||
filesystem, still reclaims it.
|
||||
"""
|
||||
parent = tmp_path / "solo"
|
||||
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.is_dir() and not any(parent.iterdir())
|
||||
|
||||
store = _store_path(checkpoint_base)
|
||||
meta_path = _project_meta_path(store, _project_hash(str(work_dir)))
|
||||
meta = json.loads(meta_path.read_text())
|
||||
meta["last_touch"] = time.time() - 60 * 86400
|
||||
meta_path.write_text(json.dumps(meta))
|
||||
|
||||
result = prune_checkpoints(
|
||||
retention_days=30, delete_orphans=True, checkpoint_base=checkpoint_base,
|
||||
)
|
||||
|
||||
assert result["deleted_stale"] >= 1
|
||||
assert not self._history_survives(checkpoint_base, work_dir)
|
||||
|
||||
def test_genuinely_deleted_project_is_still_pruned(
|
||||
self, tmp_path, checkpoint_base, monkeypatch,
|
||||
):
|
||||
"""Control: parent present, project gone → a real orphan, still pruned."""
|
||||
"""Control: a populated parent without the project → a real orphan."""
|
||||
parent = tmp_path / "projects"
|
||||
work_dir = parent / "proj"
|
||||
work_dir.mkdir(parents=True)
|
||||
(work_dir / "main.py").write_text("print('x')\n")
|
||||
(parent / "other-project").mkdir()
|
||||
self._project_with_history(work_dir, checkpoint_base, monkeypatch)
|
||||
|
||||
shutil.rmtree(work_dir)
|
||||
|
|
|
|||
|
|
@ -1304,12 +1304,25 @@ def _workdir_is_observably_gone(workdir: str) -> bool:
|
|||
"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.
|
||||
Require corroboration, in two steps.
|
||||
|
||||
First, 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.
|
||||
|
||||
Second, the present parent must actually carry information. Detaching
|
||||
storage removes the parent outright in some layouts (``/Volumes/Ext/proj``
|
||||
on macOS, ``/media/<user>/<label>/proj``), but in the classic static
|
||||
layout — ``/mnt/volume/proj``, a container bind-mount, an fstab entry —
|
||||
unmounting leaves the mount point behind as an *empty* directory. An empty
|
||||
parent is therefore the signature of a detached volume just as much as of
|
||||
a deleted project, so it corroborates nothing. Prune only when the parent
|
||||
holds something else (we observed a populated directory that does not
|
||||
contain the project) or is itself a live mount point (the volume is
|
||||
demonstrably attached and the project is demonstrably not on it).
|
||||
|
||||
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
|
||||
|
|
@ -1322,12 +1335,32 @@ def _workdir_is_observably_gone(workdir: str) -> bool:
|
|||
# to corroborate against.
|
||||
if parent == path:
|
||||
return False
|
||||
return parent.is_dir()
|
||||
if not parent.is_dir():
|
||||
return False
|
||||
if _dir_has_any_entry(parent):
|
||||
return True
|
||||
# Empty parent: only evidence if that directory is a mount point, i.e.
|
||||
# the volume is attached right now and simply does not hold the
|
||||
# project. An empty plain directory is an unmounted mount point as
|
||||
# readily as an emptied project root.
|
||||
return os.path.ismount(parent)
|
||||
except OSError:
|
||||
# Probe failed (permission, I/O error) — not evidence of deletion.
|
||||
return False
|
||||
|
||||
|
||||
def _dir_has_any_entry(directory: Path) -> bool:
|
||||
"""True when ``directory`` contains at least one entry.
|
||||
|
||||
Stops after the first entry rather than materializing the listing; a
|
||||
project root can hold a large tree.
|
||||
"""
|
||||
with os.scandir(directory) as entries:
|
||||
for _ in entries:
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
def prune_checkpoints(
|
||||
retention_days: int = 7,
|
||||
delete_orphans: bool = True,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue