♻ Improve build failure handling (#805)

When a build fails, the error is logged and the internal queues closed. When attempting to push or
upload new snapshots, the queue throws a `Closed` error. The `sdk-utils` package checks if any API
error have the message `Closed` to disable the local SDK. These `Closed` errors can occasionally
still show up in the logged output which is confusing without any other context (context is usually
further up in the logs anyway).

This handling has now been updated to better recommunicate the original error. When builds fail, the
reason is cached so other methods can rethrow it rather than let the closed error bubble. Our tests
no longer exercised the closed error, so it was made to silently fail for race condition
protection. The CLI API was updated to include build details with errors, and the `sdk-utils`
package updated to check for the build errors rather than a `Closed` error message.

packages/core/src/api.js

@@ -18,6 +18,7 @@ export function createPercyServer(percy, port) {
 
       // return json errors
       return next().catch(e => res.json(e.status ?? 500, {
+        build: percy.build,
         error: e.message,
         success: false
       }));

packages/core/src/percy.js

@@ -168,7 +168,8 @@ export class Percy {
     // handle deferred build errors
     if (this.deferUploads) {
       buildTask.catch(err => {
-        this.log.error('Failed to create build');
+        this.build = { error: 'Failed to create build' };
+        this.log.error(this.build.error);
         this.log.error(err);
         this.close();
       });
@@ -281,7 +282,7 @@ export class Percy {
     if (this.build?.failed) {
       // do not finalize failed builds
       this.log.warn(`Build #${this.build.number} failed: ${this.build.url}`, meta);
-    } else if (this.build) {
+    } else if (this.build?.id) {
       // finalize the build
       await this.client.finalizeBuild(this.build.id);
       this.log.info(`Finalized build #${this.build.number}: ${this.build.url}`, meta);
@@ -311,10 +312,9 @@ export class Percy {
   snapshot(options) {
     if (this.readyState !== 1) {
       throw new Error('Not running');
-    }
-
-    // handle multiple snapshots
-    if (Array.isArray(options)) {
+    } else if (this.build?.error) {
+      throw new Error(this.build.error);
+    } else if (Array.isArray(options)) {
       return Promise.all(options.map(o => this.snapshot(o)));
     }
 
@@ -351,6 +351,10 @@ export class Percy {
 
   // Queues a snapshot upload with the provided options
   _scheduleUpload(name, options) {
+    if (this.build?.error) {
+      throw new Error(this.build.error);
+    }
+
     return this.#uploads.push(`upload/${name}`, async () => {
       try {
         /* istanbul ignore if: useful for other internal packages */
@@ -367,6 +371,7 @@ export class Percy {
 
         // build failed at some point, stop accepting snapshots
         if (failed) {
+          this.build.error = failed.detail;
           this.build.failed = true;
           this.close();
         }

packages/core/src/queue.js

@@ -15,9 +15,8 @@ export class Queue {
   }
 
   push(id, callback, priority) {
-    if (this.closed && !id.startsWith('@@/')) {
-      throw new Error('Closed');
-    }
+    /* istanbul ignore next: race condition paranoia */
+    if (this.closed && !id.startsWith('@@/')) return;
 
     this.cancel(id);
 

packages/core/test/api.test.js

@@ -155,15 +155,23 @@ describe('API Server', () => {
 
     let [data, res] = await request('/percy/snapshot', 'POST', true);
     expect(res.statusCode).toBe(500);
-    expect(data).toEqual({ success: false, error: 'test error' });
+    expect(data).toEqual({
+      build: percy.build,
+      error: 'test error',
+      success: false
+    });
   });
 
   it('returns a 404 for any other endpoint', async () => {
     await percy.start();
 
     let [data, res] = await request('/foobar', true);
     expect(res.statusCode).toBe(404);
-    expect(data).toEqual({ success: false, error: 'Not Found' });
+    expect(data).toEqual({
+      build: percy.build,
+      error: 'Not Found',
+      success: false
+    });
   });
 
   it('facilitates logger websocket connections', async () => {

packages/core/test/percy.test.js

@@ -363,7 +363,7 @@ describe('Percy', () => {
       expect(() => percy.snapshot({
         name: 'Snapshot 2',
         url: 'http://localhost:8000'
-      })).toThrowError('Closed');
+      })).toThrowError('Failed to create build');
 
       expect(logger.stdout).toEqual([
         '[percy] Percy has started!',
@@ -374,6 +374,47 @@ describe('Percy', () => {
         '[percy] Error: build error'
       ]);
     });
+
+    it('stops accepting snapshots when an in-progress build fails', async () => {
+      mockAPI.reply('/builds/123/snapshots', () => [422, {
+        errors: [{
+          detail: 'Build has failed',
+          source: { pointer: '/data/attributes/build' }
+        }]
+      }]);
+
+      // create a new instance with default concurrency
+      percy = new Percy({ token: 'PERCY_TOKEN', snapshot: { widths: [1000] } });
+      await percy.start();
+
+      await Promise.all([
+        // upload will eventually fail
+        percy.snapshot({
+          url: 'http://localhost:8000/snapshot-1'
+        }),
+        // should not upload
+        percy.snapshot({
+          url: 'http://localhost:8000/snapshot-2',
+          // delay this snapshot so the first upload can fail
+          waitForTimeout: 100
+        })
+      ]);
+
+      await percy.idle();
+
+      expect(logger.stderr).toEqual(jasmine.arrayContaining([
+        '[percy] Encountered an error uploading snapshot: /snapshot-1',
+        '[percy] Error: Build has failed'
+      ]));
+
+      expect(mockAPI.requests['/builds/123/snapshots'].length).toEqual(1);
+
+      // stops accepting snapshots
+      expect(() => percy.snapshot({
+        name: 'Snapshot 2',
+        url: 'http://localhost:8000'
+      })).toThrowError('Build has failed');
+    });
   });
 
   describe('#stop()', () => {

packages/core/test/snapshot.test.js

@@ -45,18 +45,16 @@ describe('Snapshot', () => {
   });
 
   it('warns when missing additional snapshot names', async () => {
-    percy.close(); // close queues so snapshots fail
-
-    expect(() => percy.snapshot({
-      url: 'http://foo',
+    await percy.snapshot({
+      url: 'http://localhost:8000',
       additionalSnapshots: [{
-        waitForTimeout: 1000
+        waitForTimeout: 10
       }, {
         name: 'nombre',
         suffix: ' - 1',
-        waitForTimeout: 1000
+        waitForTimeout: 10
       }]
-    })).toThrow();
+    });
 
     expect(logger.stderr).toEqual([
       '[percy] Invalid snapshot options:',
@@ -65,10 +63,8 @@ describe('Snapshot', () => {
     ]);
   });
 
-  it('warns when providing conflicting options', () => {
-    percy.close(); // close queues so snapshots fail
-
-    expect(() => percy.snapshot({
+  it('warns when providing conflicting options', async () => {
+    await percy.snapshot({
       url: 'http://a',
       domSnapshot: 'b',
       waitForTimeout: 3,
@@ -77,7 +73,7 @@ describe('Snapshot', () => {
       additionalSnapshots: [
         { prefix: 'f' }
       ]
-    })).toThrow();
+    });
 
     expect(logger.stderr).toEqual([
       '[percy] Invalid snapshot options:',
@@ -88,10 +84,8 @@ describe('Snapshot', () => {
     ]);
   });
 
-  it('warns if options are invalid', () => {
-    percy.close(); // close queues so snapshots fail
-
-    expect(() => percy.snapshot({
+  it('warns if options are invalid', async () => {
+    await percy.snapshot({
       name: 'invalid snapshot',
       url: 'http://localhost:8000',
       widths: ['not-a-width'],
@@ -103,7 +97,7 @@ describe('Snapshot', () => {
           'finally.a-real.hostname.org'
         ]
       }
-    })).toThrow();
+    });
 
     expect(logger.stderr).toEqual([
       '[percy] Invalid snapshot options:',
@@ -114,12 +108,12 @@ describe('Snapshot', () => {
     ]);
   });
 
-  it('warns on deprecated options', () => {
-    percy.close(); // close queues so snapshots fail
-
-    expect(() => percy.snapshot({ url: 'http://a', requestHeaders: { foo: 'bar' } })).toThrow();
-    expect(() => percy.snapshot({ url: 'http://b', authorization: { username: 'foo' } })).toThrow();
-    expect(() => percy.snapshot({ url: 'http://c', snapshots: [{ name: 'foobar' }] })).toThrow();
+  it('warns on deprecated options', async () => {
+    await percy.snapshot([
+      { url: 'http://localhost:8000/a', requestHeaders: { foo: 'bar' } },
+      { url: 'http://localhost:8000/b', authorization: { username: 'foo' } },
+      { url: 'http://localhost:8000/c', snapshots: [{ name: 'foobar' }] }
+    ]);
 
     expect(logger.stderr).toEqual([
       '[percy] Warning: The snapshot option `requestHeaders` ' +

packages/sdk-utils/src/post-snapshot.js

@@ -7,7 +7,7 @@ export async function postSnapshot(options, params) {
   let query = params ? `?${new URLSearchParams(params)}` : '';
 
   await request.post(`/percy/snapshot${query}`, options).catch(err => {
-    if (err.response && err.message === 'Closed') {
+    if (err.response?.body?.build?.error) {
       percy.enabled = false;
     } else {
       throw err;

packages/sdk-utils/test/helpers.js

@@ -17,7 +17,7 @@ const helpers = {
   teardown: () => helpers.call('server.close'),
   getRequests: () => helpers.call('server.requests'),
   testReply: (path, reply) => helpers.call('server.reply', path, reply),
-  testFailure: (path, error) => helpers.call('server.test.failure', path, error),
+  testFailure: (...args) => helpers.call('server.test.failure', ...args),
   testError: path => helpers.call('server.test.error', path),
   testSerialize: fn => !fn
     ? helpers.call('server.test.serialize') // get

packages/sdk-utils/test/index.test.js

@@ -118,7 +118,8 @@ describe('SDK Utils', () => {
     });
 
     it('returns false if a snapshot is sent when the API is closed', async () => {
-      await helpers.testFailure('/percy/snapshot', 'Closed');
+      let error = 'Build failed';
+      await helpers.testFailure('/percy/snapshot', error, { build: { error } });
       await expectAsync(isPercyEnabled()).toBeResolvedTo(true);
       await expectAsync(utils.postSnapshot({})).toBeResolved();
       await expectAsync(isPercyEnabled()).toBeResolvedTo(false);
@@ -162,8 +163,9 @@ describe('SDK Utils', () => {
     });
 
     it('disables snapshots when the API is closed', async () => {
+      let error = 'Build failed';
       utils.percy.enabled = true;
-      await helpers.testFailure('/percy/snapshot', 'Closed');
+      await helpers.testFailure('/percy/snapshot', error, { build: { error } });
       await expectAsync(postSnapshot({})).toBeResolved();
       expect(utils.percy.enabled).toEqual(false);
     });

packages/sdk-utils/test/server.js

@@ -72,8 +72,8 @@ function context() {
       test: {
         get serialize() { return serializeDOM; },
         set serialize(fn) { return (serializeDOM = fn); },
-        failure: (path, error) => ctx.server.reply(path, () => (
-          [500, 'application/json', { success: false, error }])),
+        failure: (path, error, o) => ctx.server.reply(path, () => (
+          [500, 'application/json', { success: false, error, ...o }])),
         error: path => ctx.server.reply(path, r => r.connection.destroy()),
         remote: () => (allowSocketConnections = true)
       }