From fc9b4186a044b314d3d76ab47f4d123e3dd12a0b Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 19 Aug 2026 23:39:40 -0700 Subject: [PATCH] fix(worktree): pruner reaps rebase-merged trees via PR state; worktree add survives disk contention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --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. --- cli.py | 80 ++++++++++++++++++++++++++++- tests/cli/test_worktree.py | 102 +++++++++++++++++++++++++++++++++++++ 2 files changed, 180 insertions(+), 2 deletions(-) diff --git a/cli.py b/cli.py index e026a56138..ffa623875b 100644 --- a/cli.py +++ b/cli.py @@ -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: diff --git a/tests/cli/test_worktree.py b/tests/cli/test_worktree.py index aab76a6193..c2fe4119c0 100644 --- a/tests/cli/test_worktree.py +++ b/tests/cli/test_worktree.py @@ -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" + )