fix(worktree): pruner reaps rebase-merged trees via PR state; worktree add survives disk contention
Two gaps behind the recurring 'hermes -w timed out after 30 seconds': 1. Rebase-merge leak: git cherry only catches patch-identical commits. Salvage flows routinely change the diff (conflict resolution, follow-up commits), so 12 of 22 'unpushed' trees on the incident box had MERGED PRs and were preserved forever. The pruner now falls back to 'gh pr list --head <branch> --state merged' — authoritative, memoized on (branch, head_sha) with True-only caching, fail-safe to preserve. 2. Creation timeout 30s -> 120s: the ~10k-file checkout measured 113s at near-zero CPU under multi-agent disk contention vs 1.2s idle. 30s killed legitimate creates and threw away completed work.
This commit is contained in:
Teknium
@@ -1866,9 +1866,14 @@ def _setup_worktree(repo_root: str = None, sync_base: bool = True,
|
||||
"-c", "checkout.thresholdForParallelism=100",
|
||||
]
|
||||
try:
|
||||
# 120s, not 30: on a multi-agent box the ~10k-file materialization
|
||||
# contends with sibling sessions' checkouts/fetches/Electron dev
|
||||
# builds for the same disk — measured 113s wall at near-zero CPU
|
||||
# under load vs 1.2s idle (Aug 2026). A too-tight timeout kills a
|
||||
# legitimately slow create and wastes the work already done.
|
||||
result = subprocess.run(
|
||||
["git", *_wt_add_cfg, "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,
|
||||
capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=120, cwd=repo_root,
|
||||
)
|
||||
if result.returncode != 0:
|
||||
# If branching from the resolved remote ref failed for any reason
|
||||
@@ -1883,7 +1888,7 @@ def _setup_worktree(repo_root: str = None, sync_base: bool = True,
|
||||
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,
|
||||
capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=120, cwd=repo_root,
|
||||
)
|
||||
if result.returncode != 0:
|
||||
_cleanup_failed_worktree_add(repo_root, wt_path, branch_name)
|
||||
@@ -2292,6 +2297,69 @@ def _worktree_commits_all_merged_upstream(
|
||||
return False
|
||||
|
||||
|
||||
def _worktree_branch_pr_merged(
|
||||
worktree_path: str,
|
||||
timeout: int = 15,
|
||||
cache: Optional[Dict[str, bool]] = None,
|
||||
) -> bool:
|
||||
"""Return whether the worktree branch's PR is MERGED on GitHub.
|
||||
|
||||
Escape hatch for the case ``git cherry`` cannot catch: a rebase-merge that
|
||||
altered the diff (conflict resolution against a moved base, follow-up
|
||||
commits added during salvage/CI-fix) changes the patch-id, so the local
|
||||
commits are no longer patch-equivalent to anything upstream even though
|
||||
the PR merged. Those trees survive the cherry check forever (Aug 2026:
|
||||
12 of 22 "unpushed" trees on a loaded box had MERGED PRs).
|
||||
|
||||
GitHub's PR state is the authoritative merge signal, so a clean tree
|
||||
whose branch has a MERGED PR is reaped. Verdicts are memoized keyed on
|
||||
``(branch, head_sha)`` — MERGED is monotonic, so a True verdict is cached
|
||||
permanently; False is never cached (the PR may merge later without new
|
||||
local commits, which would leave the key unchanged).
|
||||
|
||||
Fails SAFE toward False (preserve): no gh binary, offline, rate-limited,
|
||||
detached HEAD, or any parse failure keeps the tree.
|
||||
"""
|
||||
import subprocess
|
||||
|
||||
try:
|
||||
head = subprocess.run(
|
||||
["git", "rev-parse", "--abbrev-ref", "HEAD"],
|
||||
capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=timeout, cwd=worktree_path,
|
||||
)
|
||||
if head.returncode != 0:
|
||||
return False
|
||||
branch = head.stdout.strip()
|
||||
if not branch or branch == "HEAD": # detached — no PR to look up
|
||||
return False
|
||||
|
||||
cache_key = None
|
||||
if cache is not None:
|
||||
sha = subprocess.run(
|
||||
["git", "rev-parse", "HEAD"],
|
||||
capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=timeout, cwd=worktree_path,
|
||||
)
|
||||
if sha.returncode == 0 and sha.stdout.strip():
|
||||
cache_key = f"pr-merged:{branch}:{sha.stdout.strip()}"
|
||||
if cache.get(cache_key) is True:
|
||||
return True
|
||||
|
||||
result = subprocess.run(
|
||||
["gh", "pr", "list", "--head", branch, "--state", "merged",
|
||||
"--json", "number", "--limit", "1"],
|
||||
capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=timeout, cwd=worktree_path,
|
||||
)
|
||||
if result.returncode != 0:
|
||||
return False
|
||||
prs = json.loads(result.stdout or "[]")
|
||||
merged = isinstance(prs, list) and len(prs) > 0
|
||||
if merged and cache is not None and cache_key is not None:
|
||||
cache[cache_key] = True
|
||||
return merged
|
||||
except Exception:
|
||||
return False
|
||||
|
||||
|
||||
def _worktree_lock_is_live(repo_root: str, worktree_path: str, timeout: int = 10):
|
||||
"""Classify a worktree's git lock as live, dead, or absent.
|
||||
|
||||
@@ -2652,6 +2720,14 @@ def _prune_stale_worktrees(repo_root: str, max_age_hours: int = 24) -> None:
|
||||
merged = _worktree_commits_all_merged_upstream(
|
||||
str(entry), timeout=30, cache=snapshot
|
||||
)
|
||||
if not merged:
|
||||
# Rebase-merge escape hatch: conflict resolution or follow-up
|
||||
# commits change the patch-id, so cherry misses them — but
|
||||
# GitHub knows the PR merged. Authoritative and cheap (~0.3s,
|
||||
# memoized on (branch, head_sha) so it's paid once per tree).
|
||||
merged = _worktree_branch_pr_merged(
|
||||
str(entry), timeout=15, cache=snapshot
|
||||
)
|
||||
with cache_lock:
|
||||
merge_cache.update(snapshot)
|
||||
if not merged:
|
||||
|
||||
@@ -1340,3 +1340,105 @@ class TestShallowCloneDeepening:
|
||||
assert wt.exists(), (
|
||||
"genuinely unpushed commit must survive even after deepening"
|
||||
)
|
||||
|
||||
|
||||
class TestPrMergedEscapeHatch:
|
||||
"""Rebase-merged PRs whose diff changed during salvage defeat ``git
|
||||
cherry`` (patch-id mismatch), so the pruner asks GitHub whether the
|
||||
branch's PR is MERGED. These tests stub the ``gh`` binary on PATH — the
|
||||
contract is about how the pruner consumes the answer, not about GitHub.
|
||||
|
||||
Contract:
|
||||
- gh reports a merged PR + tree is clean -> reaped
|
||||
- gh reports no merged PR -> preserved
|
||||
- gh missing/failing -> preserved (fail safe)
|
||||
- dirty tree -> never reaped regardless of gh
|
||||
"""
|
||||
|
||||
_age = staticmethod(TestWorktreeLockReaping._age)
|
||||
|
||||
@staticmethod
|
||||
def _mk_diverged(repo, name, age_h=100):
|
||||
"""Worktree with a commit NOT patch-equivalent to anything upstream."""
|
||||
p = repo / ".worktrees" / name
|
||||
(repo / ".worktrees").mkdir(exist_ok=True)
|
||||
subprocess.run(
|
||||
["git", "worktree", "add", str(p), "-b", f"hermes/{name}", "HEAD"],
|
||||
cwd=repo, capture_output=True,
|
||||
)
|
||||
(p / "salvaged.txt").write_text("diff that was reworked during salvage\n")
|
||||
subprocess.run(["git", "add", "salvaged.txt"], cwd=p, capture_output=True)
|
||||
subprocess.run(["git", "commit", "-m", "salvaged work"], cwd=p, capture_output=True)
|
||||
TestPrMergedEscapeHatch._age(p, age_h)
|
||||
return p
|
||||
|
||||
@staticmethod
|
||||
def _stub_gh(tmp_path, monkeypatch, stdout='[{"number": 1}]', exit_code=0):
|
||||
gh = tmp_path / "bin" / "gh"
|
||||
gh.parent.mkdir(parents=True, exist_ok=True)
|
||||
gh.write_text(f"#!/bin/sh\nprintf '%s' '{stdout}'\nexit {exit_code}\n")
|
||||
gh.chmod(0o755)
|
||||
monkeypatch.setenv("PATH", f"{gh.parent}:{os.environ['PATH']}")
|
||||
|
||||
def test_merged_pr_tree_is_reaped(self, git_repo, tmp_path, monkeypatch):
|
||||
import cli
|
||||
wt = self._mk_diverged(git_repo, "hermes-rebase-merged")
|
||||
assert cli._worktree_commits_all_merged_upstream(str(wt)) is False, (
|
||||
"precondition: cherry must NOT consider this merged — the PR "
|
||||
"check is the only thing that can reap it"
|
||||
)
|
||||
self._stub_gh(tmp_path, monkeypatch)
|
||||
cli._prune_stale_worktrees(str(git_repo))
|
||||
assert not wt.exists(), (
|
||||
"clean tree whose branch has a MERGED PR is merged work — reap it"
|
||||
)
|
||||
|
||||
def test_no_merged_pr_preserved(self, git_repo, tmp_path, monkeypatch):
|
||||
import cli
|
||||
wt = self._mk_diverged(git_repo, "hermes-pr-open")
|
||||
self._stub_gh(tmp_path, monkeypatch, stdout="[]")
|
||||
cli._prune_stale_worktrees(str(git_repo))
|
||||
assert wt.exists(), "no merged PR -> still unpushed work, preserve"
|
||||
|
||||
def test_gh_failure_fails_safe(self, git_repo, tmp_path, monkeypatch):
|
||||
import cli
|
||||
wt = self._mk_diverged(git_repo, "hermes-gh-down")
|
||||
self._stub_gh(tmp_path, monkeypatch, stdout="", exit_code=1)
|
||||
cli._prune_stale_worktrees(str(git_repo))
|
||||
assert wt.exists(), "gh failure must preserve the tree (fail safe)"
|
||||
|
||||
def test_dirty_tree_never_reaped_even_with_merged_pr(
|
||||
self, git_repo, tmp_path, monkeypatch
|
||||
):
|
||||
import cli
|
||||
wt = self._mk_diverged(git_repo, "hermes-dirty-merged")
|
||||
(wt / "uncommitted.txt").write_text("in-flight\n")
|
||||
self._age(wt, 100)
|
||||
self._stub_gh(tmp_path, monkeypatch)
|
||||
cli._prune_stale_worktrees(str(git_repo))
|
||||
assert wt.exists(), "dirty guard outranks the PR-merged verdict"
|
||||
|
||||
def test_merged_verdict_memoized_by_branch_and_head(
|
||||
self, git_repo, tmp_path, monkeypatch
|
||||
):
|
||||
import cli
|
||||
wt = self._mk_diverged(git_repo, "hermes-memo")
|
||||
self._stub_gh(tmp_path, monkeypatch)
|
||||
cache: dict = {}
|
||||
assert cli._worktree_branch_pr_merged(str(wt), cache=cache) is True
|
||||
keys = [k for k in cache if k.startswith("pr-merged:")]
|
||||
assert len(keys) == 1 and cache[keys[0]] is True
|
||||
# Break gh: a cached True verdict must not re-consult it.
|
||||
self._stub_gh(tmp_path, monkeypatch, stdout="", exit_code=1)
|
||||
assert cli._worktree_branch_pr_merged(str(wt), cache=cache) is True
|
||||
|
||||
def test_negative_verdict_not_cached(self, git_repo, tmp_path, monkeypatch):
|
||||
import cli
|
||||
wt = self._mk_diverged(git_repo, "hermes-nocache-neg")
|
||||
self._stub_gh(tmp_path, monkeypatch, stdout="[]")
|
||||
cache: dict = {}
|
||||
assert cli._worktree_branch_pr_merged(str(wt), cache=cache) is False
|
||||
assert not [k for k in cache if k.startswith("pr-merged:")], (
|
||||
"False must not be memoized — the PR can merge later with the "
|
||||
"same (branch, head) key"
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user