Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion hermes_cli/web_routers/sessions.py
Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,7 @@ def _delete():
sid = db.resolve_session_id(session_id)
if not sid:
return {"ok": True, "already_absent": True}
db.delete_session(sid)
db.delete_session(sid, reason="api.sessions.delete")
return {"ok": True}
finally:
db.close()
Expand Down
34 changes: 32 additions & 2 deletions hermes_state.py
Original file line number Diff line number Diff line change
Expand Up @@ -12952,6 +12952,8 @@ def delete_session(
session_id: str,
sessions_dir: Optional[Path] = None,
expected_delete_ids: Optional[List[str]] = None,
*,
reason: str = "",
) -> bool:
"""Delete a session and all its messages.

Expand All @@ -12968,18 +12970,29 @@ def delete_session(
inside the write transaction on purpose (TOCTOU guard); the cost is
accepted for correctness. Returns True if the session was found and
deleted.

``reason`` is an audit tag (``session.delete``, ``api.sessions.delete``,
…) logged with the id and whether the row had messages, so a silent
hard-delete is greppable after the fact (#95868).
"""
removed_delegate_ids: List[str] = []
expected_ids = (
set(expected_delete_ids) if expected_delete_ids is not None else None
)
stats: Dict[str, Any] = {"messages": None, "title_set": False}

