From 73b6c5d604f48826f729652937d795bbd0e6830d Mon Sep 17 00:00:00 2001 From: pxxD1998 <214340659+pxxD1998@users.noreply.github.com> Date: Sat, 11 Jul 2026 11:54:30 +0800 Subject: [PATCH] fix(sessions): cascade delegated subagents on parent deletion --- api/models.py | 605 +++++- api/routes.py | 15 +- docs/architecture/agent-api-contract.md | 7 +- static/sessions.js | 11 +- ...est_delete_cli_session_delegate_cascade.py | 1666 +++++++++++++++++ tests/test_issue1494_state_db_fd_leak.py | 6 +- tests/test_issue2057_worktree_lifecycle.py | 5 +- tests/test_issue2057_worktree_ui_static.py | 9 +- tests/test_state_db_readonly_reads_models.py | 7 +- 9 files changed, 2304 insertions(+), 27 deletions(-) create mode 100644 tests/test_delete_cli_session_delegate_cascade.py diff --git a/api/models.py b/api/models.py index 3dc133ea..75c22b8e 100644 --- a/api/models.py +++ b/api/models.py @@ -6,14 +6,25 @@ import hashlib import inspect import json import logging +import math import os import re import threading import time import uuid -from contextlib import closing +from contextlib import closing, contextmanager from pathlib import Path +try: # pragma: no cover - platform-specific imports. + import fcntl as _fcntl +except ImportError: # pragma: no cover + _fcntl = None + +try: # pragma: no cover - platform-specific imports. + import msvcrt as _msvcrt +except ImportError: # pragma: no cover + _msvcrt = None + import api.config as _cfg from api.compression_anchor import is_context_compression_marker from api.config import ( @@ -8594,31 +8605,599 @@ def count_conversation_rounds(sid: str, since: float | None = None) -> int: CONVERSATION_ROUND_THRESHOLD = 10 +@contextmanager +def _cleanup_manifest_process_lock(hermes_home): + """Serialize cleanup across WebUI worker processes for one profile. + + Keep the lock file in place permanently: unlinking it while another process + is waiting can split later callers across different inodes and defeat the + lock. POSIX uses ``flock``; native Windows locks the first byte with + ``msvcrt.locking``. If neither primitive exists, fail closed rather than + running destructive cleanup without cross-process serialization. + """ + lock_path = Path(hermes_home) / ".session_cleanup.lock" + lock_path.parent.mkdir(parents=True, exist_ok=True) + fd = os.open(lock_path, os.O_CREAT | os.O_RDWR, 0o600) + with os.fdopen(fd, "r+b", buffering=0) as lock_file: + if _fcntl is not None: + _fcntl.flock(lock_file.fileno(), _fcntl.LOCK_EX) + try: + yield + finally: + _fcntl.flock(lock_file.fileno(), _fcntl.LOCK_UN) + return + + if _msvcrt is not None: + if os.fstat(lock_file.fileno()).st_size == 0: + lock_file.write(b"\0") + lock_file.seek(0) + _msvcrt.locking( # type: ignore[attr-defined] + lock_file.fileno(), _msvcrt.LK_LOCK, 1 # type: ignore[attr-defined] + ) + try: + yield + finally: + lock_file.seek(0) + _msvcrt.locking( # type: ignore[attr-defined] + lock_file.fileno(), _msvcrt.LK_UNLCK, 1 # type: ignore[attr-defined] + ) + return + + raise RuntimeError("cross-process session cleanup locking is unavailable") + + +_cleanup_manifest_locks_guard = threading.Lock() +_cleanup_manifest_locks = {} + + +def _cleanup_manifest_thread_lock(hermes_home): + """Return the in-process cleanup lock for one resolved profile home.""" + key = os.fspath(Path(hermes_home)) + with _cleanup_manifest_locks_guard: + lock = _cleanup_manifest_locks.get(key) + if lock is None: + lock = threading.Lock() + _cleanup_manifest_locks[key] = lock + return lock + + def delete_cli_session(sid) -> bool: - """Delete a CLI session from state.db (messages + session row). - Returns True if deleted, False if not found or error. + """Delete a CLI session while serializing manifest and DB cleanup.""" + try: + from api.profiles import get_active_hermes_home + hermes_home = Path(get_active_hermes_home()).expanduser().resolve() + except Exception: + logger.warning("Failed to resolve active profile for session delete", exc_info=True) + return False + try: + with _cleanup_manifest_thread_lock(hermes_home): + with _cleanup_manifest_process_lock(hermes_home): + return _delete_cli_session_locked(sid, hermes_home) + except Exception: + logger.warning("Failed to delete CLI session %s from state.db", sid, exc_info=True) + return False + + +def _delete_cli_session_locked(sid, hermes_home) -> bool: + """Delete a CLI session from state.db using Hermes' session semantics. + + A scoped transaction implements the Agent invariant while giving branch and + compression evidence precedence over inherited delegate metadata. Current + Hermes Agent's canonical helper does not yet make that precedence guarantee, + so this destructive path fails closed instead of delegating to it. + + Returns True when the requested state is absent after cleanup, False on an + operational error. """ - import os try: import sqlite3 except ImportError: return False - try: - from api.profiles import get_active_hermes_home - hermes_home = Path(get_active_hermes_home()).expanduser().resolve() - except Exception: - hermes_home = Path(os.getenv('HERMES_HOME', str(HOME / '.hermes'))).expanduser().resolve() + # Process any leftover cleanup manifests from a previous failed run. + # This runs before the DB-existence check so pending artifact + # removals get another chance even when the current session ID + # is unrelated. + stale_cleanup_complete = _process_stale_cleanup_manifests(hermes_home) + db_path = hermes_home / 'state.db' if not db_path.exists(): return False try: with closing(sqlite3.connect(str(db_path))) as conn: - cur = conn.cursor() - cur.execute("DELETE FROM messages WHERE session_id = ?", (sid,)) - cur.execute("DELETE FROM sessions WHERE id = ?", (sid,)) + conn.row_factory = sqlite3.Row + # SQLite does not enforce foreign keys by default; enabling + # PRAGMA foreign_keys makes the ON DELETE CASCADE clauses on + # session_model_usage and telegram_dm_topic_bindings fire + # automatically. Compression locks have no FK, so they are + # cleaned explicitly below. + conn.execute("PRAGMA foreign_keys = ON") + conn.execute("BEGIN IMMEDIATE") + columns = { + row[1] for row in conn.execute("PRAGMA table_info(sessions)").fetchall() + } + if not {"id", "parent_session_id"}.issubset(columns): + return False + + selected = ["id", "parent_session_id"] + for column in ( + "model_config", "source", "end_reason", "started_at", "ended_at" + ): + selected.append(column if column in columns else f"NULL AS {column}") + rows = conn.execute(f"SELECT {', '.join(selected)} FROM sessions").fetchall() + + # _delegate_from is authoritative. source=subagent is the legacy + # compatibility signal for rows created before that marker existed, + # but legacy branch/compression continuations must remain intact. + records = [] + for row in rows: + model_config = {} + raw_model_config = row["model_config"] + model_config_known = raw_model_config in (None, "") + if not model_config_known: + try: + parsed_model_config = json.loads(raw_model_config) + except (TypeError, ValueError): + parsed_model_config = None + if isinstance(parsed_model_config, dict): + model_config = parsed_model_config + model_config_known = True + records.append( + { + "id": row["id"], + "parent_id": row["parent_session_id"], + "delegate_from": model_config.get("_delegate_from"), + "branched_from": model_config.get("_branched_from"), + "model_config_known": model_config_known, + "source": row["source"], + "end_reason": row["end_reason"], + "started_at": row["started_at"], + "ended_at": row["ended_at"], + } + ) + records_by_id = {record["id"]: record for record in records} + + def _timestamp_value(value): + """Return a comparable UTC timestamp, or None when ambiguous.""" + if isinstance(value, bool) or value in (None, ""): + return None + if isinstance(value, (int, float)): + numeric = float(value) + return numeric if math.isfinite(numeric) else None + if isinstance(value, datetime.datetime): + parsed = value + elif isinstance(value, str): + raw = value.strip() + if not raw: + return None + try: + numeric = float(raw) + return numeric if math.isfinite(numeric) else None + except ValueError: + try: + parsed = datetime.datetime.fromisoformat( + raw[:-1] + "+00:00" if raw.endswith("Z") else raw + ) + except ValueError: + return None + else: + return None + if parsed.tzinfo is None: + return None + try: + numeric = parsed.timestamp() + except (OverflowError, OSError, ValueError): + return None + return numeric if math.isfinite(numeric) else None + + def _must_preserve(record, parent): + """Fail closed when branch/compression evidence is ambiguous.""" + if record["branched_from"] is not None: + return True + end_reason = parent.get("end_reason") + if end_reason == "compression": + return True + if end_reason != "branched": + return False + started_at = _timestamp_value(record["started_at"]) + parent_ended_at = _timestamp_value(parent.get("ended_at")) + if started_at is None or parent_ended_at is None: + return True + return started_at >= parent_ended_at + + def _lineage_parent(record): + """Return the parent that supplies this row's lineage edge. + + ``_delegate_from`` is authoritative when present. Falling back + to the physical parent is only valid for legacy rows without + that marker; otherwise a compression continuation whose physical + parent differs from its lineage parent could be deleted. + """ + delegate_from = record["delegate_from"] + if delegate_from is not None: + return records_by_id.get(delegate_from) or {} + return records_by_id.get(record["parent_id"]) or {} + + # Once a branch/compression continuation is preserved, its physical + # child tree is outside the delete lineage. Stale inherited delegate + # markers must not let traversal re-enter that retained subtree. + preserved_ids = { + record["id"] + for record in records + if record["id"] != sid + and _must_preserve(record, _lineage_parent(record)) + } + while True: + descendants = { + record["id"] + for record in records + if record["id"] != sid + and record["parent_id"] in preserved_ids + } + new_ids = descendants - preserved_ids + if not new_ids: + break + preserved_ids.update(new_ids) + + found = {sid} + frontier = {sid} + while frontier: + next_frontier = set() + for record in records: + row_id = record["id"] + if row_id in found or row_id in preserved_ids: + continue + parent_id = record["parent_id"] + delegate_from = record["delegate_from"] + linked_to_frontier = ( + delegate_from in frontier or parent_id in frontier + ) + if not linked_to_frontier: + continue + parent = _lineage_parent(record) + if _must_preserve(record, parent): + continue + # Explicit _delegate_from is authoritative even when a + # migrated legacy row lacks source='subagent'. Branch and + # compression evidence above still wins and preserves it. + if delegate_from is not None: + if delegate_from in frontier: + next_frontier.add(row_id) + continue + # Only marker-less legacy inference requires the historical + # source tag plus a compatible physical-parent edge. + if record["source"] != "subagent": + continue + if ( + parent_id in frontier + and record["model_config_known"] + ): + next_frontier.add(row_id) + + found.update(next_frontier) + frontier = next_frontier + + delegate_ids = sorted(found - {sid}) + all_removed_ids = [sid, *delegate_ids] + placeholders = ",".join("?" * len(all_removed_ids)) + + # Delete delegate children first (messages, then orphan their + # children, then the rows themselves). + for child_id in delegate_ids: + conn.execute("DELETE FROM messages WHERE session_id = ?", (child_id,)) + for child_id in delegate_ids: + conn.execute( + "UPDATE sessions SET parent_session_id = NULL WHERE parent_session_id = ?", + (child_id,), + ) + for child_id in delegate_ids: + conn.execute("DELETE FROM sessions WHERE id = ?", (child_id,)) + + # Preserve every remaining child as an independent row, matching + # current SessionDB.delete_session(). + conn.execute( + "UPDATE sessions SET parent_session_id = NULL WHERE parent_session_id = ?", + (sid,), + ) + conn.execute("DELETE FROM messages WHERE session_id = ?", (sid,)) + conn.execute("DELETE FROM sessions WHERE id = ?", (sid,)) + + # Referential-integrity cleanup for session-owned rows that are + # not covered by ON DELETE CASCADE. session_model_usage and + # telegram_dm_topic_bindings have FK CASCADE (enforced by + # PRAGMA foreign_keys = ON above), but compression_locks has no + # foreign key, so stale rows would survive. Gate each table + # on existence so this works on older schemas too. + table_names = { + row[0] + for row in conn.execute( + "SELECT name FROM sqlite_master WHERE type='table'" + ).fetchall() + } + if "compression_locks" in table_names: + conn.execute( + f"DELETE FROM compression_locks WHERE session_id IN ({placeholders})", + all_removed_ids, + ) + # Belt-and-suspenders: also explicitly clear FK tables in case + # PRAGMA foreign_keys is OFF on an older SQLite build or a + # future schema drops the CASCADE clause. + if "session_model_usage" in table_names: + conn.execute( + f"DELETE FROM session_model_usage WHERE session_id IN ({placeholders})", + all_removed_ids, + ) + if "telegram_dm_topic_bindings" in table_names: + conn.execute( + f"DELETE FROM telegram_dm_topic_bindings WHERE session_id IN ({placeholders})", + all_removed_ids, + ) + + # Persist a cleanup manifest BEFORE the commit so artifact + # removal is idempotent and retryable. Each call uses a unique + # manifest filename (atomic temp-file + rename) so concurrent + # deletes never clobber each other's retry records. + sessions_dir = hermes_home / "sessions" + sessions_dir.mkdir(parents=True, exist_ok=True) + manifest_basename = f".cleanup_manifest_{uuid.uuid4().hex}" + manifest_path = sessions_dir / f"{manifest_basename}.json" + manifest_tmp = sessions_dir / f"{manifest_basename}.tmp" + try: + manifest_tmp.write_text( + json.dumps(sorted(str(i) for i in all_removed_ids)), + encoding="utf-8", + ) + manifest_tmp.rename(manifest_path) + except OSError: + logger.warning("Failed to write cleanup manifest", exc_info=True) + try: + manifest_tmp.unlink(missing_ok=True) + except OSError: + logger.warning( + "Failed to remove incomplete cleanup manifest %s", + manifest_tmp, + exc_info=True, + ) + # Publishing the retry record is a prerequisite for making the + # DB deletion durable. Without it, committed rows could vanish + # while transcript artifacts remain forever and the UI reports + # a false success. + conn.rollback() + return False + conn.commit() - return cur.rowcount > 0 + + # Post-commit artifact cleanup. Scan ALL outstanding manifests + # (including the one just written) and retry every pending ID. + # Each manifest entry is re-checked against the DB so a stale + # manifest from a failed commit never unlinks a live session's + # artifacts. + artifact_cleanup_failed = False + + def _is_session_alive(conn, sid): + """True when *sid* still has a row in the sessions table. + On any query failure, assume alive (fail closed) — an + uncertain liveness check must never trigger artifact + deletion on this unrecoverable state.db path. + """ + try: + cursor = conn.execute( + "SELECT 1 FROM sessions WHERE id = ?", (sid,) + ) + return cursor.fetchone() is not None + except Exception: + return True + + def _clean_artifacts_for_id(sessions_dir, removed_id): + """Remove on-disk transcript files for one session ID. + Returns True when every artifact is gone (or was absent). + """ + if not is_safe_session_id(removed_id): + return False + ok = True + for suffix in (".json", ".jsonl"): + artifact = sessions_dir / f"{removed_id}{suffix}" + if not artifact.exists(): + continue + try: + artifact.unlink(missing_ok=True) + except OSError: + ok = False + logger.warning( + "Failed to remove session artifact %s%s", + removed_id, + suffix, + exc_info=True, + ) + try: + for path in list( + sessions_dir.glob(f"request_dump_{removed_id}_*.json") + ): + try: + path.unlink(missing_ok=True) + except OSError: + ok = False + logger.warning( + "Failed to remove request dump %s", + path, + exc_info=True, + ) + except OSError: + ok = False + logger.warning( + "Failed to enumerate request dumps for %s", + removed_id, + exc_info=True, + ) + return ok + + for mp in sorted(sessions_dir.glob(".cleanup_manifest_*.json")): + try: + raw = mp.read_text(encoding="utf-8") + pending_ids = json.loads(raw) + except (OSError, ValueError, TypeError): + # The IDs are unknown, so deleting the manifest would lose + # the only retry record for potentially orphaned artifacts. + artifact_cleanup_failed = True + continue + if not isinstance(pending_ids, list) or not all( + isinstance(item, str) for item in pending_ids + ): + artifact_cleanup_failed = True + continue + still_pending = [] + for removed_id in pending_ids: + # Transaction-outcome guard: never unlink artifacts for + # a session that still exists in the DB. A stale + # manifest from a failed commit must not delete data + # belonging to a live conversation. + if _is_session_alive(conn, removed_id): + still_pending.append(removed_id) + continue + if not _clean_artifacts_for_id(sessions_dir, removed_id): + still_pending.append(removed_id) + if still_pending: + # Atomic rewrite using temp-file + rename. + tmp = mp.with_suffix(".tmp") + try: + tmp.write_text(json.dumps(still_pending), encoding="utf-8") + tmp.rename(mp) + except OSError: + logger.warning( + "Failed to rewrite manifest %s", mp, exc_info=True, + ) + artifact_cleanup_failed = True + else: + mp.unlink(missing_ok=True) + + return stale_cleanup_complete and not artifact_cleanup_failed except Exception: + logger.warning("Failed to delete CLI session %s from state.db", sid, exc_info=True) return False + +# --------------------------------------------------------------------------- +# ``delete_cli_session`` nests each profile's cross-process file lock inside +# that profile's in-process lock so stale-manifest read, DB transaction, and +# post-commit cleanup remain one critical section without blocking unrelated +# profiles. +# --------------------------------------------------------------------------- +def _process_stale_cleanup_manifests(hermes_home) -> bool: + """Process any leftover cleanup manifests outside a DB transaction. + + Called at the start of each delete_cli_session run, before the + regular transaction, so a previous run whose DB commit succeeded + but artifact cleanup failed gets another chance — even when the + session ID being deleted this time is unrelated. + + Serialized by the caller's per-profile thread and process locks so that the + manifest read → DB transaction → post-commit cleanup triad is + never interleaved across concurrent delete calls. Returns ``True`` only + when every discovered retry record was processed completely. + """ + try: + import sqlite3 + except ImportError: + return False + db_path = hermes_home / "state.db" + sessions_dir = hermes_home / "sessions" + if not sessions_dir.exists(): + return True + manifests = sorted(sessions_dir.glob(".cleanup_manifest_*.json")) + if not manifests: + return True + # Missing state.db is not proof that a manifested session is dead. It may + # have been temporarily renamed, unmounted, or made inaccessible. Preserve + # every manifest and artifact until a successful query proves absence. + if not db_path.exists(): + return False + cleanup_complete = True + for mp in manifests: + try: + raw = mp.read_text(encoding="utf-8") + pending_ids = json.loads(raw) + except (OSError, ValueError, TypeError): + # Preserve malformed/unreadable retry records. Their pending IDs + # are unknowable, so silently deleting them would turn an + # incomplete cleanup into a false success. + cleanup_complete = False + continue + if not isinstance(pending_ids, list) or not all( + isinstance(item, str) for item in pending_ids + ): + cleanup_complete = False + continue + pending_ids = list(pending_ids) + if not pending_ids: + mp.unlink(missing_ok=True) + continue + # Require a successful read-only query to prove absence. Opening via a + # read-only URI also prevents SQLite from creating a fresh empty DB if + # state.db disappears between the existence check and connect(). + try: + db_uri = f"{db_path.resolve().as_uri()}?mode=ro" + with closing(sqlite3.connect(db_uri, uri=True)) as conn: + cursor = conn.execute( + "SELECT id FROM sessions WHERE id IN ({})".format( + ",".join("?" * len(pending_ids)) + ), + pending_ids, + ) + alive = {row[0] for row in cursor.fetchall()} + except Exception: + # Liveness query failed (missing DB, lock, timeout, I/O error). + # Fail closed: preserve the manifest file on disk and skip it this + # round. A later call can retry when DB state is queryable. + cleanup_complete = False + continue + + still_pending = [] + for removed_id in pending_ids: + if removed_id in alive: + # Session still exists — the previous commit never + # reached the DB, so this manifest entry is stale. + # Drop it silently: never propagate a stale manifest + # into the post-commit cleanup loop where it would + # cause the current call to report a false failure. + continue + if not _clean_pending_artifact(sessions_dir, removed_id): + still_pending.append(removed_id) + if still_pending: + tmp = mp.with_suffix(".tmp") + try: + tmp.write_text(json.dumps(still_pending), encoding="utf-8") + tmp.rename(mp) + except OSError: + cleanup_complete = False + else: + try: + mp.unlink(missing_ok=True) + except OSError: + cleanup_complete = False + return cleanup_complete + + +def _clean_pending_artifact(sessions_dir, removed_id): + """Remove on-disk transcript files for one session ID, outside a + DB transaction. Returns True when every artifact is gone (or absent). + """ + if not is_safe_session_id(removed_id): + return False + ok = True + for suffix in (".json", ".jsonl"): + artifact = sessions_dir / f"{removed_id}{suffix}" + if not artifact.exists(): + continue + try: + artifact.unlink(missing_ok=True) + except OSError: + ok = False + try: + for path in list(sessions_dir.glob(f"request_dump_{removed_id}_*.json")): + try: + path.unlink(missing_ok=True) + except OSError: + ok = False + except OSError: + ok = False + return ok diff --git a/api/routes.py b/api/routes.py index 678e267e..ac0d143f 100644 --- a/api/routes.py +++ b/api/routes.py @@ -14666,15 +14666,24 @@ def handle_post(handler, parsed) -> bool: logger.debug("Failed to close workspace terminal for deleted session %s", sid) # Also delete from CLI state.db for CLI sessions shown in sidebar, # but never erase external messaging channel memory via WebUI delete. + state_db_cleanup_failed = False if not is_messaging_session: try: from api.models import delete_cli_session - delete_cli_session(sid) + state_db_cleanup_failed = not delete_cli_session(sid) except Exception: - logger.debug("Failed to delete CLI session %s", sid) + state_db_cleanup_failed = True + logger.warning("Failed to delete CLI session %s", sid, exc_info=True) _publish_session_list_changed("session_delete", profile=event_profile) - return j(handler, {"ok": True, **worktree_retained}) + return j( + handler, + { + "ok": True, + "state_db_cleanup_failed": state_db_cleanup_failed, + **worktree_retained, + }, + ) if parsed.path == "/api/session/clear": try: diff --git a/docs/architecture/agent-api-contract.md b/docs/architecture/agent-api-contract.md index a6f52a95..9dabe79a 100644 --- a/docs/architecture/agent-api-contract.md +++ b/docs/architecture/agent-api-contract.md @@ -41,7 +41,7 @@ helpers are genuinely shared code. | `docker_agent_source_volume` | Compose files and Docker docs expose `hermes-agent-src` and `/opt/hermes` to make the agent checkout visible to WebUI. | Remove the WebUI source mount only after startup install and runtime imports have migrated. This needs Docker/compose follow-up work, not a runtime behavior change in this audit PR. | | `startup_dependency_install` | `api/startup.py` discovers `HERMES_WEBUI_AGENT_DIR` or `$HERMES_HOME/hermes-agent`; `server.py` calls `auto_install_agent_deps()` after import verification fails; `docker_init.bash` installs from the staged agent source. | Replace source-tree pip installs with a packaged hermes-agent WebUI client plus an agent health/version capability contract. Keep `HERMES_WEBUI_AGENT_DIR` during migration as an override/debug path, but it should stop being required in normal multi-container startup. | | `runtime_auxiliary_model_metadata` | `api/streaming.py`, `api/routes.py`, `api/config.py`, and `api/providers.py` import `agent.auxiliary_client`, `agent.model_metadata`, `agent.models_dev`, `hermes_cli.models`, and `agent.account_usage`. | Existing provider/model WebUI endpoints can keep serving UI data where they already wrap agent helpers. Missing surfaces need hermes-agent endpoints or a client package for auxiliary task config, text auxiliary calls, context length, token estimate, provider catalog, and account usage. | -| `runtime_session_state` | `api/streaming.py`, `api/goals.py`, and `api/state_sync.py` import `hermes_state.SessionDB` directly. | Move cross-container state reads and writes behind hermes-agent session/state endpoints. WebUI-only presentation state can remain local, but agent session storage should not be opened from the WebUI container. | +| `runtime_session_state` | `api/streaming.py`, `api/goals.py`, and `api/state_sync.py` import `hermes_state.SessionDB` directly. `api/models.py` also opens the active profile's canonical `state.db` for scoped session deletion because the current canonical helper does not preserve branch/compression evidence ahead of inherited delegate metadata or expose retryable artifact-cleanup semantics. | Move cross-container state reads and writes, including destructive session deletion, behind hermes-agent session/state endpoints once the agent API provides equivalent lineage precedence, transaction, and retry-manifest guarantees. WebUI-only presentation state can remain local, but agent session storage should not be opened from the WebUI container. | | `runtime_gateway_provider` | `api/streaming.py` and `api/routes.py` import `hermes_cli.runtime_provider`; adapter helpers such as `agent.anthropic_adapter` are also imported for gateway normalization. | Provider resolution, runtime routing, and gateway invocation should be hermes-agent API calls. WebUI can keep request validation and display formatting, but it should not import runtime provider internals from the agent checkout. | | `webui_local_or_client_package` | WebUI imports `hermes_cli.auth`, `hermes_cli.config`, `hermes_cli.plugins`, `hermes_cli.profiles`, `hermes_cli.goals`, `agent.skill_utils`, `agent.credential_pool`, and `hermes_constants`. | Pure schemas, constants, and parsing helpers can move into a small versioned client/shared package. Privileged data such as credential pools, auth status, profile mutation, plugin discovery, and goal persistence need hermes-agent endpoints. UI-only formatting can remain in WebUI. | @@ -67,7 +67,10 @@ These surfaces require an agent-owned endpoint because they read agent state, perform provider/runtime decisions, or expose privileged data: - SessionDB/session state read and write operations now using - `hermes_state.SessionDB`. + `hermes_state.SessionDB`, plus the scoped direct-SQL session deletion in + `api/models.py`. The replacement endpoint must preserve branch/compression + precedence and expose retryable post-commit artifact cleanup before that + compatibility path can be removed. - Runtime provider resolution and gateway normalization now using `hermes_cli.runtime_provider` and `agent.anthropic_adapter`. - Auxiliary task execution and configuration now using `agent.auxiliary_client`. diff --git a/static/sessions.js b/static/sessions.js index 48830f4c..45c6e27b 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -4142,6 +4142,7 @@ function _renderBatchActionBar(){ return {response,session:sessionsById.get(sid)||null}; })); const retainedCount=_worktreeResponseCount(results); + const cleanupFailedCount=results.filter(result=>result.response&&result.response.state_db_cleanup_failed).length; ids.forEach(_clearHandoffStorageForSession); if(S.session&&ids.includes(S.session.session_id)){ S.session=null;S.messages=[];S.entries=[];localStorage.removeItem('hermes-webui-session'); @@ -4150,7 +4151,9 @@ function _renderBatchActionBar(){ if(remaining.sessions&&remaining.sessions.length){await loadSession(remaining.sessions[0].session_id);} else{$('msgInner').innerHTML='';$('emptyState').style.display='';} } - showToast((retainedCount?t('session_deleted_worktree'):t('session_delete'))+' ('+ids.length+')');exitSessionSelectMode();await renderSessionList(); + if(cleanupFailedCount) showToast(t('delete_failed')+' ('+cleanupFailedCount+'/'+ids.length+')',0,'error'); + else showToast((retainedCount?t('session_deleted_worktree'):t('session_delete'))+' ('+ids.length+')'); + exitSessionSelectMode();await renderSessionList(); }catch(e){showToast('Delete failed: '+(e.message||e));} };bar.appendChild(deleteBtn); } @@ -8887,6 +8890,7 @@ async function deleteSession(sid, beforeDelete=null){ return false; } const response=deleteResult&&deleteResult.response; + const cleanupFailed=!!(response&&response.state_db_cleanup_failed); if(typeof _clearPersistedSessionQueue==='function') _clearPersistedSessionQueue(sid); if(!optimisticRendered){ _pendingSessionReflowPositions=reflowPositions; @@ -8910,10 +8914,11 @@ async function deleteSession(sid, beforeDelete=null){ if(typeof syncAppTitlebar==='function') syncAppTitlebar(); } } - showToast(_sessionResponseRetainsWorktree(response,session)?t('session_deleted_worktree'):t('session_deleted')); + if(cleanupFailed) showToast(t('delete_failed'),0,'error'); + else showToast(_sessionResponseRetainsWorktree(response,session)?t('session_deleted_worktree'):t('session_deleted')); if(optimisticRendered) void renderSessionList().finally(()=>_optimisticallyRemovedSessionIds.delete(sid)); else await renderSessionList(); - return true; + return !cleanupFailed; } // ── Project helpers ───────────────────────────────────────────────────── diff --git a/tests/test_delete_cli_session_delegate_cascade.py b/tests/test_delete_cli_session_delegate_cascade.py new file mode 100644 index 00000000..057b6bdb --- /dev/null +++ b/tests/test_delete_cli_session_delegate_cascade.py @@ -0,0 +1,1666 @@ +"""Regression coverage for delegated subagent cleanup on CLI-session delete.""" + +import json +import os +import sqlite3 +import subprocess +import sys +import threading +import time +from pathlib import Path + +import pytest + + +def _seed_session( + conn, sid, *, parent_id=None, model_config=None, source=None, + end_reason=None, ended_at=None, started_at="2026-07-11T00:00:00Z" +): + conn.execute( + """ + INSERT INTO sessions ( + id, title, started_at, source, parent_session_id, model_config, + end_reason, ended_at + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?) + """, + ( + sid, + sid, + started_at, + source or ("subagent" if (model_config or {}).get("_delegate_from") else "cli"), + parent_id, + json.dumps(model_config) if model_config else None, + end_reason, + ended_at, + ), + ) + conn.execute( + """ + INSERT INTO messages (session_id, role, content, timestamp) + VALUES (?, 'user', ?, '2026-07-11T00:00:01Z') + """, + (sid, f"message for {sid}"), + ) + + +def _seed_transcript_artifacts(hermes_home, session_ids): + sessions_dir = hermes_home / "sessions" + sessions_dir.mkdir(exist_ok=True) + for sid in session_ids: + (sessions_dir / f"{sid}.json").write_text("{}", encoding="utf-8") + (sessions_dir / f"{sid}.jsonl").write_text("{}\n", encoding="utf-8") + (sessions_dir / f"request_dump_{sid}_1.json").write_text("{}", encoding="utf-8") + return sessions_dir + + +def _remaining_artifact_session_ids(sessions_dir): + names = {path.name for path in sessions_dir.iterdir()} + ids = set() + for name in names: + if name.startswith("request_dump_"): + ids.add(name.removeprefix("request_dump_").rsplit("_", 1)[0]) + else: + ids.add(name.rsplit(".", 1)[0]) + return ids + + +def _assert_all_artifacts_exist(sessions_dir, session_ids): + """Verify each preserved session keeps every supported artifact kind.""" + for sid in session_ids: + assert (sessions_dir / f"{sid}.json").exists() + assert (sessions_dir / f"{sid}.jsonl").exists() + assert (sessions_dir / f"request_dump_{sid}_1.json").exists() + + +@pytest.mark.requires_agent_modules +def test_delete_cli_session_cascades_delegates_but_preserves_branch(tmp_path, monkeypatch): + """Current Hermes removes delegates while preserving all other child kinds.""" + hermes_state = pytest.importorskip("hermes_state") + SessionDB = hermes_state.SessionDB + + state_db = tmp_path / "state.db" + db = SessionDB(db_path=state_db) + db.close() + + conn = sqlite3.connect(state_db) + try: + _seed_session(conn, "parent") + _seed_session( + conn, + "delegate-child", + parent_id="parent", + model_config={"_delegate_from": "parent"}, + ) + _seed_session( + conn, + "delegate-grandchild", + parent_id="delegate-child", + model_config={"_delegate_from": "delegate-child"}, + ) + _seed_session( + conn, + "ordinary-branch", + parent_id="parent", + model_config={"_branched_from": "parent"}, + source="subagent", + ) + _seed_session(conn, "generic-child", parent_id="parent") + _seed_session(conn, "compression-parent", end_reason="compression") + _seed_session( + conn, "compression-child", parent_id="compression-parent", + source="subagent", + model_config={"_delegate_from": "compression-parent"}, + ) + conn.commit() + finally: + conn.close() + + sessions_dir = _seed_transcript_artifacts( + tmp_path, + { + "parent", "delegate-child", "delegate-grandchild", "ordinary-branch", + "generic-child", "compression-parent", "compression-child", + }, + ) + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + import api.profiles + + monkeypatch.setattr(api.profiles, "get_active_hermes_home", lambda: str(tmp_path)) + + from api.models import delete_cli_session + + assert delete_cli_session("parent") is True + assert delete_cli_session("compression-parent") is True + + conn = sqlite3.connect(state_db) + try: + remaining_sessions = { + row[0]: row[1] + for row in conn.execute( + "SELECT id, parent_session_id FROM sessions " + "WHERE id IN (?, ?, ?, ?, ?, ?, ?)", + ( + "parent", "delegate-child", "delegate-grandchild", + "ordinary-branch", "generic-child", "compression-parent", + "compression-child", + ), + ) + } + remaining_messages = { + row[0] + for row in conn.execute( + "SELECT DISTINCT session_id FROM messages " + "WHERE session_id IN (?, ?, ?, ?, ?, ?, ?)", + ( + "parent", "delegate-child", "delegate-grandchild", + "ordinary-branch", "generic-child", "compression-parent", + "compression-child", + ), + ) + } + finally: + conn.close() + + assert remaining_sessions == { + "ordinary-branch": None, + "compression-child": None, + "generic-child": None, + } + assert remaining_messages == set(remaining_sessions) + assert _remaining_artifact_session_ids(sessions_dir) == set(remaining_sessions) + assert delete_cli_session("parent") is True + + +def test_delete_cli_session_cascades_marked_and_legacy_subagents( + tmp_path, monkeypatch +): + """The scoped transaction keeps the delegated cascade invariant.""" + state_db = tmp_path / "state.db" + conn = sqlite3.connect(state_db) + try: + conn.executescript( + """ + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, + title TEXT, + started_at TEXT, + source TEXT, + parent_session_id TEXT, + model_config TEXT, + end_reason TEXT, + ended_at TEXT + ); + CREATE TABLE messages ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + session_id TEXT, + role TEXT, + content TEXT, + timestamp TEXT NOT NULL + ); + """ + ) + _seed_session(conn, "parent") + _seed_session( + conn, "marked-child", parent_id="parent", + model_config={"_delegate_from": "parent"}, + ) + _seed_session(conn, "legacy-child", parent_id="parent", source="subagent") + _seed_session( + conn, "delegate-grandchild", parent_id="legacy-child", + model_config={"_delegate_from": "legacy-child"}, + ) + _seed_session( + conn, "branch-child", parent_id="parent", + model_config={"_branched_from": "parent"}, + source="subagent", + ) + _seed_session(conn, "generic-child", parent_id="parent") + _seed_session(conn, "compression-parent", end_reason="compression") + _seed_session( + conn, "compression-child", parent_id="compression-parent", + source="subagent", + model_config={"_delegate_from": "compression-parent"}, + ) + conn.commit() + finally: + conn.close() + + sessions_dir = _seed_transcript_artifacts( + tmp_path, + { + "parent", "marked-child", "legacy-child", "delegate-grandchild", + "branch-child", "generic-child", "compression-parent", + "compression-child", + }, + ) + import api.profiles + + monkeypatch.setattr(api.profiles, "get_active_hermes_home", lambda: str(tmp_path)) + + from api.models import delete_cli_session + + assert delete_cli_session("parent") is True + assert delete_cli_session("compression-parent") is True + + conn = sqlite3.connect(state_db) + try: + remaining = dict( + conn.execute("SELECT id, parent_session_id FROM sessions ORDER BY id") + ) + remaining_messages = { + row[0] for row in conn.execute("SELECT DISTINCT session_id FROM messages") + } + finally: + conn.close() + + assert remaining == { + "branch-child": None, + "compression-child": None, + "generic-child": None, + } + assert remaining_messages == set(remaining) + assert _remaining_artifact_session_ids(sessions_dir) == set(remaining) + assert delete_cli_session("parent") is True + + +def test_delete_cli_session_preserves_ambiguous_and_inherited_marker_branches( + tmp_path, monkeypatch +): + """Destructive classification must fail closed for branch evidence.""" + state_db = tmp_path / "state.db" + conn = sqlite3.connect(state_db) + try: + conn.executescript( + """ + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, + title TEXT, + started_at TEXT, + source TEXT, + parent_session_id TEXT, + model_config TEXT, + end_reason TEXT, + ended_at TEXT + ); + CREATE TABLE messages ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + session_id TEXT, + role TEXT, + content TEXT, + timestamp TEXT NOT NULL + ); + """ + ) + _seed_session(conn, "delegate-parent") + _seed_session( + conn, + "both-marker-branch", + parent_id="delegate-parent", + source="subagent", + model_config={ + "_delegate_from": "delegate-parent", + "_branched_from": "delegate-parent", + }, + ) + _seed_session( + conn, + "nested-branch-child", + parent_id="both-marker-branch", + source="subagent", + model_config={"_delegate_from": "delegate-parent"}, + ) + _seed_session(conn, "other-lineage") + _seed_session( + conn, + "mismatched-lineage-child", + parent_id="delegate-parent", + source="subagent", + model_config={"_delegate_from": "other-lineage"}, + ) + _seed_session( + conn, "corrupt-config-child", parent_id="delegate-parent", source="subagent" + ) + conn.execute( + "UPDATE sessions SET model_config = ? WHERE id = ?", + ('{"_delegate_from":"other-lineage"', "corrupt-config-child"), + ) + _seed_session( + conn, "scalar-config-child", parent_id="delegate-parent", source="subagent" + ) + conn.execute( + "UPDATE sessions SET model_config = ? WHERE id = ?", + ('"not-a-dict"', "scalar-config-child"), + ) + _seed_session( + conn, + "missing-time-parent", + end_reason="branched", + ended_at=None, + ) + _seed_session( + conn, + "missing-time-branch", + parent_id="missing-time-parent", + source="subagent", + started_at=None, + ) + _seed_session( + conn, + "malformed-time-parent", + end_reason="branched", + ended_at="not-a-timestamp", + ) + _seed_session( + conn, + "malformed-time-branch", + parent_id="malformed-time-parent", + source="subagent", + started_at="also-not-a-timestamp", + ) + _seed_session( + conn, + "nonfinite-time-parent", + end_reason="branched", + ended_at="NaN", + ) + _seed_session( + conn, + "nonfinite-time-branch", + parent_id="nonfinite-time-parent", + source="subagent", + started_at="1783728001.0", + ) + _seed_session( + conn, + "nonfinite-child-parent", + end_reason="branched", + ended_at="1783728000.0", + ) + for suffix, started_at in ( + ("nan", "NaN"), + ("positive-infinity", "Infinity"), + ("negative-infinity", "-Infinity"), + ): + _seed_session( + conn, + f"nonfinite-{suffix}-branch", + parent_id="nonfinite-child-parent", + source="subagent", + started_at=started_at, + ) + _seed_session( + conn, + "naive-time-parent", + end_reason="branched", + ended_at="2026-07-11T00:00:00", + ) + _seed_session( + conn, + "naive-time-branch", + parent_id="naive-time-parent", + source="subagent", + started_at="2026-07-10T23:59:59Z", + ) + _seed_session( + conn, + "parsed-time-parent", + end_reason="branched", + ended_at="1783728000.0", + ) + _seed_session( + conn, + "parsed-time-branch", + parent_id="parsed-time-parent", + source="subagent", + started_at="2026-07-11T00:00:01Z", + ) + _seed_session( + conn, + "parsed-time-delegate", + parent_id="parsed-time-parent", + source="subagent", + started_at="1783727999.0", + ) + conn.commit() + finally: + conn.close() + + preserved = { + "both-marker-branch", + "nested-branch-child", + "other-lineage", + "mismatched-lineage-child", + "corrupt-config-child", + "scalar-config-child", + "missing-time-branch", + "malformed-time-branch", + "nonfinite-time-branch", + "nonfinite-nan-branch", + "nonfinite-positive-infinity-branch", + "nonfinite-negative-infinity-branch", + "naive-time-branch", + "parsed-time-branch", + } + sessions_dir = _seed_transcript_artifacts( + tmp_path, + preserved + | { + "delegate-parent", + "missing-time-parent", + "malformed-time-parent", + "nonfinite-time-parent", + "nonfinite-child-parent", + "naive-time-parent", + "parsed-time-parent", + "parsed-time-delegate", + }, + ) + import api.profiles + + monkeypatch.setattr(api.profiles, "get_active_hermes_home", lambda: str(tmp_path)) + + from api.models import delete_cli_session + + assert delete_cli_session("delegate-parent") is True + assert delete_cli_session("missing-time-parent") is True + assert delete_cli_session("malformed-time-parent") is True + assert delete_cli_session("nonfinite-time-parent") is True + assert delete_cli_session("nonfinite-child-parent") is True + assert delete_cli_session("naive-time-parent") is True + assert delete_cli_session("parsed-time-parent") is True + + conn = sqlite3.connect(state_db) + try: + remaining = dict( + conn.execute("SELECT id, parent_session_id FROM sessions ORDER BY id") + ) + remaining_messages = { + row[0] for row in conn.execute("SELECT DISTINCT session_id FROM messages") + } + finally: + conn.close() + + assert remaining["nested-branch-child"] == "both-marker-branch" + assert { + sid: parent_id + for sid, parent_id in remaining.items() + if sid != "nested-branch-child" + } == {sid: None for sid in preserved - {"nested-branch-child"}} + assert remaining_messages == preserved + assert _remaining_artifact_session_ids(sessions_dir) == preserved + _assert_all_artifacts_exist(sessions_dir, preserved) + + +def test_delete_cli_session_profile_resolution_failure_does_not_guess_fallback( + tmp_path, monkeypatch +): + fallback_home = tmp_path / "fallback-profile" + fallback_home.mkdir() + state_db = fallback_home / "state.db" + conn = sqlite3.connect(state_db) + try: + conn.executescript( + """ + CREATE TABLE sessions (id TEXT PRIMARY KEY); + CREATE TABLE messages (session_id TEXT); + INSERT INTO sessions (id) VALUES ('victim'); + INSERT INTO messages (session_id) VALUES ('victim'); + """ + ) + conn.commit() + finally: + conn.close() + + import api.profiles + + monkeypatch.setenv("HERMES_HOME", str(fallback_home)) + + def _resolution_failure(): + raise RuntimeError("active profile unavailable") + + monkeypatch.setattr(api.profiles, "get_active_hermes_home", _resolution_failure) + + from api.models import delete_cli_session + + assert delete_cli_session("victim") is False + conn = sqlite3.connect(state_db) + try: + assert conn.execute("SELECT id FROM sessions").fetchall() == [("victim",)] + assert conn.execute("SELECT session_id FROM messages").fetchall() == [ + ("victim",) + ] + finally: + conn.close() + + +@pytest.mark.parametrize("sid", ["victim", "_victim", "-victim"]) +def test_delete_cli_session_reports_artifact_cleanup_failure( + tmp_path, monkeypatch, sid +): + state_db = tmp_path / "state.db" + conn = sqlite3.connect(state_db) + try: + conn.executescript( + """ + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, + title TEXT, + started_at TEXT, + source TEXT, + parent_session_id TEXT, + model_config TEXT, + end_reason TEXT, + ended_at TEXT + ); + CREATE TABLE messages ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + session_id TEXT, + role TEXT, + content TEXT, + timestamp TEXT NOT NULL + ); + """ + ) + _seed_session(conn, sid) + conn.commit() + finally: + conn.close() + + sessions_dir = _seed_transcript_artifacts(tmp_path, {sid}) + locked_artifact = sessions_dir / f"{sid}.json" + locked_content = locked_artifact.read_text(encoding="utf-8") + original_unlink = Path.unlink + + def _locked_unlink(path, *args, **kwargs): + if path == locked_artifact: + raise PermissionError("locked") + return original_unlink(path, *args, **kwargs) + + monkeypatch.setattr(Path, "unlink", _locked_unlink) + import api.profiles + + monkeypatch.setattr(api.profiles, "get_active_hermes_home", lambda: str(tmp_path)) + + from api.models import delete_cli_session + + assert delete_cli_session(sid) is False + assert locked_artifact.read_text(encoding="utf-8") == locked_content + + conn = sqlite3.connect(state_db) + try: + assert conn.execute("SELECT id FROM sessions").fetchall() == [] + assert conn.execute("SELECT session_id FROM messages").fetchall() == [] + finally: + conn.close() + + +def test_delete_cli_session_deletes_explicit_migrated_delegate_without_source_tag( + tmp_path, monkeypatch +): + """Explicit lineage is authoritative; source gates only legacy inference.""" + state_db = tmp_path / "state.db" + conn = sqlite3.connect(state_db) + try: + conn.executescript( + """ + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, + title TEXT, + started_at TEXT, + source TEXT, + parent_session_id TEXT, + model_config TEXT, + end_reason TEXT, + ended_at TEXT + ); + CREATE TABLE messages ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + session_id TEXT, + role TEXT, + content TEXT, + timestamp TEXT NOT NULL + ); + """ + ) + _seed_session(conn, "parent") + # Agent-migrated delegates can carry the authoritative marker without + # retaining the historical source='subagent' tag. + _seed_session( + conn, "migrated-delegate", parent_id="parent", + source="cli", + model_config={"_delegate_from": "parent"}, + ) + # Marker-less legacy inference still requires source='subagent'. + _seed_session(conn, "legacy-delegate", parent_id="parent", source="subagent") + _seed_session(conn, "webui-child", parent_id="parent", source="webui") + _seed_session(conn, "cli-child", parent_id="parent", source="cli") + # Branch evidence continues to take precedence over an explicit marker. + _seed_session( + conn, "migrated-branch", parent_id="parent", source="cli", + model_config={ + "_delegate_from": "parent", + "_branched_from": "parent", + }, + ) + conn.commit() + finally: + conn.close() + + sessions_dir = _seed_transcript_artifacts( + tmp_path, + { + "parent", "migrated-delegate", "legacy-delegate", + "webui-child", "cli-child", "migrated-branch", + }, + ) + import api.profiles + + monkeypatch.setattr(api.profiles, "get_active_hermes_home", lambda: str(tmp_path)) + + from api.models import delete_cli_session + + assert delete_cli_session("parent") is True + + conn = sqlite3.connect(state_db) + try: + remaining = dict( + conn.execute("SELECT id, parent_session_id FROM sessions ORDER BY id") + ) + remaining_messages = { + row[0] for row in conn.execute("SELECT DISTINCT session_id FROM messages") + } + finally: + conn.close() + + preserved = {"webui-child", "cli-child", "migrated-branch"} + assert remaining == {sid: None for sid in preserved} + assert remaining_messages == preserved + assert _remaining_artifact_session_ids(sessions_dir) == preserved + _assert_all_artifacts_exist(sessions_dir, preserved) + + +def test_delete_cli_session_serializes_manifest_lifecycle_across_processes( + tmp_path +): + """Two workers must serialize both early recovery and late artifact cleanup.""" + state_db = tmp_path / "state.db" + conn = sqlite3.connect(state_db) + try: + conn.executescript( + """ + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, title TEXT, started_at TEXT, + source TEXT, parent_session_id TEXT, model_config TEXT, + end_reason TEXT, ended_at TEXT + ); + CREATE TABLE messages ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + session_id TEXT, role TEXT, content TEXT, timestamp TEXT NOT NULL + ); + """ + ) + _seed_session(conn, "trigger-a") + _seed_session(conn, "trigger-b") + conn.commit() + finally: + conn.close() + + sessions_dir = _seed_transcript_artifacts( + tmp_path, {"trigger-a", "trigger-b", "orphan-a", "orphan-b"} + ) + (sessions_dir / ".cleanup_manifest_orphan-a.json").write_text( + json.dumps(["orphan-a"]), encoding="utf-8" + ) + (sessions_dir / ".cleanup_manifest_orphan-b.json").write_text( + json.dumps(["orphan-b"]), encoding="utf-8" + ) + + go = tmp_path / "go" + active = tmp_path / "manifest-recovery-active" + violation = tmp_path / "manifest-recovery-overlap" + artifact_active = tmp_path / "artifact-cleanup-active" + artifact_violation = tmp_path / "artifact-cleanup-overlap" + script = r""" +import os +import sys +import time +from pathlib import Path + +from api import models +from api import profiles + +home = Path(sys.argv[1]) +sid = sys.argv[2] +ready = Path(sys.argv[3]) +go = Path(sys.argv[4]) +active = Path(sys.argv[5]) +violation = Path(sys.argv[6]) +artifact_active = Path(sys.argv[7]) +artifact_violation = Path(sys.argv[8]) +artifact_seen = Path(sys.argv[9]) +original = models._process_stale_cleanup_manifests +original_unlink = Path.unlink +profiles.get_active_hermes_home = lambda: home + +def observed_recovery(hermes_home): + owns_marker = False + try: + fd = os.open(active, os.O_CREAT | os.O_EXCL | os.O_WRONLY, 0o600) + except FileExistsError: + violation.write_text("overlap", encoding="utf-8") + else: + os.close(fd) + owns_marker = True + try: + time.sleep(0.5) + return original(hermes_home) + finally: + if owns_marker: + active.unlink(missing_ok=True) + +models._process_stale_cleanup_manifests = observed_recovery + +artifact_phase_entered = False + +def observed_unlink(path, *args, **kwargs): + global artifact_phase_entered + is_current_artifact = ( + path.parent == home / "sessions" + and path.name in {f"{sid}.json", f"{sid}.jsonl"} + ) + if not is_current_artifact or artifact_phase_entered: + return original_unlink(path, *args, **kwargs) + + artifact_phase_entered = True + artifact_seen.write_text("seen", encoding="utf-8") + owns_marker = False + try: + fd = os.open(artifact_active, os.O_CREAT | os.O_EXCL | os.O_WRONLY, 0o600) + except FileExistsError: + artifact_violation.write_text("overlap", encoding="utf-8") + else: + os.close(fd) + owns_marker = True + try: + # Keep the late phase open longer than the serialized recovery probe + # above. If a future refactor releases the process lock after recovery, + # the second worker must reach this window and trip the overlap marker. + time.sleep(2.0) + return original_unlink(path, *args, **kwargs) + finally: + if owns_marker: + original_unlink(artifact_active, missing_ok=True) + +Path.unlink = observed_unlink +ready.write_text("ready", encoding="utf-8") +deadline = time.monotonic() + 15 +while not go.exists(): + if time.monotonic() >= deadline: + raise SystemExit("start barrier timeout") + time.sleep(0.01) +print(f"RESULT={models.delete_cli_session(sid)}", flush=True) +""" + repo_root = Path(__file__).resolve().parents[1] + env = os.environ.copy() + env["HERMES_HOME"] = str(tmp_path) + env["HERMES_WEBUI_ISOLATED_PROFILE"] = "1" + env["PYTHONPATH"] = os.pathsep.join( + part for part in (str(repo_root), env.get("PYTHONPATH", "")) if part + ) + procs = [] + ready_paths = [] + artifact_seen_paths = [] + for suffix in ("a", "b"): + ready = tmp_path / f"ready-{suffix}" + ready_paths.append(ready) + artifact_seen = tmp_path / f"artifact-seen-{suffix}" + artifact_seen_paths.append(artifact_seen) + procs.append( + subprocess.Popen( + [ + sys.executable, "-c", script, str(tmp_path), + f"trigger-{suffix}", str(ready), str(go), str(active), + str(violation), str(artifact_active), str(artifact_violation), + str(artifact_seen), + ], + cwd=repo_root, + env=env, + text=True, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) + ) + + deadline = time.monotonic() + 15 + while not all(path.exists() for path in ready_paths): + if time.monotonic() >= deadline: + for proc in procs: + proc.kill() + raise AssertionError("worker readiness timeout") + time.sleep(0.01) + go.write_text("go", encoding="utf-8") + + outputs = [proc.communicate(timeout=20) for proc in procs] + for proc, (stdout, stderr) in zip(procs, outputs, strict=False): + assert proc.returncode == 0, (stdout, stderr) + assert "RESULT=True" in stdout, (stdout, stderr) + assert not violation.exists(), "stale-manifest recovery overlapped across processes" + assert all(path.exists() for path in artifact_seen_paths), ( + "both workers must reach post-commit artifact cleanup" + ) + assert not artifact_violation.exists(), ( + "post-commit artifact cleanup overlapped across processes" + ) + assert not list(sessions_dir.glob(".cleanup_manifest_*.json")) + for sid in ("trigger-a", "trigger-b", "orphan-a", "orphan-b"): + assert not (sessions_dir / f"{sid}.json").exists() + assert not (sessions_dir / f"{sid}.jsonl").exists() + + +def test_cleanup_manifest_process_lock_uses_windows_byte_lock( + tmp_path, monkeypatch +): + """The native-Windows fallback locks and unlocks one persistent byte.""" + import api.models as models + + calls = [] + + class FakeMsvcrt: + LK_LOCK = 1 + LK_UNLCK = 2 + + @staticmethod + def locking(fd, mode, nbytes): + calls.append((fd, mode, nbytes)) + + monkeypatch.setattr(models, "_fcntl", None) + monkeypatch.setattr(models, "_msvcrt", FakeMsvcrt) + + with pytest.raises(RuntimeError, match="probe"): + with models._cleanup_manifest_process_lock(tmp_path): + assert [call[1:] for call in calls] == [(FakeMsvcrt.LK_LOCK, 1)] + raise RuntimeError("probe") + + assert [call[1:] for call in calls] == [ + (FakeMsvcrt.LK_LOCK, 1), + (FakeMsvcrt.LK_UNLCK, 1), + ] + lock_path = tmp_path / ".session_cleanup.lock" + assert lock_path.read_bytes() == b"\0" + + +def test_delete_cli_session_thread_lock_does_not_block_different_profiles( + tmp_path, monkeypatch +): + """A stalled cleanup in one profile must not globally block another.""" + homes = { + "profile-a": tmp_path / "profile-a", + "profile-b": tmp_path / "profile-b", + } + for name, home in homes.items(): + home.mkdir() + conn = sqlite3.connect(home / "state.db") + try: + conn.executescript( + """ + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, title TEXT, started_at TEXT, + source TEXT, parent_session_id TEXT, model_config TEXT, + end_reason TEXT, ended_at TEXT + ); + CREATE TABLE messages ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + session_id TEXT, role TEXT, content TEXT, timestamp TEXT NOT NULL + ); + """ + ) + _seed_session(conn, name) + conn.commit() + finally: + conn.close() + _seed_transcript_artifacts(home, {name}) + + import api.models as models + import api.profiles as profiles + + monkeypatch.setattr( + profiles, + "get_active_hermes_home", + lambda: homes[threading.current_thread().name], + ) + original_recovery = models._process_stale_cleanup_manifests + profile_a_entered = threading.Event() + profile_b_entered = threading.Event() + release_profile_a = threading.Event() + + def observed_recovery(hermes_home): + if Path(hermes_home) == homes["profile-a"]: + profile_a_entered.set() + if not release_profile_a.wait(timeout=10): + raise TimeoutError("profile-a release timeout") + else: + profile_b_entered.set() + return original_recovery(hermes_home) + + monkeypatch.setattr(models, "_process_stale_cleanup_manifests", observed_recovery) + results = {} + + def delete_for_profile(name): + results[name] = models.delete_cli_session(name) + + thread_a = threading.Thread(target=delete_for_profile, args=("profile-a",), name="profile-a") + thread_b = threading.Thread(target=delete_for_profile, args=("profile-b",), name="profile-b") + thread_a.start() + assert profile_a_entered.wait(timeout=10) + thread_b.start() + try: + assert profile_b_entered.wait(timeout=3), ( + "profile-b was globally blocked by profile-a's in-process lock" + ) + thread_b.join(timeout=10) + assert not thread_b.is_alive() + assert results.get("profile-b") is True + finally: + release_profile_a.set() + thread_a.join(timeout=10) + thread_b.join(timeout=10) + + assert not thread_a.is_alive() + assert not thread_b.is_alive() + assert results == {"profile-a": True, "profile-b": True} + + +def test_delete_cli_session_cleans_referential_tables(tmp_path, monkeypatch): + """Deleting a session must also remove rows from session_model_usage, + compression_locks, and telegram_dm_topic_bindings — not just + sessions and messages. + """ + state_db = tmp_path / "state.db" + conn = sqlite3.connect(state_db) + try: + conn.executescript( + """ + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, + title TEXT, + started_at TEXT, + source TEXT, + parent_session_id TEXT, + model_config TEXT, + end_reason TEXT, + ended_at TEXT + ); + CREATE TABLE messages ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + session_id TEXT, + role TEXT, + content TEXT, + timestamp TEXT NOT NULL + ); + CREATE TABLE session_model_usage ( + session_id TEXT NOT NULL, + model TEXT NOT NULL, + input_tokens INTEGER NOT NULL DEFAULT 0, + output_tokens INTEGER NOT NULL DEFAULT 0, + PRIMARY KEY (session_id, model) + ); + CREATE TABLE compression_locks ( + session_id TEXT NOT NULL, + holder TEXT NOT NULL, + acquired_at REAL NOT NULL, + expires_at REAL NOT NULL + ); + CREATE TABLE telegram_dm_topic_bindings ( + chat_id TEXT NOT NULL, + thread_id TEXT NOT NULL, + user_id TEXT NOT NULL, + session_key TEXT NOT NULL, + session_id TEXT NOT NULL, + linked_at REAL NOT NULL, + updated_at REAL NOT NULL, + PRIMARY KEY (chat_id, thread_id) + ); + """ + ) + _seed_session(conn, "parent") + _seed_session( + conn, "delegate", parent_id="parent", + source="subagent", + model_config={"_delegate_from": "parent"}, + ) + # Populate referential tables for both parent and delegate. + conn.execute( + "INSERT INTO session_model_usage (session_id, model, input_tokens, output_tokens) " + "VALUES ('parent', 'test-model', 100, 50)" + ) + conn.execute( + "INSERT INTO session_model_usage (session_id, model, input_tokens, output_tokens) " + "VALUES ('delegate', 'test-model', 200, 75)" + ) + conn.execute( + "INSERT INTO compression_locks (session_id, holder, acquired_at, expires_at) " + "VALUES ('parent', 'compressor', 1000.0, 2000.0)" + ) + conn.execute( + "INSERT INTO compression_locks (session_id, holder, acquired_at, expires_at) " + "VALUES ('delegate', 'compressor', 1000.0, 2000.0)" + ) + conn.execute( + "INSERT INTO telegram_dm_topic_bindings " + "(chat_id, thread_id, user_id, session_key, session_id, linked_at, updated_at) " + "VALUES ('chat1', 'thread1', 'user1', 'key1', 'parent', 1000.0, 1000.0)" + ) + conn.execute( + "INSERT INTO telegram_dm_topic_bindings " + "(chat_id, thread_id, user_id, session_key, session_id, linked_at, updated_at) " + "VALUES ('chat2', 'thread2', 'user1', 'key2', 'delegate', 1000.0, 1000.0)" + ) + # An unrelated session's rows must survive. + _seed_session(conn, "bystander") + conn.execute( + "INSERT INTO session_model_usage (session_id, model, input_tokens, output_tokens) " + "VALUES ('bystander', 'test-model', 10, 5)" + ) + conn.execute( + "INSERT INTO compression_locks (session_id, holder, acquired_at, expires_at) " + "VALUES ('bystander', 'compressor', 1000.0, 2000.0)" + ) + conn.execute( + "INSERT INTO telegram_dm_topic_bindings " + "(chat_id, thread_id, user_id, session_key, session_id, linked_at, updated_at) " + "VALUES ('chat3', 'thread3', 'user1', 'key3', 'bystander', 1000.0, 1000.0)" + ) + conn.commit() + finally: + conn.close() + + _seed_transcript_artifacts(tmp_path, {"parent", "delegate", "bystander"}) + import api.profiles + + monkeypatch.setattr(api.profiles, "get_active_hermes_home", lambda: str(tmp_path)) + + from api.models import delete_cli_session + + assert delete_cli_session("parent") is True + + conn = sqlite3.connect(state_db) + try: + # session_model_usage: only bystander survives. + smu = [ + row[0] + for row in conn.execute( + "SELECT session_id FROM session_model_usage ORDER BY session_id" + ) + ] + assert smu == ["bystander"] + + # compression_locks: only bystander survives. + cl = [ + row[0] + for row in conn.execute( + "SELECT session_id FROM compression_locks ORDER BY session_id" + ) + ] + assert cl == ["bystander"] + + # telegram_dm_topic_bindings: only bystander survives. + tdtb = [ + row[0] + for row in conn.execute( + "SELECT session_id FROM telegram_dm_topic_bindings ORDER BY session_id" + ) + ] + assert tdtb == ["bystander"] + finally: + conn.close() + + +def test_delete_cli_session_artifact_cleanup_is_idempotent(tmp_path, monkeypatch): + """When artifact cleanup partially fails, the manifest retains the + pending ID so a second call retries it. A second call that succeeds + must return True and leave no manifest behind. + """ + state_db = tmp_path / "state.db" + conn = sqlite3.connect(state_db) + try: + conn.executescript( + """ + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, + title TEXT, + started_at TEXT, + source TEXT, + parent_session_id TEXT, + model_config TEXT, + end_reason TEXT, + ended_at TEXT + ); + CREATE TABLE messages ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + session_id TEXT, + role TEXT, + content TEXT, + timestamp TEXT NOT NULL + ); + """ + ) + _seed_session(conn, "victim") + conn.commit() + finally: + conn.close() + + sessions_dir = _seed_transcript_artifacts(tmp_path, {"victim"}) + locked_artifact = sessions_dir / "victim.json" + locked_content = locked_artifact.read_text(encoding="utf-8") + original_unlink = Path.unlink + + call_count = {"n": 0} + + def _locked_once_unlink(path, *args, **kwargs): + if path == locked_artifact: + call_count["n"] += 1 + if call_count["n"] == 1: + raise PermissionError("locked on first attempt") + return original_unlink(path, *args, **kwargs) + + monkeypatch.setattr(Path, "unlink", _locked_once_unlink) + import api.profiles + + monkeypatch.setattr(api.profiles, "get_active_hermes_home", lambda: str(tmp_path)) + + from api.models import delete_cli_session + + # First call: DB row is deleted but artifact cleanup fails → False. + assert delete_cli_session("victim") is False + assert locked_artifact.read_text(encoding="utf-8") == locked_content + + # Manifest must still exist with "victim" pending. + manifests = sorted(sessions_dir.glob(".cleanup_manifest_*.json")) + assert len(manifests) == 1 + assert json.loads(manifests[0].read_text(encoding="utf-8")) == ["victim"] + + # DB row is already gone; second call returns True (idempotent — + # absence is success) and retries the artifact. + assert delete_cli_session("victim") is True + assert not locked_artifact.exists() + assert not list(sessions_dir.glob(".cleanup_manifest_*.json")) + + +def test_delete_cli_session_stale_manifest_preserves_live_session(tmp_path, monkeypatch): + """A stale manifest from a failed commit must not unlink artifacts + for a session that still exists in the DB. + """ + state_db = tmp_path / "state.db" + conn = sqlite3.connect(state_db) + try: + conn.executescript( + """ + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, title TEXT, started_at TEXT, + source TEXT, parent_session_id TEXT, model_config TEXT, + end_reason TEXT, ended_at TEXT + ); + CREATE TABLE messages ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + session_id TEXT, role TEXT, content TEXT, timestamp TEXT NOT NULL + ); + """ + ) + _seed_session(conn, "live-one") + _seed_session(conn, "live-two") + conn.commit() + finally: + conn.close() + + sessions_dir = _seed_transcript_artifacts(tmp_path, {"live-one", "live-two"}) + + # Write a stale manifest claiming "live-one" artifacts should be + # removed, simulating a previous delete_cli_session whose commit + # failed after the manifest was written. + stale_manifest = sessions_dir / ".cleanup_manifest_stale-sim.json" + stale_manifest.write_text( + json.dumps(["live-one"]), encoding="utf-8" + ) + + import api.profiles + monkeypatch.setattr( + api.profiles, "get_active_hermes_home", lambda: str(tmp_path) + ) + from api.models import delete_cli_session + + # Delete a session NOT in the stale manifest. + assert delete_cli_session("live-two") is True + + # _process_stale_cleanup_manifests ran at entry and should have + # skipped "live-one" because it's still alive. + assert (sessions_dir / "live-one.json").exists() + assert (sessions_dir / "live-one.jsonl").exists() + assert (sessions_dir / "request_dump_live-one_1.json").exists() + + # "live-two" artifacts should be gone. + assert not (sessions_dir / "live-two.json").exists() + assert not (sessions_dir / "live-two.jsonl").exists() + + # The stale manifest was processed and dropped (live-one is alive, + # so the manifest was identified as stale from a failed commit and + # silently removed). No manifests should linger. + assert not list(sessions_dir.glob(".cleanup_manifest_*.json")) + + +def test_delete_cli_session_concurrent_unique_manifests(tmp_path, monkeypatch): + """Each delete call writes its own unique manifest file so concurrent + deletes never clobber each other's retry records. + """ + state_db = tmp_path / "state.db" + conn = sqlite3.connect(state_db) + try: + conn.executescript( + """ + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, title TEXT, started_at TEXT, + source TEXT, parent_session_id TEXT, model_config TEXT, + end_reason TEXT, ended_at TEXT + ); + CREATE TABLE messages ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + session_id TEXT, role TEXT, content TEXT, timestamp TEXT NOT NULL + ); + """ + ) + _seed_session(conn, "parent-a") + _seed_session( + conn, "child-a", parent_id="parent-a", source="subagent", + model_config={"_delegate_from": "parent-a"}, + ) + _seed_session(conn, "parent-b") + _seed_session( + conn, "child-b", parent_id="parent-b", source="subagent", + model_config={"_delegate_from": "parent-b"}, + ) + conn.commit() + finally: + conn.close() + + sessions_dir = _seed_transcript_artifacts( + tmp_path, {"parent-a", "child-a", "parent-b", "child-b"} + ) + + import api.profiles + monkeypatch.setattr( + api.profiles, "get_active_hermes_home", lambda: str(tmp_path) + ) + from api.models import delete_cli_session + + # True concurrent delete: two threads synchronised by a Barrier so + # both enter the manifest critical section in parallel. The lock + # serialises access; both must still complete successfully. + import threading as _thr + + barrier = _thr.Barrier(2) + results = [] + + def _del_a(): + barrier.wait() + results.append(delete_cli_session("parent-a")) + + def _del_b(): + barrier.wait() + results.append(delete_cli_session("parent-b")) + + t_a = _thr.Thread(target=_del_a) + t_b = _thr.Thread(target=_del_b) + t_a.start() + t_b.start() + t_a.join() + t_b.join() + + assert results == [True, True], f"Both deletes must succeed: {results}" + + # No manifests linger; all artifacts cleaned for every deleted session. + assert not list(sessions_dir.glob(".cleanup_manifest_*.json")) + for sid in ("parent-a", "child-a", "parent-b", "child-b"): + assert not (sessions_dir / f"{sid}.json").exists() + assert not (sessions_dir / f"{sid}.jsonl").exists() + + +def test_delete_cli_session_releases_manifest_lock_after_unlink_error( + tmp_path, monkeypatch +): + """A stale-manifest exception must not deadlock every later delete.""" + state_db = tmp_path / "state.db" + conn = sqlite3.connect(state_db) + try: + conn.executescript( + """ + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, title TEXT, started_at TEXT, + source TEXT, parent_session_id TEXT, model_config TEXT, + end_reason TEXT, ended_at TEXT + ); + CREATE TABLE messages ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + session_id TEXT, role TEXT, content TEXT, timestamp TEXT NOT NULL + ); + """ + ) + _seed_session(conn, "victim") + conn.commit() + finally: + conn.close() + + sessions_dir = _seed_transcript_artifacts(tmp_path, {"victim"}) + empty_manifest = sessions_dir / ".cleanup_manifest_empty.json" + empty_manifest.write_text("[]", encoding="utf-8") + + import api.profiles + import api.models as models + + monkeypatch.setattr( + api.profiles, "get_active_hermes_home", lambda: str(tmp_path) + ) + original_unlink = Path.unlink + + def _raise_for_manifest(path, *args, **kwargs): + if path == empty_manifest: + raise PermissionError("manifest is locked") + return original_unlink(path, *args, **kwargs) + + monkeypatch.setattr(Path, "unlink", _raise_for_manifest) + assert models.delete_cli_session("victim") is False + + # The context manager must release this profile's lock even though stale + # manifest cleanup raised before the DB transaction began. + manifest_lock = models._cleanup_manifest_thread_lock(tmp_path) + assert manifest_lock.acquire(blocking=False) is True + manifest_lock.release() + + # Restore unlink behavior and prove a complete later delete can enter the + # same critical section and finish, rather than merely acquiring the lock. + monkeypatch.setattr(Path, "unlink", original_unlink) + empty_manifest.unlink() + assert models.delete_cli_session("victim") is True + assert not (sessions_dir / "victim.json").exists() + + +def test_delete_cli_session_missing_db_preserves_manifested_live_artifacts( + tmp_path, monkeypatch +): + """A temporarily missing state.db is unknown, never proof of deletion.""" + state_db = tmp_path / "state.db" + conn = sqlite3.connect(state_db) + try: + conn.executescript( + """ + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, title TEXT, started_at TEXT, + source TEXT, parent_session_id TEXT, model_config TEXT, + end_reason TEXT, ended_at TEXT + ); + CREATE TABLE messages ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + session_id TEXT, role TEXT, content TEXT, timestamp TEXT NOT NULL + ); + """ + ) + _seed_session(conn, "live-session") + conn.commit() + finally: + conn.close() + + sessions_dir = _seed_transcript_artifacts(tmp_path, {"live-session"}) + manifest = sessions_dir / ".cleanup_manifest_missing-db.json" + manifest.write_text(json.dumps(["live-session"]), encoding="utf-8") + hidden_db = tmp_path / "state.db.temporarily-hidden" + state_db.rename(hidden_db) + + import api.profiles + from api.models import delete_cli_session + + monkeypatch.setattr( + api.profiles, "get_active_hermes_home", lambda: str(tmp_path) + ) + assert delete_cli_session("unrelated") is False + + # Every artifact and the retry record survive while liveness is unknown. + assert (sessions_dir / "live-session.json").exists() + assert (sessions_dir / "live-session.jsonl").exists() + assert (sessions_dir / "request_dump_live-session_1.json").exists() + assert manifest.exists() + + hidden_db.rename(state_db) + conn = sqlite3.connect(state_db) + try: + assert conn.execute( + "SELECT 1 FROM sessions WHERE id = ?", ("live-session",) + ).fetchone() == (1,) + finally: + conn.close() + + +def test_delete_cli_session_unqueryable_db_preserves_manifested_live_artifacts( + tmp_path, monkeypatch +): + """A failed liveness query preserves every artifact and reports failure.""" + state_db = tmp_path / "state.db" + conn = sqlite3.connect(state_db) + try: + conn.executescript( + """ + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, title TEXT, started_at TEXT, + source TEXT, parent_session_id TEXT, model_config TEXT, + end_reason TEXT, ended_at TEXT + ); + CREATE TABLE messages ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + session_id TEXT, role TEXT, content TEXT, timestamp TEXT NOT NULL + ); + """ + ) + _seed_session(conn, "live-session") + conn.commit() + finally: + conn.close() + + sessions_dir = _seed_transcript_artifacts(tmp_path, {"live-session"}) + manifest = sessions_dir / ".cleanup_manifest_unqueryable-db.json" + manifest.write_text(json.dumps(["live-session"]), encoding="utf-8") + + import api.profiles + import api.models as models + + monkeypatch.setattr( + api.profiles, "get_active_hermes_home", lambda: str(tmp_path) + ) + original_connect = sqlite3.connect + + def _fail_readonly_connect(database, *args, **kwargs): + if kwargs.get("uri") is True and "mode=ro" in str(database): + raise sqlite3.OperationalError("state.db is temporarily unreadable") + return original_connect(database, *args, **kwargs) + + monkeypatch.setattr(sqlite3, "connect", _fail_readonly_connect) + assert models.delete_cli_session("unrelated") is False + + assert manifest.exists() + _assert_all_artifacts_exist(sessions_dir, {"live-session"}) + conn = original_connect(state_db) + try: + assert conn.execute( + "SELECT 1 FROM sessions WHERE id = ?", ("live-session",) + ).fetchone() == (1,) + assert conn.execute( + "SELECT 1 FROM messages WHERE session_id = ?", ("live-session",) + ).fetchone() == (1,) + finally: + conn.close() + + +def test_delete_cli_session_uses_delegate_lineage_parent_for_compression( + tmp_path, monkeypatch +): + """Authoritative delegate lineage preserves a compression continuation.""" + state_db = tmp_path / "state.db" + conn = sqlite3.connect(state_db) + try: + conn.executescript( + """ + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, title TEXT, started_at TEXT, + source TEXT, parent_session_id TEXT, model_config TEXT, + end_reason TEXT, ended_at TEXT + ); + CREATE TABLE messages ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + session_id TEXT, role TEXT, content TEXT, timestamp TEXT NOT NULL + ); + """ + ) + _seed_session(conn, "compression-parent", end_reason="compression") + _seed_session(conn, "physical-parent") + _seed_session( + conn, + "continuation", + parent_id="physical-parent", + source="subagent", + model_config={"_delegate_from": "compression-parent"}, + ) + conn.commit() + finally: + conn.close() + + sessions_dir = _seed_transcript_artifacts( + tmp_path, {"compression-parent", "physical-parent", "continuation"} + ) + + import api.profiles + from api.models import delete_cli_session + + monkeypatch.setattr( + api.profiles, "get_active_hermes_home", lambda: str(tmp_path) + ) + assert delete_cli_session("compression-parent") is True + + conn = sqlite3.connect(state_db) + try: + assert conn.execute( + "SELECT parent_session_id FROM sessions WHERE id = ?", + ("continuation",), + ).fetchone() == ("physical-parent",) + assert conn.execute( + "SELECT 1 FROM messages WHERE session_id = ?", ("continuation",) + ).fetchone() == (1,) + finally: + conn.close() + _assert_all_artifacts_exist(sessions_dir, {"continuation"}) + + +def test_delete_cli_session_manifest_publish_failure_rolls_back( + tmp_path, monkeypatch +): + """A missing retry record must prevent the DB delete from committing.""" + state_db = tmp_path / "state.db" + conn = sqlite3.connect(state_db) + try: + conn.executescript( + """ + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, title TEXT, started_at TEXT, + source TEXT, parent_session_id TEXT, model_config TEXT, + end_reason TEXT, ended_at TEXT + ); + CREATE TABLE messages ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + session_id TEXT, role TEXT, content TEXT, timestamp TEXT NOT NULL + ); + """ + ) + _seed_session(conn, "victim") + conn.commit() + finally: + conn.close() + + sessions_dir = _seed_transcript_artifacts(tmp_path, {"victim"}) + + import api.profiles + from api.models import delete_cli_session + + monkeypatch.setattr( + api.profiles, "get_active_hermes_home", lambda: str(tmp_path) + ) + original_write_text = Path.write_text + + def _deny_manifest_publish(path, *args, **kwargs): + if path.name.startswith(".cleanup_manifest_") and path.suffix == ".tmp": + raise PermissionError("manifest directory is read-only") + return original_write_text(path, *args, **kwargs) + + monkeypatch.setattr(Path, "write_text", _deny_manifest_publish) + assert delete_cli_session("victim") is False + + conn = sqlite3.connect(state_db) + try: + assert conn.execute( + "SELECT 1 FROM sessions WHERE id = ?", ("victim",) + ).fetchone() == (1,) + assert conn.execute( + "SELECT 1 FROM messages WHERE session_id = ?", ("victim",) + ).fetchone() == (1,) + finally: + conn.close() + _assert_all_artifacts_exist(sessions_dir, {"victim"}) + assert not list(sessions_dir.glob(".cleanup_manifest_*.json")) + assert not list(sessions_dir.glob(".cleanup_manifest_*.tmp")) + + +@pytest.mark.parametrize("payload", ['["orphan"', '{"orphan": true}', '[123]']) +def test_delete_cli_session_preserves_malformed_manifest_and_reports_failure( + tmp_path, monkeypatch, payload +): + """Unknown pending IDs remain recoverable and make cleanup incomplete.""" + state_db = tmp_path / "state.db" + conn = sqlite3.connect(state_db) + try: + conn.executescript( + """ + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, title TEXT, started_at TEXT, + source TEXT, parent_session_id TEXT, model_config TEXT, + end_reason TEXT, ended_at TEXT + ); + CREATE TABLE messages ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + session_id TEXT, role TEXT, content TEXT, timestamp TEXT NOT NULL + ); + """ + ) + _seed_session(conn, "victim") + conn.commit() + finally: + conn.close() + + sessions_dir = _seed_transcript_artifacts(tmp_path, {"victim", "orphan"}) + malformed = sessions_dir / ".cleanup_manifest_truncated.json" + malformed.write_text(payload, encoding="utf-8") + + import api.profiles + from api.models import delete_cli_session + + monkeypatch.setattr( + api.profiles, "get_active_hermes_home", lambda: str(tmp_path) + ) + assert delete_cli_session("victim") is False + + assert malformed.exists() + assert malformed.read_text(encoding="utf-8") == payload + _assert_all_artifacts_exist(sessions_dir, {"orphan"}) + conn = sqlite3.connect(state_db) + try: + assert conn.execute( + "SELECT 1 FROM sessions WHERE id = ?", ("victim",) + ).fetchone() is None + assert conn.execute( + "SELECT 1 FROM messages WHERE session_id = ?", ("victim",) + ).fetchone() is None + finally: + conn.close() diff --git a/tests/test_issue1494_state_db_fd_leak.py b/tests/test_issue1494_state_db_fd_leak.py index d7cdbdbf..1691e43d 100644 --- a/tests/test_issue1494_state_db_fd_leak.py +++ b/tests/test_issue1494_state_db_fd_leak.py @@ -204,7 +204,7 @@ def test_get_cli_session_messages_closes_connection(tmp_path, tracking_sqlite, m def test_delete_cli_session_closes_connection(tmp_path, tracking_sqlite, monkeypatch): - """`delete_cli_session` must close its sqlite connection (also keeps explicit commit working).""" + """The scoped delete transaction must close its sqlite connection.""" db = tmp_path / "state.db" _make_state_db(db) @@ -217,9 +217,9 @@ def test_delete_cli_session_closes_connection(tmp_path, tracking_sqlite, monkeyp deleted = delete_cli_session("s1") assert deleted is True, "delete_cli_session should report the row was removed" - # Second call: row gone, nothing to delete — connection must still close cleanly. + # Second call is idempotent; its connection must still close cleanly. deleted_again = delete_cli_session("s1") - assert deleted_again is False + assert deleted_again is True _assert_all_closed(tracking_sqlite, "delete_cli_session") # First call commits; second call short-circuits but still opens+closes a connection. diff --git a/tests/test_issue2057_worktree_lifecycle.py b/tests/test_issue2057_worktree_lifecycle.py index ef718eea..638089c5 100644 --- a/tests/test_issue2057_worktree_lifecycle.py +++ b/tests/test_issue2057_worktree_lifecycle.py @@ -95,12 +95,13 @@ def test_delete_worktree_session_reports_retained_worktree_without_cleanup(tmp_p captured = _capture_post(monkeypatch, {"session_id": session.session_id}) monkeypatch.setattr(routes, "_lookup_cli_session_metadata", lambda sid: {}) monkeypatch.setattr(routes, "_is_messaging_session_id", lambda sid: False) - monkeypatch.setattr(models, "delete_cli_session", lambda sid: None) + monkeypatch.setattr(models, "delete_cli_session", lambda sid: True) assert routes.handle_post(object(), SimpleNamespace(path="/api/session/delete")) is True assert captured["status"] == 200 assert captured["payload"]["ok"] is True + assert captured["payload"]["state_db_cleanup_failed"] is False assert captured["payload"]["worktree_retained"] is True assert captured["payload"]["worktree_path"] == str(worktree.resolve()) assert captured["payload"]["worktree_branch"] == "hermes/wtdelete1" @@ -139,6 +140,7 @@ def test_delete_session_records_tombstone_when_state_db_delete_fails(tmp_path, m assert captured["status"] == 200 assert captured["payload"]["ok"] is True + assert captured["payload"]["state_db_cleanup_failed"] is True assert not (session_dir / f"{sid}.json").exists() assert sid in models._load_webui_deleted_session_tombstone() @@ -170,6 +172,7 @@ def test_delete_messaging_session_reopens_read_only_without_deleted_webui_tombst assert captured["status"] == 200 assert captured["payload"]["ok"] is True + assert captured["payload"]["state_db_cleanup_failed"] is False assert not (session_dir / f"{sid}.json").exists() assert sid not in models._load_webui_deleted_session_tombstone() assert delete_calls == [] diff --git a/tests/test_issue2057_worktree_ui_static.py b/tests/test_issue2057_worktree_ui_static.py index 3b99921c..8b65ce51 100644 --- a/tests/test_issue2057_worktree_ui_static.py +++ b/tests/test_issue2057_worktree_ui_static.py @@ -57,8 +57,13 @@ def test_archive_delete_success_copy_prefers_response_worktree_retained(): assert "session.archived?_sessionArchiveToast(response,session):t('session_restored')" in src assert "_sessionResponseRetainsWorktree(response,session)?t('session_deleted_worktree')" in src assert "const retainedCount=_worktreeResponseCount(results)" in src + assert "const cleanupFailedCount=results.filter(result=>result.response&&result.response.state_db_cleanup_failed).length;" in src + assert "if(cleanupFailedCount) showToast(t('delete_failed')+' ('+cleanupFailedCount+'/'+ids.length+')',0,'error');" in src assert "showToast(retainedCount?t('session_archived_worktree'):t('session_archived'))" in src assert "showToast((retainedCount?t('session_deleted_worktree'):t('session_delete'))" in src + assert "const cleanupFailed=!!(response&&response.state_db_cleanup_failed);" in src + assert "if(cleanupFailed) showToast(t('delete_failed'),0,'error');" in src + assert "return !cleanupFailed;" in src def test_worktree_archive_delete_api_responses_are_explicit(): @@ -66,7 +71,9 @@ def test_worktree_archive_delete_api_responses_are_explicit(): assert "def _worktree_retained_payload(session)" in src assert "def _worktree_retained_payload_for_session_id(sid: str)" in src assert '"worktree_retained": True' in src - assert '{"ok": True, **worktree_retained}' in src + assert '"state_db_cleanup_failed": state_db_cleanup_failed' in src + assert '"ok": True,' in src + assert "**worktree_retained," in src assert '{"ok": True, "session": s.compact(), **_worktree_retained_payload(s)}' in src diff --git a/tests/test_state_db_readonly_reads_models.py b/tests/test_state_db_readonly_reads_models.py index c4d43eee..735df0ed 100644 --- a/tests/test_state_db_readonly_reads_models.py +++ b/tests/test_state_db_readonly_reads_models.py @@ -260,7 +260,12 @@ def test_delete_cli_session_still_opens_writable_connection(): read-only downgrade would make the delete silently no-op.""" import inspect - src = inspect.getsource(models.delete_cli_session) + src = "\n".join( + ( + inspect.getsource(models.delete_cli_session), + inspect.getsource(models._delete_cli_session_locked), + ) + ) assert "sqlite3.connect(str(db_path))" in src, ( "delete_cli_session must keep its writable connection" )