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. (cherry picked from commit 8346403a4b97af503d26b0f7905ff513828d821e)
2026-07-27 01:12:37 +08:00
parent a3f6953f1a
commit 9014aa0263
3 changed files with 184 additions and 12 deletions
@@ -1385,7 +1385,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."
    )


@@ -2240,8 +2241,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
@@ -2259,7 +2261,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:
@@ -2283,8 +2286,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,
            )
@@ -2309,7 +2313,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:
@@ -2386,7 +2391,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
@@ -947,3 +947,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
@@ -369,8 +369,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."
    ]

@@ -403,7 +404,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."
    ]


@@ -1057,4 +1058,3 @@ def test_prefetch_sends_contract_safe_memory_context_payload(monkeypatch):
    assert "mode" not in payload
    assert "target_uri" not in payload

-