mirror of
https://github.com/Abdess/retroarch_system.git
synced 2026-10-10 13:33:24 -05:00
fix: write artifacts whole or not at all
This commit is contained in:
1 parent
53bd42b2f4
commit
f47088fe37
4 files changed
+105
-10
No files matched your search
+30
-6
@@ -43,11 +43,18 @@ def write_if_changed(path: str, content: str, normalize=None) -> bool:
|
|||||||
)
|
)
|
||||||
if _strip_timestamps(before) == _strip_timestamps(after):
|
if _strip_timestamps(before) == _strip_timestamps(after):
|
||||||
return False
|
return False
|
||||||
# Truncate-then-write leaves a half-written artifact behind an interrupt,
|
write_text_atomic(path, content)
|
||||||
# and every generator in the repo funnels through here: a partial
|
return True
|
||||||
# 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.
|
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))
|
directory = os.path.dirname(os.path.abspath(path))
|
||||||
handle, scratch = tempfile.mkstemp(
|
handle, scratch = tempfile.mkstemp(
|
||||||
dir=directory, prefix=f".{os.path.basename(path)}.", suffix=".tmp"
|
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):
|
with contextlib.suppress(OSError):
|
||||||
os.unlink(scratch)
|
os.unlink(scratch)
|
||||||
raise
|
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:
|
def _strip_timestamps(text: str) -> str:
|
||||||
"""Remove known timestamp patterns for content comparison."""
|
"""Remove known timestamp patterns for content comparison."""
|
||||||
|
|||||||
@@ -24,6 +24,7 @@ import zipfile
|
|||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
|
||||||
import split_pack
|
import split_pack
|
||||||
|
from common import write_text_atomic
|
||||||
|
|
||||||
RECORD = "release.json"
|
RECORD = "release.json"
|
||||||
|
|
||||||
@@ -124,7 +125,7 @@ def main() -> int:
|
|||||||
print(f"Error: {line}", file=sys.stderr)
|
print(f"Error: {line}", file=sys.stderr)
|
||||||
if mismatches:
|
if mismatches:
|
||||||
return 1
|
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}")
|
print(f"Wrote {args.output}")
|
||||||
return 0
|
return 0
|
||||||
|
|
||||||
|
|||||||
@@ -19,11 +19,10 @@ from __future__ import annotations
|
|||||||
import argparse
|
import argparse
|
||||||
import hashlib
|
import hashlib
|
||||||
import os
|
import os
|
||||||
import shutil
|
|
||||||
import sys
|
import sys
|
||||||
|
|
||||||
sys.path.insert(0, os.path.dirname(__file__))
|
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]:
|
def gitignored_paths(gitignore: str) -> set[str]:
|
||||||
@@ -72,7 +71,9 @@ def restore(
|
|||||||
unsatisfied.append(path)
|
unsatisfied.append(path)
|
||||||
continue
|
continue
|
||||||
os.makedirs(os.path.dirname(path), exist_ok=True)
|
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}")
|
print(f"Restored: {path}")
|
||||||
restored += 1
|
restored += 1
|
||||||
print(f"Total: {restored} files restored")
|
print(f"Total: {restored} files restored")
|
||||||
|
|||||||
@@ -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()
|
||||||
Reference in new issue
Block a user