From 04b218d56f8d6d3d709121cfb1e9d7ff4903523e Mon Sep 17 00:00:00 2001 From: Shivanshu07 Date: Tue, 16 Jun 2026 19:07:50 +0530 Subject: [PATCH 1/3] PER-9142: fail snapshot when comparison tile upload fails after retries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a comparison tile fails to upload/verify after all retries, verify() throws — but sendComparison aborted before finalizeComparison, leaving the comparison unfinalized. The server's build-cleanup loop then swept it and surfaced a misleading "render_timeout" instead of a clean per-snapshot failure. Finalize the comparison on tile-upload failure so the server resolves it deterministically (marking that snapshot failed), then re-throw so the snapshot is reported failed to the caller. The build continues; other snapshots are unaffected. Co-Authored-By: Claude Opus 4.8 --- packages/client/src/client.js | 17 +++++++++++++++- packages/client/test/client.test.js | 31 +++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/packages/client/src/client.js b/packages/client/src/client.js index 43b21614d..f5e20db2c 100644 --- a/packages/client/src/client.js +++ b/packages/client/src/client.js @@ -879,7 +879,22 @@ export class PercyClient { } let snapshot = await this.createSnapshot(buildId, options); let comparison = await this.createComparison(snapshot.data.id, options); - await this.uploadComparisonTiles(comparison.data.id, options.tiles); + try { + await this.uploadComparisonTiles(comparison.data.id, options.tiles); + } catch (error) { + // A tile failed to upload/verify after all retries (see #verify). Finalize + // the comparison anyway so the server resolves it deterministically and + // marks this snapshot failed — an unfinalized comparison is otherwise swept + // by the build-cleanup retry loop and surfaces as a misleading render_timeout. + // Re-throw afterwards so the snapshot is reported as failed to the caller. + 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; + } this.log.debug(`Created comparison: ${comparison.data.id} ${options.tag.name}`, meta); await this.finalizeComparison(comparison.data.id); this.log.debug(`Finalized comparison: ${comparison.data.id} ${options.tag.name}`, meta); diff --git a/packages/client/test/client.test.js b/packages/client/test/client.test.js index 84f0e3b0a..49223f1f3 100644 --- a/packages/client/test/client.test.js +++ b/packages/client/test/client.test.js @@ -2413,6 +2413,37 @@ describe('PercyClient', () => { }); }); + describe('when a tile fails to upload after all retries', () => { + it('finalizes the comparison and rethrows so the snapshot is marked failed', async () => { + spyOn(client, 'uploadComparisonTiles').and.callFake(() => + Promise.reject(new Error('Uploading comparison tile failed'))); + spyOn(client, 'finalizeComparison').and.callThrough(); + + await expectAsync(client.sendComparison(123, { + name: 'test snapshot name', + tag: { name: 'test tag' }, + tiles: [{ sha: 'abcd' }] + })).toBeRejectedWithError('Uploading comparison tile failed'); + + // the comparison is still finalized so the server fails it deterministically + expect(client.finalizeComparison).toHaveBeenCalled(); + expect(api.requests['/comparisons/891011/finalize']).toBeDefined(); + }); + + it('still rethrows the original error when finalizing the failed comparison also fails', async () => { + spyOn(client, 'uploadComparisonTiles').and.callFake(() => + Promise.reject(new Error('Uploading comparison tile failed'))); + spyOn(client, 'finalizeComparison').and.callFake(() => + Promise.reject(new Error('finalize boom'))); + + await expectAsync(client.sendComparison(123, { + name: 'test snapshot name', + tag: { name: 'test tag' }, + tiles: [{ sha: 'abcd' }] + })).toBeRejectedWithError('Uploading comparison tile failed'); + }); + }); + describe('when labels are provided on a POA comparison', () => { beforeEach(async () => { await client.sendComparison(123, { From 819bf71547d252b35d4a1f8991c5962498589d98 Mon Sep 17 00:00:00 2001 From: Shivanshu07 Date: Wed, 17 Jun 2026 09:57:50 +0530 Subject: [PATCH 2/3] =?UTF-8?q?PER-9142:=20address=20review=20=E2=80=94=20?= =?UTF-8?q?null-safe=20finalize=20log=20+=20assert=20finalize=20attempted?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Coerce finalizeError defensively in the inner catch so a non-Error rejection can't throw and mask the original tile-upload error being re-thrown. - Assert finalizeComparison was still called in the finalize-also-fails test. Co-Authored-By: Claude Opus 4.8 --- packages/client/src/client.js | 4 +++- packages/client/test/client.test.js | 3 +++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/client/src/client.js b/packages/client/src/client.js index f5e20db2c..24ac6d9b2 100644 --- a/packages/client/src/client.js +++ b/packages/client/src/client.js @@ -891,7 +891,9 @@ export class PercyClient { try { await this.finalizeComparison(comparison.data.id); } catch (finalizeError) { - this.log.debug(`Failed to finalize failed comparison ${comparison.data.id}: ${finalizeError.message}`, meta); + // Coerce defensively: a non-Error rejection (null/undefined/string) must not + // throw here and mask the original tile-upload error we re-throw below. + this.log.debug(`Failed to finalize failed comparison ${comparison.data.id}: ${finalizeError?.message || finalizeError}`, meta); } throw error; } diff --git a/packages/client/test/client.test.js b/packages/client/test/client.test.js index 49223f1f3..ad52fc347 100644 --- a/packages/client/test/client.test.js +++ b/packages/client/test/client.test.js @@ -2441,6 +2441,9 @@ describe('PercyClient', () => { tag: { name: 'test tag' }, tiles: [{ sha: 'abcd' }] })).toBeRejectedWithError('Uploading comparison tile failed'); + + // the finalize attempt was still made (and its own failure swallowed) + expect(client.finalizeComparison).toHaveBeenCalled(); }); }); From 21f9aaab73e503fcc11c2da2e81574b9dfa28490 Mon Sep 17 00:00:00 2001 From: Shivanshu07 Date: Wed, 17 Jun 2026 10:13:04 +0530 Subject: [PATCH 3/3] PER-9142: restore 100% branch coverage in sendComparison failure path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The defensive `finalizeError?.message || finalizeError` introduced a logical-or branch whose fallback side the tests never exercised, tripping the repo's 100% branch-coverage gate (client.js:896). Use plain template coercion `${finalizeError}` — still null-safe, no added branch — and drop the now-inconsistent `options.tag?.name` optional chain in favour of `options.tag.name` to match the surrounding logs. Co-Authored-By: Claude Opus 4.8 --- packages/client/src/client.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/client/src/client.js b/packages/client/src/client.js index 24ac6d9b2..bd492e8b0 100644 --- a/packages/client/src/client.js +++ b/packages/client/src/client.js @@ -887,13 +887,13 @@ export class PercyClient { // marks this snapshot failed — an unfinalized comparison is otherwise swept // by the build-cleanup retry loop and surfaces as a misleading render_timeout. // Re-throw afterwards so the snapshot is reported as failed to the caller. - this.log.error(`Failed to upload tiles for comparison: ${comparison.data.id} ${options.tag?.name}`, meta); + 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) { - // Coerce defensively: a non-Error rejection (null/undefined/string) must not - // throw here and mask the original tile-upload error we re-throw below. - this.log.debug(`Failed to finalize failed comparison ${comparison.data.id}: ${finalizeError?.message || finalizeError}`, meta); + // String-coerce the rejection so a non-Error value (null/undefined/string) + // can't throw here and mask the original tile-upload error we re-throw below. + this.log.debug(`Failed to finalize failed comparison ${comparison.data.id}: ${finalizeError}`, meta); } throw error; }