fix(thumbnails): a folder check answers for the folder as it is now (#320)

Probe.check() joined a check already running, or a finished one nobody had
collected. A folder deleted right after configure() woke the probe therefore
stayed available until the next probe, which failed the Overview notice spec in
the merge queue. It now waits for a running check and starts a new one, still
never two at once, and a stuck check still times out. The spec also asserts the
fixture's removal took effect.

py/api/preview_store.py

@@ -263,22 +263,40 @@ class _Probe:
         self._checking = None
 
     def check(self, path, expected_id):
-        """``folder_state`` within IO_BUDGET; raises TimeoutError. A check
-        already running (the probe's, or a stuck one) is joined, never run
-        twice at once."""
+        """``folder_state`` within IO_BUDGET; raises TimeoutError.
+
+        The answer comes from a check that started after this call: one
+        already running may have looked before what the caller just did, so
+        it is waited for (never run twice at once; a stuck one times out here)
+        and followed by a new one."""
+        key = (path, expected_id)
         with self._lock:
-            pending = self._checking
-            if pending is None or pending[0] != (path, expected_id):
-                pending = self._start_check(path, expected_id)
-        key, done, result = pending
-        if not done.wait(IO_BUDGET):
-            raise TimeoutError("the thumbnail folder did not answer in time")
+            earlier = self._running(key)
+            pending = None if earlier else self._start_check(path, expected_id)
+        if earlier:
+            self._wait(earlier)
+            with self._lock:
+                # One another caller started meanwhile is new enough to join.
+                pending = self._running(key) or self._start_check(path, expected_id)
+        self._wait(pending)
         with self._lock:
             if self._checking is pending:
                 self._checking = None
-        if "error" in result:
-            raise result["error"]
-        return result["value"]
+        if "error" in pending[2]:
+            raise pending[2]["error"]
+        return pending[2]["value"]
+
+    def _running(self, key):
+        """The check of *key* still running (lock held), else None."""
+        pending = self._checking
+        if pending is None or pending[0] != key or pending[1].is_set():
+            return None
+        return pending
+
+    @staticmethod
+    def _wait(pending):
+        if not pending[1].wait(IO_BUDGET):
+            raise TimeoutError("the thumbnail folder did not answer in time")
 
     def _start_check(self, path, expected_id):
         """Start one check on its own thread (lock held)."""

tests/browser/workspace-thumbnail-location.spec.mjs

@@ -150,7 +150,8 @@ test('another folder’s marker offers Use this folder', async ({ page, request
 
 test('Overview shows the notice and Change location opens the row', async ({ page, request }) => {
     await expect(await request.post('/__test__/thumbnail-location', { data: { mode: 'folder' } })).toBeOK();
-    await request.post('/__test__/thumbnail-folder/remove');
+    const removed = await request.post('/__test__/thumbnail-folder/remove');
+    expect((await removed.json()).available).toBe(false);
     await page.goto(`${workspace}#/system/overview`);
     const notice = page.locator('#unavailable-notice');
     await expect(notice.locator('h2')).toHaveText('The thumbnail folder is unavailable');

tests/test_preview_probe.py

@@ -0,0 +1,93 @@
+"""The thumbnail folder check answers for the folder as it is now (#320).
+
+A check that started before the caller asked can miss what just happened: #592
+hit it in the merge queue, where a folder deleted right after
+``configure`` woke the probe stayed "available" until the next probe.
+"""
+
+import sys
+import threading
+import unittest
+from pathlib import Path
+from unittest.mock import patch
+
+sys.path.insert(0, str(Path(__file__).resolve().parents[1]))
+
+from py.api import preview_location, preview_store  # noqa: E402
+
+WAIT = 5
+HERE = (True, None, {"id": "x"})
+GONE = (False, "missing", None)
+KEY = ("/thumbs", "x")
+
+
+class TestProbeCheck(unittest.TestCase):
+    def setUp(self):
+        self.probe = preview_store._Probe()
+
+    def test_a_finished_check_nobody_collected_is_not_the_answer(self):
+        with patch.object(preview_location, "folder_state", return_value=HERE):
+            with self.probe._lock:
+                _key, done, _result = self.probe._start_check(*KEY)
+            self.assertTrue(done.wait(WAIT))
+        with patch.object(preview_location, "folder_state", return_value=GONE):
+            self.assertEqual(self.probe.check(*KEY), GONE)
+
+    def test_a_check_already_running_is_followed_by_a_new_one(self):
+        started = threading.Event()
+        release = threading.Event()
+        calls = []
+
+        def state(_path, _expected_id):
+            calls.append(threading.current_thread().name)
+            if len(calls) == 1:
+                started.set()
+                self.assertTrue(release.wait(WAIT))
+                return HERE
+            return GONE
+
+        answers = []
+        with patch.object(preview_location, "folder_state", state):
+            with self.probe._lock:
+                self.probe._start_check(*KEY)
+            self.assertTrue(started.wait(WAIT))
+            caller = threading.Thread(
+                target=lambda: answers.append(self.probe.check(*KEY))
+            )
+            caller.start()
+            release.set()
+            caller.join(WAIT)
+        self.assertFalse(caller.is_alive())
+        self.assertEqual(answers, [GONE])
+        self.assertEqual(len(calls), 2)  # one after the other, never two at once
+
+    def test_a_stuck_check_is_never_doubled(self):
+        release = threading.Event()
+        self.addCleanup(release.set)
+        calls = []
+
+        def state(_path, _expected_id):
+            calls.append(1)
+            release.wait(WAIT)
+            return HERE
+
+        with (
+            patch.object(preview_location, "folder_state", state),
+            patch.object(preview_store, "IO_BUDGET", 0.05),
+        ):
+            with self.probe._lock:
+                self.probe._start_check(*KEY)
+            with self.assertRaises(TimeoutError):
+                self.probe.check(*KEY)
+        self.assertEqual(len(calls), 1)
+
+    def test_an_error_from_the_new_check_reaches_the_caller(self):
+        with patch.object(
+            preview_location, "folder_state", side_effect=OSError("share gone")
+        ):
+            with self.assertRaises(OSError):
+                self.probe.check(*KEY)
+
+
+if __name__ == "__main__":
+    unittest.main()