From 76181217836f9969ecafe707c9fff7b2bbbd08eb Mon Sep 17 00:00:00 2001 From: 0xDevNinja Date: Mon, 22 Jun 2026 17:11:58 +0530 Subject: [PATCH] fix(openviking): refresh client from env on every access MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit initialize() snapshots OPENVIKING_* into the provider once, so /reload (which only updates os.environ) leaves viking_* tools running against stale auth — users have to restart hermes to pick up keys added to ~/.hermes/.env after startup. Add _ensure_client(), which re-resolves the connection settings via the same _resolve_connection_settings/_load_hermes_openviking_config path initialize() uses and rebuilds + health-checks the client only when an OPENVIKING_* value actually changed; otherwise it reuses the cached client so the hot path stays at one dict comparison with no network calls. Every `if not self._client:` guard in system_prompt_block, queue_prefetch, sync_turn, on_session_end, on_memory_write and handle_tool_call now goes through it. Refreshing is gated behind a flag set at the end of initialize() so the baseline is established before any env re-resolution happens — callers that wire up a client directly keep the existing client untouched. Refs #21130 (cherry picked from commit b694d21b7c4ff0330df6051e12dc8991f7ea10a6) --- plugins/memory/openviking/__init__.py | 105 ++++++++++++--- tests/openviking_plugin/test_openviking.py | 148 +++++++++++++++++++++ 2 files changed, 238 insertions(+), 15 deletions(-) diff --git a/plugins/memory/openviking/__init__.py b/plugins/memory/openviking/__init__.py index 48589492e351..b2c44c2bf8c6 100644 --- a/plugins/memory/openviking/__init__.py +++ b/plugins/memory/openviking/__init__.py @@ -1847,6 +1847,11 @@ class OpenVikingMemoryProvider(MemoryProvider): self._run_id = uuid.uuid4().hex self._run_lock_file: Optional[Any] = None self._run_lock_path: Optional[Path] = None + # Set once initialize() has resolved the connection baseline. Until then + # _ensure_client() must not re-resolve from the environment — callers + # that wire up a client directly (e.g. tests) would otherwise have it + # discarded. See _ensure_client() / #21130. + self._env_refresh_enabled = False # Guards the (_session_id, _turn_count) pair. sync_turn runs on the # MemoryManager's background sync executor while on_session_end / # on_session_switch run on the caller's thread, so the snapshot+reset @@ -2244,8 +2249,72 @@ class OpenVikingMemoryProvider(MemoryProvider): global _last_active_provider _last_active_provider = self + # Baseline established — subsequent accesses may refresh from env (#21130). + self._env_refresh_enabled = True + + def _ensure_client(self) -> Optional["_VikingClient"]: + """Return the active client, rebuilding it if the resolved config changed. + + ``/reload`` only refreshes ``os.environ`` — the existing provider + instance is not re-initialized — so OPENVIKING_* values added to + ``~/.hermes/.env`` after startup never reach the live client and tools + keep running against stale auth until the user restarts hermes (#21130). + + Re-resolve the connection settings on each access (same layering as + ``initialize``) and rebuild + health-check only when a value actually + changed; otherwise reuse the cached client so the hot path stays at one + dict comparison with zero network calls. + """ + # Before initialize() runs there is no env baseline to refresh against; + # return whatever client the caller wired up (matches legacy behavior). + if not self._env_refresh_enabled: + return self._client + + settings = _resolve_connection_settings(_load_hermes_openviking_config()) + endpoint = settings["endpoint"] + api_key = settings["api_key"] + account = settings["account"] + user = settings["user"] + agent = settings["agent"] + + config_unchanged = ( + endpoint == getattr(self, "_endpoint", None) + and api_key == getattr(self, "_api_key", None) + and account == getattr(self, "_account", None) + and user == getattr(self, "_user", None) + and agent == getattr(self, "_agent", None) + ) + if config_unchanged and self._client is not None: + return self._client + + self._endpoint = endpoint + self._api_key = api_key + self._account = account + self._user = user + self._agent = agent + + try: + client = _VikingClient( + endpoint, api_key, account=account, user=user, agent=agent, + ) + except ImportError: + logger.warning("httpx not installed — OpenViking plugin disabled") + self._client = None + return None + + health_state, health_message = _classify_runtime_openviking_health(client, endpoint) + if health_state == "healthy": + self._client = client + return self._client + if health_state == "responded": + logger.warning("%s OpenViking memory disabled until config changes.", health_message) + else: # unreachable + logger.warning("OpenViking server at %s is not reachable", endpoint) + self._client = None + return None + def system_prompt_block(self) -> str: - if not self._client: + if not self._ensure_client(): return "" # Provide brief info about the knowledge base try: @@ -2295,7 +2364,7 @@ class OpenVikingMemoryProvider(MemoryProvider): def prefetch(self, query: str, *, session_id: str = "") -> str: """Return recall context for this query/session.""" query_text = _derive_openviking_user_text(query).strip() - if not self._client: + if not self._ensure_client(): return "" effective_session_id = str(session_id or self._session_id or "").strip() @@ -2866,17 +2935,23 @@ class OpenVikingMemoryProvider(MemoryProvider): client: Optional[_VikingClient] = None, ) -> str: query_text = (query or "").strip() - if not self._client or len(query_text) < _RECALL_QUERY_MIN_CHARS: + if len(query_text) < _RECALL_QUERY_MIN_CHARS: + return "" + if client is None: + if self._env_refresh_enabled: + client = self._ensure_client() + elif self._client is not None: + client = _VikingClient( + self._endpoint, + self._api_key, + account=self._account, + user=self._user, + agent=self._agent, + ) + if client is None: return "" try: - client = client or _VikingClient( - self._endpoint, - self._api_key, - account=self._account, - user=self._user, - agent=self._agent, - ) cfg = self._recall_config() candidate_limit = max(cfg["limit"] * 4, 20) deadline = time.monotonic() + cfg["timeout_seconds"] @@ -3729,7 +3804,7 @@ class OpenVikingMemoryProvider(MemoryProvider): messages: Optional[List[Dict[str, Any]]] = None, ) -> None: """Record the conversation turn in OpenViking's session (non-blocking).""" - if not self._client: + if not self._ensure_client(): return user_content = _derive_openviking_user_text(user_content) @@ -3880,7 +3955,7 @@ class OpenVikingMemoryProvider(MemoryProvider): OpenViking automatically extracts 6 categories of memories: profile, preferences, entities, events, cases, and patterns. """ - if not self._client: + if not self._ensure_client(): return # Snapshot sid + turn count atomically against a concurrent sync_turn @@ -3931,7 +4006,7 @@ class OpenVikingMemoryProvider(MemoryProvider): ``_turn_count``. """ new_id = str(new_session_id or "").strip() - if not new_id or not self._client: + if not new_id or not self._ensure_client(): return rewound = bool(kwargs.get("rewound")) @@ -3991,7 +4066,7 @@ class OpenVikingMemoryProvider(MemoryProvider): metadata: Optional[Dict[str, Any]] = None, ) -> None: """Mirror successful built-in memory additions to OpenViking.""" - if not self._client or action != "add" or not content: + if action != "add" or not content or not self._ensure_client(): return subdir = _MEMORY_WRITE_TARGET_SUBDIR_MAP.get(target, _DEFAULT_MEMORY_SUBDIR) @@ -4036,7 +4111,7 @@ class OpenVikingMemoryProvider(MemoryProvider): ] def handle_tool_call(self, tool_name: str, args: dict, **kwargs) -> str: - if not self._client: + if not self._ensure_client(): return tool_error("OpenViking server not connected") try: diff --git a/tests/openviking_plugin/test_openviking.py b/tests/openviking_plugin/test_openviking.py index a2e7de62ea85..074eeb73b49b 100644 --- a/tests/openviking_plugin/test_openviking.py +++ b/tests/openviking_plugin/test_openviking.py @@ -1446,3 +1446,151 @@ class TestOpenVikingMemoryUriBuilder: assert f"/memories/{subdir}/mem_" in uri, ( f"subdir '{subdir}' not placed correctly in URI: {uri}" ) + + +# =================================================================== +# Issue #21130 — OPENVIKING_* not reloaded after /reload +# =================================================================== + + +class TestEnsureClientReloadsEnv: + """Verify /reload picks up new OPENVIKING_* values without a restart (#21130).""" + + def test_ensure_client_rebuilds_when_api_key_changes(self, monkeypatch): + constructions = [] + + class _StubClient: + def __init__(self, endpoint, api_key, account="", user="", agent="hermes"): + constructions.append({"endpoint": endpoint, "api_key": api_key, + "account": account, "user": user, "agent": agent}) + self.endpoint, self.api_key = endpoint, api_key + self.account, self.user, self.agent = account, user, agent + + def health(self): + return True + + monkeypatch.setattr("plugins.memory.openviking._VikingClient", _StubClient) + monkeypatch.setenv("OPENVIKING_ENDPOINT", "http://srv:31933") + monkeypatch.setenv("OPENVIKING_API_KEY", "") + + provider = OpenVikingMemoryProvider() + provider._env_refresh_enabled = True + first = provider._ensure_client() + assert first is not None + assert first.api_key == "" + assert len(constructions) == 1 + + # Same env on second call — must reuse cached client (no rebuild). + assert provider._ensure_client() is first + assert len(constructions) == 1 + + # Simulate /reload: env now carries the new API key. + monkeypatch.setenv("OPENVIKING_API_KEY", "sk-fresh") + rebuilt = provider._ensure_client() + assert rebuilt is not None + assert rebuilt is not first + assert rebuilt.api_key == "sk-fresh" + assert len(constructions) == 2 + + def test_ensure_client_rebuilds_when_endpoint_changes(self, monkeypatch): + builds = [] + + class _StubClient: + def __init__(self, endpoint, api_key, **kw): + builds.append(endpoint) + self.endpoint = endpoint + + def health(self): + return True + + monkeypatch.setattr("plugins.memory.openviking._VikingClient", _StubClient) + monkeypatch.setenv("OPENVIKING_ENDPOINT", "http://a") + monkeypatch.setenv("OPENVIKING_API_KEY", "key") + + provider = OpenVikingMemoryProvider() + provider._env_refresh_enabled = True + provider._ensure_client() + provider._ensure_client() # cached + monkeypatch.setenv("OPENVIKING_ENDPOINT", "http://b") + provider._ensure_client() # rebuilds + assert builds == ["http://a", "http://b"] + + def test_prefetch_rebuilds_client_when_api_key_changes(self, monkeypatch): + posts = [] + + class _StubClient: + def __init__(self, endpoint, api_key, **kw): + self.endpoint = endpoint + self.api_key = api_key + + def health(self): + return True + + def post(self, path, payload=None, **kwargs): + posts.append((self.api_key, path, payload or {})) + return { + "result": { + "memories": [ + { + "uri": "viking://user/default/memories/pref.md", + "abstract": f"memory from {self.api_key or 'anonymous'}", + "score": 0.9, + "level": 2, + } + ], + "resources": [], + } + } + + monkeypatch.setattr("plugins.memory.openviking._VikingClient", _StubClient) + monkeypatch.setenv("OPENVIKING_ENDPOINT", "http://srv:31933") + monkeypatch.setenv("OPENVIKING_API_KEY", "sk-old") + monkeypatch.setenv("OPENVIKING_RECALL_PREFER_ABSTRACT", "true") + + provider = OpenVikingMemoryProvider() + provider._env_refresh_enabled = True + provider._session_id = "session-1" + + first = provider.prefetch("What should OpenViking recall?", session_id="session-1") + monkeypatch.setenv("OPENVIKING_API_KEY", "sk-fresh") + second = provider.prefetch("What should OpenViking recall?", session_id="session-1") + + assert "memory from sk-old" in first + assert "memory from sk-fresh" in second + assert [call[0] for call in posts] == ["sk-old", "sk-fresh"] + assert [call[1] for call in posts] == ["/api/v1/search/search", "/api/v1/search/search"] + assert posts[0][2]["limit"] == posts[1][2]["limit"] + assert "top_k" not in posts[0][2] + + def test_ensure_client_returns_none_when_health_fails(self, monkeypatch): + class _StubClient: + def __init__(self, *a, **kw): + pass + + def health(self): + return False + + monkeypatch.setattr("plugins.memory.openviking._VikingClient", _StubClient) + monkeypatch.setenv("OPENVIKING_ENDPOINT", "http://dead") + monkeypatch.setenv("OPENVIKING_API_KEY", "") + + provider = OpenVikingMemoryProvider() + provider._env_refresh_enabled = True + assert provider._ensure_client() is None + assert provider._client is None + + def test_handle_tool_call_uses_ensure_client(self, monkeypatch): + provider = OpenVikingMemoryProvider() + provider._env_refresh_enabled = True + + class _StubClient: + def __init__(self, *a, **kw): + pass + + def health(self): + return False + + monkeypatch.setattr("plugins.memory.openviking._VikingClient", _StubClient) + + out = provider.handle_tool_call("viking_search", {"query": "x"}) + assert "not connected" in out.lower()