From b0a025a326b523ab88ebb3b41bc71e204e4a91b9 Mon Sep 17 00:00:00 2001 From: Vito Date: Fri, 23 Oct 2026 05:17:10 -0700 Subject: [PATCH 1/2] fix(resolver): read NaN as null in the scan's legacy-parser fallback (#304) ComfyUI stamps "is_changed": [NaN] on nodes that always re-run, the PromptManager node among them. The legacy gallery and admin pages replaced NaN with null before JSON.parse, so they traced these files' prompts. The scan's fallback parsed records strictly, so every NaN-stamped file the resolver could not decide (hires-fix and refiner chains, custom latent nodes) fell to "Needs review" although legacy linked its prompt. parse_page_json parses as the legacy pages did, but only the NaN number reads as null: the word inside prompt text is kept. Infinity still makes the record unreadable, as it did in the pages. --- py/api/admin.py | 11 +-- tests/test_generation_params.py | 45 +++++++++++++ tests/test_pair_scan.py | 116 ++++++++++++++++++++++++++++++++ utils/generation_params.py | 25 +++++++ 4 files changed, 193 insertions(+), 4 deletions(-) diff --git a/py/api/admin.py b/py/api/admin.py index 64104893..2b58b59f 100644 --- a/py/api/admin.py +++ b/py/api/admin.py @@ -41,7 +41,7 @@ try: PLACEHOLDERS, GenerationParamsError, extract_generation_params, - parse_js_json, + parse_page_json, ) from ...utils.prompt_pair_resolver import resolve_prompt_pair except ImportError: @@ -51,7 +51,7 @@ except ImportError: PLACEHOLDERS, GenerationParamsError, extract_generation_params, - parse_js_json, + parse_page_json, ) from utils.prompt_pair_resolver import resolve_prompt_pair @@ -1603,11 +1603,14 @@ class AdminRoutesMixin: @staticmethod def _graph_prompt_text(metadata): - """The positive prompt the browser parser traces from the graphs, or None.""" + """The positive prompt the browser parser traces from the graphs, or None. + + Records parse as the legacy pages parsed them (NaN reads as null). + """ records = {} for key in ("prompt", "workflow"): value = metadata.get(key) - records[key] = parse_js_json(value) if isinstance(value, str) else value + records[key] = parse_page_json(value) if isinstance(value, str) else value try: positive = extract_generation_params(records)["positivePrompt"] except GenerationParamsError: diff --git a/tests/test_generation_params.py b/tests/test_generation_params.py index 990d3a60..507cea29 100644 --- a/tests/test_generation_params.py +++ b/tests/test_generation_params.py @@ -5,7 +5,10 @@ returns for the same input (tests/test_generation_params_js_parity.py checks that directly when Node is available). """ +import json import os +import shutil +import subprocess # nosec B404 - runs a fixed Node one-liner import sys import unittest @@ -17,6 +20,7 @@ from utils.generation_params import ( # noqa: E402 extract_generation_params, extract_from_records, parse_js_json, + parse_page_json, ) try: @@ -219,6 +223,47 @@ class ParsesLikeJavaScript(unittest.TestCase): run("seed-beyond-double-precision")["seed"], 4974052037430000001.0 ) + def test_page_parse_reads_nan_numbers_as_null(self): + # gallery.js and admin.js replace NaN with null before JSON.parse. + self.assertEqual( + parse_page_json('{"a": NaN, "b": [NaN], "t": "NaN"}'), + {"a": None, "b": [None], "t": "NaN"}, + ) + self.assertEqual( + parse_page_json('{"seed": 9007199254740993}'), {"seed": 9007199254740992.0} + ) + for text in ('{"a": Infinity}', "[-Infinity]", "not json", "", None): + with self.subTest(text=text): + self.assertIsNone(parse_page_json(text)) + + @unittest.skipUnless(shutil.which("node"), "Node is not installed") + def test_page_parse_equals_the_legacy_pages_for_nan_numbers(self): + # gallery.js/admin.js: two NaN regexes, then JSON.parse (Infinity throws). + texts = [ + '{"1": {"is_changed": [NaN]}, "2": {"is_changed": NaN, "v": 1.5}}', + '{"a":NaN,"b":[NaN, 2]}', + '{"a": Infinity}', + '{"seed": 9007199254740993}', + ] + script = ( + "const texts = JSON.parse(require('fs').readFileSync(0, 'utf8'));" + "console.log(JSON.stringify(texts.map((t) => { try { return {v: JSON.parse(" + "t.replace(/:\\s*NaN\\b/g, ': null').replace(/\\bNaN\\b/g, 'null'))}; }" + " catch (e) { return null; } })));" + ) + completed = subprocess.run( # nosec B603 B607 - fixed argv, no shell + ["node", "-e", script], + input=json.dumps(texts), + capture_output=True, + text=True, + check=True, + timeout=60, + ) + expected = json.loads(completed.stdout) + for text, page in zip(texts, expected): + with self.subTest(text=text): + self.assertEqual(parse_page_json(text), page and page["v"]) + def test_duplicate_keys_keep_the_last_value(self): self.assertEqual(parse_js_json('{"a": 1, "a": 2}'), {"a": 2}) diff --git a/tests/test_pair_scan.py b/tests/test_pair_scan.py index 8b65fb67..b8d914d1 100644 --- a/tests/test_pair_scan.py +++ b/tests/test_pair_scan.py @@ -165,6 +165,122 @@ class TestPairScan(AdminAPITestCase): ) self.assertEqual(self.api.db.get_recent_prompts()["prompts"], []) + def _nan_scanned(self, name, prompt_graph): + """Scan ``prompt_graph`` as ComfyUI writes it: ``is_changed`` is NaN.""" + stamped = { + node_id: {**node, "is_changed": [float("nan")]} + for node_id, node in prompt_graph.items() + } + text = json.dumps(stamped) + self.assertIn("NaN", text) + return self._scanned_text(name, text) + + def _scanned_text(self, name, prompt_text): + meta = {"prompt": prompt_text} + return self.api._ingest_scanned_file(self.output_dir / name, meta) + + @staticmethod + def _hires_fix_graph(positive): + """A PromptManager prompt on a base sampler and a hires-fix sampler. + + The base latent comes from a custom batch node and the hires pass from + a custom image filter: no sampler branch traces, so the scan links the + prompt the legacy pages show. + """ + return { + "1": { + "class_type": "CheckpointLoaderSimple", + "inputs": {"ckpt_name": "m.safetensors"}, + }, + "2": { + "class_type": "MyTriggerWords", + "inputs": {"trigger_words": ["1", 2]}, + }, + "3": { + "class_type": "PromptManager", + "inputs": { + "text": positive, + "prepend_text": ["2", 0], + "append_text": "", + "clip": ["1", 1], + }, + }, + "4": { + "class_type": "CLIPTextEncode", + "inputs": {"text": "blur", "clip": ["1", 1]}, + }, + "5": {"class_type": "MyLatentBatch", "inputs": {"batch_size": 6}}, + "6": { + "class_type": "KSampler", + "inputs": { + "seed": 7, + "model": ["1", 0], + "positive": ["3", 0], + "negative": ["4", 0], + "latent_image": ["5", 0], + }, + }, + "7": { + "class_type": "VAEDecode", + "inputs": {"samples": ["6", 0], "vae": ["1", 2]}, + }, + "8": {"class_type": "MyImageFilter", "inputs": {"images": ["7", 0]}}, + "9": { + "class_type": "VAEEncode", + "inputs": {"pixels": ["8", 0], "vae": ["1", 2]}, + }, + "14": { + "class_type": "KSampler", + "inputs": { + "seed": 7, + "denoise": 0.25, + "model": ["1", 0], + "positive": ["3", 0], + "negative": ["4", 0], + "latent_image": ["9", 0], + }, + }, + "15": { + "class_type": "VAEDecode", + "inputs": {"samples": ["14", 0], "vae": ["1", 2]}, + }, + "16": {"class_type": "SaveImage", "inputs": {"images": ["15", 0]}}, + } + + def test_hires_fix_prompt_links_with_or_without_nan_is_changed(self): + # Legacy parity: the legacy pages read NaN as null before parsing, so they + # showed this prompt; the scan left NaN-stamped files in review. + for name, scan in (("plain.png", None), ("stamped.png", "nan")): + with self.subTest(name=name): + g = self._hires_fix_graph("cat on a sofa") + if scan: + status = self._nan_scanned(name, g) + else: + status = self._scanned(name, g) + self.assertIn(status, ("added", "linked")) + self.assertEqual(self._texts(), ["cat on a sofa"]) + + def test_untraced_single_sampler_links_the_legacy_prompt_with_nan(self): + g = self._hires_fix_graph("cat on a sofa") + for node_id in ("7", "8", "9", "14", "15"): + del g[node_id] + g["16"]["inputs"]["images"] = ["6", 0] + self.assertEqual(self._nan_scanned("single.png", g), "added") + self.assertEqual(self._texts(), ["cat on a sofa"]) + + def test_nan_inside_prompt_text_is_kept(self): + # Only the NaN number reads as null; legacy's regex also rewrote text. + g = self._hires_fix_graph("NaN cat, value: NaN") + self.assertEqual(self._nan_scanned("text.png", g), "added") + self.assertEqual(self._texts(), ["NaN cat, value: NaN"]) + + def test_infinity_is_still_unreadable_like_the_legacy_pages(self): + text = json.dumps(self._hires_fix_graph("cat")).replace( + '"seed": 7', '"seed": Infinity' + ) + self.assertIn(self._scanned_text("inf.png", text), ("ambiguous", "unresolved")) + self.assertEqual(self._texts(), []) + def test_raw_png_workflow_is_stored_as_object_and_repeat_is_unchanged(self): path = self.output_dir / "workflow.png" workflow = {"nodes": [{"id": 1, "type": "PromptManagerPair"}]} diff --git a/utils/generation_params.py b/utils/generation_params.py index e65ecd1e..42b7c9b3 100644 --- a/utils/generation_params.py +++ b/utils/generation_params.py @@ -28,6 +28,7 @@ __all__ = [ "extract_generation_params", "extract_from_records", "parse_js_json", + "parse_page_json", ] PLACEHOLDERS = MappingProxyType( @@ -953,6 +954,30 @@ def parse_js_json(text): return None +def _nan_as_null(name): + """The legacy pages' NaN cleaning; Infinity still throws in ``JSON.parse``.""" + if name == "NaN": + return None + raise ValueError(f"{name} is not JSON") + + +def parse_page_json(text): + """A metadata record as the legacy gallery and admin pages parsed it. + + They replace ``NaN`` with ``null`` and then ``JSON.parse`` (ComfyUI writes + ``"is_changed": [NaN]`` for nodes that always re-run, PromptManager among + them). Only the NaN number reads as null here: the pages' regex also + rewrote the word inside strings, which changed prompt text. None where the + parse throws. + """ + if not isinstance(text, str): + return None + try: + return json.loads(text, parse_constant=_nan_as_null, parse_int=_double_integer) + except (ValueError, RecursionError): + return None + + def extract_from_records(prompt_text, workflow_text): """``extract_generation_params`` for raw ``prompt``/``workflow`` record text. From 88e5b78207bc1581103c16b698cc2e6afd8e5974 Mon Sep 17 00:00:00 2001 From: Vito Date: Fri, 23 Oct 2026 05:42:59 -0700 Subject: [PATCH 2/2] fix(gallery): check duplicate deletes in the scanned root and refresh the listing (#305) The duplicate cleanup workflow failed in CI with the deleted file still served (200). The delete was real: the scan reads only the output root, and the browser fixture's input root holds a different shared/same.png. The test checked a bare /images/serve/ URL, which tries every gallery root, so after the output copy went it answered with the input copy. That only happened when the kernel stamped a bulk copy a fraction of a millisecond before the wall-clock stamp on shared/same.png, which made shared/same.png a Delete candidate (CI trace: 0000 .8605533, same .8606777, 0001 .8618972). The workflow now checks each file in its own root (?root=N), checks the input copy survives, and a new workflow forces the CI order through the scan response so the collision runs every time. The server log in that failure also showed the gallery listing a file the cleanup had deleted: delete_duplicate_images left the 30 s listing cache, so the gallery kept counting the file. A successful delete now invalidates the gallery cache. --- py/api/admin.py | 3 + tests/browser/workflows.spec.mjs | 100 +++++++++++++++++++++++-------- tests/test_api_admin.py | 17 ++++++ 3 files changed, 96 insertions(+), 24 deletions(-) diff --git a/py/api/admin.py b/py/api/admin.py index 2b58b59f..dc4e9bcb 100644 --- a/py/api/admin.py +++ b/py/api/admin.py @@ -804,6 +804,9 @@ class AdminRoutesMixin: result = await self._run_in_executor( self._delete_duplicate_images_sync, image_paths, output_dir ) + if result["deleted_count"]: + # The cached listing would keep (and count) the deleted files. + self.invalidate_gallery_cache() response_data = { "success": True, diff --git a/tests/browser/workflows.spec.mjs b/tests/browser/workflows.spec.mjs index 0ee4620c..ad42f378 100644 --- a/tests/browser/workflows.spec.mjs +++ b/tests/browser/workflows.spec.mjs @@ -354,39 +354,48 @@ test('prompt deletion requires confirmation and can retry after a network failur } }); -test('duplicate cleanup cancels safely and deletes only the selected file after a retry', async ({ page, request }) => { +// The duplicate scan reads the output root only, but the fixture's input root holds a different +// shared/same.png. A bare /images/serve/ URL tries every gallery root in turn, so once the +// output copy is deleted it answers 200 with the input copy: check each file in its own root. +const serveIn = (root, path) => `/prompt_manager/images/serve/${path.split(/[\\/]/).map(encodeURIComponent).join('/')}?root=${root}`; +const rootOf = url => new URL(url, 'http://gallery.test').searchParams.get('root'); + +// Which copy the scan keeps depends on file times: the fixture stamps shared/same.png with the +// wall clock, and the kernel can stamp its bulk copies a fraction of a millisecond earlier. +// listOrder rewrites the finished scan's groups so a test can pick the order it needs. +async function openDuplicateScan(page, request, listOrder = null) { await expect(await request.post('/__test__/reset', { data: { extra_images: 2 } })).toBeOK(); + if (listOrder) { + await page.route('**/prompt_manager/scan_duplicates/stream', async route => { + const response = await route.fetch(); + const body = (await response.text()).replace(/^data: (.*)$/gm, (line, json) => { + const event = JSON.parse(json); + if (event.type !== 'complete') return line; + for (const group of event.duplicates) group.images = listOrder(group.images); + return `data: ${JSON.stringify(event)}`; + }); + await route.fulfill({ response, body }); + }); + } await page.goto('/prompt_manager/gallery'); await expect(page.locator('#galleryGrid .image-item')).toHaveCount(4); const originals = await page.locator('#galleryGrid img[data-original]').evaluateAll(images => images.map(image => image.dataset.original)); + const output = rootOf(originals.find(url => url.includes('/bulk/'))); + const input = rootOf(originals.find(url => url.includes('/shared/same.png') && rootOf(url) !== output)); + expect(output).not.toBeNull(); + expect(input).not.toBeNull(); await page.locator('#settingsBtn').click(); await page.locator('#scanDuplicatesBtn').click(); await expect(page.locator('#duplicatesCount')).toHaveText('1'); const candidates = page.locator('.duplicate-checkbox'); await expect(candidates).toHaveCount(2); - const selected = await candidates.first().getAttribute('data-image-path'); - const retained = await candidates.last().getAttribute('data-image-path'); - const serve = path => `/prompt_manager/images/serve/${path.split(/[\\/]/).map(encodeURIComponent).join('/')}`; - await candidates.first().check(); - let deletes = 0; - page.on('request', request => { if (request.url().endsWith('/delete_duplicate_images')) deletes++; }); - confirmNextDialog(page, false, 'permanently delete 1 duplicate image files'); - await page.locator('#removeDuplicatesBtn').click(); - expect(deletes).toBe(0); - await expect(await request.get(serve(selected))).toBeOK(); - await page.route('**/prompt_manager/delete_duplicate_images', route => route.abort('failed'), { times: 1 }); - confirmNextDialog(page, true, 'permanently delete 1 duplicate image files'); - await page.locator('#removeDuplicatesBtn').click(); - await expect(page.getByText('Failed to delete duplicate images', { exact: true })).toBeVisible(); - await expect(page.locator('#removeDuplicatesBtn')).toBeEnabled(); - await expect(await request.get(serve(selected))).toBeOK(); - confirmNextDialog(page, true, 'permanently delete 1 duplicate image files'); - const deleted = page.waitForResponse(response => response.url().endsWith('/delete_duplicate_images')); - await page.locator('#removeDuplicatesBtn').click(); - expect((await (await deleted).json())).toMatchObject({ success: true, deleted_count: 1, failed_count: 0 }); - await expect(page.locator('#duplicatesModal')).toBeHidden(); - expect((await request.get(serve(selected))).status()).toBe(404); - await expect(await request.get(serve(retained))).toBeOK(); + return { originals, output, input, candidates }; +} + +async function expectOnlySelectedGone(page, request, { originals, output, input }, selected, retained) { + expect((await request.get(serveIn(output, selected))).status()).toBe(404); + await expect(await request.get(serveIn(output, retained))).toBeOK(); + await expect(await request.get(serveIn(input, 'shared/same.png'))).toBeOK(); await page.reload(); await expect(page.locator('#galleryGrid .image-item')).toHaveCount(3); const remaining = await page.locator('#galleryGrid img[data-original]').evaluateAll(images => images.map(image => image.dataset.original)); @@ -395,4 +404,47 @@ test('duplicate cleanup cancels safely and deletes only the selected file after expect(originals).toContain(url); await expect(await request.get(url)).toBeOK(); } +} + +test('duplicate cleanup cancels safely and deletes only the selected file after a retry', async ({ page, request }) => { + const scan = await openDuplicateScan(page, request); + const { output, candidates } = scan; + const selected = await candidates.first().getAttribute('data-image-path'); + const retained = await candidates.last().getAttribute('data-image-path'); + await candidates.first().check(); + let deletes = 0; + page.on('request', request => { if (request.url().endsWith('/delete_duplicate_images')) deletes++; }); + confirmNextDialog(page, false, 'permanently delete 1 duplicate image files'); + await page.locator('#removeDuplicatesBtn').click(); + expect(deletes).toBe(0); + await expect(await request.get(serveIn(output, selected))).toBeOK(); + await page.route('**/prompt_manager/delete_duplicate_images', route => route.abort('failed'), { times: 1 }); + confirmNextDialog(page, true, 'permanently delete 1 duplicate image files'); + await page.locator('#removeDuplicatesBtn').click(); + await expect(page.getByText('Failed to delete duplicate images', { exact: true })).toBeVisible(); + await expect(page.locator('#removeDuplicatesBtn')).toBeEnabled(); + await expect(await request.get(serveIn(output, selected))).toBeOK(); + confirmNextDialog(page, true, 'permanently delete 1 duplicate image files'); + const deleted = page.waitForResponse(response => response.url().endsWith('/delete_duplicate_images')); + await page.locator('#removeDuplicatesBtn').click(); + expect((await (await deleted).json())).toMatchObject({ success: true, deleted_count: 1, failed_count: 0 }); + await expect(page.locator('#duplicatesModal')).toBeHidden(); + await expectOnlySelectedGone(page, request, scan, selected, retained); +}); + +test('deleting a duplicate whose path another gallery root also holds removes only the scanned copy', async ({ page, request }) => { + // The order CI saw: a bulk copy stamped first, so shared/same.png is the first Delete candidate. + const ciOrder = ['bulk/0000.png', 'shared/same.png', 'bulk/0001.png']; + const rank = image => ciOrder.indexOf(image.path.replace(/\\/g, '/')); + const scan = await openDuplicateScan(page, request, images => [...images].sort((a, b) => rank(a) - rank(b))); + const selected = await scan.candidates.first().getAttribute('data-image-path'); + const retained = await scan.candidates.last().getAttribute('data-image-path'); + expect(selected.replace(/\\/g, '/')).toBe('shared/same.png'); + await scan.candidates.first().check(); + confirmNextDialog(page, true, 'permanently delete 1 duplicate image files'); + const deleted = page.waitForResponse(response => response.url().endsWith('/delete_duplicate_images')); + await page.locator('#removeDuplicatesBtn').click(); + expect((await (await deleted).json())).toMatchObject({ success: true, deleted_count: 1, failed_count: 0 }); + await expect(page.locator('#duplicatesModal')).toBeHidden(); + await expectOnlySelectedGone(page, request, scan, selected, retained); }); diff --git a/tests/test_api_admin.py b/tests/test_api_admin.py index 61d703b1..b34c0df9 100644 --- a/tests/test_api_admin.py +++ b/tests/test_api_admin.py @@ -868,6 +868,23 @@ class TestDeleteDuplicatesEndpoint(AdminAPITestCase): self.assertFalse(target.exists()) self.assertTrue(thumb_dir.exists()) + async def test_deleted_files_leave_the_cached_gallery_listing(self): + # The listing is cached for 30 s; a delete must not leave the gallery + # listing (and counting) a file that is gone. + GalleryConfig.MONITORING_DIRECTORIES = [self._root(self.output_dir)] + for name in ("keep.png", "gone.png"): + (self.output_dir / name).write_bytes(b"same bytes") + + async def listed(): + resp = await self.client.get("/prompt_manager/images/output") + data = await resp.json() + return data["total"], sorted(i["filename"] for i in data["images"]) + + self.assertEqual(await listed(), (2, ["gone.png", "keep.png"])) + resp = await self._post({"image_paths": ["gone.png"]}) + self.assertEqual((await resp.json())["deleted_count"], 1) + self.assertEqual(await listed(), (1, ["keep.png"])) + def test_unexpected_error_per_file_is_reported_without_path(self): self.api._delete_one_duplicate = lambda *a: (_ for _ in ()).throw( PermissionError(13, "Permission denied", os.path.join(self.tmpdir, "x.png"))