Squash Odysseus development history

routes/personal_routes.py

@@ -1,20 +1,133 @@
 # routes/personal_routes.py
 """Routes for personal documents management."""
+import asyncio
 import os
 import logging
-from typing import List
+import shutil
+import uuid
+from typing import Any, Dict, List, Tuple
 from fastapi import APIRouter, HTTPException, Query, Request, UploadFile, File, Depends
+from fastapi.concurrency import run_in_threadpool
 from src.request_models import DirectoryRequest
-from core.constants import BASE_DIR, PERSONAL_DIR
+from core.constants import BASE_DIR, PERSONAL_DIR, PERSONAL_UPLOADS_DIR
 from src.rag_singleton import get_rag_manager
-from src.auth_helpers import get_current_user, require_user
+from src.auth_helpers import require_privilege, require_user
 from core.middleware import require_admin
 from src.upload_handler import secure_filename
+from src.upload_limits import PERSONAL_UPLOAD_MAX_BYTES
 
-UPLOADS_DIR = os.path.join(BASE_DIR, "data", "personal_uploads")
+UPLOADS_DIR = PERSONAL_UPLOADS_DIR
 
 logger = logging.getLogger(__name__)
 
+def _personal_upload_dir_for_owner(owner: str | None, *, create: bool = True) -> str:
+    """Return the per-owner upload directory used for direct RAG uploads."""
+    owner_segment = secure_filename((owner or "local").strip())[:80] or "local"
+    upload_dir = os.path.abspath(os.path.join(UPLOADS_DIR, owner_segment))
+    base_abs = os.path.abspath(UPLOADS_DIR)
+    if os.path.commonpath([upload_dir, base_abs]) != base_abs:
+        raise ValueError("Unsafe upload owner path")
+    if create:
+        os.makedirs(upload_dir, exist_ok=True)
+    return upload_dir
+
+
+def _unique_personal_upload_path(upload_dir: str, original_name: str | None) -> Tuple[str, str, str]:
+    """Build a collision-resistant upload path while preserving a display name."""
+    safe_name = secure_filename(os.path.basename(original_name or "upload"))
+    if not safe_name or safe_name.startswith("."):
+        safe_name = "upload"
+
+    stem, ext = os.path.splitext(safe_name)
+    stem = (stem or "upload")[:80]
+    filename = f"{stem}-{uuid.uuid4().hex[:10]}{ext.lower()}"
+    file_path = os.path.abspath(os.path.join(upload_dir, filename))
+    upload_abs = os.path.abspath(upload_dir)
+    if os.path.commonpath([file_path, upload_abs]) != upload_abs:
+        raise ValueError("Unsafe upload filename")
+    return file_path, filename, safe_name
+
+
+def _unique_existing_target(path: str) -> str:
+    """Return a non-existing sibling path for rename collision handling."""
+    if not os.path.exists(path):
+        return path
+    stem, ext = os.path.splitext(path)
+    while True:
+        candidate = f"{stem}-{uuid.uuid4().hex[:10]}{ext}"
+        if not os.path.exists(candidate):
+            return candidate
+
+
+def _remove_empty_tree(path: str) -> None:
+    """Best-effort removal of empty directories under ``path``."""
+    if not os.path.isdir(path):
+        return
+    for root, dirs, _files in os.walk(path, topdown=False):
+        for dirname in dirs:
+            candidate = os.path.join(root, dirname)
+            try:
+                os.rmdir(candidate)
+            except OSError:
+                pass
+    try:
+        os.rmdir(path)
+    except OSError:
+        pass
+
+
+def rename_personal_upload_owner(
+    old_owner: str,
+    new_owner: str,
+    *,
+    personal_docs_manager: Any = None,
+    rag_manager: Any = None,
+) -> Dict[str, Any]:
+    """Move direct personal uploads and rewrite RAG owner metadata on user rename."""
+    old_dir = _personal_upload_dir_for_owner(old_owner, create=False)
+    new_dir = _personal_upload_dir_for_owner(new_owner, create=False)
+    path_map: Dict[str, str] = {}
+    moved_files = 0
+
+    if os.path.isdir(old_dir) and old_dir != new_dir:
+        os.makedirs(new_dir, exist_ok=True)
+        for root, _dirs, files in os.walk(old_dir):
+            rel_root = os.path.relpath(root, old_dir)
+            target_root = new_dir if rel_root == "." else os.path.join(new_dir, rel_root)
+            os.makedirs(target_root, exist_ok=True)
+            for filename in files:
+                source = os.path.abspath(os.path.join(root, filename))
+                target = _unique_existing_target(os.path.abspath(os.path.join(target_root, filename)))
+                shutil.move(source, target)
+                path_map[source] = target
+                moved_files += 1
+        _remove_empty_tree(old_dir)
+
+    if personal_docs_manager is not None:
+        rename_directory = getattr(personal_docs_manager, "rename_directory", None)
+        if callable(rename_directory):
+            rename_directory(old_dir, new_dir, path_map=path_map)
+
+    rag_result = None
+    if rag_manager is not None:
+        rename_owner = getattr(rag_manager, "rename_owner", None)
+        if callable(rename_owner):
+            rag_result = rename_owner(
+                old_owner,
+                new_owner,
+                path_map=path_map,
+                path_prefixes=[(old_dir, new_dir)],
+            )
+
+    return {
+        "old_dir": old_dir,
+        "new_dir": new_dir,
+        "moved_files": moved_files,
+        "path_map": path_map,
+        "rag_result": rag_result,
+    }
+
+
 def setup_personal_routes(personal_docs_manager, rag_manager, rag_available):
     """
     Setup personal documents related routes.
@@ -29,6 +142,22 @@ def setup_personal_routes(personal_docs_manager, rag_manager, rag_available):
     """
     router = APIRouter(prefix="/api/personal")
 
