hermes-admin/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions 28 Packages Projects Releases Wiki Activity

fix(update): launches racing an interrupted-pull restore take turns and never advise reset --hard

Browse Source 
Follow-up to #120339 from its independent re-review.

- Single-flight: the restore runs under an OS lock on <git dir>/hermes-update-pull.claim
  (flock on POSIX, msvcrt.locking on Windows; owner pid written for humans). The kernel drops
  the lock with its owner, so a dead launch's claim is broken without a check-then-unlink race.
  A launch that loses waits up to 10 s, then re-checks: if the marker is gone the tree changed
  under it and it returns True (relaunch) instead of importing a half-restored tree; if the
  holder is still busy it prints a neutral note and never touches the tree.
- index.lock is removed only by the claim holder and only when no live process has it open
  (/proc on Linux; Windows refuses the unlink of an open file; elsewhere the claim is the guard).
- The failure message no longer prints `git reset --hard <pre>`: that wipes the uncommitted
  edits the restore keeps. It names git's error and says the next launch retries.
- HEAD already moved is checked before merge/rebase state, so a git killed after committing its
  merge but before deleting MERGE_HEAD spends the marker instead of pinning it forever.
- MERGE_HEAD == the marker's target (the updater's own merge, killed mid-conflict): print
  `git -C <root> merge --abort`, then relaunch, once per launch (plus the stash name if any).
- `hermes-agent` (agent.legacy_cli:main) now repairs at the top of agent/legacy_cli.py, before
  argparse and run_agent; the entry-point spy test covers it.
- Directories git created for added files are removed bottom-up when empty; a dir pre has, or one
  holding an untracked file, is kept.
This commit is contained in:
teknium1
2026-09-23 17:58:31 -07:00
committed by Teknium
parent 51b5c314e9
commit c56c0a61c4
3 changed files with 278 additions and 74 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
agent/legacy_cli.py
View File

@@ -9,8 +9,21 @@ here too, so the installer's PATH launcher behaves the same way.
from __future__ import annotations
import argparse
from typing import Callable, List, Optional
# hermes_bootstrap first (UTF-8 stdio on Windows; no-op on POSIX), like every other entry point.
try:
import hermes_bootstrap # noqa: F401
except ModuleNotFoundError:
pass # partial `hermes update` — only skips the Windows UTF-8 stdio setup
# The `hermes-agent` console script lands here without hermes_cli.main: repair a `hermes update` killed
# while git wrote the new tree before importing anything else from the checkout.
from hermes_cli import _early_recovery
if _early_recovery.restore_interrupted_pull():
_early_recovery.relaunch_after_restore()
import argparse # noqa: E402
from typing import Callable, List, Optional # noqa: E402
def _build_parser() -> argparse.ArgumentParser:

257
hermes_cli/_early_recovery.py
View File

