Reject sync uploads once the build has failed

packages/core/src/percy.js

@@ -691,6 +691,9 @@ export class Percy {
             snapshotPromise[snapshot.name] = new Promise((resolve, reject) => {
               Object.assign(snapshot, { resolve, reject });
             });
+            // only the requested snapshot's promise is awaited, so a rejected extra
+            // (additional snapshots, array bodies) must not surface as unhandled
+            snapshotPromise[snapshot.name].catch(() => {});
           }
           // push each finished snapshot to the snapshots queue
           this.#snapshots.push(snapshot);
@@ -708,6 +711,9 @@ export class Percy {
       throw new Error('Not running');
     } else if (Array.isArray(options)) {
       return yieldAll(options.map(o => this.yield.upload(o)));
+    } else if (this.build?.error && callback && this.syncMode(options)) {
+      // a sync caller waits on the result, so fail it now instead of queueing it
+      throw new Error(this.build.error);
     }
     // validate comparison uploads and warn about any errors
 

packages/core/src/snapshot.js

@@ -420,6 +420,9 @@ export async function uploadSnapshotLog(percy, buildId, snapshotId, meta = {}) {
   }
 }
 
+// What a sync caller is told when its upload never ran and the build has not failed
+const SYNC_CANCELLED = 'Snapshot upload was cancelled before it finished, because Percy stopped or a newer snapshot with the same name replaced it';
+
 // Creates a snapshots queue that manages a Percy build and uploads snapshots.
 export function createSnapshotsQueue(percy) {
   let { concurrency } = percy.config.discovery;
@@ -542,7 +545,8 @@ export function createSnapshotsQueue(percy) {
 
       // Pushing to syncQueue, that will check for
       // snapshot processing status, and will resolve once done
-      if (snapshot.sync) {
+      // (only when someone waits on it: a job without callbacks would throw in the poller)
+      if (snapshot.sync && snapshot.resolve) {
         percy.log.info(`Waiting for snapshot '${name}' to be completed`, meta);
         const data = new JobData(response.data.id, null, snapshot.resolve, snapshot.reject);
         percy.syncQueue.push(data);
@@ -555,8 +559,12 @@ export function createSnapshotsQueue(percy) {
       let result = { ...snapshot, error };
       let { name, meta } = snapshot;
 
-      if (error.name === 'QueueClosedError') return result;
-      if (error.name === 'AbortError') return result;
+      // a closed or aborted queue never runs the task, so settle any sync caller
+      // here or the SDK request waiting on it hangs until its own timeout
+      if (error.name === 'QueueClosedError' || error.name === 'AbortError') {
+        if (snapshot.sync) snapshot.reject?.(new Error(build?.error || SYNC_CANCELLED));
+        return result;
+      }
 
       let failed = error.response?.statusCode === 422 && (
         error.response.body.errors.find(e => (
@@ -572,8 +580,10 @@ export function createSnapshotsQueue(percy) {
       let errors = error.response?.body?.errors;
       let duplicate = errors?.length > 1 && errors[1].detail.includes('must be unique');
       if (duplicate) {
+        let errMsg = `Ignored duplicate snapshot. ${errors[1].detail}`;
+        // no comparison was created, so a sync caller has nothing to wait for
+        if (snapshot.sync) snapshot.reject?.(new Error(errMsg));
         if (process.env.PERCY_IGNORE_DUPLICATES !== 'true') {
-          let errMsg = `Ignored duplicate snapshot. ${errors[1].detail}`;
           percy.log.warn(errMsg, meta);
 
           await percy.suggestionsForFix(errMsg, { snapshotLevel: true, snapshotName: name });
@@ -590,7 +600,7 @@ export function createSnapshotsQueue(percy) {
         { message: error?.message, meta }
       ];
       await percy.suggestionsForFix(snapshotErrors, { snapshotLevel: true, snapshotName: name });
-      if (snapshot.sync) snapshot.reject(error);
+      if (snapshot.sync) snapshot.reject?.(error);
       return result;
     });
 }

packages/core/test/percy.test.js

@@ -1305,6 +1305,181 @@ describe('Percy', () => {
       ]);
     });
 
+    it('does not wait on sync snapshot comparisons nobody is waiting for', async () => {
+      await percy.start();
+
+      // e.g. the comparison upload route, which passes no callbacks
+      await percy.upload({
+        name: 'Snapshot',
+        tag: { name: 'device' },
+        tiles: [{ content: 'foo' }],
+        sync: true
+      });
+
+      await percy.idle();
+      expect(api.requests['/comparisons/891011/finalize']).toHaveSize(1);
+      expect(percy.syncQueue.jobs).toHaveSize(0);
+      expect(logger.stderr).toEqual([]);
+    });
+
+    it('does not throw for failed sync uploads nobody is waiting for', async () => {
+      api.reply('/snapshots/4567/comparisons', () => [422, {
+        errors: [{ detail: 'Invalid comparison' }]
+      }]);
+
+      await percy.start();
+
+      await expectAsync(percy.upload({
+        name: 'Snapshot',
+        tag: { name: 'device' },
+        tiles: [{ content: 'foo' }],
+        sync: true
+      })).toBeResolved();
+
+      expect(logger.stderr).toEqual(jasmine.arrayContaining([
+        '[percy] Encountered an error uploading snapshot: Snapshot'
+      ]));
+      expect(logger.stderr.join()).not.toContain('TypeError');
+    });
+
+    it('rejects sync uploads once the build has failed upstream', async () => {
+      api.reply('/snapshots/4567/comparisons', () => [422, {
+        errors: [
+          { detail: 'Cannot create snapshot in failed builds' },
+          { detail: 'Build has failed', source: { pointer: '/data/attributes/build' } }
+        ]
+      }]);
+
+      await percy.start();
+
+      let upload = name => new Promise((resolve, reject) => {
+        percy.upload({
+          name,
+          tag: { name: 'device' },
+          tiles: [{ content: 'foo' }],
+          sync: true
+        }, { resolve, reject }).catch(reject);
+      });
+
+      // the upload the API rejects, then one sent after the build has failed
+      await expectAsync(upload('Snapshot 1')).toBeRejectedWithError('Build has failed');
+      await expectAsync(upload('Snapshot 2')).toBeRejectedWithError('Build has failed');
+    });
+
+    it('rejects sync uploads the API ignores as duplicates', async () => {
+      api.reply('/snapshots/4567/comparisons', () => [400, {
+        errors: [{ status: 'bad_request' },
+          { detail: 'The name of each snapshot must be unique ...' }]
+      }]);
+
+      await percy.start();
+
+      await expectAsync(new Promise((resolve, reject) => {
+        percy.upload({
+          name: 'Snapshot',
+          tag: { name: 'device' },
+          tiles: [{ content: 'foo' }],
+          sync: true
+        }, { resolve, reject }).catch(reject);
+      })).toBeRejectedWithError(
+        'Ignored duplicate snapshot. The name of each snapshot must be unique ...'
+      );
+    });
+
+    it('does not reject non-sync uploads once the build has failed upstream', async () => {
+      api.reply('/snapshots/4567/comparisons', () => [422, {
+        errors: [
+          { detail: 'Build has failed', source: { pointer: '/data/attributes/build' } }
+        ]
+      }]);
+
+      await percy.start();
+
+      let upload = name => percy.upload({
+        name,
+        tag: { name: 'device' },
+        tiles: [{ content: 'foo' }]
+      });
+
+      await expectAsync(upload('Snapshot 1')).toBeResolved();
+      expect(percy.build.error).toEqual('Build has failed');
+      await expectAsync(upload('Snapshot 2')).toBeResolved();
+
+      // sync without callbacks (e.g. the comparison upload route) nobody is waiting on
+      await expectAsync(percy.upload({
+        name: 'Snapshot 3',
+        tag: { name: 'device' },
+        tiles: [{ content: 'foo' }],
+        sync: true
+      })).toBeResolved();
+    });
+
+    it('rejects queued sync uploads when the build fails while they wait', async () => {
+      api.reply('/snapshots/4567/comparisons', req => {
+        // hold the failure so the other uploads queue behind it
+        req.delay = 500;
+        return [422, {
+          errors: [
+            { detail: 'Build has failed', source: { pointer: '/data/attributes/build' } }
+          ]
+        }];
+      });
+
+      await percy.start();
+
+      let upload = (name, callback = true) => new Promise((resolve, reject) => {
+        percy.upload({
+          name,
+          tag: { name: 'device' },
+          tiles: [{ content: 'foo' }],
+          sync: true
+        }, callback ? { resolve, reject } : null).then(resolve, reject);
+      });
+
+      // with a concurrency of 1, the first failure closes the queue while the rest
+      // wait (the next one may already be dequeued by then); every sync caller,
+      // sent or aborted while queued, must reject with the build error
+      let failed = [upload('Snapshot 1'), upload('Snapshot 2')];
+      let noCallback = upload('Snapshot 3', false);
+      let queued = upload('Snapshot 4');
+
+      await Promise.all([
+        ...[...failed, queued].map(u => expectAsync(u).toBeRejectedWithError('Build has failed')),
+        // a queued sync upload without callbacks is aborted without throwing
+        expectAsync(noCallback).toBeResolved()
+      ]);
+      // the uploads queued behind the first two (Snapshots 3 and 4) never reached the API
+      expect(api.requests['/snapshots/4567/comparisons'].length).toBeLessThan(3);
+    });
+
+    it('rejects queued sync uploads when percy is force stopped', async () => {
+      api.reply('/snapshots/4567/comparisons', req => {
+        // hold the first upload so the next one stays queued
+        req.delay = 500;
+        return [201, { data: { id: '891011' } }];
+      });
+
+      await percy.start();
+
+      let upload = name => new Promise((resolve, reject) => {
+        percy.upload({
+          name,
+          tag: { name: 'device' },
+          tiles: [{ content: 'foo' }],
+          sync: true
+        }, { resolve, reject }).catch(reject);
+      });
+
+      upload('Snapshot 1').catch(() => {});
+      let queued = upload('Snapshot 2');
+
+      let aborted = expectAsync(queued).toBeRejectedWithError(
+        'Snapshot upload was cancelled before it finished, because Percy stopped or a newer snapshot with the same name replaced it'
+      );
+      await percy.stop(true);
+      await aborted;
+    });
+
     it('should raise warning in case of upload command with sync', async () => {
       percy = new Percy({
         token: 'PERCY_TOKEN',