+    # Serializes directory index jobs across requests. Indexing runs in the
+    # threadpool (#5558), so concurrent requests would otherwise run in parallel
+    # and race PersonalDocsManager's unsynchronized list mutations and file
+    # writes; before the threadpool move they serialized on the blocked event
+    # loop, so one-at-a-time is behavior parity.
+    #
+    # An asyncio.Lock acquired in the async handler BEFORE offloading: a waiting
+    # request parks on the event loop instead of pinning a threadpool worker (an
+    # earlier threading.Lock taken INSIDE the worker meant queued jobs held pool
+    # tokens while blocked, starving every other run_in_threadpool caller).
+    # add/remove/reload all take this lock, so their mutations never interleave.
+    # Per-router (not module-global) so each app binds it to its own event loop.
+    # Scope is the single process: multi-worker deployments would need a shared
+    # lock (out of scope for #5558).
+    _index_job_lock = asyncio.Lock()
+
     def _rag():
         """Get the current RAG manager, retrying init if needed."""
         return get_rag_manager()
@@ -38,9 +167,12 @@ def setup_personal_routes(personal_docs_manager, rag_manager, rag_available):
         if not directory:
             raise HTTPException(400, "Directory path is required")
 
-        base_abs = os.path.abspath(PERSONAL_DIR)
+        # realpath (not abspath) so a symlink inside PERSONAL_DIR that points
+        # outside it is resolved before the commonpath confinement check below;
+        # abspath only normalises `..` and would let such a symlink escape.
+        base_abs = os.path.realpath(PERSONAL_DIR)
         candidate = directory if os.path.isabs(directory) else os.path.join(base_abs, directory)
-        resolved = os.path.abspath(candidate)
+        resolved = os.path.realpath(candidate)
         try:
             in_base = os.path.commonpath([resolved, base_abs]) == base_abs
         except ValueError:
@@ -57,8 +189,12 @@ def setup_personal_routes(personal_docs_manager, rag_manager, rag_available):
         return {"files": files, "directories": directories}
     
     @router.post("/reload")
