fix(config): migrate retired filesystem folder keys instead of exiting (#5211)

* fix(config): migrate retired filesystem folder keys instead of exiting

5.2 wrote filesystem.roms_folder and filesystem.firmware_folder into
config.yml on every settings save, so instances that never chose a layout
crash-looped on 5.3.1. Appliance installs (TrueNAS, Unraid, CasaOS) have
no way to edit the file.

Translate the keys at load time into the filesystem.structure templates
that reproduce the layout 5.2 resolved them to, using the same disk check
5.2 used to choose between `<folder>/{platform}` and `{platform}/<folder>`.
A declared structure template still wins. The migrated config is written
back when config.yml is writable; otherwise it applies in memory and logs
the replacement once.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>

* fix(config): refuse folder names a template can't express before migrating

Addresses Greptile review on #5211: a retired folder name containing braces
produced an invalid template after config.yml had already been rewritten.
The migrated templates are now parsed first and startup exits with the
reason, leaving the file untouched. Also trims a test docstring.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>

* fix(config): code-review fixes for the retired folder key migration

Keep an undetectable layout in memory, accept a '.' folder, honour a
differently cased declared key, stop retrying a failed write, and share
the layout detection with check_library_layout.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>

* refactor(config): simplify pass over the retired folder key migration

Build the templates in one pass with a parser per key, reuse get_nested
and the library fixture, skip the layout string on the in-memory path,
and memoize a detected layout across get_config() calls.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>

* test(config): review-polish, drop a docstring that restates the test name

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>

* fix(config): migrate retired folder keys at startup and write config.yml atomically

Loading the config only translates the retired keys in memory now;
startup writes the migration once, before any worker runs, so processes
no longer race to rewrite config.yml.

config.yml is written to a sibling temp file and renamed over the
original, keeping a symlink and the file mode. When the rename fails
(a config.yml bind-mounted on its own), the in-place write puts the
previous content back if it fails, so a full disk no longer empties it.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>

* refactor(config): drop the atomic config write and flatten the folder key migration

The temp-file rename, symlink/mode handling and in-place restore are a
separate concern from migrating the retired keys, so config.yml goes back
to the plain write. The translation validates its two templates directly
instead of through a table of parsers.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>

---------

Co-authored-by: Claude Opus 5.5 (1M context) <[email protected]>

backend/config/config_manager.py

@@ -4,8 +4,8 @@ import functools
 import glob
 import json
 import os
+import posixpath
 import sys
-from collections.abc import Callable
 from dataclasses import dataclass
 from pathlib import Path
 from typing import Any, Final, NotRequired, Self, TextIO, TypedDict
@@ -47,6 +47,9 @@ RESERVED_STRUCTURE_KEYS: Final = frozenset(
     {STRUCTURE_DEFAULT_KEY, STRUCTURE_FIRMWARE_KEY}
 )
 
+# Folder-name keys 5.2 wrote to config.yml, with the default each one had.
+RETIRED_FOLDER_KEYS: Final = {"roms_folder": "roms", "firmware_folder": "bios"}
+
 STRUCTURE_DOCS_URL: Final = (
     "https://docs.romm.app/latest/getting-started/folder-structure/"
 )
@@ -244,6 +247,20 @@ def parse_platform_templates(
     )
 
 
+def _platform_first_layout(folder: str) -> bool | None:
+    """Whether the library keeps `folder` inside each platform folder.
+
+    `<folder>` at the library root wins when both layouts exist. Returns ``None``
+    when neither does, as in an empty or not yet mounted library.
+    """
+    if os.path.isdir(os.path.join(LIBRARY_BASE_PATH, folder)):
+        return False
+    pattern = os.path.join(LIBRARY_BASE_PATH, "*", glob.escape(folder))
+    if any(os.path.isdir(match) for match in glob.iglob(pattern)):
+        return True
+    return None
+
+
 ROMM_USER_CONFIG_PATH: Final = f"{ROMM_BASE_PATH}/config"
 ROMM_USER_CONFIG_FILE: Final = f"{ROMM_USER_CONFIG_PATH}/config.yml"
 SQLITE_DB_BASE_PATH: Final = f"{ROMM_BASE_PATH}/database"
@@ -596,6 +613,8 @@ class ConfigManager:
     # Tests require custom config path
     def __init__(self, config_file: str = ROMM_USER_CONFIG_FILE) -> None:
         self.config_file = config_file
+        self._retired_layout_detected: bool | None = None
+        self._platform_first_layouts: dict[str, bool] = {}
 
         try:
             # Check if the config file is mounted
@@ -718,6 +737,7 @@ class ConfigManager:
 
     def _parse_config(self) -> None:
         """Parses each entry in the config.yml"""
+        self._retired_layout_detected = self._translate_retired_filesystem_keys()
 
         self.config = Config(
             CONFIG_FILE_MOUNTED=self._config_file_mounted,
@@ -972,35 +992,116 @@ class ConfigManager:
 
         return normalized
 
-    def _check_retired_filesystem_keys(self) -> None:
-        """Exit if config.yml still sets a folder name that a template replaced.
+    def _detect_platform_first(self, folder: str) -> bool | None:
+        # get_config() reparses on every call, and a found layout doesn't flip.
+        if folder not in self._platform_first_layouts:
+            detected = _platform_first_layout(folder)
+            if detected is None:
+                return None
+            self._platform_first_layouts[folder] = detected
+        return self._platform_first_layouts[folder]
 
-        Ignoring one would relocate the library under the user.
+    def _translate_retired_filesystem_keys(self) -> bool | None:
+        """Replace, in memory, the folder-name keys 5.2 saved on every settings
+        change with the templates that reproduce the layout 5.2 resolved them to.
+
+        Returns:
+            ``None`` when there was nothing to translate, otherwise whether the
+            library showed which layout it uses.
         """
-        retired: dict[str, tuple[str, Callable[[str], str]]] = {
-            "filesystem.roms_folder": (
-                STRUCTURE_DEFAULT_KEY,
-                lambda folder: f"{folder}/{{platform}}/{{game}}",
-            ),
-            "filesystem.firmware_folder": (
-                STRUCTURE_FIRMWARE_KEY,
-                lambda folder: f"{folder}/{{platform}}",
-            ),
+        filesystem = get_nested(self._raw_config, "filesystem")
+        if not isinstance(filesystem, dict) or not (
+            RETIRED_FOLDER_KEYS & filesystem.keys()
+        ):
+            return None
+        structure = filesystem.setdefault("structure", {})
+        if not isinstance(structure, dict):
+            return None
+
+        roms_folder, firmware_folder = (
+            filesystem.pop(key, default) for key, default in RETIRED_FOLDER_KEYS.items()
+        )
+        for key, folder in zip(
+            RETIRED_FOLDER_KEYS, (roms_folder, firmware_folder), strict=False
+        ):
+            if not isinstance(folder, str) or not folder:
+                log.critical(
+                    f"Invalid config.yml: filesystem.{key} must be a non-empty string"
+                )
+                sys.exit(3)
+
+        # Validation folds structure keys to lowercase, so `Default` is declared too.
+        undeclared = RESERVED_STRUCTURE_KEYS - {str(key).lower() for key in structure}
+        if not undeclared:
+            return True
+        platform_first = self._detect_platform_first(roms_folder)
+
+        def place(folder: str) -> str:
+            # A `.` folder is the library root, which a template spells as no section.
+            return posixpath.normpath(
+                f"{{platform}}/{folder}" if platform_first else f"{folder}/{{platform}}"
+            )
+
+        templates = {
+            STRUCTURE_DEFAULT_KEY: f"{place(roms_folder)}/{{game}}",
+            STRUCTURE_FIRMWARE_KEY: place(firmware_folder),
         }
-        for key, (structure_key, to_template) in retired.items():
-            folder = get_nested(self._raw_config, key)
-            if folder is None:
-                continue
+        # Templates have no escape for braces, so some folder names can't migrate.
+        try:
+            parse_structure_template(templates[STRUCTURE_DEFAULT_KEY])
+            parse_firmware_template(templates[STRUCTURE_FIRMWARE_KEY])
+        except ValueError as exc:
             log.critical(
-                f"Invalid config.yml: {key} is no longer supported. Replace it "
-                f"with the equivalent layout:\n\n"
-                f"  filesystem:\n"
-                f"    structure:\n"
-                f'      {structure_key}: "{to_template(folder)}"\n\n'
+                "Invalid config.yml: filesystem.roms_folder and "
+                "filesystem.firmware_folder can't be migrated to a layout "
+                f"template ({exc}). Rename the folder or set filesystem.structure. "
                 f"See {STRUCTURE_DOCS_URL}."
             )
             sys.exit(3)
 
+        structure.update({key: templates[key] for key in undeclared})
+        return platform_first is not None
+
+    def migrate_retired_filesystem_keys(self) -> None:
+        """Write the layout that replaces the retired folder keys to config.yml."""
+        config = self.get_config()
+        if self._retired_layout_detected is None:
+            return
+
+        layout = "\n".join(
+            f'      {key}: "{config.STRUCTURE_TEMPLATES[key]}"'
+            for key in (STRUCTURE_DEFAULT_KEY, STRUCTURE_FIRMWARE_KEY)
+        )
+        if not self._retired_layout_detected:
+            log.warning(
+                "The library is empty or not mounted, so filesystem.roms_folder and "
+                "filesystem.firmware_folder are migrated in memory only, as:\n\n"
+                f"  filesystem:\n    structure:\n{layout}\n\n"
+                "config.yml is updated on the first startup that finds the library."
+            )
+            return
+
+        try:
+            if not config.CONFIG_FILE_WRITABLE:
+                raise ConfigNotWritableException
+            self._write_raw_config()
+        except ConfigNotWritableException:
+            self._config_file_writable = False
+            log.warning(
+                "config.yml is read-only, so filesystem.roms_folder and "
+                "filesystem.firmware_folder are migrated in memory only. Replace "
+                f"them with the equivalent layout:\n\n"
+                f"  filesystem:\n    structure:\n{layout}\n\n"
+                f"See {STRUCTURE_DOCS_URL}."
+            )
+            return
+
+        log.info(
+            "Migrated filesystem.roms_folder and filesystem.firmware_folder "
+            f"in config.yml to the equivalent layout:\n\n"
+            f"  filesystem:\n    structure:\n{layout}\n"
+        )
+
     def check_library_layout(self) -> None:
         """Exit if the library is laid out as `{platform}/roms` with no template.
 
@@ -1009,11 +1110,7 @@ class ConfigManager:
         """
         if STRUCTURE_DEFAULT_KEY in self.config.STRUCTURE_TEMPLATES:
             return
-        if os.path.isdir(os.path.join(LIBRARY_BASE_PATH, "roms")):
-            return
-
-        pattern = os.path.join(LIBRARY_BASE_PATH, "*", "roms")
-        if not any(os.path.isdir(match) for match in glob.iglob(pattern)):
+        if not _platform_first_layout("roms"):
             return
 
         log.critical(
@@ -1029,8 +1126,6 @@ class ConfigManager:
 
     def _validate_config(self) -> None:
         """Validates the config.yml file"""
-        self._check_retired_filesystem_keys()
-
         if not isinstance(self.config.GAMELIST_AUTO_EXPORT_ON_SCAN, bool):
             log.critical("Invalid config.yml: scan.gamelist.export must be a boolean")
             sys.exit(3)
@@ -1433,6 +1528,9 @@ class ConfigManager:
                 "containers": self.config.STREAMING_CONTAINERS,
             }
 
+        self._write_raw_config()
+
+    def _write_raw_config(self) -> None:
         try:
             # Ensure the config directory exists
             os.makedirs(os.path.dirname(self.config_file), exist_ok=True)

backend/startup.py

@@ -175,6 +175,7 @@ async def main() -> None:
     async with initialize_context():
         log.info("Running startup tasks")
 
+        cm.migrate_retired_filesystem_keys()
         cm.check_library_layout()
 
         if SMTP_HOST and SMTP_SECURITY not in SMTP_SECURITY_MODES:

backend/tests/config/test_config_loader.py

@@ -4,6 +4,7 @@ from contextlib import contextmanager
 from pathlib import Path
 
 import pytest
+import yaml
 
 from config.config_manager import (
     DEFAULT_EXCLUDED_EXTENSIONS,
@@ -16,6 +17,7 @@ from config.config_manager import (
     parse_platform_templates,
     parse_structure_template,
 )
+from exceptions.config_exceptions import ConfigNotWritableException
 
 
 @contextmanager
@@ -830,23 +832,164 @@ def _write_filesystem_config(tmp_path: Path, block: str) -> ConfigManager:
     return ConfigManager(str(config_file))
 
 
+@pytest.fixture
+def library(tmp_path, mocker):
+    library = tmp_path / "library"
+    library.mkdir()
+    mocker.patch("config.config_manager.LIBRARY_BASE_PATH", str(library))
+    return library
+
+
 @pytest.mark.parametrize(
-    ("key", "folder", "expected"),
+    ("block", "folders", "expected"),
     [
-        ("roms_folder", "retro_games", 'default: "retro_games/{platform}/{game}"'),
-        ("firmware_folder", "fw", 'firmware: "fw/{platform}"'),
+        (
+            "  roms_folder: roms\n  firmware_folder: bios\n",
+            ["roms/n64"],
+            {"default": "roms/{platform}/{game}", "firmware": "bios/{platform}"},
+        ),
+        (
+            "  roms_folder: roms\n  firmware_folder: bios\n",
+            ["n64/roms"],
+            {"default": "{platform}/roms/{game}", "firmware": "{platform}/bios"},
+        ),
+        (
+            "  roms_folder: retro_games\n",
+            ["retro_games/n64"],
+            {"default": "retro_games/{platform}/{game}", "firmware": "bios/{platform}"},
+        ),
+        (
+            "  firmware_folder: fw\n",
+            ["n64/roms"],
+            {"default": "{platform}/roms/{game}", "firmware": "{platform}/fw"},
+        ),
+        (
+            "  roms_folder: .\n",
+            ["n64"],
+            {"default": "{platform}/{game}", "firmware": "bios/{platform}"},
+        ),
     ],
 )
-def test_retired_folder_keys_exit_with_the_replacement(
-    tmp_path, key, folder, expected, critical
+def test_retired_folder_keys_migrate_to_the_layout_5_2_resolved(
+    tmp_path, library, block, folders, expected
 ):
-    """Ignoring them would silently relocate the library, so refuse to start and
-    name the template that reproduces the layout."""
+    for folder in folders:
+        (library / folder).mkdir(parents=True)
+
+    loader = _write_filesystem_config(tmp_path, block)
+    loader.migrate_retired_filesystem_keys()
+
+    assert loader.get_config().STRUCTURE_TEMPLATES == expected
+    saved = yaml.safe_load((tmp_path / "config.yml").read_text())
+    assert saved["filesystem"] == {"structure": expected}
+    loader.check_library_layout()
+
+
+def test_loading_a_config_with_retired_folder_keys_never_rewrites_it(tmp_path, library):
+    """Every process loads the config, so only startup writes the migration."""
+    (library / "roms").mkdir()
+
+    loader = _write_filesystem_config(tmp_path, "  roms_folder: roms\n")
+
+    assert loader.get_config().STRUCTURE_TEMPLATES["default"] == (
+        "roms/{platform}/{game}"
+    )
+    assert (tmp_path / "config.yml").read_text() == "filesystem:\n  roms_folder: roms\n"
+
+
+@pytest.mark.parametrize("default_key", ["default", "Default"])
+def test_a_declared_structure_wins_over_a_retired_folder_key(
+    tmp_path, library, default_key
+):
+    (library / "n64" / "roms").mkdir(parents=True)
+
+    loader = _write_filesystem_config(
+        tmp_path,
+        "  roms_folder: roms\n"
+        "  structure:\n"
+        f'    {default_key}: "roms/{{platform}}/{{game}}"\n',
+    )
+
+    assert loader.get_config().STRUCTURE_TEMPLATES == {
+        "default": "roms/{platform}/{game}",
+        "firmware": "{platform}/bios",
+    }
+
+
+def test_an_undetectable_layout_migrates_in_memory_until_detected(tmp_path, library):
+    """An empty or unmounted library can't tell the layouts apart, so the guess
+    isn't written until the library shows which one it uses."""
+    loader = _write_filesystem_config(tmp_path, "  roms_folder: roms\n")
+    loader.migrate_retired_filesystem_keys()
+
+    assert loader.get_config().STRUCTURE_TEMPLATES == {
+        "default": "roms/{platform}/{game}",
+        "firmware": "bios/{platform}",
+    }
+    assert "roms_folder" in (tmp_path / "config.yml").read_text()
+
+    (library / "n64" / "roms").mkdir(parents=True)
+    loader.migrate_retired_filesystem_keys()
+
+    expected = {"default": "{platform}/roms/{game}", "firmware": "{platform}/bios"}
+    assert loader.get_config().STRUCTURE_TEMPLATES == expected
+    saved = yaml.safe_load((tmp_path / "config.yml").read_text())
+    assert saved["filesystem"] == {"structure": expected}
+
+
+def test_retired_folder_keys_migrate_in_memory_when_read_only(
+    tmp_path, library, mocker
+):
+    (library / "games").mkdir()
+    warning = mocker.patch("config.config_manager.log.warning")
+    mocker.patch("config.config_manager.os.access", return_value=False)
+    config_file = tmp_path / "config.yml"
+    config_file.write_text("filesystem:\n  roms_folder: games\n")
+
+    loader = ConfigManager(str(config_file))
+    loader.migrate_retired_filesystem_keys()
+    loader.get_config()
+
+    assert loader.config.STRUCTURE_TEMPLATES["default"] == "games/{platform}/{game}"
+    assert "roms_folder" in config_file.read_text()
+    assert warning.call_count == 1
+    assert 'default: "games/{platform}/{game}"' in warning.call_args[0][0]
+
+
+def test_a_failed_migration_write_marks_the_config_read_only(tmp_path, library, mocker):
+    (library / "roms").mkdir()
+    write = mocker.patch.object(
+        ConfigManager, "_write_raw_config", side_effect=ConfigNotWritableException
+    )
+
+    loader = _write_filesystem_config(tmp_path, "  roms_folder: roms\n")
+    loader.migrate_retired_filesystem_keys()
+    config = loader.get_config()
+
+    assert write.call_count == 1
+    assert config.CONFIG_FILE_WRITABLE is False
+    assert config.STRUCTURE_TEMPLATES["default"] == "roms/{platform}/{game}"
+
+
+def test_an_unmigratable_folder_exits_without_rewriting_the_config(
+    tmp_path, library, critical
+):
+    block = "  roms_folder: games{old}\n"
     with pytest.raises(SystemExit) as excinfo:
-        _write_filesystem_config(tmp_path, f"  {key}: {folder}\n")
+        _write_filesystem_config(tmp_path, block)
 
     assert excinfo.value.code == 3
-    assert expected in critical()
+    assert "games{old}" in critical()
+    assert (tmp_path / "config.yml").read_text() == f"filesystem:\n{block}"
+
+
+@pytest.mark.parametrize("value", ['""', "42"])
+def test_an_invalid_retired_folder_key_exits(tmp_path, library, value, critical):
+    with pytest.raises(SystemExit) as excinfo:
+        _write_filesystem_config(tmp_path, f"  roms_folder: {value}\n")
+
+    assert excinfo.value.code == 3
+    assert "filesystem.roms_folder" in critical()
 
 
 def test_an_override_may_not_move_the_platform_folder(tmp_path, critical):
@@ -886,13 +1029,11 @@ def test_an_invalid_firmware_template_is_rejected(tmp_path):
 
 
 def test_the_retired_library_layout_check_names_the_template(
-    tmp_path, mocker, critical
+    tmp_path, library, critical
 ):
     """A `{platform}/roms` library used to be auto-detected; now it has to say so
     rather than scan as empty and mark every rom missing."""
-    library = tmp_path / "library"
     (library / "n64" / "roms").mkdir(parents=True)
-    mocker.patch("config.config_manager.LIBRARY_BASE_PATH", str(library))
 
     loader = _write_filesystem_config(tmp_path, "  skip_hash_calculation: false\n")
     with pytest.raises(SystemExit) as excinfo:
@@ -902,10 +1043,8 @@ def test_the_retired_library_layout_check_names_the_template(
     assert 'default: "{platform}/roms/{game}"' in critical()
 
 
-def test_the_retired_library_layout_check_passes_once_declared(tmp_path, mocker):
-    library = tmp_path / "library"
+def test_the_retired_library_layout_check_passes_once_declared(tmp_path, library):
     (library / "n64" / "roms").mkdir(parents=True)
-    mocker.patch("config.config_manager.LIBRARY_BASE_PATH", str(library))
 
     loader = _write_filesystem_config(
         tmp_path, '  structure:\n    default: "{platform}/roms/{game}"\n'