fix(conversion): stop touching converted copies on serve

backend/tests/endpoints/roms/test_converted_download.py

@@ -272,7 +272,7 @@ def test_a_format_needs_a_single_file(
         pytest.param("head", "authenticated", id="metadata"),
     ],
 )
-def test_get_refreshes_a_cached_copy_but_head_does_not(
+def test_get_records_serve_without_touching_copy_but_head_does_not(
     client: TestClient,
     access_token: str,
     rom: Rom,
@@ -304,7 +304,14 @@ def test_get_refreshes_a_cached_copy_but_head_does_not(
 
     assert response.status_code == status.HTTP_200_OK
     assert _redirect(response) == "/cache/converts/1-abc/test_rom.chd"
-    assert (cached_copy.stat().st_mtime > time.time() - 60) is (method == "get")
+    # The copy's mtime feeds nginx's ETag: neither method may refresh it.
+    assert cached_copy.stat().st_mtime <= time.time() - 60
+    # Only a real download records the serve, sparing the copy from cleanup.
+    sidecar = cached_copy.parent / conversion_cache.LAST_SERVED_FILE
+    if method == "get":
+        assert sidecar.stat().st_mtime > time.time() - 60
+    else:
+        assert not sidecar.exists()
     assert conversion_cache.cleanup_stale_conversions() == (0 if method == "get" else 1)
     assert cached_copy.exists() is (method == "get")
     conversion.convert.assert_not_called()

backend/tests/utils/test_conversion_cache.py

@@ -123,14 +123,15 @@ async def test_unresolvable_returns_none_without_touching_disk(
         assert convert_outputs == []
         assert not any(cache_root.iterdir())
 
-    async def test_cache_hit_returns_path_and_refreshes_mtime(
+    async def test_cache_hit_returns_path_without_touching_mtime(
         self, cache_root, convert_outputs
     ):
         f = _rom_file()
         final = _seed_cached(f, age_seconds=3600)
+        before = final.stat().st_mtime
 
         assert await get_or_convert(1, f, "psp", "chd") == final
-        assert final.stat().st_mtime > time.time() - 60
+        assert final.stat().st_mtime == before
         assert convert_outputs == []
 
     async def test_oversized_conversion_returns_none_without_converting(
@@ -567,42 +568,35 @@ def test_returns_none_when_unresolvable_or_not_cached(self, cache_root, file_nam
 
         assert get_cached_converted(1, f, "psp", "chd") is None
 
-    @pytest.mark.parametrize("touch", [False, True])
-    def test_returns_the_copy_refreshing_mtime_only_when_asked(self, cache_root, touch):
+    def test_serving_leaves_mtime_untouched(self, cache_root):
+        # nginx builds its ETag from mtime+size: touching the copy on serve
+        # changes the ETag and breaks resumable downloads (If-Range never matches).
         f = _rom_file()
         final = _seed_cached(f, age_seconds=3600)
+        before = final.stat().st_mtime
 
-        assert get_cached_converted(1, f, "psp", "chd", touch=touch) == final
-        assert (final.stat().st_mtime > time.time() - 60) is touch
+        assert get_cached_converted(1, f, "psp", "chd") == final
+        assert final.stat().st_mtime == before
 
-    @pytest.mark.parametrize(
-        "touch",
-        [
-            pytest.param(False, id="lookup"),
-            pytest.param(True, id="download"),
-        ],
-    )
-    def test_returns_none_while_key_dir_is_locked(self, cache_root, touch):
+    def test_touch_records_serve_in_sidecar_without_touching_copy(self, cache_root):
+        # A recorded serve must not refresh the copy's mtime (it feeds nginx's
+        # ETag). Instead the serve is recorded in a sidecar file so cleanup
+        # can spare the copy.
         f = _rom_file()
-        final = _seed_cached(f)
+        final = _seed_cached(f, age_seconds=3600)
+        before = final.stat().st_mtime
 
-        with _held_lock(final.parent):
-            assert get_cached_converted(1, f, "psp", "chd", touch=touch) is None
+        assert get_cached_converted(1, f, "psp", "chd", touch=True) == final
+        assert final.stat().st_mtime == before
+        sidecar = final.parent / conversion_cache.LAST_SERVED_FILE
+        assert sidecar.stat().st_mtime > time.time() - 60
 
-    @pytest.mark.parametrize(
-        "error",
-        [
-            pytest.param(FileNotFoundError, id="touch-not-found"),
-            pytest.param(PermissionError, id="permission-denied"),
-        ],
-    )
-    def test_touch_failure_still_serves_the_copy(self, cache_root, mocker, error):
+    def test_returns_none_while_key_dir_is_locked(self, cache_root):
         f = _rom_file()
         final = _seed_cached(f)
-        mocker.patch("os.utime", side_effect=error)
 
-        assert get_cached_converted(1, f, "psp", "chd", touch=True) == final
-        assert final.read_bytes() == b"cached"
+        with _held_lock(final.parent):
+            assert get_cached_converted(1, f, "psp", "chd") is None
 
     def test_lock_open_failure_returns_none(self, cache_root, mocker):
         f = _rom_file()
@@ -834,29 +828,19 @@ def test_size_eviction_respects_serve_grace(
         assert conversion_cache.cleanup_stale_conversions() == (0 if kept else 1)
         assert final.exists() is kept
 
-    def test_size_eviction_rechecks_a_copy_served_after_the_ttl_pass(
-        self, cache_root, converto, mocker
-    ):
+    def test_cleanup_spares_a_recently_served_copy(self, cache_root, converto, mocker):
+        # A copy served moments ago must survive cleanup even though its own
+        # mtime is old: the serve is recorded in a sidecar, not in the copy's
+        # mtime (which feeds nginx's ETag and must stay stable).
         converto.cache_max_size_gb = 1
         mocker.patch.object(conversion_cache, "BYTES_PER_GB", 100)
         f = _rom_file()
-        final = _seed_cached(f)
+        final = _seed_cached(f, age_seconds=3600)
         final.write_bytes(b"x" * 120)
-        served = time.time() - 120
-        os.utime(final, (served, served))
-        iterdir = Path.iterdir
-
-        def serve_after_listing(path):
-            yield from iterdir(path)
-            if path == cache_root:
-                assert get_cached_converted(1, f, "psp", "chd", touch=True) == final
-
-        mocker.patch.object(
-            Path, "iterdir", side_effect=serve_after_listing, autospec=True
-        )
 
+        assert get_cached_converted(1, f, "psp", "chd", touch=True) == final
         assert conversion_cache.cleanup_stale_conversions() == 0
-        assert final.read_bytes() == b"x" * 120
+        assert final.exists()
 
 
 class TestParseFormats:

backend/utils/conversion_cache.py

@@ -48,6 +48,10 @@
 RETRY_AFTER_SECONDS: Final = 30
 PARTIAL_DIR: Final = ".partial"
 FAILED_FILE: Final = ".failed"
+# Records when a cached copy was last served. Its mtime (not the copy's) feeds
+# cleanup: touching the copy itself would change nginx's ETag and break
+# resumable downloads.
+LAST_SERVED_FILE: Final = ".last_served"
 # Give nginx time to open a copy after the X-Accel-Redirect response.
 SERVE_GRACE_SECONDS: Final = 60
 # Room a conversion to an uncompressed target reserves per input byte; the
@@ -140,16 +144,20 @@ def _lookup(
 
 
 def _serve_cached(final_path: Path, *, touch: bool) -> Path | None:
-    """`final_path` if it is cached, or None while cleanup or a conversion holds its dir."""
+    """`final_path` if it is cached, or None while cleanup or a conversion holds its dir.
+
+    Args:
+        touch: Record the serve so cleanup spares the copy, without touching
+            the copy itself (its mtime feeds nginx's ETag).
+    """
     with (
         contextlib.suppress(OSError),
         _try_lock(final_path.parent, shared=True) as locked,
     ):
         if locked and final_path.exists():
             if touch:
-                # Keep a served file fresh so TTL cleanup measures demand.
                 with contextlib.suppress(OSError):
-                    os.utime(final_path)
+                    (final_path.parent / LAST_SERVED_FILE).touch()
             return final_path
     return None
 
@@ -165,7 +173,7 @@ def get_cached_converted(
     """The converted file if it is already cached, without converting.
 
     Args:
-        touch: Refresh its mtime so TTL cleanup measures demand.
+        touch: Record the serve so cleanup spares the copy.
     """
     found = _lookup(rom_id, rom_file, platform_slug, target)
     return _serve_cached(found[1], touch=touch) if found else None
@@ -242,7 +250,7 @@ async def resolve_format_download(
     Args:
         allowed: The caller may start a conversion.
         start: Start it now, rather than only report that one would run.
-        touch: Refresh a served copy's mtime so TTL cleanup measures demand.
+        touch: Record a served copy so cleanup spares it.
     """
     if file_format(file.file_name) in formats:
         return FormatResolution(FormatOutcome.ORIGINAL)
@@ -311,8 +319,6 @@ async def get_or_convert(
                 return None
             # A conversion may have finished between the check above and the lock.
             if final_path.exists():
-                with contextlib.suppress(OSError):
-                    os.utime(final_path)
                 return final_path
             if (final_path.parent / FAILED_FILE).exists():
                 return None
@@ -388,20 +394,20 @@ def cleanup_stale_conversions(reserve_bytes: int = 0) -> int:
     now = time.time()
     expired_before = now - converto.cache_ttl_hours * SECONDS_PER_HOUR
     deleted = 0
-    # (last served, size) of the final copies in each dir that survives the TTL pass.
+    # (newest mtime, size) of the final copies in each dir that survives the TTL pass.
     kept: list[tuple[float, int, Path]] = []
 
     for key_dir in cache_root.iterdir():
         if not key_dir.is_dir():
             continue
         with contextlib.suppress(FileNotFoundError):
-            stats, last_served = _stat_files(key_dir)
+            stats, newest_mtime = _stat_files(key_dir)
             partial_dir = key_dir / PARTIAL_DIR
-            if last_served < expired_before or partial_dir.exists():
+            if newest_mtime < expired_before or partial_dir.exists():
                 with _try_lock(key_dir) as locked:
                     if locked:
-                        stats, last_served = _stat_files(key_dir)
-                        if last_served < expired_before:
+                        stats, newest_mtime = _stat_files(key_dir)
+                        if newest_mtime < expired_before:
                             shutil.rmtree(key_dir, ignore_errors=True)
                             deleted += 1
                             continue
@@ -435,7 +441,7 @@ def cleanup_stale_conversions(reserve_bytes: int = 0) -> int:
         if used + reserve_bytes <= max_bytes:
             break
         with contextlib.suppress(FileNotFoundError), _try_lock(key_dir) as locked:
-            # A download may have touched the copy since the pass above.
+            # A conversion may have finished since the pass above.
             if not locked or any(
                 p.stat().st_mtime >= now - SERVE_GRACE_SECONDS for p in _files(key_dir)
             ):