From f00aa0e4583240ab7bd818cdf89db2ec2990636d Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Wed, 11 Nov 2026 08:49:06 +0000 Subject: [PATCH] gh-prs-merge: back off secondary rate limits instead of retrying every 65 s When `gh` was refused but no `gh api rate_limit` bucket was empty (a secondary/abuse limit), parseRateLimitReset invented "now + 60 s". Every retry then printed "rate limit exhausted; waiting until it resets ... (in 2m)" and hit GitHub again 65 s later, which keeps a secondary penalty alive: the reset crept forward a minute per line and the sweep never resumed. - parseRateLimitReset returns undefined when no bucket is dry. - Secondary limits back off 1, 2, 4, 8 m (streak shared across calls, reset on the first success) and throw RateLimitError once the next step is past --rate-wait, the same as an out-of-budget primary reset. - Notices say "secondary rate limit" rather than "exhausted", and short waits print as seconds ("65s", not "2m"). Co-Authored-By: Claude Opus 5.5 --- src/gh.ts | 56 ++++++++++++++++++++++++++++++------ test/prs-merge.test.ts | 65 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 111 insertions(+), 10 deletions(-) diff --git a/src/gh.ts b/src/gh.ts index 4757a6d..791139b 100644 --- a/src/gh.ts +++ b/src/gh.ts @@ -58,10 +58,11 @@ export function isRateLimited(stderr: string): boolean { * When the quota that ran out comes back, from `gh api rate_limit`. * * That endpoint is free: it does not count against any quota, so it still - * answers while everything else is refused. A secondary limit leaves every - * bucket with quota remaining and documents no reset, so it gets a minute. + * answers while everything else is refused. `undefined` means no bucket is + * empty (or the endpoint did not answer), which is what a secondary (abuse) + * limit looks like: it documents no reset, so the caller backs off instead. */ -export function parseRateLimitReset(raw: unknown, now: number): Date { +export function parseRateLimitReset(raw: unknown, now: number): Date | undefined { const resources = (raw as { resources?: Record }) ?.resources; let latest = 0; @@ -70,7 +71,15 @@ export function parseRateLimitReset(raw: unknown, now: number): Date { latest = Math.max(latest, bucket.reset * 1000); } } - return new Date(latest > now ? latest : now + 60_000); + return latest > now ? new Date(latest) : undefined; +} + +/** First secondary-limit back-off; each further refusal in a row doubles it. */ +export const SECONDARY_BACKOFF_MS = 60_000; + +/** `45s` under a minute and a half, `3m` past it: "(in 2m)" for 65 s misleads. */ +export function formatWait(ms: number): string { + return ms < 90_000 ? `${Math.max(1, Math.ceil(ms / 1000))}s` : `${Math.ceil(ms / 60_000)}m`; } /** Values GitHub documents for `mergeable`, plus the honest fallback. */ @@ -268,6 +277,8 @@ export class Gh { private readonly sleep: typeof sleep; private readonly rateWaitMs: number; private readonly onRateLimit: (text: string) => void; + /** Secondary-limit refusals in a row, across calls; next back-off is 1m × 2^this. */ + private secondaryStreak = 0; constructor(options: GhOptions = {}) { this.exec = options.exec ?? run; @@ -284,17 +295,46 @@ export class Gh { for (;;) { const result = await this.exec('gh', args, { env }); - if (result.code === 0 || !isRateLimited(result.stderr)) return result; + if (result.code === 0 || !isRateLimited(result.stderr)) { + this.secondaryStreak = 0; + return result; + } const resetAt = await this.rateLimitReset(env); + const budget = `${Math.round(this.rateWaitMs / 60_000)}m --rate-wait budget`; + + if (!resetAt) { + // Secondary limit: every bucket still has quota and GitHub names no + // reset. Retrying on a fixed minute keeps the penalty alive (each + // refused call extends it), so double the pause on every refusal in + // a row, across calls, and give up once it outgrows the budget. + const waitMs = SECONDARY_BACKOFF_MS * 2 ** this.secondaryStreak; + if (waitMs > this.rateWaitMs) { + this.secondaryStreak = 0; + throw new RateLimitError( + `GitHub secondary rate limit still refusing; the next back-off ` + + `(${formatWait(waitMs)}) is past the ${budget} — ${result.stderr.trim()}`, + new Date(Date.now() + waitMs), + result, + ); + } + this.secondaryStreak += 1; + this.onRateLimit( + `GitHub secondary rate limit (no quota bucket is empty); backing off ` + + `${formatWait(waitMs)} (refusal ${this.secondaryStreak} in a row)`, + ); + await this.sleep(waitMs); + continue; + } + // A few seconds past the reset: GitHub's clock and ours are not one clock. const waitMs = resetAt.getTime() - Date.now() + 5_000; - const when = `${resetAt.toLocaleTimeString()} (in ${Math.ceil(waitMs / 60_000)}m)`; + const when = `${resetAt.toLocaleTimeString()} (in ${formatWait(waitMs)})`; if (waitMs > this.rateWaitMs) { throw new RateLimitError( `GitHub API rate limit exhausted; it resets at ${when}, past the ` + - `${Math.round(this.rateWaitMs / 60_000)}m --rate-wait budget — ${result.stderr.trim()}`, + `${budget} — ${result.stderr.trim()}`, resetAt, result, ); @@ -305,7 +345,7 @@ export class Gh { } } - private async rateLimitReset(env: NodeJS.ProcessEnv): Promise { + private async rateLimitReset(env: NodeJS.ProcessEnv): Promise { const result = await this.exec('gh', ['api', 'rate_limit'], { env }); let raw: unknown; try { diff --git a/test/prs-merge.test.ts b/test/prs-merge.test.ts index c95e007..e57fdf9 100644 --- a/test/prs-merge.test.ts +++ b/test/prs-merge.test.ts @@ -7,6 +7,7 @@ import { parseRunsAwaitingApproval, parsePullRequestUrl, parseRateLimitReset, + formatWait, isRateLimited, GhError, } from '../src/gh.ts'; @@ -758,8 +759,68 @@ describe('rate limit', () => { }, now, ); - expect(reset.getTime()).toBe(now + 120_000); - expect(parseRateLimitReset(undefined, now).getTime()).toBe(now + 60_000); + expect(reset?.getTime()).toBe(now + 120_000); + expect(parseRateLimitReset(undefined, now)).toBeUndefined(); + expect( + parseRateLimitReset({ resources: { core: { remaining: 4747, reset: now / 1000 + 60 } } }, now), + ).toBeUndefined(); + }); + + it('formats short waits in seconds, not a rounded-up "2m"', () => { + expect(formatWait(65_000)).toBe('65s'); + expect(formatWait(200)).toBe('1s'); + expect(formatWait(5 * 60_000 + 1)).toBe('6m'); + }); + + /** Every bucket has quota: refused `failures` times with a secondary-limit message. */ + function secondaryGh(failures: number, rateWaitMs: number) { + const waits: number[] = []; + const notices: string[] = []; + const gh = new Gh({ + rateWaitMs, + sleep: async (ms) => { + waits.push(ms); + }, + onRateLimit: (text) => notices.push(text), + exec: async (_f, args) => { + if (args[0] === 'api' && args[1] === 'rate_limit') { + const reset = Math.floor(Date.now() / 1000) + 600; + return { + code: 0, + stdout: JSON.stringify({ + resources: { core: { remaining: 4747, reset }, graphql: { remaining: 3758, reset } }, + }), + stderr: '', + }; + } + if (args[0] === 'search' && failures > 0) { + failures -= 1; + return { code: 1, stdout: '', stderr: 'You have exceeded a secondary rate limit.' }; + } + return { code: 0, stdout: '[]', stderr: '' }; + }, + }); + return { gh, waits, notices }; + } + + it('backs off a secondary limit exponentially instead of every 65 s', async () => { + const { gh, waits, notices } = secondaryGh(3, 15 * 60_000); + const { summary } = await runSweep(gh, baseOptions()); + + expect(waits).toEqual([60_000, 120_000, 240_000]); + expect(notices[0]).toContain('secondary rate limit'); + expect(notices[0]).not.toContain('exhausted'); + expect(summary.failed).toBe(0); + }); + + it('gives up on a secondary limit once the back-off outgrows the budget', async () => { + const { gh, waits } = secondaryGh(99, 15 * 60_000); + const { summary, text } = await runSweep(gh, baseOptions({ orgs: ['acme'], users: [] })); + + // 1 + 2 + 4 + 8 = 15 minutes; the 16 m step is past the budget. + expect(waits).toEqual([60_000, 120_000, 240_000, 480_000]); + expect(text).toContain('FAILED org:acme — GitHub secondary rate limit still refusing'); + expect(summary.failed).toBe(1); }); it('waits out a reset inside the budget, then sweeps', async () => {