diff --git a/TESTING.md b/TESTING.md index bafc5529..3281eb8b 100644 --- a/TESTING.md +++ b/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 diff --git a/tests/test_media_capture.py b/tests/test_media_capture.py index e5eb812d..b021ada1 100644 --- a/tests/test_media_capture.py +++ b/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): diff --git a/utils/media_capture.py b/utils/media_capture.py index eeafee10..f7c22c0d 100644 --- a/utils/media_capture.py +++ b/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: