fix(gax): stop retrying a call after the caller cancels it

core/packages/gax/src/normalCalls/retries.ts

@@ -68,6 +68,10 @@
   return (argument: RequestType, callback: APICallback) => {
     let canceller: GRPCCallResult | null;
     let timeoutId: ReturnType<typeof setTimeout> | null;
+    // Set by `cancel()` below. The retry decision is made purely on the error
+    // code, which cannot distinguish a transport failure from the abort this
+    // call issued itself, so the caller's intent has to be recorded.
+    let cancelled = false;
     let now = new Date();
     let deadline: number;
     if (retry.backoffSettings.totalTimeoutMillis) {
@@ -152,6 +156,32 @@
           return;
         }
         canceller = null;
+        // The caller cancelled, so this error is the consequence of the abort
+        // issued just above, whatever the transport chose to report it as.
+        //
+        // Without this, the decision below is made on the error code alone. A
+        // transport that reports an aborted request as a retryable code — the
+        // REST fallback reports transport failures as UNAVAILABLE — schedules
+        // another attempt, which discards the cancellation. Worse, `cancel()`
+        // has already run: `canceller` is null and the timer it would have
+        // cleared has not been created yet, so nothing remains that can stop
+        // the new attempt. It runs unbounded and the caller's callback is
+        // never invoked.
+        if (cancelled) {
+          // A transport that already reported the abort as CANCELLED has said
+          // everything worth saying; re-wrapping would only bury it.
+          if (err.code === Status.CANCELLED) {
+            callback(err);
+            return;
+          }
+          const error = new GoogleError(
+            'cancelled' + errorDetailsSuffix(errorsEncountered),
+            {cause: err},
+          );
+          error.code = Status.CANCELLED;
+          callback(error);
+          return;
+        }
         if (
           retry.retryCodes.length > 0 &&
           retry.retryCodes.indexOf(err!.code!) < 0
@@ -175,8 +205,8 @@
         }
       });
       if (canceller instanceof Promise) {
         canceller.catch(err => {
           callback(new GoogleError(err));
         });
       }
     }
@@ -194,6 +224,10 @@
 
     return {
       cancel() {
+        // Recorded before aborting: `canceller.cancel()` can drive the
+        // transport's callback synchronously, and the guard there has to see
+        // this already set.
+        cancelled = true;
         if (timeoutId) {
           clearTimeout(timeoutId);
         }

core/packages/gax/test/unit/apiCallable.ts

@@ -375,9 +375,9 @@
         assert.ok(Array.isArray(response));
         assert.strictEqual(response[0], 42);
         assert.ok(deadlineArg);
         return done();
       })
       .catch(done);
   });
 
   it('emits error on rejected promise', async () => {
@@ -405,12 +405,12 @@
     const promise = (apiCall as any)(null);
     promise
       .then(() => {
         return done(new Error('should not reach'));
       })
       .catch((err: {code: number}) => {
         assert(err instanceof GoogleError);
         assert.strictEqual(err.code, status.CANCELLED);
         done();
       });
     promise.cancel();
   });
@@ -445,18 +445,113 @@
     const promise = (apiCall as any)(null);
     promise
       .then(() => {
         return done(new Error('should not reach'));
       })
       .catch(() => {
         assert(callCount < 4);
         done();
       })
       .catch(done);
     setTimeout(() => {
       promise.cancel();
     }, 15);
   });
 
+  it('does not retry a cancelled call whose error is retryable', done => {
+    // The REST fallback reports any transport-level failure as UNAVAILABLE,
+    // including the abort that `cancel()` itself triggers, so a cancelled call
+    // arrives here carrying a retryable code. Retrying it would discard the
+    // cancellation, and because `cancel()` has already run nothing remains that
+    // could stop the new attempt: it would run unbounded and this promise would
+    // never settle. Before the fix this test times out rather than failing.
+    const retryOptions = gax.createRetryOptions(
+      [status.UNAVAILABLE],
+      gax.createBackoffSettings(1, 1, 1, 1000, 1, 1000, 5000),
+    );
+
+    let callCount = 0;
+    function func(
+      argument: {},
+      metadata: {},
+      options: {},
+      callback: Function,
+    ) {
+      callCount++;
+      // Completes only when cancelled, like a request to an endpoint that
+      // accepted the connection and then went quiet.
+      return function cancelFunc() {
+        const err = new GoogleError('The operation was aborted.');
+        err.code = status.UNAVAILABLE;
+        callback(err);
+      };
+    }
+
+    const apiCall = createApiCall(func, {
+      settings: {retry: retryOptions},
+      returnCancelFunc: true,
+    });
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    const promise = (apiCall as any)(null);
+    promise
+      .then(() => done(new Error('should not reach')))
+      .catch((err: GoogleError) => {
+        try {
+          assert.strictEqual(err.code, status.CANCELLED);
+          assert.strictEqual(
+            callCount,
+            1,
+            `expected no retry after cancel, saw ${callCount} invocations`,
+          );
+          done();
+        } catch (e) {
+          done(e);
+        }
+      });
+    setTimeout(() => promise.cancel(), 10);
+  });
+
+  it('keeps a CANCELLED reported by the transport intact', done => {
+    // A transport that already resolved the abort to CANCELLED has said
+    // everything worth saying, so it should not be re-wrapped.
+    const retryOptions = gax.createRetryOptions(
+      [status.UNAVAILABLE],
+      gax.createBackoffSettings(1, 1, 1, 1000, 1, 1000, 5000),
+    );
+
+    const transportError = new GoogleError('cancelled by the transport');
+    transportError.code = status.CANCELLED;
+
+    function func(
+      argument: {},
+      metadata: {},
+      options: {},
+      callback: Function,
+    ) {
+      return function cancelFunc() {
+        callback(transportError);
+      };
+    }
+
+    const apiCall = createApiCall(func, {
+      settings: {retry: retryOptions},
+      returnCancelFunc: true,
+    });
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    const promise = (apiCall as any)(null);
+    promise
+      .then(() => done(new Error('should not reach')))
+      .catch((err: GoogleError) => {
+        try {
+          assert.strictEqual(err, transportError);
+          assert.strictEqual(err.message, 'cancelled by the transport');
+          done();
+        } catch (e) {
+          done(e);
+        }
+      });
+    setTimeout(() => promise.cancel(), 10);
+  });
+
   it('does not return promise when callback is supplied', done => {
     function func(argument: {}, metadata: {}, options: {}, callback: Function) {
       callback(null, 42);