From b4aa25a3e7d301dfda52cdbeb3a046d7858e3697 Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Thu, 22 Oct 2026 19:17:53 -0700 Subject: [PATCH] fix(inspect): abortable, stepped local reads with a busy state The wait for the node table now ends with the selection's signal. A local read resolves and projects in bounded steps with a turn of the event loop between them, and the inspector carries aria-busy while it loads. A budget test covers maximal 2 MiB graphs and checks the stepped read is byte-identical to one synchronous build. The D117 wrap rule now targets only the summary button, and TESTING.md scopes the restart line to PRs L1 and L2. --- TESTING.md | 18 ++- tests/browser/full-inspector.spec.mjs | 3 +- tests/browser/inspector-concept.spec.mjs | 2 +- tests/js/workspace-file-inspector.test.js | 30 +++++ tests/js/workspace-local-inspection.test.js | 120 ++++++++++++++++++++ tests/js/workspace-local-session.test.js | 4 +- web/js/workspace/file-inspector.mjs | 6 +- web/js/workspace/local-inspection.mjs | 44 ++++++- web/js/workspace/workspace.css | 2 +- 9 files changed, 214 insertions(+), 15 deletions(-) diff --git a/TESTING.md b/TESTING.md index 1008d651..123bdc7c 100644 --- a/TESTING.md +++ b/TESTING.md @@ -1588,7 +1588,8 @@ AVI/WMV preview only), chosen or dropped, previews inline. The heading's one act the shared viewer with the full inspector on a record read entirely in this browser (`web/js/workspace/inspect.mjs`, `local-session.mjs`, `local-inspection.mjs` and the L1 readers `png-metadata.js`, `local-video.mjs`, `local-projection.mjs`). Nothing is -uploaded, imported or stored. Only HTML and JS change, so no ComfyUI restart is needed. +uploaded, imported or stored. PRs L1 and L2 change only HTML and JS, so they need no +ComfyUI restart; PR L3 does (below). Local roles (#249 PR L3, decision LI-1): the prompts and resolution of a local record are the server inspector's for the same records. `web/js/workspace/local-resolver.mjs` (with @@ -1610,7 +1611,20 @@ vector (prompts, resolution and facts), the golden and selection record sets bound and 1,200 seeded fuzz cases; `tests/fixtures/local-inspect/resolver-vectors.json` holds the committed answers the Node unit test reads. The browser spec `chooser shows the Images roles and resolution for every parity PNG` compares the rail with Images for all -25 committed parity PNGs. The static route change needs a ComfyUI restart. +25 committed parity PNGs. The static route change and the resolver fixes (Python) need a +ComfyUI restart. + +Main thread (M4): the resolver runs on the page's thread, not a worker. On a desktop +(Node 26) `resolveLocalEvidence` on maximal 2 MiB graphs (a 19,990-sampler chain, 6,600 +independent samplers, a consumer-dense scan, a 100,000-node PromptManager workflow) took +130–455 ms; the server took 80–905 ms on the same inputs. `localFileAdapter.read` runs in +bounded steps (read, table, parse and resolve, project, fit) with a turn of the event +loop between them, every wait ends with the selection's signal (a stalled table request +included), and the inspector carries `aria-busy="true"` with "Loading file details…" +until the answer lands. `tests/js/workspace-local-inspection.test.js` pins a 2,500 ms +budget per step on maximal independent-sampler and stage-chain graphs, checks that the +stepped read answers byte for byte as one synchronous build, and that an abort at any +step ends the read. Bounds (the server's own budgets, ported by PR L1; see "Follow-up: local Inspect parity" under "Chronological Library history" for the known divergences): diff --git a/tests/browser/full-inspector.spec.mjs b/tests/browser/full-inspector.spec.mjs index 1b40673a..e4efc5a7 100644 --- a/tests/browser/full-inspector.spec.mjs +++ b/tests/browser/full-inspector.spec.mjs @@ -240,8 +240,9 @@ test('viewer details hold provenance, raw records and downloads',async({page,req // The panel content without the cover, D117's Copy settings summary after the facts, then the two disclosures, // then the pair send and history buttons (D20). expect(await viewerStructure(details)).toEqual({outer:['div','button.send','button'], - inner:['div.panel-heading','p.small','div.tags','section.prompt-section.positive','section.prompt-section.negative','div.tags','dl.meta-grid','button','details','details']}); + inner:['div.panel-heading','p.small','div.tags','section.prompt-section.positive','section.prompt-section.negative','div.tags','dl.meta-grid','button.settings-summary','details','details']}); await expect(details.locator(':scope > div > button')).toHaveText('Copy settings summary'); + await expect(details.locator(':scope > div > button.settings-summary')).toHaveCSS('white-space','normal'); await expect(details.locator('.detail-cover,.detail-inner,.detail-footer')).toHaveCount(0); await expect(details.locator('.panel-heading h2')).toHaveText(POSITIVE_TITLE);await expect(details.locator('.panel-heading .chip')).toHaveText('PNG'); await expect(details.locator(':scope > div > p.small')).toHaveText('complete.png'); diff --git a/tests/browser/inspector-concept.spec.mjs b/tests/browser/inspector-concept.spec.mjs index 41ae63a2..0a43d7f1 100644 --- a/tests/browser/inspector-concept.spec.mjs +++ b/tests/browser/inspector-concept.spec.mjs @@ -119,7 +119,7 @@ async function plainViewerChips(page) { }); const summary=document.querySelector('#viewer-details [data-action="download-workflow"]').cloneNode(false); for(const key of Object.keys(summary.dataset))delete summary.dataset[key]; - summary.textContent='Copy settings summary'; + summary.textContent='Copy settings summary';summary.classList.add('settings-summary'); document.querySelector('#viewer-details > div > dl.meta-grid').after(summary);window.__summaryButton=summary; }); await settle(page); diff --git a/tests/js/workspace-file-inspector.test.js b/tests/js/workspace-file-inspector.test.js index e4c8bf39..6371f59c 100644 --- a/tests/js/workspace-file-inspector.test.js +++ b/tests/js/workspace-file-inspector.test.js @@ -303,6 +303,8 @@ test('Copy settings summary follows the facts in the full inspector only', async const inner = full.button.parent, index = inner.children.indexOf(full.button); assert.equal(inner.children[index - 1].tagName, 'dl', 'it follows the six facts'); assert.equal(inner.children[index + 1].tagName, 'details', 'File & provenance follows it'); + // M7: the D117 wrap rule targets this button by its class, never any button of a details block. + assert.equal(full.button.className, 'settings-summary'); } finally { full.done(); } const panel = renderSummary(record(), {variant:'panel'}); try { assert.equal(panel.button, null, 'the Images side panel keeps the concept footer'); } finally { panel.done(); } @@ -333,3 +335,31 @@ test('Copy settings summary without a clipboard opens the select-to-copy fallbac } finally { view.done(); } } }); + +test('the inspector is busy while a read is loading, then not', async () => { + const f = mountedDom(), scope = createScope(); + const view = mountFileInspector(f.container, {sender:{open() {}}, parentScope:scope, variant:'full'}); + try { + view.setState({loading:true, error:false, record:null}); + assert.equal(f.container.getAttribute('aria-busy'), 'true'); + assert.ok(f.walk(f.container, n => n.textContent === 'Loading file details…').length); + view.setState({loading:false, error:false, record:record()}); + assert.equal(f.container.getAttribute('aria-busy'), null); + view.setState({loading:true, error:false, record:null}); + view.setState({loading:false, error:true, record:null}); + assert.equal(f.container.getAttribute('aria-busy'), null); + view.setState({loading:true, error:false, record:null}); + } finally { + view.dispose(); + assert.equal(f.container.getAttribute('aria-busy'), null, 'dispose clears the busy state'); + scope.dispose(); f.restore(); + } +}); + +test('the D117 wrap rule targets only the settings summary button', async () => { + const fs = await import('node:fs'); + const css = fs.readFileSync(new URL('../../web/js/workspace/workspace.css', import.meta.url), 'utf8'); + const rules = css.split('\n').filter(line => line.includes('D117')); + assert.deepEqual(rules.map(line => line.trim()), ['.ca-tools #viewer-details .settings-summary { white-space: normal; /* D117 */ }']); + assert.doesNotMatch(css, /#viewer-details > div > button/); +}); diff --git a/tests/js/workspace-local-inspection.test.js b/tests/js/workspace-local-inspection.test.js index b8e6e58f..2994f9e7 100644 --- a/tests/js/workspace-local-inspection.test.js +++ b/tests/js/workspace-local-inspection.test.js @@ -23,8 +23,11 @@ globalThis.ComfyMetadata = ComfyMetadata; // The page's API client and the node table it serves (local-semantics.mjs fetches it once). const SEMANTICS = fs.readFileSync(path.join(here, "..", "..", "web", "js", "inspection-semantics.json"), "utf8"); globalThis.window = { ApiClient: { resolveUrl: (value) => value } }; +// A test can hold the table request (`tableGate`) to stand for a slow or stalled proxy. +let tableGate = null; globalThis.fetch = async (url) => { if (url !== "/prompt_manager/js/inspection-semantics.json") throw new Error(`unexpected request ${url}`); + if (tableGate) await tableGate; return new Response(SEMANTICS, { status: 200 }); }; const m = await import("../../web/js/workspace/local-inspection.mjs").catch((error) => { @@ -66,6 +69,37 @@ test("contract", () => { assert.ok(Object.isFrozen(m.localFileAdapter)); }); +// M3: the first read waits for the page's one table request; an aborted, replaced or cleared selection must not. +test("a read waiting for the node table ends with its selection", async () => { + let release; + tableGate = new Promise((resolve) => { release = resolve; }); + try { + const file = pngFile("slow.png", [tEXt("prompt", PROMPT)]); + const aborted = new AbortController(); + const reading = m.localFileAdapter.read(await handleFor(file), { signal: aborted.signal }); + const settled = (promise) => Promise.race([promise.then(() => "resolved", (error) => error.name), new Promise((resolve) => setTimeout(() => resolve("pending"), 200))]); + await new Promise((resolve) => setTimeout(resolve, 20)); + aborted.abort(); + assert.equal(await settled(reading), "AbortError"); + // Through the controller: clearing the selection ends its read; nothing is published for it. + const controller = createInspectionController({ api: null, adapter: m.localFileAdapter, extractFacts: extractGenerationFacts }); + const states = []; + controller.subscribe((state) => states.push(state)); + const selected = controller.select(await handleFor(file)); + await new Promise((resolve) => setTimeout(resolve, 20)); + controller.clear(); + assert.equal(await settled(selected), "resolved"); + assert.ok(states.every((state) => state.record === null && state.error === false), "no record or error for a cleared selection"); + controller.dispose(); + } finally { + tableGate = null; + release(); + } + // The shared request still lands for the next selection. + const { wire } = await recordFor(pngFile("next.png", [tEXt("prompt", PROMPT)])); + assert.equal(wire.resolution.state, "resolved"); +}); + test("a PNG File becomes the server-shaped wire with local provenance", async () => { const file = pngFile("Fox Render.PNG", [tEXt("prompt", PROMPT), tEXt("workflow", WORKFLOW)]); const { wire, record, handle } = await recordFor(file); @@ -345,3 +379,89 @@ test("the parity corpus PNGs read into renderable records", async () => { assert.ok(["parsed", "limited", "malformed"].includes(wire.metadata.prompt_graph.status)); } }); + +// M4: resolution runs on the page's main thread, so a read yields between its bounded steps (the busy state paints +// and input is handled) and no step may exceed a budget on the largest record the readers keep (2 MiB of roles). +const ROLE_BYTES = 2 * 1024 * 1024; +// Generous for a shared CI runner: a desktop resolves these in 130-455 ms. +const STEP_BUDGET_MS = 2500; +const ENCODE = (text) => ({ class_type: "CLIPTextEncode", inputs: { text, clip: ["4", 1] } }); +const KSAMPLER = (positive, negative, latent) => ({ class_type: "KSampler", inputs: { seed: 1, steps: 20, cfg: 8.0, sampler_name: "euler", scheduler: "normal", + denoise: 1.0, model: ["4", 0], positive, negative, latent_image: latent } }); +const LOADERS = { "4": { class_type: "CheckpointLoaderSimple", inputs: { ckpt_name: "m.safetensors" } }, "5": { class_type: "EmptyLatentImage", inputs: { width: 64, height: 64, batch_size: 1 } } }; +/** The largest graph of this shape whose JSON fits the role budget. */ +function maximal(build, high) { + let low = 1; + while (low < high) { + const middle = (low + high + 1) >> 1; + if (Buffer.byteLength(JSON.stringify(build(middle)), "utf8") <= ROLE_BYTES) low = middle; + else high = middle - 1; + } + return JSON.stringify(build(low)); +} +const MAXIMAL_GRAPHS = { + // Many candidates: every sampler is a final stage with its own pair. + independent: maximal((count) => { + const graph = { ...LOADERS }; + for (let index = 0; index < count; index++) { + graph[`s${index}`] = KSAMPLER([`p${index}`, 0], [`n${index}`, 0], ["5", 0]); + graph[`p${index}`] = ENCODE(`fox ${index}`); + graph[`n${index}`] = ENCODE(`blur ${index}`); + } + return graph; + }, 6666), + // One long stage chain through the latent: the deepest stage walk. + chain: maximal((count) => { + const graph = { ...LOADERS, "6": ENCODE("fox"), "7": ENCODE("blur") }; + for (let index = 0; index < count; index++) graph[`s${index}`] = KSAMPLER(["6", 0], ["7", 0], index ? [`s${index - 1}`, 0] : ["5", 0]); + return graph; + }, 19996), +}; + +test("resolution of a maximal 2 MiB graph stays within the step budget", async () => { + const { resolveLocalEvidence } = await import("../../web/js/workspace/local-resolver.mjs"); + const semantics = await (await import("../../web/js/workspace/local-semantics.mjs")).loadInspectionSemantics(); + for (const [name, prompt] of Object.entries(MAXIMAL_GRAPHS)) { + assert.ok(Buffer.byteLength(prompt, "utf8") > ROLE_BYTES - 1024, `${name} fills the role budget`); + const started = performance.now(); + const evidence = resolveLocalEvidence({ prompt }, semantics); + const elapsed = performance.now() - started; + assert.ok(elapsed < STEP_BUDGET_MS, `${name}: ${Math.round(elapsed)} ms`); + assert.notEqual(evidence.resolution.reason_code, "malformed", name); + } +}); + +test("a read yields between bounded steps and answers byte for byte as one synchronous build", async () => { + const semantics = await (await import("../../web/js/workspace/local-semantics.mjs")).loadInspectionSemantics(); + const file = pngFile("maximal.png", [tEXt("prompt", MAXIMAL_GRAPHS.independent)]); + const handle = await handleFor(file); + const marks = []; + const yieldControl = async (signal) => { + signal?.throwIfAborted(); + marks.push(performance.now()); + await new Promise((resolve) => setTimeout(resolve, 0)); + }; + const started = performance.now(); + const wire = await m.localFileAdapter.read(handle, { signal: new AbortController().signal, yieldControl }); + marks.push(performance.now()); + // Resolution and the projection each run in a turn of their own after the table is in. + assert.ok(marks.length >= 3, `yields: ${marks.length - 1}`); + const gaps = marks.map((mark, index) => mark - (index ? marks[index - 1] : started)); + assert.ok(Math.max(...gaps) < STEP_BUDGET_MS, `longest step ${Math.round(Math.max(...gaps))} ms`); + const { result } = await m.readLocalFile(file, { classification: handle.classification }); + const direct = await m.fitLocalPayload(m.buildLocalPayload({ read: result, file: { name: handle.filename, size: file.size, lastModified: file.lastModified }, + classification: handle.classification, semantics })); + assert.equal(JSON.stringify(wire), JSON.stringify(direct)); + assert.equal(wire.resolution.state, "ambiguous"); + // An abort at any yield ends the read there. + for (let stop = 0; stop < marks.length - 1; stop++) { + const controller = new AbortController(); + let seen = 0; + const stopping = async (signal) => { + if (seen++ === stop) controller.abort(); + signal.throwIfAborted(); + await new Promise((resolve) => setTimeout(resolve, 0)); + }; + await assert.rejects(m.localFileAdapter.read(handle, { signal: controller.signal, yieldControl: stopping }), { name: "AbortError" }, `yield ${stop}`); + } +}); diff --git a/tests/js/workspace-local-session.test.js b/tests/js/workspace-local-session.test.js index 77498763..36bd64d6 100644 --- a/tests/js/workspace-local-session.test.js +++ b/tests/js/workspace-local-session.test.js @@ -193,8 +193,8 @@ test("unclassified input creates no object URL", async () => { // declares a supported type it opens the inspector's unreadable state, with no preview and no URL (ledger ruling). const gone = pngFile("gone.png", "fox", { gate: () => Promise.reject(new DOMException("The file was removed", "NotReadableError")) }); assert.equal(value.select([gone]), "accepted"); - await settle(); - const unreadable = value.controller.state; + // The read yields between its steps (local-inspection.mjs): wait for the answer, not a fixed number of turns. + const unreadable = await recorded(value.controller); assert.ok(unreadable.record, "the unreadable record is published"); assert.deepEqual([value.state.phase, value.state.message, value.state.preview], ["ready", m.LOCAL_SESSION_MESSAGES.unreadable, null]); assert.equal(m.LOCAL_SESSION_MESSAGES.unreadable, "Embedded metadata could not be read."); diff --git a/web/js/workspace/file-inspector.mjs b/web/js/workspace/file-inspector.mjs index 59b2a3cb..2d7a032d 100644 --- a/web/js/workspace/file-inspector.mjs +++ b/web/js/workspace/file-inspector.mjs @@ -205,7 +205,7 @@ export function mountFileInspector(container, {api, sender, parentScope, variant scope.listen(container, 'keydown', onInteraction); scope.listen(container, 'click', onInteraction); scope.listen(container, 'wheel', onInteraction, {passive:true}); - scope.cleanup(() => {contentScope?.dispose(); container.replaceChildren(); container.classList.remove('file-inspector');}); + scope.cleanup(() => {contentScope?.dispose(); container.replaceChildren(); container.classList.remove('file-inspector'); container.removeAttribute('aria-busy');}); const full = (variant ?? (inline ? 'panel' : 'full')) === 'full'; function statusView(state, parent) { @@ -224,6 +224,8 @@ export function mountFileInspector(container, {api, sender, parentScope, variant if (scope.signal.aborted) return; contentScope?.dispose(); contentScope = childScope(scope); container.replaceChildren(); + // A local read resolves on this thread in bounded steps; the busy state stays until its answer lands. + if (state.loading) container.setAttribute('aria-busy', 'true'); else container.removeAttribute('aria-busy'); render(state); } /** @@ -290,7 +292,7 @@ export function mountFileInspector(container, {api, sender, parentScope, variant * copy does: the shared toast, or the select-to-copy dialog where the clipboard is unavailable or refused. */ function summaryButton(record) { - const button = createButton('Copy settings summary'), text = settingsSummary(record); + const button = createButton('Copy settings summary', 'settings-summary'), text = settingsSummary(record); contentScope.listen(button, 'click', async () => { try { if (!navigator.clipboard) throw new Error('Clipboard requires a secure context'); diff --git a/web/js/workspace/local-inspection.mjs b/web/js/workspace/local-inspection.mjs index 81e7d9c7..e68889b6 100644 --- a/web/js/workspace/local-inspection.mjs +++ b/web/js/workspace/local-inspection.mjs @@ -18,6 +18,24 @@ const MAX_FILENAME_CHARS = 255; const MEDIA_TYPES = new Set(['image', 'video']); const object = value => value !== null && typeof value === 'object' && !Array.isArray(value); +const abortError = () => new DOMException('The local selection ended', 'AbortError'); + +/** `promise`, or an AbortError as soon as `signal` aborts (the work behind `promise` is shared and keeps going). */ +function untilAborted(promise, signal) { + if (!signal) return promise; + if (signal.aborted) return Promise.reject(abortError()); + return new Promise((resolve, reject) => { + const onAbort = () => reject(abortError()); + signal.addEventListener('abort', onAbort, { once: true }); + promise.then(value => { signal.removeEventListener('abort', onAbort); resolve(value); }, + error => { signal.removeEventListener('abort', onAbort); reject(error); }); + }); +} + +/** One turn of the event loop between bounded steps, so the busy state paints and input is handled. */ +function nextTurn(signal) { + return untilAborted(new Promise(resolve => setTimeout(resolve, 0)), signal); +} const pngReader = () => globalThis.PngMetadata || globalThis.window?.PngMetadata; /** A display name with no path separators or control characters; never empty. */ @@ -87,16 +105,21 @@ export function toInspectionRead(classification, result) { const dimension = value => Number.isSafeInteger(value) && value > 0 ? value : null; +/** The roles for one local read (`{prompts, resolution}`): the bounded parse and resolution step of buildLocalPayload. */ +export function localEvidence(read, classification, semantics = null) { + const inspection = toInspectionRead(classificationOf(classification), read); + return inspection.read === null ? unreadableEvidence() : resolveLocalEvidence(inspection.read.records, semantics); +} + /** * The `inspection_version: 1` wire for one local read, before the response budget. Its roles are the server * inspector's for the same records (local-resolver.mjs with `semantics`, the compiled node table; null answers * "unavailable" as the server does), so the legacy fill in projectInspectionPayload applies exactly where it does * for Images. */ -export function buildLocalPayload({ read, file, classification, semantics = null }) { +export function buildLocalPayload({ read, file, classification, semantics = null, evidence = localEvidence(read, classification, semantics) }) { const classified = classificationOf(classification); const inspection = toInspectionRead(classified, read); - const evidence = inspection.read === null ? unreadableEvidence() : resolveLocalEvidence(inspection.read.records, semantics); const filename = sanitizeLocalFilename(file?.name); const size = Number.isSafeInteger(file?.size) && file.size >= 0 ? file.size : null; const modified = typeof file?.lastModified === 'number' && Number.isFinite(file.lastModified) && file.lastModified >= 0 ? file.lastModified / 1000 : null; @@ -171,7 +194,12 @@ export const localFileAdapter = Object.freeze({ scope(handle) { owned(handle); return LOCAL_SCOPE; }, // An inert sentinel: the controller's same-key check works and no URL is ever built. inspectionPath(handle) { return `local-file:${owned(handle).key}`; }, - async read(handle, { signal } = {}) { + /** + * Reads, resolves and projects one selection on the page's thread in bounded steps (each bound by the 2 MiB role + * and 4 MiB workflow budgets and the resolver's work limits) with a turn of the event loop between them, so the + * inspector's busy state paints. Every wait ends with the selection's `signal`; `yieldControl` is for tests. + */ + async read(handle, { signal, yieldControl = nextTurn } = {}) { const secret = owned(handle); let result = null; try { @@ -181,10 +209,14 @@ export const localFileAdapter = Object.freeze({ // MetadataReadError, an unreadable record. Abort and programming errors still reject. if (!(error instanceof DOMException) || error.name === 'AbortError' || signal?.aborted) throw error; } - const semantics = await loadInspectionSemantics(); - signal?.throwIfAborted(); + const semantics = await untilAborted(loadInspectionSemantics(), signal); + await yieldControl(signal); + const evidence = localEvidence(result, secret.classification, semantics); + await yieldControl(signal); const file = { name: secret.filename, size: secret.file.size, lastModified: secret.file.lastModified }; - return fitLocalPayload(buildLocalPayload({ read: result, file, classification: secret.classification, semantics }), MAX_RESPONSE_BYTES, { signal }); + const payload = buildLocalPayload({ read: result, file, classification: secret.classification, semantics, evidence }); + await yieldControl(signal); + return fitLocalPayload(payload, MAX_RESPONSE_BYTES, { signal }); }, toRecord(result, handle, extractFacts) { const secret = owned(handle); diff --git a/web/js/workspace/workspace.css b/web/js/workspace/workspace.css index ae874a07..71173760 100644 --- a/web/js/workspace/workspace.css +++ b/web/js/workspace/workspace.css @@ -213,7 +213,7 @@ main:focus-visible { outline: 2px solid var(--ca-focus); outline-offset: 4px; } where the toolkit's `nowrap` would push them past its edge. */ .ca-tools #viewer-details > button { white-space: normal; /* D20 */ } /* LI-4: "Copy settings summary" after the facts wraps the same way in the narrow rail. */ -.ca-tools #viewer-details > div > button { white-space: normal; /* D117 */ } +.ca-tools #viewer-details .settings-summary { white-space: normal; /* D117 */ } /* The Images page narrows to one column inside a narrow container (the Library-embedded gallery). */ .images-main { container: images-page / inline-size; } @container images-page (inline-size < 600px) {