Sessions: allow deleting memory-only ghost sessions

A session that exists only in the in-memory SessionManager — never persisted,
or whose DB row was removed out-of-band — was listed by GET /api/sessions (the
list is built from the in-memory manager) but 404'd on every per-session
operation, so it could never be deleted.

Two causes, both fixed:

1. _verify_session_owner() only consulted the DB and raised 404 when no row
   existed. It now falls back to the in-memory session's owner when (and only
   when) a session_manager is supplied and the caller actually owns the ghost.
   The DB row stays authoritative when present, and a ghost owned by another
   user still 404s, so the ownership/security model is unchanged. The new
   parameter defaults to None, preserving behavior for all other callers.

2. SessionManager.delete_session() only removed the in-memory entry when a DB
   row was found, so memory-only ghosts survived. It now drops the in-memory
   copy regardless and reports success when either the DB row or the in-memory
   entry was removed.

Added tests/test_session_ghost_delete.py covering both layers, including the
cross-owner 404, the unauthenticated 403, DB-row-wins precedence, and backward
compatibility when no manager is passed.

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

core/session_manager.py

@@ -466,11 +466,17 @@ class SessionManager:
             db_session = db.query(DbSession).filter(DbSession.id == session_id).first()
             if db_session:
                 db.delete(db_session)
+
+            # Drop the in-memory copy even when there is no DB row. A "ghost"
+            # session lives only here (never persisted, or its row was removed
+            # out-of-band); without this it can never be cleared and keeps
+            # 404ing on every operation (issue #1044).
+            removed_in_memory = self.sessions.pop(session_id, None) is not None
+
+            if db_session or removed_in_memory:
+                # Commit the document-detach / message-delete above (a no-op when
+                # the ghost had no rows) together with the session delete.
                 db.commit()
-
-                if session_id in self.sessions:
-                    del self.sessions[session_id]
-
                 logger.info(f"Deleted session {session_id}")
                 return True
             return False

routes/session_routes.py

@@ -21,20 +21,38 @@ def _sanitize_export_filename(name: str) -> str:
     return name[:128]
 
 
-def _verify_session_owner(request: Request, session_id: str):
+def _verify_session_owner(request: Request, session_id: str, session_manager=None):
-    """Verify the current user owns the session. Raises 404 if not."""
+    """Verify the current user owns the session. Raises 404 if not.
+
+    Ownership is checked against the DB row when one exists (unchanged). If
+    there is no DB row but the caller owns an in-memory "ghost" session — one
+    that lives only in ``session_manager`` because it was never persisted, or
+    its DB row was removed out-of-band — fall back to the in-memory owner so the
+    user can still manage and delete it. Without this fallback such sessions are
+    listed by ``/api/sessions`` (they come from the in-memory manager) yet every
+    per-session operation 404s, making them impossible to delete (issue #1044).
+
+    ``session_manager`` is optional and defaults to ``None`` so existing callers
+    that only care about persisted sessions keep their exact prior behavior.
+    """
     user = effective_user(request)
     if not user:
         raise HTTPException(403, "Authentication required")
     db = SessionLocal()
     try:
         row = db.query(DbSession.owner).filter(DbSession.id == session_id).first()
-        if not row:
-            raise HTTPException(404, f"Session {session_id} not found")
-        if row.owner != user:
-            raise HTTPException(404, f"Session {session_id} not found")
     finally:
         db.close()
+    if row is not None:
+        if row.owner != user:
+            raise HTTPException(404, f"Session {session_id} not found")
+        return
+    # No DB row — allow the caller to act on an in-memory ghost they own.
+    if session_manager is not None:
+        ghost = getattr(session_manager, "sessions", {}).get(session_id)
+        if ghost is not None and getattr(ghost, "owner", None) == user:
+            return
+    raise HTTPException(404, f"Session {session_id} not found")
 
 logger = logging.getLogger(__name__)
 
