From c081da115a9f398cb0bc5966f86f243a55ce87c5 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 2 Nov 2026 08:55:09 -0500 Subject: [PATCH 1/2] fix(source-control): refuse pre-since legacy FAILs and retry the FAIL index pre-read legacy-fails now refuses, writing nothing, when the whole artifact listing holds a FAIL uploaded before --since, naming the earliest such artifact, its run and upload time. A FAIL of a run created before --since but uploaded at or after it is still added through the listing. The listing keeps each FAIL's created_at; a checkpoint whose listing lacks it lists again. upload-fail-index reads the run's artifacts up to three times, 5 s then 10 s apart, before failing on a read that fails or misses its count. Co-Authored-By: Claude Opus 5.5 --- .../fix-legacy-fails-since-guard-4e8b0d15.md | 7 ++ .../fix-legacy-fails-since-guard-7a3c91e2.md | 7 ++ .github/actions/upload-fail-index/action.yml | 27 ++++--- .github/scripts/pr-verify/workflow.test.mjs | 41 ++++++++-- .../scripts/babysit_verify_evidence.py | 34 +++++++- .../tests/test_babysit_verify_index.py | 79 ++++++++++++++++--- 6 files changed, 166 insertions(+), 29 deletions(-) create mode 100644 .changes/pr-pipeline/fix-legacy-fails-since-guard-4e8b0d15.md create mode 100644 .changes/source-control/fix-legacy-fails-since-guard-7a3c91e2.md diff --git a/.changes/pr-pipeline/fix-legacy-fails-since-guard-4e8b0d15.md b/.changes/pr-pipeline/fix-legacy-fails-since-guard-4e8b0d15.md new file mode 100644 index 00000000000..ae99bfeca55 --- /dev/null +++ b/.changes/pr-pipeline/fix-legacy-fails-since-guard-4e8b0d15.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- `upload-fail-index` reads the run's artifacts up to three times, 5 s then 10 s apart, before failing on a read that fails or misses its own count, so one transient API error no longer leaves a FAIL judgment with no verdict. Every other check is unchanged. diff --git a/.changes/source-control/fix-legacy-fails-since-guard-7a3c91e2.md b/.changes/source-control/fix-legacy-fails-since-guard-7a3c91e2.md new file mode 100644 index 00000000000..3b2ad043bf8 --- /dev/null +++ b/.changes/source-control/fix-legacy-fails-since-guard-7a3c91e2.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- `babysit_verify_evidence.py legacy-fails` refuses, writing nothing, when the whole artifact listing holds a FAIL uploaded before `--since`, naming the earliest such artifact, its run and its upload time so the operator can lower `--since`. A FAIL of a run created before `--since` but uploaded at or after it is still added through the listing. A checkpoint whose listing kept no upload times lists again. diff --git a/.github/actions/upload-fail-index/action.yml b/.github/actions/upload-fail-index/action.yml index 62b7f1ed511..994bef66f60 100644 --- a/.github/actions/upload-fail-index/action.yml +++ b/.github/actions/upload-fail-index/action.yml @@ -52,15 +52,24 @@ runs: exit 1 fi # The names this run holds already, every attempt's, in one read - # that must hold its own count. - if ! body=$(gh api "repos/$REPO/actions/runs/$RUN_ID/artifacts?per_page=100"); then - echo "::error::the run's artifacts could not be read" - exit 1 - fi - if ! jq -e '.total_count == (.artifacts | length) and .total_count < 100' <<<"$body" >/dev/null; then - echo "::error::the run's artifacts do not hold their count" - exit 1 - fi + # that must hold its own count: three attempts, 5 s then 10 s apart. + attempt=0 + for wait in 5 10 ''; do + attempt=$((attempt + 1)) + if ! body=$(gh api "repos/$REPO/actions/runs/$RUN_ID/artifacts?per_page=100"); then + why="the run's artifacts could not be read" + elif ! jq -e '.total_count == (.artifacts | length) and .total_count < 100' <<<"$body" >/dev/null; then + why="the run's artifacts do not hold their count" + else + break + fi + if [ -z "$wait" ]; then + echo "::error::$why (attempt $attempt of 3)" + exit 1 + fi + echo "::warning::$why (attempt $attempt of 3), reading again in ${wait}s" + sleep "$wait" + done mapfile -t held < <(jq -r '.artifacts[].name' <<<"$body") slot=0 for name in "${names[@]}"; do diff --git a/.github/scripts/pr-verify/workflow.test.mjs b/.github/scripts/pr-verify/workflow.test.mjs index bdcb3d6e188..45224765241 100644 --- a/.github/scripts/pr-verify/workflow.test.mjs +++ b/.github/scripts/pr-verify/workflow.test.mjs @@ -115,23 +115,36 @@ const upstreamRun = (overrides = {}) => ({ }); // upload-fail-index's "Split the names" step, run with a stand-in gh that -// answers the run's artifacts read with `held` (or fails), returning its exit -// status and the outputs it wrote. +// answers the run's artifacts read with `held`, after failing its first +// `failFirst` reads (all of them with `fail`), and a stand-in sleep that +// records each wait, returning its exit status, the outputs it wrote, the +// reads made and the waits. const ACTION = readFileSync(new URL("../../actions/upload-fail-index/action.yml", import.meta.url), "utf8"); -const splitNames = (names, held, { total, fail = false } = {}) => { +const splitNames = (names, held, { total, fail = false, failFirst = 0 } = {}) => { const script = /\n {6}run: \|\n([\s\S]*?)\n {4}- name: /.exec(ACTION)[1].replace(/^ {8}/gm, ""); const dir = tempDir("upload-fail-index-"); const bin = path.join(dir, "bin"); mkdirSync(bin); const body = JSON.stringify({ total_count: total ?? held.length, artifacts: held.map((name) => ({ name })) }); - writeFileSync(path.join(bin, "gh"), fail ? "#!/bin/sh\nexit 1\n" : `#!/bin/sh\ncat <<'JSON'\n${body}\nJSON\n`, { mode: 0o755 }); + const reads = path.join(dir, "reads"); + const waits = path.join(dir, "waits"); + writeFileSync(reads, ""); + writeFileSync(waits, ""); + const failing = fail ? 99 : failFirst; + writeFileSync( + path.join(bin, "gh"), + `#!/bin/sh\necho x >>'${reads}'\n[ "$(wc -l <'${reads}')" -gt ${failing} ] || exit 1\ncat <<'JSON'\n${body}\nJSON\n`, + { mode: 0o755 }, + ); + writeFileSync(path.join(bin, "sleep"), `#!/bin/sh\necho "$1" >>'${waits}'\n`, { mode: 0o755 }); const out = path.join(dir, "out"); writeFileSync(out, ""); const run = spawnSync("bash", ["--noprofile", "--norc", "-eo", "pipefail", "-c", script], { env: { PATH: `${bin}:${process.env.PATH}`, NAMES: names.join("\n"), RETENTION: "90", GITHUB_OUTPUT: out, REPO: "o/r", RUN_ID: "1", GH_TOKEN: "t" }, encoding: "utf8", }); - return { status: run.status, outputs: readFileSync(out, "utf8") }; + const lines = (file) => readFileSync(file, "utf8").split("\n").filter(Boolean); + return { status: run.status, outputs: readFileSync(out, "utf8"), reads: lines(reads).length, waits: lines(waits) }; }; test("a FAIL index never replaces or keeps a name its run already holds", () => { @@ -144,9 +157,21 @@ test("a FAIL index never replaces or keeps a name its run already holds", () => assert.doesNotMatch(rerun.outputs, /^n\d+=/m); // Another name the run holds (the verdict's own) uploads every index. assert.equal(splitNames(names, ["pr-verify-41-x-fail"]).outputs, `retention-days=90\n${names.map((n, i) => `n${i + 1}=${n}`).join("\n")}\n`); - // A read that fails or does not hold its count uploads nothing. - assert.notEqual(splitNames(names, [], { fail: true }).status, 0); - assert.notEqual(splitNames(names, [names[0]], { total: 2 }).status, 0); + // A read that fails or does not hold its count, three times, uploads nothing. + const failed = splitNames(names, [], { fail: true }); + assert.notEqual(failed.status, 0); + assert.deepEqual([failed.reads, failed.waits], [3, ["5", "10"]]); + assert.doesNotMatch(failed.outputs, /^n\d+=/m); + const short = splitNames(names, [names[0]], { total: 2 }); + assert.notEqual(short.status, 0); + assert.deepEqual([short.reads, short.waits], [3, ["5", "10"]]); + // A read that fails twice and then holds its count uploads every index. + const third = splitNames(names, [], { failFirst: 2 }); + assert.equal(third.status, 0); + assert.deepEqual([third.reads, third.waits], [3, ["5", "10"]]); + assert.equal(third.outputs, `retention-days=90\n${names.map((n, i) => `n${i + 1}=${n}`).join("\n")}\n`); + // A retried read that then shows a held name still fails the step. + assert.notEqual(splitNames(names, [names[1]], { failFirst: 1 }).status, 0); assert.doesNotMatch(ACTION, /overwrite/); assert.equal((ACTION.match(/uses: actions\/upload-artifact@/g) ?? []).length, 13); assert.equal((ACTION.match(/if: \$\{\{ steps\.slots\.outputs\.n\d+ != '' \}\}/g) ?? []).length, 13); diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py index 1fc82527bd1..a1e60e66e36 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py @@ -5675,6 +5675,7 @@ def _listing_fails(self) -> list[dict[str, Any]]: "id": item.get("id"), "name": name, "run": json_object(item.get("workflow_run")).get("id"), + "created_at": item.get("created_at"), } if len(items) < LISTING_PAGE_SIZE or page * LISTING_PAGE_SIZE >= count: break @@ -5726,6 +5727,29 @@ def _running_check(self) -> None: f"verifier run {run.get('id')} from before the cutover is {status}" ) + def _uploaded_since(self) -> None: + """Every listed FAIL was uploaded at or after `since`, or `since` is + too late: refuse, naming the earliest. A FAIL of a run created before + `since` but uploaded after it (a re-run, a late judgment) passes.""" + early = [] + for item in self.state["listed"]: + created = _when(item.get("created_at")) + if created is None: + raise _Unreadable( + f"listed FAIL {item.get('id')} of run {item.get('run')} " + "has no creation time" + ) + if created < self.since: + early.append((created, item)) + if early: + created, item = min(early, key=lambda e: e[0]) + raise _Unreadable( + f"{len(early)} listed FAIL(s) uploaded before --since " + f"{_stamp(self.since)}, the earliest artifact {item.get('id')} of run " + f"{item.get('run')} at {_stamp(created)}: lower --since to at most " + f"{_stamp(created)} and start a new --checkpoint" + ) + def build(self) -> dict[str, Any]: self.load() windows, runs = self.state["windows"], self.state["runs"] @@ -5759,10 +5783,13 @@ def build(self) -> dict[str, Any]: # its creation time, and a judgment can upload hours after it) is # read here, so the file is the union of the windows and the listing. self._running_check() - if "listed" not in self.state: + # A checkpoint whose listing kept no upload times lists again. + listed = self.state.get("listed") + if not is_json_array(listed) or any("created_at" not in i for i in listed): self.state["listed"] = self._listing_fails() self.save() self._running_check() + self._uploaded_since() added = [] for item in self.state["listed"]: run_id = item["run"] @@ -5819,8 +5846,9 @@ def build_legacy_fails( are read the same way. It keeps every unexpired FAIL-shaped artifact of a run of verifier origin that uploaded no FAIL index, with its verdict's head branch. A run not completed, a read that fails or does not hold its - count, an origin or head branch that cannot be read, and a listed FAIL - no run read holds raise `_Unreadable`; every finished window and run is + count, an origin or head branch that cannot be read, a listed FAIL + uploaded before `since` (its `since` is too late), and a listed FAIL no + run read holds raise `_Unreadable`; every finished window and run is kept in `checkpoint`, so a stopped build resumes where it stopped.""" if not since < cutover <= now: raise _Unreadable("the range must run from --since to a cutover in the past") diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_index.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_index.py index 4d61c852372..7b4d5f4cb17 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_index.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_index.py @@ -318,6 +318,12 @@ def test_own_verdicts_reads_the_pull_request_and_head_names(self) -> None: MERGE_SHA = "9" * 40 MERGED_AT = "2026-10-15T20:00:00Z" # its cutover, with the margin: 21:00 +SINCE = "2026-10-14T00:00:00Z" # the build's `--since` + + +def uploaded(items: list[dict[str, Any]], at: str) -> list[dict[str, Any]]: + """`items`, each uploaded at `at`.""" + return [{**item, "created_at": at} for item in items] class RunsGitHub: @@ -335,11 +341,12 @@ def __init__(self) -> None: 6, created_at="2026-10-15T03:00:00Z", path=".github/workflows/other.yml" ), ] + # Each uploaded within its run's hour. self.artifacts = { - 5: [artifact(named(EARLIER, "fail"), 5)], - 3: fail(PR, UNRELATED, 3), - 1: [artifact(named(HEAD, "pass"), 1)], - 6: [artifact(named(HEAD, "fail", 90), 6)], + 5: uploaded([artifact(named(EARLIER, "fail"), 5)], "2026-10-14T03:30:00Z"), + 3: uploaded(fail(PR, UNRELATED, 3), "2026-10-15T01:30:00Z"), + 1: uploaded([artifact(named(HEAD, "pass"), 1)], "2026-10-15T02:30:00Z"), + 6: uploaded([artifact(named(HEAD, "fail", 90), 6)], "2026-10-15T03:30:00Z"), } # Runs outside every window, read only by id. self.other_runs: dict[int, dict[str, Any]] = {} @@ -451,7 +458,7 @@ def run_build( "--repo", REPO, "--since", - "2026-10-14T00:00:00Z", + SINCE, "--merge-sha", MERGE_SHA, "--cutover", @@ -493,10 +500,13 @@ def test_the_last_window_reads_the_cutovers_own_second(self) -> None: def test_a_fail_of_a_run_from_before_since_is_added_through_the_listing( self, ) -> None: - # A re-run keeps its creation time, so no window holds run 8. + # A re-run keeps its creation time, so no window holds run 8; its + # FAIL uploaded after `--since`, so the listing adds it. fake = RunsGitHub() fake.other_runs[8] = run(8, created_at="2026-10-13T23:00:00Z") - fake.artifacts[8] = [artifact(named(IN_PR, "fail"), 8)] + fake.artifacts[8] = uploaded( + [artifact(named(IN_PR, "fail"), 8)], "2026-10-14T01:00:00Z" + ) fake.records[named(IN_PR, "fail")] = record(sha=IN_PR, verdict="fail") code, err = self.run_build(fake) self.assertEqual(code, 0, err) @@ -507,6 +517,55 @@ def test_a_fail_of_a_run_from_before_since_is_added_through_the_listing( sorted([named(EARLIER, "fail"), named(IN_PR, "fail")]), ) + def test_a_listed_fail_uploaded_before_since_refuses_naming_the_earliest( + self, + ) -> None: + # Runs 8 and 9 sit outside every window and their FAILs predate + # `--since`: `--since` is too late, so nothing is written. + fake = RunsGitHub() + for run_id, at in ((8, "2026-10-13T22:00:00Z"), (9, "2026-10-13T20:00:00Z")): + fake.other_runs[run_id] = run(run_id, created_at="2026-10-13T19:00:00Z") + fake.artifacts[run_id] = uploaded( + [artifact(named(IN_PR, "fail", 40 + run_id), run_id)], at + ) + code, err = self.run_build(fake) + self.assertEqual(code, 1, err) + self.assertIn( + "2 listed FAIL(s) uploaded before --since 2026-10-14T00:00:00Z, the " + "earliest artifact 90 of run 9 at 2026-10-13T20:00:00Z: lower --since " + "to at most 2026-10-13T20:00:00Z", + err, + ) + self.assertFalse(self.out.exists()) + # Nothing outside the windows was read for them. + self.assertNotIn(f"repos/{REPO}/actions/runs/9", fake.calls) + + def test_a_listed_fail_with_no_creation_time_refuses(self) -> None: + fake = RunsGitHub() + fake.artifacts[5] = uploaded(fake.artifacts[5], None) # type: ignore[arg-type] + code, err = self.run_build(fake) + self.assertEqual(code, 1, err) + self.assertIn("listed FAIL 50 of run 5 has no creation time", err) + + def test_a_checkpoint_listing_without_upload_times_is_listed_again(self) -> None: + # A checkpoint from before the listing kept `created_at`. + fake = RunsGitHub() + self.assertEqual(self.run_build(fake)[0], 0) + saved = json.loads(self.checkpoint.read_text()) + saved["listed"] = [ + {k: v for k, v in item.items() if k != "created_at"} + for item in saved["listed"] + ] + self.checkpoint.write_text(json.dumps(saved)) + resumed = RunsGitHub() + resumed.artifacts[5] = uploaded(resumed.artifacts[5], "2026-10-13T12:00:00Z") + code, err = self.run_build(resumed) + self.assertEqual(code, 1, err) + self.assertIn("the earliest artifact 50 of run 5", err) + self.assertIn( + f"repos/{REPO}/actions/artifacts?per_page=100&page=1", resumed.calls + ) + def test_a_run_created_before_since_and_updated_after_it_is_read(self) -> None: # Run 9 was re-run after `--since`; run 7 was not, so it is not read. fake = RunsGitHub() @@ -518,7 +577,9 @@ def test_a_run_created_before_since_and_updated_after_it_is_read(self) -> None: 7, created_at="2026-10-02T00:00:00Z", updated_at="2026-10-02T01:00:00Z" ), ] - fake.artifacts[9] = [artifact(named(IN_PR, "fail"), 9)] + fake.artifacts[9] = uploaded( + [artifact(named(IN_PR, "fail"), 9)], "2026-10-14T05:00:00Z" + ) fake.records[named(IN_PR, "fail")] = record(sha=IN_PR, verdict="fail") code, err = self.run_build(fake) self.assertEqual(code, 0, err) @@ -560,7 +621,7 @@ def test_a_verifier_run_from_before_the_cutover_still_running_refuses(self) -> N def test_a_listed_fail_no_run_read_holds_refuses(self) -> None: fake = RunsGitHub() - ghost = artifact(named(IN_PR, "fail"), 5) + (ghost,) = uploaded([artifact(named(IN_PR, "fail"), 5)], "2026-10-14T04:00:00Z") ghost["id"] = 51 fake.listed_only.append(ghost) code, err = self.run_build(fake) From b4e248a02c1e45b5357b991a6187bc50489798a4 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 2 Nov 2026 08:57:03 -0500 Subject: [PATCH 2/2] fix(source-control): keep the checkpoint path in the pre-since refusal A new --checkpoint path would start a fresh .window file and reset the per-window call cap; tell the operator to remove the checkpoint file and run again instead. Co-Authored-By: Claude Opus 5.5 --- .../skills/babysit-prs/scripts/babysit_verify_evidence.py | 2 +- .../babysit-prs/scripts/tests/test_babysit_verify_index.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py index a1e60e66e36..25155328de4 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py @@ -5747,7 +5747,7 @@ def _uploaded_since(self) -> None: f"{len(early)} listed FAIL(s) uploaded before --since " f"{_stamp(self.since)}, the earliest artifact {item.get('id')} of run " f"{item.get('run')} at {_stamp(created)}: lower --since to at most " - f"{_stamp(created)} and start a new --checkpoint" + f"{_stamp(created)}, remove the checkpoint file and run again" ) def build(self) -> dict[str, Any]: diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_index.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_index.py index 7b4d5f4cb17..7ffec392d66 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_index.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_verify_index.py @@ -533,7 +533,8 @@ def test_a_listed_fail_uploaded_before_since_refuses_naming_the_earliest( self.assertIn( "2 listed FAIL(s) uploaded before --since 2026-10-14T00:00:00Z, the " "earliest artifact 90 of run 9 at 2026-10-13T20:00:00Z: lower --since " - "to at most 2026-10-13T20:00:00Z", + "to at most 2026-10-13T20:00:00Z, remove the checkpoint file and run " + "again", err, ) self.assertFalse(self.out.exists())