From c0b4f62cba321d1e1556694fe98f95808e3ee6bd Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 31 Oct 2026 13:54:39 -0700 Subject: [PATCH 1/6] chore(quest): claim quest/m0/github-retry.md Quest-Claim: 68e122f3af183d13 From af1b33f751e3cc0c8b6c4429b7526166b0af2a0b Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 31 Oct 2026 14:08:36 -0700 Subject: [PATCH 2/6] fix(github): retry 5xx and dropped connections, confirming writes first GitHub.request retries a 5xx or a dropped connection up to four times (1, 2, 4, 8 s, or GitHub's Retry-After), then fails with the last error. Reads, PUT, PATCH, DELETE and label adds repeat; a merge, a comment, a new pull request, or a pull request mutation is repeated only after a read-back shows the lost attempt took no effect. Any other POST or mutation never repeats. Co-Authored-By: Claude Opus 5.5 --- README.md | 3 +- cloudflare/dev/github.ts | 6 +- quest/m0/README.md | 1 - quest/m0/github-retry.md | 21 --- src/cli/github.ts | 204 ++++++++++++++++++++++++----- test/github.test.ts | 275 +++++++++++++++++++++++++++++++++++++++ test/land.test.ts | 31 ++++- 7 files changed, 478 insertions(+), 63 deletions(-) delete mode 100644 quest/m0/github-retry.md create mode 100644 test/github.test.ts diff --git a/README.md b/README.md index 14e52204..6c01ea17 100644 --- a/README.md +++ b/README.md @@ -362,7 +362,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 237f80d0..dc48e635 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, @@ -903,8 +906,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 a829f363..ff57fe3f 100644 --- a/quest/m0/README.md +++ b/quest/m0/README.md @@ -45,7 +45,6 @@ ownership-manifest design: - [Accurate head_moved](/quest/m0/land-head-moved.md) - the gate's own base-merge push and batch-mate failures are not reported as a moved head - [Queue-time checks](/quest/m0/land-queue-checks.md) - queued PRs are checked against each other and `quest check` runs per PR before batching - [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 - [Codex cloud off](/quest/m0/codex-cloud-off.md) - automatic Codex cloud reviews stop once no open PR relies on them 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..4dfbb7f3 100644 --- a/src/cli/github.ts +++ b/src/cli/github.ts @@ -35,6 +35,27 @@ 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; + +/** + * 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 +153,24 @@ 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; 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 dropped connection, 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 repeat = options.repeat ?? this.repeatable(url, options); + const started = this.deps.now(); + 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 +187,109 @@ 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); + if (response.ok || response.status === 304 || options.allow?.includes(response.status)) return response; + // A body cut off mid-read still leaves the status to act on. + const body = await response.text().catch(() => ""); + retry = seconds(response.headers.get("retry-after")); + 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. + if (limits >= 6) + throw new Error(`GitHub HTTP ${response.status} for ${url.pathname}; rate limit persisted`); + 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 ** limits : Math.max(1000, reset - this.deps.now())); + limits++; + this.deps.warn(`GitHub rate limited this request; waiting ${Math.ceil(delay / 1000)} seconds`); + await this.pause(delay); + 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"}`, ); - 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 (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; + } } } + /** + * 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, a + * pull request by its branch. Any other POST never repeats. + */ + private repeatable(url: URL, options: RequestOptions): Repeat { + 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 text = body.body; + return async (started) => { + const since = new Date(started - CLOCK_SKEW).toISOString(); + const comments = await this.all(`${url.pathname}?since=${since}&per_page=100`); + const found = comments.find((comment) => comment.body === text); + 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,9 +308,16 @@ 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. + */ async graphql(query: string, variables: ObjectValue): Promise { - const response = await this.request("/graphql", { body: { query, variables } }); + const response = await this.request("/graphql", { + body: { query, variables }, + repeat: this.mutationRepeat(query, variables), + }); const body = object(await response.json()); if (body.errors !== undefined) throw new Error( @@ -222,8 +327,37 @@ export class GitHub { ); return object(body.data); } + + /** Repeat a query; confirm a pull request mutation `PULL_MUTATIONS` knows; never repeat another. */ + 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 read = `query($id: ID!) { node(id: $id) { ... on PullRequest { ${effect.fields} } } }`; + return async () => + effect.landed(object((await this.graphql(read, { id })).node)) ? 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 +479,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..3452757f --- /dev/null +++ b/test/github.test.ts @@ -0,0 +1,275 @@ +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 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 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([ + 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 }); + expect(calls.map(({ method, path }) => `${method} ${path}`)).toEqual([ + `POST ${repo}/issues/1/comments`, + // A minute before the first attempt, for GitHub's clock. + `GET ${repo}/issues/1/comments?since=2026-10-31T11:59:00.000Z&per_page=100`, + ]); + }); + + test("a comment that did not post is posted again", async () => { + const { github, calls } = fixture([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(["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 { 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", async (mutation, node) => { + const { github, calls } = fixture([bad(502), json({ data: { node } })]); + await github.graphql( + `mutation($id: ID!, $sha: GitObjectID!) { ${mutation}(input: { pullRequestId: $id, expectedHeadOid: $sha }) { clientMutationId } }`, + { id: "PR_1", sha: HEAD }, + ); + expect(calls).toHaveLength(2); + }); + + 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 78756abb..3419203f 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -1325,7 +1325,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(); @@ -1336,6 +1336,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", ); @@ -1925,7 +1944,7 @@ ${requeueScript} ); await fix(dir, log, [1], agent, ["-"]); }); - test("hands back a job after losing the enable-auto-merge response", async () => { + test("requeues a job after confirming a lost enable-auto-merge response took effect", async () => { const fixture = await fixerFixture(requeueScript); await fixture.finished; let lost = false; @@ -1941,10 +1960,10 @@ ${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.autoMerge?.commitBody).toBe(pin(fixture.pull.head!)); + expect(fixture.pull.labels).toContain("quest:merge"); + expect(fixture.forge.comment(1)).toContain("Requeued unchanged:"); }); test("preserves a refused-push handback across a failed comment and restart", async () => { From 4ff38d4a85eeed43b01fc5274ad3e039834f6178 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 31 Oct 2026 14:13:06 -0700 Subject: [PATCH 3/6] fix(github): confirm a pinned mutation only at its expected head Co-Authored-By: Claude Opus 5.5 --- src/cli/github.ts | 17 +++++++++++++---- test/github.test.ts | 20 ++++++++++++-------- 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/src/cli/github.ts b/src/cli/github.ts index 4dfbb7f3..e81823a5 100644 --- a/src/cli/github.ts +++ b/src/cli/github.ts @@ -328,16 +328,25 @@ export class GitHub { return object(body.data); } - /** Repeat a query; confirm a pull request mutation `PULL_MUTATIONS` knows; never repeat another. */ + /** + * 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 read = `query($id: ID!) { node(id: $id) { ... on PullRequest { ${effect.fields} } } }`; - return async () => - effect.landed(object((await this.graphql(read, { id })).node)) ? Response.json({ data: {} }) : undefined; + 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; + }; } } diff --git a/test/github.test.ts b/test/github.test.ts index 3452757f..65f379a9 100644 --- a/test/github.test.ts +++ b/test/github.test.ts @@ -232,7 +232,7 @@ describe("GraphQL", () => { expect(await github.graphql(disable, { id: "PR_1" })).toEqual({}); expect(calls).toHaveLength(2); expect(calls[1].body?.query).toContain( - "node(id: $id) { ... on PullRequest { autoMergeRequest { enabledAt } } }", + "node(id: $id) { ... on PullRequest { headRefOid autoMergeRequest { enabledAt } } }", ); expect(calls[1].body?.variables).toEqual({ id: "PR_1" }); }); @@ -251,13 +251,17 @@ describe("GraphQL", () => { ["mergePullRequest", { merged: true }], ["enablePullRequestAutoMerge", { merged: false, autoMergeRequest: null, mergeQueueEntry: { state: "QUEUED" } }], ["markPullRequestReadyForReview", { isDraft: false }], - ])("%s is confirmed from the pull request", async (mutation, node) => { - const { github, calls } = fixture([bad(502), json({ data: { node } })]); - await github.graphql( - `mutation($id: ID!, $sha: GitObjectID!) { ${mutation}(input: { pullRequestId: $id, expectedHeadOid: $sha }) { clientMutationId } }`, - { id: "PR_1", sha: HEAD }, - ); - expect(calls).toHaveLength(2); + ])("%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); }); test("a mutation without a confirmation never repeats", async () => { From d6048b36e7ecdc7718d61ce9b81bb94517c8bdb4 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 31 Oct 2026 14:23:32 -0700 Subject: [PATCH 4/6] fix(github): wait out GraphQL RATE_LIMITED and retry a body cut off mid-read request reads a response's body before returning it, so a connection that drops mid-body goes through the same repeat and confirm rules as one that drops before the response. graphql waits out a 200 whose errors say RATE_LIMITED like a REST rate limit, within the deadline. Co-Authored-By: Claude Opus 5.5 --- src/cli/github.ts | 125 +++++++++++++++++++++++++++----------------- test/github.test.ts | 103 ++++++++++++++++++++++++++++++++++++ 2 files changed, 179 insertions(+), 49 deletions(-) diff --git a/src/cli/github.ts b/src/cli/github.ts index e81823a5..089445d6 100644 --- a/src/cli/github.ts +++ b/src/cli/github.ts @@ -39,6 +39,8 @@ export type ObjectValue = Record; 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 @@ -153,10 +155,11 @@ export class GitHub { } /** - * A 2xx or 304 response; 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 dropped connection, 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. + * 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: RequestOptions = {}): Promise { // Follow only same-origin pagination links; never send a token to another host. @@ -192,39 +195,49 @@ export class GitHub { let retry: number | undefined; if (response) { 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; - // A body cut off mid-read still leaves the status to act on. - const body = await response.text().catch(() => ""); - retry = seconds(response.headers.get("retry-after")); - 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. - if (limits >= 6) - throw new Error(`GitHub HTTP ${response.status} for ${url.pathname}; rate limit persisted`); - 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 ** limits : Math.max(1000, reset - this.deps.now())); - limits++; - this.deps.warn(`GitHub rate limited this request; waiting ${Math.ceil(delay / 1000)} seconds`); - await this.pause(delay); - 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"}`, + // 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 }), ); + 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; @@ -238,6 +251,20 @@ export class GitHub { } } + /** + * 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 @@ -311,21 +338,21 @@ export class GitHub { /** * 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. + * 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 }, - repeat: this.mutationRepeat(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"); + } } /** diff --git a/test/github.test.ts b/test/github.test.ts index 65f379a9..7ae8915d 100644 --- a/test/github.test.ts +++ b/test/github.test.ts @@ -22,6 +22,16 @@ function json(body: unknown, status = 200, headers: Record = {}) 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) { @@ -85,6 +95,32 @@ describe("transient failures", () => { 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`); @@ -170,6 +206,19 @@ describe("confirmed writes", () => { ]); }); + test("a comment whose response body was cut off is confirmed, not posted again", async () => { + const { github, calls } = fixture([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(["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([dropped(), json([{ id: 1, body: "older" }]), json({ id: 2 }, 201)]); await github.request(`${repo}/issues/1/comments`, { body: { body: text } }); @@ -264,6 +313,60 @@ describe("GraphQL", () => { 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( From 752dfcc442343210b5579fc227840cc075f5025f Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sun, 1 Nov 2026 13:19:35 -0800 Subject: [PATCH 5/6] fix(github): take a lost comment only from after its request, on GitHub's clock An identical comment posted within the minute before, as when a requeued PR is ejected again for the same reason, passed for the lost one, so the new comment was never posted. Measure GitHub's clock from each response's Date header and accept only comments created since the request started, to its whole seconds; keep the minute's slack until a response has measured it. Co-Authored-By: Claude Opus 5.5 --- cloudflare/dev/github.ts | 6 ++++-- src/cli/github.ts | 32 +++++++++++++++++++++++++----- test/github.test.ts | 43 ++++++++++++++++++++++++++++++++-------- 3 files changed, 66 insertions(+), 15 deletions(-) diff --git a/cloudflare/dev/github.ts b/cloudflare/dev/github.ts index 8ad7c117..d2dfc194 100644 --- a/cloudflare/dev/github.ts +++ b/cloudflare/dev/github.ts @@ -934,7 +934,9 @@ export function fakeGitHub(world: World) { if (comment && method === "GET") { const item = [...repo.pulls, ...repo.issues].find((i) => String(i.number) === comment[1]); if (!item) return notFound(); - return json((item.comments ?? []).map((c) => ({ body: c.body, user: { login: c.author } }))); + return json( + (item.comments ?? []).map((c) => ({ body: c.body, user: { login: c.author }, created_at: c.at })), + ); } if (comment && method === "POST") { if (!writes("issues") && !writes("pull_requests")) return forbidden(); @@ -946,7 +948,7 @@ export function fakeGitHub(world: World) { at: new Date().toISOString(), }; item.comments = [...(item.comments ?? []), added]; - return json({ body: added.body, user: { login: added.author } }, 201); + return json({ body: added.body, user: { login: added.author }, created_at: added.at }, 201); } const labels = tail.match(/^\/issues\/(\d+)\/labels(?:\/([^/]+))?$/); if (labels && (method === "POST" || method === "DELETE")) { diff --git a/src/cli/github.ts b/src/cli/github.ts index 089445d6..49a46e51 100644 --- a/src/cli/github.ts +++ b/src/cli/github.ts @@ -37,8 +37,13 @@ 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. */ +/** + * How far GitHub's clock may run behind ours when looking for what a lost request created, before + * any response has said what GitHub's clock reads. + */ const CLOCK_SKEW = 60_000; +/** The slack on GitHub's measured clock: its Date header and created_at are both whole seconds. */ +const DATE_SLACK = 2000; /** Statuses whose response has no body. */ const NULL_BODY = [204, 205, 304]; @@ -140,6 +145,8 @@ export function gitHubDependencies(warn: (message: string) => void): GitHubDepen export class GitHub { /** The longest X-Poll-Interval GitHub asked for since the caller last set this, in milliseconds. */ pollInterval = 0; + /** GitHub's clock less ours, from the latest response's Date header; it can only read early, by its latency. */ + private skew: number | undefined; constructor( private readonly deps: GitHubDependencies, @@ -195,6 +202,8 @@ export class GitHub { let retry: number | undefined; if (response) { this.pollInterval = Math.max(this.pollInterval, seconds(response.headers.get("x-poll-interval")) ?? 0); + const date = Date.parse(response.headers.get("date") ?? ""); + if (Number.isFinite(date)) this.skew = date - this.deps.now(); // 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 }), @@ -294,9 +303,12 @@ export class GitHub { if (issue?.[1] === "comments" && typeof body.body === "string") { const text = body.body; return async (started) => { - const since = new Date(started - CLOCK_SKEW).toISOString(); - const comments = await this.all(`${url.pathname}?since=${since}&per_page=100`); - const found = comments.find((comment) => comment.body === text); + const since = this.createdSince(started); + // `since` filters by the last update; an identical comment created before this request is not ours. + const comments = await this.all(`${url.pathname}?since=${new Date(since).toISOString()}&per_page=100`); + const found = comments.find( + (comment) => comment.body === text && Date.parse(String(comment.created_at)) >= since, + ); return found && Response.json(found, { status: 201 }); }; } @@ -309,7 +321,8 @@ export class GitHub { ); const found = open.rows.find( (pull) => - typeof pull.created_at === "string" && Date.parse(pull.created_at) >= started - CLOCK_SKEW, + typeof pull.created_at === "string" && + Date.parse(pull.created_at) >= this.createdSince(started), ); return found && Response.json(found, { status: 201 }); }; @@ -317,6 +330,15 @@ export class GitHub { return false; } + /** + * The earliest created_at, on GitHub's clock, of anything a request we started at `started` created: + * within seconds of it once a response has measured GitHub's clock, so an identical comment + * posted a moment earlier, as when a requeued PR is ejected again, is never taken for this one. + */ + private createdSince(started: number): number { + return this.skew === undefined ? started - CLOCK_SKEW : started + this.skew - DATE_SLACK; + } + /** 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); diff --git a/test/github.test.ts b/test/github.test.ts index 7ae8915d..ffcd8597 100644 --- a/test/github.test.ts +++ b/test/github.test.ts @@ -190,26 +190,53 @@ 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 posted = { id: 2, body: text, created_at: "2026-10-31T12:00:00Z" }; const { github, calls } = fixture([ bad(502), - json([ - { id: 1, body: "older" }, - { id: 2, body: text }, - ]), + json([{ id: 1, body: "older", created_at: "2026-10-31T11:59:30Z" }, posted]), ]); const response = await github.request(`${repo}/issues/1/comments`, { body: { body: text } }); - expect(await response.json()).toEqual({ id: 2, body: text }); + expect(await response.json()).toEqual(posted); expect(calls.map(({ method, path }) => `${method} ${path}`)).toEqual([ `POST ${repo}/issues/1/comments`, - // A minute before the first attempt, for GitHub's clock. + // A minute before the first attempt, since no response has said what GitHub's clock reads. `GET ${repo}/issues/1/comments?since=2026-10-31T11:59:00.000Z&per_page=100`, ]); }); + test("an identical comment from before a lost one, as when a PR is ejected again, is posted again", async () => { + const date = { date: "Sat, 31 Oct 2026 12:00:00 GMT" }; + const earlier = { id: 1, body: text, created_at: "2026-10-31T11:59:30Z" }; + const { github, calls } = fixture([json({}, 200, date), dropped(), json([earlier]), json({ id: 2 }, 201)]); + await github.request(`${repo}/pulls/1`); + await github.request(`${repo}/issues/1/comments`, { body: { body: text } }); + expect(calls.map(({ method, path }) => `${method} ${path}`)).toEqual([ + `GET ${repo}/pulls/1`, + `POST ${repo}/issues/1/comments`, + // GitHub's clock, measured from the last response, narrows the window to its whole seconds. + `GET ${repo}/issues/1/comments?since=2026-10-31T11:59:58.000Z&per_page=100`, + `POST ${repo}/issues/1/comments`, + ]); + }); + + test("a lost comment is found on GitHub's clock when it runs behind ours", async () => { + const posted = { id: 2, body: text, created_at: "2026-10-31T11:55:00Z" }; + const { github, calls } = fixture([ + json({}, 200, { date: "Sat, 31 Oct 2026 11:55:00 GMT" }), + bad(502), + json([posted]), + ]); + await github.request(`${repo}/pulls/1`); + const response = await github.request(`${repo}/issues/1/comments`, { body: { body: text } }); + expect(await response.json()).toEqual(posted); + expect(calls[2].path).toBe(`${repo}/issues/1/comments?since=2026-10-31T11:54:58.000Z&per_page=100`); + }); + test("a comment whose response body was cut off is confirmed, not posted again", async () => { - const { github, calls } = fixture([cut(201), json([{ id: 2, body: text }])]); + const posted = { id: 2, body: text, created_at: "2026-10-31T12:00:00Z" }; + const { github, calls } = fixture([cut(201), json([posted])]); const response = await github.request(`${repo}/issues/1/comments`, { body: { body: text } }); - expect(await response.json()).toEqual({ id: 2, body: text }); + expect(await response.json()).toEqual(posted); expect(calls.map(({ method }) => method)).toEqual(["POST", "GET"]); }); From c253a944c078cd41a484133d57693a1d0d4b9216 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sun, 1 Nov 2026 13:23:25 -0800 Subject: [PATCH 6/6] fix(github): confirm a lost comment only among those posted after it started Snapshot the identical comments already on the issue before posting, so a requeued PR's earlier ejection comment is never taken for a lost one, however soon it came. This replaces the measured clock window, which still let one through within its slack. Co-Authored-By: Claude Opus 5.5 --- cloudflare/dev/github.ts | 6 ++--- src/cli/github.ts | 46 ++++++++++--------------------- test/github.test.ts | 58 ++++++++++++++++------------------------ 3 files changed, 39 insertions(+), 71 deletions(-) diff --git a/cloudflare/dev/github.ts b/cloudflare/dev/github.ts index d2dfc194..8ad7c117 100644 --- a/cloudflare/dev/github.ts +++ b/cloudflare/dev/github.ts @@ -934,9 +934,7 @@ export function fakeGitHub(world: World) { if (comment && method === "GET") { const item = [...repo.pulls, ...repo.issues].find((i) => String(i.number) === comment[1]); if (!item) return notFound(); - return json( - (item.comments ?? []).map((c) => ({ body: c.body, user: { login: c.author }, created_at: c.at })), - ); + return json((item.comments ?? []).map((c) => ({ body: c.body, user: { login: c.author } }))); } if (comment && method === "POST") { if (!writes("issues") && !writes("pull_requests")) return forbidden(); @@ -948,7 +946,7 @@ export function fakeGitHub(world: World) { at: new Date().toISOString(), }; item.comments = [...(item.comments ?? []), added]; - return json({ body: added.body, user: { login: added.author }, created_at: added.at }, 201); + return json({ body: added.body, user: { login: added.author } }, 201); } const labels = tail.match(/^\/issues\/(\d+)\/labels(?:\/([^/]+))?$/); if (labels && (method === "POST" || method === "DELETE")) { diff --git a/src/cli/github.ts b/src/cli/github.ts index 49a46e51..dfe18754 100644 --- a/src/cli/github.ts +++ b/src/cli/github.ts @@ -37,13 +37,8 @@ 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, before - * any response has said what GitHub's clock reads. - */ +/** How far GitHub's clock may run behind ours when looking for what a lost request created. */ const CLOCK_SKEW = 60_000; -/** The slack on GitHub's measured clock: its Date header and created_at are both whole seconds. */ -const DATE_SLACK = 2000; /** Statuses whose response has no body. */ const NULL_BODY = [204, 205, 304]; @@ -145,8 +140,6 @@ export function gitHubDependencies(warn: (message: string) => void): GitHubDepen export class GitHub { /** The longest X-Poll-Interval GitHub asked for since the caller last set this, in milliseconds. */ pollInterval = 0; - /** GitHub's clock less ours, from the latest response's Date header; it can only read early, by its latency. */ - private skew: number | undefined; constructor( private readonly deps: GitHubDependencies, @@ -172,8 +165,8 @@ export class GitHub { // 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"); - const repeat = options.repeat ?? this.repeatable(url, options); const started = this.deps.now(); + const repeat = options.repeat ?? (await this.repeatable(url, options, started)); let limits = 0; let failures = 0; for (;;) { @@ -202,8 +195,6 @@ export class GitHub { let retry: number | undefined; if (response) { this.pollInterval = Math.max(this.pollInterval, seconds(response.headers.get("x-poll-interval")) ?? 0); - const date = Date.parse(response.headers.get("date") ?? ""); - if (Number.isFinite(date)) this.skew = date - this.deps.now(); // 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 }), @@ -277,10 +268,10 @@ export class GitHub { /** * 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, a - * pull request by its branch. Any other POST never repeats. + * 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 repeatable(url: URL, options: RequestOptions): Repeat { + 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); @@ -301,14 +292,15 @@ export class GitHub { 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 text = body.body; - return async (started) => { - const since = this.createdSince(started); - // `since` filters by the last update; an identical comment created before this request is not ours. - const comments = await this.all(`${url.pathname}?since=${new Date(since).toISOString()}&per_page=100`); - const found = comments.find( - (comment) => comment.body === text && Date.parse(String(comment.created_at)) >= since, + 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 }); }; } @@ -321,8 +313,7 @@ export class GitHub { ); const found = open.rows.find( (pull) => - typeof pull.created_at === "string" && - Date.parse(pull.created_at) >= this.createdSince(started), + typeof pull.created_at === "string" && Date.parse(pull.created_at) >= started - CLOCK_SKEW, ); return found && Response.json(found, { status: 201 }); }; @@ -330,15 +321,6 @@ export class GitHub { return false; } - /** - * The earliest created_at, on GitHub's clock, of anything a request we started at `started` created: - * within seconds of it once a response has measured GitHub's clock, so an identical comment - * posted a moment earlier, as when a requeued PR is ejected again, is never taken for this one. - */ - private createdSince(started: number): number { - return this.skew === undefined ? started - CLOCK_SKEW : started + this.skew - DATE_SLACK; - } - /** 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); diff --git a/test/github.test.ts b/test/github.test.ts index ffcd8597..11a1c9cd 100644 --- a/test/github.test.ts +++ b/test/github.test.ts @@ -190,54 +190,37 @@ 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 posted = { id: 2, body: text, created_at: "2026-10-31T12:00:00Z" }; const { github, calls } = fixture([ + json([{ id: 1, body: "older" }]), bad(502), - json([{ id: 1, body: "older", created_at: "2026-10-31T11:59:30Z" }, posted]), + 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(posted); + 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`, - // A minute before the first attempt, since no response has said what GitHub's clock reads. - `GET ${repo}/issues/1/comments?since=2026-10-31T11:59:00.000Z&per_page=100`, + listing, ]); }); test("an identical comment from before a lost one, as when a PR is ejected again, is posted again", async () => { - const date = { date: "Sat, 31 Oct 2026 12:00:00 GMT" }; - const earlier = { id: 1, body: text, created_at: "2026-10-31T11:59:30Z" }; - const { github, calls } = fixture([json({}, 200, date), dropped(), json([earlier]), json({ id: 2 }, 201)]); - await github.request(`${repo}/pulls/1`); + 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, path }) => `${method} ${path}`)).toEqual([ - `GET ${repo}/pulls/1`, - `POST ${repo}/issues/1/comments`, - // GitHub's clock, measured from the last response, narrows the window to its whole seconds. - `GET ${repo}/issues/1/comments?since=2026-10-31T11:59:58.000Z&per_page=100`, - `POST ${repo}/issues/1/comments`, - ]); - }); - - test("a lost comment is found on GitHub's clock when it runs behind ours", async () => { - const posted = { id: 2, body: text, created_at: "2026-10-31T11:55:00Z" }; - const { github, calls } = fixture([ - json({}, 200, { date: "Sat, 31 Oct 2026 11:55:00 GMT" }), - bad(502), - json([posted]), - ]); - await github.request(`${repo}/pulls/1`); - const response = await github.request(`${repo}/issues/1/comments`, { body: { body: text } }); - expect(await response.json()).toEqual(posted); - expect(calls[2].path).toBe(`${repo}/issues/1/comments?since=2026-10-31T11:54:58.000Z&per_page=100`); + 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 posted = { id: 2, body: text, created_at: "2026-10-31T12:00:00Z" }; - const { github, calls } = fixture([cut(201), json([posted])]); + 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(posted); - expect(calls.map(({ method }) => method)).toEqual(["POST", "GET"]); + 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 () => { @@ -247,9 +230,14 @@ describe("confirmed writes", () => { }); test("a comment that did not post is posted again", async () => { - const { github, calls } = fixture([dropped(), json([{ id: 1, body: "older" }]), json({ id: 2 }, 201)]); + 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(["POST", "GET", "POST"]); + expect(calls.map(({ method }) => method)).toEqual(["GET", "POST", "GET", "POST"]); }); test("a pull request whose response was lost is found by its branch", async () => {