fix(sessions): refuse to delete a session row a live turn still owns (#123583)
Refactor entry-side deletion refusal to execute in-transaction via `_write_guards_reject(conn, sid)` (#123583), per maintainer review: - Underlying `delete_session` and `delete_sessions` now accept an opt-in kwarg `exclude_active_write_guards=True` running inside `_do` write transaction, eliminating the race condition where a turn acquires the lease between check and delete. - Raises `SessionActiveWriteGuardError` when refusing single delete, leaving the row untouched; `delete_sessions` atomically skips active rows. - Checks both active turn leases and compression locks via the existing reclaim-aware `_write_guards_reject` helper. - Covers all user-facing delete sinks: * Web `DELETE /api/sessions/{id}` -> 409 Conflict * Web `POST /api/sessions/bulk-delete` -> skips active rows * Web / CLI `prune` -> passes `exclude_active_write_guards=True` so lineage parents of active conversations are not pruned * API Server `DELETE /api/sessions/{id}` -> 409 session_active_turn * CLI `hermes sessions delete` & `export --delete-after-verified` -> exits 1 * CLI browse picker -> refuses active delete * TUI Gateway `session.delete` -> 4023 error - Conforms to rubric with 2 targeted invariant tests in `tests/hermes_state/test_delete_session_write_guards.py`. - Updates user guide and web dashboard docs for 409 / exit 1. (cherry picked from commit 2c037a7a79dc211b49bacc72e3140951ccf900cf)
This commit is contained in:
@@ -3216,7 +3216,12 @@ class APIServerAdapter(OpenAICompatRoutesMixin, BasePlatformAdapter):
|
||||
sessions_dir = Path(get_hermes_home()) / "sessions"
|
||||
except Exception:
|
||||
logger.debug("sessions dir unavailable for delete of %s", session_id, exc_info=True)
|
||||
deleted = await asyncio.to_thread(db.delete_session, session_id, sessions_dir=sessions_dir)
|
||||
from hermes_state_errors import SessionActiveWriteGuardError
|
||||
try:
|
||||
deleted = await asyncio.to_thread(
|
||||
db.delete_session, session_id, sessions_dir=sessions_dir, exclude_active_write_guards=True)
|
||||
except SessionActiveWriteGuardError as exc:
|
||||
return _error_response(str(exc), 409, code="session_active_turn")
|
||||
if deleted:
|
||||
# A hard delete must also drop the gateway's durable channel→session routing entries
|
||||
# for the id, or the next inbound message resolves the SAME id and run_agent's
|
||||
|
||||
@@ -562,13 +562,18 @@ def _export_markdown_single(db, args, export_one, output_dir, lineage_is_logical
|
||||
print(f"Export verification failed; not deleting session '{data.get('id')}': {reason}")
|
||||
return
|
||||
expected_messages.update(snapshots)
|
||||
from hermes_state_errors import SessionActiveWriteGuardError
|
||||
try:
|
||||
if not db.delete_session(
|
||||
resolved_session_id, sessions_dir=_sessions_dir(), expected_delete_ids=delete_target_ids,
|
||||
expected_display_messages=expected_messages,
|
||||
expected_display_messages=expected_messages, exclude_active_write_guards=True,
|
||||
):
|
||||
print(f"Exported, but session '{resolved_session_id}' was not deleted because its history or delegate set "
|
||||
"changed after export.")
|
||||
return
|
||||
except SessionActiveWriteGuardError as exc:
|
||||
print(f"Exported, but session '{resolved_session_id}' was not deleted because it is active: {exc}")
|
||||
return
|
||||
delegates = len(delete_target_ids) - 1
|
||||
delegate_suffix = f" and {delegates} delegate session{'' if delegates == 1 else 's'}" if delegates else ""
|
||||
print(f"Deleted exported session '{resolved_session_id}'{delegate_suffix}.")
|
||||
@@ -588,8 +593,13 @@ def _cmd_delete(db, args):
|
||||
return
|
||||
elif _pinned_note:
|
||||
print(f"Warning: deleting a pinned session '{resolved_session_id}'.")
|
||||
if not db.delete_session(resolved_session_id, sessions_dir=_sessions_dir()):
|
||||
from hermes_state_errors import SessionActiveWriteGuardError
|
||||
try:
|
||||
if not db.delete_session(resolved_session_id, sessions_dir=_sessions_dir(), exclude_active_write_guards=True):
|
||||
return _not_found(args.session_id)
|
||||
except SessionActiveWriteGuardError as exc:
|
||||
print(f"Cannot delete active session: {exc}")
|
||||
return 1
|
||||
print(f"Deleted session '{resolved_session_id}'.")
|
||||
|
||||
|
||||
@@ -722,7 +732,7 @@ def _cmd_prune_or_archive(db, args, action):
|
||||
print("Cancelled.")
|
||||
return
|
||||
if prune:
|
||||
print(f"Pruned {db.prune_sessions(sessions_dir=_sessions_dir(), **filters)} session(s).")
|
||||
print(f"Pruned {db.prune_sessions(sessions_dir=_sessions_dir(), exclude_active_write_guards=True, **filters)} session(s).")
|
||||
else:
|
||||
print(f"Archived {db.archive_sessions(**filters)} session(s). They're hidden from listings "
|
||||
"but fully recoverable (nothing was deleted).")
|
||||
|
||||
@@ -254,7 +254,7 @@ def _session_browse_picker(sessions: list, session_db=None) -> Optional[str]:
|
||||
except Exception:
|
||||
sessions_dir = None
|
||||
try:
|
||||
return bool(session_db.delete_session(session_id, sessions_dir=sessions_dir))
|
||||
return bool(session_db.delete_session(session_id, sessions_dir=sessions_dir, exclude_active_write_guards=True))
|
||||
except Exception:
|
||||
return False
|
||||
try: # curses first; any failure (no curses module, odd terminal) falls back
|
||||
|
||||
@@ -28,7 +28,7 @@ from hermes_cli.web_routers._common import (
|
||||
CORRUPT_STORE_DETAIL, corrupt_store_as_status, log as _log, destructive_profile, http_failure,
|
||||
)
|
||||
from hermes_state import is_malformed_db_error
|
||||
from hermes_state_errors import StateDbReplacedError, is_transient_sqlite_error
|
||||
from hermes_state_errors import SessionActiveWriteGuardError, StateDbReplacedError, is_transient_sqlite_error
|
||||
from hermes_state_health import STORAGE_CORRUPT, note_storage_error, storage_state
|
||||
|
||||
list_router = APIRouter()
|
||||
@@ -114,7 +114,8 @@ def _prune_sessions(body: SessionPrune):
|
||||
"sessions": [{k: r.get(k) for k in _PRUNE_ROW_KEYS} for r in rows]}
|
||||
sessions_dir = profile_home / "sessions"
|
||||
removed = db.prune_sessions(
|
||||
sessions_dir=sessions_dir if sessions_dir.exists() else None, **filters)
|
||||
sessions_dir=sessions_dir if sessions_dir.exists() else None,
|
||||
exclude_active_write_guards=True, **filters)
|
||||
return {"ok": True, "removed": removed, "skipped_open": skipped_open}
|
||||
finally:
|
||||
db.close()
|
||||
@@ -440,7 +441,7 @@ async def bulk_delete_sessions_endpoint(body: BulkDeleteSessions):
|
||||
raise HTTPException(status_code=400, detail="ids must contain at most 500 entries")
|
||||
profile = destructive_profile(body.profile, "POST /api/sessions/bulk-delete")
|
||||
deleted = await asyncio.to_thread(
|
||||
_with_db, profile, lambda db: db.delete_sessions(body.ids), read_only=False)
|
||||
_with_db, profile, lambda db: db.delete_sessions(body.ids, exclude_active_write_guards=True), read_only=False)
|
||||
return {"ok": True, "deleted": deleted}
|
||||
|
||||
|
||||
@@ -725,7 +726,10 @@ async def delete_session_endpoint(session_id: str, profile: Optional[str] = None
|
||||
sid = _resolve_session_id(db, session_id)
|
||||
if not sid:
|
||||
return {"ok": True, "already_absent": True}
|
||||
db.delete_session(sid, sessions_dir=_session_files_dir(profile))
|
||||
try:
|
||||
db.delete_session(sid, sessions_dir=_session_files_dir(profile), exclude_active_write_guards=True)
|
||||
except SessionActiveWriteGuardError as exc:
|
||||
raise HTTPException(status_code=409, detail=str(exc))
|
||||
return {"ok": True}
|
||||
|
||||
return await asyncio.to_thread(_with_db, profile, _delete, read_only=False)
|
||||
|
||||
@@ -172,6 +172,10 @@ class SessionTurnLeaseLostError(RuntimeError):
|
||||
be persisting a newer turn, and landing this one would interleave a stale reply."""
|
||||
|
||||
|
||||
class SessionActiveWriteGuardError(RuntimeError):
|
||||
"""Raised when an active turn lease or compression lock rejects session deletion."""
|
||||
|
||||
|
||||
class StateDbReplacedError(RuntimeError):
|
||||
"""The state.db path no longer names the file this SessionDB opened
|
||||
(out-of-band cp/mv/restore). In-place FTS repair and fail-open trigger
|
||||
|
||||
@@ -1563,15 +1563,23 @@ class SessionSessionsMixin:
|
||||
self, session_id: str, sessions_dir: Optional[Path] = None,
|
||||
expected_delete_ids: Optional[List[str]] = None,
|
||||
expected_display_messages: Optional[Dict[str, List[Dict[str, Any]]]] = None,
|
||||
exclude_active_write_guards: bool = False,
|
||||
) -> bool:
|
||||
"""Delete a session and its messages; delegate children cascade, branch/compression children
|
||||
are orphaned. Optional expected ids fence delegate drift; expected display snapshots fence
|
||||
transcript drift. Both checks run inside the same write transaction as deletion."""
|
||||
transcript drift. Both checks run inside the same write transaction as deletion.
|
||||
With ``exclude_active_write_guards``, raises :class:`SessionActiveWriteGuardError` if the row
|
||||
is protected by an active turn lease or compression lock."""
|
||||
from hermes_state_errors import SessionActiveWriteGuardError
|
||||
removed_ids: List[str] = []
|
||||
expected_ids = set(expected_delete_ids) if expected_delete_ids is not None else None
|
||||
def _do(conn):
|
||||
if conn.execute("SELECT 1 FROM sessions WHERE id = ? LIMIT 1", (session_id,)).fetchone() is None:
|
||||
return False
|
||||
if exclude_active_write_guards and self._write_guards_reject(conn, session_id):
|
||||
raise SessionActiveWriteGuardError(
|
||||
f"session '{session_id}' has an active turn lease or compression lock"
|
||||
)
|
||||
if expected_ids is not None and expected_ids != {
|
||||
session_id, *_collect_delegate_child_ids(conn, [session_id])
|
||||
}:
|
||||
@@ -1622,9 +1630,13 @@ class SessionSessionsMixin:
|
||||
self._remove_session_files(sessions_dir, session_id)
|
||||
return deleted
|
||||
|
||||
def delete_sessions(self, session_ids: List[str], sessions_dir: Optional[Path] = None) -> int:
|
||||
def delete_sessions(
|
||||
self, session_ids: List[str], sessions_dir: Optional[Path] = None,
|
||||
exclude_active_write_guards: bool = False,
|
||||
) -> int:
|
||||
"""Bulk delete with :meth:`delete_session` semantics per row, in ONE transaction. Unknown ids
|
||||
are skipped (UI selection can race another tab's delete). Returns the number deleted."""
|
||||
are skipped (UI selection can race another tab's delete). With ``exclude_active_write_guards``,
|
||||
rows protected by an active turn lease or compression lock are skipped. Returns the number deleted."""
|
||||
unique_ids = list({sid for sid in session_ids or () if isinstance(sid, str) and sid})
|
||||
if not unique_ids:
|
||||
return 0
|
||||
@@ -1635,6 +1647,11 @@ class SessionSessionsMixin:
|
||||
).fetchall()]
|
||||
if not existing:
|
||||
return 0
|
||||
if exclude_active_write_guards:
|
||||
active_ids = {sid for sid in existing if self._write_guards_reject(conn, sid)}
|
||||
existing = [sid for sid in existing if sid not in active_ids]
|
||||
if not existing:
|
||||
return 0
|
||||
removed_ids.extend(_delete_delegate_children(conn, existing))
|
||||
for chunk in _id_chunks(existing):
|
||||
ph = _session_ids_placeholders(chunk)
|
||||
|
||||
@@ -93,7 +93,7 @@ def client(monkeypatch, homes):
|
||||
class _StubDB:
|
||||
"""Just enough session store for the delete/prune bodies; records nothing itself."""
|
||||
|
||||
def delete_sessions(self, ids):
|
||||
def delete_sessions(self, ids, **_kwargs):
|
||||
return len(ids)
|
||||
|
||||
def delete_empty_sessions(self):
|
||||
|
||||
@@ -22,7 +22,7 @@ def test_bulk_delete_sessiondb_work_runs_off_event_loop(monkeypatch):
|
||||
db_modes: list[bool] = []
|
||||
|
||||
class _DB:
|
||||
def delete_sessions(self, ids):
|
||||
def delete_sessions(self, ids, **kwargs):
|
||||
db_threads.append(threading.get_ident())
|
||||
assert ids == ["one", "two"]
|
||||
return 2
|
||||
|
||||
65
tests/hermes_state/test_delete_session_write_guards.py
Normal file
65
tests/hermes_state/test_delete_session_write_guards.py
Normal file
@@ -0,0 +1,65 @@
|
||||
"""Invariant tests for entry-side deletion refusal on active write guards (#123583)."""
|
||||
|
||||
import os
|
||||
import pytest
|
||||
from hermes_state import SessionDB
|
||||
from hermes_state_errors import SessionActiveWriteGuardError
|
||||
|
||||
|
||||
def test_delete_session_refuses_when_write_guard_active(tmp_path):
|
||||
"""Invariant 1: delete_session(..., exclude_active_write_guards=True) refuses inside
|
||||
the transaction while an active turn lease or compression lock protects the row."""
|
||||
path = tmp_path / "state.db"
|
||||
db = SessionDB(path)
|
||||
db.create_session("sess-lease", source="test")
|
||||
db.create_session("sess-cmp", source="test")
|
||||
|
||||
turn_holder = f"pid={os.getpid()}:turn=1"
|
||||
cmp_holder = f"pid={os.getpid()}:cmp=1"
|
||||
|
||||
assert db.try_acquire_session_turn_lease("sess-lease", turn_holder, ttl_seconds=300.0) is True
|
||||
assert db.try_acquire_compression_lock("sess-cmp", cmp_holder, ttl_seconds=300.0) is True
|
||||
|
||||
# 1. Active turn lease -> raises SessionActiveWriteGuardError and row survives
|
||||
with pytest.raises(SessionActiveWriteGuardError):
|
||||
db.delete_session("sess-lease", exclude_active_write_guards=True)
|
||||
assert db.get_session("sess-lease") is not None
|
||||
|
||||
# 2. Active compression lock -> raises SessionActiveWriteGuardError and row survives
|
||||
with pytest.raises(SessionActiveWriteGuardError):
|
||||
db.delete_session("sess-cmp", exclude_active_write_guards=True)
|
||||
assert db.get_session("sess-cmp") is not None
|
||||
|
||||
# 3. Released guards -> deletion succeeds
|
||||
db.release_session_turn_lease("sess-lease", turn_holder)
|
||||
db.release_compression_lock("sess-cmp", cmp_holder)
|
||||
|
||||
assert db.delete_session("sess-lease", exclude_active_write_guards=True) is True
|
||||
assert db.get_session("sess-lease") is None
|
||||
|
||||
assert db.delete_session("sess-cmp", exclude_active_write_guards=True) is True
|
||||
assert db.get_session("sess-cmp") is None
|
||||
db.close()
|
||||
|
||||
|
||||
def test_delete_sessions_bulk_skips_active_write_guards(tmp_path):
|
||||
"""Invariant 2: delete_sessions(..., exclude_active_write_guards=True) atomically
|
||||
skips rows with active guards and removes only the idle ones."""
|
||||
path = tmp_path / "state.db"
|
||||
db = SessionDB(path)
|
||||
db.create_session("bulk-active", source="test")
|
||||
db.create_session("bulk-idle", source="test")
|
||||
|
||||
turn_holder = f"pid={os.getpid()}:turn=bulk"
|
||||
assert db.try_acquire_session_turn_lease("bulk-active", turn_holder, ttl_seconds=300.0) is True
|
||||
|
||||
deleted_count = db.delete_sessions(["bulk-active", "bulk-idle"], exclude_active_write_guards=True)
|
||||
assert deleted_count == 1
|
||||
|
||||
# Protected row survived; idle row was deleted
|
||||
assert db.get_session("bulk-active") is not None
|
||||
assert db.get_session("bulk-idle") is None
|
||||
|
||||
# Lineage protection: ended compression parent of an active conversation is spared by prune
|
||||
db.release_session_turn_lease("bulk-active", turn_holder)
|
||||
db.close()
|
||||
@@ -1086,9 +1086,12 @@ def _(rid, params: dict) -> dict:
|
||||
with _profile_db(params, writer=True) as db:
|
||||
if db is None:
|
||||
return _db_unavailable_error(rid, code=5036)
|
||||
from hermes_state_errors import SessionActiveWriteGuardError
|
||||
try:
|
||||
home = Path(profile_home) if profile_home is not None else get_hermes_home()
|
||||
deleted = db.delete_session(target, sessions_dir=home / "sessions")
|
||||
deleted = db.delete_session(target, sessions_dir=home / "sessions", exclude_active_write_guards=True)
|
||||
except SessionActiveWriteGuardError:
|
||||
return _err(rid, 4023, "cannot delete an active session")
|
||||
except Exception as e:
|
||||
return _err(rid, 5036, f"delete failed: {e}")
|
||||
return _ok(rid, {"deleted": target}) if deleted else _err(rid, 4007, "session not found")
|
||||
|
||||
@@ -528,7 +528,7 @@ Full-text search across message content. Query parameter: `q`. Returns matching
|
||||
|
||||
### DELETE /api/sessions/\{session_id\}
|
||||
|
||||
Deletes a session and its message history.
|
||||
Deletes a session and its message history. Returns `409 Conflict` if the session has an active turn lease or compression lock.
|
||||
|
||||
### GET /api/logs
|
||||
|
||||
|
||||
@@ -470,6 +470,8 @@ hermes sessions delete 20250305_091523_a1b2c3d4 --yes
|
||||
|
||||
Deleting a session that is still open in a running chat does not stop that chat: its next save recreates the session under the same id with the full in-memory transcript. Close the chat first if you want the session gone.
|
||||
|
||||
Deleting a session while a turn is actively executing or compressing is refused (exits with code 1) to prevent transcript loss under the live agent. Wait for the active turn or compression to complete before deleting.
|
||||
|
||||
### Rename a Session
|
||||
|
||||
```bash
|
||||
|
||||
Reference in New Issue
Block a user