diff --git a/scripts/common.py b/scripts/common.py index 2520359e..3459107e 100644 --- a/scripts/common.py +++ b/scripts/common.py @@ -716,8 +716,17 @@ _data_dir_index: dict[str, dict[str, list[tuple[int, str]]]] = {} def _data_dir_basenames(cache_dir: str) -> dict[str, list[tuple[int, str]]]: - """Casefolded basename -> (walk order, path) of every file in a cache.""" - key = os.path.realpath(cache_dir) + """Casefolded basename -> (walk order, path) of every file in a cache. + + Keyed by the tree's identity as well as its path: a refresh swaps in a + new directory under the same name, and an index of the old one named + files that were gone. + """ + try: + stat = os.stat(cache_dir) + except OSError: + return {} + key = f"{os.path.realpath(cache_dir)}:{stat.st_dev}:{stat.st_ino}" if key not in _data_dir_index: index: dict[str, list[tuple[int, str]]] = {} order = 0 diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index 6eb70995..d3bf829c 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -64,7 +64,7 @@ from common import ( yaml_load, ) import packresolve -from refresh_data_dirs import cache_lock +from refresh_data_dirs import cache_lock, hold_data_dirs import region as region_mod from manifestselect import SelectionIndex import slot as slot_mod @@ -2777,7 +2777,11 @@ def _given_flags(args) -> set[str]: def main(): + with contextlib.ExitStack() as held: + _main(held) + +def _main(held: contextlib.ExitStack) -> None: parser = argparse.ArgumentParser(description="Generate platform BIOS ZIP packs") parser.add_argument("--platform", "-p", help="Platform name (e.g., retroarch)") parser.add_argument( @@ -2936,6 +2940,10 @@ def main(): _validate_args(args, parser) # Hash lookup / pack mode + # A platform run may refresh the trees below, under the exclusive lock: + # it takes the shared ones after that. Every other mode reads them now. + if not (args.platform or args.all) or args.offline or args.no_refresh_data: + hold_data_dirs(held, load_data_dir_registry(args.platforms_dir)) has_from_md5 = bool(args.from_md5 or getattr(args, "from_md5_file", None)) if has_from_md5: hashes = ( @@ -3049,6 +3057,8 @@ def main(): failed = sorted(k for k, v in results.items() if v is None) if failed: print(f"WARNING: data directory refresh failed: {', '.join(failed)}") + if data_registry: + hold_data_dirs(held, data_registry) emu_profiles = load_emulator_profiles(args.emulators_dir) diff --git a/scripts/refresh_data_dirs.py b/scripts/refresh_data_dirs.py index 08d38baa..f034a4bf 100644 --- a/scripts/refresh_data_dirs.py +++ b/scripts/refresh_data_dirs.py @@ -62,6 +62,21 @@ def cache_lock(cache_dir: str | Path, shared: bool = False): return _file_lock(cache_dir.with_name(f".{cache_dir.name}.lock"), shared=shared) +def hold_data_dirs(stack: contextlib.ExitStack, registry: dict) -> None: + """Hold every cached data directory shared for the life of *stack*. + + A pack resolves names against a tree, then copies the file minutes + later; a refresh swapping the tree in between shipped bytes nobody + verified, or died on a file the new tree no longer has. A refresh waits + for the holders. Never called before a refresh in the same process: + the exclusive lock would wait on this one. + """ + for entry in registry.values(): + cache = entry.get("local_cache", "") + if cache and os.path.isdir(cache): + stack.enter_context(cache_lock(cache, shared=True)) + + def _load_versions(versions_path: str = VERSIONS_FILE) -> dict[str, dict]: path = Path(versions_path) if not path.exists(): diff --git a/scripts/verify.py b/scripts/verify.py index 57823e83..8351c0c8 100644 --- a/scripts/verify.py +++ b/scripts/verify.py @@ -25,6 +25,7 @@ Usage: from __future__ import annotations import argparse +import contextlib import json import os import sys @@ -2077,6 +2078,11 @@ def _run_listing(args: argparse.Namespace, parser: argparse.ArgumentParser) -> N def main(): + with contextlib.ExitStack() as held: + _main(held) + + +def _main(held: contextlib.ExitStack) -> None: parser = argparse.ArgumentParser(description="Platform-native BIOS verification") parser.add_argument("--platform", "-p", help="Platform name") parser.add_argument( @@ -2146,6 +2152,10 @@ def main(): "--platform, --all, --emulator, and --system are mutually exclusive" ) _refuse_mode_flags(args, parser) + # The trees stay put while names resolve against them and files are read. + from refresh_data_dirs import hold_data_dirs + + hold_data_dirs(held, load_data_dir_registry(args.platforms_dir)) with open(args.db) as f: db = json.load(f) diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index e8db4ca7..9d0c1f4d 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -3190,3 +3190,57 @@ class AnEmptyTargetIsNotEveryCore(unittest.TestCase): self.assertEqual(published["board"], sorted(builder)) self.assertEqual(published["board"], ["gpsp"]) self.assertIsNone(published["full"]) + + +class ADataDirectoryIsHeldWhileRead(unittest.TestCase): + """A pack resolved names against data/bluemsx, then a concurrent refresh + swapped the tree: the index named files that were gone and the copy + died on FileNotFoundError, or shipped bytes nobody had verified.""" + + def setUp(self): + self.tmp = Path(tempfile.mkdtemp(dir=TMP_ROOT)) + self.addCleanup(lambda: __import__("shutil").rmtree(self.tmp, True)) + + def test_the_index_follows_a_swapped_tree(self): + from scripts import common + + cache = self.tmp / "cache" + cache.mkdir() + (cache / "old.rom").write_bytes(b"old") + self.assertIn("old.rom", common._data_dir_basenames(str(cache))) + cache.rename(self.tmp / "gone") + cache.mkdir() + (cache / "new.rom").write_bytes(b"new") + index = common._data_dir_basenames(str(cache)) + self.assertIn("new.rom", index) + self.assertNotIn("old.rom", index) + + def test_a_holder_makes_the_refresh_lock_wait(self): + import fcntl + + from scripts import refresh_data_dirs + + cache = self.tmp / "bluemsx" + cache.mkdir() + lock = cache.with_name(f".{cache.name}.lock") + + def exclusive_free() -> bool: + with open(lock, "a") as handle: + try: + fcntl.flock(handle, fcntl.LOCK_EX | fcntl.LOCK_NB) + except BlockingIOError: + return False + fcntl.flock(handle, fcntl.LOCK_UN) + return True + + with contextlib.ExitStack() as held: + refresh_data_dirs.hold_data_dirs(held, {"bluemsx": {"local_cache": str(cache)}}) + self.assertFalse(exclusive_free()) + self.assertTrue(exclusive_free()) + + def test_both_entry_points_hold_the_trees(self): + pack = (ROOT / "scripts" / "generate_pack.py").read_text() + verify_src = (ROOT / "scripts" / "verify.py").read_text() + self.assertIn("hold_data_dirs(held,", verify_src) + refresh = pack.index("results = refresh_all(") + self.assertIn("hold_data_dirs(held, data_registry)", pack[refresh:])