diff --git a/py/api/autotag_routes.py b/py/api/autotag_routes.py index b3b3158..f72db79 100644 --- a/py/api/autotag_routes.py +++ b/py/api/autotag_routes.py @@ -64,6 +64,12 @@ def path_is_within(candidate, root) -> bool: return child != parent and child.is_relative_to(parent) +async def _refusal(title): + """The single error event of an auto-tag batch refused by another job.""" + message = f"{title} already running" + yield {"type": "error", "status": "error", "message": message} + + class AutotagRoutesMixin: """Mixin providing auto-tagging API endpoints.""" @@ -454,7 +460,10 @@ class AutotagRoutesMixin: return web.json_response( {"success": False, "error": _BUSY_ERROR}, status=409 ) - return self._long_job_busy_response(busy.title) + # Both legacy pages read this route as a stream, where a 409 reads + # as a lost connection: refuse with one error frame instead (the + # admin page keys on ``type``, the gallery on ``status``). + return await self._stream_sse(request, _refusal(busy.title)) async with self._jobs.hold( AUTOTAG_BATCH_JOB_KIND, "Auto-tag images (legacy page)", diff --git a/py/api/lora_integration.py b/py/api/lora_integration.py index 18da1df..7e66c2d 100644 --- a/py/api/lora_integration.py +++ b/py/api/lora_integration.py @@ -37,6 +37,15 @@ def _lora_settings_mutation(values): return mutate +def _lora_error_frame(message): + """An import stream's error event, as the admin page's modal reads it. + + The modal shows ``status`` and ``progress`` from every frame and closes + on ``type: error``, naming ``message``. + """ + return {"type": "error", "status": message, "progress": 0, "message": message} + + class LoraIntegrationMixin: """Mixin providing LoraManager detection, scanning, and trigger word endpoints.""" @@ -290,7 +299,7 @@ class LoraIntegrationMixin: if busy is not None: await response.prepare(request) message = f"{busy.title} already running" - await send_progress({"type": "error", "message": message}) + await send_progress(_lora_error_frame(message)) await response.write_eof() return response @@ -303,9 +312,7 @@ class LoraIntegrationMixin: except Exception as e: self.logger.exception("LoRA scan failed") job.finish("failed") - await send_progress( - {"type": "error", "message": self._public_error(e)} - ) + await send_progress(_lora_error_frame(self._public_error(e))) await response.write_eof() return response diff --git a/tests/js/legacy-refusals.test.js b/tests/js/legacy-refusals.test.js new file mode 100644 index 0000000..6daec1c --- /dev/null +++ b/tests/js/legacy-refusals.test.js @@ -0,0 +1,150 @@ +// The legacy admin and gallery pages show a server refusal's reason (#220, PR #289 review I1 and M8): a job that +// runs in the workspace refuses their record-changing requests, and a corrupt config.json refuses a settings save. +// Each page must name the reason, not report a lost connection, a bare HTTP status or a success. +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const vm = require('node:vm'); + +function elements() { + const nodes = {}; + return { + nodes, + getElementById: id => nodes[id] ||= { id, textContent: '', innerHTML: '', disabled: false, style: {}, + value: '', checked: false, addEventListener() {}, querySelectorAll: () => [] }, + }; +} + +function jsonResponse(status, body) { + return { ok: status >= 200 && status < 300, status, statusText: status === 409 ? 'Conflict' : 'OK', + json: async () => body }; +} + +function sseResponse(frames) { + const bytes = new TextEncoder().encode(frames.map(frame => `data: ${JSON.stringify(frame)}\n\n`).join('')); + let sent = false; + return { ok: true, status: 200, body: { getReader: () => ({ + read: async () => (sent ? { done: true } : (sent = true, { done: false, value: bytes })), + }) } }; +} + +function load(file, className, fetchUrl, extra = {}, dom = elements()) { + const timers = []; + const notifications = []; + const storage = new Map([['pm_cached', '1'], ['userData', 'keep']]); + const localStorage = { + getItem: key => storage.get(key) ?? null, + setItem: (key, value) => storage.set(key, String(value)), + removeItem: key => storage.delete(key), + }; + const context = vm.createContext({ + document: { readyState: 'loading', addEventListener() {}, getElementById: dom.getElementById, + querySelectorAll: () => [] }, + window: {}, console: { error() {}, warn() {}, debug() {} }, + setTimeout: callback => timers.push(callback), + confirm: () => true, + localStorage, + Object: { ...Object, keys: target => (target === localStorage ? [...storage.keys()] : Object.keys(target)) }, + TextDecoder, TextEncoder, JSON, + ApiClient: { fetchUrl }, + ...extra, + }); + const source = fs.readFileSync(path.join(__dirname, `../../web/js/${file}`), 'utf8'); + const Page = vm.runInContext(`${source}\n${className};`, context, { filename: path.join(__dirname, `../../web/js/${file}`) }); + const page = Object.create(Page.prototype); + page.showNotification = (message, type) => notifications.push([type, message]); + page.showModal = id => notifications.push(['show', id]); + page.hideModal = id => notifications.push(['hide', id]); + return { page, dom, timers, notifications, storage }; +} + +const BUSY = 'Scan for metadata already running'; + +test('gallery cache clear reports a refused server clear and never claims success', async () => { + const calls = []; + const { page, dom, notifications, storage } = load('gallery.js', 'GalleryManager', async url => { + calls.push(url); + return jsonResponse(409, { success: false, error: BUSY }); + }); + page.getSettings = () => ({ thumbnailsGenerated: true }); + dom.getElementById('thumbnailStatus').textContent = 'Generated'; + + await page.clearCache(); + + assert.deepEqual(calls, ['/prompt_manager/images/clear-thumbnails']); + assert.deepEqual(notifications, [['error', `Thumbnails were not cleared: ${BUSY}`]]); + assert.equal(dom.nodes.thumbnailStatus.textContent, 'Generated'); + assert.equal(storage.get('gallerySettings'), undefined); // thumbnailsGenerated is not reset +}); + +test('gallery cache clear still says so when the server clears', async () => { + const { page, dom, notifications, storage } = load('gallery.js', 'GalleryManager', + async () => jsonResponse(200, { success: true, cleared_count: 3 })); + page.getSettings = () => ({ thumbnailsGenerated: true }); + + await page.clearCache(); + + assert.deepEqual(notifications, [['success', 'PromptManager cache cleared successfully! Original images untouched.']]); + assert.equal(dom.nodes.thumbnailStatus.textContent, 'Not generated'); + assert.equal(JSON.parse(storage.get('gallerySettings')).thumbnailsGenerated, false); + assert.equal(storage.get('userData'), 'keep'); + assert.equal(storage.has('pm_cached'), false); +}); + +test('admin LoRA import shows a refusal frame and closes its modal', async () => { + const { page, dom, timers, notifications } = load('admin.js', 'PromptAdmin', + async () => sseResponse([{ type: 'error', status: BUSY, progress: 0, message: BUSY }])); + page.saveLoraSettings = async () => true; + page.loadStatistics = () => assert.fail('a refused import must not reload statistics'); + + await page.runLoraImport(); + + assert.equal(dom.nodes.loraImportStatus.textContent, `Error: ${BUSY}`); + assert.deepEqual(notifications.filter(([type]) => type === 'error'), [['error', `LoRA import failed: ${BUSY}`]]); + assert.equal(timers.length, 1); + timers[0](); + assert.deepEqual(notifications.at(-1), ['hide', 'loraImportModal']); +}); + +test('admin settings save shows why config.json refused it', async () => { + const message = 'config.json cannot be read, so nothing was saved. Fix or remove the file, then save again.'; + const { page, notifications } = load('admin.js', 'PromptAdmin', + async () => jsonResponse(409, { success: false, persisted: false, code: 'config-unreadable', error: message })); + page.settings = {}; + page._collectScanPaths = () => []; + page.saveLoraSettings = async () => assert.fail('nothing else is saved after a refusal'); + + await page.saveSettings(); + + assert.deepEqual(notifications, [['error', `Failed to save settings: ${message}`]]); +}); + +test('admin settings save keeps its plain message when the reply has no reason', async () => { + const { page, notifications } = load('admin.js', 'PromptAdmin', async () => ({ ok: false, status: 500, + json: async () => { throw new SyntaxError('not json'); } })); + page.settings = {}; + page._collectScanPaths = () => []; + + await page.saveSettings(); + + assert.deepEqual(notifications, [['error', 'Failed to save settings']]); +}); + +test('admin maintenance names the job that refused it', async () => { + const { page, dom, notifications } = load('admin.js', 'PromptAdmin', + async () => jsonResponse(409, { success: false, error: BUSY }), { + escapeHtml: text => String(text), + document: { readyState: 'loading', addEventListener() {}, getElementById: id => dom.getElementById(id), + querySelectorAll: selector => (selector === '.maintenance-option:checked' ? [{ id: 'vacuum' }] : []) }, + }); + + await page.runMaintenance(); + + const results = dom.nodes.maintenanceResults.innerHTML; + assert.match(results, /Maintenance Failed/); + assert.match(results, new RegExp(BUSY)); + assert.doesNotMatch(results, /HTTP 409/); + assert.deepEqual(notifications, [['error', '❌ Maintenance failed']]); + assert.equal(dom.nodes.runMaintenanceBtn.disabled, false); +}); diff --git a/tests/test_api_lora.py b/tests/test_api_lora.py index 743f1e3..78957d4 100644 --- a/tests/test_api_lora.py +++ b/tests/test_api_lora.py @@ -767,6 +767,12 @@ class TestLoraScanRoute(LoraAPITestCase): body = await resp.text() self.assertNotIn(str(self.tmp_root), body) self.assertIn("x.db", body) + # The admin page's modal shows the failure as its status and closes. + error = self._sse_events(body)[-1] + self.assertEqual( + (error["type"], error["status"], error["progress"]), + ("error", error["message"], 0), + ) @staticmethod def _sse_events(text): diff --git a/tests/test_job_admission.py b/tests/test_job_admission.py index 61cf20c..59e4821 100644 --- a/tests/test_job_admission.py +++ b/tests/test_job_admission.py @@ -295,6 +295,70 @@ class TestLegacyRoutesHoldTheRegistry(_LegacyHoldCase): await self._busy(resp) self.assertEqual(self._db_digest(), before) + async def test_read_only_maintenance_runs_during_scan(self): + # The legacy statistics and consistency checks only read: they hold + # the registry as "read", so a long scan does not block them. + self.api.db.save_prompt(text="keep me", prompt_hash="h1") + _gate, scan = self._run_job() + listed = [] + real = self.api._run_maintenance_operations + + def observed(operations): + listed.extend( + (job.kind, job.admission, job.state) for job in self.api._jobs.list() + ) + return real(operations) + + self.api._run_maintenance_operations = observed + resp = await self.client.request( + "POST", + "/prompt_manager/maintenance", + json={ + "operations": [ + "statistics", + "check_consistency", + "check_hash_duplicates", + ] + }, + ) + body = await resp.json() + self.assertEqual(resp.status, 200) + self.assertIs(body["success"], True) + self.assertEqual( + set(body["results"]), + {"statistics", "check_consistency", "check_hash_duplicates"}, + ) + self.assertIn(("maintenance", "read", "running"), listed) + self.assertEqual(scan.state, "running") + maintenance = self.api._jobs.list()[-1] + self.assertEqual( + (maintenance.kind, maintenance.admission, maintenance.state), + ("maintenance", "read", "finished"), + ) + + async def test_mixed_maintenance_selection_is_refused_during_scan(self): + # One write operation among read-only ones makes the run "records". + self.api.db.save_prompt(text="keep me", prompt_hash="h1") + before = self._db_digest() + self._run_job() + resp = await self.client.request( + "POST", + "/prompt_manager/maintenance", + json={"operations": ["statistics", "vacuum"]}, + ) + await self._busy(resp) + self.assertEqual(self._db_digest(), before) + + async def test_default_maintenance_selection_is_refused_during_scan(self): + # No "operations" key means the default list, which vacuums. + self.api.db.save_prompt(text="keep me", prompt_hash="h1") + before = self._db_digest() + self._run_job() + resp = await self.client.request("POST", "/prompt_manager/maintenance", json={}) + await self._busy(resp) + self.assertEqual(self._db_digest(), before) + self.assertEqual([j.kind for j in self.api._jobs.list()], ["scan"]) + async def test_legacy_maintenance_refused_while_image_tags_runs(self): self._run_job("image-tags", "Add tags to 3 images") resp = await self.client.request("POST", "/prompt_manager/maintenance") @@ -365,18 +429,43 @@ class TestLegacyRoutesHoldTheRegistry(_LegacyHoldCase): text = await resp.text() self.assertEqual(resp.status, 200) # the legacy page reads a stream events = [json.loads(line[6:]) for line in text.split("\n\n") if line] + # The admin page's modal shows ``status`` and closes on ``type: error``. + message = "Scan for metadata already running" self.assertEqual( events, - [{"type": "error", "message": "Scan for metadata already running"}], + [{"type": "error", "status": message, "progress": 0, "message": message}], ) self.assertEqual(imported, []) async def test_legacy_autotag_refused_while_scan_runs(self): + # Both pages read this route as a stream (a 409 reads as a lost + # connection there), so a refusal by another job is one error frame: + # the admin page keys on ``type``, the gallery on ``status``. self._run_job() resp = await self.client.request( "POST", "/prompt_manager/autotag/start", json={} ) - await self._busy(resp) + text = await resp.text() + self.assertEqual(resp.status, 200) + self.assertEqual(resp.headers["Content-Type"], "text/event-stream") + events = [json.loads(line[6:]) for line in text.split("\n\n") if line] + message = "Scan for metadata already running" + self.assertEqual( + events, [{"type": "error", "status": "error", "message": message}] + ) + self.assertEqual( + [j.kind for j in self.api._jobs.list() if j.kind == "autotag"], [] + ) + + async def test_second_legacy_autotag_batch_keeps_its_json_409(self): + from py.api.autotag_routes import _BUSY_ERROR + + self._run_job("autotag", "Auto-tag images (legacy page)") + resp = await self.client.request( + "POST", "/prompt_manager/autotag/start", json={} + ) + self.assertEqual(resp.status, 409) + self.assertEqual(await resp.json(), {"success": False, "error": _BUSY_ERROR}) async def test_legacy_payloads_unchanged_when_idle(self): resp = await self.client.request( diff --git a/web/js/admin.js b/web/js/admin.js index da699ac..e74d347 100644 --- a/web/js/admin.js +++ b/web/js/admin.js @@ -1180,6 +1180,11 @@ `Done — ${data.imported} imported, ${data.skipped} skipped`; this.loadStatistics(); setTimeout(() => this.hideModal("loraImportModal"), 2000); + } else if (data.type === "error") { + // A refusal (another job runs) or a failed import: name it and close the modal. + document.getElementById("loraImportStatus").textContent = `Error: ${data.message}`; + this.showNotification(`LoRA import failed: ${data.message}`, "error"); + setTimeout(() => this.hideModal("loraImportModal"), 3000); } } catch (e) { /* skip malformed SSE */ } } @@ -1231,10 +1236,13 @@ } this.hideModal("settingsModal"); } else { - throw new Error("Failed to save settings"); + // A refusal names its reason (a corrupt config.json, a changed file): show it. + const data = await response.json().catch(() => ({})); + throw new Error(data.error ? `Failed to save settings: ${data.error}` : "Failed to save settings"); } } catch (error) { - this.showNotification("Failed to save settings", "error"); + const reason = error.message.startsWith("Failed to save settings:") ? error.message : "Failed to save settings"; + this.showNotification(reason, "error"); } } @@ -1848,7 +1856,9 @@ throw new Error(data.error || 'Maintenance failed'); } } else { - throw new Error(`HTTP ${response.status}: ${response.statusText}`); + // A refusal names the job that runs ("Scan for metadata already running"). + const data = await response.json().catch(() => ({})); + throw new Error(data.error || `HTTP ${response.status}: ${response.statusText}`); } } catch (error) { console.error('Maintenance error:', error); diff --git a/web/js/gallery.js b/web/js/gallery.js index 56596f7..7f6b2d6 100644 --- a/web/js/gallery.js +++ b/web/js/gallery.js @@ -2129,8 +2129,11 @@ Seed: ${this.currentMetadata.seed || 'Unknown'}`; headers: { 'Content-Type': 'application/json' } }); - if (response.ok) { - const data = await response.json(); + if (!response.ok) { + // Refused (a running job holds the thumbnails): say why and claim nothing. + const data = await response.json().catch(() => ({})); + this.showNotification(`Thumbnails were not cleared: ${data.error || `HTTP ${response.status}`}`, 'error'); + return; } } catch (e) { console.warn('Could not clear server thumbnails:', e);