PER-9142: fail snapshot when comparison tile upload fails after retries

[Shivanshu-07]

Problem

For App Percy / Percy-on-Automate comparisons, client.verify() retries a tile up to 20× and then throws Uploading comparison tile failed (intended: "the comparison will be failed even if 1 tile gets failed").

But that throw happens inside uploadComparisonTiles, before finalizeComparison:

// sendComparison (before)
await this.uploadComparisonTiles(comparison.data.id, options.tiles); // verify throws here
await this.finalizeComparison(comparison.data.id);                   // never reached

So the comparison is left unfinalized. The server's build-cleanup loop then reinserts the stuck comparison up to 10× and finally marks the build render_timeout — a misleading failure reason, and the snapshot is never cleanly reported as failed. (This is the CLI-side counterpart of the App Automate dropped-tile issue investigated in PER-9142 / browserstack/mobile-common#1197.)

Fix

On tile-upload failure, finalize the comparison anyway so the server resolves it deterministically (and marks that snapshot failed) instead of leaving it to be swept as a render_timeout, then re-throw so the snapshot is reported failed to the caller:

try {
  await this.uploadComparisonTiles(comparison.data.id, options.tiles);
} catch (error) {
  this.log.error(`Failed to upload tiles for comparison: ${comparison.data.id} ${options.tag?.name}`, meta);
  try { await this.finalizeComparison(comparison.data.id); }
  catch (finalizeError) { this.log.debug(`Failed to finalize failed comparison ${comparison.data.id}: ${finalizeError.message}`, meta); }
  throw error;
}

• The re-throw routes to the existing snapshot error handler in core/src/snapshot.js, which logs the failure and (for sync snapshots) rejects that snapshot's promise — so failure is surfaced per-snapshot.
• The build is not failed and other snapshots continue (the throw is not a 422 build error).
• If the finalize-on-failure itself errors, the original tile-upload error is still re-thrown.

Tests

Added to #sendComparison():

• finalizes the comparison and rethrows so the snapshot is marked failed
• still rethrows the original error when finalizing the failed comparison also fails

yarn test --node (client) → all sendComparison specs green; existing specs unaffected. ESLint clean on changed files. (The 2 failures in test/unit/proxy.test.js on my machine are pre-existing/environmental — a local proxy + a Node-20 Invalid URL message-format difference — unrelated to this change.)

Related

• browserstack/mobile-common#1197 — terminal-side fix (verify the tile actually reached GCS before returning the sha).
• Together: the terminal stops handing back shas for un-uploaded tiles, and the CLI fails the snapshot cleanly if a tile still can't be verified.

🤖 Generated with Claude Code

[Shivanshu-07]

Claude Code PR Review

PR: #2292 • Head: 819bf71 • Reviewers: stack:code-reviewer

Summary

Fails the affected snapshot when a comparison tile upload/verify fails after all retries: sendComparison now finalizes the comparison on tile-upload failure (so the server resolves it deterministically as failed instead of it being swept into a misleading render_timeout) and re-throws so the snapshot is reported failed.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No secrets touched.
High	Security	Authentication/authorization checks present	N/A	No auth surface changed.
High	Security	Input validation and sanitization	N/A	No new external input.
High	Security	No IDOR — resource ownership validated	N/A	Uses existing comparison id from API response.
High	Security	No SQL injection (parameterized queries)	N/A	No DB access.
High	Correctness	Logic is correct, handles edge cases	Pass	try/catch finalizes then re-throws; happy path unchanged.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Original error always re-thrown; inner finalize failure logged + null-safe.
High	Correctness	No race conditions or concurrency issues	Pass	Sequential awaits; no shared state.
Medium	Testing	New code has corresponding tests	Pass	Two specs: finalize+rethrow, and finalize-also-fails+rethrow.
Medium	Testing	Error paths and edge cases tested	Pass	Both failure branches asserted (incl. finalize attempted).
Medium	Testing	Existing tests still pass (no regressions)	Pass	client node suite green except 2 pre-existing/environmental proxy.test.js failures.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	One extra finalize call only on the failure path.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable.
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors existing finalize/log patterns in client.js.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to sendComparison tile-failure handling.
Low	Quality	Meaningful names, no dead code	Pass	—
Low	Quality	Comments explain why, not what	Pass	Comment explains the render_timeout rationale.
Low	Quality	No unnecessary dependencies added	Pass	None.

Findings

Reviewer raised no Critical/High issues. Two actionable items were flagged and have been addressed in 819bf71:

• File: packages/client/src/client.js (inner finalize catch)

• Severity: Medium

• Reviewer: stack:code-reviewer

• Issue: finalizeError.message could throw a TypeError on a non-Error rejection and mask the original tile-upload error.

• Resolution: Coerced defensively to ${finalizeError?.message || finalizeError}.

• File: packages/client/test/client.test.js (finalize-also-fails test)

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: Did not assert finalizeComparison was attempted.

• Resolution: Added expect(client.finalizeComparison).toHaveBeenCalled().

Remaining reviewer notes (optional-chaining consistency in the new log line vs neighbouring lines; redundant API-request assertion in test 1) are Low/cosmetic and left as-is.

---

Verdict: PASS