fix(idempotency): check error identity via names

[dreamorosi]

Summary

Changes

Please provide a summary of what's being changed

This PR makes changes to the custom errors that can be thrown by the Idempotency utility so that their respective name property is set to the actual name of the error, for example:

class IdempotencyUnknownError extends Error {
  public constructor(message?: string, options?: ErrorOptions) {
    super(message, options);
+   this.name = 'IdempotencyUnknownError';
  }
}

This change allows to handle errors without having to import the error class itself, and without having to rely on a referential identity check (i.e. error instanceof IdempotencyUnknownError).

Currently, we use error instanceof ... in several places within the internals of the utility. This is a result of two factors: a/ the implementation relies heavily on throwing errors as a way of controlling flow, and b/ composition and object-oriented programming patterns are prevalent, with errors being thrown in certain code paths and then handled gracefully with try/catch blocks in other parts.

For example, we have a few instances in which we do something like this:

try {
  const { isIdempotent, result } =
    await this.#saveInProgressOrReturnExistingResult();
  if (isIdempotent) return result as ReturnType<Func>;

  return await this.getFunctionResult();
} catch (error) {
  if (
    error instanceof IdempotencyInconsistentStateError &&
    retryNo < MAX_RETRIES
  ) {
    // Retry
    continue;
  }
  // Retries exhausted or other error
  e = error;
  break;
}

Without getting too much into the weeds of what the code above does, the important parts to know are that:

1. we run some code in the try block - this code might throw either an error that we can recover from, or other types of errors that we can't handle
2. in the catch block we check if the error being caught is one that we know how to handle

• if this is the case, we handle it
• if not, we re-throw it

Because the code above relies on instanceof, JavaScript checks that the object in the left side of the statement is an instance of the class in the right side which might not be the case (for a number of reasons).

One of these reasons can happen because of the famed dual package hazard, which in simple terms means when one part of your code imports the CommonJS (CJS) version of a module, and another part of your code imports the same module but using ECMAScript modules (ESM).

This is something that our utilities are naturally exposed to since we started shipping ESM and CJS in v2.0. This is a tradeoff that we accepted at the time to allow customers to use Powertools for AWS Lambda regardless of what they used.

In practice, this is relatively unlikely to happen to our customers since our utilities have only a few entry points and most of them do a relatively good job at isolating state. This PR tries to address one of the areas that did not do this propertly.

Specifically, under certain circumstances ([1] & [2]) customers might end up with multiple copies of the errors defined in the Idempotency utility.

When this happens, the internal flow of the utility breaks down causing requests to be repeated, which is what was happening in the linked issue. Once the changes in this PR are merged, it will be possible the utility should be more resilient to this type of issue because the checks on errors won't rely on referential identity anymore.

For example, the code from above would change to this:

try {
  const { isIdempotent, result } =
    await this.#saveInProgressOrReturnExistingResult();
  if (isIdempotent) return result as ReturnType<Func>;

  return await this.getFunctionResult();
} catch (error) {
  if (!(error instanceof Error))
    throw new IdempotencyUnknownError(
      'An unknown error occurred while processing the request.',
      { cause: error }
    );
  if (
    error.name === 'IdempotencyInconsistentStateError' && // we now use `error.name` instead of instanceof
    retryNo < MAX_RETRIES
  ) {
    // Retry
    continue;
  }
  // Retries exhausted or other error
  e = error;
  break;
}

With the changes above, it won't matter if the objects don't share the same identity, meaning that as long as they share the same implementation the utility will behave as expected.

There are additional areas of improvements that we can address to further decrease the risk of dual-package hazard such as providing even more granular subpath scoped exports (i.e. this customer was importing IdempotencyConfig via default export - aka barrel file - which caused the errors to be brought in because they're exported in the packages/idempotency/src/index.ts file here). However, in the interest of keeping this essay short (lol) and the PR focused I'll look into making these changes in a future PR.

Please add the issue number below, if no issue is present the PR might get blocked and not be reviewed

Issue number: #2517

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

[am29d]

Thanks for the PR! It was a deep investigation and great informative summary to understand the problem.

We could potentially extract the name strings into constants, to avoid any typo mistakes, but the number of errors is not high and we don't have many places where they are used.

Overall, great work on narrowing down this complex issue 👏

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud