From 7fde19afcc45ea774446b93db1053069dc87a6dc Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sun, 5 Jul 2026 03:16:06 -0700 Subject: [PATCH] =?UTF-8?q?fix(cli):=20unwedge=20cua-driver=20installer=20?= =?UTF-8?q?timeouts=20=E2=80=94=20group-kill,=20stale-lock=20pre-clear,=20?= =?UTF-8?q?660s=20ceiling=20(#58767)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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. --- hermes_cli/tools_config.py | 144 +++++++++++++++++++- tests/hermes_cli/test_install_cua_driver.py | 134 ++++++++++++++++++ 2 files changed, 273 insertions(+), 5 deletions(-) diff --git a/hermes_cli/tools_config.py b/hermes_cli/tools_config.py index 047c2abe223..9a74877a0ca 100644 --- a/hermes_cli/tools_config.py +++ b/hermes_cli/tools_config.py @@ -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}") diff --git a/tests/hermes_cli/test_install_cua_driver.py b/tests/hermes_cli/test_install_cua_driver.py index f95a191512a..3b7dfa36601 100644 --- a/tests/hermes_cli/test_install_cua_driver.py +++ b/tests/hermes_cli/test_install_cua_driver.py @@ -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