fix(gax): stop retrying a call after the caller cancels it - #9359
shivanee-p wants to merge 1 commit into
Conversation
`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. </CodeContent> <parameter name="Description">Commit message for the retries.ts cancellation fix.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a cancelled flag in retryable calls to prevent automatic retries when a request is explicitly cancelled by the caller, even if the transport reports a retryable error code. It also adds corresponding unit tests to verify this behavior. Feedback on the changes suggests using optional chaining (err?.code) when checking the error code to prevent potential null pointer exceptions or TypeScript compilation errors under strict null checks.
| 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) { |
There was a problem hiding this comment.
Since err can be null or undefined (as indicated by the use of non-null assertions like err!.code! on line 187), accessing err.code directly here will cause a TypeScript compilation error under strict null checks, or a runtime TypeError if err is indeed nullish.
Using optional chaining (err?.code) is safer and adheres to defensive programming practices.
| if (err.code === Status.CANCELLED) { | |
| if (err?.code === Status.CANCELLED) { |
1 participant
retryabledecides whether to retry purely on the error code returned by the in-flight call. The abort thatcancel()itself triggers is indistinguishable from a transport failure, and a transport that reports it with a retryable code gets retried.By the time the error arrives,
cancel()has already run: it nulledcancellerand cleared the retry timer, and the timer for the next attempt does not exist yet. So 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.