diff --git a/src/cli/land-watch.ts b/src/cli/land-watch.ts index 49585479..b22f6e17 100644 --- a/src/cli/land-watch.ts +++ b/src/cli/land-watch.ts @@ -27,6 +27,7 @@ import { startDaemon, } from "./land-serve"; import { advanceFixes, endPick, type FixContext, readRemoval, requeuedHeads } from "./land-watch-fix"; +import { headBefore } from "./status"; export { readRemoval } from "./land-watch-fix"; @@ -48,7 +49,7 @@ export interface WatchOptions { * until a writer enqueues the PR again: a push since the queue dropped it (`pushed`), a person's * removal (`removed`), or a reason the watcher does not know (`unrecognized`). */ -export type Verdict = "fix" | "pushed" | "removed" | "unrecognized"; +export type Verdict = "fix" | "pushed" | "removed" | "unrecognized" | "unknown"; /** A `merge_group` job that failed, or was still running when its checks timed out. */ export interface FailedJob { @@ -117,7 +118,7 @@ const FIXABLE = ["failed_checks", "checks_timed_out", "merge_conflict"]; const REMOVALS = `pageInfo { hasNextPage endCursor } nodes { ... on RemovedFromMergeQueueEvent { - id createdAt reason actor { login } enqueuer { login } beforeCommit { oid } + id createdAt reason actor { login } enqueuer { login } } }`; @@ -326,8 +327,17 @@ const AFTER: Record string> = { "\n\nA person's removal ends the pick: the watcher leaves this pull request alone until a maintainer enqueues it again.", unrecognized: () => "\n\nThe watcher does not recognize this reason, so it leaves this pull request alone until a maintainer enqueues it again.", + unknown: () => + "\n\nThe watcher cannot tell which head entered the queue, since GitHub has not listed the head branch's pushes or the watcher cannot read them, so it leaves this pull request alone until a maintainer enqueues it again.", }; +/** + * How long a removal whose enqueued head the push activity cannot tell yet waits for GitHub to list + * it (14 s to 6 min observed) before it ends the pick. It stays within `SETTLE`, so every pass until + * then reads the removal again. + */ +const UNLISTED = SETTLE - 2 * 60_000; + /** * Handle one removal: classify it, fetch a failed build's logs, comment on the PR once (a retry * finds the marker and posts nothing), and record it for the fixer. A merge, and the App's own @@ -342,23 +352,35 @@ async function handle( fixing: boolean, pull: ObjectValue, event: ObjectValue, -): Promise { + note: (text: string) => void, + now: number, +): Promise { const reason = string(event.reason).toLowerCase(); const actor = login(event.actor); if (reason === "merged" || account(actor) === app) return undefined; const number = Number(pull.number); const current = string(pull.headRefOid); - const head = event.beforeCommit == null ? current : string(object(event.beforeCommit).oid); const at = string(event.createdAt); - const { pick, queued } = await pickOf(github, string(pull.id), event, head, app, requeuedHeads(state, number)); + const own = requeuedHeads(state, number); + // The dropped head is the PR's head when it entered the queue: the removal's `beforeCommit` is the + // merge group's own commit, never the PR's head. The enqueue time does not depend on the head. + const first = await pickOf(github, string(pull.id), event, current, app, own); + const enqueued = first.queued ?? first.pick.at; + const known = await headBefore(github, pull, Date.parse(enqueued), `the head ${number} was enqueued at`, note); + // An unknown head never authorizes the fixer: wait for GitHub to list the push, then end the pick. + if (known === null && now - Date.parse(at) < UNLISTED) return null; + const head = known ?? current; + const { pick, queued } = head === current ? first : await pickOf(github, string(pull.id), event, head, app, own); const verdict: Verdict = - head !== current - ? "pushed" - : reason === "manual" - ? "removed" - : FIXABLE.includes(reason) - ? "fix" - : "unrecognized"; + known === null + ? "unknown" + : head !== current + ? "pushed" + : reason === "manual" + ? "removed" + : FIXABLE.includes(reason) + ? "fix" + : "unrecognized"; const build = reason === "failed_checks" || reason === "checks_timed_out" ? await failedBuild( @@ -534,7 +556,11 @@ export async function watchPass( fix !== undefined, pull, event, + (text) => output.push(text), + deps.now(), ); + // Not handled yet: a later pass within SETTLE reads it again. + if (line === null) continue; if (line) output.push(line); handled[id] = at; save(path, progress); diff --git a/src/cli/status.ts b/src/cli/status.ts index b5c6cc27..1d7feb7e 100644 --- a/src/cli/status.ts +++ b/src/cli/status.ts @@ -205,23 +205,38 @@ export async function enabledHead( const request = object(pull.autoMergeRequest); const pin = pinned(typeof request.commitBody === "string" ? request.commitBody : ""); if (pin !== null || pull.headRepository == null) return pin; - const enabled = Date.parse(string(request.enabledAt)); + return headBefore(github, pull, Date.parse(string(request.enabledAt)), "the head auto-merge was enabled on", warn); +} + +/** + * The PR's head just before `time`: the last push or merge to its head branch before then, read from + * the head repository's activity once that lists the current head's push. A push in the same second + * is ambiguous. An activity listing the token cannot read leaves it unknown (null), with a warning + * naming `what`, rather than failing the caller. The node needs `headRefOid`, `headRefName`, and + * `headRepository { nameWithOwner }`. + */ +export async function headBefore( + github: GitHub, + pull: ObjectValue, + time: number, + what: string, + warn: (message: string) => void, +): Promise { + if (pull.headRepository == null) return null; const head = string(pull.headRefOid); const pushes: { sha: string; time: number }[] = []; const repository = string(object(pull.headRepository).nameWithOwner); let path: string | undefined = `/repos/${repository}/activity?ref=${encodeURIComponent(string(pull.headRefName))}&per_page=100`; - // Newest first: read until a push before enabling, which is the head auto-merge was enabled on. - while (path && !pushes.some((push) => push.time < enabled)) { + // Newest first: read until a push before `time`, which is the head then. + while (path && !pushes.some((push) => push.time < time)) { let response: Response; try { response = await github.request(path, { allow: [404] }); } catch (error) { // A deadline is the caller's to handle; a forbidden listing, as for a token without contents access, is not. if (error instanceof GitHubDeadline || !(error instanceof Error)) throw error; - warn( - `cannot read ${repository}'s push activity, so the head auto-merge was enabled on is unknown: ${error.message}`, - ); + warn(`cannot read ${repository}'s push activity, so ${what} is unknown: ${error.message}`); return null; } if (response.status === 404) return null; @@ -231,8 +246,8 @@ export async function enabledHead( path = link(response, "next"); } // GitHub lists a push seconds to minutes late; until the current head's is listed, an earlier one may be missing. - if (!pushes.some((push) => push.sha === head) || pushes.some((push) => push.time === enabled)) return null; - return pushes.find((push) => push.time < enabled)?.sha ?? null; + if (!pushes.some((push) => push.sha === head) || pushes.some((push) => push.time === time)) return null; + return pushes.find((push) => push.time < time)?.sha ?? null; } function summarize( diff --git a/test/land-watch.test.ts b/test/land-watch.test.ts index 2e24e1d2..3cb0c5ec 100644 --- a/test/land-watch.test.ts +++ b/test/land-watch.test.ts @@ -24,6 +24,8 @@ const BASE = "b".repeat(40); /** The base the merge group was built on, which its branch names. */ const BUILT = "c".repeat(40); const HEAD = "a".repeat(40); +/** A merge group commit, which a removal names as its `beforeCommit`. */ +const GROUP = "9".repeat(40); const PUSHED = "d".repeat(40); const APP = "moq-bot"; @@ -88,6 +90,8 @@ interface World { refuse?: (body: string) => boolean; /** A read of a PR's queue state that fails, once. */ unreadable?: (pull: Pull) => boolean; + /** PRs whose push activity GitHub has not listed yet. */ + unlisted?: number[]; } const at = (minutes: number) => new Date(Date.UTC(2026, 10, 1, 12, minutes)).toISOString().replace(".000", ""); @@ -112,10 +116,32 @@ function removalNode(event: Removed) { reason: event.reason, actor: { login: event.actor }, enqueuer: event.enqueuer === undefined ? { login: "maintainer" } : { login: event.enqueuer }, - beforeCommit: event.before === null ? null : { oid: event.before ?? HEAD }, + // GitHub's `beforeCommit` is the merge group's commit on the base, never the PR's head. + beforeCommit: event.before === null ? null : { oid: GROUP }, }; } +/** + * The head branch's push activity, newest first, as GitHub's `/activity` lists it. Each removal's + * dropped head (`before`) was pushed a second before the enqueue that preceded the removal; a current + * head that is none of them was pushed after the last removal, as a push that ends the pick is. + */ +function pushes(world: World, pull: Pull) { + const rows: { activity_type: string; after: string; timestamp: string }[] = []; + const stamp = (time: number) => new Date(time).toISOString().replace(".000", ""); + for (const event of pull.removals) { + const enqueued = pull.enqueues.filter((added) => added.createdAt <= event.createdAt).at(-1)?.createdAt; + const time = Date.parse(enqueued ?? event.createdAt) - 1000; + rows.push({ activity_type: "push", after: event.before ?? HEAD, timestamp: stamp(time) }); + } + const current = headOf(world, pull); + if (!rows.some((row) => row.after === current)) { + const last = Math.max(Date.parse(at(-100)), ...pull.removals.map((event) => Date.parse(event.createdAt))); + rows.push({ activity_type: "push", after: current, timestamp: stamp(last + 1000) }); + } + return rows.sort((a, b) => b.timestamp.localeCompare(a.timestamp)); +} + /** Removals since `since`, paged two at a time so a PR's further pages are read by node. */ function removalPage(pull: Pull, since: string, after: unknown) { const all = pull.removals.filter((event) => event.createdAt >= since); @@ -275,6 +301,11 @@ function deps(world: World): GitHubDependencies { } return Response.json(list); } + const activity = /^\/repos\/upstream\/project\/activity$/.exec(path); + if (activity) { + const pull = world.pulls.find((item) => `branch-${item.number}` === url.searchParams.get("ref"))!; + return Response.json(world.unlisted?.includes(pull.number) ? [] : pushes(world, pull)); + } throw new Error(`unexpected request: ${path}`); }) as typeof fetch, }; @@ -493,6 +524,31 @@ test("a push since the removal ends the pick, and merges and the App's own remov expect(github.comments[8][0].body).toContain("does not recognize this reason"); }); +test("a removal whose enqueued head GitHub has not listed waits, then ends the pick unfixed", async () => { + const state = tempDir(); + const github = world( + [ + { + number: 6, + head: HEAD, + enqueues: [{ createdAt: at(1), actor: "maintainer" }], + removals: [{ id: "R1", createdAt: at(5), reason: "failed_checks", actor: "github-merge-queue" }], + }, + ], + [failedRun(6, at(3))], + ); + github.unlisted = [6]; + github.now = Date.parse(at(6)); + // Not handled yet: no comment, no record, and the next pass reads it again. + expect(await watchPass(REPOSITORY, { state }, deps(github))).toEqual([]); + expect(github.comments[6]).toBeUndefined(); + expect(readRemoval(state, 6)).toBeUndefined(); + github.now = Date.parse(at(14)); + expect(await watchPass(REPOSITORY, { state }, deps(github))).toEqual(["removed 6 failed_checks unknown"]); + expect(github.comments[6][0].body).toContain("cannot tell which head entered the queue"); + expect(readRemoval(state, 6)!.failure).toBeUndefined(); +}); + test("a failed build with no failing job found, or a job with no log, still comments", async () => { const state = tempDir(); const run = failedRun(9, at(3));