Skip to content

feat(preprod): Run snapshot odiff comparisons in parallel#116712

Merged
NicoHinderling merged 2 commits into
masterfrom
nico/parallelize-snapshot-odiff
Jun 2, 2026
Merged

feat(preprod): Run snapshot odiff comparisons in parallel#116712
NicoHinderling merged 2 commits into
masterfrom
nico/parallelize-snapshot-odiff

Conversation

@NicoHinderling

Copy link
Copy Markdown
Contributor

Parallelize the preprod snapshot odiff comparison phase. The single serial OdiffServer is replaced with a pool of N persistent servers (tunable via the new preprod.snapshots.odiff-worker-count option, default 4) driven by a ContextPropagatingThreadPoolExecutor. The per-batch fetch → compare → upload work is extracted into _process_batch, which checks a server out of a queue.Queue, runs the batch, uploads its diff masks, and returns partial results that are merged single-threaded in the main loop.

Why: for large builds the comparison diffs thousands of changed image pairs one at a time against one node process, taking 3+ minutes — long enough to hit the task's 300s processing_deadline (ProcessingDeadlineExceeded) or be interrupted by a deploy SIGTERM. A real build (~6,000 changed pairs across 431 batches) was timing out this way. Spreading the batches across N servers brings it comfortably under the deadline.

Notes for reviewers:

  • Concurrency safety: each OdiffServer is only ever used by one thread (checked out of the pool); the objectstore Session is thread-safe for get/put; each batch's compare_images_batch uses its own tempdir, so there are no cross-thread filename collisions. All result accumulation happens in the main thread — workers mutate no shared state.
  • A mid-batch failure is isolated: already-processed pairs are preserved and only the unprocessed ones are marked errored, so a single bad batch doesn't fail the whole comparison.
  • Memory scales with N (each worker can hold a pair's decoded/padded images), which is why N is an FLAG_AUTOMATOR_MODIFIABLE option — it can be dialed down in prod without a deploy. MAX_DIFF_PIXELS still caps any single pair.
  • Adds a compare_snapshots: odiff phase complete summary log (duration, throughput, slowest batch, worker count) for post-deploy diagnosis, and the first end-to-end tests for compare_snapshots (which previously had no task-level coverage).

This is the durable follow-up to #116708 (which stopped killed comparisons from getting stuck in PROCESSING); together they address the timeout end-to-end.

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Jun 2, 2026
@NicoHinderling
NicoHinderling marked this pull request as ready for review June 2, 2026 19:39
@NicoHinderling
NicoHinderling requested review from a team as code owners June 2, 2026 19:39

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 2c17f56df69701ff004d9e5605a26c624f08a12a. Configure here.

Comment thread src/sentry/preprod/snapshots/tasks.py Outdated
@NicoHinderling
NicoHinderling force-pushed the nico/parallelize-snapshot-odiff branch 2 times, most recently from 73ee96e to d1ebe08 Compare June 2, 2026 19:48
NicoHinderling and others added 2 commits June 2, 2026 15:07
Replace the single serial OdiffServer with a pool of N persistent servers
(tunable via preprod.snapshots.odiff-worker-count, default 4) driven by a
thread pool. The per-batch fetch -> compare -> upload work is extracted into
_process_batch, which returns partial results merged in the main thread; a
mid-batch failure is isolated (only its unprocessed pairs are marked errored)
rather than aborting the whole comparison.

This cuts a multi-minute comparison of thousands of changed pairs to well
under the processing deadline, so large comparisons stop getting killed. The
deadline is also raised from 300s to 600s as an extra safety margin. Adds an
odiff phase-summary log (duration, throughput, slowest batch, worker count)
for post-deploy diagnosis, and the first end-to-end tests for compare_snapshots.

Co-Authored-By: Claude <[email protected]>
@NicoHinderling
NicoHinderling force-pushed the nico/parallelize-snapshot-odiff branch from 17680b3 to 181f16e Compare June 2, 2026 22:07
@NicoHinderling
NicoHinderling merged commit 8fcc162 into master Jun 2, 2026
84 checks passed
@NicoHinderling
NicoHinderling deleted the nico/parallelize-snapshot-odiff branch June 2, 2026 22:24
@NicoHinderling NicoHinderling added the Trigger: Revert Add to a merged PR to revert it (skips CI) label Jun 3, 2026
@getsentry-bot

Copy link
Copy Markdown
Contributor

PR reverted: bce98e2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components Trigger: Revert Add to a merged PR to revert it (skips CI)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants