From b49427d85fd6628eb4a7fe099e5c390c5c4cc935 Mon Sep 17 00:00:00 2001 From: Jeff Mettel Date: Thu, 30 Jul 2026 13:12:17 -0500 Subject: [PATCH] fix(openviking): don't spawn a second server onto a live port MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_start_local_openviking_server()` spawned `openviking-server` unconditionally. Both callers — `initialize()` and the runtime unreachable handler — reach it from a health probe, and that probe can time out client-side while the server is up and serving. The spawned process then loses the data-directory lock and exits immediately with `DataDirectoryLocked`; because the probe keeps timing out, the cycle repeats every cooldown window (~5 min observed). The existing 30s `_failed_refresh` cooldown paces the loop but cannot stop it, since it expires while the underlying condition persists. Probe the target host:port before spawning and treat an occupied port as already-started. This guards both call sites at their single convergence point. The probe deliberately tests only that a listener owns the port — enough to know a second server would lose the lock — and says nothing about that listener's health. The parse/probe now precedes the PATH lookup, so a reachable server is reported as running even when `openviking-server` is not on PATH. Fixes #74846 Co-Authored-By: Claude Opus 5 (1M context) --- plugins/memory/openviking/__init__.py | 33 +++++++++- .../memory/test_openviking_provider.py | 65 +++++++++++++++++++ 2 files changed, 95 insertions(+), 3 deletions(-) diff --git a/plugins/memory/openviking/__init__.py b/plugins/memory/openviking/__init__.py index e59f9467ac236..e61f9d56d6848 100644 --- a/plugins/memory/openviking/__init__.py +++ b/plugins/memory/openviking/__init__.py @@ -33,6 +33,7 @@ import mimetypes import os import re import shutil +import socket import stat import subprocess import tempfile @@ -126,6 +127,10 @@ _GENERATED_MEMORY_SUMMARY_FILENAMES = { } _LOCAL_OPENVIKING_HOSTS = {"localhost", "127.0.0.1", "::1"} _LOCAL_OPENVIKING_AUTOSTART_TIMEOUT = 60.0 +# Pre-spawn liveness probe budget. A loopback TCP connect either completes or +# is refused in well under this; it exists only so a wedged listener cannot +# block the autostart path. +_LOCAL_OPENVIKING_PROBE_TIMEOUT = 2.0 # After a refresh attempt fails for a given (unchanged) config, skip re-probing # for this long. Keeps "unavailable endpoints reconnect on a later access" # true while preventing every provider access from paying a 3s health probe @@ -1214,14 +1219,36 @@ def _openviking_server_log_path() -> Path: return home / _OPENVIKING_SERVER_LOG_RELATIVE_PATH +def _local_openviking_port_is_open(host: str, port: int) -> bool: + """Return True when something already accepts TCP connections on host:port. + + Used as a pre-spawn guard only. A successful connect proves a listener owns + the port, which is enough to know a second ``openviking-server`` would lose + the data-directory lock — it deliberately says nothing about whether that + listener is healthy. + """ + try: + with socket.create_connection((host, port), timeout=_LOCAL_OPENVIKING_PROBE_TIMEOUT): + return True + except OSError: + return False + + def _start_local_openviking_server(endpoint: str) -> tuple[bool, str]: - server_cmd = shutil.which("openviking-server") - if not server_cmd: - return False, "openviking-server was not found on PATH. Start it manually, then retry." try: host, port = _local_openviking_bind(endpoint) except ValueError as e: return False, f"Could not parse local OpenViking URL: {e}" + # Health probes can time out client-side while the server is up and well. + # Spawning on that signal alone produces a process that immediately dies on + # DataDirectoryLocked, and — because the probe keeps timing out — repeats + # every cooldown window. Treat an occupied port as "already started": both + # callers only need the server running, not started by us. + if _local_openviking_port_is_open(host, port): + return True, f"openviking-server is already running on {host}:{port}." + server_cmd = shutil.which("openviking-server") + if not server_cmd: + return False, "openviking-server was not found on PATH. Start it manually, then retry." log_path = _openviking_server_log_path() try: log_path.parent.mkdir(parents=True, exist_ok=True) diff --git a/tests/plugins/memory/test_openviking_provider.py b/tests/plugins/memory/test_openviking_provider.py index 2b8d92e61d84b..f103691ce91ec 100644 --- a/tests/plugins/memory/test_openviking_provider.py +++ b/tests/plugins/memory/test_openviking_provider.py @@ -1,5 +1,6 @@ import json import os +import socket import stat import threading import time @@ -268,6 +269,7 @@ def test_start_local_openviking_server_uses_endpoint_host_and_port(monkeypatch): popen_calls.append((args, kwargs)) return object() + monkeypatch.setattr(openviking_module, "_local_openviking_port_is_open", lambda host, port: False) monkeypatch.setattr(openviking_module.shutil, "which", lambda name: "/usr/local/bin/openviking-server") monkeypatch.setattr(openviking_module.subprocess, "Popen", fake_popen) @@ -280,6 +282,69 @@ 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_does_not_spawn_when_port_already_open(monkeypatch): + """A live listener means a second server would just die on DataDirectoryLocked.""" + probed = [] + + def fake_probe(host, port): + probed.append((host, port)) + return True + + monkeypatch.setattr(openviking_module, "_local_openviking_port_is_open", fake_probe) + monkeypatch.setattr(openviking_module.shutil, "which", lambda name: "/usr/local/bin/openviking-server") + monkeypatch.setattr( + openviking_module.subprocess, + "Popen", + MagicMock(side_effect=AssertionError("must not spawn while a server is already listening")), + ) + + started, message = openviking_module._start_local_openviking_server("http://127.0.0.1:1934") + + assert started is True + assert "already running" in message + assert probed == [("127.0.0.1", 1934)] + + +def test_start_local_openviking_server_reports_running_server_without_cli_on_path(monkeypatch): + """The port probe outranks PATH: a reachable server is started, whoever launched it.""" + monkeypatch.setattr(openviking_module, "_local_openviking_port_is_open", lambda host, port: True) + monkeypatch.setattr(openviking_module.shutil, "which", lambda name: None) + monkeypatch.setattr( + openviking_module.subprocess, + "Popen", + MagicMock(side_effect=AssertionError("must not spawn")), + ) + + started, message = openviking_module._start_local_openviking_server("http://127.0.0.1:1934") + + assert started is True + assert "already running" in message + + +def test_start_local_openviking_server_rejects_unparseable_url_before_probing(monkeypatch): + monkeypatch.setattr( + openviking_module, + "_local_openviking_port_is_open", + MagicMock(side_effect=AssertionError("must not probe an unparseable endpoint")), + ) + + started, message = openviking_module._start_local_openviking_server("http://127.0.0.1:not-a-port") + + assert started is False + assert "Could not parse local OpenViking URL" in message + + +def test_local_openviking_port_is_open_detects_listener_and_closed_port(): + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as listener: + listener.bind(("127.0.0.1", 0)) + listener.listen(1) + _host, port = listener.getsockname() + assert openviking_module._local_openviking_port_is_open("127.0.0.1", port) is True + + # Socket closed: the same port no longer accepts connections. + assert openviking_module._local_openviking_port_is_open("127.0.0.1", port) is False + + def test_https_local_endpoint_is_not_runtime_autostart_eligible(monkeypatch): _clear_openviking_env(monkeypatch) monkeypatch.setenv("OPENVIKING_ENDPOINT", "https://localhost:1934")