@@ -363,7 +381,7 @@ def setup_session_routes(session_manager: SessionManager, config: dict, webhook_
             ids = []
         for sid in ids:
             try:
-                _verify_session_owner(request, sid)
+                _verify_session_owner(request, sid, session_manager)
                 session_manager.delete_session(sid)
                 db = SessionLocal()
                 try:
@@ -381,7 +399,7 @@ def setup_session_routes(session_manager: SessionManager, config: dict, webhook_
     @router.delete("/session/{sid}")
     def delete_session(request: Request, sid: str):
         """Permanently delete a session and all its messages."""
-        _verify_session_owner(request, sid)
+        _verify_session_owner(request, sid, session_manager)
         try:
             # Block deletion of starred/favorited sessions
             db = SessionLocal()

tests/test_session_ghost_delete.py

@@ -0,0 +1,135 @@
+"""Regression tests for issue #1044 — "ghost" sessions that appear in the list
+but 404 on every operation and can never be deleted.
+
+A ghost session lives only in the in-memory ``SessionManager`` (it was never
+persisted, or its DB row was removed out-of-band). ``GET /api/sessions`` lists
+sessions from the in-memory manager, so a ghost shows up; but ``_verify_session_owner``
+only consulted the DB, so every per-session op 404'd, and ``SessionManager.delete_session``
+only dropped the in-memory copy when a DB row existed — so the ghost was undeletable.
+
+These tests pin both halves of the fix while proving the ownership/security model
+is preserved (a ghost owned by another user still 404s; the DB row stays
+authoritative when present).
+
+Style mirrors tests/test_session_owner_attribution.py: stub the heavy ORM modules
+so the real route + manager code can be imported under the MagicMock sqlalchemy
+stub from conftest.
+"""
+
+import sys
+import importlib
+from types import SimpleNamespace
+from unittest.mock import MagicMock
+
+import pytest
+
+# Import the *real* core.session_manager + routes.session_routes under conftest's
+# MagicMock sqlalchemy stub. The real core.database defines declarative classes
+# that blow up under that stub, so temporarily swap in MagicMock module objects
+# (auto-creating attributes satisfy any `from core.database import X`). Crucially
+# we RESTORE sys.modules immediately after import so these stubs never leak into
+# sibling test modules — the imported SM/SR objects keep their captured bindings.
+_ABSENT = object()
+_TEMP_STUBS = ("core.database", "core.models", "src.request_models")
+_saved = {name: sys.modules.get(name, _ABSENT) for name in _TEMP_STUBS}
+_saved["core.session_manager"] = sys.modules.get("core.session_manager", _ABSENT)
+try:
+    for _name in _TEMP_STUBS:
+        sys.modules[_name] = MagicMock(name=_name)
+    if isinstance(sys.modules.get("core.session_manager"), MagicMock):
+        del sys.modules["core.session_manager"]
+    SM = importlib.import_module("core.session_manager")
+    import routes.session_routes as SR  # noqa: E402
+finally:
+    for _name, _val in _saved.items():
+        if _val is _ABSENT:
+            sys.modules.pop(_name, None)
+        else:
+            sys.modules[_name] = _val
+
+from fastapi import HTTPException  # noqa: E402
+
+
+_MISSING = object()
+
+
+def _req(**state):
+    return SimpleNamespace(state=SimpleNamespace(**state))
+
+
+def _session_local_returning(owner_value):
+    """Mock SessionLocal whose query(...).filter(...).first() yields a row with
+    the given owner, or None when owner_value is _MISSING ('no DB row')."""
+    db = MagicMock()
+    row = None if owner_value is _MISSING else SimpleNamespace(owner=owner_value)
+    db.query.return_value.filter.return_value.first.return_value = row
+    return MagicMock(return_value=db)
+
+
+def _manager_with(sessions):
+    """A SessionManager instance with the given in-memory sessions and no __init__."""
+    mgr = SM.SessionManager.__new__(SM.SessionManager)
+    mgr.sessions = dict(sessions)
+    return mgr
+
+
+# --- route layer: _verify_session_owner ghost fallback ---------------------
+
+def test_owned_ghost_is_allowed_when_manager_passed(monkeypatch):
+    # No DB row, but the caller owns the in-memory ghost -> must NOT raise.
+    monkeypatch.setattr(SR, "SessionLocal", _session_local_returning(_MISSING))
+    sm = SimpleNamespace(sessions={"ghost": SimpleNamespace(owner="alice")})
+    SR._verify_session_owner(_req(api_token=False, current_user="alice"), "ghost", sm)
+
+
+def test_ghost_owned_by_another_user_still_404(monkeypatch):
+    # Security: a ghost owned by bob must never be reachable by alice.
+    monkeypatch.setattr(SR, "SessionLocal", _session_local_returning(_MISSING))
+    sm = SimpleNamespace(sessions={"ghost": SimpleNamespace(owner="bob")})
+    with pytest.raises(HTTPException) as exc:
+        SR._verify_session_owner(_req(api_token=False, current_user="alice"), "ghost", sm)
+    assert exc.value.status_code == 404
+
+
+def test_no_manager_keeps_legacy_404(monkeypatch):
+    # Backward compat: callers that don't pass a manager behave exactly as before.
+    monkeypatch.setattr(SR, "SessionLocal", _session_local_returning(_MISSING))
+    with pytest.raises(HTTPException) as exc:
+        SR._verify_session_owner(_req(api_token=False, current_user="alice"), "ghost")
+    assert exc.value.status_code == 404
+
+
+def test_db_row_stays_authoritative(monkeypatch):
+    # When a DB row exists it wins; the ghost map is not consulted.
+    monkeypatch.setattr(SR, "SessionLocal", _session_local_returning("alice"))
+    sm = SimpleNamespace(sessions={"sid": SimpleNamespace(owner="bob")})
+    SR._verify_session_owner(_req(api_token=False, current_user="alice"), "sid", sm)
+
+
+def test_unauthenticated_still_403(monkeypatch):
+    monkeypatch.setattr(SR, "SessionLocal", _session_local_returning(_MISSING))
+    sm = SimpleNamespace(sessions={"ghost": SimpleNamespace(owner=None)})
+    with pytest.raises(HTTPException) as exc:
+        SR._verify_session_owner(_req(api_token=False, current_user=None), "ghost", sm)
+    assert exc.value.status_code == 403
+
+
+# --- manager layer: delete_session clears memory-only ghosts ---------------
+
+def test_manager_deletes_memory_only_ghost(monkeypatch):
+    # No DB row, but the session is in memory -> delete it and report success.
+    fake_db = MagicMock()
+    fake_db.query.return_value.filter.return_value.first.return_value = None
+    monkeypatch.setattr(SM, "SessionLocal", MagicMock(return_value=fake_db))
+    mgr = _manager_with({"ghost": SimpleNamespace(id="ghost", owner="alice")})
+    assert mgr.delete_session("ghost") is True
+    assert "ghost" not in mgr.sessions
+
+
+def test_manager_delete_unknown_returns_false(monkeypatch):
+    # Nothing in the DB and nothing in memory -> nothing deleted.
+    fake_db = MagicMock()
+    fake_db.query.return_value.filter.return_value.first.return_value = None
+    monkeypatch.setattr(SM, "SessionLocal", MagicMock(return_value=fake_db))
+    mgr = _manager_with({})
+    assert mgr.delete_session("nope") is False