From 1d0afe2d2ef043324d5891d9c4f7c75ec9b93522 Mon Sep 17 00:00:00 2001 From: ARUNAVO RAY Date: Thu, 24 Sep 2026 00:11:30 +0530 Subject: [PATCH] fix: hold every GitHub request while the source is rate limited (#437) (#452) The v3.37.0 change stopped the scheduler run from holding its lock through a rate limit, but only the refused request stopped. Every other call queued in the run still went to GitHub and came back 403, and the retry helper sent each of them three more times. GitHub counts requests made while limited toward abuse detection, and a reporter's account was suspended that way. Every GitHub client now carries a request wrap that reads the gate before the network: a reset within two minutes is waited out inside the request, a longer one fails at once with a RateLimitPausedError, and only the rate limit probe endpoint is exempt. The retry helper does not retry a rate limit refusal and stops starting the remaining items, surfacing a RateLimitedBatchError. The metadata component catches in both mirror paths rethrow rate limit errors so the repository fails and the scheduler restores it, and the pull request detail fallback no longer writes a stripped down issue after a refusal. The secondary rate limit handler closes the gate for its retry-after window. --- CHANGELOG.md | 8 + src/lib/gitea-enhanced.ts | 16 ++ src/lib/gitea.ts | 44 ++++++ src/lib/github-rate-limit-gate.test.ts | 141 ++++++++++++++++++ src/lib/github.ts | 26 +++- src/lib/rate-limit-gate-wrap.test.ts | 125 ++++++++++++++++ src/lib/rate-limit-gate.ts | 81 ++++++++++ src/lib/utils/concurrency-rate-limit.test.ts | 91 +++++++++++ src/lib/utils/concurrency.ts | 56 ++++++- .../use-cases/github-api-rate-limits.mdx | 2 + 10 files changed, 586 insertions(+), 4 deletions(-) create mode 100644 src/lib/github-rate-limit-gate.test.ts create mode 100644 src/lib/rate-limit-gate-wrap.test.ts create mode 100644 src/lib/utils/concurrency-rate-limit.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 6cd830a..08910f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- No request leaves the process while GitHub is rate limited (#437, second report) + - The v3.37.0 change stopped the scheduler run from holding its lock through a rate limit, but every other request already queued in the run still went to GitHub and came back 403, and the retry helper sent each of them three more times; GitHub counts requests made while limited toward abuse detection and one account was suspended that way + - Every GitHub client now holds its requests while the source is paused: a limit that resets within two minutes is waited out inside the request, a longer one fails the request at once without contacting GitHub, and the rate limit probe endpoint stays available so the pause can end early + - The retry helper no longer retries a rate limit refusal and stops starting the remaining items of the batch + - A rate limit refusal inside the issues, pull requests, releases, labels or milestones pass now fails the repository (which the scheduler already puts back to its previous status and retries after the reset) instead of moving on to the next component, and a pull request whose detail call was refused is no longer written as a stripped down issue + - A secondary rate limit pauses the whole client for its retry-after window, not only the request that was told to slow down + ### Security - Source URLs go through the outbound guard too (GHSA-p7w3-46pg-mv6h): `POST /api/sources`, `PUT /api/sources/:id` and the configuration save refuse link local and metadata addresses for source and destination URLs, the source fetch helper pins and never follows redirects like the other user supplied URLs, source errors no longer carry the upstream body in their message, and errors raised by a request to another host are never forwarded to API clients whatever words they contain - Fixed four privately reported vulnerabilities (GHSA-9m33-xfrc-5jxw, GHSA-6m23-28hh-gjh2, GHSA-2hpx-83vg-gm45, GHSA-5pp8-r7f5-6q8p) diff --git a/src/lib/gitea-enhanced.ts b/src/lib/gitea-enhanced.ts index f13957b..bd83dcb 100644 --- a/src/lib/gitea-enhanced.ts +++ b/src/lib/gitea-enhanced.ts @@ -7,6 +7,7 @@ */ import type { Config } from "@/types/config"; +import { isRateLimitError } from "@/lib/rate-limit-gate"; import { getRepositorySource, SOURCE_PROVIDER_LABELS, @@ -1072,6 +1073,9 @@ export async function syncGiteaRepoEnhanced({ ); } } catch (releaseError) { + // Rate limited: fail the sync so the scheduler restores the row and + // retries after the reset instead of running the next component (#437). + if (isRateLimitError(releaseError)) throw releaseError; console.error( `[Sync] Failed to mirror releases for ${repository.name}: ${ releaseError instanceof Error @@ -1106,6 +1110,9 @@ export async function syncGiteaRepoEnhanced({ `[Sync] Mirrored issues for ${repository.name} after sync` ); } catch (issueError) { + // Rate limited: fail the sync so the scheduler restores the row and + // retries after the reset instead of running the next component (#437). + if (isRateLimitError(issueError)) throw issueError; console.error( `[Sync] Failed to mirror issues for ${repository.name}: ${ issueError instanceof Error @@ -1140,6 +1147,9 @@ export async function syncGiteaRepoEnhanced({ `[Sync] Mirrored pull requests for ${repository.name} after sync` ); } catch (prError) { + // Rate limited: fail the sync so the scheduler restores the row and + // retries after the reset instead of running the next component (#437). + if (isRateLimitError(prError)) throw prError; console.error( `[Sync] Failed to mirror pull requests for ${repository.name}: ${ prError instanceof Error ? prError.message : String(prError) @@ -1170,6 +1180,9 @@ export async function syncGiteaRepoEnhanced({ `[Sync] Mirrored labels for ${repository.name} after sync` ); } catch (labelError) { + // Rate limited: fail the sync so the scheduler restores the row and + // retries after the reset instead of running the next component (#437). + if (isRateLimitError(labelError)) throw labelError; console.error( `[Sync] Failed to mirror labels for ${repository.name}: ${ labelError instanceof Error @@ -1202,6 +1215,9 @@ export async function syncGiteaRepoEnhanced({ `[Sync] Mirrored milestones for ${repository.name} after sync` ); } catch (milestoneError) { + // Rate limited: fail the sync so the scheduler restores the row and + // retries after the reset instead of running the next component (#437). + if (isRateLimitError(milestoneError)) throw milestoneError; console.error( `[Sync] Failed to mirror milestones for ${repository.name}: ${ milestoneError instanceof Error diff --git a/src/lib/gitea.ts b/src/lib/gitea.ts index 476c706..4d8b673 100644 --- a/src/lib/gitea.ts +++ b/src/lib/gitea.ts @@ -29,6 +29,7 @@ import { findSourceForRepository, listSources, } from "./sources"; +import { isRateLimitError } from "./rate-limit-gate"; import { advanceMetadataSyncCursor, parseRepositoryMetadataState, @@ -1160,6 +1161,10 @@ export const mirrorGithubRepoToGitea = async ({ ); } } catch (error) { + // A rate limit is not this repository's fault and the other + // components would run into the same wall: fail the mirror so the + // scheduler puts the row back and retries after the reset (#437). + if (isRateLimitError(error)) throw error; console.error( `[Metadata] Failed to mirror releases for ${repository.name}: ${ error instanceof Error ? error.message : String(error) @@ -1197,6 +1202,10 @@ export const mirrorGithubRepoToGitea = async ({ `[Metadata] Successfully mirrored issues for ${repository.name}` ); } catch (error) { + // A rate limit is not this repository's fault and the other + // components would run into the same wall: fail the mirror so the + // scheduler puts the row back and retries after the reset (#437). + if (isRateLimitError(error)) throw error; console.error( `[Metadata] Failed to mirror issues for ${repository.name}: ${ error instanceof Error ? error.message : String(error) @@ -1229,6 +1238,10 @@ export const mirrorGithubRepoToGitea = async ({ `[Metadata] Successfully mirrored pull requests for ${repository.name}` ); } catch (error) { + // A rate limit is not this repository's fault and the other + // components would run into the same wall: fail the mirror so the + // scheduler puts the row back and retries after the reset (#437). + if (isRateLimitError(error)) throw error; console.error( `[Metadata] Failed to mirror pull requests for ${repository.name}: ${ error instanceof Error ? error.message : String(error) @@ -1261,6 +1274,10 @@ export const mirrorGithubRepoToGitea = async ({ `[Metadata] Successfully mirrored labels for ${repository.name}` ); } catch (error) { + // A rate limit is not this repository's fault and the other + // components would run into the same wall: fail the mirror so the + // scheduler puts the row back and retries after the reset (#437). + if (isRateLimitError(error)) throw error; console.error( `[Metadata] Failed to mirror labels for ${repository.name}: ${ error instanceof Error ? error.message : String(error) @@ -1291,6 +1308,10 @@ export const mirrorGithubRepoToGitea = async ({ `[Metadata] Successfully mirrored milestones for ${repository.name}` ); } catch (error) { + // A rate limit is not this repository's fault and the other + // components would run into the same wall: fail the mirror so the + // scheduler puts the row back and retries after the reset (#437). + if (isRateLimitError(error)) throw error; console.error( `[Metadata] Failed to mirror milestones for ${repository.name}: ${ error instanceof Error ? error.message : String(error) @@ -1938,6 +1959,10 @@ export async function mirrorGitHubRepoToGiteaOrg({ ); } } catch (error) { + // A rate limit is not this repository's fault and the other + // components would run into the same wall: fail the mirror so the + // scheduler puts the row back and retries after the reset (#437). + if (isRateLimitError(error)) throw error; console.error( `[Metadata] Failed to mirror releases for ${repository.name}: ${ error instanceof Error ? error.message : String(error) @@ -1973,6 +1998,10 @@ export async function mirrorGitHubRepoToGiteaOrg({ `[Metadata] Successfully mirrored issues for ${repository.name} to org ${orgName}/${targetRepoName}` ); } catch (error) { + // A rate limit is not this repository's fault and the other + // components would run into the same wall: fail the mirror so the + // scheduler puts the row back and retries after the reset (#437). + if (isRateLimitError(error)) throw error; console.error( `[Metadata] Failed to mirror issues for ${repository.name} to org ${orgName}/${targetRepoName}: ${ error instanceof Error ? error.message : String(error) @@ -2005,6 +2034,10 @@ export async function mirrorGitHubRepoToGiteaOrg({ `[Metadata] Successfully mirrored pull requests for ${repository.name} to org ${orgName}/${targetRepoName}` ); } catch (error) { + // A rate limit is not this repository's fault and the other + // components would run into the same wall: fail the mirror so the + // scheduler puts the row back and retries after the reset (#437). + if (isRateLimitError(error)) throw error; console.error( `[Metadata] Failed to mirror pull requests for ${repository.name} to org ${orgName}/${targetRepoName}: ${ error instanceof Error ? error.message : String(error) @@ -2037,6 +2070,10 @@ export async function mirrorGitHubRepoToGiteaOrg({ `[Metadata] Successfully mirrored labels for ${repository.name} to org ${orgName}/${targetRepoName}` ); } catch (error) { + // A rate limit is not this repository's fault and the other + // components would run into the same wall: fail the mirror so the + // scheduler puts the row back and retries after the reset (#437). + if (isRateLimitError(error)) throw error; console.error( `[Metadata] Failed to mirror labels for ${repository.name} to org ${orgName}/${targetRepoName}: ${ error instanceof Error ? error.message : String(error) @@ -2067,6 +2104,10 @@ export async function mirrorGitHubRepoToGiteaOrg({ `[Metadata] Successfully mirrored milestones for ${repository.name} to org ${orgName}/${targetRepoName}` ); } catch (error) { + // A rate limit is not this repository's fault and the other + // components would run into the same wall: fail the mirror so the + // scheduler puts the row back and retries after the reset (#437). + if (isRateLimitError(error)) throw error; console.error( `[Metadata] Failed to mirror milestones for ${repository.name} to org ${orgName}/${targetRepoName}: ${ error instanceof Error ? error.message : String(error) @@ -4487,6 +4528,9 @@ export async function mirrorGitRepoPullRequestsToGitea({ successCount++; console.log(`[Pull Requests] ✅ Successfully created issue for PR #${pr.number}`); } catch (apiError) { + // A rate limit refusal is not a reason to write a stripped down + // issue; let it end the pass so the next run redoes this PR (#437). + if (isRateLimitError(apiError)) throw apiError; // If the detailed fetch fails, fall back to basic PR info console.log(`[Pull Requests] Falling back to basic info for PR #${pr.number} due to error: ${apiError}`); const basicIssueData = { diff --git a/src/lib/github-rate-limit-gate.test.ts b/src/lib/github-rate-limit-gate.test.ts new file mode 100644 index 0000000..3afcd7b --- /dev/null +++ b/src/lib/github-rate-limit-gate.test.ts @@ -0,0 +1,141 @@ +/** + * The GitHub clients built by github.ts hold their requests while the gate + * is closed, and the secondary limit handler closes the gate for everyone + * (issue #437, second report). Uses a fake global fetch so nothing leaves + * the process; the point of these tests is that nothing does. + */ + +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { + createGitHubClient, + createPublicGitHubClient, + githubThrottleOptions, +} from "@/lib/github"; +import { + RateLimitPausedError, + clearAllRateLimits, + isRateLimitError, + markRateLimited, + rateLimitGateKey, + rateLimitedUntil, +} from "@/lib/rate-limit-gate"; + +const realFetch = globalThis.fetch; + +describe("GitHub clients while the gate is closed", () => { + let sent: string[] = []; + + beforeEach(() => { + clearAllRateLimits(); + sent = []; + globalThis.fetch = (async (input: any) => { + const url = typeof input === "string" ? input : input.url; + sent.push(url); + return new Response(JSON.stringify({ resources: {}, rate: { remaining: 1 } }), { + status: 200, + headers: { + "content-type": "application/json", + "x-ratelimit-remaining": "4999", + "x-ratelimit-reset": String(Math.floor(Date.now() / 1000) + 3600), + }, + }); + }) as unknown as typeof fetch; + }); + + afterEach(() => { + globalThis.fetch = realFetch; + clearAllRateLimits(); + }); + + test("an authenticated client sends nothing during a long pause and fails with the paused error", async () => { + const octokit = createGitHubClient("ghp_test", "user-1"); + markRateLimited(rateLimitGateKey("user-1"), Date.now() + 3_263_000); + + let caught: unknown; + try { + await octokit.rest.repos.get({ owner: "a", repo: "b" }); + } catch (error) { + caught = error; + } + expect(caught).toBeInstanceOf(RateLimitPausedError); + expect(isRateLimitError(caught)).toBe(true); + expect(sent).toEqual([]); + }); + + test("paginate stops at the gate too, before the first page", async () => { + const octokit = createGitHubClient("ghp_test", "user-2"); + markRateLimited(rateLimitGateKey("user-2"), Date.now() + 3_600_000); + + await expect( + octokit.paginate(octokit.rest.issues.listForRepo, { owner: "a", repo: "b", per_page: 100 }) + ).rejects.toBeInstanceOf(RateLimitPausedError); + expect(sent).toEqual([]); + }); + + test("the rate limit probe still goes out, so the pause can be confirmed or lifted", async () => { + const octokit = createGitHubClient("ghp_test", "user-3"); + markRateLimited(rateLimitGateKey("user-3"), Date.now() + 3_600_000); + + await octokit.rest.rateLimit.get(); + expect(sent).toHaveLength(1); + expect(sent[0]).toContain("/rate_limit"); + // The probe answered with budget left, which opens the gate again. + expect(rateLimitedUntil(rateLimitGateKey("user-3"))).toBeNull(); + await octokit.rest.repos.get({ owner: "a", repo: "b" }); + expect(sent).toHaveLength(2); + }); + + test("another user's pause does not hold this user's client", async () => { + const octokit = createGitHubClient("ghp_test", "user-4"); + markRateLimited(rateLimitGateKey("user-5"), Date.now() + 3_600_000); + + await octokit.rest.repos.get({ owner: "a", repo: "b" }); + expect(sent).toHaveLength(1); + }); + + test("tokenless clients share the anonymous gate", async () => { + const octokit = createPublicGitHubClient(); + markRateLimited(rateLimitGateKey(undefined), Date.now() + 3_600_000); + + await expect(octokit.rest.repos.get({ owner: "a", repo: "b" })).rejects.toBeInstanceOf( + RateLimitPausedError + ); + expect(sent).toEqual([]); + }); +}); + +describe("secondary rate limit handler", () => { + beforeEach(() => { + clearAllRateLimits(); + }); + + test("closes the gate for the retry-after window and still retries the request itself", async () => { + const before = Date.now(); + const options = githubThrottleOptions("user-secondary"); + const retry = await options.onSecondaryRateLimit( + 60, + { method: "GET", url: "/repos/a/b/issues" }, + {}, + 0 + ); + expect(retry).toBe(true); + + const until = rateLimitedUntil(rateLimitGateKey("user-secondary")); + expect(until).not.toBeNull(); + const pausedFor = until!.getTime() - before; + expect(pausedFor).toBeGreaterThanOrEqual(59_000); + expect(pausedFor).toBeLessThanOrEqual(61_000); + }); + + test("gives up after two retries but the gate stays closed for the window", async () => { + const options = githubThrottleOptions("user-secondary-2"); + expect(await options.onSecondaryRateLimit(30, { method: "GET", url: "/x" }, {}, 2)).toBe(false); + expect(rateLimitedUntil(rateLimitGateKey("user-secondary-2"))).not.toBeNull(); + }); + + test("a tokenless client records the pause under the anonymous key", async () => { + const options = githubThrottleOptions(); + await options.onSecondaryRateLimit(10, { method: "GET", url: "/x" }, {}, 0); + expect(rateLimitedUntil(rateLimitGateKey(undefined))).not.toBeNull(); + }); +}); diff --git a/src/lib/github.ts b/src/lib/github.ts index 2f49155..276ab61 100644 --- a/src/lib/github.ts +++ b/src/lib/github.ts @@ -10,6 +10,7 @@ import { } from "@/lib/github-conditional-requests"; import { clearRateLimit, + installRateLimitGate, markRateLimited, rateLimitGateKey, } from "@/lib/rate-limit-gate"; @@ -80,7 +81,7 @@ export function decideRateLimitRetry({ * handlers only log and schedule the retry — there is no per-user * rate-limit state or UI event to update for a tokenless source. */ -function githubThrottleOptions(userId?: string) { +export function githubThrottleOptions(userId?: string) { return { onRateLimit: async ( retryAfter: number, @@ -161,6 +162,11 @@ function githubThrottleOptions(userId?: string) { `[GitHub] Secondary rate limit hit for ${options.method} ${options.url}`, ); + // A secondary limit is GitHub asking the whole client to slow down, + // not only this request. Close the gate for the retry-after window so + // the other queued calls wait instead of piling on (issue #437). + markRateLimited(rateLimitGateKey(userId), Date.now() + retryAfter * 1000); + // Update status and notify UI (if available) if (userId && publishEvent) { await publishEvent({ @@ -306,6 +312,13 @@ export function createGitHubClient( scope: userId ?? username ?? conditionalRequestTokenScope(token), }); + // Registered last so it runs first: a closed gate is answered before the + // conditional request cache and before anything reaches the network. + installRateLimitGate(octokit, { + key: rateLimitGateKey(userId), + maxWaitMs: RATE_LIMIT_FAST_FAIL_THRESHOLD_SECONDS * 1000, + }); + return octokit; } @@ -328,7 +341,7 @@ export function createPublicGitHubClient(apiBaseUrl?: string): Octokit { // with GitHub Actions, which sets it to https://api.github.com by default). const baseUrl = apiBaseUrl || process.env.GH_API_URL || process.env.GITHUB_API_URL || "https://api.github.com"; - return new MyOctokit({ + const octokit = new MyOctokit({ userAgent: "gitea-mirror/3.5.4", baseUrl, // Configurable for E2E testing log: { @@ -345,6 +358,15 @@ export function createPublicGitHubClient(apiBaseUrl?: string): Octokit { }, throttle: githubThrottleOptions(), }); + + // Anonymous clients share one gate key, so one exhausted 60 req/hr budget + // pauses every tokenless source together. + installRateLimitGate(octokit, { + key: rateLimitGateKey(undefined), + maxWaitMs: RATE_LIMIT_FAST_FAIL_THRESHOLD_SECONDS * 1000, + }); + + return octokit; } /** diff --git a/src/lib/rate-limit-gate-wrap.test.ts b/src/lib/rate-limit-gate-wrap.test.ts new file mode 100644 index 0000000..0063a2e --- /dev/null +++ b/src/lib/rate-limit-gate-wrap.test.ts @@ -0,0 +1,125 @@ +/** + * The request wrap that holds every GitHub call while the gate is closed + * (issue #437, second report). Tested against a hand made hook so the + * clock and the sleep are under control. + */ + +import { beforeEach, describe, expect, test } from "bun:test"; +import { + RateLimitPausedError, + clearAllRateLimits, + installRateLimitGate, + isRateLimitError, + markRateLimited, +} from "@/lib/rate-limit-gate"; + +type Wrapper = (request: (o: any) => Promise, options: any) => Promise; + +function fakeOctokit() { + const holder: { wrapper?: Wrapper } = {}; + return { + holder, + octokit: { + hook: { + wrap: (_name: string, wrapper: Wrapper) => { + holder.wrapper = wrapper; + }, + }, + }, + }; +} + +describe("installRateLimitGate", () => { + const key = "user-gate"; + const base = new Date("2026-09-24T10:00:00.000Z"); + + beforeEach(() => { + clearAllRateLimits(); + }); + + test("an open gate passes the request straight through", async () => { + const { octokit, holder } = fakeOctokit(); + const sleeps: number[] = []; + installRateLimitGate(octokit, { key, maxWaitMs: 120_000, sleep: async (ms) => void sleeps.push(ms), now: () => base }); + + const sent: any[] = []; + const result = await holder.wrapper!(async (o) => { sent.push(o); return "ok"; }, { method: "GET", url: "/repos/a/b" }); + expect(result).toBe("ok"); + expect(sent).toHaveLength(1); + expect(sleeps).toEqual([]); + }); + + test("a long pause fails the request without calling the inner request", async () => { + const { octokit, holder } = fakeOctokit(); + installRateLimitGate(octokit, { key, maxWaitMs: 120_000, sleep: async () => {}, now: () => base }); + markRateLimited(key, base.getTime() + 3_263_000); + + let inner = 0; + let caught: unknown; + try { + await holder.wrapper!(async () => { inner += 1; return "no"; }, { method: "GET", url: "/repos/a/b/issues" }); + } catch (error) { + caught = error; + } + expect(inner).toBe(0); + expect(caught).toBeInstanceOf(RateLimitPausedError); + expect((caught as RateLimitPausedError).status).toBe(403); + expect((caught as RateLimitPausedError).message).toContain("paused until 2026-09-24T10:54:23.000Z"); + expect((caught as RateLimitPausedError).message).toContain("GET /repos/a/b/issues"); + // The rest of the code base classifies it like GitHub's own refusal. + expect(isRateLimitError(caught)).toBe(true); + }); + + test("a short pause is slept off with a one second cushion, then the request goes out", async () => { + const { octokit, holder } = fakeOctokit(); + const sleeps: number[] = []; + installRateLimitGate(octokit, { key, maxWaitMs: 120_000, sleep: async (ms) => void sleeps.push(ms), now: () => base }); + markRateLimited(key, base.getTime() + 45_000); + + let inner = 0; + const result = await holder.wrapper!(async () => { inner += 1; return "sent"; }, { method: "GET", url: "/repos/a/b" }); + expect(result).toBe("sent"); + expect(inner).toBe(1); + expect(sleeps).toEqual([46_000]); + }); + + test("exactly the threshold still waits; one millisecond more fails", async () => { + const { octokit, holder } = fakeOctokit(); + const sleeps: number[] = []; + installRateLimitGate(octokit, { key, maxWaitMs: 120_000, sleep: async (ms) => void sleeps.push(ms), now: () => base }); + + markRateLimited(key, base.getTime() + 120_000); + await holder.wrapper!(async () => "a", { method: "GET", url: "/x" }); + expect(sleeps).toEqual([121_000]); + + clearAllRateLimits(); + markRateLimited(key, base.getTime() + 120_001); + await expect(holder.wrapper!(async () => "b", { method: "GET", url: "/x" })).rejects.toBeInstanceOf(RateLimitPausedError); + }); + + test("the rate limit probe is exempt so the pause can be checked", async () => { + const { octokit, holder } = fakeOctokit(); + installRateLimitGate(octokit, { key, maxWaitMs: 120_000, sleep: async () => {}, now: () => base }); + markRateLimited(key, base.getTime() + 3_600_000); + + let inner = 0; + await holder.wrapper!(async () => { inner += 1; return "probe"; }, { method: "GET", url: "/rate_limit" }); + expect(inner).toBe(1); + await expect(holder.wrapper!(async () => "no", { method: "GET", url: "/rate_limited_looking/but/not" })).rejects.toBeInstanceOf(RateLimitPausedError); + }); + + test("keys are independent: another user's pause does not hold this client", async () => { + const { octokit, holder } = fakeOctokit(); + installRateLimitGate(octokit, { key, maxWaitMs: 120_000, sleep: async () => {}, now: () => base }); + markRateLimited("someone-else", base.getTime() + 3_600_000); + + let inner = 0; + await holder.wrapper!(async () => { inner += 1; return "ok"; }, { method: "GET", url: "/repos/a/b" }); + expect(inner).toBe(1); + }); + + test("a client without a hook system is left alone", () => { + expect(() => installRateLimitGate({}, { key, maxWaitMs: 1 })).not.toThrow(); + expect(() => installRateLimitGate(undefined, { key, maxWaitMs: 1 })).not.toThrow(); + }); +}); diff --git a/src/lib/rate-limit-gate.ts b/src/lib/rate-limit-gate.ts index 07b7d95..a2b030f 100644 --- a/src/lib/rate-limit-gate.ts +++ b/src/lib/rate-limit-gate.ts @@ -112,3 +112,84 @@ export function isRateLimitError(error: unknown): boolean { // all that is left to go on. return status === undefined || status === 403; } + +/** + * The error a gated request fails with before anything is sent. Shaped like + * Octokit's own primary rate limit refusal (status 403, "rate limit" in the + * message) so every existing `isRateLimitError` check treats it the same. + */ +export class RateLimitPausedError extends Error { + readonly status = 403; + readonly resetAt: Date; + + constructor(resetAt: Date, endpoint?: string) { + super( + `GitHub API rate limit exceeded; requests are paused until ${resetAt.toISOString()}` + + (endpoint ? ` (not sent: ${endpoint})` : "") + ); + this.name = "RateLimitPausedError"; + this.resetAt = resetAt; + } +} + +/** + * Endpoints that never count against the limit and are needed to learn when + * it opens again. Everything else is held while the gate is closed. + */ +const GATE_EXEMPT_ROUTES = [/\/rate_limit(\?|$)/]; + +export interface RateLimitGateOptions { + /** Gate key of this client, from rateLimitGateKey(). */ + key: string; + /** + * A closed gate that opens again within this many milliseconds is waited + * out inside the request instead of failing it, so a short primary or + * secondary limit stays invisible to the caller. Longer waits fail at once. + */ + maxWaitMs: number; + /** Injectable for tests. */ + sleep?: (ms: number) => Promise; + now?: () => Date; +} + +/** + * Hold every request of an Octokit client while its gate is closed + * (issue #437, second report). + * + * The throttle handler records a reset time when a request is refused, but + * on its own that only affects the one request: every other call queued in + * the same run still went to GitHub and came back 403, and the retry helper + * sent each of them three more times. GitHub counts requests made while + * limited toward abuse detection, and one reporter's account was suspended + * that way. This wrap runs before the network: a closed gate either waits + * (short reset) or throws (long reset), and nothing leaves the process. + */ +export function installRateLimitGate(octokit: unknown, options: RateLimitGateOptions): void { + const hook = (octokit as { hook?: { wrap?: unknown } })?.hook; + if (typeof hook?.wrap !== "function") return; + + const sleep = options.sleep ?? ((ms: number) => new Promise((resolve) => setTimeout(resolve, ms))); + const now = options.now ?? (() => new Date()); + + (hook.wrap as (name: string, wrapper: (request: any, requestOptions: any) => Promise) => void)( + "request", + async (request, requestOptions) => { + const url = String(requestOptions?.url ?? ""); + if (GATE_EXEMPT_ROUTES.some((route) => route.test(url))) { + return request(requestOptions); + } + + const until = rateLimitedUntil(options.key, now()); + if (until) { + const waitMs = until.getTime() - now().getTime(); + if (waitMs > options.maxWaitMs) { + throw new RateLimitPausedError(until, `${requestOptions?.method ?? "GET"} ${url}`); + } + // Small cushion: GitHub's reset is second precision. + await sleep(waitMs + 1000); + } + + return request(requestOptions); + } + ); +} diff --git a/src/lib/utils/concurrency-rate-limit.test.ts b/src/lib/utils/concurrency-rate-limit.test.ts new file mode 100644 index 0000000..16ad05f --- /dev/null +++ b/src/lib/utils/concurrency-rate-limit.test.ts @@ -0,0 +1,91 @@ +/** + * processWithRetry and a rate limited source (issue #437, second report). + * A refusal used to be retried three times with backoff like any other + * error, and the items behind it were started anyway, so one exhausted + * budget turned into thousands of 403 responses. + */ + +import { describe, expect, test } from "bun:test"; +import { RateLimitedBatchError, processWithRetry } from "./concurrency"; +import { isRateLimitError } from "@/lib/rate-limit-gate"; + +function rateLimitRefusal() { + const error = new Error("API rate limit exceeded for user ID 1.") as Error & { status: number }; + error.status = 403; + return error; +} + +describe("processWithRetry under a rate limit", () => { + test("a rate limit refusal is not retried and the remaining items are not started", async () => { + const attempts: number[] = []; + let caught: unknown; + try { + await processWithRetry( + [1, 2, 3, 4, 5, 6], + async (n) => { + attempts.push(n); + if (n === 2) throw rateLimitRefusal(); + return n * 10; + }, + { concurrencyLimit: 2, maxRetries: 3, retryDelay: 1 } + ); + } catch (error) { + caught = error; + } + + expect(caught).toBeInstanceOf(RateLimitedBatchError); + const failure = caught as RateLimitedBatchError; + // Item 2 was attempted exactly once; items 3 to 6 never started. The two + // items of the first batch run concurrently, so their order is not fixed. + expect([...attempts].sort()).toEqual([1, 2]); + expect(failure.results).toEqual([10]); + expect(failure.message).toContain("5 item(s) left"); + expect(failure.status).toBe(403); + // Callers up the stack classify it like the refusal that caused it. + expect(isRateLimitError(failure)).toBe(true); + expect(isRateLimitError(failure.cause)).toBe(true); + }); + + test("a 429 from a paginated call counts as a rate limit too", async () => { + const attempts: number[] = []; + const tooMany = Object.assign(new Error("Too many requests"), { status: 429 }); + await expect( + processWithRetry( + [1, 2, 3], + async (n) => { + attempts.push(n); + if (n === 1) throw tooMany; + return n; + }, + { concurrencyLimit: 1, maxRetries: 3, retryDelay: 1 } + ) + ).rejects.toBeInstanceOf(RateLimitedBatchError); + expect(attempts).toEqual([1]); + }); + + test("other errors keep their retries and do not stop the batch", async () => { + const attempts: number[] = []; + const results = await processWithRetry( + [1, 2, 3], + async (n) => { + attempts.push(n); + if (n === 2 && attempts.filter((a) => a === 2).length < 3) { + throw new Error("Gitea hiccup"); + } + return n; + }, + { concurrencyLimit: 1, maxRetries: 3, retryDelay: 1 } + ); + expect(results).toEqual([1, 2, 3]); + expect(attempts).toEqual([1, 2, 2, 2, 3]); + }); + + test("a batch that finishes without a refusal returns every result as before", async () => { + const results = await processWithRetry([1, 2, 3], async (n) => n + 1, { + concurrencyLimit: 3, + maxRetries: 0, + retryDelay: 1, + }); + expect(results).toEqual([2, 3, 4]); + }); +}); diff --git a/src/lib/utils/concurrency.ts b/src/lib/utils/concurrency.ts index 48a140c..b8044ed 100644 --- a/src/lib/utils/concurrency.ts +++ b/src/lib/utils/concurrency.ts @@ -1,4 +1,26 @@ import { JOB_HEARTBEAT_INTERVAL_MS } from "@/lib/interrupted-job-detection"; +import { isRateLimitError } from "@/lib/rate-limit-gate"; + +/** + * Thrown by processWithRetry when a batch stopped because the source is rate + * limited. Carries the results gathered so far so a caller can still count + * what was done. Shaped like a GitHub refusal so isRateLimitError matches it. + */ +export class RateLimitedBatchError extends Error { + readonly status = 403; + readonly results: R[]; + readonly cause: unknown; + + constructor(cause: unknown, results: R[], remaining: number) { + super( + `Rate limit hit while processing items; stopped with ${remaining} item(s) left ` + + `(${cause instanceof Error ? cause.message : String(cause)})` + ); + this.name = "RateLimitedBatchError"; + this.cause = cause; + this.results = results; + } +} /** * Utility for processing items in parallel with concurrency control @@ -13,7 +35,12 @@ export async function processInParallel( items: T[], processItem: (item: T) => Promise, concurrencyLimit: number = 5, // Safe default for GitHub API (max 100 concurrent, but 5-10 recommended) - onProgress?: (completed: number, total: number, result?: R) => void + onProgress?: (completed: number, total: number, result?: R) => void, + /** + * Asked before each batch. Returning true ends the loop with the results + * gathered so far; the remaining items are not started. + */ + shouldStop?: () => boolean ): Promise { const results: R[] = []; let completed = 0; @@ -21,6 +48,11 @@ export async function processInParallel( // Process items in batches to control concurrency for (let i = 0; i < total; i += concurrencyLimit) { + if (shouldStop?.()) { + console.warn(`[Batch] Stopping before item ${i} of ${total}: source is rate limited`); + break; + } + const batch = items.slice(i, i + concurrencyLimit); const batchPromises = batch.map(async (item) => { @@ -119,6 +151,11 @@ export async function processWithRetry( // Track checkpoint counter let itemsProcessedSinceLastCheckpoint = 0; + // The first rate limit refusal ends the run: retrying it, or starting the + // items behind it, would only send more requests into the same wall + // (issue #437). The caller gets a RateLimitedBatchError with what finished. + let rateLimitError: unknown = null; + // Wrap the process function with retry logic const processWithRetryLogic = async (item: T): Promise => { let lastError: Error | null = null; @@ -153,6 +190,16 @@ export async function processWithRetry( } catch (error) { lastError = error instanceof Error ? error : new Error(String(error)); + if (isRateLimitError(error)) { + if (!rateLimitError) { + rateLimitError = error; + console.warn( + `[Batch] Rate limit hit on ${getItemId ? getItemId(item) : "an item"}; not retrying and not starting the remaining items` + ); + } + throw lastError; + } + if (attempt <= maxRetries) { if (onRetry) { onRetry(item, lastError, attempt); @@ -190,9 +237,14 @@ export async function processWithRetry( items, processWithRetryLogic, concurrencyLimit, - onProgress + onProgress, + () => rateLimitError !== null ); + if (rateLimitError) { + throw new RateLimitedBatchError(rateLimitError, results, items.length - results.length); + } + return results; } diff --git a/www/src/pages/use-cases/github-api-rate-limits.mdx b/www/src/pages/use-cases/github-api-rate-limits.mdx index a7dfe91..6c3074f 100644 --- a/www/src/pages/use-cases/github-api-rate-limits.mdx +++ b/www/src/pages/use-cases/github-api-rate-limits.mdx @@ -35,6 +35,8 @@ The practical effect is that sync frequency mostly stops mattering. A `30m` inte Issues and pull requests go one step further. After the first complete pass, a scheduled sync asks GitHub only for the issues and pull requests updated since the previous one, and fetches comments, commits and changed files for those alone. A tracker with thousands of issues and a handful of changes costs a handful of requests. A full pass still runs once a week to catch anything the incremental listing cannot see. +When the limit is hit anyway, nothing else is sent. Every GitHub client pauses until the reset GitHub reported: a reset within two minutes is waited out inside the request, a longer one fails the request at once without contacting GitHub, and the run ends so the next scheduled one starts a minute after the reset. Repositories that failed only because of the limit go back to their previous status and are retried then. This matters because GitHub counts requests made while limited toward abuse detection, which can suspend the account. + ## Configuration that stays inside the budget A few settings still matter for large or busy accounts: