fix(thumbnails): never let an older folder check overwrite a newer answer (#611) (#612)

Two refresh() callers could publish out of order. A check that looked
before the folder was removed could set "available" after a newer check
said "missing", and the state stayed wrong until the next probe. Each
check now has an order number, and an older answer is dropped.

py/api/preview_store.py

@@ -69,6 +69,8 @@ _state_lock = threading.Lock()
 _parsed = preview_location.ParsedLocation()
 _available = True
 _problem = None
+# The newest check whose answer was published.
+_published_order = 0
 _keys = {}
 _key_cache = {}
 _clock = time.monotonic
@@ -167,10 +169,13 @@ def refresh():
     if parsed.mode != FOLDER_MODE or parsed.problem is not None:
         return parsed.mode == SOURCE_MODE
     try:
-        available, problem, marker = _probe.check(parsed.path, parsed.id)
+        (available, problem, marker), order = _probe.ordered_check(
+            parsed.path, parsed.id
+        )
     except (OSError, TimeoutError, RuntimeError):
         available, problem, marker = False, "unreachable", None
-    _set_state(parsed, available, problem, marker)
+        order = _probe.latest_order()
+    _set_state(parsed, available, problem, marker, order)
     return available
 
 
@@ -207,11 +212,16 @@ def mark_invalid():
         _available, _problem = False, "invalid"
 
 
-def _set_state(parsed, available, problem, marker):
-    global _available, _problem
+def _set_state(parsed, available, problem, marker, order):
+    """Publish a check's answer, unless a check that started later already
+    published (two callers can finish out of order, #611)."""
+    global _available, _problem, _published_order
     with _state_lock:
         if _parsed is not parsed:
             return  # the setting changed meanwhile
+        if order < _published_order:
+            return
+        _published_order = order
         _available, _problem = available, problem
         keys = dict((marker or {}).get("keys") or {})
         if keys != _keys:
@@ -261,14 +271,25 @@ class _Probe:
         self._stop = threading.Event()
         self._thread = None
         self._checking = None
+        self._order = 0
 
     def check(self, path, expected_id):
-        """``folder_state`` within IO_BUDGET; raises TimeoutError.
+        """``folder_state`` within IO_BUDGET; see :meth:`ordered_check`."""
+        return self.ordered_check(path, expected_id)[0]
 
-        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."""
+    def latest_order(self):
+        """The order number of the check started last."""
+        with self._lock:
+            return self._order
+
+    def ordered_check(self, path, expected_id):
+        """``(folder_state, order)`` within IO_BUDGET; raises TimeoutError.
+
+        ``order`` grows with each check started, so answers can be published
+        in the order they looked. 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:
             earlier = self._running(key)
@@ -284,7 +305,7 @@ class _Probe:
                 self._checking = None
         if "error" in pending[2]:
             raise pending[2]["error"]
-        return pending[2]["value"]
+        return pending[2]["value"], pending[2]["order"]
 
     def _running(self, key):
         """The check of *key* still running (lock held), else None."""
@@ -300,7 +321,8 @@ class _Probe:
 
     def _start_check(self, path, expected_id):
         """Start one check on its own thread (lock held)."""
-        result = {}
+        self._order += 1
+        result = {"order": self._order}
         done = threading.Event()
 
         def run():

tests/test_preview_probe.py

@@ -89,5 +89,55 @@ class TestProbeCheck(unittest.TestCase):
                 self.probe.check(*KEY)
 
 
+class TestRefreshOrder(unittest.TestCase):
+    """Only the newest check's answer is published (#611)."""
+
+    def test_an_older_check_never_overwrites_a_newer_answer(self):
+        parsed = preview_location.ParsedLocation(
+            mode=preview_store.FOLDER_MODE, path="/thumbs", id="x"
+        )
+        self.addCleanup(setattr, preview_store, "_available", preview_store._available)
+        self.addCleanup(setattr, preview_store, "_problem", preview_store._problem)
+        started = threading.Event()
+        release = threading.Event()
+        newer_published = threading.Event()
+        calls = []
+        publish = preview_store._set_state
+
+        def state(_path, _expected_id):
+            calls.append(1)
+            if len(calls) == 1:
+                started.set()
+                self.assertTrue(release.wait(WAIT))
+                return HERE
+            return GONE
+
+        def set_state(*args, **kwargs):
+            if args[1]:  # the older check's "available" arrives last
+                self.assertTrue(newer_published.wait(WAIT))
+            publish(*args, **kwargs)
+
+        def newer():
+            preview_store.refresh()
+            newer_published.set()
+
+        with (
+            patch.object(preview_store, "_parsed", parsed),
+            patch.object(preview_location, "folder_state", state),
+            patch.object(preview_store, "_set_state", set_state),
+        ):
+            first = threading.Thread(target=preview_store.refresh)
+            first.start()
+            self.assertTrue(started.wait(WAIT))
+            second = threading.Thread(target=newer)
+            second.start()
+            release.set()
+            second.join(WAIT)
+            first.join(WAIT)
+            self.assertFalse(first.is_alive() or second.is_alive())
+            self.assertFalse(preview_store._available)
+            self.assertEqual(preview_store._problem, "missing")
+
+
 if __name__ == "__main__":
     unittest.main()