diff --git a/cli.py b/cli.py index 1094496edd2..8a64966fc81 100644 --- a/cli.py +++ b/cli.py @@ -1454,7 +1454,11 @@ def _path_is_within_root(path: Path, root: Path) -> bool: return False -def _resolve_worktree_base(repo_root: str) -> tuple: +def _resolve_worktree_base( + repo_root: str, + fetch_timeout: float = 5, + freshness_window: float = 300, +) -> tuple: """Resolve the freshest base ref to branch a new worktree from. The standalone clone's ``HEAD`` can lag the remote by hundreds of commits @@ -1466,14 +1470,27 @@ def _resolve_worktree_base(repo_root: str) -> tuple: freshly-fetched remote tip instead means the worktree starts current. Strategy (each step falls back to the next on failure): - 1. If the current branch tracks an upstream, fetch and use that upstream - ref — so a deliberate feature-branch worktree tracks its own remote, - not the default branch. - 2. Else fetch the remote's default branch (``origin/HEAD`` → e.g. + 1. If the current branch tracks an upstream, refresh and use that + upstream ref — so a deliberate feature-branch worktree tracks its own + remote, not the default branch. + 2. Else refresh the remote's default branch (``origin/HEAD`` → e.g. ``origin/main``) and use it. 3. Else fall back to ``HEAD`` (offline, no remote, or detached) — the old behavior, never worse than before. + "Refresh" is deliberately cheap on the startup path (the fetch here used + to stall ``hermes -w`` launches for 30-60s on flaky smart-HTTP + connections): + + - The fetch is SKIPPED entirely when the repo's ``FETCH_HEAD`` is younger + than *freshness_window* seconds — a base fetched moments ago cannot have + meaningfully moved, so repeated launches don't re-pay a network round + trip. + - The fetch is capped at *fetch_timeout* seconds. On timeout or failure we + fall back to the locally-known remote-tracking ref (labelled "cached") + instead of cascading into a second fetch attempt. Genuine staleness is + backstopped by the pre-push stale-base gate. + Returns ``(base_ref, label)`` where *base_ref* is a git revision suitable for ``git worktree add ... `` and *label* is a short human-readable description for the session banner. @@ -1482,7 +1499,7 @@ def _resolve_worktree_base(repo_root: str) -> tuple: from hermes_cli._subprocess_compat import noninteractive_git_env - def _git(args, timeout=20): + def _git(args, timeout: float = 20): return subprocess.run( ["git", *args], capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=timeout, cwd=repo_root, @@ -1490,16 +1507,59 @@ def _resolve_worktree_base(repo_root: str) -> tuple: env=noninteractive_git_env(), ) + def _ref_exists(ref: str) -> bool: + try: + return _git(["rev-parse", "--verify", "--quiet", ref + "^{commit}"]).returncode == 0 + except Exception: + return False + + def _fetch_head_age() -> Optional[float]: + """Seconds since the last fetch in this repo, or None if unknown.""" + try: + gd = _git(["rev-parse", "--git-dir"]) + if gd.returncode != 0: + return None + git_dir = Path(gd.stdout.strip()) + if not git_dir.is_absolute(): + git_dir = Path(repo_root) / git_dir + fetch_head = git_dir / "FETCH_HEAD" + if not fetch_head.exists(): + return None + return max(0.0, time.time() - fetch_head.stat().st_mtime) + except Exception: + return None + + def _refresh(remote: str, branch: str, ref: str) -> tuple: + """Return (ref, label) after a cheap best-effort refresh of *ref*. + + Never raises, never fetches twice, never blocks longer than + *fetch_timeout*. + """ + age = _fetch_head_age() + if age is not None and age < freshness_window and _ref_exists(ref): + return ref, f"{ref} (fetched {int(age)}s ago)" + try: + fetched = _git(["fetch", remote, branch], timeout=fetch_timeout) + if fetched.returncode == 0: + return ref, f"{ref} (fetched)" + reason = "fetch failed" + except subprocess.TimeoutExpired: + reason = f"fetch timed out after {fetch_timeout:g}s" + except Exception as e: + reason = f"fetch error: {e}" + if _ref_exists(ref): + logger.debug("worktree base: %s — using cached %s", reason, ref) + return ref, f"{ref} (cached — {reason})" + return "HEAD", f"HEAD (local — {reason}, no cached {ref})" + # 1. Current branch's upstream, if it tracks one. try: up = _git(["rev-parse", "--abbrev-ref", "--symbolic-full-name", "@{upstream}"]) if up.returncode == 0: upstream = up.stdout.strip() # e.g. "origin/main" if upstream and "/" in upstream: - remote = upstream.split("/", 1)[0] - # Fetch just that branch; fail-soft if offline. - _git(["fetch", remote, upstream.split("/", 1)[1]], timeout=30) - return upstream, f"{upstream} (fetched)" + remote, branch = upstream.split("/", 1) + return _refresh(remote, branch, upstream) except Exception as e: logger.debug("worktree base: upstream resolution failed: %s", e) @@ -1511,8 +1571,9 @@ def _resolve_worktree_base(repo_root: str) -> tuple: if head_ref.returncode == 0: default_ref = head_ref.stdout.strip().replace("refs/remotes/", "", 1) if not default_ref: - # origin/HEAD not set locally; ask the remote. - show = _git(["remote", "show", "origin"], timeout=30) + # origin/HEAD not set locally; ask the remote (network — capped + # like the fetch so a stalled connection can't hang startup). + show = _git(["remote", "show", "origin"], timeout=max(fetch_timeout, 5)) for line in show.stdout.splitlines(): line = line.strip() if line.startswith("HEAD branch:"): @@ -1524,8 +1585,7 @@ def _resolve_worktree_base(repo_root: str) -> tuple: break if default_ref and "/" in default_ref: remote, branch = default_ref.split("/", 1) - _git(["fetch", remote, branch], timeout=30) - return default_ref, f"{default_ref} (fetched)" + return _refresh(remote, branch, default_ref) except Exception as e: logger.debug("worktree base: default-branch resolution failed: %s", e) diff --git a/tests/cli/test_worktree_sync_base.py b/tests/cli/test_worktree_sync_base.py index e7f2a53a57f..224eeeb1502 100644 --- a/tests/cli/test_worktree_sync_base.py +++ b/tests/cli/test_worktree_sync_base.py @@ -10,7 +10,9 @@ works offline in the hermetic sandbox), proving the worktree includes commits that exist on the remote but not on the stale local HEAD. """ +import os import subprocess +import time from pathlib import Path import pytest @@ -95,6 +97,99 @@ class TestResolveWorktreeBase: assert "HEAD" in label +class TestResolveWorktreeBaseStartupCost: + """The fetch on the -w startup path must be cheap and stall-proof. + + A flaky smart-HTTP connection used to stall ``hermes -w`` for 30-60s + (30s fetch timeout in step 1, then a SECOND 30s fetch in step 2). + """ + + def test_fresh_fetch_head_skips_the_fetch(self, remote_and_clone, monkeypatch): + """FETCH_HEAD younger than the freshness window -> no fetch at all.""" + clone, remote_head, _ = remote_and_clone + # Prime FETCH_HEAD (and the origin/main tracking ref) with a real fetch. + _run(["git", "fetch", "origin", "main"], clone) + + calls = [] + real_run = subprocess.run + + def spy(args, **kw): + if isinstance(args, (list, tuple)) and "fetch" in args: + calls.append(list(args)) + return real_run(args, **kw) + + monkeypatch.setattr(subprocess, "run", spy) + base_ref, label = cli._resolve_worktree_base(str(clone)) + assert base_ref == "origin/main" + assert "fetched" in label and "ago" in label + assert calls == [], "fresh FETCH_HEAD must skip the network fetch" + # And the skipped-fetch ref still points at the remote tip. + resolved = _run(["git", "rev-parse", base_ref], clone).stdout.strip() + assert resolved == remote_head + + def test_stale_fetch_head_refetches(self, remote_and_clone): + """FETCH_HEAD older than the window -> a real fetch happens.""" + clone, remote_head, _ = remote_and_clone + _run(["git", "fetch", "origin", "main"], clone) + fetch_head = Path(clone) / ".git" / "FETCH_HEAD" + old = time.time() - 3600 + os.utime(fetch_head, (old, old)) + base_ref, label = cli._resolve_worktree_base(str(clone)) + assert base_ref == "origin/main" + assert label == "origin/main (fetched)" + + def test_fetch_timeout_falls_back_to_cached_ref(self, remote_and_clone, monkeypatch): + """A stalled fetch must yield the locally-cached tracking ref, fast — + not cascade into a second fetch or blow up.""" + clone, remote_head, stale_local_head = remote_and_clone + + real_run = subprocess.run + fetches = [] + + def stall_fetches(args, **kw): + if isinstance(args, (list, tuple)) and "fetch" in args: + fetches.append(list(args)) + raise subprocess.TimeoutExpired(cmd=args, timeout=kw.get("timeout", 5)) + return real_run(args, **kw) + + monkeypatch.setattr(subprocess, "run", stall_fetches) + start = time.monotonic() + base_ref, label = cli._resolve_worktree_base(str(clone)) + elapsed = time.monotonic() - start + # Cached tracking ref, single fetch attempt, no step-2 cascade. + assert base_ref == "origin/main" + assert "cached" in label and "timed out" in label + assert len(fetches) == 1, "timeout must not cascade into a second fetch" + assert elapsed < 5, f"fallback path took {elapsed:.1f}s — must be fast" + # The cached ref is the clone-time origin/main (pre-advance), which is + # still a valid base — staleness is backstopped by the pre-push gate. + resolved = _run(["git", "rev-parse", base_ref], clone).stdout.strip() + assert resolved == stale_local_head + + def test_fetch_timeout_without_cached_ref_falls_back_to_head( + self, tmp_path, monkeypatch + ): + """No usable tracking ref + failed fetch -> HEAD, never a bogus ref.""" + # A repo whose branch claims an upstream that has no tracking ref. + repo = tmp_path / "broken-upstream" + repo.mkdir() + _run(["git", "init"], repo) + _run(["git", "config", "user.email", "t@t.com"], repo) + _run(["git", "config", "user.name", "T"], repo) + _run(["git", "checkout", "-b", "main"], repo) + _commit(repo, "README.md", "base") + _run(["git", "remote", "add", "origin", str(tmp_path / "nonexistent.git")], repo) + _run( + ["git", "config", "branch.main.remote", "origin"], repo + ) + _run( + ["git", "config", "branch.main.merge", "refs/heads/main"], repo + ) + base_ref, label = cli._resolve_worktree_base(str(repo)) + assert base_ref == "HEAD" + assert "HEAD" in label + + class TestSetupWorktreeSyncBase: def test_sync_true_branches_from_remote_tip(self, remote_and_clone, monkeypatch): clone, remote_head, stale_local_head = remote_and_clone