Merge pull request #3388 from rommapp/hardlink-resources-gamelist

feat(fs): hardlink import/export assets, harden sync init

Author: gantoine

backend/handler/filesystem/base_handler.py

@@ -16,8 +16,9 @@
 from starlette.datastructures import UploadFile
 
 from config.config_manager import config_manager as cm
+from logger.logger import log
 from models.base import FILE_NAME_MAX_LENGTH
-from utils.filesystem import iter_directories, iter_files
+from utils.filesystem import iter_directories, iter_files, link_or_copy_file
 
 TAG_REGEX = re.compile(r"\(([^)]+)\)|\[([^]]+)\]")
 EXTENSION_REGEX = re.compile(r"\.(([a-z]+\.)*\w+)$")
@@ -148,13 +149,22 @@ class Asset(Enum):
 
 
 class FSHandler:
-    def __init__(self, base_path: str):
+    def __init__(self, base_path: str, tolerate_missing_base: bool = False):
         self.base_path = Path(base_path).resolve()
         self._locks: dict[str, asyncio.Lock] = {}
         self._lock_mutex = asyncio.Lock()
 
-        # Create base directory synchronously during initialization
-        self.base_path.mkdir(parents=True, exist_ok=True)
+        # Create base directory synchronously during initialization.
+        try:
+            self.base_path.mkdir(parents=True, exist_ok=True)
+        except OSError:
+            if not tolerate_missing_base:
+                raise
+
+            log.warning(
+                f"Could not create or access {self.base_path}; "
+                "feature will be unavailable until the directory is writable."
+            )
 
     async def _get_file_lock(self, file_path: str) -> asyncio.Lock:
         """Get or create a lock for a specific file path."""
@@ -487,13 +497,21 @@ async def stream_file(self, file_path: str):
 
             return await open_file(full_path, "rb")
 
-    async def copy_file(self, source_full_path: Path, dest_path: str) -> None:
+    async def copy_file(
+        self,
+        source_full_path: Path,
+        dest_path: str,
+        allow_link: bool = False,
+    ) -> None:
         """
         Copy a file from source to destination.
 
         Args:
             source_full_path: Absolute path to the source file
             dest_path: Relative path to the destination file
+            allow_link: Try a hardlink first and fall back to
+            a copy when the link isn't possible (cross-device,
+            unsupported filesystem, etc.)
 
         Raises:
             FileNotFoundError: If source file does not exist
@@ -518,7 +536,10 @@ async def copy_file(self, source_full_path: Path, dest_path: str) -> None:
             # Create destination directory if needed
             dest_parent_anyio_path = AnyioPath(str(dest_full_path.parent))
             await dest_parent_anyio_path.mkdir(parents=True, exist_ok=True)
-            shutil.copy2(str(source_full_path), str(dest_full_path))
+            if allow_link:
+                link_or_copy_file(source_full_path, dest_full_path)
+            else:
+                shutil.copy2(str(source_full_path), str(dest_full_path))
 
     async def move_file_or_folder(self, source_path: str, dest_path: str) -> None:
         """

backend/handler/filesystem/resources_handler.py

