diff --git a/scripts/artifacts.py b/scripts/artifacts.py index 7243904d..21124928 100644 --- a/scripts/artifacts.py +++ b/scripts/artifacts.py @@ -43,11 +43,18 @@ def write_if_changed(path: str, content: str, normalize=None) -> bool: ) if _strip_timestamps(before) == _strip_timestamps(after): return False - # 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. + write_text_atomic(path, content) + return True + + +def write_text_atomic(path: str, content: str) -> None: + """Write a whole file or nothing. + + Truncate-then-write leaves a half-written artifact behind an interrupt: + a partial database.json, manifest 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" @@ -60,7 +67,24 @@ def write_if_changed(path: str, content: str, normalize=None) -> bool: with contextlib.suppress(OSError): os.unlink(scratch) raise - return True + + +def copy_file_atomic(source: str, path: str) -> None: + """Copy a file into place whole or not at all, metadata included.""" + import shutil + + directory = os.path.dirname(os.path.abspath(path)) + handle, scratch = tempfile.mkstemp( + dir=directory, prefix=f".{os.path.basename(path)}.", suffix=".tmp" + ) + os.close(handle) + try: + shutil.copy2(source, scratch) + os.replace(scratch, path) + except BaseException: + with contextlib.suppress(OSError): + os.unlink(scratch) + raise def _strip_timestamps(text: str) -> str: """Remove known timestamp patterns for content comparison.""" diff --git a/scripts/release_record.py b/scripts/release_record.py index 6984f426..49d5af22 100644 --- a/scripts/release_record.py +++ b/scripts/release_record.py @@ -24,6 +24,7 @@ import zipfile from pathlib import Path import split_pack +from common import write_text_atomic RECORD = "release.json" @@ -124,7 +125,7 @@ def main() -> int: print(f"Error: {line}", file=sys.stderr) if mismatches: return 1 - args.output.write_text(json.dumps(record, indent=2) + "\n", encoding="utf-8") + write_text_atomic(str(args.output), json.dumps(record, indent=2) + "\n") print(f"Wrote {args.output}") return 0 diff --git a/scripts/restore_large_files.py b/scripts/restore_large_files.py index 194397ca..8200cd80 100644 --- a/scripts/restore_large_files.py +++ b/scripts/restore_large_files.py @@ -19,11 +19,10 @@ from __future__ import annotations import argparse import hashlib import os -import shutil import sys sys.path.insert(0, os.path.dirname(__file__)) -from common import load_database +from common import copy_file_atomic, load_database def gitignored_paths(gitignore: str) -> set[str]: @@ -72,7 +71,9 @@ def restore( unsatisfied.append(path) continue os.makedirs(os.path.dirname(path), exist_ok=True) - shutil.copy2(source, path) + # Whole or absent: a scan running beside a plain copy hashed the + # truncated file and published that in the database. + copy_file_atomic(source, path) print(f"Restored: {path}") restored += 1 print(f"Total: {restored} files restored") diff --git a/tests/test_atomic_artifacts.py b/tests/test_atomic_artifacts.py new file mode 100644 index 00000000..b6649ed4 --- /dev/null +++ b/tests/test_atomic_artifacts.py @@ -0,0 +1,69 @@ +"""Generated artifacts are written whole or not at all. + +write_if_changed got the scratch-and-rename treatment, but the install +manifests, the target manifests and release.json were still truncated at +their final name, and restore_large_files copied an asset straight to its +path in bios/: a scan running beside the copy hashed the truncated file. +""" + +from __future__ import annotations + +import os +import sys +import tempfile +import unittest +from pathlib import Path +from unittest import mock + +REPO_ROOT = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(REPO_ROOT / "scripts")) + +import artifacts # noqa: E402 + + +class WholeOrNothing(unittest.TestCase): + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.dir = Path(self._tmp.name) + + def tearDown(self): + self._tmp.cleanup() + + def test_an_interrupted_write_keeps_the_previous_file(self): + target = self.dir / "manifest.json" + target.write_text("{}") + with mock.patch("os.replace", side_effect=OSError("cut")), self.assertRaises(OSError): + artifacts.write_text_atomic(str(target), "{\"files\": []}") + self.assertEqual(target.read_text(), "{}") + self.assertEqual(os.listdir(self.dir), ["manifest.json"]) + + def test_an_interrupted_copy_leaves_no_file(self): + source = self.dir / "asset.bin" + source.write_bytes(b"x" * 1000) + target = self.dir / "bios" / "asset.bin" + target.parent.mkdir() + with mock.patch("shutil.copy2", side_effect=OSError("cut")), self.assertRaises(OSError): + artifacts.copy_file_atomic(str(source), str(target)) + self.assertEqual(os.listdir(target.parent), []) + + +class EveryWriterGoesThroughIt(unittest.TestCase): + def test_manifests_record_and_restore_use_the_atomic_helpers(self): + scripts = REPO_ROOT / "scripts" + for name, helper in ( + ("generate_pack.py", "write_text_atomic("), + ("release_record.py", "write_text_atomic("), + ("restore_large_files.py", "copy_file_atomic("), + ): + source = (scripts / name).read_text(encoding="utf-8") + with self.subTest(script=name): + self.assertIn(helper, source) + pack = (scripts / "generate_pack.py").read_text(encoding="utf-8") + self.assertNotIn('with open(path, "w") as f:\n f.write(new_json)', pack) + self.assertNotIn("json.dump(result, f", pack) + self.assertNotIn("args.output.write_text(", (scripts / "release_record.py").read_text()) + self.assertNotIn("shutil.copy2(source, path)", (scripts / "restore_large_files.py").read_text()) + + +if __name__ == "__main__": + unittest.main()