Merge pull request #4505 from snyk/feat/add-retry-for-sendTestPayload

feat: add retry to sendTestPayload

src/lib/errors/bad-gateway-error.ts

@@ -0,0 +1,14 @@
+import { CustomError } from './custom-error';
+
+export class BadGatewayError extends CustomError {
+  private static ERROR_CODE = 502;
+  private static ERROR_STRING_CODE = 'BAD_GATEWAY_ERROR';
+  private static ERROR_MESSAGE = 'Bad gateway error';
+
+  constructor(userMessage) {
+    super(BadGatewayError.ERROR_MESSAGE);
+    this.code = BadGatewayError.ERROR_CODE;
+    this.strCode = BadGatewayError.ERROR_STRING_CODE;
+    this.userMessage = userMessage || BadGatewayError.ERROR_MESSAGE;
+  }
+}

src/lib/errors/index.ts

@@ -10,6 +10,8 @@ export { ConnectionTimeoutError } from './connection-timeout-error';
 export { FailedToLoadPolicyError } from './failed-to-load-policy-error';
 export { PolicyNotFoundError } from './policy-not-found-error';
 export { InternalServerError } from './internal-server-error';
+export { BadGatewayError } from './bad-gateway-error';
+export { ServiceUnavailableError } from './service-unavailable-error';
 export { FailedToGetVulnerabilitiesError } from './failed-to-get-vulnerabilities-error';
 export { FailedToGetVulnsFromUnavailableResource } from './failed-to-get-vulns-from-unavailable-resource';
 export { UnsupportedPackageManagerError } from './unsupported-package-manager-error';

src/lib/errors/service-unavailable-error.ts

@@ -0,0 +1,14 @@
+import { CustomError } from './custom-error';
+
+export class ServiceUnavailableError extends CustomError {
+  private static ERROR_CODE = 503;
+  private static ERROR_STRING_CODE = 'SERVICE_UNAVAILABLE_ERROR';
+  private static ERROR_MESSAGE = 'Service unavailable error';
+
+  constructor(userMessage) {
+    super(ServiceUnavailableError.ERROR_MESSAGE);
+    this.code = ServiceUnavailableError.ERROR_CODE;
+    this.strCode = ServiceUnavailableError.ERROR_STRING_CODE;
+    this.userMessage = userMessage || ServiceUnavailableError.ERROR_MESSAGE;
+  }
+}

src/lib/snyk-test/common.ts

@@ -65,3 +65,6 @@ export enum FAIL_ON {
 }
 
 export type FailOn = 'all' | 'upgradable' | 'patchable';
+
+export const RETRY_ATTEMPTS = 3;
+export const RETRY_DELAY = 500;

src/lib/snyk-test/run-test.ts

@@ -30,6 +30,8 @@ import {
   NoSupportedManifestsFoundError,
   NotFoundError,
   errorMessageWithRetry,
+  BadGatewayError,
+  ServiceUnavailableError,
 } from '../errors';
 import * as snyk from '../';
 import { isCI } from '../is-ci';
@@ -68,6 +70,8 @@ import { assembleEcosystemPayloads } from './assemble-payloads';
 import { makeRequest } from '../request';
 import { spinner } from '../spinner';
 import { hasUnknownVersions } from '../dep-graph';
+import { sleep } from '../common';
+import { RETRY_ATTEMPTS, RETRY_DELAY } from './common';
 
 const debug = debugModule('snyk:run-test');
 
@@ -234,13 +238,38 @@ async function sendAndParseResults(
     response: any;
   };
   const requests: (() => Promise<TestResponse>)[] = [];
