diff --git a/TESTING.md b/TESTING.md index 21144033..ff09299f 100644 --- a/TESTING.md +++ b/TESTING.md @@ -2961,6 +2961,12 @@ mapped differences"): change within the workspace waits the same way: the shell renders the next route once a video still starting on Inspect settles (`leaveGuard`; the latest route wins). Route changes at `loadstart` + 0–7 ms hung 4–5 of 120 pages before and 0 of 120 after. + The shared viewer's own video gets the same guard (#483): a new selection while the + viewer D155 opened is still starting its video leaves the viewer open, untouched, until + that video reports metadata or an error (or 15 s), then closes it; the URL is revoked + after that, and a viewer asked for meanwhile (the new file's D155 open) opens then. The + merge queue hung once without it (run 40903238387: the PNG selection 12 ms after the + inline video showed, with the viewer's new video still starting). A real page leave cannot wait. With no page code releasing anything, navigating away hung 8 of 120 pages and reload 22 of 120 (3 and 8 of 120 with the page's own release), so that WebKit teardown stays a known limitation; dispose still releases at diff --git a/tests/browser/workspace-inspect.spec.mjs b/tests/browser/workspace-inspect.spec.mjs index 3d77f112..81795e44 100644 --- a/tests/browser/workspace-inspect.spec.mjs +++ b/tests/browser/workspace-inspect.spec.mjs @@ -704,6 +704,147 @@ test('a video replaced while still starting is reset once it settles', async ({ } }); +test('a new selection while the viewer\'s own video is still starting leaves it until it settles (#483)', async ({ context, browserName }) => { + test.setTimeout(180000); + // D155 opens the viewer once the inline preview has settled; the viewer then starts a video of its own. A new + // selection from that video's own `loadstart` (readyState HAVE_NOTHING), or a few milliseconds after it, must not + // reset it or take it off the page while it starts (WebKit, GStreamer: TESTING.md D62; merge-queue run + // 40903238387). The viewer stays open until it reports metadata or an error, then closes; the URL goes last. + for (let attempt = 0; attempt < 10; attempt++) { + const page = await context.newPage(); + try { + if (attempt === 0) await startCoverage(page, browserName); + await page.addInitScript(() => { + const load = HTMLMediaElement.prototype.load; + HTMLMediaElement.prototype.load = function () { if (!this.hasAttribute('src')) { this.__resets = (this.__resets ?? 0) + 1; this.__resetReadyState ??= this.readyState; } return load.call(this); }; + const revoke = URL.revokeObjectURL; + URL.revokeObjectURL = url => { + const starting = window.__starting; + if (url === starting?.url) starting.revoked = { src: starting.node.hasAttribute('src'), connected: starting.node.isConnected, resets: starting.node.__resets ?? 0 }; + return revoke(url); + }; + }); + await openInspect(page); + await page.evaluate(() => { + const input = document.querySelector('#local-file'); + new MutationObserver((records, observer) => { + const video = document.querySelector('#viewer-dialog video'); + if (!video) return; + observer.disconnect(); + const replace = () => { + window.__starting = { node: video, url: video.getAttribute('src'), readyState: video.readyState }; + input.files = window.__pngFiles; input.dispatchEvent(new Event('change', { bubbles: true })); + window.__starting.replaced = { connected: video.isConnected, src: video.hasAttribute('src'), resets: video.__resets ?? 0 }; + }; + video.addEventListener('loadstart', () => { if (window.__delay) setTimeout(replace, window.__delay); else replace(); }, { once: true }); + }).observe(document.body, { childList: true, subtree: true }); + }); + const png = parityPng('f02.png'); + await page.evaluate(bytes => { + const transfer = new DataTransfer(); + transfer.items.add(new File([new Uint8Array(bytes)], 'f02.png', { type: 'image/png' })); + window.__pngFiles = transfer.files; + }, [...png.buffer]); + await page.evaluate(delay => { window.__delay = delay; }, attempt % 5); + await choose(page, localFixture('core-tags.webm', 'video/webm')); + await expect(preview(page).locator('img.local-file-preview')).toBeVisible({ timeout: 8000 }); + await expect.poll(() => page.evaluate(() => window.__starting?.revoked ?? null)).toEqual({ src: false, connected: false, resets: 1 }); + const { readyState, replaced } = await page.evaluate(() => ({ readyState: window.__starting.readyState, replaced: window.__starting.replaced })); + if (attempt % 5 === 0) expect(readyState).toBe(0); + // Starting when replaced: still on the page with its URL, never reset; settled: released at once. + expect(replaced).toEqual(readyState === 0 ? { connected: true, src: true, resets: 0 } : { connected: false, src: false, resets: 1 }); + expect(await page.evaluate(() => window.__starting.node.__resets)).toBe(1); + // Its one reset came after it had metadata, never while it was starting. + expect(await page.evaluate(() => window.__starting.node.__resetReadyState)).toBeGreaterThanOrEqual(1); + // The new file's viewer opens after the old one closed, and the page still answers. + await expect(viewer(page).getByRole('img', { name: 'f02.png', exact: true })).toBeVisible({ timeout: 8000 }); + await expect(viewer(page).locator('video')).toHaveCount(0); + if (attempt === 0) await saveCoverage(page, browserName); + } finally { + await page.close(); + } + } +}); + +/** #483: holds the viewer's video in HAVE_NOTHING (its `loadedmetadata` never fires) so a hold can be driven by hand. */ +async function stallViewerVideo(page) { + await page.addInitScript(() => { + const describe = Object.getOwnPropertyDescriptor(HTMLMediaElement.prototype, 'readyState'); + Object.defineProperty(HTMLMediaElement.prototype, 'readyState', { configurable: true, get() { + return this.closest?.('#viewer-dialog') && window.__stallViewer ? 0 : describe.get.call(this); + } }); + window.addEventListener('loadedmetadata', event => { + if (window.__stallViewer && event.target.closest?.('#viewer-dialog')) event.stopImmediatePropagation(); + }, true); + }); +} + +test('a held viewer closed by hand, or replaced twice, opens only the latest file\'s viewer at once (#483)', async ({ page }) => { + await stallViewerVideo(page); + await openInspect(page); + await page.evaluate(() => { window.__stallViewer = true; }); + await choose(page, localFixture('core-tags.webm', 'video/webm')); + await expect(viewer(page).locator('video')).toBeAttached(); + // Two selections while the viewer's video is (made to look) still starting: the viewer stays, untouched. + await chooser(page).setInputFiles(parityPng('f02.png', 'first.png')); + await chooser(page).setInputFiles(parityPng('f02.png', 'second.png')); + await expect(preview(page).locator('img.local-file-preview')).toBeVisible(); + await expect(viewer(page).locator('video')).toBeAttached(); + // Open file metadata pressed meanwhile (the page behind the viewer is inert, so by script): the latest request. + await action(page).evaluate(node => node.click()); + // Closing it by hand ends the hold at once: the latest file's viewer opens, once, well before D62's 15 s. + await page.keyboard.press('Escape'); + await expect(viewer(page).getByRole('img', { name: 'second.png', exact: true })).toBeVisible({ timeout: 5000 }); + await expect(viewer(page).getByRole('img', { name: 'first.png', exact: true })).toHaveCount(0); + await expect(viewer(page).locator('video')).toHaveCount(0); + await expect(page.locator('dialog[open]')).toHaveCount(1); +}); + +test('a held viewer revokes its URL only once it is gone, drops a stale open, and ends on page leave (#483)', async ({ page }) => { + await stallViewerVideo(page); + await openInspect(page); + await trackReleases(page); + await page.evaluate(() => { window.__stallViewer = true; }); + const videoUrl = () => page.evaluate(() => document.querySelector('#viewer-dialog video')?.getAttribute('src') ?? null); + const released = url => page.evaluate(url => window.__released.filter(item => item.url === url), url); + await choose(page, localFixture('core-tags.webm', 'video/webm')); + await expect(viewer(page).locator('video')).toBeAttached(); + const first = await videoUrl(); + await chooser(page).setInputFiles(parityPng('f02.png', 'held.png')); + await expect(preview(page).locator('img.local-file-preview')).toBeVisible(); + // The session asked to revoke the video's URL; it waits while the held viewer still plays it. + expect(await released(first)).toEqual([]); + // An Open asked for during the hold, then an AVI (which never opens the viewer by itself): the open for the earlier + // file is stale and is dropped when the hold ends. + await action(page).evaluate(node => node.click()); + const avi = { name: 'clip.avi', mimeType: 'video/x-msvideo', buffer: Buffer.concat([Buffer.from('RIFF'), Buffer.alloc(4), Buffer.from('AVI LIST'), Buffer.alloc(64)]) }; + await chooser(page).setInputFiles(avi); + await expect(page.locator('#workspace-toast')).toHaveText(TOAST); + await page.keyboard.press('Escape'); + await expect(page.locator('dialog[open]')).toHaveCount(0); + await expect.poll(() => released(first)).toEqual([{ url: first, live: 0 }]); + // Page leave while held: the viewer goes and the deferred URL is revoked with nothing holding it. + await choose(page, localFixture('core-tags.webm', 'video/webm')); + await expect(viewer(page).locator('video')).toBeAttached(); + const second = await videoUrl(); + await chooser(page).setInputFiles(parityPng('f02.png', 'leave.png')); + await expect(viewer(page).locator('video')).toBeAttached(); + expect(await released(second)).toEqual([]); + await page.evaluate(() => window.dispatchEvent(new PageTransitionEvent('pagehide', { persisted: true }))); + await expect(page.locator('dialog[open]')).toHaveCount(0); + await expect.poll(() => released(second)).toEqual([{ url: second, live: 0 }]); + // Another route while held: the page's dispose ends the hold and revokes the URL with nothing holding it. + await page.evaluate(() => window.dispatchEvent(new PageTransitionEvent('pageshow', { persisted: true }))); + await choose(page, localFixture('core-tags.webm', 'video/webm')); + await expect(viewer(page).locator('video')).toBeAttached(); + const third = await videoUrl(); + await chooser(page).setInputFiles(parityPng('f02.png', 'route.png')); + await expect(viewer(page).locator('video')).toBeAttached(); + await page.evaluate(() => { window.location.hash = '#/library'; }); + await expect(main(page).getByRole('heading', { level: 1, name: 'Prompt library', exact: true })).toBeVisible(); + await expect.poll(() => released(third)).toEqual([{ url: third, live: 0 }]); +}); + test('leaving for another route while a video starts waits for it to settle', async ({ context, browserName }) => { test.setTimeout(180000); // A WebKit (GStreamer) video detached before its pipeline had started deadlocked the page (TESTING.md D62); a diff --git a/tests/test_api_images.py b/tests/test_api_images.py index 0775bcb1..edb3c363 100644 --- a/tests/test_api_images.py +++ b/tests/test_api_images.py @@ -35,6 +35,7 @@ from aiohttp import web # noqa: E402 from aiohttp.test_utils import AioHTTPTestCase # noqa: E402 from PIL import Image # noqa: E402 +from database.models import PromptModel # noqa: E402 from database.operations import PromptDatabase # noqa: E402 import py.api.images as images_module from py.api import gallery_listing # noqa: E402 @@ -92,14 +93,41 @@ class ImageAPITestCase(AioHTTPTestCase): self._anchor_patch.stop() # Windows refuses to unlink a database that still has open handles. self.api.db.close_all() - for path in ( - self._temp_db.name, - self._temp_db.name + "-wal", - self._temp_db.name + "-shm", - ): - if os.path.exists(path): - os.unlink(path) - self._output_tmp.cleanup() + try: + leaked = self._close_leaked_connections() + finally: + for path in ( + self._temp_db.name, + self._temp_db.name + "-wal", + self._temp_db.name + "-shm", + ): + if os.path.exists(path): + os.unlink(path) + self._output_tmp.cleanup() + if leaked: + raise AssertionError( + f"{leaked} connection(s) to the test database were still open at " + "teardown; Windows cannot unlink it (WinError 32). Close every " + "extra PromptDatabase before tearDownAsync, not in a cleanup." + ) + + def _close_leaked_connections(self): + """Close and count connections other models still hold on the temp DB. + + Linux unlinks an open SQLite file, Windows does not; counting here makes + a handle left open past teardown fail on every OS (#475). + """ + key = os.path.normcase(os.path.realpath(self._temp_db.name)) + with PromptModel._instances_lock: + models = list(PromptModel._instances) + leaked = 0 + for model in models: + if model._schema_key() != key: + continue + with model._conn_lock: + leaked += len(model._connections) + model.close_all() + return leaked # -- helpers --------------------------------------------------------- diff --git a/tests/test_api_workspace_restart.py b/tests/test_api_workspace_restart.py index d975c640..d2c80bad 100644 --- a/tests/test_api_workspace_restart.py +++ b/tests/test_api_workspace_restart.py @@ -38,11 +38,25 @@ def _payload(token): class TestRestartKeepsTheOpenPage(ImageAPITestCase): async def get_application(self): + self._restarts = [] app = await super().get_application() for index in range(3): make_png(self.output_dir / f"image_{index}.png") return app + async def tearDownAsync(self): + # Before the base teardown unlinks the database: cleanups run after + # tearDownAsync, and Windows cannot unlink a file still open (#475). + errors = [] + while self._restarts: + try: + await self._restarts.pop()() + except Exception as error: # every closer runs; the first error wins + errors.append(error) + await super().tearDownAsync() + if errors: + raise errors[0] + async def first_page(self, client=None, **params): response = await (client or self.client).get( LISTING, params={"mode": "workspace", "limit": "1", **params} @@ -67,7 +81,7 @@ class TestRestartKeepsTheOpenPage(ImageAPITestCase): api.close_auto_thumbnails() api.db.close_all() - self.addAsyncCleanup(close) + self._restarts.append(close) return api, client async def test_a_restart_keeps_every_held_scope_and_cursor_valid(self): @@ -152,8 +166,13 @@ class TestRestartKeepsTheOpenPage(ImageAPITestCase): other_db = self.comfy_dir / "elsewhere" / "prompts.db" other_db.parent.mkdir() api = PromptManagerAPI(db=PromptDatabase(str(other_db))) - self.addCleanup(api.db.close_all) - self.addCleanup(api.close_auto_thumbnails) + + async def close(): + api.close_auto_thumbnails() + api.db.close_all() + + # Closed before teardown removes the folder that holds its database. + self._restarts.append(close) self.assertNotEqual( api._gallery_root_scope(self.output_dir, 0), page["images"][0]["root_scope"] ) diff --git a/tests/test_run_attribution.py b/tests/test_run_attribution.py index bb4e69dd..63d2199b 100644 --- a/tests/test_run_attribution.py +++ b/tests/test_run_attribution.py @@ -772,6 +772,13 @@ class TestMonitorHistoryRecheck(RecordedRuns, unittest.TestCase): run_attribution.notify_outputs_sealed("run", []) # no dead callback raises +def folder_is_case_sensitive(folder): + """True when ``folder``'s volume tells names apart by case: the existing + folder spelled with swapped case is not found (#476).""" + head, name = os.path.split(folder) + return not os.path.exists(os.path.join(head, name.swapcase())) + + class TestMonitorForeignRuns(RecordedRuns, unittest.TestCase): """#247: outputs of workflows without a PromptManager node, in the monitor.""" @@ -942,11 +949,21 @@ class TestMonitorForeignRuns(RecordedRuns, unittest.TestCase): self.assertEqual(self.handler.awaiting_foreign(), []) def test_the_sealed_reference_spells_the_file_as_history_recorded_it(self): - """Windows: the watcher's spelling differs in case from the run's report.""" - Path(self.output, "Dataset").mkdir() - recorded = self.image("IMG_GEN_00007_.png", subfolder="Dataset") + """Windows: the watcher's spelling differs in case from the run's report. + + On a case-sensitive folder the two spellings are two files and the + folded key joins them; on a case-insensitive one (Windows, default + macOS) they already name one file, so only history's spelling and + the watcher's differ (#476).""" seen = os.path.join(self.output, "dataset", "img_gen_00007_.png") Path(seen).write_bytes(b"png") + recorded = os.path.join(self.output, "Dataset", "IMG_GEN_00007_.png") + self.assertNotEqual(os.path.basename(recorded), os.path.basename(seen)) + if folder_is_case_sensitive(os.path.join(self.output, "dataset")): + Path(self.output, "Dataset").mkdir() + self.image("IMG_GEN_00007_.png", subfolder="Dataset") + else: + self.assertTrue(os.path.exists(recorded)) folded = staticmethod(lambda path: os.path.realpath(path).lower()) with mock.patch.object(ImageGenerationHandler, "_awaiting_key", folded): self.handler.process_new_image(seen) diff --git a/web/js/workspace/inspect.mjs b/web/js/workspace/inspect.mjs index ee67d7ec..2a2ef612 100644 --- a/web/js/workspace/inspect.mjs +++ b/web/js/workspace/inspect.mjs @@ -115,9 +115,39 @@ export function mountInspect(main, { parentScope, api, sender }) { retiring.delete(url); entry.waiting.abort(); clearTimeout(entry.timer); releaseMedia(entry.preview); - if (entry.revoke) URL.revokeObjectURL(url); + if (entry.revoke) revokeObjectURL(url); entry.done(); } + // #483 (D62): the viewer's own video, when a new selection closes the viewer while that video is still starting. + // The viewer stays open until the video reports metadata or an error (or D62's 15 s), then closes; its URL, which + // the session asked to revoke, goes after that. A viewer asked for meanwhile opens then: only the latest request, + // and only if its selection is still current (an automatic open waits for its own inline video again). + let held = null; + function closeViewer() { + const node = document.querySelector('#viewer-dialog video'); + if (!node || !isStarting(node)) { viewer.close(); return; } + const waiting = new AbortController(); + held = { url: node.getAttribute('src'), revoke: false, waiting, pending: null, timer: setTimeout(finishHeld, STARTING_VIDEO_MS) }; + for (const type of ['loadedmetadata', 'error']) node.addEventListener(type, finishHeld, { once: true, signal: waiting.signal }); + } + function endHeld() { + const entry = held; + if (!entry) return null; + held = null; + entry.waiting.abort(); clearTimeout(entry.timer); + return entry; + } + function finishHeld() { + const entry = endHeld(); + if (!entry) return; + viewer.close(); + // Through the session's hook, so an inline video still starting on the same URL keeps it until it settles. + if (entry.revoke) revokeObjectURL(entry.url); + const request = entry.pending; + if (!request || scope.signal.aborted || session.entry !== request.entry || session.state.phase !== 'ready') return; + if (request.auto) autoOpen(request.entry); + else openViewer(request.trigger); + } function retire(preview, url) { const waiting = new AbortController(); const entry = { preview, waiting, revoke: false, timer: setTimeout(() => finishRetiring(url), STARTING_VIDEO_MS) }; @@ -129,6 +159,7 @@ export function mountInspect(main, { parentScope, api, sender }) { } const finishAllRetiring = () => { for (const url of [...retiring.keys()]) finishRetiring(url); }; const revokeObjectURL = url => { + if (held?.url === url) { held.revoke = true; return; } const entry = retiring.get(url); if (entry) entry.revoke = true; else URL.revokeObjectURL(url); @@ -136,7 +167,8 @@ export function mountInspect(main, { parentScope, api, sender }) { const session = createLocalSession({ parentScope: sessionScope, pageTarget: null, revokeObjectURL }); const viewer = createMediaViewer({ api, container: main, parentScope: scope, inspectionController: session.controller, sender, adapter: localFileAdapter, onRestoreFocus: () => { if (open.isConnected) open.focus({ preventScroll: true }); }, - onClose: () => { viewerKey = null; } }); + // A held viewer closed by hand (Escape, Close) ends its hold at once: its video is gone. + onClose: () => { viewerKey = null; if (held) finishHeld(); } }); let viewerKey = null, media = null, mediaUrl = null, statusLine = null, rejectionLine = null, rejectionTimer = 0, announced = null, chosen = null; function statusElement(message) { @@ -209,7 +241,7 @@ export function mountInspect(main, { parentScope, api, sender }) { scope.cleanup(session.subscribe(state => { // D155: a new selection can start while the viewer shows the previous one; it closes first, so no element holds // the old URL when the session revokes it (the new file's viewer opens once it is ready). - if (viewerKey !== null && (state.phase !== 'ready' || session.entry?.key !== viewerKey)) { viewer.close(); viewerKey = null; } + if (viewerKey !== null && (state.phase !== 'ready' || session.entry?.key !== viewerKey)) { closeViewer(); viewerKey = null; } showMedia(state); showStatus(state.message); const entry = session.entry; @@ -220,17 +252,14 @@ export function mountInspect(main, { parentScope, api, sender }) { // while this selection is still the current one and no other dialog is open (the viewer is replaced). D62: // a video opens it once the inline preview has settled, so a replacement while it starts never tears down a // second starting video in the viewer (that deadlocked WebKit). - if (AUTO_OPEN.has(state.message)) whenSettled(media, () => { - if (scope.signal.aborted || session.entry !== entry || session.state.phase !== 'ready' || document.querySelector('dialog[open]:not(#viewer-dialog)')) return; - openViewer(input); - }); + if (AUTO_OPEN.has(state.message)) autoOpen(entry); // An unreadable or failed preview says so in its alert; the toast never claims metadata is available. else if (TOASTABLE.has(state.message)) showWorkspaceToast(INSPECT_COPY.toast); } })); // A replacement or page leave revokes the URL after this subscriber dropped the media; a dispose closes the // viewer (on abort), drops the media, then ends the session. - scope.cleanup(() => viewer.dispose()); + scope.cleanup(() => { const entry = endHeld(); viewer.dispose(); if (entry?.revoke) revokeObjectURL(entry.url); }); scope.cleanup(() => { releaseMedia(media); media = null; mediaUrl = null; }); scope.cleanup(finishAllRetiring); scope.cleanup(() => sessionScope.dispose()); @@ -239,9 +268,11 @@ export function mountInspect(main, { parentScope, api, sender }) { // cleared, so a page restored from the back-forward cache never names a file beside the empty selection. function leave() { clearRejection(); + const entry = endHeld(); viewer.close(); releaseMedia(media); media = null; mediaUrl = null; finishAllRetiring(); + if (entry?.revoke) revokeObjectURL(entry.url); session.clear(); chosen = null; input.value = ''; } @@ -287,10 +318,20 @@ export function mountInspect(main, { parentScope, api, sender }) { for (const type of ['loadedmetadata', 'error']) preview.node.addEventListener(type, done, { once: true, signal: settled.signal }); scope.cleanup(done); } + /** D155: the viewer opens on `entry` once its inline preview has settled, while it is still the current selection. */ + function autoOpen(entry) { + whenSettled(media, () => { + if (scope.signal.aborted || session.entry !== entry || session.state.phase !== 'ready' || document.querySelector('dialog[open]:not(#viewer-dialog)')) return; + // #483: the previous viewer is still closing around its starting video; this one opens once it has. + if (held) { held.pending = { entry, auto: true }; return; } + openViewer(input); + }); + } /** The viewer on the current selection, details showing (D155 amends D97); focus returns to `trigger` on close. */ function openViewer(trigger) { const entry = session.entry; if (!entry || session.state.phase !== 'ready') return; + if (held) { held.pending = { entry, trigger, auto: false }; return; } viewer.open({ items: [entry], selectedKey: entry.key, trigger, details: 'shown' }); viewerKey = entry.key; }