From 31a443ff8871f04ff170ec50688b9cf0db21f3e7 Mon Sep 17 00:00:00 2001 From: Rod Boev Date: Sun, 28 Jun 2026 22:09:28 -0400 Subject: [PATCH] fix(health): preserve restart failure contract and proofs (#5156) --- api/gateway_restart.py | 2 +- tests/test_health_restart.py | 161 ++++++++++++++++++++++++++++++++- tests/test_update_apply_ui.py | 162 ++++++++++++++++++++++++++++------ 3 files changed, 295 insertions(+), 30 deletions(-) diff --git a/api/gateway_restart.py b/api/gateway_restart.py index 366b1ae8..2cf54f08 100644 --- a/api/gateway_restart.py +++ b/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, } diff --git a/tests/test_health_restart.py b/tests/test_health_restart.py index de2e0bf4..bc112695 100644 --- a/tests/test_health_restart.py +++ b/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, diff --git a/tests/test_update_apply_ui.py b/tests/test_update_apply_ui.py index a342a1d0..4618b1cf 100644 --- a/tests/test_update_apply_ui.py +++ b/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"}]