tui_gateway: session.resume abandons the profile SessionDB it opens

[ghost]

Problem. In app-global remote mode, session.resume resumes a session that
lives in another local profile's state.db. For that case it opens a
dedicated writer handle (tui_gateway/methods_session.py:320-327):

# In a profile scope, the agent OWNS a long-lived db handle bound to that
# profile (do NOT auto-close it here). Otherwise reuse the shared launch db.
if profile_home is not None:
    from hermes_state import SessionDB

    db = SessionDB(db_path=profile_home / "state.db")
else:
    db = _get_db()          # shared launch handle, outlives the RPC

That handle belongs to the caller until it is handed to the long-lived agent —
_init_session explicitly does not close a caller-supplied session_db
(_init_owns_db stays False, tui_gateway/server.py:6366-6368). But the
handler returns on ten paths before that transfer and closes it on none of
them: session-not-found (:350), the live-session fast path (:397), the
lazy/watch failure, concurrent-winner and success returns (:422, :436,
:454), the cold-resume failure, concurrent-winner and success returns
(:501, :527, :553), the eager "resume failed" return (:608), and the
double-checked-locking discard that throws away a just-built agent (:637).
Two of those are not edge cases: :397 is every reconnect/tile-paint resume of
an already-live chat, and :553 is the default cold resume — eager_build
is off unless a caller asks for it, so an ordinary "switch to this chat" in a
profile scope goes down a path that never transfers the handle at all.

The codebase already has the right pattern twice — _profile_db
(tui_gateway/server.py:1208-1222) and _session_db
(tui_gateway/server.py:2637-2662) are contextmanagers that close a dedicated
profile handle and leave the shared one alone. session.resume is the one
profile-aware open that does neither.

Why this is worth fixing: an abandoned SessionDB is released only when the
garbage collector gets to it, and as soon as anything holds a strong reference
it is pinned for the life of the process — SessionDB pins itself the first
time its background token writer starts, via
atexit.register(self._drain_token_queue_at_exit) (hermes_state.py:4081),
which only close() unregisters (hermes_state.py:2639). The running writer
thread holds a second strong reference.

Change. Give the handler explicit ownership, in the style of the two
existing contextmanagers: an owns_db flag set only on the profile-scoped open,
a try/finally around the whole body, and a close in the finally.

Review this with git diff -w: 40 insertions / 2 deletions ignoring
whitespace. The raw diff reads as 664+/336− on a hot file, but essentially
all of that is the re-indentation forced by wrapping the handler body in
try: so the handle can be closed on every early return.

