From a073dc613e444d4a8577b8a99c4e928e9c137780 Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Sun, 25 Oct 2026 09:48:55 -0700 Subject: [PATCH] fix(lora): keep the save outcome, honour the umask and sweep stale downloads Leaving Integrations drops the CivitAI key draft only when a key was typed, so the last Save's outcome survives and Sources still marks a refused source. The key status line is rewritten only when it changes. Cached CivitAI images are opened like a plain save, so they keep the umask's mode, and each import removes .pm-*.part files older than an hour from the image cache. A legacy scan stopped because its page left ends cancelled in Activity, and staging is awaited to the end even if the handler is cancelled again. --- TESTING.md | 12 ++-- py/api/lora_import_routes.py | 1 + py/api/lora_integration.py | 31 ++++++++-- py/lora_utils.py | 59 ++++++++++++++++--- .../system-integrations-concept.spec.mjs | 11 +++- .../workspace-system-integrations.spec.mjs | 35 +++++++++++ tests/test_lora_import.py | 19 ++++++ tests/test_lora_utils.py | 56 ++++++++++++++++++ web/js/workspace/system-integrations.mjs | 11 +++- 9 files changed, 213 insertions(+), 22 deletions(-) diff --git a/TESTING.md b/TESTING.md index aa86024..100a636 100644 --- a/TESTING.md +++ b/TESTING.md @@ -2664,8 +2664,9 @@ Recorded deviations (plan D60–D70, D74, D75, D79–D82 renumbered D92–D102, A `p.small#civitai-key-status[role=status]` (it also describes the field) says "A key is saved." / "No key saved." / "The new key replaces the saved one when you save." / "The saved key is removed when you save."; pressing "Clear saved key" keeps the - keyboard on the key field. A typed key is dropped when the section is left (the field - comes back empty). The field is `autocomplete=new-password`, so browsers never offer it + keyboard on the key field, and the line is rewritten only when its text changes. A typed + key is dropped when the section is left (the field comes back empty); leaving without one + keeps the last Save's outcome, so Sources still marks a refused source. The field is `autocomplete=new-password`, so browsers never offer it as a login; the concept's prototype warning is dropped. The LoRA rows save through the one Save row: `POST /lora/enable` with `expected_revision` (a config.json changed elsewhere is @@ -3147,9 +3148,12 @@ imports changed meanwhile), `blocked-by-pairs`, `nothing-to-import`, `backup-fai whatever happened after. The tests change real files behind a held download (`test_files_changed_while_staging_fail_the_job`, removed and added files too). The replace binds the category, not one parameter per prompt, so SQLite's variable limit -never caps a collection. The legacy `POST /lora/scan` runs the same stages under its SSE +never caps a collection. Cached CivitAI images are written to a `.pm-*.part` file in the +same folder and renamed into place, with the umask's mode as a plain save gives; each import +first removes `.pm-*.part` files older than an hour from the image cache and its LoRA +folders (`TestStaleDownloadSweep`). The legacy `POST /lora/scan` runs the same stages under its SSE event shape; a page that closes its connection stops staging before its next LoRA, and the -scan keeps its records hold until staging has stopped +scan keeps its records hold until staging has stopped, then ends `cancelled` in Activity (`test_a_disconnected_legacy_scan_stops_and_holds_until_it_stopped`) (`test_legacy_scan_events_unchanged_on_success`), holds the registry as "LoRA import (admin page)" and takes no safety backup. `POST /lora/enable` accepts the optional `expected_revision`. LoraManager detection also searches every diff --git a/py/api/lora_import_routes.py b/py/api/lora_import_routes.py index 1531a8c..e0c3122 100644 --- a/py/api/lora_import_routes.py +++ b/py/api/lora_import_routes.py @@ -135,6 +135,7 @@ class LoraImportRoutesMixin: from ..config import IntegrationConfig cache_dir = await self._run_in_executor(lora_utils.get_lora_image_cache_dir) + await self._run_in_executor(lora_utils.sweep_stale_downloads, cache_dir) loop = asyncio.get_running_loop() def report(index, record, staged): diff --git a/py/api/lora_integration.py b/py/api/lora_integration.py index 0d562a0..f038a22 100644 --- a/py/api/lora_integration.py +++ b/py/api/lora_integration.py @@ -73,6 +73,13 @@ def _closed(request): return closed +def _stopped(error) -> bool: + """Whether a legacy scan ended because its page closed the connection.""" + from .. import lora_import + + return isinstance(error, (ConnectionError, lora_import.ImportCancelled)) + + class LoraIntegrationMixin: """Mixin providing LoraManager detection, scanning, and trigger word endpoints.""" @@ -357,8 +364,13 @@ class LoraIntegrationMixin: await self._lora_scan_stream( allowed, send_progress, disconnected=_closed(request) ) + except asyncio.CancelledError: + # The server cancelled the handler when the page left. + job.finish("cancelled") + raise except Exception as e: - job.finish("failed") + # A page that left stopped the scan; nothing failed (R2). + job.finish("cancelled" if _stopped(e) else "failed") await self._send_scan_failure(e, send_progress) with contextlib.suppress(ConnectionError, RuntimeError): await response.write_eof() @@ -375,14 +387,13 @@ class LoraIntegrationMixin: import) are warnings in the admin page's own words; anything else is logged with its traceback. """ - from .. import lora_import from .lora_import_routes import ( EmptyReplacement, PairDependencyError, PlanChanged, ) - if isinstance(error, (ConnectionError, lora_import.ImportCancelled)): + if _stopped(error): self.logger.info("LoRA scan stopped: the page closed its connection") return if isinstance(error, PlanChanged): @@ -498,8 +509,18 @@ class LoraIntegrationMixin: await report(*events.get_nowait()) finally: stop.set() - with contextlib.suppress(Exception): - await asyncio.shield(staging) + # Staging must have stopped before the records hold is released, + # even when the handler is cancelled again while it waits (R3). + interrupted = False + while not staging.done(): + try: + await asyncio.shield(staging) + except asyncio.CancelledError: + interrupted = True + except Exception: + pass + if interrupted: + raise asyncio.CancelledError staged = await staging if not await self.lora_files_unchanged(lm_path, reviewed): raise LoraFilesChanged(_FILES_CHANGED) diff --git a/py/lora_utils.py b/py/lora_utils.py index 7a23f68..cde7cc3 100644 --- a/py/lora_utils.py +++ b/py/lora_utils.py @@ -524,21 +524,35 @@ def _download_one(url: str, local_path: Path, api_key: str) -> Optional[str]: return None +_PART_PREFIX = ".pm-" +_PART_SUFFIX = ".part" +# A temp download older than this was left by a killed process, not a live one +# (each download has a 10 s timeout). +STALE_PART_SECONDS = 3600 + + def _save_atomically(image, local_path: Path) -> None: """Write ``image`` as JPEG so ``local_path`` only ever names a whole file. - The image goes to a temp file in the same folder, which is closed before - ``os.replace`` puts it in place (Windows cannot replace an open file); a - failure removes the temp file. A reader (another import's cache check) - never sees a half-written image. + The image goes to a ``.pm-*.part`` file in the same folder, which is + closed before ``os.replace`` puts it in place (Windows cannot replace an + open file); a failure removes the temp file. A reader (another import's + cache check) never sees a half-written image. The temp file is opened + like ``img.save`` opens a file, so the image keeps the umask's mode. """ - import tempfile + import secrets - fd, temp = tempfile.mkstemp( - dir=str(local_path.parent), prefix=".pm-", suffix=".part" - ) + while True: + temp = local_path.parent / ( + f"{_PART_PREFIX}{secrets.token_hex(8)}{_PART_SUFFIX}" + ) + try: + handle = open(temp, "xb") + except FileExistsError: + continue + break try: - with os.fdopen(fd, "wb") as handle: + with handle: image.save(handle, "JPEG", quality=85) os.replace(temp, local_path) except BaseException: @@ -549,6 +563,33 @@ def _save_atomically(image, local_path: Path) -> None: raise +def sweep_stale_downloads(cache_dir: Path) -> None: + """Remove ``.pm-*.part`` files a killed download left in the image cache. + + Only that exact pattern, only in ``cache_dir`` and its LoRA folders, and + only once older than ``STALE_PART_SECONDS``; a file that cannot be read + or removed is left for a later import. + """ + import time + + cutoff = time.time() - STALE_PART_SECONDS + try: + folders = [cache_dir] + [p for p in cache_dir.iterdir() if p.is_dir()] + except OSError: + return + for folder in folders: + try: + candidates = list(folder.glob(f"{_PART_PREFIX}*{_PART_SUFFIX}")) + except OSError: + continue + for path in candidates: + try: + if path.is_file() and path.stat().st_mtime < cutoff: + os.remove(path) + except OSError: + logger.debug(f"Could not remove stale download {path}") + + def _civitai_cache_plan( metadata: Dict, metadata_path: Path, cache_dir: Path ) -> Tuple[Path, List[str], List[Tuple[str, Path]]]: diff --git a/tests/browser/system-integrations-concept.spec.mjs b/tests/browser/system-integrations-concept.spec.mjs index 3990e72..b9c41cd 100644 --- a/tests/browser/system-integrations-concept.spec.mjs +++ b/tests/browser/system-integrations-concept.spec.mjs @@ -71,7 +71,6 @@ function measureIntegrations(mainSelector) { // The concept's `.small` text (its Save row status); production's row notes use the same class. small: css(layout.querySelector('#settings-status'), 'fontSize', 'color'), note: note && css(note, 'fontSize', 'color'), - // Whether each select shows its whole chosen option (its text within the box, less the native arrow). // Whether each select is as wide as the browser lays out a copy holding only its chosen option, i.e. it shows // that option whole (the browser's own select sizing, arrow included). selectsFit: [...layout.querySelectorAll('.setting-row > select')].map(select => { @@ -178,6 +177,16 @@ for (const theme of THEMES) for (const [width, height] of SIZES) { }); } +// D106 below the concept's smallest size: on a 320px phone both selects still show their whole option, without page +// scroll (S10 re-review N9). +for (const theme of THEMES) test(`integrations selects fit at 320 wide ${theme}`, async ({ page, request }) => { + await page.setViewportSize({ width: 320, height: 700 }); + await fakeLoraManager(request); + const production = await productionIntegrations(page, theme); + expect(production.selectsFit).toEqual([true, true]); + expect(production.pageScroll).toBe(false); +}); + test('integrations comparison detects mutations', async ({ page, request }) => { await page.setViewportSize({ width: 1600, height: 1100 }); await fakeLoraManager(request); diff --git a/tests/browser/workspace-system-integrations.spec.mjs b/tests/browser/workspace-system-integrations.spec.mjs index faf78e1..47358ef 100644 --- a/tests/browser/workspace-system-integrations.spec.mjs +++ b/tests/browser/workspace-system-integrations.spec.mjs @@ -218,6 +218,18 @@ test('key never prefilled; blank keeps; Clear saved key clears; saved through Sa await expect(field).toHaveValue(''); await expect(key.locator('p.small')).toHaveText('A key is saved.'); expect(statusBodies.join('')).not.toContain('sk-test-key'); + // The announced line is rewritten only when its text changes, not on every edit elsewhere (S10 re-review N8). + await page.evaluate(() => { + window.__keyNoteWrites = 0; + new MutationObserver(records => { window.__keyNoteWrites += records.length; }) + .observe(document.querySelector('#civitai-key-status'), { childList: true, characterData: true, subtree: true }); + }); + const triggers = settingOf(page, 'Automatically include trigger words').locator('input'); + await triggers.check(); + await expect(status(page)).toHaveText('Unsaved changes.'); + await triggers.uncheck(); + await expect(status(page)).toHaveText('No unsaved changes.'); + expect(await page.evaluate(() => window.__keyNoteWrites)).toBe(0); // A blank field keeps the saved key: nothing to save. await field.fill(' '); await expect(status(page)).toHaveText('No unsaved changes.'); @@ -285,6 +297,29 @@ test('a save where only the LoRA part is written says so; the next save needs no expect((await serverJson(request, '/prompt_manager/settings')).sources.configured.map(source => source.path)).toEqual(['output']); }); +test('a partial save made from Integrations keeps its outcome when Sources opens (S10 re-review R1)', + async ({ page, request }) => { + await lora(request); + await expect(await request.post('/__test__/system/missing-source', { data: { inside: true } })).toBeOK(); + await openIntegrations(page); + const nav = page.locator('#workspace-main .section-nav'); + const sourceRow = title => layoutOf(page).locator('#source-list .operation').filter({ has: page.locator('h3', { hasText: title }) }); + await nav.getByRole('button', { name: 'Sources & scans', exact: true }).click(); + await expect(sourceRow('unplugged')).toBeVisible(); + await expect(await request.post('/__test__/system/unplug')).toBeOK(); + await sourceRow('ComfyUI input').getByRole('button', { name: 'Remove', exact: true }).click(); + // Back in Integrations, a LoRA change, then Save from there. + await nav.getByRole('button', { name: 'Integrations', exact: true }).click(); + await settingOf(page, 'Automatically include trigger words').locator('input').check(); + await save(page).click(); + const reason = "output/unplugged can't be used: Gallery root does not exist. Remove it or reconnect it, then save."; + await expect(status(page)).toHaveText(reason); + // Leaving Integrations (no key was typed) keeps the outcome: Sources shows the reason on the refused row. + await nav.getByRole('button', { name: 'Sources & scans', exact: true }).click(); + await expect(sourceRow('unplugged')).toHaveAttribute('aria-invalid', 'true'); + await expect(status(page)).toHaveText(reason); + }); + test('Review import shows replacement, blocked items and network effects with the backup box; Start opens the job (D107)', async ({ page, request }) => { await lora(request, { loras: 3, civitai: true, previous: 2 }); diff --git a/tests/test_lora_import.py b/tests/test_lora_import.py index 3433b49..ffdcff5 100644 --- a/tests/test_lora_import.py +++ b/tests/test_lora_import.py @@ -13,6 +13,7 @@ import sqlite3 import sys import tempfile import threading +import time import unittest from pathlib import Path from unittest.mock import patch @@ -531,6 +532,9 @@ class LoraImportRouteTests(LoraImportAPITestCase): break await asyncio.sleep(0.01) self.assertIsNone(self.api._jobs.active("lora_import")) + # R2 (re-review): the user left and nothing failed, so Activity says so. + job = next(j for j in self.api._jobs.list() if j.kind == "lora_import") + self.assertEqual(job.state, "cancelled") # Staging stopped at the next LoRA: no download runs after the hold. served = len(self.requests) self.assertLess(served, 4) @@ -538,6 +542,21 @@ class LoraImportRouteTests(LoraImportAPITestCase): self.assertEqual(len(self.requests), served) self.assertEqual(_snapshot(self.api.db), before) + async def test_an_import_sweeps_stale_temp_downloads_from_the_cache(self): + # N7 (re-review): a download killed mid-write leaves a .pm-*.part file; + # the next import removes it once it is an hour old. + self._two_loras() + stale = self.cache_dir / "old_lora" / ".pm-dead.part" + stale.parent.mkdir(parents=True) + stale.write_bytes(b"x") + stamp = time.time() - 2 * 3600 + os.utime(stale, (stamp, stamp)) + + resp = await self.client.request("POST", SCAN) + await resp.text() + + self.assertFalse(stale.exists()) + async def test_legacy_scan_events_unchanged_on_success(self): self._two_loras() (self.loras / "broken.metadata.json").write_text("{{{") diff --git a/tests/test_lora_utils.py b/tests/test_lora_utils.py index 24ebe0e..81f3357 100644 --- a/tests/test_lora_utils.py +++ b/tests/test_lora_utils.py @@ -12,6 +12,7 @@ import shutil import sys import tempfile import threading +import time import types import unittest import urllib.parse @@ -576,6 +577,61 @@ class TestDownloadIsAtomic(unittest.TestCase): self.assertIsNone(result) self.assertEqual(list(self.tmpdir.iterdir()), []) + @unittest.skipIf(os.name == "nt", "POSIX file modes") + def test_the_image_gets_the_permissions_a_plain_save_gives(self): + # S10 re-review N7: the cached JPEG honours the umask like img.save did. + previous = os.umask(0o022) + self.addCleanup(os.umask, previous) + opener = _FakeOpener(_FakeResponse(_png_bytes())) + with patch.object(lora_utils, "_open_url", opener): + lora_utils._download_one( + "https://civitai.com/api/download/1", self.target, "" + ) + self.assertEqual(self.target.stat().st_mode & 0o777, 0o644) + + +class TestStaleDownloadSweep(unittest.TestCase): + """S10 re-review N7: a killed download's ``.pm-*.part`` is swept later.""" + + def setUp(self): + self.cache = Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, self.cache, True) + self.lora = self.cache / "my_lora" + self.lora.mkdir() + + def _file(self, path, age_seconds): + path.write_bytes(b"x") + stamp = time.time() - age_seconds + os.utime(path, (stamp, stamp)) + return path + + def test_only_old_temp_downloads_are_removed(self): + old = self._file(self.lora / ".pm-abc123.part", 2 * 3600) + old_top = self._file(self.cache / ".pm-top.part", 2 * 3600) + fresh = self._file(self.lora / ".pm-new.part", 60) + image = self._file(self.lora / "abc.jpg", 2 * 3600) + other = self._file(self.lora / "keep.part", 2 * 3600) + prefixed = self._file(self.lora / ".pm-keep.jpg", 2 * 3600) + deep = self.lora / "nested" + deep.mkdir() + nested = self._file(deep / ".pm-deep.part", 2 * 3600) + + lora_utils.sweep_stale_downloads(self.cache) + + self.assertFalse(old.exists()) + self.assertFalse(old_top.exists()) + for kept in (fresh, image, other, prefixed, nested): + self.assertTrue(kept.exists(), kept.name) + + def test_a_missing_cache_is_fine(self): + lora_utils.sweep_stale_downloads(self.cache / "absent") + + def test_a_file_that_cannot_be_removed_is_left(self): + old = self._file(self.lora / ".pm-abc.part", 2 * 3600) + with patch.object(lora_utils.os, "remove", side_effect=PermissionError): + lora_utils.sweep_stale_downloads(self.cache) + self.assertTrue(old.exists()) + class _FakeHTTPSHandler(urllib.request.HTTPSHandler): """Serves canned responses through a real urllib opener, recording requests. diff --git a/web/js/workspace/system-integrations.mjs b/web/js/workspace/system-integrations.mjs index 1313d5d..456bbbb 100644 --- a/web/js/workspace/system-integrations.mjs +++ b/web/js/workspace/system-integrations.mjs @@ -414,7 +414,9 @@ export function mountIntegrations(container, { api, activity, scope, drafts, cli pathInput.value = values.lora_path || values.lora_detected_path || ''; setNote(triggers, cacheText(values)); keyInput.disabled = unread; - keyNote.textContent = keyText(values, changes) ?? ''; + // A live region: rewritten only when it says something new, so it is not re-announced on every edit. + const note = keyText(values, changes) ?? ''; + if (keyNote.textContent !== note) keyNote.textContent = note; // D114: "Clear saved key" is offered only while a saved key would stay. if (values.lora_has_key === true && changes.clear_civitai_api_key !== true) key.children[0].append(keyActions); else { @@ -434,8 +436,11 @@ export function mountIntegrations(container, { api, activity, scope, drafts, cli }; scope.cleanup(drafts.subscribe(repaint)); // A typed key is held only while its field shows it: leaving the section drops it, since the field comes back - // empty and a hidden key draft could not be seen or discarded (S10 review N4). - scope.cleanup(() => drafts.set('host', 'civitai_api_key', undefined)); + // empty and a hidden key draft could not be seen or discarded (S10 review N4). Only a typed key: any set clears + // the last Save's outcome, which Sources still needs (S10 re-review R1). + scope.cleanup(() => { + if (drafts.changes('host').civitai_api_key !== undefined) drafts.set('host', 'civitai_api_key', undefined); + }); repaint(); return [comfy, surface]; }