From bf03051c892c3652a31d525cd9c333e55518ed92 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 24 Oct 2026 08:25:04 -0400 Subject: [PATCH] fix(pr-merge): fetch full history for the lane-room read and give it its own timeout The enable-auto-merge checkout was a depth-1 clone, so the post-merge check's local containment test could not decide and every lane record fell back to a compare API call. Fetch full history, as post-merge-dispatch and merge do. The lane-room read gets LANE_ROOM_TIMEOUT_MS (8 minutes) as a backstop, since it may run a full post-merge check. The arm step's timeout becomes 18 minutes and the job's 30 so the room read, the verdict budget and the last read still fit. Refs: #9577 Co-Authored-By: Claude Sonnet 5.5 --- .../fix-lane-room-timeout-6d2f8a94.md | 7 +++++++ .github/scripts/pr-merge/enable-auto-merge.mjs | 15 +++++++++------ .../scripts/pr-merge/enable-auto-merge.test.mjs | 14 ++++++++++++++ .github/scripts/pr-merge/workflow.test.mjs | 16 +++++++++++++--- .github/workflows/pr-merge.yml | 5 +++-- 5 files changed, 46 insertions(+), 11 deletions(-) create mode 100644 .changes/pr-pipeline/fix-lane-room-timeout-6d2f8a94.md diff --git a/.changes/pr-pipeline/fix-lane-room-timeout-6d2f8a94.md b/.changes/pr-pipeline/fix-lane-room-timeout-6d2f8a94.md new file mode 100644 index 0000000000..0098147e5f --- /dev/null +++ b/.changes/pr-pipeline/fix-lane-room-timeout-6d2f8a94.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- `enable-auto-merge` checks out the default branch with full history, as the post-merge check and the merge job do, so the room's post-merge recompute decides containment locally instead of making a compare API call per lane record. The lane-room read also has its own 8 minute timeout, apart from the verifier reads' 4 minutes, and a timeout still holds the lane. The arm step's timeout is now 18 minutes and the job's 30. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index 49b873fae2..0f6af5a30e 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -470,7 +470,7 @@ export function pythonVerify(env) { // `lane-room`), read with the reader's Actions token when it has one, as the // merge job reads the post-merge check, so the reads stay off the lane App's // quota. -export function pythonRoom(env) { +export function pythonRoom(env, spawn = spawnSync) { return async () => { const args = [env.VERIFY_EVIDENCE_SCRIPT || VERIFY_SCRIPT, "lane-room", "--repo", env.GITHUB_REPOSITORY, "--postmerge-result", env.POSTMERGE_RESULT ?? ""]; if (env.RUNG_CAP !== undefined) { @@ -478,7 +478,7 @@ export function pythonRoom(env) { } const token = env.BABYSIT_VERIFY_GH_TOKEN; const roomEnv = token ? { ...env, GH_TOKEN: token, API_BUDGET_TOKEN: "github-token" } : env; - const done = spawnSync("python3", args, { encoding: "utf8", env: roomEnv, timeout: VERIFY_TIMEOUT_MS }); + const done = spawn("python3", args, { encoding: "utf8", env: roomEnv, timeout: LANE_ROOM_TIMEOUT_MS }); if (done.status !== 0) { throw new Error(`babysit_verify_evidence.py lane-room exited ${done.status ?? done.signal}: ${String(done.stderr ?? "").trim().split("\n").at(-1)}`); } @@ -512,11 +512,14 @@ export function readRoom(answer) { // stuck. export const READER_EVENTS = Object.freeze(["schedule", "workflow_dispatch"]); // The reader's per pull request limit, and when the run stops starting one: -// the room read (at most VERIFY_TIMEOUT_MS) comes first, then a read started -// at the budget ends by VERIFY_BUDGET_MS + VERIFY_TIMEOUT_MS, 13 minutes in -// all, inside the step's 14 (pr-merge.yml), so no read is cut off and the -// rest of the pull requests are left for the next run. +// the room read (at most LANE_ROOM_TIMEOUT_MS) comes first, then a read +// started at the budget ends by VERIFY_BUDGET_MS + VERIFY_TIMEOUT_MS, 17 +// minutes in all, inside the step's 18 (pr-merge.yml), so no read is cut off +// and the rest of the pull requests are left for the next run. export const VERIFY_TIMEOUT_MS = 4 * 60_000; +// Longer than a verifier read: the room may run a full post-merge check. A +// timeout still reads as unreadable, which holds the lane. +export const LANE_ROOM_TIMEOUT_MS = 8 * 60_000; export const VERIFY_BUDGET_MS = 5 * 60_000; // What to do with one listed pull request: { action: arm | disarm | none, reason }. diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index 2bf6d42e46..428802f239 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -28,6 +28,8 @@ import { READER_EVENTS, main, pythonRoom, + LANE_ROOM_TIMEOUT_MS, + VERIFY_TIMEOUT_MS, readRoom, verifyHold, } from "./enable-auto-merge.mjs"; @@ -1232,6 +1234,18 @@ print(json.dumps({"argv": sys.argv[1:], "token": os.environ.get("GH_TOKEN"), "bu assert.deepEqual((await pythonRoom(env)()).argv, ["lane-room", "--repo", REPO, "--postmerge-result", "/tmp/post-merge.json"]); }); +test("pythonRoom gives the lane-room read its own 8 minute timeout and reads a kill as unreadable", async () => { + const seen = []; + const killed = (_cmd, _args, options) => { + seen.push(options.timeout); + return { status: null, signal: "SIGTERM", stdout: "", stderr: "" }; + }; + await assert.rejects(pythonRoom({ GITHUB_REPOSITORY: REPO }, killed)(), /lane-room exited SIGTERM/); + assert.deepEqual(seen, [8 * 60_000]); + assert.equal(LANE_ROOM_TIMEOUT_MS, 8 * 60_000); + assert.equal(VERIFY_TIMEOUT_MS, 4 * 60_000); +}); + // The gate holds on the security lane's open CRITICAL findings // (babysit_risk LANES, security-finding) and on a review not bound to the // head, whether or not the lane applied its hold label. diff --git a/.github/scripts/pr-merge/workflow.test.mjs b/.github/scripts/pr-merge/workflow.test.mjs index 191a58a8aa..9017807486 100644 --- a/.github/scripts/pr-merge/workflow.test.mjs +++ b/.github/scripts/pr-merge/workflow.test.mjs @@ -12,7 +12,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { test } from "node:test"; -import { VERIFY_BUDGET_MS, VERIFY_TIMEOUT_MS } from "./enable-auto-merge.mjs"; +import { LANE_ROOM_TIMEOUT_MS, VERIFY_BUDGET_MS, VERIFY_TIMEOUT_MS } from "./enable-auto-merge.mjs"; const read = (name) => readFileSync(new URL(`../../workflows/${name}`, import.meta.url), "utf8"); const merge = read("pr-merge.yml"); @@ -169,7 +169,7 @@ test("the last verifier read enable-auto-merge starts ends inside its step's tim const step = /- name: Arm and disarm auto-merge\n {8}if: .*\n {8}timeout-minutes: ([0-9]+)\n/.exec(arm); assert.ok(step, "the arm step names its timeout"); // The room read, then the verdict budget and the last read it starts. - assert.ok(VERIFY_TIMEOUT_MS + VERIFY_BUDGET_MS + VERIFY_TIMEOUT_MS < Number(step[1]) * 60_000); + assert.ok(LANE_ROOM_TIMEOUT_MS + VERIFY_BUDGET_MS + VERIFY_TIMEOUT_MS < Number(step[1]) * 60_000); }); test("enable-auto-merge logs both tokens' readings at job end, before the token is revoked", () => { @@ -221,7 +221,7 @@ test("enable-auto-merge reads the lane's room from this run's post-merge-dispatc // A room that checks the post-merge answer again reads the lane records the // last run kept, as post-merge-dispatch's check does, so the check answers // them with no artifact or run read each; without them the check can run -// past the room's VERIFY_TIMEOUT_MS. +// past the room's LANE_ROOM_TIMEOUT_MS. test("enable-auto-merge hands the room's post-merge check the lane records the last run kept", () => { const arm = job("enable-auto-merge"); const records = /- name: Read the lane records the last run kept\n([\s\S]*?)(?=\n {6}- name: )/.exec(arm)?.[1]; @@ -237,6 +237,16 @@ test("enable-auto-merge hands the room's post-merge check the lane records the l assert.match(arm, /\n {6}actions: read #/); }); +// Without history the post-merge check's containment test cannot decide, and +// every lane record falls back to a compare API call. +test("the default branch checkouts of the post-merge check, the merge and enable-auto-merge fetch full history", () => { + for (const name of ["post-merge-dispatch", "merge", "enable-auto-merge"]) { + const checkout = /- name: Check out the default branch tip\n([\s\S]*?)(?=\n {6}- name: |\n*$)/.exec(job(name))?.[1]; + assert.ok(checkout, `${name} checks out the default branch tip`); + assert.match(checkout, /\n {10}fetch-depth: 0\n/, name); + } +}); + test("enable-auto-merge's step timeouts fit inside its job's", () => { const arm = job("enable-auto-merge"); const total = Number(/\n {4}timeout-minutes: ([0-9]+)\n/.exec(arm)[1]); diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index 10879ebb7b..3f9906ec86 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -1364,7 +1364,7 @@ jobs: queue: max runs-on: ubuntu-24.04 # Above the sum of its steps' timeouts (workflow.test.mjs checks it). - timeout-minutes: 25 + timeout-minutes: 30 permissions: contents: read # check out main actions: read # the verifier verdicts, their runs and the head-ref map @@ -1378,6 +1378,7 @@ jobs: with: ref: refs/heads/main path: .base + fetch-depth: 0 persist-credentials: false - name: Set up Node @@ -1513,7 +1514,7 @@ jobs: # decision and writes nothing. - name: Arm and disarm auto-merge if: steps.lane-token.outputs.token != '' - timeout-minutes: 14 + timeout-minutes: 18 env: GH_TOKEN: ${{ steps.lane-token.outputs.token }} KILL_SWITCH: ${{ steps.kill.outputs.proceed == 'true' && 'proceed' || '' }}