fix(acp): stamp ended_at on ACP sessions at adapter shutdown (#118216)
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).
This commit is contained in:
@@ -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__":
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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]
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
import { normalize } from '@/lib/text'
|
||||
|
||||
const SOURCE_LABELS: Record<string, string> = {
|
||||
acp: 'ACP',
|
||||
api_server: 'API',
|
||||
bluebubbles: 'iMessage',
|
||||
cli: 'CLI',
|
||||
@@ -43,8 +44,9 @@ const SOURCE_ALIASES: Record<string, string[]> = {
|
||||
// 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
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user