From 2e9322d5c44599d22f00bb6ed4d47853c25a777f Mon Sep 17 00:00:00 2001 From: Vito Date: Sun, 1 Nov 2026 21:16:55 +0000 Subject: [PATCH] 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 | 44 ++++++++++++++++++++++++-------- tests/test_preview_probe.py | 50 +++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 11 deletions(-) diff --git a/py/api/preview_store.py b/py/api/preview_store.py index e3709eac..2e68331d 100644 --- a/py/api/preview_store.py +++ b/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(): diff --git a/tests/test_preview_probe.py b/tests/test_preview_probe.py index 496a8165..b259febb 100644 --- a/tests/test_preview_probe.py +++ b/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()