fix(health): preserve restart failure contract and proofs (#5156)

api/gateway_restart.py

@@ -100,7 +100,7 @@ def restart_active_profile_gateway(
             logger.error("Gateway service restart failed with code %s: %s", proc.returncode, stderr)
             return {
                 "status": "failed",
-                "message": f"Restart failed: {stderr or stdout or f'exit code {proc.returncode}'}",
+                "message": f"Restart failed: {stderr or stdout}",
                 "detail": stdout or stderr,
                 "returncode": proc.returncode,
             }

tests/test_health_restart.py

@@ -1,19 +1,165 @@
-"""Health route restart contract checks for /api/health/restart."""
+"""Health route and shared gateway restart helper checks."""
 
+import io
+import subprocess
+import threading
 import types
 
-
+import api.gateway_restart as gateway_restart
 import api.routes as routes
 
 
+class MockPopen:
+    def __init__(
+        self,
+        args,
+        *,
+        stdout_text="",
+        stderr_text="",
+        returncode=0,
+        communicate_timeout=False,
+        wait_timeout=False,
+        env=None,
+    ):
+        self.args = args
+        self.env = env or {}
+        self.returncode = returncode
+        self.stdout = io.StringIO(stdout_text)
+        self.stderr = io.StringIO(stderr_text)
+        self.communicate_timeout = communicate_timeout
+        self.wait_timeout = wait_timeout
+        self.terminated = False
+        self.killed = False
+        self.communicate_timeout_arg = None
+        self.wait_timeout_arg = None
+
+    def communicate(self, timeout=None):
+        self.communicate_timeout_arg = timeout
+        if self.communicate_timeout:
+            raise subprocess.TimeoutExpired(self.args, timeout)
+        return self.stdout.getvalue(), self.stderr.getvalue()
+
+    def wait(self, timeout=None):
+        self.wait_timeout_arg = timeout
+        if self.wait_timeout:
+            raise subprocess.TimeoutExpired(self.args, timeout)
+        return self.returncode
+
+    def terminate(self):
+        self.terminated = True
+
+    def kill(self):
+        self.killed = True
+
+
+class InlineThread:
+    def __init__(self, *, target, args=(), daemon=None):
+        self.target = target
+        self.args = args
+        self.daemon = daemon
+
+    def start(self):
+        self.target(*self.args)
+
+
 def _call_health_restart(monkeypatch, helper_result):
     handler = types.SimpleNamespace()
     responses = []
-    monkeypatch.setattr(routes, "j", lambda handler, payload, **kw: responses.append((payload, kw.get("status", 200))) or True)
+    monkeypatch.setattr(
+        routes,
+        "j",
+        lambda handler, payload, **kw: responses.append((payload, kw.get("status", 200))) or True,
+    )
     monkeypatch.setattr(routes, "restart_active_profile_gateway", lambda: dict(helper_result))
     return routes._handle_health_restart(handler), responses
 
 
+def test_restart_active_profile_gateway_success_uses_active_profile_home(monkeypatch):
+    gateway_restart._GATEWAY_RESTART_LOCK = threading.Lock()
+    called = {}
+
+    def fake_popen(args, stdout=None, stderr=None, text=True, env=None):
+        called["args"] = args
+        called["env"] = env
+        return MockPopen(
+            args,
+            stdout_text="✓ Service restarted",
+            returncode=0,
+            env=env,
+        )
+
+    monkeypatch.setattr(gateway_restart, "get_active_hermes_home", lambda: "/mock/hermes/home")
+    monkeypatch.setattr(gateway_restart.shutil, "which", lambda cmd: "/mock/bin/hermes")
+    monkeypatch.setattr(gateway_restart.subprocess, "Popen", fake_popen)
+
+    result = gateway_restart.restart_active_profile_gateway()
+
+    assert result["status"] == "completed"
+    assert result["message"] == "Gateway service restarted successfully"
+    assert called["args"] == ["/mock/bin/hermes", "gateway", "restart"]
+    assert called["env"]["HERMES_HOME"] == "/mock/hermes/home"
+    assert gateway_restart._GATEWAY_RESTART_LOCK.locked() is False
+
+
+def test_restart_active_profile_gateway_failure_preserves_empty_output_contract(monkeypatch):
+    gateway_restart._GATEWAY_RESTART_LOCK = threading.Lock()
+
+    monkeypatch.setattr(gateway_restart, "get_active_hermes_home", lambda: "/mock/hermes/home")
+    monkeypatch.setattr(gateway_restart.shutil, "which", lambda cmd: "/mock/bin/hermes")
+    monkeypatch.setattr(
+        gateway_restart.subprocess,
+        "Popen",
+        lambda args, stdout=None, stderr=None, text=True, env=None: MockPopen(
+            args,
+            returncode=7,
+            env=env,
+        ),
+    )
+
+    result = gateway_restart.restart_active_profile_gateway()
+
+    assert result["status"] == "failed"
+    assert result["message"] == "Restart failed: "
+    assert result["returncode"] == 7
+    assert gateway_restart._GATEWAY_RESTART_LOCK.locked() is False
+
+
+def test_restart_active_profile_gateway_timeout_releases_lock_after_background_wait(monkeypatch):
+    gateway_restart._GATEWAY_RESTART_LOCK = threading.Lock()
+    proc = MockPopen(
+        ["/mock/bin/hermes", "gateway", "restart"],
+        communicate_timeout=True,
+        env={"HERMES_HOME": "/mock/hermes/home"},
+    )
+
+    monkeypatch.setattr(gateway_restart, "get_active_hermes_home", lambda: "/mock/hermes/home")
+    monkeypatch.setattr(gateway_restart.shutil, "which", lambda cmd: "/mock/bin/hermes")
+    monkeypatch.setattr(gateway_restart.subprocess, "Popen", lambda *args, **kwargs: proc)
+    monkeypatch.setattr(gateway_restart.threading, "Thread", InlineThread)
+
+    result = gateway_restart.restart_active_profile_gateway()
+
+    assert result["status"] == "in_progress"
+    assert proc.communicate_timeout_arg == 2.0
+    assert proc.wait_timeout_arg == 240.0
+    assert gateway_restart._GATEWAY_RESTART_LOCK.locked() is False
+
+
+def test_restart_active_profile_gateway_busy_reports_contention(monkeypatch):
+    gateway_restart._GATEWAY_RESTART_LOCK = threading.Lock()
+    assert gateway_restart._GATEWAY_RESTART_LOCK.acquire(blocking=False) is True
+
+    try:
+        result = gateway_restart.restart_active_profile_gateway()
+    finally:
+        gateway_restart._GATEWAY_RESTART_LOCK.release()
+
+    assert result == {
+        "status": "busy",
+        "message": "Restart already in progress. Please wait a moment and try again.",
+    }
+
+
 def test_handle_health_restart_success(monkeypatch):
     result, responses = _call_health_restart(
         monkeypatch,
@@ -32,6 +178,15 @@ def test_handle_health_restart_timeout(monkeypatch):
     assert responses == [({"ok": True, "message": "Gateway service restart initiated (in progress)"}, 200)]
 
 
+def test_handle_health_restart_failure_preserves_empty_output_message(monkeypatch):
+    result, responses = _call_health_restart(
+        monkeypatch,
+        {"status": "failed", "message": "Restart failed: "},
+    )
+    assert result is True
+    assert responses == [({"ok": False, "error": "Restart failed: "}, 500)]
+
+
 def test_handle_health_restart_failure(monkeypatch):
     result, responses = _call_health_restart(
         monkeypatch,

tests/test_update_apply_ui.py

@@ -1,15 +1,125 @@
 """Frontend regression coverage for Update Now apply failures (#1321)."""
+import json
 from pathlib import Path
 import re
+import shutil
+import subprocess
+
+import pytest
 
 ROOT = Path(__file__).resolve().parents[1]
 UI_JS = ROOT / "static" / "ui.js"
+NODE = shutil.which("node")
 
 
 def _ui_js() -> str:
     return UI_JS.read_text(encoding="utf-8")
 
 
+def _run_apply_updates_harness(update_data, responses):
+    if NODE is None:
+        pytest.skip("node not available for applyUpdates harness")
+    js = r"""
+const fs = require('fs');
+const updateData = JSON.parse(process.argv[1]);
+const responses = JSON.parse(process.argv[2]);
+const uiPath = process.argv[3];
+const src = fs.readFileSync(uiPath, 'utf8');
+const start = src.indexOf('async function applyUpdates()');
+const end = src.indexOf('function _showUpdateError', start);
+const snippet = src.slice(start, end);
+
+const button = { disabled: false, textContent: 'Update Now' };
+const errorEl = { style: { display: 'none' }, textContent: '' };
+const forceBtn = { style: { display: 'block' }, dataset: { target: 'stale-target' } };
+const dom = {
+  btnApplyUpdate: button,
+  updateError: errorEl,
+  btnForceUpdate: forceBtn,
+};
+const sessionStorage = {
+  removed: [],
+  removeItem(key) { this.removed.push(key); },
+};
+const showToasts = [];
+const apiCalls = [];
+const waitCalls = [];
+const errors = [];
+let readHealthCalls = 0;
+
+function $(id) { return dom[id] || null; }
+async function api(url, opts) {
+  const payload = JSON.parse(opts.body);
+  apiCalls.push(payload.target);
+  const res = responses[apiCalls.length - 1];
+  if (res.throwMessage) throw new Error(res.throwMessage);
+  return res;
+}
+async function _readHealthServerIdentity() {
+  readHealthCalls += 1;
+  return 'baseline-id';
+}
+function _waitForServerThenReload(opts) {
+  waitCalls.push({
+    baselineServerIdentity: opts.baselineServerIdentity,
+    apiCallsSnapshot: apiCalls.slice(),
+  });
+}
+function _showUpdateError(target, res) { errors.push({ target, message: res.message || '' }); }
+function _formatUpdateApplyExceptionMessage(e) { return 'Update failed: ' + e.message; }
+function showToast(message, duration, kind) {
+  showToasts.push({ message, duration, kind });
+}
+function setTimeout(cb, ms) { cb(); return 1; }
+function clearTimeout() {}
+
+global.window = { _updateApplyInFlight: false, _updateData: updateData };
+global.sessionStorage = sessionStorage;
+global.$ = $;
+global.api = api;
+global._readHealthServerIdentity = _readHealthServerIdentity;
+global._waitForServerThenReload = _waitForServerThenReload;
+global._showUpdateError = _showUpdateError;
+global._formatUpdateApplyExceptionMessage = _formatUpdateApplyExceptionMessage;
+global.showToast = showToast;
+global.setTimeout = setTimeout;
+global.clearTimeout = clearTimeout;
+
+eval(snippet);
+
+(async () => {
+  await applyUpdates();
+  console.log(JSON.stringify({
+    apiCalls,
+    waitCalls,
+    errors,
+    showToasts,
+    errorDisplay: errorEl.style.display,
+    errorText: errorEl.textContent,
+    removedKeys: sessionStorage.removed,
+    inFlight: window._updateApplyInFlight,
+    buttonDisabled: button.disabled,
+    buttonText: button.textContent,
+    forceHidden: forceBtn.style.display,
+    forceTarget: forceBtn.dataset.target,
+    readHealthCalls,
+  }));
+})().catch((error) => {
+  console.error(error.stack || String(error));
+  process.exit(1);
+});
+"""
+    result = subprocess.run(
+        [NODE, "-e", js, json.dumps(update_data), json.dumps(responses), str(UI_JS)],
+        capture_output=True,
+        text=True,
+        timeout=30,
+    )
+    if result.returncode != 0:
+        raise RuntimeError(f"node harness failed: {result.stderr or result.stdout}")
+    return json.loads(result.stdout.strip().splitlines()[-1])
+
+
 def test_update_apply_network_error_has_recovery_message_not_raw_failed_to_fetch():
     """Network/interrupted update apply failures should not surface raw fetch text alone."""
     src = _ui_js()
@@ -91,37 +201,37 @@ def test_update_apply_prevents_duplicate_apply_requests_while_in_flight():
 
 
 def test_update_apply_rejects_zero_target_success_path():
-    """Update Now must not claim success when no webui/agent target is selected."""
-    src = _ui_js()
-    apply_start = src.index("async function applyUpdates()")
-    target_agent = src.index("if(window._updateData?.agent?.behind>0) targets.push('agent');", apply_start)
-    try_start = src.index("try{", target_agent)
-    zero_target_guard = src.find("if(!targets.length)", target_agent, try_start)
-
-    assert zero_target_guard >= 0, (
-        "applyUpdates must return before the success/restart flow when targets is empty"
+    """Update Now must return before the toast and reload flow when nothing is behind."""
+    result = _run_apply_updates_harness(
+        {"agent": {"behind": 0}, "webui": {"behind": 0}},
+        [],
     )
+    assert result["apiCalls"] == []
+    assert result["waitCalls"] == []
+    assert result["showToasts"] == []
+    assert result["errorDisplay"] == "block"
+    assert "No update target selected" in result["errorText"]
 
 
 def test_apply_updates_queues_agent_before_webui():
-    src = _ui_js()
-    apply_start = src.index("async function applyUpdates()")
-    next_fn = src.index("function _showUpdateError", apply_start)
-    body = src[apply_start:next_fn]
-
-    agent_idx = body.index("if(window._updateData?.agent?.behind>0) targets.push('agent');")
-    webui_idx = body.index("if(window._updateData?.webui?.behind>0) targets.push('webui');")
-    assert agent_idx < webui_idx, "agent target should be queued before webui"
+    result = _run_apply_updates_harness(
+        {"agent": {"behind": 1}, "webui": {"behind": 1}},
+        [{"ok": True}, {"ok": True}],
+    )
+    assert result["apiCalls"] == ["agent", "webui"]
+    assert result["waitCalls"] == [
+        {
+            "baselineServerIdentity": "baseline-id",
+            "apiCallsSnapshot": ["agent", "webui"],
+        }
+    ]
 
 
 def test_apply_updates_wait_for_all_targets_before_reload():
-    src = _ui_js()
-    apply_start = src.index("async function applyUpdates()")
-    next_fn = src.index("function _showUpdateError", apply_start)
-    body = src[apply_start:next_fn]
-
-    loop_end = body.index("for(const target of targets)")
-    restart_wait = body.index("_waitForServerThenReload({baselineServerIdentity});")
-    assert loop_end < restart_wait, (
-        "_waitForServerThenReload should happen only after the update loop"
+    result = _run_apply_updates_harness(
+        {"agent": {"behind": 1}, "webui": {"behind": 1}},
+        [{"ok": True}, {"ok": False, "message": "webui failed"}],
     )
+    assert result["apiCalls"] == ["agent", "webui"]
+    assert result["waitCalls"] == []
+    assert result["errors"] == [{"target": "webui", "message": "webui failed"}]