From 42805b8765f0f40215977760c260dcd13e188553 Mon Sep 17 00:00:00 2001 From: Shivanee Persaud Date: Wed, 16 Sep 2026 15:07:38 -0700 Subject: [PATCH] fix(gax): stop retrying a call after the caller cancels it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `retryable` decides whether to retry purely on the error code returned by the in-flight call. Nothing records that the caller asked to cancel, so the abort that `cancel()` itself triggers is indistinguishable from a transport failure, and a transport that reports it with a retryable code gets retried. The consequence is worse than a redundant request. By the time the error arrives, `cancel()` has already run: it nulled `canceller` and cleared the retry timer, and the timer for the next attempt does not exist yet. So the retry is dispatched with nothing left that can stop it. It runs unbounded and the caller's callback is never invoked — the call hangs rather than reporting cancellation. Nothing on the gRPC path reports an abort with a retryable code today, so this is currently latent. The REST fallback reaches it as soon as transport failures are mapped to UNAVAILABLE, which is what #9346 does: cancelling a fallback call then dispatches a second request and never settles. Record the cancellation and check it before the retry decision. An error that already carries CANCELLED is passed through untouched, since the transport has already said everything worth saying; anything else is reported as CANCELLED with the original kept as `cause`. Fixing it here rather than in the transport keeps the guarantee where the retry decision is made, so it holds for any transport and any future error mapping, instead of depending on every transport resolving its own aborts correctly. The regression test hangs to the mocha timeout without the fix rather than failing an assertion, which is the symptom itself. Commit message for the retries.ts cancellation fix. --- core/packages/gax/src/normalCalls/retries.ts | 34 +++++++ core/packages/gax/test/unit/apiCallable.ts | 95 ++++++++++++++++++++ 2 files changed, 129 insertions(+) diff --git a/core/packages/gax/src/normalCalls/retries.ts b/core/packages/gax/src/normalCalls/retries.ts index 0ba7b31017cd..115653afef2c 100644 --- a/core/packages/gax/src/normalCalls/retries.ts +++ b/core/packages/gax/src/normalCalls/retries.ts @@ -68,6 +68,10 @@ export function retryable( return (argument: RequestType, callback: APICallback) => { let canceller: GRPCCallResult | null; let timeoutId: ReturnType | 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 @@ export function retryable( 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 @@ -194,6 +224,10 @@ export function retryable( 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); } diff --git a/core/packages/gax/test/unit/apiCallable.ts b/core/packages/gax/test/unit/apiCallable.ts index f7cfaed41485..0cef34818607 100644 --- a/core/packages/gax/test/unit/apiCallable.ts +++ b/core/packages/gax/test/unit/apiCallable.ts @@ -457,6 +457,101 @@ describe('Promise', () => { }, 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);