Skip to content

refactor: replace retry with internal async backoff retry loop #10663

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
joehan wants to merge 1 commit into
base: main
Choose a base branch
from
+97 −120
Open

refactor: replace retry with internal async backoff retry loop #10663

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 5 additions & 15 deletions npm-shrinkwrap.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,6 @@
"portfinder": "^1.0.32",
"progress": "^2.0.3",
"proxy-agent": "^6.3.0",
"retry": "^0.13.1",
"semver": "^7.5.2",
"sql-formatter": "^15.3.0",
"stream-chain": "^2.2.4",
Expand Down Expand Up @@ -214,7 +213,6 @@
"@types/progress": "^2.0.3",
"@types/react": "^18.2.58",
"@types/react-dom": "^18.2.19",
"@types/retry": "^0.12.1",
"@types/semver": "^6.0.0",
"@types/sinon": "^9.0.10",
"@types/sinon-chai": "^3.2.2",
Expand Down
195 changes: 92 additions & 103 deletions src/apiv2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import { AbortSignal } from "abort-controller";
import AbortController, { AbortSignal } from "abort-controller";
import { URL, URLSearchParams } from "url";
import { Readable } from "stream";
import { ProxyAgent } from "proxy-agent";
import * as retry from "retry";
import AbortController from "abort-controller";
import fetch, { HeadersInit, Response, RequestInit, Headers } from "node-fetch";
import util from "util";

Expand All @@ -13,11 +11,12 @@
import { logger } from "./logger";
import { responseToError } from "./responseToError";
import * as FormData from "form-data";
import { sleep } from "./utils";

// Using import would require resolveJsonModule, which seems to break the
// build/output format.
const pkg = require("../package.json");

Check warning on line 18 in src/apiv2.ts

View workflow job for this annotation

GitHub Actions / lint (24) 

Require statement not part of import statement

Check warning on line 18 in src/apiv2.ts

View workflow job for this annotation

GitHub Actions / lint (24) 

Unsafe assignment of an `any` value
const CLI_VERSION: string = pkg.version;

Check warning on line 19 in src/apiv2.ts

View workflow job for this annotation

GitHub Actions / lint (24) 

Unsafe member access .version on an `any` value

Check warning on line 19 in src/apiv2.ts

View workflow job for this annotation

GitHub Actions / lint (24) 

Unsafe assignment of an `any` value