-    def api_personal_reload(owner: str = Depends(require_user), _admin: None = Depends(require_admin)):
-        personal_docs_manager.refresh_index()
+    async def api_personal_reload(owner: str = Depends(require_user), _admin: None = Depends(require_admin)):
+        # refresh_index() re-extracts text across every tracked directory —
+        # blocking work. Take the shared job lock (so it cannot race an add /
+        # remove) and run it off the event loop.
+        async with _index_job_lock:
+            await run_in_threadpool(personal_docs_manager.refresh_index)
         return {"ok": True, "count": len(personal_docs_manager.index)}
     
     @router.post("/add_directory")
@@ -92,12 +228,26 @@ def setup_personal_routes(personal_docs_manager, rag_manager, rag_available):
             # Use the RAGManager to index the directory
             rag = _rag()
             if rag:
-                result = rag.index_personal_documents(directory, owner=owner)
-                
+                def _index_directory():
+                    result = rag.index_personal_documents(directory, owner=owner)
+                    if result["success"]:
+                        # Also update the personal_docs_manager to track this
+                        # directory. Kept inside the offloaded call: it triggers
+                        # refresh_index(), which re-extracts text across tracked
+                        # directories.
+                        personal_docs_manager.add_directory(directory, index=False)
+                    return result
+
+                # Indexing walks, embeds, and stores the whole tree — minutes
+                # on a real directory. The handler is async, so calling it
+                # inline runs it on the event loop and every other request
+                # queues behind it until it finishes (#5558). Serialize on the
+                # async job lock BEFORE offloading so a queued request parks on
+                # the loop instead of pinning a threadpool worker.
+                async with _index_job_lock:
+                    result = await run_in_threadpool(_index_directory)
+
                 if result["success"]:
