From f5856076ceefc68a221e311cd2ad3fa65ceb5a98 Mon Sep 17 00:00:00 2001 From: Vito Date: Wed, 28 Oct 2026 23:12:24 +0000 Subject: [PATCH 1/3] fix(test): own /dev/shm reset scratch by a flock, not a pid (#419) (#439) A browser server inside an --ipc=host container shares the host's /dev/shm but not its PID namespace, so the pid probe saw live servers' scratch folders as orphans and deleted them. A live owner now holds a flock on owner.lock in its folder; the kernel drops it when the owner dies, in any namespace. The cleaner removes only held-named folders whose lock is free and leaves legacy and staging folders alone. Without flock (Windows) the scratch stays on disk. --- tests/browser/fast_reset.py | 84 +++++++++++++++---- tests/test_browser_fast_reset.py | 140 +++++++++++++++++++++++++++---- 2 files changed, 192 insertions(+), 32 deletions(-) diff --git a/tests/browser/fast_reset.py b/tests/browser/fast_reset.py index c305c686..9706b13f 100644 --- a/tests/browser/fast_reset.py +++ b/tests/browser/fast_reset.py @@ -28,6 +28,11 @@ import weakref from pathlib import Path from typing import Any, Callable, TypeVar +try: + import fcntl +except ImportError: # Windows: no flock, and no /dev/shm to share + fcntl = None + T = TypeVar("T") # Set to 1 to run seeders on the live database as before #384 (the equivalence @@ -43,8 +48,13 @@ MEMORY_ROOT = Path("/dev/shm") # the browsers and the other workers; with less free than this, the scratch # copy goes on disk instead. MEMORY_FREE_BYTES = 32 * 1024 * 1024 -# Scratch folders are named -. +# Scratch folders are created as new--, locked, then +# renamed to held--. The pid is for people only: +# ownership is the flock on OWNER_LOCK, which every PID namespace sees (#419). SCRATCH_PREFIX = "pm-reset-" +STAGING_PREFIX = f"{SCRATCH_PREFIX}new-" +HELD_PREFIX = f"{SCRATCH_PREFIX}held-" +OWNER_LOCK = "owner.lock" def _quote(name: str) -> str: @@ -96,29 +106,40 @@ def table_digests(conn: sqlite3.Connection, schema: str = "main") -> dict[str, s return result -def _process_alive(pid: int) -> bool: +def _hold(folder: Path, *, create: bool) -> int: + """Take ``folder``'s owner lock without waiting; the open descriptor.""" + flags = os.O_RDWR | (os.O_CREAT if create else 0) + fd = os.open(folder / OWNER_LOCK, flags, 0o600) try: - os.kill(pid, 0) - except ProcessLookupError: - return False + fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) except OSError: - return True # it exists but belongs to someone else - return True + os.close(fd) + raise + return fd def remove_orphaned_scratch(root: Path) -> None: """Remove scratch folders of servers that died without cleaning up. A server killed outright (SIGKILL, a crash) never runs ``close``, and - tmpfs keeps its folder in memory until reboot. The folder name carries - the owner's process id; a folder whose process is gone is removed. + tmpfs keeps its folder in memory until reboot. A live owner holds the + flock on the folder's owner lock; the kernel drops it when the owner + dies, in whatever PID namespace it ran (a container with ``--ipc=host`` + shares /dev/shm but not the pids, #419). A folder whose lock is free is + removed. Folders without a lock (staging, or named before #419) cannot + be proven orphaned and are left alone. """ - if os.name != "posix": - return # os.kill(pid, 0) would terminate the process on Windows - for folder in root.glob(f"{SCRATCH_PREFIX}*-*"): - pid = folder.name[len(SCRATCH_PREFIX) :].split("-", 1)[0] - if pid.isdigit() and not _process_alive(int(pid)): + if fcntl is None: + return + for folder in root.glob(f"{HELD_PREFIX}*"): + try: + fd = _hold(folder, create=False) + except OSError: + continue # held by its owner, no lock to judge by, or gone + try: shutil.rmtree(folder, ignore_errors=True) + finally: + os.close(fd) class _SeedApi: @@ -162,6 +183,7 @@ class ScratchSeeder: self._fallback = Path(directory) self._memory_root = memory_root self._directory: Path | None = None + self._owner_lock: int | None = None self._on_live = on_live self._scratch = None self._unsynced: "weakref.WeakSet[sqlite3.Connection]" = weakref.WeakSet() @@ -266,6 +288,9 @@ class ScratchSeeder: if self._directory is not None: shutil.rmtree(self._directory, ignore_errors=True) self._directory = None + if self._owner_lock is not None: + os.close(self._owner_lock) # after the removal: never a free lock + self._owner_lock = None def _scratch_directory(self) -> Path: """A folder in memory (tmpfs) where there is one, else ``directory``. @@ -274,10 +299,35 @@ class ScratchSeeder: kernel is writing back the pages they rewrite. """ root = self._memory_root - if root is not None and root.is_dir() and os.access(root, os.W_OK): + if ( + fcntl is not None + and root is not None + and root.is_dir() + and os.access(root, os.W_OK) + ): remove_orphaned_scratch(root) if shutil.disk_usage(root).free >= MEMORY_FREE_BYTES: - prefix = f"{SCRATCH_PREFIX}{os.getpid()}-" - return Path(tempfile.mkdtemp(prefix=prefix, dir=root)) + held = self._held_memory_directory(root) + if held is not None: + return held self._fallback.mkdir(parents=True, exist_ok=True) return self._fallback + + def _held_memory_directory(self, root: Path) -> Path | None: + """A new folder under ``root`` that this seeder holds the lock of. + + It is locked under the staging name, which no cleaner judges, and + only then renamed: no cleaner ever sees it held-named and unlocked. + """ + staging = Path( + tempfile.mkdtemp(prefix=f"{STAGING_PREFIX}{os.getpid()}-", dir=root) + ) + try: + fd = _hold(staging, create=True) + except OSError: + shutil.rmtree(staging, ignore_errors=True) + return None # no locks on this filesystem: use the disk + held = root / (HELD_PREFIX + staging.name[len(STAGING_PREFIX) :]) + os.rename(staging, held) + self._owner_lock = fd + return held diff --git a/tests/test_browser_fast_reset.py b/tests/test_browser_fast_reset.py index 6be9a54a..c23fae43 100644 --- a/tests/test_browser_fast_reset.py +++ b/tests/test_browser_fast_reset.py @@ -22,6 +22,9 @@ from database.operations import PromptDatabase from tests.browser import fast_reset from tests.browser.fast_reset import ScratchSeeder, table_digests +# The owner lock of /dev/shm scratch folders needs POSIX flock (#419). +HAS_FLOCK = unittest.skipUnless(os.name == "posix", "needs POSIX flock") + def digests(db: PromptDatabase) -> dict: return table_digests(db.model.get_connection()) @@ -275,6 +278,7 @@ class TestScratchSeeder(unittest.TestCase): self.assertEqual(digests(self.live), expected) self.assertFalse((self.root / "unused").exists()) + @HAS_FLOCK def test_the_scratch_lives_under_the_memory_root_when_there_is_one(self): memory = self.root / "memory" memory.mkdir() @@ -288,6 +292,16 @@ class TestScratchSeeder(unittest.TestCase): seeder.close() self.assertEqual(list(memory.iterdir()), []) + @unittest.skipIf(os.name == "posix", "without flock the memory root is unused") + def test_without_flock_the_scratch_stays_on_disk(self): + memory = self.root / "memory" + memory.mkdir() + seeder = ScratchSeeder(self.api, self.root / "fallback", memory_root=memory) + self.addCleanup(seeder.close) + seeder.run(reseed) + self.assertEqual(list(memory.iterdir()), []) + self.assertTrue((self.root / "fallback" / "seed.db").exists()) + def test_a_full_memory_root_puts_the_scratch_on_disk(self): memory = self.root / "memory" memory.mkdir() @@ -303,26 +317,32 @@ class TestScratchSeeder(unittest.TestCase): self.assertEqual(list(memory.iterdir()), []) self.assertTrue((self.root / "fallback" / "seed.db").exists()) - @unittest.skipUnless(os.name == "posix", "process probe is POSIX only") - def test_scratch_folders_of_dead_servers_are_removed(self): + @HAS_FLOCK + def test_the_scratch_folder_is_held_by_a_lock_until_close(self): memory = self.root / "memory" memory.mkdir() - dead = subprocess.Popen([sys.executable, "-c", "pass"]) - dead.wait() - orphan = memory / f"pm-reset-{dead.pid}-abc" - alive = memory / f"pm-reset-{os.getpid()}-def" - other = memory / "pm-reset-notapid" - for folder in (orphan, alive, other): - folder.mkdir() - (folder / "seed.db").write_bytes(b"x") seeder = ScratchSeeder(self.api, self.root / "fallback", memory_root=memory) self.addCleanup(seeder.close) seeder.run(reseed) - self.assertFalse(orphan.exists()) - self.assertTrue(alive.exists()) - self.assertTrue(other.exists()) - own = [p for p in memory.glob(f"pm-reset-{os.getpid()}-*") if p != alive] - self.assertEqual(len(own), 1) + (folder,) = memory.iterdir() + self.assertTrue(folder.name.startswith(f"pm-reset-held-{os.getpid()}-")) + self.assertFalse(lock_is_free(folder / "owner.lock")) + seeder.close() + self.assertEqual(list(memory.iterdir()), []) + + @HAS_FLOCK + def test_a_lock_that_cannot_be_taken_puts_the_scratch_on_disk(self): + memory = self.root / "memory" + memory.mkdir() + seeder = ScratchSeeder(self.api, self.root / "fallback", memory_root=memory) + self.addCleanup(seeder.close) + refuse = mock.patch.object( + fast_reset.fcntl, "flock", side_effect=OSError("no locks here") + ) + with refuse: + seeder.run(reseed) + self.assertEqual(list(memory.iterdir()), []) + self.assertTrue((self.root / "fallback" / "seed.db").exists()) def test_close_releases_the_scratch_files(self): self.seeder.run(reseed) @@ -331,5 +351,95 @@ class TestScratchSeeder(unittest.TestCase): self.seeder.close() # idempotent +# A process that takes the owner lock of a folder, says so, and waits. +HOLDER = """ +import fcntl, os, sys, time +fd = os.open(os.path.join(sys.argv[1], "owner.lock"), os.O_RDWR | os.O_CREAT) +fcntl.flock(fd, fcntl.LOCK_EX) +print("held", flush=True) +time.sleep(600) +""" + + +def lock_is_free(path: Path) -> bool: + fd = os.open(path, os.O_RDWR) + try: + fast_reset.fcntl.flock(fd, fast_reset.fcntl.LOCK_EX | fast_reset.fcntl.LOCK_NB) + except BlockingIOError: + return False + finally: + os.close(fd) + return True + + +@HAS_FLOCK +class TestOrphanedScratch(unittest.TestCase): + """Only folders whose owner lock is free are orphans (#419).""" + + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.addCleanup(self.tmp.cleanup) + self.memory = Path(self.tmp.name) + + def folder(self, name: str) -> Path: + folder = self.memory / name + folder.mkdir() + (folder / "seed.db").write_bytes(b"x") + return folder + + def hold(self, folder: Path) -> subprocess.Popen: + holder = subprocess.Popen( + [sys.executable, "-c", HOLDER, str(folder)], + stdout=subprocess.PIPE, + text=True, + ) + self.addCleanup(holder.wait) + self.addCleanup(holder.stdout.close) + self.addCleanup(holder.kill) + self.assertEqual(holder.stdout.readline().strip(), "held") + return holder + + def test_a_folder_whose_owner_was_killed_is_removed(self): + folder = self.folder("pm-reset-held-123-abc") + holder = self.hold(folder) + holder.kill() + holder.wait() + fast_reset.remove_orphaned_scratch(self.memory) + self.assertFalse(folder.exists()) + + def test_a_held_folder_is_kept_whatever_pid_its_name_carries(self): + # Under --ipc=host the owner's pid is from another PID namespace: it + # does not exist here, but its lock does. + dead = subprocess.Popen([sys.executable, "-c", "pass"]) + dead.wait() + folder = self.folder(f"pm-reset-held-{dead.pid}-abc") + self.hold(folder) + fast_reset.remove_orphaned_scratch(self.memory) + self.assertTrue(folder.exists()) + self.assertTrue((folder / "seed.db").exists()) + + def test_folders_without_an_owner_lock_are_kept(self): + dead = subprocess.Popen([sys.executable, "-c", "pass"]) + dead.wait() + legacy = self.folder(f"pm-reset-{dead.pid}-abc") + staging = self.folder(f"pm-reset-new-{dead.pid}-def") + unlocked = self.folder(f"pm-reset-held-{dead.pid}-ghi") + fast_reset.remove_orphaned_scratch(self.memory) + for folder in (legacy, staging, unlocked): + self.assertTrue(folder.exists(), folder.name) + + def test_a_folder_held_by_another_seeder_in_this_process_is_kept(self): + api = types.SimpleNamespace(db=PromptDatabase(str(self.memory / "live.db"))) + self.addCleanup(api.db.close_all) + root = self.memory / "shm" + root.mkdir() + other = ScratchSeeder(api, self.memory / "fallback", memory_root=root) + self.addCleanup(other.close) + other.run(reseed) + (folder,) = root.iterdir() + fast_reset.remove_orphaned_scratch(root) + self.assertTrue(folder.exists()) + + if __name__ == "__main__": unittest.main() From 9eb5e9453d4702c2f9905c7f94f1f36aa5f0b871 Mon Sep 17 00:00:00 2001 From: Vito Date: Wed, 28 Oct 2026 23:19:54 +0000 Subject: [PATCH 2/3] docs(redesign): record Copy settings summary and Local Inspect PR L3 as shipped (#436) (#440) * docs(redesign): record Copy settings summary and Local Inspect PR L3 as shipped (#436) * docs(redesign): cite D117 and fix the stale Task 9 tense in local-resolver.mjs (#436) * docs(inspect): local-resolver.mjs header says Task 9 shipped (#436) --- docs/application-redesign.md | 2 +- web/js/workspace/local-resolver.mjs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/application-redesign.md b/docs/application-redesign.md index 7df56194..536464d0 100644 --- a/docs/application-redesign.md +++ b/docs/application-redesign.md @@ -12,7 +12,7 @@ code. This plan proposes implementation order and acceptance gates. - The native Prompt Library merged in [#210](https://github.com/ComfyAssets/ComfyUI_PromptManager/pull/210) for [#208](https://github.com/ComfyAssets/ComfyUI_PromptManager/issues/208): one positive parent, saved negative variants, full selected text and a shared filtered image gallery. It does not invent chronological runs from counters or scan dates. - [Durable history storage](prompt-history-storage.md) records job attempts and outcomes from [#212](https://github.com/ComfyAssets/ComfyUI_PromptManager/pull/212), frozen prompt contexts from [#213](https://github.com/ComfyAssets/ComfyUI_PromptManager/pull/213), and observed run-owned output locators from [#214](https://github.com/ComfyAssets/ComfyUI_PromptManager/pull/214). Those locators do not verify file bytes or establish gallery-image associations. That association contract and the chronological execution timeline remain pending under [#209](https://github.com/ComfyAssets/ComfyUI_PromptManager/issues/209). - [#215](https://github.com/ComfyAssets/ComfyUI_PromptManager/issues/215) brings the shell, Images and natural-height Prompt Library presentation into the approved geometry and themes while retaining their existing operations. [#216](https://github.com/ComfyAssets/ComfyUI_PromptManager/issues/216) adds the shared current-file inspector with exact generation facts, scoped provenance, raw/download evidence and loaded-collection viewer controls, including a slideshow that pauses at native video. This delivery does not establish whole-application parity, change the default entry point or constitute a release. Recorded chronological run/context browsing continues under [#209](https://github.com/ComfyAssets/ComfyUI_PromptManager/issues/209); image tagging/review, System consolidation and remaining organization capabilities continue under #196. -- Local Inspect (#249, PRs L1–L2) replaces the `#/inspect` handoff with the concept's native "Inspect a file" page: one local image or video previews inline and opens the shared viewer and full inspector on a record read entirely in the browser, with nothing uploaded, imported or stored. The legacy metadata page (`web/metadata.html`, served at `/prompt_manager/gallery.html`) stays reachable from Overview; its capabilities map to the shared inspector, except its one-click COPY ALL summary, whose ruled replacement ("Copy settings summary") is pending. Recorded deviations D60–D66 are in [TESTING.md](../TESTING.md). Local roles equal to the server's resolver follow in PR L3. +- Local Inspect (#249, PRs L1–L3) replaces the `#/inspect` handoff with the concept's native "Inspect a file" page: one local image or video previews inline and opens the shared viewer and full inspector on a record read entirely in the browser, with nothing uploaded, imported or stored. The legacy metadata page (`web/metadata.html`, served at `/prompt_manager/gallery.html`) stays reachable from Overview; its capabilities map to the shared inspector, including its one-click COPY ALL summary, which is the shared inspector's "Copy settings summary" (D117, LI-4). PR L3 ([#286](https://github.com/ComfyAssets/ComfyUI_PromptManager/pull/286)) shipped that button and local roles equal to the server's resolver (`web/js/workspace/local-resolver.mjs`, TESTING.md "Local roles (#249 PR L3, decision LI-1)"). Recorded deviations D60–D66 and D117 are in [TESTING.md](../TESTING.md). ## Scope and compatibility diff --git a/web/js/workspace/local-resolver.mjs b/web/js/workspace/local-resolver.mjs index b246adaa..435b1a2a 100644 --- a/web/js/workspace/local-resolver.mjs +++ b/web/js/workspace/local-resolver.mjs @@ -4,7 +4,7 @@ // tests/test_local_inspect_parity.py compares every answer with the server's on the same records. Values follow // Python's model (py-json.mjs): dict order, int versus float, code-point lengths, `str.strip()`. // -// Task 8 ported execution evidence and graph tracing for one sampler; Task 9 adds candidate selection among several +// Task 8 ported execution evidence and graph tracing for one sampler; Task 9 added candidate selection among several // samplers and detailer stages (local-resolver-stages.mjs) and A1111 `parameters` (local-parameters.mjs). No DOM. import { PyValueError, isDict, isList, isInt, pyJsonLoads, pyLen, pySlice, pyStrip, pyStr, pyGet } from './py-json.mjs'; import { From 827de84f405488b2d3b66e7ed769e26f25e9a727 Mon Sep 17 00:00:00 2001 From: Vito Date: Wed, 28 Oct 2026 23:31:53 +0000 Subject: [PATCH 3/3] fix(workspace): keep an open dialog when a restore it saw finishes (#415) (#441) When the page's Activity store saw a restore run (its 15 s job-list poll landed inside it), the S9 reload on finish re-rendered the open page and closed any open dialog, dropping what was typed without notice. Whether that happened depended on the poll's timing. Now the S9 reload waits while any dialog is open and runs once the last one closes (createRestoreReload, a MutationObserver on dialog[open]), so the dialog keeps its entries and its next write meets the existing restored-storage refusal with Reload, as when the page never saw the restore. An open record-changing review is the exception: its staged plan no longer describes storage, so it closes as dismissed, as before (closeReviewDialog). Tests: followRestore makes the page see a restore run and finish without timers; browser cases for Create tag, New prompt and the restore review; unit tests for the deferral and closeReviewDialog. --- TESTING.md | 6 +- tests/browser/unseen-restore.mjs | 40 ++++++++++++ .../browser/workspace-library-editor.spec.mjs | 33 +++++++++- .../browser/workspace-system-backups.spec.mjs | 19 ++++++ tests/browser/workspace-vocabulary.spec.mjs | 27 +++++++- tests/js/workspace-system-backups.test.js | 65 +++++++++++++++++++ tests/js/workspace-toolbar.test.js | 28 ++++++++ web/js/workspace/index.mjs | 13 +++- web/js/workspace/system-backups.mjs | 37 +++++++++++ web/js/workspace/workspace-dialogs.mjs | 14 +++- 10 files changed, 273 insertions(+), 9 deletions(-) diff --git a/TESTING.md b/TESTING.md index 8155f912..ae294387 100644 --- a/TESTING.md +++ b/TESTING.md @@ -3346,7 +3346,11 @@ the per-section hand-off panel, was retired by S12 (#329): every section is nati mid-response is not discarded by the page: the server's 15-minute expiry removes it. When a restore the page saw running finishes, the cached Images and Library visits are dropped, the sidebar counts and saved views are read again and the open route is shown again (the Activity job view - stays). A refused file toasts "This file can't be restored: .". A + stays). #415: an open review (`openReviewDialog`) then closes as dismissed, since its plan no + longer describes storage; while any other dialog is open, all of that waits until the last + one has closed, so a dialog keeps what was typed and its next write meets the restored-storage + refusal, as when the page never saw the restore. `followRestore` (tests/browser/unseen-restore.mjs) + makes the page see a restore run and finish deterministically. A refused file toasts "This file can't be restored: .". A review value may wrap anywhere (`#modal-body > dl.meta-grid dd`, CSS under D105), so a long file name keeps the concept's two equal columns at phone width. - **D106** (Integrations, S10, Q8): the send bridge always asks for the ComfyUI window diff --git a/tests/browser/unseen-restore.mjs b/tests/browser/unseen-restore.mjs index aabd8962..6d70d881 100644 --- a/tests/browser/unseen-restore.mjs +++ b/tests/browser/unseen-restore.mjs @@ -52,3 +52,43 @@ export async function unseenRestore(page, restoring) { open(); } } + +// A restore the open page does see run and finish (#415), driven by the test instead of the 15 s poll's timing. The +// job is shaped as the server's (py/api/backup_restore.py: kind `restore`, one `errors` group, not cancellable). +const RESTORE_JOB_ID = '415415415415415415415415415415ab'; +const restoreJob = state => ({ id: RESTORE_JOB_ID, kind: 'restore', title: 'Database restore', state, started_at: 1, + finished_at: state === 'running' ? null : 2, progress: 0, status_line: state === 'running' ? '' : 'Restored the test snapshot.', + processed: 0, total: 0, counters: { errors: 0 }, scope: {}, listed: { errors: 0 }, log_length: 0, cancellable: false }); +/** The Activity pill's link while the page follows that restore. */ +export const SEEN_RESTORE_PILL = `#/activity/${RESTORE_JOB_ID}`; +/** The toast the page shows when it sees that restore finish (activity-kinds.mjs `restore.finishToast`). */ +export const SEEN_RESTORE_TOAST = 'Database restore finished · Restored the test snapshot.'; + +/** + * Call before the page's first navigation, after `armUnseenRestores(page)` (a later route wins). The page's first + * job-list read lists a restore job running, so its Activity store follows it; its job stream is refused, so the store + * reads the list again, and every later read is held until `finish()`, which answers them with the job finished and + * resolves once one of those answers reached the page. The real restore the test runs meanwhile goes through the API. + */ +export async function followRestore(page) { + let release; + const released = new Promise(resolve => { release = resolve; }); + let first = true; + await page.route(`${JOBS}/*/events`, route => route.abort().catch(() => {})); + await page.route(JOBS, async route => { + if (first) { + first = false; + return route.fulfill({ json: { success: true, jobs: [restoreJob('running')] } }).catch(() => {}); + } + await released; + // A read the page has aborted since (a newer read supersedes it) can no longer be answered. + return route.fulfill({ json: { success: true, jobs: [restoreJob('finished')] } }).catch(() => {}); + }); + return { + async finish() { + const answered = page.waitForResponse(response => new URL(response.url()).pathname === '/prompt_manager/jobs'); + release(); + await answered; + }, + }; +} diff --git a/tests/browser/workspace-library-editor.spec.mjs b/tests/browser/workspace-library-editor.spec.mjs index 2cc85b25..fa8346bc 100644 --- a/tests/browser/workspace-library-editor.spec.mjs +++ b/tests/browser/workspace-library-editor.spec.mjs @@ -1,7 +1,7 @@ import {writeFile} from 'node:fs/promises'; import {startCoverage,saveCoverage} from '../../tools/vendor/browser-coverage.mjs'; import {test,expect} from './fixtures.mjs'; -import {armUnseenRestores,unseenRestore} from './unseen-restore.mjs'; +import {armUnseenRestores,unseenRestore,followRestore,SEEN_RESTORE_PILL,SEEN_RESTORE_TOAST} from './unseen-restore.mjs'; import {captureCanonicalState} from './history-canonical-comparison.mjs'; import {inventoryOf,mapConcept,diffInventory} from './library-editor-canonical-comparison.mjs'; @@ -454,6 +454,37 @@ async function leaveAndReturn(page,id) { } const laneLabel=(page,text)=>variantsPanel(page).locator('.variant-lane').filter({hasText:text}); +// #415: a restore the page sees finish while New prompt is open keeps the dialog and its entries; the Library is shown +// again from the restored data only once the dialog has closed. +test('a restore seen finishing keeps New prompt open and reloads the Library after it closes',async({page,request})=>{ + const f=page.fixture;const snapshot=await request.get('/__test__/library-editor/snapshot');await expect(snapshot).toBeOK(); + const buffer=await snapshot.body(); + const current=await detail(request,f.positive_id,'active'); + await expect(await request.patch(`/prompt_manager/library/${f.positive_id}`,{data:{expected_epoch:current.library_epoch,if_match:current.positive.edit_token,metadata:{title:'Retitled elsewhere'}}})).toBeOK(); + const restoreRun=await followRestore(page); + await page.goto(`${workspace}#/library`);const card=page.locator(`article[data-prompt-id="${f.positive_id}"]`); + await expect(card).toContainText('Retitled elsewhere'); + await expect(page.locator('#activity-pill')).toHaveAttribute('href',SEEN_RESTORE_PILL); + await headingActions(page).getByRole('button',{name:'+ New prompt',exact:true}).click(); + const dialog=editorDialog(page);await expect(dialog.getByRole('heading',{name:'New prompt',exact:true})).toBeVisible();await ready(dialog); + await field(dialog,'Title').fill('Typed before restore');await field(dialog,'Positive prompt').fill('A prompt typed while storage was restored'); + await expect(await request.post('/prompt_manager/restore',{multipart:{database_file:{name:'editor.db',mimeType:'application/octet-stream',buffer}}})).toBeOK(); + page.writes.length=0; + await restoreRun.finish(); + await expect(toast(page)).toHaveText(SEEN_RESTORE_TOAST); + await expect(dialog).toBeVisible(); + await expect(field(dialog,'Title')).toHaveValue('Typed before restore'); + await expect(field(dialog,'Positive prompt')).toHaveValue('A prompt typed while storage was restored'); + await expect(card).toContainText('Retitled elsewhere'); + await button(dialog,'Save').click(); + await expect(note(dialog)).toHaveText('Prompt storage was restored since this dialog opened. Nothing was saved.'); + await expect(button(dialog,'Save')).toBeDisabled();expect(page.writes).toHaveLength(1); + await button(dialog,'Cancel').click();await expect(dialog).toHaveCount(0); + await expect(card).not.toContainText('Retitled elsewhere');await expect(card).toContainText('Editor portrait'); + await expect(page.locator('#workspace-main h1')).toBeFocused(); + expect(page.writes).toHaveLength(1); +}); + test('library heading offers Export then + New prompt, and New opens the saved prompt history',async({page,request})=>{ await page.goto(`${workspace}#/library`);await expect(page.locator('article[data-prompt-id]').first()).toBeVisible(); const actions=headingActions(page).locator('button'); diff --git a/tests/browser/workspace-system-backups.spec.mjs b/tests/browser/workspace-system-backups.spec.mjs index edc369d1..3a0404d4 100644 --- a/tests/browser/workspace-system-backups.spec.mjs +++ b/tests/browser/workspace-system-backups.spec.mjs @@ -1,6 +1,7 @@ import { startCoverage, saveCoverage } from '../../tools/vendor/browser-coverage.mjs'; import { test, expect } from './fixtures.mjs'; import { seedConcept } from './concept-inventory.mjs'; +import { followRestore, SEEN_RESTORE_PILL, SEEN_RESTORE_TOAST } from './unseen-restore.mjs'; // System › Backup & export (#249 System S9 Task 24) on the S8 backend and the concept-shaped fixture: Back up and its // status line (D103), the restore review (D105) and its job, the exports, Import / inspect and the recovery policy @@ -265,6 +266,24 @@ test('a finished restore shows the open page again from the restored data', asyn expect(await libraryTotal(request)).toBe(restoredTotal); }); +// #415: an open dialog waits out a finished restore, except a review: the plan it staged no longer describes storage, +// so it closes as dismissed (its stage discarded), as before. +test('a restore seen finishing elsewhere closes an open restore review and discards its stage', async ({ page, request }) => { + const bytes = await backupBytes(request); + const restoreRun = await followRestore(page); + await openBackups(page); + await expect(page.locator('#activity-pill')).toHaveAttribute('href', SEEN_RESTORE_PILL); + await chooseBackup(page, bytes); + await expect(dialog(page).locator('#modal-title')).toHaveText('Restore preview'); + expect((await serverJson(request, '/__test__/system/backups')).stages).toBe(1); + const discarded = page.waitForResponse(response => response.url().includes(`${PREVIEW_URL}/`) && response.request().method() === 'DELETE'); + await restoreRun.finish(); + await expect(toast(page)).toHaveText(SEEN_RESTORE_TOAST); + expect((await discarded).status()).toBe(200); + await expect(dialog(page)).toHaveCount(0); + expect((await serverJson(request, '/__test__/system/backups')).stages).toBe(0); +}); + test('invalid file shows the validator message; nothing is staged', async ({ page, request }) => { const before = await serverJson(request, '/__test__/system/digest'); await openBackups(page); diff --git a/tests/browser/workspace-vocabulary.spec.mjs b/tests/browser/workspace-vocabulary.spec.mjs index 3a223501..ef5ab643 100644 --- a/tests/browser/workspace-vocabulary.spec.mjs +++ b/tests/browser/workspace-vocabulary.spec.mjs @@ -1,7 +1,7 @@ import {startCoverage,saveCoverage} from '../../tools/vendor/browser-coverage.mjs'; import {writeFile} from 'node:fs/promises'; import {test,expect} from './fixtures.mjs'; -import {armUnseenRestores,unseenRestore} from './unseen-restore.mjs'; +import {armUnseenRestores,unseenRestore,followRestore,SEEN_RESTORE_PILL,SEEN_RESTORE_TOAST} from './unseen-restore.mjs'; import {captureCanonicalState} from './history-canonical-comparison.mjs'; import {pageInventoryOf,inventoryOf,diffInventory,mapVocabularyConcept,mapVocabularyDialog} from './vocabulary-canonical-comparison.mjs'; @@ -552,6 +552,31 @@ test('conflict and restore states keep the dialog and write nothing',async({page expect(writes.filter(write=>write.url==='/prompt_manager/vocabulary/tags')).toHaveLength(1); }); +// #415: a restore the page sees finish while Create tag is open keeps the dialog and its entries; the page is shown +// again from the restored data only once the dialog has closed. +test('a restore seen finishing keeps an open dialog and reloads the page after it closes',async({page,request})=>{ + const saved=await backup(request); + await expect(await request.put('/prompt_manager/tags/mist',{data:{new_name:'fog'}})).toBeOK(); + const restoreRun=await followRestore(page);const writes=trackWrites(page);await openPage(page); + await expect(page.locator('#activity-pill')).toHaveAttribute('href',SEEN_RESTORE_PILL); + await expect(tagRow(page,'fog')).toHaveCount(1); + const box=await openDialog(page,main(page).getByRole('button',{name:'+ Create tag',exact:true}),'Create tag'); + await field(box,'Tag name').fill('sunrise'); + await expect(await request.post('/prompt_manager/restore',{multipart:{database_file:{name:'vocabulary.db',mimeType:'application/octet-stream',buffer:saved}}})).toBeOK(); + await restoreRun.finish(); + await expect(toast(page)).toHaveText(SEEN_RESTORE_TOAST); + await expect(box).toBeVisible();await expect(field(box,'Tag name')).toHaveValue('sunrise'); + await expect(tagRow(page,'fog')).toHaveCount(1); + await button(box,'Apply').click(); + await expect(note(box)).toHaveText('Prompt storage was restored since this dialog opened. Nothing was changed.'); + expect(await actionNames(box)).toEqual(['Reload','Cancel','Apply']); + expect(tagNames(await snapshot(request))).not.toContain('sunrise'); + await button(box,'Cancel').click();await expect(box).toHaveCount(0); + await expect(tagRow(page,'mist')).toHaveCount(1);await expect(tagRow(page,'fog')).toHaveCount(0); + await expect(main(page).getByRole('heading',{level:1})).toBeFocused(); + expect(writes.filter(write=>write.url==='/prompt_manager/vocabulary/tags')).toHaveLength(1); +}); + test('lost response is outcome-unknown',async({page,request})=>{ const writes=trackWrites(page);await openPage(page); await page.route('**/prompt_manager/vocabulary/tags/*',async route=>{ diff --git a/tests/js/workspace-system-backups.test.js b/tests/js/workspace-system-backups.test.js index 3a1454ae..4a5550fc 100644 --- a/tests/js/workspace-system-backups.test.js +++ b/tests/js/workspace-system-backups.test.js @@ -192,6 +192,71 @@ test('views reload once after a restore this page saw finish; a failed restore c assert.equal(listeners.size, 0); }); +// #415: a fake document whose open dialogs the test sets, and a MutationObserver stand-in it triggers. +function dialogPage() { + const page = { open: 0, observers: new Set() }; + page.doc = { documentElement: {}, querySelector: selector => (selector === 'dialog[open]' && page.open > 0 ? {} : null) }; + page.Observer = class { + constructor(callback) { this.callback = callback; } + observe(target, options) { this.target = target; this.options = options; page.observers.add(this); } + disconnect() { page.observers.delete(this); } + }; + page.mutate = open => { page.open = open; for (const observer of [...page.observers]) observer.callback([]); }; + return page; +} + +test('the restore reload runs at once when no dialog is open', async () => { + const { createRestoreReload } = await backups; + const page = dialogPage(); + let reloads = 0; + const reload = createRestoreReload(() => { reloads += 1; }, { doc: page.doc, Observer: page.Observer }); + reload.request(); + assert.equal(reloads, 1); + assert.equal(page.observers.size, 0, 'nothing waits'); + reload.request(); + assert.equal(reloads, 2); +}); + +test('the restore reload waits for every open dialog to close, then runs once (#415)', async () => { + const { createRestoreReload } = await backups; + const page = dialogPage(); + page.open = 1; + let reloads = 0; + const reload = createRestoreReload(() => { reloads += 1; }, { doc: page.doc, Observer: page.Observer }); + reload.request(); + reload.request(); + assert.equal(reloads, 0, 'an open dialog holds the reload'); + assert.equal(page.observers.size, 1, 'one observer for repeated requests'); + const [observer] = page.observers; + assert.equal(observer.target, page.doc.documentElement); + assert.deepEqual(observer.options, { subtree: true, childList: true, attributes: true, attributeFilter: ['open'] }); + page.mutate(1); + assert.equal(reloads, 0, 'a dialog swapped for another keeps it waiting'); + page.mutate(0); + assert.equal(reloads, 1); + assert.equal(page.observers.size, 0, 'the observer stops once it ran'); + page.mutate(0); + assert.equal(reloads, 1); + page.open = 1; + reload.request(); + page.mutate(0); + assert.equal(reloads, 2, 'a later restore waits again'); +}); + +test('disposing drops a waiting restore reload', async () => { + const { createRestoreReload } = await backups; + const page = dialogPage(); + page.open = 1; + let reloads = 0; + const reload = createRestoreReload(() => { reloads += 1; }, { doc: page.doc, Observer: page.Observer }); + reload.request(); + reload.dispose(); + assert.equal(page.observers.size, 0); + page.mutate(0); + assert.equal(reloads, 0); + reload.dispose(); +}); + test('a downloaded backup the server could not verify says so', async () => { const { downloadDatabaseBackup, BACKUP_DONE, backupUnverified } = await scan; const toasts = []; diff --git a/tests/js/workspace-toolbar.test.js b/tests/js/workspace-toolbar.test.js index 3ee4cc0c..ad4d8641 100644 --- a/tests/js/workspace-toolbar.test.js +++ b/tests/js/workspace-toolbar.test.js @@ -908,6 +908,34 @@ test('openReviewDialog lays out heading, grid, options, sentence, backup box and } }); +// #415: a finished restore ends an open review (its reviewed plan no longer describes storage) as a dismissal, and +// leaves any other dialog, with what was typed in it, open. +test('closeReviewDialog closes only an open review, as dismissed', async () => { + const { openReviewDialog, openInputDialog, closeReviewDialog, closeWorkspaceDialog } = await dialogsModule; + const { createScope } = await coreModule; + const dom = fakeDOM(); + const scope = createScope(); + try { + closeReviewDialog(); + const outcomes = []; + openInputDialog({ scope, title: 'Save view', label: 'Name', value: 'typed', action: 'Save', onApply() {} }); + closeReviewDialog(); + assert.equal(modal(dom).open, true, 'an input dialog stays'); + assert.equal(byId(dom, 'action-input').value, 'typed'); + openReviewDialog({ scope, title: 'Restore preview', grid: [['Integrity', 'Verified']], cancelLabel: 'Cancel', + primary: { label: 'Restore', danger: true }, onStart: () => outcomes.push('start'), onDismiss: () => outcomes.push('dismiss') }); + closeReviewDialog(); + assert.deepEqual(outcomes, ['dismiss']); + assert.equal(modal(dom), undefined, 'the shell is gone'); + closeReviewDialog(); + assert.deepEqual(outcomes, ['dismiss']); + } finally { + scope.dispose(); + closeWorkspaceDialog(); + dom.restore(); + } +}); + test('review dialog backup box keeps the dialog open and toasts the outcome', async () => { const { openReviewDialog, closeWorkspaceDialog, SCAN_BACKUP_COPY } = await dialogsModule; const { downloadDatabaseBackup, BACKUP_DONE, BACKUP_FAILED } = await scanModule; diff --git a/web/js/workspace/index.mjs b/web/js/workspace/index.mjs index 018d181f..81d1b132 100644 --- a/web/js/workspace/index.mjs +++ b/web/js/workspace/index.mjs @@ -9,7 +9,7 @@ import { mountVocabulary } from './vocabulary.mjs'; import { createWorkspaceIcon } from './icons.mjs'; import { bindCanvasLink, canvasHref } from './topbar.mjs'; import { renderHeading, pageHeading } from './heading.mjs'; -import { closeWorkspaceDialog } from './workspace-dialogs.mjs'; +import { closeReviewDialog, closeWorkspaceDialog } from './workspace-dialogs.mjs'; import { createSavedViews, openSaveViewDialog, openEditViewDialog, savedViewHash } from './saved-views.mjs'; import { createCollections, collectionHash, openEditCollectionDialog, missingNote, DELETED_PAGE_TEXT, OPEN_IMAGES_LINK, LIST_FAILED_TEXT } from './collections.mjs'; @@ -20,7 +20,7 @@ import { mountInspect } from './inspect.mjs'; import { mountReview } from './review.mjs'; import { createPromptSender } from './prompt-sender.mjs'; import { mountSystem } from './system.mjs'; -import { onRestoreFinished, onRestoreHandedBack, showRestoreReview } from './system-backups.mjs'; +import { createRestoreReload, onRestoreFinished, onRestoreHandedBack, showRestoreReview } from './system-backups.mjs'; import { mountLibraryBanner } from './library-location.mjs'; import { detectJxlSupport } from './thumbnail-support.mjs'; @@ -184,13 +184,20 @@ function mountWorkspace() { refreshCounts(NAV_COUNT_ROUTES); // S9: a finished restore replaced the library. The cached visits are dropped, the counts and saved views read // again, and the open page is shown again from the restored data; the Activity job view the restore opened stays. - scope.cleanup(onRestoreFinished(activity, () => { + // #415: an open review closes (its plan no longer describes storage); while any other dialog is open, all of it + // waits until the last one has closed, so the dialog keeps what was typed. + const restoreReload = createRestoreReload(() => { forgetImagesVisit(); forgetLibraryVisit(); refreshCounts(NAV_COUNT_ROUTES); savedViews.load(); collections.load(); if (currentKey !== 'activity') navigateTo(window.location.hash); + }); + scope.cleanup(() => restoreReload.dispose()); + scope.cleanup(onRestoreFinished(activity, () => { + closeReviewDialog(); + restoreReload.request(); })); // S9 I1: a restore this page started that could not run yet (other requests kept reading) hands its review back: // it opens again over the page shown now, with Restore (again) and Discard; leaving it any other way discards it. diff --git a/web/js/workspace/system-backups.mjs b/web/js/workspace/system-backups.mjs index 8d5745dc..a2893624 100644 --- a/web/js/workspace/system-backups.mjs +++ b/web/js/workspace/system-backups.mjs @@ -326,6 +326,43 @@ export function onRestoreFinished(activity, onFinished) { }); } +const OPEN_DIALOG = 'dialog[open]'; +const DIALOG_MUTATIONS = Object.freeze({ subtree: true, childList: true, attributes: true, attributeFilter: ['open'] }); + +/** + * #415: the S9 reload after a finished restore never runs under an open dialog, which it would close with whatever + * was typed in it. `request()` runs `reload()` at once when no dialog is open; otherwise it runs once the last open + * dialog has closed (repeated requests meanwhile run it once). The dialog keeps its entries, and its next write is + * refused by the restored storage with its own note and Reload, as when this page never saw the restore. A + * MutationObserver sees every dialog close: removing `open` and removing the node are synchronous, while the dialog + * `close` event is a later task that never bubbles, and every dialog here is removed in the task that closes it. + * `dispose()` drops a waiting reload. Returns `{request, dispose}`. + */ +export function createRestoreReload(reload, { doc = globalThis.document, Observer = globalThis.MutationObserver } = {}) { + let observer = null; + const dialogOpen = () => doc.querySelector(OPEN_DIALOG) !== null; + const stop = () => { + observer?.disconnect(); + observer = null; + }; + return { + request() { + if (observer) return; + if (!dialogOpen()) { + reload(); + return; + } + observer = new Observer(() => { + if (dialogOpen()) return; + stop(); + reload(); + }); + observer.observe(doc.documentElement, DIALOG_MUTATIONS); + }, + dispose: stop, + }; +} + function surface(heading, ...nodes) { const section = el('section', undefined, 'surface'); section.append(heading, ...nodes); diff --git a/web/js/workspace/workspace-dialogs.mjs b/web/js/workspace/workspace-dialogs.mjs index fff32920..0d6d4643 100644 --- a/web/js/workspace/workspace-dialogs.mjs +++ b/web/js/workspace/workspace-dialogs.mjs @@ -112,13 +112,13 @@ function bindScope(scope) { }); } -function show(heading, nodes, { scope, trigger, focusTarget, onClose = null, canClose = null }) { +function show(heading, nodes, { scope, trigger, focusTarget, onClose = null, canClose = null, review = false }) { shell ??= createShell(); if (!shell.dialog.isConnected) document.body.append(shell.dialog); bindScope(scope); // A swapped-out body is gone from the shell, so its owner releases it. const replaced = session?.onClose; - session = { scope, trigger: trigger ?? session?.trigger ?? null, onClose, canClose }; + session = { scope, trigger: trigger ?? session?.trigger ?? null, onClose, canClose, review }; replaced?.(); shell.title.textContent = heading; shell.body.replaceChildren(...nodes); @@ -133,6 +133,14 @@ export function closeWorkspaceDialog() { shell = null; } +/** + * #415: a finished restore ends an open record-changing review (`openReviewDialog`) as a dismissal, since the plan it + * reviewed no longer describes storage and it holds nothing typed. Any other dialog stays open. + */ +export function closeReviewDialog() { + if (session?.review) closeWorkspaceDialog(); +} + function fillSelect(select, key, options) { if (!Array.isArray(options) || options.length === 0 || options.some(pair => !Array.isArray(pair) || typeof pair[0] !== 'string' || typeof pair[1] !== 'string')) { @@ -497,7 +505,7 @@ export function openReviewDialog({ scope, title, heading = null, grid, options = actions.append(cancel, start); nodes.push(actions); // Cancel, ×, Escape, a replacing dialog or the page leaving: everything but the primary is a dismissal. - show(title, nodes, { scope, trigger, onClose: onDismiss ? () => { if (!started) onDismiss(); } : null }); + show(title, nodes, { scope, trigger, onClose: onDismiss ? () => { if (!started) onDismiss(); } : null, review: true }); return { setValue(term, text) { const dd = values.get(term);