diff --git a/integrations/hermes/__init__.py b/integrations/hermes/__init__.py index 789765149..0d8096138 100644 --- a/integrations/hermes/__init__.py +++ b/integrations/hermes/__init__.py @@ -817,6 +817,17 @@ def is_available(self) -> bool: # ---- Lifecycle ---- def initialize(self, session_id: str, **kwargs: Any) -> None: + # Hermes may initialize memory providers repeatedly in long-lived + # gateway/dashboard processes. Each initialization starts a bm MCP + # stdio child; if we overwrite _actor without shutting it down, those + # children accumulate and retain hundreds of MB each. Make re-init + # idempotent from a process-lifecycle perspective. + if self._actor is not None or self._initialized: + try: + self.shutdown() + except Exception as e: + logger.debug("basic-memory: shutdown before reinitialize failed: %s", e) + self._session_id = session_id or "" self._hermes_home = kwargs.get("hermes_home") or os.path.expanduser("~/.hermes") cfg = _load_config(self._hermes_home) @@ -955,6 +966,15 @@ def shutdown(self) -> None: logger.debug("actor shutdown: %s", e) self._actor = None self._initialized = False + # Clear session-scoped state so the next initialize() starts fresh. + # Without this, reusing a provider singleton across sessions causes + # the first turn of the next session to append to the prior session's + # transcript (_session_note_id) and lose the opening user message + # (_first_user_msg), matching the Codex review on PR #1046. + self._session_note_id = None + self._first_user_msg = None + self._session_started_at = None + self._session_id = "" # ---- Tool surface ---- def system_prompt_block(self) -> str: @@ -1806,6 +1826,8 @@ def _register_via_plugin_manager( # --------------------------------------------------------------------------- _active_providers: list[BasicMemoryProvider] = [] +_provider_singleton: BasicMemoryProvider | None = None +_provider_singleton_lock = threading.Lock() def _atexit_cleanup() -> None: @@ -1825,9 +1847,22 @@ def _atexit_cleanup() -> None: def register(ctx: Any) -> None: - """Register basic-memory as a memory provider plugin.""" - provider = BasicMemoryProvider() - _active_providers.append(provider) + """Register basic-memory as a memory provider plugin. + + Hermes can call plugin register() more than once inside the same + long-lived process (gateway/dashboard command discovery, session startup, + cron runs). A fresh provider owns a fresh bm MCP subprocess, so creating + a new provider on every register leaks bm mcp children until process exit. + Reuse one provider per process and let initialize() refresh its session + state. + """ + global _provider_singleton + with _provider_singleton_lock: + if _provider_singleton is None: + _provider_singleton = BasicMemoryProvider() + _active_providers.append(_provider_singleton) + provider = _provider_singleton + ctx.register_memory_provider(provider) # Bundle the user-facing skill so `hermes plugins install` wires it up diff --git a/integrations/hermes/tests/conftest.py b/integrations/hermes/tests/conftest.py index d62ec45e2..26b88dfa5 100644 --- a/integrations/hermes/tests/conftest.py +++ b/integrations/hermes/tests/conftest.py @@ -58,6 +58,20 @@ def bm(): return _plugin +@pytest.fixture(autouse=True) +def reset_plugin_lifecycle_state(): + """Keep module-level provider singletons isolated between tests.""" + if hasattr(_plugin, "_active_providers"): + _plugin._active_providers.clear() + if hasattr(_plugin, "_provider_singleton"): + setattr(_plugin, "_provider_singleton", None) + yield + if hasattr(_plugin, "_active_providers"): + _plugin._active_providers.clear() + if hasattr(_plugin, "_provider_singleton"): + setattr(_plugin, "_provider_singleton", None) + + # ---- Synthetic MCP CallToolResult objects (no MCP SDK needed at test time) ---- diff --git a/integrations/hermes/tests/test_provider.py b/integrations/hermes/tests/test_provider.py index 39d78a282..fd570e47e 100644 --- a/integrations/hermes/tests/test_provider.py +++ b/integrations/hermes/tests/test_provider.py @@ -3,7 +3,6 @@ from __future__ import annotations import json -from pathlib import Path from unittest.mock import MagicMock @@ -107,7 +106,7 @@ def test_handle_tool_call_dispatches(bm): actor.call.return_value = json.dumps({"results": []}) p._actor = actor - out = p.handle_tool_call("bm_search", {"query": "hi", "limit": 3}) + p.handle_tool_call("bm_search", {"query": "hi", "limit": 3}) actor.call.assert_called_once() bm_tool, bm_args = actor.call.call_args[0][:2] assert bm_tool == "search_notes" @@ -269,13 +268,108 @@ def test_get_config_schema_shape(bm): }.issubset(keys) -def test_register_appends_to_active_providers(bm): - fake_ctx = MagicMock() - bm._active_providers.clear() - bm.register(fake_ctx) - fake_ctx.register_memory_provider.assert_called_once() - assert len(bm._active_providers) == 1 - bm._active_providers.clear() +def test_register_reuses_single_provider_per_process(bm): + """Repeated plugin registration must not create providers that each own a bm mcp child.""" + first_ctx = MagicMock() + second_ctx = MagicMock() + + bm.register(first_ctx) + bm.register(second_ctx) + + first_provider = first_ctx.register_memory_provider.call_args.args[0] + second_provider = second_ctx.register_memory_provider.call_args.args[0] + assert first_provider is second_provider + assert bm._provider_singleton is first_provider + assert bm._active_providers == [first_provider] + + +def test_initialize_shuts_down_existing_actor_before_reinitializing(bm, monkeypatch, tmp_path): + """Repeated initialize() calls must not orphan the previous bm mcp actor.""" + actors = [] + + class _FakeActor: + def __init__(self, argv): + self.argv = argv + self.shutdown_calls = 0 + actors.append(self) + + def start(self, timeout=25.0): + return None + + def shutdown(self, timeout=5.0): + self.shutdown_calls += 1 + + def list_tools(self): + return [{"name": name} for name in bm._HERMES_TO_BM.values()] + + monkeypatch.setattr(bm, "_MCP_AVAILABLE", True) + monkeypatch.setattr(bm, "_bm_binary_path", lambda: "/fake/bm") + monkeypatch.setattr(bm, "_BmMcpActor", _FakeActor) + monkeypatch.setattr(bm.BasicMemoryProvider, "_ensure_local_project", lambda self: None) + monkeypatch.setattr(bm.BasicMemoryProvider, "_verify_project_registered", lambda self: True) + monkeypatch.setattr(bm.BasicMemoryProvider, "_server_argv", lambda self: ["/fake/bm", "mcp"]) + + p = bm.BasicMemoryProvider() + p.initialize(session_id="first", hermes_home=str(tmp_path)) + first_actor = p._actor + p.initialize(session_id="second", hermes_home=str(tmp_path)) + + assert len(actors) == 2 + assert first_actor is actors[0] + assert actors[0].shutdown_calls == 1 + assert p._actor is actors[1] + assert p._initialized is True + + +def test_shutdown_clears_session_scoped_state(bm, monkeypatch, tmp_path): + """shutdown() must clear _session_note_id, _first_user_msg, and + _session_id so the next initialize() does not append turns into the + previous session's transcript. + + Regression test for the Codex review finding on PR #1046: + without this, reusing a provider singleton causes the next session + to write into the prior session note and lose its opening message. + """ + + class _FakeActor: + def __init__(self, argv): + self.argv = argv + + def start(self, timeout=25.0): + return None + + def shutdown(self, timeout=5.0): + pass + + def list_tools(self): + return [{"name": name} for name in bm._HERMES_TO_BM.values()] + + monkeypatch.setattr(bm, "_MCP_AVAILABLE", True) + monkeypatch.setattr(bm, "_bm_binary_path", lambda: "/fake/bm") + monkeypatch.setattr(bm, "_BmMcpActor", _FakeActor) + monkeypatch.setattr(bm.BasicMemoryProvider, "_ensure_local_project", lambda self: None) + monkeypatch.setattr(bm.BasicMemoryProvider, "_verify_project_registered", lambda self: True) + monkeypatch.setattr(bm.BasicMemoryProvider, "_server_argv", lambda self: ["/fake/bm", "mcp"]) + + p = bm.BasicMemoryProvider() + p.initialize(session_id="first", hermes_home=str(tmp_path)) + + # Set session-scoped state (as would happen during a real session) + p._session_note_id = "test/hermes-sessions/session-old" + p._first_user_msg = "old first message" + p._session_started_at = None # doesn't matter — shutdown clears the important fields + p._session_id = "first" + + p.shutdown() + + # After shutdown, session-scoped state must be cleared + assert p._session_note_id is None, ( + "_session_note_id not cleared — next session would append, not create" + ) + assert p._first_user_msg is None, ( + "_first_user_msg not cleared — next session would lose its opening message" + ) + assert p._session_id == "", "_session_id not cleared" def test_register_also_registers_bundled_skill(bm):