diff --git a/README.md b/README.md index 8fee397d..cdef02e5 100644 --- a/README.md +++ b/README.md @@ -364,7 +364,8 @@ Each poll makes one conditional timeline request and one conditional review-comment request every five minutes once it has read the existing pages, following a new page when the current one fills, plus one GraphQL query each for reactions and checks. It reads the base branch's required checks once per -call. It honors GitHub's rate-limit headers and fails loudly on invalid responses. +call. It honors GitHub's rate-limit headers, retries a 5xx or a dropped +connection a few times, and fails loudly on invalid responses. There is no deadline unless `--timeout ` is supplied. Activity exits 0, timeout exits 3, invalid arguments exit 2, and other failures exit 1; interrupt with Ctrl-C to stop waiting. A timeout prints a resume point as its only line: diff --git a/cloudflare/dev/github.ts b/cloudflare/dev/github.ts index 57d5a2c4..8ad7c117 100644 --- a/cloudflare/dev/github.ts +++ b/cloudflare/dev/github.ts @@ -39,6 +39,8 @@ export type Pull = Issue & { merged?: boolean; /** Optional squash result supplied by fixtures that model real git merges. */ squash?: string; + /** The commit a REST merge made. */ + mergeCommit?: string; /** REST mergeability can lag the base; fixtures can also model policy refusals. */ mergeable?: boolean | null; mergeableState?: string; @@ -565,6 +567,7 @@ export function fakeGitHub(world: World) { html_url: `https://github.com/${repo.owner}/${repo.name}/pull/${pull.number}`, state: pull.closed ? "closed" : "open", merged: pull.merged ?? false, + merge_commit_sha: pull.mergeCommit ?? null, mergeable: pull.mergeable !== undefined ? pull.mergeable : pull.merge !== "DIRTY", mergeable_state: pull.mergeableState ?? (pull.merge === "DIRTY" ? "dirty" : "clean"), title: pull.title, @@ -920,8 +923,9 @@ export function fakeGitHub(world: World) { pull.landing = false; pull.autoMerge = undefined; if (pull.squash) repo.branches[pull.base ?? repo.defaultBranch ?? "main"] = pull.squash; + pull.mergeCommit = pull.squash ?? commitOf(repo); return json({ - sha: pull.squash ?? commitOf(repo), + sha: pull.mergeCommit, merged: true, message: "Pull Request successfully merged", }); diff --git a/quest/m0/README.md b/quest/m0/README.md index 8a5b5a37..47ccaebe 100644 --- a/quest/m0/README.md +++ b/quest/m0/README.md @@ -69,7 +69,6 @@ ownership-manifest design: - [The land fixer handles every mechanical failure](/quest/m0/land-fixer/README.md) - attempts, registry network, triage, flaky tests, local mergeability, protected edits, and fork commits - [Landing priority](/quest/m0/land-priority.md) - cheap checks wait a bounded time and the land daemon's check runs ahead of agent builds - [Drop emptied Required headings](/quest/m0/required-heading.md) - a clean driver merge that empties a `Required` list passes `quest check` -- [GitHub retries](/quest/m0/github-retry.md) - one 5xx or dropped connection no longer fails a command or land step - [Current conflict reports](/quest/m0/land-conflicts-refresh.md) - a queued PR's conflict report updates when the queue ahead of it changes - [Land checks at gate priority](/quest/m0/land-priority-sandbox.md) - `quest land serve` passes `QUEST_GATE_PRIORITY` to its sandbox - [Kix decision UI](/quest/m0/kix/README.md) - planning and implementation through prompts and summaries, run on the maintainer's own runners diff --git a/quest/m0/github-retry.md b/quest/m0/github-retry.md deleted file mode 100644 index d7fe5f04..00000000 --- a/quest/m0/github-retry.md +++ /dev/null @@ -1,21 +0,0 @@ -# [S] GitHub requests retry transient failures - -## Goal - -One 5xx response or dropped connection no longer fails a quest command or a -land step. `GitHub.request` (`src/cli/github.ts`) retries 5xx, 429 and -network errors with bounded backoff, then fails with the last error. - -## Plan - -Decided 2026-10-31 (user, /quest-plan of the follow-ups from moq.pro's and rust2's cutover to the host gate and the fixer repairs [moq-dev/quest#320](https://github.com/moq-dev/quest/pull/320), [#325](https://github.com/moq-dev/quest/pull/325), [#327](https://github.com/moq-dev/quest/pull/327) and [#328](https://github.com/moq-dev/quest/pull/328)). - -- A 502 while the fixer handed back - [moq-dev/moq#7323](https://github.com/moq-dev/moq/pull/7323) replaced the - crash reason; [moq-dev/quest#320](https://github.com/moq-dev/quest/pull/320) - kept the original error but left the retry for this quest. -- Retry only idempotent reads and writes whose repeat is safe (GET, PUT, - DELETE, label and auto-merge mutations that check state first). A POST that - creates a comment or a merge must not run twice; check the effect before - repeating it. -- Honour `Retry-After` and the secondary rate limit. diff --git a/src/cli/github.ts b/src/cli/github.ts index a4d5b275..dfe18754 100644 --- a/src/cli/github.ts +++ b/src/cli/github.ts @@ -35,6 +35,29 @@ export class GitHubDeadline extends Error { export type ObjectValue = Record; +/** Retries of a 5xx or a dropped connection, waiting 1, 2, 4, then 8 seconds unless GitHub says how long. */ +const TRANSIENT_RETRIES = 4; +/** How far GitHub's clock may run behind ours when looking for what a lost request created. */ +const CLOCK_SKEW = 60_000; +/** Statuses whose response has no body. */ +const NULL_BODY = [204, 205, 304]; + +/** + * Whether a request may repeat after a 5xx or a dropped connection, after which GitHub may have + * acted anyway: `true` when repeating leaves the same state, or a check, run before each repeat, + * for the response of an earlier attempt that took effect (undefined if none did). + */ +export type Repeat = boolean | ((started: number) => Promise); + +export interface RequestOptions { + etag?: string; + body?: unknown; + method?: "POST" | "PUT" | "PATCH" | "DELETE"; + allow?: number[]; + /** Overrides how the request repeats; the default follows its method and endpoint. */ + repeat?: Repeat; +} + /** Accept a GitHub PR URL, or a number in the origin repository. */ export function pullRequest(value: string, root: string): PullRequest { if (/^[1-9]\d*$/.test(value)) { @@ -132,22 +155,25 @@ export class GitHub { } /** - * A 2xx or 304 response; anything else fails after rate-limit retries, with - * GitHub's reason when it gives one, unless `allow` lists its status. `body` makes it a POST - * unless `method` says otherwise. + * A 2xx or 304 response, its body already read; anything else fails, with GitHub's reason when + * it gives one, unless `allow` lists its status. Rate limits are waited out. A 5xx or a + * connection dropped before the body is in, after which GitHub may or may not have acted, is + * retried with bounded backoff as `repeat` allows, then fails with the last error. `body` makes + * it a POST unless `method` says otherwise. */ - async request( - path: string, - options: { etag?: string; body?: unknown; method?: "POST" | "PUT" | "PATCH" | "DELETE"; allow?: number[] } = {}, - ): Promise { + async request(path: string, options: RequestOptions = {}): Promise { // Follow only same-origin pagination links; never send a token to another host. const url = new URL(path, API); if (url.origin !== API || url.username || url.password) throw new Error("invalid GitHub pagination URL"); - let attempts = 0; + const started = this.deps.now(); + const repeat = options.repeat ?? (await this.repeatable(url, options, started)); + let limits = 0; + let failures = 0; for (;;) { const remaining = this.deadline - this.deps.now(); if (remaining <= 0) throw new GitHubDeadline(); - let response: Response; + let response: Response | undefined; + let failure: unknown; try { response = await this.deps.fetch(url, { ...(options.body === undefined @@ -164,34 +190,137 @@ export class GitHub { }); } catch (error) { if (this.deps.now() >= this.deadline) throw new GitHubDeadline(); - throw error; + failure = error; } - this.pollInterval = Math.max(this.pollInterval, seconds(response.headers.get("x-poll-interval")) ?? 0); - if (response.ok || response.status === 304 || options.allow?.includes(response.status)) return response; - const body = await response.text(); - const limited = - response.status === 429 || - (response.status === 403 && - (response.headers.has("retry-after") || - response.headers.get("x-ratelimit-remaining") === "0" || - /rate limit/i.test(body))); - if (!limited || attempts >= 6) - throw new Error( - `GitHub HTTP ${response.status} for ${url.pathname}; ${limited ? "rate limit persisted" : (explanation(body) ?? "check repository access and token permissions")}`, + let retry: number | undefined; + if (response) { + this.pollInterval = Math.max(this.pollInterval, seconds(response.headers.get("x-poll-interval")) ?? 0); + // Read the body here, so a connection that drops mid-body is retried like one that drops before it. + const read = await response.text().then( + (text) => ({ text }), + (error: unknown) => ({ error }), ); - const retry = seconds(response.headers.get("retry-after")); - const reset = - response.headers.get("x-ratelimit-remaining") === "0" - ? seconds(response.headers.get("x-ratelimit-reset")) - : undefined; - const delay = - retry ?? (reset === undefined ? 60_000 * 2 ** attempts : Math.max(1000, reset - this.deps.now())); - attempts++; - this.deps.warn(`GitHub rate limited this request; waiting ${Math.ceil(delay / 1000)} seconds`); + if (response.ok || response.status === 304 || options.allow?.includes(response.status)) { + const { status, statusText, headers } = response; + if ("text" in read) + return new Response(NULL_BODY.includes(status) ? null : read.text, { + status, + statusText, + headers, + }); + if (this.deps.now() >= this.deadline) throw new GitHubDeadline(); + failure = read.error; + } else { + // A refusal's status still says what to do when its body was cut off. + const body = "text" in read ? read.text : ""; + const limited = + response.status === 429 || + (response.status === 403 && + (response.headers.has("retry-after") || + response.headers.get("x-ratelimit-remaining") === "0" || + /rate limit/i.test(body))); + if (limited) { + // GitHub turned the request away unprocessed, so any request repeats. + await this.waitOut( + response.headers, + limits++, + `GitHub HTTP ${response.status} for ${url.pathname}; rate limit persisted`, + ); + continue; + } + const reason = explanation(body); + if (response.status < 500) + throw new Error( + `GitHub HTTP ${response.status} for ${url.pathname}; ${reason ?? "check repository access and token permissions"}`, + ); + failure = new Error( + `GitHub HTTP ${response.status} for ${url.pathname}; ${reason ?? "GitHub server error"}`, + ); + retry = seconds(response.headers.get("retry-after")); + } + } + if (repeat === false || failures >= TRANSIENT_RETRIES) throw failure; + const delay = retry ?? 1000 * 2 ** failures; + failures++; + this.deps.warn(`${message(failure)}; retrying in ${Math.ceil(delay / 1000)} seconds`); await this.pause(delay); + if (typeof repeat === "function") { + const landed = await repeat(started); + if (landed) return landed; + } } } + /** + * Wait out a rate limit, the `limits`th in a row, as its headers say: Retry-After, else the + * primary limit's reset, else a minute doubling each time. Fails with `persisted` after six. + */ + private async waitOut(headers: Headers, limits: number, persisted: string): Promise { + if (limits >= 6) throw new Error(persisted); + const retry = seconds(headers.get("retry-after")); + const reset = + headers.get("x-ratelimit-remaining") === "0" ? seconds(headers.get("x-ratelimit-reset")) : undefined; + const delay = retry ?? (reset === undefined ? 60_000 * 2 ** limits : Math.max(1000, reset - this.deps.now())); + this.deps.warn(`GitHub rate limited this request; waiting ${Math.ceil(delay / 1000)} seconds`); + await this.pause(delay); + } + + /** + * How a request may repeat after a 5xx or a dropped connection. A GET reads and a PUT, PATCH, or + * DELETE sets state, so each repeats; so does adding labels. A merge is confirmed first by its + * pinned head, and a POST that creates something by what it created: a comment by its body, among + * those not already there before it, a pull request by its branch. Any other POST never repeats. + */ + private async repeatable(url: URL, options: RequestOptions, started: number): Promise { + const method = options.method ?? (options.body === undefined ? "GET" : "POST"); + const body = typeof options.body === "object" && options.body !== null ? (options.body as ObjectValue) : {}; + const merge = /^(\/repos\/[^/]+\/[^/]+\/pulls\/\d+)\/merge$/.exec(url.pathname); + if (method === "PUT" && merge) { + return async () => { + const pull = object(await (await this.request(merge[1])).json()); + // Merged at another head is not this merge; repeating it gets GitHub's refusal for the caller. + if (pull.merged !== true || typeof pull.merge_commit_sha !== "string") return; + if (body.sha !== undefined && object(pull.head).sha !== body.sha) return; + return Response.json({ + merged: true, + sha: pull.merge_commit_sha, + message: "Pull Request successfully merged", + }); + }; + } + if (method !== "POST") return true; + const issue = /^\/repos\/[^/]+\/[^/]+\/issues\/\d+\/(labels|comments)$/.exec(url.pathname); + if (issue?.[1] === "labels") return true; + if (issue?.[1] === "comments" && typeof body.body === "string") { + const since = new Date(started - CLOCK_SKEW).toISOString(); + const identical = async () => + (await this.all(`${url.pathname}?since=${since}&per_page=100`)).filter( + (comment) => comment.body === body.body, + ); + // An identical comment already there, such as one from a requeued PR's earlier ejection, is not this one. + const earlier = new Set((await identical()).map((comment) => comment.id)); + return async () => { + const found = (await identical()).find((comment) => !earlier.has(comment.id)); + return found && Response.json(found, { status: 201 }); + }; + } + const pulls = /^\/repos\/([^/]+)\/[^/]+\/pulls$/.exec(url.pathname); + if (pulls && typeof body.head === "string") { + const head = body.head.includes(":") ? body.head : `${pulls[1]}:${body.head}`; + return async (started) => { + const open = await this.list( + `${url.pathname}?state=open&head=${encodeURIComponent(head)}&per_page=100`, + ); + const found = open.rows.find( + (pull) => + typeof pull.created_at === "string" && Date.parse(pull.created_at) >= started - CLOCK_SKEW, + ); + return found && Response.json(found, { status: 201 }); + }; + } + return false; + } + /** One page of a REST listing, and its response for the next link. */ async list(path: string): Promise<{ rows: ObjectValue[]; response: Response }> { const response = await this.request(path); @@ -210,20 +339,65 @@ export class GitHub { return rows; } - /** Run a GraphQL query and return its data, failing on any reported error. */ + /** + * Run a GraphQL query and return its data, failing on any reported error. A query repeats after + * a 5xx or a dropped connection; a mutation only once its effect is confirmed missing, and one + * confirmed to have taken effect returns no data. GitHub can report a rate limit as a 200 whose + * errors say `RATE_LIMITED`; that is waited out like a REST one. + */ async graphql(query: string, variables: ObjectValue): Promise { - const response = await this.request("/graphql", { body: { query, variables } }); - const body = object(await response.json()); - if (body.errors !== undefined) - throw new Error( - `GitHub GraphQL error: ${array(body.errors) - .map((error) => String(error.message)) - .join("; ")}`, - ); - return object(body.data); + const repeat = this.mutationRepeat(query, variables); + for (let limits = 0; ; limits++) { + const response = await this.request("/graphql", { body: { query, variables }, repeat }); + const body = object(await response.json()); + if (body.errors === undefined) return object(body.data); + const errors = array(body.errors); + if (!errors.some((error) => error.type === "RATE_LIMITED")) + throw new Error(`GitHub GraphQL error: ${errors.map((error) => String(error.message)).join("; ")}`); + // GitHub turned the operation away unprocessed, so any operation repeats. + await this.waitOut(response.headers, limits, "GitHub GraphQL rate limit persisted"); + } + } + + /** + * Repeat a query; confirm a pull request mutation `PULL_MUTATIONS` knows; never repeat another. + * One pinned to `expectedHeadOid` is confirmed only at that head; at any other, its repeat gets + * GitHub's refusal for the caller. + */ + private mutationRepeat(query: string, variables: ObjectValue): Repeat { + if (!/^\s*mutation\b/.test(query)) return true; + const call = /^\s*mutation\b[^{]*\{\s*(\w+)\s*\(\s*input:\s*\{\s*pullRequestId:\s*\$(\w+)\b/.exec(query); + const effect = call && Object.hasOwn(PULL_MUTATIONS, call[1]) ? PULL_MUTATIONS[call[1]] : undefined; + const id = call && variables[call[2]]; + if (!effect || typeof id !== "string") return false; + const pinned = /\bexpectedHeadOid:\s*\$(\w+)\b/.exec(query); + const head = pinned ? variables[pinned[1]] : undefined; + const read = `query($id: ID!) { node(id: $id) { ... on PullRequest { headRefOid ${effect.fields} } } }`; + return async () => { + const pull = object((await this.graphql(read, { id })).node); + const landed = (head === undefined || pull.headRefOid === head) && effect.landed(pull); + return landed ? Response.json({ data: {} }) : undefined; + }; } } +/** + * Pull request mutations whose effect can be read back: the fields that show it, and whether + * they do. Each takes its pull request as `pullRequestId`. + */ +const PULL_MUTATIONS: Record = { + mergePullRequest: { fields: "merged", landed: (pull) => pull.merged === true }, + enablePullRequestAutoMerge: { + fields: "merged autoMergeRequest { enabledAt } mergeQueueEntry { state }", + landed: (pull) => pull.merged === true || pull.autoMergeRequest != null || pull.mergeQueueEntry != null, + }, + disablePullRequestAutoMerge: { + fields: "autoMergeRequest { enabledAt }", + landed: (pull) => pull.autoMergeRequest === null, + }, + markPullRequestReadyForReview: { fields: "isDraft", landed: (pull) => pull.isDraft === false }, +}; + /** A check a base branch requires. */ export interface RequiredCheck { name: string; @@ -345,6 +519,10 @@ function explanation(body: string): string | undefined { } } +function message(error: unknown): string { + return error instanceof Error ? error.message : String(error); +} + function seconds(value: string | null): number | undefined { if (value === null || !/^\d+(?:\.\d+)?$/.test(value)) return undefined; const result = Number(value) * 1000; diff --git a/test/github.test.ts b/test/github.test.ts new file mode 100644 index 00000000..11a1c9cd --- /dev/null +++ b/test/github.test.ts @@ -0,0 +1,397 @@ +import { describe, expect, test } from "vitest"; + +import { GitHub, GitHubDeadline, type GitHubDependencies } from "../src/cli/github"; + +const repo = "/repos/example/project"; +const HEAD = "a".repeat(40); +const SQUASH = "b".repeat(40); + +/** One request as the fake saw it. */ +interface Call { + method: string; + path: string; + body?: { query?: string; variables?: Record } & Record; +} + +type Reply = Response | Error | ((call: Call) => Response | Error); + +function json(body: unknown, status = 200, headers: Record = {}): Response { + return new Response(JSON.stringify(body), { status, headers }); +} + +const bad = (status: number, headers: Record = {}) => + json({ message: "Server Error" }, status, headers); +const dropped = () => new TypeError("fetch failed: connection reset"); +/** A response whose connection drops while its body is read. */ +const cut = (status = 200) => + new Response( + new ReadableStream({ + start(controller) { + controller.error(new TypeError("body cut off")); + }, + }), + { status }, + ); + +/** A client whose fetch answers each request with the next reply, on a clock only its sleeps advance. */ +function fixture(replies: Reply[], deadline = Infinity) { + let now = Date.parse("2026-10-31T12:00:00Z"); + const calls: Call[] = []; + const sleeps: number[] = []; + const warnings: string[] = []; + const deps: GitHubDependencies = { + token: "token", + now: () => now, + sleep: async (milliseconds) => { + sleeps.push(milliseconds); + now += milliseconds; + }, + warn: (text) => warnings.push(text), + fetch: (async (input, init) => { + const url = new URL(String(input)); + const call: Call = { + method: init?.method ?? "GET", + path: url.pathname + url.search, + ...(init?.body ? { body: JSON.parse(String(init.body)) } : {}), + }; + calls.push(call); + const next = replies.shift(); + if (next === undefined) throw new Error(`unexpected ${call.method} ${call.path}`); + const reply = typeof next === "function" ? next(call) : next; + if (reply instanceof Error) throw reply; + return reply; + }) as typeof fetch, + }; + const github = new GitHub(deps, deadline === Infinity ? Infinity : now + deadline); + return { github, calls, sleeps, warnings, now: () => now }; +} + +describe("transient failures", () => { + test("a read retries a 5xx with backoff and returns the next success", async () => { + const { github, calls, sleeps, warnings } = fixture([bad(502), bad(500), json({ ok: true })]); + const response = await github.request(`${repo}/pulls/1`); + expect(await response.json()).toEqual({ ok: true }); + expect(calls).toHaveLength(3); + expect(sleeps).toEqual([1000, 2000]); + expect(warnings[0]).toBe(`GitHub HTTP 502 for ${repo}/pulls/1; Server Error; retrying in 1 seconds`); + }); + + test("a persistent 5xx fails with the last error after bounded retries", async () => { + const { github, calls, sleeps } = fixture([bad(502), bad(502), bad(502), bad(502), json({}, 503)]); + await expect(github.request(`${repo}/pulls/1`)).rejects.toThrow( + `GitHub HTTP 503 for ${repo}/pulls/1; GitHub server error`, + ); + expect(calls).toHaveLength(5); + expect(sleeps).toEqual([1000, 2000, 4000, 8000]); + }); + + test("a dropped connection retries, and a persistent one rethrows its error", async () => { + const recovered = fixture([dropped(), json([])]); + await recovered.github.request(`${repo}/pulls`); + expect(recovered.sleeps).toEqual([1000]); + + const persistent = fixture(Array.from({ length: 5 }, dropped)); + await expect(persistent.github.request(`${repo}/pulls`)).rejects.toThrow("connection reset"); + expect(persistent.calls).toHaveLength(5); + }); + + test("a read whose body is cut off retries, and the body returned is already read", async () => { + const { github, calls, sleeps, warnings } = fixture([cut(), json([{ id: 1 }])]); + const response = await github.request(`${repo}/pulls`); + expect(await response.json()).toEqual([{ id: 1 }]); + expect(calls).toHaveLength(2); + expect(sleeps).toEqual([1000]); + expect(warnings).toEqual(["body cut off; retrying in 1 seconds"]); + }); + + test("an accepted status with no body keeps its status and headers", async () => { + const { github } = fixture([new Response(null, { status: 304, headers: { etag: '"x"' } })]); + const response = await github.request(`${repo}/pulls`, { etag: '"x"' }); + expect(response.status).toBe(304); + expect(response.headers.get("etag")).toBe('"x"'); + }); + + test("a refusal whose body is cut off still acts on its status", async () => { + const notFound = fixture([cut(404)]); + await expect(notFound.github.request(`${repo}/pulls/1`)).rejects.toThrow("GitHub HTTP 404"); + expect(notFound.calls).toHaveLength(1); + + const limited = fixture([cut(429), json({})]); + await limited.github.request(`${repo}/pulls/1`); + expect(limited.sleeps).toEqual([60_000]); + }); + + test("a 5xx honours Retry-After", async () => { + const { github, sleeps } = fixture([bad(503, { "retry-after": "30" }), json({})]); + await github.request(`${repo}/pulls/1`); + expect(sleeps).toEqual([30_000]); + }); + + test("a 4xx is not retried", async () => { + const { github, calls, sleeps } = fixture([json({ message: "Not Found" }, 404)]); + await expect(github.request(`${repo}/pulls/1`)).rejects.toThrow("GitHub HTTP 404"); + expect(calls).toHaveLength(1); + expect(sleeps).toEqual([]); + }); + + test("the deadline cuts a backoff short", async () => { + const { github, sleeps } = fixture([bad(502)], 500); + await expect(github.request(`${repo}/pulls/1`)).rejects.toBeInstanceOf(GitHubDeadline); + expect(sleeps).toEqual([500]); + }); + + test("PUT, PATCH, DELETE, and adding labels repeat", async () => { + const { github, calls } = fixture([ + bad(502), + json({}), + dropped(), + json({}), + bad(504), + new Response(null, { status: 204 }), + bad(502), + json([]), + ]); + await github.request(`${repo}/pulls/1/update-branch`, { method: "PUT", body: {} }); + await github.request(`${repo}/pulls/1`, { method: "PATCH", body: { state: "closed" } }); + await github.request(`${repo}/issues/1/labels/x`, { method: "DELETE" }); + await github.request(`${repo}/issues/1/labels`, { body: { labels: ["x"] } }); + expect(calls.map(({ method }) => method)).toEqual([ + "PUT", + "PUT", + "PATCH", + "PATCH", + "DELETE", + "DELETE", + "POST", + "POST", + ]); + }); + + test("a rate-limited POST repeats, since GitHub refused it unprocessed", async () => { + const { github, calls } = fixture([ + json({ message: "secondary rate limit" }, 403, { "retry-after": "60" }), + json({}), + ]); + await github.request(`${repo}/dispatches`, { body: {} }); + expect(calls).toHaveLength(2); + }); + + test("any other POST never repeats after a 5xx or a dropped connection", async () => { + for (const failure of [bad(502), dropped()]) { + const { github, calls, sleeps } = fixture([failure]); + await expect(github.request(`${repo}/dispatches`, { body: {} })).rejects.toThrow(); + expect(calls).toHaveLength(1); + expect(sleeps).toEqual([]); + } + }); +}); + +describe("confirmed writes", () => { + const text = "landed by quest land"; + + test("a comment whose response was lost is found by its body and not posted again", async () => { + const { github, calls } = fixture([ + json([{ id: 1, body: "older" }]), + bad(502), + json([ + { id: 1, body: "older" }, + { id: 2, body: text }, + ]), + ]); + const response = await github.request(`${repo}/issues/1/comments`, { body: { body: text } }); + expect(await response.json()).toEqual({ id: 2, body: text }); + // A minute before the first attempt, for GitHub's clock. + const listing = `GET ${repo}/issues/1/comments?since=2026-10-31T11:59:00.000Z&per_page=100`; + expect(calls.map(({ method, path }) => `${method} ${path}`)).toEqual([ + listing, + `POST ${repo}/issues/1/comments`, + listing, + ]); + }); + + test("an identical comment from before a lost one, as when a PR is ejected again, is posted again", async () => { + const earlier = { id: 1, body: text }; + const { github, calls } = fixture([json([earlier]), dropped(), json([earlier]), json({ id: 2 }, 201)]); + await github.request(`${repo}/issues/1/comments`, { body: { body: text } }); + expect(calls.map(({ method }) => method)).toEqual(["GET", "POST", "GET", "POST"]); + }); + + test("a comment whose response body was cut off is confirmed, not posted again", async () => { + const { github, calls } = fixture([json([]), cut(201), json([{ id: 2, body: text }])]); + const response = await github.request(`${repo}/issues/1/comments`, { body: { body: text } }); + expect(await response.json()).toEqual({ id: 2, body: text }); + expect(calls.map(({ method }) => method)).toEqual(["GET", "POST", "GET"]); + }); + + test("any other POST whose response body was cut off fails without repeating", async () => { + const { github, calls } = fixture([cut(201)]); + await expect(github.request(`${repo}/dispatches`, { body: {} })).rejects.toThrow("body cut off"); + expect(calls).toHaveLength(1); + }); + + test("a comment that did not post is posted again", async () => { + const { github, calls } = fixture([ + json([]), + dropped(), + json([{ id: 1, body: "older" }]), + json({ id: 2 }, 201), + ]); + await github.request(`${repo}/issues/1/comments`, { body: { body: text } }); + expect(calls.map(({ method }) => method)).toEqual(["GET", "POST", "GET", "POST"]); + }); + + test("a pull request whose response was lost is found by its branch", async () => { + const created = { number: 7, created_at: "2026-10-31T12:00:00Z" }; + const { github, calls } = fixture([ + bad(502), + json([{ number: 3, created_at: "2026-10-01T00:00:00Z" }, created]), + ]); + const response = await github.request(`${repo}/pulls`, { body: { head: "quest/m0/a", base: "main" } }); + expect(await response.json()).toEqual(created); + expect(calls[1].path).toBe(`${repo}/pulls?state=open&head=example%3Aquest%2Fm0%2Fa&per_page=100`); + }); + + test("a merge whose response was lost is confirmed from the PR at its pinned head", async () => { + const merged = { merged: true, merge_commit_sha: SQUASH, head: { sha: HEAD } }; + const { github, calls } = fixture([bad(502), json(merged)]); + const response = await github.request(`${repo}/pulls/1/merge`, { + method: "PUT", + body: { merge_method: "squash", sha: HEAD }, + allow: [405, 409], + }); + expect(await response.json()).toMatchObject({ merged: true, sha: SQUASH }); + expect(calls.map(({ method }) => method)).toEqual(["PUT", "GET"]); + }); + + test("a merge not confirmed at its pinned head is sent again, for GitHub to merge or refuse", async () => { + for (const pull of [ + { merged: false, merge_commit_sha: null, head: { sha: HEAD } }, + { merged: true, merge_commit_sha: SQUASH, head: { sha: "c".repeat(40) } }, + ]) { + const refusal = json({ message: "Pull Request is not mergeable" }, 405); + const { github, calls } = fixture([dropped(), json(pull), refusal]); + const response = await github.request(`${repo}/pulls/1/merge`, { + method: "PUT", + body: { merge_method: "squash", sha: HEAD }, + allow: [405, 409], + }); + expect(response.status).toBe(405); + expect(calls.map(({ method }) => method)).toEqual(["PUT", "GET", "PUT"]); + } + }); +}); + +describe("GraphQL", () => { + const disable = + "mutation($id: ID!) { disablePullRequestAutoMerge(input: { pullRequestId: $id }) { clientMutationId } }"; + + test("a query repeats", async () => { + const { github, calls } = fixture([bad(502), json({ data: { viewer: { login: "bot" } } })]); + expect(await github.graphql("query { viewer { login } }", {})).toEqual({ viewer: { login: "bot" } }); + expect(calls).toHaveLength(2); + }); + + test("a pull request mutation confirmed to have taken effect is not sent again", async () => { + const { github, calls } = fixture([bad(502), json({ data: { node: { autoMergeRequest: null } } })]); + expect(await github.graphql(disable, { id: "PR_1" })).toEqual({}); + expect(calls).toHaveLength(2); + expect(calls[1].body?.query).toContain( + "node(id: $id) { ... on PullRequest { headRefOid autoMergeRequest { enabledAt } } }", + ); + expect(calls[1].body?.variables).toEqual({ id: "PR_1" }); + }); + + test("a pull request mutation confirmed missing is sent again", async () => { + const { github, calls } = fixture([ + dropped(), + json({ data: { node: { autoMergeRequest: { enabledAt: "2026-10-31T11:00:00Z" } } } }), + json({ data: { disablePullRequestAutoMerge: { clientMutationId: null } } }), + ]); + await github.graphql(disable, { id: "PR_1" }); + expect(calls.map(({ body }) => body?.query?.split("(")[0])).toEqual(["mutation", "query", "mutation"]); + }); + + test.each([ + ["mergePullRequest", { merged: true }], + ["enablePullRequestAutoMerge", { merged: false, autoMergeRequest: null, mergeQueueEntry: { state: "QUEUED" } }], + ["markPullRequestReadyForReview", { isDraft: false }], + ])("%s is confirmed from the pull request at its pinned head", async (mutation, node) => { + const query = `mutation($id: ID!, $sha: GitObjectID!) { ${mutation}(input: { pullRequestId: $id, expectedHeadOid: $sha }) { clientMutationId } }`; + const confirmed = fixture([bad(502), json({ data: { node: { ...node, headRefOid: HEAD } } })]); + await confirmed.github.graphql(query, { id: "PR_1", sha: HEAD }); + expect(confirmed.calls).toHaveLength(2); + + // Another head took effect, not this mutation: the repeat gets GitHub's refusal. + const refusal = json({ errors: [{ message: "Head sha didn't match expected head" }] }); + const moved = fixture([bad(502), json({ data: { node: { ...node, headRefOid: SQUASH } } }), refusal]); + await expect(moved.github.graphql(query, { id: "PR_1", sha: HEAD })).rejects.toThrow("Head sha didn't match"); + expect(moved.calls).toHaveLength(3); + }); + + const limited = (headers: Record = {}) => + json({ errors: [{ type: "RATE_LIMITED", message: "API rate limit exceeded" }] }, 200, headers); + const addComment = 'mutation($id: ID!) { addComment(input: { subjectId: $id, body: "x" }) { clientMutationId } }'; + + test("a RATE_LIMITED 200 is waited out until the reset, then the operation repeats", async () => { + const reset = Date.parse("2026-10-31T12:05:00Z") / 1000; + const { github, calls, sleeps, warnings } = fixture([ + limited({ "x-ratelimit-remaining": "0", "x-ratelimit-reset": `${reset}` }), + json({ data: { viewer: { login: "bot" } } }), + ]); + expect(await github.graphql("query { viewer { login } }", {})).toEqual({ viewer: { login: "bot" } }); + expect(calls).toHaveLength(2); + expect(sleeps).toEqual([300_000]); + expect(warnings).toEqual(["GitHub rate limited this request; waiting 300 seconds"]); + }); + + test("a RATE_LIMITED mutation repeats, since GitHub turned it away unprocessed", async () => { + const { github, calls, sleeps } = fixture([ + limited({ "retry-after": "30" }), + json({ data: { addComment: { clientMutationId: null } } }), + ]); + await github.graphql(addComment, { id: "PR_1" }); + expect(calls.map(({ body }) => body?.query)).toEqual([addComment, addComment]); + expect(sleeps).toEqual([30_000]); + }); + + test("a persistent RATE_LIMITED fails loudly, and the deadline cuts its wait", async () => { + const persistent = fixture(Array.from({ length: 7 }, () => limited())); + await expect(persistent.github.graphql("query { viewer { login } }", {})).rejects.toThrow( + "GitHub GraphQL rate limit persisted", + ); + expect(persistent.sleeps).toEqual([60_000, 120_000, 240_000, 480_000, 960_000, 1_920_000]); + + const bounded = fixture([limited()], 10_000); + await expect(bounded.github.graphql("query { viewer { login } }", {})).rejects.toBeInstanceOf(GitHubDeadline); + expect(bounded.sleeps).toEqual([10_000]); + }); + + test("other GraphQL errors fail without waiting", async () => { + const { github, sleeps } = fixture([json({ errors: [{ type: "NOT_FOUND", message: "Not Found" }] })]); + await expect(github.graphql("query { viewer { login } }", {})).rejects.toThrow( + "GitHub GraphQL error: Not Found", + ); + expect(sleeps).toEqual([]); + }); + + test("a mutation whose response body was cut off is confirmed, not sent again", async () => { + const disable = + "mutation($id: ID!) { disablePullRequestAutoMerge(input: { pullRequestId: $id }) { clientMutationId } }"; + const { github, calls } = fixture([cut(), json({ data: { node: { autoMergeRequest: null } } })]); + expect(await github.graphql(disable, { id: "PR_1" })).toEqual({}); + expect(calls.map(({ body }) => body?.query?.split("(")[0])).toEqual(["mutation", "query"]); + }); + + test("a mutation without a confirmation never repeats", async () => { + const { github, calls } = fixture([bad(502)]); + await expect( + github.graphql( + 'mutation($id: ID!) { addComment(input: { subjectId: $id, body: "x" }) { clientMutationId } }', + { + id: "PR_1", + }, + ), + ).rejects.toThrow("GitHub HTTP 502"); + expect(calls).toHaveLength(1); + }); +}); diff --git a/test/land.test.ts b/test/land.test.ts index b30b973e..ef4cde9b 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -1809,7 +1809,7 @@ mv '${result}.new' '${result}' ); }); - test("a lost merge response never re-merges or ejects the merged PR after restart", async () => { + test("a lost merge response is confirmed from the PR, never merged again, and the batch goes on", async () => { const forge = new Forge(); for (let i = 1; i <= 3; i++) forge.pull(`PR ${i}`, { [`${i}.txt`]: "yes" }); const deps = await forge.deps(); @@ -1820,6 +1820,25 @@ mv '${result}.new' '${result}' if (String(input).endsWith("/pulls/1/merge")) throw new Error("lost merge response"); return response; }) as typeof fetch; + const output = await serveBatch(forge.clone(), plan, sandbox(), deps, async () => deps.token); + // A batch of three takes two. + const batch = forge.repo.pulls.slice(0, 2); + expect(output).toEqual(batch.map((pull) => `landed ${pull.number} ${pull.squash}`)); + expect(forge.merges.map(({ number }) => number)).toEqual([1, 2]); + expect(forge.comment(1)).toBeUndefined(); + }); + + test("a merge response lost past every retry never re-merges or ejects the merged PR after restart", async () => { + const forge = new Forge(); + for (let i = 1; i <= 3; i++) forge.pull(`PR ${i}`, { [`${i}.txt`]: "yes" }); + const deps = await forge.deps(); + const plan = await resolveQueue(REPOSITORY, undefined, deps); + const original = deps.fetch; + deps.fetch = (async (input, init) => { + const response = await original(input, init); + if (/\/pulls\/1(\/merge)?$/.test(String(input))) throw new Error("lost merge response"); + return response; + }) as typeof fetch; await expect(serveBatch(forge.clone(), plan, sandbox(), deps, async () => deps.token)).rejects.toThrow( "lost merge response", ); @@ -2664,7 +2683,7 @@ ${requeueScript}`); await fixture.finished; expect(await fixture.collect()).toEqual(["requeued 1"]); }); - test("hands back a job after losing the enable-auto-merge response", async () => { + test("requeues a fixed job after confirming a lost enable-auto-merge response took effect", async () => { const fixture = await fixerFixture(fixedScript()); await fixture.finished; let lost = false; @@ -2680,10 +2699,15 @@ ${requeueScript}`); }) as typeof fetch, }; - expect(await fixture.collect(broken)).toEqual(["handback 1"]); - expect(fixture.pull.landing).toBe(false); - expect(fixture.pull.labels).not.toContain("quest:merge"); - expect(fixture.forge.comment(1)).toContain("lost response"); + expect(await fixture.collect(broken)).toEqual(["requeued 1"]); + expect(fixture.pull.head).not.toBe(fixture.plan.queue[0].head); + expect(fixture.pull.autoMerge?.commitBody).toBe(pin(fixture.pull.head!)); + expect(fixture.pull.labels).toContain("quest:merge"); + // The fix is recorded in `## Review`; a PR that stays queued gets no outcome comment. + expect(fixture.pull.body).toContain( + `mechanical fix by agent / Codex: \`preserve both conflict sides\` (head ${fixture.pull.head})`, + ); + expect(fixture.forge.comment(1)).toBeUndefined(); }); test("preserves a refused-push handback across a failed comment and restart", async () => {