fix(annotations): compare capture identity like with like and keep paths out of errors

Native Windows CI (Python 3.12/3.13) rejected untouched files with
CaptureChanged because the path stat and the descriptor fstat disagree
there (change time versus creation time, possibly dev/ino encodings).
The descriptor is now checked against the path only on the API-stable
facts (size, mtime_ns); the full identity (size, mtime_ns, ctime_ns,
dev/ino where meaningful) is compared between the fstat before and
after the copy and between the path stats before and after. A test
injects shifted fstat results to simulate the Windows discrepancy, and
a fresh file is captured twice with no platform guard.

Error messages carry strerror only, never the path (the path goes to
the debug log); a test asserts no CaptureError names the root. Files
are unlinked outside the store lock; close() during an in-flight
capture lets that worker remove its temp file and then the directory.
get() documents that readers of capture.path must acquire/release_use.
The test fixture resolves its temp root (macOS /private/var, Windows
8.3 names). TESTING.md states the real identity contract.

TESTING.md

@@ -1372,11 +1372,14 @@ browser fixtures clear `media_image_tags`, `media_annotation_bytes` and
 Capture store (#249, `utils/media_capture.py`, `tests/test_media_capture.py`):
 one explicit action captures one rooted media file into an owner-only
 `prompt-manager-captures-*` temp directory, streaming it in 1 MiB chunks while
-hashing, with the opened descriptor compared to the path (`dev`, `ino` where
-meaningful, `size`, `mtime_ns`) before and after the copy and `ctime_ns`
-compared between the two `fstat` reads (Python 3.12+ on Windows reports the
-change time from `stat` but the creation time from `fstat`); any disagreement
-is `CaptureChanged` and the copy is removed. Aliases (a symlinked
+hashing. Identity is compared like with like: the opened descriptor against
+the path only on the API-stable facts (`size`, `mtime_ns`); the full identity
+(`size`, `mtime_ns`, `ctime_ns`, `dev`/`ino` where meaningful) between the
+`fstat` before and after the copy and between the path stats before and after
+(native Windows CI with Python 3.12/3.13 showed `stat` and `fstat` disagreeing
+on `st_ctime_ns`: change time versus creation time; 3.10/3.11 agree). Any
+disagreement is `CaptureChanged` and the copy is removed. Error messages carry
+`strerror` only, never a path. Aliases (a symlinked
 file or directory, even inside the root), escapes, non-media names and
 directories are `CaptureDenied`; the locator is
 `normcase(join(realpath(root), relative))`, so a symlinked root shares the
@@ -1387,10 +1390,13 @@ tested with an injected clock. An acquired capture outlives expiry and release
 until its last use ends. Header-only dimensions (4096 px per side,
 16,777,216 px) and video mark Generate unsupported; `verify` re-hashes the
 current file without a copy. Every handle closes before unlink (a patched
-`os.unlink` asserts it, plus `assert_closed`). Known limit: Windows exposes no
-change time through `stat`, so a same-size rewrite that restores `mtime_ns`
-during the copy window completes there with a digest of the copied bytes; POSIX
-detects it through `ctime`.
+`os.unlink` asserts it, plus `assert_closed`), and files are unlinked outside
+the store lock. Known limit: Windows reports the creation time from `fstat`, so
+a same-size rewrite that restores `mtime_ns` during the copy window completes
+there with a digest of the copied bytes; POSIX detects it through `ctime`.
+Vocabulary rename, merge and delete bump the `tag_revision` of every image set
+they change (and `image_index_revision`), so a Tag dialog opened before the
+change gets `tag-conflict` on Save (`tests/test_pair_tag_maintenance.py`).
 
 Budgets: `test_filter_work_is_bounded` always runs. It asserts that a plain first
 page makes 1 link SELECT and a plain cursor page none, that every filter shape

tests/test_media_capture.py

@@ -75,7 +75,8 @@ class CaptureStoreTestCase(unittest.TestCase):
         parent = tempfile.TemporaryDirectory()
         self.addCleanup(parent.cleanup)
         self.addCleanup(assert_closed, self, parent.name)
-        self.parent = Path(parent.name)
+        # The resolved path: macOS /var -> /private/var, Windows 8.3 names.
+        self.parent = Path(os.path.realpath(parent.name))
         self.root = self.parent / "output"
         self.root.mkdir()
         self.clock = FakeClock()
@@ -512,6 +513,8 @@ class CaptureStoreTestCase(unittest.TestCase):
 
         def checked_unlink(path, *args, **kwargs):
             self.assertEqual(open_handles_under(path), [])
+            self.assertTrue(store._lock.acquire(blocking=False))  # no I/O under lock
+            store._lock.release()
             unlinked.append(os.fspath(path))
             return original_unlink(path, *args, **kwargs)
 
@@ -705,6 +708,105 @@ class CaptureStoreTestCase(unittest.TestCase):
         os.rmdir(store.directory)
         assert_closed(self, self.parent)
 
+    # -- platform identity checks -------------------------------------------
+
+    def test_path_and_handle_stats_are_never_compared_across_apis(self):
+        """Windows (Python 3.12+) reports the change time from stat but the
+        creation time from fstat, and may encode dev/ino differently; only
+        size and mtime_ns may be compared across the two APIs."""
+        source_bytes = png_bytes(2, 2, b"\x66" * 40)
+        self.write("a.png", source_bytes)
+        store = self.make_store()
+        original_fstat = os.fstat
+
+        def shifted(fd):
+            st = original_fstat(fd)
+            return os.stat_result(
+                (
+                    st.st_mode,
+                    st.st_ino + 7,
+                    st.st_dev + 7,
+                    st.st_nlink,
+                    st.st_uid,
+                    st.st_gid,
+                    st.st_size,
+                    st.st_atime,
+                    st.st_mtime,
+                    st.st_ctime + 1,
+                ),
+                {
+                    "st_atime_ns": st.st_atime_ns,
+                    "st_mtime_ns": st.st_mtime_ns,
+                    "st_ctime_ns": st.st_ctime_ns + 1_000_000_000,
+                },
+            )
+
+        with patch.object(capture_module.os, "fstat", shifted):
+            cap = self.capture(store, "a.png")
+            facts = store.verify(self.root, "a.png")
+        self.assertEqual(cap.sha256, hashlib.sha256(source_bytes).hexdigest())
+        self.assertEqual(facts.sha256, cap.sha256)
+        store.release(cap.token)
+        self.assert_empty(store)
+
+    def test_fresh_file_captures_twice_on_every_platform(self):
+        data = png_bytes(1, 1, os.urandom(3 * capture_module.CHUNK_SIZE))
+        self.write("fresh.png", data)
+        store = self.make_store()
+        first = self.capture(store, "fresh.png")
+        second = self.capture(store, "fresh.png")
+        self.assertEqual(first.sha256, hashlib.sha256(data).hexdigest())
+        self.assertEqual(second.sha256, first.sha256)
+        self.assertNotEqual(first.token, second.token)
+        self.assertEqual(store.stats()["captures"], 2)
+
+    def test_error_messages_never_name_server_paths(self):
+        self.write("a.png", png_bytes(1, 1))
+        self.write("dir.png/x", b"")  # dir.png is a directory
+        store = self.make_store(max_file=64)
+        attempts = [
+            lambda: self.capture(store, "missing.png"),
+            lambda: self.capture(store, "a.png"),  # over the 64-byte cap
+            lambda: self.capture(store, "dir.png"),
+            lambda: self.capture(store, "../a.png"),
+            lambda: self.capture(store, "notes.txt"),
+            lambda: store.verify(self.root, "missing.png"),
+        ]
+        if os.name == "posix" and os.geteuid() != 0:
+            locked = self.write("locked.png", bytes(10))
+            os.chmod(locked, 0)
+            self.addCleanup(os.chmod, locked, 0o600)
+            attempts.append(lambda: self.capture(store, "locked.png"))
+        for attempt in attempts:
+            with self.assertRaises(capture_module.CaptureError) as caught:
+                attempt()
+            message = str(caught.exception)
+            self.assertTrue(message)
+            self.assertNotIn(str(self.root), message)
+            self.assertNotIn(str(self.parent), message)
+            self.assertNotIn(os.sep, message)
+        self.assert_empty(store)
+
+    def test_close_during_a_capture_removes_the_directory_once_it_finishes(self):
+        self.write("a.png", png_bytes(1, 1, bytes(64)))
+        store = CaptureStore(self.parent / "captures", clock=self.clock)
+        original = CaptureStore._read
+
+        def closing(store_, stream, size):
+            data = original(store_, stream, 16)
+            if data and not getattr(closing, "done", False):
+                closing.done = True
+                store.close()
+            return data
+
+        with patch.object(CaptureStore, "_read", closing):
+            with self.assertRaises(RuntimeError):
+                self.capture(store, "a.png")
+        self.assertFalse(Path(store.directory).exists())
+        self.assertEqual(store.stats()["reserved_bytes"], 0)
+        store.close()
+        assert_closed(self, self.parent)
+
     # -- constants ---------------------------------------------------------
 
     def test_defaults_match_the_plan(self):

utils/media_capture.py

@@ -11,8 +11,8 @@ lifetimes.
 
 The contract is "these bytes at this location, last checked at this time".
 Stat equality is never content proof: a same-size rewrite that restores
-``mtime_ns`` is visible only through the POSIX change time, which Windows does
-not expose through ``stat``.
+``mtime_ns`` is visible only through the change time between the two
+``fstat`` reads, which on Windows is the creation time (no signal).
 """
 
 from __future__ import annotations
@@ -146,18 +146,22 @@ class _Entry:
         self.doomed = False
 
 
-def _same_file(
-    before: os.stat_result, after: os.stat_result, *, change_time: bool
-) -> bool:
-    """Size and mtime must agree, device/inode where meaningful.
+def _same_facts(before: os.stat_result, after: os.stat_result) -> bool:
+    """The facts every stat API reports alike: size and modification time.
 
-    The change time is compared only between two reads of the same kind
-    (``fstat`` before and after the copy): Windows reports the creation time
-    from ``fstat`` but, on Python 3.12+, the change time from ``stat``.
+    This is the only comparison made across APIs (a path ``stat`` against the
+    opened descriptor's ``fstat``). Windows reports the creation time from
+    ``fstat`` but, on Python 3.12+, the change time from ``stat``, and may
+    encode device and inode differently, so those fields are compared only
+    between two reads of the same kind (``_same_identity``).
     """
-    if before.st_size != after.st_size or before.st_mtime_ns != after.st_mtime_ns:
-        return False
-    if change_time and before.st_ctime_ns != after.st_ctime_ns:
+    return before.st_size == after.st_size and before.st_mtime_ns == after.st_mtime_ns
+
+
+def _same_identity(before: os.stat_result, after: os.stat_result) -> bool:
+    """Full identity between two reads of the same kind (both ``fstat`` or both
+    path stats): size, mtime, change time, and device/inode where meaningful."""
+    if not _same_facts(before, after) or before.st_ctime_ns != after.st_ctime_ns:
         return False
     if before.st_ino and after.st_ino and before.st_ino != after.st_ino:
         return False
@@ -166,6 +170,11 @@ def _same_file(
     return True
 
 
+def _reason(error: OSError) -> str:
+    """The OS error text without the path, so messages never name server paths."""
+    return error.strerror or type(error).__name__
+
+
 def _unlink_quietly(path: str) -> None:
     try:
         os.unlink(path)
@@ -219,6 +228,7 @@ class CaptureStore:
         self._pending_bytes = 0
         self._workers = 0
         self._closed = False
+        self._stale: list[str] = []
 
     # -- public API ------------------------------------------------------------
 
@@ -264,66 +274,84 @@ class CaptureStore:
         return FileFacts(locator, sha256, before.st_size, before.st_mtime_ns)
 
     def get(self, token: str) -> Capture | None:
-        """The live capture for *token*, touching its idle clock; else None."""
-        with self._lock:
-            entry = self._live_locked(token)
-            if entry is None:
-                return None
-            entry.last_used_at = self._clock()
-            return entry.capture
+        """The live capture for *token*, touching its idle clock; else None.
+
+        ``get`` does not pin the file: a reader that streams ``capture.path``
+        (the preview route) must bracket the read with ``acquire`` and
+        ``release_use``, or a concurrent ``sweep``/``release`` may unlink the
+        file under it (and, on Windows, leak it until ``close``).
+        """
+        try:
+            with self._lock:
+                entry = self._live_locked(token)
+                if entry is None:
+                    return None
+                entry.last_used_at = self._clock()
+                return entry.capture
+        finally:
+            self._flush()
 
     def acquire(self, token: str) -> Capture | None:
         """Mark the capture in use (inference); it outlives expiry until released."""
-        with self._lock:
-            entry = self._live_locked(token)
-            if entry is None:
-                return None
-            entry.uses += 1
-            entry.last_used_at = self._clock()
-            return entry.capture
+        try:
+            with self._lock:
+                entry = self._live_locked(token)
+                if entry is None:
+                    return None
+                entry.uses += 1
+                entry.last_used_at = self._clock()
+                return entry.capture
+        finally:
+            self._flush()
 
     def release_use(self, token: str) -> None:
         """End one ``acquire``; a doomed capture is removed with its last use."""
         with self._lock:
             entry = self._entries.get(token)
-            if entry is None:
-                return
-            entry.uses = max(0, entry.uses - 1)
-            entry.last_used_at = self._clock()
-            if entry.doomed and entry.uses == 0:
-                self._remove_locked(entry)
+            if entry is not None:
+                entry.uses = max(0, entry.uses - 1)
+                entry.last_used_at = self._clock()
+                if entry.doomed and entry.uses == 0:
+                    self._remove_locked(entry)
+        self._flush()
 
     def release(self, token: str) -> bool:
         """Explicit release; True when the token was live."""
         with self._lock:
             entry = self._entries.get(token)
-            if entry is None or entry.doomed:
-                return False
-            self._doom_locked(entry)
-            return True
+            live = entry is not None and not entry.doomed
+            if live:
+                self._doom_locked(entry)
+        self._flush()
+        return live
 
     def invalidate_all(self) -> None:
         """Expire every capture now (storage epoch changed, restore, shutdown)."""
         with self._lock:
             for entry in list(self._entries.values()):
                 self._doom_locked(entry)
+        self._flush()
 
     def sweep(self) -> None:
         """Remove idle and hard-expired captures that are not in use."""
         with self._lock:
             self._sweep_locked(self._clock())
+        self._flush()
 
     def expires_in(self, token: str) -> float | None:
         """Seconds until *token* expires without further use; None when gone."""
-        with self._lock:
-            entry = self._live_locked(token)
-            if entry is None:
-                return None
-            now = self._clock()
-            remaining = self.hard - (now - entry.created_at)
-            if entry.uses == 0:
-                remaining = min(remaining, self.idle - (now - entry.last_used_at))
-            return max(0.0, remaining)
+        try:
+            with self._lock:
+                entry = self._live_locked(token)
+                if entry is None:
+                    return None
+                now = self._clock()
+                remaining = self.hard - (now - entry.created_at)
+                if entry.uses == 0:
+                    remaining = min(remaining, self.idle - (now - entry.last_used_at))
+                return max(0.0, remaining)
+        finally:
+            self._flush()
 
     def stats(self) -> dict:
         with self._lock:
@@ -334,11 +362,21 @@ class CaptureStore:
             }
 
     def close(self) -> None:
-        """Remove every owned capture file and the owned directory. Idempotent."""
+        """Remove every owned capture file and the owned directory. Idempotent.
+
+        A capture still in flight finishes first: its worker removes its own
+        temp file and then the directory (``_release_admission``).
+        """
         with self._lock:
             self._closed = True
             for entry in list(self._entries.values()):
                 self._remove_locked(entry)
+            busy = self._pending > 0
+        self._flush()
+        if not busy:
+            self._remove_directory()
+
+    def _remove_directory(self) -> None:
         try:
             os.rmdir(self.directory)
         except FileNotFoundError:
@@ -351,6 +389,12 @@ class CaptureStore:
     # -- admission -------------------------------------------------------------
 
     def _admit(self, size: int, *, reserve: bool) -> None:
+        try:
+            self._admit_locked(size, reserve=reserve)
+        finally:
+            self._flush()
+
+    def _admit_locked(self, size: int, *, reserve: bool) -> None:
         with self._lock:
             if self._closed:
                 raise RuntimeError("The capture store is closed")
@@ -373,6 +417,9 @@ class CaptureStore:
             if reserve:
                 self._pending -= 1
                 self._pending_bytes -= size
+            last_out = self._closed and self._pending == 0 and self._workers == 0
+        if last_out:
+            self._remove_directory()
 
     def _register(self, capture: Capture) -> None:
         with self._lock:
@@ -408,9 +455,17 @@ class CaptureStore:
             self._remove_locked(entry)
 
     def _remove_locked(self, entry: _Entry) -> None:
-        _unlink_quietly(entry.capture.path)
+        """Forget the entry; its file is unlinked by ``_flush`` after the lock."""
         if self._entries.pop(entry.capture.token, None) is not None:
             self._completed_bytes -= entry.capture.size
+            self._stale.append(entry.capture.path)
+
+    def _flush(self) -> None:
+        """Unlink files released under the lock, without holding it (slow I/O)."""
+        with self._lock:
+            stale, self._stale = self._stale, []
+        for path in stale:
+            _unlink_quietly(path)
 
     # -- copy and hash -----------------------------------------------------------
 
@@ -420,7 +475,10 @@ class CaptureStore:
         except FileNotFoundError:
             raise CaptureChanged("The file is no longer there") from None
         except OSError as error:
-            raise CaptureChanged(f"The file could not be read: {error}") from None
+            logger.debug("Capture pre-stat failed for %s: %s", candidate, error)
+            raise CaptureChanged(
+                f"The file could not be read ({_reason(error)})"
+            ) from None
         if stat_module.S_ISLNK(facts.st_mode):
             raise CaptureDenied("Symbolic links are not captured")
         if not stat_module.S_ISREG(facts.st_mode):
@@ -485,10 +543,13 @@ class CaptureStore:
         try:
             fd = os.open(candidate, _OPEN_FLAGS)
         except OSError as error:
-            raise CaptureChanged(f"The file could not be opened: {error}") from None
+            logger.debug("Capture open failed for %s: %s", candidate, error)
+            raise CaptureChanged(
+                f"The file could not be opened ({_reason(error)})"
+            ) from None
         with os.fdopen(fd, "rb") as stream:
             opened = os.fstat(stream.fileno())
-            if not _same_file(before, opened, change_time=False):
+            if not _same_facts(before, opened):
                 raise CaptureChanged("The file changed before it was read")
             while True:
                 chunk = self._read(stream, CHUNK_SIZE)
@@ -501,24 +562,25 @@ class CaptureStore:
                 if sink is not None:
                     sink.write(chunk)
             after = os.fstat(stream.fileno())
-        if (
-            total != before.st_size
-            or not _same_file(opened, after, change_time=True)
-            or not _same_file(before, after, change_time=False)
-        ):
+        if total != before.st_size or not _same_identity(opened, after):
             raise CaptureChanged("The file changed while it was read")
         try:
-            current = os.stat(candidate)
+            current = os.lstat(candidate)
         except OSError:
             raise CaptureChanged("The file is no longer there") from None
-        if not _same_file(before, current, change_time=False):
+        if not _same_identity(before, current):
             raise CaptureChanged("The path names another file now")
         if os.path.normcase(os.path.realpath(candidate)) != locator:
             raise CaptureChanged("The path resolves elsewhere now")
         return digest.hexdigest()
 
     def _probe(self, path: str, media_type: str):
-        """Header-only dimensions of the capture; never decodes pixels."""
+        """Header-only dimensions of the capture; never decodes pixels.
+
+        Uses the inspection reader, so a PNG whose prompt metadata is over the
+        inspection budgets is reported as unreadable (not inferable) although
+        its pixels decode; B2 may switch to a dimensions-only header read.
+        """
         if media_type == "video":
             return None, None, False, VIDEO_ONLY_REASON
         try: