diff --git a/cli.py b/cli.py index 40f875ee00..12b179f1d8 100644 --- a/cli.py +++ b/cli.py @@ -1475,6 +1475,86 @@ def _path_is_within_root(path: Path, root: Path) -> bool: return False +def _cleanup_failed_worktree_add(repo_root: str, wt_path: Path, branch_name: str) -> None: + """Make a failed/timed-out ``git worktree add`` atomic after the fact. + + ``git worktree add`` is not transactional: killed mid-checkout (the 30s + timeout) it leaves (a) the partially-materialized worktree directory, + (b) an admin entry under ``.git/worktrees/`` that is LOCKED with a + reason naming the *current, live* pid — so the startup pruner's + dead-pid unlock will never touch it — and (c) sometimes the new branch. + Any retry of the same name then fails on the leftovers. Sweep all three, + quietly; every step is fail-soft because this runs on an error path. + """ + import shutil + import subprocess + + def _git(*args: str) -> None: + try: + subprocess.run( + ["git", *args], + capture_output=True, text=True, timeout=15, cwd=repo_root, check=False, + ) + except Exception: + pass + + try: + # Unlock first: `worktree remove --force` refuses a locked tree. + _git("worktree", "unlock", str(wt_path)) + _git("worktree", "remove", "--force", str(wt_path)) + if wt_path.exists(): + shutil.rmtree(wt_path, ignore_errors=True) + # Drop the orphaned admin entry when the dir is already gone + # (`remove` needs the dir; `prune` handles the dirless case). + _git("worktree", "prune") + _git("branch", "-D", branch_name) + except Exception as e: + logger.debug("cleanup after failed worktree add: %s", e) + + +_PACK_SPRAWL_THRESHOLD = 15 + + +def _maintain_pack_health(repo_root: str) -> None: + """Repack the object store when pack files sprawl (background thread). + + On a multi-agent box every fetch/salvage session adds packs; git never + consolidates them on its own aggressively enough (``gc --auto``'s + threshold is 50 *and* it counts only non-kept packs). Past a few dozen + packs every object lookup scans every pack index, and worktree creation + can blow its 30s timeout under concurrent load (Aug 2026 incident: 39 + packs, 638MB → ``hermes -w`` timing out; a full repack halved the store + and restored 0.5s creates). Threshold 15 keeps lookups fast without + repacking on every startup; ``nice`` + background thread keeps it off + the startup path. Fail-soft everywhere. + """ + import subprocess + + try: + pack_dir = Path(repo_root) / ".git" / "objects" / "pack" + if not pack_dir.is_dir(): + return + packs = len(list(pack_dir.glob("*.pack"))) + if packs < _PACK_SPRAWL_THRESHOLD: + return + logger.info("git pack sprawl (%d packs) — repacking in background", packs) + cmd = ["git", "repack", "-a", "-d", "--quiet"] + if os.name == "posix": + cmd = ["nice", "-n", "19", *cmd] + subprocess.run( + cmd, + capture_output=True, text=True, timeout=1800, cwd=repo_root, check=False, + ) + # Repacking can strand now-duplicated admin files; a prune here keeps + # the worktree bookkeeping tight on the same maintenance pass. + subprocess.run( + ["git", "worktree", "prune"], + capture_output=True, text=True, timeout=60, cwd=repo_root, check=False, + ) + except Exception as e: + logger.debug("pack maintenance skipped: %s", e) + + def _resolve_worktree_base( repo_root: str, fetch_timeout: float = 5, @@ -1711,15 +1791,25 @@ def _setup_worktree(repo_root: str = None, sync_base: bool = True, "worktree add from %s failed (%s); retrying from local HEAD", base_ref, result.stderr.strip(), ) + _cleanup_failed_worktree_add(repo_root, wt_path, branch_name) base_ref, base_label = "HEAD", "HEAD (fallback — remote base failed)" result = subprocess.run( ["git", "worktree", "add", str(wt_path), "-b", branch_name, base_ref], capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=30, cwd=repo_root, ) if result.returncode != 0: + _cleanup_failed_worktree_add(repo_root, wt_path, branch_name) print(f"\033[31m✗ Failed to create worktree: {result.stderr.strip()}\033[0m") return None except Exception as e: + # A timed-out/failed `worktree add` is NOT atomic: git leaves the + # partially-materialized directory plus a LOCKED admin entry under + # .git/worktrees/ whose lock pid is THIS live process — so the + # startup pruner's dead-pid unlock never reaps it and every retry of + # the same name fails. Clean up our own wreckage before surfacing + # the error (Aug 2026 incident: 30s timeout during pack-sprawl left + # exactly this poison). + _cleanup_failed_worktree_add(repo_root, wt_path, branch_name) print(f"\033[31m✗ Failed to create worktree: {e}\033[0m") return None @@ -19730,8 +19820,16 @@ def main( # is immune to reaping (<24h age gate + live pid lock). _repo = _git_repo_root() if _repo: + def _worktree_maintenance(repo: str) -> None: + _prune_stale_worktrees(repo) + # Same pass: repack when packs sprawl, so object + # lookups (and the next `worktree add`) stay fast + # on multi-agent boxes. After the pruner so the + # repack sees final refs. + _maintain_pack_health(repo) + threading.Thread( - target=_prune_stale_worktrees, + target=_worktree_maintenance, args=(_repo,), name="worktree-prune", daemon=True, diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 8e43f0ffa0..575c87ff48 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -2707,6 +2707,7 @@ def _launch_tui( from cli import ( _cleanup_worktree, _git_repo_root, + _maintain_pack_health, _prune_stale_worktrees, _setup_worktree, ) @@ -2714,6 +2715,19 @@ def _launch_tui( repo = _git_repo_root() if repo: _prune_stale_worktrees(repo) + # Same maintenance pass as the CLI path: repack on pack + # sprawl so `worktree add` never crawls on a multi-agent box + # (cli._maintain_pack_health is a cheap no-op below the + # threshold). Runs on a thread — the TUI path calls the + # pruner synchronously, and a repack must not block launch. + import threading as _threading + + _threading.Thread( + target=_maintain_pack_health, + args=(repo,), + name="pack-maintenance", + daemon=True, + ).start() wt_info = _setup_worktree() except Exception as exc: print(f"✗ Failed to create TUI worktree: {exc}", file=sys.stderr) diff --git a/tests/cli/test_worktree_selfheal.py b/tests/cli/test_worktree_selfheal.py new file mode 100644 index 0000000000..9afed83464 --- /dev/null +++ b/tests/cli/test_worktree_selfheal.py @@ -0,0 +1,129 @@ +"""Tests for git self-heal: atomic worktree-add failure cleanup + pack maintenance. + +Regression for the Aug 2026 `hermes -w` timeout incident: 39 accumulated packs +slowed object lookups until `git worktree add` blew its 30s timeout, and the +timed-out add left a partially-materialized worktree plus a LOCKED admin entry +(lock pid = the live hermes process), poisoning every retry. + +Two behaviors: +1. `_cleanup_failed_worktree_add` — removes the partial dir, the admin entry + (even when LOCKED), and the orphaned branch, so a failed add is atomic. +2. `_maintain_pack_health` — repacks when *.pack count reaches the sprawl + threshold; no-op below it. +""" + +import subprocess +from pathlib import Path + +import pytest + + +def _git(cwd, *args, check=True): + return subprocess.run( + ["git", *args], cwd=str(cwd), capture_output=True, text=True, check=check + ) + + +@pytest.fixture +def repo(tmp_path): + root = tmp_path / "repo" + root.mkdir() + _git(root, "init", "-q", "-b", "main") + _git(root, "config", "user.email", "t@t") + _git(root, "config", "user.name", "t") + (root / "f.txt").write_text("x\n") + _git(root, "add", "-A") + _git(root, "commit", "-qm", "init") + return root + + +class TestCleanupFailedWorktreeAdd: + def _simulate_timed_out_add(self, repo): + """Reproduce git's post-timeout wreckage: partial dir + LOCKED admin + entry + branch. Built from a real add then re-locking + damaging it, + which yields the same on-disk shape as a killed `worktree add`.""" + wt = repo / ".worktrees" / "hermes-dead00" + _git(repo, "worktree", "add", str(wt), "-b", "hermes/hermes-dead00") + # Live-pid lock, exactly what `hermes -w` writes before the checkout. + _git(repo, "worktree", "lock", str(wt), "--reason", "hermes pid=999999") + # Partial materialization: gut the checkout but keep the dir + .git file. + for child in wt.iterdir(): + if child.name != ".git": + child.unlink() + return wt + + def test_sweeps_dir_admin_entry_and_branch(self, repo): + from cli import _cleanup_failed_worktree_add + + wt = self._simulate_timed_out_add(repo) + admin = repo / ".git" / "worktrees" / "hermes-dead00" + assert admin.exists() and (admin / "locked").exists() + + _cleanup_failed_worktree_add(str(repo), wt, "hermes/hermes-dead00") + + assert not wt.exists(), "partial worktree dir must be removed" + assert not admin.exists(), "LOCKED admin entry must be removed" + branches = _git(repo, "branch", "--list", "hermes/hermes-dead00").stdout + assert branches.strip() == "", "orphaned branch must be deleted" + + def test_retry_succeeds_after_cleanup(self, repo): + """The whole point: the same worktree name is creatable again.""" + from cli import _cleanup_failed_worktree_add + + wt = self._simulate_timed_out_add(repo) + _cleanup_failed_worktree_add(str(repo), wt, "hermes/hermes-dead00") + + result = _git( + repo, "worktree", "add", str(wt), "-b", "hermes/hermes-dead00", check=False + ) + assert result.returncode == 0, f"retry failed: {result.stderr}" + + def test_noop_when_nothing_exists(self, repo): + """Fail-soft on an error path where git never created anything.""" + from cli import _cleanup_failed_worktree_add + + _cleanup_failed_worktree_add( + str(repo), repo / ".worktrees" / "never-existed", "hermes/never-existed" + ) # must not raise + + +class TestMaintainPackHealth: + def _pack_count(self, repo): + return len(list((repo / ".git" / "objects" / "pack").glob("*.pack"))) + + def _make_packs(self, repo, n): + """Create n distinct packs by committing + repacking incrementally.""" + for i in range(n): + (repo / f"p{i}.txt").write_text(f"{i}\n") + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", f"c{i}") + # `repack` without -a packs only loose objects → one new pack. + _git(repo, "repack", "-q") + return self._pack_count(repo) + + def test_repacks_at_threshold(self, repo, monkeypatch): + import cli + + made = self._make_packs(repo, 6) + assert made >= 6 + monkeypatch.setattr(cli, "_PACK_SPRAWL_THRESHOLD", 5) + + cli._maintain_pack_health(str(repo)) + + after = self._pack_count(repo) + assert after <= 2, f"expected consolidation, still {after} packs" + + def test_noop_below_threshold(self, repo, monkeypatch): + import cli + + made = self._make_packs(repo, 3) + monkeypatch.setattr(cli, "_PACK_SPRAWL_THRESHOLD", 50) + + cli._maintain_pack_health(str(repo)) + + assert self._pack_count(repo) == made, "below threshold must be a no-op" + + def test_fail_soft_on_missing_pack_dir(self, tmp_path): + from cli import _maintain_pack_health + + _maintain_pack_health(str(tmp_path / "not-a-repo")) # must not raise