From 315fdae5f8adba658cffc1400aaa1d4279e0a5e9 Mon Sep 17 00:00:00 2001 From: Hao Zhe Date: Wed, 17 Jun 2026 01:23:05 +0800 Subject: [PATCH] fix(memory): tighten OpenViking local autostart --- plugins/memory/openviking/__init__.py | 64 +++++++++++--- .../memory/test_openviking_provider.py | 87 +++++++++++++++++++ 2 files changed, 141 insertions(+), 10 deletions(-) diff --git a/plugins/memory/openviking/__init__.py b/plugins/memory/openviking/__init__.py index 07dd3317957..452c543d04d 100644 --- a/plugins/memory/openviking/__init__.py +++ b/plugins/memory/openviking/__init__.py @@ -88,6 +88,7 @@ _MEMORY_WRITE_TARGET_SUBDIR_MAP = { } _LOCAL_OPENVIKING_HOSTS = {"localhost", "127.0.0.1", "::1"} _LOCAL_OPENVIKING_AUTOSTART_TIMEOUT = 60.0 +_OPENVIKING_SERVER_LOG_RELATIVE_PATH = Path("logs") / "openviking-server.log" _SETUP_CANCELLED = object() @@ -664,7 +665,8 @@ def _is_local_openviking_url(value: str) -> bool: if "://" not in candidate: candidate = f"//{candidate}" parsed = urlparse(candidate) - return (parsed.hostname or "").lower() in _LOCAL_OPENVIKING_HOSTS + scheme = (parsed.scheme or "http").lower() + return scheme == "http" and (parsed.hostname or "").lower() in _LOCAL_OPENVIKING_HOSTS def _load_hermes_openviking_config() -> dict: @@ -939,6 +941,15 @@ def _local_openviking_bind(endpoint: str) -> tuple[str, int]: return host, port +def _openviking_server_log_path() -> Path: + try: + from hermes_constants import get_hermes_home + home = get_hermes_home() + except Exception: + home = Path(os.environ.get("HERMES_HOME", "")).expanduser() if os.environ.get("HERMES_HOME") else Path.home() / ".hermes" + return home / _OPENVIKING_SERVER_LOG_RELATIVE_PATH + + def _start_local_openviking_server(endpoint: str) -> tuple[bool, str]: server_cmd = shutil.which("openviking-server") if not server_cmd: @@ -947,17 +958,20 @@ def _start_local_openviking_server(endpoint: str) -> tuple[bool, str]: host, port = _local_openviking_bind(endpoint) except ValueError as e: return False, f"Could not parse local OpenViking URL: {e}" + log_path = _openviking_server_log_path() try: - subprocess.Popen( - [server_cmd, "--host", host, "--port", str(port)], - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - stdin=subprocess.DEVNULL, - start_new_session=True, - ) + log_path.parent.mkdir(parents=True, exist_ok=True) + with log_path.open("ab") as log_file: + subprocess.Popen( + [server_cmd, "--host", host, "--port", str(port)], + stdout=log_file, + stderr=log_file, + stdin=subprocess.DEVNULL, + start_new_session=True, + ) except Exception as e: return False, f"Could not start openviking-server: {e}" - return True, f"Started openviking-server on {host}:{port} in the background." + return True, f"Started openviking-server on {host}:{port} in the background. Logs: {log_path}" def _wait_for_openviking_health(endpoint: str, *, timeout_seconds: float = 15.0) -> bool: @@ -1036,6 +1050,29 @@ def _runtime_openviking_timeout_message(endpoint: str) -> str: ) +def _classify_runtime_openviking_health(client: _VikingClient, endpoint: str) -> tuple[str, str]: + """Classify runtime health without treating every false result as server absence.""" + try: + if hasattr(client, "health_payload"): + payload = client.health_payload() + if payload.get("healthy") is False: + return ( + "responded", + f"OpenViking server at {endpoint} responded but reported unhealthy status.", + ) + return "healthy", "" + if client.health(): + return "healthy", "" + except _OpenVikingHTTPError as e: + return ( + "responded", + f"OpenViking server at {endpoint} responded with {_format_openviking_exception(e)}.", + ) + except Exception: + return "unreachable", "" + return "unreachable", "" + + def _prompt_profile_name(prompt, select, cancelled) -> str | object: while True: name = _clean_config_value(prompt("OpenViking profile name")) @@ -1811,11 +1848,18 @@ class OpenVikingMemoryProvider(MemoryProvider): self._endpoint, self._api_key, account=self._account, user=self._user, agent=self._agent, ) - if not self._client.health(): + health_state, health_message = _classify_runtime_openviking_health(self._client, self._endpoint) + if health_state == "unreachable": self._handle_runtime_openviking_unreachable( status_callback=status_callback, warning_callback=warning_callback, ) + elif health_state != "healthy": + _emit_runtime_warning( + f"{health_message} OpenViking memory disabled for this Hermes run.", + warning_callback, + ) + self._client = None except ImportError: logger.warning("httpx not installed — OpenViking plugin disabled") self._client = None diff --git a/tests/plugins/memory/test_openviking_provider.py b/tests/plugins/memory/test_openviking_provider.py index 36e0658f338..d9aba21ca95 100644 --- a/tests/plugins/memory/test_openviking_provider.py +++ b/tests/plugins/memory/test_openviking_provider.py @@ -640,6 +640,93 @@ def test_start_local_openviking_server_uses_endpoint_host_and_port(monkeypatch): assert kwargs["start_new_session"] is True +def test_start_local_openviking_server_writes_output_to_log(tmp_path, monkeypatch): + hermes_home = tmp_path / "hermes" + monkeypatch.setenv("HERMES_HOME", str(hermes_home)) + popen_calls = [] + + class FakeProcess: + pass + + def fake_popen(args, **kwargs): + popen_calls.append((args, kwargs)) + assert kwargs["stdout"] is kwargs["stderr"] + assert kwargs["stdout"].name == str(hermes_home / "logs" / "openviking-server.log") + assert not kwargs["stdout"].closed + return FakeProcess() + + monkeypatch.setattr(openviking_module.shutil, "which", lambda name: "/usr/local/bin/openviking-server") + monkeypatch.setattr(openviking_module.subprocess, "Popen", fake_popen) + + started, message = openviking_module._start_local_openviking_server("http://127.0.0.1:1934") + + assert started is True + assert str(hermes_home / "logs" / "openviking-server.log") in message + assert popen_calls + + +def test_https_local_endpoint_is_not_runtime_autostart_eligible(monkeypatch): + _clear_openviking_env(monkeypatch) + monkeypatch.setenv("OPENVIKING_ENDPOINT", "https://localhost:1934") + + class FakeVikingClient: + def __init__(self, endpoint, api_key="", account="", user="", agent=""): + assert endpoint == "https://localhost:1934" + + def health(self): + return False + + monkeypatch.setattr(openviking_module, "_VikingClient", FakeVikingClient) + monkeypatch.setattr( + openviking_module, + "_start_local_openviking_server", + MagicMock(side_effect=AssertionError("https localhost endpoint should not auto-start")), + ) + + warnings = [] + provider = OpenVikingMemoryProvider() + provider.initialize("session-1", platform="cli", warning_callback=warnings.append) + + assert provider._client is None + assert warnings == [ + "Remote OpenViking server at https://localhost:1934 is not reachable; " + "OpenViking memory disabled for this Hermes run. " + "Check the configured endpoint and network connectivity." + ] + + +def test_runtime_does_not_autostart_when_local_server_reports_unhealthy(monkeypatch): + _clear_openviking_env(monkeypatch) + monkeypatch.setenv("OPENVIKING_ENDPOINT", "http://localhost:1934") + + class FakeVikingClient: + def __init__(self, endpoint, api_key="", account="", user="", agent=""): + assert endpoint == "http://localhost:1934" + + def health(self): + return False + + def health_payload(self): + return {"healthy": False} + + monkeypatch.setattr(openviking_module, "_VikingClient", FakeVikingClient) + monkeypatch.setattr( + openviking_module, + "_start_local_openviking_server", + MagicMock(side_effect=AssertionError("responding unhealthy server should not auto-start another process")), + ) + + warnings = [] + provider = OpenVikingMemoryProvider() + provider.initialize("session-1", platform="cli", warning_callback=warnings.append) + + assert provider._client is None + assert warnings == [ + "OpenViking server at http://localhost:1934 responded but reported unhealthy status. " + "OpenViking memory disabled for this Hermes run." + ] + + def test_handle_unreachable_endpoint_does_not_wait_when_autostart_command_missing(monkeypatch, capsys): monkeypatch.setattr( openviking_module,