fix: retry transactions that fail with expired transaction IDs (#1347)

googleapis · Nov 2, 2020 · a18ab50 · a18ab50
1 parent 8717bf2
commit a18ab50
Show file tree

Hide file tree

Showing 2 changed files with 77 additions and 30 deletions.
diff --git a/dev/src/transaction.ts b/dev/src/transaction.ts
@@ -423,21 +423,22 @@ export class Transaction implements firestore.Transaction {
     let lastError: GoogleError | undefined = undefined;
 
     for (let attempt = 0; attempt < maxAttempts; ++attempt) {
-      if (lastError) {
-        logger(
-          'Firestore.runTransaction',
-          this._requestTag,
-          'Retrying transaction after error:',
-          lastError
-        );
-      }
+      try {
+        if (lastError) {
+          logger(
+            'Firestore.runTransaction',
+            this._requestTag,
+            'Retrying transaction after error:',
+            lastError
+          );
+          await this.rollback();
+        }
 
-      this._writeBatch._reset();
-      await this.maybeBackoff(lastError);
+        this._writeBatch._reset();
+        await this.maybeBackoff(lastError);
 
-      await this.begin();
+        await this.begin();
 
-      try {
         const promise = updateFunction(this);
         if (!(promise instanceof Promise)) {
           throw new Error(
@@ -455,12 +456,10 @@ export class Transaction implements firestore.Transaction {
           err
         );
 
-        await this.rollback();
+        lastError = err;
 
-        if (isRetryableTransactionError(err)) {
-          lastError = err;
-        } else {
-          return Promise.reject(err); // Callback failed w/ non-retryable error
+        if (!this._transactionId || !isRetryableTransactionError(err)) {
+          break;
         }
       }
     }
@@ -471,6 +470,8 @@ export class Transaction implements firestore.Transaction {
       'Transaction not eligible for retry, returning error: %s',
       lastError
     );
+
+    await this.rollback();
     return Promise.reject(lastError);
   }
 
@@ -599,6 +600,11 @@ function isRetryableTransactionError(error: GoogleError): boolean {
       case Status.UNAUTHENTICATED:
       case Status.RESOURCE_EXHAUSTED:
         return true;
+      case Status.INVALID_ARGUMENT:
+        // The Firestore backend uses "INVALID_ARGUMENT" for transactions
+        // IDs that have expired. While INVALID_ARGUMENT is generally not
+        // retryable, we retry this specific case.
+        return !!error.message.match(/transaction has expired/);
       default:
         return false;
     }

diff --git a/dev/test/transaction.ts b/dev/test/transaction.ts
@@ -355,7 +355,10 @@ function runTransaction<T>(
       });
     } finally {
       setTimeoutHandler(setTimeout);
-      expect(expectedRequests.length).to.equal(0);
+      expect(expectedRequests.length).to.equal(
+        0,
+        'Missing requests: ' + expectedRequests.map(r => r.type).join(', ')
+      );
     }
   });
 }
@@ -433,6 +436,25 @@ describe('failed transactions', () => {
     }
   });
 
+  it('retries commit for expired transaction', async () => {
+    const transactionFunction = () => Promise.resolve();
+
+    const serverError = new GoogleError(
+      'The referenced transaction has expired or is no longer valid.'
+    );
+    serverError.code = Status.INVALID_ARGUMENT;
+
+    await runTransaction(
+      transactionFunction,
+      begin('foo1'),
+      commit('foo1', undefined, serverError),
+      rollback('foo1'),
+      backoff(),
+      begin('foo2', 'foo1'),
+      commit('foo2')
+    );
+  });
+
   it('retries runQuery based on error code', async () => {
     const transactionFunction = (
       transaction: Transaction,
@@ -506,6 +528,37 @@ describe('failed transactions', () => {
     }
   });
 
+  it('retries rollback based on error code', async () => {
+    const transactionFunction = () => Promise.resolve();
+
+    for (const [errorCode, retry] of Object.entries(retryBehavior)) {
+      const serverError = new GoogleError('Test Error');
+      serverError.code = Number(errorCode) as Status;
+
+      if (retry) {
+        await runTransaction(
+          transactionFunction,
+          begin('foo1'),
+          commit('foo1', /* writes=*/ undefined, serverError),
+          rollback('foo1', serverError),
+          rollback('foo1'),
+          backoff(),
+          begin('foo2', 'foo1'),
+          commit('foo2')
+        );
+      } else {
+        await expect(
+          runTransaction(
+            transactionFunction,
+            begin('foo1'),
+            commit('foo1', /* writes=*/ undefined, serverError),
+            rollback('foo1', serverError)
+          )
+        ).to.eventually.be.rejected;
+      }
+    }
+  });
+
   it('requires update function', () => {
     const overrides: ApiOverride = {
       beginTransaction: () => Promise.reject(),
@@ -618,18 +671,6 @@ describe('failed transactions', () => {
       commit('foo2')
     );
   });
-
-  it('fails on rollback', () => {
-    return expect(
-      runTransaction(
-        () => {
-          return Promise.reject();
-        },
-        begin(),
-        rollback('foo', new Error('Fails on rollback'))
-      )
-    ).to.eventually.be.rejectedWith('Fails on rollback');
-  });
 });
 
 describe('transaction operations', () => {