From 30ff4fd2eb28907e36d0e164a962bae680c90bcd Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Sat, 31 Oct 2026 22:39:29 -0700 Subject: [PATCH] fix(thumbnails): lock the folder marker across processes (#320) Review finding P2 on #591: two installs sharing one thumbnail folder could each read the marker, add a key and replace it, so the last writer dropped the other's key. write_marker now re-reads, changes and replaces the marker under a lock file made with O_CREAT | O_EXCL beside it (atomic on shares, unlike flock); a lock older than 30 s, left by a writer that died, is taken over, and a wait past 1.5 s fails as a TimeoutError like an unreachable folder. Tested with three processes adding keys at once. --- py/api/preview_location.py | 71 +++++++++++++++++++++++++++- tests/test_api_thumbnail_location.py | 56 ++++++++++++++++++++++ tests/trash_route_audit.py | 2 + 3 files changed, 127 insertions(+), 2 deletions(-) diff --git a/py/api/preview_location.py b/py/api/preview_location.py index f2cd6337..7e9f8638 100644 --- a/py/api/preview_location.py +++ b/py/api/preview_location.py @@ -29,8 +29,10 @@ import posixpath import re import stat as stat_module import threading +import time import unicodedata import uuid +from contextlib import contextmanager from dataclasses import dataclass, field try: @@ -44,6 +46,11 @@ SOURCE_MODE = "source" FOLDER_MODE = "folder" MARKER_NAME = ".promptmanager-thumbnails.json" MARKER_VERSION = 1 +# Beside the marker while one process re-reads, changes and replaces it. +MARKER_LOCK_NAME = ".promptmanager-thumbnails.lock" +MARKER_LOCK_TIMEOUT = 1.5 # a writer waits this long (under preview_store.IO_BUDGET) +MARKER_LOCK_STALE = 30.0 # a lock this old was left by a writer that died +MARKER_LOCK_POLL = 0.02 # A source key: one folder name, a slug and a 8-16 hex hash (``source_key``). KEY_PATTERN = re.compile(r"^[a-z0-9_-]{1,24}-[0-9a-f]{8,16}$") # A marker is a few keys; anything larger is not ours. @@ -413,10 +420,70 @@ def valid_keys(keys): return kept +@contextmanager +def _marker_lock(folder): + """Hold the folder's marker lock: a file made with ``O_CREAT | O_EXCL`` + (atomic on local disks and SMB/NFS shares, unlike ``flock``), so installs + sharing the folder change the marker one at a time. A lock older than + MARKER_LOCK_STALE was left by a writer that died and is taken over; + waiting longer than MARKER_LOCK_TIMEOUT raises TimeoutError.""" + lock = os.path.join(folder, MARKER_LOCK_NAME) + deadline = time.monotonic() + MARKER_LOCK_TIMEOUT + while True: + try: + handle = os.open(lock, os.O_CREAT | os.O_EXCL | os.O_WRONLY, 0o644) + except FileExistsError: + _break_stale_lock(lock) + if time.monotonic() >= deadline: + raise TimeoutError( + errno.ETIMEDOUT, "the thumbnail folder marker is locked", lock + ) + time.sleep(MARKER_LOCK_POLL) + continue + try: + os.write(handle, f"{os.getpid()}\n".encode("ascii")) + finally: + os.close(handle) # closed before any unlink (Windows) + break + try: + yield + finally: + try: + os.unlink(lock) + except FileNotFoundError: + pass # taken over as stale by another writer: nothing to release + + +def _break_stale_lock(lock): + """Remove *lock* when it is older than MARKER_LOCK_STALE (its writer + died); a lock that changed since it was looked at is left alone.""" + try: + before = os.stat(lock) + except FileNotFoundError: + return + if time.time() - before.st_mtime < MARKER_LOCK_STALE: + return + logger.warning(f"Taking over a stale thumbnail folder lock: {lock}") + try: + after = os.stat(lock) + if (after.st_ino, after.st_mtime_ns) == (before.st_ino, before.st_mtime_ns): + os.unlink(lock) + except FileNotFoundError: + pass # another writer took it over first + + def write_marker(folder, mutate): """Re-read the marker, apply ``mutate(marker)`` and write it atomically - (a temp sibling renamed over it). Callers hold ``preview_store.write_lock``. - Returns the marker written.""" + (a temp sibling renamed over it), all under the folder's marker lock so a + second install sharing the folder never drops this one's change (and + the other way round). Callers hold ``preview_store.write_lock`` (threads + of this process). Returns the marker written; TimeoutError when another + writer holds the lock too long.""" + with _marker_lock(folder): + return _write_marker_locked(folder, mutate) + + +def _write_marker_locked(folder, mutate): current = read_marker(folder) or {} marker = { "version": MARKER_VERSION, diff --git a/tests/test_api_thumbnail_location.py b/tests/test_api_thumbnail_location.py index d1eb2fd8..67470ddf 100644 --- a/tests/test_api_thumbnail_location.py +++ b/tests/test_api_thumbnail_location.py @@ -189,6 +189,62 @@ class TestMarker(unittest.TestCase): # No temp file is left beside it. self.assertEqual(os.listdir(self.folder), [pl.MARKER_NAME]) + def test_two_processes_adding_keys_at_once_keep_every_key(self): + """Installs sharing one folder (bot review, P2): the read-modify-write + of the marker is locked across processes, so no writer drops another's + key. Each child widens the window by pausing inside ``mutate``.""" + import subprocess + + pl.init_folder(self.folder) + child = ( + "import sys, time\n" + "sys.path.insert(0, sys.argv[1])\n" + "from py.api import preview_location as pl\n" + "folder, tag, start = sys.argv[2], sys.argv[3], float(sys.argv[4])\n" + "while time.time() < start: time.sleep(0.001)\n" + "for n in range(8):\n" + " def add(m, n=n):\n" + " time.sleep(0.01)\n" + " m['keys'][f'{tag}-{n:08x}'] = {'install': tag}\n" + " pl.write_marker(folder, add)\n" + ) + import time + + start = str(time.time() + 1.0) + procs = [ + subprocess.Popen( # nosec B603 - this interpreter, fixed arguments + [sys.executable, "-c", child, str(REPO), self.folder, tag, start] + ) + for tag in ("a", "b", "c") + ] + for proc in procs: + self.assertEqual(proc.wait(timeout=60), 0) + keys = pl.read_marker(self.folder)["keys"] + self.assertEqual(len(keys), 24, sorted(keys)) + self.assertEqual(os.listdir(self.folder), [pl.MARKER_NAME]) + + def test_a_lock_left_by_a_crashed_writer_is_taken_over_when_stale(self): + pl.init_folder(self.folder) + lock = os.path.join(self.folder, pl.MARKER_LOCK_NAME) + with open(lock, "w", encoding="utf-8") as handle: + handle.write("crashed") + old = pl.time.time() - pl.MARKER_LOCK_STALE - 5 + os.utime(lock, (old, old)) + pl.write_marker(self.folder, lambda m: m["keys"].update({"a-0123abcd": {}})) + self.assertIn("a-0123abcd", pl.read_marker(self.folder)["keys"]) + self.assertFalse(os.path.exists(lock)) + + def test_a_held_lock_times_out_without_writing(self): + pl.init_folder(self.folder) + lock = os.path.join(self.folder, pl.MARKER_LOCK_NAME) + with open(lock, "w", encoding="utf-8") as handle: + handle.write("held") + with patch.object(pl, "MARKER_LOCK_TIMEOUT", 0.05): + with self.assertRaises(TimeoutError): + pl.write_marker(self.folder, lambda m: m["keys"].update({"x": {}})) + self.assertTrue(os.path.exists(lock)) # never taken from its holder + self.assertEqual(pl.read_marker(self.folder)["keys"], {}) + def test_a_folder_without_the_marker_is_missing(self): self.assertEqual(pl.folder_state(self.folder, "x")[:2], (False, "missing")) missing = os.path.join(self.folder, "gone") diff --git a/tests/trash_route_audit.py b/tests/trash_route_audit.py index c8fa208e..6eb30553 100644 --- a/tests/trash_route_audit.py +++ b/tests/trash_route_audit.py @@ -15,6 +15,8 @@ KNOWN_FILE_READERS = frozenset( "images.py:os.open(", "__init__.py:Image.open(", "logging_routes.py:os.open(", + # The thumbnail folder's marker lock file (#320), never a media file. + "preview_location.py:os.open(", "thumbnail_auto.py:FileResponse(", "thumbnail_encode.py:Image.open(", }