diff --git a/.gitignore b/.gitignore index 57731696..0f8fd356 100644 --- a/.gitignore +++ b/.gitignore @@ -214,3 +214,7 @@ coverage/ # Old library files a Move library renamed (never deleted; #249 S6b) *.moved-to-* + +# Per-install key behind the workspace scopes and cursors (#430) +workspace-scope.key +.workspace-scope.key.*.tmp diff --git a/TESTING.md b/TESTING.md index 7dacb574..0f4b3233 100644 --- a/TESTING.md +++ b/TESTING.md @@ -551,6 +551,49 @@ hold a walk open with a gated scan instead of sleeping. time to network idle and the 10 slowest requests, from a fresh browser context each run. +## Open pages across restarts and source changes (#430) + +A page addresses a listed file as `{root_index, root_scope, path}`. The root +scope is an HMAC over the root's index and canonical path, so the server refuses +(409 `source-changed`) an action aimed at a root that was renumbered or replaced +since the page listed it (a same-named `ComfyUI_00001_.png` sits in every output +folder). Its key, and the listing cursors' key, derive from one random secret in +`workspace-scope.key` beside the database (`utils/workspace_keys.py`; owner-only, +64 hex characters; never in config.json or the database, so no backup or export +carries it; Move library copies it, best effort). A ComfyUI restart therefore keeps +every scope and cursor an open page holds: `tests/test_api_workspace_restart.py` +proves it with a second API on its own server and with two fresh interpreters on +the same database. The key file is created with a temp file and `os.link` (of two +first starts one file wins; without hard links `O_EXCL`), re-read after any write, +replaced with a WARNING when corrupt, and a folder that cannot hold it gives that +process a per-process key (one WARNING; open pages then refresh after a restart). +Tests never write it outside their own temp folders. + +The cursor is `{"v": 2, "key"}` with the listing scope (it hashes every absolute +root) bound into its MAC only, never sent. Bump `v` whenever the key tuple's shape +changes: a cursor now outlives a restart and an upgrade. A cursor the server +cannot verify (another key, root list or filter set) answers 400 +`cursor-stale`; Images then shows "The image collection changed. Refresh image +collection to continue." and its Retry becomes **Refresh image collection**. + +When a source really changed, `POST /prompt_manager/images/rescope` (`{"target": +{...}}`, validated by `media_target`, index ≤ 4095) maps the held target to the +first current root whose scope at the old index equals the held scope, and answers +the file's current workspace descriptor; a removed or replaced root, another key +or a foreign scope is 409 `source-changed`, a missing file 404 `file-missing`, a +path leaving the root or a preview 403 (always `no-store`). Only the Images roots +are mapped. Images (`images-source-recovery.mjs`) asks once per held +`(key, scope)` pair, shared by the inspector (an inspection that turns +`refreshRequired` with `code: "source-changed"`), the viewer (a media load error) +and the Tag image dialog. Found under another index, nothing is kept by an old key +(`root_index:path` could name another source's file): the viewer closes, the checks +clear, a Source filter on the old index follows the file (another one falls back to +All sources), only that file stays selected and the collection lists again; a +manual Refresh forgets every pair. Not found, the existing visible Refresh stays. +Browser tests: `workspace-stale-source.spec.mjs` (`/__test__/sources/reverse` and +`/__test__/scope-key/rotate`, both undone by every reset); the manual-Refresh +recovery specs in `workspace-images.spec.mjs` answer every rescope 409. + ## Windows portability preflight Before changing paths, SQLite, streams, restore or cleanup: @@ -1697,10 +1740,11 @@ wins), and toasts the mock's copy verbatim, " tags saved for ." eve for one tag, plus " Prompt association still awaits review." from Needs review. Tagging one file never changes another file (an equal-byte copy included), a prompt, a pair or a link. States the mock lacks (generating, busy, conflict, -changed storage or file, expired capture, unconfirmed save, unavailable model, -video) are text in `#tag-generation-status` only; **Retry** (D116) leads the -actions while the controller allows it, where the other workspace dialogs place -their Retry. The Tags +unavailable model, video) are text in `#tag-generation-status` only; a state that +blocks Save (a failed or refused capture, a changed source, file or storage, an +expired capture, an unconfirmed save) shows its reason beside the actions instead +(D147); **Retry** (D116) leads the action buttons while the controller allows it, +where the other workspace dialogs place their Retry. The Tags page re-reads its lists after every confirmed save, so the Images counts follow. The image-tagging plan's deviation numbers were renumbered by +23 because the @@ -1744,8 +1788,8 @@ Inspect plan keeps D60–D66 (plan D60 → D83, D61 → D84, D65 → D88, D68 its "Auto-tag images" always opens the dialog. Production opens it for the image selected in Images when it was last left; with none it is `aria-disabled` and pressing it toasts "Select an image in Images first." (no new chrome). -- **D116** (ruling T5): **Retry** leads `.dialog-actions` (before Cancel and the - primary) while a failed observation, a failed Generate, an unconfirmed save or an +- **D116** (ruling T5): **Retry** leads the buttons of `.dialog-actions` (before + Cancel and the primary; D147's notice row sits above them) while a failed observation, a failed Generate, an unconfirmed save or an answer that needs a fresh observation (changed storage or file, expired capture) can be retried; the concept has no failure states, so it has no Retry. After a failed Generate it takes Generate's own path (an invalid field reports its @@ -1756,6 +1800,25 @@ Inspect plan keeps D60–D66 (plan D60 → D83, D61 → D84, D65 → D88, D68 Browser tests: "D116: Retry leads the actions after an unconfirmed save…", "D116: Retry after an expired capture observes again and keeps the typed draft (M5)" and the paired `production-unknown-outcome` capture. +- **D147** (#430, beyond the Global Constraints' "status text only"): a state + that blocks Save shows its reason in `div.tag-action-notice` (its sentence in a + `role="status"` span, written only when it changes), the first, full-width row of + the sticky `.dialog-actions`, above the buttons; `#tag-generation-status` is then + empty, so the sentence shows once. Generate and Save are `aria-disabled` and look + it (`.tagging-modal [aria-disabled="true"]`: opacity .55, `not-allowed`). A failed + or refused capture replaces the empty preview with a same-size + `div.tag-image-preview.tag-preview-empty` "Preview unavailable" (`role="img"`). + A changed source (409 `source-changed` from observe, Generate or Save, phase + `source-changed`, never retried as is) first asks the page where the file is + (`onSourceChanged`, see "Open pages across restarts and source changes"): found, + the dialog stays open and observes it there (`controller.retarget`, the typed + draft kept, the old capture released; refused while a save is in flight); not + found, the notice shows the server's sentence and **Refresh image collection** + (Images and Library-embedded galleries; **Reload page** elsewhere), which closes + the dialog first. Opened, generated and empty states are unchanged, so the concept + comparisons are not. Browser tests: `tests/browser/workspace-stale-source.spec.mjs`; + the file-changed and unconfirmed-save checks in `workspace-tagging.spec.mjs` and + `tagging-concept.spec.mjs` read the notice. - **D29** (no prototype copy) in the dialog: no "Prototype · suggestions are simulated…" note, the status reads "Suggestions from . Approve tags to save." (not "Sample suggestions from …"), and the toast has no " in this tab". @@ -1808,7 +1871,8 @@ production-versus-concept difference of PR C and its record: | Image tags first in the inspector and viewer chip row | D88 (ruling T7/C2) | | Auto-tag images `aria-disabled` without a selected image | D115 | | Retry leads the actions when the controller allows it | D116 (ruling T5) | -| States the concept lacks (busy, conflict, changed storage or file, unconfirmed save, unavailable model, video) | Global Constraints: `#tag-generation-status` text only | +| States the concept lacks (busy, conflict, unavailable model, video) | Global Constraints: `#tag-generation-status` text only | +| A state that blocks Save (failed capture, changed source, file or storage, unconfirmed save): its reason and Refresh beside the actions, a "Preview unavailable" box | D147 (#430) | ### Bulk tagging from the selection bar (#249 PR D) @@ -3377,7 +3441,11 @@ the per-section hand-off panel, was retired by S12 (#329): every section is nati mid-response is not discarded by the page: the server's 15-minute expiry removes it. When a restore the page saw running finishes, the cached Images and Library visits are dropped, the sidebar counts and saved views are read again and the open route is shown again (the Activity job view - stays). A refused file toasts "This file can't be restored: .". A + stays). #415: an open review (`openReviewDialog`) then closes as dismissed, since its plan no + longer describes storage; while any other dialog is open, all of that waits until the last + one has closed, so a dialog keeps what was typed and its next write meets the restored-storage + refusal, as when the page never saw the restore. `followRestore` (tests/browser/unseen-restore.mjs) + makes the page see a restore run and finish deterministically. A refused file toasts "This file can't be restored: .". A review value may wrap anywhere (`#modal-body > dl.meta-grid dd`, CSS under D105), so a long file name keeps the concept's two equal columns at phone width. - **D106** (Integrations, S10, Q8): the send bridge always asks for the ComfyUI window diff --git a/database/library_move.py b/database/library_move.py index 1cec52e6..91dc7274 100644 --- a/database/library_move.py +++ b/database/library_move.py @@ -56,9 +56,11 @@ from .models import BUSY_TIMEOUT_SECONDS, PromptModel try: from ..utils import library_location as ll from ..utils.logging_config import get_logger + from ..utils.workspace_keys import KEY_FILE_NAME except ImportError: from utils import library_location as ll from utils.logging_config import get_logger + from utils.workspace_keys import KEY_FILE_NAME logger = get_logger("prompt_manager.database.library_move") @@ -491,7 +493,8 @@ class _Move: sweep_move_temps(target) if ll.read_marker(target).get("state") == ll.MARKER_PENDING: # An earlier move stopped before it switched: its files are ours. - for name in (ll.DATABASE_NAME, ll.CONFIG_NAME) + self.plan.backups: + names = (ll.DATABASE_NAME, ll.CONFIG_NAME, KEY_FILE_NAME) + for name in names + self.plan.backups: _remove(os.path.join(target, name)) for suffix in ("-wal", "-shm", "-journal"): _remove(os.path.join(target, ll.DATABASE_NAME + suffix)) @@ -536,6 +539,7 @@ class _Move: raise MoveFailed("copy-failed") from error _check_copies([(self.plan.config_path, temp, expected)]) copies = copies + [(self.plan.config_path, temp, expected, ll.CONFIG_NAME)] + copies = copies + self._key_copy() self.marker = write_marker( target, ll.MARKER_PENDING, self.plan.db_path, self.plan.config_path ) @@ -556,6 +560,28 @@ class _Move: self.checkpoint("marked") return files + def _key_copy(self) -> list: + """The workspace key's verified copy (#430), or none: it is re-creatable. + + Without it the moved library starts with a new key, so pages left open + across the restart refresh once; that never fails the move. + """ + source = os.path.join(self.plan.source_dir, KEY_FILE_NAME) + if not os.path.isfile(source): + return [] + temp = self.temp(".key") + try: + expected = _hash_copy(source, temp) + _check_copies([(source, temp, expected)]) + except (OSError, MoveFailed) as error: + _remove(temp) + logger.warning( + f"Library move: the workspace key was not copied ({error}); " + "open workspace pages refresh once after the restart" + ) + return [] + return [(source, temp, expected, KEY_FILE_NAME)] + def cleanup(self) -> None: if self.moved: return diff --git a/docs/application-redesign.md b/docs/application-redesign.md index 7df56194..536464d0 100644 --- a/docs/application-redesign.md +++ b/docs/application-redesign.md @@ -12,7 +12,7 @@ code. This plan proposes implementation order and acceptance gates. - The native Prompt Library merged in [#210](https://github.com/ComfyAssets/ComfyUI_PromptManager/pull/210) for [#208](https://github.com/ComfyAssets/ComfyUI_PromptManager/issues/208): one positive parent, saved negative variants, full selected text and a shared filtered image gallery. It does not invent chronological runs from counters or scan dates. - [Durable history storage](prompt-history-storage.md) records job attempts and outcomes from [#212](https://github.com/ComfyAssets/ComfyUI_PromptManager/pull/212), frozen prompt contexts from [#213](https://github.com/ComfyAssets/ComfyUI_PromptManager/pull/213), and observed run-owned output locators from [#214](https://github.com/ComfyAssets/ComfyUI_PromptManager/pull/214). Those locators do not verify file bytes or establish gallery-image associations. That association contract and the chronological execution timeline remain pending under [#209](https://github.com/ComfyAssets/ComfyUI_PromptManager/issues/209). - [#215](https://github.com/ComfyAssets/ComfyUI_PromptManager/issues/215) brings the shell, Images and natural-height Prompt Library presentation into the approved geometry and themes while retaining their existing operations. [#216](https://github.com/ComfyAssets/ComfyUI_PromptManager/issues/216) adds the shared current-file inspector with exact generation facts, scoped provenance, raw/download evidence and loaded-collection viewer controls, including a slideshow that pauses at native video. This delivery does not establish whole-application parity, change the default entry point or constitute a release. Recorded chronological run/context browsing continues under [#209](https://github.com/ComfyAssets/ComfyUI_PromptManager/issues/209); image tagging/review, System consolidation and remaining organization capabilities continue under #196. -- Local Inspect (#249, PRs L1–L2) replaces the `#/inspect` handoff with the concept's native "Inspect a file" page: one local image or video previews inline and opens the shared viewer and full inspector on a record read entirely in the browser, with nothing uploaded, imported or stored. The legacy metadata page (`web/metadata.html`, served at `/prompt_manager/gallery.html`) stays reachable from Overview; its capabilities map to the shared inspector, except its one-click COPY ALL summary, whose ruled replacement ("Copy settings summary") is pending. Recorded deviations D60–D66 are in [TESTING.md](../TESTING.md). Local roles equal to the server's resolver follow in PR L3. +- Local Inspect (#249, PRs L1–L3) replaces the `#/inspect` handoff with the concept's native "Inspect a file" page: one local image or video previews inline and opens the shared viewer and full inspector on a record read entirely in the browser, with nothing uploaded, imported or stored. The legacy metadata page (`web/metadata.html`, served at `/prompt_manager/gallery.html`) stays reachable from Overview; its capabilities map to the shared inspector, including its one-click COPY ALL summary, which is the shared inspector's "Copy settings summary" (D117, LI-4). PR L3 ([#286](https://github.com/ComfyAssets/ComfyUI_PromptManager/pull/286)) shipped that button and local roles equal to the server's resolver (`web/js/workspace/local-resolver.mjs`, TESTING.md "Local roles (#249 PR L3, decision LI-1)"). Recorded deviations D60–D66 and D117 are in [TESTING.md](../TESTING.md). ## Scope and compatibility diff --git a/py/api/__init__.py b/py/api/__init__.py index bd6ce9af..b2376662 100644 --- a/py/api/__init__.py +++ b/py/api/__init__.py @@ -74,6 +74,7 @@ from .autotag_routes import AutotagRoutesMixin from .lora_integration import LoraIntegrationMixin from .lora_import_routes import LoraImportRoutesMixin from .image_categories import ImageCategoryRoutesMixin +from .source_rescope import SourceRescopeMixin try: from ...database.operations import PromptDatabase @@ -342,6 +343,7 @@ class PromptManagerAPI( LoraIntegrationMixin, LoraImportRoutesMixin, ImageCategoryRoutesMixin, + SourceRescopeMixin, ): """REST API handler for PromptManager operations and web interface. @@ -708,6 +710,7 @@ class PromptManagerAPI( self._register_lora_routes(routes) self._register_lora_import_routes(routes) self._register_image_category_routes(routes) + self._register_rescope_routes(routes) # Register gzip compression middleware (once) global _gzip_registered diff --git a/py/api/images.py b/py/api/images.py index 614287b9..4726d995 100644 --- a/py/api/images.py +++ b/py/api/images.py @@ -13,6 +13,7 @@ import re import stat import subprocess # nosec B404 - ffmpeg frames through run_encoder, fixed argv import sys +import threading import time as _time import urllib.parse from bisect import bisect_left, bisect_right, insort @@ -81,6 +82,7 @@ try: reveal_executor, reveal_in_file_manager, ) + from ...utils.workspace_keys import gallery_root_scope, workspace_keys_for except ImportError: from database.image_collections import CollectionMissing from database.image_favorites import MAX_KEY_LENGTH @@ -105,6 +107,7 @@ except ImportError: reveal_executor, reveal_in_file_manager, ) + from utils.workspace_keys import gallery_root_scope, workspace_keys_for IMAGE_EXTENSIONS = _IMAGE_EXTENSIONS @@ -518,7 +521,8 @@ MAX_JSON_BODY_BYTES = 1024 * 1024 _READ_CHUNK = 64 * 1024 -_WORKSPACE_CURSOR_SECRET = os.urandom(32) +# Guards the first load of an API's workspace keys (#430, _workspace_keys). +_WORKSPACE_KEYS_LOCK = threading.Lock() def _workspace_root(query: dict, roots: list[Path]) -> int | None: @@ -618,15 +622,37 @@ def _rated_key(entry: _Listed, rank: int) -> tuple: return (-rank, *_workspace_key(entry, "rating")) -def _workspace_cursor(key: tuple, scope: str) -> str: +# Bump whenever the cursor's key tuple changes shape: a cursor now outlives a +# restart and an upgrade (#430), so an old shape must be refused, not parsed. +_CURSOR_VERSION = 2 +_CURSOR_STALE = "The image collection changed; refresh the gallery" + + +class CursorStale(ValueError): + """A well-formed cursor this listing cannot verify (#430). + + Another key, another root list or other filters: the page refreshes its + collection instead of retrying the same cursor. + """ + + +def _cursor_mac(secret: bytes, scope: str, payload: bytes) -> bytes: + # The listing scope (it hashes every absolute root) is bound in the MAC + # only, never sent: the scope is 64 hex, so the "\n" join is unambiguous. + return hmac.new(secret, scope.encode() + b"\n" + payload, hashlib.sha256).digest() + + +def _workspace_cursor(key: tuple, scope: str, secret: bytes) -> str: + """The signed keyset cursor of *key*; *secret* is the API's cursor key.""" payload = json.dumps( - {"v": 1, "scope": scope, "key": key}, separators=(",", ":"), ensure_ascii=False + {"v": _CURSOR_VERSION, "key": key}, separators=(",", ":"), ensure_ascii=False ).encode() - signature = hmac.new(_WORKSPACE_CURSOR_SECRET, payload, hashlib.sha256).digest() + signature = _cursor_mac(secret, scope, payload) return base64.urlsafe_b64encode(signature + payload).decode().rstrip("=") -def _workspace_cursor_key(token: str, scope: str, order: str) -> tuple: +def _workspace_cursor_key(token: str, scope: str, order: str, secret: bytes) -> tuple: + """The keyset key of *token*; CursorStale when it is not this listing's.""" try: if len(token) > 4096: raise ValueError() @@ -634,13 +660,14 @@ def _workspace_cursor_key(token: str, scope: str, order: str) -> tuple: token + "=" * (-len(token) % 4), altchars=b"-_", validate=True ) signature, payload = raw[:32], raw[32:] - expected = hmac.new(_WORKSPACE_CURSOR_SECRET, payload, hashlib.sha256).digest() - if not hmac.compare_digest(signature, expected): + if len(signature) != 32 or not payload: raise ValueError() + if not hmac.compare_digest(signature, _cursor_mac(secret, scope, payload)): + raise CursorStale(_CURSOR_STALE) data = json.loads(payload) key = data["key"] - if data["v"] != 1 or data["scope"] != scope or not isinstance(key, list): - raise ValueError() + if data["v"] != _CURSOR_VERSION or not isinstance(key, list): + raise CursorStale(_CURSOR_STALE) types = ( (str, str, int, (float, int)) if order in ("name", "name-desc") @@ -654,6 +681,8 @@ def _workspace_cursor_key(token: str, scope: str, order: str) -> tuple: ): raise ValueError() return tuple(key) + except CursorStale: + raise except (ValueError, KeyError, TypeError, UnicodeError, binascii.Error): raise ValueError("Invalid gallery cursor; refresh the gallery") from None @@ -1338,13 +1367,27 @@ def _in_subfolder(rel_dir, subfolder): class ImageRoutesMixin: """Mixin providing image and gallery-related API endpoints.""" + def _workspace_keys(self): + """This API's scope and cursor keys, loaded once (#430). + + They derive from the secret persisted beside the database, so a restart + keeps every scope and cursor an open page holds. The first call reads + (or writes) one small file; later calls return the cached keys, which + a live library move or restore does not change before a restart. + """ + keys = self.__dict__.get("_workspace_keys_cache") + if keys is None: + with _WORKSPACE_KEYS_LOCK: + keys = self.__dict__.get("_workspace_keys_cache") + if keys is None: + db = getattr(self, "db", None) + model = getattr(db, "model", None) + keys = workspace_keys_for(getattr(model, "db_path", None)) + self.__dict__["_workspace_keys_cache"] = keys + return keys + def _gallery_root_scope(self, root: Path, index: int) -> str: - payload = json.dumps( - ["prompt-manager-gallery-root-v1", index, str(_canonical(root))], - separators=(",", ":"), - ensure_ascii=False, - ).encode("utf-8") - return hmac.new(_WORKSPACE_CURSOR_SECRET, payload, hashlib.sha256).hexdigest() + return gallery_root_scope(self._workspace_keys(), str(_canonical(root)), index) def _gallery_scope_error( self, query: Mapping, roots: list[Path] @@ -2008,6 +2051,8 @@ class ImageRoutesMixin: limit, _offset = parse_page_params(request.query, default_limit=100) except ValueError: return bad_request("limit and offset must be integers") + # #430: the first use may create the key file; never on the event loop. + cursor_secret = (await self._run_in_executor(self._workspace_keys)).cursor try: root_index = _workspace_root(request.query, output_dirs) filters = _workspace_filters(request.query) @@ -2031,7 +2076,15 @@ class ImageRoutesMixin: ).hexdigest() token = request.query.get("cursor") after = ( - _workspace_cursor_key(token, scope, filters["order"]) if token else None + _workspace_cursor_key(token, scope, filters["order"], cursor_secret) + if token + else None + ) + except CursorStale as exc: + return web.json_response( + {"success": False, "code": "cursor-stale", "error": str(exc)}, + status=400, + headers={"Cache-Control": "no-store"}, ) except ValueError as exc: return bad_request(str(exc)) @@ -2472,7 +2525,11 @@ class ImageRoutesMixin: positive_count = _COUNT_OMITTED has_more = len(page) > limit page = page[:limit] - next_cursor = _workspace_cursor(key(page[-1]), scope) if has_more else None + next_cursor = ( + _workspace_cursor(key(page[-1]), scope, self._workspace_keys().cursor) + if has_more + else None + ) return ( [(entry.path, entry.mtime, entry.root, entry.index) for entry in page], total, diff --git a/py/api/source_rescope.py b/py/api/source_rescope.py new file mode 100644 index 00000000..caa9bfa9 --- /dev/null +++ b/py/api/source_rescope.py @@ -0,0 +1,100 @@ +"""Where a stale Images target is now (#430): ``POST /prompt_manager/images/rescope``. + +A page addresses a listed file as ``{root_index, root_scope, path}``. When the +sources are renumbered its scope no longer names the root at that index, and +every per-file action answers 409 ``source-changed``. Before it gives up, the +page asks here. The target is mapped only when its scope is, at its old +index, the scope of one of the current roots: the server issued that scope +for exactly that folder, so the answer (the file's current listing +descriptor) names nothing the page could not already address. A removed or +replaced root, a scope of another key and a foreign scope stay refused. + +Only the Images roots (``_get_all_output_dirs``) are mapped; a LoRA-linked +root's target is never re-scoped here. +""" + +import hmac +from pathlib import Path + +from aiohttp import web + +from .images import ( + _forbidden, + _is_thumbnail_cache_path, + _json_body_or_error, + _media_access_error, +) +from .media_target import media_target +from .prompts import bad_request + +# Far above any real source count; keeps the index out of huge-int territory. +MAX_ROOT_INDEX = 4095 +SOURCE_GONE = ( + "The source of this image changed or was removed. Refresh the image " + "collection to find it again." +) + + +def _coded(status: int, code: str, message: str) -> web.Response: + return web.json_response( + {"success": False, "code": code, "error": message}, status=status + ) + + +def _no_store(response: web.Response) -> web.Response: + response.headers["Cache-Control"] = "no-store" + return response + + +class SourceRescopeMixin: + """The rescope route; composed into ``PromptManagerAPI``.""" + + def _register_rescope_routes(self, routes): + @routes.post("/prompt_manager/images/rescope") + async def rescope_route(request): + return _no_store(await self.rescope_image(request)) + + def _current_root_for(self, scope: str, old_index: int, roots: list) -> int | None: + """The index of the first current root *scope* was issued for at *old_index*.""" + for index, root in enumerate(roots): + if hmac.compare_digest(scope, self._gallery_root_scope(root, old_index)): + return index + return None + + def _rescope_blocking(self, scope: str, old_index: int, relative: str): + """The answer to one rescope (blocking: it resolves paths).""" + roots = list(self._get_all_output_dirs()) + index = self._current_root_for(scope, old_index, roots) + if index is None: + return _coded(409, "source-changed", SOURCE_GONE) + root = Path(roots[index]) + path = root / relative + denied = _media_access_error(path, [root]) + if denied is not None: + return denied + try: + mtime = path.stat().st_mtime if path.is_file() else None + except OSError: + mtime = None + if mtime is None: + return _coded(404, "file-missing", "Image file not found") + [image] = self._format_workspace_page([(path, mtime, root, index)]) + return web.json_response({"success": True, "image": image}) + + async def rescope_image(self, request) -> web.Response: + data, error = await _json_body_or_error(request) + if error is not None: + return error + if set(data) != {"target"}: + return bad_request("Send exactly one target") + target, error = media_target(data["target"]) + if error is not None: + return error + old_index, scope, relative = target + if old_index > MAX_ROOT_INDEX: + return bad_request("Invalid gallery root") + if _is_thumbnail_cache_path(relative): + return _forbidden("Previews are not re-scoped; use the original") + return await self._run_in_executor( + self._rescope_blocking, scope, old_index, relative + ) diff --git a/tests/browser/fast_reset.py b/tests/browser/fast_reset.py index c305c686..9706b13f 100644 --- a/tests/browser/fast_reset.py +++ b/tests/browser/fast_reset.py @@ -28,6 +28,11 @@ import weakref from pathlib import Path from typing import Any, Callable, TypeVar +try: + import fcntl +except ImportError: # Windows: no flock, and no /dev/shm to share + fcntl = None + T = TypeVar("T") # Set to 1 to run seeders on the live database as before #384 (the equivalence @@ -43,8 +48,13 @@ MEMORY_ROOT = Path("/dev/shm") # the browsers and the other workers; with less free than this, the scratch # copy goes on disk instead. MEMORY_FREE_BYTES = 32 * 1024 * 1024 -# Scratch folders are named -. +# Scratch folders are created as new--, locked, then +# renamed to held--. The pid is for people only: +# ownership is the flock on OWNER_LOCK, which every PID namespace sees (#419). SCRATCH_PREFIX = "pm-reset-" +STAGING_PREFIX = f"{SCRATCH_PREFIX}new-" +HELD_PREFIX = f"{SCRATCH_PREFIX}held-" +OWNER_LOCK = "owner.lock" def _quote(name: str) -> str: @@ -96,29 +106,40 @@ def table_digests(conn: sqlite3.Connection, schema: str = "main") -> dict[str, s return result -def _process_alive(pid: int) -> bool: +def _hold(folder: Path, *, create: bool) -> int: + """Take ``folder``'s owner lock without waiting; the open descriptor.""" + flags = os.O_RDWR | (os.O_CREAT if create else 0) + fd = os.open(folder / OWNER_LOCK, flags, 0o600) try: - os.kill(pid, 0) - except ProcessLookupError: - return False + fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) except OSError: - return True # it exists but belongs to someone else - return True + os.close(fd) + raise + return fd def remove_orphaned_scratch(root: Path) -> None: """Remove scratch folders of servers that died without cleaning up. A server killed outright (SIGKILL, a crash) never runs ``close``, and - tmpfs keeps its folder in memory until reboot. The folder name carries - the owner's process id; a folder whose process is gone is removed. + tmpfs keeps its folder in memory until reboot. A live owner holds the + flock on the folder's owner lock; the kernel drops it when the owner + dies, in whatever PID namespace it ran (a container with ``--ipc=host`` + shares /dev/shm but not the pids, #419). A folder whose lock is free is + removed. Folders without a lock (staging, or named before #419) cannot + be proven orphaned and are left alone. """ - if os.name != "posix": - return # os.kill(pid, 0) would terminate the process on Windows - for folder in root.glob(f"{SCRATCH_PREFIX}*-*"): - pid = folder.name[len(SCRATCH_PREFIX) :].split("-", 1)[0] - if pid.isdigit() and not _process_alive(int(pid)): + if fcntl is None: + return + for folder in root.glob(f"{HELD_PREFIX}*"): + try: + fd = _hold(folder, create=False) + except OSError: + continue # held by its owner, no lock to judge by, or gone + try: shutil.rmtree(folder, ignore_errors=True) + finally: + os.close(fd) class _SeedApi: @@ -162,6 +183,7 @@ class ScratchSeeder: self._fallback = Path(directory) self._memory_root = memory_root self._directory: Path | None = None + self._owner_lock: int | None = None self._on_live = on_live self._scratch = None self._unsynced: "weakref.WeakSet[sqlite3.Connection]" = weakref.WeakSet() @@ -266,6 +288,9 @@ class ScratchSeeder: if self._directory is not None: shutil.rmtree(self._directory, ignore_errors=True) self._directory = None + if self._owner_lock is not None: + os.close(self._owner_lock) # after the removal: never a free lock + self._owner_lock = None def _scratch_directory(self) -> Path: """A folder in memory (tmpfs) where there is one, else ``directory``. @@ -274,10 +299,35 @@ class ScratchSeeder: kernel is writing back the pages they rewrite. """ root = self._memory_root - if root is not None and root.is_dir() and os.access(root, os.W_OK): + if ( + fcntl is not None + and root is not None + and root.is_dir() + and os.access(root, os.W_OK) + ): remove_orphaned_scratch(root) if shutil.disk_usage(root).free >= MEMORY_FREE_BYTES: - prefix = f"{SCRATCH_PREFIX}{os.getpid()}-" - return Path(tempfile.mkdtemp(prefix=prefix, dir=root)) + held = self._held_memory_directory(root) + if held is not None: + return held self._fallback.mkdir(parents=True, exist_ok=True) return self._fallback + + def _held_memory_directory(self, root: Path) -> Path | None: + """A new folder under ``root`` that this seeder holds the lock of. + + It is locked under the staging name, which no cleaner judges, and + only then renamed: no cleaner ever sees it held-named and unlocked. + """ + staging = Path( + tempfile.mkdtemp(prefix=f"{STAGING_PREFIX}{os.getpid()}-", dir=root) + ) + try: + fd = _hold(staging, create=True) + except OSError: + shutil.rmtree(staging, ignore_errors=True) + return None # no locks on this filesystem: use the disk + held = root / (HELD_PREFIX + staging.name[len(STAGING_PREFIX) :]) + os.rename(staging, held) + self._owner_lock = fd + return held diff --git a/tests/browser/prompt-inspector.spec.mjs b/tests/browser/prompt-inspector.spec.mjs index 8841598a..7abdabb5 100644 --- a/tests/browser/prompt-inspector.spec.mjs +++ b/tests/browser/prompt-inspector.spec.mjs @@ -38,6 +38,9 @@ async function chooseDestination(page) { test('collection viewer rejects stale source and sends only refreshed API evidence',async({page,request,context})=>{ const host=await fakeHost(context,page); + // #430: the file is never found again here, so the manual Refresh is what this covers (the found-again path is + // workspace-stale-source.spec.mjs). + await page.route('**/prompt_manager/images/rescope',route=>route.fulfill({status:409,json:{success:false,code:'source-changed',error:'The source of this image changed or was removed.'}})); await page.addInitScript(()=>{window.copied=[];Object.defineProperty(navigator,'clipboard',{configurable:true,value:{writeText:async text=>window.copied.push(text)}});}); try { const loaded=await(await request.get('/prompt_manager/images/output?mode=workspace&limit=40')).json();const stale=loaded.images.find(image=>image.root_index===0&&image.path==='shared/same.png'); diff --git a/tests/browser/server.py b/tests/browser/server.py index da867594..45c83c8e 100644 --- a/tests/browser/server.py +++ b/tests/browser/server.py @@ -46,6 +46,7 @@ from tests.browser.workspace_library_editor_fixture import ( ) from tests.browser.workspace_vocabulary_fixture import register_vocabulary_commands from tests.browser.workspace_tagging_fixture import register_tagging_commands +from tests.browser.source_change_fixture import register_source_change_commands from tests.browser.workspace_review_fixture import register_review_commands from tests.browser.workspace_collections_fixture import register_collections_commands from tests.browser.workspace_diagnostics_fixture import ( @@ -231,7 +232,8 @@ def create_app(root: Path) -> web.Application: history_commands = register_workspace_history_commands(routes, api, root) library_editor_commands = register_library_editor_commands(routes, api, root) register_vocabulary_commands(routes, api, root) - register_tagging_commands(routes, api, root, model) + restore_sources = register_source_change_commands(routes, api) + register_tagging_commands(routes, api, root, model, on_reset=restore_sources) register_review_commands(routes, api, root) register_collections_commands(routes, api, root) reveal_stub = register_reveal_stub(routes, api, root) @@ -439,6 +441,7 @@ def create_app(root: Path) -> web.Application: async def reset(request: web.Request) -> web.Response: nonlocal history_fixture request_holds.release() + restore_sources() # No preview queued by the last test may land in the new fixture. await api._run_in_executor(api.close_auto_thumbnails) library_editor_commands.reset() diff --git a/tests/browser/source_change_fixture.py b/tests/browser/source_change_fixture.py new file mode 100644 index 00000000..4400404f --- /dev/null +++ b/tests/browser/source_change_fixture.py @@ -0,0 +1,66 @@ +"""Source changes under an open workspace page (#430), undone by every reset. + +- ``POST /__test__/sources/reverse``: the image sources are listed in reverse + order, as System › Sources would renumber them, so every held + ``root_scope`` names another index and the page must re-scope its file. +- ``POST /__test__/scope-key/rotate``: the API's scope and cursor keys + change, as when ``workspace-scope.key`` is lost before a restart: a held + scope then cannot be mapped and the page offers Refresh. +""" + +from __future__ import annotations + +import secrets +from typing import TYPE_CHECKING, Callable + +from aiohttp import web + +from utils.workspace_keys import WorkspaceKeys + +if TYPE_CHECKING: + from py.api import PromptManagerAPI + +_CACHE = "_workspace_keys_cache" + + +def register_source_change_commands( + routes: web.RouteTableDef, api: PromptManagerAPI +) -> Callable[[], None]: + """Register the commands; returns the restore every reset runs.""" + # Imported here: py.config needs the host stubs create_app installs first. + from py.config import GalleryConfig + + saved: dict = {} + + def restore() -> None: + if "roots" in saved: + GalleryConfig.MONITORING_DIRECTORIES = saved.pop("roots") + api.invalidate_gallery_cache(retire=True) + if "keys" in saved: + keys = saved.pop("keys") + if keys is None: + api.__dict__.pop(_CACHE, None) + else: + api.__dict__[_CACHE] = keys + + @routes.post("/__test__/sources/reverse") + async def reverse(request: web.Request) -> web.Response: + if request.query or request.can_read_body: + raise web.HTTPBadRequest() + roots = list(GalleryConfig.MONITORING_DIRECTORIES) + saved.setdefault("roots", roots) + GalleryConfig.MONITORING_DIRECTORIES = roots[::-1] + await api._run_in_executor(api.invalidate_gallery_cache, True) + return web.json_response({"success": True}) + + @routes.post("/__test__/scope-key/rotate") + async def rotate(request: web.Request) -> web.Response: + if request.query or request.can_read_body: + raise web.HTTPBadRequest() + saved.setdefault("keys", api.__dict__.get(_CACHE)) + api.__dict__[_CACHE] = WorkspaceKeys( + secrets.token_bytes(32), secrets.token_bytes(32), False + ) + return web.json_response({"success": True}) + + return restore diff --git a/tests/browser/tagging-concept.spec.mjs b/tests/browser/tagging-concept.spec.mjs index 756cde9c..4d7eec6a 100644 --- a/tests/browser/tagging-concept.spec.mjs +++ b/tests/browser/tagging-concept.spec.mjs @@ -231,6 +231,8 @@ test('paired Tag dialog captures', async ({ page, request }, testInfo) => { await expect(await request.post('/__test__/tagging/suggestions', { data: { scored: ALPINE } })).toBeOK(); const save = () => tagDialog(page).locator('.dialog-actions .ca-primary').click(); const status = () => tagDialog(page).locator('#tag-generation-status'); + // D147: a state that blocks Save shows its reason beside the actions, not in the status line. + const notice = () => tagDialog(page).locator('.dialog-actions > .tag-action-notice'); // Unavailable model. await expect(await request.post('/__test__/tagging/models', { data: { unavailable: ['wd14-swinv2'] } })).toBeOK(); @@ -269,7 +271,8 @@ test('paired Tag dialog captures', async ({ page, request }, testInfo) => { await saveImageTags(request, path, CONCEPT_TAGS); await openTagDialog(page, 'production', 'dark'); await save(); - await expect(status()).not.toHaveText(''); + await expect(notice()).toHaveText('The file changed since it was previewed. Reopen the Tag dialog.'); + await expect(status()).toHaveText(''); await shot('production-file-changed'); await page.unroute(writes); await page.keyboard.press('Escape'); @@ -278,7 +281,7 @@ test('paired Tag dialog captures', async ({ page, request }, testInfo) => { await page.route(writes, route => route.fulfill({ status: 504, body: 'Gateway Timeout' })); await openTagDialog(page, 'production', 'dark'); await save(); - await expect(status()).toHaveText('The save could not be confirmed. Retry sends the same tags again.'); + await expect(notice()).toHaveText('The save could not be confirmed. Retry sends the same tags again.'); expect((await inventory(page)).actions.buttons.map(button => [button.text, button.primary, button.disabled])).toEqual( [['Retry', false, false], ['Cancel', false, false], ['Save manual tags', true, true]]); await shot('production-unknown-outcome'); diff --git a/tests/browser/unseen-restore.mjs b/tests/browser/unseen-restore.mjs index aabd8962..6d70d881 100644 --- a/tests/browser/unseen-restore.mjs +++ b/tests/browser/unseen-restore.mjs @@ -52,3 +52,43 @@ export async function unseenRestore(page, restoring) { open(); } } + +// A restore the open page does see run and finish (#415), driven by the test instead of the 15 s poll's timing. The +// job is shaped as the server's (py/api/backup_restore.py: kind `restore`, one `errors` group, not cancellable). +const RESTORE_JOB_ID = '415415415415415415415415415415ab'; +const restoreJob = state => ({ id: RESTORE_JOB_ID, kind: 'restore', title: 'Database restore', state, started_at: 1, + finished_at: state === 'running' ? null : 2, progress: 0, status_line: state === 'running' ? '' : 'Restored the test snapshot.', + processed: 0, total: 0, counters: { errors: 0 }, scope: {}, listed: { errors: 0 }, log_length: 0, cancellable: false }); +/** The Activity pill's link while the page follows that restore. */ +export const SEEN_RESTORE_PILL = `#/activity/${RESTORE_JOB_ID}`; +/** The toast the page shows when it sees that restore finish (activity-kinds.mjs `restore.finishToast`). */ +export const SEEN_RESTORE_TOAST = 'Database restore finished · Restored the test snapshot.'; + +/** + * Call before the page's first navigation, after `armUnseenRestores(page)` (a later route wins). The page's first + * job-list read lists a restore job running, so its Activity store follows it; its job stream is refused, so the store + * reads the list again, and every later read is held until `finish()`, which answers them with the job finished and + * resolves once one of those answers reached the page. The real restore the test runs meanwhile goes through the API. + */ +export async function followRestore(page) { + let release; + const released = new Promise(resolve => { release = resolve; }); + let first = true; + await page.route(`${JOBS}/*/events`, route => route.abort().catch(() => {})); + await page.route(JOBS, async route => { + if (first) { + first = false; + return route.fulfill({ json: { success: true, jobs: [restoreJob('running')] } }).catch(() => {}); + } + await released; + // A read the page has aborted since (a newer read supersedes it) can no longer be answered. + return route.fulfill({ json: { success: true, jobs: [restoreJob('finished')] } }).catch(() => {}); + }); + return { + async finish() { + const answered = page.waitForResponse(response => new URL(response.url()).pathname === '/prompt_manager/jobs'); + release(); + await answered; + }, + }; +} diff --git a/tests/browser/workspace-images.spec.mjs b/tests/browser/workspace-images.spec.mjs index 5c2184b4..78e5baff 100644 --- a/tests/browser/workspace-images.spec.mjs +++ b/tests/browser/workspace-images.spec.mjs @@ -569,6 +569,10 @@ test('source and folder discovery failure stays visible and Import Rescan restor async function staleSourceRecovery(page, request, roots) { await expect(await request.post('/__test__/reset', { data: { extra_images: 80 } })).toBeOK(); await page.setViewportSize({ width: 1600, height: 1100 }); + // #430: the page first asks the server where a stale file is now. These tests cover the manual Refresh, so the + // file is never found; workspace-stale-source.spec.mjs covers a renumbered file that is found again. + await page.route('**/prompt_manager/images/rescope', route => route.fulfill({ status: 409, + json: { success: false, code: 'source-changed', error: 'The source of this image changed or was removed.' } })); await page.goto(workspace); const source = refineDialog(page).getByLabel('Source', { exact: true }); const folder = refineDialog(page).getByLabel('Folder', { exact: true }); diff --git a/tests/browser/workspace-library-editor.spec.mjs b/tests/browser/workspace-library-editor.spec.mjs index 2cc85b25..fa8346bc 100644 --- a/tests/browser/workspace-library-editor.spec.mjs +++ b/tests/browser/workspace-library-editor.spec.mjs @@ -1,7 +1,7 @@ import {writeFile} from 'node:fs/promises'; import {startCoverage,saveCoverage} from '../../tools/vendor/browser-coverage.mjs'; import {test,expect} from './fixtures.mjs'; -import {armUnseenRestores,unseenRestore} from './unseen-restore.mjs'; +import {armUnseenRestores,unseenRestore,followRestore,SEEN_RESTORE_PILL,SEEN_RESTORE_TOAST} from './unseen-restore.mjs'; import {captureCanonicalState} from './history-canonical-comparison.mjs'; import {inventoryOf,mapConcept,diffInventory} from './library-editor-canonical-comparison.mjs'; @@ -454,6 +454,37 @@ async function leaveAndReturn(page,id) { } const laneLabel=(page,text)=>variantsPanel(page).locator('.variant-lane').filter({hasText:text}); +// #415: a restore the page sees finish while New prompt is open keeps the dialog and its entries; the Library is shown +// again from the restored data only once the dialog has closed. +test('a restore seen finishing keeps New prompt open and reloads the Library after it closes',async({page,request})=>{ + const f=page.fixture;const snapshot=await request.get('/__test__/library-editor/snapshot');await expect(snapshot).toBeOK(); + const buffer=await snapshot.body(); + const current=await detail(request,f.positive_id,'active'); + await expect(await request.patch(`/prompt_manager/library/${f.positive_id}`,{data:{expected_epoch:current.library_epoch,if_match:current.positive.edit_token,metadata:{title:'Retitled elsewhere'}}})).toBeOK(); + const restoreRun=await followRestore(page); + await page.goto(`${workspace}#/library`);const card=page.locator(`article[data-prompt-id="${f.positive_id}"]`); + await expect(card).toContainText('Retitled elsewhere'); + await expect(page.locator('#activity-pill')).toHaveAttribute('href',SEEN_RESTORE_PILL); + await headingActions(page).getByRole('button',{name:'+ New prompt',exact:true}).click(); + const dialog=editorDialog(page);await expect(dialog.getByRole('heading',{name:'New prompt',exact:true})).toBeVisible();await ready(dialog); + await field(dialog,'Title').fill('Typed before restore');await field(dialog,'Positive prompt').fill('A prompt typed while storage was restored'); + await expect(await request.post('/prompt_manager/restore',{multipart:{database_file:{name:'editor.db',mimeType:'application/octet-stream',buffer}}})).toBeOK(); + page.writes.length=0; + await restoreRun.finish(); + await expect(toast(page)).toHaveText(SEEN_RESTORE_TOAST); + await expect(dialog).toBeVisible(); + await expect(field(dialog,'Title')).toHaveValue('Typed before restore'); + await expect(field(dialog,'Positive prompt')).toHaveValue('A prompt typed while storage was restored'); + await expect(card).toContainText('Retitled elsewhere'); + await button(dialog,'Save').click(); + await expect(note(dialog)).toHaveText('Prompt storage was restored since this dialog opened. Nothing was saved.'); + await expect(button(dialog,'Save')).toBeDisabled();expect(page.writes).toHaveLength(1); + await button(dialog,'Cancel').click();await expect(dialog).toHaveCount(0); + await expect(card).not.toContainText('Retitled elsewhere');await expect(card).toContainText('Editor portrait'); + await expect(page.locator('#workspace-main h1')).toBeFocused(); + expect(page.writes).toHaveLength(1); +}); + test('library heading offers Export then + New prompt, and New opens the saved prompt history',async({page,request})=>{ await page.goto(`${workspace}#/library`);await expect(page.locator('article[data-prompt-id]').first()).toBeVisible(); const actions=headingActions(page).locator('button'); diff --git a/tests/browser/workspace-stale-source.spec.mjs b/tests/browser/workspace-stale-source.spec.mjs new file mode 100644 index 00000000..1d6144fe --- /dev/null +++ b/tests/browser/workspace-stale-source.spec.mjs @@ -0,0 +1,236 @@ +// #430: an open workspace page whose sources changed under it. A ComfyUI restart no longer changes any scope (the +// key is persisted, see tests/test_api_workspace_restart.py); what is left is a real source change. Renumbered +// sources (`/__test__/sources/reverse`) are found again: the Tag image dialog stays open on its file and works, and +// the panel keeps the file. A scope that cannot be mapped (`/__test__/scope-key/rotate`, a lost key) blocks Generate +// and Save visibly, says why beside them and offers Refresh. Both commands are undone by every reset. +import { startCoverage, saveCoverage } from '../../tools/vendor/browser-coverage.mjs'; +import { test, expect } from './fixtures.mjs'; + +const images = '/prompt_manager/workspace#/images'; +const FIRST = 'tagging/dunes_00001.png', SECOND = 'tagging/dunes_00002.png'; +const SOURCE_CHANGED = 'The source changed. Refresh the image collection before tagging this file.'; +const card = (page, path, root = 0) => page.locator(`article[data-image-key="${root}:${path}"]`); +const cards = page => page.locator('.gallery-body article[data-image-key]'); +const tagButton = (page, path, root = 0) => card(page, path, root).getByRole('button', { name: `Tag or auto-tag ${path.split('/').pop()}`, exact: true }); +const infoButton = (page, path, root = 0) => card(page, path, root).getByRole('button', { name: `Inspect ${path.split('/').pop()}`, exact: true }); +const dialog = page => page.getByRole('dialog', { name: 'Tag image', exact: true }); +const manual = page => dialog(page).getByRole('textbox', { name: 'Image tags · edit manually', exact: true }); +const generate = page => dialog(page).getByRole('button', { name: 'Generate suggestions', exact: true }); +const primary = page => dialog(page).locator('.dialog-actions .ca-primary'); +const notice = page => dialog(page).locator('.dialog-actions > .tag-action-notice'); +const status = page => dialog(page).locator('#tag-generation-status'); +const chips = page => dialog(page).locator('#tag-suggestions .chip.tag-suggestion'); +const panel = page => page.getByRole('complementary', { name: 'Selected image details', exact: true }); +const toast = page => page.locator('#workspace-toast'); +const answered = (page, suffix, method = 'POST') => page.waitForResponse(item => new URL(item.url()).pathname.endsWith(suffix) && item.request().method() === method); + +test.beforeEach(async ({ page, request, browserName }) => { + await startCoverage(page, browserName); + await expect(await request.post('/__test__/tagging/reset', { data: {} })).toBeOK(); + page.staleErrors = []; + page.on('pageerror', error => page.staleErrors.push(error.message)); +}); +test.afterEach(async ({ page, request, browserName }) => { + await saveCoverage(page, browserName); + expect(page.staleErrors).toEqual([]); + // The next spec file starts on the standard sources and key. + await expect(await request.post('/__test__/tagging/reset', { data: {} })).toBeOK(); +}); + +async function openTag(page, path = FIRST, root = 0) { + const observed = answered(page, '/image-annotations/observe'); + await tagButton(page, path, root).click(); + await expect(dialog(page)).toBeVisible(); + return observed; +} +async function savedTags(request, path, root) { + const listing = await (await request.get('/prompt_manager/images/output?mode=workspace&limit=40')).json(); + const item = listing.images.find(image => image.path === path && image.root_index === root); + const target = { root_index: root, root_scope: item.root_scope, path }; + const observed = await (await request.post('/prompt_manager/image-annotations/observe', { data: { target } })).json(); + await request.delete(`/prompt_manager/image-annotations/capture/${observed.capture.token}`); + return observed.tags; +} + +test('renumbered sources under an open Tag dialog: it finds the file again, keeps the draft and Generate works', async ({ page, request }) => { + await page.goto(images); + await expect(card(page, FIRST)).toHaveClass(/\bselected\b/); + expect((await openTag(page)).status()).toBe(200); + await manual(page).fill('kept_draft'); + await expect(await request.post('/__test__/sources/reverse')).toBeOK(); + + // Generate is refused (the held scope names another index now); the page asks where the file is and retargets. + const refused = answered(page, '/image-annotations/suggest'); + const rescoped = answered(page, '/images/rescope'); + const observedAgain = answered(page, '/image-annotations/observe'); + await generate(page).click(); + expect((await refused).status()).toBe(409); + const mapped = await (await rescoped).json(); + expect([mapped.image.root_index, mapped.image.path]).toEqual([1, FIRST]); + const observed = await observedAgain; + expect(observed.status()).toBe(200); + expect(observed.request().postDataJSON().target).toEqual({ root_index: 1, root_scope: mapped.image.root_scope, path: FIRST }); + await expect(dialog(page)).toBeVisible(); + await expect(notice(page)).toHaveCount(0); + await expect(manual(page)).toHaveValue('kept_draft'); + await expect(dialog(page).locator('img.tag-image-preview')).toHaveAttribute('src', /\/image-annotations\/capture\//); + + // The page listed again under the new numbering and kept the file selected. + await expect(card(page, FIRST, 1)).toHaveClass(/\bselected\b/); + await expect(card(page, FIRST, 0)).toHaveCount(0); + + const suggested = answered(page, '/image-annotations/suggest'); + await generate(page).click(); + expect((await suggested).status()).toBe(200); + await expect(chips(page).first()).toBeVisible(); + const saved = answered(page, '/image-annotations/tags', 'PUT'); + await primary(page).click(); + expect((await saved).request().postDataJSON().target.root_index).toBe(1); + await expect(toast(page)).toContainText('tags saved for dunes_00001.png.'); + expect(await savedTags(request, FIRST, 1)).toContain('kept_draft'); +}); + +test('the inspector finds a renumbered file again; checked files from the old numbering are cleared', async ({ page, request }) => { + await page.goto(images); + await expect(card(page, FIRST)).toHaveClass(/\bselected\b/); + await card(page, FIRST).getByRole('checkbox', { name: /^Select / }).check(); + await expect(page.locator('.bulkbar')).toBeVisible(); + await expect(await request.post('/__test__/sources/reverse')).toBeOK(); + + const refused = page.waitForResponse(item => new URL(item.url()).pathname === `/prompt_manager/images/inspect/${SECOND}` && + new URL(item.url()).searchParams.get('root') === '0'); + const reread = page.waitForResponse(item => new URL(item.url()).pathname === `/prompt_manager/images/inspect/${SECOND}` && + new URL(item.url()).searchParams.get('root') === '1'); + await infoButton(page, SECOND).click(); + expect((await refused).status()).toBe(409); + expect((await reread).status()).toBe(200); + await expect(card(page, SECOND, 1)).toHaveClass(/\bselected\b/); + await expect(panel(page)).not.toContainText('The source changed'); + await expect(panel(page).getByRole('button', { name: 'Refresh image collection', exact: true })).toHaveCount(0); + // A checked key of the old list could name a same-named file in another source: nothing stays checked. + await expect(page.locator('.bulkbar')).toBeHidden(); + await expect(cards(page).getByRole('checkbox', { checked: true })).toHaveCount(0); +}); + +test('a scope that cannot be mapped: Generate and Save are visibly disabled, the reason and Refresh sit beside them', async ({ page }) => { + await page.goto(images); + await expect(card(page, FIRST)).toBeVisible(); + await expect(await page.request.post('/__test__/scope-key/rotate')).toBeOK(); + const rescoped = answered(page, '/images/rescope'); + expect((await openTag(page)).status()).toBe(409); + expect((await rescoped).status()).toBe(409); + + await expect(notice(page)).toContainText(SOURCE_CHANGED); + await expect(status(page)).toHaveText(''); + const refresh = notice(page).getByRole('button', { name: 'Refresh image collection', exact: true }); + await expect(refresh).toBeVisible(); + for (const button of [generate(page), primary(page)]) { + await expect(button).toHaveAttribute('aria-disabled', 'true'); + await expect(button).toHaveCSS('opacity', '0.55'); + } + // The notice is its own first row of the sticky actions, above the buttons. + const rows = await dialog(page).locator('.dialog-actions').evaluate(actions => { + const noticeBox = actions.firstElementChild.getBoundingClientRect(); + return { first: actions.firstElementChild.className, below: [...actions.querySelectorAll(':scope > button')].every(button => button.getBoundingClientRect().top >= noticeBox.bottom - 1) }; + }); + expect(rows).toEqual({ first: 'tag-action-notice', below: true }); + const preview = dialog(page).locator('.tag-image-preview'); + await expect(preview).toHaveText('Preview unavailable'); + await expect(preview).toHaveAttribute('role', 'img'); + const suggest = []; + page.on('request', item => { if (item.url().endsWith('/image-annotations/suggest')) suggest.push(item.url()); }); + // Pressed anyway (Playwright refuses an aria-disabled target, so the press is dispatched), nothing is sent. + await generate(page).dispatchEvent('click'); + await primary(page).dispatchEvent('click'); + expect(suggest).toEqual([]); + await page.screenshot({ path: test.info().outputPath('stale-source-tag-dialog.png') }); + + // Refresh closes the dialog and lists again with the current scopes; Tag then works. + const relisted = page.waitForResponse(item => item.url().includes('/images/output?')); + await refresh.click(); + await expect(dialog(page)).toHaveCount(0); + await relisted; + await expect(card(page, FIRST)).toBeVisible(); + expect((await openTag(page)).status()).toBe(200); + await expect(notice(page)).toHaveCount(0); + await expect(generate(page)).not.toHaveAttribute('aria-disabled', 'true'); +}); + +test('an unmappable inspector read keeps its visible Refresh, and Refresh recovers the panel', async ({ page }) => { + await page.goto(images); + await expect(card(page, FIRST)).toHaveClass(/\bselected\b/); + await expect(await page.request.post('/__test__/scope-key/rotate')).toBeOK(); + const rescoped = answered(page, '/images/rescope'); + await infoButton(page, SECOND).click(); + expect((await rescoped).status()).toBe(409); + await expect(panel(page)).toContainText('The source changed. Refresh the image collection before inspecting this file.'); + const relisted = page.waitForResponse(item => item.url().includes('/images/output?')); + await panel(page).getByRole('button', { name: 'Refresh image collection', exact: true }).click(); + await relisted; + await expect(cards(page).first()).toHaveClass(/\bselected\b/); + await expect(panel(page)).not.toContainText('The source changed'); +}); + +test('a Load more cursor the server cannot verify offers Refresh instead of a dead Retry', async ({ page, request }) => { + await expect(await request.post('/__test__/tagging/reset', { data: { extra_images: 60 } })).toBeOK(); + await page.goto(images); + await expect(cards(page).first()).toBeVisible(); + await expect(page.locator('#gallery-continuation')).toContainText('more load as you scroll'); + await expect(await request.post('/__test__/scope-key/rotate')).toBeOK(); + const stale = page.waitForResponse(item => item.url().includes('/images/output?') && new URL(item.url()).searchParams.has('cursor')); + await page.locator('#gallery-continuation').scrollIntoViewIfNeeded(); + const answer = await stale; + expect(answer.status()).toBe(400); + expect((await answer.json()).code).toBe('cursor-stale'); + const continuation = page.locator('#gallery-continuation'); + await expect(continuation).toContainText('The image collection changed. Refresh image collection to continue.'); + const relisted = page.waitForResponse(item => item.url().includes('/images/output?') && !new URL(item.url()).searchParams.has('cursor')); + await continuation.getByRole('button', { name: 'Refresh image collection', exact: true }).click(); + expect((await relisted).status()).toBe(200); + await expect(continuation).not.toContainText('The image collection changed'); +}); + +test('a viewer whose media source was renumbered closes onto the file, kept selected', async ({ page, request }) => { + await page.goto(images); + await expect(card(page, FIRST)).toHaveClass(/\bselected\b/); + await expect(await request.post('/__test__/sources/reverse')).toBeOK(); + const rescoped = answered(page, '/images/rescope'); + await card(page, FIRST).locator('.image-open').click(); + const mapped = await (await rescoped).json(); + expect([mapped.image.root_index, mapped.image.path]).toEqual([1, FIRST]); + await expect(page.locator('#viewer-dialog')).toHaveCount(0); + await expect(card(page, FIRST, 1)).toHaveClass(/\bselected\b/); + // Opened again, it shows the file under its current source. + await card(page, FIRST, 1).locator('.image-open').click(); + await expect(page.locator('#viewer-dialog[open]')).toBeVisible(); + await expect(page.locator('#viewer-dialog').getByText('Media could not be loaded. Retry this file.', { exact: true })).toBeHidden(); + await expect.poll(() => page.locator('#viewer-dialog img').first().evaluate(image => image.complete && image.naturalWidth > 0)).toBe(true); +}); + +test('a collection page finds a renumbered member again and stays on its members', async ({ page, request }) => { + // Members are stored by root path, not index (#331), so a renumbering keeps them; the page's scopes go stale. + const listing = await (await request.get('/prompt_manager/images/output?mode=workspace&limit=40')).json(); + const first = listing.images.find(image => image.path === FIRST && image.root_index === 0); + const name = `Stale ${test.info().workerIndex}-${Date.now()}`; + const added = await request.post('/prompt_manager/collections/items', { data: { name, items: [ + { root_index: 0, root_scope: first.root_scope, path: FIRST }] } }); + await expect(added).toBeOK(); + const collections = (await (await request.get('/prompt_manager/collections')).json()).collections; + const { id } = collections.find(item => item.name === name); + await page.goto(`/prompt_manager/workspace#/collection/${id}`); + await expect(cards(page)).toHaveCount(1); + await expect(card(page, FIRST)).toHaveClass(/\bselected\b/); + await expect(await request.post('/__test__/sources/reverse')).toBeOK(); + + const rescoped = answered(page, '/images/rescope'); + const relisted = page.waitForResponse(item => item.url().includes('/images/output?') && new URL(item.url()).searchParams.get('collection') === String(id)); + await card(page, FIRST).locator('.image-open').click(); + expect((await (await rescoped).json()).image.root_index).toBe(1); + expect((await relisted).status()).toBe(200); + await expect(page).toHaveURL(new RegExp(`#/collection/${id}$`)); + await expect(cards(page)).toHaveCount(1); + await expect(card(page, FIRST, 1)).toHaveClass(/\bselected\b/); + // Tag image works on the member under its new source. + expect((await openTag(page, FIRST, 1)).status()).toBe(200); + await expect(notice(page)).toHaveCount(0); +}); diff --git a/tests/browser/workspace-system-backups.spec.mjs b/tests/browser/workspace-system-backups.spec.mjs index edc369d1..3a0404d4 100644 --- a/tests/browser/workspace-system-backups.spec.mjs +++ b/tests/browser/workspace-system-backups.spec.mjs @@ -1,6 +1,7 @@ import { startCoverage, saveCoverage } from '../../tools/vendor/browser-coverage.mjs'; import { test, expect } from './fixtures.mjs'; import { seedConcept } from './concept-inventory.mjs'; +import { followRestore, SEEN_RESTORE_PILL, SEEN_RESTORE_TOAST } from './unseen-restore.mjs'; // System › Backup & export (#249 System S9 Task 24) on the S8 backend and the concept-shaped fixture: Back up and its // status line (D103), the restore review (D105) and its job, the exports, Import / inspect and the recovery policy @@ -265,6 +266,24 @@ test('a finished restore shows the open page again from the restored data', asyn expect(await libraryTotal(request)).toBe(restoredTotal); }); +// #415: an open dialog waits out a finished restore, except a review: the plan it staged no longer describes storage, +// so it closes as dismissed (its stage discarded), as before. +test('a restore seen finishing elsewhere closes an open restore review and discards its stage', async ({ page, request }) => { + const bytes = await backupBytes(request); + const restoreRun = await followRestore(page); + await openBackups(page); + await expect(page.locator('#activity-pill')).toHaveAttribute('href', SEEN_RESTORE_PILL); + await chooseBackup(page, bytes); + await expect(dialog(page).locator('#modal-title')).toHaveText('Restore preview'); + expect((await serverJson(request, '/__test__/system/backups')).stages).toBe(1); + const discarded = page.waitForResponse(response => response.url().includes(`${PREVIEW_URL}/`) && response.request().method() === 'DELETE'); + await restoreRun.finish(); + await expect(toast(page)).toHaveText(SEEN_RESTORE_TOAST); + expect((await discarded).status()).toBe(200); + await expect(dialog(page)).toHaveCount(0); + expect((await serverJson(request, '/__test__/system/backups')).stages).toBe(0); +}); + test('invalid file shows the validator message; nothing is staged', async ({ page, request }) => { const before = await serverJson(request, '/__test__/system/digest'); await openBackups(page); diff --git a/tests/browser/workspace-tagging.spec.mjs b/tests/browser/workspace-tagging.spec.mjs index 0c4cf8ec..3b65a367 100644 --- a/tests/browser/workspace-tagging.spec.mjs +++ b/tests/browser/workspace-tagging.spec.mjs @@ -21,6 +21,8 @@ const model = page => dialog(page).getByRole('combobox', { name: 'Tagging model' const generate = page => dialog(page).getByRole('button', { name: 'Generate suggestions', exact: true }); const chips = page => dialog(page).locator('#tag-suggestions .chip.tag-suggestion'); const status = page => dialog(page).locator('#tag-generation-status'); +// D147: the reason a state blocks Save, on its own row in the sticky actions. +const notice = page => dialog(page).locator('.dialog-actions > .tag-action-notice'); const toast = page => page.locator('#workspace-toast'); const nav = (page, label) => page.locator('aside.sidebar nav').getByRole('link', { name: new RegExp(`^${label}(?:\\s*[\\d,]+)?$`) }); const FIRST = 'tagging/dunes_00001.png', SECOND = 'tagging/dunes_00002.png', COPY = 'copies/dunes_00001.png'; @@ -728,7 +730,8 @@ test('D116: Retry leads the actions after an unconfirmed save and resends the sa let lost = true; await page.route('**/prompt_manager/image-annotations/tags', route => (lost ? (lost = false, route.fulfill({ status: 504, body: 'Gateway Timeout' })) : route.continue())); await dialog(page).getByRole('button', { name: 'Save manual tags', exact: true }).click(); - await expect(status(page)).toHaveText('The save could not be confirmed. Retry sends the same tags again.'); + await expect(notice(page)).toHaveText('The save could not be confirmed. Retry sends the same tags again.'); + await expect(status(page)).toHaveText(''); const inventory = await page.locator('dialog#modal').evaluate(taggingInventory); expect(inventory.actions.buttons.map(button => [button.text, button.primary, button.disabled])).toEqual( [['Retry', false, false], ['Cancel', false, false], ['Save manual tags', true, true]]); diff --git a/tests/browser/workspace-vocabulary.spec.mjs b/tests/browser/workspace-vocabulary.spec.mjs index 3a223501..ef5ab643 100644 --- a/tests/browser/workspace-vocabulary.spec.mjs +++ b/tests/browser/workspace-vocabulary.spec.mjs @@ -1,7 +1,7 @@ import {startCoverage,saveCoverage} from '../../tools/vendor/browser-coverage.mjs'; import {writeFile} from 'node:fs/promises'; import {test,expect} from './fixtures.mjs'; -import {armUnseenRestores,unseenRestore} from './unseen-restore.mjs'; +import {armUnseenRestores,unseenRestore,followRestore,SEEN_RESTORE_PILL,SEEN_RESTORE_TOAST} from './unseen-restore.mjs'; import {captureCanonicalState} from './history-canonical-comparison.mjs'; import {pageInventoryOf,inventoryOf,diffInventory,mapVocabularyConcept,mapVocabularyDialog} from './vocabulary-canonical-comparison.mjs'; @@ -552,6 +552,31 @@ test('conflict and restore states keep the dialog and write nothing',async({page expect(writes.filter(write=>write.url==='/prompt_manager/vocabulary/tags')).toHaveLength(1); }); +// #415: a restore the page sees finish while Create tag is open keeps the dialog and its entries; the page is shown +// again from the restored data only once the dialog has closed. +test('a restore seen finishing keeps an open dialog and reloads the page after it closes',async({page,request})=>{ + const saved=await backup(request); + await expect(await request.put('/prompt_manager/tags/mist',{data:{new_name:'fog'}})).toBeOK(); + const restoreRun=await followRestore(page);const writes=trackWrites(page);await openPage(page); + await expect(page.locator('#activity-pill')).toHaveAttribute('href',SEEN_RESTORE_PILL); + await expect(tagRow(page,'fog')).toHaveCount(1); + const box=await openDialog(page,main(page).getByRole('button',{name:'+ Create tag',exact:true}),'Create tag'); + await field(box,'Tag name').fill('sunrise'); + await expect(await request.post('/prompt_manager/restore',{multipart:{database_file:{name:'vocabulary.db',mimeType:'application/octet-stream',buffer:saved}}})).toBeOK(); + await restoreRun.finish(); + await expect(toast(page)).toHaveText(SEEN_RESTORE_TOAST); + await expect(box).toBeVisible();await expect(field(box,'Tag name')).toHaveValue('sunrise'); + await expect(tagRow(page,'fog')).toHaveCount(1); + await button(box,'Apply').click(); + await expect(note(box)).toHaveText('Prompt storage was restored since this dialog opened. Nothing was changed.'); + expect(await actionNames(box)).toEqual(['Reload','Cancel','Apply']); + expect(tagNames(await snapshot(request))).not.toContain('sunrise'); + await button(box,'Cancel').click();await expect(box).toHaveCount(0); + await expect(tagRow(page,'mist')).toHaveCount(1);await expect(tagRow(page,'fog')).toHaveCount(0); + await expect(main(page).getByRole('heading',{level:1})).toBeFocused(); + expect(writes.filter(write=>write.url==='/prompt_manager/vocabulary/tags')).toHaveLength(1); +}); + test('lost response is outcome-unknown',async({page,request})=>{ const writes=trackWrites(page);await openPage(page); await page.route('**/prompt_manager/vocabulary/tags/*',async route=>{ diff --git a/tests/browser/workspace_tagging_fixture.py b/tests/browser/workspace_tagging_fixture.py index 7e2cca21..4fe5bce9 100644 --- a/tests/browser/workspace_tagging_fixture.py +++ b/tests/browser/workspace_tagging_fixture.py @@ -194,9 +194,16 @@ def tagging_snapshot(api: PromptManagerAPI) -> dict: def register_tagging_commands( - routes: web.RouteTableDef, api: PromptManagerAPI, root: Path, model + routes: web.RouteTableDef, + api: PromptManagerAPI, + root: Path, + model, + on_reset=lambda: None, ) -> None: - """Register fixed seed, snapshot and fake-model commands; no paths or SQL input.""" + """Register fixed seed, snapshot and fake-model commands; no paths or SQL input. + + ``on_reset`` runs first on every tagging reset (the source-change undo). + """ # The fake download (``/__test__/tagging/download``): waits on ``event``, # then answers ``result``. Every reset sets the event and puts back the # model's raising default, so no test can leave a worker hanging. @@ -230,6 +237,7 @@ def register_tagging_commands( raise web.HTTPBadRequest() if not 0 <= extra <= MAX_EXTRA_IMAGES: raise web.HTTPBadRequest() + on_reset() model.unavailable = set() model.missing_packages = {} model.suggestions = None diff --git a/tests/fixtures/legacy_contract_snapshot.json b/tests/fixtures/legacy_contract_snapshot.json index 27568173..62ee7af2 100644 --- a/tests/fixtures/legacy_contract_snapshot.json +++ b/tests/fixtures/legacy_contract_snapshot.json @@ -121,6 +121,7 @@ "POST /prompt_manager/images/generate-thumbnails", "POST /prompt_manager/images/generate-thumbnails/progress", "POST /prompt_manager/images/link", + "POST /prompt_manager/images/rescope", "POST /prompt_manager/images/reveal", "POST /prompt_manager/jobs/autotag", "POST /prompt_manager/jobs/duplicates", diff --git a/tests/js/workspace-image-tagging-state.test.js b/tests/js/workspace-image-tagging-state.test.js index 4f64b71f..4b7ea2ac 100644 --- a/tests/js/workspace-image-tagging-state.test.js +++ b/tests/js/workspace-image-tagging-state.test.js @@ -253,15 +253,16 @@ test('an unreadable or malformed models read falls back to the documented defaul test('observe failures and malformed answers end in observe-error and retry observes again', async () => { const { createImageTaggingController } = await load(); - let answer = () => fail(409, 'source-changed', 'The source changed. Refresh the image collection before tagging this file.'); + let answer = () => fail(500, 'observe-failed'); const h = harness(defaultRoutes({ [`POST ${OBSERVE}`]: () => answer() })); const c = createImageTaggingController({ api: h.api, target: TARGET }); await c.open(); assert.equal(c.state.phase, 'observe-error'); - assert.equal(c.state.statusText, 'The source changed. Refresh the image collection before tagging this file.'); + assert.equal(c.state.statusText, 'The image could not be opened for tagging.'); assert.equal(c.state.canSave, false); assert.equal(c.state.canGenerate, false); assert.equal(c.state.canRetry, true); + assert.equal(c.state.blocked, true, 'a failed capture blocks Generate and Save with its reason'); answer = () => response(200, observed({ tag_revision: -1 })); await c.retry(); assert.equal(c.state.phase, 'observe-error'); @@ -274,9 +275,71 @@ test('observe failures and malformed answers end in observe-error and retry obse await c.retry(); await settle(); assert.equal(c.state.phase, 'ready'); + assert.equal(c.state.blocked, false); assert.equal(h.of('POST', OBSERVE).length, 4); }); +test('#430: a changed source blocks the dialog until it is retargeted, keeping the draft', async () => { + const { createImageTaggingController } = await load(); + const SOURCE = 'The source changed. Refresh the image collection before tagging this file.'; + const fresh = { root_index: 1, root_scope: 'e'.repeat(64), path: TARGET.path }; + const h = harness(defaultRoutes({ + [`POST ${OBSERVE}`]: call => (call.body.target.root_scope === SCOPE ? fail(409, 'source-changed', SOURCE) : response(200, observed())), + })); + const c = createImageTaggingController({ api: h.api, target: TARGET }); + await c.open(); + assert.equal(c.state.phase, 'source-changed'); + assert.equal(c.state.statusText, SOURCE); + assert.deepEqual([c.state.canSave, c.state.canGenerate, c.state.canRetry, c.state.blocked], [false, false, false, true]); + assert.equal(await c.retry(), false, 'the same stale target is never observed again'); + assert.equal(h.of('POST', OBSERVE).length, 1); + assert.throws(() => c.retarget({ ...fresh, root_scope: 'bad' }), TypeError); + await c.retarget(fresh); + await settle(); + assert.equal(c.state.phase, 'ready'); + assert.deepEqual(h.of('POST', OBSERVE).at(-1).body.target, fresh); + assert.deepEqual({ ...c.target }, fresh); + // Generate and Save now carry the new target. + c.setManual('kept draft'); + await c.generate(); + assert.deepEqual(h.of('POST', SUGGEST)[0].body.target, fresh); +}); + +test('#430: a source change answered to Generate or Save also blocks; retarget keeps the typed draft', async () => { + for (const route of [SUGGEST, TAGS]) { + const method = route === TAGS ? 'PUT' : 'POST'; + let stale = true; + const { c, h } = await opened({ + [`${method} ${route}`]: call => (stale ? fail(409, 'source-changed', 'server says source') : + route === TAGS ? response(200, receipt(call.body.tags)) : response(200, suggested())), + }); + c.setManual('kept draft'); + if (route === TAGS) await c.save(); else await c.generate(); + assert.equal(c.state.phase, 'source-changed'); + assert.equal(c.state.statusText, 'server says source'); + stale = false; + await c.retarget({ root_index: 2, root_scope: 'f'.repeat(64), path: TARGET.path }); + await settle(); + assert.equal(c.state.phase, 'ready'); + assert.equal(c.state.manual, 'kept draft'); + assert.equal(h.of('DELETE', CAPTURE).length, 1, 'the old capture is released'); + } +}); + +test('#430: retarget waits for nothing: it refuses while a save is in flight and after close', async () => { + const gate = deferred(); + const { c, h } = await opened({ [`PUT ${TAGS}`]: () => gate.promise }); + const saving = c.save(); + await flush(); + assert.equal(c.state.phase, 'saving'); + assert.equal(await c.retarget({ ...TARGET, root_index: 3 }), false); + gate.resolve(response(200, receipt(['Minimal']))); + await saving; + c.cancel(); + assert.equal(await c.retarget({ ...TARGET, root_index: 3 }), false); + assert.equal(h.of('POST', OBSERVE).length, 1); +}); + test('model change clears suggestions count and status but keeps manual', async () => { const { c } = await opened(); c.setManual('Minimal, dune'); @@ -547,7 +610,7 @@ test('write errors keep the draft and allow another save', async () => { }); test('storage-changed and file-changed disable save until reopen', async () => { - for (const [code, phase] of [['storage-changed', 'storage-changed'], ['file-changed', 'file-changed'], ['source-changed', 'file-changed'], ['capture-expired', 'capture-expired']]) { + for (const [code, phase] of [['storage-changed', 'storage-changed'], ['file-changed', 'file-changed'], ['source-changed', 'source-changed'], ['capture-expired', 'capture-expired']]) { const message = `server says ${code}`; let answer = () => fail(409, code, message); const { c, h } = await opened({ [`PUT ${TAGS}`]: () => answer() }); diff --git a/tests/js/workspace-images-source-recovery.test.js b/tests/js/workspace-images-source-recovery.test.js new file mode 100644 index 00000000..a03a968d --- /dev/null +++ b/tests/js/workspace-images-source-recovery.test.js @@ -0,0 +1,172 @@ +// Run with: node --test tests/js/workspace-images-source-recovery.test.js +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const load = () => import('../../web/js/workspace/images-source-recovery.mjs'); + +const OLD = Object.freeze({ root_index: 0, root_scope: 'a'.repeat(64), path: 'sub/a.png', filename: 'a.png' }); +const FRESH = Object.freeze({ root_index: 1, root_scope: 'b'.repeat(64), path: 'sub/a.png', filename: 'a.png', url: '/u' }); + +function fakeApi(answer) { + const calls = []; + return { calls, post: async (url, body, options) => { calls.push({ url, body, options }); return answer(body); } }; +} +const flush = () => new Promise(resolve => setImmediate(resolve)); + +test('rescopeImage posts only the target and returns the fresh descriptor', async () => { + const { rescopeImage, RESCOPE_URL } = await load(); + const api = fakeApi(() => ({ success: true, image: FRESH })); + const signal = new AbortController().signal; + const fresh = await rescopeImage(api, OLD, { signal }); + assert.deepEqual(fresh, FRESH); + assert.ok(Object.isFrozen(fresh)); + assert.deepEqual(api.calls, [{ url: RESCOPE_URL, body: { target: { root_index: 0, root_scope: OLD.root_scope, path: 'sub/a.png' } }, options: { signal } }]); +}); + +test('rescopeImage answers null for a refusal, a malformed answer, another path or a bad target', async () => { + const { rescopeImage } = await load(); + const refused = { post: async () => { throw Object.assign(new Error('gone'), { status: 409, code: 'source-changed' }); } }; + assert.equal(await rescopeImage(refused, OLD), null); + const lost = { post: async () => { throw new TypeError('Failed to fetch'); } }; + await assert.rejects(rescopeImage(lost, OLD), TypeError, 'a lost answer is not final'); + for (const answer of [null, { success: false }, { success: true, image: { ...FRESH, root_scope: 'x' } }, + { success: true, image: { ...FRESH, path: 'other.png' } }, { success: true, image: { ...FRESH, root_index: -1 } }]) { + assert.equal(await rescopeImage(fakeApi(() => answer), OLD), null); + } + const api = fakeApi(() => ({ success: true, image: FRESH })); + for (const image of [null, { ...OLD, root_scope: 'short' }, { ...OLD, path: '' }, { ...OLD, root_index: 1.5 }]) { + assert.equal(await rescopeImage(api, image), null); + } + assert.equal(api.calls.length, 0, 'a malformed target is never sent'); +}); + +test('one rescope per held pair: callers share it, a tried pair answers its first result, reset forgets', async () => { + const { createSourceRecovery } = await load(); + let answer = { success: true, image: FRESH }; + const api = fakeApi(() => answer); + const applied = []; + const recovery = createSourceRecovery({ api, generation: () => 7, apply: async (image, fresh, started) => { applied.push([image.path, fresh.root_index, started]); return fresh; } }); + const [first, second] = await Promise.all([recovery.recover(OLD), recovery.recover({ ...OLD })]); + assert.deepEqual([first, second], [FRESH, FRESH]); + assert.equal(api.calls.length, 1); + assert.deepEqual(applied, [['sub/a.png', 1, 7]]); + answer = { success: false }; + assert.deepEqual(await recovery.recover(OLD), FRESH, 'the pair is not asked again'); + recovery.reset(); + assert.equal(await recovery.recover(OLD), null, 'after a reset the pair is asked again'); + assert.equal(api.calls.length, 2); + assert.equal(await recovery.recover({ ...OLD, root_scope: 'bad' }), null); + assert.equal(api.calls.length, 2); +}); + +test('a refused pair never loops, an aborted page applies nothing and a failing apply is null', async () => { + const { createSourceRecovery } = await load(); + const api = fakeApi(() => ({ success: true, image: FRESH })); + const page = new AbortController(); + let applied = 0; + const recovery = createSourceRecovery({ api, signal: page.signal, apply: async () => { applied += 1; return FRESH; } }); + page.abort(); + assert.equal(await recovery.recover(OLD), null); + assert.equal(applied, 0); + const failing = createSourceRecovery({ api, apply: async () => { throw new Error('page gone'); } }); + assert.equal(await failing.recover(OLD), null); + assert.throws(() => createSourceRecovery({ api }), TypeError); + // An api without post (an embedded gallery's page adapter) finds nothing. + assert.equal(await createSourceRecovery({ api: {}, apply: async () => FRESH }).recover(OLD), null); +}); + +test('the memo forgets its oldest pairs past its limit', async () => { + const { createSourceRecovery } = await load(); + const api = fakeApi(body => ({ success: true, image: { ...FRESH, path: body.target.path } })); + const recovery = createSourceRecovery({ api, apply: async (_image, fresh) => fresh }); + for (let index = 0; index < 65; index++) await recovery.recover({ ...OLD, path: `f${index}.png` }); + assert.equal(api.calls.length, 65); + await recovery.recover({ ...OLD, path: 'f64.png' }); + assert.equal(api.calls.length, 65, 'a recent pair is still remembered'); + await recovery.recover({ ...OLD, path: 'f0.png' }); + assert.equal(api.calls.length, 66, 'the oldest pair was forgotten'); +}); + +function fakePage({ selected = true, root = 'all', current = true } = {}) { + const log = []; + const page = { + filters: { root, subfolder: root === 'all' ? '' : 'sub' }, + current: started => { log.push(['current', started]); return current; }, + selected: image => selected && image.path === OLD.path, + inspect: image => log.push(['inspect', image.root_index]), + dropSelection: () => log.push(['drop']), + stream: { + refresh: async () => { log.push(['refresh']); throw new Error('read failed'); }, + reset: (filters, options) => log.push(['reset', { ...filters }, options]), + }, + updateScope: () => log.push(['scope']), + closeViewer: () => log.push(['close-viewer']), + clearChecks: () => log.push(['clear-checks']), + listsNow: () => true, + rediscover: () => log.push(['rediscover']), + }; + return { page, log }; +} + +test('a renumbered source keeps nothing by its old key: only the file stays selected', async () => { + const { recoveredSource } = await load(); + const { page, log } = fakePage({ root: '0' }); + assert.equal(await recoveredSource(page)(OLD, FRESH, 3), FRESH); + assert.deepEqual(log, [['current', 3], ['close-viewer'], ['clear-checks'], ['inspect', 1], ['scope'], + ['reset', { root: '1', subfolder: 'sub' }, { load: true }], ['rediscover']]); +}); + +test('another Source filter falls back to All sources; an unselected file drops the old selection', async () => { + const { recoveredSource } = await load(); + const { page, log } = fakePage({ root: '2', selected: false }); + await recoveredSource(page)(OLD, FRESH, 0); + assert.deepEqual(log.find(entry => entry[0] === 'reset')[1], { root: 'all', subfolder: '' }); + assert.ok(log.some(entry => entry[0] === 'drop')); + assert.ok(!log.some(entry => entry[0] === 'inspect')); + const all = fakePage({ root: 'all' }); + await recoveredSource(all.page)(OLD, FRESH, 0); + assert.deepEqual(all.log.find(entry => entry[0] === 'reset')[1], { root: 'all', subfolder: '' }); +}); + +test('an unchanged numbering re-reads in place; a page that moved on is left alone', async () => { + const { recoveredSource } = await load(); + const same = { ...FRESH, root_index: 0 }; + const { page, log } = fakePage(); + assert.equal(await recoveredSource(page)(OLD, same, 1), same); + assert.deepEqual(log, [['current', 1], ['inspect', 0], ['refresh']], 'a failed re-read is not an error'); + const unselected = fakePage({ selected: false }); + await recoveredSource(unselected.page)(OLD, same, 1); + assert.deepEqual(unselected.log, [['current', 1], ['refresh']]); + const moved = fakePage({ current: false }); + assert.equal(await recoveredSource(moved.page)(OLD, FRESH, 1), FRESH, 'the dialog may still retarget'); + assert.deepEqual(moved.log, [['current', 1]]); + await flush(); +}); + +test('a scope that still names its root is not stale: nothing is applied', async () => { + const { recoveredSource } = await load(); + const { page, log } = fakePage(); + assert.equal(await recoveredSource(page)(OLD, { ...OLD, url: '/u' }, 1), null); + assert.deepEqual(log, []); +}); + +test('a lost rescope answer is not remembered: the pair is asked again', async () => { + const { createSourceRecovery } = await load(); + let down = true; + const api = { calls: 0, post: async () => { api.calls += 1; if (down) throw Object.assign(new Error('bad gateway'), { status: 502 }); return { success: true, image: FRESH }; } }; + const recovery = createSourceRecovery({ api, apply: async (_image, fresh) => fresh }); + assert.equal(await recovery.recover(OLD), null); + down = false; + assert.deepEqual(await recovery.recover(OLD), FRESH); + assert.equal(api.calls, 2); +}); + +test('the same index under another scope (a changed key) re-reads in place and keeps the selection', async () => { + const { recoveredSource } = await load(); + const rekeyed = { ...OLD, root_scope: 'c'.repeat(64) }; + const { page, log } = fakePage(); + page.stream.refresh = async () => { log.push(['refresh']); }; + assert.equal(await recoveredSource(page)(OLD, rekeyed, 2), rekeyed); + assert.deepEqual(log, [['current', 2], ['inspect', 0], ['refresh']]); + assert.ok(!log.some(entry => ['reset', 'drop', 'clear-checks', 'close-viewer'].includes(entry[0]))); +}); diff --git a/tests/js/workspace-system-backups.test.js b/tests/js/workspace-system-backups.test.js index 3a1454ae..4a5550fc 100644 --- a/tests/js/workspace-system-backups.test.js +++ b/tests/js/workspace-system-backups.test.js @@ -192,6 +192,71 @@ test('views reload once after a restore this page saw finish; a failed restore c assert.equal(listeners.size, 0); }); +// #415: a fake document whose open dialogs the test sets, and a MutationObserver stand-in it triggers. +function dialogPage() { + const page = { open: 0, observers: new Set() }; + page.doc = { documentElement: {}, querySelector: selector => (selector === 'dialog[open]' && page.open > 0 ? {} : null) }; + page.Observer = class { + constructor(callback) { this.callback = callback; } + observe(target, options) { this.target = target; this.options = options; page.observers.add(this); } + disconnect() { page.observers.delete(this); } + }; + page.mutate = open => { page.open = open; for (const observer of [...page.observers]) observer.callback([]); }; + return page; +} + +test('the restore reload runs at once when no dialog is open', async () => { + const { createRestoreReload } = await backups; + const page = dialogPage(); + let reloads = 0; + const reload = createRestoreReload(() => { reloads += 1; }, { doc: page.doc, Observer: page.Observer }); + reload.request(); + assert.equal(reloads, 1); + assert.equal(page.observers.size, 0, 'nothing waits'); + reload.request(); + assert.equal(reloads, 2); +}); + +test('the restore reload waits for every open dialog to close, then runs once (#415)', async () => { + const { createRestoreReload } = await backups; + const page = dialogPage(); + page.open = 1; + let reloads = 0; + const reload = createRestoreReload(() => { reloads += 1; }, { doc: page.doc, Observer: page.Observer }); + reload.request(); + reload.request(); + assert.equal(reloads, 0, 'an open dialog holds the reload'); + assert.equal(page.observers.size, 1, 'one observer for repeated requests'); + const [observer] = page.observers; + assert.equal(observer.target, page.doc.documentElement); + assert.deepEqual(observer.options, { subtree: true, childList: true, attributes: true, attributeFilter: ['open'] }); + page.mutate(1); + assert.equal(reloads, 0, 'a dialog swapped for another keeps it waiting'); + page.mutate(0); + assert.equal(reloads, 1); + assert.equal(page.observers.size, 0, 'the observer stops once it ran'); + page.mutate(0); + assert.equal(reloads, 1); + page.open = 1; + reload.request(); + page.mutate(0); + assert.equal(reloads, 2, 'a later restore waits again'); +}); + +test('disposing drops a waiting restore reload', async () => { + const { createRestoreReload } = await backups; + const page = dialogPage(); + page.open = 1; + let reloads = 0; + const reload = createRestoreReload(() => { reloads += 1; }, { doc: page.doc, Observer: page.Observer }); + reload.request(); + reload.dispose(); + assert.equal(page.observers.size, 0); + page.mutate(0); + assert.equal(reloads, 0); + reload.dispose(); +}); + test('a downloaded backup the server could not verify says so', async () => { const { downloadDatabaseBackup, BACKUP_DONE, backupUnverified } = await scan; const toasts = []; diff --git a/tests/js/workspace-toolbar.test.js b/tests/js/workspace-toolbar.test.js index 3ee4cc0c..ad4d8641 100644 --- a/tests/js/workspace-toolbar.test.js +++ b/tests/js/workspace-toolbar.test.js @@ -908,6 +908,34 @@ test('openReviewDialog lays out heading, grid, options, sentence, backup box and } }); +// #415: a finished restore ends an open review (its reviewed plan no longer describes storage) as a dismissal, and +// leaves any other dialog, with what was typed in it, open. +test('closeReviewDialog closes only an open review, as dismissed', async () => { + const { openReviewDialog, openInputDialog, closeReviewDialog, closeWorkspaceDialog } = await dialogsModule; + const { createScope } = await coreModule; + const dom = fakeDOM(); + const scope = createScope(); + try { + closeReviewDialog(); + const outcomes = []; + openInputDialog({ scope, title: 'Save view', label: 'Name', value: 'typed', action: 'Save', onApply() {} }); + closeReviewDialog(); + assert.equal(modal(dom).open, true, 'an input dialog stays'); + assert.equal(byId(dom, 'action-input').value, 'typed'); + openReviewDialog({ scope, title: 'Restore preview', grid: [['Integrity', 'Verified']], cancelLabel: 'Cancel', + primary: { label: 'Restore', danger: true }, onStart: () => outcomes.push('start'), onDismiss: () => outcomes.push('dismiss') }); + closeReviewDialog(); + assert.deepEqual(outcomes, ['dismiss']); + assert.equal(modal(dom), undefined, 'the shell is gone'); + closeReviewDialog(); + assert.deepEqual(outcomes, ['dismiss']); + } finally { + scope.dispose(); + closeWorkspaceDialog(); + dom.restore(); + } +}); + test('review dialog backup box keeps the dialog open and toasts the outcome', async () => { const { openReviewDialog, closeWorkspaceDialog, SCAN_BACKUP_COPY } = await dialogsModule; const { downloadDatabaseBackup, BACKUP_DONE, BACKUP_FAILED } = await scanModule; diff --git a/tests/test_api_images.py b/tests/test_api_images.py index cc74c6cf..0775bcb1 100644 --- a/tests/test_api_images.py +++ b/tests/test_api_images.py @@ -2556,9 +2556,10 @@ class TestRootScopeServing(ImageAPITestCase): listing = await self.client.get("/prompt_manager/images/output?mode=workspace") descriptor = (await listing.json())["images"][0] self.assertRegex(descriptor.get("root_scope", ""), r"^[0-9a-f]{64}$") - with patch.object( - images_module, "_WORKSPACE_CURSOR_SECRET", b"different-process-key" - ): + from utils.workspace_keys import WorkspaceKeys + + rotated = WorkspaceKeys(b"n" * 32, b"m" * 32, False) + with patch.object(self.api, "_workspace_keys", return_value=rotated): await self.assert_no_access("image.png", scope=descriptor["root_scope"]) self.api._get_all_output_dirs = lambda: [] await self.assert_no_access("image.png", scope=descriptor["root_scope"]) diff --git a/tests/test_api_prompt_inspector.py b/tests/test_api_prompt_inspector.py index 8d301060..21ed8023 100644 --- a/tests/test_api_prompt_inspector.py +++ b/tests/test_api_prompt_inspector.py @@ -1418,9 +1418,10 @@ class TestPromptInspector(ImageAPITestCase): async def test_root_scope_rotated_key_and_removed_index_skip_reader_and_db(self): png(self.output_dir / "image.png", prompt=graph()) descriptor = await self.scoped_descriptor() - import py.api.images as images_module + from utils.workspace_keys import WorkspaceKeys - with patch.object(images_module, "_WORKSPACE_CURSOR_SECRET", b"new-key"): + rotated = WorkspaceKeys(b"n" * 32, b"m" * 32, False) + with patch.object(self.api, "_workspace_keys", return_value=rotated): await self.assert_scope_skips_access(descriptor.get("root_scope", "0" * 64)) self.api._get_all_output_dirs = lambda: [] await self.assert_scope_skips_access(descriptor.get("root_scope", "0" * 64)) diff --git a/tests/test_api_rescope.py b/tests/test_api_rescope.py new file mode 100644 index 00000000..f753b0ed --- /dev/null +++ b/tests/test_api_rescope.py @@ -0,0 +1,169 @@ +"""POST /prompt_manager/images/rescope: a stale target's current identity (#430). + +A page that holds ``{root_index, root_scope, path}`` after the sources were +renumbered asks where that file is now. The server maps the target only when +its scope was issued, at its old index, for one of the current roots, so the +answer names nothing the page could not already address; a removed or +replaced root, a foreign scope or a changed key stay refused. +""" + +import json +import os +import sys +from unittest.mock import patch + +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + +try: + from test_api_images import ImageAPITestCase, make_png +except ImportError: # imported as tests.test_api_rescope + from tests.test_api_images import ImageAPITestCase, make_png + +from utils.workspace_keys import WorkspaceKeys # noqa: E402 + +RESCOPE = "/prompt_manager/images/rescope" + + +class TestRescopeRoute(ImageAPITestCase): + async def get_application(self): + app = await super().get_application() + self.other_dir = self.comfy_dir / "other" + self.other_dir.mkdir() + make_png(self.output_dir / "sub" / "a.png") + make_png(self.other_dir / "sub" / "a.png", color=(0, 0, 255)) + return app + + def target(self, path="sub/a.png", index=0, root=None): + root = root or self.output_dir + return { + "root_index": index, + "root_scope": self.api._gallery_root_scope(root, index), + "path": path, + } + + async def post(self, body): + response = await self.client.post(RESCOPE, json=body) + data = await response.json() + self.assertNotIn(str(self.comfy_dir), json.dumps(data)) + self.assertEqual(response.headers.get("Cache-Control"), "no-store") + return response.status, data + + async def listed(self): + response = await self.client.get( + "/prompt_manager/images/output", params={"mode": "workspace"} + ) + return { + (item["root_index"], item["path"]): item + for item in (await response.json())["images"] + } + + async def test_an_unchanged_root_answers_the_listing_descriptor(self): + status, data = await self.post({"target": self.target()}) + self.assertEqual(status, 200) + self.assertTrue(data["success"]) + self.assertEqual(data["image"], (await self.listed())[(0, "sub/a.png")]) + + async def test_a_renumbered_root_maps_to_its_new_index(self): + held = self.target() + self.api._get_all_output_dirs = lambda: [self.other_dir, self.output_dir] + status, data = await self.post({"target": held}) + self.assertEqual(status, 200) + image = data["image"] + self.assertEqual((image["root_index"], image["path"]), (1, "sub/a.png")) + self.assertEqual( + image["root_scope"], self.api._gallery_root_scope(self.output_dir, 1) + ) + self.assertEqual(image, (await self.listed())[(1, "sub/a.png")]) + + async def test_a_removed_replaced_or_foreign_root_is_source_changed(self): + held = self.target() + cases = { + "removed": lambda: [], + "replaced": lambda: [self.other_dir], + } + for name, roots in cases.items(): + with self.subTest(name): + self.api._get_all_output_dirs = roots + status, data = await self.post({"target": held}) + self.assertEqual((status, data["code"]), (409, "source-changed")) + self.api._get_all_output_dirs = lambda: [self.output_dir] + for scope in ("0" * 64, self.api._gallery_root_scope(self.output_dir, 1)): + with self.subTest(scope=scope[:8]): + status, data = await self.post( + {"target": {**held, "root_scope": scope}} + ) + self.assertEqual((status, data["code"]), (409, "source-changed")) + + async def test_a_changed_key_cannot_be_mapped(self): + held = self.target() + rotated = WorkspaceKeys(b"n" * 32, b"m" * 32, False) + with patch.object(self.api, "_workspace_keys", return_value=rotated): + status, data = await self.post({"target": held}) + self.assertEqual((status, data["code"]), (409, "source-changed")) + + async def test_the_first_of_two_spellings_of_one_root_wins(self): + held = self.target() + self.api._get_all_output_dirs = lambda: [ + self.other_dir, + self.output_dir, + self.output_dir / "sub" / "..", + ] + status, data = await self.post({"target": held}) + self.assertEqual((status, data["image"]["root_index"]), (200, 1)) + + async def test_a_missing_file_is_not_found(self): + status, data = await self.post({"target": self.target("sub/gone.png")}) + self.assertEqual((status, data["code"]), (404, "file-missing")) + (self.output_dir / "dir.png").mkdir() + status, data = await self.post({"target": self.target("dir.png")}) + self.assertEqual((status, data["code"]), (404, "file-missing")) + + async def test_a_path_leaving_the_root_is_refused(self): + outside = make_png(self.comfy_dir / "outside.png") + self._make_symlink(outside, self.output_dir / "escape.png") + status, _data = await self.post({"target": self.target("escape.png")}) + self.assertEqual(status, 403) + + async def test_previews_are_not_rescoped(self): + make_png(self.output_dir / "thumbnails" / "a_thumb.png") + status, _data = await self.post( + {"target": self.target("thumbnails/a_thumb.png")} + ) + self.assertEqual(status, 403) + + async def test_malformed_bodies_are_refused_before_any_file_access(self): + good = self.target() + bodies = [ + b"{", + [], + {}, + {"target": good, "extra": 1}, + {"target": {**good, "root_index": -1}}, + {"target": {**good, "root_index": True}}, + {"target": {**good, "root_index": 4096}}, + # Past Python's int-to-text digit limit: never a 500. + b'{"target": {"root_index": 1' + b"0" * 5000 + b"}}", + {"target": {**good, "root_scope": "A" * 64}}, + {"target": {**good, "root_scope": "abc"}}, + {"target": {**good, "path": ""}}, + {"target": {**good, "path": "../a.png"}}, + {"target": {**good, "path": "/abs.png"}}, + {"target": {**good, "path": "C:/a.png"}}, + {"target": {**good, "path": "sub\\a.png"}}, + {"target": {**good, "path": "sub//a.png"}}, + {"target": {**good, "path": "notes.txt"}}, + {"target": {**good, "path": "x" * 2000 + ".png"}}, + ] + with patch("pathlib.Path.is_file", side_effect=AssertionError("file access")): + for body in bodies: + with self.subTest(body=str(body)[:60]): + if isinstance(body, bytes): + response = await self.client.post( + RESCOPE, + data=body, + headers={"Content-Type": "application/json"}, + ) + else: + response = await self.client.post(RESCOPE, json=body) + self.assertIn(response.status, (400, 403)) + self.assertFalse((await response.json())["success"]) diff --git a/tests/test_api_workspace_images.py b/tests/test_api_workspace_images.py index cd998722..82507e7a 100644 --- a/tests/test_api_workspace_images.py +++ b/tests/test_api_workspace_images.py @@ -1882,9 +1882,13 @@ class TestWorkspaceCursorEncoding(unittest.TestCase): for order in ("newest", "oldest", "name", "name-desc"): with self.subTest(sort=order): key = _workspace_key(entry, order) - token = _workspace_cursor(key, "scope") + token = _workspace_cursor(key, "scope", b"k" * 32) self.assertLessEqual(len(token), 4096) - self.assertEqual(_workspace_cursor_key(token, "scope", order), key) + self.assertEqual( + _workspace_cursor_key(token, "scope", order, b"k" * 32), key + ) + with self.assertRaises(ValueError): + _workspace_cursor_key(token, "scope", order, b"o" * 32) # The pre-#379 candidate read, kept as the differential reference for the diff --git a/tests/test_api_workspace_restart.py b/tests/test_api_workspace_restart.py new file mode 100644 index 00000000..d975c640 --- /dev/null +++ b/tests/test_api_workspace_restart.py @@ -0,0 +1,218 @@ +"""A ComfyUI restart keeps an open workspace page's scopes and cursors (#430). + +A restart is a new ``PromptManagerAPI`` on the same database in a new +process. Here a second API instance on its own aiohttp server stands in for +it (``tests/test_workspace_keys.py`` also derives the scope in a real child +process); it must accept every ``root_scope`` and listing cursor the first one +handed out, while a changed root list or key still refuses them. +""" + +import base64 +import json +import os +import sys +from unittest.mock import patch +from urllib.parse import quote + +from aiohttp import web +from aiohttp.test_utils import TestClient, TestServer + +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + +try: + from test_api_images import ImageAPITestCase, make_png +except ImportError: # imported as tests.test_api_workspace_restart + from tests.test_api_images import ImageAPITestCase, make_png + +from database.operations import PromptDatabase # noqa: E402 +from py.api import PromptManagerAPI # noqa: E402 +from utils.workspace_keys import KEY_FILE_NAME, WorkspaceKeys # noqa: E402 + +LISTING = "/prompt_manager/images/output" + + +def _payload(token): + raw = base64.urlsafe_b64decode(token + "=" * (-len(token) % 4)) + return json.loads(raw[32:]) + + +class TestRestartKeepsTheOpenPage(ImageAPITestCase): + async def get_application(self): + app = await super().get_application() + for index in range(3): + make_png(self.output_dir / f"image_{index}.png") + return app + + async def first_page(self, client=None, **params): + response = await (client or self.client).get( + LISTING, params={"mode": "workspace", "limit": "1", **params} + ) + self.assertEqual(response.status, 200) + return await response.json() + + async def restarted(self): + """A second API on the same database and roots, on its own server.""" + api = PromptManagerAPI(db=PromptDatabase(self._temp_db.name)) + api._get_all_output_dirs = lambda: [self.output_dir] + api._find_comfyui_output_dir = lambda: str(self.output_dir) + app = web.Application() + routes = web.RouteTableDef() + api.add_routes(routes) + app.router.add_routes(routes) + client = TestClient(TestServer(app)) + await client.start_server() + + async def close(): + await client.close() + api.close_auto_thumbnails() + api.db.close_all() + + self.addAsyncCleanup(close) + return api, client + + async def test_a_restart_keeps_every_held_scope_and_cursor_valid(self): + page = await self.first_page() + image, cursor = page["images"][0], page["next_cursor"] + self.assertTrue( + os.path.isfile( + os.path.join(os.path.dirname(self._temp_db.name), KEY_FILE_NAME) + ) + ) + _api, client = await self.restarted() + + more = await client.get( + LISTING, params={"mode": "workspace", "limit": "1", "cursor": cursor} + ) + self.assertEqual(more.status, 200, await more.text()) + self.assertNotEqual((await more.json())["images"][0]["path"], image["path"]) + + inspected = await client.get( + f"/prompt_manager/images/inspect/{quote(image['path'])}", + params={"root": "0", "root_scope": image["root_scope"]}, + ) + self.assertEqual(inspected.status, 200, await inspected.text()) + served = await client.get( + f"/prompt_manager/images/serve/{quote(image['path'])}", + params={"root": "0", "root_scope": image["root_scope"]}, + ) + self.assertEqual(served.status, 200) + relisted = await self.first_page(client) + self.assertEqual(relisted["images"][0]["root_scope"], image["root_scope"]) + + async def test_a_key_change_refuses_the_cursor_as_stale(self): + cursor = (await self.first_page())["next_cursor"] + rotated = WorkspaceKeys(b"n" * 32, b"m" * 32, False) + with patch.object(self.api, "_workspace_keys", return_value=rotated): + response = await self.client.get( + LISTING, params={"mode": "workspace", "limit": "1", "cursor": cursor} + ) + self.assertEqual(response.status, 400) + self.assertEqual((await response.json())["code"], "cursor-stale") + + async def test_renumbered_roots_refuse_the_cursor_as_stale(self): + cursor = (await self.first_page())["next_cursor"] + other = self.comfy_dir / "other" + other.mkdir() + self.api._get_all_output_dirs = lambda: [other, self.output_dir] + response = await self.client.get( + LISTING, params={"mode": "workspace", "limit": "1", "cursor": cursor} + ) + self.assertEqual(response.status, 400) + self.assertEqual((await response.json())["code"], "cursor-stale") + + async def test_another_listing_refuses_the_cursor_as_stale(self): + cursor = (await self.first_page())["next_cursor"] + response = await self.client.get( + LISTING, + params={"mode": "workspace", "limit": "1", "cursor": cursor, "q": "x"}, + ) + self.assertEqual(response.status, 400) + self.assertEqual((await response.json())["code"], "cursor-stale") + + async def test_a_malformed_cursor_is_a_plain_bad_request(self): + # A 32-byte signature over a payload is well formed: a wrong one is stale. + for cursor in ("!!", "a" * 5000, base64.urlsafe_b64encode(b"x" * 20).decode()): + with self.subTest(cursor=cursor[:10]): + response = await self.client.get( + LISTING, + params={"mode": "workspace", "limit": "1", "cursor": cursor}, + ) + self.assertEqual(response.status, 400) + self.assertNotEqual((await response.json()).get("code"), "cursor-stale") + + async def test_the_cursor_carries_no_digest_of_the_root_paths(self): + cursor = (await self.first_page())["next_cursor"] + payload = _payload(cursor) + # v2: the listing scope (it hashes every absolute root) is in the MAC only. + self.assertEqual(set(payload), {"v", "key"}) + self.assertEqual(payload["v"], 2) + + async def test_another_database_folder_gets_other_scopes(self): + page = await self.first_page() + other_db = self.comfy_dir / "elsewhere" / "prompts.db" + other_db.parent.mkdir() + api = PromptManagerAPI(db=PromptDatabase(str(other_db))) + self.addCleanup(api.db.close_all) + self.addCleanup(api.close_auto_thumbnails) + self.assertNotEqual( + api._gallery_root_scope(self.output_dir, 0), page["images"][0]["root_scope"] + ) + + +# A fresh interpreter: the API on the given database, as after a restart. +_CHILD = """ +import json, sys +from pathlib import Path +from unittest.mock import MagicMock +sys.path.insert(0, sys.argv[1]) +from tests.host_stubs import install_folder_paths_stub +install_folder_paths_stub() +sys.modules.setdefault("server", MagicMock()) +from database.operations import PromptDatabase +from py.api import PromptManagerAPI +from py.api.images import _workspace_cursor_key +api = PromptManagerAPI(db=PromptDatabase(sys.argv[2])) +scope = api._gallery_root_scope(Path(sys.argv[3]), 0) +secret = api._workspace_keys().cursor +key = _workspace_cursor_key(sys.argv[4], "s" * 64, "newest", secret) +api.close_auto_thumbnails() +api.db.close_all() +print(json.dumps({"scope": scope, "key": list(key)})) +""" + + +class TestRestartInANewProcess(ImageAPITestCase): + """The real thing: two other interpreters on the same database.""" + + def child(self, cursor): + import subprocess + + repo = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + result = subprocess.run( + [ + sys.executable, + "-c", + _CHILD, + repo, + self._temp_db.name, + str(self.output_dir), + cursor, + ], + cwd=self.comfy_dir, + capture_output=True, + text=True, + timeout=120, + ) + self.assertEqual(result.returncode, 0, result.stderr[-2000:]) + return json.loads(result.stdout.strip().splitlines()[-1]) + + async def test_new_processes_derive_the_same_scope_and_accept_the_cursor(self): + from py.api.images import _workspace_cursor + + make_png(self.output_dir / "image.png") + listing = await self.client.get(LISTING, params={"mode": "workspace"}) + scope = (await listing.json())["images"][0]["root_scope"] + key = (-12.5, "image.png", "0" * 64, 0) + cursor = _workspace_cursor(key, "s" * 64, self.api._workspace_keys().cursor) + for _restart in range(2): + self.assertEqual(self.child(cursor), {"scope": scope, "key": list(key)}) diff --git a/tests/test_browser_fast_reset.py b/tests/test_browser_fast_reset.py index 6be9a54a..c23fae43 100644 --- a/tests/test_browser_fast_reset.py +++ b/tests/test_browser_fast_reset.py @@ -22,6 +22,9 @@ from database.operations import PromptDatabase from tests.browser import fast_reset from tests.browser.fast_reset import ScratchSeeder, table_digests +# The owner lock of /dev/shm scratch folders needs POSIX flock (#419). +HAS_FLOCK = unittest.skipUnless(os.name == "posix", "needs POSIX flock") + def digests(db: PromptDatabase) -> dict: return table_digests(db.model.get_connection()) @@ -275,6 +278,7 @@ class TestScratchSeeder(unittest.TestCase): self.assertEqual(digests(self.live), expected) self.assertFalse((self.root / "unused").exists()) + @HAS_FLOCK def test_the_scratch_lives_under_the_memory_root_when_there_is_one(self): memory = self.root / "memory" memory.mkdir() @@ -288,6 +292,16 @@ class TestScratchSeeder(unittest.TestCase): seeder.close() self.assertEqual(list(memory.iterdir()), []) + @unittest.skipIf(os.name == "posix", "without flock the memory root is unused") + def test_without_flock_the_scratch_stays_on_disk(self): + memory = self.root / "memory" + memory.mkdir() + seeder = ScratchSeeder(self.api, self.root / "fallback", memory_root=memory) + self.addCleanup(seeder.close) + seeder.run(reseed) + self.assertEqual(list(memory.iterdir()), []) + self.assertTrue((self.root / "fallback" / "seed.db").exists()) + def test_a_full_memory_root_puts_the_scratch_on_disk(self): memory = self.root / "memory" memory.mkdir() @@ -303,26 +317,32 @@ class TestScratchSeeder(unittest.TestCase): self.assertEqual(list(memory.iterdir()), []) self.assertTrue((self.root / "fallback" / "seed.db").exists()) - @unittest.skipUnless(os.name == "posix", "process probe is POSIX only") - def test_scratch_folders_of_dead_servers_are_removed(self): + @HAS_FLOCK + def test_the_scratch_folder_is_held_by_a_lock_until_close(self): memory = self.root / "memory" memory.mkdir() - dead = subprocess.Popen([sys.executable, "-c", "pass"]) - dead.wait() - orphan = memory / f"pm-reset-{dead.pid}-abc" - alive = memory / f"pm-reset-{os.getpid()}-def" - other = memory / "pm-reset-notapid" - for folder in (orphan, alive, other): - folder.mkdir() - (folder / "seed.db").write_bytes(b"x") seeder = ScratchSeeder(self.api, self.root / "fallback", memory_root=memory) self.addCleanup(seeder.close) seeder.run(reseed) - self.assertFalse(orphan.exists()) - self.assertTrue(alive.exists()) - self.assertTrue(other.exists()) - own = [p for p in memory.glob(f"pm-reset-{os.getpid()}-*") if p != alive] - self.assertEqual(len(own), 1) + (folder,) = memory.iterdir() + self.assertTrue(folder.name.startswith(f"pm-reset-held-{os.getpid()}-")) + self.assertFalse(lock_is_free(folder / "owner.lock")) + seeder.close() + self.assertEqual(list(memory.iterdir()), []) + + @HAS_FLOCK + def test_a_lock_that_cannot_be_taken_puts_the_scratch_on_disk(self): + memory = self.root / "memory" + memory.mkdir() + seeder = ScratchSeeder(self.api, self.root / "fallback", memory_root=memory) + self.addCleanup(seeder.close) + refuse = mock.patch.object( + fast_reset.fcntl, "flock", side_effect=OSError("no locks here") + ) + with refuse: + seeder.run(reseed) + self.assertEqual(list(memory.iterdir()), []) + self.assertTrue((self.root / "fallback" / "seed.db").exists()) def test_close_releases_the_scratch_files(self): self.seeder.run(reseed) @@ -331,5 +351,95 @@ class TestScratchSeeder(unittest.TestCase): self.seeder.close() # idempotent +# A process that takes the owner lock of a folder, says so, and waits. +HOLDER = """ +import fcntl, os, sys, time +fd = os.open(os.path.join(sys.argv[1], "owner.lock"), os.O_RDWR | os.O_CREAT) +fcntl.flock(fd, fcntl.LOCK_EX) +print("held", flush=True) +time.sleep(600) +""" + + +def lock_is_free(path: Path) -> bool: + fd = os.open(path, os.O_RDWR) + try: + fast_reset.fcntl.flock(fd, fast_reset.fcntl.LOCK_EX | fast_reset.fcntl.LOCK_NB) + except BlockingIOError: + return False + finally: + os.close(fd) + return True + + +@HAS_FLOCK +class TestOrphanedScratch(unittest.TestCase): + """Only folders whose owner lock is free are orphans (#419).""" + + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.addCleanup(self.tmp.cleanup) + self.memory = Path(self.tmp.name) + + def folder(self, name: str) -> Path: + folder = self.memory / name + folder.mkdir() + (folder / "seed.db").write_bytes(b"x") + return folder + + def hold(self, folder: Path) -> subprocess.Popen: + holder = subprocess.Popen( + [sys.executable, "-c", HOLDER, str(folder)], + stdout=subprocess.PIPE, + text=True, + ) + self.addCleanup(holder.wait) + self.addCleanup(holder.stdout.close) + self.addCleanup(holder.kill) + self.assertEqual(holder.stdout.readline().strip(), "held") + return holder + + def test_a_folder_whose_owner_was_killed_is_removed(self): + folder = self.folder("pm-reset-held-123-abc") + holder = self.hold(folder) + holder.kill() + holder.wait() + fast_reset.remove_orphaned_scratch(self.memory) + self.assertFalse(folder.exists()) + + def test_a_held_folder_is_kept_whatever_pid_its_name_carries(self): + # Under --ipc=host the owner's pid is from another PID namespace: it + # does not exist here, but its lock does. + dead = subprocess.Popen([sys.executable, "-c", "pass"]) + dead.wait() + folder = self.folder(f"pm-reset-held-{dead.pid}-abc") + self.hold(folder) + fast_reset.remove_orphaned_scratch(self.memory) + self.assertTrue(folder.exists()) + self.assertTrue((folder / "seed.db").exists()) + + def test_folders_without_an_owner_lock_are_kept(self): + dead = subprocess.Popen([sys.executable, "-c", "pass"]) + dead.wait() + legacy = self.folder(f"pm-reset-{dead.pid}-abc") + staging = self.folder(f"pm-reset-new-{dead.pid}-def") + unlocked = self.folder(f"pm-reset-held-{dead.pid}-ghi") + fast_reset.remove_orphaned_scratch(self.memory) + for folder in (legacy, staging, unlocked): + self.assertTrue(folder.exists(), folder.name) + + def test_a_folder_held_by_another_seeder_in_this_process_is_kept(self): + api = types.SimpleNamespace(db=PromptDatabase(str(self.memory / "live.db"))) + self.addCleanup(api.db.close_all) + root = self.memory / "shm" + root.mkdir() + other = ScratchSeeder(api, self.memory / "fallback", memory_root=root) + self.addCleanup(other.close) + other.run(reseed) + (folder,) = root.iterdir() + fast_reset.remove_orphaned_scratch(root) + self.assertTrue(folder.exists()) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_library_move.py b/tests/test_library_move.py index b838af3a..9a450fde 100644 --- a/tests/test_library_move.py +++ b/tests/test_library_move.py @@ -209,6 +209,38 @@ class TestMove(_MoveCase): ) self.assertEqual(ll.read_marker(self.target)["state"], ll.MARKER_MOVED) + def test_the_workspace_key_moves_with_the_library(self): + # #430: open pages keep their scopes after the restart that follows. + from utils.workspace_keys import KEY_FILE_NAME, load_install_secret + + secret = load_install_secret(self.ext) + result = self.move() + self.assertIn(KEY_FILE_NAME, result.files) + self.assertEqual(load_install_secret(self.target), secret) + if os.name == "posix": + mode = os.stat(os.path.join(self.target, KEY_FILE_NAME)).st_mode + self.assertEqual(mode & 0o777, 0o600) + + def test_a_key_that_cannot_be_copied_never_fails_the_move(self): + from utils.workspace_keys import KEY_FILE_NAME, load_install_secret + + load_install_secret(self.ext) + real = lm._hash_copy + + def failing(source, destination): + if os.path.basename(source) == KEY_FILE_NAME: + raise OSError("disk error") + return real(source, destination) + + with mock.patch.object(lm, "_hash_copy", failing): + with self.assertLogs("prompt_manager.database.library_move", "WARNING"): + result = self.move() + self.assertNotIn(KEY_FILE_NAME, result.files) + self.assertEqual(_rows(self.new_db), self.before["rows"]) + self.assertFalse( + any(n.startswith(lm.TEMP_PREFIX) for n in os.listdir(self.target)) + ) + def test_new_files_are_private(self): if os.name != "posix": self.skipTest("POSIX permissions") diff --git a/tests/test_workspace_keys.py b/tests/test_workspace_keys.py new file mode 100644 index 00000000..b3ed70da --- /dev/null +++ b/tests/test_workspace_keys.py @@ -0,0 +1,247 @@ +"""The per-install workspace secret (#430, utils/workspace_keys.py). + +A ComfyUI restart must not change the gallery root scopes or the workspace +cursors while the output roots stay the same, so the secret that keys them is +persisted beside the database instead of drawn per process. +""" + +import importlib +import json +import logging +import os +import stat +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path +from unittest.mock import patch + +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + +import utils.workspace_keys as workspace_keys # noqa: E402 +from utils.workspace_keys import ( # noqa: E402 + KEY_FILE_NAME, + WorkspaceKeys, + gallery_root_scope, + load_install_secret, + workspace_keys_for, +) + +REPO = Path(__file__).resolve().parents[1] + + +class TempFolderCase(unittest.TestCase): + def setUp(self): + folder = tempfile.TemporaryDirectory(ignore_cleanup_errors=True) + self.addCleanup(folder.cleanup) + self.folder = Path(os.path.realpath(folder.name)) + self.key_file = self.folder / KEY_FILE_NAME + + def leftovers(self): + return sorted(p.name for p in self.folder.iterdir() if p.name != KEY_FILE_NAME) + + +class TestLoadInstallSecret(TempFolderCase): + def test_a_missing_file_is_created_with_64_hex_and_owner_only_mode(self): + secret = load_install_secret(str(self.folder)) + self.assertEqual(len(secret), 32) + text = self.key_file.read_text(encoding="ascii") + self.assertEqual(text, secret.hex() + "\n") + self.assertEqual(self.leftovers(), [], "no temp file is left behind") + if os.name == "posix": + self.assertEqual(stat.S_IMODE(self.key_file.stat().st_mode) & 0o077, 0) + + def test_an_existing_file_is_reused(self): + first = load_install_secret(str(self.folder)) + self.assertEqual(load_install_secret(str(self.folder)), first) + + def test_an_uppercase_or_unterminated_file_is_still_read(self): + self.key_file.write_text("AB" * 32, encoding="ascii") + self.assertEqual( + load_install_secret(str(self.folder)), bytes.fromhex("ab" * 32) + ) + + def test_a_corrupt_file_is_replaced_with_a_warning(self): + for content in (b"", b"short", b"zz" * 32, b"ab" * 33, b"\xff" * 64): + with self.subTest(content=content): + self.key_file.write_bytes(content) + with self.assertLogs("prompt_manager.api", logging.WARNING) as logs: + secret = load_install_secret(str(self.folder)) + self.assertEqual(len(secret), 32) + self.assertEqual( + self.key_file.read_text(encoding="ascii"), secret.hex() + "\n" + ) + self.assertIn("replaced", "\n".join(logs.output)) + self.assertNotIn(secret.hex(), "\n".join(logs.output)) + self.assertEqual(self.leftovers(), []) + + def test_a_lost_creation_race_reads_the_winners_file(self): + winner = "cd" * 32 + "\n" + real_link = os.link + + def racing_link(source, target, *args, **kwargs): + Path(target).write_text(winner, encoding="ascii") + return real_link(source, target, *args, **kwargs) # FileExistsError + + with patch.object(workspace_keys.os, "link", racing_link): + secret = load_install_secret(str(self.folder)) + self.assertEqual(secret, bytes.fromhex("cd" * 32)) + self.assertEqual(self.key_file.read_text(encoding="ascii"), winner) + self.assertEqual(self.leftovers(), []) + + def test_a_file_system_without_hard_links_falls_back_to_replace(self): + def no_link(*args, **kwargs): + raise OSError(1, "Operation not permitted") + + with patch.object(workspace_keys.os, "link", no_link): + secret = load_install_secret(str(self.folder)) + self.assertEqual(self.key_file.read_text(encoding="ascii"), secret.hex() + "\n") + self.assertEqual(self.leftovers(), []) + + def test_an_unwritable_folder_returns_none_and_leaves_nothing(self): + def refuse(*args, **kwargs): + raise PermissionError(13, "Permission denied") + + with patch.object(workspace_keys.os, "open", refuse): + with self.assertLogs("prompt_manager.api", logging.WARNING): + self.assertIsNone(load_install_secret(str(self.folder))) + self.assertFalse(self.key_file.exists()) + with self.assertLogs("prompt_manager.api", logging.WARNING): + self.assertIsNone(load_install_secret(str(self.folder / "missing"))) + + def test_an_unreadable_key_path_returns_none(self): + self.key_file.mkdir() # a folder where the file should be + with self.assertLogs("prompt_manager.api", logging.WARNING): + self.assertIsNone(load_install_secret(str(self.folder))) + + def test_a_refused_replace_of_a_corrupt_key_falls_back(self): + # Windows refuses os.replace while another process holds the file open. + self.key_file.write_text("broken", encoding="ascii") + + def refuse(*args, **kwargs): + raise PermissionError(13, "Access is denied") + + with patch.object(workspace_keys.os, "replace", refuse): + with self.assertLogs("prompt_manager.api", logging.WARNING): + self.assertIsNone(load_install_secret(str(self.folder))) + self.assertEqual(self.key_file.read_text(encoding="ascii"), "broken") + self.assertEqual(self.leftovers(), []) + + def test_a_lost_race_against_an_unfinished_file_is_unusable_not_replaced(self): + def racing_link(source, target, *args, **kwargs): + Path(target).write_text("ab", encoding="ascii") # still being written + raise FileExistsError(target) + + with patch.object(workspace_keys.os, "link", racing_link): + with self.assertLogs("prompt_manager.api", logging.WARNING): + self.assertIsNone(load_install_secret(str(self.folder))) + self.assertEqual(self.key_file.read_text(encoding="ascii"), "ab") + + @unittest.skipUnless(os.name == "posix", "POSIX permission bits") + def test_an_existing_key_readable_by_others_is_tightened(self): + secret = load_install_secret(str(self.folder)) + os.chmod(self.key_file, 0o644) + self.assertEqual(load_install_secret(str(self.folder)), secret) + self.assertEqual(stat.S_IMODE(self.key_file.stat().st_mode), 0o600) + + def test_old_temp_files_are_swept_and_fresh_ones_kept(self): + old = self.folder / f".{KEY_FILE_NAME}.dead.tmp" + fresh = self.folder / f".{KEY_FILE_NAME}.live.tmp" + other = self.folder / "prompts.db" + for path in (old, fresh, other): + path.write_text("x") + os.utime(old, (1, 1)) + load_install_secret(str(self.folder)) + self.assertEqual(self.leftovers(), sorted([fresh.name, other.name])) + + +class TestWorkspaceKeys(TempFolderCase): + def test_purposes_are_domain_separated_and_stable(self): + keys = workspace_keys_for(str(self.folder / "prompts.db")) + self.assertTrue(keys.persisted) + self.assertNotEqual(keys.root_scope, keys.cursor) + again = workspace_keys_for(str(self.folder / "prompts.db")) + self.assertEqual( + (again.root_scope, again.cursor), (keys.root_scope, keys.cursor) + ) + self.assertNotIn( + self.key_file.read_text().strip(), + (keys.root_scope.hex(), keys.cursor.hex()), + ) + + def test_another_database_folder_gets_other_keys(self): + other = tempfile.TemporaryDirectory(ignore_cleanup_errors=True) + self.addCleanup(other.cleanup) + first = workspace_keys_for(str(self.folder / "prompts.db")) + second = workspace_keys_for(os.path.join(other.name, "prompts.db")) + self.assertNotEqual(first.root_scope, second.root_scope) + + def test_an_unusable_folder_falls_back_to_a_process_key_with_a_warning(self): + with patch.object(workspace_keys, "load_install_secret", return_value=None): + with self.assertLogs("prompt_manager.api", logging.WARNING) as logs: + keys = workspace_keys_for(str(self.folder / "prompts.db")) + self.assertFalse(keys.persisted) + self.assertEqual(len(keys.root_scope), 32) + self.assertIn("restart", "\n".join(logs.output)) + + def test_memory_and_blank_paths_never_write_into_the_working_folder(self): + for path in (":memory:", "", None): + with self.subTest(path=path): + with patch.object(workspace_keys, "load_install_secret") as load: + with self.assertLogs("prompt_manager.api", logging.WARNING): + keys = workspace_keys_for(path) + load.assert_not_called() + self.assertFalse(keys.persisted) + + def test_the_scope_is_the_historic_payload_under_the_root_scope_key(self): + import hashlib + import hmac + + keys = WorkspaceKeys(b"r" * 32, b"c" * 32, True) + payload = json.dumps( + ["prompt-manager-gallery-root-v1", 3, "/x/ü"], + separators=(",", ":"), + ensure_ascii=False, + ).encode("utf-8") + self.assertEqual( + gallery_root_scope(keys, "/x/ü", 3), + hmac.new(b"r" * 32, payload, hashlib.sha256).hexdigest(), + ) + + +class TestRestartKeepsScopes(TempFolderCase): + """A new process or a reloaded module derives the same scope (#430).""" + + def test_a_module_reload_keeps_the_scope(self): + db = str(self.folder / "prompts.db") + before = gallery_root_scope(workspace_keys_for(db), "/out", 0) + reloaded = importlib.reload(workspace_keys) + self.addCleanup(importlib.reload, workspace_keys) + self.assertEqual( + reloaded.gallery_root_scope(reloaded.workspace_keys_for(db), "/out", 0), + before, + ) + + def test_a_new_process_derives_the_same_scope(self): + db = str(self.folder / "prompts.db") + before = gallery_root_scope(workspace_keys_for(db), "/out", 0) + code = ( + "import sys\n" + "from utils.workspace_keys import gallery_root_scope, workspace_keys_for\n" + "print(gallery_root_scope(workspace_keys_for(sys.argv[1]), '/out', 0))\n" + ) + # -I would drop the repo from sys.path; the child imports only stdlib code. + result = subprocess.run( + [sys.executable, "-c", code, db], + cwd=REPO, + capture_output=True, + text=True, + timeout=60, + check=True, + ) + self.assertEqual(result.stdout.strip(), before) + + +if __name__ == "__main__": + unittest.main() diff --git a/tools/vendor/frontend-coverage-baseline.json b/tools/vendor/frontend-coverage-baseline.json index 396c4835..b5292c45 100644 --- a/tools/vendor/frontend-coverage-baseline.json +++ b/tools/vendor/frontend-coverage-baseline.json @@ -266,7 +266,7 @@ "workspace/image-tagging.mjs": { "lines": 97, "branches": 92, - "functions": 88, + "functions": 89, "statements": 97 }, "workspace/images-category.mjs": { @@ -299,6 +299,12 @@ "functions": 98, "statements": 98 }, + "workspace/images-source-recovery.mjs": { + "lines": 98, + "branches": 98, + "functions": 98, + "statements": 98 + }, "workspace/images-state.mjs": { "lines": 98, "branches": 98, diff --git a/utils/workspace_keys.py b/utils/workspace_keys.py new file mode 100644 index 00000000..5087c53b --- /dev/null +++ b/utils/workspace_keys.py @@ -0,0 +1,213 @@ +"""The per-install secret behind the workspace's gallery scopes and cursors (#430). + +A page addresses a listed file as ``{root_index, root_scope, path}``. The root +scope is an HMAC over the root's index and canonical path, so the server can +refuse an action aimed at a root that was renumbered or replaced since the page +listed it (``py/api/images.py`` ``_gallery_scope_error``). The listing's keyset +cursors are MACed with a second key, so a forged position never reaches the +sort code. + +Both keys used to be drawn per process, so every ComfyUI restart made every +open workspace tab stale. They now derive from one random secret persisted in +``workspace-scope.key`` beside the database (never in ``config.json`` and never +in the database, so a backup or an export does not carry it). When the folder +cannot hold it, the process falls back to a per-process secret, as before. + +Standard library only: a fresh process can derive the same scope without +importing the API. +""" + +from __future__ import annotations + +import hashlib +import hmac +import json +import logging +import os +import re +import secrets +import stat +import time +from typing import NamedTuple + +logger = logging.getLogger("prompt_manager.api") + +KEY_FILE_NAME = "workspace-scope.key" +SECRET_BYTES = 32 +_HEX = re.compile(r"[0-9a-fA-F]{64}") +# Bounded read: a valid file is 64 hex characters and a newline. +_MAX_FILE_BYTES = 128 +_ROOT_SCOPE_PURPOSE = b"prompt-manager/gallery-root-scope/v1" +_CURSOR_PURPOSE = b"prompt-manager/workspace-cursor/v1" +_SCOPE_TAG = "prompt-manager-gallery-root-v1" +_TEMP_PREFIX = f".{KEY_FILE_NAME}." +_TEMP_SUFFIX = ".tmp" +_STALE_TEMP_SECONDS = 60 + + +class WorkspaceKeys(NamedTuple): + """The derived keys; ``persisted`` is False for a per-process fallback.""" + + root_scope: bytes + cursor: bytes + persisted: bool + + +def _read(path: str) -> bytes | None: + """The secret in *path*, None when it is missing or not 64 hex characters. + + Raises OSError for anything but a missing file (a folder, no permission). + """ + try: + with open(path, "rb") as handle: + raw = handle.read(_MAX_FILE_BYTES + 1) + except FileNotFoundError: + return None + try: + text = raw.decode("ascii").strip() + except UnicodeDecodeError: + return b"" + return bytes.fromhex(text) if _HEX.fullmatch(text) else b"" + + +def _write_exclusive(path: str, secret: bytes) -> None: + """Create *path* (it must not exist) holding *secret*, owner-only, fsynced.""" + flags = os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, "O_BINARY", 0) + descriptor = os.open(path, flags, 0o600) + try: + with os.fdopen(descriptor, "wb") as handle: + handle.write((secret.hex() + "\n").encode("ascii")) + handle.flush() + os.fsync(handle.fileno()) + except BaseException: + _unlink(path) + raise + + +def _unlink(path: str) -> None: + try: + os.unlink(path) + except OSError: + pass + + +def _sweep_temps(directory: str) -> None: + """Remove temp files a crashed first start left (older than a minute).""" + try: + names = os.listdir(directory) + except OSError: + return + cutoff = time.time() - _STALE_TEMP_SECONDS + for name in names: + if name.startswith(_TEMP_PREFIX) and name.endswith(_TEMP_SUFFIX): + path = os.path.join(directory, name) + try: + if os.stat(path).st_mtime < cutoff: + os.unlink(path) + except OSError: + pass + + +def _tighten(path: str) -> None: + """On POSIX an existing key readable by group or others becomes 0o600.""" + if os.name != "posix": + return # the file inherits the folder's ACL, as the database does + try: + if stat.S_IMODE(os.stat(path).st_mode) & 0o077: + os.chmod(path, 0o600) + except OSError: + pass + + +def _store(directory: str, path: str, secret: bytes, *, replace: bool) -> None: + """Publish *secret* at *path*; a new file never overwrites a concurrent winner. + + The secret is written to a temp file first. ``os.link`` then fails when + *path* exists, so of two first starts exactly one file wins + (FileExistsError for the other). A file system without hard links (FAT, + some shares) creates *path* with ``O_EXCL`` instead. *replace* (a corrupt + key) swaps the file with ``os.replace``; Windows refuses that while + another process holds the file open, which the caller treats as unusable. + """ + temp = os.path.join( + directory, f"{_TEMP_PREFIX}{secrets.token_hex(8)}{_TEMP_SUFFIX}" + ) + _write_exclusive(temp, secret) + try: + if replace: + os.replace(temp, path) + return + try: + os.link(temp, path) + except FileExistsError: + raise + except OSError: + # Not atomic: a second first start reading mid-write would see a + # corrupt key. Rare (two first starts on FAT or a share at once); + # each process still uses the file it reads back. + _write_exclusive(path, secret) + finally: + _unlink(temp) + + +def load_install_secret(directory: str) -> bytes | None: + """The persisted secret of *directory*, created on first use; None if unusable. + + After any write the file is read back and its content is what counts, so + processes racing on a first start end with the one secret on disk. + """ + path = os.path.join(directory, KEY_FILE_NAME) + try: + _sweep_temps(directory) + current = _read(path) + if current: + _tighten(path) + return current + corrupt = current is not None + try: + _store(directory, path, secrets.token_bytes(SECRET_BYTES), replace=corrupt) + except FileExistsError: + pass # another process created it first: its file counts + stored = _read(path) + if not stored: + raise OSError(f"{KEY_FILE_NAME} is not readable after it was written") + if corrupt: + logger.warning( + f"{KEY_FILE_NAME} was not a valid key and was replaced; " + "open workspace pages must refresh once" + ) + return stored + except OSError as exc: + logger.warning(f"Could not use {KEY_FILE_NAME}: {exc.__class__.__name__}") + return None + + +def _derive(secret: bytes, purpose: bytes) -> bytes: + return hmac.new(secret, purpose, hashlib.sha256).digest() + + +def workspace_keys_for(db_path: str | None) -> WorkspaceKeys: + """The keys for the library whose database is *db_path* (blocking I/O).""" + secret = None + if isinstance(db_path, str) and db_path.strip() and db_path != ":memory:": + secret = load_install_secret(os.path.dirname(os.path.abspath(db_path))) + persisted = secret is not None + if not persisted: + logger.warning( + "Workspace scopes use a per-process key: open workspace pages must " + "refresh after a restart" + ) + secret = secrets.token_bytes(SECRET_BYTES) + return WorkspaceKeys( + _derive(secret, _ROOT_SCOPE_PURPOSE), + _derive(secret, _CURSOR_PURPOSE), + persisted, + ) + + +def gallery_root_scope(keys: WorkspaceKeys, canonical_root: str, index: int) -> str: + """The 64-hex scope of the root at *index* (its canonical path string).""" + payload = json.dumps( + [_SCOPE_TAG, index, canonical_root], separators=(",", ":"), ensure_ascii=False + ).encode("utf-8") + return hmac.new(keys.root_scope, payload, hashlib.sha256).hexdigest() diff --git a/web/js/workspace/image-tagging-state.mjs b/web/js/workspace/image-tagging-state.mjs index 6bb74132..4cf050c2 100644 --- a/web/js/workspace/image-tagging-state.mjs +++ b/web/js/workspace/image-tagging-state.mjs @@ -34,9 +34,13 @@ const MAX_SUGGESTIONS = 100; const MAX_INSTRUCTION_CHARS = 4000; const EDITABLE = new Set(['ready', 'generate-error', 'write-error', 'conflict']); // Answers after which only a fresh observation can save (the capture or epoch is gone); Retry observes again (D116). -const REOPEN = {'storage-changed': 'storage-changed', 'file-changed': 'file-changed', 'source-changed': 'file-changed', 'capture-expired': 'capture-expired'}; -const REOBSERVE = new Set(Object.values(REOPEN)); +const REOBSERVE = new Set(['storage-changed', 'file-changed', 'capture-expired']); +// #430: a changed source (the page's root scope no longer names its root) is never retried as is: the page re-scopes +// the target (`retarget`) or the dialog offers a refresh. +const REOPEN = Object.freeze({...Object.fromEntries([...REOBSERVE].map(code => [code, code])), 'source-changed': 'source-changed'}); const RETRYABLE = new Set(['observe-error', 'generate-error', 'unknown-outcome', ...REOBSERVE]); +// States whose reason blocks Save (and Generate): the dialog shows it beside its actions (D147). +const BLOCKED = new Set(['observe-error', 'source-changed', 'unknown-outcome', ...REOBSERVE]); // Generic failure envelopes whose fixed sentence the dialog words itself. const GENERIC_CODES = new Set(['observe-failed', 'suggest-failed', 'save-failed', 'capture-error']); // A proxy timeout or bad gateway cannot tell whether the server committed the write. @@ -269,6 +273,7 @@ function derive(state) { canSave: editable, canGenerate: editable && state.generateSupported, canRetry: RETRYABLE.has(state.phase), + blocked: BLOCKED.has(state.phase), }; } @@ -291,7 +296,7 @@ function validatePreset(preset) { */ export function createImageTaggingController({api, target, source = 'images', onLateSave = () => {}, preset = null} = {}) { requireValue(object(api) && ['get', 'post', 'put', 'del'].every(name => typeof api[name] === 'function'), 'An api client is required'); - const imageTarget = validateTarget(target); + let imageTarget = validateTarget(target); requireValue(SOURCES.has(source), 'Unknown tagging source'); requireValue(typeof onLateSave === 'function', 'onLateSave must be a function'); const chosen = validatePreset(preset); @@ -364,6 +369,10 @@ export function createImageTaggingController({api, target, source = 'images', on } } catch (error) { if (closed || serial !== openSerial) return; + if (isApiError(error) && error.code === 'source-changed') { + set({phase: 'source-changed', statusText: error.message}); + return; + } set({phase: 'observe-error', statusText: messageOf(error, COPY.observeFailed)}); return; } @@ -531,6 +540,16 @@ export function createImageTaggingController({api, target, source = 'images', on return send(lastBody); } + /** + * #430: the page found the file under a fresh `target` (its sources were renumbered): observe it there, keeping the + * typed draft. Refused (false) while a save is in flight or after close; a Generate for the old target is dropped. + */ + function retarget(next) { + if (closed || saving) return Promise.resolve(false); + imageTarget = validateTarget(next); + return open({keepDraft: observedOnce || Boolean(state.manual)}).then(() => true); + } + function cancel() { if (closed) return; set({closed: true}); @@ -556,7 +575,8 @@ export function createImageTaggingController({api, target, source = 'images', on return { get state() {return state;}, - subscribe, open, setManual, setModel, setOption, generate, remove, save, retry, cancel, dispose, + get target() {return imageTarget;}, + subscribe, open, setManual, setModel, setOption, generate, remove, save, retry, retarget, cancel, dispose, }; } diff --git a/web/js/workspace/image-tagging.mjs b/web/js/workspace/image-tagging.mjs index 52d31c20..d02699be 100644 --- a/web/js/workspace/image-tagging.mjs +++ b/web/js/workspace/image-tagging.mjs @@ -24,6 +24,9 @@ import { showWorkspaceToast } from './toast.mjs'; import { cardTitle } from './images-state.mjs'; const MANUAL_HELP = 'Comma-separated. Generated suggestions are added only after you approve them.'; +// #430: while the page looks for a renumbered source's file, and the preview that a refused capture leaves. +const FINDING_SOURCE = 'The source of this image changed. Finding it in the refreshed image collection…'; +const PREVIEW_UNAVAILABLE = 'Preview unavailable'; const SCOPE_NOTE = 'Tags belong to this image. Resolve its prompt association separately in Needs review.'; // The option inputs per controller key; the classifier keys only exist for WD14 models. const OPTION_IDS = Object.freeze({ @@ -194,6 +197,13 @@ function previewOf(image, state) { if (video) { node.muted = true; node.preload = 'metadata'; node.playsInline = true; } return node; } +/** D147: the preview box a failed or refused capture leaves, saying so (the reason is beside the actions). */ +function unavailablePreview() { + const node = el('div', PREVIEW_UNAVAILABLE, 'tag-image-preview tag-preview-empty'); + node.setAttribute('role', 'img'); + node.setAttribute('aria-label', PREVIEW_UNAVAILABLE); + return node; +} /** * Open "Tag image" for one listed `image` (its tagging target). `source` is the page (`images`, `library`, `review`, @@ -207,7 +217,8 @@ function previewOf(image, state) { * Escape and × write nothing and call `onStop` once. Swapping the next image's dialog in is not a stop. */ export function openImageTagging({api, scope, image, source = 'images', trigger = null, onSaved = null, preset = null, - autoGenerate = false, actions: mode = 'tag', position = null, onSkip = null, onStop = null}) { + autoGenerate = false, actions: mode = 'tag', position = null, onSkip = null, onStop = null, onSourceChanged = null, + onRefresh = null}) { const target = imageTarget(image); if (!target) throw new TypeError('The image has no tagging target'); const review = mode === 'review'; @@ -216,7 +227,8 @@ export function openImageTagging({api, scope, image, source = 'images', trigger // Set once this dialog hands over to the session (Approve, Skip): its close is then not a Stop. let advancing = false; const saved = ({filename: name, tags, toast}) => { - const event = {target, filename: name, tags, source}; + // A retargeted dialog (#430) reports the file under its current identity. + const event = {target: {...controller.target}, filename: name, tags, source}; announceSaved(event); if (toast) showWorkspaceToast(toast); onSaved?.(event); @@ -280,6 +292,15 @@ export function openImageTagging({api, scope, image, source = 'images', trigger : (last ? 'Save and finish' : 'Save and next')); if (review) actions.append(cancel, skip, primary); else actions.append(cancel, primary); + // D147: the reason a state blocks Save (and Generate), on its own row above the buttons; for a changed source the + // page is asked to find the file again (`onSourceChanged`), else Refresh is offered. + const notice = el('div', undefined, 'tag-action-notice'); + const noticeText = el('span'); + noticeText.setAttribute('role', 'status'); + const refresh = createButton(onRefresh ? 'Refresh image collection' : 'Reload page'); + notice.append(noticeText); + const tried = new Set(); + let finding = false, current = image; const nodes = [heading]; if (controller.state.showScopeNote) nodes.push(el('p', SCOPE_NOTE, 'tag-scope-note small')); @@ -334,8 +355,45 @@ export function openImageTagging({api, scope, image, source = 'images', trigger closeWorkspaceDialog(); saved({filename: state.image.filename, tags: state.savedTags.slice(), toast: state.toast}); } + /** #430: ask the page once per target where a renumbered source's file is now; a found file is retargeted. */ + function findSource() { + const {root_index: index, root_scope: held} = controller.target; + const attempt = `${index}:${held}`; + if (!onSourceChanged || tried.has(attempt)) return; + tried.add(attempt); + finding = true; + Promise.resolve().then(() => onSourceChanged({...current, ...controller.target})).catch(() => null).then(fresh => { + finding = false; + if (!open || completed) return; + const next = imageTarget(fresh); + if (next && controller.state.phase === 'source-changed') { + current = fresh; + controller.retarget(next); + } else render(controller.state); + }); + } + function renderNotice(state) { + const changed = state.phase === 'source-changed'; + const text = !state.blocked ? '' : changed && finding ? FINDING_SOURCE : state.statusText; + setText(noticeText, text); + const offer = changed && !finding; + if (offer !== refresh.isConnected) { + if (offer) notice.append(refresh); + else refresh.remove(); + } + // Its own first row; Retry still leads the buttons after it (D116). + if (text && actions.firstElementChild !== notice) actions.prepend(notice); + else if (!text && notice.isConnected) notice.remove(); + // The empty preview of a refused or failed capture says why it is empty. + if (state.blocked && !state.image && preview.tagName !== 'DIV') { + const next = unavailablePreview(); + preview.replaceWith(next); + preview = next; + } + } function render(state) { if (!open || completed) return; + if (state.phase === 'source-changed' && !finding) findSource(); if (state.image && state.image !== shownImage) renderImage(state.image); // The observation's saved tags, a kept draft, or a conflict's current set ahead of the draft (I2). if (state.draftRevision !== shownDraft) { @@ -344,7 +402,8 @@ export function openImageTagging({api, scope, image, source = 'images', trigger } if (state.suggestions !== shownRows || state.suggestionsText !== shownText) renderSuggestions(state); setText(count, state.countText); - setText(status, state.statusText); + // A blocking reason is shown once, beside the actions (D147). + setText(status, state.blocked ? '' : state.statusText); setText(primary, review ? reviewLabel(state) : state.primaryLabel); setDisabled(primary, !state.canSave); // D90: while Approve's save is in flight the session cannot be left, so its count includes that save. @@ -356,10 +415,16 @@ export function openImageTagging({api, scope, image, source = 'images', trigger retry.remove(); } } + renderNotice(state); if (state.phase === 'saved') complete(state); } cancel.addEventListener('click', () => { if (!disabled(cancel)) closeWorkspaceDialog(); }); + refresh.addEventListener('click', () => { + closeWorkspaceDialog(); + if (onRefresh) onRefresh(); + else window.location.reload(); + }); skip.addEventListener('click', () => { if (!open || advancing || disabled(skip)) return; advancing = true; diff --git a/web/js/workspace/images-source-recovery.mjs b/web/js/workspace/images-source-recovery.mjs new file mode 100644 index 00000000..b8f686ce --- /dev/null +++ b/web/js/workspace/images-source-recovery.mjs @@ -0,0 +1,96 @@ +/** + * Finding a file again after its source changed (#430). + * + * A listed file is addressed as `{root_index, root_scope, path}`. When the sources are renumbered every per-file + * action on it answers 409 `source-changed`. `rescopeImage` asks the server (POST /prompt_manager/images/rescope) + * for the file's current listing descriptor: the server maps the target only when its scope was issued for one of + * the current roots, so a removed or replaced source (or a changed key) answers null and the page offers Refresh. + * + * `createSourceRecovery` is the page's single flight: one rescope per held `(key, scope)` pair, its result shared + * by every caller (the inspector, the viewer and the Tag image dialog may all ask for the same file), and a pair + * already tried answers its first result, so a refusal can never loop. `apply(image, fresh, started)` is the page's + * part (refresh the collection, move the selection); `started` is the page generation when the rescope began, so a + * page whose filters changed meanwhile can leave itself alone. `reset()` (a manual Refresh) forgets every pair. + */ +import { imageKey } from './images-state.mjs'; + +export const RESCOPE_URL = '/prompt_manager/images/rescope'; +// A page holds a handful of stale files at once; older pairs are forgotten first. +const MEMO_LIMIT = 64; +const hex64 = value => typeof value === 'string' && /^[0-9a-f]{64}$/.test(value); + +function validTarget(value) { + return value !== null && typeof value === 'object' && Number.isSafeInteger(value.root_index) && value.root_index >= 0 && + hex64(value.root_scope) && typeof value.path === 'string' && value.path.length > 0; +} + +/** The fresh descriptor of `image` (same path, current root and scope), else null. */ +export async function rescopeImage(api, image, {signal} = {}) { + if (!validTarget(image)) return null; + let value; + try { + value = await api.post(RESCOPE_URL, {target: {root_index: image.root_index, root_scope: image.root_scope, path: image.path}}, + {signal}); + } catch (error) { + // 409 (the source is gone) or 404 (the file is gone): final, the caller offers Refresh. Anything else (no + // answer, a 5xx) is rethrown so the pair may be asked again. + if (error?.status === 404 || error?.status === 409) return null; + throw error; + } + const fresh = value?.success === true ? value.image : null; + if (!validTarget(fresh) || fresh.path !== image.path) return null; + return Object.freeze({...fresh}); +} + +export function createSourceRecovery({api, signal, apply, generation = () => 0}) { + // An embedded gallery's own page adapter may have no `post`: its rescope then simply finds nothing. + if (typeof apply !== 'function') throw new TypeError('createSourceRecovery needs apply'); + const memo = new Map(); + function recover(image) { + if (!validTarget(image)) return Promise.resolve(null); + const pair = `${imageKey(image)}|${image.root_scope}`; + if (memo.has(pair)) return memo.get(pair); + const started = generation(); + const pending = rescopeImage(api, image, {signal}) + .then(fresh => (fresh && !signal?.aborted ? apply(image, fresh, started) : null)) + .then(result => result ?? null, () => { + if (memo.get(pair) === pending) memo.delete(pair); + return null; + }); + memo.set(pair, pending); + if (memo.size > MEMO_LIMIT) memo.delete(memo.keys().next().value); + return pending; + } + return {recover, reset: () => memo.clear()}; +} + +/** + * The Images page's `apply` (#430). Keys are `root_index:path`, so after a renumbering nothing from the old list is + * kept by key (a checked or selected key could name a same-named file in another source): the viewer closes, the + * checks clear, a Source filter on the old index follows the file, and only the file itself stays selected while the + * collection lists again. An unchanged numbering re-reads the loaded range in place. `page` holds the page's hooks. + */ +export function recoveredSource(page) { + return async (image, fresh, started) => { + // The held scope still names its root: nothing is stale (a media error had another cause). + if (fresh.root_index === image.root_index && fresh.root_scope === image.root_scope) return null; + if (!page.current(started)) return fresh; + const selected = page.selected(image); + if (fresh.root_index === image.root_index) { + if (selected) page.inspect(fresh); + await page.stream.refresh().catch(() => {}); + return fresh; + } + page.closeViewer(); + page.clearChecks(); + const {filters} = page; + if (filters.root === String(image.root_index)) filters.root = String(fresh.root_index); + else if (filters.root !== 'all') { filters.root = 'all'; filters.subfolder = ''; } + if (selected) page.inspect(fresh); + else page.dropSelection(); + page.updateScope(); + page.stream.reset(filters, {load:page.listsNow()}); + page.rediscover(); + return fresh; + }; +} diff --git a/web/js/workspace/images-tagging.mjs b/web/js/workspace/images-tagging.mjs index dc3f0f91..2e5f36ba 100644 --- a/web/js/workspace/images-tagging.mjs +++ b/web/js/workspace/images-tagging.mjs @@ -14,9 +14,9 @@ export const REFRESH_FAILED = 'Images could not be refreshed after the tag save. /** Open Tag image for `image` when `action` is its card's Tag button (without selecting the card, as the concept's * `image-tag` does); false for any other control. */ -export function openCardTagging({api, scope, image, action, embedded}) { +export function openCardTagging({api, scope, image, action, embedded, onSourceChanged = null, onRefresh = null}) { if (!isTagButton(action)) return false; - openImageTagging({api, scope, image, source: embedded ? 'library' : 'images', trigger: action}); + openImageTagging({api, scope, image, source: embedded ? 'library' : 'images', trigger: action, onSourceChanged, onRefresh}); return true; } diff --git a/web/js/workspace/images.mjs b/web/js/workspace/images.mjs index 469671e0..f35e42a3 100644 --- a/web/js/workspace/images.mjs +++ b/web/js/workspace/images.mjs @@ -22,6 +22,7 @@ import { displayPreferences } from './preferences.mjs'; import { onImageTagsSaved } from './image-tagging.mjs'; import { followImageTagSaves, openCardTagging } from './images-tagging.mjs'; import { favoriteBusy, paintStar, receiveFavorites, setFavoriteVisitTarget, toggleFavorite } from './images-favorites.mjs'; +import { createSourceRecovery, recoveredSource } from './images-source-recovery.mjs'; export { onFavoriteChange } from './images-favorites.mjs'; /** The concept's panel without an image (app.js updateImages a456–458). */ @@ -68,6 +69,7 @@ const RETRY_RESCAN = 'Rescan from Import to retry.'; // The concept's empty collection (app.js updateImages). const NO_MATCHES = 'No matching images. Try another search or category.'; const RETRY_REFRESH = 'Refresh image collection to retry.'; +const CURSOR_STALE = 'The image collection changed. Refresh image collection to continue.'; /** One alert for whichever of the source discovery and the categories failed, so two failures announce once. */ function loadFailure(sourcesRetry, categoriesFailed) { if (sourcesRetry && categoriesFailed) return `Sources, folders and categories could not be loaded. ${sourcesRetry}`; @@ -128,7 +130,14 @@ export function mountImages(main, api, parentScope, {embedded = false, fetchPage // System › Appearance: the display defaults this visit starts from (preferences.mjs); null without gallery-settings.js. const preferences = displayPreferences(); const inspection = createInspectionController({api, autoLoad:preferences?.autoLoadMetadata !== false, cache:preferences?.cacheMetadata !== false}); - scope.cleanup(inspection.subscribe(next => detailInspector?.setState(next))); + // The panel's listed file: a changed source is looked up once, the panel's Refresh stays the fallback (#430). + let selectedImage = null; + scope.cleanup(inspection.subscribe(next => { + detailInspector?.setState(next); + if (next.refreshRequired && next.code === 'source-changed' && selectedImage && imageKey(selectedImage) === selection) { + recovery.recover(selectedImage); + } + })); scope.cleanup(() => inspection.dispose()); scope.cleanup(() => detailInspector?.dispose()); // A Tags & categories Browse (browseImages) is handed to this mount once: a fresh Images visit filtered by its @@ -298,7 +307,7 @@ export function mountImages(main, api, parentScope, {embedded = false, fetchPage main.append(body); const mediaViewer = createMediaViewer({api,container:main,parentScope:scope,inspectionController:inspection,sender, onSelect:image => {if (selection !== imageKey(image)) inspect(image);else inspection.select(image);}, - onRefresh:refreshInspectionCollection,onClose:scheduleLoad,onRestoreFocus:image => { + onRefresh:refreshInspectionCollection,onClose:scheduleLoad,onMediaError:image => recovery.recover(image),onRestoreFocus:image => { const index = state?.items.findIndex(entry => imageKey(entry) === imageKey(image)); if (index >= 0) focusImage(index);else focusFallback.focus({preventScroll:true}); }}); @@ -310,6 +319,7 @@ export function mountImages(main, api, parentScope, {embedded = false, fetchPage function inspect(image) { detailInspector?.dispose(); selection = imageKey(image); + selectedImage = image; for (const video of details.querySelectorAll('video')) releaseVideo(video); detailInspector = mountFileInspector(details, {sender, parentScope:scope, variant:'panel', onTagFilter:embedded ? undefined : filterByTag, onRetry:() => inspection.retry(), onLoad:() => inspection.load(), onRefresh:refreshInspectionCollection}); @@ -350,7 +360,8 @@ export function mountImages(main, api, parentScope, {embedded = false, fetchPage const action = event.target.closest('.image-card-actions button'); if (event.target.closest('.star-button')) toggleFavorite(api, image); else if (event.target.closest('.image-open')) previewImage(image); - else if (action && !openCardTagging({api, scope, image, action, embedded})) inspect(image); + else if (action && !openCardTagging({api, scope, image, action, embedded, onSourceChanged:recovery.recover, + onRefresh:refreshInspectionCollection})) inspect(image); }); function renderVisible() { @@ -498,6 +509,7 @@ export function mountImages(main, api, parentScope, {embedded = false, fetchPage function continuationText(next) { if (recovering) return 'Images paused until sources and folders are available.'; if (collectionGone(next)) return ''; + if (next.error?.code === 'cursor-stale') return CURSOR_STALE; if (next.error) return 'Images could not be loaded. Retry or rescan the collection.'; if (next.loading && !next.loaded) return 'Loading images…'; if (next.hasMore) return `${next.loaded.toLocaleString('en-US')} of ${next.total.toLocaleString('en-US')} ${noun(next.total, 'image')} · more load as you scroll`; @@ -535,6 +547,8 @@ export function mountImages(main, api, parentScope, {embedded = false, fetchPage setText(message, continuationText(next)); setAttr(message, 'role', next.error && !gone ? 'alert' : 'status'); retry.hidden = !next.error || gone; + // A cursor the server can no longer verify is never resent: Retry becomes the collection refresh (#430). + setText(retry, next.error?.code === 'cursor-stale' ? 'Refresh image collection' : 'Retry'); more.hidden = !next.hasMore || !!next.error; more.setAttribute('aria-disabled', String(next.loading || recovering)); if (onCollectionMissing && filters.collection && isCollectionGone(next.error) && reportedMissing !== next.error) { @@ -579,7 +593,7 @@ export function mountImages(main, api, parentScope, {embedded = false, fetchPage } } scope.listen(more, 'click', pressLoad); - scope.listen(retry, 'click', pressLoad); + scope.listen(retry, 'click', () => (state?.error?.code === 'cursor-stale' ? refreshInspectionCollection() : pressLoad())); function maybeLoad() { if (!observer || recovering || scope.signal.aborted || !state?.hasMore || state.loading || state.error || main.querySelector('#viewer-dialog[open]')) return; const bounds = continuation.getBoundingClientRect(); @@ -607,8 +621,18 @@ export function mountImages(main, api, parentScope, {embedded = false, fetchPage details.replaceChildren(noSelection()); } selection = null; + selectedImage = null; } + // Bumped by every reset: a source recovery that began before it leaves the page alone (#430). + let generation = 0; + const recovery = createSourceRecovery({api, signal:scope.signal, generation:() => generation, apply:recoveredSource({ + selected:image => selection === imageKey(image), inspect, dropSelection, stream, filters, updateScope, + closeViewer:() => { mediaViewer.stopSlideshow(); main.querySelector('#viewer-dialog[open]')?.close(); }, + clearChecks:() => bar?.clear(), listsNow:() => !recovering, + current:started => !scope.signal.aborted && started === generation, + rediscover:() => loadOptions().then(found => { if (found) renderLoadStatus(); })})}); function reset() { + generation += 1; mediaViewer.stopSlideshow(); main.querySelector('#viewer-dialog[open]')?.close(); dropSelection({keepPanel:recovering}); @@ -712,6 +736,7 @@ export function mountImages(main, api, parentScope, {embedded = false, fetchPage } async function refreshInspectionCollection() { if (scope.signal.aborted) return; + recovery.reset(); // Numeric roots may now name another source; retire the old stream first. recovering = true; filters.root = 'all';filters.subfolder = ''; diff --git a/web/js/workspace/index.mjs b/web/js/workspace/index.mjs index 018d181f..81d1b132 100644 --- a/web/js/workspace/index.mjs +++ b/web/js/workspace/index.mjs @@ -9,7 +9,7 @@ import { mountVocabulary } from './vocabulary.mjs'; import { createWorkspaceIcon } from './icons.mjs'; import { bindCanvasLink, canvasHref } from './topbar.mjs'; import { renderHeading, pageHeading } from './heading.mjs'; -import { closeWorkspaceDialog } from './workspace-dialogs.mjs'; +import { closeReviewDialog, closeWorkspaceDialog } from './workspace-dialogs.mjs'; import { createSavedViews, openSaveViewDialog, openEditViewDialog, savedViewHash } from './saved-views.mjs'; import { createCollections, collectionHash, openEditCollectionDialog, missingNote, DELETED_PAGE_TEXT, OPEN_IMAGES_LINK, LIST_FAILED_TEXT } from './collections.mjs'; @@ -20,7 +20,7 @@ import { mountInspect } from './inspect.mjs'; import { mountReview } from './review.mjs'; import { createPromptSender } from './prompt-sender.mjs'; import { mountSystem } from './system.mjs'; -import { onRestoreFinished, onRestoreHandedBack, showRestoreReview } from './system-backups.mjs'; +import { createRestoreReload, onRestoreFinished, onRestoreHandedBack, showRestoreReview } from './system-backups.mjs'; import { mountLibraryBanner } from './library-location.mjs'; import { detectJxlSupport } from './thumbnail-support.mjs'; @@ -184,13 +184,20 @@ function mountWorkspace() { refreshCounts(NAV_COUNT_ROUTES); // S9: a finished restore replaced the library. The cached visits are dropped, the counts and saved views read // again, and the open page is shown again from the restored data; the Activity job view the restore opened stays. - scope.cleanup(onRestoreFinished(activity, () => { + // #415: an open review closes (its plan no longer describes storage); while any other dialog is open, all of it + // waits until the last one has closed, so the dialog keeps what was typed. + const restoreReload = createRestoreReload(() => { forgetImagesVisit(); forgetLibraryVisit(); refreshCounts(NAV_COUNT_ROUTES); savedViews.load(); collections.load(); if (currentKey !== 'activity') navigateTo(window.location.hash); + }); + scope.cleanup(() => restoreReload.dispose()); + scope.cleanup(onRestoreFinished(activity, () => { + closeReviewDialog(); + restoreReload.request(); })); // S9 I1: a restore this page started that could not run yet (other requests kept reading) hands its review back: // it opens again over the page shown now, with Restore (again) and Discard; leaving it any other way discards it. diff --git a/web/js/workspace/inspection-state.mjs b/web/js/workspace/inspection-state.mjs index 44639bce..ef9050e8 100644 --- a/web/js/workspace/inspection-state.mjs +++ b/web/js/workspace/inspection-state.mjs @@ -21,6 +21,7 @@ export class InspectionSourceChangedError extends Error { super(SOURCE_CHANGED); this.name = 'InspectionSourceChangedError'; this.refreshRequired = true; + this.code = 'source-changed'; } } diff --git a/web/js/workspace/local-resolver.mjs b/web/js/workspace/local-resolver.mjs index b246adaa..435b1a2a 100644 --- a/web/js/workspace/local-resolver.mjs +++ b/web/js/workspace/local-resolver.mjs @@ -4,7 +4,7 @@ // tests/test_local_inspect_parity.py compares every answer with the server's on the same records. Values follow // Python's model (py-json.mjs): dict order, int versus float, code-point lengths, `str.strip()`. // -// Task 8 ported execution evidence and graph tracing for one sampler; Task 9 adds candidate selection among several +// Task 8 ported execution evidence and graph tracing for one sampler; Task 9 added candidate selection among several // samplers and detailer stages (local-resolver-stages.mjs) and A1111 `parameters` (local-parameters.mjs). No DOM. import { PyValueError, isDict, isList, isInt, pyJsonLoads, pyLen, pySlice, pyStrip, pyStr, pyGet } from './py-json.mjs'; import { diff --git a/web/js/workspace/media-viewer.mjs b/web/js/workspace/media-viewer.mjs index 834c9957..106932af 100644 --- a/web/js/workspace/media-viewer.mjs +++ b/web/js/workspace/media-viewer.mjs @@ -19,7 +19,7 @@ function element(tag, className, text) { return node; } -export function createMediaViewer({api,container,parentScope,inspectionController,sender,onSelect,onRefresh,onRestoreFocus,onClose,adapter = rootedFileAdapter}) { +export function createMediaViewer({api,container,parentScope,inspectionController,sender,onSelect,onRefresh,onRestoreFocus,onClose,onMediaError,adapter = rootedFileAdapter}) { const scope = createScope(); let active = null, retryVersion = 0; const dispose = () => scope.dispose(); @@ -148,7 +148,8 @@ export function createMediaViewer({api,container,parentScope,inspectionControlle src = url.href; } const node = element(selected.is_video ? 'video' : 'img'); - mediaLocal.listen(node,'error',() => {if (current()) session.failed(key);}); + // #430: the page may find the file under a renumbered source (`onMediaError`). + mediaLocal.listen(node,'error',() => {if (current()) {session.failed(key);onMediaError?.(selected);}}); if (selected.is_video) { node.controls = true;node.playsInline = true;node.preload = 'metadata'; const preferences = readViewerPreferences(); diff --git a/web/js/workspace/system-backups.mjs b/web/js/workspace/system-backups.mjs index 8d5745dc..a2893624 100644 --- a/web/js/workspace/system-backups.mjs +++ b/web/js/workspace/system-backups.mjs @@ -326,6 +326,43 @@ export function onRestoreFinished(activity, onFinished) { }); } +const OPEN_DIALOG = 'dialog[open]'; +const DIALOG_MUTATIONS = Object.freeze({ subtree: true, childList: true, attributes: true, attributeFilter: ['open'] }); + +/** + * #415: the S9 reload after a finished restore never runs under an open dialog, which it would close with whatever + * was typed in it. `request()` runs `reload()` at once when no dialog is open; otherwise it runs once the last open + * dialog has closed (repeated requests meanwhile run it once). The dialog keeps its entries, and its next write is + * refused by the restored storage with its own note and Reload, as when this page never saw the restore. A + * MutationObserver sees every dialog close: removing `open` and removing the node are synchronous, while the dialog + * `close` event is a later task that never bubbles, and every dialog here is removed in the task that closes it. + * `dispose()` drops a waiting reload. Returns `{request, dispose}`. + */ +export function createRestoreReload(reload, { doc = globalThis.document, Observer = globalThis.MutationObserver } = {}) { + let observer = null; + const dialogOpen = () => doc.querySelector(OPEN_DIALOG) !== null; + const stop = () => { + observer?.disconnect(); + observer = null; + }; + return { + request() { + if (observer) return; + if (!dialogOpen()) { + reload(); + return; + } + observer = new Observer(() => { + if (dialogOpen()) return; + stop(); + reload(); + }); + observer.observe(doc.documentElement, DIALOG_MUTATIONS); + }, + dispose: stop, + }; +} + function surface(heading, ...nodes) { const section = el('section', undefined, 'surface'); section.append(heading, ...nodes); diff --git a/web/js/workspace/workspace-dialogs.mjs b/web/js/workspace/workspace-dialogs.mjs index fff32920..0d6d4643 100644 --- a/web/js/workspace/workspace-dialogs.mjs +++ b/web/js/workspace/workspace-dialogs.mjs @@ -112,13 +112,13 @@ function bindScope(scope) { }); } -function show(heading, nodes, { scope, trigger, focusTarget, onClose = null, canClose = null }) { +function show(heading, nodes, { scope, trigger, focusTarget, onClose = null, canClose = null, review = false }) { shell ??= createShell(); if (!shell.dialog.isConnected) document.body.append(shell.dialog); bindScope(scope); // A swapped-out body is gone from the shell, so its owner releases it. const replaced = session?.onClose; - session = { scope, trigger: trigger ?? session?.trigger ?? null, onClose, canClose }; + session = { scope, trigger: trigger ?? session?.trigger ?? null, onClose, canClose, review }; replaced?.(); shell.title.textContent = heading; shell.body.replaceChildren(...nodes); @@ -133,6 +133,14 @@ export function closeWorkspaceDialog() { shell = null; } +/** + * #415: a finished restore ends an open record-changing review (`openReviewDialog`) as a dismissal, since the plan it + * reviewed no longer describes storage and it holds nothing typed. Any other dialog stays open. + */ +export function closeReviewDialog() { + if (session?.review) closeWorkspaceDialog(); +} + function fillSelect(select, key, options) { if (!Array.isArray(options) || options.length === 0 || options.some(pair => !Array.isArray(pair) || typeof pair[0] !== 'string' || typeof pair[1] !== 'string')) { @@ -497,7 +505,7 @@ export function openReviewDialog({ scope, title, heading = null, grid, options = actions.append(cancel, start); nodes.push(actions); // Cancel, ×, Escape, a replacing dialog or the page leaving: everything but the primary is a dismissal. - show(title, nodes, { scope, trigger, onClose: onDismiss ? () => { if (!started) onDismiss(); } : null }); + show(title, nodes, { scope, trigger, onClose: onDismiss ? () => { if (!started) onDismiss(); } : null, review: true }); return { setValue(term, text) { const dd = values.get(term); diff --git a/web/js/workspace/workspace.css b/web/js/workspace/workspace.css index 7fbe8ffe..db268fe2 100644 --- a/web/js/workspace/workspace.css +++ b/web/js/workspace/workspace.css @@ -456,3 +456,9 @@ pre.prompt-text { font: inherit; font-size: 12px; line-height: 1.75; margin-bloc /* #332 D146: the category chip names its origin (" · this image", " · prompt", " · recipe") in muted text that never splits; at 390px the chip may wrap between the category and its origin. */ .chip .chip-source { color: var(--ca-muted); white-space: nowrap; } +/* D147 (#430): a state that blocks Save shows its reason on its own row above the sticky actions, with Refresh for a + changed source; the disabled actions look disabled; a refused capture's empty preview says so. */ +.tagging-modal .dialog-actions [aria-disabled="true"], .tagging-modal .tag-model-panel [aria-disabled="true"] { opacity: .55; cursor: not-allowed; /* D147 */ } +.tagging-modal .tag-action-notice { flex: 1 0 100%; display: flex; flex-wrap: wrap; align-items: center; justify-content: space-between; gap: 8px; margin: 0; color: var(--ca-text); font-size: 12px; /* D147 */ } +.tagging-modal .tag-action-notice > span { flex: 1 1 16em; } +.tag-image-preview.tag-preview-empty { display: grid; place-items: center; padding: 8px; text-align: center; font-size: 12px; color: var(--ca-muted); background: var(--ca-field); box-sizing: border-box; /* D147 */ }