The one subtlety is where ownership ends. It is cleared on exactly one line —
immediately after _init_session(...) returns — because that is the point the
registered session's agent takes the handle for its lifetime. Clearing it
earlier, at the _make_agent(..., session_db=db) call (:595-602), would be
wrong: the double-checked-locking branch below can still discard that agent and
return, and that path must close. Clearing it later would be wrong too —
closing after a successful transfer hands the live session a dead connection and
faults every subsequent turn with "Cannot operate on a closed database". The
in-code comment at :320-321 ("the agent OWNS a long-lived db handle … do NOT
auto-close") describes only the post-transfer state; it is reworded to say when
ownership actually changes hands.

The shared launch handle keeps its existing semantics: owns_db stays False, so
it is never closed here.

One case the finally alone gets wrong, and must be handled with it.
_init_session registers _sessions[sid] (tui_gateway/server.py:6334)
before its first read through the handle (:6381). If that read raises —
database is locked is the realistic trigger — ownership has not transferred,
so the new finally correctly closes the handle; but the half-built session is
already registered and holds a reference to it through its agent. The
live-session fast path (methods_session.py:397) then serves that dead session
on every later resume of the same id, and the chat is permanently broken with
AttributeError: 'NoneType' object has no attribute 'execute'. Before this
patch the same failure merely leaked the handle and the session kept working, so
adding the finally in isolation converts a leak into a hard, sticky
user-visible break.

The fix is to undo the registration on that path: in the except
(methods_session.py:680), if owns_db: with _sessions_lock: _sessions.pop(sid, None) before returning the error. owns_db still being True is precisely the
signal that the transfer did not happen and the registration is ours to remove.
_sessions_lock is a reentrant RLock (tui_gateway/server.py:152), so taking
it there is safe.

Evidence/Repro. Drive the profile-scoped session not found early return
and count the handles the handler opened vs. closed:

import hermes_state
from tui_gateway import server

class Counting(hermes_state.SessionDB):
    opened = closed = 0
    def __init__(self, *a, **kw):
        super().__init__(*a, **kw); type(self).opened += 1
    def close(self):
        type(self).closed += 1; return super().close()

hermes_state.SessionDB = Counting
server._profile_home = lambda p: profile_home if p else None   # a real profile dir

for i in range(25):
    server.handle_request({"id": str(i), "method": "session.resume",
                           "params": {"session_id": "no-such-session",
                                      "profile": "work"}})
print(Counting.opened, Counting.closed)

before:  opened=25  closed=0   abandoned=25
after:   opened=25  closed=25  abandoned=0

What an abandoned handle costs once it is pinned rather than collected — same 25
iterations, with the token writer started per handle (queue_token_counts, the
call agent/conversation_loop.py:3331 makes on every turn), fds counted after a
forced gc.collect():

abandoned + pinned:  102 open fds against <profile>/state.db,  25 live session-db-token-writer threads
closed:                0 open fds,                              0 threads

Being explicit about the measurement: on the early-return paths as they exist
today none of those calls start the token writer, so the abandoned handles are
reclaimed by the cycle collector — 14 fds were still open against the profile's
state.db when the 25-iteration loop finished, and a forced gc.collect() drove
that to 0. So this patch removes a latent contract violation, moves release from
the collector's schedule to the return itself, and makes the pinned case
unreachable; it is not a fix for an observed fd exhaustion.

Tests. tests/tui_gateway/test_session_resume_db_ownership.py (new, 7
tests) pins both directions: the pre-transfer early returns (session-not-found,
"resume failed", the live-session fast path, the default deferred cold resume)
each close the dedicated handle; a resume that completes the transfer leaves it
open and asserts the agent and the live session both received that handle; an
_init_session that raises after registering leaves nothing behind in
_sessions; and the shared launch handle is never closed.

Without the change, 5 of the 7 fail (the two "must NOT close" tests pass, as
they should). With the finally but without the _sessions.pop, exactly the
half-built-registration test fails.

./scripts/run_tests.sh tests/tui_gateway/test_session_resume_db_ownership.py   # 7 passed
./scripts/run_tests.sh tests/test_tui_gateway_server.py                        # 502 passed
./scripts/run_tests.sh tests/tui_gateway/                                      # 328 passed, 49 files

Alternatives considered. Wrapping the open in the existing _profile_db
contextmanager is the obvious move and does not work here: _profile_db closes
unconditionally on exit, but this handler must keep the handle open on the one
path that transfers it to the agent. A contextlib.ExitStack with pop_all()
on the transfer would express that, but it is heavier than a boolean for a
single conditional close and reads less like the surrounding code. Making
_init_session take ownership of the handle it is passed was rejected as a
wider behavioural change — its other callers pass the shared handle.

[teknium1]

Thanks for tracing the pre-transfer resume paths; the leak premise is confirmed on current main: tui_gateway/methods_session.py:325 opens a dedicated profile handle and returns at :350 without closing it.

Problems

• The new ownership transfer remains incomplete. After the proposed owns_db = False, normal teardown calls agent.close() through tui_gateway/server.py:831-855, but AIAgent.close() only calls session_db.end_session() (run_agent.py:4095-4101), not close(). A successfully resumed profile session can therefore keep its dedicated SQLite handle alive after session.close.
• The same pre-transfer failure pattern exists in the deferred builder: tui_gateway/server.py:1989-1994 opens a profile SessionDB, _make_agent receives it at :2029, and the failure handler at :2103-2105 does not close it. The branch construction path similarly opens branch_db at tui_gateway/methods_session.py:2651-2655.

Suggested changes

• Add an explicit dedicated-handle teardown owner, preserving the shared launch DB behavior, and close untransferred handles on every failed build path.
• Add teardown and failed-deferred-build ownership tests alongside the new early-return tests.

Automated hermes-sweeper review.

[ghost]

Both confirmed, both fixed in 9c62a1b5a. Thanks — the first one is the more
important of the two, because it meant the original patch moved the leak rather
than closing it.

Teardown. Right: after the transfer, AIAgent.close() called
session_db.end_session(), which finalizes the session row and not the
connection, so a successfully resumed profile session kept its dedicated handle,
its db/-wal/-shm fds and its token-writer thread for the life of the gateway.

The reason close() could not simply close its _session_db is that almost
every agent is handed the shared launch handle, which outlives it and backs
every other live session — closing that at teardown would break every other chat
in the gateway. So ownership is now explicit: AIAgent._owns_session_db,
defaulting False, set only by the sites that open a dedicated handle, at the
point ownership actually changes hands. close() closes iff it owns, and clears
the flag first so the documented idempotency still holds.

The other pre-transfer paths. Fixed with the same flag, via a small
_transfer_db_to_agent(agent, db) helper that refuses the transfer unless the
agent really holds that handle — a refusal is what tells the caller the handle
is still its own to close:

• _start_agent_build's deferred builder (tui_gateway/server.py) — including
  the case where the session is reaped mid-build. That one is worth calling
  out: the builder already computes replaced for its approval-notifier
  cleanup, and when it is true the agent it just built is unreachable, so
  _teardown_session will never close it. Transferring to that agent would have
  leaked exactly as before, so that path closes here instead.
• session.branch's branch_db (tui_gateway/methods_session.py).
• the compute host's per-profile open (tui_gateway/compute_host.py) — not in
  your list, same shape: _make_agent raising is the one path where nothing
  takes the handle.
• AIAgent._get_session_db_for_recall's own lazy SessionDB() open. Nothing
  else ever holds a reference to that one, so it was unconditionally abandoned;
  it is now owned by definition.

One deliberate asymmetry, in case it looks inconsistent on review: where a
handle has already reached a registered session, the ownership drop is
unconditional and the transfer is best-effort on top of it. Past _init_session
the session is live and holding that handle, so a refused transfer must still
not close it — that leaves the old leak, which is survivable, whereas closing
under a live session is the permanent "Cannot operate on a closed database"
break the original patch exists to avoid. The build paths, where nothing is
registered against the handle yet, do gate the close on the transfer result.

Tests. tests/tui_gateway/test_session_db_ownership_teardown.py (new, 14):
teardown closes an owned handle and never a shared one, close() stays
idempotent, end_session() still runs before the close, the lazy recall open
is owned, the transfer helper refuses a mismatched agent, and the deferred
builder closes on build failure, closes on reap-mid-build, and transfers on
success.

11 of the 14 fail without the change. The 3 that pass are the "must NOT close"
guards — they hold in both directions by design, which is the point of having
them.

tests/tui_gateway/test_session_db_ownership_teardown.py   14 passed
tests/tui_gateway/                                       342 passed
tests/test_tui_gateway_server.py                         502 passed
tests/run_agent/                                        1310 passed, 4 skipped

[kshitijk4poor]

Merged via #81071 — both your commits were cherry-picked onto current main with your authorship preserved (rebase-merge; commit 1 re-applied onto main's drifted session.resume with the hunk set verified identical), plus one follow-up test pinning the raising-close swallow/no-retry contract. Your analysis of the atexit token-writer pinning and the ten pre-transfer early returns was exactly right and is what made this the canonical fix for the fd-leak class. Thanks!