Merge remote-tracking branch 'origin/main' into feat/443-bulk-prompt-delete

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

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

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 ---------------------------------------------------------
 

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"]
         )

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)

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;
     }