From 289a888eb2db614626e26c037b2a6c9011359a58 Mon Sep 17 00:00:00 2001 From: Joe Hanley Date: Mon, 15 Jun 2026 16:06:25 -0700 Subject: [PATCH] refactor: replace retry with internal async backoff retry loop ### Description Removed the external third-party `retry` and `@types/retry` dependencies and replaced them with a native async retry loop with exponential backoff inside `src/apiv2.ts`. ### Scenarios Tested - Run all `src/apiv2.spec.ts` unit tests successfully. ### Sample Commands - `npx mocha src/apiv2.spec.ts` --- npm-shrinkwrap.json | 20 ++--- package.json | 2 - src/apiv2.ts | 195 +++++++++++++++++++++----------------------- 3 files changed, 97 insertions(+), 120 deletions(-) diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index e48c43b2e05..f2fd890c3e9 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -62,7 +62,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", @@ -120,7 +119,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", @@ -5307,12 +5305,6 @@ "integrity": "sha512-60BCwRFOZCQhDncwQdxxeOEEkbc5dIMccYLwbxsS4TUNeVECQ/pBJ0j09mrHOl/JJvpRPGwO9SvE4nR2Nb/a4Q==", "dev": true }, - "node_modules/@types/retry": { - "version": "0.12.1", - "resolved": "https://registry.npmjs.org/@types/retry/-/retry-0.12.1.tgz", - "integrity": "sha512-xoDlM2S4ortawSWORYqsdU+2rxdh4LRW9ytc3zmT37RIKQh6IHyKwwtKhKis9ah8ol07DCkZxPt8BBvPjC6v4g==", - "dev": true - }, "node_modules/@types/scheduler": { "version": "0.16.2", "resolved": "https://registry.npmjs.org/@types/scheduler/-/scheduler-0.16.2.tgz", @@ -18417,6 +18409,8 @@ "version": "0.13.1", "resolved": "https://registry.npmjs.org/retry/-/retry-0.13.1.tgz", "integrity": "sha512-XQBQ3I8W1Cge0Seh+6gjj03LbmRFWuoszgK9ooCpwYIrhhoO80pfq4cUkU5DkknwfOfFteRwlZ56PYOGYyFWdg==", + "dev": true, + "optional": true, "engines": { "node": ">= 4" } @@ -25690,12 +25684,6 @@ "integrity": "sha512-60BCwRFOZCQhDncwQdxxeOEEkbc5dIMccYLwbxsS4TUNeVECQ/pBJ0j09mrHOl/JJvpRPGwO9SvE4nR2Nb/a4Q==", "dev": true }, - "@types/retry": { - "version": "0.12.1", - "resolved": "https://registry.npmjs.org/@types/retry/-/retry-0.12.1.tgz", - "integrity": "sha512-xoDlM2S4ortawSWORYqsdU+2rxdh4LRW9ytc3zmT37RIKQh6IHyKwwtKhKis9ah8ol07DCkZxPt8BBvPjC6v4g==", - "dev": true - }, "@types/scheduler": { "version": "0.16.2", "resolved": "https://registry.npmjs.org/@types/scheduler/-/scheduler-0.16.2.tgz", @@ -35228,7 +35216,9 @@ "retry": { "version": "0.13.1", "resolved": "https://registry.npmjs.org/retry/-/retry-0.13.1.tgz", - "integrity": "sha512-XQBQ3I8W1Cge0Seh+6gjj03LbmRFWuoszgK9ooCpwYIrhhoO80pfq4cUkU5DkknwfOfFteRwlZ56PYOGYyFWdg==" + "integrity": "sha512-XQBQ3I8W1Cge0Seh+6gjj03LbmRFWuoszgK9ooCpwYIrhhoO80pfq4cUkU5DkknwfOfFteRwlZ56PYOGYyFWdg==", + "dev": true, + "optional": true }, "retry-request": { "version": "5.0.2", diff --git a/package.json b/package.json index 182110a90c2..e28c425fa58 100644 --- a/package.json +++ b/package.json @@ -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", @@ -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", diff --git a/src/apiv2.ts b/src/apiv2.ts index c9b716f9609..4dcf3aa0b8d 100644 --- a/src/apiv2.ts +++ b/src/apiv2.ts @@ -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"; @@ -13,6 +11,7 @@ import { isFirebaseMcp, detectAIAgent } from "./env"; 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. @@ -414,116 +413,106 @@ export class Client { 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>((resolve, reject) => { - // eslint-disable-next-line @typescript-eslint/no-misused-promises - operation.attempt(async (currentAttempt): Promise => { - 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); + } 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, + }); } - 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}`); + } - 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): void {