From b2c739570a9c016fe8a0b0716eaf87451b519a13 Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Fri, 17 Apr 2026 11:16:30 -0700 Subject: [PATCH] ref(preprod): Revert odiff CLI-mode workaround after v4.3.8 fix odiff-bin v4.3.8 ships the upstream fix for the server-mode stdin buffer reuse bug (https://github.com/dmtrKovalenko/odiff/pull/170), so we can drop the CLI-mode fallback introduced in #112829 and go back to using server mode. Also: - Pin odiff-bin to exact 4.3.8 (no caret) in package.json and CI - Point _find_odiff_binary() at node_modules/odiff-bin/bin/odiff.exe; v4.3.8's post-install only chmods this path, while raw_binaries/ ship without the execute bit - Extract _fetch_batch_images helper in tasks.py to avoid per-iteration closure rebinding inside the batch loop Co-Authored-By: Claude --- .github/workflows/backend.yml | 2 +- package.json | 2 +- pnpm-lock.yaml | 10 +- .../preprod/snapshots/image_diff/compare.py | 27 +- .../preprod/snapshots/image_diff/odiff.py | 61 +--- src/sentry/preprod/snapshots/tasks.py | 270 +++++++++--------- 6 files changed, 158 insertions(+), 214 deletions(-) diff --git a/.github/workflows/backend.yml b/.github/workflows/backend.yml index fbcde1793f94..7dd75aef2030 100644 --- a/.github/workflows/backend.yml +++ b/.github/workflows/backend.yml @@ -271,7 +271,7 @@ jobs: - name: Download odiff binary run: | - curl -sL https://registry.npmjs.org/odiff-bin/-/odiff-bin-4.3.2.tgz \ + curl -sL https://registry.npmjs.org/odiff-bin/-/odiff-bin-4.3.8.tgz \ | tar -xz --strip-components=2 package/raw_binaries/odiff-linux-x64 sudo install -m 755 odiff-linux-x64 /usr/local/bin/odiff rm odiff-linux-x64 diff --git a/package.json b/package.json index 53fdb8a844c4..66d6ee4dfb1e 100644 --- a/package.json +++ b/package.json @@ -277,7 +277,7 @@ "jest-fail-on-console": "3.3.4", "jest-junit": "16.0.0", "knip": "6.4.1", - "odiff-bin": "^4.3.2", + "odiff-bin": "4.3.8", "oxfmt": "^0.41.0", "playwright": "^1.58.2", "postcss-styled-syntax": "0.7.0", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index c877123c1eeb..dc47f1a72d49 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -694,8 +694,8 @@ importers: specifier: 6.4.1 version: 6.4.1 odiff-bin: - specifier: ^4.3.2 - version: 4.3.2 + specifier: 4.3.8 + version: 4.3.8 oxfmt: specifier: ^0.41.0 version: 0.41.0 @@ -7387,8 +7387,8 @@ packages: obuf@1.1.2: resolution: {integrity: sha512-PX1wu0AmAdPqOL1mWhqmlOd8kOIZQwGZw6rh7uby9fTc5lhaOWFLX3I6R1hrF9k3zUY40e6igsLGkDXK92LJNg==} - odiff-bin@4.3.2: - resolution: {integrity: sha512-irdWCW/b/b4aQiEfwNolbS0ZYUe2PAK6BKPt9Orzz3mGlko7ZqmEB1oDwCTAd2TQe3RmTpnePyd77xJzvWJHYQ==} + odiff-bin@4.3.8: + resolution: {integrity: sha512-nEGbO932GgDZUT6KNI30wio+JaNhLHGbeXrDnYQF4UeSmroC55w8wRXqOAYqGJXk2xFK72RxxLnGofofwV+eDQ==} hasBin: true on-finished@2.4.1: @@ -17492,7 +17492,7 @@ snapshots: obuf@1.1.2: {} - odiff-bin@4.3.2: {} + odiff-bin@4.3.8: {} on-finished@2.4.1: dependencies: diff --git a/src/sentry/preprod/snapshots/image_diff/compare.py b/src/sentry/preprod/snapshots/image_diff/compare.py index 8b8bc10213e8..4ce103cded89 100644 --- a/src/sentry/preprod/snapshots/image_diff/compare.py +++ b/src/sentry/preprod/snapshots/image_diff/compare.py @@ -8,7 +8,7 @@ from PIL import Image -from .odiff import OdiffServer, compare_cli +from .odiff import OdiffServer from .types import DiffResult logger = logging.getLogger(__name__) @@ -79,17 +79,13 @@ def compare_images_batch( tmpdir_path = Path(tmpdir) if server is not None: return _compare_pairs(pairs, server, tmpdir_path) - # Temporarily skip creating OdiffServer due to server-mode stdin - # buffer reuse bug (https://github.com/dmtrKovalenko/odiff/pull/170). - # Revert once the fix is released in odiff-bin. - # with OdiffServer() as new_server: - # return _compare_pairs(pairs, new_server, tmpdir_path) - return _compare_pairs(pairs, None, tmpdir_path) + with OdiffServer() as new_server: + return _compare_pairs(pairs, new_server, tmpdir_path) def _compare_pairs( pairs: Sequence[tuple[bytes | Image.Image, bytes | Image.Image]], - server: OdiffServer | None, + server: OdiffServer, tmpdir_path: Path, ) -> list[DiffResult | None]: return [ @@ -102,7 +98,7 @@ def _compare_single_pair( idx: int, before: bytes | Image.Image, after: bytes | Image.Image, - server: OdiffServer | None, + server: OdiffServer, tmpdir_path: Path, ) -> DiffResult | None: before_img: Image.Image | None = None @@ -127,18 +123,7 @@ def _compare_single_pair( after_padded.save(after_path, "PNG") output_path = tmpdir_path / f"diff_{idx}.png" - # Use CLI mode instead of server mode to work around stdin buffer - # reuse bug (https://github.com/dmtrKovalenko/odiff/pull/170). - # Revert to server.compare() once the fix is released in odiff-bin. - # resp = server.compare( - # before_path, - # after_path, - # output_path, - # threshold=ODIFF_SENSITIVITY_DIFF_THRESHOLD, - # antialiasing=True, - # outputDiffMask=True, - # ) - resp = compare_cli( + resp = server.compare( before_path, after_path, output_path, diff --git a/src/sentry/preprod/snapshots/image_diff/odiff.py b/src/sentry/preprod/snapshots/image_diff/odiff.py index 683abc0e0127..372668a48653 100644 --- a/src/sentry/preprod/snapshots/image_diff/odiff.py +++ b/src/sentry/preprod/snapshots/image_diff/odiff.py @@ -1,6 +1,5 @@ from __future__ import annotations -import platform import select import shutil import subprocess @@ -10,30 +9,16 @@ from sentry.preprod.snapshots.image_diff.types import OdiffResponse from sentry.utils import json -_CLI_FLAG_MAP = { - "outputDiffMask": "diff-mask", -} - -ODIFF_PLATFORM_SUFFIXES = { - ("arm64", "Darwin"): "macos-arm64", - ("aarch64", "Darwin"): "macos-arm64", - ("x86_64", "Darwin"): "macos-x64", - ("x86_64", "Linux"): "linux-x64", - ("aarch64", "Linux"): "linux-arm64", - ("arm64", "Linux"): "linux-arm64", -} - def _find_odiff_binary() -> str: - suffix = ODIFF_PLATFORM_SUFFIXES.get((platform.machine(), platform.system())) - current = Path(__file__).resolve() for parent in current.parents: if (parent / "pyproject.toml").exists(): - if suffix: - raw = parent / "node_modules" / "odiff-bin" / "raw_binaries" / f"odiff-{suffix}" - if raw.exists(): - return str(raw) + # odiff-bin's post_install.js copies the platform-specific raw binary here with +x + # on every OS; the .exe suffix is a package convention, not Windows-only. + binary = parent / "node_modules" / "odiff-bin" / "bin" / "odiff.exe" + if binary.exists(): + return str(binary) break found = shutil.which("odiff") @@ -42,42 +27,6 @@ def _find_odiff_binary() -> str: raise FileNotFoundError("odiff binary not found. Run 'pnpm install' to install odiff-bin.") -def compare_cli( - base_path: str | Path, - compare_path: str | Path, - output_path: str | Path, - **options: object, -) -> OdiffResponse: - """Run a single odiff comparison via CLI (no server mode). - - Workaround for server-mode stdin buffer reuse bug - (https://github.com/dmtrKovalenko/odiff/pull/170). - Remove once the fix is released in odiff-bin. - """ - binary = _find_odiff_binary() - cmd = [binary, str(base_path), str(compare_path), str(output_path)] - - for key, value in options.items(): - flag = _CLI_FLAG_MAP.get(key, key) - if isinstance(value, bool): - if value: - cmd.append(f"--{flag}") - else: - cmd.extend([f"--{flag}", str(value)]) - - result = subprocess.run(cmd, capture_output=True, timeout=30) - - # Exit codes: 0=match, 21=layout diff, 22=pixel diff - if result.returncode not in (0, 21, 22): - stderr = result.stderr.decode(errors="replace").strip() - raise RuntimeError(f"odiff CLI error (exit {result.returncode}): {stderr}") - - return OdiffResponse( - requestId=0, - match=(result.returncode == 0), - ) - - class OdiffServer: def __init__(self) -> None: self._process: subprocess.Popen[bytes] | None = None diff --git a/src/sentry/preprod/snapshots/tasks.py b/src/sentry/preprod/snapshots/tasks.py index 650b750565a3..991622dd284c 100644 --- a/src/sentry/preprod/snapshots/tasks.py +++ b/src/sentry/preprod/snapshots/tasks.py @@ -15,6 +15,7 @@ from sentry.objectstore import get_preprod_session from sentry.preprod.models import PreprodArtifact, PreprodComparisonApproval from sentry.preprod.snapshots.image_diff.compare import DIFF_ALGORITHM_VERSION, compare_images_batch +from sentry.preprod.snapshots.image_diff.odiff import OdiffServer from sentry.preprod.snapshots.manifest import ( ComparisonManifest, ComparisonSummary, @@ -123,6 +124,30 @@ def _image_name_to_path_stem(name: str) -> str: return normalized.rsplit(".", 1)[0] if "." in normalized else normalized +def _fetch_batch_images( + session: Session, + key_prefix: str, + hashes: set[str], +) -> tuple[dict[str, bytes], set[str]]: + cache: dict[str, bytes] = {} + failed: set[str] = set() + lock = threading.Lock() + + def fetch(image_hash: str) -> None: + try: + data = session.get(f"{key_prefix}/{image_hash}").payload.read() + with lock: + cache[image_hash] = data + except Exception: + with lock: + failed.add(image_hash) + + with ContextPropagatingThreadPoolExecutor(max_workers=8) as executor: + list(executor.map(fetch, hashes)) + + return cache, failed + + def _create_pixel_batches( items: list[_DiffCandidate], max_pixels_per_batch: int, @@ -505,146 +530,131 @@ def compare_snapshots( # TODO: spawn N OdiffServer workers and distribute pairs across them # via a thread pool to parallelize the odiff comparison step per batch - # Temporarily bypassing OdiffServer due to server-mode stdin buffer - # reuse bug (https://github.com/dmtrKovalenko/odiff/pull/170). - # Revert once the fix is released in odiff-bin. - # with OdiffServer() as server: - for batch in batches: - diff_pairs: list[tuple[bytes, bytes]] = [] - batch_names: list[str] = [] - batch_hashes: list[tuple[str, str]] = [] - - unique_hashes: set[str] = set() - for candidate in batch: - unique_hashes.add(candidate.head_hash) - unique_hashes.add(candidate.base_hash) - - fetch_cache: dict[str, bytes] = {} - failed_hashes: set[str] = set() - cache_lock = threading.Lock() - - def _fetch_hash(h: str) -> None: - try: - data = session.get(f"{image_key_prefix}/{h}").payload.read() - with cache_lock: - fetch_cache[h] = data - except Exception: - with cache_lock: - failed_hashes.add(h) - - # Fetch unique hashes in parallel; session.get() is thread-safe - with ContextPropagatingThreadPoolExecutor(max_workers=8) as executor: - list(executor.map(_fetch_hash, unique_hashes)) - - for candidate in batch: - if candidate.head_hash in failed_hashes or candidate.base_hash in failed_hashes: - logger.warning( - "compare_snapshots: failed to fetch images for %s", - candidate.name, - extra={ - "head_artifact_id": head_artifact_id, + with OdiffServer() as server: + for batch in batches: + diff_pairs: list[tuple[bytes, bytes]] = [] + batch_names: list[str] = [] + batch_hashes: list[tuple[str, str]] = [] + + unique_hashes: set[str] = set() + for candidate in batch: + unique_hashes.add(candidate.head_hash) + unique_hashes.add(candidate.base_hash) + + # Fetch unique hashes in parallel; session.get() is thread-safe + fetch_cache, failed_hashes = _fetch_batch_images( + session, image_key_prefix, unique_hashes + ) + + for candidate in batch: + if candidate.head_hash in failed_hashes or candidate.base_hash in failed_hashes: + logger.warning( + "compare_snapshots: failed to fetch images for %s", + candidate.name, + extra={ + "head_artifact_id": head_artifact_id, + "head_hash": candidate.head_hash, + "base_hash": candidate.base_hash, + }, + ) + error_count += 1 + image_results[candidate.name] = { + "status": "errored", "head_hash": candidate.head_hash, "base_hash": candidate.base_hash, + "reason": "image_fetch_failed", + } + continue + head_data = fetch_cache[candidate.head_hash] + base_data = fetch_cache[candidate.base_hash] + total_fetched_bytes += len(head_data) + len(base_data) + total_fetched_count += 2 + diff_pairs.append((base_data, head_data)) + batch_names.append(candidate.name) + batch_hashes.append((candidate.head_hash, candidate.base_hash)) + + logger.info( + "compare_snapshots: running batch of %d pairs (%d unique hashes fetched)", + len(diff_pairs), + len(fetch_cache), + extra={"head_artifact_id": head_artifact_id, "names": batch_names}, + ) + diff_results = compare_images_batch(diff_pairs, server=server) + logger.info( + "compare_snapshots: batch complete, %d results", + len(diff_results), + extra={"head_artifact_id": head_artifact_id}, + ) + + for name, (head_hash, base_hash), diff_result in zip( + batch_names, batch_hashes, diff_results, strict=True + ): + if diff_result is None: + error_count += 1 + image_results[name] = { + "status": "errored", + "head_hash": head_hash, + "base_hash": base_hash, + "reason": "image_processing_failed", + } + continue + + stem = _image_name_to_path_stem(name) + diff_mask_key = ( + f"{image_key_prefix}/{head_artifact_id}/{base_artifact_id}/diff/{stem}.png" + ) + diff_mask_bytes = diff_result.diff_mask_png + logger.info( + "compare_snapshots: uploading mask for %s (%d bytes, changed_px=%d)", + name, + len(diff_mask_bytes), + diff_result.changed_pixels, + extra={ + "head_artifact_id": head_artifact_id, + "diff_mask_key": diff_mask_key, }, ) - error_count += 1 - image_results[candidate.name] = { - "status": "errored", - "head_hash": candidate.head_hash, - "base_hash": candidate.base_hash, - "reason": "image_fetch_failed", - } - continue - head_data = fetch_cache[candidate.head_hash] - base_data = fetch_cache[candidate.base_hash] - total_fetched_bytes += len(head_data) + len(base_data) - total_fetched_count += 2 - diff_pairs.append((base_data, head_data)) - batch_names.append(candidate.name) - batch_hashes.append((candidate.head_hash, candidate.base_hash)) + session.put(diff_mask_bytes, key=diff_mask_key, content_type="image/png") - logger.info( - "compare_snapshots: running batch of %d pairs (%d unique hashes fetched)", - len(diff_pairs), - len(fetch_cache), - extra={"head_artifact_id": head_artifact_id, "names": batch_names}, - ) - diff_results = compare_images_batch(diff_pairs) - logger.info( - "compare_snapshots: batch complete, %d results", - len(diff_results), - extra={"head_artifact_id": head_artifact_id}, - ) + diff_pct = ( + diff_result.changed_pixels / diff_result.total_pixels + if diff_result.total_pixels > 0 + else 0 + ) + effective_threshold = diff_threshold if diff_threshold is not None else 0.0 + is_changed = diff_pct > effective_threshold + if is_changed: + changed_count += 1 + else: + unchanged_count += 1 + + logger.debug( + "compare_snapshots: %s diff_pct=%.6f threshold=%s is_changed=%s pixels=%d/%d", + name, + diff_pct, + diff_threshold, + is_changed, + diff_result.changed_pixels, + diff_result.total_pixels, + extra={"head_artifact_id": head_artifact_id}, + ) + + diff_mask_image_id = f"{head_artifact_id}/{base_artifact_id}/diff/{stem}.png" - for name, (head_hash, base_hash), diff_result in zip( - batch_names, batch_hashes, diff_results, strict=True - ): - if diff_result is None: - error_count += 1 image_results[name] = { - "status": "errored", + "status": "changed" if is_changed else "unchanged", "head_hash": head_hash, "base_hash": base_hash, - "reason": "image_processing_failed", - } - continue - - stem = _image_name_to_path_stem(name) - diff_mask_key = ( - f"{image_key_prefix}/{head_artifact_id}/{base_artifact_id}/diff/{stem}.png" - ) - diff_mask_bytes = diff_result.diff_mask_png - logger.info( - "compare_snapshots: uploading mask for %s (%d bytes, changed_px=%d)", - name, - len(diff_mask_bytes), - diff_result.changed_pixels, - extra={ - "head_artifact_id": head_artifact_id, + "changed_pixels": diff_result.changed_pixels, + "total_pixels": diff_result.total_pixels, "diff_mask_key": diff_mask_key, - }, - ) - session.put(diff_mask_bytes, key=diff_mask_key, content_type="image/png") - - diff_pct = ( - diff_result.changed_pixels / diff_result.total_pixels - if diff_result.total_pixels > 0 - else 0 - ) - effective_threshold = diff_threshold if diff_threshold is not None else 0.0 - is_changed = diff_pct > effective_threshold - if is_changed: - changed_count += 1 - else: - unchanged_count += 1 - - logger.debug( - "compare_snapshots: %s diff_pct=%.6f threshold=%s is_changed=%s pixels=%d/%d", - name, - diff_pct, - diff_threshold, - is_changed, - diff_result.changed_pixels, - diff_result.total_pixels, - extra={"head_artifact_id": head_artifact_id}, - ) - - diff_mask_image_id = f"{head_artifact_id}/{base_artifact_id}/diff/{stem}.png" - - image_results[name] = { - "status": "changed" if is_changed else "unchanged", - "head_hash": head_hash, - "base_hash": base_hash, - "changed_pixels": diff_result.changed_pixels, - "total_pixels": diff_result.total_pixels, - "diff_mask_key": diff_mask_key, - "diff_mask_image_id": diff_mask_image_id, - "before_width": diff_result.before_width, - "before_height": diff_result.before_height, - "after_width": diff_result.after_width, - "after_height": diff_result.after_height, - "aligned_height": diff_result.aligned_height, - } + "diff_mask_image_id": diff_mask_image_id, + "before_width": diff_result.before_width, + "before_height": diff_result.before_height, + "after_width": diff_result.after_width, + "after_height": diff_result.after_height, + "aligned_height": diff_result.aligned_height, + } for name in sorted(added): image_results[name] = {"status": "added", "head_hash": head_by_name[name]}