diff --git a/scripts/largefiles.py b/scripts/largefiles.py index dcf2d214..81eb43a4 100644 --- a/scripts/largefiles.py +++ b/scripts/largefiles.py @@ -164,18 +164,15 @@ def _fetch_asset( if os.path.isdir(cached): return None + if os.path.exists(cached) and not (expected_sha1 or expected_md5): + return cached if os.path.exists(cached): - try: - hashes = compute_hashes(cached) if (expected_sha1 or expected_md5) else {} - except FileNotFoundError: - hashes = None - if hashes is None: + snapshot = _read_once(cached) + if snapshot is None: pass - elif not (expected_sha1 or expected_md5) or _matches( - hashes, expected_sha1, expected_md5 - ): + elif _matches(snapshot[0], expected_sha1, expected_md5): return cached - elif not _replaced_on_release(name, cached, offline): + elif not _replaced_on_release(name, snapshot[0]["sha256"], snapshot[1], offline): # A copy of this asset that answers another hash: the caller # wants a different revision under the same name, and the # release still serves this one. Keeping it is what lets the @@ -249,8 +246,32 @@ def _asset_urls(name: str) -> list[tuple[str, str]]: ] -def _replaced_on_release(name: str, cached: str, offline: bool) -> bool: - """Whether the release now serves other bytes than the cached copy. +def _read_once(path: str) -> tuple[dict[str, str], int] | None: + """Digests and size of the file at *path*, all read from one file, or + None when it is gone. + + Every judgement on a cache copy reads this one snapshot. Hashed, then + hashed again by path for the release check, a copy another run promoted + in between answered the second question: the copy the caller wanted was + in the cache and the caller was told there was none. + """ + # A promotion is an os.replace, so it changes the inode: the same inode + # on both sides of the read means the digests describe that one file. + for _attempt in range(3): + try: + before = os.stat(path) + hashes = compute_hashes(path) + after = os.stat(path) + except FileNotFoundError: + return None + if (before.st_ino, before.st_mtime_ns) == (after.st_ino, after.st_mtime_ns): + return hashes, after.st_size + return None + + +def _replaced_on_release(name: str, sha256: str, size: int, offline: bool) -> bool: + """Whether the release now serves other bytes than the cached copy, + whose *sha256* and *size* the caller read. The release lists a sha256 per asset: a dump rebuilt at the same size (the eight 251658304-byte DSi NANDs) differs only there, and judged by @@ -266,9 +287,9 @@ def _replaced_on_release(name: str, cached: str, offline: bool) -> bool: None, ) if digest: - return compute_hashes(cached, frozenset({"sha256"}))["sha256"] != digest + return sha256 != digest served = _served_size(name) - return served is not None and served != os.path.getsize(cached) + return served is not None and served != size _digests_cache: dict[str, str] | None = None diff --git a/tests/test_large_file_cache.py b/tests/test_large_file_cache.py index 9fa948c6..54a322cf 100644 --- a/tests/test_large_file_cache.py +++ b/tests/test_large_file_cache.py @@ -691,7 +691,10 @@ class ASameSizeReuploadIsSeen(unittest.TestCase): def replaced(self, digests: dict, size: int | None) -> bool: with mock.patch.object(largefiles, "_served_digests", lambda: digests), \ mock.patch.object(largefiles, "_served_size", lambda name: size): - return largefiles._replaced_on_release("dsi_nand.bin", str(self.cached), False) + return largefiles._replaced_on_release( + "dsi_nand.bin", hashlib.sha256(self.cached.read_bytes()).hexdigest(), + self.cached.stat().st_size, False, + ) def test_the_digest_decides(self): same = hashlib.sha256(b"OLD NAND").hexdigest() @@ -703,6 +706,30 @@ class ASameSizeReuploadIsSeen(unittest.TestCase): self.assertFalse(self.replaced({}, 8)) self.assertTrue(self.replaced({}, 9)) + def test_a_copy_promoted_during_the_check_is_the_one_judged(self): + """Another run promotes the served revision while this one hashes the + stale copy: the release check reread the path, found the served + bytes, and the caller was told the cache held no such file.""" + new = b"NEW NAND" + real = largefiles.compute_hashes + + def promote_while_hashing(path, *args, **kwargs): + result = real(path, *args, **kwargs) + if Path(path).read_bytes() != new: + promoted = self.dir / "promoted.tmp" + promoted.write_bytes(new) + os.replace(promoted, self.cached) + return result + + digests = {"dsi_nand.bin": hashlib.sha256(new).hexdigest()} + with mock.patch.object(largefiles, "compute_hashes", promote_while_hashing), \ + mock.patch.object(largefiles, "_served_digests", lambda: digests): + found = largefiles._fetch_asset( + "dsi_nand.bin", str(self.dir), hashlib.sha1(new).hexdigest(), "", + offline=False, + ) + self.assertEqual(found, str(self.cached)) + def test_the_release_check_compares_digests_at_equal_size(self): expected = {"bios/Nintendo/DSi/dsi_nand.bin": 8} assets = {"dsi_nand.bin": 8}