-  for (const payload of payloads) {
+  for (const originalPayload of payloads) {
     const request = async (): Promise<TestResponse> => {
-      /** sendTestPayload() deletes the request.body from the payload once completed. */
-      const originalPayload = Object.assign({}, payload);
-      const response = await sendTestPayload(payload);
-      return { payload, originalPayload, response };
+      let step = 0;
+      let error;
+
+      while (step < RETRY_ATTEMPTS) {
+        debug(`sendTestPayload retry step ${step} out of ${RETRY_ATTEMPTS}`);
+        try {
+          /** sendTestPayload() deletes the request.body from the payload once completed. */
+          const payload = Object.assign({}, originalPayload);
+          const response = await sendTestPayload(payload);
+
+          return { payload, originalPayload, response };
+        } catch (err) {
+          error = err;
+          step++;
+
+          if (
+            err instanceof InternalServerError ||
+            err instanceof BadGatewayError ||
+            err instanceof ServiceUnavailableError
+          ) {
+            await sleep(RETRY_DELAY);
+          } else {
+            break;
+          }
+        }
+      }
+
+      throw error;
     };
+
     requests.push(request);
   }
 
@@ -516,6 +545,14 @@ function handleTestHttpErrorResponse(res, body) {
       err = new InternalServerError(userMessage);
       err.innerError = body.stack;
       break;
+    case 502:
+      err = new BadGatewayError(userMessage);
+      err.innerError = body.stack;
+      break;
+    case 503:
+      err = new ServiceUnavailableError(userMessage);
+      err.innerError = body.stack;
+      break;
     default:
       err = new FailedToGetVulnerabilitiesError(userMessage, statusCode);
       err.innerError = body.error;

test/acceptance/fake-server.ts

@@ -21,6 +21,8 @@ export type FakeServer = {
   setDepGraphResponse: (next: Record<string, unknown>) => void;
   setNextResponse: (r: any) => void;
   setNextStatusCode: (c: number) => void;
+  setStatusCode: (c: number) => void;
+  setStatusCodes: (c: number[]) => void;
   setFeatureFlag: (featureFlag: string, enabled: boolean) => void;
   unauthorizeAction: (action: string, reason?: string) => void;
   listen: (port: string | number, callback: () => void) => void;
@@ -39,12 +41,17 @@ export const fakeServer = (basePath: string, snykToken: string): FakeServer => {
   let requests: express.Request[] = [];
   let featureFlags: Map<string, boolean> = featureFlagDefaults();
   let unauthorizedActions = new Map();
+  // the status code to return for the next request, overriding statusCode
   let nextStatusCode: number | undefined = undefined;
+  // the status code to return for all the requests
+  let statusCode: number | undefined = undefined;
+  let statusCodes: number[] = [];
   let nextResponse: any = undefined;
   let depGraphResponse: Record<string, unknown> | undefined = undefined;
   let server: http.Server | undefined = undefined;
 
   const restore = () => {
+    statusCode = undefined;
     requests = [];
     depGraphResponse = undefined;
     featureFlags = featureFlagDefaults();
@@ -79,6 +86,14 @@ export const fakeServer = (basePath: string, snykToken: string): FakeServer => {
     nextStatusCode = code;
   };
 
+  const setStatusCode = (code: number) => {
+    statusCode = code;
+  };
+
+  const setStatusCodes = (codes: number[]) => {
+    statusCodes = codes;
+  };
+
   const setFeatureFlag = (featureFlag: string, enabled: boolean) => {
     featureFlags.set(featureFlag, enabled);
   };
@@ -129,7 +144,7 @@ export const fakeServer = (basePath: string, snykToken: string): FakeServer => {
     if (
       req.url?.includes('/iac-org-settings') ||
       req.url?.includes('/cli-config/feature-flags/') ||
-      (!nextResponse && !nextStatusCode)
+      (!nextResponse && !nextStatusCode && !statusCode)
     ) {
       return next();
     }
@@ -139,7 +154,10 @@ export const fakeServer = (basePath: string, snykToken: string): FakeServer => {
       const code = nextStatusCode;
       nextStatusCode = undefined;
       res.status(code);
+    } else if (statusCode) {
+      res.status(statusCode);
     }
+
     res.send(response);
   });
 
@@ -195,6 +213,12 @@ export const fakeServer = (basePath: string, snykToken: string): FakeServer => {
       return next();
     }
 
+    const statusCode = statusCodes.shift();
+    if (statusCode && statusCode !== 200) {
+      res.sendStatus(statusCode);
+      return next();
+    }
+
     if (depGraphResponse) {
       res.send(depGraphResponse);
       return next();
@@ -574,6 +598,8 @@ export const fakeServer = (basePath: string, snykToken: string): FakeServer => {
     setDepGraphResponse,
     setNextResponse,
     setNextStatusCode,
+    setStatusCode,
+    setStatusCodes,
     setFeatureFlag,
     unauthorizeAction,
     listen,

test/jest/acceptance/snyk-fix/fix.spec.ts

@@ -90,7 +90,7 @@ describe('snyk fix', () => {
 
   it('fails when api requests fail', async () => {
     const project = await createProjectFromWorkspace('no-vulns');
-    server.setNextStatusCode(500);
+    server.setStatusCode(500);
     const { code, stdout, stderr } = await runSnykCLI(
       'fix --org=aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee',
       {

test/jest/acceptance/snyk-test/retry-mechanism.spec.ts

@@ -0,0 +1,60 @@
+import { fakeServer } from '../../../acceptance/fake-server';
+import { createProjectFromWorkspace } from '../../util/createProject';
+import { runSnykCLI } from '../../util/runSnykCLI';
+import { RETRY_ATTEMPTS } from '../../../../src/lib/snyk-test/common';
+
+jest.setTimeout(2000 * 60);
+
+describe('snyk test retry mechanism', () => {
+  let server;
+  let env: Record<string, string>;
+
+  beforeAll((done) => {
+    const port = process.env.PORT || process.env.SNYK_PORT || '12345';
+    const baseApi = '/api/v1';
+    env = {
+      ...process.env,
+      SNYK_API: 'http://localhost:' + port + baseApi,
+      SNYK_HOST: 'http://localhost:' + port,
+      SNYK_TOKEN: '123456789',
+      SNYK_DISABLE_ANALYTICS: '1',
+    };
+    server = fakeServer(baseApi, env.SNYK_TOKEN);
+    server.listen(port, () => {
+      done();
+    });
+  });
+
+  afterAll((done) => {
+    server.close(() => {
+      done();
+    });
+  });
+
+  test('run `snyk test` on an arbitrary cocoapods project and expect retries in case of failures', async () => {
+    const project = await createProjectFromWorkspace('cocoapods-app');
+    const statuses = Array(RETRY_ATTEMPTS - 1)
+      .fill(500)
+      .concat([200]);
+    server.setStatusCodes(statuses);
+
+    const { code } = await runSnykCLI('test', {
+      cwd: project.path(),
+      env,
+    });
+
+    expect(code).toEqual(0);
+  });
+
+  test('run `snyk test` on an arbitrary cocoapods project and expect failure when the retry budget is consumed', async () => {
+    const project = await createProjectFromWorkspace('cocoapods-app');
+    server.setStatusCodes(Array(RETRY_ATTEMPTS).fill(500));
+
+    const { code } = await runSnykCLI('test', {
+      cwd: project.path(),
+      env,
+    });
+
+    expect(code).toEqual(2);
+  });
+});

test/tap/cli-test.acceptance.test.ts

@@ -113,6 +113,7 @@ test(GenericTests.language, async (t) => {
         { chdirWorkspaces },
       ),
     );
+    server.restore();
   }
 });
 
@@ -125,6 +126,7 @@ test(AllProjectsTests.language, async (t) => {
         { chdirWorkspaces },
       ),
     );
+    server.restore();
   }
 });
 

test/tap/cli-test/cli-test.generic.spec.ts

@@ -405,7 +405,7 @@ export const GenericTests: AcceptanceTests = {
     'error 500 handling': (params, utils) => async (t) => {
       utils.chdirWorkspaces();
 
-      params.server.setNextStatusCode(500);
+      params.server.setStatusCode(500);
 
       try {
         await params.cli.test('ruby-app-thresholds');

test/tap/remote-package.test.ts

@@ -187,7 +187,7 @@ test('test for existing remote package with dev-deps only', async (t) => {
 
 test('test for non-existing', async (t) => {
   try {
-    server.setNextStatusCode(500);
+    server.setStatusCode(500);
     const res = await cli.test('@123');
     t.fail('should fail, instead received ' + res);
   } catch (error) {