@@ -129,7 +129,9 @@ async def _store_cover(
                     log.warning(f"Cover file not found: {url_cover}")
                     return None
                 dest_path = f"{cover_file}/{size.value}.png"
-                await self.copy_file(resolved, dest_path)
+                # Small-size covers get resized in place, which would mutate
+                # the user's source image if the destination were a hardlink.
+                await self.copy_file(resolved, dest_path, allow_link=False)
 
                 if ENABLE_SCHEDULED_CONVERT_IMAGES_TO_WEBP:
                     self.image_converter.convert_to_webp(
@@ -302,7 +304,9 @@ async def _store_screenshot(self, rom: Rom, url_screenhot: str, idx: int):
                 if resolved is None or not await AnyioPath(resolved).exists():
                     log.warning(f"Screenshot file not found: {url_screenhot}")
                     return None
-                await self.copy_file(resolved, f"{screenshot_path}/{idx}.jpg")
+                await self.copy_file(
+                    resolved, f"{screenshot_path}/{idx}.jpg", allow_link=True
+                )
             except Exception as exc:
                 log.error(f"Unable to copy screenshot file {url_screenhot}: {str(exc)}")
                 return None
@@ -416,7 +420,9 @@ async def _store_manual(self, rom: Rom, url_manual: str):
                 if resolved is None or not await AnyioPath(resolved).exists():
                     log.warning(f"Manual file not found: {url_manual}")
                     return None
-                await self.copy_file(resolved, f"{manual_path}/{rom.id}.pdf")
+                await self.copy_file(
+                    resolved, f"{manual_path}/{rom.id}.pdf", allow_link=True
+                )
             except Exception as exc:
                 log.error(f"Unable to copy manual file {url_manual}: {str(exc)}")
                 return None
@@ -555,7 +561,7 @@ async def store_media_file(self, url_media: str, dest_path: str) -> None:
             try:
                 resolved = _resolve_local_file_uri(url_media)
                 if resolved is not None and await AnyioPath(resolved).exists():
-                    await self.copy_file(resolved, dest_path)
+                    await self.copy_file(resolved, dest_path, allow_link=True)
             except Exception as exc:
                 log.error(f"Unable to copy media file {url_media}: {str(exc)}")
                 return None

backend/handler/filesystem/sync_handler.py

@@ -12,7 +12,7 @@ class FSSyncHandler(FSHandler):
     """Filesystem handler for sync folder operations (File Transfer mode)."""
 
     def __init__(self) -> None:
-        super().__init__(base_path=SYNC_BASE_PATH)
+        super().__init__(base_path=SYNC_BASE_PATH, tolerate_missing_base=True)
 
     def build_incoming_path(
         self, device_id: str, platform_slug: str | None = None

backend/tasks/manual/cleanup_orphaned_resources.py

@@ -87,7 +87,7 @@ async def run(self) -> dict[str, int]:
         platform_dirs: set[int] = {
             int(entry.name)
             async for entry in roms_resources_dir.iterdir()
-            if await entry.is_dir()
+            if entry.name.isdigit() and await entry.is_dir()
         }
 
         rom_dirs_by_platform: dict[int, set[int]] = {}
@@ -97,7 +97,7 @@ async def run(self) -> dict[str, int]:
             rom_dirs_by_platform[platform_dir] = {
                 int(entry.name)
                 async for entry in platform_dir_path.iterdir()
-                if await entry.is_dir()
+                if entry.name.isdigit() and await entry.is_dir()
             }
 
         cleanup_stats.update(

backend/tests/handler/filesystem/test_base_handler.py

@@ -1,4 +1,5 @@
 import asyncio
+import errno
 import shutil
 import tempfile
 from io import BytesIO
@@ -515,3 +516,119 @@ async def create_and_remove_dir(dir_name):
         dirs = await handler.list_directories(".")
         for i in range(5):
             assert f"test_dir_{i}" not in dirs
+
+    async def test_copy_file_does_not_hardlink_by_default(
+        self, handler: FSHandler, sample_file_content
+    ):
+        """copy_file defaults to allow_link=False: the destination is a real
+        copy, so mutating it won't affect the source. Callers must opt in to
+        hardlinking explicitly."""
+        source_full = handler.base_path / "source.bin"
+        source_full.write_bytes(sample_file_content)
+
+        await handler.copy_file(source_full, "dest.bin")
+
+        dest_full = handler.base_path / "dest.bin"
+        assert dest_full.read_bytes() == sample_file_content
+        assert source_full.stat().st_ino != dest_full.stat().st_ino
+
+    async def test_copy_file_allow_link_true_hardlinks(
+        self, handler: FSHandler, sample_file_content
+    ):
+        """allow_link=True: when source and dest are on the same filesystem,
+        the destination is a hardlink to the source (same inode)."""
+        source_full = handler.base_path / "source.bin"
+        source_full.write_bytes(sample_file_content)
+
+        await handler.copy_file(source_full, "dest.bin", allow_link=True)
+
+        dest_full = handler.base_path / "dest.bin"
+        assert dest_full.read_bytes() == sample_file_content
+        assert source_full.stat().st_ino == dest_full.stat().st_ino
+
+    async def test_copy_file_allow_link_false_does_real_copy(
+        self, handler: FSHandler, sample_file_content
+    ):
+        """allow_link=False (explicit) produces a real copy — content matches
+        but inodes differ. This matches the default but is exercised explicitly
+        to lock in the contract."""
+        source_full = handler.base_path / "source.bin"
+        source_full.write_bytes(sample_file_content)
+
+        await handler.copy_file(source_full, "dest.bin", allow_link=False)
+
+        dest_full = handler.base_path / "dest.bin"
+        assert dest_full.read_bytes() == sample_file_content
+        assert source_full.stat().st_ino != dest_full.stat().st_ino
+
+    async def test_copy_file_allow_link_falls_back_to_copy_on_exdev(
+        self, handler: FSHandler, sample_file_content
+    ):
+        """When allow_link=True and os.link raises EXDEV (cross-filesystem),
+        copy_file transparently falls back to a real copy."""
+        source_full = handler.base_path / "source.bin"
+        source_full.write_bytes(sample_file_content)
+
+        with patch(
+            "utils.filesystem.os.link",
+            side_effect=OSError(errno.EXDEV, "Cross-device link"),
+        ):
+            await handler.copy_file(source_full, "dest.bin", allow_link=True)
+
+        dest_full = handler.base_path / "dest.bin"
+        assert dest_full.read_bytes() == sample_file_content
+        assert source_full.stat().st_ino != dest_full.stat().st_ino
+
+    async def test_copy_file_nonexistent_source(self, handler: FSHandler):
+        """Missing source must raise FileNotFoundError regardless of link mode."""
+        with pytest.raises(FileNotFoundError, match="Source file not found"):
+            await handler.copy_file(handler.base_path / "missing.bin", "dest.bin")
+
+
+class TestFSHandlerTolerateMissingBase:
+    """Tests for the tolerate_missing_base flag, which lets optional features
+    (like the sync folder) come up degraded instead of crashing the whole app
+    when the base path can't be created."""
+
+    def test_default_raises_on_mkdir_failure(self):
+        """Default behavior: an OSError from mkdir propagates, so misconfigured
+        critical paths (resources, library) still fail loudly at startup."""
+        with patch.object(
+            Path, "mkdir", side_effect=PermissionError(errno.EACCES, "denied")
+        ):
+            with pytest.raises(PermissionError):
+                FSHandler("/some/unwritable/path")
+
+    def test_tolerate_missing_base_swallows_oserror(self, tmp_path, caplog):
+        """With tolerate_missing_base=True, mkdir failure is logged but does
+        not raise — the handler instance is still constructed so module-level
+        imports don't crash."""
+        target = tmp_path / "missing"
+
+        with patch.object(
+            Path, "mkdir", side_effect=PermissionError(errno.EACCES, "denied")
+        ):
+            handler = FSHandler(str(target), tolerate_missing_base=True)
+
+        # Handler exists and base_path is set, even though the directory
+        # could not be created.
+        assert handler.base_path == target.resolve()
+        assert not handler.base_path.exists()
+
+    def test_tolerate_missing_base_still_creates_when_possible(self, tmp_path):
+        """tolerate_missing_base=True should not change behavior when mkdir
+        succeeds — the directory must still be created normally."""
+        target = tmp_path / "new_dir"
+        assert not target.exists()
+
+        handler = FSHandler(str(target), tolerate_missing_base=True)
+
+        assert handler.base_path.exists()
+        assert handler.base_path.is_dir()
+
+    def test_tolerate_missing_base_reraises_non_oserror(self, tmp_path):
+        """Only OSError gets swallowed; programming errors (e.g. a RuntimeError
+        from corrupted state) must still surface."""
+        with patch.object(Path, "mkdir", side_effect=RuntimeError("unexpected")):
+            with pytest.raises(RuntimeError, match="unexpected"):
+                FSHandler(str(tmp_path / "x"), tolerate_missing_base=True)

backend/tests/handler/filesystem/test_sync_handler.py

@@ -1,9 +1,11 @@
 """Tests for filesystem sync handler."""
 
+import errno
 import os
 import shutil
 import tempfile
 from pathlib import Path
+from unittest.mock import patch
 
 import pytest
 
@@ -133,3 +135,21 @@ def test_remove_incoming_file_outside_base_raises(
     def test_remove_incoming_file_nonexistent(self, handler: FSSyncHandler):
         # Should not raise for nonexistent files
         handler.remove_incoming_file("/nonexistent/path/file.sav")
+
+
+class TestFSSyncHandlerStartup:
+    """Sync is an optional feature: if /romm/sync isn't writable (bad mount,
+    wrong ownership), the app must still boot. Failures should surface when
+    sync is actually used, not at module-import time."""
+
+    def test_init_does_not_raise_when_base_path_unwritable(self):
+        """Regression test for the PermissionError on /romm/sync at boot.
+        FSSyncHandler must construct successfully even when mkdir fails."""
+        with patch.object(
+            Path, "mkdir", side_effect=PermissionError(errno.EACCES, "denied")
+        ):
+            handler = FSSyncHandler()
+
+        assert handler is not None
+        # base_path is set even though the directory wasn't created.
+        assert isinstance(handler.base_path, Path)