diff --git a/scripts/releases/r2.py b/scripts/releases/r2.py index 7c4e8afe31..2962251b18 100644 --- a/scripts/releases/r2.py +++ b/scripts/releases/r2.py @@ -363,6 +363,10 @@ def read_public_receipt(base: str, key: str) -> dict: conn.close() +class _TransferMismatch(ValueError): + """A completed download whose size or SHA256 disagrees with its receipt.""" + + def download_public_object(base: str, key: str, file: Path, *, expected_size: int, expected_sha256: str) -> None: _download_url(public_artifact_url(base, key), file, lambda _attempt: {}, expected_size=expected_size, expected_sha256=expected_sha256) @@ -393,12 +397,17 @@ def _download_url(url: str, file: Path, request_headers: Callable[[int], dict[st digest.update(chunk) size += len(chunk) if size > expected_size: - raise ValueError(f"Artifact checksum mismatch: {parsed.path}") + raise _TransferMismatch(f"Artifact checksum mismatch: {parsed.path}") output.write(chunk) if size != expected_size or digest.hexdigest() != expected_sha256: - raise ValueError(f"Artifact checksum mismatch: {parsed.path}") + raise _TransferMismatch(f"Artifact checksum mismatch: {parsed.path}") temporary.replace(file) return + except _TransferMismatch: + # The receipt is the authority: a body that disagrees with it is a bad transfer, so + # fetch again; a stored object that really differs fails the same way three times. + if attempt == 3: + raise except R2RequestError as error: if error.status not in _RETRYABLE_STATUSES or attempt == 3: raise diff --git a/tests/scripts/test_release_r2.py b/tests/scripts/test_release_r2.py index cfe50969eb..67826239d8 100644 --- a/tests/scripts/test_release_r2.py +++ b/tests/scripts/test_release_r2.py @@ -793,11 +793,28 @@ def test_download_streams_verified_bytes_and_preserves_destination_on_failure(r2 monkeypatch.setattr(r2.time, 'sleep', lambda _: None) r2.download_object(**args) assert request_times == [NOW, refreshed] + # A body corrupted in transit (right length, wrong bytes) is fetched again, not failed on the spot. + sent = [] + + def corrupt_once(handler): + sent.append(1) + if len(sent) == 1: + handler._send(200, payload[:-1] + b"!") + else: + original_get(handler) + + monkeypatch.setattr(_R2StubHandler, 'do_GET', corrupt_once) + target.write_bytes(b"previous complete file") + r2.download_object(**args) + assert target.read_bytes() == payload and len(sent) == 2 monkeypatch.setattr(_R2StubHandler, 'do_GET', original_get) + # A stored object that really differs from its receipt still fails, after the bounded attempts. r2_server.store[key] = (b"corrupt replacement", '"e"') + r2_server.requests.clear() with pytest.raises(ValueError, match="checksum mismatch"): r2.download_object(**args) + assert sum(1 for method, _, _ in r2_server.requests if method == "GET") == 3 assert target.read_bytes() == payload assert list(target.parent.iterdir()) == [target] del r2_server.store[key]