fix(openviking): don't spawn a second server onto a live port (#74846)

plugins/memory/openviking/__init__.py

@@ -33,6 +33,7 @@
 import os
 import re
 import shutil
+import socket
 import stat
 import subprocess
 import tempfile
@@ -126,6 +127,10 @@
 }
 _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)

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 fake_popen(args, **kwargs):
         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 fake_popen(args, **kwargs):
     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")