@@ -15,7 +15,7 @@ import shutil
import subprocess
import sys
import time
from pathlib import Path
from pathlib import Path, PurePosixPath
# Core packages a failed lazy ``uv pip install`` is known to leave with intact distribution
# metadata but wiped import files. ``module`` is probed via a real import; ``attr`` guards against
@@ -271,8 +271,9 @@ def _hash_worktree(git, paths: list[str]) -> dict[str, str]:
return blobs
def _paths_git_wrote(git, root: Path, pre: str, target: str) -> tuple[list[str], list[str]] | None:
"""Paths the killed git already touched on the way to ``target``: (restore from HEAD, delete as added).
def _paths_git_wrote(git, root: Path, pre: str, target: str) -> tuple[list[str], list[str], set[str]] | None:
"""Paths the killed git already touched on the way to ``target``: (restore from HEAD, delete as added,
directories git may have created for its added files).
Git rewrites a file as unlink, create, write, so a kill leaves it missing, empty or cut short:
all of those count as git's, like the full new blob. Content that matches neither side and is not
@@ -312,95 +313,217 @@ def _paths_git_wrote(git, root: Path, pre: str, target: str) -> tuple[list[str],
stdin=subprocess.DEVNULL).stdout.startswith(content) for blob in blobs)
if written:
(added if old_blob is None else restore).append(path)
return restore, added
new_dirs = {str(parent) for path, (_m, old_blob, _n) in entries.items() if old_blob is None
for parent in PurePosixPath(path).parents if parent.parts}
if new_dirs:
new_dirs -= set(git("ls-tree", "-r", "-d", "--name-only", "-z", pre).stdout.split("\0"))
return restore, added, new_dirs
# Launches that start together after a killed update (a restarting gateway or Desktop backend next to
# the user's CLI) restore one at a time: the claim holder owns the git index, the rest wait for it and
# relaunch from its tree. An OS lock, not a pid file: the kernel drops it with its owner, so a dead
# launch's claim is broken without a check-then-unlink race.
_RESTORE_CLAIM = "hermes-update-pull.claim"
_RESTORE_CLAIM_WAIT_SECONDS = 10.0
_merge_advice_shown = False
def _lock_fd(fd: int, lock: bool) -> bool:
try:
if sys.platform == "win32":
import msvcrt
os.lseek(fd, 0, os.SEEK_SET) # msvcrt locks from the file position
msvcrt.locking(fd, msvcrt.LK_NBLCK if lock else msvcrt.LK_UNLCK, 1)
else:
import fcntl
fcntl.flock(fd, (fcntl.LOCK_EX | fcntl.LOCK_NB) if lock else fcntl.LOCK_UN)
return True
except OSError:
return False
@contextlib.contextmanager
def _restore_claim(git_dir: Path):
"""Yields True while this launch holds the restore claim, False when another held it past the wait."""
try:
fd = os.open(git_dir / _RESTORE_CLAIM, os.O_RDWR | os.O_CREAT, 0o644)
except OSError: # read-only git dir: no restore can write there either, the attempt reports why
yield True
return
try:
deadline = time.monotonic() + _RESTORE_CLAIM_WAIT_SECONDS
while not _lock_fd(fd, True):
if time.monotonic() > deadline:
yield False
return
time.sleep(0.05)
try:
os.ftruncate(fd, 0)
os.write(fd, f"pid={os.getpid()}\n".encode())
except OSError:
pass
try:
yield True
finally:
_lock_fd(fd, False)
finally:
os.close(fd)
def _held_open(path: Path) -> bool:
"""True when a running process has ``path`` open: a live git inside a command still holds its lock.
Linux answers exactly through /proc. Windows refuses to unlink a file another process has open, so
the caller's unlink is its probe; elsewhere nothing portable exists and the claim is the guard.
"""
proc = Path("/proc")
if not (proc / "self" / "fd").is_dir():
return False
target = os.path.realpath(path)
for fd_dir in proc.glob("[0-9]*/fd"):
try:
if any(os.readlink(entry.path) == target for entry in os.scandir(fd_dir)):
return True
except OSError:
continue
return False
def _release_dead_index_lock(git_dir: Path) -> bool:
"""Drop the killed git's ``index.lock`` (it refuses every git command); False while a live git holds it."""
lock = git_dir / "index.lock"
deadline = time.monotonic() + 5
while lock.exists():
if not _held_open(lock):
try:
lock.unlink()
return True
except FileNotFoundError:
return True
except PermissionError: # Windows: open in a live process
pass
if time.monotonic() > deadline:
return False
time.sleep(0.1)
return True
def restore_interrupted_pull(project_root: Path | None = None) -> bool:
"""Put back the files a killed ``hermes update`` had half-moved to the new commit.
Returns True when files were restored: modules this process already imported may be the
Returns True when the tree changed under this process: modules it already imported may be the
half-written ones, so the caller must relaunch (``relaunch_after_restore``).
Fast path (no marker) is one or two ``stat`` calls. Acts only when the marker's owner is gone,
HEAD is still the pre-pull commit and no merge/rebase is in progress; then every path git wrote
(the target's content, or torn on the way there) returns to HEAD (the commit the venv was built
for), so the install is whole again and ``hermes update`` redoes the update from the start. Local
edits are never touched; the updater's autostash (if any) stays in ``git stash list``.
edits are never touched; the updater's autostash (if any) stays in ``git stash list``. Concurrent
launches take turns (``_restore_claim``); a launch that waited out another's restore relaunches.
Limits, by design: a torn ``hermes_cli/__init__.py`` or ``hermes_bootstrap.py`` fails before this
runs (``git -C <root> reset --hard <pre>`` from the marker repairs it). A file git also changes
that the user deleted, emptied or cut to a prefix of git's version looks exactly like git's own
half-written file and is restored too, as is a user edit to a conflicted path or, on git < 2.38, to a
path both sides of a custom-branch merge changed.
runs. A file git also changes that the user deleted, emptied or cut to a prefix of git's version
looks exactly like git's own half-written file and is restored too, as is a user edit to a
conflicted path or, on git < 2.38, to a path both sides of a custom-branch merge changed.
"""
try:
root = _project_root() if project_root is None else project_root
marker = interrupted_pull_marker(root)
if not marker.is_file() or _pytest_owns_live_checkout(root):
return False
git_dir = marker.parent
fields = dict(line.partition("=")[::2] for line in marker.read_text(encoding="utf-8").splitlines())
try:
owner = int(fields.get("pid", ""))
except ValueError:
owner = -1
# Our own pid is never the owner: this runs at startup, and containers hand a retry the
# killed updater's pid.
if (owner != os.getpid() and _pid_is_running(owner)
and time.time() - marker.stat().st_mtime < _INTERRUPTED_PULL_MAX_AGE_SECONDS):
return False
if any((git_dir / name).exists() for name in _GIT_OPERATION_IN_PROGRESS):
return False
def git(*args: str, stdin: str | None = None) -> subprocess.CompletedProcess:
return subprocess.run(["git", "--literal-pathspecs", "-C", str(root), *args], input=stdin,
capture_output=True, text=True, encoding="utf-8", errors="replace",
timeout=120, stdin=None if stdin is not None else subprocess.DEVNULL)
pre, target = fields.get("pre", "").strip(), fields.get("target", "").strip()
if not pre or not target or git("rev-parse", "HEAD").stdout.strip() != pre:
marker.unlink() # git finished (HEAD moved) or the marker is unusable
return False
written = _paths_git_wrote(git, root, pre, target)
if written is None: # after a gc or re-clone: nothing left to compare against
marker.unlink()
print(f"⚠ Ignoring a stale interrupted-update marker: commit {target[:10]} is gone.", file=sys.stderr)
return False
restore, added = written
if not restore and not added:
marker.unlink() # the killed git never reached the tree: nothing to put back
return False
print("⚠ A previous `hermes update` was killed while git was writing the new code — "
f"restoring the checkout to {pre[:10]}...", file=sys.stderr)
# The dead git's index lock would refuse every command below.
(git_dir / "index.lock").unlink(missing_ok=True)
ok = True
if restore:
ok = git("restore", "--source=HEAD", "--staged", "--worktree", "--pathspec-from-file=-",
"--pathspec-file-nul", stdin="\0".join(restore)).returncode == 0
if added and ok:
ok = git("rm", "-q", "--cached", "--ignore-unmatch", "--pathspec-from-file=-",
"--pathspec-file-nul", stdin="\0".join(added)).returncode == 0
for rel in added:
path = root / rel
path.unlink(missing_ok=True)
with contextlib.suppress(OSError):
os.removedirs(path.parent) # stops at the first non-empty dir
if ok:
marker.unlink()
print(" ✓ Checkout restored; `hermes update` updates it again.", file=sys.stderr)
if fields.get("stash", "").strip():
print(f" Your local changes are still in the update's stash ({fields['stash'].strip()}).",
file=sys.stderr)
return True
print(f" ✗ Could not restore it automatically. Recover with: git -C {root} reset --hard {pre}",
file=sys.stderr)
with _restore_claim(marker.parent) as claimed:
if not claimed:
print("⚠ Another Hermes launch is still repairing the checkout after an interrupted "
"`hermes update`; if this one fails, launch again in a moment.", file=sys.stderr)
return False
if not marker.is_file():
return True # another launch finished while this one started: rerun from its tree
return _restore_holding_claim(root, marker)
except (OSError, subprocess.SubprocessError, ValueError) as exc:
# Never block launch: the import that follows surfaces any real breakage.
print(f"⚠ Could not check for an interrupted `hermes update`: {exc}", file=sys.stderr)
return False
def _restore_holding_claim(root: Path, marker: Path) -> bool:
global _merge_advice_shown
git_dir = marker.parent
fields = dict(line.partition("=")[::2] for line in marker.read_text(encoding="utf-8").splitlines())
try:
owner = int(fields.get("pid", ""))
except ValueError:
owner = -1
# Our own pid is never the owner: this runs at startup, and containers hand a retry the
# killed updater's pid.
if (owner != os.getpid() and _pid_is_running(owner)
and time.time() - marker.stat().st_mtime < _INTERRUPTED_PULL_MAX_AGE_SECONDS):
return False
pre, target = fields.get("pre", "").strip(), fields.get("target", "").strip()
stash = fields.get("stash", "").strip()
def git(*args: str, stdin: str | None = None) -> subprocess.CompletedProcess:
return subprocess.run(["git", "--literal-pathspecs", "-C", str(root), *args], input=stdin,
capture_output=True, text=True, encoding="utf-8", errors="replace",
timeout=120, stdin=None if stdin is not None else subprocess.DEVNULL)
if not pre or not target or git("rev-parse", "HEAD").stdout.strip() != pre:
marker.unlink() # git finished (HEAD moved) or the marker is unusable
return False
if any((git_dir / name).exists() for name in _GIT_OPERATION_IN_PROGRESS):
merge_head = git_dir / "MERGE_HEAD"
if (not _merge_advice_shown and merge_head.is_file()
and merge_head.read_text(encoding="utf-8").strip() == target):
# The killed updater's own merge: its conflict markers may sit in startup modules.
_merge_advice_shown = True
print(f"⚠ A killed `hermes update` left its merge unfinished. Run `git -C {root} merge --abort`, "
"then launch again." + (f" Your local changes are in its stash ({stash})." if stash else ""),
file=sys.stderr)
return False
written = _paths_git_wrote(git, root, pre, target)
if written is None: # after a gc or re-clone: nothing left to compare against
marker.unlink()
print(f"⚠ Ignoring a stale interrupted-update marker: commit {target[:10]} is gone.", file=sys.stderr)
return False
restore, added, new_dirs = written
if restore or added:
print("⚠ A previous `hermes update` was killed while git was writing the new code — "
f"restoring the checkout to {pre[:10]}...", file=sys.stderr)
if not _release_dead_index_lock(git_dir):
print(" A running git holds the index; the next launch finishes the restore.", file=sys.stderr)
return False
failed = None
if restore:
run = git("restore", "--source=HEAD", "--staged", "--worktree", "--pathspec-from-file=-",
"--pathspec-file-nul", stdin="\0".join(restore))
failed = run if run.returncode else None
if added and not failed:
run = git("rm", "-q", "--cached", "--ignore-unmatch", "--pathspec-from-file=-",
"--pathspec-file-nul", stdin="\0".join(added))
failed = run if run.returncode else None
for rel in added:
(root / rel).unlink(missing_ok=True)
if failed:
# No manual recipe: a reset would also wipe the edits this restore keeps, and the
# marker stays so the next launch retries.
reason = (failed.stderr.strip().splitlines() or ["git failed"])[-1]
print(f" ✗ Could not restore it automatically ({reason}); the next launch retries.",
file=sys.stderr)
return False
for rel in sorted(new_dirs, key=lambda d: d.count("/"), reverse=True):
with contextlib.suppress(OSError):
(root / rel).rmdir() # only when empty: an untracked file inside keeps it
marker.unlink()
if not restore and not added:
return False # the killed git never reached the tree: nothing to put back
print(" ✓ Checkout restored; `hermes update` updates it again.", file=sys.stderr)
if stash:
print(f" Your local changes are still in the update's stash ({stash}).", file=sys.stderr)
return True
def relaunch_after_restore() -> None:
"""Re-run this command from the restored tree; never returns.

78
tests/hermes_cli/test_update_interrupted_pull.py
View File

@@ -28,13 +28,16 @@ _MULTI = "top = 1\nx = 0\ny = 0\nz = 0\nend = 1\n"
# Runs an entry module with the repair replaced by a probe that lists the checkout modules imported so
# far (the entry module, its packages and what hermes_bootstrap needs excluded), then stops.
# far (the entry module, its package's __init__ and what hermes_bootstrap needs excluded: those run
# before any code in the entry can), then stops.
_ENTRY_SPY = """
import importlib, json, os, sys
import hermes_bootstrap
from hermes_cli import _early_recovery as er
before, venv, entry = set(sys.modules), os.path.realpath(sys.prefix), sys.argv[1]
venv, entry = os.path.realpath(sys.prefix), sys.argv[1]
importlib.import_module(entry.rpartition(".")[0] or "hermes_cli")
before = set(sys.modules)
def probe():
loaded = (n for n in set(sys.modules) - before if not f"{entry}.".startswith(n + "."))
@@ -67,6 +70,8 @@ def checkout(tmp_path, monkeypatch):
(origin / "gone.py").unlink()
(origin / "newpkg").mkdir()
(origin / "newpkg" / "__init__.py").write_text("from utils import NEW\n", encoding="utf-8", newline="")
(origin / "newpkg" / "sub").mkdir()
(origin / "newpkg" / "sub" / "mod.py").write_text("m = 1\n", encoding="utf-8", newline="")
_git(origin, "add", "-A")
_git(origin, "update-index", "--chmod=+x", "tool.sh")
_git(origin, "commit", "-qm", "B")
@@ -95,6 +100,7 @@ def test_killed_pull_is_restored_on_next_launch_and_update_reruns(checkout, monk
(root / "half.py").write_bytes(b"h = 2 # long") # a multi-page write cut short
(root / "newpkg").mkdir()
(root / "newpkg" / "__init__.py").write_text("from utils import NEW\n", encoding="utf-8", newline="")
(root / "newpkg" / "sub").mkdir() # created for its next file, which the kill cut off
(root / ".git" / "index.lock").touch()
raise KeyboardInterrupt # SIGKILL: nothing after this line of the updater runs
return real(git_cmd, args, *rest, **kw)
@@ -137,13 +143,13 @@ def test_killed_pull_is_restored_on_next_launch_and_update_reruns(checkout, monk
# Every console script (`hermes`, `hermes-agent`, `hermes-acp`) repairs before its entry module imports
# any other checkout module past hermes_bootstrap: any of them may be a half-written file.
repo = os.path.realpath(Path(er.__file__).parent.parent)
for entry in ("hermes_cli.main", "run_agent", "acp_adapter.entry"):
for entry in ("hermes_cli.main", "agent.legacy_cli", "run_agent", "acp_adapter.entry"):
run = subprocess.run([sys.executable, "-c", _ENTRY_SPY, entry], cwd=repo, capture_output=True, text=True,
encoding="utf-8", env={**os.environ, "PYTHONPATH": repo}, timeout=120)
assert run.stdout.strip().splitlines()[-1:] == ["[]"], (entry, run.stdout[-500:], run.stderr[-2000:])
def test_restore_never_touches_user_work_when_git_wrote_nothing(checkout):
def test_restore_never_touches_user_work_when_git_wrote_nothing(checkout, capsys, monkeypatch):
"""sys.exit on a merge conflict is not a kill, and a marker git never acted on restores nothing."""
root, a, b = checkout
_git(root, "config", "user.email", "t@example.invalid")
@@ -165,8 +171,12 @@ def test_restore_never_touches_user_work_when_git_wrote_nothing(checkout):
assert (root / ".git" / "MERGE_HEAD").exists(), merge.stdout + merge.stderr
(root / "utils.py").write_text("OLD = 1 # resolved by hand\n", encoding="utf-8", newline="")
before = _git(root, "status", "--porcelain", "--untracked-files=all")
assert er.restore_interrupted_pull(root) is False
monkeypatch.setattr(er, "_merge_advice_shown", False, raising=False)
capsys.readouterr()
assert er.restore_interrupted_pull(root) is False and er.restore_interrupted_pull(root) is False
assert _git(root, "status", "--porcelain", "--untracked-files=all") == before
# MERGE_HEAD is the update's own target: say how to get out of it, once per launch.
assert capsys.readouterr().err.count(f"git -C {root} merge --abort") == 1
_git(root, "reset", "-q", "--hard") # the user gives up on the merge
(root / "utils.py").write_text("OLD = 1 # my stash, re-applied\n", encoding="utf-8", newline="")
# tool.sh only changes mode upstream and git never reached it: nothing to restore, no relaunch.
@@ -194,3 +204,61 @@ def test_restore_never_touches_user_work_when_git_wrote_nothing(checkout):
assert er.restore_interrupted_pull(root) is True
assert _git(root, "rev-parse", "HEAD") == pre and not marker.exists()
assert _git(root, "status", "--porcelain", "--untracked-files=all") == "M other.py"
_RACER = """
import sys
from pathlib import Path
from hermes_cli import _early_recovery as er
print("ready", flush=True)
sys.stdin.readline()
print(er.restore_interrupted_pull(Path(sys.argv[1])))
"""
def test_concurrent_launches_take_turns_and_all_rerun_from_the_restored_tree(tmp_path):
"""Launches racing on one torn checkout (a restarting gateway next to the user's CLI) restore once.
None may break another's git (index.lock), print recovery advice while another is restoring or
has finished, or carry on importing from a tree that changed under it.
"""
origin = tmp_path / "origin"
origin.mkdir()
_git(origin, "init", "-q", "-b", "main")
_git(origin, "config", "user.email", "t@example.invalid")
_git(origin, "config", "user.name", "t")
names = [f"m{i}.py" for i in range(300)] # enough work that the launches overlap
for i, name in enumerate(names):
(origin / name).write_text(f"V = 'old {i}'\n" * 50, encoding="utf-8", newline="")
_git(origin, "add", "-A")
_git(origin, "commit", "-qm", "A")
for i, name in enumerate(names):
(origin / name).write_text(f"V = 'new {i}'\n" * 50, encoding="utf-8", newline="")
_git(origin, "commit", "-qam", "B")
root = tmp_path / "install"
_git(tmp_path, "clone", "-q", str(origin), str(root))
_git(root, "reset", "-q", "--hard", "HEAD~1")
pre, target = _git(root, "rev-parse", "HEAD"), _git(root, "rev-parse", "origin/main")
for i, name in enumerate(names[:150]): # git got halfway
(root / name).write_text(f"V = 'new {i}'\n" * 50, encoding="utf-8", newline="")
(root / names[-1]).write_text("user edit\n", encoding="utf-8", newline="")
marker = er.interrupted_pull_marker(root)
marker.write_text(f"pid=0\npre={pre}\ntarget={target}\nstash=\n", encoding="utf-8", newline="")
repo = os.path.realpath(Path(er.__file__).parent.parent)
launches = [subprocess.Popen([sys.executable, "-c", _RACER, str(root)], cwd=repo, text=True, encoding="utf-8",
env={**os.environ, "PYTHONPATH": repo}, stdin=subprocess.PIPE,
stdout=subprocess.PIPE, stderr=subprocess.PIPE) for _ in range(3)]
for launch in launches:
assert launch.stdout.readline().strip() == "ready"
for launch in launches: # release them together
launch.stdin.write("go\n")
launch.stdin.flush()
results = [(launch.communicate(timeout=120), launch.returncode) for launch in launches]
for (out, err), code in results:
assert code == 0 and out.split()[-1:] == ["True"], (out, err)
assert "Could not" not in err and "reset --hard" not in err, err
assert _git(root, "status", "--porcelain", "--untracked-files=all") == f"M {names[-1]}"
assert (root / names[-1]).read_text(encoding="utf-8") == "user edit\n"
assert not marker.exists()