fix(openviking): don't spawn a second server onto a live port
`_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) <noreply@anthropic.com>
This commit is contained in:
parent
8defb9fd60
commit
b49427d85f
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
|
|
|
|||
Loading…
Reference in New Issue