Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 27 additions & 6 deletions backend/handler/filesystem/base_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -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+)$")
Expand Down Expand Up @@ -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."""
Expand Down Expand Up @@ -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
Expand All @@ -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:
Comment thread
gantoine marked this conversation as resolved.
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:
"""
Expand Down
14 changes: 10 additions & 4 deletions backend/handler/filesystem/resources_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion backend/handler/filesystem/sync_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions backend/tasks/manual/cleanup_orphaned_resources.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]] = {}
Expand All @@ -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(
Expand Down
117 changes: 117 additions & 0 deletions backend/tests/handler/filesystem/test_base_handler.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import asyncio
import errno
import shutil
import tempfile
from io import BytesIO
Expand Down Expand Up @@ -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)
20 changes: 20 additions & 0 deletions backend/tests/handler/filesystem/test_sync_handler.py
Original file line number Diff line number Diff line change
@@ -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

Expand Down Expand Up @@ -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)
Loading
Loading