export const standardHeaders: () => Record<string, string> = () => {
const agent = detectAIAgent();
Expand Down Expand Up @@ -134,7 +133,7 @@

/**
* Gets a singleton access token
* @returns An access token

Check warning on line 136 in src/apiv2.ts

View workflow job for this annotation

GitHub Actions / lint (24) 

Invalid JSDoc tag (preference). Replace "returns" JSDoc tag with "return"
*/
export async function getAccessToken(): Promise<string> {
const valid = auth.haveValidTokens(refreshToken, []);
Expand Down Expand Up @@ -239,7 +238,7 @@
* Makes a request as specified by the options.
* By default, this will:
* - use content-type: application/json
* - assume the HTTP GET method

Check warning on line 241 in src/apiv2.ts

View workflow job for this annotation

GitHub Actions / lint (24) 

Expected only 0 line after block description
*
* @example
* const res = apiv2.request<ResourceType>({
Expand Down Expand Up @@ -278,8 +277,8 @@
}
try {
return await this.doRequest<ReqT, ResT>(internalReqOptions);
} catch (thrown: any) {

Check warning on line 280 in src/apiv2.ts

View workflow job for this annotation

GitHub Actions / lint (24) 

Unexpected any. Specify a different type
const originalErrorMessage = thrown.original?.message || thrown.message || "";

Check warning on line 281 in src/apiv2.ts

View workflow job for this annotation

GitHub Actions / lint (24) 

Unsafe member access .message on an `any` value

Check warning on line 281 in src/apiv2.ts

View workflow job for this annotation

GitHub Actions / lint (24) 

Unsafe member access .original on an `any` value

Check warning on line 281 in src/apiv2.ts

View workflow job for this annotation

GitHub Actions / lint (24) 

Unsafe assignment of an `any` value
if (originalErrorMessage.includes(CLI_OAUTH_PROJECT_NUMBER)) {
// Error messages mentioning the CLI's OAuth project number should not be shared with end users,
// since they will never be actionable for them. If we do display them, support gets a bunch of tickets asking for quota
Expand Down Expand Up @@ -414,116 +413,106 @@
fetchOptions.body = JSON.stringify(options.body);
}

// TODO(bkendall): Refactor this to use Throttler _or_ refactor Throttle to use `retry`.
const operationOptions: retry.OperationOptions = {
retries: options.retryCodes?.length ? 1 : 2,
minTimeout: 1 * 1000,
maxTimeout: 5 * 1000,
};
if (typeof options.retries === "number") {
operationOptions.retries = options.retries;
}
if (typeof options.retryMinTimeout === "number") {
operationOptions.minTimeout = options.retryMinTimeout;
}
if (typeof options.retryMaxTimeout === "number") {
operationOptions.maxTimeout = options.retryMaxTimeout;
}
const operation = retry.operation(operationOptions);

return await new Promise<ClientResponse<ResT>>((resolve, reject) => {
// eslint-disable-next-line @typescript-eslint/no-misused-promises
operation.attempt(async (currentAttempt): Promise<void> => {
let res: Response;
let body: ResT;
const retries = options.retries ?? (options.retryCodes?.length ? 1 : 2);
const minTimeout = options.retryMinTimeout ?? 1000;
const maxTimeout = options.retryMaxTimeout ?? 5000;

let currentAttempt = 1;
let timeout = minTimeout;

while (true) {
let res: Response;
let body: ResT;
try {
if (currentAttempt > 1) {
logger.debug(
`*** [apiv2] Attempting the request again. Attempt number ${currentAttempt}`,
);
}
this.logRequest(options);
try {
if (currentAttempt > 1) {
logger.debug(
`*** [apiv2] Attempting the request again. Attempt number ${currentAttempt}`,
);
res = await fetch(fetchURL, fetchOptions);

Check failure

Code scanning / CodeQL

Server-side request forgery Critical

The
URL
Loading
of this request depends on a
user-provided value
Loading
.
} catch (thrown: any) {
const err = thrown instanceof Error ? thrown : new Error(thrown);
Comment on lines +435 to +436

@gemini-code-assist gemini-code-assist Bot Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Avoid using any as an escape hatch per the TypeScript guidelines in the repository style guide. We should type the caught variable as unknown and use a safe conversion to Error to ensure type safety.

Suggested change
} catch (thrown: any) {
const err = thrown instanceof Error ? thrown : new Error(thrown);
} catch (thrown: unknown) {
const err = thrown instanceof Error ? thrown : new Error(String(thrown));
References
  1. Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)

logger.debug(
`*** [apiv2] error from fetch(${fetchURL}, ${JSON.stringify(fetchOptions)}): ${err}`,
);
const isAbortError = err.name.includes("AbortError");
if (isAbortError) {
throw new FirebaseError(`Timeout reached making request to ${fetchURL}`, {
original: err,
});
}
this.logRequest(options);
try {
res = await fetch(fetchURL, fetchOptions);
} catch (thrown: any) {
const err = thrown instanceof Error ? thrown : new Error(thrown);
logger.debug(
`*** [apiv2] error from fetch(${fetchURL}, ${JSON.stringify(fetchOptions)}): ${err}`,
);
const isAbortError = err.name.includes("AbortError");
if (isAbortError) {
throw new FirebaseError(`Timeout reached making request to ${fetchURL}`, {
original: err,
});
}
throw new FirebaseError(`Failed to make request to ${fetchURL}`, { original: err });
} finally {
// If we succeed or failed, clear the timeout.
if (reqTimeout) {
clearTimeout(reqTimeout);
}
throw new FirebaseError(`Failed to make request to ${fetchURL}`, { original: err });
} finally {
// If we succeed or failed, clear the timeout.
if (reqTimeout) {
clearTimeout(reqTimeout);
}
}

if (options.responseType === "json") {
const text = await res.text();
// Some responses, such as 204 and occasionally 202s don't have
// any content. We can't just rely on response code (202 may have conent)
// and unfortuantely res.length is unreliable (many requests return zero).
if (!text.length) {
body = undefined as unknown as ResT;
} else {
try {
body = JSON.parse(text) as ResT;
} catch (err: unknown) {
// JSON-parse errors are useless. Log the response for better debugging.
this.logResponse(res, text, options);
throw new FirebaseError(`Unable to parse JSON: ${err}`);
}
}
} else if (options.responseType === "xml") {
body = (await res.text()) as unknown as ResT;
} else if (options.responseType === "stream") {
body = res.body as unknown as ResT;
if (options.responseType === "json") {
const text = await res.text();
// Some responses, such as 204 and occasionally 202s don't have
// any content. We can't just rely on response code (202 may have content)
// and unfortunately res.length is unreliable (many requests return zero).
if (!text.length) {
body = undefined as unknown as ResT;
} else {
throw new FirebaseError(`Unable to interpret response. Please set responseType.`, {
exit: 2,
});
try {
body = JSON.parse(text) as ResT;
} catch (err: unknown) {
// JSON-parse errors are useless. Log the response for better debugging.
this.logResponse(res, text, options);
throw new FirebaseError(`Unable to parse JSON: ${err}`);
}
}
} catch (err: unknown) {
return err instanceof FirebaseError ? reject(err) : reject(new FirebaseError(`${err}`));
} else if (options.responseType === "xml") {
body = (await res.text()) as unknown as ResT;
} else if (options.responseType === "stream") {
body = res.body as unknown as ResT;
} else {
throw new FirebaseError(`Unable to interpret response. Please set responseType.`, {
exit: 2,
});
}
} catch (err: unknown) {
throw err instanceof FirebaseError ? err : new FirebaseError(`${err}`);
}
Comment on lines +479 to +481

@gemini-code-assist gemini-code-assist Bot Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When wrapping a generic caught error in a FirebaseError, using new FirebaseError(${err}) converts the error to a string (which includes the error name, e.g., "Error: message") and discards the original stack trace.

Instead, we should check if the caught error is an instance of Error and pass it as the original property to preserve the stack trace and original error details.

      } catch (err: unknown) {
        if (err instanceof FirebaseError) {
          throw err;
        }
        if (err instanceof Error) {
          throw new FirebaseError(err.message, { original: err });
        }
        throw new FirebaseError(String(err));
      }


this.logResponse(res, body, options);

if (res.status >= 400) {
if (res.status === 401 && this.opts.auth) {
// If we get a 401, access token is expired or otherwise invalid.
// Throw it away and get a new one. We check for validity before using
// tokens, so this should not happen.
logger.debug(
"Got a 401 Unauthenticated error for a call that required authentication. Refreshing tokens.",
);
setAccessToken();
setAccessToken(await getAccessToken());
}
if (options.retryCodes?.includes(res.status)) {
const err = responseToError({ statusCode: res.status }, body, fetchURL) || undefined;
if (operation.retry(err)) {
return;
}
}
if (!options.resolveOnHTTPError) {
return reject(responseToError({ statusCode: res.status }, body, fetchURL));
}
this.logResponse(res, body, options);

if (res.status >= 400) {
if (res.status === 401 && this.opts.auth) {
// If we get a 401, access token is expired or otherwise invalid.
// Throw it away and get a new one. We check for validity before using
// tokens, so this should not happen.
logger.debug(
"Got a 401 Unauthenticated error for a call that required authentication. Refreshing tokens.",
);
setAccessToken();
setAccessToken(await getAccessToken());
}
if (options.retryCodes?.includes(res.status) && currentAttempt <= retries) {
logger.debug(
`*** [apiv2] Retrying request on status code ${res.status}. Next attempt in ${timeout}ms.`,
);
await sleep(timeout);
currentAttempt++;
timeout = Math.min(timeout * 2, maxTimeout);
continue;
}
if (!options.resolveOnHTTPError) {
throw responseToError({ statusCode: res.status }, body, fetchURL);
}
}

resolve({
status: res.status,
response: res,
body,
});
});
});
return {
status: res.status,
response: res,
body,
};
}
}

private logRequest(options: InternalClientRequestOptions<unknown>): void {
Expand Down
Loading