feat(pacmak): retry select external command invocations (#2013)

When generating packages, `jsii-pacmak` will defer to external tools for
compilation (`dotnet`, `mvn`, `pip`, ...). Those commands will typically
require some kind of network transfer (e.g: to resolve dependencies from
their standard package registry), which is susceptible to fail (due to
poor network conditions, getting throttled in CI/CD, etc...).

In order to reduce the likelihood that such failures will interrupt the
user workflow abruptly (forcing a manual retry, and possibly breaking
CI/CD workflows), back-off and retries will be performed when these
commands fail (up to 5 tentatives in total, with exponential back-off
using a 150ms base cooldown and a factor of 2).



---

By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license].

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0

packages/jsii-pacmak/lib/targets/dotnet.ts

@@ -58,6 +58,7 @@ export class DotnetBuilder implements TargetBuilder {
         ['build', '--force', '--configuration', 'Release'],
         {
           cwd: tempSourceDir.directory,
+          retry: { maxAttempts: 5 },
         },
       );
 

packages/jsii-pacmak/lib/targets/java.ts

@@ -449,6 +449,7 @@ export default class Java extends Target {
             process.env.MAVEN_OPTS ?? ''
           } -XX:+TieredCompilation -XX:TieredStopAtLevel=1`,
         },
+        retry: { maxAttempts: 5 },
       },
     );
   }
@@ -1547,7 +1548,11 @@ class JavaGenerator extends Generator {
     this.code.line();
     this.code.line('/**');
     // eslint-disable-next-line prettier/prettier
-    this.code.line(` * ${stabilityPrefixFor(cls.initializer)}A fluent builder for {@link ${builtType}}.`);
+    this.code.line(
+      ` * ${stabilityPrefixFor(
+        cls.initializer,
+      )}A fluent builder for {@link ${builtType}}.`,
+    );
     this.code.line(' */');
     this.emitStabilityAnnotations(cls.initializer);
     this.code.openBlock(

packages/jsii-pacmak/lib/targets/python.ts

@@ -66,13 +66,15 @@ export default class Python extends Target {
       VIRTUAL_ENV: venv,
     };
     const python = path.join(venvBin, 'python');
+
     // Install the necessary things
     await shell(
       python,
       ['-m', 'pip', 'install', '--no-input', ...pythonBuildTools, 'twine~=3.2'],
       {
         cwd: sourceDir,
         env,
+        retry: { maxAttempts: 5 },
       },
     );
 

packages/jsii-pacmak/lib/util.ts

@@ -5,15 +5,6 @@ import * as os from 'os';
 import * as path from 'path';
 import * as logging from './logging';
 
-export interface ShellOptions extends SpawnOptions {
-  /**
-   * Retry execution up to 3 times if it fails
-   *
-   * @default false
-   */
-  retry?: boolean;
-}
-
 /**
  * Given an npm package directory and a dependency name, returns the package directory of the dep.
  * @param packageDir     the root of the package declaring the dependency.
@@ -30,12 +21,132 @@ export function resolveDependencyDirectory(
   );
 }
 
+export interface RetryOptions {
+  /**
+   * The maximum amount of attempts to make.
+   *
+   * @default 5
+   */
+  maxAttempts?: number;
+
+  /**
+   * The amount of time (in milliseconds) to wait after the first failed attempt.
+   *
+   * @default 150
+   */
+  backoffBaseMilliseconds?: number;
+
+  /**
+   * The multiplier to apply after each failed attempts. If the backoff before
+   * the previous attempt was `B`, the next backoff is computed as
+   * `B * backoffMultiplier`, creating an exponential series.
+   *
+   * @default 2
+   */
+  backoffMultiplier?: number;
+
+  /**
+   * An optionnal callback that gets invoked when an attempt failed. This can be
+   * used to give the user indications of what is happening.
+   *
+   * This callback must not throw.
+   *
+   * @param error               the error that just occurred
+   * @param attemptsLeft        the number of attempts left
+   * @param backoffMilliseconds the amount of milliseconds of back-off that will
+   *                            be awaited before making the next attempt (if
+   *                            there are attempts left)
+   */
+  onFailedAttempt?: (
+    error: unknown,
+    attemptsLeft: number,
+    backoffMilliseconds: number,
+  ) => void;
+}
+
+export class AllAttemptsFailed<R> extends Error {
+  public constructor(
+    public readonly callback: () => Promise<R>,
+    public readonly errors: readonly Error[],
+  ) {
+    super(
+      `All attempts failed. Last error: ${errors[errors.length - 1].message}`,
+    );
+  }
+}
+
+/**
+ * Adds back-off and retry logic around the provided callback.
+ *
+ * @param cb   the callback which is to be retried.
+ * @param opts the backoff-and-retry configuration
+ *
+ * @returns the result of `cb`
+ */
+export async function retry<R>(
+  cb: () => Promise<R>,
+  opts: RetryOptions = {},
+  waiter: (ms: number) => Promise<void> = wait,
+): Promise<R> {
+  let attemptsLeft = opts.maxAttempts ?? 5;
+  let backoffMs = opts.backoffBaseMilliseconds ?? 150;
+  const backoffMult = opts.backoffMultiplier ?? 2;
+
+  // Check for incorrect usage
+  if (attemptsLeft <= 0) {
+    throw new Error('maxTries must be > 0');
+  }
+  if (backoffMs <= 0) {
+    throw new Error('backoffBaseMilliseconds must be > 0');
+  }
+  if (backoffMult <= 1) {
+    throw new Error('backoffMultiplier must be > 1');
+  }
+
+  const errors = new Array<Error>();
+  while (attemptsLeft > 0) {
+    attemptsLeft--;
+    try {
+      // eslint-disable-next-line no-await-in-loop
+      return await cb();
+    } catch (error) {
+      errors.push(error);
+      if (opts.onFailedAttempt != null) {
+        opts.onFailedAttempt(error, attemptsLeft, backoffMs);
+      }
+    }
+    if (attemptsLeft > 0) {
+      // eslint-disable-next-line no-await-in-loop
+      await waiter(backoffMs).then(() => (backoffMs *= backoffMult));
+    }
+  }
+  return Promise.reject(new AllAttemptsFailed(cb, errors));
+}
+
+export interface ShellOptions extends Omit<SpawnOptions, 'shell' | 'stdio'> {
+  /**
+   * Configure in-line retries if the execution fails.
+   *
+   * @default - no retries
+   */
+  readonly retry?: RetryOptions;
+}
+
+/**
+ * Spawns a child process with the provided command and arguments. The child
+ * process is always spawned using `shell: true`, and the contents of
+ * `process.env` is used as the initial value of the `env` spawn option (values
+ * provided in `options.env` can override those).
+ *
+ * @param cmd     the command to shell out to.
+ * @param args    the arguments to provide to `cmd`
+ * @param options any options to pass to `spawn`
+ */
 export async function shell(
   cmd: string,
   args: string[],
-  options: ShellOptions = {},
+  { retry: retryOptions, ...options }: ShellOptions = {},
 ): Promise<string> {
-  // eslint-disable-next-line @typescript-eslint/require-await
   async function spawn1() {
     logging.debug(cmd, args.join(' '), JSON.stringify(options));
     return new Promise<string>((ok, ko) => {
@@ -91,21 +202,27 @@ export async function shell(
       });
     });
   }
-  /* eslint-enable @typescript-eslint/require-await */
 
-  let attempts = options.retry ? 3 : 1;
-  while (attempts > 0) {
-    attempts--;
-    try {
-      return spawn1();
-    } catch (e) {
-      if (attempts === 0) {
-        throw e;
-      }
-      logging.info(`${e.message} (retrying)`);
-    }
+  if (retryOptions != null) {
+    return retry(spawn1, {
+      ...retryOptions,
+      onFailedAttempt:
+        retryOptions.onFailedAttempt ??
+        ((error, attemptsLeft, backoffMs) => {
+          const message = (error as Error).message ?? error;
+          const retryInfo =
+            attemptsLeft > 0
+              ? `Waiting ${backoffMs} ms before retrying (${attemptsLeft} attempts left)`
+              : 'No attempts left';
+          logging.info(
+            `Command "${cmd} ${args.join(
+              ' ',
+            )}" failed with ${message}. ${retryInfo}.`,
+          );
+        }),
+    });
   }
