fix(cli): self-heal git health around worktree creation
Two gaps from the Aug 2026 'hermes -w timed out after 30s' incident: 1. Atomic failure cleanup: a timed-out/failed `git worktree add` left a partially-materialized directory plus a LOCKED admin entry under .git/worktrees/ (lock pid = the live hermes process that timed out), which the startup pruner's dead-pid unlock never reaps — retries of the same name fail forever. _cleanup_failed_worktree_add sweeps dir, admin entry, and orphaned branch on every failure path (timeout, nonzero exit, remote-base retry). 2. Pack maintenance: nothing consolidated the object store; on a multi-agent box packs sprawl (39 packs / 638MB at the incident) and every object lookup scans all pack indexes until worktree creation blows its timeout. _maintain_pack_health repacks (niced, background, fail-soft) when *.pack count reaches 15, wired into the existing startup maintenance thread on both the CLI (-w) and TUI paths. gc --auto doesn't cover this: its threshold is 50 packs. Both sabotage-verified; full repack on the incident box: 39 packs -> 2, 638MB -> 287MB, worktree add 30s-timeout -> 0.5s.
This commit is contained in:
@@ -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/<name>`` 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/<name> 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,
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
Reference in New Issue
Block a user