From 7baea4c056038ca337e6f4cdbc2c60d6d641c5d4 Mon Sep 17 00:00:00 2001 From: Jack Lau <72348727+jackulau@users.noreply.github.com> Date: Mon, 17 Aug 2026 14:53:01 -0500 Subject: [PATCH] fix(gateway): resolve the session DB inside the active profile scope Fixes #88532. A multiplexed gateway serves every profile from one process, but SessionStore bound a single SessionDB during __init__: self._db = SessionDB() SessionDB(db_path=None) resolves _default_db_path() at call time and does follow the context-local HERMES_HOME override, so the path machinery was already correct. The problem was when it ran: at construction, on the process's own root home, long before any inbound event enters _profile_runtime_scope. Every profile's rows therefore landed in the root state.db, even though the scope had redirected get_hermes_home() correctly for the turn (that helper's own docstring lists "sessions" among what it scopes). The rows still carry the right profile_name, stamped from source.profile by the same handler, so nothing in the data looks wrong. The only visible symptom is the desktop listing a profile's session under the default bot: _open_session_db_for_profile opens profiles//state.db, which never received the write. Look the handle up through a property instead, resolving the active scope per access and caching one handle per resolved path so a hot inbound path opens SQLite once per profile rather than once per message. Construction stays under the cache lock so a concurrent first message on a profile cannot open and then leak a second handle. Assignment is preserved as an explicit pin, which is what the existing suites rely on when they install a fake handle or disable the DB with store._db = None, and a pin keeps winning across scope changes. Behavior is unchanged when no profile scope is active, so single-profile gateways resolve exactly the path they did before. This does not migrate rows that already landed in the root store; those stay where they are. --- gateway/session.py | 107 +++++++++-- ...test_multiplex_session_db_profile_scope.py | 172 ++++++++++++++++++ 2 files changed, 264 insertions(+), 15 deletions(-) create mode 100644 tests/gateway/test_multiplex_session_db_profile_scope.py diff --git a/gateway/session.py b/gateway/session.py index cd62db5048f77..8b2b10cf08700 100644 --- a/gateway/session.py +++ b/gateway/session.py @@ -1235,6 +1235,13 @@ async def _offloaded(*args, **kwargs) -> Any: return _offloaded +# Sentinel for "no explicit SessionDB has been pinned on this store", so the +# ``_db`` property can distinguish "resolve from the active profile scope" +# from a deliberate ``store._db = None`` (which disables the DB and selects +# the JSONL fallback). A plain ``None`` cannot express both. +_DB_UNPINNED = object() + + class SessionStore: """ Manages session storage and retrieval. @@ -1285,21 +1292,91 @@ def __init__(self, sessions_dir: Path, config: GatewayConfig, getattr(config, "write_sessions_json", True) ) - # Initialize SQLite session database - self._db = None - try: - from hermes_state import SessionDB - self._db = SessionDB() - except RuntimeError as e: - if "live-system guard" in str(e): - # Test-isolation guard fired: a pytest-context process - # resolved the developer's production state.db. Never - # swallow this into the JSONL fallback — the whole point - # is a loud, hard failure. - raise - print(f"[gateway] Warning: SQLite session store unavailable, falling back to JSONL: {e}") - except Exception as e: - print(f"[gateway] Warning: SQLite session store unavailable, falling back to JSONL: {e}") + # Initialize SQLite session database. + # + # Handles are cached per resolved path and looked up through the + # ``_db`` property instead of being bound to one handle here. A + # multiplexed gateway serves every profile from a SINGLE process, so + # a handle bound during __init__ is frozen to the process's own root + # home; every profile's rows then land in the root state.db even + # though ``_profile_runtime_scope`` has already redirected + # ``get_hermes_home()`` for the turn (its docstring lists "sessions" + # among what it scopes). The row still carries the right + # ``profile_name``, so the damage is invisible in the data and shows + # up only as the desktop listing a profile's session under the + # default bot -- ``_open_session_db_for_profile`` reads + # ``profiles//state.db``, which never received the write. + # See #88532. + # + # Priming the handle for the current scope here keeps the startup + # diagnostics exactly where they were: the live-DB isolation guard + # still raises during construction, and the JSONL-fallback warning + # is still printed once at startup rather than on first use. + self._db_pinned = _DB_UNPINNED + self._db_handles: Dict[Path, Any] = {} + self._db_handles_lock = threading.Lock() + self._open_session_db_for_active_scope() + + def _open_session_db_for_active_scope(self): + """Return the SessionDB for the profile scope active on this task. + + ``SessionDB(db_path=None)`` resolves ``_default_db_path()`` at call + time, and that helper follows the context-local HERMES_HOME override + installed by ``_profile_runtime_scope``. Resolving here rather than + once in ``__init__`` is the whole fix for #88532: it lets the + scoping that the multiplexed inbound path already performs actually + reach session storage. + + Handles are cached per resolved path, so a hot inbound path opens + SQLite once per profile rather than once per message, and two + profiles never share a handle. Construction is done under the lock + so a concurrent first message on the same profile cannot open (and + then leak) a second handle for the same path. + + A construction failure is cached as ``None`` for that path, matching + the previous behavior where a failed startup left ``_db`` None for + the life of the store and callers fell back to JSONL. + """ + from hermes_state import SessionDB, _default_db_path + + path = Path(_default_db_path()) + with self._db_handles_lock: + if path in self._db_handles: + return self._db_handles[path] + db = None + try: + db = SessionDB() + except RuntimeError as e: + if "live-system guard" in str(e): + # Test-isolation guard fired: a pytest-context process + # resolved the developer's production state.db. Never + # swallow this into the JSONL fallback — the whole point + # is a loud, hard failure. Deliberately not cached: the + # guard must fire again on the next attempt. + raise + print(f"[gateway] Warning: SQLite session store unavailable, falling back to JSONL: {e}") + except Exception as e: + print(f"[gateway] Warning: SQLite session store unavailable, falling back to JSONL: {e}") + self._db_handles[path] = db + return db + + @property + def _db(self): + """The SessionDB for the active profile scope, or a pinned override. + + Assigning ``store._db`` pins that value for every subsequent read, + which is what tests rely on to install a fake or to disable the DB + with ``store._db = None``. Unpinned (the production path), each read + resolves the scope so a multiplexed profile's writes reach its own + store. + """ + if self._db_pinned is not _DB_UNPINNED: + return self._db_pinned + return self._open_session_db_for_active_scope() + + @_db.setter + def _db(self, value) -> None: + self._db_pinned = value def _has_active_processes_safe(self, session_key: str, *, context: str) -> bool: """Return whether a session has active work, failing closed on registry errors.""" diff --git a/tests/gateway/test_multiplex_session_db_profile_scope.py b/tests/gateway/test_multiplex_session_db_profile_scope.py new file mode 100644 index 0000000000000..84ce3b396c826 --- /dev/null +++ b/tests/gateway/test_multiplex_session_db_profile_scope.py @@ -0,0 +1,172 @@ +"""Regression coverage for #88532. + +A multiplexed gateway serves every profile from one process. ``SessionStore`` +used to bind a single ``SessionDB`` during ``__init__``, freezing it to the +process's own root home, so a named profile's sessions were physically written +to the root ``state.db`` even though ``_profile_runtime_scope`` had already +redirected ``get_hermes_home()`` for that turn. The rows carried the correct +``profile_name``, which is why the only visible symptom was the desktop listing +a profile's session under the default bot: the desktop reads +``profiles//state.db``, which never received the write. + +These tests pin the handle to the *active* scope rather than to construction +time. ``test_write_under_profile_scope_lands_in_profile_store`` is the one +that reproduces the report; it fails against the pre-fix code with the session +row sitting in the root store. +""" + +import sqlite3 +from pathlib import Path +from unittest.mock import patch + +import pytest + +from gateway.config import GatewayConfig +from gateway.session import SessionStore +from hermes_constants import reset_hermes_home_override, set_hermes_home_override + + +@pytest.fixture +def multiplex_homes(tmp_path, monkeypatch): + """A root home plus a named profile home, with HERMES_HOME on the root. + + Mirrors the reported layout: one gateway process launched under the root + home, serving a ``fitness`` profile whose store lives under + ``profiles/fitness``. + """ + import hermes_state + + root = tmp_path / "hermes" + profile = root / "profiles" / "fitness" + root.mkdir(parents=True) + profile.mkdir(parents=True) + monkeypatch.setenv("HERMES_HOME", str(root)) + + # The suite-wide fixture in conftest re-points ``hermes_state.DEFAULT_DB_PATH`` + # at a fake home, which trips the deliberate escape hatch in + # ``_default_db_path()``: a re-pointed constant wins over everything, + # including the context-local override. That is correct for tests that + # want one fixed DB, but it would pin every lookup here to a single path + # and make these assertions vacuous. Restore the import-time snapshot so + # the hatch is closed and resolution goes through ``get_hermes_home()``, + # which is what production does. ``HERMES_HOME`` above still keeps that + # resolution inside ``tmp_path``, so no real store is ever opened. + monkeypatch.setattr( + hermes_state, "DEFAULT_DB_PATH", hermes_state._IMPORT_DEFAULT_DB_PATH + ) + return root, profile + + +def _make_store(root: Path) -> SessionStore: + with patch("gateway.session.SessionStore._ensure_loaded"): + store = SessionStore(sessions_dir=root / "sessions", config=GatewayConfig()) + store._loaded = True + return store + + +def _session_ids(db_path: Path) -> set: + """Read session ids straight out of a state.db, or empty if absent.""" + if not db_path.exists(): + return set() + conn = sqlite3.connect(str(db_path)) + try: + rows = conn.execute("SELECT id FROM sessions").fetchall() + except sqlite3.OperationalError: + # No sessions table: nothing was ever written here. + return set() + finally: + conn.close() + return {r[0] for r in rows} + + +def test_store_uses_root_db_when_no_profile_scope_is_active(multiplex_homes): + """Single-profile gateways are unaffected: no scope, same path as before.""" + root, _profile = multiplex_homes + store = _make_store(root) + + assert Path(store._db.db_path) == root / "state.db" + + +def test_db_handle_follows_the_active_profile_scope(multiplex_homes): + """The handle is resolved per access, not frozen at construction.""" + root, profile = multiplex_homes + store = _make_store(root) + + # Constructed outside any scope, exactly as the gateway constructs it. + assert Path(store._db.db_path) == root / "state.db" + + token = set_hermes_home_override(str(profile)) + try: + assert Path(store._db.db_path) == profile / "state.db" + finally: + reset_hermes_home_override(token) + + # And the scope is restored once the turn's scope exits. + assert Path(store._db.db_path) == root / "state.db" + + +def test_write_under_profile_scope_lands_in_profile_store(multiplex_homes): + """The reported bug: the row must be in the profile's own file. + + This is the assertion the issue makes by hand with ``sqlite3``: the + session for profile ``fitness`` belongs in ``profiles/fitness/state.db`` + and must NOT be in the root store. + """ + root, profile = multiplex_homes + store = _make_store(root) + + token = set_hermes_home_override(str(profile)) + try: + store._db.create_session("20260817_233028_542fda58", "feishu") + finally: + reset_hermes_home_override(token) + + assert _session_ids(profile / "state.db") == {"20260817_233028_542fda58"} + assert _session_ids(root / "state.db") == set() + + +def test_handles_are_cached_per_path(multiplex_homes): + """One handle per profile: no reopen per message, no sharing across profiles.""" + root, profile = multiplex_homes + store = _make_store(root) + + root_first = store._db + root_second = store._db + assert root_first is root_second + + token = set_hermes_home_override(str(profile)) + try: + profile_first = store._db + profile_second = store._db + finally: + reset_hermes_home_override(token) + + assert profile_first is profile_second + assert profile_first is not root_first + + +def test_explicitly_pinned_handle_still_wins(multiplex_homes): + """``store._db = ...`` remains authoritative for every subsequent read. + + Guardrail rather than a bug reproduction: a large number of existing + tests install a fake handle or disable the DB this way, and the property + must not quietly resolve past a deliberate assignment. + """ + root, profile = multiplex_homes + store = _make_store(root) + + sentinel = object() + store._db = sentinel + token = set_hermes_home_override(str(profile)) + try: + assert store._db is sentinel + finally: + reset_hermes_home_override(token) + + # Disabling the DB (the JSONL-fallback path) must survive scope changes. + store._db = None + token = set_hermes_home_override(str(profile)) + try: + assert store._db is None + finally: + reset_hermes_home_override(token)