From e9f1ff397da58f3ed3796fc1e2ed5b143b857202 Mon Sep 17 00:00:00 2001 From: Omar Baradei Date: Sun, 2 Aug 2026 08:47:06 -0700 Subject: [PATCH] fix(gateway): close a profile session's row in its own profile db MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_finalize_session` resolved the session store with `_get_db()`, the process-global SessionDB handle pinned to the gateway's launch home (`hermes_state.DEFAULT_DB_PATH` is a module-level constant evaluated at import). Every neighbouring write in the file already routes through the profile-aware `_session_db(session)`; this one did not. Same defect class as #323's /undo branch, one function away. A session created with `profile: ""` keeps its row in `/profiles//state.db`, so the launch handle was the wrong file, with two consequences: 1. `db.get_session(session_id)` returned None, so `source` was "" and `_is_gateway_owned_source("")` was False — `_tui_owns_lifecycle` became True for every profile session, including a Telegram/Discord one the desktop is only viewing. The #60609 Groundhog Day guard was INERT for profile sessions. 2. `db.end_session(session_id, "tui_close")` ran `UPDATE ... WHERE id = ? AND ended_at IS NULL` against the launch db and matched 0 rows — a silent no-op. The row was closed later as `agent_close` by agent teardown (run_agent.py), a reason `find_latest_gateway_session_for_peer` treats as *recoverable*, so a cleanly-closed session stayed stale-routable. Route the write through `_session_db(session)`: `/state.db` when the session carries a `profile_home`, the shared `_get_db()` handle otherwise, so the ordinary single-profile path is unchanged and the context manager closes the per-profile handle on exit. Making the guard live does not reintroduce #60609 — it extends the protection it was written for. For a gateway-owned profile session the db outcome is identical (the row still ends as the recoverable `agent_close`, previously by accident of the 0-row UPDATE, now because the guard fires). The one behavioural change there is the #55578 delegation interrupt: `_tui_owns_lifecycle` is now False, so closing a viewer tab no longer interrupts the gateway's background subagents by durable session_key — exactly what the comment at that call site says should happen. TUI/desktop-owned profile sessions are unaffected by that branch and simply stop leaving ghost rows in /resume. Co-Authored-By: Claude Opus 5 --- tests/test_tui_gateway_server.py | 51 ++++++ .../test_finalize_session_profile_db.py | 149 ++++++++++++++++++ tui_gateway/server.py | 38 +++-- 3 files changed, 224 insertions(+), 14 deletions(-) create mode 100644 tests/tui_gateway/test_finalize_session_profile_db.py diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index 8fe8e1f938b0..253637fdf3ea 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -1787,6 +1787,57 @@ def update_session_cwd(self, *_args): assert "launch_update" not in captured +def test_finalize_session_profile_session_ends_in_profile_db(monkeypatch, tmp_path): + """Closing a profile-scoped session ends it in THAT profile's state.db. + + Same contract as the resume/cwd paths above: the launch profile's cached + ``_get_db()`` handle must not be read or written. It was, here — so the end + write was a 0-row UPDATE against the wrong database and the #60609 + gateway-owned lookup read a missing row. + """ + target = "stored-profile-session" + profile_home = tmp_path / "profiles" / "worker" + profile_home.mkdir(parents=True) + captured = {} + + class ProfileDB: + def get_session(self, session_id): + captured["profile_lookup"] = session_id + return {"id": session_id, "source": "desktop"} + + def end_session(self, session_id, end_reason): + captured["profile_end"] = (session_id, end_reason) + + def close(self): + captured["profile_closed"] = True + + class LaunchDB: + def get_session(self, _session_id): + captured["launch_lookup"] = True + return {"id": target, "source": "desktop"} + + def end_session(self, *_args): + captured["launch_end"] = True + + monkeypatch.setattr("hermes_state.SessionDB", lambda db_path=None: ProfileDB()) + monkeypatch.setattr(server, "_get_db", lambda: LaunchDB()) + + session = { + "session_key": target, + "profile_home": str(profile_home), + "agent": types.SimpleNamespace(session_id=target), + "history": [], + "history_lock": None, + } + server._finalize_session(session, end_reason="tui_close") + + assert captured["profile_lookup"] == target + assert captured["profile_end"] == (target, "tui_close") + assert captured["profile_closed"] is True + assert "launch_lookup" not in captured + assert "launch_end" not in captured + + def test_stored_session_runtime_overrides_skips_bare_billing_provider(): """A bare billing bucket ("custom"/"auto"/"openrouter") must not be restored as the provider identity on resume. A custom endpoint that never used `/model` persists only diff --git a/tests/tui_gateway/test_finalize_session_profile_db.py b/tests/tui_gateway/test_finalize_session_profile_db.py new file mode 100644 index 000000000000..db6b883ca648 --- /dev/null +++ b/tests/tui_gateway/test_finalize_session_profile_db.py @@ -0,0 +1,149 @@ +"""``_finalize_session`` must close a session's row in ITS OWN profile db. + +Real ``SessionDB`` files under a temp HERMES_HOME with two profiles — no db +mocks — because a mocked handle hides the defect entirely: the finalize write +resolved its db with ``_get_db()``, the launch profile's cached ``SessionDB`` +(bound to ``DEFAULT_DB_PATH``, evaluated at import time), while every +neighbouring write in ``tui_gateway/server.py`` goes through the profile-aware +``_session_db(session)``. + +A session created with ``profile: ""`` keeps its row in +``/profiles//state.db``, so that had two consequences: + +1. ``get_session`` returned None → ``source`` was "" → the #60609 gateway-owned + guard never fired. The TUI treated every profile session as one it owns, + including a Telegram/Discord session it is only a viewer of. +2. ``end_session`` ran ``UPDATE ... WHERE id = ? AND ended_at IS NULL`` against + the launch db and matched 0 rows — a silent no-op. The row was closed later + as ``agent_close`` by agent teardown, a reason + ``find_latest_gateway_session_for_peer`` treats as *recoverable*, so a + cleanly-closed session stayed stale-routable. + +Both profile dbs seed a row under the SAME id so the write target is +unambiguous: before the fix it was the launch row that moved. +""" + +import contextlib +import threading +import types + +import pytest + +import tools.async_delegation as async_delegation +from hermes_state import SessionDB +from tui_gateway import server + +SESSION_ID = "sess-profile-1" + + +@pytest.fixture +def homes(tmp_path, monkeypatch): + """A real HERMES_HOME root: launch ``state.db`` + one extra profile's.""" + root = tmp_path / "hermes" + worker_home = root / "profiles" / "worker" + worker_home.mkdir(parents=True) + launch = SessionDB(db_path=root / "state.db") + worker = SessionDB(db_path=worker_home / "state.db") + # Never let the finalize path fall through to the host's real state.db. + monkeypatch.setattr(server, "_get_db", lambda: launch) + try: + yield types.SimpleNamespace( + launch=launch, worker=worker, worker_home=worker_home + ) + finally: + for db in (launch, worker): + with contextlib.suppress(Exception): + db.close() + + +def _session(profile_home=None, *, sid="tab1"): + return { + "agent": types.SimpleNamespace(session_id=SESSION_ID), + "history": [], + "history_lock": threading.Lock(), + "session_key": SESSION_ID, + "profile_home": str(profile_home) if profile_home else None, + "_sid": sid, + } + + +def _seed(homes, *, profile_source, launch_source="desktop"): + homes.worker.create_session(SESSION_ID, source=profile_source) + homes.launch.create_session(SESSION_ID, source=launch_source) + + +def test_profile_session_is_ended_in_its_own_profile_db(homes): + _seed(homes, profile_source="desktop") + + server._finalize_session(_session(homes.worker_home), end_reason="tui_close") + + row = homes.worker.get_session(SESSION_ID) + assert row["ended_at"] is not None + assert row["end_reason"] == "tui_close" + # The launch profile's own row is untouched — it was never this session's. + assert homes.launch.get_session(SESSION_ID)["ended_at"] is None + + +def test_gateway_owned_profile_session_is_not_ended(homes): + """#60609's guard has to read the source from the session's OWN db. + + Reading the launch db returned no row (source=""), so the TUI ended a + gateway-owned profile session as ``tui_close``/``ws_orphan_reap`` — the + write the guard exists to prevent. + """ + _seed(homes, profile_source="telegram") + + server._finalize_session(_session(homes.worker_home), end_reason="ws_orphan_reap") + + assert homes.worker.get_session(SESSION_ID)["ended_at"] is None + assert homes.launch.get_session(SESSION_ID)["ended_at"] is None + + +def test_launch_profile_session_still_ends_in_the_launch_db(homes): + """Control: a session with no profile binding is unchanged.""" + homes.launch.create_session(SESSION_ID, source="tui") + + server._finalize_session(_session(None), end_reason="tui_close") + + row = homes.launch.get_session(SESSION_ID) + assert row["ended_at"] is not None + assert row["end_reason"] == "tui_close" + + +def test_profile_session_delegations_are_interrupted_by_key(homes, monkeypatch): + """The TUI owns a desktop/TUI profile session — its background subagents + end with it (#55578).""" + _seed(homes, profile_source="desktop") + captured = {} + monkeypatch.setattr( + async_delegation, + "interrupt_for_session", + lambda **kwargs: captured.update(kwargs) or 0, + ) + + server._finalize_session( + _session(homes.worker_home, sid="tab9"), end_reason="tui_close" + ) + + assert captured["session_key"] == SESSION_ID + assert captured["origin_ui_session_id"] == "tab9" + + +def test_gateway_owned_profile_session_keeps_gateway_delegations(homes, monkeypatch): + """The other half of a live guard: closing a viewer tab on a gateway-owned + profile session must not kill the gateway's own background work. Only this + tab's own dispatches (origin id) are interrupted.""" + _seed(homes, profile_source="telegram") + captured = {} + monkeypatch.setattr( + async_delegation, + "interrupt_for_session", + lambda **kwargs: captured.update(kwargs) or 0, + ) + + server._finalize_session( + _session(homes.worker_home, sid="tab9"), end_reason="ws_orphan_reap" + ) + + assert captured["session_key"] == "" + assert captured["origin_ui_session_id"] == "tab9" diff --git a/tui_gateway/server.py b/tui_gateway/server.py index 9eddf0b10071..af4b47ac1ab8 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -675,20 +675,30 @@ def _finalize_session(session: dict | None, end_reason: str = "tui_close") -> No _tui_owns_lifecycle = True if session_id: try: - db = _get_db() - if db is not None: - # Don't end gateway-originated sessions — the gateway owns - # their lifecycle. The TUI is a viewer, not the owner. - # Ending a gateway session in state.db triggers a Groundhog - # Day routing loop: the gateway's #54878 self-heal detects - # the stale entry, recovers to the parent session, context - # compression splits back to the reaped child, and the cycle - # repeats on every inbound message. (#60609) - row = db.get_session(session_id) - source = (row or {}).get("source", "") - _tui_owns_lifecycle = not _is_gateway_owned_source(source) - if _tui_owns_lifecycle: - db.end_session(session_id, end_reason) + # Profile-aware, like every other session write in this file: a + # session created with ``profile: ""`` owns a row in THAT + # profile's state.db. ``_get_db()`` is the launch profile's cached + # handle (bound to the import-time ``DEFAULT_DB_PATH``), so it read + # and wrote the wrong database entirely for those sessions: + # ``get_session`` returned None, the guard below saw source="" and + # never fired, and ``end_session`` was a silent 0-row UPDATE that + # left the row to be closed later as ``agent_close`` — a reason + # ``find_latest_gateway_session_for_peer`` still treats as + # recoverable, so a cleanly-closed session stayed stale-routable. + with _session_db(session) as db: + if db is not None: + # Don't end gateway-originated sessions — the gateway owns + # their lifecycle. The TUI is a viewer, not the owner. + # Ending a gateway session in state.db triggers a Groundhog + # Day routing loop: the gateway's #54878 self-heal detects + # the stale entry, recovers to the parent session, context + # compression splits back to the reaped child, and the cycle + # repeats on every inbound message. (#60609) + row = db.get_session(session_id) + source = (row or {}).get("source", "") + _tui_owns_lifecycle = not _is_gateway_owned_source(source) + if _tui_owns_lifecycle: + db.end_session(session_id, end_reason) except Exception: pass