diff --git a/scripts/artifacts.py b/scripts/artifacts.py index 8251944b..b901a8a9 100644 --- a/scripts/artifacts.py +++ b/scripts/artifacts.py @@ -8,6 +8,7 @@ from __future__ import annotations import contextlib import os +import tempfile import re @@ -42,8 +43,23 @@ def write_if_changed(path: str, content: str, normalize=None) -> bool: ) if _strip_timestamps(before) == _strip_timestamps(after): return False - with open(path, "w") as f: - f.write(content) + # Truncate-then-write leaves a half-written artifact behind an interrupt, + # and every generator in the repo funnels through here: a partial + # database.json or README.md is committed-looking and silently wrong. + # The scratch file sits beside the target so the rename stays on one + # filesystem, which is what makes it atomic. + directory = os.path.dirname(os.path.abspath(path)) + handle, scratch = tempfile.mkstemp( + dir=directory, prefix=f".{os.path.basename(path)}.", suffix=".tmp" + ) + try: + with os.fdopen(handle, "w") as f: + f.write(content) + os.replace(scratch, path) + except BaseException: + with contextlib.suppress(OSError): + os.unlink(scratch) + raise return True def _strip_timestamps(text: str) -> str: diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index 048867c7..e303b322 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -2445,9 +2445,10 @@ def main(): _run_verify_packs(args) return if args.manifest_targets: - generate_target_manifests( - os.path.join(args.platforms_dir, "targets"), args.output_dir - ) + with _pack_output_lock(args.output_dir): + generate_target_manifests( + os.path.join(args.platforms_dir, "targets"), args.output_dir + ) return if args.list: for p in list_platforms(args.platforms_dir): @@ -2498,18 +2499,19 @@ def main(): ) return zip_contents = build_zip_contents_index(db) - result = generate_md5_pack( - hashes=hashes, - db=db, - bios_dir=args.bios_dir, - output_dir=args.output_dir, - zip_contents=zip_contents, - platform_name=args.platform, - platforms_dir=args.platforms_dir, - emulator_name=args.emulator, - emulators_dir=args.emulators_dir, - standalone=getattr(args, "standalone", False), - ) + with _pack_output_lock(args.output_dir): + result = generate_md5_pack( + hashes=hashes, + db=db, + bios_dir=args.bios_dir, + output_dir=args.output_dir, + zip_contents=zip_contents, + platform_name=args.platform, + platforms_dir=args.platforms_dir, + emulator_name=args.emulator, + emulators_dir=args.emulators_dir, + standalone=getattr(args, "standalone", False), + ) if not result: sys.exit(1) return @@ -2520,36 +2522,40 @@ def main(): # Emulator mode if args.emulator: names = [n.strip() for n in args.emulator.split(",") if n.strip()] - if not generate_emulator_pack( - names, - args.emulators_dir, - db, - args.bios_dir, - args.output_dir, - args.standalone, - zip_contents, - required_only=args.required_only, - regions=getattr(args, "regions", None), - offline=args.offline, - ): + with _pack_output_lock(args.output_dir): + built = generate_emulator_pack( + names, + args.emulators_dir, + db, + args.bios_dir, + args.output_dir, + args.standalone, + zip_contents, + required_only=args.required_only, + regions=getattr(args, "regions", None), + offline=args.offline, + ) + if not built: sys.exit(1) return # System mode (standalone, without platform context) if args.system and not args.platform and not args.all: system_ids = [s.strip() for s in args.system.split(",") if s.strip()] - if not generate_system_pack( - system_ids, - args.emulators_dir, - db, - args.bios_dir, - args.output_dir, - args.standalone, - zip_contents, - required_only=args.required_only, - regions=getattr(args, "regions", None), - offline=args.offline, - ): + with _pack_output_lock(args.output_dir): + built = generate_system_pack( + system_ids, + args.emulators_dir, + db, + args.bios_dir, + args.output_dir, + args.standalone, + zip_contents, + required_only=args.required_only, + regions=getattr(args, "regions", None), + offline=args.offline, + ) + if not built: sys.exit(1) return @@ -2608,9 +2614,10 @@ def main(): ) if args.manifest: - _run_manifest_mode( - args, groups, db, zip_contents, emu_profiles, target_cores_cache - ) + with _pack_output_lock(args.output_dir): + _run_manifest_mode( + args, groups, db, zip_contents, emu_profiles, target_cores_cache + ) else: with _pack_output_lock(args.output_dir): _run_platform_packs( diff --git a/scripts/largefiles.py b/scripts/largefiles.py index 685ac2cf..7178a55e 100644 --- a/scripts/largefiles.py +++ b/scripts/largefiles.py @@ -5,6 +5,7 @@ against the hash the caller declares.""" from __future__ import annotations +import contextlib import os import tempfile import urllib.error @@ -28,17 +29,29 @@ def fetch_large_file( ) -> str | None: """Return a verified cached large file, downloading it only when allowed.""" cached = os.path.join(dest_dir, name) + # Between the existence test and the hash, a concurrent run can drop the + # same stale entry: the file is gone by the time this one reads it, and + # both of them try to unlink it. + def _drop(path: str) -> None: + with contextlib.suppress(FileNotFoundError): + os.unlink(path) + if os.path.exists(cached): - if expected_sha1 or expected_md5: - hashes = compute_hashes(cached) + try: + hashes = compute_hashes(cached) if (expected_sha1 or expected_md5) else {} + except FileNotFoundError: + hashes = None + if hashes is None: + pass + elif expected_sha1 or expected_md5: if expected_sha1 and hashes["sha1"].lower() != expected_sha1.lower(): - os.unlink(cached) + _drop(cached) elif expected_md5: md5_list = [ m.strip().lower() for m in expected_md5.split(",") if m.strip() ] if hashes["md5"].lower() not in md5_list: - os.unlink(cached) + _drop(cached) else: return cached else: diff --git a/scripts/refresh_data_dirs.py b/scripts/refresh_data_dirs.py index c8fa6071..3775d812 100644 --- a/scripts/refresh_data_dirs.py +++ b/scripts/refresh_data_dirs.py @@ -282,10 +282,24 @@ def _download_and_extract_zip( shutil.copyfileobj(src, dst) file_count += 1 - if cache_dir.exists(): - shutil.rmtree(cache_dir) + # The old tree is stepped aside rather than deleted: removing it + # first and then failing to move the new one in left the cache with + # nothing at all, and the next run reads that as "never fetched". cache_dir.parent.mkdir(parents=True, exist_ok=True) - shutil.move(str(extract_dir), str(cache_dir)) + previous = None + if cache_dir.exists(): + previous = cache_dir.with_name(cache_dir.name + ".previous") + if previous.exists(): + shutil.rmtree(previous) + os.replace(cache_dir, previous) + try: + shutil.move(str(extract_dir), str(cache_dir)) + except BaseException: + if previous is not None: + os.replace(previous, cache_dir) + raise + if previous is not None: + shutil.rmtree(previous, ignore_errors=True) return file_count diff --git a/tests/test_artifact_lock.py b/tests/test_artifact_lock.py index 032336f5..93d0945c 100644 --- a/tests/test_artifact_lock.py +++ b/tests/test_artifact_lock.py @@ -96,6 +96,36 @@ class PackLockCliTest(unittest.TestCase): self.assertEqual(proc.returncode, 1, proc.stdout + proc.stderr) self.assertIn("is in use by another run", proc.stdout) + def test_every_writing_mode_refuses_a_locked_output(self): + """Two of seven modes took the lock; five wrote straight into it. + + A mode that writes packs or manifests into a directory another run + holds produces the half-written artifact the lock exists to prevent. + """ + modes = [ + (["--emulator", "handy", "--offline"], "emulator"), + (["--system", "atari-lynx", "--offline"], "system"), + (["--manifest-targets"], "manifest-targets"), + (["--platform", "misterfpga", "--manifest", "--offline"], "manifest"), + ( + ["--platform", "misterfpga", "--from-md5", + "d8f1206299c48946e6ec5ef96d014eaa", "--offline"], + "from-md5", + ), + ] + with artifact_lock(self.dir): + for argv, label in modes: + with self.subTest(mode=label): + proc = self._run( + ["scripts/generate_pack.py", *argv, + "--output-dir", self.dir] + ) + self.assertNotEqual( + proc.returncode, 0, + f"{label} wrote into a directory held by another run", + ) + self.assertIn("in use", (proc.stdout + proc.stderr).lower()) + def test_verify_packs_refuses_a_writer(self): with artifact_lock(self.dir): proc = self._run(