-                    # Also update the personal_docs_manager to track this directory
-                    personal_docs_manager.add_directory(directory, index=False)
-                    
                     return {
                         "success": True,
                         "message": f"Successfully indexed {result['indexed_count']} chunks from {directory}",
@@ -128,22 +278,33 @@ def setup_personal_routes(personal_docs_manager, rag_manager, rag_available):
             JSON response confirming removal
         """
         try:
-            if not directory:
-                raise HTTPException(400, "Directory path is required")
+            # Confine to PERSONAL_DIR — parity with add_directory_to_rag (which
+            # resolves the path the same way). Without this, an arbitrary or
+            # `..`-escaping path is passed straight to
+            # personal_docs_manager.remove_directory / rag.remove_directory.
+            directory = _resolve_allowed_personal_dir(directory)
 
             logger.info(f"Removing directory from RAG: {directory}")
 
-            # Always remove from personal_docs_manager tracking
-            if hasattr(personal_docs_manager, 'remove_directory'):
-                personal_docs_manager.remove_directory(directory)
-
-            # Remove from RAG vector store (best-effort)
             rag = _rag()
-            if rag:
-                try:
-                    rag.remove_directory(directory)
-                except Exception as e:
-                    logger.warning(f"RAG removal failed for directory {directory}: {e}")
+
+            def _remove_directory():
+                # Always remove from personal_docs_manager tracking. This
+                # mutates the same unsynchronized list/index an add job touches
+                # and re-extracts text (refresh_index), so it is blocking work.
+                if hasattr(personal_docs_manager, 'remove_directory'):
+                    personal_docs_manager.remove_directory(directory)
+                # Remove from RAG vector store (best-effort).
+                if rag:
+                    try:
+                        rag.remove_directory(directory)
+                    except Exception as e:
+                        logger.warning(f"RAG removal failed for directory {directory}: {e}")
+
+            # Same job lock as add/reload so remove cannot interleave with an
+            # in-flight add; offloaded off the event loop.
+            async with _index_job_lock:
+                await run_in_threadpool(_remove_directory)
 
             return {
                 "success": True,
@@ -160,70 +321,84 @@ def setup_personal_routes(personal_docs_manager, rag_manager, rag_available):
     @router.post("/upload")
     async def upload_files_to_rag(request: Request, files: List[UploadFile] = File(...)):
         """Upload files directly into RAG. Supports text and PDF."""
-        user = get_current_user(request)
+        user = require_privilege(request, "can_use_documents")
         rag = _rag()
         if not rag:
             raise HTTPException(503, "RAG system is not available — is the embedding service running?")
 
-        os.makedirs(UPLOADS_DIR, exist_ok=True)
+        upload_dir = _personal_upload_dir_for_owner(user)
 
         total_indexed = 0
         total_failed = 0
         uploaded_files = []
 
-        for upload in files:
-            try:
-                # Sanitize filename — strip directory components and unsafe chars
-                safe_name = secure_filename(os.path.basename(upload.filename or "upload"))
-                if not safe_name or safe_name.startswith("."):
-                    safe_name = f"upload_{total_indexed + total_failed}"
-                file_path = os.path.join(UPLOADS_DIR, safe_name)
-                # Defense-in-depth: ensure resolved path stays under UPLOADS_DIR
-                base_abs = os.path.abspath(UPLOADS_DIR)
-                if os.path.commonpath([os.path.abspath(file_path), base_abs]) != base_abs:
-                    logger.warning(f"Rejected unsafe upload path: {upload.filename!r}")
-                    total_failed += 1
-                    continue
-                content_bytes = await upload.read()
-                with open(file_path, "wb") as f:
-                    f.write(content_bytes)
-
-                ext = os.path.splitext(safe_name)[1].lower()
-                if ext == ".pdf":
-                    from src.personal_docs import extract_pdf_text
-                    text = extract_pdf_text(file_path)
-                else:
-                    text = content_bytes.decode("utf-8", errors="replace")
-
-                if not text or not text.strip():
-                    total_failed += 1
-                    continue
-
-                # Chunk and index
-                chunks = rag._split_into_chunks(text, chunk_size=500)
-                for i, chunk in enumerate(chunks):
-                    metadata = {
-                        "source": file_path,
-                        "filename": safe_name,
-                        "directory": UPLOADS_DIR,
-                        "type": ext,
-                        "chunk_id": i,
-                    }
-                    if user:
-                        metadata["owner"] = user
-                    if rag.add_document(chunk, metadata):
-                        total_indexed += 1
-                    else:
+        # Chunking, embedding and the tracking update are blocking work over the
+        # same vector/tracking state add_directory mutates (#5634). Take the
+        # shared job lock BEFORE offloading so a queued request parks on the loop
+        # instead of pinning a threadpool worker, matching add_directory.
+        # Read and process one capped payload at a time so a multi-file request
+        # cannot retain len(files) * PERSONAL_UPLOAD_MAX_BYTES in memory.
+        async with _index_job_lock:
+            for upload in files:
+                try:
+                    file_path, stored_name, safe_name = _unique_personal_upload_path(
+                        upload_dir, upload.filename
+                    )
+                    content_bytes = await upload.read(PERSONAL_UPLOAD_MAX_BYTES + 1)
+                    if len(content_bytes) > PERSONAL_UPLOAD_MAX_BYTES:
+                        logger.warning(f"Rejected oversized personal upload: {upload.filename!r}")
                         total_failed += 1
+                        continue
 
-                uploaded_files.append(safe_name)
-            except Exception as e:
-                logger.error(f"Failed to upload/index {upload.filename}: {e}")
-                total_failed += 1
+                    def _index_upload():
+                        with open(file_path, "wb") as f:
+                            f.write(content_bytes)
 
-        # Track uploads directory
-        if uploaded_files and hasattr(personal_docs_manager, "add_directory"):
-            personal_docs_manager.add_directory(UPLOADS_DIR, index=False)
+                        ext = os.path.splitext(safe_name)[1].lower()
+                        if ext == ".pdf":
+                            from src.personal_docs import extract_pdf_text
+                            text = extract_pdf_text(file_path)
+                        else:
+                            text = content_bytes.decode("utf-8", errors="replace")
+
+                        if not text or not text.strip():
+                            return 0, 1, None
+
+                        indexed = 0
+                        failed = 0
+                        chunks = rag._split_into_chunks(text, chunk_size=500)
+                        for i, chunk in enumerate(chunks):
+                            metadata = {
+                                "source": file_path,
+                                "filename": safe_name,
+                                "stored_filename": stored_name,
+                                "directory": upload_dir,
+                                "type": ext,
+                                "chunk_id": i,
+                            }
+                            if user:
+                                metadata["owner"] = user
+                            if rag.add_document(chunk, metadata):
+                                indexed += 1
+                            else:
+                                failed += 1
+                        return indexed, failed, safe_name
+
+                    indexed, failed, uploaded_name = await run_in_threadpool(_index_upload)
+                    total_indexed += indexed
+                    total_failed += failed
+                    if uploaded_name:
+                        uploaded_files.append(uploaded_name)
+                except Exception as e:
+                    logger.error(f"Failed to upload/index {upload.filename}: {e}")
+                    total_failed += 1
+
+            # Same transition, same lock: the tracking update must not land
+            # while another job is mid-write over the same state.
+            if uploaded_files and hasattr(personal_docs_manager, "add_directory"):
+                await run_in_threadpool(
+                    personal_docs_manager.add_directory, upload_dir, index=False
+                )
 
         return {
             "success": True,
@@ -236,33 +411,47 @@ def setup_personal_routes(personal_docs_manager, rag_manager, rag_available):
     async def delete_file_from_rag(filepath: str = Query(...), owner: str = Depends(require_user), _admin: None = Depends(require_admin)):
         """Delete a specific file from RAG index and optionally from disk."""
         try:
-            # Remove chunks from RAG vector store (best-effort)
-            removed = 0
-            rag = _rag()
-            if rag:
+            def _delete_file():
+                # Remove chunks from RAG vector store (best-effort)
+                removed = 0
+                rag = _rag()
+                if rag:
+                    try:
+                        removed = rag.delete_by_source(filepath)
+                    except Exception as e:
+                        logger.warning(f"RAG removal failed for {filepath}: {e}")
+
+                # Delete file from disk if it's in the caller's own uploads dir.
+                # Scope to the per-owner subdir, not the shared uploads root, so one
+                # admin can't delete another user's personal files by path.
+                deleted_from_disk = False
                 try:
-                    removed = rag.delete_by_source(filepath)
-                except Exception as e:
-                    logger.warning(f"RAG removal failed for {filepath}: {e}")
+                    abs_target = os.path.realpath(filepath)
+                    base_abs = os.path.realpath(_personal_upload_dir_for_owner(owner, create=False))
+                    in_uploads = (
+                        abs_target == base_abs
+                        or os.path.commonpath([abs_target, base_abs]) == base_abs
+                    )
+                except ValueError:
+                    # commonpath raises on mixed drives / non-comparable paths
+                    in_uploads = False
+                if in_uploads and abs_target != base_abs:
+                    try:
+                        os.remove(abs_target)
+                        deleted_from_disk = True
+                    except FileNotFoundError:
+                        pass  # already gone — race with another request or cleanup
 
-            # Delete file from disk if it's in uploads dir
-            deleted_from_disk = False
-            try:
-                abs_target = os.path.abspath(filepath)
-                base_abs = os.path.abspath(UPLOADS_DIR)
-                in_uploads = (
-                    abs_target == base_abs
-                    or os.path.commonpath([abs_target, base_abs]) == base_abs
-                )
-            except ValueError:
-                # commonpath raises on mixed drives / non-comparable paths
-                in_uploads = False
-            if in_uploads and abs_target != base_abs and os.path.exists(abs_target):
-                os.remove(abs_target)
-                deleted_from_disk = True
+                # Exclude the file from the listing (persists across restarts)
+                personal_docs_manager.exclude_file(filepath)
+                return removed, deleted_from_disk
 
-            # Exclude the file from the listing (persists across restarts)
-            personal_docs_manager.exclude_file(filepath)
+            # Vector removal, the disk unlink and the exclusion write are one
+            # transition over the same state add_directory mutates (#5634), and
+            # all three block. Take the shared job lock BEFORE offloading, as
+            # add_directory does.
+            async with _index_job_lock:
+                removed, deleted_from_disk = await run_in_threadpool(_delete_file)
 
             return {
                 "success": True,