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

@@ -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:

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:

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:

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('');

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

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 };
 }

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; }