diff --git a/src/services/merge-failure.ts b/src/services/merge-failure.ts index 6003b37f1..df7173078 100644 --- a/src/services/merge-failure.ts +++ b/src/services/merge-failure.ts @@ -77,6 +77,14 @@ function isConvergenceForbiddenMessage(message: string): boolean { return /resource not accessible by integration|secondary rate limit|api rate limit|abuse detection/i.test(message); } +/** A GitHub rate-limit signal — the message GitHub attaches to a secondary/abuse or primary rate-limit + * response. A merge failing on this is an infra-scoped, self-healing window, whether GitHub spelled it 403 or + * 429 (src/github/client.ts treats both the same). Exported so the 403 and 429 branches share one definition + * (#9693). */ +export function isRateLimitMessage(message: string): boolean { + return /secondary rate limit|abuse|api rate limit exceeded/i.test(message); +} + /** Read the HTTP status off an Octokit RequestError (it sets `.status`); undefined for non-HTTP errors. */ function httpStatus(error: unknown): number | undefined { const status = (error as { status?: unknown } | null | undefined)?.status; @@ -119,6 +127,13 @@ export function classifyMergeFailure(error: unknown): { terminal: boolean; reaso const message = errorMessage(error); const status = httpStatus(error); if (status === 401) return { terminal: true, scope: "infra", reason: `installation token rejected: App suspended or key rotated (401): ${message}` }; + // A secondary rate-limit window surfaces as 403 OR 429 (src/github/client.ts:422) and is fleet-wide + self- + // healing — infra-scoped so it lapses on the TTL re-probe instead of stranding every in-flight merge on a + // head-scoped block until an unrelated commit lands (#9693). The 429 arm sits with the other status branches; + // the rate-limited-403 arm must precede the terminal `403` below so it is not swallowed by it. + if (status === 429) return { terminal: false, scope: "infra", reason: `merge rate-limited (429 — secondary rate limit, self-healing window): ${message}` }; + if (status === 403 && isRateLimitMessage(message)) + return { terminal: false, scope: "infra", reason: `merge rate-limited (403 — secondary rate limit, self-healing window): ${message}` }; if (status === 403 && isConvergenceForbiddenMessage(message)) return { terminal: false, scope: "infra", reason: `merge forbidden for now (403 — branch protection or GitHub permission visibility may still be converging): ${message}` }; if (status === 403) return { terminal: true, scope: "commit", reason: `merge forbidden (403): ${message}` }; diff --git a/test/unit/merge-block-recovery.test.ts b/test/unit/merge-block-recovery.test.ts index 6128f8f31..f777b6617 100644 --- a/test/unit/merge-block-recovery.test.ts +++ b/test/unit/merge-block-recovery.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; import { getPullRequest, markPullRequestMergeBlocked, bumpPullRequestMergeAttempt, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub, upsertInstallation } from "../../src/db/repositories"; -import { activeMergeBlockedSha, classifyMergeFailure, INFRA_MERGE_BLOCK_TTL_MS, isMergeBlockInEffect } from "../../src/services/merge-failure"; +import { activeMergeBlockedSha, classifyMergeFailure, INFRA_MERGE_BLOCK_TTL_MS, isMergeBlockInEffect, MERGE_RETRY_CAP } from "../../src/services/merge-failure"; import { AGENT_LABEL_NEEDS_REVIEW, AGENT_LABEL_READY, planAgentMaintenanceActions } from "../../src/settings/agent-actions"; import { createTestEnv } from "../helpers/d1"; @@ -115,6 +115,47 @@ describe("markPullRequestMergeBlocked persists the scope (#9012)", () => { }); }); +// #9693: a 429 secondary-rate-limit window that burns MERGE_RETRY_CAP must persist an INFRA-scoped (expiring) +// block, not a commit-scoped one that strands the PR until an unrelated commit lands. handleMergeFailure derives +// the expiry from classifyMergeFailure(...).scope, so we drive that composition exactly as the executor does. +describe("a rate-limit-exhausted merge persists a self-healing infra block (#9693)", () => { + const NOW = Date.parse("2026-07-26T12:00:00.000Z"); + const blockFor = (error: unknown) => { + // Mirror handleMergeFailure's retry-cap path: on exhaustion the classified scope decides the expiry. + const { scope, terminal } = classifyMergeFailure(error); + // A 429/rate-limited failure is non-terminal, so it reaches the cap path and escalates there. + expect(terminal).toBe(false); + return scope === "infra" ? new Date(NOW + INFRA_MERGE_BLOCK_TTL_MS).toISOString() : undefined; + }; + + it("persists a non-null, lapsing mergeBlockedUntil after MERGE_RETRY_CAP 429 failures", async () => { + const env = createTestEnv(); + await seedPr(env, "sha-1"); + // Exhaust the retry budget on the same head, exactly as repeated 429 attempts would. + for (let i = 0; i < MERGE_RETRY_CAP; i += 1) await bumpPullRequestMergeAttempt(env, "alice/repo", 5, "sha-1"); + const expiresAt = blockFor(httpError(429, "You have exceeded a secondary rate limit")); + expect(expiresAt).not.toBeUndefined(); + await markPullRequestMergeBlocked(env, "alice/repo", 5, "sha-1", "merge could not complete after 5 attempt(s)", expiresAt); + + const stored = await getPullRequest(env, "alice/repo", 5); + expect(stored?.mergeBlockedUntil).not.toBeNull(); + // The block genuinely lapses on the TTL — no new commit required. + expect(isMergeBlockInEffect(stored!, "sha-1", NOW + INFRA_MERGE_BLOCK_TTL_MS + 1)).toBe(false); + }); + + it("keeps a 409 merge-conflict block commit-scoped (mergeBlockedUntil null), unchanged", async () => { + const env = createTestEnv(); + await seedPr(env, "sha-1"); + // A 409 is terminal on the first failure — commit-scoped, no expiry. + const { scope, terminal } = classifyMergeFailure(httpError(409, "Required status check is expected.")); + expect(terminal).toBe(true); + const expiresAt = scope === "infra" ? new Date(NOW + INFRA_MERGE_BLOCK_TTL_MS).toISOString() : undefined; + await markPullRequestMergeBlocked(env, "alice/repo", 5, "sha-1", "merge conflict (409)", expiresAt); + + expect((await getPullRequest(env, "alice/repo", 5))?.mergeBlockedUntil ?? null).toBeNull(); + }); +}); + // #9012 compounding bug: mergeAttemptCount's own schema and function docs promised "a new commit's attempts // start fresh once the row's head advances", but nothing reset it — bumpPullRequestMergeAttempt only scoped the // INCREMENT to the head. So once one head exhausted MERGE_RETRY_CAP, every later head was one-strike-terminal. diff --git a/test/unit/merge-failure.test.ts b/test/unit/merge-failure.test.ts index 62bbf600d..2156c5233 100644 --- a/test/unit/merge-failure.test.ts +++ b/test/unit/merge-failure.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { classifyMergeFailure, isMergeConflictMessage, isNoNewBaseCommitsMessage, isWorkflowScopeRefusalMessage, MERGE_RETRY_CAP } from "../../src/services/merge-failure"; +import { classifyMergeFailure, isMergeConflictMessage, isNoNewBaseCommitsMessage, isRateLimitMessage, isWorkflowScopeRefusalMessage, MERGE_RETRY_CAP } from "../../src/services/merge-failure"; /** Build an Octokit-style RequestError: an Error carrying an HTTP `.status`. */ function httpError(status: number, message: string): Error { @@ -36,13 +36,33 @@ describe("classifyMergeFailure", () => { }); it("retries GitHub's generic 403 merge rejection because branch protection can still converge", () => { - for (const message of ["Resource not accessible by integration", "secondary rate limit", "API rate limit exceeded", "abuse detection mechanism triggered"]) { + // A non-rate-limit convergence 403 (permission visibility still settling) — the converging arm. + const result = classifyMergeFailure(httpError(403, "Resource not accessible by integration")); + expect(result.terminal).toBe(false); + expect(result.scope).toBe("infra"); + expect(result.reason).toMatch(/converging/i); + }); + + it("classifies a 429 and a rate-limited 403 as an infra-scoped, self-healing window (#9693)", () => { + // 429: no branch matched this before, so it fell through to a commit-scoped block that stranded the PR. + const rateLimited429 = classifyMergeFailure(httpError(429, "You have exceeded a secondary rate limit")); + expect(rateLimited429).toMatchObject({ terminal: false, scope: "infra" }); + expect(rateLimited429.reason).toMatch(/rate-limited/i); + // 403 spellings of the same window are treated identically, before the terminal 403 branch. + for (const message of ["secondary rate limit", "API rate limit exceeded", "abuse detection mechanism triggered"]) { const result = classifyMergeFailure(httpError(403, message)); - expect(result.terminal).toBe(false); - expect(result.reason).toMatch(/converging/i); + expect(result).toMatchObject({ terminal: false, scope: "infra" }); + expect(result.reason).toMatch(/rate-limited/i); } }); + it("exposes isRateLimitMessage matching only rate-limit text (#9693)", () => { + expect(isRateLimitMessage("You have exceeded a secondary rate limit")).toBe(true); + expect(isRateLimitMessage("abuse detection mechanism triggered")).toBe(true); + expect(isRateLimitMessage("API rate limit exceeded")).toBe(true); + expect(isRateLimitMessage("Repository does not allow squash merges")).toBe(false); + }); + it("treats non-convergence 403s, 409, and real merge-conflict text as terminal", () => { expect(classifyMergeFailure(httpError(403, "Repository does not allow squash merges")).terminal).toBe(true); expect(classifyMergeFailure(httpError(409, "Required status check is expected.")).terminal).toBe(true);