mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-08 13:12:08 +00:00
fix(cli): unwedge cua-driver installer timeouts — group-kill, stale-lock pre-clear, 660s ceiling (#58767)
* fix(cli): unwedge cua-driver installer timeouts — group-kill on timeout, stale-lock pre-clear, 660s ceiling The cua-driver refresh in hermes update could wedge permanently: subprocess timeout (300s) killed only the outer shell, orphaning the curl|bash grandchildren and the upstream installer's concurrent-install lock (~/.cua-driver/packages/.install.lock.d). The installer only reclaims a stale lock after 600s of waiting — longer than our old ceiling — so every subsequent run was killed before recovery could fire: 'always times out'. - Run the installer in its own process group (start_new_session) and SIGKILL the whole group on timeout, so no lock-holding orphans survive. - Pre-clear a provably-stale lock (dead holder pid, or pid-less and older than the upstream 600s window) before invoking the installer. - Raise the ceiling to 660s (> upstream LOCK_STALE_AFTER_SECONDS=600). - Timeout message now names the lock path and the manual re-run command. Fixes #58762 * chore: suppress windows-footgun lint on platform-gated kill calls Both sites are POSIX-only: _clear_stale_cua_install_lock early-returns on win32, and os.killpg sits in the 'not is_windows' branch.
This commit is contained in:
Teknium
GitHub
parent
d537d29a6f
commit
7fde19afcc
2 changed files with 273 additions and 5 deletions
|
|
@ -862,6 +862,87 @@ def install_cua_driver(upgrade: bool = False) -> bool:
|
|||
return ok
|
||||
|
||||
|
||||
# Ceiling for one upstream-installer run. Must exceed the installer's own
|
||||
# stale-lock recovery window: _install-rust.sh serializes concurrent installs
|
||||
# with a lock dir at ~/.cua-driver/packages/.install.lock.d and only
|
||||
# force-releases a dead holder's lock after LOCK_STALE_AFTER_SECONDS=600 of
|
||||
# waiting. With a shorter Python-side timeout, a stale lock means every run
|
||||
# gets killed before the installer's recovery can fire — a permanent
|
||||
# "always times out" wedge (issue #58762). 660s = 600s lock window + 60s
|
||||
# headroom for the actual download/swap.
|
||||
_CUA_INSTALLER_TIMEOUT = 660
|
||||
|
||||
# Upstream installer's stale-lock threshold (LOCK_STALE_AFTER_SECONDS in
|
||||
# _install-rust.sh). Used by the pre-clear below to avoid yanking a lock
|
||||
# that a live-but-slow install still holds.
|
||||
_CUA_LOCK_STALE_AFTER = 600
|
||||
|
||||
|
||||
def _cua_install_lock_dir() -> "Path":
|
||||
"""Path of the upstream installer's concurrent-install lock dir."""
|
||||
home = os.environ.get("CUA_DRIVER_RS_HOME") or str(Path.home() / ".cua-driver")
|
||||
return Path(home) / "packages" / ".install.lock.d"
|
||||
|
||||
|
||||
def _clear_stale_cua_install_lock() -> None:
|
||||
"""Best-effort: remove a stale installer lock left by a dead holder.
|
||||
|
||||
A previous timed-out/killed install can orphan
|
||||
``~/.cua-driver/packages/.install.lock.d`` (the holder's pid is stamped
|
||||
into its ``info`` file). The upstream installer only reclaims it after
|
||||
waiting 600s — longer than our old subprocess timeout — so an orphaned
|
||||
lock wedged every subsequent refresh. Clear it up front when the holder
|
||||
is provably dead; leave it alone when the holder is alive (a slow
|
||||
concurrent install) or liveness can't be determined.
|
||||
|
||||
POSIX-only: the lock protocol lives in the bash installer; install.ps1
|
||||
does not use it.
|
||||
"""
|
||||
if sys.platform == "win32":
|
||||
return
|
||||
lock_dir = _cua_install_lock_dir()
|
||||
try:
|
||||
if not lock_dir.is_dir():
|
||||
return
|
||||
holder_pid = None
|
||||
info = lock_dir / "info"
|
||||
try:
|
||||
for line in info.read_text(encoding="utf-8", errors="replace").splitlines():
|
||||
if line.startswith("pid="):
|
||||
holder_pid = int(line.split("=", 1)[1].strip())
|
||||
break
|
||||
except (OSError, ValueError):
|
||||
holder_pid = None
|
||||
|
||||
if holder_pid is not None:
|
||||
try:
|
||||
os.kill(holder_pid, 0) # windows-footgun: ok — function early-returns on win32
|
||||
# Holder alive → a concurrent install is running; don't touch.
|
||||
return
|
||||
except ProcessLookupError:
|
||||
pass # dead holder → stale, clear below
|
||||
except PermissionError:
|
||||
# Alive but owned by someone else — treat as live.
|
||||
return
|
||||
else:
|
||||
# No readable pid. Only clear if the lock is old enough that the
|
||||
# upstream installer itself would consider it reclaimable.
|
||||
import time as _time
|
||||
try:
|
||||
age = _time.time() - lock_dir.stat().st_mtime
|
||||
except OSError:
|
||||
return
|
||||
if age < _CUA_LOCK_STALE_AFTER:
|
||||
return
|
||||
|
||||
import shutil as _shutil
|
||||
_shutil.rmtree(lock_dir, ignore_errors=True)
|
||||
logger.info("Cleared stale cua-driver install lock at %s", lock_dir)
|
||||
_print_info(f" Cleared stale cua-driver install lock ({lock_dir}).")
|
||||
except Exception as e:
|
||||
logger.debug("stale cua install lock check failed: %s", e)
|
||||
|
||||
|
||||
def _run_cua_driver_installer(label: str = "Installing", verbose: bool = True) -> bool:
|
||||
"""Run the upstream cua-driver installer for this platform.
|
||||
|
||||
|
|
@ -909,6 +990,30 @@ def _run_cua_driver_installer(label: str = "Installing", verbose: bool = True) -
|
|||
else:
|
||||
_print_info(f" {label} cua-driver...")
|
||||
driver_cmd = _cua_driver_cmd()
|
||||
|
||||
# A previous timed-out install can leave the upstream installer's
|
||||
# concurrent-install lock behind; clear it when provably stale so the
|
||||
# refresh doesn't wedge waiting on a dead holder (issue #58762).
|
||||
_clear_stale_cua_install_lock()
|
||||
|
||||
# POSIX: run the installer in its own process group so a timeout kill
|
||||
# takes out the whole `curl | bash` pipeline (and the exec'd
|
||||
# _install-rust.sh), not just the outer shell. Otherwise the surviving
|
||||
# grandchildren keep holding the install lock, wedging every later run.
|
||||
popen_kwargs = {}
|
||||
if not is_windows:
|
||||
popen_kwargs["start_new_session"] = True
|
||||
|
||||
def _kill_installer_tree(proc):
|
||||
import signal as _signal
|
||||
try:
|
||||
if not is_windows:
|
||||
os.killpg(os.getpgid(proc.pid), _signal.SIGKILL) # windows-footgun: ok — POSIX branch only
|
||||
else:
|
||||
proc.kill()
|
||||
except (OSError, ProcessLookupError):
|
||||
proc.kill()
|
||||
|
||||
try:
|
||||
# When not verbose (e.g. `hermes update`'s refresh), capture the
|
||||
# installer's chatty "Next steps" wall instead of dumping it to the
|
||||
|
|
@ -916,12 +1021,32 @@ def _run_cua_driver_installer(label: str = "Installing", verbose: bool = True) -
|
|||
# debuggable. Verbose installs (interactive `computer-use install`)
|
||||
# keep streaming live.
|
||||
if verbose:
|
||||
result = subprocess.run(install_cmd, shell=use_shell, timeout=300, env=_cua_driver_env())
|
||||
proc = subprocess.Popen(
|
||||
install_cmd, shell=use_shell, env=_cua_driver_env(), **popen_kwargs
|
||||
)
|
||||
try:
|
||||
proc.communicate(timeout=_CUA_INSTALLER_TIMEOUT)
|
||||
except subprocess.TimeoutExpired:
|
||||
_kill_installer_tree(proc)
|
||||
proc.communicate()
|
||||
raise
|
||||
result = subprocess.CompletedProcess(
|
||||
install_cmd, proc.returncode, stdout=None, stderr=None
|
||||
)
|
||||
else:
|
||||
result = subprocess.run(
|
||||
install_cmd, shell=use_shell, timeout=300, env=_cua_driver_env(),
|
||||
proc = subprocess.Popen(
|
||||
install_cmd, shell=use_shell, env=_cua_driver_env(),
|
||||
stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
|
||||
text=True, encoding="utf-8", errors="replace",
|
||||
text=True, encoding="utf-8", errors="replace", **popen_kwargs
|
||||
)
|
||||
try:
|
||||
out, _ = proc.communicate(timeout=_CUA_INSTALLER_TIMEOUT)
|
||||
except subprocess.TimeoutExpired:
|
||||
_kill_installer_tree(proc)
|
||||
proc.communicate()
|
||||
raise
|
||||
result = subprocess.CompletedProcess(
|
||||
install_cmd, proc.returncode, stdout=out, stderr=None
|
||||
)
|
||||
# Preserve the full installer output. During `hermes update`,
|
||||
# sys.stdout is the mirroring _UpdateOutputStream whose `_log`
|
||||
|
|
@ -960,7 +1085,16 @@ def _run_cua_driver_installer(label: str = "Installing", verbose: bool = True) -
|
|||
_print_info(f" {manual_hint}")
|
||||
return False
|
||||
except subprocess.TimeoutExpired:
|
||||
_print_warning(f" cua-driver {label.lower()} timed out. Re-run manually.")
|
||||
_print_warning(
|
||||
f" cua-driver {label.lower()} timed out after "
|
||||
f"{_CUA_INSTALLER_TIMEOUT}s."
|
||||
)
|
||||
if not is_windows:
|
||||
_print_info(
|
||||
" If this repeats, a stale installer lock may be present — "
|
||||
f"check {_cua_install_lock_dir()}"
|
||||
)
|
||||
_print_info(f" Re-run manually: {manual_hint}")
|
||||
return False
|
||||
except Exception as e:
|
||||
_print_warning(f" cua-driver {label.lower()} failed: {e}")
|
||||
|
|
|
|||
|
|
@ -192,3 +192,137 @@ class TestArchProbeRemoval:
|
|||
runner.assert_called_once()
|
||||
# Probe deleted — no direct GitHub API call from Python.
|
||||
urlopen.assert_not_called()
|
||||
|
||||
|
||||
class TestStaleInstallLockClear:
|
||||
"""_clear_stale_cua_install_lock: pre-clears the upstream installer's
|
||||
concurrent-install lock only when the holder is provably dead (or the
|
||||
lock is old and pid-less). Issue #58762."""
|
||||
|
||||
def _make_lock(self, tmp_path, pid=None):
|
||||
import os
|
||||
home = tmp_path / ".cua-driver"
|
||||
lock = home / "packages" / ".install.lock.d"
|
||||
lock.mkdir(parents=True)
|
||||
if pid is not None:
|
||||
(lock / "info").write_text(f"pid={pid}\n")
|
||||
os.environ["CUA_DRIVER_RS_HOME"] = str(home)
|
||||
return lock
|
||||
|
||||
def teardown_method(self):
|
||||
import os
|
||||
os.environ.pop("CUA_DRIVER_RS_HOME", None)
|
||||
|
||||
def test_dead_holder_lock_is_cleared(self, tmp_path):
|
||||
from hermes_cli import tools_config
|
||||
|
||||
dead_pid = 4194000 # above default pid_max on most systems
|
||||
lock = self._make_lock(tmp_path, pid=dead_pid)
|
||||
with patch.object(tools_config, "_print_info"):
|
||||
tools_config._clear_stale_cua_install_lock()
|
||||
assert not lock.exists()
|
||||
|
||||
def test_live_holder_lock_is_kept(self, tmp_path):
|
||||
import os
|
||||
from hermes_cli import tools_config
|
||||
|
||||
lock = self._make_lock(tmp_path, pid=os.getpid())
|
||||
tools_config._clear_stale_cua_install_lock()
|
||||
assert lock.exists()
|
||||
|
||||
def test_pidless_fresh_lock_is_kept(self, tmp_path):
|
||||
from hermes_cli import tools_config
|
||||
|
||||
lock = self._make_lock(tmp_path, pid=None)
|
||||
tools_config._clear_stale_cua_install_lock()
|
||||
assert lock.exists()
|
||||
|
||||
def test_pidless_old_lock_is_cleared(self, tmp_path):
|
||||
import os
|
||||
import time
|
||||
from hermes_cli import tools_config
|
||||
|
||||
lock = self._make_lock(tmp_path, pid=None)
|
||||
old = time.time() - (tools_config._CUA_LOCK_STALE_AFTER + 60)
|
||||
os.utime(lock, (old, old))
|
||||
with patch.object(tools_config, "_print_info"):
|
||||
tools_config._clear_stale_cua_install_lock()
|
||||
assert not lock.exists()
|
||||
|
||||
def test_no_lock_is_noop(self, tmp_path):
|
||||
import os
|
||||
os.environ["CUA_DRIVER_RS_HOME"] = str(tmp_path / ".cua-driver")
|
||||
from hermes_cli import tools_config
|
||||
tools_config._clear_stale_cua_install_lock() # must not raise
|
||||
|
||||
|
||||
class TestInstallerTimeoutKillsProcessGroup:
|
||||
"""On timeout the whole installer process group must be killed, so the
|
||||
`curl | bash` grandchildren can't survive holding the install lock."""
|
||||
|
||||
def test_timeout_kills_process_group_and_returns_false(self, tmp_path):
|
||||
import os
|
||||
import signal
|
||||
import subprocess
|
||||
import sys as _sys
|
||||
from unittest.mock import MagicMock
|
||||
from hermes_cli import tools_config
|
||||
|
||||
killed = {}
|
||||
|
||||
fake_proc = MagicMock()
|
||||
fake_proc.pid = 12345
|
||||
# First communicate() raises TimeoutExpired, second (post-kill) returns.
|
||||
fake_proc.communicate.side_effect = [
|
||||
subprocess.TimeoutExpired(cmd="x", timeout=1),
|
||||
("", None),
|
||||
]
|
||||
|
||||
def fake_killpg(pgid, sig):
|
||||
killed["pgid"] = pgid
|
||||
killed["sig"] = sig
|
||||
|
||||
with patch("platform.system", return_value="Linux"), \
|
||||
patch("subprocess.Popen", return_value=fake_proc), \
|
||||
patch.object(tools_config.os, "getpgid", return_value=99999), \
|
||||
patch.object(tools_config.os, "killpg", side_effect=fake_killpg), \
|
||||
patch.object(tools_config, "_clear_stale_cua_install_lock"), \
|
||||
patch.object(tools_config, "_print_warning"), \
|
||||
patch.object(tools_config, "_print_info"):
|
||||
ok = tools_config._run_cua_driver_installer(label="Refreshing", verbose=False)
|
||||
|
||||
assert ok is False
|
||||
assert killed.get("pgid") == 99999
|
||||
assert killed.get("sig") == signal.SIGKILL
|
||||
# Post-kill reap happened.
|
||||
assert fake_proc.communicate.call_count == 2
|
||||
|
||||
def test_timeout_ceiling_exceeds_upstream_lock_window(self):
|
||||
from hermes_cli import tools_config
|
||||
# The upstream installer waits up to 600s before reclaiming a stale
|
||||
# lock; our ceiling must give that window room to complete.
|
||||
assert tools_config._CUA_INSTALLER_TIMEOUT > tools_config._CUA_LOCK_STALE_AFTER
|
||||
|
||||
def test_installer_runs_in_new_session_on_posix(self, tmp_path):
|
||||
import subprocess
|
||||
from unittest.mock import MagicMock
|
||||
from hermes_cli import tools_config
|
||||
|
||||
captured = {}
|
||||
fake_proc = MagicMock()
|
||||
fake_proc.pid = 1
|
||||
fake_proc.returncode = 1
|
||||
fake_proc.communicate.return_value = ("", None)
|
||||
|
||||
def fake_popen(*args, **kwargs):
|
||||
captured.update(kwargs)
|
||||
return fake_proc
|
||||
|
||||
with patch("platform.system", return_value="Linux"), \
|
||||
patch("subprocess.Popen", side_effect=fake_popen), \
|
||||
patch.object(tools_config, "_clear_stale_cua_install_lock"), \
|
||||
patch.object(tools_config, "_print_warning"), \
|
||||
patch.object(tools_config, "_print_info"):
|
||||
tools_config._run_cua_driver_installer(label="Refreshing", verbose=False)
|
||||
|
||||
assert captured.get("start_new_session") is True
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue