fix: unknown errors should not be retried (googleapis#1388)

Unknown errors that are returned during a transaction should not cause the entire transaction to be automatically retried, as there is no absolute guarantee that the initial attempt did not succeed. A `Transaction Outcome Unknown` (or other Unknown) error could be returned by the `Commit` RPC, and in that case there is a possibility that the transaction was actually committed. Fixes googleapis#1387
AVaksman · Jun 9, 2021 · 1d6f4e2 · 1d6f4e2
1 parent 1eeacc9
commit 1d6f4e2
Show file tree

Hide file tree

Showing 2 changed files with 4 additions and 76 deletions.
diff --git a/src/transaction-runner.ts b/src/transaction-runner.ts
@@ -29,7 +29,7 @@ import {Database} from './database';
 const jsonProtos = require('../protos/protos.json');
 const RETRY_INFO = 'google.rpc.retryinfo-bin';
 
-const RETRYABLE: grpc.status[] = [grpc.status.ABORTED, grpc.status.UNKNOWN];
+const RETRYABLE: grpc.status[] = [grpc.status.ABORTED];
 
 // tslint:disable-next-line variable-name
 const RetryInfo = Root.fromJSON(jsonProtos).lookup('google.rpc.RetryInfo');

diff --git a/test/transaction-runner.ts b/test/transaction-runner.ts
@@ -243,7 +243,7 @@ describe('TransactionRunner', () => {
 
       it('should reject for non-retryable errors', done => {
         const fakeError = new Error('err') as grpc.ServiceError;
-        fakeError.code = grpc.status.DEADLINE_EXCEEDED;
+        fakeError.code = grpc.status.UNKNOWN;
 
         runFn.rejects(fakeError);
 
@@ -273,26 +273,6 @@ describe('TransactionRunner', () => {
         assert.strictEqual(delayStub.callCount, 1);
       });
 
-      it('should retry on UNKNOWN errors', async () => {
-        const fakeReturnValue = 12;
-        const fakeError = new Error('err') as grpc.ServiceError;
-        fakeError.code = grpc.status.UNKNOWN;
-
-        runFn.onCall(0).rejects(fakeError);
-        runFn.onCall(1).resolves(fakeReturnValue);
-
-        const delayStub = sandbox
-          .stub(runner, 'getNextDelay')
-          .withArgs(fakeError)
-          .returns(0);
-
-        const returnValue = await runner.run();
-
-        assert.strictEqual(returnValue, fakeReturnValue);
-        assert.strictEqual(runner.attempts, 1);
-        assert.strictEqual(delayStub.callCount, 1);
-      });
-
       it('should throw a DeadlineError if the timeout is exceeded', done => {
         const fakeError = new Error('err') as grpc.ServiceError;
         fakeError.code = grpc.status.ABORTED;
@@ -394,7 +374,7 @@ describe('TransactionRunner', () => {
 
         it('should return non-retryable request errors', async () => {
           const fakeError = new Error('err') as grpc.ServiceError;
-          fakeError.code = grpc.status.DEADLINE_EXCEEDED;
+          fakeError.code = grpc.status.UNKNOWN;
 
           fakeTransaction.request.withArgs(CONFIG).callsFake((_, callback) => {
             callback(fakeError);
@@ -428,25 +408,6 @@ describe('TransactionRunner', () => {
           assert.strictEqual(callbackStub.callCount, 1);
           assert.strictEqual(runFn.callCount, 2);
         });
-
-        it('should intercept UNKNOWN request errors', async () => {
-          const fakeError = new Error('err') as grpc.ServiceError;
-          fakeError.code = grpc.status.UNKNOWN;
-
-          fakeTransaction.request
-            .onCall(0)
-            .callsFake((_, callback) => callback(fakeError));
-
-          fakeTransaction.request.onCall(1).callsFake((_, callback) => {
-            callback(null);
-            setImmediate(() => fakeTransaction.emit('end'));
-          });
-
-          await runner.run();
-
-          assert.strictEqual(callbackStub.callCount, 1);
-          assert.strictEqual(runFn.callCount, 2);
-        });
       });
 
       describe('transaction streams', () => {
@@ -479,7 +440,7 @@ describe('TransactionRunner', () => {
           fakeTransaction.requestStream.withArgs(CONFIG).returns(fakeStream);
 
           const fakeError = new Error('err') as grpc.ServiceError;
-          fakeError.code = grpc.status.DEADLINE_EXCEEDED;
+          fakeError.code = grpc.status.UNKNOWN;
 
           runFn.callsFake((err, transaction) => {
             assert.ifError(err);
@@ -526,39 +487,6 @@ describe('TransactionRunner', () => {
           runner.run().catch(done);
           setImmediate(() => badStream.destroy(fakeError));
         });
-
-        it('should intercept UNKNOWN streaming errors', done => {
-          const badStream = through.obj();
-          const goodStream = through.obj();
-
-          const fakeError = new Error('err') as grpc.ServiceError;
-          fakeError.code = grpc.status.UNKNOWN;
-
-          const fakeData = [{a: 'b'}, {c: 'd'}, {e: 'f'}];
-          fakeData.forEach(data => goodStream.push(data));
-          goodStream.push(null);
-
-          fakeTransaction.requestStream.onCall(0).returns(badStream);
-          fakeTransaction.requestStream.onCall(1).returns(goodStream);
-
-          runFn.callsFake((err, transaction) => {
-            assert.ifError(err);
-
-            transaction
-              .requestStream(CONFIG)
-              .on('error', done)
-              .pipe(
-                concat(data => {
-                  assert.deepStrictEqual(data, fakeData);
-                  assert.strictEqual(runFn.callCount, 2);
-                  done();
-                })
-              );
-          });
-
-          runner.run().catch(done);
-          setImmediate(() => badStream.destroy(fakeError));
-        });
       });
     });
   });