From 9fb2dd4c3961005d99b34aae22343e73423ec5f0 Mon Sep 17 00:00:00 2001 From: Hermes Agent Date: Thu, 24 Sep 2026 18:38:38 -0500 Subject: [PATCH] fix(acp): stamp ended_at on ACP sessions at adapter shutdown (#118216) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ACP v0.9 has no per-session destroy, so the adapter's stdio shutdown is the session end — but nothing ever wrote it: session rows created with source="acp" kept ended_at NULL forever, the ended-session guard in hermes_state_maintenance (prune/archive share it) could never select them, and the desktop recents list accumulated one auto-titled row per editor wake. - SessionManager.end_all_sessions(): best-effort end_session("acp_disconnect") for every live session; called from entry.py's finally so EOF, SIGINT and a crash all stamp the rows. - _restore() reopens a row ended by a previous adapter process before resuming it — the same contract the TUI gateway's cold resume uses, so load/resume across editor restarts keeps working. - Desktop sidebar: 'acp' joins SIDEBAR_EXCLUDED_SOURCES (recents) and LOCAL_SESSION_SOURCE_IDS (keeps it out of the messaging slice); the conversations live in the editor, not the app's recents. The prune/archive "open session(s) also match these filters" warning the issue asks for already exists on main (count_open_prune_matches in _cmd_prune_or_archive). --- acp_adapter/entry.py | 6 ++ acp_adapter/session.py | 37 ++++++++++ .../session/hooks/use-session-list-actions.ts | 7 +- apps/desktop/src/lib/session-source.ts | 6 +- tests/acp_adapter/test_session.py | 69 +++++++++++++++++++ 5 files changed, 121 insertions(+), 4 deletions(-) diff --git a/acp_adapter/entry.py b/acp_adapter/entry.py index b129d6036c..c8c14485e7 100644 --- a/acp_adapter/entry.py +++ b/acp_adapter/entry.py @@ -223,6 +223,12 @@ def main(argv: list[str] | None = None) -> None: except Exception: logger.exception("ACP agent crashed") sys.exit(1) + finally: + # The stdio client that drove these conversations is gone. Without an + # ended_at writer here, source='acp' rows stay open forever and the + # ended-session guard keeps prune/archive away from them (#118216). A + # later load/resume reopens the row (acp_adapter.session._restore). + agent.session_manager.end_all_sessions() if __name__ == "__main__": diff --git a/acp_adapter/session.py b/acp_adapter/session.py index 9598f18af6..d226105d5e 100644 --- a/acp_adapter/session.py +++ b/acp_adapter/session.py @@ -273,6 +273,34 @@ class SessionManager: if state is not None: self._persist(state) + def end_all_sessions(self, end_reason: str = "acp_disconnect") -> int: + """Stamp ``ended_at`` on every live session (#118216). + + ACP v0.9 has no per-session destroy, so the stdio shutdown that ends + this process is the session end: the client that drove the + conversation is gone. Without this writer, source='acp' rows keep + ``ended_at`` NULL forever and the ended-session guard shared by + prune/archive (``hermes_state_maintenance``) can never reach them. + A later load/resume reopens the row (see ``_restore``), the same + contract the TUI gateway's resume path uses. Best-effort: teardown + must never raise. Returns the number of sessions ended. + """ + db = self._get_db() + if db is None: + return 0 + with self._lock: + session_ids = list(self._sessions.keys()) + ended = 0 + for session_id in session_ids: + try: + db.end_session(session_id, end_reason) + ended += 1 + except Exception: + logger.debug("Failed to end ACP session %s", session_id, exc_info=True) + if ended: + logger.info("Ended %d ACP session(s) on shutdown (%s)", ended, end_reason) + return ended + # ---- persistence via SessionDB ------------------------------------------ def _install_state(self, session_id: str, agent: Any, cwd: str, model: str, @@ -428,6 +456,15 @@ class SessionManager: if row is None or row.get("source") != "acp": return None + # A previous adapter process stamped the row ended at its stdio + # shutdown (#118216); resuming the conversation reopens it, the same + # contract the TUI gateway's cold-resume path uses. + if row.get("ended_at") is not None: + try: + db.reopen_session(session_id) + except Exception: + logger.debug("Failed to reopen ACP session %s", session_id, exc_info=True) + meta = _parse_model_config(row.get("model_config")) cwd, model = meta.get("cwd", "."), row.get("model") or None diff --git a/apps/desktop/src/app/session/hooks/use-session-list-actions.ts b/apps/desktop/src/app/session/hooks/use-session-list-actions.ts index ea193b5a7f..7f279eeb2b 100644 --- a/apps/desktop/src/app/session/hooks/use-session-list-actions.ts +++ b/apps/desktop/src/app/session/hooks/use-session-list-actions.ts @@ -53,8 +53,11 @@ import { refreshCronJobs as refreshCronJobsStore } from '../../cron/cron-actions // (telegram, discord, …) is fetched separately into its own self-managed // sidebar section (refreshMessagingSessions). Excluding them here keeps // "Load more" paging through interactive local chats instead of -// interleaving gateway threads that bury them. -const SIDEBAR_EXCLUDED_SOURCES = ['cron', 'kanban', 'oneshot', 'subagent', 'tool', ...MESSAGING_SESSION_SOURCE_IDS] +// interleaving gateway threads that bury them. ACP rows are editor-driven +// conversations: every editor wake mints an auto-titled row, so they would +// bury local chats — and they were never ended before #118216, which also +// kept prune/archive away from them. +const SIDEBAR_EXCLUDED_SOURCES = ['acp', 'cron', 'kanban', 'oneshot', 'subagent', 'tool', ...MESSAGING_SESSION_SOURCE_IDS] // The messaging slice is the inverse: drop cron + every local source so only // external-platform conversations remain, then split per platform in the UI. const MESSAGING_EXCLUDED_SOURCES = ['cron', ...LOCAL_SESSION_SOURCE_IDS] diff --git a/apps/desktop/src/lib/session-source.ts b/apps/desktop/src/lib/session-source.ts index d04904e595..0991f882d0 100644 --- a/apps/desktop/src/lib/session-source.ts +++ b/apps/desktop/src/lib/session-source.ts @@ -1,6 +1,7 @@ import { normalize } from '@/lib/text' const SOURCE_LABELS: Record = { + acp: 'ACP', api_server: 'API', bluebubbles: 'iMessage', cli: 'CLI', @@ -43,8 +44,9 @@ const SOURCE_ALIASES: Record = { // Sources that run on the local machine rather than an external messaging // platform. A handoff *from* one of these isn't a platform origin worth a badge. // Exported so the recents fetch can keep these in the main list while the -// messaging fetch excludes them. -export const LOCAL_SESSION_SOURCE_IDS = ['cli', 'codex', 'desktop', 'gateway', 'kanban', 'local', 'oneshot', 'tui'] +// messaging fetch excludes them. `acp` runs as a local stdio process spawned +// by an editor, and its rows must never land in the messaging slice either. +export const LOCAL_SESSION_SOURCE_IDS = ['acp', 'cli', 'codex', 'desktop', 'gateway', 'kanban', 'local', 'oneshot', 'tui'] const LOCAL_SOURCE_IDS = new Set(LOCAL_SESSION_SOURCE_IDS) // External messaging platforms that each get their own self-managed sidebar diff --git a/tests/acp_adapter/test_session.py b/tests/acp_adapter/test_session.py index 294ba86bf0..917f95eea3 100644 --- a/tests/acp_adapter/test_session.py +++ b/tests/acp_adapter/test_session.py @@ -434,6 +434,75 @@ class TestPersistence: # Should not be found via ACP SessionManager. assert manager.get_session("cli-session-123") is None + def test_end_all_sessions_stamps_ended_and_unlocks_prune(self, tmp_path): + """#118216: the adapter's stdio shutdown is the ACP session end. Without the + ended_at writer, source='acp' rows are invisible to prune/archive forever (the + maintenance filter only ever selects ended sessions) and the desktop sidebar + accumulates one auto-titled row per editor wake.""" + agent = SimpleNamespace(model="test-model", provider=None, base_url=None, api_mode=None) + db = SessionDB(tmp_path / "state.db") + manager = SessionManager(agent_factory=lambda: agent, db=db) + ended_earlier = manager.create_session(cwd="/work") + live_one = manager.create_session(cwd="/work") + live_two = manager.create_session(cwd="/work") + for state in (ended_earlier, live_one, live_two): + state.history.append({"role": "user", "content": "hello"}) + manager.save_session(state.session_id) + # A row already ended by an earlier boundary keeps its first end_reason + # (end_session is first-writer-wins). + db.end_session(ended_earlier.session_id, "compression") + + # Before the shutdown writer, only the already-ended row is a prune + # candidate — the two live acp rows are invisible to maintenance. + assert {row["id"] for row in db.list_prune_candidates(source="acp", older_than_days=0)} == { + ended_earlier.session_id} + + ended = manager.end_all_sessions() + + assert ended == 3 + rows = {sid: db.get_session(sid) for sid in + (ended_earlier.session_id, live_one.session_id, live_two.session_id)} + assert all(row is not None for row in rows.values()) + assert rows[ended_earlier.session_id]["end_reason"] == "compression" + for sid in (live_one.session_id, live_two.session_id): + assert rows[sid]["ended_at"] is not None + assert rows[sid]["end_reason"] == "acp_disconnect" + candidates = db.list_prune_candidates(source="acp", older_than_days=0) + assert {row["id"] for row in candidates} == { + ended_earlier.session_id, live_one.session_id, live_two.session_id} + + def test_restore_reopens_a_session_ended_by_previous_process(self, tmp_path): + """#118216: an ACP row stamped ended at a previous adapter's shutdown is + reopened when a later process resumes it — the same contract as the TUI + gateway's cold resume.""" + agent = SimpleNamespace(model="test-model", provider=None, base_url=None, api_mode=None) + db = SessionDB(tmp_path / "state.db") + first = SessionManager(agent_factory=lambda: agent, db=db) + state = first.create_session(cwd="/work") + state.history.append({"role": "user", "content": "hello"}) + first.save_session(state.session_id) + first.end_all_sessions() + assert db.get_session(state.session_id)["ended_at"] is not None + + second = SessionManager(agent_factory=lambda: agent, db=db) + restored = second.get_session(state.session_id) + + assert restored is not None + row = db.get_session(state.session_id) + assert row is not None + assert row["ended_at"] is None + assert row["end_reason"] is None + + def test_end_all_sessions_without_db_is_a_noop(self): + """Teardown must never raise; a DB-unavailable process just ends nothing.""" + manager = SessionManager( + agent_factory=_mock_agent, + db=None, + ) + manager._db_instance = None + with patch.object(manager, "_get_db", return_value=None): + assert manager.end_all_sessions() == 0 + def test_sessions_searchable_via_fts(self, manager): """ACP sessions stored in SessionDB are searchable via FTS5.""" state = manager.create_session()