diff --git a/TESTING.md b/TESTING.md index 1808a641..2caf2bd7 100644 --- a/TESTING.md +++ b/TESTING.md @@ -2315,14 +2315,18 @@ Thumbnail format and settings (#317, `py/api/thumbnail_settings.py`, signature) and renamed over the preview; a failure keeps the old file. ffmpeg writes with `-update 1`, so a `%` in a name is never a sequence pattern, and reads a video as `file:`. Temp files a crash left (`..part--`) are - removed by the clear and, when older than two minutes, by a sweep at startup. + removed by the clear and, when older than two minutes, by a sweep at startup (which + also removes one carrying its own PID that predates the process: containers reuse + PIDs). - JPEG XL needs no new dependency: Pillow (natively or with `pillow_jxl`, imported when installed), else ffmpeg with `libjxl`, else `cjxl`, detected once. Quality maps to libjxl's distance (`0.1 + (100 − q) × 0.09` from 30 up: q80 → 1.9, q85 → 1.45; never lossless). Without an encoder, saving `jxl` is a 400 `jxl-unavailable` with the - reason. An encoder that fails makes that write detect again once: another encoder - found is used, none at all writes the file as WebP (logged once) and refreshes the - availability Appearance reads; if JPEG XL has no encoder, previews are written as WebP + reason. An encoder that fails because its binary or module is gone makes that write + detect again once: another encoder found is used, none at all writes the file as WebP + (logged once) and refreshes the availability Appearance reads; any other failure + raises without a probe. "No encoder" is probed again on the settings read, on a + `jxl` save and after 60 s, so one slow probe never disables JPEG XL until a restart; if JPEG XL has no encoder, previews are written as WebP and count as the format written. Encoder processes (and ffmpeg's frames) are tracked, bounded by a 30 s timeout and killed at the app's cleanup, after which none starts. - A preview in another format than the one written now is *outdated*. @@ -2334,7 +2338,8 @@ Thumbnail format and settings (#317, `py/api/thumbnail_settings.py`, frame fails instead of getting the placeholder; a current preview it keeps must decode), then removes that file's old-format previews (regular files in its `thumbnails/` folder that the source owns, the source still present; one already gone - counts as removed). Files sharing a preview name are converted once, as a group, from + counts as removed, and a path that is the new preview under another spelling is never + removed). Files sharing a preview name are converted once, as a group, from the first source above, and each file's bytes count once. A failure keeps the old preview; orphans (source gone) and links are never touched. While it runs, a change of the thumbnail format, quality or size limit is a 409 `busy` naming it, and should the diff --git a/py/api/settings_routes.py b/py/api/settings_routes.py index e3c39a3f..466ba41d 100644 --- a/py/api/settings_routes.py +++ b/py/api/settings_routes.py @@ -13,6 +13,7 @@ checks one path and writes nothing. """ import contextlib +import functools import os from aiohttp import web @@ -311,7 +312,9 @@ class SettingsRoutesMixin: if updates.get("THUMBNAIL_FORMAT") == "jxl": # #317: JPEG XL only with an encoder (a cached probe). encoder = await self._run_in_executor( - thumbnail_encode.detect_jxl_encoder + functools.partial( + thumbnail_encode.detect_jxl_encoder, recheck_missing=True + ) ) if encoder is None: return web.json_response( diff --git a/py/api/thumbnail_convert.py b/py/api/thumbnail_convert.py index f83abea4..f43a0e52 100644 --- a/py/api/thumbnail_convert.py +++ b/py/api/thumbnail_convert.py @@ -110,6 +110,22 @@ def _group_targets(targets): return ordered +def _same_file(a, b): + """Whether *a* and *b* name one file (re-review N1: on a case-insensitive + disk ``x_thumb.JPG`` is the ``x_thumb.jpg`` just written). Without inode + numbers (st_ino 0 on some SMB/FAT mounts) the resolved, normcased paths + are compared.""" + try: + first, second = os.stat(a), os.stat(b) + except OSError: + return False + if first.st_ino and second.st_ino: + return (first.st_dev, first.st_ino) == (second.st_dev, second.st_ino) + return os.path.normcase(os.path.realpath(a)) == os.path.normcase( + os.path.realpath(b) + ) + + def _format_changed(settings): """Whether previews are written in another format than *settings* now.""" return writer_settings().extension != settings.extension @@ -297,6 +313,7 @@ class ThumbnailConvertMixin: return {"action": "error", "error": SOURCE_GONE} if _format_changed(settings): return {"action": "stopped"} + old = [(path, size) for path, size in old if not _same_file(path, dst)] removed, kept = self._remove_old_previews(old) if kept: return {"action": "error", "error": OLD_NOT_REMOVED} diff --git a/py/api/thumbnail_encode.py b/py/api/thumbnail_encode.py index ac90033e..bcbee3ef 100644 --- a/py/api/thumbnail_encode.py +++ b/py/api/thumbnail_encode.py @@ -20,10 +20,13 @@ Quality maps to a JPEG XL distance with libjxl's own (q80 → d1.9, q85 → d1.45, q90 → d1.0), the quadratic below 30, and q100 → d0.1 (a preview is never lossless). Pillow's plugin applies the same mapping inside libjxl. Detection is cached (``detect_jxl_encoder(refresh=True)`` -probes again). An encoder that fails (its binary gone, or a non-zero exit) -makes one write detect again: another encoder found is used for that file, -none at all writes it as WebP (logged once) and refreshes what Appearance -reads (``encoder_status``). The same encoder found again raises. +probes again); "none found" is probed again on the settings read +(``encoder_status``) and after MISSING_ENCODER_TTL, so one slow probe never +turns JPEG XL off until a restart. An encoder that fails because its binary +or module is gone makes that write detect again: another encoder found is +used for that file, none at all writes it as WebP (logged once) and +refreshes what Appearance reads. Any other failure (one bad file) raises +without a probe. Encoder processes (and ffmpeg's video frames, images.py) go through ``run_encoder``: tracked, bounded by a timeout, and killed by @@ -108,6 +111,9 @@ def replace_unless_held(tmp, dst, guard=None): _TEMP_NAME = re.compile(r"^\..+\.(?:part|src)-(\d+)-\d+\.[A-Za-z0-9]+$") # A temp at least this old (seconds) outlived its write: no encode runs longer. STRAY_TEMP_AGE = 4 * ENCODE_TIMEOUT +# When this process started (this module's import): a temp carrying its PID +# that is older was left by an earlier run with the same PID (containers). +PROCESS_STARTED = time.time() def sweep_stray_temps(folder, min_age=None): @@ -116,8 +122,9 @@ def sweep_stray_temps(folder, min_age=None): Only ``temp_sibling`` names are touched, never a link. With *min_age* (seconds, the startup sweep) a temp younger than that, or one this - process may still be writing, is left; ``min_age=0`` (the clear, which - holds every preview write) removes them all. + process wrote since it started (it may still be writing it), is left; + ``min_age=0`` (the clear, which holds every preview write) removes them + all. """ min_age = STRAY_TEMP_AGE if min_age is None else min_age now = time.time() @@ -133,7 +140,10 @@ def sweep_stray_temps(folder, min_age=None): stat = path.lstat() if path.is_symlink() or not path.is_file(): continue - if min_age and (match.group(1) == own or now - stat.st_mtime < min_age): + if min_age and ( + now - stat.st_mtime < min_age + or (match.group(1) == own and stat.st_mtime >= PROCESS_STARTED) + ): continue path.unlink() except OSError as exc: @@ -229,6 +239,9 @@ def reset_encoders(): _UNSET = object() _detect_lock = threading.Lock() _detected = _UNSET +_detected_at = 0.0 +# Seconds after which "no JPEG XL encoder" is probed again. +MISSING_ENCODER_TTL = 60 _fallback_logged = False @@ -290,18 +303,34 @@ def _detect(): return None -def detect_jxl_encoder(refresh=False): - """``pillow``, ``ffmpeg``, ``cjxl`` or None (cached; blocking on a probe).""" - global _detected +def detect_jxl_encoder(refresh=False, recheck_missing=False): + """``pillow``, ``ffmpeg``, ``cjxl`` or None (cached; blocking on a probe). + + A cached None is probed again with *recheck_missing* or once it is older + than MISSING_ENCODER_TTL.""" + global _detected, _detected_at with _detect_lock: - if refresh or _detected is _UNSET: + now = time.monotonic() + stale_missing = _detected is None and ( + recheck_missing or now - _detected_at >= MISSING_ENCODER_TTL + ) + if refresh or _detected is _UNSET or stale_missing: _detected = _detect() + _detected_at = now return _detected +def _encoder_gone(encoder): + """Whether *encoder*'s binary or module is no longer there.""" + if encoder == "pillow": + return not _pillow_can_save_jxl() + return shutil.which(encoder) is None + + def encoder_status(): - """Which formats can be written now (``GET /prompt_manager/settings``).""" - encoder = detect_jxl_encoder() + """Which formats can be written now (``GET /prompt_manager/settings``; + a "none found" is probed again here).""" + encoder = detect_jxl_encoder(recheck_missing=True) return { "webp": {"available": True}, "jpeg": {"available": True}, @@ -367,13 +396,16 @@ class _EncoderGone(RuntimeError): def _write_jxl(img, tmp, settings, track): - """Encode with the detected encoder; one that fails makes the write - detect again once (another one is used, none raises _EncoderGone).""" + """Encode with the detected encoder. One that failed because it is gone + makes the write detect again once (another one is used, none raises + _EncoderGone); any other failure raises without a probe.""" encoder = detect_jxl_encoder() try: _encode_jxl(encoder, img, tmp, settings, track) return except (OSError, EncoderFailed) as exc: + if encoder is not None and not _encoder_gone(encoder): + raise again = detect_jxl_encoder(refresh=True) if again is None: raise _EncoderGone(str(exc)) from exc diff --git a/tests/test_thumbnail_convert.py b/tests/test_thumbnail_convert.py index 0b4124c1..8293a3ad 100644 --- a/tests/test_thumbnail_convert.py +++ b/tests/test_thumbnail_convert.py @@ -361,6 +361,55 @@ class TestConvertOutdated(ConvertCase): self.assertTrue(self._thumb_path("f3", ".jpg").exists(), "the new format stays") self.assertEqual(sum(1 for p in olds if p.exists()), 3) + async def test_the_new_preview_is_never_removed_as_an_old_one(self): + # Re-review N1: on a case-insensitive disk x_thumb.JPG (an old + # spelling) can be the x_thumb.jpg just written. A hard link stands + # in for that second name of one file. + PromptManagerConfig.update_config( + {"thumbnails": {"format": "jpeg", "quality": 80, "edge": 512}} + ) + src = self.output_dir / "x.jpg" + Image.new("RGB", (64, 64), (200, 10, 10)).save(src, "JPEG") + alias = self._thumb_path("x", ".JPG") + alias.parent.mkdir(parents=True, exist_ok=True) + Image.new("RGB", (16, 16)).save(alias, "JPEG") + later = src.stat().st_mtime + 5 + os.utime(alias, (later, later)) + real = self.api._generate_one + + def same_file(source, dst, *args, **kwargs): + made = real(source, dst, *args, **kwargs) + os.remove(alias) + os.link(made["path"], alias) + return made + + self.api._generate_one = same_file + _, job = await self._convert() + self.assertTrue(self._thumb_path("x", ".jpg").exists()) + self.assertTrue(alias.exists(), "the new preview's other name is kept") + self.assertEqual(job.counters["errors"], 0) + + def test_same_file_falls_back_to_the_path_without_inode_numbers(self): + from py.api import thumbnail_convert as tc + + a = self.output_dir / "a_thumb.jpg" + b = self.output_dir / "b_thumb.jpg" + for path in (a, b): + path.write_bytes(b"x") + real_stat = os.stat + + def no_inode(path, *args, **kwargs): + result = real_stat(path, *args, **kwargs) + return os.stat_result( + (result.st_mode, 0, result.st_dev) + tuple(result)[3:] + ) + + with patch.object(tc.os, "stat", side_effect=no_inode): + self.assertFalse(tc._same_file(a, b)) + self.assertTrue(tc._same_file(a, a)) + self.assertTrue(tc._same_file(a, self.output_dir / "." / "a_thumb.jpg")) + self.assertFalse(tc._same_file(a, self.output_dir / "gone.jpg")) + def test_byte_sizes_are_readable(self): from py.api.thumbnail_convert import readable_bytes diff --git a/tests/test_thumbnail_encode.py b/tests/test_thumbnail_encode.py index 330d83cb..3c402011 100644 --- a/tests/test_thumbnail_encode.py +++ b/tests/test_thumbnail_encode.py @@ -341,7 +341,8 @@ class TestStrayTemps(TempCase): self._touch(".c_thumb.part-x-2.webp"), self._touch(".young_thumb.part-7-8.webp", age=0), ] - own = self._touch(f".mine_thumb.part-{os.getpid()}-9.webp") + # A temp of this process written since it started may be in use. + own = self._touch(f".mine_thumb.part-{os.getpid()}-9.webp", age=0) count, size = te.sweep_stray_temps(self.tmp, min_age=60) self.assertEqual((count, size), (3, 30)) for path in strays: @@ -363,6 +364,17 @@ class TestStrayTemps(TempCase): if link is not None: self.assertTrue(link.is_symlink()) + def test_an_own_pid_temp_older_than_this_process_is_swept(self): + # Re-review N3: under stable PIDs (Docker) the previous run's crash + # leftovers carry this process's PID; they predate its start. + before = te.PROCESS_STARTED - 3600 + stale = self._touch(f".old_thumb.part-{os.getpid()}-9.webp") + os.utime(stale, (before, before)) + live = self._touch(f".new_thumb.part-{os.getpid()}-9.webp", age=0) + self.assertEqual(te.sweep_stray_temps(self.tmp, min_age=60)[0], 1) + self.assertFalse(stale.exists()) + self.assertTrue(live.exists()) + def test_a_missing_folder_sweeps_nothing(self): self.assertEqual(te.sweep_stray_temps(self.tmp / "none"), (0, 0)) @@ -373,9 +385,10 @@ class TestEncoderVanishes(TempCase): availability Appearance reads is refreshed.""" def test_a_missing_binary_falls_back_to_webp_and_refreshes(self): - found = iter(["cjxl", None]) + found = iter(["cjxl", None, None]) with ( patch.object(te, "_detect", side_effect=lambda: next(found)), + patch.object(te.shutil, "which", return_value=None), patch.object(te, "run_encoder", side_effect=FileNotFoundError("cjxl")), self.assertLogs("prompt_manager.thumbnails", logging.WARNING), ): @@ -405,6 +418,11 @@ class TestEncoderVanishes(TempCase): with ( patch.object(te, "_detect", side_effect=lambda: next(found)), + patch.object( + te.shutil, + "which", + side_effect=lambda name: None if name == "cjxl" else name, + ), patch.object(te, "run_encoder", side_effect=fake), patch.object(te, "_pillow_decodes_jxl", return_value=False), ): @@ -417,10 +435,13 @@ class TestEncoderVanishes(TempCase): self.assertEqual(used, ["cjxl", "ffmpeg"]) self.assertEqual(self._leftovers(), []) - def test_the_same_encoder_failing_again_raises(self): + def test_a_failure_with_the_encoder_present_does_not_detect_again(self): + # Re-review N2: one bad file never runs a probe (which may time out + # under load and turn JPEG XL off). failed = subprocess.CompletedProcess([], 1, b"", b"bad input") with ( patch.object(te, "_detect", return_value="cjxl") as detect, + patch.object(te.shutil, "which", return_value="/usr/bin/cjxl"), patch.object(te, "run_encoder", return_value=failed), ): with self.assertRaises(te.EncoderFailed): @@ -429,7 +450,29 @@ class TestEncoderVanishes(TempCase): self.tmp / "x_thumb.jxl", ThumbnailSettings("jxl", 80, 64), ) - self.assertEqual(detect.call_count, 2, "detected again once") + self.assertEqual(detect.call_count, 1, "no second probe") + + def test_no_encoder_found_is_checked_again_on_the_settings_read(self): + # Re-review N2: a probe that found nothing (a timeout) recovers. + found = iter([None, "cjxl"]) + with patch.object(te, "_detect", side_effect=lambda: next(found)): + self.assertIsNone(te.detect_jxl_encoder()) + self.assertEqual(te.encoder_status()["jxl"]["encoder"], "cjxl") + self.assertEqual(te.detect_jxl_encoder(), "cjxl") + + def test_no_encoder_found_is_checked_again_after_a_while(self): + found = iter([None, "ffmpeg"]) + clock = [1000.0] + with ( + patch.object(te, "_detect", side_effect=lambda: next(found)) as detect, + patch.object(te.time, "monotonic", side_effect=lambda: clock[0]), + ): + self.assertIsNone(te.detect_jxl_encoder()) + self.assertIsNone(te.detect_jxl_encoder(), "cached within the TTL") + clock[0] += te.MISSING_ENCODER_TTL + 1 + self.assertEqual(te.detect_jxl_encoder(), "ffmpeg") + self.assertEqual(te.detect_jxl_encoder(), "ffmpeg") + self.assertEqual(detect.call_count, 2) class TestEncoderProcesses(TempCase):