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-verdict-reader-per-run-3b8d71e0.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
bump: patch
---

### Fixed

- In the merge queue, ci-status titles every verifier hold's error annotation `pr-verify hold` (merge-holds.mjs `VERIFIER_TITLE`), so a `verify:unreadable` refusal no longer reads as a body hold of the step it shares. The verdict reader's short incremental artifact listing is read again after a wait before it pages every artifact, and the cold-read headroom counts the near-expiry caps once for the incremental reads and once for the whole listings' top-ups (1,681 to 1,739 at 7,522 artifacts).
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
bump: patch
---

### Fixed

- **babysit-prs**: the pr-verify verdict reader reads its incremental artifact listing again after a wait when the pages near expiry leave the count short, instead of paging the whole repository at once. That short count is a page-boundary skip in the fresh pages, which a second read restores. It reads again only while the near-expiry caps left cover another such read. The whole listings' top-ups now get those caps again, so incremental reads that spent them do not starve a top-up.
19 changes: 15 additions & 4 deletions .github/scripts/pr-merge/merge-holds.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -1403,6 +1403,11 @@ export function verdictHold(records, repo, branch = "main") {
return pass ? null : "pr-verify has not passed this head";
}

// The annotation title of a verifier hold (verdictHold, linkHold,
// evidenceHold), so a reader of the job's annotations tells a pr-verify
// refusal from a body hold, member veto or hold path in the same step.
export const VERIFIER_TITLE = "pr-verify hold";

