diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index bc7321fa..43bf3d99 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -221,7 +221,10 @@ jobs: - uses: actions/setup-python@v7 with: python-version: "3.12" - - run: pip install black flake8 + cache: pip + cache-dependency-path: requirements-dev.txt + # The exact black and flake8 pins from requirements-dev.txt, so a new release cannot turn CI red. + - run: pip install $(grep -E '^(black|flake8)==' requirements-dev.txt) - run: black --check . - run: flake8 . @@ -235,6 +238,8 @@ jobs: - uses: actions/setup-node@v7 with: node-version: "24" + cache: npm + cache-dependency-path: tools/vendor/package-lock.json - name: Check shipped extension and page modules parse run: | diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index ca3295df..9106e4fc 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -13,9 +13,11 @@ repos: language: system types: [python] + # Fails at the severity CI enforces (code-quality.yml: -lll, high only), never stricter. + # pre-commit appends the staged files, so no shell wrapper can drop the first one. - id: bandit name: bandit - entry: bash -c 'bandit "$@" -ll -q 2>&1; true' + entry: bandit -lll -q language: system types: [python] exclude: ^(tests/|reference/) diff --git a/TESTING.md b/TESTING.md index ce4bd6b2..67df06a6 100644 --- a/TESTING.md +++ b/TESTING.md @@ -3200,12 +3200,16 @@ the per-section hand-off panel, was retired by S12 (#329): every section is nati again"; an Unload that found nothing loaded only reads the status again. At phone width the concept's 120px select cap cuts every engine label, so `#tag-model` alone keeps its natural width there and its row wraps the select under the text (`workspace.css` - `/* D132 */`); Existing tags stays as the concept draws it, cut at 390 as there. The - comparison maps `selectsFit` only for the engine select and measures each row's - `flexWrap` and whether its control sits under the text. Pinned by `auto-tagging matches - the concept` (both themes and sizes), `auto-tagging at 390 wraps only the engine row`, - `auto-tagging engine select fits at 320 wide` and the mutation check (it includes a - wrapped row) (system-tagging-concept.spec), and the model, Download, Unload and Preview + `/* D132 */`). The concept's cap cuts Existing tags too ("Replace after review"); by the + maintainer's decision (#398) that parity is not kept: `#tag-existing` never shrinks for + its row text and at phone width keeps its natural width beside the text (its row does + not wrap; the text column narrows). 1600 is as the concept draws it. The comparison + expects `selectsFit` true for every select and measures each row's `flexWrap` and + whether its control sits under the text. Pinned by `auto-tagging matches the concept` + (both themes and sizes), `auto-tagging at 390 wraps only the engine row`, `auto-tagging + selects fit every option at 390/320 wide` (every option of both selects, both themes, no + page scroll) and the mutation check (a wrapped row, a squeezed engine and Existing tags + select) (system-tagging-concept.spec), and the model, Download, Unload and Preview tests of workspace-system-tagging.spec. - **D133** (Auto-tagging, S11): a "Suggestions per image" row (`#tag-suggestions`, number 1–100, host key `autotag_suggestion_limit`) follows the two confidence rows: @@ -3979,7 +3983,30 @@ modals use the same routes and keep working. other: each file's identity is checked just before its unlink. - **Clear all logs** (`POST /logs/truncate`, also the admin page's Clear) empties the active log through its handler, deletes the rotated copies (Rule R) and clears the - buffer; other files and links are kept. Response keys are unchanged. + buffer; other files and links are kept. Response keys are unchanged. It deletes or + empties no more than its confirmation showed (#389): + - `/logs/files` rows carry `identity`, the string `"::"` (a + string: `mtime_ns` and many inodes are beyond a browser's exact integers). The + workspace sends `{"expected": [{name, identity}]}` with the active and rotated rows + its dialog was built from. Under the files lock and every handler lock each listed + rotated copy must still match (`_matches_review`) and the listed active log must be + the same inode, or nothing changes: 409 `logs-changed`. Only listed copies are + deleted; one that appeared after the confirmation is kept. A malformed list is 400. + - Without a body (the legacy admin page, whose confirm lists nothing; also the + workspace when the file list could not be read and the dialog said "deletes its + rotated copies") every rotated copy present at the locked rescan is deleted. + - Either way, before anything changes (best effort, not a cross-process lock: a + write before our own last write, or any while file logging is off here, is not + seen): a second writer of the active log (our + handler's flushed stream is behind the file's size, or its file is now a rotated + copy: another ComfyUI sharing the folder) is 409 `logs-foreign-writer`; an active + log with more than one name (`st_nlink > 1` by `lstat` or the handler's `fstat`, + NTFS included) is 409 `logs-hard-link`, never truncated (replacing it instead would + rename a file our handler holds open, which Windows refuses). Each copy is + `lstat`-checked again just before its unlink; one that changed stops the clear there + and keeps it, the copies after it and the active log, which is emptied last. + - The browser fixture grows the active log through the handler's own stream, since a + second handle appending to it is exactly what Clear refuses. - **Runtime settings** (`GET/POST /logs/config`): the level (DEBUG…CRITICAL) and console logging apply until the server restarts; the retention values are shown read-only in the workspace and the API keeps accepting them. @@ -4099,6 +4126,33 @@ list stay unscoped: any row's control proves them. | API timeout `apiTimeout` | `gallery-settings.js:37` | Legacy gallery (S12-10; read by no request) | kept in legacy | — | defaults are a complete, sane settings object (gallery-settings.test.js) | | config.json-only keys | `py/config.py:212–234,722–739`; `py/api/diagnostics_report.py:185` | S12 report (read-only) | covered | `diagnostics` · button · "Download diagnostic report" | test_report_lists_config_only_keys (test_api_diagnostics_report.py) | +### Legacy System UI removed by S13 + +System S13 (#330) points the legacy pages' System buttons at native System (a link that +opens `workspace#/system/` in a new tab) and removes the legacy copy of a capability +only when its row above is `covered`. One row per removed capability: Capability is the +exact Capability cell of its coverage row, Page is `admin` or `gallery`, Removed ids and +Removed methods list what went (`—` for none), and Native test copies that row's Test +cell. An id that stays on a System link is not listed, only its handler. + +`tests/vendor/legacy-coverage.test.mjs` fails when a row cites a capability that is not +`covered` or copies another test, when an id or method is listed twice for one page, or +when a listed id is still on its page (`id="…"` in the markup or the script, a +`getElementById`/`querySelector('#…')` lookup, an entry of a modal-id list) or a listed +method is still defined or called (`.name(`). A deletion listed nowhere is caught by the +kept-flow tests (`kept admin flows still work` in `legacy-system-links.spec.mjs` and the +legacy page workflows), not by this check. + +| Capability | Page | Removed ids | Removed methods | Native test | +| --- | --- | --- | --- | --- | +| Diagnostics run | admin | `#diagnosticsModal`, `#runDiagnosticsBtn`, `#diagnosticsContent` | `showDiagnosticsModal()`, `closeDiagnostics()`, `runDiagnostics()`, `renderDiagnostics()` | Run checks opens the shared Health report (workspace-system-diagnostics.spec.mjs) | +| Synthetic image-link test | admin | `#testImageLinkBtn` | `testImageLink()` | Create test link confirms first and names the prompt (workspace-system-diagnostics.spec.mjs) | +| View logs with level and limit | admin | `#logsModal`, `#logLevel`, `#logLimit`, `#refreshLogsBtn`, `#logsContainer` | `showLogsModal()`, `closeLogs()`, `refreshLogs()`, `displayLogs()` | level filter is minimum-level over all six choices (workspace-system-diagnostics.spec.mjs) | +| Log statistics | admin | `#logStatsTotal`, `#logStatsErrors`, `#logStatsWarnings`, `#logStatsLevel`, `#logStatsSize` | `loadLogStats()`, `formatBytes()` | log statistics show entries, errors, warnings, files and size (workspace-system-diagnostics.spec.mjs) | +| Log files list and download | admin | `#downloadLogsBtn`, `#logFilesList` | `loadLogFiles()`, `downloadLogs()`, `downloadLogFile()` | each listed file downloads by its returned name and a link has no Download (workspace-system-diagnostics.spec.mjs) | +| Clear/truncate logs | admin | `#clearLogsBtn` | `clearLogs()` | Clear all logs states both counts, confirms first and reads everything again (workspace-system-diagnostics.spec.mjs) | +| Set log level (incl. CRITICAL), console logging | admin | `#updateLogConfigBtn`, `#setLogLevel`, `#consoleLogging` | `loadLogConfig()`, `updateLogConfig()` | runtime level (including CRITICAL) and console logging save and say they apply until the server restarts (workspace-system-diagnostics.spec.mjs) | + ### Browser cache ownership (#222) The legacy gallery's Clear cache used to delete every localStorage key starting `pm_` or @@ -4592,7 +4646,23 @@ files, oldest forgotten first, keyed by 16-byte digests: 8.6 MiB measured at the cap). A file whose inspection raises is logged, counted in `review_errors` and reported `unresolved`; the scan goes on. An older version's pending, undecided item of bytes now untraced leaves the queue -(`settle_stale_review_items`), so an item never stays stuck at 409. The 16-candidate limited +(`settle_stale_review_items`), so an item never stays stuck at 409. +Saver anchor (#390, `tests/test_review_saver_anchor.py`): the scan and Apply +resolve with the file's name as the workspace inspector does, so the one saver +whose `filename_prefix` names the file anchors its branch. A file is never both +unlinked and unqueued (maintainer ruling): when only that anchor resolves it +(the named inspection run resolves, the unnamed one does not) and neither the +default profile nor the parser floor links it, the scan queues it with the +anchor's pair as its one candidate (evidence `anchor: "saver"`; the page +preselects it), never linking it. Such an item counts as a choice for the +untraced settle. Files already linked or resolved by the default path are +unchanged. No match, several matches or an unfinished walk anchor nothing, and +the file is queued as before. `REVIEW_RULE_VERSION` is folded into inspection +item versions, so older inspection items are recorded again once. Apply accepts +the anchored pair and refuses a choice the anchor contradicts. Measured on the +maintainer's 40,890 outputs: the queue is unchanged (249 files); the 242 +`gpu_unlock_bench` files the inspector resolves are linked by the parser +floor. The 16-candidate limited rule and the 8 MiB bound apply unchanged. The queue heals itself: after each batch, the scan removes the pending items of every listed file (read or skipped as known) that a saved row now links, and the monitor does the same for each ambiguous output it links diff --git a/database/review_schema.py b/database/review_schema.py index 0aa8318b..c0c45d76 100644 --- a/database/review_schema.py +++ b/database/review_schema.py @@ -229,13 +229,15 @@ def evidence_offers_choice(evidence: Any) -> bool: """ if not isinstance(evidence, dict): return False + # The saver anchor's one pair (#390) is a suggestion to confirm. + needed = 1 if evidence.get("anchor") == "saver" else MIN_CHOOSABLE if evidence.get("limited"): total = evidence.get("candidate_total") - return type(total) is int and total >= MIN_CHOOSABLE + return type(total) is int and total >= needed candidates = evidence.get("candidates") if not isinstance(candidates, list): return False - return sum(1 for c in candidates if choosable(c)) >= MIN_CHOOSABLE + return sum(1 for c in candidates if choosable(c)) >= needed def evidence_digest(text: str) -> str: diff --git a/py/api/admin.py b/py/api/admin.py index 36a61988..013982ae 100644 --- a/py/api/admin.py +++ b/py/api/admin.py @@ -1637,7 +1637,14 @@ class AdminRoutesMixin: if self._review_file_linked(media_file, roots): outcome = "unchanged" # already linked: not a review case else: - self._record_scanned_review(media_file, roots, review) + # The records name the item versions that count; the + # saver anchor (#390) is checked on the captured bytes. + self._record_scanned_review( + media_file, + roots, + review, + scanned=(records.get("canonical"), None), + ) elif untraced and outcome == "unresolved": # Untraced by the default profile: the inspection profile may # still offer a choice (queued for review, never linked). @@ -1698,9 +1705,9 @@ class AdminRoutesMixin: ``"ambiguous"`` (nothing linked it) is a Needs review case. An ambiguous result with nothing to choose (``reviewable`` is False: fewer than two candidates carry a prompt) that nothing links returns - ``"unresolved"`` instead, and ``media_file`` goes to ``untraced``; - its parsed records go to ``records["canonical"]`` when ``records`` is - a dict. + ``"unresolved"`` instead, and ``media_file`` goes to ``untraced``. + The parsed records of either ambiguous file go to + ``records["canonical"]`` when ``records`` is a dict. """ if not metadata: # No prompt metadata (--disable-metadata, bare savers): only the @@ -1754,6 +1761,8 @@ class AdminRoutesMixin: records["canonical"] = canonical elif resolution["state"] == "ambiguous" and ambiguous is not None: ambiguous.append(media_file) + if records is not None: + records["canonical"] = canonical if resolution["state"] == "resolved": result = ingest_resolved_pair( self.db, str(media_file), canonical, resolution diff --git a/py/api/backup_restore.py b/py/api/backup_restore.py index edd60d14..a83ecbfa 100644 --- a/py/api/backup_restore.py +++ b/py/api/backup_restore.py @@ -483,13 +483,59 @@ class BackupRestoreRoutesMixin: self.logger.info(f"Removed {len(removed)} old safety backups") async def _publish(self, model, staged): - """Publish ``staged``; on a page-copy failure prune and re-raise.""" + """Publish ``staged``; on a page-copy failure prune and re-raise. + + A cancel (server stop) waits for the publishing thread (#360): the + caller's cleanup and its registry admission must outlive it. When the + copy then succeeded the library was replaced, so it is adopted + (``_after_publish``, itself run to its end) before the cancel goes + on; a copy that failed is handled as usual, then the cancel goes on. + """ + future = self._in_executor_task(model.publish_staged, staged) + cancelled = await _outlive_cancel(future) try: - return await self._run_in_executor(model.publish_staged, staged) + backup_path = future.result() except sqlite3.OperationalError as exc: if getattr(exc, "_prompt_manager_restore_backup_created", False) is True: await self._prune_after(model.db_path, failed=True) + if cancelled: + raise asyncio.CancelledError from exc raise + except Exception as exc: + if cancelled: + raise asyncio.CancelledError from exc + raise + if cancelled: + self.logger.info("The restore finished while the server was stopping") + # A second cancel (the loop's own shutdown) must not cut it short. + adopted = asyncio.ensure_future(self._after_publish(model.db_path)) + await _outlive_cancel(adopted) + if not adopted.cancelled() and adopted.exception() is not None: + self.logger.error( + "Could not adopt the restored library", + exc_info=adopted.exception(), + ) + raise asyncio.CancelledError + return backup_path + + async def _stage_to_end(self, model, temp_path): + """Stage ``temp_path``; a cancel waits for the worker (#360). + + The upload stays until the staging thread is done with it, and a + stage it finished after the cancel is removed before the cancel goes + on. + """ + future = self._in_executor_task(model.stage_restore, temp_path) + if await _outlive_cancel(future): + if not future.cancelled() and future.exception() is None: + await self._discard_held([_Orphan(future.result())]) + raise asyncio.CancelledError + return future.result() + + def _in_executor_task(self, func, *args): + """``_run_in_executor(func, *args)`` as a task ``_outlive_cancel`` can + wait for (shielded, the executor call is never cancelled).""" + return asyncio.ensure_future(self._run_in_executor(func, *args)) async def _after_publish(self, db_path): """Everything after the copy replaced the library: best effort, logged. @@ -609,7 +655,7 @@ class BackupRestoreRoutesMixin: {"success": False, "error": REQUESTS_BUSY}, status=409 ) model = self.db.model - staged = await self._run_in_executor(model.stage_restore, temp_path) + staged = await self._stage_to_end(model, temp_path) try: backup_path = await self._publish(model, staged) finally: @@ -927,6 +973,30 @@ class BackupRestoreRoutesMixin: job.update(progress=100, status_line=line) +async def _outlive_cancel(future): + """Await ``future`` (an executor call's task) to its end, even through + cancels (#360). + + A cancelled task cannot stop the worker thread, so the files it uses and + the registry admission it runs under must outlive it: the cancel is held + until the worker returned. Returns whether the task was cancelled + meanwhile; the caller reads the outcome from ``future`` and re-raises + ``CancelledError``. A future cancelled itself (an executor shut down) is + not a cancel of the task: ``future.result()`` raises it. Never catches + ``GeneratorExit``: a closed loop still closes the coroutine. + """ + cancelled = False + while not future.done(): + try: + await asyncio.shield(future) + except asyncio.CancelledError: + if not future.cancelled(): + cancelled = True + except Exception: + pass # the caller reads it from the future + return cancelled + + class _Orphan: """A stage with no store entry, in the shape ``_discard_held`` takes.""" diff --git a/py/api/logging_routes.py b/py/api/logging_routes.py index 8918986c..ed13096a 100644 --- a/py/api/logging_routes.py +++ b/py/api/logging_routes.py @@ -11,6 +11,11 @@ import stat from aiohttp import web +try: + from ...utils.logging_config import ClearRefused, parse_confirmed +except ImportError: # loaded as the top-level ``py`` package (tests) + from utils.logging_config import ClearRefused, parse_confirmed + # Keys PromptManagerLogger.update_config() understands. Anything else is # rejected so a client cannot inject arbitrary entries into the logging config. LOG_CONFIG_KEYS = frozenset( @@ -263,10 +268,43 @@ class LoggingRoutesMixin: ) async def truncate_logs(self, request): - """Truncate all log files.""" + """Clear all logs: what ``{"expected": [...]}`` lists, or all (#389). + + ``expected`` holds the ``{name, identity}`` rows of ``/logs/files`` the + confirmation showed. Without a body (the legacy admin page, whose + confirmation lists nothing) every rotated copy present now is deleted. + A malformed list is 400; a clear that would empty or delete what was + not confirmed changes nothing and is 409 with the reason's ``code``. + """ try: + args = () + if request.can_read_body: + data, error_response = await self._read_json_body(request) + if error_response is not None: + return error_response + if "expected" in data: + try: + parse_confirmed(data["expected"]) + except ValueError: + return web.json_response( + { + "success": False, + "error": "expected must list name and identity" + " rows from /logs/files", + }, + status=400, + ) + args = (data["expected"],) logger_manager = self._get_logger_manager() - results = await self._run_in_executor(logger_manager.truncate_logs) + try: + results = await self._run_in_executor( + logger_manager.truncate_logs, *args + ) + except ClearRefused as refused: + return web.json_response( + {"success": False, "error": str(refused), "code": refused.code}, + status=409, + ) return web.json_response( { diff --git a/py/api/review.py b/py/api/review.py index 6f81e545..a07c02ce 100644 --- a/py/api/review.py +++ b/py/api/review.py @@ -677,7 +677,14 @@ def _verify_and_read(store, capture, item, root, need_metadata: bool): facts = store.verify(root, relative) captured = None if need_metadata: - captured = captured_review(store, capture, item_profile(item["evidence"])) + # The file's own name, so an inspection item resolves anchored on its + # saver as it was recorded (#390). + captured = captured_review( + store, + capture, + item_profile(item["evidence"]), + relative.rsplit("/", 1)[-1], + ) return relative, facts, captured @@ -723,6 +730,9 @@ def _candidate_still_offered(resolution: dict, choice: str) -> bool: that rewords the top-level reason or reorders candidates leaves a pending item applicable. """ + if isinstance(resolution, dict) and resolution.get("anchor") == "saver": + # Only the saver anchor resolves the bytes (#390): its pair alone. + resolution = {"state": "ambiguous", "candidates": [resolution]} if not isinstance(resolution, dict) or resolution.get("state") != "ambiguous": return False evidence, _count, limited = review_evidence(resolution) diff --git a/py/api/review_recording.py b/py/api/review_recording.py index 6de59641..141cf14a 100644 --- a/py/api/review_recording.py +++ b/py/api/review_recording.py @@ -88,7 +88,8 @@ class ReviewRecordingMixin: def _record_scanned_review(self, media_file, roots, review=None, scanned=None): """Record one ambiguous, unlinked scanned file (logged and counted). - ``scanned`` is the scan's ``(metadata, inspection_resolution)``. + ``scanned`` is the scan's ``(metadata, inspection_resolution)``; the + resolution is None when the scan did not run the inspection profile. """ try: outcome = self._record_review_sync( @@ -199,7 +200,7 @@ class ReviewRecordingMixin: key = _memory_key(locator) if state is not None and memory.get(key) == state: return None - resolution = review_capture.inspection_choice(canonical) + resolution = review_capture.inspection_choice(canonical, media_file.name) if resolution is not None: memory.pop(key, None) return canonical, resolution diff --git a/py/api/thumbnail_auto.py b/py/api/thumbnail_auto.py index bfe38315..0c9095cf 100644 --- a/py/api/thumbnail_auto.py +++ b/py/api/thumbnail_auto.py @@ -88,9 +88,9 @@ from .thumbnail_encode import shutdown_encoders, writer_settings from .thumbnail_jobs import ThumbnailOptions try: - from ...utils.parallel import map_parallel + from ...utils.parallel import map_parallel, note_refusal, refused_at_shutdown except ImportError: # tests import the package top-level - from utils.parallel import map_parallel + from utils.parallel import map_parallel, note_refusal, refused_at_shutdown # The placeholder frame a video without a preview is answered with (JPEG # bytes, never written to disk); written previews follow the settings. @@ -410,7 +410,8 @@ class AutoThumbnailer: def enqueue(self, src, dst, is_video): """Queue *dst*'s preview for the worker; False when already queued, - full or closed. Never blocks on a write.""" + full, closed or its worker is refused at interpreter exit. Never + blocks on a write.""" key = _dst_key(dst) with self._changed: if self._closed or key in self._queued: @@ -420,30 +421,41 @@ class AutoThumbnailer: self._queued.add(key) self._queue.append((key, Path(src), Path(dst), bool(is_video))) self._busy = True + started = True if self._start_worker and not self._worker_running: - self._start_worker_locked() + started = self._start_worker_locked() + if not started: + # Refused at interpreter exit: no worker will take it (#375). + self._queue.pop() + self._queued.discard(key) self._changed.notify_all() - return True + return started def _start_worker_locked(self): - """Start the worker (lock held). A start that fails leaves the file - queued and the next enqueue tries again.""" + """Start the worker (lock held); False only when the interpreter is + exiting (#375: logged once at DEBUG, the caller drops the file). + Another start failure leaves the file queued and the next enqueue + tries again.""" worker = threading.Thread(target=self._run, name=WORKER_NAME, daemon=True) worker.pm_preview_token = _MODULE_TOKEN worker.pm_preview_close = self.close self._worker_running = True try: worker.start() - except RuntimeError: + except RuntimeError as error: self._worker_running = False self._busy = False + if refused_at_shutdown(error): + note_refusal(self._logger, "The automatic preview worker") + return False self._logger.warning( "The automatic preview worker could not start; the next output " "tries again", exc_info=True, ) - return + return True self._worker = worker + return True def _run(self): try: @@ -790,8 +802,8 @@ class AutoThumbnailMixin: Returns ``queued``, ``fresh`` (its preview is current) or ``skipped`` (not a media file under a current root, a link, a preview itself, - already queued, or after the app's cleanup). Never writes on the - caller's thread. + already queued, after the app's cleanup, or when its worker is refused + at interpreter exit). Never writes on the caller's thread. """ if self._auto_thumbnails_shut: return "skipped" diff --git a/requirements-dev.txt b/requirements-dev.txt index 43e2130a..6db215e2 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -8,8 +8,8 @@ coverage>=7.16.2 numpy>=2.2.6 # WD14 engine tests # Code quality -black>=26.10.1 -flake8>=7.4.1 +black==26.10.1 +flake8==7.4.1 # Security scanning bandit[toml]>=1.9.4 diff --git a/tests/browser/images-concept.spec.mjs b/tests/browser/images-concept.spec.mjs index 08479b3d..fea7a5ea 100644 --- a/tests/browser/images-concept.spec.mjs +++ b/tests/browser/images-concept.spec.mjs @@ -286,7 +286,8 @@ const continuationText=page=>page.locator('.infinite-continuation > p').first(). async function searchOf(page,term) { const before=await countText(page); await page.locator('#search').fill(term); - await expect.poll(()=>countText(page)).not.toBe(before); + // Production's caption reads "Loading images…" until the search's listing answers (#391); the concept's is instant. + await expect.poll(async()=>{const text=await countText(page);return text!==before&&text!=='Loading images…';}).toBe(true); await expect.poll(()=>page.locator(CARDS).count()).toBeLessThan(FIXTURE_IMAGES); await settle(page); const result={count:await countText(page),continuation:await continuationText(page),cards:await page.locator(`${CARDS} .image-open`).evaluateAll(nodes=>nodes.map(node=>node.getAttribute('aria-label')))}; diff --git a/tests/browser/inspector-concept.spec.mjs b/tests/browser/inspector-concept.spec.mjs index 0a43d7f1..48b24739 100644 --- a/tests/browser/inspector-concept.spec.mjs +++ b/tests/browser/inspector-concept.spec.mjs @@ -228,7 +228,8 @@ async function productionSendOf(page,context) { async function tagOf(page) { const before=await page.locator('#result-count').textContent(); await panelOf(page).locator('.detail-inner > .tags').last().getByRole('button',{name:'cinematic',exact:true}).click(); - await expect.poll(()=>page.locator('#result-count').textContent()).not.toBe(before); + // Production's caption reads "Loading images…" until the tag's listing answers (#391); the concept's search is instant. + await expect.poll(async()=>{const text=await page.locator('#result-count').textContent();return text!==before&&text!=='Loading images…';}).toBe(true); await settle(page); return {count:await page.locator('#result-count').textContent(), cards:await page.locator(`${CARDS} .image-open`).evaluateAll(nodes=>nodes.map(node=>node.getAttribute('aria-label')))}; @@ -328,6 +329,25 @@ test('a recipe-linked image without recipe metadata shows its positive prompt me await expect(panelOf(page).locator('.detail-footer button')).toHaveText(['Send to ComfyUI','Prompt history']); }); +// #391: a tag chip replaces the collection; until its listing answers the caption says it is loading, never that the +// filter matched nothing ("0 of 0 images" was painted before the response and read by a fast reader). +test('a tag chip collection reads as loading, not empty, until its listing answers (#391)',async({page})=>{ + await page.setViewportSize({width:1600,height:1100}); + await openProductionPanel(page,'dark'); + let release,arrived; + const held=new Promise(resolve=>{release=resolve;}),reached=new Promise(resolve=>{arrived=resolve;}); + await page.route(url=>url.pathname==='/prompt_manager/images/output'&&url.searchParams.get('tags')==='cinematic', + async route=>{arrived();await held;await route.continue();},{times:1}); + const count=page.locator('#result-count'); + await panelOf(page).locator('.detail-inner > .tags').last().getByRole('button',{name:'cinematic',exact:true}).click(); + await reached; + await expect(count).toHaveText('Loading images…'); + await expect(page.locator(CARDS)).toHaveCount(0); + release(); + await expect(count).toHaveText(countCopy('8 of 8 images · 2 positive prompts')); + await expect(page.locator(CARDS)).toHaveCount(8); +}); + // Task C3 (D88, #249 PR C): the concept's chip row is the image's own tags (`i.tags`, app.js a402). With those names // saved as the file's current image tags, production draws the row from them (Tags filter buttons in the panel, plain // in the viewer, the linked positive's equal tags not repeated) and still equals the concept, in both sizes. diff --git a/tests/browser/legacy-system-links.spec.mjs b/tests/browser/legacy-system-links.spec.mjs new file mode 100644 index 00000000..50b1c420 --- /dev/null +++ b/tests/browser/legacy-system-links.spec.mjs @@ -0,0 +1,163 @@ +// System S13 (#330): the legacy admin and gallery pages open native System for their System tools. Each replaced +// control is a plain link (`workspace#/system/`, a new tab), and the legacy flows S13 keeps still work. +import { startCoverage, saveCoverage } from '../../tools/vendor/browser-coverage.mjs'; +import { test, expect } from './fixtures.mjs'; + +const PAGES = Object.freeze({ admin: '/prompt_manager/admin', gallery: '/prompt_manager/gallery' }); +const LABELS = Object.freeze({ + overview: 'Overview', + appearance: 'Appearance', + sources: 'Sources & scans', + integrations: 'Integrations', + tagging: 'Auto-tagging', + maintenance: 'Maintenance', + backups: 'Backup & export', + diagnostics: 'Diagnostics & logs', +}); + +// One row per System link of the design's §2 map: the page, the control's label, how to find it, the section it opens +// and what a test must arrange before the control shows. +const LINKS = Object.freeze([ + { page: 'admin', label: 'Diagnostics', locate: page => page.locator('#diagnosticsBtn'), section: 'diagnostics' }, + { page: 'admin', label: 'View Logs', locate: page => page.locator('#logsBtn'), section: 'diagnostics' }, +]); + +async function post(request, url, data) { + await expect(await request.post(url, data === undefined ? {} : { data })).toBeOK(); +} + +test.beforeEach(async ({ page, request, browserName }) => { + await startCoverage(page, browserName); + await post(request, '/__test__/reset'); + await post(request, '/__test__/system/reset'); + await post(request, '/__test__/tagging/reset'); + // The 3.2.0 update notice would cover the admin page's toolbar. + await page.addInitScript(() => { localStorage.setItem('pm_update_notice_dismissed', '3.2.0'); }); + page.legacyErrors = []; + page.on('pageerror', error => page.legacyErrors.push(error.message)); +}); + +test.afterEach(async ({ page, request, browserName }) => { + await saveCoverage(page, browserName); + await post(request, '/__test__/tagging/reset'); + await post(request, '/__test__/system/reset'); + expect(page.legacyErrors).toEqual([]); +}); + +/** Opens the legacy page with the link showing; returns the link. */ +async function openLink(page, request, link) { + if (link.arrange) await link.arrange(page, request); + await page.goto(PAGES[link.page]); + if (link.reveal) await link.reveal(page); + const control = link.locate(page); + await expect(control).toBeVisible(); + return control; +} + +for (const link of LINKS) { + test(`legacy ${link.page} ${link.label} opens System ${link.section}`, async ({ page, context, request }) => { + const control = await openLink(page, request, link); + const legacyUrl = page.url(); + await expect(control).toHaveAttribute('href', `workspace#/system/${link.section}`); + await expect(control).toHaveAttribute('target', '_blank'); + await expect(control).toHaveAttribute('rel', 'noopener'); + const opened = context.waitForEvent('page'); + await control.click(); + const system = await opened; + await expect(system).toHaveURL(new RegExp(`/prompt_manager/workspace#/system/${link.section}$`)); + // System's page heading is "System"; the open section is the pressed button of its section switcher. + const main = system.locator('#workspace-main'); + await expect(main.getByRole('heading', { level: 1, name: 'System', exact: true })).toBeVisible(); + await expect(main.getByRole('group', { name: 'System sections', exact: true }) + .getByRole('button', { name: LABELS[link.section], exact: true })).toHaveAttribute('aria-pressed', 'true'); + expect(page.url()).toBe(legacyUrl); + await system.close(); + }); +} + +test('a System link sends no request from the legacy page', async ({ page, context, request }) => { + for (const link of LINKS) { + const control = await openLink(page, request, link); + // The page's own start-up reads (list, film strips, settings) finish first; then the click must add none. + await page.waitForLoadState('networkidle'); + const sent = []; + const record = item => sent.push(`${item.method()} ${item.url()}`); + page.on('request', record); + const opened = context.waitForEvent('page'); + await control.click(); + const system = await opened; + await system.waitForLoadState('domcontentloaded'); + page.off('request', record); + expect(sent, `${link.page} ${link.label}`).toEqual([]); + await system.close(); + } +}); + +test('kept admin flows still work', async ({ page, context }) => { + await page.goto(PAGES.admin); + const texts = page.locator('#resultsList .prompt-text'); + await expect(texts.first()).toBeVisible(); + + // K1: sorting reorders the list. + for (const [value, order] of [['text_asc', 1], ['text_desc', -1]]) { + const listed = page.waitForResponse(item => /\/prompt_manager\/(recent|search)\?/.test(item.url()) && item.url().includes(`sort=${value}`)); + await page.locator('#sortBy').selectOption(value); + await listed; + await expect.poll(async () => { + const shown = await texts.allTextContents(); + // The server sorts case-insensitively (SQLite NOCASE), not by locale. + const key = text => text.toLowerCase(); + const sorted = [...shown].sort((a, b) => order * (key(a) < key(b) ? -1 : key(a) > key(b) ? 1 : 0)); + return shown.length > 1 && shown.join('\n') === sorted.join('\n'); + }).toBe(true); + } + // K3: Add Prompt saves a prompt the list then shows. + await page.locator('#addPromptBtn').click(); + await page.locator('#addPromptText').fill('Kept flow lighthouse'); + const saved = page.waitForResponse(item => item.request().method() === 'POST' && item.url().endsWith('/prompt_manager/save')); + await page.locator('#saveNewPromptBtn').click(); + expect((await saved).ok()).toBe(true); + await expect(page.locator('#addPromptModal')).toBeHidden(); + await expect(texts.filter({ hasText: 'Kept flow lighthouse' })).toBeVisible(); + + // K1: a search narrows the list, and a selection enables the bulk buttons. + const all = await texts.allTextContents(); + const word = all[0].trim().split(/\s+/)[0]; + const searched = page.waitForResponse(item => item.url().includes('/prompt_manager/search?') && item.url().includes('text=')); + await page.locator('#searchText').fill(word); + await page.locator('#searchBtn').click(); + await searched; + await expect.poll(async () => { + const shown = await texts.allTextContents(); + return shown.length > 0 && shown.length < all.length && shown.every(text => text.toLowerCase().includes(word.toLowerCase())); + }).toBe(true); + await expect(page.locator('#bulkDeleteBtn')).toBeDisabled(); + await page.locator('#resultsList .prompt-checkbox').first().check(); + for (const id of ['#bulkDeleteBtn', '#bulkTagBtn', '#bulkCategoryBtn']) await expect(page.locator(id)).toBeEnabled(); + + // K4: Stats reads the statistics again; Export downloads the prompts. + const stats = page.waitForResponse(item => item.url().endsWith('/prompt_manager/stats') && item.ok()); + await page.locator('#statsBtn').click(); + await stats; + await expect(page.getByText('Stats refreshed successfully!')).toBeVisible(); + const download = page.waitForEvent('download'); + await page.locator('#exportBtn').click(); + expect((await download).suggestedFilename()).toMatch(/^prompts-\d{4}-\d{2}-\d{2}\.json$/); + + // K5: Scan Images opens its dialog (with the quick backup) without starting a scan. + await page.locator('#scanBtn').click(); + await expect(page.locator('#scanModal')).toBeVisible(); + await expect(page.locator('#quickBackupBtn')).toBeVisible(); + await page.locator('#cancelScan').click(); + await expect(page.locator('#scanModal')).toBeHidden(); + + // K7: Metadata and Gallery open their pages in a new tab. + for (const [id, path] of [['#metadataBtn', '/prompt_manager/gallery.html'], ['#galleryBtn', '/prompt_manager/gallery']]) { + const opened = context.waitForEvent('page'); + await page.locator(id).click(); + const tab = await opened; + await tab.waitForLoadState('domcontentloaded'); + expect(new URL(tab.url()).pathname).toBe(path); + await tab.close(); + } +}); diff --git a/tests/browser/system-tagging-concept.spec.mjs b/tests/browser/system-tagging-concept.spec.mjs index b02f5a83..ff79595c 100644 --- a/tests/browser/system-tagging-concept.spec.mjs +++ b/tests/browser/system-tagging-concept.spec.mjs @@ -127,9 +127,9 @@ function mapTaggingConcept(reference, { template, phone }) { actions: { ...reference.actions, buttons: [preview, ['Download again', ''], ['Unload', '']] }, // D134: the prompt-tag batch, reviewed first. operations: [{ tree: ['div', 'div.row-actions'], title: 'Tag output images', description: BATCH, actions: [['Review', '']] }], - // D132: the engine select shows its whole chosen option at every width (at 390 the concept's 120px cap cuts it; - // `#tag-model` alone may grow there). Existing tags keeps the concept's own fit, cut or not. - selectsFit: reference.selectsFit.map((fit, index) => (index === 0 ? true : fit)), + // D132 (#398): every select shows its whole chosen option at every width (at 390 the concept's 120px cap cuts + // them; production lets each grow past it). + selectsFit: reference.selectsFit.map(() => true), }; } @@ -168,16 +168,28 @@ for (const theme of THEMES) for (const [width, height] of SIZES) { }); } -// D132 below the concept's smallest size: on a 320px phone the engine select still shows its whole option. -for (const theme of THEMES) test(`auto-tagging engine select fits at 320 wide ${theme}`, async ({ page }) => { - await page.setViewportSize({ width: 320, height: 700 }); - const production = await productionTagging(page, theme); - for (const model of ['hf', 'gguf', 'wd14-swinv2']) { - await page.locator('#workspace-main #tag-model').selectOption(model); - expect((await page.evaluate(measureTagging, '#workspace-main')).selectsFit[0], model).toBe(true); - } - expect(production.pageScroll).toBe(false); -}); +// D132 (#398) at phone width and below the concept's smallest size: every option of every select, once chosen, shows +// whole, with no page scroll. +for (const theme of THEMES) for (const width of [390, 320]) { + test(`auto-tagging selects fit every option at ${width} wide ${theme}`, async ({ page }, testInfo) => { + await page.setViewportSize({ width, height: 844 }); + await productionTagging(page, theme); + await page.locator('#workspace-main #tag-existing').selectOption('replace_after_review'); + await page.locator('#workspace-main section.surface').screenshot({ path: testInfo.outputPath(`production-tagging-selects-${theme}-${width}.png`) }); + const selects = page.locator('#workspace-main section.surface > .setting-row > select'); + await expect(selects).toHaveCount(2); + for (let index = 0; index < 2; index += 1) { + const select = selects.nth(index); + const values = await select.evaluate(node => [...node.options].map(option => option.value)); + for (const value of values) { + await select.selectOption(value); + const measured = await page.evaluate(measureTagging, '#workspace-main'); + expect(measured.selectsFit, `${await select.getAttribute('id')}=${value}`).toEqual([true, true]); + expect(measured.pageScroll, value).toBe(false); + } + } + }); +} test('auto-tagging comparison detects mutations', async ({ page, request }) => { await page.setViewportSize({ width: 1600, height: 1100 }); @@ -222,6 +234,9 @@ test('auto-tagging comparison detects mutations', async ({ page, request }) => { const undoSqueeze = await mutate(page, { css: '#workspace-main #tag-model { max-width: 60px !important; min-width: 0 !important; }' }); expect(await paths()).toEqual(expect.arrayContaining(['selectsFit.0'])); await undoSqueeze(); + const undoSqueezeExisting = await mutate(page, { css: '#workspace-main #tag-existing { max-width: 60px !important; min-width: 0 !important; }' }); + expect(await paths()).toEqual(expect.arrayContaining(['selectsFit.1'])); + await undoSqueezeExisting(); // A row that wraps where the concept's does not. const undoWrap = await mutate(page, { css: '#workspace-main .setting-row:has(> #tag-existing) { flex-wrap: wrap; }' }); expect(await paths()).toEqual(['rows.1.flexWrap']); @@ -233,14 +248,14 @@ test('auto-tagging comparison detects mutations', async ({ page, request }) => { expect(diffInventory(expected, await measure())).toEqual([]); }); -// D132 at phone width, unmapped: only the engine row wraps; Existing tags keeps the concept's cut select. +// D132 at phone width: only the engine row wraps; Existing tags grows beside its text instead (#398). test('auto-tagging at 390 wraps only the engine row', async ({ page, request }) => { await page.setViewportSize({ width: 390, height: 844 }); const concept = await conceptTagging(page, 'dark'); const production = await productionTagging(page, 'dark'); expect(diffInventory(mapTaggingConcept(concept, await savedTemplate(request, page)), production)).toEqual([]); expect(production.rows.map(row => row.flexWrap)).toEqual(['wrap', 'nowrap', 'nowrap', 'nowrap', 'nowrap']); - expect(production.selectsFit[1]).toBe(concept.selectsFit[1]); + expect(production.selectsFit).toEqual([true, true]); }); /** Runs in the page: the open review dialog: its title, body and the concept's tokens. */ diff --git a/tests/browser/unseen-restore.mjs b/tests/browser/unseen-restore.mjs new file mode 100644 index 00000000..aabd8962 --- /dev/null +++ b/tests/browser/unseen-restore.mjs @@ -0,0 +1,54 @@ +// A restore the open workspace page has not noticed yet (#413). +// +// Specs that restore through the API while a page is open check what the page does with a restore it did not see: +// its next write is refused as "Prompt storage was restored since this dialog opened". But the page's Activity store +// reads the job list every 15 s (activity-store.mjs POLL_MS), and a read the server answers while the restore job runs +// makes the page follow it: when it finishes, the page is shown again from the restored data (S9, index.mjs +// onRestoreFinished), which closes any open dialog. On a slow runner a restore can take longer than the time left to +// the next read (CI run 40646112668: the restore took 16 s), so whether the page saw it depended on timing. +// +// `armUnseenRestores(page)`, before the page loads, passes every job-list read of the page through one route, so it +// knows which reads the server has not answered yet. `unseenRestore(page, restoring)` then closes the gate, waits +// for those reads' answers (sent before the restore began, they cannot list it), runs `restoring()`, and only after +// it has answered lets the held reads go on: they read the restore as already ended, as a tab that read its job list +// just before and just after the restore does. A restore that is never seen running never reloads the page. +const JOBS = '**/prompt_manager/jobs'; +const gates = new WeakMap(); + +/** Route `page`'s job-list reads through the restore gate. Call before the page's first navigation. */ +export async function armUnseenRestores(page) { + const gate = { closed: null, inFlight: 0, idle: [] }; + gates.set(page, gate); + await page.route(JOBS, async route => { + if (gate.closed) { + await gate.closed; + return route.fallback(); + } + gate.inFlight += 1; + try { + const response = await route.fetch(); + await route.fulfill({ response }).catch(() => {}); + } catch { + await route.abort().catch(() => {}); + } finally { + gate.inFlight -= 1; + if (gate.inFlight === 0) for (const resolve of gate.idle.splice(0)) resolve(); + } + }); +} + +/** Run `restoring()` (an API restore) unseen by `page`'s Activity store; resolves with what it resolves with. */ +export async function unseenRestore(page, restoring) { + const gate = gates.get(page); + if (!gate) throw new Error('armUnseenRestores(page) must run before the page loads'); + let open; + gate.closed = new Promise(resolve => { open = resolve; }); + try { + // Reads already on their way were sent before the restore: their answers cannot list it. + if (gate.inFlight > 0) await new Promise(resolve => gate.idle.push(resolve)); + return await restoring(); + } finally { + gate.closed = null; + open(); + } +} diff --git a/tests/browser/workspace-history.spec.mjs b/tests/browser/workspace-history.spec.mjs index 48f81074..f912a892 100644 --- a/tests/browser/workspace-history.spec.mjs +++ b/tests/browser/workspace-history.spec.mjs @@ -1,5 +1,6 @@ import {startCoverage,saveCoverage} from '../../tools/vendor/browser-coverage.mjs'; import {test,expect} from './fixtures.mjs'; +import {armUnseenRestores,unseenRestore} from './unseen-restore.mjs'; import {readFile,writeFile} from 'node:fs/promises'; import {createHash} from 'node:crypto'; @@ -16,13 +17,13 @@ const marker=(page,id,anchor='10')=>recorded(page).locator(`button[data-run-id=" const exact=async locator=>locator.evaluate(node=>node.textContent); test.beforeEach(async({page,request,browserName})=>{ - await startCoverage(page,browserName);page.historyErrors=[];page.on('pageerror',error=>page.historyErrors.push(error.message)); + await startCoverage(page,browserName);await armUnseenRestores(page);page.historyErrors=[];page.on('pageerror',error=>page.historyErrors.push(error.message)); const reset=await request.post('/__test__/reset',{data:{chronological_history:true}});await expect(reset).toBeOK();page.fixture=await reset.json(); }); test.afterEach(async({page,browserName})=>{await saveCoverage(page,browserName);expect(page.historyErrors).toEqual([]);}); const command=async(request,data)=>{const response=await request.post('/__test__/history/commands',{data});await expect(response).toBeOK();return response.json();}; -const restoreSnapshot=async(request,bytes)=>{const response=await request.post('/prompt_manager/restore',{multipart:{database_file:{name:'history.db',mimeType:'application/octet-stream',buffer:bytes}}});await expect(response).toBeOK();expect((await response.json()).success).toBe(true);}; +const restoreSnapshot=async(page,request,bytes)=>{const response=await unseenRestore(page,()=>request.post('/prompt_manager/restore',{multipart:{database_file:{name:'history.db',mimeType:'application/octet-stream',buffer:bytes}}}));await expect(response).toBeOK();expect((await response.json()).success).toBe(true);}; const historyRead=async(request,id)=>{const response=await request.get(`/prompt_manager/library/${id}/history`);await expect(response).toBeOK();return response.json();}; const frozenContexts=contexts=>contexts.map(({navigation,positive_prompt_id,negative_prompt_id,pair_id,...frozen})=>frozen); const immutableReceipts=items=>items.map(({current_candidate,...receipt})=>receipt); @@ -62,7 +63,7 @@ for(const boundary of ['window','detail','candidate','inspection','media']) test await command(request,{action:'arm',boundary});const pending=request.get(target); try { await expect.poll(async()=> (await(await request.get('/__test__/history/checkpoint')).json()).entered).toBe(true); - await restoreSnapshot(request,bytes); + await restoreSnapshot(page,request,bytes); } finally {await command(request,{action:'release'});} const retired=await pending;expect(retired.status()).toBe(409);expect((await retired.json()).code).toBe('history-changed'); for(const url of [`/prompt_manager/library/${f.detail_id}/history?limit=1&cursor=${encodeURIComponent(firstWindow.next_cursor)}`,`${base}?epoch=${epoch}`,`${base}/outputs?epoch=${epoch}`,`${base}/outputs?epoch=${epoch}&cursor=${encodeURIComponent(outputs.next_cursor)}`,media.url,media.thumbnail_url,media.inspection_url]) { @@ -130,7 +131,7 @@ test('archive delete merge and prune preserve frozen evidence without repairing test('actual restore with reused numeric owner rejects stale history and offers explicit saved recovery',async({page,request})=>{ const f=page.fixture;await open(page,f.detail_id);const original=await exact(saved(page).getByRole('region',{name:'Positive prompt',exact:true}).locator('p.prompt-text')); - const source=await request.get('/__test__/history/reused-snapshot');await expect(source).toBeOK();await restoreSnapshot(request,await source.body()); + const source=await request.get('/__test__/history/reused-snapshot');await expect(source).toBeOK();await restoreSnapshot(page,request,await source.body()); await marker(page,f.runs.shared).click();await expect(dialog(page)).toContainText('Recorded evidence changed');await page.keyboard.press('Escape'); await expect(page.locator('.library-parent-status')).toContainText('changed');expect(await exact(saved(page).getByRole('region',{name:'Positive prompt',exact:true}).locator('p.prompt-text'))).toBe(original); await expect(recorded(page).getByRole('button',{name:'Reload recorded history',exact:true})).toBeVisible(); @@ -142,12 +143,12 @@ for(const outcome of ['success','error'])test(`delayed actual old browser ${outc const f=page.fixture;await open(page,f.detail_id);const snapshot=await request.get('/__test__/history/snapshot');const bytes=await snapshot.body(); let entered,release;const started=new Promise(resolve=>{entered=resolve;}),held=new Promise(resolve=>{release=resolve;}); await page.route(new RegExp(`/history/runs/${f.runs.shared}\\?`),async route=>{ - if(outcome==='error')await restoreSnapshot(request,bytes); + if(outcome==='error')await restoreSnapshot(page,request,bytes); const response=await route.fetch();expect(response.status()).toBe(outcome==='success'?200:409);entered();await held; await route.fulfill({response}).catch(()=>{}); }); try { - await marker(page,f.runs.shared).click();await started;if(outcome==='success')await restoreSnapshot(request,bytes); + await marker(page,f.runs.shared).click();await started;if(outcome==='success')await restoreSnapshot(page,request,bytes); await page.evaluate(()=>{location.hash='#/overview';});await expect(page.locator('#workspace-main h1')).toHaveText('Overview');release(); await page.unroute(new RegExp(`/history/runs/${f.runs.shared}\\?`));await open(page,f.detail_id); await expect(recorded(page).locator('button[data-run-id]')).toHaveCount(0); @@ -258,7 +259,7 @@ test('inspection first discovers a real restore and immediately retires every su if(changed)window.recordedRecoveryStates.push({receipts:run.querySelectorAll('[data-receipt]').length,active:[...run.querySelectorAll('[data-send-role],[data-copy-role],[data-view-receipt]')].filter(node=>!node.disabled&&node.checkVisibility()).length}); });observer.observe(run,{subtree:true,childList:true,attributes:true,characterData:true}); }); - await restoreSnapshot(request,await snapshot.body()); + await restoreSnapshot(page,request,await snapshot.body()); } finally {await command(request,{action:'release'});} const response=await inspectionRead;expect(response.status()).toBe(409);expect((await response.json()).code).toBe('history-changed'); await expect(dialog(page).locator('.recorded-run-status')).toContainText('Recorded evidence changed'); diff --git a/tests/browser/workspace-library-editor.spec.mjs b/tests/browser/workspace-library-editor.spec.mjs index cac57771..2cc85b25 100644 --- a/tests/browser/workspace-library-editor.spec.mjs +++ b/tests/browser/workspace-library-editor.spec.mjs @@ -1,6 +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 {captureCanonicalState} from './history-canonical-comparison.mjs'; import {inventoryOf,mapConcept,diffInventory} from './library-editor-canonical-comparison.mjs'; @@ -13,7 +14,7 @@ const RATINGS=[['5','5 / 5'],['4','4 / 5'],['3','3 / 5'],['2','2 / 5'],['1','1 / const editorDialog=page=>page.locator('dialog.library-editor'); test.beforeEach(async({page,request,browserName})=>{ - await startCoverage(page,browserName); + await startCoverage(page,browserName);await armUnseenRestores(page); const reset=await request.post('/__test__/library-editor/reset',{data:{}});await expect(reset).toBeOK();page.fixture=await reset.json(); page.editorErrors=[];page.on('pageerror',error=>page.editorErrors.push(error.message)); page.writes=[];page.on('request',item=>{if(item.method()!=='GET')page.writes.push({method:item.method(),url:new URL(item.url()).pathname,body:item.postDataJSON()});}); @@ -260,7 +261,8 @@ test('recovery states render inline and never replay a write',async({page,reques page.writes.length=0; dialog=await openEditor(page,{mode:'new'});await ready(dialog); const snapshot=await request.get('/__test__/library-editor/snapshot');await expect(snapshot).toBeOK(); - const restore=await request.post('/prompt_manager/restore',{multipart:{database_file:{name:'editor.db',mimeType:'application/octet-stream',buffer:await snapshot.body()}}}); + const buffer=await snapshot.body(); + const restore=await unseenRestore(page,()=>request.post('/prompt_manager/restore',{multipart:{database_file:{name:'editor.db',mimeType:'application/octet-stream',buffer}}})); await expect(restore).toBeOK();page.writes.length=0; await field(dialog,'Title').fill('After restore');await field(dialog,'Positive prompt').fill('Restored storage prompt');await button(dialog,'Save').click(); await expect(note(dialog)).toHaveText('Prompt storage was restored since this dialog opened. Nothing was saved.'); @@ -588,7 +590,7 @@ test('saved refresh failure retries only reads',async({page})=>{ test('restored storage after a confirmed save or before an archive offers an explicit Library reload',async({page,request})=>{ const f=page.fixture;const snapshot=await(await request.get('/__test__/library-editor/snapshot')).body(); - const restore=()=>request.post('/prompt_manager/restore',{multipart:{database_file:{name:'editor.db',mimeType:'application/octet-stream',buffer:snapshot}}}); + const restore=()=>unseenRestore(page,()=>request.post('/prompt_manager/restore',{multipart:{database_file:{name:'editor.db',mimeType:'application/octet-stream',buffer:snapshot}}})); await openHistory(page,f.positive_id); await page.route(`**/prompt_manager/library/${f.positive_id}`,async route=>{ if(route.request().method()!=='PATCH')return route.continue(); @@ -615,7 +617,7 @@ test('restored storage after a confirmed save or before an archive offers an exp const DISCARDS='Leaving this dialog discards your entries.'; test('restored storage before an Edit or Add negative save offers only the Library reload exit',async({page,request})=>{ const f=page.fixture;const snapshot=await(await request.get('/__test__/library-editor/snapshot')).body(); - const restore=()=>request.post('/prompt_manager/restore',{multipart:{database_file:{name:'editor.db',mimeType:'application/octet-stream',buffer:snapshot}}}); + const restore=()=>unseenRestore(page,()=>request.post('/prompt_manager/restore',{multipart:{database_file:{name:'editor.db',mimeType:'application/octet-stream',buffer:snapshot}}})); await openHistory(page,f.positive_id); await page.getByRole('button',{name:'Edit metadata',exact:true}).click();let dialog=editorDialog(page);await ready(dialog); await expect(await restore()).toBeOK(); diff --git a/tests/browser/workspace-system-backups.spec.mjs b/tests/browser/workspace-system-backups.spec.mjs index 6856490f..6251d0f1 100644 --- a/tests/browser/workspace-system-backups.spec.mjs +++ b/tests/browser/workspace-system-backups.spec.mjs @@ -47,6 +47,17 @@ async function serverJson(request, url) { return response.json(); } +/** + * The restore job's own end (#361): its progress stream closes when the job takes its final state, however long the + * disk-bound publish takes (bounded by the test, not by a 5 s expect), and the job finished. The toast the page shows + * for it is then the page's reaction alone, checked with the default timeout. + */ +async function restoreFinished(request, id) { + await expect(await request.get(`/prompt_manager/jobs/${id}/events`)).toBeOK(); + const { job } = await serverJson(request, `/prompt_manager/jobs/${id}`); + expect([job.kind, job.state]).toEqual(['restore', 'finished']); +} + async function openBackups(page) { // A fresh page each time: the drafts live for the page session (Q10), and a hash-only goto would not reload. if (page.url().endsWith('#/system/backups')) await page.reload(); @@ -216,6 +227,7 @@ test('Restore starts the job, opens Activity; views reload after it finishes', a const { job } = await (await applied).json(); expect([job.kind, job.title, job.cancellable]).toEqual(['restore', 'Database restore', false]); await expect(page).toHaveURL(`${workspace}#/activity/${job.id}`); + await restoreFinished(request, job.id); await expect(toast(page)).toHaveText(/^Database restore finished · Restored \d+ prompts?, /); // The library is the backup's again, and the page says so without a reload: the sidebar counts were read again. expect(await libraryTotal(request)).toBe(restoredTotal); @@ -244,7 +256,8 @@ test('a finished restore shows the open page again from the restored data', asyn const card = page.locator('#workspace-main article.prompt-card').filter({ hasText: 'S9 prompt the restore removes' }); await expect(card).toHaveCount(1); await expect(await request.post('/__test__/release')).toBeOK(); - await applied; + const { job } = await (await applied).json(); + await restoreFinished(request, job.id); await expect(toast(page)).toHaveText(/^Database restore finished · /); // The shelf was read again from the restored library, on the same route. await expect(card).toHaveCount(0); @@ -422,6 +435,7 @@ test('a refused restore reopens its review: Restore retries it, Discard releases await dialog(page).getByRole('button', { name: 'Restore', exact: true }).click(); const { job } = await (await applied).json(); await expect(page).toHaveURL(`${workspace}#/activity/${job.id}`); + await restoreFinished(request, job.id); await expect(toast(page)).toHaveText(/^Database restore finished · /); expect(await stagesOf(request)).toBe(0); }); diff --git a/tests/browser/workspace-system-diagnostics.spec.mjs b/tests/browser/workspace-system-diagnostics.spec.mjs index c0bcb963..9f811362 100644 --- a/tests/browser/workspace-system-diagnostics.spec.mjs +++ b/tests/browser/workspace-system-diagnostics.spec.mjs @@ -516,7 +516,7 @@ test('a rollover after the review deletes nothing and Activity offers Review aga test('Clear all logs states both counts, confirms first and reads everything again', async ({ page, request }) => { await seedLogs(request, LOG_FOLDER); const truncates = []; - page.on('request', item => { if (item.url().endsWith('/prompt_manager/logs/truncate')) truncates.push(item.url()); }); + page.on('request', item => { if (item.url().endsWith('/prompt_manager/logs/truncate')) truncates.push(item.postDataJSON()); }); await openDiagnostics(page); await expect(lines(page)).toContainText('Database backup verified'); const details = await openFiles(page); @@ -535,6 +535,10 @@ test('Clear all logs states both counts, confirms first and reads everything aga await dialog(page).getByRole('button', { name: 'Clear', exact: true }).click(); await expect(toast(page)).toHaveText('Logs cleared · 3 rotated copies deleted.'); expect(truncates).toHaveLength(1); + // #389: Clear sends back exactly the files the dialog was built from, each with its identity string. + expect(truncates[0].expected.map(row => row.name).sort()).toEqual(['prompt_manager.log', 'prompt_manager.log.1', + 'prompt_manager.log.2', 'prompt_manager.log.3']); + for (const row of truncates[0].expected) expect(row.identity).toMatch(/^\d+:\d+:\d+$/); // Read again: the viewer, the file line and the list. await expect(lines(page)).not.toContainText('Database backup verified'); await expect(details.locator(':scope > p.small').first()).toHaveText(/rotated copies: 0$/); @@ -564,6 +568,23 @@ test('Clear all logs reads the files again before it states what it deletes', as expect(await names(request)).toEqual(['prompt_manager.log', 'prompt_manager.log.1', 'prompt_manager.log.2', 'prompt_manager.log.3']); }); +test('a rollover while Clear all logs is open deletes nothing and says why (#389)', async ({ page, request }) => { + await seedLogs(request, { rotated: [1024, 1024], active: 100 }); + await openDiagnostics(page); + await openFiles(page); + await operation(page, 'Clear all logs').getByRole('button', { name: 'Clear all logs…', exact: true }).click(); + await expect(dialog(page).locator('#modal-body > p')).toHaveText(/^Empties prompt_manager\.log, deletes 2 rotated copies /); + // The log rolls over after the confirmation was built: the copies it listed are not the ones there now. + await expect(await request.post('/__test__/logs/rotate')).toBeOK(); + const rolled = await names(request); + const answer = page.waitForResponse(response => response.url().endsWith('/prompt_manager/logs/truncate')); + await dialog(page).getByRole('button', { name: 'Clear', exact: true }).click(); + expect((await answer).status()).toBe(409); + await expect(toast(page)).toHaveText('The log files changed since you confirmed; nothing was cleared. ' + + 'Read the list again and clear again.'); + expect(await names(request)).toEqual(rolled); +}); + test('each listed file downloads by its returned name and a link has no Download', async ({ page, request }) => { await seedLogs(request, LOG_FOLDER); await openDiagnostics(page); diff --git a/tests/browser/workspace-vocabulary.spec.mjs b/tests/browser/workspace-vocabulary.spec.mjs index 44248694..b9b82da2 100644 --- a/tests/browser/workspace-vocabulary.spec.mjs +++ b/tests/browser/workspace-vocabulary.spec.mjs @@ -1,6 +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 {captureCanonicalState} from './history-canonical-comparison.mjs'; import {pageInventoryOf,inventoryOf,diffInventory,mapVocabularyConcept,mapVocabularyDialog} from './vocabulary-canonical-comparison.mjs'; @@ -40,7 +41,7 @@ async function expectLibrary(page,{tags='',category='',count}) { let manifest; test.beforeEach(async({page,request,browserName})=>{ - await startCoverage(page,browserName);manifest=await seed(request); + await startCoverage(page,browserName);await armUnseenRestores(page);manifest=await seed(request); page.vocabularyErrors=[];page.on('pageerror',error=>page.vocabularyErrors.push(error.message)); }); test.afterEach(async({page,browserName})=>{await saveCoverage(page,browserName);expect(page.vocabularyErrors).toEqual([]);}); @@ -264,7 +265,8 @@ const toast=page=>page.locator('#workspace-toast'); const actionNames=scope=>scope.locator('.dialog-actions button').evaluateAll(nodes=>nodes.map(node=>node.textContent.trim())); const snapshot=async request=>{const response=await request.get('/__test__/vocabulary/snapshot');await expect(response).toBeOK();return response.json();}; const backup=async request=>{const response=await request.get('/__test__/vocabulary/backup');await expect(response).toBeOK();return response.body();}; -const restore=async(request,buffer)=>expect(await request.post('/prompt_manager/restore',{multipart:{database_file:{name:'vocabulary.db',mimeType:'application/octet-stream',buffer}}})).toBeOK(); +// A restore the open page has not seen, so its dialog stays to report it (#413). +const restore=async(page,request,buffer)=>expect(await unseenRestore(page,()=>request.post('/prompt_manager/restore',{multipart:{database_file:{name:'vocabulary.db',mimeType:'application/octet-stream',buffer}}}))).toBeOK(); const tagNames=rows=>rows.tags.map(([,name])=>name); function trackWrites(page) { const writes=[];page.on('request',item=>{if(item.method()!=='GET')writes.push({method:item.method(),url:new URL(item.url()).pathname,body:item.postDataJSON()});});return writes; @@ -482,7 +484,7 @@ test('conflict and restore states keep the dialog and write nothing',async({page const before=await snapshot(request); box=await openDialog(page,main(page).getByRole('button',{name:'+ Create tag',exact:true}),'Create tag'); - await restore(request,saved); + await restore(page,request,saved); await field(box,'Tag name').fill('sunrise');await button(box,'Apply').click(); await expect(note(box)).toHaveText('Prompt storage was restored since this dialog opened. Nothing was changed.'); await expect(button(box,'Apply')).toBeDisabled(); @@ -622,7 +624,7 @@ test('delete confirmation read failure, missing tag and restore keep Delete disa await expect(button(box,'Delete')).toBeDisabled();await expect(tagRow(page,'calm').locator('td:nth-child(2)')).toHaveText('2'); await button(box,'Cancel').click(); - box=await openRename(page,'mist');await restore(request,saved); + box=await openRename(page,'mist');await restore(page,request,saved); await button(box,'Delete tag…').click(); await expect(note(box)).toHaveText('Prompt storage was restored since this dialog opened. Nothing was changed.'); await expect(button(box,'Delete')).toBeDisabled();expect(await actionNames(box)).toEqual(['Reload','Cancel','Delete']); diff --git a/tests/browser/workspace_diagnostics_fixture.py b/tests/browser/workspace_diagnostics_fixture.py index b55cf343..bdbc1193 100644 --- a/tests/browser/workspace_diagnostics_fixture.py +++ b/tests/browser/workspace_diagnostics_fixture.py @@ -123,6 +123,34 @@ def _touch(path: Path, size: int, moment: datetime.datetime) -> None: os.utime(path, (stamp, stamp)) +def _append_active(manager, log_dir: Path, size: int) -> None: + """Grow the active log as this process's handler does (#389). + + A second handle appending to the file the handler writes is what Clear + all logs refuses as another program writing to it, so the bytes go + through the handler's own stream; with file logging off, to the file. + """ + + def same(path) -> bool: + return os.path.normcase(os.path.realpath(path)) == os.path.normcase( + os.path.realpath(log_dir / ACTIVE) + ) + + for handler in manager._file_handlers(): + stream = getattr(handler, "stream", None) + if stream is None or not same(handler.baseFilename): + continue + handler.acquire() + try: + stream.write("x" * size) + stream.flush() + finally: + handler.release() + return + with open(log_dir / ACTIVE, "ab") as handle: + handle.write(b"x" * size) + + def _bounded(value, name, top=MAX_SEED_BYTES): if isinstance(value, bool) or not isinstance(value, int) or not 0 <= value <= top: raise web.HTTPBadRequest(text=f"{name} must be an integer 0..{top}") @@ -169,8 +197,7 @@ def register_diagnostics_commands(app, routes, api, root: Path): for index, (time_text, level, message) in enumerate(CONCEPT_RECORDS): record = _record(time_text, level, message, index) manager.add_to_buffer(record, record.getMessage()) - with open(log_dir / ACTIVE, "ab") as handle: - handle.write(b"x" * CONCEPT_ACTIVE) + _append_active(manager, log_dir, CONCEPT_ACTIVE) for number, size in enumerate(CONCEPT_ROTATED, start=1): moment = ROTATED_TIME - datetime.timedelta(hours=number) _touch(log_dir / f"{ACTIVE}.{number}", size, moment) @@ -194,8 +221,7 @@ def register_diagnostics_commands(app, routes, api, root: Path): manager.add_to_buffer(record, record.getMessage()) active = _bounded(data.get("active", 0), "active") if active: - with open(log_dir / ACTIVE, "ab") as handle: - handle.write(b"x" * active) + _append_active(manager, log_dir, active) rotated = data.get("rotated", []) if not isinstance(rotated, list) or len(rotated) > 20: raise web.HTTPBadRequest(text="rotated must be a list of sizes") diff --git a/tests/browser/workspace_system_fixture.py b/tests/browser/workspace_system_fixture.py index 9900abc8..c7d87af3 100644 --- a/tests/browser/workspace_system_fixture.py +++ b/tests/browser/workspace_system_fixture.py @@ -35,7 +35,8 @@ read. These per-server hooks only change what a System test needs on top: previews for Convert outdated; - ``POST /__test__/system/jxl-encoder`` ``{available}`` (#317) makes the server read as having (``cjxl``) or lacking a JPEG XL encoder without - probing; reset restores the thumbnail settings and the real detection. + probing, whatever the host has installed (#371: ``false`` stands in for the + detector itself); reset restores the thumbnail settings and the real detection. - ``POST /__test__/system/duplicate-prompts`` (S7) adds two prompts whose text differs only in case and spacing (a duplicate prompt text group); @@ -118,6 +119,11 @@ def register_system_commands(app, routes, api, root: Path): } from py.api import thumbnail_encode + real_detect = thumbnail_encode._detect + + def _no_encoder(): + return None + monitor = getattr(image_monitor, "_monitor_instance", None) failures: dict[str, int] = {} @@ -147,6 +153,7 @@ def register_system_commands(app, routes, api, root: Path): api._health_report = None for (owner, name), value in host_values.items(): setattr(owner, name, value) + thumbnail_encode._detect = real_detect thumbnail_encode.reset_encoders() image_monitor._monitor_instance = monitor api.__dict__.pop("_library_resolver", None) @@ -298,7 +305,12 @@ def register_system_commands(app, routes, api, root: Path): available = data.get("available") if not isinstance(available, bool): raise web.HTTPBadRequest() - # Test-only: stand in the cached detection (no probe). + # Test-only: the fixture, not the host, decides (#371). Without an encoder the + # detector itself stands in, so a re-probe (a "none found" is probed again on + # every settings read) cannot find the host's real encoder; with one the cached + # answer stands, and a failed write still probes the real detector as before. + # Reset restores the real detector. + thumbnail_encode._detect = real_detect if available else _no_encoder with thumbnail_encode._detect_lock: thumbnail_encode._detected = "cjxl" if available else None return web.json_response({"success": True}) diff --git a/tests/disk_waits.py b/tests/disk_waits.py new file mode 100644 index 00000000..26a1f879 --- /dev/null +++ b/tests/disk_waits.py @@ -0,0 +1,16 @@ +"""Hang guards for waits on disk-bound work (tests only). + +A test that waits for a job or handoff whose work is file and SQLite I/O +(a safety backup with fsync, a restore, hashing and unlinking files) waits +on that state; the ceiling below only ends a real hang. Hosted Windows +runners stall disk I/O for tens of seconds at a time: a restore took over +10 s (merge queue, #382), and on PR #405 every test setup in a 50 s window +took up to 9 s against 0.1-0.6 s around it while the test bodies stayed +fast, so a one-file duplicate cleanup missed a 10 s bound (#409). + +The ceiling is per wait and stays under the suite's 120 s per-test hang +watchdog. A test with several such waits in a row can reach the watchdog +first; either way a real hang fails, the watchdog with every thread's stack. +""" + +DISK_BOUND_SECONDS = 90 diff --git a/tests/fixtures/README.md b/tests/fixtures/README.md index 25b61bc9..7e1476a2 100644 --- a/tests/fixtures/README.md +++ b/tests/fixtures/README.md @@ -113,3 +113,19 @@ by their own suites. The expectations start as the observed baseline. A change that alters an outcome edits the vector in the same commit. `tests/js/legacy-metadata-characterization.test.js` pins what the legacy viewers show on the same files. + +# Legacy contract snapshot + +`legacy_contract_snapshot.json` records what the legacy admin and gallery pages +talk to while System S13 (#330) moves their tools to native System: every +registered route as `" "` (`routes`) and the key paths of +`GET /prompt_manager/settings` to depth 2 (`settings_keys`; list items give the +union of their keys as `name[].key`). `tests/test_legacy_contract_snapshot.py` +compares a temp-database API against it. Record it again only for an intended +change, and say why in the PR body: + +```sh +PM_RECORD_LEGACY_SNAPSHOT=1 python3 -m unittest tests.test_legacy_contract_snapshot +``` + +Cutover (#334) removes the test and this fixture. diff --git a/tests/fixtures/legacy_contract_snapshot.json b/tests/fixtures/legacy_contract_snapshot.json new file mode 100644 index 00000000..50a48f53 --- /dev/null +++ b/tests/fixtures/legacy_contract_snapshot.json @@ -0,0 +1,200 @@ +{ + "routes": [ + "DELETE /prompt_manager/delete/{prompt_id}", + "DELETE /prompt_manager/image-annotations/capture/{token}", + "DELETE /prompt_manager/images/favorite", + "DELETE /prompt_manager/images/{image_id}", + "DELETE /prompt_manager/pairs/{pair_id}", + "DELETE /prompt_manager/prompts/{prompt_id}/tags", + "DELETE /prompt_manager/restore/preview/{token}", + "DELETE /prompt_manager/saved-views/{view_id}", + "DELETE /prompt_manager/tags/{tag_name}", + "GET /prompt_manager/admin", + "GET /prompt_manager/autotag/models", + "GET /prompt_manager/backup", + "GET /prompt_manager/backup/status", + "GET /prompt_manager/categories", + "GET /prompt_manager/diagnostics", + "GET /prompt_manager/export", + "GET /prompt_manager/gallery", + "GET /prompt_manager/gallery.html", + "GET /prompt_manager/gallery/subfolders", + "GET /prompt_manager/history/runs/{run_id}", + "GET /prompt_manager/history/runs/{run_id}/output/current/inspection", + "GET /prompt_manager/history/runs/{run_id}/output/current/media", + "GET /prompt_manager/history/runs/{run_id}/output/current/thumbnail", + "GET /prompt_manager/history/runs/{run_id}/outputs", + "GET /prompt_manager/image-annotations/capture/{token}", + "GET /prompt_manager/images/all", + "GET /prompt_manager/images/inspect/{image_path}", + "GET /prompt_manager/images/output", + "GET /prompt_manager/images/prompt/{image_path}", + "GET /prompt_manager/images/recent", + "GET /prompt_manager/images/search", + "GET /prompt_manager/images/serve/{filepath}", + "GET /prompt_manager/images/thumbnails/plan", + "GET /prompt_manager/images/thumbnails/status", + "GET /prompt_manager/images/{image_id}/file", + "GET /prompt_manager/jobs", + "GET /prompt_manager/jobs/{job_id}", + "GET /prompt_manager/jobs/{job_id}/events", + "GET /prompt_manager/jobs/{job_id}/report", + "GET /prompt_manager/js/{filepath}", + "GET /prompt_manager/lib/{filepath}", + "GET /prompt_manager/library", + "GET /prompt_manager/library/export", + "GET /prompt_manager/library/options", + "GET /prompt_manager/library/{positive_id}", + "GET /prompt_manager/library/{positive_id}/history", + "GET /prompt_manager/library/{positive_id}/history/pairs/{pair_id}/newest", + "GET /prompt_manager/library/{positive_id}/images", + "GET /prompt_manager/logs", + "GET /prompt_manager/logs/config", + "GET /prompt_manager/logs/download/{filename}", + "GET /prompt_manager/logs/files", + "GET /prompt_manager/logs/stats", + "GET /prompt_manager/lora/detect", + "GET /prompt_manager/lora/status", + "GET /prompt_manager/lora/trigger-words", + "GET /prompt_manager/maintenance/duplicates/{job_id}", + "GET /prompt_manager/pairs", + "GET /prompt_manager/pairs/export", + "GET /prompt_manager/pairs/{pair_id}", + "GET /prompt_manager/prompts/{prompt_id}/images", + "GET /prompt_manager/recent", + "GET /prompt_manager/review/events", + "GET /prompt_manager/review/items", + "GET /prompt_manager/review/items/{item_id}", + "GET /prompt_manager/saved-views", + "GET /prompt_manager/scan/status", + "GET /prompt_manager/scan_duplicates", + "GET /prompt_manager/scan_duplicates/status", + "GET /prompt_manager/scan_output_dir", + "GET /prompt_manager/search", + "GET /prompt_manager/settings", + "GET /prompt_manager/stats", + "GET /prompt_manager/subfolders", + "GET /prompt_manager/system/diagnostics", + "GET /prompt_manager/system/health", + "GET /prompt_manager/system/library", + "GET /prompt_manager/system/overview", + "GET /prompt_manager/system/statistics", + "GET /prompt_manager/tags", + "GET /prompt_manager/tags/filter", + "GET /prompt_manager/tags/stats", + "GET /prompt_manager/tags/suggest", + "GET /prompt_manager/tags/{tag_name}/prompts", + "GET /prompt_manager/test", + "GET /prompt_manager/vocabulary/categories", + "GET /prompt_manager/vocabulary/tags", + "GET /prompt_manager/vocabulary/tags/{tag_id}/impact", + "GET /prompt_manager/web", + "GET /prompt_manager/workspace", + "PATCH /prompt_manager/library/{positive_id}", + "PATCH /prompt_manager/pairs/{pair_id}", + "PATCH /prompt_manager/saved-views/{view_id}", + "PATCH /prompt_manager/vocabulary/tags/{tag_id}", + "POST /prompt_manager/autotag/apply", + "POST /prompt_manager/autotag/batch/preview", + "POST /prompt_manager/autotag/download/{model_type}", + "POST /prompt_manager/autotag/single", + "POST /prompt_manager/autotag/start", + "POST /prompt_manager/autotag/unload", + "POST /prompt_manager/bulk/category", + "POST /prompt_manager/bulk/delete", + "POST /prompt_manager/bulk/tags", + "POST /prompt_manager/cleanup", + "POST /prompt_manager/delete_duplicate_images", + "POST /prompt_manager/diagnostics/test-link", + "POST /prompt_manager/gallery/rescan", + "POST /prompt_manager/image-annotations/observe", + "POST /prompt_manager/image-annotations/suggest", + "POST /prompt_manager/images/clear-thumbnails", + "POST /prompt_manager/images/generate-thumbnails", + "POST /prompt_manager/images/generate-thumbnails/progress", + "POST /prompt_manager/images/link", + "POST /prompt_manager/images/reveal", + "POST /prompt_manager/jobs/autotag", + "POST /prompt_manager/jobs/duplicates", + "POST /prompt_manager/jobs/image-tags", + "POST /prompt_manager/jobs/model-download", + "POST /prompt_manager/jobs/scan", + "POST /prompt_manager/jobs/thumbnails", + "POST /prompt_manager/jobs/{job_id}/cancel", + "POST /prompt_manager/library", + "POST /prompt_manager/library/{positive_id}/variants", + "POST /prompt_manager/logs/config", + "POST /prompt_manager/logs/truncate", + "POST /prompt_manager/lora/enable", + "POST /prompt_manager/lora/import/apply", + "POST /prompt_manager/lora/import/preview", + "POST /prompt_manager/lora/refresh-cache", + "POST /prompt_manager/lora/scan", + "POST /prompt_manager/maintenance", + "POST /prompt_manager/maintenance/apply", + "POST /prompt_manager/maintenance/preview", + "POST /prompt_manager/pairs", + "POST /prompt_manager/prompts/tags", + "POST /prompt_manager/prompts/{prompt_id}/tags", + "POST /prompt_manager/restore", + "POST /prompt_manager/restore/apply", + "POST /prompt_manager/restore/preview", + "POST /prompt_manager/review/items/{item_id}/decision", + "POST /prompt_manager/save", + "POST /prompt_manager/saved-views", + "POST /prompt_manager/scan", + "POST /prompt_manager/scan/cancel", + "POST /prompt_manager/scan_duplicates/stream", + "POST /prompt_manager/settings", + "POST /prompt_manager/settings/validate-source", + "POST /prompt_manager/system/health", + "POST /prompt_manager/system/library/keep", + "POST /prompt_manager/system/library/move", + "POST /prompt_manager/tags/merge", + "POST /prompt_manager/vocabulary/categories/apply", + "POST /prompt_manager/vocabulary/categories/preview", + "POST /prompt_manager/vocabulary/tags", + "POST /prompt_manager/vocabulary/tags/{tag_id}/delete", + "PUT /prompt_manager/image-annotations/tags", + "PUT /prompt_manager/images/favorite", + "PUT /prompt_manager/prompts/{prompt_id}", + "PUT /prompt_manager/prompts/{prompt_id}/rating", + "PUT /prompt_manager/tags/{tag_name}" + ], + "settings_keys": [ + "monitor", + "monitor.observer_alive", + "monitor.present", + "monitor.scheduled", + "revision", + "settings", + "settings.autotag_character_threshold", + "settings.autotag_existing", + "settings.autotag_general_threshold", + "settings.autotag_model_type", + "settings.autotag_suggestion_limit", + "settings.autotag_template", + "settings.cpu_count", + "settings.gallery_root_path", + "settings.gallery_root_paths", + "settings.infinite_scroll", + "settings.monitored_directories", + "settings.monitoring_enabled", + "settings.result_timeout", + "settings.safety_backup", + "settings.thumbnail_edge", + "settings.thumbnail_format", + "settings.thumbnail_quality", + "settings.webui_display_mode", + "settings.worker_threads", + "sources", + "sources.auto_detected", + "sources.configured", + "sources.effective", + "success", + "thumbnail_encoders", + "thumbnail_encoders.jpeg", + "thumbnail_encoders.jxl", + "thumbnail_encoders.webp" + ] +} diff --git a/tests/js/workspace-images-state.test.js b/tests/js/workspace-images-state.test.js index 49cc0801..db74a8b6 100644 --- a/tests/js/workspace-images-state.test.js +++ b/tests/js/workspace-images-state.test.js @@ -234,6 +234,23 @@ test('the count line reads N of M images and K positive prompts only when known' assert.equal(countLine({loaded:1200,total:3400,positiveCount:0}),'1,200 of 3,400 images · 0 positive prompts'); }); +test('the count line reads as loading, never as an empty result, until a first page answers (#391)', async () => { + const {countLine,createImageStream}=await source; + assert.equal(countLine({loaded:0,total:0,positiveCount:null,loading:true,error:null}),'Loading images…'); + // A failed first page and a settled empty one say what the server said; a later page keeps the loaded count. + assert.equal(countLine({loaded:0,total:0,positiveCount:null,loading:false,error:new Error('x')}),'0 of 0 images'); + assert.equal(countLine({loaded:0,total:0,positiveCount:0,loading:false,error:null}),'0 of 0 images · 0 positive prompts'); + assert.equal(countLine({loaded:40,total:102,positiveCount:2,loading:true,error:null}),'40 of 102 images · 2 positive prompts'); + // While a reset's first page is in flight the line reads as loading (the cleared state before it is replaced in + // the same task, so it is never painted). + let answer;const lines=[]; + const stream=createImageStream({api:{get:()=>new Promise(resolve=>{answer=resolve;})},onChange:state=>lines.push(countLine(state))}); + const loading=stream.reset({tags:['cinematic']}); + assert.equal(lines.at(-1),'Loading images…'); + answer({images:[{path:'a.png',root_index:0}],total:1,has_more:false,next_cursor:null,positive_count:1});await loading; + assert.equal(lines.at(-1),'1 of 1 image · 1 positive prompt'); +}); + test('the caption date names the newest result day under Newest first only', async () => { const {captionDate}=await source; const now=new Date(2026,9,17,15,0); diff --git a/tests/js/workspace-system-diagnostics.test.js b/tests/js/workspace-system-diagnostics.test.js index 65b0bddb..161c1bfa 100644 --- a/tests/js/workspace-system-diagnostics.test.js +++ b/tests/js/workspace-system-diagnostics.test.js @@ -175,6 +175,28 @@ test('Clear all logs states both counts and its toast says what was done', async assert.equal(clearToast(undefined), 'Logs cleared · 0 rotated copies deleted.'); }); +test('Clear all logs sends back exactly the files its confirmation showed (#389)', async () => { + const { clearBody, clearFailureText, COPY } = await diagnostics; + const files = [ + { filename: 'prompt_manager.log', kind: 'active', identity: '10:1761000000123456789:9007199254740993' }, + { filename: 'prompt_manager.log.1', kind: 'rotated', identity: '20:1761000000123456789:42' }, + { filename: 'prompt_manager.log.4', kind: 'link', identity: '5:1:7' }, + { filename: 'prompt_manager.log.bak', kind: 'other', identity: '1:1:8' }, + { filename: 'prompt_manager.log.2', kind: 'rotated' }, + ]; + // Identities stay strings: mtime_ns and large inodes are not exact JS numbers. + assert.deepEqual(clearBody(files), { expected: [ + { name: 'prompt_manager.log', identity: '10:1761000000123456789:9007199254740993' }, + { name: 'prompt_manager.log.1', identity: '20:1761000000123456789:42' }, + ] }); + assert.deepEqual(clearBody([]), { expected: [] }); + assert.deepEqual(clearBody(null), {}); + const refused = Object.assign(new Error('The log files changed since you confirmed; nothing was cleared.'), { status: 409 }); + assert.equal(clearFailureText(refused), refused.message); + assert.equal(clearFailureText(Object.assign(new Error('boom'), { status: 500 })), COPY.clearFailed); + assert.equal(clearFailureText(undefined), COPY.clearFailed); +}); + test('the test-link dialog names the prompt and how Missing-file links removes the record', async () => { const { testLinkSentence, promptName } = await diagnostics; assert.equal(testLinkSentence({ id: 812, text: 'Misty harbor' }), 'Writes a placeholder image record ' diff --git a/tests/private_temp.py b/tests/private_temp.py new file mode 100644 index 00000000..dee79ce9 --- /dev/null +++ b/tests/private_temp.py @@ -0,0 +1,24 @@ +"""A temp SQLite database in a folder no other test process shares (#346). + +``PromptManagerAPI`` sweeps its database's folder at startup for the stages, +uploads and safety-backup copies an interrupted restore or backup left. A +fixture database directly in the system temp folder made every other test +process that started an API (another worktree's suite, a pre-push hook) delete +this process's in-flight restore files, and left safety backups behind there. +""" + +import tempfile + + +def private_temp_db(case): + """Return a closed ``NamedTemporaryFile`` handle for an empty ``.db`` file. + + The file lives in a new folder that ``case`` removes at cleanup, after its + own teardown has closed and unlinked the database (cleanups run after + ``tearDown``), together with anything a restore or backup left next to it. + """ + folder = tempfile.TemporaryDirectory(ignore_cleanup_errors=True) + case.addCleanup(folder.cleanup) + handle = tempfile.NamedTemporaryFile(delete=False, suffix=".db", dir=folder.name) + handle.close() + return handle diff --git a/tests/test_api.py b/tests/test_api.py index 051881a8..efcb433a 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -8,7 +8,6 @@ without requiring a full ComfyUI server. import json import os import sys -import tempfile import unittest sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) @@ -18,6 +17,7 @@ from aiohttp.test_utils import AioHTTPTestCase from database.operations import PromptDatabase from py.api import PromptManagerAPI +from tests.private_temp import private_temp_db from utils.hashing import generate_prompt_hash @@ -25,8 +25,7 @@ class APITestCase(AioHTTPTestCase): """Base class that stands up a test aiohttp app with PromptManager routes.""" async def get_application(self): - self._temp_db = tempfile.NamedTemporaryFile(delete=False, suffix=".db") - self._temp_db.close() + self._temp_db = private_temp_db(self) app = web.Application() routes = web.RouteTableDef() diff --git a/tests/test_api_autotag.py b/tests/test_api_autotag.py index e2d2dffc..cfe3f47a 100644 --- a/tests/test_api_autotag.py +++ b/tests/test_api_autotag.py @@ -35,6 +35,7 @@ from aiohttp.test_utils import AioHTTPTestCase from database.operations import PromptDatabase from py.api import PromptManagerAPI +from tests.private_temp import private_temp_db from py.api.autotag_routes import MODEL_BUSY_ERROR from py.api.jobs import JobBusy from utils.hashing import generate_prompt_hash @@ -143,8 +144,7 @@ class AutotagAPITestCase(AioHTTPTestCase): fake engine.""" async def get_application(self): - self._temp_db = tempfile.NamedTemporaryFile(delete=False, suffix=".db") - self._temp_db.close() + self._temp_db = private_temp_db(self) self.tmp_root = Path(tempfile.mkdtemp()) self.output_dir = self.tmp_root / "output" self.output_dir.mkdir() diff --git a/tests/test_api_duplicate_files.py b/tests/test_api_duplicate_files.py index 91bd0ea3..9c78ba4a 100644 --- a/tests/test_api_duplicate_files.py +++ b/tests/test_api_duplicate_files.py @@ -18,15 +18,19 @@ from pathlib import Path from unittest.mock import patch try: + from tests.disk_waits import DISK_BOUND_SECONDS from tests.open_handles import open_handles_under from tests.test_api_maintenance import ApplyTestCase, _digest except ImportError: # discovered with tests/ as the top-level directory + from disk_waits import DISK_BOUND_SECONDS from open_handles import open_handles_under from test_api_maintenance import ApplyTestCase, _digest from py.api import duplicate_files # noqa: E402 from py.config import GalleryConfig, PromptManagerConfig # noqa: E402 +# Gates and server-state polls. Waits on a find or cleanup job's task use +# DISK_BOUND_SECONDS: those jobs hash, back up and unlink files (#409). WAIT = 10 FIND = "/prompt_manager/jobs/duplicates" GROUPS = "/prompt_manager/maintenance/duplicates/" @@ -83,7 +87,7 @@ class DuplicateFilesTestCase(ApplyTestCase): self.assertEqual(response.status, 202, text) self.assertNotIn(self.tmpdir, text) job = self.api._jobs.get(json.loads(text)["job"]["id"]) - await asyncio.wait_for(asyncio.shield(job.task), WAIT) + await asyncio.wait_for(asyncio.shield(job.task), DISK_BOUND_SECONDS) return job async def groups(self, job_id, status=200, **query): @@ -127,7 +131,7 @@ class DuplicateFilesTestCase(ApplyTestCase): plan = await self.review(job.id, [self.keep_of(group, path)]) body = await self.apply(plan["token"]) cleanup = self.api._jobs.get(body["job"]["id"]) - await asyncio.wait_for(asyncio.shield(cleanup.task), WAIT) + await asyncio.wait_for(asyncio.shield(cleanup.task), DISK_BOUND_SECONDS) return plan, cleanup async def until(self, predicate): @@ -178,7 +182,7 @@ class TestFindJob(DuplicateFilesTestCase): gate.set() body = await (await legacy).text() job = self.api._jobs.get(summary["id"]) - await asyncio.wait_for(asyncio.shield(job.task), WAIT) + await asyncio.wait_for(asyncio.shield(job.task), DISK_BOUND_SECONDS) events = _sse_payloads(body) done = events[-1] self.assertEqual(done["type"], "complete") @@ -241,7 +245,7 @@ class TestFindJob(DuplicateFilesTestCase): self.assertTrue((await cancel.json())["cancellation_requested"]) gate.set() job = self.api._jobs.get(job_id) - await asyncio.wait_for(asyncio.shield(job.task), WAIT) + await asyncio.wait_for(asyncio.shield(job.task), DISK_BOUND_SECONDS) self.assertEqual(job.state, "cancelled") last = _sse_payloads(await (await stream).text())[-1] self.assertEqual(last, {"type": "error", "message": "Duplicate scan cancelled"}) @@ -341,6 +345,8 @@ class TestReview(DuplicateFilesTestCase): self.assertEqual(plan["protected"], {"count": 0, "reasons": []}) async def test_cannot_delete_keeper_or_outside_group(self): + # About which files may go, not the backup: skip its fsync-heavy copy. + PromptManagerConfig.SAFETY_BACKUP = False self.trio() self.write("e.png", b"other pair") self.write("f.png", b"other pair") @@ -372,7 +378,7 @@ class TestReview(DuplicateFilesTestCase): self.assertEqual([row["label"] for row in plan["details"]], ["b.png"]) body = await self.apply(plan["token"]) cleanup = self.api._jobs.get(body["job"]["id"]) - await asyncio.wait_for(asyncio.shield(cleanup.task), WAIT) + await asyncio.wait_for(asyncio.shield(cleanup.task), DISK_BOUND_SECONDS) self.assertEqual(cleanup.counters["removed"], 1) self.assertTrue((self.output_dir / "a.png").is_file()) self.assertTrue((self.output_dir / "sub" / "c.png").is_file()) @@ -496,7 +502,7 @@ class TestCleanupJob(DuplicateFilesTestCase): plan = await self.review(job.id, keep) body = await self.apply(plan["token"]) cleanup = self.api._jobs.get(body["job"]["id"]) - await asyncio.wait_for(asyncio.shield(cleanup.task), WAIT) + await asyncio.wait_for(asyncio.shield(cleanup.task), DISK_BOUND_SECONDS) self.assertTrue(kept.is_file()) self.assertFalse(gone.exists()) self.assertFalse(video_thumb.exists()) @@ -508,7 +514,7 @@ class TestCleanupJob(DuplicateFilesTestCase): self.write("b.png", b"new bytes") # changed since the review body = await self.apply(plan["token"]) cleanup = self.api._jobs.get(body["job"]["id"]) - await asyncio.wait_for(asyncio.shield(cleanup.task), WAIT) + await asyncio.wait_for(asyncio.shield(cleanup.task), DISK_BOUND_SECONDS) self.assertEqual(cleanup.counters, {"removed": 1, "kept": 2, "errors": 0}) self.assertEqual((self.output_dir / "b.png").read_bytes(), b"new bytes") self.assertFalse((self.output_dir / "sub" / "c.png").exists()) @@ -522,7 +528,7 @@ class TestCleanupJob(DuplicateFilesTestCase): self.write("a.png", b"edited keeper") body = await self.apply(plan["token"]) cleanup = self.api._jobs.get(body["job"]["id"]) - await asyncio.wait_for(asyncio.shield(cleanup.task), WAIT) + await asyncio.wait_for(asyncio.shield(cleanup.task), DISK_BOUND_SECONDS) self.assertEqual(cleanup.counters["removed"], 0) self.assertTrue((self.output_dir / "b.png").is_file()) self.assertTrue((self.output_dir / "sub" / "c.png").is_file()) @@ -549,7 +555,7 @@ class TestCleanupJob(DuplicateFilesTestCase): with patch.object(duplicate_files, "_unlink", side_effect=unlink): body = await self.apply(plan["token"]) cleanup = self.api._jobs.get(body["job"]["id"]) - await asyncio.wait_for(asyncio.shield(cleanup.task), WAIT) + await asyncio.wait_for(asyncio.shield(cleanup.task), DISK_BOUND_SECONDS) self.assertEqual(cleanup.state, "failed") self.assertEqual(cleanup.counters, {"removed": 1, "kept": 1, "errors": 1}) self.assertEqual( @@ -584,7 +590,7 @@ class TestCleanupJob(DuplicateFilesTestCase): ): body = await self.apply(plan["token"]) cleanup = self.api._jobs.get(body["job"]["id"]) - await asyncio.wait_for(asyncio.shield(cleanup.task), WAIT) + await asyncio.wait_for(asyncio.shield(cleanup.task), DISK_BOUND_SECONDS) self.assertEqual(cleanup.counters, {"removed": 0, "kept": 3, "errors": 1}) self.assertEqual( cleanup.outcomes["errors"][0]["detail"], duplicate_files.DELETE_FAILED @@ -619,7 +625,7 @@ class TestCleanupJob(DuplicateFilesTestCase): with context: body = await self.apply(plan["token"]) cleanup = self.api._jobs.get(body["job"]["id"]) - await asyncio.wait_for(asyncio.shield(cleanup.task), WAIT) + await asyncio.wait_for(asyncio.shield(cleanup.task), DISK_BOUND_SECONDS) finally: if handle is not None: # pragma: no cover - Windows handle.close() @@ -642,7 +648,7 @@ class TestCleanupJob(DuplicateFilesTestCase): with patch.object(duplicate_files, "_unlink", side_effect=unlink): body = await self.apply(plan["token"]) cleanup = self.api._jobs.get(body["job"]["id"]) - await asyncio.wait_for(asyncio.shield(cleanup.task), WAIT) + await asyncio.wait_for(asyncio.shield(cleanup.task), DISK_BOUND_SECONDS) self.assertEqual(cleanup.counters["removed"], 2) self.assertEqual(open_at_unlink, []) @@ -703,7 +709,7 @@ class TestCleanupJob(DuplicateFilesTestCase): with patch.object(duplicate_files, "_unlink", side_effect=unlink): body = await self.apply(plan["token"]) cleanup = self.api._jobs.get(body["job"]["id"]) - await asyncio.wait_for(asyncio.shield(cleanup.task), WAIT) + await asyncio.wait_for(asyncio.shield(cleanup.task), DISK_BOUND_SECONDS) # Its records moved to the identical kept copy before the unlink failed. [image] = self.api.db.get_prompt_images(prompt) self.assertTrue(image["image_path"].endswith("a.png")) @@ -719,7 +725,7 @@ class TestCleanupJob(DuplicateFilesTestCase): self.assertEqual((busy["code"], busy["running"]["id"]), ("busy", find["id"])) gate.set() await asyncio.wait_for( - asyncio.shield(self.api._jobs.get(find["id"]).task), WAIT + asyncio.shield(self.api._jobs.get(find["id"]).task), DISK_BOUND_SECONDS ) # The token survived the refusal; the cleanup holds off a new find. release = asyncio.Event() @@ -737,7 +743,7 @@ class TestCleanupJob(DuplicateFilesTestCase): self.assertEqual(legacy.status, 409) release.set() cleanup = self.api._jobs.get(body["job"]["id"]) - await asyncio.wait_for(asyncio.shield(cleanup.task), WAIT) + await asyncio.wait_for(asyncio.shield(cleanup.task), DISK_BOUND_SECONDS) async def test_cleanup_refused_up_front_when_the_backup_does_not_fit(self): from py.api import maintenance @@ -965,7 +971,7 @@ class TestReviewHelpers(DuplicateFilesTestCase): GalleryConfig.MONITORING_DIRECTORIES = [self._root(self.second_source())] body = await self.apply(plan["token"]) cleanup = self.api._jobs.get(body["job"]["id"]) - await asyncio.wait_for(asyncio.shield(cleanup.task), WAIT) + await asyncio.wait_for(asyncio.shield(cleanup.task), DISK_BOUND_SECONDS) self.assertEqual(cleanup.state, "failed") self.assertEqual(cleanup.last_event["code"], "sources-changed") self.assertEqual(self.backups(), []) @@ -985,7 +991,7 @@ class TestReviewHelpers(DuplicateFilesTestCase): ): body = await self.apply(plan["token"]) cleanup = self.api._jobs.get(body["job"]["id"]) - await asyncio.wait_for(asyncio.shield(cleanup.task), WAIT) + await asyncio.wait_for(asyncio.shield(cleanup.task), DISK_BOUND_SECONDS) self.assertEqual(cleanup.state, "failed") self.assertEqual( cleanup.outcomes["errors"][-1]["detail"], duplicate_files.RECORDS_FAILED diff --git a/tests/test_api_duplicate_safety.py b/tests/test_api_duplicate_safety.py index 02115143..8ae50edb 100644 --- a/tests/test_api_duplicate_safety.py +++ b/tests/test_api_duplicate_safety.py @@ -19,17 +19,17 @@ from pathlib import Path from unittest.mock import patch try: + from tests.disk_waits import DISK_BOUND_SECONDS from tests.open_handles import open_handles_under from tests import test_api_duplicate_files as dup_tests except ImportError: # discovered with tests/ as the top-level directory import test_api_duplicate_files as dup_tests + from disk_waits import DISK_BOUND_SECONDS from open_handles import open_handles_under from py.api import duplicate_files # noqa: E402 from py.config import GalleryConfig # noqa: E402 -WAIT = 10 - def _locator(path): return os.path.normcase(os.path.realpath(str(path))) @@ -49,7 +49,7 @@ class SafetyTestCase(dup_tests.DuplicateFilesTestCase): async def apply_plan(self, plan): body = await self.apply(plan["token"]) cleanup = self.api._jobs.get(body["job"]["id"]) - await asyncio.wait_for(asyncio.shield(cleanup.task), WAIT) + await asyncio.wait_for(asyncio.shield(cleanup.task), DISK_BOUND_SECONDS) return cleanup diff --git a/tests/test_api_history.py b/tests/test_api_history.py index 67476d7a..dc952332 100644 --- a/tests/test_api_history.py +++ b/tests/test_api_history.py @@ -1169,3 +1169,47 @@ class HistoryHostCleanupTests(unittest.IsolatedAsyncioTestCase): fence.validate(epoch, generation) with self.assertRaises(sqlite3.Error): api._history_fence() + + +class SharedTempFolderRestoreTests(ImageAPITestCase): + """#346: a restore survives another suite starting in the shared temp folder. + + Every ``PromptManagerAPI`` sweeps its database's folder for stages and + uploads an interrupted restore left. A fixture database directly in the + shared temp folder let any other test process (another worktree's suite, + a pre-push hook) delete this restore's files mid-request. The shared + folder here is a scratch stand-in for the system temp folder, so the test + never sweeps the real one. + """ + + async def get_application(self): + shared = tempfile.TemporaryDirectory(ignore_cleanup_errors=True) + # Cleanups run last-in first-out: this one after the fixture's own. + self.addCleanup(shared.cleanup) + self.shared_tmp = shared.name + with patch.object(tempfile, "tempdir", shared.name): + return await super().get_application() + + async def test_restore_survives_another_suite_sweeping_the_shared_folder(self): + from database import restore_stage + from database.models import PromptModel + + self.api.db.save_prompt("kept by the upload") + source = self.comfy_dir / "upload.db" + self.assertTrue(self.api.db.model.backup_database(str(source))) + stage = PromptModel.stage_restore + swept = [] + + def stage_then_other_suite_starts(model, src_path): + staged = stage(model, src_path) + # What another suite's PromptManagerAPI does at startup. + other = os.path.join(self.shared_tmp, "other-suite.db") + swept.extend(restore_stage.sweep_restore_temps(other)) + return staged + + form = FormData() + form.add_field("database_file", source.read_bytes(), filename="upload.db") + with patch.object(PromptModel, "stage_restore", stage_then_other_suite_starts): + response = await self.client.post("/prompt_manager/restore", data=form) + self.assertEqual(response.status, 200, await response.text()) + self.assertEqual(swept, [], "another suite's sweep reached this restore") diff --git a/tests/test_api_images.py b/tests/test_api_images.py index 08713ddb..cc74c6cf 100644 --- a/tests/test_api_images.py +++ b/tests/test_api_images.py @@ -42,6 +42,7 @@ from py import lora_utils # noqa: E402 from py import lora_manager_layout as layout_module # noqa: E402 from py.api.thumbnail_settings import ThumbnailSettings # noqa: E402 from py.api import PromptManagerAPI # noqa: E402 +from tests.private_temp import private_temp_db # noqa: E402 from py.api.prompts import MAX_PAGE_LIMIT # noqa: E402 from py.config import GalleryConfig # noqa: E402 from utils.hashing import generate_prompt_hash # noqa: E402 @@ -60,8 +61,7 @@ class ImageAPITestCase(AioHTTPTestCase): """App with PromptManager routes, a temp DB and a temp output directory.""" async def get_application(self): - self._temp_db = tempfile.NamedTemporaryFile(delete=False, suffix=".db") - self._temp_db.close() + self._temp_db = private_temp_db(self) self._output_tmp = tempfile.TemporaryDirectory(ignore_cleanup_errors=True) # A fake ComfyUI tree: /output is the gallery root and is the diff --git a/tests/test_api_log_cleanup.py b/tests/test_api_log_cleanup.py index d9287088..9622e599 100644 --- a/tests/test_api_log_cleanup.py +++ b/tests/test_api_log_cleanup.py @@ -415,5 +415,74 @@ class TestApply(LogCleanupTestCase): self.assertEqual(job.status_line, "The log cleanup failed.") +TRUNCATE = "/prompt_manager/logs/truncate" +FILES = "/prompt_manager/logs/files" + + +class TestClearAllLogsConfirmed(LogCleanupTestCase): + """#389: POST /logs/truncate deletes no more than the confirmation listed.""" + + async def confirmed(self): + response = await self.client.request("GET", FILES) + data = await response.json() + return [ + {"name": row["filename"], "identity": row["identity"]} + for row in data["files"] + if row["kind"] in ("active", "rotated") + ] + + def handler(self): + return next( + h + for h in self.manager.logger.handlers + if isinstance(h, logging.FileHandler) + ) + + async def test_a_rollover_after_the_confirmation_is_refused(self): + self.write("prompt_manager.log.1", b"a" * 100) + self.write("prompt_manager.log.2", b"b" * 200) + expected = await self.confirmed() + self.app_logger.info("written after the confirmation") + self.handler().doRollover() + before = self.snapshot() + body = await self.post(TRUNCATE, {"expected": expected}, 409) + self.assertEqual(body["success"], False) + self.assertEqual(body["code"], "logs-changed") + self.assertIn("nothing was cleared", body["error"]) + self.assertEqual(self.snapshot(), before) + + async def test_the_confirmed_files_are_cleared(self): + self.write("prompt_manager.log.1", b"a" * 100) + expected = await self.confirmed() + body = await self.post(TRUNCATE, {"expected": expected}, 200) + self.assertEqual( + sorted(body["results"]["truncated"]), + ["prompt_manager.log", "prompt_manager.log.1"], + ) + self.assertEqual(self.names(), ["prompt_manager.log"]) + + async def test_the_legacy_call_without_a_body_still_clears(self): + self.write("prompt_manager.log.1", b"a" * 100) + response = await self.client.request("POST", TRUNCATE) + self.assertEqual(response.status, 200, await response.text()) + self.assertEqual(self.names(), ["prompt_manager.log"]) + + async def test_a_foreign_writer_is_refused_with_a_clear_message(self): + self.write("prompt_manager.log.1", b"a" * 100) + with open(self.path("prompt_manager.log"), "ab") as other: + other.write(b"line from another ComfyUI\n") + body = await self.post(TRUNCATE, {"expected": await self.confirmed()}, 409) + self.assertEqual(body["code"], "logs-foreign-writer") + self.assertIn("Another program is writing", body["error"]) + self.assertIn("prompt_manager.log.1", self.names()) + + async def test_a_malformed_confirmation_is_400(self): + self.write("prompt_manager.log.1", b"a" * 100) + for expected in ("all", [{"name": "prompt_manager.log.1", "identity": 5}]): + with self.subTest(expected=expected): + await self.post(TRUNCATE, {"expected": expected}, 400) + self.assertIn("prompt_manager.log.1", self.names()) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_api_lora.py b/tests/test_api_lora.py index 8fda0562..99446360 100644 --- a/tests/test_api_lora.py +++ b/tests/test_api_lora.py @@ -36,6 +36,7 @@ from aiohttp.test_utils import AioHTTPTestCase from database.operations import PromptDatabase from py.api import PromptManagerAPI +from tests.private_temp import private_temp_db from py import config_store, lora_import, lora_utils from py.config import IntegrationConfig, PromptManagerConfig from py.api import settings_routes as settings_module @@ -57,8 +58,7 @@ class LoraAPITestCase(AioHTTPTestCase): """Stands up the PromptManager routes with a temp DB and neutral LoRA config.""" async def get_application(self): - self._temp_db = tempfile.NamedTemporaryFile(delete=False, suffix=".db") - self._temp_db.close() + self._temp_db = private_temp_db(self) self.tmp_root = Path(tempfile.mkdtemp()) self.custom_nodes = self.tmp_root / "custom_nodes" self.custom_nodes.mkdir() diff --git a/tests/test_api_prompts.py b/tests/test_api_prompts.py index c4af9c68..c8625c25 100644 --- a/tests/test_api_prompts.py +++ b/tests/test_api_prompts.py @@ -5,7 +5,6 @@ Route tests for the prompt API (py/api/prompts.py). import json import os import sys -import tempfile import unittest from unittest.mock import MagicMock, patch @@ -29,6 +28,7 @@ except ImportError: # discovered with tests/ as the top-level directory from database.operations import PromptDatabase # noqa: E402 from py.api import PromptManagerAPI # noqa: E402 +from tests.private_temp import private_temp_db # noqa: E402 import py.api.prompts as prompts_module # noqa: E402 from py.api.prompts import ( # noqa: E402 BULK_CATEGORY_MAX_PROMPTS, @@ -43,8 +43,7 @@ class PromptAPITestCase(AioHTTPTestCase): """App with PromptManager routes and a temporary SQLite database.""" async def get_application(self): - self._temp_db = tempfile.NamedTemporaryFile(delete=False, suffix=".db") - self._temp_db.close() + self._temp_db = private_temp_db(self) app = web.Application() routes = web.RouteTableDef() diff --git a/tests/test_api_review.py b/tests/test_api_review.py index 6eeaa89d..0e19c6ed 100644 --- a/tests/test_api_review.py +++ b/tests/test_api_review.py @@ -1040,8 +1040,8 @@ class TestApplyRefusals(ReviewRoutesTestCase): _path, item, detail, observed, cat = await self._opened() real = review_capture_module.resolve_prompt_pair - def reworded(metadata): - resolution = real(metadata) + def reworded(metadata, **kwargs): + resolution = real(metadata, **kwargs) return { **resolution, "reason": "a reworded reason from a newer resolver", @@ -1065,8 +1065,8 @@ class TestApplyRefusals(ReviewRoutesTestCase): _path, item, detail, observed, cat = await self._opened() real = review_capture_module.resolve_prompt_pair - def without_cat(metadata): - resolution = real(metadata) + def without_cat(metadata, **kwargs): + resolution = real(metadata, **kwargs) kept = [ c for c in resolution["candidates"] diff --git a/tests/test_execution_history_writer.py b/tests/test_execution_history_writer.py index 1d93dcbd..c0b11633 100644 --- a/tests/test_execution_history_writer.py +++ b/tests/test_execution_history_writer.py @@ -31,6 +31,12 @@ from utils.execution_history_writer import HistoryWriter # 0.19 s on macOS py3.13 (PR #351). NON_BLOCKING_CEILING = 1.0 +# Saturation tests block the worker while they queue 1023 more items. The +# block must hold until the test releases it (#346/#352): a 3 s timer let a +# loaded runner unblock the worker mid-fill and drain it. This is only a hang +# guard; the test, or cleanup, always sets the release. +SATURATION_HOLD_SECONDS = 60 + class ManualClock: def __init__(self): @@ -84,6 +90,26 @@ class HistoryWriterTests(unittest.TestCase): threading.Event().wait(0.005) self.fail("finite deadline expired waiting for worker behavior") + def wait_for_worker_pass(self, writer): + """Wait until the idle worker has run one whole loop pass from now. + + A worker with no accepted work ends every pass by parking on its wake + event. The first park counted may end a pass already under way; the + second ends a pass that began after this call, so whatever the worker + decides at the current clock value has happened (state, not a + wall-clock pause that a loaded runner can outlast). + """ + wake = writer._wake + park = wake.wait + parks = [] + + def counted(timeout=None): + parks.append(timeout) + return park(timeout) + + with patch.object(wake, "wait", counted): + self.wait_for(lambda: len(parks) >= 2) + def writer(self, factory=None, ready=True): writer = HistoryWriter(factory or (lambda: self.db), clock=self.clock) self.writers.append(writer) @@ -301,7 +327,7 @@ class HistoryWriterTests(unittest.TestCase): def write(command): entered.set() - release.wait(3) + release.wait(SATURATION_HOLD_SECONDS) return original(command) with patch.object(self.db, "write_execution_start", write): @@ -348,9 +374,12 @@ class HistoryWriterTests(unittest.TestCase): first = self.sealed(writer) charge = first.lease.byte_count + left = threading.Event() + def write(*args, **kwargs): entered.set() release.wait(3) + left.set() return original(*args, **kwargs) with patch.object(self.db, "write_execution_contexts_for_finish", write): @@ -396,8 +425,21 @@ class HistoryWriterTests(unittest.TestCase): before = monotonic() self.assertFalse(writer.close(0.02)) elapsed = monotonic() - before + # close() waits until its own monotonic deadline, so the lower + # bound holds on any runner. self.assertGreaterEqual(elapsed, 0.02) - self.assertLess(elapsed, 0.2) + if early_join: + # Manual clock: only the patched join/sleep advance it, so this + # exact bound proves early joins do not spin forever. + self.assertLess(elapsed, 0.2) + else: + # Real clock: a loaded runner may run late, but close() must + # not wait anywhere near the 3 s the blocked write holds. + self.assertLess(elapsed, 0.02 + NON_BLOCKING_CEILING) + # Bounded on the real clock too: close() gave up while the + # in-flight write was still blocked, instead of waiting it out. + self.assertFalse(left.is_set()) + self.assertTrue(writer.status().alive) self.assertEqual(pool.charged_bytes, charge) replacement = HistoryWriter(lambda: self.db, context_budget=pool) self.writers.append(replacement) @@ -771,7 +813,7 @@ class HistoryWriterTests(unittest.TestCase): self.wait_for(lambda: not writer.status().accepting) self.assertFalse(writer.submit(fresh)) self.clock.advance(4.99) - threading.Event().wait(0.08) + self.wait_for_worker_pass(writer) self.assertEqual(probes, [0]) self.clock.advance(0.01) self.wait_for(lambda: writer.status().accepting) @@ -794,12 +836,12 @@ class HistoryWriterTests(unittest.TestCase): writer = self.writer(factory, ready=False) self.wait_for(lambda: len(calls) == 1) self.clock.advance(4.99) - threading.Event().wait(0.08) + self.wait_for_worker_pass(writer) self.assertEqual(calls, [0]) self.clock.advance(0.01) self.wait_for(lambda: len(calls) == 2) self.clock.advance(4.99) - threading.Event().wait(0.08) + self.wait_for_worker_pass(writer) self.assertEqual(calls, [0, 5]) self.clock.advance(0.01) self.wait_for(lambda: writer.status().accepting) @@ -853,7 +895,7 @@ class HistoryWriterTests(unittest.TestCase): ) writer.invalidate() release.set() - threading.Event().wait(0.08) + self.wait_for_worker_pass(writer) self.assertFalse(writer.status().accepting) self.assertIsNone(writer.status().epoch) self.assertFalse(writer.submit(self.start_command(epoch=stale_epoch))) @@ -871,7 +913,7 @@ class HistoryWriterTests(unittest.TestCase): lock.execute("BEGIN IMMEDIATE") before = time.monotonic() self.assertTrue(writer.submit(command)) - self.assertLess(time.monotonic() - before, 0.1) + self.assertLess(time.monotonic() - before, NON_BLOCKING_CEILING) self.wait_for(lambda: not writer.status().accepting) lock.rollback() self.clock.advance(5) diff --git a/tests/test_execution_output_writer.py b/tests/test_execution_output_writer.py index 06475d24..b274e535 100644 --- a/tests/test_execution_output_writer.py +++ b/tests/test_execution_output_writer.py @@ -18,6 +18,7 @@ from utils.execution_output_capture import seal_outputs from utils.execution_outputs import snapshot_output_roots from utils.execution_history_writer import HistoryWriter from tests.history_waits import drain_writer +from tests.disk_waits import DISK_BOUND_SECONDS try: from tests import test_execution_history_writer as fixtures @@ -28,11 +29,17 @@ except ImportError: # Upper bound a blocked fake write waits for its release. Tests prove ordering # with events, so this only caps a failing run and never decides a pass. BLOCK_SECONDS = 10 + +# Saturation tests block the worker while they queue 1023 more items. The +# block must hold until the test releases it (#346/#352): a 3 s timer let a +# loaded runner unblock the worker mid-fill and drain it. This is only a hang +# guard; the test, or cleanup, always sets the release. +SATURATION_HOLD_SECONDS = 60 + # A restore is a full verify/migrate/backup/replace cycle and is bound by the # disk: a busy Windows runner took over 10 s (merge queue, #382). Waits that -# span a restore use this ceiling, which stays under the runner's 120 s hang -# watchdog, so a real hang still fails with the thread dump. -RESTORE_SECONDS = 90 +# span a restore use the shared disk-bound hang guard (tests/disk_waits.py). +RESTORE_SECONDS = DISK_BOUND_SECONDS class OutputWriterTests(fixtures.unittest.TestCase): @@ -591,7 +598,7 @@ class OutputWriterTests(fixtures.unittest.TestCase): def write(command): entered.set() - release.wait(3) + release.wait(SATURATION_HOLD_SECONDS) return original(command) with patch.object(self.db, "write_execution_start", write): diff --git a/tests/test_exit_window_threads.py b/tests/test_exit_window_threads.py new file mode 100644 index 00000000..1323f4db --- /dev/null +++ b/tests/test_exit_window_threads.py @@ -0,0 +1,266 @@ +"""A thread or pool refused at interpreter shutdown is quiet (#375). + +When the interpreter exits, ``threading._shutdown`` first shuts every +``ThreadPoolExecutor`` (``submit`` then raises "cannot schedule new futures +after interpreter shutdown") and joins the non-daemon threads; only then do +the ``atexit`` callbacks (the package teardown) run. Our daemon threads keep +working meanwhile. ``Thread.start`` is refused ("can't create new thread at +interpreter shutdown") only later, once the interpreter is finalizing; the +unit tests cover that case, the child process the pool case. Such a refusal +is logged once at DEBUG, with no traceback, and leaves nothing pending. + +``threading.Thread`` is replaced only in the module under test (a copy of +the ``threading`` namespace), never process-wide. The child-process test runs +the real exit: a daemon thread queues previews while the main thread returns. +""" + +import json +import logging +import os +import subprocess +import sys +import tempfile +import textwrap +import threading +import types +import unittest +from concurrent.futures import ThreadPoolExecutor +from pathlib import Path +from unittest import mock + +from py.api import thumbnail_auto +from utils import image_monitor, parallel + +ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +CHILD_CEILING_SECONDS = 60 # the hang watchdog allows 120 s per test +REPORT_PREFIX = "PM_EXIT_REPORT " +THREAD_REFUSAL = "can't create new thread at interpreter shutdown" +POOL_REFUSAL = "cannot schedule new futures after interpreter shutdown" + + +def _refusing_threading(): + """A ``threading`` namespace whose ``Thread.start`` is refused.""" + + class RefusedThread(threading.Thread): + def start(self): + raise RuntimeError(THREAD_REFUSAL) + + namespace = types.SimpleNamespace(**vars(threading)) + namespace.Thread = RefusedThread + return namespace + + +class _ShutPool(ThreadPoolExecutor): + def submit(self, *args, **kwargs): + raise RuntimeError(POOL_REFUSAL) + + +class _Capture(logging.Handler): + def __init__(self): + super().__init__(logging.DEBUG) + self.records = [] + + def emit(self, record): + self.records.append(record) + + +def _capture(testcase, name): + parallel._noted.clear() # "once" is per process: start each test afresh + logger = logging.getLogger(name) + handler = _Capture() + previous = logger.level + logger.addHandler(handler) + logger.setLevel(logging.DEBUG) + + def restore(): + logger.removeHandler(handler) + logger.setLevel(previous) + + testcase.addCleanup(restore) + return handler.records + + +def _assert_one_debug_line(testcase, records): + """Exactly one refusal line, at DEBUG and without a traceback.""" + refusals = [r for r in records if "interpreter is exiting" in r.getMessage()] + testcase.assertEqual(len(refusals), 1, [r.getMessage() for r in records]) + testcase.assertEqual(refusals[0].levelno, logging.DEBUG) + testcase.assertEqual([r for r in records if r.levelno > logging.DEBUG], []) + testcase.assertTrue(all(r.exc_info is None for r in records)) + + +class TestScheduleRefusedAtShutdown(unittest.TestCase): + def setUp(self): + self.handler = image_monitor.ImageGenerationHandler( + mock.MagicMock(), mock.MagicMock() + ) + self.addCleanup(self.handler.close) + + def test_schedule_returns_false_and_leaves_nothing_pending(self): + records = _capture(self, "prompt_manager.image_monitor") + with mock.patch.object(image_monitor, "threading", _refusing_threading()): + self.assertFalse(self.handler.schedule("/out/a.png")) + self.assertFalse(self.handler.schedule("/out/b.png")) + self.assertEqual(self.handler.pending_count(), 0) + _assert_one_debug_line(self, records) + + def test_another_start_failure_still_raises_and_leaves_nothing_pending(self): + class Failing(threading.Thread): + def start(self): + raise RuntimeError("can't start new thread") + + namespace = types.SimpleNamespace(**vars(threading)) + namespace.Thread = Failing + with mock.patch.object(image_monitor, "threading", namespace): + with self.assertRaises(RuntimeError): + self.handler.schedule("/out/a.png") + self.assertEqual(self.handler.pending_count(), 0) + + def test_schedule_before_shutdown_is_unchanged(self): + self.handler.processing_delay = 3600 + self.assertTrue(self.handler.schedule("/out/a.png")) + self.assertEqual(self.handler.pending_count(), 1) + + +class TestPreviewQueueRefusedAtShutdown(unittest.TestCase): + def thumbnailer(self, logger): + made = thumbnail_auto.AutoThumbnailer( + lambda *a, **k: {"action": "generated"}, lambda: True, logger + ) + self.addCleanup(made.close) + return made + + def test_enqueue_is_refused_quietly_and_drops_the_file(self): + logger = logging.getLogger("prompt_manager.test_exit_window") + records = _capture(self, logger.name) + thumbnailer = self.thumbnailer(logger) + src = Path("/out/a.png") + with mock.patch.object(thumbnail_auto, "threading", _refusing_threading()): + self.assertFalse(thumbnailer.enqueue(src, Path("/out/t/a.webp"), False)) + self.assertFalse(thumbnailer.enqueue(src, Path("/out/t/b.webp"), False)) + self.assertFalse(thumbnailer._worker_running) + self.assertEqual(len(thumbnailer._queue), 0) + self.assertEqual(thumbnailer._queued, set()) + self.assertTrue(thumbnailer.wait_idle(0)) + _assert_one_debug_line(self, records) + + +class TestMapParallelRefusedAtShutdown(unittest.TestCase): + def test_a_shut_pool_runs_the_items_on_the_calling_thread(self): + records = _capture(self, "prompt_manager.parallel") + with mock.patch.object(parallel, "ThreadPoolExecutor", _ShutPool): + self.assertEqual( + parallel.map_parallel(lambda n: n * n, [1, 2, 3], 4), [1, 4, 9] + ) + _assert_one_debug_line(self, records) + + def test_another_pool_error_still_raises(self): + class Broken(ThreadPoolExecutor): + def submit(self, *args, **kwargs): + raise RuntimeError("something else") + + with mock.patch.object(parallel, "ThreadPoolExecutor", Broken): + with self.assertRaises(RuntimeError): + parallel.map_parallel(lambda n: n, [1, 2], 2) + + +CHILD = textwrap.dedent(""" + import threading + + # Registered before concurrent.futures is imported, so it runs after the + # pool's own exit hook: from here on, a pool refuses new work. + exiting = threading.Event() + threading._register_atexit(exiting.set) + + import json + import logging + import os + import sys + from pathlib import Path + + root, tmp = sys.argv[1], sys.argv[2] + sys.path.insert(0, root) + + from py.api.thumbnail_auto import AutoThumbnailer + + logging.basicConfig(level=logging.WARNING, stream=sys.stderr) + logger = logging.getLogger("prompt_manager.exit_child") + + def generate(src, dst, is_video, *, stop, guard, track): + Path(dst).write_bytes(b"preview") + return {"action": "generated"} + + thumbnailer = AutoThumbnailer( + generate, lambda: True, logger, workers=lambda: 4 + ) + queued = threading.Event() + + def late_monitor(): + exiting.wait() + # One batch of two: the worker maps it on a pool. + with thumbnailer._changed: + for name in ("a", "b"): + src = Path(tmp, name + ".png") + src.write_bytes(b"x") + thumbnailer.enqueue(src, Path(tmp, name + "_thumb.webp"), False) + queued.set() + + def holder(): + # Non-daemon: the interpreter waits for it, inside the exit window. + exiting.wait() + queued.wait(30) + idle = thumbnailer.wait_idle(30) + written = sorted( + n for n in os.listdir(tmp) if n.endswith("_thumb.webp") + ) + print("PM_EXIT_REPORT " + json.dumps({"idle": idle, "written": written})) + sys.stdout.flush() + + threading.Thread(target=late_monitor, daemon=True).start() + threading.Thread(target=holder).start() + # Return normally, as ComfyUI does after Ctrl+C. + """) + + +class TestExitWindowChild(unittest.TestCase): + def test_previews_queued_in_the_exit_window_log_no_traceback(self): + with tempfile.TemporaryDirectory() as tmp: + script = os.path.join(tmp, "child.py") + with open(script, "w", encoding="utf-8") as handle: + handle.write(CHILD) + env = dict(os.environ) + env.pop("PROMPT_MANAGER_CONFIG_PATH", None) + env["PROMPT_MANAGER_DATA_DIR"] = tmp + env["PYTHONIOENCODING"] = "utf-8" + try: + completed = subprocess.run( + [sys.executable, script, ROOT, tmp], + cwd=ROOT, + env=env, + capture_output=True, + timeout=CHILD_CEILING_SECONDS, + encoding="utf-8", + errors="replace", + ) + except subprocess.TimeoutExpired as hung: + self.fail( + f"the child did not exit within {CHILD_CEILING_SECONDS} s\n" + f"stdout:\n{hung.stdout}\nstderr:\n{hung.stderr}" + ) + detail = f"stdout:\n{completed.stdout}\nstderr:\n{completed.stderr}" + self.assertEqual(completed.returncode, 0, detail) + reports = [ + line[len(REPORT_PREFIX) :] + for line in completed.stdout.splitlines() + if line.startswith(REPORT_PREFIX) + ] + self.assertEqual(len(reports), 1, detail) + report = json.loads(reports[0]) + self.assertTrue(report["idle"], detail) + # The batch still ran: on the worker's own thread. + self.assertEqual(report["written"], ["a_thumb.webp", "b_thumb.webp"], detail) + self.assertNotIn("Traceback", completed.stderr, detail) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_legacy_contract_snapshot.py b/tests/test_legacy_contract_snapshot.py new file mode 100644 index 00000000..1ed7b7a0 --- /dev/null +++ b/tests/test_legacy_contract_snapshot.py @@ -0,0 +1,136 @@ +"""The legacy pages' backend contract does not move under System S13 (#330). + +S13 points the legacy admin and gallery buttons at native System and removes +their duplicate dialogs, changing no backend file. This snapshot pins what the +legacy pages talk to: every registered route (method and path) and the key +paths of ``GET /prompt_manager/settings``, the one route whose legacy request +body S13 shrinks. Record again only with a reason in the PR body: + + PM_RECORD_LEGACY_SNAPSHOT=1 python3 -m unittest tests.test_legacy_contract_snapshot + +Cutover (#334) removes this test and its fixture. +""" + +import json +import os +import unittest +from pathlib import Path + +try: + from tests.test_api_admin import AdminAPITestCase +except ImportError: # discovered with tests/ as the top-level directory + from test_api_admin import AdminAPITestCase + +from py.config import GalleryConfig # noqa: E402 + +SNAPSHOT = ( + Path(__file__).resolve().parent / "fixtures" / "legacy_contract_snapshot.json" +) +RECORD_ENV = "PM_RECORD_LEGACY_SNAPSHOT" +SETTINGS = "/prompt_manager/settings" +DEPTH = 2 +CHANGED = ( + "the legacy contract changed; if the change is intended (a new route, a new" + " settings key), record it with PM_RECORD_LEGACY_SNAPSHOT=1 and say why in" + " the PR body" +) + + +def key_paths(value, prefix="", depth=1): + """The key paths of a JSON object to ``DEPTH`` levels. + + ``{"a": {"b": 1}}`` gives ``a`` and ``a.b``; the items of a list of + objects give the union of their keys as ``a[].b``. + """ + paths = set() + for key, child in value.items(): + path = f"{prefix}{key}" + paths.add(path) + if depth >= DEPTH: + continue + if isinstance(child, dict): + paths |= key_paths(child, f"{path}.", depth + 1) + elif isinstance(child, list): + for item in child: + if isinstance(item, dict): + paths |= key_paths(item, f"{path}[].", depth + 1) + return paths + + +class TestLegacyContractSnapshot(AdminAPITestCase): + """Routes and settings keys against tests/fixtures/legacy_contract_snapshot.json. + + The base case isolates config.json (``PROMPT_MANAGER_CONFIG_PATH`` in the + temp tree), stubs ComfyUI's ``folder_paths`` and resets the gallery roots; + nothing here reads the maintainer's install. + """ + + async def asyncSetUp(self): + await super().asyncSetUp() + self.api._get_all_output_dirs = lambda: [str(self.output_dir)] + GalleryConfig.MONITORING_DIRECTORIES = [str(self.output_dir)] + + def _routes(self): + return sorted( + { + f"{route.method} {route.resource.canonical}" + for route in self.app.router.routes() + if route.method != "HEAD" + } + ) + + async def _settings_keys(self): + resp = await self.client.request("GET", SETTINGS) + body = await resp.json() + self.assertEqual(resp.status, 200, body) + return sorted(key_paths(body)) + + def _snapshot(self): + with open(SNAPSHOT, encoding="utf-8") as f: + return json.load(f) + + def _record(self, key, value): + recorded = self._snapshot() if SNAPSHOT.exists() else {} + recorded[key] = value + with open(SNAPSHOT, "w", encoding="utf-8", newline="\n") as f: + json.dump(recorded, f, indent=2) + f.write("\n") + + async def test_routes_match_the_snapshot(self): + routes = self._routes() + if os.environ.get(RECORD_ENV) == "1": + self._record("routes", routes) + self.assertGreater(len(routes), 50) + self.assertEqual(routes, self._snapshot()["routes"], CHANGED) + + async def test_settings_response_keeps_its_keys(self): + keys = await self._settings_keys() + if os.environ.get(RECORD_ENV) == "1": + self._record("settings_keys", keys) + self.assertIn("settings.infinite_scroll", keys) + self.assertEqual(keys, self._snapshot()["settings_keys"], CHANGED) + + +class TestKeyPaths(unittest.TestCase): + def test_depth_and_list_items(self): + value = { + "success": True, + "settings": {"a": 1, "deep": {"hidden": 1}}, + "rows": [{"path": "x"}, {"label": "y"}], + } + self.assertEqual( + sorted(key_paths(value)), + [ + "rows", + "rows[].label", + "rows[].path", + "settings", + "settings.a", + "settings.deep", + "success", + ], + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_restore_shutdown.py b/tests/test_restore_shutdown.py new file mode 100644 index 00000000..2ebf787f --- /dev/null +++ b/tests/test_restore_shutdown.py @@ -0,0 +1,233 @@ +"""A restore cut off by a server stop waits for its worker thread (#360). + +A cancelled handler (or job) task cannot stop the executor thread that +stages or publishes. Its cleanup (the stage, the upload) and the registry's +``restore`` admission must outlive that thread, and the live library ends as +the old or the restored one, never partial, with the safety backup kept. + +Every positive wait is on an event. The one bounded wait (``SETTLE``) is a +negative check: the cancelled task must still be pending while its worker is +held. +""" + +import asyncio +import os +import shutil +import sqlite3 +import threading +from unittest.mock import patch + +from database import restore_stage +from database.models import PromptModel +from py.api import admin as admin_mod +from py.api.jobs import RESTORE_JOB_KIND + +try: + from tests.test_restore_plans import RestoreTestCase, _texts +except ImportError: # discovered with tests/ as the top-level directory + from test_restore_plans import RestoreTestCase, _texts + +WAIT = 60 # a ceiling only a hung test reaches +SETTLE = 0.3 # the negative check's bound: the old code's cleanup takes ms + + +class _Gate: + """Hold one blocking call on its worker thread until released.""" + + def __init__(self, real): + self.real = real + self.entered = threading.Event() + self.release = threading.Event() + self.done = threading.Event() + + def method(self): + """A function to patch onto the class (bound like the real one).""" + + def held(*args): + return self(*args) + + return held + + def __call__(self, *args): + self.entered.set() + self.release.wait(WAIT) + try: + return self.real(*args) + finally: + self.done.set() + + +class ShutdownRestoreCase(RestoreTestCase): + def gate(self, real): + gate = _Gate(real) + # A failed check never leaves the worker held until WAIT. + self._gates = getattr(self, "_gates", []) + [gate] + self.addCleanup(gate.release.set) + return gate + + def stages(self): + """The stage folders (the upload's prefix starts the same way).""" + return [ + name + for name in super().stages() + if os.path.isdir(os.path.join(self.tmpdir, name)) + ] + + def uploads(self): + """The upload files (not the -wal/-shm its readers may add).""" + return sorted( + name + for name in os.listdir(self.tmpdir) + if name.startswith(restore_stage.UPLOAD_PREFIX) and name.endswith(".tmp") + ) + + def fake_upload(self, texts): + """Make ``_receive_restore_upload`` hand over a prepared upload.""" + self.upload(texts, "source.db") + source = os.path.join(self.tmpdir, "uploads", "source.db") + target = os.path.join(self.tmpdir, restore_stage.UPLOAD_PREFIX + "x.tmp") + shutil.copyfile(source, target) + + async def receive(request, uploads): + uploads.append(target) + return target, None + + return patch.object(self.api, "_receive_restore_upload", receive) + + async def wait_thread(self, event): + loop = asyncio.get_running_loop() + self.assertTrue(await loop.run_in_executor(None, event.wait, WAIT)) + + async def assert_still_pending(self, task): + done, _ = await asyncio.wait({task}, timeout=SETTLE) + if done: + for gate in getattr(self, "_gates", ()): + gate.release.set() # let the worker end before teardown + self.fail("the cancelled task ended while its worker still ran") + + +class TestLegacyRestoreAtShutdown(ShutdownRestoreCase): + async def test_cancel_during_publish_waits_for_the_worker(self): + self.save("old") + old_db = self.api.db + gate = self.gate(PromptModel.publish_staged) + with ( + self.fake_upload(["restored"]), + patch.object(PromptModel, "publish_staged", gate.method()), + ): + task = asyncio.ensure_future(self.api.restore_database(None)) + await self.wait_thread(gate.entered) + self.assertEqual(len(self.stages()), 1) + task.cancel() + await self.assert_still_pending(task) + # The worker still has its stage and upload, and runs alone. + self.assertEqual(len(self.stages()), 1) + self.assertEqual(len(self.uploads()), 1) + self.assertIsNotNone(self.api._jobs.active(RESTORE_JOB_KIND)) + gate.release.set() + with self.assertRaises(asyncio.CancelledError): + await asyncio.wait_for(task, WAIT) + self.assertTrue(gate.done.is_set()) + self.assertEqual(_texts(self.db_path), ["restored"]) + self.assertIsNot(self.api.db, old_db) # the restored library adopted + self.assertEqual(len(self.backups()), 1) # the safety backup is kept + self.assertEqual(self.stages(), []) + self.assertEqual(self.uploads(), []) + self.assertIsNone(self.api._jobs.active(RESTORE_JOB_KIND)) + + async def test_cancel_during_staging_waits_and_changes_nothing(self): + self.save("old") + gate = self.gate(PromptModel.stage_restore) + with ( + self.fake_upload(["restored"]), + patch.object(PromptModel, "stage_restore", gate.method()), + ): + task = asyncio.ensure_future(self.api.restore_database(None)) + await self.wait_thread(gate.entered) + task.cancel() + await self.assert_still_pending(task) + self.assertEqual(len(self.uploads()), 1) # still being read + self.assertIsNotNone(self.api._jobs.active(RESTORE_JOB_KIND)) + gate.release.set() + with self.assertRaises(asyncio.CancelledError): + await asyncio.wait_for(task, WAIT) + self.assertEqual(_texts(self.db_path), ["old"]) + self.assertEqual(self.backups(), []) + self.assertEqual(self.stages(), []) # the finished stage is removed + self.assertEqual(self.uploads(), []) + self.assertIsNone(self.api._jobs.active(RESTORE_JOB_KIND)) + + async def test_a_publish_that_fails_after_the_cancel_still_cancels(self): + self.save("old") + + def refuse(model, staged): + raise sqlite3.OperationalError("refused") + + gate = self.gate(refuse) + with ( + self.fake_upload(["restored"]), + patch.object(PromptModel, "publish_staged", gate.method()), + ): + task = asyncio.ensure_future(self.api.restore_database(None)) + await self.wait_thread(gate.entered) + task.cancel() + await self.assert_still_pending(task) + gate.release.set() + # Not answered as a 500: the cancel goes on. + with self.assertRaises(asyncio.CancelledError): + await asyncio.wait_for(task, WAIT) + self.assertEqual(_texts(self.db_path), ["old"]) + self.assertEqual(self.stages(), []) + self.assertEqual(self.uploads(), []) + self.assertIsNone(self.api._jobs.active(RESTORE_JOB_KIND)) + + async def test_a_second_cancel_still_adopts_the_restored_library(self): + self.save("old") + old_db = self.api.db + publish = self.gate(PromptModel.publish_staged) + prune = self.gate(admin_mod._prune_safety_backups) + with ( + self.fake_upload(["restored"]), + patch.object(PromptModel, "publish_staged", publish.method()), + patch.object(admin_mod, "_prune_safety_backups", prune), + ): + task = asyncio.ensure_future(self.api.restore_database(None)) + await self.wait_thread(publish.entered) + task.cancel() + await self.assert_still_pending(task) + publish.release.set() + await self.wait_thread(prune.entered) # adopting the library + task.cancel() # the loop's own shutdown cancels again + await self.assert_still_pending(task) + prune.release.set() + with self.assertRaises(asyncio.CancelledError): + await asyncio.wait_for(task, WAIT) + self.assertEqual(_texts(self.db_path), ["restored"]) + self.assertIsNot(self.api.db, old_db) + self.assertEqual(len(self.backups()), 1) + self.assertEqual(self.stages(), []) + self.assertIsNone(self.api._jobs.active(RESTORE_JOB_KIND)) + + +class TestReviewedRestoreAtShutdown(ShutdownRestoreCase): + async def test_cancelled_job_waits_for_the_publish(self): + self.save("old") + old_db = self.api.db + body = await self.preview(self.upload(["restored"])) + gate = self.gate(PromptModel.publish_staged) + with patch.object(PromptModel, "publish_staged", gate.method()): + started = await self.apply(body["token"]) + job = self.api._jobs.get(started["job"]["id"]) + await self.wait_thread(gate.entered) + job.task.cancel() + await self.assert_still_pending(job.task) + self.assertEqual(len(self.stages()), 1) + self.assertTrue(job.running) # its exclusive admission holds + gate.release.set() + with self.assertRaises(asyncio.CancelledError): + await asyncio.wait_for(job.task, WAIT) + self.assertEqual(_texts(self.db_path), ["restored"]) + self.assertIsNot(self.api.db, old_db) + self.assertEqual(len(self.backups()), 1) + self.assertEqual(self.stages(), []) + self.assertFalse(job.running) diff --git a/tests/test_review_inspection.py b/tests/test_review_inspection.py index bec27c2a..ea0774a1 100644 --- a/tests/test_review_inspection.py +++ b/tests/test_review_inspection.py @@ -298,19 +298,21 @@ class TestQwenEditBatchIsQueued(InspectionTestCase): class TestDefaultProfileStillDecides(InspectionTestCase): async def test_ambiguous_file_with_texts_keeps_the_default_profile(self): - """A default-ambiguous file with a choice never runs the inspection - profile, even when that profile would offer more candidates.""" + """A default-ambiguous file with a choice is recorded from the + default profile, even when the inspection profile would offer more + candidates. That profile only checks the saver anchor (#390): no + saver names this file, so it anchors nothing.""" self._roots() graph = { **_prefixed(qwen_edit_graph("make it night"), "q"), **unlinkable_graph("cat", "dog"), } - self._write(graph=graph) + self._write("unsaved_00001_.png", graph=graph) with patch.object( review_capture, - "resolve_inspection", - side_effect=AssertionError("inspection profile ran"), + "inspection_choice", + side_effect=AssertionError("inspection choices were asked"), ): await self._scan() @@ -321,17 +323,24 @@ class TestDefaultProfileStillDecides(InspectionTestCase): self.assertEqual(item["resolver_version"], review_capture.RESOLVER_VERSION) self.assertEqual(_texts(evidence), ["cat", "dog"]) - async def test_apply_of_a_default_item_never_runs_the_inspection_profile(self): + async def test_apply_of_a_default_item_inspects_only_for_the_anchor( + self, + ): self._roots() write_png(self.output_dir / "two.png", prompt=unlinkable_graph("cat", "dog")) await self._scan() - with patch.object( - review_capture, - "resolve_inspection", - side_effect=AssertionError("inspection profile ran"), + with ( + _counted(review_capture, "resolve_inspection") as calls, + patch.object( + review_capture, + "inspection_choice", + side_effect=AssertionError("inspection choices were asked"), + ), ): _item, (status, data) = await self._apply("cat") self.assertEqual(status, 200, data) + # The saver anchor check (#390): with the file's name, nothing matches. + self.assertEqual([c[1] for c in calls], ["two.png"]) self.assertEqual(self._linked_text(self.output_dir / "two.png"), "cat") @@ -369,15 +378,16 @@ class TestApplyInspectionCandidate(InspectionTestCase): calls = [] real = review_capture.resolve_inspection - def spy(metadata): - calls.append(metadata) - return real(metadata) + def spy(metadata, file_name=None): + calls.append((metadata, file_name)) + return real(metadata, file_name) with patch.object(review_capture, "resolve_inspection", side_effect=spy): _item, (status, data) = await self._apply("make it night") self.assertEqual(status, 200, data) self.assertEqual(len(calls), 1) + self.assertEqual(calls[0][1], NAME) # anchored on its saver (#390) async def test_table_change_keeps_the_item_applicable(self): """A table change for a class the graph uses moves the item's @@ -853,8 +863,8 @@ class TestInspectionResolverVersion(InspectionTestCase): version = _version() real = review_capture.resolve_inspection - def newer(metadata): - resolution = real(metadata) + def newer(metadata, file_name=None): + resolution = real(metadata, file_name) return { **resolution, "candidates": [ @@ -899,10 +909,10 @@ class TestScanContainment(InspectionTestCase): self._write("b.png", graph=qwen_edit_graph("snow", "rain")) real = review_capture.resolve_inspection - def fails_for_a(metadata): + def fails_for_a(metadata, file_name=None): if "make it night" in json.dumps(metadata): raise RuntimeError("inspection blew up") - return real(metadata) + return real(metadata, file_name) with ( patch.object(review_capture, "resolve_inspection", side_effect=fails_for_a), diff --git a/tests/test_review_saver_anchor.py b/tests/test_review_saver_anchor.py new file mode 100644 index 00000000..c61d8112 --- /dev/null +++ b/tests/test_review_saver_anchor.py @@ -0,0 +1,333 @@ +"""The scan and Needs review anchor a file on the saver that wrote it (#390). + +The workspace inspector (since #383) passes the file's name to the resolver, +which anchors the file on the one saver whose ``filename_prefix`` names it +(``_GraphResolver.saver_stages``). The scan and Needs review use the same +rule. Maintainer ruling: a file never falls through both the link and the +queue, so when only that anchor resolves a file the default profile and the +parser floor do not link, it is queued with the anchor's pair as its one +choice (never linked automatically). No match, several matches or an +unfinished walk anchor nothing, so such a file is queued as before. Apply +re-resolves with the file's name, so a choice the anchor contradicts is +refused as stale evidence. +""" + +import json +from unittest.mock import patch + +try: + from tests.test_review import unlinkable_graph, write_png + from tests.test_review_inspection import ( + PROMPTS, + _counted, + InspectionTestCase, + qwen_edit_graph, + ) +except ImportError: # discovered with tests/ as the top-level directory + from test_review import unlinkable_graph, write_png + from test_review_inspection import ( + PROMPTS, + _counted, + InspectionTestCase, + qwen_edit_graph, + ) + +from utils import review_capture + + +def branch_graph(*prompts, prefixes=None): + """A Qwen-edit batch whose every SaveImage has its own literal prefix.""" + graph = qwen_edit_graph(*prompts) + savers = [n for n in graph.values() if n["class_type"] == "SaveImage"] + names = prefixes or [f"branch_{index}" for index in range(len(savers))] + for saver, prefix in zip(savers, names): + saver["inputs"]["filename_prefix"] = prefix + return graph + + +def saved_unlinkable_graph(*positives): + """``unlinkable_graph`` whose branches save under their own prefix. + + The default profile finds it ambiguous with a choice and the scan's + browser-parser floor links nothing, so it is queued from the default + profile unless the saver that wrote the file anchors its branch. + """ + graph = unlinkable_graph(*positives) + samplers = [ + node_id + for node_id, node in graph.items() + if node["class_type"] == "KSampler" and node_id != "3" + ] + for index, sampler in enumerate(samplers): + graph[f"{sampler}1"] = { + "class_type": "VAEDecode", + "inputs": {"samples": [sampler, 0], "vae": ["1", 2]}, + } + graph[f"{sampler}2"] = { + "class_type": "SaveImage", + "inputs": { + "images": [f"{sampler}1", 0], + "filename_prefix": f"side_{index}", + }, + } + return graph + + +def _without_name(real): + """``resolve_inspection`` as it was before #390: the file name ignored.""" + + def unnamed(metadata, file_name=None): + return real(metadata) + + return unnamed + + +def _only_candidate(test, item): + """The positive text of an anchored item's one candidate.""" + evidence = json.loads(item["evidence"]) + test.assertEqual(evidence.get("anchor"), "saver") + test.assertEqual(evidence.get("profile"), "inspection") + (candidate,) = evidence["candidates"] + return candidate["positive"]["text"] + + +class TestInspectionItemsAnchor(InspectionTestCase): + async def test_file_its_saver_names_is_queued_with_the_anchor_choice(self): + self._roots() + path = self._write("branch_1_00001_.png", graph=branch_graph(*PROMPTS)) + + job = await self._scan() + + (item,) = self._items() + self.assertEqual(_only_candidate(self, item), "add a red hat") + self.assertEqual(item["candidate_count"], 1) + self.assertEqual( + item["resolver_version"], + review_capture.inspection_review_version( + {"prompt": branch_graph(*PROMPTS)} + ), + ) + (row,) = job["outcomes"]["review"] + self.assertEqual(row["path"], "branch_1_00001_.png") + # Queued, never linked: ingestion stays on the default profile. + self.assertIsNone(self._linked_text(path)) + + async def test_anchored_item_stays_on_rescan(self): + self._roots() + self._write("branch_1_00001_.png", graph=branch_graph(*PROMPTS)) + await self._scan() + (item,) = self._items() + + job = await self._scan("all") + + self.assertEqual([i["id"] for i in self._items()], [item["id"]]) + self.assertEqual(job["scope"]["review_untraced"], 0) + + async def test_file_no_saver_names_stays_queued(self): + self._roots() + self._write("elsewhere_00001_.png", graph=branch_graph(*PROMPTS)) + + await self._scan() + + (item,) = self._items() + self.assertEqual(item["candidate_count"], len(PROMPTS)) + self.assertNotIn("anchor", json.loads(item["evidence"])) + + async def test_file_several_savers_name_stays_queued(self): + self._roots() + graph = branch_graph(*PROMPTS, prefixes=["same", "same", "other"]) + self._write("same_00001_.png", graph=graph) + + await self._scan() + + (item,) = self._items() + self.assertEqual(item["candidate_count"], len(PROMPTS)) + + async def test_old_rule_item_is_replaced_by_the_anchor_choice(self): + self._roots() + self._write("branch_1_00001_.png", graph=branch_graph(*PROMPTS)) + real = review_capture.resolve_inspection + with ( + patch.object(review_capture, "REVIEW_RULE_VERSION", 0), + patch.object( + review_capture, "resolve_inspection", side_effect=_without_name(real) + ), + ): + await self._scan() + (old,) = self._items() # queued under the old rule with every branch + self.assertEqual(old["candidate_count"], len(PROMPTS)) + + await self._scan("all") + + (item,) = self._items() + self.assertNotEqual(item["id"], old["id"]) + self.assertEqual(_only_candidate(self, item), "add a red hat") + + async def test_apply_links_the_anchor_choice(self): + self._require_routes() + self._roots() + path = self._write("branch_1_00001_.png", graph=branch_graph(*PROMPTS)) + await self._scan() + + _item, (status, data) = await self._apply("add a red hat") + + self.assertEqual(status, 200, data) + self.assertEqual(self._linked_text(path), "add a red hat") + + async def test_apply_refuses_an_old_choice_the_anchor_contradicts(self): + self._require_routes() + self._roots() + path = self._write("branch_1_00001_.png", graph=branch_graph(*PROMPTS)) + real = review_capture.resolve_inspection + with ( + patch.object(review_capture, "REVIEW_RULE_VERSION", 0), + patch.object( + review_capture, "resolve_inspection", side_effect=_without_name(real) + ), + ): + await self._scan() + item, detail, observed = await self._open() + # branch_1 is the saver of "add a red hat"; choose another branch. + wrong = self._candidate(detail, "make it night") + + status, data = await self._decide( + item["id"], self._body(detail, observed, wrong["hash"]) + ) + + self.assertEqual(status, 409, data) + self.assertEqual(data["code"], "review-conflict", data) + self.assertIsNone(self._linked_text(path)) + + +class TestDefaultItemsAnchor(InspectionTestCase): + async def test_anchor_resolves_and_nothing_links_so_it_is_queued(self): + """Maintainer ruling: never unlinked and unqueued. The anchor resolves + the file, the default profile and the parser floor link nothing, so + it is queued with the anchor's pair as its choice.""" + self._roots() + path = self.output_dir / "side_0_00001_.png" + graph = saved_unlinkable_graph("cat", "dog") + write_png(path, prompt=graph) + self.assertEqual( + review_capture.resolve_prompt_pair({"prompt": graph})["state"], "ambiguous" + ) + + job = await self._scan() + + (item,) = self._items() + self.assertEqual(_only_candidate(self, item), "cat") + (row,) = job["outcomes"]["review"] + self.assertIn("more than one prompt could match", row["detail"]) + self.assertIsNone(self._linked_text(path)) + + async def test_default_ambiguous_file_no_saver_names_stays_queued(self): + self._roots() + write_png( + self.output_dir / "unsaved_00001_.png", + prompt=saved_unlinkable_graph("cat", "dog"), + ) + + await self._scan() + + (item,) = self._items() + evidence = json.loads(item["evidence"]) + self.assertNotIn("profile", evidence) + self.assertNotIn("anchor", evidence) + self.assertEqual(item["resolver_version"], review_capture.RESOLVER_VERSION) + + async def test_file_the_parser_floor_links_is_not_queued(self): + """Already linked by the default path: unchanged by the anchor.""" + self._roots() + graph = saved_unlinkable_graph("cat", "dog") + del graph["3"] # the parser floor now reads a real sampler + path = self.output_dir / "side_0_00001_.png" + write_png(path, prompt=graph) + + await self._scan() + + self.assertEqual(self._items(), []) + self.assertIsNotNone(self._linked_text(path)) + + async def test_apply_refuses_an_old_default_choice_the_anchor_contradicts(self): + self._require_routes() + self._roots() + path = self.output_dir / "side_0_00001_.png" + write_png(path, prompt=saved_unlinkable_graph("cat", "dog")) + with patch.object(review_capture, "_captured_inspection", return_value=None): + await self._scan() # queued from the default profile, as before #390 + item, detail, observed = await self._open() + # side_0 is the saver of "cat"; choose the other branch. + wrong = self._candidate(detail, "dog") + + status, data = await self._decide( + item["id"], self._body(detail, observed, wrong["hash"]) + ) + + self.assertEqual(status, 409, data) + self.assertEqual(data["code"], "review-conflict", data) + self.assertIsNone(self._linked_text(path)) + + async def test_old_default_item_is_replaced_by_the_anchor_choice(self): + self._roots() + write_png( + self.output_dir / "side_0_00001_.png", + prompt=saved_unlinkable_graph("cat", "dog"), + ) + with patch.object(review_capture, "_captured_inspection", return_value=None): + await self._scan() # queued from the default profile, as before #390 + (old,) = self._items() + self.assertEqual(old["resolver_version"], review_capture.RESOLVER_VERSION) + + await self._scan("all") + + (item,) = self._items() + self.assertNotEqual(item["id"], old["id"]) + self.assertEqual(_only_candidate(self, item), "cat") + + with _counted(review_capture, "resolve_inspection") as calls: + await self._scan("all") # now queued under its version: no re-read + self.assertEqual(calls, []) + self.assertEqual([i["id"] for i in self._items()], [item["id"]]) + + async def test_decided_anchored_item_is_kept(self): + self._require_routes() + self._roots() + write_png( + self.output_dir / "side_0_00001_.png", + prompt=saved_unlinkable_graph("cat", "dog"), + ) + await self._scan() + item, detail, observed = await self._open() + status, data = await self._decide( + item["id"], self._body(detail, observed, "unresolved") + ) + self.assertEqual(status, 200, data) + before = (self._items(), self._decisions()) + + await self._scan("all") + + self.assertEqual((self._items(), self._decisions()), before) + + async def test_apply_accepts_an_old_default_choice_the_anchor_names(self): + self._require_routes() + self._roots() + path = self.output_dir / "side_0_00001_.png" + write_png(path, prompt=saved_unlinkable_graph("cat", "dog")) + with patch.object(review_capture, "_captured_inspection", return_value=None): + await self._scan() + + _item, (status, data) = await self._apply("cat") + + self.assertEqual(status, 200, data) + self.assertEqual(self._linked_text(path), "cat") + + +class TestSaverAnchored(InspectionTestCase): + def test_only_the_anchor_counts(self): + anchored = {"prompt": saved_unlinkable_graph("cat", "dog")} + self.assertTrue(review_capture.saver_anchored(anchored, "side_1_00001_.png")) + self.assertFalse(review_capture.saver_anchored(anchored, "nope_00001_.png")) + self.assertFalse(review_capture.saver_anchored(anchored, None)) + # One sampler resolves without any name: the anchor decides nothing. + single = {"prompt": branch_graph("make it night")} + self.assertFalse(review_capture.saver_anchored(single, "branch_0_00001_.png")) diff --git a/tests/test_thumbnail_auto.py b/tests/test_thumbnail_auto.py index 71697131..18c92db1 100644 --- a/tests/test_thumbnail_auto.py +++ b/tests/test_thumbnail_auto.py @@ -978,6 +978,27 @@ class TestThumbnailOnCapture(AutoThumbnailCase): (self.output_dir / "thumbnails" / f"{name}_thumb.webp").is_file() ) + async def test_a_worker_refused_at_interpreter_exit_skips_quietly(self): + """#375: refused once the interpreter exits; nothing stays queued.""" + + class Refused(threading.Thread): + def start(self): + raise RuntimeError("can't create new thread at interpreter shutdown") + + namespace = type(sys)("threading_copy") + namespace.__dict__.update(vars(threading)) + namespace.Thread = Refused + thumbnailer = self.api._auto_thumbnails() + path = make_png(self.output_dir / "late.png", size=(32, 32)) + with ( + patch.object(auto, "threading", namespace), + self.assertNoLogs("prompt_manager.api", "INFO"), + ): + self.assertEqual(self.api.queue_capture_thumbnail(str(path)), "skipped") + self.assertFalse(thumbnailer._worker_running) + self.assertEqual(len(thumbnailer._queue), 0) + self.assertTrue(thumbnailer.wait_idle(0)) + def test_queue_is_bounded(self): thumbnailer = auto.AutoThumbnailer( lambda *a: {"action": "generated"}, diff --git a/tests/test_utils_logging_config.py b/tests/test_utils_logging_config.py index 3b07911e..d64ee51e 100644 --- a/tests/test_utils_logging_config.py +++ b/tests/test_utils_logging_config.py @@ -709,7 +709,8 @@ class TestRotatedCopyRule(IsolatedLoggerTestCase): self.assertNotEqual(link["size"], os.path.getsize(target)) for row in rows.values(): self.assertEqual( - set(row), {"filename", "path", "size", "modified", "is_main", "kind"} + set(row), + {"filename", "path", "size", "modified", "is_main", "kind", "identity"}, ) def test_an_unreadable_timestamp_does_not_break_the_scan(self): @@ -763,6 +764,217 @@ class TestRotatedCopyRule(IsolatedLoggerTestCase): self.manager.log_file_entry(name) +class TestClearOnlyWhatWasConfirmed(IsolatedLoggerTestCase): + """#389: Clear all logs empties or deletes no more than its confirmation showed.""" + + def setUp(self): + super().setUp() + self.logger.info("active log line") # the active log exists + self.outside = os.path.join(self.tmp.name, "outside") + os.makedirs(self.outside) + + def _path(self, name): + return os.path.join(self.log_dir, name) + + def _write(self, name, data=b"rotated\n"): + with open(self._path(name), "wb") as handle: + handle.write(data) + + def _read(self, path): + with open(path, "rb") as handle: + return handle.read() + + def _snapshot(self): + return {name: self._read(self._path(name)) for name in os.listdir(self.log_dir)} + + def _assert_unchanged(self, before, growing="prompt_manager.log"): + """Nothing deleted or emptied (the refusal itself logs to ``growing``).""" + after = self._snapshot() + self.assertEqual(sorted(after), sorted(before)) + for name, data in before.items(): + if name == growing: + self.assertTrue(after[name].startswith(data), name) + else: + self.assertEqual(after[name], data, name) + + def _confirmed(self): + """What the workspace dialog showed: the listed active and rotated rows.""" + return [ + {"name": row["filename"], "identity": row["identity"]} + for row in self.manager.get_log_files() + if row["kind"] in ("active", "rotated") + ] + + def _file_handler(self): + return next( + h + for h in self.manager.logger.handlers + if isinstance(h, logging.FileHandler) + ) + + def test_listed_identity_is_a_string_that_survives_json_numbers(self): + # mtime_ns (~1.7e18) and many inodes exceed 2**53: as JSON numbers a + # browser would round them and every confirmed clear would be refused. + self._write("prompt_manager.log.1") + for row in self.manager.get_log_files(): + with self.subTest(name=row["filename"]): + self.assertIsInstance(row["identity"], str) + self.assertRegex(row["identity"], r"^\d+:-?\d+:\d+$") + + def test_a_copy_dated_before_1970_can_be_confirmed(self): + self._write("prompt_manager.log.1") + try: + os.utime(self._path("prompt_manager.log.1"), ns=(0, -(10**9))) + except (OSError, OverflowError, ValueError) as error: + self.skipTest(f"this filesystem cannot store a time before 1970: {error}") + confirmed = self._confirmed() + if not any(":-" in row["identity"] for row in confirmed): + self.skipTest("the filesystem did not keep the negative time") + result = self.manager.truncate_logs(confirmed) + self.assertIn("prompt_manager.log.1", result["truncated"]) + + def test_a_rollover_after_the_confirmation_deletes_nothing(self): + self._write("prompt_manager.log.1", b"one" * 10) + self._write("prompt_manager.log.2", b"two" * 10) + confirmed = self._confirmed() + self.logger.info("written after the confirmation") + self._file_handler().doRollover() # .2 -> .3, .1 -> .2, log -> .1 + before = self._snapshot() + with self.assertRaises(logging_config.ClearRefused) as caught: + self.manager.truncate_logs(confirmed) + self.assertEqual(caught.exception.code, "logs-changed") + self._assert_unchanged(before) + + def test_a_copy_that_appeared_after_the_confirmation_is_kept(self): + self._write("prompt_manager.log.1", b"one") + confirmed = self._confirmed() + self._write("prompt_manager.log.5", b"not shown") + result = self.manager.truncate_logs(confirmed) + self.assertEqual(result["errors"], []) + self.assertEqual( + sorted(result["truncated"]), ["prompt_manager.log", "prompt_manager.log.1"] + ) + self.assertEqual( + sorted(os.listdir(self.log_dir)), + ["prompt_manager.log", "prompt_manager.log.5"], + ) + + def _clear(self, confirmed=None): + """Run a clear and return what it raised (None), so the files are + asserted first: on the old code the test fails on lost bytes.""" + try: + if confirmed is None: + self.manager.truncate_logs() # the legacy page's call + else: + self.manager.truncate_logs(confirmed) + except Exception as error: # noqa: BLE001 - asserted below + return error + return None + + def test_a_foreign_append_to_the_active_log_is_refused(self): + # A second ComfyUI sharing the folder writes the same active log. + self._write("prompt_manager.log.1") + listings = (None, "listed") + for listing in listings: + with self.subTest(listed=listing is not None): + confirmed = self._confirmed() if listing else None + with open(self._path("prompt_manager.log"), "ab") as other: + other.write(b"line from another process\n") + before = self._snapshot() + error = self._clear(confirmed) + self._assert_unchanged(before) + self.assertEqual(getattr(error, "code", None), "logs-foreign-writer") + self.logger.info("our own line") # our stream catches up again + + @unittest.skipIf(os.name == "nt", "Windows cannot rename a log that is open") + def test_a_foreign_rotation_of_our_log_is_refused(self): + # Another process rolled the shared log over: our handler still writes + # the file now named .1, and a new active log is the other's. + os.rename(self._path("prompt_manager.log"), self._path("prompt_manager.log.1")) + self._write("prompt_manager.log", b"the other process's log\n") + before = self._snapshot() + with self.assertRaises(logging_config.ClearRefused) as caught: + self.manager.truncate_logs() + self.assertEqual(caught.exception.code, "logs-foreign-writer") + # Our handler still writes the renamed file, so the refusal lands there. + self._assert_unchanged(before, growing="prompt_manager.log.1") + + def test_a_copy_that_changes_mid_clear_stops_the_clear(self): + # The identity is checked again just before each unlink: another + # process rotating while we delete stops the clear there. + for n in (1, 2): + self._write(f"prompt_manager.log.{n}", b"r" * n) + confirmed = self._confirmed() + real_unlink = os.unlink + calls = [] + + def unlink_then_rotate(path, *args, **kwargs): + real_unlink(path, *args, **kwargs) + if not calls: + with open(self._path("prompt_manager.log.2"), "ab") as handle: + handle.write(b"rotated in by another process") + calls.append(os.path.basename(path)) + + active_before = self._read(self._path("prompt_manager.log")) + with mock.patch.object(logging_config.os, "unlink", unlink_then_rotate): + result = self.manager.truncate_logs(confirmed) + self.assertEqual(calls, ["prompt_manager.log.1"]) + self.assertEqual(result["truncated"], ["prompt_manager.log.1"]) + self.assertEqual(len(result["errors"]), 1) + self.assertIn("prompt_manager.log.2", result["errors"][0]) + self.assertTrue(os.path.exists(self._path("prompt_manager.log.2"))) + # The active log is emptied last, so it is kept too. + self.assertTrue( + self._read(self._path("prompt_manager.log")).startswith(active_before) + ) + + def _require_hard_link(self, source, link): + try: + os.link(source, link) + except (OSError, NotImplementedError, AttributeError) as error: + self.skipTest(f"this filesystem cannot make hard links: {error}") + + def test_a_hard_linked_active_log_is_never_emptied(self): + other_name = os.path.join(self.outside, "kept-by-the-user.log") + self._require_hard_link(self._path("prompt_manager.log"), other_name) + kept = self._read(other_name) + self.assertTrue(kept) + for listed in (False, True): + with self.subTest(listed=listed): + error = self._clear(self._confirmed() if listed else None) + self.assertTrue(self._read(other_name).startswith(kept)) + self.assertEqual(getattr(error, "code", None), "logs-hard-link") + + def test_a_hard_linked_active_log_with_file_logging_off_is_kept(self): + self.manager.update_config({"file_logging": False}) + other_name = os.path.join(self.outside, "kept-by-the-user.log") + self._require_hard_link(self._path("prompt_manager.log"), other_name) + kept = self._read(other_name) + error = self._clear() + self.assertEqual(self._read(other_name), kept) + self.assertEqual(getattr(error, "code", None), "logs-hard-link") + + def test_a_malformed_confirmation_is_refused_before_anything_changes(self): + self._write("prompt_manager.log.1") + before = self._snapshot() + for expected in ( + "prompt_manager.log.1", + [{"name": "prompt_manager.log.1"}], + [{"name": "prompt_manager.log.1", "identity": 7}], + [{"name": "prompt_manager.log.1", "identity": "1:2"}], + [{"name": 1, "identity": "1:2:3"}], + [ + {"name": "prompt_manager.log.1", "identity": "1:2:3"}, + {"name": "prompt_manager.log.1", "identity": "1:2:3"}, + ], + ["prompt_manager.log.1"], + ): + with self.subTest(expected=expected): + with self.assertRaises(ValueError): + self.manager.truncate_logs(expected) + self._assert_unchanged(before) + + class TestConfiguration(IsolatedLoggerTestCase): def test_in_flight_record_cannot_reopen_retired_file_handler(self) -> None: handler = next( diff --git a/tests/test_workspace_system_fixture.py b/tests/test_workspace_system_fixture.py index a43ec854..23189560 100644 --- a/tests/test_workspace_system_fixture.py +++ b/tests/test_workspace_system_fixture.py @@ -1,12 +1,26 @@ -"""The System browser hooks only ever reset their own server's config.json (#249 S3).""" +"""The System browser hooks only ever reset their own server's config.json (#249 S3) +and decide JPEG XL availability whatever the host has installed (#371).""" +import asyncio import os +import sys import tempfile +import types import unittest from pathlib import Path from unittest import mock -from tests.browser.workspace_system_fixture import register_system_commands +from aiohttp import web +from aiohttp.test_utils import TestClient, TestServer + +_mock_server = mock.MagicMock() +_mock_server.PromptServer.instance.routes = mock.MagicMock() +sys.modules.setdefault("server", _mock_server) + +from py.api import thumbnail_encode # noqa: E402 +from tests.browser.workspace_system_fixture import ( # noqa: E402 + register_system_commands, +) class TestWorkspaceSystemFixture(unittest.TestCase): @@ -33,6 +47,48 @@ class TestWorkspaceSystemFixture(unittest.TestCase): register_system_commands(None, None, None, self.root) self.assertEqual(elsewhere.read_bytes(), b'{"real": true}') + def test_no_encoder_holds_even_when_the_host_has_one(self): + """``{available: false}`` survives the settings read's re-probe (#371).""" + config = self.root / "config.json" + env = {**os.environ, "PROMPT_MANAGER_CONFIG_PATH": str(config)} + api = types.SimpleNamespace() + app, routes = web.Application(), web.RouteTableDef() + real_detect = thumbnail_encode._detect + with mock.patch.dict(os.environ, env, clear=True): + reset = register_system_commands(app, routes, api, self.root) + app.add_routes(routes) + self.addCleanup(thumbnail_encode.reset_encoders) + self.addCleanup(setattr, thumbnail_encode, "_detect", real_detect) + + async def scenario(): + async with TestClient(TestServer(app)) as client: + + async def post(available): + response = await client.post( + "/__test__/system/jxl-encoder", json={"available": available} + ) + self.assertEqual(response.status, 200) + + await post(False) + status = thumbnail_encode.encoder_status()["jxl"] + self.assertFalse(status["available"]) + self.assertIsNone( + thumbnail_encode.detect_jxl_encoder(recheck_missing=True) + ) + await post(True) + status = thumbnail_encode.encoder_status()["jxl"] + self.assertEqual(status["encoder"], "cjxl") + await post(False) + + # The host's real detector finds an encoder (Pillow can save JPEG XL). + with mock.patch.object(thumbnail_encode, "_pillow_can_save_jxl", lambda: True): + asyncio.run(scenario()) + # Reset gives detection back to the host. + reset() + self.assertIs(thumbnail_encode._detect, real_detect) + status = thumbnail_encode.encoder_status()["jxl"] + self.assertEqual(status["encoder"], "pillow") + if __name__ == "__main__": unittest.main() diff --git a/tests/vendor/ci-workflow.test.mjs b/tests/vendor/ci-workflow.test.mjs index f489fd64..cfafcaae 100644 --- a/tests/vendor/ci-workflow.test.mjs +++ b/tests/vendor/ci-workflow.test.mjs @@ -247,3 +247,32 @@ test('the Python matrix and coverage run the suite through the watchdog runner', assert.doesNotMatch(found.get(id), /unittest discover/, `${id} no longer runs the bare unittest discover`); } }); + +// CI hygiene (#352): lint tools are pinned, the bandit hook can fail, the JS job caches npm. +const readRepoFile = path => readFileSync(new URL(`../../${path}`, import.meta.url), 'utf8'); + +test('black and flake8 are pinned exactly and the lint job installs those pins', () => { + const requirements = readRepoFile('requirements-dev.txt'); + assert.match(requirements, /^black==\d+\.\d+\.\d+$/m, 'black is pinned exactly'); + assert.match(requirements, /^flake8==\d+\.\d+\.\d+$/m, 'flake8 is pinned exactly'); + const lint = jobs(readWorkflow('test.yml')).get('lint'); + assert.match(lint, /pip install \$\(grep -E '\^\(black\|flake8\)==' requirements-dev\.txt\)/); + assert.doesNotMatch(lint, /pip install black flake8/, 'lint no longer installs the latest releases'); +}); + +test('the frontend modules job caches npm keyed on the vendor lockfile', () => { + const setupNode = steps(jobs(readWorkflow('test.yml')).get('js-test')) + .find(step => /uses: actions\/setup-node@v\d+/.test(step)); + assert.match(setupNode, /^ {10}cache: npm$/m); + assert.match(setupNode, /^ {10}cache-dependency-path: tools\/vendor\/package-lock\.json$/m); +}); + +test('the bandit pre-commit hook fails at the severity CI enforces and passes every file', () => { + const quality = readWorkflow('code-quality.yml'); + assert.match(quality, /^ {8}run: bandit -r \. -lll -q --exclude /m, 'CI fails on high severity'); + const hook = /- id: bandit\n([\s\S]*?)(?=\n {6}(?:- id:|#)|\n*$)/.exec(readRepoFile('.pre-commit-config.yaml'))?.[1]; + assert.ok(hook, 'the bandit hook exists'); + assert.match(hook, /^ {8}entry: bandit -lll -q$/m, 'the hook enforces the same -lll level as CI'); + assert.doesNotMatch(hook, /true|exit-zero|bash -c/, 'nothing masks the exit code or drops a file'); + assert.doesNotMatch(hook, /pass_filenames: false/, 'the hook scans the staged files'); +}); diff --git a/tests/vendor/legacy-coverage.test.mjs b/tests/vendor/legacy-coverage.test.mjs index c1bf38d7..4ab74223 100644 --- a/tests/vendor/legacy-coverage.test.mjs +++ b/tests/vendor/legacy-coverage.test.mjs @@ -1,6 +1,7 @@ import assert from 'node:assert/strict'; import test from 'node:test'; -import { COLUMNS, CONTROL_EXEMPTIONS, STATUSES, declaredTitles, parseControl, parseCoverageTable, readCoverageTable, +import { COLUMNS, CONTROL_EXEMPTIONS, PAGES, REMOVAL_COLUMNS, STATUSES, declaredTitles, duplicateRemovals, idUses, methodUses, + parseControl, parseCoverageTable, parseRemovalTable, readCoverageTable, readPage, readRemovalTable, removalProblems, rowProblems } from '../../tools/vendor/legacy-coverage.mjs'; // TESTING.md "Legacy System capability coverage" (#249 System S12, #329): every legacy System capability names an @@ -114,3 +115,99 @@ test('a covered row names its native control unless it is exempt', () => { assert.deepEqual([row.Status, row['Native control']], ['covered', '—']); } }); + +// TESTING.md "Legacy System UI removed by S13" (#330): a legacy element is removed only when its capability is +// `covered`, the removal copies that row's native test, and the element is really gone from its page. +const COVERAGE_HEADER = '| Capability | Legacy anchor | Owner | Status | Native control | Test |\n| --- | --- | --- | --- | --- | --- |'; +const REMOVAL_HEADER = '| Capability | Page | Removed ids | Removed methods | Native test |\n| --- | --- | --- | --- | --- |'; +const FIXTURE_COVERAGE = parseCoverageTable(['### Legacy System capability coverage', '', COVERAGE_HEADER, + '| View logs | `admin.html:1` | S12 Logs | covered | `diagnostics` · combobox · "Level" | level filter works (logs.spec.mjs) |', + '| Infinite scrolling default | `admin.html:2` | Legacy prompt list | kept in legacy | — | scrolls (list-state.test.js) |', + ''].join('\n')); +const removalRow = cells => parseRemovalTable(['### Legacy System UI removed by S13', '', REMOVAL_HEADER, `| ${cells} |`, ''] + .join('\n'))[0]; +const EMPTY_PAGE = () => ({ html: '', script: 'class Admin {\n kept() {}\n}\n' }); + +test('every removed legacy element cites a covered capability and its native test', () => { + const coverage = readCoverageTable(); + const rows = readRemovalTable(); + for (const row of rows) { + const cited = coverage.find(item => item.Capability === row.Capability); + assert.ok(cited, `TESTING.md:${row.line} cites "${row.Capability}"`); + assert.equal(cited.Status, 'covered', `TESTING.md:${row.line} ${row.Capability}`); + assert.equal(row['Native test'], cited.Test, `TESTING.md:${row.line} ${row.Capability}`); + assert.ok(Object.keys(PAGES).includes(row.Page), `TESTING.md:${row.line} page ${row.Page}`); + } + assert.deepEqual(duplicateRemovals(rows), []); + assert.deepEqual(REMOVAL_COLUMNS, ['Capability', 'Page', 'Removed ids', 'Removed methods', 'Native test']); +}); + +test('a removed id or method is gone from its page', () => { + const coverage = readCoverageTable(); + for (const row of readRemovalTable()) { + assert.deepEqual(removalProblems(row, coverage, page => readPage(page)), [], `TESTING.md:${row.line} ${row.Capability}`); + } + // Each way a page can still hold an id or a method is found; a call through another object, or a longer name, is not + // a definition. + const page = { + html: '
', + script: [ + "document.getElementById('logLevel').value;", + "this.showModal('diagnosticsModal');", + '', + 'document.querySelector("#logLimit option");', + "const backdrops = ['settingsModal', 'duplicatesModal'];", + 'class Admin {', + ' async refreshLogs(level) {', + ' }', + '} displayLogs(logs) {', + ' }', + 'this.loadLogStats();', + '// downloadLogs() {', + 'other.formatBytesLater(1);', + ].join('\n'), + }; + assert.deepEqual(idUses('logsModal', page), ['id="logsModal" in the markup']); + assert.deepEqual(idUses('logLevel', page), ["getElementById('logLevel')"]); + assert.deepEqual(idUses('logLimit', page), ["querySelector('#logLimit')"]); + assert.deepEqual(idUses('duplicatesModal', page), ["'duplicatesModal' in a modal list"]); + assert.deepEqual(idUses('logLevelX', page), []); + assert.deepEqual(idUses('diagnosticsModal', page), ["a …Modal('diagnosticsModal') call"]); + assert.deepEqual(idUses('logsContainer', page), []); + assert.deepEqual(methodUses('closeLogs', page), ['.closeLogs( called in the markup']); + assert.deepEqual(methodUses('refreshLogs', page), ['refreshLogs() defined']); + assert.deepEqual(methodUses('displayLogs', page), ['displayLogs() defined']); + assert.deepEqual(methodUses('loadLogStats', page), ['.loadLogStats( called']); + assert.deepEqual(methodUses('downloadLogs', page), []); + assert.deepEqual(methodUses('formatBytes', page), []); + // One id or method is listed on one row per page. + const twice = [removalRow('View logs | admin | `#logsModal` | — | level filter works (logs.spec.mjs)'), + { ...removalRow('View logs | admin | `#logsModal` | — | level filter works (logs.spec.mjs)'), line: 9 }]; + assert.match(duplicateRemovals(twice).join(), /admin #logsModal is listed on rows/); + assert.deepEqual(removalProblems(removalRow('View logs | admin | `logsModal`, `#ok` | `close` | level filter works (logs.spec.mjs)'), + FIXTURE_COVERAGE, EMPTY_PAGE), ['malformed entry "logsModal"', 'malformed entry "close"']); +}); + +test('a removal citing a kept-in-legacy capability fails', () => { + const row = removalRow('Infinite scrolling default | admin | `#infiniteScrollDefault` | — | scrolls (list-state.test.js)'); + assert.deepEqual(removalProblems(row, FIXTURE_COVERAGE, EMPTY_PAGE), ['cites a kept in legacy capability']); + const unknown = removalRow('Something else | admin | — | — | scrolls (list-state.test.js)'); + assert.deepEqual(removalProblems(unknown, FIXTURE_COVERAGE, EMPTY_PAGE), ['cites no single coverage row "Something else"']); +}); + +test('a removal whose native test differs fails', () => { + const row = removalRow('View logs | admin | `#logsModal` | `showLogsModal()` | another title (logs.spec.mjs)'); + assert.deepEqual(removalProblems(row, FIXTURE_COVERAGE, EMPTY_PAGE), + ['its native test differs from the coverage row\'s "level filter works (logs.spec.mjs)"']); + assert.deepEqual(removalProblems({ ...row, 'Native test': 'level filter works (logs.spec.mjs)' }, FIXTURE_COVERAGE, EMPTY_PAGE), []); +}); + +test('a removed id still on its page fails', () => { + const row = removalRow('View logs | admin | `#logsModal` | `showLogsModal()` | level filter works (logs.spec.mjs)'); + const page = () => ({ html: '', script: 'class Admin {\n showLogsModal() {\n }\n}\n' }); + assert.deepEqual(removalProblems(row, FIXTURE_COVERAGE, page), [ + '#logsModal is still on the admin page: id="logsModal" in the markup', + 'showLogsModal() is still on the admin page: showLogsModal() defined', + ]); + assert.deepEqual(removalProblems({ ...row, Page: 'metadata' }, FIXTURE_COVERAGE, page), ['unknown page "metadata"']); +}); diff --git a/tools/vendor/legacy-coverage.mjs b/tools/vendor/legacy-coverage.mjs index 46a807c7..4da4a74f 100644 --- a/tools/vendor/legacy-coverage.mjs +++ b/tools/vendor/legacy-coverage.mjs @@ -42,25 +42,30 @@ function cells(line) { return out; } -/** The table's rows as objects keyed by COLUMNS, from the section that starts at HEADING. */ -export function parseCoverageTable(markdown) { +/** The rows, keyed by `columns`, of the first Markdown table under `heading` (`label` names it in errors). */ +function parseTable(markdown, heading, columns, label) { const lines = markdown.split('\n'); - const start = lines.findIndex(line => line.trim() === HEADING); - if (start === -1) throw new Error(`TESTING.md has no "${HEADING}" section`); + const start = lines.findIndex(line => line.trim() === heading); + if (start === -1) throw new Error(`TESTING.md has no "${heading}" section`); const header = lines.findIndex((line, index) => index > start && line.trim().startsWith('|')); if (header === -1 || lines.slice(start + 1, header).some(line => /^#{1,3} /.test(line))) { - throw new Error('The coverage section has no table'); + throw new Error(`The ${label} section has no table`); } - const columns = cells(lines[header]); - if (columns.join('|') !== COLUMNS.join('|')) throw new Error(`The coverage table's columns are ${columns.join(', ')}`); + const found = cells(lines[header]); + if (found.join('|') !== columns.join('|')) throw new Error(`The ${label} table's columns are ${found.join(', ')}`); const rows = []; for (let index = header + 2; index < lines.length && lines[index].trim().startsWith('|'); index += 1) { const values = cells(lines[index]); - rows.push({ line: index + 1, ...Object.fromEntries(COLUMNS.map((column, at) => [column, values[at] ?? ''])) }); + rows.push({ line: index + 1, ...Object.fromEntries(columns.map((column, at) => [column, values[at] ?? ''])) }); } return rows; } +/** The table's rows as objects keyed by COLUMNS, from the section that starts at HEADING. */ +export function parseCoverageTable(markdown) { + return parseTable(markdown, HEADING, COLUMNS, 'coverage'); +} + /** * A Native control cell: `` `
` · · "" ``, optionally ` · in ""` when the name is shared * by several rows of the section → {section, role, name, row}; `—` → null; else it throws. @@ -213,3 +218,130 @@ export function rowProblems(row, { root = ROOT, locate = file => testFile(file, export function readCoverageTable(root = ROOT) { return parseCoverageTable(readFileSync(join(root, 'TESTING.md'), 'utf8')); } + +// ── Legacy System UI removed by S13 (#330) ────────────────────────────────────────────────────────────────────────── +// TESTING.md "Legacy System UI removed by S13": one row per covered capability whose legacy UI S13 removed, with the +// element ids and methods that went. A row must cite a `covered` coverage row and copy its Test cell, and none of its +// ids or methods may still be on its page. +export const REMOVAL_HEADING = '### Legacy System UI removed by S13'; +export const REMOVAL_COLUMNS = Object.freeze(['Capability', 'Page', 'Removed ids', 'Removed methods', 'Native test']); +/** Each legacy page's markup and script. */ +export const PAGES = Object.freeze({ + admin: Object.freeze({ html: 'web/admin.html', script: 'web/js/admin.js' }), + gallery: Object.freeze({ html: 'web/gallery.html', script: 'web/js/gallery.js' }), +}); + +/** The removal table's rows, from the section that starts at REMOVAL_HEADING. */ +export function parseRemovalTable(markdown) { + return parseTable(markdown, REMOVAL_HEADING, REMOVAL_COLUMNS, 'removal'); +} + +/** The removal table read from TESTING.md. */ +export function readRemovalTable(root = ROOT) { + return parseRemovalTable(readFileSync(join(root, 'TESTING.md'), 'utf8')); +} + +/** A Removed ids or Removed methods cell → its entries without backticks (`—` → none). */ +export function listCell(cell) { + if (cell.trim() === NONE) return []; + return cell.split(',').map(entry => entry.trim().replace(/^`|`$/g, '')).filter(Boolean); +} + +/** The removed ids of a row (without `#`) and its malformed entries. */ +function removedIds(row) { + const entries = listCell(row['Removed ids']); + return { ids: entries.filter(entry => /^#[\w-]+$/.test(entry)).map(entry => entry.slice(1)), + bad: entries.filter(entry => !/^#[\w-]+$/.test(entry)) }; +} + +/** The removed methods of a row (without `()`) and its malformed entries. */ +function removedMethods(row) { + const entries = listCell(row['Removed methods']); + return { names: entries.filter(entry => /^[A-Za-z_$][\w$]*\(\)$/.test(entry)).map(entry => entry.slice(0, -2)), + bad: entries.filter(entry => !/^[A-Za-z_$][\w$]*\(\)$/.test(entry)) }; +} + +const escapeRegExp = text => text.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + +/** + * Where `id` is still on a page: `id=""` in its markup or script; in its script a lookup, a `…Modal("")` call + * (`showModal`, `hideModal`) or a modal-list entry. + */ +export function idUses(id, { html, script }) { + const name = escapeRegExp(id); + const code = stripJsComments(script); + const uses = []; + // `id=`, not `data-id=`: the attribute starts after a space or a tag's `(` call (`this.`, `window.admin.`) + * in its script or markup. + */ +export function methodUses(name, { html, script }) { + const method = escapeRegExp(name); + const code = stripJsComments(script); + const uses = []; + if (new RegExp(`(?:^|[\\s;}])(?:async\\s+)?${method}\\s*\\([^)]*\\)\\s*\\{`, 'm').test(code)) uses.push(`${name}() defined`); + if (new RegExp(`\\.${method}\\s*\\(`).test(code)) uses.push(`.${name}( called`); + if (new RegExp(`\\.${method}\\s*\\(`).test(html)) uses.push(`.${name}( called in the markup`); + return uses; +} + +/** + * Every problem of one removal row (empty: sound). `coverage` is the coverage table's rows; `readPage(page)` returns + * that page's `{html, script}`. + */ +export function removalProblems(row, coverage, readPage) { + const problems = []; + const cited = coverage.filter(item => item.Capability === row.Capability); + if (cited.length !== 1) problems.push(`cites no single coverage row "${row.Capability}"`); + else if (cited[0].Status !== 'covered') problems.push(`cites a ${cited[0].Status} capability`); + else if (cited[0].Test !== row['Native test']) problems.push(`its native test differs from the coverage row's "${cited[0].Test}"`); + if (!Object.hasOwn(PAGES, row.Page)) { + problems.push(`unknown page "${row.Page}"`); + return problems; + } + const { ids, bad: badIds } = removedIds(row); + const { names, bad: badNames } = removedMethods(row); + for (const entry of [...badIds, ...badNames]) problems.push(`malformed entry "${entry}"`); + const page = readPage(row.Page); + for (const id of ids) for (const use of idUses(id, page)) problems.push(`#${id} is still on the ${row.Page} page: ${use}`); + for (const name of names) for (const use of methodUses(name, page)) problems.push(`${name}() is still on the ${row.Page} page: ${use}`); + return problems; +} + +/** Ids and methods listed on more than one row of the same page. */ +export function duplicateRemovals(rows) { + const seen = new Map(); + const duplicates = []; + for (const row of rows) { + for (const entry of [...listCell(row['Removed ids']), ...listCell(row['Removed methods'])]) { + const key = `${row.Page} ${entry}`; + if (seen.has(key)) duplicates.push(`${key} is listed on rows ${seen.get(key)} and ${row.line}`); + else seen.set(key, row.line); + } + } + return duplicates; +} + +/** A page's markup and script from the repository. */ +export function readPage(page, root = ROOT) { + const files = PAGES[page]; + return { html: readFileSync(join(root, files.html), 'utf8'), script: readFileSync(join(root, files.script), 'utf8') }; +} diff --git a/utils/image_monitor.py b/utils/image_monitor.py index 58e8ae22..78cfcb10 100644 --- a/utils/image_monitor.py +++ b/utils/image_monitor.py @@ -35,6 +35,7 @@ from watchdog.events import FileSystemEventHandler from .hashing import generate_prompt_hash from .metadata_extractor import ComfyUIMetadataExtractor +from .parallel import note_refusal, refused_at_shutdown from .logging_config import get_logger from .prompt_graph import ( metadata_opted_out, @@ -200,8 +201,9 @@ class ImageGenerationHandler(FileSystemEventHandler): and once its size has stopped changing. Returns: - False when the path is already pending, the pending set is full or - the handler is closed + False when the path is already pending, the pending set is full, + the handler is closed or the interpreter is exiting (its worker + cannot start) """ with self._pending_lock: if self._closed: @@ -217,7 +219,15 @@ class ImageGenerationHandler(FileSystemEventHandler): return False due = time.monotonic() + self.processing_delay self._pending[image_path] = (prompt_snapshot, due) - self._ensure_worker() + try: + self._ensure_worker() + except RuntimeError as error: + # No worker will ever take it: never leave it pending (#375). + self._pending.pop(image_path, None) + if not refused_at_shutdown(error): + raise + note_refusal(self.logger, "The image worker") + return False self._work.put(image_path) return True diff --git a/utils/logging_config.py b/utils/logging_config.py index 97a37639..08bb0a26 100644 --- a/utils/logging_config.py +++ b/utils/logging_config.py @@ -39,6 +39,35 @@ class RotatedChanged(Exception): self.name = name +# Clear all logs (#389): why it changed nothing, as the API and the pages say it. +CLEAR_CHANGED = ( + "The log files changed since you confirmed; nothing was cleared." + " Read the list again and clear again." +) +CLEAR_FOREIGN_WRITER = ( + "Another program is writing to prompt_manager.log, such as a second ComfyUI" + " sharing this log folder; nothing was cleared. Stop it and clear again." +) +CLEAR_HARD_LINK = ( + "prompt_manager.log has another name (a hard link); emptying it would empty" + " that file too, so nothing was cleared. Remove the other name, then clear" + " again." +) +# A confirmed row's identity: "::", a string because +# mtime_ns and many inodes are larger than a browser's exact integers. +# mtime_ns is negative for a time before 1970 (a restored archive, say). +_IDENTITY = re.compile(r"([0-9]{1,20}):(-?[0-9]{1,20}):([0-9]{1,40})") +MAX_CONFIRMED_ROWS = 10000 + + +class ClearRefused(Exception): + """Clear all logs changed nothing; ``code`` names why, the message says it.""" + + def __init__(self, code: str, message: str): + super().__init__(message) + self.code = code + + def _name_key(name: str) -> str: """How a name compares: case-folded where the filesystem folds it (Windows).""" return os.path.normcase(name) @@ -87,35 +116,37 @@ def scan_log_folder(log_dir) -> List[Dict[str, Any]]: be read is left out. Never logs: the cleanup calls it while it holds the file handler's lock. """ - rows = [] try: names = [entry.name for entry in os.scandir(log_dir)] except OSError: - return rows - for name in names: - path = os.path.join(log_dir, name) - try: - st = os.lstat(path) - kind = _kind(name, path, st) - except OSError: - continue - try: - modified = datetime.fromtimestamp(st.st_mtime).isoformat() - except (OSError, OverflowError, ValueError): # a time before 1970 on Windows - modified = "" - rows.append( - { - "name": name, - "kind": kind, - "size": st.st_size, - "mtime_ns": st.st_mtime_ns, - "ino": st.st_ino, - "modified": modified, - "_dir": stat.S_ISDIR(st.st_mode) and kind != "link", - "_regular": stat.S_ISREG(st.st_mode) and kind != "link", - } - ) - return rows + return [] + rows = (scan_log_entry(log_dir, name) for name in names) + return [row for row in rows if row is not None] + + +def scan_log_entry(log_dir, name) -> Optional[Dict[str, Any]]: + """One entry's scan row (``lstat``), or None when it cannot be read.""" + path = os.path.join(log_dir, name) + try: + st = os.lstat(path) + kind = _kind(name, path, st) + except OSError: + return None + try: + modified = datetime.fromtimestamp(st.st_mtime).isoformat() + except (OSError, OverflowError, ValueError): # a time before 1970 on Windows + modified = "" + return { + "name": name, + "kind": kind, + "size": st.st_size, + "mtime_ns": st.st_mtime_ns, + "ino": st.st_ino, + "modified": modified, + "_dir": stat.S_ISDIR(st.st_mode) and kind != "link", + "_regular": stat.S_ISREG(st.st_mode) and kind != "link", + "_nlink": st.st_nlink, + } def public_row(row: Dict[str, Any]) -> Dict[str, Any]: @@ -127,6 +158,37 @@ def rotated_sort_key(row: Dict[str, Any]) -> int: return _rotated_number(row["name"]) or 0 +def file_identity(row: Dict[str, Any]) -> str: + """A listed file's identity as the client sends it back: size, mtime_ns, ino.""" + return f"{row['size']}:{row['mtime_ns']}:{row['ino']}" + + +def parse_confirmed(expected) -> List[Dict[str, Any]]: + """The rows a Clear all logs confirmation showed, as review rows. + + ``expected`` is a list of ``{name, identity}`` objects from + ``/logs/files``; anything else raises ValueError. + """ + if not isinstance(expected, list) or len(expected) > MAX_CONFIRMED_ROWS: + raise ValueError("expected must be a list of listed log files") + rows, seen = [], set() + for item in expected: + if not isinstance(item, dict): + raise ValueError("each expected file must be an object") + name, identity = item.get("name"), item.get("identity") + if not isinstance(name, str) or not isinstance(identity, str): + raise ValueError("each expected file needs a name and an identity") + match = _IDENTITY.fullmatch(identity) + if match is None: + raise ValueError("an expected file's identity is not size:mtime_ns:ino") + size, mtime_ns, ino = (int(part) for part in match.groups()) + if _name_key(name) in seen: + raise ValueError("an expected file is listed twice") + seen.add(_name_key(name)) + rows.append({"name": name, "size": size, "mtime_ns": mtime_ns, "ino": ino}) + return rows + + def log_file_entry(log_dir, name) -> Path: """The path of a downloadable log file named ``name`` (blocking). @@ -157,6 +219,11 @@ def _stream_is(stream, ino: int) -> bool: return not (ino and open_ino and open_ino != ino) +def _stream_ino_is(opened: os.stat_result, ino: int) -> bool: + """Whether a stream's ``fstat`` is the file with inode ``ino`` (0: unknown).""" + return not (ino and opened.st_ino and opened.st_ino != ino) + + def _error_text(error: BaseException) -> str: """An OSError's reason and file name, without the folder it is in.""" if isinstance(error, OSError): @@ -432,6 +499,7 @@ class PromptManagerLogger: "modified": row["modified"], "is_main": row["kind"] == "active", "kind": row["kind"], + "identity": file_identity(row), } ) @@ -530,8 +598,14 @@ class PromptManagerLogger: finally: handler.release() if not handled: - flags = os.O_WRONLY | os.O_TRUNC | getattr(os, "O_NOFOLLOW", 0) - os.close(os.open(str(log_file), flags)) + flags = os.O_WRONLY | getattr(os, "O_NOFOLLOW", 0) + fd = os.open(str(log_file), flags) + try: + if os.fstat(fd).st_nlink > 1: # emptying it would empty the other + raise ClearRefused("logs-hard-link", CLEAR_HARD_LINK) + os.ftruncate(fd, 0) + finally: + os.close(fd) def delete_rotated(self, expected: List[Dict[str, Any]]) -> Dict[str, Any]: """Delete exactly the reviewed rotated copies, or none of them (blocking). @@ -582,47 +656,126 @@ class PromptManagerLogger: ) return {"removed": removed, "failed": failed} - def truncate_logs(self) -> Dict[str, Any]: - """Empty the active log and delete its rotated copies (Rule R). + def _active_streams(self, active: Optional[Dict[str, Any]]): + """``fstat`` and offset of every handler stream writing the active name.""" + target = os.path.normcase( + os.path.realpath(os.path.join(self.log_dir, ACTIVE_LOG_NAME)) + ) + for handler in self._file_handlers(): + stream = getattr(handler, "stream", None) + base = getattr(handler, "baseFilename", None) + if stream is None or base is None: + continue + if os.path.normcase(os.path.realpath(base)) != target: + continue + try: + stream.flush() + fd = stream.fileno() + yield os.fstat(fd), os.lseek(fd, 0, os.SEEK_CUR) + except (OSError, ValueError, AttributeError): + continue - Every other file in the log folder, and every link, is kept. Also - clears the memory buffer. Runs under ``_files_lock`` and the file - handlers' locks, so no rollover recreates a just-deleted copy, and - logs only once they are released. + def _check_clear_is_safe(self, rows, active) -> None: + """Refuse a clear that would empty or delete what nobody confirmed. + + Called under the files lock and every handler lock, so nothing of this + process writes or rolls over meanwhile; what changes is another + program's doing. Our handler's stream is flushed: a file larger than + the stream's own offset was appended to by someone else, and a stream + whose file is now a rotated copy was rolled over by someone else. + Either is a second writer (another ComfyUI sharing the folder). This + is best effort, not a lock: writes before our own last write, or any + write while this process has file logging off, go unseen. An + active log with more than one name (a hard link) is never emptied: the + other name would be emptied too. + """ + rotated = {row["ino"] for row in rows if row["kind"] == "rotated"} + rotated.discard(0) + nlink = active["_nlink"] if active is not None else 1 + for opened, offset in self._active_streams(active): + if (opened.st_ino and opened.st_ino in rotated) or opened.st_size > offset: + raise ClearRefused("logs-foreign-writer", CLEAR_FOREIGN_WRITER) + if active is not None and _stream_ino_is(opened, active["ino"]): + nlink = max(nlink, opened.st_nlink) + if nlink > 1: + raise ClearRefused("logs-hard-link", CLEAR_HARD_LINK) + + @staticmethod + def _confirmed_targets(confirmed, current, active) -> List[Dict[str, Any]]: + """The rotated copies a confirmation listed, each as it was listed. + + Refuses (``logs-changed``) when any listed copy is missing or differs + in kind, size, ``mtime_ns`` or inode, or when the listed active log is + now another file (it was rolled over). A copy nobody listed is kept. + """ + targets = [] + for reviewed in confirmed: + key = _name_key(reviewed["name"]) + row = current.get(key) + if key == _name_key(ACTIVE_LOG_NAME): + ino = reviewed["ino"] + if active is None or (ino and active["ino"] and active["ino"] != ino): + raise ClearRefused("logs-changed", CLEAR_CHANGED) + continue + if _rotated_number(reviewed["name"]) is None: + continue # never deleted, listed or not + if not _matches_review(row, reviewed): + raise ClearRefused("logs-changed", CLEAR_CHANGED) + targets.append(row) + return targets + + def truncate_logs(self, expected=None) -> Dict[str, Any]: + """Empty the active log and delete its rotated copies (Rule R, #389). + + ``expected`` is what the confirmation showed: ``{name, identity}`` + rows of ``/logs/files`` (``parse_confirmed``; ValueError when + malformed). Then only the listed rotated copies are deleted, and only + when each still has the listed size, ``mtime_ns`` and inode and the + active log is still the listed file; otherwise ClearRefused + (``logs-changed``) with nothing changed. Without it (the legacy admin + page, whose confirmation lists nothing) every rotated copy present now + is deleted. + + Either way, a second writer of the active log (``logs-foreign-writer``) + or an active log with a hard link (``logs-hard-link``) refuses the + whole clear first. Each copy's identity is checked again just before + its unlink; one that changed (another process rolling over meanwhile) + stops the clear there, keeping it, the rest and the active log, which + is emptied last. Every other file in the log folder, and every link, + is kept. Also clears the memory buffer. Runs under ``_files_lock`` and + the file handlers' locks and logs only once they are released. Returns: Dictionary with: - truncated: List of successfully truncated files - errors: List of error messages for failed operations """ + confirmed = None if expected is None else parse_confirmed(expected) results = {"truncated": [], "errors": []} + refused = None with self._files_lock: held = self._acquire_file_handlers() try: rows = scan_log_folder(self.log_dir) - active = [row for row in rows if row["kind"] == "active"] - rotated = sorted( - (row for row in rows if row["kind"] == "rotated"), - key=rotated_sort_key, - ) - for row in active + rotated: - path = os.path.join(self.log_dir, row["name"]) - try: - if row["kind"] == "active": - self._truncate_main_log(Path(path), row["ino"]) - else: - os.unlink(path) - results["truncated"].append(row["name"]) - except OSError as error: - results["errors"].append( - f"Failed to truncate {row['name']}: {_error_text(error)}" - ) - with self._buffer_lock: - self._log_buffer.clear() + current = {_name_key(row["name"]): row for row in rows} + active = next((row for row in rows if row["kind"] == "active"), None) + try: + if confirmed is None: + targets = [row for row in rows if row["kind"] == "rotated"] + else: + targets = self._confirmed_targets(confirmed, current, active) + self._check_clear_is_safe(rows, active) + except ClearRefused as error: + refused = error + else: + self._clear(sorted(targets, key=rotated_sort_key), active, results) finally: self._release_handlers(held) + if refused is not None: + self.logger.warning("Clear all logs refused (%s)", refused.code) + raise refused if results["truncated"]: self.logger.info("Truncated log files: %s", ", ".join(results["truncated"])) for error_msg in results["errors"]: @@ -630,6 +783,39 @@ class PromptManagerLogger: return results + def _clear(self, targets, active, results) -> None: + """Delete ``targets`` (each re-checked) then empty ``active`` (locks held).""" + for reviewed in targets: + name = reviewed["name"] + if not _matches_review(scan_log_entry(self.log_dir, name), reviewed): + results["errors"].append( + f"{name} changed while clearing (another program may be writing" + " to this log folder); it, the copies after it and the active" + " log were kept" + ) + return + try: + os.unlink(os.path.join(self.log_dir, name)) + results["truncated"].append(name) + except OSError as error: + results["errors"].append( + f"Failed to truncate {name}: {_error_text(error)}" + ) + if active is not None: + try: + self._truncate_main_log( + Path(self.log_dir, active["name"]), active["ino"] + ) + results["truncated"].append(active["name"]) + except ClearRefused as error: + results["errors"].append(str(error)) + except OSError as error: + results["errors"].append( + f"Failed to truncate {active['name']}: {_error_text(error)}" + ) + with self._buffer_lock: + self._log_buffer.clear() + def update_config(self, new_config: Dict[str, Any]): """Update logging configuration. diff --git a/utils/parallel.py b/utils/parallel.py index 8b183dd8..65dcca44 100644 --- a/utils/parallel.py +++ b/utils/parallel.py @@ -5,17 +5,51 @@ a CUDA-initialised, multi-threaded process is unsafe and the spawn start method would re-import ComfyUI's main module in every child. Pillow releases the GIL while decoding, resizing and encoding, so threads still scale close to linearly for both jobs. + +Interpreter exit (#375): ``threading._shutdown`` shuts every thread pool +(``submit`` then raises "cannot schedule new futures after interpreter +shutdown") before it joins the non-daemon threads and long before the package +teardown runs, while our daemon threads keep working. Measured on 3.12 and +3.14, ``Thread.start`` still succeeds in that window; it is refused ("can't +create new thread at interpreter shutdown") only once the interpreter is +marked finalizing, after the ``atexit`` callbacks. ``refused_at_shutdown`` +recognises both; ``note_refusal`` logs one DEBUG line per place, with no +traceback. """ +import logging +import threading from concurrent.futures import ThreadPoolExecutor +SHUTDOWN_REFUSAL = "interpreter shutdown" +_logger = logging.getLogger("prompt_manager.parallel") +_noted = set() +_noted_lock = threading.Lock() + + +def refused_at_shutdown(error): + """Whether ``error`` is a thread or pool start refused at interpreter exit.""" + return isinstance(error, RuntimeError) and SHUTDOWN_REFUSAL in str(error) + + +def note_refusal(logger, what): + """Log once per ``what``, at DEBUG and without a traceback, that ``what`` + was refused because the interpreter is exiting.""" + key = (logger.name, what) + with _noted_lock: + if key in _noted: + return + _noted.add(key) + logger.debug(f"{what}: refused, the interpreter is exiting") + def map_parallel(func, items, workers): """Apply ``func`` to every item on up to ``workers`` threads, keeping order. ``workers`` of 1 (or fewer) runs on the calling thread with no pool. An exception in ``func`` propagates to the caller, so callers that must - survive bad input wrap ``func`` themselves. + survive bad input wrap ``func`` themselves. A pool refused at interpreter + exit runs the items it did not start on the calling thread (#375). """ items = list(items) workers = max(1, int(workers or 1)) @@ -23,4 +57,35 @@ def map_parallel(func, items, workers): return [func(item) for item in items] max_workers = min(workers, len(items)) with ThreadPoolExecutor(max_workers, thread_name_prefix="pm-worker") as pool: - return list(pool.map(func, items)) + futures = [] + try: + for item in items: + futures.append(pool.submit(func, item)) + except RuntimeError as error: + if not refused_at_shutdown(error): + raise + note_refusal(_logger, "A worker pool") + # Drop what was queued but not started; it runs here instead. + # (A refused Thread.start leaves its item queued without a future + # here: it is cancelled too, unless a busy pool thread took it in + # between, the one case where an item can run twice.) + pool.shutdown(wait=True, cancel_futures=True) + else: + return [future.result() for future in futures] + return _run_rest(func, items, futures) + + +def _run_rest(func, items, futures): + """The pool's finished results, and every item it did not run, run here. + + After ``shutdown(cancel_futures=True)`` each submitted future either ran + (its result or exception is kept) or was cancelled before it started. + """ + results = [] + for index, item in enumerate(items): + future = futures[index] if index < len(futures) else None + if future is not None and not future.cancelled(): + results.append(future.result()) + else: + results.append(func(item)) + return results diff --git a/utils/review_capture.py b/utils/review_capture.py index 1859dee7..8fadab72 100644 --- a/utils/review_capture.py +++ b/utils/review_capture.py @@ -40,6 +40,19 @@ while it is pending and undecided (never beside a decision, see profile the item recorded (``captured_review(..., item_profile(evidence))``). The inspection profile only ever produces review choices; ingestion's own links stay on the default profile, which never loads the table (D1). + +The saver anchor (#390): the inspection profile resolves a file with its +name, as the workspace inspector does since #383, so the one saver whose +``filename_prefix`` names the file anchors its branch. When that anchor alone +resolves a file the default profile left ambiguous and nothing links +(``saver_anchored``), the scan queues it with the anchor's pair as its one +candidate (evidence ``anchor: "saver"``), never linking it: a file never +falls through both the link and the queue (maintainer ruling). No match, +several matches or an unfinished walk anchor nothing and the file is queued +as before. Apply re-resolves with the file's name, so only the anchored +branch can be applied. ``REVIEW_RULE_VERSION`` is folded into inspection +item versions, so inspection items recorded before the anchor are recorded +again by the next scan. """ from __future__ import annotations @@ -104,10 +117,16 @@ MAX_CANDIDATE_DEPTH = 8 # The resolver profile an item's evidence was built with (``profile``). DEFAULT = "default" INSPECTION = "inspection" +# The ``anchor`` an item recorded from the saver anchor names (#390). +SAVER_ANCHOR = "saver" +ANCHOR_REASON = "The saver that wrote this file names its branch" # Inspection item versions: above every plausible ``RESOLVER_VERSION`` and, # with 40 bits of the folded digest, below 2**53 (exact as a JSON number). INSPECTION_VERSION_BASE = 1 << 40 _INSPECTION_VERSION_HEX = 10 +# How the scan and Apply use the inspection profile, folded into inspection +# item versions. 1: they resolve with the file's name (the saver anchor, #390). +REVIEW_RULE_VERSION = 1 # Above this many distinct node classes the whole table is the fingerprint. @@ -154,38 +173,92 @@ def inspection_review_version(metadata: Any = None) -> int | None: """The ``resolver_version`` of an item recorded from the inspection profile. Folds the profile with its budgets, ``RESOLVER_VERSION``, - ``INSPECTION_RESOLVER_VERSION`` and ``semantics_version(metadata)``, so - a change of either resolver, of the profile's bounds or of the table - about the graph's classes moves it. None when the table cannot be loaded. + ``INSPECTION_RESOLVER_VERSION``, ``REVIEW_RULE_VERSION`` and + ``semantics_version(metadata)``, so a change of either resolver, of how + review uses the profile, of the profile's bounds or of the table about + the graph's classes moves it. None when the table cannot be loaded. """ table = semantics_version(metadata) if table is None: return None profile = repr(astuple(INSPECTION_PROFILE)) seed = f"{profile}:{RESOLVER_VERSION}:{INSPECTION_RESOLVER_VERSION}:{table}" + if REVIEW_RULE_VERSION: # 0 is the seed items had before the rule existed + seed += f":review-rule-{REVIEW_RULE_VERSION}" folded = hashlib.sha256(seed.encode("utf-8")).hexdigest() return INSPECTION_VERSION_BASE + int(folded[:_INSPECTION_VERSION_HEX], 16) -def resolve_inspection(metadata: dict) -> dict: - """``metadata`` resolved with the inspection profile (bounded work).""" - return resolve_prompt_pair(metadata, profile=INSPECTION_PROFILE) +def resolve_inspection(metadata: dict, file_name: str | None = None) -> dict: + """``metadata`` resolved with the inspection profile (bounded work). + + ``file_name`` (the file's last path part) anchors the file on the one + saver that wrote it, as in the workspace inspector (#390). + """ + return resolve_prompt_pair( + metadata, profile=INSPECTION_PROFILE, file_name=file_name + ) -def inspection_choice(metadata: dict) -> dict | None: - """The inspection profile's ambiguous result when it offers a choice. +def inspection_choice(metadata: dict, file_name: str | None = None) -> dict | None: + """The inspection profile's result when the file is a review case. The scan asks only for a file the default profile found ambiguous - without a choice that nothing links. + without a choice that nothing links. The result is ambiguous with a + choice, or ``resolved`` when ``file_name`` anchors the file on its saver + and only that anchor resolves it (``saver_anchored``): the item then + offers the anchor's pair. None otherwise. """ if inspection_review_version(metadata) is None: return None - resolution = resolve_inspection(metadata) + resolution = resolve_inspection(metadata, file_name) + if resolution.get("state") == "resolved": + return _anchor_of(metadata, file_name, resolution) if resolution.get("state") != "ambiguous" or not reviewable(resolution): return None return resolution +def _anchor_of(metadata: Any, file_name: str | None, named: dict) -> dict | None: + """``named`` tagged ``anchor: "saver"`` when only the saver anchor + resolves the file, to a pair with a positive text to choose; else None. + + ``named`` is ``metadata`` resolved with ``file_name``; the profile runs + once more without the name only when it is resolved and choosable. + """ + if not file_name or named.get("state") != "resolved": + return None + try: + offered = choosable(_projection(named)) + except (KeyError, TypeError): + return None + if not offered or resolve_inspection(metadata).get("state") == "resolved": + return None + return {**named, "anchor": SAVER_ANCHOR} + + +def saver_anchored(metadata: Any, file_name: str | None) -> bool: + """True when the saver that wrote ``file_name`` resolves its branch (#390). + + The inspection profile resolves ``metadata`` with the file's name, and + without it the file does not resolve: the anchor alone decides it. A file + the default profile left ambiguous is then queued with the anchor's pair + as its one candidate. No name, an unusable table, no matching saver, + several, or an unfinished walk anchor nothing (False). The profile + without the name runs only when the named run resolved. + """ + return _anchored(metadata, file_name) is not None + + +def _anchored(metadata: Any, file_name: str | None) -> dict | None: + """The anchored result ``saver_anchored`` describes, else None.""" + if not file_name or not isinstance(metadata, dict): + return None + if inspection_review_version(metadata) is None: + return None + return _anchor_of(metadata, file_name, resolve_inspection(metadata, file_name)) + + def item_profile(evidence: Any) -> str: """The profile an item's stored evidence was built with.""" if isinstance(evidence, dict) and evidence.get("profile") == INSPECTION: @@ -246,7 +319,9 @@ def record_ambiguous_file( """ versions = item_versions(scanned[0] if scanned is not None else None) spelled_path, queued = _queued_spelling(db, root, relative, versions) - if queued is not None: + if queued is not None and not ( + inspection and _default_item_to_anchor(db, queued, scanned, relative) + ): return _already_queued(db, *queued, versions) epoch = db.get_execution_epoch() try: @@ -272,24 +347,24 @@ def record_ambiguous_file( if resolution is None or resolution.get("state") != "ambiguous": return "skipped" profile, version = DEFAULT, RESOLVER_VERSION - if not reviewable(resolution): - version = inspection_review_version(captured[0]) if inspection else None - if version is None: + named = _captured_inspection(captured[0], relative, scanned) if inspection else None + anchored = named is not None and named[0].get("anchor") == SAVER_ANCHOR + if anchored: + # Only the saver anchor resolves it (#390): offer that one pair. + profile, version = INSPECTION, named[1] + resolution = { + "state": "ambiguous", + "reason": ANCHOR_REASON, + "candidates": [named[0]], + } + elif not reviewable(resolution): + if named is None: return "untraced" - profile = INSPECTION - if ( - scanned is not None - and scanned[1] is not None - and _same_records(captured[0], scanned[0]) - ): - resolution = scanned[1] # the same records: no second run - else: - # Heavier, so only for a file ambiguous with nothing to choose. - resolution = resolve_inspection(captured[0]) + profile, (resolution, version) = INSPECTION, named if resolution.get("state") != "ambiguous" or not reviewable(resolution): return "untraced" evidence, count, limited = review_evidence( - resolution, profile, version, captured[0] + resolution, profile, version, captured[0], anchored=anchored ) try: inserted = db.record_review_item( @@ -313,6 +388,60 @@ def record_ambiguous_file( return "limited" if limited else "recorded" +def _default_item_to_anchor(db, queued: tuple, scanned, relative: str) -> bool: + """True when the bytes' only pending item is a default item recorded + before #390 and the saver anchor alone resolves the scanned records. + + The file is then captured again so the anchored item supersedes that + pending, undecided item. Decided bytes and bytes already queued under + the current inspection version are never re-read. + """ + if scanned is None or scanned[0] is None: + return False + version = inspection_review_version(scanned[0]) + if version is None or db.review_pending_at(*queued, version): + return False + if not db.review_pending_at(*queued, RESOLVER_VERSION): + return False + return _anchored(scanned[0], _file_name(relative)) is not None + + +def _captured_inspection(metadata: dict, relative: str, scanned) -> tuple | None: + """``(resolution, item_version)`` of the captured records under the + inspection profile, anchored on the file's name; None when the table is + unusable. + + A result tagged ``anchor: "saver"`` means the saver anchor alone + resolves the file (#390); any other resolved result is returned as + ``unresolved`` (no review case, as before). The scan's result is reused + when the capture holds the same records (``inspection_choice`` applies + the same rule). + """ + version = inspection_review_version(metadata) + if version is None: + return None + if ( + scanned is not None + and scanned[1] is not None + and _same_records(metadata, scanned[0]) + ): + return scanned[1], version # the same records: no second run + name = _file_name(relative) + resolution = resolve_inspection(metadata, name) + if resolution.get("state") == "resolved": + # Resolved without the anchor too: no review case, as before. + resolution = _anchor_of(metadata, name, resolution) or { + **resolution, + "state": "unresolved", + } + return resolution, version + + +def _file_name(relative: str) -> str: + """The last part of a ``/``-separated relative path (the saved name).""" + return relative.rsplit("/", 1)[-1] + + def _already_queued(db, locator: str, size: int, mtime_ns: int, versions) -> str: """``duplicate`` while an existing item is pending, else ``decided``.""" for version in versions: @@ -354,14 +483,19 @@ def _queued_spelling(db, root, relative: str, versions) -> tuple[str, tuple | No return spelled, None -def captured_review(store, capture, profile: str = DEFAULT) -> tuple[dict, dict] | None: +def captured_review( + store, capture, profile: str = DEFAULT, file_name: str | None = None +) -> tuple[dict, dict] | None: """``(metadata, resolution)`` of the capture's own bytes, else None. ``metadata`` is what the scan hands the ingester: prompt and workflow JSON-decoded and ``file_info`` (size, dimensions, format) from the capture. ``resolution`` comes from ``profile`` (``DEFAULT`` or - ``INSPECTION``). None when the capture is gone or carries no prompt - metadata. + ``INSPECTION``; the latter anchored on ``file_name``, the file's own + name, as the item was recorded). With ``file_name`` (Apply), a default + result its saver anchor alone resolves (``saver_anchored``) is that + anchored result, so only the anchored branch is still offered. None + when the capture is gone or carries no prompt metadata. """ if store.acquire(capture.token) is None: return None @@ -389,8 +523,15 @@ def captured_review(store, capture, profile: str = DEFAULT) -> tuple[dict, dict] info["format"] = read.format canonical["file_info"] = info if profile == INSPECTION: - return canonical, resolve_inspection(canonical) - return canonical, resolve_prompt_pair(canonical) + named = resolve_inspection(canonical, file_name) + return canonical, _anchor_of(canonical, file_name, named) or named + resolution = resolve_prompt_pair(canonical) + if file_name is not None and resolution.get("state") == "ambiguous": + anchored = _anchored(canonical, file_name) + if anchored is not None: + # Its saver names its branch (#390): only that one is offered. + return canonical, anchored + return canonical, resolution def reviewable(resolution: Any) -> bool: @@ -416,12 +557,14 @@ def review_evidence( profile: str = DEFAULT, version: int | None = None, metadata: Any = None, + anchored: bool = False, ) -> tuple[dict, int, bool]: """``(evidence, candidate_count, limited)`` for one ambiguous result. Inspection evidence also names its ``profile`` and ``semantics``, the table fingerprint of ``metadata``'s graph it was resolved with; - ``version`` is the item's version. + ``version`` is the item's version. ``anchored`` evidence (the saver + anchor's one pair, #390) also names ``anchor: "saver"``. """ base = { "resolver_version": RESOLVER_VERSION if version is None else version, @@ -430,6 +573,8 @@ def review_evidence( if profile == INSPECTION: base["profile"] = INSPECTION base["semantics"] = semantics_version(metadata) + if anchored: + base["anchor"] = SAVER_ANCHOR candidates: list[dict] = [] seen: set[str] = set() total = 0 diff --git a/web/admin.html b/web/admin.html index f80b3565..2f064a3c 100644 --- a/web/admin.html +++ b/web/admin.html @@ -168,10 +168,11 @@ class="px-2.5 py-1 bg-pm-surface border border-pm hover:bg-pm-hover text-pm text-xs font-medium rounded-pm-sm transition-colors"> Settings - + - + - - -
-
- - -
-
-
- Click "Run Diagnostics" to check the gallery system status -
-
-
- - -