From 1b869c88594d22a3960ccc59c81a4372f502dc99 Mon Sep 17 00:00:00 2001 From: Weiyi Feng Date: Mon, 27 Apr 2026 15:44:24 +0000 Subject: [PATCH 1/4] fix(gateway): replace bash restart watcher with fcntl file-lock based Python process MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The detached /restart mechanism spawned a bash shell that polled the old gateway's PID with `kill -0` and then ran `hermes gateway restart`. This had two race conditions in container environments: 1. Zombie PID: `kill -0` on a zombie (Z) returns 0, so the bash wrapper could loop indefinitely until the zombie was reaped by init. 2. Cmdline matching: the bash command `hermes gateway restart` contained the string "hermes gateway", which matched `find_gateway_pids()`'s `_scan_gateway_pids` patterns. This caused the bash wrapper itself to be sent SIGTERM during the restart flow, which propagated to the child gateway process. Replace the bash wrapper with a minimal Python process that uses fcntl.flock(LOCK_EX) on the existing gateway.lock file. The kernel releases flock locks atomically when the owning process dies — regardless of zombie state. After the lock is released, the watcher tries LOCK_EX|LOCK_NB to check whether another gateway already claimed it (meaning someone else restarted), skipping if so. Changes: - gateway/run.py: _launch_detached_restart_command() now spawns python3 -c '' instead of bash -lc '' --- gateway/run.py | 72 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 51 insertions(+), 21 deletions(-) diff --git a/gateway/run.py b/gateway/run.py index 01eb52969378..b816ab20f1ac 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -2056,7 +2056,6 @@ def _clear_restart_failure_count(self, session_key: str) -> None: pass async def _launch_detached_restart_command(self) -> None: - import shutil import subprocess hermes_cmd = _resolve_hermes_bin() @@ -2064,27 +2063,58 @@ async def _launch_detached_restart_command(self) -> None: logger.error("Could not locate hermes binary for detached /restart") return - current_pid = os.getpid() - cmd = " ".join(shlex.quote(part) for part in hermes_cmd) - shell_cmd = ( - f"while kill -0 {current_pid} 2>/dev/null; do sleep 0.2; done; " - f"{cmd} gateway restart" - ) - setsid_bin = shutil.which("setsid") - if setsid_bin: - subprocess.Popen( - [setsid_bin, "bash", "-lc", shell_cmd], - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - start_new_session=True, - ) + # Instead of a bash wrapper (fragile: zombie PID can block kill -0 + # forever, and the bash cmdline matches _scan_gateway_pids patterns), + # spawn a minimal Python process that blocks on the existing + # gateway.lock file lock. fcntl.flock(LOCK_EX) is released atomically + # by the OS when the owning process exits — no PID polling, no + # zombie edge-case, no cmdline collision. + # + # After acquiring the lock, try to acquire it again non-blocking. + # If the lock is CLAIMABLE (no one else holds it), we're safe to + # restart. If someone else claimed it in the meantime (another + # /restart or a manual restart beat us to it), exit silently. + import fcntl + try: + from gateway.status import _get_gateway_lock_path + except ImportError: + # Fallback: derive path the same way status.py does + lock_path = get_hermes_home() / "gateway.lock" else: - subprocess.Popen( - ["bash", "-lc", shell_cmd], - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - start_new_session=True, - ) + lock_path = _get_gateway_lock_path() + + cmd_repr = repr([str(p) for p in hermes_cmd] + ["gateway", "restart"]) + lock_path_repr = repr(str(lock_path)) + + watcher_code = ( + "import fcntl, os, subprocess, sys\n" + f"lock_path = {lock_path_repr}\n" + "# Block until the old gateway releases the file lock.\n" + "# flock(LOCK_EX) waits indefinitely — no timeout, no polling.\n" + "# The lock is released by the kernel when the owner process\n" + "# dies (even if it becomes a zombie, the lock goes away).\n" + "fd = os.open(lock_path, os.O_RDONLY)\n" + "fcntl.flock(fd, fcntl.LOCK_EX)\n" + "# Old gateway is gone. Check if someone else already restarted\n" + "# by trying the lock non-blocking. If we can't get it, someone\n" + "# else is already running — skip.\n" + "try:\n" + " fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB)\n" + " fcntl.flock(fd, fcntl.LOCK_UN)\n" + "except BlockingIOError:\n" + " sys.exit(0) # another gateway already running\n" + "finally:\n" + " os.close(fd)\n" + f"subprocess.run({cmd_repr})\n" + ) + + subprocess.Popen( + [sys.executable, "-c", watcher_code], + stdin=subprocess.DEVNULL, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + start_new_session=True, + ) def request_restart(self, *, detached: bool = False, via_service: bool = False) -> bool: if self._restart_task_started: From aa5e5a94ab42e19ce4d73dd39d0b8254af4840ab Mon Sep 17 00:00:00 2001 From: cxgreat2014 Date: Wed, 29 Apr 2026 11:13:23 +0000 Subject: [PATCH 2/4] test: add tests for gateway restart fcntl file-lock watcher --- tests/gateway/test_restart_watcher.py | 122 ++++++++++++++++++++++++++ 1 file changed, 122 insertions(+) create mode 100644 tests/gateway/test_restart_watcher.py diff --git a/tests/gateway/test_restart_watcher.py b/tests/gateway/test_restart_watcher.py new file mode 100644 index 000000000000..e99674a06fa5 --- /dev/null +++ b/tests/gateway/test_restart_watcher.py @@ -0,0 +1,122 @@ +"""Tests for PR #16621: gateway restart watcher using fcntl file-lock.""" + +import sys +from unittest.mock import patch + +import pytest + +from gateway.run import GatewayRunner + + +@pytest.mark.asyncio +async def test_launch_detached_restart_spawns_python_not_bash(): + """The restart watcher must use Python+fcntl, not bash+kill -0.""" + runner = object.__new__(GatewayRunner) + runner._background_tasks = set() + + with ( + patch("gateway.run._resolve_hermes_bin", return_value=["/usr/bin/hermes"]), + patch("subprocess.Popen") as mock_popen, + patch("gateway.status._get_gateway_lock_path", return_value="/tmp/gateway.lock"), + patch("gateway.run.logger"), + ): + await runner._launch_detached_restart_command() + + mock_popen.assert_called_once() + args, kwargs = mock_popen.call_args + popen_cmd = args[0] if args else kwargs.get("args", []) + + # The command should be Python, NOT bash + assert popen_cmd[0] == sys.executable, ( + f"Expected Python ({sys.executable}), got {popen_cmd[0]}" + ) + assert popen_cmd[1] == "-c", "Expected '-c' to run inline code" + + watcher_code = popen_cmd[2] + + # Must use fcntl.flock, not kill -0 polling + assert "fcntl.flock" in watcher_code, ( + "Watcher must use fcntl.flock for lock-based waiting" + ) + assert "kill -0" not in watcher_code, ( + "Old bash kill -0 pattern must not be present" + ) + + # Must handle duplicate restart via non-blocking lock + assert "BlockingIOError" in watcher_code, ( + "Watcher must handle duplicate restart via BlockingIOError" + ) + assert "LOCK_NB" in watcher_code, ( + "Watcher must use non-blocking lock (LOCK_NB) for dedup" + ) + + # Must run in a new session (detached) + assert kwargs.get("start_new_session") is True, ( + "Watcher must run in a detached session" + ) + + +@pytest.mark.asyncio +async def test_launch_detached_restart_no_bash_invocation(): + """Verify no bash or setsid is invoked in the new watcher.""" + runner = object.__new__(GatewayRunner) + runner._background_tasks = set() + + with ( + patch("gateway.run._resolve_hermes_bin", return_value=["/usr/bin/hermes"]), + patch("subprocess.Popen") as mock_popen, + patch("gateway.status._get_gateway_lock_path", return_value="/tmp/gateway.lock"), + patch("gateway.run.logger"), + ): + await runner._launch_detached_restart_command() + + mock_popen.assert_called_once() + args, _ = mock_popen.call_args + popen_cmd = args[0] + + # No bash anywhere in the command + for part in popen_cmd: + assert "bash" not in str(part), ( + f"bash should not appear in restart command, found: {part}" + ) + + +@pytest.mark.asyncio +async def test_launch_detached_restart_graceful_missing_binary(): + """Should return silently (no crash) when hermes binary is not found.""" + runner = object.__new__(GatewayRunner) + runner._background_tasks = set() + + with ( + patch("gateway.run._resolve_hermes_bin", return_value=None), + patch("subprocess.Popen") as mock_popen, + patch("gateway.run.logger"), + ): + await runner._launch_detached_restart_command() + + # Must NOT call subprocess.Popen when no binary + mock_popen.assert_not_called() + + +@pytest.mark.asyncio +async def test_watcher_code_is_valid_python(): + """The generated watcher code must be syntactically valid Python.""" + runner = object.__new__(GatewayRunner) + runner._background_tasks = set() + + with ( + patch("gateway.run._resolve_hermes_bin", return_value=["/usr/bin/hermes"]), + patch("subprocess.Popen") as mock_popen, + patch("gateway.status._get_gateway_lock_path", return_value="/tmp/gateway.lock"), + patch("gateway.run.logger"), + ): + await runner._launch_detached_restart_command() + + args, _ = mock_popen.call_args + watcher_code = args[0][2] + + # Must compile without syntax errors + try: + compile(watcher_code, "", "exec") + except SyntaxError as e: + pytest.fail(f"Watcher code has syntax error: {e}") From 9afcf6b50c5f9e762801e2bb070aff0d1bf451a6 Mon Sep 17 00:00:00 2001 From: cxgreat2014 Date: Wed, 29 Apr 2026 17:42:00 +0000 Subject: [PATCH 3/4] fix(cli): reject literal 'custom' as provider slug in model picker When a prior failed model switch wrote `provider: custom` to config.yaml, `list_authenticated_providers()` would use the literal string `'custom'` as the provider slug instead of the canonical `custom:` format. This caused every subsequent session to fail with `Unknown provider 'custom'`. Fix: add `current_provider != custom` guard to the base-URL matching branch so the stale literal value doesn't propagate. Tests: - 4 new tests covering the bug scenario - `custom_provider_slug()` format validation --- hermes_cli/model_switch.py | 1 + .../test_model_switch_custom_provider.py | 78 +++++++++++++++++++ 2 files changed, 79 insertions(+) create mode 100644 tests/hermes_cli/test_model_switch_custom_provider.py diff --git a/hermes_cli/model_switch.py b/hermes_cli/model_switch.py index d9e1b04183a0..0e016c484e56 100644 --- a/hermes_cli/model_switch.py +++ b/hermes_cli/model_switch.py @@ -1388,6 +1388,7 @@ def list_authenticated_providers( if ( current_base_url and api_url == current_base_url.strip().rstrip("/") + and current_provider != "custom" ): slug = current_provider or custom_provider_slug(display_name) else: diff --git a/tests/hermes_cli/test_model_switch_custom_provider.py b/tests/hermes_cli/test_model_switch_custom_provider.py new file mode 100644 index 000000000000..032bc7f62d81 --- /dev/null +++ b/tests/hermes_cli/test_model_switch_custom_provider.py @@ -0,0 +1,78 @@ +"""Tests for custom provider slug assignment in list_authenticated_providers.""" + +import os +from unittest.mock import patch + +from hermes_cli.model_switch import list_authenticated_providers +from hermes_cli.providers import custom_provider_slug + + +class TestCustomProviderSlugAssignment: + """When current_provider is the literal 'custom', it must not be used as a slug. + + Regression test for #17478: a prior failed switch writes ``provider: custom`` + to config.yaml. On the next picker run, ``list_authenticated_providers()`` + assigns ``slug = 'custom'`` (the literal string) instead of the canonical + ``custom:`` slug, which causes ``resolve_provider_full('custom', ...)`` + to return None → ``Unknown provider 'custom'`` error. + """ + + CUSTOM_PROVIDERS = [ + { + "name": "xiaomi-coding", + "base_url": "https://token-plan-sgp.xiaomimimo.com/v1", + "api_key": "sk-test123", + } + ] + + def _call_with_custom(self, current_provider: str, current_base_url: str = "") -> str: + """Call list_authenticated_providers and extract the slug for our custom provider.""" + with ( + patch("agent.models_dev.fetch_models_dev", return_value={}), + patch("os.getenv", return_value=""), + patch("os.path.exists", return_value=False), + ): + results = list_authenticated_providers( + current_provider=current_provider, + current_base_url=current_base_url, + custom_providers=self.CUSTOM_PROVIDERS, + ) + + # Find our custom provider entry + for r in results: + if "xiaomi" in r.get("name", "").lower(): + return r["slug"] + return "" + + def test_when_current_provider_is_custom_literal_uses_canonical_slug(self): + """current_provider='custom' should NOT produce slug='custom'.""" + slug = self._call_with_custom( + current_provider="custom", + current_base_url="https://token-plan-sgp.xiaomimimo.com/v1", + ) + assert slug == "custom:xiaomi-coding", ( + f"Expected 'custom:xiaomi-coding', got '{slug}'" + ) + + def test_when_current_provider_is_empty_uses_canonical_slug(self): + """Empty current_provider should fall through to custom_provider_slug.""" + slug = self._call_with_custom( + current_provider="", + current_base_url="https://token-plan-sgp.xiaomimimo.com/v1", + ) + assert slug == "custom:xiaomi-coding" + + def test_when_base_url_does_not_match_uses_canonical_slug(self): + """Non-matching base_url should always fall through to canonical slug.""" + slug = self._call_with_custom( + current_provider="custom", + current_base_url="https://some-other-url.com/v1", + ) + assert slug == "custom:xiaomi-coding", ( + f"Expected fallback slug 'custom:xiaomi-coding', got '{slug}'" + ) + + def test_custom_provider_slug_format(self): + """custom_provider_slug must produce 'custom:' format.""" + assert custom_provider_slug("xiaomi-coding") == "custom:xiaomi-coding" + assert custom_provider_slug("My Provider") == "custom:my-provider" From b5710b829366cde432e8dc044bb30a95bb8c5a42 Mon Sep 17 00:00:00 2001 From: cxgreat2014 Date: Wed, 29 Apr 2026 18:10:47 +0000 Subject: [PATCH 4/4] fix(gateway): force-kill unresponsive gateway during systemd restart (#12438) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the gateway is in a crashed/unresponsive state (hung event loop, crash-loop), 'hermes gateway restart' sends SIGUSR1 but the signal handler never executes. The 90s drain timeout expires with a warning, but then 'systemctl start' is a no-op because systemd still sees the hung process as alive — leaving the gateway permanently stuck. Fix: 1. After the 90s drain timeout, force-kill (SIGKILL) the stuck gateway process via terminate_pid(pid, force=True) 2. Use 'systemctl restart' instead of 'systemctl start' so systemd explicitly relaunches the service regardless of prior state Adds a regression test: test_systemd_restart_force_kills_unresponsive_gateway verifies terminate_pid(force=True) is called and systemctl restart is used. Existing tests updated to match 'systemctl restart'. Fixes #12438 --- hermes_cli/gateway.py | 8 ++- tests/hermes_cli/test_gateway_service.py | 91 +++++++++++++++++++++++- 2 files changed, 95 insertions(+), 4 deletions(-) diff --git a/hermes_cli/gateway.py b/hermes_cli/gateway.py index aede480bfed7..6e58d902fad6 100644 --- a/hermes_cli/gateway.py +++ b/hermes_cli/gateway.py @@ -1865,6 +1865,12 @@ def systemd_restart(system: bool = False): break # old process is gone else: print(f"⚠ Old process (PID {pid}) still alive after 90s") + # Gateway is stuck (crashed, hung event loop, etc.). SIGUSR1 was + # sent but never handled. Force-kill so systemd can relaunch. + from gateway.status import terminate_pid + + terminate_pid(pid, force=True) + time.sleep(0.5) # The gateway exits with code 75 for a planned service restart. # systemd can sit in the RestartSec window or even wedge itself into a @@ -1878,7 +1884,7 @@ def systemd_restart(system: bool = False): timeout=30, ) _run_systemctl( - ["start", svc], + ["restart", svc], system=system, check=False, timeout=90, diff --git a/tests/hermes_cli/test_gateway_service.py b/tests/hermes_cli/test_gateway_service.py index bd429bff2b4a..ca9e61eb3d24 100644 --- a/tests/hermes_cli/test_gateway_service.py +++ b/tests/hermes_cli/test_gateway_service.py @@ -490,8 +490,8 @@ def fake_subprocess_run(cmd, **kwargs): if "reset-failed" in cmd: calls.append(("reset-failed", cmd)) return SimpleNamespace(stdout="", returncode=0) - if "start" in cmd: - calls.append(("start", cmd)) + if "restart" in cmd: + calls.append(("restart", cmd)) return SimpleNamespace(stdout="", returncode=0) if "show" in cmd: new_pid[0] = 999 @@ -513,7 +513,92 @@ def fake_get_pid(): assert ("self", 654) in calls assert any(call[0] == "reset-failed" for call in calls) - assert any(call[0] == "start" for call in calls) + assert any(call[0] == "restart" for call in calls) + out = capsys.readouterr().out.lower() + assert "restarted" in out + + def test_systemd_restart_force_kills_unresponsive_gateway(self, monkeypatch, capsys): + calls = [] + + monkeypatch.setattr(gateway_cli, "_select_systemd_scope", lambda system=False: False) + monkeypatch.setattr(gateway_cli, "_preflight_user_systemd", lambda: None) + monkeypatch.setattr(gateway_cli, "refresh_systemd_unit_if_needed", lambda system=False: calls.append(("refresh",))) + + # Gateway has a running PID + monkeypatch.setattr( + "gateway.status.get_running_pid", + lambda: 654, + ) + + # SIGUSR1 sent successfully but process never dies (hung event loop) + monkeypatch.setattr( + gateway_cli, + "_request_gateway_self_restart", + lambda pid: calls.append(("self", pid)) or True, + ) + + # os.kill(PID, 0) always succeeds → drain loop times out after 90s + monkeypatch.setattr(os, "kill", lambda pid, sig: None) + + # Trap terminate_pid (force=True) call + terminate_pid_calls = [] + real_terminate_pid = gateway_cli.terminate_pid if hasattr(gateway_cli, "terminate_pid") else None + monkeypatch.setattr( + "gateway.status.terminate_pid", + lambda pid, *, force=False: terminate_pid_calls.append((pid, force)), + ) + + # Speed up the 90s drain loop so the test doesn't actually wait + import time as time_module + real_time = time_module.time + start_time = [0.0] + + def fake_time(): + if not start_time[0]: + start_time[0] = real_time() + # After a few iterations, jump past the 90s deadline + elapsed = real_time() - start_time[0] + if elapsed > 0.5: + return start_time[0] + 120 # well past 90s + return start_time[0] + + monkeypatch.setattr(time_module, "time", fake_time) + + # Mock systemctl calls + def fake_subprocess_run(cmd, **kwargs): + if "reset-failed" in cmd: + calls.append(("reset-failed", cmd)) + return SimpleNamespace(stdout="", returncode=0) + if "restart" in cmd: + calls.append(("restart", cmd)) + return SimpleNamespace(stdout="", returncode=0) + if "show" in cmd: + return SimpleNamespace( + stdout="ActiveState=active\nSubState=running\nResult=success\nExecMainStatus=0\n", + returncode=0, + ) + raise AssertionError(f"Unexpected systemctl call: {cmd}") + + monkeypatch.setattr(gateway_cli.subprocess, "run", fake_subprocess_run) + + # Simulate service becomes active with new PID + pid_calls = [0] + def fake_get_pid(): + pid_calls[0] += 1 + return 999 if pid_calls[0] > 1 else 654 + monkeypatch.setattr("gateway.status.get_running_pid", fake_get_pid) + + gateway_cli.systemd_restart() + + # Verify force-kill was attempted on the stuck process + assert any(c == (654, True) for c in terminate_pid_calls), ( + f"Expected terminate_pid(654, force=True) but got: {terminate_pid_calls}" + ) + + # Verify systemctl restart was used (not start) + assert any(call[0] == "restart" for call in calls), ( + f"Expected systemctl restart but got calls: {calls}" + ) out = capsys.readouterr().out.lower() assert "restarted" in out