From 2f0eaa39c5f8607a6721226f161263ec29ace228 Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Sat, 31 Oct 2026 21:15:38 -0700 Subject: [PATCH] fix(thumbnails): keep a move's own previews and never walk out of a store (#320) A cleanup that waited from an earlier move never names a key folder the new move just filled, so moving back keeps the previews. Removing empty folders walks top-down and never into a link or junction. Any error while copying undoes the copies; once the switch is committed, a journal that cannot be removed no longer undoes it. Hand-edited cleanup entries and marker keys never name a folder outside the stores; removal stops at interpreter exit. The path field carries aria-invalid with its reason, and a review that went stale reviews again. --- py/api/thumbnail_move.py | 39 ++++++-- py/api/thumbnail_move_files.py | 16 +++- py/api/thumbnail_move_plan.py | 2 + .../workspace-thumbnail-location.spec.mjs | 3 +- tests/test_thumbnail_move.py | 94 +++++++++++++++++++ .../workspace/system-thumbnail-location.mjs | 56 +++++++---- web/js/workspace/workspace.css | 1 - 7 files changed, 180 insertions(+), 31 deletions(-) diff --git a/py/api/thumbnail_move.py b/py/api/thumbnail_move.py index afc156c2..412af1c7 100644 --- a/py/api/thumbnail_move.py +++ b/py/api/thumbnail_move.py @@ -148,7 +148,7 @@ class ThumbnailMoveMixin: ] total = len(items) job.update(total=total, processed=0, progress=0) - chunk = self._move_chunk(plan) + chunk = await self._run_in_executor(self._move_chunk, plan) workers = self._move_workers() done = 0 for start in range(0, total, chunk): @@ -173,6 +173,12 @@ class ThumbnailMoveMixin: "The new folder could not be reached or is full; nothing changed.", ) return + except Exception: + # Anything else before the switch: undo, never half a move. + self.logger.exception("Move thumbnails failed while copying") + await self._run_in_executor(self._move_rollback, plan, state) + self._end_move(job, "failed", "The copy failed; nothing changed.") + return for (source, item), (outcome, detail) in zip(batch, results): row = { "path": "/".join(item[0]), @@ -373,7 +379,14 @@ class ThumbnailMoveMixin: for store in old_entry.get("stores") or [] if isinstance(store, str) and os.path.normcase(store) not in filled ] - cleanup.append({**old_entry, "stores": stores}) + folder = old_entry.get("folder") + drop = [ + key + for key in old_entry.get("drop_keys") or [] + if not folder + or os.path.normcase(os.path.join(folder, key)) not in filled + ] + cleanup.append({**old_entry, "stores": stores, "drop_keys": drop}) cleanup.append(entry) if plan.to_folder: value = { @@ -403,8 +416,14 @@ class ThumbnailMoveMixin: preview_store.configure(value, check=False) get_config_store().commit(mutate, expected_revision=None, publish=publish) - preview_store.refresh() - _remove(move_plan.journal_path()) + # The switch is made: what follows never undoes it. + try: + preview_store.refresh() + _remove(move_plan.journal_path()) + except OSError as error: + self.logger.warning( + f"The thumbnail move journal stays until the next start: {error}" + ) def _old_keys_to_drop(self, plan): if not plan.from_folder: @@ -481,8 +500,14 @@ class ThumbnailMoveMixin: def _remove_store(self, store, folder, job): """Remove the previews and temps of one old store; True when done.""" store = Path(store) - if folder and not preview_location.valid_key(store.name): - return True + if folder: + # A key folder directly in the old thumbnail folder, nothing else. + if not preview_location.valid_key(store.name) or os.path.normcase( + str(store.parent) + ) != os.path.normcase(str(folder)): + return True + elif store.name != "thumbnails": + return True # a hand-edited entry never names another folder if not store.exists(): return True if preview_location.is_link_like(store): @@ -498,6 +523,8 @@ class ThumbnailMoveMixin: if files.is_temp_name(name) or files._preview_parts(name): if preview_location.is_link_like(path): continue + if exit_signal.requested(): + return False # exiting: the next start finishes it if files.remove_file(path): removed += 1 else: diff --git a/py/api/thumbnail_move_files.py b/py/api/thumbnail_move_files.py index ca5b147a..4b0821fc 100644 --- a/py/api/thumbnail_move_files.py +++ b/py/api/thumbnail_move_files.py @@ -207,6 +207,12 @@ def _make_folders(base, relative_dir, created): created.append(str(current)) except FileExistsError: pass + except OSError as error: + if error.errno in (errno.ENOSPC, errno.EDQUOT, errno.EROFS): + raise FolderGone(str(error)) from error + raise CopyFailed( + "Its folder could not be made; made again on view" + ) from error if preview_location.is_link_like(current): raise CopyFailed("a link in the destination") @@ -295,12 +301,16 @@ def remove_empty_folders(top, *, keep_top=False): left = [] if not top.is_dir() or preview_location.is_link_like(top): return left - for folder, dirs, _files in os.walk(top, topdown=False): + # Top-down, pruning every link, junction or reparse point, then bottom-up: + # never into a folder outside *top* (Windows junctions included). + folders = [] + for folder, dirs, _files in os.walk(top): folder = Path(folder) + dirs[:] = [d for d in dirs if not preview_location.is_link_like(folder / d)] + folders.append(folder) + for folder in reversed(folders): if folder == top and keep_top: continue - if preview_location.is_link_like(folder): - continue try: names = os.listdir(folder) except OSError: diff --git a/py/api/thumbnail_move_plan.py b/py/api/thumbnail_move_plan.py index 973a057d..afbc1b09 100644 --- a/py/api/thumbnail_move_plan.py +++ b/py/api/thumbnail_move_plan.py @@ -373,6 +373,8 @@ class ThumbnailMovePlanMixin: } found = [] for key, entry in sorted(keys.items()): + if not preview_location.valid_key(key) or not isinstance(entry, dict): + continue # a marker is data: never a path out of the folder if entry.get("install") not in (install, None): continue if entry.get("identity") in listed: diff --git a/tests/browser/workspace-thumbnail-location.spec.mjs b/tests/browser/workspace-thumbnail-location.spec.mjs index a58311cf..9b132eb4 100644 --- a/tests/browser/workspace-thumbnail-location.spec.mjs +++ b/tests/browser/workspace-thumbnail-location.spec.mjs @@ -93,7 +93,8 @@ test('a folder the server refuses is explained under the field', async ({ page } await row.getByRole('button', { name: 'Review move…' }).click(); await expect(row.locator('#location-problem')).toHaveText( 'Enter a full folder path, such as D:\\Thumbnails or /mnt/fast/thumbnails.'); - await expect(row.locator('.location-path')).toHaveAttribute('aria-invalid', 'true'); + await expect(row.locator('#location-path')).toHaveAttribute('aria-invalid', 'true'); + await expect(row.locator('#location-path')).toHaveAttribute('aria-describedby', 'location-problem'); await expect(dialog(page)).toHaveCount(0); await row.locator('#location-path').fill('/somewhere/else'); await expect(row.locator('#location-problem')).toHaveText(''); diff --git a/tests/test_thumbnail_move.py b/tests/test_thumbnail_move.py index 9390a11c..52543f1e 100644 --- a/tests/test_thumbnail_move.py +++ b/tests/test_thumbnail_move.py @@ -412,3 +412,97 @@ class TestFiles(unittest.TestCase): if __name__ == "__main__": unittest.main() + + +class TestReviewFindings(MoveCase): + async def test_moving_back_never_removes_previews_a_waiting_cleanup_named(self): + await self.move({"mode": "folder", "path": str(self.folder)}) + other = Path(self.tmpdir) / "thumbs2" + # The old copies in the first folder cannot go yet (held open). + with patch.object(files, "remove_file", lambda path, **_k: False): + await self.move({"mode": "folder", "path": str(other)}) + self.assertTrue(preview_store.setting().cleanup) + _plan, job = await self.move({"mode": "folder", "path": str(self.folder)}) + self.assertEqual(job.state, "finished", job.log) + self.assertEqual(len(self.key_files()), 2) # the previews moved back stay + + @unittest.skipUnless(hasattr(os, "symlink"), "needs symlinks") + def test_empty_folder_removal_never_walks_through_a_link(self): + outside = Path(self.tmpdir) / "outside" + (outside / "empty").mkdir(parents=True) + (outside / "Thumbs.db").write_bytes(b"x") + store = Path(self.tmpdir) / "store" + store.mkdir() + try: + (store / "link").symlink_to(outside, target_is_directory=True) + except OSError: + self.skipTest("symlinks not permitted") + files.remove_empty_folders(store) + self.assertTrue((outside / "empty").is_dir()) + self.assertTrue((outside / "Thumbs.db").exists()) + + @unittest.skipUnless(os.name == "nt", "junctions are Windows only") + def test_empty_folder_removal_never_walks_through_a_junction(self): + import _winapi + + outside = Path(self.tmpdir) / "outside-j" + (outside / "empty").mkdir(parents=True) + (outside / "Thumbs.db").write_bytes(b"x") + store = Path(self.tmpdir) / "store-j" + store.mkdir() + _winapi.CreateJunction(str(outside), str(store / "junction")) + files.remove_empty_folders(store) + self.assertTrue((outside / "empty").is_dir()) + self.assertTrue((outside / "Thumbs.db").exists()) + + async def test_an_unexpected_error_while_copying_changes_nothing(self): + def broken(*_args, **_kwargs): + raise PermissionError("denied") + + with patch.object(files, "_make_folders", broken): + _plan, job = await self.move({"mode": "folder", "path": str(self.folder)}) + self.assertEqual(job.state, "failed") + self.assertFalse(self.journal.exists()) + self.assertFalse(self.folder.exists()) + self.assertEqual(preview_store.current().mode, "source") + + async def test_a_journal_that_cannot_be_removed_never_undoes_the_switch(self): + from py.api import thumbnail_move + + real = thumbnail_move._remove + + def stuck(path): + if str(path) == str(self.journal): + raise PermissionError("in use") + return real(path) + + with patch.object(thumbnail_move, "_remove", stuck): + _plan, job = await self.move({"mode": "folder", "path": str(self.folder)}) + self.assertEqual(job.state, "finished", job.log) + self.assertEqual(len(self.key_files()), 2) + self.assertEqual(preview_store.current().mode, "folder") + + async def test_a_hostile_key_is_never_pruned(self): + await self.move({"mode": "folder", "path": str(self.folder)}) + victim = Path(self.tmpdir) / "victim" + make_png(victim / "z_thumb.webp", size=(4, 4)) + marker = preview_location.read_marker(str(self.folder)) + marker["keys"]["../victim"] = {"identity": "/gone", "install": marker_install()} + (self.folder / preview_location.MARKER_NAME).write_text( + json.dumps(marker), encoding="utf-8" + ) + preview_store.refresh() + resp = await self.client.post(START, json={"mode": "prune"}) + job = self.api._jobs.get((await resp.json())["job"]["id"]) + await job.task + self.assertTrue((victim / "z_thumb.webp").exists()) + + def test_a_hand_edited_source_store_must_be_a_thumbnails_folder(self): + elsewhere = Path(self.tmpdir) / "photos" + make_png(elsewhere / "keep_thumb.webp", size=(4, 4)) + self.assertTrue(self.api._remove_store(elsewhere, None, None)) + self.assertTrue((elsewhere / "keep_thumb.webp").exists()) + + +def marker_install(): + return preview_store.setting().install diff --git a/web/js/workspace/system-thumbnail-location.mjs b/web/js/workspace/system-thumbnail-location.mjs index 92175b67..d9766f0d 100644 --- a/web/js/workspace/system-thumbnail-location.mjs +++ b/web/js/workspace/system-thumbnail-location.mjs @@ -156,7 +156,8 @@ function radio(value, label, hint) { } /** Start the move with the plan's `token`; opens it in Activity, or says why not. */ -export async function startMove({ store, scope, token, confirmed, notify = showWorkspaceToast, navigate = navigateTo }) { +export async function startMove({ store, scope, token, confirmed, notify = showWorkspaceToast, navigate = navigateTo, + onStale = null }) { try { const result = await store.start('thumbnails', { mode: 'move', token, confirmed }); if (scope.signal.aborted) return null; @@ -167,13 +168,19 @@ export async function startMove({ store, scope, token, confirmed, notify = showW navigate(`#/activity/${result.job.id}`); return result.job; } catch (error) { - if (!scope.signal.aborted) notify(typeof error?.code === 'string' && error.message ? error.message : START_FAILED); + if (scope.signal.aborted) return null; + // The sources or the location changed since the review: review again with what is there now. + if (error?.code === 'plan-stale' && onStale) { + onStale(); + return null; + } + notify(typeof error?.code === 'string' && error.message ? error.message : START_FAILED); return null; } } /** Open the D197 review over `plan`; a plan the server refused for space keeps Move thumbnails unavailable. */ -export function openMoveReview({ scope, store, plan, refusal = null, trigger = null, notify, navigate }) { +export function openMoveReview({ scope, store, plan, refusal = null, trigger = null, notify, navigate, onStale = null }) { const options = []; let box = null; if (plan.confirm_required && !refusal) { @@ -187,7 +194,8 @@ export function openMoveReview({ scope, store, plan, refusal = null, trigger = n if (refusal) options.push(el('p', refusal, 'small review-refusal')); const review = openReviewDialog({ scope, trigger, title: 'Move thumbnails', grid: reviewRows(plan), options, sentence: refusal ? null : SENTENCE, cancelLabel: 'Cancel', primary: { label: 'Move thumbnails' }, - onStart: () => { startMove({ store, scope, token: plan.token, confirmed: Boolean(box?.checked), notify, navigate }); } }); + onStart: () => { startMove({ store, scope, token: plan.token, confirmed: Boolean(box?.checked), notify, navigate, + onStale }); } }); const locked = () => Boolean(refusal) || (box !== null && !box.checked); review.setPrimary({ label: 'Move thumbnails', disabled: locked() }); if (box) scope.listen(box, 'change', () => review.setPrimary({ label: 'Move thumbnails', disabled: locked() })); @@ -235,6 +243,14 @@ export function locationBlock({ api, store, scope, location, notify = showWorksp const setProblem = message => { problem.textContent = message || ''; pathField.setAttribute('aria-invalid', String(Boolean(message))); + // The field itself carries the state, so a screen reader announces it with the reason. + if (message) { + input.setAttribute('aria-invalid', 'true'); + input.setAttribute('aria-describedby', 'location-problem'); + } else { + input.removeAttribute('aria-invalid'); + input.removeAttribute('aria-describedby'); + } }; const current = () => ({ mode: folder.input.checked ? 'folder' : 'source', path: input.value }); const repaintButton = () => { @@ -272,7 +288,9 @@ export function locationBlock({ api, store, scope, location, notify = showWorksp review.setAttribute('aria-disabled', 'true'); try { const plan = await api.post(PLAN_URL, body, { signal: scope.signal }); - if (!scope.signal.aborted) openMoveReview({ scope, store, plan, trigger: review, notify, navigate }); + if (!scope.signal.aborted) { + openMoveReview({ scope, store, plan, trigger: review, notify, navigate, onStale: () => review.click() }); + } } catch (error) { if (scope.signal.aborted) return; if (error?.status === 507 && error?.plan) { @@ -377,20 +395,18 @@ export function noticeSlot({ api, scope, location, onChange, onDone, notify = sh const anchor = document.createComment('thumbnail-location-notice'); let shownFor; let notice = null; - return { - anchor, - paint() { - const now = location(); - // Not mounted yet: the next paint (a microtask, or the host read) places it. - if (!anchor.parentNode) { - queueMicrotask(() => { if (anchor.parentNode && !scope.signal.aborted) this.paint(); }); - return; - } - if (now === shownFor) return; - shownFor = now; - notice?.remove(); - notice = locationNotice({ api, scope, location: now, onChange, onDone, notify }); - if (notice) anchor.after(notice); - }, + const paint = () => { + const now = location(); + // Not mounted yet: the next paint (a microtask, or the next host read) places it. + if (!anchor.parentNode) { + queueMicrotask(() => { if (anchor.parentNode && !scope.signal.aborted) paint(); }); + return; + } + if (now === shownFor) return; + shownFor = now; + notice?.remove(); + notice = locationNotice({ api, scope, location: now, onChange, onDone, notify }); + if (notice) anchor.after(notice); }; + return { anchor, paint }; } diff --git a/web/js/workspace/workspace.css b/web/js/workspace/workspace.css index 756702bf..cf537f4d 100644 --- a/web/js/workspace/workspace.css +++ b/web/js/workspace/workspace.css @@ -600,7 +600,6 @@ pre.prompt-text { font: inherit; font-size: 12px; line-height: 1.75; margin-bloc padding: 10px 12px; border: 1px solid var(--line); border-radius: 6px; background: var(--ca-field); } .review-confirm input { margin-top: 2px; flex-shrink: 0; } .review-refusal { color: var(--red); } -.meta-grid dd { overflow-wrap: anywhere; } @media (max-width: 700px) { .location-choice { flex-direction: column; }