chore: retry all idempotent operations on cancelled errors

momentohq · Nov 12, 2024 · 9bdef8f · 9bdef8f
1 parent 29c0ee5
commit 9bdef8f
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 52 deletions.
diff --git a/packages/client-sdk-nodejs/src/config/retry/default-eligibility-strategy.ts b/packages/client-sdk-nodejs/src/config/retry/default-eligibility-strategy.ts
@@ -5,36 +5,11 @@ import {
   EligibleForRetryProps,
 } from './eligibility-strategy';
 
-const retryableWriteGrpcStatusCodes: Array<Status> = [
+const retryableGrpcStatusCodes: Array<Status> = [
   // including all the status codes for reference, but
   // commenting out the ones we don't want to retry on for now.
 
   // Status.OK,
-  // Status.CANCELLED,
-  // Status.UNKNOWN,
-  // Status.INVALID_ARGUMENT,
-  // Status.DEADLINE_EXCEEDED,
-  // Status.NOT_FOUND,
-  // Status.ALREADY_EXISTS,
-  // Status.PERMISSION_DENIED,
-  // Status.RESOURCE_EXHAUSTED,
-  // Status.FAILED_PRECONDITION,
-  // Status.ABORTED,
-  // Status.OUT_OF_RANGE,
-  // Status.UNIMPLEMENTED,
-  Status.INTERNAL,
-  Status.UNAVAILABLE,
-  // Status.DATA_LOSS,
-  // Status.UNAUTHENTICATED
-];
-
-const retryableReadGrpcStatusCodes: Array<Status> = [
-  // including all the status codes for reference, but
-  // commenting out the ones we don't want to retry on for now.
-
-  // Status.OK,
-  // Read requests can be safely retried for CANCELLED errors. These may pop us sometimes during
-  // client or server side deployments
   Status.CANCELLED,
   // Status.UNKNOWN,
   // Status.INVALID_ARGUMENT,
@@ -53,38 +28,35 @@ const retryableReadGrpcStatusCodes: Array<Status> = [
   // Status.UNAUTHENTICATED
 ];
 
-const retryableWriteRequestTypes: Array<string> = [
+const retryableRequestTypes: Array<string> = [
   '/cache_client.Scs/Set',
+  '/cache_client.Scs/Get',
   '/cache_client.Scs/Delete',
   '/cache_client.Scs/DictionarySet',
   // not idempotent: '/cache_client.Scs/DictionaryIncrement',
+  '/cache_client.Scs/DictionaryGet',
+  '/cache_client.Scs/DictionaryFetch',
   '/cache_client.Scs/DictionaryDelete',
   '/cache_client.Scs/SetUnion',
+  '/cache_client.Scs/SetDifference',
+  '/cache_client.Scs/SetFetch',
   // not idempotent: '/cache_client.Scs/ListPushFront',
   // not idempotent: '/cache_client.Scs/ListPushBack',
   // not idempotent: '/cache_client.Scs/ListPopFront',
   // not idempotent: '/cache_client.Scs/ListPopBack',
+  '/cache_client.Scs/ListFetch',
   /*
    *  Warning: in the future, this may not be idempotent
    *  Currently it supports removing all occurrences of a value.
    *  In the future, we may also add "the first/last N occurrences of a value".
    *  In the latter case it is not idempotent.
    */
   '/cache_client.Scs/ListRemove',
+  '/cache_client.Scs/ListLength',
   // not idempotent: '/cache_client.Scs/ListConcatenateFront',
   // not idempotent: '/cache_client.Scs/ListConcatenateBack'
 ];
 
-const retryableReadRequestTypes: Array<string> = [
-  '/cache_client.Scs/Get',
-  '/cache_client.Scs/DictionaryGet',
-  '/cache_client.Scs/DictionaryFetch',
-  '/cache_client.Scs/SetDifference',
-  '/cache_client.Scs/SetFetch',
-  '/cache_client.Scs/ListFetch',
-  '/cache_client.Scs/ListLength',
-];
-
 export class DefaultEligibilityStrategy implements EligibilityStrategy {
   private readonly logger: MomentoLogger;
 
@@ -93,23 +65,20 @@ export class DefaultEligibilityStrategy implements EligibilityStrategy {
   }
 
   isEligibleForRetry(props: EligibleForRetryProps): boolean {
-    if (
-      retryableReadGrpcStatusCodes.includes(props.grpcStatus.code) &&
-      retryableReadRequestTypes.includes(props.grpcRequest.path)
-    ) {
-      return true;
+    if (!retryableGrpcStatusCodes.includes(props.grpcStatus.code)) {
+      this.logger.debug(
+        `Response with status code ${props.grpcStatus.code} is not retryable.`
+      );
+      return false;
     }
 
-    if (
-      retryableWriteGrpcStatusCodes.includes(props.grpcStatus.code) &&
-      retryableWriteRequestTypes.includes(props.grpcRequest.path)
-    ) {
-      return true;
+    if (!retryableRequestTypes.includes(props.grpcRequest.path)) {
+      this.logger.debug(
+        `Request with type ${props.grpcRequest.path} is not retryable.`
+      );
+      return false;
     }
 
-    this.logger.debug(
-      `Request with type ${props.grpcRequest.path} and status code ${props.grpcStatus.code} is not retryable.`
-    );
-    return false;
+    return true;
   }
 }
diff --git a/packages/client-sdk-nodejs/test/unit/config/retry/default-eligibility-strategy.test.ts b/packages/client-sdk-nodejs/test/unit/config/retry/default-eligibility-strategy.test.ts
@@ -74,7 +74,7 @@ describe('DefaultEligibilityStrategy', () => {
     expect(isEligible).toBe(true);
   });
 
-  it('should return false for CANCELLED status code and SET request path', () => {
+  it('should return true for CANCELLED status code and SET request path', () => {
     const grpcStatus = {code: Status.CANCELLED} as StatusObject;
     const grpcRequest = {
       path: '/cache_client.Scs/Set',
@@ -87,6 +87,22 @@ describe('DefaultEligibilityStrategy', () => {
       requestMetadata,
     });
 
+    expect(isEligible).toBe(true);
+  });
+
+  it('should return false for CANCELLED status code and dictionary increment request path', () => {
+    const grpcStatus = {code: Status.CANCELLED} as StatusObject;
+    const grpcRequest = {
+      path: '/cache_client.Scs/DictionaryIncrement',
+    } as ClientMethodDefinition<unknown, unknown>;
+    const requestMetadata = new Metadata();
+
+    const isEligible = eligibilityStrategy.isEligibleForRetry({
+      grpcStatus,
+      grpcRequest,
+      requestMetadata,
+    });
+
     expect(isEligible).toBe(false);
   });
 });