Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changes/pr-pipeline/fix-legacy-fails-since-guard-4e8b0d15.md
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -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.
27 changes: 18 additions & 9 deletions .github/actions/upload-fail-index/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
41 changes: 33 additions & 8 deletions .github/scripts/pr-verify/workflow.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)}, remove the checkpoint file and run again"
)

def build(self) -> dict[str, Any]:
self.load()
windows, runs = self.state["windows"], self.state["runs"]
Expand Down Expand Up @@ -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"]
Expand Down Expand Up @@ -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")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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]] = {}
Expand Down Expand Up @@ -451,7 +458,7 @@ def run_build(
"--repo",
REPO,
"--since",
"2026-10-14T00:00:00Z",
SINCE,
"--merge-sha",
MERGE_SHA,
"--cutover",
Expand Down Expand Up @@ -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)
Expand All @@ -507,6 +517,56 @@ 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, remove the checkpoint file and run "
"again",
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()
Expand All @@ -518,7 +578,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)
Expand Down Expand Up @@ -560,7 +622,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)
Expand Down
Loading