diff --git a/apps/backend/tests/status.up.test.js b/apps/backend/tests/status.up.test.js index 02b9a28b29c93..bcf4421d4b09e 100644 --- a/apps/backend/tests/status.up.test.js +++ b/apps/backend/tests/status.up.test.js @@ -209,7 +209,16 @@ describe("Test /api/status/up", () => { mock.onPost("https://api.github.com/graphql").networkError(); const { req, res } = faker({}, {}); - await up(req, res); + // the retryer sleeps through its transient-backoff schedule + // (2 PATs x [1s, 2s, 4s] + jitter) before giving up + vi.useFakeTimers(); + try { + const pending = up(req, res); + await vi.advanceTimersByTimeAsync(20_000); + await pending; + } finally { + vi.useRealTimers(); + } expect(res.setHeader).toHaveBeenCalledWith( "Content-Type", @@ -234,7 +243,16 @@ describe("Test /api/status/up", () => { mock.onPost("https://api.github.com/graphql").networkError(); const { req, res } = faker({}, {}); - await up(req, res); + // the retryer sleeps through its transient-backoff schedule + // (2 PATs x [1s, 2s, 4s] + jitter) before giving up + vi.useFakeTimers(); + try { + const pending = up(req, res); + await vi.advanceTimersByTimeAsync(20_000); + await pending; + } finally { + vi.useRealTimers(); + } expect(res.setHeader.mock.calls).toEqual([ ["Content-Type", "application/json"], diff --git a/packages/core/src/api/gist.js b/packages/core/src/api/gist.js index 67505392b76f7..d6eaafa5755b1 100644 --- a/packages/core/src/api/gist.js +++ b/packages/core/src/api/gist.js @@ -2,6 +2,7 @@ import { renderGistCard } from "../cards/gist.js"; import { findInvalidColor } from "../common/color.js"; import { MissingParamError, + describeError, retrieveSecondaryMessage, } from "../common/error.js"; import { parseBoolean } from "../common/ops.js"; @@ -102,6 +103,7 @@ export default async ( if (err instanceof Error) { return { status: "error - temporary", + error: describeError(err), content: renderError({ message: err.message, secondaryMessage: retrieveSecondaryMessage(err), diff --git a/packages/core/src/api/index.js b/packages/core/src/api/index.js index 87f6bfd9056cf..49eca1416968c 100644 --- a/packages/core/src/api/index.js +++ b/packages/core/src/api/index.js @@ -2,6 +2,7 @@ import { renderStatsCard } from "../cards/stats.js"; import { findInvalidColor } from "../common/color.js"; import { MissingParamError, + describeError, retrieveSecondaryMessage, } from "../common/error.js"; import { parseArray, parseBoolean } from "../common/ops.js"; @@ -171,6 +172,7 @@ export default async ( if (err instanceof Error) { return { status: "error - temporary", + error: describeError(err), content: renderError({ message: err.message, secondaryMessage: retrieveSecondaryMessage(err), diff --git a/packages/core/src/api/pin.js b/packages/core/src/api/pin.js index fe59500dfe228..190516671b834 100644 --- a/packages/core/src/api/pin.js +++ b/packages/core/src/api/pin.js @@ -2,6 +2,7 @@ import { renderRepoCard } from "../cards/repo.js"; import { findInvalidColor } from "../common/color.js"; import { MissingParamError, + describeError, retrieveSecondaryMessage, } from "../common/error.js"; import { parseArray, parseBoolean } from "../common/ops.js"; @@ -131,6 +132,7 @@ export default async ( if (err instanceof Error) { return { status: "error - temporary", + error: describeError(err), content: renderError({ message: err.message, secondaryMessage: retrieveSecondaryMessage(err), diff --git a/packages/core/src/api/top-langs.js b/packages/core/src/api/top-langs.js index 46e106eeb6cac..3a98140c2ba52 100644 --- a/packages/core/src/api/top-langs.js +++ b/packages/core/src/api/top-langs.js @@ -2,6 +2,7 @@ import { renderTopLanguages } from "../cards/top-languages.js"; import { findInvalidColor } from "../common/color.js"; import { MissingParamError, + describeError, retrieveSecondaryMessage, } from "../common/error.js"; import { parseArray, parseBoolean } from "../common/ops.js"; @@ -169,6 +170,7 @@ export default async ( if (err instanceof Error) { return { status: "error - temporary", + error: describeError(err), content: renderError({ message: err.message, secondaryMessage: retrieveSecondaryMessage(err), diff --git a/packages/core/src/api/wakatime.js b/packages/core/src/api/wakatime.js index 7d03c58e49813..051c314fbcc15 100644 --- a/packages/core/src/api/wakatime.js +++ b/packages/core/src/api/wakatime.js @@ -2,6 +2,7 @@ import { renderWakatimeCard } from "../cards/wakatime.js"; import { findInvalidColor } from "../common/color.js"; import { MissingParamError, + describeError, retrieveSecondaryMessage, } from "../common/error.js"; import { parseArray, parseBoolean } from "../common/ops.js"; @@ -116,6 +117,7 @@ export default async ({ if (err instanceof Error) { return { status: "error - temporary", + error: describeError(err), content: renderError({ message: err.message, secondaryMessage: retrieveSecondaryMessage(err), diff --git a/packages/core/src/common/error.ts b/packages/core/src/common/error.ts index 0cb3b0f8c5877..c62c14a95a96a 100644 --- a/packages/core/src/common/error.ts +++ b/packages/core/src/common/error.ts @@ -87,10 +87,26 @@ const retrieveSecondaryMessage = (err: Error): string | undefined => { : undefined; }; +/** + * Extract structured details from a caught error. + * + * Callers attach the result to API results as an optional `error` field. + * The `status` value itself stays stable, so exact comparisons in + * `apps/backend/router.js` and external callers keep working. + * + * @param err The caught error. + * @returns The error type and message. + */ +const describeError = (err: Error): { type?: string; message: string } => { + const type = "type" in err && typeof err.type === "string" ? err.type : ""; + return type ? { type, message: err.message } : { message: err.message }; +}; + export { CustomError, MissingParamError, SECONDARY_ERROR_MESSAGES, TRY_AGAIN_LATER, + describeError, retrieveSecondaryMessage, }; diff --git a/packages/core/src/common/retryer.ts b/packages/core/src/common/retryer.ts index 4b07f36466b70..912efb2b94e0c 100644 --- a/packages/core/src/common/retryer.ts +++ b/packages/core/src/common/retryer.ts @@ -28,6 +28,50 @@ function getRandomInt(max: number): number { return Math.floor(Math.random() * max); } +/** + * Delay before each transient retry of the same PAT. + * + * Transient failures are network-level errors (ECONNRESET, ETIMEDOUT, + * socket hang up) and retryable HTTP statuses. Token rotation stays + * separate from this backoff. + */ +const TRANSIENT_RETRY_DELAYS_MS = [1000, 2000, 4000]; + +/** Random extra wait added to each transient retry delay. */ +const TRANSIENT_RETRY_JITTER_MS = 250; + +/** + * HTTP statuses worth a same-token retry. Server-side blips only. + * 429 and rate-limit 403 answers skip quick retries entirely: hammering a + * limited token violates GitHub's Retry-After / reset guidance and can get + * the token blocked. Permanent statuses such as 401/404/422 stay outside + * this set. + */ +const RETRYABLE_HTTP_STATUS_CODES = new Set([502, 503, 504]); + +/** + * Wait for `ms` milliseconds. + */ +const sleep = (ms: number): Promise => { + return new Promise((resolve) => setTimeout(resolve, ms)); +}; + +/** + * Optional overrides for {@link retryer}, mainly for tests. + */ +interface RetryerOptions { + /** + * Delays between transient retries of one PAT. + * An empty array disables transient retries. + */ + transientRetryDelaysMs?: Array; +} + +/** + * Kind of the failure that caused the latest PAT rotation. + */ +type FailureKind = "transient" | "rate-limit" | "credential"; + /** * A fetcher's Axios response. `TData` is the shape of `response.data`, * which is intersected with {@link ResponseErrors} so the retryer can inspect @@ -55,6 +99,7 @@ const retryer = async ( fetcher: FetcherFunction, variables: Record, pat: string | null = null, + { transientRetryDelaysMs = TRANSIENT_RETRY_DELAYS_MS }: RetryerOptions = {}, ): Promise> => { const PATs = pat ? [{ name: "user PAT from database", value: pat }] @@ -65,61 +110,124 @@ const retryer = async ( } const startPAT = getRandomInt(PATs.length); + let lastTransientError: unknown = null; + // Kind of the most recent rotation. The final message reflects the last + // observed failure, not the first one. + let lastFailureKind: FailureKind | null = null; + for (let retries = 0; retries < PATs.length; retries++) { const currentPAT = PATs[(startPAT + retries) % PATs.length]; if (!currentPAT) { continue; } - try { - const response = await fetcher( - variables, - currentPAT.value, - // used in tests for faking rate limit - retries, - ); - - // react on both type and message-based rate-limit signals. - // https://github.com/anuraghazra/github-readme-stats/issues/4425 - const errors = response.data.errors; - const errorType = errors?.[0]?.type; - const errorMsg = errors?.[0]?.message ?? ""; - const isRateLimited = - (!!errors && errorType === "RATE_LIMITED") || - /rate limit/i.test(errorMsg); - - if (isRateLimited) { - logger.log(`${currentPAT.name} Failed due to rate limiting`); - } else { + // One transient retry per delay entry. The last pass has no delay left + // and rotates to the next PAT instead. + for (let attempt = 0; attempt <= transientRetryDelaysMs.length; attempt++) { + try { + const response = await fetcher( + variables, + currentPAT.value, + // used in tests for faking rate limit + retries, + ); + + // react on both type and message-based rate-limit signals. + // https://github.com/anuraghazra/github-readme-stats/issues/4425 + const errors = response.data.errors; + const errorType = errors?.[0]?.type; + const errorMsg = errors?.[0]?.message ?? ""; + const isRateLimited = + (!!errors && errorType === "RATE_LIMITED") || + /rate limit/i.test(errorMsg); + + if (isRateLimited) { + logger.log(`${currentPAT.name} Failed due to rate limiting`); + lastFailureKind = "rate-limit"; + break; // rotate to next PAT + } return response; - } - } catch (err) { - const e = err as { response?: FetcherResponse }; - - // network/unexpected error → let caller treat as failure - if (!e.response) { - throw err; - } + } catch (err) { + const e = err as { + response?: FetcherResponse; + isAxiosError?: boolean; + message?: unknown; + }; + + // Rate-limit responses never get quick retries. Quick retries would + // violate GitHub's Retry-After / reset guidance and can get the + // token blocked, so rotate to the next PAT instead. HTTP 429 and + // rate-limit 403 answers both carry this meaning. + const status = e.response?.status; + const carriesRateLimitMessage = /rate limit/i.test( + e.response?.data.message ?? "", + ); + const isRateLimitResponse = + status === 429 || (status === 403 && carriesRateLimitMessage); + + if (isRateLimitResponse && e.response) { + logger.log( + `${currentPAT.name} hit a rate limit (HTTP ${status}), rotating`, + ); + lastFailureKind = "rate-limit"; + break; // rotate to next PAT + } + + // Transient failure: network-level error without a response, or a + // retryable HTTP status. Retry the same PAT with backoff before + // rotating to the next token. + const isTransient = + (!e.response && e.isAxiosError === true) || + (!!e.response && RETRYABLE_HTTP_STATUS_CODES.has(e.response.status)); + + if (isTransient) { + lastTransientError = err; + lastFailureKind = "transient"; + const delayMs = transientRetryDelaysMs[attempt]; + if (delayMs !== undefined) { + logger.log( + `${currentPAT.name} transient failure (${String(e.message)}), retrying`, + ); + await sleep(delayMs + getRandomInt(TRANSIENT_RETRY_JITTER_MS)); + continue; + } + break; // retries exhausted → rotate to next PAT + } + + // non-axios errors are bugs, not transient failures + if (!e.response) { + throw err; + } + + // also checking for bad credentials if any tokens gets invalidated + const message = e.response.data.message; + const isBadCredential = message === "Bad credentials"; + const isAccountSuspended = + message === "Sorry. Your account was suspended."; + + if (isBadCredential || isAccountSuspended) { + logger.log(`${currentPAT.name} Failed due to bad credentials`); + lastFailureKind = "credential"; + break; // rotate to next PAT + } - // also checking for bad credentials if any tokens gets invalidated - const message = e.response.data.message; - const isBadCredential = message === "Bad credentials"; - const isAccountSuspended = - message === "Sorry. Your account was suspended."; - - if (isBadCredential || isAccountSuspended) { - logger.log(`${currentPAT.name} Failed due to bad credentials`); - } else { // HTTP error with a response → return it for caller-side handling return e.response; } } } - throw new CustomError( - "Downtime due to GitHub API rate limiting", - CustomError.MAX_RETRY, - ); + // The final message reflects the last failure kind. Claiming "rate + // limiting" without a rate limit would mislead the reader. A missing kind + // means no PAT slot was usable; keep the historical message there. + let reason = "Downtime due to GitHub API rate limiting"; + if (lastFailureKind === "transient" && lastTransientError instanceof Error) { + reason = `GitHub API request failed after transient retries: ${lastTransientError.message}`; + } else if (lastFailureKind === "credential") { + reason = "GitHub API request failed due to invalid credentials"; + } + + throw new CustomError(reason, CustomError.MAX_RETRY); }; export { retryer }; diff --git a/packages/core/tests/describeError.test.ts b/packages/core/tests/describeError.test.ts new file mode 100644 index 0000000000000..d7ef3f67b6f25 --- /dev/null +++ b/packages/core/tests/describeError.test.ts @@ -0,0 +1,104 @@ +import axios from "axios"; +import MockAdapter from "axios-mock-adapter"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import api from "../src/api/index.js"; +import { + CustomError, + MissingParamError, + describeError, +} from "../src/common/error.js"; + +vi.mock(import("../src/common/log.js"), async () => { + const { createLoggerMock } = await import("./utils.js"); + return createLoggerMock(); +}); + +// The handler is a JS function whose inferred options type spells out every +// query parameter. Tests pass partial query maps on purpose and only assert +// the parts of the result they care about, so they call through this +// deliberately narrowed view instead of the raw inferred signature. +interface TestApiResult { + status: string; + error?: { + type?: string; + message?: string; + }; + content: string; +} +const callApi = (options: Record): Promise => { + return api(options as Parameters[0]); +}; + +describe("Test describeError", () => { + it("should return message only for errors without a type", () => { + expect(describeError(new Error("boom"))).toStrictEqual({ + message: "boom", + }); + }); + + it("should return type and message for custom errors", () => { + expect( + describeError( + new CustomError( + "Downtime due to GitHub API rate limiting", + CustomError.MAX_RETRY, + ), + ), + ).toStrictEqual({ + type: CustomError.MAX_RETRY, + message: "Downtime due to GitHub API rate limiting", + }); + }); + + it("should omit the type for missing param errors", () => { + expect(describeError(new MissingParamError(["username"]))).toStrictEqual({ + message: + 'Missing params "username" make sure you pass the parameters in URL', + }); + }); +}); + +describe("Test API result error contract", () => { + let mock: MockAdapter; + + beforeEach(() => { + mock = new MockAdapter(axios); + }); + + afterEach(() => { + mock.restore(); + }); + + it("stats handler should attach typed details on rate limit exhaustion", async () => { + mock.onPost("https://api.github.com/graphql").reply(200, { + errors: [{ type: "RATE_LIMITED" }], + }); + + const result = await callApi({ username: "octocat" }); + + // status keeps its exact value for comparisons in apps/backend/router.js + expect(result.status).toBe("error - temporary"); + expect(result).toMatchObject({ + status: "error - temporary", + error: { + type: CustomError.MAX_RETRY, + message: "Downtime due to GitHub API rate limiting", + }, + }); + }); + + it("stats handler should attach message-only details for missing params", async () => { + const result = await callApi({}); + const { error } = result as { + error?: { + type?: string; + message?: string; + }; + }; + + expect(result.status).toBe("error - temporary"); + expect(error).not.toHaveProperty("type"); + expect(error?.message).toContain('Missing params "username"'); + }); +}); diff --git a/packages/core/tests/retryer.test.ts b/packages/core/tests/retryer.test.ts index 99061efe98f11..90e37de08cdfc 100644 --- a/packages/core/tests/retryer.test.ts +++ b/packages/core/tests/retryer.test.ts @@ -44,6 +44,20 @@ const customFetcher = vi.fn((_variables: unknown, token: string) => { return Promise.resolve({ data: { token } }); }) as unknown as Fetcher; +const networkError = (): Error => { + return Object.assign(new Error("network error"), { + isAxiosError: true, + code: "ECONNRESET", + }); +}; + +const httpError = (status: number): Error => { + return Object.assign(new Error("http error"), { + isAxiosError: true, + response: { status, data: {} }, + }); +}; + describe("Test Retryer", () => { it("retryer should return value and have zero retries on first try", async () => { const res = await retryer(fetcher, {}); @@ -84,4 +98,169 @@ describe("Test Retryer", () => { ); expect(res).toStrictEqual({ data: { token: "user-pat-token" } }); }); + + it("retryer should retry transient network errors on the same PAT", async () => { + const fetcherTransient = vi + .fn() + .mockRejectedValueOnce(networkError()) + .mockResolvedValue({ data: "ok" }) as unknown as Fetcher; + + const res = await retryer(fetcherTransient, {}, "user-pat-token", { + transientRetryDelaysMs: [0], + }); + + expect(fetcherTransient).toHaveBeenCalledTimes(2); + expect(res).toStrictEqual({ data: "ok" }); + }); + + it("retryer should retry retryable HTTP statuses on the same PAT", async () => { + const fetcher502 = vi + .fn() + .mockRejectedValueOnce(httpError(502)) + .mockResolvedValue({ data: "ok" }) as unknown as Fetcher; + + const res = await retryer(fetcher502, {}, "user-pat-token", { + transientRetryDelaysMs: [0], + }); + + expect(fetcher502).toHaveBeenCalledTimes(2); + expect(res).toStrictEqual({ data: "ok" }); + }); + + it("retryer should not retry non-retryable HTTP statuses", async () => { + const response = { status: 404, data: { message: "Not Found" } }; + const fetcher404 = vi.fn().mockRejectedValue( + Object.assign(new Error("http error"), { + isAxiosError: true, + response, + }), + ); + + const res = await retryer( + fetcher404 as unknown as Fetcher, + {}, + "user-pat-token", + { transientRetryDelaysMs: [0] }, + ); + + expect(fetcher404).toHaveBeenCalledTimes(1); + expect(res).toStrictEqual(response); + }); + + it("retryer should throw non-axios errors immediately without retrying", async () => { + const fetcherBug = vi + .fn() + .mockRejectedValue(new TypeError("boom")) as unknown as Fetcher; + + await expect( + retryer(fetcherBug, {}, "user-pat-token", { + transientRetryDelaysMs: [0], + }), + ).rejects.toThrow("boom"); + + expect(fetcherBug).toHaveBeenCalledTimes(1); + }); + + it("retryer should mention the last transient error when retries are exhausted", async () => { + const fetcherAlwaysFails = vi.fn().mockRejectedValue(networkError()); + + await expect( + retryer(fetcherAlwaysFails as unknown as Fetcher, {}, "user-pat-token", { + transientRetryDelaysMs: [], + }), + ).rejects.toThrow( + "GitHub API request failed after transient retries: network error", + ); + + expect(fetcherAlwaysFails).toHaveBeenCalledTimes(1); + }); + + it("retryer should not claim rate limiting when only transient errors occurred", async () => { + const fetcherAlwaysFails = vi.fn().mockRejectedValue(httpError(503)); + + const promise = retryer( + fetcherAlwaysFails as unknown as Fetcher, + {}, + "user-pat-token", + { transientRetryDelaysMs: [] }, + ); + + await expect(promise).rejects.toThrow( + /GitHub API request failed after transient retries/, + ); + await expect(promise).rejects.not.toThrow(/rate limiting/i); + }); + + it("retryer should report rate limiting when the last rotation hit a rate limit", async () => { + // pin the rotation start to the first PAT + const randomSpy = vi.spyOn(Math, "random").mockReturnValue(0); + const fetcherMixed = vi.fn((_vars, _token, retries) => { + if (retries === 0) { + return Promise.reject(networkError()); + } + return Promise.resolve({ data: { errors: [{ type: "RATE_LIMITED" }] } }); + }) as unknown as Fetcher; + + await expect( + retryer(fetcherMixed, {}, undefined, { transientRetryDelaysMs: [] }), + ).rejects.toThrow("Downtime due to GitHub API rate limiting"); + + expect(fetcherMixed).toHaveBeenCalledTimes(2); + randomSpy.mockRestore(); + }); + + it("retryer should report credential failure when the last rotation had bad credentials", async () => { + // pin the rotation start to the first PAT + const randomSpy = vi.spyOn(Math, "random").mockReturnValue(0); + const fetcherMixed = vi.fn((_vars, _token, retries) => { + if (retries === 0) { + return Promise.reject(networkError()); + } + // GitHub answers 401 for bad credentials, so axios rejects + return Promise.reject( + Object.assign(new Error("http error"), { + isAxiosError: true, + response: { status: 401, data: { message: "Bad credentials" } }, + }), + ); + }) as unknown as Fetcher; + + await expect( + retryer(fetcherMixed, {}, undefined, { transientRetryDelaysMs: [] }), + ).rejects.toThrow("GitHub API request failed due to invalid credentials"); + + randomSpy.mockRestore(); + }); + + it("retryer should not quick-retry HTTP 429 and report rate limiting", async () => { + const fetcher429 = vi.fn().mockRejectedValue(httpError(429)); + + await expect( + retryer(fetcher429 as unknown as Fetcher, {}, "user-pat-token", { + transientRetryDelaysMs: [0], + }), + ).rejects.toThrow("Downtime due to GitHub API rate limiting"); + + expect(fetcher429).toHaveBeenCalledTimes(1); + }); + + it("retryer should treat a rate-limit 403 like a rate limit", async () => { + const fetcher403 = vi.fn().mockRejectedValue( + Object.assign(new Error("http error"), { + isAxiosError: true, + response: { + status: 403, + data: { message: "API rate limit exceeded for ..." }, + }, + }), + ); + + await expect( + retryer(fetcher403 as unknown as Fetcher, {}, "user-pat-token", { + transientRetryDelaysMs: [0], + }), + ).rejects.toThrow("Downtime due to GitHub API rate limiting"); + + expect(fetcher403).toHaveBeenCalledTimes(1); + }); }); diff --git a/scripts/generate-profile-cards.mjs b/scripts/generate-profile-cards.mjs index ea95851a3d5a5..1df7916d4e166 100644 --- a/scripts/generate-profile-cards.mjs +++ b/scripts/generate-profile-cards.mjs @@ -61,7 +61,14 @@ const parseArgs = (args) => { const writeCard = async ({ handler, options, output, token }) => { const result = await handler(options, token); if (result.status !== "success") { - throw new Error(`Card generation failed with status: ${result.status}`); + const detail = [result.error?.type, result.error?.message] + .filter(Boolean) + .join(": "); + throw new Error( + detail + ? `Card generation failed (${result.status}): ${detail}` + : `Card generation failed with status: ${result.status}`, + ); } await mkdir(path.dirname(output), { recursive: true }); await writeFile(output, result.content, "utf8");