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

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

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")

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(",
     }