-  throw new Error('No attempts left'); // This is, in fact, unreachable code.
+  return spawn1();
 }
 
 /**
@@ -181,3 +298,7 @@ export async function filterAsync<A>(
   );
   return mapped.filter(({ pred }) => pred).map(({ x }) => x);
 }
+
+export async function wait(ms: number): Promise<void> {
+  return new Promise((ok) => setTimeout(ok, ms));
+}

packages/jsii-pacmak/test/util.test.ts

@@ -0,0 +1,133 @@
+import { AllAttemptsFailed, retry, wait } from '../lib/util';
+
+type Waiter = typeof wait;
+
+describe(retry, () => {
+  const mockWaiter = jest.fn<ReturnType<Waiter>, Parameters<Waiter>>();
+
+  beforeEach((done) => {
+    mockWaiter.mockImplementation(() => Promise.resolve());
+    done();
+  });
+
+  afterEach((done) => {
+    jest.resetAllMocks();
+    done();
+  });
+
+  test('throws on invalid maxTries', async () => {
+    // GIVEN
+    const fakeCallback = jest.fn();
+
+    // WHEN
+    const result = retry(fakeCallback, { maxAttempts: 0 }, mockWaiter);
+
+    // THEN
+    await expect(result).rejects.toThrow('maxTries must be > 0');
+    expect(fakeCallback).not.toHaveBeenCalled();
+  });
+
+  test('throws on invalid backoffBaseMilliseconds', async () => {
+    // GIVEN
+    const fakeCallback = jest.fn();
+
+    // WHEN
+    const result = retry(
+      fakeCallback,
+      { backoffBaseMilliseconds: 0 },
+      mockWaiter,
+    );
+
+    // THEN
+    await expect(result).rejects.toThrow('backoffBaseMilliseconds must be > 0');
+    expect(fakeCallback).not.toHaveBeenCalled();
+  });
+
+  test('throws on invalid backoffMultiplier', async () => {
+    // GIVEN
+    const fakeCallback = jest.fn();
+
+    // WHEN
+    const result = retry(fakeCallback, { backoffMultiplier: 1 }, mockWaiter);
+
+    // THEN
+    await expect(result).rejects.toThrow('backoffMultiplier must be > 1');
+    expect(fakeCallback).not.toHaveBeenCalled();
+  });
+
+  test('throws when it exhausted all attempts', async () => {
+    // GIVEN
+    const alwaysFail = () => Promise.reject(new Error('Always'));
+
+    // WHEN
+    const result = retry(alwaysFail, {}, mockWaiter);
+
+    // THEN
+    await expect(result).rejects.toThrow(AllAttemptsFailed);
+    expect(mockWaiter).toHaveBeenCalledTimes(4);
+    for (let i = 0; i < 4; i++) {
+      expect(mockWaiter).toHaveBeenNthCalledWith(i + 1, 150 * Math.pow(2, i));
+    }
+  });
+
+  test('behaves correctly if callback throws non-Error', async () => {
+    // GIVEN
+    const alwaysFail = () => Promise.reject('Always');
+
+    // WHEN
+    const result = retry(
+      alwaysFail,
+      { maxAttempts: 3, backoffBaseMilliseconds: 1337, backoffMultiplier: 42 },
+      mockWaiter,
+    );
+
+    // THEN
+    await expect(result).rejects.toThrow(AllAttemptsFailed);
+    expect(mockWaiter).toHaveBeenCalledTimes(2);
+  });
+
+  test('correctly implements exponential back-off and calls onFaieldAttempt appropriately', async () => {
+    // GIVEN
+    const expectedResult = Math.random();
+    let attemptNumber = 0;
+    const failFourTimes = () =>
+      new Promise((ok, ko) => {
+        attemptNumber++;
+        if (attemptNumber <= 4) {
+          ko(new Error(`Attempt #${attemptNumber}`));
+        } else {
+          ok(expectedResult);
+        }
+      });
+    const onFailedAttempt = jest
+      .fn()
+      .mockName('onFailedAttempt')
+      .mockImplementation(
+        (error: Error, attemptsLeft: number, backoffMs: number) => {
+          expect(error.message).toBe(`Attempt #${attemptNumber}`);
+          expect(attemptsLeft).toBe(5 - attemptNumber);
+          expect(backoffMs).toBe(1337 * Math.pow(42, attemptNumber - 1));
+        },
+      );
+
+    // WHEN
+    const result = await retry(
+      failFourTimes,
+      {
+        backoffBaseMilliseconds: 1337,
+        backoffMultiplier: 42,
+        onFailedAttempt,
+      },
+      mockWaiter,
+    );
+
+    // THEN
+    expect(result).toBe(result);
+
+    expect(onFailedAttempt).toHaveBeenCalledTimes(4);
+    expect(mockWaiter).toHaveBeenCalledTimes(4);
+    for (let i = 0; i < 4; i++) {
+      expect(mockWaiter).toHaveBeenNthCalledWith(i + 1, 1337 * Math.pow(42, i));
+    }
+  });
+});