fix: closed document stays active & leaks into new chats (#1160) (#1238)

* fix: closed document no longer stays active and leaks into new chats (#1160)

Closing a document tab calls _detachDocFromSession: a doc with content is
PATCHed to session_id="" (unlinked, session_id -> NULL, is_active stays True),
an empty one is DELETEd. But the in-memory active-document pointer
(tool_implementations._active_document_id) was never cleared on either path.

The chat doc-injection last-resort looks up that pointer by id and injects it
when `not cand.session_id or cand.session_id == session`. An unlinked doc has
session_id NULL, so the stale pointer re-surfaced a closed document in later,
unrelated chats — the agent kept reading/suggesting edits to a doc the user
had closed.

Fix: add clear_active_document(doc_id) and call it when a document is unlinked
(PATCH session_id="") or deleted, so the pointer no longer resurrects a closed
document. clear_active_document only clears when the id matches (or no id), so a
different active doc is left untouched.

Covered by tests/test_active_document_clear.py (4 cases).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test: add route-level regression for #1160 (detach/delete clears active doc)

Per review: prove the actual API path, not just the helper. Drives
PATCH /api/document/{id} (session_id="") and DELETE /api/document/{id}
through TestClient against a temp SQLite DB under real owner routing, and
asserts get_active_document() is cleared (and untouched when a different
document is closed).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test: make #1160 route regression hang-proof and dev-DB-independent

The route test could hang in other environments: it set DATABASE_URL at import
time, which is ignored if core.database was already imported, so it fell back to
the real dev DB and could contend for its locks (maintainer saw it hang, exit
124).

Rebind to a DEDICATED temporary SQLite engine (NullPool) and patch the document
route module's SessionLocal to it via an autouse fixture — so the test never
touches the dev DB and is independent of import order. Runs in ~0.3s.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test: drive #1160 route regression without TestClient (fixes local hang)

The route test used Starlette TestClient (middleware app + threadpool), which
hung in the maintainer's environment. Rework it to call the async route handlers
directly — extracted from the router — with a minimal fake request against a
temp-SQLite-patched SessionLocal. Same real coverage (handler + DB + owner
routing), but it completes reliably (~0.3s) with no TestClient/threadpool.

Verified the maintainer's exact batch now passes:
  pytest tests/test_document_close_clears_active_route.py \
         tests/test_active_document_clear.py \
         tests/test_document_tool_owner_scope.py  -> 14 passed

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

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:

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

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

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