diff --git a/.changes/pr-pipeline/perf-postmerge-attempt-carry-2b9f6a17.md b/.changes/pr-pipeline/perf-postmerge-attempt-carry-2b9f6a17.md new file mode 100644 index 0000000000..87bf6ea8e5 --- /dev/null +++ b/.changes/pr-pipeline/perf-postmerge-attempt-carry-2b9f6a17.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Changed + +- pr-merge's `post-merge-dispatch` carries the completed run attempts its post-merge check read from one tick to the next, as `attempts.json` in the `pr-merge-post-merge-report` artifact it already keeps. The check step copies the last trusted run's file (read by `last-report.mjs`) to the path it names in `BABYSIT_POSTMERGE_ATTEMPTS`, and keeps the file the check leaves, whether the check runs or an idle tick reuses the report. On the 2026-10-28 ticks this spares the 8 attempt reads each check made, of the job's 50 to 57 GITHUB_TOKEN calls. The `merge` job's check names no file and reads as before. Permissions are unchanged. diff --git a/.changes/source-control/perf-postmerge-attempt-carry-7c41d0e2.md b/.changes/source-control/perf-postmerge-attempt-carry-7c41d0e2.md new file mode 100644 index 0000000000..580c59f85f --- /dev/null +++ b/.changes/source-control/perf-postmerge-attempt-carry-7c41d0e2.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Changed + +- `/source-control:babysit-prs`: the post-merge check (`babysit_postmerge.py`) answers an earlier attempt of a re-run full run from the file named in `BABYSIT_POSTMERGE_ATTEMPTS` before reading it, when the file holds that run's attempt, completed, at the same head SHA. A completed attempt never changes, so the answer decides as the read would. The check writes back the completed attempts it used; a check that looks up no attempt leaves the file as it was. A missing or malformed file (another version or repository, a key that does not match its entry, an attempt not completed, a field the reader does not use) is no file, and every attempt is read as before. Without the variable nothing changes. diff --git a/.github/scripts/pr-merge/merge-job.test.mjs b/.github/scripts/pr-merge/merge-job.test.mjs index a5a46fb765..1559f528bf 100644 --- a/.github/scripts/pr-merge/merge-job.test.mjs +++ b/.github/scripts/pr-merge/merge-job.test.mjs @@ -542,7 +542,7 @@ test("the early check fails red on a rate-limit stop, after its outputs and repo // check. const LAST_SHA = "d".repeat(40); const lastReport = () => ({ readable: true, ok: true, needsDispatch: false, shared: { mainSha: "a".repeat(40) } }); -const runIdleCheck = ({ last, code = "0", settle = 0, exit = 3 } = {}) => { +const runIdleCheck = ({ last, code = "0", settle = 0, exit = 3, attempts } = {}) => { const dir = tempDir("pr-merge-idle-"); try { const bin = join(dir, "bin"); @@ -560,6 +560,7 @@ const runIdleCheck = ({ last, code = "0", settle = 0, exit = 3 } = {}) => { writeFileSync(join(dir, "post-merge-last/post-merge-code"), `${code}\n`); writeFileSync(join(dir, "post-merge-last/post-merge-code-sha"), `${LAST_SHA}\n`); writeFileSync(join(dir, "post-merge-last/lane-records.json"), "the last run's"); + if (attempts !== undefined) writeFileSync(join(dir, "post-merge-last/attempts.json"), attempts); } const output = join(dir, "output"); const result = spawnSync("bash", ["-c", runScript("Check earlier lane merges", stepsOf(jobBlock("post-merge-dispatch")))], { @@ -578,6 +579,8 @@ const runIdleCheck = ({ last, code = "0", settle = 0, exit = 3 } = {}) => { keptCode: kept("post-merge-code"), keptSha: kept("post-merge-code-sha"), records: kept("lane-records.json"), + attempts: kept("attempts.json"), + handed: existsSync(join(dir, "post-merge-attempts.json")) ? readFileSync(join(dir, "post-merge-attempts.json"), "utf8") : null, }; } finally { rmSync(dir, { recursive: true, force: true }); @@ -619,6 +622,21 @@ test("a tick runs the check itself for no report, a refusal, a crash or a check } }); +// The attempt carry (babysit_postmerge.py ATTEMPTS_ENV): the last run's +// attempts file is handed to the check at the path the step names, and kept +// for the next tick whether the check runs or an idle tick reuses the report. +test("post-merge-dispatch hands the last run's attempts to the check and keeps them", () => { + const file = "${{ runner.temp }}/post-merge-attempts.json"; + assert.ok(step("Check earlier lane merges", stepsOf(jobBlock("post-merge-dispatch"))).includes(`BABYSIT_POSTMERGE_ATTEMPTS: ${file}\n`)); + for (const options of [{}, { settle: 1 }]) { + const run = runIdleCheck({ last: lastReport(), attempts: "carried", ...options }); + assert.equal(run.ran, options.settle === 1); + assert.equal(run.handed, "carried"); + assert.equal(run.attempts, "carried"); + } + assert.equal(runIdleCheck({ last: lastReport(), settle: 1 }).attempts, null); +}); + test("post-merge-dispatch reads the last run's report before its check, from a trusted run alone", () => { const list = stepsOf(jobBlock("post-merge-dispatch")); const names = list.map((s) => s.name); diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index 7e0512d578..3365907393 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -209,12 +209,17 @@ jobs: env: GH_TOKEN: ${{ github.token }} BABYSIT_POSTMERGE_LANE_RECORDS: ${{ runner.temp }}/lane-records.json + BABYSIT_POSTMERGE_ATTEMPTS: ${{ runner.temp }}/post-merge-attempts.json REPO: ${{ github.repository }} run: | set -euo pipefail last=$RUNNER_TEMP/post-merge-last scripts=.base/plugins/source-control/skills/babysit-prs/scripts code='' code_sha='' + # The completed attempts the last check read, which the check + # answers with no read (babysit_postmerge.py ATTEMPTS_ENV); kept + # below whether or not the check runs. + [ ! -f "$last/attempts.json" ] || cp "$last/attempts.json" "$RUNNER_TEMP/post-merge-attempts.json" # An idle tick: the last run's report, from a check that passed # (exit 0 or 5), stands while the lane room's reuse rule keeps it # and nothing else the check reads has moved (settle --idle-tick). @@ -275,6 +280,7 @@ jobs: # A reused report keeps the commit whose code made it. [ -z "$code_sha" ] || echo "$code_sha" >"$kept/post-merge-code-sha" [ ! -f "$RUNNER_TEMP/lane-records.json" ] || cp "$RUNNER_TEMP/lane-records.json" "$kept/lane-records.json" + [ ! -f "$RUNNER_TEMP/post-merge-attempts.json" ] || cp "$RUNNER_TEMP/post-merge-attempts.json" "$kept/attempts.json" # A rate-limit stop fails the job red once the outputs are written. [ "$code" != 75 ] || exit 1 diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py index e66f374211..b0a233a430 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py @@ -114,6 +114,15 @@ _LANE_RECORD_NAME = re.compile( r"pr-merge-(?:dispatched-[1-9][0-9]{0,15}|rerun-[1-9][0-9]{0,15}-attempt-2)" ) +# A completed attempt never changes, so pr-merge.yml's post-merge-dispatch +# names a file here holding the earlier attempts of re-run full runs that the +# last check read (`_Context._attempt`), carried in its kept report. An entry +# answers with no read only for the same run, attempt and head SHA, completed; +# the check writes back the completed attempts it used. Any other file is no +# file, and every attempt is read as before. +ATTEMPTS_ENV = "BABYSIT_POSTMERGE_ATTEMPTS" +ATTEMPTS_VERSION = 1 +_ATTEMPT_KEY = re.compile(r"[1-9][0-9]{0,15}:[1-9][0-9]{0,2}") # With `--lane-merges-only`, a red attempt 1 of a lane-dispatched full run # created within red-main.confirm-wait-minutes plus this, with a newly failing # check that demotes (`_Context.regressions`), gets that re-run even when no @@ -804,7 +813,8 @@ def _runs( return [r for r in runs if r.created_at >= since and r.head_branch == branch] -def _attempt(owner: str, name: str, run_id: int, attempt: int) -> Run | None: +def _attempt(owner: str, name: str, run_id: int, attempt: int) -> Any: + """The attempt's payload as the API returns it, or None on a 404.""" args = ["api", f"repos/{owner}/{name}/actions/runs/{run_id}/attempts/{attempt}"] try: proc = gh_capture(args) @@ -817,11 +827,62 @@ def _attempt(owner: str, name: str, run_id: int, attempt: int) -> Run | None: f"run {run_id} attempt {attempt}: {(proc.stderr or '').strip()}" ) try: - return Run.from_api(json.loads(proc.stdout)) + return json.loads(proc.stdout) except ValueError as exc: raise PostMergeReadError(f"run {run_id} attempt {attempt}: {exc}") from exc +def _attempt_fields(payload: Any) -> dict[str, Any]: + """The fields of an attempt payload `Run.from_api` reads, and no others.""" + item = json_object(payload) + return { + **{ + key: item.get(key) + for key in ( + "id", + "run_attempt", + "status", + "conclusion", + "head_sha", + "head_branch", + "created_at", + "html_url", + ) + }, + "actor": {"login": json_object(item.get("actor")).get("login")}, + "triggering_actor": { + "login": json_object(item.get("triggering_actor")).get("login") + }, + } + + +def load_attempts(path: str, repo: str) -> dict[str, dict[str, Any]]: + """The completed attempts in the file at `path`, by `:`. + Raises `ValueError` for any file of another shape, so none of it is used.""" + data = json_object(json.loads(Path(path).read_text(encoding="utf-8"))) + attempts = data.get("attempts") + if ( + data.get("version") != ATTEMPTS_VERSION + or data.get("repo") != repo + or not is_json_object(attempts) + ): + raise ValueError("an unexpected shape") + for key, entry in attempts.items(): + try: + run = Run.from_api(entry) + except PostMergeReadError as exc: + raise ValueError(f"{key!r}: {exc}") from exc + if not ( + _ATTEMPT_KEY.fullmatch(key) + and key == f"{run.id}:{run.attempt}" + and run.status == "completed" + and _FULL_SHA.fullmatch(run.head_sha) + and entry == _attempt_fields(entry) + ): + raise ValueError(f"{key!r}: an entry of another shape") + return attempts + + # --- Judging runs ------------------------------------------------------------------- _FULL_SHA = re.compile(r"[0-9a-f]{40}") @@ -1132,6 +1193,13 @@ class _Context: lane_proved: dict[str, dict[str, Any]] = field(default_factory=dict) # Carried records whose sender failed `_lane_sender`: never written back. lane_rejected: set[str] = field(default_factory=set) + # The attempts file (`ATTEMPTS_ENV`), its attempts once loaded, and the + # completed attempts this check used, which `save_attempts` writes. + attempts_path: str | None = field( + default_factory=lambda: os.environ.get(ATTEMPTS_ENV) or None + ) + attempts_carried: dict[str, dict[str, Any]] | None = None + attempts_used: dict[str, dict[str, Any]] = field(default_factory=dict) def dispatch_runs_through( self, commit: str, merged_at: datetime, deadline: datetime @@ -1940,7 +2008,60 @@ def outcome(self, run: Run, *, attributable: bool) -> tuple[str, str]: return self.outcome_cache[key] def _attempt(self, run: Run, n: int) -> Run | None: - return run if run.attempt == n else _attempt(self.owner, self.name, run.id, n) + """Attempt `n` of `run`: the listed run itself, a completed attempt + the last check carried for the same run and head SHA, or a read.""" + if run.attempt == n: + return run + key = f"{run.id}:{n}" + payload = self._carried_attempts().get(key) + if payload is None or Run.from_api(payload).head_sha != run.head_sha: + payload = _attempt(self.owner, self.name, run.id, n) + if payload is None: + return None + found = Run.from_api(payload) + if ( + found.status == "completed" + and f"{found.id}:{found.attempt}" == key + and found.head_sha == run.head_sha + ): + self.attempts_used[key] = _attempt_fields(payload) + return found + + def _carried_attempts(self) -> dict[str, dict[str, Any]]: + if self.attempts_carried is None: + self.attempts_carried = {} + path = self.attempts_path + if path and Path(path).exists(): + try: + self.attempts_carried = load_attempts( + path, f"{self.owner}/{self.name}" + ) + except Exception as exc: # noqa: BLE001 (any unusable file is no file) + text = re.sub(r"[\r\n%]", " ", f"{type(exc).__name__}: {exc}") + print(f"attempts not used: {text[:300]}", file=sys.stderr) + return self.attempts_carried + + def save_attempts(self) -> None: + """Write the attempts file: the completed attempts this check used. + Untouched when the check looked up no attempt. Best effort: a + failure is logged, and the file is left as it was.""" + path = self.attempts_path + if not path or self.attempts_carried is None: + return + payload = { + "version": ATTEMPTS_VERSION, + "repo": f"{self.owner}/{self.name}", + "attempts": self.attempts_used, + } + target = Path(path) + tmp = target.with_name(f"{target.name}.{os.getpid()}.tmp") + try: + tmp.write_text(json.dumps(payload), encoding="utf-8") + os.replace(tmp, target) + except OSError as exc: + print(f"attempts not written to {path}: {exc}", file=sys.stderr) + with contextlib.suppress(OSError): + tmp.unlink(missing_ok=True) def _outcome(self, run: Run, attributable: bool) -> tuple[str, str]: first = self._attempt(run, 1) @@ -2397,6 +2518,7 @@ def lane_prs_at(sha: str) -> str | None: finally: if ctx is not None: ctx.save_lane_records(merged_since) + ctx.save_attempts() return PostMergeVerdict( True, merges=states, diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge_attempts.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge_attempts.py new file mode 100644 index 0000000000..de1eb4bfcf --- /dev/null +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge_attempts.py @@ -0,0 +1,222 @@ +"""The attempt carry of the post-merge check (`babysit_postmerge`). + +A completed attempt of a workflow run never changes, so a check answers an +earlier attempt of a re-run full run from the file pr-merge.yml names in +`BABYSIT_POSTMERGE_ATTEMPTS` with no read, for the same run, attempt and head +SHA, and writes back the completed attempts it used. The reports are pins: +the check with no file (the code before the carry) is the source, and a +carried check must report the same. Request counts come from the +`babysit-gh-calls` counter (`babysit_gh._run_gh`), with gh stubbed at +`run_command` by the same fake as the check's tests; each run at attempt 2 +costs one read of its attempt 1 (`_Context._outcome`: attempt 1 decides). +""" + +from __future__ import annotations + +import contextlib +import io +import json +import pathlib +import sys +import tempfile +import unittest +from datetime import UTC, datetime +from typing import Any +from unittest import mock + +sys.path.insert(0, str(pathlib.Path(__file__).resolve().parent.parent)) +sys.path.insert(0, str(pathlib.Path(__file__).resolve().parent)) + +import babysit_gh as gh +import babysit_postmerge as pm +from test_babysit_postmerge import ( + IDENTITY, + OTHER_SHA, + REPO, + SINCE, + FakeGh, + by, + canary_run, + dispatch_run, + human, + schedule_run, +) + +NOW = datetime(2026, 10, 12, 11, 0, tzinfo=UTC) +ATTEMPT_READS = "default GET repos/{}/{}/actions/runs/{}/attempts/{}" + + +class AttemptCase(unittest.TestCase): + """Dispatch run 11 failed attempt 1 and passed its re-run (a flake), and + schedule run 31 failed both attempts (a failure): two attempt-1 reads.""" + + def setUp(self) -> None: + self.gh = FakeGh() + tmp = tempfile.TemporaryDirectory(prefix="attempts-") + self.addCleanup(tmp.cleanup) + self.path = pathlib.Path(tmp.name) / "attempts.json" + + def run_command(command: list[str], **kwargs: Any) -> Any: + # The fake answers without headers, so the `-i` the guard adds is dropped. + args = [a for i, a in enumerate(command[1:]) if (i, a) != (1, "-i")] + return self.gh(args, timeout_seconds=kwargs.get("timeout_seconds")) + + patcher = mock.patch.object(gh, "run_command", side_effect=run_command) + patcher.start() + self.addCleanup(patcher.stop) + self.addCleanup(gh.reset_gh_call_counts) + self.gh.add_runs("post-merge-detect-silent-revert.yml", "push", canary_run(21)) + self.gh.add_runs( + "pr-require-checks.yml", + "workflow_dispatch", + dispatch_run(11, run_attempt=2, conclusion="success"), + ) + self.gh.attempts[(11, 1)] = dispatch_run(11, conclusion="failure") + after = "2026-10-12T10:02:00Z" + self.gh.add_runs( + "pr-require-checks.yml", + "schedule", + schedule_run( + 31, run_attempt=2, triggering_actor=by(IDENTITY), created_at=after + ), + ) + self.gh.attempts[(31, 1)] = schedule_run(31, created_at=after) + + def check(self, path: pathlib.Path | str | None = None) -> pm.PostMergeVerdict: + """One check, with the attempts file at `path` (None: no file named, + the check as it was before the carry).""" + gh.reset_gh_call_counts() + env = {pm.ATTEMPTS_ENV: "" if path is None else str(path)} + with ( + mock.patch.dict("os.environ", env), + contextlib.redirect_stderr(io.StringIO()) as self.stderr, + ): + return pm.check( + REPO, + agent_identity=IDENTITY, + is_human=human, + merged_since=SINCE, + now=NOW, + pinned_sha=lambda merge: None, + ) + + def reads(self) -> int: + return gh.gh_call_counts().get(ATTEMPT_READS, 0) + + def written(self) -> dict[str, Any]: + return json.loads(self.path.read_text(encoding="utf-8")) + + def baseline(self) -> dict[str, Any]: + """The report of the check with no file: the pin's source.""" + report = self.check().report() + self.assertEqual(self.reads(), 2) + return report + + +class TheCarrySparesTheAttemptReads(AttemptCase): + def test_a_carried_check_reports_as_one_with_no_file_in_two_fewer_reads( + self, + ) -> None: + expected = self.baseline() + self.assertEqual( + [f.signal for f in self.check(self.path).failures], ["schedule"] + ) + self.assertEqual(self.reads(), 2) + self.assertEqual(set(self.written()["attempts"]), {"11:1", "31:1"}) + carried = self.check(self.path) + self.assertEqual(self.reads(), 0) + self.assertEqual(carried.report(), expected) + # The flake and the failure stand on the carried attempt 1. + self.assertEqual( + carried.flakes, (f"https://github.com/{REPO}/actions/runs/11",) + ) + self.assertEqual( + carried.failures[0].run_url, + f"https://github.com/{REPO}/actions/runs/31/attempts/2", + ) + + def test_a_carried_check_writes_the_same_attempts_back(self) -> None: + self.check(self.path) + first = self.written() + self.check(self.path) + self.assertEqual(self.written(), first) + + def test_with_no_file_named_every_attempt_is_read(self) -> None: + self.check(self.path) + self.check(None) + self.assertEqual(self.reads(), 2) + + +class AnUnusableFileIsNoFile(AttemptCase): + def carry(self) -> dict[str, Any]: + self.check(self.path) + return self.written() + + def assert_read_as_with_no_file(self, expected: dict[str, Any]) -> None: + verdict = self.check(self.path) + self.assertEqual(self.reads(), 2) + self.assertEqual(verdict.report(), expected) + + def test_garbage_is_read_live(self) -> None: + expected = self.baseline() + self.path.write_text("{not json", encoding="utf-8") + self.assert_read_as_with_no_file(expected) + self.assertIn("attempts not used", self.stderr.getvalue()) + + def test_another_repository_or_version_is_read_live(self) -> None: + expected = self.baseline() + for change in ({"repo": "other/repo"}, {"version": 2}): + data = {**self.carry(), **change} + self.path.write_text(json.dumps(data), encoding="utf-8") + self.assert_read_as_with_no_file(expected) + + def test_an_in_progress_or_misfiled_entry_drops_the_whole_file(self) -> None: + expected = self.baseline() + for key, change in ( + ("11:1", {"status": "in_progress", "conclusion": None}), + ("11:1", {"id": 12}), + ("31:1", {"run_attempt": 2}), + ("31:1", {"head_sha": "not-a-sha"}), + ("31:1", {"extra": True}), + ): + data = self.carry() + data["attempts"][key] = {**data["attempts"][key], **change} + self.path.write_text(json.dumps(data), encoding="utf-8") + self.assert_read_as_with_no_file(expected) + + def test_an_entry_for_another_head_sha_is_read_live(self) -> None: + expected = self.baseline() + data = self.carry() + for entry in data["attempts"].values(): + entry["head_sha"] = OTHER_SHA + self.path.write_text(json.dumps(data), encoding="utf-8") + self.assert_read_as_with_no_file(expected) + # The live reads replace the stale entries. + self.assertNotIn( + OTHER_SHA, {e["head_sha"] for e in self.written()["attempts"].values()} + ) + + +class OnlyCompletedAttemptsAreWritten(AttemptCase): + def test_an_in_progress_attempt_is_not_written(self) -> None: + self.gh.attempts[(11, 1)] = dispatch_run( + 11, status="in_progress", conclusion=None + ) + self.check(self.path) + self.assertEqual(set(self.written()["attempts"]), {"31:1"}) + + def test_a_check_that_looks_up_no_attempt_leaves_the_file_as_it_was( + self, + ) -> None: + self.check(self.path) + before = self.path.read_text(encoding="utf-8") + self.gh.runs.clear() + self.gh.add_runs("post-merge-detect-silent-revert.yml", "push", canary_run(21)) + self.gh.add_runs("pr-require-checks.yml", "workflow_dispatch", dispatch_run(11)) + self.check(self.path) + self.assertEqual(self.reads(), 0) + self.assertEqual(self.path.read_text(encoding="utf-8"), before) + + +if __name__ == "__main__": + unittest.main()