From db10c8d95be8c17b6aa1c49c0b9ffd186179005e Mon Sep 17 00:00:00 2001 From: Shaw Date: Tue, 2 Jun 2026 07:51:26 -0400 Subject: [PATCH] Sessions: allow deleting memory-only ghost sessions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- core/session_manager.py | 14 ++- routes/session_routes.py | 34 ++++++-- tests/test_session_ghost_delete.py | 135 +++++++++++++++++++++++++++++ 3 files changed, 171 insertions(+), 12 deletions(-) create mode 100644 tests/test_session_ghost_delete.py diff --git a/core/session_manager.py b/core/session_manager.py index e9a2740..8b41a47 100644 --- a/core/session_manager.py +++ b/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 diff --git a/routes/session_routes.py b/routes/session_routes.py index e524791..2a2df38 100644 --- a/routes/session_routes.py +++ b/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): - """Verify the current user owns the session. Raises 404 if not.""" +def _verify_session_owner(request: Request, session_id: str, session_manager=None): + """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() diff --git a/tests/test_session_ghost_delete.py b/tests/test_session_ghost_delete.py new file mode 100644 index 0000000..dc6a4c9 --- /dev/null +++ b/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