✨ Discovery retry support + fixed a race condition in network handling

packages/core/src/config.js

@@ -234,6 +234,10 @@ export const configSchema = {
         type: 'integer',
         minimum: 1
       },
+      retry: {
+        type: 'boolean',
+        default: false
+      },
       launchOptions: {
         type: 'object',
         additionalProperties: false,
@@ -281,7 +285,8 @@ export const snapshotSchema = {
             captureMockedServiceWorker: { $ref: '/config/discovery#/properties/captureMockedServiceWorker' },
             captureSrcset: { $ref: '/config/discovery#/properties/captureSrcset' },
             userAgent: { $ref: '/config/discovery#/properties/userAgent' },
-            devicePixelRatio: { $ref: '/config/discovery#/properties/devicePixelRatio' }
+            devicePixelRatio: { $ref: '/config/discovery#/properties/devicePixelRatio' },
+            retry: { $ref: '/config/discovery#/properties/retry' }
           }
         }
       },

packages/core/src/discovery.js

@@ -8,7 +8,8 @@ import {
   createPercyCSSResource,
   createLogResource,
   yieldAll,
-  snapshotLogName
+  snapshotLogName,
+  withRetries
 } from './utils.js';
 
 // Logs verbose debug logs detailing various snapshot options.
@@ -301,37 +302,47 @@ export function createDiscoveryQueue(percy) {
       let assetDiscoveryPageEnableJS = (snapshot.cliEnableJavaScript && !snapshot.domSnapshot) || (snapshot.enableJavaScript ?? !snapshot.domSnapshot);
 
       percy.log.debug(`Asset discovery Browser Page enable JS: ${assetDiscoveryPageEnableJS}`);
-      // create a new browser page
-      let page = yield percy.browser.page({
-        enableJavaScript: assetDiscoveryPageEnableJS,
-        networkIdleTimeout: snapshot.discovery.networkIdleTimeout,
-        requestHeaders: snapshot.discovery.requestHeaders,
-        authorization: snapshot.discovery.authorization,
-        userAgent: snapshot.discovery.userAgent,
-        captureMockedServiceWorker: snapshot.discovery.captureMockedServiceWorker,
-        meta: snapshot.meta,
-
-        // enable network inteception
-        intercept: {
-          enableJavaScript: snapshot.enableJavaScript,
-          disableCache: snapshot.discovery.disableCache,
-          allowedHostnames: snapshot.discovery.allowedHostnames,
-          disallowedHostnames: snapshot.discovery.disallowedHostnames,
-          getResource: u => snapshot.resources.get(u) || cache.get(u),
-          saveResource: r => { snapshot.resources.set(r.url, r); if (!r.root) { cache.set(r.url, r); } }
-        }
-      });
 
-      try {
-        yield* captureSnapshotResources(page, snapshot, {
-          captureWidths: !snapshot.domSnapshot && percy.deferUploads,
-          capture: callback,
-          captureForDevices: percy.deviceDetails || []
+      await withRetries(async function*() {
+        // create a new browser page
+        let page = yield percy.browser.page({
+          enableJavaScript: assetDiscoveryPageEnableJS,
+          networkIdleTimeout: snapshot.discovery.networkIdleTimeout,
+          requestHeaders: snapshot.discovery.requestHeaders,
+          authorization: snapshot.discovery.authorization,
+          userAgent: snapshot.discovery.userAgent,
+          captureMockedServiceWorker: snapshot.discovery.captureMockedServiceWorker,
+          meta: snapshot.meta,
+
+          // enable network inteception
+          intercept: {
+            enableJavaScript: snapshot.enableJavaScript,
+            disableCache: snapshot.discovery.disableCache,
+            allowedHostnames: snapshot.discovery.allowedHostnames,
+            disallowedHostnames: snapshot.discovery.disallowedHostnames,
+            getResource: u => snapshot.resources.get(u) || cache.get(u),
+            saveResource: r => { snapshot.resources.set(r.url, r); if (!r.root) { cache.set(r.url, r); } }
+          }
         });
-      } finally {
-        // always close the page when done
-        await page.close();
-      }
+
+        try {
+          yield* captureSnapshotResources(page, snapshot, {
+            captureWidths: !snapshot.domSnapshot && percy.deferUploads,
+            capture: callback,
+            captureForDevices: percy.deviceDetails || []
+          });
+        } finally {
+          // always close the page when done
+          await page.close();
+        }
+      }, {
+        count: snapshot.discovery.retry ? 3 : 1,
+        onRetry: () => {
+          percy.log.info(`Retrying snapshot: ${snapshotLogName(snapshot.name, snapshot.meta)}`, snapshot.meta);
+        },
+        signal: snapshot._ctrl.signal,
+        throwOn: ['AbortError']
+      });
     })
     .handle('error', ({ name, meta }, error) => {
       if (error.name === 'AbortError' && queue.readyState < 3) {

packages/core/src/network.js

@@ -31,6 +31,7 @@ export class Network {
   #requests = new Map();
   #authentications = new Set();
   #aborted = new Set();
+  #finishedUrls = new Set();
 
   constructor(page, options) {
     this.page = page;
@@ -86,6 +87,13 @@ export class Network {
       }
 
       requests = Array.from(this.#requests.values()).filter(filter);
+      // remove requests which are finished at least once
+      // this happens when same request is made multiple times by browser in parallel and one of
+      // them gets stuck in pending state in browser [ need to debug why ]. So we dont receive
+      // loadingFinished event, causing it to show up in Active requests, but we can only store one
+      // response per url so as long as we have captured one, we dont care about other such requests
+      requests = requests.filter((req) => !this.#finishedUrls.has(req.url));
+
       return requests.length === 0;
     }, {
       timeout: Network.TIMEOUT,
@@ -131,9 +139,10 @@ export class Network {
   }
 
   // Called when a request should be removed from various trackers
-  _forgetRequest({ requestId, interceptId }, keepPending) {
+  _forgetRequest({ requestId, interceptId, url }, keepPending) {
     this.#requests.delete(requestId);
     this.#authentications.delete(interceptId);
+    this.#finishedUrls.add(url);
 
     if (!keepPending) {
       this.#pending.delete(requestId);

packages/core/src/queue.js

@@ -79,11 +79,17 @@ export class Queue {
 
     // call or set up other handlers
     let exists = this.cancel(item);
+    task.ctrl = new AbortController();
+    // duplicate abortion controller on task, so it can can be used in further
+    // generators and can be cancelled internally
+    // TODO fix this for non object item usecase
+    if (typeof item === 'object' && !Array.isArray(item) && item !== null) {
+      item._ctrl = task.ctrl;
+    }
     task.item = item = this.#handlers.push
       ? this.#handlers.push(item, exists) : item;
     task.handler = () => this.#handlers.task
       ? this.#handlers.task(item, ...args) : item;
-    task.ctrl = new AbortController();
 
     // queue this task & maybe dequeue the next task
     this.#queued.add(task);

packages/core/src/snapshot.js

@@ -118,7 +118,8 @@ function getSnapshotOptions(options, { config, meta }) {
       disableCache: config.discovery.disableCache,
       captureMockedServiceWorker: config.discovery.captureMockedServiceWorker,
       captureSrcset: config.discovery.captureSrcset,
-      userAgent: config.discovery.userAgent
+      userAgent: config.discovery.userAgent,
+      retry: config.discovery.retry
     }
   }, options], (path, prev, next) => {
     switch (path.map(k => k.toString()).join('.')) {

packages/core/src/utils.js

@@ -324,6 +324,25 @@ export function serializeFunction(fn) {
   return fnbody;
 }
 
+export async function withRetries(fn, { count, onRetry, signal, throwOn }) {
+  count ||= 1; // default a single try
+  let run = 0;
+  while (true) {
+    run += 1;
+    try {
+      return await generatePromise(fn, signal);
+    } catch (e) {
+      // if this error should not be retried on, we want to skip errors
+      let throwError = throwOn?.includes(e.name);
+      if (!throwError && run < count) {
+        await onRetry?.();
+        continue;
+      }
+      throw e;
+    }
+  }
+}
+
 export function snapshotLogName(name, meta) {
   if (meta?.snapshot?.testCase) {
     return `testCase: ${meta.snapshot.testCase}, ${name}`;

packages/core/test/discovery.test.js

@@ -910,6 +910,121 @@ describe('Discovery', () => {
         '^\\[percy:core:discovery] Setting PERCY_NETWORK_IDLE_WAIT_TIMEOUT over 60000ms is not'
       ));
     });
+
+    describe('with multiple network requests with same url', () => {
+      beforeEach(async () => {
+        // a custom page where we make 2 requests to same url where only one of them will
+        // finish before network idle wait timeout, in such cases we expect to ignore another
+        // request stuck in loading state
+        server.reply('/', () => [200, 'text/html', dedent`
+          <html>
+          <body>
+            <P>Hello Percy!<p>
+            <script>
+              // allow page load to fire and then execute this script
+              setTimeout(async () => {
+                var p1 = fetch('./img.gif');
+                var p2 = fetch('./img.gif');
+                await p2; // we will resolve second request instantly
+                await p1; // we will delay first request by 800ms
+              }, 10);
+            </script>
+          </body>
+          </html>`
+        ]);
+        // trigger responses in expected time
+        let counter = 0;
+        // we have idle timeout at 500 ms so we are resolving other request at 1 sec
+        server.reply('/img.gif', () => new Promise(r => (
+          (counter += 1) && setTimeout(r, counter === 2 ? 0 : 1000, [200, 'image/gif', pixel]))));
+      });
+
+      it('shows debug info when navigation fails within the timeout', async () => {
+        percy.loglevel('debug');
+
+        await percy.snapshot({
+          name: 'navigation idle',
+          url: 'http://localhost:8000'
+        });
+
+        expect(logger.stderr).not.toContain(jasmine.stringMatching([
+          '^\\[percy:core] Error: Timed out waiting for network requests to idle.',
+          '',
+          '  Active requests:',
+          '  - http://localhost:8000/img.gif',
+          '',
+          '(?<stack>(.|\n)*)$'
+        ].join('\n')));
+      });
+    });
+  });
+
+  describe('discovery retry', () => {
+    let Page;
+    let fastCount;
+
+    beforeEach(async () => {
+      // reset page timeout so that it gets read from env again
+      ({ Page } = await import('../src/page.js'));
+      Page.TIMEOUT = undefined;
+      process.env.PERCY_PAGE_LOAD_TIMEOUT = 500;
+
+      // some async request that takes a while and only resolves 4th time
+      let counter = 0;
+      server.reply('/', () => new Promise(r => (
+        (counter += 1) &&
+        setTimeout(r, counter === fastCount ? 0 : 2000, [200, 'text/html', '<html></html>']))));
+    });
+
+    afterAll(() => {
+      delete process.env.PERCY_PAGE_LOAD_TIMEOUT;
+    });
+
+    it('should retry the snapshot discovery upto 3 times', async () => {
+      // 3rd request will resolve instantly
+      fastCount = 3;
+
+      await percy.snapshot({
+        name: 'test navigation timeout',
+        url: 'http://localhost:8000',
+        discovery: { retry: true },
+        widths: [400, 800]
+      });
+
+      await percy.idle();
+
+      expect(logger.stdout).toEqual([
+        '[percy] Percy has started!',
+        '[percy] Retrying snapshot: test navigation timeout',
+        '[percy] Retrying snapshot: test navigation timeout',
+        '[percy] Snapshot taken: test navigation timeout'
+      ]);
+    });
+
+    it('throws exception after last retry', async () => {
+      // 3rd request will also resolve in delayed fashion
+      fastCount = 4;
+
+      await percy.snapshot({
+        name: 'test navigation timeout',
+        url: 'http://localhost:8000',
+        discovery: { retry: true },
+        widths: [400, 800]
+      });
+
+      await percy.idle();
+
+      expect(logger.stdout).toEqual([
+        '[percy] Percy has started!',
+        '[percy] Retrying snapshot: test navigation timeout',
+        '[percy] Retrying snapshot: test navigation timeout'
+      ]);
+
+      expect(logger.stderr).toEqual([
+        '[percy] Encountered an error taking snapshot: test navigation timeout',
+        '[percy] Error: Navigation failed: Timed out waiting for the page load event'
+      ]);
+    });
   });
 
   describe('navigation timeout', () => {