def _do(conn):
cursor = conn.execute(
"SELECT 1 FROM sessions WHERE id = ? LIMIT 1", (session_id,)
"SELECT title, "
"(SELECT COUNT(*) FROM messages WHERE session_id = sessions.id) "
"FROM sessions WHERE id = ? LIMIT 1",
(session_id,),
)
if cursor.fetchone() is None:
row = cursor.fetchone()
if row is None:
return False
stats["title_set"] = bool(row[0])
stats["messages"] = int(row[1] or 0)
if expected_ids is not None:
actual_ids = {
session_id,
Expand All @@ -13001,6 +13014,13 @@ def _do(conn):

deleted = self._execute_write(_do)
if deleted:
logger.info(
"delete_session id=%s reason=%s messages=%s title=%s",
session_id,
reason or "-",
stats["messages"],
"set" if stats["title_set"] else "null",
)
for delegate_id in removed_delegate_ids:
self._remove_session_files(sessions_dir, delegate_id)
self._remove_session_files(sessions_dir, session_id)
Expand All @@ -13010,6 +13030,8 @@ def delete_session_if_empty(
self,
session_id: str,
sessions_dir: Optional[Path] = None,
*,
reason: str = "",
) -> bool:
"""Delete *session_id* only when it never gained resumable content.

Expand All @@ -13024,6 +13046,9 @@ def delete_session_if_empty(
children (delegate subagent runs) are preserved — a parent that
spawned work is not "empty" even if its own transcript never
flushed. Returns True if the session was deleted.

``reason`` is an audit tag logged when a row is actually removed
(#95868).
"""
def _do(conn):
cursor = conn.execute(
Expand All @@ -13047,6 +13072,11 @@ def _do(conn):

deleted = self._execute_write(_do)
if deleted:
logger.info(
"delete_session_if_empty id=%s reason=%s",
session_id,
reason or "-",
)
self._remove_session_files(sessions_dir, session_id)
return bool(deleted)

Expand Down
6 changes: 6 additions & 0 deletions hermes_state_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,12 @@ def _shape_preview(raw: Any) -> str:
# row was closed at the next boot instead. Same accident class as
# ws_orphan_reap — kept distinct for forensics — and equally resumable.
"startup_orphan_reap",
# Process-exit teardown of the TUI/desktop gateway (atexit
# ``_shutdown_sessions``): a backend bounce, profile switch, or WS 1012
# service restart is the same accident class as ``ws_orphan_reap``.
# Current code no longer stamps this on populated rows (#95868), but
# rows ended by older builds stay resumable.
"tui_shutdown",
)
_RECOVERABLE_END_REASONS_SQL = ", ".join(f"'{reason}'" for reason in _RECOVERABLE_END_REASONS)

Expand Down
28 changes: 28 additions & 0 deletions tests/test_empty_session_hygiene.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,34 @@ def test_no_file_cleanup_when_kept(self, db, tmp_path):
assert (sessions_dir / "busy.json").exists()


def test_delete_session_logs_reason_and_message_count(self, db, caplog):
"""Hard-deletes must be greppable after the fact (#95868)."""
db.create_session(session_id="logged", source="desktop", model="test")
db.append_message("logged", role="user", content="keep me")
db.set_session_title("logged", "Working chat")

with caplog.at_level("INFO", logger="hermes_state"):
assert db.delete_session("logged", reason="session.delete") is True

joined = "\n".join(caplog.messages)
assert "delete_session id=logged" in joined
assert "reason=session.delete" in joined
assert "messages=1" in joined
assert "title=set" in joined
assert db.get_session("logged") is None


def test_delete_session_if_empty_logs_reason(self, db, caplog):
db.create_session(session_id="draft", source="cli", model="test")

with caplog.at_level("INFO", logger="hermes_state"):
assert db.delete_session_if_empty("draft", reason="cli_exit") is True

joined = "\n".join(caplog.messages)
assert "delete_session_if_empty id=draft" in joined
assert "reason=cli_exit" in joined



class TestCLIDiscardSessionIfEmpty:
"""Wiring tests for HermesCLI._discard_session_if_empty."""
Expand Down
49 changes: 44 additions & 5 deletions tests/test_tui_gateway_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -14082,7 +14082,7 @@ def test_session_delete_refuses_active_session(monkeypatch):
called: list[str] = []

class _DB:
def delete_session(self, sid, sessions_dir=None):
def delete_session(self, sid, sessions_dir=None, **_kw):
called.append(sid)
return True

Expand Down Expand Up @@ -14133,7 +14133,7 @@ def values(self):

def test_session_delete_returns_4007_when_missing(monkeypatch):
class _DB:
def delete_session(self, sid, sessions_dir=None):
def delete_session(self, sid, sessions_dir=None, **_kw):
return False

monkeypatch.setattr(server, "_get_db", lambda: _DB())
Expand All @@ -14148,7 +14148,7 @@ def delete_session(self, sid, sessions_dir=None):

def test_session_delete_propagates_db_exception(monkeypatch):
class _DB:
def delete_session(self, sid, sessions_dir=None):
def delete_session(self, sid, sessions_dir=None, **_kw):
raise RuntimeError("disk full")

monkeypatch.setattr(server, "_get_db", lambda: _DB())
Expand All @@ -14169,7 +14169,7 @@ def test_session_delete_success_returns_deleted_id(monkeypatch):
captured: dict = {}

class _DB:
def delete_session(self, sid, sessions_dir=None):
def delete_session(self, sid, sessions_dir=None, **_kw):
captured["sid"] = sid
captured["sessions_dir"] = sessions_dir
return True
Expand Down Expand Up @@ -14323,7 +14323,7 @@ class ProfileDB:
def __init__(self, db_path=None):
captured["db_path"] = db_path

def delete_session(self, sid, sessions_dir=None):
def delete_session(self, sid, sessions_dir=None, **_kw):
captured["sid"] = sid
captured["sessions_dir"] = sessions_dir
return True
Expand Down Expand Up @@ -17725,6 +17725,45 @@ def test_close_sessions_for_transport_closes_flagged_repoints_rest(monkeypatch):
server._sessions.clear()


def test_close_sessions_for_transport_parks_without_reap_on_service_restart(
monkeypatch,
):
"""WS 1012 / service restart must park Desktop sessions, not orphan-reap.

The reporter's ``reaped_sessions=0 detached_sessions=7`` line is this
path: close_on_disconnect is off, so sessions are detached. Arming the
20s orphan reap raced atexit ``_shutdown_sessions`` on a dying backend
and ended durable chats the next process should resume (#95868).
"""
reaps = []
closed = []
monkeypatch.setattr(
server, "_schedule_ws_orphan_reap", lambda sid: reaps.append(sid)
)
monkeypatch.setattr(
server,
"_close_session_by_id",
lambda sid, *, end_reason: closed.append((sid, end_reason)) or True,
)
transport = object()
server._sessions.clear()
server._sessions["chat"] = {
"transport": transport,
"close_on_disconnect": False,
}
try:
reaped, detached = server._close_sessions_for_transport(
transport, end_reason="ws_service_restart"
)
assert reaped == 0
assert detached == 1
assert closed == []
assert reaps == []
assert server._sessions["chat"]["transport"] is server._detached_ws_transport
finally:
server._sessions.clear()


def test_close_sessions_for_transport_skips_rebound_session(monkeypatch):
"""Rebind-between-snapshot-and-stomp (#77129 concept salvage).

Expand Down
18 changes: 18 additions & 0 deletions tests/test_tui_gateway_ws.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
import threading
import time

import pytest

from hermes_cli import mcp_startup
from tui_gateway import server
from tui_gateway import ws as ws_mod
Expand Down Expand Up @@ -272,3 +274,19 @@ async def send_second():
asyncio.run(scenario())


@pytest.mark.parametrize(
("code", "expected"),
[
(1012, "ws_service_restart"),
(1001, "ws_service_restart"),
(1000, "ws_disconnect"),
(1006, "ws_disconnect"),
(None, "ws_disconnect"),
("", "ws_disconnect"),
],
)
def test_teardown_end_reason_for_ws_close(code, expected):
"""Service-restart close codes park sessions; other codes still reap."""
assert ws_mod.teardown_end_reason_for_ws_close(code) == expected


69 changes: 69 additions & 0 deletions tests/tui_gateway/test_finalize_session_persist.py
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,32 @@ def test_db_end_session_still_called(self, mock_get_db):
mock_db.end_session.assert_called_once_with("sess_123", "test")


@patch("tui_gateway.server._get_db")
def test_tui_shutdown_does_not_end_or_delete_session(self, mock_get_db):
"""Process exit (#95868) must not close or hard-delete a durable row.

atexit ``_shutdown_sessions`` finalizes every live Desktop chat with
``tui_shutdown`` when the backend bounces. Ending that row made the
chat vanish from the sidebar after reload; deleting it would match
the reporter's empty ``SELECT id FROM sessions``. Persist still
runs; the row stays open for the next process to resume.
"""
from tui_gateway.server import _finalize_session

mock_db = MagicMock()
mock_db.get_session.return_value = {"id": "sess_123", "source": "desktop"}
mock_get_db.return_value = mock_db

agent = _make_agent(session_id="sess_123")
session = _make_session(agent=agent, history=[{"role": "user", "content": "x"}])

_finalize_session(session, end_reason="tui_shutdown")

mock_db.end_session.assert_not_called()
mock_db.delete_session.assert_not_called()
mock_db.delete_session_if_empty.assert_not_called()


class TestFinalizeSessionPersistE2E:
"""End-to-end: _finalize_session must actually land unflushed turns in
state.db on disconnect/restart.
Expand Down Expand Up @@ -247,6 +273,49 @@ def test_resumed_session_not_reflushed_as_duplicates(self, tmp_path, monkeypatch
assert len(after) == 2, after


def test_tui_shutdown_keeps_populated_desktop_row_open(self, tmp_path, monkeypatch):
"""A populated Desktop chat must survive process teardown (#95868).

Profile switch / gateway reload closes the WS with 1012, then atexit
``_shutdown_sessions`` finalizes in-memory sessions with
``tui_shutdown``. The row must remain in ``sessions`` with
``ended_at IS NULL`` and its messages intact so the next process
can list and resume it.
"""
monkeypatch.setenv("HERMES_HOME", str(tmp_path / ".hermes"))
from hermes_state import SessionDB
import tui_gateway.server as srv

db = SessionDB(db_path=tmp_path / "state.db")
session_id = "20260826_132708_a12a19"
db.create_session(session_id=session_id, source="desktop")
db.append_message(session_id, role="user", content="hour-long working session")
db.append_message(
session_id, role="assistant", content="continuing the work…"
)
monkeypatch.setattr(srv, "_get_db", lambda: db)

agent = self._real_agent(db, session_id, [])
session = _make_session(agent=agent, history=[], session_key=session_id)
session["source"] = "desktop"

srv._finalize_session(session, end_reason="tui_shutdown")

ids = [
r["id"]
for r in db._conn.execute("SELECT id FROM sessions").fetchall()
]
assert session_id in ids
row = db.get_session(session_id)
assert row is not None
assert row["ended_at"] is None
assert row["end_reason"] is None
contents = [
m.get("content") for m in db.get_messages_as_conversation(session_id)
]
assert any("hour-long" in (c or "") for c in contents), contents


class TestOnSessionEndHook:
"""Verify on_session_end plugin hook fires on finalize."""

Expand Down
11 changes: 11 additions & 0 deletions tests/tui_gateway/test_gateway_owned_session_reap.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,3 +72,14 @@ def test_missing_row_still_ended(self, mock_get_db):
_finalize_session(_make_session(), end_reason="tui_close")

db.end_session.assert_called_once_with("sess_1", "tui_close")

@patch("tui_gateway.server._get_db")
def test_missing_row_still_ended_on_tui_shutdown(self, mock_get_db):
"""Keep-open (#95868) skips end_session only when a durable row exists."""
db = MagicMock()
db.get_session.return_value = None
mock_get_db.return_value = db

_finalize_session(_make_session(), end_reason="tui_shutdown")

db.end_session.assert_called_once_with("sess_1", "tui_shutdown")
2 changes: 2 additions & 0 deletions tests/tui_gateway/test_startup_orphan_sweep.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ def test_swept_row_stays_resumable(self, monkeypatch, tmp_path):

# The distinct reason is a recoverable accident, not a boundary.
assert "startup_orphan_reap" in SessionDB.RECOVERABLE_END_REASONS
# Process-exit teardown is the same accident class (#95868).
assert "tui_shutdown" in SessionDB.RECOVERABLE_END_REASONS

# Peer-keyed recovery still surfaces the swept row...
recovered = db.find_latest_gateway_session_for_peer(
Expand Down
4 changes: 3 additions & 1 deletion tui_gateway/methods_session.py
Original file line number Diff line number Diff line change
Expand Up @@ -1283,7 +1283,9 @@ def _(rid, params: dict) -> dict:
else:
sessions_dir = get_hermes_home() / "sessions"
try:
deleted = db.delete_session(target, sessions_dir=sessions_dir)
deleted = db.delete_session(
target, sessions_dir=sessions_dir, reason="session.delete"
)
except Exception as e:
return _err(rid, 5036, f"delete failed: {e}")
if not deleted:
Expand Down
Loading
Loading