fix(preprod): Reap stuck PROCESSING snapshot comparisons#116708
Merged
Conversation
github-actions
Bot
added
the
Scope: Backend
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
When a compare_snapshots task is hard-killed mid-run (SIGKILL after a deploy/worker SIGTERM, or OOM), the cleanup that marks the comparison FAILED never runs, leaving the row stuck in PROCESSING. The task's retry guard only re-runs PENDING or FAILED rows, and the task is only enqueued on build upload or staff recompare, so a stuck comparison is skipped forever and spins indefinitely for the customer. Extend the existing hourly detect_expired_preprod_artifacts reaper to also mark PreprodSnapshotComparison rows stuck in PROCESSING for more than 30 minutes as FAILED with a TIMEOUT error code, mirroring the existing PreprodArtifactSizeComparison handling. This makes the row retryable again on the next upload or recompare. Co-Authored-By: Claude <noreply@anthropic.com>
NicoHinderling
force-pushed
the
nico/reap-stuck-snapshot-comparisons
branch
from
5775663 to
bca2b07
Compare
mtopo27
approved these changes
Jun 2, 2026
NicoHinderling
added a commit
that referenced
this pull request
Jun 2, 2026
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. --------- Co-authored-by: Claude <noreply@anthropic.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extend the hourly
detect_expired_preprod_artifactsreaper to also markPreprodSnapshotComparisonrows stuck inPROCESSINGfor more than 30 minutes asFAILED+TIMEOUT, mirroring the existingPreprodArtifactSizeComparisonhandling.When a
compare_snapshotstask is hard-killed mid-run — SIGKILL after a deploy/worker SIGTERM, or OOM — theexcept BaseExceptioncleanup that would set the row toFAILEDnever runs, so the row is left stuck inPROCESSING. The task's retry guard only re-runsPENDING/FAILEDrows, and the task is only enqueued on a build upload or a staff recompare — never on page load/poll. So a stuck comparison is skipped forever (skipping, comparison not in retryable state (state=1)) and spins indefinitely for the customer.Marking it
FAILEDmakes the row retryable again on the next upload or recompare, and turns the perpetual spinner into an honest failed state. This matches how the siblingPreprodArtifactSizeComparisonPROCESSING rows are already recovered.This is fix #3 of a two-PR Graphite stack. The follow-up PR (#1) parallelizes the odiff comparison phase so large comparisons finish well within the deadline — that's the durable fix for the underlying slowness; this PR stops affected comparisons from getting permanently wedged in the meantime.