From bca76e8ace2224391bd99302f65a45683550002c Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 05:17:16 +0200 Subject: [PATCH] fix: judge a cached large file from one read _fetch_asset hashed the cache copy, then hashed the path again for the release check. A copy another run promoted in between answered the second question, and the caller was told the cache held nothing when it held the file asked for. Both checks now read one snapshot, taken while the inode stays the same. --- scripts/largefiles.py | 47 ++++++++++++++++++++++++---------- tests/test_large_file_cache.py | 29 ++++++++++++++++++++- 2 files changed, 62 insertions(+), 14 deletions(-) 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}