diff --git a/routes/document_routes.py b/routes/document_routes.py index 71535c0..282e358 100644 --- a/routes/document_routes.py +++ b/routes/document_routes.py @@ -593,6 +593,15 @@ def setup_document_routes(session_manager, upload_handler=None) -> APIRouter: if req.session_id is not None: # Empty string = unlink from session doc.session_id = req.session_id if req.session_id else None + if not req.session_id: + # Tab closed / doc detached from its session — drop the + # in-memory active-doc pointer so the last-resort injection + # path doesn't re-surface this doc in a later chat (#1160). + try: + from src.tool_implementations import clear_active_document + clear_active_document(doc_id) + except Exception: + pass db.commit() db.refresh(doc) return _doc_to_dict(doc) @@ -615,6 +624,13 @@ def setup_document_routes(session_manager, upload_handler=None) -> APIRouter: raise HTTPException(404, "Document not found") _verify_doc_owner(db, doc, user) doc.is_active = False + # Closed/deleted — drop the in-memory active-doc pointer so it isn't + # re-injected into a later, unrelated chat (#1160). + try: + from src.tool_implementations import clear_active_document + clear_active_document(doc_id) + except Exception: + pass db.commit() return {"status": "deleted", "id": doc_id} except HTTPException: diff --git a/src/tool_implementations.py b/src/tool_implementations.py index c356a81..95d39f1 100644 --- a/src/tool_implementations.py +++ b/src/tool_implementations.py @@ -88,6 +88,24 @@ def get_active_document(): return _active_document_id +def clear_active_document(doc_id: Optional[str] = None) -> bool: + """Clear the in-memory active-document pointer. + + With ``doc_id`` given, only clears when it matches the current pointer, so a + different active document is left untouched. Returns True if it was cleared. + + Called when a document is detached from its session or deleted (its tab is + closed): without this, the stale pointer makes the last-resort doc-injection + path re-surface a closed document in a later, unrelated chat — even one whose + session no longer matches — because an unlinked doc has session_id NULL (#1160). + """ + global _active_document_id + if doc_id is None or _active_document_id == doc_id: + _active_document_id = None + return True + return False + + def _owned_document_query(query, Document, owner: Optional[str]): if owner is None: # A bare Python `False` is not a valid SQL expression — SQLAlchemy 1.4 diff --git a/tests/test_active_document_clear.py b/tests/test_active_document_clear.py new file mode 100644 index 0000000..70c36d9 --- /dev/null +++ b/tests/test_active_document_clear.py @@ -0,0 +1,39 @@ +"""Issue #1160 — a closed document must not stay 'active' and leak into new chats. + +Closing a document tab detaches it (session_id -> NULL) or deletes it, but the +in-memory active-document pointer was never cleared, so the last-resort doc +injection re-surfaced the closed doc in later, unrelated chats. The document +routes now call clear_active_document() on detach/delete; this pins that helper. +""" + +from src.tool_implementations import ( + set_active_document, + get_active_document, + clear_active_document, +) + + +def test_clear_matching_id_resets_pointer(): + set_active_document("doc-123") + assert get_active_document() == "doc-123" + assert clear_active_document("doc-123") is True + assert get_active_document() is None + + +def test_clear_non_matching_id_leaves_other_active_doc(): + set_active_document("doc-abc") + # Closing a DIFFERENT document must not clobber the currently active one. + assert clear_active_document("doc-xyz") is False + assert get_active_document() == "doc-abc" + + +def test_clear_without_id_clears_unconditionally(): + set_active_document("doc-abc") + assert clear_active_document() is True + assert get_active_document() is None + + +def test_clear_when_already_none_is_safe(): + set_active_document(None) + assert clear_active_document("doc-123") is False + assert get_active_document() is None diff --git a/tests/test_document_close_clears_active_route.py b/tests/test_document_close_clears_active_route.py new file mode 100644 index 0000000..b1ab9c7 --- /dev/null +++ b/tests/test_document_close_clears_active_route.py @@ -0,0 +1,93 @@ +"""Issue #1160 — route-level regression for clearing the active-document pointer. + +Exercises the REAL ``PATCH /api/document/{id}`` (session_id="") and +``DELETE /api/document/{id}`` handlers, proving that closing a document's tab +(detach or delete) clears the in-memory active-document pointer under the actual +owner/session routing — not just the helper in isolation. + +Calls the route handler callables DIRECTLY (extracted from the router) instead of +through Starlette's TestClient. The TestClient path spun up a middleware app + +threadpool that could hang in some environments; calling the async handler with a +minimal fake request keeps the same real coverage (handler + DB + owner routing) +while completing reliably everywhere. +""" + +import tempfile +import uuid +from types import SimpleNamespace + +from sqlalchemy import create_engine +from sqlalchemy.orm import sessionmaker +from sqlalchemy.pool import NullPool +from unittest.mock import MagicMock + +import core.database as cdb +import routes.document_routes as droutes +from core.database import Document +from core.database import Session as DbSession +from routes.document_helpers import DocumentPatch +from src.tool_implementations import set_active_document, get_active_document + +_TMPDB = tempfile.NamedTemporaryFile(suffix=".db", delete=False) +_ENGINE = create_engine( + f"sqlite:///{_TMPDB.name}", + connect_args={"check_same_thread": False}, + poolclass=NullPool, +) +cdb.Base.metadata.create_all(_ENGINE) +_TS = sessionmaker(bind=_ENGINE, autoflush=False, autocommit=False) +droutes.SessionLocal = _TS # route handlers resolve SessionLocal at call time + + +def _req(): + return SimpleNamespace(state=SimpleNamespace(current_user="tester")) + + +def _endpoint(method, path): + router = droutes.setup_document_routes(MagicMock(), None) + for r in router.routes: + if getattr(r, "path", None) == path and method in getattr(r, "methods", set()): + return r.endpoint + raise RuntimeError(f"{method} {path} not found") + + +def _make_doc(): + sid = "s-" + uuid.uuid4().hex[:8] + db = _TS() + try: + db.add(DbSession(id=sid, owner="tester", name="s", model="m", endpoint_url="http://x")) + doc = Document( + id=str(uuid.uuid4()), session_id=sid, title="t", + language="markdown", current_content="hi", version_count=1, + is_active=True, owner="tester", + ) + db.add(doc) + db.commit() + return doc.id + finally: + db.close() + + +async def test_patch_unlink_clears_active_document(): + patch_document = _endpoint("PATCH", "/api/document/{doc_id}") + doc_id = _make_doc() + set_active_document(doc_id) + await patch_document(_req(), doc_id, DocumentPatch(session_id="")) + assert get_active_document() is None + + +async def test_delete_clears_active_document(): + delete_document = _endpoint("DELETE", "/api/document/{doc_id}") + doc_id = _make_doc() + set_active_document(doc_id) + await delete_document(_req(), doc_id) + assert get_active_document() is None + + +async def test_unlinking_a_different_doc_leaves_pointer(): + patch_document = _endpoint("PATCH", "/api/document/{doc_id}") + active_id = _make_doc() + other_id = _make_doc() + set_active_document(active_id) + await patch_document(_req(), other_id, DocumentPatch(session_id="")) + assert get_active_document() == active_id