diff --git a/TESTING.md b/TESTING.md index ce4bd6b2..d66064e8 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: @@ -4099,6 +4103,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 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/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/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/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/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_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/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 6e13dede..7c70d876 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 resolve_text, run_prompt_nodes from .prompt_pair_resolver import resolve_prompt_pair @@ -194,8 +195,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: @@ -211,7 +213,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/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/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 -
-
-
- - -