diff --git a/database/health.py b/database/health.py index e065046c..83ffb61d 100644 --- a/database/health.py +++ b/database/health.py @@ -16,18 +16,17 @@ check, and none of them carries a server path. import datetime import logging -import ntpath import os import sqlite3 -from pathlib import PurePosixPath, PureWindowsPath -from urllib.parse import quote try: from ..utils.hashing import register_prompt_functions from .models import normalize_image_path + from .sqlite_uri import readonly_uri except ImportError: from utils.hashing import register_prompt_functions from database.models import normalize_image_path + from database.sqlite_uri import readonly_uri logger = logging.getLogger("prompt_manager.database.health") @@ -89,25 +88,6 @@ def _counted(lines): return _result(count, detail) -def readonly_uri(db_path, *, windows=None): - """The ``mode=ro`` SQLite URI of *db_path*, on this platform or the given one. - - pathlib spells a UNC path ``\\\\server\\share\\x.db`` as - ``file://server/share/x.db``, a remote authority SQLite refuses to open. - With an empty authority (``file:////server/share/x.db``) SQLite hands - Windows the ``//server/share`` path, which it opens as the UNC path. - """ - if windows is None: - windows = os.name == "nt" - db_path = os.path.abspath(db_path) - if not windows: - return PurePosixPath(db_path).as_uri() + "?mode=ro" - path = PureWindowsPath(db_path) - if ntpath.splitdrive(str(path))[0].startswith("\\\\"): - return "file:" + quote("//" + path.as_posix()) + "?mode=ro" - return path.as_uri() + "?mode=ro" - - def _integrity(conn, _roots): rows = [ row[0] for row in conn.execute(f"PRAGMA integrity_check({INTEGRITY_LIMIT})") diff --git a/database/library_move.py b/database/library_move.py index b4451a79..c84a9673 100644 --- a/database/library_move.py +++ b/database/library_move.py @@ -47,11 +47,11 @@ import sqlite3 import time import types import uuid -from pathlib import Path from dataclasses import dataclass, field from typing import Callable, Dict, List, Optional, Tuple from .models import BUSY_TIMEOUT_SECONDS, PromptModel +from .sqlite_uri import readonly_uri try: from ..utils import library_location as ll @@ -322,7 +322,7 @@ def _verify_copy(temp: str, counts, documents) -> None: "verify-failed", f"{MESSAGES['verify-failed']} {detail}" if detail else None, ) - uri = Path(os.path.abspath(temp)).as_uri() + "?mode=ro" + uri = readonly_uri(temp) try: with contextlib.closing(sqlite3.connect(uri, uri=True)) as conn: copied = _describe(conn) diff --git a/database/models.py b/database/models.py index 4c506962..ba8d9653 100644 --- a/database/models.py +++ b/database/models.py @@ -14,6 +14,7 @@ from contextlib import closing from pathlib import Path from typing import Optional, Set, Tuple +from .sqlite_uri import readonly_uri from .managed_connection import ( ManagedConnection, await_closed, @@ -1199,7 +1200,7 @@ class PromptModel: if header != SQLITE_HEADER: return False, "Not a SQLite database file" - uri = Path(os.path.abspath(path)).as_uri() + "?mode=ro" + uri = readonly_uri(path) try: conn = sqlite3.connect(uri, uri=True) except sqlite3.Error as e: @@ -1306,9 +1307,7 @@ class PromptModel: replacement (for example, incompatible page sizes in WAL mode), propagate the error rather than copy files under other open owners. """ - source = sqlite3.connect( - Path(src_path).resolve().as_uri() + "?mode=ro", uri=True - ) + source = sqlite3.connect(readonly_uri(Path(src_path).resolve()), uri=True) try: target = self.get_connection() # The target is written under backup's step too: mark it in use diff --git a/database/restore_stage.py b/database/restore_stage.py index 1a3a3762..c204f1f2 100644 --- a/database/restore_stage.py +++ b/database/restore_stage.py @@ -42,6 +42,7 @@ from typing import Dict, Optional from . import workflow_documents as wd from .execution_history_schema import rotate_history_epoch from .safety_backup import take_safety_backup +from .sqlite_uri import readonly_uri logger = logging.getLogger("prompt_manager.database.restore_stage") @@ -114,7 +115,7 @@ def file_digest(path: str) -> str: def _readonly(path: str) -> sqlite3.Connection: - return sqlite3.connect(Path(path).resolve().as_uri() + "?mode=ro", uri=True) + return sqlite3.connect(readonly_uri(Path(path).resolve()), uri=True) def _schema(path: str) -> list: diff --git a/database/sqlite_uri.py b/database/sqlite_uri.py new file mode 100644 index 00000000..07e22362 --- /dev/null +++ b/database/sqlite_uri.py @@ -0,0 +1,25 @@ +"""The read-only SQLite URI of a path, safe for Windows network shares.""" + +import ntpath +import os +from pathlib import PurePosixPath, PureWindowsPath +from urllib.parse import quote + + +def readonly_uri(db_path, *, windows=None): + """The ``mode=ro`` SQLite URI of *db_path*, on this platform or the given one. + + pathlib spells a UNC path ``\\\\server\\share\\x.db`` as + ``file://server/share/x.db``, a remote authority SQLite refuses to open. + With an empty authority (``file:////server/share/x.db``) SQLite hands + Windows the ``//server/share`` path, which it opens as the UNC path. + """ + if windows is None: + windows = os.name == "nt" + db_path = os.path.abspath(db_path) + if not windows: + return PurePosixPath(db_path).as_uri() + "?mode=ro" + path = PureWindowsPath(db_path) + if ntpath.splitdrive(str(path))[0].startswith("\\\\"): + return "file:" + quote("//" + path.as_posix()) + "?mode=ro" + return path.as_uri() + "?mode=ro" diff --git a/py/api/admin.py b/py/api/admin.py index f0ea05f6..93833709 100644 --- a/py/api/admin.py +++ b/py/api/admin.py @@ -30,8 +30,10 @@ from .jobs import ( ) try: + from ...database.sqlite_uri import readonly_uri from ...utils.parallel import map_parallel except ImportError: + from database.sqlite_uri import readonly_uri from utils.parallel import map_parallel try: @@ -399,7 +401,7 @@ def _inspect_restore_upload(path): Returns: None when acceptable, otherwise a human-readable reason (blocking). """ - uri = Path(os.path.abspath(path)).as_uri() + "?mode=ro" + uri = readonly_uri(path) try: conn = sqlite3.connect(uri, uri=True) except sqlite3.Error as e: diff --git a/py/api/autotag_routes.py b/py/api/autotag_routes.py index ccc7fd5f..d751763c 100644 --- a/py/api/autotag_routes.py +++ b/py/api/autotag_routes.py @@ -1411,7 +1411,8 @@ class AutotagRoutesMixin: ) model_type = service.get_loaded_model_type() - service.unload_model() + # Off the loop; a dropped request keeps the gate until it ends. + await model_gate.run(owner, service.unload_model) finally: model_gate.release(owner) diff --git a/py/api/backup_restore.py b/py/api/backup_restore.py index ab3d030a..0e765c92 100644 --- a/py/api/backup_restore.py +++ b/py/api/backup_restore.py @@ -45,7 +45,6 @@ import os import sqlite3 import tempfile from contextlib import closing -from pathlib import Path from aiohttp import web @@ -56,6 +55,7 @@ from .maintenance_plans import PlanExpired, PlanUsed from .restore_plans import RestorePlanStore try: + from ...database.sqlite_uri import readonly_uri from ...database import restore_stage from ...database.operations import PromptDatabase from ...database.safety_backup import ( @@ -66,6 +66,7 @@ try: safety_temp, ) except ImportError: + from database.sqlite_uri import readonly_uri from database import restore_stage from database.operations import PromptDatabase from database.safety_backup import ( @@ -186,7 +187,7 @@ def snapshot_corruption(path): with open(path, "rb") as handle: if handle.read(len(SQLITE_HEADER)) != SQLITE_HEADER: return "Not a SQLite database file" - uri = Path(os.path.abspath(path)).as_uri() + "?mode=ro" + uri = readonly_uri(path) with closing(sqlite3.connect(uri, uri=True)) as conn: row = conn.execute("PRAGMA integrity_check").fetchone() except (OSError, sqlite3.Error) as error: diff --git a/py/api/prompts.py b/py/api/prompts.py index de2c6716..3ce25cd2 100644 --- a/py/api/prompts.py +++ b/py/api/prompts.py @@ -727,7 +727,15 @@ class PromptRoutesMixin: async def save_prompt(self, request): """Save a new prompt with metadata and duplicate detection.""" try: - data = await request.json() + data, error = await self._read_json_body(request) + if error is not None: + return error + for field in ("text", "category", "notes"): + if data.get(field) is not None and not isinstance(data[field], str): + return web.json_response( + {"success": False, "error": f"{field} must be text"}, + status=400, + ) text = (data.get("text") or "").strip() if not text: diff --git a/tests/test_api_autotag.py b/tests/test_api_autotag.py index ae643f9f..a3bffda0 100644 --- a/tests/test_api_autotag.py +++ b/tests/test_api_autotag.py @@ -732,6 +732,32 @@ class TestAutotagErrorBodiesHideServerPaths(AutotagAPITestCase): class TestUnloadAutotag(AutotagAPITestCase): + async def test_unload_runs_off_the_loop_and_holds_the_gate_until_done(self): + started = threading.Event() + release = threading.Event() + self.addCleanup(release.set) + threads = [] + + def unload(): + threads.append(threading.get_ident()) + started.set() + release.wait(5) + self.service._loaded_type = None + + self.service._loaded_type = "gguf" + self.service.unload_model = unload + first = asyncio.ensure_future( + self.client.request("POST", "/prompt_manager/autotag/unload") + ) + self.assertTrue(await asyncio.to_thread(started.wait, 5)) + self.assertNotEqual(threads, [threading.get_ident()]) + second = await self.client.request("POST", "/prompt_manager/autotag/unload") + self.assertEqual(second.status, 409) + release.set() + data = await (await first).json() + self.assertEqual(data["message"], "GGUF model unloaded successfully") + self.assertIsNone(model_gate.holder()) + async def test_nothing_loaded(self): resp = await self.client.request("POST", "/prompt_manager/autotag/unload") data = await resp.json() diff --git a/tests/test_api_prompts.py b/tests/test_api_prompts.py index c8625c25..895d2bad 100644 --- a/tests/test_api_prompts.py +++ b/tests/test_api_prompts.py @@ -684,6 +684,28 @@ class TestTagMutationRoutes(RouteCoverageCase): class TestSaveRoute(RouteCoverageCase): + async def test_a_malformed_body_is_refused_and_saves_nothing(self): + for body in ( + [], + "text", + {"text": 123}, + {"text": "fixture", "category": 123}, + {"text": "fixture", "notes": 123}, + {"text": "fixture", "tags": 123}, + {"text": "fixture", "rating": "high"}, + ): + with self.subTest(body=body): + status, data = await self._json( + "POST", "/prompt_manager/save", json=body + ) + self.assertEqual(status, 400) + self.assertFalse(data["success"]) + status, data = await self._json( + "POST", "/prompt_manager/save", data=b"{not json" + ) + self.assertEqual(status, 400) + self.assertEqual(self.api.db.search_prompts(text="fixture"), []) + async def test_invalid_rating_is_400(self): status, data = await self._json( "POST", "/prompt_manager/save", json={"text": "ok", "rating": 99} diff --git a/tests/test_sqlite_uri.py b/tests/test_sqlite_uri.py new file mode 100644 index 00000000..bc8ed98d --- /dev/null +++ b/tests/test_sqlite_uri.py @@ -0,0 +1,39 @@ +"""Every read-only SQLite open uses one URI helper (#620). + +pathlib writes a Windows network path as ``file://server/share/x.db``, which +SQLite refuses. A reader that builds its own URI breaks backups, restores, +library moves or history reads for a library on a network share. +""" + +import re +import sys +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(ROOT)) + +from database.sqlite_uri import readonly_uri # noqa: E402 + + +class TestReadonlyUri(unittest.TestCase): + def test_a_network_path_has_an_empty_authority(self): + self.assertEqual( + readonly_uri(r"\\server\share\Comfy UI\prompts.db", windows=True), + "file:////server/share/Comfy%20UI/prompts.db?mode=ro", + ) + + def test_no_reader_builds_its_own_uri(self): + offenders = [] + for folder in ("database", "py", "utils"): + for path in (ROOT / folder).rglob("*.py"): + if path.name == "sqlite_uri.py": + continue + text = path.read_text(encoding="utf-8") + if re.search(r"as_uri\(\)\s*\+\s*[\"']\?mode=ro", text): + offenders.append(path.relative_to(ROOT).as_posix()) + self.assertEqual(offenders, []) + + +if __name__ == "__main__": + unittest.main() diff --git a/utils/history_read_fence.py b/utils/history_read_fence.py index dd78c61b..2213aba5 100644 --- a/utils/history_read_fence.py +++ b/utils/history_read_fence.py @@ -1,7 +1,6 @@ """Restore publication checks for snapshot-owned history reads.""" import os -from pathlib import Path import sqlite3 import threading from typing import TYPE_CHECKING @@ -10,11 +9,13 @@ if TYPE_CHECKING: from ..database.operations import PromptDatabase try: + from ..database.sqlite_uri import readonly_uri from ..database.execution_history import ( HistoryEpochChanged, subscribe_history_restore, ) except ImportError: + from database.sqlite_uri import readonly_uri from database.execution_history import ( HistoryEpochChanged, subscribe_history_restore, @@ -53,9 +54,7 @@ class HistoryReadFence: self._check(generation, epoch) # An API restore retires its previous model. Never reopen that owner, # and never reuse the projection transaction's view of the epoch. - conn = sqlite3.connect( - Path(self._path).as_uri() + "?mode=ro", uri=True, timeout=0.25 - ) + conn = sqlite3.connect(readonly_uri(self._path), uri=True, timeout=0.25) try: row = conn.execute( "SELECT storage_epoch FROM execution_history_metadata "