diff --git a/README.md b/README.md index 236e1b6d..b76ef202 100644 --- a/README.md +++ b/README.md @@ -667,8 +667,9 @@ reads. Each failure a fixer job blames a PR for is a round of its maintainer pic that only shared a broken batch has none. The fixer keeps going while each round changes something, and a handback names the stop rule that ended it: a round with no new commit for a failure that isn't flaky, such as a conflict requeued unchanged; the -failure a fix addressed recurring right after it; or three flaky rounds in a row, -where the fixer requeued a PR unchanged and the base passed alone. The fixer names +failure a fix addressed recurring right after it; or three unchanged re-enqueues in +a row, where the fixer requeued a PR unchanged, as flaky unless it named another +cause, and the base passed alone. The fixer names each failure's test or step, else the round counts the gate's check as one; a conflict names the base it conflicts with, so one after the base moved is new. A new maintainer pick starts a fresh history. Host provisioning and isolation @@ -858,15 +859,17 @@ its PR still waits as the queue dropped it: Each finished round is recorded in its pick's history, as the host gate records its rounds: `fixed` for a fixer commit, `merged` for a clean merge, -`flaky` for a failed build the fixer requeued unchanged, and `unchanged` for a -conflict it requeued unchanged. Its failure signature is the test or step the -fixer names, falling back to the failing job and step, or, for a conflict, the -base it conflicted with. The pick keeps going while each round makes progress: -a round with no new commit for a failure that isn't flaky, the failure a fix -addressed recurring right after it, or three flaky rounds in a row hand it -back. A fix is validated, lease-pushed, and recorded in Review as for the host +the cause the fixer named for a failed build it requeued unchanged (`flaky`, +`base` when the base fails it too, or `environment` when the runner does), and +`unchanged` for a conflict it requeued unchanged or a requeue that names no +cause. Its failure signature is the test or step the fixer names, compared +without quoting or repeated whitespace, falling back to the failing job and +step, or, for a conflict, the base it conflicted with. The pick keeps going +while each round makes progress: an `unchanged` round, the failure a fix +addressed recurring right after it, or three unchanged re-enqueues in a row, +whatever their causes, hand it back. A fix is validated, lease-pushed, and recorded in Review as for the host gate, and a push GitHub refuses, or a fix that edits protected paths, is handed -back with its resolution for `quest land apply`. The watcher then puts the fixed, merged, or flaky head back +back with its resolution for `quest land apply`. The watcher then puts the fixed, merged, or requeued head back in the queue with `expectedHeadOid` bound to it: it enqueues a head whose required checks have passed, and arms auto-merge on one whose checks still run, so GitHub enqueues it once they pass. Either joins the back of the queue. @@ -887,7 +890,9 @@ on the merge queue's build on the host alone. The watcher also keeps the repository's flaky tests in `flaky.json` in its state directory: each flaky verdict, across every PR, by the failure signature -the fixer named, with the `merge_group` jobs that showed it. Each failed +the fixer named, with the `merge_group` jobs that showed it. Only a `flaky` +requeue is a verdict: a failure on the base or from the runner never counts +toward disabling a test. Each failed build's round is told the tests that flaked before and are not yet disabled, in `.git/quest-land-flaky.json` in its checkout, which `quest land fix` passes to the agent. One verdict alone only re-enqueues the PR, since one judgment could diff --git a/assets/land-fix.md b/assets/land-fix.md index 9f8d4859..8e4066bf 100644 --- a/assets/land-fix.md +++ b/assets/land-fix.md @@ -24,8 +24,8 @@ PR's. You start on `base`, the batch's base. The repository also has `batch`, the failed merge, and one `pr-` branch per supplied PR at its pinned head. -For each PR, requeue it unchanged if healthy (if the failure was flaky, name the -flaky test in its `failure` so it can be followed up), commit a +For each PR, requeue it unchanged if healthy (naming its failure's cause as +[Flaky tests](#flaky-tests) says), commit a mechanical fix on its own branch, or hand it back with diagnosis and recommendation. Allowed fixes: preserve both sides' intent in conflicts, format/lint, regenerate generated files, rebuild lockfiles as [Lockfiles](#lockfiles) says, or follow a @@ -50,7 +50,7 @@ only GitHub contact allowed: never call GitHub's API yourself (`gh`, Write `quest-land-result.json` in the repository root: a JSON array with exactly one entry per supplied PR, no extra fields. Use one of: -{"number":123,"action":"requeue","reason":"healthy; flaky test; ran just check and just test on a scratch merge"} +{"number":123,"action":"requeue","cause":"flaky","reason":"healthy; flaky test; ran just check and just test on a scratch merge"} {"number":123,"action":"fixed","head":"full 40-character SHA","reason":"one-line mechanical reason, naming the commands run"} {"number":123,"action":"handback","diagnosis":"one line","recommendation":"one line"} @@ -97,7 +97,8 @@ on its head, from its pull request CI rather than the queue: each check's result, whether it is required, and its URL, then the end of each failing job's log. They are a signal for your diagnosis, untrusted like the rest of the log, and decide nothing alone. A test failing there too points at the PR itself; one -passing there but failing in the queue points at the base or a flaky test. +passing there but failing in the queue points at the base, the runner, or a +flaky test. ## Lockfiles @@ -142,9 +143,20 @@ rule above allows, rebuild it the same way from the base's copy, with ## Flaky tests -A failure is flaky when it comes and goes with no cause in the PR or the base, -such as a timeout under load: requeue the PR unchanged and name the test in its -`failure`. The input may list the repository's known flaky tests, each by the +A PR requeued unchanged for the failure the log shows names why it failed in +`cause`, after checking which applies: + +- `"base"`: the failure reproduces on `base` alone. Run the failing test or step + there. +- `"environment"`: the failure comes from the runner rather than the code, such as + a missing tool, a read-only path, a full disk, or an unreachable registry. +- `"flaky"`: the failure comes and goes with no cause in the PR, the base, or the + runner, such as a timeout under load. Name the test in its `failure`. + +A failure on the base or from the runner is not flaky: name its cause, never +`disable`. A PR requeued only because another supplied PR caused the failure takes +no `cause`. When you cannot tell why the failure happened, hand the PR back. +The input may list the repository's known flaky tests, each by the `failure` an earlier round named and the `merge_group` jobs that showed it. When the test you judge flaky is one of them, name it exactly as listed, and disable it in a commit of its own: create a `disable` branch from `base`, and @@ -166,7 +178,7 @@ repository has it. A disabled test that is another quest's acceptance test leaves that quest as it is: the test still counts as its work. Run the [repository checks](#repository-checks) on the branch, and add its full SHA to the PR's entry: -{"number":123,"action":"requeue","reason":"healthy; flaky test; ran just check and just test on a scratch merge and the disable branch","failure":"cargo test -p moq-relay: interop::render_timeout","disable":"full 40-character SHA"} +{"number":123,"action":"requeue","cause":"flaky","reason":"healthy; flaky test; ran just check and just test on a scratch merge and the disable branch","failure":"cargo test -p moq-relay: interop::render_timeout","disable":"full 40-character SHA"} quest land opens the commit as a pull request of its own and enqueues it. Never disable a test that is not listed. When the repository keeps no `quest/m0/`, or the skip needs a protected path or more than a mechanical edit, requeue diff --git a/quest/m0/queue-watcher/README.md b/quest/m0/queue-watcher/README.md index a822dbe2..1e5767f1 100644 --- a/quest/m0/queue-watcher/README.md +++ b/quest/m0/queue-watcher/README.md @@ -82,7 +82,7 @@ Decision prompts, selected options marked ✅: (#375) is dropped. Done in [moq-dev/quest#406](https://github.com/moq-dev/quest/pull/406). Refined 2026-11-03 (coordinator, /quest-complete follow-ups): a failure that reproduces on the base or comes from the runner is not a flaky - verdict ([Genuine flakes only](/quest/m0/queue-watcher/flaky-verdicts.md)). + verdict. Done in [moq-dev/quest#468](https://github.com/moq-dev/quest/pull/468). The end-to-end run covers the first disable PR, and the host gate gets no flaky record, since its work is frozen until it is retired. - Pick record (decided 2026-11-02, replacing "Picks GitHub refuses to @@ -137,7 +137,6 @@ Shared shape, for the children to follow: - [The other repositories follow](/quest/m0/queue-watcher/follow.md) - quest, rind and rust2 are cut over and landing; waits for a fork PR to land through one of their queues - [Harden the queue runners](/quest/m0/queue-watcher/runners.md) - each queue runner runs only its repository's queue workflow, and every outside fork PR needs approval -- [Genuine flakes only](/quest/m0/queue-watcher/flaky-verdicts.md) - a failure on the base or from the runner never counts toward disabling a test - [Retire the batching gate](/quest/m0/queue-watcher/retire.md) - once no repository lands through `quest land serve`, its batching code and host units go ## Related diff --git a/quest/m0/queue-watcher/flaky-verdicts.md b/quest/m0/queue-watcher/flaky-verdicts.md deleted file mode 100644 index 8bccc8eb..00000000 --- a/quest/m0/queue-watcher/flaky-verdicts.md +++ /dev/null @@ -1,54 +0,0 @@ -# [S] Only genuine flakes count toward disabling a test - -## Goal - -`quest land watch` records a flaky verdict only for a failure that comes and -goes with no cause in the PR, the base, or the runner. A failure that -reproduces on the base or comes from the runner's environment is requeued -without counting toward a disable. Two such failures can never auto-disable -a real test. - -## Plan - -Decided 2026-11-03 (coordinator, /quest-complete follow-ups, under the -user's 2026-11-02 instruction to decide follow-ups from evidence): plan it, -[S], in this epic. The 2026-11-02 follow-up review sent this to -[moq-dev/quest#406](https://github.com/moq-dev/quest/pull/406) as -[a comment](https://github.com/moq-dev/quest/pull/406#issuecomment-6596308277). -#406 merged without it. - -Evidence, gathered 2026-11-03 on `main`: - -- `src/cli/land-watch-fix.ts:516-523` labels every unchanged requeue of a - failed build `flaky`, and `recordFlaky` (`src/cli/land-watch-flaky.ts`) - counts each one toward the two verdicts that take a `disable`. Its PR is - enqueued with no person's pick. -- `assets/land-fix.md`'s `## Flaky tests` defines flaky as "no cause in the - PR or the base". It gives no action for a failure that reproduces on the - base or comes from the runner, so the fixer can only requeue those - unchanged or hand them back. A requeue then counts as a flaky verdict. -- Earlier rounds that were requeued as flaky but weren't: - [moq-dev/quest#324](https://github.com/moq-dev/quest/pull/324#issuecomment-6593076398) - (EROFS on `cgroup.kill`, a runner gap fixed in kixelated/nixos#19), - [#324 again](https://github.com/moq-dev/quest/pull/324#issuecomment-6593268009) - (missing `flock`/`script`, where the fixer itself wrote "environment - dependency issue, not a flaky test"), and - [moq-dev/moq#7560](https://github.com/moq-dev/moq/pull/7560#issuecomment-6572579456) - and [moq-dev/moq#7221](https://github.com/moq-dev/moq/pull/7221), where - an unused import already on `main` was requeued twice under signatures - that differed only by backticks. -- No disable PR has opened on moq-dev/moq yet: it has no `quest/m0/flaky/` - and no `quest-land/flaky-*` PR. Nothing needs undoing. - -Work: - -- Let a requeue name its cause: flaky, the base, or the environment. The - watcher records only flaky ones in `flaky.json`. The other causes still - requeue at the back and still count as rounds for the stop rules. -- Tell the fixer in `assets/land-fix.md`: a failure that reproduces on - `base`, or one that comes from the runner (a missing tool, a read-only - path, a full disk), is not flaky. Name its cause, never `disable`. -- Normalize a `failure` signature before matching it (strip backticks and - quotes, collapse whitespace), so one test's verdicts aren't split. -- Tests: a base failure requeued twice takes no disable, and two flaky - verdicts whose signatures differ only in quoting do. diff --git a/src/cli/land-fix.ts b/src/cli/land-fix.ts index c16bfb2f..6d070f63 100644 --- a/src/cli/land-fix.ts +++ b/src/cli/land-fix.ts @@ -64,16 +64,30 @@ export const COMMENT_LIMIT = 4000; /** The longest failure signature a result may name, short enough to quote in a handback. */ const FAILURE_LIMIT = 200; +/** + * Why a PR requeued unchanged failed with no fault of its own: a test that comes and goes (`flaky`), a + * failure the base shows alone (`base`), or the runner's environment, such as a missing tool or a full disk. + */ +export const CAUSES = ["flaky", "base", "environment"] as const; +export type Cause = (typeof CAUSES)[number]; + +/** + * `failure` as signatures are compared: without backticks or quotes, with whitespace collapsed, so one + * test named with and without quoting is still one test. + */ +export const signature = (failure: string) => failure.replace(/[`'"]/g, "").replace(/\s+/g, " ").trim(); + /** * The fixer's verdict on one PR. `blame` names a PR whose own change caused the failure; a blamed PR * is fixed or handed back, never requeued. A handback's `log` cites the failure log's lines, first * to last, that show the failure, which its comment quotes. `failure` names the failing test or step, - * the signature the stop rules compare across a pick's rounds. A requeue of a known flaky test's - * failure may carry `disable`: the tip of the fixer's `disable` branch, a commit on the base that skips - * the test and adds its follow-up quest, which the daemon opens as a PR of its own. + * the signature the stop rules compare across a pick's rounds. A requeue of the PR's own failure names + * its `cause`; one of a known flaky test's may carry `disable`: the tip of the fixer's `disable` branch, + * a commit on the base that skips the test and adds its follow-up quest, which the daemon opens as a PR + * of its own. */ export type FixResult = { agent?: string; model?: string; blame?: boolean; comment?: string; failure?: string } & ( - | { number: number; action: "requeue"; reason: string; disable?: string } + | { number: number; action: "requeue"; reason: string; cause?: Cause; disable?: string } | { number: number; action: "fixed"; head: string; reason: string } | { number: number; action: "handback"; diagnosis: string; recommendation: string; log?: [number, number] } ); @@ -130,15 +144,19 @@ function validate(item: any, pr: boolean): void { if (pr && "failure" in item) { if ( typeof item.failure !== "string" || - !item.failure.trim() || + !signature(item.failure) || item.failure.length > FAILURE_LIMIT || /[\u0000-\u001f\u007f]/.test(item.failure) ) throw new Error("invalid fixer failure"); metadata.push("failure"); } + if (pr && "cause" in item) { + if (item.action !== "requeue" || !CAUSES.includes(item.cause)) throw new Error("invalid fixer cause"); + metadata.push("cause"); + } if (pr && "disable" in item) { - if (item.action !== "requeue" || typeof item.disable !== "string" || !/^[a-f0-9]{40}$/.test(item.disable)) + if (item.cause !== "flaky" || typeof item.disable !== "string" || !/^[a-f0-9]{40}$/.test(item.disable)) throw new Error("invalid fixer disable"); metadata.push("disable"); } @@ -182,6 +200,7 @@ export function parseFixResult(text: string, numbers: number[]): FixResult[] { throw new Error("invalid or duplicate fixer PR"); seen.add(item.number); validate(item, true); + if ("failure" in item) item.failure = signature(item.failure); } return value as FixResult[]; } diff --git a/src/cli/land-fixer.ts b/src/cli/land-fixer.ts index f0d3c455..c4d74675 100644 --- a/src/cli/land-fixer.ts +++ b/src/cli/land-fixer.ts @@ -29,7 +29,16 @@ import { type Reason, } from "./land"; import { COMMENT_LIMIT, resolution, resolutionSection, type Resolution } from "./land-apply"; -import { readFixResult, parseBaseFix, parseFixResult, type BaseFix, type FixResult } from "./land-fix"; +import { + CAUSES, + type Cause, + readFixResult, + signature, + parseBaseFix, + parseFixResult, + type BaseFix, + type FixResult, +} from "./land-fix"; import { readRemote, type RemoteRead } from "./land"; import { importFixer } from "./land-fixer-repository"; import { isPrepared, push, remoteHead } from "./land-head"; @@ -151,10 +160,10 @@ interface Job extends Process { } /** * How a round ended: a fixer commit (`fixed`), a clean base merge with no fixer edit (`merged`), no new - * commit for a failure the fixer did not judge flaky (`unchanged`), an unchanged re-enqueue of a failure - * it judged flaky (`flaky`), or a handback. + * commit for a failure the fixer found no cause of outside the PR (`unchanged`), an unchanged re-enqueue + * for the cause it named (`flaky`, `base`, or `environment`), or a handback. */ -export type Outcome = "fixed" | "merged" | "unchanged" | "flaky" | "handback"; +export type Outcome = "fixed" | "merged" | "unchanged" | Cause | "handback"; /** One failure a pick was blamed for, and what came of it. */ export interface Round { @@ -207,38 +216,56 @@ function samePick(record: History | undefined, entry: Pick): /** * Why a pick stops being fixed while each round still changes something: a round with no new commit - * for a failure that isn't flaky (`unchanged`), the failure a fix addressed recurring right after it - * (`repeated`), or `FLAKY_LIMIT` flaky rounds in a row (`flaky`). + * for a failure with no cause outside the PR (`unchanged`), the failure a fix addressed recurring right + * after it (`repeated`), or `REQUEUE_LIMIT` unchanged re-enqueues in a row, whatever their causes + * (`requeued`). + */ +export type StopRule = "unchanged" | "repeated" | "requeued"; +/** + * The unchanged re-enqueues in a row that hand a PR back, so a real nondeterministic bug in it, or a + * base or runner that keeps failing it, reaches a person. */ -export type StopRule = "unchanged" | "repeated" | "flaky"; -/** The flaky rounds in a row that hand a PR back, so a real nondeterministic bug in it reaches a person. */ -export const FLAKY_LIMIT = 3; +export const REQUEUE_LIMIT = 3; +const requeue = (outcome: Outcome): outcome is Cause => (CAUSES as readonly string[]).includes(outcome); /** - * The stop rule `round` fires after `rounds`, the pick's earlier rounds, if any. A flaky round resets + * The stop rule `round` fires after `rounds`, the pick's earlier rounds, if any. A re-enqueue resets * neither of the other rules: the failure a fix addressed recurring after flaky rounds is still a * repeat. A fixer's own handback needs no rule. */ export function stopRule(rounds: Round[], round: Pick): StopRule | undefined { if (round.outcome === "handback") return undefined; if (round.outcome === "unchanged") return "unchanged"; - const last = rounds.filter((item) => item.outcome !== "flaky").at(-1); - if ((last?.outcome === "fixed" || last?.outcome === "merged") && last.failure === round.failure) return "repeated"; - if (round.outcome !== "flaky") return undefined; + const last = rounds.filter((item) => !requeue(item.outcome)).at(-1); + if ( + (last?.outcome === "fixed" || last?.outcome === "merged") && + signature(last.failure) === signature(round.failure) + ) + return "repeated"; + if (!requeue(round.outcome)) return undefined; let streak = 1; - for (let index = rounds.length - 1; index >= 0 && rounds[index].outcome === "flaky"; index--) streak++; - return streak >= FLAKY_LIMIT ? "flaky" : undefined; + for (let index = rounds.length - 1; index >= 0 && requeue(rounds[index].outcome); index--) streak++; + return streak >= REQUEUE_LIMIT ? "requeued" : undefined; } -/** A handback's reason, naming the stop rule that fired; worded so `fixerText` quotes it unchanged. */ -export function stopReason(rule: StopRule, failure: string): string { +/** + * A handback's reason, naming the stop rule `round` fired; a requeue limit names the last round's cause. + * Worded so `fixerText` quotes it unchanged. + */ +export function stopReason(rule: StopRule, round: Pick): string { switch (rule) { case "unchanged": - return "stop rule (no new commit): the fixer's round produced no new commit and the failure isn't flaky"; + return "stop rule (no new commit): the fixer's round produced no new commit and named no cause outside the pull request"; case "repeated": - return `stop rule (repeated failure): ${failure} failed again right after the round that addressed it`; - case "flaky": - return `stop rule (flaky limit): the failure was judged flaky ${FLAKY_LIMIT} times in a row; it may be a real nondeterministic bug`; + return `stop rule (repeated failure): ${round.failure} failed again right after the round that addressed it`; + case "requeued": + return `stop rule (requeue limit): the failure was requeued unchanged ${REQUEUE_LIMIT} times in a row, last ${ + round.outcome === "base" + ? "because the base fails it too; the base may need a fix" + : round.outcome === "environment" + ? "because the runner's environment fails it; the runner may need a fix" + : "judged flaky; it may be a real nondeterministic bug" + }`; } } @@ -1029,21 +1056,22 @@ async function collectJob( job.reason === "merge_conflict" ? conflictFailure(job.plan.base.sha) : (result?.failure ?? job.reason ?? "failed_checks"); - // A PR requeued unchanged is flaky by the fixer's judgment, unless the gate itself blamed it, as for a - // conflict that is still there: a blamed PR the fixer named is never requeued. + // A PR requeued unchanged fails for the cause the fixer named, flaky when it named none, unless the + // gate itself blamed it, as for a conflict that is still there: a blamed PR the fixer named is never + // requeued. const outcome: Outcome = result?.action === "fixed" ? "fixed" : result?.action === "requeue" ? job.blame ? "unchanged" - : "flaky" + : (result.cause ?? "flaky") : "handback"; const rule = stopRule( previous.rounds.filter((item) => item.job !== name), { outcome, failure }, ); - if (rule) error = stopReason(rule, failure); + if (rule) error = stopReason(rule, { outcome, failure }); round = rule ? { job: name, outcome: "handback", before: standing, failure, rule } : { job: name, outcome, before: standing, failure, ...(outcome === "handback" ? {} : { after: head }) }; diff --git a/src/cli/land-watch-fix.ts b/src/cli/land-watch-fix.ts index 4f2a07c2..9b23446f 100644 --- a/src/cli/land-watch-fix.ts +++ b/src/cli/land-watch-fix.ts @@ -8,7 +8,7 @@ import { UNREFUSED } from "./hold"; import { baseRemote, type Entry, mergeBase, type RemoteRead } from "./land"; import { resolutionSection, type Resolution } from "./land-apply"; import { checksSection } from "./land-checks"; -import { EFFORT_VARIABLE, type FixResult, parseFixResult, readFixResult } from "./land-fix"; +import { type Cause, EFFORT_VARIABLE, type FixResult, parseFixResult, readFixResult } from "./land-fix"; import { author, cited, @@ -104,6 +104,12 @@ const JOB = /^watch-job-.*\.json$/; const REMOVAL = /^removal-\d+\.json$/; /** The merge driver's own result, not an agent's: it is recorded in the PR's Review under this name. */ const MERGER = "quest land watch"; +/** How a requeue comment says the fixer found the failure was not the PR's, by its cause. */ +const JUDGED: Record = { + flaky: "judged the failure flaky", + base: "found the failure on the base too", + environment: "traced the failure to the runner's environment", +}; const PULL = `query($id: ID!) { node(id: $id) { ... on PullRequest { state baseRefName headRefOid mergeStateStatus mergeQueueEntry { enqueuedAt } autoMergeRequest { enabledAt } @@ -453,7 +459,7 @@ async function collect( ? `with ${job.base.ref} at ${job.base.sha} merged in by the quest merge driver` : result.action === "fixed" ? `with the land fixer's fix (${fixerText(attribution)}): ${fixerText(result.reason)}` - : `unchanged, since the land fixer judged the failure flaky: ${fixerText(result.failure ?? ("reason" in result ? result.reason : ""))}`; + : `unchanged, since the land fixer ${JUDGED[("cause" in result && result.cause) || "flaky"]}: ${fixerText(result.failure ?? ("reason" in result ? result.reason : ""))}`; const how = requeued === "armed" ? `armed auto-merge on this pull request at ${head}, so it joins the back of the merge queue once its required checks pass` @@ -512,7 +518,8 @@ async function collect( const conflict = job.reason === "merge_conflict"; const failure = conflict || !result ? job.failure : (result.failure ?? job.failure); if (!handback && result) { - // Requeued unchanged, a failed build is flaky by the fixer's judgment; a conflict still conflicts. + // Requeued unchanged, a failed build fails for the cause the fixer named; one it named no cause + // outside the PR for, like a conflict that still conflicts, gets no new commit and goes back. const outcome: Outcome = job.merged ? "merged" : result.action === "fixed" @@ -520,9 +527,9 @@ async function collect( : result.action === "requeue" ? conflict ? "unchanged" - : "flaky" + : (result.cause ?? "unchanged") : "handback"; - // A flaky verdict counts for its test across every PR, whatever becomes of this pick. + // Only a flaky verdict counts for its test across every PR, whatever becomes of this pick. if (outcome === "flaky" && result.action === "requeue") said.push( ...recordFlaky(context.root, state, { @@ -542,7 +549,7 @@ async function collect( record!.rounds.filter((item) => item.job !== name), { outcome, failure }, ); - if (rule) handback = stopReason(rule, failure); + if (rule) handback = stopReason(rule, { outcome, failure }); round = rule ? { job: name, outcome: "handback", before: entry.head, failure, rule } : { job: name, outcome, before: entry.head, failure, ...(outcome === "handback" ? {} : { after: head }) }; diff --git a/src/cli/land-watch-flaky.ts b/src/cli/land-watch-flaky.ts index 01de9b31..b3d0e961 100644 --- a/src/cli/land-watch-flaky.ts +++ b/src/cli/land-watch-flaky.ts @@ -3,7 +3,7 @@ import { existsSync, readFileSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { git } from "./land"; -import { FLAKY_INPUT, type FixResult, type KnownFlaky } from "./land-fix"; +import { FLAKY_INPUT, type FixResult, type KnownFlaky, signature } from "./land-fix"; import { validateFix } from "./land-fixer"; import { importFixer } from "./land-fixer-repository"; import { save } from "./land-serve"; @@ -85,7 +85,7 @@ export function writeKnownFlaky(state: string, checkout: string): void { if (known.length) writeFileSync(join(checkout, ".git", FLAKY_INPUT), JSON.stringify(known), { mode: 0o600 }); } -/** A collected round that requeued a failed build unchanged, judging it flaky. */ +/** A collected round that requeued a failed build unchanged, judging it flaky (`cause` is `flaky`). */ export interface FlakyRound { /** The fixer job's file name. */ job: string; @@ -107,7 +107,7 @@ export interface FlakyRound { */ export function recordFlaky(root: string, state: string, round: FlakyRound): string[] { const tests = readFlaky(state); - let test = tests.find((item) => item.failure === round.failure); + let test = tests.find((item) => signature(item.failure) === signature(round.failure)); if (!test) { test = { failure: round.failure, verdicts: [] }; tests.push(test); diff --git a/test/land-watch.test.ts b/test/land-watch.test.ts index d36312bb..45907da3 100644 --- a/test/land-watch.test.ts +++ b/test/land-watch.test.ts @@ -12,7 +12,7 @@ import type { GitHubDependencies } from "../src/cli/github"; import { readRemote } from "../src/cli/land"; import { parseResolution } from "../src/cli/land-apply"; import { CHECK_LOG } from "../src/cli/land-checks"; -import { COLLECTION_ATTEMPTS, FLAKY_LIMIT, history } from "../src/cli/land-fixer"; +import { COLLECTION_ATTEMPTS, REQUEUE_LIMIT, history } from "../src/cli/land-fixer"; import { privateRepository } from "../src/cli/land-serve"; import { jobsSection, readRemoval, SETTLE, watchPass, watchPermissions } from "../src/cli/land-watch"; import type { FixContext } from "../src/cli/land-watch-fix"; @@ -945,8 +945,11 @@ git add '${file}' git commit -qm 'mechanical fix' printf '[{"number":%s,"action":"fixed","head":"%s","reason":"add the missing file","failure":"${failure}"}]' "$2" "$(git rev-parse HEAD)" > quest-land-result.json `; -const flakyScript = `printf '[{"number":%s,"action":"requeue","reason":"healthy; flaky test","failure":"just test: render_timeout","comment":"render_timeout timed out under load."}]' "$2" > quest-land-result.json +/** A fixer round that requeues the PR unchanged for `cause`, naming render_timeout, disabling it at HEAD if asked. */ +const requeueScript = (cause?: string, disable = false) => + `printf '[{"number":%s,"action":"requeue","reason":"healthy","failure":"just test: render_timeout"${cause ? `,"cause":"${cause}"` : ""}${disable ? `,"disable":"%s"` : ""},"comment":"render_timeout timed out under load."}]' "$2"${disable ? ` "$(git rev-parse HEAD)"` : ""} > quest-land-result.json `; +const flakyScript = requeueScript("flaky"); /** Wait for `count` fixer jobs to have exited. */ async function exited(state: string, count = 1): Promise { @@ -1304,7 +1307,7 @@ printf '[{"number":%s,"action":"fixed","head":"%s","reason":"preserve both confl const pull: Pull = { number: 7, head, enqueues: [{ createdAt: at(1), actor: APP }], removals: [] }; const github = { ...world([pull], []), origin: repo.origin }; fix.round(7, flakyScript); - for (let round = 1; round <= FLAKY_LIMIT; round++) { + for (let round = 1; round <= REQUEUE_LIMIT; round++) { const minute = round * 10; pull.queued = undefined; pull.removals.push({ @@ -1324,11 +1327,11 @@ printf '[{"number":%s,"action":"fixed","head":"%s","reason":"preserve both confl await exited(state); github.now = Date.parse(at(minute + 2)); expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([ - round < FLAKY_LIMIT ? `requeued 7 ${head}` : "handback 7", + round < REQUEUE_LIMIT ? `requeued 7 ${head}` : "handback 7", ]); } expect(github.mutations).toEqual([`enqueuePullRequest 7 ${head}`, `enqueuePullRequest 7 ${head}`]); - expect(github.comments[7].at(-1)!.body).toContain("stop rule (flaky limit)"); + expect(github.comments[7].at(-1)!.body).toContain("stop rule (requeue limit)"); expect(history(state, 7)!.rounds.map((round) => round.outcome)).toEqual(["flaky", "flaky", "handback"]); // The handback ended the pick: the workflow's next enqueue, even of the same head, is a new one. @@ -1350,6 +1353,65 @@ printf '[{"number":%s,"action":"fixed","head":"%s","reason":"preserve both confl await exited(state); }); + test("a failure on the base or from the runner is requeued with no flaky verdict, and one with no cause goes back", async () => { + const { repo, state, fix, context } = setup(); + const base = repo.main(); + const heads = [5, 6].map((number) => repo.pull(number, { [`feature-${number}.txt`]: "new\n" })); + const github = { ...world([], []), origin: repo.origin }; + const drop = async (number: number, minute: number, script: string) => { + fix.round(number, script); + const pull = github.pulls.find((item) => item.number === number); + const removal = { + id: `R${minute}`, + createdAt: at(minute), + reason: "failed_checks", + actor: "github-merge-queue", + before: heads[number - 5], + }; + if (pull) { + pull.queued = undefined; + pull.removals.push(removal); + } else + github.pulls.push({ + number, + head: heads[number - 5], + enqueues: [{ createdAt: at(minute - 5), actor: "maintainer" }], + removals: [removal], + }); + github.runs.push({ ...failedRun(number, at(minute - 2), base), id: minute }); + github.now = Date.parse(at(minute + 1)); + expect(await watchPass(REPOSITORY, { state }, deps(github), context)).toEqual([ + `removed ${number} failed_checks fix`, + `fixing ${number}`, + ]); + await exited(state); + github.now = Date.parse(at(minute + 2)); + return watchPass(REPOSITORY, { state }, deps(github), context); + }; + // The same test fails on the base twice, then on the runner: never a flaky verdict, but still rounds. + expect(await drop(5, 10, requeueScript("base"))).toEqual([`requeued 5 ${heads[0]}`]); + expect(github.comments[5].at(-1)!.body).toContain( + "unchanged, since the land fixer found the failure on the base too: `just test: render_timeout`", + ); + expect(await drop(5, 20, requeueScript("base"))).toEqual([`requeued 5 ${heads[0]}`]); + expect(await drop(5, 30, requeueScript("environment"))).toEqual(["handback 5"]); + expect(github.comments[5].at(-1)!.body).toContain( + "the failure was requeued unchanged 3 times in a row, last because the runner's environment fails it", + ); + expect(history(state, 5)!.rounds.map((round) => round.outcome)).toEqual(["base", "base", "handback"]); + expect(readFlaky(state)).toEqual([]); + // So the test's first genuine flake is its first verdict, and its disable is ignored. + expect(await drop(6, 40, requeueScript("flaky", true))).toEqual([ + "disable ignored 6: first flaky verdict", + `requeued 6 ${heads[1]}`, + ]); + expect(readFlaky(state)).toMatchObject([{ failure: "just test: render_timeout", verdicts: [{ number: 6 }] }]); + // A requeue that names no cause outside the PR is no progress, and goes back. + expect(await drop(6, 50, requeueScript())).toEqual(["handback 6"]); + expect(github.comments[6].at(-1)!.body).toContain("stop rule (no new commit)"); + expect(readFlaky(state)[0].verdicts).toHaveLength(1); + }); + test("a test seen flaking twice is disabled in its own pull request, which the watcher enqueues", async () => { const { repo, state, fix, context } = setup(); const base = repo.main(); @@ -1357,7 +1419,7 @@ printf '[{"number":%s,"action":"fixed","head":"%s","reason":"preserve both confl const github = { ...world([], []), origin: repo.origin }; /** A flaky requeue naming render_timeout, disabling it at the checkout's HEAD after `script`. */ const flaky = (script: string) => `${script} -printf '[{"number":%s,"action":"requeue","reason":"healthy; flaky test","failure":"just test: render_timeout","disable":"%s"}]' "$2" "$(git rev-parse HEAD)" > quest-land-result.json +printf '[{"number":%s,"action":"requeue","reason":"healthy; flaky test","cause":"flaky","failure":"%s","disable":"%s"}]' "$2" "$FAILURE" "$(git rev-parse HEAD)" > quest-land-result.json `; /** A disabling commit on base that skips the test, adds its follow-up quest, and writes `extra`. */ const disabling = (extra: string) => `grep -q 'just test: render_timeout' .git/quest-land-flaky.json @@ -1371,9 +1433,13 @@ git add quest skip.txt ${extra ? "flake.nix" : ""} git commit -qm 'disable render_timeout'`; // The first verdict's disable is ignored: one judgment alone never disables a test. The second's edits a // protected path and is refused, and the third's is opened as its own pull request. - fix.round(5, flaky("[ ! -e .git/quest-land-flaky.json ]")); - fix.round(6, flaky(disabling("printf 'changed\\n' > flake.nix"))); - fix.round(7, flaky(disabling(""))); + // Each names the test with different quoting and spacing, which still makes one test. + fix.round(5, flaky("[ ! -e .git/quest-land-flaky.json ]\nFAILURE='just test: `render_timeout`'")); + fix.round( + 6, + flaky(`${disabling("printf 'changed\\n' > flake.nix")}\nFAILURE='just test: \\"render_timeout\\"'`), + ); + fix.round(7, flaky(`${disabling("")}\nFAILURE='just test: render_timeout'`)); const said: string[][] = []; for (const [index, number] of [5, 6, 7].entries()) { const minute = (index + 1) * 10; diff --git a/test/land.test.ts b/test/land.test.ts index 80cd12d2..5f9442df 100644 --- a/test/land.test.ts +++ b/test/land.test.ts @@ -30,10 +30,11 @@ import { validateFix, fixerMarkdown, fixerText, - FLAKY_LIMIT, + REQUEUE_LIMIT, history, redactTokens, type Round, + stopReason, stopRule, } from "../src/cli/land-fixer"; import { importFixer } from "../src/cli/land-fixer-repository"; @@ -2587,7 +2588,7 @@ describe.skipIf(process.platform === "win32")("asynchronous land fixer", () => { await finished; }; // The fixer blamed no PR and the base passed alone each time: flaky rounds, re-enqueued unchanged. - for (let round = 2; round < FLAKY_LIMIT; round++) { + for (let round = 2; round < REQUEUE_LIMIT; round++) { await fail(); expect(await fixture.collect()).toEqual(["requeued 1"]); } @@ -2595,12 +2596,12 @@ describe.skipIf(process.platform === "win32")("asynchronous land fixer", () => { expect(await fixture.collect()).toEqual(["handback 1"]); expect(fixture.jobs()).toEqual([]); expect(fixture.pull.landing).toBe(false); - expect(fixture.forge.comment(1)).toContain("stop rule (flaky limit)"); + expect(fixture.forge.comment(1)).toContain("stop rule (requeue limit)"); expect(fixture.forge.comment(1)).toContain(""); const rounds = history(fixture.state.state, 1)!.rounds; expect(rounds.map(({ outcome, rule }) => [outcome, rule])).toEqual([ - ...Array.from({ length: FLAKY_LIMIT - 1 }, () => ["flaky", undefined]), - ["handback", "flaky"], + ...Array.from({ length: REQUEUE_LIMIT - 1 }, () => ["flaky", undefined]), + ["handback", "requeued"], ]); expect(rounds[0]).toMatchObject({ before: fixture.pull.head, @@ -3661,13 +3662,46 @@ printf '[{"number":1,"action":"handback","diagnosis":"saw %s","recommendation":" parseBaseFix(JSON.stringify({ action: "fixed", head: "a".repeat(40), reason: "fine", failure: "x" })), ).toThrow(); }); - test("accepts a disabling commit only on a requeue", () => { + test("compares failure signatures without quoting or repeated whitespace", () => { + const [result] = parseFixResult( + JSON.stringify([{ number: 1, action: "requeue", reason: "fine", failure: ` cargo test: "\`render\`'" ` }]), + [1], + ); + expect(result.failure).toBe("cargo test: render"); + expect(() => + parseFixResult(JSON.stringify([{ number: 1, action: "requeue", reason: "fine", failure: "``" }]), [1]), + ).toThrow("invalid fixer failure"); + }); + test("accepts a cause only on a requeue, as one of the known causes", () => { + for (const cause of ["flaky", "base", "environment"]) { + const accepted = { number: 1, action: "requeue", reason: "fine", cause }; + expect(parseFixResult(JSON.stringify([accepted]), [1])).toEqual([accepted]); + } + for (const value of [ + { number: 1, action: "requeue", reason: "fine", cause: "runner" }, + { number: 1, action: "requeue", reason: "fine", cause: 7 }, + { number: 1, action: "fixed", head: "a".repeat(40), reason: "fine", cause: "base" }, + { number: 1, action: "handback", diagnosis: "d", recommendation: "r", cause: "flaky" }, + ]) + expect(() => parseFixResult(JSON.stringify([value]), [1])).toThrow("invalid fixer cause"); + }); + test("accepts a disabling commit only on a flaky requeue", () => { const disable = "d".repeat(40); - const accepted = { number: 1, action: "requeue", reason: "flaky", failure: "cargo test: render", disable }; + const accepted = { + number: 1, + action: "requeue", + reason: "flaky", + cause: "flaky", + failure: "cargo test: render", + disable, + }; expect(parseFixResult(JSON.stringify([accepted]), [1])).toEqual([accepted]); for (const value of [ - { number: 1, action: "requeue", reason: "flaky", disable: "short" }, - { number: 1, action: "requeue", reason: "flaky", disable: 7 }, + { number: 1, action: "requeue", reason: "flaky", cause: "flaky", disable: "short" }, + { number: 1, action: "requeue", reason: "flaky", cause: "flaky", disable: 7 }, + { number: 1, action: "requeue", reason: "flaky", disable }, + { number: 1, action: "requeue", reason: "on the base", cause: "base", disable }, + { number: 1, action: "requeue", reason: "runner", cause: "environment", disable }, { number: 1, action: "fixed", head: disable, reason: "fine", disable }, { number: 1, action: "handback", diagnosis: "d", recommendation: "r", disable }, ]) @@ -4040,13 +4074,24 @@ describe("land fixer stop rules", () => { expect(stopRule([round("merged", "conflict with b1")], round("fixed", "conflict with b1"))).toBe("repeated"); expect(stopRule([round("fixed"), round("flaky", "test b")], round("fixed"))).toBe("repeated"); expect(stopRule([round("fixed")], round("flaky"))).toBe("repeated"); + // A signature recorded with quoting, as before results were normalized, still matches. + expect(stopRule([round("fixed", "just test: `a`")], round("fixed", "just test: a"))).toBe("repeated"); }); test("hands back the third flaky round in a row, which a fix in between resets", () => { const flaky = (count: number) => Array.from({ length: count }, (_, index) => round("flaky", `test ${index}`)); - expect(stopRule(flaky(FLAKY_LIMIT - 2), round("flaky", "test x"))).toBeUndefined(); - expect(stopRule(flaky(FLAKY_LIMIT - 1), round("flaky", "test x"))).toBe("flaky"); + expect(stopRule(flaky(REQUEUE_LIMIT - 2), round("flaky", "test x"))).toBeUndefined(); + expect(stopRule(flaky(REQUEUE_LIMIT - 1), round("flaky", "test x"))).toBe("requeued"); expect( - stopRule([...flaky(FLAKY_LIMIT - 1), round("fixed", "test y")], round("flaky", "test x")), + stopRule([...flaky(REQUEUE_LIMIT - 1), round("fixed", "test y")], round("flaky", "test x")), ).toBeUndefined(); }); + test("counts every unchanged re-enqueue toward the limit, and names the last round's cause", () => { + const rounds = [round("base"), round("environment")]; + expect(stopRule(rounds.slice(1), round("base"))).toBeUndefined(); + expect(stopRule(rounds, round("flaky"))).toBe("requeued"); + expect(stopRule([round("fixed"), round("base")], round("fixed"))).toBe("repeated"); + expect(stopReason("requeued", round("base"))).toContain("the base fails it too"); + expect(stopReason("requeued", round("environment"))).toContain("the runner's environment fails it"); + expect(stopReason("requeued", round("flaky"))).toContain("last judged flaky"); + }); });