fix(gateway): force-kill unresponsive gateway during systemd restart (#12438)

gateway/run.py

@@ -2056,35 +2056,65 @@ 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()
         if not hermes_cmd:
             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:

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,

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:

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, "<watcher>", "exec")
+    except SyntaxError as e:
+        pytest.fail(f"Watcher code has syntax error: {e}")

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