fix(openviking): drop stale "disabled for this Hermes run" warnings
The provider used to disable OpenViking permanently when the server was unreachable. That was fixed: `_ensure_client()` now reconnects lazily, with a 30s cooldown gate in `_ensure_client_locked`. Only one of the seven user-facing warnings was updated to match. The other six still told the user memory was "disabled for this Hermes run", which is no longer true — every one of those paths is retried on the next access. A user who reads the old message has no reason to retry, which is very likely how #5721 ("never recovers") came to be filed against behaviour that already recovers. All six sites were traced to confirm none is terminal for the run: the `initialize()`-time and waiter-thread failures never arm `_failed_refresh` (only line 2439 does), so they retry on the very next access with no cooldown at all. The replacement wording deliberately omits the "(after cooldown)" parenthetical used at the already-correct site — that detail is only accurate where `_failed_refresh` was just armed. The neutral phrasing is true at all six. Also promotes two clause separators to periods to avoid "…; …disabled;" collisions.
This commit is contained in:
parent
339d968689
commit
8346403a4b
|
|
@ -1332,7 +1332,8 @@ def _runtime_openviking_timeout_message(endpoint: str) -> str:
|
|||
f"Local OpenViking server at {endpoint} is not reachable. "
|
||||
"Tried to start openviking-server, but it did not become reachable "
|
||||
f"within {_LOCAL_OPENVIKING_AUTOSTART_TIMEOUT:.0f} seconds. "
|
||||
"OpenViking memory disabled for this Hermes run."
|
||||
"OpenViking memory disabled; will retry on a later access or when "
|
||||
"the config changes."
|
||||
)
|
||||
|
||||
|
||||
|
|
@ -2187,8 +2188,9 @@ class OpenVikingMemoryProvider(MemoryProvider):
|
|||
return
|
||||
if not healthy:
|
||||
warning_message = (
|
||||
f"OpenViking server at {endpoint} is still not reachable after auto-start; "
|
||||
"OpenViking memory disabled for this Hermes run."
|
||||
f"OpenViking server at {endpoint} is still not reachable after auto-start. "
|
||||
"OpenViking memory disabled; will retry on a later access or when "
|
||||
"the config changes."
|
||||
)
|
||||
else:
|
||||
self._client = client
|
||||
|
|
@ -2206,7 +2208,8 @@ class OpenVikingMemoryProvider(MemoryProvider):
|
|||
except Exception as e:
|
||||
warning_message = (
|
||||
f"OpenViking server at {endpoint} could not be attached after auto-start: {e}. "
|
||||
"OpenViking memory disabled for this Hermes run."
|
||||
"OpenViking memory disabled; will retry on a later access or when "
|
||||
"the config changes."
|
||||
)
|
||||
|
||||
if warning_message:
|
||||
|
|
@ -2230,8 +2233,9 @@ class OpenVikingMemoryProvider(MemoryProvider):
|
|||
endpoint = self._endpoint
|
||||
if not _is_local_openviking_url(endpoint):
|
||||
_emit_runtime_warning(
|
||||
f"Remote OpenViking server at {endpoint} is not reachable; "
|
||||
"OpenViking memory disabled for this Hermes run. "
|
||||
f"Remote OpenViking server at {endpoint} is not reachable. "
|
||||
"OpenViking memory disabled; will retry on a later access or when "
|
||||
"the config changes. "
|
||||
"Check the configured endpoint and network connectivity.",
|
||||
warning_callback,
|
||||
)
|
||||
|
|
@ -2256,7 +2260,8 @@ class OpenVikingMemoryProvider(MemoryProvider):
|
|||
self._runtime_start_pending = False
|
||||
warning_message = (
|
||||
f"Local OpenViking server at {endpoint} is not reachable. {start_message} "
|
||||
"OpenViking memory disabled for this Hermes run."
|
||||
"OpenViking memory disabled; will retry on a later access or when "
|
||||
"the config changes."
|
||||
)
|
||||
self._client = None
|
||||
else:
|
||||
|
|
@ -2333,7 +2338,8 @@ class OpenVikingMemoryProvider(MemoryProvider):
|
|||
)
|
||||
elif health_state != "healthy":
|
||||
_emit_runtime_warning(
|
||||
f"{health_message} OpenViking memory disabled for this Hermes run.",
|
||||
f"{health_message} OpenViking memory disabled; will retry on a "
|
||||
"later access or when the config changes.",
|
||||
warning_callback,
|
||||
)
|
||||
self._client = None
|
||||
|
|
|
|||
|
|
@ -2002,3 +2002,169 @@ class TestEnsureClientFailureHardening:
|
|||
assert provider._conn_snapshot == healthy_snapshot
|
||||
built = provider._new_client()
|
||||
assert built.endpoint == "https://up.example"
|
||||
|
||||
|
||||
class TestUnavailableWarningsPromiseRetry:
|
||||
"""Every "OpenViking is unavailable" warning must describe what actually
|
||||
happens next.
|
||||
|
||||
``_ensure_client()`` rebuilds and re-probes the client whenever the
|
||||
resolved config changes or the failed-config cooldown has elapsed, so no
|
||||
warning may tell the user memory is off for the rest of the run — that
|
||||
reads as "it never recovers" and sends people restarting hermes for
|
||||
nothing (#5721).
|
||||
"""
|
||||
|
||||
@staticmethod
|
||||
def _assert_promises_retry(message: str) -> None:
|
||||
assert "for this Hermes run" not in message, message
|
||||
assert "will retry on a later access" in message, message
|
||||
assert "when the config changes" in message, message
|
||||
|
||||
@staticmethod
|
||||
def _stub_client(health_result):
|
||||
class _StubClient:
|
||||
def __init__(self, endpoint, api_key="", account="", user="", agent=""):
|
||||
self.endpoint = endpoint
|
||||
|
||||
def health(self):
|
||||
return health_result
|
||||
|
||||
return _StubClient
|
||||
|
||||
def test_local_autostart_timeout_warning(self):
|
||||
self._assert_promises_retry(
|
||||
openviking_plugin._runtime_openviking_timeout_message("http://127.0.0.1:1934")
|
||||
)
|
||||
|
||||
def test_remote_unreachable_warning(self):
|
||||
provider = OpenVikingMemoryProvider()
|
||||
provider._endpoint = "https://remote.example"
|
||||
warnings: list[str] = []
|
||||
|
||||
provider._handle_runtime_openviking_unreachable(warning_callback=warnings.append)
|
||||
|
||||
assert provider._client is None
|
||||
assert len(warnings) == 1
|
||||
self._assert_promises_retry(warnings[0])
|
||||
|
||||
def test_local_autostart_refused_warning(self, monkeypatch):
|
||||
monkeypatch.setattr(
|
||||
openviking_plugin,
|
||||
"_start_local_openviking_server",
|
||||
lambda endpoint: (False, "openviking-server was not found on PATH."),
|
||||
)
|
||||
provider = OpenVikingMemoryProvider()
|
||||
provider._endpoint = "http://127.0.0.1:1934"
|
||||
warnings: list[str] = []
|
||||
|
||||
provider._handle_runtime_openviking_unreachable(warning_callback=warnings.append)
|
||||
|
||||
assert provider._client is None
|
||||
assert len(warnings) == 1
|
||||
self._assert_promises_retry(warnings[0])
|
||||
|
||||
def test_still_unhealthy_after_autostart_warning(self, monkeypatch):
|
||||
monkeypatch.setattr(openviking_plugin, "_VikingClient", self._stub_client(False))
|
||||
monkeypatch.setattr(
|
||||
openviking_plugin, "_wait_for_openviking_health", lambda endpoint, **kwargs: True
|
||||
)
|
||||
provider = OpenVikingMemoryProvider()
|
||||
provider._endpoint = "http://127.0.0.1:1934"
|
||||
warnings: list[str] = []
|
||||
|
||||
provider._finish_runtime_openviking_start(warning_callback=warnings.append)
|
||||
|
||||
assert provider._client is None
|
||||
assert len(warnings) == 1
|
||||
self._assert_promises_retry(warnings[0])
|
||||
|
||||
def test_attach_failure_after_autostart_warning(self, monkeypatch):
|
||||
def _explode(*args, **kwargs):
|
||||
raise RuntimeError("connection reset by peer")
|
||||
|
||||
monkeypatch.setattr(openviking_plugin, "_VikingClient", _explode)
|
||||
monkeypatch.setattr(
|
||||
openviking_plugin, "_wait_for_openviking_health", lambda endpoint, **kwargs: True
|
||||
)
|
||||
provider = OpenVikingMemoryProvider()
|
||||
provider._endpoint = "http://127.0.0.1:1934"
|
||||
warnings: list[str] = []
|
||||
|
||||
provider._finish_runtime_openviking_start(warning_callback=warnings.append)
|
||||
|
||||
assert provider._client is None
|
||||
assert len(warnings) == 1
|
||||
self._assert_promises_retry(warnings[0])
|
||||
|
||||
def test_initialize_responded_unhealthy_warning(self, monkeypatch, tmp_path):
|
||||
class _UnhealthyClient:
|
||||
def __init__(self, endpoint, api_key="", account="", user="", agent=""):
|
||||
self.endpoint = endpoint
|
||||
|
||||
def health_payload(self):
|
||||
return {"healthy": False}
|
||||
|
||||
def health(self):
|
||||
return False
|
||||
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path / ".hermes"))
|
||||
monkeypatch.setenv("OPENVIKING_ENDPOINT", "https://sick.example")
|
||||
monkeypatch.setattr(openviking_plugin, "_VikingClient", _UnhealthyClient)
|
||||
provider = OpenVikingMemoryProvider()
|
||||
warnings: list[str] = []
|
||||
|
||||
provider.initialize("session-1", platform="cli", warning_callback=warnings.append)
|
||||
|
||||
assert provider._client is None
|
||||
assert len(warnings) == 1
|
||||
self._assert_promises_retry(warnings[0])
|
||||
|
||||
def test_ensure_client_responded_unhealthy_warning(self, monkeypatch, caplog):
|
||||
class _UnhealthyClient:
|
||||
def __init__(self, endpoint, api_key="", account="", user="", agent=""):
|
||||
self.endpoint = endpoint
|
||||
|
||||
def health_payload(self):
|
||||
return {"healthy": False}
|
||||
|
||||
monkeypatch.setenv("OPENVIKING_ENDPOINT", "https://sick.example")
|
||||
monkeypatch.setattr(openviking_plugin, "_VikingClient", _UnhealthyClient)
|
||||
provider = OpenVikingMemoryProvider()
|
||||
provider._env_refresh_enabled = True
|
||||
|
||||
with caplog.at_level("WARNING", logger=openviking_plugin.__name__):
|
||||
assert provider._ensure_client() is None
|
||||
|
||||
self._assert_promises_retry(caplog.text)
|
||||
|
||||
def test_startup_failure_really_does_reconnect_on_a_later_access(
|
||||
self, monkeypatch, tmp_path
|
||||
):
|
||||
"""The warnings promise a retry — prove the provider delivers one."""
|
||||
probes: list[str] = []
|
||||
|
||||
class _FlakyClient:
|
||||
def __init__(self, endpoint, api_key="", account="", user="", agent=""):
|
||||
self.endpoint = endpoint
|
||||
|
||||
def health(self):
|
||||
probes.append(self.endpoint)
|
||||
return len(probes) > 1 # down at startup, up on the next access
|
||||
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path / ".hermes"))
|
||||
monkeypatch.setenv("OPENVIKING_ENDPOINT", "https://remote.example")
|
||||
monkeypatch.setattr(openviking_plugin, "_VikingClient", _FlakyClient)
|
||||
provider = OpenVikingMemoryProvider()
|
||||
warnings: list[str] = []
|
||||
|
||||
provider.initialize("session-1", platform="cli", warning_callback=warnings.append)
|
||||
assert provider._client is None
|
||||
assert len(warnings) == 1
|
||||
self._assert_promises_retry(warnings[0])
|
||||
|
||||
# A startup failure arms no cooldown, so the very next access re-probes.
|
||||
client = provider._ensure_client()
|
||||
assert client is not None
|
||||
assert client.endpoint == "https://remote.example"
|
||||
assert len(probes) == 2
|
||||
|
|
|
|||
|
|
@ -779,8 +779,9 @@ def test_https_local_endpoint_is_not_runtime_autostart_eligible(monkeypatch):
|
|||
|
||||
assert provider._client is None
|
||||
assert warnings == [
|
||||
"Remote OpenViking server at https://localhost:1934 is not reachable; "
|
||||
"OpenViking memory disabled for this Hermes run. "
|
||||
"Remote OpenViking server at https://localhost:1934 is not reachable. "
|
||||
"OpenViking memory disabled; will retry on a later access or when "
|
||||
"the config changes. "
|
||||
"Check the configured endpoint and network connectivity."
|
||||
]
|
||||
|
||||
|
|
@ -813,7 +814,7 @@ def test_runtime_does_not_autostart_when_local_server_reports_unhealthy(monkeypa
|
|||
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."
|
||||
"OpenViking memory disabled; will retry on a later access or when the config changes."
|
||||
]
|
||||
|
||||
|
||||
|
|
@ -1104,7 +1105,8 @@ def test_runtime_openviking_waiter_warns_when_background_start_times_out(monkeyp
|
|||
assert warnings == [
|
||||
"Local OpenViking server at http://127.0.0.1:1934 is not reachable. "
|
||||
"Tried to start openviking-server, but it did not become reachable "
|
||||
"within 60 seconds. OpenViking memory disabled for this Hermes run."
|
||||
"within 60 seconds. OpenViking memory disabled; will retry on a later access "
|
||||
"or when the config changes."
|
||||
]
|
||||
|
||||
|
||||
|
|
@ -1197,7 +1199,7 @@ def test_initialize_emits_cli_warning_when_local_runtime_autostart_fails(monkeyp
|
|||
assert warnings == [
|
||||
"Local OpenViking server at http://localhost:1934 is not reachable. "
|
||||
"openviking-server was not found on PATH. "
|
||||
"OpenViking memory disabled for this Hermes run."
|
||||
"OpenViking memory disabled; will retry on a later access or when the config changes."
|
||||
]
|
||||
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue