fix(gateway): resolve the session DB inside the active profile scope - #88632
Closed
jackulau wants to merge 1 commit into
Closed
fix(gateway): resolve the session DB inside the active profile scope#88632jackulau wants to merge 1 commit into
jackulau wants to merge 1 commit into
Conversation
Fixes NousResearch#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/<name>/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.
teknium1
added a commit
that referenced
this pull request
Aug 17, 2026
…nd release cached handles Follow-up to the salvaged #88632 (Jack Lau) fix for #88532, extending the same repair to the sibling frozen-at-init handle and closing the handle lifecycle gap the per-path cache introduces. 1. GatewayRunner._session_db had the identical bug class: bound once as AsyncSessionDB(SessionDB()) in __init__ on the root home, while /resume, /title, /history and session search all execute inside _profile_runtime_scope on a multiplexed gateway. Convert it to the same property-with-pin pattern: per-access resolution of _default_db_path(), one cached AsyncSessionDB per resolved path under a lock, and assignment preserved as an explicit pin (many suites install fakes or None). Construction-time priming keeps the #88235 init-failure broadcast at startup. 2. Handle lifecycle: the per-path caches accumulate one open SessionDB per profile served, but the shutdown path closed only store._db / runner._session_db - which now resolve just the shutdown task's own (root) scope. Secondary profiles' handles would strand their WAL write locks until process exit, recreating the abandoned-handle leak b454e4d fixed and breaking --replace restarts with 'database is locked'. Add close_all_db_handles() / close_all_session_db_handles() sweeps and call both from the gateway teardown path. SessionDB.close() is idempotent, so the root handle being closed by both the legacy loop and the sweep is safe. Tests: sweep coverage plus a runner-property scope/pin/cache test in tests/gateway/test_multiplex_session_db_profile_scope.py.
Contributor
|
Merged in #88734 with your commit cherry-picked and authorship preserved — excellent diagnosis (the frozen |
10 tasks
lisajlau
pushed a commit
to lisajlau/hermes-agent
that referenced
this pull request
Aug 20, 2026
…nd release cached handles Follow-up to the salvaged NousResearch#88632 (Jack Lau) fix for NousResearch#88532, extending the same repair to the sibling frozen-at-init handle and closing the handle lifecycle gap the per-path cache introduces. 1. GatewayRunner._session_db had the identical bug class: bound once as AsyncSessionDB(SessionDB()) in __init__ on the root home, while /resume, /title, /history and session search all execute inside _profile_runtime_scope on a multiplexed gateway. Convert it to the same property-with-pin pattern: per-access resolution of _default_db_path(), one cached AsyncSessionDB per resolved path under a lock, and assignment preserved as an explicit pin (many suites install fakes or None). Construction-time priming keeps the NousResearch#88235 init-failure broadcast at startup. 2. Handle lifecycle: the per-path caches accumulate one open SessionDB per profile served, but the shutdown path closed only store._db / runner._session_db - which now resolve just the shutdown task's own (root) scope. Secondary profiles' handles would strand their WAL write locks until process exit, recreating the abandoned-handle leak b454e4d fixed and breaking --replace restarts with 'database is locked'. Add close_all_db_handles() / close_all_session_db_handles() sweeps and call both from the gateway teardown path. SessionDB.close() is idempotent, so the root handle being closed by both the legacy loop and the sweep is safe. Tests: sweep coverage plus a runner-property scope/pin/cache test in tests/gateway/test_multiplex_session_db_profile_scope.py.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
A multiplexed gateway serves every profile from one process, but
SessionStorebound a singleSessionDBduring__init__:SessionDB(db_path=None)falls back to_default_db_path(), which resolvesget_hermes_home()at call time and correctly follows the context-local override installed byset_hermes_home_override. So the path machinery was already right. The bug is when it ran: at construction, on the process's own root home, long before any inbound event enters a profile scope.The per-profile scoping on the inbound path is also already correct:
and
_profile_runtime_scopedocuments itself as redirectingget_hermes_home()for "config, skills, memory, SOUL, sessions". Sessions were meant to follow the profile. The frozen handle is what prevented it: nothing re-resolved the DB at that point, so the write went through the handle pinned to the root home at startup.Because
profile_nameis stamped fromsource.profileby that same handler, the row is correct in every column and simply lands in the wrong file. That is why the only visible symptom is a display one: the desktop reads per profile,so
/api/sessions?profile=fitnessopensprofiles/fitness/state.db, legitimately does not find the session, and the row shows up under the default/hermes bot instead.The fix looks the handle up through a property that resolves the active scope per access, caching one handle per resolved path. The scoping the inbound path already performs then does the routing on its own.
Why this approach
Per-profile physical stores plus read-only cross-profile aggregation already look like the intended architecture rather than something invented here:
_open_session_db_for_profileopens other profiles' stores directly, andSessionDB(read_only=True)documents itself as the non-contending attach for exactly that aggregation. On that reading the writer was the side out of line, so this moves the writer into the existing design rather than adding a new mechanism.Two details worth calling out:
store._db = None, andNonealone cannot express "unpinned" versus "deliberately disabled", so an_DB_UNPINNEDsentinel distinguishes them. A pin keeps winning across scope changes.Scope and limits (please confirm these are the calls you want)
#76584framesdefaultbeing a hardcoded alias for the root home as the underlying design problem. This is deliberately the narrow writer fix, not that decoupling.#88437adapter-derived session keys,#88387profile fences in lookup/inheritance,#75198peer fallback scoping). All three are about which profile a row is labeled with; this report confirms the label was already correct, so there is no overlap in behavior or in files beyondgateway/session.pybeing untouched by the first and third.Related Issue
Fixes #88532
Type of Change
Changes Made
gateway/session.pySessionStore.__init__no longer bindsself._db. It primes the handle for the current scope instead, which keeps 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.SessionStore._open_session_db_for_active_scope()resolves_default_db_path()per call and caches oneSessionDBper resolved path under a lock. A construction failure is cached asNonefor that path, matching the previous behavior where a failed startup left_dbNonefor the life of the store and callers fell back to JSONL. The live-guardRuntimeErroris deliberately re-raised and not cached, so it fires again on the next attempt.SessionStore._dbis now a property with a setter; the setter pins the value._DB_UNPINNEDsentinel.tests/gateway/test_multiplex_session_db_profile_scope.py(new, 5 tests).How to Test
Proof the tests encode the bug. Reverting only
gateway/session.pytomainand re-running fails 3 of the 5, including the one that reproduces the report by hand exactly as the issue does withsqlite3:The row is in the root store and the profile store is empty, which is the reported condition. With the fix, 5 passed.
The other two are guardrails, not reproductions, and pass either way by design: one pins that no-scope resolution is unchanged (single-profile gateways), the other that an explicit
store._db = ...still wins across scope changes.Note for anyone writing tests near this. The suite-wide conftest fixture re-points
hermes_state.DEFAULT_DB_PATH, which trips the deliberate escape hatch in_default_db_path()(a re-pointed constant beats everything, including the context-local override). Under that fixture every lookup is pinned to one path and scope assertions become vacuous. The fixture here restores the import-time snapshot to close the hatch, whileHERMES_HOMEkeeps resolution insidetmp_path, so no real store is ever opened.Regression sweep. All 45 test files mentioning
SessionStore:The one failure is
tests/gateway/test_session_store_prune.py::test_session_store_default_db_uses_runtime_hermes_homeand it is pre-existing and unrelated to this PR. It passes in isolation both with and without the change, and fails identically in the combined run withgateway/session.pyreverted tomain(1 failed, 431 passed), so it is a cross-file fixture-ordering interaction that already exists onmain. Flagging it only so a CI run does not get misread as a regression here.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — not claimed. I ran the 45SessionStoresuites (436 passed, 1 pre-existing failure documented above) plus the new file, not the full tree. Leaving this unchecked rather than overstating it; CI covers the rest.Documentation & Housekeeping
docs/, docstrings) — the reasoning lives in the docstrings and comments on the new resolver and property, since that is where the next reader will lookcli-config.yaml.exampleif I added/changed config keys — N/A, no config keysCONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/Apathliband the existingget_hermes_home(), which already resolves%LOCALAPPDATA%on Windows. Developed and tested on Windows 11.