mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-31 19:16:29 +00:00
perf(cli): stop hermes -w stalling 30-60s on a flaky fetch in _resolve_worktree_base
The #71637 prune fix cut one stage of -w startup, but the base-ref resolution right after it still ran an uncapped-in-practice 'git fetch origin main' (timeout=30) on every launch — and on a flaky smart-HTTP connection that fetch intermittently stalled to the full 30s, then cascaded into step 2's SECOND 30s fetch. Measured: back-to-back fetches of 0.9s, 1.0s, 63.5s on the same box with healthy TLS (~185ms). _resolve_worktree_base now: - skips the fetch entirely when FETCH_HEAD is < 5 min old and the tracking ref exists (repeat launches pay zero network cost) - caps the fetch at 5s and falls back to the locally-known tracking ref (labelled 'cached') on timeout/failure instead of cascading into a second fetch — genuine staleness stays backstopped by the pre-push stale-base gate - caps 'git remote show origin' the same way Worst case drops ~60s -> ~5s; warm path is ~0.02s (was up to 30.8s). sync_base=False and the offline HEAD fallback are unchanged.
This commit is contained in:
parent
7142dc4580
commit
1cf5d3841b
2 changed files with 169 additions and 14 deletions
88
cli.py
88
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 ... <base_ref>`` 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)
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue