From 3ca97d5e591120ec067d8887b98a7d246efe8e8a Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 7 Oct 2026 07:29:35 -0700 Subject: [PATCH] fix(pm): orphan reclaim runs on every prune pass and keeps checkouts it cannot see Follow-up to the review of this PR: - The startup pruner returned early (no .worktrees/, or no tree past 24h) before the reclaim ran, so a tree removed by hand kept its state until some other tree aged out. _prune_stale_worktrees now runs the reclaim in a finally around the prune phases. - A missing recorded path is not proof of deletion: a container sharing the data root (-v ~/.hermes:/opt/data) cannot see the host's checkout, and gc there deleted the host's live dependency environment. A state is an orphan only when its checkout sat under the data root that owns installs/, or directly in a .worktrees/ dir that still exists. - PM runtime generation leases (pm-runtime/generations/*/.leases) now count as held, so a worker still running from a deleted checkout's runtime keeps it. --- hermes_cli/worktree_ops.py | 38 ++++++++----- pm/install_states.py | 31 ++++++---- tests/pm/test_install_states_gc.py | 59 ++++++++++++-------- website/docs/reference/package-management.md | 2 +- 4 files changed, 83 insertions(+), 47 deletions(-) diff --git a/hermes_cli/worktree_ops.py b/hermes_cli/worktree_ops.py index bba0a4218b..7299522b9f 100644 --- a/hermes_cli/worktree_ops.py +++ b/hermes_cli/worktree_ops.py @@ -971,6 +971,29 @@ def _prune_stale_worktrees(repo_root: str, max_age_hours: int = 24) -> None: Phases: ``_prune_candidates`` -> ``_classify_prune_candidates`` -> ``_reap_prune_verdicts`` -> ``_prune_orphaned_branches``. """ + try: + _prune_stale_worktree_trees(repo_root, max_age_hours) + finally: + _reclaim_orphan_install_states() + + +def _reclaim_orphan_install_states() -> None: + """Trees removed by any path (the prune pass, `git worktree remove` by hand, `rm -rf` of a + scratch clone) leave their ~200 MB dependency state under /installs/; reclaim it on + every pass, including the early returns where no tree is old enough to prune. Startup + maintenance must never block a launch; the traceback goes to the debug log.""" + try: + from pm.environments import installs_root + from pm.install_states import collect_orphan_install_states + + reclaimed = collect_orphan_install_states(installs_root()) + if reclaimed: + logger.info("Reclaimed %d dependency state dir(s) of deleted checkouts", len(reclaimed)) + except Exception: + logger.debug("Orphan install-state reclaim failed", exc_info=True) + + +def _prune_stale_worktree_trees(repo_root: str, max_age_hours: int) -> None: worktrees_dir = Path(repo_root) / ".worktrees" if not worktrees_dir.exists(): _prune_orphaned_branches(repo_root) @@ -996,19 +1019,6 @@ def _prune_stale_worktrees(repo_root: str, max_age_hours: int = 24) -> None: _prune_orphaned_branches(repo_root, protect=kept_branches) - # Trees removed by any path (this pass, `git worktree remove` by hand, a campaign's own - # cleanup) leave their ~200 MB dependency state under /installs/ forever; reclaim it. - # Startup maintenance must never block a launch; the traceback goes to the debug log. - try: - from pm.environments import installs_root - from pm.install_states import collect_orphan_install_states - - reclaimed = collect_orphan_install_states(installs_root()) - if reclaimed: - logger.info("Reclaimed %d dependency state dir(s) of deleted checkouts", len(reclaimed)) - except Exception: - logger.debug("Orphan install-state reclaim failed", exc_info=True) - # The conservative startup pass accumulates trees it can never reclaim; say so once per launch. try: from hermes_cli.worktree_gc import worktrees_summary @@ -1019,7 +1029,7 @@ def _prune_stale_worktrees(repo_root: str, max_age_hours: int = 24) -> None: logger.warning(".worktrees/ holds %d tree(s) (%s) — run `hermes worktree list` " "to audit and `hermes worktree prune` to reclaim safely.", count, size_txt) except Exception: - pass + logger.debug("worktree summary failed", exc_info=True) def _prune_orphaned_branches(repo_root: str, protect: Optional[set] = None) -> None: diff --git a/pm/install_states.py b/pm/install_states.py index 11a2c208ec..1ae35054c6 100644 --- a/pm/install_states.py +++ b/pm/install_states.py @@ -7,9 +7,13 @@ PM runtime, ~200 MB each), and nothing removed it when the tree was deleted: one host carried 402 such orphans (80 GB) after a fortnight of worktree campaigns. The state dir records the checkout it belongs to in ``inputs/.project-root`` (written by -:func:`pm.environments.record_activation_inputs`); a dir whose recorded checkout no longer -exists has no possible reader. Removal still fails closed on anything a live process could -hold: the install lock, or a generation lease. +:func:`pm.environments.record_activation_inputs`). A missing path alone does not prove the +checkout is gone: a data root shared with a container (``-v ~/.hermes:/opt/data``) or an +unmounted volume records paths this process cannot see. So a dir is an orphan only when its +checkout sat where this process can see that it was deleted -- under the data root that owns +``installs/`` (scratch clones, the runtime checkout) or directly in a ``.worktrees/`` dir that +still exists. Removal still fails closed on anything a live process could hold: the install +lock, or a dependency / PM runtime generation lease. """ from __future__ import annotations @@ -22,10 +26,17 @@ from pathlib import Path logger = logging.getLogger(__name__) +def _provably_deleted(root: Path, data_root: Path) -> bool: + if root.exists(): + return False + return root.is_relative_to(data_root) or (root.parent.name == ".worktrees" and root.parent.is_dir()) + + def orphan_install_states(installs: Path) -> list[Path]: - """State dirs whose recorded checkout is gone. Dirs without a record are left alone.""" + """State dirs whose recorded checkout is provably deleted. Dirs without a record are left alone.""" if not installs.is_dir(): return [] + data_root = installs.resolve().parent orphans: list[Path] = [] for state in sorted(installs.iterdir()): record = state / "inputs" / ".project-root" @@ -35,7 +46,7 @@ def orphan_install_states(installs: Path) -> list[Path]: root = record.read_text(encoding="utf-8-sig").strip() except OSError: continue - if root and not Path(root).exists(): + if root and _provably_deleted(Path(root), data_root): orphans.append(state) return orphans @@ -57,11 +68,11 @@ def _held(state: Path) -> bool: return True finally: os.close(fd) - environments = state / "environments" - if environments.is_dir(): - for generation in environments.iterdir(): - if generation.is_dir() and (generation / ".leases").is_dir() and leases_held(generation): - return True + for generations in (state / "environments", state / "pm-runtime" / "generations"): + if generations.is_dir(): + for generation in generations.iterdir(): + if generation.is_dir() and (generation / ".leases").is_dir() and leases_held(generation): + return True return False diff --git a/tests/pm/test_install_states_gc.py b/tests/pm/test_install_states_gc.py index 270b305c82..3ed517e200 100644 --- a/tests/pm/test_install_states_gc.py +++ b/tests/pm/test_install_states_gc.py @@ -1,10 +1,13 @@ -"""Dependency state of a deleted checkout is reclaimed; state anything live could still read is -kept (``pm/install_states.py``).""" +"""Dependency state of a deleted checkout is reclaimed; state anything live could still read, or +whose checkout this process merely cannot see, is kept (``pm/install_states.py``).""" from __future__ import annotations import os +import subprocess +import pm.environments +from hermes_cli.worktree_ops import _prune_stale_worktrees from pm.filesystem import lock_fd from pm.install_states import collect_orphan_install_states, orphan_install_states @@ -17,37 +20,49 @@ def _state(installs, key, project_root): return state -def test_only_states_of_deleted_checkouts_are_reclaimed(tmp_path): - installs = tmp_path / "installs" - live_root = tmp_path / "checkout-live" - live_root.mkdir() +def test_startup_prune_reclaims_only_provably_deleted_checkouts(tmp_path, monkeypatch): + installs = tmp_path / "home" / "installs" + repo = tmp_path / "repo" + (repo / ".worktrees").mkdir(parents=True) + subprocess.run(["git", "init", "-q", str(repo)], check=True) + live_root = tmp_path / "home" / "hermes-agent" + live_root.mkdir(parents=True) live = _state(installs, "aaaa", live_root) - gone = _state(installs, "bbbb", tmp_path / "checkout-deleted") + gone_scratch = _state(installs, "bbbb", tmp_path / "home" / "cache" / "scratch" / "clone") + gone_worktree = _state(installs, "cccc", repo / ".worktrees" / "removed-by-hand") # Pre-record state (no .project-root) has no evidence either way: never touched. - unknown = installs / "cccc" + unknown = installs / "dddd" (unknown / "environments").mkdir(parents=True) + # A data root shared across a mount boundary records checkouts this process cannot see: + # a host install from inside a container, a container install from the host. + host_install = _state(installs, "eeee", tmp_path / "other-host-home" / ".hermes" / "hermes-agent") + container_install = _state(installs, "ffff", tmp_path / "opt-hermes") - assert orphan_install_states(installs) == [gone] - assert collect_orphan_install_states(installs) == [gone] - assert live.is_dir() and unknown.is_dir() and not gone.exists() + assert orphan_install_states(installs) == [gone_scratch, gone_worktree] + monkeypatch.setattr(pm.environments, "installs_root", lambda: installs) + # No tree under .worktrees/ is old enough to prune: the reclaim must still run. + _prune_stale_worktrees(str(repo)) + assert not gone_scratch.exists() and not gone_worktree.exists() + assert all(p.is_dir() for p in (live, unknown, host_install, container_install)) def test_held_orphan_is_kept(tmp_path): installs = tmp_path / "installs" locked = _state(installs, "dddd", tmp_path / "gone-a") leased = _state(installs, "eeee", tmp_path / "gone-b") + runtime = _state(installs, "ffff", tmp_path / "gone-c") - fd = os.open(locked / ".install.lock", os.O_CREAT | os.O_RDWR, 0o600) - assert lock_fd(fd, wait=False) - lease_dir = leased / "environments" / "gen" / ".leases" - lease_dir.mkdir() - lease_fd = os.open(lease_dir / "reader", os.O_CREAT | os.O_RDWR, 0o600) - assert lock_fd(lease_fd, wait=False) + fds = [os.open(locked / ".install.lock", os.O_CREAT | os.O_RDWR, 0o600)] + for lease_dir in (leased / "environments" / "gen" / ".leases", + runtime / "pm-runtime" / "generations" / "gen" / ".leases"): + lease_dir.mkdir(parents=True) + fds.append(os.open(lease_dir / "reader", os.O_CREAT | os.O_RDWR, 0o600)) + assert all(lock_fd(fd, wait=False) for fd in fds) try: assert collect_orphan_install_states(installs) == [] - assert locked.is_dir() and leased.is_dir() + assert locked.is_dir() and leased.is_dir() and runtime.is_dir() finally: - os.close(fd) - os.close(lease_fd) - # Locks released (owner exited): both are reclaimed on the next pass. - assert sorted(p.name for p in collect_orphan_install_states(installs)) == ["dddd", "eeee"] + for fd in fds: + os.close(fd) + # Locks released (owner exited): all are reclaimed on the next pass. + assert sorted(p.name for p in collect_orphan_install_states(installs)) == ["dddd", "eeee", "ffff"] diff --git a/website/docs/reference/package-management.md b/website/docs/reference/package-management.md index 9bb369b11e..7f84060bb1 100644 --- a/website/docs/reference/package-management.md +++ b/website/docs/reference/package-management.md @@ -535,7 +535,7 @@ hermes pm install chromium | `pm doctor` | Check installed tool identities, files, and digests against the lock. | | `pm repair` | Rebuild the recorded Python dependency set in a new generation, validate it, then select it. Does not update pins, features, or plugin configuration. | | `pm status` | Print the latest sync/update receipt as JSON, or report that no receipt exists. | -| `pm gc` | Remove unreferenced tool-store entries, eligible download partials, unused lease-managed Python generations, and the `installs/INSTALL_KEY/` state of checkouts that no longer exist (a deleted worktree or clone; skipped while its install lock or a generation lease is still held). The worktree pruner that runs at startup reclaims those orphaned state dirs too. | +| `pm gc` | Remove unreferenced tool-store entries, eligible download partials, unused lease-managed Python generations, and the `installs/INSTALL_KEY/` state of deleted checkouts (a worktree removed from a `.worktrees/` dir that still exists, or a clone under the data root; checkouts this process cannot see, such as a host install viewed from a container sharing the data root, are kept, as is any state whose install lock or generation lease is still held). The worktree pruner that runs at startup reclaims those orphaned state dirs too. | `pm env` excludes inherited process variables, including credentials. Its output can still reveal local installation paths; review it before sharing.