mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-24 16:54:43 +00:00
fix(openviking): refresh client from env on every access
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 b694d21b7c)
This commit is contained in:
parent
9291b786b4
commit
7618121783
2 changed files with 238 additions and 15 deletions
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue