From a99a311e822e82fe380e7a18aba64847c56552db Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Sat, 24 Oct 2026 21:02:30 -0700 Subject: [PATCH] fix(system): address the S7 final review - I1: an apply answer is a refusal only when our server sent it (a known code, or a 4xx whose body says success:false); a proxy's 5xx, an unreadable 2xx or a dropped connection says Outcome not confirmed and reads Activity again. ApiError gains 'refused'; the server's own pre-start failure is coded 'apply-failed'. - M1: the lost-response test lets the apply run, then drops its answer. - M2: a failed apply records its code in the job's scope; Activity offers Review again for an overtaken plan (Maintenance for a duplicate cleanup whose sources changed). - M3: a failed duplicate cleanup toasts the server's reason. - M4: a review button holds while its preview is read (one preview per double press). - M5: the storage guard also catches bracket and property assignment and CacheStorage/IndexedDB opens; why the Toolkit build is skipped is written down. - M6: a refused storage listing says so on the Metadata cache row. - M7, M8: D100 text corrected; workflow_storage has its title. - M9: a busy start names the job the server named. --- TESTING.md | 39 +++++-- py/api/maintenance.py | 15 ++- .../workspace-system-maintenance.spec.mjs | 103 +++++++++++++++++- tests/js/api-client.test.js | 9 ++ tests/js/cache-ownership.test.js | 55 ++++++++-- tests/js/workspace-activity-kinds.test.js | 9 ++ tests/js/workspace-activity-store.test.js | 13 +++ tests/js/workspace-system-maintenance.test.js | 30 ++++- tests/test_api_maintenance.py | 31 ++++++ web/js/api-client.js | 4 + web/js/workspace/activity-job-view.mjs | 34 +++++- web/js/workspace/activity-kinds.mjs | 4 +- web/js/workspace/activity-store.mjs | 3 +- web/js/workspace/maintenance-review.mjs | 28 +++-- web/js/workspace/system-maintenance.mjs | 11 +- web/js/workspace/system-overview.mjs | 10 +- 16 files changed, 354 insertions(+), 44 deletions(-) diff --git a/TESTING.md b/TESTING.md index 2c4b9849..b14d5375 100644 --- a/TESTING.md +++ b/TESTING.md @@ -2441,7 +2441,9 @@ Recorded deviations (plan D60–D70, D79–D81 renumbered D92–D102 and D111– finished · N groups found", with "Search log" and "Cancel search". The duplicate cleanup (`duplicate_cleanup`, not cancellable) reads "Removing duplicate files", counts Removed (green), Kept and Errors (Kept rows open the viewer), and toasts "Duplicate - cleanup finished · N removed · M errors". Every job keeps Download report. Tests: the + cleanup finished · N removed · M errors" or, when it failed, "Duplicate cleanup failed · + ". A refused start names the job the server named, not the + first running one. Every job keeps Download report. Tests: the three S7 cases in `tests/js/workspace-activity-kinds.test.js`. - **D112** (Activity, S5): a legacy-page operation held on the registry (not cancellable, no outcome groups) reads as its server title ("Database restore (admin @@ -2451,11 +2453,15 @@ Recorded deviations (plan D60–D70, D79–D81 renumbered D92–D102 and D111– - **D100** (Maintenance, S7): the concept's `maintenancePlan` dialog ("Review maintenance plan") on the server's read-only plan (`POST /maintenance/preview`), opened by each Review plan and by the health report's Review missing links. No sample note. The `h3` - is the operation's concept name; the `dl.meta-grid` keeps Scope / Protected / Backup / + is the operation's row title (the concept prints its `data-plan` name, "duplicate + prompts"); the `dl.meta-grid` keeps Scope / Protected / Backup / Media on disk with the plan's values: its scope label, its protected reasons joined with " · " ("Nothing is protected" when none), "Create and verify before changes - (.backup_)", "No safety backup (the policy is off)" or "Always - created and verified first (…)" for Store each workflow once, and its media line. A + (.backup_)" or "No safety backup (the policy is off)", and its + media line. (A `workflow_storage` plan would read "Store each workflow once" and + "Always created and verified first (…)", but Maintenance has no row for it: the + concept has none, so the migration is reviewed and applied through the API and only + its jobs appear, in Activity.) A `details` "Listed records (N)" ("(N of M)" past the server's 50) holds the records the plan names in a `pre`. The `p.small` says "Nothing changes until you apply. This review holds for N minutes." (from `expires_at`), plus "Not enough free disk space: @@ -2466,8 +2472,19 @@ Recorded deviations (plan D60–D70, D79–D81 renumbered D92–D102 and D111– Activity. A refused apply stays and says why: "Another job is running: ." (D57), "This plan was already applied: <job>." (`plan-used`, linking the job), the server's line for an expired or replaced review (then the plan is reviewed again at - once), for a short disk and for nothing to do, and "Outcome not confirmed. Check - Activity before applying it again." when the answer was lost (the job may run). + once), for a short disk, for nothing to do and for the server's own pre-start failure + (`apply-failed`). Only our server's refusal is definitive: a known code, or a 4xx whose + body says `success:false`. A dropped connection, a proxy's 5xx (no code) or a 2xx whose + body cannot be read may follow a started job, so they say "Outcome not confirmed. Check + Activity before applying it again." and read the Activity list again + (`lost apply response…` runs the apply and then drops its answer; `a proxy error or an + unreadable answer is not a refusal` answers a 504 after the server started the job). A + review button holds ("Reviewing…", `aria-disabled`) while its preview is read, so a + double press sends one preview (`a review button pressed twice sends one preview`). A + failed apply records its code as the job's `scope.failure`; when the plan was overtaken + (`plan-changed`, `storage-changed`, `plan-expired`), its Activity view offers "Review + again", which reviews the same operation afresh (a duplicate cleanup whose sources + changed goes back to Maintenance to search again). Cancel and Escape write nothing (`Cancel writes nothing`: a read-only dump of the rows and the media hashes, `GET /__test__/system/digest`). `plan dialogs match the concept` (system-maintenance-concept.spec, both themes and sizes) compares each of the four @@ -2493,7 +2510,8 @@ Recorded deviations (plan D60–D70, D79–D81 renumbered D92–D102 and D111– Prompt Manager owns as a cache (`web/js/cache-ownership.js`). The inventory of every storage writer has none (see "Browser cache ownership" below), so the row's line adds "Nothing is cached in this browser." and its Review plan is `aria-disabled` and opens - nothing. With owned entries (a test names one through the module's inventory), Review + nothing; a browser that refused to list its storage says "This browser did not allow + checking its storage." instead. With owned entries (a test names one through the module's inventory), Review plan opens "Review maintenance plan" › "Metadata cache" (Scope "N cached entries in this browser", Protected "Preferences and other tools’ data in this browser", Backup "Not needed: the database is not changed", Media "No files are removed") and "Clear @@ -2926,7 +2944,12 @@ writes is a preference (`gallerySettings`, `pm_workspace_theme`, `web/sw.js` service worker that did never shipped). So nothing is removed, and the toast says "Nothing of Prompt Manager's is cached in this browser." `the inventory covers every browser storage writer in web/` (`tests/js/cache-ownership.test.js`) fails when a -page writes a key that is neither a preference nor inventoried, or opens a cache. The +page writes a key that is neither a preference nor inventoried, writes one it cannot name +(a computed `setItem` key, a bracket or property assignment on a Storage), or opens +CacheStorage or IndexedDB; `the storage scan reports every way of writing browser +storage` pins those patterns. It skips `web/lib` (third-party builds) and the pinned +Toolkit build (`web/js/shared/toolkit`, never hand-edited, verified by `npm run check`; +its storage belongs to the Toolkit repository and a new pin is reviewed on its own). The gallery's server thumbnail clear stays its own request and runs first: refused or unreachable, it changes nothing and says so (the old code toasted success when the request failed); refused browser storage is reported, never hidden diff --git a/py/api/maintenance.py b/py/api/maintenance.py index dddc1144..989f489f 100644 --- a/py/api/maintenance.py +++ b/py/api/maintenance.py @@ -37,7 +37,8 @@ runner runs); a refused start keeps it. The runner: The job log names the backup by its file name only, and the job says "No safety backup" when none was taken. Apply errors: ``invalid-request`` (400), -``plan-expired`` (410: unknown, expired or forgotten after a restart), +``apply-failed`` (500: nothing started), ``plan-expired`` (410: unknown, +expired or forgotten after a restart), ``plan-used`` (409, with the job that applied it), ``storage-changed`` (409: a restore replaced the database since the review) and ``busy`` (409). """ @@ -644,7 +645,9 @@ class MaintenanceRoutesMixin: ) except (OSError, sqlite3.Error): self.logger.exception("Could not measure the disk space for an apply") - return _failure(APPLY_FAILED, 500) + # Coded: the workspace tells the server's own refusal (nothing + # started) from a proxy's 5xx (the job may have started). + return _failure(APPLY_FAILED, 500, "apply-failed") if short is not None: # Refused before any job starts: nothing fails halfway for space. return _failure(short_space_message(short), 507, "insufficient-space") @@ -808,7 +811,13 @@ def backup_note(backup): def _fail_job(job, code, message): - """End ``job`` failed with ``message`` (one error row) and its code.""" + """End ``job`` failed with ``message`` (one error row) and its code. + + The code is also the job's ``scope["failure"]``, so Activity can offer + Review again for a plan the library overtook. + """ + if code: + job.scope["failure"] = code job.record("errors", {"path": "", "detail": message}, f"! {message}") job.update(status_line=message) job.publish({"type": "error", "code": code, "message": message}) diff --git a/tests/browser/workspace-system-maintenance.spec.mjs b/tests/browser/workspace-system-maintenance.spec.mjs index 768181bb..cb22357b 100644 --- a/tests/browser/workspace-system-maintenance.spec.mjs +++ b/tests/browser/workspace-system-maintenance.spec.mjs @@ -21,6 +21,7 @@ test.beforeEach(async ({ page, request, browserName }) => { test.afterEach(async ({ page, request, browserName }) => { await saveCoverage(page, browserName); + await request.post('/__test__/release'); await request.post('/__test__/jobs/restart'); await request.post('/__test__/system/reset'); expect(page.systemErrors).toEqual([]); @@ -212,6 +213,49 @@ test('plan-changed asks for a new review', async ({ page, request }) => { await expect(toast(page)).toHaveText(`Duplicate prompt text failed · ${CHANGED}`); await expect(page.locator('#workspace-main')).toContainText(CHANGED); expect((await serverJson(request, '/__test__/system/digest')).rows).toEqual(before.rows); + // S7 review M2: the failed job offers Review again, which reviews the same operation afresh. + const previews = []; + page.on('request', item => { if (item.url().endsWith(PREVIEW_URL)) previews.push(item.postDataJSON()); }); + await page.locator('#workspace-main').getByRole('button', { name: 'Review again', exact: true }).click(); + await expect(dialog(page).locator('#modal-title')).toHaveText('Review maintenance plan'); + await expect(dialog(page).locator('#modal-body > h3')).toHaveText('Duplicate prompt text'); + expect(previews).toEqual([{ operation: 'duplicate_prompts', options: {} }]); +}); + +test('a review button pressed twice sends one preview (S7 review M4)', async ({ page, request }) => { + await expect(await request.post('/__test__/system/missing-file')).toBeOK(); + await expect(await request.post('/__test__/system/duplicate-files')).toBeOK(); + const previews = []; + page.on('request', item => { if (item.url().endsWith(PREVIEW_URL)) previews.push(item.postDataJSON().operation); }); + await openMaintenance(page); + await row(page, 'Integrity & relationship checks').getByRole('button', { name: 'Check', exact: true }).click(); + // Hold the preview at the server so the second press lands while it is read. + await expect(await request.post('/__test__/hold', { data: { path: PREVIEW_URL } })).toBeOK(); + // The pressed button reads "Reviewing…" while held, so it is found by its place, not its name. + const missing = dialog(page).locator('.dialog-actions > button').last(); + await expect(missing).toHaveText('Review missing links'); + await missing.click(); + await expect(missing).toHaveAttribute('aria-disabled', 'true'); + await expect(missing).toHaveText('Reviewing…'); + await missing.dispatchEvent('click'); + await expect.poll(async () => (await serverJson(request, '/__test__/hold')).held[PREVIEW_URL]).toBe(1); + await expect(await request.post('/__test__/release')).toBeOK(); + await expect(dialog(page).locator('#modal-body > h3')).toHaveText('Missing-file links'); + expect(previews).toEqual(['missing_links']); + await page.keyboard.press('Escape'); + + await row(page, 'Duplicate image files').getByRole('button', { name: 'Find duplicates', exact: true }).click(); + await page.locator('#workspace-main').getByRole('button', { name: 'Review groups', exact: true }).click(); + await expect(await request.post('/__test__/hold', { data: { path: PREVIEW_URL } })).toBeOK(); + const cleanup = dialog(page).locator('.dialog-actions > button').last(); + await expect(cleanup).toHaveText('Review duplicate cleanup'); + await cleanup.click(); + await expect(cleanup).toHaveAttribute('aria-disabled', 'true'); + await cleanup.dispatchEvent('click'); + await expect.poll(async () => (await serverJson(request, '/__test__/hold')).held[PREVIEW_URL]).toBe(1); + await expect(await request.post('/__test__/release')).toBeOK(); + await expect(dialog(page).locator('#modal-body > h3')).toHaveText('Duplicate image files'); + expect(previews).toEqual(['missing_links', 'duplicate_files']); }); test('plan-used links the job that applied it', async ({ page, request }) => { @@ -232,15 +276,68 @@ test('plan-used links the job that applied it', async ({ page, request }) => { await expect(page).toHaveURL(`${workspace}#/system/maintenance`); }); +/** + * Record every toast the page shows (`[text, [link hrefs]]`): a job's finish toast may replace an earlier one before a + * test reads it. + */ +async function recordToasts(page) { + await page.addInitScript(() => { + window.toastLog = []; + const record = node => window.toastLog.push([node.textContent, [...node.querySelectorAll('a')].map(a => a.getAttribute('href'))]); + new MutationObserver(() => { + const node = document.getElementById('workspace-toast'); + const last = window.toastLog.at(-1); + if (node && node.textContent && last?.[0] !== node.textContent) record(node); + }).observe(document, { subtree: true, childList: true, characterData: true }); + }); + return () => page.evaluate(() => window.toastLog); +} + +const NOT_CONFIRMED = ['Outcome not confirmed. Check Activity before applying it again.', ['#/activity']]; + +/** The maintenance jobs the server lists (the apply ran even though its answer was lost). */ +async function maintenanceJobs(request) { + return (await serverJson(request, '/prompt_manager/jobs')).jobs.filter(job => job.kind === 'maintenance'); +} + test('lost apply response says Outcome not confirmed and points to Activity', async ({ page, request }) => { await expect(await request.post('/__test__/system/duplicate-prompts')).toBeOK(); - await page.route(`**${APPLY_URL}`, route => route.abort('failed')); + // The server applies the plan; its answer never reaches the page. + const reads = []; + await page.route(`**${APPLY_URL}`, async route => { + await route.fetch(); + page.on('request', item => { if (item.url().endsWith('/prompt_manager/jobs')) reads.push(item.url()); }); + await route.abort('failed'); + }); + const toasts = await recordToasts(page); await openMaintenance(page); await row(page, 'Duplicate prompt text').getByRole('button', { name: 'Review plan', exact: true }).click(); await dialog(page).getByRole('button', { name: 'Apply reviewed operation', exact: true }).click(); - await expect(toast(page)).toHaveText('Outcome not confirmed. Check Activity before applying it again.'); - await expect(toast(page).getByRole('link', { name: 'Check Activity', exact: true })).toHaveAttribute('href', '#/activity'); + await expect.poll(toasts).toContainEqual(NOT_CONFIRMED); await expect(page).toHaveURL(`${workspace}#/system/maintenance`); + // The Activity list is read again at once, and the job the lost answer started is in it. + await expect.poll(() => reads.length).toBeGreaterThan(0); + expect((await maintenanceJobs(request)).map(job => job.title)).toEqual(['Duplicate prompt text']); + await page.evaluate(() => { location.hash = '#/activity'; }); + await expect(page.locator('#workspace-main')).toContainText('Duplicate prompt text'); +}); + +test('a proxy error or an unreadable answer is not a refusal: Outcome not confirmed (S7 review I1)', async ({ page, request }) => { + await expect(await request.post('/__test__/system/duplicate-prompts')).toBeOK(); + const reads = []; + // A reverse proxy times out after the server started the job. + await page.route(`**${APPLY_URL}`, async route => { + await route.fetch(); + page.on('request', item => { if (item.url().endsWith('/prompt_manager/jobs')) reads.push(item.url()); }); + await route.fulfill({ status: 504, contentType: 'text/html', body: '<html><body>Gateway Timeout</body></html>' }); + }); + const toasts = await recordToasts(page); + await openMaintenance(page); + await row(page, 'Duplicate prompt text').getByRole('button', { name: 'Review plan', exact: true }).click(); + await dialog(page).getByRole('button', { name: 'Apply reviewed operation', exact: true }).click(); + await expect.poll(toasts).toContainEqual(NOT_CONFIRMED); + await expect.poll(() => reads.length).toBeGreaterThan(0); + expect(await maintenanceJobs(request)).toHaveLength(1); }); test('a refused apply names the running job', async ({ page, request }) => { diff --git a/tests/js/api-client.test.js b/tests/js/api-client.test.js index fb69933c..1204be53 100644 --- a/tests/js/api-client.test.js +++ b/tests/js/api-client.test.js @@ -387,3 +387,12 @@ test("ApiError keeps a validated running job on busy and a validated source on i } assert.deepEqual([new ApiError(409, "old caller").running, new ApiError(409, "old caller").source], [null, null]); }); + +test("ApiError says whether the server itself refused (a parsed success:false body; S7 review I1)", () => { + assert.equal(new ApiError(400, "bad", null, null, { success: false, error: "bad" }).refused, true); + assert.equal(new ApiError(409, "busy", "busy", null, { success: false, code: "busy" }).refused, true); + // A proxy's page, a body that was not JSON, or no body at all: nothing says the server refused. + for (const body of [null, undefined, "<html>", { success: true }, [], { error: "x" }]) { + assert.equal(new ApiError(504, "Gateway Timeout", null, null, body).refused, false); + } +}); diff --git a/tests/js/cache-ownership.test.js b/tests/js/cache-ownership.test.js index b70fac5f..2ea9e582 100644 --- a/tests/js/cache-ownership.test.js +++ b/tests/js/cache-ownership.test.js @@ -68,9 +68,50 @@ test('removes only inventoried owned keys', async () => { assert.equal(CacheOwnership.clearMessage(result), CacheOwnership.NOTHING_CACHED); }); +/** + * Every browser-storage write in one file's text: `{keys}` (setItem keys, as written) and `problems` (writes no key + * can be read from: a bracket or property assignment on a Storage, a computed setItem key, an IndexedDB or + * CacheStorage open or write). The scan is textual, so it errs on the side of reporting. + */ +function storageWrites(text) { + const keys = []; + const problems = []; + for (const match of text.matchAll(/\.setItem\(\s*([^,)]+)/g)) { + const key = match[1].trim(); + const literal = /^(?:'([^']+)'|"([^"]+)")$/.exec(key); + if (literal) keys.push(literal[1] ?? literal[2]); + else if (/^[A-Z_]+_KEY$/.test(key)) keys.push(`$${key}`); + else problems.push(`a computed setItem key: ${key}`); + } + if (/\b\w*[sS]torage\s*\[[^\]]+\]\s*=(?!=)/.test(text)) problems.push('a bracket assignment on a Storage'); + if (/\b(?:local|session)Storage\s*\.\s*(?!getItem|setItem|removeItem|key\b|length\b|clear\b)\w+\s*=(?!=)/.test(text)) { + problems.push('a property assignment on a Storage'); + } + if (/\bcaches\s*\.\s*open\s*\(/.test(text)) problems.push('opens a CacheStorage cache'); + if (/\bindexedDB\s*\.\s*open\s*\(/.test(text)) problems.push('opens IndexedDB'); + return { keys, problems }; +} + +test('the storage scan reports every way of writing browser storage (S7 review M5)', () => { + assert.deepEqual(storageWrites("localStorage.setItem('gallerySettings', x); storage.setItem(NOTICE_KEY, v);").keys, + ['gallerySettings', '$NOTICE_KEY']); + for (const text of ["localStorage['pm_cache'] = 1;", 'sessionStorage[key] = value;', 'storage[name] = x;', + 'localStorage.pm_cache = 1;', 'sessionStorage.someKey = "v";', 'localStorage.setItem(key, 1);', + "caches.open('pm').then(cache => cache.put(url, response));", "indexedDB.open('pm-cache', 1);"]) { + assert.equal(storageWrites(text).problems.length, 1, text); + } + for (const text of ["if (localStorage['pm'] === x) {}", 'const value = localStorage.getItem(KEY);', + 'localStorage.removeItem(KEY);', 'const n = localStorage.length;']) { + assert.deepEqual(storageWrites(text), { keys: [], problems: [] }, text); + } +}); + test('the inventory covers every browser storage writer in web/', () => { // Every key a Prompt Manager page writes is a preference or an inventoried cache entry; a new writer must be - // classified here before a cache clear may remove it. + // classified here before a cache clear may remove it. Skipped: web/lib (third-party Viewer.js and Tailwind + // output) and web/js/shared/toolkit (the pinned ComfyAssets Toolkit build, never hand-edited here and verified by + // `npm run --prefix tools/vendor check`). Neither writes browser storage today; the Toolkit's own repository owns + // its storage, and a new pin is reviewed as its own change. const files = []; const walk = dir => { for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { @@ -85,15 +126,9 @@ test('the inventory covers every browser storage writer in web/', () => { for (const file of files) { const text = fs.readFileSync(file, 'utf8'); for (const match of text.matchAll(/(?:const|export const)\s+([A-Z_]+_KEY)\s*=\s*'([^']+)'/g)) constants[match[1]] = match[2]; - assert.equal(/caches\.open\(/.test(text), false, `${path.relative(WEB, file)} opens a CacheStorage cache`); - assert.equal(/indexedDB\.open\(/.test(text), false, `${path.relative(WEB, file)} opens IndexedDB`); - for (const match of text.matchAll(/\.setItem\(\s*([^,)]+)/g)) { - const literal = /^(?:'([^']+)'|"([^"]+)")$/.exec(match[1].trim()); - const constant = /^[A-Z_]+_KEY$/.test(match[1].trim()) ? `$${match[1].trim()}` : null; - // A key computed at run time could be anything: it must be a literal or a named key constant. - assert.ok(literal || constant, `${path.relative(WEB, file)} writes a computed key: ${match[1]}`); - written.add(literal ? literal[1] ?? literal[2] : constant); - } + const { keys, problems } = storageWrites(text); + assert.deepEqual(problems, [], path.relative(WEB, file)); + for (const key of keys) written.add(key); } const keys = [...written].map(key => (key.startsWith('$') ? constants[key.slice(1)] : key)); assert.ok(keys.length >= 3, 'the writers were found'); diff --git a/tests/js/workspace-activity-kinds.test.js b/tests/js/workspace-activity-kinds.test.js index 107f147b..dd712620 100644 --- a/tests/js/workspace-activity-kinds.test.js +++ b/tests/js/workspace-activity-kinds.test.js @@ -216,3 +216,12 @@ test('a duplicate cleanup counts removed, kept and errors and cannot be cancelle assert.equal(finishToast({ ...cleanup, state: 'finished', counters: { removed: 3, kept: 1, errors: 1 } }), 'Duplicate cleanup finished · 3 removed · 1 error'); }); + +test('a failed duplicate cleanup says the server reason (S7 review M3)', async () => { + const { finishToast } = await store; + const reason = 'The sources changed since the search. Search for duplicates again.'; + const failed = job({ kind: 'duplicate_cleanup', title: 'Remove duplicate image files', state: 'failed', status_line: reason, + counters: { removed: 0, kept: 0, errors: 1 }, cancellable: false }); + assert.equal(finishToast(failed), `Duplicate cleanup failed · ${reason}`); + assert.equal(finishToast({ ...failed, status_line: '' }), 'Duplicate cleanup failed · 0 removed · 1 error'); +}); diff --git a/tests/js/workspace-activity-store.test.js b/tests/js/workspace-activity-store.test.js index e6819714..33a69d65 100644 --- a/tests/js/workspace-activity-store.test.js +++ b/tests/js/workspace-activity-store.test.js @@ -437,3 +437,16 @@ test('numbers group the way the server writes them, whatever the browser locale' Number.prototype.toLocaleString = original; } }); + +test('a busy start names the job the server named, not the first running one (S7 review M9)', async () => { + const { createActivityStore } = await source; + const { world, ...deps } = harness(); + const store = createActivityStore(deps); + const holder = 'b'.repeat(32); + // Two read jobs run side by side; the server says the second one holds the slot. + world.list = [summary(), summary({ id: holder, kind: 'duplicates', title: 'Find duplicate image files' })]; + world.postError = Object.assign(apiError(409, 'busy', 'busy'), + { running: { id: holder, title: 'Find duplicate image files' } }); + const busy = await store.start('duplicates'); + assert.deepEqual([busy.busy.id, busy.busy.title], [holder, 'Find duplicate image files']); +}); diff --git a/tests/js/workspace-system-maintenance.test.js b/tests/js/workspace-system-maintenance.test.js index 1c3d5cdd..95c282c3 100644 --- a/tests/js/workspace-system-maintenance.test.js +++ b/tests/js/workspace-system-maintenance.test.js @@ -61,6 +61,8 @@ test("plan dialog shows the server's scope, protected, backup policy and media l assert.deepEqual(detailLines(PLAN), ['a cat · 2 copies merged into the oldest', 'a dog']); assert.deepEqual(detailLines({ details: [null, { label: 7 }] }), []); assert.equal(OPERATION_TITLES.missing_links, 'Missing-file links'); + // S7 review M8: the workflow migration's plan reads its own title (its job is started outside Maintenance). + assert.equal(OPERATION_TITLES.workflow_storage, 'Store each workflow once'); }); test('a refused apply names its reason; a lost answer is not confirmed', async () => { @@ -79,7 +81,15 @@ test('a refused apply names its reason; a lost answer is not confirmed', async ( { toast: 'This review expired. Review the plan again.', review: true }); assert.deepEqual(applyRefusal(error(409, 'storage-changed', 'Replaced.')), { toast: 'Replaced.', review: true }); assert.deepEqual(applyRefusal(error(507, 'insufficient-space', 'Not enough free disk space.')), { toast: 'Not enough free disk space.' }); - assert.deepEqual(applyRefusal(error(500, null, 'Traceback …')), { toast: APPLY_FAILED }); + // S7 review I1: only our server's own refusal is definitive. A proxy's 5xx, a 2xx whose body could not be read, + // or any answer without the server's refusal may have followed a started job: not confirmed, read Activity again. + assert.deepEqual(applyRefusal(error(504, null, 'Gateway Timeout')), { toast: NOT_CONFIRMED, lost: true }); + assert.deepEqual(applyRefusal(error(500, null, 'Internal Server Error')), { toast: NOT_CONFIRMED, lost: true }); + assert.deepEqual(applyRefusal(error(202, null, 'Invalid JSON response')), { toast: NOT_CONFIRMED, lost: true }); + assert.deepEqual(applyRefusal(error(502, null, 'Bad Gateway', { refused: false })), { toast: NOT_CONFIRMED, lost: true }); + assert.deepEqual(applyRefusal(error(400, null, 'token must be the plan token', { refused: true })), { toast: APPLY_FAILED }); + assert.deepEqual(applyRefusal(error(500, 'apply-failed', 'Maintenance failed; nothing was changed.', { refused: true })), + { toast: 'Maintenance failed; nothing was changed.' }); assert.equal(reviewRefusal(error(413, 'too-large', 'This plan is too large to review at once.')), 'This plan is too large to review at once.'); assert.equal(reviewRefusal(error(500, null, 'boom')), REVIEW_FAILED); @@ -117,6 +127,9 @@ test('Metadata cache follows Q5: disabled with its reason while nothing is owned assert.equal(METADATA_CACHE_DESCRIPTION, 'Clear only Prompt Manager metadata cached by this browser.'); assert.equal(metadataCacheDescription({ keys: [], caches: [] }), `${METADATA_CACHE_DESCRIPTION} ${NOTHING_CACHED_REASON}`); assert.equal(metadataCacheDescription({ keys: ['k'], caches: [] }), METADATA_CACHE_DESCRIPTION); + // S7 review M6: a browser that refused the listing is not claimed empty. + assert.equal(metadataCacheDescription({ keys: [], caches: [], denied: true }), + `${METADATA_CACHE_DESCRIPTION} This browser did not allow checking its storage.`); }); test('only a finished duplicate search with groups offers Review groups', async () => { @@ -128,3 +141,18 @@ test('only a finished duplicate search with groups offers Review groups', async assert.equal(hasDuplicateGroups(job('finished', 2, 'scan')), false); assert.equal(hasDuplicateGroups({ kind: 'duplicates', state: 'finished', counters: {} }), false); }); + +test('a failed apply the library overtook offers Review again, back to its review (S7 review M2)', async () => { + const { reviewAgainOf } = await jobView; + const failed = (kind, failure, operation) => ({ kind, state: 'failed', scope: { operation, failure } }); + assert.deepEqual(reviewAgainOf(failed('maintenance', 'plan-changed', 'duplicate_prompts')), + { operation: 'duplicate_prompts' }); + assert.deepEqual(reviewAgainOf(failed('maintenance', 'storage-changed', 'optimize')), { operation: 'optimize' }); + // A duplicate cleanup reviewed against a search the sources overtook: search again in Maintenance. + assert.deepEqual(reviewAgainOf(failed('duplicate_cleanup', 'sources-changed', 'duplicate_files')), + { section: '#/system/maintenance' }); + assert.equal(reviewAgainOf(failed('maintenance', 'backup-failed', 'optimize')), null); + assert.equal(reviewAgainOf(failed('maintenance', 'plan-changed', 'not_an_operation')), null); + assert.equal(reviewAgainOf({ ...failed('maintenance', 'plan-changed', 'optimize'), state: 'finished' }), null); + assert.equal(reviewAgainOf(failed('scan', 'plan-changed', 'optimize')), null); +}); diff --git a/tests/test_api_maintenance.py b/tests/test_api_maintenance.py index 2066d2a7..67dfccb6 100644 --- a/tests/test_api_maintenance.py +++ b/tests/test_api_maintenance.py @@ -949,6 +949,37 @@ class TestFinalReviewFixes(ApplyTestCase): self.assertEqual(reply["code"], "plan-used") self.assertEqual(self.api._jobs.list(), []) + async def test_an_apply_that_never_started_says_so_with_its_code(self): + """S7 review I1: the server's own pre-start failure is definitive, so + it carries a code the workspace can tell from a proxy's 5xx.""" + from py.api import maintenance + + self.save("Orphan") + plan = await self.preview("unused_components") + with patch.object(maintenance, "disk_plan", side_effect=OSError("disk")): + reply = await self.apply(plan["token"], 500) + self.assertEqual(reply["code"], "apply-failed") + self.assertEqual(reply["error"], maintenance.APPLY_FAILED) + self.assertEqual(self.api._jobs.list(), []) + + async def test_a_failed_apply_names_its_failure_in_its_scope(self): + """S7 review M2: Activity offers Review again from the job's own record.""" + orphan, soon = self.save("Orphan"), self.save("Soon a recipe") + plan = await self.preview("unused_components") + with self.api.db.model.get_connection() as conn: + conn.execute( + "INSERT INTO prompt_library_state(prompt_id,title) VALUES(?,?)", + (soon, "Recipe"), + ) + body = await self.apply(plan["token"]) + job = self.api._jobs.get(body["job"]["id"]) + await asyncio.wait_for(asyncio.shield(job.task), WAIT) + self.assertEqual( + job.summary()["scope"], + {"operation": "unused_components", "failure": "plan-changed"}, + ) + self.assertIsNotNone(self.api.db.get_prompt_by_id(orphan)) + async def test_the_backup_decision_is_frozen_when_the_job_starts(self): PromptManagerConfig.SAFETY_BACKUP = False self.save("Orphan") diff --git a/web/js/api-client.js b/web/js/api-client.js index 92d3f4c4..7860b4fb 100644 --- a/web/js/api-client.js +++ b/web/js/api-client.js @@ -50,6 +50,10 @@ running.title.length > 0 && running.title.length <= 200) { this.running = { id: running.id, title: running.title }; } + // Whether our server itself refused: its body parsed and says success:false. A proxy's error page or an + // unreadable body is not a refusal (the request may have done its work; System S7 review I1). + this.refused = Boolean(current && typeof current === "object" && !Array.isArray(current) && + current.success === false); // A used plan names the job that applied it (System S7); its title is null once that job was pruned. this.job = null; const job = current && current.job; diff --git a/web/js/workspace/activity-job-view.mjs b/web/js/workspace/activity-job-view.mjs index c500b84d..80783c0b 100644 --- a/web/js/workspace/activity-job-view.mjs +++ b/web/js/workspace/activity-job-view.mjs @@ -37,7 +37,8 @@ import { createMediaViewer } from './media-viewer.mjs'; import { createInspectionController } from './inspection-state.mjs'; import { createPromptSender } from './prompt-sender.mjs'; import { imageKey } from './images-state.mjs'; -import { openDuplicatesReview } from './maintenance-review.mjs'; +import { OPERATION_TITLES, openDuplicatesReview, reviewPlan } from './maintenance-review.mjs'; +import { navigateTo } from './state.mjs'; export const DETAIL_INTERVAL_MS = 1000; export const NOT_FOUND = 'This job is not in this session’s Activity. Jobs are kept until ComfyUI restarts.'; @@ -76,6 +77,26 @@ export function parseDetail(raw) { return { job, outcomes, log }; } +// Failures that mean the reviewed plan no longer matches: the review must be done again (S7 review M2). +const OVERTAKEN = new Set(['plan-changed', 'storage-changed', 'plan-expired']); + +/** + * Where a failed apply's Review again goes (S7 review M2): a maintenance plan the library overtook is reviewed again + * (`{operation}`); a duplicate cleanup whose sources or plan changed needs a new search, in Maintenance (`{section}`); + * otherwise null. + */ +export function reviewAgainOf(job) { + if (job.state !== 'failed') return null; + const failure = job.scope?.failure; + if (job.kind === 'maintenance' && OVERTAKEN.has(failure) && Object.hasOwn(OPERATION_TITLES, job.scope?.operation)) { + return { operation: job.scope.operation }; + } + if (job.kind === 'duplicate_cleanup' && (OVERTAKEN.has(failure) || failure === 'sources-changed')) { + return { section: '#/system/maintenance' }; + } + return null; +} + /** D101: a duplicate search that finished with groups to review. */ export function hasDuplicateGroups(job) { return job.kind === 'duplicates' && job.state === 'finished' && (job.counters.groups_found ?? 0) > 0; @@ -135,6 +156,8 @@ export function mountActivityJobView(main, { store, scope, api, jobId }) { const review = createReviewMetadataButton(scope); // D87 // System S7 (D101): a finished duplicate search that found groups reviews them. const reviewGroups = createButton('Review groups', 'ca-primary'); + // S7 review M2: a failed apply whose plan the library overtook goes back to its review. + const reviewAgain = createButton('Review again', 'ca-primary'); const progressPart = section('Progress', 'job-progress-title'); const status = element('span', '', 'small'); @@ -237,6 +260,7 @@ export function mountActivityJobView(main, { store, scope, api, jobId }) { const focused = main.contains(document.activeElement) && document.activeElement !== parts.h1; download.remove(); reviewGroups.remove(); + reviewAgain.remove(); cancel.remove(); progressPart.node.remove(); resultsPart.node.remove(); @@ -302,6 +326,8 @@ export function mountActivityJobView(main, { store, scope, api, jobId }) { else review.remove(); if (hasDuplicateGroups(job)) { if (!reviewGroups.isConnected) download.after(reviewGroups); } // D101 else reviewGroups.remove(); + if (reviewAgainOf(job)) { if (!reviewAgain.isConnected) download.after(reviewAgain); } + else reviewAgain.remove(); // Truth rule: a job that cannot be cancelled (a held legacy-page job) shows no Cancel. if (canCancel(job)) { if (!cancel.isConnected) parts.actions.append(cancel); @@ -404,6 +430,12 @@ export function mountActivityJobView(main, { store, scope, api, jobId }) { } }); scope.listen(reviewGroups, 'click', () => openDuplicatesReview({ api, store, scope, jobId, trigger: reviewGroups })); + scope.listen(reviewAgain, 'click', () => { + const job = current(); + const target = job ? reviewAgainOf(job) : null; + if (target?.operation) reviewPlan({ api, store, scope, operation: target.operation, trigger: reviewAgain }); + else if (target?.section) navigateTo(target.section); + }); scope.listen(cancel, 'click', async () => { if (cancel.getAttribute('aria-disabled') === 'true') return; cancel.setAttribute('aria-disabled', 'true'); diff --git a/web/js/workspace/activity-kinds.mjs b/web/js/workspace/activity-kinds.mjs index 12c3dd51..233a6a16 100644 --- a/web/js/workspace/activity-kinds.mjs +++ b/web/js/workspace/activity-kinds.mjs @@ -172,7 +172,9 @@ export const KINDS = Object.freeze({ duplicate_cleanup: Object.freeze({ ...GENERIC, pillVerb: 'Removing duplicate files', - finishToast: namedFinish(() => 'Duplicate cleanup', removedOf, cleanupLine), + // A failed cleanup says the server's reason (sources changed, no safety backup), as a maintenance apply does. + finishToast: job => (job.state === 'failed' && job.status_line ? `Duplicate cleanup failed · ${job.status_line}` + : namedFinish(() => 'Duplicate cleanup', removedOf, cleanupLine)(job)), line: cleanupLine, kept: removedOf, tiles: tiles([['removed', 'Removed', 'added'], ['kept', 'Kept', ''], ['errors', 'Errors', 'bad']]), diff --git a/web/js/workspace/activity-store.mjs b/web/js/workspace/activity-store.mjs index ca535c43..fa80c491 100644 --- a/web/js/workspace/activity-store.mjs +++ b/web/js/workspace/activity-store.mjs @@ -246,7 +246,8 @@ export function createActivityStore({ api, scope, connect = defaultConnect, noti } catch (error) { if (error?.status !== 409 || error?.code !== 'busy') throw error; await load(); - return { busy: store.running()[0] ?? null, message: error.message }; + // The job the server named holds the slot; read jobs run side by side, so the first running one may not. + return { busy: error.running ?? store.running()[0] ?? null, message: error.message }; } }, /** diff --git a/web/js/workspace/maintenance-review.mjs b/web/js/workspace/maintenance-review.mjs index d6a7ca9e..0373e204 100644 --- a/web/js/workspace/maintenance-review.mjs +++ b/web/js/workspace/maintenance-review.mjs @@ -49,12 +49,14 @@ export const OPERATION_TITLES = Object.freeze({ unused_components: 'Unused prompt components', missing_links: 'Missing-file links', duplicate_files: 'Duplicate image files', + // Reviewed and applied through the API and seen in Activity; Maintenance has no row for it (the concept has none). + workflow_storage: 'Store each workflow once', }); // The review is gone or no longer matches the database: review it again. const REVIEW_AGAIN = new Set(['plan-expired', 'storage-changed']); // Refusals whose server message is the whole truth. const SERVER_SAYS = new Set(['insufficient-space', 'nothing-to-do', 'too-large', 'invalid-request', 'not-found', - 'job-running', 'no-groups']); + 'job-running', 'no-groups', 'apply-failed']); const number = value => value.toLocaleString('en-US'); @@ -117,9 +119,17 @@ export function detailLines(plan) { .map(row => (row.detail ? `${row.label} · ${row.detail}` : row.label)); } -/** The toast for a refused or failed apply; `review` when the plan must be reviewed again. */ +/** + * The toast for a refused or failed apply; `review` when the plan must be reviewed again, `lost` when the outcome is + * not known. Only our server's own refusal is definitive (a known code, or a 4xx whose body says success:false): a + * dropped connection, a proxy's 5xx or a 2xx whose body could not be read may follow a started job. + */ export function applyRefusal(error) { - if (!Number.isInteger(error?.status)) return { toast: NOT_CONFIRMED, lost: true }; + const status = error?.status; + const known = error?.code && (error.code === 'busy' || error.code === 'plan-used' || REVIEW_AGAIN.has(error.code) + || SERVER_SAYS.has(error.code)); + const refused = Number.isInteger(status) && status >= 400 && status < 500 && error.refused === true; + if (!known && !refused) return { toast: NOT_CONFIRMED, lost: true }; if (error.code === 'busy') return { toast: busyToast(error.running) }; if (error.code === 'plan-used') { const job = error.job; @@ -203,12 +213,14 @@ export function showPlan(plan, { api, store, scope, trigger = null, reviewAgain, /** * Review `operation` (`POST /maintenance/preview`) and show its plan. `options` are the preview's (duplicate_files: - * `{job_id, keep}`). Resolves once the plan is shown or the refusal toasted. + * `{job_id, keep}`). `hold` (default `trigger`, which gets focus back) is the pressed button: it reads "Reviewing…" + * and ignores presses until the review answered, so a double press sends one preview. Resolves once the plan is shown + * or the refusal toasted. */ -export async function reviewPlan({ api, store, scope, operation, options = {}, trigger = null, notify = showWorkspaceToast, - navigate = navigateTo }) { +export async function reviewPlan({ api, store, scope, operation, options = {}, trigger = null, hold = trigger, + notify = showWorkspaceToast, navigate = navigateTo }) { const again = () => reviewPlan({ api, store, scope, operation, options, trigger, notify, navigate }); - await holding(trigger, 'Reviewing…', async () => { + await holding(hold, 'Reviewing…', async () => { let plan; try { plan = await api.post(PREVIEW_URL, { operation, options }, { signal: scope.signal }); @@ -344,7 +356,7 @@ export async function openDuplicatesReview({ api, store, scope, jobId, trigger = scope.listen(cancel, 'click', () => closeWorkspaceDialog()); scope.listen(review, 'click', () => { if (review.disabled) return; - reviewPlan({ api, store, scope, operation: 'duplicate_files', trigger, + reviewPlan({ api, store, scope, operation: 'duplicate_files', trigger, hold: review, options: { job_id: jobId, keep: keepEntries(chosen) }, notify, navigate }); }); const nodes = [status, groupsNode]; diff --git a/web/js/workspace/system-maintenance.mjs b/web/js/workspace/system-maintenance.mjs index c035488a..742f7c65 100644 --- a/web/js/workspace/system-maintenance.mjs +++ b/web/js/workspace/system-maintenance.mjs @@ -28,6 +28,7 @@ import { noun } from './plural.mjs'; export const DUPLICATES_KIND = 'duplicates'; export const FIND_FAILED = 'The duplicate search could not start. Try again.'; export const NOTHING_CACHED_REASON = 'Nothing is cached in this browser.'; +export const STORAGE_UNCHECKED_REASON = 'This browser did not allow checking its storage.'; export const METADATA_CACHE_DESCRIPTION = 'Clear only Prompt Manager metadata cached by this browser.'; /** The concept's two surfaces and their rows: `[title, description, label, action, danger]`. */ @@ -75,10 +76,13 @@ export async function findDuplicates({ store, scope, notify = showWorkspaceToast } } -/** D102: the Metadata cache row's description: the concept's, plus the reason its review is unavailable. */ +/** + * D102: the Metadata cache row's description: the concept's, plus the reason its review is unavailable. A browser that + * refused to list its storage is never claimed empty. + */ export function metadataCacheDescription(entries) { if (entries.keys.length + entries.caches.length > 0) return METADATA_CACHE_DESCRIPTION; - return `${METADATA_CACHE_DESCRIPTION} ${NOTHING_CACHED_REASON}`; + return `${METADATA_CACHE_DESCRIPTION} ${entries.denied ? STORAGE_UNCHECKED_REASON : NOTHING_CACHED_REASON}`; } /** D102: review the owned browser entries, then clear exactly those and say what happened. */ @@ -136,7 +140,8 @@ function metadataCacheRow({ scope, notify, cache }) { */ export function mountMaintenance(container, { api, activity, scope, navigate = navigateTo, notify = showWorkspaceToast, cache = ownership() }) { - const reviewMissing = trigger => reviewPlan({ api, store: activity, scope, operation: 'missing_links', trigger, notify, navigate }); + const reviewMissing = (trigger, pressed = trigger) => reviewPlan({ api, store: activity, scope, operation: 'missing_links', + trigger, hold: pressed, notify, navigate }); const actions = { health: button => runHealthAction({ api, activity, scope, button, onReviewMissing: reviewMissing, notify }), duplicates: () => findDuplicates({ store: activity, scope, notify, navigate }), diff --git a/web/js/workspace/system-overview.mjs b/web/js/workspace/system-overview.mjs index f04996d5..91b6a5ef 100644 --- a/web/js/workspace/system-overview.mjs +++ b/web/js/workspace/system-overview.mjs @@ -214,7 +214,7 @@ export function healthNodes(report, { scope, onReviewMissing }) { const close = createButton('Close'); const review = createButton('Review missing links', 'ca-primary'); scope.listen(close, 'click', () => closeWorkspaceDialog()); - scope.listen(review, 'click', () => onReviewMissing()); + scope.listen(review, 'click', () => onReviewMissing(review)); const actions = el('div', undefined, 'dialog-actions'); actions.append(close, review); return [list, el('p', healthSummary(report), 'small'), actions]; @@ -269,13 +269,13 @@ export async function runHealthCheck(api, { activity, signal } = {}) { /** * Open the Health report for `report` (Maintenance's notice and its Check action). Its "Review missing links" runs - * `onReviewMissing(trigger)`: Maintenance reviews a fresh Missing-file links plan (S7), never a cleanup; without one it - * goes to Maintenance. + * `onReviewMissing(trigger, pressed)`: Maintenance reviews a fresh Missing-file links plan (S7), never a cleanup, + * holding the pressed button while it is read; without one it goes to Maintenance. */ export function openHealthReport(report, { scope, trigger = null, navigate = navigateTo, onReviewMissing = null }) { openContentDialog({ scope, title: 'Health report', trigger, nodes: healthNodes(report, { scope, - onReviewMissing: () => { - if (onReviewMissing) onReviewMissing(trigger); + onReviewMissing: pressed => { + if (onReviewMissing) onReviewMissing(trigger, pressed); else { closeWorkspaceDialog(); navigate('#/system/maintenance'); } } }) }); }