// The refusals of babysit_verify_evidence.py a pass decides: verdictHold and
// linkHold judge the pass here, and an approval lifts them as it lifts this.
const PASS_REFUSALS = Object.freeze(["verify:missing", "verify:spec-changed", "verify:untrusted-spec"]);
Expand Down Expand Up @@ -1673,6 +1678,9 @@ export async function main(argv = process.argv.slice(2), { env = process.env, gi
if (widened && lifters.length) {
console.log(`#${number}: ${lifters.join(", ")} ${how}, which lifts its trusted-actor list hold (${escape(widened)}).`);
}
// The verifier's holds, recorded under their own annotation title
// (VERIFIER_TITLE): they share this step with the body and member holds.
const verifier = [];
if (verdictsFile) {
const unverified = verdictHold(lines(verdictsFile).map((line) => JSON.parse(line)), env.GITHUB_REPOSITORY);
// An approval lifts the verifier hold whole, the link with it.
Expand All @@ -1682,11 +1690,11 @@ export async function main(argv = process.argv.slice(2), { env = process.env, gi
} else if (!unverified && approved.length === 0) {
// pr-verify skips a head it already passed (resolve-pr.mjs `judged`),
// so only a new head or an approval recovers it.
reasons.push(`${relinked}; pr-verify does not judge this head again, so push a new head for it to judge, after which enable-auto-merge arms it again, or an approving review of this head by a person on the trusted-actor list lifts it, then re-queue`);
verifier.push(`${relinked}; pr-verify does not judge this head again, so push a new head for it to judge, after which enable-auto-merge arms it again, or an approving review of this head by a person on the trusted-actor list lifts it, then re-queue`);
} else if (approved.length) {
console.log(`#${number}: no verifier hold: ${approved.join(", ")} approved this head, which lifts '${unverified}'.`);
} else {
reasons.push(
verifier.push(
`${unverified}; wait for pr-verify to pass it, after which enable-auto-merge arms it again, or an approving review of this head by a person on the trusted-actor list lifts it, then re-queue`,
);
}
Expand All @@ -1697,16 +1705,19 @@ export async function main(argv = process.argv.slice(2), { env = process.env, gi
} else if (approved.length) {
console.log(`#${number}: no pr-verify FAIL hold: ${approved.join(", ")} approved this head, which lifts '${escape(failed)}'.`);
} else {
reasons.push(`${failed}; push a new head for pr-verify to judge, after which enable-auto-merge arms it again, or an approving review of this head by a person on the trusted-actor list lifts it, then re-queue`);
verifier.push(`${failed}; push a new head for pr-verify to judge, after which enable-auto-merge arms it again, or an approving review of this head by a person on the trusted-actor list lifts it, then re-queue`);
}
}
for (const reason of reasons) {
console.log(`::error::#${number}: ${escape(reason)}.`);
}
for (const reason of verifier) {
console.log(`::error title=${VERIFIER_TITLE}::#${number}: ${escape(reason)}.`);
}
if (!reasons.length) {
console.log(`#${number}: no body hold, no member veto and no hold path.`);
}
return reasons.length ? 1 : 0;
return reasons.length || verifier.length ? 1 : 0;
} catch (error) {
console.log(`::error::merge-holds: ${String(error?.message ?? error).replaceAll(/[\r\n%]/g, " ")}`);
return 2;
Expand Down
14 changes: 12 additions & 2 deletions .github/scripts/pr-merge/merge-holds.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -973,8 +973,18 @@ test("in the merge queue, a reader that refuses only on the pass leaves that to
test("in the merge queue, a FAIL the reader counts holds a passed head", async () => {
const { code, out } = await queued(report("verify:fail", { detail: "fails at the head" }));
assert.equal(code, 1, out);
assert.match(out, /::error::#99: pr-verify's reader refuses this head \(verify:fail: fails at the head\); push a new head/);
assert.match(out, /::error title=pr-verify hold::#99: pr-verify's reader refuses this head \(verify:fail: fails at the head\); push a new head/);
assert.doesNotMatch(out, /no pr-verify FAIL hold/);
// A verifier hold is not a body hold: no untitled error line.
assert.doesNotMatch(out, /^::error::/m);
assert.match(out, /^#99: no body hold, no member veto and no hold path\.$/m);
});

test("in the merge queue, the reader's verify:unreadable is a pr-verify hold beside a body hold", async () => {
const { code, out } = await queued(report("verify:unreadable", { detail: "1 whole listing never held" }));
assert.equal(code, 1, out);
assert.match(out, /^::error title=pr-verify hold::#99: pr-verify's reader refuses this head \(verify:unreadable: 1 whole listing never held\)/m);
assert.doesNotMatch(out, /^::error::/m);
});

test("in the merge queue, a trusted approval of the head lifts a FAIL hold", async () => {
Expand All @@ -993,7 +1003,7 @@ test("in the merge queue, a missing or unreadable reader report holds", async ()
for (const evidence of [undefined, "", "not json", JSON.stringify(report(null, { head: OLD }))]) {
const { code, out } = await queued(evidence);
assert.equal(code, 1, `${evidence}: ${out}`);
assert.match(out, /::error::#99: its pr-verify FAILs could not be read/, String(evidence));
assert.match(out, /::error title=pr-verify hold::#99: its pr-verify FAILs could not be read/, String(evidence));
}
});

Expand Down
20 changes: 12 additions & 8 deletions .github/workflows/pr-require-checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6137,7 +6137,10 @@ jobs:
# evidenceHold, which holds on every refusal but the three a pass decides
# (those stay with verdictHold and linkHold): every FAIL class,
# verify:unreadable and verify:too-old. The same approval of the head
# lifts it. The reader reads with this job's token, which holds no
# lifts it. Every verifier hold's error annotation is titled
# `pr-verify hold` (merge-holds.mjs VERIFIER_TITLE), so a reader of this
# step's annotations tells it from a body hold, member veto or hold
# path. The reader reads with this job's token, which holds no
# issues scope: on this public repository its issue reads answer as
# public reads, and one that does not read refuses verify:unreadable,
# which holds. Its gh calls go to the gh binary, outside this step's
Expand Down Expand Up @@ -6455,15 +6458,16 @@ jobs:
# and LISTING_READS (3) whole listings, each up to
# 1 + len(LISTING_WAITS) (3) attempts that page from the first,
# and a top-up after each (LISTING_TOP_UP_PAGES, 7), plus at most
# NEAR_EXPIRY_PAGE_CAP (24) pages near expiry read again when a
# count is over, and NEAR_EXPIRY_ID_READ_CAP (34) artifacts read
# by id that those pages omit. A whole
# NEAR_EXPIRY_PAGE_CAP (24) pages near expiry read again and
# NEAR_EXPIRY_ID_READ_CAP (34) artifacts read by id that those
# pages omit, once for the incremental reads and once for the
# whole listings' top-ups. A whole
# listing pages ceil(total_count / 100), plus one for uploads while
# it pages, so a cold read costs at most
# 3 x FAILs + 3 x 13 + 1 + 3 x (3 x pages + 7) + 24 + 34, plus the
# 3 x FAILs + 3 x 13 + 1 + 3 x (3 x pages + 7) + 2 x (24 + 34), plus the
# pages of this step's own listing, which every entry the queue
# builds at once also reads, cold_cost below: 1,681 at 7,522
# artifacts and 264 FAILs, 2,079 at 10,450 and 300.
# builds at once also reads, cold_cost below: 1,739 at 7,522
# artifacts and 264 FAILs, 2,137 at 10,450 and 300.
# test_babysit_verify_evidence runs these lines against those caps
# at several sizes. It covers this path only: an
# entry with a carried run cache but no carried listing pays the
Expand Down Expand Up @@ -6505,7 +6509,7 @@ jobs:
exit 1
fi
listing_pages=$(((total + 99) / 100 + 1))
cold_cost=$((listing_pages + 3 * fails + 3 * 13 + 1 + 3 * (3 * listing_pages + 7) + 24 + 34))
cold_cost=$((listing_pages + 3 * fails + 3 * 13 + 1 + 3 * (3 * listing_pages + 7) + 2 * (24 + 34)))
rate=$(read_api "the GITHUB_TOKEN rate limit" rate_limit) || rate=
if ! remaining=$(jq -er '.resources.core.remaining | numbers | floor' <<<"$rate" 2>/dev/null) ||
! limit=$(jq -er '.resources.core.limit | numbers | floor' <<<"$rate" 2>/dev/null) ||
Expand Down
4 changes: 3 additions & 1 deletion docs/conventions/pr-pipeline/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -2671,7 +2671,9 @@ kept only when it holds exactly that page's `total_count`. A count above it firs
around kept entries near expiry, up to a page cap, since GitHub can delete an artifact up to an hour
before its `expires_at` (`EARLY_DELETION_LEAD`, `NEAR_EXPIRY_PAGE_CAP`). Those pages can skip an
entry at a boundary, so an entry they omit is dropped only when its own read answers 404, up to an
id-read cap (`NEAR_EXPIRY_ID_READ_CAP`). Any count still other than it, a missing, older or
id-read cap (`NEAR_EXPIRY_ID_READ_CAP`); the whole listings' top-ups get both caps again. A count
those pages leave short is a skip in the fresh pages and is read again after a wait while the caps
left cover another such read. Any count still other than it, a missing, older or
malformed base, or any failed read lists every page, so a carried listing that leaves out a FAIL
lists every page too (the rule and its basis: `babysit_verify_evidence.py`, "Carried reads").
A whole listing is held to the same count: deduplicated by id, added to the whole listings read
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,11 +157,15 @@
around the kept entries near expiry are read again and decide every id they
span, an id they omit by its own read (`Reader._near_expiry`), at a few pages
a group whatever the repository's total and at most `NEAR_EXPIRY_PAGE_CAP` a
listing. They are read on an exact count too: an entry deleted early can stand
listing's incremental reads, and as many again for its whole listings'
top-ups. They are read on an exact count too: an entry deleted early can stand
in for a FAIL the pages skipped. An over-count those pages do not settle goes
to the one whole listing, an exact count they do not settle fails the
listing (as transient when only `NEAR_EXPIRY_PAGE_CAP` or
`NEAR_EXPIRY_ID_READ_CAP` left it so, so a later Reader may list), a count they leave short goes to a whole listing with no wait, and a
`NEAR_EXPIRY_ID_READ_CAP` left it so, so a later Reader may list), a count
they leave short is a page-boundary skip in the fresh pages and is read
again after a wait while the caps left cover another such read (else a whole
listing), and a
page or id read still failing on a 5xx after `LISTING_WAITS` fails it as
transient. Such a listing
could hide a FAIL by leaving it out, so it is kept only when it holds exactly
Expand Down Expand Up @@ -411,22 +415,23 @@ def verdict_name(name: str) -> re.Match[str] | None:
# line whose `listed` exceeds `total_count`. Twice the largest lead seen
# (judgment on that data).
EARLY_DELETION_LEAD = timedelta(hours=2)
# The most pages `Reader._near_expiry` reads for one listing, its incremental
# read and top-up together; past it an over-count stands and goes to the one
# The most pages `Reader._near_expiry` reads for one listing's incremental
# reads, and again for its whole listings' top-ups (`_complete_listing` resets
# both caps); past it an over-count stands and goes to the one
# whole listing, and an exact count fails the listing. On the 2026-11-01
# carry behind #11903's refusal it read 12 pages; twice that (judgment on
# that data), at `LISTING_PAGE_SECONDS` a page, stays well inside
# verify-cap.mjs's 120 s limit and a fifth of that day's 133-page whole
# listing.
NEAR_EXPIRY_PAGE_CAP = 24
# The most artifacts `Reader._near_expiry` reads by id for one listing, its
# incremental read and top-up together: each id its pages did not show is
# The most artifacts `Reader._near_expiry` reads by id for one listing's
# incremental reads, and again for its top-ups: each id its pages did not show is
# dropped only on a 404 or an expired record. It drops at most the listing's
# surplus over the live count, which was 31 to 34 in the three over-counts
# behind #11903's refusal; the largest (judgment on that data). Past it an
# over-count stands and an exact count fails the listing. Both caps reset
# with each listing the Reader reads, as after `refresh()`;
# pr-require-checks.yml's cold_cost counts them once.
# with each listing the Reader reads, as after `refresh()`, and again before
# its whole listings; pr-require-checks.yml's cold_cost counts them twice.
NEAR_EXPIRY_ID_READ_CAP = 34
# The `cause` a `LISTING_MISS_PREFIX` line names when the caps above alone
# leave an exact count unsettled: refused, but not noted for the job.
Expand Down Expand Up @@ -463,10 +468,10 @@ def no_incremental_waits() -> bool:
class _CountMissed:
"""`Reader._incremental`'s answer when its pages read whole but do not
hold the live count: `COUNT_MISSED` (fewer), the one case read again on
the same base, or `COUNT_OVER` (more, or any count once the pages near
expiry were read again, except a top-up's exact count left short), which
goes to the whole listings, since no wait removes an entry GitHub no
longer counts."""
the same base, or `COUNT_OVER` (more, a top-up's over-count left short,
or an incremental count left short once the near-expiry caps cannot
cover another read), which goes to the whole listings, since no wait
removes an entry GitHub no longer counts."""


# Fewer than the live count: the pages skipped an artifact, which another
Expand Down Expand Up @@ -1483,6 +1488,9 @@ def _complete_listing(self) -> list[dict[str, Any]]:
it when the caller's limit cut the whole listings' timeout and too
little of it is left for another wait (`LISTING_WAITS`)."""
self._whole_paged = False
# The whole listings' top-ups get near-expiry caps of their own, so
# incremental reads that spent theirs leave the top-ups theirs.
self._near_expiry_reads = self._artifact_reads = 0
cap, bound = listing_cap()
callers = cap < LISTING_TIMEOUT_SECONDS
if callers and cap <= 0:
Expand Down Expand Up @@ -1604,12 +1612,12 @@ def _incremental(
gives None, since an entry deleted early may stand in for a FAIL. The
result must hold exactly the first page's `total_count` artifacts,
GitHub's live count: fewer gives `COUNT_MISSED`, which
`_current_listing` reads again on the same base after a wait, and
more, or any count once the pages near expiry were read (a top-up's
exact count left short aside, which another whole listing can
restore), gives `COUNT_OVER`, which goes to the whole listings (one,
after an over-count), since no wait removes an entry GitHub no
longer counts; either prints one
`_current_listing` reads again on the same base after a wait (after
the pages near expiry were read, only while the caps left cover
another such read), and more, or a top-up's over-count left short,
gives `COUNT_OVER`, which goes to the whole listings (one, after an
over-count), since no wait removes an entry GitHub no longer counts;
either prints one
`LISTING_MISS_PREFIX` line on stderr naming `stage`, the counts and
the pages read. No base, an entry or page of another shape, a missing
`Date`, or any failed read gives None (`_page_transient` says whether
Expand All @@ -1620,6 +1628,8 @@ def _incremental(
and sets `_near_expiry_spent` when only the caps, or the deadline
running out during those reads, left it so."""
self._page_transient = self._near_expiry_spent = False
spent = (self._near_expiry_reads, self._artifact_reads)
started = _listing_clock()
if base is not None and not all(_listed_entry(a) for a in base[1]):
return None
base = base or self._incremental_base()
Expand Down Expand Up @@ -1726,11 +1736,31 @@ def _incremental(
file=sys.stderr,
flush=True,
)
# Once the pages near expiry were read, no wait on this base settles
# the count: the base still holds what they dropped. A top-up's exact
# count left short goes to another whole listing, which can restore a
# skip outside those pages.
if over or len(listing) > total or (near and stage != "top-up"):
if len(listing) > total:
return COUNT_OVER
# Short once the pages near expiry were read: they dropped only ids
# GitHub answered gone, so the fresh pages skipped an artifact at a
# page boundary (#10486), which a read after a wait can restore, as
# for any short count. It is read again only while the caps left
# cover another read like this one, and a caller's limit still fits
# that read, the longest wait and one whole listing; else a whole
# listing. A top-up's exact count left short goes to another whole
# listing, which can restore a skip outside those pages.
if stage == "incremental" and (over or near):
pages = self._near_expiry_reads - spent[0]
ids = self._artifact_reads - spent[1]
cap = listing_cap()[0]
took = _listing_clock() - started
if (
NEAR_EXPIRY_PAGE_CAP - self._near_expiry_reads < pages
or NEAR_EXPIRY_ID_READ_CAP - self._artifact_reads < ids
or (
cap < LISTING_TIMEOUT_SECONDS
and cap < took + max(INCREMENTAL_WAITS) + listing_seconds(total)
)
):
return COUNT_OVER
elif over:
return COUNT_OVER
return COUNT_MISSED

Expand Down
Loading
Loading