From cd34d4ed3889fe21d5b5d4f435697b0ab9c28ba8 Mon Sep 17 00:00:00 2001 From: luojiyin Date: Mon, 24 Aug 2026 09:43:47 +0800 Subject: [PATCH 1/9] fix(retryer): retry transient network errors with backoff --- apps/backend/tests/status.up.test.js | 22 +++- packages/core/src/common/retryer.ts | 146 ++++++++++++++++++++------- packages/core/tests/retryer.test.ts | 88 ++++++++++++++++ 3 files changed, 216 insertions(+), 40 deletions(-) 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/common/retryer.ts b/packages/core/src/common/retryer.ts index 4b07f36466b70..21da8d9cbc34d 100644 --- a/packages/core/src/common/retryer.ts +++ b/packages/core/src/common/retryer.ts @@ -28,6 +28,42 @@ 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. + * 401/404/422 are permanent failures, so they stay outside this set. + */ +const RETRYABLE_HTTP_STATUS_CODES = new Set([429, 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; +} + /** * 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 +91,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,51 +102,82 @@ const retryer = async ( } const startPAT = getRandomInt(PATs.length); + let lastTransientError: unknown = 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`); + 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; - } - - // 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."; + } catch (err) { + const e = err as { + response?: FetcherResponse; + isAxiosError?: boolean; + message?: unknown; + }; + + // 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; + 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`); + break; // rotate to next PAT + } - 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; } @@ -117,7 +185,9 @@ const retryer = async ( } throw new CustomError( - "Downtime due to GitHub API rate limiting", + lastTransientError instanceof Error + ? `Downtime due to GitHub API rate limiting (last transient error: ${lastTransientError.message})` + : "Downtime due to GitHub API rate limiting", CustomError.MAX_RETRY, ); }; diff --git a/packages/core/tests/retryer.test.ts b/packages/core/tests/retryer.test.ts index 99061efe98f11..d13b0a833c563 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,78 @@ 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("last transient error: network error"); + + expect(fetcherAlwaysFails).toHaveBeenCalledTimes(1); + }); }); From c5867f800aa5751543506d7bc5329eec5b0eb4e8 Mon Sep 17 00:00:00 2001 From: luojiyin Date: Mon, 24 Aug 2026 10:15:41 +0800 Subject: [PATCH 2/9] feat(api,error): expose structured error details in API results --- packages/core/src/api/gist.js | 2 ++ packages/core/src/api/index.js | 2 ++ packages/core/src/api/pin.js | 2 ++ packages/core/src/api/top-langs.js | 2 ++ packages/core/src/api/wakatime.js | 2 ++ packages/core/src/common/error.ts | 25 +++++++++++++++++++++++++ scripts/generate-profile-cards.mjs | 9 ++++++++- 7 files changed, 43 insertions(+), 1 deletion(-) 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..d1095e5c532ae 100644 --- a/packages/core/src/common/error.ts +++ b/packages/core/src/common/error.ts @@ -87,10 +87,35 @@ const retrieveSecondaryMessage = (err: Error): string | undefined => { : undefined; }; +/** + * Structured details of a caught error for API results. + */ +export interface ErrorDetails { + /** Error type such as `MAX_RETRY`. Absent when the error has no type. */ + type?: string; + message: string; +} + +/** + * 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): ErrorDetails => { + 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/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"); From a9989217db6f5747e63b430f158a29549b72fa75 Mon Sep 17 00:00:00 2001 From: luojiyin Date: Mon, 24 Aug 2026 10:23:27 +0800 Subject: [PATCH 3/9] fix(retryer): report transient exhaustion without claiming rate limiting --- packages/core/src/common/retryer.ts | 5 ++++- packages/core/tests/retryer.test.ts | 20 +++++++++++++++++++- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/packages/core/src/common/retryer.ts b/packages/core/src/common/retryer.ts index 21da8d9cbc34d..b454c5d0933a1 100644 --- a/packages/core/src/common/retryer.ts +++ b/packages/core/src/common/retryer.ts @@ -184,9 +184,12 @@ const retryer = async ( } } + // Rate-limit rotation exhaustion keeps the historical message. Transient + // exhaustion reports the real cause, since claiming "rate limiting" would + // mislead when no rate limit was observed. throw new CustomError( lastTransientError instanceof Error - ? `Downtime due to GitHub API rate limiting (last transient error: ${lastTransientError.message})` + ? `GitHub API request failed after transient retries: ${lastTransientError.message}` : "Downtime due to GitHub API rate limiting", CustomError.MAX_RETRY, ); diff --git a/packages/core/tests/retryer.test.ts b/packages/core/tests/retryer.test.ts index d13b0a833c563..ebbbedb642184 100644 --- a/packages/core/tests/retryer.test.ts +++ b/packages/core/tests/retryer.test.ts @@ -168,8 +168,26 @@ describe("Test Retryer", () => { retryer(fetcherAlwaysFails as unknown as Fetcher, {}, "user-pat-token", { transientRetryDelaysMs: [], }), - ).rejects.toThrow("last transient error: network error"); + ).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); + }); }); From 2d10fa94e49639dc4856159dedda1e3c461ed417 Mon Sep 17 00:00:00 2001 From: luojiyin Date: Mon, 24 Aug 2026 10:30:57 +0800 Subject: [PATCH 4/9] test(error): cover describeError and API result error contract --- packages/core/tests/describeError.test.ts | 85 +++++++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 packages/core/tests/describeError.test.ts diff --git a/packages/core/tests/describeError.test.ts b/packages/core/tests/describeError.test.ts new file mode 100644 index 0000000000000..60ab8531c16ba --- /dev/null +++ b/packages/core/tests/describeError.test.ts @@ -0,0 +1,85 @@ +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(); +}); + +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 api({ 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 api({}); + const { error } = result as { + error?: Partial>; + }; + + expect(result.status).toBe("error - temporary"); + expect(error).not.toHaveProperty("type"); + expect(error?.message).toContain('Missing params "username"'); + }); +}); From e92a3a24139f83c1409d4977244eb4bb5fed3070 Mon Sep 17 00:00:00 2001 From: luojiyin Date: Mon, 24 Aug 2026 10:43:21 +0800 Subject: [PATCH 5/9] fix(retryer): pick final error message from last failure kind --- packages/core/src/common/retryer.ts | 31 +++++++++++++++------- packages/core/tests/retryer.test.ts | 41 +++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 9 deletions(-) diff --git a/packages/core/src/common/retryer.ts b/packages/core/src/common/retryer.ts index b454c5d0933a1..dd817e8fb0899 100644 --- a/packages/core/src/common/retryer.ts +++ b/packages/core/src/common/retryer.ts @@ -64,6 +64,11 @@ interface RetryerOptions { 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 @@ -103,6 +108,9 @@ 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]; @@ -132,6 +140,7 @@ const retryer = async ( if (isRateLimited) { logger.log(`${currentPAT.name} Failed due to rate limiting`); + lastFailureKind = "rate-limit"; break; // rotate to next PAT } return response; @@ -151,6 +160,7 @@ const retryer = async ( if (isTransient) { lastTransientError = err; + lastFailureKind = "transient"; const delayMs = transientRetryDelaysMs[attempt]; if (delayMs !== undefined) { logger.log( @@ -175,6 +185,7 @@ const retryer = async ( if (isBadCredential || isAccountSuspended) { logger.log(`${currentPAT.name} Failed due to bad credentials`); + lastFailureKind = "credential"; break; // rotate to next PAT } @@ -184,15 +195,17 @@ const retryer = async ( } } - // Rate-limit rotation exhaustion keeps the historical message. Transient - // exhaustion reports the real cause, since claiming "rate limiting" would - // mislead when no rate limit was observed. - throw new CustomError( - lastTransientError instanceof Error - ? `GitHub API request failed after transient retries: ${lastTransientError.message}` - : "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: all GitHub tokens were rejected"; + } + + throw new CustomError(reason, CustomError.MAX_RETRY); }; export { retryer }; diff --git a/packages/core/tests/retryer.test.ts b/packages/core/tests/retryer.test.ts index ebbbedb642184..dcd99efa3c0aa 100644 --- a/packages/core/tests/retryer.test.ts +++ b/packages/core/tests/retryer.test.ts @@ -190,4 +190,45 @@ describe("Test Retryer", () => { ); 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("all GitHub tokens were rejected"); + + randomSpy.mockRestore(); + }); }); From 3eb3f231f9de938f911276f25a7ea998859b0cfd Mon Sep 17 00:00:00 2001 From: luojiyin Date: Mon, 24 Aug 2026 11:00:53 +0800 Subject: [PATCH 6/9] fix(retryer): rotate on 429/rate-limit 403 and report last failure kind --- packages/core/src/common/retryer.ts | 30 +++++++++++++++++++++---- packages/core/tests/retryer.test.ts | 34 ++++++++++++++++++++++++++++- 2 files changed, 59 insertions(+), 5 deletions(-) diff --git a/packages/core/src/common/retryer.ts b/packages/core/src/common/retryer.ts index dd817e8fb0899..df2f57537ca1b 100644 --- a/packages/core/src/common/retryer.ts +++ b/packages/core/src/common/retryer.ts @@ -41,10 +41,13 @@ const TRANSIENT_RETRY_DELAYS_MS = [1000, 2000, 4000]; const TRANSIENT_RETRY_JITTER_MS = 250; /** - * HTTP statuses worth a same-token retry. - * 401/404/422 are permanent failures, so they stay outside this set. + * HTTP statuses worth a same-token retry. Server-side blips only. + * 429 and rate-limit 403 answers go through the rate-limit path instead: + * GitHub expects clients to honor Retry-After or the reset time there, and + * quick retries against a limited token can get the token blocked. + * Permanent statuses such as 401/404/422 stay outside this set. */ -const RETRYABLE_HTTP_STATUS_CODES = new Set([429, 502, 503, 504]); +const RETRYABLE_HTTP_STATUS_CODES = new Set([502, 503, 504]); /** * Wait for `ms` milliseconds. @@ -151,6 +154,25 @@ const retryer = async ( message?: unknown; }; + // Rate-limit responses never get quick retries. GitHub expects + // clients to honor Retry-After or the reset time, so the only safe + // move is to rotate to the next PAT. 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. @@ -202,7 +224,7 @@ const retryer = async ( 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: all GitHub tokens were rejected"; + reason = "GitHub API request failed due to invalid credentials"; } throw new CustomError(reason, CustomError.MAX_RETRY); diff --git a/packages/core/tests/retryer.test.ts b/packages/core/tests/retryer.test.ts index dcd99efa3c0aa..90e37de08cdfc 100644 --- a/packages/core/tests/retryer.test.ts +++ b/packages/core/tests/retryer.test.ts @@ -227,8 +227,40 @@ describe("Test Retryer", () => { await expect( retryer(fetcherMixed, {}, undefined, { transientRetryDelaysMs: [] }), - ).rejects.toThrow("all GitHub tokens were rejected"); + ).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); + }); }); From 3520e0fea6130ba292d5df0caed9b99efce39f1a Mon Sep 17 00:00:00 2001 From: luojiyin Date: Mon, 24 Aug 2026 11:01:07 +0800 Subject: [PATCH 7/9] fix(error): remove unused exported ErrorDetails type --- packages/core/src/common/error.ts | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/packages/core/src/common/error.ts b/packages/core/src/common/error.ts index d1095e5c532ae..c62c14a95a96a 100644 --- a/packages/core/src/common/error.ts +++ b/packages/core/src/common/error.ts @@ -87,15 +87,6 @@ const retrieveSecondaryMessage = (err: Error): string | undefined => { : undefined; }; -/** - * Structured details of a caught error for API results. - */ -export interface ErrorDetails { - /** Error type such as `MAX_RETRY`. Absent when the error has no type. */ - type?: string; - message: string; -} - /** * Extract structured details from a caught error. * @@ -106,7 +97,7 @@ export interface ErrorDetails { * @param err The caught error. * @returns The error type and message. */ -const describeError = (err: Error): ErrorDetails => { +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 }; }; From 7cbe9dd4fc47d16ed115552df69e2f9b334a16f2 Mon Sep 17 00:00:00 2001 From: luojiyin Date: Mon, 24 Aug 2026 11:11:25 +0800 Subject: [PATCH 8/9] test(error): pass strict typecheck in contract tests --- packages/core/tests/describeError.test.ts | 25 ++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/packages/core/tests/describeError.test.ts b/packages/core/tests/describeError.test.ts index 60ab8531c16ba..d7ef3f67b6f25 100644 --- a/packages/core/tests/describeError.test.ts +++ b/packages/core/tests/describeError.test.ts @@ -14,6 +14,22 @@ vi.mock(import("../src/common/log.js"), async () => { 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({ @@ -59,7 +75,7 @@ describe("Test API result error contract", () => { errors: [{ type: "RATE_LIMITED" }], }); - const result = await api({ username: "octocat" }); + const result = await callApi({ username: "octocat" }); // status keeps its exact value for comparisons in apps/backend/router.js expect(result.status).toBe("error - temporary"); @@ -73,9 +89,12 @@ describe("Test API result error contract", () => { }); it("stats handler should attach message-only details for missing params", async () => { - const result = await api({}); + const result = await callApi({}); const { error } = result as { - error?: Partial>; + error?: { + type?: string; + message?: string; + }; }; expect(result.status).toBe("error - temporary"); From ddc4a77122c6052e498aeed8aac9068c9ee6ddb2 Mon Sep 17 00:00:00 2001 From: luojiyin Date: Mon, 24 Aug 2026 11:11:39 +0800 Subject: [PATCH 9/9] docs(retryer): clarify rate-limit path does not wait on Retry-After --- packages/core/src/common/retryer.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/core/src/common/retryer.ts b/packages/core/src/common/retryer.ts index df2f57537ca1b..912efb2b94e0c 100644 --- a/packages/core/src/common/retryer.ts +++ b/packages/core/src/common/retryer.ts @@ -42,10 +42,10 @@ const TRANSIENT_RETRY_JITTER_MS = 250; /** * HTTP statuses worth a same-token retry. Server-side blips only. - * 429 and rate-limit 403 answers go through the rate-limit path instead: - * GitHub expects clients to honor Retry-After or the reset time there, and - * quick retries against a limited token can get the token blocked. - * Permanent statuses such as 401/404/422 stay outside this set. + * 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]); @@ -154,10 +154,10 @@ const retryer = async ( message?: unknown; }; - // Rate-limit responses never get quick retries. GitHub expects - // clients to honor Retry-After or the reset time, so the only safe - // move is to rotate to the next PAT. HTTP 429 and rate-limit 403 - // answers both carry this meaning. + // 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 ?? "",